diff --git a/changelog.d/garrytan-gst-1408-review-exclude-older-receipts.md b/changelog.d/garrytan-gst-1408-review-exclude-older-receipts.md new file mode 100644 index 000000000..21355c1d9 --- /dev/null +++ b/changelog.d/garrytan-gst-1408-review-exclude-older-receipts.md @@ -0,0 +1 @@ +- **Adding `review_exclude_paths` to the base policy no longer stops every receipt written before it.** `gstack-ship-queue ready`, train build and land refused each older receipt with `field=review_exclude expected= recorded=none`, even when the PR touched none of the new patterns. On harness-evals one policy PR stopped 9 queued PRs, and the only fix was to rerun review on each unchanged head. The queue now accepts a receipt with no recorded digest when no file the PR changes matches any current pattern, since its reviewers read the same full diff. It still refuses a recorded digest that differs, a PR that touches a pattern (excluded or kept), and a base policy it can't read. Carry still compares digests, so an affected PR that merges main still needs /ship once. (GST-1408) diff --git a/docs/ship-policy.md b/docs/ship-policy.md index f6f45bb61..346b868dd 100644 --- a/docs/ship-policy.md +++ b/docs/ship-policy.md @@ -1466,6 +1466,7 @@ checks the digest against the base policy, so a stale selection cannot be recorded by accident, but it does not read the review ledger: a write given the `expected=` digest passes without a review, and only the review run shows that the reviewers read that selection. +The queue makes one exception (GST-1408): a receipt with no `review_exclude`, written before the base gained the key, still binds when no file the head changes matches any current pattern, excluded or kept, because its reviewers read the same full diff; a recorded digest that differs, a matched file and an unreadable base policy still refuse, and carry still compares digests. **Adopting it.** (1) Open a PR on the base that adds the patterns and `min_gstack_version` at least `REVIEW_EXCLUDE_KEY_VERSION`, and ship it like any diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 55232733a..bb61f5b92 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -3474,6 +3474,10 @@ run `gstack-review-dispatch verify --ledger `, which prints `REVIEW_EXCLUDE_RECORD: digest=` from the briefs the readers got, then `gstack-ship-receipt write --pr --review-exclude --body `. +The queue's check skips a receipt with `recorded=none` (written before the base gained +`review_exclude_paths`) when no file the head changes matches any current pattern, so +this line then means the PR touches a pattern, or the recorded digest differs. + ### `REVIEW_DIFF_TOO_LARGE` diff --git a/lib/ship-receipt-depth.ts b/lib/ship-receipt-depth.ts index 69b083f97..e60a776bf 100644 --- a/lib/ship-receipt-depth.ts +++ b/lib/ship-receipt-depth.ts @@ -281,12 +281,15 @@ function headInputs(root: string, baseRef: string, head: string, r?: ShipReceipt * The queue's exclusion check: the digest the receipt recorded (none when absent) against the one `head` has under the current * base policy. Only the digest counts (the policy's patterns and the excluded list), so a base merge or a moved base that leaves * both unchanged binds; a changed selection needs the review rerun (write checks the same digest and does not read the - * review ledger). + * review ledger). A receipt with no digest also binds when no file the head changed matches any current pattern, excluded + * or kept (GST-1408): the selection then equals the full diff that receipt's review read, so a pattern the base gained later + * changes nothing the reviewers saw. */ export function excludeMismatch(x: Exclusion, r: ShipReceipt, head: string, base: string): string | null { const expected = x.state === 'active' ? x.record.digest : 'none'; const recorded = r.review_exclude?.digest ?? 'none'; if (expected === recorded) return null; + if (recorded === 'none' && x.state === 'active' && x.record.unmatched.length === x.record.patterns.length) return null; const source = x.state === 'active' ? x.record.source : r.review_exclude?.source ?? base; const h = head.slice(0, 12); return `field=review_exclude expected=${expected} recorded=${recorded} source=${source}: the receipt records another selection than ${base}'s review_exclude_paths gives ${h}; the receipt is unchanged; fix: rerun /ship's review on ${h} (gstack-review-dispatch pass --base ${base.replace(/^(refs\/remotes\/)?origin\//, '')}), then gstack-ship-receipt write --pr --review-exclude --body with the digest gstack-review-dispatch verify --ledger prints on REVIEW_EXCLUDE_RECORD: (write checks the digest against the base policy and does not read the review ledger, so only the rerun shows the reviewers read this selection)`; diff --git a/test/review-exclude.test.ts b/test/review-exclude.test.ts index cc50440b4..d87523d91 100644 --- a/test/review-exclude.test.ts +++ b/test/review-exclude.test.ts @@ -342,6 +342,84 @@ describe('the receipt binds the selection the reviewers read', () => { expect({ where, says: text.includes('does not read the review ledger') }).toEqual({ where, says: true }); } }, 240_000); + + // Value: protects=a base policy that gains review_exclude_paths does not void a receipt whose PR touches none of the new + // patterns, while a recorded digest that differs, a PR touching a pattern (an excluded or a kept file) and an unreadable base + // policy still refuse; fails_when=the queue compares recorded=none to the new digest without asking whether any changed + // file matches (GST-1408: harness-evals #264 stopped 9 receipted PRs), or the acceptance spreads to any of the four + // refusals; why_new=GST-1408; seam=bindShippedReceipt, which gstack-ship-queue ready, train build and land call, on receipts + // gstack-ship-receipt write produced; no_claim=carry (bin/gstack-ship-receipt carry keeps its own comparison) + describe('a base policy that gains review_exclude_paths after the receipt (GST-1408)', () => { + const policyOnBaseOnly = (repo: FixtureRepo, text: string) => { + repo.git('checkout', '-q', 'main'); + commit(repo, { '.gstack/ship-policy.json': text }, 'base policy change'); + repo.git('push', '-q', 'origin', 'main'); + repo.git('checkout', '-q', 'feature'); + repo.git('fetch', '-q', 'origin'); + }; + const shipped = (files: Record, basePolicy: string[] | null = null) => { + const repo = makeFixtureRepo({ prefix: 'gstack-exclude-late-', baseFiles: basePolicy ? { '.gstack/ship-policy.json': policyText(basePolicy) } : {} }); + repos.push(repo); + commit(repo, files); + const body = path.join(repo.root, 'body.md'); + const extra = [...targetedAtHead(repo)]; + if (basePolicy) { + const expected = writeReceipt(repo, body, extra).err.match(/field=review_exclude expected=([0-9a-f]{64}) /)?.[1]; + expect(expected).toBeDefined(); + extra.push('--review-exclude', expected!); + } + const w = writeReceipt(repo, body, extra); + expect({ status: w.status, err: w.err }).toMatchObject({ status: EXIT.ok }); + return { repo, body }; + }; + const bind = (repo: FixtureRepo, body: string) => { + const head = repo.git('rev-parse', 'HEAD'); + return bindShippedReceipt(repo.work, 'origin/main', fs.readFileSync(body, 'utf8'), undefined, [head]); + }; + const CODE = { 'lib/reader.js': 'exports.p = 1;\n', 'docs/guide.md': '# guide\n' }; + + test('a receipt with no digest binds when the PR touches none of the new patterns', () => { + const { repo, body } = shipped(CODE); + expect(receiptIn(body).review_exclude).toBeUndefined(); + policyOnBaseOnly(repo, policyText(['receipts/**', '**/*.jsonl'])); + const b = bind(repo, body); + expect({ ok: b.ok, message: b.ok ? '' : b.message }).toEqual({ ok: true, message: '' }); + }, 240_000); + + test('a receipt with no digest still refuses when the PR touches a new pattern, excluded or kept', () => { + const touches: Array> = [{ 'receipts/a/result.json': '{"ok": true}\n' }, { 'receipts/run.js': 'exports.run = 1;\n' }]; + for (const touched of touches) { + const { repo, body } = shipped({ ...CODE, ...touched }); + policyOnBaseOnly(repo, policyText(['receipts/**'])); + const b = bind(repo, body); + expect(b.ok).toBe(false); + if (b.ok) continue; + expect(b.code).toBe('RECEIPT_INVALID'); + expect(b.message).toMatch(/^field=review_exclude expected=[0-9a-f]{64} recorded=none /); + } + }, 240_000); + + test('a recorded digest that differs from the current one still refuses, though the PR touches no pattern', () => { + const { repo, body } = shipped(CODE, ['receipts/**']); + const recorded = receiptIn(body).review_exclude!.digest; + expect(bind(repo, body).ok).toBe(true); + policyOnBaseOnly(repo, policyText(['receipts/**', 'results/**'])); + const b = bind(repo, body); + expect(b.ok).toBe(false); + if (b.ok) return; + expect(b.code).toBe('RECEIPT_INVALID'); + expect(b.message).toMatch(new RegExp(`^field=review_exclude expected=[0-9a-f]{64} recorded=${recorded} `)); + }, 240_000); + + test('an unreadable base policy still refuses a receipt with no digest', () => { + const { repo, body } = shipped(CODE); + policyOnBaseOnly(repo, '{ "review_exclude_paths": "receipts/**" }\n'); + const b = bind(repo, body); + expect(b.ok).toBe(false); + if (b.ok) return; + expect(b.code).toBe('POLICY_INVALID'); + }, 240_000); + }); }); describe('a branch cannot hide code under a base pattern', () => {