chore: repo hygiene cleanup + test quality/speed uplift#7
Conversation
|
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:
📝 Walkthroughwalkthroughadds a repo-hygiene cli with changes
sequence diagram(s)sequenceDiagram
actor user
participant cli as "repo-hygiene cli"
participant git as git
participant fs as filesystem
rect rgba(100,150,200,0.5)
Note over user,git: check command flow
user->>cli: run check
cli->>git: git ls-files
git-->>cli: tracked files
cli->>fs: read .gitignore
fs-->>cli: patterns
cli->>cli: validate tracked-scratch absence & required patterns
alt violations
cli-->>user: error + non-zero exit
else
cli-->>user: success
end
end
rect rgba(150,100,200,0.5)
Note over user,fs: clean command flow
user->>cli: run clean [--dry-run | --mode aggressive]
cli->>cli: parse args & validate repo root
cli->>fs: scan for candidates (generated dirs, root-temp, tracked-scratch)
cli->>cli: filter protected names & safety-check paths
alt safety fail
cli-->>user: error + non-zero exit
else dry-run
cli-->>user: report planned deletions
else perform delete
cli->>fs: delete paths
fs-->>cli: deletions complete
cli-->>user: summary
end
end
estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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/repo-hygiene.js`:
- Around line 149-160: The getTrackedPaths function currently swallows all
errors from execFileSync and returns an empty array; change the catch to capture
the error (e) and either log a clear warning with the error details using
console.warn (including context like "git ls-files failed in getTrackedPaths")
or rethrow/exit to make the failure fatal depending on desired behavior; update
the catch block in getTrackedPaths to include the error variable and call
console.warn or throw the error so missing git or repo corruption doesn't
silently pass.
- Around line 181-183: The code reads .gitignore without handling a missing file
which can throw; wrap the read in a try/catch around the await fs.readFile call
(referencing gitignorePath and the gitignore variable) and if the file is
absent, treat gitignore as an empty string or log a clear diagnostic and
continue; then compute missingPatterns using REQUIRED_GITIGNORE_PATTERNS.filter
as before and ensure the error path returns/continues gracefully instead of
letting the exception bubble.
- Around line 51-62: The loop that parses argv reads argv[i + 1] for "--mode"
and "--root" without checking bounds, so update the parsing in the for-loop to
validate that argv[i + 1] exists and is not another flag before assigning to
args.mode or args.root; if the value is missing, throw or print a clear error
and exit (or set a sensible default) so you don't assign undefined. Specifically
modify the block handling the "--mode" and "--root" cases (the code that sets
args.mode and args.root using argv[i + 1]) to check existence and non-flagness
of argv[i + 1] and only increment i after a successful assignment. Ensure the
"--dry-run" handling remains unchanged.
- Line 183: The current substring check using gitignore.includes(pattern) yields
false positives; change to a line-based match by splitting gitignore into lines
(e.g., gitignore.split(/\r?\n/)), trimming each line and filtering out empty
lines/comments, then compute missingPatterns by checking that no gitignore line
equals the REQUIRED_GITIGNORE_PATTERNS entry (or matches the exact pattern
string) so only whole-line matches satisfy the requirement; update the variable
referenced (missingPatterns) and use the existing REQUIRED_GITIGNORE_PATTERNS
and gitignore identifiers when implementing the stricter check.
In `@test/repo-hygiene.test.ts`:
- Around line 38-49: The test creates a temp repo with makeRepoFixture() and
removes it only at the end of the test body, which leaks temp dirs if assertions
fail; change the test suite to track the created root and perform cleanup in an
afterEach (or wrap each test body in try/finally) so removal always runs.
Specifically, keep using makeRepoFixture() to set root and the existing fs.rm
call but move the removal into an afterEach that checks and deletes the tracked
root (or ensure each test enclosing logic around runRepoHygiene(...),
expect(...), and fs.stat(...) is wrapped in try/finally) so tests in this file
never leave temporary directories behind.
- Around line 31-35: The tests omit a Windows CRLF regression case: update or
add a test in test/** that uses makeRepoFixture and configures the git repo to
produce CRLF output (e.g., set core.autocrlf=true or write a committed file with
\r\n line endings), then run the code path that reads git ls-files and ensure
parsing in scripts/repo-hygiene.js (see the regexp at repo-hygiene.js:156)
handles CRLF correctly; concretely, add a new test which uses makeRepoFixture to
init the repo, creates/commits a file with CRLF line endings, invokes the
repo-hygiene logic, and asserts the expected output to prevent regression.
In `@test/stream-failover.test.ts`:
- Around line 38-40: The file already calls vi.useRealTimers() in the afterEach
cleanup block, so remove the redundant per-test invocations of
vi.useRealTimers() inside individual tests in test/stream-failover.test.ts
(search for and delete the standalone vi.useRealTimers() calls that appear
within test bodies); leave the afterEach(() => { vi.useRealTimers(); }) intact
so timers are restored for every test.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (8)
.github/workflows/ci.yml.gitignorepackage.jsonscripts/repo-hygiene.jstest-results.mdtest/repo-hygiene.test.tstest/stream-failover.test.tsvitest.config.ts
💤 Files with no reviewable changes (1)
- test-results.md
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/stream-failover.test.tstest/repo-hygiene.test.ts
🧬 Code graph analysis (2)
test/stream-failover.test.ts (1)
lib/request/stream-failover.ts (1)
withStreamingFailover(115-234)
test/repo-hygiene.test.ts (1)
scripts/repo-hygiene.js (2)
args(39-44)args(200-200)
🔇 Additional comments (9)
.gitignore (1)
29-29: lgtm.addition of
test-results.mdto.gitignoreis consistent withscripts/repo-hygiene.js:17-22tracked scratch files set and aligns with the hygiene tooling.package.json (1)
45-46: lgtm.scripts wired correctly to
scripts/repo-hygiene.js. clean command requires--mode aggressivewhich matches the validation atscripts/repo-hygiene.js:67-69..github/workflows/ci.yml (1)
31-33: lgtm.hygiene check runs after
npm ciand before tests. placement is correct. running only on the test matrix is reasonable since the check is node-version-agnostic.test/stream-failover.test.ts (2)
52-68: deterministic timer approach is correct.the refactor from real timeouts to fake timers at
test/stream-failover.test.ts:53-67eliminates flakiness. the pattern of capturingtextPromisebefore advancing timers ensures the stream read is in flight when time advances.
89-102: rejection test pattern looks correct.capturing the assertion promise before advancing timers at
test/stream-failover.test.ts:97-100is the right way to test async rejections with fake timers. this avoids races between timer advancement and promise settlement.vitest.config.ts (1)
12-17: lgtm.exclusion patterns align with
scripts/repo-hygiene.js:9-16GENERATED_DIRS. prevents vitest from scanning temp/generated directories that may contain stale.test.tsfiles.Also applies to: 22-27
test/repo-hygiene.test.ts (2)
69-81: good coverage for tracked scratch file detection.test at
test/repo-hygiene.test.ts:69-81correctly usesgit add -fto force-add an ignored file and verifies the check command catches it.
93-102: lgtm.test at
test/repo-hygiene.test.ts:93-102verifies the happy path. assertions for exit code and stdout message are appropriate.scripts/repo-hygiene.js (1)
127-147: clean function is solid.
scripts/repo-hygiene.js:127-147has good defensive checks: refuses to delete paths outside root (line 136-138), uses{ recursive: true, force: true }for safe removal. no concurrency risks since operations are sequential.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
scripts/repo-hygiene.js (3)
199-209:⚠️ Potential issue | 🟠 Majordo not silently treat git failures as clean state
lib/scripts/repo-hygiene.js:207-209(Line 207) returns[]on anygit ls-fileserror. that can makecheckpass when git is unavailable or repo metadata is broken.proposed fix
- } catch { - return []; + } catch (error) { + throw new Error( + `git ls-files failed: ${error instanceof Error ? error.message : String(error)}`, + ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/repo-hygiene.js` around lines 199 - 209, The getTrackedPaths function currently swallows any git errors and returns an empty array, which can hide problems and make checks pass incorrectly; change the catch to surface the failure instead of returning [] — either rethrow the caught error or throw a new Error with context (e.g., include the original error message) so callers of getTrackedPaths see that git failed; locate the function getTrackedPaths and update its catch block to not return a silent empty array but to propagate/log and throw the error so repo-hygiene checks fail visibly when git is unavailable or repo metadata is broken.
231-234:⚠️ Potential issue | 🟠 Majorswitch .gitignore validation to line-based matching
lib/scripts/repo-hygiene.js:233(Line 233) uses substring matching. a line liketmp-special/can incorrectly satisfy the requiredtmpentry.proposed fix
- const missingPatterns = REQUIRED_GITIGNORE_PATTERNS.filter((pattern) => !gitignore.includes(pattern)); + const gitignoreLines = new Set( + gitignore + .split(/\r?\n/) + .map((line) => line.trim()) + .filter((line) => line.length > 0 && !line.startsWith("#")), + ); + const missingPatterns = REQUIRED_GITIGNORE_PATTERNS.filter((pattern) => !gitignoreLines.has(pattern));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/repo-hygiene.js` around lines 231 - 234, The current substring check using gitignore.includes(...) can produce false positives (e.g., "tmp-special/" matching required "tmp"), so change the validation to line-based matching: split the file content from gitignore into lines, trim each line and ignore empty lines and comment lines (starting with '#'), normalize both existing lines and REQUIRED_GITIGNORE_PATTERNS by removing trailing slashes before comparison, then build a Set of normalized lines and compute missingPatterns by checking which normalized required patterns are not present in that Set (use the variables gitignore, REQUIRED_GITIGNORE_PATTERNS and update missingPatterns logic in repo-hygiene.js accordingly).
62-69:⚠️ Potential issue | 🟡 Minorvalidate flag values before consuming argv next token
lib/scripts/repo-hygiene.js:64-69(Line 64, Line 68) accepts missing values and also accepts another flag as a value. this turns input errors into confusing downstream failures.proposed fix
if (arg === "--mode") { - args.mode = argv[i + 1] ?? ""; + const value = argv[i + 1]; + if (!value || value.startsWith("--")) { + throw new Error("--mode requires a value"); + } + args.mode = value; i += 1; } else if (arg === "--root") { - args.root = argv[i + 1] ?? ""; + const value = argv[i + 1]; + if (!value || value.startsWith("--")) { + throw new Error("--root requires a value"); + } + args.root = value; i += 1; } else if (arg === "--dry-run") {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/repo-hygiene.js` around lines 62 - 69, The flag-parsing loop accepts missing values or treats the next flag as a value for args.mode and args.root; update the loop that iterates argv to validate the next token before consuming it: in the for-loop handling "--mode" and "--root" check that argv[i+1] exists and does not start with "--" (or is not another known flag), otherwise throw or print a clear error and exit (or set a default), then only assign args.mode and args.root and increment i when the next token is valid; reference the existing argv loop and the args.mode / args.root assignments to locate where to add the validation.test/repo-hygiene.test.ts (2)
38-49:⚠️ Potential issue | 🟡 Minorguarantee fixture cleanup even when assertions fail
test/repo-hygiene.test.ts:48,test/repo-hygiene.test.ts:69,test/repo-hygiene.test.ts:83,test/repo-hygiene.test.ts:93, andtest/repo-hygiene.test.ts:104(Line 48, Line 69, Line 83, Line 93, Line 104) cleanup only runs on success. useafterEachor per-testtry/finallyso temp repos are always removed.as per coding guidelines, "tests must stay deterministic and use vitest."
Also applies to: 51-70, 72-84, 86-94, 96-105
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/repo-hygiene.test.ts` around lines 38 - 49, The tests create temporary repo fixtures with makeRepoFixture() and remove them with fs.rm(...) but cleanup only runs on success; change each test using runRepoHygiene(...) to guarantee removal by wrapping the per-test fixture lifecycle in a try/finally or move rm into a shared afterEach hook that deletes the fixture root, ensuring makeRepoFixture(), runRepoHygiene(), and any asserts all run inside the try and fs.rm(root, { recursive: true, force: true }) runs in finally (or implement afterEach that references the created root), and apply this pattern to all affected tests in test/repo-hygiene.test.ts so temp repos are always removed even if assertions fail.
31-35:⚠️ Potential issue | 🟠 Majoradd a windows/crlf regression case for tracked-path parsing
test/repo-hygiene.test.ts:31-35(Line 31) initializes fixtures but the suite still lacks a dedicated windows filesystem regression. add one case that exercisesgit ls-filesoutput with\r\nand validatescheckbehavior remains correct.as per coding guidelines, "tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/repo-hygiene.test.ts` around lines 31 - 35, Add a deterministic Vitest regression that simulates Windows CRLF git output for tracked-path parsing: in repo-hygiene.test.ts alongside makeRepoFixture, add a test which stubs the git invocation that the module uses (e.g., mock child_process.execFileSync or the internal helper that calls `git ls-files`) to return a CRLF-separated string like "file1.txt\r\nsub/dir/file2.txt\r\n", then call the exported check function (or the parsing function under test) and assert it correctly returns/handles the two paths; ensure the stub is only active for that test and is restored after to keep tests deterministic and isolated.
🤖 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/repo-hygiene.js`:
- Around line 152-174: collectCandidates currently deduplicates root-level
entries via the candidatePaths Set in addCandidate, but
collectNestedTempCandidates is adding nested candidates directly so duplicates
(same path) can be added twice; modify collectNestedTempCandidates to use the
same addCandidate/candidatePaths Set or accept addCandidate as a parameter so
all nested additions call addCandidate (or check candidatePaths before pushing)
ensuring collectCandidates, addCandidate, candidatePaths and
collectNestedTempCandidates share the same path-set to avoid processing the same
path twice.
---
Duplicate comments:
In `@scripts/repo-hygiene.js`:
- Around line 199-209: The getTrackedPaths function currently swallows any git
errors and returns an empty array, which can hide problems and make checks pass
incorrectly; change the catch to surface the failure instead of returning [] —
either rethrow the caught error or throw a new Error with context (e.g., include
the original error message) so callers of getTrackedPaths see that git failed;
locate the function getTrackedPaths and update its catch block to not return a
silent empty array but to propagate/log and throw the error so repo-hygiene
checks fail visibly when git is unavailable or repo metadata is broken.
- Around line 231-234: The current substring check using gitignore.includes(...)
can produce false positives (e.g., "tmp-special/" matching required "tmp"), so
change the validation to line-based matching: split the file content from
gitignore into lines, trim each line and ignore empty lines and comment lines
(starting with '#'), normalize both existing lines and
REQUIRED_GITIGNORE_PATTERNS by removing trailing slashes before comparison, then
build a Set of normalized lines and compute missingPatterns by checking which
normalized required patterns are not present in that Set (use the variables
gitignore, REQUIRED_GITIGNORE_PATTERNS and update missingPatterns logic in
repo-hygiene.js accordingly).
- Around line 62-69: The flag-parsing loop accepts missing values or treats the
next flag as a value for args.mode and args.root; update the loop that iterates
argv to validate the next token before consuming it: in the for-loop handling
"--mode" and "--root" check that argv[i+1] exists and does not start with "--"
(or is not another known flag), otherwise throw or print a clear error and exit
(or set a default), then only assign args.mode and args.root and increment i
when the next token is valid; reference the existing argv loop and the args.mode
/ args.root assignments to locate where to add the validation.
In `@test/repo-hygiene.test.ts`:
- Around line 38-49: The tests create temporary repo fixtures with
makeRepoFixture() and remove them with fs.rm(...) but cleanup only runs on
success; change each test using runRepoHygiene(...) to guarantee removal by
wrapping the per-test fixture lifecycle in a try/finally or move rm into a
shared afterEach hook that deletes the fixture root, ensuring makeRepoFixture(),
runRepoHygiene(), and any asserts all run inside the try and fs.rm(root, {
recursive: true, force: true }) runs in finally (or implement afterEach that
references the created root), and apply this pattern to all affected tests in
test/repo-hygiene.test.ts so temp repos are always removed even if assertions
fail.
- Around line 31-35: Add a deterministic Vitest regression that simulates
Windows CRLF git output for tracked-path parsing: in repo-hygiene.test.ts
alongside makeRepoFixture, add a test which stubs the git invocation that the
module uses (e.g., mock child_process.execFileSync or the internal helper that
calls `git ls-files`) to return a CRLF-separated string like
"file1.txt\r\nsub/dir/file2.txt\r\n", then call the exported check function (or
the parsing function under test) and assert it correctly returns/handles the two
paths; ensure the stub is only active for that test and is restored after to
keep tests deterministic and isolated.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
scripts/repo-hygiene.jstest/repo-hygiene.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/repo-hygiene.test.ts
🧬 Code graph analysis (1)
test/repo-hygiene.test.ts (1)
scripts/repo-hygiene.js (2)
args(50-55)args(250-250)
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
scripts/repo-hygiene.js (2)
207-221:⚠️ Potential issue | 🟠 Majordo not let
git ls-filesfailures degrade into a passing check.Line 207 in
lib/scripts/repo-hygiene.js:207returns[]on git failures, which can falsely pass tracked-scratch validation incheck.proposed fix
function getTrackedPaths(rootPath) { try { const output = execFileSync("git", ["ls-files"], { @@ }); return output.split(/\r?\n/).filter(Boolean); } catch (error) { - console.warn( - `repo-hygiene warning: git ls-files failed in getTrackedPaths: ${error instanceof Error ? error.message : String(error)}`, - ); - return []; + throw new Error( + `git ls-files failed in ${rootPath}: ${error instanceof Error ? error.message : String(error)}`, + ); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/repo-hygiene.js` around lines 207 - 221, The getTrackedPaths function currently swallows git errors and returns [] which masks failures; modify getTrackedPaths so it does not return an empty array on failure but instead rethrows the caught error (or throw a new Error with context) so the calling validation (check) fails hard; update the catch block in getTrackedPaths to throw the error (including a descriptive message and original error details) rather than console.warn/return [], ensuring callers like check will surface the failure.
137-158:⚠️ Potential issue | 🟡 Minornested candidate collection still bypasses dedupe.
Line 145 in
lib/scripts/repo-hygiene.js:145pushes directly intocandidates, so paths can be processed twice (root scan + nested scan) when names match both collectors.proposed fix
-async function collectNestedTempCandidates(rootPath, dirPath, candidates) { +async function collectNestedTempCandidates(rootPath, dirPath, addCandidate) { const entries = await fs.readdir(dirPath, { withFileTypes: true }); for (const entry of entries) { const entryPath = path.join(dirPath, entry.name); @@ if (isDeepTempCandidate(entry.name, entry.isDirectory())) { - candidates.push({ - name: path.relative(rootPath, entryPath).replaceAll("\\", "/"), - path: entryPath, - isDirectory: entry.isDirectory(), - }); + addCandidate( + path.relative(rootPath, entryPath).replaceAll("\\", "/"), + entryPath, + entry.isDirectory(), + ); if (entry.isDirectory()) { continue; } } if (entry.isDirectory()) { - await collectNestedTempCandidates(rootPath, entryPath, candidates); + await collectNestedTempCandidates(rootPath, entryPath, addCandidate); } } } @@ - await collectNestedTempCandidates(rootPath, rootPath, candidates); + await collectNestedTempCandidates(rootPath, rootPath, addCandidate);Also applies to: 160-182
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/repo-hygiene.js` around lines 137 - 158, collectNestedTempCandidates currently pushes entries directly into candidates, causing duplicates when the same relative path was already added by the root scan; fix by deduplicating before push: compute the relative name (as currently done), maintain a seen set (e.g., seenCandidateNames) or check candidates for an existing entry with the same name, and only push if not already present; apply the same dedupe check to any other nested collector functions that add to the same candidates array.
🤖 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/repo-hygiene.js`:
- Around line 62-81: The CLI currently ignores unrecognized flags in the argv
parsing loop, which can let typos like "--roo" silently fall through; update the
for-loop that reads argv (the block that sets args.mode, args.root, args.dryRun)
to detect any argument that starts with "--" but isn't one of the handled flags
and immediately throw an Error (e.g. "Unknown flag: <flag>") instead of ignoring
it; reference the existing symbols argv, args.mode, args.root, args.dryRun and
the argv parsing loop to add this validation so unknown --flags are rejected.
In `@test/repo-hygiene.test.ts`:
- Around line 117-129: Add a deterministic regression test that verifies Windows
backslash separators are normalized by the tracked-scratch detection: in
test/repo-hygiene.test.ts add a case similar to the CRLF test but create a
nested file (e.g., path.join(root, "nested", "task_plan.md")), ensure git tracks
it, set core.autocrlf as in the existing test, run
runRepoHygiene(["check","--root", root]) and assert the stderr contains the
expected nested filename using a Windows-style separator (backslash) to ensure
the logic in lib/scripts/repo-hygiene.js (candidate naming normalization around
the code at the former line 146) correctly handles '\' separators. Ensure the
test is vitest-compatible and deterministic (create the nested directory, write
the file, add/commit as needed) so the regression is locked down.
In `@test/stream-failover.test.ts`:
- Around line 52-67: Add a deterministic race-regression test that forces the
timeout and a read failure to happen concurrently to ensure
withStreamingFailover only triggers one failover: create a test using
vi.useFakeTimers() and a fallback spy (vi.fn) plus the existing helpers
makeStallingResponse() and makeSseResponse() to craft a response that will both
throw on read and hit the stallTimeoutMs at the same instant, call
response.text(), advance timers with vi.advanceTimersByTimeAsync(...) to create
the race, then await the text and assert the fallback spy was called exactly
once and that the failover marker emitted by withStreamingFailover appears
exactly once in the resulting text (e.g., contains "codex-multi-auth failover
1"). Ensure the test is fully deterministic under vitest fake timers and mirrors
the existing "switches to fallback stream when primary stalls" structure so it
repros the concurrency bug covered by lib/request/stream-failover.ts handling.
---
Duplicate comments:
In `@scripts/repo-hygiene.js`:
- Around line 207-221: The getTrackedPaths function currently swallows git
errors and returns [] which masks failures; modify getTrackedPaths so it does
not return an empty array on failure but instead rethrows the caught error (or
throw a new Error with context) so the calling validation (check) fails hard;
update the catch block in getTrackedPaths to throw the error (including a
descriptive message and original error details) rather than console.warn/return
[], ensuring callers like check will surface the failure.
- Around line 137-158: collectNestedTempCandidates currently pushes entries
directly into candidates, causing duplicates when the same relative path was
already added by the root scan; fix by deduplicating before push: compute the
relative name (as currently done), maintain a seen set (e.g.,
seenCandidateNames) or check candidates for an existing entry with the same
name, and only push if not already present; apply the same dedupe check to any
other nested collector functions that add to the same candidates array.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
scripts/repo-hygiene.jstest/repo-hygiene.test.tstest/stream-failover.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/stream-failover.test.tstest/repo-hygiene.test.ts
🧬 Code graph analysis (2)
scripts/repo-hygiene.js (1)
scripts/install-codex-auth.js (1)
dryRun(31-31)
test/stream-failover.test.ts (1)
lib/request/stream-failover.ts (1)
withStreamingFailover(115-234)
🔇 Additional comments (2)
test/stream-failover.test.ts (1)
38-40: timer restoration is clean and deterministic.
test/stream-failover.test.ts:38centralizesvi.useRealTimers()inafterEach, which keeps fake-timer state isolated across tests.test/repo-hygiene.test.ts (1)
40-46: fixture teardown is now failure-safe.
test/repo-hygiene.test.ts:40guarantees temp repo cleanup even when assertions fail.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
scripts/repo-hygiene.js (1)
62-81:⚠️ Potential issue | 🟠 Majorreject unknown args instead of silently ignoring them.
lib/scripts/repo-hygiene.js:62-81ignores unexpected flags/positionals. a typo like--roofalls back to default root and can run cleanup in the wrong repo.proposed fix
} else if (arg === "--dry-run") { args.dryRun = true; + } else if (arg.startsWith("--")) { + throw new Error(`Unknown flag: ${arg}`); + } else { + throw new Error(`Unexpected positional argument: ${arg}`); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/repo-hygiene.js` around lines 62 - 81, The argument parsing loop currently ignores unknown flags which allows typos to be silently accepted; update the for-loop that iterates argv (the block handling "--mode", "--root", "--dry-run") to reject any unrecognized argument by throwing an Error (e.g., "Unknown argument: <arg>") instead of falling through, and ensure you still advance the index for recognized flags (args.mode, args.root, args.dryRun) while not swallowing or misinterpreting stray positionals or malformed flags.test/repo-hygiene.test.ts (1)
117-129:⚠️ Potential issue | 🟠 Majoradd a windows separator regression, not just crlf coverage.
test/repo-hygiene.test.ts:117-129only verifies crlf parsing and root-level filename matching. it still does not lock down windows\separator behavior for nested tracked scratch paths consumed bylib/scripts/repo-hygiene.js:225-231.proposed deterministic regression test
+ it("detects nested tracked scratch files with windows-style separators", async () => { + fixtureRoot = makeRepoFixture(); + const root = fixtureRoot; + await fs.writeFile(path.join(root, ".gitignore"), requiredGitignore.replaceAll("\n", "\r\n")); + await fs.mkdir(path.join(root, "nested"), { recursive: true }); + await fs.writeFile(path.join(root, "nested", "task_plan.md"), "temp plan\r\n"); + execFileSync("git", ["config", "core.autocrlf", "true"], { cwd: root, stdio: "ignore" }); + execFileSync("git", ["add", ".gitignore"], { cwd: root, stdio: "ignore" }); + execFileSync("git", ["add", "-f", path.join("nested", "task_plan.md")], { cwd: root, stdio: "ignore" }); + + const result = runRepoHygiene(["check", "--root", root]); + expect(result.status).toBe(1); + expect(result.stderr).toMatch(/nested[\\/]+task_plan\.md/); + });As per coding guidelines
test/**:tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/repo-hygiene.test.ts` around lines 117 - 129, Add a deterministic regression test in test/repo-hygiene.test.ts that covers Windows path-separator behavior for nested tracked scratch files: create a nested directory (e.g., "nested/path/task_plan.md" using path.join), write and git-add that file, then run runRepoHygiene(["check", "--root", root]) and assert the tool returns non-zero and that stderr contains the nested file name regardless of separator; specifically ensure the assertion accepts both forward-slash and backslash variants (or normalize separators before asserting) to validate lib/scripts/repo-hygiene.js (the code around lines 225-231) correctly handles backslash-separated git ls-files output.
🤖 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/repo-hygiene.js`:
- Around line 225-231: The current detection loop using tracked.has(name) misses
nested or Windows-style paths; update the logic around getTrackedPaths(rootPath)
/ tracked / TRACKED_SCRATCH_FILES to normalize and compare paths instead of
exact string equality: normalize all tracked paths and scratch patterns to a
consistent separator (e.g., replace backslashes with forward slashes or use
path.posix.normalize), then detect violations by checking if any normalized
tracked path endsWith the normalized scratch entry (or matches via path.relative
=== '' or only filename match) and push those into trackedViolations; ensure you
update the comparison in the loop that references tracked,
TRACKED_SCRATCH_FILES, and getTrackedPaths so nested paths like
"nested/task_plan.md" are flagged.
In `@test/repo-hygiene.test.ts`:
- Around line 37-144: Add a deterministic regression test that runs repo-hygiene
"check" and "clean" concurrently against the same fixture to exercise the race
windows in the repo-hygiene handlers; use the existing helpers makeRepoFixture,
requiredGitignore and runRepoHygiene to create a fixture root, write .gitignore,
then launch both runRepoHygiene([... "check", "--root", root]) and
runRepoHygiene([... "clean", "--mode", "aggressive", "--root", root]) in
parallel via Promise.all, wait for both results, assert their exit codes and
that the final filesystem state is consistent (expected files
removed/preserved), and ensure determinism by using the fixture sentinel files
(create known files like task_plan.md and a temporary audit file) so the overlap
is reproducible and the assertions validate no intermittent failures.
- Around line 136-140: The test mutates process.env.PATH directly and restores
it only after calling runRepoHygiene, risking leakage if runRepoHygiene throws;
wrap the mutation and invocation in a try/finally so originalPath is always
restored — e.g., save originalPath, set process.env.PATH = "", call
runRepoHygiene(["check","--root", root]) inside try, and restore
process.env.PATH = originalPath in finally (reference runRepoHygiene and the
test block in test/repo-hygiene.test.ts).
---
Duplicate comments:
In `@scripts/repo-hygiene.js`:
- Around line 62-81: The argument parsing loop currently ignores unknown flags
which allows typos to be silently accepted; update the for-loop that iterates
argv (the block handling "--mode", "--root", "--dry-run") to reject any
unrecognized argument by throwing an Error (e.g., "Unknown argument: <arg>")
instead of falling through, and ensure you still advance the index for
recognized flags (args.mode, args.root, args.dryRun) while not swallowing or
misinterpreting stray positionals or malformed flags.
In `@test/repo-hygiene.test.ts`:
- Around line 117-129: Add a deterministic regression test in
test/repo-hygiene.test.ts that covers Windows path-separator behavior for nested
tracked scratch files: create a nested directory (e.g.,
"nested/path/task_plan.md" using path.join), write and git-add that file, then
run runRepoHygiene(["check", "--root", root]) and assert the tool returns
non-zero and that stderr contains the nested file name regardless of separator;
specifically ensure the assertion accepts both forward-slash and backslash
variants (or normalize separators before asserting) to validate
lib/scripts/repo-hygiene.js (the code around lines 225-231) correctly handles
backslash-separated git ls-files output.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (2)
scripts/repo-hygiene.jstest/repo-hygiene.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/repo-hygiene.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/repo-hygiene.test.ts`:
- Line 175: The variable `result` is declared without a type causing an
implicit-any lint error; change its declaration to use an explicit type like
`let result: ReturnType<typeof runRepoHygiene>` and update any subsequent
accesses/assertions to use optional chaining (e.g., `result?.someProp`) to be
defensive against null/undefined; locate `result` in the test and the
`runRepoHygiene` call to ensure the type aligns with the function's return shape
and adjust assertions accordingly.
ℹ️ Review info
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to data retention organization setting
📒 Files selected for processing (3)
scripts/repo-hygiene.jstest/repo-hygiene.test.tstest/stream-failover.test.ts
📜 Review details
🧰 Additional context used
📓 Path-based instructions (1)
test/**
⚙️ CodeRabbit configuration file
tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior. reject changes that mock real secrets or skip assertions.
Files:
test/stream-failover.test.tstest/repo-hygiene.test.ts
🧬 Code graph analysis (2)
test/stream-failover.test.ts (1)
lib/request/stream-failover.ts (1)
withStreamingFailover(115-234)
test/repo-hygiene.test.ts (1)
scripts/repo-hygiene.js (2)
args(50-55)args(279-279)
🪛 Biome (2.4.4)
test/repo-hygiene.test.ts
[error] 175-175: This variable implicitly has the any type.
(lint/suspicious/noImplicitAnyLet)
🔇 Additional comments (5)
test/stream-failover.test.ts (1)
117-149: good deterministic concurrency regression coverage.the new race test at
test/stream-failover.test.ts:117-149locks down the timeout/read-error overlap and asserts single failover behavior clearly.As per coding guidelines
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.scripts/repo-hygiene.js (2)
62-84: argument parsing hardening looks correct.rejecting unknown flags/extra positionals in
lib/scripts/repo-hygiene.js:62-84is the right safety guard for destructive commands.
229-267: tracked-scratch and gitignore validation are now robust.the normalization + basename check and line-based pattern matching in
lib/scripts/repo-hygiene.js:229-267correctly closes prior false-negative/false-positive gaps.test/repo-hygiene.test.ts (2)
152-167: windows path-separator regression is solid.the nested scratch assertion in
test/repo-hygiene.test.ts:152-167correctly validates separator-agnostic detection.As per coding guidelines
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
187-206: deterministic concurrent check/clean coverage is well added.the parallel execution case in
test/repo-hygiene.test.ts:187-206is deterministic and exercises the race window directly.As per coding guidelines
test/**: tests must stay deterministic and use vitest. demand regression cases that reproduce concurrency bugs, token refresh races, and windows filesystem behavior.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
@greptile review |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| console.log(`[dry-run] delete ${candidate.name}`); | ||
| continue; | ||
| } | ||
| await fs.rm(candidate.path, { recursive: true, force: true }); |
There was a problem hiding this comment.
missing retry logic for windows filesystem locks (EBUSY, EPERM, ENOTEMPTY). antivirus can lock files during deletion causing this to fail. test/rotation-integration.test.ts already implements removeWithRetry for this exact issue - same pattern needed here.
| await fs.rm(candidate.path, { recursive: true, force: true }); | |
| const retryableCodes = new Set(["ENOTEMPTY", "EPERM", "EBUSY"]); | |
| const maxAttempts = 6; | |
| for (let attempt = 1; attempt <= maxAttempts; attempt += 1) { | |
| try { | |
| await fs.rm(candidate.path, { recursive: true, force: true }); | |
| console.log(`deleted ${candidate.name}`); | |
| break; | |
| } catch (error) { | |
| const maybeCode = error instanceof Error && "code" in error ? (error as { code?: string }).code : undefined; | |
| const shouldRetry = maybeCode !== undefined && retryableCodes.has(maybeCode); | |
| if (!shouldRetry || attempt === maxAttempts) { | |
| throw error; | |
| } | |
| await new Promise((resolve) => setTimeout(resolve, attempt * 25)); | |
| } | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: scripts/repo-hygiene.js
Line: 206
Comment:
missing retry logic for windows filesystem locks (EBUSY, EPERM, ENOTEMPTY). antivirus can lock files during deletion causing this to fail. `test/rotation-integration.test.ts` already implements `removeWithRetry` for this exact issue - same pattern needed here.
```suggestion
const retryableCodes = new Set(["ENOTEMPTY", "EPERM", "EBUSY"]);
const maxAttempts = 6;
for (let attempt = 1; attempt <= maxAttempts; attempt += 1) {
try {
await fs.rm(candidate.path, { recursive: true, force: true });
console.log(`deleted ${candidate.name}`);
break;
} catch (error) {
const maybeCode = error instanceof Error && "code" in error ? (error as { code?: string }).code : undefined;
const shouldRetry = maybeCode !== undefined && retryableCodes.has(maybeCode);
if (!shouldRetry || attempt === maxAttempts) {
throw error;
}
await new Promise((resolve) => setTimeout(resolve, attempt * 25));
}
}
```
How can I resolve this? If you propose a fix, please make it concise.
| if (!fixtureRoot) { | ||
| return; | ||
| } | ||
| await fs.rm(fixtureRoot, { recursive: true, force: true }); |
There was a problem hiding this comment.
test fixture cleanup uses bare fs.rm without retry logic. on windows this can fail with EBUSY/EPERM when antivirus locks the temp directory. use the same removeWithRetry pattern from test/rotation-integration.test.ts
Prompt To Fix With AI
This is a comment left during a code review.
Path: test/repo-hygiene.test.ts
Line: 65
Comment:
test fixture cleanup uses bare `fs.rm` without retry logic. on windows this can fail with EBUSY/EPERM when antivirus locks the temp directory. use the same `removeWithRetry` pattern from `test/rotation-integration.test.ts`
How can I resolve this? If you propose a fix, please make it concise.
|
Superseded by #14 — all changes from this PR have been merged into the unified supersede PR, with additional Windows filesystem safety fixes applied. |
1 participant
Summary
scripts/repo-hygiene.jsclean:repo:check)test-results.mdtest/repo-hygiene.test.tsstream-failovertestsWhy
The repo had significant local trash/temp drift patterns (
.tmp*,tmp*, generated/local state dirs). This change makes cleanup explicit and auditable, then blocks regressions in CI.What Changed
scripts/repo-hygiene.jsclean --mode aggressive [--root] [--dry-run]check [--root].gitignorehygiene patterns and blocks tracked scratch filespackage.jsonclean:repoclean:repo:check.github/workflows/ci.ymlRepository hygiene checkstep.gitignoretest-results.mdtest/repo-hygiene.test.tstest/stream-failover.test.tsvitest.config.ts.tmp*,.omx,.opencode,.sisyphus,.history, coverage variants)test-results.mdAudit Evidence
Worktree
origin/main@0636840chore/repo-hygiene-quality-speed5b9cad2Local aggressive cleanup executed
.tmp*/tmp*files/dirs and generated/local dirs including.omx,.opencode,.sisyphus,coverage,dist,node_modules, plus local scratch docs (task_plan.md,findings.md,progress.md,test-results.md)Validation
npm run clean:repo:check✅npm run typecheck✅npm run lint✅npm test✅ (85 files, 2002 tests)npm run build✅Runtime snapshot
npm testwall ~9.34snpm testwall ~8.39sRisk
note: greptile review for oc-chatgpt-multi-auth. cite files like
lib/foo.ts:123. confirm regression tests + windows concurrency/token redaction coverage.Greptile Summary
adds deterministic repository hygiene tooling with CI enforcement, removes tracked scratch artifacts, and improves test reliability/speed.
key changes:
scripts/repo-hygiene.jsprovidesclean --mode aggressiveandcheckcommands with allowlisted cleanup targeting.tmp*,tmp*, generated dirs (coverage, dist, node_modules), and scratch docsnpm run clean:repo:checkbefore security audit to block tracked scratch files and validate required.gitignorepatternsstream-failover.test.tsreplaces timeout-dependent assertions with fake-timer deterministic flow, reducing runtime from ~9.34s to ~8.39srotation-integration.test.tsaddsremoveWithRetryhelper with exponential backoff to handle windows filesystem lock races (ENOTEMPTY/EPERM/EBUSY).tmp*,.omx,.opencode,.sisyphus,.history)critical issue:
scripts/repo-hygiene.jsline 206 uses barefs.rmwithout retry logic for windows filesystem locks. antivirus can hold EBUSY/EPERM/ENOTEMPTY locks during deletion causing the script to fail. the same PR addsremoveWithRetryintest/rotation-integration.test.tsfor this exact race condition - inconsistent pattern.Confidence Score: 3/5
scripts/repo-hygiene.jsfs.rm calls. windows users (primary audience per custom rule 637a42e6) will hit antivirus lock failures during cleanup. PR already demonstrates awareness of this pattern viaremoveWithRetryin rotation-integration.test.ts but doesn't apply it consistently to the hygiene script itself.scripts/repo-hygiene.jsneeds windows filesystem retry logic before merge to prevent antivirus lock failures in production cleanupImportant Files Changed
removeWithRetryto handle windows filesystem lock race conditions (ENOTEMPTY/EPERM/EBUSY) with exponential backoff during temp directory cleanupLast reviewed commit: 93f069e
Context used:
dashboard- What: Every code change must explain how it defends against Windows filesystem concurrency bugs and ... (source)