diff --git a/docs/conventions/loop-lane/README.md b/docs/conventions/loop-lane/README.md index 3d02950ab..0834d54c1 100644 --- a/docs/conventions/loop-lane/README.md +++ b/docs/conventions/loop-lane/README.md @@ -426,9 +426,20 @@ item, one each, and no instance ever edits another's. `claude-ops`'s `telemetry-upsert.sh` is the interim home of this contract and a compatible reader (`morning-brief` reads the same surface); an installed plugin cannot invoke a sibling plugin's script, so each lane **inlines** the small `gh api` upsert and the coupling to `claude-ops` stays -one-directional. An inlined upsert carries none of the wrapper's body checks, so it is bound by the -`@path`-as-body rule in [`claude-ops` lanes](../../../plugins/claude-ops/skills/lanes/SKILL.md), -section "Never pass a body as an `@path` string". +one-directional. An inlined upsert is bound by the `@path`-as-body rule in +[`claude-ops` lanes](../../../plugins/claude-ops/skills/lanes/SKILL.md), section "Never pass a body as +an `@path` string", and encodes that rule mechanically in its own block (#943) as three checks. A +**pre-write gate** refuses a body that is empty, a literal `@path`, not sentinel-prefixed, or under a +16-byte payload floor measured below the sentinel line, before any API call. The **write's own exit +status** is then checked, because a failed write leaves the previous cycle's body in place — which a +read-back running regardless would accept. A **post-write read-back** re-reads what the write stored, +the only check that sees a write which reported success and stored something else. Every branch that +ends without a verified body reports UNREPORTED and skips the duplicate-supersede pass, so a cycle +whose own write is unproven never tombstones a racing session's comment; carry that forward, since +stderr does not survive the session. Known limits inherited from the wrapper: a PATCH that succeeds +while storing the previous body still verifies, and the read-back proves *some* well-formed telemetry +is present, not *this* cycle's. Not replicated inline: the 64 KiB cap, the body-file containment +checks, retries, and the wrapper's distinct non-zero exits — every inline branch exits 0. **Lane-instance identity (#1295).** The marker names the **writer**, not the lane type. A marker that names only the lane makes two concurrent instances resolve one comment and clobber each other's diff --git a/plugins/claude-ops/CHANGELOG.md b/plugins/claude-ops/CHANGELOG.md index 35aca1b83..5008312ca 100644 --- a/plugins/claude-ops/CHANGELOG.md +++ b/plugins/claude-ops/CHANGELOG.md @@ -5,6 +5,28 @@ All notable changes to the `claude-ops` plugin are documented here. Format follo ## [0.25.1] +### Changed + +- **The `@path`-as-body rule now records that an inlined upsert enforces it mechanically, not on + trust — and corrects which consumer the failure actually deceives (#943).** The rule's closing + paragraph claimed the prose was "the only thing standing between" an inlined upsert and a silent + observability fail-open. That is no longer true: every lane that inlines the `gh api` upsert — + `source-control:babysit-loop`, `work-items:work-loop`, `work-items:attend-queue` — now carries three + checks in its own block: a pre-write body gate, a check of the write's own exit status, and a + post-write read-back of what the write stored. The paragraph states which guarantees travel inline + (those three) and which do not: the 64 KiB cap, the body-file containment checks, retries, and this + script's distinct non-zero exit codes — an inline branch always exits 0 and reports through stderr, + so a caller cannot detect a failed cycle from its exit status. It also names the limits an inline + block inherits rather than fixes: a PATCH that succeeds while storing the previous body still + verifies, and the read-back proves *some* well-formed telemetry is present, not *this* cycle's. +- **Corrected: `morning-brief` is not the check a degraded telemetry body deceives.** The rule said a + freshness check "passes over a blind lane". Verified against `morning-brief.sh`'s `print_telemetry`: + it parses `lane:` and `last-cycle:` out of the comment BODY, so an `@path` body carries no `lane:` + field and the lane disappears from the report entirely rather than reading as healthy. What a + degraded body deceives is any consumer keying on the comment's timestamp instead of its body — the + timestamp moves on every successful write regardless of content. The rule now attributes the + failure that way rather than naming a sibling reader that would in fact surface it. + ### Fixed - **`lanes`: a lane field whose JSON value is `false` is no longer read as an absent field (#1784).** diff --git a/plugins/claude-ops/skills/lanes/SKILL.md b/plugins/claude-ops/skills/lanes/SKILL.md index ff8fa097e..1ff26e179 100644 --- a/plugins/claude-ops/skills/lanes/SKILL.md +++ b/plugins/claude-ops/skills/lanes/SKILL.md @@ -291,10 +291,25 @@ takes `gh issue comment --body-file`, or `gh api -F`/`--field key=@path` — per command's own `--help` (gh 2.95.0); `gh api` has no `--body-file` flag at all. The failure is invisible from the outside (#943): the comment's timestamp still -moves, so the telemetry surface looks **fresh** while carrying no data, and a -freshness check passes over a blind lane. `telemetry-upsert.sh` refuses such a -body before it writes anything; an inlined upsert has no such gate, so this rule -is the only thing standing between it and a silent observability fail-open. +moves, so any check keying on `updatedAt` reads the lane as **fresh** while it +carries no data. (This skill's sibling reader `morning-brief` is not that check — +it parses `lane:` and `last-cycle:` out of the body, so a degraded comment makes +the lane vanish from its report rather than look healthy. What a degraded body +deceives is any consumer that keys on the comment's timestamp instead of reading +its body.) + +`telemetry-upsert.sh` refuses such a body before it writes anything and re-reads +what landed afterward. An inlined upsert now encodes three checks itself: a +pre-write gate (empty, leading `@`, not sentinel-prefixed, or under a 16-byte +payload floor measured below the sentinel line → skip the cycle, no API call), a +check of the write's own exit status (a failed write leaves the previous cycle's +body in place, which a read-back running regardless would accept), and a +post-write read-back of what the write stored. What an inline block does NOT +replicate: the 64 KiB cap, the body-file containment checks, retries, and this +script's distinct non-zero exit codes — an inline branch always exits 0 and +reports through stderr. It also inherits this script's own limits: a PATCH that +succeeds while storing the previous body still verifies, and the read-back proves +*some* well-formed telemetry is present, not *this* cycle's. ## Cross-references diff --git a/plugins/source-control/.claude-plugin/plugin.json b/plugins/source-control/.claude-plugin/plugin.json index b5a202e2b..449ae933d 100644 --- a/plugins/source-control/.claude-plugin/plugin.json +++ b/plugins/source-control/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "source-control", - "version": "0.44.0", + "version": "0.44.1", "description": "Git and GitHub delivery workflow: /commit (Conventional Commits + Co-Authored-By trailer via safe heredoc mechanics), /pull-request (prep, create, CI monitoring, review-comment triage, merge, CI-log fetch), /babysit-prs (self-pacing fleet loop — safe by default; opt-in worker/autopilot tiers add gate-checked merge and thread resolution behind a deterministic Python engine), /babysit-loop (the loop-lane merge lane: a standing or drain loop that invokes babysit-prs per cycle, configured through repo-scoped babysit_loop_* keys on the layered source-control.md seam, with merge authority human-only until the target repo's tracked config adopts the lane, a gate-proven C2-mechanical baseline once adopted, and standing merge-rung raises binding from the team-tracked layer only — with one named exception, where an invocation line explicitly typing both the autopilot tier keyword and the dedicated raise argument --merge c3-this-run widens that single invocation's merge authority up to C3 behind a fresh independent frontier-tier resolver, while C4-structural and C5-untrusted-provenance stay unconditionally human-merge), /worktree (create, status, cleanup, audit for parallel-session isolation), /setup (check the effective commit-subject / PR-title convention merged across its config layers and the babysit-prs config, or apply — interview the repo and write the convention config to a chosen layer), and /resolve-conflicts (intent-first merge/rebase conflict resolution with a semantic-conflict sweep — never --abort). The commit-subject / PR-title convention is configurable via a source-control.md config written by a re-runnable setup skill, layered across a ~/.claude user-global file, the tracked team file, and a gitignored .claude/source-control.local.md personal overlay merged per key; Conventional Commits is the default when no convention is declared.", "author": { "name": "Melodic Software", diff --git a/plugins/source-control/CHANGELOG.md b/plugins/source-control/CHANGELOG.md index 016d4acf9..2c06805d3 100644 --- a/plugins/source-control/CHANGELOG.md +++ b/plugins/source-control/CHANGELOG.md @@ -3,6 +3,37 @@ All notable changes to the `source-control` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.44.1] + +### Fixed + +- **`babysit-loop`'s inlined telemetry upsert now gates its body and verifies what landed (#943).** + The lane's telemetry comment was observed carrying a literal `@C:/…/telemetry_combined.txt` as its + entire body across three sessions: `gh` expands a leading `@` only for `--body-file` / `-F + field=@file`, so an `@path` passed as a body VALUE is transmitted as text. `claude-ops`'s + `telemetry-upsert.sh` refuses such a body and re-reads what it wrote, but an installed plugin cannot + invoke a sibling plugin's script, so this lane inlines its own upsert and inherited neither + protection. The block now carries three checks, which catch different failures. A **pre-write gate** + rejects a `$BODY_FILE` that is empty, opens with a literal `@`, is not sentinel-prefixed, or holds + under 16 bytes of payload — no POST, no PATCH. The **write's own exit status** is then checked, because a + failed PATCH leaves the previous cycle's body in place and a read-back running regardless would + accept it. A **post-write read-back** 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 + check that would have caught the actual #943 shape, where the composed file is perfectly fine and + the defect is the invocation (`-f body=@FILE` instead of `-F body=@FILE`) — a file-only check is + structurally blind to it. The create path is covered by the same cycle's + PATCH, and a degraded POST leaves no sentinel-prefixed comment to re-read, so that branch now + reports UNREPORTED too instead of falling through silently. The 16-byte floor is measured on everything below the + sentinel LINE, so it matches the wrapper's `MIN_BODY_BYTES` byte-for-byte on LF and CRLF alike; + prefix comparison is byte-wise, so a CRLF body is not false-rejected. Every branch that ends without + a verified body reports UNREPORTED and skips the duplicate-supersede pass, so a cycle whose own + write is unproven never tombstones a racing session's comment. The `$BODY_FILE` sentinel-first-line contract is now stated in + prose rather than left implicit in a comment. Two wrapper limits are inherited rather than fixed: a + PATCH that succeeds while storing the previous body still verifies, and the read-back proves *some* + well-formed telemetry is present, not *this* cycle's. Not replicated at all: the 64 KiB cap, the + body-file containment checks, retries, and the wrapper's distinct non-zero exits — every inline + branch exits 0 and reports through stderr. + ## [0.44.0] ### Fixed diff --git a/plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md b/plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md index b06f5d45b..ce9eb2ddc 100644 --- a/plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md +++ b/plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md @@ -35,19 +35,36 @@ esac exit 1 } MARKER="source-control:babysit-loop@$INSTANCE" -SENT="" # first line of $BODY_FILE +SENT="" # $BODY_FILE MUST open with this line LOOKUP() { gh api --paginate "repos/$REPO/issues/$ISSUE/comments" \ --jq ".[] | select(.body | startswith(\"$SENT\")) | .id"; } -if ! LIST=$(LOOKUP); then +SENTINEL_OK() { # $1 = text; true iff line 1 is exactly $SENT and >=16 payload bytes follow + [ "$(printf '%s' "$1" | head -c ${#SENT})" = "$SENT" ] && + [ "$(printf '%s' "$1" | tail -n +2 | wc -c | tr -d ' ')" -ge 16 ] +} +VERIFY() { # $1 = comment id; re-read what LANDED, whatever form the write took + BACK="$(gh api "repos/$REPO/issues/comments/$1" --jq '.body' 2>/dev/null | tr -d '\r')" && + SENTINEL_OK "$BACK" +} +if [ ! -s "$BODY_FILE" ] || [ "$(head -c 1 "$BODY_FILE")" = "@" ]; then + echo "telemetry: body is empty or a literal @path - nothing written; fix the body composition, do not re-run blind" >&2 +elif ! SENTINEL_OK "$(cat "$BODY_FILE")"; then + echo "telemetry: body is not sentinel-prefixed or carries no payload - nothing written; fix the body composition, do not re-run blind" >&2 +elif ! LIST=$(LOOKUP); then echo "telemetry: comment lookup failed; skipping upsert this cycle (fail closed)" >&2 else if [ -z "$LIST" ]; then - gh api -X POST "repos/$REPO/issues/$ISSUE/comments" -F body=@"$BODY_FILE" >/dev/null + gh api -X POST "repos/$REPO/issues/$ISSUE/comments" -F body=@"$BODY_FILE" >/dev/null || true LIST=$(LOOKUP) || LIST="" # re-list; a failure here converges next cycle fi CANON=$(printf '%s\n' "$LIST" | sort -n | head -n1) - if [ -n "$CANON" ]; then - gh api -X PATCH "repos/$REPO/issues/comments/$CANON" -F body=@"$BODY_FILE" + if [ -z "$CANON" ]; then + echo "telemetry: no comment available to write to (a create may have landed but was not re-found) - treat the lane as UNREPORTED and carry that forward to the next cycle" >&2 + 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 and carry that forward to the next cycle; the comment holds an earlier body, not this cycle's write" >&2 + elif ! VERIFY "$CANON"; then + echo "telemetry: comment $CANON does NOT carry a well-formed telemetry body after the write - treat the lane as UNREPORTED and carry that forward to the next cycle; do not trust the timestamp" >&2 + else for DUP in $(printf '%s\n' "$LIST" | sort -n | tail -n +2); do gh api -X PATCH "repos/$REPO/issues/comments/$DUP" \ -f body="Superseded duplicate - canonical telemetry comment: $CANON" || true @@ -56,11 +73,44 @@ else fi ``` +**`$BODY_FILE` contract.** The file's FIRST line must be exactly `$SENT`, with the cycle's telemetry +below it. The lookup matches on that prefix, so a body composed without it is not merely rejected here +— it would never be found again, and the next cycle would post a second comment. Compose the sentinel +into the file; do not rely on anything downstream to add it. + +**Body gate, write check, and read-back (encoded above, #943).** Three checks, because they catch +different failures. The **pre-write** assertions run before any API call and reject a `$BODY_FILE` +that is empty, opens with a literal `@`, is not sentinel-prefixed, or carries under 16 payload bytes +below the sentinel — the mechanical form of the `@path`-as-body rule owned by the `claude-ops` lanes +skill ("Never pass a body as an `@path` string"). 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 +**write's own exit status** is checked next: a PATCH that fails leaves the previous cycle's body in +place, which a read-back running regardless would happily accept. The **post-write** `VERIFY` then +re-reads what the write stored — the only check that sees a write which reported success and stored +something else: a mangled body, a concurrent overwrite, a deleted comment. It is also the half that +would have caught #943 itself, where the composed file was correct and the defect was the invocation +(`-f body=@FILE` transmits the literal path; this block only ever uses `-F body=@`). + +Every branch that ends without a verified body says so and skips the duplicate-supersede pass, so a +cycle whose own write is unproven never tombstones a racing session's comment. A degraded body that +does land still moves the comment's timestamp, so any consumer keying on that timestamp rather than +on the body reads the lane as **fresh** while it carries nothing — which is why a refusal, a failed +write, and a failed verification all have to be carried forward: stderr does not survive the session, +and the next cycle must see that this one did not report. A lane with durable loop state records it +there; a lane without one carries it in the cycle's own summary. + +Known limits, inherited from the wrapper: a PATCH that succeeds while storing the previous body still +verifies, and `VERIFY` asserts that *some* well-formed telemetry is present, not that *this* cycle's +write is what is present. Not replicated at all: the 64 KiB cap, the body-file containment checks, +retries, and the wrapper's distinct non-zero exit codes — every branch here exits 0 and reports +through stderr alone. + **Creation race reconcile (encoded above).** Two sessions racing the first-ever upsert can both see an empty lookup and both POST, forking the singleton. The upsert converges every cycle duplicates are visible: the LOWEST comment id is canonical (numeric sort, deterministic for every session), the canonical comment receives the current cycle's full state, and every other -sentinel comment is edited to a one-line tombstone so it never matches a lookup again — this +sentinel comment is edited to a one-line tombstone — only once the canonical write verifies — so +it never matches a lookup again — this covers a racer that died between its POST and its own re-list, because the NEXT session's ordinary upsert performs the same reconcile. A crashed racer's unmerged counters are an accepted loss (durable state re-derives over a cycle); nothing is deleted. The reconcile converges diff --git a/plugins/work-items/.claude-plugin/plugin.json b/plugins/work-items/.claude-plugin/plugin.json index 6ac9ea29a..f27a40803 100644 --- a/plugins/work-items/.claude-plugin/plugin.json +++ b/plugins/work-items/.claude-plugin/plugin.json @@ -1,7 +1,7 @@ { "$schema": "https://json.schemastore.org/claude-code-plugin-manifest.json", "name": "work-items", - "version": "0.31.0", + "version": "0.31.1", "description": "Manages development work items through a provider-neutral tracker seam that ships with the plugin (bundled dispatcher plus github and local-markdown adapters; seam plugin-dir canonical, adapters consumer-local-first): dashboard, taxonomy-labeled creation, a race-safe assignee-plus-lease claim protocol, recurring-schedule checks, TODO scanning, stale-lease auditing, plan decomposition into vertical-slice items, raw-intake triage (issues and unsolicited PRs through raw, verified, briefed, autonomous-eligible states), plus the two work-items loop lanes of the loop-lane convention: a self-paced autonomous work-loop drain (work-class admission gate, adaptive item cap, PR-only) and an attended attend-queue escalation lane. The re-runnable setup skill binds the provider (.work-item-tracker.json), seeds the recurring-schedule seam (.github/recurring-schedule.json), and remaps canonical role labels.", "author": { "name": "Melodic Software", diff --git a/plugins/work-items/CHANGELOG.md b/plugins/work-items/CHANGELOG.md index 8769ef555..9acec7042 100644 --- a/plugins/work-items/CHANGELOG.md +++ b/plugins/work-items/CHANGELOG.md @@ -3,6 +3,45 @@ All notable changes to the `work-items` plugin are documented here. Format follows [Keep a Changelog](https://keepachangelog.com/en/1.1.0/); this plugin uses semantic versioning. +## [0.31.1] + +### Fixed + +- **`work-loop` and `attend-queue`'s inlined telemetry upserts now gate their body and verify what + landed (#943).** Both lanes inline the same `gh api` upsert the babysit lane does — an installed + plugin cannot invoke `claude-ops`'s `telemetry-upsert.sh` — and so inherited none of that wrapper's + body checks. The defect that surfaced on the babysit lane is a property of the shared upsert shape, + not of one lane: an `@path` passed as a body VALUE is transmitted as literal text (`gh` expands a + leading `@` only for `--body-file` / `-F field=@file`). Both blocks now carry three checks. A + **pre-write gate** rejects a `$BODY_FILE` that is empty, opens with a literal `@`, is not + sentinel-prefixed, or holds under 16 bytes of payload — no POST, no PATCH. The **write's own exit + status** is then checked, because a failed PATCH leaves the previous cycle's body in place and a + read-back running regardless would accept it. A **post-write read-back** 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 check that would have caught the actual #943 shape, where the composed + file is fine and the defect is the invocation (`-f body=@FILE` instead of `-F body=@FILE`) — a + file-only check cannot see it. Every branch that ends without a verified body — including a + degraded create, which leaves no sentinel-prefixed comment to re-read — reports UNREPORTED and + skips the duplicate-supersede pass, so a cycle whose own write is unproven never tombstones a + racing session's comment. The 16-byte floor is measured on everything below the sentinel LINE, so + it matches the wrapper's `MIN_BODY_BYTES` byte-for-byte on LF and CRLF alike; prefix comparison is + byte-wise, so a CRLF body is not false-rejected. `work-loop` additionally records a refusal or + failed verification in durable loop state; `attend-queue` has none, so it carries the same fact in + the cycle's own summary — either way stderr does not survive the session and a cycle that did not + report must stay visible to the next one. The `$BODY_FILE` sentinel-first-line contract is now + stated in prose. Two wrapper limits are inherited rather than fixed: a PATCH that succeeds while + storing the previous body still verifies, and the read-back proves *some* well-formed telemetry is + present, not *this* cycle's. Not replicated at all: the 64 KiB cap, the containment checks, + retries, and the wrapper's distinct non-zero exits — every inline branch exits 0. +- **`work-loop`'s telemetry upsert moves to `reference/telemetry-upsert.md`.** SKILL.md sat at 499 of + its 500-line hard cap, so the checks above did not fit. The upsert — lane-instance resolution and + validation, the singleton lookup, the body gate, the write-status check and read-back, the + POST/PATCH, and the creation-race reconcile — moves verbatim into a spoke, the same shape the + sibling `source-control:babysit-loop` lane already uses for the identical block. SKILL.md keeps the + telemetry home and the durable-state contract and points at the spoke for the mechanism; the + rationale for inlining rather than calling `claude-ops`'s wrapper is now stated once instead of + twice. + ## [0.31.0] ### Fixed diff --git a/plugins/work-items/skills/attend-queue/SKILL.md b/plugins/work-items/skills/attend-queue/SKILL.md index 7372169e2..db954468b 100644 --- a/plugins/work-items/skills/attend-queue/SKILL.md +++ b/plugins/work-items/skills/attend-queue/SKILL.md @@ -134,19 +134,36 @@ esac exit 1 } MARKER="work-items:attend-queue@$INSTANCE" -SENT="" # first line of $BODY_FILE +SENT="" # $BODY_FILE MUST open with this line LOOKUP() { gh api --paginate "repos/$REPO/issues/$ISSUE/comments" \ --jq ".[] | select(.body | startswith(\"$SENT\")) | .id"; } -if ! LIST=$(LOOKUP); then +SENTINEL_OK() { # $1 = text; true iff line 1 is exactly $SENT and >=16 payload bytes follow + [ "$(printf '%s' "$1" | head -c ${#SENT})" = "$SENT" ] && + [ "$(printf '%s' "$1" | tail -n +2 | wc -c | tr -d ' ')" -ge 16 ] +} +VERIFY() { # $1 = comment id; re-read what LANDED, whatever form the write took + BACK="$(gh api "repos/$REPO/issues/comments/$1" --jq '.body' 2>/dev/null | tr -d '\r')" && + SENTINEL_OK "$BACK" +} +if [ ! -s "$BODY_FILE" ] || [ "$(head -c 1 "$BODY_FILE")" = "@" ]; then + echo "telemetry: body is empty or a literal @path - nothing written; fix the body composition, do not re-run blind" >&2 +elif ! SENTINEL_OK "$(cat "$BODY_FILE")"; then + echo "telemetry: body is not sentinel-prefixed or carries no payload - nothing written; fix the body composition, do not re-run blind" >&2 +elif ! LIST=$(LOOKUP); then echo "telemetry: comment lookup failed; skipping upsert this cycle (fail closed)" >&2 else if [ -z "$LIST" ]; then - gh api -X POST "repos/$REPO/issues/$ISSUE/comments" -F body=@"$BODY_FILE" >/dev/null + gh api -X POST "repos/$REPO/issues/$ISSUE/comments" -F body=@"$BODY_FILE" >/dev/null || true LIST=$(LOOKUP) || LIST="" # re-list; a failure here converges next cycle fi CANON=$(printf '%s\n' "$LIST" | sort -n | head -n1) - if [ -n "$CANON" ]; then - gh api -X PATCH "repos/$REPO/issues/comments/$CANON" -F body=@"$BODY_FILE" + if [ -z "$CANON" ]; then + echo "telemetry: no comment available to write to (a create may have landed but was not re-found) - treat the lane as UNREPORTED and carry that forward to the next cycle" >&2 + 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 and carry that forward to the next cycle; the comment holds an earlier body, not this cycle's write" >&2 + elif ! VERIFY "$CANON"; then + echo "telemetry: comment $CANON does NOT carry a well-formed telemetry body after the write - treat the lane as UNREPORTED and carry that forward to the next cycle; do not trust the timestamp" >&2 + else for DUP in $(printf '%s\n' "$LIST" | sort -n | tail -n +2); do gh api -X PATCH "repos/$REPO/issues/comments/$DUP" \ -f body="Superseded duplicate - canonical telemetry comment: $CANON" || true @@ -155,11 +172,44 @@ else fi ``` +**`$BODY_FILE` contract.** The file's FIRST line must be exactly `$SENT`, with the pass report below +it. The lookup matches on that prefix, so a body composed without it is not merely rejected here — it +would never be found again, and the next pass would post a second comment. Compose the sentinel into +the file; do not rely on anything downstream to add it. + +**Body gate, write check, and read-back (encoded above, #943).** Three checks, because they catch +different failures. The **pre-write** assertions run before any API call and reject a `$BODY_FILE` +that is empty, opens with a literal `@`, is not sentinel-prefixed, or carries under 16 payload bytes +below the sentinel — the mechanical form of the `@path`-as-body rule owned by the `claude-ops` lanes +skill ("Never pass a body as an `@path` string"). 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 +**write's own exit status** is checked next: a PATCH that fails leaves the previous cycle's body in +place, which a read-back running regardless would happily accept. The **post-write** `VERIFY` then +re-reads what the write stored — the only check that sees a write which reported success and stored +something else: a mangled body, a concurrent overwrite, a deleted comment. It is also the half that +would have caught #943 itself, where the composed file was correct and the defect was the invocation +(`-f body=@FILE` transmits the literal path; this block only ever uses `-F body=@`). + +Every branch that ends without a verified body says so and skips the duplicate-supersede pass, so a +cycle whose own write is unproven never tombstones a racing session's comment. A degraded body that +does land still moves the comment's timestamp, so any consumer keying on that timestamp rather than +on the body reads the lane as **fresh** while it carries nothing — which is why a refusal, a failed +write, and a failed verification all have to be carried forward: stderr does not survive the session, +and the next cycle must see that this one did not report. A lane with durable loop state records it +there; a lane without one carries it in the cycle's own summary. + +Known limits, inherited from the wrapper: a PATCH that succeeds while storing the previous body still +verifies, and `VERIFY` asserts that *some* well-formed telemetry is present, not that *this* cycle's +write is what is present. Not replicated at all: the 64 KiB cap, the body-file containment checks, +retries, and the wrapper's distinct non-zero exit codes — every branch here exits 0 and reports +through stderr alone. + **Creation race reconcile (encoded above).** Two sessions racing the first-ever upsert can both see an empty lookup and both POST, forking the singleton. The upsert converges every cycle duplicates are visible: the LOWEST comment id is canonical (numeric sort, deterministic for every session), the canonical comment receives the current cycle's full state, and every other -sentinel comment is edited to a one-line tombstone so it never matches a lookup again — this +sentinel comment is edited to a one-line tombstone — only once the canonical write verifies — so +it never matches a lookup again — this covers a racer that died between its POST and its own re-list, because the NEXT session's ordinary upsert performs the same reconcile. A crashed racer's unmerged counters are an accepted loss (durable state re-derives over a cycle); nothing is deleted. The reconcile converges diff --git a/plugins/work-items/skills/work-loop/SKILL.md b/plugins/work-items/skills/work-loop/SKILL.md index ee77ec5a4..7b9eaf1f3 100644 --- a/plugins/work-items/skills/work-loop/SKILL.md +++ b/plugins/work-items/skills/work-loop/SKILL.md @@ -68,71 +68,9 @@ The telemetry home is a **per-lane tracking issue in the target repository**, re config; default: the open issue titled `Lane telemetry: work-loop` (exact match), created through the seam `create-item` verb when absent (announce the creation). Maintain exactly ONE status comment on it **per lane instance**, sentinel-identified and edited in place (the `claude-ops` -lane-telemetry contract; one writer identity owns a marker). The upsert is inlined here because an -installed plugin cannot invoke a sibling plugin's scripts. - -**Resolve the lane instance first (#1295).** The marker names the *writer*, not the lane type — per -the convention's lane-instance identity rule. The id is `${user_config.lane_instance}`; a surviving -literal `${user_config.…}` placeholder means the key is unset, so fall back to the sanitized -lowercased hostname (headless-config floor: log the assumption). It is operator-supplied text about -to be interpolated into a shell string and a `jq` program, so it is validated and **rejected**, -never sanitized-and-continued. Substitute the resolved value for `` below; the check -runs **before** `MARKER` is built, because a lane that validates only in prose has documented a -guard that does not run: - -```bash -INSTANCE="" # ${user_config.lane_instance}, else `hostname` sanitized -[ -n "$INSTANCE" ] || INSTANCE="$(hostname | tr '[:upper:]' '[:lower:]' | tr -c 'a-z0-9-' '-')" -# ^[a-z0-9][a-z0-9-]{0,31}$ — empty, a leading hyphen, any other character, or -# over 32 chars is REJECTED, never trimmed into something that looks valid. -case "$INSTANCE" in -"" | -* | *[!a-z0-9-]*) - echo "telemetry: lane_instance '$INSTANCE' is not ^[a-z0-9][a-z0-9-]{0,31}\$; refusing to build a marker" >&2 - exit 1 - ;; -esac -[ "${#INSTANCE}" -le 32 ] || { - echo "telemetry: lane_instance '$INSTANCE' exceeds 32 characters; refusing to build a marker" >&2 - exit 1 -} -``` - -The hostname fallback is a *default*, not a sanitizer: the same gate validates it, so a hostname -that cannot produce a conforming id stops the lane rather than yielding a marker nobody chose. - -```bash -MARKER="work-items:work-loop@$INSTANCE" -SENT="" # first line of $BODY_FILE -LOOKUP() { gh api --paginate "repos/$REPO/issues/$ISSUE/comments" \ - --jq ".[] | select(.body | startswith(\"$SENT\")) | .id"; } -if ! LIST=$(LOOKUP); then - echo "telemetry: comment lookup failed; skipping upsert this cycle (fail closed)" >&2 -else - if [ -z "$LIST" ]; then - gh api -X POST "repos/$REPO/issues/$ISSUE/comments" -F body=@"$BODY_FILE" >/dev/null - LIST=$(LOOKUP) || LIST="" # re-list; a failure here converges next cycle - fi - CANON=$(printf '%s\n' "$LIST" | sort -n | head -n1) - if [ -n "$CANON" ]; then - gh api -X PATCH "repos/$REPO/issues/comments/$CANON" -F body=@"$BODY_FILE" - for DUP in $(printf '%s\n' "$LIST" | sort -n | tail -n +2); do - gh api -X PATCH "repos/$REPO/issues/comments/$DUP" \ - -f body="Superseded duplicate - canonical telemetry comment: $CANON" || true - done - fi -fi -``` - -**Creation race reconcile (encoded above).** Two sessions racing the first-ever upsert can both -see an empty lookup and both POST, forking the singleton. The upsert converges every cycle -duplicates are visible: the LOWEST comment id is canonical (numeric sort, deterministic for -every session), the canonical comment receives the current cycle's full state, and every other -sentinel comment is edited to a one-line tombstone so it never matches a lookup again — this -covers a racer that died between its POST and its own re-list, because the NEXT session's -ordinary upsert performs the same reconcile. A crashed racer's unmerged counters are an -accepted loss (durable state re-derives over a cycle); nothing is deleted. The reconcile converges -duplicates **within one instance's own sentinel set** — a sibling instance's comment carries a -different marker and never enters `$LIST`, so it is neither canonical nor tombstoned. +lane-telemetry contract; one writer identity owns a marker). The upsert itself — lane-instance resolution and validation, the singleton lookup, the body gate, the +write-status check and read-back, the POST/PATCH, and the creation-race reconcile — is owned by +[reference/telemetry-upsert.md](reference/telemetry-upsert.md). When the bound provider is not `github`, this upsert is unavailable: carry the same telemetry content — state block included — in the lane's cycle report/log, noting the comment surface is diff --git a/plugins/work-items/skills/work-loop/reference/telemetry-upsert.md b/plugins/work-items/skills/work-loop/reference/telemetry-upsert.md new file mode 100644 index 000000000..832c315cf --- /dev/null +++ b/plugins/work-items/skills/work-loop/reference/telemetry-upsert.md @@ -0,0 +1,122 @@ +# Telemetry comment upsert (per-instance singleton, race-converging) + +The exact upsert this lane runs to maintain its ONE sentinel-identified status comment **for this +lane instance**. `SKILL.md`'s "Telemetry and durable loop state" owns where the comment lives and +what goes in it; this file owns how the singleton is maintained, how its body is gated and verified, +and how a creation race converges. + +The upsert is inlined in this plugin rather than invoked from `claude-ops` because an installed +plugin cannot invoke a sibling plugin's scripts. + +**Resolve the lane instance first (#1295).** The marker names the *writer*, not the lane type — per +the convention's lane-instance identity rule. The id is `${user_config.lane_instance}`; a surviving +literal `${user_config.…}` placeholder means the key is unset, so fall back to the sanitized +lowercased hostname (headless-config floor: log the assumption). It is operator-supplied text about +to be interpolated into a shell string and a `jq` program, so it is validated and **rejected**, +never sanitized-and-continued. Substitute the resolved value for `` below; the check +runs **before** `MARKER` is built, because a lane that validates only in prose has documented a +guard that does not run: + +```bash +INSTANCE="" # ${user_config.lane_instance}, else `hostname` sanitized +[ -n "$INSTANCE" ] || INSTANCE="$(hostname | tr '[:upper:]' '[:lower:]' | tr -c 'a-z0-9-' '-')" +# ^[a-z0-9][a-z0-9-]{0,31}$ — empty, a leading hyphen, any other character, or +# over 32 chars is REJECTED, never trimmed into something that looks valid. +case "$INSTANCE" in +"" | -* | *[!a-z0-9-]*) + echo "telemetry: lane_instance '$INSTANCE' is not ^[a-z0-9][a-z0-9-]{0,31}\$; refusing to build a marker" >&2 + exit 1 + ;; +esac +[ "${#INSTANCE}" -le 32 ] || { + echo "telemetry: lane_instance '$INSTANCE' exceeds 32 characters; refusing to build a marker" >&2 + exit 1 +} +``` + +The hostname fallback is a *default*, not a sanitizer: the same gate validates it, so a hostname +that cannot produce a conforming id stops the lane rather than yielding a marker nobody chose. + +```bash +MARKER="work-items:work-loop@$INSTANCE" +SENT="" # $BODY_FILE MUST open with this line +LOOKUP() { gh api --paginate "repos/$REPO/issues/$ISSUE/comments" \ + --jq ".[] | select(.body | startswith(\"$SENT\")) | .id"; } +SENTINEL_OK() { # $1 = text; true iff line 1 is exactly $SENT and >=16 payload bytes follow + [ "$(printf '%s' "$1" | head -c ${#SENT})" = "$SENT" ] && + [ "$(printf '%s' "$1" | tail -n +2 | wc -c | tr -d ' ')" -ge 16 ] +} +VERIFY() { # $1 = comment id; re-read what LANDED, whatever form the write took + BACK="$(gh api "repos/$REPO/issues/comments/$1" --jq '.body' 2>/dev/null | tr -d '\r')" && + SENTINEL_OK "$BACK" +} +if [ ! -s "$BODY_FILE" ] || [ "$(head -c 1 "$BODY_FILE")" = "@" ]; then + echo "telemetry: body is empty or a literal @path - nothing written; fix the body composition, do not re-run blind" >&2 +elif ! SENTINEL_OK "$(cat "$BODY_FILE")"; then + echo "telemetry: body is not sentinel-prefixed or carries no payload - nothing written; fix the body composition, do not re-run blind" >&2 +elif ! LIST=$(LOOKUP); then + echo "telemetry: comment lookup failed; skipping upsert this cycle (fail closed)" >&2 +else + if [ -z "$LIST" ]; then + gh api -X POST "repos/$REPO/issues/$ISSUE/comments" -F body=@"$BODY_FILE" >/dev/null || true + LIST=$(LOOKUP) || LIST="" # re-list; a failure here converges next cycle + fi + CANON=$(printf '%s\n' "$LIST" | sort -n | head -n1) + if [ -z "$CANON" ]; then + echo "telemetry: no comment available to write to (a create may have landed but was not re-found) - treat the lane as UNREPORTED and carry that forward to the next cycle" >&2 + 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 and carry that forward to the next cycle; the comment holds an earlier body, not this cycle's write" >&2 + elif ! VERIFY "$CANON"; then + echo "telemetry: comment $CANON does NOT carry a well-formed telemetry body after the write - treat the lane as UNREPORTED and carry that forward to the next cycle; do not trust the timestamp" >&2 + else + for DUP in $(printf '%s\n' "$LIST" | sort -n | tail -n +2); do + gh api -X PATCH "repos/$REPO/issues/comments/$DUP" \ + -f body="Superseded duplicate - canonical telemetry comment: $CANON" || true + done + fi +fi +``` + +**`$BODY_FILE` contract.** The file's FIRST line must be exactly `$SENT`, with the cycle's telemetry +below it. The lookup matches on that prefix, so a body composed without it is not merely rejected here +— it would never be found again, and the next cycle would post a second comment. Compose the sentinel +into the file; do not rely on anything downstream to add it. + +**Body gate, write check, and read-back (encoded above, #943).** Three checks, because they catch +different failures. The **pre-write** assertions run before any API call and reject a `$BODY_FILE` +that is empty, opens with a literal `@`, is not sentinel-prefixed, or carries under 16 payload bytes +below the sentinel — the mechanical form of the `@path`-as-body rule owned by the `claude-ops` lanes +skill ("Never pass a body as an `@path` string"). 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 +**write's own exit status** is checked next: a PATCH that fails leaves the previous cycle's body in +place, which a read-back running regardless would happily accept. The **post-write** `VERIFY` then +re-reads what the write stored — the only check that sees a write which reported success and stored +something else: a mangled body, a concurrent overwrite, a deleted comment. It is also the half that +would have caught #943 itself, where the composed file was correct and the defect was the invocation +(`-f body=@FILE` transmits the literal path; this block only ever uses `-F body=@`). + +Every branch that ends without a verified body says so and skips the duplicate-supersede pass, so a +cycle whose own write is unproven never tombstones a racing session's comment. A degraded body that +does land still moves the comment's timestamp, so any consumer keying on that timestamp rather than +on the body reads the lane as **fresh** while it carries nothing — which is why a refusal, a failed +write, and a failed verification all have to be carried forward: stderr does not survive the session, +and the next cycle must see that this one did not report. A lane with durable loop state records it +there; a lane without one carries it in the cycle's own summary. + +Known limits, inherited from the wrapper: a PATCH that succeeds while storing the previous body still +verifies, and `VERIFY` asserts that *some* well-formed telemetry is present, not that *this* cycle's +write is what is present. Not replicated at all: the 64 KiB cap, the body-file containment checks, +retries, and the wrapper's distinct non-zero exit codes — every branch here exits 0 and reports +through stderr alone. + +**Creation race reconcile (encoded above).** Two sessions racing the first-ever upsert can both +see an empty lookup and both POST, forking the singleton. The upsert converges every cycle +duplicates are visible: the LOWEST comment id is canonical (numeric sort, deterministic for +every session), the canonical comment receives the current cycle's full state, and every other +sentinel comment is edited to a one-line tombstone — only once the canonical write verifies — so +it never matches a lookup again — this +covers a racer that died between its POST and its own re-list, because the NEXT session's +ordinary upsert performs the same reconcile. A crashed racer's unmerged counters are an +accepted loss (durable state re-derives over a cycle); nothing is deleted. The reconcile converges +duplicates **within one instance's own sentinel set** — a sibling instance's comment carries a +different marker and never enters `$LIST`, so it is neither canonical nor tombstoned.