docs(plan): record the Phase 4 acceptance run and hold the tag at DOING by kyle-sexton · Pull Request #362 · melodic-software/ci-workflows

kyle-sexton · 2026-08-06T08:46:37Z

Records the Phase 4 acceptance run in docs/topics/claude-review-lanes/PLAN.md. Draft, because the ledger it writes asserts that Phase 4 is not closeable yet, and that claim should be checked before it lands.

Why the tag is `[DOING]`, not `[DONE]`

The acceptance test ran end to end for the first time. It did what an acceptance test is for — it found three defects and two spec conflicts that every prior green signal had hidden — so SC4 ("#228/#238 closed with pointers") is unmet and both issues stay open. They also carry needs-human, which bars autonomous closure independently of the evidence.

What is now done: the write-path defect is fixed and merged (#359 → 058ed1a), and the incident lifecycle is re-demonstrated on main. What remains is tracked in #363, #364, and two adjudication items recorded on #238.

Phase 3's ENUMERATION FIX is the binding precedent here: a phase's close-out must enumerate its DELIVERABLES, not just its checks. The ledger therefore lists the unbuilt ones and dispositions each, so advancing the tag later cannot strand one.

What acceptance demonstrated

All four lane artifacts plus auto-resolve, on real lane output:

Artifact	Evidence
Green check on a dead credential	sandbox run 31079823199 — `review / review` concluded `success`
Marker comment	posted on claude-lane-sandbox#2, carrying `Failure class: auth`
`class=auth` annotation	`api_error_status: 401` in the check-run annotation
Incident opens	#361, via aggregator run 31082662762
Auto-resolve	runs 31083255896, 31083299991 (`action=update`), 31083359365 (`action=close`) — each `read-errors=0 cycle=clean coverage=complete`

The runbook's one empirically unverified choice — the bad-token VALUE — is now confirmed: the SDK reached the API and was rejected 401, yielding class=auth rather than degrading to the non-escalating other.

Re-demonstrated on `main`, because the first pass only proved a branch

Every incident write in the table above ran on the unmerged branch of #359 — main could not write an incident at all. #359 has since merged as 058ed1a, and the whole cycle was re-run on main:

Run (ref `main`)	Deliverable line	Outcome
31095551306	`read-errors=0 cycle=incident coverage=complete action=open`	incident #365 opened
31096144924	`read-errors=0 cycle=clean coverage=complete action=update`	clean 1
31096193502	`read-errors=0 cycle=clean coverage=complete action=update`	clean 2
31096244305	`read-errors=0 cycle=clean coverage=complete action=close`	#365 closed, `cleanCycles: 3`, recovery comment

So the acceptance evidence now covers the shipped product. Teardown is clean: no repo-level override remains, zero incidents open, and the org secret's updated_at never moved from 2026-08-05T13:32:31Z across either pass.

The three defects

1. The write path had never once executed — fixed and merged (#359). The poll renders the incident body to the dot-prefixed .claude-lane-incident.md, and upload-artifact ignores hidden files by default, so the upload collected nothing and if-no-files-found: error failed the step.

That is provable rather than sampled, and the corpus is pinned by code state rather than a run count so it does not drift as the schedule keeps firing. While main carried the pre-fix code, any cycle reaching action != none must have failed at this upload. Every main run before 058ed1a succeeded except the dispatch that forced this incident — therefore every one of those reported action=none, and the write path was never exercised. Runs at or after 058ed1a sit outside that corpus by construction: the four main re-demo runs above report action != none and succeed, which is the fix working, not a counterexample. (Dated observation, deliberately not load-bearing: 143 such pre-fix main runs as of 2026-08-06.) Each pre-fix run emitted the read-errors=0 cycle=clean line PLAN identifies as the antidote to the ≤ 1 ceiling check, while the write path could not fire.

2. Lane routing was never implemented — tracked in #364. #238's Contract requires the incident issue to carry the human-gated role label plus a kind=routed-advisory escalation-marker comment. #361 and #365 carried neither. Neither needs-human (the role label both #228 and #238 themselves wear) nor routed-advisory appears anywhere in this repository, on either branch — so this is unbuilt, not misconfigured. The label is applied inside the write-gate's byte-pinned region, so this PR does not patch it.

The operational point: an auth incident inherently needs a human at the provider layer, and the issue does not wear the label that routes it to one.

3. A validation skip is a silent no-review the aggregator cannot see — tracked in #363. When claude-code-action skips itself on workflow validation it exits 0, the check concludes green, nothing is reviewed, and no class= token is emitted. The aggregator's whole detection mechanism is that token, so this failure mode is invisible to it by construction. That is the #228 harm class, uncovered. The same issue records two lesser findings: a review-count comment that counted a review which never happened, and marker copy falsely asserting a push does not re-trigger the lane.

Two spec-vs-implementation conflicts, both recorded on #238 for adjudication

Neither is patched here — which side is wrong is a judgement call, and #238's Contract is ratified.

The reopen criterion. #238's third acceptance criterion says "a second incident reopens the same marker-selected issue (no duplicate)". The aggregator deliberately does not do that; its lookup queries state: "open" only and says so:

Open issues only. A closed incident is superseded by a fresh one rather than reopened, so this never paginates closed history.

nextState's only actions are open/update/close/none — no reopen exists — and a test pins the opposite behavior explicitly. So a second episode opens a new issue. The "no duplicate" half still holds (never more than one open incident, which is what SC3 ceilings), but "reopens the same issue" is contradicted by design, not merely unexercised.

The close condition. #238's Contract says the incident closes on the "first window whose review runs include a success and no auth class". This phase specifies — and the code implements — three consecutive clean cycles. The stricter shape ran, so nothing is broken, but two ratified authorities disagree and #238's wording is the stale one.

The sixth bullet's two downstream actions, now performed

Phase 4's sixth bullet requires more than closing #228/#238 — it also requires comment-closing claude-code-plugins#1327 with root cause and a pointer, and commenting provisioning#215 as folded into the taxonomy. Both were un-dispositioned; both are done, and both stay open.

claude-code-plugins#1327 — commented, not closed. It carries needs-human, which bars autonomous resolution. The comment states the half that is easy to gloss: that signature classifies as other, which is non-escalating by design, so detection is proven while no incident opens. Whether the instant-fail signature deserves its own escalating class is exactly the human decision left on it.
provisioning#215 — commented, folded into the taxonomy, state untouched. Same honesty: class=runner is an escalating class in the taxonomy, and nothing in production emits it — the only occurrences are the aggregator's unit tests. So that substrate-silence would still be silent today. Unpark trigger: caller-side selector-failure emission shipping.

The canary property's four targets, each with a verdict

The canary bullet names four coverage targets, and its headline ("asserts on REAL lane output") is satisfied — which is exactly why a silent three-quarters made the whole bullet read as covered. Review caught that; each now carries its own verdict in the ledger.

Target	Verdict
upstream #1501 silent-green + seat/credential death	covered — the round's strongest result; one variant (no class token at all) is uncovered and is defect 3
claude-code-plugins#1327 SDK instant-fail	detected, not escalating — classifies as `other` by design, so no incident opens
provisioning#215 runner mismatch	not exercised — `class=runner` is escalating in the taxonomy but nothing in production emits it
caller-side `class=runner` selector-failure marker (3a)	not shipped — it is the emission the row above needs, so both unblock together

Known coverage gaps, recorded rather than claimed

The multi-repo shape was never exercised. Incident aggregator: dead-credential detection from consumer review-run annotations (scheduled, zero new secrets) #238's first criterion describes auth-class annotations "across multiple consumer repos in one window" — the #1122 replay. Acceptance drove one repo (repositoriesSeen: 1).
The hysteresis is not a soak. The three clean cycles completed in roughly 92 seconds. It is counter arithmetic — three cycles however fast they arrive — so auto-close proves recovery was observed three times, never that it held for any duration. Correct per spec; recorded so no one infers a soak test.

Also recorded

Runbook Step 1 is wrong as written. It expects the wiring PR's own run to be "green and genuinely reviewed", which is structurally impossible on the PR that introduces the caller — claude-code-action validates the workflow against the default branch and skips. The baseline moved post-merge, before any credential was broken, which is what makes the later class=auth unambiguous instead of confoundable with a wiring gap.
Canary deferral, with a dated trigger (re-evaluate 2026-11-06). The rationale is stronger than Incident aggregator: dead-credential detection from consumer review-run annotations (scheduled, zero new secrets) #238's original: a workflow-validation skip is green, reviews nothing, and emits no class= token at all, so an annotation-based aggregator is structurally incapable of seeing it. A synthetic canary is the only proposed mechanism that would.
The org secret visibility flip is still pending and blocked nothing. Under visibility: all the sandbox reads the org credential and the selected-repositories list is legitimately empty — the runbook's own Step 0 disposition. Recorded as a hardening item, not an acceptance gate. The secret was never edited or re-scoped: updated_at stayed 2026-08-05T13:32:31Z throughout, and the forced failure came from a repo-level override that was set and then deleted.

Sequencing

This PR only records state; #359 is already merged, so the ledger's narrative is true on main as written. Do not advance the Phase 4 tag on the strength of this PR — the ledger's own "REMAINING TO CLOSE PHASE 4" list is what gates that, and it still names #363, #364, the two #238 adjudications, and the multi-repo gap.

It also corrects PLAN's own documented deliverable-line format against the shipped emitter: cycle carries a third value incident, and there is a coverage= field the documented line never had. That omission mattered — incident is the value that says the watchdog fired, so the documented enum was missing the one outcome that matters.

No linked issue — this PR records evidence and closes nothing. #228 and #238 remain open by design, for the reasons the ledger states.

fix(claude-lane-incident): upload the dot-prefixed incident body #359 — fixed the write-path defect this run uncovered; merged as 058ed1a.
test(claude-lane-incident): pin the whole body-artifact round trip #367 — hardened the same round trip after review; merged as fe1b880.
claude-review: a workflow-validation skip is an undetectable silent no-review, plus a false review count and false marker copy #363 — the validation-skip silent no-review, plus the false review count and false marker copy.
Incident aggregator: lane routing (needs-human label + kind=routed-advisory marker) was never implemented #364 — the unimplemented lane routing.
Incident aggregator: dead-credential detection from consumer review-run annotations (scheduled, zero new secrets) #238 — the aggregator this phase consumes; carries the two adjudication items as a comment.
ci(claude-review): dead-credential canary + error-class surfacing — a revoked OAuth token was invisible for 19h #228 — the parent dead-credential issue; the canary deferral's only anchor while it stays open.
Phase 4 close-out: operator actions and remaining execution sequence #349 — carries the sandbox wiring bundle and acceptance runbook.
[Incident] Claude lane infrastructure failure #365 — the incident opened and auto-closed by the main re-demonstration.
[Incident] Claude lane infrastructure failure #361 — the incident issue that opened and then auto-closed.
feat: wire the claude-review lane for the incident-aggregator fixture claude-lane-sandbox#1, feat!: convert reusable workflows to composite actions #2 — the wiring and probe pull requests.

🤖 Generated with Claude Code

The acceptance test ran end to end for the first time and found two defects that every prior green signal had hidden. All four lane artifacts plus the three-clean-cycle auto-resolve are demonstrated on real lane output, and the runbook's one empirically unverified choice — the bad-token value — is now confirmed to produce class=auth rather than the non-escalating other. The tag stays at DOING rather than advancing: SC4 is unmet, the write path is only fixed on the #359 branch, and #238's lane routing (role label plus the routed-advisory marker) was never implemented. Deliverables are enumerated and individually dispositioned per Phase 3's ENUMERATION FIX, so closing the phase cannot strand an unbuilt one. The canary deferral gains a dated re-evaluation trigger and a strengthened rationale: a workflow-validation skip is green, reviews nothing, and emits no class token at all, so an annotation-based aggregator cannot see it. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

markdownlint reads a line starting with `#238` as a heading (MD018). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The aggregator's lookup queries open issues only and documents the design as supersede-not-reopen, which contradicts #238's third acceptance criterion rather than merely leaving it unexercised. Which side is wrong is a decision, so it is recorded as blocking closure instead of patched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ling Across all 146 runs of this workflow exactly one ever failed, and on main's code a cycle reaching action != none must fail at the upload — so every other run necessarily reported action=none. That argument is airtight where the earlier "twenty-five consecutive runs" phrasing was only a sample. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ter grammar Adds what the acceptance run established after the first ledger pass: - the branch-vs-main qualifier, stated as a qualifier rather than a footnote, since reading branch evidence as product evidence defeats the test's purpose - the 92-second detail: the hysteresis is counter arithmetic, so auto-close proves recovery was observed three times, never that it held - the third defect (a validation skip emits no class token at all) and the two lesser findings beside it, tracked in #363 - lane routing tracked in #364; neither needs-human nor routed-advisory exists anywhere in the repository, so it is unbuilt rather than misconfigured - the close-condition conflict and the unexercised multi-repo shape, both recorded on #238 for adjudication rather than resolved here Also corrects PLAN's own documented deliverable line against the shipped emitter: cycle carries a third value `incident` and there is a `coverage=` field. The omission mattered — `incident` is the value that says the watchdog fired, so the documented enum was missing the one outcome that matters. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

#359 merged as 058ed1a and the full open-to-close incident cycle then ran on main: incident #365 opened by run 31095551306, and closed by 31096244305 after three covering clean cycles. The first pass had only ever proven the fix branch, so the acceptance evidence now covers the shipped product rather than code that never ran in production. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ath fix Review of the fix found it pinned only its own input, leaving three further ways to break the same round trip without any run going red. All three are now asserted, and the ledger says so rather than leaving the fix reading as complete on its own. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The never-executed argument was self-contradicted by the very re-demo runs the ledger cites: they report action != none AND succeed. The corpus is now pinned by CODE STATE — every main run before 058ed1a — so the argument holds as the schedule keeps firing, and the run count is demoted to a dated observation rather than load-bearing. Deliverable-line excerpts are reordered to match the emitter, which puts read-errors before cycle and coverage. The sixth bullet's two downstream comment actions were un-dispositioned. Both are now performed and recorded, each carrying the half that is easy to gloss: claude-code-plugins#1327's signature classifies as non-escalating other, so detection is proven while no incident opens; provisioning#215's class=runner is escalating in the taxonomy but emitted by nothing in production, so that silence would still be silent. Neither is comment-closed — both carry needs-human. Also drops the "not ours to change" overstatement here, matching the wording already corrected in the workflow and the pull request bodies. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

The re-demo runs it points at are recorded above the block, not below. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…ings The blocking one was an enumeration hole I had not seen: the canary-property bullet names FOUR coverage targets and the ledger judged one. Its headline ("asserts on REAL lane output") IS satisfied, which is precisely why a silent three-quarters made the whole bullet read as covered. Each target now carries its own verdict — the credential-death target genuinely covered, the SDK instant-fail detected but non-escalating by design, and the two runner targets not exercised because nothing in production emits that class at all. Remaining findings: - the pre-fix run count is now scoped to the pre-fix corpus and marked as a dated observation (143 main runs, verified independently), so no sentence's validity rests on a number the schedule keeps changing - the opener said "two defects" over a section enumerating three, and its unbuilt-deliverable count is replaced by the enumeration itself so the two cannot drift apart again - the label-absence claim is scoped to workflows and scripts; as written it would have been falsified by this very diff, which puts both strings in prose - REMAINING is now a list, and states plainly that this phase cannot be closed out autonomously at all: everything left is either code or a human's judgement, including the closure half of the sixth bullet - the marker-comment evidence points at the run log and explains why the pull request no longer shows it, so its absence does not read as fabrication - "roughly 92 seconds" matched no measurable boundary; "under two minutes" does Left deliberately untouched: the pre-existing verbatim quote of run 30571900637, which predates the coverage field and must not gain one, and the corrected emitter grammar line. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

cursor · 2026-08-06T13:49:53Z

Bugbot is not enabled for your account, so this pull request was not reviewed.

Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs.

claude · 2026-08-06T13:50:06Z

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

Review complete

Gather context (diff, CLAUDE.md, REVIEW.md)
Read changed file (PLAN.md)
Review for correctness / alignment with CLAUDE.md and REVIEW.md
Post findings

Summary

This is a docs-only PR (docs/topics/claude-review-lanes/PLAN.md, +270/-2) that appends a "PHASE 4 CLOSE-OUT LEDGER" entry and corrects the documented claude-lane-incident deliverable-line format. Per REVIEW.md, this repo has a dedicated claude-security-review.yml lane, so security findings are out of scope here — moot anyway for a pure-docs change.

Since this PR's central claims are checkable facts about the shipped code, I cross-verified the ledger's assertions against the actual sources rather than taking them at face value:

Deliverable-line correction (diff around L1917-1925): the new documented format cycle=<clean|incident|indeterminate> coverage=<complete|incomplete> action=... matches the emitter verbatim (.github/workflows/claude-lane-incident-aggregator.yml:596), and "incident" is a real return value of the classifier in .github/scripts/claude-lane-incident.cjs:333. Confirmed accurate.
"Lane routing unbuilt" claim (L1666-1682): grepped all of .github/ for needs-human and routed-advisory — neither string appears in any workflow or script. Confirmed accurate, matches the "unbuilt, not misconfigured" framing.
Reopen-vs-supersede spec conflict (L1697-1710): state: "open" is indeed the only state queried (claude-lane-incident-aggregator.yml:502), and nextState()'s only actions are open/update/close/none — no reopen branch exists in claude-lane-incident.cjs. Confirmed accurate.
Write-path fix (L1651-1659): include-hidden-files: true is present at claude-lane-incident-aggregator.yml:642, consistent with the described fix(claude-lane-incident): upload the dot-prefixed incident body #359 fix.
"Per Phase 3's ENUMERATION FIX" precedent: that heading exists earlier in the same file (L1362) and the new ledger follows its pattern (enumerate deliverables individually rather than a count).
The new ledger is inserted directly under the Phase 4 heading, ahead of the pre-existing 2026-07-31 "what gates it" entry — at first glance this reads as if the newer content buries older context, but that's this document's established reverse-chronological append style (same pattern used elsewhere, e.g. Phase 3), not something introduced by this PR.

No correctness or documentation-quality issues found — the ledger's factual claims about the shipped code check out, the tag is correctly held at [DOING] given the enumerated unbuilt deliverables, and the PR does exactly what it says: records evidence without touching code or closing issues that shouldn't be closed. No inline comments were needed.

Note: this PR is already merged; this review is retrospective and does not block anything.

github-actions · 2026-08-06T13:53:26Z

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

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 893d16ec09

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

chatgpt-codex-connector · 2026-08-06T13:54:00Z

+NOT BLOCKING, recorded so it is not mistaken for a gate: org secret
+`CLAUDE_CODE_OAUTH_TOKEN` visibility is still `all`, NOT the
+selected-repositories flip. That flip is operator-only UI work and remains
+PENDING. It blocked nothing here — under `all` the sandbox reads the org
+credential and the selected-repositories list is legitimately empty, which is
+exactly the runbook's Step 0 disposition. It is a hardening item, not an


Remove the pending secret-visibility flip

If an operator follows this purported hardening item, they will violate the security contract in CLAUDE.md:39-43, which explicitly defines all-repository visibility as deliberate and says never to “correct” this secret to selected repositories. Re-scoping it can omit current or future consumers, causing their Claude lanes to run without the credential and potentially produce the advisory green/no-review failure this phase is intended to detect; keep visibility: all as the required final state rather than recording a flip as pending.

AGENTS.md reference: AGENTS.md:L9-L11

Useful? React with 👍 / 👎.

kyle-sexton and others added 11 commits August 6, 2026 04:43

docs(plan): rewrap so issue refs do not open ATX headings

fabbb74

markdownlint reads a line starting with `#238` as a heading (MD018). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

docs(plan): rewrap the reopen note past MD018

a2f2987

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

docs(plan): correct a cross-reference direction in the write-path block

b5ac812

The re-demo runs it points at are recorded above the block, not below. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

kyle-sexton marked this pull request as ready for review August 6, 2026 13:49

kyle-sexton merged commit 1011512 into main Aug 6, 2026
40 checks passed

kyle-sexton deleted the docs/plan-phase4-closeout branch August 6, 2026 13:49

chatgpt-codex-connector Bot reviewed Aug 6, 2026

View reviewed changes

Navigation Menu

Uh oh!

docs(plan): record the Phase 4 acceptance run and hold the tag at DOING - #362

docs(plan): record the Phase 4 acceptance run and hold the tag at DOING#362
kyle-sexton merged 11 commits into
mainfrom
docs/plan-phase4-closeout

kyle-sexton commented Aug 6, 2026 •

edited

Loading

Uh oh!

cursor Bot commented Aug 6, 2026

Uh oh!

Uh oh!

claude Bot commented Aug 6, 2026 •

edited

Loading

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Aug 6, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

kyle-sexton commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why the tag is [DOING], not [DONE]

What acceptance demonstrated

Re-demonstrated on main, because the first pass only proved a branch

The three defects

Two spec-vs-implementation conflicts, both recorded on #238 for adjudication

The sixth bullet's two downstream actions, now performed

The canary property's four targets, each with a verdict

Known coverage gaps, recorded rather than claimed

Also recorded

Sequencing

Related

Uh oh!

cursor Bot commented Aug 6, 2026

Uh oh!

Uh oh!

claude Bot commented Aug 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review complete

Summary

Uh oh!

github-actions Bot commented Aug 6, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Aug 6, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

kyle-sexton commented Aug 6, 2026 •

edited

Loading

Why the tag is `[DOING]`, not `[DONE]`

Re-demonstrated on `main`, because the first pass only proved a branch

claude Bot commented Aug 6, 2026 •

edited

Loading