Repository navigation
ci-failure-analyst caller: the self-exclusion guard never matches a real check name, and no-PR failures still start the analyst #2175
Description
Problem
The CI Failure Analyst caller's self-exclusion guard never matches, and failed check runs that belong to no pull request still start the analyst. Both are pre-existing (the pre-collapse stub had them), found while reviewing the markets ADR-0007 collapse (combsmith/markets#513). The collapse deliberately preserved them to stay behavior-equivalent, and the cascade review flagged the guard as a minor finding.
Evidence
- Dead loop guard. The caller's
if:isgithub.event.check_run.conclusion == 'failure' && !startsWith(github.event.check_run.name, 'CI Failure Analyst'). A reusable-workflow check run is named<caller job> / <reusable job>; the real name isanalyzein the pre-collapse markets stub (jobanalyze, reusable jobanalyze) andci-failure-analyst / analyzeafter the collapse. Neither starts withCI Failure Analyst(that is a workflow display name, not a check name). Observed on markets: the analyst's check runs are namedanalyze. So a failed analyst run can start another analyst run on the same head SHA. It is bounded by the job's per-SHA non-cancelling concurrency group and the reusable's non-fork filter, but it is not prevented. The new markets bats test pins the dead string. - No-PR failures. The job fires on every
failurecheck run, including CI runs from pushes tomainthat have no pull request, so the reusable starts with no PR to comment on. Both the stub and (now) the markets ingress behave this way.
Required
- Replace the guard with one that matches the analyst's real check names (both
analyzeand<role> / analyze, case-sensitive), and prove it with a test fed realistic check-run payloads, including a failing analyst run (must be excluded) and a failing ordinary CI run (must still start the analyst). - Decide, and document, the no-PR behavior: either require an associated PR in the
if:(payload-only, so it stays a pure event filter) or accept parity and say why. If theif:changes, it must still passvalidate-ingress-if.sh. - The fix lands in the standard caller template (petry-projects/.github
standards/workflows/ci-failure-analyst.yml) and in every collapsed ingress (marketsagent-ingress.ymljobci-failure-analyst, and the markets collapse package §3). Keep the ingress job and the ratified package in step (see Sync the markets collapse package §3/§4b to the deployed ingress (pr-auto-review group + ci-failure-analyst pin) #2171).
Out of scope
The pin of the analyst's reusable (separate issue).
Activity
github-actions commentedon Oct 10, 2026
Thanks for the thorough write-up and evidence on this guard and the no-PR behavior. The details on the real check names (
analyzeand<role> / analyze) make the problem clear. One question to settle before a fix: for failed check runs with no associated pull request, such as CI runs from pushes tomain, do you want the analyst to be skipped by requiring an associated PR in theif:, or kept for parity with the current behavior?don-petry commentedon Oct 10, 2026
@petry-projects/solution-architect ruling requested on the no-PR question here.
The caller
if:startsci-failure-analystfor any failingcheck_run, including ones with no PR. The reusable already skips those at runtime (ci-failure-analyst-reusable.ymllines 32-57). It falls back tocommits/{sha}/pullswhencheck_run.pull_requestsis empty, which catches the race where the payload omits a PR that exists.Options:
github.event.check_run.pull_requests[0]to the callerif:. This avoids a runner for every no-PR failure, but loses the race-fallback case.Which one? Separately, the loop guard (
startsWith(name, 'CI Failure Analyst')) never matches the real check name,ci-failure-analyst / analyze, and will be corrected either way. The fix spanstemplates/ci-failure-analyst.yml,.github/workflows/ci-failure-analyst.lock.yml,docs/aw/ci-failure-analyst.mdand the markets package, with the canonical template inpetry-projects/.github.Generated by Claude Code
don-petry commentedon Oct 10, 2026
Solution Architect: structural advisory
Governing ADR: ADR-0007 (One agent-ingress stub per repo), which extends ADR-0001 (Thin-caller / reusable two-tier). ADR-0010 sets the matching bounds for
concurrency.group.Alignment: aligned if you pick (b). Option (a) is a legal event filter, but it gives up correctness that only the reusable can provide.
What I'd shore up (highest leverage first):
commits/{sha}/pullsfallback reads repo state. ADR-0007 says anif:that "reaches for repo state … belongs in the reusable", so (a) can't keep that fallback. It would drop it. ADR-0007's Consequences already accepted this exact cost: "a small per-event runner cost" in exchange for a filter that needs state. The ADRs have already settled this trade-off. Don't reopen it to save a short runner on pushes tomain.startsWith. Matchcheck_run.name == 'analyze' || check_run.name == 'ci-failure-analyst / analyze'. That is a pure payload predicate, so it passesvalidate-ingress-if.sh. The trade-off: a consumer whose own CI job is literally namedanalyzegets excluded. Say so in the template header.docs/aw/ci-failure-analyst.mdand the template header, citing ADR-0007. A new ADR isn't needed because nothing new is being decided.petry-projects/.githubfirst, then the.lock.yml, the markets ingress job and the §3 package together (per Sync the markets collapse package §3/§4b to the deployed ingress (pr-auto-review group + ci-failure-analyst pin) #2171). Update thecaller_stub_freezebaseline in the same change, or the freeze check will flag it.Escalate? No.
Advisory only. I comment; I don't change code. Ground: BMAD Method architect (bmad-agent-architect v6.8.0). To opt out on this item, add the solution-architect:hands-off label.
fix(standards): ci-failure-analyst loop guard matches its own check name
ci(ingress): pin ci-failure-analyst to v1-stable and fix the self-tri…
don-petry commentedon Oct 12, 2026
Status update after combsmith/markets#523 (merged as
9310b6e).Required 1 (guard): done for the collapsed ingress. The markets
ci-failure-analystjob now has!startsWith(github.event.check_run.name, 'ci-failure-analyst'). That excludes the analyst's real post-collapse check name,ci-failure-analyst / analyze. A markets bats test reads the prefix out of theif:and asserts thatci-failure-analyst / analyzematches it. The canonical template, combsmith/.github#1316 (head4cdd886), carries the same block byte-for-byte.This is a role prefix, not the exact-equality match the architect advised. Inside an ingress the two behave the same, because the only check runs whose names start with
ci-failure-analystare the analyst's own. Neither form covers the legacy bareanalyzename, which only a non-collapsed per-role stub produces.Required 2 (no-PR): ruled (b), parity. The caller
if:stays a pure event filter, and the reusable'scommits/{sha}/pullsruntime skip handles failures with no pull request. The ingressif:already matches this ruling, so it needed no change.Still open on this issue:
standards/workflows/ci-failure-analyst.yml). Its guard still uses'CI Failure Analyst'. That template needs the corrected guard for the bareanalyzecheck name, the realistic-payload test, and the no-PR decision written into its header anddocs/aw/ci-failure-analyst.md, citing ADR-0007.Generated by Claude Code
fix(standards): sync agent-ingress template job blocks to markets (pr…
test(agent-ingress): pin package §3 to the markets fixture and the gu…
docs(agent-ingress): sync the markets pilot package to markets#523 (c…