From 9a5474e4666d4b82163539604d25f086701d39e8 Mon Sep 17 00:00:00 2001 From: garrytan Date: Fri, 30 Oct 2026 15:30:13 +0000 Subject: [PATCH] fix(sealed): ignored-waiver hint names the finding's commit to review, not a sha to write Choose the shortened-vs-another-commit wording by prefix against the finding's commit (fixes the SHA-256 40-hex mislabel and non-prefix short waivers), and check applied waivers with a Set instead of rescanning. Co-authored-by: capy-ai[bot] <230910855+capy-ai[bot]@users.noreply.github.com> --- CHANGELOG.md | 2 +- docs/pregate.md | 4 ++-- lib/sealed-guard.ts | 9 ++++++--- test/sealed-guard.test.ts | 22 +++++++++++++++++++--- 4 files changed, 28 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b851c2c8b..6e66e077c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,7 +2,7 @@ ## [Unreleased] -**A sealed-guard waiver now counts only when it names the full commit sha.** A `.gstack/sealed-allow.tsv` line used to accept a sha as short as 7 hex characters and waive any commit whose sha started with it. A PR author could generate a commit with the same first 7 characters in minutes and publish different content at the same path under an old, reviewed waiver. A waiver now matches only the finding's full commit sha (40 hex characters, 64 in a SHA-256 repository). A shortened, uppercase or mistyped sha is ignored, so the finding still refuses, and stderr says which line was ignored and which full sha to write. +**A sealed-guard waiver now counts only when it names the full commit sha.** A `.gstack/sealed-allow.tsv` line used to accept a sha as short as 7 hex characters and waive any commit whose sha started with it. A PR author could generate a commit with the same first 7 characters in minutes and publish different content at the same path under an old, reviewed waiver. A waiver now matches only the finding's full commit sha (40 hex characters, 64 in a SHA-256 repository). A shortened, uppercase or mistyped sha is ignored, so the finding still refuses, and stderr says which line was ignored and which commit the finding is in, to review before anyone waives it. ### Changed - `lib/sealed-guard.ts`: waivers compare the whole commit sha; `gstack-pregate sealed --json` adds `ignoredWaivers` (rule, path, the sha as written, the finding's commit) for waiver lines that name a finding's rule and path but not its commit, once per line and commit. Lines that match no finding, and waivers that applied to another commit in the push, stay silent. diff --git a/docs/pregate.md b/docs/pregate.md index a3427f145..6ad67bece 100644 --- a/docs/pregate.md +++ b/docs/pregate.md @@ -843,8 +843,8 @@ gstack-pregate sealed [--range ] [--remote origin] [--base ] [--js listed on every push. A waiver counts only when `sha` is the finding's full commit sha (40 hex characters, 64 in a SHA-256 repository); a shortened or mistyped one is ignored, so the finding still refuses, and stderr names it - (`ignored waiver sha (...)`) with the full commit to - write. + (`ignored waiver sha (...)`) with the finding's full + commit, which needs a review before anyone waives it. `--json` lists them as `ignoredWaivers`. - **Visibility.** `sealed.visibility` from the base branch, else a cache under the state root (`sealed-visibility.json`: a `public` answer for 24 hours, a diff --git a/lib/sealed-guard.ts b/lib/sealed-guard.ts index 6bc9272dc..976a44860 100644 --- a/lib/sealed-guard.ts +++ b/lib/sealed-guard.ts @@ -275,9 +275,10 @@ export function evaluateSealed(o: EvaluateOptions): SealedReport { if (entry) waived.push({ ...f, reason: entry.reason }); return !entry; }); + const applied = new Set(waived.map(w => `${w.rule}\t${w.file}\t${w.commit}`)); const ignored = new Map(); for (const f of findings) for (const a of allow) { - if (a.rule !== f.rule || a.path !== f.file || waived.some(w => w.commit === a.sha && w.rule === a.rule && w.file === a.path)) continue; + if (a.rule !== f.rule || a.path !== f.file || applied.has(`${a.rule}\t${a.path}\t${a.sha}`)) continue; ignored.set(`${a.rule}\t${a.path}\t${a.sha}\t${f.commit}`, { rule: a.rule, path: a.path, sha: a.sha, commit: f.commit }); } const ignoredWaivers = [...ignored.values()]; @@ -286,7 +287,6 @@ export function evaluateSealed(o: EvaluateOptions): SealedReport { return { status, policyLabels: labels, commits: commits.length, findings, exempt, waived, ignoredWaivers, visibility: vis.visibility, visibilitySource: vis.source, target: o.target }; } -const FULL_SHA = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/; const where = (f: SealedFinding) => `${f.file || '(unknown file)'}${f.line ? `:${f.line}` : ''}`; /** stderr text for a report: findings one per line (` commit `), never the matched text. */ @@ -294,7 +294,10 @@ export function renderSealed(r: SealedReport, context: string): string { const lines: string[] = []; for (const e of r.exempt) lines.push(`${context}: exempt ${e.rule} ${where(e)} commit ${e.commit.slice(0, 12)} (sealed.allow_paths ${e.glob})`); for (const w of r.waived) lines.push(`${context}: waived ${w.rule} ${where(w)} commit ${w.commit.slice(0, 12)} (${SEALED_ALLOW_PATH}: ${w.reason})`); - for (const i of r.ignoredWaivers) lines.push(`${context}: ignored waiver ${i.rule} ${i.path} sha ${i.sha} (${FULL_SHA.test(i.sha) ? `it names another commit; this finding is in commit ${i.commit}` : `${SEALED_ALLOW_PATH} must name the full commit sha ${i.commit}`})`); + for (const i of r.ignoredWaivers) { + const why = i.commit.startsWith(i.sha.toLowerCase()) ? `${SEALED_ALLOW_PATH} must name the full lowercase commit sha` : 'it names another commit'; + lines.push(`${context}: ignored waiver ${i.rule} ${i.path} sha ${i.sha} (${why}; this finding is in commit ${i.commit}, review it before waiving)`); + } const vis = `visibility: ${r.visibility}${r.visibility === 'unknown' ? ', counted public' : ''}; ${r.visibilitySource}`; const body = r.findings.map(f => ` ${f.rule} ${where(f)} commit ${f.commit.slice(0, 12)}`); if (r.status === 'error') { diff --git a/test/sealed-guard.test.ts b/test/sealed-guard.test.ts index 3c6afcedf..1d159e69e 100644 --- a/test/sealed-guard.test.ts +++ b/test/sealed-guard.test.ts @@ -377,15 +377,17 @@ describe('waivers name the full commit sha (GST-268)', () => { const TRANSCRIPT = 'runs/c1/transcript.jsonl'; const line = (sha: string) => `sealed.transcript\t${TRANSCRIPT}\t${sha}\tsynthetic fixture\n`; - const SHORT: Array<[string, (x: string) => string]> = [['a 7-character prefix', x => x.slice(0, 7)], ['a 12-character prefix', x => x.slice(0, 12)], ['a 39-character prefix', x => x.slice(0, 39)], ['an uppercase sha', x => x.toUpperCase()], ['one extra character', x => `${x}a`]]; - test.each(SHORT)('%s does not waive and is named as ignored with the full commit', (_label, cut) => { + const PREFIX = 'must name the full lowercase commit sha'; + const OTHER = 'it names another commit'; + const SHORT: Array<[string, (x: string) => string, string]> = [['a 7-character prefix', x => x.slice(0, 7), PREFIX], ['a 12-character prefix', x => x.slice(0, 12), PREFIX], ['a 39-character prefix', x => x.slice(0, 39), PREFIX], ['an uppercase sha', x => x.toUpperCase(), PREFIX], ['one extra character', x => `${x}a`, OTHER]]; + test.each(SHORT)('%s does not waive and is named as ignored with the full commit', (_label, cut, why) => { const s = scratch({ '.gstack/pregate.json': policy(SEALED) }); const sha = commit(s, { [TRANSCRIPT]: '{}\n' }); landOnMain(s, { '.gstack/sealed-allow.tsv': line(cut(sha)) }); const h = hook(s); expect(h.code).toBe(1); expect(h.err).toContain(`ignored waiver sealed.transcript ${TRANSCRIPT} sha ${cut(sha)}`); - expect(h.err).toContain(`full commit sha ${sha}`); + expect(h.err).toContain(`${why}; this finding is in commit ${sha}, review it before waiving)`); const pre = preflight(s); expect(pre.code).toBe(1); expect(pre.out).toMatch(/^SEALED: refused findings=1 .* waived=0 /m); @@ -415,6 +417,19 @@ describe('waivers name the full commit sha (GST-268)', () => { expect(r.err).toContain(`it names another commit; this finding is in commit ${second}`); }); + test("a short waiver for an older commit is named as another commit, never as this commit's shortened sha", () => { + const s = scratch({ '.gstack/pregate.json': policy(SEALED) }); + const first = commit(s, { [TRANSCRIPT]: '{}\n' }); + landOnMain(s, { '.gstack/sealed-allow.tsv': line(first.slice(0, 7)) }); + sh(s.work, s.env, ['reset', '-q', '--hard', 'origin/main']); + const second = commit(s, { [TRANSCRIPT]: '{"other":1}\n' }); + expect(second.startsWith(first.slice(0, 7))).toBe(false); + const r = hook(s); + expect(r.code).toBe(1); + expect(r.err).toContain(`ignored waiver sealed.transcript ${TRANSCRIPT} sha ${first.slice(0, 7)} (${OTHER}; this finding is in commit ${second}, review it before waiving)`); + expect(r.err).not.toContain(PREFIX); + }); + test('a short waiver line on the base branch prints nothing on a push with no matching finding', () => { const s = scratch({ '.gstack/pregate.json': policy(SEALED), '.gstack/sealed-allow.tsv': line('abc1234') }); commit(s, { 'notes.md': 'ok\n' }); @@ -432,6 +447,7 @@ describe('waivers name the full commit sha (GST-268)', () => { const pre = preflight(s); expect(pre.code).toBe(waives ? 0 : 1); expect(pre.err.includes('ignored waiver')).toBe(!waives); + if (!waives) expect(pre.err).toContain(`${cut(sha).length < 64 ? PREFIX : OTHER}; this finding is in commit ${sha}, review it before waiving)`); }); test('a waiver that applied in one commit is not reported as ignored for a later commit at the same path', () => {