feat(claude-config): give audit-pass an executable for the run state it specified - #2441
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 3m 9s —— View job Security review
This PR is entirely shell scripts and Markdown (no GitHub Actions workflow changes, so the zizmor-covered lanes — unpinned actions, dangerous triggers, excessive Scope reviewed: IMPORTANT (confidence: plausible) —
|
![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: dd3b010d76
ℹ️ 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".
…it specified `audit-pass` specified a lease (path, refresh discipline, two-sided liveness window, `released` tombstone, `owner_epoch` fencing) and an append-only partial across `SKILL.md` and ten `reference/*.md`, and shipped no `scripts/` directory at all — the only audit skill in the plugin without one. Sharper than "no scripts": `lib/state-key.sh`, whose header records the keying scheme as `audit-pass`'s "reused rather than reinvented", was called by three other skills and never by the one that specified it. Adds `skills/audit-pass/scripts/run-state.sh` — `paths`, `lease acquire|heartbeat|release|classify`, `partial append` — with `run-state.test.sh` carrying three negative tests that delete one check from a copy of the script and assert the mutated copy reaches the outcome the real one refuses. Corrects the two clauses a skill-driven run cannot keep: the 60-second wall-clock heartbeat (a skill has no timer) becomes boundary-driven refresh with the thresholds recorded in the lease, and §5's separate "run manifest" becomes the partial's own lane records, which is what §7 already required. Phase 4's `open` terminator is now appended when the handoff is recorded, so the report's `--resume` instruction has an artifact to attach to. Stale-lease adoption and §7 assembly stay unimplemented, and §3/§7 now say so — a contract that reads as enforced while nothing enforces it is the defect. Refs #2280 (F3, F5, F12, F13) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…surfaces Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
3506d53 to
afc3486
Compare
|
Claude finished @kyle-sexton's task in 3m 2s —— View job Security review
Scope reviewed: IMPORTANT (confidence: plausible) —
|
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 24s —— View job Security review
Scope reviewed: IMPORTANT (confidence: plausible) —
|
|
Claude finished @kyle-sexton's task in 6m 31s —— View job Code Review (correctness / maintainability lane)
This repo has a Note on verification: Important — the applying lock is unimplemented, and unlike the two clauses this PR does disclose, that's not stated anywhere§3 specifies two run-state mechanisms: the lease (which this PR implements: What makes this worth flagging rather than out-of-scope-and-fine: the PR is careful to name the two things it deliberately leaves unimplemented — stale-lease adoption and §7 assembly — in both the script header and a new "What is executable here, and what is not" paragraph it adds right into this same reference doc (reference/run-state-and-resumability.md#L96-L103). That disclosure paragraph is scoped to "everything below" it — i.e. the lease subsection — and the lock, described just above it in the same §3, gets no equivalent statement anywhere in this diff or pre-existing text. SKILL.md still just says "an applying run takes an exclusive advisory lock and refuses rather than queues" (SKILL.md#L134) with no note that this, too, has no executable behind it. This is pre-existing (the lock was never implemented before this PR either), so it's not a regression — but this diff is the one that establishes the "declare what you don't implement" norm in this exact section, and the lock is the one adjacent mechanism the norm isn't applied to. Extending the "what is executable here, and what is not" paragraph to cover the lock (or adding a one-line sibling note near SKILL.md's lock-posture paragraph) would close it cheaply and keep the PR's own standard applied consistently. Two inline findings (test coverage / validation)
No other issues found
|
…state A lease recording `stale_after_s=0` satisfies the staleness test the moment it is written — born abandoned, and adoptable by `--resume` out from under the run that just wrote it. §3 now documents `--stale-after` as an operator lever, so the zero value has to refuse rather than quietly produce a lease no run could keep. Refused rather than clamped: a clamp hands a caller a window it did not choose and then reports on it. `--skew-grace 0` stays legal — "tolerate no forward clock jump" is a coherent choice and inverts nothing. Also names `lease acquire --epoch <n>` as the adoption seam it is, rather than leaving an undocumented flag reading as a feature: an adopting run passes the epoch it won, and the compare-and-set that decides who won is §3's, performed by the run and not by this script. Refs #2280 (F5) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 6s —— View job Security review
Scope reviewed: all 7 files changed in this PR at HEAD The newest commit ( IMPORTANT (confidence: plausible) —
|
kyle-sexton
left a comment
kyle-sexton
left a comment
There was a problem hiding this comment.
Do not merge without orchestrator sign-off on the two spec reductions. Leaving this as an
unresolved thread rather than only in the PR body, because this repo has two recorded instances of an
explicitly-blocked PR being merged by another actor, and the durable form of a blocking finding is a
thread that outlives the reviewing session.
Nothing here is unsafe and nothing must-not-ship. What needs a human is that two clauses of a
shipped contract were reduced rather than implemented, and a reduction is a judgment call a silent
merge would ship unreviewed:
- The 60-second wall-clock heartbeat became boundary-driven refresh, and the default staleness
threshold moved 5 minutes to 30. My reasoning: a skill acts between tool calls and has no timer,
so the timed cadence named a mechanism no run could keep — and the derived 5-minute threshold is
shorter than a single delegated lane can run, which classifies a running pass as abandoned and
lets--resumeadopt a live run's artifact. The threshold now travels in the lease soclassify
reads what was written rather than assuming a constant. The alternative I rejected was spawning a
detached heartbeat process: an orphaned one pins the lease live forever, which is strictly worse
than the defect. - §5's separate "run manifest" became the partial's own lane records. §7 already required that
completion state be derivable from the partial "rather than tracked beside it and able to disagree
with it", and a manifest beside the partial is exactly what can disagree. This also closes the
second link in F12's chain — making the partial real while leaving completion state in a file
nothing writes would have moved the defect, not fixed it.
Also flagged for the reviewer's attention, though I believe it is correct: stale-lease adoption
(owner_epoch compare-and-set) and §7 assembly are deliberately left unimplemented, and §3/§7 now
say so explicitly rather than reading as machinery. If the orchestrator wants those built rather than
declared, this PR is the wrong shape and should be split.
Resolve this thread once those three calls are confirmed.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
|
Claude has reviewed this PR 2 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
|
Claude finished @kyle-sexton's task in 6m 13s —— View job Code review
Scope. This repo runs a dedicated security lane ( Posted 3 inline findings on this pass, all correctness/design/test-coverage, none security:
Everything else checked, no issues:
Note: I did not independently run |
…ate records, pin the write tree (#2446) ## Summary **`claude-config` 0.38.0 is on `main` carrying three defects in the script it shipped.** They were found by review on #2441; that PR merged at 13:19:55Z while the fixes were still being written, so they ship here as 0.38.1. Anyone on 0.38.0 has all three. All three are one family, and it is the family this batch keeps finding: **a control that does not enforce what its surface claims.** **1. The partial was named for the lease's *current* epoch, not the writer's.** `partial append` read `owner_epoch` out of the lease at append time. That defeats precisely the isolation `run-state-and-resumability.md` §3 describes: a stale holder that wakes after an adopter has incremented the epoch reads the *adopter's* value and appends into the *adopter's* file, so two writers interleave under one attempt ordinal — which §3 itself calls "the one failure the attempt machinery cannot absorb". The script was contradicting the paragraph it was written to enforce. `partial append --epoch <held>` now names the writer's own file whatever the lease says, and prints `FENCED` on stderr when the two differ so the run aborts on the signal rather than corrupting the artifact quietly. Omitting the flag still falls back to the lease's epoch, correct only for a run whose epoch nothing has moved — and that is now stated rather than assumed. **2. `{bad json}` was accepted and appended permanently.** The check was `case "$record" in '{'*)`, which passes any string starting with a brace. A malformed row in an append-only artifact cannot be taken back, and `--resume` and assembly are its only readers, so a quoting slip in the caller cost the run's whole persisted state rather than one record. Records are now verified as well-formed single-line JSON objects. `jq` decides where it is installed; where it is not, a scan that tracks string context and escape sequences still rejects `{bad json}`, a truncated row, and an unbalanced one. **`jq` is deliberately not a hard requirement** — this is the run's state-persistence path, and failing it closed on a missing optional tool would cost the artifact the check exists to protect. Both rungs are asserted; the fallback runs with a `PATH` holding only `bash`. **3. `lease acquire` created and wrote into any `--run-dir` it was handed.** A wrong or invented run directory — the target root, say — was created and had a `lease` written into it. This skill keeps Bash specifically for state writes *while promising that a bare audit writes nothing into the target*, so that promise was enforceable only by the caller getting the argument right. `acquire` is the only command that creates a directory, so it is where the write tree is pinned: it now requires `--plugin-data` and refuses any run directory outside `<plugin-data>/runs/`. Every later command operates on a directory `acquire` already validated. Version: `claude-config` **0.38.0 to 0.38.1**. The shipped `## [0.38.0]` section is untouched — 0.38.1 is additive and says outright that 0.38.0 carries all three. ## Test plan **Fail-before / pass-after, per finding.** Each was reproduced against the merged 0.38.0 script before the fix, and each ships an assertion that fails without it. Verbatim transcript. `v0380.sh` is `git show origin/main:.../run-state.sh` — the script as merged. The lease is moved to `owner_epoch=9` as an adopter would leave it, while the writer still holds 3. ``` --- 1. epoch binding, 0.38.0 (writer holds 3, lease says 9) --- /tmp/tmp.RpfHhnqzmO/data/runs/demo/r1/findings.partial.9.jsonl rc=0 --- 2. malformed record, 0.38.0 --- /tmp/tmp.RpfHhnqzmO/data/runs/demo/r1/findings.partial.9.jsonl rc=0 --- 3. write-tree containment, 0.38.0 --- /tmp/tmp.RpfHhnqzmO/target-repo/.claude/lease rc=0 lease ^ the lease was written into a non-plugin tree === now the fixed script === --- 1. fixed --- run-state.sh: FENCED — lease owner_epoch is 9 but this writer holds 3; appending to the writer's own epoch file and this run must abort /tmp/tmp.RpfHhnqzmO/data/runs/demo/r1/findings.partial.3.jsonl rc=0 --- 2. fixed --- run-state.sh: --record must be a well-formed single-line JSON object: {bad json} rc=2 --- 3. fixed --- run-state.sh: --run-dir is not under $plugin-data/runs/ — refusing to write outside the plugin's own tree: /tmp/tmp.RpfHhnqzmO/target-repo2/.claude rc=2 ``` Read line 2 against line 12: on 0.38.0 the record the writer holding epoch 3 appended landed in `findings.partial.**9**.jsonl` — the adopter's file. On the fix it lands in `.3.` and the fence is announced. Line 4 is `{bad json}` accepted and appended at `rc=0`; line 7 is a lease written into `target-repo/.claude`, a directory the script created outside the plugin's tree. **Suite — 73 checks, up from 57:** ``` $ bash plugins/claude-config/skills/audit-pass/scripts/run-state.test.sh PASS: a run dir outside <plugin-data>/runs/ is refused PASS: the refusal says it will not write outside the plugin tree PASS: the refused run dir was not created PASS: a run dir containing '..' is refused PASS: acquire without --plugin-data has nothing to check containment against, and refuses PASS: a balanced-but-malformed record is refused, not appended PASS: a truncated record is refused PASS: a brace inside a string does not fool the check PASS: and the malformed records left the artifact untouched PASS: without jq, a balanced-but-malformed record is still refused PASS: without jq, a truncated record is still refused PASS: without jq, a brace inside a string does not fool the fallback PASS: without jq, an empty object is accepted PASS: a fenced writer appends to its own epoch file, not the adopter's PASS: and the fence is reported rather than swallowed PASS: omitting --epoch still falls back to the lease's current epoch ... All 73 checks passed. ``` The three mutation-based negative tests from 0.38.0 still pass unchanged — each deletes exactly one check from a copy of the script and asserts the mutated copy reaches the outcome the real one refuses. **Repo gates, locally:** ``` $ shellcheck --rcfile .shellcheckrc -S info plugins/claude-config/skills/audit-pass/scripts/*.sh (clean) $ typos --config _typos.toml plugins/claude-config/ (clean) $ bash scripts/check-changed-skills.sh origin/main CHECK-SKILL audit-pass: PASS — 0 errors, 2 warning(s) $ bash scripts/check-changelog-parity.sh --check-preserved origin/main All 1 changed changelog(s) preserve every version heading they carried at e4501a0 (68 compared). $ 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 "plugins/claude-config/skills/audit-pass/**/*.md" "plugins/claude-config/CHANGELOG.md" Summary: 0 issues in 0 files ``` Both `.sh` files carry mode `100755` — the `exec-bit` sub-check of `hygiene` fails on `100644`, and it is invisible to every local linter. ## Review rounds on this PR, and what they changed Seven further findings landed after the first push. All are fixed here, each with an assertion, and every one is the same class again — a control not enforcing what its surface claims. Recorded because three of them were in *my own fix for that class*. - **🔴 `FENCED` was advisory only.** The script printed "this run must abort" and returned 0, so the abort depended on the caller spotting a substring on stderr. A fenced append now **exits 3**; the record still lands in the writer's own epoch file, and `SKILL.md` Phase 3 says what to do with the code. The test asserts the exit code, not just the message. - **🔴 The symlink fix broke the very first run.** `pwd -P` cannot resolve a directory that is not there, so canonicalizing `--plugin-data` before creating it made `lease acquire` fail on a plugin's first-ever run — the only run whose data root has never existed. **Every test in the file pre-makes that directory, so none could see it.** Reproduced against the prior commit (`--plugin-data cannot be resolved`, `rc=2`), fixed, and a test now exercises the fresh-install path. - **P1: the no-parser rung over-claimed.** `{"a" garbage}` balances and opens with a quoted key, and no non-parser catches it. `python3` is now the second definitive rung after `jq`, and where neither exists the structural scan **announces on stderr that it checked structurally only** rather than passing for a validator. - **P2: containment was lexical, so a symlink defeated it.** Both sides are canonicalized with `pwd -P` — `--run-dir` through its deepest existing ancestor. Tests assert the escape is refused, that no lease was written through the link, and that a symlink resolving back inside still works. The arm skips on Windows (`ln -s` refused) and **passes on CI**. - **P2: `report-location-and-schema.md` §7 still described the old append contract**, so an audit following that page after an adoption would have omitted `--epoch` and reinstated the interleaving. Both surfaces now carry the same command. - **🟡 check-then-act window** between symlink resolution and `mkdir`: accepted and now recorded in the code as a disclosed residual. Closing it needs an atomic create-and-verify no portable shell offers, and the attacker would already need write access inside the plugin's own data directory. - **🟡 a shared validator printed a staleness-specific reason at an `--epoch` rejection.** The reason is now an argument. Suite: **81 checks**, all passing. All six bot threads are replied to and resolved; the one open thread is my own do-not-merge marker. ## Related Refs #2280 No linked issue closes here. #2280 was closed by #2441's merge and its four live rows (F3, F5, F12, F13) are genuinely retired by the mechanism that shipped; this PR repairs three defects *in that mechanism* rather than reopening a row, so it deliberately carries no closing keyword. Follow-up to #2441 (`claude-config` 0.38.0). Origin: handoff-inbox item `20260811-020411-claude-config-audit-pass-report-path-inside-scan-set`. **Note for the reviewer, recorded rather than left implicit.** #2441 was merged by another actor while it carried an explicit "do not merge" and an unresolved review comment naming two deliberate spec reductions for sign-off. Nothing shipped that is unsafe, but three real defects reached `main` that would not have. Those two reductions — the 60-second wall-clock heartbeat becoming boundary-driven refresh with the thresholds recorded in the lease, and §5's separate run manifest becoming the partial's own lane records — are now live on `main` unreviewed and still want a human read. They are described in #2441's body and in the `## [0.38.0]` CHANGELOG entry. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ate records, pin the write tree (#2446) ## Summary **`claude-config` 0.38.0 is on `main` carrying three defects in the script it shipped.** They were found by review on #2441; that PR merged at 13:19:55Z while the fixes were still being written, so they ship here as 0.38.1. Anyone on 0.38.0 has all three. All three are one family, and it is the family this batch keeps finding: **a control that does not enforce what its surface claims.** **1. The partial was named for the lease's *current* epoch, not the writer's.** `partial append` read `owner_epoch` out of the lease at append time. That defeats precisely the isolation `run-state-and-resumability.md` §3 describes: a stale holder that wakes after an adopter has incremented the epoch reads the *adopter's* value and appends into the *adopter's* file, so two writers interleave under one attempt ordinal — which §3 itself calls "the one failure the attempt machinery cannot absorb". The script was contradicting the paragraph it was written to enforce. `partial append --epoch <held>` now names the writer's own file whatever the lease says, and prints `FENCED` on stderr when the two differ so the run aborts on the signal rather than corrupting the artifact quietly. Omitting the flag still falls back to the lease's epoch, correct only for a run whose epoch nothing has moved — and that is now stated rather than assumed. **2. `{bad json}` was accepted and appended permanently.** The check was `case "$record" in '{'*)`, which passes any string starting with a brace. A malformed row in an append-only artifact cannot be taken back, and `--resume` and assembly are its only readers, so a quoting slip in the caller cost the run's whole persisted state rather than one record. Records are now verified as well-formed single-line JSON objects. `jq` decides where it is installed; where it is not, a scan that tracks string context and escape sequences still rejects `{bad json}`, a truncated row, and an unbalanced one. **`jq` is deliberately not a hard requirement** — this is the run's state-persistence path, and failing it closed on a missing optional tool would cost the artifact the check exists to protect. Both rungs are asserted; the fallback runs with a `PATH` holding only `bash`. **3. `lease acquire` created and wrote into any `--run-dir` it was handed.** A wrong or invented run directory — the target root, say — was created and had a `lease` written into it. This skill keeps Bash specifically for state writes *while promising that a bare audit writes nothing into the target*, so that promise was enforceable only by the caller getting the argument right. `acquire` is the only command that creates a directory, so it is where the write tree is pinned: it now requires `--plugin-data` and refuses any run directory outside `<plugin-data>/runs/`. Every later command operates on a directory `acquire` already validated. Version: `claude-config` **0.38.0 to 0.38.1**. The shipped `## [0.38.0]` section is untouched — 0.38.1 is additive and says outright that 0.38.0 carries all three. ## Test plan **Fail-before / pass-after, per finding.** Each was reproduced against the merged 0.38.0 script before the fix, and each ships an assertion that fails without it. Verbatim transcript. `v0380.sh` is `git show origin/main:.../run-state.sh` — the script as merged. The lease is moved to `owner_epoch=9` as an adopter would leave it, while the writer still holds 3. ``` --- 1. epoch binding, 0.38.0 (writer holds 3, lease says 9) --- /tmp/tmp.RpfHhnqzmO/data/runs/demo/r1/findings.partial.9.jsonl rc=0 --- 2. malformed record, 0.38.0 --- /tmp/tmp.RpfHhnqzmO/data/runs/demo/r1/findings.partial.9.jsonl rc=0 --- 3. write-tree containment, 0.38.0 --- /tmp/tmp.RpfHhnqzmO/target-repo/.claude/lease rc=0 lease ^ the lease was written into a non-plugin tree === now the fixed script === --- 1. fixed --- run-state.sh: FENCED — lease owner_epoch is 9 but this writer holds 3; appending to the writer's own epoch file and this run must abort /tmp/tmp.RpfHhnqzmO/data/runs/demo/r1/findings.partial.3.jsonl rc=0 --- 2. fixed --- run-state.sh: --record must be a well-formed single-line JSON object: {bad json} rc=2 --- 3. fixed --- run-state.sh: --run-dir is not under $plugin-data/runs/ — refusing to write outside the plugin's own tree: /tmp/tmp.RpfHhnqzmO/target-repo2/.claude rc=2 ``` Read line 2 against line 12: on 0.38.0 the record the writer holding epoch 3 appended landed in `findings.partial.**9**.jsonl` — the adopter's file. On the fix it lands in `.3.` and the fence is announced. Line 4 is `{bad json}` accepted and appended at `rc=0`; line 7 is a lease written into `target-repo/.claude`, a directory the script created outside the plugin's tree. **Suite — 73 checks, up from 57:** ``` $ bash plugins/claude-config/skills/audit-pass/scripts/run-state.test.sh PASS: a run dir outside <plugin-data>/runs/ is refused PASS: the refusal says it will not write outside the plugin tree PASS: the refused run dir was not created PASS: a run dir containing '..' is refused PASS: acquire without --plugin-data has nothing to check containment against, and refuses PASS: a balanced-but-malformed record is refused, not appended PASS: a truncated record is refused PASS: a brace inside a string does not fool the check PASS: and the malformed records left the artifact untouched PASS: without jq, a balanced-but-malformed record is still refused PASS: without jq, a truncated record is still refused PASS: without jq, a brace inside a string does not fool the fallback PASS: without jq, an empty object is accepted PASS: a fenced writer appends to its own epoch file, not the adopter's PASS: and the fence is reported rather than swallowed PASS: omitting --epoch still falls back to the lease's current epoch ... All 73 checks passed. ``` The three mutation-based negative tests from 0.38.0 still pass unchanged — each deletes exactly one check from a copy of the script and asserts the mutated copy reaches the outcome the real one refuses. **Repo gates, locally:** ``` $ shellcheck --rcfile .shellcheckrc -S info plugins/claude-config/skills/audit-pass/scripts/*.sh (clean) $ typos --config _typos.toml plugins/claude-config/ (clean) $ bash scripts/check-changed-skills.sh origin/main CHECK-SKILL audit-pass: PASS — 0 errors, 2 warning(s) $ bash scripts/check-changelog-parity.sh --check-preserved origin/main All 1 changed changelog(s) preserve every version heading they carried at e4501a0 (68 compared). $ 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 "plugins/claude-config/skills/audit-pass/**/*.md" "plugins/claude-config/CHANGELOG.md" Summary: 0 issues in 0 files ``` Both `.sh` files carry mode `100755` — the `exec-bit` sub-check of `hygiene` fails on `100644`, and it is invisible to every local linter. ## Review rounds on this PR, and what they changed Seven further findings landed after the first push. All are fixed here, each with an assertion, and every one is the same class again — a control not enforcing what its surface claims. Recorded because three of them were in *my own fix for that class*. - **🔴 `FENCED` was advisory only.** The script printed "this run must abort" and returned 0, so the abort depended on the caller spotting a substring on stderr. A fenced append now **exits 3**; the record still lands in the writer's own epoch file, and `SKILL.md` Phase 3 says what to do with the code. The test asserts the exit code, not just the message. - **🔴 The symlink fix broke the very first run.** `pwd -P` cannot resolve a directory that is not there, so canonicalizing `--plugin-data` before creating it made `lease acquire` fail on a plugin's first-ever run — the only run whose data root has never existed. **Every test in the file pre-makes that directory, so none could see it.** Reproduced against the prior commit (`--plugin-data cannot be resolved`, `rc=2`), fixed, and a test now exercises the fresh-install path. - **P1: the no-parser rung over-claimed.** `{"a" garbage}` balances and opens with a quoted key, and no non-parser catches it. `python3` is now the second definitive rung after `jq`, and where neither exists the structural scan **announces on stderr that it checked structurally only** rather than passing for a validator. - **P2: containment was lexical, so a symlink defeated it.** Both sides are canonicalized with `pwd -P` — `--run-dir` through its deepest existing ancestor. Tests assert the escape is refused, that no lease was written through the link, and that a symlink resolving back inside still works. The arm skips on Windows (`ln -s` refused) and **passes on CI**. - **P2: `report-location-and-schema.md` §7 still described the old append contract**, so an audit following that page after an adoption would have omitted `--epoch` and reinstated the interleaving. Both surfaces now carry the same command. - **🟡 check-then-act window** between symlink resolution and `mkdir`: accepted and now recorded in the code as a disclosed residual. Closing it needs an atomic create-and-verify no portable shell offers, and the attacker would already need write access inside the plugin's own data directory. - **🟡 a shared validator printed a staleness-specific reason at an `--epoch` rejection.** The reason is now an argument. Suite: **81 checks**, all passing. All six bot threads are replied to and resolved; the one open thread is my own do-not-merge marker. ## Related Refs #2280 No linked issue closes here. #2280 was closed by #2441's merge and its four live rows (F3, F5, F12, F13) are genuinely retired by the mechanism that shipped; this PR repairs three defects *in that mechanism* rather than reopening a row, so it deliberately carries no closing keyword. Follow-up to #2441 (`claude-config` 0.38.0). Origin: handoff-inbox item `20260811-020411-claude-config-audit-pass-report-path-inside-scan-set`. **Note for the reviewer, recorded rather than left implicit.** #2441 was merged by another actor while it carried an explicit "do not merge" and an unresolved review comment naming two deliberate spec reductions for sign-off. Nothing shipped that is unsafe, but three real defects reached `main` that would not have. Those two reductions — the 60-second wall-clock heartbeat becoming boundary-driven refresh with the thresholds recorded in the lease, and §5's separate run manifest becoming the partial's own lane records — are now live on `main` unreviewed and still want a human read. They are described in #2441's body and in the `## [0.38.0]` CHANGELOG entry. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ate records, pin the write tree (#2446) ## Summary **`claude-config` 0.38.0 is on `main` carrying three defects in the script it shipped.** They were found by review on #2441; that PR merged at 13:19:55Z while the fixes were still being written, so they ship here as 0.38.1. Anyone on 0.38.0 has all three. All three are one family, and it is the family this batch keeps finding: **a control that does not enforce what its surface claims.** **1. The partial was named for the lease's *current* epoch, not the writer's.** `partial append` read `owner_epoch` out of the lease at append time. That defeats precisely the isolation `run-state-and-resumability.md` §3 describes: a stale holder that wakes after an adopter has incremented the epoch reads the *adopter's* value and appends into the *adopter's* file, so two writers interleave under one attempt ordinal — which §3 itself calls "the one failure the attempt machinery cannot absorb". The script was contradicting the paragraph it was written to enforce. `partial append --epoch <held>` now names the writer's own file whatever the lease says, and prints `FENCED` on stderr when the two differ so the run aborts on the signal rather than corrupting the artifact quietly. Omitting the flag still falls back to the lease's epoch, correct only for a run whose epoch nothing has moved — and that is now stated rather than assumed. **2. `{bad json}` was accepted and appended permanently.** The check was `case "$record" in '{'*)`, which passes any string starting with a brace. A malformed row in an append-only artifact cannot be taken back, and `--resume` and assembly are its only readers, so a quoting slip in the caller cost the run's whole persisted state rather than one record. Records are now verified as well-formed single-line JSON objects. `jq` decides where it is installed; where it is not, a scan that tracks string context and escape sequences still rejects `{bad json}`, a truncated row, and an unbalanced one. **`jq` is deliberately not a hard requirement** — this is the run's state-persistence path, and failing it closed on a missing optional tool would cost the artifact the check exists to protect. Both rungs are asserted; the fallback runs with a `PATH` holding only `bash`. **3. `lease acquire` created and wrote into any `--run-dir` it was handed.** A wrong or invented run directory — the target root, say — was created and had a `lease` written into it. This skill keeps Bash specifically for state writes *while promising that a bare audit writes nothing into the target*, so that promise was enforceable only by the caller getting the argument right. `acquire` is the only command that creates a directory, so it is where the write tree is pinned: it now requires `--plugin-data` and refuses any run directory outside `<plugin-data>/runs/`. Every later command operates on a directory `acquire` already validated. Version: `claude-config` **0.38.0 to 0.38.1**. The shipped `## [0.38.0]` section is untouched — 0.38.1 is additive and says outright that 0.38.0 carries all three. ## Test plan **Fail-before / pass-after, per finding.** Each was reproduced against the merged 0.38.0 script before the fix, and each ships an assertion that fails without it. Verbatim transcript. `v0380.sh` is `git show origin/main:.../run-state.sh` — the script as merged. The lease is moved to `owner_epoch=9` as an adopter would leave it, while the writer still holds 3. ``` --- 1. epoch binding, 0.38.0 (writer holds 3, lease says 9) --- /tmp/tmp.RpfHhnqzmO/data/runs/demo/r1/findings.partial.9.jsonl rc=0 --- 2. malformed record, 0.38.0 --- /tmp/tmp.RpfHhnqzmO/data/runs/demo/r1/findings.partial.9.jsonl rc=0 --- 3. write-tree containment, 0.38.0 --- /tmp/tmp.RpfHhnqzmO/target-repo/.claude/lease rc=0 lease ^ the lease was written into a non-plugin tree === now the fixed script === --- 1. fixed --- run-state.sh: FENCED — lease owner_epoch is 9 but this writer holds 3; appending to the writer's own epoch file and this run must abort /tmp/tmp.RpfHhnqzmO/data/runs/demo/r1/findings.partial.3.jsonl rc=0 --- 2. fixed --- run-state.sh: --record must be a well-formed single-line JSON object: {bad json} rc=2 --- 3. fixed --- run-state.sh: --run-dir is not under $plugin-data/runs/ — refusing to write outside the plugin's own tree: /tmp/tmp.RpfHhnqzmO/target-repo2/.claude rc=2 ``` Read line 2 against line 12: on 0.38.0 the record the writer holding epoch 3 appended landed in `findings.partial.**9**.jsonl` — the adopter's file. On the fix it lands in `.3.` and the fence is announced. Line 4 is `{bad json}` accepted and appended at `rc=0`; line 7 is a lease written into `target-repo/.claude`, a directory the script created outside the plugin's tree. **Suite — 73 checks, up from 57:** ``` $ bash plugins/claude-config/skills/audit-pass/scripts/run-state.test.sh PASS: a run dir outside <plugin-data>/runs/ is refused PASS: the refusal says it will not write outside the plugin tree PASS: the refused run dir was not created PASS: a run dir containing '..' is refused PASS: acquire without --plugin-data has nothing to check containment against, and refuses PASS: a balanced-but-malformed record is refused, not appended PASS: a truncated record is refused PASS: a brace inside a string does not fool the check PASS: and the malformed records left the artifact untouched PASS: without jq, a balanced-but-malformed record is still refused PASS: without jq, a truncated record is still refused PASS: without jq, a brace inside a string does not fool the fallback PASS: without jq, an empty object is accepted PASS: a fenced writer appends to its own epoch file, not the adopter's PASS: and the fence is reported rather than swallowed PASS: omitting --epoch still falls back to the lease's current epoch ... All 73 checks passed. ``` The three mutation-based negative tests from 0.38.0 still pass unchanged — each deletes exactly one check from a copy of the script and asserts the mutated copy reaches the outcome the real one refuses. **Repo gates, locally:** ``` $ shellcheck --rcfile .shellcheckrc -S info plugins/claude-config/skills/audit-pass/scripts/*.sh (clean) $ typos --config _typos.toml plugins/claude-config/ (clean) $ bash scripts/check-changed-skills.sh origin/main CHECK-SKILL audit-pass: PASS — 0 errors, 2 warning(s) $ bash scripts/check-changelog-parity.sh --check-preserved origin/main All 1 changed changelog(s) preserve every version heading they carried at e4501a0 (68 compared). $ 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 "plugins/claude-config/skills/audit-pass/**/*.md" "plugins/claude-config/CHANGELOG.md" Summary: 0 issues in 0 files ``` Both `.sh` files carry mode `100755` — the `exec-bit` sub-check of `hygiene` fails on `100644`, and it is invisible to every local linter. ## Review rounds on this PR, and what they changed Seven further findings landed after the first push. All are fixed here, each with an assertion, and every one is the same class again — a control not enforcing what its surface claims. Recorded because three of them were in *my own fix for that class*. - **🔴 `FENCED` was advisory only.** The script printed "this run must abort" and returned 0, so the abort depended on the caller spotting a substring on stderr. A fenced append now **exits 3**; the record still lands in the writer's own epoch file, and `SKILL.md` Phase 3 says what to do with the code. The test asserts the exit code, not just the message. - **🔴 The symlink fix broke the very first run.** `pwd -P` cannot resolve a directory that is not there, so canonicalizing `--plugin-data` before creating it made `lease acquire` fail on a plugin's first-ever run — the only run whose data root has never existed. **Every test in the file pre-makes that directory, so none could see it.** Reproduced against the prior commit (`--plugin-data cannot be resolved`, `rc=2`), fixed, and a test now exercises the fresh-install path. - **P1: the no-parser rung over-claimed.** `{"a" garbage}` balances and opens with a quoted key, and no non-parser catches it. `python3` is now the second definitive rung after `jq`, and where neither exists the structural scan **announces on stderr that it checked structurally only** rather than passing for a validator. - **P2: containment was lexical, so a symlink defeated it.** Both sides are canonicalized with `pwd -P` — `--run-dir` through its deepest existing ancestor. Tests assert the escape is refused, that no lease was written through the link, and that a symlink resolving back inside still works. The arm skips on Windows (`ln -s` refused) and **passes on CI**. - **P2: `report-location-and-schema.md` §7 still described the old append contract**, so an audit following that page after an adoption would have omitted `--epoch` and reinstated the interleaving. Both surfaces now carry the same command. - **🟡 check-then-act window** between symlink resolution and `mkdir`: accepted and now recorded in the code as a disclosed residual. Closing it needs an atomic create-and-verify no portable shell offers, and the attacker would already need write access inside the plugin's own data directory. - **🟡 a shared validator printed a staleness-specific reason at an `--epoch` rejection.** The reason is now an argument. Suite: **81 checks**, all passing. All six bot threads are replied to and resolved; the one open thread is my own do-not-merge marker. ## Related Refs #2280 No linked issue closes here. #2280 was closed by #2441's merge and its four live rows (F3, F5, F12, F13) are genuinely retired by the mechanism that shipped; this PR repairs three defects *in that mechanism* rather than reopening a row, so it deliberately carries no closing keyword. Follow-up to #2441 (`claude-config` 0.38.0). Origin: handoff-inbox item `20260811-020411-claude-config-audit-pass-report-path-inside-scan-set`. **Note for the reviewer, recorded rather than left implicit.** #2441 was merged by another actor while it carried an explicit "do not merge" and an unresolved review comment naming two deliberate spec reductions for sign-off. Nothing shipped that is unsafe, but three real defects reached `main` that would not have. Those two reductions — the 60-second wall-clock heartbeat becoming boundary-driven refresh with the thresholds recorded in the lease, and §5's separate run manifest becoming the partial's own lane records — are now live on `main` unreviewed and still want a human read. They are described in #2441's body and in the `## [0.38.0]` CHANGELOG entry. --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
audit-passspecified a runtime and shipped no executable.SKILL.mdplus tenreference/*.mdspell out a lease — path, refresh discipline, a two-sided liveness window, a
releasedtombstone,owner_epochfencing — and an epoch-scoped append-only partial that--resumereads instead of thereport. The skill shipped no
scripts/directory at all, the only audit skill inclaude-configwithout one.
The gap has a sharper form than "no scripts", and it is fully verifiable from the tree:
lib/state-key.sh's own header records the keying scheme as "audit-pass's, reused rather thanreinvented" and points at
skills/audit-pass/reference/run-state-and-resumability.md§3 — andgrep -rl state-key.sh plugins/returnsaudit-instructions,audit-prompting-posturesandclaude-memory:audit. Every skill called it except the one that specified it.What this PR adds.
skills/audit-pass/scripts/run-state.sh, with subcommandspaths,lease acquire|heartbeat|release|classify, andpartial append, plusrun-state.test.sh.pathsderives
<plugin-data>/runs/<state-key>/<run-id>throughlib/state-key.sh, so the skill now runson its own scheme. No
jq, no GNU-only constructs, no network, no background process.What it deliberately does not add, and now says so in-contract. Stale-lease adoption (the
owner_epochcompare-and-set) and §7 assembly are not implemented. §3 and §7 now state whichclauses the script enforces and which remain the run's own discipline. The standard this PR is held
to is that a contract must not read as enforced while nothing enforces it — that is satisfied by
implementing or by declaring, and each clause here does one of the two.
Two clauses were reduced rather than implemented, because a skill cannot keep them.
calls and has no timer, so that cadence named a mechanism no run could provide — the same defect
as specifying a lease and shipping no writer. Refresh is now boundary-driven (acquire, each
lane's persistence point, release). Each lease records the
stale_after_sandskew_grace_sitswriter committed to, and
classifyreads them from the artifact — which is what the section's own"two implementations must reach it identically" concern actually needed. The default threshold
moves 5 minutes to 30: with boundary-driven refresh a single delegated lane can outlast five
minutes, and a threshold shorter than a lane classifies a running pass as abandoned, the unsafe
direction because it lets
--resumeadopt a live run's artifact.partial. §7 already required that
--resumeread the partial "so completion state is derivablefrom the artifact rather than tracked beside it and able to disagree with it" — and a manifest
beside the partial is precisely what can disagree with it. §5 now states the manifest as the
partial's own lane records. This closes the second link in F12's chain: making the partial real
while leaving completion state in a file nothing writes would have moved the defect, not fixed it.
Evidence discipline. F5 is
SELF_REPORTEDand its "the observed run wrote none" leg is notreproducible from this repository. Nothing in the code, the CHANGELOG, or this PR asserts it. The
basis for every change here is the specification-versus-implementation gap, which is verifiable
from the tree and was verified.
Trust surface. The script writes only under the plugin data directory it is given — it
discovers no directory — and never inside a target repository, so the report-only contract
(
disallowed-tools: Edit, NotebookEdit) is untouched. Both path segments it contributes arevalidated:
lib/state-key.shalready refuses a remote URL that would become traversing directorycomponents, and a
--run-idoutside[A-Za-z0-9][A-Za-z0-9_.-]*, or containing.., is refusedhere. Both refusals carry negative tests.
Version:
claude-config0.37.1 to 0.38.0 (new executable surface), with a CHANGELOG entry.Test plan
Fail-before. The script is new, so
main's state is the script absent. Withrun-state.shmovedaside, every one of the 52 checks fails:
Pass-after.
The three negative tests are the point. Each mutates a copy of the script to delete exactly one
check and asserts the mutated copy reaches the outcome the real one refuses — a passing test that
would still pass with the check deleted proves nothing:
heartbeat_attwo hours in the future readsliveinstead ofstale, so a dead run pins itself live and every--resumerefuses an abandonedrun indefinitely (assertion 3.9);
..arm of--run-idvalidation, anda..bis accepted and a traversing run dir isassembled;
/etc/passwdis accepted as a run id and escapes the pluginnamespace.
Each construction is verified before it is trusted: if the
sedtarget no longer matches, the testfails with "UNVERIFIED by this run" rather than passing vacuously.Repo gates, run locally:
Every other
plugins/claude-config/**/*.test.shstill passes (lib/state-key.test.sh23,instruction-scan.test.sh70,permission-rule-check.test.sh136, and the rest).Both
SKILL.mdwarnings are pre-existing: the 200-line soft target, and a stalefresh-eyes-exemptdirective in
## Self-check, a section this PR does not touch.Related
Closes #2280
Origin: handoff-inbox item
20260811-020411-claude-config-audit-pass-report-path-inside-scan-set(ledger
.work/handoff-inbox-batch-4/ledgers/I8-audit-pass-report-path.md§ F3, F5, F12, F13).Row state, all six:
scripts/run-state.sh+run-state.test.sh, three negative testslease acquire|heartbeat|release|classify; the unkeepable 60s cadence reduced to boundary-driven refresh--resumereads a partial nothing writespartial append, Phase 4'sopenterminator written when the handoff is recorded, and §5's separate manifest folded into the partial's lane recordspartial append; the disclaimer itself is unchanged, since persistence bounds nothing{id}mangling and the Bash-env fact unstatedgit log -Sand by readingreference/report-location-and-schema.md:25-37${CLAUDE_PROJECT_DIR}"when set" conditionalSKILL.md:48-58Adjacent and deliberately not touched: #1430 (cross-epoch carry-forward), #1940 (
rerun-contract.mddesign-doc drift), #1568 (
${CLAUDE_*}substitution scope). F4's refuse-vs-specify fork is settledon
mainalready and is not reopened here.