-
Notifications
You must be signed in to change notification settings - Fork 793
feat(hooks): add comment-checker gating to skip unnecessary CLI calls #485
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: dev
Are you sure you want to change the base?
feat(hooks): add comment-checker gating to skip unnecessary CLI calls #485
Conversation
|
All contributors have signed the CLA. Thank you! ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ed04e35b2b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Greptile SummaryAdded content-based gating to skip comment-checker CLI execution when no comment markers are detected in file operations (Write/Edit/MultiEdit tools), reducing unnecessary blocking latency. Key Changes:
Impact: Confidence Score: 5/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Agent
participant Hook as comment-checker hook
participant Gate as shouldRunCommentChecker()
participant CLI as comment-checker CLI
Agent->>Hook: tool.execute.before (Write/Edit/MultiEdit)
Note over Hook: Store pendingCall with<br/>content/edits in Map
Agent->>Agent: Execute tool (write file/edit content)
Agent->>Hook: tool.execute.after
Hook->>Hook: Retrieve pendingCall from Map
Hook->>Hook: Check if CLI available
alt CLI not available
Hook-->>Agent: Skip (return early)
else CLI available
Hook->>Gate: shouldRunCommentChecker(pendingCall)
Gate->>Gate: Check content for markers<br/>(// /* # <!-- ''' """)
Gate->>Gate: Check oldString/newString for markers
Gate->>Gate: Check edits array for markers
alt No comment markers found
Gate-->>Hook: false
Hook-->>Agent: Skip CLI call (return early)
Note over Hook,CLI: PERFORMANCE OPTIMIZATION:<br/>Skip blocking CLI call
else Comment markers detected
Gate-->>Hook: true
Hook->>CLI: runCommentChecker(hookInput)
CLI->>CLI: Parse and analyze comments
CLI-->>Hook: CheckResult (hasComments, message)
alt hasComments = true
Hook->>Hook: Append warning message to output
end
Hook-->>Agent: Return (with optional warning)
end
end
|
Greptile's behavior is changing!From now on, if a review finishes with no comments, we will not post an additional "statistics" comment to confirm that our review found nothing to comment on. However, you can confirm that we reviewed your changes in the status check section. This feature can be toggled off in your Code Review Settings by deselecting "Create a status check for each PR". |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/hooks/comment-checker/index.ts">
<violation number="1" location="src/hooks/comment-checker/index.ts:23">
P2: Adding `"--"` as a comment marker will cause many false positives, reducing the effectiveness of the gating optimization. The `--` pattern appears frequently in non-comment contexts (CLI arguments like `--help`, git commands, npm scripts). Consider a more specific pattern like requiring `--` at the start of a line followed by a space (standard SQL comment format), or remove this marker if false positive rate is unacceptable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 2 files (changes from recent commits).
Prompt for AI agents (all issues)
Check if these issues are valid — if so, understand the root cause of each and fix them.
<file name="src/hooks/claude-code-hooks/post-tool-use.ts">
<violation number="1" location="src/hooks/claude-code-hooks/post-tool-use.ts:159">
P3: Inconsistent logging pattern. The log function expects the descriptive message as the first argument (see line 97), but here a short tag is passed first and the error message second. Consider combining them into a single descriptive message.</violation>
</file>
<file name="src/hooks/comment-checker/index.ts">
<violation number="1" location="src/hooks/comment-checker/index.ts:23">
P2: Adding trailing space to `"-- "` causes false negatives for Lua, Haskell, and Ada comments which don't require a space after `--`. This gating mechanism will skip the CLI for legitimate comments like `--comment` in these languages. Consider keeping `"--"` to avoid missing valid comments, or add language-specific detection logic.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
66fc079 to
73f8967
Compare
|
Not sure why this is required. this is already covered by comment checker. am i missing something? |
|
Thanks for the question! This isn’t about detection coverage (comment-checker already does that). The new gating is a perf optimization: if the file content/old+new strings/edits contain no comment markers, we skip invoking the comment-checker CLI entirely. That avoids a blocking process spawn for pure code changes and reduces latency, while keeping comment-checker as the source of truth whenever markers are present. The gating is intentionally conservative (false positives are acceptable; false negatives would be worse). |
73f8967 to
d0e64f6
Compare
- Add shouldRunCommentChecker() to detect comment markers in content - Skip CLI execution when no comment-bearing content detected - Reduce blocking latency for edits without comments - Added unit tests for gating behavior
d0e64f6 to
9e77eed
Compare
feat(hooks): add comment-checker gating to skip unnecessary CLI calls
Summary
Changes
src/hooks/comment-checker/index.ts: AddedshouldRunCommentChecker()withCOMMENT_MARKERSdetection, integrated gating intoprocessWithCli()flowsrc/hooks/comment-checker/index.test.ts: Added unit tests for gating behaviorTesting
bun run typecheck bun test src/hooks/comment-checker/index.test.tsRelated Issues
Summary by cubic
Run the comment-checker only when content or edits contain comment markers, skipping the CLI to reduce latency for non-comment edits.
New Features
Bug Fixes
Written for commit 9e77eed. Summary will update on new commits.