diff --git a/packages/cli/src/commands/review/capture-local.incremental.test.ts b/packages/cli/src/commands/review/capture-local.incremental.test.ts index d4b5c851c59..ea02c12c65d 100644 --- a/packages/cli/src/commands/review/capture-local.incremental.test.ts +++ b/packages/cli/src/commands/review/capture-local.incremental.test.ts @@ -12,6 +12,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; import { execFileSync } from 'node:child_process'; +import { createHash } from 'node:crypto'; import { mkdtempSync, rmSync, @@ -477,6 +478,177 @@ describe('capture-local — round-2 regressions from the stop work', () => { ) as Record; expect(sidecar['runId']).toBe('run-abc'); }); + + it('stamps the fence’s binding fields — null hash when no cache was seen', () => { + // A first clean-tree stop saw no cache: null is the stampable value, + // and the compose fence fails closed on a cache file appearing since. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'all committed'); + const plan = capture(); + expect(plan['nothingToReview']).toEqual({ reason: 'clean-tree' }); + const sidecar = JSON.parse( + readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'), + ) as Record; + expect(sidecar['cachePath']).toBe(plan['cachePath']); + expect(sidecar['findingsHash']).toBeNull(); + }); + + it('stamps the cache a cached stop saw — the ledger’s content hash', () => { + // The compose grant re-hashes the cache the plan names and refuses on + // any departure, so a ledger edited between capture and compose fails + // closed like a foreign stamp. + seedDirtyTree(); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + recordOpenCritical(cachePath); + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second['nothingToReview']).toEqual({ + reason: 'unchanged-since-last-round', + }); + const sidecar = JSON.parse( + readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'), + ) as Record; + expect(sidecar['cachePath']).toBe(second['cachePath']); + expect(sidecar['findingsHash']).toBe( + createHash('sha256').update(readFileSync(cachePath)).digest('hex'), + ); + }); + + it('binds the ledger the stop DECIDED from — a file-form --cache outside the canonical dir', () => { + // The stop decision reads the `--cache`-resolved ledger; the stamp and + // the plan's published `cachePath` must name that same file. Stamping + // the canonical `.qwen/review-cache/…` path while the decision + // consulted a caller-named file had the fence verify a baseline the + // stop never saw — an ENOENT null hash over a nonexistent canonical + // file, an empty grant baseline, and an exit 0 over the open Critical + // the stop had just consumed. + seedDirtyTree(); + const canonical = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + // Outside the repo entirely, so the hand-named copy is not a new + // untracked file that would itself defeat the unchanged stop. + const outside = join(repo, '..', `hand-named-ledger-${Date.now()}.json`); + writeFileSync(outside, readFileSync(canonical)); + rmSync(canonical); + recordOpenCritical(outside); + const second = capture({ cache: outside, model: 'model-a' }); + expect(second['nothingToReview']).toEqual({ + reason: 'unchanged-since-last-round', + }); + // ONE resolved value for every consumer: plan, sidecar, and hash. + expect(second['cachePath']).toBe(outside); + const sidecar = JSON.parse( + readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'), + ) as Record; + expect(sidecar['cachePath']).toBe(outside); + expect(sidecar['findingsHash']).toBe( + createHash('sha256').update(readFileSync(outside)).digest('hex'), + ); + }); + + it('hashes the DECISION-time ledger bytes, not a second read at stamp time', async () => { + // A ledger edit landing in the decision→stamp window (a concurrent + // round's Step-8 rewrite of the shared file) must not be baked into + // the stamp: the stamp and the decision are projections of ONE read. + // The spy makes every cache read AFTER the first return bytes with the + // blocker dropped — with the fix the stamp still hashes the + // decision-time bytes; without it the stamp followed the second read. + const { readFileSync: realRead } = + await vi.importActual('node:fs'); + seedDirtyTree(); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + recordOpenCritical(cachePath); + const original = realRead(cachePath) as Buffer; + const expected = createHash('sha256').update(original).digest('hex'); + const mutatedCache = JSON.parse(original.toString('utf8')) as Record< + string, + unknown + >; + mutatedCache['findings'] = []; + const mutated = Buffer.from(JSON.stringify(mutatedCache)); + let cacheReads = 0; + vi.mocked(readFileSync).mockImplementation((( + path: unknown, + opts: unknown, + ) => { + if (path === cachePath) { + cacheReads++; + if (cacheReads > 1) { + return typeof opts === 'string' ? mutated.toString('utf8') : mutated; + } + } + return realRead( + path as Parameters[0], + opts as Parameters[1], + ); + }) as typeof readFileSync); + try { + const second = capture({ cache: cachePath, model: 'model-a' }); + expect(second['nothingToReview']).toEqual({ + reason: 'unchanged-since-last-round', + }); + } finally { + vi.mocked(readFileSync).mockRestore(); + } + const sidecar = JSON.parse( + readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'), + ) as Record; + expect(sidecar['findingsHash']).toBe(expected); + }); + + it('stamps the scope-emptied split into the sidecar beside the hash', () => { + // The `superseded` deduction reads membership off `supersededPaths`, + // and the plan copy is model-editable after this write — only the + // capture-stamped copy certifies the split, and the compose fence + // refuses a plan whose split departs from it. + seedDirtyTree(); + const cachePath = promoteCandidate( + capture({ model: 'model-a' }), + 'model-a', + ); + recordOpenCritical(cachePath); + git('checkout', '--', '.'); + const plan = capture({ cache: cachePath, model: 'model-a' }); + expect(plan['nothingToReview']).toEqual({ reason: 'scope-emptied' }); + const scope = ( + plan['incremental'] as { scope: { supersededPaths?: string[] } } + ).scope; + const sidecar = JSON.parse( + readFileSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json'), 'utf8'), + ) as Record; + expect(sidecar['supersededPaths']).toEqual(scope.supersededPaths); + expect((sidecar['supersededPaths'] as string[]).length).toBeGreaterThan(0); + }); + + it('unlinks a stale stop sidecar when a later capture proves the tree moved', () => { + // An earlier round's sidecar at this stable name stays fence-valid + // (same reason, same cache, same hash) after the tree moves on — a + // hand-written stop plan could ride it. A capture that decides NO stop + // removes it: absent is the truthful state. + seedDirtyTree(); + git('add', '-A'); + git('commit', '-q', '--no-verify', '-m', 'all committed'); + const stopped = capture(); + expect(stopped['nothingToReview']).toEqual({ reason: 'clean-tree' }); + expect( + existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')), + ).toBe(true); + // The tree moves; the next capture decides a real round. + writeFileSync(join(repo, CHANGED), 'export const moved = 1;\n'); + const moved = capture(); + expect(moved['nothingToReview']).toBeUndefined(); + expect( + existsSync(join(repo, '.qwen/tmp/qwen-review-local-stop.json')), + ).toBe(false); + }); }); describe('capture-local — a narrower round cannot certify a wider one', () => { diff --git a/packages/cli/src/commands/review/capture-local.ts b/packages/cli/src/commands/review/capture-local.ts index c572944b685..d206d08eb68 100644 --- a/packages/cli/src/commands/review/capture-local.ts +++ b/packages/cli/src/commands/review/capture-local.ts @@ -61,7 +61,8 @@ import { movedSince, hashWorktreeFiles, isPathProvablyAbsent, - readLocalCache, + type readLocalCache, + readLocalCacheFromBytes, revisionIdentities, stateIdOf, UNHASHABLE, @@ -736,8 +737,24 @@ function runCaptureLocal(args: CaptureLocalArgs): void { args.cache !== undefined ? resolveCachePath(args.cache, target, sourcePath) : null; + // ONE read of the ledger's bytes: the stop DECISION below parses this + // buffer and the stop stamp hashes the SAME buffer — a second disk read + // at stamp time let a concurrent round's ledger rewrite land in the + // decision→stamp window and be baked into the stamp, invisible to the + // compose fence (which then verified a baseline the decision never + // consulted). Raw bytes are kept beside the parse because the stamp is + // sha256 of the FILE's bytes, malformed JSON included — the parse + // fail-quiets, the hash must not. + let cacheEarlyBytes: Buffer | null = null; + if (cachePathEarly !== null) { + try { + cacheEarlyBytes = readFileSync(cachePathEarly); + } catch { + // No cache file — the decision sees no anchor and the stamp is null. + } + } const cacheEarly = - cachePathEarly === null ? null : readLocalCache(cachePathEarly); + cacheEarlyBytes === null ? null : readLocalCacheFromBytes(cacheEarlyBytes); const vanishedPresent: readonly string[] = cacheEarly === null ? [] @@ -1146,7 +1163,43 @@ function runCaptureLocal(args: CaptureLocalArgs): void { // so a parent polling `qwen-review--plan.json` found nothing for // every file review and reported "Review did not complete" over a decided // round. This name is derived from the same `target` the parent derives. + // ONE resolved value for every consumer: the stop DECISION above read the + // `--cache`-resolved ledger (`cachePathEarly` — a file-form `--cache` is + // returned unchanged, directory form resolves the canonical basename), so + // the stamp below and the plan's published `cachePath` must name that + // same file. Stamping the canonical `.qwen/review-cache/…` path while the + // decision consulted a caller-named file had the fence faithfully verify + // a baseline the stop never saw — an ENOENT hash over a nonexistent + // canonical file, an empty grant baseline, and an exit 0 over the open + // Critical the stop had just consumed. + const cachePath = cachePathEarly ?? cachePathFor(target, sourcePath); if (nothingToReview) { + // The baseline's content bound into the stamp: the compose grant + // re-hashes the cache the plan names and refuses on any departure, so + // a ledger edited between capture and compose fails closed like a + // foreign stamp. Null is a stampable value — no cache existed at this + // stop, so no findings were seen, and the fence fails closed on a file + // appearing since. The hash is of the DECISION-time bytes when a + // `--cache` scoped this round — stamp and decision are projections of + // the one read above, so an edit landing in the decision→stamp window + // cannot be baked into the stamp. Only the no-`--cache` canonical + // path still reads the disk here: that decision consulted no ledger, + // so there is no decision-time buffer to prefer. + let findingsHash: string | null = null; + if (cachePathEarly !== null) { + findingsHash = + cacheEarlyBytes === null + ? null + : createHash('sha256').update(cacheEarlyBytes).digest('hex'); + } else { + try { + findingsHash = createHash('sha256') + .update(readFileSync(cachePath)) + .digest('hex'); + } catch { + // No cache file at this stop. + } + } writeFileSync( tmpFile(target, 'stop.json'), `${JSON.stringify( @@ -1161,12 +1214,37 @@ function runCaptureLocal(args: CaptureLocalArgs): void { ...(process.env['QWEN_REVIEW_RUN_ID'] ? { runId: process.env['QWEN_REVIEW_RUN_ID'] } : {}), + // The compose fence's binding fields: the cache the grant must + // read, and the hash its content must still carry. + cachePath, + findingsHash, + // The scope-emptied split, capture-certified: the `superseded` + // deduction's input must be THIS list, and the plan it also + // rides in is model-editable after this write — a split edited + // between capture and compose could blanket-supersede a live + // blocker past a fence that binds only reason/cache/hash. + // Stamped in the interactive (no-run-id) shape too. + ...(nothingToReview.reason === 'scope-emptied' + ? { supersededPaths: incremental?.scope?.supersededPaths ?? [] } + : {}), }, null, 2, )}\n`, 'utf8', ); + } else { + // This capture proves the tree MOVED past whatever an earlier stop + // certified, so an earlier round's sidecar at this stable name is now + // a stale stamp: left in place, it stays fence-valid (same reason, + // same cache path, same hash if the ledger did not change) and a + // later hand-written stop plan could ride it. Absent IS the truthful + // state — this round decided no stop. + try { + unlinkSync(tmpFile(target, 'stop.json')); + } catch { + // nothing to remove + } } const diffPath = tmpFile(target, 'diff.txt'); @@ -1202,7 +1280,7 @@ function runCaptureLocal(args: CaptureLocalArgs): void { // diverges from any hand recipe). A round-2 medium review of // `srclink/foo.ts` predicted `srclink_foo.ts.json`, found nothing, and // ruled on zero ledger entries over a Critical that still stood. - cachePath: cachePathFor(target, sourcePath), + cachePath, cacheCandidatePath, ...(candidateWritten ? { cacheCandidateStateId: candidate.stateId } : {}), ...planEffortField(args.effort), diff --git a/packages/cli/src/commands/review/compose-review.test.ts b/packages/cli/src/commands/review/compose-review.test.ts index 9eb2c7d6894..41d81702996 100644 --- a/packages/cli/src/commands/review/compose-review.test.ts +++ b/packages/cli/src/commands/review/compose-review.test.ts @@ -15712,6 +15712,1201 @@ describe('draftedFindingsOf — the drafts as the convergence diagnosis reads th }); }); +describe('composeReview — the decided-stop re-rule', () => { + let cwd0: string; + beforeEach(() => { + cwd0 = process.cwd(); + process.chdir(dir); + // A sidecar stamped by one test must not vouch for the next: the fence + // now binds every stop compose (run id or not), so a leftover stamp is + // cross-test state. + for (const stem of ['local', 'other']) { + rmSync(join(dir, `.qwen/tmp/qwen-review-${stem}-stop.json`), { + force: true, + }); + } + }); + afterEach(() => { + process.chdir(cwd0); + }); + + function stopPlan( + opts: { + stop?: boolean; + ledger?: unknown[]; + name?: string; + reason?: string; + cacheFile?: string; + supersededPaths?: string[]; + prNumber?: number; + /** `false` leaves the sidecar to the test — the fence-shape tests. */ + sidecar?: boolean; + } = {}, + ): string { + const cachePath = join(dir, `review-cache-${opts.name ?? 'default'}.json`); + writeFileSync( + cachePath, + JSON.stringify({ + findings: opts.ledger ?? [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + title: 'the mechanism still fires — re-read at HEAD', + }, + { + id: 'R1-2', + severity: 'Critical', + status: 'fixed', + title: 'the patched hole', + }, + { + id: 'R1-3', + severity: 'Suggestion', + status: 'open', + title: 'the open suggestion', + }, + ], + }), + ); + const p = join(dir, `stop-plan-${opts.name ?? 'default'}.json`); + writeFileSync( + p, + JSON.stringify({ + chunks: [], + files: [], + diffLines: 0, + srcDiffLines: 0, + skippedFiles: [], + target: 'local', + cachePath: opts.cacheFile ?? cachePath, + ...(opts.prNumber !== undefined ? { prNumber: opts.prNumber } : {}), + ...(opts.supersededPaths + ? { + incremental: { + scope: { supersededPaths: opts.supersededPaths }, + }, + } + : {}), + ...(opts.stop === false + ? {} + : { + nothingToReview: { + reason: opts.reason ?? 'unchanged-since-last-round', + }, + }), + }), + ); + // The capture always leaves its sidecar beside a decided stop — the + // fence requires it with or without a published run id — so the plan + // fixture stamps the matching one (run-id-less, the interactive shape) + // unless the test owns the stamp itself. + if (opts.sidecar !== false && opts.stop !== false) { + stampStopSidecar({ + name: opts.name, + reason: opts.reason, + cacheFile: opts.cacheFile, + supersededPaths: opts.supersededPaths, + }); + } + return p; + } + + function reRule(over: Record = {}) { + return composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + env: ENV, + modelId: MODEL, + stopReRule: { + dispositions: [{ id: 'R1-1', ruling: 'still-stands' }], + }, + bodyCriticals: ['R1-1: the mechanism still fires — re-read at HEAD'], + // Built ONLY when the test does not bring its own: the default plan + // stamps the default sidecar, which would overwrite the stamp a + // caller-built plan (or the test itself) just wrote. + ...('planPath' in over ? {} : { planPath: stopPlan() }), + ...over, + }); + } + + function stampStopSidecar( + opts: { + name?: string; + reason?: string; + /** Omitted entirely when not given — the interactive capture's shape. */ + runId?: string; + target?: string; + cacheFile?: string; + supersededPaths?: string[]; + } = {}, + ): string { + const cachePath = + opts.cacheFile === '' + ? null + : (opts.cacheFile ?? + join(dir, `review-cache-${opts.name ?? 'default'}.json`)); + let findingsHash: string | null = null; + try { + if (cachePath !== null) { + findingsHash = createHash('sha256') + .update(readFileSync(cachePath)) + .digest('hex'); + } + } catch { + // A cache that does not exist stamps null — the fence re-hashes. + } + mkdirSync(join(dir, '.qwen/tmp'), { recursive: true }); + const sidecarPath = join( + dir, + `.qwen/tmp/qwen-review-${opts.target ?? 'local'}-stop.json`, + ); + const reason = opts.reason ?? 'unchanged-since-last-round'; + writeFileSync( + sidecarPath, + JSON.stringify({ + reason, + ...(opts.runId !== undefined ? { runId: opts.runId } : {}), + cachePath, + findingsHash, + // The capture stamps the split on every scope-emptied stop — the + // fence fails closed on its absence for that reason. + ...(reason === 'scope-emptied' + ? { supersededPaths: opts.supersededPaths ?? [] } + : {}), + }), + ); + return sidecarPath; + } + + it('composes REQUEST_CHANGES from a standing re-rule, floors skipped', () => { + const r = reRule(); + expect(r.event).toBe('REQUEEST_CHANGES'.replace('EE', 'E')); + expect(r.body).toContain('Decided-stop re-rule'); + expect(r.cappedBy).not.toContain('chunk-nobody-read'); + }); + + it('a re-rule that cleared every blocker COMMENTS, never approves', () => { + // A `fixed` ruling is licensed only under clean-tree — the judged stop — + // so the cleared shape rides one; over the deduced stops the same + // disposition is refused below. + const r = reRule({ + planPath: stopPlan({ name: 'cleared', reason: 'clean-tree' }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'fixed' }] }, + bodyCriticals: [], + }); + expect(r.event).toBe('COMMENT'); + // The opener may not certify a review that never ran: the cleared + // stop's COMMENT once opened 'Reviewed — no blockers.' two paragraphs + // above its own 'no review agents ran this round' disclosure. + expect(r.body).not.toContain('Reviewed — no blockers.'); + expect(r.body).not.toMatch(/^Reviewed\./); + expect(r.body).toContain( + 'Re-rule of standing findings — no new review ran.', + ); + }); + + it('refuses a full-round plan wearing the flag', () => { + expect(() => + reRule({ planPath: stopPlan({ stop: false, name: 'full' }) }), + ).toThrow(/no nothingToReview decision/); + }); + + it('renders the round-kind disclosure on its own line, never under "Not linted"', () => { + // The disclosure used to ride gateDisclosed, whose only renderer wraps + // every entry in "Not linted (tool limitation…)" — a round kind is not + // a linting gap. + const r = reRule(); + expect(r.body).toContain('Decided-stop re-rule'); + expect(r.body).not.toMatch(/Not linted[^\n]*Decided-stop re-rule/); + }); + + it('says why a cleared stop is a Comment — no dangling colon', () => { + // The stop demotion is the one APPROVE→COMMENT mover with empty + // cappedBy and no presubmit downgrade; joining the empty reason list + // printed 'an Approve was NOT available: ' over nothing. + const r = reRule({ + planPath: stopPlan({ name: 'cleared-line', reason: 'clean-tree' }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'fixed' }] }, + bodyCriticals: [], + }); + const line = verdictLine(r); + expect(line).toContain('reviews nothing new'); + expect(line).not.toMatch(/NOT available:\s*$/); + }); + + it('refuses a decided-stop plan composed WITHOUT stopReRule', () => { + // The mirror of the forged-flag refusal above: a stop plan walked + // through the regular floors would compose a non-blocking artifact, + // and `run.ts` reads any composed artifact as this round's completion + // — exit 0 over the ledger's standing blockers. + expect(() => + composeReview({ + criticalsInline: 0, + suggestionsInline: 0, + planPath: stopPlan({ name: 'no-rerule' }), + env: ENV, + modelId: MODEL, + }), + ).toThrow(/composes only through its re-rule/); + }); + + it('refuses a null stopReRule with the designed refusal', () => { + expect(() => reRule({ stopReRule: null })).toThrow( + /must be an object carrying dispositions/, + ); + }); + + it('refuses a ledger carrying two rows under one id', () => { + // Two open Criticals under ONE id collapse the set-based completeness + // check and the last-wins title/file maps into one disposition — the + // real blocker leaves the verdict lineage through its filler twin. A + // repeated id is an unreadable baseline like any other shape drift. + expect(() => + reRule({ + planPath: stopPlan({ + name: 'dup-id', + ledger: [ + { + id: 'R2-1', + severity: 'Critical', + status: 'open', + title: 'real blocker citing a.ts', + }, + { + id: 'R2-1', + severity: 'Critical', + status: 'open', + title: 'filler citing b.ts', + }, + ], + }), + stopReRule: { dispositions: [{ id: 'R2-1', ruling: 'still-stands' }] }, + bodyCriticals: ['R2-1: filler citing b.ts'], + }), + ).toThrow(/ledger the plan names cannot be read/); + }); + + it('refuses the model-written census on a stop re-rule — no minted blocker', () => { + // No agents ran, so nothing this round could have measured a + // fresh/induced split — yet a supplied census satisfied the + // fresh <= reported cross-check through the carried-id re-assertions + // the grant itself proves are NOT fresh, and minted the + // non-convergence blocker over a round that measured nothing. Refused + // like the round-0 and context-unavailable unmeasurable states: the + // streak carries. + writeFileSync( + join(dir, 'qwen-review-pr-8255-prev-ledger.json'), + JSON.stringify({ v: 1, findings: [], round: 2, churnRounds: 1 }), + ); + const ledger = [1, 2, 3, 4].map((i) => ({ + id: `R1-${i}`, + severity: 'Critical', + status: 'open', + title: `standing blocker ${i}`, + })); + const r = reRule({ + planPath: stopPlan({ name: 'census', ledger, prNumber: 8255 }), + stopReRule: { + dispositions: ledger.map((e) => ({ id: e.id, ruling: 'still-stands' })), + }, + bodyCriticals: ledger.map((e) => `${e.id}: ${e.title}`), + convergence: { fresh: 4, induced: 2 }, + }); + expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.body).not.toContain('is not converging'); + }); + + it('refuses a ledger row whose status drifts from the vocabulary', () => { + // `status: 'oppn'` used to skip the row silently — the baseline shrank + // below what the ledger really held, the completeness check passed + // over the shrunken set, and the blocker never re-asserted. A drifted + // status is an unreadable baseline, exactly like a drifted severity. + expect(() => + reRule({ + planPath: stopPlan({ + name: 'status-drift', + ledger: [{ id: 'R1-1', severity: 'Critical', status: 'oppn' }], + }), + stopReRule: { dispositions: [] }, + bodyCriticals: [], + }), + ).toThrow(/ledger the plan names cannot be read/); + }); + + it('refuses a Critical riding the deferral channel on a stop re-rule', () => { + // The floor's reroute moved a drafted Critical out of the inline set + // BEFORE the grant read the count, and the moved entry posted in the + // deferral list without ever reaching the body↔disposition bind. Both + // deferred legs — the reroute's and the model's own — are refused: on + // a stop round nothing new was reviewed, so a deferral-channel + // Critical can only be an unbound claim. + expect(() => + reRule({ + severityFloor: 'critical', + draftedComments: [ + { + path: 'src/new.ts', + line: 3, + body: '**[Critical]** [fails-closed] [new-surface] a brand-new blocker', + }, + ], + criticalsInline: 1, + }), + ).toThrow(/a Critical rides the deferral channel/); + expect(() => + reRule({ + severityFloor: 'critical', + deferredSuggestions: [ + { + file: 'src/new.ts', + line: 3, + source: 'review', + severity: 'Critical', + direction: 'fails-closed', + baseline: 'new-surface', + title: 'a deferred blocker the bind cannot reach', + }, + ], + }), + ).toThrow(/a Critical rides the deferral channel/); + }); + + it('does not let an unvouched relocated re-assertion’s source defeat the softening', () => { + // The relocated-leg twin of the tagged-unvouched test above: a + // title-less ledger entry re-asserted through the deferral channel + // with a deterministic `source` kept its deterministic credit, and an + // unverified blocker posted as an unsoftened REQUEST_CHANGES. + const r = reRule({ + planPath: stopPlan({ + name: 'reloc-unvouched', + ledger: [{ id: 'R1-1', severity: 'Critical', status: 'open' }], + }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'still-stands' }] }, + bodyCriticals: [], + deferredSuggestions: [ + { + file: 'src/wedge.ts', + line: 12, + source: 'test', + severity: 'Critical', + title: 'R1-1: the claim nobody recorded a title for', + }, + ], + }); + expect(r.event).toBe('COMMENT'); + expect(r.baseEvent).toBe('REQUEST_CHANGES'); + expect(r.cappedBy).toContain('criticals-unverified'); + }); + + it('binds the relocated leg’s COLLAPSED title — a multi-line tail cannot smuggle', () => { + // First line matches the recorded claim verbatim; the tail carries a + // brand-new claim. A first-line-only readback passed it and the ledger + // builder recorded only line 1, so no future round would ever rule on + // the tail. + expect(() => + reRule({ + planPath: stopPlan({ + name: 'reloc-tail', + reason: 'clean-tree', + ledger: [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + title: 'the mechanism still fires — re-read at HEAD', + }, + ], + }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'still-stands' }] }, + bodyCriticals: [], + deferredSuggestions: [ + { + file: 'src/wedge.ts', + line: 12, + source: 'review', + severity: 'Critical', + title: + 'R1-1: the mechanism still fires — re-read at HEAD\n\nA brand-new claim nobody verified', + }, + ], + }), + ).toThrow(/re-asserted with content that departs/); + }); + + it('keeps a granted stop’s REQUEST_CHANGES past a presubmit downgrade flag', () => { + // No presubmit ran on a stop round (no agents did), so the flag can + // only be stale or forged — it was the one softening channel the grant + // did not machine-check, and it moved a certified-standing blocker to + // COMMENT under `--fail-on request-changes`. + const r = reRule({ + presubmit: { downgradeRequestChanges: true, reasons: [] }, + }); + expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.downgraded).toBe(false); + }); + + it('refuses when an open ledger Critical has no disposition', () => { + expect(() => + reRule({ + planPath: stopPlan({ + name: 'two-open', + ledger: [ + { id: 'R1-1', severity: 'Critical', status: 'open' }, + { id: 'R2-9', severity: 'Critical', status: 'open' }, + ], + }), + }), + ).toThrow(/R2-9 has no disposition/); + }); + + it('refuses a disposition that matches no open ledger Critical', () => { + expect(() => + reRule({ + stopReRule: { + dispositions: [ + { id: 'R1-1', ruling: 'still-stands' }, + { id: 'R9-9', ruling: 'fixed' }, + ], + }, + }), + ).toThrow(/R9-9 matches no open ledger Critical/); + }); + + it('refuses still-stands without its body Critical, and fixed with one', () => { + const cleared = () => + stopPlan({ name: 'body-check', reason: 'clean-tree' }); + expect(() => reRule({ planPath: cleared(), bodyCriticals: [] })).toThrow( + /still-stands but no body Critical/, + ); + expect(() => + reRule({ + planPath: cleared(), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'fixed' }] }, + }), + ).toThrow(/ruled fixed yet a body Critical/); + }); + + it('honours the runId fence when a parent published one', () => { + const env = { ...ENV, QWEN_REVIEW_RUN_ID: 'run-X' }; + const planPath = stopPlan({ sidecar: false }); + expect(() => reRule({ env, planPath })).toThrow(/no stop sidecar/); + // A run-id-less stamp is not this run's stamp either. + stampStopSidecar({}); + expect(() => reRule({ env, planPath })).toThrow(/no stop sidecar/); + stampStopSidecar({ runId: 'run-X' }); + const r = reRule({ env, planPath }); + expect(r.event).toBe('REQUEST_CHANGES'); + }); + + it('binds the sidecar with no published run id — the interactive fence', () => { + // No run id waives only the run-id equality: the sidecar itself, its + // reason, cache path, and findings hash still bind. Skipping the fence + // outright left every interactive grant gated by nothing but + // model-supplied inputs — a hand-authored plan + ledger with no capture + // behind them composed a floor-exempt verdict. + const planPath = stopPlan({ sidecar: false }); + expect(() => reRule({ planPath })).toThrow(/no stop sidecar/); + // A stamp for a departed reason does not vouch either. + stampStopSidecar({ reason: 'clean-tree' }); + expect(() => reRule({ planPath })).toThrow(/records reason/); + // The capture's own stamp binds — a stamped run id is ignored here. + stampStopSidecar({ runId: 'run-ELSEWHERE' }); + expect(reRule({ planPath }).event).toBe('REQUEST_CHANGES'); + stampStopSidecar({}); + expect(reRule({ planPath }).event).toBe('REQUEST_CHANGES'); + }); + + it('refuses a fixed ruling under unchanged-since-last-round — a byte-identical tree can only still-stand', () => { + expect(() => + reRule({ + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'fixed' }] }, + bodyCriticals: [], + }), + ).toThrow(/R1-1 is ruled fixed under unchanged-since-last-round/); + }); + + it('refuses a fixed ruling under scope-emptied, admits superseded', () => { + expect(() => + reRule({ + planPath: stopPlan({ name: 'emptied', reason: 'scope-emptied' }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'fixed' }] }, + bodyCriticals: [], + }), + ).toThrow(/R1-1 is ruled fixed under scope-emptied/); + const r = reRule({ + planPath: stopPlan({ + name: 'emptied-ok', + reason: 'scope-emptied', + ledger: [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + file: 'src/gone.ts', + title: 'the mechanism still fires — re-read at HEAD', + }, + ], + supersededPaths: ['src/gone.ts'], + }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'superseded' }] }, + bodyCriticals: [], + }); + expect(r.event).toBe('COMMENT'); + }); + + it('machine-checks a superseded deduction against the published split', () => { + // `scope-emptied` licences `superseded` as a DEDUCED ruling, and the + // deduction's input is the capture's `supersededPaths`: a superseded + // whose cited file is still live — or whose row records no file at all + // — is a judgement wearing a deduction's licence, and retires a live + // blocker silently. + expect(() => + reRule({ + planPath: stopPlan({ + name: 'emptied-live-file', + reason: 'scope-emptied', + ledger: [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + file: 'src/live.ts', + title: 'the mechanism still fires — re-read at HEAD', + }, + ], + supersededPaths: [], + }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'superseded' }] }, + bodyCriticals: [], + }), + ).toThrow(/not in the plan's supersededPaths/); + expect(() => + reRule({ + planPath: stopPlan({ + name: 'emptied-no-file', + reason: 'scope-emptied', + ledger: [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + title: 'the mechanism still fires — re-read at HEAD', + }, + ], + supersededPaths: ['src/gone.ts'], + }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'superseded' }] }, + bodyCriticals: [], + }), + ).toThrow(/the ledger records no file for it/); + }); + + it('does not let an unvouched re-assertion’s tag defeat the unverified softening', () => { + // A title-less ledger entry binds on id alone; its re-assertion is + // unvouched, and on a granted stop no tool ran this round — so a + // `[test]` substring on it is prose, not provenance, and may not feed + // the deterministic exception that keeps a Request changes hard. + const r = reRule({ + planPath: stopPlan({ + name: 'tagged-unvouched', + ledger: [{ id: 'R1-1', severity: 'Critical', status: 'open' }], + }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'still-stands' }] }, + bodyCriticals: ['R1-1: [test] the claim'], + }); + expect(r.event).toBe('COMMENT'); + expect(r.baseEvent).toBe('REQUEST_CHANGES'); + expect(r.cappedBy).toContain('criticals-unverified'); + }); + + it('keeps a vouched re-assertion’s deterministic tag', () => { + // The exception stays for entries the ledger's recorded title vouched + // for: they re-assert findings a full round verified, tag and all. + const title = '[test] the mechanism still fires — re-read at HEAD'; + const r = reRule({ + planPath: stopPlan({ + name: 'tagged-vouched', + ledger: [{ id: 'R1-1', severity: 'Critical', status: 'open', title }], + }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'still-stands' }] }, + bodyCriticals: [`R1-1: ${title}`], + }); + expect(r.event).toBe('REQUEST_CHANGES'); + }); + + it('refuses an unknown stop reason — the grant fails closed', () => { + expect(() => + reRule({ + planPath: stopPlan({ name: 'odd', reason: 'model-invented' }), + }), + ).toThrow(/unknown stop reason/); + }); + + it('a still-stands re-assertion composes when a fixed sibling id prefixes it', () => { + // The old substring check matched `R1-1` inside `R1-10: …` and threw + // 'ruled fixed yet a body Critical still carries its id' on this fully + // compliant re-rule — every retry unsatisfiable. Per-entry id binding + // reads each entry's OWN leading token. + const r = reRule({ + planPath: stopPlan({ + name: 'prefix', + reason: 'clean-tree', + ledger: [ + { id: 'R1-1', severity: 'Critical', status: 'open' }, + { + id: 'R1-10', + severity: 'Critical', + status: 'open', + title: 'the mechanism still fires — re-read at HEAD', + }, + ], + }), + stopReRule: { + dispositions: [ + { id: 'R1-1', ruling: 'fixed' }, + { id: 'R1-10', ruling: 'still-stands' }, + ], + }, + bodyCriticals: ['R1-10: the mechanism still fires — re-read at HEAD'], + }); + expect(r.event).toBe('REQUEST_CHANGES'); + }); + + it('refuses a still-stands id present only inside another entry’s prose', () => { + expect(() => + reRule({ + planPath: stopPlan({ + name: 'prose', + reason: 'clean-tree', + ledger: [ + { id: 'R2-5', severity: 'Critical', status: 'open' }, + { id: 'R3-7', severity: 'Critical', status: 'open' }, + ], + }), + stopReRule: { + dispositions: [ + { id: 'R2-5', ruling: 'still-stands' }, + { id: 'R3-7', ruling: 'still-stands' }, + ], + }, + bodyCriticals: ['R3-7: the gap remains — see R2-5 for context'], + }), + ).toThrow(/R2-5 is ruled still-stands but no body Critical/); + }); + + it('refuses a relocated Critical titled with an id ruled fixed', () => { + // The deferral channel's Criticals are relocated into the FINAL body + // set, so the cross-check must see them too: one titled with an id the + // re-rule judged fixed is exactly the blocker the grant would post + // against its own ruling. + expect(() => + reRule({ + planPath: stopPlan({ name: 'reloc', reason: 'clean-tree' }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'fixed' }] }, + bodyCriticals: [], + deferredSuggestions: [ + { + file: 'src/wedge.ts', + line: 12, + source: 'review', + severity: 'Critical', + title: 'R1-1: the blocker the deferral channel carried', + }, + ], + }), + ).toThrow(/R1-1 is ruled fixed yet a body Critical/); + }); + + it('refuses an invented body Critical carrying no ledger id', () => { + expect(() => + reRule({ + planPath: stopPlan({ + name: 'invented', + reason: 'clean-tree', + ledger: [{ id: 'R1-1', severity: 'Critical', status: 'fixed' }], + }), + stopReRule: { dispositions: [] }, + bodyCriticals: ['a brand-new blocker no round ever ruled on'], + }), + ).toThrow(/must carry exactly one still-stands ledger id/); + }); + + it('refuses two body entries re-asserting one still-stands id', () => { + // Both entries verbatim-match the recorded title, so the content + // binding admits them and the COUNT check is what refuses: two + // re-assertions of one still-stands ruling would post the blocker + // twice. + expect(() => + reRule({ + bodyCriticals: [ + 'R1-1: the mechanism still fires — re-read at HEAD', + 'R1-1: the mechanism still fires — re-read at HEAD', + ], + }), + ).toThrow(/exactly one body Critical/); + }); + + it('refuses inline Criticals on a granted stop round', () => { + expect(() => reRule({ criticalsInline: 1 })).toThrow( + /inline Criticals cannot ride a stop re-rule/, + ); + }); + + it('refuses a (fix-induced) marking riding a stop re-rule', () => { + // The marking says NEW work under an old id; a stop re-rule posts only + // re-assertions of verified findings — nothing was reviewed this round + // that could have induced a fix-induced defect. + expect(() => + reRule({ + bodyCriticals: [ + 'R1-1 (fix-induced): the mechanism still fires — re-read at HEAD', + ], + }), + ).toThrow(/carries the \(fix-induced\) marking/); + }); + + it('refuses a plan whose supersededPaths depart from the stamped split', () => { + // The fence bound reason/cache/hash but not the split — a plan edited + // AFTER the capture stamped could blanket-supersede a live blocker + // through the one ruling channel the fence did not bind. + const planPath = stopPlan({ + name: 'forged-split', + reason: 'scope-emptied', + ledger: [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + file: 'src/live.ts', + title: 'the mechanism still fires — re-read at HEAD', + }, + ], + supersededPaths: [], + }); + // The model edits the PLAN's split after the stamp; the sidecar still + // certifies the empty one. + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as Record< + string, + unknown + >; + plan['incremental'] = { scope: { supersededPaths: ['src/live.ts'] } }; + writeFileSync(planPath, JSON.stringify(plan)); + expect(() => + reRule({ + planPath, + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'superseded' }] }, + bodyCriticals: [], + }), + ).toThrow(/depart from the split the capture stamped/); + }); + + it('consumes an interactive sidecar on grant — a replay is refused', () => { + // #10654's interim hardening: nothing else ever reads a no-run-id + // sidecar, and left on disk it re-licences the same plan on a later, + // moved tree. Consumed only after the FULL grant — a refusal leaves it + // for the corrected retry — and never under a published run id, where + // the parent still reads it for completion. + const planPath = stopPlan({ name: 'consume' }); + expect(reRule({ planPath }).event).toBe('REQUEST_CHANGES'); + expect(() => reRule({ planPath })).toThrow(/no stop sidecar/); + // A refused grant leaves the sidecar in place for the retry. + const planPath2 = stopPlan({ name: 'consume-retry' }); + expect(() => + reRule({ + planPath: planPath2, + stopReRule: { dispositions: [] }, + bodyCriticals: [], + }), + ).toThrow(/has no disposition/); + expect(reRule({ planPath: planPath2 }).event).toBe('REQUEST_CHANGES'); + // Under a published run id the sidecar stays for the parent. + const env = { ...ENV, QWEN_REVIEW_RUN_ID: 'run-X' }; + const planPath3 = stopPlan({ name: 'consume-gated', sidecar: false }); + stampStopSidecar({ name: 'consume-gated', runId: 'run-X' }); + expect(reRule({ planPath: planPath3, env }).event).toBe('REQUEST_CHANGES'); + expect(reRule({ planPath: planPath3, env }).event).toBe('REQUEST_CHANGES'); + }); + + it('refuses a sidecar that parses to null with the designed refusal', () => { + const planPath = stopPlan({ name: 'null-sidecar', sidecar: false }); + mkdirSync(join(dir, '.qwen/tmp'), { recursive: true }); + writeFileSync(join(dir, '.qwen/tmp/qwen-review-local-stop.json'), 'null'); + expect(() => reRule({ planPath })).toThrow(/no stop sidecar/); + }); + + it('refuses a cache file that APPEARED after a null-hash stamp', () => { + // Null is a stampable value — no cache existed at the stop — and the + // fence must fail closed on a file appearing since, not read it as an + // admitted empty baseline. + const missing = join(dir, 'appearing-cache.json'); + rmSync(missing, { force: true }); + const planPath = stopPlan({ name: 'appearing', cacheFile: missing }); + writeFileSync( + missing, + JSON.stringify({ + findings: [{ id: 'R9-9', severity: 'Critical', status: 'open' }], + }), + ); + expect(() => + reRule({ planPath, stopReRule: { dispositions: [] }, bodyCriticals: [] }), + ).toThrow(/not the ones the capture stamped/); + }); + + it('refuses a sidecar stamped by a different run', () => { + const planPath = stopPlan({ sidecar: false }); + stampStopSidecar({ runId: 'run-OLD' }); + expect(() => + reRule({ env: { ...ENV, QWEN_REVIEW_RUN_ID: 'run-X' }, planPath }), + ).toThrow(/no stop sidecar/); + }); + + it('refuses a stamped sidecar whose stem differs from the plan’s target', () => { + // The fence reads the ONE sidecar the plan's target names — a stamp + // vouching for another target (the old family scan admitted it) must + // not vouch for this re-rule. + const planPath = stopPlan({ sidecar: false }); + stampStopSidecar({ runId: 'run-X', target: 'other' }); + expect(() => + reRule({ env: { ...ENV, QWEN_REVIEW_RUN_ID: 'run-X' }, planPath }), + ).toThrow(/no stop sidecar/); + }); + + it('refuses a same-stem sidecar whose reason departs from the plan’s', () => { + // The licence-bearing reason is the capture's: a plan claiming a + // wider-licencing reason than the sidecar recorded must not ride it. + const planPath = stopPlan({ sidecar: false }); + stampStopSidecar({ runId: 'run-X', reason: 'clean-tree' }); + expect(() => + reRule({ env: { ...ENV, QWEN_REVIEW_RUN_ID: 'run-X' }, planPath }), + ).toThrow(/records reason/); + }); + + it('refuses a sidecar naming a different cache than the plan', () => { + const planPath = stopPlan({ sidecar: false }); + stampStopSidecar({ + runId: 'run-X', + cacheFile: join(dir, 'another-cache.json'), + }); + expect(() => + reRule({ env: { ...ENV, QWEN_REVIEW_RUN_ID: 'run-X' }, planPath }), + ).toThrow(/names a different cache/); + }); + + it('refuses when the ledger moved between capture and compose', () => { + const env = { ...ENV, QWEN_REVIEW_RUN_ID: 'run-X' }; + const planPath = stopPlan({ name: 'moved' }); + stampStopSidecar({ name: 'moved', runId: 'run-X' }); + // Control: the untouched ledger composes. + expect(reRule({ env, planPath }).event).toBe('REQUEST_CHANGES'); + // Tamper: a phantom open Critical appended after the stamp. + const cachePath = join(dir, 'review-cache-moved.json'); + const cache = JSON.parse(readFileSync(cachePath, 'utf8')) as { + findings: unknown[]; + }; + cache.findings.push({ + id: 'R9-9', + severity: 'Critical', + status: 'open', + title: 'phantom', + }); + writeFileSync(cachePath, JSON.stringify(cache)); + expect(() => reRule({ env, planPath })).toThrow( + /not the ones the capture stamped/, + ); + }); + + it('refuses a plan carrying no usable target under a published run id', () => { + const planPath = stopPlan({ name: 'no-target' }); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as Record< + string, + unknown + >; + delete plan['target']; + writeFileSync(planPath, JSON.stringify(plan)); + stampStopSidecar({ name: 'no-target', runId: 'run-X' }); + expect(() => + reRule({ env: { ...ENV, QWEN_REVIEW_RUN_ID: 'run-X' }, planPath }), + ).toThrow(/no usable target/); + }); + + it('refuses a superseded ruling that re-asserts its body Critical', () => { + expect(() => + reRule({ + planPath: stopPlan({ name: 'superseded-body', reason: 'clean-tree' }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'superseded' }] }, + }), + ).toThrow(/R1-1 is ruled superseded yet a body Critical/); + }); + + it('refuses when the plan names no readable ledger', () => { + expect(() => + reRule({ + planPath: stopPlan({ name: 'no-cache', cacheFile: '' }), + }), + ).toThrow(/ledger the plan names cannot be read/); + }); + + it('treats a cache that does not exist as an empty ledger, not an unreadable one', () => { + // A decided stop whose round never cached anything composes its + // no-event verdict over zero entries; the fence's findings hash binds + // the absence on the run-fenced path. + const planPath = stopPlan({ + name: 'gone-cache', + cacheFile: join(dir, 'no-such-cache.json'), + }); + expect(() => reRule({ planPath })).toThrow( + /R1-1 matches no open ledger Critical/, + ); + const r = reRule({ + planPath, + stopReRule: { dispositions: [] }, + bodyCriticals: [], + }); + expect(r.event).toBe('COMMENT'); + }); + + it('refuses a duplicate disposition for one id', () => { + expect(() => + reRule({ + stopReRule: { + dispositions: [ + { id: 'R1-1', ruling: 'still-stands' }, + { id: 'R1-1', ruling: 'fixed' }, + ], + }, + }), + ).toThrow(/duplicate disposition for R1-1/); + }); + + it('refuses a cache whose findings field is present but not an array', () => { + // A parseable cache with a non-array `findings` is a baseline that + // could not be read, not an empty ledger: over it the completeness + // check would certify a ruling set of nothing. + expect(() => + reRule({ + planPath: stopPlan({ + name: 'findings-not-array', + ledger: 'R1-1 open' as unknown as unknown[], + }), + }), + ).toThrow(/ledger the plan names cannot be read/); + }); + + it('refuses a ledger holding an entry that violates the schema', () => { + // The cache is model-written, so a drifting entry is an unreadable + // baseline, never a row to skip: skipping would shrink the open set + // below what the ledger really holds, and the grant would issue over + // Criticals it could not enumerate. + const drifts: unknown[][] = [ + [{ id: 'R2-1', severity: 'critical', status: 'open' }], + [{ id: 'R2-1', severity: 'Critical' }], + [null], + ]; + drifts.forEach((ledger, i) => { + expect(() => + reRule({ + planPath: stopPlan({ name: `schema-drift-${i}`, ledger }), + }), + ).toThrow(/ledger the plan names cannot be read/); + }); + }); + + it('refuses a still-stands re-assertion whose content departs from the recorded title', () => { + // The id alone would let a brand-new claim wear a verified id's + // exemption; the content the ledger recorded under the id is the + // contract a re-assertion is bound by. + expect(() => + reRule({ + planPath: stopPlan({ + name: 'fabricated', + ledger: [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + title: 'the mechanism still fires — re-read at HEAD', + }, + ], + }), + bodyCriticals: ['R1-1: a brand-new claim nobody ever verified'], + }), + ).toThrow(/re-asserted with content that departs/); + }); + + it('a re-assertion the ledger recorded no title for loses the verify-floor exemption', () => { + // No recorded content to bind against — the re-assertion cannot be + // SHOWN, so its Critical rides the regular floor: disclosed, not + // blocking. + const r = reRule({ + planPath: stopPlan({ + name: 'untitled', + ledger: [{ id: 'R1-1', severity: 'Critical', status: 'open' }], + }), + }); + expect(r.event).toBe('COMMENT'); + expect(r.baseEvent).toBe('REQUEST_CHANGES'); + expect(r.cappedBy).toContain('criticals-unverified'); + }); + + it('refuses a prototype-chain stop reason with the designed refusal', () => { + // A model-written reason indexes the ruling table: a prototype key + // must fail closed as an UNKNOWN reason, not crash on the prototype's + // members. + for (const reason of [ + '__proto__', + 'constructor', + 'toString', + 'hasOwnProperty', + ]) { + expect(() => + reRule({ planPath: stopPlan({ name: `proto-${reason}`, reason }) }), + ).toThrow(/unknown stop reason/); + } + }); + + it('composes a stop round holding two still-standing Criticals', () => { + // The shape the grant exists for — open Criticals accumulate across + // rounds and every one re-asserts. N=1 alone would let a + // first-entry-only binding loop ship green. + const r = reRule({ + planPath: stopPlan({ + name: 'two-standing', + ledger: [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + title: 'the mechanism still fires — re-read at HEAD', + }, + { + id: 'R2-5', + severity: 'Critical', + status: 'open', + title: 'the second gap remains', + }, + ], + }), + stopReRule: { + dispositions: [ + { id: 'R1-1', ruling: 'still-stands' }, + { id: 'R2-5', ruling: 'still-stands' }, + ], + }, + bodyCriticals: [ + 'R1-1: the mechanism still fires — re-read at HEAD', + 'R2-5: the second gap remains', + ], + }); + expect(r.event).toBe('REQUEST_CHANGES'); + expect(r.body).toContain('R1-1'); + expect(r.body).toContain('R2-5'); + }); + + it('binds a re-assertion that opens with invisible residue', () => { + // The leading strip is what lets a ZWSP/BOM-class residue bind at + // all: without it the id readback fails and the grant refuses a + // valid re-rule. + const r = reRule({ + bodyCriticals: [ + '\u200bR1-1: the mechanism still fires — re-read at HEAD', + ], + }); + expect(r.event).toBe('REQUEST_CHANGES'); + }); + + it('refuses a superseded ruling under unchanged-since-last-round', () => { + // A byte-identical tree replaced nothing, so `superseded` is a + // forged disposition there — the licence table's refusal cell. + expect(() => + reRule({ + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'superseded' }] }, + bodyCriticals: [], + }), + ).toThrow(/R1-1 is ruled superseded under unchanged-since-last-round/); + }); + + it('checks the per-reason licence for every disposition, not only the first', () => { + expect(() => + reRule({ + planPath: stopPlan({ + name: 'emptied-two', + reason: 'scope-emptied', + ledger: [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + title: 'the mechanism still fires — re-read at HEAD', + }, + { + id: 'R2-2', + severity: 'Critical', + status: 'open', + title: 'the second mechanism', + }, + ], + }), + stopReRule: { + dispositions: [ + { id: 'R1-1', ruling: 'still-stands' }, + { id: 'R2-2', ruling: 'fixed' }, + ], + }, + bodyCriticals: ['R1-1: the mechanism still fires — re-read at HEAD'], + }), + ).toThrow(/R2-2 is ruled fixed under scope-emptied/); + }); + + it('binds the relocated leg by content, not by id alone', () => { + // The deferral channel's relocated Criticals bind through the SAME + // content check as the ingested entries: one carrying a still-stands + // id with fabricated text is refused exactly like its own-leg twin. + expect(() => + reRule({ + planPath: stopPlan({ + name: 'reloc-fabricated', + reason: 'clean-tree', + ledger: [ + { + id: 'R1-1', + severity: 'Critical', + status: 'open', + title: 'the mechanism still fires — re-read at HEAD', + }, + ], + }), + stopReRule: { dispositions: [{ id: 'R1-1', ruling: 'still-stands' }] }, + bodyCriticals: [], + deferredSuggestions: [ + { + file: 'src/wedge.ts', + line: 12, + source: 'review', + severity: 'Critical', + title: 'R1-1: a different claim entirely', + }, + ], + }), + ).toThrow(/re-asserted with content that departs/); + }); + + it('refuses a cache that is not an object at all', () => { + // A bare-array cache file is unreadable the same way: the grant must + // not read it as an empty ledger. Re-stamped after the rewrite so the + // fence passes and the SHAPE check is what refuses. + const planPath = stopPlan({ name: 'not-object' }); + const plan = JSON.parse(readFileSync(planPath, 'utf8')) as { + cachePath: string; + }; + writeFileSync(plan.cachePath, JSON.stringify([1, 2])); + stampStopSidecar({ name: 'not-object' }); + expect(() => reRule({ planPath })).toThrow( + /ledger the plan names cannot be read/, + ); + }); +}); + describe('Critical deferral by axes at the critical floor (#10291)', () => { // The severity bit carried three decision axes, so past the convergence // rounds everything that mattered still landed on the floor and the floor diff --git a/packages/cli/src/commands/review/compose-review.ts b/packages/cli/src/commands/review/compose-review.ts index db6c4af8f94..dddd0abfd33 100644 --- a/packages/cli/src/commands/review/compose-review.ts +++ b/packages/cli/src/commands/review/compose-review.ts @@ -22,7 +22,9 @@ import type { CommandModule } from 'yargs'; import { certifierMatchesRound, roundModelIdFrom } from './lib/round-model.js'; -import { mkdirSync, readFileSync, writeFileSync } from 'node:fs'; +import { mkdirSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs'; +import { createHash } from 'node:crypto'; +import { tmpFile } from './lib/paths.js'; import { dirname, join } from 'node:path'; import { writeStdoutLine, writeStderrLine } from '../../utils/stdioHelpers.js'; import { getCliVersion } from '../../utils/version.js'; @@ -191,6 +193,338 @@ const BODY_MAX_CHARS = 65536; const MARKER_RESERVE = LEDGER_MAX_BYTES + 2; const BODY_SAFETY_MARGIN = 512; +/** + * Everything the decided-stop grant reads off the plan and the cache it + * names, taken in ONE read each. The grant used to re-read both files per + * consumer — the fence hashed the cache in one `readFileSync` and the + * ledger enumeration read it again in another — and nothing bound the bytes + * the fence certified to the bytes the grant enumerated: a writer + * alternating the model-writable cache between the stamped state and an + * emptied one won that race in a measured probe. The snapshot is the one + * owner of the bytes for the whole grant; the hash and the enumeration are + * projections of the same buffer. + * + * `reason` is the capture's own decided-stop reason — the + * `nothingToReview.reason` field `capture-local` writes when the round is + * one of the three decided stops — or null when the plan carries no + * decision. The FIELD is the capture's own: no full-round plan carries it, + * so a model-written `stopReRule` on a full round finds nothing here and is + * refused. (The path arrives through the model-written state — the same + * seam every other `planPath` reader here trusts.) The REASON certifies + * what could have moved since the ledger round, and the grant's per-reason + * ruling constraints read that certification. + */ +interface StopSnapshot { + reason: string | null; + target: string | null; + cachePath: string | null; + /** + * The scope-emptied split key the capture published — the paths whose + * recorded change is gone. A `superseded` disposition is deduced ONLY + * from membership here; absent or empty licences none. + */ + supersededPaths: readonly string[]; + cache: + | { kind: 'no-path' } + | { kind: 'missing' } + | { kind: 'unreadable' } + | { kind: 'bytes'; bytes: Buffer }; +} + +function readStopSnapshot(planPath: string | undefined): StopSnapshot { + const empty: StopSnapshot = { + reason: null, + target: null, + cachePath: null, + supersededPaths: [], + cache: { kind: 'no-path' }, + }; + if (!planPath) return empty; + let plan: { + nothingToReview?: unknown; + target?: unknown; + cachePath?: unknown; + incremental?: { scope?: { supersededPaths?: unknown } }; + }; + try { + plan = JSON.parse(readFileSync(planPath, 'utf8')) as typeof plan; + } catch { + return empty; + } + if (typeof plan !== 'object' || plan === null) return empty; + const stop = plan.nothingToReview; + const reason = + typeof stop === 'object' && + stop !== null && + typeof (stop as { reason?: unknown }).reason === 'string' && + (stop as { reason: string }).reason !== '' + ? (stop as { reason: string }).reason + : null; + const target = + typeof plan.target === 'string' && plan.target !== '' ? plan.target : null; + const cachePath = + typeof plan.cachePath === 'string' && plan.cachePath !== '' + ? plan.cachePath + : null; + const rawSuperseded = plan.incremental?.scope?.supersededPaths; + const supersededPaths = Array.isArray(rawSuperseded) + ? rawSuperseded.filter((p): p is string => typeof p === 'string') + : []; + let cache: StopSnapshot['cache'] = { kind: 'no-path' }; + if (cachePath !== null) { + try { + cache = { kind: 'bytes', bytes: readFileSync(cachePath) }; + } catch (err) { + // A cache that does not exist recorded no findings — an EMPTY + // baseline, not an unreadable one. Every other read failure is + // unreadable: the grant must refuse, never enumerate a guess. + cache = + (err as NodeJS.ErrnoException).code === 'ENOENT' + ? { kind: 'missing' } + : { kind: 'unreadable' }; + } + } + return { reason, target, cachePath, supersededPaths, cache }; +} + +/** + * The rulings each decided-stop reason licences — the capture's own + * certification of what could have moved since the ledger round (SKILL Step + * 1's stop branches prescribe the same split): `unchanged-since-last-round` + * certifies a byte-identical tree where every open finding stands VERBATIM + * (dispositions are DEDUCED, not judged); `scope-emptied` certifies each + * anchored path removed or byte-identical, so a finding stands or its bytes + * superseded it — nothing was reviewed that could fix; `clean-tree` + * certifies nothing moved since the findings were recorded, so the re-rule + * JUDGES them. A reason this table does not name licences nothing — the + * grant fails closed on it. + */ +const STOP_REASON_RULINGS: Record = { + 'unchanged-since-last-round': ['still-stands'], + 'scope-emptied': ['still-stands', 'superseded'], + 'clean-tree': ['still-stands', 'fixed', 'superseded'], +}; + +/** + * Why a reason's licence is narrower than the full ruling set — the refusal + * line's second half. `clean-tree` carries no entry: every ruling is + * licensed there, so no refusal is ever built for it. + */ +const STOP_REASON_REFUSAL: Record = { + 'unchanged-since-last-round': 'a byte-identical tree can only still-stand', + 'scope-emptied': + 'an emptied scope still-stands or supersedes — nothing was reviewed that could fix', +}; + +/** + * The cache ledger's bytes bound into the stop fence — the SHA-256 of the + * snapshot's bytes, or null when there is no file to hash. A cache that + * does not exist holds no findings, so null IS a stampable value: the + * capture stamps it when nothing was cached, and the grant fails closed on + * a file that appeared since. Computed from the SNAPSHOT, never a second + * disk read: the hash the fence certifies and the ledger the grant + * enumerates must be projections of one buffer (the TOCTOU the snapshot + * exists to close). + */ +function cacheFindingsHash(cache: StopSnapshot['cache']): string | null { + if (cache.kind !== 'bytes') return null; + return createHash('sha256').update(cache.bytes).digest('hex'); +} + +/** + * The fence `run.ts` applies to the same decided-stop decision, read + * against the ONE sidecar the capture could have stamped for THIS plan — + * never the family: a family scan let a sidecar stamped for another target + * vouch for this one. The fence binds what it finds three ways — the run + * id the parent published (when one is), the plan's own stop reason (the + * licence-bearing field is the capture's, not the plan's; `run.ts`'s + * `readStopSidecar` reads it from the sidecar too), and the cache ledger's + * content hash the capture stamped at stop time, so the grant's baseline + * is the ledger the capture saw. With NO published id (an interactive + * round no `review run` gate reads) the run-id equality alone is waived — + * the sidecar itself is still required, and its reason, cache path, and + * findings hash still bind: `capture-local` stamps all three with or + * without a parent, so there is always something to match, and skipping + * the fence outright left every interactive grant gated by nothing but + * model-supplied inputs. Anything else — no usable target, a missing, + * unparsable, or foreign-stamped sidecar, a departed reason, cache path, + * or hash — fails closed. Returns null when the fence passes; the refusal + * line's second half otherwise. + */ +function stopSidecarFenceRefusal( + snap: StopSnapshot, + planStopReason: string, + env: NodeJS.ProcessEnv | undefined, +): string | null { + const runIdRaw = (env ?? process.env)['QWEN_REVIEW_RUN_ID']; + const runId = + typeof runIdRaw === 'string' && runIdRaw !== '' ? runIdRaw : null; + if (snap.target === null) { + return ( + 'the plan carries no usable target — the sidecar the capture ' + + 'stamped for this re-rule cannot be located.' + ); + } + const noSidecar = + runId !== null + ? 'a run id is published but no stop sidecar carries its stamp — a ' + + 'stale or foreign stop plan matches the shape but never the fence.' + : "no stop sidecar carries the capture's stamp for this plan — a " + + 'stop plan without its capture-written sidecar is a shape, not a ' + + 'decision.'; + let stop: { + runId?: unknown; + reason?: unknown; + cachePath?: unknown; + findingsHash?: unknown; + supersededPaths?: unknown; + }; + try { + const parsed: unknown = JSON.parse( + readFileSync(tmpFile(snap.target, 'stop.json'), 'utf8'), + ); + // `JSON.parse('null')` succeeds — a null or non-object sidecar must be + // the designed refusal, never a bare TypeError off a property read. + if (typeof parsed !== 'object' || parsed === null) return noSidecar; + stop = parsed as typeof stop; + } catch { + return noSidecar; + } + if (runId !== null && stop.runId !== runId) { + return noSidecar; + } + if (stop.reason !== planStopReason) { + return ( + `the stamped stop sidecar records reason '${String(stop.reason)}', ` + + `not the plan's '${planStopReason}' — the licence is the capture's ` + + 'own decision, not a reason chosen for it.' + ); + } + if (stop.cachePath !== snap.cachePath) { + return ( + 'the stamped stop sidecar names a different cache than the plan — ' + + "the grant's baseline must be the ledger the capture saw." + ); + } + if (stop.findingsHash !== cacheFindingsHash(snap.cache)) { + return ( + 'the cache findings are not the ones the capture stamped — the ' + + 'ledger moved between capture and compose.' + ); + } + // The scope-emptied split binds too: the `superseded` deduction reads + // membership off the plan's `supersededPaths`, and the plan is + // model-editable after the capture wrote it — a split edited between + // capture and compose could blanket-supersede a live blocker past a + // fence that bound only reason/cache/hash. Only the capture-stamped + // copy certifies the split; a sidecar without one (older, or + // hand-written) fails closed for this reason. + if (planStopReason === 'scope-emptied') { + const stamped = stop.supersededPaths; + if ( + !Array.isArray(stamped) || + JSON.stringify(stamped) !== JSON.stringify(snap.supersededPaths) + ) { + return ( + "the plan's supersededPaths depart from the split the capture " + + 'stamped — a superseded deduction reads only the ' + + 'capture-certified split.' + ); + } + } + return null; +} + +/** + * The status vocabulary a ledger row may carry — Step 6's own ruling + * discipline. Anything else is a DRIFTED row, and a drifted row is an + * unreadable baseline, never a skipped one: `status: 'oppn'` silently + * shrank the open set below what the ledger really held. + */ +const LEDGER_STATUS_VOCABULARY = new Set(['open', 'fixed', 'superseded']); + +/** + * The OPEN Critical entries in the cache ledger the snapshot read — the + * exact set a decided-stop re-rule owes a ruling for, each with the title + * the ledger recorded under its id when it carries one (the + * body↔disposition cross-check binds a re-assertion's content against it) + * and the file it cited (the scope-emptied `superseded` deduction reads + * membership in `supersededPaths` off it). Null when the plan names no + * cache or the ledger cannot be read: the completeness check then refuses, + * because a re-rule whose baseline cannot be read cannot be shown + * complete. One exception: a cache file that does not exist recorded no + * findings, so the baseline is EMPTY, not unreadable — that is the + * nothing-open stop's no-event compose. The cache is model-written (Step + * 8's prose rules), so every entry is re-validated, and a shape violation + * — a drifted `status` string included — is an unreadable baseline, never + * a skipped row: skipping shrinks the open set below what the ledger + * really holds, and the grant would issue over Criticals it could not + * enumerate. Enumerated from the SNAPSHOT's bytes — the same buffer the + * fence hashed — so no second read can race the certification. + */ +function openLedgerCriticalEntries( + snap: StopSnapshot, +): Array<{ id: string; title?: string; file?: string }> | null { + if (snap.cache.kind === 'no-path' || snap.cache.kind === 'unreadable') { + return null; + } + if (snap.cache.kind === 'missing') return []; + try { + const cache = JSON.parse(snap.cache.bytes.toString('utf8')) as unknown; + if (typeof cache !== 'object' || cache === null || Array.isArray(cache)) { + return null; + } + // Older caches carry no findings — nothing to track. + if (!('findings' in cache)) return []; + // Present but not an array — the baseline is unreadable, not empty. + if (!Array.isArray(cache.findings)) return null; + const entries: Array<{ id: string; title?: string; file?: string }> = []; + // A repeated id is the same unreadable-baseline refusal as any other + // shape violation: two rows under one id collapse the grant's + // set-based completeness check and the last-wins title/file maps into + // ONE disposition — the "shrank the open set below what the ledger + // really holds" shape, from the ledger side (the disposition-side + // duplicate was already refused). + const seenIds = new Set(); + for (const f of cache.findings) { + const e = f as { + id?: unknown; + severity?: unknown; + status?: unknown; + title?: unknown; + file?: unknown; + }; + if ( + typeof e !== 'object' || + e === null || + typeof e.id !== 'string' || + e.id === '' || + (e.severity !== 'Critical' && e.severity !== 'Suggestion') || + typeof e.status !== 'string' || + !LEDGER_STATUS_VOCABULARY.has(e.status) + ) { + return null; + } + if (seenIds.has(e.id)) return null; + seenIds.add(e.id); + if (e.severity === 'Critical' && e.status === 'open') { + entries.push({ + id: e.id, + ...(typeof e.title === 'string' && e.title.trim() !== '' + ? { title: e.title.trim() } + : {}), + ...(typeof e.file === 'string' && e.file !== '' + ? { file: e.file } + : {}), + }); + } + } + return entries; + } catch { + return null; + } +} + /** * Does this plan name a pull request? The budget and the marker must not * disagree about whether a marker will ride, so both ask here. @@ -987,6 +1321,36 @@ export interface ComposeReviewInput { * check is off: every non-high review, which runs no Step 5. */ findingsPath?: string; + /** + * The decided-stop re-rule (SKILL Step 1's stop branches): the capture + * decided there is nothing to review, and the orchestrator re-ruled the + * cache ledger's OPEN Criticals against the current tree. One entry per + * open ledger Critical, under its ledger id, with the Step 6 ruling. + * + * Machine-checked for completeness before anything is granted: the set of + * ids here must equal the set of open Critical ids in the ledger the + * plan's `cachePath` names — both directions — and every `still-stands` + * ruling must have a matching body Critical carrying its id while + * `fixed`/`superseded` ones must not. Any mismatch throws; a model cannot + * drop a blocker by omitting its row, and cannot resurrect one the ledger + * never held. The grant additionally requires the plan to carry the + * capture's own `nothingToReview` field (no full-round plan does) and — + * under a `review run` parent, which publishes a run id — the runId-fenced + * stop sidecar the same capture wrote. + * + * Granted, it exempts the round from the agent-transcript floors: no + * agents ran, so no transcripts, receipts, verifiers or script-lint + * evidence exist or CAN exist — demanding them is an unsatisfiable cap. + * The verify floor is covered by the completeness check itself: every + * posted blocker is a re-assertion, under its original id, of a finding a + * previous full round verified. + */ + stopReRule?: { + dispositions: Array<{ + id: string; + ruling: 'still-stands' | 'fixed' | 'superseded'; + }>; + }; /** * Where to look for the harness's records. Defaults to the environment the CLI * exported. A test seam only — production never passes it, and a model cannot: @@ -1748,8 +2112,17 @@ export function composeReview( // defines "measured". SKILL tells the round to omit the field there; this // refusal is the module's half, symmetric with round 1 — absence then // carries the streak, exactly as an unmeasured round must. + // A stop re-rule is the THIRD unmeasurable state: no agents ran, so + // nothing this round could have derived a fresh/induced split — every + // posted entry is a carried-id re-assertion the grant itself proves is + // NOT fresh, which is exactly what let a model-written census satisfy + // the fresh <= reported cross-check and mint the non-convergence blocker + // over a round that measured nothing. Refused as null so the streak + // CARRIES rather than resets, like the other two. const readCensus = - prevRound === 0 || input.contextUnavailable === true + prevRound === 0 || + input.contextUnavailable === true || + input.stopReRule !== undefined ? null : churnCensusOf(input.convergence); const churnCensus = @@ -3169,6 +3542,7 @@ function composeReviewBody( const { deferred: modelDeferred, relocated: relocatedCriticals, + relocatedEntries, relocatedDeterministic, } = splitDeferralChannel( input.deferredSuggestions, @@ -3462,9 +3836,205 @@ function composeReviewBody( // the re-post out of the body while `modelBodyCriticals` still counted it // toward `criticalsNeedingVerify` — a blocker the linter proved would go on // pulling the unverified cap through a copy that no longer posts. - const gate = input.planPath - ? scriptLintGate(input.planPath) - : { criticals: [], unreviewed: [], disclosed: [] }; + // The decided-stop re-rule grant — validated fail-closed BEFORE any floor + // is skipped. See ComposeReviewInput.stopReRule for the contract; every + // refusal here THROWS rather than degrading to the regular floors, because + // running transcript floors over a stop state composes garbage caps and + // the orchestrator needs the actual reason. + // The granted dispositions, captured for the body↔disposition + // cross-check below — that check runs over the FINAL body set, after the + // deferral channel's relocation push and the gate-repost dedup, so the + // grant records the rulings and the check binds them to what posts. + const stopRulings = new Map(); + // The titles the ledger recorded under each open Critical id, captured + // inside the grant below for the body↔disposition cross-check: a + // re-assertion binds by CONTENT against them, not by id alone. + const ledgerTitles = new Map(); + // Re-assertions the id binding admitted but no recorded title vouched + // for — they lose the verify-floor exemption below, and (on a granted + // stop, where no tool ran this round) a deterministic tag on one is + // prose, not provenance, so it loses the deterministic exception too. + let unvouchedReAssertions = 0; + let unvouchedTaggedReAssertions = 0; + let unvouchedRelocatedDeterministic = 0; + // The granted plan's own target, kept for the post-bind sidecar consume — + // re-reading the plan there would be the second read the snapshot + // doctrine exists to avoid. + let grantedStopTarget: string | null = null; + const stopReRuleGranted = (() => { + // Model-written state: the declared type promises an object, the file + // on disk can hand anything — a `null` here must be the designed + // refusal, never a bare TypeError off a property read. + const srrRaw: unknown = input.stopReRule; + // ONE plan read and ONE cache read for the whole grant — the fence's + // hash and the completeness check's enumeration are projections of the + // same snapshot, so nothing can move between certification and use. + const snap = readStopSnapshot(input.planPath); + if (srrRaw === undefined) { + // A decided stop composes ONLY through its re-rule: a stop plan + // walked through the regular floors would mint a non-blocking + // verdict over a ledger nobody re-ruled, and `run.ts` would read it + // as this round's completion — exit 0 over the standing blockers. + if (snap.reason !== null) { + throw new Error( + `compose-review refused: the plan carries a decided stop ` + + `('${snap.reason}') but no stopReRule — a decided stop ` + + 'composes only through its re-rule.', + ); + } + return false; + } + if ( + srrRaw === null || + typeof srrRaw !== 'object' || + Array.isArray(srrRaw) + ) { + throw new Error( + 'stopReRule refused: stopReRule must be an object carrying ' + + 'dispositions — one entry per open ledger Critical.', + ); + } + const srr = srrRaw as { dispositions?: unknown }; + if (!Array.isArray(srr.dispositions)) { + throw new Error( + 'stopReRule.dispositions must be an array — one entry per open ' + + 'ledger Critical.', + ); + } + if (criticalsInline > 0) { + throw new Error( + 'stopReRule refused: inline Criticals cannot ride a stop re-rule — ' + + 'a granted stop re-asserts only ledger ids a previous full round ' + + 'verified, and no verifier ran this round.', + ); + } + // The floor's reroute and the model's own deferral channel are the same + // hole from two sides: a Critical riding either leg posts in the body's + // deferral list without ever reaching the body↔disposition bind. On a + // stop round nothing new was reviewed, so a deferral-channel Critical + // can only be a rerouted draft or a claim the bind cannot reach — + // refuse both, before any floor is skipped. + if ( + reroute.entries.some((e) => e.severity === 'Critical') || + modelDeferred.some((e) => e.severity === 'Critical') + ) { + throw new Error( + 'stopReRule refused: a Critical rides the deferral channel — every ' + + 'Critical on a stop re-rule must be a bound re-assertion in ' + + 'bodyCriticals, and the deferral channel is not bound.', + ); + } + const stopReason = snap.reason; + if (stopReason === null) { + throw new Error( + 'stopReRule refused: the plan carries no nothingToReview decision — ' + + 'a full round takes the regular floors, never the stop re-rule.', + ); + } + // Object.hasOwn, never a bare read: the reason arrives through + // model-written plan state, and a prototype-chain key (`__proto__`, + // `constructor`) resolves through the table's prototype instead of + // undefined — the refusal must name the reason, not throw a TypeError. + const allowedRulings = Object.hasOwn(STOP_REASON_RULINGS, stopReason) + ? STOP_REASON_RULINGS[stopReason] + : undefined; + if (allowedRulings === undefined) { + throw new Error( + `stopReRule refused: unknown stop reason '${stopReason}' — the ` + + 'grant fails closed on a reason it cannot rule.', + ); + } + const fenceRefusal = stopSidecarFenceRefusal(snap, stopReason, input.env); + if (fenceRefusal !== null) { + throw new Error(`stopReRule refused: ${fenceRefusal}`); + } + const ledger = openLedgerCriticalEntries(snap); + if (ledger === null) { + throw new Error( + 'stopReRule refused: the ledger the plan names cannot be read — a ' + + 're-rule whose baseline is unreadable cannot be shown complete.', + ); + } + const ledgerFiles = new Map(); + for (const e of ledger) { + if (e.title !== undefined) ledgerTitles.set(e.id, e.title); + if (e.file !== undefined) ledgerFiles.set(e.id, e.file); + } + for (const dRaw of srr.dispositions as unknown[]) { + const d = dRaw as { id?: unknown; ruling?: unknown } | null; + if ( + typeof d?.id !== 'string' || + d.id === '' || + !['still-stands', 'fixed', 'superseded'].includes(d?.ruling as string) + ) { + throw new Error( + 'stopReRule refused: every disposition needs an id and a ruling ' + + 'of still-stands, fixed, or superseded.', + ); + } + if (stopRulings.has(d.id)) { + throw new Error( + `stopReRule refused: duplicate disposition for ${d.id}.`, + ); + } + stopRulings.set(d.id, d.ruling as string); + } + const ledgerSet = new Set(ledger.map((e) => e.id)); + for (const id of ledgerSet) { + if (!stopRulings.has(id)) { + throw new Error( + `stopReRule refused: open ledger Critical ${id} has no ` + + 'disposition — a blocker cannot be dropped by omitting its row.', + ); + } + } + for (const id of stopRulings.keys()) { + if (!ledgerSet.has(id)) { + throw new Error( + `stopReRule refused: disposition ${id} matches no open ledger ` + + 'Critical — a ruling cannot invent its subject.', + ); + } + } + for (const [id, ruling] of stopRulings) { + if (!allowedRulings.includes(ruling)) { + throw new Error( + `stopReRule refused: ${id} is ruled ${ruling} under ` + + `${stopReason} — ${STOP_REASON_REFUSAL[stopReason]}.`, + ); + } + // `scope-emptied` licences `superseded` as a DEDUCED ruling, and the + // deduction's input is the capture-published split: the cited file's + // membership in `supersededPaths`. A ruling is only deduced when the + // machine reads the deduction's input — a `superseded` whose cited + // file the capture did not name as superseded (or whose row records + // no file at all) is a judgement wearing a deduction's licence. + // `clean-tree` is the JUDGED stop; its `superseded` needs no split. + if (ruling === 'superseded' && stopReason === 'scope-emptied') { + const cited = ledgerFiles.get(id); + if (cited === undefined || !snap.supersededPaths.includes(cited)) { + throw new Error( + `stopReRule refused: ${id} is ruled superseded but ` + + (cited === undefined + ? 'the ledger records no file for it' + : `its cited file '${cited}' is not in the plan's ` + + 'supersededPaths') + + ' — a deduced supersession must read its deduction from the ' + + "capture's published split.", + ); + } + } + } + grantedStopTarget = snap.target; + // The body↔disposition cross-check is NOT here: it runs below, over the + // final local body set. Relocation pushes entries after this point and + // ingest transforms them, so a check over the raw input missed both. + return true; + })(); + const gate = + input.planPath && !stopReRuleGranted + ? scriptLintGate(input.planPath) + : { criticals: [], unreviewed: [], disclosed: [] }; const ownAfterGateDedup = withoutGateReposts(bodyCriticals, gate.criticals); bodyCriticals.length = 0; bodyCriticals.push(...ownAfterGateDedup); @@ -3480,7 +4050,146 @@ function composeReviewBody( // resolve one after a specialist inspects it, so capping here would make every // affected review impossible to approve. const repositoryContextNotes: string[] = []; - if (input.planPath) { + if (stopReRuleGranted) { + // No agents ran: the round IS the capture's stop decision plus the + // orchestrator's re-rule of the open ledger. Disclosed on every verdict + // so the body says what kind of round this was — through its OWN block + // (`stopRoundBlock` below), never `gateDisclosed`: that channel's one + // renderer wraps every entry in "Not linted (tool limitation…)", and a + // round kind is not a linting gap. + // The body↔disposition cross-check, over the FINAL local set — the + // ingested entries plus the deferral channel's relocated Criticals, + // past the gate-repost dedup — because that set is what the body posts. + // Ids bind PER ENTRY through the claim head's own leading token — the + // same readback the ledger builder applies — never by substring over + // the joined text: a prefix collision (`R1-1` ⊂ `R1-10`) or a sibling + // id quoted inside another entry's prose is not a re-assertion. The + // relocated leg reads the TYPED entry's title because its rendered + // line wraps the claim where no readback reaches. + const stillStands = new Set(); + for (const [id, ruling] of stopRulings) { + if (ruling === 'still-stands') stillStands.add(id); + } + const carriedIds = new Set(); + // A re-assertion binds by CONTENT, not by id alone: the claim title + // read back from the entry must equal the title the ledger recorded + // under that id — the SKILL's verbatim re-assertion contract makes + // the equality exact. An id alone would let a brand-new claim wear a + // verified id's exemption. An entry the ledger recorded no title for + // keeps its id binding but loses the verify-floor exemption below — + // returned to the caller, because the relocated leg must also strip + // such an entry's deterministic-source credit (its typed `source` is + // prose on a round no tool ran, exactly like an own-leg tag). + const bindEntry = ( + claim: { id?: string; fixInduced: boolean; title: string }, + scanText?: string, + ): boolean => { + if (claim.fixInduced) { + throw new Error( + `stopReRule refused: ${claim.id ?? 'a body Critical'} carries ` + + 'the (fix-induced) marking — a stop re-rule posts only ' + + 're-assertions of verified findings, never new work under ' + + 'an old id.', + ); + } + const id = claim.id; + if (id === undefined || !stopRulings.has(id)) { + throw new Error( + 'stopReRule refused: a body Critical must carry exactly one ' + + 'still-stands ledger id — an entry no re-rule ruled standing ' + + 'posts a blocker no full round verified.', + ); + } + const ruling = stopRulings.get(id); + if (ruling !== 'still-stands') { + throw new Error( + `stopReRule refused: ${id} is ruled ${ruling} yet a body ` + + 'Critical still carries its id — one ruling per finding.', + ); + } + const recorded = ledgerTitles.get(id); + let unvouched = false; + if (recorded === undefined) { + unvouched = true; + unvouchedReAssertions++; + if (scanText !== undefined && DETERMINISTIC_TAG_RE.test(scanText)) { + unvouchedTaggedReAssertions++; + } + } else if (recorded !== claim.title) { + throw new Error( + `stopReRule refused: ${id} is re-asserted with content that ` + + 'departs from the title the ledger recorded — a standing ' + + 'blocker re-asserts its verified claim, not a new claim ' + + 'under an old id.', + ); + } + carriedIds.add(id); + return unvouched; + }; + const ownCount = bodyCriticals.length - relocatedCriticals.length; + for (const entry of bodyCriticals.slice(0, ownCount)) { + bindEntry( + readClaim( + stripForUnattributedPost(entry).replace(LEADING_INVISIBLE_RE, ''), + ), + entry, + ); + } + for (const entry of relocatedEntries) { + // The relocated leg binds the COLLAPSED title, symmetric with the + // collapse the own leg's entries get at ingest: a multi-line title + // whose first line matches the recorded claim must not smuggle new + // claims in its tail past a first-line-only readback — the ledger + // builder records only the first line, so no future round would ever + // rule on the tail. The leading-invisible strip is the same symmetry. + const unvouched = bindEntry( + readClaim(collapseEntry(entry.title).replace(LEADING_INVISIBLE_RE, '')), + ); + // An unvouched relocated re-assertion loses its deterministic-source + // credit: `relocatedDeterministic` counted it on the typed `source` + // alone, and on a granted stop no tool ran that could make that + // source provenance — without this the unverified softening below is + // defeated by exactly the entries nobody's recorded title vouched. + if (unvouched && DETERMINISTIC_SOURCES.has(entry.source)) { + unvouchedRelocatedDeterministic++; + } + } + for (const id of stillStands) { + if (!carriedIds.has(id)) { + throw new Error( + `stopReRule refused: ${id} is ruled still-stands but no body ` + + 'Critical carries its id — a standing blocker must post.', + ); + } + } + if (bodyCriticals.length !== stillStands.size) { + throw new Error( + `stopReRule refused: ${bodyCriticals.length} body Criticals ` + + `over ${stillStands.size} still-stands rulings — every ` + + 'still-stands ruling re-asserts exactly one body Critical.', + ); + } + // Interim hardening for the write-surface class (#10654): an + // interactive (no-run-id) sidecar is CONSUMED once every bind above + // passed — nothing else ever reads it (no parent is polling), and + // left on disk it re-licences this same plan on a later, moved tree + // (a replay clears a blocker no round re-verified). Consumed only + // AFTER the full grant, so a refusal above leaves the sidecar for the + // orchestrator's corrected retry. The gated sidecar stays: the parent + // still reads it for completion, and its runId fence already refuses + // replays across runs. + const grantRunId = (input.env ?? process.env)['QWEN_REVIEW_RUN_ID']; + if ( + (typeof grantRunId !== 'string' || grantRunId === '') && + grantedStopTarget !== null + ) { + try { + unlinkSync(tmpFile(grantedStopTarget, 'stop.json')); + } catch { + // already gone + } + } + } else if (input.planPath) { // The gate ran above, where its claims were needed to dedup the model's // re-posts before provenance was taken. ONE invocation, reused here. bodyCriticals.push(...gate.criticals); // render + count toward `c`, deterministic @@ -3526,9 +4235,31 @@ function composeReviewBody( ); const nonDeterministicBodyCriticals = ownBodyCriticals.filter((x) => !DETERMINISTIC_TAG_RE.test(x)).length + - (relocatedCount - relocatedDeterministic); - const criticalsNeedingVerify = - criticalsInline + nonDeterministicBodyCriticals; + (relocatedCount - relocatedDeterministic) + + // On a granted stop no tool ran this round, so an UNVOUCHED + // re-assertion's `[build]`/`[test]`/`[probe]` substring is prose, not + // provenance, and may not feed the deterministic exception the + // softening reads. Vouched re-assertions keep theirs — they re-assert + // findings a full round verified, tag and all — and the CLI-minted + // nonConvergence Critical is deterministic by provenance and never + // rides this term. The relocated term is the same correction on the + // other leg: an unvouched relocated entry was counted into + // `relocatedDeterministic` on its typed `source` alone, and adding it + // back here keeps its blocker unverified-softenable like its own-leg + // twin. + (stopReRuleGranted + ? unvouchedTaggedReAssertions + unvouchedRelocatedDeterministic + : 0); + const criticalsNeedingVerify = stopReRuleGranted + ? // Every posted blocker on a granted stop re-rule is a re-assertion, + // under its original id, of a finding a previous full round verified — + // and the completeness gate above already proved the set exact. But + // the exemption belongs to the entries the ledger's recorded title + // vouched for: a re-assertion nobody recorded content for cannot be + // SHOWN to be one, and rides the regular floor instead. No verifier + // ran this round because no agents did. + unvouchedReAssertions + : criticalsInline + nonDeterministicBodyCriticals; // Fail closed at every exit: this flag softens a Request changes below, and // it must end up true whenever the review posts non-deterministic Criticals // and CANNOT SHOW they were verified — verifier absent, transcripts @@ -3548,7 +4279,14 @@ function composeReviewBody( // // What it supplies is `planPath` — a path, whose contents the CLI wrote. The // transcripts are found from the environment the CLI exported. - if (!input.planPath) { + if (stopReRuleGranted) { + // Nothing to recompute: no agents, no transcripts, no receipts. The + // grant's two-read gate (the plan's own nothingToReview plus the + // runId-fenced sidecar) is what stands where coverage proof would. + // The verify floor still reads the exemption: a re-assertion the + // recorded title could not vouch for leaves its Critical unverified. + criticalsUnverified = criticalsNeedingVerify >= 1; + } else if (!input.planPath) { coverageEntries.push({ subject: 'coverage', reason: @@ -4243,6 +4981,11 @@ function composeReviewBody( let event: ReviewEvent = baseEvent; if (event === 'APPROVE' && cappedBy.length > 0) event = 'COMMENT'; + // A stop re-rule that cleared every blocker still reviewed NOTHING new — + // it re-ruled old findings on a tree the capture certified unchanged. A + // Comment passes `--fail-on request-changes` exactly like an Approve + // would, without claiming a review that never ran. + if (stopReRuleGranted && event === 'APPROVE') event = 'COMMENT'; // The caps that reach a Request changes — because they remove the premise // the never-soften rule stands on. "A REQUEST_CHANGES earned by a // confirmed Critical is never softened" presumes CONFIRMED, and these @@ -4280,9 +5023,15 @@ function composeReviewBody( // Presubmit downgrades apply after the caps and only when the verdict they // name was the one on the table — `baseEvent` is the row before every cap, // so a softening cap that ran first cannot erase the presubmit's reasons. + // Never on a granted stop re-rule: no presubmit ran this round (no agents + // did), so `input.presubmit` can only be stale or forged there — and a + // model-written `downgradeRequestChanges: true` was the one softening + // channel the grant did not machine-check, moving a certified-standing + // blocker to COMMENT and exit 0 under `--fail-on request-changes`. let downgraded = false; let downgradedFrom: 'Approve' | 'Request changes' | null = null; if ( + !stopReRuleGranted && (event === 'APPROVE' || (baseEvent === 'APPROVE' && event === 'COMMENT')) && downgradeApprove ) { @@ -4290,6 +5039,7 @@ function composeReviewBody( downgraded = true; downgradedFrom = 'Approve'; } else if ( + !stopReRuleGranted && (event === 'REQUEST_CHANGES' || (baseEvent === 'REQUEST_CHANGES' && event === 'COMMENT')) && downgradeRequestChanges @@ -5172,6 +5922,27 @@ function composeReviewBody( } : undefined; + // The round-kind disclosure, on its own line — never inside the lint + // gate's "Not linted" wrapper: a decided-stop re-rule is not a tool + // limitation, and a reader handed "Not linted: Decided-stop re-rule …" + // reads the round kind as a linting gap. Rendered on the two events a + // granted stop can produce (REQUEST_CHANGES and COMMENT; APPROVE is + // demoted before the body composes). + const stopRoundBlock: Bi[] = stopReRuleGranted + ? [ + { + trim: 2, + en: + 'Decided-stop re-rule: the verdict below is the re-rule of the ' + + "cache ledger's open Criticals against the current tree — no " + + 'review agents ran this round.', + zh: + '决定性停止重裁：以下裁决是对 cache 台账中 open Critical 在当前' + + '树上的重裁——本轮没有任何评审 agent 运行。', + }, + ] + : []; + // A deferred checker (actionlint's embedded shell): disclosed on EVERY verdict — // including Approve — so the reader knows a workflow's shell was not linted, but // it does not cap the verdict (it is a tool limitation, not a finding or an @@ -5584,6 +6355,7 @@ function composeReviewBody( ...cannotTellBlock, ...notReviewedForBody, ...unverifiedTagsBlock, + ...stopRoundBlock, ...deferredBlock, ...testPlanBlock, ...repositoryContextBlock, @@ -5790,27 +6562,39 @@ function composeReviewBody( // untagged, a merge of only these defaulted to the weakest, and the tail // cut spent "Review incomplete — unverified findings disclosed." before // it spent a single blocker. + // The granted stop takes its own opener AHEAD of the whole certifying + // chain: no review ran, so neither 'Reviewed — no blockers.' nor the + // bare 'Reviewed.' fallback may open the body — a cleared stop's + // COMMENT opened exactly that way, two paragraphs above its own 'no + // review agents ran this round' disclosure. The wording matches + // `stopRoundBlock`'s frame: this round re-ruled standing findings. clauses.push( - coverageOpener ?? - (canCertify - ? { - keep: 1, - en: 'Reviewed — no blockers.', - zh: '已审查——无阻断问题。', - } - : findingsFileUnreadable - ? { - keep: 1, - en: 'Review incomplete — findings unavailable.', - zh: '审查未完成——发现不可用。', - } - : findingsUnverifiedAtCompose + stopReRuleGranted + ? { + keep: 1, + en: 'Re-rule of standing findings — no new review ran.', + zh: '对既有发现的重裁——本轮未运行新的审查。', + } + : (coverageOpener ?? + (canCertify ? { keep: 1, - en: 'Review incomplete — unverified findings disclosed.', - zh: '审查未完成——未验证的发现已披露。', + en: 'Reviewed — no blockers.', + zh: '已审查——无阻断问题。', } - : { keep: 1, en: 'Reviewed.', zh: '已审查。' }), + : findingsFileUnreadable + ? { + keep: 1, + en: 'Review incomplete — findings unavailable.', + zh: '审查未完成——发现不可用。', + } + : findingsUnverifiedAtCompose + ? { + keep: 1, + en: 'Review incomplete — unverified findings disclosed.', + zh: '审查未完成——未验证的发现已披露。', + } + : { keep: 1, en: 'Reviewed.', zh: '已审查。' })), ); } @@ -5874,6 +6658,10 @@ function composeReviewBody( // `— [unverified]` tags, machine-read. clauses.push(...unverifiedTagsBlock); + // 6b-. Round-kind disclosure (non-capping) — a decided-stop re-rule says + // what kind of round this was, on its own line. + clauses.push(...stopRoundBlock); + // 6b. Deferred-checker disclosure (non-capping) — a workflow whose embedded // shell actionlint would lint but we do not yet trust. clauses.push(...deferredBlock); @@ -6734,8 +7522,19 @@ export const composeReviewCommand: CommandModule = { // from. `event` + `cappedBy` alone cannot reconstruct it — a presubmit // downgrade also depends on `downgraded`/`downgradedFrom` — and Step 8's // archived report copies this line rather than re-deriving a lossy one. + // The parent's run stamp is echoed into the artifact, mirroring the stop + // sidecar's fence: `run.ts` accepts only a verdict stamped by ITS run, + // so a leftover artifact from a concurrent same-stem run — or a file + // written around this command — never reads as this round's verdict. + // Absent when no parent published one (an interactive compose), which + // is exactly when no gate is reading. + const composedRunId = process.env['QWEN_REVIEW_RUN_ID']; const json = JSON.stringify( - { ...result, verdictLine: verdictLine(result) }, + { + ...result, + verdictLine: verdictLine(result), + ...(composedRunId ? { runId: composedRunId } : {}), + }, null, 2, ); @@ -7187,7 +7986,13 @@ export function verdictLine(r: ComposeReviewResult): string { } else if (r.baseEvent === 'APPROVE' && r.event !== 'APPROVE') { const reasons = r.cappedBy.map((c) => why[c] ?? c); if (r.downgraded) reasons.push('a presubmit check failed'); - line += ` — an Approve was NOT available: ${reasons.join('; ')}`; + // Empty reasons is a real state, not a gap: the decided-stop re-rule + // demotes a cleared round's APPROVE to COMMENT with no cap and no + // presubmit — joining an empty list printed a dangling colon there. + line += reasons.length + ? ` — an Approve was NOT available: ${reasons.join('; ')}` + : ' — a decided-stop re-rule reviews nothing new, so a cleared ' + + 'round comments rather than approves'; } else if (r.downgradedFrom === 'Request changes') { // The decisive case, and the one a review caught. A presubmit downgrade can // move a REQUEST_CHANGES — a review with **confirmed Criticals** — down to diff --git a/packages/cli/src/commands/review/lib/local-anchor.ts b/packages/cli/src/commands/review/lib/local-anchor.ts index 64facb09f6f..8bb9752c337 100644 --- a/packages/cli/src/commands/review/lib/local-anchor.ts +++ b/packages/cli/src/commands/review/lib/local-anchor.ts @@ -765,9 +765,28 @@ export function stateIdOf( * skip. */ export function readLocalCache(path: string): LocalReviewCache | null { + let bytes: Buffer; + try { + bytes = readFileSync(path); + } catch { + return null; + } + return readLocalCacheFromBytes(bytes); +} + +/** + * The parse half of `readLocalCache`, over bytes already in hand — for the + * caller that must make its decision AND its stamp projections of ONE read + * (`capture-local`'s stop path: a ledger edit landing between a decision + * read and a second stamp read would be baked into the stamp and invisible + * to the compose fence). + */ +export function readLocalCacheFromBytes( + bytes: Buffer, +): LocalReviewCache | null { let raw: unknown; try { - raw = JSON.parse(readFileSync(path, 'utf8')); + raw = JSON.parse(bytes.toString('utf8')); } catch { return null; } diff --git a/packages/cli/src/commands/review/run.test.ts b/packages/cli/src/commands/review/run.test.ts index b8cd87d91ad..15d832a324e 100644 --- a/packages/cli/src/commands/review/run.test.ts +++ b/packages/cli/src/commands/review/run.test.ts @@ -440,25 +440,44 @@ describe('review run (handler)', () => { }); } - /** Child that "completes", writing (or not) a composed verdict first. */ + /** + * The run id the handler under test published to its (mock) child — for + * fixture writes that happen OUTSIDE the spawn mock, after spawn ran. + * `readComposed` fences on it exactly as `readStopSidecar` always has. + */ + function spawnedRunId(): string { + const opts = spawnMock.mock.calls.at(-1)?.[2] as + | { env: NodeJS.ProcessEnv } + | undefined; + return String(opts?.env['QWEN_REVIEW_RUN_ID']); + } + + /** + * Child that "completes", writing (or not) a composed verdict first — + * stamped with the run id the parent published into its env, exactly as + * compose-review stamps the artifact (readComposed fences on it). + */ function armChild(exit: number, composed?: Record): void { - spawnMock.mockImplementation(() => { - const child = new FakeChild(); - setImmediate(() => { - if (composed) { - mkdirSync(REVIEW_TMP_DIR, { recursive: true }); - mkdirSync(REVIEWS_DIR, { recursive: true }); - writeFileSync( - join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), - JSON.stringify(composed), - 'utf8', - ); - writeFileSync(join(REVIEWS_DIR, 'review.md'), '# report', 'utf8'); - } - child.emit('close', exit); - }); - return child; - }); + spawnMock.mockImplementation( + (...args: [unknown, unknown, { env: NodeJS.ProcessEnv }]) => { + const runId = args[2].env['QWEN_REVIEW_RUN_ID']; + const child = new FakeChild(); + setImmediate(() => { + if (composed) { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + mkdirSync(REVIEWS_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ runId, ...composed }), + 'utf8', + ); + writeFileSync(join(REVIEWS_DIR, 'review.md'), '# report', 'utf8'); + } + child.emit('close', exit); + }); + return child; + }, + ); } it('republishes the composed verdict and exits 0', async () => { @@ -518,17 +537,23 @@ describe('review run (handler)', () => { // the verdict while the child still runs. vi.useFakeTimers(); let child!: FakeChild; - spawnMock.mockImplementation(() => { - // Step 6: compose-review writes the composed verdict. - mkdirSync(REVIEW_TMP_DIR, { recursive: true }); - writeFileSync( - join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), - JSON.stringify({ event: 'APPROVE', verdictLine: 'Verdict: Approve' }), - 'utf8', - ); - child = new FakeChild(); - return child; - }); + spawnMock.mockImplementation( + (...args: [unknown, unknown, { env: NodeJS.ProcessEnv }]) => { + // Step 6: compose-review writes the composed verdict. + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ + runId: args[2].env['QWEN_REVIEW_RUN_ID'], + event: 'APPROVE', + verdictLine: 'Verdict: Approve', + }), + 'utf8', + ); + child = new FakeChild(); + return child; + }, + ); const done = runHandler(); // The capture poll snapshots the verdict while the child still runs... @@ -597,6 +622,7 @@ describe('review run (handler)', () => { writeFileSync( join(REVIEW_TMP_DIR, 'qwen-review-pr-9014-composed.json'), JSON.stringify({ + runId: spawnedRunId(), event: 'COMMENT', verdictLine: 'Verdict: Comment', }), @@ -621,18 +647,24 @@ describe('review run (handler)', () => { // verdict. vi.useFakeTimers(); let child!: FakeChild; - spawnMock.mockImplementation(() => { - mkdirSync(REVIEW_TMP_DIR, { recursive: true }); - const path = join(REVIEW_TMP_DIR, 'qwen-review-pr-7-composed.json'); - writeFileSync( - path, - JSON.stringify({ event: 'APPROVE', verdictLine: 'Verdict: Approve' }), - 'utf8', - ); - utimesSync(path, Date.now() / 1000, Date.now() / 1000); - child = new FakeChild(); - return child; - }); + spawnMock.mockImplementation( + (...args: [unknown, unknown, { env: NodeJS.ProcessEnv }]) => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + const path = join(REVIEW_TMP_DIR, 'qwen-review-pr-7-composed.json'); + writeFileSync( + path, + JSON.stringify({ + runId: args[2].env['QWEN_REVIEW_RUN_ID'], + event: 'APPROVE', + verdictLine: 'Verdict: Approve', + }), + 'utf8', + ); + utimesSync(path, Date.now() / 1000, Date.now() / 1000); + child = new FakeChild(); + return child; + }, + ); const done = runHandler({ target: '7' }); await vi.advanceTimersByTimeAsync(1_000); @@ -640,6 +672,7 @@ describe('review run (handler)', () => { writeFileSync( path, JSON.stringify({ + runId: spawnedRunId(), event: 'REQUEST_CHANGES', verdictLine: 'Verdict: Request changes', }), @@ -1010,16 +1043,22 @@ describe('review run (handler)', () => { // records that the timer fired. vi.useFakeTimers(); let child!: FakeChild; - spawnMock.mockImplementation(() => { - mkdirSync(REVIEW_TMP_DIR, { recursive: true }); - writeFileSync( - join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), - JSON.stringify({ event: 'APPROVE', verdictLine: 'Verdict: Approve' }), - 'utf8', - ); - child = new FakeChild(); - return child; - }); + spawnMock.mockImplementation( + (...args: [unknown, unknown, { env: NodeJS.ProcessEnv }]) => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ + runId: args[2].env['QWEN_REVIEW_RUN_ID'], + event: 'APPROVE', + verdictLine: 'Verdict: Approve', + }), + 'utf8', + ); + child = new FakeChild(); + return child; + }, + ); const done = runHandler({ 'timeout-minutes': 1 }); // The capture poll snapshots the verdict while the child still runs... @@ -1190,7 +1229,8 @@ describe('review run (handler)', () => { opts: { env: Record }, ) => { // Step 1: the capture decides nothing to review and writes the stop - // sidecar, stamped by THIS run. + // sidecar, stamped by THIS run; the nothing-open ledger composes a + // no-event Comment. mkdirSync(REVIEW_TMP_DIR, { recursive: true }); writeFileSync( join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), @@ -1200,6 +1240,15 @@ describe('review run (handler)', () => { }), 'utf8', ); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ + runId: opts.env['QWEN_REVIEW_RUN_ID'], + event: 'COMMENT', + verdictLine: 'Verdict: Comment', + }), + 'utf8', + ); child = new FakeChild(); return child; }, @@ -1223,6 +1272,7 @@ describe('review run (handler)', () => { const result = JSON.parse(outs.join('')); expect(result.completed).toBe(true); + expect(result.event).toBe('COMMENT'); expect(process.exitCode).toBe(0); }); @@ -1248,6 +1298,15 @@ describe('review run (handler)', () => { }), 'utf8', ); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ + runId: opts.env['QWEN_REVIEW_RUN_ID'], + event: 'COMMENT', + verdictLine: 'Verdict: Comment', + }), + 'utf8', + ); child = new FakeChild(); return child; }, @@ -1261,6 +1320,7 @@ describe('review run (handler)', () => { const result = JSON.parse(outs.join('')); expect(result.completed).toBe(true); + expect(result.event).toBe('COMMENT'); expect(process.exitCode).toBe(0); }); @@ -1288,6 +1348,15 @@ describe('review run (handler)', () => { }), 'utf8', ); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ + runId: opts.env['QWEN_REVIEW_RUN_ID'], + event: 'COMMENT', + verdictLine: 'Verdict: Comment', + }), + 'utf8', + ); // Close synchronously on the next microtask — before ANY timer. queueMicrotask(() => child.emit('close', 0)); return child; @@ -1301,11 +1370,57 @@ describe('review run (handler)', () => { expect(process.exitCode).toBe(0); }); - it('exits 0 under --fail-on for a decided stop round', async () => { - // A stop composes no verdict and synthesises none: the ledger it renders - // is rewritten only by a cache-writing round, so a blocker fixed and - // committed stays `open` there — gating on it was a failure no action - // could clear. The gate fires only on a composed REQUEST_CHANGES. + it('gates a stop round through its COMPOSED verdict — the #9908 path', async () => { + // Step 1's stop branches now compose a real verdict when the ledger + // holds open Criticals (deduced dispositions on the incremental stops, + // judged on clean-tree). The parent needs no new plumbing: the composed + // artifact rides the same name a full round writes, so a standing + // blocker exits 3 under --fail-on and a cleared one comments to exit 0. + spawnMock.mockImplementation( + ( + _cmd: unknown, + _argv: unknown, + opts: { env: Record }, + ) => { + const child = new FakeChild(); + setImmediate(() => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), + JSON.stringify({ + reason: 'unchanged-since-last-round', + runId: opts.env['QWEN_REVIEW_RUN_ID'], + }), + 'utf8', + ); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ + runId: opts.env['QWEN_REVIEW_RUN_ID'], + event: 'REQUEST_CHANGES', + verdictLine: 'Verdict: Request changes — R1-1 still stands', + }), + 'utf8', + ); + child.emit('close', 0); + }); + return child; + }, + ); + + await runHandler({ 'fail-on': 'request-changes' }); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(true); + expect(result.event).toBe('REQUEST_CHANGES'); + expect(process.exitCode).toBe(3); + }); + + it('exits 0 under --fail-on for a nothing-open stop round that composed', async () => { + // Every decided stop composes a verdict now — a nothing-open ledger + // composes a no-event Comment (SKILL Step 1's stop branches) — so the + // composed artifact, not the sidecar alone, completes the round. The + // gate still fires only on a composed REQUEST_CHANGES. spawnMock.mockImplementation( ( _cmd: unknown, @@ -1323,6 +1438,15 @@ describe('review run (handler)', () => { }), 'utf8', ); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ + runId: opts.env['QWEN_REVIEW_RUN_ID'], + event: 'COMMENT', + verdictLine: 'Verdict: Comment', + }), + 'utf8', + ); child.emit('close', 0); }); return child; @@ -1331,11 +1455,167 @@ describe('review run (handler)', () => { await runHandler({ 'fail-on': 'request-changes' }); + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(true); + expect(result.event).toBe('COMMENT'); + expect(process.exitCode).toBe(0); + }); + + it('exits 1 when a decided stop never composed a verdict — the refused re-rule', async () => { + // The consumption gap: a re-rule the compose gate refused leaves a stop + // sidecar with NO composed artifact while the cache ledger still holds + // its open Criticals. That shape must read as "no verdict" (exit 1), + // never exit 0 under --fail-on like a clean stop. + spawnMock.mockImplementation( + ( + _cmd: unknown, + _argv: unknown, + opts: { env: Record }, + ) => { + const child = new FakeChild(); + setImmediate(() => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), + JSON.stringify({ + reason: 'unchanged-since-last-round', + runId: opts.env['QWEN_REVIEW_RUN_ID'], + }), + 'utf8', + ); + child.emit('close', 0); + }); + return child; + }, + ); + + await runHandler({ 'fail-on': 'request-changes' }); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(false); + expect(result.event).toBeNull(); + expect(process.exitCode).toBe(1); + }); + + it('a PR stop exits 0 over the PR cache’s open Criticals — disclosed residual', async () => { + // The PR stops (up-to-date, empty-diff) write ONLY the stop sidecar — + // they consume no plan, so compose-review's stopReRule grant is + // unreachable there and no verdict composes: a gate-only re-run exits 0 + // even when the PR cache still holds open Criticals. The capture stops + // compose a re-rule; this test pins the residual the exit-contract + // comment discloses (#9908 tracks the capture stops only). + spawnMock.mockImplementation( + ( + _cmd: unknown, + _argv: unknown, + opts: { env: Record }, + ) => { + const child = new FakeChild(); + setImmediate(() => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-pr-9014-stop.json'), + JSON.stringify({ + reason: 'up-to-date', + runId: opts.env['QWEN_REVIEW_RUN_ID'], + }), + 'utf8', + ); + child.emit('close', 0); + }); + return child; + }, + ); + + await runHandler({ target: '9014', 'fail-on': 'request-changes' }); + const result = JSON.parse(outs.join('')); expect(result.completed).toBe(true); expect(result.event).toBeNull(); expect(process.exitCode).toBe(0); }); + + it('refuses the sidecar-alone completion on a LOCAL target wearing a PR reason', async () => { + // The exemption above is keyed on the TARGET CLASS beside the reason + // string: capture-local stamps only the three decided reasons, so a + // local sidecar wearing `up-to-date` is a forged or drifted stamp — + // completing on it would let the local cache's open Criticals slip an + // exit 0 with no composed artifact at all. + spawnMock.mockImplementation( + ( + _cmd: unknown, + _argv: unknown, + opts: { env: Record }, + ) => { + const child = new FakeChild(); + setImmediate(() => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), + JSON.stringify({ + reason: 'up-to-date', + runId: opts.env['QWEN_REVIEW_RUN_ID'], + }), + 'utf8', + ); + child.emit('close', 0); + }); + return child; + }, + ); + + await runHandler({ 'fail-on': 'request-changes' }); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(false); + expect(result.event).toBeNull(); + expect(process.exitCode).toBe(1); + }); + + it('reads an unstamped composed artifact as no verdict — the fence the sidecar has', async () => { + // A file at the composed name without this run's stamp is not this + // run's verdict: a child that skipped compose-review and wrote the + // artifact by hand — or a concurrent same-stem run's leftover inside + // the mtime window — must not decide this run's exit code. + spawnMock.mockImplementation( + ( + _cmd: unknown, + _argv: unknown, + opts: { env: Record }, + ) => { + const child = new FakeChild(); + setImmediate(() => { + mkdirSync(REVIEW_TMP_DIR, { recursive: true }); + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-stop.json'), + JSON.stringify({ + reason: 'unchanged-since-last-round', + runId: opts.env['QWEN_REVIEW_RUN_ID'], + }), + 'utf8', + ); + // No runId stamp: the hand-written shape the fence exists for. + writeFileSync( + join(REVIEW_TMP_DIR, 'qwen-review-local-composed.json'), + JSON.stringify({ + event: 'COMMENT', + verdictLine: 'Verdict: Comment', + }), + 'utf8', + ); + child.emit('close', 0); + }); + return child; + }, + ); + + await runHandler({ 'fail-on': 'request-changes' }); + + const result = JSON.parse(outs.join('')); + expect(result.completed).toBe(false); + expect(result.event).toBeNull(); + expect(process.exitCode).toBe(1); + }); }); describe('classifyRunTarget — a trailing backslash is a POSIX filename character', () => { diff --git a/packages/cli/src/commands/review/run.ts b/packages/cli/src/commands/review/run.ts index ba5469fd4b5..e277b26c8ba 100644 --- a/packages/cli/src/commands/review/run.ts +++ b/packages/cli/src/commands/review/run.ts @@ -422,11 +422,22 @@ export function newestArtifactSince( * Exit code contract: 0 = the review completed (whatever it decided); 1 = it * never reached a verdict (child failed, timed out with no verdict captured, * or left no composed artifact); 3 = it completed AND the caller asked - * --fail-on request-changes AND the event is REQUEST_CHANGES. A stop carries - * no composed verdict and no synthesised one: the cache ledger a stop renders - * is rewritten only by a round that writes the cache, so a blocker fixed and - * committed stays `open` in it — an exit code keyed on that count is a - * failure no action clears. 3, not 2 — yargs exits 1 on usage errors and + * --fail-on request-changes AND the event is REQUEST_CHANGES. A + * capture-stop round whose cache ledger holds open Criticals composes a REAL + * verdict now — the orchestrator's re-rule of those findings (deduced on the + * two incremental stops, judged on clean-tree; SKILL Step 1's capture-stop + * branches, machine-checked by compose-review's stopReRule gate) — and gates + * here exactly like a full round; a stop whose ledger holds nothing open + * composes a no-event Comment the same way, so a decided stop with NO + * composed artifact is a re-rule the compose gate refused — no verdict, + * never a silent completion. Known residual: the PR-target stops + * (up-to-date, empty-diff) write only the stop sidecar — they consume no + * plan, so the stopReRule grant is unreachable there and no verdict composes; + * a gate-only PR re-run exits 0 even when the PR cache still holds open + * Criticals. No verdict is ever synthesised from a ledger COUNT: that count is + * rewritten only by a cache-writing round, so a blocker fixed and committed + * stays `open` in it, and an exit code keyed on it is a failure no action + * clears (#9659's deleted blocker-dating chain). 3, not 2 — yargs exits 1 on usage errors and * some shells reserve 2, so a CI gate can tell "review is blocking" from * "the tool broke" without parsing anything. */ @@ -440,9 +451,20 @@ export function exitCodeFor( return 0; } -function readComposed(path: string): ComposedVerdict | null { +function readComposed(path: string, runId: string): ComposedVerdict | null { try { - const parsed = JSON.parse(readFileSync(path, 'utf8')) as ComposedVerdict; + const parsed = JSON.parse(readFileSync(path, 'utf8')) as ComposedVerdict & { + runId?: unknown; + }; + // Stamped by THIS run, or it is not this run's verdict — the same fence + // the stop sidecar carries (`readStopSidecar`), because this artifact is + // MORE verdict-bearing than the sidecar, not less: it alone decides the + // event a `--fail-on` gate acts on, its name is the same non-injective + // flattened target token, and the mtime window alone admitted any file a + // concurrent same-stem run — or something that skipped `compose-review` + // entirely — wrote into it. compose-review stamps the id it inherited + // from this parent's `childEnv`. + if (parsed.runId !== runId) return null; // The one field everything downstream keys on. A file without it is not a // composed verdict, whatever its name says. return typeof parsed.event === 'string' ? parsed : null; @@ -647,7 +669,7 @@ async function runReview(args: RunReviewArgs): Promise { const best = newestArtifactSince(REVIEW_TMP_DIR, composedPattern, cutoffMs); if (best === null || best.mtime <= capturedMtime) return; // A half-written file fails to parse; the next tick retries it. - const verdict = readComposed(best.path); + const verdict = readComposed(best.path, runId); if (verdict !== null) { capturedPath = best.path; capturedVerdict = verdict; @@ -725,32 +747,53 @@ async function runReview(args: RunReviewArgs): Promise { if (composed === null) { const best = newestArtifactSince(REVIEW_TMP_DIR, composedPattern, cutoffMs); composedPath = best?.path ?? null; - composed = composedPath ? readComposed(composedPath) : null; + composed = composedPath ? readComposed(composedPath, runId) : null; } const reportPath = newestArtifactSince(REVIEWS_DIR, reportPatternFor(targetClass), cutoffMs) ?.path ?? null; - // A round the CAPTURE decided had nothing to review is complete, even - // though no composed verdict exists: `compose-review` is reached only from - // Step 6, and both stops fire in Step 1. Polling for the verdict alone - // reported "Review did not complete" over a round whose own output was - // decided — a cached second round on an unchanged tree, or a clean tree - // whose earlier blocker the ledger still renders as standing. The signal is - // a field the CLI wrote into its own plan, not a sentence the model chose. - // The in-run snapshot first: it holds the stamped verdict even if a - // concurrent run overwrote or swept the shared sidecar since. The - // post-close scan covers a child that wrote the sidecar and exited inside - // one poll tick. + // The capture's decided-stop signal, read so the completion check below + // can tell "the capture decided this round" from "the run wandered off". + // Every decided capture stop composes a verdict via Step 1's re-rule (a + // REQUEST_CHANGES over standing blockers, or a no-event Comment when the + // ledger holds no open Criticals) — the sidecar alone never completes one + // (see the exit-contract comment on `exitCodeFor`); only the two PR stops + // ride on the sidecar by itself. The signal is a file the CLI wrote, not + // a sentence the model chose. The in-run snapshot first: it holds the + // stamped verdict even if a concurrent run overwrote or swept the shared + // sidecar since. The post-close scan covers a child that wrote the + // sidecar and exited inside one poll tick. const stop = capturedStop ?? nothingToReviewFrom(targetClass, cutoffMs, runId); - const completed = composed !== null || stop !== null; + // The PR stops (up-to-date, empty-diff) consume no plan and compose no + // verdict — the sidecar alone completes the round. Every DECIDED capture + // stop composes one: the re-rule of the ledger's open Criticals, or a + // no-event Comment when nothing is open (SKILL Step 1's stop branches). + // A decided stop with no composed artifact is therefore a re-rule the + // compose gate REFUSED — no verdict was produced, and the round must not + // exit 0 over the ledger's still-open Criticals like a clean stop. The + // exemption is keyed on the TARGET CLASS beside the reason string: only + // the PR path ever writes these two reasons (capture-local stamps only + // the three decided ones), so a local/file sidecar wearing `up-to-date` + // is a forged or drifted stamp, not a PR stop, and completing on it + // would let the local cache's open Criticals slip an exit 0. + const completed = + composed !== null || + (stop !== null && + targetClass.kind === 'pr' && + (stop.reason === 'up-to-date' || stop.reason === 'empty-diff')); // A stop carries no synthesised event, deliberately: the stop's rendered // blocker list comes from the cache ledger, which only a cache-writing // round rewrites — a stop never does — so a blocker fixed and committed // stays `open` there, and an exit code keyed on it is a failure no action - // clears. A composed verdict on the stop path — the model re-ruling the - // ledger — is the answer that can gate; until then a stop exits 0. + // clears. The gate on the capture-stop path is the composed verdict read + // above: Step 1's capture-stop branches re-rule the ledger's open + // Criticals and call compose-review (its stopReRule gate machine-checks + // the dispositions), so a standing blocker arrives here as a real + // REQUEST_CHANGES and a ledger with nothing open completes with no event. + // The PR stops (up-to-date, empty-diff) are the disclosed residual: they + // write only the sidecar and exit 0 over whatever the PR cache holds open. const result: RunReviewResult = { completed, diff --git a/packages/core/src/skills/bundled/review/SKILL.md b/packages/core/src/skills/bundled/review/SKILL.md index 2d10425b391..9e2a7aa9b99 100644 --- a/packages/core/src/skills/bundled/review/SKILL.md +++ b/packages/core/src/skills/bundled/review/SKILL.md @@ -114,11 +114,11 @@ Based on the parsed `target.type`: - **`local`**: Review local uncommitted changes — staged, unstaged, **and untracked**. Capture them with `qwen review capture-local` (below); do not run `git diff` yourself. A `git diff` of any form reports changes to files git already **tracks**, and a file the user created but has not `git add`ed is in neither the index nor HEAD — so it appears in no `git diff` output at all. Reviews have skipped brand-new files this way — not judged low-risk, simply unseen (measured; DESIGN.md — The unseen untracked file). - **At medium effort, the cache is a LEDGER, never an anchor**: read the `findings` of the cache the capture names in its plan (`cachePath`) — **read that field, do not compute the name**: `target` is derived inside the command and `safeTarget` is not hand-reproducible (past 64 characters it suffixes a digest, and symlink canonicalisation diverges from any hand recipe), so a predicted name misses exactly the spellings the canonicalisation exists for and the round then rules on zero entries over a Critical that still stands. At this effort the capture runs without `--cache`, so run it first and read the field off the plan — Step 6 owes each entry a ruling at medium too, and a medium round that cannot see the previous high round's open Critical presents zero blockers over a blocker that still stands. Do NOT pass `--cache` to the capture and do NOT write the cache: incremental scoping and the cache write stay high-only, for the PR cache's exact reasons. - **Incremental local rounds** (high effort only — the same gate, and the same reasons, as the PR cache): append `--cache .qwen/review-cache` to the `capture-local` command — **the DIRECTORY, not a file name you compute**. For a plain local round the file is `local.json` and either form works; for a FILE review the name is namespaced by the source path (`file--.json`), and `target` is derived inside the command from `--file`, so it does not exist yet when this step runs. Predicting it is the same hand-derivation the capture block forbids, wrong by construction — the name carries a digest only the command computes — and wrong in exactly the spelling classes canonicalisation exists for: `ln -s src srclink` then a review of `srclink/foo.ts` predicts from `srclink/foo.ts` while the command canonicalises to `src/foo.ts`, so the prediction misses and the round silently loses BOTH incremental scoping and the findings ledger, with no refusal line printed. Given the directory, the command resolves the file from the target it derived, and a directory holding no cache for this target reads as no anchor. **Do not pass a model**: the command rules the same-model gate over the identity the runtime published, not over a token you carry. A hand-carried one was wrong every time it was written, because `{{model}}` interpolates the BARE model id while the identity the CLI records is provider-qualified — two provider configurations exposing one model name compared equal and passed each other's gate, which is the whole contract. The command enforces the gates itself — same identity, same HEAD, content actually unchanged — and on any refusal falls back to the full capture with the reason on stderr; **repeat that line to the user**, whichever way it went. When it does scope incrementally, the plan carries an `incremental` block (changed files + one-import-hop interaction files, the rest left out) and the chunk briefs direct each agent accordingly; the rest of the flow reads the same plan shape it always did. **Also read the cache's `findings` ledger**: those are the previous local round's findings with their ids, and Step 6 owes each of them a ruling this round, exactly as on the PR path. - - If the plan carries `nothingToReview: { reason: "unchanged-since-last-round" }` — the field, not the stderr sentence; the capture writes it and `qwen review run` reads it, so a decided stop no longer reaches the parent as "Review did not complete" — **first check the cache's `findings` for open entries.** The state is byte-identical to the round that recorded them, so every open finding still stands VERBATIM — render the still-open list with ids and titles (no re-ruling is needed; nothing they describe can have changed), keeping severities distinct: open Criticals remain the round's blockers, open Suggestions are re-listed as open suggestions and block nothing. Then stop. Only when the cached ledger has no open findings does the stop read as clean: inform the user nothing changed since the previous round's clean review — name that round's verdict — and stop here. This is NOT the clean-tree case below: the tree is dirty, but it is byte-identical to the state the previous round already reviewed. - - If the plan carries `nothingToReview: { reason: "scope-emptied" }`, the round is decided the same way, for a different reason: the incremental slice kept zero sections — each anchored path has since been REMOVED (a file deleted, or the change discarded) or sits BYTE-IDENTICAL to what the previous round reviewed, and the stop gate does not distinguish the two. So split the cache's still-open findings by their CITED PATHS against the plan's `incremental.scope.supersededPaths` — the capture publishes exactly the paths whose recorded change is gone, and file PRESENCE cannot answer this (a discarded change leaves the file present with the cited bytes gone): a finding whose cited file IS IN `supersededPaths` is SUPERSEDED — the bytes it cited no longer exist and there is nothing left for it to block; **Never render these findings as still-standing blockers** and do not re-rule them — a verdict that rendered them as standing would repeat that contradiction every round, until HEAD or the model changes. A finding whose cited file is NOT in the list sits byte-identical to what the previous round reviewed — render it as still-standing exactly as the `unchanged-since-last-round` bullet above does (open Criticals remain the round's blockers; open Suggestions are re-listed as open suggestions and block nothing). Then stop. (Without this bullet the shape had no branch at all: `chunks: []` with an `incremental` block, so neither stop fired, `agent-prompt --roster` threw on the first diff-reading role, and the parent reported "Review did not complete" over a decided round.) + - If the plan carries `nothingToReview: { reason: "unchanged-since-last-round" }` — the field, not the stderr sentence; the capture writes it and `qwen review run` reads it, so a decided stop no longer reaches the parent as "Review did not complete" — **first check the cache's `findings` for open entries.** The state is byte-identical to the round that recorded them, so every open finding still stands VERBATIM — render the still-open list with ids and titles (no re-ruling is needed; nothing they describe can have changed), keeping severities distinct: open Criticals remain the round's blockers, open Suggestions are re-listed as open suggestions and block nothing. **When open Criticals exist, compose the stop verdict before stopping** — this is what lets `qwen review run --fail-on request-changes` gate the round instead of passing over standing blockers (the byte-identical state makes every disposition DEDUCED, not judged): write a compose state whose `bodyCriticals` re-assert each open Critical verbatim under its original id, with `stopReRule: { dispositions: [...] }` listing every open ledger Critical as `still-stands` (Criticals only — Suggestions never enter dispositions), plus an empty `--comments` file, and run `compose-review` with the Step 6 template's `--input`/`--comments`/`--out` names; the CLI machine-checks the dispositions against the ledger both ways and refuses any omission, and the composed verdict is REQUEST_CHANGES exactly as a full round's would be. Then stop. When the cached ledger holds no open Criticals — open Suggestions alone block nothing, so a Suggestions-only ledger takes this branch too, symmetric with the scope-emptied and clean-tree bullets — the stop STILL composes before stopping — `qwen review run` reads a decided stop with no composed artifact as "Review did not complete", and a nothing-open ledger composes a no-event Comment: write the same compose state with empty `bodyCriticals` and `stopReRule: { dispositions: [] }`, run `compose-review` with the Step 6 template's names, and only then inform the user nothing changed since the previous round's clean review — name that round's verdict — and stop here. This is NOT the clean-tree case below: the tree is dirty, but it is byte-identical to the state the previous round already reviewed. + - If the plan carries `nothingToReview: { reason: "scope-emptied" }`, the round is decided the same way, for a different reason: the incremental slice kept zero sections — each anchored path has since been REMOVED (a file deleted, or the change discarded) or sits BYTE-IDENTICAL to what the previous round reviewed, and the stop gate does not distinguish the two. So split the cache's still-open findings by their CITED PATHS against the plan's `incremental.scope.supersededPaths` — the capture publishes exactly the paths whose recorded change is gone, and file PRESENCE cannot answer this (a discarded change leaves the file present with the cited bytes gone): a finding whose cited file IS IN `supersededPaths` is SUPERSEDED — the bytes it cited no longer exist and there is nothing left for it to block; **Never render these findings as still-standing blockers** and do not re-rule them — a verdict that rendered them as standing would repeat that contradiction every round, until HEAD or the model changes. A finding whose cited file is NOT in the list sits byte-identical to what the previous round reviewed — render it as still-standing exactly as the `unchanged-since-last-round` bullet above does (open Criticals remain the round's blockers; open Suggestions are re-listed as open suggestions and block nothing). **When open Criticals exist, compose the stop verdict before stopping, exactly as that bullet prescribes** — here the deduced dispositions follow the split: `superseded` for a Critical whose cited file is in `supersededPaths`, `still-stands` (with its verbatim body re-assertion) otherwise. A round whose every open Critical is superseded composes a Comment, never an Approve — nothing new was reviewed. A round whose ledger holds NO open Criticals composes the same way with empty `bodyCriticals` and `stopReRule: { dispositions: [] }` — a decided stop with no composed artifact reads as "Review did not complete". Then stop. (Without this bullet the shape had no branch at all: `chunks: []` with an `incremental` block, so neither stop fired, `agent-prompt --roster` threw on the first diff-reading role, and the parent reported "Review did not complete" over a decided round.) - If the plan has `chunks: []` and a NON-EMPTY `skippedFiles` and NO `nothingToReview`, that is not a stop and must never be reported as one: the capture read nothing AND could not read what it skipped. Report every skipped entry under "Not reviewed" with its reason, tell the user the working tree was not reviewed, and end the round WITHOUT a clean verdict — the absent field is the capture refusing to call this decided, and the round owes the user that distinction. - If the plan has `chunks: []` and an EMPTY `skippedFiles` and NO `nothingToReview` on a plain local round, the capture withheld the stop field — a stop is a DECIDED outcome, and none of the shapes that land here is decided. In one, the tree MOVED while the capture was hashing it (`WARNING: 0 chunks, but the working tree changed while the capture was being hashed`); in another, a cached path DROPPED OUT of the capture while still on disk and diverges from HEAD (`WARNING: 0 chunks, but a cached path dropped out of this capture while still on disk and diverges from HEAD`) — an edit git cannot see (`git update-index --assume-unchanged` is the live case), which the anchor refusal above already named; in the third, tracked paths carry an `--assume-unchanged`/`--skip-worktree` bit (or the bits could not be enumerated), and `git diff` is blind to any edit on them (`WARNING: 0 chunks, but … carry an --assume-unchanged/--skip-worktree bit`, or the same sentence on stderr from an incremental round whose stop it withheld); in the fourth, the round ran with `--no-untracked`, so the untracked half was never enumerated — the clean-tree stop's third clause, checked by nobody, which is exactly the shape the oversized-skip recovery re-run lands in (`the tracked tree is clean, but untracked files were not enumerated (--no-untracked)`), and the two INCREMENTAL stops carry the same exclusion and withhold under the same flag (`The incremental scope kept nothing to review, but untracked files were not enumerated (--no-untracked)`): their comparisons cover tracked content only, and the gate admits no narrower round than the cache, so the cached round ran narrow too and a brand-new file is invisible to both. Never report nothing-to-review on any of these shapes and never take the clean-tree branch: for the `--no-untracked` shape do NOT re-run — report the untracked scope under "Not reviewed" and end the round without a clean verdict; for the others re-run `capture-local` once, and if the warning repeats tell the user — for the moved tree, that their tree is being modified while the review captures it; for the dropped-out path, that a file diverges from HEAD invisibly to git (an `--assume-unchanged`/`--skip-worktree` bit, or an ignore rule) and needs their inspection; for the visibility bits, which paths carry them and that clearing them (`git update-index --no-assume-unchanged` / `--no-skip-worktree`) restores reviewability — and end the round without a verdict. (A FILE review reaching this shape takes the no-diff branch below instead: a whole-file review reads the current state either way.) - - If the plan carries `nothingToReview: { reason: "clean-tree" }` (`chunks: []` — nothing staged, nothing unstaged, nothing untracked), inform the user there are no changes to review and stop here — do not proceed to the review agents. Read the FIELD, not the chunk count: a capture that SKIPPED files also has no chunks, and that round could not read what it skipped, so the capture withholds the field there and the round owes a "Not reviewed" section instead of a stop. `qwen review run` reads the same field, so this stop no longer reaches the parent as "Review did not complete". **First, the same ledger carve-out the no-changes stop above carries**: read the cache at the plan's `cachePath` and, if it holds open findings, render them with ids and severities before stopping — open Criticals as the round's still-standing blockers, open Suggestions as still-open suggestions. A clean tree is not a resolution: the common shape is a user who COMMITS the change without fixing the blocker, leaving a permanently clean tree, and without this the finding is never surfaced on any later round + - If the plan carries `nothingToReview: { reason: "clean-tree" }` (`chunks: []` — nothing staged, nothing unstaged, nothing untracked), **first read the `findings` of the cache the plan names in `cachePath`**: when it holds OPEN Criticals, the clean tree means the change they were found in was committed or discarded WITHOUT a ruling — so re-rule each one against the current tree (read its cited file at HEAD; Step 6's discipline: still-stands / fixed / superseded — here, unlike the two incremental stops, the dispositions are judged, not deduced: no anchor certifies what moved), then compose the stop verdict exactly as the `unchanged-since-last-round` bullet prescribes (`stopReRule` dispositions for every open Critical, verbatim body re-assertions for the still-standing, an empty `--comments` file) so `--fail-on request-changes` gates a commit-without-fixing instead of passing over it. Open Suggestions are re-listed as still-open suggestions and block nothing. Then — or when the ledger holds no open Criticals (compose the no-event verdict first, exactly as the `unchanged-since-last-round` bullet prescribes for its nothing-open shape — a decided stop with no composed artifact reads as "Review did not complete") — inform the user there are no changes to review and stop here; do not proceed to the review agents. Read the FIELD, not the chunk count: a capture that SKIPPED files also has no chunks, and that round could not read what it skipped, so the capture withholds the field there and the round owes a "Not reviewed" section instead of a stop. `qwen review run` reads the same field, so this stop no longer reaches the parent as "Review did not complete". - **`pr-number`, or `pr-url` with a matching remote** (cross-repo `pr-url`s are handled by the lightweight mode above): diff --git a/packages/core/src/skills/bundled/review/SKILL.test.ts b/packages/core/src/skills/bundled/review/SKILL.test.ts index 200d10a6ddd..64cf4ac16d0 100644 --- a/packages/core/src/skills/bundled/review/SKILL.test.ts +++ b/packages/core/src/skills/bundled/review/SKILL.test.ts @@ -51,6 +51,17 @@ function incidentHeadings(): string[] { } describe('bundled review skill', () => { + it('composes EVERY decided stop — a refused re-rule must not hide behind a clean-stop exit', () => { + // `qwen review run` completes a decided stop only when a composed + // verdict exists: a nothing-open ledger composes a no-event Comment, + // and a stop with no composed artifact is a re-rule the compose gate + // refused — exit 1, never a silent exit 0 over standing blockers. + const body = skillBody(); + expect(body).toContain('the stop STILL composes before stopping'); + expect(body).toContain('`stopReRule: { dispositions: [] }`'); + expect(body).toContain('decided stop with no composed artifact'); + }); + it('routes scope-emptied findings by cited path — superseded only when the bytes are gone', () => { // The stop gate cannot tell "every anchored path vanished" from // "anchored paths sit byte-identical to the reviewed round" — the slice @@ -1687,3 +1698,48 @@ describe('bundled review skill', () => { ); }); }); + +describe('bundled review skill — the decided-stop composed verdict (#9908)', () => { + it('routes every ledger-bearing stop through compose-review', () => { + // A decided stop used to complete with event: null, so `--fail-on + // request-changes` passed over standing blockers — the R8-1/R13-3 + // residual. Each stop now composes a real verdict when open Criticals + // exist, and the dispositions channel is machine-checked by the CLI. + const body = skillBody(); + // The two incremental stops DEDUCE dispositions (byte-identical state / + // the supersededPaths split); clean-tree JUDGES them (no anchor). + expect(body).toContain( + '**When open Criticals exist, compose the stop verdict before stopping**', + ); + expect(body).toContain('stopReRule: { dispositions: [...] }'); + expect(body).toContain( + 'compose the stop verdict before stopping, exactly as that bullet prescribes', + ); + expect(body).toContain( + '`superseded` for a Critical whose cited file is in `supersededPaths`', + ); + expect(body).toContain( + 'the dispositions are judged, not deduced: no anchor certifies what moved', + ); + // Criticals only — Suggestions never enter dispositions, and a + // cleared stop comments rather than approves. + expect(body).toContain( + 'Criticals only — Suggestions never enter dispositions', + ); + expect(body).toContain('composes a Comment, never an Approve'); + }); + + it('keys the unchanged bullet’s nothing-open branch on open CRITICALS, like its siblings', () => { + // "No open findings" left a Suggestions-only ledger in NEITHER branch: + // the model stopped without composing, run.ts read a decided stop with + // no composed artifact, and the round exited 1 ("Review did not + // complete") on every unchanged re-run — a standing wedge with nothing + // open to fix. The scope-emptied and clean-tree bullets already key + // this branch on "no open Criticals". + const body = skillBody(); + expect(body).toContain( + 'When the cached ledger holds no open Criticals — open Suggestions alone block nothing', + ); + expect(body).not.toContain('When the cached ledger has no open findings'); + }); +});