Skip to content

ci: enrich scheduled-failure issues with an LLM triage pass - #2663

Draft
tonyandrewmeyer wants to merge 32 commits into
canonical:mainfrom
tonyandrewmeyer:ai-failure-notifications-step-5
Draft

ci: enrich scheduled-failure issues with an LLM triage pass#2663
tonyandrewmeyer wants to merge 32 commits into
canonical:mainfrom
tonyandrewmeyer:ai-failure-notifications-step-5

Conversation

@tonyandrewmeyer

@tonyandrewmeyer tonyandrewmeyer commented Jul 24, 2026

Copy link
Copy Markdown
Collaborator

Today, when a scheduled workflow fails, notify-scheduled-failure.yaml opens an issue whose title and body are a generic one-liner plus a link to the job. A human then has to read the logs, check whether it duplicates an existing issue, and rewrite the title and body before it is useful to anyone.

This adds a second stage that does that first pass automatically: extract a deterministic failure signature from the failing jobs' logs, dedupe against real candidate issues, ask an LLM to draft a descriptive issue or comment, validate its output against a schema, and apply it, falling back to today's plain notice at every layer if anything goes wrong.

Two stages, not one

Stage 1 (notify-scheduled-failure.yaml) keeps its current guarantee: no secrets, no LLM, no network beyond gh, always produces a notification. The only change is a coarse dedupe, searching open issues for the workflow name and commenting on a match instead of opening a duplicate.

Stage 2 (ai-failure-enrich.yaml, new) does the enrichment. It is a separate file called from the notifier after the notify job rather than another job inside it, so that stage 1's guarantee stays legible: everything that needs an API key, an environment or a checkout lives in stage 2, and stage 1 depends on none of it.

Because stage 2 runs inside the caller's run, the seven callers have to hand down what it needs: actions: read for the log fetch, and OPENROUTER_API_KEY passed by name. A called workflow holds no scope its caller withheld, and cannot use a secret that no call site named.

Repository layout

The script and its tests now live in a new top-level scripts/, with the tests in scripts/test/ where the normal unit run collects them. The alternative was the script in .github/, where pytest never looks, and its tests in test/ alongside the tests for ops itself, reaching it through a sys.path insert. release.py and the other .github scripts belong here too, but that is a follow-up rather than more churn in this PR.

Dedupe ladder

  1. Marker match: if a :sig= marker for this exact run id already exists anywhere in the repo's issues, this is a re-run of the same failing jobs. Comment "re-run attempt still failing" and skip signature extraction and the LLM call entirely. On a ten-run calibration corpus this caught 2 of the 4 real duplicate pairs, the single highest-value rung, and the reason both real July duplicates existed at all was re-runs rather than recurring signatures.
  2. No API key: plain fallback body, still marker-stamped.
  3. Candidate search: open issues plus recently-closed ones (14 days or fewer, capped at 3, and explicitly labelled "(closed …)" in the block the model sees, with an instruction never to auto-treat a closed issue as a strong match) feed the LLM's comment-vs-new decision.
  4. Schema-invalid output: plain fallback body, with the validation errors written to the job's step summary.
  5. Valid new envelope on a fresh placeholder: gh issue edit the placeholder in place, rather than opening a second issue.
  6. Valid new envelope where stage 1 had commented on an older issue: the coarse match was wrong, so open a genuinely new issue and leave a pointer comment on the older one.

The marker format is <!-- ai-failure-notifications:run=<id>:origin=new|comment --> from stage 1 and <!-- ai-failure-notifications:run=<id>:sig=<hash> --> from stage 2. Rung 1 keys specifically off :sig=, so a same-run re-fire arriving before enrichment finishes is not mistaken for "already handled".

The coarse search matches on workflow name alone rather than the full "Scheduled workflow 'X' failed" sentence, because enrichment rewrites the title and would otherwise break its own future matching. The applier always appends a Workflow: <name> footer so the match survives.

Failure handling

A notification is never lost, and a lost enrichment does not cost a duplicate issue:

  1. Inside the script. No API key, an OpenRouter error, or schema-invalid output all fall back to the plain body through the same marker-aware applier. gh search failures degrade to "no marker" or "no candidates" rather than raising.
  2. In the enricher, if stage 1 itself failed. Finding no marker for the run, it creates the plain issue rather than assuming one exists.
  3. Stage 1 never depends on stage 2 at all.

There is deliberately no third, workflow-level fallback job. Reaching one would mean gh issue create had already failed in notify and the enricher had died before trying it again, and a third attempt in a third job is not a reason to expect a fourth to work. An enrich that dies on its own leaves an un-enriched placeholder and a red job, which is today's behaviour rather than a lost notification.

One unintuitive thing, worth a look

The key is configured on stage 2's ai-failure-triage environment, and each call site passes it down by name. What every level actually passes is an empty string, since no job outside that environment can read an environment secret. The real value arrives from environment: on the enrich job.

Both halves are needed. Take the pass-through out as redundant, which is what it looks like, and the environment stops filling it in too: the key is then empty, and enrichment quietly degrades to a plain notice with nothing failing loudly. I could not find this documented anywhere, so it is measured rather than promised, and the comments at each call site say so.

An earlier version of this used secrets: inherit, which also works and would have added seven secrets-inherit suppressions, the repository's first. Passing by name avoids all of them, and stops stage 2 seeing secrets it has no business with, UPDATE_CHARM_PINS_ACCESS_TOKEN among them. Thanks to @james-garner-canonical for pushing on this.

Testing

The read-only gh layer has been exercised for real against this repository, using run 29847889218 (the 2026-07-21 "Example Charm Integration Tests" scheduled failure, which opened #2658). On that run the extractor independently produced traceback_top_error: "KeyError: 'loki/0'" along with the three failing tests and their timeout messages, which matches the hand-written diagnosis left on #2658. That is the first check of the extractor against a log it was not calibrated on.

The write calls have now been exercised too, in my fork, against a deliberately-failing stand-in caller with a real OpenRouter key, running the whole path: log fetch, signature, candidate search, LLM call, schema validation and apply.

Before this does anything visible

  • The ai-failure-triage environment and its OPENROUTER_API_KEY need provisioning. Until then every run takes the "no API key" path, which is still better than today's baseline since the coarse dedupe now works, and makes this safe to merge behind.
  • OPENROUTER_MODEL repo variable: the script defaults to deepseek/deepseek-chat if unset, worth pinning explicitly once the model choice is confirmed against live traffic.

Comment thread .github/workflows/ai-failure-enrich.yaml Fixed
Comment thread .github/workflows/ai-failure-enrich.yaml Fixed
When a scheduled workflow fails, notify-scheduled-failure.yaml opens an
issue whose title and body are a generic one-liner plus a link to the job.
A human then reads the logs, works out whether it duplicates an existing
issue, and rewrites the title and body before it is useful to anyone.

This adds a second workflow that does that first pass: extract a
deterministic failure signature from the failing jobs' logs, dedupe against
real candidate issues, ask an LLM to draft a descriptive issue or comment,
validate the result against a schema, and apply it -- falling back to the
plain notice at every layer if anything goes wrong.

The notifier keeps its guarantee of always producing a notification with no
secrets and no LLM; its only change is a coarse dedupe by workflow name.
The enricher is a separate workflow rather than being spliced into the
notifier, so an unprovisioned or broken enricher cannot affect whether a
notification happens. It subscribes via workflow_run to the seven scheduled
workflows that call the notifier, because a reusable workflow invoked via
workflow_call gets no independent run to subscribe to.

The script keeps its pure logic -- log parsing, marker handling, prompt
building, schema validation -- separate from everything that talks to `gh`
or OpenRouter, so the logic is testable without mocking the network. It
needs nothing outside the standard library.

Two things reviewers should look at:

- The `# zizmor: ignore[dangerous-triggers]` on the workflow_run trigger is
  the first zizmor suppression in this repository, and canonical#2612 removed the
  only zizmor config three weeks ago by fixing the underlying issue
  instead. The reasoning for treating this as a false positive is written
  out at the trigger; the precedent is a decision for reviewers, and the
  alternative is splicing the enrichment call into all seven callers.
- Nothing is provisioned yet: without the `ai-failure-triage` environment
  and an OPENROUTER_API_KEY, every run takes the no-API-key path and
  produces today's plain notice, with the coarse dedupe as the only
  visible change. That makes this safe to merge ahead of the secret.

The `gh` read path has been exercised against this repository using run
29847889218; on that run the extractor independently produced
`traceback_top_error: "KeyError: 'loki/0'"`, matching the diagnosis a
maintainer had written by hand on the issue it opened (canonical#2658). The write
calls are covered only by mocks, since exercising them means posting here;
they want a workflow_dispatch run after this merges.
@tonyandrewmeyer
tonyandrewmeyer force-pushed the ai-failure-notifications-step-5 branch from 9371af3 to 41325bb Compare July 25, 2026 00:55
tonyandrewmeyer and others added 8 commits July 25, 2026 13:51
Both found by dogfooding in the fork.

The `Workflow: <name>` footer that the notifier's coarse search depends on
was never written. It existed only in the prompt template, while the
notifier's comment, the design and the applier all assumed the applier
appended it. Enriched issue #24 in the fork went out without it, and the
coarse search kept working only because the model happened to leave the
workflow name in the title -- one different title and the issue thread
would split permanently. Add render_body(), use it on every create, comment
and in-place edit, and cover it with tests.

Also make write_step_summary report to stderr as well as the summary file.
Every fallback in this script reports through it, so a fallback was
invisible in the job log and over the API -- which is exactly what made the
second fork run hard to diagnose: it produced a plain-fallback comment with
no way to tell whether OpenRouter had errored or the output had failed
schema validation.
The LLM path was falling back to the plain body on almost every run. The
step summary said `envelope: unknown field(s) ['also']`.

`validate_envelope` calls `validate_entry` for the top-level envelope, and
`validate_entry` checks unknown keys against a set that does not contain
`also`, so the error was appended before `validate_envelope` reached its own
unknown-field check, which did exclude `also`. That exclusion was dead code.
Since the JSON schema handed to the model declares `also`, the model emits
it routinely, so this was the normal path rather than an edge case.

Tell validate_entry whether it is looking at the top-level envelope, where
`also` is legal, and drop the now genuinely redundant second check.

The three existing `also` tests did not catch this because they assert
invalidity and match on the substring "also", which the spurious error
contained -- they passed for the wrong reason. They now match on the
specific message, and there are tests for a valid envelope with `also`,
with an empty `also`, and for a genuinely unknown field still being
rejected. Verified the new tests fail against the unfixed validator.
Two more from dogfooding, the first of which defeated the whole point of
the dedup path.

The origin issue was excluded from the candidate pool unconditionally. That
is right for a placeholder this run just created, but wrong when the
notifier commented on an issue that already existed -- the case for every
recurrence after the first. That issue is the most likely duplicate, and
removing it left the model with an empty candidate list, so it answered
"new" and a duplicate issue was opened with a pointer comment: exactly what
this path exists to prevent. Only exclude the origin when we created it.

The validator also rejected an envelope for merely containing an
inapplicable key, even when its value was null. The schema sent to
OpenRouter is `strict`, so models return every declared property and null
what does not apply; this discarded good output and fell back to the plain
body. Treat null as absent, while still rejecting a real conflicting value.

Verified all three new tests fail against the unfixed script.
The model chose `action: comment` -- correctly, now that it can see the
matched issue again -- and returned a title, labels and issue_type
alongside. Those mean nothing for a comment and were never going to be
applied, but the validator treated their presence as an error, so the whole
response was discarded and the plain notice went out instead.

Drop them before validating and report what was ignored, rather than
failing. This is a deliberate loosening: the schema handed to the model is
`strict`, so it returns every declared property and fills whatever does not
apply to the branch it chose. Policing that costs us the enrichment and
buys nothing, since the applier only ever reads the fields for the action.
A conflicting value that we *would* act on is still an error.

Applies to `also` entries as well as the top-level envelope.
`locate_run_markers` looked the marker up with `gh search issues`. The
issue search index is not read-your-writes, and the notifier stamps its
marker moments before the enricher runs, so an unindexed marker reads as
"no notifier marker found" -- at which point main() takes its
missing-marker fallback and opens a *second* issue for a run that already
has one. Two threads for one failure, CI green, nothing in the log to say
why.

Scan the most recently updated issues first instead. The list endpoint has
no index lag, and the artefact the notifier just touched is by construction
among the most recently updated issues in the repo. Search stays as a
fallback for the one case a bounded listing cannot cover: more than
RECENT_ISSUE_SCAN issues updated in between, where a stale index still
beats no lookup at all.

Not passing the number forward from the notifier: the two stages are
separate workflow runs, so the only channels are an artefact or the
triggering run's log. Downloading an artefact from the triggering run is
precisely the thing the enricher's zizmor suppression argues it does not
do, and is not worth trading that argument away for.

The five new tests all fail against the unfixed lookup.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
`plain-fallback` created an issue unconditionally whenever `enrich` did not
set `handled`. But by the time it can run, the notifier has always already
notified: `workflow_run: completed` only fires once the caller's run,
including its `open-issue` job, has finished. So any `enrich` crash --
network down, `uv run` failing, an unhandled exception -- produced two
issues for one failure. It also caught `enrich` having applied its result
and then died before setting the output.

Look for `ai-failure-notifications:run=<id>:` and comment on that issue
instead, creating one only when the marker is genuinely absent, which is
the one case that means the notifier itself failed. The trailing colon
matches the notifier's `:origin=` and the enricher's `:sig=` alike, so both
cases above are covered.

Lists recently updated issues rather than searching, for the same
read-your-writes reason written out at locate_run_markers(): the marker is
minutes old, and reading a stale index as "no issue exists" is exactly the
duplicate this removes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tonyandrewmeyer
tonyandrewmeyer marked this pull request as ready for review July 26, 2026 23:41
@tonyandrewmeyer

Copy link
Copy Markdown
Collaborator Author

@benhoyt adding you as a reviewer specifically around the Zizmor/workflow question (feel free to review anything else as well, of course).

@benhoyt benhoyt left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've only reviewed the Zizmor change - the rationale for that looks reasonable to me.

@benhoyt

benhoyt commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Interesting idea, though! Let's see how it works.

@james-garner-canonical james-garner-canonical left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the high-level idea. I have a number of concerns with the way this PR proposes integrating the workflow+script and its tests into the existing CI -- mostly I wonder if we could simplify it by calling the workflow/script directly and trusting the script to handle the fallback branch.

I've read parts of the script itself -- in particular the system + user prompt seem very reasonable to me. I've held of on reviewing the script further because I imagine it might churn a fair bit if we make the surrounding infra changes I'm suggesting.

Comment thread test/test_ai_failure_notifier.py Outdated
Comment thread test/test_ai_failure_notifier.py Outdated
Comment thread test/test_ai_failure_notifier.py Outdated
from typing import Any
from unittest import mock

sys.path.insert(0, str(pathlib.Path(__file__).parent.parent / '.github'))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super thrilled by this approach: 1) including a test of unrelated infra in the unit tests for ops itself, and 2) requiring path manipulation to make it work (though that bit might be unavoidable). I'm not opposed to landing it this way, but I think we should at least have a rainy day item for running workflow/script specific tests in CI (and ideally locally too -- maybe easier once we land the Makefile move).

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is why the test didn't start out there. We do have test_infra already, which has a cross ops/ops-scenario documentation test, and previously has had stuff like "are there copyright headers".

I could move it elsewhere, like back into .github or maybe a .github/tests folder or a new top-level folder (or tests subfolder?) for more infra type stuff, and things like release script tests could end up there too.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking about it a bit more, I like the idea of a proper tested home for this, release.py,and the other github scripts. Like a top-level scripts folder with a test folder int here, which pytest would pick up automatically, and we could get rid of the path manipulation. Not being a dot directory would mean pytest would just work both with tox and make.

A PR for that first? Intro in this one and then move the others in a follow-up?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.github/tests sounds reasonable (and I could see us ending up in .scripts/tests or similar instead/as well if we move non-Github-CI specific stuff out of .github eventually). Happy for that to be a follow-up though, as long as we track it with an issue.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I sent my reply before seeing your second reply. I'd be on board with that approach too. Happy for either PR ordering.

Comment thread scripts/test/test_ai_failure_notifier.py
Comment thread test/test_ai_failure_notifier.py Outdated
Comment on lines +9 to +52
# `workflow_run` cannot target notify-scheduled-failure.yaml directly --
# reusable workflows invoked via `workflow_call` don't get an independent run
# for `workflow_run` to subscribe to. Instead this targets the *callers* (the
# scheduled workflows that `uses:` it); their overall conclusion is
# `failure` whenever the job that triggered notify-scheduled-failure.yaml
# failed, which is the same condition those callers gate the notifier call
# on. Keep this list in sync with any workflow whose
# `open-issue-on-failure-if-scheduled` job calls notify-scheduled-failure.yaml
# (grep .github/workflows/ for `notify-scheduled-failure` to check).

# zizmor flags every `workflow_run` trigger as dangerous, because the usual
# uses of it are: download an artifact from the triggering (potentially
# fork-controlled) run and trust it, or check out
# `github.event.workflow_run.head_sha` and execute it with this workflow's
# secrets and write token. Neither happens here:
#
# - no artifact is downloaded;
# - `actions/checkout` takes no `ref`, so it checks out `GITHUB_SHA`, which
# for `workflow_run` is the default branch tip, never the triggering run's
# ref, and it uses `persist-credentials: false`;
# - every `github.event.workflow_run.*` value reaches the script through
# `env:`, never interpolated into a `run:` block, so there is no template
# injection surface;
# - the jobs gate on `github.event.workflow_run.event == 'schedule'`, and a
# fork pull request produces `event == 'pull_request'`, so a fork cannot
# trigger this at all. Every run that reaches it originates from
# `schedule:` on the default branch.
#
# `workflow_run` is also not substitutable here: the notifier is a *reusable*
# workflow invoked via `workflow_call`, which gets no independent run for
# `workflow_run` to subscribe to, so this subscribes to its callers instead.
# The alternative is splicing an enrichment call into all seven callers, which
# is what the two-stage design exists to avoid.
#
on: # zizmor: ignore[dangerous-triggers]
workflow_run:
workflows:
- "Example Charm charmcraft test"
- "Example Charm Integration Tests"
- "ops Integration Tests"
- "ops Smoke Tests"
- "TIOBE Quality Checks"
- "Update Best Practices Doc"
- "Update Charm Pins"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the amount of explanatory text that this design ends up producing, I wonder if it would be better to make this workflow run on workflow_call, in one of the following ways:

  1. Have notify-scheduled-failure call this workflow at the end, and wire any required data through from the original call site.
  2. Just add a block to the workflows that use notify-scheduled-failure to call this workflow, like:
      enrich-failure-issue:
        needs: [open-issue-on-failure-if-scheduled]
        permissions:
          issues: write
        uses: ./.github/workflows/ai-failure-enrich.yaml

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Claude and I were wrong about the logs not being available, there was a different problem with my testing on my fork.

So we can do the workflow call approach. The catch is that it requires a secrets: inherit to get the OpenRouter API key passed through (one per workflow). That's also a Zizmor warning, so we would suppress 7 (in this repo) rather than one - but it's a lesser risk I think (the workflows that have a separate secret, like to update the charm pins, expose that to the LLM workflow).

I'm going to put the PR back to that approach.

@james-garner-canonical james-garner-canonical Aug 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Interesting, my brief reading of docs and stuff indicated that the logs were indeed unavailable from within a run.

Can we pass just the OpenRouter API key explicitly instead (like this) instead of using secrets: inherit?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verified on my fork, and you're right, we can pass it explicitly. All seven callers now pass OPENROUTER_API_KEY by name, the enricher declares it, and the repository is back to zero zizmor suppressions.

One weird thing: what each level passes is an empty string: no job outside the environment can read an environment secret, so the caller genuinely has nothing to hand over. The real value comes from environment: on the enrich job. But if a call site does not name the secret, the environment does not fill it in either, and the key arrives empty. Removing the "redundant" pass-through therefore breaks enrichment silently. Fork runs, if you want them: 32681905277 names it and the key arrives, 32682872399 does not and it is empty, and 32682645113 shows the caller holding length 0 while the enrich job holds the real length in the same run. I can't find this documented anywhere, so I'm not certain a it is meant to work like this, but it seems like it does...

Comment on lines +89 to +105
# Last-resort fallback, so a failure notification is never lost. The
# `enrich` job has its own internal fallbacks for a missing API key, an
# OpenRouter error or invalid model output; this covers the case where it
# never gets far enough to use them -- network fully down, `uv run` itself
# failing, an unhandled exception. Deliberately kept to `gh` alone, with no
# script and no secrets.
#
# `always()` plus checking `needs.enrich.outputs.handled` (rather than
# `needs.enrich`'s job status) means this fires both when `enrich` goes red
# AND when it somehow succeeds without having handled anything.
#
# By the time this can run, the notifier has already produced a
# notification: `workflow_run: completed` only fires once the caller's run,
# including its `open-issue` job, has finished. So creating an issue
# unconditionally means two issues for one failure. The only case that
# genuinely needs a new issue is the notifier itself having failed, and
# that is exactly what the absence of its marker says.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really need this? We're defending here against basic infra failing (uv run, random errors in our own script) -- I think we can just rely on the fallback in ai_failure_notifier.py itself, and live with only having the failing workflow if the job inexplicably fails without reaching the internal fallback.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered by this reply below.

gh issue create --repo "$REPO" \
--title "Scheduled workflow '$WORKFLOW_NAME' failed" \
--body "$issue_body"
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like it would be simpler to fold the dumb match and comment logic into the fallback branch of the enrich script and just call the enrich workflow here ... or even just fold the workflow in here and call the script directly. If the concern is making the AI parts easy to tear out or disable later, I think we could do that with clearly commented sections of input parameters and in the script itself; or perhaps factor the script into two separate modules.

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The enricher is now on: workflow_call and this workflow calls it after open-issue, so the workflow_run trigger, the hardcoded list of seven caller workflow names, and the zizmor: ignore[dangerous-triggers] suppression are all gone. I had tried something along those lines when experimenting with this on my fork but hadn't found this path, which doesn't seem better, thanks! (I'm double-checking it on the fork at the moment.)

I'd like to keep the coarse match as bash in its own job rather than folding it into the script:

  • open-issue currently runs with nothing but gh and github.token. Folding it in adds a checkout, setup-uv, and the environment as prerequisites for a notification happening at all.
  • enrich needs environment: ai-failure-triage for the OpenRouter key, and environments can carry protection rules including required reviewers. One job means the notification inherits that, so a misconfigured environment or an approval gate doesn't degrade the notification, it blocks it pending a human for a workflow whose whole purpose is telling people something broke overnight. Admittedly, we would presumably configure that in canonical-repo-automation so hopefully notice it, but it could happen.

The split is down to about 30 lines of bash now that it isn't also carrying the workflow_run argument, which feels ok for a notification path with no checkout, no uv and no environment on it. Happy to revisit if you still think it's not worth it, particularly once all the other changes are verified to be working.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current split and permissions rationale makes sense to me, thanks.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about notify outputing the issue number for us (commented on or created), which could simplify the ai_failure_notifier.py script since we wouldn't need to duplicate the issue lookup logic and can just treat it as an input?

If we do want enrich to run even if notify fails, this also lets us cleanly distinguish "we were passed a specific issue" from "we have to track down an issue on our own", so I think outputting the issue number could be helpful even if we end up needing to retain issue lookup logic in the script.

tonyandrewmeyer and others added 3 commits August 18, 2026 16:49
Drop the shebang and the `if __name__ == '__main__'` block: the test was
originally written to run standalone beside the script, but it now lives
in test/ and is collected by the normal unit run. Drop the inline script
metadata with it, since the `requires-python = ">=3.11"` pin it declared
would be wrong for a suite CI also runs on 3.10.

Wrap the two long fixture strings in explicit parentheses so their
continuation lines indent under the key rather than sitting at the dict's
own level. `ruff format --preview` leaves this form alone.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The enricher subscribed to the notifier's seven scheduled callers by name,
because `workflow_run` cannot target a reusable workflow directly. That
list had to be kept in sync by hand -- renaming a scheduled workflow would
have silently stopped enrichment -- and the trigger needed the repository's
only `# zizmor: ignore[dangerous-triggers]`, plus a long comment arguing
why the audit did not apply.

Call it from the notifier instead, after the `open-issue` job. Both stages
then run inside the caller's run, so `github.run_id` and `github.workflow`
name the scheduled workflow that failed, which is what the workflow_run
payload was supplying. The seven callers are unchanged.

Stage 1 keeps its guarantee: `open-issue` still needs no secrets, no LLM
and no network beyond `gh`, and it runs to completion before the enricher
starts. `if: always()` on the call means a failed stage 1 still reaches the
enricher's fallback.

zizmor now reports no findings on either file without any suppression.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The fallback job defended against the enricher dying before it could reach
its own internal fallback -- network down, `uv run` failing, an unhandled
exception. But stage 1 has already created or commented on the issue by
then, so an enricher that dies on its own leaves an un-enriched placeholder
and a red job, which is the same outcome as before any of this existed. No
notification is lost.

The case that genuinely needs an issue created is `open-issue` failing
*and* the enricher failing, since then nothing has notified anyone. Gate on
exactly that. Both stages now run in the same run, so `needs.*.result` says
directly whether a notification exists and the `gh issue list` marker
search that used to work it out is no longer needed.

The `handled` output existed only to drive the old condition, so it goes
too, along with `set_output()`, which had no other caller.

Also mark the script executable: it has a shebang, and the repo's other
shebanged .github scripts are all 100755. The pre-commit hook that checks
this only runs on files a commit touches, which is why it went unnoticed.

The enricher is 49 lines, from 151.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Comment on lines +62 to +63
# `always()` runs it even if stage 1 failed, since the enricher's own
# fallback handles a missing marker by creating the issue itself.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about this -- stage 1 doesn't look like it should fail on a missing marker, instead it creates the issue. If gh issue create fails I don't think we want to fall through to another attempt at it, do we?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair. It had already narrowed to firing only when open-issue and enrich had both failed. open-issue creates the issue; if that failed, the enricher finds no marker and creates it itself. Both failing leaves a red workflow, which is where we were before any of this existed.

@tonyandrewmeyer
tonyandrewmeyer marked this pull request as draft August 18, 2026 05:11
Comment on lines +58 to +61
# Stage 2. Called from here, rather than subscribing to this workflow's
# callers with `workflow_run`, so that there is no list of caller workflow
# names to keep in sync and no `dangerous-triggers` suppression to justify.
# `needs: [open-issue]` is what orders it after the placeholder exists;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Stage 2. Called from here, rather than subscribing to this workflow's
# callers with `workflow_run`, so that there is no list of caller workflow
# names to keep in sync and no `dangerous-triggers` suppression to justify.
# `needs: [open-issue]` is what orders it after the placeholder exists;

Comment on lines +71 to +73
# If stage 1 failed *and* the enricher never got far enough to use its own
# fallback, nothing has notified anyone -- the one case here that needs an
# issue created. An un-enriched placeholder is a red job, not a lost

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you've just marked this as a draft, but since I've already typed it: I'm likewise unconvinced by this -- why will gh issue create succeed here if it fails in open-issue? (Which probably needs a rename now.)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both fair. The fallback job is gone (see the other threads), and open-issue is now notify, since commenting on a match is the branch that matters most and the old name only described the other one. The callers' open-issue-on-failure-if-scheduled has the same problem, but it predates this PR and is in all seven of them, so I have left it rather than widening the diff.

This reverts the trigger change from d99b81d and the same-run form of the
fallback from f0ce3b7. Measured in the fork (runs 32101849632 and
32102193223): a job's logs are not retrievable through
`/actions/jobs/{id}/logs` while its *run* is still in progress, even 51
seconds after that individual job completed, and even with `actions: read`
granted. They become available once the run completes.

The enricher exists to read those logs, and under `workflow_call` it is
part of the run it needs to read. It cannot wait for the run either, since
the run cannot complete until it finishes. So `workflow_run` is not a
stylistic choice: it is the only trigger under which the logs exist. The
same applies to any in-run variant, including an enrich job added to each
caller.

The second run showed why this matters more than a red job would: with the
signature reduced to a job name, the model still produced a fluent,
confident diagnosis ("consistent with infrastructure timeout issues seen in
previous runs") for a run whose log was three named pytest failures. Green
job, plausible issue, invented content.

Two things from that work are kept:

- `actions: read` on the enrich job, which was missing all along. It is
  what the log fetch needs, and a `permissions:` block sets every scope it
  does not name to `none`.
- The narrower fallback: create an issue only when nothing has notified,
  rather than also commenting when enrichment failed. An `enrich` that dies
  on its own leaves an un-enriched placeholder, which is a red job, not a
  lost notification. `handled` and `set_output()` stay deleted; the job
  status plus the marker say enough.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tonyandrewmeyer

Copy link
Copy Markdown
Collaborator Author

FYI, I tried the alternative approaches in my fork and couldn't get them to work. The logs don't seem to be available until the (original) workflow finishes, so workflow_call can't get hold of them, and that means that it doesn't have the context to be able to do the enrichment. I'll see if I can find other alternatives but I suspect that moving back to the original approach may be needed.

@james-garner-canonical

Copy link
Copy Markdown
Contributor

FYI, I tried the alternative approaches in my fork and couldn't get them to work. The logs don't seem to be available until the (original) workflow finishes, so workflow_call can't get hold of them, and that means that it doesn't have the context to be able to do the enrichment. I'll see if I can find other alternatives but I suspect that moving back to the original approach may be needed.

Thanks, I saw the explanation in the revert commit message too. I think the inability to get the logs until the workflow completes is all the justification we need for the workflow_run approach.

The notifier script lived in .github/, so its tests could not live beside
it: pytest skips dot-directories, and anything under .github/ never runs in
CI. They went in test/ instead, alongside the tests for ops itself, and
reached the script through a sys.path insert.

Move both into a top-level scripts/, with the tests in scripts/test/, which
the normal unit run collects without any path manipulation --
`pythonpath = ["scripts"]` in pyproject.toml lets the tests import the
script by bare module name, since these are standalone scripts rather than
a package.

pyright's include gains scripts/test/*.py, matching the coverage the tests
had under test/*.py, and its extraPaths follows the script to scripts/. The
script itself stays outside include, as it was in .github/; putting a
1400-line script under strict mode is a separate change.

release.py and the other .github scripts belong here too, but that is a
follow-up rather than more churn in this PR.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tonyandrewmeyer and others added 7 commits August 24, 2026 10:54
The warning carried `gh exit 1` and nothing else, because only stdout was
captured from the call. Every failure looks the same through that: a 404
for a log that is not ready, a 403 for a token that has lost `actions:
read`, and a transient 5xx all exit 1, and the two that matter want
opposite fixes.

This is not hypothetical. The fork run that produced the workflow_call
revert (32102193223) logged exactly that bare line, so the evidence for
"the log does not exist until the run completes" rests on an exit code
that cannot distinguish it from a permissions problem.

`gh` puts the status on stderr ("gh: Not Found (HTTP 404)"), which the
subprocess call already captures. Fold it into the summary, whitespace-
collapsed to one line and capped, and say "no stderr" when there was none.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
From gh 2.9x, `gh api` refuses to write a response containing terminal
escape sequences: "the response contains terminal escape sequences; pass
--allow-escape-sequences to output it anyway". It writes nothing and exits
non-zero. Actions logs are full of escapes -- the ANSI pattern near the top
of this file exists to strip them -- so every log fetch hit this, and the
enricher has never once read a log in CI.

Measured in fork run 32673538357, from the enrich job, with the run still
in progress: `curl` returned HTTP 200 and all 8242 bytes; `gh api` on the
same URL, same token, seconds later, returned nothing and that message. The
runner had gh 2.97.0.

This is what the workflow_call revert misread. The enricher was falling
back to a job-name-only signature every time, so the model was writing
confident diagnoses from nothing -- which the revert recorded, and
attributed to the trigger. Logs turn out to be readable in-run: five fork
runs now read one back mid-run, three from a plain job (32672640478,
32672780488, 32672784971) and two from inside the nested workflow_call
chain (32673244396, 32673538357), between 5 and 37 seconds after the
failing job finished.

Local gh 2.45.0 has no such check, which is why this never showed up
outside CI. It also has no such flag, and rejects it as unknown rather than
ignoring it, so an "unknown flag" stderr retries without it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The enricher subscribed to the notifier's seven scheduled callers by name,
because `workflow_run` cannot target a reusable workflow directly. That
list had to be kept in sync by hand -- renaming a scheduled workflow would
have silently stopped enrichment -- and the trigger needed the repository's
only `dangerous-triggers` suppression, plus forty lines arguing why the
audit did not apply.

Call it from the notifier instead, after `open-issue`. Both stages then run
inside the caller's run, so `github.run_id` and `github.workflow` name the
scheduled workflow that failed, which is what the workflow_run payload was
supplying.

This was tried once before and reverted, on the grounds that a job's logs
cannot be read while its run is still in progress. That is not true. Five
fork runs read one back mid-run, 5 to 37 seconds after the failing job
finished: three from a plain job (32672640478, 32672780488, 32672784971)
and two from inside this nested chain (32673244396, 32673883720). The
actual fault was `gh api` refusing to emit a response containing terminal
escapes, fixed in the previous commit; the enricher had never read a log in
CI, under either trigger.

What is true, and is the cost of this shape: a called workflow holds no
scope and sees no secret its caller did not hand down. So the seven callers
now grant `actions: read` for stage 2's log fetch, and inherit secrets so
the OpenRouter key configured on the `ai-failure-triage` environment
actually arrives -- measured empty without it in run 32673883720, and
present in 32102193223 with it. `environment:` names where the key lives
but does not deliver it to a called workflow.

That trades one `dangerous-triggers` suppression for seven
`secrets-inherit` ones, and gives stage 2 every secret its caller can see
rather than only the one it needs. Both stages are in-repo code, so the
blast radius is bounded, but the environment scoping the enricher's comment
used to claim is gone and the comment now says so.

Stage 1 keeps its guarantee: `open-issue` still needs no secrets, no LLM
and no network beyond `gh`, and runs to completion before the enricher
starts. `if: always()` means a failed stage 1 still reaches the enricher's
fallback, and `plain-fallback` still covers both failing at once.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The runner opens every step with "##[group]Run <script>", echoes the whole
`run:` block a line at a time, dumps the step's env, and closes the group.
None of that is output. The parser read it all as if the step had printed
it, so a multi-line `run:` contributed its every branch to the signature,
including branches that did not execute.

Caught end to end on the fork (run 32674850519). The canary's `run:` is a
`case` with a sample failure per shape, so a pytest-shape run produced
`traceback_top_error: KeyError: 'loki/0'` -- text from the traceback-only
branch -- and each of the three real pytest failures twice, once echoed and
once printed. The model then wrote, accurately for what it was given, that
the run "encountered the same KeyError ('loki/0')". It never did.

Colour cannot separate the two: the runner marks echoed lines cyan-bold,
but ANSI strips that before anything is matched. The group boundary can,
and dropping the header group takes the env dump with it.

The seven callers all run one-liners today, so this changes nothing for
them yet. It matters because a wrong signature does not degrade the output,
it makes it confidently wrong, and a `run:` block only has to grow to two
lines to start feeding the model its own source.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
James asked whether the key could be passed explicitly instead of with
`secrets: inherit`. It can, and it costs nothing: the seven
`secrets-inherit` suppressions come out, the repository is back to none at
all, and stage 2 stops receiving secrets it has no use for, including the
charm-pin and docs tokens.

The key also stays on the `ai-failure-triage` environment. Demoting it to a
repository secret, which is what "pass it explicitly" first looked like it
would need, turns out to be unnecessary.

The mechanics are unintuitive and the comments say so at each call site,
because the obvious tidy-up breaks it silently. What every level passes is
an empty string: no job outside the environment can read an environment
secret. The real value comes from `environment:` on the enrich job. But
unless each call site names the secret anyway, the environment does not
fill it in either, and enrichment degrades to a plain notice with nothing
failing loudly.

Measured on the fork rather than derived: 32681905277 names it and the key
arrives, 32682872399 does not and it is empty, 32682645113 shows the caller
holding 0 while the enrich job holds the real length in the same run. No
documentation found saying it should work this way, which the comment also
records.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The job existed to cover `enrich` dying before it could reach its own
fallback. It has been narrowed twice since, and now fires only when
`open-issue` and `enrich` have both failed, which is to say when
`gh issue create` has already failed once and the enricher never got far
enough to try it again. Its answer to that is a third attempt, in a third
job, on the same API. There is no reason to think the third succeeds where
the first did not.

There are two layers left for one notification, which is enough:
`open-issue` creates the issue, and if that failed the enricher finds no
marker and creates it itself. Losing both leaves a red workflow, which is
where we were before any of this existed.

Twenty-three lines and one job per scheduled failure.

The one case that argued for keeping it: if `ai-failure-triage` ever gains
required reviewers, `enrich` blocks pending approval, so a coincident
`open-issue` failure would leave nobody notified until someone approved.
That is a reason not to put protection rules on that environment, not a
reason to keep a job.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
It has not only opened issues since the coarse dedupe went in: on a match
it comments instead, which is the branch that matters most for not filing
duplicates. `notify` says what the job is for and stays true whichever
branch runs, and the step name under it still spells out the mechanism.

Caught by James in review.

The callers' own `open-issue-on-failure-if-scheduled` is inaccurate in the
same way, but it predates this work and lives in all seven of them, so it
is left alone rather than widening the diff.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tonyandrewmeyer

Copy link
Copy Markdown
Collaborator Author

The infra concerns should now be addressed, and in the direction you (James) suggested: the enricher is called via workflow_call from the notifier, the workflow-level fallback is gone so the script's own fallback is the only one, and the script and its tests moved to a top-level scripts/ with scripts/test/, which the normal unit run collects with no path manipulation. The repository is also back to zero zizmor suppressions after the secrets: change on the other thread.

That should be the churn done, so the script itself is ready whenever you have time for it. I'm just going to do one more review pass myself and then will move out of draft.

tonyandrewmeyer and others added 4 commits August 24, 2026 15:22
The inline metadata claimed `requires-python = ">=3.11"`. Nothing in the
script needs it: the imports are all stdlib, the repo targets 3.10 in both
ruff and pyright, and CI runs the tests that import this on 3.10, so the
pin has been contradicted by a passing test matrix the whole time.
Confirmed directly under 3.10.20: the module imports and all 70 tests pass.

The block held nothing else, no dependencies, so it goes entirely rather
than being left as an empty header. `uv run` then uses the ambient
interpreter, which is what the callers already give it.

The same pin came out of the test file in 29a2f84 for the same reason. It
should have come out of both.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The comment explains why the runner's own step script has to be skipped.
How that was found, on a fork canary whose `case` statement carried a
sample traceback per shape, does not help anyone reading the parser and
points at a fixture that exists nowhere in this repository. The mechanism
above it is the part worth keeping.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Same as the previous commit, for the test that covers it. What the comment
needs to say is why an unexecuted branch must not reach the signature, not
which fork run happened to expose it.

The fixture keeps its `case` statement and its stray traceback: that is the
shape being tested, and it reads clearly enough without the provenance.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tonyandrewmeyer
tonyandrewmeyer marked this pull request as ready for review August 24, 2026 03:34

@james-garner-canonical james-garner-canonical left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for all the work and investigation on this. I like the shape of the workflows now. I've read through them and left comments -- mostly cutting down excessive LLM comments. The only scheduled workflow I commented on is smoke.yaml, but my comments there apply equally to all the other scheduled workflows.


I have two suggestions for hand-off from notify to enrich:

  1. notify could output the issue number, and enrich could take it as explicit input, so it doesn't need to fish through recent issues looking for a match -- if it doesn't get an issue number as input, then notify definitely failed.
  2. If notify fails, I still think that bailing out is fine because we wouldn't expect gh issue comment/create to suddenly pass in the next job.

If you're not convinced by those suggestions, then we can stick with the current approach.


I haven't read much of ai_failure_notifier.py yet, just because it's 1500 LOC of code that's tricky to follow on Github.

I'm not sure what the best move here is:

  • The various chunks of code do seem to be semantically grouped, but maybe it would be easier if collections of functions were grouped onto classes (even as static/class methods), or split into separate modules?
  • Maybe just arranging things in a review-friendly ordering would be enough (main, then what main calls, and so on, with related functions grouped into sections ordered similarly)?
  • Maybe a thorough read-through of the code isn't necessary here -- this isn't library code, so I'm not intending to review it to that standard, but do we still want someone to have read it all?

If you'd like to keep the script as-is, I'll try to read through it in an IDE instead of just on Github.

Comment thread .github/workflows/notify-scheduled-failure.yaml Outdated
# not a reason to believe the fourth would work.
enrich:
needs: [notify]
if: ${{ always() }}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this should run if notify fails. notify already just looks for a match, updates if it finds it, or creates a new issue if it doesn't. I don't see anything there that we'd expect to fail or be able to do something particularly interesting if it did (e.g. Github authentication failure, infra is down).

Suggested change
if: ${{ always() }}

# A called workflow cannot hold a scope its caller did not grant, so
# `actions: read` has to be repeated at every level of the chain for
# the enricher's log fetch to have it.
actions: read

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to document the Github workflow permission model here -- that a called workflow must have the required permissions granted by the caller. Also, the enricher workflow only has a single job (with that job requiring actions: read) so I'm not convinced that we need to document here that it's for log fetching.

Suggested change
actions: read

Comment on lines +92 to +97
# This passes an empty string, and is still required. Nothing in this
# chain can read the OpenRouter key: it lives on stage 2's
# `ai-failure-triage` environment, and only the job declaring that
# environment resolves it. But unless the secret is named at each call
# site, stage 2's own environment does not fill it in either, and
# enrichment degrades to a plain notice with nothing failing loudly.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They key piece of information here is that you require both the secret the environment to read this specific secret. But does that need to be documented here?

Suggested change
# This passes an empty string, and is still required. Nothing in this
# chain can read the OpenRouter key: it lives on stage 2's
# `ai-failure-triage` environment, and only the job declaring that
# environment resolves it. But unless the secret is named at each call
# site, stage 2's own environment does not fill it in either, and
# enrichment degrades to a plain notice with nothing failing loudly.

Comment thread .github/workflows/smoke.yaml Outdated
Comment on lines +53 to +56
# Stage 2 reads this run's job logs, and a called workflow cannot hold
# a scope its caller withheld, so `actions: read` has to be granted
# here even though nothing in this file uses it.
actions: read

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
# Stage 2 reads this run's job logs, and a called workflow cannot hold
# a scope its caller withheld, so `actions: read` has to be granted
# here even though nothing in this file uses it.
actions: read
actions: read # for ai-failure-enrich.yaml

or

Suggested change
# Stage 2 reads this run's job logs, and a called workflow cannot hold
# a scope its caller withheld, so `actions: read` has to be granted
# here even though nothing in this file uses it.
actions: read
actions: read

gh issue create --repo "$REPO" \
--title "Scheduled workflow '$WORKFLOW_NAME' failed" \
--body "$issue_body"
fi

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WDYT about notify outputing the issue number for us (commented on or created), which could simplify the ai_failure_notifier.py script since we wouldn't need to duplicate the issue lookup logic and can just treat it as an input?

If we do want enrich to run even if notify fails, this also lets us cleanly distinguish "we were passed a specific issue" from "we have to track down an issue on our own", so I think outputting the issue number could be helpful even if we end up needing to retain issue lookup logic in the script.

Comment thread pyproject.toml
Comment on lines +111 to +115
[tool.pytest.ini_options]
# The workflow scripts in scripts/ are standalone files, not a package, so
# their tests in scripts/test/ import them by bare module name.
pythonpath = ["scripts"]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this would be more cleanly isolated in scripts/test/conftest.py, WDYT?

@@ -0,0 +1,1461 @@
#!/usr/bin/env python3
#

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We use uv run so we could drop the shebang here (and this wouldn't need to be executable). Alternatively, we could use a uv shebang here and then execute this script directly in ci (run: scripts/ai_failure_notifier.py).

Comment on lines +1246 to +1249
# The marker lookup is the first thing main() does, so an uncaught
# failure here takes out the whole enrich job and hands every run to
# the workflow-level plain-fallback -- losing enrichment silently
# rather than degrading through the script's own fallback path.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment seems outdated.

Comment on lines +1274 to +1276
if origin_issue is None:
# Shouldn't happen -- the notifier always stamps a marker -- but
# don't lose the notification if it does.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess this is where we'd branch on explicit input if we had notify output the issue number.

Co-authored-by: James Garner <james.garner@canonical.com>
@tonyandrewmeyer
tonyandrewmeyer marked this pull request as draft August 26, 2026 00:10
The notify job now outputs the issue it created or commented on, and enrich
takes it as an input, so the script is told which artefact to upgrade instead
of searching for the marker notify stamped moments earlier. GitHub's issue
search index is not read-your-writes, so that search could miss the marker and
open a second issue for a run that already had one.

Both inputs are optional and enrich still runs on always(), so a notify that
fails before opening anything leaves them empty and the script falls back to
looking the issue up, as it does today.

Also drops a duplicated `issues: write` from the enrich job's permissions,
which yamllint reports as an error, and moves the comment that explains
`actions: read` to sit above it rather than above the key it is not about.
The trailing whitespace fix on line 5 is pre-commit's, not mine.
tonyandrewmeyer added a commit to canonical/charm-tech-code that referenced this pull request Aug 26, 2026
…r it

Review suggestion on canonical/operator#2663: have the notify job output the
issue it created or commented on, so this script is told which artefact to
upgrade rather than looking it up.

The lookup it replaces existed to work around GitHub's issue search index not
being read-your-writes: the notifier stamps its marker seconds before the
enrich job runs, so a search can read "no marker found" and open a second
issue for a run that already has one. A number passed through the workflow
cannot be stale, so that failure mode is gone rather than defended against.

It does not remove the lookup entirely, which the suggestion allowed for.
Rung zero is a fact about an earlier run of this script, not about what the
notifier just did, so it still has to be looked up - but knowing the issue
narrows that from a scan of the repo's recently updated issues to reading the
one issue we were handed. With no issue passed, from an unmigrated caller or
a notifier that failed before opening anything, the original repo-wide scan
still runs.
Review suggestion: enrich ran on always(), so a failed notify still reached
it and the script's own fallback opened the issue instead. Both jobs
authenticate the same way and both shell out to gh, so the failures that stop
notify - authentication, infra being down - stop enrich as well, and the
fallback was defending against a class of failure it cannot actually survive.

What it does give up is the narrow case of a transient failure in notify's
issue search, where the fallback would have produced a notification and now
nothing will: the scheduled workflow just goes red, which is where this was
before any of it existed.

The script keeps its lookup fallback regardless. Inside this repository it is
now unreachable, but repositories adopt this at their own pace and a caller
that has not been migrated passes no issue number at all.
Review suggestion. That a called workflow holds no scope its caller withheld
is documented upstream, and the enricher has a single job, so saying there
that its actions: read is for fetching logs is not telling a reader anything
the next few lines do not.

Removed from both workflows for the same reason, not just the one that was
commented on.

The secrets comment below stays, and the difference is the point: it records
behaviour that is not documented anywhere and was measured across three fork
runs, where removing the pass-through it describes silently degrades
enrichment rather than failing.
Review suggestion asked whether this needs documenting here. It does not: the
same fact was written out in both workflows, and this was the shorter, vaguer
copy, in the file that does not declare the environment it is about.

The full note stays in ai-failure-enrich.yaml, next to the environment: key
and the fork runs that measured it. What is left here is one line saying the
pass-through is required and where to read why, which is the part that stops
someone removing a line that provably passes an empty string.
Review suggestion, applied to all seven callers rather than only the one it
was left on. Each of them carried the same eleven lines explaining GitHub's
permission model and the secret mechanics, so the fact was written out nine
times across this PR once the two called workflows are counted.

What a caller actually needs to know is that both lines exist for the
enricher and that the key only resolves inside its environment. That is two
comments. The mechanics are documented once, in ai-failure-enrich.yaml.
Review suggestion, taken as offered, plus the terse numbered list it proposed
in place of the two-stage paragraph.

The permission-model sentence goes for the same reason as the others: it
describes GitHub, not this workflow. The numbered list keeps the one claim
worth keeping, that notify is the guarantee and enrich is best effort, which
is why they are separate jobs at all.
tonyandrewmeyer added a commit to canonical/charm-tech-code that referenced this pull request Aug 26, 2026
Review suggestion on canonical/operator#2663, where the choice was between
dropping it and switching to a uv shebang so CI could execute the file
directly. Moving here settles it: this is a module inside an installed
package, reached through the ai-failure-notifier console script, so nothing
executes it by path and the line is dead text. The file was already not
executable.
tonyandrewmeyer added a commit to canonical/charm-tech-code that referenced this pull request Aug 26, 2026
Asked for in review on canonical/operator#2663: 1500 lines is hard to follow
on GitHub, and the reviewer offered to read it in an IDE instead if we would
rather leave it. Splitting is the better answer, and it is cheap here in a way
it was not in operator - there is no in-flight review of these files to
disturb.

The boundaries are the ones the single file already documented with its
`# --- section ---` banners, plus the I/O half divided by what it talks to:
gh, OpenRouter, the step summary, and applying the result. Largest module is
now 293 lines. `__init__` re-exports every public name, so
`from charm_tech_code import ai_failure_notifier` is unchanged for callers.

Cross-module function calls go through the module rather than importing the
name, so that a test patching `<module>.<name>` reaches every call site
instead of only the definer. Those imports are aliased with a leading
underscore because three module names - envelope, prompt, summary - are also
local variable names in the code.

No assertion changed. The test diff is entirely patch targets moving from
`afn.<name>` to `afn.<module>.<name>`, which is what makes the same 75 tests
evidence that this refactor preserved behaviour.
@tonyandrewmeyer

Copy link
Copy Markdown
Collaborator Author

I've moved most of this to canonical/charm-tech-code#1 (I still need to update this PR to reflect that), and tried to address all the comments here in that PR since I can't move the comments across.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants