Conversation
|
Warning Ignoring CodeRabbit configuration file changes. For security, only the configuration from the base branch is applied for open source repositories. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughAdds a hooks system: new source hooks in Changes
Sequence Diagram(s)mermaid Editor->>HookRunner: write/edit file (tool event) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
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
There was a problem hiding this comment.
Pull request overview
Adds automatic hook-based guardrails for the Doc Detective Claude plugin and Gemini extension to reduce common spec-format mistakes, validate/format specs, and surface install/test reminders; also bumps the package/plugin version to 1.3.0.
Changes:
- Add hook scripts (pre-write blocker, post-edit validators/formatters/warnings, session-start install check) and hook config files for Claude + Gemini.
- Update build pipeline to sync hooks into build artifacts and plugin output.
- Bump version metadata across skills/agents/plugin/extension and document hook behavior.
Reviewed changes
Copilot reviewed 61 out of 61 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/skills/doc-detective-validate/SKILL.md | Bump skill metadata version to 1.3.0 |
| src/skills/doc-detective-test/SKILL.md | Bump skill metadata version to 1.3.0 |
| src/skills/doc-detective-project-bootstrap/SKILL.md | Bump skill metadata version to 1.3.0 |
| src/skills/doc-detective-install-github-action/SKILL.md | Bump skill metadata version to 1.3.0 |
| src/skills/doc-detective-inline-test-injection/SKILL.md | Bump skill metadata version to 1.3.0 |
| src/skills/doc-detective-inject/SKILL.md | Bump skill metadata version to 1.3.0 |
| src/skills/doc-detective-init/SKILL.md | Bump skill metadata version to 1.3.0 |
| src/skills/doc-detective-generate/SKILL.md | Bump skill metadata version to 1.3.0 |
| src/skills/doc-detective-doc-testing/SKILL.md | Bump skill metadata version to 1.3.0 |
| src/hooks/scripts/session-start-check-install.sh | Add session-start CLI availability check hook script |
| src/hooks/scripts/pre-edit-block-action-antipattern.js | Add pre-write blocker for {"action":"..."} anti-pattern |
| src/hooks/scripts/post-edit-warn-inline-tests.js | Add warning hook for edits to docs with inline tests |
| src/hooks/scripts/post-edit-validate-test-spec.js | Add post-edit auto-validation hook for JSON test specs |
| src/hooks/scripts/post-edit-suggest-testing.js | Add doc-edit reminder hook when Doc Detective config exists |
| src/hooks/scripts/post-edit-format-test-spec.js | Add post-edit JSON formatting hook for test specs |
| src/hooks/gemini-hooks.json | Add Gemini hook configuration wiring to scripts |
| src/hooks/claude-hooks.json | Add Claude hook configuration wiring to scripts |
| src/agents/doc-detective-specialist.md | Bump agent metadata version to 1.3.0 |
| skills/doc-detective-validate/SKILL.md | Bump artifact skill metadata version to 1.3.0 |
| skills/doc-detective-test/SKILL.md | Bump artifact skill metadata version to 1.3.0 |
| skills/doc-detective-project-bootstrap/SKILL.md | Bump artifact skill metadata version to 1.3.0 |
| skills/doc-detective-install-github-action/SKILL.md | Bump artifact skill metadata version to 1.3.0 |
| skills/doc-detective-inline-test-injection/SKILL.md | Bump artifact skill metadata version to 1.3.0 |
| skills/doc-detective-inject/SKILL.md | Bump artifact skill metadata version to 1.3.0 |
| skills/doc-detective-init/SKILL.md | Bump artifact skill metadata version to 1.3.0 |
| skills/doc-detective-generate/SKILL.md | Bump artifact skill metadata version to 1.3.0 |
| skills/doc-detective-doc-testing/SKILL.md | Bump artifact skill metadata version to 1.3.0 |
| plugins/doc-detective/skills/doc-detective-validate/SKILL.md | Bump plugin skill metadata version to 1.3.0 |
| plugins/doc-detective/skills/doc-detective-test/SKILL.md | Bump plugin skill metadata version to 1.3.0 |
| plugins/doc-detective/skills/doc-detective-project-bootstrap/SKILL.md | Bump plugin skill metadata version to 1.3.0 |
| plugins/doc-detective/skills/doc-detective-install-github-action/SKILL.md | Bump plugin skill metadata version to 1.3.0 |
| plugins/doc-detective/skills/doc-detective-inline-test-injection/SKILL.md | Bump plugin skill metadata version to 1.3.0 |
| plugins/doc-detective/skills/doc-detective-inject/SKILL.md | Bump plugin skill metadata version to 1.3.0 |
| plugins/doc-detective/skills/doc-detective-init/SKILL.md | Bump plugin skill metadata version to 1.3.0 |
| plugins/doc-detective/skills/doc-detective-generate/SKILL.md | Bump plugin skill metadata version to 1.3.0 |
| plugins/doc-detective/skills/doc-detective-doc-testing/SKILL.md | Bump plugin skill metadata version to 1.3.0 |
| plugins/doc-detective/hooks/scripts/session-start-check-install.sh | Add plugin-distributed session-start install check script |
| plugins/doc-detective/hooks/scripts/pre-edit-block-action-antipattern.js | Add plugin-distributed pre-write anti-pattern blocker |
| plugins/doc-detective/hooks/scripts/post-edit-warn-inline-tests.js | Add plugin-distributed inline-test warning hook |
| plugins/doc-detective/hooks/scripts/post-edit-validate-test-spec.js | Add plugin-distributed post-edit spec validator hook |
| plugins/doc-detective/hooks/scripts/post-edit-suggest-testing.js | Add plugin-distributed doc-edit test reminder hook |
| plugins/doc-detective/hooks/scripts/post-edit-format-test-spec.js | Add plugin-distributed post-edit spec formatter hook |
| plugins/doc-detective/hooks/hooks.json | Add Claude plugin hook config (renamed from claude-hooks.json in build) |
| plugins/doc-detective/agents/doc-detective-specialist.md | Bump plugin agent metadata version to 1.3.0 |
| plugins/doc-detective/.claude-plugin/plugin.json | Bump plugin.json version to 1.3.0 |
| package.json | Bump package version to 1.3.0 |
| hooks/scripts/session-start-check-install.sh | Add root artifact session-start install check script |
| hooks/scripts/pre-edit-block-action-antipattern.js | Add root artifact pre-write anti-pattern blocker |
| hooks/scripts/post-edit-warn-inline-tests.js | Add root artifact inline-test warning hook |
| hooks/scripts/post-edit-validate-test-spec.js | Add root artifact post-edit spec validator hook |
| hooks/scripts/post-edit-suggest-testing.js | Add root artifact doc-edit test reminder hook |
| hooks/scripts/post-edit-format-test-spec.js | Add root artifact post-edit spec formatter hook |
| hooks/gemini-hooks.json | Add root artifact Gemini hook config |
| hooks/claude-hooks.json | Add root artifact Claude hook config |
| gemini-extension.json | Add hooksFile reference and bump version to 1.3.0 |
| build.js | Add build sync step to copy hooks into artifacts/plugin and set executable bits |
| agents/doc-detective-specialist.md | Bump artifact agent metadata version to 1.3.0 |
| README.md | Document hooks usage and add hooks directory to install steps |
| GEMINI.md | Document hooks behavior in Gemini extension context |
| .husky/pre-commit | Include hooks/ in pre-commit build artifacts to stage |
| .claude-plugin/marketplace.json | Bump marketplace metadata version to 1.3.0 |
Comments suppressed due to low confidence (4)
src/hooks/scripts/session-start-check-install.sh:1
- The session-start hook invokes
timeoutandnpx. (1)timeoutis not guaranteed to exist in all environments (e.g., macOS often lacks GNUtimeout), which can produce noisy stderr or break the check; and (2)npx doc-detective --versionmay trigger a network fetch at session start, which is likely to create the “high-traffic” errors this PR aims to avoid. Prefer a no-network availability check (e.g.,npx --no-install .../npm exec --no -- ...) and guardtimeoutusage (or implement a portable timeout strategy) so the hook stays quiet and deterministic.
src/hooks/scripts/pre-edit-block-action-antipattern.js:1 - This blocker runs for common documentation extensions (e.g.,
.md) and will also match/abort when writing docs that describe the anti-pattern (like README snippets showing{"action":"goTo"}as an incorrect example). That makes legitimate documentation edits fail. To avoid false positives, narrow the check to actual test spec files (e.g.,.jsonthat looks like a Doc Detective spec) and/or only scan inline-test regions rather than the entire doc content.
src/hooks/scripts/pre-edit-block-action-antipattern.js:1 - This blocker runs for common documentation extensions (e.g.,
.md) and will also match/abort when writing docs that describe the anti-pattern (like README snippets showing{"action":"goTo"}as an incorrect example). That makes legitimate documentation edits fail. To avoid false positives, narrow the check to actual test spec files (e.g.,.jsonthat looks like a Doc Detective spec) and/or only scan inline-test regions rather than the entire doc content.
src/hooks/scripts/post-edit-validate-test-spec.js:1 - The hook ignores
stderrand only reports failures whenerr.code === 1. If the validator crashes, times out, or can’t be executed, the hook becomes silent (noadditionalContext) and loses the actionable error details. Consider surfacing unexpected errors (includingstderrand timeout cases) inadditionalContextso users can distinguish “invalid spec” from “validator failed to run.”
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
hooks/scripts/pre-edit-block-action-antipattern.js (1)
48-60:⚠️ Potential issue | 🟠 MajorSame false-positive blocker risk as the mirrored plugin script.
This copy has the same over-broad extension gating and can block valid docs edits when anti-pattern text appears as documentation content.
🧹 Nitpick comments (2)
src/hooks/scripts/session-start-check-install.sh (1)
20-26: Doublenpxinvocation adds unnecessary latency.The version is fetched twice: once to check availability (line 21) and again to capture it (line 22). This doubles the wait time (up to 16s) on the npx path.
♻️ Suggested improvement
# Check 2: npx if [ "$AVAILABLE" = "false" ] && command -v npx &>/dev/null; then - if timeout 8 npx doc-detective --version &>/dev/null 2>&1; then - VERSION=$(timeout 8 npx doc-detective --version 2>/dev/null || echo "unknown") + VERSION=$(timeout 8 npx doc-detective --version 2>/dev/null) + if [ $? -eq 0 ] && [ -n "$VERSION" ]; then STATUS="doc-detective available via npx (version: ${VERSION})" AVAILABLE="true" fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/scripts/session-start-check-install.sh` around lines 20 - 26, Replace the double npx invocation with a single call that captures the output and exit status: run timeout 8 npx doc-detective --version once, store its stdout (or "unknown" on failure) into VERSION and check the command's exit code to set AVAILABLE="true" and STATUS="doc-detective available via npx (version: ${VERSION})" only when the command succeeded; reference the existing variables and constructs (AVAILABLE, VERSION, STATUS, timeout, npx doc-detective --version) so you avoid calling npx twice and eliminate the extra latency.plugins/doc-detective/hooks/scripts/session-start-check-install.sh (1)
20-26: Minor inefficiency:npx doc-detective --versionruns twice.The condition on line 21 runs the command to check success, then line 22 runs it again to capture the version. This doubles the 8-second potential wait time.
♻️ Suggested optimization to run npx once
# Check 2: npx if [ "$AVAILABLE" = "false" ] && command -v npx &>/dev/null; then - if timeout 8 npx doc-detective --version &>/dev/null 2>&1; then - VERSION=$(timeout 8 npx doc-detective --version 2>/dev/null || echo "unknown") + VERSION=$(timeout 8 npx doc-detective --version 2>/dev/null) + if [ $? -eq 0 ] && [ -n "$VERSION" ]; then STATUS="doc-detective available via npx (version: ${VERSION})" AVAILABLE="true" + else + VERSION="" fi fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@plugins/doc-detective/hooks/scripts/session-start-check-install.sh` around lines 20 - 26, Replace the two separate invocations of "timeout 8 npx doc-detective --version" with a single invocation whose output and exit status are captured once: run the command into a variable (e.g., capture output into a temp variable), test that the command succeeded by checking its exit status or non-empty output, then set VERSION from that captured output (falling back to "unknown" if empty) and set STATUS and AVAILABLE accordingly; update the block where AVAILABLE, VERSION, STATUS and the "npx doc-detective --version" calls appear so the command is executed only once.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.husky/pre-commit:
- Line 2: The pre-commit hook currently stages generated hook outputs but omits
the source directory in the git add command; update the git add invocation (the
line containing "git add agents/ skills/ hooks/ commands/ plugins/doc-detective/
.claude-plugin/marketplace.json gemini-extension.json") to also stage the source
hooks directory (e.g., add src/hooks/ or src/hooks/**) so that source-of-truth
changes are committed alongside generated artifacts.
In `@plugins/doc-detective/hooks/scripts/pre-edit-block-action-antipattern.js`:
- Around line 48-60: The rule is too broad because it inspects text formats and
blocks legitimate docs that contain JSON-like examples; restrict the check so
ACTION_PATTERN matching only runs on actual Doc Detective data files (e.g., JSON
payloads) rather than Markdown/HTML/XML/AsciiDoc. Update the ext filter logic
(the ext variable and the conditional that currently uses DOC_EXTENSIONS) so
that only '.json' (or a narrow filename pattern used for Doc Detective data)
proceeds to extract content and run ACTION_PATTERN.match; do not run the match
on extensions in DOC_EXTENSIONS or other prose formats to avoid false positives
when matching content.
In `@src/hooks/scripts/post-edit-validate-test-spec.js`:
- Around line 62-74: The current handler only logs when err.code === 1 or no
err, silently swallowing timeouts/other failures from execFile; add an explicit
branch for any other err (e.g., timeout, ENOENT, signal termination) to emit a
JSON console.log with additionalContext containing the fileName, output and rich
error details (err.message, err.code, err.signal and/or err.killed) so users see
the failure; update the block around the existing if (err && err.code === 1) /
else if (!err) to include an else if (err) branch that logs these error details
before the existing process.exit(0) call.
In `@src/hooks/scripts/session-start-check-install.sh`:
- Around line 37-41: The JSON output currently interpolates $STATUS directly
which can break JSON if it contains quotes, backslashes, or newlines; replace
the vulnerable printf interpolation in the AVAILABLE=true branch by constructing
the JSON with a proper JSON-escaping tool (e.g., use jq: jq -n --arg status
"$STATUS" '{"additionalContext":("Doc Detective status: \($status). Tests can be
executed directly.")}') and write that to stdout, referencing the existing
AVAILABLE and STATUS variables and replacing the printf call; if jq may not be
available, sanitize/escape $STATUS before interpolation (escape backslashes,
double quotes and newlines) and then produce the JSON string to ensure valid
output.
---
Nitpick comments:
In `@plugins/doc-detective/hooks/scripts/session-start-check-install.sh`:
- Around line 20-26: Replace the two separate invocations of "timeout 8 npx
doc-detective --version" with a single invocation whose output and exit status
are captured once: run the command into a variable (e.g., capture output into a
temp variable), test that the command succeeded by checking its exit status or
non-empty output, then set VERSION from that captured output (falling back to
"unknown" if empty) and set STATUS and AVAILABLE accordingly; update the block
where AVAILABLE, VERSION, STATUS and the "npx doc-detective --version" calls
appear so the command is executed only once.
In `@src/hooks/scripts/session-start-check-install.sh`:
- Around line 20-26: Replace the double npx invocation with a single call that
captures the output and exit status: run timeout 8 npx doc-detective --version
once, store its stdout (or "unknown" on failure) into VERSION and check the
command's exit code to set AVAILABLE="true" and STATUS="doc-detective available
via npx (version: ${VERSION})" only when the command succeeded; reference the
existing variables and constructs (AVAILABLE, VERSION, STATUS, timeout, npx
doc-detective --version) so you avoid calling npx twice and eliminate the extra
latency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 70cfd563-5773-41c3-8276-458be49f1870
📒 Files selected for processing (61)
.claude-plugin/marketplace.json.husky/pre-commitGEMINI.mdREADME.mdagents/doc-detective-specialist.mdbuild.jsgemini-extension.jsonhooks/claude-hooks.jsonhooks/gemini-hooks.jsonhooks/scripts/post-edit-format-test-spec.jshooks/scripts/post-edit-suggest-testing.jshooks/scripts/post-edit-validate-test-spec.jshooks/scripts/post-edit-warn-inline-tests.jshooks/scripts/pre-edit-block-action-antipattern.jshooks/scripts/session-start-check-install.shpackage.jsonplugins/doc-detective/.claude-plugin/plugin.jsonplugins/doc-detective/agents/doc-detective-specialist.mdplugins/doc-detective/hooks/hooks.jsonplugins/doc-detective/hooks/scripts/post-edit-format-test-spec.jsplugins/doc-detective/hooks/scripts/post-edit-suggest-testing.jsplugins/doc-detective/hooks/scripts/post-edit-validate-test-spec.jsplugins/doc-detective/hooks/scripts/post-edit-warn-inline-tests.jsplugins/doc-detective/hooks/scripts/pre-edit-block-action-antipattern.jsplugins/doc-detective/hooks/scripts/session-start-check-install.shplugins/doc-detective/skills/doc-detective-doc-testing/SKILL.mdplugins/doc-detective/skills/doc-detective-generate/SKILL.mdplugins/doc-detective/skills/doc-detective-init/SKILL.mdplugins/doc-detective/skills/doc-detective-inject/SKILL.mdplugins/doc-detective/skills/doc-detective-inline-test-injection/SKILL.mdplugins/doc-detective/skills/doc-detective-install-github-action/SKILL.mdplugins/doc-detective/skills/doc-detective-project-bootstrap/SKILL.mdplugins/doc-detective/skills/doc-detective-test/SKILL.mdplugins/doc-detective/skills/doc-detective-validate/SKILL.mdskills/doc-detective-doc-testing/SKILL.mdskills/doc-detective-generate/SKILL.mdskills/doc-detective-init/SKILL.mdskills/doc-detective-inject/SKILL.mdskills/doc-detective-inline-test-injection/SKILL.mdskills/doc-detective-install-github-action/SKILL.mdskills/doc-detective-project-bootstrap/SKILL.mdskills/doc-detective-test/SKILL.mdskills/doc-detective-validate/SKILL.mdsrc/agents/doc-detective-specialist.mdsrc/hooks/claude-hooks.jsonsrc/hooks/gemini-hooks.jsonsrc/hooks/scripts/post-edit-format-test-spec.jssrc/hooks/scripts/post-edit-suggest-testing.jssrc/hooks/scripts/post-edit-validate-test-spec.jssrc/hooks/scripts/post-edit-warn-inline-tests.jssrc/hooks/scripts/pre-edit-block-action-antipattern.jssrc/hooks/scripts/session-start-check-install.shsrc/skills/doc-detective-doc-testing/SKILL.mdsrc/skills/doc-detective-generate/SKILL.mdsrc/skills/doc-detective-init/SKILL.mdsrc/skills/doc-detective-inject/SKILL.mdsrc/skills/doc-detective-inline-test-injection/SKILL.mdsrc/skills/doc-detective-install-github-action/SKILL.mdsrc/skills/doc-detective-project-bootstrap/SKILL.mdsrc/skills/doc-detective-test/SKILL.mdsrc/skills/doc-detective-validate/SKILL.md
plugins/doc-detective/hooks/scripts/pre-edit-block-action-antipattern.js
Show resolved
Hide resolved
- Stage src/hooks/ in pre-commit hook to prevent source/artifact drift - Handle timeout and unexpected errors in post-edit-validate-test-spec.js - Sanitize version string in session-start-check-install.sh to prevent JSON malformation
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/scripts/post-edit-validate-test-spec.js (1)
62-78: Includestderrfor richer validation failure details.The validator script outputs detailed validation results (the JSON object with specific errors) to
stderr, but the hook only captures and reportsstdout. When validation fails, users miss the detailed information about why the spec is invalid.♻️ Suggested improvement to include stderr in output
execFile('node', [validatorPath, filePath], { timeout: 25000 }, (err, stdout, stderr) => { const fileName = path.basename(filePath); const output = (stdout || '').trim(); + const errOutput = (stderr || '').trim(); if (err && err.code === 1) { // Validation failed console.log(JSON.stringify({ - additionalContext: `Doc Detective test spec validation FAILED for ${fileName}:\n${output}` + additionalContext: `Doc Detective test spec validation FAILED for ${fileName}:\n${output}${errOutput ? '\n' + errOutput : ''}` })); } else if (err) { // Timeout or unexpected error const reason = err.killed ? 'timed out' : err.message || 'unknown error'; console.log(JSON.stringify({ - additionalContext: `Doc Detective test spec validation could not complete for ${fileName}: ${reason}` + additionalContext: `Doc Detective test spec validation could not complete for ${fileName}: ${reason}${errOutput ? '\n' + errOutput : ''}` })); } else {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/hooks/scripts/post-edit-validate-test-spec.js` around lines 62 - 78, When validation fails or errors (the branches checking err && err.code === 1 and the else if (err) branch), include the validator's stderr along with stdout in the logged additionalContext so users see the detailed JSON error payload; update the console.log messages that reference output/fileName to also append or merge a stderr variable (from the child process callback) and ensure both stderr and output are presented (e.g., "stdout: ...; stderr: ...") for the failing branches.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/hooks/scripts/post-edit-validate-test-spec.js`:
- Around line 62-78: When validation fails or errors (the branches checking err
&& err.code === 1 and the else if (err) branch), include the validator's stderr
along with stdout in the logged additionalContext so users see the detailed JSON
error payload; update the console.log messages that reference output/fileName to
also append or merge a stderr variable (from the child process callback) and
ensure both stderr and output are presented (e.g., "stdout: ...; stderr: ...")
for the failing branches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a39a00f-3936-49a9-bee9-0fe2f39ced6d
📒 Files selected for processing (7)
.husky/pre-commithooks/scripts/post-edit-validate-test-spec.jshooks/scripts/session-start-check-install.shplugins/doc-detective/hooks/scripts/post-edit-validate-test-spec.jsplugins/doc-detective/hooks/scripts/session-start-check-install.shsrc/hooks/scripts/post-edit-validate-test-spec.jssrc/hooks/scripts/session-start-check-install.sh
✅ Files skipped from review due to trivial changes (1)
- .husky/pre-commit
🚧 Files skipped from review as they are similar to previous changes (5)
- src/hooks/scripts/session-start-check-install.sh
- plugins/doc-detective/hooks/scripts/post-edit-validate-test-spec.js
- hooks/scripts/session-start-check-install.sh
- hooks/scripts/post-edit-validate-test-spec.js
- plugins/doc-detective/hooks/scripts/session-start-check-install.sh
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
tests/hooks/hook1-validate-test-spec.test.js (1)
60-66: Use fully valid test-spec fixtures in the “valid spec” case.At Line [62]-Line [65], add
descriptionto each test object so this fixture matches the canonical test-spec structure.🧪 Fixture adjustment
const spec = { tests: [{ testId: 'test1', + description: 'basic navigation test', steps: [{ goTo: 'https://example.com' }, { find: 'Welcome' }], }], };Based on learnings: Applies to **/*.json : Test specifications must follow the format with
testsarray containing objects withtestId,description, andstepsproperties.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hook1-validate-test-spec.test.js` around lines 60 - 66, The fixture `spec` used in the test case currently has test objects missing the required `description` field; update the `spec` object (the `tests` array and its test object with `testId: 'test1'`) to include a `description` string for each test so the fixture matches the canonical test-spec structure (each test must have `testId`, `description`, and `steps`).tests/hooks/integration.test.js (1)
97-114: Good workaround for Gemini's workspace restriction.Creating temp directories inside the project root is necessary since Gemini CLI restricts writes to the workspace. One minor consideration: if tests fail before
after()runs,.tmp-gemini-test-*directories could remain in the repo.Consider adding these patterns to
.gitignoreto prevent accidental commits:+# Gemini integration test artifacts +.tmp-gemini-test-*🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/integration.test.js` around lines 97 - 114, Tests create temporary dirs named by tmpDir using prefix ".tmp-gemini-test-" inside the project (see the before/after hooks in the "Integration: Gemini CLI" describe block and the tmpDir variable); add a gitignore entry for the pattern ".tmp-gemini-test-*" (and optionally ".tmp-gemini-test-*/") to the repository .gitignore so leftover temp directories won't be committed if a test fails before after() cleans them up.tests/hooks/helpers.js (1)
12-35: Minor:codefield can be a string for system errors.When
execFileencounters a system error (e.g., timeout),err.codeis a string like'ETIMEDOUT'rather than a numeric exit code. The current logic would return that string as thecodevalue, which could cause unexpected comparisons if callers assume it's always a number.For test helper code this is acceptable—callers can check both string and numeric codes as needed. Consider adding a brief JSDoc note if type consistency matters for future maintainers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/helpers.js` around lines 12 - 35, The runHook helper currently returns err.code directly which can be a string for system errors; update the execFile callback in runHook to normalize the exit code: set the returned code to the numeric exit code when typeof err.code === 'number' (or 0 when no err), otherwise default to 1 for non-numeric/system errors, and preserve the original err.code/string on a new property (e.g., errCode) alongside stderr/stdout so callers can still inspect the string error like 'ETIMEDOUT'; adjust the resolved object shape accordingly and update any callers/tests or add a short JSDoc on runHook describing the normalized code and the errCode field.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.js`:
- Around line 381-406: The build silently continues when expected provider hook
manifests (claude-hooks.json / gemini-hooks.json) are absent, leaving hooks.json
missing; update the renaming logic around rootClaude/rootHooksDest and
pluginClaude/pluginHooksDest so the build fails fast: before attempting to
rename, assert the source file exists and if not, throw an error or call
process.exit(1) with a clear message referencing the missing manifest (e.g.,
"Missing claude-hooks.json in hooks dir"); do the same for the plugin copy case
(pluginClaude -> pluginHooksDest) and for any other expected provider files
(rootGemini/pluginGemini) so the build exits immediately rather than silently
producing broken artifacts.
In `@tests/hooks/hook1-validate-test-spec.test.js`:
- Around line 75-84: The test currently treats any non-null
parseOutput(result.stdout) as OK and can false-pass on malformed JSON; change
the check to explicitly assert that result.stdout is either empty (silent exit)
or valid JSON parsed by parseOutput. Concretely, update the conditional around
parseOutput/result.stdout so you first assert that result.stdout.trim() === ''
|| output, e.g. if result.stdout is non-empty assert output is truthy (meaning
parseOutput succeeded) and then assert output.additionalContext includes
'spec.json'; reference parseOutput(result.stdout), result.stdout and
output.additionalContext when making the change.
---
Nitpick comments:
In `@tests/hooks/helpers.js`:
- Around line 12-35: The runHook helper currently returns err.code directly
which can be a string for system errors; update the execFile callback in runHook
to normalize the exit code: set the returned code to the numeric exit code when
typeof err.code === 'number' (or 0 when no err), otherwise default to 1 for
non-numeric/system errors, and preserve the original err.code/string on a new
property (e.g., errCode) alongside stderr/stdout so callers can still inspect
the string error like 'ETIMEDOUT'; adjust the resolved object shape accordingly
and update any callers/tests or add a short JSDoc on runHook describing the
normalized code and the errCode field.
In `@tests/hooks/hook1-validate-test-spec.test.js`:
- Around line 60-66: The fixture `spec` used in the test case currently has test
objects missing the required `description` field; update the `spec` object (the
`tests` array and its test object with `testId: 'test1'`) to include a
`description` string for each test so the fixture matches the canonical
test-spec structure (each test must have `testId`, `description`, and `steps`).
In `@tests/hooks/integration.test.js`:
- Around line 97-114: Tests create temporary dirs named by tmpDir using prefix
".tmp-gemini-test-" inside the project (see the before/after hooks in the
"Integration: Gemini CLI" describe block and the tmpDir variable); add a
gitignore entry for the pattern ".tmp-gemini-test-*" (and optionally
".tmp-gemini-test-*/") to the repository .gitignore so leftover temp directories
won't be committed if a test fails before after() cleans them up.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: be452961-62a3-4f1f-935d-9d2ef59fe804
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (18)
build.jsgemini-extension.jsongemini-hook-test.jsonhooks/hooks.jsonpackage.jsonplugins/doc-detective/skills/doc-detective-doc-testing/scripts/doc-detective-validate-test.jsskills/doc-detective-doc-testing/scripts/doc-detective-validate-test.jssrc/hooks/gemini-hooks.jsonsrc/skills/doc-detective-doc-testing/scripts/doc-detective-validate-test.jstest-hook.mdtests/hooks/helpers.jstests/hooks/hook1-validate-test-spec.test.jstests/hooks/hook2-block-action-antipattern.test.jstests/hooks/hook3-suggest-testing.test.jstests/hooks/hook4-session-start-check-install.test.jstests/hooks/hook5-format-test-spec.test.jstests/hooks/hook6-warn-inline-tests.test.jstests/hooks/integration.test.js
✅ Files skipped from review due to trivial changes (4)
- gemini-hook-test.json
- gemini-extension.json
- src/hooks/gemini-hooks.json
- test-hook.md
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
…en test assertion - build.js: throw error instead of silently continuing when claude-hooks.json or gemini-hooks.json are missing from the hooks output directory - hook1 test: assert stdout is either empty or valid JSON, preventing false passes on malformed output
- session-start-check-install.sh: use npx --yes to suppress install prompts, pipe through tail -1 to ignore progress output that contaminates stdout - hook4 test: increase timeout to 30s for slow CI npx, add stdout diagnostic to assertion message for easier debugging
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
tests/hooks/hook2-block-action-antipattern.test.js (1)
67-72: Avoid another hard-coded copy of the action list.The same action set already exists in
src/hooks/scripts/pre-edit-block-action-antipattern.js. Keeping a separate copy here makes future action additions easy to miss in either the hook or the test. Consider sharing one exported constant instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hook2-block-action-antipattern.test.js` around lines 67 - 72, The test duplicates the action list that's already defined in pre-edit-block-action-antipattern.js; refactor by exposing the single source of truth from that module (create/export a constant like ACTION_NAMES or KNOWN_ACTIONS in pre-edit-block-action-antipattern.js if it doesn't exist) and update the test to import and use that exported constant instead of the hard-coded array in the it('should detect all known action names', ...) block; ensure the exported symbol name you choose matches the import in the test and keep the array content unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@build.js`:
- Around line 369-371: The check that currently returns when srcHooksDir is
missing should instead fail the build: inside the same function where
srcHooksDir is checked (after cleanOutputDirs() runs) replace the early return
with throwing an Error that includes the missing path (srcHooksDir) and a clear
message so the build fails fast; ensure callers of this routine will surface the
thrown error rather than treating it as success.
In `@tests/hooks/integration.test.js`:
- Around line 60-69: The test 'should report Doc Detective CLI availability via
Hook 4' currently only checks for a forced YES/NO and can pass without the
session-start hook; update the assertion to verify a deterministic marker that
Hook 4 injects at session start (e.g., a specific string like the hook payload's
marker) rather than relying on result.stdout including 'YES' or 'NO'. Locate the
test case using the test title and the runClaude call, and replace or augment
the assert.ok that checks result.stdout with an assertion that
result.stdout.includes('<HOOK4_SESSION_MARKER>') (or the actual marker string
the hook sends). Apply the same change to the parallel test at lines 153-164 so
both tests verify the hook-injected marker instead of only the constrained
YES/NO response.
- Around line 83-93: The current assertion in tests/hooks/integration.test.js
uses broad substring checks on result.stdout which can false-positive; update
the assertion that follows runClaude to assert that result.stdout (or combined
stdout+stderr) includes the exact hook warning text emitted by the hook (e.g.,
the literal "contains inline Doc Detective test comments" from
src/hooks/scripts/post-edit-warn-inline-tests.js) or another stable sentinel
string produced by Hook 6, and remove the fallback to generic
toLowerCase('warn'|'inline') checks; apply the same replacement for the other
occurrence of this assertion in the file (the block at the later test around
lines 177-192) so both tests verify the hook's specific warning message rather
than broad keywords.
---
Nitpick comments:
In `@tests/hooks/hook2-block-action-antipattern.test.js`:
- Around line 67-72: The test duplicates the action list that's already defined
in pre-edit-block-action-antipattern.js; refactor by exposing the single source
of truth from that module (create/export a constant like ACTION_NAMES or
KNOWN_ACTIONS in pre-edit-block-action-antipattern.js if it doesn't exist) and
update the test to import and use that exported constant instead of the
hard-coded array in the it('should detect all known action names', ...) block;
ensure the exported symbol name you choose matches the import in the test and
keep the array content unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 5732e8cb-5a6d-4bd4-99ec-35b713b85a54
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
.github/workflows/test-hooks.ymlbuild.jsgemini-extension.jsongemini-hook-test.jsonhooks/hooks.jsonhooks/scripts/session-start-check-install.shpackage.jsonplugins/doc-detective/hooks/scripts/session-start-check-install.shplugins/doc-detective/skills/doc-detective-doc-testing/scripts/doc-detective-validate-test.jsskills/doc-detective-doc-testing/scripts/doc-detective-validate-test.jssrc/hooks/gemini-hooks.jsonsrc/hooks/scripts/session-start-check-install.shsrc/skills/doc-detective-doc-testing/scripts/doc-detective-validate-test.jstest-hook.mdtests/hooks/helpers.jstests/hooks/hook1-validate-test-spec.test.jstests/hooks/hook2-block-action-antipattern.test.jstests/hooks/hook3-suggest-testing.test.jstests/hooks/hook4-session-start-check-install.test.jstests/hooks/hook5-format-test-spec.test.jstests/hooks/hook6-warn-inline-tests.test.jstests/hooks/integration.test.js
✅ Files skipped from review due to trivial changes (8)
- package.json
- gemini-extension.json
- test-hook.md
- .github/workflows/test-hooks.yml
- src/hooks/gemini-hooks.json
- hooks/hooks.json
- tests/hooks/hook5-format-test-spec.test.js
- tests/hooks/helpers.js
🚧 Files skipped from review as they are similar to previous changes (7)
- gemini-hook-test.json
- src/hooks/scripts/session-start-check-install.sh
- hooks/scripts/session-start-check-install.sh
- tests/hooks/hook1-validate-test-spec.test.js
- plugins/doc-detective/hooks/scripts/session-start-check-install.sh
- tests/hooks/hook4-session-start-check-install.test.js
- tests/hooks/hook3-suggest-testing.test.js
…st assertions - build.js: Throw error instead of silently returning when src/hooks/ is missing - integration tests: Check for hook marker text in SessionStart assertions - integration tests: Check for exact hook warning text in inline-test assertions
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/hooks/integration.test.js (2)
65-70:⚠️ Potential issue | 🟠 MajorMake SessionStart assertions hook-specific (remove
YES/NOfallback).Line 69 and Line 170 let this test pass from prompt-constrained model output even when the SessionStart hook context is absent.
Suggested diff
- assert.ok( - combined.includes('Doc Detective status:') || combined.includes('Doc Detective CLI is not installed') || - result.stdout.includes('YES') || result.stdout.includes('NO'), - `Expected hook marker or YES/NO. Got: ${result.stdout.slice(0, 200)}` - ); + assert.ok( + combined.includes('Doc Detective status:') || combined.includes('Doc Detective CLI is not installed'), + `Expected SessionStart hook marker. Got: ${(result.stdout + result.stderr).slice(0, 200)}` + );Also applies to: 167-170
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/integration.test.js` around lines 65 - 70, The current assertion in the test uses a fallback on result.stdout including 'YES'/'NO', which lets prompt-constrained model output pass even when the SessionStart hook context is missing; update the assertions in the test (the assertion that constructs combined from result.stdout + result.stderr and the duplicate at the later location) to only assert for the hook-specific markers (e.g., combined.includes('Doc Detective status:') || combined.includes('Doc Detective CLI is not installed')) and remove the result.stdout.includes('YES')/includes('NO') alternatives so the test fails when the SessionStart hook marker is absent.
95-97:⚠️ Potential issue | 🟠 MajorUse only the hook’s warning marker in inline-warning assertions.
Line 96 and Line 198 still allow prompt-driven/generic “warn” matches, so these tests can pass without Hook 6 / AfterTool output.
Suggested diff
- assert.ok( - combined.includes('contains inline Doc Detective test comments') || - result.stdout.includes('WARNED') || result.stdout.toLowerCase().includes('warn'), - `Expected hook warning about inline tests. Got: ${result.stdout.slice(0, 300)}` - ); + assert.ok( + combined.includes('contains inline Doc Detective test comments'), + `Expected inline-test hook warning marker. Got: ${(result.stdout + result.stderr).slice(0, 300)}` + );- assert.ok( - combined.includes('contains inline Doc Detective test comments') || - combined.includes('WARNED') || combined.toLowerCase().includes('warn'), - `Expected hook warning about inline tests. Got stdout: ${result.stdout.slice(0, 200)}` - ); + assert.ok( + combined.includes('contains inline Doc Detective test comments'), + `Expected inline-test hook warning marker. Got: ${(result.stdout + result.stderr).slice(0, 200)}` + );Also applies to: 197-199
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/integration.test.js` around lines 95 - 97, The assertion is too permissive because it allows generic "WARNED" or any "warn" substrings; update the check around combined and result.stdout so it only looks for the Hook 6/AfterTool specific warning marker (the exact string used by the hook, e.g. "contains inline Doc Detective test comments") instead of result.stdout.includes('WARNED') or result.stdout.toLowerCase().includes('warn'). Concretely, modify the assertion that references combined and result.stdout to remove the generic checks and only use combined.includes('contains inline Doc Detective test comments') || result.stdout.includes('contains inline Doc Detective test comments') (or the exact AfterTool marker string your hook emits) so the test only passes when the hook's warning is present.
🧹 Nitpick comments (1)
tests/hooks/integration.test.js (1)
164-164: Harden auth/rate-limit skip detection to reduce flaky failures.These checks are case-sensitive and
stdout-only; auth/rate-limit messages can surface instderror with different casing.Suggested diff
- if (!result.stdout.trim() || result.stdout.includes('authentication')) this.skip(); + const output = `${result.stdout}\n${result.stderr}`.toLowerCase(); + if (!result.stdout.trim() || output.includes('authentication')) this.skip();- if (!result.stdout.trim() || result.stdout.includes('authentication')) this.skip(); + const output = `${result.stdout}\n${result.stderr}`.toLowerCase(); + if (!result.stdout.trim() || output.includes('authentication')) this.skip();Also applies to: 192-192
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/integration.test.js` at line 164, The flaky skip logic only checks result.stdout case-sensitively; update the checks around the test result (the variable result used at the skip points) to inspect both result.stdout and result.stderr and perform case-insensitive matching (e.g., normalize to lower-case) for keywords like "authentication" and "rate-limit" so skips fire reliably regardless of casing or stream; replace the two existing lines that call this.skip() based on result.stdout with a shared predicate that concatenates or checks both streams in lower-case before deciding to this.skip().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/hooks/integration.test.js`:
- Around line 65-70: The current assertion in the test uses a fallback on
result.stdout including 'YES'/'NO', which lets prompt-constrained model output
pass even when the SessionStart hook context is missing; update the assertions
in the test (the assertion that constructs combined from result.stdout +
result.stderr and the duplicate at the later location) to only assert for the
hook-specific markers (e.g., combined.includes('Doc Detective status:') ||
combined.includes('Doc Detective CLI is not installed')) and remove the
result.stdout.includes('YES')/includes('NO') alternatives so the test fails when
the SessionStart hook marker is absent.
- Around line 95-97: The assertion is too permissive because it allows generic
"WARNED" or any "warn" substrings; update the check around combined and
result.stdout so it only looks for the Hook 6/AfterTool specific warning marker
(the exact string used by the hook, e.g. "contains inline Doc Detective test
comments") instead of result.stdout.includes('WARNED') or
result.stdout.toLowerCase().includes('warn'). Concretely, modify the assertion
that references combined and result.stdout to remove the generic checks and only
use combined.includes('contains inline Doc Detective test comments') ||
result.stdout.includes('contains inline Doc Detective test comments') (or the
exact AfterTool marker string your hook emits) so the test only passes when the
hook's warning is present.
---
Nitpick comments:
In `@tests/hooks/integration.test.js`:
- Line 164: The flaky skip logic only checks result.stdout case-sensitively;
update the checks around the test result (the variable result used at the skip
points) to inspect both result.stdout and result.stderr and perform
case-insensitive matching (e.g., normalize to lower-case) for keywords like
"authentication" and "rate-limit" so skips fire reliably regardless of casing or
stream; replace the two existing lines that call this.skip() based on
result.stdout with a shared predicate that concatenates or checks both streams
in lower-case before deciding to this.skip().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1a5e871f-c01f-4666-ad66-9e88cac5cefc
📒 Files selected for processing (2)
build.jstests/hooks/integration.test.js
🚧 Files skipped from review as they are similar to previous changes (1)
- build.js
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
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
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.
Add hooks to streamline Doc Detective usage and avoid high-traffic errors.
Summary by CodeRabbit
New Features
Tests
Documentation
Chores