fix(lanes): latch marker consumption; stop reading a JSON false as absent - #1851
Conversation
![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: 955675ec29
ℹ️ 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".
…sent Two stranded findings from #969, re-verified against origin/main. lane-stop-gate.sh: the completion marker's one-shot authorization was latched solely by deleting the file, which lives in the watched checkout. An `rm` the OS refuses left a file that still satisfied `[[ -f ]]` on a later, unrelated lane run. Consumption is now recorded in the plugin's own data directory, derived from the hook's install path under the documented plugins/cache anchor; the delete is the tidy-up rather than the latch. lane-launcher.sh: both field readers used jq's `//` alternative, which fires on every falsy value rather than on absence. `"settings": false` yielded empty, reached bash as "", skipped the guarded type check, and launched the lane with `--settings` silently omitted. Presence is now tested with `has`, and the scalar fields are typed once at config time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW
955675e to
79c8f7f
Compare
|
Claude finished @kyle-sexton's task in 4m 43s —— View job Security review — PR #1851
Scope: only the 9 files this PR touches (no workflow files are among them, so no CI-permission/trigger surface to review). No secrets, hardcoded credentials, or injection into One finding in the new marker-consumption ledger in IMPORTANT / confidence: PLAUSIBLE — cksum collision lets one marker's consumed-record silently overwrite another's, reviving a stale marker's authorization
The ledger keys each marker's consumed-record purely on Concretely:
This requires stacking the already-failing- No other security issues found in this PR's diff. (The Codex bot's P2 finding about |
|
Claude finished @kyle-sexton's task in 4m 35s —— View job Code review — PR #1851
Scope per Important — recreating a marker with the same size inside the same wall-clock second is silently treated as still-consumed, defeating the recreation-invalidation guarantee
This isn't a security bypass (it fails in the over-blocking direction, consistent with the file's own stated "strict direction for a gate" philosophy for the undetectable-identity case) — but it undermines the specific guarantee this PR adds and tests (case 21: "recreating the marker authorizes again"), for exactly the marker style (empty flag file) this plugin's own test suite uses as its convention. It requires the same failed- Suggest widening the identity signal beyond second-granularity mtime + size — e.g. an inode number ( Everything else checked out well:
I could not execute the shell test suites or shellcheck myself in this sandbox ( |
|
Claude has reviewed this PR 4 times. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
…'s real resolution Review found the marker-identity read overclaims. Both portable `stat` dialects report whole-second mtime, so a marker recreated at the same size within the same second — an empty `touch`-style marker being the realistic case — is indistinguishable from the consumed one and stays latched. Sub-second and inode spellings are GNU-only, and this identity feeds a gate: the coarse read costs a stop delayed to the next second, while a wrong "recreated" verdict costs the unearned second authorization the ledger exists to prevent. Delay is the correct failure direction, so the portable spelling stands and the claim is narrowed to match it. A test pins the boundary and reports which side of the second it landed on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW
|
Claude finished @kyle-sexton's task in 1m 33s —— View job Security review — PR #1851 (re-run at HEAD
|
|
Claude finished @kyle-sexton's task in 2m 11s —— View job Code review — PR #1851 (re-run at HEAD
|
Two conflicts, both in `claude-ops`, both from main's 0.25.0 landing while this branch held an unreleased 0.24.5. - `.claude-plugin/plugin.json`: 0.24.5 vs 0.25.0 → 0.25.1. This branch's change is a Fixed entry, so it is a patch on top of main's minor, not a re-take of 0.25.0. - `CHANGELOG.md`: both sides opened a new top section. Composed rather than chosen — the `false`-read-as-absent entry is re-headed 0.25.1 above main's intact 0.25.0. Semantic check beyond the markers: this branch types `name`/`model`/`effort`/ `prompt` as strings at config time, and main added `lanes[].telemetry.instance` to the same `.lanes[]` config that `restart-consumer.sh` reads. The type gate selects only those four scalar keys, so main's nested `telemetry` object is untouched by it and no field main added is now rejected. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013dsTvnGqMJeLTZe4DpGGVQ
…ssert The narrowing in f0c67db got the conclusion right and the mechanism wrong. It said a same-second, same-size recreation "stays latched until the second turns over", which reads as self-healing on the next tick. It is not: `stat` reports the file's mtime, and an mtime does not advance on its own. Once the recreation collides with the recorded identity, that record holds for as long as the marker goes unwritten — what clears it is the marker's NEXT write landing in a different second, not the clock passing one. The cost is that one completion signal, and the one after it authorizes. The CHANGELOG and the `marker_identity` comment now say that; the surviving claim is unchanged, since a stop delayed is still the gate's correct failure direction. `marker_already_consumed`'s own comment asserted a recreated marker "authorizes normally" without qualification, which is the same overclaim one function down. Case 21b raced for the collision and called `ok` on both branches, so it could not fail and pinned nothing — while its comment claimed a future finer-grained identity would have to move it deliberately. It now forces the collision with `touch -r` from the consumed file's own timestamp and asserts the blocked outcome, which is what makes that claim true. Verified the force is load- bearing: `touch -r` reproduces an exact whole-second mtime across a 3s gap. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013dsTvnGqMJeLTZe4DpGGVQ
|
Claude finished @kyle-sexton's task in 1m 22s —— View job Security review — PR #1851 (re-run at HEAD
|
|
Claude finished @kyle-sexton's task in 2m 11s —— View job Code review — PR #1851 (re-run at HEAD
|
…dentity The comment's own correction two sentences up says an mtime does not advance on its own, then the cost clause still read "a stop delayed to the next second" — the framing that correction exists to retire. The coarse read costs one skipped completion signal, not a sub-second wait, which is what the CHANGELOG already says. Wording only; no behavior change. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013dsTvnGqMJeLTZe4DpGGVQ
|
Claude finished @kyle-sexton's task in 1m 39s —— View job Code review — PR #1851 (re-run at HEAD
|
|
Claude finished @kyle-sexton's task in 49s —— View job Security review — PR #1851 (re-run at HEAD
|
…ed channels only (#1865) ## Summary Closes the remaining P1 of #1784. `lane-stop-gate.sh` read `CLAUDE_PLUGIN_OPTION_LANE_STOP_GATE_ENABLED` straight off the environment — channel B of `docs/conventions/hook-config-delivery`, whose rule 3 requires channel F for a safety-critical optional-with-default toggle, and whose fact 4 records that an unconfigured key lets a watched repository's own `.claude/settings.json` `env` block populate `CLAUDE_PLUGIN_OPTION_*` freely. A gate whose enablement the watched repository controls is not a gate. Per-key resolution is now, in precedence order: managed settings (fixed root-owned paths plus `managed-settings.d/` drop-ins) ▷ the per-session arm record ▷ the user `settings.json` located only from the hook's own `plugins/cache` install anchor ▷ the in-script defaults. No path any of these reads is env-derived; the managed-settings platform comes from `uname -s`, not the repo-settable `$OSTYPE`, and the resolved primary is asserted absolute so it can never become a cwd-relative (repo-plantable) path. The env mirrors are never read as values — presence alone only decides whether to evaluate and whether to surface a visible once-per-session notice. Because a hook cannot observe `--settings` (channel F's documented residual), the per-session opt-in the launcher previously shipped that way needed a trusted replacement rather than deletion. New channel **G**: `hooks/lane-stop-gate-arm.sh` writes a per-session record under the plugin's own install-derived data directory, and the session carries only a random record id. The id is a capability pointer, never authority — shape-validated before any path use, looked up only in the install-anchored store, claimed by the first presenting session so a replay is refused, and TTL-bounded. `lane-launcher.sh` arms a gate-requesting lane at launch and fails closed: a lane that cannot be armed is skipped with an error rather than launched silently ungated. The two sibling P2s of #1784 landed separately in #1851 and are untouched here; this branch was rebased onto them and both fixes were confirmed intact (`lane_json_field`'s `has($k)` check and the marker-consumption ledger). ## Fixes from independent review A fresh-context audit of this branch, run with the rationale withheld, found four defects in the work; all four are fixed here: - Keying every settings read on the marketplace-qualified id left the **managed scope contributing no verdict without a `plugins/cache` anchor** — silently disabling the org-mandate path on the one install class (`--plugin-dir`) for which it is the only enable path, and on whose availability the arm helper's refusal to arm there is premised. An unanchored install now matches on the manifest name beside the hook; anchored installs keep their exact-id match. - The launcher **accepted a partial arm**: one helper succeeding marked the lane armed, though each install writes into its own store and the launcher cannot tell which one the session loads. Every discovered helper must now arm. - The presence preflight read discovery as `find_gate_arm_scripts | grep -q .`, which **under `pipefail` takes SIGPIPE on the producer's second write** — so a machine carrying two autonomy installs read as "no helper found" and was refused a gate-requesting launch outright. - An **empty configured sentinel** silenced the token channel while the block reason still instructed the agent to emit an empty token on its own line. It now falls back to the default. The audit also found a defect that is **not** fixed here and is filed as **#1883**: every hook built on `hook::buffer_stdin` can be disengaged by a repo `env` block setting `CLAUDE_PLUGIN_OPTION_STDIN_READ_TIMEOUT` below the read's practical resolution (`lib/hook-utils.sh` rejects exact zero but accepts `0.000001`). It is pre-existing on `main`, is not the enable flag #1784 names, and lives in a shared library materialized into 16 plugin copies — a fleet-wide change that deserves its own review rather than riding this PR. ## Test plan Full gate battery run against the shipped tree: - [x] `plugins/autonomy/hooks/lane-stop-gate.test.sh` — PASS=72 FAIL=0 - [x] `plugins/claude-ops/skills/lanes/scripts/lane-launcher.test.sh` — PASS, 169 cases - [x] `shellcheck --rcfile .shellcheckrc` on all six changed shell files — clean - [x] `scripts/check-changelog-parity.sh` `--check`, `--check-order`, `--check-bump origin/main` - [x] `scripts/check-changed-skills.sh origin/main` - [x] `scripts/check-shell-portability.sh origin/main` — no unexcused GNU-only constructs - [x] `markdownlint-cli2` on the changed docs — 0 errors Regression evidence, observed rather than asserted: - Substituting `origin/main`'s `lane-stop-gate.sh` under the new suite yields **PASS=54 FAIL=14**, with the pinned attack case reporting `env-only ENABLED=true wrongly engaged the gate (channel-B authority)`. - The four assertions added for the review fixes yield **FAIL=4** against this branch's own pre-fix commit, including the empty-token nudge reproduced verbatim. - The multi-install launcher case fails against the pre-fix launcher, which is how the SIGPIPE defect above was found. ## Related Closes #1784 Refs #1883 --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…gate arming to the installs that asked (#2050) Three bot-filed P2 defects in the `claude-ops` lanes launcher, all verified to reproduce at `origin/main` and to stop reproducing here. Every fix is covered by a new assertion that FAILS against the pre-fix launcher and passes against this one. ## 1. The launch-commit marker key was digested in the wrong repository (#1383) `git hash-object` uses the object format of whatever repository it resolves. The launcher called it **unscoped**, so it keyed on the *caller's* format while taking the toplevel from the repository `--repo` names. Reached from a SHA-1 working directory, a SHA-256 target produced a 40-character key, while `skills/lanes/context/refresh.md`'s probe runs inside that checkout and computed the 64-character one — the launcher wrote its marker to a directory the probe never reads and staleness detection was silently off. The comment above the key asserted the two sides agree because both call `git rev-parse --show-toplevel`. That settles the *path* and says nothing about the *digest*, so the invariant it claimed did not hold. Both digests are now taken with `-C "$REPO"`. The anchor is `$REPO` (guaranteed by `resolve_repo` to be an existing directory) rather than the hashed `$top` (a string git handed back) — anchoring on a path that may not exist would fail the digest into the `unkeyed` fallback and collapse every such repo onto one key. **Scope is broader than the filed report:** `restart-consumer.sh` derived its ledger key the same unscoped way and is fixed with it. The hand-recompute snippets in the README, the changelog, and `refresh.md` already run inside the target repository and were correct as written; they are untouched. ## 2. An explicitly empty stop-gate marker was read as an absent one (#1865) `gate_option_from_settings` ended `select(type == "string") ] | last // empty`, which prints nothing for an explicit `""` and nothing for an absent key. `[[ -n "$marker" ]]` then dropped `--marker` for both, the arm record carried no marker key at all, and `lane-stop-gate.sh`'s precedence (managed ▷ arm record ▷ user settings ▷ default) walked past it to the user-level marker — where a marker file left over from another lane can authorize a stop this lane never signaled. A `v:` prefix now carries "the lane set this" through the shell, so an explicit empty value reaches the helper as `--marker ""`, `lane-stop-gate-arm.sh` records `{"marker": ""}`, and the gate's `[[ -n "$MARKER" ]]` guard leaves the marker channel off instead of falling through. **The sentinel is deliberately not symmetric.** `lane-stop-gate.sh` substitutes the default token for an empty sentinel, so emptiness is not a configured value there; recording one would buy no behavior change while shadowing the user-level sentinel. An empty sentinel is therefore still treated as absent, and a fixture pins that asymmetry. ## 3. The stop-gate arm id reached installs that never asked for it (#1865) Arming keyed off an any-quantifier over the `autonomy` / `autonomy@*` namespace, then injected `lane_stop_gate_arm_id` into **every** entry in it, and option extraction took its last match from any entry rather than a requesting one. The gate never treats this channel as a trusted verdict in either direction, so an id landing on an entry set to `false` was not overriding that `false`. What it did do is mark installs the lane never asked to arm — leaving the settings handed to `claude` an inaccurate record of what was requested, and letting a non-requesting entry's marker reach the arm call. One shared filter now defines "an entry that requested the gate", and detection, option extraction, and injection all use it. Arming every discovered helper script is unchanged and deliberate. ## Tests `lane-launcher.test.sh` grows a SHA-256 cross-format marker fixture (skipped where git cannot create a SHA-256 repository) and four gate-arming fixtures. Against the pre-fix launcher with this test file, six assertions fail: - `marker: written under the TARGET repo's object-format key` - `marker: nothing is written under the caller-format key` - `arm: an explicitly empty marker still reaches the helper` - `arm: options come from the requesting entry` - `arm: a disabled sibling's marker never reaches the helper` - `arm: the explicitly-disabled entry receives no arm id` All six pass here; the suite is 193 assertions, 0 failures. ## Verification (independent re-run) The pre-fix control was reproduced by copying the scripts directory to a scratch path, replacing `lane-launcher.sh` with `origin/main`'s, and running this branch's **unchanged** test file against it: `lane-launcher.test: FAIL — 6 case(s) failed` there, `PASS — 193 cases` here. The six failures are exactly the list above, so no fixture is passing on both trees. The SHA-256 block **executed** rather than skipping — `git version 2.54.0.windows.1` creates `--object-format=sha256` repositories, and cases 158-160 report PASS. The skip guard remains because the format is not universally compiled in. Beyond the arm stub: `lane-stop-gate-arm.sh` invoked directly with `--marker ""` writes `"marker": ""` into the arm record, while omitting the flag writes no `marker` key at all. The launcher's explicit-empty distinction therefore survives to the gate, whose `gate_option` returns the empty string (via the same `v:` idiom) rather than falling through to user settings. Folding the key test and the value test into one `select` adds no type fragility: jq's `and` short-circuits, so a non-autonomy scalar entry is never indexed, and an *autonomy* entry whose value is a scalar errors identically under the old and new filters. Gates from the worktree root, all green: `check-changelog-parity.sh` `--check` / `--check-bump origin/main` / `--check-order`, `check-shell-portability.sh`, `check-skill-portability.sh`, `check-silent-skips.sh`, `check-plugin-manifest-presence.sh`, `check-changed-skills.sh origin/main` (0 errors; one pre-existing SKILL.md-length warning), `validate-plugins.sh`, `markdownlint-cli2` on the changelog, and `shellcheck -x` on all three changed scripts. Version renumbered to `0.27.6` — `main` published `0.27.4` and then `0.27.5` while this branch was in flight. ## Related No linked issue The three findings were filed as review threads on merged PRs #1383 and #1865, not as issues. Those PRs are referenced for provenance only — this PR closes nothing. A fourth thread on #1383, `PRRT_kwDOTCGFQM6TzlNw`, needs no change here: the vacuous-traversal escape it describes was already closed by `353baf64` (#1851), and a control at `353baf64^` reproduces it. Its adjacent defence-in-depth observation — the three preflight `jq` substitutions in `lane-launcher.sh` that ignore exit status — is deliberately left for a separate change. Lines 1-406 of that file are byte-identical to `main` and all seven `$(jq …)` command substitutions in it are unchanged; the only edit above the marker-key block is line 407, where the property list's own count went from "Two" to "Three". --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
No linked issue
Summary
Fixes two of the three defects re-verified in #1784 — the two stranded #969 findings. Both were a
value that is present but falsy being read as absent.
#1784 is deliberately NOT closed by this PR. Its P1 (the lane-stop gate reading its enable flag
off channel B) is untouched — see "Not addressed" below.
Fix
plugins/autonomy/hooks/lane-stop-gate.sh(0.11.7 → 0.11.8). The completion marker's one-shotauthorization was latched solely by deleting the file, and the marker lives in the watched checkout —
a directory the hook is not guaranteed to be able to write. An
rmthe OS refused left a file thatstill satisfied
[[ -f "$MARKER" ]]on the next run: exactly the cross-run bypass that consuming themarker exists to close. The surrounding comment asserted "the next run must not rely on that stale
file" while nothing enforced it.
Consumption is now recorded in the plugin's own persistent data directory — the marker path plus the
consumed file's identity (mtime and size) — and the deletion is the tidy-up rather than the latch. A
marker recorded as consumed is not a signal however long it survives on disk; a marker later
recreated has a new identity, so the stale record is dropped and the fresh marker authorizes
normally.
Recreation recovery is best-effort by design, and the second commit scopes the claim to what the
identity read actually resolves. Both portable
statdialects report whole-second mtime, so a markerrecreated at the same size within the same second — an empty
touch-style marker being the realisticcase — is indistinguishable from the consumed one and stays latched until the second turns over.
Sub-second (
%.9Y) and inode spellings would narrow that window but are GNU-only, and this identityfeeds a gate: the coarse read costs a stop delayed by under a second, while a wrong "recreated"
verdict costs the unearned second authorization the ledger exists to prevent. Delay is the correct
failure direction, so the portable spelling stands and the comment and CHANGELOG say so rather than
implying recovery is guaranteed. A host where neither dialect reports an identity holds the record
for the same reason.
The data directory is derived from the hook's own install path (the
plugins/cacheanchor ClaudeCode documents), falling back to
CLAUDE_PLUGIN_DATAonly for a--plugin-dirinstall carrying nosuch anchor: the script's own location is not something a watched repository can redirect, whereas
CLAUDE_PLUGIN_DATAis an env value a reposettings.jsonenvblock reaches. Where no datadirectory can be written, the deletion remains the only latch — the behavior that predates this
ledger.
plugins/claude-ops/skills/lanes/scripts/lane-launcher.sh(0.24.4 → 0.24.5). Both field readersused jq's
//alternative operator, which fires on every FALSY value rather than on absence. A laneconfigured
"settings": falseyieldedempty, reached bash as"", and — becausevalidate_launch_inputsguards its "settings must be a JSON object" check on[[ -n "$settings" ]]—that type check never ran at all: the lane launched with
--settingssilently omitted, no error,nothing for the operator to see.
lane_json_fieldnow tests presence withhas, sofalsereachesthe type check and the lane is skipped with the error already written for it. The scalar reader had
the same collapse for
name/model/effort/prompt(a mistyped"effort": falselaunched a lanewith no effort), so those fields are typed once at config time and a non-string value is a config
error alongside the existing duplicate-name and path-traversal checks. An explicit
nullstays theJSON spelling of "no value" and remains equivalent to an absent field in both readers.
Test plan
Red-to-green proven for both defects by running the new cases against the pre-fix sources checked
out from
origin/maininto a staged copy, then against the fixed sources.origin/mainsource)plugins/autonomy/hooks/lane-stop-gate.test.sha surviving consumed marker wrongly authorized a later runplugins/claude-ops/skills/lanes/scripts/lane-launcher.test.shboolean .name/.model/.effort/.promptcases plussettings:false reaches the type check,settings:false lane not launched,settings:false surfaces a non-zero exitCase counts differ across the two columns for the gate suite because cases 21b and 22–23 were added
after that red run; the red run is the marker regression alone.
Case 21b pins the same-second/same-size recreation boundary described above, and reports which side
of the second it landed on rather than asserting a timing race — so a future finer-grained identity
has to move that case deliberately.
A defect the new coverage found. The first draft of the marker ledger tested only the
CLAUDE_PLUGIN_DATAfallback — never the install-path derivation the tamper-resistance claimactually rests on. Cases 22–23 stage the hook under a synthetic
<root>/plugins/cache/<marketplace>/<name>/<version>/hooks/tree with an unrelatedCLAUDE_PLUGIN_DATApresent, and they failed:gate_data_dirappended/pluginstwice, writing to<root>/plugins/plugins/data/<id>instead of the documented<root>/plugins/data/<id>. The layoutwas checked against a real install on this machine (
~/.claude/plugins/cache/melodic-software/autonomy/<ver>alongside
~/.claude/plugins/data/autonomy-melodic-software) and the path is corrected here.Gates run locally against
origin/mainas base before pushing, all green (and re-confirmed by thefull CI run on this PR):
scripts/check-changelog-parity.sh --check,--check-bump origin/main,--check-orderscripts/check-changed-skills.sh origin/mainscripts/check-silent-skips.shscripts/check-shell-portability.sh origin/mainshellcheck --rcfile=.shellcheckrcover all four changed shell filesmarkdownlint-cli2 --config .markdownlint-cli2.jsoncover the two CHANGELOGs andconfig.mdlane-notify,machine-behavior,restart-consumer,telemetry-upsertNot addressed
#1784's P1 — the lane-stop gate reads its enable flag off channel B — is not fixed here, and the
issue stays open for it.
lane-stop-gate.shstill readsCLAUDE_PLUGIN_OPTION_LANE_STOP_GATE_ENABLEDfrom bare env, whichdocs/conventions/hook-config-delivery/README.mdrule 3 forbids for a safety-criticaloptional-with-default toggle (fact 4: an unconfigured key lets a repo's own
settings.jsonenvblock populate it freely).
That fix is a channel-F reader, and the only shipped exemplar is Python
(
plugins/disk-hygiene/lib/killswitch_config.py); a bash equivalent needs the${CLAUDE_PLUGIN_ROOT}anchor, the fixed-path managed-settings table plus
managed-settings.d/drop-ins, an explicit faildirection, and an entry in the convention's Adopters table. That is its own design and its own
security review, not a rider on this one. Acceptance criterion 1 of #1784 therefore remains unmet;
criterion 5 is only partly reachable — two of the three #969 threads become resolvable when this
lands, the channel-B thread does not. #1784 remains the tracker for both.
Related
Refs #1784
Refs #969
Refs #1777
Refs #1781