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-01T01:37:02Z

+
+from charm_tech_code.ai_failure_notifier.cli import main
+
+__all__ = ['main']


This tool probably doesn't need to usable from import?

Suggested change

from charm_tech_code.ai_failure_notifier.cli import main

__all__ = ['main']

james-garner-canonical · 2026-09-01T01:38:32Z

@@ -0,0 +1,79 @@
+# Copyright 2026 Canonical Ltd.


Might as well make all the modules private?

I guess it doesn't really matter either way, but why not make it harder for us to depend on tool internals in weird ways in future CI hacks.

james-garner-canonical · 2026-09-01T01:39:54Z

+# Ruff configuration is at the root of the monorepo, deliberately not repeated
+# here: ruff uses the closest config it finds rather than merging, so a
+# [tool.ruff] block in this file would silently override the shared one.


Makes sense, but alternatively:

Suggested change

# Ruff configuration is at the root of the monorepo, deliberately not repeated

# here: ruff uses the closest config it finds rather than merging, so a

# [tool.ruff] block in this file would silently override the shared one.

[tool.ruff]

extend = "../pyproject.toml"

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.

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