Skip to content

fix(lanes): key telemetry state per lane instance, not per lane type - #1841

Merged
kyle-sexton merged 9 commits into
mainfrom
fix/1295-lane-instance-telemetry-partition
Jul 31, 2026
Merged

fix(lanes): key telemetry state per lane instance, not per lane type#1841
kyle-sexton merged 9 commits into
mainfrom
fix/1295-lane-instance-telemetry-partition

Conversation

@kyle-sexton

Copy link
Copy Markdown
Contributor

Fixes #1295

Implements the operator decision recorded on the issue: concurrent multi-lane operation against one
repository is supported, and writer identity is partitioned in the sentinel marker, not in the
issue title
.

The defect

Every lane built its telemetry sentinel from a fixed marker naming the lane type
(MARKER="work-items:work-loop"). Two instances of one lane on one repository therefore resolved
the same comment on the same telemetry issue and overwrote it last-writer-wins. The creation-race
reconcile already in the upsert does not help — it converges duplicate comments, not conflicting
state written by two live lanes, and the issue thread carries a timestamped live reproduction of
exactly that post-creation overwrite race.

item_cap, clean_streak, and rate_limit_latch silently stopped reflecting either lane's
experience. first_drain_complete is the serious one: it ends the first-drain C3 ratification gate,
so one machine finishing a drain ended the earn-trust period for every machine — a safety property
failing open with no human ratification.

The fix

The marker gains a lane-instance suffix, the lane-type marker becoming its prefix
(work-items:work-loop@<instance>). Each instance creates, reads, and edits exactly one comment no
sibling can match, so every counter in the block becomes per-instance — the only semantically
correct reading, since each measures that lane's observed experience.

The id is ${user_config.lane_instance} (the repo's sanctioned skill-body channel), defaulting to
the sanitized lowercased hostname, validated ^[a-z0-9][a-z0-9-]{0,31}$ inside each lane's own
executable block
and rejected rather than sanitized-and-continued — it is operator-supplied text
interpolated into a shell string and a jq program, and a guard that lives only in prose does not
run. The hostname fallback is a default, not a sanitizer: it passes the same gate.

Partitioning is correct only while ids are distinct, so collision is detected rather than assumed.
The state block gains lane_instance, a per-session writer_nonce, a per-cycle heartbeat_at, and
paused_until. A differing nonce over a stale block is the ordinary restart adoption; over a
fresh block it means another live lane holds the id — the lane writes nothing, escalates, and
stops cleanly. The check runs before any write, so a duplicate id degrades to a stopped lane rather
than a clobbered first_drain_complete. The 2h staleness window is twice the one-hour
ScheduleWakeup ceiling, so maximum idle backoff never reads as death; paused_until keeps a
rate-limit pause from doing so either.

Acceptance criteria

  • AC1 — two lanes maintain separate durable state. Each instance's lookup matches only its own
    full sentinel.
  • AC2 — first_drain_complete cannot be set by another lane's progress. It lives in a block no
    other instance can write. Earn-trust is re-earned per instance; item-level ratifications still
    travel with the item, so only the blanket period-end flag resets.
  • AC3 — the exclusion contract is untouched. The Lane telemetry: <lane> title does not
    change at all
    — that is why the marker was chosen as the seam over the title. Mechanically
    checkable: git diff origin/main | grep 'Lane telemetry' returns only prose about the contract,
    never a changed title literal. The drain-exit snapshot, the intake sweep, and the attention view
    all still match.

Two consumers moved with it

  • telemetry-upsert.sh admits @ in the marker charset and in both fallback lookaround classes
    — without the latter, lane:x matches inside lane:x@laptop-a and would adopt that instance's
    comment, the same superstring rule the existing lane:triage / lane:triage-old guard enforces.
  • restart-consumer.sh matched a bound telemetry.marker by exact equality. Once lanes carry
    the suffix, no bound lane's comment would match — the consumer would report no-state forever and
    restart nothing, the worst failure shape for an unattended relaunch trigger, since a silent no-op
    is indistinguishable from a healthy quiet tick. A bound marker now names a lane type across every
    instance, with a new optional telemetry.instance key to pin one; the scan also no longer lets a
    quiet sibling instance mask a later instance's live restart request.

Migration

A deliberate reset. No pre-existing comment matches an instance's new sentinel — neither the legacy
un-suffixed markers nor the improvised work-items:telemetry lane=… instance=… comments some lanes
began posting in practice — so the first cycle posts a fresh block from defaults, including
first_drain_complete:false. That fails closed and is intended; it produces one burst of
ratification queue comments on the next drain. A lane never adopts, edits, or tombstones the legacy
comment: its marker names no writer, so no instance can prove it owns it, and adopting it would
reintroduce the very clobber this removes. Retiring it is an operator action.

claude-ops:morning-brief keeps working, cosmetically degraded — N instances render as N rows all
labelled with the same lane. The instance is emitted on its own instance: line (that reader's
lane: capture is [a-z0-9_-]+ and would truncate a suffix appended there). Rendering per-instance
rows is a separate cosmetic follow-up in claude-ops.

Verification

  • telemetry-upsert.test.sh — 92 cases pass, including three new ones: a lane-type marker must not
    adopt an instance's comment, an instance marker must not adopt a longer instance marker, and a
    suffixed marker validates at all.
  • restart-consumer.test.sh — 141 cases pass, including three new ones: a bare bound marker matches
    an instance-suffixed comment, a superstring lane type is not adopted, and a quiet sibling instance
    does not mask a later request.
  • check-changed-skills.sh, check-changelog-parity.sh --check / --check-order, markdownlint,
    shellcheck, bash -n — all clean.

babysit-loop/SKILL.md was already 495/500 lines on main, so its collision-check branches went to
reference/telemetry-upsert.md, which already owns every other mechanic of the same upsert.

Related

Overlap note for concurrent branches. Sibling work is in flight on work-items triage (#1739)
and source-control babysit skills (#1660 / #1641 / #1525). This branch is based on origin/main
only. It touches plugins/source-control/skills/babysit-loop/SKILL.md (telemetry section, ~line
359) and its reference/telemetry-upsert.md, plus plugins/work-items/skills/attend-queue/SKILL.md
(telemetry section) — a babysit-loop branch editing the telemetry section, or any branch touching
the three plugin manifests' userConfig blocks or CHANGELOGs, will conflict textually.

Do not merge without review

This is a C4 structural change to a cross-plugin convention contract (loop-lane convention
7.0.0), so it stays human-merge unconditionally.

kyle-sexton and others added 4 commits July 30, 2026 17:35
The telemetry sentinel marker was a fixed constant naming the lane TYPE, so
every concurrent instance of a lane resolved the same comment on the same
telemetry issue and overwrote it last-writer-wins. The creation-race reconcile
already in the upsert did not help: it converges duplicate COMMENTS, not
conflicting STATE written by two live lanes.

The serious loss was first_drain_complete. It ends the first-drain C3
ratification gate, so one machine finishing a drain ended the earn-trust period
for every other machine, widening autonomy with no human ratification.

The marker now carries a lane-instance suffix, the lane-type marker becoming its
prefix. The `Lane telemetry: <lane>` issue title is deliberately untouched,
which is why the marker was chosen as the seam: the drain-exit snapshot, the
intake sweep, and the attention view all match lane infrastructure by that title
contract, and a prose-matching miss turns lane infrastructure into backlog.

Partitioning is only correct while instance ids are distinct, so collision is
detected rather than assumed: the state block gains lane_instance, a per-session
writer_nonce, a per-cycle heartbeat_at, and paused_until. A differing nonce over
a stale block is the ordinary restart adoption; over a fresh block it means
another live lane holds the id, and the lane writes nothing, escalates, and
stops. Detection runs before any write, so a duplicate id degrades to a stopped
lane rather than a clobbered first_drain_complete.

Two consumers move with it. telemetry-upsert.sh admits @ in the marker charset
and in both fallback lookaround classes, so lane:x never adopts lane:x@laptop-a.
restart-consumer.sh matched a bound marker by exact equality and would have gone
silently blind once lanes carried the suffix -- reporting no-state forever and
restarting nothing; a bound marker now names a lane type across every instance,
with an optional instance key to pin one, and the scan no longer lets a quiet
sibling instance mask a later instance's live restart request.

Fixes #1295

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

Its per-lane telemetry.marker binding matched a comment by exact marker
equality. Once lanes carry the writer-identity suffix, no bound lane's comment
matches -- the consumer reports no-state forever and restarts nothing, the worst
failure shape for an unattended relaunch trigger, since a silent no-op looks
identical to a healthy quiet tick.

A bound marker now names a lane TYPE and matches every writer instance of it,
with the same trailing boundary that keeps work-items:work-loop from adopting
work-items:work-loop-v2. A new optional telemetry.instance key pins one
instance, as does writing the suffix into marker itself.

The scan also no longer stops at the first matching comment when that comment is
not asking: with several instances writing to one issue, a quiet sibling
appearing first would mask a later instance's live restart request. Stopping
early was safe only while one comment per lane existed.

Fixes #1295

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

The instance resolution read a bare ${LANE_INSTANCE} env var, which the harness
never sets for a skill body -- so every lane would have silently taken the
hostname fallback and no operator-set lane_instance would ever have reached the
marker. Two lanes on one machine, the case the collision detector exists for,
would then share a hostname-derived id and escalate on their first cycle.

Skill bodies read config through ${user_config.KEY} substitution (hook-config
delivery convention, channel E), the same channel work-loop already uses for its
item-cap bounds; a surviving literal placeholder means the key is unset. The
hostname fallback stays a default rather than a sanitizer: it passes through the
same charset gate, so a hostname that cannot yield a conforming id stops the
lane instead of producing a marker nobody chose.

Also states what a lane does with the legacy un-suffixed comment: nothing. Its
marker names no writer, so no instance can prove it owns it, and a lane that
adopted it would reintroduce the shared-comment clobber. Retiring it is an
operator action; until then it reads as stale, which is honest.

Fixes #1295

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RhS3T7ShwJgKTrvk2Mvd3C
babysit-loop's SKILL.md was already 495/500 lines before this branch, so adding
the instance-collision branches inline pushed it to 519 and failed the hard cap
in check-changed-skills. The branches belong in reference/telemetry-upsert.md
regardless: that spoke already owns every other mechanic of the same upsert --
resolution, validation, lookup, POST/PATCH, race reconcile -- and splitting one
procedure across a hub and a spoke is what progressive disclosure exists to
avoid. SKILL.md keeps the state-block shape and what the fields are for; the
spoke keeps how the check runs.

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

@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: aa12122ef3

ℹ️ 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/work-items/skills/work-loop/SKILL.md Outdated
Comment thread plugins/claude-ops/skills/lanes/scripts/restart-consumer.sh Outdated
Comment thread plugins/work-items/skills/work-loop/SKILL.md
kyle-sexton and others added 3 commits July 30, 2026 23:27
Review findings on the lane-instance telemetry partition:

- Clean handoff: a replacement session no longer reads a stopped
  predecessor's fresh block as a live collision. A non-null
  `restart_request` is a stopping lane's last write, so the replacement
  adopts immediately, clears the request, and continues instead of
  waiting out the two-hour staleness window.
- First-cycle claim: an unclaimed marker is claimed with a cycle-0 block
  and re-read through the creation-race reconcile before any work, so two
  same-id sessions starting together stop before either overwrites the
  other's first durable state.
- restart-consumer local binding: an instance-suffixed comment under an
  unpinned binding is another machine's writer, so its request now
  reports as `unbound-instance` and relaunches nothing — consuming it
  would start unintended sibling instances on every stopped consumer
  sharing the issue. The pinned instance's comment and the legacy
  un-suffixed one stay actionable.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013dsTvnGqMJeLTZe4DpGGVQ
source-control re-bumps to 0.44.0: 0.43.0 shipped from #1838 while this
branch held the same number.
@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

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


Security review — PR #1841 (lane-instance telemetry partitioning)

  • Gather context (PR body, diff, changed files)
  • Fetch full diff against origin/main
  • Review shell scripts for injection risks (telemetry-upsert.sh, restart-consumer.sh)
  • Review SKILL.md executable blocks (work-loop, babysit-loop, attend-queue) for shell/jq injection via lane_instance
  • Review plugin.json / userConfig changes for scope/config risk
  • Post findings

Scope: only the 20 files changed in this PR (git diff origin/main...HEAD), reviewed at HEAD 9cab942. zizmor-covered classes (unpinned actions, workflow trigger/permission issues, template injection via the github context) are out of scope here and weren't specifically re-checked; this PR touches no .github/workflows/* file anyway.

IMPORTANT — restart-consumer.sh's unattended relaunch is silently inert for every unpinned lane binding after this migration

Confidence: high (verified by tracing the code path and cross-checking the new test suite)

restart-consumer.sh#L962-L1002

Once a lane's marker carries the new @<instance> suffix, lane_comment_marker_matches (line 721) still lets any suffixed comment through the coarse filter when marker is unset or has no @, but the classification that follows (line 979) treats every comment whose comment_marker contains @ as a sibling when the local binding has no @ in marker and no instance key is set:

if [[ "$comment_marker" == *@* && "$marker" != *@* && -z "$instance" ]]; then
  ...
  continue     # -> reported as unbound-instance, request stays "null"
fi

This isn't limited to genuine sibling collisions — it's true even when there is only one running instance of the lane and nobody else is competing for the id. After this PR ships, every lane's own telemetry comment carries the suffix (the PR explicitly resets all telemetry to the new per-instance scheme), and no existing lanes[].telemetry config has instance set (it's a brand-new key). So for the single-instance, zero-config-change case — almost certainly the majority of current deployments — restart-consumer.sh will report unbound-instance and relaunch nothing, forever, until an operator manually adds instance (or bakes @<instance> into marker) to every consumer's config.

This reproduces, for the unattended relaunch trigger itself, exactly the failure shape the PR calls out as the worst case for restart-consumer.sh pre-fix ("the consumer would report no-state forever and restart nothing... indistinguishable from a healthy quiet tick"): a monitoring/relaunch safety control fails silently closed with no loud signal, and record ... "unbound-instance" only surfaces in the ledger/report a human has to go read.

Confirmed against the new test coverage in restart-consumer.test.sh (§8b): every added case pairs an unpinned config with a suffixed comment and asserts unbound-instance or no-request — none exercises "single instance, unpinned config, own request" resulting in would-restart. Only the explicitly-pinned-config test (CONFIG_PIN, instance: "laptop-b" / marker with @laptop-b baked in) reaches would-restart.

Suggested fix: when a lane binding is unpinned and exactly one distinct instance suffix is observed among the sentinel-matching comments for that lane, treat it as the (trivially unambiguous) local writer and act on its request — reserve unbound-instance for the case where more than one distinct instance is actually competing. Alternatively, if requiring an explicit pin is intentional, this needs to ship as a breaking-change migration note (CHANGELOG / README) telling every existing consumer operator to add instance (or embed the suffix in marker) before or at upgrade time, since otherwise the auto-restart safety net goes dark with no error.

Fix this →


No injection or authorization issues found

Reviewed carefully and found sound:

  • lane_instance validation (work-loop/SKILL.md, attend-queue/SKILL.md, babysit-loop/reference/telemetry-upsert.md): the id is validated against ^[a-z0-9][a-z0-9-]{0,31}$ and rejected (not sanitized) before MARKER is built and before any shell/jq interpolation, closing off shell-metacharacter and jq-program injection via a hostile ${user_config.lane_instance} or a pathological hostname.
  • telemetry-upsert.sh marker-charset widening (@ added): all jq interpolation of $MARKER/$SENTINEL goes through --arg, never raw string concatenation into the jq program text, so the new charset doesn't introduce a jq-filter injection. The prefix/superstring lookaround guard is correctly extended to treat @ as a marker-class boundary character (verified against the new test cases), preventing lane:x from adopting lane:x@laptop-a's comment.
  • --repo traversal guard and --body-file containment (pre-existing, unchanged by this PR) still apply; nothing in this diff loosens them.
  • restart-consumer.shsanitize() (strips CR/LF/backtick/pipe, caps 200 chars) is applied to attacker/sibling-controlled sibling_instance text before it lands in the markdown report row, and all jq construction of ledger entries uses --arg/--argjson. No injection path found there.
  • userConfig.lane_instance additions to the three plugin.json manifests are plain string config with documentation warning operators to use an opaque id if they don't want a hostname published — no secret/PII handling issue.
  • No secrets, tokens, or credentials appear in the diff (checked docs/*/CHANGELOG.md, README.md additions too).
  • No .github/workflows/* files are touched by this PR.

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

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


Code review

  • Gather context (PR body, diff, review comments)
  • Review diff against origin/main
  • Check fixes already applied in d13a9f8 (clean-handoff + first-cycle claim) for correctness
  • Check test coverage claims
  • Check CLAUDE.md / repo convention alignment (fresh-docs mandate, plugin design rules)
  • Post final review

Scope: correctness, architecture, error handling, test coverage, maintainability. Security review is left to the dedicated security lane per REVIEW.md.

I verified that d13a9f8 correctly closes the two P1/P2 gaps chatgpt-codex-connector flagged (clean handoff on a fresh block with a non-null restart_request in work-loop/SKILL.md, and the first-cycle claim-before-work branch; restart-consumer.sh's unbound-instance report-only path traced through correctly). Tests added in restart-consumer.test.sh / telemetry-upsert.test.sh target the right edge cases (superstring lane types, sibling-instance masking order, marker-vs-instance pinning).

Finding: the convention doc (the canonical cross-plugin spec) wasn't updated with d13a9f8's protocol changes

docs/conventions/loop-lane/README.md §4 "Instance-collision detection" still documents only the original three-branch state machine (nonce matches / stale / fresh-collision). d13a9f8 added two more branches — clean handoff (fresh block, non-null restart_request → adopt immediately, no staleness wait) and first-cycle claim (unclaimed marker → claim with a cycle-0 block, re-read through the creation-race reconcile before any work) — to work-loop/SKILL.md and babysit-loop/reference/telemetry-upsert.md, but never touched the convention README or its own CHANGELOG.md 7.0.0 entry (confirmed: git show d13a9f8 --stat touches neither file; grep -c 'restart_request\|cycle-0\|clean handoff' docs/conventions/loop-lane/README.md → 0).

This matters because the PR frames this convention doc as the authoritative, versioned, cross-plugin contract ("C4 structural change to a cross-plugin convention contract," §"Tier ratified as major"). A future lane implementer who builds strictly from the convention doc — rather than diffing work-loop's inlined block — would reproduce exactly the P1 bug Codex caught here: a replacement session reading a stopped predecessor's fresh restart_request block as a live collision and refusing to hand off, plus the unmitigated first-cycle race on a newly claimed instance id. check-changelog-parity.sh wouldn't catch this since it's a cross-doc semantic gap, not a missing-entry-vs-version-bump mismatch.

Suggested fix: port the two extra branches (and their justification — "recording the request is a stopping lane's last write," "claiming first bounds the race to the claim itself") from work-loop/SKILL.md's "Instance-collision check" section into docs/conventions/loop-lane/README.md §4, and add a short 7.0.1 (or fold into a not-yet-released 7.0.0 if it hasn't shipped externally) CHANGELOG.md note.

Fix this →

Everything else checked out: the @ charset/lookaround extension in telemetry-upsert.sh is correct and covers the superstring case at both ends; restart-consumer.sh's lane_comment_marker_matches matching-rule table (no bound marker / bound-with-@ / bound-without-@) is internally consistent and matches its own docstring; attend-queue's explicit opt-out of the collision check (no durable-state block, operator present) is a reasonable, documented scope boundary; version bumps are minor/additive as expected for new userConfig keys.

I was not able to execute the shell test suites in this sandbox (bash <script>.test.sh requires approval that isn't available here), so the "92/141 cases pass" claims in the PR body are verified by static reading of the new test cases, not by running them.
· branch

The #1660 merge-conflict resolution round-tripped the file through
cp1252, double-encoding every non-ASCII character; the corruption
shipped to main in the #1838 squash. Restored by reversing the same
round-trip; the 0.44.0 section was authored clean and is untouched.

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

github-actions Bot commented Jul 31, 2026

Copy link
Copy Markdown

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

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

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


Security review — PR #1841 (lane-instance telemetry partitioning)

  • Gather context (PR body, prior review comments, diff)
  • Determine what changed since the last security review (commit 9cab942)
  • Re-verify injection/authorization surfaces at current HEAD
  • Post findings

Scope: the 20 files changed in this PR, re-reviewed at HEAD bad1bf9. This lane already posted a security review at 9cab942 (2026-07-31T04:14Z); this run re-verifies that review against the current head rather than duplicating it wholesale.

What changed since the last review: exactly one commit, bad1bf9, and it touches only plugins/source-control/CHANGELOG.md — repairing double-encoded em-dash characters (—) left over from a prior write. No executable code, no shell/jq interpolation, no config or manifest touched. (The other commit that appears between the two reviews, 74aa481, is upstream origin/main content absorbed via the 9cab942 merge, not new PR-owned work — it predates the prior review's scope.) I independently re-read telemetry-upsert.sh, the lane_instance validation blocks in all three consuming files, and the restart-consumer.sh matching/classification logic at current HEAD; nothing in the security-relevant surface has changed.

Still open — restart-consumer.sh's unattended relaunch goes silently inert for every unpinned lane binding

Severity: IMPORTANT · Confidence: high — unchanged from the prior review, still present at HEAD.

restart-consumer.sh#L971-L1004

process_lane classifies any instance-suffixed comment as an unactionable sibling (unbound-instance, line 1000) whenever the local binding has no @ in marker and no instance key set — regardless of whether more than one instance is actually competing. Since this PR resets every lane onto suffixed markers and no existing lanes[].telemetry config sets the new instance key, the single-instance/zero-config-change case — almost certainly the majority of current deployments — degrades restart-consumer.sh to reporting unbound-instance and relaunching nothing, forever, until an operator manually pins instance (or bakes @<instance> into marker) on every consumer. This reproduces, for the unattended relaunch trigger itself, the exact failure shape the PR calls out as worst-case pre-fix: a safety-relevant automation fails silently closed with no loud signal (the ledger-only unbound-instance record requires a human to go read it).

This is a logic/authorization-adjacent gap (a control that should require positive identity-matching before withholding action, ends up withholding action even in the unambiguous single-instance case) rather than an injection or credential issue, which is why it's carried forward here rather than deferred to the earlier review.

Not re-flagging as a new finding since the last review already surfaced it in detail with a suggested fix (auto-adopt the single distinct instance suffix when unambiguous, or ship explicit migration documentation) — that guidance stands unchanged.

No injection, authorization, or secret-handling issues found in the unchanged code

Re-confirmed by direct reading, not just diff inspection:

  • lane_instance validation (work-loop/SKILL.md#L84-L98, mirrored in attend-queue/SKILL.md and babysit-loop/reference/telemetry-upsert.md): the id is validated against ^[a-z0-9][a-z0-9-]{0,31}$ via an explicit reject-list case pattern ("" | -* | *[!a-z0-9-]*) and exit 1 on failure, before MARKER is built and before any shell/jq interpolation — a hostile ${user_config.lane_instance} or a pathological hostname cannot reach either surface. The hostname fallback passes through the identical gate rather than bypassing it.
  • telemetry-upsert.sh: all jq interpolation of $MARKER/$SENTINEL/$REPO goes through --arg/--argjson, never raw string concatenation into program text; the @-charset widening is correctly reflected in both lookaround boundary classes of the marker-fallback regex ((?<![A-Za-z0-9:@._-])…(?![A-Za-z0-9:@._-])), so lane:x cannot adopt lane:x@laptop-a's comment. --repo is validated as a single owner/repo pair (no .. traversal) before any URL interpolation, and --body-file containment (symlink-leaf rejection + canonicalized-parent prefix match) is unchanged.
  • plugin.json userConfig.lane_instance additions (work-items, source-control, claude-ops manifests): plain string config, documented as appearing verbatim in a public tracker comment with an explicit warning to use an opaque id — no secret/PII handling gap.
  • No secrets, tokens, or credentials appear anywhere in the diff (including the CHANGELOG/README additions).
  • No .github/workflows/* file is touched by this PR.

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

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


Code review — PR #1841 (re-check at HEAD bad1bf9)

  • Determine what changed since the last code review (posted at 9cab942)
  • Re-verify prior finding (convention doc not updated) still stands
  • Review the new commit (bad1bf9) for correctness issues
  • Post final review

Scope: re-verification against the prior code review posted at 9cab942, triggered here by the new commit bad1bf9. Security is out of scope for this lane (see the dedicated security-review comments on this PR).

What changed since the last code review: exactly one commit, bad1bf9, touching only plugins/source-control/CHANGELOG.md (460 lines changed, all encoding round-trips — no prose or semantic content altered). It repairs mojibake (— etc.) introduced by a cp1252 round-trip during the #1660 merge-conflict resolution that shipped to main in the #1838 squash and was pulled into this branch by the 9cab942 merge. Verified: grep -c 'â' over the file now returns 0, and the commit message itself documents the 0.44.0 section (authored clean) as untouched. This is a mechanical, correct fix with no code or test surface touched — nothing new to review there.

Still open — convention doc wasn't updated with d13a9f8's two new protocol branches

Confidence: high — unchanged from the prior review; re-confirmed at current HEAD.

docs/conventions/loop-lane/README.md#L464-L481 ("Instance-collision detection") still documents only the original three-branch state machine (nonce matches / stale / fresh-collision). d13a9f8 added two more branches to work-loop/SKILL.md and babysit-loop/reference/telemetry-upsert.mdclean handoff (fresh block with a non-null restart_request → adopt immediately, no staleness wait) and first-cycle claim (unclaimed marker → claim with a cycle-0 block, re-read through the creation-race reconcile before any work) — but the convention doc and its CHANGELOG.md 7.0.0 entry were never touched (grep -c 'restart_request\|cycle-0\|clean handoff' docs/conventions/loop-lane/README.md → 0, still).

This still matters for the reason given previously: the PR frames this doc as the authoritative, versioned, cross-plugin contract for a "C4 structural" convention bump. An implementer building strictly from the convention doc — rather than diffing work-loop's inlined block — would reproduce the exact P1 Codex caught (treating a stopped predecessor's fresh restart_request as a live collision) and the unmitigated first-cycle race.

Suggested fix (unchanged): port both branches, with their rationale, into docs/conventions/loop-lane/README.md §"Instance-collision detection", and add a short changelog note.

Fix this →

Nothing else new to raise — the code/test surface reviewed previously is untouched by bad1bf9.
· branch

work-loop and babysit-loop SKILL.md were at 506 and 500 lines against the
500 hard cap: tightened prose and reflowed ragged wraps with no semantic
loss (both at 499). Renamed the `ba` test fixture variable family the
spell gate flagged.

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

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

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


Security review — PR #1841 (re-check at HEAD 40175bb)

  • Determine what changed since the last security review (posted at bad1bf9)
  • Re-verify injection/authorization surfaces at current HEAD
  • Post findings

Scope: the 20 files changed in this PR, re-reviewed at HEAD 40175bb. This lane has already posted three security reviews on this PR (9cab942, and a re-verification at bad1bf9); this run checks only what changed since the last one rather than re-auditing the full surface.

What changed since the last review: exactly one commit, 40175bb, touching plugins/work-items/skills/work-loop/SKILL.md, plugins/source-control/skills/babysit-loop/SKILL.md, and plugins/claude-ops/skills/lanes/scripts/restart-consumer.test.sh. I diffed all three directly (not just read the commit message):

  • The two SKILL.md files are line-wrap/prose-tightening only — reflowing paragraphs to fit the 500-line skill cap and dropping one redundant clause ("inlined in this plugin because an installed plugin cannot invoke a sibling's scripts" → link only). No code block, no shell/jq snippet, no validation logic, no marker-construction logic, and no instance-collision branch changed. The INSTANCE/MARKER validation gate (work-loop/SKILL.md#L84-L104) is byte-identical in the fenced code blocks, only the surrounding prose reflowed.
  • restart-consumer.test.sh only renames the wq/wa/bq/ba fixture variable family to work_a/work_b/babysit_a/babysit_b (the spell-checker gate flagged ba/bq as typos). Same jq -n --arg construction, same fixture values, same assertions — no test behavior changed.

No new injection, authorization, or secret-handling surface was introduced by this commit.

Still open — restart-consumer.sh's unattended relaunch goes silently inert for every unpinned lane binding

Severity: IMPORTANT · Confidence: high — unchanged from the prior two reviews; the script itself was not touched by this commit.

restart-consumer.sh#L971-L1004

process_lane still classifies any instance-suffixed telemetry comment as an unactionable sibling (unbound-instance) whenever the local binding has no @ in marker and no instance key set, regardless of whether more than one instance is actually competing for the id. Since this PR resets every lane onto suffixed markers and no pre-existing lanes[].telemetry config sets the new instance key, the single-instance/zero-config-change case — almost certainly the majority of current deployments — degrades the unattended relaunch trigger to reporting unbound-instance and restarting nothing, forever, until an operator manually pins instance (or bakes @<instance> into marker) for every consumer. This is a logic/authorization-adjacent gap (a control withholds action even in the unambiguous single-instance case) rather than a fresh injection issue, which is why it's carried forward rather than filed as new. Not re-elaborating further since the original finding already has the full trace and a suggested fix (auto-adopt the single distinct instance suffix when unambiguous, or ship explicit migration documentation).

No new injection, authorization, or secret-handling issues found

Everything previously verified sound (marker validation ordering, jq --arg interpolation discipline in both telemetry-upsert.sh and restart-consumer.sh, the @-charset lookaround boundary fix, --repo/--body-file containment, and the userConfig.lane_instance plain-string/no-PII documentation) is untouched by this commit and remains sound at current HEAD.

@claude

claude Bot commented Jul 31, 2026

Copy link
Copy Markdown

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


Code review — PR #1841 (re-check at HEAD 40175bb)

  • Determine what changed since the last code review (posted at bad1bf9)
  • Diff the new commit (40175bb) directly against bad1bf9
  • Re-verify prior finding (convention doc not updated) still stands
  • Check the line-wrap reflow for unintended structural changes
  • Post final review

Scope: re-verification against the prior code review, triggered by the new commit 40175bb. Security is out of scope for this lane per REVIEW.md's split.

What changed since the last code review: exactly one commit, 40175bb — reflowing work-loop/SKILL.md and babysit-loop/SKILL.md to satisfy the 500-line skill cap, plus a fixture-variable rename (wq/wa/bq/bawork_a/work_b/babysit_a/babysit_b) in restart-consumer.test.sh to satisfy the typos gate. I diffed the commit directly (git diff bad1bf9 40175bb) rather than trusting the message.

Finding: the reflow merged ordered-list item 6 into item 5's paragraph, breaking the cycle-shape list

work-loop/SKILL.md#L308

Before this commit, step 6 of "## Cycle shape" started its own line:

   ...that directory out of the tree this lane runs its gates against.
6. **Report and pace.** Update the no-progress streak — ...

The line-wrap pass reflowed the paragraph above it and, in doing so, pulled 6. **Report and pace.** up onto the tail of item 5's last continuation line:

   that directory out of the tree this lane runs its gates against. 6. **Report and pace.** Update
   the no-progress streak — ...

6. no longer starts at column 0 of its own line — under CommonMark, an ordered-list marker must begin a line to start a new item; mid-line it's just literal text. The rendered doc now shows five numbered cycle steps, with step 6 ("Report and pace" — the telemetry upsert, exit-condition check, and ScheduleWakeup call) folded silently into step 5's ("Escalate") paragraph text instead of appearing as its own step. Confirmed structurally: grep -n '^6\. ' finds no match anywhere in the file, while ^5\. does.

This is exactly the kind of content change the commit message ("tightened prose and reflowed ragged wraps with no semantic loss") explicitly disclaims, so it's worth a second pass rather than being a deliberate edit. babysit-loop/SKILL.md's reflow, by contrast, only dropped a genuinely redundant clause and is fine.

Suggested fix: insert a blank line (or at minimum a hard line break) before 6. **Report and pace.** so it starts its own line again, then re-check the file still fits the 500-line cap.

Fix this →

Still open — convention doc wasn't updated with d13a9f8's two new protocol branches

Confidence: high — re-confirmed unchanged at current HEAD; untouched by 40175bb.

docs/conventions/loop-lane/README.md §"Instance-collision detection" still documents only the original three-branch state machine (nonce matches / stale / fresh-collision). d13a9f8 added clean handoff and first-cycle claim to work-loop/SKILL.md and babysit-loop/reference/telemetry-upsert.md, but the convention doc and its CHANGELOG.md 7.0.0 entry were never touched (grep -c 'restart_request\|cycle-0\|clean handoff' docs/conventions/loop-lane/README.md → 0, still). Not re-elaborating further — see the prior review for the full trace and suggested fix.

Nothing else new

The restart-consumer.test.sh fixture rename (wq/wa/bq/bawork_a/work_b/babysit_a/babysit_b) is mechanical and complete — verified no leftover references to the old short names anywhere in the file. babysit-loop/SKILL.md's one-line trim is a correct, harmless simplification.
· branch

@kyle-sexton
kyle-sexton merged commit c10f39b into main Jul 31, 2026
32 checks passed
@kyle-sexton
kyle-sexton deleted the fix/1295-lane-instance-telemetry-partition branch July 31, 2026 04:44
kyle-sexton added a commit that referenced this pull request Jul 31, 2026
#1841 keys the telemetry marker per lane instance and adds a lane_instance
validation gate ahead of it, in the same three inlined blocks this branch
rewrites. Both sides kept: main's INSTANCE validation and instance-qualified
MARKER, this branch's sentinel contract, payload floor, write-status check,
read-back, and gated supersede pass.

main also released claude-ops 0.25.0, source-control 0.44.0, and work-items
0.31.0, so this branch's entries move to 0.25.1, 0.44.1, and 0.31.1.

Re-ran the six-scenario harness against all three merged blocks: the happy
path supersedes duplicates silently; a failed PATCH, a degraded stored body,
a degraded create, a literal @path body, and a 15-byte payload each report
and none tombstone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Jul 31, 2026
…1850)

Fixes #943

## Summary

The three lanes that inline the `gh api` telemetry upsert —
`source-control:babysit-loop`,
`work-items:work-loop`, `work-items:attend-queue` — wrote `$BODY_FILE`
with no assertion that what
they composed was usable and no check that what they sent actually
landed. `claude-ops`'s
`telemetry-upsert.sh` refuses a degraded body and reads the comment
back; an installed plugin cannot
invoke a sibling plugin's script, so the three inlined copies inherited
none of that. The observed
failure was a telemetry comment carrying a literal
`@C:/…/telemetry_combined.txt` as its entire body
across three sessions — the lane reported nothing while its `updatedAt`
kept moving, so any consumer
keying on the comment's timestamp rather than its body read it as fresh.

## Fix

Each inlined block now carries three checks, because they catch
different failures.

**Pre-write gate.** Before any API call, a `$BODY_FILE` that is empty,
opens with a literal `@`, is
not sentinel-prefixed, or carries under 16 payload bytes below the
sentinel is refused and the cycle
skips the upsert fail-closed — no POST, no PATCH. The floor is measured
on everything below line 1,
so it matches the wrapper's `MIN_BODY_BYTES` byte-for-byte whether that
line ends in LF or CRLF. The
prefix assertion compares bytes rather than a whole first line, so a
CRLF body is not false-rejected.

**Write-status check.** The PATCH's own exit status is checked before
anything trusts the result. A
failed write leaves the previous cycle's body in place, which a
read-back running regardless would
accept — so the read-back alone cannot carry the guarantee.

**Post-write read-back.** The block then re-reads what the write stored
and reports the cycle
UNREPORTED unless that body still opens with the sentinel and clears the
same floor. This is the only
check that sees a write which reported success and stored something
else, and the one that would have
caught #943 itself: the composed file was correct and the defect was the
invocation (`-f body=@FILE`
transmits the literal path; these blocks only ever use `-F body=@`).

Every branch that ends without a verified body — including a degraded
create, which leaves no
sentinel-prefixed comment to re-read — reports and **skips the
duplicate-supersede pass**, so a cycle
whose own write is unproven never tombstones a racing session's comment.

Supporting corrections in the same change:

- The `$BODY_FILE` sentinel-first-line contract was implicit in a
trailing comment. A body composed
without it is not merely rejected — the lookup matches on that prefix,
so it would never be found
again and the next cycle would post a second comment. Now stated in
prose.
- The prose and CHANGELOGs claimed `morning-brief`'s freshness check is
what a degraded body
deceives. Verified against its `print_telemetry`: it parses
`lane:`/`last-cycle:` out of the body,
so a degraded comment makes the lane vanish from the report rather than
look healthy. Attributed
instead to any consumer keying on the comment's timestamp instead of its
body.
- The prose no longer justifies the read-back with an invocation the
block never emits, and now
states the limits it inherits rather than fixes: a PATCH that succeeds
while storing the previous
body still verifies, the read-back proves *some* well-formed telemetry
is present rather than
*this* cycle's, and every inline branch exits 0 and reports through
stderr alone.

Deliberately not replicated from the wrapper: the 64 KiB cap, the
body-file containment checks,
retries, and its distinct non-zero exit codes. Both surfaces now say
which guarantees travel with
which.

## Verification

The three blocks are byte-identical apart from their `MARKER` line
(checked mechanically, not by
eye). Each was extracted and driven against a stubbed `gh`, once per
lane:

| scenario | notice on stderr | duplicates superseded |
| --- | --- | --- |
| PATCH succeeds, stored body fresh | silent | yes |
| **PATCH fails, comment still holds the previous cycle** |
**UNREPORTED** | no |
| PATCH succeeds, stored body degraded | UNREPORTED | no |
| first-ever POST lands degraded, nothing to verify | UNREPORTED | no |
| body is a literal `@path` | refused, no API call | no |
| sentinel with a 15-byte payload | refused, no API call | no |

Row 2 is the one an independent review found silent before this change:
the write failed, the
read-back re-read a well-formed *previous* body, and the cycle reported
success over a stale comment.

The block was also driven under `set -e` with a failing create: it
reaches the "no comment available
to write to" notice and exits 0, rather than aborting before any notice.
That is what the bare POST's
`|| true` buys, and it is what keeps the "every branch reports" claim
true inside a strict-mode
wrapper.

`SENTINEL_OK` was also driven directly across the floor boundary:
sentinel-only, 15-byte and 16-byte
payloads, LF and CRLF, a non-sentinel 51-byte body, and a sentinel that
is not the first line. LF and
CRLF now flip at the same 15/16 boundary, which is what "matches
`MIN_BODY_BYTES`" requires;
`lane: from-stdin` is the same 16-byte string `telemetry-upsert.test.sh`
uses to pin that floor.

Repo gates, run locally against `origin/main` and green in CI on this
branch:

- `scripts/check-changed-skills.sh origin/main` — PASS (`plugin-gate`)
- `scripts/check-changelog-parity.sh --check` / `--check-bump` /
`--check-order` — PASS
- `markdownlint-cli2` over the changed markdown — 0 issues

Version bumps are re-derived on every merge from `main`, which released
all three plugins while this
branch was open. The entries currently sit at `claude-ops` 0.25.1,
`source-control` 0.44.1, and
`work-items` 0.31.1, each above the newest upstream entry in its own
CHANGELOG. If `main` releases
any of them again before this merges, the top entry needs renumbering
once more — the collision is in
the CHANGELOG heading and the manifest version, not in the change
itself.

This branch merges `main` rather than rebasing, so the feature commits
stay reachable and every push
is a fast-forward. #1841 landed mid-review and keys the marker per lane
instance in these same three
blocks; both sides are kept — its `INSTANCE` validation and
instance-qualified `MARKER`, this
branch's gate, write check, read-back, and gated supersede pass.

`work-loop`'s SKILL.md sat at 499 of its 500-line hard cap on `main`, so
these additions tripped the
skill-quality gate. Its upsert moves to `reference/telemetry-upsert.md`
— the same shape the sibling
`babysit-loop` lane already uses for the identical block — leaving
SKILL.md at 437 lines with the
telemetry home and durable-state contract, pointing at the spoke for the
mechanism. The block moves
verbatim; the scenario table above was re-run against the spoke.

## Follow-ups deliberately not taken here

- Every inline branch exits 0 and signals through stderr only; the
wrapper uses distinct non-zero
exits (3 gate, 5 API, 6 read-back). Giving the inline blocks
machine-readable outcomes is a
  lane-driver change, not a #943 fix.
- The three blocks are byte-identical with no drift gate.
`scripts/check-cross-plugin-source-drift.sh`
structurally cannot see this cluster — `skip_basenames` excludes
`SKILL.md`, and the third copy
sits at a different path-within-plugin. Registering the cluster is worth
its own change.

## Related

- Refs #502 — the observability fail-open this defect instantiates.
- Refs #1841 — keys the telemetry marker per lane instance in the same
blocks; merged in here.
- `plugins/claude-ops/skills/lanes/scripts/telemetry-upsert.sh` — the
wrapper whose gate and
read-back these inline blocks replicate, and whose limits they inherit.
- `docs/conventions/loop-lane/README.md` — the loop-lane convention
updated alongside.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

https://claude.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Jul 31, 2026
Two textual conflicts, both mechanical: main shipped work-items 0.31.0 and
0.31.1 while this branch held 0.30.4, so the manifest takes 0.31.2 and this
branch's entry is renumbered and stacked above main's shipped entries, which are
untouched.

Semantic check on the auto-merged side: main (#1841, #1850) moved work-loop's
telemetry upsert body into reference/telemetry-upsert.md and made the sentinel
marker lane-instance-suffixed. Both surfaces this branch cites still hold -- the
"Telemetry and durable loop state" heading still states that the telemetry home
is resolved from launch config before the default title, and the sentinel this
branch matches on is written with an ellipsis after `marker=`, so the instance
suffix does not falsify it.

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
kyle-sexton added a commit that referenced this pull request Aug 13, 2026
<!-- CURSOR_AGENT_PR_BODY_BEGIN -->
Fixes #1290

## Summary

Two concurrent attended sessions on one repository could both surface
and mutate the same row because `/work-items:attend-queue` had no claim
protocol while `/work-items:work` already used the seam assignee + lease
(`exit 7` → advance).

Documents the same seam claim protocol `work` uses, scoped to
attend-queue row disposition: claim before mutate, release before flip
to autonomous-eligible, session-start reclaim, binding routing. Bumps
`work-items` to **0.35.16**.

## Test plan

- [x] Eval #5 covers concurrent-session exit-7 skip behavior
- [x] Single-session run needs no new required argument

## Related

- #1290
- Unblocked by #1295 / #1841

<!-- CURSOR_AGENT_PR_BODY_END -->

<div><a
href="https://cursor.com/agents/bc-8279724f-c3b6-4e4f-be24-3dcf28782ae1?cursor_ref=pr_footer&cursor_cta=open_in_web"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-web-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-web-light.png"><img
alt="Open in Web" width="114" height="28"
src="https://cursor.com/assets/images/open-in-web-dark.png"></picture></a>&nbsp;<a
href="https://cursor.com/background-agent?bcId=bc-8279724f-c3b6-4e4f-be24-3dcf28782ae1&cursor_ref=pr_footer&cursor_cta=open_in_cursor"><picture><source
media="(prefers-color-scheme: dark)"
srcset="https://cursor.com/assets/images/open-in-cursor-dark.png"><source
media="(prefers-color-scheme: light)"
srcset="https://cursor.com/assets/images/open-in-cursor-light.png"><img
alt="Open in Cursor" width="131" height="28"
src="https://cursor.com/assets/images/open-in-cursor-dark.png"></picture></a>&nbsp;</div>

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.com>
kyle-sexton added a commit that referenced this pull request Aug 13, 2026
Fixes #1291

## Summary

Gives `/work-items:work-loop` a minimal invocation argument surface
mirroring babysit-loop where appropriate:

`[<owner/repo>] [--drain] [--shard <i>/<n>] [--ordering
oldest-first|newest-first] [--instance <id>] [--scope <label>]`

Rejects `--merge` / babysit tier / cap knobs fail-closed. Progressive
disclosure moves stop-mode exit semantics into
`reference/mode-standing.md` and `reference/mode-drain.md`.

Bumps `work-items` to **0.35.17** (0.35.16 reserved for #1290
attend-queue claim).

## Test plan

- [x] Evals 6–8 cover shard/ordering/rejected-merge, drain vs standing,
scope partitioning
- [x] Eval 3 updated to pass `--drain` where drain-terminal stop is
expected

## Related

- #1291
- Unblocked by #1295 / #1841

---------

Co-authored-by: Cursor Agent <cursoragent@cursor.com>
Co-authored-by: Kyle Sexton <kyle-sexton@users.noreply.github.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.

work-items(work-loop): fixed telemetry title and sentinel make two lanes on one repo clobber durable state, including first_drain_complete

1 participant