docs(source-control): give the nesting invariant one owner, an expiry, and a recorded fixture - #2349
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 21s —— View job Security review complete
Scope: all 10 files changed in this PR ( FindingsNo security issues found. What I checked, and why it's clean:
This PR's stated self-review ("no trust surface changes... The one new executable is an opt-in probe run by hand... The |
![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: e9604c7684
ℹ️ 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".
…, and a recorded fixture
The mechanism claim justifying a machine-wide worktree-placement rule enforced
by a fail-closed hook was restated as an undated absolute at 13 sites against
exactly two dated statements — and the one site that asserted 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. It is
one owner, twelve pointers, and a test that fails when a second site states the
mechanism.
Three further defects in the same paragraph:
- Both recheck triggers were structurally incapable of firing. #16600 has not
changed state since well before the as-of date, and an event-keyed trigger
cannot fire on an opaque release stanza. The stamp gains an unconditional
expiry (2.1.244 or 2026-11-07), composed with docs/conventions/upstream-drift
rather than a parallel mechanism.
- The measurement was disputed, not refuted, and neither the original run nor
its counter-reproduction recorded a fixture — so it was unadjudicable while
reading as settled. It now names the dispute, carries an arm-by-arm status
table (the nested-in-an-unrelated-repo arm is untested and NOT refuted), and
ships a probe pinning every discriminator neither run disclosed. The probe is
written and has NOT been run; that is stated at the top of the script and in
the fixture record, and nothing is claimed on its authority.
- "the single-string form silently never fires" contradicts the hooks docs,
which document both command forms with no event-specific carve-out — and this
plugin's own hooks.json uses the single-string form throughout. Replaced with
the documented args rule; the genuinely unprobed question is named as unknown.
The "no external prose doc" absolute was false and is now scoped, with an
inbound channel so a consumer that measures a contradiction can correct the
owner instead of diverging from it silently.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
e9604c7 to
714f92a
Compare
…2333) Fixes #2162. Adds `scripts/test-git-helpers.sh` with `git_init_safe` (refuses paths inside the current checkout) and `git_test_config` (per-invocation identity). Adopted across the six harnesses named in the issue. ## Related Fixes #2162. Shared git test harness adoption across plugin test scripts. --------- Co-authored-by: Cursor Agent <cursoragent@cursor.com> Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
…ption change Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
… with SSOT pointer Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
… D-F1/D-F6 fix #2332 shipped the same two corrections inside the old single-paragraph shape while this release restructures that paragraph into an owned section. Both of its corrections survive here; what 0.53.0 adds is D-F2, the fixture #2332 explicitly left open. Saying so keeps the two entries from reading as independent fixes of the same rows. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 55s —— View job Security review complete
Scope: all 16 files changed in this PR ( FindingsNo security issues found. What was checked
This PR's own stated self-review ("no trust surface changes... The |
|
Claude finished @kyle-sexton's task in 7m 17s —— View job Code review
Scope: all 16 files changed in this PR ( Posted 3 inline comments on lines I could anchor to. One more, cross-file, below. Findings1. 2. Same file, no positive control before trusting a "no" verdict (L122, inline comment posted) — the script guards the zero-trace case explicitly but doesn't confirm the triggering read actually happened (via the worktree's own rule producing a 3. The new SSOT test's coverage is narrower than the invariant it enforces (cross-file — 4. Cosmetic ( What's solid
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| # here as refuting arm A: they are different claims. | ||
| P4="$(mkparent "$WORKDIR/parentD" "$GLOB")" | ||
| UNREL="$(mkparent "$WORKDIR/unrelated" "$GLOB")" | ||
| arm unrelated-nested "$P4" "$UNREL/nested/wt" "src/target.md" |
There was a problem hiding this comment.
Arm D (unrelated-nested) checks the wrong repository, and its glob can't match its own target even if fixed.
arm() is called as arm unrelated-nested "$P4" "$UNREL/nested/wt" "src/target.md", so inside the function repo="$P4" (the worktree's own checkout) while the physically-enclosing, git-unrelated repo is $UNREL. But the verdict check at line 126 is:
if grep -qF "$repo/.claude/rules/scoped.md" "$TRACE_DIR/trace.jsonl"; then$repo/.claude/rules/scoped.md here is $P4/.claude/rules/scoped.md — a path that isn't even an ancestor of $wt on disk ($P4 = $WORKDIR/parentD, $wt = $WORKDIR/unrelated/nested/wt), so it can never appear in a trace for this arm regardless of whether the leak the arm exists to test for is real. The check that would actually adjudicate this arm is grep -qF "$UNREL/.claude/rules/scoped.md".
Even fixing that, the arm still can't pass: mkparent "$WORKDIR/unrelated" "$GLOB" writes paths: ["src/**"] into $UNREL/.claude/rules/scoped.md, anchored (per the doc's own "anchoring root" framing) at $UNREL. The read target is $wt/src/target.md = $UNREL/nested/wt/src/target.md, whose path relative to $UNREL is nested/wt/src/target.md — not matched by src/**. So arm D as written cannot produce a true positive no matter what the harness actually does.
This is the one arm the PR explicitly flags as "untested by anyone" and the fixture the SKILL.md stamp names as "the adjudicator" for it — as written, running it would misreport this arm as refuted (a clean "no") rather than surfacing a fixture defect, which is exactly the trap fixtures/README.md says the script guards against. Fix this →
| printf 'worktree target\n' >"$wt/$readpath" | ||
| ( cd "$wt" && claude -p "Read $readpath and reply with exactly: OK" \ | ||
| --settings "$SETTINGS" >/dev/null 2>&1 ) | ||
| if [[ -s "$TRACE_DIR/trace.jsonl" ]]; then |
There was a problem hiding this comment.
A nonempty trace isn't proof the triggering read happened — no positive control before reading a "no" verdict.
claude -p ...'s exit status is discarded (line 120-121), and the only gate before trusting a "PARENT rule loaded? no" verdict is [[ -s "$TRACE_DIR/trace.jsonl" ]] — i.e. some InstructionsLoaded event fired. InstructionsLoaded fires at session start regardless of whether the model goes on to actually read $readpath (it may decline, error, or answer without invoking the Read tool), so a nonempty trace can come entirely from session-start instructions loading, with the paths:-glob-triggered read that's the actual subject of the test never having occurred.
The script explicitly guards the zero-trace case as "a fixture failure, not evidence of absence" (lines 131-136), but doesn't extend that same skepticism to a nonzero-but-incomplete trace — it never confirms the worktree's own scoped.md rule produced a path_glob_match (a positive control) before accepting the parent's absence as a real "no". Since this arm's "no" is the one that would settle the disputed measurement in SKILL.md § "The nesting invariant, verified", a false "no" here would silently downgrade a disputed claim to refuted.
| # the helper's core purpose — see skills/worktree/SKILL.md § "The nesting | ||
| # invariant, verified" for the measured claim, and note that a worktree dropped | ||
| # inside a .git or bare directory additionally mixes the checkout into git | ||
| # metadata, which is a separate and undisputed reason to refuse. The root resolution above does not catch a root explicitly |
There was a problem hiding this comment.
Minor: this line runs on for ~130 columns, folding "...a separate and undisputed reason to refuse." together with the next sentence ("The root resolution above does not catch a root explicitly") that used to start its own wrapped line. Every other line in this comment block wraps at ~80-90 columns — looks like an edit artifact from the rewrite rather than intentional. Not functionally significant (it's a # comment), just a readability nit.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
2 participants
Summary
skills/worktree/SKILL.mdcarries 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.mdnow carries the claim under an explicit### The nesting invariant, verifiedheading 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:
#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:
#16600has 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/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.shpins every discriminator neither original run disclosed: creation mechanism, launch mode, the exactpaths: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.#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
argswhenever the hook references a path placeholder, since each element is passed as one argument with no quoting." This plugin's ownhooks/hooks.jsonregisters 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 anInstructionsLoadedhook supplied viaclaude -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
mktempworkdir and creates throwaway repos. The.claude-plugin/plugin.jsonedit is a description string only — no newuserConfigkey, no default changed.Test plan
New test, on this branch:
Fails before the change — the same test against the pre-change docs (stashed doc edits):
No regression in the neighbouring suite (0.52.0's gate work is now on
main):Repo gates, run locally on the rebased branch:
Version basis re-derived, not remembered.
mainat4c90b454carriessource-control0.52.0; this change adds behavior-adjacent doc surface and a new test, so 0.53.0. The0.53.0on the pre-rebase branch was computed against a superseded rebase and was re-checked againstmain, not carried forward on trust.Related
Closes #2213
#2212 was closed by #2332 while this branch was in flight, covering only its
D-F1andD-F6rows — that PR says so itself ("D-F2 (fixture recording) remains open"). This PR carriesD-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: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 nonce20260811T023241Z), 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).
Reviewer note, worth verifying rather than trusting: this branch was rebased onto a fast-moving
main. A conflicted merge ref suppresses thepull_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.