perf(hooks): batch format+typecheck at Stop instead of per Edit#746
Merged
affaan-m merged 8 commits intoMar 31, 2026
Merged
Conversation
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces per-Edit JS/TS format/typecheck hooks with a batched flow: edits are appended to a session-scoped accumulator during Edit/Write/MultiEdit, and a Stop hook reads/clears the accumulator to run project-scoped formatting and TypeScript checks once for all accumulated files. Changes
Sequence DiagramsequenceDiagram
participant Editor as Editor
participant AccHook as post-edit-accumulate Hook
participant AccFile as Accumulator File (OS Temp)
participant StopHook as stop-format-typecheck Hook
participant Formatter as Formatter (Biome/Prettier)
participant TSC as TypeScript Compiler (tsc)
Editor->>AccHook: PostToolUse (Edit/Write/MultiEdit with file_path or edits[])
AccHook->>AccFile: Append edited JS/TS paths
Note over Editor,AccFile: Multiple edits append paths (duplicates allowed)
Editor->>StopHook: Stop event triggers
StopHook->>AccFile: Read accumulated paths
StopHook->>AccFile: Delete accumulator file
StopHook->>Formatter: Group by project root → run formatter per root
Formatter->>StopHook: Return formatting result
StopHook->>TSC: Group .ts/.tsx by tsconfig → run npx tsc --noEmit per tsconfig
TSC->>StopHook: Return typecheck result
StopHook->>Editor: Return original input (logs/errors to stderr)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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 |
Contributor
Greptile SummaryThis PR replaces per- Key design decisions that are well-handled:
Two minor remaining items:
Confidence Score: 5/5Safe to merge; all remaining findings are P2 style/doc suggestions with no impact on correctness for the vast majority of setups. The two open findings are both P2: a stale comment (90 s vs 30 s headroom) and a package-manager inconsistency that only affects Yarn Berry PnP users. All previously raised P0/P1 concerns (race condition, 30 s spawn cap, Windows path injection, cumulative timeout, session contamination) have been addressed in the new implementation. The core batching logic, budget distribution, and atomic append strategy are sound. scripts/hooks/stop-format-typecheck.js — stale header comment and npx hardcoding in typecheckBatch Important Files Changed
Sequence DiagramsequenceDiagram
participant CC as Claude Code
participant RWF as run-with-flags.js
participant ACC as post-edit-accumulator.js
participant TMP as /tmp/ecc-edited-{session}.txt
participant SFT as stop-format-typecheck.js
participant FMT as Biome / Prettier
participant TSC as tsc (npx)
loop For each Edit / Write / MultiEdit
CC->>RWF: PostToolUse event (stdin JSON)
RWF->>ACC: run(rawInput) [in-process]
ACC->>TMP: appendFileSync(filePath + "\n")
ACC-->>RWF: rawInput (pass-through)
RWF-->>CC: stdout = rawInput
end
CC->>RWF: Stop event (stdin JSON)
RWF->>SFT: run(rawInput) [in-process, 300 s budget]
SFT->>TMP: readFileSync → parse & deduplicate
SFT->>TMP: unlinkSync (clear accumulator)
SFT->>SFT: group by projectRoot & tsConfigDir
note over SFT: perBatchMs = 270 000 / totalBatches
loop Per project root
SFT->>FMT: biome check --write / prettier --write
FMT-->>SFT: exit 0 (or silent fail)
end
loop Per tsconfig dir
SFT->>TSC: npx tsc --noEmit --pretty false
TSC-->>SFT: errors filtered to edited files → stderr
end
SFT-->>RWF: rawInput (pass-through)
RWF-->>CC: stdout = rawInput
Reviews (8): Last reviewed commit: "fix(hooks): resolve merge conflict — kee..." | Re-trigger Greptile |
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
hooks/hooks.json (1)
169-178:⚠️ Potential issue | 🟠 MajorRemove old hook calls from
.cursor/hooksfiles to prevent duplicate processing.The new hooks.json configuration (
post:edit:accumulate+stop:format-typecheck) conflicts with existing hook handlers. Both.cursor/hooks/after-file-edit.js(lines 12–13) and.cursor/hooks/after-tab-file-edit.js(line 9) still call the oldpost-edit-format.jsandpost-edit-typecheck.jsscripts, which means your code will be formatted and type-checked twice: once per-edit via the.cursor/hooksmechanism and again in batch via the new Stop hook.Remove the redundant
runExistingHook()calls forpost-edit-format.jsandpost-edit-typecheck.jsfrom both files.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@hooks/hooks.json` around lines 169 - 178, Remove the redundant per-edit hook invocations that cause duplicate formatting/typechecking: open .cursor/hooks/after-file-edit.js and .cursor/hooks/after-tab-file-edit.js and delete or comment out the runExistingHook(...) calls that invoke post-edit-format.js and post-edit-typecheck.js (the runExistingHook calls in those files); keep the rest of the file intact so the new post:edit:accumulate and stop:format-typecheck flow handles formatting and typechecking only once.
🧹 Nitpick comments (3)
tests/hooks/stop-format-typecheck.test.js (1)
137-137: Consider hoistingrequire('child_process')to module scope.The
child_processmodule is required three times inside test functions. Moving it to the top with other requires improves readability.Minor cleanup
const assert = require('assert'); const fs = require('fs'); const os = require('os'); const path = require('path'); +const { execFileSync } = require('child_process'); const accumulator = require('../../scripts/hooks/post-edit-accumulator');Then remove the inline
require('child_process')calls from each test.Also applies to: 155-155, 168-168
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/stop-format-typecheck.test.js` at line 137, Hoist the child_process import to module scope by adding a top-level require for execFileSync (or require('child_process')) and replace the three inline require('child_process') calls inside tests with that module/function; update uses of execFileSync in the tests (e.g., in the tests that currently call require('child_process').execFileSync) to reference the hoisted execFileSync variable instead to improve readability and avoid repeated requires.scripts/hooks/stop-format-typecheck.js (1)
51-92: Batch formatting implementation is solid with good Windows handling.The UNSAFE_PATH_CHARS check before using
shell: trueon Windows is a proper security measure. Falling back silently when formatter fails keeps the hook non-blocking.One potential concern: passing many files as arguments could exceed command-line length limits on Windows (~8191 chars) or Unix systems (~128KB). For typical refactors this is unlikely, but a 100+ file batch with long paths could hit this limit.
Consider chunking very large batches
If you want to handle edge cases with many files:
// Chunk files to avoid command-line length limits (~8000 chars on Windows) const MAX_ARG_LENGTH = 7000; function chunkByArgLength(files) { const chunks = []; let current = []; let currentLen = 0; for (const f of files) { if (currentLen + f.length + 1 > MAX_ARG_LENGTH && current.length > 0) { chunks.push(current); current = []; currentLen = 0; } current.push(f); currentLen += f.length + 1; } if (current.length > 0) chunks.push(current); return chunks; }Then iterate over chunks in
formatBatch. This is optional since typical refactors won't hit the limit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/stop-format-typecheck.js` around lines 51 - 92, formatBatch can exceed OS command-line length when passing many/long file paths; implement chunking of existingFiles by a safe MAX_ARG_LENGTH (e.g., ~7000 on Windows) and iterate over each chunk instead of one big call. Update formatBatch (reference function name) to split existingFiles into chunks (use file path lengths + separators to decide chunk boundaries), then for each chunk run the same Windows (.cmd + shell + UNSAFE_PATH_CHARS check using UNSAFE_PATH_CHARS) or non-Windows execFileSync logic with the same options and error handling; keep the try/catch behavior and timeouts unchanged and ensure early returns only when no files/chunks exist.scripts/hooks/post-edit-accumulator.js (1)
24-27: Consider sanitizing the session ID before using it in the filename.If
CLAUDE_SESSION_IDcontains path separators or special characters, it could result in unexpected file paths. While this is unlikely in practice (Claude controls the session ID), defensive sanitization would prevent potential path traversal issues.🛡️ Optional sanitization
function getAccumFile() { - const sessionId = process.env.CLAUDE_SESSION_ID || 'default'; + const rawId = process.env.CLAUDE_SESSION_ID || 'default'; + // Sanitize: keep only alphanumeric, dash, underscore + const sessionId = rawId.replace(/[^a-zA-Z0-9_-]/g, '_'); return path.join(os.tmpdir(), `ecc-edited-${sessionId}.json`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/post-edit-accumulator.js` around lines 24 - 27, getAccumFile currently uses process.env.CLAUDE_SESSION_ID directly when building a temp filename (in function getAccumFile), which could allow path separators or unsafe chars into the path; sanitize the sessionId before joining by normalizing to a safe filename (e.g., strip or replace path separators and any non-alphanumeric/[-_.] characters, or fallback to a hash/base64 of the original value) and then use the sanitized value when constructing the filename so path.join(os.tmpdir(), `ecc-edited-${sanitizedSessionId}.json`) cannot be influenced by directory traversal or special characters.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@hooks/hooks.json`:
- Around line 169-178: Remove the redundant per-edit hook invocations that cause
duplicate formatting/typechecking: open .cursor/hooks/after-file-edit.js and
.cursor/hooks/after-tab-file-edit.js and delete or comment out the
runExistingHook(...) calls that invoke post-edit-format.js and
post-edit-typecheck.js (the runExistingHook calls in those files); keep the rest
of the file intact so the new post:edit:accumulate and stop:format-typecheck
flow handles formatting and typechecking only once.
---
Nitpick comments:
In `@scripts/hooks/post-edit-accumulator.js`:
- Around line 24-27: getAccumFile currently uses process.env.CLAUDE_SESSION_ID
directly when building a temp filename (in function getAccumFile), which could
allow path separators or unsafe chars into the path; sanitize the sessionId
before joining by normalizing to a safe filename (e.g., strip or replace path
separators and any non-alphanumeric/[-_.] characters, or fallback to a
hash/base64 of the original value) and then use the sanitized value when
constructing the filename so path.join(os.tmpdir(),
`ecc-edited-${sanitizedSessionId}.json`) cannot be influenced by directory
traversal or special characters.
In `@scripts/hooks/stop-format-typecheck.js`:
- Around line 51-92: formatBatch can exceed OS command-line length when passing
many/long file paths; implement chunking of existingFiles by a safe
MAX_ARG_LENGTH (e.g., ~7000 on Windows) and iterate over each chunk instead of
one big call. Update formatBatch (reference function name) to split
existingFiles into chunks (use file path lengths + separators to decide chunk
boundaries), then for each chunk run the same Windows (.cmd + shell +
UNSAFE_PATH_CHARS check using UNSAFE_PATH_CHARS) or non-Windows execFileSync
logic with the same options and error handling; keep the try/catch behavior and
timeouts unchanged and ensure early returns only when no files/chunks exist.
In `@tests/hooks/stop-format-typecheck.test.js`:
- Line 137: Hoist the child_process import to module scope by adding a top-level
require for execFileSync (or require('child_process')) and replace the three
inline require('child_process') calls inside tests with that module/function;
update uses of execFileSync in the tests (e.g., in the tests that currently call
require('child_process').execFileSync) to reference the hoisted execFileSync
variable instead to improve readability and avoid repeated requires.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8863aeb7-3edc-483c-8042-dd35574f6220
📒 Files selected for processing (4)
hooks/hooks.jsonscripts/hooks/post-edit-accumulator.jsscripts/hooks/stop-format-typecheck.jstests/hooks/stop-format-typecheck.test.js
Contributor
There was a problem hiding this comment.
6 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/hooks/post-edit-accumulator.js">
<violation number="1" location="scripts/hooks/post-edit-accumulator.js:25">
P1: Unsanitized `CLAUDE_SESSION_ID` is used in `path.join` for a writable temp-file path, allowing path normalization to escape the intended temp directory with crafted separator/`..` input.</violation>
</file>
<file name="scripts/hooks/stop-format-typecheck.js">
<violation number="1" location="scripts/hooks/stop-format-typecheck.js:41">
P1: Fallbacking to a shared `'default'` session ID breaks accumulator isolation and can cause cross-run edit loss when stop unlinks the shared file.</violation>
<violation number="2" location="scripts/hooks/stop-format-typecheck.js:41">
P1: Unsanitized CLAUDE_SESSION_ID is used in accumulator path construction, allowing path traversal that can redirect read/unlink to unintended files.</violation>
<violation number="3" location="scripts/hooks/stop-format-typecheck.js:105">
P2: Tsconfig lookup is artificially capped at 20 levels and can silently skip typechecking for valid edited TS files when config is higher in the tree.</violation>
</file>
<file name="hooks/hooks.json">
<violation number="1" location="hooks/hooks.json:174">
P1: Batched Stop format/typecheck can miss files because the accumulator is only attached to `Edit`, while the Stop description claims coverage of all edited JS/TS files in the response.</violation>
<violation number="2" location="hooks/hooks.json:232">
P2: Hard 120s Stop timeout can prematurely kill legitimate batched format/typecheck runs, causing incomplete checks in larger or multi-tsconfig repositories.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
Contributor
There was a problem hiding this comment.
4 issues found across 3 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/hooks/stop-format-typecheck.test.js">
<violation number="1" location="tests/hooks/stop-format-typecheck.test.js:97">
P2: The test claims to validate concurrent-write safety but only performs synchronous serial calls, so it cannot catch real race-condition data loss.</violation>
<violation number="2" location="tests/hooks/stop-format-typecheck.test.js:106">
P2: The modified tests no longer validate deduplication in the Stop hook path; they dedup inside the test itself, leaving a regression risk for the batching behavior.</violation>
</file>
<file name="scripts/hooks/post-edit-accumulator.js">
<violation number="1" location="scripts/hooks/post-edit-accumulator.js:49">
P2: Raw newline-delimited path accumulation is lossy for valid filenames containing newlines, causing the Stop hook to misparse edited files.</violation>
</file>
<file name="scripts/hooks/stop-format-typecheck.js">
<violation number="1" location="scripts/hooks/stop-format-typecheck.js:40">
P1: Windows formatter batching is incorrectly skipped when any file path contains spaces/parentheses, causing whole-batch formatting to be dropped.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
scripts/hooks/stop-format-typecheck.js (3)
171-176: Race condition window between read and unlink.There's a small window where another process could write to the accumulator after
readFileSyncbut beforeunlinkSync. Those paths would be lost. Consider using atomic rename-then-read pattern, or accept this as a rare edge case given the session-scoped design.💡 Alternative: atomic rename before read
+ const processingFile = accumFile + '.processing'; + let raw; try { + // Atomically claim the file to prevent race with concurrent writes + fs.renameSync(accumFile, processingFile); + raw = fs.readFileSync(processingFile, 'utf8'); + } catch (e) { + if (e.code === 'ENOENT') return; // No accumulator + throw e; + } + + try { + fs.unlinkSync(processingFile); - raw = fs.readFileSync(accumFile, 'utf8'); } catch { - // No accumulator — nothing was edited this response, nothing to do - return; + // Best-effort cleanup } - - // Clear immediately so repeated Stop calls don't double-process - try { - fs.unlinkSync(accumFile); - } catch { - // Best-effort - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/stop-format-typecheck.js` around lines 171 - 176, The current clear logic races between fs.readFileSync and fs.unlinkSync on accumFile; to fix it, perform an atomic rename of accumFile to a temp path (e.g., append a unique suffix) before reading so no other process can write into the renamed file, then read the temp path and finally unlink the temp file, replacing the direct fs.unlinkSync(accumFile) call; reference the accumFile variable and the existing fs.readFileSync/fs.unlinkSync usage in this file when making the change.
160-213:main()is 54 lines, slightly exceeding the 50-line guideline.Consider extracting the formatting loop (lines 183-196) or typechecking loop (lines 198-212) into helper functions to improve readability and stay within guidelines.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/stop-format-typecheck.js` around lines 160 - 213, The main() function exceeds the line-count guideline—extract one of the two grouping+dispatch loops into a helper to reduce main() size: move the "Format: group by project root" loop (the code that builds byProjectRoot and calls formatBatch) or the "Typecheck: group by tsconfig dir" loop (the code that builds byTsConfigDir and calls typecheckBatch) into a new function (e.g., collectAndFormatFiles(rootFiles) or collectAndTypecheckFiles(tsFiles)) that accepts the files array and uses findProjectRoot/formatBatch or findTsConfigDir/typecheckBatch respectively; update main() to call the new helper and keep all existing checks (extension filters, path.resolve, fs.existsSync) and Set deduplication intact.
136-150: Consider guarding against false positives from partial basename matches.The current filter checks if the line
includes(candidate). If two edited files share a common substring (e.g.,src/utils.tsandsrc/utils-extra.ts), errors from one could be attributed to both. The candidates Set helps, but tsc output format ispath(line,col): error..., so a more precise match on the path portion would reduce false positives.💡 More precise path matching
const relevantLines = lines .filter(line => { for (const candidate of candidates) { - if (line.includes(candidate)) return true; + // tsc format: path(line,col): error TS... + // Match at start of line or after whitespace to avoid substring false positives + if (line.startsWith(candidate) || line.includes(' ' + candidate)) return true; } return false; })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/stop-format-typecheck.js` around lines 136 - 150, The filter can false-match when one file path is a substring of another (e.g., src/utils.ts vs src/utils-extra.ts); change the predicate in the relevantLines computation to match the path portion of tsc output precisely: for each candidate (from editedFiles and relPath) build a regex-escaped candidate that checks for the candidate followed immediately by a tsc delimiter like "(" or ":" (also normalize path separators) and only accept lines where that precise regex matches, rather than using line.includes(candidate); update the variables used here (editedFiles, relPath, candidates, relevantLines, lines) accordingly to perform this stricter match.scripts/hooks/post-edit-accumulator.js (1)
42-56: Consider validating thatfilePathdoesn't contain newlines.Since the accumulator uses newline-delimited format, a malicious or malformed
file_pathcontaining embedded newlines could inject additional paths. While unlikely in practice, adding a simple check would harden the input validation.🛡️ Optional defensive check
function run(rawInput) { try { const input = JSON.parse(rawInput); const filePath = input.tool_input?.file_path; - if (filePath && /\.(ts|tsx|js|jsx)$/.test(filePath)) { + if (filePath && /\.(ts|tsx|js|jsx)$/.test(filePath) && !filePath.includes('\n')) { // appendFileSync is atomic for small writes — safe under concurrency fs.appendFileSync(getAccumFile(), filePath + '\n', 'utf8'); } } catch { // Invalid input — pass through } return rawInput; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/post-edit-accumulator.js` around lines 42 - 56, The run function currently appends tool_input.file_path directly to the accumulator which allows embedded newlines to inject extra entries; before calling fs.appendFileSync(getAccumFile(), filePath + '\n', 'utf8') add a validation that filePath is a non-empty string and does not contain newline or carriage return characters (e.g. check filePath && !/[\r\n]/.test(filePath)); if the check fails, skip the append (optionally log or silently ignore) and still return rawInput unchanged. Ensure you update the validation around the existing filePath variable in run so only safe, newline-free paths are written to getAccumFile().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/hooks/post-edit-accumulator.js`:
- Line 12: Update the descriptive comment "This hook is intentionally cheap: it
only appends a path to a JSON file" to accurately state the accumulator is a
newline-delimited text file (e.g., ".txt"); locate that comment in
post-edit-accumulator.js and replace "JSON file" with "newline-delimited text
file (plain .txt)" so the docs match the actual behavior.
In `@scripts/hooks/stop-format-typecheck.js`:
- Around line 123-131: The typecheckBatch function currently runs npx via
execFileSync (using npx.cmd on Windows), which fails for .cmd batch files;
mirror the formatBatch approach: detect process.platform === 'win32' and if so
use spawnSync with shell: true to invoke the command (instead of execFileSync),
otherwise run the POSIX binary normally; update typecheckBatch to use the same
npx detection and spawnSync + shell: true pattern as formatBatch to ensure
Windows .cmd files execute correctly.
---
Nitpick comments:
In `@scripts/hooks/post-edit-accumulator.js`:
- Around line 42-56: The run function currently appends tool_input.file_path
directly to the accumulator which allows embedded newlines to inject extra
entries; before calling fs.appendFileSync(getAccumFile(), filePath + '\n',
'utf8') add a validation that filePath is a non-empty string and does not
contain newline or carriage return characters (e.g. check filePath &&
!/[\r\n]/.test(filePath)); if the check fails, skip the append (optionally log
or silently ignore) and still return rawInput unchanged. Ensure you update the
validation around the existing filePath variable in run so only safe,
newline-free paths are written to getAccumFile().
In `@scripts/hooks/stop-format-typecheck.js`:
- Around line 171-176: The current clear logic races between fs.readFileSync and
fs.unlinkSync on accumFile; to fix it, perform an atomic rename of accumFile to
a temp path (e.g., append a unique suffix) before reading so no other process
can write into the renamed file, then read the temp path and finally unlink the
temp file, replacing the direct fs.unlinkSync(accumFile) call; reference the
accumFile variable and the existing fs.readFileSync/fs.unlinkSync usage in this
file when making the change.
- Around line 160-213: The main() function exceeds the line-count
guideline—extract one of the two grouping+dispatch loops into a helper to reduce
main() size: move the "Format: group by project root" loop (the code that builds
byProjectRoot and calls formatBatch) or the "Typecheck: group by tsconfig dir"
loop (the code that builds byTsConfigDir and calls typecheckBatch) into a new
function (e.g., collectAndFormatFiles(rootFiles) or
collectAndTypecheckFiles(tsFiles)) that accepts the files array and uses
findProjectRoot/formatBatch or findTsConfigDir/typecheckBatch respectively;
update main() to call the new helper and keep all existing checks (extension
filters, path.resolve, fs.existsSync) and Set deduplication intact.
- Around line 136-150: The filter can false-match when one file path is a
substring of another (e.g., src/utils.ts vs src/utils-extra.ts); change the
predicate in the relevantLines computation to match the path portion of tsc
output precisely: for each candidate (from editedFiles and relPath) build a
regex-escaped candidate that checks for the candidate followed immediately by a
tsc delimiter like "(" or ":" (also normalize path separators) and only accept
lines where that precise regex matches, rather than using
line.includes(candidate); update the variables used here (editedFiles, relPath,
candidates, relevantLines, lines) accordingly to perform this stricter match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34269714-1f7a-4e73-bd9e-edb926b99222
📒 Files selected for processing (3)
scripts/hooks/post-edit-accumulator.jsscripts/hooks/stop-format-typecheck.jstests/hooks/stop-format-typecheck.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/hooks/stop-format-typecheck.test.js
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
Contributor
There was a problem hiding this comment.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="hooks/hooks.json">
<violation number="1" location="hooks/hooks.json:236">
P2: Stop hook timeout increase is likely ineffective because only matcher-entry timeout was raised while command timeout remains 120s.</violation>
</file>
<file name="tests/hooks/stop-format-typecheck.test.js">
<violation number="1" location="tests/hooks/stop-format-typecheck.test.js:193">
P2: The new dedup test does not assert deduplication behavior; it only checks accumulator deletion, so dedup regressions can pass undetected.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
Comment on lines
+40
to
+46
| function getAccumFile() { | ||
| const raw = | ||
| process.env.CLAUDE_SESSION_ID || | ||
| crypto.createHash('sha1').update(process.cwd()).digest('hex').slice(0, 12); | ||
| const sessionId = raw.replace(/[^a-zA-Z0-9_-]/g, '_').slice(0, 64); | ||
| return path.join(os.tmpdir(), `ecc-edited-${sessionId}.txt`); | ||
| } |
Contributor
There was a problem hiding this comment.
getAccumFile() duplicated — silent mismatch risk
getAccumFile() is copy-pasted verbatim in both post-edit-accumulator.js (lines 24–30) and stop-format-typecheck.js (lines 40–46). If either implementation is updated without updating the other — for example, to change the temp-file prefix or the session-ID sanitisation — the accumulator will write to one path while the Stop hook reads from a different path, and every format/typecheck pass will silently do nothing (the readFileSync catch will swallow the ENOENT and return early).
Since both files already require from ../lib/, extracting getAccumFile() into a shared utility (e.g. scripts/lib/accum-file.js) would eliminate this divergence risk entirely.
Contributor
There was a problem hiding this comment.
3 issues found across 4 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="scripts/hooks/stop-format-typecheck.js">
<violation number="1" location="scripts/hooks/stop-format-typecheck.js:33">
P2: Windows formatting batches can be incorrectly skipped because the unsafe-path filter treats valid spaces/parentheses as disqualifying and aborts the entire batch.</violation>
</file>
<file name="scripts/hooks/post-edit-accumulator.js">
<violation number="1" location="scripts/hooks/post-edit-accumulator.js:24">
P3: Extract `getAccumFile()` into a shared helper and import it from both hooks. This path computation is correctness-critical, and duplicating it in two files creates drift risk that can silently disable Stop-time format/typecheck when writer and reader diverge.</violation>
<violation number="2" location="scripts/hooks/post-edit-accumulator.js:42">
P2: Accumulator writes raw file paths into a newline-delimited file without escaping/rejecting newlines, so one path can be parsed as multiple fabricated entries.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
affaan-m
requested changes
Mar 22, 2026
affaan-m
left a comment
Owner
There was a problem hiding this comment.
Implementation is solid, close to merge. Three items:
- Full CI matrix — the new test file needs to pass across all platforms/Node versions. Check if CI ran all checks
- Old hook cleanup —
.cursor/hooks/after-file-edit.jsstill references oldpost-edit-format.js/post-edit-typecheck.js, which would cause duplicate processing - Dependency check — confirm
resolve-formatter.jsexists at the import path
Once those are addressed this is good to go.
yetval
commented
Mar 22, 2026
yetval
left a comment
Contributor
Author
There was a problem hiding this comment.
Thanks for the review! Addressed all three items:
- CI matrix — all 4 checks pass (CodeRabbit, GitGuardian, Greptile, cubic)
- Old hook cleanup — updated
.cursor/hooks/after-file-edit.jsto replace the removedpost-edit-format.js/post-edit-typecheck.jscalls withpost-edit-accumulator.js - Dependency check —
resolve-formatter.jsexists atscripts/lib/resolve-formatter.jswhich matches the import path instop-format-typecheck.js
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
Comment on lines
+91
to
+113
| function typecheckBatch(tsConfigDir, editedFiles, timeoutMs) { | ||
| const isWin = process.platform === 'win32'; | ||
| const npxBin = isWin ? 'npx.cmd' : 'npx'; | ||
| const args = ['tsc', '--noEmit', '--pretty', 'false']; | ||
| const opts = { cwd: tsConfigDir, encoding: 'utf8', stdio: ['pipe', 'pipe', 'pipe'], timeout: timeoutMs }; | ||
|
|
||
| let stdout = ''; | ||
| let stderr = ''; | ||
| let failed = false; | ||
|
|
||
| try { | ||
| if (isWin) { | ||
| // .cmd files require shell: true on Windows | ||
| const result = spawnSync(npxBin, args, { ...opts, shell: true }); | ||
| if (result.error) return; // timed out or not found — non-blocking | ||
| if (result.status !== 0) { | ||
| stdout = result.stdout || ''; | ||
| stderr = result.stderr || ''; | ||
| failed = true; | ||
| } | ||
| } else { | ||
| execFileSync(npxBin, args, opts); | ||
| } |
Contributor
There was a problem hiding this comment.
On non-Windows, a tsc timeout causes execFileSync to throw with err.code === 'ETIMEDOUT'. The catch block captures partial err.stdout/err.stderr and sets failed = true, so any output collected before the kill is still shown to the user.
On Windows (line 105), a timeout produces result.error.code === 'ETIMEDOUT' which is caught by if (result.error) return;, silently swallowing both the timeout signal and any partial diagnostic output. The user receives no feedback that typecheck was aborted.
Consider emitting a stderr warning when the Windows path returns early due to an error:
if (result.error) {
process.stderr.write(`[Hook] typecheckBatch aborted (${result.error.code || result.error.message})\n`);
return;
}
Comment on lines
+122
to
+132
| const lines = (stdout + stderr).split('\n'); | ||
| for (const filePath of editedFiles) { | ||
| const relPath = path.relative(tsConfigDir, filePath); | ||
| const candidates = new Set([filePath, relPath]); | ||
| const relevantLines = lines | ||
| .filter(line => { for (const c of candidates) { if (line.includes(c)) return true; } return false; }) | ||
| .slice(0, 10); | ||
| if (relevantLines.length > 0) { | ||
| process.stderr.write(`[Hook] TypeScript errors in ${path.basename(filePath)}:\n`); | ||
| relevantLines.forEach(line => process.stderr.write(line + '\n')); | ||
| } |
Contributor
There was a problem hiding this comment.
path.relative(tsConfigDir, filePath) on Windows returns backslash-separated paths (e.g. src\components\Button.tsx), but TypeScript's diagnostic messages consistently use forward slashes on all platforms (e.g. src/components/Button.tsx(10,5): error TS2345…).
The line.includes(relPath) check therefore always fails on Windows, so typecheck errors in edited files are never surfaced to the user even when tsc exits non-zero.
One portable fix:
const relPath = path.relative(tsConfigDir, filePath).replace(/\\/g, '/');
Owner
|
thanks for the contribution. there are merge conflicts with the latest |
Fixes #735. The per-edit post:edit:format and post:edit:typecheck hooks ran synchronously after every Edit call, adding 15-30s of latency per file — up to 7.5 minutes for a 10-file refactor. New approach: - post-edit-accumulator.js (PostToolUse/Edit): lightweight hook that records each edited JS/TS path to a session-scoped temp file in os.tmpdir(). No formatters, no tsc — exits in microseconds. - stop-format-typecheck.js (Stop): reads the accumulator once per response, groups files by project root and runs the formatter in one batched invocation per root, then groups .ts/.tsx files by tsconfig dir and runs tsc once per tsconfig. Clears the accumulator immediately on read so repeated Stop calls don't double-process. For a 10-file refactor: was 10 × (15s + 30s) = 7.5 min overhead, now 1 × (batch format + batch tsc) = ~5-30s total.
…uard Three issues raised in code review: 1. Race condition: switched accumulator from non-atomic JSON read-modify-write to appendFileSync (one path per line). Concurrent Edit hook processes each append independently without clobbering each other. Deduplication moved to the Stop hook at read time. 2. Effective timeout: added run() export to stop-format-typecheck.js so run-with-flags.js uses the direct require() path instead of falling through to spawnSync (which has a hardcoded 30s cap). The 120s timeout in hooks.json now governs the full batch as intended. 3. Windows path guard: added spaces and parentheses to UNSAFE_PATH_CHARS so paths like "C:\Users\John Doe\project\file.ts" are caught before being passed to cmd.exe with shell: true.
- Replace 'default' session ID fallback with a cwd-based sha1 hash so concurrent sessions in different projects don't share the same accumulator file when CLAUDE_SESSION_ID is unset - Remove stale "JSON file" reference in accumulator header (format is now newline-delimited plain text) - Remove redundant/verbose inline comments throughout both files
- Sanitize CLAUDE_SESSION_ID with /[^a-zA-Z0-9_-]/g before embedding in the temp filename so crafted separators or '..' sequences cannot escape os.tmpdir() (cubic P1) - Fix typecheckBatch on Windows: npx.cmd requires shell:true like formatBatch already does; use spawnSync and extract stdout/stderr from the result object (coderabbit P1) - Proportional per-batch timeouts: divide 270s budget across all format and typecheck batches so sequential runs in monorepos stay within the Stop hook wall-clock limit (greptile P2) - Raise Stop hook timeout from 120s to 300s to give large monorepos adequate headroom (cubic P2)
- Extend matcher from Edit to Edit|Write|MultiEdit so files created with Write and all files in a MultiEdit batch are included in the Stop-time format+typecheck pass (cubic P1) - Handle tool_input.edits[] array in accumulator for MultiEdit support - Rename misleading 'concurrent writes' test to clarify it tests append preservation, not true concurrency (cubic P2) - Add Stop hook dedup test: writes duplicate paths to accumulator and verifies the hook clears it cleanly (cubic P2) - Add Write and MultiEdit accumulation tests
- Move timeout: 300 from the matcher object to the hook command object where it is actually enforced; the previous position was a no-op (cubic P2) - Extract parseAccumulator() and export it so tests can assert dedup behavior directly without relying only on side effects (cubic P2) - Add two unit tests for parseAccumulator: deduplication and blank-line handling; rename the integration test to match its scope
35b6611 to
0019909
Compare
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
…te check-console-log to inline bootstrap Keeps the new batch stop-format-typecheck hook from this PR and adopts upstream's inline bootstrap form for the check-console-log Stop hook (consistent with how other Stop hooks are wired after the upstream change).
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
19 tasks
peiking88
pushed a commit
to peiking88/everything-claude-code
that referenced
this pull request
Apr 4, 2026
…an-m#746) * perf(hooks): batch format+typecheck at Stop instead of per Edit Fixes affaan-m#735. The per-edit post:edit:format and post:edit:typecheck hooks ran synchronously after every Edit call, adding 15-30s of latency per file — up to 7.5 minutes for a 10-file refactor. New approach: - post-edit-accumulator.js (PostToolUse/Edit): lightweight hook that records each edited JS/TS path to a session-scoped temp file in os.tmpdir(). No formatters, no tsc — exits in microseconds. - stop-format-typecheck.js (Stop): reads the accumulator once per response, groups files by project root and runs the formatter in one batched invocation per root, then groups .ts/.tsx files by tsconfig dir and runs tsc once per tsconfig. Clears the accumulator immediately on read so repeated Stop calls don't double-process. For a 10-file refactor: was 10 × (15s + 30s) = 7.5 min overhead, now 1 × (batch format + batch tsc) = ~5-30s total. * fix(hooks): address race condition, spawn timeout, and Windows path guard Three issues raised in code review: 1. Race condition: switched accumulator from non-atomic JSON read-modify-write to appendFileSync (one path per line). Concurrent Edit hook processes each append independently without clobbering each other. Deduplication moved to the Stop hook at read time. 2. Effective timeout: added run() export to stop-format-typecheck.js so run-with-flags.js uses the direct require() path instead of falling through to spawnSync (which has a hardcoded 30s cap). The 120s timeout in hooks.json now governs the full batch as intended. 3. Windows path guard: added spaces and parentheses to UNSAFE_PATH_CHARS so paths like "C:\Users\John Doe\project\file.ts" are caught before being passed to cmd.exe with shell: true. * fix(hooks): fix session fallback, stale comment, trim verbose comments - Replace 'default' session ID fallback with a cwd-based sha1 hash so concurrent sessions in different projects don't share the same accumulator file when CLAUDE_SESSION_ID is unset - Remove stale "JSON file" reference in accumulator header (format is now newline-delimited plain text) - Remove redundant/verbose inline comments throughout both files * fix(hooks): sanitize session ID, fix Windows tsc, proportional timeouts - Sanitize CLAUDE_SESSION_ID with /[^a-zA-Z0-9_-]/g before embedding in the temp filename so crafted separators or '..' sequences cannot escape os.tmpdir() (cubic P1) - Fix typecheckBatch on Windows: npx.cmd requires shell:true like formatBatch already does; use spawnSync and extract stdout/stderr from the result object (coderabbit P1) - Proportional per-batch timeouts: divide 270s budget across all format and typecheck batches so sequential runs in monorepos stay within the Stop hook wall-clock limit (greptile P2) - Raise Stop hook timeout from 120s to 300s to give large monorepos adequate headroom (cubic P2) * fix(hooks): extend accumulator to Write|MultiEdit, fix tests - Extend matcher from Edit to Edit|Write|MultiEdit so files created with Write and all files in a MultiEdit batch are included in the Stop-time format+typecheck pass (cubic P1) - Handle tool_input.edits[] array in accumulator for MultiEdit support - Rename misleading 'concurrent writes' test to clarify it tests append preservation, not true concurrency (cubic P2) - Add Stop hook dedup test: writes duplicate paths to accumulator and verifies the hook clears it cleanly (cubic P2) - Add Write and MultiEdit accumulation tests * fix(hooks): move timeout to command level, add dedup unit tests - Move timeout: 300 from the matcher object to the hook command object where it is actually enforced; the previous position was a no-op (cubic P2) - Extract parseAccumulator() and export it so tests can assert dedup behavior directly without relying only on side effects (cubic P2) - Add two unit tests for parseAccumulator: deduplication and blank-line handling; rename the integration test to match its scope * fix(hooks): replace removed format/typecheck hooks with accumulator in cursor adapter
1 task
FrancescoRosciano
pushed a commit
to FRosciano-Mambo/everything-claude-code
that referenced
this pull request
Jun 1, 2026
…an-m#746) * perf(hooks): batch format+typecheck at Stop instead of per Edit Fixes affaan-m#735. The per-edit post:edit:format and post:edit:typecheck hooks ran synchronously after every Edit call, adding 15-30s of latency per file — up to 7.5 minutes for a 10-file refactor. New approach: - post-edit-accumulator.js (PostToolUse/Edit): lightweight hook that records each edited JS/TS path to a session-scoped temp file in os.tmpdir(). No formatters, no tsc — exits in microseconds. - stop-format-typecheck.js (Stop): reads the accumulator once per response, groups files by project root and runs the formatter in one batched invocation per root, then groups .ts/.tsx files by tsconfig dir and runs tsc once per tsconfig. Clears the accumulator immediately on read so repeated Stop calls don't double-process. For a 10-file refactor: was 10 × (15s + 30s) = 7.5 min overhead, now 1 × (batch format + batch tsc) = ~5-30s total. * fix(hooks): address race condition, spawn timeout, and Windows path guard Three issues raised in code review: 1. Race condition: switched accumulator from non-atomic JSON read-modify-write to appendFileSync (one path per line). Concurrent Edit hook processes each append independently without clobbering each other. Deduplication moved to the Stop hook at read time. 2. Effective timeout: added run() export to stop-format-typecheck.js so run-with-flags.js uses the direct require() path instead of falling through to spawnSync (which has a hardcoded 30s cap). The 120s timeout in hooks.json now governs the full batch as intended. 3. Windows path guard: added spaces and parentheses to UNSAFE_PATH_CHARS so paths like "C:\Users\John Doe\project\file.ts" are caught before being passed to cmd.exe with shell: true. * fix(hooks): fix session fallback, stale comment, trim verbose comments - Replace 'default' session ID fallback with a cwd-based sha1 hash so concurrent sessions in different projects don't share the same accumulator file when CLAUDE_SESSION_ID is unset - Remove stale "JSON file" reference in accumulator header (format is now newline-delimited plain text) - Remove redundant/verbose inline comments throughout both files * fix(hooks): sanitize session ID, fix Windows tsc, proportional timeouts - Sanitize CLAUDE_SESSION_ID with /[^a-zA-Z0-9_-]/g before embedding in the temp filename so crafted separators or '..' sequences cannot escape os.tmpdir() (cubic P1) - Fix typecheckBatch on Windows: npx.cmd requires shell:true like formatBatch already does; use spawnSync and extract stdout/stderr from the result object (coderabbit P1) - Proportional per-batch timeouts: divide 270s budget across all format and typecheck batches so sequential runs in monorepos stay within the Stop hook wall-clock limit (greptile P2) - Raise Stop hook timeout from 120s to 300s to give large monorepos adequate headroom (cubic P2) * fix(hooks): extend accumulator to Write|MultiEdit, fix tests - Extend matcher from Edit to Edit|Write|MultiEdit so files created with Write and all files in a MultiEdit batch are included in the Stop-time format+typecheck pass (cubic P1) - Handle tool_input.edits[] array in accumulator for MultiEdit support - Rename misleading 'concurrent writes' test to clarify it tests append preservation, not true concurrency (cubic P2) - Add Stop hook dedup test: writes duplicate paths to accumulator and verifies the hook clears it cleanly (cubic P2) - Add Write and MultiEdit accumulation tests * fix(hooks): move timeout to command level, add dedup unit tests - Move timeout: 300 from the matcher object to the hook command object where it is actually enforced; the previous position was a no-op (cubic P2) - Extract parseAccumulator() and export it so tests can assert dedup behavior directly without relying only on side effects (cubic P2) - Add two unit tests for parseAccumulator: deduplication and blank-line handling; rename the integration test to match its scope * fix(hooks): replace removed format/typecheck hooks with accumulator in cursor adapter
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
2 participants
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.
Summary
Fixes #735. The per-edit
post:edit:formatandpost:edit:typecheckhooks ran synchronously after everyEditcall — up to 7.5 minutes of overhead for a 10-file refactor. This replaces them with a two-part pipeline that runs once per response instead.post-edit-accumulator.js(PostToolUse/Edit): records each edited JS/TS path to a session-scoped temp file. No formatters, no tsc. Exits in microseconds.stop-format-typecheck.js(Stop): reads the accumulator once at the end of the response, batches format by project root (one formatter invocation per root), batches typecheck by tsconfig dir (onetsc --noEmitper tsconfig).For a 10-file refactor:
10 × (15s + 30s) = 7.5 min→ one batch pass (~5–30s total).Type
Testing
node tests/hooks/stop-format-typecheck.test.js→ 11/11 passnode tests/hooks/hooks.test.js→ 215/215 passChecklist
Summary by cubic
Batch JS/TS formatting and TypeScript checks now run once at Stop instead of after each edit, cutting per-response latency from minutes to seconds for multi-file changes. Fixes #735.
Refactors
Edit|Write|MultiEditaccumulator that appends paths atomically to a session-scoped temp file; sanitizesCLAUDE_SESSION_IDwith a cwd‑SHA‑1 fallback; Cursor adapter now callspost-edit-accumulator.js.tsc --noEmit; proportional per-batch timeouts within a 270s budget; Windows fixes (npx.cmdwithshell:true, broader unsafe-path guard); switchedcheck-console-logStop hook to the inline bootstrap form;hooks/hooks.jsonsets a 300s command-level timeout.Tests
parseAccumulatorwith unit tests for dedup and blank-line handling.Written for commit 5aabb24. Summary will update on new commits.
Summary by CodeRabbit
Refactor
New Features
Tests