Skip to content

feat(claude-ops): consume lane restart-requests via scheduled headless reader - #1720

Merged
kyle-sexton merged 8 commits into
mainfrom
feat/1653-restart-request-consumer
Jul 29, 2026
Merged

feat(claude-ops): consume lane restart-requests via scheduled headless reader#1720
kyle-sexton merged 8 commits into
mainfrom
feat/1653-restart-request-consumer

Conversation

@kyle-sexton

@kyle-sexton kyle-sexton commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

A loop lane that hits its per-session cycle budget or the /loop seven-day expiry writes a restart_request into its telemetry state block and stops cleanly — a running loop cannot relaunch itself, and nothing consumed that field, so every budget or expiry hit was a terminal manual-restart state. This PR ships the consumer: a deterministic bash reader (plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh, new consume-restarts action on /claude-ops:lanes) that reads each configured lane's telemetry and relaunches the stopped lanes that asked, meant to run unattended on an OS-owned schedule (Task Scheduler / cron).

Design record (three-option bake-off, doc citations, verified CLI surface): the decision comment on #1653. In short: a watchdog/wake lane is circular (itself a /loop bound by the same expiry and budget); a Stop hook cannot fire when the failed thing is the process or the machine (deferred with a trigger, as a latency layer only); only an OS scheduler survives a clean stop, a crash, AND a reboot. Cloud /schedule routines stay rejected (no access to local checkouts) and were not re-litigated.

Acceptance criteria

  • A written restart_request relaunches the lane without manual intervention — relaunch predicate: lane named in the operator's local config AND state block parses with non-null restart_request AND lane not currently running (claude agents --json, name + kind: background) AND circuit breaker has room (default 3 restarts / rolling 24 h). The not-running condition makes the predicate self-clearing without editing another writer's telemetry comment. The telemetry comment is a signal, never a target: nothing read from a comment is interpolated into a command, path, or repo.
  • The consumer is itself observable — JSONL run ledger under the plugin data dir; its own sentinel-marked telemetry comment posted by default to the issue morning-brief.sh resolves (that reader's own title search, reused), so a schedule that stops firing surfaces as a STALE lane in the morning brief; honest exit codes (a failed/unconfirmed relaunch or tripped breaker exits 5, visible in Task Scheduler history).
  • Restart respects the lane's launch shape and autonomy tier — by delegation to lane-launcher.sh restart <lane>, which carries the lane's prompt, --model, --effort, and --settings from the same config. claude respawn is deliberately not used: it resumes the conversation intact, and a restart-request exists precisely because a fresh context is the only reset a lane gets.

Operator registration (not performed by this PR)

print-schedule emits the exact schtasks /Create (poll + ONLOGON cold-start, /RU "%USERNAME%" /IT /RL LIMITED — no elevation, no stored password) and schtasks /Delete reversal lines with Windows-form paths, plus cron/launchd/systemd-user equivalents and a zero-model-cost offline form. Verify and Reversal lines follow the ClaudeCodeOtelPrune precedent, including its Windows-first posture (the non-Windows lines are equivalents, not a parity claim). No scheduled task is registered by this change.

Explicitly UNVERIFIED (labeled in context/restart-consumer.md)

  • Fully-logged-off operation (S4U /RU <user> /NP): nothing was registered on the authoring machine; documented coverage is "logged on or locked".
  • Whether a claude --bg lane launched from a scheduler-spawned process outlives that process on Windows (job-object process-tree kill): the consumer degrades safely instead of assuming — it re-polls claude agents --json after each relaunch and records a loud failed row (exit 5) if the lane never appears.

Not lane pacing

The consumer's polling tick is NOT lane cadence: lanes stay self-paced via ScheduleWakeup, and a tick where no configured lane has a non-null restart_request is a no-op that only refreshes the consumer's own freshness telemetry. Stated explicitly in SKILL.md and the context doc.

Verification

  • 95-case deterministic test suite (restart-consumer.test.sh), offline via --telemetry-json / --agents-json / --now injection; all green. Shellcheck clean on the script and the suite.
  • Gates green: check-changed-skills.sh origin/main, check-changelog-parity.sh --check and --check-bump origin/main, check-shell-portability.sh, check-silent-skips.sh, markdownlint.
  • Two independent fresh-context verification rounds; round 1 findings (scheduled command missing the run sub-action, MSYS-form /TR paths unusable from cmd.exe, morning-brief discovery mismatch) fixed and re-verified READY.

Four review findings fixed on the branch

Each was checked against the current code rather than the thread's outdated flag; two of the four were still live.

  • P1 — the target repo could not resolve from the checkout path. gh repo view takes an [<owner>/]<repo> argument and parses a leading path segment as a HOST, so passing the absolute checkout path made the default (no --target-repo) path — the one every generated scheduled command uses — exit 4 before reading any request. Verified empirically on this D: checkout: the positional-path form reports error connecting to d, while the same command run in the directory resolves the slug. The repo now resolves from the working directory.
  • P1 — a failed issue lookup read as a routine tick. The comment read already returned its own api-error, but resolve_issue_by_title piped gh issue list into jq, so an unreachable forge became empty output and recorded no-telemetry with no failure flag — an unattended consumer looked healthy while never observing that lane's request. It now returns non-zero and the caller records api-error, matching the comment-read path.
  • P2 — the circuit breaker could not open on the failure it exists for. It counted only restarted, while a launcher that exits non-zero or returns success without the lane appearing writes failed. Every tick re-attempted a pull, a marketplace refresh, and a launch indefinitely. It now counts relaunch attempts; error and api-error stay uncounted so a transient forge outage never spends a lane's budget.
  • P2 — the published telemetry comment carried the absolute ledger path, which embeds the operator's home-directory user name by default and can carry internal host names under --data-dir. It now names the file relative to whatever data dir the reader's own machine resolves.

Related

Closes #1653

🤖 Generated with Claude Code

kyle-sexton and others added 4 commits July 28, 2026 21:58
Deterministic reader of each configured lane's telemetry restart_request
that relaunches stopped lanes through lane-launcher.sh restart, with a
per-lane circuit breaker, a JSONL run ledger, its own morning-brief
telemetry comment, and print-schedule emitting the operator-run OS
scheduler registration (Task Scheduler / cron) with removal commands.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRSgwJ7RN4vPZt4oJAADHy
…context doc

valid_repo_slug's character class admits `.`, so `../evil` passed as an
owner/name slug and reached the gh API path — test 49 caught it; reject
dot-only segments explicitly. Adds the operator/reviewer-facing
context/restart-consumer.md (design rationale, OS-scheduler registration
with Verify/Reversal, labeled UNVERIFIED items, observability contract).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0134vdV1jXJhzpSmeT7y5B8N
Route the consume-restarts action through SKILL.md to
restart-consumer.sh (which now strips the leading skill-level action
token so $ARGUMENTS forwards verbatim, matching the lane-launcher
idiom), surface it in the plugin manifest description and README, and
record the 0.22.0 release note.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0134vdV1jXJhzpSmeT7y5B8N
…relaunch

Independent verification found the documented schedule inert: the
headless command omitted the `run` sub-action, so a registered task ran
the read-only default `check` forever and never relaunched a lane or
refreshed the consumer's telemetry. The /TR payload also embedded
MSYS-form paths cmd.exe cannot execute, and the backslash-continued
schtasks lines were pasteable into no Windows shell.

- carry `consume-restarts run` in every scheduled command (schtasks,
  cron, and the documented headless form)
- emit Windows-form /TR paths via cygpath -w (which also restores the
  real claude.exe behind the extensionless shim) and single-line
  schtasks commands, with an explicit run-from-cmd.exe note (Git Bash
  MSYS path conversion mangles /-style options)
- default the consumer's own telemetry comment onto the issue
  morning-brief.sh itself resolves (reusing that reader's title search)
  so the appears-in-the-brief claim holds without pinning; exact-title
  fallback and loud ledger-only degradation kept
- note the --data-dir ledger-split hazard on the offline variant

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0134vdV1jXJhzpSmeT7y5B8N
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2e27ec7cb7

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

Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh Outdated
Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh Outdated
Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh Outdated
Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh Outdated
kyle-sexton and others added 2 commits July 29, 2026 00:03
… closed

The emitted registration is TWO scheduled tasks — a 15-minute poll and an
ONLOGON companion — and at logon both fire. Task Scheduler's instance policy is
per task, so it cannot serialize them; lane-launcher.sh's running-lane guard is
same-process; and a just-launched background session does not appear in
`claude agents --json` instantly. Unsynchronized, both runs read the same
breaker count, both relaunch, and one lane name ends up with two `claude --bg`
sessions plus two `restarted` rows burning the budget for one effective restart.

A `run` now holds an mkdir-atomic sentinel across the whole read -> decide ->
relaunch -> append span, released on an EXIT trap — the idiom the observability
prune's `.prune-in-progress` established. A run that cannot take it skips
cleanly: exit 0, a `lock-held` flag, nothing launched and nothing written. A
lock left by a hard-killed run (no EXIT trap) ages out after an hour, so an
unattended schedule cannot wedge permanently; a lock with no stamp yet is dated
rather than stolen, because its holder may have won the mkdir microseconds ago.

The circuit breaker now fails CLOSED. `jq -s ... || printf '0'` meant one torn
line silently restored the full restart budget — on exactly the ledger an
unsynchronized or crashed writer leaves behind, turning corruption into an
unbounded restart loop. An unparseable ledger reports the budget as spent, with
a warning naming the file.

Two more honesty fixes in the same surface:

- A failed telemetry READ was swallowed into `[]` and surfaced as `no-state`, so
  a transient `gh` blip was indistinguishable from "the lane did not ask". It is
  now its own `api-error` decision, flagged and exit 5.
- `append_ledger` was gated only on `--dry-run`, so a `check` wrote to disk
  despite the `--help` contract documenting it read-only. It is gated on the
  action being `run`, and only INCIDENT decisions are ledgered: the routine
  per-tick ones would add hundreds of rows a day, forever, to the file the
  breaker re-reads once per lane per tick, and none can change its verdict.
  Filter-on-write, not prune-on-write — the breaker's memory should not depend
  on a whole-file rewrite going right.

`print-schedule` now substitutes a passed --data-dir into the offline form,
which previously emitted a placeholder while warning that a split data dir would
widen the restart budget.

Tests: 39 new cases (53 -> 92), including the lock's concurrency behavior driven
through the launcher stub, and argv-recording `gh` coverage for
`upsert_own_telemetry` — pinned issue, search hit, exact-title fallback, and no
issue found — which every prior case suppressed with --no-telemetry. A grep gate
asserts the morning-brief discovery literals stay byte-identical in both files.
19 of the new cases fail against the pre-fix script.

Refs #1653

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0134vdV1jXJhzpSmeT7y5B8N
… exists to catch

The documented Verify step told the operator to confirm "a fresh `last-cycle:`
on the consumer's telemetry comment (or a fresh row in the local run ledger)".
The disjunction's second branch was satisfied by the exact failure the step
exists to catch: `upsert_own_telemetry` returns early unless the action is
`run`, but `append_ledger` was gated only on `--dry-run` — so a `check`-only
schedule, the misregistration commit 2e27ec7 fixed, produced no fresh
`last-cycle:` and a fresh ledger row anyway, and passed.

Both halves are fixed. The ledger alternative is dropped from the step: it
answers "did something run", where the step must answer "did a `run` run", and
`last-cycle:` is the only signal that carries that. The read-only contradiction
is resolved in favour of the `--help` contract — `append_ledger` is gated on the
action being `run` — because a `check` an operator runs by hand must never move
the circuit breaker's memory.

Also documents the cross-process lock and why it is load-bearing rather than
defensive (the registration is two tasks that both fire at logon), the
fail-closed breaker, the incidents-only ledger, and `api-error` as distinct from
`no-state`. CATALOG regenerated for the lanes skill summary.

Refs #1653

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_0134vdV1jXJhzpSmeT7y5B8N
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand request
  • Read restart-consumer.sh (core script)
  • Read restart-consumer.test.sh (test suite)
  • Read restart-consumer.md (context doc)
  • Read SKILL.md changes
  • Review for correctness, security, and design alignment
  • Post findings

The implementation is architecturally sound and carefully designed — the security discipline (comment-is-a-signal, input through jq only, valid_repo_slug validation, no interpolation from comment bodies into commands) is thorough, and the 53-case test suite is unusually strong for a shell script. Three findings follow, in severity order.


Finding 1 — gh repo view "$REPO" passes a local path as a repo slug (P1, correctness)

File: plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh:407

TARGET_REPO="$(gh repo view "$REPO" --json nameWithOwner -q .nameWithOwner 2>/dev/null)" || TARGET_REPO=""

The gh repo view positional argument is documented as [HOST/]OWNER/REPO, not a filesystem path. Passing $REPO (an absolute path like /home/user/projects/foo) causes gh to reject it or misinterpret it, so TARGET_REPO stays empty and the consumer exits 4 on any unattended run that doesn't explicitly pass --target-repo.

Every test case in the suite passes --target-repo "owner/name" directly, so this code path is never exercised by the tests — the bug is masked.

Fix: run gh repo view from within the target directory so it auto-resolves the remote:

TARGET_REPO="$(cd "$REPO" && gh repo view --json nameWithOwner -q .nameWithOwner 2>/dev/null)" || TARGET_REPO=""

The gh CLI also accepts -C <dir> as an alternative. Either form matches what gh repo view (no positional arg) documents as its default-resolution behavior.

Impact: every production unattended run that relies on auto-resolution (i.e., doesn't have --target-repo baked into the scheduled command) exits 4 before reading any lanes.

Fix this →


Finding 2 — failed relaunches don't count toward the circuit breaker (P2, correctness)

File: plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh:651

jq -s -r --arg l "$lane" --argjson c "$cutoff" \
  '[ .[] | select(.lane == $l and .decision == "restarted" and (.epoch // 0) >= $c) ] | length' \
  "$ledger" 2>/dev/null

The breaker counts only decision == "restarted". The ledger does record failed rows (launcher exited non-zero, or lane never appeared in the session list), but those rows are invisible to the breaker. This means:

  • Lane has a non-null restart_request
  • Launcher fails consistently (wrong config, Windows job-object kill, etc.)
  • failed rows accumulate in the ledger — exit 5 is emitted each tick — but the breaker never opens
  • The consumer retries the launcher, the marketplace pull, and every associated side effect on every polling tick indefinitely

The PR description says this case is "loud and rate-limited by the circuit breaker" but the rate-limiting only applies to successful restarts. The documented unverified scenario on Windows (process-tree kill) is exactly the case that could trigger this unbounded retry.

Options:

  1. Count failed toward the budget alongside restarted in the select predicate.
  2. Track attempts separately from successes.

Option 1 is a one-word change and matches the intent of "at most N restart attempts per lane per window":

-'[ .[] | select(.lane == $l and .decision == "restarted" and (.epoch // 0) >= $c) ] | length'
+'[ .[] | select(.lane == $l and (.decision == "restarted" or .decision == "failed") and (.epoch // 0) >= $c) ] | length'

This also needs a test that seeds the ledger with failed rows and asserts the breaker opens — currently there is no such case.

Fix this →


Finding 3 — machine-local ledger path (including username) posted to a GitHub issue comment (P2, information exposure)

File: plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh:866

body+="
Run ledger on this machine: \`$(ledger_path)\`"

ledger_path expands to something like /home/alice/.claude/plugins/data/claude-ops/lanes/<hash>/restart-consumer.jsonl. The full absolute path — including the OS username embedded in the home directory — is then posted to whatever GitHub issue upsert_own_telemetry resolves, which by default is the issue that morning-brief.sh reads (potentially a shared team issue on the org's repo).

The comment also notes this is included in the review by the PR description as a concern about the local path. The issue may be private, but it's still unnecessary machine-specific data in a comment that exists specifically to surface status to teammates.

Fix: omit the path from the remote telemetry comment, or replace it with a generic indicator:

-body+="
-Run ledger on this machine: \`$(ledger_path)\`"
+body+="
+Run ledger: local machine only (not posted to this comment)"

Or emit the path only to info (stdout) so it appears in the local run's output/Task Scheduler history but not in the GitHub comment.


On the Codex P1-2 finding (telemetry read failure not propagated)

This appears to be a false positive. lane_comment_bodies returns 1 on gh api failure, and process_lane catches the non-zero return and records api-error rather than no-state. Test case 18 explicitly verifies this: GH_FAIL_COMMENTS=1 triggers api-error output and exit code 5. The reviewer may have been looking at an earlier version of the code.


Other observations (no action required)

  • Security posture is strong. The "comment is a signal, never a target" discipline is consistently enforced: lane names, prompts, models, and settings come exclusively from the operator config; nothing from comment bodies reaches a command line. The valid_repo_slug guard before any URL interpolation is correct. The sanitize helper for report strings is applied at the right boundary.
  • Lock design is correct. The mkdir-atomic sentinel, the stamp file to detect stale locks (abandoned by a hard-killed run), the "no stamp → don't steal" behavior, and the hour-long abandonment threshold are all well-reasoned and tested.
  • Breaker fails closed on corrupt ledger — the warning + budget-spent fallback is the right default.
  • Test case 1's assertion label says "bare-true request on a stopped lane is a request" but asserts | work | failed | (because the stub launcher doesn't update the agents fixture). The behavior is correct; the label is slightly misleading about what's being tested. Minor.
  • SKILL.md routing correctly strips the leading consume-restarts token before forwarding $ARGUMENTS, and the script's parse_args strips it itself too — the double-strip is intentional and harmless.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


Security Review — PR #1720

Reviewed restart-consumer.sh (new, 1001 lines), restart-consumer.test.sh, context/restart-consumer.md, and SKILL.md. The PR diff is otherwise version bumps and docs.


Core trust-boundary analysis

The PR's central security claim — "THE COMMENT IS A SIGNAL, NEVER A TARGET" — is accurate and well-enforced. Telemetry comment content is parsed exclusively through jq --arg / --argjson parameters, never interpolated into command strings or eval'd. Lane names, launch parameters, and repo slugs all originate from the operator's local config file, not from GitHub comments. The valid_repo_slug check correctly rejects traversal sequences. No command-injection path through untrusted comment content exists.

The cross-process lock is sound: mkdir atomicity is the correct arbiter; the stale-lock reclaim correctly loses a race on its own mkdir; the loser-stamps-with-now behaviour for an unstamped lock is the right conservative choice.


Findings

[IMPORTANT | CONFIRMED] Operator's home-directory path posted verbatim to GitHub

restart-consumer.sh L866:

body+="
Run ledger on this machine: \`$(ledger_path)\`"

ledger_path() expands to ${DATA_DIR_OVERRIDE:-${CLAUDE_PLUGIN_DATA:-$HOME/.claude/plugins/data/claude-ops}}/lanes/<sha>/restart-consumer.jsonl. Without an explicit --data-dir, the default embeds the operator's $HOME directory — which on every major OS encodes the system username — in the telemetry comment body that upsert_own_telemetry then POSTs to GitHub. On any repo with read access broader than a single operator (public repos, team repos, repos audited by third parties) this reveals the operator's local system username and internal data-directory structure.

The context doc already flags the ledger path as machine-local. The fix is to emit a generic label (e.g., "(local ledger — see this machine's CLAUDE_PLUGIN_DATA/lanes)") rather than the absolute path. If the path is genuinely useful for debugging it could be printed to stdout only, not embedded in the GitHub comment.

Fix this →


[IMPORTANT | CONFIRMED] Circuit breaker does not bound failed relaunch attempts

restart-consumer.sh L650–655:

jq -s -r --arg l "$lane" --argjson c "$cutoff" \
  '[ .[] | select(.lane == $l and .decision == "restarted" and (.epoch // 0) >= $c) ] | length' \
  "$ledger"

restarts_in_window counts only rows whose decision is "restarted". Rows with decision == "failed" — which cover both a non-zero launcher exit (L805) and the explicitly-unverified Windows job-object scenario where confirm_running never returns true (L814–818) — are ledgered but excluded from the count.

Consequence: when a lane's restart_request is non-null and the lane persistently fails to appear in the session list, the predicate fires on every scheduled tick (restart_request non-null ✓ + lane not running ✓ + breaker budget not exceeded ✓), the launcher is invoked, confirm_running polls for 10 seconds and returns false, the decision is recorded as failed, and the breaker budget remains untouched. The next tick repeats. The breaker never opens.

The PR itself acknowledges the Windows uncertainty ("UNVERIFIED") and says the consumer "degrades safely … records a loud failed row (exit 5), flagged in its telemetry." The exit-5 and the telemetry flag are correct, but the absence of breaker coverage means the "loud failure" repeats unboundedly at the scheduler's polling rate rather than stopping after the configured maximum.

An operator whose Windows task-object assumption does not hold, or a lane-launcher.sh that is systematically misconfigured, will generate a failed relaunch attempt every poll interval with no self-limiting. Including "failed" (and optionally "unconfirmed" if added) in the restarts_in_window count applies the breaker's protection symmetrically across all failure modes.

Fix this →


Non-findings worth recording

  • No command injection via telemetry content. Every value extracted from a telemetry comment reaches jq as --arg/--argjson data, never as filter text. The sanitize() function (strip CR/LF/pipe/backtick, cap at 200 chars) provides a second layer before user-visible output.
  • No path traversal in file operations. All file paths originate from operator-controlled config or the script's own $HOME/$CLAUDE_PLUGIN_DATA defaults; no component derived from telemetry is used in a filesystem path.
  • No credential or token exposure. gh and claude are invoked via their CLIs; no tokens are materialized or embedded.
  • valid_repo_slug is correct. It blocks traversal segments (..), option-looking values (requires at least one /), and arbitrary characters.

Merge: main landed claude-ops 0.22.1, so this branch's entry moves to
0.23.0 in both the changelog and the manifest, keeping the description
that documents the new consume-restarts action.

Fixes:

- gh repo view takes an [<owner>/]<repo> argument and parses a leading
  path segment as a HOST, so passing the checkout path made the default
  (no --target-repo) path exit 4 before reading any request. Verified
  empirically: `gh repo view <path>` reports "error connecting to d" for
  a D: checkout, while the same command run in the directory resolves
  the slug. The repo is now selected by the working directory.
- resolve_issue_by_title piped gh issue list into jq, so an unreachable
  forge read as "no issue carries this title" and an unattended consumer
  stayed apparently healthy while never observing that lane's request. It
  now returns non-zero on the list failure and the caller records
  api-error, matching what lane_comment_bodies already did for the
  comment read.
- The circuit breaker counted only `restarted`, leaving it permanently
  closed on the failure it exists for: a launcher that fails, or that
  returns success while the lane never appears, retried every tick
  forever. It now counts relaunch attempts (`restarted` and `failed`);
  the pre-launch `error`/`api-error` reads stay uncounted so a transient
  forge outage never spends a lane's budget.
- The published telemetry comment no longer carries the absolute ledger
  path, which embeds the operator's home-directory user name by default
  and can carry internal host names under --data-dir.

Tests: the breaker case that asserted the old counting rule now asserts
the new one, with added cases proving a `failed` row spends budget and
`error`/`api-error` rows do not. The argument-parsing case passes
--max-restarts 0 to isolate it from the shared ledger, which earlier
sections leave `failed` rows in. 95 cases, 0 failures; shellcheck clean.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and understand request
  • Read PR diff and current HEAD state
  • Verify which prior findings were fixed in f03b81f
  • Review restart-consumer.sh for remaining issues
  • Post findings

The merge commit (f03b81f) addresses all four findings raised across the two previous review passes (code review + security review + Codex). No new findings.

All four findings confirmed fixed

Finding 1 — gh repo view passed a path as a repo slug ✅ Fixed

restart-consumer.sh:413

# was: gh repo view "$REPO" --json nameWithOwner -q .nameWithOwner
TARGET_REPO="$(cd "$REPO" && gh repo view --json nameWithOwner -q .nameWithOwner 2>/dev/null)"

The fix correctly selects the repo by working directory rather than passing $REPO as a positional argument (which gh interprets as [HOST]/owner/name, making the leading path segment a host). The comment at the site explains the reasoning so the pattern is not re-introduced. Empirical verification is recorded in the review thread.


Finding 2 (Codex P1) — resolve_issue_by_title silently coerced API failures to "no issue" ✅ Fixed

restart-consumer.sh:607–612

resolve_issue_by_title() {
  local repo="$1" raw
  raw="$(gh issue list --repo "$repo" ... 2>/dev/null)" || return 1
  jq -r ... <<<"$raw" 2>/dev/null || return 1
}

Returns 1 — never empty — on gh failure. The comment explains the same invariant lane_comment_bodies already held. The process_lane caller at line 770 now checks the non-zero return and records api-error, consistent with how comment-read failures were already handled.


Finding 3 — circuit breaker counted only restarted, leaving it permanently open on persistent failures ✅ Fixed

restart-consumer.sh:677–682

jq -s -r --arg l "$lane" --argjson c "$cutoff" \
  '[ .[] | select(.lane == $l and (.decision == "restarted" or .decision == "failed") and ...) ] | length' \

The fix counts relaunch attempts (restarted + failed) but deliberately excludes error and api-error, since those are pre-launch read failures that must not spend a lane's restart budget on a transient forge outage. The asymmetry is explained in the function comment at line 656.

Two new test cases verify the new behavior: a failed row spends budget and trips the breaker (lines 227–237), and error/api-error rows do not (lines 241–250).


Finding 4 — machine-local ledger path (including username) posted to GitHub ✅ Fixed

restart-consumer.sh:896–903

# was: Run ledger on this machine: `$(ledger_path)`
body+="
Run ledger: \`<data-dir>/$(ledger_relpath)\` on the machine running this consumer."

ledger_relpath() (line 450) returns the repo-keyed subpath only (<sha>/restart-consumer.jsonl), not the absolute path. The comment explains why — default paths embed $HOME and --data-dir overrides can carry internal hostnames.


Test coverage

The suite grew from 53 to 95 cases. The additions directly target the fixed paths:

  • failed rows spending breaker budget (case 7, lines 227–237)
  • error/api-error rows NOT spending budget (lines 239–250)
  • --max-restarts 0 isolation fix for earlier sections leaving failed rows in the shared ledger
  • The morning-brief.sh discovery literals verbatim-match gate (case 20)
  • print-schedule offline-form substitution (case 21)

No remaining findings

The implementation is ready to merge.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


Security Review — PR #1720

  • Gather context and understand request
  • Fetch PR diff to identify changed files
  • Read restart-consumer.sh (main script, security-critical)
  • Read restart-consumer.test.sh (verify no test-only injection paths)
  • Read context/restart-consumer.md and SKILL.md
  • Analyze for logic flaws, injection surfaces, authorization gaps, credential exposure
  • Post security findings

Reviewed at HEAD f03b81f. All three previously identified findings are remediated in this commit. No new security issues found.


Prior findings — status at HEAD

Finding 1 (P1) — gh repo view "$REPO" passing a filesystem path as a slug

FIXED. L413:

TARGET_REPO="$(cd "$REPO" && gh repo view --json nameWithOwner -q .nameWithOwner 2>/dev/null)" || TARGET_REPO=""

The comment at L410–413 documents the failure mode (leading path segment is parsed as a HOST). The fix correctly uses the working-directory-resolution form.


Finding 2 (P2) — failed relaunches not counted toward the circuit breaker

FIXED. The function is renamed attempts_in_window and the jq filter at L678 now counts both "restarted" and "failed":

'[ .[] | select(.lane == $l and (.decision == "restarted" or .decision == "failed") and (.epoch // 0) >= $c) ] | length'

A new test sub-case (test file L225–237) seeds the ledger with a restarted+failed pair and asserts the breaker opens. The design rationale is documented at L656–664: error and api-error are deliberately excluded since they precede any launch and a transient forge outage must not spend a lane's restart budget.


Finding 3 (P2) — machine-local ledger path (including username) posted to GitHub

FIXED. L901–903:

body+="
Run ledger: \`<data-dir>/$(ledger_relpath)\` on the machine running this consumer."

ledger_relpath returns <sha-hash>/restart-consumer.jsonl (a git hash of the repo path, no username). The literal string <data-dir> is a template placeholder, not an expansion of $HOME. The comment at L897–901 documents the rationale.


Trust-boundary analysis (no new findings)

Telemetry content never reaches shell evaluation. The entire flow from GitHub comment body → extract_state_blockjq -c '.'jq -c '.restart_request' passes only through jq data channels (--arg / --argjson / stdin). Nothing from comment content reaches a command argument or eval. sanitize (strip CR/LF/pipe/backtick, cap 200 chars) provides a second layer before any user-visible output.

valid_repo_slug is applied at every trust boundary. At resolve_target_repo (L419), at process_lane when a lane config carries its own telemetry.repo (L757), and at CLI validation (L419–422). The check correctly blocks traversal segments and rejects option-looking values.

Session list failure fails closed. load_sessions (L564–569) exits 4 rather than defaulting to an empty list — an empty list would make every lane appear stopped and trigger relaunches for running lanes. confirm_running's tolerant re-read (L585 || true) uses the stale pre-relaunch snapshot on CLI failure; in that snapshot the newly-launched lane is absent, so the function returns false and the lane is recorded failed (spends budget, exits 5) rather than silently restarted. Correct.

Cross-process lock is safe under concurrent reclaim. The mkdir call at L500 inside the stale-lock reclaim path is still the atomic arbiter — two concurrent reclaimers both reach that mkdir and exactly one wins. The loser returns 1. No duplication escapes the mutex.

print-schedule output is documentation, not execution. The generated schtasks and cron lines go to stdout for the operator to review and paste; the script never executes them. Windows paths cannot contain ", so the \"$win_repo\" quoting in the schtasks /TR payload is safe. The claude_bin path passes through cygpath -w, which emits Windows-form paths free of shell metacharacters.

Lane names, prompts, models, and settings come exclusively from the operator config. Nothing from a telemetry comment reaches lane-launcher.sh's argv. The --lane array filter uses exact string comparison, no glob expansion.


Verdict

All three previously confirmed findings are fixed. The security posture is sound: no injection path through telemetry content, no credential exposure, no path-traversal surface, and the circuit breaker now correctly bounds all relaunch attempt modes.

@kyle-sexton
kyle-sexton merged commit 61d3a3b into main Jul 29, 2026
32 checks passed
@kyle-sexton
kyle-sexton deleted the feat/1653-restart-request-consumer branch July 29, 2026 19:23

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: f03b81fb66

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

Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh
Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh
Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh
Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh
Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh
Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh
kyle-sexton added a commit that referenced this pull request Jul 29, 2026
…hangelog order (#1758)

## Why

The per-cycle usage-sample invariant (loop-lane 6.0.0, shipped in #1719)
is self-contradictory as
written. §4 permits reading the previous sample back **"for exactly one
operation: subtracting its
`five_hour_pct`"**, then states **"No other read is permitted"** — but
the same invariant withholds
a delta when the window rolled over, and deciding that requires a
*second* comparison against that
previous reading. No lane can satisfy both clauses.

This is not theoretical. Both `babysit-loop` and `work-loop` were
rewritten to hold the readback
contract **by citation** ("Everything else — the single permitted
readback … — is the convention's")
while independently mandating the rollover comparison. The contradiction
therefore has exactly one
authoritative site, and every consuming lane inherits it.

`chatgpt-codex-connector` raised this on #1719. The fix was drafted but
never committed — the
authoring agent was interrupted mid-edit and #1719 merged without it.

## What changed

- **The permission is scoped by *purpose*, not by *operation*.** It now
covers deriving
`five_hour_delta_pct` — the subtraction **and** the rollover comparison
— as one derivation.
**The measure-only guarantee is unchanged**: the value still reaches no
decision, at any
  threshold, in a lane or in any gate a lane runs.
- **`at` disambiguated.** It is when the lane read the tee, not the
snapshot's own `captured_at`,
  which the staleness rule permits to lag it.
- **The delta's `null` condition widened.** "Either sample is missing"
excluded a present sample
carrying a `null` `five_hour_pct`; it is now `null` whenever either
side's `five_hour_pct` is
  unavailable.

### Changelog version regression (separate defect, same file)

`docs/conventions/loop-lane/CHANGELOG.md` on `main` read `6.0.0 → 3.1.1
→ 5.0.0 → 4.0.0 → 3.1.0`.
The `#1638` entry was authored against `3.1.0` and merged (#1686,
17:46:59Z) after `4.0.0` had
already landed (17:44:23Z) — a stale-branch renumber miss, in a file
with no CI gate for version
order.

Renumbered **`4.0.1`** and repositioned below `5.0.0`, which preserves
both descending version order
and the order entries actually shipped in. **Its wording is unchanged.**

Verified by script — all three touched changelogs are now strictly
descending with no duplicates.

## Deliberately not done

The `source-control` `0.39.0` and `work-items` `0.29.0` entries describe
the field as *"deliberately
inert: no lane behavior reads it back"*, which the shipped contract
contradicts. Those versions have
already been published, so they are **left as shipped** and superseded
by the new `0.40.2` / `0.30.2`
entries rather than rewritten in place.

## Verification

- `node scripts/validate-plugin-contracts.mjs` — 43 setup skills, 2150
files, pass
- `bash scripts/check-changed-skills.sh origin/main` — 2 skills, 0
failures
  (`babysit-loop` 495/500, `work-loop` 434/500)
- `npx markdownlint-cli2` over all 6 changed markdown files — 0 errors.
Run standalone because
`check-changed-skills.sh:67` sets `CHECK_SKILL_SKIP_MARKDOWNLINT=1` by
design (documented at
  line 19; markdown is gated by the hygiene lane).
- Changelog ordering verified by script against `sort -rV`.

## Related

No linked issue — this corrects defects in already-merged work; both
originating issues are closed.

Refs #1651 (the usage-sample invariant this corrects, shipped via #1719)
Refs #1638 (the changelog entry renumbered here, shipped via #1686)
Refs #1720 (its post-merge review findings are tracked separately, not
in this PR)

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Jul 30, 2026
… merged (#1760)

## Why

`chatgpt-codex-connector` posted six findings on #1720 at `19:24:03Z` —
**46 seconds after it
merged** at `19:23:17Z`. No ruleset gate could hold them (the threads
did not exist at merge time),
and nothing surfaces open threads on a merged PR, so they were never
triaged. All six are real, all
are in `lanes/scripts/restart-consumer.sh`, and none was addressed on
`main`.

## What changed

**Two that could silently disable an unattended consumer**

- **A broken lock store read as a held lock.** An ignored `mkdir`
failure fell through to the
contention branch: the absent stamp read as zero and the run reported
`lock-held` with exit 0. A
mistyped data dir, a permissions problem, or an unavailable volume meant
**Task Scheduler recorded
healthy ticks forever while no lane was ever processed**. `acquire_lock`
now returns a third state
  — store unusable (exit 4, loud) vs. race lost (exit 0, routine).
- **An unwritable ledger was a warning.** The breaker counts attempts by
querying that ledger, so an
unrecordable attempt was invisible to `--max-restarts` and a failing
launcher was retried on every
tick forever. Writability is proved *before* the relaunch, and a failed
append fails the lane.

**Two concurrency races**

- **A lock reclaimed on age alone.** A legitimate run outliving the
one-hour bound had its live lock
removed, letting a second run enter the relaunch span — reachable
because `lane-launcher.sh` does
an unbounded `git pull --ff-only` and marketplace update before launch.
The holder now records a
  PID; age only decides *when to ask*, liveness decides the outcome.
- **The relaunch predicate read a stale snapshot.** The session list
loads once per run, so a lane
started since by a concurrent operator invocation still read as stopped
— and `lane-launcher.sh
restart` **stops** a running lane before relaunching. A healthy session
could be interrupted
despite the documented "not currently running" predicate. Now rechecked
against a fresh list
  immediately before mutating.

**Two correctness gaps**

- **Offline telemetry parse failures were swallowed** by an
unconditional `return 0`, so an
unreadable or malformed `--telemetry-json` reported `no-state` —
indistinguishable from "the lane
did not ask". The offline branch now carries the contract the network
read already had.
- **`print-schedule` dropped behavior-affecting options.** A non-default
`--config`/`--target-repo`
was missing from all four emitted forms (schtasks, logon, cron,
offline), so the registered task
would silently drive a different lane configuration and a different
telemetry repository than the
  command that generated it.

## Also fixed while here

Bash reports a failed redirection *itself*, before the command runs, so
`2>/dev/null` on `printf`
never suppressed it — `Is a directory` was leaking into the operator's
report. The redirections now
run in a subshell.

## Verification

- `restart-consumer.test.sh`: **95 → 126 cases, 0 failures.** Every
finding gets a regression case,
each paired with a negative case so the fix cannot over-correct — a
*dead* owner's lock is still
reclaimed, a *defaulted* option is still omitted, and the report must
not contain the leaked
  redirection error.
- `shellcheck -x` on both scripts — clean.
- `node scripts/validate-plugin-contracts.mjs` — 43 setup skills, 2150
files, pass.
- `npx markdownlint-cli2` on all three changed markdown files — 0
errors.

## Docs

`context/restart-consumer.md` claimed a lock "ages out after an hour",
which is no longer the whole
rule — updated to state that age never reclaims on its own. `SKILL.md`'s
`argument-hint` gained
`--target-repo`, which `print-schedule` now emits and the hint omitted.

## Related

Closes #1759
Refs #1720
Refs #1653

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Jul 30, 2026
…uests (#1781)

## Why

A review that lands **after** a merge has nowhere to go:

- the ruleset's `required_review_thread_resolution` is a **merge-time
predicate** that already passed;
- the babysit lane works *open* PRs, and a merged PR leaves that queue;
- nothing on a merged PR surfaces its open threads — GitHub shows the
merge, not the findings.

Six findings — one **P1** — posted **46 seconds after #1720 merged** and
sat unread for a day. They
surfaced only because a later session happened to audit that merge
batch. Nothing was bypassed; the
gate was satisfied *because the threads did not yet exist*.

The morning brief is the right home: read-only, unattended, and already
where attention signals land.

## What it does

Compares each unresolved thread's **first-comment timestamp** against
the PR's `mergedAt`, and
reports only threads the gate could never have seen. A thread that
predates the merge was visible to
the gate — that is an ordinary unresolved thread, not this failure mode,
and it stays out.

- **One line per PR, at that PR's worst severity, with a finding
count.** Several findings on one PR
are one thing to go look at; repeating the title per thread buries every
other PR. Collapsing on
the *worst* severity means a P0 sitting beside advisory findings can
never be softened.
- **Severity survives to the operator** — a stranded P1 must not read
like a P3.
- **`--stranded-days`** (default 3) — wide enough to cover slow bot
review *and* an operator-absent
  weekend.

## It fails loud, not clear

A GraphQL error document is well-formed JSON that simply carries no
`data`. The extraction would
yield an empty list and render **"every merged PR in the window is
clear"** — an all-clear asserted
from an answer never received, which is the same fail-open shape this
section exists to catch.

This is not hypothetical: a rate-limit error did exactly that during
development. An API error now
says explicitly that it is *not* an all-clear, and prints the message.
Covered by a regression case.

## This is a standing leak, not a one-off

Its **first live run** against this repository immediately surfaced four
more stranded findings on
other merged PRs — including a **P1 on #1694** (merged `05:04:45Z`,
finding posted `05:05:20Z`, 35
seconds later) recording that a shipped `autonomy` cell **never reached
installations**.

## Verification

- `morning-brief.test.sh`: **30 → 63 cases, 0 failures.**
- The **negative** cases carry the weight — a pre-merge thread, an
already-resolved post-merge
thread, and a merge outside the window must all stay silent, or the
section is noise rather than
signal. Plus: collapse-does-not-soften-severity, highest-severity-first,
window-widening, and the
  API-error case above.
- The fixture mirrors the real #1720 shape, including the 46-second gap.
- `shellcheck -x` on script and test — clean. One `SC2016` is declared,
not blanket-suppressed: the
`$owner`/`$name`/`$endCursor` in the GraphQL query are server-side
variables bound by `-F` and
  **must** reach the server unexpanded.
- `node scripts/validate-plugin-contracts.mjs` — 43 setup skills, 2153
files, pass.
- `npx markdownlint-cli2` on both changed markdown files — 0 errors.

### Live run — posted in full in the comments below

A live run on the current branch found **44 merged PRs carrying
post-merge findings in a five-day
window: 0 P0, 10 P1, 34 P2.** Among the P1s: **#1503**, a
guardrail-bypass fix whose own review
landed unread, and **#1322** with 5 findings.

Read the **second** comment for the authoritative figures — the first
was produced by the
pre-review severity logic and reported a false P0, which review then
caught. No truncation warning
fired, so the read is complete.

The five-day window filter was spot-checked against `mergedAt` (a PR
numbered #969 in a 5-day window
looks wrong until you check: it merged `2026-07-25`, 4.2 days before the
run).

This is a far larger leak than the six findings that exposed it.

## Related

Closes #1777
Refs #1720
Refs #1759

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Consume lane restart-requests via scheduled headless reader

1 participant