From c5c604299ea4548c382917585aaeaaa9bba21e90 Mon Sep 17 00:00:00 2001 From: "[._.]/ Adam Eivy" Date: Fri, 4 Sep 2026 05:16:51 +0000 Subject: [PATCH 1/3] show which reviewers a manual claim will actually run, not the install defaults MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../src/components/apps/SlashDoRunDrawer.jsx | 52 ++++++++++--- .../components/apps/SlashDoRunDrawer.test.jsx | 36 +++++++++ client/src/components/apps/tabs/IssuesTab.jsx | 31 +++++++- .../components/apps/tabs/IssuesTab.test.jsx | 43 +++++++++++ client/src/components/cos/constants.js | 1 + .../tabs/schedule/GlobalConfigControls.jsx | 19 ++--- client/src/hooks/README.md | 1 + client/src/hooks/index.js | 1 + client/src/hooks/useClaimReviewers.js | 65 ++++++++++++++++ client/src/lib/reviewerPins.js | 19 +++++ client/src/services/apiApps.js | 11 +++ server/lib/apiRouteCatalog.generated.json | 8 ++ server/lib/reviewerConfig.js | 31 ++++++++ server/lib/reviewerConfig.test.js | 46 ++++++++++++ server/routes/apps/taskTypes.js | 44 ++++++++++- server/routes/apps/taskTypes.test.js | 74 +++++++++++++++++++ 16 files changed, 457 insertions(+), 25 deletions(-) create mode 100644 client/src/hooks/useClaimReviewers.js diff --git a/client/src/components/apps/SlashDoRunDrawer.jsx b/client/src/components/apps/SlashDoRunDrawer.jsx index ccbab708d2..297445924a 100644 --- a/client/src/components/apps/SlashDoRunDrawer.jsx +++ b/client/src/components/apps/SlashDoRunDrawer.jsx @@ -7,6 +7,7 @@ import useProviderModels from '../../hooks/useProviderModels'; import useReviewerModelOptions from '../../hooks/useReviewerModelOptions'; import { reviewerModelsFromDefaults, reviewerEffortsFromDefaults } from '../../lib/reviewerModels'; import { CodeReviewDefaultsProvider, useCodeReviewDefaults } from '../../hooks/useCodeReviewDefaults'; +import useClaimReviewers from '../../hooks/useClaimReviewers'; import { isProcessProvider } from '../../utils/providers'; import WorkItemPicker from './WorkItemPicker'; import * as api from '../../services/api'; @@ -30,6 +31,10 @@ const enabledProcessProviderFilter = (p) => Boolean(p?.enabled) && isProcessProv */ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, onQueued }) { const codeReviewDefaults = useCodeReviewDefaults(); + // What a claim actually resolves for this app — the claim-work override layer + // the defaults above cannot see. Only `/do:next` reads reviewers server-side, + // so no other command pays for the lookup. + const claimReviewers = useClaimReviewers(command === 'next' ? appId : null); // Resolved model lists for the reviewer table's Model column (the picker never // fetches — see its `modelOptions` prop). const reviewerModelOptions = useReviewerModelOptions(); @@ -42,19 +47,37 @@ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, o const [effort, setEffort] = useState(''); const [simplify, setSimplify] = useState(true); - // Seeded from the install's Code Review Defaults for display. `reviewDirty` - // gates whether they're SENT — see the component doc. + // Seeded from what the RUN will resolve for display. `review` staying null is + // what gates whether the fields are SENT — see the component doc. const [review, setReview] = useState(null); + // Seeded display for an untouched picker. It has to show the reviewers the run + // would resolve, which is NOT the Code Review Defaults: a claim resolves its + // claim-work task metadata first and only falls back to them, so an override + // there runs a chain this drawer previously never showed (a claim reviewed with + // `codex` while the picker displayed `antigravity`). `claimReviewers` is that + // resolution; the defaults remain the fallback for the window before it lands + // and for a lookup that failed — an unresolved lookup must not seed an empty + // chain, which would read as "no reviewers configured". + // // The defaults carry per-reviewer models and efforts as `Model` / // `Effort` scalars; the picker takes the token-keyed maps, so fold them - // in for the seeded (untouched) display. + // in for that fallback. const seededReview = useMemo( - () => ({ - ...codeReviewDefaults, - reviewerModels: reviewerModelsFromDefaults(codeReviewDefaults), - reviewerEfforts: reviewerEffortsFromDefaults(codeReviewDefaults), - }), - [codeReviewDefaults] + () => (claimReviewers.resolved + ? { + reviewers: claimReviewers.reviewers, + usernames: claimReviewers.usernames, + optionalReviewers: claimReviewers.optionalReviewers, + reviewerMaxRounds: claimReviewers.reviewerMaxRounds, + reviewerModels: claimReviewers.reviewerModels, + reviewerEfforts: claimReviewers.reviewerEfforts, + } + : { + ...codeReviewDefaults, + reviewerModels: reviewerModelsFromDefaults(codeReviewDefaults), + reviewerEfforts: reviewerEffortsFromDefaults(codeReviewDefaults), + }), + [claimReviewers, codeReviewDefaults] ); const reviewValue = review ?? seededReview; @@ -171,6 +194,17 @@ function SlashDoRunDrawerBody({ open, command, label, appId, appName, onClose, o The claim flow opens and merges its own PR, so these reviewers gate that merge (slashdo --review-with). {!review && ' Leave them untouched to use this app’s configured reviewers.'}

+ {/* Which layer supplied the seeded list. A claim-work override wins + over Models → Code Reviewers silently, so a user who changed the + install default and sees a different chain here needs to be told + where it came from — that mismatch is exactly what sent a claim to + `codex` after the defaults had been moved to `antigravity`. */} + {!review && claimReviewers.source === 'task-override' && ( +

+ These come from the claim-work task override in Chief of Staff → Schedule, not from + Models → Code Reviewers. Clear it there (“Use system Code Review Defaults”) to follow the install default again. +

+ )} )} diff --git a/client/src/components/apps/SlashDoRunDrawer.test.jsx b/client/src/components/apps/SlashDoRunDrawer.test.jsx index 2dfbc27bf8..7d451c8aa0 100644 --- a/client/src/components/apps/SlashDoRunDrawer.test.jsx +++ b/client/src/components/apps/SlashDoRunDrawer.test.jsx @@ -9,6 +9,9 @@ const api = vi.hoisted(() => ({ // Backs the reviewer table's Model column (useReviewerModelOptions). getLocalLlmStatus: vi.fn(), getAppWorkItems: vi.fn(), + // What the RUN resolves — the claim-work override layer getCodeReviewDefaults + // cannot see, and what the untouched picker is seeded from. + getAppClaimReviewers: vi.fn(), createSlashdoTask: vi.fn() })); @@ -49,6 +52,10 @@ describe('SlashDoRunDrawer', () => { reason: 'actionable-issues', transient: false }); + api.getAppClaimReviewers.mockResolvedValue({ + source: 'defaults', reviewers: ['copilot'], usernames: [], optionalReviewers: [], + reviewerMaxRounds: {}, reviewerModels: {}, reviewerEfforts: {}, csv: 'copilot' + }); api.createSlashdoTask.mockResolvedValue({ id: 'task-1', status: 'pending' }); }); @@ -70,6 +77,35 @@ describe('SlashDoRunDrawer', () => { expect(settings.reviewers).toBeUndefined(); }); + // The bug this seeding fixes: the picker used to display the Code Review + // Defaults, which do NOT include the claim-work task override the run resolves + // FIRST. A user who had moved the install default to `antigravity` saw + // `antigravity` here while every claim actually reviewed with codex + claude. + it('seeds the untouched picker from the reviewers the RUN resolves, not the install defaults', async () => { + api.getCodeReviewDefaults.mockResolvedValue({ reviewers: ['antigravity'], usernames: [], optionalReviewers: [] }); + api.getAppClaimReviewers.mockResolvedValue({ + source: 'task-override', reviewers: ['codex', 'claude'], usernames: [], optionalReviewers: [], + reviewerMaxRounds: {}, reviewerModels: {}, reviewerEfforts: {}, csv: 'codex,claude' + }); + + renderDrawer(); + + // Selected reviewers render as Remove buttons; unselected ones as Add. + await waitFor(() => expect(screen.getByRole('button', { name: /Remove Codex/ })).toBeInTheDocument()); + expect(screen.getByRole('button', { name: /Remove Claude/ })).toBeInTheDocument(); + expect(screen.queryByRole('button', { name: /Remove Antigravity/ })).not.toBeInTheDocument(); + // …and the user is told WHERE that list comes from, since it isn't the panel + // they would go to in order to change it. + expect(screen.getByText(/claim-work/)).toBeInTheDocument(); + }); + + it('does not blame a claim-work override when the reviewers came from the install defaults', async () => { + renderDrawer(); + + await waitFor(() => expect(screen.getByText('Reviewers (in order):')).toBeInTheDocument()); + expect(screen.queryByText(/claim-work/)).not.toBeInTheDocument(); + }); + it('sends the reviewer list only once the user edits it', async () => { const onQueued = vi.fn(); renderDrawer({ onQueued }); diff --git a/client/src/components/apps/tabs/IssuesTab.jsx b/client/src/components/apps/tabs/IssuesTab.jsx index 572e0fc817..5ed74cd409 100644 --- a/client/src/components/apps/tabs/IssuesTab.jsx +++ b/client/src/components/apps/tabs/IssuesTab.jsx @@ -12,6 +12,7 @@ import ProviderModelSelector from '../../ProviderModelSelector'; import { useThemeContext } from '../../ThemeContext'; import { useCosTaskUpdates } from '../../../hooks/useCosTaskUpdates'; import useProviderModels from '../../../hooks/useProviderModels'; +import useClaimReviewers from '../../../hooks/useClaimReviewers'; import { chipColors } from '../../../lib/chipContrast'; import { isProcessProvider } from '../../../utils/providers'; import * as api from '../../../services/api'; @@ -241,8 +242,10 @@ export default function IssuesTab({ appId, appName }) { // Page-level provider/model/effort pin for every Claim AND Replan button on this tab — // left untouched (blank), a claim resolves the install's active provider, // same as the bare button always did (POST /tasks/slashdo -> resolveAgentProviderAndModel; - // this manual path does NOT consult the app's scheduled claim-work override — - // that's a separate resolution used only by the automated claim-work task). + // this manual path does NOT consult the app's scheduled claim-work override for + // the PROVIDER — that pin is read only by the automated claim-work task). + // Scoped to the provider deliberately: the REVIEWERS below do come from that + // override, which is precisely the mismatch `claimReviewers` exists to surface. // This picker never persists across a reload; it's a session convenience for // "claim the next several issues with model X" without reopening the Agent // Operations drawer each time. @@ -252,6 +255,13 @@ export default function IssuesTab({ appId, appName }) { } = useProviderModels({ filter: enabledProcessProviderFilter, allowDefault: true, silent: true, withEffort: true }); const [effort, setEffort] = useState(''); const [overrideContext, setOverrideContext] = useState(''); + // The reviewers a Claim launched from this tab will actually run. This tab + // offers no reviewer picker, so the resolution was previously invisible here — + // and it is NOT the Models → Code Reviewers list a user would assume: a + // claim-work task override wins over it, which is how a claim from this tab + // reviewed with `codex` after the install default had been changed. Surfaced + // read-only next to the provider pin, with its source named. + const claimReviewers = useClaimReviewers(appId); // Keep the event-driven path based on the latest runs without putting a // mutable state snapshot in its effect dependencies. Socket callbacks can @@ -575,6 +585,23 @@ export default function IssuesTab({ appId, appName }) { /> + {claimReviewers.resolved && ( +
+ + Reviewed by + +

+ {claimReviewers.reviewers.length + ? {claimReviewers.csv || claimReviewers.reviewers.join(',')} + : 'No reviewers resolve for this app.'} + {claimReviewers.source === 'task-override' + ? <> — from the claim-work task override in{' '} + Chief of Staff → Schedule, + not Models → Code Reviewers. Clear it there to follow the install default. + : <> — from Models → Code Reviewers.} +

+
+ )}
- {claimReviewers.resolved && ( + {claimReviewers && (
Reviewed by

- {claimReviewers.reviewers.length - ? {claimReviewers.csv || claimReviewers.reviewers.join(',')} - : 'No reviewers resolve for this app.'} - {claimReviewers.source === 'task-override' - ? <> — from the claim-work task override in{' '} - Chief of Staff → Schedule, - not Models → Code Reviewers. Clear it there to follow the install default. - : <> — from Models → Code Reviewers.} + {claimReviewers.reviewers.length ? ( + <> + {claimReviewers.csv} + + + ) : 'No reviewers resolve for this app — a Claim will merge without one.'}

)} diff --git a/client/src/components/cos/constants.js b/client/src/components/cos/constants.js index ba06b8bb02..926f778578 100644 --- a/client/src/components/cos/constants.js +++ b/client/src/components/cos/constants.js @@ -336,6 +336,8 @@ export { REVIEW_STOP_MODES, DEFAULT_REVIEW_STOP_MODE, REVIEWER_OVERRIDE_KEYS, + REVIEWER_LIST_OVERRIDE_KEYS, + hasReviewerOverride, normalizeReviewers, MODEL_CAPABLE_CLI_REVIEWERS, LOCAL_LLM_REVIEWERS, diff --git a/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx b/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx index 4f125f53c0..a81269b5d7 100644 --- a/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx +++ b/client/src/components/cos/tabs/schedule/GlobalConfigControls.jsx @@ -2,7 +2,7 @@ import { useState, useEffect, useMemo } from 'react'; import useFieldDraft from '../../../../hooks/useFieldDraft'; import { RotateCcw, AlertCircle } from 'lucide-react'; import CronInput from '../../../CronInput'; -import { AGENT_OPTIONS, BRANCHES_PER_AGENT_DEFAULT, BRANCHES_PER_AGENT_OPTIONS, BRANCHES_PER_AGENT_TASK_TYPES, DEFAULT_REVIEW_STOP_MODE, REVIEWER_OVERRIDE_KEYS, IMPLICIT_PR_COMPLETION, PR_AUTHOR_FILTER_OPTIONS, PR_COMPLETION_OPTIONS, pinnedPrCompletion, prCompletionOption, ISSUE_AUTHOR_FILTER_OPTIONS, ISSUE_AUTHOR_FILTER_TASK_TYPES, SWARM_COUNT_OPTIONS, SWARM_TASK_TYPES } from '../../constants'; +import { AGENT_OPTIONS, BRANCHES_PER_AGENT_DEFAULT, BRANCHES_PER_AGENT_OPTIONS, BRANCHES_PER_AGENT_TASK_TYPES, DEFAULT_REVIEW_STOP_MODE, REVIEWER_OVERRIDE_KEYS as REVIEW_CONFIG_KEYS, IMPLICIT_PR_COMPLETION, PR_AUTHOR_FILTER_OPTIONS, PR_COMPLETION_OPTIONS, pinnedPrCompletion, prCompletionOption, ISSUE_AUTHOR_FILTER_OPTIONS, ISSUE_AUTHOR_FILTER_TASK_TYPES, SWARM_COUNT_OPTIONS, SWARM_TASK_TYPES } from '../../constants'; import ReviewerPicker from '../../ReviewerPicker'; import Banner from '../../../ui/Banner'; import InfoTooltip from '../../../ui/InfoTooltip'; @@ -25,13 +25,14 @@ import { INTERVAL_DESCRIPTIONS, PERPETUAL_DESCRIPTION, toggleMetadataField, pipe // runs (no app) land on the server-side fallback. const PR_COMPLETION_INHERIT_HINT = `Uses the target app's "After opening PR" default (Apps → Edit App), or "${prCompletionOption(IMPLICIT_PR_COMPLETION)?.label}" when it has none.`; -// These fields are the task-local reviewer-loop override. Removing them lets -// the picker and server resolver fall back to the install-wide Code Review -// Defaults without changing the task's PR policy or other agent options. The -// roster is the shared REVIEWER_OVERRIDE_KEYS, so the reset button clears -// exactly what the server counts as an override when it reports which layer a -// claim run's reviewers came from. -const REVIEW_CONFIG_KEYS = REVIEWER_OVERRIDE_KEYS; +// The task-local reviewer-loop override is REVIEWER_OVERRIDE_KEYS (imported as +// REVIEW_CONFIG_KEYS above). Removing those keys lets the picker and the server +// resolver fall back to the install-wide Code Review Defaults without changing +// the task's PR policy or other agent options. +// +// Deliberately the WIDE roster, not `hasReviewerOverride`'s list-bearing subset: +// the reset clears the two run flags too, so gating its visibility on the subset +// would leave a stop-mode-only override on screen with no control that removes it. export default function GlobalConfigControls({ taskType, config, onUpdate, onTrigger, category: _category, providers, providersLoaded = true, activeProviderId, apps, updating, setUpdating, allTaskTypes, improvementDisabled, dataInputCatalog }) { const reviewDefaults = useCodeReviewDefaults(); @@ -223,9 +224,18 @@ export default function GlobalConfigControls({ taskType, config, onUpdate, onTri // Reviewers only run under review-then-merge, so the picker hides for the two // policies that never reach them — but an unpinned ('') task may still inherit // review-then-merge from its app, so that keeps it. - const reviewersApply = config.taskMetadata?.openPR - ? prCompletion === '' || prCompletion === 'review-then-merge' - : !!config.taskMetadata?.reviewLoop; + // + // A claimFlow task is unconditional: its PROMPT opens and merges its own PR and + // runs the reviewers itself, so the resolved list is operative no matter what + // `openPR` / `reviewLoop` say (both are false in the shipped claim metadata). + // Without this the picker — and the "Use system Code Review Defaults" reset + // beside it — never render for claim-work, leaving a reviewer override that + // every claim obeys with no control anywhere that can clear it. + const reviewersApply = config.taskMetadata?.claimFlow + ? true + : config.taskMetadata?.openPR + ? prCompletion === '' || prCompletion === 'review-then-merge' + : !!config.taskMetadata?.reviewLoop; // `selectedProvider` / `availableModels` come from useTaskModelPins above — it // resolves the pin against the active provider, lists Antigravity's BASE models diff --git a/client/src/components/cos/tabs/schedule/GlobalConfigControls.test.jsx b/client/src/components/cos/tabs/schedule/GlobalConfigControls.test.jsx index 0ece7f2585..82d13bf51a 100644 --- a/client/src/components/cos/tabs/schedule/GlobalConfigControls.test.jsx +++ b/client/src/components/cos/tabs/schedule/GlobalConfigControls.test.jsx @@ -111,6 +111,17 @@ describe('GlobalConfigControls — After opening PR', () => { expect(screen.getByTestId('reviewer-picker')).toBeInTheDocument(); }); + it('keeps the reviewer picker for a claim flow, whose shipped metadata sets neither flag', () => { + // A claim PROMPT opens and merges its own PR and runs the reviewers itself, + // so the resolved list is operative even though `openPR` and `reviewLoop` are + // both false — which is exactly the shipped `claim-work` metadata. Hiding the + // picker here leaves a reviewer override that every claim obeys with no + // control anywhere that can clear it, while the claim surfaces tell the user + // to come here and do precisely that. + renderControls({ taskType: 'claim-work', taskMetadata: { useWorktree: false, openPR: false, claimFlow: true } }); + expect(screen.getByTestId('reviewer-picker')).toBeInTheDocument(); + }); + it('resets the task review override while preserving unrelated task metadata', () => { const onUpdate = renderControls({ taskMetadata: { diff --git a/client/src/hooks/useClaimReviewers.js b/client/src/hooks/useClaimReviewers.js index 0a079fd404..16f58f4a54 100644 --- a/client/src/hooks/useClaimReviewers.js +++ b/client/src/hooks/useClaimReviewers.js @@ -1,61 +1,37 @@ import { useEffect, useState } from 'react'; import * as api from '../services/api'; -// Sentinel shape while the lookup is in flight or has failed. `reviewers: null` -// is deliberately NOT `[]` — an empty array is a real answer ("this app resolves -// to no reviewers") and a caller that seeds a picker from it would render an -// empty chain as though it were configured. Callers gate on `resolved`. -const PENDING = Object.freeze({ - resolved: false, - source: null, - reviewers: null, - usernames: [], - optionalReviewers: [], - reviewerMaxRounds: {}, - reviewerModels: {}, - reviewerEfforts: {}, - csv: '' -}); - /** - * The reviewers a `/do:next` claim will ACTUALLY run for `appId`. + * The reviewers a `/do:next` claim will ACTUALLY run for `appId`, or `null` + * while the lookup is in flight or has failed. + * + * Distinct from `useCodeReviewDefaults`, and the distinction is the point: the + * defaults hook reads Models → Code Reviewers, while a claim resolves its + * reviewers from the claim-work task metadata FIRST and only falls back to those + * defaults. An override there therefore runs a chain the defaults hook cannot + * see — which is how a claim reviewed with `codex` while every reviewer control + * on screen showed `antigravity`. Seed claim surfaces from here; the payload's + * `source` says which layer won. * - * Distinct from `useCodeReviewDefaults`, and the distinction is the whole point: - * the defaults hook reads Models → Code Reviewers, while a claim resolves its - * reviewers as claim-work task metadata FIRST and only falls back to those - * defaults. A `claim-work` reviewer override therefore runs a chain the defaults - * hook cannot see — which is how a claim launched from the Issues tab came to - * review with `codex` while every reviewer control on screen showed - * `antigravity`. Seed claim surfaces from here; `source` says which layer won. + * `null` rather than an empty chain, because "couldn't ask" and "nothing + * configured" must not collapse: an empty reviewer list rendered as fact would + * read as a claim that merges with no review at all. * - * A failed lookup stays `resolved: false` rather than reporting an empty chain — - * "couldn't ask" and "nothing configured" must not collapse. Fetches once per - * mount; a claim drawer is mounted only while open, which is the refresh. + * Fetches once per mount; a claim drawer is mounted only while open, which is + * the refresh. */ export default function useClaimReviewers(appId) { - const [value, setValue] = useState(PENDING); + const [value, setValue] = useState(null); useEffect(() => { - if (!appId) { - setValue(PENDING); - return undefined; - } + setValue(null); + if (!appId) return undefined; let cancelled = false; - setValue(PENDING); api.getAppClaimReviewers(appId) .then((data) => { - if (cancelled || !Array.isArray(data?.reviewers)) return; - setValue({ - resolved: true, - source: data.source || null, - reviewers: data.reviewers, - usernames: Array.isArray(data.usernames) ? data.usernames : [], - optionalReviewers: Array.isArray(data.optionalReviewers) ? data.optionalReviewers : [], - reviewerMaxRounds: data.reviewerMaxRounds || {}, - reviewerModels: data.reviewerModels || {}, - reviewerEfforts: data.reviewerEfforts || {}, - csv: data.csv || '' - }); + // The one guard that matters: without a list there is nothing to show, + // and every other field is shaped by the route from the same resolver. + if (!cancelled && Array.isArray(data?.reviewers)) setValue(data); }) .catch(() => {}); return () => { cancelled = true; }; diff --git a/client/src/lib/reviewerPins.js b/client/src/lib/reviewerPins.js index c103d294e4..f7c233fe71 100644 --- a/client/src/lib/reviewerPins.js +++ b/client/src/lib/reviewerPins.js @@ -192,12 +192,11 @@ export const REVIEW_STOP_MODES = [ export const DEFAULT_REVIEW_STOP_MODE = 'all'; // The task-metadata keys that together form a task-local reviewer override — -// mirror of REVIEWER_OVERRIDE_KEYS. A task type carrying any of them has pinned -// its own reviewers and no longer tracks the install-wide Code Review Defaults, -// which is what the picker's "Use system Code Review Defaults" reset clears. A -// key missing here leaves that reset visible after it has already removed -// everything it knows about, and leaves the removed key silently pinning a -// reviewer the user thinks they just cleared. +// mirror of REVIEWER_OVERRIDE_KEYS. Everything the picker writes, and so +// everything its "Use system Code Review Defaults" reset has to remove. A key +// missing here leaves that reset visible after it has already removed everything +// it knows about, and leaves the removed key silently pinning a reviewer the +// user thinks they just cleared. export const REVIEWER_OVERRIDE_KEYS = Object.freeze([ 'reviewer', 'reviewers', @@ -210,6 +209,21 @@ export const REVIEWER_OVERRIDE_KEYS = Object.freeze([ 'reviewerApplies', ]); +// The subset that can actually change the resolved reviewer LIST — mirror of +// REVIEWER_LIST_OVERRIDE_KEYS. The two run flags are excluded: a claim flow has +// no slashdo flag string to put them in, so neither changes which reviewers run. +export const REVIEWER_LIST_OVERRIDE_KEYS = Object.freeze( + REVIEWER_OVERRIDE_KEYS.filter((key) => key !== 'reviewStopMode' && key !== 'reviewerApplies') +); + +// Mirror of hasReviewerOverride. Key PRESENCE, not truthiness — an explicitly +// empty `optionalReviewers: []` is a real override, so `||`-style checks report +// the wrong answer. +export function hasReviewerOverride(metadata) { + return !!metadata && typeof metadata === 'object' && !Array.isArray(metadata) + && REVIEWER_LIST_OVERRIDE_KEYS.some((key) => key in metadata); +} + // Resolve task metadata to an ordered, deduped reviewer list (mirror of the // server's normalizeReviewers): prefers `reviewers`, falls back to the legacy // single `reviewer`, defaults to DEFAULT_REVIEWERS. diff --git a/server/lib/README.md b/server/lib/README.md index e196918ac0..da7ee06b72 100644 --- a/server/lib/README.md +++ b/server/lib/README.md @@ -143,7 +143,7 @@ The barrel `server/lib/index.js` is a machine-checkable enumeration of every pub | `antigravity.js` | Antigravity (`agy`) CLI provider helpers — id/sentinel constants (`ANTIGRAVITY_CLI_ID`, `ANTIGRAVITY_CONFIGURED_DEFAULT`, `LEGACY_GEMINI_*`), `isAntigravityCommand`/`isAntigravityCliProvider` predicates, and `ensureAntigravityPrintArgs(args, {model, effort})`/`ensureAntigravityTuiArgs(args, {model, effort})`/`stripAntigravityUnsupportedArgs` argv normalizers. `parseAntigravityModelList(stdout)` parses `agy models` rows — accepts both the modern `\t