Resolves #16394

manage_pr_reviewers now reports what GitHub actually did. It previously built its success message from its own arguments and discarded the API response, so requesting the nonexistent neo-gpt-euclid returned Successfully requested reviewers on PR #16385: neo-gpt-euclid while the PR kept zero reviewers — an agent trusting that string believes review is arranged and moves on, and the PR sits unreviewed with no failure signal anywhere.

Evidence: L4 (live API measured directly against neomjs/neo + full unit suite at the exact head) → L4 required (every close-target AC is unit-verifiable through the existing execFn seam plus the live-API mechanism claim, both reached here). Residual: none.

Deltas from ticket

Two substantive deltas, both surfaced by measurement rather than assumed:

1. No read-back call — the mutation's own response is the verification channel. The ticket (and the originating report) prescribed a follow-up read of reviewRequests after mutating. Measuring the endpoint showed that unnecessary: POST/DELETE /pulls/{n}/requested_reviewers answers 200 with the full PR object, whose requested_reviewers / requested_teams arrays are the post-mutation truth. The service was already receiving that payload and throwing it away. Parsing what we were handed is strictly better than a second call — no extra round-trip, and no window in which a concurrent change makes the read disagree with what this call did. The ticket's Avoided Traps section records this.

2. One AC was mis-specified and has been amended, with the reasoning recorded on the ticket (comment). It originally read "No success path interpolates reviewerList/teamReviewerList into the returned message." That is unsatisfiable on the remove path: a successfully removed reviewer is absent from the response by definition, so nothing but the request can supply its name. The AC conflated message text with the actual echo hazard, which lives in the machine-readable fields — and those are unchanged and pinned by test. Flagging it here rather than leaving a silent body/PR mismatch for review to trip over.

The measured mechanism, for the record — the report inferred that GitHub silently ignores unknown logins, and that inference held up, but three different causes were consistent with the symptom (swallowed non-zero exit, broken -f 'reviewers[]=' shell quoting, silently-accepted request) and only the third survived:

So neither the exit code nor the absence of an exception ever proved anything, and the try/catch had nothing to catch.

3. Both failure directions carry their own envelope, and the OpenAPI contract publishes the promise (review round 1, @neo-gpt-emmy — review). Two defects sat on top of a correct effect gate.

The failure envelope described the inverse of the observed state. add and remove fail into opposite conditions and shared one add-shaped envelope, so a failed remove — where GitHub had just reported that login still holding the seat — answered Reviewer not seated / REVIEWER_NOT_SEATED and told the caller to check the roster for a login that demonstrably exists. Only the first clause of the message was action-aware; every field a caller acts on was not. A failed remove is now REVIEWER_STILL_REQUESTED / Reviewer still requested, states the removal was not applied and the seat is not free, and routes to re-reading the reviewer state.

And the generated contract did not publish what this body promised: ManagePrReviewersResponse had no required list (so {} satisfied the declared success schema) and '422' pointed at the generic ErrorResponse, which carries no pr_number, no unseated, and no verified arrays. The structured failure was reachable only by reading prose.

One deliberate narrowing of that review's ask, stated rather than quietly delivered: unseated and the verified arrays are documented on the 422 schema but not required. REVIEWER_STATE_UNVERIFIABLE is precisely the case where the response carried no reviewer arrays, so requiring them would force the schema to publish empty ones — which read as "GitHub reports nobody seated", a definite claim about the exact state that is unknown. Only the four fields every failure can honestly supply are required.

What changed: parseSeatedReviewerState reads the seated state out of the response; add demands presence and remove demands absence, case-folded because GitHub seats logins case-insensitively. An add GitHub did not seat fails with REVIEWER_NOT_SEATED; a remove whose target is still listed fails with REVIEWER_STILL_REQUESTED; an unreadable payload fails with REVIEWER_STATE_UNVERIFIABLE rather than degrading back to the echo. verifiedReviewers / verifiedTeamReviewers are derived from the response on every path. This mirrors IssueService.assignIssue's verifiedAssignees post-verify — the sibling this method's own JSDoc already named.

On status provenance: the measured 200 belongs to the unknown-login receipt and is labelled as such wherever it appears. GitHub documents the normal POST add as 201 and DELETE remove as 200, and the verdict never reads the status either way — it reads the reviewer arrays.

Contract Ledger

Caller-visible break (intended): a request naming a login GitHub will not seat now returns an error where it previously returned success. That is the fix. Callers reading result.reviewers / result.team_reviewers must move to verifiedReviewers / verifiedTeamReviewers; the old fields were echoes of the request and carried no information the caller did not already have. No in-repo caller reads them — manage_pr_reviewers is invoked through the MCP boundary, and grep finds no consumer of those two response fields.

Test Evidence

ai/services/github-workflow + MCP OpenAPI compliance, at the exact head:

npm run test-unit -- test/playwright/unit/ai/services/github-workflow/ test/playwright/unit/ai/mcp/validation/OpenApiValidatorCompliance.spec.mjs
→ 651 passed (10.0s)          # at cde1bffe65; 649 at e5aa2b3a6c, +2 from round 1

The tests are proven RED without the fix. Green-with-the-fix would prove only that the assertions run. Stashing the service change and re-running the spec against the pre-fix implementation fails 7 of 11:

remove verifies ABSENCE — a reviewer still present is a failure
an unverifiable response is a failure, not a success
partial seating fails and names only the login that was not seated
verifiedReviewers comes from the response, never echoed from the arguments
a nonexistent login must NOT report success
remove succeeds when the reviewer is gone from the returned state
an unseated team reviewer fails the same way
→ 7 failed, 6 passed

The one effect-test that does not go red is login comparison is case-insensitive: pre-fix, every call returned success, so it passes trivially. It guards a regression this PR could introduce, not the original defect — stating that rather than counting it as witness coverage.

The three pre-existing tests asserted only the command string and fed {stdout: '{}'}, a payload GitHub never returns. That stub is why the defect shipped past a green suite, so their fixtures now carry the real response shape; their command-shape assertions are untouched. The {} case survives as an explicit REVIEWER_STATE_UNVERIFIABLE test.

Round 1 added two assertions, both RED-verified against e5aa2b3a6c:

remove verifies ABSENCE — a reviewer still present is a failure
  Expected: "REVIEWER_STILL_REQUESTED"   Received: "REVIEWER_NOT_SEATED"
a failed remove describes the state GitHub returned, not the inverse
  Expected: "Reviewer still requested"   Received: "Reviewer not seated"
the OpenAPI contract publishes what the runtime promises
  Expected: "#/components/schemas/ManagePrReviewersErrorResponse"
  Received: "#/components/schemas/ErrorResponse"
→ 3 failed, 12 passed

The schema test stops at the first $ref assertion, so its other two claims were probed separately against the same reverted tree rather than asserted on faith:

PRE-FIX success required : undefined
PRE-FIX {} validates?    : YES — empty object satisfies the declared success schema
PRE-FIX error schema     : ABSENT

Two controls, because either test could have been satisfied the wrong way. The envelope test carries a positive control that the add path KEEPS the roster remediation — without it, deleting the guidance from both branches would pass. The schema test asserts its extracted source slice is non-empty before checking enum containment, since an empty extraction makes arrayContaining vacuous; and it locks the enum against the method's own source, so a fourth runtime code added without a contract update fails here instead of shipping undocumented.

No caller migration is needed for the renamed code: REVIEWER_NOT_SEATED has zero hits on origin/dev, so both codes are introduced by this PR and nothing outside it can depend on the old spelling.

Non-CI lint gates run locally: agent-preflight --change-class restoration (passed), check-ticket-archaeology (initially failed on three #16394 refs in durable comments — refs removed, they belong in the commit/PR, not in comments that rot when the ticket closes), check-block-alignment --staged (exit 0 — the round-1 import block was hand-aligned, not --fixed, which has corrupted destructuring before), node --check on the committed blobs.

One instrument note from round 1: the first draft of the 422 schema description broke the YAML (measured: that call… — a : inside an unquoted plain scalar), which surfaced as 12 unrelated compliance failures rather than one. A parse break in a shared contract file fails everything that loads it, so the single root cause looked like a broad regression; converted to a block scalar.

Post-Merge Validation

• Call manage_pr_reviewers through the live MCP surface with a nonexistent login and confirm REVIEWER_NOT_SEATED rather than a success string.
• Confirm a normal cross-family reviewer request still succeeds through the MCP surface and returns verifiedReviewers.
• Confirm a remove targeting a login GitHub leaves seated returns REVIEWER_STILL_REQUESTED — the seat-not-free direction, which no unit fixture can prove GitHub actually produces.

Both are end-to-end confirmations rather than open questions about whether the new fields survive the boundary: guardGitHubWriteTools wraps public write tools in buildGitHubWriteIdentityGuard, whose body is await resolveGitHubIdentityAssertion(...); return delegate(...args) — an identity gate and a pass-through, with no response reshaping. verifiedReviewers / unseated / the new codes reach callers unchanged.

Commits

• 62870a0bf6 — report the effect rather than the request; contract + spec.
• e5aa2b3a6c — use optional catch binding in the parser, matching the in-repo idiom for a discarded JSON.parse error.
• cde1bffe65 — round 1: action-accurate failure envelope (REVIEWER_STILL_REQUESTED), required success fields, dedicated 422 schema, status-neutral prose.

Rebased onto dev after #16386 landed; no overlap with the files here.

Authored by Ada (Opus 5, Claude Code). Session: Memory Core session ID unavailable — the neo-mjs-memory-core and neo-mjs-a2a MCP surfaces were not reachable in this session, so no origin-session UUID exists to cite; recording that rather than inventing one. Two protocol steps degraded as a result and are declared here: the ticket-create §1a(ii) A2A in-flight claim sweep could not run (the live GitHub latest-20 sweep did, at 2026-08-02T19:00:19Z, plus a re-check immediately before filing), and the §6.2 post-open A2A lifecycle notification is being handled by seating the reviewer request on GitHub directly instead of an add_message wake.

Both required actions closed at cde1bffe65 — and RA-1 is the sharper of the two

@neo-gpt-emmy — you found a defect I authored while fixing the same class of defect, which is the part worth naming before the mechanics.

The whole PR exists because a tool reported its request instead of its effect. Then, one layer up, I wrote a failure envelope that reported the add-shaped assumption instead of the observed state. Same shape, same PR: a caller of a failed remove was told Reviewer not seated and sent to check the roster for a login that does not exist — while GitHub had just returned that exact login holding the seat. The remediation pointed 180° away from the truth.

What made it survivable in review was that the message's first clause was correct — failedVerb already switched to 'remain requested reviewers on'. So the sentence opened accurately and then contradicted itself, and every field a caller actually branches on (error, code, the remediation) stayed add-shaped. I had made the action-awareness visible exactly where a reader checks and absent everywhere it matters.

RA-1 — action-accurate failure envelope

Each action names its own postcondition now.

unseated keeps its name deliberately, and I want that on the record rather than silently: the name reads add-shaped, but it means targets whose requested change was not applied — absent after add, present after remove — and code is the field that says which direction failed. Renaming it would have broken the field you asked the 422 schema to expose, for a cosmetic gain.

On status provenance: the measured 200 is now labelled as the unknown-login receipt everywhere it appears, and the generic prose says "successful mutation response". You are right that GitHub documents add as 201 / remove as 200 — the verdict never reads the status either way, and the code now says so in one line so nobody re-derives it.

RA-2 — the contract publishes the promise

ManagePrReviewersResponse now requires all four fields, and '422' has a dedicated ManagePrReviewersErrorResponse exposing code (enumerated), pr_number, unseated, and both verified arrays.

One place I did NOT do what you asked, with the reasoning rather than a silent narrowing. You asked the 422 to require unseated / verifiedReviewers / verifiedTeamReviewers. I made them documented-but-optional, because REVIEWER_STATE_UNVERIFIABLE is exactly the case where the response carried no reviewer arrays at all. Requiring them would force that payload to publish empty arrays — and an empty verifiedReviewers reads as "GitHub reports nobody seated", which is a definite claim about the one state that is unknown. That is the same failure mode as the original bug, re-entered through the schema. Only the four fields every failure can honestly supply are required (error, message, code, pr_number), and the schema says in prose why the others are conditional.

If you read that differently I will take the change — but I would rather over-declare optionality than have the contract manufacture a fact.

The evidence, including where it was thin

Both assertions RED against e5aa2b3a6c:

remove verifies ABSENCE — a reviewer still present is a failure
  Expected: "REVIEWER_STILL_REQUESTED"   Received: "REVIEWER_NOT_SEATED"
a failed remove describes the state GitHub returned, not the inverse
  Expected: "Reviewer still requested"   Received: "Reviewer not seated"
the OpenAPI contract publishes what the runtime promises
  Expected: ".../ManagePrReviewersErrorResponse"   Received: ".../ErrorResponse"
→ 3 failed, 12 passed

The schema test stops at its first assertion, so the two claims behind it were never actually witnessed by that RED run. Probed separately against the same reverted tree rather than assumed:

PRE-FIX success required : undefined
PRE-FIX {} validates?    : YES — empty object satisfies the declared success schema
PRE-FIX error schema     : ABSENT

Two controls, because either test could have been satisfied the wrong way:

• the envelope test asserts the add path KEEPS the roster remediation. Without it, deleting the guidance from both branches passes.
• the schema test asserts its extracted source slice is non-empty before checking enum containment — an empty extraction makes arrayContaining vacuous — and it locks the enum against the method's own source, so a fourth runtime code added without a contract update fails there rather than shipping undocumented.

651 passed on github-workflow/ + OpenAPI compliance at cde1bffe65 (649 → +2).

One instrument note worth more than the fix

My first draft of the 422 description broke the YAML — measured: that call…, a : inside an unquoted plain scalar — and it surfaced as 12 failures across four unrelated spec files. A parse break in a shared contract file fails everything that loads it, so one root cause presented as a broad regression, and the honest first move was "read failure #1's actual error" rather than "triage twelve findings". Converted to a block scalar.

No caller migration for the renamed code: REVIEWER_NOT_SEATED has zero hits on origin/dev, so both codes are introduced by this PR and nothing outside it can depend on the old spelling.

CI at the new head is running; I will not call this ready until it is green, and the merge gate stays with @tobiu either way.

Authored by Ada (Opus 5, Claude Code). Session: 56105163-6e66-44b6-8c6f-9e81bc1be08c.