mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-05 16:34:00 +01:00
fix: queued PRs keep their ship receipts when main starts excluding paths they never touched (GST-1408) (#4018)
* fix(ship-queue): a receipt with no review_exclude digest binds when the PR touches no new pattern (GST-1408) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * test(review-exclude): type the touched-file cases for the test typecheck ratchet Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(changelog): name the GST-1408 fragment by branch slug Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> * docs(troubleshooting): the queue accepts a recorded=none receipt when no pattern matches (GST-1408) Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --------- Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
33d7352a7f
commit
3b66745ce5
5 files changed
+88
-1
No files matched your search
@@ -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=<digest> 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)
|
||||
@@ -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
|
||||
|
||||
@@ -3474,6 +3474,10 @@ run `gstack-review-dispatch verify --ledger <ledger>`, which prints
|
||||
`REVIEW_EXCLUDE_RECORD: digest=<d>` from the briefs the readers got, then
|
||||
`gstack-ship-receipt write --pr <n> --review-exclude <d> --body <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.
|
||||
|
||||
<a id="review-diff-too-large"></a>
|
||||
### `REVIEW_DIFF_TOO_LARGE`
|
||||
|
||||
|
||||
@@ -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 <n> --review-exclude <d> --body <body> with the digest gstack-review-dispatch verify --ledger <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)`;
|
||||
|
||||
@@ -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<string, string>, 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<Record<string, string>> = [{ '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', () => {
|
||||
|
||||
Reference in new issue
Block a user