fix(review): define what a Step 5c Skip option says

Step 5c named "B) Skip" without saying what its description may claim. Two
CI captures (path-eligibility on 131d43be, index-flags on 4643cb85) offered a
Skip whose description added effects beyond declining: "The extraction can be
applied in a later editing review pass" and "replacing the invalidated prior
Skip". Those read as change commitments, so the no-change actor refused both.
Step 5c now says to describe Skip only as no code/index change with the Skip
recorded; adjacent lines are compacted so the review parity caps hold
unchanged. Both exact packets are kept as a free regression: still refused,
and accepted once Skip follows the rule. The actor's classifier is unchanged.
This commit is contained in:
garrytan committed 2026-09-30 19:36:02 +00:00
1 parent cc044e5f89
commit f02636f05e
4 files changed
+86 -9

No files matched your search

+4 -4
View File
@@ -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
+4 -4
View File
@@ -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
@@ -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
}
]
}
}
]
}
+23 -1
View File
@@ -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 = {