fix(hook-utils): scope read_file_path to git worktrees when project dir unset - #2450
Conversation
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
Claude finished @kyle-sexton's task in 1m 57s —— View job Security Review
Scope: What changed: Findings: none. Specifically checked and found sound:
No CRITICAL, IMPORTANT, or SUGGESTION-level security issues identified in this PR's diff. |
![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: baa7140ae6
ℹ️ 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".
|
Claude finished @kyle-sexton's task in 4m 34s —— View job Code Review
Reviewed the 18 changed files ( The core change itself is sound: 🔴 Blocking — carrying plugins were not version-bumped, and CI is red because of it.
Test coverage gap (Suggestion, inline on Nit (inline): the new "Test 12e" sorts before pre-existing "Test 12d" in the file, breaking the otherwise-sequential test labeling. One other CI failure on this PR ( |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 5 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
e9026c9 to
04ba21a
Compare
…ckRollup running-check pitfall (#2513) ## Summary Lands the two PR-lifecycle facts #1436 recorded from driving #1393/#1316/#1318/#1322 to merge, in the document that was prescribing the expensive path: - **Stale-branch recovery now defaults to merge-forward.** `monitor.md`'s mergeable pre-check (§3.1 step 1) and stale-branch recovery rule (§3.2) both prescribed "force-push with lease" — but auto-mode permission classifiers commonly deny force-push, and the recorded consequence was a fresh branch + fresh PR + closing the old one per rebase (the #1315 → #1377 → #1393 churn, with every review thread re-opened on the successor). Merging the default branch *into* the PR branch resolves staleness and pushes **fast-forward** — no force-push — and under a squash-only default branch the merge commits collapse to one commit on merge, so linear-history requirements stay satisfied. Verified in the issue's own record: #1393 landed that way and #1318 was merge-forwarded five times without needing a new branch. Rebase stays available as the exception for projects requiring a linear PR branch where force-push is actually permitted. - **`statusCheckRollup` reports a running check as `conclusion: ""` (empty string), not `null`.** The complement-shaped filter (`conclusion != null and != "SUCCESS"`) therefore counts every in-progress check as a failure — the exact misreport in the issue (two "failing" checks that were simply still running). The multi-PR scan section (§3.0.6, the one place this skill reads `statusCheckRollup`) now documents the pitfall with value-positive jq selectors for "failed" and "still running". Version `0.53.11` → `0.53.14` (patch; `0.53.12`/`0.53.13` are claimed by in-flight PRs #2450/#2453/#2483/#2510 and #2469 — skipping past them per the #1746 collision pattern). ## Test plan - `npx markdownlint-cli2@0.23.2` on both edited markdown files — 0 issues. - Docs-only change to skill reference text; no scripts or hooks touched. The jq forms added are the ones from the issue, verified against `gh pr view --json statusCheckRollup` semantics. ## Related Fixes #1436 Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…ckRollup running-check pitfall (#2513) ## Summary Lands the two PR-lifecycle facts #1436 recorded from driving #1393/#1316/#1318/#1322 to merge, in the document that was prescribing the expensive path: - **Stale-branch recovery now defaults to merge-forward.** `monitor.md`'s mergeable pre-check (§3.1 step 1) and stale-branch recovery rule (§3.2) both prescribed "force-push with lease" — but auto-mode permission classifiers commonly deny force-push, and the recorded consequence was a fresh branch + fresh PR + closing the old one per rebase (the #1315 → #1377 → #1393 churn, with every review thread re-opened on the successor). Merging the default branch *into* the PR branch resolves staleness and pushes **fast-forward** — no force-push — and under a squash-only default branch the merge commits collapse to one commit on merge, so linear-history requirements stay satisfied. Verified in the issue's own record: #1393 landed that way and #1318 was merge-forwarded five times without needing a new branch. Rebase stays available as the exception for projects requiring a linear PR branch where force-push is actually permitted. - **`statusCheckRollup` reports a running check as `conclusion: ""` (empty string), not `null`.** The complement-shaped filter (`conclusion != null and != "SUCCESS"`) therefore counts every in-progress check as a failure — the exact misreport in the issue (two "failing" checks that were simply still running). The multi-PR scan section (§3.0.6, the one place this skill reads `statusCheckRollup`) now documents the pitfall with value-positive jq selectors for "failed" and "still running". Version `0.53.11` → `0.53.14` (patch; `0.53.12`/`0.53.13` are claimed by in-flight PRs #2450/#2453/#2483/#2510 and #2469 — skipping past them per the #1746 collision pattern). ## Test plan - `npx markdownlint-cli2@0.23.2` on both edited markdown files — 0 issues. - Docs-only change to skill reference text; no scripts or hooks touched. The jq forms added are the ones from the issue, verified against `gh pr view --json statusCheckRollup` semantics. ## Related Fixes #1436 Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
04ba21a to
cb48d95
Compare
|
Claude finished @cursor[bot]'s task in 1m 6s —— View job Security Review
Skip gate hit: condition 3 (this head already has a successful security review that still applies). I compared the actual PR diff ( Per the skip gate, no new findings to report — nothing else to add. The prior review's conclusion stands: no CRITICAL, IMPORTANT, or SUGGESTION-level security issues in this PR's diff. |
|
Last security-reviewed head: |
8e8f910 to
b356ab8
Compare
|
Claude finished @cursor[bot]'s task in 1m 26s —— View job Security Review
Skip gate hit: condition 3 (this head already has a successful security review that still applies). The branch was rebased onto the latest
Per the skip gate, no new findings to report. The prior review's conclusion stands: no CRITICAL, IMPORTANT, or SUGGESTION-level security issues in this PR's diff — the new |
|
Claude finished @cursor[bot]'s task in 2m 30s —— View job Code Review
Skip gate hit: condition 4 (this head already has a successful review from this lane that still applies). I diffed the current head ( Status of the prior pass's findings, re-verified against the current diff:
Not re-litigating these since the underlying lines haven't changed — the existing inline threads from the earlier pass on this commit lineage still apply and don't need duplicates. One item worth a note: at the time of the earlier pass, No new findings from this lane. · branch |
…ir unset (#1091) Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
b356ab8 to
5f1dd20
Compare
|
Claude finished @cursor[bot]'s task in 1m 30s —— View job Security Review
Skip gate hit: condition 3 (this head already has a successful security review that still applies). Current head is
Per the skip gate, no new findings to report. The prior conclusion stands: no CRITICAL, IMPORTANT, or SUGGESTION-level security issues in this PR's diff — |
|
Claude finished @cursor[bot]'s task in 3m 23s —— View job Code Review
Skip gate: partially applies. 🔴 Prior blocking finding — resolved. The earlier code-review pass (and Codex) flagged that none of the 17 carrying plugins had bumped New finding (inline on lib/hook-utils.sh:446): the new worktree-scoping branch only runs Still open from the prior review (unchanged content, so not re-posted as duplicates — see the existing inline threads on this PR):
None of these are blocking on their own — they're edge-case robustness/coverage gaps, not correctness regressions in the primary path. |
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…_DIR read_file_path now rejects out-of-tree scratch files when the project dir is unset (#1091). Point each case at its own project root so the hook still runs. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 1m 8s —— View job Security Review
Skip gate hit: condition 3 (this head already has a successful security review that still applies). Current head is
Per the skip gate, no new findings to report. The prior conclusion stands: no CRITICAL, IMPORTANT, or SUGGESTION-level security issues in this PR's diff — (Note: an earlier code-review pass on this branch — a separate lane, not security — flagged a robustness gap where the worktree check falls through to unscoped behavior if |
Without git on PATH, read_file_path no longer falls through to the unscoped pre-#1091 behavior. Test 12f covers the rejection path. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 1m 12s —— View job Security Review
Skip gate: does not apply — new security-relevant code since the last review. The last successful security review on this PR covered up to commit What changed: this commit directly resolves the fail-open gap flagged by the earlier code-review pass on this PR (and by Codex): previously, when Findings: none. This is a strict tightening of the existing behavior — it removes the one remaining fail-open edge case in
No CRITICAL, IMPORTANT, or SUGGESTION-level security issues identified in this PR's diff. |
read_file_path now rejects files when git is unavailable and CLAUDE_PROJECT_DIR is unset. Update the git-absence contract cases to match. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 5m 26s —— View job Code Review
Skip gate: partially applies. This lane's last full pass reviewed head
🟠 New finding (posted inline on
Non-blocking observation: No other findings in the new commits. |
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 5m 21s —— View job Code Review
Skip gate: partially applies. The last code-review pass on this PR reviewed up to commit 🔴 Blocking (new, inline on Suggestion (new, inline on Still open from earlier passes on this PR (unchanged content in
Separately reviewed and found sound (no issues): the |
|
Claude finished @cursor[bot]'s task in 1m 11s —— View job Security Review
Skip gate hit: condition 3 (this head already has a successful security review that still applies). Current head is
Per the skip gate, no new findings to report. The prior conclusion stands: no CRITICAL, IMPORTANT, or SUGGESTION-level security issues in this PR's diff — |
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 59s —— View job Security Review
Skip gate hit: condition 3 (this head already has a successful security review that still applies). Current head is
Per the skip gate, no new findings to report. The prior conclusion stands: no CRITICAL, IMPORTANT, or SUGGESTION-level security issues in this PR's diff — (Note: the still-open Test 12f coverage gap flagged in an earlier review pass — the |
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 1m 1s —— View job Security Review
Skip gate hit: condition 3 (this head already has a successful security review that still applies), reinforced by condition 2 for what's new. Current head is
Per the skip gate, no new findings to report. The prior conclusion stands: no CRITICAL, IMPORTANT, or SUGGESTION-level security issues in this PR's diff — (Note: the still-open Test 12f coverage gap flagged in an earlier review pass — the |
Fix test 12f so PATH hides git for the read_file_path consumer (not only the pipeline producer) while keeping jq available, and assert rc=1 on an in-repo file. Rewrite the markdown-format git-absent in-repo symlink commentary to match fail-closed expectations. Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
|
Claude finished @cursor[bot]'s task in 55s —— View job Security Review
Skip gate hit: condition 3 (this head already has a successful security review that still applies). Current head is
Per the skip gate, no new findings to report. The prior conclusion stands: no CRITICAL, IMPORTANT, or SUGGESTION-level security issues in this PR's diff — |
|
Review threads addressed in 0105962:
All three review threads resolved. Awaiting CI then merge. |
…2522) (#2530) Fixes #2522 ## Summary The `install_new` userConfig paragraph in `plugins/claude-ops/skills/plugins/SKILL.md` embedded `${user_config.install_new}` inside explanatory prose about the unset state. Claude Code substitutes every occurrence, so when the key is set (e.g. to `all`) the paragraph falsely claims it still renders as the literal placeholder — right after the **Configured value** line already shows the real value. This PR rewrites the explanation so only the **Configured value** line keeps the live `${user_config.install_new}` token. Surrounding sentences describe the placeholder by shape (`${user_config.…}`) or by reference to that line, staying readable whether the key is set or unset. Bumps `claude-ops` to `0.31.10` (0.31.9 reserved for in-flight hook-utils sync on #2450). ## Test plan - [x] No automated tests cover this prose contract. - [x] Manual review: unset render — **Configured value** line shows the placeholder token; explanatory prose does not embed a second live `${user_config.install_new}`. - [x] Manual review: set render (e.g. `all`) — **Configured value** line shows `all`; explanatory prose still correctly describes unset behavior. ## Related - #2522 Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
2 participants
Closes #1091
Summary
Scope
read_file_pathto git worktrees when the project directory is unset.Related
N/A