mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-03 07:18:03 +01:00
fix: /ship review pass 1: notes-only floor, scoped carry order, named temp and diff refusals
- A notes-only pass is recorded clean only when every finding is fixed, or noted or deferred below the blocking floor (never CRITICAL or blocking), by its logger-stamped action, and the findings account for the claimed issue and critical counts (GST-430). - The preflight scoped carry route commits and pushes the base-merge fix before writing the packet (GST-442). - patchId refuses by name on an unwritable TMPDIR; the tree receipt names why its diff failed (GST-448). - Step 16 stage 3 spells out --docs-requested with its --docs-result (GST-444). Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com>
This commit is contained in:
1 parent
ae38884dc4
commit
c56c0068df
16 files changed
+70
-18
No files matched your search
@@ -391,7 +391,8 @@ function writeRunRecord(a: { receipt: ShipReceipt; head: string; pr: string | un
|
||||
*/
|
||||
function patchId(repoRoot: string, from: string, to: string, pathspec: string[]): string | { error: string } {
|
||||
const firstLine = (s: string | null | undefined, fallback: string) => (s ?? '').trim().split('\n')[0]?.slice(0, 200) || fallback;
|
||||
const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-patch-id-'));
|
||||
let dir: string;
|
||||
try { dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gstack-patch-id-')); } catch (e) { return { error: `cannot create a temp file under ${os.tmpdir()} (${firstLine((e as Error).message, 'mkdtemp failed')}); set TMPDIR to a writable directory` }; }
|
||||
const file = path.join(dir, 'diff.patch');
|
||||
let fd: number | undefined;
|
||||
try {
|
||||
|
||||
@@ -548,7 +548,13 @@ export interface DerivedReviewStatus {
|
||||
export function isOpenFinding(row: Record<string, unknown>): boolean {
|
||||
if (typeof row.disposition === 'string') return !RESOLVED.includes(row.disposition);
|
||||
if (typeof row.status === 'string') return !['resolved', 'fixed', 'closed'].includes(row.status);
|
||||
return !['fixed', 'auto-fixed', 'deferred', 'noted'].includes(row.action as string);
|
||||
return isOpenByAction(row);
|
||||
}
|
||||
/** A finding's `action` alone: fixed and auto-fixed resolve it; deferred and noted resolve only a non-blocking, non-CRITICAL one (review policy's floor). */
|
||||
export function isOpenByAction(row: Record<string, unknown>): boolean {
|
||||
if (row.action === 'fixed' || row.action === 'auto-fixed') return false;
|
||||
if (row.action === 'deferred' || row.action === 'noted') return row.blocking === true || isCriticalFinding(row);
|
||||
return true;
|
||||
}
|
||||
export function isCriticalFinding(row: Record<string, unknown>): boolean {
|
||||
return typeof row.severity === 'string' && row.severity.toLowerCase() === 'critical';
|
||||
|
||||
@@ -2,7 +2,7 @@ import { createHash } from 'node:crypto';
|
||||
import { spawnSync } from 'node:child_process';
|
||||
import { closeSync, constants, fstatSync, lstatSync, mkdirSync, openSync, readFileSync, unlinkSync, writeFileSync } from 'node:fs';
|
||||
import { join } from 'node:path';
|
||||
import { deriveReviewStatus, isOpenFinding, readJsonl } from './headless-artifacts';
|
||||
import { deriveReviewStatus, isCriticalFinding, isOpenByAction, readJsonl } from './headless-artifacts';
|
||||
import { resultLine, type ResultCodeName } from './result-codes';
|
||||
|
||||
const DIFF_REVIEWS = new Set(['review', 'review-fix-check', 'adversarial-review', 'codex-review', 'design-review-lite', 'ship']);
|
||||
@@ -271,10 +271,13 @@ export function bindReview(rec: Record<string, any>, token: string, env = proces
|
||||
if (env.GSTACK_STAMP_TREE) rec.tree = env.GSTACK_STAMP_TREE;
|
||||
if (env.GSTACK_STAMP_DIRTY) rec.dirty = env.GSTACK_STAMP_DIRTY === 'true';
|
||||
stampGateFindings(rec.findings, env.GSTACK_STAMP_SESSION_KIND);
|
||||
// A /ship pass whose findings were all fixed, deferred or noted left no open defect (review-army item 6), so a
|
||||
// claimed issues_found is recorded clean with zero counts and Step 11.5 can bind it (GST-430).
|
||||
// A /ship pass whose findings were all fixed, or deferred or noted below the floor, left no open defect (review-army
|
||||
// item 6), so a claimed issues_found is recorded clean with zero counts and Step 11.5 can bind it (GST-430). Only the
|
||||
// logger-stamped action counts (never a caller's status), and the findings must account for every claimed issue.
|
||||
if (!findingsPath && (rec.skill === 'review' || rec.skill === 'review-fix-check') && rec.status === 'issues_found' && rec.completed === true &&
|
||||
Array.isArray(rec.findings) && rec.findings.length > 0 && rec.findings.every(f => record(f) && typeof f.action === 'string' && !isOpenFinding(f))) {
|
||||
Array.isArray(rec.findings) && rec.findings.length > 0 && rec.findings.length >= Number(rec.issues_found ?? 0) &&
|
||||
rec.findings.filter(f => record(f) && isCriticalFinding(f)).length >= Number(rec.critical ?? 0) &&
|
||||
rec.findings.every(f => record(f) && typeof f.action === 'string' && !isOpenByAction(f))) {
|
||||
Object.assign(rec, { status: 'clean', issues_found: 0, critical: 0, informational: 0 });
|
||||
}
|
||||
if (!DIFF_REVIEWS.has(rec.skill)) {
|
||||
|
||||
@@ -51,7 +51,7 @@ function integrate(cwd: string, base: string, r: Report): boolean {
|
||||
return false;
|
||||
}
|
||||
if (line === 'CARRY: refused (RECEIPT_CARRY_SCOPED_REVIEW)') {
|
||||
r.next(`carry route (Step 1.4): the base edited context next to reviewed hunks: gstack-ship-receipt carry --pr ${n} --base ${base} writes the packet; one fix-check reviewer reads it; fix what the base merge broke, gstack-pregate --allow-repo-commands, then carry again with --scoped-review <record.json> --pregate .gstack/tmp/pregate.json; a blocking finding runs Steps 2-16`);
|
||||
r.next(`carry route (Step 1.4): the base edited context next to reviewed hunks: fix what the base merge broke (CHANGELOG, size caps), commit and push; then gstack-ship-receipt carry --pr ${n} --base ${base} writes the packet for that head; one fix-check reviewer reads it; gstack-pregate --allow-repo-commands, then carry again with --scoped-review <record.json> --pregate .gstack/tmp/pregate.json; a blocking finding runs Steps 2-16`);
|
||||
return false;
|
||||
}
|
||||
} else r.add('PR: none open');
|
||||
|
||||
+6
-2
@@ -170,8 +170,12 @@ export function buildReceipt(o: ReceiptOptions): TreeReceipt | { error: string;
|
||||
const versionRel = o.versionRel ?? ('rel' in cv ? cv.rel : 'rel' in gv ? gv.rel : 'VERSION');
|
||||
const oldV = 'version' in gv ? gv.version : null;
|
||||
const newV = 'version' in cv ? cv.version : null;
|
||||
const diffText = git(o.repoRoot, ['diff', '--unified=0', '--no-color', '--no-ext-diff', gatedHead, currentHead]);
|
||||
if (diffText === null) return { error: `git diff ${gatedHead.slice(0, 12)} ${currentHead.slice(0, 12)} failed, so the changed files cannot be classified (GST-448)`, code: 'EVIDENCE_IDENTITY_UNKNOWN' };
|
||||
const d = spawnSync('git', ['diff', '--unified=0', '--no-color', '--no-ext-diff', gatedHead, currentHead], { cwd: o.repoRoot, encoding: 'utf8', timeout: 120_000, maxBuffer: 256 * 1024 * 1024 });
|
||||
if (d.status !== 0) {
|
||||
const why = (d.error as NodeJS.ErrnoException | undefined)?.code ?? (d.signal ? `killed by ${d.signal}` : (d.stderr ?? '').trim().split('\n')[0]?.slice(0, 200) || `exit ${d.status}`);
|
||||
return { error: `git diff ${gatedHead.slice(0, 12)} ${currentHead.slice(0, 12)} failed (${why}), so the changed files cannot be classified (GST-448)`, code: 'EVIDENCE_IDENTITY_UNKNOWN' };
|
||||
}
|
||||
const diffText = d.stdout.replace(/\n$/, '');
|
||||
const files = classifyFiles(o.repoRoot, parseUnifiedDiff(diffText), gatedHead, currentHead, o.policy, versionRel, oldV, newV);
|
||||
const residual = files.filter(f => f.verdict === 'changed').map(f => f.path);
|
||||
const stampFiles = files.filter(f => f.verdict === 'stamp').map(f => f.path);
|
||||
|
||||
+1
-1
@@ -719,7 +719,7 @@ audit or risk decision never qualifies.
|
||||
|
||||
Report changed inputs, blockers and attempts used:
|
||||
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested`.
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested <path> --docs-result <json>`.
|
||||
- **An attempt remains, with changed inputs or an available repair:** insert
|
||||
`14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count.
|
||||
Validate the outcome before Step 15, then restart Step 16 stage 1.
|
||||
|
||||
+1
-1
@@ -440,7 +440,7 @@ audit or risk decision never qualifies.
|
||||
|
||||
Report changed inputs, blockers and attempts used:
|
||||
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested`.
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested <path> --docs-result <json>`.
|
||||
- **An attempt remains, with changed inputs or an available repair:** insert
|
||||
`14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count.
|
||||
Validate the outcome before Step 15, then restart Step 16 stage 1.
|
||||
|
||||
+1
-1
@@ -719,7 +719,7 @@ audit or risk decision never qualifies.
|
||||
|
||||
Report changed inputs, blockers and attempts used:
|
||||
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested`.
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested <path> --docs-result <json>`.
|
||||
- **An attempt remains, with changed inputs or an available repair:** insert
|
||||
`14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count.
|
||||
Validate the outcome before Step 15, then restart Step 16 stage 1.
|
||||
|
||||
+1
-1
@@ -719,7 +719,7 @@ audit or risk decision never qualifies.
|
||||
|
||||
Report changed inputs, blockers and attempts used:
|
||||
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested`.
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested <path> --docs-result <json>`.
|
||||
- **An attempt remains, with changed inputs or an available repair:** insert
|
||||
`14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count.
|
||||
Validate the outcome before Step 15, then restart Step 16 stage 1.
|
||||
|
||||
+1
-1
@@ -721,7 +721,7 @@ audit or risk decision never qualifies.
|
||||
|
||||
Report changed inputs, blockers and attempts used:
|
||||
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested`.
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested <path> --docs-result <json>`.
|
||||
- **An attempt remains, with changed inputs or an available repair:** insert
|
||||
`14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count.
|
||||
Validate the outcome before Step 15, then restart Step 16 stage 1.
|
||||
|
||||
+1
-1
@@ -723,7 +723,7 @@ audit or risk decision never qualifies.
|
||||
|
||||
Report changed inputs, blockers and attempts used:
|
||||
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested`.
|
||||
- **A fix the audit's result named:** rerun finish with `--docs-requested <path> --docs-result <json>`.
|
||||
- **An attempt remains, with changed inputs or an available repair:** insert
|
||||
`14.5 → 15 → 16` before Step 17. Use Blocked recovery with the existing count.
|
||||
Validate the outcome before Step 15, then restart Step 16 stage 1.
|
||||
|
||||
@@ -57,6 +57,10 @@ describe('a notes-only /ship review pass (GST-430)', () => {
|
||||
['gate open', pass([note(1, 'noted'), note(2, 'kept', { gate: 'RH1', gate_id: 'g', reason: 'r' })])],
|
||||
['no action', pass([note(1, 'noted'), { fingerprint: 'lib/a.ts:9:testing', severity: 'CRITICAL' }])],
|
||||
['incomplete', pass([note(1, 'noted')], { completed: false })],
|
||||
['caller status over a stamped-open gate', pass([note(1, 'noted'), note(2, 'kept', { gate: 'RH1', gate_id: 'g', reason: 'r', severity: 'CRITICAL', status: 'resolved' })])],
|
||||
['critical noted', pass([note(1, 'noted'), note(2, 'noted', { severity: 'CRITICAL' })])],
|
||||
['blocking deferred', pass([note(1, 'noted'), note(2, 'deferred', { blocking: true, issue: null })])],
|
||||
['counts beyond the findings', pass([note(1, 'noted')], { issues_found: 2, critical: 1 })],
|
||||
];
|
||||
for (const [name, row] of cases) {
|
||||
expect(run([row]).code).toBe(0);
|
||||
|
||||
@@ -50,4 +50,21 @@ describe('carry with a large product diff (GST-448)', () => {
|
||||
const out = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main']);
|
||||
expect([out.status, out.out.split('\n').find(l => l.startsWith('CARRY:'))]).toEqual([EXIT.refused, 'CARRY: refused (RECEIPT_CARRY_PRODUCT_CHANGED)']);
|
||||
});
|
||||
|
||||
// Value: protects=an unwritable TMPDIR is a named refusal with its repair, never a stack trace that preflight reads as
|
||||
// 'CARRY: check failed'; fails_when=patchId creates its temp file outside its error handling; seam=TMPDIR; no_claim=none
|
||||
test('an unwritable TMPDIR refuses the carry by name', () => {
|
||||
const repo = makeFixtureRepo(); repos.push(repo);
|
||||
commit(repo, receipts(3, 'base'), 'results: 3 run receipts');
|
||||
const body = path.join(repo.root, 'body.md');
|
||||
expect(runBin(repo, 'gstack-ship-receipt', ['write', '--pr', '9', '--base', 'main', '--version', '1.0.0', '--session-kind', 'unattended', '--body', body]).status).toBe(EXIT.ok);
|
||||
repo.git('checkout', '-q', 'main');
|
||||
write(repo.work, 'docs/README.md', '# docs moved on main\n');
|
||||
repo.git('add', '-A'); repo.git('commit', '-qm', 'main moves on'); repo.git('push', '-q', 'origin', 'main');
|
||||
repo.git('checkout', '-q', 'feature');
|
||||
repo.git('merge', '-q', '--no-edit', 'origin/main');
|
||||
const out = runBin(repo, 'gstack-ship-receipt', ['carry', '--body', body, '--base', 'main'], { TMPDIR: path.join(repo.root, 'no-such-dir') });
|
||||
expect([out.status, out.out.split('\n').find(l => l.startsWith('CARRY:'))]).toEqual([EXIT.refused, 'CARRY: refused (RECEIPT_CARRY_REFUSED)']);
|
||||
expect(out.err).toContain('set TMPDIR to a writable directory');
|
||||
});
|
||||
});
|
||||
@@ -501,7 +501,7 @@ describe('preflight carry route for an adjacent base edit (GST-442)', () => {
|
||||
f.gh(`case "$1 $2" in "pr list") echo 9 ;; "pr view") cat '${view}' ;; *) exit 1 ;; esac`);
|
||||
const r = f.run(PREFLIGHT, ['--base', 'main']);
|
||||
expect(lineOf(r.stdout, 'CARRY')).toBe('CARRY: refused (RECEIPT_CARRY_SCOPED_REVIEW)');
|
||||
expect(r.stdout).toMatch(/^NEXT: carry route \(Step 1\.4\): .*one fix-check reviewer.*--scoped-review <record\.json>/m);
|
||||
expect(r.stdout).toMatch(/^NEXT: carry route \(Step 1\.4\): .*fix what the base merge broke.*commit and push.*writes the packet.*one fix-check reviewer.*--scoped-review <record\.json>/m);
|
||||
expect(fs.existsSync(path.join(f.work, '.git', 'gstack', `scoped-review-${head.slice(0, 12)}.md`))).toBe(false);
|
||||
const wrote = f.run(path.join(BIN, 'gstack-ship-receipt'), ['carry', '--pr', '9', '--base', 'main']);
|
||||
expect([wrote.status, fs.existsSync(path.join(f.work, '.git', 'gstack', `scoped-review-${head.slice(0, 12)}.md`))]).toEqual([EXIT.refused, true]);
|
||||
|
||||
@@ -1,4 +1,4 @@
|
||||
{
|
||||
"bytes": -22,
|
||||
"why": "GST-442/GST-444: Step 1.4 follows a scoped-review NEXT; Step 16 stage 2 and 3 route finish scoped-review STOP and --docs-requested; trims in Steps 3, 12 and 16 keep the skill under its cap"
|
||||
"bytes": 6,
|
||||
"why": "GST-442/GST-444: Step 1.4 follows a scoped-review NEXT; Step 16 stage 2 and 3 route finish scoped-review STOP and --docs-requested with its --docs-result; trims in Steps 3, 12 and 16 keep the skill under its cap"
|
||||
}
|
||||
@@ -224,6 +224,23 @@ describe('prefix trees modulo stamps', () => {
|
||||
expect(stampFreeTree(git, 'f'.repeat(40), repo.work, policy)).toHaveProperty('error');
|
||||
});
|
||||
|
||||
// Value: protects=a gated-to-current diff git cannot produce refuses with its cause instead of reading as 'no changes'
|
||||
// (GST-448); fails_when=buildReceipt treats a failed diff as an empty one or drops why it failed; seam=a missing blob; no_claim=none
|
||||
test('a diff git cannot produce refuses the receipt with EVIDENCE_IDENTITY_UNKNOWN and git\'s reason', () => {
|
||||
const policy = loadPolicyFromHead(repo.work).policy;
|
||||
repo.git('checkout', '-q', '-B', 'lost-blob', sha.t1!);
|
||||
fs.writeFileSync(path.join(repo.work, 'lost.txt'), 'a blob this test deletes from the object store\n');
|
||||
repo.git('add', '-A'); repo.git('commit', '-q', '-m', 'add a blob');
|
||||
const head = repo.git('rev-parse', 'HEAD');
|
||||
const blobId = repo.git('rev-parse', `${head}:lost.txt`);
|
||||
const gitDir = repo.git('rev-parse', '--absolute-git-dir');
|
||||
fs.rmSync(path.join(gitDir, 'objects', blobId.slice(0, 2), blobId.slice(2)));
|
||||
const r = buildReceipt({ repoRoot: repo.work, gated: sha.t1!, current: head, policy, policyLabel: 'fixture' }) as { error?: string; code?: string };
|
||||
expect(r.code).toBe('EVIDENCE_IDENTITY_UNKNOWN');
|
||||
expect(r.error).toMatch(/^git diff [0-9a-f]{12} [0-9a-f]{12} failed \(.+\), so the changed files cannot be classified/);
|
||||
repo.git('checkout', '-q', '-f', 'squash');
|
||||
});
|
||||
|
||||
// #3512: a stamp path carrying a tentative `*_KEY_VERSION = '<next minor>'` (gstack's own convention since #3511) digests the
|
||||
// same before and after the slot stamp moves it, so train-backstop finds the record that covers main; a base-version mention
|
||||
// on a line the base already carried digests the same at the prefix (where it equals VERSION) and on the stamped main.
|
||||
|
||||
Reference in new issue
Block a user