ci: enrich scheduled-failure issues with an LLM triage pass - #2663
ci: enrich scheduled-failure issues with an LLM triage pass#2663tonyandrewmeyer wants to merge 32 commits into
Conversation
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.
9371af3 to
41325bb
Compare
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>
|
@benhoyt adding you as a reviewer specifically around the Zizmor/workflow question (feel free to review anything else as well, of course). |
benhoyt
left a comment
There was a problem hiding this comment.
I've only reviewed the Zizmor change - the rationale for that looks reasonable to me.
|
Interesting idea, though! Let's see how it works. |
james-garner-canonical
left a comment
There was a problem hiding this comment.
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.
| from typing import Any | ||
| from unittest import mock | ||
|
|
||
| sys.path.insert(0, str(pathlib.Path(__file__).parent.parent / '.github')) |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
.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.
There was a problem hiding this comment.
I sent my reply before seeing your second reply. I'd be on board with that approach too. Happy for either PR ordering.
| # `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" |
There was a problem hiding this comment.
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:
- Have
notify-scheduled-failurecall this workflow at the end, and wire any required data through from the original call site. - Just add a block to the workflows that use
notify-scheduled-failureto call this workflow, like:enrich-failure-issue: needs: [open-issue-on-failure-if-scheduled] permissions: issues: write uses: ./.github/workflows/ai-failure-enrich.yaml
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
| # 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. |
There was a problem hiding this comment.
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.
| gh issue create --repo "$REPO" \ | ||
| --title "Scheduled workflow '$WORKFLOW_NAME' failed" \ | ||
| --body "$issue_body" | ||
| fi |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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-issuecurrently runs with nothing butghandgithub.token. Folding it in adds a checkout, setup-uv, and the environment as prerequisites for a notification happening at all.enrichneedsenvironment: ai-failure-triagefor 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.
There was a problem hiding this comment.
The current split and permissions rationale makes sense to me, thanks.
There was a problem hiding this comment.
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.
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>
| # `always()` runs it even if stage 1 failed, since the enricher's own | ||
| # fallback handles a missing marker by creating the issue itself. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
| # 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; |
There was a problem hiding this comment.
| # 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; |
| # 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 |
There was a problem hiding this comment.
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.)
There was a problem hiding this comment.
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>
|
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 |
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>
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>
|
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. |
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>
james-garner-canonical
left a comment
There was a problem hiding this comment.
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:
notifycould output the issue number, andenrichcould 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, thennotifydefinitely failed.- If
notifyfails, I still think that bailing out is fine because we wouldn't expectgh issue comment/createto 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.
| # not a reason to believe the fourth would work. | ||
| enrich: | ||
| needs: [notify] | ||
| if: ${{ always() }} |
There was a problem hiding this comment.
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).
| 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 |
There was a problem hiding this comment.
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.
| actions: read |
| # 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. |
There was a problem hiding this comment.
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?
| # 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. |
| # 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 |
There was a problem hiding this comment.
| # 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
| # 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 |
There was a problem hiding this comment.
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.
| [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"] | ||
|
|
There was a problem hiding this comment.
I wonder if this would be more cleanly isolated in scripts/test/conftest.py, WDYT?
| @@ -0,0 +1,1461 @@ | |||
| #!/usr/bin/env python3 | |||
| # | |||
There was a problem hiding this comment.
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).
| # 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. |
There was a problem hiding this comment.
This comment seems outdated.
| if origin_issue is None: | ||
| # Shouldn't happen -- the notifier always stamps a marker -- but | ||
| # don't lose the notification if it does. |
There was a problem hiding this comment.
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>
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.
…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.
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.
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.
|
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. |
Today, when a scheduled workflow fails,
notify-scheduled-failure.yamlopens 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 beyondgh, 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 thenotifyjob 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: readfor the log fetch, andOPENROUTER_API_KEYpassed 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 inscripts/test/where the normal unit run collects them. The alternative was the script in.github/, where pytest never looks, and its tests intest/alongside the tests foropsitself, reaching it through asys.pathinsert.release.pyand the other.githubscripts belong here too, but that is a follow-up rather than more churn in this PR.Dedupe ladder
: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.newenvelope on a fresh placeholder:gh issue editthe placeholder in place, rather than opening a second issue.newenvelope 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:
ghsearch failures degrade to "no marker" or "no candidates" rather than raising.There is deliberately no third, workflow-level fallback job. Reaching one would mean
gh issue createhad already failed innotifyand 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. Anenrichthat 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-triageenvironment, 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 fromenvironment: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 sevensecrets-inheritsuppressions, 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_TOKENamong them. Thanks to @james-garner-canonical for pushing on this.Testing
The read-only
ghlayer 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 producedtraceback_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
ai-failure-triageenvironment and itsOPENROUTER_API_KEYneed 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_MODELrepo variable: the script defaults todeepseek/deepseek-chatif unset, worth pinning explicitly once the model choice is confirmed against live traffic.