feat(workflows): Claude lane incident aggregator (Phase 4, #238) - #286
Conversation
Work-in-progress output from an in-flight worker that died with its session before reporting or being verified. Committed as-is to make it durable and diffable for a fresh-context verifier; not yet reviewed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pLW2dybov9xvTFtx48Ueb
Completes PLAN.md Phase 4: the scheduled watchdog that turns the review lanes' advisory-green infrastructure failures into one visible incident item. The lanes conclude green on a dead credential by design, so "every PR in every repo went unreviewed" is currently indistinguishable from "everything is fine". This is the aggregate view. The pure state machine arrived in 47591d2 unreviewed. Reviewed here and corrected: - classifyCycle now takes readErrors and refuses to call any cycle clean while one is non-zero. Without it a single unreadable consumer repo — a 403 under a narrowed credential, a rate limit, a transient 5xx — was swallowed, reported clean, and three such cycles auto-closed a live incident. - Added the lane check-run name contract (LANE_CHECK_RUN_JOB_IDS, isLaneCheckRun), pinned by test against the three reusables' actual inner job ids. A name the aggregator cannot match makes laneRunsObserved permanently zero: the incident never opens and never auto-closes. - nextState's issueExists/issueOpen pair collapses to issueOpen; the workflow searches open issues only, so the two were never independently reachable. - Two noAssignInExpression lint failures fixed; the scripts now pass `biome ci --error-on-warnings` at the pinned 2.5.4. The credential is an unresolved user gate, so the workflow ships as a swappable seam with a structurally-guaranteed dry run: the job grants `contents: read` + `issues: read` and no write scope of any kind, so the ambient token is incapable of mutating an issue however the steps are wired, and every write step is gated on the same minted-token expression that authors it. Absent CLAUDE_LANE_INCIDENT_APP_PRIVATE_KEY no token is minted, the poll scopes to this repository, and the body it would have written lands in the job summary. Polling scope is the credential's own installation, so granting App access is what enrols a consumer — no second inventory to drift. Test plan: `node --test .github/scripts/*.test.cjs` 336 pass; `biome ci --error-on-warnings` clean; `actionlint` clean on the new workflow; comment-hygiene clean at the CI extension set. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pLW2dybov9xvTFtx48Ueb
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
Independent fresh-context verifier findings against the aggregator, in
severity order.
HIGH — an incident could auto-close while nothing was being reviewed.
The poll counted every matched check run as lane liveness without
reading status or conclusion, and any positive count made the cycle
clean. A skipped lane job still publishes a check run under the same
name: that is the kill-switch's whole design (a name-stable skip reads
as success), and draft PRs and skip-actors produce the same shape. So
flipping CLAUDE_LANES_DISABLED to quiet the noise during a credential
outage would have auto-closed the very incident tracking it three
cycles later. Liveness now requires status completed and a conclusion
outside {skipped, cancelled, neutral}. Annotations are still read from
every lane check run regardless of conclusion — the lane concludes
green on an infrastructure failure by design, so conclusion must never
filter the detection path.
MEDIUM — per-class counts measured polling cadence, not blast radius.
Annotations persist, so a stuck PR was re-counted every hour and summed
into the carried state: one broken PR read "auth: 24" after a day, next
to an affected-repository table reporting one repository. Counts are
now distinct affected pull requests, deduplicated within a cycle and
carried across cycles as the peak rather than the sum. The body column
says so.
MEDIUM — two reads truncated silently, contradicting the invariant
stated four lines above them. checks.listForRef and listAnnotations
took one page without comparing against total_count and
annotations_count. Both now count a read error, which caps the cycle at
indeterminate.
LOW — the state block could 422 the update on the largest incident. The
render bounds capped the tables but not the tracked index the state
block serializes, and that index accumulates. Bounded in
mergeRepositories, so both are capped; the test now drives twelve
cycles over a 200-repository fleet at the per-repository ceiling
instead of picking a shape that stays under the limit by luck.
LOW — a fork pull request could vote on liveness or open a false
incident. A fork head ships its own workflow files, so an outside
contributor could declare a job named `review` emitting any annotation
they like. The lanes do not review fork PRs at all, so a fork head is
never this fleet's signal; the poll skips them before spending an API
call. The "over-matching is safe" comment that claimed otherwise is
corrected.
LOW — dropped the `push` trigger. It is beyond PLAN.md's scheduled-
workflow spec, and an out-of-band cycle counts toward the three clean
cycles that auto-close an incident, so merging an unrelated edit during
an outage would shorten the recovery window.
Also: the reported API budget now covers the issue lookup, not only the
poll, so the deliverable line means "total" as PLAN.md asks.
Tests: the canary now asserts on REAL lane output. Both suites
previously hand-wrote annotation fixtures that differed from each other
and from the shipped emitter's template; a new set drives the real
classify.cjs over seven execution-file shapes, composes the annotation
the way the composite composes it, pins that composition against
action.yml, and proves a class token planted in the model-authored
`result` field never reaches the aggregator.
Test plan: `node --test .github/scripts/*.test.cjs` 347 pass, 0 fail;
`biome ci --error-on-warnings` clean; `actionlint` clean;
comment-hygiene clean; typos clean.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013pLW2dybov9xvTFtx48Ueb
|
Claude encountered an error after 0s —— View job I'll analyze this and get back to you. |
Second independent fresh-context verifier pass. It confirmed the five findings from ea7823b resolved and the dry-run no-write guarantee still holding, then found that the fix for the last one had shipped a workflow that does nothing. HIGH — the dry run could not read anything, so the only path that ships today produced nothing. The job granted `contents: read` + `issues: read`, and declaring `permissions:` at all sets every unlisted scope to none. The check-run and annotation endpoints require Checks:read and carry no public-repository exemption, so the first listForRef of every run 403s, every cycle reports indeterminate, and the hourly deliverable is the string `none` forever — while the header claimed the dry run "exercises the whole pipeline against live data". Granted `checks: read` and `pull-requests: read`. The no-write guarantee is the ABSENCE of any write scope, not the shortness of the list, so adding reads does not weaken it; a new test asserts all four reads are present, next to the one asserting no write is. MEDIUM — the truncation guards traded silent truncation for a permanent stall. Refusing to read a head past one page made any consumer whose PRs carry more than 100 check runs raise a read error every cycle forever, which pins the whole fleet at indeterminate so no incident can ever auto-close. ci-workflows heads already carry 36-51. Both endpoints now paginate, as the installation list and the issue lookup already did. MEDIUM — the 422 fix bounded the accumulating term and left the dominant one alone. A repository row repeats the repository name once per pull-request link, so 440 copies land in the render against 60 in the state block: the same 40 rows span ~48k characters at a 36-char name and ~98k at GitHub's 140-char maximum. The row-count budget meant the body fit or 422'd depending on how the org happens to name its repositories. Replaced with a character budget against the real ceiling. The old test passed only because it picked a short name — the same luck it criticized — so the body-size tests now drive the longest `owner/name` that can exist. LOW — the remainders lied. With the tracked index capped at 60, a 200-repository incident rendered "40 shown, +20 more" and an operator read 60. State now carries `repositoriesSeen` and a per-repository `pullsSeen`, so every "+N more" counts the incident rather than the surviving slice, at a cost of two integers. LOW — the class column did not say what its number means (it is a peak within one 24h window, not a running total), the Remediate list told the reader to wait for an auto-close that cannot happen while a kill-switch is set, `state.unrecognized` was tracked and round-tripped but never rendered so its early warning reached nobody, and the fork skip was case-sensitive against a human-typed dispatch input. All four fixed. Test plan: `node --test .github/scripts/*.test.cjs` 349 pass, 0 fail; `biome ci --error-on-warnings` clean; `actionlint` clean; comment-hygiene clean; typos clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_013pLW2dybov9xvTFtx48Ueb
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
1 similar comment
|
Claude encountered an error after 1s —— View job I'll analyze this and get back to you. |
…2 tests failing)
Uncommitted working state recovered from a worker that died on a session
limit mid-edit. Committed as-is to make it durable and diffable, not
because it is correct.
Known failing in .github/scripts/claude-lane-incident-aggregator.test.cjs:
- check runs and annotations are paginated, neither truncated at one
page nor refused
- the API-call count reported to the log is the number of reads
actually attempted
claude-lane-incident.test.cjs passes 37/37. The PR remains a draft.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 5m 57s —— View job Security reviewScope: the 4 changed files (
This is a carefully built piece of infrastructure — the untrusted-annotation-text discipline ( 1. IMPORTANT — confidence: MEDIUM — the minted App token's
The mint step requests The workflow's actual writes never leave this repository — Because a single 2. SUGGESTION — confidence: LOW-MEDIUM — incident-issue identification trusts any
The practical exploitability is low — pulling this off requires an actor who can already get some installed App to author issue content on this repo, which is itself a meaningfully privileged position — so this is registered as a hardening suggestion rather than a vulnerability with a clear path to abuse. Tightening the candidate filter to also check No injection surfaces were found in the |
…ness The two failures 1455e50 left behind were both in the TEST HARNESS, and fixing them surfaced a third defect in the poll itself. 1. The pagination test asserted `github.paginate(github.rest.checks.*)`, a form the poll deliberately abandoned: that helper returns one flattened array (hiding how many requests it made, so the workflow's deliverable API-budget line would undercount its dominant term) and follows Link headers to exhaustion (so a consumer publishing thousands of annotations could walk the poll past its own job timeout). The assertion never matched anything in the file — its own comment cites the installation list and the issue lookup as precedent, and both of those use `paginate.iterator` too. Re-pinned on the real contract, and strengthened: the helper must paginate, both endpoints must go through it, and neither may be invoked directly. 2. The mock double-counted. Each endpoint pushed a call record AND the iterator pushed one per page, so a single-page read recorded two entries against the one request the poll counted. Endpoints now resolve a record shape without performing a request, and the iterator records once per page carrying that endpoint's own kind, so `calls` is a one-record-per-request ledger and the per-kind filters other tests depend on keep working. 3. A multi-page invariant, added so the oracle cannot be satisfied by arithmetic coincidence, exposed the real defect: `readAll` counted a page only once received, so a request that threw was never counted while the direct `read` helper counts before it can fail. The budget therefore SHRANK once reads started 403ing — understating spend on exactly the cycles worth diagnosing. Counted in the catch. Full suite green: 363 node tests, 6 shell suites, actionlint exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 1m 40s —— View job Security reviewScope: the 4 files in this PR at
The new commit ( Two items from the prior review remain present and unaddressed in this revision (unchanged lines, carried here for visibility since this is a fresh review pass): 1. IMPORTANT — confidence: MEDIUM — the minted App token's
2. SUGGESTION — confidence: LOW-MEDIUM — incident-issue identification trusts any
The candidate filter is No new injection surfaces, permission changes, or credential-handling issues in this revision. The untrusted-annotation discipline in |
`biome ci` fails on this file at `1455e50`, not at the fix commit: the
`yield { data: items.slice(...) }` line arrived over the print width when
the page-aware mock was first written, and the formatter wraps the object
literal. Reformatted with biome 2.5.4 under the repo's own config
(fixtures/typescript/good/biome.json) rather than by hand.
Behavior unchanged: 363 node tests pass.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Claude finished @kyle-sexton's task in 43s —— View job Security reviewScope: the 4 files in this PR at
Two items from the prior reviews remain present and unaddressed at this HEAD (unchanged lines, carried here for visibility since this is a fresh review pass): 1. IMPORTANT — confidence: MEDIUM — the minted App token's
2. SUGGESTION — confidence: LOW-MEDIUM — incident-issue identification trusts any
The candidate filter is No new injection surfaces, permission changes, or credential-handling issues in this revision. The untrusted-annotation discipline in |
|
Claude finished @kyle-sexton's task in 4m 3s —— View job Code review complete
Per Scope: the 4 changed files at 🔴 Important — the close sequence is three separate writes to the same issue with no atomicity, so an interrupted run can leave it half-closed or double-commented
This isn't silently unrecoverable — the incident state lives in the issue body, The GitHub REST API accepts This step also has no dedicated unit-test coverage in Everything else held up well against a close read:
|
|
Claude finished @kyle-sexton's task in 2m 27s —— View job Security reviewScope: the 4 files changed in this PR at
No new logic, permission, or credential-handling issues found. The two items already raised in prior review passes remain present and unaddressed at this HEAD (carried here for visibility since this is a fresh review pass, not because anything changed): 1. IMPORTANT — confidence: MEDIUM — the minted App token's
2. SUGGESTION — confidence: LOW-MEDIUM — incident-issue identification trusts any
The candidate filter is Everything else holds up on this re-read: the untrusted-annotation discipline in Branch: |
![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: c7c1704cbc
ℹ️ 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".
| !issue.pull_request && | ||
| issue.user?.type === "Bot" && | ||
| (issue.body ?? "").includes(SELECTOR_MARKER), |
There was a problem hiding this comment.
When another installed App or github-actions[bot] workflow creates an issue containing this public marker, filtering only on user.type === "Bot" adopts that unrelated issue; subsequent incident or recovery cycles can overwrite or close it, while two such issues make every lookup fail. Compare issue.user.login with the expected ${steps.credential.outputs.app-slug}[bot] identity so only issues authored by this workflow's App qualify.
Useful? React with 👍 / 👎.
| if (pages >= MAX_PAGES_PER_READ) { | ||
| readErrors += 1; |
There was a problem hiding this comment.
When a paginated endpoint returns 901–999 items, page 10 is a short final page with no remaining results, but this condition still increments readErrors and reports truncation merely because ten pages were read. Since classifyCycle maps any read error to indeterminate, a sufficiently large installation or check-run result set can prevent an existing incident from ever accumulating clean cycles despite a complete poll; inspect the final page length or probe for an eleventh page before declaring truncation.
Useful? React with 👍 / 👎.
|
Claude has reviewed this PR 1 time. The lane skips further automatic reviews after 5; deleting this comment resets the count. |
## Summary - Correct the claude-lane-incident-aggregator header and matching comments that overclaimed dry-run reads always 403 without Checks:read. - Document the empirical credential-sizing story: public dry-run reads work with the ambient token; private reads typically 404 without an installation token; writes still need the App. Closes #325 ## Related - #286 (aggregator) - #228 - #238 ## Test plan - [x] `node --test .github/scripts/claude-lane-incident-aggregator.test.cjs` (30/30 pass) - [x] Confirmed no remaining `no public-repository exemption` / `dry run 403s` wording in the workflow or matching test - [ ] Skim PR diff: comments/docs only; permissions block and runtime behavior unchanged Made with [Cursor](https://cursor.com) Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
No linked issue — see
## Relatedfor why #228/#238 stay open.Summary
Completes PLAN.md Phase 4: the scheduled aggregator that turns the Claude
review lanes' advisory-green infrastructure failures into one visible incident
item.
The lanes are advisory by contract — a dead org credential, a rate limit, an SDK
instant-fail, or an unresolvable runner concludes the check GREEN and posts a
marker comment. Nothing aggregates those markers today, so "every PR in every
repo went unreviewed for two days" looks exactly like "everything is fine". This
workflow polls consumer repositories for the machine-readable
class=<token>annotation
.github/actions/claude-lane-outcomeemits, and maintains ONEincident issue here: opened on the first escalating class (
auth/runner),kept current in place, closed by itself after three consecutive clean cycles.
THIS PR IS A DRAFT BECAUSE THE CREDENTIAL IS AN UNRESOLVED USER GATE
The aggregator needs cross-repository read plus issue write, and no existing
App holds that set — the runner-observer App
(
CI_RUNNER_OBSERVER_CLIENT_ID) holdsactions:read,administration:read,metadata:read,organization_self_hosted_runners:readand nothing else(verified live). Widening an App or creating one is an org-owner decision, so
this PR does not make it. What it ships instead is a swappable credential
seam plus a dry-run mode, exactly as PLAN.md's Phase 4 credential gate
anticipated.
Correction to PLAN.md's stated permission set. Phase 4 names
checks:read+issues:write. The actual minimum is three repository permissions plus theimplicit
metadata:read: finding the heads to inspect needspull-requests:readas well. The mint step declares all three.The dry run is structural, not a flag
A per-write-site
if: !dry-runis the shape where missing one site isinvisible. This does not use that shape:
checks,contents,issues,pull-requests) and no write scope of any kind, so the ambientGITHUB_TOKENis incapable of creating, editing, or closing an issuehowever the steps are wired. A reviewer confirms "this cannot write" by
reading one
permissions:block. Two tests guard that block from both sides:one asserts no write scope appears, the other asserts every read the poll
makes is granted — omitting a read is how a dry run silently becomes a no-op.
CLAUDE_LANE_INCIDENT_APP_PRIVATE_KEY, no App token is minted at all.steps.credential.outputs.token != ''andauthors with that same token — the gate and the write credential are one
condition and cannot drift apart.
to
$GITHUB_STEP_SUMMARY, and the API-budget line is logged either way.Three tests pin that contract against the shipped YAML text
(
claude-lane-incident-aggregator.test.cjs).Polling scope is the credential's own installation
GET /installation/repositories, one call. Granting the App access to arepository is what enrols it — there is no second, hand-maintained inventory to
drift (the same rationale
standards-sync-stuck-automerge-alert.ymlrecords forderiving its targets from the manifest). A dry run has no installation, so it
scopes to this repository alone, which is a real consumer of its own lanes
(
claude-review-self.yml) — the dry run exercises the whole pipeline againstlive data rather than a fixture.
What was wrong in the inherited commit
47591d2was committed unreviewed from a worker that died mid-flight. The purestate machine is sound; it had two real defects and one avoidable generality:
classifyCyclehad no notion of a failed readclean, and three such cycles auto-closed a live incident.readErrorsnow caps any cycle atindeterminate; an observed escalating class still wins outright.laneRunsObservedpermanently zero — the incident never opens and never auto-closes. NowLANE_CHECK_RUN_JOB_IDS+isLaneCheckRun, matched per/-separated segment (soreview / reviewand a differently-named caller job both hit), pinned by test against the three reusables' actual inner job ids.nextState({ issueExists, issueOpen })issueOpen; a closed incident is superseded by a fresh one rather than reopened, which keeps "≤1 open incident item" without paginating closed history every cycle.Also: two
noAssignInExpressionlint failures in the inherited module — it didnot pass
biome ci --error-on-warningsat the pinned 2.5.4 as committed.Two deliberate deviations from PLAN.md's literal text
Both are in the inherited module, both are documented at the site, and both are
called out here rather than left for a reviewer to find:
concurrencyis absent fromRECOGNIZED_CLASSES. PLAN.md:99 sketchesclass=auth|rate-limit|concurrency|overloaded|other, butclaude-lane-outcome/classify.cjs— the sole emitter — can never emitconcurrency. An unemitted allowlist entry is indistinguishable from a forgedone on a public repo.
runneris present although nothing emits it yet. It is the caller-sideselector-failure token PLAN.md 3a commits to; listing it now means the
aggregator recognises it the moment it lands rather than counting it as
unrecognised.
Test plan
Run locally against this branch:
node --test .github/scripts/*.test.cjs→ 349 pass, 0 fail (up from 310on
main; 34 module tests, 29 workflow-contract tests). The workflow testsextract the shipped
actions/github-scriptbodies out of the YAML and executethem against a mock Octokit — the same technique
standards-sync-stuck-automerge-alert.test.cjsuses — so the text under testis the text that ships.
npx @biomejs/biome@2.5.4 ci --error-on-warnings .github/scripts fixtures/typescript/good --config-path fixtures/typescript/good/biome.json→ clean.actionlint .github/workflows/claude-lane-incident-aggregator.yml→ exit 0.comment-hygieneat the CI extension set →clean.typoson all four files → exit 0.Behavioral coverage worth naming: an escalating annotation on a green lane
check run opens the incident, while a skipped one is not evidence the lanes
ran; an unreadable repository yields
cycle=indeterminateand does not abortthe scan of its peers; a fork pull request can neither vote on liveness nor open
an incident; the body stays under GitHub's 65536-character limit at the longest
owner/nameGitHub permits, across twelve accumulating cycles; a human-authoreddecoy carrying the public marker string is never adopted, and a pile of decoys
cannot fail the lookup closed and suppress a real incident. The canary asserts
on real lane output: seven execution-file shapes are driven through the
actual
classify.cjs, composed into the annotation the way the shippedcomposite composes it (pinned against
action.yml), and aclass=tokenplanted in the model-authored
resultfield is proven not to propagate.Not verified — deliberately out of scope for this PR: the Phase 4 acceptance
test (sandbox repo, bad-token override → green check + marker +
class=authannotation + incident opens → restore → auto-resolves after 3 clean cycles).
PLAN.md sequences it after Phase 3d wave 1, and it cannot run without the
credential. The first scheduled run after merge is a live dry run against this
repository and is the nearest available evidence.
Open item this PR does not resolve: the label
claude-lane-incidentdoes not exist in this repository (gh label listverified). The taxonomy here is github-iac-managed — the
governance: managedlabel states hand edits are pruned on the next apply — and this branch's scope
fence forbids touching github-iac. The workflow applies the label on issue
creation so PLAN.md's Sanity Check query works, but it must be declared in
github-iac's
Labels.csor the next apply prunes it. Issue selection neverkeys on the label (the body marker plus bot authorship do), so a pruned label
degrades the
--labelsanity query and nothing else.Related
keyword. PLAN.md gates closing them on "merge and passing acceptance
test", and the acceptance test runs after Phase 3d wave 1, which has not
happened. A closing keyword here would fire the moment this is un-drafted and
merged.
docs/topics/claude-review-lanes/PLAN.md) is thespecification this implements.
.github/actions/claude-lane-outcome(merged ina7c7145) is the annotation emitter this consumes.the
claude-lane-incidentlabel in github-iac, and the Phase 4 acceptancetest.
The decision being asked of a human
Grant this workflow an identity, one of two ways: (a) widen the existing
runner-observer App with repository permissions
checks: read,pull-requests: read, andissues: write, or (b) create a minimal new Appholding exactly those three — PLAN.md's Approval-record item 6 prefers (a) if
the permissions fit, else (b). Either way the App's installation must be scoped
to the consumer repositories that should be watched, because that installation
list is this workflow's polling scope; then set
CLAUDE_LANE_INCIDENT_APP_CLIENT_IDandCLAUDE_LANE_INCIDENT_APP_PRIVATE_KEYas secrets on this repository. Until that happens the workflow runs, costs a
handful of API calls an hour, reports its findings to the job summary, and
writes nothing — which is the intended resting state, not a broken one.