Skip to content

feat!: surface unresolved PR comments and tasks to agents - #308

Open
vriesdemichael wants to merge 8 commits into
mainfrom
feat/pr-review-visibility
Open

feat!: surface unresolved PR comments and tasks to agents#308
vriesdemichael wants to merge 8 commits into
mainfrom
feat/pr-review-visibility

Conversation

@vriesdemichael

@vriesdemichael vriesdemichael commented Aug 10, 2026

Copy link
Copy Markdown
Owner

Closes #306.

Eight commits, stacked. The first three are the feature; the rest are things it uncovered that had to be fixed for it to work correctly against a real server.


1. Unresolved comments and tasks (issue #306)

An agent working a PR with reviewers had no way to see anything was waiting on it, and had to be told that comments and tasks were two separate commands.

bb pr get now carries a review_summary with a single action_required flag:

#42	OPEN	feature/my-work -> main	Add payment retries
Reviewers: 2
Open items: 3 unresolved comments, 1 open task
Needs work: carol

bb pr comment list returns comment threads and tasks together, unresolved first — Bitbucket models a task as a blocker comment, so one call now answers what took two. New --unresolved, --state, --tasks-only, --with-replies. bb pr list shows per-PR counts.

Counts are absent rather than zero when they could not be measured, with counts_source naming the source. Reporting an unmeasured count as zero would make a PR with open feedback look clean — the exact failure this exists to prevent.

Also: MCP tools carry the summary, published output schemas for both commands (validated against real command output across 8 invocation variants), and a rewritten SKILL.md.

⚠️ Breaking: bb pr comment list --json emits the summarised thread view instead of raw comment objects, which embedded an entire pull request per comment. --full restores the old payload; bb pr comment get is unchanged.

2. Two Bitbucket bugs found by verifying against a real server

OPENAPI-007 — the activity timeline serialises an inline comment's anchor.path as a string while the spec models an object, so one inline comment made the whole page fail to decode. bb pr comment list has been returning nothing for any PR with inline review comments.

OPENAPI-008 — a retired endpoint and a missing resource are both 404s but differ in body shape. Separating them lets the summary degrade only for features a server lacks and report everything else.

3. The CI gap

bb pr task * calls an endpoint Atlassian removed in 8.0. It 404s against this repo's own stack and CI was green, because the live tests hid it:

if strings.Contains(strings.ToLower(err.Error()), "not_found") {
    t.Skipf("pull request task endpoint unavailable in live environment: %v", err)
}

A skipped test looks identical to a passing one. tools/cli-live-coverage reports a command whose only live coverage can skip itself on error as masked, not covered. Current: 53% asserted, 4 masked (exactly the broken pr task * set), 101 never invoked. Baseline + ratchet, static analysis, no live infra needed.

4. The generated client was not reproducible

Collision suffixes were assigned while ranging over a Go map, so the same spec produced a different operationId→endpoint mapping every run — whether Get3WithResponse hit /basicauth/latest/config or a pull-request endpoint was decided by chance. Invisible because models:verify and client:verify were never run by CI. Both now are.

5. Vendoring the Bitbucket 10.2 spec

The reference was 9.4 while the stack and live suite run 10.2 — that drift is why the retired endpoint survived. Almost entirely additive, but 10.2 raises required-field counts from 27 to 117, so codegen emits values where 9.4 emitted pointers; ~30 call sites adjusted. One behavioural change the compiler caught: fork-sync moved GetStatus2GetStatus.

Per ADR 042 this states no supported version — the spec is provenance for the generated client, not a claim about which servers work. defaultBitbucketVersionTarget stays empty and ADR 018 stays superseded.

⚠️ Breaking: generated models expose required fields as values, and RestReviewerGroup drops displayName. Consumers of the generated packages must adjust. The bb surface and JSON output are unchanged.

6. Tests must not reconfigure the repo they run in

Backend.Clone persists http.extraHeader, and a test that once pointed it at the working copy wrote this into this repo's .git/config:

http.extraheader = Authorization: Basic <base64 of dummy-user:dummy-password>
user.name        = Test User
user.email       = test@example.local

An unscoped http.extraHeader beats any credential helper, so every git push sent dummy-user:dummy-password and was rejected with "Password authentication is not supported for Git operations" — which reads like a bad token and sends you hunting in the wrong place. The identity override authored real commits as Test User <test@example.local>, including some already on main.

internal/git/gittest snapshots repository-scoped config before each package's tests and fails on any change, naming the keys. Before/after comparison, so a developer's own settings are invisible to it. Its git calls use execgit.ScopeFreeEnv so it inspects the right repository under a hook.


Verification

  • Full unit suite and full live suite green against the container stack
  • models:verify / client:verify clean — generated code reproduces byte-identically
  • Patch coverage 95%, combined scoped 91%
  • Docs build strictly; decisions validate; all generated artifacts regenerated

Note

Two feat!: commits, so merging cuts v2.0.0. Retitle them if you'd rather not.

docker/README.md still says 9.4.16 while the stack runs 10.2 — under ADR 042 the fix is to delete that line, not update it, so I left it alone.

🤖 Generated with Claude Code

@vriesdemichael
vriesdemichael force-pushed the feat/pr-review-visibility branch from 170f3d0 to cf20d2c Compare August 10, 2026 22:37
vriesdemichael and others added 8 commits August 11, 2026 07:23
An agent working a PR with reviewers had no way to see that anything was waiting
on it, and had to be told that comments and tasks were two separate commands.

bb pr get now carries a review_summary with an action_required flag. bb pr list
shows per-PR counts. bb pr comment list returns comment threads and tasks
together, unresolved first, with --unresolved, --state, --tasks-only and
--with-replies.

Counts are absent rather than zero when they could not be measured, and
counts_source names the source. Reporting an unmeasured count as zero would make
a pull request with open feedback look clean.

Two Bitbucket behaviours found while verifying against a real server, both
registered in docs/openapi/fixes.yaml and pinned by live tests:

- OPENAPI-007: the activity timeline serialises an inline comment's anchor path
  as a string while the spec models an object, so one inline comment made the
  whole page fail to decode. Pull requests with inline review comments were
  returning no comments at all.
- OPENAPI-008: a retired endpoint and a missing resource are both 404s but
  differ in body shape. Separating them lets the summary degrade only for
  features a server lacks and report everything else.

BREAKING CHANGE: bb pr comment list --json emits a summarised thread view
instead of raw Bitbucket comment objects, which embedded an entire pull request
per comment. Pass --full for the previous payload. bb pr comment get is
unchanged.

Refs #306

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
bb pr task * called a REST endpoint Atlassian removed in Bitbucket 8.0 and CI
stayed green, for two reasons. Most commands are never invoked by a live test,
so a mock that still serves a retired endpoint is the only thing checking them.
Worse, the pull request task tests did invoke the command and skipped themselves
when it failed:

    if strings.Contains(strings.ToLower(err.Error()), "not_found") {
        t.Skipf("pull request task endpoint unavailable in live environment: %v", err)
    }

A skipped test looks the same as a passing one in CI output.

tools/cli-live-coverage walks the Cobra tree and parses the live tests for
executeLiveCLI calls and error-conditioned skips. A command whose only live
coverage can skip itself on error is reported as masked, not covered.

Current state is 53% asserted, 4 masked, 101 never invoked. The committed report
is a baseline and verification is a ratchet: it fails when a command loses
coverage, arrives without it, or becomes masked. Static analysis, so CI needs no
live infrastructure.

Also retries project seeding on 409. Keys derive from the clock and Bitbucket
deletes projects asynchronously, so a key can outlive its delete.

Refs #306

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Regenerated from a full unit and live run against Bitbucket 10.2.1.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Collision suffixes were assigned while ranging over the spec's paths map. Go
randomises map iteration, so the same spec produced a different
operationId-to-endpoint mapping on every run: whether Get3WithResponse called
/basicauth/latest/config or a pull request endpoint was decided by chance at
generation time. Nothing caught it because models:verify and client:verify are
not run by CI. Paths are now sorted.

Suffixing also assumed the suffixed name was free. Bitbucket 10.2 ships an
operation already called get_2, so renaming a second get collided with it and
the client failed to compile with duplicate Get2 declarations. Suffixes now
increment until the canonical id is unused.

Bitbucket 10.2 also adds four schemas that reference each other through
required, non-nullable $refs. Go cannot size such a value cycle. No operation
references any of them, so both codegen configs exclude them.

Registered as OPENAPI-009 and OPENAPI-010.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The vendored API reference was Bitbucket 9.4 while the container stack and the
live suite run 10.2. That drift is why the CLI still called endpoints Atlassian
removed in 8.0: the spec described a server nobody tested against.

The delta is almost entirely additive, but 10.2 marks far more fields required
(27 required entries across all schemas became 117), so oapi-codegen emits value
types where 9.4 emitted pointers. Call sites are adjusted accordingly.

Two changes are behavioural rather than mechanical:

- Fork synchronization status moves from GetStatus2 to GetStatus. Seven
  operationIds swap endpoints between the specs and this is the only one the CLI
  calls; the compiler caught it.
- RestReviewerGroup no longer carries displayName, so default reviewer listings
  fall back to name.

Per ADR 042 this states no supported version: the spec is provenance for the
generated client, not a claim about which servers work, and the version under
test stays recorded only in docker/compose.yml. The configuration default stays
unset.

CI now runs models:verify and client:verify, which were never run before.

BREAKING CHANGE: models generated from the 10.2 spec expose required fields as
values rather than pointers, and RestReviewerGroup drops displayName. Consumers
of internal/openapi/generated and internal/models/generated must adjust. The bb
command surface and JSON output are unchanged.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Backend.Clone persists http.extraHeader into the repository it clones into so
later fetches carry authentication. A test that pointed it at the working copy
rather than a temporary directory wrote this into the project's own .git/config:

    http.extraheader = Authorization: Basic <base64 dummy-user:dummy-password>
    user.name        = Test User
    user.email       = test@example.local

An unscoped http.extraHeader beats any credential helper, so every push to
GitHub sent dummy-user:dummy-password and came back with "Password
authentication is not supported for Git operations", which reads like a bad
token and sends you looking in the wrong place. The identity override authored
real commits as Test User <test@example.local>, including some on main.

The tests were later isolated with t.TempDir, but nothing detected the damage or
would catch a recurrence.

internal/git/gittest snapshots the repository-scoped configuration before a
package's tests and compares it afterwards. TestMain in internal/git/execgit,
internal/cli and tests/integration/live fails the package when anything changed,
naming the keys and how to undo them. Comparing before against after keeps a
developer's own settings invisible to the guard.

Its git calls use execgit.ScopeFreeEnv, so the guard inspects the repository the
tests run in rather than the one GIT_DIR points at under a hook.

Verified by reproducing the original bug: the offending test still passes and
the package fails with the added key named.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Make unresolved PR commens and tasks more visible to agents

1 participant