manage_pr_reviewers reports a reviewer it never seated

Context

Operator bug report 2026-08-02, from a live observation on #16385. Reproduced independently before filing — the mechanism below is measured, not inferred from the symptom.

manage_pr_reviewers returns a success string built from its own request arguments. It never reads what GitHub actually did. A login that does not exist is echoed back as "Successfully requested", and the PR is left with no reviewer and no failure signal.

Live latest-open sweep 2026-08-02T19:00:19Z (latest 20 open issues); no equivalent found. #16348 ("A backup that captured nothing must not report success") is the same defect class in a different subsystem — report-the-request-instead-of-the-effect — not a duplicate. A2A in-flight claim sweep could not run: the neo-mjs-a2a MCP surface was unavailable in the filing session, so this ticket rests on the live GitHub sweep alone. Herd risk is low — the defect came from a single operator observation, not a broadcast prompt.

The Problem

Observed call:

manage_pr_reviewers({pr_number: 16385, action: 'add', reviewers: ['neo-gpt-euclid']})
→ {"message":"Successfully requested reviewers on PR #16385: neo-gpt-euclid",
   "pr_number":16385,"reviewers":["neo-gpt-euclid"],"team_reviewers":[]}

neo-gpt-euclid has never existed. The canonical roster in ai/graph/identityRoots.mjs carries @neo-gpt (line 334) and @neo-gpt-emmy (line 375) — no euclid seat.

Measured mechanism (reproduced 2026-08-02, exact command the service builds):

gh api repos/neomjs/neo/pulls/16385/requested_reviewers -X POST -f 'reviewers[]=neo-gpt-euclid'

So GitHub accepts the request, returns 200, and silently seats nobody. The tool's try/catch never fires because there is no error to catch.

The correction to the reported fix shape. The report proposed a second API call to read back reviewRequests. That is unnecessary: the mutation's own 200 response already carries the truth and the service throws it away — await execFn(command, ...) at PullRequestService.mjs:2731 discards the return value. Parsing the response we already receive is strictly better: no extra round-trip, and no TOCTOU window between mutating and re-reading.

Positive control (read-only, same representation the POST returns) confirms the field is a truthful effect-channel rather than one that is always empty in this shape:

The DELETE path returns the same PR-object shape (probed with a nil-effect removal), so the remove branch verifies through the identical channel — inverted: the login must be absent from the result.

Why it matters. An agent that trusts the success string believes the cross-family review mandate (pull-request §6.1) is satisfied and moves on. The PR then sits unreviewed with no signal that anything failed. This is the invitation layer of that mandate — when it lies, the mandate's validation layer never gets a reviewer to validate. At filing time four open PRs carry no seated reviewer (#16389, #16386, #16385, #16381); this defect makes "reviewer requested" unfalsifiable from the tool result, so none of them can be distinguished from a successful request.

The Architectural Reality

• ai/services/github-workflow/PullRequestService.mjs:2692 — managePrReviewers. Builds the REST command, awaits execFn, discards the response, and returns a message interpolated from reviewerList / teamReviewerList (the caller's own input) at lines 2731-2738.
• ai/services/github-workflow/IssueService.mjs:260 — assignIssue already implements the correct discipline: mutate, then #fetchCurrentAssignees post-verify, then return verifiedAssignees (line 275). The named sibling in managePrReviewers's own JSDoc — the pattern exists in this file's neighbour and was simply not applied here.
• ai/mcp/server/github-workflow/openapi.yaml:970 — manage_pr_reviewers operation; its '200' currently $refs the generic SuccessResponse (line 2105), which documents only message + details and therefore cannot express a seated-vs-requested distinction. CheckoutPullRequestResponse (line 2116) is the in-repo precedent for a dedicated verifiable-result schema — it exists precisely so callers can compare a claimed result against live state.
• ai/mcp/server/github-workflow/toolService.mjs:442 — binding; :59 classifies the tool PUBLIC_GITHUB_WRITE_ACCESS.
• test/playwright/unit/ai/services/github-workflow/PullRequestServiceReviewers.spec.mjs — three existing tests, all asserting the command string that gets built (add → REST POST, remove → REST DELETE, guards). None asserts anything about the returned result, which is why the defect shipped: the suite proves the request is well-formed and is structurally blind to whether it took effect.

The Fix

Report the effect, not the request. Fail loud when a requested login was not seated.

1. Capture the response in managePrReviewers — execFn already returns it; stop discarding it. Parse requested_reviewers[].login and requested_teams[].slug from the 200 body.
2. Diff requested against seated. For action: 'add', any entry of reviewerList / teamReviewerList absent from the parsed response is a failure. For action: 'remove', any entry still present is a failure.
3. Fail loud with a structured error naming exactly which logins were not seated — a new REVIEWER_NOT_SEATED code — rather than a partial success. A reviewer that was not seated is a failure.
4. Return the verified set on success as verifiedReviewers / verifiedTeamReviewers, mirroring verifiedAssignees, so the caller can always tell requested from seated.
5. Guard the unparseable case. If the response cannot be parsed into the expected shape, that is a failure too — it must not silently degrade to the current echo behaviour. (SuccessResponse has no additionalProperties: false, so added fields are non-breaking.)
6. Contract + spec. Add a ManagePrReviewersResponse schema to openapi.yaml following the CheckoutPullRequestResponse precedent, and extend PullRequestServiceReviewers.spec.mjs.

Contract Ledger Matrix

Decision Record impact

none — no ADR governs MCP tool response contracts. The work aligns with the verify-effect vocabulary in learn/agentos/process/evidence-ladder.md and with the established verifiedAssignees post-verify precedent, neither of which is ADR-bound.

Acceptance Criteria

• managePrReviewers parses the requested_reviewers / requested_teams payload from the mutation response instead of discarding it.
• action: 'add' with a login absent from the response returns a structured error coded REVIEWER_NOT_SEATED naming each unseated login — not a success message.
• action: 'remove' with a login still present in the response returns the same structured failure.
• A successful add returns verifiedReviewers / verifiedTeamReviewers derived from the response, never from the caller's input arguments.
• A response that cannot be parsed into the expected shape returns a structured error, not a success.
• The success path is unreachable unless every requested target was verified against the response — so the names in the success message are a verified statement rather than an echo. (Amended during implementation: this AC originally read "no success path interpolates reviewerList/teamReviewerList into the returned message". That is unsatisfiable on the remove path — a removed reviewer is by definition absent from the response, so its name can only come from the request. The echo hazard is real but lives in the verified* fields, which the AC below pins; the message text was the wrong lever.)
• Spec: requesting a nonexistent login must not report success — asserted against a fixture reproducing the measured 200-with-empty-requested_reviewers response.
• Spec: partial seating (one of two logins seated) fails and names only the unseated one.
• Spec: the remove path verifies absence.
• openapi.yaml documents the result contract via a dedicated ManagePrReviewersResponse schema; OpenApiValidatorCompliance.spec.mjs stays green.
• The three existing tests in PullRequestServiceReviewers.spec.mjs still pass.

Out of Scope

• Pre-validating logins against ai/graph/identityRoots.mjs before the call. Roster membership and GitHub-collaborator status are different predicates; the post-verify gate catches every unseated case regardless of cause, including causes a roster check cannot see (revoked collaborator, org removal, typo'd but real account). A roster-lint belongs in its own ticket if wanted.
• Auditing or re-seating reviewers on the PRs currently missing one. Repair is lane work, not this fix.
• Any change to the cross-family review mandate itself (pull-request §6.1) — this is its invitation layer only.
• Retrofitting effect-verification onto other write tools. Real, but a sweep with its own scope; see #16348 for the sibling instance.

Avoided Traps

• Trusting a zero exit code as proof of effect. The whole defect. gh exited 0, GitHub returned 200, and nothing happened. Any fix that keeps reasoning from exit status reproduces the bug.
• Adding a second read-back call. The reported fix shape, and the intuitive one. Measurement shows the mutation response already carries requested_reviewers, so a follow-up GET would add a round-trip and a TOCTOU window in which a concurrent change could make the read disagree with what this call did. Parse the response you already have.
• Reporting partial success. Returning "2 of 3 seated" with a success shape leaves the caller to notice the shortfall — the same trust failure one level down. An unseated reviewer is a failure.
• Inferring the mechanism from the symptom. "Empty reviewRequests afterwards" is consistent with several causes — a swallowed non-zero exit, broken -f 'reviewers[]=' shell quoting, or a silently-accepted request. Only the third survived measurement; the first two would have demanded entirely different fixes.

Related

• #16348 — same defect class (report-the-request, not the effect) in the backup lane. Independent fix; shared principle.
• #16385 — the PR where this was observed; still unreviewed at filing.
• ai/graph/identityRoots.mjs — canonical roster proving neo-gpt-euclid is not a seat.

Handoff Retrieval Hints

• Retrieval Hint: "manage_pr_reviewers reports request not effect"
• Retrieval Hint: "requested_reviewers 200 empty silently ignored unknown login"
• Anchors: PullRequestService.mjs:2692 (defect), IssueService.mjs:260 (the pattern to mirror)