feat(hooks): aelf-pre-issue-create guard — duplicate-detection before gh issue create (#941) - #945
Merged
Conversation
|
Warning Review limit reached
More reviews will be available in 44 minutes and 18 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (12)
📝 WalkthroughWalkthroughThis PR introduces a pre-issue-create guard hook that runs before ChangesPre-issue-create Guard
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
Reviewer's GuideAdds a new default-on PreToolUse:Bash hook (aelf-pre-issue-hook) that runs deterministic duplicate detection before gh issue create, wires it into setup/auto-install/CLI, and ships a comprehensive test + docs update for configuration and installation behavior. Sequence diagram for pre-issue-create duplicate-detection hooksequenceDiagram
actor Agent
participant AelfPreIssueHook as aelf-pre-issue-hook
participant gh as gh
participant git as git
Agent->>AelfPreIssueHook: main() stdin JSON (tool_name, tool_input.command)
AelfPreIssueHook->>AelfPreIssueHook: run_guard(stdin_json)
alt ALLOW_DUP_ISSUE or AELFRICE_NO_PRE_ISSUE_GUARD
AelfPreIssueHook-->>Agent: exit 0
else Non-Bash or not _is_gh_issue_create(command)
AelfPreIssueHook-->>Agent: exit 0
else Valid gh issue create with --title
AelfPreIssueHook->>AelfPreIssueHook: tokenize_title(title)
AelfPreIssueHook->>AelfPreIssueHook: _top_query_tokens(tokens)
par Build candidates
AelfPreIssueHook->>gh: _build_gh_candidates() via gh issue list
gh-->>AelfPreIssueHook: JSON issues
AelfPreIssueHook->>git: _build_git_candidates() via git log
git-->>AelfPreIssueHook: commit lines
end
AelfPreIssueHook->>AelfPreIssueHook: _score_and_rank(tokens, candidates)
alt max score >= BLOCK_THRESHOLD
AelfPreIssueHook->>AelfPreIssueHook: _format_block_message()
AelfPreIssueHook-->>Agent: stderr message, exit 2 (BLOCK)
else below threshold or no candidates
AelfPreIssueHook-->>Agent: exit 0 (PASS)
end
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
robotrocketscience
added
the
attn:review
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 security issues, 2 other issues, and left some high level feedback:
Security issues:
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
- Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'. (link)
General comments:
- In
pre_issue_create_hook._safe_read_body_file,_CLAUDE_DIRis computed once at import time usingPath.home(), so any later HOME changes (common in tests or subprocesses) won’t be reflected; consider resolving the claude dir inside the function (or via a small helper) so it always reflects the current environment. - The body-file content is currently read in
run_guardvia_safe_read_body_filebut never incorporated into scoring or messaging; if this is intentional, a short comment explaining why the body is ignored would help future readers, otherwise consider either wiring it into the duplicate heuristic or dropping the read to avoid dead work.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `pre_issue_create_hook._safe_read_body_file`, `_CLAUDE_DIR` is computed once at import time using `Path.home()`, so any later HOME changes (common in tests or subprocesses) won’t be reflected; consider resolving the claude dir inside the function (or via a small helper) so it always reflects the current environment.
- The body-file content is currently read in `run_guard` via `_safe_read_body_file` but never incorporated into scoring or messaging; if this is intentional, a short comment explaining why the body is ignored would help future readers, otherwise consider either wiring it into the duplicate heuristic or dropping the read to avoid dead work.
## Individual Comments
### Comment 1
<location path="src/aelfrice/pre_issue_create_hook.py" line_range="182-189" />
<code_context>
+ return ""
+
+
+def _safe_read_body_file(path_str: str) -> str:
+ """Read *path_str* if it is a regular file outside ``~/.claude/``.
+
+ Returns empty string on any failure or if the path is under ~/.claude/.
+ """
+ if not path_str:
+ return ""
+ p = Path(path_str)
+ try:
+ resolved = p.resolve()
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Body-file path check won’t catch `~/.claude/...` because `Path.resolve()` doesn’t expand `~`.
Because `Path(path_str).resolve()` doesn’t expand `~`, a path like `~/.claude/foo` won’t be recognized as under `_CLAUDE_DIR`, so the protection only works if the caller has already expanded the user dir. To make this robust, call `Path(path_str).expanduser().resolve()` and apply `expanduser()` when computing `_CLAUDE_DIR` as well, so tilde paths are consistently blocked.
Suggested implementation:
```python
if not path_str:
return ""
# Normalize user directories (e.g. "~") before resolving, so "~/.claude"
# is consistently recognized as being under the Claude config directory.
p = Path(path_str).expanduser()
try:
resolved = p.resolve()
except (OSError, ValueError):
return ""
# Refuse paths that originate under ~/.claude/
try:
resolved.relative_to(_CLAUDE_DIR.expanduser().resolve())
return "" # inside ~/.claude/ — refuse
except ValueError:
pass
```
To fully implement the suggestion, also ensure `_CLAUDE_DIR` is created with `expanduser()`, e.g.:
`_CLAUDE_DIR = Path("~/.claude").expanduser()`. If `_CLAUDE_DIR` is currently defined without `expanduser()`, update that definition accordingly so both the directory constant and incoming paths are using the same tilde-expanded base.
</issue_to_address>
### Comment 2
<location path="src/aelfrice/pre_issue_create_hook.py" line_range="375-377" />
<code_context>
+ return 0
+
+ # --- Optional body read (title-only scoring is fine without body) --------
+ body_file = _extract_body_file(command)
+ _safe_read_body_file(body_file) # read but not currently used in scoring
+
+ # --- Tokenize and build query -------------------------------------------
</code_context>
<issue_to_address>
**suggestion (performance):** Body-file read is currently unused, adding I/O cost without affecting the decision.
`_safe_read_body_file(body_file)` performs filesystem I/O on every guarded `gh issue create` but its result isn’t used for tokenization or scoring. Consider removing this read (or guarding it behind a flag) until body content is actually incorporated into the similarity logic.
```suggestion
# --- Optional body path (reserved for future body-aware scoring) --------
body_file = _extract_body_file(command)
# NOTE: We intentionally avoid reading the body here to prevent
# unnecessary filesystem I/O until body content participates in scoring.
```
</issue_to_address>
### Comment 3
<location path="src/aelfrice/pre_issue_create_hook.py" line_range="388-390" />
<code_context>
result = subprocess.run(
argv, capture_output=True, text=True, timeout=10,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>
### Comment 4
<location path="src/aelfrice/pre_issue_create_hook.py" line_range="394-396" />
<code_context>
result = subprocess.run(
argv, capture_output=True, text=True, timeout=5,
)
</code_context>
<issue_to_address>
**security (python.lang.security.audit.dangerous-subprocess-use-audit):** Detected subprocess function 'run' without a static string. If this data can be controlled by a malicious actor, it may be an instance of command injection. Audit the use of this call to ensure it is not controllable by an external resource. You may consider using 'shlex.escape()'.
*Source: opengrep*
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
Owner
Author
|
[claim:review:Setr:2026-06-04T19:53:18Z] |
Owner
Author
|
[claim:review:Kulili:2026-06-04T19:53:26Z] |
Owner
Author
|
[release:review:Kulili:2026-06-04T19:53:30Z] |
Owner
Author
|
Substantively approve, two small follow-ups to consider before merge (or as a follow-up PR — either works). Verified
Sourcery's two subprocess findings are false positivesBoth "subprocess.run without static string" hits operate on token-list arguments built from sorted query tokens ( Two real findings worth a small follow-up(1)
|
robotrocketscience
added
the
ready-to-merge
Owner
Author
|
[release:review:Setr:2026-06-04T19:56:07Z] |
|
merge-train: blocked 3 review thread(s) are unresolved on these files: src/aelfrice/pre_issue_create_hook.py, tests/test_aelf_setup_pre_issue_guard.py, tests/test_pre_issue_create_hook.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
Owner
Author
|
[claim:review:Kulili:2026-06-04T21:28:01Z] |
robotrocketscience
force-pushed
the
feat/issue-941-pre-issue-create-guard
branch
from
June 4, 2026 21:30
c4d9530 to
2b2a695
Compare
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked branch is not fast-forward on The |
github-actions
Bot
removed
the
ready-to-merge
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/user/CONFIG.md`:
- Line 509: Update the wording in the docs so the threshold describes the actual
implemented comparison (>= 0.5) rather than “exceeds 0.5”; change the phrase at
the line that currently reads “exceeds 0.5” to something like “is at least 0.5
(>= 0.5)” to match the behavior enforced by pre_issue_create_hook which guards
on >= 0.5.
In `@docs/user/INSTALL.md`:
- Around line 148-149: Update the documentation to remove or amend the blanket
statement that “All hooks are non-blocking… exit 0” to reflect that the newly
introduced pre-issue-guard (PreToolUse:Bash) is a blocking hook; explicitly
mention that pre-issue-guard (v3.4.0+) will block `gh issue create` when titles
overlap an existing issue/commit above 0.5 Jaccard and therefore can exit
non-zero to prevent the action, and adjust any examples or the sentence at the
end of the hooks list to note that most hooks are non-blocking except for the
blocking pre-issue-guard.
In `@src/aelfrice/pre_issue_create_hook.py`:
- Around line 240-244: The code currently returns any parsed list from
json.loads(raw) as-is (parsed) but later code assumes each candidate supports
.get(...), so filter and validate the JSON row shape: replace the raw return of
parsed with a filtered list like [item for item in parsed if isinstance(item,
dict)] (or coerce invalid rows into dicts or drop them) and optionally log/count
dropped items; update the branch handling json.loads(raw) to return only dict
entries so downstream uses of candidate.get(...) are safe.
In `@tests/test_pre_issue_create_hook.py`:
- Line 212: The test function test_real_file has the tmp_path parameter
incorrectly typed as pytest.TempdirFactory; change the parameter annotation to
Path (from pathlib) and add an import for Path if missing so the signature
becomes def test_real_file(self, tmp_path: Path) -> None and the test uses the
correct pathlib.Path type provided by the tmp_path fixture.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 5d765742-5e97-4d23-b1cc-8ccf2eccfef7
📒 Files selected for processing (12)
docs/user/CONFIG.mddocs/user/INSTALL.mdpyproject.tomlsrc/aelfrice/auto_install.pysrc/aelfrice/cli.pysrc/aelfrice/data/hook_manifest.jsonsrc/aelfrice/pre_issue_create_hook.pysrc/aelfrice/setup.pytests/test_aelf_setup_pre_issue_guard.pytests/test_aelf_setup_search_tool_bash.pytests/test_cli_setup_opt_out_sync.pytests/test_pre_issue_create_hook.py
robotrocketscience
force-pushed
the
feat/issue-941-pre-issue-create-guard
branch
from
June 4, 2026 21:38
8bf85f1 to
5803c69
Compare
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked 3 review thread(s) are unresolved on these files: docs/user/CONFIG.md, src/aelfrice/pre_issue_create_hook.py, tests/test_pre_issue_create_hook.py. Resolve them on the PR (click 'Resolve conversation' on each) and re-add the label. The |
github-actions
Bot
removed
the
ready-to-merge
robotrocketscience
added
the
ready-to-merge
|
merge-train: blocked branch is not fast-forward on The |
github-actions
Bot
removed
the
ready-to-merge
Tokenizes issue titles (strips conventional-commit prefix, lowercases, drops stop-words), scores candidates via Jaccard similarity, and blocks gh issue create calls whose title overlaps an existing issue or merged commit above the 0.5 threshold. ALLOW_DUP_ISSUE=1 and AELFRICE_NO_PRE_ISSUE_GUARD=1 bypass the guard. Runners are injectable for unit testing.
Unit tests for tokenize_title (prefix stripping, stop-word removal), jaccard, score_candidate, and run_guard with mocked gh/git runners. Covers PASS/BLOCK paths, env-var overrides, false-positive guard, and the ~/.claude/ body-file refusal path.
Registers aelf-pre-issue-hook = aelfrice.pre_issue_create_hook:main in pyproject.toml so the guard ships as an installable console script alongside the other hook entry points.
Adds resolve_pre_issue_guard_command, install_pre_issue_guard_hook, uninstall_pre_issue_guard_hook to setup.py; wires them into _cmd_setup and _cmd_unsetup with --pre-issue-guard / --no-pre-issue-guard BooleanOptionalAction flags; registers the hook in auto_install and hook_manifest.json (default_on, since 3.4.0). Tests: test_aelf_setup_pre_issue_guard.py (unit + CLI) and two new opt-out-sync cases; existing search-tool-bash CLI tests scoped to --no-pre-issue-guard to remain single-hook assertions.
Adds the pre-issue-guard row to the hook bundle table in INSTALL.md, the --no-pre-issue-guard opt-out line to the code block, and a new CONFIG.md section covering the env-var overrides and how to opt out per-call or globally.
- Path(path_str).expanduser() so ~/.claude/ guard catches tilde-prefixed paths (was previously bypassed because Path.resolve() doesn't expand ~). - Document the three intentional except:pass sites with one-line comments explaining the fail-open intent (CodeQL: py/empty-except). - Drop unused 'json' import in test_aelf_setup_pre_issue_guard.py. - Drop unused 'BLOCK_THRESHOLD' import in test_pre_issue_create_hook.py.
Sourcery review flags subprocess.run() with dynamic argv as a potential shell-injection vector. The list-form (no shell=True) is structurally safe — argv is built from constants + tokens we tokenized ourselves — but the static-analysis flag blocks merge. Annotate the two call sites with # noqa: S603 and a one-line rationale.
robotrocketscience
force-pushed
the
feat/issue-941-pre-issue-create-guard
branch
from
June 4, 2026 21:45
5803c69 to
f0f1efa
Compare
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged f0f1efa → |
Owner
Author
|
[release:review:Kulili:2026-06-04T21:48:55Z] |
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
2 participants
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.
What
Implements #941: a Claude Code
PreToolUse:Bashguard that runs duplicate-detection beforegh issue createis executed. Closes the gap betweenaelf-pr-open.sh(which gatesgh pr create) andgh issue create(which had no analogous gate). The trap this closes is filing an issue describing behavior that already shipped under a different number — exactly the #929/#930→#781 incident the issue body cites.The guard is default-on (matching the 7 existing default-on hooks in the bundle), opt-out via
--no-pre-issue-guardataelf setuporAELFRICE_NO_PRE_ISSUE_GUARD=1in env, and emergency-bypass viaALLOW_DUP_ISSUE=1. Non-gh issue createcommands always PASS.Mechanism
PreToolUse:Bashevent. Iftool_name != "Bash"or the command does not start withgh issue create, exit 0 immediately.--title(and--body-filecontent if the path is a regular file outside~/.claude/— paths inside~/.claude/are silently refused).gh issue list --state all --search "<top-3 keywords>" --json number,title,state,stateReason,closedAtgit log --grep="<keyword|keyword|keyword>" --extended-regexp --oneline -20Per #605 PHILOSOPHY: pure deterministic surface — no embeddings, no LLM, no fuzzy heuristics beyond Jaccard. Per #606 hook convention: default-on, env opt-out, atomic install via
aelf setup.Smoke tests (live, against this branch)
Files touched
src/aelfrice/pre_issue_create_hook.py(new, 445 LOC) — module: tokenizer, Jaccard scorer,run_guard()with injectable runners for testing,main()with real subprocess runnerssrc/aelfrice/setup.py(+95 LOC) —PRE_ISSUE_GUARD_*constants,install_pre_issue_guard_hook,uninstall_pre_issue_guard_hook,resolve_pre_issue_guard_commandvia the_resolve_scripthelper that landed in bug: aelf:setup pins worktree-local venv path into user settings.json, leaving stale hook entries on worktree removal #928's refactorsrc/aelfrice/cli.py(+58 LOC) — wiring into_cmd_setup/_cmd_unsetup,--no-pre-issue-guardflag,_SETUP_FLAG_TO_HOOK_NAMEentrysrc/aelfrice/auto_install.py—pre_issue_guardentry in_HOOK_INSTALLERSsrc/aelfrice/data/hook_manifest.json— new entry (default_on: true,since: 3.4.0)pyproject.toml—aelf-pre-issue-hook = "aelfrice.pre_issue_create_hook:main"tests/test_pre_issue_create_hook.py(new, 52 tests) — tokenizer, scorer, run_guard with mocked runners, body-file path safety, env overridestests/test_aelf_setup_pre_issue_guard.py(new, 14 tests) — install / uninstall / CLI flag wiringtests/test_cli_setup_opt_out_sync.py,tests/test_aelf_setup_search_tool_bash.py— adjusted existing assertions for the new hook countdocs/user/CONFIG.md,docs/user/INSTALL.md— one-paragraph descriptionsAcceptance ↔ implementation map
aelf setup(default-on).--no-pre-issue-guardopts out.ALLOW_DUP_ISSUE=1 gh issue create ...→ PASS. (Smoke test above; covered in unit test.)fix(hooks): ...vsfix(retrieval): ...do not BLOCK at default threshold. (Covered intest_pre_issue_create_hook.py::test_false_positive_distinct_domain_passes.)Out-of-scope (per issue body)
Test plan
uv run pytest tests/test_pre_issue_create_hook.py tests/test_aelf_setup_pre_issue_guard.py tests/test_cli_setup_opt_out_sync.py -x -q— 71 passed.uv run pytest -x -q— 4950 passed, 68 skipped, 75 xfailed.%G? = G), conventional-commit prefixes, atomic.Closes #941.
Summary by Sourcery
Add a default-on pre-issue guard hook that detects potentially duplicate GitHub issues before creation and wire it into setup, auto-install, and CLI configuration, with supporting tests and documentation.
New Features:
PreToolUse:Bashhook (aelf-pre-issue-hook) that inspectsgh issue createcommands and blocks creation when the title closely matches existing issues or recent commits using deterministic Jaccard similarity.Enhancements:
Tests:
Summary by CodeRabbit
New Features
aelf setup --no-pre-issue-guardCLI option.Documentation