From 8854b13edac2ee461efc2b0cc5c88e8967f240c2 Mon Sep 17 00:00:00 2001 From: norvalbv Date: Fri, 7 Aug 2026 13:39:24 +0100 Subject: [PATCH] feat(review): sentry-additive restages keep earned verdicts, sentry judge gets a cache MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two amendments so a sentry-gate block no longer re-bills the whole gate fleet: 1. Review-cache keys and waiver fingerprints hash diffCacheIdentity(diff) β€” a normalizer (judge/diff-focus) that strips inert Sentry capture/import lines, hunk coordinates, context lines and hunked files' blob shas from the HASH INPUT only. Every hunk keeps an ANCHOR: git's function context, or its restage-stable old-side start line where git emits none (JSON, top-of-file) β€” so relocation across anchors always invalidates; hunk-less segments (binary/mode/rename) keep their verbatim bytes including index shas, and a diff that normalizes to nothing (a capture-only commit) degrades to exact-bytes keying so unrelated capture-only commits can never share a key or waiver. Strictly conservative matching: only Sentry.-QUALIFIED captureException/captureMessage with identifier/string-literal args β€” bare/unqualified calls (incl. the captureMainMessage wrapper, whose origin one diff line can't prove), nested calls, template literals, payload objects, and removed captures all invalidate. Import lines strip only when origin-checked: @sentry/* packages, or capture-name imports whose path's final segment NAMES sentry (presentry-shim does not). Documented residual: relocation WITHIN one anchor span does not invalidate. Judges still read the raw diff. 2. The sentry commit-msg judge checkpoints SKIP and confident MONITOR in .devkit/sentry-verdict-cache.json (via the decisions verdictKey; diff tier ONLY β€” on message/names tiers the demanded fix can't change the evidence, so a cached block would replay forever). A byte-identical retry replays the verdict instead of re-paying 3 haiku samples; outage/tie/bypass runs never read or write it; a lost write emits cache_write_failed. Escape hatch documented in docs/troubleshooting.md + glossary (rm the store file). Wiring: store registered in REVIEW_CACHE_STORE_NAMES + review-target.sh's 4-store protocol + consumer gitignore lines + devkit's own .gitignore. session.mts's leftover local fail() now imports the shared/common.mts helper (dup-gate burn-down). Spawned sentry gate tests redirect to a private store root (they were writing the real checkout's .devkit). Migration note: in-flight committed waiver fingerprints void once on upgrade. Decision noted under ship-gates-converge-not-restart. πŸ€– Generated with [Claude Code](https://claude.com/claude-code) --- .gitignore | 1 + cli/__tests__/review-cache-session.test.mts | 4 +- cli/lib/install/gitignore-cache.mts | 1 + cli/lib/ship/review-target.sh | 10 +- cli/lib/ship/review/cache/session.mts | 6 +- dist/cli/lib/install/gitignore-cache.mjs | 1 + dist/cli/lib/ship/review-target.sh | 10 +- dist/cli/lib/ship/review/cache/session.mjs | 5 +- dist/gate-engine/judge/diff-focus.mjs | 100 +++++++ dist/gate-engine/review/lens/split.mjs | 9 +- dist/gate-engine/review/overrides.mjs | Bin 14306 -> 14738 bytes dist/gate-engine/sentry/check-sentry.mjs | 23 +- dist/gate-engine/sentry/verdict-cache.mjs | 73 +++++ .../ship-gates-converge-not-restart.md | 1 + docs/glossary.md | 5 +- docs/troubleshooting.md | 8 +- .../judge/__tests__/diff-focus.test.mts | 262 +++++++++++++++++- gate-engine/judge/diff-focus.mts | 105 +++++++ .../review/__tests__/lens-split.test.mts | 35 +++ .../review/__tests__/overrides.test.mts | 22 ++ gate-engine/review/lens/split.mts | 9 +- gate-engine/review/overrides.mts | Bin 15521 -> 15951 bytes .../sentry/__tests__/check-sentry.test.mts | 13 +- .../__tests__/sentry-hard-defaults.test.mts | 11 +- .../sentry/__tests__/verdict-cache.test.mts | 88 ++++++ gate-engine/sentry/check-sentry.mts | 24 +- gate-engine/sentry/verdict-cache.mts | 95 +++++++ 27 files changed, 870 insertions(+), 51 deletions(-) create mode 100644 dist/gate-engine/sentry/verdict-cache.mjs create mode 100644 gate-engine/sentry/__tests__/verdict-cache.test.mts create mode 100644 gate-engine/sentry/verdict-cache.mts diff --git a/.gitignore b/.gitignore index 74cc0059..2743e8ec 100644 --- a/.gitignore +++ b/.gitignore @@ -10,6 +10,7 @@ node_modules .devkit/prefix-cache.json .devkit/decisions-verdict-cache.json .devkit/review-cache.json +.devkit/sentry-verdict-cache.json .devkit/review-progress-*.json .devkit/review-runs/ .devkit/last-ship-gates-*.log diff --git a/cli/__tests__/review-cache-session.test.mts b/cli/__tests__/review-cache-session.test.mts index cf411474..31e907f1 100644 --- a/cli/__tests__/review-cache-session.test.mts +++ b/cli/__tests__/review-cache-session.test.mts @@ -149,13 +149,15 @@ describe('review cache session', () => { expect(prepared.status, prepared.stderr.toString()).toBe(0); expect(prepared.stdout.toString().split('\0')).toEqual([ 'devkit-review-cache-session-v1', - '3', + '4', 'review-cache.json', '', 'decisions-verdict-cache.json', '', 'prefix-cache.json', '', + 'sentry-verdict-cache.json', + '', '', ]); diff --git a/cli/lib/install/gitignore-cache.mts b/cli/lib/install/gitignore-cache.mts index cefc6302..11a92f94 100644 --- a/cli/lib/install/gitignore-cache.mts +++ b/cli/lib/install/gitignore-cache.mts @@ -21,6 +21,7 @@ export const DEVKIT_CACHE_IGNORES = [ '.devkit/prefix-cache.json', '.devkit/decisions-verdict-cache.json', '.devkit/review-cache.json', + '.devkit/sentry-verdict-cache.json', '.devkit/review-progress-*.json', '.devkit/review-runs/', '.devkit/last-ship-gates-*.log', diff --git a/cli/lib/ship/review-target.sh b/cli/lib/ship/review-target.sh index f7ccdec3..598bf04c 100644 --- a/cli/lib/ship/review-target.sh +++ b/cli/lib/ship/review-target.sh @@ -752,14 +752,14 @@ node "$CACHE_SESSION_TOOL" prepare "$PERSISTENT_CACHE_ROOT" "$PRIVATE_DATA_ROOT" > "$CACHE_FIELDS_FILE" CACHE_FIELDS=() while IFS= read -r -d '' field; do CACHE_FIELDS+=("$field"); done < "$CACHE_FIELDS_FILE" -[ "${#CACHE_FIELDS[@]}" -eq 8 ] && \ +[ "${#CACHE_FIELDS[@]}" -eq 10 ] && \ [ "${CACHE_FIELDS[0]}" = devkit-review-cache-session-v1 ] && \ - [ "${CACHE_FIELDS[1]}" = 3 ] || { + [ "${CACHE_FIELDS[1]}" = 4 ] || { echo 'devkit review: cache session returned a malformed protocol.' >&2 exit 1 } -CACHE_NAMES=("${CACHE_FIELDS[2]}" "${CACHE_FIELDS[4]}" "${CACHE_FIELDS[6]}") -CACHE_GENERATIONS=("${CACHE_FIELDS[3]}" "${CACHE_FIELDS[5]}" "${CACHE_FIELDS[7]}") +CACHE_NAMES=("${CACHE_FIELDS[2]}" "${CACHE_FIELDS[4]}" "${CACHE_FIELDS[6]}" "${CACHE_FIELDS[8]}") +CACHE_GENERATIONS=("${CACHE_FIELDS[3]}" "${CACHE_FIELDS[5]}" "${CACHE_FIELDS[7]}" "${CACHE_FIELDS[9]}") export DEVKIT_RUN_MODE=review export DEVKIT_REVIEW_GUARDS="$GUARDS" @@ -901,7 +901,7 @@ review_phase cache-promote CACHE_RESET=0 if [ "$AUTHORITY_OK" -eq 1 ]; then index=0 - while [ "$index" -lt 3 ]; do + while [ "$index" -lt "${#CACHE_NAMES[@]}" ]; do promotion_status=0 node "$CACHE_SESSION_TOOL" promote "$PERSISTENT_CACHE_ROOT" "$PRIVATE_DATA_ROOT" \ "${CACHE_NAMES[$index]}" "${CACHE_GENERATIONS[$index]}" >/dev/null || promotion_status=$? diff --git a/cli/lib/ship/review/cache/session.mts b/cli/lib/ship/review/cache/session.mts index 532c7b26..052f2000 100644 --- a/cli/lib/ship/review/cache/session.mts +++ b/cli/lib/ship/review/cache/session.mts @@ -11,6 +11,7 @@ import { } from '../../../../../gate-engine/judge/verdict-store.mts'; import { runDirectReviewCli } from '../run-direct.mts'; import { reviewPathWithin } from '../runtime-paths.mts'; +import { fail } from '../shared/common.mts'; const PREPARE_PROTOCOL = 'devkit-review-cache-session-v1'; const PROMOTION_PROTOCOL = 'devkit-review-cache-promotion-v1'; @@ -21,6 +22,7 @@ export const REVIEW_CACHE_STORE_NAMES = [ 'review-cache.json', 'decisions-verdict-cache.json', 'prefix-cache.json', + 'sentry-verdict-cache.json', ] as const; export type ReviewCacheStoreName = (typeof REVIEW_CACHE_STORE_NAMES)[number]; @@ -31,10 +33,6 @@ export interface ReviewCacheCheckpoint { generation: string | null; } -function fail(message: string): never { - throw new Error(`devkit review: ${message}`); -} - function physicalRoot(requestedPath: string, label: string): string { if (!requestedPath || requestedPath.includes('\0')) fail(`${label} must be a physical directory.`); diff --git a/dist/cli/lib/install/gitignore-cache.mjs b/dist/cli/lib/install/gitignore-cache.mjs index e8872a84..7f2adeb7 100644 --- a/dist/cli/lib/install/gitignore-cache.mjs +++ b/dist/cli/lib/install/gitignore-cache.mjs @@ -20,6 +20,7 @@ export const DEVKIT_CACHE_IGNORES = [ '.devkit/prefix-cache.json', '.devkit/decisions-verdict-cache.json', '.devkit/review-cache.json', + '.devkit/sentry-verdict-cache.json', '.devkit/review-progress-*.json', '.devkit/review-runs/', '.devkit/last-ship-gates-*.log', diff --git a/dist/cli/lib/ship/review-target.sh b/dist/cli/lib/ship/review-target.sh index f7ccdec3..598bf04c 100644 --- a/dist/cli/lib/ship/review-target.sh +++ b/dist/cli/lib/ship/review-target.sh @@ -752,14 +752,14 @@ node "$CACHE_SESSION_TOOL" prepare "$PERSISTENT_CACHE_ROOT" "$PRIVATE_DATA_ROOT" > "$CACHE_FIELDS_FILE" CACHE_FIELDS=() while IFS= read -r -d '' field; do CACHE_FIELDS+=("$field"); done < "$CACHE_FIELDS_FILE" -[ "${#CACHE_FIELDS[@]}" -eq 8 ] && \ +[ "${#CACHE_FIELDS[@]}" -eq 10 ] && \ [ "${CACHE_FIELDS[0]}" = devkit-review-cache-session-v1 ] && \ - [ "${CACHE_FIELDS[1]}" = 3 ] || { + [ "${CACHE_FIELDS[1]}" = 4 ] || { echo 'devkit review: cache session returned a malformed protocol.' >&2 exit 1 } -CACHE_NAMES=("${CACHE_FIELDS[2]}" "${CACHE_FIELDS[4]}" "${CACHE_FIELDS[6]}") -CACHE_GENERATIONS=("${CACHE_FIELDS[3]}" "${CACHE_FIELDS[5]}" "${CACHE_FIELDS[7]}") +CACHE_NAMES=("${CACHE_FIELDS[2]}" "${CACHE_FIELDS[4]}" "${CACHE_FIELDS[6]}" "${CACHE_FIELDS[8]}") +CACHE_GENERATIONS=("${CACHE_FIELDS[3]}" "${CACHE_FIELDS[5]}" "${CACHE_FIELDS[7]}" "${CACHE_FIELDS[9]}") export DEVKIT_RUN_MODE=review export DEVKIT_REVIEW_GUARDS="$GUARDS" @@ -901,7 +901,7 @@ review_phase cache-promote CACHE_RESET=0 if [ "$AUTHORITY_OK" -eq 1 ]; then index=0 - while [ "$index" -lt 3 ]; do + while [ "$index" -lt "${#CACHE_NAMES[@]}" ]; do promotion_status=0 node "$CACHE_SESSION_TOOL" promote "$PERSISTENT_CACHE_ROOT" "$PRIVATE_DATA_ROOT" \ "${CACHE_NAMES[$index]}" "${CACHE_GENERATIONS[$index]}" >/dev/null || promotion_status=$? diff --git a/dist/cli/lib/ship/review/cache/session.mjs b/dist/cli/lib/ship/review/cache/session.mjs index b64a612a..f29fd7f1 100644 --- a/dist/cli/lib/ship/review/cache/session.mjs +++ b/dist/cli/lib/ship/review/cache/session.mjs @@ -4,6 +4,7 @@ import { join, resolve } from 'node:path'; import { loadEntries, replaceEntries, saveEntriesIfGeneration, verdictStoreGeneration, } from "../../../../../gate-engine/judge/verdict-store.mjs"; import { runDirectReviewCli } from "../run-direct.mjs"; import { reviewPathWithin } from "../runtime-paths.mjs"; +import { fail } from "../shared/common.mjs"; const PREPARE_PROTOCOL = 'devkit-review-cache-session-v1'; const PROMOTION_PROTOCOL = 'devkit-review-cache-promotion-v1'; const STABLE_READ_ATTEMPTS = 8; @@ -12,10 +13,8 @@ export const REVIEW_CACHE_STORE_NAMES = [ 'review-cache.json', 'decisions-verdict-cache.json', 'prefix-cache.json', + 'sentry-verdict-cache.json', ]; -function fail(message) { - throw new Error(`devkit review: ${message}`); -} function physicalRoot(requestedPath, label) { if (!requestedPath || requestedPath.includes('\0')) fail(`${label} must be a physical directory.`); diff --git a/dist/gate-engine/judge/diff-focus.mjs b/dist/gate-engine/judge/diff-focus.mjs index 4cddc4e4..e354f8bd 100644 --- a/dist/gate-engine/judge/diff-focus.mjs +++ b/dist/gate-engine/judge/diff-focus.mjs @@ -65,3 +65,103 @@ export function focusHunks(diff, isRelevant, omitNoun = 'unrelated') { const note = omitted ? `[${omitted} ${omitNoun} hunk(s) omitted]\n` : ''; return `${header}${note}${kept.join('\n')}`.trim(); } +// A purely-additive Sentry instrumentation line β€” matched CONSERVATIVELY, because every match is +// erased from the cache identity and can therefore never invalidate an earned verdict: +// - The call must be `Sentry.`-QUALIFIED. NO bare names, wrappers included: a bare +// `captureException(...)` or `captureMainMessage(...)` from an arbitrary module may be a local +// function carrying real logic and must invalidate β€” origin cannot be checked from one diff +// line, so unqualified calls simply cost a re-review. +// - Arguments must be inert: identifier/member chains or plain string literals, comma-separated. +// No nested calls (`captureException(mutate(err))` is REAL logic riding along), no template +// literals, no object payloads β€” a multi-line capture's opening line ends in `{`/`(` and fails +// the whole-line match anyway. +// - Imports ARE origin-checked, so they may name the wrapper: anything from an `@sentry/*` +// package, or a named import of only the recognized capture names from a path naming sentry. +const SENTRY_ARG = /(?:[\w$.]+|'[^']*'|"[^"]*")/; +const SENTRY_CAPTURE_RE = new RegExp(`^(?:await\\s+)?(?:void\\s+)?Sentry\\.capture(?:Exception|Message)\\(\\s*(?:${SENTRY_ARG.source}(?:\\s*,\\s*${SENTRY_ARG.source})*\\s*)?\\);?$`); +// The named-import alternative's path must NAME sentry as its final segment's leading word +// (`./sentry`, `../lib/sentry-client`, `electron/sentry.main`) β€” a substring match would also strip +// e.g. `../utils/presentry-shim`, a local module whose real logic must invalidate. +const SENTRY_IMPORT_RE = /^import\s+(?:type\s+)?(?:\*\s+as\s+\w+|\w+|\{[^}]*\})\s+from\s+['"]@sentry\/[^'"]+['"];?$|^import\s+\{\s*capture(?:Exception|Message|MainMessage)(?:\s*,\s*capture(?:Exception|Message|MainMessage))*\s*\}\s+from\s+['"](?:[^'"]*\/)?[sS]entry(?:[-.][^'"]*)?['"];?$/; +const HUNK_HEADER_RE = /^@@ /; +const HUNK_FUNC_RE = /^@@ [^@]*@@ ?/; +const HUNK_OLD_START_RE = /^@@ -(\d+)/; +const INDEX_LINE_RE = /^index [0-9a-f]+\.\.[0-9a-f]+/; +function sentryAdditive(content) { + return SENTRY_CAPTURE_RE.test(content) || SENTRY_IMPORT_RE.test(content); +} +/** + * The diff's CACHE IDENTITY: the text hashed into a gate's verdict-cache key, normalized so that a + * restage whose only delta is purely-additive Sentry instrumentation (the exact fix the sentry gate + * blocks for) hashes identically to the pre-fix diff β€” earned reviewer PASSes survive the fix + * commit instead of re-billing the whole fleet for one capture line. + * + * Why headers AND context must go: inserting one line shifts every later `@@` coordinate, can + * extend a hunk's trailing context, materialize a brand-new hunk (~6 context lines the old diff + * never showed), or merge adjacent hunks β€” so any surviving context/`@@`/`index` byte would still + * miss. What remains per file is its header identity plus the ordered `Β±` content lines, with + * sentry-additive `+` lines dropped (`-` lines NEVER dropped: removing instrumentation is a real + * change). A file whose surviving body is empty (touched only by sentry lines) drops entirely. + * + * What survives per file: its header identity, plus each hunk's anchor followed by that hunk's + * surviving `Β±` lines. The anchor is git's FUNCTION CONTEXT (the text after the second `@@` β€” + * stable under line-number shifts); where git emits none (JSON, hunks above a file's first + * anchor-matching line) it is the hunk's OLD-side start line, which a purely-additive restage + * cannot move (both diffs share the pre-image). Consecutive identical anchors collapse to one, so + * two same-function hunks merging (a capture inserted between them) keeps the identity stable, + * while relocating a change into a different function β€” or, for anchorless types, a different + * old-side position β€” voids the key. Deliberate residual weakening: context bytes and hunked + * files' blob shas are excluded, so relocating a change WITHIN one anchor span (same anchor, same + * `Β±` sequence) does not invalidate β€” a narrow residual, and every consumer still salts the key + * with reviewer identity, Targets, lens group, and devkit version. NOT for judge evidence β€” + * judges read the raw diff. + * + * Fixpoint guarantee: a hunk-less segment (binary / mode-only / rename-only / non-diff text) + * passes through VERBATIM, `index` shas included β€” a binary blob's sha is its only content + * identity β€” so unexpected input degrades to exact-bytes keying. + */ +export function diffCacheIdentity(diff) { + const out = []; + for (const seg of splitDiffByFile(diff)) { + const lines = seg.split('\n'); + const firstHunk = lines.findIndex((l) => HUNK_HEADER_RE.test(l)); + if (firstHunk === -1) { + // Hunk-less segment (binary / mode-only / rename-only / non-diff text): keep it VERBATIM, + // `index` shas included β€” for a git-binary file those shas are the only content identity, + // and they move only when the blob does. This is the exact-bytes degradation path. + if (seg.trim()) + out.push(seg.trim()); + continue; + } + const header = lines.slice(0, firstHunk).filter((l) => !INDEX_LINE_RE.test(l)); + const body = []; + let anchor = ''; // current hunk's anchor; emitted lazily, deduped consecutively + let emitted = null; + for (const line of lines.slice(firstHunk)) { + if (HUNK_HEADER_RE.test(line)) { + // Anchor on git's function context; where git emits none (JSON, top-of-file hunks, .sh + // preambles), fall back to the OLD-side start line β€” stable across a purely-additive + // restage (the pre-image is shared), yet distinct across relocations within the file. + const func = line.replace(HUNK_FUNC_RE, ''); + anchor = func ? `@ ${func}` : `@ :${line.match(HUNK_OLD_START_RE)?.[1] ?? '?'}`; + continue; + } + if (line.startsWith('-') || (line.startsWith('+') && !sentryAdditive(line.slice(1).trim()))) { + // `+++` can't reach here β€” file headers all precede the first `@@`, so every `+` is an add. + if (anchor !== emitted) { + body.push(anchor); + emitted = anchor; + } + body.push(line); + } + // everything else β€” ` ` context, blank context, `\ No newline` β€” is dropped + } + if (body.length) + out.push([...header, ...body].join('\n')); + } + // A diff whose EVERY line normalized away (a wholly-sentry-additive commit) must not collapse to + // the one shared empty identity β€” two unrelated capture-only commits would collide on a key and + // share a verdict/waiver. There is no prior verdict such a commit could converge to anyway, so + // degrade it to exact-bytes keying. + return out.length ? out.join('\n') : String(diff); +} diff --git a/dist/gate-engine/review/lens/split.mjs b/dist/gate-engine/review/lens/split.mjs index 5a458d6c..e72a86e4 100644 --- a/dist/gate-engine/review/lens/split.mjs +++ b/dist/gate-engine/review/lens/split.mjs @@ -32,6 +32,7 @@ * name, and gate-verdict-attribution expects ONE review_result row per reviewer β€” so renaming the * derived clones would void every committed waiver and split the telemetry in two. */ +import { diffCacheIdentity } from "../../judge/diff-focus.mjs"; import { emitGateEvent } from "../../judge/gate-events.mjs"; import { composeTranscript, saveTranscript } from "../../judge/transcript-store.mjs"; import { itemFields, mergeItemVectors } from "../evidence/items.mjs"; @@ -239,17 +240,21 @@ export function planReviewWork(selected, diffs, cache, salts, keyOf, groups = re const sel = selected[i]; const name = sel.reviewer.name; const salt = salts.get(name) ?? ''; + // Keys hash the diff's CACHE IDENTITY (sentry-additive lines normalized out) so a restage whose + // only delta is the capture the sentry gate demanded keeps every earned PASS. Judges, transcripts + // and scope rows still get the RAW diffs[i] β€” only the key input is normalized. + const idText = diffCacheIdentity(diffs[i]); const split = groups && name === 'correctness-reviewer' && sel.reviewer.skill ? groups : null; const parts = split ? split.map((g) => ({ sel: { ...sel, reviewer: deriveLensReviewer(sel.reviewer, g) }, - key: keyOf(name, diffs[i], `${salt}|split:${lensGroupId(g)}`), + key: keyOf(name, idText, `${salt}|split:${lensGroupId(g)}`), diffText: diffs[i], splitOf: name, group: lensGroupId(g), base: sel, })) - : [{ sel, key: keyOf(name, diffs[i], salt), diffText: diffs[i], base: sel }]; + : [{ sel, key: keyOf(name, idText, salt), diffText: diffs[i], base: sel }]; const allCached = parts.every((p) => Boolean(cache[p.key])); scope.push({ sel, diff: diffs[i], cached: allCached }); if (allCached) { diff --git a/dist/gate-engine/review/overrides.mjs b/dist/gate-engine/review/overrides.mjs index 9ec423a2f28e115426d354300a23ae8dcb33bc59..541f277d9b931edbaf50864326010fc6863e221d 100644 GIT binary patch delta 705 zcmaiyK}#D!6vsgqB?>*19z+mcgOF_NuHsF_gT#P)u?Is-X`zfeZ+9o%oe49uiCd8P z8MC`eN9_L%qP|Nis)&!hOQ{`^=RQ$NeR#aO1~)|t1TJJja`p=;uqO%EoL&fGs|`P~Qcx;{cE6-4 zZd=tL5pmg9{+3;%1P~aM67woh#)zx}LMhX1h-S63V?=PE%$USjNNV@0s<7(a7ypjrw#N1c-{M91CJUex<)pVpq2ww7jJnKO>+B~4N4EhvbN zGse^ZBlW{E9N8VSNK85s6Cq8Kr5qYc$_cyz*k$VALrOFP#e#6ETtKEUzk=oY6Ij6= z@zoc|>viw_+j1~4dD)mwyI+gr}jjZd|tZi+K&M&3ACtck?>9>IQq0UiUS4 y=$%l58Si>-6Z|Wh8az5Mx6!3pN3nU^KR$FxpQzy^71A1UJS?agCfz1ljs5^O32aRO diff --git a/dist/gate-engine/sentry/check-sentry.mjs b/dist/gate-engine/sentry/check-sentry.mjs index fba8b8e9..8fda7566 100644 --- a/dist/gate-engine/sentry/check-sentry.mjs +++ b/dist/gate-engine/sentry/check-sentry.mjs @@ -33,10 +33,9 @@ * The diff directive also self-clears a fix that ELIMINATES the silent path (measured on the * elimination tier: 15/23 -> 19/23 with the clause, no real-slice recall loss under K=3). * - few-shot >> zero-shot, chain-of-thought does not help commit classification (arXiv 2605.02033). - * - self-consistency (sample N, majority-vote) reliably lifts a model; reasoning tiers give no - * advantage and are slow (arXiv 2510.22389) β€” so prefer *_SENTRY_SAMPLES over a reasoning tier. - * The eval/ benchmark sweeps {model, context, shots, samples}; the env defaults below are the cell the - * seed corpus picks (haiku + diff β€” see the CONTEXT_TIER note) β€” re-run the sweep on your own corpus. + * - self-consistency (majority vote) lifts a model; reasoning tiers don't and are slow (arXiv + * 2510.22389) β€” prefer *_SENTRY_SAMPLES. The eval/ benchmark sweeps {model, context, shots, + * samples}; the defaults below are the seed corpus's pick (haiku + diff) β€” re-sweep on your own. * * --gate : exit 0 = SKIP / warn-only / skipped / fail-open Β· exit 1 = hard mode + confident MONITOR Β· exit 2 = could-not-run * (no flag) : report mode β€” judge the given message and PRINT the verdict, exit 0. @@ -60,9 +59,8 @@ * * GOVERNING RULE (devkit "ship the generator, never the data"): every runtime path resolves against * the CONSUMER cwd, never __dirname. The WATCHLIST + the BASELINE stay the consumer's data (born in - * their repo, never shipped). The eval `cases.jsonl` DOES ship β€” 127 cases (104 real-derived + 23 - * authored elimination-tier) β€” but it is a - * dev-only SEED the gate never reads at runtime; a consumer copies + grows it with their own commits. + * their repo, never shipped). The eval `cases.jsonl` DOES ship (127 cases: 104 real-derived + 23 + * authored elimination-tier) but is a dev-only SEED the gate never reads at runtime. */ import { execSync } from 'node:child_process'; import { appendFileSync, existsSync, readFileSync, realpathSync } from 'node:fs'; @@ -73,6 +71,7 @@ import { focusHunks } from "../judge/diff-focus.mjs"; import { JUDGE_ISOLATION, JUDGE_READ_ONLY } from "../judge/judge-isolation.mjs"; import { reportGateInfraFailure } from "../judge/odb-probe.mjs"; import { execJudge } from "../judge/run-judge.mjs"; +import { judgeSentryWithCache } from "./verdict-cache.mjs"; // Read a GUARD_* env var, falling back to its FRINK_* alias for back-compat with the original frink // gate. Mirrors the config loader's envVar so every devkit gate reads env the same way. function envVar(name) { @@ -412,9 +411,13 @@ export function run(gate) { // Hard-by-default (envBool distinguishes unset β†’ hard from an explicit =0 soften); resolve it // BEFORE judging so the samples default can follow it. Report mode never blocks β†’ warn tier. const hard = gate && effectiveHard(envBool('SENTRY_HARD') ?? true, CONTEXT_TIER, diff); - const result = judge(buildContext(message, nameStatus, diff, CONTEXT_TIER), { - samples: resolveSamples(hard), - }); + const input = buildContext(message, nameStatus, diff, CONTEXT_TIER); + const opts = { model: MODEL, samples: resolveSamples(hard), prompt: SENTRY_JUDGE_PROMPT }; + // Diff-tier only: message/names evidence can't change with the demanded FIX, so a cached hard + // MONITOR would replay forever there. A bypassed run (SENTRY_NO_LLM) earns and replays nothing. + const result = envVar('SENTRY_NO_LLM') || CONTEXT_TIER !== 'diff' + ? judge(input, opts) + : judgeSentryWithCache(CWD, input, opts, () => judge(input, opts)); if (!gate) { console.log(reportLine(result)); process.exit(0); diff --git a/dist/gate-engine/sentry/verdict-cache.mjs b/dist/gate-engine/sentry/verdict-cache.mjs new file mode 100644 index 00000000..21cf6c74 --- /dev/null +++ b/dist/gate-engine/sentry/verdict-cache.mjs @@ -0,0 +1,73 @@ +/** + * Verdict cache for the sentry commit-msg judge β€” the one gate that used to re-bill its 3-sample + * haiku vote on EVERY commit attempt, including a byte-identical retry after ITS OWN hard block. + * + * Unlike the decisions cache (decisions/verdict-cache.mts, non-blocking verdicts only), this store + * caches BOTH verdicts by design: SKIP replays the pass, and a confident MONITOR replays the block + * instantly β€” the author is mid-fix-loop and an identical retry cannot flip a majority vote worth + * re-paying for. Anything unearned (outage, ambiguous/tied vote, no-LLM run) is never cached. + * DIFF TIER ONLY (the caller gates this): caching a MONITOR is sound only because adding the + * demanded capture changes the focused-diff evidence and so the key β€” on the message/names tiers + * the fix can't move the evidence and a cached block would replay forever. Escape hatch for a + * wedged entry: `rm .devkit/sentry-verdict-cache.json` (documented in docs/troubleshooting.md). + * + * Key = sha256 over the judge's EXACT inputs plus its identity: devkit version (an upgrade may + * change parsing), model, sample count (a 1-sample warn verdict must never replay as a 3-sample + * hard block, and vice versa), the prompt bytes (edits invalidate even between releases), and the + * full stdin payload (message + focused error-hunk evidence). Any restage that changes the + * error-hunks β€” including adding the demanded capture β€” re-judges. + * + * Storage/atomicity/failure direction: shared judge/verdict-store (`.devkit/sentry-verdict-cache + * .json`, main-checkout anchored, corrupt β†’ re-judge, failed write β†’ verdict stands, unremembered). + */ +import { createHash } from 'node:crypto'; +import { verdictKey } from "../decisions/verdict-cache.mjs"; +import { emitCacheHit, emitGateEvent } from "../judge/gate-events.mjs"; +import { devkitDataFile, loadEntries, saveEntries } from "../judge/verdict-store.mjs"; +const STORE_FILE = 'sentry-verdict-cache.json'; +const CACHEABLE = new Set(['MONITOR', 'SKIP']); +/** Stable cache key over the judge's exact inputs + identity. Built on the decisions cache's + * `verdictKey` (NUL-separated parts + devkit-version salt) so the two stores' key formulas cannot + * drift; the prompt rides as its own sha256 to keep the key line-length sane. */ +export function sentryVerdictKey(input, { model, samples, prompt }) { + const promptDigest = createHash('sha256').update(prompt).digest('hex'); + return verdictKey('sentry', model, samples, promptDigest, input); +} +/** + * Judge `input` through the cache: an earned verdict for these exact inputs replays without a + * judge call; a miss runs `judgeFn` and remembers a confident MONITOR/SKIP (best-effort β€” a failed + * write leaves the verdict standing for this run). Callers on a bypass path (NO_SENTRY_JUDGE, + * SENTRY_NO_LLM) must not reach this at all: a bypassed run earns nothing and must not replay a + * cached block the owner explicitly softened. + */ +export function judgeSentryWithCache(cwd, input, identity, judgeFn) { + const file = devkitDataFile(cwd, STORE_FILE); + const key = sentryVerdictKey(input, identity); + const hit = loadEntries(file)[key]; + if (hit && typeof hit.verdict === 'string' && CACHEABLE.has(hit.verdict)) { + console.error(`sentry-judge: cached ${hit.verdict} (identical message + error-hunk evidence)`); + emitCacheHit('sentry-advisory', hit.model, hit.duration_ms); + return { verdict: hit.verdict, evidence: String(hit.evidence ?? '') }; + } + const started = Date.now(); + const result = judgeFn(); + if (result?.verdict && CACHEABLE.has(result.verdict)) { + const saved = saveEntries(file, { + [key]: { + at: new Date().toISOString(), + verdict: result.verdict, + evidence: result.evidence, + model: identity.model, + samples: identity.samples, + duration_ms: Math.max(0, Date.now() - started), + }, + }); + if (!saved) { + // The verdict stands for this run; it just isn't remembered. Name it (gate-telemetry-self- + // describing): a silently lost write would read as a judge that keeps re-billing for no reason. + console.error('sentry-judge: verdict earned but NOT cached (store write failed)'); + emitGateEvent({ type: 'cache_write_failed', judge: 'sentry-advisory' }); + } + } + return result; +} diff --git a/docs/decisions/ship-gates-converge-not-restart.md b/docs/decisions/ship-gates-converge-not-restart.md index 9d64a418..62fce6ff 100644 --- a/docs/decisions/ship-gates-converge-not-restart.md +++ b/docs/decisions/ship-gates-converge-not-restart.md @@ -34,3 +34,4 @@ created: 2026-07-03 - 2026-08-05 β€” The ceiling now covers setup and teardown, not just the gate command. SHIP_COMMIT_TIMEOUT is read INSIDE run_gates_with_capture, so the ~245 lines of preflight in review-target.sh (two git worktree add checkouts, submodule + dependency materialization twice over, asset runtime, baseline capture) and the ~56 lines of teardown after it were unbounded β€” and nothing bounds review-target.sh from outside either (cli/commands/review.mts spawns it with no timeout). Reported symptom: a review sat 15+ minutes indistinguishable from a hang. A watchdog now bounds both stretches at DEVKIT_PREFLIGHT_TIMEOUT, which DEFAULTS to SHIP_COMMIT_TIMEOUT so operators keep one knob; the separate name exists because any value low enough to exercise the setup guard stops the run reaching the chain, leaving the two mutually untestable. Consequence knowingly accepted: two independent timers off one default means a worst case of 2x the knob, not 1x. It signals the process GROUP (kill -TERM -PGID), because bash defers a trapped signal until the running FOREGROUND command returns and never signals that child β€” a pid-targeted kill writes a timeout banner and then hangs anyway, which is strictly worse than the silent hang it replaces. Same mechanism run-packaged-script.mts already uses from the outside. Deliberately does NOT escalate to SIGKILL: on_exit's teardown runs in a signal-ignoring subshell so worktree removal cannot be interrupted, and a KILL would defeat that and strand the ephemeral worktrees checked out. An unkillable wedge is reported instead of forced. - 2026-08-05 β€” Promoting a rationale that lived only as a source comment (gate-engine/review/progress.mts:1-11), since it is the writer/reader contract behind this target's 'the timeout banner names the mid-flight stage + unfinished reviewers' guarantee. The ship banner used to awk the stderr log for 'guard-review: - ' lines to name the reviewers a timeout killed mid-flight; a wording tweak on either side silently broke it, and the banner then named nothing while still looking correct. run-review now writes {running, completed} reviewer names to the JSON file exported as DEVKIT_REVIEW_PROGRESS, and the banner reads it through one shared reader (guard-review unfinished ), so writer and reader cannot drift; an integration test exercises engine to file to reader together. Progress writes are best-effort by design - a missing or unparsable file means 'nothing to report' and must never fail the gate, because it is telemetry for a kill that may not come. Related: the run log now also carries per-phase setup/teardown lines, which is a separate human-facing channel and not part of this machine-readable contract. - 2026-08-06 β€” The reviewer cache key now salts on the devkit VERSION, closing a gap in guarantee (1) 'reviewer PASSes checkpoint per-completion'. cacheKey (reviewers.mts) hashed identitySalt + diff bytes, and identitySalt (hashReviewerIdentity, runtime.mts) covers reviewer ASSET bytes plus gate config only β€” so a devkit upgrade that changed review-ENGINE semantics (escalation policy, model selection, verdict parsing, lens grouping) left every asset byte-identical and replayed PASSes earned under the old engine. prefix-cache.mts already folded devkitVersion() in for exactly this reason ('upgrading devkit or editing the hook re-runs the gates'); the reviewer key did not, and sc-1437 salted the key with consumer-side reviewer IDENTITY without closing the engine axis. The salt is an injectable parameter defaulting to devkitVersion(), so reviewers.mts keeps its no-I/O contract under test. Convergence is unaffected within a version: retries of the same ship still hit. Cost knowingly accepted: a one-time full review-cache miss on each upgrade, which is the correct behaviour and bounded by the same cascade the first ship pays. Also in this change and recorded here because it lands in this Target's Scope: verdict-store's retention now emits cache_evicted (store, dropped, retained) instead of dropping cached PASSes silently, and the policy moved to gate-engine/judge/store-retention.mts. MAX_ENTRIES stays 100 β€” a replay of ~/.devkit/telemetry over 3,035 review judge executions found 99.5% of misses were on keys never cached before and ZERO evicted-then-needed, so the cap is not currently costing anything; the event exists so the next investigation is a query rather than a study. +- 2026-08-07 β€” Review-cache keys and waiver fingerprints now hash diffCacheIdentity(diff) (judge/diff-focus) instead of the raw staged bytes. What the identity EXCLUDES, in full: hunk line numbers, context lines, index blob shas, and additions whose whole line is an inert Sentry.-QUALIFIED capture (Sentry.captureException/captureMessage with identifier/string-literal arguments only β€” bare/unqualified calls including the captureMainMessage wrapper, nested calls, template literals, payload objects, and REMOVED captures all still invalidate, because a bare name's origin cannot be checked from one diff line; origin-checked sentry import lines do strip). What it KEEPS: file headers, every other Β± line in order, and each hunk's git function-context anchor β€” so relocating a change into a different function voids the key. Accepted residual: relocating a change WITHIN one function (same anchor, same Β± sequence) does not invalidate; bounded by the reviewer-identity/Targets/lens/version salts also in the key. Consequence: the restage that fixes a sentry-gate block keeps every earned reviewer PASS and recorded waiver. Migration: fingerprints for in-flight (uncommitted-diff) waivers in .devkit/correctness-overrides.json void once on upgrade β€” a one-time re-waive. The sentry judge itself now checkpoints SKIP and confident MONITOR in .devkit/sentry-verdict-cache.json keyed via the decisions verdictKey on its exact inputs (message + focused error-hunks + model/samples/promptHash/version), replaying identical retries instead of re-paying 3 haiku samples; a lost store write emits cache_write_failed. Anchors: each hunk keys on git's function context, or its restage-stable OLD-side start line where git emits none (JSON, top-of-file), so cross-anchor relocation always invalidates; hunk-less segments (binary/mode/rename) key on their verbatim bytes including index shas, and a diff that normalizes to NOTHING (a capture-only commit) degrades to exact-bytes keying so two unrelated capture-only commits can never share a key or waiver. The sentry cache is DIFF-TIER ONLY β€” on message/names tiers the demanded fix cannot change the evidence, so a cached hard MONITOR would replay forever; escape hatch for a provably-stale entry: rm .devkit/sentry-verdict-cache.json (docs/troubleshooting.md). This store is the FOURTH cache file under .devkit/ β€” the Negative above ('Three cache files') predates it. Judges always still read the raw diff; only key inputs are normalized. diff --git a/docs/glossary.md b/docs/glossary.md index edfd98b4..0b1a0891 100644 --- a/docs/glossary.md +++ b/docs/glossary.md @@ -53,7 +53,10 @@ The jargon you'll meet in devkit's help, prompts, and gate output β€” in one pla - **checkpointed verdicts** β€” earned AI verdicts persist per-completion so a killed ship re-runs only unfinished work: reviewer PASSes checkpoint as each reviewer finishes (not batched at the end), and decisions ROUTINE/ALIGN/depth-PASS verdicts cache on their exact evidence bytes (`.devkit/`). This is what - lets a ship retry **converge** instead of restart. Drop them with `guard-review clear-cache`. + lets a ship retry **converge** instead of restart. Drop them with `guard-review clear-cache`. The sentry + commit-msg judge checkpoints too (`.devkit/sentry-verdict-cache.json`) β€” uniquely it also replays a + confident MONITOR (a block) for byte-identical diff-tier evidence; any restage re-judges, and `rm` on the + store file resets it. ## Config & structure diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index b589648a..2a22ed8c 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -70,10 +70,14 @@ it was mid-flight in and any reviewers missing a completion heartbeat. For more ## A `.devkit/` ship cache looks stale (gates pass when they shouldn't) The **deterministic-prefix cache** and **checkpointed verdicts** live under `.devkit/`, keyed on the staged-tree hash and evidence bytes. They can go stale against **gitignored** inputs a gate reads but the -key can't see (e.g. the search-code index behind `guard-dup`). Escape hatches β€” both only discard cached -*passes*, never hide a failure: +key can't see (e.g. the search-code index behind `guard-dup`). Escape hatches β€” the first two only +discard cached *passes*, never hide a failure: - `guard-prefix clear` β€” drop the cached all-green deterministic prefix (forces a full deterministic re-run). - `guard-review clear-cache` β€” drop cached reviewer PASS verdicts (forces the reviewers to re-run). +- `rm .devkit/sentry-verdict-cache.json` β€” drop cached sentry-judge verdicts. Unlike the two above, this + store also persists a confident **MONITOR** (a block): a byte-identical retry replays it **by design**, + and any restage of the staged diff re-judges (the cache is diff-tier-only), so remove the file only when + a cached block is provably stale (e.g. after rolling devkit back). ## `βœ— deterministic gates failed: ` The deterministic gates (structure, fanout, size, dup, clone …) run all-and-**aggregate**: instead of diff --git a/gate-engine/judge/__tests__/diff-focus.test.mts b/gate-engine/judge/__tests__/diff-focus.test.mts index 42e7b68f..b601173a 100644 --- a/gate-engine/judge/__tests__/diff-focus.test.mts +++ b/gate-engine/judge/__tests__/diff-focus.test.mts @@ -1,8 +1,12 @@ // Unit tests for the shared diff-evidence primitives (split + hunk focus) used by the sentry, detect, // and reviewer judge gates. +import { execSync } from 'node:child_process'; +import { mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; import { describe, expect, it } from 'vitest'; -import { filePathOf, focusHunks, splitDiffByFile } from '../diff-focus.mts'; +import { diffCacheIdentity, filePathOf, focusHunks, splitDiffByFile } from '../diff-focus.mts'; const git = (path: string, hunk: string) => `diff --git a/${path} b/${path}\nindex 111..222 100644\n--- a/${path}\n+++ b/${path}\n@@ -1,2 +1,3 @@\n${hunk}`; @@ -69,3 +73,259 @@ describe('focusHunks', () => { ); }); }); + +describe('diffCacheIdentity', () => { + // Real-git harness: the geometry claims (hunk extension, new hunks, merges, header shifts) are + // asserted against diffs GIT actually produces, not hand-authored approximations. + const gitDiffOf = (base: string, staged: string): string => { + const dir = mkdtempSync(join(tmpdir(), 'dci-')); + try { + execSync('git init -q && git config user.email t@t && git config user.name t', { cwd: dir }); + writeFileSync(join(dir, 'app.ts'), base); + execSync('git add . && git commit -qm base', { cwd: dir }); + writeFileSync(join(dir, 'app.ts'), staged); + execSync('git add .', { cwd: dir }); + return execSync('git -c diff.noprefix=false diff --cached', { cwd: dir, encoding: 'utf8' }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + const BASE = Array.from({ length: 40 }, (_, i) => `line${i}();`).join('\n'); + const edit = (at: number, insert: string[], from = BASE) => { + const lines = from.split('\n'); + lines.splice(at, 0, ...insert); + return lines.join('\n'); + }; + + it.each([ + ['inside the touched hunk', 11], + ['far away (its own new hunk + fresh context)', 30], + ['adjacent (hunks merge)', 14], + ])('stable when a capture lands %s', (_label, at) => { + const fixed = edit(10, ['handle();']); // the "real" change, committed intent + const d1 = gitDiffOf(BASE, fixed); + const d2 = gitDiffOf(BASE, edit(at, ['Sentry.captureException(e);'], fixed)); + expect(d2).not.toBe(d1); // git really did reshape the diff… + expect(diffCacheIdentity(d2)).toBe(diffCacheIdentity(d1)); // …but the identity held + }); + + it('strips a wrapper-name import when the path itself names sentry', () => { + const fixed = edit(10, ['handle();']); + const d1 = gitDiffOf(BASE, fixed); + const d2 = gitDiffOf( + BASE, + edit(0, ["import { captureMainMessage } from './lib/sentry';"], fixed), + ); + expect(diffCacheIdentity(d2)).toBe(diffCacheIdentity(d1)); + }); + + it('stable across awaited captures, imports, and several at once', () => { + const fixed = edit(10, ['handle();']); + const d1 = gitDiffOf(BASE, fixed); + const d2 = gitDiffOf( + BASE, + edit( + 30, + ['await Sentry.captureException(err);', 'Sentry.captureMessage("x");'], + edit(0, ["import * as Sentry from '@sentry/electron';"], fixed), + ), + ); + expect(diffCacheIdentity(d2)).toBe(diffCacheIdentity(d1)); + }); + + it('drops a whole file segment that only gained sentry lines', () => { + const twoFile = (extra: string | null): string => { + const dir = mkdtempSync(join(tmpdir(), 'dci-')); + try { + execSync('git init -q && git config user.email t@t && git config user.name t', { + cwd: dir, + }); + writeFileSync(join(dir, 'app.ts'), BASE); + writeFileSync(join(dir, 'other.ts'), BASE); + execSync('git add . && git commit -qm base', { cwd: dir }); + writeFileSync(join(dir, 'app.ts'), edit(10, ['handle();'])); + if (extra !== null) writeFileSync(join(dir, 'other.ts'), edit(20, [extra])); + execSync('git add .', { cwd: dir }); + return execSync('git -c diff.noprefix=false diff --cached', { cwd: dir, encoding: 'utf8' }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + expect(diffCacheIdentity(twoFile('Sentry.captureMessage("degraded");'))).toBe( + diffCacheIdentity(twoFile(null)), + ); + }); + + it.each([ + ['a real line rides along', ['Sentry.captureException(e);', 'refund(user);']], + ['multi-line payload capture (opening line only)', ['Sentry.captureException(e, {']], + ])('invalidates when %s', (_label, insert) => { + const fixed = edit(10, ['handle();']); + const d1 = gitDiffOf(BASE, fixed); + const d2 = gitDiffOf(BASE, edit(30, insert, fixed)); + expect(diffCacheIdentity(d2)).not.toBe(diffCacheIdentity(d1)); + }); + + it('two unrelated capture-ONLY commits never collide on an empty identity', () => { + const a = gitDiffOf(BASE, edit(10, ['Sentry.captureException(err);'])); + const b = gitDiffOf(BASE, edit(25, ['Sentry.captureMessage("degraded");'])); + expect(diffCacheIdentity(a)).not.toBe(''); + expect(diffCacheIdentity(a)).not.toBe(diffCacheIdentity(b)); // exact-bytes degradation + }); + + it('invalidates when a capture is REMOVED (deletions are never stripped)', () => { + const withCapture = edit(10, ['Sentry.captureException(e);']); + const d = gitDiffOf(withCapture, BASE); + expect(diffCacheIdentity(d)).not.toBe(''); + expect(diffCacheIdentity(d)).toContain('-Sentry.captureException(e);'); + }); + + it('distinguishes two genuinely different diffs and ignores blob-sha churn', () => { + const a = gitDiffOf(BASE, edit(10, ['alpha();'])); + const b = gitDiffOf(BASE, edit(10, ['beta();'])); + expect(diffCacheIdentity(a)).not.toBe(diffCacheIdentity(b)); + expect(diffCacheIdentity(a)).not.toContain('index '); // sha lines out of the identity + }); + + it('fixpoint: hunk-less input passes through VERBATIM instead of vanishing', () => { + expect(diffCacheIdentity('not a diff at all')).toBe('not a diff at all'); + const modeOnly = 'diff --git a/x.sh b/x.sh\nold mode 100644\nnew mode 100755'; + expect(diffCacheIdentity(modeOnly)).toBe(modeOnly); + }); + + it('binary segments keep their index shas β€” different blobs must not collide', () => { + const bin = (shas: string) => + `diff --git a/blob.bin b/blob.bin\nindex ${shas} 100644\nBinary files a/blob.bin and b/blob.bin differ`; + expect(diffCacheIdentity(bin('1111111..2222222'))).not.toBe( + diffCacheIdentity(bin('1111111..3333333')), + ); + }); +}); + +// The three holes the correctness reviewer proved in the first shipped draft (opus-confirmed): +// bare foreign capture names, nested-call arguments, and position-blind relocation. +describe('diffCacheIdentity β€” conservative matching + function anchors', () => { + const gitDiffOf = (base: string, staged: string): string => { + const dir = mkdtempSync(join(tmpdir(), 'dci2-')); + try { + execSync('git init -q && git config user.email t@t && git config user.name t', { cwd: dir }); + writeFileSync(join(dir, 'app.ts'), base); + execSync('git add . && git commit -qm base', { cwd: dir }); + writeFileSync(join(dir, 'app.ts'), staged); + execSync('git add .', { cwd: dir }); + return execSync('git -c diff.noprefix=false diff --cached', { cwd: dir, encoding: 'utf8' }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + const BASE = Array.from({ length: 40 }, (_, i) => `line${i}();`).join('\n'); + const edit = (at: number, insert: string[], from = BASE) => { + const lines = from.split('\n'); + lines.splice(at, 0, ...insert); + return lines.join('\n'); + }; + + it.each([ + ['a BARE capture from an arbitrary module', 'captureException(userInput);'], + ['a BARE wrapper call (no origin check possible)', 'captureMainMessage(chatId, text);'], + [ + 'a capture-name import from a path merely CONTAINING "sentry"', + "import { captureException } from '../utils/presentry-shim';", + ], + ['a nested call smuggled as the argument', 'Sentry.captureException(mutateGlobalState(err));'], + ['a template-literal argument', 'Sentry.captureMessage(`${sideEffect()}`);'], + ])('does NOT strip %s β€” the restage re-reviews', (_label, line) => { + const fixed = edit(10, ['handle();']); + const d1 = gitDiffOf(BASE, fixed); + const d2 = gitDiffOf(BASE, edit(30, [line], fixed)); + expect(diffCacheIdentity(d2)).not.toBe(diffCacheIdentity(d1)); + }); + + it('relocating the same added line into a DIFFERENT function voids the key (anchors)', () => { + const fn = (name: string) => + [ + `function ${name}() {`, + ...Array.from({ length: 10 }, (_, i) => ` ${name}${i}();`), + '}', + ].join('\n'); + const twoFns = `${fn('alpha')}\n${fn('beta')}`; + const insertAt = (line: number) => { + const lines = twoFns.split('\n'); + lines.splice(line, 0, ' probe();'); + return lines.join('\n'); + }; + const inAlpha = gitDiffOf(twoFns, insertAt(6)); + const inBeta = gitDiffOf(twoFns, insertAt(18)); + expect(diffCacheIdentity(inAlpha)).not.toBe(diffCacheIdentity(inBeta)); + expect(diffCacheIdentity(inAlpha)).toContain('@ function alpha'); + expect(diffCacheIdentity(inBeta)).toContain('@ function beta'); + }); + + it('two same-function hunks merging via a capture insertion still hit (anchor dedupe)', () => { + const fixed = edit(14, ['later();'], edit(10, ['handle();'])); + const d1 = gitDiffOf(BASE, fixed); + const merged = gitDiffOf(BASE, edit(12, ['Sentry.captureException(e);'], fixed)); + expect(diffCacheIdentity(merged)).toBe(diffCacheIdentity(d1)); + }); +}); + +// Anchorless file types (git emits no function context for JSON): the completeness gate proved a +// whole-file relocation collision β€” the old-side start-line fallback anchor closes it. +describe('diffCacheIdentity β€” anchorless files fall back to old-side position anchors', () => { + const gitDiffOf = (name: string, base: string, staged: string): string => { + const dir = mkdtempSync(join(tmpdir(), 'dci3-')); + try { + execSync('git init -q && git config user.email t@t && git config user.name t', { cwd: dir }); + writeFileSync(join(dir, name), base); + execSync('git add . && git commit -qm base', { cwd: dir }); + writeFileSync(join(dir, name), staged); + execSync('git add .', { cwd: dir }); + return execSync('git -c diff.noprefix=false diff --cached', { cwd: dir, encoding: 'utf8' }); + } finally { + rmSync(dir, { recursive: true, force: true }); + } + }; + const arr = (name: string, items: string[]) => + ` "${name}": [\n${items.map((i) => ` "${i}",`).join('\n')}\n ]`; + const json = (trusted: string[], pub: string[]) => + `{\n${arr('trustedOrigins', trusted)},\n${arr('publicOrigins', pub)}\n}\n`; + const BASE = json( + ['a.example.com', 'b.example.com', 'c.example.com', 'd.example.com', 'e.example.com'], + ['f.example.com', 'g.example.com', 'h.example.com', 'i.example.com', 'j.example.com'], + ); + + it('the same added line in two different JSON blocks yields two different identities', () => { + const inTrusted = gitDiffOf( + 'origins.json', + BASE, + json( + [ + 'a.example.com', + 'b.example.com', + 'evil.example.com', + 'c.example.com', + 'd.example.com', + 'e.example.com', + ], + ['f.example.com', 'g.example.com', 'h.example.com', 'i.example.com', 'j.example.com'], + ), + ); + const inPublic = gitDiffOf( + 'origins.json', + BASE, + json( + ['a.example.com', 'b.example.com', 'c.example.com', 'd.example.com', 'e.example.com'], + [ + 'f.example.com', + 'g.example.com', + 'evil.example.com', + 'h.example.com', + 'i.example.com', + 'j.example.com', + ], + ), + ); + expect(diffCacheIdentity(inTrusted)).not.toBe(diffCacheIdentity(inPublic)); + expect(diffCacheIdentity(inTrusted)).toMatch(/@ :\d+/); // positional fallback anchor in play + }); +}); diff --git a/gate-engine/judge/diff-focus.mts b/gate-engine/judge/diff-focus.mts index 6a216d27..75bb0c8e 100644 --- a/gate-engine/judge/diff-focus.mts +++ b/gate-engine/judge/diff-focus.mts @@ -66,3 +66,108 @@ export function focusHunks( const note = omitted ? `[${omitted} ${omitNoun} hunk(s) omitted]\n` : ''; return `${header}${note}${kept.join('\n')}`.trim(); } + +// A purely-additive Sentry instrumentation line β€” matched CONSERVATIVELY, because every match is +// erased from the cache identity and can therefore never invalidate an earned verdict: +// - The call must be `Sentry.`-QUALIFIED. NO bare names, wrappers included: a bare +// `captureException(...)` or `captureMainMessage(...)` from an arbitrary module may be a local +// function carrying real logic and must invalidate β€” origin cannot be checked from one diff +// line, so unqualified calls simply cost a re-review. +// - Arguments must be inert: identifier/member chains or plain string literals, comma-separated. +// No nested calls (`captureException(mutate(err))` is REAL logic riding along), no template +// literals, no object payloads β€” a multi-line capture's opening line ends in `{`/`(` and fails +// the whole-line match anyway. +// - Imports ARE origin-checked, so they may name the wrapper: anything from an `@sentry/*` +// package, or a named import of only the recognized capture names from a path naming sentry. +const SENTRY_ARG = /(?:[\w$.]+|'[^']*'|"[^"]*")/; +const SENTRY_CAPTURE_RE = new RegExp( + `^(?:await\\s+)?(?:void\\s+)?Sentry\\.capture(?:Exception|Message)\\(\\s*(?:${SENTRY_ARG.source}(?:\\s*,\\s*${SENTRY_ARG.source})*\\s*)?\\);?$`, +); +// The named-import alternative's path must NAME sentry as its final segment's leading word +// (`./sentry`, `../lib/sentry-client`, `electron/sentry.main`) β€” a substring match would also strip +// e.g. `../utils/presentry-shim`, a local module whose real logic must invalidate. +const SENTRY_IMPORT_RE = + /^import\s+(?:type\s+)?(?:\*\s+as\s+\w+|\w+|\{[^}]*\})\s+from\s+['"]@sentry\/[^'"]+['"];?$|^import\s+\{\s*capture(?:Exception|Message|MainMessage)(?:\s*,\s*capture(?:Exception|Message|MainMessage))*\s*\}\s+from\s+['"](?:[^'"]*\/)?[sS]entry(?:[-.][^'"]*)?['"];?$/; + +const HUNK_HEADER_RE = /^@@ /; +const HUNK_FUNC_RE = /^@@ [^@]*@@ ?/; +const HUNK_OLD_START_RE = /^@@ -(\d+)/; +const INDEX_LINE_RE = /^index [0-9a-f]+\.\.[0-9a-f]+/; + +function sentryAdditive(content: string): boolean { + return SENTRY_CAPTURE_RE.test(content) || SENTRY_IMPORT_RE.test(content); +} + +/** + * The diff's CACHE IDENTITY: the text hashed into a gate's verdict-cache key, normalized so that a + * restage whose only delta is purely-additive Sentry instrumentation (the exact fix the sentry gate + * blocks for) hashes identically to the pre-fix diff β€” earned reviewer PASSes survive the fix + * commit instead of re-billing the whole fleet for one capture line. + * + * Why headers AND context must go: inserting one line shifts every later `@@` coordinate, can + * extend a hunk's trailing context, materialize a brand-new hunk (~6 context lines the old diff + * never showed), or merge adjacent hunks β€” so any surviving context/`@@`/`index` byte would still + * miss. What remains per file is its header identity plus the ordered `Β±` content lines, with + * sentry-additive `+` lines dropped (`-` lines NEVER dropped: removing instrumentation is a real + * change). A file whose surviving body is empty (touched only by sentry lines) drops entirely. + * + * What survives per file: its header identity, plus each hunk's anchor followed by that hunk's + * surviving `Β±` lines. The anchor is git's FUNCTION CONTEXT (the text after the second `@@` β€” + * stable under line-number shifts); where git emits none (JSON, hunks above a file's first + * anchor-matching line) it is the hunk's OLD-side start line, which a purely-additive restage + * cannot move (both diffs share the pre-image). Consecutive identical anchors collapse to one, so + * two same-function hunks merging (a capture inserted between them) keeps the identity stable, + * while relocating a change into a different function β€” or, for anchorless types, a different + * old-side position β€” voids the key. Deliberate residual weakening: context bytes and hunked + * files' blob shas are excluded, so relocating a change WITHIN one anchor span (same anchor, same + * `Β±` sequence) does not invalidate β€” a narrow residual, and every consumer still salts the key + * with reviewer identity, Targets, lens group, and devkit version. NOT for judge evidence β€” + * judges read the raw diff. + * + * Fixpoint guarantee: a hunk-less segment (binary / mode-only / rename-only / non-diff text) + * passes through VERBATIM, `index` shas included β€” a binary blob's sha is its only content + * identity β€” so unexpected input degrades to exact-bytes keying. + */ +export function diffCacheIdentity(diff: string): string { + const out: string[] = []; + for (const seg of splitDiffByFile(diff)) { + const lines = seg.split('\n'); + const firstHunk = lines.findIndex((l) => HUNK_HEADER_RE.test(l)); + if (firstHunk === -1) { + // Hunk-less segment (binary / mode-only / rename-only / non-diff text): keep it VERBATIM, + // `index` shas included β€” for a git-binary file those shas are the only content identity, + // and they move only when the blob does. This is the exact-bytes degradation path. + if (seg.trim()) out.push(seg.trim()); + continue; + } + const header = lines.slice(0, firstHunk).filter((l) => !INDEX_LINE_RE.test(l)); + const body: string[] = []; + let anchor = ''; // current hunk's anchor; emitted lazily, deduped consecutively + let emitted: string | null = null; + for (const line of lines.slice(firstHunk)) { + if (HUNK_HEADER_RE.test(line)) { + // Anchor on git's function context; where git emits none (JSON, top-of-file hunks, .sh + // preambles), fall back to the OLD-side start line β€” stable across a purely-additive + // restage (the pre-image is shared), yet distinct across relocations within the file. + const func = line.replace(HUNK_FUNC_RE, ''); + anchor = func ? `@ ${func}` : `@ :${line.match(HUNK_OLD_START_RE)?.[1] ?? '?'}`; + continue; + } + if (line.startsWith('-') || (line.startsWith('+') && !sentryAdditive(line.slice(1).trim()))) { + // `+++` can't reach here β€” file headers all precede the first `@@`, so every `+` is an add. + if (anchor !== emitted) { + body.push(anchor); + emitted = anchor; + } + body.push(line); + } + // everything else β€” ` ` context, blank context, `\ No newline` β€” is dropped + } + if (body.length) out.push([...header, ...body].join('\n')); + } + // A diff whose EVERY line normalized away (a wholly-sentry-additive commit) must not collapse to + // the one shared empty identity β€” two unrelated capture-only commits would collide on a key and + // share a verdict/waiver. There is no prior verdict such a commit could converge to anyway, so + // degrade it to exact-bytes keying. + return out.length ? out.join('\n') : String(diff); +} diff --git a/gate-engine/review/__tests__/lens-split.test.mts b/gate-engine/review/__tests__/lens-split.test.mts index 4db3c6eb..e47c0a54 100644 --- a/gate-engine/review/__tests__/lens-split.test.mts +++ b/gate-engine/review/__tests__/lens-split.test.mts @@ -332,3 +332,38 @@ describe('mergeItemVectors β€” per-lens attribution across a split', () => { expect((merged as unknown as { itemCount?: number }).itemCount).toBeUndefined(); }); }); + +// The sentry-gate fix loop: the ONLY delta between attempts is the capture line the gate demanded. +// Keys hash diffCacheIdentity(diff), so every PASS earned on the pre-fix diff must survive it. +describe('planReviewWork β€” a sentry-additive restage keeps earned keys', () => { + const sel = { reviewer: base, files: ['src/a.ts'] }; + const key = (n: string, d: string, salt: string) => `${n}|${d}|${salt}`; + const d1 = + 'diff --git a/src/a.ts b/src/a.ts\nindex 1111111..2222222 100644\n--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1,2 +1,3 @@\n ctx();\n+handle();\n more();\n'; + const d2 = + 'diff --git a/src/a.ts b/src/a.ts\nindex 1111111..3333333 100644\n--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1,2 +1,4 @@\n ctx();\n+handle();\n+Sentry.captureException(e);\n more();\n'; + + it('a PASS earned pre-fix is a full cache hit after the capture-only restage (all lens groups)', () => { + const before = planReviewWork([sel], [d1], {}, new Map(), key, DEFAULT_LENS_GROUPS); + const cache = Object.fromEntries(before.tasks.map((t) => [t.key, { at: 'n' }])); + const after = planReviewWork([sel], [d2], cache, new Map(), key, DEFAULT_LENS_GROUPS); + expect(after.tasks).toHaveLength(0); + expect(after.fullyCached).toHaveLength(1); + expect(after.scope[0].cached).toBe(true); + }); + + it('judges/transcripts still receive the RAW restaged diff, never the normalized identity', () => { + const plan = planReviewWork([sel], [d2], {}, new Map(), key, null); + expect(plan.tasks[0].diffText).toBe(d2); + expect(plan.scope[0].diff).toBe(d2); + }); + + it('a real change riding along with the capture re-runs every group', () => { + const d3 = d2.replace('+Sentry.captureException(e);', '+refund(user);'); + const before = planReviewWork([sel], [d1], {}, new Map(), key, DEFAULT_LENS_GROUPS); + const cache = Object.fromEntries(before.tasks.map((t) => [t.key, { at: 'n' }])); + const after = planReviewWork([sel], [d3], cache, new Map(), key, DEFAULT_LENS_GROUPS); + expect(after.tasks).toHaveLength(DEFAULT_LENS_GROUPS.length); + expect(after.fullyCached).toHaveLength(0); + }); +}); diff --git a/gate-engine/review/__tests__/overrides.test.mts b/gate-engine/review/__tests__/overrides.test.mts index 3a9ac575..26e9b41f 100644 --- a/gate-engine/review/__tests__/overrides.test.mts +++ b/gate-engine/review/__tests__/overrides.test.mts @@ -236,3 +236,25 @@ describe('reconcile β€” author pass-through', () => { }); }); }); + +// fingerprint hashes diffCacheIdentity(diff): a waiver recorded pre-fix must survive the +// purely-sentry-additive restage the sentry gate demands, and void on anything more. +describe('fingerprint across a sentry-additive restage', () => { + const d1 = + 'diff --git a/src/a.ts b/src/a.ts\nindex 1111111..2222222 100644\n--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1,2 +1,3 @@\n ctx();\n+handle();\n more();\n'; + const d2 = + 'diff --git a/src/a.ts b/src/a.ts\nindex 1111111..3333333 100644\n--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1,2 +1,4 @@\n ctx();\n+handle();\n+Sentry.captureException(e);\n more();\n'; + + it('survives the capture-only restage', () => { + expect(fingerprint('correctness-reviewer', 'concurrency-races', d1)).toBe( + fingerprint('correctness-reviewer', 'concurrency-races', d2), + ); + }); + + it('voids when a real line rides along', () => { + const d3 = d2.replace('+Sentry.captureException(e);', '+refund(user);'); + expect(fingerprint('correctness-reviewer', 'concurrency-races', d1)).not.toBe( + fingerprint('correctness-reviewer', 'concurrency-races', d3), + ); + }); +}); diff --git a/gate-engine/review/lens/split.mts b/gate-engine/review/lens/split.mts index 7115bd1a..a245b1e9 100644 --- a/gate-engine/review/lens/split.mts +++ b/gate-engine/review/lens/split.mts @@ -33,6 +33,7 @@ * derived clones would void every committed waiver and split the telemetry in two. */ +import { diffCacheIdentity } from '../../judge/diff-focus.mts'; import { emitGateEvent } from '../../judge/gate-events.mts'; import { composeTranscript, saveTranscript } from '../../judge/transcript-store.mts'; import { itemFields, mergeItemVectors } from '../evidence/items.mts'; @@ -329,17 +330,21 @@ export function planReviewWork( const sel = selected[i]; const name = sel.reviewer.name; const salt = salts.get(name) ?? ''; + // Keys hash the diff's CACHE IDENTITY (sentry-additive lines normalized out) so a restage whose + // only delta is the capture the sentry gate demanded keeps every earned PASS. Judges, transcripts + // and scope rows still get the RAW diffs[i] β€” only the key input is normalized. + const idText = diffCacheIdentity(diffs[i]); const split = groups && name === 'correctness-reviewer' && sel.reviewer.skill ? groups : null; const parts: ReviewTask[] = split ? split.map((g) => ({ sel: { ...sel, reviewer: deriveLensReviewer(sel.reviewer as ChecklistReviewer, g) }, - key: keyOf(name, diffs[i], `${salt}|split:${lensGroupId(g)}`), + key: keyOf(name, idText, `${salt}|split:${lensGroupId(g)}`), diffText: diffs[i], splitOf: name, group: lensGroupId(g), base: sel, })) - : [{ sel, key: keyOf(name, diffs[i], salt), diffText: diffs[i], base: sel }]; + : [{ sel, key: keyOf(name, idText, salt), diffText: diffs[i], base: sel }]; const allCached = parts.every((p) => Boolean(cache[p.key])); scope.push({ sel, diff: diffs[i], cached: allCached }); if (allCached) { diff --git a/gate-engine/review/overrides.mts b/gate-engine/review/overrides.mts index 1050bf982be0fa7d0a3ff64596bc3c73eb7b2d4d..bca23d09020f1c25040d9974958128582341d6c8 100644 GIT binary patch delta 705 zcmaiyK}#D!6vv@1iZzHHJXk0^i;%3ERp?2J2Z;f5Ne>1^Je29q+uhOKnJ_b(xSA4= z-q$bCORpC4LHr1UpTwDof{j9Nk9}|c@Be=P{axG-{{EO6QWixG5+>N9$TibNO(zIS zlNjkoSOeP!ra@wlVIQgQ`|x;u3vP;J2wcdFr0fzYuxAO^96zXsRvXT-P>?Hx+F4Fx z+_b9oNQC*B@;B@nC4fLTmzWlHWsJz8PAFxX_0X(#c8myil^K&53Q5hrJ~3?x{e-0m zgcqhmQV0{mV}zsct^Gsm+`1)ZM+x4Svgz$i*?Q=$mVdl$TaO;R(+^*-+@=4JfL{YUiyYHt`jPm^b+jH7yuQ{;LB zGNQwT@%VkFelUbRyJM1xiAQ22q=_<@J%dR(f&UP8p*s2y6OBMIBb+LikSWY>!P4}Z ztl$p$+$-kgCvRmj?{-aKHm7#;7=Gq)dAG1sKFqb}L&23cm|~MB?3Q=4pV!}zajZIc cp?yCXPu8jlUR3>WogWZX_1Hp&b2T&_p z;d)=d!dG%{L%?okzQ6f?Uq0%u$IdI~1eS3@5~Rv9Pz;8i6|G68>9_}{Fkw)(<;)8L z6sEM+Q_jpvTPQOK2Dqe+JZaa0Jfz#37iOv=ISRB8Ft3>`prL?V%e2ivE#dxla&rf` z;&O?s1QtaN>zs6uZhrz%N&=CiUXXQ1@kvklG%jlI;+~=__Zba3?fes6Hr?Hao#w4; uqGPI1@V4jXgMUTc`{=-7Jav2h#C^xGKckA%u~a^TlVMKPFl{dShv$FZC}_a| diff --git a/gate-engine/sentry/__tests__/check-sentry.test.mts b/gate-engine/sentry/__tests__/check-sentry.test.mts index 12aac2d1..2a59b552 100644 --- a/gate-engine/sentry/__tests__/check-sentry.test.mts +++ b/gate-engine/sentry/__tests__/check-sentry.test.mts @@ -3,7 +3,7 @@ // every pure rule is exercised via it.each so the assertions read as data, not boilerplate. import { spawnSync } from 'node:child_process'; -import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -398,7 +398,7 @@ describe('run dispatch (in-process; covers readMessage + skipReason + applyGateR describe('check-sentry gate β€” fail-open / bypass / free-skip (provider-absent degrades silently)', () => { const gate = (env, msg) => spawnSync('node', [SCRIPT, '--gate', msg], { - env: { ...process.env, ...env }, + env: { ...process.env, ...privateStore(), ...env }, encoding: 'utf8', }); // Stub `claude` on PATH so the real judge path runs (runJudgeOnce + the sample loop). Stubs return @@ -415,6 +415,15 @@ describe('check-sentry gate β€” fail-open / bypass / free-skip (provider-absent afterEach(() => { while (stubs.length) rmSync(stubs.pop(), { recursive: true, force: true }); }); + // The sentry verdict cache anchors to the real checkout (git-common-dir), so a judged spawn would + // read a prior test's verdict AND pollute the developer's .devkit. Every gate() spawn gets a + // private store root (the managed-review redirect). realpath: reviewDataRoot rejects the + // /varβ†’/private/var alias. + const privateStore = () => { + const dir = realpathSync(mkdtempSync(join(tmpdir(), 'sentry-store-'))); + stubs.push(dir); + return { DEVKIT_RUN_MODE: 'review', DEVKIT_REVIEW_DATA_ROOT: dir }; + }; it('GUARD_NO_SENTRY_JUDGE=1 skips entirely β†’ exit 0', () => { expect(gate({ GUARD_NO_SENTRY_JUDGE: '1' }, 'fix(x): y').status).toBe(0); diff --git a/gate-engine/sentry/__tests__/sentry-hard-defaults.test.mts b/gate-engine/sentry/__tests__/sentry-hard-defaults.test.mts index f046087f..500fbf58 100644 --- a/gate-engine/sentry/__tests__/sentry-hard-defaults.test.mts +++ b/gate-engine/sentry/__tests__/sentry-hard-defaults.test.mts @@ -5,7 +5,7 @@ // covered in check-sentry.test.mts). import { spawnSync } from 'node:child_process'; -import { chmodSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs'; +import { chmodSync, mkdtempSync, realpathSync, rmSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import { dirname, join, resolve } from 'node:path'; import { fileURLToPath } from 'node:url'; @@ -47,15 +47,20 @@ describe('gate mode is hard by default (spawned; stubbed claude, message tier, t return `${dir}:${process.env.PATH}`; }; // MONITOR on the warn path appends to the watchlist β€” point it at a tmp file, never the repo's. - // The tmp dir joins `stubs` so afterEach reclaims it with the claude stubs. + // The verdict cache likewise anchors to the real checkout, so each spawn gets a private store + // root (realpath: reviewDataRoot rejects the /varβ†’/private/var alias). Both tmp dirs join + // `stubs` so afterEach reclaims them with the claude stubs. const gate = (env: Record, msg: string) => { const wlDir = mkdtempSync(join(tmpdir(), 'sentry-hard-wl-')); - stubs.push(wlDir); + const storeDir = realpathSync(mkdtempSync(join(tmpdir(), 'sentry-hard-store-'))); + stubs.push(wlDir, storeDir); return spawnSync('node', [SCRIPT, '--gate', msg], { env: { ...process.env, GUARD_SENTRY_CONTEXT: 'message', GUARD_SENTRY_WATCHLIST: join(wlDir, 'wl.md'), + DEVKIT_RUN_MODE: 'review', + DEVKIT_REVIEW_DATA_ROOT: storeDir, ...env, }, encoding: 'utf8', diff --git a/gate-engine/sentry/__tests__/verdict-cache.test.mts b/gate-engine/sentry/__tests__/verdict-cache.test.mts new file mode 100644 index 00000000..0814b2f5 --- /dev/null +++ b/gate-engine/sentry/__tests__/verdict-cache.test.mts @@ -0,0 +1,88 @@ +// The sentry judge's verdict cache: an identical commit attempt must replay its earned verdict +// (including a hard-block MONITOR) instead of re-billing the 3-sample judge, and anything +// unearned (null / ambiguous) must never be remembered. + +import { mkdtempSync, rmSync } from 'node:fs'; +import { tmpdir } from 'node:os'; +import { join } from 'node:path'; +import { afterEach, describe, expect, it, vi } from 'vitest'; +import { judgeSentryWithCache, sentryVerdictKey } from '../verdict-cache.mts'; + +const IDENTITY = { model: 'haiku', samples: 3, prompt: 'PROMPT' }; + +describe('sentryVerdictKey', () => { + it('differs on every identity part (input, model, samples, prompt)', () => { + const base = sentryVerdictKey('INPUT', IDENTITY); + expect(sentryVerdictKey('OTHER', IDENTITY)).not.toBe(base); + expect(sentryVerdictKey('INPUT', { ...IDENTITY, model: 'sonnet' })).not.toBe(base); + // A 1-sample warn verdict must never replay as a 3-sample hard block: + expect(sentryVerdictKey('INPUT', { ...IDENTITY, samples: 1 })).not.toBe(base); + expect(sentryVerdictKey('INPUT', { ...IDENTITY, prompt: 'EDITED' })).not.toBe(base); + }); + + it('is boundary-safe (NUL separators β€” shifting bytes across parts cannot collide)', () => { + expect(sentryVerdictKey('INPUT', { ...IDENTITY, model: 'a', prompt: 'bc' })).not.toBe( + sentryVerdictKey('INPUT', { ...IDENTITY, model: 'ab', prompt: 'c' }), + ); + }); + + it('is stable across calls', () => { + expect(sentryVerdictKey('INPUT', IDENTITY)).toBe(sentryVerdictKey('INPUT', IDENTITY)); + }); +}); + +describe('judgeSentryWithCache', () => { + // Non-repo temp cwd β†’ devkitDataFile's degraded per-cwd fallback keeps the store local + disposable. + const dirs: string[] = []; + const tempCwd = () => { + const dir = mkdtempSync(join(tmpdir(), 'sentry-cache-')); + dirs.push(dir); + return dir; + }; + afterEach(() => { + while (dirs.length) rmSync(dirs.pop() as string, { recursive: true, force: true }); + vi.restoreAllMocks(); + }); + + it.each([ + ['MONITOR', 'adoption refusal path uncaptured'], + ['SKIP', ''], + ])('replays an earned %s without re-invoking the judge', (verdict, evidence) => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const cwd = tempCwd(); + const first = vi.fn(() => ({ verdict, evidence })); + expect(judgeSentryWithCache(cwd, 'INPUT', IDENTITY, first)).toEqual({ verdict, evidence }); + expect(first).toHaveBeenCalledTimes(1); + const second = vi.fn(() => ({ verdict: 'SKIP', evidence: 'should not run' })); + expect(judgeSentryWithCache(cwd, 'INPUT', IDENTITY, second)).toEqual({ verdict, evidence }); + expect(second).not.toHaveBeenCalled(); + }); + + it('different inputs miss β€” the fix restage (changed error-hunks) re-judges', () => { + vi.spyOn(console, 'error').mockImplementation(() => {}); + const cwd = tempCwd(); + judgeSentryWithCache(cwd, 'INPUT-A', IDENTITY, () => ({ verdict: 'MONITOR', evidence: 'e' })); + const fresh = vi.fn(() => ({ verdict: 'SKIP', evidence: 'capture added' })); + expect(judgeSentryWithCache(cwd, 'INPUT-B', IDENTITY, fresh)?.verdict).toBe('SKIP'); + expect(fresh).toHaveBeenCalledTimes(1); + }); + + it.each([ + ['outage (null result)', () => null], + ['ambiguous vote (null verdict)', () => ({ verdict: null, evidence: '' })], + ])('never caches %s β€” the next attempt re-judges', (_label, judgeFn) => { + const cwd = tempCwd(); + judgeSentryWithCache(cwd, 'INPUT', IDENTITY, judgeFn); + const retry = vi.fn(() => null); + judgeSentryWithCache(cwd, 'INPUT', IDENTITY, retry); + expect(retry).toHaveBeenCalledTimes(1); + }); + + it('a cache hit names itself on stderr (the gate output stays explainable)', () => { + const errs = vi.spyOn(console, 'error').mockImplementation(() => {}); + const cwd = tempCwd(); + judgeSentryWithCache(cwd, 'INPUT', IDENTITY, () => ({ verdict: 'MONITOR', evidence: 'e' })); + judgeSentryWithCache(cwd, 'INPUT', IDENTITY, () => null); + expect(errs.mock.calls.flat().join('\n')).toContain('cached MONITOR'); + }); +}); diff --git a/gate-engine/sentry/check-sentry.mts b/gate-engine/sentry/check-sentry.mts index c6f8130e..0b158eae 100755 --- a/gate-engine/sentry/check-sentry.mts +++ b/gate-engine/sentry/check-sentry.mts @@ -34,10 +34,9 @@ * The diff directive also self-clears a fix that ELIMINATES the silent path (measured on the * elimination tier: 15/23 -> 19/23 with the clause, no real-slice recall loss under K=3). * - few-shot >> zero-shot, chain-of-thought does not help commit classification (arXiv 2605.02033). - * - self-consistency (sample N, majority-vote) reliably lifts a model; reasoning tiers give no - * advantage and are slow (arXiv 2510.22389) β€” so prefer *_SENTRY_SAMPLES over a reasoning tier. - * The eval/ benchmark sweeps {model, context, shots, samples}; the env defaults below are the cell the - * seed corpus picks (haiku + diff β€” see the CONTEXT_TIER note) β€” re-run the sweep on your own corpus. + * - self-consistency (majority vote) lifts a model; reasoning tiers don't and are slow (arXiv + * 2510.22389) β€” prefer *_SENTRY_SAMPLES. The eval/ benchmark sweeps {model, context, shots, + * samples}; the defaults below are the seed corpus's pick (haiku + diff) β€” re-sweep on your own. * * --gate : exit 0 = SKIP / warn-only / skipped / fail-open Β· exit 1 = hard mode + confident MONITOR Β· exit 2 = could-not-run * (no flag) : report mode β€” judge the given message and PRINT the verdict, exit 0. @@ -61,9 +60,8 @@ * * GOVERNING RULE (devkit "ship the generator, never the data"): every runtime path resolves against * the CONSUMER cwd, never __dirname. The WATCHLIST + the BASELINE stay the consumer's data (born in - * their repo, never shipped). The eval `cases.jsonl` DOES ship β€” 127 cases (104 real-derived + 23 - * authored elimination-tier) β€” but it is a - * dev-only SEED the gate never reads at runtime; a consumer copies + grows it with their own commits. + * their repo, never shipped). The eval `cases.jsonl` DOES ship (127 cases: 104 real-derived + 23 + * authored elimination-tier) but is a dev-only SEED the gate never reads at runtime. */ import { execSync } from 'node:child_process'; @@ -75,6 +73,7 @@ import { focusHunks } from '../judge/diff-focus.mts'; import { JUDGE_ISOLATION, JUDGE_READ_ONLY } from '../judge/judge-isolation.mts'; import { reportGateInfraFailure } from '../judge/odb-probe.mts'; import { execJudge } from '../judge/run-judge.mts'; +import { judgeSentryWithCache } from './verdict-cache.mts'; // Read a GUARD_* env var, falling back to its FRINK_* alias for back-compat with the original frink // gate. Mirrors the config loader's envVar so every devkit gate reads env the same way. @@ -476,9 +475,14 @@ export function run(gate: boolean): void { // Hard-by-default (envBool distinguishes unset β†’ hard from an explicit =0 soften); resolve it // BEFORE judging so the samples default can follow it. Report mode never blocks β†’ warn tier. const hard = gate && effectiveHard(envBool('SENTRY_HARD') ?? true, CONTEXT_TIER, diff); - const result = judge(buildContext(message, nameStatus, diff, CONTEXT_TIER), { - samples: resolveSamples(hard), - }); + const input = buildContext(message, nameStatus, diff, CONTEXT_TIER); + const opts = { model: MODEL, samples: resolveSamples(hard), prompt: SENTRY_JUDGE_PROMPT }; + // Diff-tier only: message/names evidence can't change with the demanded FIX, so a cached hard + // MONITOR would replay forever there. A bypassed run (SENTRY_NO_LLM) earns and replays nothing. + const result = + envVar('SENTRY_NO_LLM') || CONTEXT_TIER !== 'diff' + ? judge(input, opts) + : judgeSentryWithCache(CWD, input, opts, () => judge(input, opts)); if (!gate) { console.log(reportLine(result)); process.exit(0); diff --git a/gate-engine/sentry/verdict-cache.mts b/gate-engine/sentry/verdict-cache.mts new file mode 100644 index 00000000..50688a3c --- /dev/null +++ b/gate-engine/sentry/verdict-cache.mts @@ -0,0 +1,95 @@ +/** + * Verdict cache for the sentry commit-msg judge β€” the one gate that used to re-bill its 3-sample + * haiku vote on EVERY commit attempt, including a byte-identical retry after ITS OWN hard block. + * + * Unlike the decisions cache (decisions/verdict-cache.mts, non-blocking verdicts only), this store + * caches BOTH verdicts by design: SKIP replays the pass, and a confident MONITOR replays the block + * instantly β€” the author is mid-fix-loop and an identical retry cannot flip a majority vote worth + * re-paying for. Anything unearned (outage, ambiguous/tied vote, no-LLM run) is never cached. + * DIFF TIER ONLY (the caller gates this): caching a MONITOR is sound only because adding the + * demanded capture changes the focused-diff evidence and so the key β€” on the message/names tiers + * the fix can't move the evidence and a cached block would replay forever. Escape hatch for a + * wedged entry: `rm .devkit/sentry-verdict-cache.json` (documented in docs/troubleshooting.md). + * + * Key = sha256 over the judge's EXACT inputs plus its identity: devkit version (an upgrade may + * change parsing), model, sample count (a 1-sample warn verdict must never replay as a 3-sample + * hard block, and vice versa), the prompt bytes (edits invalidate even between releases), and the + * full stdin payload (message + focused error-hunk evidence). Any restage that changes the + * error-hunks β€” including adding the demanded capture β€” re-judges. + * + * Storage/atomicity/failure direction: shared judge/verdict-store (`.devkit/sentry-verdict-cache + * .json`, main-checkout anchored, corrupt β†’ re-judge, failed write β†’ verdict stands, unremembered). + */ + +import { createHash } from 'node:crypto'; +import { verdictKey } from '../decisions/verdict-cache.mts'; +import { emitCacheHit, emitGateEvent } from '../judge/gate-events.mts'; +import { devkitDataFile, loadEntries, saveEntries } from '../judge/verdict-store.mts'; + +const STORE_FILE = 'sentry-verdict-cache.json'; +const CACHEABLE = new Set(['MONITOR', 'SKIP']); + +/** The judge result shape shared with check-sentry (kept structural to avoid a cyclic import). */ +export interface CachedSentryVerdict { + verdict: string | null; + evidence: string; +} + +/** Identity inputs the key must cover beyond the stdin payload itself. */ +export interface SentryJudgeIdentity { + model: string; + samples: number; + prompt: string; +} + +/** Stable cache key over the judge's exact inputs + identity. Built on the decisions cache's + * `verdictKey` (NUL-separated parts + devkit-version salt) so the two stores' key formulas cannot + * drift; the prompt rides as its own sha256 to keep the key line-length sane. */ +export function sentryVerdictKey(input: string, { model, samples, prompt }: SentryJudgeIdentity) { + const promptDigest = createHash('sha256').update(prompt).digest('hex'); + return verdictKey('sentry', model, samples, promptDigest, input); +} + +/** + * Judge `input` through the cache: an earned verdict for these exact inputs replays without a + * judge call; a miss runs `judgeFn` and remembers a confident MONITOR/SKIP (best-effort β€” a failed + * write leaves the verdict standing for this run). Callers on a bypass path (NO_SENTRY_JUDGE, + * SENTRY_NO_LLM) must not reach this at all: a bypassed run earns nothing and must not replay a + * cached block the owner explicitly softened. + */ +export function judgeSentryWithCache( + cwd: string, + input: string, + identity: SentryJudgeIdentity, + judgeFn: () => CachedSentryVerdict | null, +): CachedSentryVerdict | null { + const file = devkitDataFile(cwd, STORE_FILE); + const key = sentryVerdictKey(input, identity); + const hit = loadEntries(file)[key]; + if (hit && typeof hit.verdict === 'string' && CACHEABLE.has(hit.verdict)) { + console.error(`sentry-judge: cached ${hit.verdict} (identical message + error-hunk evidence)`); + emitCacheHit('sentry-advisory', hit.model, hit.duration_ms); + return { verdict: hit.verdict, evidence: String(hit.evidence ?? '') }; + } + const started = Date.now(); + const result = judgeFn(); + if (result?.verdict && CACHEABLE.has(result.verdict)) { + const saved = saveEntries(file, { + [key]: { + at: new Date().toISOString(), + verdict: result.verdict, + evidence: result.evidence, + model: identity.model, + samples: identity.samples, + duration_ms: Math.max(0, Date.now() - started), + }, + }); + if (!saved) { + // The verdict stands for this run; it just isn't remembered. Name it (gate-telemetry-self- + // describing): a silently lost write would read as a judge that keeps re-billing for no reason. + console.error('sentry-judge: verdict earned but NOT cached (store write failed)'); + emitGateEvent({ type: 'cache_write_failed', judge: 'sentry-advisory' }); + } + } + return result; +}