feat: add an ai-failure-notifier workflow and script by tonyandrewmeyer · Pull Request #1 · canonical/charm-tech-code

tonyandrewmeyer · 2026-08-26T02:14:08Z

Moves the scheduled-failure triage script out of canonical/operator, where it was 2,692 lines that every adopting repo would otherwise have had to copy, and leaves only workflow YAML behind in each repo.

One directory per tool.
Consumed by uvx --from "git+https://github.com/canonical/charm-tech-code@<sha>#subdirectory=ai-failure-notifier", so there is no release process and the SHA is the version.

The script is 1,461 lines and its tests another 1,231, and canonical/operator is only the first repository meant to run it. Copying that into each adopting repo means fixing every bug as many times as there are repos, so it moves here and each repo keeps only the workflow YAML that differs. Both files are verbatim from canonical/operator#2663 at f15bcf1e, with exactly one line changed: the test's `import ai_failure_notifier as afn` becomes `from charm_tech_code import ai_failure_notifier as afn`. Nothing else in either file is touched, so the 70 tests passing here are the same 70 assertions that passed there. Splitting the script into modules is the next commit, kept separate so this one stays a move and that one stays a refactor. The package has no runtime dependencies, which is what makes `uvx --from git+` viable as the distribution mechanism with no release process to run.

Matches the Python versions canonical/operator tests against, since that is where this code came from and where it runs. Actions are SHA-pinned to the same revisions the calling workflow already pins. Lint runs but the formatter does not, deliberately: the moved file is byte-identical to what is under review in operator#2663, and turning the formatter on now would put a 230-line diff on it.

One line per paragraph rather than hard breaks at 76 columns, so diffs on a reworded sentence stay one line instead of reflowing the paragraph around it.

A single top-level src/ and tests/ only works while there is one tool in here, and the whole argument for this repo existing is that there will be more. canonical/charmlibs already solved this - a directory per package, each with its own pyproject.toml, src/, tests/ and lockfile - so this follows that rather than inventing a second convention for the team to remember. The concrete win is dependency isolation. ai-failure-notifier has no runtime dependencies and is invoked by uvx on a runner after a scheduled workflow has already failed, so anything a future tool needs would otherwise be installed on every failed run of a workflow whose point is to work when things are broken. The formatter is on again rather than dropped. The settings were never the problem: operator runs `ruff format --preview`, and the preview style hugs brackets inside calls, which is why the moved file appeared to need 230 lines of reformatting. Ruff's config is copied from operator into the root pyproject.toml with preview set there instead of passed as a flag, so an editor and CI agree without anyone remembering it. Packages carry no [tool.ruff] block of their own, because ruff takes the closest config rather than merging and a local one would quietly override the shared one. Lockfiles are committed now, and CI syncs with --locked so that a lockfile which has drifted from its pyproject.toml fails instead of silently resolving to something else.

…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 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.

Review comment: it was outdated. It justified the try/except by pointing at the workflow-level fallback job, which was removed earlier in the same review at the reviewer's suggestion, so it sent a reader looking for a safety net that no longer exists. The except is still needed, for the opposite reason: nothing catches this above us now, so an uncaught failure loses the enrichment outright. Also notes what the recovery actually does since the notifier started passing its issue number through.

Review comment guessed correctly that this is where an explicit input would branch, which is now what happens. That makes the old comment wrong in a quiet way: "shouldn't happen -- the notifier always stamps a marker" was true when this depended on finding one by search, but reaching here now means either an unmigrated caller or a failed lookup, and the first is the normal state of a repository part way through adopting this.

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.

Splitting this package into modules moved `gh` from the module the tests patch into `github.py`. `mock.patch.object` went on succeeding - the attribute was still there - while no longer intercepting what the call sites resolved, so the suite ran the real `gh` as whoever invoked it. It opened two issues and posted two comments on a live repository before that was noticed, and the only symptom was the run taking 35 seconds rather than a tenth of one. Two independent fixes, either of which would have been enough: conftest.py replaces subprocess.run/Popen/call/check_call/check_output and urllib.request.urlopen with functions that raise, for every test, so a missing or misdirected mock fails at the boundary and names the command it was about to run. Verified by reintroducing the exact bug: the suite fails in 0.35s quoting `gh issue create --repo ... --title t`, instead of succeeding. The fixtures no longer name a real repository. Every `canonical/operator` in the tests is now `example/repo`, so even a total patch failure has nowhere real to write. The one mention left is prose in a docstring describing an actual past run.

Only re-export names actually consumed from outside the package: the console-script entry point (main) and the names the test suite reaches via `afn.<name>`. Everything else is now imported from its own module directly, as the rest of the package already does internally. While tracing consumers, found two test mocks patching the trimmed package-level names (`fetch_run_meta`, `call_openrouter`) rather than the submodule the real call sites resolve against -- exactly the footgun tests/conftest.py's no_real_side_effects fixture exists to catch. Repointed them at afn.github / afn.openrouter to match their sibling patches in the same blocks.

…ders These "--- Section ---" comments marked boundaries within the original 2,700-line script and just repeat what the module split already says now that each one is its own file. Removed across the package; left comments that explain a design decision alone.

…ring The hand-rolled-validation rationale was a comment sitting right below the module docstring, saying the same kind of thing a docstring is for. Folded it in.

main() was a 250-line function covering config loading, origin resolution, signature building, the LLM round trip, and applying the result. Split into private helpers named for what each stage does (_read_config, _resolve_origin, _build_run_signature, _fetch_envelope, _apply_envelope, ...), with a _RunConfig dataclass to carry the env-derived settings between them instead of a long parameter list. Pure refactor: no behaviour change. The three call sites that built an identical plain-fallback entry dict (no API key / OpenRouter call failed / LLM output failed validation) now share one _plain_fallback_entry helper -- same dict, same apply_entry call, one definition instead of three copies.

The package __init__ re-exported 30 names across every submodule, which kept the flat namespace the single-file script had. That was never the point of the split, and the earlier trim preserved it because the test suite reached each name as afn.<name> -- the tests were written against the old shape, so satisfying them was entrenching what the split was meant to undo. __init__ now exports main, the console-script entry point, and nothing else. The tests reach each name through its owning submodule instead, matching what the mock.patch.object targets already did for github, summary, openrouter and prompt.

…ports cli.py, apply.py and github.py imported their siblings as `github as _github`, `summary as _summary` and so on. Aliasing a module inside its own package reads as though the name were private, which it isn't, and the tests then patch the un-aliased `afn.github` anyway. github, openrouter, prompt and summary stay module-object imports, because the suite patches them with mock.patch.object and a name import would resolve past the patch -- the failure conftest's no_real_side_effects fixture exists to catch. The rest import the names they use, which also removes the two shadowing hazards that made the aliases look necessary: cli.py's local `envelope` and github.py's local `markers` no longer collide with a module of the same name.

tonyandrewmeyer · 2026-08-30T21:01:17Z

@james-garner-canonical I've tried to address all the feedback from the PR in operator here.

james-garner-canonical

Had a quick look, but please give additional guidance on what and how to review.

james-garner-canonical · 2026-09-01T03:43:19Z

+# How many recently-updated issues to scan for the notifier's marker. The
+# artefact we are looking for was touched minutes ago, so this only has to
+# cover issue churn in that window; 50 is far more than `operator` sees.
+RECENT_ISSUE_SCAN = 50


In the other PR I suggested that this should be an input -- if we updated an issue or created a new one, we know which one, otherwise we know there isn't an update to look for.

This is already an input: resolve_origin() takes notify_issue/notify_origin, which come from the NOTIFY_ISSUE/NOTIFY_ORIGIN env vars set by the workflow. When the notifier tells us which issue it created or commented on, we read that one issue and never scan.

RECENT_ISSUE_SCAN only bounds the fallback for when we're not told: the notifier step failed, or a caller hasn't been migrated to pass the output through yet.

The half that isn't here is the workflow YAML that sets the env vars: that stays in each consuming repo (only the operator PR for now), which is maybe why it looked like the input didn't exist.

Env var is fine, though an explicit, required CLI argument could make things clearer. Either way, I'd still like to slim this down and have this script implement a single easy to follow control flow rather than having fallbacks like this.

Rename all twelve modules to `_`-prefixed names and drop the `main` re-export from `__init__.py`, so nothing in the package is reachable by import. The tool is consumed as a console script, not as a library, and the point is to make it awkward for a future CI hack to reach in and depend on an internal. The entry point moves to `_cli:main` accordingly, and the test suite imports the private modules explicitly rather than relying on `__init__` to have pulled them in as attributes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012U3sSiKZahNuah4eDysMku

Ruff already resolved to the monorepo root by discovery, and a comment said so. Extending it instead makes the dependency explicit, and means that adding a `[tool.ruff]` setting here later overrides one key rather than silently replacing the whole shared config. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012U3sSiKZahNuah4eDysMku

tonyandrewmeyer · 2026-09-08T01:14:15Z

Had a quick look, but please give additional guidance on what and how to review.

Well, it's up to you at the end of the day. It's what you are comfortable with (a) getting executed from other charm tech repo CI and (b) the team maintaining. I do think we should consider this repo's content things for our team to use with no guarantee of support for anyone else that starts using them. Maybe there should be some file that makes that explicit.

If you want suggestions on where to focus effort, I'd suggest the below. But again, up to you to choose how to review.

Probably not worth reading closely: the body of ai-failure-notifier/src/ and tests/. That's the 2,692-line script from operator, split into modules; the logic and the 75 tests came across as-is. If you read it there, then you can just check or trust it's the same. Up to you how high quality you feel this code needs to be.

Worth a proper look:

The repo shape — one directory per tool, each with its own pyproject.toml/src//tests/.
Consumption via uvx --from "git+https://github.com/canonical/charm-tech-code@<sha>#subdirectory=ai-failure-notifier", so there's no release process and the SHA is the version.
The shared ruff config at the root, and each tool extending it.

Where the real risk is: resolve_origin() in _github.py and the read-your-writes hazard the docstring describes. That's the part that, if wrong, silently opens duplicate issues, although I guess we would notice fairly fast.

Four conventions settled on canonical#1 that apply here too, so that the second package in the repo does not arrive with a different set. * Apache licence header on every Python file, which none of these had. * Modules are private (`_cli`, `_common`, `_tier`, `_checks`, `_fixes`), so it stays hard to depend on tool internals from a CI hack later. * `__init__.py` is a docstring and nothing else -- the tool is a console script, not something to import -- with the entry point moved to `._cli`. * `[tool.ruff] extend = "../pyproject.toml"` rather than a comment asking people not to add a `[tool.ruff]` block, since extending means a setting added here overrides one key instead of the whole shared config. The modules inside `_checks` and `_fixes` keep their public-looking names: the runner derives check and fix identifiers from them, so `add_agents_md` is the name of the fix on the command line rather than an importable API. Also drops two comments explaining what a check used to do when it was a standalone script, which is the same leftover-from-one-file class that was picked up on canonical#1. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XdsdR8QdcZY6GUJgd1Wt7c

james-garner-canonical

I'm keen to try this approach in our CI.

I haven't thoroughly reviewed the code, and there are parts where I worry that we're taking on additional complexity needlessly, in particular including layered fallback behaviours for different missing inputs.

The resolve_origin that your comment highlights as a risk is exactly in this category I think -- options would be: hard error in cases like this (missing input), caller can create a failure step to manually create an issue that enrich failed; or this tool has a single dumb "create a new issue about our own failure" pathway instead.

I like the repo shape, consumption model, and shared ruff config.

james-garner-canonical · 2026-09-08T08:12:50Z

+# How many recently-updated issues to scan for the notifier's marker. The
+# artefact we are looking for was touched minutes ago, so this only has to
+# cover issue churn in that window; 50 is far more than `operator` sees.
+RECENT_ISSUE_SCAN = 50


Env var is fine, though an explicit, required CLI argument could make things clearer. Either way, I'd still like to slim this down and have this script implement a single easy to follow control flow rather than having fallbacks like this.

james-garner-canonical · 2026-09-08T08:15:33Z

+
+"""Validating the model's response envelope.
+
+Hand-rolled deliberately, not the `jsonschema` package, so the script keeps


Now that this is a package which we execute with uvx, it would be easy to use jsonschema. WDYT about using jsonschema now for ease of review? It would be easy to drop jsonschema in favour of a hand-rolled implementation later if we wanted.

tonyandrewmeyer added 10 commits August 26, 2026 11:28

docs: soft wrap the README

8d719f9

One line per paragraph rather than hard breaks at 76 columns, so diffs on a reworded sentence stay one line instead of reflowing the paragraph around it.

tonyandrewmeyer changed the title ~~feat: move the ai-failure-notifier here from operator~~ feat: add an ai-failure-notifier workflow and script Aug 26, 2026