feat(review): sentry-additive restages keep earned verdicts, sentry judge gets a cache - #370
Merged
Merged
Conversation
…udge gets a cache 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)
Contributor
📝 WalkthroughWalkthroughThe PR adds normalized diff identities for review caches, introduces persisted Sentry verdict caching, and expands the review cache-session protocol to support the new cache store. Tests, isolation setup, ignore rules, and documentation cover the new behavior. ChangesCache convergence
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ReviewGate
participant SentryVerdictCache
participant SentryJudge
participant ReviewDataStore
ReviewGate->>SentryVerdictCache: send diff input and judge options
SentryVerdictCache->>ReviewDataStore: read verdict key
alt cache hit
ReviewDataStore-->>SentryVerdictCache: return cached MONITOR or SKIP
SentryVerdictCache-->>ReviewGate: return cached verdict
else cache miss
SentryVerdictCache->>SentryJudge: evaluate diff
SentryJudge-->>SentryVerdictCache: return verdict
SentryVerdictCache->>ReviewDataStore: write eligible verdict
SentryVerdictCache-->>ReviewGate: return current verdict
end
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
gate-engine/judge/__tests__/diff-focus.test.mts (1)
205-331: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract one shared git harness.
gitDiffOf,BASE, andeditare declared three times in this file, with only the temp-directory prefix and the file name differing. Extract a single module-level helper that accepts the file name. This keeps the three suites in sync and applies any harness fix, such as git-config isolation, in one place.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@gate-engine/judge/__tests__/diff-focus.test.mts` around lines 205 - 331, Extract the duplicated git setup into one module-level gitDiffOf helper that accepts the file name and preserves the existing base/staged inputs, while allowing each suite’s temp-directory prefix to remain distinct as needed. Replace the local gitDiffOf declarations and reuse the shared helper across all three suites, keeping BASE/edit shared where their behavior is identical and centralizing git configuration and cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/glossary.md`:
- Around line 56-59: Update the cache invalidation wording at
docs/glossary.md:56-59 and docs/troubleshooting.md:73-80 to state that restaging
re-judges only when it changes the commit message or focused error-hunk
evidence, rather than implying every staged-diff restage does so. Preserve the
surrounding cache-reset and replay behavior descriptions.
In `@gate-engine/judge/__tests__/diff-focus.test.mts`:
- Around line 80-92: Update the git harnesses, including gitDiffOf and the
harnesses near the other referenced sections, so every execSync call runs with
GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM set to empty paths. Preserve the
existing command behavior and diff.noprefix setting while ensuring all git
operations are isolated from ambient global and system configuration.
---
Nitpick comments:
In `@gate-engine/judge/__tests__/diff-focus.test.mts`:
- Around line 205-331: Extract the duplicated git setup into one module-level
gitDiffOf helper that accepts the file name and preserves the existing
base/staged inputs, while allowing each suite’s temp-directory prefix to remain
distinct as needed. Replace the local gitDiffOf declarations and reuse the
shared helper across all three suites, keeping BASE/edit shared where their
behavior is identical and centralizing git configuration and cleanup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9d0c1c52-1895-43ea-a45e-9bb06f967309
⛔ Files ignored due to path filters (8)
dist/cli/lib/install/gitignore-cache.mjsis excluded by!**/dist/**dist/cli/lib/ship/review-target.shis excluded by!**/dist/**dist/cli/lib/ship/review/cache/session.mjsis excluded by!**/dist/**dist/gate-engine/judge/diff-focus.mjsis excluded by!**/dist/**dist/gate-engine/review/lens/split.mjsis excluded by!**/dist/**dist/gate-engine/review/overrides.mjsis excluded by!**/dist/**dist/gate-engine/sentry/check-sentry.mjsis excluded by!**/dist/**dist/gate-engine/sentry/verdict-cache.mjsis excluded by!**/dist/**
📒 Files selected for processing (19)
.gitignorecli/__tests__/review-cache-session.test.mtscli/lib/install/gitignore-cache.mtscli/lib/ship/review-target.shcli/lib/ship/review/cache/session.mtsdocs/decisions/ship-gates-converge-not-restart.mddocs/glossary.mddocs/troubleshooting.mdgate-engine/judge/__tests__/diff-focus.test.mtsgate-engine/judge/diff-focus.mtsgate-engine/review/__tests__/lens-split.test.mtsgate-engine/review/__tests__/overrides.test.mtsgate-engine/review/lens/split.mtsgate-engine/review/overrides.mtsgate-engine/sentry/__tests__/check-sentry.test.mtsgate-engine/sentry/__tests__/sentry-hard-defaults.test.mtsgate-engine/sentry/__tests__/verdict-cache.test.mtsgate-engine/sentry/check-sentry.mtsgate-engine/sentry/verdict-cache.mts
Comment on lines
+56
to
+59
| 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. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Describe invalidation by judge input.
The cache key covers the generated judge input, not every staged-diff byte. A restage outside the focused error-hunk evidence can replay a cached verdict. State that a restage re-judges when it changes the commit message or focused error-hunk evidence.
docs/glossary.md#L56-L59: Replace “any restage re-judges” with the evidence-scoped condition.docs/troubleshooting.md#L73-L80: Replace “any restage of the staged diff re-judges” with the evidence-scoped condition.
📍 Affects 2 files
docs/glossary.md#L56-L59(this comment)docs/troubleshooting.md#L73-L80
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/glossary.md` around lines 56 - 59, Update the cache invalidation wording
at docs/glossary.md:56-59 and docs/troubleshooting.md:73-80 to state that
restaging re-judges only when it changes the commit message or focused
error-hunk evidence, rather than implying every staged-diff restage does so.
Preserve the surrounding cache-reset and replay behavior descriptions.
Comment on lines
+80
to
+92
| 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 }); | ||
| } | ||
| }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Isolate the harness from ambient git configuration.
The harness pins only diff.noprefix. It still inherits the global and system gitconfig. Settings such as diff.algorithm, diff.context, core.autocrlf, or diff.external change the emitted hunk geometry. The assertions in this suite depend on exact hunk shaping, so such a setting can make the suite fail on a developer machine or a CI image.
Set GIT_CONFIG_GLOBAL and GIT_CONFIG_SYSTEM to an empty path for every execSync call in the harness. The harnesses at lines 208-220 and 275-287 need the same change.
🧪 Proposed isolation for the harness
const gitDiffOf = (base: string, staged: string): string => {
const dir = mkdtempSync(join(tmpdir(), 'dci-'));
+ const env = { ...process.env, GIT_CONFIG_GLOBAL: '/dev/null', GIT_CONFIG_SYSTEM: '/dev/null' };
try {
- execSync('git init -q && git config user.email t@t && git config user.name t', { cwd: dir });
+ execSync('git init -q && git config user.email t@t && git config user.name t', { cwd: dir, env });
writeFileSync(join(dir, 'app.ts'), base);
- execSync('git add . && git commit -qm base', { cwd: dir });
+ execSync('git add . && git commit -qm base', { cwd: dir, env });
writeFileSync(join(dir, 'app.ts'), staged);
- execSync('git add .', { cwd: dir });
- return execSync('git -c diff.noprefix=false diff --cached', { cwd: dir, encoding: 'utf8' });
+ execSync('git add .', { cwd: dir, env });
+ return execSync('git -c diff.noprefix=false diff --cached', { cwd: dir, encoding: 'utf8', env });
} finally {
rmSync(dir, { recursive: true, force: true });
}
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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 gitDiffOf = (base: string, staged: string): string => { | |
| const dir = mkdtempSync(join(tmpdir(), 'dci-')); | |
| const env = { ...process.env, GIT_CONFIG_GLOBAL: '/dev/null', GIT_CONFIG_SYSTEM: '/dev/null' }; | |
| try { | |
| execSync('git init -q && git config user.email t@t && git config user.name t', { cwd: dir, env }); | |
| writeFileSync(join(dir, 'app.ts'), base); | |
| execSync('git add . && git commit -qm base', { cwd: dir, env }); | |
| writeFileSync(join(dir, 'app.ts'), staged); | |
| execSync('git add .', { cwd: dir, env }); | |
| return execSync('git -c diff.noprefix=false diff --cached', { cwd: dir, encoding: 'utf8', env }); | |
| } finally { | |
| rmSync(dir, { recursive: true, force: true }); | |
| } | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@gate-engine/judge/__tests__/diff-focus.test.mts` around lines 80 - 92, Update
the git harnesses, including gitDiffOf and the harnesses near the other
referenced sections, so every execSync call runs with GIT_CONFIG_GLOBAL and
GIT_CONFIG_SYSTEM set to empty paths. Preserve the existing command behavior and
diff.noprefix setting while ensuring all git operations are isolated from
ambient global and system configuration.
This was referenced Aug 9, 2026
norvalbv
added a commit
that referenced
this pull request
Aug 9, 2026
## Summary - read the raw-line baseline from the exact ship base before creating a worktree - print current lines, effective ceiling, headroom, and working-tree/base drift - share authoritative size policy and mirror gate semantics for shipped baselines, skipped directories, and disabled caps - distinguish proven violations from unavailable preview reads, which defer to the authoritative gate - run the preview from the repository root for both new ships and PR reships - document the standalone command and add focused unit plus ship integration regressions - repair the pre-existing PR #370 test fixture encoding that made main fail Biome, without changing its value ## Root cause The authoritative guard runs in the ship worktree, where the baseline comes from the selected base. The shared checkout could expose a different size-lines baseline, so contributors could not see the real cap until after the expensive gate chain started. ## Validation - clean detached origin/main build: bun run build - Story #1494 focused regressions: 9 passed - diff-focus prerequisite regression: 30 passed - base-aware preflight against origin/main: all changed source/test files within their effective ceilings - duplication guard: PASS - GitNexus scoped Story #1494 change audit: LOW risk, zero affected processes Story: sc-1494 Autonomous report: 20dc0667-a65d-4cf8-9284-09080bc91ff7
norvalbv
added a commit
that referenced
this pull request
Aug 9, 2026
## Summary - construct the template-literal fixture from string fragments - preserve the exact tested fixture bytes - restore a clean full-repository Biome gate on origin/main ## Root cause PR #370 added a deliberately literal `${sideEffect()}` fixture inside a single-quoted string. Biome correctly flags that representation as noTemplateCurlyInString, and PR #371 made warnings fatal, so every unrelated devkit ship became blocked. ## Validation - focused diff-focus Vitest: 30 passed - Biome check for the changed file: clean - matcher preflight: 0 new candidates - commit guard: PASS - GitNexus: exercised review-cache path is high reach, but production code and fixture value are unchanged Tracks autonomous report 741fde8b-e5ef-41d5-aaa6-df158dd673ce.
norvalbv
added a commit
that referenced
this pull request
Aug 9, 2026
* fix(review): make template fixture Biome-safe ## Summary - construct the template-literal fixture from string fragments - preserve the exact tested fixture bytes - restore a clean full-repository Biome gate on origin/main ## Root cause PR #370 added a deliberately literal `${sideEffect()}` fixture inside a single-quoted string. Biome correctly flags that representation as noTemplateCurlyInString, and PR #371 made warnings fatal, so every unrelated devkit ship became blocked. ## Validation - focused diff-focus Vitest: 30 passed - Biome check for the changed file: clean - matcher preflight: 0 new candidates - commit guard: PASS - GitNexus: exercised review-cache path is high reach, but production code and fixture value are unchanged Tracks autonomous report 741fde8b-e5ef-41d5-aaa6-df158dd673ce. * fix(review): resolve checklist assets across providers (sc-1397) ## Summary - resolve checklist scripts from the provider-projected root - keep prompts, Bash allowlists, retries, and identities on the same root - make the correctness skill and reviewer brief portable across Codex, Claude, and Cursor projections - extract cascade and asset resolution helpers to respect the size ratchet - add regression coverage for provider-specific checklist resolution ## Root cause The correctness review runtime and generated assets assumed a .claude projection. Consumers with the valid .agents projection could therefore invoke a nonexistent checklist path and fail with MODULE_NOT_FOUND. ## Validation - reproduced the failure shape in Frink and verified portable resolution selects .agents/skills/correctness/scripts/checklist.mjs - review orchestration: 123 tests passed - provider and asset suites: 106 tests passed - full review cluster: 663 tests passed before the behavior-preserving extraction - correctness review: all four lenses passed - Biome, ESLint, git diff check, and size gate passed - matcher preflight: 0 candidates; commit-guard passed ## Shared checkout note The shared checkout TypeScript run is blocked only by unrelated concurrent duplicate imports in setup-manifest.mts and setup-runtime.mts. The isolated ship excludes those changes. ## Dependency Stacked on #374, which fixes the pre-existing origin/main Biome warning that otherwise blocks every devkit ship.
Merged
norvalbv
added a commit
that referenced
this pull request
Aug 10, 2026
Bump 0.50.0 -> 0.51.0 and rebuild dist from a clean origin/main worktree (f817b75). Minor, not patch: two feature PRs landed since 0.50.0 — sentry-additive restages that keep earned verdicts plus a sentry judge cache (#370), and the opt-in priorArtGate deny-once step-0 ordering component (#382). The rest of the range is fixes (#371-#383). The rebuild also picks up two dist modules that git had never seen. dist/ is gitignored on working branches by design, so gate-engine/review/cascade/ consumer-assets.mjs and reviewer.mjs — the compiled output of the #376 checklist-asset refactor — existed only on disk. They are force-added here, which is exactly the path `devkit release` takes for ignored dist output. Without them the shipped runtime.mjs would import a module absent from the tag. Release smoke checks ran: built bin reports 0.51.0, dist/package.json agrees, and dist/cli/lib/ship/ship-branch.sh still wires ship_read_stdin_body (the sc-1419 stdin-hang fix survived the build). Gates bypassed at the maintainer's request (--no-verify); the range was already reviewed on its constituent PRs. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two amendments so a sentry-gate block no longer re-bills the whole gate fleet:
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
Summary by CodeRabbit