feat: add an ai-failure-notifier workflow and script - #1
Draft
tonyandrewmeyer wants to merge 17 commits into
Draft
feat: add an ai-failure-notifier workflow and script#1tonyandrewmeyer wants to merge 17 commits into
tonyandrewmeyer wants to merge 17 commits into
Conversation
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.
tonyandrewmeyer
changed the title
tonyandrewmeyer
commented
Aug 26, 2026
tonyandrewmeyer
commented
Aug 26, 2026
tonyandrewmeyer
commented
Aug 26, 2026
tonyandrewmeyer
commented
Aug 26, 2026
|
|
||
| """Triage and enrich the issue opened when a scheduled workflow fails.""" | ||
|
|
||
| from charm_tech_code.ai_failure_notifier.apply import ( |
Collaborator
Author
There was a problem hiding this comment.
I see no reason to re-export all these names.
tonyandrewmeyer
commented
Aug 26, 2026
| ) | ||
| from charm_tech_code.ai_failure_notifier.models import CandidateIssue | ||
|
|
||
| # --- Candidate pool --- |
Collaborator
Author
There was a problem hiding this comment.
All the comments like this across the PR are leftovers from this being one file and should be removed.
tonyandrewmeyer
commented
Aug 26, 2026
|
|
||
| from typing import Any | ||
|
|
||
| # --- Envelope schema validation (hand-rolled -- deliberately not the |
Collaborator
Author
There was a problem hiding this comment.
This comment belongs in the docstring.
tonyandrewmeyer
commented
Aug 26, 2026
| from charm_tech_code.ai_failure_notifier.constants import DEFAULT_MODEL, MARKER_PREFIX | ||
|
|
||
|
|
||
| def main() -> int: |
Collaborator
Author
There was a problem hiding this comment.
I don't think we want a 250 line main function. Let's break this up into some private helpers.
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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.pyproject.toml,src/,tests/and lockfile, matchingcanonical/charmlibsrather than inventing a second convention for the team to remember. The real win is dependency isolation: this tool has no runtime dependencies and runs after a scheduled workflow has already failed, so a future tool's dependencies should not be installed on every such run.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.