mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-10 02:30:39 +01:00
fix: no test edit skips review; guard packet writes; exclusive attempt state
A test-only edit with no gate tag gets the scoped fix-check of its test hunks instead of a lanes-only rerun. Packet write failures print a STOP or RECEIPT_CARRY_REFUSED. attempt's allowance uses an exclusive create. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
f6d6e23e7b
commit
e70da9bf4a
7 files changed
+70
-16
No files matched your search
@@ -186,7 +186,12 @@ function main(argv: string[]): void {
|
||||
process.exit(3);
|
||||
}
|
||||
fs.mkdirSync(path.dirname(path.resolve(state)), { recursive: true });
|
||||
fs.writeFileSync(state, JSON.stringify({ not_counted: seen + 1 }) + '\n', { mode: 0o600 });
|
||||
try { fs.writeFileSync(state, JSON.stringify({ not_counted: 1 }) + '\n', { mode: 0o600, flag: 'wx' }); } catch (e) {
|
||||
if ((e as NodeJS.ErrnoException).code !== 'EEXIST') throw e;
|
||||
console.log('ATTEMPT: counted (another launch in this invocation already used the allowance)');
|
||||
console.log(`STOP: run the install fix (${install}) and confirm its gstack: ok trailer before another launch`);
|
||||
process.exit(3);
|
||||
}
|
||||
console.log(`ATTEMPT: not-counted (install refused: ${install})`);
|
||||
return;
|
||||
}
|
||||
|
||||
@@ -610,8 +610,12 @@ function cmdCarry(rest: string[], json: boolean): number {
|
||||
if (!record) {
|
||||
const gitPath = git(['rev-parse', '--git-path', `gstack/scoped-review-${head.slice(0, 12)}.md`], repoRoot);
|
||||
const packet = path.resolve(repoRoot, gitPath ?? `.git/gstack/scoped-review-${head.slice(0, 12)}.md`);
|
||||
fs.mkdirSync(path.dirname(packet), { recursive: true });
|
||||
fs.writeFileSync(packet, contextPacket(repoRoot, { reviewedBase: fromBase, reviewed: from, currentBase: headBase, current: head }, contextPaths));
|
||||
try {
|
||||
fs.mkdirSync(path.dirname(packet), { recursive: true });
|
||||
fs.writeFileSync(packet, contextPacket(repoRoot, { reviewedBase: fromBase, reviewed: from, currentBase: headBase, current: head }, contextPaths));
|
||||
} catch (e) {
|
||||
return refused('RECEIPT_CARRY_REFUSED', `base context moved next to the reviewed hunks in ${contextPaths.join(', ')}, but the scoped-review packet ${packet} cannot be written (${(e as Error).message.split('\n')[0]}); make its directory writable, then rerun carry`);
|
||||
}
|
||||
return refused('RECEIPT_CARRY_SCOPED_REVIEW', `base context moved next to the reviewed hunks in ${contextPaths.join(', ')}: one fix-check reviewer reads ${packet}, then rerun carry with --scoped-review <record.json> ({"head":"${head}","paths":[…],"reviewer":"<model>","verdict":"clean"}); a blocking finding means /ship on ${head.slice(0, 12)}`);
|
||||
}
|
||||
const review = readScopedReview(record, { head }, contextPaths);
|
||||
|
||||
@@ -1,7 +1,7 @@
|
||||
**/ship no longer pays for a second review loop when only main moved, a test file changed, or the docs audit's own requested fix landed.** Each of these used to count as a product change: a base merge after review (`ROUTE: … behavior=86` on gbrain #7739), main rewording the line next to a reviewed hunk (#3631, founderstar#441), a test-only fix (gbrain #8084), and a one-line index link the audit asked for (gbrain #8087). Each one cost another ~$19 loop or an owner's documentation exception. A real product change still sends the PR back through the full loop (GST-407, GST-384, GST-391).
|
||||
|
||||
### Changed
|
||||
- **`gstack-ship-release finish --reviewed-base <sha>`** (the merge base /ship now saves with the reviewed tree) compares the branch's own patch per path. Paths only the base moved count as `base=`. A test code edit that only adds lines, with no gate edit (a skipped test, a snapshot or a relaxed gate value), counts as `tests=`, and the lanes rerun it. A removed or changed line, such as a weakened assertion, or a test file the scan cannot read still goes through the full loop. Without the flag the route is unchanged and prints `base=unknown`.
|
||||
- **`gstack-ship-release finish --reviewed-base <sha>`** (the merge base /ship now saves with the reviewed tree) compares the branch's own patch per path. Paths only the base moved count as `base=`. A test code edit that only adds lines, with no gate edit (a skipped test, a snapshot or a relaxed gate value), counts as `tests=`: one fix-check reviewer reads its test hunks in the scoped-review packet, and the lanes rerun it, instead of the full loop. No test edit skips review. A removed or changed line, such as a weakened assertion, or a test file the scan cannot read still goes through the full loop. Without the flag the route is unchanged and prints `base=unknown`.
|
||||
- **A version stamp after review counts as release metadata.** `finish` used to read a slot stamp in `stamp_files` paths (a plugin.json `"version"` line, a stamp comment in a doc) as a behavior and docs change, so it stopped for the loop (`ROUTE: … behavior=8 docs=5` on gbrain #8095). A `stamp_files` path whose only edit swaps the old version for the new one, under a built-in form or a `stamp_patterns` template, is now metadata, the same rule the tree receipt and carry use. Any other edit to that path still stops.
|
||||
- **Adjacent base edits get a scoped review, not the full loop.** When the branch's +/- lines are the reviewed ones in the same places and only context lines the base edited differ, `finish` stops with `context=N` and `gstack-ship-receipt carry` with `RECEIPT_CARRY_SCOPED_REVIEW`. Both write a packet with just those hunks. After one fix-check reviewer comes back clean, `--context-reviewed <record>` or `carry --scoped-review <record>` continues, and the carry reason names the reviewer. The same lines moved somewhere else are still a product change.
|
||||
- **Docs audit freshness** ignores test code and paths only the base moved. `--docs-requested <path> --docs-result <audit result>` carries a fix the audit's own result asked for when its blockers or decisions name that exact path, the edit is at most 5 lines, and the doc caps still pass.
|
||||
|
||||
@@ -5071,7 +5071,10 @@ carry reason records the reviewer and the paths. A blocking finding means /ship
|
||||
on the new head. `gstack-ship-release finish` stops the same way with
|
||||
`context=N` and takes `--context-reviewed <record.json>` bound to the tree;
|
||||
write that record beside the packet, outside the worktree, so it does not change
|
||||
the tree it is bound to.
|
||||
the tree it is bound to. Finish also stops this way for test code changed since
|
||||
review with no gate edit (`tests=N`): the packet holds those test hunks, and the
|
||||
reviewer checks that each test still tests what it did. A packet that cannot be
|
||||
written (an unwritable `.git`) is a STOP or `RECEIPT_CARRY_REFUSED` naming the path.
|
||||
|
||||
<a id="receipt-prose-mismatch"></a>
|
||||
### `RECEIPT_PROSE_MISMATCH`
|
||||
|
||||
+4
-3
@@ -125,13 +125,14 @@ function hunkTexts(diff: string): string[] {
|
||||
}
|
||||
|
||||
/** The scoped-review packet: per context path, the hunks that differ between the two branch patches and the base's own change. */
|
||||
export function contextPacket(repoRoot: string, s: PatchSides, paths: readonly string[]): string {
|
||||
export function contextPacket(repoRoot: string, s: PatchSides, paths: readonly string[], tests: readonly string[] = []): string {
|
||||
const fence = (label: string, body: string) => {
|
||||
const all = body.split('\n');
|
||||
return `${label}${all.length > 200 ? ` (first 200 of ${all.length} lines; read the rest with git diff)` : ''}\n\`\`\`diff\n${all.slice(0, 200).join('\n').trimEnd()}\n\`\`\`\n`;
|
||||
};
|
||||
const parts = [`# Scoped review: base context moved next to the reviewed hunks\n\nreviewed ${s.reviewed.slice(0, 12)} against ${s.reviewedBase.slice(0, 12)}; current ${s.current.slice(0, 12)} against ${s.currentBase.slice(0, 12)}. The branch's +/- lines are unchanged; read whether the merge kept their meaning next to the base's edit. Report clean, or findings at the blocking floor.\n`];
|
||||
for (const rel of paths) {
|
||||
if (tests.length) parts.push(`Test code changed since review in ${tests.join(', ')}: read whether each test still checks what it did (no weakened or bypassed assertion, early return, new exemption, skip or mock). Report clean, or findings at the blocking floor.\n`);
|
||||
for (const rel of [...paths, ...tests]) {
|
||||
const r = hunkTexts(pathDiff(repoRoot, s.reviewedBase, s.reviewed, rel, 3) ?? '');
|
||||
const c = hunkTexts(pathDiff(repoRoot, s.currentBase, s.current, rel, 3) ?? '');
|
||||
parts.push(`## ${rel}\n`, fence('Reviewed hunks', r.filter(h => !c.includes(h)).join('')), fence('Current hunks', c.filter(h => !r.includes(h)).join('')),
|
||||
@@ -148,7 +149,7 @@ export function isTestCode(rel: string): boolean {
|
||||
/**
|
||||
* Test paths whose change from..to needs review: any removed or changed line (a weakened assertion or a changed
|
||||
* expected value carries no gate tag), a gate edit lib/gate-diff tags (a skipped test, a snapshot, a relaxed gate
|
||||
* value), or a diff the scan cannot read. Only added lines with no tag stay a lanes-only rerun.
|
||||
* value), or a diff the scan cannot read. The rest still get the scoped fix-check of their test hunks (lib/ship-release.ts route).
|
||||
*/
|
||||
export function gateEditedPaths(repoRoot: string, from: string, to: string, paths: readonly string[]): Set<string> | null {
|
||||
if (!paths.length) return new Set();
|
||||
|
||||
+13
-6
@@ -186,17 +186,24 @@ function route(cwd: string, base: string, reviewed: string, reviewedBase: string
|
||||
by.tests = by.tests.filter(p => !gated.has(p));
|
||||
r.add(`ROUTE: reviewed=${reviewed.slice(0, 12)} current=${now.slice(0, 12)} behavior=${by.behavior.length} docs=${by.docs.length} metadata=${by.metadata.length} tests=${by.tests.length} base=${sides ? by.base.length : 'unknown'} context=${by.context.length}`);
|
||||
if (by.behavior.length) { r.stop(`behavior, tests or build inputs changed since review (${short(by.behavior.sort())}): insert 5–11.5 → 12–14 → 16 before Step 17 (stage 2.1)`); return null; }
|
||||
if (by.context.length && sides) {
|
||||
const scoped = [...by.context, ...by.tests].sort();
|
||||
if (scoped.length && sides) {
|
||||
if (!opts.contextReviewed) {
|
||||
const packet = path.resolve(cwd, git(cwd, ['rev-parse', '--git-path', `gstack/scoped-review-${now.slice(0, 12)}.md`]).stdout.trim() || `.git/gstack/scoped-review-${now.slice(0, 12)}.md`);
|
||||
fs.mkdirSync(path.dirname(packet), { recursive: true });
|
||||
fs.writeFileSync(packet, contextPacket(cwd, sides, by.context));
|
||||
r.stop(`base context moved next to the reviewed hunks in ${short(by.context)}: one fix-check reviewer reads ${packet}, then rerun with --context-reviewed <record.json> written beside the packet, outside the worktree ({"tree":"${now}","paths":[…],"reviewer":"<model>","verdict":"clean"}); a blocking finding is a behavior change (stage 2.1)`);
|
||||
const why = [by.context.length ? `base context moved next to the reviewed hunks in ${short(by.context)}` : '', by.tests.length ? `test code changed since review in ${short(by.tests)}` : ''].filter(Boolean).join('; ');
|
||||
try {
|
||||
fs.mkdirSync(path.dirname(packet), { recursive: true });
|
||||
fs.writeFileSync(packet, contextPacket(cwd, sides, by.context, by.tests));
|
||||
} catch (e) {
|
||||
r.stop(`${why}, but the scoped-review packet ${packet} cannot be written (${firstLine((e as Error).message)}): make its directory writable, then rerun finish (stage 2.1)`);
|
||||
return null;
|
||||
}
|
||||
r.stop(`${why}: one fix-check reviewer reads ${packet}, then rerun with --context-reviewed <record.json> written beside the packet, outside the worktree ({"tree":"${now}","paths":[…],"reviewer":"<model>","verdict":"clean"}); a blocking finding is a behavior change (stage 2.1)`);
|
||||
return null;
|
||||
}
|
||||
const review = readScopedReview(opts.contextReviewed, { tree: now }, by.context);
|
||||
const review = readScopedReview(opts.contextReviewed, { tree: now }, scoped);
|
||||
if (!review.ok) { r.stop(`--context-reviewed: ${review.message}`); return null; }
|
||||
r.add(`SCOPED_REVIEW: clean by ${review.review.reviewer} (${short(by.context)})`);
|
||||
r.add(`SCOPED_REVIEW: clean by ${review.review.reviewer} (${short(scoped)})`);
|
||||
}
|
||||
if (by.docs.length && !opts.docsRechecked) { r.stop(`authored docs changed since review (${short(by.docs)}): recheck the plan items they touch (stage 2.2), then rerun with --docs-rechecked`); return null; }
|
||||
return by;
|
||||
|
||||
@@ -263,6 +263,10 @@ describe('adjacent base edits (GST-384, #3631, founderstar#441)', () => {
|
||||
spawnMerge(repo, { 'lib/rows.js': src({ 5: 'exports.v5 = "five";', 6: 'exports.v6 = 600;' }) });
|
||||
const head = repo.git('rev-parse', 'HEAD');
|
||||
const carry = (extra: string[] = []) => runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main', ...extra]);
|
||||
fs.writeFileSync(path.join(repo.work, '.git', 'gstack'), 'not a directory');
|
||||
const blocked = carry();
|
||||
expect([blocked.status, blocked.out.includes('RECEIPT_CARRY_REFUSED'), blocked.err.includes('cannot be written')]).toEqual([EXIT.refused, true, true]);
|
||||
fs.rmSync(path.join(repo.work, '.git', 'gstack'));
|
||||
const ask = carry();
|
||||
expect(ask.status).toBe(EXIT.refused);
|
||||
expect(ask.out.trim()).toBe('CARRY: refused (RECEIPT_CARRY_SCOPED_REVIEW)\nGSTACK_RESULT: skill=ship status=refused run=-');
|
||||
@@ -305,10 +309,36 @@ describe('test-only and audit-requested edits after the docs audit (GST-391, gbr
|
||||
const reviewed = wtree(f);
|
||||
const post = snapshot(f);
|
||||
f.write({ 'test/b.test.ts': `${spec('b')}test('b is even', () => expect(2 % 2).toBe(0));\n` }); f.commit('test: add a case');
|
||||
const r = finish(f, reviewed, post, ['--reviewed-base', mergeBase(f)]);
|
||||
expect(lineOf(r.stdout, 'ROUTE')).toMatch(/ behavior=0 docs=0 metadata=0 tests=1 /);
|
||||
const ask = finish(f, reviewed, post, ['--reviewed-base', mergeBase(f)]);
|
||||
expect(lineOf(ask.stdout, 'ROUTE')).toMatch(/ behavior=0 docs=0 metadata=0 tests=1 /);
|
||||
expect([ask.status, lineOf(ask.stdout, 'STOP')]).toEqual([3, expect.stringContaining('STOP: test code changed since review in test/b.test.ts: one fix-check reviewer reads ')]);
|
||||
const packet = /reads (\S+\.md), then/.exec(ask.stdout)![1]!;
|
||||
expect(fs.readFileSync(packet, 'utf8')).toContain("test('b is even'");
|
||||
const record = path.join(f.root, 'tests-reviewed.json');
|
||||
fs.writeFileSync(record, JSON.stringify({ tree: wtree(f), paths: ['test/b.test.ts'], reviewer: 'anthropic/claude-opus-5-5', verdict: 'clean' }));
|
||||
const r = finish(f, reviewed, post, ['--reviewed-base', mergeBase(f), '--context-reviewed', record]);
|
||||
expect(lineOf(r.stdout, 'SCOPED_REVIEW')).toBe('SCOPED_REVIEW: clean by anthropic/claude-opus-5-5 (test/b.test.ts)');
|
||||
expect(lineOf(r.stdout, 'DOCS')).toContain('fresh');
|
||||
expect(r.status).toBe(0);
|
||||
|
||||
const table = (extra: string) => `import { expect, test } from 'bun:test';\nconst EXEMPT = [\n 'bin/a',\n${extra}];\ntest('exempt', () => {\n expect(EXEMPT.length).toBeLessThan(5);\n});\n`;
|
||||
const early = (ret: string) => `import { expect, test } from 'bun:test';\ntest('rows', () => {\n${ret} expect(JSON.parse('[1]')).toEqual([1]);\n});\n`;
|
||||
for (const [name, before, after] of [['exemption-table entry', table(''), table(" 'bin/b',\n")], ['return before an expect', early(''), early(' return;\n')]]) {
|
||||
const w = fx({}, { 'src/b.ts': 'export const b = 2;\n', 'test/gate.test.ts': before });
|
||||
const rw = wtree(w);
|
||||
const pw = snapshot(w);
|
||||
w.write({ 'test/gate.test.ts': after }); w.commit(`test: ${name}`);
|
||||
const out = finish(w, rw, pw, ['--reviewed-base', mergeBase(w)]);
|
||||
expect([name, out.status, lineOf(out.stdout, 'STOP')]).toEqual([name, 3, expect.stringContaining('STOP: test code changed since review in test/gate.test.ts: one fix-check reviewer reads ')]);
|
||||
}
|
||||
|
||||
const locked = fx({}, { 'src/b.ts': 'export const b = 2;\n', 'test/b.test.ts': spec('b') });
|
||||
const rl = wtree(locked);
|
||||
const pl = snapshot(locked);
|
||||
locked.write({ 'test/b.test.ts': `${spec('b')}test('b again', () => expect(1).toBe(1));\n` }); locked.commit('test: add');
|
||||
fs.writeFileSync(path.join(locked.work, '.git', 'gstack'), 'not a directory');
|
||||
const blocked = finish(locked, rl, pl, ['--reviewed-base', mergeBase(locked)]);
|
||||
expect([blocked.status, lineOf(blocked.stdout, 'STOP')]).toEqual([3, expect.stringMatching(/^STOP: test code changed since review in test\/b\.test\.ts, but the scoped-review packet \S+ cannot be written \(/)]);
|
||||
f.write({ 'test/b.test.ts': `${spec('b', 'test.skip')}test('b is even', () => expect(2 % 2).toBe(0));\n` }); f.commit('test: skip');
|
||||
const skip = finish(f, reviewed, post, ['--reviewed-base', mergeBase(f)]);
|
||||
expect([skip.status, skip.stdout.includes('STOP: behavior, tests or build inputs changed since review (test/b.test.ts)')]).toEqual([3, true]);
|
||||
@@ -387,6 +417,10 @@ describe('test-only and audit-requested edits after the docs audit (GST-391, gbr
|
||||
expect(second.status).toBe(3);
|
||||
expect(second.stdout).toContain('ATTEMPT: counted (a second install refusal in this invocation)');
|
||||
expect(second.stdout).toContain('STOP: run the install fix');
|
||||
const raced = path.join(f.root, 'raced.json');
|
||||
fs.writeFileSync(raced, JSON.stringify({ not_counted: 0 }));
|
||||
const race = f.run(path.join(BIN, 'gstack-docs-candidate'), ['attempt', refused, '--state', raced]);
|
||||
expect([race.status, race.stdout.includes('ATTEMPT: counted (another launch in this invocation already used the allowance)')]).toEqual([3, true]);
|
||||
const corrupt = path.join(f.root, 'corrupt.json');
|
||||
fs.writeFileSync(corrupt, '{not json');
|
||||
const reset = f.run(path.join(BIN, 'gstack-docs-candidate'), ['attempt', refused, '--state', corrupt]);
|
||||
|
||||
Reference in new issue
Block a user