diff --git a/client/src/components/apps/ClaimReviewerSource.jsx b/client/src/components/apps/ClaimReviewerSource.jsx new file mode 100644 index 0000000000..e21019dfaa --- /dev/null +++ b/client/src/components/apps/ClaimReviewerSource.jsx @@ -0,0 +1,36 @@ +import { Link } from 'react-router'; + +/** + * Where a claim's reviewer list came from, in one sentence. + * + * Shared by the two manual claim surfaces because they were already drifting on + * the answer: a claim resolves the claim-work task metadata FIRST and only falls + * back to the install-wide Code Review Defaults, so a user who changed the + * defaults and sees a different chain needs to be sent to whichever one is + * actually supplying it — and being sent to the wrong panel is the same class of + * confusion as not being told at all. + * + * The link target is deliberate and verified: the reviewer picker (and the "Use + * system Code Review Defaults" reset beside it) is rendered ONLY by + * `GlobalConfigControls`, reachable only through Chief of Staff → Schedule's + * TaskConfigDrawer. The per-app `claim-work` override the server merges on top + * carries reviewer keys too, but the app's Automation tab has no picker for + * them — naming it here would send the user to a screen with no such control. + */ +export default function ClaimReviewerSource({ source }) { + if (source === 'task-override') { + return ( + <> + {' — from the '}claim-work{' reviewer override in '} + Chief of Staff → Schedule + {', not Models → Code Reviewers. Clear it there to follow the install default again.'} + + ); + } + return ( + <> + {' — from '} + Models → Code Reviewers. + + ); +} diff --git a/client/src/components/apps/SlashDoRunDrawer.jsx b/client/src/components/apps/SlashDoRunDrawer.jsx index ccbab708d2..6af617d78d 100644 --- a/client/src/components/apps/SlashDoRunDrawer.jsx +++ b/client/src/components/apps/SlashDoRunDrawer.jsx @@ -7,6 +7,8 @@ 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 ClaimReviewerSource from './ClaimReviewerSource'; import { isProcessProvider } from '../../utils/providers'; import WorkItemPicker from './WorkItemPicker'; import * as api from '../../services/api'; @@ -30,6 +32,11 @@ 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. `installed` still comes from the + // defaults, which is why both are fetched. + 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 +49,22 @@ 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); - // 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. + // Seeded display for an untouched picker. It has to show the reviewers the RUN + // would resolve, which is not the Code Review Defaults whenever a claim-work + // override is in play (see `GET /apps/:id/claim-reviewers`). The defaults are + // the fallback for the window before the lookup lands and for one that failed — + // they carry per-reviewer pins as `Model` / `Effort` + // scalars, which the picker takes as token-keyed maps. const seededReview = useMemo( - () => ({ + () => claimReviewers ?? { ...codeReviewDefaults, reviewerModels: reviewerModelsFromDefaults(codeReviewDefaults), reviewerEfforts: reviewerEffortsFromDefaults(codeReviewDefaults), - }), - [codeReviewDefaults] + }, + [claimReviewers, codeReviewDefaults] ); const reviewValue = review ?? seededReview; @@ -171,6 +181,15 @@ 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 has to be told + where it came from. */} + {!review && claimReviewers?.source === 'task-override' && ( +

+ Seeded +

+ )} )} diff --git a/client/src/components/apps/SlashDoRunDrawer.test.jsx b/client/src/components/apps/SlashDoRunDrawer.test.jsx index 2dfbc27bf8..24d4c38df5 100644 --- a/client/src/components/apps/SlashDoRunDrawer.test.jsx +++ b/client/src/components/apps/SlashDoRunDrawer.test.jsx @@ -1,6 +1,7 @@ import { describe, expect, it, vi, beforeEach } from 'vitest'; import { render, screen, waitFor } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; +import { MemoryRouter } from 'react-router'; import SlashDoRunDrawer from './SlashDoRunDrawer'; const api = vi.hoisted(() => ({ @@ -9,6 +10,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() })); @@ -22,17 +26,21 @@ vi.mock('../../services/apiLocalLlm', () => ({ getToolUseModels: vi.fn(() => new Promise(() => {})), })); +// Routed: the override note links to the panel that owns the pin, so the drawer +// needs a router the way it has one in the app. const renderDrawer = (props = {}) => render( - + + + ); describe('SlashDoRunDrawer', () => { @@ -49,6 +57,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 +82,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..f01032c1c2 100644 --- a/client/src/components/apps/tabs/IssuesTab.jsx +++ b/client/src/components/apps/tabs/IssuesTab.jsx @@ -12,6 +12,8 @@ import ProviderModelSelector from '../../ProviderModelSelector'; import { useThemeContext } from '../../ThemeContext'; import { useCosTaskUpdates } from '../../../hooks/useCosTaskUpdates'; import useProviderModels from '../../../hooks/useProviderModels'; +import useClaimReviewers from '../../../hooks/useClaimReviewers'; +import ClaimReviewerSource from '../ClaimReviewerSource'; import { chipColors } from '../../../lib/chipContrast'; import { isProcessProvider } from '../../../utils/providers'; import * as api from '../../../services/api'; @@ -241,8 +243,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 +256,11 @@ 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 — NOT the + // Models → Code Reviewers list, whenever a claim-work override is in play (see + // `GET /apps/:id/claim-reviewers`). This tab has no reviewer picker, so it + // names them read-only beside the provider pin. + 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 +584,20 @@ export default function IssuesTab({ appId, appName }) { /> + {claimReviewers && ( +
+ + Reviewed by + + {/* No empty-list branch: the route resolves through + `claimSafeReviewers`, which falls back to a non-empty list rather + than ever handing a claim agent nothing to run. */} +

+ {claimReviewers.csv} + +

+
+ )}