show which reviewers a manual claim will actually run - #6211
Merged
Conversation
…l defaults
A `/do:next` claim resolves its reviewers from the claim-work task metadata
FIRST and only falls back to the install-wide Code Review Defaults. Both manual
claim surfaces seeded their reviewer display from `GET /api/code-review/defaults`
alone, which cannot see that override — so a claim-work pin saved months earlier
kept running codex + claude while every reviewer control on screen showed the
antigravity chain the user had since configured. The Issues tab, which offers no
reviewer picker at all, showed nothing.
- New `GET /api/apps/:id/claim-reviewers` resolves the chain through the same
`resolveClaimWorkMetadata` → `resolveClaimReviewerConfig` path
`buildClaimWorkTask` uses to fill the prompt's `{reviewers}` token, and reports
`source` (`task-override` vs `defaults`) so the UI can name the layer that won.
- `useClaimReviewers` backs both surfaces. A failed lookup stays unresolved
rather than reporting an empty chain — "couldn't ask" must not read as "merges
with no review".
- The run drawer seeds its untouched picker from that resolution and, on an
override, points at Chief of Staff → Schedule rather than Models → Code
Reviewers. The Issues tab renders the same read-only summary beside its
provider pin.
- `REVIEWER_OVERRIDE_KEYS` / `hasReviewerOverride` replace the hand-listed roster
in GlobalConfigControls, so the picker's "Use system Code Review Defaults"
reset clears exactly what the server counts as an override; the client mirror
is pinned by the existing parity test.
… clearable Cleanup on the claim-reviewer lookup, plus the one gap that made its advice unfollowable. - `claimReviewersFrom` / `resolveAppClaimReviewers` (cosTaskGenerator) now own the layer precedence for both the claim builder and the lookup route. The route had hand-copied that chain, which is the drift the lookup exists to prevent. - `hasReviewerOverride` keys on `REVIEWER_LIST_OVERRIDE_KEYS`, not the full roster: `reviewStopMode` / `reviewerApplies` are slashdo run flags and a claim prompt has no flag string to put them in, so neither can change which reviewers run. Reporting a stop-mode as the source sent the user to clear a pin that wasn't supplying the list they were looking at. The wide roster stays for the picker's reset, which does clear both. - `ClaimReviewerSource` renders the "where this came from" sentence for both surfaces; they had already diverged on which panel they pointed at. - The reviewer picker — and the "Use system Code Review Defaults" reset beside it — now render for claimFlow task types. Their shipped metadata sets neither `openPR` nor `reviewLoop`, so the picker never appeared for `claim-work`: the override every claim obeys had no control anywhere that could clear it, while the claim surfaces told the user to come here and do exactly that. - `useClaimReviewers` returns the payload or `null` instead of a 9-field sentinel with a derivable `resolved` flag, which flattens the drawer's seeding memo. - Trimmed the retold rationale to the resolution site, and the route's response test down to what the route itself decides — the resolution it previews is covered behaviorally on the shared resolver. Follow-ups filed: #6208 (the picker persists a defaults snapshot as an override, which is what manufactured the stale pin) and #6210 (the JIRA play button skips the claim-work layer its own docstring promises to honor).
- ClaimReviewerSource sent the user to "this app's Automation tab" to clear the override. The reviewer picker is rendered only by GlobalConfigControls, reached only through Chief of Staff → Schedule; the Automation tab has no such control, so that half of the sentence was a dead end. Names the one screen that works. - The apiApps doc said `task-override` means the defaults "were never consulted". resolveReviewerConfig falls back per FIELD, so a task pinning only `reviewers` still takes its models and usernames from the defaults. - IssuesTab carried an empty-reviewers branch saying a Claim "will merge without one". claimSafeReviewers never returns an empty list, so the branch was unreachable and its claim was wrong either way.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
A
/do:nextclaim resolves its reviewers from the claim-work task metadata first, falling back to the install-wide Code Review Defaults only when that layer pins nothing. Both manual claim surfaces seeded their reviewer display fromGET /api/code-review/defaultsalone — which cannot see that override — so a claim-work pin saved months earlier kept runningcodex,claudewhile every reviewer control on screen showed theantigravitychain the user had since configured. The app Issues tab, which has no reviewer picker at all, showed nothing.GET /api/apps/:id/claim-reviewersresolves throughresolveAppClaimReviewers, the same functionbuildClaimWorkTaskfills the claim prompt's{reviewers}token from, and reportssource(task-override/defaults) so the UI can name the layer that won. ExtractingclaimReviewersFromis what makes the preview and the run structurally unable to disagree — the route had hand-copied that precedence.useClaimReviewersbacks both surfaces. It returns the payload ornull: "couldn't ask" and "nothing configured" must not collapse, since an empty chain rendered as fact reads as a claim that merges with no review.hasReviewerOverridekeys onREVIEWER_LIST_OVERRIDE_KEYS, not the full roster.reviewStopMode/reviewerAppliesare slashdo run flags and a claim prompt has no flag string to put them in, so neither changes which reviewers run — reporting a stop-mode as the source would send the user to clear a pin that isn't supplying the list they're looking at. The wide roster stays for the picker's reset, which does clear both.The gap that made the advice unfollowable
The banner tells the user to clear the override in Chief of Staff → Schedule. That control never rendered for
claim-work:reviewersApplygated onopenPR/reviewLoop, and the shipped claim metadata sets neither. A claimFlow task's prompt opens and merges its own PR and runs the reviewers itself, so its resolved list is operative regardless — the picker (and its "Use system Code Review Defaults" reset) now render for those task types. Without this the PR would have named an override with no control anywhere that could clear it.Test plan
resolveAppClaimReviewerscovered behaviorally incosTaskGenerator.test.js— override-wins, defaults-fallthrough, the stop-mode-only case that must not count as an override, and the copilot strip. The route test covers only what the route decides (thesourcemapping, and that the two run flags stay unpublished).hasReviewerOverridepinned on presence-not-truthiness, including the explicitly-empty pins; the client mirror is asserted against the server for both rosters and the predicate, since||-style drift is the half that's easy to get wrong.Follow-ups filed: #6208 (the picker persists a defaults snapshot as an override — the mechanism that manufactured the stale pin) and #6210 (the JIRA play button skips the claim-work layer its own docstring promises to honor).