fix: address review findings in qyl-continuation stop-judge.py (#159)#159
fix: address review findings in qyl-continuation stop-judge.py (#159)#159
Conversation
- Security: move throttle file from /tmp to ~/.claude/qyl-continuation/ (symlink attack) - Bug: remove stop_hook_active gate — field doesn't exist in Stop payload - Bug: read msg.message.content for transcript entries (H1-H4 were inoperative) - Bug: read transcript in Python instead of shelling out to tail - Bug: byte-aware truncation for MAX_CONTEXT_BYTES - Logic: H3/H4 now check for NEXT_STEP_RX before stopping - Robustness: handle structured_output being null
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses several critical findings from previous reviews, significantly enhancing the robustness, security, and logical accuracy of the Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Cache: Disabled due to data retention organization setting Knowledge base: Disabled due to data retention organization setting 📝 WalkthroughWalkthroughModified the stop-judge hook to introduce persistent working directory storage, refactor transcript loading from subprocess-based to direct disk reading, update throttle and heuristic evaluation logic, and enhance judge phase robustness with improved JSON parsing and context truncation. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes ✨ 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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3cfd5b46e8
ℹ️ 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".
| tail_bytes = raw[-(TAIL_LINES * 8192):] if len(raw) > TAIL_LINES * 8192 else raw | ||
| lines = [l.strip() for l in tail_bytes.decode("utf-8", errors="ignore").split("\n") if l.strip()] |
There was a problem hiding this comment.
Using raw[-(TAIL_LINES * 8192):] assumes each of the last transcript lines is <=8KB, so a valid but large final JSON line gets truncated and fails json.loads, which can leave messages empty and force approve("Empty transcript") even when there is actionable context to continue. This is a behavioral regression from line-based tailing and will misclassify long tool outputs or assistant messages.
Useful? React with 👍 / 👎.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses previous review findings, enhancing security, fixing bugs, and refining logic, specifically by relocating the throttle file and improving message content parsing. However, it introduces two security issues: a potential path traversal vulnerability on Windows due to incomplete sanitization of the session_id, and a denial-of-service risk caused by reading the entire transcript file into memory. This memory-intensive approach also poses a significant performance issue for large files. These issues should be addressed to ensure the robustness and security of the hook across all platforms.
| lines = [l.strip() for l in tail.stdout.strip().split("\n") if l.strip()] | ||
| except (subprocess.TimeoutExpired, OSError): | ||
| p = Path(transcript_path) | ||
| raw = p.read_bytes() |
There was a problem hiding this comment.
The current implementation reads the entire transcript file into memory using p.read_bytes(). This poses a denial-of-service risk, as extremely large files or named pipes could cause the script to crash or hang indefinitely due to excessive memory consumption. This also represents a performance regression from the previous tail-based approach. A more memory-efficient approach would be to seek near the end of the file and read only the last chunk.
| transcript_path = event.get("transcript_path", "") | ||
| session_id = event.get("session_id", "unknown") | ||
| throttle_file = Path(f"/tmp/.qyl-continue-{session_id.replace('/', '_')}") | ||
| throttle_file = WORK_DIR / f"throttle-{session_id.replace('/', '_')}" |
There was a problem hiding this comment.
The session_id is sanitized by replacing forward slashes with underscores, but backslashes (which are also path separators on Windows) are not replaced. This allows for potential path traversal out of the intended WORK_DIR on Windows systems. An attacker who can control the session_id could potentially point the throttle_file to a sensitive location, leading to unauthorized file deletion or modification when write_text() or unlink() is called.
There was a problem hiding this comment.
Pull request overview
This PR updates the qyl-continuation stop hook judge to address prior review findings, primarily around safer throttling, correct transcript parsing, and more robust judge-response handling.
Changes:
- Moves the throttle state file into
~/.claude/qyl-continuation/and removes the non-functionalstop_hook_activegating so throttling actually applies. - Fixes transcript parsing by reading
msg.message.contentin addition tomsg.content, and replacestailwith a Python-based tail implementation. - Improves continuation logic and robustness (NEXT_STEP heuristic checks, byte-aware prompt truncation, and
structured_output: nullhandling).
| WORK_DIR = Path.home() / ".claude" / "qyl-continuation" | ||
| WORK_DIR.mkdir(parents=True, exist_ok=True) |
There was a problem hiding this comment.
WORK_DIR.mkdir(...) runs at import time and can raise (e.g., HOME not set, permission issues), which would crash the hook without emitting the required JSON decision. Consider wrapping directory creation in a try/except and falling back to a safe behavior (e.g., disable throttling / use a temporary per-run dir) while still calling approve/block.
| p = Path(transcript_path) | ||
| raw = p.read_bytes() | ||
| tail_bytes = raw[-(TAIL_LINES * 8192):] if len(raw) > TAIL_LINES * 8192 else raw | ||
| lines = [l.strip() for l in tail_bytes.decode("utf-8", errors="ignore").split("\n") if l.strip()] | ||
| lines = lines[-TAIL_LINES:] |
There was a problem hiding this comment.
The transcript tailing logic reads the entire transcript into memory (read_bytes()), and then slices a fixed byte window (TAIL_LINES * 8192). Besides being costly for large transcripts, it can also miss/partial the last JSONL entries when a single line is larger than the byte window, causing json.loads to fail and heuristics to operate on an incomplete/empty message set. Consider reading from the end of the file via seek/backward scan until you have N newlines (and avoid loading the whole file).
Fixes all findings from PR #158 reviews (Gemini, Copilot, Codex):
Security (Gemini — High)
/tmpto~/.claude/qyl-continuation/— eliminates symlink attack vectorBugs (Copilot + Gemini)
stop_hook_activegate — field doesn't exist in Stop hook payload, throttle guard was never firingmsg.message.contentin addition tomsg.content— transcript entries nest content, so H1-H4 heuristics were inoperativetailsubprocess (platform-independent)MAX_CONTEXT_BYTES— no more mid-codepoint slicingstructured_output: null— prevents AttributeError in judge response parsingLogic (Codex)
NEXT_STEP_RXbefore stopping — don't stop when assistant explicitly states pending next actionSummary by CodeRabbit
Bug Fixes
Refactor