fix(loop-lane): gate and verify the inlined telemetry upsert's body

[kyle-sexton]

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 babysit lane telemetry edit posts literal @path body instead of file content (#502 observability fail-open) #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 ops: loop-lane telemetry — running per-lane status (babysit / triage / work) #502 — the observability fail-open this defect instantiates.
• Refs fix(lanes): key telemetry state per lane instance, not per lane type #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.ai/code/session_01FVoZoMYXqf8ZVbQYixPVPW

[claude]

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

---

Claude finished reviewing

Todo list:

• Gather context (PR body, changed files, CLAUDE.md/AGENTS.md rules)
• Fetch base and inspect diff (used merge-base ec7ed1b, the exact origin/main tip this branch merged — shallow clone blocked fetching the current tip, but no plugin-relevant commits landed on main in between)
• Review each inlined gate/read-back block for correctness
• Check consistency across the three lanes (source-control, work-items x2)
• Check CHANGELOG/plugin.json version bump consistency
• Post final review

Summary

The SENTINEL_OK/VERIFY logic is correct and byte-identical across all three inlined blocks (babysit-loop, work-loop, attend-queue). I hand-checked the boundary math: ${#SENT} + 17 total bytes = sentinel + 1-byte \n separator + 16-byte payload floor, which matches the wrapper's MIN_BODY_BYTES=16 measured on the payload alone — the verification table in the PR body checks out. The head -c ${#SENT} prefix comparison correctly sidesteps the CRLF pitfall a head -n1 approach would hit. Version bumps (claude-ops 0.24.4, source-control 0.42.4, work-items 0.30.4) are each sequential above their prior CHANGELOG entry with no duplicates, and match their plugin.json versions.

One correctness/completeness gap worth addressing:

"Record it in durable state" has no addressable target in two of the three lanes.
All three inlined blocks emit the identical failure message on a failed VERIFY:
...record it in durable state; do not trust the timestamp, and the convention doc states the same rule generally at docs/conventions/loop-lane/README.md#L434.

• attend-queue has no durable-state mechanism at all — its SKILL.md has a ## Telemetry section but never a ## Telemetry and durable loop state section or a JSON state block (unlike work-loop/babysit-loop, confirmed by grepping the file for any state schema). The telemetry comment itself is the only record this lane keeps, so when that same comment's write fails verification, "record it in durable state" has nowhere to go. See attend-queue/SKILL.md#L148.
• work-loop and babysit-loop each do carry a durable-state JSON block (work-items/loop-state@1 / source-control/babysit-loop-state@1, re-read every cycle start), but neither schema — unchanged by this PR — has a field for "previous cycle's telemetry write was UNREPORTED." See the schema at work-loop/SKILL.md#L149-L154 against the new instruction at work-loop/SKILL.md#L128-L130.

The PR's own rationale for this instruction is that "stderr does not survive the session," i.e., the whole point is to make an UNREPORTED cycle visible to the next cycle through something more durable than a log line. As written, an agent following this prose has no schema field to write that fact into, so the guidance is currently a well-intentioned no-op rather than a mechanism — which undercuts the observability goal this PR exists to close. Since this is a prose-only PR, the fix is likely either (a) add an unreported/last_telemetry_ok field to the two existing state schemas and give attend-queue an equivalent minimal state block, or (b) soften the instruction to something actually actionable today (e.g., "surface it prominently in the next cycle's report line") until the schema gains a field.

Everything else — the pre-write gate's fail-closed control flow, the $BODY_FILE sentinel-first-line contract now being explicit, the claude-ops lanes SKILL.md correction attributing the deceived check to updatedAt rather than morning-brief, and the CHANGELOG/version-bump bookkeeping — looks correct and consistent across all three lanes.

[claude]

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

---

Security Review

• Gather context (PR diff, changed files)
• Read all changed files in full
• Analyze for logic flaws, injection surfaces, auth gaps, secret handling, dangerous patterns
• Post findings

This PR is prose/documentation only (three skill/reference bodies with inlined gh api shell blocks, one convention doc, three CHANGELOGs, three plugin manifest version bumps) — no workflow files, no executable scripts, no dependency/action pins changed, so zizmor's advisory lane doesn't apply here.

I reviewed the three near-identical inlined telemetry-upsert shell blocks (plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md, plugins/work-items/skills/work-loop/SKILL.md, plugins/work-items/skills/attend-queue/SKILL.md) for injection, TOCTOU, and auth-boundary issues. $BODY_FILE is always passed via -F body=@"$BODY_FILE" (binary-safe field upload, never shell-evaluated), all shell variables are quoted, and $SENT/$MARKER are fixed per-lane literals, not attacker-influenced — no command/argument injection introduced by this diff.

One logic-flaw finding on the newly-added VERIFY post-write check:

IMPORTANT / PLAUSIBLE — VERIFY can't distinguish "PATCH failed to land" from "PATCH succeeded," undermining the write-landed guarantee this PR adds.

In all three inlined blocks (e.g. telemetry-upsert.md#L36-L38, work-loop/SKILL.md#L101-L103, attend-queue/SKILL.md#L146-L148):

gh api -X PATCH "repos/$REPO/issues/comments/$CANON" -F body=@"$BODY_FILE"
VERIFY "$CANON" ||
  echo "telemetry: comment $CANON does NOT carry a well-formed telemetry body after the write ..." >&2

The gh api -X PATCH write's own exit status is never checked — the script falls straight into VERIFY, which independently re-GETs the comment and checks SENTINEL_OK on whatever it reads back (VERIFY definition, e.g. telemetry-upsert.md#L19-L22). If the PATCH itself fails for reasons unrelated to body content — a secondary rate limit, a transient 5xx, an auth/permission change on the write path specifically — the comment is left holding its previous cycle's body, which (having itself been a valid prior write) still satisfies SENTINEL_OK. VERIFY then reports success, and the "does NOT carry a well-formed telemetry body" branch never fires, even though this cycle's telemetry never landed.

This is exactly the failure class the PR's own description calls out (Refs #502, "the observability fail-open this defect instantiates") and the PR's prose oversells VERIFY's coverage: "the post-write VERIFY re-reads what actually landed, which is what catches the failure the pre-write half structurally cannot" implies it catches any write failure, but it reliably only catches the specific -f-vs--F body-corruption case described in the PR. A failed write that leaves prior well-formed content in place is silently reported as verified-good. (Note: a read failure on the VERIFY GET itself — e.g. a 404 on a deleted comment — is caught correctly, since the read-back then returns empty content and fails SENTINEL_OK; the gap is specifically "write failed, but the comment still holds old valid content.")

Suggested fix: check the PATCH exit status directly (e.g. gh api -X PATCH ... -F body=@"$BODY_FILE" || echo "telemetry: PATCH failed; treat cycle as UNREPORTED" >&2) rather than relying solely on content re-verification to detect a failed write.

No other security issues found — no injection surfaces, no secret/token mishandling, and no authorization gaps introduced by this diff's changed content.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b805dc8011

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

[github-actions]

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

[claude]

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

---

Security Review

• Gather context (current PR diff, origin/main...HEAD at f090499)
• Read all changed files in full
• Analyze for logic flaws, injection surfaces, auth gaps, secret handling, dangerous patterns
• Post findings

This PR remains prose/documentation only (three inlined gh api telemetry-upsert blocks, one convention doc, three CHANGELOGs, three plugin manifest version bumps) — no workflows, no executable scripts, no dependency/action pins changed, so zizmor's advisory lane doesn't apply.

Since the last review (b805dc8), one new commit landed (35440a4, "report a degraded telemetry create, don't fall through silently"), adding an else branch so a degraded POST/create now reports UNREPORTED instead of silently skipping when $CANON is empty. I re-read this addition in all three lanes plus the unchanged parts — quoting, variable provenance, and control flow are consistent with the prior review.

IMPORTANT / PLAUSIBLE — the PATCH write's own exit status is still never checked; VERIFY alone cannot distinguish a failed write from a successful one. This is the same gap flagged in the prior review and by the Codex reviewer, and it was not addressed by the new commit — that commit only closed the analogous gap on the create/POST path. In all three lanes, the PATCH still runs unchecked immediately before VERIFY:

• telemetry-upsert.md#L36-L37
• work-loop/SKILL.md#L101-L102
• attend-queue/SKILL.md#L146-L147

gh api -X PATCH "repos/$REPO/issues/comments/$CANON" -F body=@"$BODY_FILE"
VERIFY "$CANON" ||
  echo "telemetry: comment $CANON does NOT carry a well-formed telemetry body after the write ..." >&2

If the PATCH itself fails for a reason unrelated to body content — a transient 5xx, a secondary rate limit, a write-scoped auth/permission change — the comment is left holding its previous cycle's (validly-formed) body, and SENTINEL_OK on that stale content still passes. VERIFY reports success even though this cycle's telemetry never landed, which is exactly the observability fail-open class this PR exists to close (refs #502). The fix suggested previously still applies: check the PATCH exit status directly, e.g. gh api -X PATCH ... -F body=@"$BODY_FILE" || echo "telemetry: PATCH failed; treat cycle as UNREPORTED" >&2, rather than relying solely on content re-verification.

No other issues found in the delta or the unchanged portions: $BODY_FILE is always passed via -F body=@ (binary-safe, never shell-evaluated), all variables are quoted, $SENT/$MARKER are fixed per-lane literals, and $CANON/$DUP are always numeric comment IDs sourced from jq .id — no injection surface. The new else-branch addition on the create path introduces no new risk (a fixed diagnostic string to stderr, no interpolation of untrusted data).

Fix this →

[claude]

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

---

Code Review

• Gather context (PR diff, REVIEW.md lane scope, CLAUDE.md)
• Read all changed files in full
• Review for correctness, design, conventions, error handling, observability, tests, documentation (security is out of scope here — owned by the security lane, which has already reported on the PATCH-exit-status gap)
• Post findings

Per this repo's REVIEW.md, this lane owns correctness/design/conventions/error-handling/observability/tests/docs and leaves security to the dedicated security lane — so the already-reported "PATCH exit status isn't checked before VERIFY" finding is intentionally not repeated here.

🔴 Important — unresolved merge-conflict markers committed into two CHANGELOGs

plugins/claude-ops/CHANGELOG.md and plugins/source-control/CHANGELOG.md each carry a literal, uncleaned git merge diff3 ancestor marker, committed at HEAD:

• plugins/claude-ops/CHANGELOG.md#L25 — ||||||| ec7ed1b3 followed by a stray blank line, sitting between the new ## [0.24.5] entry and ## [0.24.4].
• plugins/source-control/CHANGELOG.md#L29 — ||||||| 23521e3d, same pattern, between ## [0.43.1] and ## [0.43.0].

I traced this to merge commit 4f94322 ("Merge origin/main into fix/943-inlined-upsert-body-gate"): git show 4f94322 -- plugins/claude-ops/CHANGELOG.md shows the diff --cc conflict resolution kept the ||||||| base-ancestor line instead of removing it along with the rest of the conflict markup. plugins/work-items/CHANGELOG.md (also touched by this PR) is clean — this is specific to the two files whose conflict got mis-resolved.

This survived every stated verification gate because none of them parse changelog body content: check-changelog-parity.sh checks version headers/ordering/bump direction, not prose, and markdownlint-cli2 has no rule for a ||||||| text line. Fix: remove the ||||||| <sha> line and the blank line under it in both files.

Fix this →

🟡 Suggestion — "record it in durable state" still has no schema field to write into

Restating and sharpening a gap flagged in an earlier review pass on this PR, which remains unaddressed as of f090499: the work-loop block's failure message and its new CHANGELOG line (work-loop/SKILL.md#L134, work-items/CHANGELOG.md#L25-26) instruct: "record it in durable state so the next cycle sees it, since stderr does not survive the session." The work-items/loop-state@1 JSON block (work-loop/SKILL.md#L155) still has no field for this — no unreported/last_telemetry_ok key was added.

Beyond just missing a field, there's a circularity worth calling out: the skill states the telemetry comment itself is the sole durable-state store ("the comment, not the conversation, is the source of truth for these counters," line 151-152). If the write to that comment is exactly what just failed VERIFY, "record it in durable state" is asking the agent to persist a fact into the very artifact whose write is currently failing — with no described retry/second-write mechanism, an agent following this prose literally has nowhere to put the flag on a failed cycle. attend-queue has no durable-state block at all, so the same instruction there is unconditionally a no-op.

Since this is prose-only, either add the field (and describe writing it on the next successful cycle, since this cycle's write is what failed) or soften the instruction to something achievable today, e.g. surfacing it in the next cycle's human-readable report line instead of a structured field.

Everything else checks out: the SENTINEL_OK/VERIFY shell logic is byte-identical and correct across all three inlined lanes, the pre-write gate's fail-closed control flow is sound, the new else branch on the degraded-create path is consistent across lanes, and the version bumps (claude-ops 0.24.5, source-control 0.43.1, work-items 0.30.4) each match their plugin.json and are correctly ordered/non-duplicated in their CHANGELOGs (content pollution aside).

[claude]

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

---

Code Review

• Gather context (PR diff since last review, f090499 → 431a8e6)
• Read the new commit (8b1ed72, "check the telemetry write's exit status before trusting the read-back") in full
• Verify the PATCH-exit-status fix and the byte-floor rewrite for correctness
• Check CHANGELOG/prose consistency across all three lanes plus claude-ops
• Post findings

Per this repo's REVIEW.md, this lane owns correctness/design/conventions/error-handling/observability/tests/docs and leaves security to the dedicated security lane — the PATCH-exit-status gap flagged by that lane and by Codex is addressed by the new commit, so I'm not re-litigating it here beyond confirming the fix.

Since the last code-review pass (f090499), commit 8b1ed72 landed and:

• Checks the PATCH's own exit status (elif ! gh api -X PATCH ... >/dev/null; then) before falling into VERIFY, closing exactly the gap raised by both the security lane and Codex's review comment.
• Gates the duplicate-supersede loop behind a successful VERIFY, so a cycle with an unproven write no longer tombstones a racing session's comment.
• Rewrites the byte floor from ${#SENT} + 17 (assumes exactly one LF separator) to measuring everything below line 1 against 16 directly — this is correct and now agrees byte-for-byte with the wrapper's MIN_BODY_BYTES=16 on both LF and CRLF bodies, fixing the off-by-one the prior review's own verification table didn't catch.
• Removes the two leftover ||||||| diff3 markers flagged in the prior code-review pass — confirmed clean, no ||||||| anywhere in plugins/ or docs/ now.

I hand-verified the new control flow and byte math in all three lanes (babysit-loop's reference file, work-loop/SKILL.md, attend-queue/SKILL.md) — the shell blocks are still byte-identical across lanes, and the four-way branch (no comment / PATCH failed / VERIFY failed / success) is consistent everywhere.

🟡 Finding — claude-ops CHANGELOG's [0.25.1] entry now understates its own fix

plugins/claude-ops/CHANGELOG.md#L14-L17 still reads:

every lane that inlines the gh api upsert ... now carries both a pre-write body gate and a post-write read-back in its own block. The paragraph states which guarantees travel inline (both halves) ...

But the prose it's describing — plugins/claude-ops/skills/lanes/SKILL.md#L301-L312 — was rewritten by this same PR's last commit (8b1ed72) to describe three checks: "An inlined upsert now encodes three checks itself: a pre-write gate ..., a check of the write's own exit status ..., and a post-write read-back." The sibling source-control and work-items CHANGELOGs were correctly updated to say "three checks" in this same commit (source-control/CHANGELOG.md#L16, work-items/CHANGELOG.md#L15) — only claude-ops's entry was missed, since 8b1ed72's diff to that file only touched the adjacent morning-brief bullet, not this one.

This is exactly the class of drift check-changelog-parity.sh can't catch (it validates headers/ordering/bump direction, not prose accuracy), so it won't be caught by CI. Since claude-ops 0.25.1 is otherwise correctly bumped and ordered, the fix is a wording-only edit to lines 14-17: "both halves" → "all three checks", and list the exit-status check alongside the pre-write gate and read-back.

Fix this →

Restating (still open, lower priority) — "record it in durable state" still has no schema field

Flagged in two earlier passes on this PR, only partially softened by 8b1ed72: the prose now says "a lane with durable loop state records it there; a lane without one carries it in the cycle's own summary" (work-loop/SKILL.md#L128-L131), which is more honest than the earlier bare "record it in durable state" instruction. But no field (unreported/last_telemetry_ok) was actually added to the work-items/loop-state@1 or source-control/babysit-loop-state@1 JSON schemas, so an agent following "records it there" still has no key to write to. Not blocking given the wording is now at least accurate about what exists today, but worth a follow-up if #943's observability goal is meant to survive into the next cycle's durable state rather than just the current cycle's stderr/summary.

Everything else checks out: the SENTINEL_OK/VERIFY/exit-status control flow is correct and consistent across all three lanes, the byte-floor fix is sound on both LF and CRLF, and the version bumps (claude-ops 0.25.1, source-control 0.44.1, work-items 0.31.1) each match their plugin.json and are sequential/non-duplicated in their CHANGELOGs.

[claude]

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

---

Security Review

• Gather context (PR diff at HEAD 431a8e6, git diff origin/main...HEAD)
• Read all changed files in full
• Analyze for logic flaws, injection surfaces, auth gaps, secret handling, dangerous patterns
• Check whether the previously-flagged PATCH-exit-status gap was addressed
• Post findings

This PR remains prose/documentation only (three inlined gh api telemetry-upsert blocks, one convention doc, three CHANGELOGs, three plugin manifest version bumps) — no workflow files, no executable scripts, no dependency/action pins changed, so zizmor's advisory lane doesn't apply here.

The PATCH-exit-status gap flagged in the prior three review rounds (and by the Codex reviewer) is now fixed. All three inlined blocks — telemetry-upsert.md#L61-66, work-loop/SKILL.md#L128-133, attend-queue/SKILL.md#L160-165 — now check the PATCH exit status directly as its own elif branch, before falling into VERIFY:

elif ! gh api -X PATCH "repos/$REPO/issues/comments/$CANON" -F body=@"$BODY_FILE" >/dev/null; then
  echo "telemetry: the PATCH of comment $CANON failed - treat the lane as UNREPORTED ..." >&2
elif ! VERIFY "$CANON"; then
  echo "telemetry: comment $CANON does NOT carry a well-formed telemetry body after the write ..." >&2

This closes the exact class of failure identified: a PATCH that fails for reasons unrelated to body content (transient 5xx, rate limit, write-scoped auth change) now reports UNREPORTED immediately rather than falling through to VERIFY, which could pass on the comment's still-valid previous-cycle body. The prose in all three lanes plus the convention doc and the claude-ops lanes skill now correctly describe this as a distinct third check ("the write's own exit status is checked next"), and both files explicitly disclose the one limitation that's inherent rather than fixable at this layer — a PATCH that succeeds while re-storing the previous cycle's identical body still passes VERIFY, since there is no way to distinguish "no-op success" from "this cycle's write" from the outside. That's stated as a known, accepted limit rather than left implicit, which is the right call.

I re-checked the analogous create/POST path for the same class of gap: it doesn't check the POST exit status directly either, but a failed POST leaves no new comment behind, so the subsequent LOOKUP naturally returns empty, $CANON resolves empty, and the [ -z "$CANON" ] branch already reports UNREPORTED — so the create path doesn't have the PATCH path's problem (an existing target masking a failed overwrite) and needs no equivalent direct check.

No other issues found: $BODY_FILE is always passed via -F body=@ (binary-safe, never shell-evaluated), all variables are quoted, $SENT/$MARKER are fixed per-lane literals, $CANON/$DUP are always numeric comment IDs sourced from jq .id, and the duplicate-supersede -f body="Superseded duplicate ...: $CANON" interpolates only that trusted numeric id — no injection surface. The convention-doc and claude-ops lanes SKILL.md prose changes in this revision are documentation-only and introduce no new shell logic beyond what's reviewed above. Version bumps (claude-ops 0.25.1, source-control 0.44.1, work-items 0.31.1) are plugin.json-only edits with no security relevance.

[claude]

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

---

Code Review

• Gather context (PR diff since last code-review pass at 431a8e6)
• Read the new commit ddb50a5 in full
• Check correctness of the work-loop → reference-spoke extraction
• Check cross-lane consistency (byte-identical claim) still holds
• Check CHANGELOG/docs consistency
• Post findings

Per this repo's REVIEW.md, this lane owns correctness/design/conventions/error-handling/observability/tests/docs and leaves security to the dedicated security lane.

Since the last code-review pass, one new commit landed: ddb50a5 ("refactor(work-items): move work-loop's telemetry upsert to a reference spoke"). This is a mechanical extraction, not a behavior change — work-loop/SKILL.md was at 548 lines (over the 500-line skill-quality cap) after the #943 gate/write-check/read-back landed, so the upsert mechanism moves to a new reference/telemetry-upsert.md, mirroring how babysit-loop already splits the identical block into its own reference file.

I verified the extraction directly:

• Line count now clears the cap. work-loop/SKILL.md is 437 lines; the new reference file is 121 lines — matches the commit message's claim.
• The moved content is byte-identical to the claim. I diffed the shell block and its four surrounding prose paragraphs ($BODY_FILE contract, "Body gate, write check, and read-back", the known-limits paragraph, and the creation-race-reconcile paragraph) against both babysit-loop/reference/telemetry-upsert.md and attend-queue/SKILL.md — all three are still identical apart from the per-lane MARKER line, so the PATCH-exit-status fix and byte-floor rewrite from the prior commit (8b1ed72) carried through the move intact.
• The new file's intro paragraph was correctly rewritten, not copied wrong. babysit-loop's equivalent intro cites "cycle-shape step 6"; work-loop's cycle shape numbers its steps differently, and the new spoke's intro correctly drops that step-number claim rather than porting a wrong one over.
• The relative link style is consistent with the established convention, not just superficially similar: [reference/telemetry-upsert.md](reference/telemetry-upsert.md) at work-loop/SKILL.md#L73 uses a bare relative path rather than ${CLAUDE_PLUGIN_ROOT}/reference/..., which at first glance looks inconsistent with this same file's other reference links (tracker-seam.md, item-content-trust.md, pipeline-shape.md). Checking those: they all point at plugins/work-items/reference/*.md — a plugin-root-level shared reference directory — while the new link points at a skill-local skills/work-loop/reference/ subdirectory, exactly matching how babysit-loop links its own skill-local spoke. The distinction (plugin-root shared refs get ${CLAUDE_PLUGIN_ROOT}, skill-local spokes get a bare relative link) is real and intentional, not drift.
• No CHANGELOG/version bump was added for this commit, and correctly so — it's a pure internal reorganization with no user-facing behavior change; plugin.json and CHANGELOG.md are untouched by ddb50a5, consistent with the rest of the PR's version bookkeeping.
• Confirmed plugins/work-items/CHANGELOG.md's [0.31.1] entry and plugin.json's 0.31.1 still match after this commit (unaffected, as expected).

No new findings on this delta. The still-open, lower-priority item from earlier passes on this PR — work-items/loop-state@1 and source-control/babysit-loop-state@1 having no schema field for "previous cycle's telemetry write was UNREPORTED," so the "record it in durable state" instruction has nowhere to write — remains unaddressed but was not touched by this commit and is unaffected by the file split.

[claude]

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

---

Security Review

• Gather context (diff since last review round, 431a8e6 → ddb50a5)
• Read all changed files in full
• Analyze for logic flaws, injection surfaces, auth gaps, secret handling, dangerous patterns
• Post findings

The only change since the last security-review pass is commit ddb50a5 ("refactor(work-items): move work-loop's telemetry upsert to a reference spoke"), touching exactly two files: plugins/work-items/skills/work-loop/SKILL.md and the new plugins/work-items/skills/work-loop/reference/telemetry-upsert.md.

This is a pure verbatim relocation, not a logic change. work-loop/SKILL.md was at 499/500 lines on main; adding the #943 gate/write-check/read-back pushed it over the skill-quality line cap. The commit moves the entire upsert block (lane-instance resolution/validation, MARKER/SENT/SENTINEL_OK/VERIFY, the pre-write gate, the PATCH-exit-status check, VERIFY, and the duplicate-supersede/creation-race reconcile) out to a new reference file that SKILL.md now links to, mirroring the pattern babysit-loop already used. I diffed the moved block against its old location and against the sibling babysit-loop and attend-queue copies: byte-identical apart from the per-lane MARKER literal (work-items:work-loop@$INSTANCE vs. source-control:babysit-loop@$INSTANCE vs. work-items:attend-queue@$INSTANCE), which is expected and non-security-relevant.

No new shell logic, no new variable interpolation, no new API calls, and no change to quoting, gating, or control flow was introduced by this move — the PATCH-exit-status check, VERIFY read-back, and pre-write gate that were already reviewed and confirmed sound in the prior three review rounds are unchanged in substance, only relocated. All prior findings on this PR (the PATCH-exit-status gap, flagged by this lane and Codex) were already fixed in commit 8b1ed72 and remain fixed here.

No new security issues found in this delta.

[claude]

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

---

Security Review

• Gather context (diff since last security-review pass, ddb50a5 → 80eec2a)
• Read all changed files in full
• Analyze for logic flaws, injection surfaces, auth gaps, secret handling, dangerous patterns
• Post findings

The only change since the last security-review pass on this PR is commit 80eec2a ("docs(work-items): record the work-loop telemetry spoke move in the changelog"), which touches exactly one file: plugins/work-items/CHANGELOG.md (+8/-0 lines).

This is a pure prose addition — a new bullet in the [Unreleased]/0.31.1 entry documenting the prior commit's work-loop → reference/telemetry-upsert.md file move. No shell logic, no variable interpolation, no API calls, and no control-flow changes are introduced by this delta. I confirmed via git diff ddb50a5..80eec2a --name-only that no other file changed.

No new security issues found in this delta.

All substantive findings from earlier review rounds on this PR remain resolved as of this HEAD:

• The PATCH-exit-status gap (flagged by this lane and by the Codex reviewer) was fixed in commit 8b1ed72, which added a direct exit-status check on the gh api -X PATCH call before falling into VERIFY.
• The work-loop → reference-spoke relocation (ddb50a5) was reviewed in the prior pass and confirmed to be a byte-identical verbatim move with no logic change.
• $BODY_FILE continues to be passed via -F body=@ (binary-safe, never shell-evaluated) everywhere it's used; all shell variables remain quoted; $SENT/$MARKER are fixed per-lane literals; $CANON/$DUP are always numeric comment IDs sourced from jq .id — no injection surface anywhere in this PR's scope.

This PR remains prose/documentation only (inlined gh api telemetry-upsert blocks, a convention doc, CHANGELOGs, plugin manifest version bumps) — no workflow files, no dependency/action pins, so zizmor's advisory lane doesn't apply.

[claude]

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

---

Code Review

• Gather context (PR diff since last code-review pass, ddb50a5 → 80eec2a)
• Read the new commit in full
• Check consistency with prior findings
• Post findings

Per this repo's REVIEW.md, this lane owns correctness/design/conventions/error-handling/observability/tests/docs and leaves security to the dedicated security lane.

Since the last code-review pass (ddb50a5), one new commit landed: 80eec2a ("docs(work-items): record the work-loop telemetry spoke move in the changelog"). It touches exactly one file, plugins/work-items/CHANGELOG.md (+8/-0), adding a bullet under the existing [0.31.1] / ### Fixed entry that documents the prior ddb50a5 extraction (work-loop's upsert moving to reference/telemetry-upsert.md).

I checked the new bullet's factual claims directly rather than taking them at face value:

• "SKILL.md sat at 499 of its 500-line hard cap." Confirmed: git show c10f39b:plugins/work-items/skills/work-loop/SKILL.md | wc -l → 499. c10f39b is the merge-base commit (#1841, merged into this branch), i.e. the baseline this PR built on before its own additions — the figure is accurate.
• Current line counts. work-loop/SKILL.md is 437 lines, reference/telemetry-upsert.md is 121 lines — matches both this bullet and the commit message.
• "The rationale ... is now stated once instead of twice." Diffed ddb50a5 directly: the old SKILL.md had the "installed plugin cannot invoke a sibling plugin's scripts" rationale both in the "Shared tracker context" intro (still present, line 25-26) and restated in the "Telemetry and durable loop state" section body (now removed, replaced by a pointer to the spoke, which restates it once at reference/telemetry-upsert.md#L8-9). The claim holds for what it's describing (no longer duplicated within the telemetry section itself).
• Changelog placement/formatting. Bullet sits correctly under the still-open [0.31.1] entry (no premature second version bump needed since that entry hasn't been released), matches the existing bold-lead-bullet style, and doesn't disturb ordering.

No functional/shell logic changed in this delta — it's prose-only, consistent with what the security lane already found for this same commit.

Minor, non-blocking nit: this bullet documents a pure internal reorg (file split, no behavior change) but is filed under ### Fixed rather than ### Changed. Given it's presented as a sub-bullet directly under the parent #943 fix entry rather than a standalone top-level change, this is a defensible editorial choice, not an error — flagging only since Keep a Changelog (which this file cites) reserves "Fixed" for bug fixes.

No new correctness issues found. All prior findings on this PR are already resolved as of this HEAD:

• The PATCH-exit-status gap (flagged by this lane, the security lane, and Codex) — fixed in 8b1ed72.
• The leftover ||||||| diff3 merge markers in two CHANGELOGs — fixed in 8b1ed72.
• The claude-ops CHANGELOG's [0.25.1] entry understating its own fix as "both halves" instead of "three checks" — fixed in 431a8e6.
• Still open, low-priority, unaffected by this commit: work-items/loop-state@1/source-control/babysit-loop-state@1 have no schema field for "previous cycle's telemetry write was UNREPORTED," so the "record it in durable state" instruction has nowhere to write to on a failed cycle. Worth a follow-up, not blocking.

[claude]

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

---

Security Review

• Gather context (diff since last security-review pass, 80eec2a → 60d6860)
• Read the new commit in full
• Check consistency with prior findings
• Post findings

The only change since the last security-review pass on this PR is commit 60d6860 ("fix(loop-lane): make the notices and the changelogs true of the branches they describe"), touching five files: the claude-ops and source-control CHANGELOGs, and the three inlined telemetry-upsert bodies (babysit-loop's reference file, attend-queue/SKILL.md, work-loop's reference file).

This is a prose-and-diagnostics-only commit, not a logic change:

• CHANGELOG wording fix. plugins/claude-ops/CHANGELOG.md#L14-L20 now says "three checks" (pre-write gate, write's own exit status, post-write read-back) instead of the stale "both halves" — this closes the wording-drift finding from the prior code-review pass and has no security content itself.
• || true added after the create-path gh api -X POST (e.g. work-loop/reference/telemetry-upsert.md#L61). I checked whether this changes control flow: the block has no set -e, and this line's exit status was never checked before either — the subsequent LIST=$(LOOKUP) || LIST="" and the [ -z "$CANON" ] branch already handle a failed/no-op create by falling through to the "no comment available to write to" UNREPORTED report. The || true is a no-op clarification, not a behavior change, and introduces no new trust boundary.
• Diagnostic message wording only — "no sentinel-prefixed comment" → "no comment available to write to (a create may have landed but was not re-found)", and "the comment still holds an earlier cycle" → "the comment holds an earlier body, not this cycle's write". Both are stderr-only text changes with no interpolation of untrusted data beyond the existing numeric $CANON, already reviewed.
• Reconcile-loop prose clarification — "sentinel comment is edited to a one-line tombstone — only once the canonical write verifies —" makes explicit that the duplicate-supersede for DUP in ... loop only runs in the final else branch (i.e., only after VERIFY has already succeeded). I confirmed this against the actual code structure in all three files: the loop is unreachable unless every prior elif (gate/PATCH-status/VERIFY) has passed, so the prose now accurately describes control flow that was already correct — no code changed here, only the description of it.

No new shell logic, no new variable interpolation, no new API calls, and no change to quoting, gating, or control flow was introduced by this delta. All prior findings on this PR remain resolved as of this HEAD:

• The PATCH-exit-status gap (flagged by this lane and by the Codex reviewer) was fixed in commit 8b1ed72.
• $BODY_FILE continues to be passed via -F body=@ (binary-safe, never shell-evaluated) everywhere; all shell variables remain quoted; $SENT/$MARKER are fixed per-lane literals; $CANON/$DUP are always numeric comment IDs sourced from jq .id — no injection surface anywhere in this PR's scope.

This PR remains prose/documentation only (inlined gh api telemetry-upsert blocks, a convention doc, CHANGELOGs, plugin manifest version bumps) — no workflow files, no dependency/action pins, so zizmor's advisory lane doesn't apply.

No new security issues found in this delta, and no outstanding security findings remain open on this PR.

[claude]

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

---

Security Review

• Gather context (diff since last security-review pass, 60d6860 → 18cfe89)
• Read all changed files in full
• Analyze for logic flaws, injection surfaces, auth gaps, secret handling, dangerous patterns
• Post findings

The only change since the last security-review pass on this PR is merge commit 18cfe89 ("Merge origin/main into fix/943-inlined-upsert-body-gate"), which pulled in origin/main's unrelated commit 353baf6 (#1851, "latch marker consumption; stop reading a JSON false as absent").

Restricting to the files this PR actually owns (git diff origin/main...HEAD --stat confirms the PR's file set is unchanged: the same 11 files as before), the merge touched exactly one of them — plugins/claude-ops/CHANGELOG.md — adding 15 lines. I diffed that delta directly: it's main's own new ### Fixed entry for #1784 (a jq //-operator falsy-vs-absent bug in lane-launcher.sh, unrelated to this PR's telemetry-upsert work), merged in cleanly alongside this PR's own [0.25.1] entry. No <<<<<<</=======/>>>>>>>/||||||| conflict-marker residue anywhere in plugins/claude-ops/CHANGELOG.md, plugins/source-control/CHANGELOG.md, or plugins/work-items/CHANGELOG.md (checked directly — the diff3-marker class of defect flagged in an earlier round of this PR does not recur here).

No shell logic, no variable interpolation, no API calls, and no telemetry-upsert control flow were touched by this merge. This PR remains prose/documentation only (inlined gh api telemetry-upsert blocks, a convention doc, CHANGELOGs, plugin manifest version bumps) — no workflow files, no dependency/action pins, so zizmor's advisory lane doesn't apply.

No new security issues found in this delta, and no outstanding security findings remain open on this PR. All substantive findings from earlier review rounds remain resolved as of this HEAD:

• The PATCH-exit-status gap (flagged by this lane and by the Codex reviewer) — fixed in commit 8b1ed72, which added a direct exit-status check on the gh api -X PATCH call before falling into VERIFY.
• $BODY_FILE continues to be passed via -F body=@ (binary-safe, never shell-evaluated) everywhere it's used; all shell variables remain quoted; $SENT/$MARKER are fixed per-lane literals; $CANON/$DUP are always numeric comment IDs sourced from jq .id — no injection surface anywhere in this PR's scope.