fix(claude-ops): harden lanes launcher refresh — abort on failure, validate targets first

[kyle-sexton]

Summary

Two deferred low-severity follow-ups from PR #616's review of the claude-ops lanes skill (lane-launcher.sh). Both hardened the pre-launch refresh step so lanes never start from state the user did not sign off on.

Fix

Item 1 — abort the launch on a failed refresh. action_start / action_restart ran refresh_repo_and_plugins || rc=1 and proceeded to for_each_lane regardless, so a failed git pull --ff-only (divergent/dirty checkout) or claude plugin marketplace update still seeded background lanes from stale repo/plugin state, surfacing non-zero only at the very end. The refresh is a documented start/restart prerequisite, so an unexpected failure now hard-stops the launch (refresh_repo_and_plugins || return 1) with an actionable message emitted from refresh_repo_and_plugins. --no-pull / --no-update remain the intentional-skip path: a skipped step leaves the refresh status 0, so the abort keys on real failure only, never on an intentional bypass.

Item 2 — validate targets before refresh mutations. restart does-not-exist ran the full refresh (git pull + marketplace update) before for_each_lane discovered the target was unknown and exited 3 — inconsistent with stop does-not-exist, which validates before anything happens. The TARGET_LANES existence check moved out of for_each_lane into a new validate_target_lanes function called from main ahead of action dispatch, so a misspelled target fails fast (exit 3) with no repo/plugin mutation. Single source of truth: for_each_lane's stale "unknown target names are an error" comment was updated to point at the new owner.

Verification

lane-launcher.test.sh grew from 71 to 85 cases (+14) covering both fixes: the refresh-failure abort (pull failure and marketplace-update failure, for both start and restart, plus the --no-pull/--no-update bypass still launching) and the pre-refresh target rejection (no pull/update in the command log). New stub knobs STUB_GIT_PULL_RC and STUB_CLAUDE_UPDATE_RC drive the failure paths.

$ bash plugins/claude-ops/skills/lanes/scripts/lane-launcher.test.sh
...
PASS: [85] restart unknown lane does not update the marketplace

lane-launcher.test: PASS — 85 cases

$ shellcheck plugins/claude-ops/skills/lanes/scripts/lane-launcher.sh plugins/claude-ops/skills/lanes/scripts/lane-launcher.test.sh
shellcheck: clean

$ bash -n plugins/claude-ops/skills/lanes/scripts/lane-launcher.sh   # syntax OK
$ jq -r '.version' plugins/claude-ops/.claude-plugin/plugin.json      # 0.15.1

Closes #639

Related

• ops/lanes: harden lane-launcher refresh step — abort on refresh failure; validate targets before mutating #639 — this issue (both items)
• feat(claude-ops): lanes skill — scripted background loop-lane launcher (#508) #616 — the lanes-skill PR whose review deferred these follow-ups
• ops: scripted lane launcher — start/restart loop lanes headlessly from canonical prompt files #508 — parent tracking issue

[claude]

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

---

PR Review — fix(claude-ops): harden lanes launcher refresh

• Gather context
• Read changed files
• Review logic correctness
• Review test coverage
• Post findings

---

The two fixes are correct and address the right problems. The code is clean, shellcheck-clean per the verification output, and the test structure is solid. A few observations below.

---

Correctness

Item 1 — refresh abort: The change from refresh_repo_and_plugins || rc=1 to refresh_repo_and_plugins || return 1 in both action_start and action_restart is exactly right. The intentional-skip invariant (skipped step leaves rc=0) is correctly documented in the comment block and verified by the bypass test.

Item 2 — validate-before-refresh: Moving the target check from inside for_each_lane into validate_target_lanes and calling it from main ahead of action dispatch is correct. One note: it now runs before load_sessions as well (not just before the refresh step) — this is a behavioral improvement (faster failure for stop bogus too), not a regression.

---

Minor observations

1. Error message emitted from refresh_repo_and_plugins rather than the caller (line 391)

((rc)) && err "refresh failed — aborting launch (pass --no-pull/--no-update to skip refresh intentionally)"
return "$rc"

The phrase "aborting launch" is stated inside refresh_repo_and_plugins, but the function itself doesn't abort — it returns non-zero and the callers abort. This works perfectly today (both callers abort on failure), but creates a latent correctness risk: if refresh_repo_and_plugins is ever called from a context that tolerates non-zero (e.g., a future --best-effort flag), the message will be wrong. Consider moving the message to the callers or softening it to something like "refresh step failed — callers should abort".

2. Marketplace-update failure test is missing the message assertion (line 366–372)

The git pull failure test checks:

assert_contains "start refresh-failure message is actionable" "$out" "refresh failed — aborting launch"
assert_not_contains "no lane launched after a failed pull" "$log" "--bg -n"

But the marketplace-update failure test only checks:

assert_eq "start aborts when marketplace update fails (exit 1)" 1 "$rc"
assert_not_contains "no lane launched after a failed update" "$log" "--bg -n"

The message assertion is missing for the update path. Fix this →

3. Bypass test only covers start, not restart (line 383–390)

The --no-pull --no-update intentional-bypass test runs start only. restart is also covered by Item 1 and has a slightly different code path (_restart_one vs _start_one). A matching bypass case for restart would round out the coverage.

---

CHANGELOG note

The refresh-abort entry is under ### Changed; it reads more like a ### Fixed (it was a bug: launch proceeded on failure). Not a blocker — the distinction is editorial.

---

Overall this is a clean, well-reasoned hardening pass. The two items are independent, test coverage is comprehensive for the regression cases that matter, and the code is consistent with the existing script conventions. Approve pending the minor observations above (none are blockers).

[claude]

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

---

PR Review — fix(claude-ops): harden lanes launcher refresh

• Gather context
• Read changed files (diff against origin/main)
• Review logic correctness and test coverage
• Check if prior review observations were addressed
• Post findings

---

Latest commit (302c3c3)

The synchronize push is purely cosmetic — the two block comments in the test file had their Issue #639 item N — prefixes stripped to satisfy CI's tracker-ref:issue-reference hygiene check. The substantive what/why descriptions are preserved in full. No logic changes; this is correct.

---

Outstanding from prior review

Three observations from the prior review remain unaddressed. None are blockers, but two (items 2 and 3) are easy one-line additions that would round out the coverage before merge.

1. Missing message assertion for the marketplace-update failure case (line 369–370)

The git pull failure block asserts the actionable message:

assert_contains "start refresh-failure message is actionable" "$out" "refresh failed — aborting launch"

The marketplace-update failure block does not. The message fires on the same code path (refresh_repo_and_plugins line 391) so the assertion would pass — it's just missing. Fix this →

2. Bypass test covers start only, not restart (line 380–386)

The intentional-bypass invariant (--no-pull --no-update leaves rc=0) is tested for start only. action_restart also calls refresh_repo_and_plugins || return 1 (the parallel fix from Item 1 of the PR) and goes through a different callback path (_restart_one). Adding a matching bypass case for restart --dry-run would give the same assurance that the invariant holds for both entry points. Fix this →

3. Error message "aborting launch" emitted inside the function that doesn't itself abort (line 391)

refresh_repo_and_plugins returns non-zero and the callers abort, but the message says "aborting launch" as if the function itself is stopping things. Today both callers abort, so this is safe; the latent risk is that a future caller that tolerates failure would emit a misleading message. Not a regression, and not blocking.

---

CHANGELOG note (editorial)

The refresh-abort entry is under ### Changed; it more naturally belongs under ### Fixed since it corrects behavior where a failure was silently swallowed. Not a blocker.

---

Overall

The two commits are correct. The substantive logic is sound, shellcheck-clean, and well-reasoned. Items 2 and 3 above are small, low-effort additions that would give the test suite full coverage of both the failure and bypass paths for both start and restart. Approve pending those if desired.

[claude]

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

---

PR Review — fix(claude-ops): harden lanes launcher refresh (latest commit: 15d8de6)

• Gather context
• Read changed files against origin/main
• Check if prior review observations were addressed
• Post findings

---

All three outstanding observations from the prior review are addressed in this commit. Nothing new introduced.

---

What changed in 15d8de6

1. Error message wording (lane-launcher.sh line 391)

Prior observation: "refresh failed — aborting launch" was stated inside refresh_repo_and_plugins, which doesn't itself abort — the callers do. Risk: a future tolerant caller would emit a misleading message.

Resolution: "aborting launch" dropped from the message. Now reads "refresh failed (pass --no-pull/--no-update to skip refresh intentionally)" — neutral and accurate regardless of what the caller decides. The corresponding test assertion was updated from "refresh failed — aborting launch" to "refresh failed" to match.

2. Missing message assertion for marketplace-update failure (line 370)

Prior observation: the start pull-failure test asserted the actionable message; the start marketplace-update failure test did not.

Resolution: assert_contains "start marketplace-update refresh-failure message is actionable" "$out" "refresh failed" added. Now both failure paths assert both exit code and message.

3. Bypass test covered start only, not restart (lines 389–396)

Prior observation: action_restart also calls refresh_repo_and_plugins || return 1 via a different callback path (_restart_one), but the bypass invariant was only tested for start.

Resolution: matching case added — restart work --no-pull --no-update --dry-run with both stub failure RCs set, asserting exit 0 and the dry-run relaunch preview. Comment explains the different callback path. The comment is clear and correct.

4. CHANGELOG ### Changed → ### Fixed (editorial)

Prior observation: the refresh-abort entry belonged under ### Fixed, not ### Changed.

Resolution: both entries consolidated under a single ### Fixed header. Correct.

---

Net test count

88 → 91 assertions (+3). All three additions are well-scoped and match the failure/bypass semantics of the code paths they exercise.

---

Verdict

Clean resolution of all prior observations. The diff is minimal, targeted, and introduces no new issues. The test suite now has symmetric coverage: both start and restart are tested for failure-abort, message emission, and bypass. Ready to merge.

[kyle-sexton]

Post-green review pass complete (rule 8). Bot review classification:

Finding	Verdict	Action
Missing message assertion (marketplace-update failure path)	VALID, non-blocking	Fixed — assertion added
Bypass test coverage gap (restart untested)	VALID, non-blocking	Fixed — matching restart bypass case added
Misleading "aborting launch" phrasing inside a non-aborting function	VALID, non-blocking (latent-risk only)	Fixed — reworded to be caller-agnostic
CHANGELOG section (Changed vs Fixed)	Editorial	Fixed — consolidated under Fixed

All 4 addressed on-branch, no inline threads to resolve, CI fully green (18/18), 91 test assertions passing.

Flipping to ready for review; handing off to the babysit lane for merge.

[claude]

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

---

PR Review — fix(claude-ops): harden lanes launcher refresh (ready-for-review pass)

• Gather context and read changed files
• Verify prior review observations are addressed
• Confirm test correctness and coverage
• Post final findings

---

This is a final review pass after the PR was flipped to ready-for-review. All four observations from the previous review cycles are confirmed addressed. Nothing new introduced.

---

Prior observations — all resolved

1. Error message wording (line 391)

The misleading "aborting launch" phrase inside a function that doesn't itself abort is gone. Now reads "refresh failed (pass --no-pull/--no-update to skip refresh intentionally)" — neutral and correct regardless of what the caller decides. The test assertions were updated from "refresh failed — aborting launch" to "refresh failed" to match. ✓

2. Missing marketplace-update message assertion (line 370)

assert_contains "start marketplace-update refresh-failure message is actionable" "$out" "refresh failed" is present. Both failure paths (git pull and marketplace update) now assert both exit code and message. ✓

3. Bypass test covered start only (lines 389–396)

Matching bypass case added for restart — uses AGENTS_RUNNING, --no-pull --no-update --dry-run, asserts exit 0 and dry-run relaunch preview. Comment explicitly calls out the _restart_one callback path. ✓

4. CHANGELOG section (CHANGELOG.md)

Both entries consolidated under ### Fixed. ✓

---

Correctness check on the new stubs

The STUB_CLAUDE_UPDATE_RC stub fires on $1 == "plugin" — matching claude plugin marketplace update. The STUB_GIT_PULL_RC stub fires on *pull* in $* — matching git -C <repo> pull --ff-only. Both are correct for the code paths they gate. The comment notes that git is only ever invoked for pull in this script (repos passed via --repo), which removes the risk of a too-broad pattern.

---

One minor gap (not previously flagged, not blocking)

The restart + marketplace-update failure path has no dedicated test (the test matrix covers restart + pull failure and start + marketplace failure, but not restart + marketplace failure). Since both go through the same refresh_repo_and_plugins || return 1 in action_restart, the risk is extremely low — but a future contributor extending the test suite might notice the asymmetry. Worth a follow-up note in #639 if anything.

---

Net state

91 test assertions, shellcheck-clean, version bumped to 0.15.1, CHANGELOG correct. The two hardening items are logically correct and symmetric. The diff is minimal, targeted, and introduces no new issues.

Ready to merge.