diff --git a/review/SKILL.md b/review/SKILL.md index c2e05ded6..9a7960216 100644 --- a/review/SKILL.md +++ b/review/SKILL.md @@ -965,13 +965,13 @@ Retain the completed action in the invocation action list before starting any re ### Step 5c: Batch-ask about ASK items -If there are ASK items remaining, present them in ONE AskUserQuestion: +Present remaining ASK items in ONE AskUserQuestion: -- List each item with a number, the severity label (or `[ADVISORY]` for optional advice), the problem, and a recommended fix -- For each item, provide options: A) Fix as recommended, B) Skip +- Number each item with its severity label (or `[ADVISORY]` for optional advice), problem and recommended fix +- Options per item: A) Fix as recommended, B) Skip (describe only as: no code/index change; Skip recorded) - Include an overall RECOMMENDATION -If 3 or fewer ASK items, you may use individual AskUserQuestion calls instead of batching. +With 3 or fewer ASK items, individual AskUserQuestion calls are fine. Retain each explicit Skip choice and its finding metadata in the invocation action list. Do not record an unanswered question as skipped or ask again about a decision already revalidated in this invocation. ### Step 5d: Apply user-approved fixes diff --git a/review/SKILL.md.tmpl b/review/SKILL.md.tmpl index 9aae36eb3..9a9c84948 100644 --- a/review/SKILL.md.tmpl +++ b/review/SKILL.md.tmpl @@ -284,13 +284,13 @@ Retain the completed action in the invocation action list before starting any re ### Step 5c: Batch-ask about ASK items -If there are ASK items remaining, present them in ONE AskUserQuestion: +Present remaining ASK items in ONE AskUserQuestion: -- List each item with a number, the severity label (or `[ADVISORY]` for optional advice), the problem, and a recommended fix -- For each item, provide options: A) Fix as recommended, B) Skip +- Number each item with its severity label (or `[ADVISORY]` for optional advice), problem and recommended fix +- Options per item: A) Fix as recommended, B) Skip (describe only as: no code/index change; Skip recorded) - Include an overall RECOMMENDATION -If 3 or fewer ASK items, you may use individual AskUserQuestion calls instead of batching. +With 3 or fewer ASK items, individual AskUserQuestion calls are fine. Retain each explicit Skip choice and its finding metadata in the invocation action list. Do not record an unanswered question as skipped or ask again about a decision already revalidated in this invocation. ### Step 5d: Apply user-approved fixes diff --git a/test/fixtures/shared-libs-index-flags-skip-description-public.json b/test/fixtures/shared-libs-index-flags-skip-description-public.json new file mode 100644 index 000000000..4cfa78c74 --- /dev/null +++ b/test/fixtures/shared-libs-index-flags-skip-description-public.json @@ -0,0 +1,55 @@ +{ + "source": "CI E2E Evals runs 36755432181 (head 131d43be, slice 9) and 36762284181 (head 4643cb85, slice 10); exact public AskUserQuestion inputs whose Skip descriptions narrate later-pass work or prior-record replacement. Both captures remain failed.", + "cases": [ + { + "run": "36755432181", + "test": "shared-libs-review-path-eligibility", + "scenario": "submodule", + "git_sha": "9045484c", + "input": { + "questions": [ + { + "question": "[ADVISORY] src/retry-worker.ts:2 — the diff replaced the worker's one-line re-export of lib/retry-after with a verbatim 15-line copy of retrySeconds; modules/retry/retry-route.ts (first-party submodule, same runtime bundle per both READMEs) carries the same copy plus a new trailing comment added after the prior decision. The prior Skip is not reusable (checker: reusable:false; submodule path not snapshot-covered and changed). Proposed fix: re-export the tested lib/retry-after.ts retrySeconds from both callers (worker: 1-line re-export; submodule route: 1-line re-export via a submodule commit plus gitlink bump), keeping deployment boundaries as documented. Estimated implementation: remove 32, add 2, save ~30 lines; existing test/retry-after.test.ts covers the contract, ~2–4 optional lines to assert re-export identity. Shared-failure blast radius: scheduler and src/retry-route already depend on this helper, so worker and submodule route would join the same failure domain. RECOMMENDATION: Skip in this bounded no-edit replay (a Fix requires source and submodule edits that this replay cannot apply), and apply the extraction in a normal editing pass. How do you want to handle this advisory?", + "header": "Advisory", + "options": [ + { + "label": "Skip (Recommended)", + "description": "Record an explicit Skip for this advisory now; no edits. The extraction can be applied in a later editing review pass." + }, + { + "label": "Fix as recommended", + "description": "Approve re-exporting retrySeconds from lib/retry-after in the worker and the submodule route. This requires source and submodule edits, which this bounded no-edit replay cannot apply; the review would be reported blocked rather than completed." + } + ], + "multiSelect": false + } + ] + } + }, + { + "run": "36762284181", + "test": "shared-libs-review-index-flags", + "scenario": "skip-worktree", + "git_sha": "3bbf9334", + "input": { + "questions": [ + { + "question": "[ADVISORY] src/retry-worker.ts:2 — the diff replaces the worker's one-line re-export of the tested helper lib/retry-after.ts#retrySeconds (HEAD commit 'worker initially reuses the existing helper') with a byte-identical 14-line copy of that function. src/retry-route.ts:2-15 already carries the same verbatim copy, and src/scheduler.ts:1 imports the real helper. The helper's contract (null/blank fallback, integer seconds, HTTP-date parsing, 3600s ceiling, negative → 0) is covered by test/retry-after.test.ts:3-9. Proposal: revert the worker to `export { retrySeconds } from '../lib/retry-after';` and migrate the route to the same import. No behavior differences to preserve (identical bodies, same TypeScript runtime). Estimated implementation: worker -15/+1, route -17/+1, total ≈ -30 lines; existing helper tests cover the shared contract, an import smoke test could add ~0-5 lines. Shared-failure blast radius: scheduler, worker and route all depend on one parser, which is already true for the scheduler and the helper is tested. Caveat: src/retry-route.ts has the skip-worktree index flag and an uncommitted trailing edit, so the normal diff hides it and `git add` will not stage a migration there until the flag is cleared. RECOMMENDATION: Fix. Note this is a bounded no-edit replay: choosing Fix records the fix as approved but blocked, not applied. How do you want to handle this advisory?", + "header": "Advisory", + "options": [ + { + "label": "Fix as recommended (Recommended)", + "description": "Approve reverting the worker to the helper re-export and migrating the route caller. In this no-edit replay the edit cannot be applied, so the review reports the fix as approved-but-blocked and does not claim completion." + }, + { + "label": "Skip", + "description": "Keep the duplicated parser in the worker and route for now. Recorded as an explicit new Skip decision for this finding identity (worker, route, helper), replacing the invalidated prior Skip." + } + ], + "multiSelect": false + } + ] + } + } + ] +} diff --git a/test/shared-libs-fixture.test.ts b/test/shared-libs-fixture.test.ts index 15c798a72..7affb5de8 100644 --- a/test/shared-libs-fixture.test.ts +++ b/test/shared-libs-fixture.test.ts @@ -6,7 +6,7 @@ import * as path from 'node:path'; import { execFileSync, spawnSync } from 'node:child_process'; import { createSharedInteractiveToolHandler, createSharedLibsFixture, fixtureGit, fixtureWrite, installSourceShims, - installHostileGitConfig, isGuardedGitRequest, standaloneInstructions, SHARED_LIBS_ROOT, readRequests, seedOpportunitySources, sharedReadOnlyViolations, shellQuote, snapshotFixture, type SharedLibsFixture, + installHostileGitConfig, isGuardedGitRequest, reviewLifecycleInstructions, standaloneInstructions, SHARED_LIBS_ROOT, readRequests, seedOpportunitySources, sharedReadOnlyViolations, shellQuote, snapshotFixture, type SharedLibsFixture, SharedCaptureAccumulator, type SharedCaptureAttempt, isInternalClaudeGitRequest, SHARED_LIBS_OLDER_OPEN_PRS, incompleteFirstFileView, } from './helpers/shared-libs-eval-fixture'; import { EvalCollector, type EvalTestEntry } from './helpers/eval-store'; @@ -14,6 +14,7 @@ import { collectorOutcomeCounts } from '../scripts/test-paid-shards'; import { E2E_TOUCHFILES, GLOBAL_TOUCHFILES, selectTests } from './helpers/touchfiles'; import nativeNoChangeCases from './fixtures/shared-libs-no-change-ci-public.json'; import r44 from './fixtures/shared-libs-index-flags-r44-packets.json'; +import skipDescriptions from './fixtures/shared-libs-index-flags-skip-description-public.json'; import { seedPathReviewPrerequisites, checkPathReviewPrerequisites } from './helpers/shared-libs-path-fixture'; const cleanup: string[] = []; @@ -72,6 +73,27 @@ describe('shared-code Git guard', () => { }); }); +describe('review Skip option description', () => { + test.each(skipDescriptions.cases)('Step 5c limits Skip to its own effect; the $scenario capture narrated more and stays refused', async ({ input }) => { + const rule = 'B) Skip (describe only as: no code/index change; Skip recorded)'; + expect(fs.readFileSync(path.join(SHARED_LIBS_ROOT, 'review/SKILL.md'), 'utf8')).toContain(rule); + const workflow = fs.readFileSync(reviewLifecycleInstructions({ root: scratch() } as SharedLibsFixture), 'utf8'); + expect(workflow.slice(workflow.indexOf('### Step 5c'), workflow.indexOf('### Step 5d'))).toContain(rule); + const refusals: Error[] = []; + const handler = () => createSharedInteractiveToolHandler('skip', { nonQuestion: () => { throw new Error('unexpected tool'); }, + onQuestion: () => {}, onAnswer: () => {}, onRefusal: error => { refusals.push(error); } }); + const skip = input.questions[0].options.find(option => option.label.startsWith('Skip'))!; + expect(skip.description).toMatch(/can be applied in a later|replacing the invalidated prior Skip/); + await expect(handler()('AskUserQuestion', input)).rejects.toThrow('No unambiguous no-change option'); + expect(refusals).toHaveLength(1); + const ruled = structuredClone(input); + ruled.questions[0].options.find(option => option.label.startsWith('Skip'))!.description = 'No code/index change; Skip recorded.'; + const answer = await handler()('AskUserQuestion', ruled); + expect(answer.updatedInput.answers).toEqual({ [ruled.questions[0].question]: ruled.questions[0].options.find(option => option.label.startsWith('Skip'))!.label }); + expect(refusals).toHaveLength(1); + }); +}); + describe('shared-code legacy interactive actor', () => { test('R58 acknowledges the exact removed-filter Skip packet without authorizing its recommended fix', async () => { const input = {