[AlexKantor87]

Part of #1283.

kosli attest pullrequest bitbucket and kosli assert pullrequest bitbucket crashed when Bitbucket sent a field as null where the code expected a value. A crash prints a Go stack trace that doesn't say which field caused it. For now we treat every field as optional (#1284 will decide which fields are required).

What changed

• Safe readers. str, obj, boolean and objects read a field and give an empty value when it's missing, null or a different type. They replace every unchecked read in bitbucket.go, on both the V1 path (assert) and the V2 path (attest).
• Unreadable list entries still fail. A null item in values or participants returns an error that names the list and index. Skipping it could hide a "changes requested" review.
• Empty timestamps are 0, not an error.
• Errors where we can't carry on. A PR with no links.self.href (the next request's URL) or no id (needed to fetch commits) returns a readable error. So does a commit with no hash. An unreadable list entry's error names its commit or PR.
• Commit paging stops. On main it loops forever if Bitbucket's next link points back to a page already read. It now stops after 1,000 pages with an error.
• Participants with no name (a deleted account) are never approvers. In reviews only a change request is kept, as on main, because dropping it would read as no objection.

Tests

odd_replies_test.go sends realistic replies through V1 and V2 once per field, with that field made null. It checks there's never a crash, and never an error except the cases above. When a participant has no name, it checks that only that person's entries change. Before the fix, 62 cases crashed.

Full CLI suite against a local server: 3,997 tests passed.

🤖 Generated with Claude Code

[claude]

Claude finished @AlexKantor87's task in 1m 33s —— View job

---

Follow-up review: BEFORE 224e2237 → AFTER b52ca3a9

• Fetch diff between BEFORE and AFTER
• Fetch previous review findings
• Check status of each previous finding touched by new commits
• Review new commits for bugs/regressions in changed lines
• Post inline comments for new issues (none needed)
• Post summary comment

New commits (e20143ff, c2eb4701, b52ca3a9) only touch bitbucket.go's objects() helper (added an owner parameter so null-entry errors name the commit/PR/page they belong to) and getPullRequestCommitsFromBitbucket (a commit with no hash now errors instead of being recorded with an empty SHA, numbered across pages), plus matching test updates in odd_replies_test.go.

Status of previous findings (none of their locations were touched by this push — re-check, not re-review):

• bitbucket.go:354 (empty date → timestamp: 0) — not addressed, by design: deferred to Decide which PR evidence fields are required for each provider #1284. Thread already resolved on GitHub.
• bitbucket.go (paging continues on missing values instead of breaking) — resolved by 747773f2's page cap. Thread already resolved on GitHub.
• odd_replies_test.go:367 (regexp compiled per subtest) — declined as a style nit. Thread already resolved on GitHub.

mbevc1's two review comments:

• "A commit with no hash is recorded with an empty SHA" — addressed by c2eb4701/b52ca3a9: a commit with hash: null or "" now returns fmt.Errorf("commit %d for pull request %d has no hash", ...), where the index counts across pages (confirmed by the new TestOddReplies_CommitWithNoHashFails, which checks index 0 on page 1 and 1 on page 2). oddAllowedErrors was also extended for both commit pages.
• "Approver and review can disagree when state is null" — not addressed, out of scope here: deferred to Decide which PR evidence fields are required for each provider #1284 as noted elsewhere in this PR's thread.

New issues in the changed lines: none. All three objects() call sites were updated consistently with the new owner parameter, there are no other callers in the repo, and the new tests' expected error substrings (oddListOwners) match the new error format. I confirmed this by static reading only — I don't have permission to run go build/go test in this environment, so this isn't verified by an actual test run.

0 new inline comments.
(branch)

[AlexKantor87]

This branch now includes #1291's branch, so the Go 1.26.10 change clears the Snyk check here. That's why go.mod, Dockerfile, .github/dependabot.yml and internal/version/toolchain_test.go show in this diff. They're identical to #1291 and drop out of the diff once it merges. Please review those files on #1291, not here.

no search: answers no review finding; it brings in #1291's reviewed files unchanged (git diff origin/claude/go-1.26.10 HEAD on those four files is empty).

[claude]

Follow-up review: BEFORE 5b6d96b..AFTER 507500b

The new commits (9a1287f8, 507500b3) only merge in origin/main, which already includes #1291. The only line-level change in git diff 5b6d96b..507500b is .github/dependabot.yml's schedule tweak — identical to #1291 and out of scope here per the PR description's note to review that on #1291 instead. No bitbucket-related files were touched by this push.

Status of previous findings (none of these locations were touched by the new commits, so this is a re-check, not a re-review):

• bitbucket.go:338 (possible infinite loop if values is missing/non-list) — resolved, fixed by 747773f2 (defaultMaxPages cap, confirmed present in the code). Thread already resolved.
• bitbucket.go:345 (empty date → timestamp: 0) — not addressed, by design: deferred to Decide which PR evidence fields are required for each provider #1284 per author reply. Thread already resolved (acknowledged, no code change expected here).
• odd_replies_test.go:313 (regexp compiled per subtest) — not addressed, declined as a style nit per author reply. Thread already resolved.

New issues in the changed lines: none. The only diff is the dependabot schedule change, which is trivial and already covered by #1291.

No new inline comments needed.

View job run

[claude]

Follow-up review: BEFORE 507500b → AFTER 224e223

New commits 423653d5 (fix) and 224e2237 (test) touch only bitbucketReviews() in internal/bitbucket/bitbucket.go (lines ~280–301) and its tests — a different location from all three previously-raised findings.

Status of previous findings (none of their locations were touched by this push, so this is a re-check, not a re-review):

• bitbucket.go:350 (empty date → timestamp: 0) — already resolved/acknowledged (deferred to Decide which PR evidence fields are required for each provider #1284). Thread confirmed resolved.
• bitbucket.go:343 (paging continues on missing values instead of breaking) — already resolved by 747773f2's page cap. Thread confirmed resolved.
• odd_replies_test.go:357 (regexp compiled per subtest) — already declined as a style nit by the author. Thread confirmed resolved.

New issues in the changed lines: none. The fix correctly keeps nameless changes_requested participants in Reviews (so a hidden "changes requested" from a deleted account is never dropped) while excluding nameless approvals and nameless comments from both Approvers and Reviews. odd_replies_test.go's extended oddNameless cases and reviews_test.go's updated fixture correctly exercise all three nameless states (approval, changes_requested, comment) and match the implementation.

Note: I wasn't able to run go build/go test in this environment (commands beyond git require approval not available here) — review is based on static reading of the diff and existing test fixtures.

0 new inline comments.

View job run

[mbevc1]

1. A commit with no hash is recorded with an empty SHA.
   "hash": null gives no error, and the payload carries "sha1": "". main panics here, so erroring breaks nothing that works today. GitLab and Azure both error on a commit without an id.
   Suggestion: error when hash is missing, and add pullrequest_commits_* values[0].hash to oddAllowedErrors.

2. Approver and review can disagree when state is null (for Decide which PR evidence fields are required for each provider #1284).
   A participant with approved: true, state: null is an approver (V2 with an empty State), but COMMENTED in reviews. Approvers come from approved, reviews from state. On main, V1 already counts this participant as an approver, and V2 panicked.

[AlexKantor87]

Thanks @mbevc1.

1. Commit with no hash. Fixed in c2eb470. It now returns an error naming the commit's index and the pull request. b52ca3a counts that index across pages, so one index always means one commit. I checked the other providers for the same gap. GitLab (on main since #1287) and Azure (#1288) already fail on a commit with no id. GitHub's schema says a commit's oid is never null: GitHub nulls the whole commit instead, which already fails.

2. Approver and review disagreeing. Added to #1284. The same thing happens on Azure (a vote of 5 is APPROVED in reviews but not an approver) and on GitLab (an approver who also requested changes).

Same sweep, after your #1287 nit: errors for an unreadable list entry now name their commit or pull request, plus the page for commit pages (e20143f).

search: grep -nE 'objects(|has no hash' internal/bitbucket/bitbucket.go

141: objects(responseData, "values", "commit "+commit)
187: objects(responseData, "participants", "pull request "+prApiUrl)
344: objects(responseData, "values", fmt.Sprintf("pull request %d, page %d", prID, pages+1))
351: fmt.Errorf("commit %d for pull request %d has no hash", len(allCommits), prID)

verified: sed -n '349,352p' internal/bitbucket/bitbucket.go

		for _, commit := range commits {
			if str(commit, "hash") == "" {
				return nil, fmt.Errorf("commit %d for pull request %d has no hash", len(allCommits), prID)
			}

mutation: drop the hash check -> TestOddReplies_CommitWithNoHashFails red (2 cases)
mutation: drop the owner from the objects() error -> TestOddReplies red (8 cases)