feat(review): commit message as advisory reviewer context (sc-1442) - #334
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe change passes commit intent through temporary files to review gates. The gate engine loads and sanitizes the message, adds it to reviewer prompts as advisory context, separates semantic Target data from cache salts, and preserves scope results when semantic lookup fails. ChangesCommit intent review flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ShipFlow
participant ReviewContext
participant TargetLoader
participant ReviewerPrompts
participant GateScope
ShipFlow->>ReviewContext: provide DEVKIT_COMMIT_MSG_FILE
ReviewContext->>TargetLoader: load message subject and Target blocks
TargetLoader-->>ReviewContext: return salt block and prompt block
ReviewContext->>ReviewerPrompts: provide commit message and Target prompt extras
ReviewerPrompts-->>GateScope: run review with advisory context
GateScope-->>ReviewContext: emit commit_msg and targets_via fields
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 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 |
The domain reviewers run at pre-commit, where no commit message exists — so they judged diffs blind to the change's stated intent, the one context advantage that let the completeness gate catch a real deny-floor bug they PASSed. Now devkit ship writes its composed message to a temp file and exports DEVKIT_COMMIT_MSG_FILE before git commit; devkit review synthesizes the reviewed range's subjects into the same channel; a bare git commit degrades to a placeholder. The message renders as a FENCED, neutralized advisory block in every judge prompt (forged headers and VERDICT/OFFENDING tokens are prefix-defused — untrusted author text lands on a haiku pass whose PASS never escalates), and its subject activates semantic Target retrieval. The message NEVER enters a reviewer cache key (ship-gates-converge-not-restart): the Targets block renders twice from one scopedTargets call — the salt from the scope-match subset only, the prompt from the enriched result — and the partition is structural, not incidental: scoped-targets catches semantic-tier errors around the supplement alone (also fixing completeness dropping ALL Targets on a semantic throw), and a 10s bound falls back to the scope-only render, which IS the salt render. guard-review scan now composes the same salt as the gate instead of reporting stale '[cached PASS]'. review_scope rows gain commit_msg/targets_via for the epic's field receipt, and the bench gate hashes now cover targets-block.mts, commit-message.mts AND staged-git.mts (comparability gaps; the last was the correctness judge's own in-gate finding on this very commit). gitCached/stagedFiles moved to evidence/staged-git.mts to keep run-review under its ratchet (569 → 552). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
norvalbv
force-pushed
the
feat/sc-1442-commit-msg-context
branch
from
August 4, 2026 15:06
92f57e7 to
b92b9b2
Compare
![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: 4
🤖 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 `@cli/lib/ship/commit-with-gate-capture.sh`:
- Around line 100-109: In the commit-message setup flow before the mktemp call,
clear any inherited DEVKIT_COMMIT_MSG_FILE so failures cannot preserve stale
state. Keep the existing temporary-file and cleanup behavior, and add a test
covering an injected DEVKIT_COMMIT_MSG_FILE together with forced temporary-file
creation failure, verifying the placeholder path is used.
In `@gate-engine/review/evidence/commit-message.mts`:
- Around line 43-49: Update loadCommitMessage and the corresponding paths at the
referenced call sites to validate that DEVKIT_COMMIT_MSG_FILE resolves to a
regular file before opening it, then read at most the 2 KB limit and derive both
subject and text from that bounded content. Avoid unbounded readFileSync calls
and preserve the existing null behavior for missing, empty, or COMMIT_EDITMSG
inputs.
- Line 39: Update INSTRUCTION_SHAPED_LINE to normalize Markdown structure before
matching verdict-related tokens, covering block quotes, list markers,
indentation, and fence-marker lines such as > ## RECORDED TARGETS and - ##
RECORDED TARGETS. Preserve detection of existing heading and token-prefixed
forms, and add coverage for quoted, listed, and fenced instruction-shaped lines.
In `@gate-engine/review/evidence/targets-block.mts`:
- Around line 112-126: Update loadReviewerTargetsBlocks to create an
AbortController for the semantic query, pass its signal through scopedTargets
and rankAxes, and abort it when the SEMANTIC_BUDGET_MS timer wins. Ensure
semantic embedding/retrieval observes the signal and terminates promptly before
the scope-only fallback runs. Add a cancellation test using an active operation
with retained handles, not only a permanently pending promise.
🪄 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: 5b50c235-b66f-46e4-a721-67110b8db80e
⛔ Files ignored due to path filters (11)
dist/cli/commands/review.mjsis excluded by!**/dist/**dist/cli/lib/ship/commit-with-gate-capture.shis excluded by!**/dist/**dist/cli/lib/ship/review-target.shis excluded by!**/dist/**dist/gate-engine/decisions/scoped-targets.mjsis excluded by!**/dist/**dist/gate-engine/review/cli.mjsis excluded by!**/dist/**dist/gate-engine/review/evidence/commit-message.mjsis excluded by!**/dist/**dist/gate-engine/review/evidence/scope.mjsis excluded by!**/dist/**dist/gate-engine/review/evidence/staged-git.mjsis excluded by!**/dist/**dist/gate-engine/review/evidence/targets-block.mjsis excluded by!**/dist/**dist/gate-engine/review/reviewers.mjsis excluded by!**/dist/**dist/gate-engine/review/run-review.mjsis excluded by!**/dist/**
📒 Files selected for processing (21)
cli/__tests__/review.test.mtscli/__tests__/ship-branch.test.mtscli/commands/review.mtscli/lib/ship/commit-with-gate-capture.shcli/lib/ship/review-target.shdocs/decisions/review-gate-in-chain.mdeslint/baselines/size-lines.jsongate-engine/decisions/__tests__/scoped-targets.test.mtsgate-engine/decisions/scoped-targets.mtsgate-engine/review/__tests__/commit-message.test.mtsgate-engine/review/__tests__/reviewers.test.mtsgate-engine/review/__tests__/run-review.test.mtsgate-engine/review/cli.mtsgate-engine/review/eval/conventions/bench.mtsgate-engine/review/eval/reviewers/corpus.mtsgate-engine/review/evidence/commit-message.mtsgate-engine/review/evidence/scope.mtsgate-engine/review/evidence/staged-git.mtsgate-engine/review/evidence/targets-block.mtsgate-engine/review/reviewers.mtsgate-engine/review/run-review.mts
Comment on lines
+100
to
+109
| local msgf="" | ||
| msgf=$(mktemp "${TMPDIR:-/tmp}/devkit-ship-msg.XXXXXX" 2>/dev/null) || msgf="" | ||
| if [ -n "$msgf" ]; then | ||
| if printf '%s\n\n%s\n' "$title" "$body" > "$msgf" 2>/dev/null; then | ||
| export DEVKIT_COMMIT_MSG_FILE="$msgf" | ||
| else | ||
| rm -f -- "$msgf" 2>/dev/null || true | ||
| msgf="" | ||
| fi | ||
| fi |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Clear inherited commit-message state before creating the temporary file.
If mktemp or printf fails, the function leaves an inherited DEVKIT_COMMIT_MSG_FILE unchanged. The gates can then read stale intent instead of using the placeholder.
Unset the variable before mktemp. Add a test that combines an injected path with a forced temporary-file failure.
Proposed fix
local msgf=""
+ unset DEVKIT_COMMIT_MSG_FILE
msgf=$(mktemp "${TMPDIR:-/tmp}/devkit-ship-msg.XXXXXX" 2>/dev/null) || msgf=""📝 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
| local msgf="" | |
| msgf=$(mktemp "${TMPDIR:-/tmp}/devkit-ship-msg.XXXXXX" 2>/dev/null) || msgf="" | |
| if [ -n "$msgf" ]; then | |
| if printf '%s\n\n%s\n' "$title" "$body" > "$msgf" 2>/dev/null; then | |
| export DEVKIT_COMMIT_MSG_FILE="$msgf" | |
| else | |
| rm -f -- "$msgf" 2>/dev/null || true | |
| msgf="" | |
| fi | |
| fi | |
| local msgf="" | |
| unset DEVKIT_COMMIT_MSG_FILE | |
| msgf=$(mktemp "${TMPDIR:-/tmp}/devkit-ship-msg.XXXXXX" 2>/dev/null) || msgf="" | |
| if [ -n "$msgf" ]; then | |
| if printf '%s\n\n%s\n' "$title" "$body" > "$msgf" 2>/dev/null; then | |
| export DEVKIT_COMMIT_MSG_FILE="$msgf" | |
| else | |
| rm -f -- "$msgf" 2>/dev/null || true | |
| msgf="" | |
| fi | |
| fi |
🤖 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 `@cli/lib/ship/commit-with-gate-capture.sh` around lines 100 - 109, In the
commit-message setup flow before the mktemp call, clear any inherited
DEVKIT_COMMIT_MSG_FILE so failures cannot preserve stale state. Keep the
existing temporary-file and cleanup behavior, and add a test covering an
injected DEVKIT_COMMIT_MSG_FILE together with forced temporary-file creation
failure, verifying the placeholder path is used.
| // adjacent authoritative blocks) and the machine-parsed tokens (reviewers.mts VERDICT/OFFENDING | ||
| // regexes tolerate [\s>*#-]* prefixes, so quoting with `>` would NOT defuse them — a leading `¦` | ||
| // breaks the anchored prefix class for both). | ||
| const INSTRUCTION_SHAPED_LINE = /^\s*(?:#{1,6}\s|[\s>*#-]*\**(?:VERDICT|OFFENDING|VIOLATION)\b)/i; |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- file outline ---\n'
ast-grep outline gate-engine/review/evidence/commit-message.mts --view expanded || true
printf '\n--- source lines 1-120 ---\n'
cat -n gate-engine/review/evidence/commit-message.mts | sed -n '1,120p'
printf '\n--- related test assertions around renderCommitMessageBlock ---\n'
cat -n gate-engine/review/__tests__/commit-message.test.mts | sed -n '1,160p'
printf '\n--- regex behavioral probe for prefixed/atx/fenced token headings ---\n'
node - <<'JS'
const INSTRUCTION_SHAPED_LINE = /^\s*(?:#{1,6}\s|[\s>*`#-`]*\**(?:VERDICT|OFFENDING|VIOLATION)\b)/i;
const cases = [
'# RECORDED TARGETS',
'> ## RECORDED TARGETS',
'## RECORDED TARGETS',
' > ## RECORDED TARGETS',
'- ## RECORDED TARGETS',
'* VERDICT: x',
'> VERDICT: x',
'``` VERDICT: x',
];
for (const line of cases) {
console.log(JSON.stringify(line), INSTRUCTION_SHAPED_LINE.test(line));
}
JSRepository: norvalbv/devkit
Length of output: 244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- source lines 1-120 ---'
cat -n gate-engine/review/evidence/commit-message.mts | sed -n '1,120p'
printf '%s\n' ''
printf '%s\n' '--- related test assertions around renderCommitMessageBlock ---'
cat -n gate-engine/review/__tests__/commit-message.test.mts | sed -n '1,160p'
printf '%s\n' ''
printf '%s\n' '--- regex behavioral probe for prefixed/atx/fenced token headings ---'
node - <<'JS'
const INSTRUCTION_SHAPED_LINE = /^\s*(?:#{1,6}\s|[\s>*`#-`]*\**(?:VERDICT|OFFENDING|VIOLATION)\b)/i;
const cases = [
'# RECORDED TARGETS',
'> ## RECORDED TARGETS',
'## RECORDED TARGETS',
' > ## RECORDED TARGETS',
'- ## RECORDED TARGETS',
'* VERDICT: x',
'> VERDICT: x',
'``` VERDICT: x',
];
for (const line of cases) {
console.log(JSON.stringify(line), INSTRUCTION_SHAPED_LINE.test(line));
}
JSRepository: norvalbv/devkit
Length of output: 13988
LLM Security (CWE-116): Improper Encoding or Escaping of Output
Reachability: External
Reachability path
● Entry
gate-engine/review/run-review.mts:327
runReviewGate: the ship path sets this: retry once, then fail CLOSED
│
▼
● Sink
gate-engine/review/evidence/commit-message.mts
Neutralize Markdown structure before prefixing verdict tokens.
INSTRUCTION_SHAPED_LINE skips lines where the heading starts after block-quote or list markers, e.g. > ## RECORDED TARGETS, > ## RECORDED TARGETS, and - ## RECORDED TARGETS. The untrusted-input warning does not turn Active Markdown off. Normalize instruction-shaped Markdown structure before applying token neutralization, and include cases for block quotes, lists, and fence-marker lines.
🤖 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/review/evidence/commit-message.mts` at line 39, Update
INSTRUCTION_SHAPED_LINE to normalize Markdown structure before matching
verdict-related tokens, covering block quotes, list markers, indentation, and
fence-marker lines such as > ## RECORDED TARGETS and - ## RECORDED TARGETS.
Preserve detection of existing heading and token-prefixed forms, and add
coverage for quoted, listed, and fenced instruction-shaped lines.
Comment on lines
+43
to
+49
| export function loadCommitMessage(env: NodeJS.ProcessEnv = process.env): CommitMessage | null { | ||
| const file = env.DEVKIT_COMMIT_MSG_FILE; | ||
| if (!file || basename(file) === 'COMMIT_EDITMSG') return null; | ||
| try { | ||
| const text = readFileSync(file, 'utf8').trim(); | ||
| if (!text) return null; | ||
| return { subject: (text.split('\n')[0] ?? '').trim(), text }; |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Apply the byte limit before reading and querying.
readFileSync loads the complete file before the 2 KB rendering cap applies. A large intent file can consume excessive memory. A FIFO or device path can block the gate indefinitely.
The uncapped first line also becomes the semantic query. Open only regular files and read a bounded number of bytes before deriving subject and text.
Also applies to: 59-62, 92-93
🤖 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/review/evidence/commit-message.mts` around lines 43 - 49, Update
loadCommitMessage and the corresponding paths at the referenced call sites to
validate that DEVKIT_COMMIT_MSG_FILE resolves to a regular file before opening
it, then read at most the 2 KB limit and derive both subject and text from that
bounded content. Avoid unbounded readFileSync calls and preserve the existing
null behavior for missing, empty, or COMMIT_EDITMSG inputs.
Comment on lines
+112
to
+126
| export async function loadReviewerTargetsBlocks( | ||
| cwd: string, | ||
| files: string[], | ||
| query = '', | ||
| ): Promise<string> { | ||
| const targets = await scopedTargets(files, query, 6, cwd).catch(() => []); | ||
| return renderTargets(targets, REVIEWER_TARGETS_FRAMING, 8_192); | ||
| ): Promise<ReviewerTargetsBlocks> { | ||
| let targets: GoverningTarget[] | null = null; | ||
| if (query.trim()) { | ||
| targets = await Promise.race([ | ||
| scopedTargets(files, query, 6, cwd).catch((): null => null), | ||
| new Promise<null>((resolve) => setTimeout(() => resolve(null), SEMANTIC_BUDGET_MS).unref()), | ||
| ]); | ||
| } | ||
| // No query, supplement timed out, or the load itself threw → the scope-only load (cheap, sync | ||
| // file reads; scopedTargets already survives semantic-tier errors internally). | ||
| if (targets === null) targets = await scopedTargets(files, '', 6, cwd).catch(() => []); |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Cancel semantic retrieval when the budget expires.
Promise.race stops awaiting scopedTargets, but it does not cancel the losing operation. Active embedding requests can continue after scope-only fallback. They can retain event-loop resources and consume the semantic service after the 10-second budget.
Pass an AbortSignal through scopedTargets and rankAxes. Abort the semantic operation when the timer expires. Test cancellation with an active operation, not a permanently pending promise without handles.
🤖 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/review/evidence/targets-block.mts` around lines 112 - 126, Update
loadReviewerTargetsBlocks to create an AbortController for the semantic query,
pass its signal through scopedTargets and rankAxes, and abort it when the
SEMANTIC_BUDGET_MS timer wins. Ensure semantic embedding/retrieval observes the
signal and terminates promptly before the scope-only fallback runs. Add a
cancellation test using an active operation with retained handles, not only a
permanently pending promise.
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.
What
sc-1442 (epic 1435): the domain reviewers judged diffs blind to the change's stated intent — the one context advantage that let the completeness gate catch the frink deny-floor bug that api-security and correctness PASSed. This PR hands them the commit message as advisory context, everywhere a message (or a synthesizable intent) exists:
devkit ship(commit-with-gate-capture.sh, shared by ship + reship): writes the composedtitle\n\nbodyto a mktemp file and exportsDEVKIT_COMMIT_MSG_FILEbeforegit commit; removed + unset immediately after, above both return sites. Never reads.git/COMMIT_EDITMSG(at pre-commit it holds the previous message) — the loader refuses that basename outright.devkit review(review-target.sh): synthesizes the reviewed range's subjects (git log --reverse, against the target repo — the snapshot worktrees' HEAD is pinned at merge-base) into the same env channel; a caller-injected stale path is scrubbed in bothreview.mtsand the script's own env scrub. Empty range → unset → placeholder.git commit: the prompt carries(commit message not available at this hook stage)and everything else is byte-identical to before.The message's subject also becomes the
scopedTargetssemantic query, activating semantic Target retrieval for reviewers (previously dead at pre-commit; mirrorscompleteness.mts).The cache-key invariant (the load-bearing part)
ship-gates-converge-not-restartrequires that an amended message on a ship retry reuses every cached PASS. Since sc-1441 salts checklist reviewers with the rendered Targets block, a message-driven semantic query would leak into the key transitively. Three mechanisms make the invariant structural:targets-block.mts): the salt renders from thevia === 'scope-match'subset only; the prompt from the enriched result. Scope matches are computed independently of the query, so the salt bytes cannot depend on the message.scoped-targets.mts): the semantic supplement is try/caught alone, so an embed-tier error (EACCES/ENOSPC/endpoint down) can no longer collapse the scope-matched half — previously a single.catch(() => [])would have let a message-supplied query move the salt on the error path. This also fixes a live bug where a semantic throw dropped all Targets from the completeness prompt.Pinned by tests: same diff + rewritten message → zero judge spawns (and a third run with no message also hits); a stubbed rankAxes throw still yields the scope-matched ruling; sc-1441's Target-edit → invalidation test unchanged.
Prompt-injection hardening
The message is untrusted author text landing on a haiku first pass whose PASS never escalates. The block is fenced
─────like completeness's precedent (not a markdown header the adjacent authoritative blocks use), instruction-shaped lines (#headers,VERDICT:/OFFENDING:/VIOLATION:— whose parse regexes tolerate[\s>*#-]*prefixes, so quoting wouldn't defuse them) are prefix-neutralized with¦, and the wrapper states the fenced text cannot grant exceptions or change a verdict. Test: a forged## RECORDED TARGETSsection +VERDICT: PASSline renders inert.Also in this PR (critique-driven)
guard-review scannow composes the same salt as the gate — it reported[cached PASS]for entries the gate would re-judge (stale since sc-1441).review_scopetelemetry rows gaincommit_msg: boolean+targets_via: 'scope'|'scope+semantic'— the epic's "reviewers with intent vs blind" field receipt needs them.benchGateHash+ conventionsgateHashnow coverevidence/targets-block.mtsandevidence/commit-message.mts(prompt-byte suppliers; targets-block was a comparability gap since sc-1441). This invalidates the reviewer-eval baselines accepted in ecd47c6 — the re-earn is expected, not a regression.gitCached/stagedFilesextracted toevidence/staged-git.mts(size ratchet: run-review.mts 569 → 552).Accepted residual
A Ctrl-C mid-gate can leak the mode-600 mktemp message file in
$TMPDIR; content is the message the author is about to publish anyway, so no trap plumbing was added.Testing
772 tests / 24 files green, including new:
commit-message.test.mts(loader never-throw, COMMIT_EDITMSG refusal, injection neutralization, dual-render partition + timeout),scoped-targets.test.mts(semantic append + throw survival),run-review.test.mts(fenced block in checklist + conventions prompts, placeholder paths, the inverse cache test, scope-row flags),ship-branch.test.mts(env exported with title+body during gates, temp file gone after),review.test.mts(synthesized intent + stale-path scrub, empty-range degrade).🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation