fix(source-control): make the worktree-create gate's failures legible and its opt-out honest - #2309
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. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1c52101037
ℹ️ 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".
1c52101 to
32110be
Compare
|
Claude finished @kyle-sexton's task in 1m 22s —— View job Security Review
Scope reviewed ( No GitHub Actions workflow files are touched by this PR, so the No security issues found. Specifically checked and clear:
Net effect of this PR, as the author states, is strictly more fail-closed (disabled path moved from exit 0 → exit 1); no new trust surface, network read, hook, or grant was introduced. |
32110be to
f15be4f
Compare
|
Claude finished @kyle-sexton's task in 1m 56s —— View job Security Review
Scope reviewed ( No security issues found. This diff is message-text plus one exit-status bugfix; the control-flow paths that matter for a fail-closed guarantee were specifically checked:
No new hook, grant, network read, command-injection surface, or file write outside existing paths was introduced. |
|
Claude finished @kyle-sexton's task in 5m 7s —— View job Code Review
Scope: this is the code-review lane per FindingsOne inline comment posted, on What I verified directly (not just author-claimed)
Not verified (sandbox restrictions)I was not able to execute shell commands beyond No other correctness, error-handling, or maintainability issues found. The |
f15be4f to
7db1318
Compare
|
Claude finished @kyle-sexton's task in 2m 12s —— View job Security Review
Scope reviewed (HEAD No CRITICAL or IMPORTANT issues found. This diff is message-text plus one exit-status bugfix, and the control-flow paths that matter for the fail-closed guarantee hold:
Two low-severity notes, informational only (not blocking):
Net effect of this PR, consistent with the author's own note: strictly more fail-closed (disabled path exit 0 → exit 1), no new hook, grant, network read, or file write outside existing paths. |
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
19bb43b to
6d9b693
Compare
6d9b693 to
8e9548e
Compare
|
Claude finished @kyle-sexton's task in 2m 6s —— View job Security Review
Scope reviewed: exactly the 9 files this PR touches — No CRITICAL or IMPORTANT security issues found. This diff is message-text plus one exit-status correction. The fail-closed control flow that matters holds end-to-end:
One SUGGESTION, LOW confidence (informational only, not blocking):
Net effect of this PR, consistent with the author's own note: strictly more fail-closed (disabled path exit 0 → exit 1), no new hook, grant, network read, command-injection surface, or file write outside existing paths. Outside this lane's scope (flagging only for visibility, not as a security finding): |
8e9548e to
515416d
Compare
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
The check is green on purpose, and it is not evidence. It certifies that a security pass ran, and this one did not complete — but the cause is outside this PR's control, so merging is deliberately left unblocked rather than locking every merge for the length of the outage. Nothing was reviewed at this head. Where this check is required, it is satisfied without that evidence; a human should review security-sensitive changes here before merging. Re-run the job to retry the review; a new push also retries it only if the caller's Re-running does NOT help for every class:
|
… and its opt-out honest
The gate reported a constant `exited 0` for every helper failure: `status=$?`
sat inside the body of `if ! path="$(…)"`, where `$?` is the status of the
negated compound — 0 exactly when the command failed. That constant is what
produced the "a hook exited 0 while failing" theory a whole verification pass
had to unwind. The assignment now stands alone, and the helper's documented
0/2/3/4 taxonomy is translated into distinct messages instead of being
discarded, so "not a repository", "no worktree_root configured" and "illegal
branch name" are no longer one indistinguishable line. Every refusal leads with
a remedy. An empty stdin buffer is now its own message rather than being
misreported as a payload missing `.name`.
Separately, `worktree_create_gate_enabled=false` did not do what it said.
Measured on Claude Code 2.1.228: a WorktreeCreate hook that exits 0 without
printing a path FAILS the creation ("hook succeeded but returned no worktree
path"), and an exit-0 hook's stderr is dropped. So the opt-out produced the same
outcome as a refusal while suppressing every explanation. It now refuses out
loud and names the real stand-downs — `worktree.bgIsolation: "none"`, or
disabling the plugin. The four-arm probe behind all of this ships as a runnable
fixture with an as-of stamp and a recheck trigger.
Corrected mechanism vs the filed issue: #2209 assumed the transcript surfaces
only the FIRST stderr line. It surfaces all of it. Remedy-first still holds as a
readability argument, not a truncation one.
Both worktree suites also gained `commit.gpgsign false` on their throwaway repo
fixtures; without it they are unrunnable on a machine that signs by default.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e4b1b93 to
24f8110
Compare
…, and a recorded fixture (#2349) ## Summary `skills/worktree/SKILL.md` carries the claim that justifies a machine-wide worktree-placement rule enforced by a fail-closed hook. Four defects sat in it. ### #2213 — the claim was restated as an undated absolute at 13 sites Against exactly two dated statements. Worse, the one site asserting freshness — "It is the live constraint, not a historical one" — was itself undated, so a pointer landed the reader precisely there. **The fix is not thirteen updated copies.** `SKILL.md` now carries the claim under an explicit `### The nesting invariant, verified` heading and every other surface points at it. Sites converted: `SKILL.md:54`/`:106`, `.claude-plugin/plugin.json`, `README.md` (regenerated from the manifest), `context/create.md` ×3, `scripts/worktree-create.sh` ×5, `hooks/worktree-create-gate.sh`. A new test, `skills/worktree/nesting-invariant-ssot.test.sh`, is what stops the re-drift: it fails when a second site states the mechanism, so the next person to explain it in place has to point instead. **Two deliberate deviations, stated so neither reads as an oversight:** 1. **Pointers do not restate the as-of date**, which the issue's fix sketch asks for. Twelve restated dates are twelve drift sites — the defect being removed. Pointers instead say the claim is dated and measured, and name the section carrying the stamp. One stamp, one place to refresh. 2. **The two exit-3 heredocs keep a short restatement** alongside their pointer. They are read at the moment creation fails, when the reader cannot go follow a link; a pointer-only refusal there would be a regression. Both restatements are deliberately non-causal ("can pick up") rather than the absolute the sweep removed. ### #2213 — both recheck triggers were structurally incapable of firing The claim was guarded by "a release note naming worktree rule-file loading" **or** "upstream #16600 changing state". Neither can fire: `#16600` has not changed state since well before the 2026-08-07 as-of date, and an event-keyed trigger cannot fire on an opaque stanza (2.1.226 is entirely "Bug fixes and reliability improvements"). The most consequential claim in this plugin was guarded by two triggers that could not go off. The stamp now adds an **unconditional expiry — 2.1.244 or 2026-11-07, whichever comes first** — composed with [`docs/conventions/upstream-drift/`](../blob/main/docs/conventions/upstream-drift/README.md) rather than inventing a parallel mechanism, and states *why* the event triggers cannot carry it alone so a later reader does not delete the expiry as redundant. The test pins that reasoning in place. ### #2213 — the ownership claim was a false absolute with no back-channel "This skill is the canonical owner … — **no external prose doc**" was untrue: a consumer doc outside this repository defers mechanism to this skill *and* is more current than it. Ownership is now scoped to this plugin fleet, and states how a consumer who measures a contradiction gets that correction back into the owner. Per the issue's scope correction, the consumer is a machine-local file outside this repository and **is not touched**. ### #2212 — the measurement was disputed, read as settled, and had no fixture The 2.1.224 leak measurement was **disputed, not refuted** (a 2.1.227 counter-reproduction did not observe it), and *neither run recorded its fixture* — so the two results could not be compared and the claim was not adjudicable, while reading as settled. Now: the section names the dispute, and carries an **arm-by-arm status table** so a fix to one arm cannot silently weaken another. Per the issue's explicit instruction, the **nested-in-an-unrelated-repo** arm is recorded as *untested by anyone and NOT refuted* — the dispute does not reach it. `fixtures/nesting-invariant-probe.sh` pins every discriminator neither original run disclosed: creation mechanism, launch mode, the exact `paths:` glob **and its anchoring root**, whether the parent's rule file was committed, and the three placements as separate arms. It also guards the trap that most likely produced the dispute: zero trace events means *the hook did not fire* — a fixture failure, not evidence of absence — and the script says so rather than printing a null. > **The probe is written and has NOT been run.** That is stated at the top of the script, in `fixtures/README.md`, and here. Nothing in this PR is claimed on its authority. It converts a recheck *trigger* into a recheck *procedure*; running it is what would settle the arm. ### #2212 — the reproduction guidance contradicted the hooks docs It claimed the single-string command shape "silently never fires". <https://code.claude.com/docs/en/hooks> (raw markdown, fetched 2026-08-11) documents **both** command forms with no event-specific carve-out, and the documented rule is narrower: "Set `args` whenever the hook references a path placeholder, since each element is passed as one argument with no quoting." This plugin's own `hooks/hooks.json` registers all three of its hooks in the single-string form and they fire. Replaced with the documented rule; the genuinely unknown part is now named as unknown — whether the single-string form fires for an `InstructionsLoaded` hook supplied via `claude -p --settings <file>` is **unprobed by anyone**. ### #2212 — the version basis read as a release fact "which 2.1.224 already handles correctly" sat several sentences from the only "Basis:" clause and had already been misread as a version fact by two independent readers. The basis is now inlined at the claim: it is a **null result from the same trace**, not a release note, and the changelog scan behind it is packet-sourced and has not been re-run — said so explicitly. **Security review:** no trust surface changes. No new hook, grant, or network read. The one new executable is an opt-in probe run by hand; it writes only under a `mktemp` workdir and creates throwaway repos. The `.claude-plugin/plugin.json` edit is a description string only — no new `userConfig` key, no default changed. ## Test plan New test, on this branch: ``` $ bash plugins/source-control/skills/worktree/nesting-invariant-ssot.test.sh PASS: [1] the undated absolute form of the claim appears nowhere PASS: [2] the measurement is stated at exactly one site PASS: [3] that site is the owner PASS: [4] the owner carries the anchor every pointer cites PASS: [5] the owner carries an as-of date PASS: [6] the owner carries an unconditional expiry, not only event triggers PASS: [7] the expiry states why the event triggers cannot carry this alone PASS: [8] the owner names the arm as disputed rather than settled PASS: [9] the owner points at the fixture that would adjudicate it PASS: [10] the former restatement sites cite the owner by name (7 files) # rc 0 ``` Fails before the change — the same test against the pre-change docs (stashed doc edits): ``` FAIL: [1] the undated absolute form of the claim appears nowhere — expected 0 got 6 sites: .claude-plugin/plugin.json README.md hooks/worktree-create-gate.sh scripts/worktree-create.sh skills/worktree/SKILL.md skills/worktree/context/create.md FAIL: [4] the owner carries the anchor every pointer cites — expected "### The nesting invariant, verified" ``` No regression in the neighbouring suite (0.52.0's gate work is now on `main`): ``` $ bash plugins/source-control/hooks/worktree-create-gate.test.sh rc=0 pass=34 fail=0 ``` Repo gates, run locally on the rebased branch: ``` $ python3 scripts/sync-plugin-options-docs.py --check plugin options docs: up to date $ bash scripts/check-changelog-parity.sh --check # rc 0 $ bash scripts/check-changelog-parity.sh --check-order All 76 changelog(s) read newest-first with no duplicate versions. $ bash scripts/check-changelog-parity.sh --check-bump origin/main Every plugin whose version changed vs origin/main has a '## [<version>]' CHANGELOG.md entry. $ npx markdownlint-cli2 "**/*.md" Linting: 1095 files Summary: 0 issues in 0 files $ shellcheck -x -f gcc <both new shell files> # rc 0, no findings $ bash scripts/check-shell-portability.sh --paths <both new shell files> No unexcused GNU-only constructs in 2 shell file(s). $ git grep -nE '^(<<<<<<< |>>>>>>> |\|\|\|\|\|\|\| )' -- plugins/ scripts/ docs/ (no output — checked before pushing, per the conflicted-merge-ref hazard below) ``` **Version basis re-derived, not remembered.** `main` at `4c90b454` carries `source-control` **0.52.0**; this change adds behavior-adjacent doc surface and a new test, so **0.53.0**. The `0.53.0` on the pre-rebase branch was computed against a superseded rebase and was re-checked against `main`, not carried forward on trust. ## Related Closes #2213 **#2212 was closed by #2332 while this branch was in flight, covering only its `D-F1` and `D-F6` rows** — that PR says so itself ("D-F2 (fixture recording) remains open"). This PR carries **`D-F2`**, the row it left open, plus the same two corrections restated inside the restructured owner section. Both of #2332's corrections were verified present here before the rebase resolution was taken: ``` $ grep -c "null result from this same trace" …/SKILL.md # 1 (D-F1) $ grep -c "Set \`args\` whenever the hook references a path placeholder" …/SKILL.md # 1 (D-F6) $ grep -n "silently never fires" …/SKILL.md 75: … An earlier version of this section claimed … "silently never fires"; that is not what the docs say # the falsified claim survives only inside its own refutation ``` The 0.53.0 CHANGELOG entry records the supersession so the two releases do not read as independent fixes of the same rows. Inbox item: `2026-08-10-plugin-quality-audit-four-components` (packet nonce `20260811T023241Z`), ledger `.work/handoff-inbox-batch-4/ledgers/I7-four-components-023241Z.md` § "Lane D — `source-control:worktree`". Adjacent, not touched: #1971 (whether to file an upstream issue for nested-worktree rule inheritance — same claim, different question; this PR makes its status legible), #2208 (upstream draft), #2309 (merged — 0.52.0, the gate work these pointers sit alongside). <sub>Reviewer note, worth verifying rather than trusting: this branch was rebased onto a fast-moving `main`. A conflicted merge ref suppresses the `pull_request`-triggered workflow runs entirely, so a PR can display **3 checks, all passing** instead of ~35. Compare the check *count* against a known-good PR before reading this as green — "0 failures" is not sufficient on its own.</sub> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…es fixtures (#2368) Fixes #2358 ## Summary - Set `commit.gpgsign false` repo-locally in `make_repo` so fixture setup does not attempt GPG signing on machines with global `commit.gpgsign=true`. - Matches the pattern already used in `worktree-create-gate.test.sh` and `scripts/test-git-helpers.sh`. ## Test plan - [x] `python3 -m unittest tests.test_prune_babysit_worktrees` (45/0) ## Related - #2309 — same class of defect fixed in shell suites - #2333 — shared `test-git-helpers.sh` contract this mirrors Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
Summary
Two issues in one file pair, both about what a stuck user actually sees when a
WorktreeCreatefails.#2209 — the failure output.
status=$?sat inside the body ofif ! path="$(bash "$helper" …)", where$?is the status of the negated compound —0exactly when the command failed. Every failure therefore reportedexited 0. Proven directly:That constant is what produced the "a hook exited 0 while failing" theory a whole verification pass had to unwind. The assignment now stands alone, and the helper's documented
0/2/3/4taxonomy is translated into distinct messages instead of being discarded — "not a repository", "noworktree_rootconfigured" and "illegal branch name" were previously one indistinguishable line. Every refusal now leads with a remedy and follows with the diagnosis.hook::buffer_stdin's status is checked, so an empty/unbufferable payload is no longer misreported as "the payload carried no.name".Corrected mechanism. #2209 was filed on the premise that the transcript surfaces only the first stderr line. Measured on 2.1.228, a failing hook's stderr is surfaced in full:
Remedy-first still holds — it is the line a reader acts on — but as a readability argument, not a truncation one. The CHANGELOG states the corrected version.
#2211 — the opt-out asserted an unmeasured harness behavior, and it was false. The issue said one
claude --worktreein a scratch repo would settle it. It was run — four arms, on Claude Code 2.1.228:<repo>/.claude/worktrees/probe0, branchworktree-probe0, locked (claude session probe0 (pid 29884))exit 0, no stdoutexit 3, two stderr linesSo
worktree-create-gate.sh:46-47's header ("Disabled means 'let Claude Code use its own default', which is exit 0 with an empty stdout") and the two suite assertions at:131-132were both false. The old exit-0 path produced the same outcome as a refusal while suppressing every explanation, because an exit-0 hook's stderr is dropped — arm 2's stderr marker is absent from the harness output while arm 3's two lines are present. The option only became reachable at 0.51.7 (#2193 declared it inuserConfig), so this is the first release anyone could hit it.The disabled path now exits non-zero and names the real stand-downs:
worktree.bgIsolation: "none", or disabling the plugin.plugin.jsonandREADME.mdsay the same.The docs also agree now, contrary to what #2211 recorded. Raw-markdown fetch of
https://code.claude.com/docs/en/hooks.md(2026-08-11) carries "Hook failure or missing path fails creation" and "If the hook fails or produces no path, worktree creation fails with an error." #2211 recorded those sentences as unreachable; at the current revision they are reachable and they match the measurement.The probe is now a recorded, runnable fixture —
skills/worktree/fixtures/worktree-create-hook-probe.shplus aREADME.mdcarrying the outcome, verbatim harness strings, doc quotes, an as-of stamp (2026-08-11, 2.1.228) and a recheck trigger, composed withdocs/conventions/upstream-drift/. A recheck is one command, not a re-derivation.Not changed, deliberately:
hooks.jsonregistration scope (#2211'sSC-F1).WorktreeCreatesupports no matcher — "no matcher support / always fires on every occurrence" — so registration is the only seam and there is nothing narrower to register. Per AD-4 the non-repo outcome is upstream (the harness skips isolation and edits in place), and the issue itself notes a change here would not be fleet-complete since other installed plugins may register the same event. Filed-not-fixed with that reason, not dropped.Also fixed, in files already being edited: both worktree suites set a throwaway git identity but not
commit.gpgsign false, so on a machine that signs by default every fixture commit failed and the suites reported their creation cases as failures while their refusal cases still passed — a shape that reads as a regression rather than an unrunnable fixture.worktree-create.test.shgoes 94/154 → 154/154.Security review: no new trust surface. No new hook, grant, network read, or file write outside existing paths. The change is strictly message text plus one exit-status correction, and it moves the disabled path from exit 0 to exit 1 — i.e. more fail-closed, never less. The new fixture script is opt-in and run by hand.
Test plan
New assertions fail against the pre-change code and pass after — verified by checking the merge-base (
a0abaf81) copies of both scripts back in and re-running:After the change:
Lint and repo hygiene:
The
silent-skip-ok:marker was removed along with the exit-0 skip it justified;check-silent-skips.shconfirms nothing now needs one.Related
Closes #2209
Closes #2211
Inbox item:
20260811-021645-plugin-audit-four-components-and-guard-deadlock-ownership(ledger.work/handoff-inbox-batch-4/ledgers/I9-021645-four-components.md, § "Lane D —source-control/worktree-create-gate").Settles DQ-6 from
.work/handoff-inbox-batch-4/RECONCILE.md: the oneclaude --worktreecommand was run, and exit-0-with-no-path fails creation.Adjacent, not touched here: #2146 (
hook::require_jqfail-open in the same utility file — different function), #1971 (upstream coverage of nested-worktree rule inheritance), #2208 (the upstream draft for the non-repo isolation divergence).