feat(pr-update): change reviewers on an existing PR, without discarding approvals

[daniel-pittman]

Closes #57.

Reviewers could only be set at creation time. As the issue puts it, that is the wrong way round: PRs get opened, and then someone works out who should look at them.

bb pr-update 42 --reviewer "{6666...}"           # add, keeping the existing ones
bb pr-update 42 --remove-reviewer "{1111...}"    # remove, keeping everyone else

Mirrored as add_reviewers / remove_reviewers on bb_ops.pr_update and the MCP pr_update tool, whose docstring said reviewers were preserved and now explains that they are the exception to the field merge.

Decision 1: add/remove, not replace

You flagged this as worth deciding explicitly, so here is the reasoning.

Bitbucket has no add-a-reviewer endpoint. The PR PUT replaces the whole reviewers array, so sending just the person being added silently unassigns everyone else — and the request still returns 200, so the bug is invisible without inspecting the body. Both flags therefore read the PR first and send the full resulting list. Adding someone already on the PR is a no-op, not a duplicate, so both are idempotent.

The issue floated --set-reviewers for wholesale replace. This ships add/remove and deliberately omits replace. Replace is precisely the operation that silently discards other people's approvals, and add/remove composes to the same result with every change stated explicitly. bb pr already lists reviewers, so the current set is readable before changing it. If you want replace anyway, say so and it's a small follow-up — but I'd rather not ship the footgun by default.

The race is real and unclosable here. A reviewer added by someone else between the GET and the PUT is lost. Bitbucket exposes no ETag or if-match on this endpoint, so it cannot be fixed in the client; the window is narrow and the operation is trivially repeatable. It's noted in both implementations rather than left for the next reader to find.

Decision 2: approvals are refused, not warned

Removing a reviewer who has already approved discards that approval, and re-adding them does not bring it back. That is unrecoverable from the CLI, so it is refused rather than warned about, unless you pass --drop-approvals (Python: drop_approvals=True). This matches the repo-wide rule that a destructive action is never a default. Removing a reviewer who has not approved needs no opt-in.

$ bb pr-update 42 --remove-reviewer "{bbbb...}"
Error: refusing to remove reviewer(s) who have already approved:
  {bbbb...}
  Removing them discards the approval, and re-adding them does
  not restore it. Pass --drop-approvals to do it anyway.

One subtlety worth a reviewer's eye: the guard reads participants[], not reviewers[]. Approval state exists only on the former — .reviewers[] carries no approved field at all — so a guard that looks for it on reviewers finds nothing and lets every removal through. That is the silent-approval-loss bug in disguise. A test pins the distinction against a fixture whose reviewers carry no approval field, matching what the real API returns.

Readback

bb pr now prints each reviewer's UUID beside the name, and pr-update echoes the resulting list after a reviewer change. Display names are not unique in a workspace (see below), so a name alone doesn't confirm the right person was assigned.

Your bb members note: the suggested fix wouldn't have worked

You saw two entries with the same display name and suggested marking inactive members. I probed the live workspace before implementing that, and it would not have helped:

• The two accounts share display name and nickname.
• Both are active (all 18 members are).
• They differ only by account_id and uuid.

So the ambiguity isn't an active/inactive distinction. bb members now says so explicitly:

  Note: 1 display name/nickname pair(s) are shared by more than
  one account above. They differ only by UUID, so confirm which account
  you mean before using it as a reviewer.

That fires on your real workspace today. Deactivated accounts are also marked (inactive), since they can still be assigned and are dead weight on a PR — but that's a separate, generic improvement, not the fix for what you hit. The status rides in on the same request via fields=+values.user.account_status, so it costs no extra call and no per-member lookup.

Tests

26 new pytest cases (646 total), plus two bash harnesses at 36 and 42 assertions.

Every reviewer assertion reads the actual PUT body, because the silent-unassign bug returns 200 and is invisible from the status code. The approval cases assert no PUT was issued at all, because a guard that fires after the write is worthless.

Mutation-verified rather than trusted for being green:

Mutation	Assertions failed
naive "send only the added reviewer" (the silent-unassign bug)	11
approval guard removed	7
approval sourced from reviewers instead of participants	7
guard moved to after the write	7
remove becomes a no-op	5
read-modify-write runs on title-only updates	2
(inactive) marker inverted	2
collision warning disabled	2

Three pre-existing MCP tests asserted the exact kwargs dict pr_update forwards; the call shape changed by design, so they were updated to the new full dict rather than loosened to a subset check.

Also run: all five bash harnesses (11 + 29 + 37 + 42 + 36), 646 pytest, py_compile, and a bash 3.2 parse.

Incidentally corrects the README's MCP tool count, stale at 42 since members_list landed in v1.10.0.

🤖 Generated with Claude Code

[claude]

Claude finished @daniel-pittman's task in 2m 40s —— View job

---

Code Review

Reviewed the full diff against origin/develop. This is a careful, well-reasoned change — the design decisions in the PR body hold up against the code, and the read-modify-write, the participants-vs-reviewers distinction, and the destructive-default guard are all implemented correctly on both the bash and Python sides. A few notes below; nothing blocking.

✅ What's right

• Reviewer merge is genuinely additive. Both bb_ops.pr_update (bb_ops.py:781-834) and cmd_pr_update (bb:1608-1660) GET the PR, keep survivors in order, append new UUIDs, and dedup — so the PUT carries the full array and never silently unassigns. Parity is real, not approximate.
• The approval guard reads participants[], not reviewers[] (bb_ops.py:799-812, bb:1621-1636). This is the subtle correctness point the PR body calls out, and it's implemented the right way on both sides. The guard also runs before the PUT, so a refusal costs no write.
• Destructive-default rule respected. Removing an approver is refused unless --drop-approvals / drop_approvals=True, consistent with the repo-wide stance in CLAUDE.md.
• Iterable[str] bare-string trap closed (_normalise_reviewer_uuids, bb_ops.py:674) — add_reviewers="{abc}" would otherwise become one reviewer per character.
• The GET-then-PUT race is documented in both implementations rather than left for the next reader, and the "no replace flag" omission is justified in code, docstring, and README.

🟡 MEDIUM — bare (unbraced) UUID on --remove-reviewer is a silent no-op

_require_reviewer_uuid (bb:521-543) deliberately accepts bare UUIDs — its own comment says "users routinely strip the braces when copying" — but it stores the value raw (bb:1519-1522), it does not canonicalize. The current reviewers and participants come back from the API braced ({…}). So the jq comparisons match by exact string:

[$current[] | select(. as $u | ($remove | index($u)) | not)]   # bb:1651
[.participants[]? | select(.approved) | .user.uuid] as $approved  # bb:1626

A user who runs bb pr-update 42 --remove-reviewer aaaaaaaa-1111-... (no braces, which the validator explicitly permits) matches nothing → the reviewer is not removed, exit 0, no message. Same latent behavior on the Python side (removing & approved_uuids / current_uuids are all braced from the API), so parity is preserved.

Severity is MEDIUM, not higher, because it fails closed — it under-removes rather than dropping an approval or unassigning someone — and the new "Reviewers now:" readback means the person is still visible afterward, so an attentive user can notice. But the input contract advertises bare UUIDs as valid, so this is a foreseeable path, not a disallowed input. Suggest normalizing the stored UUID to a canonical braced form once (in _require_reviewer_uuid for bash, and in _normalise_reviewer_uuids for Python) so add/remove/dedup all compare apples to apples. --reviewer (add) has the milder version of the same issue: a bare add won't dedup against a braced existing entry, yielding a duplicate array element.

Fix this →

🟢 LOW — stale clause in agents/bitbucket.md

The updated pr_update line still reads "the PUT merges them into the existing PR, preserving source/destination branches and reviewers" and then immediately explains reviewers are the exception that gets replaced. The "and reviewers" in the first clause now contradicts the rest of the sentence — drop those two words. (mcp_server.py and the README were both corrected; this one wasn't.)

Test coverage

CI is green — both CI runs report success across Python 3.10/3.11/3.12 and the bash syntax/32-floor checks (per the "CI Tests" comment and the CI status API); Semgrep is clean.

Coverage is strong and matches the change:

• 13 new Python cases in TestPrUpdateReviewers assert the actual PUT body, not just status — the only way to catch the silent-unassign bug, which returns 200. Approval-refusal cases assert ["GET"] with no PUT, pinning that the guard fires before the write. test_approval_is_read_from_participants_not_reviewers explicitly asserts "approved" not in pr["reviewers"][0], locking in the distinction. The mutation table in the PR body (11/7/7/7/5 assertions failing under seeded bugs) is credible given these assertions.
• The new bash harness test_pr_update_reviewers.sh stands up a mock API and reads the recorded PUT body the same way — good parity of test strategy, not just code.
• The three MCP kwargs-dict tests were updated to the new full dict rather than loosened, which is the right call.

One gap worth noting relative to the MEDIUM above: no test exercises a bare (unbraced) UUID through add or remove — every fixture UUID (_A/_B/_C, and the bash equivalents) is braced, so the normalization path is unverified. If you take the fix, add a bare-UUID removal case on both sides.

No test changes needed for the README/agents doc edits and the CI YAML wiring — those are exempt.

---

Nice work overall — the reasoning quality in both the code comments and the PR description is high, and the security-sensitive parts (guard ordering, participants source, no-replace-by-default) are correct.

[claude]

Claude finished @daniel-pittman's task in 2m 22s —— View job

---

Code Review

Re-reviewed the full diff against origin/develop, focusing on commit 05e08e9, which addresses the two findings from the earlier pass. Both are correctly resolved, and the fix carries its own tests and mutation evidence. Nothing blocking.

✅ Prior findings resolved

• MEDIUM (bare-UUID silent no-op) — fixed on both sides, correctly. Reviewer UUIDs now canonicalise to the braced form the API returns before the set arithmetic runs, so add/remove/dedup compare like-for-like:

  • Bash: _require_reviewer_uuid (bb:543-554) sets REVIEWER_UUID="{${core}}" in the caller's scope and the call sites store $REVIEWER_UUID rather than the raw $2 (bb:1319-1320, 1529-1534). Assigning instead of echoing is the right call here — an exit 1 inside a $( ) would only kill the subshell and let a rejected value through as empty, the same reasoning resolve_repo uses.
  • Python: _canonical_reviewer_uuid (bb_ops.py:674) is invoked from the shared _normalise_reviewer_uuids, and pr_create now routes through that same helper (bb_ops.py:658-660) instead of its old inline loop — so create and update validate and canonicalise identically. Good de-duplication of contract.
  • The bash validator only strips a matched brace pair before checking, so a half-brace ({abc… with no closer) still fails validation rather than being mangled — nicely defensive.

• LOW (stale clause in agents/bitbucket.md) — fixed. The contradicting "and reviewers" was dropped from the "preserving source/destination branches" clause (agents/bitbucket.md:63), matching the correction already made in mcp_server.py and the README.

🟢 LOW — Python _canonical_reviewer_uuid double-braces a half-braced UUID

The regex _UUID_RE (bb_ops.py:668) uses \{?…\}?, which matches an unbalanced brace like {aaaaaaaa-1111-1111-1111-111111111111 (leading brace, no closer). For that input the core = stripped[1:-1] if startswith("{") and endswith("}") guard is false, so core stays {aaaa… and the function returns {{aaaa…} — a doubled brace, contradicting the docstring's promise that non-canonical input is "returned untouched."

This is LOW, not blocking: it's malformed input the contract doesn't invite, and it fails closed — the API rejects {{…} with a 400 rather than corrupting any existing reviewer. It's purely a parity nit: bash rejects the half-brace locally with a clear message, while the MCP path sends it and gets an opaque 400. If you want them symmetric, require balanced braces in the regex (^(\{[hex-]+\}|[hex-]+)$) or mirror bash's matched-pair strip.

Fix this →

Test coverage

CI is green — the "CI Tests" comment reports the pytest suite passing across Python 3.10/3.11/3.12 plus the bash syntax/32-floor checks on 05e08e95a, and the Semgrep (OSS) scan is clean. (I don't have a runnable environment here, so this reflects the posted CI outcome.)

The fix commit is source-changing behavior and ships with tests — no coverage gap:

• Three new Python cases pin the fix exactly: test_bare_uuid_is_canonicalised_for_removal (the reported no-op), test_bare_uuid_dedups_on_add (the duplicate-append half), and test_non_uuid_shaped_values_pass_through_untouched (the escape hatch). These assert the actual PUT body, which is the only way to catch the original bug since it returned 200.
• The v1.10.0 pr_create assertion that pinned bare pass-through was correctly updated to pin canonicalisation rather than loosened — the send shape changed by design.
• Two new bash cases in test_members_and_reviewers.sh cover bare removal and bare add against the mock API. The PR body's mutation evidence (raw-store reintroduction fails 2 pytest + 2 bash assertions per harness) is consistent with these assertions.
• The one open LOW above (half-brace double-brace) is unverified by any test, but it's malformed-input hardening, not a regression in the shipped behavior — no test is required for it.

---

The security-sensitive core of this PR — participants-vs-reviewers approval source, guard-before-PUT ordering, no replace-by-default, and now brace canonicalisation so the guards can't be bypassed by input form — is correct on both surfaces. Solid work.

[github-actions]

Semgrep (OSS): no findings

Clean scan. Posted before the Claude review.

[github-actions]

CI Tests

✅ Tests passed: the pytest suite across Python 3.10/3.11/3.12 (and bash syntax checks) is green on af35acaef.

Posted before the Claude review so the test outcome is folded into the review.

[claude]

Claude finished @daniel-pittman's task in 1m 16s —— View job

---

Code Review

Re-reviewed the full diff against origin/develop, focusing on commit af35aca, which resolves the round-2 LOW (half-braced UUID double-bracing). It's fixed correctly on both surfaces and ships with tests. Nothing blocking — this is ready.

✅ Prior finding resolved

• LOW (half-brace double-brace) — fixed, correctly. _UUID_RE (bb_ops.py:672) is now an alternation that requires balanced braces — ^(?:\{CORE\}|CORE)$ — so a half-braced {aaaa… (or aaaa…}) matches neither arm and _canonical_reviewer_uuid returns it verbatim rather than re-wrapping it into {{aaaa…}. That restores the docstring's "returned untouched" promise. I traced both orientations by hand against the regex and the startswith("{") and endswith("}") strip guard; both fail the match and short-circuit before the strip, so the value is passed through unchanged.
• Parity holds. The bash side already rejected the same input locally: _require_reviewer_uuid's strip only fires on a matched pair ([[ "$raw" == "{"*"}" ]], bb:533), so a half-brace survives into the hex regex (bb:537) as {aaaa… and fails validation with a clear message. So bash rejects locally, Python passes the malformed value through to a 400 — both fail closed, and the asymmetry the round-2 review named (Python silently double-bracing) is gone.

Nothing new found

I re-checked the security-sensitive core once more since the canonicalisation now sits upstream of the guards:

• The approval guard still reads participants[], not reviewers[], on both sides (bb_ops.py:817-825, bb:1631-1636), and still runs before the PUT.
• Canonicalisation runs in _normalise_reviewer_uuids before the set arithmetic, and pr_create routes through the same helper (bb_ops.py:659-660), so create and update validate and canonicalise identically. The braced-form convergence is what lets the removing & approved_uuids / dedup comparisons match the braced values the API returns — the guards can't be bypassed by brace form.
• No replace-by-default; --drop-approvals / drop_approvals=True is the only opt-in to the destructive path, consistent with the CLAUDE.md rule.

Test coverage

Per the CI Tests comment, the pytest suite is green across Python 3.10/3.11/3.12 and the bash syntax/32-floor checks on af35acaef, and the Semgrep (OSS) scan is clean. (No runnable environment here, so this reflects the posted CI outcome.)

The fix commit changes source behavior and ships with tests — no gap:

• test_half_braced_uuid_is_not_mangled is parametrized over both half-brace orientations ({aaaa… and aaaa…}) and asserts the actual PUT body carries the value untouched ([{"uuid": half_braced}]) — the right assertion, since the pass-through is exactly what the loose regex broke. The commit notes it's mutation-verified by restoring the loose \{?…\}? regex.
• test_non_uuid_shaped_values_pass_through_untouched pins the escape hatch, and the round-1 cases (test_bare_uuid_is_canonicalised_for_removal, test_bare_uuid_dedups_on_add) still lock the braced/bare convergence — all reading the PUT body, which is the only way to catch the silent-unassign/no-op class.
• Half-brace parity on the bash side is not separately tested, but that path is unchanged by this commit and was already exercised by the existing _require_reviewer_uuid validation cases — no test required for it.

---

All three findings from the earlier passes are now resolved, each with a test and mutation evidence. The reviewer-change feature is correct on both surfaces — participants-sourced approval guard, guard-before-PUT, no replace-by-default, and brace canonicalisation upstream of the guards so input form can't slip past them. Solid work.

[github-actions]

Review Gate

✅ Gate: PASS — Review reported no HIGH/CRITICAL findings.