fix(claude-config): bind partial appends to the writer's epoch, validate records, pin the write tree

[kyle-sexton]

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.

[cursor]

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]

Claude finished @kyle-sexton's task in 4m 54s —— View job

---

Claude's Code Review

Tasks

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Understand the request (code review, correctness + maintainability lane)
• Review run-state.sh changes (epoch binding, JSON validation, write-tree containment)
• Review run-state.test.sh changes
• Review docs (SKILL.md, run-state-and-resumability.md, CHANGELOG.md, plugin.json)
• Post inline comments for findings
• Post final summary

Summary

Reviewed the three fixes against origin/main (fix/2280-followup-run-state-hardening, HEAD 8fb7583). Root CLAUDE.md is empty; followed REVIEW.md's scope split — this repo has .github/workflows/claude-security-review.yml, so security findings (none found anyway) are left to that lane. This is a script-only change; no app to run, so no runtime testing was performed beyond static reading of the diff.

Two of the three fixes read as correct on inspection:

• Fix 3 (write-tree containment): cmd_lease_acquire is confirmed the only call site with mkdir in the whole script, so pinning containment there is structurally sound. The --run-dir under $plugin_data/runs/* check plus the *..* rejection is a reasonable belt-and-suspenders pair for this internal tool.
• Fix 2 (JSON validation): the no-jq fallback correctly tracks string/escape context and bracket depth, and rejects the three documented cases ({bad json}, truncated, unbalanced). It's a deliberately scoped heuristic rather than a full parser (e.g. it would still accept a trailing-comma object like {"a":1,}), which the PR is explicit about — not a gap I'd block on.

Fix 1 (epoch binding) has one real gap — flagged inline on run-state.sh (lines 651–660): the FENCED condition is reported on stderr with text stating "this run must abort," but cmd_partial_append still exits 0 regardless, and the reference doc (run-state-and-resumability.md#L239) asserts "the run aborts on that signal" as if it were enforced. It isn't — enforcement depends entirely on whatever calls this script noticing a specific stderr substring, and SKILL.md's actual Phase 3 operational instructions for partial append (SKILL.md#L320) don't mention checking for it at all. This is the same defect family the PR itself names ("a control that does not enforce what its surface claims") — worth closing here too, either by making the script exit non-zero on a fence or by adding an explicit instruction for the calling agent to check for it. To be clear: this does not reopen the original isolation bug — the writer's record still correctly lands in its own epoch file — the gap is only that a fenced run isn't actually forced to stop.

Everything else — version bump, CHANGELOG entry, doc updates, and test additions — is consistent and proportionate to the changes.

[claude]

Claude finished @kyle-sexton's task in 4m 4s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Read changed scripts (run-state.sh, run-state.test.sh) in full
• Review epoch-binding / fencing logic for auth/logic gaps
• Review JSON record validation (jq path + fallback scanner) for injection/bypass
• Review write-tree containment check (--run-dir / --plugin-data validation) for path traversal / symlink issues
• Review SKILL.md / reference doc changes for security-relevant claims
• Compile findings with severity + confidence

Scope: only the 6 files changed in this PR (no GitHub Actions workflows are touched, so no zizmor-lane items apply here).

---

Findings

1. IMPORTANT / Confidence: High — the no-jq JSON fallback validator accepts several classes of malformed JSON, undermining the exact guarantee this PR adds it to provide

record_is_json_object, fallback branch at L218–255.

When jq is not on PATH, the fallback only checks: first/last char are {/}, brace/bracket depth balances, string context is tracked correctly, and the first non-whitespace character after the opening { is either a " or the closing }. It never validates the grammar between tokens — no check for : after a key, a value after :, or the absence of a trailing comma. Traced by hand against the actual algorithm (character-by-character, in plugins/claude-config/skills/audit-pass/scripts/run-state.sh:218-255):

• {"a":1,} (trailing comma) — passes: braces balance, rest after { is "a":1,} which starts with " → accepted.
• {"a" 1} (missing colon) — passes: same reasoning, rest starts with " → accepted.
• {"a":} (missing value) — passes: rest starts with " → accepted.

None of these are well-formed JSON (jq -e would reject all three), yet the fallback rung — the one exercised in the PR's own "without jq" test cases — accepts them and they get appended permanently to findings.partial.<epoch>.jsonl, exactly the failure mode Finding #2 in this PR's description says it's closing ("a malformed row in an append-only artifact is permanent... a quoting slip... costs the run's persisted state"). The added test coverage in run-state.test.sh (without jq, a balanced-but-malformed record is still refused, etc.) only exercises {bad json}, a truncated record, and a brace-in-a-string — it doesn't cover trailing-comma / missing-colon / missing-value cases, so this gap isn't caught by the new suite.

Impact: on any host without jq installed (the PR explicitly treats this as a supported, non-degraded path — "jq is deliberately not a hard requirement"), the append-only artifact that --resume and assembly are the only readers of can still be corrupted with syntactically invalid JSON, silently, at rc=0. This is a real gap in a data-integrity control, in the jq-absent path this very PR added to harden.

Suggested fix: either (a) tighten the fallback to also validate that each "key" is followed by : and a value token, and that no , immediately precedes }/], or (b) make jq a hard requirement for partial append (the PR's stated rationale against this is availability, but a partially-validating fallback that still permits invalid JSON may be worse than failing closed, since the top-level goal is "never append something resume/assembly can't parse").

Fix this →

---

Other areas reviewed, no issues found

• Epoch fencing (cmd_partial_append) — --epoch is validated as a positive integer before use in the filename (findings.partial.$epoch.jsonl), so there's no path-injection surface via that argument. The FENCED message doesn't itself abort the process (exit stays 0), but that's a documented, intentional design choice: the actual safety property (a fenced writer's record lands in its own epoch file rather than the adopter's) holds regardless of whether the caller heeds the stderr signal, so nothing is silently corrupted.
• Write-tree containment (cmd_lease_acquire) — the "$plugin_data"/runs/* case-pattern prefix check correctly treats the quoted $plugin_data expansion as a literal string (bash quoting inside a case pattern suppresses glob-metacharacter interpretation), so a --plugin-data value containing *, ?, or [ can't be used to widen the match. The separate *..* literal check on --run-dir closes the traversal case, and since run_dir must literally begin with the plugin_data string, any .. embedded via --plugin-data would necessarily also appear in --run-dir and get caught by that same check. (A pre-existing, out-of-scope caveat: this is a textual/prefix check, not a resolved-realpath check, so a symlink placed under <plugin-data>/runs/ by something that already has write access to that tree could redirect a write elsewhere — but that requires pre-existing write access to the plugin's own state directory, which this script already trusts, so it's not a new vulnerability introduced by this PR.)
• printf/format-string usage — all printf calls with attacker-influenced values use fixed %s-based format strings with values passed positionally, never as the format string itself, so there's no format-string injection.
• jq invocation — the record is piped to jq via stdin (printf '%s' "$s" | jq -e ...), never interpolated into a jq program string or passed as a shell-evaluated argument, so there's no jq-filter or command injection surface there.
• Doc/changelog/plugin.json changes — no security-relevant content; version bump and prose only.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8fb7583ffd

ℹ️ 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".

[github-actions]

Claude has reviewed this PR 3 times. The lane skips further automatic reviews after 5; deleting this comment resets the count.

[claude]

Claude finished @kyle-sexton's task in 5m 37s —— View job

---

Claude's Code Review

Tasks

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Understand the request (code-review lane: correctness + maintainability, security left to the security lane per REVIEW.md)
• Review run-state.sh changes (epoch binding, JSON validation, write-tree containment, and this session's follow-up commit bb8a6cb addressing the prior review round)
• Review run-state.test.sh changes
• Review docs (SKILL.md, reference docs, CHANGELOG.md, plugin.json)
• Post inline comments for findings
• Post final summary

Summary

This PR was already reviewed once at commit 8fb7583, and the most recent commit (bb8a6cb, "enforce the fence, disclose the no-parser rung, resolve symlinks") is a direct, thorough response to that round: FENCED now exits 3 instead of only printing to stderr (and SKILL.md Phase 3 now tells the calling agent to stop on it), the no-jq fallback adds python3 as a second definitive parser rung and explicitly announces its own residual limitation when neither is available, and lease acquire now canonicalizes both --plugin-data and --run-dir's existing ancestor with pwd -P before the containment check, closing the symlink-escape gap Codex flagged. reference/report-location-and-schema.md §7 was also brought back in sync with the actual --epoch-carrying append contract. I traced all three fixes against the code and they read as correct closes of the specific gaps raised last round.

One new gap surfaced in the fix that closes the symlink issue — flagged inline on run-state.sh (lines 476-486): the new physical_path() call on --plugin-data requires that directory to already exist (cd into a non-existent dir fails), so lease acquire's very first invocation against a fresh ${CLAUDE_PLUGIN_DATA} now dies with "--plugin-data cannot be resolved" instead of bootstrapping the tree the way mkdir -p used to. This directly contradicts the script's own comment that acquire "is the only command that creates a directory," and it's invisible to the new test suite because run-state.test.sh pre-creates the shared plugin-data fixture (mkdir -p "$DATA") before any case runs. I traced this statically — I did not have an approved shell in this session to execute the script and confirm at runtime, so this is a high-confidence code trace, not an executed repro; noting that per the review lane's verification rule.

A second, minor finding (nit, inline on line 725): the new partial append --epoch validation reuses require_int_at_least_one, whose rejection message is hardcoded to a lease-staleness justification that doesn't apply in this context.

Everything else — the version bump, the additive ## [0.38.1] CHANGELOG entry (0.38.0 section left untouched, consistent with the stated intent), the doc updates keeping SKILL.md / both reference docs / the script in sync, and the expanded test coverage for the three original fixes — reads as consistent and proportionate.

I did not execute run-state.test.sh or the script directly in this session (no approved shell for running scripts was available), so the PR's own "73 checks passed" transcript and repo-gate output are author-claimed and unverified by me; the findings above are based on static reading of the diff.

[claude]

Claude finished @kyle-sexton's task in 2m 56s —— View job

---

Security review complete

• Gather context (PR diff, changed files)
• Read changed scripts (run-state.sh, run-state.test.sh) in full
• Review epoch-binding / fencing logic for trust-boundary gaps
• Review JSON record validation (jq path + fallback scanner) for injection/bypass
• Review write-tree containment check (--run-dir / --plugin-data) for path traversal / symlink issues
• Review doc changes for security-relevant claims
• Compile findings with severity + confidence

Scope: the 7 files changed in this PR (no workflow files touched, so zizmor's supply-chain / trigger / permissions lane doesn't apply here).

This is the third commit on the branch, and it specifically responds to the two automated review passes already on this PR (Codex's P1 finding and Claude's own prior "FENCED is advisory only" finding). I re-verified all three against the current head (bb8a6cb) rather than assuming the fix landed correctly:

1. FENCED is now actually enforced, not just announced — confirmed fixed. cmd_partial_append now returns EXIT_FENCED (3) when the writer's held epoch differs from the lease's current epoch, instead of returning 0 after only printing to stderr. SKILL.md Phase 3 (SKILL.md#L327-L330) and run-state-and-resumability.md were both updated to instruct the caller to stop on exit 3, and run-state.test.sh now asserts the exit code itself (assert_exit "a fenced append exits 3..." 3 "$rc"), not just the stderr substring. The reference doc's earlier claim ("the run aborts on that signal") is now backed by an actual mechanism.

2. No-jq JSON validation gap — mitigated with an honest disclosure, not silently left open. The fallback scanner (structurally) still can't catch every malformed-but-balanced object (e.g. {"a" garbage}, missing colon) — that specific residual gap Codex/Claude flagged still technically exists at the third rung. But this commit adds a python3 rung (run-state.sh#L257-L262) as a second definitive parser between jq and the structural scan, so the weak rung is now reached only on a host with neither jq nor python3 — a much narrower window than before (this plugin already depends on python3 elsewhere per the code comments). Where the weak rung is reached, it now announces its own limitation on stderr (no jq or python3 on PATH — the record was checked structurally only; a string that balances but is not JSON ... can pass) rather than claiming full validation. This turns it from "a control that claims more than it verifies" (the exact defect family this PR targets) into a disclosed, narrow-scope residual. Test coverage was added for the python3 rung specifically catching {"a" garbage}. I don't consider the remaining structural-rung gap a new finding worth blocking on — it's now honestly scoped rather than silently wrong, and the practical exposure (host with no jq and no python3) is narrow.

3. Write-tree containment now survives symlinks — confirmed fixed. cmd_lease_acquire previously only did a lexical prefix check on --run-dir against --plugin-data, which the earlier review round noted (as an out-of-scope caveat) could be defeated by a symlink under <plugin-data>/runs/. This commit adds physical_path/physical_existing_prefix (run-state.sh#L164-L185) to canonicalize both --plugin-data and the deepest existing ancestor of --run-dir via pwd -P before comparing, closing that gap. New tests exercise both the escape case (runs/link -> /tmp/outside is refused, and confirm no lease is written through it) and the non-escape case (a symlink that resolves back inside the tree is still accepted).

New finding (SUGGESTION, low confidence) — posted inline on run-state.sh:486: there's a narrow check-then-act window between the new symlink-resolution containment check and the mkdir -p "$run_dir" call that follows it — a component that didn't exist at check time could in principle be swapped for an outward-pointing symlink before mkdir -p runs. The code's own comments already disclose this as out of scope, and exploiting it requires an actor that already has write access to the plugin's own trusted data directory, so I'm not treating it as blocking. Flagging it because it's exactly the kind of trust-boundary/race reasoning static analysis (zizmor, shellcheck) won't catch.

Other areas reviewed, no issues found:

• No injection surface via --epoch, --record, --run-id, or --plugin-data — all are validated (integer/absolute-path/plain-segment patterns) before use in filenames or case patterns, and quoted case patterns treat variable expansions literally (no glob-metacharacter reinterpretation).
• jq/python3 invocations read untrusted data only via stdin, never as an interpolated filter/program string — no jq-filter or Python code injection.
• No new command-injection, path-traversal-via-.., or format-string surface introduced by this commit's changes to cmd_lease_acquire/cmd_partial_append.
• Doc changes (SKILL.md, report-location-and-schema.md, run-state-and-resumability.md) are consistent with the script's new --epoch/--plugin-data requirements — the exact inconsistency Codex flagged (SKILL.md vs. §7 disagreeing on whether --epoch is required) is resolved; both now show the full form with --epoch.
• CHANGELOG.md / plugin.json version bump: prose/version only, no security-relevant content.

No CRITICAL or IMPORTANT severity findings. The three defects this PR set out to fix, and the two gaps flagged by the prior automated review round on this same PR, all check out as genuinely closed in the current head.
· branch fix/2280-followup-run-state-hardening

[claude]

Claude finished @kyle-sexton's task in 2m 16s —— View job

---

Claude's Code Review

Tasks

• Gather context (PR diff, CLAUDE.md, REVIEW.md)
• Understand the request (code-review lane: correctness + maintainability; security left to the security lane)
• Review run-state.sh changes at current HEAD (4893dd2), focused on whether the latest commit closes the IMPORTANT findings from prior rounds
• Review run-state.test.sh changes
• Review docs (SKILL.md, reference docs, CHANGELOG.md, plugin.json)
• Post inline comments for findings
• Post final summary

Summary

This PR has already been through two prior automated review rounds on this same branch (at 8fb7583 and bb8a6cb). The newest commit, 4893dd2, is a direct, scoped response to the one IMPORTANT finding left open after the second round — the previous commit's physical_path() symlink-containment check required --plugin-data to already exist on disk (cd into a non-existent directory fails), which broke lease acquire on a plugin's very first run and contradicted the script's own comment that acquire "is the only command that creates a directory."

I traced the fix against the code and it closes the gap correctly:

• cmd_lease_acquire now does mkdir -p "$plugin_data" before resolving it with physical_path(), so the bootstrap case is no longer gated behind a precondition only a prior run could satisfy. This ordering is safe: --plugin-data is already validated as an absolute path by require_absolute_path (line 459) before the mkdir -p, and creating the plugin's own data root stays inside the script's stated write-scope (it's not a target-repo write). The containment check that follows (physical-path comparison against --run-dir's resolved ancestor) is unaffected and still runs after.
• The check-then-act window between symlink resolution and mkdir -p "$run_dir" — flagged as a low-confidence SUGGESTION in the prior round — is now recorded directly in the code as a disclosed residual (lines 482–487) rather than left implicit. Reasonable given closing it needs an atomic create-and-verify no portable shell offers, and the caveat's own precondition (write access inside the plugin's own data directory) already put it outside this script's trust boundary.
• The nit about require_int_at_least_one's hardcoded staleness-specific message being reused for partial append --epoch is also folded in: the helper now takes an optional why argument, and all three call sites (lines 437, 440, 746) now pass a context-appropriate reason — no call site was missed.
• A new test case (run-state.test.sh lines ~312–319) exercises the fresh-install path specifically with a directory tree that was never created ($TEST_TMPDIR/never-used/data), unlike every other case in the file which pre-creates the shared $DATA fixture — this is exactly the gap the prior review noted was invisible to the existing suite.
• SKILL.md Phase 3 (line 327) explicitly instructs the calling agent to stop on exit 3, and the FENCED-enforcement mechanism from the prior commit (EXIT_FENCED=3, returned from cmd_partial_append) is untouched by this commit — so that finding remains closed as reported last round.
• CHANGELOG, plugin.json (0.38.1), and both reference docs are all consistent with the current script behavior; I didn't find any doc now describing a behavior this commit changed.

No new findings. I did not have an approved shell in this session to execute run-state.test.sh or run shellcheck/markdownlint — the fixes above are a static trace against the diff, not an executed repro; the PR's own "73 checks passed" transcript and repo-gate output remain author-claimed and unverified by me.
· branch fix/2280-followup-run-state-hardening

[claude]

Claude finished @kyle-sexton's task in 1m 26s —— View job

---

Security review complete

• Gather context (PR diff, prior review rounds, custom instructions)
• Diff the latest commit (4893dd2) against what was already reviewed at bb8a6cb
• Re-verify the three original fixes still hold at current HEAD
• Check whether 4893dd2 reopens or introduces any new trust-boundary/write-tree gap
• Compile findings with severity + confidence
• Post final review

Scope: this PR's changed files only (run-state.sh, run-state.test.sh, CHANGELOG.md, SKILL.md, two reference docs, plugin.json). No workflow files touched, so zizmor's supply-chain/trigger/permissions lane doesn't apply.

This PR has already had two full security-review rounds at 8fb7583 and bb8a6cb, both posted on this thread. HEAD is now 4893dd2, whose only substantive change is plugins/claude-config/skills/audit-pass/scripts/run-state.sh — a direct fix for the IMPORTANT finding a prior review round raised (physical_path() on --plugin-data required the directory to already exist, so lease acquire couldn't bootstrap a fresh ${CLAUDE_PLUGIN_DATA} tree). I traced this fix specifically for whether closing that gap reopened either of the write-tree issues from earlier rounds:

Order of operations is still safe. cmd_lease_acquire validates --plugin-data/--run-dir as absolute paths, rejects .., and does the lexical "$plugin_data"/runs/* prefix check before the new mkdir -p "$plugin_data" on line 486. So the new directory-creation call cannot be reached with a run_dir that hasn't already passed containment against the caller-supplied plugin_data string — the new mkdir -p only ever creates the trust anchor itself (a path the caller explicitly named as --plugin-data), never anything derived from an unvalidated value.

No new escape via plugin_data as a dangling or pre-existing symlink. If --plugin-data is a symlink to an existing directory, mkdir -p is a no-op and the subsequent physical_path() call still resolves and canonicalizes the real target exactly as before this commit — this is the same symlink-handling behavior the prior round's fix already established, just now also reachable on a first run. If --plugin-data is a dangling symlink, mkdir -p fails (a file already exists at that path) and the script dies via || die, which is a safe closed failure, not a silent write elsewhere.

The disclosed check-then-act residual (plugin_data resolution → mkdir -p "$run_dir") is unchanged in kind, just now explicitly documented in the code comment (lines 480–491) rather than only in the CHANGELOG. This is the same narrow TOCTOU a prior round already flagged as SUGGESTION/non-blocking (requires an actor with pre-existing write access inside the plugin's own trusted data directory) — not a new or widened gap.

The require_int_at_least_one nit from the prior round (hardcoded staleness-specific message reused for partial append --epoch) is also fixed in this commit by threading a why argument through — not a security finding, just confirming the earlier nit is closed.

No new CRITICAL, IMPORTANT, or SUGGESTION security findings in this commit. All three original defects, plus the gaps raised across the two prior review rounds on this PR, check out as genuinely closed at current HEAD (4893dd2).