feat(claude-ops): lane-launcher captures and persists the launch commit - #1383
Conversation
context/refresh.md's git staleness probe referenced a <lane-launch-commit>
placeholder with no producer: lane-launcher.sh never recorded the repo HEAD
when a lane last (re)started, so the probe was advisory-only with no
automated way to fill it in.
lane-launcher.sh now captures `git rev-parse HEAD` right after the pre-launch
pull and writes it, for every lane actually (re)started that run, to
<data-dir>/lanes/<lane>-launch-commit (a lane `start` skips as
already-running keeps its existing marker). New --data-dir option; SKILL.md
threads ${CLAUDE_PLUGIN_DATA} through explicitly since a skill-invoked
script does not inherit it as an env var the way hook/MCP/LSP subprocesses do
(verified against current plugins-reference). The write is best-effort and
never fails an already-launched lane.
Closes #792
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 2m 23s —— View job PR #1383 Review
SummaryThis is a well-scoped, high-quality fix. The core problem was real:
|
|
Claude finished @kyle-sexton's task in 5m 15s —— View job Security Review
Scope: Files changed in this PR — SUGGESTION — Lane name used as file path component without character validationConfidence: CONFIRMED
Lane names are read from the config with printf '%s/%s-launch-commit' "$(resolve_data_dir)" "$1"No character set restriction is enforced on the name component. A lane name containing The written content is a hex SHA from Recommendation: validate lane names against a safe character set (e.g., SUGGESTION — Staleness probe omits the format guard the document itself recommendsConfidence: PLAUSIBLE The probe snippet reads the marker file and interpolates its content into a git command: lane_launch_commit="$(cat "$data_dir/lanes/<lane>-launch-commit" 2>/dev/null | tr -d '\r')"
[[ -n "$lane_launch_commit" ]] && git log --oneline "${lane_launch_commit}..${default}" -- plugins/claude-ops/The document at lines 71-79 explicitly identifies this surface and prescribes a validation guard — Shell injection is not possible here: Nonetheless, the document already has the right answer and the probe should model it. Operator-executed probe snippets are more likely to be copied verbatim than followed with careful annotation-reading. Recommendation: add the guard the document already cites, directly in the code block — between the # Validate: must be a bare hex SHA (the only value lane-launcher.sh ever writes).
[[ "$lane_launch_commit" =~ ^[0-9a-f]{7,64}$ ]] || { echo "marker invalid — skipping probe"; lane_launch_commit=""; }No findings on remaining changed files
|
![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: 912d25cb8d
ℹ️ 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".
…-capture-launch-commit # Conflicts: # plugins/claude-ops/CHANGELOG.md
Three review findings on the launch-commit marker. The probe in context/refresh.md resolved its data dir from a $CLAUDE_PLUGIN_DATA env fallback, but that file is read raw (no placeholder substitution) and the Bash tool never receives the variable — only hook and MCP/LSP subprocesses do. The probe therefore fell through to the unqualified ~/.claude/plugins/data/claude-ops guess, read no marker, and skipped the staleness check silently. SKILL.md, which is skill content and does substitute, now carries the data_dir assignment; refresh.md points at it. A lane name is the marker's filename, so config preflight now rejects a name that is not a single path component: `work` and `group/../work` otherwise shared one marker and a targeted restart of either corrupted the other's probe. A (re)start that cannot record its commit now removes the previous launch's marker instead of leaving it to be read as this session's launch point.
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Warning Automated security review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
|
Warning Automated review did not complete — this is an infrastructure failure, not a review verdict. Treat any Claude comment on this PR (including a placeholder like "I'll analyze this and get back to you") as incomplete, not "no findings."
Re-running the job, or pushing a new commit, will retry the review. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dfe17a7cce
ℹ️ 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".
The data directory is plugin-wide, but a lane name is only unique within one repo — `work` is a conventional name in every checkout the launcher manages. Starting `work` in repo B overwrote repo A's marker, so A's staleness probe diffed against a SHA from an unrelated history: an invalid-revision error at best, a silently wrong answer at worst. The marker path gains a repo component: <data-dir>/lanes/<repo-key>/<lane>-launch-commit, where <repo-key> is the resolved absolute repo path folded to [A-Za-z0-9_-] — the same shape Claude Code uses for its own per-project directories.
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3eeb474701
ℹ️ 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".
The character fold introduced with the repo namespace was not injective: /repos/foo-bar and /repos/foo/bar collapse onto one key, reinstating the very collision the namespace exists to prevent. And it keyed on the --repo argument verbatim, while the documented probe asks git for the toplevel — so a --repo naming a symlink wrote under one key and the probe read another. Both sides now derive the key the same way: git hash-object over `git rev-parse --show-toplevel`. README's persistent-state section, which still advertised the pre-namespace path, carries the new layout and the one-liner that prints a checkout's key.
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 2s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b3bfa5d8b9
ℹ️ 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".
| local top | ||
| top="$(git -C "$REPO" rev-parse --show-toplevel 2>/dev/null)" || top="" | ||
| [[ -n "$top" ]] || top="$REPO" | ||
| REPO_MARKER_KEY="$(printf '%s' "$top" | git hash-object --stdin 2>/dev/null)" |
There was a problem hiding this comment.
When --repo targets a SHA-256 repository but the launcher is invoked outside that checkout or from a SHA-1 repository, this unscoped git hash-object uses the caller's repository format. With Git 2.43, the launcher consequently produces a 40-character SHA-1 key while the documented refresh probe, run inside the target checkout, produces a 64-character SHA-256 key for the same canonical path. The marker is then written under a directory the probe never reads, silently disabling staleness detection; run this hash with git -C "$REPO" so both sides use the target repository's object format.
Useful? React with 👍 / 👎.
| traversal="$(jq -r ' | ||
| [ .lanes[].name | ||
| | select(. != null) | ||
| | select(test("[/\\\\]") or . == "." or . == "..") ] | join(", ")' "$CONFIG")" |
There was a problem hiding this comment.
When any earlier lane has a non-string name, jq raises number cannot be matched (or the equivalent type error) at test(), but the script does not use set -e or check this command substitution's status. traversal therefore remains empty and preflight continues without examining later names, so a subsequent ../escape name reaches marker-path construction despite the new containment check. Validate every name's type before calling test, and fail the config when the validation query itself fails.
Useful? React with 👍 / 👎.
…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
Summary
context/refresh.md's git staleness probe (added in #514 / PR #791) referenced a<lane-launch-commit>placeholder described as "the repo HEAD whenlanes start/restartlast ran" — butlane-launcher.shnever captured this value anywhere. An operator following the probe literally had no automated way to fill in<lane-launch-commit>; it was advisory-only.Fix
lane-launcher.shcapturesgit rev-parse HEADright after the pre-launchgit pull(a pure read, so it also previews correctly under--dry-run) and writes it, for every lane actually (re)started that run, to<data-dir>/lanes/<lane>-launch-commit(bare hex SHA + newline). A lanestartskips as already-running keeps its existing marker untouched — only lanes that actually launch this run get recorded. Best-effort: a write failure or an unresolvable HEAD warns on stderr but never fails an already-launched session.--data-dir DIRoption (default:$CLAUDE_PLUGIN_DATAenv var if set, else~/.claude/plugins/data/claude-ops, matchingcheck-all.sh's existing fallback convention in the same plugin).SKILL.md's invocation now passes--data-dir "${CLAUDE_PLUGIN_DATA}"explicitly. Per current plugins-reference (fetched and verified this session, per the repo's fresh-docs mandate):CLAUDE_PLUGIN_DATAis exported as a real environment variable only to hook processes and MCP/LSP subprocesses — for skill content it instead resolves by inline text substitution "anywhere the placeholder appears" in the rendered skill body, exactly like the existing${CLAUDE_PLUGIN_ROOT}usage on the same line. A script a skill shells out to via the Bash tool does not inheritCLAUDE_PLUGIN_DATAas an env var, so leaving--data-diroff would silently fall through to the script's own~/.claude/plugins/data/claude-opsguess instead of the real marketplace-qualified directory Claude Code resolves.$ARGUMENTScomes after the injected--data-dir, so an operator-supplied--data-dirstill wins (last flag wins in the parser).context/refresh.mdnow points the probe at the real marker file (cat "$data_dir/lanes/<lane>-launch-commit" | tr -d '\r'— the repo's standing CRLF-hazard convention for any captured Windows value, e.g. the 0.19.1 CHANGELOG's claude-ops: fix plugins-skill default-marketplace resolver on version skew; generalize CRLF gotcha; correct install_new render doc (F1-F3) #1176/F2 note) instead of the unfillable placeholder, with an explicit note that a hex-onlygit rev-parsevalue carries no injection risk but any future non-git-rev-parsesource must be validated before reachinggit log.README.md/SKILL.md— documented the new persisted artifact and gotcha (per-machine, best-effort; a missing marker means "never started here vialane-launcher.sh", not "launcher broken").0.19.2→0.20.0(minor — new backward-compatible CLI option + new persisted artifact) + matchingCHANGELOG.mdentry.Verification
lane-launcher.test.sh: 100/100 PASS (12 new cases covering capture/write, skip-if-already-running,--dry-runpreview-only, unresolvable-HEAD best-effort, and the$CLAUDE_PLUGIN_DATAfallback).shellcheckclean on bothlane-launcher.shandlane-launcher.test.sh.shfmt -d— no formatting diff.markdownlint-cli2clean on all touched markdown (SKILL.md,context/refresh.md,README.md,CHANGELOG.md).skill-quality:check lanes— PASS, 0 errors, 1 soft warning (SKILL.md 216/200 soft line target — pre-existing category, not introduced by this change beyond the doc additions).skill-quality:check validate-evals lanes— conforms (check-jsonschema,ok).Closes #792
Related
reclaimseam verb was blocked by the local auto-mode permission classifier when run against ~44 stale self-assigned items at session start (a pre-existing gap the loop's own permission preflight independently flagged as operator-side, not self-fixable). Reclaim is orthogonal to this item — claude-ops: lane-launcher never captures launch commit, leaving refresh.md's staleness probe unfillable #792 itself had no assignee/branch/PR before this session claimed it via the seam'sclaimverb, which succeeded normally.This was generated by AI during work-loop execution.