Skip to content

fix(loop-lane): scope the permitted readback by purpose and correct changelog order - #1758

Merged
kyle-sexton merged 1 commit into
mainfrom
fix/loop-lane-readback-contradiction
Jul 29, 2026
Merged

fix(loop-lane): scope the permitted readback by purpose and correct changelog order#1758
kyle-sexton merged 1 commit into
mainfrom
fix/loop-lane-readback-contradiction

Conversation

@kyle-sexton

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

Copy link
Copy Markdown
Contributor

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)

…hangelog order

The per-cycle usage-sample invariant shipped self-contradictory. It permitted
reading the previous sample back "for exactly one operation: subtracting its
five_hour_pct", then forbade every other read — while the same invariant
withholds a delta when the window rolled over, which requires a second
comparison against that previous reading. No lane could satisfy both clauses.
The consuming skills now bind to the convention by citation, so the
contradiction had a single authoritative site.

Scope the permission by purpose rather than by operation: deriving
five_hour_delta_pct, covering the subtraction and the rollover comparison
together. The measure-only guarantee is unchanged — the value still reaches no
decision at any threshold.

Also corrects two narrower defects in the same surface: `at` was ambiguous
between the lane's tee read and the snapshot's own captured_at, and the delta's
null condition ("either sample is missing") excluded a present sample carrying a
null five_hour_pct.

Separately, the loop-lane changelog carried a version regression. The #1638
entry was authored against 3.1.0 and merged as 3.1.1 after 4.0.0 had landed,
leaving a descending-order file out of order. Renumbered 4.0.1 and repositioned
below 5.0.0, preserving both version order and shipping order; its wording is
unchanged. The two plugin changelog entries that described the field as
"deliberately inert" have already shipped and are left as published, superseded
by the new entries rather than rewritten.

Refs #1651

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

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


PR Review

  • Gather context and read changed files
  • Review convention docs (loop-lane CHANGELOG + README)
  • Review skill files (babysit-loop, work-loop)
  • Review plugin manifests and plugin CHANGELOGs
  • Assess correctness, security, and CLAUDE.md alignment
  • Post final review

Overall assessment

The fix is correct, logically complete, and consistently applied across all eight files. The contradiction in 6.0.0 was real — "exactly one operation: subtracting" and "No other read is permitted" made the rollover comparison (also a read of previous.five_hour_pct) technically non-compliant, even though both lanes already did it. Scoping the permission by purpose rather than operation resolves that cleanly without weakening the measure-only guarantee.

Three observations worth considering, none of them blockers:


1. The 6.0.0 CHANGELOG entry was retroactively edited

The diff modifies the 6.0.0 entry in docs/conventions/loop-lane/CHANGELOG.md to replace the erroneous "one operation: subtracting" wording with the corrected "one purpose: deriving" language. The 6.0.1 entry then documents this correction.

The result is self-consistent: the 6.0.0 entry now describes what 6.0.0 intended, and 6.0.1 explains what had to be corrected. However, the 6.0.0 entry no longer represents what actually shipped at that version — if anyone needs to verify what text was in the file at the 6.0.0 merge, git history is the reliable source, not the CHANGELOG entry itself.

This is a common tradeoff in self-describing changelog files (correctness of the historical record vs. readability of the live file), and the choice here is reasonable given the 6.0.1 entry's full explanation. Just worth flagging explicitly, since the PR description doesn't call it out.


2. Sentence structure in the skill files is slightly awkward

In both babysit-loop/SKILL.md and work-loop/SKILL.md, the new prose reads:

five_hour_delta_pct is null whenever either side's five_hour_pct is unavailable … — and null when the current reading is lower than the previous one (the window rolled over);

The double null ("is null whenever … and null when") reads a bit clunkily. Compare to the cleaner convention doc in README.md §4, which handles the same cases without repeating the word. Minor, but worth smoothing if the text is touched again.


3. Versioning tier: 6.0.1 vs. 6.1.0 or 7.0.0

The loop-lane versioning rule is: "a change to … any loop-layer invariant is a major bump." The §4 "Per-cycle usage sample" invariant's text was changed. The PR authors this as 6.0.1 (patch) on the rationale that the invariant's intent is unchanged and this resolves a self-contradiction rather than altering behavior.

That reading is defensible — the invariant was unimplementable as written, and correcting a logical error is distinct from adding or altering an obligation. And the rule has no explicit carve-out for corrections, so some ambiguity is baked in (comparable to the bump-ambiguity discussions already documented in 3.1.0 and 2.0.0 entries). The 6.0.1 choice is reasonable; recording the rationale in the CHANGELOG entry (as this PR does) is exactly the right pattern for those cases.


Security / CLAUDE.md alignment: No concerns. The changes are entirely prose and version metadata — no code execution, no egress, no secrets, no hardcoded paths. Version bumps in both plugin.json files are properly scoped to the changed plugins. The convention ownership model (shared contract lives outside both plugins) is respected throughout.

@claude

claude Bot commented Jul 29, 2026

Copy link
Copy Markdown

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


Security Review

  • Gather PR diff and changed files
  • Read changed skill files for injection surfaces / unsafe patterns
  • Read plugin.json changes for permission/config widening
  • Analyze data-flow and trust boundaries in convention changes
  • Post findings

No security issues found. Analysis by area:

Readback invariant scope change (docs/conventions/loop-lane/README.md)

The core change widens the permitted readback from "exactly one operation: subtracting" to "exactly one purpose: deriving five_hour_delta_pct (subtraction + rollover comparison)." The measure-only guarantee — that the value never reaches a decision at any threshold, in a lane or in any gate a lane runs — is preserved verbatim and explicitly called out as unchanged in the PR body.

The rollover comparison introduced into the permitted scope only determines whether five_hour_delta_pct is written as null or a numeric value. Null vs. non-null on a measure-only field carries no more decision authority than the value itself; both states are inert to every decision gate the convention lists (pacing, backoff, cap, rung, admission, escalation, warning, pause). No loophole is opened.

at timestamp clarification (SKILL.md × 2)

The change distinguishes at (the lane's own observation timestamp) from captured_at (the tee file's own field, which the staleness rule permits to lag). This is a precision clarification. captured_at is used only in a staleness comparison (arithmetic on a timestamp) and is never interpolated into a shell command or passed to another interpreter — it does not add an injection surface. The "Untrusted fields" rule in the inlined rate-limit guard floor (covering session_id, session_name, and future account fields) applies to a disjoint set and is unaffected.

five_hour_delta_pct null condition broadening (SKILL.md × 2)

The null condition widens from "either sample is missing" to "either side's five_hour_pct is unavailable" — catching a present sample with a null field value that the old phrasing excluded. This narrows the signal (more cycles produce null), which is the conservative direction. No decision gate branches on null vs. non-null.

Plugin manifests (plugin.json × 2)

Pure version bumps (0.40.1 → 0.40.2, 0.30.1 → 0.30.2). No new userConfig keys, no permission grants, no URL changes, no capability widening of any kind.

Injection surface audit

The telemetry upsert shell script inlined in work-loop/SKILL.md (lines 76–95) is unchanged by this PR. $REPO and $ISSUE in that script are lane-configuration values, not derived from tee-file or item content. The rate-limit tee fields read in this PR's scope are consumed as JSON and never shell-interpolated.

GitHub Actions

No workflow files are touched. Zizmor's static checks are not triggered.

@kyle-sexton
kyle-sexton merged commit f4d43ce into main Jul 29, 2026
32 of 33 checks passed
@kyle-sexton
kyle-sexton deleted the fix/loop-lane-readback-contradiction branch July 29, 2026 23:58
kyle-sexton added a commit that referenced this pull request Jul 30, 2026
## Why

#1758 renumbered a `3.1.1` entry that had reached `main` sitting *below*
`4.0.0`. This gate is the
reason that could happen at all, and it closes it.

The entry was authored against `3.1.0`, and merged (#1686, `17:46:59Z`)
after `4.0.0` had already
landed (`17:44:23Z`). Its number was a regression the instant it merged.
Nothing caught it, because
**no gate reads a changelog as a sequence**:

- `--check` asks whether a versioned plugin has a changelog *at all*.
- `--check-bump` asks whether this change set added an entry for *its
own* new version.

Both reason about one version in isolation, so neither can see that a
branch staged a number already
behind `main`, or that two branches staged the same one. A reviewer
cannot see it either — the diff
hunk shows the new entry, never the resulting order.

That is not a one-off. The batch this came from had `source-control
0.34.0` claimed by four branches
and `work-items 0.26.0` by five; those were caught only because a human
renumbered them by hand,
one merge at a time.

## What this adds

`scripts/check-changelog-parity.sh --check-order` reads each changelog
**whole** and fails on:

- a version sitting below a later one (naming the offending pair), and
- any version listed twice — the two-branches-staged-the-same-number
case.

Wired into `ci.yml` as a non-PR-scoped step, because the defect is a
property of the merged file
rather than of any one diff.

### Scope note

It covers `docs/conventions/*/CHANGELOG.md` as well as
`plugins/*/CHANGELOG.md`. That is deliberate
and load-bearing: convention changelogs carry no manifest version, so
the other two modes never look
at them — and a convention changelog is exactly where this shipped.

## Verification

Adversarial, not just green: **the gate fails on `main`'s pre-#1758
loop-lane changelog and passes
once the renumber is applied.**

```
MISORDERED CHANGELOG: docs/conventions/loop-lane/CHANGELOG.md is not newest-first
  — 5.0.0 (below 3.1.1).
```

- `check-changelog-parity.test.sh`: **26 → 32 cases, 0 failures.**
Includes the exact shape that
shipped (`6.0.0 → 3.1.1 → 5.0.0 → 4.0.0`, unbracketed convention
headings), a duplicate-version
case, and a `10.0.0 > 9.0.0` case so the comparison cannot regress to
lexical.
- `shellcheck -x` on both scripts — clean, no suppressions added.
- `--check-order` across the repo: all 71 changelogs pass.

Both heading forms this repo uses are parsed: `## [1.2.3]` (plugins) and
`## 1.2.3 — date`
(conventions). Comparison is `sort -rV`.

## Related

No linked issue — this is the preventive half of #1758, which fixed the
instance.

Refs #1758
Refs #1686

---------

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.

1 participant