You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Summary: This PR adds debug logging (gated on ROSETTA_DEBUG=1) to the lint-format-advisory hook's run() function and substantially expands unit test coverage with 6 targeted gap-filling test groups (gaps A, B, C, D, E, H4) addressing case-insensitive extension matching, exclusion boundary precision, MultiEdit tool, file-path field fallback chain, Codex apply_patch, and null-session throttle deduplication.
Findings:
Empty PR body: No motivation or context is provided. A brief description of which gaps were identified and why the debug log was added would help reviewers and future bisects.
Ambiguous inline comment: The comment // Logged via debugLog (gated on ROSETTA_DEBUG=1); .cursor/hooks.json sets it. is unclear — it implies .cursor/hooks.json sets ROSETTA_DEBUG=1 by default, which is surprising. If this is intentional, state it explicitly; otherwise remove the claim to avoid confusion.
Date.now() in gap H4: The throttle test uses Date.now() to generate a unique path and avoid lock collisions. This is reasonable but could be brittle if tests run concurrently. A short comment noting the isolation intent would make it self-documenting.
No assertion on debugLog output: The new debugLog call in the source is untested (debug side effects are acceptable to leave unasserted, but worth noting for completeness).
Good gap labeling: All 6 test groups include clear inline comments explaining which coverage gap they address. This is helpful for future maintainers.
Suggestions:
Fill in the PR body: e.g., "Identified test coverage gaps in lint-format-advisory via static analysis. Adds a debug log and fills gaps A/B/C/D/E/H4."
Clarify or remove the .cursor/hooks.json sets it remark in the inline comment.
Summary: This PR adds a debugLog instrumentation call to the lint-format-advisory hook's run() function (gated behind ROSETTA_DEBUG=1, zero production impact) and significantly expands test coverage with 7 new suites targeting previously-unexercised code paths.
Findings:
PR description is empty. The PR body contains no explanation of what changed or why. Adding even 2–3 sentences (motivation, what paths are now covered) makes review faster and the git history more useful.
debugLog call itself is not covered by tests. The new tests validate the advisory output only; the ROSETTA_DEBUG=1 path (where the log actually writes) is never exercised. Consider adding a test that sets the env var and either mocks appendFileSync or asserts the log file is written when the hook fires. Without it, a typo inside debugLog would not be caught.
Dist file committed alongside source.src/hooks/dist/src/hooks/lint-format-advisory.js is included in the diff. If committing compiled output is the project convention (it appears to be), this is fine — but ensure CI regenerates and validates it so the dist never drifts from source.
Comment verbosity in run(). The two-line inline comment (// run() is reached only after all gating...Logged via debugLog...hooks.json sets it.) is above the usual bar for inline comments in this file. The first sentence is the non-obvious invariant (worth keeping); the second sentence (Logged via debugLog...hooks.json sets it.) describes what the code does and could be dropped.
Positives:
Strong coverage of genuinely missed edge cases: case-insensitive extension matching, exclusion-boundary near-misses, MultiEdit tool, filePath/path field fallback, Codex apply_patch, and session-less throttle dedup.
Each test suite has a concise block comment explaining the invariant under test and why it was previously uncovered — exactly the right level of documentation.
Debug instrumentation is safely no-op unless ROSETTA_DEBUG=1; no behavior change in production.
Throttle test correctly uses Date.now()-suffixed path with explanation to prevent cross-run key collisions.
Suggestions:
Add at minimum one test that spies on debugLog (or appendFileSync) and asserts it is called when the hook fires, to lock in the new instrumentation behavior.
Fill in the PR description before merge — even a short paragraph.
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
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.
No description provided.