Skip to content

feat(loop-lane): sample per-cycle usage into lane telemetry (measure-only) - #1719

Merged
kyle-sexton merged 11 commits into
mainfrom
feat/1651-telemetry-token-sampling
Jul 29, 2026
Merged

feat(loop-lane): sample per-cycle usage into lane telemetry (measure-only)#1719
kyle-sexton merged 11 commits into
mainfrom
feat/1651-telemetry-token-sampling

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Contributor

Phase 1 of #1651: turn each loop lane's spend from a blind spot into a measured one, by recording a
per-cycle usage_sample in the lane telemetry state block that already exists. Nothing reads the
field back. Phase 2 — a budget config key — is deliberately not designed here, and no threshold,
cap, warning, pause, backoff, pacing change, or merge-rung change derives from the sampled value.

The signal, and why it is not a token count

The issue title says "token usage". This ships window-percentage usage instead, because no token
count is readable at a lane's cycle boundary. What each lane actually records is the reading its
rate-limit guard step already took that cycle:

  • Field: rate_limits.five_hour.used_percentage / rate_limits.seven_day.used_percentage
  • Produced by: plugins/rate-limit-guard/scripts/statusline-tee.sh, which selects rate_limits
    out of the status-line stdin payload and atomically rewrites
    ~/.claude/rate-limit-guard/rate-limits.json
  • Doc: https://code.claude.com/docs/en/statuslineused_percentage is 0–100, resets_at is
    Unix epoch seconds, and rate_limits appears only for Claude.ai subscribers after the session's
    first API response, with each window independently absent. Re-verified 2026-07-28; no drift.

Because the guard step already holds this value, the invariant costs a write, not an observation.

Why not tokens. context_window.total_input_tokens / total_output_tokens are documented as
token counts currently in the context window, from the most recent API response — cumulative
session totals only before Claude Code v2.1.132. The /usage, /goal, and /workflows figures the
issue cites are interactive displays, not values a skill body can parse at a cycle step.

One signal does exist, and is recorded as deferred rather than denied. cost.total_cost_usd is
documented as the session's client-side accumulated cost, resetting on /clear — machine-readable
and session-scoped, so it would attribute to a single lane. The tee does not forward it. Widening
the tee is a rate-limit-guard change (its own bump plus a reader-contract amendment) and is out of
scope for a measure-only item, so this PR records it as the deferred candidate with its trigger
rather than claiming no signal exists.

Caveats, re-derived rather than inherited

The issue's acceptance criterion names "approximate, machine-local". Both are recorded, but
attributed to what they actually describe:

  • Approximate was a /usage caveat and does not hold for used_percentage, which the
    status-line schema documents without qualification. The real bound is the guard's staleness rule,
    so the sample is described as a snapshot no fresher than that rule allows.
  • Machine-local describes the tee file — last-writer-wins, no account identifier — not the
    reading, which is account-scope. The two are now stated separately.
  • Disclosed gap: the tee refreshes only while an interactive session renders a status line, and
    a lane is by construction a claude --bg session. An unattended lane with no interactive window
    open samples null every cycle. An empty sample therefore means unobserved, never zero. This is
    stated rather than glossed, because the framing "the reading is already in hand" otherwise hides
    it.

Scope

  • Extends the existing #502 durable-state block in both loop lanes — no second telemetry
    mechanism. plugins/claude-ops/skills/lanes/scripts/telemetry-upsert.sh is body-opaque and
    validates no schema, so it needs no change; nothing else in the repo parses the state block.
  • The schema tag stays @1: the added key is additive-optional and no reader validates the shape.
  • The attended attend-queue lane is unaffected — §4 binds loop lanes, and that lane holds no
    durable-state block.
  • plugins/autonomy/reference/ is untouched. That tree holds the OTLP telemetry contract, not the
    lane state-block shape; the shape lives in the loop-lane convention and the two lane bodies.

Versioning — renumbered as predicted

The competing numbers this PR's table anticipated all landed first, so every version here was
re-derived against origin/main at merge time rather than against this branch's original merge
base:

File Now Superseded by what landed
docs/conventions/loop-lane/CHANGELOG.md 6.0.0 #1690 took 4.0.0, #1710 took 5.0.0
plugins/source-control 0.39.0 #1690 0.35.0, #1710 0.36.0, plus 0.37.0 and 0.38.0 landing mid-merge
plugins/work-items 0.29.0 #1690 0.27.0, #1710 0.28.0

The loop-lane bump stays major on the convention's own discriminator — §4 states loop-layer
invariants and this adds one every loop-lane body must carry. The competing minor reading (a field
nothing reads back changes no behavior) is recorded in the entry per the established pattern. The
convention entry was also moved back to the top of its changelog, where the merge had left it below
the entries that landed ahead of it.

§Versioning re-derivation trigger 2 is satisfied: the status-line claim was re-verified against its
cited page and the stamp refreshed, recorded as a changelog entry. rate-limit-guard's reader
contract carries its own 2026-07-23 stamp on the same page; it is unchanged by this PR and its
refresh belongs to that plugin's own bump.

Composition with the two features that landed first

Both lane bodies now carry three loop-layer features in one cycle step and one state block: the
no-progress streak (#1648), the escalation record write (#1650), and this usage sample. Composing
them pushed babysit-loop's SKILL.md past the 500-line hard cap, so two things were tightened
without changing any contract:

  • The usage-sample prose in both lane bodies keeps the field contract (the at semantics, every
    null rule, the delta's roll-over and first-cycle cases) and cites §4 for the single permitted
    readback, the preceding-interval delta, and the three bounding properties — all of which §4
    already states in full. §4's own wording changed from "recorded alongside the sample in each lane
    body" to "held by citation from each lane body" to match.
  • babysit-loop's telemetry upsert mechanics — the singleton lookup and the creation-race reconcile
    — moved to reference/telemetry-upsert.md, beside the pre-escalation-dispatch.md and
    no-progress-detector.md spokes already there. Still inlined in this plugin rather than invoked
    from claude-ops, for the same reason as before.

Verification

Gates green after the merge: check-changed-skills.sh (babysit-loop 486/500 and work-loop 451/500
against the hard 500-line cap, both re-measured with all three features composed),
check-changelog-parity.sh --check and --check-bump origin/main, and markdownlint over every
changed markdown file. No evals.json is touched.

A fresh-context verifier audited the diff with the rationale withheld and traced the tee's producer
and the live status-line doc independently. Its findings drove the cumulative-cost correction, the
caveat re-attribution, the background-session disclosure, and the removal of a cited precedent that
did not exist. Its remaining version-collision finding is the table above.

Related

Closes #1651

🤖 Generated with Claude Code

kyle-sexton and others added 5 commits July 27, 2026 22:34
Both loop lanes now record a `usage_sample` in the durable-state block they
already upsert every cycle, holding the two subscription-window percentages the
rate-limit guard step already reads plus the rise since the previous sample.

The field is deliberately inert: no lane reads it back and no pacing, backoff,
adaptive cap, merge rung, or pause derives from it. Measuring first is the
point; whether the data supports acting on it is a separately decided question.

Caveats are recorded beside the sample because they bound what the data can
support: the readings are approximate, machine-local, account-scope, and a
percentage of a subscription window rather than a token count.

Closes #1651
…nt (#1651)

Adds the loop-lane convention changelog entry for the measure-only per-cycle
usage sample and reconciles the README's caveat count with the lane bodies,
which already record three bounding properties.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…am (#1651)

The absolute claim that no in-session signal exposes cumulative token spend
overreached: it is readability at a cycle boundary that is missing, not the
existence of any display. Narrows the claim in the convention and both lane
changelogs to what was checked.

Records the §Versioning trigger-2 re-verification against the status-line stdin
schema and refreshes the stamp to 2026-07-28 (no drift).

Makes `at` unconditional in the sample so an unobservable cycle stays distinct
from an unsampled one, and states why only the five-hour window carries a delta.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
)

The bumps skipped a minor (0.33.2 -> 0.35.0, 0.25.4 -> 0.27.0), which reads as
coordinating around other in-flight PRs' numbering. Loop-lane changelog entries
are numbered against their own merge base only; the next minor from this
branch's merge base is 0.34.0 (source-control) and 0.26.0 (work-items).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01JifsqDc7vY8NfwBB1fi8sE
…source (#1651)

Independent verification refuted three claims:

The assertion that every surface reporting cumulative spend is an interactive
display is false. `cost.total_cost_usd` is machine-readable on the same
status-line payload the guard's tee already receives, and is session-scoped, so
it would attribute to a lane. It is recorded as the deferred candidate; the
reason a lane cannot read it today is that the tee does not forward it, which is
a guard-side change this item does not make.

"Approximate" was imported from `/usage` and never held for
`rate_limits.used_percentage`, which the status-line schema documents without
qualification. The real bound is the guard's staleness rule, so the caveat now
names that. "Machine-local" described the tee file, not the reading, while the
next clause correctly called the reading account-scope; the two are now
attributed separately.

Discloses the gap the sample's framing hid: the tee refreshes only while an
interactive session renders a status line, and a lane is by construction a
background session, so an unattended lane samples null every cycle. An empty
sample means unobserved, never zero.

Also drops a no-progress-detector precedent that exists nowhere in this
changelog, and restores the established bump-ambiguity paragraph.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 1s —— View job


I'll analyze this and get back to you.

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 43ff0aac46

ℹ️ 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 docs/conventions/loop-lane/README.md Outdated
Comment thread plugins/work-items/skills/work-loop/SKILL.md Outdated
…sline source (#1651)

An independent pre-merge audit read "both null when the guard is not proactive" as
implying that a proactive guard guarantees a reading. It does not: a window is
independently null when the tee file is absent or stale, when the window is absent
upstream, and when the guard's reader contract rejects the window as unknown. The
last case mattered most — "the readings as taken" could be read as writing a value
the guard had already rejected. Both lane bodies now state all three.

The README paragraph asserting current-context token occupancy and a session-scoped
cumulative cost field carried no inline citation, while the neighbouring paragraphs
stamp exactly such claims inline. It now matches that pattern.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

…its delta attribution (#1651)

Review found two contradictions in the invariant as staged.

The invariant required `five_hour_delta_pct` to be derived from the previous
`usage_sample` while the same text said no lane reads the field back. Both cannot
hold: the telemetry block is the durable source across compaction, so a lane
obeying the prohibition literally could not obtain the previous percentage and
would have to emit null or fabricate the delta. The boundary is now stated as a
closed operation rather than a blanket prohibition — the previous sample is read
back for exactly one thing, subtracting its `five_hour_pct` to compute the new
sample's delta, and that derivation is the field's only permitted consumer. The
value still never reaches a decision: not pacing, backoff, an adaptive or item
cap, a merge rung, admission, escalation, a warning, or a pause, at any threshold,
in a lane or in any gate a lane runs. Enumerating the forbidden consumers is what
keeps the carve-out from reading as permission to act on the value.

The guard reading is taken during Re-anchor, before intake and execution, but the
field was documented as "this cycle's" sample. The delta recorded for cycle N
therefore measured the interval since cycle N-1's start and excluded cycle N's own
execution. Of the two remedies, re-attribution is taken rather than a
post-execution reading: a second reading would be an observation of the guard's
tee that no lane's cycle shape performs, requiring the reader contract's staleness
and per-window capability logic to run again, which contradicts the invariant's
own justification that the reading is already in hand and costs only a write. The
field's documented meaning now says so — `at` is the cycle-start observation time,
the delta covers the interval preceding the cycle whose report carries it, and
that cycle's own consumption lands in the next cycle's sample.

Both clarifications are stated standalone in the README and in both lane bodies so
the three cannot drift, and the existing 4.0.0 changelog entry is amended rather
than superseded. "Inert" is retired wherever it described the field: it is now
false, since the field is read once, narrowly. No enforcement, threshold, or
consumer is introduced; measure-only still holds.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

Claude encountered an error after 0s —— View job


I'll analyze this and get back to you.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


PR Review: feat(loop-lane): sample per-cycle usage into lane telemetry (measure-only)

  • Gather context and understand PR scope
  • Read changed files in detail
  • Review loop-lane convention changes
  • Review babysit-loop and work-loop skill changes
  • Review plugin version bumps and changelogs
  • Assess Codex review findings
  • Post comprehensive review

This is a well-structured, carefully documented PR. The changes are purely specification/documentation (SKILL.md files are natural-language instructions for LLM lane sessions, not executable code), so the review focuses on internal consistency, correctness of the documented contract, and alignment with the repo's conventions.

Codex P2 findings — already addressed

Both automated findings are handled in the PR text:

"Permit readback when deriving the delta" — The PR explicitly names the one permitted readback in all three sites. Convention README lines 238–243: "The one permitted readback. The previous cycle's sample is read back for exactly one operation: subtracting its five_hour_pct to compute the new sample's five_hour_delta_pct. That derivation is the field's only permitted consumer." Both lane bodies mirror this exactly. No ambiguity remains.

"Sample after execution before attributing usage to this cycle" — The PR explicitly chose the cycle-start reading and documented the tradeoff. The CHANGELOG (lines 38–40) records the rejection of post-execution sampling: "The alternative — a post-execution reading — was rejected because it would be a second observation of the guard's tee that no lane's cycle shape performs, contradicting the invariant's own justification that the reading is already in hand." The lagging nature is disclosed as an explicit invariant ("Read the series as a lagging one") in both the convention and lane bodies.

Convention-to-lane consistency

The convention (§4) and both lane body implementations are consistent. Differences between the two lane bodies are appropriate to each lane's decision surface:

  • babysit-loop lists "pacing, backoff, a tier or merge rung, admission, escalation, a warning, or a pause" (SKILL.md line 382)
  • work-loop lists "pacing, the adaptive item cap, admission, escalation, a warning, or a pause" (SKILL.md line 130)

Both lists correctly substitute the lane-specific decision surfaces while preserving the measure-only invariant. JSON schema examples are byte-identical across lanes.

Null conditions and delta semantics

The delta null conditions are sound:

  • First cycle: always null (no prior sample) ✓
  • Window rollover (current < previous): null rather than a negative ✓
  • Either pct unreadable/absent: null
  • Guard not proactive: both pcts null, delta implicitly null
  • Seven-day window carries no delta (moves too little per cycle to clear approximation noise) ✓

One observation worth noting: when a five-hour window resets, the cycle immediately after the reset will produce a null delta (current < previous). This means one cycle's worth of consumption is unattributed in the series at each window boundary. This is an acceptable limitation and fits within the "lagging measurement, measure-only" framing, but it could be worth stating explicitly in the caveats if a future phase tries to aggregate deltas.

Versioning rationale

The major bump (3.0.0 → 4.0.0 for the convention, minor bumps for the two plugins) is correct under the convention's own rule: "a change to... any loop-layer invariant is a major bump." Adding a new §4 invariant every loop lane must implement is a new obligation, which the "Bump ambiguity" section in the changelog correctly distinguishes from a purely additive field. The attend-queue carve-out (§4 binds loop lanes, and that lane holds no durable-state block) is accurate.

The version collision table is appropriately disclosed. This is a process issue, not a correctness one.

Minor observations

  1. at always written: The design that at is always stamped (even when pcts are null) is a good choice — it lets consumers distinguish "unobserved" from "never sampled" (babysit-loop SKILL.md line 374, work-loop SKILL.md line 122).

  2. schema tag stays @1: Correct — the added key is additive-optional and nothing validates the shape. No schema bump needed.

  3. Background-session disclosure: The explicit disclosure that unattended lanes sample null every cycle (convention README lines 250–252) is the most important caveat in this PR, and it's placed prominently in all three sites. This prevents misreading an empty usage_sample as "zero usage."

  4. Deferred cost field: Recording cost.total_cost_usd as the deferred candidate with its specific trigger (a tee-widening guard-side change) is a good pattern — it removes the "no signal exists" false conclusion without committing this PR to the scope.

Summary

No correctness issues found. Both Codex P2 findings are already resolved in the PR's documentation. The convention, both lane bodies, and both changelogs are internally consistent. The major version bump rationale is sound under the convention's own rules. The measure-only constraint is stated unambiguously in all three implementation sites with the correct decision surface list per lane. Ready to merge pending version collision resolution with sibling PRs.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather PR diff and changed files
  • Read changed skill/script files for security issues
  • Analyze for logic flaws, injection surfaces, secret handling, authorization gaps
  • Report findings

No security issues found.

This PR is exclusively documentation — CHANGELOG.md entries, README.md additions, SKILL.md convention prose, and plugin.json version bumps. No new scripts, hooks, or GitHub Actions are introduced. All security-relevant infrastructure (statusline-tee.sh, telemetry-upsert.sh) is unchanged.

The data-flow security posture was reviewed end to end:

Tee file as data source. ~/.claude/rate-limit-guard/rate-limits.json is written with a chmod 700 directory and umask 077 file, so only the owner can write it. All values — including used_percentage and captured_at — are produced by jq from the statusline stdin payload, guaranteeing valid JSON numbers and timestamps regardless of the raw input. A tampered file can at most produce an unknown guard mode (the reader contract already validates that an out-of-range used_percentage makes that window unknown), not a code-execution or decision-path change.

Injection surfaces. five_hour_pct / seven_day_pct are JSON numbers (0–100); at comes from date -u. None of these can carry shell metacharacters through the jq pipeline. The telemetry-upsert script already enforces: --marker constrained to [A-Za-z0-9:._-]+, --repo validated as owner/repo before URL interpolation, --body-file path-canonicalized under a safe directory with symlink rejection, and the body posted via gh api -f body=... (no shell interpolation of the body). None of those protections are relaxed by this PR.

Secret / credential exposure. usage_sample contains only rate-limit percentages (0–100%) and a cycle-start timestamp. No credentials, tokens, or cost figures are included; the PR explicitly defers cost.total_cost_usd as a future guard-side change. The tee file has no account-identifier field (a known documented gap), and that gap is recorded but not worsened here.

One-permitted-readback authorization. The convention's "measure-only" constraint prohibits any decision — pacing, backoff, merge rung, pause — from deriving from the sampled value. The only permitted readback (five_hour_pct for delta arithmetic) produces null on any missing or non-numeric value; a human editing the telemetry comment to inject a malformed value cannot escalate beyond a null delta.

Reader contract classification. The reader contract correctly distinguishes used_percentage (Claude API–sourced, validated numeric) from session_id/session_name/account fields (user/AI-influenced, explicitly untrusted). The new sample fields inherit the numeric-validation path already specified in the capability-detection table; no gap in the trust classification is introduced.

kyle-sexton and others added 2 commits July 29, 2026 14:33
main landed #1650 and #1648, taking convention 4.0.0/5.0.0,
source-control 0.36.0 and work-items 0.28.0 — the renumber this PR's
body predicted. This branch's entries compose above them (convention
6.0.0, source-control 0.37.0, work-items 0.29.0), and the convention
entry moves back to the top of its changelog. The step 6 and state-block
conflicts compose all three features: the no-progress streak update, the
escalation record write, and the usage sample now sit in one step and
one state block.

babysit-loop's SKILL.md crossed the 500-line hard cap with the third
feature in it. The usage-sample prose in both lane bodies now keeps the
field contract and cites §4 for the readback rule, the delta semantics,
and the three bounding properties the convention already states in full;
the telemetry upsert mechanics move to a spoke beside the existing two.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
main landed source-control 0.37.0 while this merge was in flight, which
collided with the number this branch had just taken. The branch's entry
moves to 0.38.0 in both the changelog and the manifest, and the two
entries the union had collapsed under one heading are split back apart.

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

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


PR Review: feat(loop-lane): sample per-cycle usage into lane telemetry (measure-only)

  • Gather context and understand PR scope
  • Read changed files via git diff
  • Review loop-lane convention changes (README.md + CHANGELOG.md)
  • Review babysit-loop SKILL.md changes
  • Review work-loop SKILL.md changes
  • Review new telemetry-upsert.md reference file
  • Review plugin version bumps and changelogs
  • Assess Codex P2 findings against actual diff
  • Post comprehensive review

Findings

1. Undisclosed scope: upsert script moved to reference/telemetry-upsert.md (medium)

The PR body's scope section says: "Extends the existing #502 durable-state block in both loop lanes — no second telemetry mechanism." However the babysit-loop diff does more than that: it extracts the previously-inline upsert script from SKILL.md body into plugins/source-control/skills/babysit-loop/reference/telemetry-upsert.md.

This matters operationally. Before this PR, the singleton upsert script (with its creation-race reconcile) was directly embedded in the babysit-loop SKILL.md — unavoidably in context when the lane executes step 6. After this PR, the SKILL.md says:

The upsert itself … is owned by reference/telemetry-upsert.md.

A lane session now has to notice the link and read the reference file before executing the upsert. The cycle-shape step 6 instruction (SKILL.md line ~294) says "upsert the telemetry comment" but does not say "first read reference/telemetry-upsert.md." An AI following the cycle shape literally could improvise the upsert rather than using the race-converging implementation.

Asymmetry with work-loop: work-loop's equivalent upsert script remains inline in its SKILL.md (lines 76–104), with no equivalent reference-file extraction. The same contract is now enforced by two different operational patterns.

If this extraction is intentional, the PR body should describe it and the cycle-shape step 6 should be updated to explicitly say "read and follow reference/telemetry-upsert.md." If it's coincidental scope, the script should remain inline in SKILL.md (matching work-loop's pattern).

Fix this →

2. "Inlined in this plugin" is misleading after the extraction (minor)

SKILL.md lines ~361–362:

It is inlined in this plugin (never invoked from claude-ops) because an installed plugin cannot invoke a sibling plugin's scripts.

"Inlined in this plugin" now reads as the old state (script embedded in SKILL.md body). The script is now in reference/telemetry-upsert.md, so "inlined" means "bundled within this plugin's file tree" — not "embedded in this document." The old comment was accurate when the script was literally in the file; after the extraction, a reader who finds this phrase before finding the reference link could be confused about where the script lives.

Suggest: "The upsert script lives in reference/telemetry-upsert.md within this plugin (never invoked from claude-ops) because an installed plugin cannot invoke a sibling plugin's scripts."

3. PR body version table is stale (informational)

The PR body's collision table says "This PR: 4.0.0" for the loop-lane convention and cites 0.34.0/0.26.0 for the two plugins. The actual diff shows 6.0.0, 0.38.0, and 0.29.0 respectively. This reflects the two Merge origin/main commits in the branch history (which incorporated the sibling PRs that landed) — the PR body was written before those merges. The changelog entries use the correct post-merge version numbers; only the PR description prose is stale.

Codex P2 findings — addressed

Both automated findings are resolved:

"Permit readback when deriving the delta" — The convention README (~line 474) names the single permitted readback explicitly ("read back for exactly one operation: subtracting its five_hour_pct to compute the new sample's five_hour_delta_pct. That derivation is the field's only permitted consumer"). Both lane SKILL.md files mirror this verbatim.

"Sample after execution before attributing usage to this cycle" — The PR chose the cycle-start reading deliberately and named the tradeoff. The CHANGELOG entry records the rejection of post-execution sampling ("a second observation of the guard's tee that no lane's cycle shape performs, contradicting the invariant's own justification"). The lagging nature is disclosed as an explicit invariant in all three sites.

Correctness of sampling logic

Null conditions are complete and sound:

  • First cycle: five_hour_delta_pct always null
  • Window rollover (current < previous): null
  • Guard not proactive: both pcts null, delta implicitly null
  • Seven-day window: no delta (moves too little per cycle) ✓
  • at always written (distinguishes "unobserved" from "never sampled") ✓

The background-session null disclosure — an unattended lane samples null every cycle — is the most important caveat and appears prominently in all three changed sites (convention + both lane bodies).

Version bump rationale

6.0.0 is correct under the convention's own discriminator: a new §4 invariant every loop-lane body must implement is a new obligation → major bump. The changelog's "Bump ambiguity" section records and resolves the counter-reading (additive field → minor). Plugin versions 0.38.0 / 0.29.0 are correct minor bumps. Schema stays @1 (additive-optional, nothing validates shape).


Summary

One medium finding: the upsert script extraction is undisclosed scope that changes babysit-loop's operational pattern (requires a reference-file read vs. having the script directly in context) and creates asymmetry with work-loop. If the extraction is intentional, step 6 of the cycle shape should explicitly direct the AI to reference/telemetry-upsert.md; if not, the script belongs inline as in work-loop. The minor wording issue ("inlined") is a secondary cleanup. All other content — convention accuracy, delta semantics, null conditions, measure-only constraint — is correct and internally consistent across the three sites.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


Security Review — PR #1719

  • Fetch PR diff and identify changed files
  • Read changed files (SKILL.md lanes, reference script, convention README, plugin.json, changelogs)
  • Analyze data-flow, trust boundaries, injection surfaces, decision-path authorization
  • Post findings

No security issues found.

This PR is almost entirely documentation and convention prose. The only change with any executable surface is the relocation of an existing bash script from inline in babysit-loop/SKILL.md to a dedicated reference file, plus instructional text adding a new usage_sample field to the telemetry state block. All surfaces were reviewed.

Scope of review

Nine changed files: two convention docs, one new reference file (reference/telemetry-upsert.md), two SKILL.md instruction bodies, two plugin.json version bumps, and two changelogs. No GitHub Actions workflows were touched. No new scripts, hooks, MCP servers, or CI triggers are introduced.

Script relocation — unchanged attack surface

reference/telemetry-upsert.md contains the bash block that was previously inlined in babysit-loop/SKILL.md. A diff of the two confirms the content is byte-identical. The security posture of the script is therefore unchanged — the prior security review's analysis of $REPO/$ISSUE URL interpolation, $BODY_FILE via @-file (no shell interpolation of body content), the $SENT sentinel in the jq filter (hardcoded $MARKER, no jq injection path), and the numeric comment-ID iteration all still apply without change.

usage_sample field and the delta readback

The new usage_sample field copies values the rate-limit guard step already holds at cycle start (five_hour_pct, seven_day_pct). These values flow from ~/.claude/rate-limit-guard/rate-limits.json (written by statusline-tee.sh via jq, validating them to JSON numbers before writing). The path to the telemetry comment body is: validated numeric → embedded in $BODY_FILE → posted via gh api -F body=@"$BODY_FILE". No shell interpolation of the values occurs; gh api -F reads file contents verbatim.

The five_hour_delta_pct computation reads back the previous cycle's five_hour_pct from the durable state comment. This adds one field to the existing readback pattern that already applies to cycle, backoff_level, no_progress_streak, stop_mode, tier, and merge_rung. Those fields drive real decisions (backoff, tier, merge rung). The new field drives none — the measure-only constraint (README.md lines 473–476) prohibits the value from reaching any decision at any threshold. An attacker who could modify the telemetry comment to inject a crafted five_hour_pct could already modify merge_rung or tier, which carry far greater impact. The usage_sample readback strictly extends an existing, acknowledged attack surface in a less dangerous direction — no escalation in risk.

The instruction to treat non-numeric or out-of-range values as null (SKILL.md lines 371–376) is the same pattern already used for all guard-mode and rate-limit fields, and is sufficient given the zero-decision-path constraint.

Information exposure via telemetry comment

usage_sample adds subscription window percentages (0–100 numeric, no secrets, no credentials, no account identifiers) to what is already a public-ish durable-state comment on a repo issue. The prior review documented that the tee file has no account-identifier field (a known gap). This PR does not widen that gap.

main took source-control 0.38.0; this branch's entry moves to 0.39.0 in
both the changelog and the manifest, with the two entries kept separate
under their own headings.

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

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


PR Review: feat(loop-lane): sample per-cycle usage into lane telemetry (measure-only)

  • Gather context and read changed files
  • Review convention changes (README.md, CHANGELOG.md)
  • Review babysit-loop SKILL.md and new telemetry-upsert.md
  • Review work-loop SKILL.md
  • Review plugin version bumps and changelogs
  • Assess prior review findings and post comprehensive review

New Finding: CHANGELOG ordering incorrect (medium)

docs/conventions/loop-lane/CHANGELOG.md has the 3.1.1 entry misplaced between 6.0.0 and 5.0.0:

Actual order Correct semver order
6.0.0 6.0.0
3.1.1 ← wrong position 5.0.0
5.0.0 4.0.0
4.0.0 3.1.1
3.1.0 3.1.0

The 3.1.1 entry (docs-only, dated 2026-07-29) should sit between 4.0.0 and 3.1.0. It appears between 6.0.0 and 5.0.0 as a merge conflict resolution artifact — the PR body notes the 6.0.0 entry "was moved back to the top of its changelog, where the merge had left it below the entries that landed ahead of it," but the 3.1.1 entry that preceded 5.0.0 in the branch's original view was not repositioned when the conflict was resolved.

Fix this →


Prior finding confirmed: step 6 doesn't reference telemetry-upsert.md (medium)

This finding from the earlier review at 18:35 holds. babysit-loop/SKILL.md line 294–297 (cycle-shape step 6) says "upsert the telemetry comment" without directing the AI to reference/telemetry-upsert.md. The reference is in the "Telemetry and durable loop state" section (line 359), which appears later in the file — an AI following the cycle shape narrowly would need to scan past step 6 to find it, and might otherwise improvise the race-converging singleton logic.

By contrast, work-loop SKILL.md line 72 says "The upsert is inlined here" (with the script literally present inline), so work-loop's cycle shape step 6 is self-contained. The babysit-loop pattern should be equally explicit in the step itself: adding "(see reference/telemetry-upsert.md for the singleton upsert protocol)" to step 6 would close the gap.

Fix this →


Prior finding confirmed: "inlined in this plugin" wording (minor)

babysit-loop/SKILL.md line 361–362: "It is inlined in this plugin (never invoked from claude-ops) because an installed plugin cannot invoke a sibling plugin's scripts." The word "inlined" reads as "embedded in this file," which was true before the extraction to reference/telemetry-upsert.md. Now it means "bundled within this plugin's file tree" — a distinct thing. Work-loop's equivalent at line 72 says "inlined here" with the script literally present, making the contrast noticeable. Suggest: "It is bundled within this plugin (never invoked from claude-ops)…"


Plugin version table in PR body (informational)

The PR body's collision table lists source-control "Now: 0.38.0" but the actual diff bumps from 0.38.00.39.0 (both plugin.json and CHANGELOG.md show 0.39.0). The table was written when 0.38.0 was still the intended version; a later PR took that slot. The changelogs and manifests are correct — this is stale PR description prose only.


What's correct

  • usage_sample null rules are complete and consistent across both lane bodies and the convention: first cycle, window rollover, guard not proactive, window absent/unreadable — all produce null; at is always written to distinguish "unobserved" from "never sampled". ✓
  • Convention §4 wording is accurately cited rather than restated in both lane bodies; the JSON examples are structurally consistent. ✓
  • Single permitted readback (previous five_hour_pct for delta arithmetic) is stated unambiguously in the convention and both lane bodies, with the correct null-on-lower-reading rollover guard. ✓
  • Version bumps: 6.0.0 for loop-lane convention is correct (new §4 obligation = major under the convention's own rule). 0.39.0 / 0.29.0 are correct minor bumps for the two plugins. Schema stays @1 (additive-optional, no reader validates the shape). ✓
  • Background-session disclosure ("an unattended lane samples null every cycle, and an empty sample means unobserved rather than zero") appears in all three sites and is the most important caveat in the PR. ✓
  • Deferred cost field (cost.total_cost_usd) documented as the future candidate with its specific trigger (tee-widening guard-side change), correctly scoped out of this PR. ✓
  • Codex P2 findings are pre-addressed: the one permitted readback is explicit in all three sites; the cycle-start lagging read is disclosed as an explicit invariant with the rejection of post-execution sampling recorded in the CHANGELOG. ✓

Summary

One new medium finding: the loop-lane CHANGELOG has 3.1.1 misplaced between 6.0.0 and 5.0.0 — it should sit between 4.0.0 and 3.1.0. One prior medium finding confirmed: babysit-loop's cycle-shape step 6 should explicitly reference reference/telemetry-upsert.md to match the self-contained pattern in work-loop. Both are mechanical fixes. The measure-only constraint, delta semantics, null rules, and version rationale are all correct.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


Security Review — PR #1719

  • Fetch PR diff
  • Read changed files (convention docs, SKILL.md bodies, reference/telemetry-upsert.md, changelogs, plugin.json bumps)
  • Analyze security surfaces
  • Post findings

No security issues found.

This PR is nine changed files: two convention documents (changelog + README), two SKILL.md instruction bodies, one new reference file, two changelogs, and two plugin.json version bumps. No GitHub Actions workflows or CI configuration is touched. The only executable surface is the bash block in reference/telemetry-upsert.md.

Script relocation — zero change to attack surface

reference/telemetry-upsert.md contains the upsert bash block moved verbatim from the previous inline position in babysit-loop/SKILL.md. A line-level diff confirms no code changed. The script's security properties are unchanged from prior reviews:

  • $REPO, $ISSUE, and $BODY_FILE are lane-owned variables set at initialization, not read from untrusted external input.
  • The jq filter (startswith("$SENT")) uses a hardcoded sentinel; $MARKER and $SENT are compile-time literals, not dynamic values — no jq injection path.
  • $CANON and $DUP come from jq .id (JSON integer fields), passed through sort -n. No non-numeric value can reach the gh api … /comments/$CANON URL position.
  • gh api -F body=@"$BODY_FILE" reads the file content; the body is never shell-interpolated.
  • The tombstone replacement (-f body="Superseded duplicate…") is a literal string with no user-controlled content.

usage_sample readback — bounded and decision-free

The one new trust-boundary crossing is reading five_hour_pct from the previous cycle's durable state block (a GitHub issue comment) to compute five_hour_delta_pct = current_pct − prev_pct. Several constraints bound any exploitation:

  1. Zero decision authority. The convention (README.md lines 468–476) and both lane bodies state explicitly that the value never reaches pacing, backoff, merge rung, admission, escalation, warning, or pause — at any threshold, in any gate. The delta is written back to telemetry and consumed nowhere else. An injected five_hour_pct produces an anomalous delta that is immediately overwritten by the next cycle's live reading.

  2. Validation floor. Non-numeric, absent, or out-of-range values resolve to null; the delta itself resolves to null when either sample is missing or when the computed value is negative (rollover guard). A crafted injection cannot produce a negative or non-numeric value that propagates — only null.

  3. No new escalation relative to prior state. Anyone with write access to the telemetry issue comment could already modify merge_rung, tier, or backoff_level in the same durable state block — all of which drive real decisions. The usage_sample readback extends an acknowledged attack surface in strictly less-impactful direction. The prior security review's conclusion stands: the more dangerous fields already existed, and usage_sample is not a lever.

Information exposure

usage_sample records subscription window percentages (0–100, no credentials, no account identifiers, no cost figures). The PR explicitly defers cost.total_cost_usd as a future guard-side change; no cost data is included. The tee file's absence of an account identifier is a documented gap, not widened here.

@kyle-sexton
kyle-sexton merged commit 54b5ba2 into main Jul 29, 2026
32 of 33 checks passed
@kyle-sexton
kyle-sexton deleted the feat/1651-telemetry-token-sampling branch July 29, 2026 18:48
kyle-sexton added a commit that referenced this pull request Jul 29, 2026
…hangelog order (#1758)

## Why

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

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

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

## What changed

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

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

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

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

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

## Deliberately not done

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

## Verification

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

## Related

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

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

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.

Sample per-cycle token usage into lane telemetry (measure before enforcing)

1 participant