From 41325bbe5e572837e8859c1fe9cf94dd7a8db392 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Sat, 25 Jul 2026 12:44:42 +1200 Subject: [PATCH 01/37] ci: enrich scheduled-failure issues with an LLM triage pass 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 #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 (#2658). The write calls are covered only by mocks, since exercising them means posting here; they want a workflow_dispatch run after this merges. --- .github/ai_failure_notifier.py | 1283 +++++++++++++++++ .github/workflows/ai-failure-enrich.yaml | 119 ++ .../workflows/notify-scheduled-failure.yaml | 45 +- pyproject.toml | 2 +- test/test_ai_failure_notifier.py | 772 ++++++++++ 5 files changed, 2212 insertions(+), 9 deletions(-) create mode 100644 .github/ai_failure_notifier.py create mode 100644 .github/workflows/ai-failure-enrich.yaml create mode 100644 test/test_ai_failure_notifier.py diff --git a/.github/ai_failure_notifier.py b/.github/ai_failure_notifier.py new file mode 100644 index 000000000..f03bdda09 --- /dev/null +++ b/.github/ai_failure_notifier.py @@ -0,0 +1,1283 @@ +#!/usr/bin/env python3 +# /// script +# requires-python = ">=3.11" +# /// +# +# Copyright 2026 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""ai-failure-notifications enrichment step. + +Invoked by `.github/workflows/ai-failure-enrich.yaml` after +`.github/workflows/notify-scheduled-failure.yaml` (the notifier) has already +created or commented on a placeholder issue for a failed scheduled workflow +run. This script: + +1. Finds the placeholder the notifier just touched (or, on a same-run + re-fire, the issue an earlier run of this script already enriched). +2. Fetches and parses the failing job logs into a deterministic failure + signature. +3. Builds a small candidate-issue pool (coarse title/body search). +4. Asks an LLM (via OpenRouter) to decide comment-vs-new and draft the text, + validates the response against the envelope schema, and applies it via + `gh`. +5. Falls back to a plain, generic issue/comment (still marker-stamped) if + OpenRouter is unreachable, misconfigured, or returns invalid JSON. + +The functions above the `--- I/O ---` marker are pure and unit-tested in +`test_ai_failure_notifier.py`. Everything below it talks to `gh` or +OpenRouter and is exercised only by mocking in tests. +""" + +from __future__ import annotations + +import dataclasses +import datetime +import hashlib +import json +import os +import re +import subprocess +import sys +import urllib.request +from typing import Any, Literal + +MARKER_PREFIX = 'ai-failure-notifications' +DEFAULT_MODEL = 'deepseek/deepseek-chat' # DeepSeek V3 on OpenRouter. +CLOSED_CANDIDATE_WINDOW_DAYS = 14 +MAX_CANDIDATES = 3 + +# Colour escapes, which Actions logs are full of. Two alternatives, because +# the logs contain both the real thing and a mangled form where the ESC byte +# has already been stripped, leaving a bare "[32m". +ANSI = re.compile( + r""" + \x1b\[ [0-9;]* [A-Za-z] # a full escape: ESC [ params letter + | + \[ \d+ (?:;\d+)* m # ESC already stripped: [32m, [1;33m + """, + re.VERBOSE, +) + +# The timestamp Actions prefixes to every log line, for example +# "2026-07-21T16:17:04.8204062Z ". Stripped before anything else is matched. +TS = re.compile( + r""" + ^\d{4}-\d{2}-\d{2} # date: 2026-07-21 + T\d{2}:\d{2}:\d{2} # time: T16:17:04 + \.\d+Z[ ] # fractional seconds, zone, one trailing space + """, + re.VERBOSE, +) + +# Actions' own annotation for a failing step. +ERROR_MARKER = re.compile(r'##\[error\]') + +# A line from pytest's short summary, for example +# "FAILED tests/integration/test_charm.py::test_deploy - TimeoutError: ...". +PYTEST_SUMMARY = re.compile( + r""" + ^(FAILED|ERROR)[ ] # which of the two pytest reports + (\S+?)[ ]-[ ] # the test id, up to the " - " separator + (.+)$ # the error message, to end of line + """, + re.VERBOSE, +) + +# The end of pytest's summary section, for example +# "======== 3 failed, 41 passed, 2 warnings in 512.44s ========". +PYTEST_SUMMARY_END = re.compile( + r""" + ={3,} # the run of = that brackets the line + .*(failed|passed|error) # and one of pytest's outcome words + """, + re.VERBOSE, +) + +# A failing Go test, for example "--- FAIL: TestFoo (0.01s)". +GO_FAIL = re.compile(r'^--- FAIL: (\S+)') + +# The last line of a Python traceback: the exception type and its message, +# for example "KeyError: 'loki/0'". Deliberately narrow -- it must look like an +# exception class name -- so that arbitrary "word: text" log lines don't match. +TRACEBACK_END = re.compile( + r""" + ^([A-Z] # exception types start with a capital + [A-Za-z_.]* # dotted path allowed: ops.pebble.APIError + (?:Error|Exception|Warning)) # and conventionally end one of three ways + :[ ](.*)$ # then ": " and the message + """, + re.VERBOSE, +) + +# Matches markers stamped by either workflow: +# notifier: +# +# enricher: +MARKER_RE = re.compile( + r' + """, + re.VERBOSE, +) + + +# --- Structured shapes --- +# +# The signature is built here, serialised into the prompt, and hashed for the +# marker, so its shape is worth pinning down rather than passing dicts around. +# `dataclasses.asdict` preserves field declaration order, which is what the +# prompt's JSON ends up in. + + +@dataclasses.dataclass(frozen=True) +class PytestFailure: + """One line of pytest's short summary.""" + + kind: str # "FAILED" or "ERROR" -- pytest reports both here. + test: str + error: str + + +@dataclasses.dataclass(frozen=True) +class JobSignature: + """What the deterministic parser could extract from one failed job's log.""" + + job_id: int + job_name: str + failed_step: str | None + pytest_failures: list[PytestFailure] + go_failures: list[str] + traceback_top_error: str | None + tail_excerpt: list[str] + + +@dataclasses.dataclass(frozen=True) +class RunSignature: + """Every failed job of one run, plus the run's own identifying fields.""" + + run_id: str + workflow_name: str + html_url: str + created_at: str + jobs: list[JobSignature] + + def as_json(self) -> str: + """Render for the prompt, in field declaration order.""" + return json.dumps(dataclasses.asdict(self), indent=2) + + +@dataclasses.dataclass(frozen=True) +class FailedJob: + """A failed job as listed by `gh run view`, before its log is fetched.""" + + id: int + name: str + failed_step: str | None + + +@dataclasses.dataclass(frozen=True) +class CandidateIssue: + """An existing issue that might already track this failure.""" + + number: int + title: str + body: str | None + closed_at: str | None + + @classmethod + def from_gh(cls, data: dict[str, Any]) -> CandidateIssue: + """Build from one element of `gh issue list --json ...` output.""" + return cls( + number=data['number'], + title=data['title'], + body=data.get('body'), + closed_at=data.get('closedAt'), + ) + + def excerpt(self) -> str: + """The first line of the body, bounded, for the candidate block.""" + lines = (self.body or '').strip().splitlines() + return lines[0][:300] if lines else '(no body)' + + +# --- Signature extraction --- + + +def strip_line(line: str) -> str: + """Remove GHA timestamp and ANSI colours.""" + line = TS.sub('', line, count=1) + line = ANSI.sub('', line) + return line.rstrip('\r\n') + + +def parse_job_log( + text: str, +) -> tuple[list[PytestFailure], list[str], str | None, list[str]]: + """Parse one job's raw log text. + + Returns (pytest_failures, go_failures, traceback_top_error, tail_excerpt). + """ + lines = [strip_line(line) for line in text.splitlines()] + + pytest_failures: list[PytestFailure] = [] + go_failures: list[str] = [] + in_summary = False + + for line in lines: + if 'short test summary info' in line: + in_summary = True + continue + if in_summary: + m = PYTEST_SUMMARY.match(line) + if m: + kind, test, err = m.groups() + pytest_failures.append(PytestFailure(kind, test, err.strip())) + continue + if PYTEST_SUMMARY_END.match(line): + in_summary = False + m = GO_FAIL.match(line) + if m: + go_failures.append(m.group(1)) + + traceback_top_error: str | None = None + for line in reversed(lines): + m = TRACEBACK_END.match(line) + if m: + traceback_top_error = f'{m.group(1)}: {m.group(2).strip()}' + break + + error_idx = None + for i, line in enumerate(lines): + if ERROR_MARKER.search(line): + error_idx = i + break + tail: list[str] = [] + if error_idx is not None: + for line in reversed(lines[:error_idx]): + if not line.strip(): + continue + if line.startswith('##[group]') or line.startswith('##[endgroup]'): + continue + tail.append(line) + if len(tail) >= 40: + break + tail.reverse() + + return pytest_failures, go_failures, traceback_top_error, tail + + +def build_job_signature( + job_id: int, job_name: str, failed_step: str | None, log_text: str +) -> JobSignature: + """Parse one job's log into its signature.""" + pytest_failures, go_failures, traceback_top_error, tail = parse_job_log(log_text) + return JobSignature( + job_id=job_id, + job_name=job_name, + failed_step=failed_step, + pytest_failures=pytest_failures, + go_failures=go_failures, + traceback_top_error=traceback_top_error, + tail_excerpt=tail, + ) + + +def build_run_signature( + run_id: str, workflow_name: str, html_url: str, created_at: str, jobs: list[JobSignature] +) -> RunSignature: + """Combine per-job signatures into the full run signature.""" + return RunSignature( + run_id=str(run_id), + workflow_name=workflow_name, + html_url=html_url, + created_at=created_at, + jobs=jobs, + ) + + +# --- Marker + signature hashing --- + + +def signature_hash(signature: RunSignature) -> str: + """Deterministic short fingerprint of a run signature. + + Used only for the marker's :sig= suffix (not for dedup decisions -- + that's the LLM's job, guided by the candidate pool). + """ + parts: list[str] = [] + for job in signature.jobs: + parts.extend(failure.test for failure in job.pytest_failures) + parts.extend(job.go_failures) + if job.traceback_top_error: + parts.append(job.traceback_top_error) + if job.failed_step: + parts.append(job.failed_step) + canonical = '\n'.join(sorted(parts)) or signature.workflow_name + return hashlib.sha1(canonical.encode('utf-8'), usedforsecurity=False).hexdigest()[:16] + + +# Which artefact the notifier touched: a fresh placeholder issue, or a comment +# on an issue that already existed. A Literal so a type checker rejects a bad +# value before it reaches a marker. +Origin = Literal['new', 'comment'] + + +def render_notifier_marker(run_id: str, origin: Origin) -> str: + """Render the marker the notifier stamps, telling the enricher what it touched.""" + return f'' + + +def render_enriched_marker(run_id: str, signature: RunSignature) -> str: + """Render the marker this script stamps once it has fully processed a run. + + Presence of :sig= is what makes rung zero (find_run_markers) treat a later + same-run-id trigger as "already enriched, just note the re-run". + """ + return f'' + + +def find_run_markers( + texts: list[tuple[int, str]], run_id: str +) -> tuple[int | None, str | None, int | None]: + """Scan (issue_number, text) pairs for markers belonging to `run_id`. + + Returns (enriched_issue, origin_kind, origin_issue): + - enriched_issue: an issue number carrying a :sig= marker for this run + (rung zero -- this run was already fully enriched once), else None. + - origin_kind / origin_issue: the "new"/"comment" marker the notifier + stamped for this run (identifies which artefact to upgrade), else + (None, None). + """ + run_id = str(run_id) + enriched_issue = None + origin_kind = None + origin_issue = None + for number, text in texts: + if not text: + continue + for match in MARKER_RE.finditer(text): + if match['run_id'] != run_id: + continue + if match['sig']: + enriched_issue = number + elif match['origin']: + origin_kind = match['origin'] + origin_issue = number + return enriched_issue, origin_kind, origin_issue + + +# --- Candidate pool --- + + +def within_window(iso_timestamp: str, now: datetime.datetime, days: int) -> bool: + """Return whether `iso_timestamp` falls within `days` of `now`.""" + ts = datetime.datetime.fromisoformat(iso_timestamp.replace('Z', '+00:00')) + return now - ts <= datetime.timedelta(days=days) + + +def build_candidates_block( + open_issues: list[CandidateIssue], + closed_issues: list[CandidateIssue], + now: datetime.datetime, +) -> str: + """Render the {{CANDIDATES_BLOCK}} the prompt expects. + + Up to MAX_CANDIDATES entries: open issues first, then recently-closed + issues (<=14 days) filling any remaining slots, explicitly labelled as + closed so the LLM never auto-treats one as a strong match. Calibration on + past scheduled failures found a closed issue can corroborate a match but + should never be enough to dedupe against on its own. + """ + entries: list[str] = [] + for issue in open_issues: + if len(entries) >= MAX_CANDIDATES: + break + entries.append(f'- **#{issue.number} — {issue.title}** (open)\n > {issue.excerpt()}') + + recent_closed = [ + i + for i in closed_issues + if i.closed_at and within_window(i.closed_at, now, CLOSED_CANDIDATE_WINDOW_DAYS) + ] + for issue in recent_closed: + if len(entries) >= MAX_CANDIDATES: + break + entries.append( + f'- **#{issue.number} — {issue.title}** (closed {issue.closed_at} -- ' + f'recently closed; treat as at most a medium-confidence match)\n > {issue.excerpt()}' + ) + + if not entries: + return '(no open issues found for this workflow)' + return '\n'.join(entries) + + +# --- Prompt building --- + +SYSTEM_PROMPT = """\ +You are the enrichment step of an internal CI failure-triage bot for the +Canonical Charm Tech team. A scheduled GitHub Actions workflow just failed. +A separate deterministic parser has already extracted a structured failure +signature from the run's logs -- you do not have repository access, log +access, or internet access beyond what is given to you in this message. +Work only from the signature JSON and candidate issues you're given. + +Your job: decide whether this failure is a new occurrence of an +already-tracked problem (comment on the existing issue) or something not +currently tracked (open a new issue), and draft the text for whichever +artefact you choose. Output ONLY the JSON envelope described below -- no +prose before or after it, no markdown code fences around it. + +You are reporting, not fixing. Never propose a fix, a patch, a diff, a +workaround, a configuration change, a retry, a next step, or anything else +that tells a reader what to do about the failure -- not as a section, not +as a sentence, not as an aside, however confident you are and however +obvious it looks. Deciding what to do about a failure is someone else's +job, and a wrong suggestion from you is worse than none, because it +anchors whoever picks the issue up. Describe what failed and, where the +signature genuinely supports it, why. Then stop. + +## Reading the signature + +The signature has one entry per failed job in `jobs[]`. Each entry may +carry, in decreasing order of how much you should trust it: + +1. `pytest_failures[]` -- `{kind, test, error}` triples parsed from + pytest's "short test summary info" block. `test` is a real pytest node + id; `error` is the tail of that summary line and CAN BE TRUNCATED by + pytest itself (it cuts long messages short, for example + "PendingDeprecat..."). If an `error` string ends in `...`, treat it + as unreliable for anything beyond "this test failed" -- do not quote it + as the root cause, do not put the truncated fragment in a title. Look + at `traceback_top_error` and `tail_excerpt` for that job instead; if + they don't resolve it either, describe the failure by test name only + and say the specific assertion text is unavailable. +2. `traceback_top_error` -- the last `: ` line found + anywhere in the job's log. Usually the real exception, but it is a + last-line heuristic: on jobs where cleanup code raises its own warning + after the real failure (a `ResourceWarning` from tempfile cleanup is + the known example), this field can point at the cleanup noise instead + of the actual cause. If `traceback_top_error` names a `Warning` class + while `pytest_failures[]` for the same job names an `Error` class, + trust the `pytest_failures[]` entry for what actually failed and treat + `traceback_top_error` as noise. +3. `tail_excerpt[]` -- the last ~40 non-empty log lines before the job's + first `##[error]` marker. This is what's left when neither of the + above fired. Sometimes it contains an unambiguous plain-text failure + (for example a Go `panic:`, a shell command's final non-zero-exit message, an + infra tool's own `level=ERROR msg="..."` line) -- if so, use it. Other + times it shows the *shape* of a timeout or an in-progress hook without + ever stating what actually broke. Do not guess a specific root cause + from an inconclusive `tail_excerpt`. It is fine, and preferred, to say + plainly that the cause isn't visible in the available log excerpt. + +A job with no `pytest_failures`, no `traceback_top_error`, and a +`tail_excerpt` that never names an exception, an error code, or an +explicit failure message (only status-transition noise) is very likely an +**infrastructural** failure (bootstrap, provisioning, network) rather than +a test regression, PROVIDED the excerpt at least shows a concrete +infra-level error. Say so explicitly -- title and body should make clear +this is "infrastructure failing before tests could run" language, not +"test X failed" language, and do not name a specific test as the culprit. + +If even that infra-level signal is missing or the excerpt is genuinely +inconclusive, do not invent a specific-sounding title. Use a plain, honest +title that names the workflow and says the cause is unclear from the log +excerpt, set "confidence": "low", and say in `dedup_reason` what +information would be needed to do better. Never fabricate a +plausible-sounding cause to fill the gap. + +A run can have multiple failed jobs with different signatures. Handle this +as follows: + +- If all failed jobs share essentially the same signature, treat it as one + failure and write one title/body for it, noting how many jobs it hit. +- If failed jobs split into distinct signatures, decide whether one is + clearly the dominant, actionable story, with others being a smaller + number of already-familiar, separately-tracked issues riding along. If + so, make the dominant one the subject of `title`/`action`, and mention + the others in `body` as a secondary note plus in `dedup_reason`. +- If failed jobs are multiple genuinely distinct, comparably-important + problems with no dominant one, use a title naming the workflow and the + count/spread of distinct causes and list each in `body` as its own + bullet. Don't pick one arbitrarily and bury the rest. + +## Handling multiple independent failures (`also`) + +When a run has a dominant story plus one or two secondary failures that +have distinct signatures from the dominant one AND would either match a +different existing tracked issue or themselves be dominant enough to +warrant their own artefact if seen alone, emit an `also` array on the +envelope with one entry per secondary. Each `also[i]` is a self-contained +decision (its own `action`, its own `target_issue`/`title`, its own +`confidence`, its own `dedup_reason`). + +Do not use `also` to split a single failure across two entries; do not +nest `also` inside an `also` entry; cap: 2 `also` entries per envelope. + +## Body structure (for `action: "new"`) + +Use this shape, adapting to how many distinct failures you're describing: + +``` +## Summary + + +## Failures +- ****: ", + or "cause unclear from the available log excerpt"> + (omit this section entirely if there's exactly one failing job) + +## Likely root cause + +``` + +Do not add sections beyond these. In particular there is no "suggested +fix", "next steps", "workaround" or "recommendation" section, and none may +be added. + +For `action: "comment"`, keep the comment short: what matches the existing +issue (or what's new/different), and nothing else. + +## Deciding comment vs new + +You are given up to three candidate existing issues (title + excerpt), already +pre-filtered to the same workflow by a coarser deterministic search. Some +candidates may be marked "(closed ...)" -- these are recently-closed +issues included for context only; never target a closed issue with +`action: "comment"`, and never let a closed candidate alone justify +`confidence: "high"`. + +- **Strong** -- at least one `pytest_failures[].test` (or, for + infra/tail-only failures, the same `failed_step` plus the same concrete + error text) matches an OPEN candidate, AND the top error class matches + too -> `action: "comment"`, `confidence: "high"`. +- **Medium** -- same workflow and same `failed_step`, or same top error + class, but the specific test/error text has drifted, OR the only match + is a recently-closed candidate -> `action: "comment"` (target the open + issue only; if the only match is closed, use `action: "new"` instead and + mention the closed issue in `dedup_reason`), `confidence: "medium"`, and + say the drift explicitly in `body` and `dedup_reason`. +- **Weak** -- only the workflow name matches, or only a vague thematic + overlap -- this is not a dedup match. `action: "new"`. + +Do not comment on a candidate just because one exists for the same +workflow -- check whether the *signature* actually matches. + +## Labels and issue type + +The repository has a fixed, centrally-managed label set. You may only use +labels from this list, and a label that does not exist in the repository is +dropped before the issue is created, so inventing one achieves nothing: + +- `tests` -- the failure is in the tests or the test harness. This is the + common case for a failing integration or unit workflow. +- `docs` -- the failure is in documentation building, linting or link + checking, or in a workflow that maintains a doc. +- `performance` -- only where the failure *is* a performance result, such + as a benchmark regression or a timing threshold. Not for an ordinary + timeout, which is usually a hang or an infrastructure problem. +- `small item` -- only where the signature makes it obvious the work is + trivial. Prefer omitting it: you cannot see the code, so you are usually + not in a position to judge size. + +An empty `labels` list is a perfectly good answer. Use it whenever none of +the above clearly applies, rather than reaching for the nearest one. + +`issue_type` is "bug" when you're reasonably sure this is a defect. Use +`null` when genuinely unsure -- this is normal and expected for +low-confidence signatures, not an edge case. + +## Never + +- Never suggest a fix or a remedy of any kind. See "You are reporting, not + fixing" above; this is the constraint most easily broken by accident, + usually as a closing sentence offering a next step. +- Never invent a root cause, a PR number, or a file/line that isn't + directly supported by the signature JSON you were given. +- Never use a label that isn't in the list above. +- Never output anything except the single JSON envelope object. +""" + +USER_PROMPT_TEMPLATE = """\ +Workflow: {workflow_name} +Run: {run_url} + +## Extracted failure signature (deterministic parser output, JSON) + +{signature_json} + +## Candidate existing open issues (same workflow, pre-filtered by title; +## may include recently-closed issues explicitly marked as such; may be +## empty) + +{candidates_block} + +Produce the JSON envelope now. +""" + + +def build_prompt( + workflow_name: str, run_url: str, signature: RunSignature, candidates_block: str +) -> tuple[str, str]: + """Render the (system, user) prompt pair for the OpenRouter call.""" + user = USER_PROMPT_TEMPLATE.format( + workflow_name=workflow_name, + run_url=run_url, + signature_json=signature.as_json(), + candidates_block=candidates_block, + ) + return SYSTEM_PROMPT, user + + +# --- Envelope schema validation (hand-rolled -- deliberately not the +# `jsonschema` package, so the script keeps needing nothing outside the stdlib. +# Mirrors ENVELOPE_JSON_SCHEMA below, which is what OpenRouter is asked to +# conform to; this re-checks it on the applier side.) --- + +_ENTRY_COMMON_REQUIRED = ('action', 'body', 'dedup_reason', 'confidence') +_ENTRY_KNOWN_KEYS = { + 'action', + 'body', + 'dedup_reason', + 'confidence', + 'title', + 'labels', + 'issue_type', + 'target_issue', +} + + +def validate_entry(entry: Any, *, path: str) -> list[str]: + """Validate one envelope entry (top-level or an `also[i]`) against the schema.""" + errors: list[str] = [] + if not isinstance(entry, dict): + return [f'{path}: expected an object, got {type(entry).__name__}'] + + for field in _ENTRY_COMMON_REQUIRED: + if field not in entry: + errors.append(f"{path}: missing required field '{field}'") + + unknown = set(entry) - _ENTRY_KNOWN_KEYS + if unknown: + errors.append(f'{path}: unknown field(s) {sorted(unknown)}') + + action = entry.get('action') + if action not in ('comment', 'new'): + errors.append(f"{path}.action: must be 'comment' or 'new', got {action!r}") + return errors # can't check action-conditional fields without a valid action + + if not isinstance(entry.get('body'), str) or not entry.get('body'): + errors.append(f'{path}.body: must be a non-empty string') + if not isinstance(entry.get('dedup_reason'), str) or not entry.get('dedup_reason'): + errors.append(f'{path}.dedup_reason: must be a non-empty string') + if entry.get('confidence') not in ('high', 'medium', 'low'): + errors.append( + f'{path}.confidence: must be one of high/medium/low, got {entry.get("confidence")!r}' + ) + + if action == 'new': + for field in ('title', 'labels', 'issue_type'): + if field not in entry: + errors.append(f"{path}: action='new' requires '{field}'") + if 'target_issue' in entry: + errors.append(f"{path}: action='new' must not include 'target_issue'") + labels = entry.get('labels') + if labels is not None and ( + not isinstance(labels, list) or not all(isinstance(x, str) for x in labels) + ): + errors.append(f'{path}.labels: must be an array of strings') + if 'issue_type' in entry and not ( + entry['issue_type'] is None or isinstance(entry['issue_type'], str) + ): + errors.append(f'{path}.issue_type: must be a string or null') + else: # comment + if 'target_issue' not in entry: + errors.append(f"{path}: action='comment' requires 'target_issue'") + elif not isinstance(entry['target_issue'], int) or entry['target_issue'] < 1: + errors.append(f'{path}.target_issue: must be a positive integer') + for field in ('title', 'labels', 'issue_type'): + if field in entry: + errors.append(f"{path}: action='comment' must not include '{field}'") + + return errors + + +def validate_envelope(envelope: Any) -> list[str]: + """Validate a top-level envelope (may carry `also`). + + Returns a list of human-readable errors; empty list means valid. + """ + if not isinstance(envelope, dict): + return ['envelope: expected a JSON object'] + + errors = validate_entry(envelope, path='envelope') + + also = envelope.get('also') + if also is not None: + if not isinstance(also, list) or len(also) > 2: + errors.append('envelope.also: must be an array of at most two entries') + else: + for i, entry in enumerate(also): + if isinstance(entry, dict) and 'also' in entry: + errors.append(f"envelope.also[{i}]: nested 'also' is not allowed") + errors.extend(validate_entry(entry, path=f'envelope.also[{i}]')) + + unknown_top = set(envelope) - _ENTRY_KNOWN_KEYS - {'also'} + if unknown_top: + errors.append(f'envelope: unknown field(s) {sorted(unknown_top)}') + + return errors + + +ENVELOPE_JSON_SCHEMA = { + '$schema': 'https://json-schema.org/draft/2020-12/schema', + 'title': 'ai-failure-notifications envelope', + 'type': 'object', + 'required': ['action', 'body', 'dedup_reason', 'confidence'], + 'properties': { + 'action': {'enum': ['comment', 'new']}, + 'body': {'type': 'string', 'minLength': 1}, + 'dedup_reason': {'type': 'string', 'minLength': 1}, + 'confidence': {'enum': ['high', 'medium', 'low']}, + 'title': {'type': 'string', 'minLength': 1}, + 'labels': {'type': 'array', 'items': {'type': 'string'}}, + 'issue_type': {'type': ['string', 'null']}, + 'target_issue': {'type': 'integer', 'minimum': 1}, + 'also': {'type': 'array', 'maxItems': 2, 'items': {'$ref': '#/$defs/envelopeEntry'}}, + }, + 'additionalProperties': False, + 'allOf': [{'$ref': '#/$defs/actionConditionals'}], + '$defs': { + 'actionConditionals': { + 'allOf': [ + { + 'if': {'properties': {'action': {'const': 'new'}}}, + 'then': { + 'required': ['title', 'labels', 'issue_type'], + 'not': {'required': ['target_issue']}, + 'properties': {'labels': {'type': 'array', 'items': {'type': 'string'}}}, + }, + }, + { + 'if': {'properties': {'action': {'const': 'comment'}}}, + 'then': { + 'required': ['target_issue'], + 'not': { + 'anyOf': [ + {'required': ['title']}, + {'required': ['labels']}, + {'required': ['issue_type']}, + ] + }, + }, + }, + ] + }, + 'envelopeEntry': { + 'type': 'object', + 'required': ['action', 'body', 'dedup_reason', 'confidence'], + 'properties': { + 'action': {'enum': ['comment', 'new']}, + 'body': {'type': 'string', 'minLength': 1}, + 'dedup_reason': {'type': 'string', 'minLength': 1}, + 'confidence': {'enum': ['high', 'medium', 'low']}, + 'title': {'type': 'string', 'minLength': 1}, + 'labels': {'type': 'array', 'items': {'type': 'string'}}, + 'issue_type': {'type': ['string', 'null']}, + 'target_issue': {'type': 'integer', 'minimum': 1}, + }, + 'additionalProperties': False, + 'allOf': [{'$ref': '#/$defs/actionConditionals'}], + }, + }, +} + + +# --- I/O --- + + +def gh(*args: str, check: bool = True) -> subprocess.CompletedProcess: + """Run a `gh` subcommand, returning the completed process.""" + # S607: `gh` is deliberately called by name, resolved from the runner's PATH. + return subprocess.run(['gh', *args], text=True, capture_output=True, check=check) # noqa: S607 + + +def gh_json(*args: str) -> Any: + """Run a `gh ... --json ...` subcommand and parse its stdout as JSON.""" + result = gh(*args) + return json.loads(result.stdout) if result.stdout.strip() else None + + +def fetch_failed_jobs(repo: str, run_id: str) -> list[FailedJob]: + """List the failed jobs of a run, each with its id, name, and failed step.""" + data = gh_json('run', 'view', str(run_id), '--repo', repo, '--json', 'jobs') or {} + failed: list[FailedJob] = [] + for job in data.get('jobs', []): + if job.get('conclusion') != 'failure': + continue + failed_step = None + for step in job.get('steps') or []: + if step.get('conclusion') == 'failure': + failed_step = step.get('name') + break + failed.append(FailedJob(id=job['databaseId'], name=job['name'], failed_step=failed_step)) + return failed + + +def fetch_job_log(repo: str, run_id: str, job_id: int) -> str: + """Fetch one job's full log text. + + Uses the REST logs endpoint rather than `gh run view --log`: the latter + exits 0 with empty stdout on some `gh` builds (reproduced on 2.45.0), which + silently degrades the extracted signature to nothing. An empty log here is + reported rather than swallowed. + """ + result = gh('api', f'repos/{repo}/actions/jobs/{job_id}/logs', check=False) + if not result.stdout.strip(): + write_step_summary( + f'Warning: no log text for job {job_id} of run {run_id} ' + f'(gh exit {result.returncode}); signature will be based on the job name alone.' + ) + return result.stdout + + +def fetch_run_meta(repo: str, run_id: str) -> dict[str, str]: + """Fetch a run's display metadata (title, workflow name, url, createdAt).""" + return ( + gh_json( + 'run', + 'view', + str(run_id), + '--repo', + repo, + '--json', + 'displayTitle,workflowName,url,createdAt', + ) + or {} + ) + + +def search_issue_numbers(repo: str, query_text: str) -> list[int]: + """Search issues (any state) in `repo` for `query_text`, return issue numbers.""" + # The repo must be passed as `--repo`, not folded into the positional query. + # `gh search issues` quotes each positional argument as a single search + # keyword, so `repo:owner/name "text"` becomes the literal keyword + # `repo:"owner/name \"text\""` and GitHub rejects it as an invalid query. + data = ( + gh_json( + 'search', 'issues', '--repo', repo, '--limit', '10', '--json', 'number', query_text + ) + or [] + ) + return [item['number'] for item in data] + + +def fetch_issue_texts(repo: str, number: int) -> list[str]: + """Fetch an issue's body plus all comment bodies, for marker scanning.""" + data = gh_json('issue', 'view', str(number), '--repo', repo, '--json', 'body,comments') or {} + texts = [data.get('body') or ''] + for c in data.get('comments') or []: + texts.append(c.get('body') or '') + return texts + + +def locate_run_markers(repo: str, run_id: str) -> tuple[int | None, str | None, int | None]: + """Search `repo` for markers belonging to `run_id` and classify them.""" + hits = search_issue_numbers(repo, f'{MARKER_PREFIX}:run={run_id}') + texts: list[tuple[int, str]] = [] + for number in hits: + for text in fetch_issue_texts(repo, number): + texts.append((number, text)) + return find_run_markers(texts, run_id) + + +def search_candidates( + repo: str, workflow_name: str +) -> tuple[list[CandidateIssue], list[CandidateIssue]]: + """Coarse candidate search: open and closed issues matching the workflow name.""" + fields = 'number,title,body,createdAt,closedAt' + open_issues = ( + gh_json( + 'issue', + 'list', + '--repo', + repo, + '--state', + 'open', + '--search', + f'"{workflow_name}"', + '--json', + fields, + '--limit', + '20', + ) + or [] + ) + closed_issues = ( + gh_json( + 'issue', + 'list', + '--repo', + repo, + '--state', + 'closed', + '--search', + f'"{workflow_name}"', + '--json', + fields, + '--limit', + '20', + ) + or [] + ) + return ( + [CandidateIssue.from_gh(i) for i in open_issues], + [CandidateIssue.from_gh(i) for i in closed_issues], + ) + + +def existing_labels(repo: str) -> set[str]: + """Return the set of label names that already exist in `repo`.""" + data = gh_json('label', 'list', '--repo', repo, '--json', 'name', '--limit', '100') or [] + return {item['name'] for item in data} + + +def filter_labels(labels: list[str], available: set[str]) -> list[str]: + """Drop labels that don't already exist in the repo (never auto-create).""" + return [label for label in labels if label in available] + + +def call_openrouter( + system_prompt: str, user_prompt: str, model: str, api_key: str +) -> dict[str, Any]: + """POST the prompt to OpenRouter with the envelope schema, return the parsed JSON. + + Uses urllib rather than requests so the script has no third-party + dependencies at all. urlopen raises HTTPError (a subclass of OSError) on a + non-2xx response, which main() treats the same as any other OpenRouter + failure: fall back to the plain body. + """ + payload = { + 'model': model, + 'messages': [ + {'role': 'system', 'content': system_prompt}, + {'role': 'user', 'content': user_prompt}, + ], + 'response_format': { + 'type': 'json_schema', + 'json_schema': { + 'name': 'ai_failure_notification', + 'strict': True, + 'schema': ENVELOPE_JSON_SCHEMA, + }, + }, + } + request = urllib.request.Request( + 'https://openrouter.ai/api/v1/chat/completions', + data=json.dumps(payload).encode(), + headers={'Authorization': f'Bearer {api_key}', 'Content-Type': 'application/json'}, + method='POST', + ) + # S310: the URL is a literal https endpoint, not caller-controlled. + with urllib.request.urlopen(request, timeout=60) as response: # noqa: S310 + body = json.loads(response.read().decode()) + content = body['choices'][0]['message']['content'] + return json.loads(content) + + +def write_step_summary(message: str) -> None: + """Append a line to the job's step summary (or stderr, outside Actions).""" + path = os.environ.get('GITHUB_STEP_SUMMARY') + if not path: + print(message, file=sys.stderr) + return + with open(path, 'a', encoding='utf-8') as f: + f.write(message + '\n') + + +def set_output(name: str, value: str) -> None: + """Write a `name=value` line to $GITHUB_OUTPUT, if set.""" + path = os.environ.get('GITHUB_OUTPUT') + if not path: + return + with open(path, 'a', encoding='utf-8') as f: + f.write(f'{name}={value}\n') + + +def plain_fallback_body(workflow_name: str, run_url: str) -> str: + """The plain, generic body text used whenever enrichment is unavailable.""" + return f"Scheduled workflow '{workflow_name}' failed: {run_url}" + + +def apply_entry( + repo: str, entry: dict[str, Any], marker: str, *, default_target: int | None = None +) -> str: + """Create or comment on an issue per one envelope entry, stamping `marker`.""" + body = entry['body'].rstrip() + f'\n\n{marker}' + if entry['action'] == 'new': + # The repo's label set is centrally managed, so anything the model + # asked for that doesn't exist is dropped rather than created. + labels = filter_labels(entry.get('labels') or [], existing_labels(repo)) + dropped = set(entry.get('labels') or []) - set(labels) + if dropped: + write_step_summary( + f'Dropped labels that do not exist in this repo: {", ".join(sorted(dropped))}.' + ) + args = ['issue', 'create', '--repo', repo, '--title', entry['title'], '--body', body] + for label in labels: + args += ['--label', label] + issue_type = entry.get('issue_type') + result = None + if issue_type: + result = gh(*args, '--type', issue_type, check=False) + if result.returncode != 0: + write_step_summary( + f'`gh issue create --type {issue_type}` failed ({result.stderr.strip()}); ' + 'retrying without --type.' + ) + result = None + if result is None: + result = gh(*args) + return result.stdout.strip() + else: + target = entry.get('target_issue', default_target) + gh('issue', 'comment', str(target), '--repo', repo, '--body', body) + return f'commented on #{target}' + + +def main() -> int: + """Entry point: locate the run's marker, enrich or fall back, apply, and exit.""" + repo = os.environ['REPO'] + run_id = str(os.environ['RUN_ID']) + workflow_name = os.environ['WORKFLOW_NAME'] + run_url = os.environ['RUN_URL'] + api_key = os.environ.get('OPENROUTER_API_KEY', '') + model = os.environ.get('OPENROUTER_MODEL') or DEFAULT_MODEL + + try: + enriched_issue, origin_kind, origin_issue = locate_run_markers(repo, run_id) + except Exception as exc: # search API rejection, rate limit, transient 5xx. + # 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. + write_step_summary(f'Marker lookup failed ({exc}); treating this run as un-marked.') + enriched_issue, origin_kind, origin_issue = None, None, None + + if enriched_issue is not None: + # Rung zero: this run id was already fully enriched once -- a re-run of + # the same failing jobs re-triggered us. Comment, don't skip and don't + # redo the full LLM pass. On a corpus of past scheduled failures this + # rung accounted for half the real duplicate pairs, making it the + # highest-value one. + gh( + 'issue', + 'comment', + str(enriched_issue), + '--repo', + repo, + '--body', + f'Re-run attempt still failing: {run_url}\n\n', + ) + write_step_summary( + f'Rung zero: run {run_id} already enriched on #{enriched_issue}; ' + 'commented re-run note.' + ) + set_output('handled', 'true') + return 0 + + if origin_issue is None: + # Shouldn't happen -- the notifier always stamps a marker -- but + # don't lose the notification if it does. + write_step_summary( + 'No notifier marker found for this run id; falling back to a plain issue.' + ) + result = gh( + 'issue', + 'create', + '--repo', + repo, + '--title', + f"Scheduled workflow '{workflow_name}' failed", + '--body', + plain_fallback_body(workflow_name, run_url) + + f'\n\n', + ) + origin_issue = int(result.stdout.strip().rstrip('/').rsplit('/', 1)[-1]) + origin_kind = 'new' + + failed_jobs = fetch_failed_jobs(repo, run_id) + jobs_sig = [ + build_job_signature(job.id, job.name, job.failed_step, fetch_job_log(repo, run_id, job.id)) + for job in failed_jobs + ] + meta = fetch_run_meta(repo, run_id) + signature = build_run_signature( + run_id, workflow_name, run_url, meta.get('createdAt', ''), jobs_sig + ) + enriched_marker = render_enriched_marker(run_id, signature) + + if not api_key: + write_step_summary('No OPENROUTER_API_KEY configured -- using the plain fallback body.') + apply_entry( + repo, + { + 'action': 'comment' if origin_kind == 'comment' else 'new', + 'body': plain_fallback_body(workflow_name, run_url), + 'title': f"Scheduled workflow '{workflow_name}' failed", + 'labels': [], + 'issue_type': None, + } + if origin_kind != 'comment' + else { + 'action': 'comment', + 'body': plain_fallback_body(workflow_name, run_url), + 'target_issue': origin_issue, + }, + enriched_marker, + default_target=origin_issue, + ) + set_output('handled', 'true') + return 0 + + try: + open_candidates, closed_candidates = search_candidates(repo, workflow_name) + except Exception as exc: # as above: degrade to "no candidates", don't crash. + write_step_summary(f'Candidate search failed ({exc}); proceeding with no candidates.') + open_candidates, closed_candidates = [], [] + open_candidates = [c for c in open_candidates if c.number != origin_issue] + candidates_block = build_candidates_block( + open_candidates, closed_candidates, datetime.datetime.now(datetime.timezone.utc) + ) + system_prompt, user_prompt = build_prompt(workflow_name, run_url, signature, candidates_block) + + try: + envelope = call_openrouter(system_prompt, user_prompt, model, api_key) + except Exception as exc: # network error, non-2xx, bad JSON, and so on. + write_step_summary(f'OpenRouter call failed ({exc}); using the plain fallback body.') + apply_entry( + repo, + { + 'action': 'new', + 'body': plain_fallback_body(workflow_name, run_url), + 'title': f"Scheduled workflow '{workflow_name}' failed", + 'labels': [], + 'issue_type': None, + } + if origin_kind != 'comment' + else { + 'action': 'comment', + 'body': plain_fallback_body(workflow_name, run_url), + 'target_issue': origin_issue, + }, + enriched_marker, + default_target=origin_issue, + ) + set_output('handled', 'true') + return 0 + + errors = validate_envelope(envelope) + if errors: + write_step_summary( + 'LLM output failed schema validation:\n' + '\n'.join(f'- {e}' for e in errors) + ) + apply_entry( + repo, + { + 'action': 'new', + 'body': plain_fallback_body(workflow_name, run_url), + 'title': f"Scheduled workflow '{workflow_name}' failed", + 'labels': [], + 'issue_type': None, + } + if origin_kind != 'comment' + else { + 'action': 'comment', + 'body': plain_fallback_body(workflow_name, run_url), + 'target_issue': origin_issue, + }, + enriched_marker, + default_target=origin_issue, + ) + set_output('handled', 'true') + return 0 + + if envelope['action'] == 'new' and origin_kind == 'new': + # Upgrade the placeholder in place rather than creating a duplicate. + available = existing_labels(repo) + labels = filter_labels(envelope.get('labels') or [], available) + edit_args = [ + 'issue', + 'edit', + str(origin_issue), + '--repo', + repo, + '--title', + envelope['title'], + '--body', + envelope['body'].rstrip() + f'\n\n{enriched_marker}', + ] + for label in labels: + edit_args += ['--add-label', label] + gh(*edit_args) + elif envelope['action'] == 'comment' and envelope.get('target_issue') == origin_issue: + apply_entry(repo, envelope, enriched_marker, default_target=origin_issue) + elif envelope['action'] == 'comment': + # LLM picked a different candidate than the notifier's coarse match. + apply_entry(repo, envelope, enriched_marker) + if origin_kind == 'comment': + gh( + 'issue', + 'comment', + str(origin_issue), + '--repo', + repo, + '--body', + f'This looks like a distinct issue -- see #{envelope["target_issue"]}.\n\n' + f'{enriched_marker}', + ) + else: + # action == "new" but origin_kind == "comment": the coarse title + # match landed on an unrelated older issue; this is genuinely new. + apply_entry(repo, envelope, enriched_marker) + gh( + 'issue', + 'comment', + str(origin_issue), + '--repo', + repo, + '--body', + f'This looks like a distinct issue from this one -- opened separately.\n\n' + f'{enriched_marker}', + ) + + for also_entry in envelope.get('also') or []: + apply_entry(repo, also_entry, enriched_marker) + + set_output('handled', 'true') + return 0 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/.github/workflows/ai-failure-enrich.yaml b/.github/workflows/ai-failure-enrich.yaml new file mode 100644 index 000000000..5cb74a6c4 --- /dev/null +++ b/.github/workflows/ai-failure-enrich.yaml @@ -0,0 +1,119 @@ +name: AI-enrich scheduled-failure notification + +# The second of two stages. The first, +# .github/workflows/notify-scheduled-failure.yaml, always creates or comments +# on a placeholder issue with no LLM involved. This workflow fires after a +# scheduled-workflow caller of that notifier completes, and tries to rewrite +# whatever it produced into a triaged, deduplicated artefact. +# +# `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" + types: [completed] + +permissions: {} + +jobs: + enrich: + if: ${{ github.event.workflow_run.conclusion == 'failure' && github.event.workflow_run.event == 'schedule' }} + runs-on: ubuntu-latest + environment: ai-failure-triage + permissions: + issues: write + outputs: + handled: ${{ steps.enrich.outputs.handled }} + steps: + - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 + with: + persist-credentials: false + - uses: astral-sh/setup-uv@fac544c07dec837d0ccb6301d7b5580bf5edae39 # v8.2.0 + - name: Extract signature, dedupe, ask the LLM, apply + id: enrich + env: + GH_TOKEN: ${{ github.token }} + REPO: ${{ github.event.workflow_run.repository.full_name }} + RUN_ID: ${{ github.event.workflow_run.id }} + WORKFLOW_NAME: ${{ github.event.workflow_run.name }} + RUN_URL: ${{ github.event.workflow_run.html_url }} + # Secret and variable come from the `ai-failure-triage` + # environment, scoped to this workflow so the OpenRouter key is not + # readable by anything else in the repo. Each repo adopting this + # provisions its own. Absent key -> the script uses the plain + # fallback body without calling OpenRouter (see + # ai_failure_notifier.py main()). + OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} + OPENROUTER_MODEL: ${{ vars.OPENROUTER_MODEL }} + run: uv run .github/ai_failure_notifier.py + + # 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 the simplest thing that + # can work: one `gh issue create`, no script, 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. + plain-fallback: + needs: [enrich] + if: >- + ${{ always() && github.event.workflow_run.conclusion == 'failure' && + github.event.workflow_run.event == 'schedule' && + needs.enrich.outputs.handled != 'true' }} + runs-on: ubuntu-latest + permissions: + issues: write + steps: + - name: Create a plain issue (fallback) + env: + GH_TOKEN: ${{ github.token }} + WORKFLOW_NAME: ${{ github.event.workflow_run.name }} + REPO: ${{ github.event.workflow_run.repository.full_name }} + RUN_URL: ${{ github.event.workflow_run.html_url }} + run: | + gh issue create \ + --repo "$REPO" \ + --title "Scheduled workflow '$WORKFLOW_NAME' failed" \ + --body "Scheduled workflow '$WORKFLOW_NAME' failed: $RUN_URL" diff --git a/.github/workflows/notify-scheduled-failure.yaml b/.github/workflows/notify-scheduled-failure.yaml index 06efc56de..d642bb95e 100644 --- a/.github/workflows/notify-scheduled-failure.yaml +++ b/.github/workflows/notify-scheduled-failure.yaml @@ -1,8 +1,17 @@ name: Notify on scheduled failure -# Reusable workflow: opens an issue when a scheduled workflow fails. -# Callers gate the invocation with `if: failure() && github.event_name == 'schedule'` -# and pass `permissions: issues: write`. +# Reusable workflow: opens (or comments on) an issue when a scheduled +# workflow fails. Callers gate the invocation with +# `if: failure() && github.event_name == 'schedule'` and pass +# `permissions: issues: write`. +# +# This is the first of two stages: a cheap, deterministic, always-works +# dedup pre-check, with no LLM dependency and no secrets. The second, +# .github/workflows/ai-failure-enrich.yaml, is triggered separately by +# `workflow_run` on this workflow's callers, and rewrites whatever this one +# produced into a triaged artefact. Keeping them separate is what lets this +# workflow keep its guarantee: if the enricher is unprovisioned or broken, a +# notification still happens. on: workflow_call: @@ -13,14 +22,34 @@ jobs: permissions: issues: write steps: - - name: Create issue on failure + - name: Create or comment on issue, deduping coarsely by workflow name env: GH_TOKEN: ${{ github.token }} WORKFLOW_NAME: ${{ github.workflow }} # The workflow that called this one. REPO: ${{ github.repository }} + RUN_ID: ${{ github.run_id }} RUN_URL: ${{ github.server_url }}/${{ github.repository }}/actions/runs/${{ github.run_id }} run: | - gh issue create \ - --repo "$REPO" \ - --title "Scheduled workflow '$WORKFLOW_NAME' failed" \ - --body "Scheduled workflow '$WORKFLOW_NAME' failed: $RUN_URL" + set -euo pipefail + marker="" + gh issue comment "$match" --repo "$REPO" --body "$comment_body" + else + issue_body="Scheduled workflow '$WORKFLOW_NAME' failed: $RUN_URL"$'\n\n'"${marker}new -->" + gh issue create --repo "$REPO" \ + --title "Scheduled workflow '$WORKFLOW_NAME' failed" \ + --body "$issue_body" + fi diff --git a/pyproject.toml b/pyproject.toml index 04a54ce8b..a99e2a684 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -288,7 +288,7 @@ include = ["ops/*.py", "ops/_private/*.py", "test/*.py", "test/charms/*/src/*.py exclude = [ "tracing/*", ] -extraPaths = ["testing", "tracing"] +extraPaths = ["testing", "tracing", ".github"] # .github: workflow scripts under test pythonVersion = "3.10" # check no python > 3.10 features are used pythonPlatform = "All" typeCheckingMode = "strict" diff --git a/test/test_ai_failure_notifier.py b/test/test_ai_failure_notifier.py new file mode 100644 index 000000000..28bcf4009 --- /dev/null +++ b/test/test_ai_failure_notifier.py @@ -0,0 +1,772 @@ +#!/usr/bin/env python3 +# /// script +# requires-python = ">=3.11" +# /// +# +# Copyright 2026 Canonical Ltd. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Unit tests for .github/ai_failure_notifier.py. + +Lives here, rather than beside the script, so `tox -e unit` collects it -- +pytest skips dot-directories, so anything under .github/ never runs in CI. + +No network calls and no `gh` calls happen in this file -- OpenRouter and gh +I/O are mocked. FIXTURE below is the extracted signature, candidate issue and +LLM envelope from a real failing scheduled run (28141163589, "Broad Charm +Compatibility Tests", 2026-06-25); the `tail_excerpt` arrays are trimmed for +size; `pytest_failures` and `traceback_top_error` are verbatim, since those are +what the dedup and schema logic actually exercise. +""" + +from __future__ import annotations + +import contextlib +import datetime +import email.message +import io +import json +import pathlib +import sys +import unittest +import urllib.error +from typing import Any +from unittest import mock + +sys.path.insert(0, str(pathlib.Path(__file__).parent.parent / '.github')) + +import ai_failure_notifier as afn + +# The signature, candidate issue and envelope from a real failing scheduled run +# (28141163589, "Broad Charm Compatibility Tests", 2026-06-25). The tail_excerpt +# lists are trimmed for size; pytest_failures and traceback_top_error are +# verbatim, since those are what the dedup and schema logic exercise. +FIXTURE_SIGNATURE = afn.RunSignature( + run_id='28141163589', + workflow_name='Broad Charm Compatibility Tests', + html_url='https://github.com/canonical/operator/actions/runs/28141163589', + created_at='2026-06-25T01:40:15Z', + jobs=[ + afn.JobSignature( + job_id=83338922280, + job_name='charm-tests (canonical/charm-ubuntu, .)', + failed_step="Run the charm's unit tests", + pytest_failures=[ + afn.PytestFailure( + kind='ERROR', + test='tests/unit/test_charm.py::TestCharm::test_charm_ready', + error='PendingDeprecat...', + ), + afn.PytestFailure( + kind='ERROR', + test='tests/unit/test_charm.py::TestCharm::test_hostname', + error='PendingDeprecation...', + ), + afn.PytestFailure( + kind='ERROR', + test='tests/unit/test_charm.py::TestCharm::test_version', + error='PendingDeprecationW...', + ), + ], + go_failures=[], + traceback_top_error=( + 'ResourceWarning: Implicitly cleaning up " + ), + tail_excerpt=[ + 'pytest.PytestUnraisableExceptionWarning: Exception ignored in: ', + 'unit: FAIL code 1 (2.04=setup[1.23]+cmd[0.81] seconds)', + 'evaluation failed :( (2.07 seconds)', + ], + ), + afn.JobSignature( + job_id=83338922315, + job_name='charm-tests (canonical/k8s-operator, charms/worker/k8s)', + failed_step="Run the charm's static tests", + pytest_failures=[], + go_failures=[], + traceback_top_error=None, + tail_excerpt=[ + 'Total issues (by severity):', + 'static: FAIL code 1 (16.01=setup[0.42]+cmd[15.59] seconds)', + 'evaluation failed :( (16.06 seconds)', + ], + ), + afn.JobSignature( + job_id=83338923301, + job_name='charm-tests (canonical/seldon-core-operator, .)', + failed_step="Run the charm's unit tests", + pytest_failures=[ + afn.PytestFailure( + kind='FAILED', + test='tests/unit/test_operator.py::TestCharm::test_prometheus_data_set', + error=( + "AttributeError: module 'ops.testing' has no attribute " + "'_TestingModelBackend'" + ), + ), + ], + go_failures=[], + traceback_top_error=( + "AttributeError: module 'ops.testing' has no attribute '_TestingModelBackend'" + ), + tail_excerpt=[ + 'FAILED tests/unit/test_operator.py::TestCharm::test_prometheus_data_set - ' + "AttributeError: module 'ops.testing' has no attribute '_TestingModelBackend'", + 'unit: FAIL code 1 (8.95=setup[0.60]+cmd[8.35] seconds)', + ], + ), + afn.JobSignature( + job_id=83338923304, + job_name='charm-tests (canonical/self-signed-certificates-operator, .)', + failed_step="Run the charm's unit tests", + pytest_failures=[], + go_failures=[], + traceback_top_error=None, + tail_excerpt=[ + 'ERROR tests/unit/test_charm_collect_status.py', + '!!!!!!!!!!!!!!!!!!! Interrupted: 5 errors during collection !!!!!!!!!!!!!!!!!!!!', + 'unit: FAIL code 2 (3.43=setup[1.18]+cmd[0.06,0.02,2.16] seconds)', + ], + ), + afn.JobSignature( + job_id=83338923318, + job_name='charm-tests (canonical/traefik-k8s-operator, .)', + failed_step="Run the charm's unit tests", + pytest_failures=[], + go_failures=[], + traceback_top_error=( + "ImportError: cannot import name 'Event' from 'scenario' (/home/runner/work/operat" + 'or/operator/charm-repo/.tox/unit/lib/python3.12/site-packages/scenario/__init__.p' + 'y)' + ), + tail_excerpt=[ + ( + "ImportError: cannot import name 'Event' from 'scenario' " + '(/home/runner/work/operator/operator/charm-repo/.tox/unit/lib/python3.12/site' + '-packages/scenario/__init__.py)' + ), + 'unit: FAIL code 1 (6.43=setup[2.14]+cmd[0.06,0.02,4.21] seconds)', + ], + ), + ], +) + +FIXTURE_CANDIDATES = [ + afn.CandidateIssue( + number=9010, + title='Broad Charm Compatibility Tests: 4 downstream charms failing, independent causes', + body=( + 'canonical/k8s-operator: bandit exit 1 (Medium: 24, High: 591). ' + "canonical/seldon-core-operator: AttributeError: module 'ops.testing' has no attribute" + " '_TestingModelBackend'. canonical/self-signed-certificates-operator: 5 collection " + "errors. canonical/traefik-k8s-operator: ImportError: cannot import name 'Event' from " + "'scenario'." + ), + closed_at=None, + ), +] + +FIXTURE_ENVELOPE: dict[str, Any] = { + 'action': 'comment', + 'target_issue': 9010, + 'body': 'Another occurrence: ' + 'https://github.com/canonical/operator/actions/runs/28141163589\n' + '\n' + '4 of the 5 failing charms match #9010 unchanged. New this run: ' + 'canonical/charm-ubuntu is now also failing its unit tests.', + 'dedup_reason': "4 of 5 failing charms match #9010's signature; charm-ubuntu is a " + 'new failure not present in #9010', + 'confidence': 'medium', +} + + +class SignatureExtractionTests(unittest.TestCase): + def test_strip_line_removes_timestamp_and_ansi(self): + raw = '2026-06-25T01:40:15.8141713Z \x1b[36mhello\x1b[0m' + self.assertEqual( + afn.strip_line(raw), + '\x1b[36mhello\x1b[0m'.replace('\x1b[36m', '').replace('\x1b[0m', ''), + ) + + def test_parse_job_log_pytest_failures_and_summary_bound(self): + log = '\n'.join([ + '2026-06-25T01:40:15.0000000Z ============ short test summary info ============', + '2026-06-25T01:40:15.0000000Z FAILED tests/unit/test_x.py::test_a - AssertionError: x', + '2026-06-25T01:40:15.0000000Z ERROR tests/unit/test_x.py::test_b - PendingDeprecat...', + '2026-06-25T01:40:15.0000000Z ============ 2 failed in 1.23s ============', + '2026-06-25T01:40:15.0000000Z some trailing noise, not part of the summary', + ]) + pytest_failures, go_failures, _tb, _tail = afn.parse_job_log(log) + self.assertEqual( + pytest_failures, + [ + afn.PytestFailure('FAILED', 'tests/unit/test_x.py::test_a', 'AssertionError: x'), + afn.PytestFailure('ERROR', 'tests/unit/test_x.py::test_b', 'PendingDeprecat...'), + ], + ) + self.assertEqual(go_failures, []) + + def test_parse_job_log_go_failures(self): + log = '--- FAIL: TestFoo (0.03s)\n--- FAIL: TestBar (0.01s)\n' + _pytest_failures, go_failures, _tb, _tail = afn.parse_job_log(log) + self.assertEqual(go_failures, ['TestFoo', 'TestBar']) + + def test_parse_job_log_traceback_top_error_prefers_last_match(self): + log = '\n'.join([ + 'ValueError: first, ignored', + 'some other output', + 'AttributeError: the real one', + ]) + _, _, tb, _ = afn.parse_job_log(log) + self.assertEqual(tb, 'AttributeError: the real one') + + def test_parse_job_log_tail_excerpt_stops_before_first_error_marker(self): + log = '\n'.join([ + 'line before 1', + 'line before 2', + '##[error]something broke', + 'line after (should not appear in tail)', + ]) + _, _, _, tail = afn.parse_job_log(log) + self.assertEqual(tail, ['line before 1', 'line before 2']) + + def test_build_run_signature_matches_fixture_shape(self): + jobs = [ + afn.build_job_signature(j.job_id, j.job_name, j.failed_step, '') + for j in FIXTURE_SIGNATURE.jobs + ] + sig = afn.build_run_signature( + '28141163589', 'Broad Charm Compatibility Tests', 'url', '2026-06-25T01:40:15Z', jobs + ) + self.assertEqual(sig.run_id, '28141163589') + self.assertEqual(len(sig.jobs), 5) + # as_json is what reaches the prompt; field order is declaration order. + self.assertEqual( + list(json.loads(sig.as_json())), + ['run_id', 'workflow_name', 'html_url', 'created_at', 'jobs'], + ) + + +class MarkerTests(unittest.TestCase): + def test_render_and_parse_notifier_marker(self): + marker = afn.render_notifier_marker('123', 'new') + enriched, origin_kind, origin_issue = afn.find_run_markers([(42, marker)], '123') + self.assertIsNone(enriched) + self.assertEqual(origin_kind, 'new') + self.assertEqual(origin_issue, 42) + + def test_render_and_parse_enriched_marker_is_rung_zero(self): + marker = afn.render_enriched_marker('28141163589', FIXTURE_SIGNATURE) + run_id = '28141163589' + enriched, origin_kind, _origin_issue = afn.find_run_markers([(9010, marker)], run_id) + self.assertEqual(enriched, 9010) + self.assertIsNone(origin_kind) + + def test_marker_for_different_run_id_does_not_match(self): + marker = afn.render_notifier_marker('999', 'comment') + enriched, origin_kind, origin_issue = afn.find_run_markers([(1, marker)], '123') + self.assertIsNone(enriched) + self.assertIsNone(origin_kind) + self.assertIsNone(origin_issue) + + def test_signature_hash_is_deterministic_and_order_independent_of_call(self): + h1 = afn.signature_hash(FIXTURE_SIGNATURE) + h2 = afn.signature_hash(FIXTURE_SIGNATURE) # independently constructed + self.assertEqual(h1, h2) + self.assertEqual(len(h1), 16) + + def test_no_marker_present_returns_all_none(self): + enriched, origin_kind, origin_issue = afn.find_run_markers( + [(1, 'just a normal comment, no marker')], '123' + ) + self.assertIsNone(enriched) + self.assertIsNone(origin_kind) + self.assertIsNone(origin_issue) + + +class CandidateBlockTests(unittest.TestCase): + def test_open_candidate_rendered(self): + block = afn.build_candidates_block( + FIXTURE_CANDIDATES, [], datetime.datetime.now(datetime.timezone.utc) + ) + self.assertIn('#9010', block) + self.assertIn('Broad Charm Compatibility Tests', block) + self.assertNotIn('closed', block) + + def test_empty_candidates_block(self): + block = afn.build_candidates_block([], [], datetime.datetime.now(datetime.timezone.utc)) + self.assertEqual(block, '(no open issues found for this workflow)') + + def test_recently_closed_candidate_is_labelled_and_capped_at_medium(self): + now = datetime.datetime(2026, 6, 25, tzinfo=datetime.timezone.utc) + closed = [ + afn.CandidateIssue.from_gh({ + 'number': 42, + 'title': 'old thing', + 'body': 'x', + 'closedAt': '2026-06-20T00:00:00Z', + }) + ] + block = afn.build_candidates_block([], closed, now) + self.assertIn('#42', block) + self.assertIn('closed', block) + self.assertIn('medium-confidence', block) + + def test_closed_candidate_outside_window_is_dropped(self): + now = datetime.datetime(2026, 6, 25, tzinfo=datetime.timezone.utc) + closed = [ + afn.CandidateIssue.from_gh({ + 'number': 42, + 'title': 'ancient', + 'body': 'x', + 'closedAt': '2026-01-01T00:00:00Z', + }) + ] + block = afn.build_candidates_block([], closed, now) + self.assertEqual(block, '(no open issues found for this workflow)') + + def test_candidates_capped_at_three(self): + opens = [ + afn.CandidateIssue.from_gh({ + 'number': n, + 'title': f'issue {n}', + 'body': 'x', + 'closedAt': None, + }) + for n in range(5) + ] + block = afn.build_candidates_block(opens, [], datetime.datetime.now(datetime.timezone.utc)) + self.assertEqual(block.count('- **#'), 3) + + +class SchemaValidationTests(unittest.TestCase): + def test_valid_comment_envelope_from_fixture(self): + errors = afn.validate_envelope(FIXTURE_ENVELOPE) + self.assertEqual(errors, []) + + def test_valid_new_envelope(self): + envelope: dict[str, Any] = { + 'action': 'new', + 'title': 'Something failed', + 'body': 'details', + 'labels': ['tests'], + 'issue_type': None, + 'dedup_reason': 'no match', + 'confidence': 'low', + } + self.assertEqual(afn.validate_envelope(envelope), []) + + def test_new_envelope_with_no_labels_is_valid(self): + # No label is mandatory: the repo's label set is centrally managed, and + # an empty list is the right answer when none of it applies. + envelope: dict[str, Any] = { + 'action': 'new', + 'title': 'Something failed', + 'body': 'details', + 'labels': [], + 'issue_type': None, + 'dedup_reason': 'no match', + 'confidence': 'low', + } + self.assertEqual(afn.validate_envelope(envelope), []) + + def test_new_envelope_with_non_string_labels_is_invalid(self): + envelope: dict[str, Any] = { + 'action': 'new', + 'title': 'Something failed', + 'body': 'details', + 'labels': ['tests', 7], + 'issue_type': None, + 'dedup_reason': 'no match', + 'confidence': 'low', + } + self.assertTrue(any('labels' in e for e in afn.validate_envelope(envelope))) + + def test_new_envelope_with_target_issue_is_invalid(self): + envelope = { + 'action': 'new', + 'title': 't', + 'body': 'b', + 'labels': ['tests'], + 'issue_type': None, + 'dedup_reason': 'd', + 'confidence': 'low', + 'target_issue': 5, + } + errors = afn.validate_envelope(envelope) + self.assertTrue(any('target_issue' in e for e in errors)) + + def test_comment_envelope_with_title_is_invalid(self): + envelope = { + 'action': 'comment', + 'target_issue': 5, + 'title': 'should not be here', + 'body': 'b', + 'dedup_reason': 'd', + 'confidence': 'high', + } + errors = afn.validate_envelope(envelope) + self.assertTrue(any('title' in e for e in errors)) + + def test_bad_action_value_is_invalid(self): + envelope = {'action': 'delete', 'body': 'b', 'dedup_reason': 'd', 'confidence': 'high'} + errors = afn.validate_envelope(envelope) + self.assertTrue(any('action' in e for e in errors)) + + def test_also_capped_at_two_entries(self): + base = dict(FIXTURE_ENVELOPE) + base['also'] = [dict(FIXTURE_ENVELOPE) for _ in range(3)] + errors = afn.validate_envelope(base) + self.assertTrue(any('also' in e for e in errors)) + + def test_nested_also_is_invalid(self): + base = dict(FIXTURE_ENVELOPE) + inner = dict(FIXTURE_ENVELOPE) + inner['also'] = [dict(FIXTURE_ENVELOPE)] + base['also'] = [inner] + errors = afn.validate_envelope(base) + self.assertTrue(any('also' in e for e in errors)) + + def test_also_entries_individually_validated(self): + base = dict(FIXTURE_ENVELOPE) + broken = {'action': 'comment'} # missing body/dedup_reason/confidence/target_issue + base['also'] = [broken] + errors = afn.validate_envelope(base) + self.assertTrue(any('also[0]' in e for e in errors)) + + +class MainFlowTests(unittest.TestCase): + """Exercises main()'s branching with gh and OpenRouter mocked out -- + No live gh or OpenRouter calls happen in this test. + """ + + def setUp(self): + self.env = { + 'REPO': 'canonical/operator', + 'RUN_ID': '28141163589', + 'WORKFLOW_NAME': 'Broad Charm Compatibility Tests', + 'RUN_URL': 'https://github.com/canonical/operator/actions/runs/28141163589', + 'OPENROUTER_API_KEY': 'test-key', + } + + def _patch_common( + self, + *, + locate_return: tuple[int | None, str | None, int | None], + gh_calls: mock.Mock, + ) -> list[Any]: + patches = [ + mock.patch.object(afn, 'locate_run_markers', return_value=locate_return), + mock.patch.object(afn, 'fetch_failed_jobs', return_value=[]), + mock.patch.object( + afn, 'fetch_run_meta', return_value={'createdAt': '2026-06-25T01:40:15Z'} + ), + mock.patch.object(afn, 'search_candidates', return_value=(FIXTURE_CANDIDATES, [])), + mock.patch.object(afn, 'existing_labels', return_value={'tests', 'docs'}), + mock.patch.object(afn, 'gh', side_effect=gh_calls), + mock.patch.object(afn, 'write_step_summary'), + mock.patch.object(afn, 'set_output'), + ] + return patches + + def test_rung_zero_comments_and_skips_llm(self): + gh_calls = mock.Mock(return_value=mock.Mock(returncode=0, stdout='', stderr='')) + patches = self._patch_common(locate_return=(9010, None, None), gh_calls=gh_calls) + with ( + mock.patch.dict('os.environ', self.env, clear=True), + mock.patch.object(afn, 'call_openrouter') as call_openrouter, + contextlib.ExitStack() as stack, + ): + for p in patches: + stack.enter_context(p) + rc = afn.main() + self.assertEqual(rc, 0) + call_openrouter.assert_not_called() + gh_calls.assert_called_once() + self.assertEqual(gh_calls.call_args.args[:3], ('issue', 'comment', '9010')) + + def test_valid_llm_response_upgrades_placeholder_in_place(self): + gh_calls = mock.Mock(return_value=mock.Mock(returncode=0, stdout='', stderr='')) + patches = self._patch_common(locate_return=(None, 'new', 4242), gh_calls=gh_calls) + envelope = { + 'action': 'new', + 'title': 'x', + 'body': 'y', + 'labels': ['tests'], + 'issue_type': None, + 'dedup_reason': 'd', + 'confidence': 'medium', + } + with ( + mock.patch.dict('os.environ', self.env, clear=True), + mock.patch.object(afn, 'call_openrouter', return_value=envelope), + contextlib.ExitStack() as stack, + ): + for p in patches: + stack.enter_context(p) + rc = afn.main() + self.assertEqual(rc, 0) + edit_calls = [c for c in gh_calls.call_args_list if c.args[:2] == ('issue', 'edit')] + self.assertEqual(len(edit_calls), 1) + self.assertEqual(edit_calls[0].args[2], '4242') + + def test_invalid_llm_response_falls_back_to_plain_comment(self): + gh_calls = mock.Mock(return_value=mock.Mock(returncode=0, stdout='', stderr='')) + patches = self._patch_common(locate_return=(None, 'comment', 4242), gh_calls=gh_calls) + with ( + mock.patch.dict('os.environ', self.env, clear=True), + mock.patch.object( + afn, 'call_openrouter', return_value={'action': 'not-a-real-action'} + ), + contextlib.ExitStack() as stack, + ): + for p in patches: + stack.enter_context(p) + rc = afn.main() + self.assertEqual(rc, 0) + comment_calls = [c for c in gh_calls.call_args_list if c.args[:2] == ('issue', 'comment')] + self.assertEqual(len(comment_calls), 1) + self.assertEqual(comment_calls[0].args[2], '4242') + + def test_no_api_key_uses_plain_fallback_without_calling_llm(self): + gh_calls = mock.Mock(return_value=mock.Mock(returncode=0, stdout='', stderr='')) + patches = self._patch_common(locate_return=(None, 'new', 4242), gh_calls=gh_calls) + env = dict(self.env) + env.pop('OPENROUTER_API_KEY') + with ( + mock.patch.dict('os.environ', env, clear=True), + mock.patch.object(afn, 'call_openrouter') as call_openrouter, + contextlib.ExitStack() as stack, + ): + for p in patches: + stack.enter_context(p) + rc = afn.main() + self.assertEqual(rc, 0) + call_openrouter.assert_not_called() + + +class GhCallShapeTests(unittest.TestCase): + """Pins the argv of each read-only gh call. + + These mock only the `gh` subprocess boundary, not the functions under + test, so a wrong flag or a mis-quoted positional is visible here. The + MainFlowTests above patch out `locate_run_markers` and `search_candidates` + wholesale, which is why both shipped with argv bugs that 29 green tests + did not catch -- see the 2026-07-25 dev-box run against canonical/operator. + """ + + def _capture(self, stdout: str = '[]') -> mock.Mock: + return mock.Mock(return_value=mock.Mock(returncode=0, stdout=stdout, stderr='')) + + def test_search_issue_numbers_passes_repo_as_a_flag(self): + gh_calls = self._capture('[{"number": 2658}]') + with mock.patch.object(afn, 'gh', side_effect=gh_calls): + numbers = afn.search_issue_numbers('canonical/operator', 'Example Charm Tests') + self.assertEqual(numbers, [2658]) + args = gh_calls.call_args.args + self.assertEqual(args[:2], ('search', 'issues')) + self.assertIn('--repo', args) + self.assertEqual(args[args.index('--repo') + 1], 'canonical/operator') + # The query is a bare positional -- no `repo:` prefix, no added quotes. + # `gh search issues` quotes each positional as one keyword, so folding + # the repo in produces `repo:"canonical/operator \"text\""`, which + # GitHub rejects with "Invalid search query". + self.assertIn('Example Charm Tests', args) + for arg in args: + self.assertNotIn('repo:canonical/operator', arg) + + def test_search_candidates_passes_state_and_search_flags(self): + gh_calls = self._capture('[]') + with mock.patch.object(afn, 'gh', side_effect=gh_calls): + afn.search_candidates('canonical/operator', 'Example Charm Tests') + states: list[str] = [] + for call in gh_calls.call_args_list: + args = call.args + self.assertEqual(args[:2], ('issue', 'list')) + self.assertEqual(args[args.index('--repo') + 1], 'canonical/operator') + self.assertEqual(args[args.index('--search') + 1], '"Example Charm Tests"') + states.append(args[args.index('--state') + 1]) + self.assertEqual(states, ['open', 'closed']) + + def test_fetch_job_log_uses_the_rest_logs_endpoint(self): + gh_calls = self._capture('2026-07-21T16:17:04Z some log line\n') + with mock.patch.object(afn, 'gh', side_effect=gh_calls): + log = afn.fetch_job_log('canonical/operator', '29847889218', 88693036489) + self.assertIn('some log line', log) + self.assertEqual( + gh_calls.call_args.args, + ('api', 'repos/canonical/operator/actions/jobs/88693036489/logs'), + ) + + def test_fetch_job_log_reports_an_empty_log_instead_of_swallowing_it(self): + gh_calls = self._capture('') + with ( + mock.patch.object(afn, 'gh', side_effect=gh_calls), + mock.patch.object(afn, 'write_step_summary') as summary, + ): + log = afn.fetch_job_log('canonical/operator', '29847889218', 88693036489) + self.assertEqual(log, '') + summary.assert_called_once() + self.assertIn('no log text', summary.call_args.args[0]) + + def test_fetch_failed_jobs_requests_the_jobs_field(self): + gh_calls = self._capture( + '{"jobs": [{"databaseId": 1, "name": "j", "conclusion": "failure",' + ' "steps": [{"name": "s", "conclusion": "failure"}]}]}' + ) + with mock.patch.object(afn, 'gh', side_effect=gh_calls): + jobs = afn.fetch_failed_jobs('canonical/operator', '29847889218') + self.assertEqual(jobs, [afn.FailedJob(id=1, name='j', failed_step='s')]) + args = gh_calls.call_args.args + self.assertEqual(args[:3], ('run', 'view', '29847889218')) + self.assertEqual(args[args.index('--json') + 1], 'jobs') + + def test_existing_labels_requests_the_name_field(self): + gh_calls = self._capture('[{"name": "tests"}, {"name": "docs"}]') + with mock.patch.object(afn, 'gh', side_effect=gh_calls): + labels = afn.existing_labels('canonical/operator') + self.assertEqual(labels, {'tests', 'docs'}) + args = gh_calls.call_args.args + self.assertEqual(args[:2], ('label', 'list')) + self.assertEqual(args[args.index('--json') + 1], 'name') + + +class MainDegradationTests(unittest.TestCase): + """main() degrades through its own fallbacks when a gh search fails. + + Without these, a search failure raises out of main(), kills the enrich + job, and hands every run to the workflow-level plain-fallback -- so + enrichment silently never happens and the job still looks healthy. + """ + + def setUp(self): + self.env = { + 'REPO': 'canonical/operator', + 'RUN_ID': '28141163589', + 'WORKFLOW_NAME': 'Broad Charm Compatibility Tests', + 'RUN_URL': 'https://github.com/canonical/operator/actions/runs/28141163589', + } + + def test_marker_lookup_failure_does_not_crash_main(self): + gh_calls = mock.Mock( + return_value=mock.Mock( + returncode=0, stdout='https://github.com/canonical/operator/issues/9999', stderr='' + ) + ) + with ( + mock.patch.dict('os.environ', self.env, clear=True), + mock.patch.object(afn, 'locate_run_markers', side_effect=RuntimeError('boom')), + mock.patch.object(afn, 'fetch_failed_jobs', return_value=[]), + mock.patch.object(afn, 'fetch_run_meta', return_value={'createdAt': ''}), + mock.patch.object(afn, 'existing_labels', return_value=set()), + mock.patch.object(afn, 'gh', side_effect=gh_calls), + mock.patch.object(afn, 'write_step_summary') as summary, + mock.patch.object(afn, 'set_output') as set_output, + ): + rc = afn.main() + self.assertEqual(rc, 0) + set_output.assert_called_with('handled', 'true') + self.assertTrue( + any('Marker lookup failed' in c.args[0] for c in summary.call_args_list), + 'the failure should be reported in the step summary', + ) + + def test_candidate_search_failure_proceeds_with_no_candidates(self): + gh_calls = mock.Mock(return_value=mock.Mock(returncode=0, stdout='', stderr='')) + env = dict(self.env, OPENROUTER_API_KEY='test-key') + with ( + mock.patch.dict('os.environ', env, clear=True), + mock.patch.object(afn, 'locate_run_markers', return_value=(None, 'new', 4242)), + mock.patch.object(afn, 'fetch_failed_jobs', return_value=[]), + mock.patch.object(afn, 'fetch_run_meta', return_value={'createdAt': ''}), + mock.patch.object(afn, 'search_candidates', side_effect=RuntimeError('boom')), + mock.patch.object(afn, 'existing_labels', return_value=set()), + mock.patch.object( + afn, 'call_openrouter', return_value={'action': 'not-a-real-action'} + ), + mock.patch.object(afn, 'gh', side_effect=gh_calls), + mock.patch.object(afn, 'write_step_summary') as summary, + mock.patch.object(afn, 'set_output'), + ): + rc = afn.main() + self.assertEqual(rc, 0) + self.assertTrue( + any('Candidate search failed' in c.args[0] for c in summary.call_args_list), + 'the failure should be reported in the step summary', + ) + + +class OpenRouterCallTests(unittest.TestCase): + """The OpenRouter call is built with urllib, so it has no third-party deps. + + Previously this function was only ever mocked, so nothing checked the + request it actually builds. + """ + + def _response(self, content: str) -> mock.MagicMock: + payload = json.dumps({'choices': [{'message': {'content': content}}]}).encode() + response = mock.MagicMock() + response.read.return_value = payload + response.__enter__.return_value = response + return response + + def test_posts_json_with_auth_and_schema(self): + envelope = {'action': 'new', 'body': 'b'} + with mock.patch.object( + afn.urllib.request, 'urlopen', return_value=self._response(json.dumps(envelope)) + ) as urlopen: + result = afn.call_openrouter('sys', 'user', 'some/model', 'secret-key') + + self.assertEqual(result, envelope) + request = urlopen.call_args.args[0] + self.assertEqual(request.method, 'POST') + self.assertEqual(request.full_url, 'https://openrouter.ai/api/v1/chat/completions') + # urllib title-cases header keys, so compare case-insensitively. + headers = {k.lower(): v for k, v in request.headers.items()} + self.assertEqual(headers['Authorization'.lower()], 'Bearer secret-key') + self.assertEqual(headers['Content-type'.lower()], 'application/json') + self.assertEqual(urlopen.call_args.kwargs['timeout'], 60) + + sent = json.loads(request.data.decode()) + self.assertEqual(sent['model'], 'some/model') + self.assertEqual([m['role'] for m in sent['messages']], ['system', 'user']) + self.assertEqual(sent['messages'][0]['content'], 'sys') + self.assertEqual(sent['messages'][1]['content'], 'user') + self.assertEqual(sent['response_format']['type'], 'json_schema') + self.assertEqual( + sent['response_format']['json_schema']['schema'], afn.ENVELOPE_JSON_SCHEMA + ) + self.assertTrue(sent['response_format']['json_schema']['strict']) + + def test_http_error_propagates_so_main_can_fall_back(self): + # HTTPError holds a file object and warns on implicit cleanup, which + # the unit env's -W error turns into a failure. Give it a real `fp` + # (it fabricates a tempfile when passed None) and close it explicitly. + error = urllib.error.HTTPError( + 'https://openrouter.ai/api/v1/chat/completions', + 500, + 'boom', + email.message.Message(), + io.BytesIO(b''), + ) + self.addCleanup(error.close) + with mock.patch.object(afn.urllib.request, 'urlopen', side_effect=error): + with self.assertRaises(urllib.error.HTTPError): + afn.call_openrouter('sys', 'user', 'm', 'k') + + +if __name__ == '__main__': + unittest.main() From 2710672b3058de52450bb602fbc4706647c4cd75 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Sat, 25 Jul 2026 13:23:46 +1200 Subject: [PATCH 02/37] fix: append the Workflow footer, and report fallbacks to the log Both found by dogfooding in the fork. The `Workflow: ` 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. --- .github/ai_failure_notifier.py | 49 +++++++++++++++++------- test/test_ai_failure_notifier.py | 64 ++++++++++++++++++++++++++++++++ 2 files changed, 100 insertions(+), 13 deletions(-) diff --git a/.github/ai_failure_notifier.py b/.github/ai_failure_notifier.py index f03bdda09..d1c522403 100644 --- a/.github/ai_failure_notifier.py +++ b/.github/ai_failure_notifier.py @@ -1007,13 +1007,17 @@ def call_openrouter( def write_step_summary(message: str) -> None: - """Append a line to the job's step summary (or stderr, outside Actions).""" + """Report a line to the job's step summary and to the log. + + Always goes to stderr as well as the summary: every fallback path in this + script reports through here, and a fallback that only shows up in the + summary is invisible to anyone reading the job log or the API. + """ + print(message, file=sys.stderr) path = os.environ.get('GITHUB_STEP_SUMMARY') - if not path: - print(message, file=sys.stderr) - return - with open(path, 'a', encoding='utf-8') as f: - f.write(message + '\n') + if path: + with open(path, 'a', encoding='utf-8') as f: + f.write(message + '\n') def set_output(name: str, value: str) -> None: @@ -1030,11 +1034,27 @@ def plain_fallback_body(workflow_name: str, run_url: str) -> str: return f"Scheduled workflow '{workflow_name}' failed: {run_url}" +def render_body(body: str, workflow_name: str, marker: str) -> str: + """Assemble an issue or comment body, footer and marker included. + + The `Workflow: ` footer is what keeps the notifier's coarse search + working after enrichment has rewritten the title and body: the search + matches on the workflow name, and without the footer it would depend on + the model happening to leave the name in the title. + """ + return f'{body.rstrip()}\n\nWorkflow: {workflow_name}\n\n{marker}' + + def apply_entry( - repo: str, entry: dict[str, Any], marker: str, *, default_target: int | None = None + repo: str, + entry: dict[str, Any], + marker: str, + workflow_name: str, + *, + default_target: int | None = None, ) -> str: """Create or comment on an issue per one envelope entry, stamping `marker`.""" - body = entry['body'].rstrip() + f'\n\n{marker}' + body = render_body(entry['body'], workflow_name, marker) if entry['action'] == 'new': # The repo's label set is centrally managed, so anything the model # asked for that doesn't exist is dropped rather than created. @@ -1156,6 +1176,7 @@ def main() -> int: 'target_issue': origin_issue, }, enriched_marker, + workflow_name, default_target=origin_issue, ) set_output('handled', 'true') @@ -1192,6 +1213,7 @@ def main() -> int: 'target_issue': origin_issue, }, enriched_marker, + workflow_name, default_target=origin_issue, ) set_output('handled', 'true') @@ -1218,6 +1240,7 @@ def main() -> int: 'target_issue': origin_issue, }, enriched_marker, + workflow_name, default_target=origin_issue, ) set_output('handled', 'true') @@ -1236,16 +1259,16 @@ def main() -> int: '--title', envelope['title'], '--body', - envelope['body'].rstrip() + f'\n\n{enriched_marker}', + render_body(envelope['body'], workflow_name, enriched_marker), ] for label in labels: edit_args += ['--add-label', label] gh(*edit_args) elif envelope['action'] == 'comment' and envelope.get('target_issue') == origin_issue: - apply_entry(repo, envelope, enriched_marker, default_target=origin_issue) + apply_entry(repo, envelope, enriched_marker, workflow_name, default_target=origin_issue) elif envelope['action'] == 'comment': # LLM picked a different candidate than the notifier's coarse match. - apply_entry(repo, envelope, enriched_marker) + apply_entry(repo, envelope, enriched_marker, workflow_name) if origin_kind == 'comment': gh( 'issue', @@ -1260,7 +1283,7 @@ def main() -> int: else: # action == "new" but origin_kind == "comment": the coarse title # match landed on an unrelated older issue; this is genuinely new. - apply_entry(repo, envelope, enriched_marker) + apply_entry(repo, envelope, enriched_marker, workflow_name) gh( 'issue', 'comment', @@ -1273,7 +1296,7 @@ def main() -> int: ) for also_entry in envelope.get('also') or []: - apply_entry(repo, also_entry, enriched_marker) + apply_entry(repo, also_entry, enriched_marker, workflow_name) set_output('handled', 'true') return 0 diff --git a/test/test_ai_failure_notifier.py b/test/test_ai_failure_notifier.py index 28bcf4009..f2c4472f8 100644 --- a/test/test_ai_failure_notifier.py +++ b/test/test_ai_failure_notifier.py @@ -37,8 +37,10 @@ import email.message import io import json +import os import pathlib import sys +import tempfile import unittest import urllib.error from typing import Any @@ -644,6 +646,68 @@ def test_existing_labels_requests_the_name_field(self): self.assertEqual(args[args.index('--json') + 1], 'name') +class BodyFooterTests(unittest.TestCase): + """Every body carries the footer the notifier's coarse search matches on. + + Found by dogfooding: the footer was described in the design and in the + notifier's own comment, but only ever existed in the prompt template, so + enriched issues went out without it. The coarse search then depended on + the model happening to leave the workflow name in the title. + """ + + def test_render_body_has_footer_and_marker(self): + body = afn.render_body('Some detail.', 'Example Charm Tests', '') + self.assertEqual(body, 'Some detail.\n\nWorkflow: Example Charm Tests\n\n') + + def test_applied_comment_body_has_the_footer(self): + gh_calls = mock.Mock(return_value=mock.Mock(returncode=0, stdout='', stderr='')) + entry: dict[str, Any] = {'action': 'comment', 'body': 'Another occurrence.'} + with mock.patch.object(afn, 'gh', side_effect=gh_calls): + afn.apply_entry( + 'canonical/operator', entry, '', 'ops Smoke Tests', default_target=7 + ) + args = gh_calls.call_args.args + self.assertEqual(args[:3], ('issue', 'comment', '7')) + self.assertIn('Workflow: ops Smoke Tests', args[args.index('--body') + 1]) + + def test_applied_new_issue_body_has_the_footer(self): + gh_calls = mock.Mock( + return_value=mock.Mock(returncode=0, stdout='https://x/issues/9', stderr='') + ) + entry: dict[str, Any] = { + 'action': 'new', + 'title': 't', + 'body': 'Detail.', + 'labels': [], + 'issue_type': None, + } + with ( + mock.patch.object(afn, 'gh', side_effect=gh_calls), + mock.patch.object(afn, 'existing_labels', return_value=set()), + ): + afn.apply_entry('canonical/operator', entry, '', 'ops Smoke Tests') + args = gh_calls.call_args.args + self.assertIn('Workflow: ops Smoke Tests', args[args.index('--body') + 1]) + + +class StepSummaryTests(unittest.TestCase): + """Fallback reporting reaches the job log, not only the step summary.""" + + def test_message_goes_to_stderr_even_with_a_summary_file(self): + with tempfile.NamedTemporaryFile('w+', suffix='.md', delete=False) as handle: + summary_path = handle.name + self.addCleanup(os.unlink, summary_path) + stderr = io.StringIO() + with ( + mock.patch.dict(os.environ, {'GITHUB_STEP_SUMMARY': summary_path}, clear=True), + contextlib.redirect_stderr(stderr), + ): + afn.write_step_summary('OpenRouter call failed (boom)') + self.assertIn('OpenRouter call failed (boom)', stderr.getvalue()) + with open(summary_path, encoding='utf-8') as handle: + self.assertIn('OpenRouter call failed (boom)', handle.read()) + + class MainDegradationTests(unittest.TestCase): """main() degrades through its own fallbacks when a gh search fails. From b51bb2e13b80fec52622a127abeca7a378ae3a77 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Sat, 25 Jul 2026 13:28:18 +1200 Subject: [PATCH 03/37] fix: stop rejecting every envelope that carries `also` 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. --- .github/ai_failure_notifier.py | 19 +++++++++++-------- test/test_ai_failure_notifier.py | 25 +++++++++++++++++++++++-- 2 files changed, 34 insertions(+), 10 deletions(-) diff --git a/.github/ai_failure_notifier.py b/.github/ai_failure_notifier.py index d1c522403..136a06bc8 100644 --- a/.github/ai_failure_notifier.py +++ b/.github/ai_failure_notifier.py @@ -668,8 +668,14 @@ def build_prompt( } -def validate_entry(entry: Any, *, path: str) -> list[str]: - """Validate one envelope entry (top-level or an `also[i]`) against the schema.""" +def validate_entry(entry: Any, *, path: str, allow_also: bool = False) -> list[str]: + """Validate one envelope entry (top-level or an `also[i]`) against the schema. + + `also` is only legal on the top-level envelope, so the caller says whether + this is that. Without it the top-level check rejected every envelope that + carried `also` -- which the model emits routinely, since the schema it is + given declares the field. + """ errors: list[str] = [] if not isinstance(entry, dict): return [f'{path}: expected an object, got {type(entry).__name__}'] @@ -678,7 +684,8 @@ def validate_entry(entry: Any, *, path: str) -> list[str]: if field not in entry: errors.append(f"{path}: missing required field '{field}'") - unknown = set(entry) - _ENTRY_KNOWN_KEYS + known = _ENTRY_KNOWN_KEYS | {'also'} if allow_also else _ENTRY_KNOWN_KEYS + unknown = set(entry) - known if unknown: errors.append(f'{path}: unknown field(s) {sorted(unknown)}') @@ -731,7 +738,7 @@ def validate_envelope(envelope: Any) -> list[str]: if not isinstance(envelope, dict): return ['envelope: expected a JSON object'] - errors = validate_entry(envelope, path='envelope') + errors = validate_entry(envelope, path='envelope', allow_also=True) also = envelope.get('also') if also is not None: @@ -743,10 +750,6 @@ def validate_envelope(envelope: Any) -> list[str]: errors.append(f"envelope.also[{i}]: nested 'also' is not allowed") errors.extend(validate_entry(entry, path=f'envelope.also[{i}]')) - unknown_top = set(envelope) - _ENTRY_KNOWN_KEYS - {'also'} - if unknown_top: - errors.append(f'envelope: unknown field(s) {sorted(unknown_top)}') - return errors diff --git a/test/test_ai_failure_notifier.py b/test/test_ai_failure_notifier.py index f2c4472f8..c4e2c6909 100644 --- a/test/test_ai_failure_notifier.py +++ b/test/test_ai_failure_notifier.py @@ -428,11 +428,32 @@ def test_bad_action_value_is_invalid(self): errors = afn.validate_envelope(envelope) self.assertTrue(any('action' in e for e in errors)) + def test_envelope_with_also_is_valid(self): + # Regression: `also` is legal on the top-level envelope, and the model + # emits it routinely because the schema it is given declares it. The + # top-level unknown-field check used to reject every envelope carrying + # it, so the LLM path always fell back to the plain body. The three + # tests below did not catch it: they assert invalidity and match on the + # substring "also", which the spurious error also contained. + base = dict(FIXTURE_ENVELOPE) + base['also'] = [dict(FIXTURE_ENVELOPE)] + self.assertEqual(afn.validate_envelope(base), []) + + def test_envelope_with_empty_also_is_valid(self): + base = dict(FIXTURE_ENVELOPE) + base['also'] = [] + self.assertEqual(afn.validate_envelope(base), []) + + def test_genuinely_unknown_top_level_field_is_still_invalid(self): + base = dict(FIXTURE_ENVELOPE) + base['nonsense'] = 1 + self.assertTrue(any('nonsense' in e for e in afn.validate_envelope(base))) + def test_also_capped_at_two_entries(self): base = dict(FIXTURE_ENVELOPE) base['also'] = [dict(FIXTURE_ENVELOPE) for _ in range(3)] errors = afn.validate_envelope(base) - self.assertTrue(any('also' in e for e in errors)) + self.assertTrue(any('at most two entries' in e for e in errors), errors) def test_nested_also_is_invalid(self): base = dict(FIXTURE_ENVELOPE) @@ -440,7 +461,7 @@ def test_nested_also_is_invalid(self): inner['also'] = [dict(FIXTURE_ENVELOPE)] base['also'] = [inner] errors = afn.validate_envelope(base) - self.assertTrue(any('also' in e for e in errors)) + self.assertTrue(any("nested 'also'" in e for e in errors), errors) def test_also_entries_individually_validated(self): base = dict(FIXTURE_ENVELOPE) From 5b0534ec81ec1e399c7e1d080f525fca4284611f Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Sat, 25 Jul 2026 13:33:01 +1200 Subject: [PATCH 04/37] fix: keep the matched issue as a candidate, and tolerate null fields 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. --- .github/ai_failure_notifier.py | 17 ++++- test/test_ai_failure_notifier.py | 113 +++++++++++++++++++++++++++++++ 2 files changed, 127 insertions(+), 3 deletions(-) diff --git a/.github/ai_failure_notifier.py b/.github/ai_failure_notifier.py index 136a06bc8..d76098bf9 100644 --- a/.github/ai_failure_notifier.py +++ b/.github/ai_failure_notifier.py @@ -703,11 +703,15 @@ def validate_entry(entry: Any, *, path: str, allow_also: bool = False) -> list[s f'{path}.confidence: must be one of high/medium/low, got {entry.get("confidence")!r}' ) + # A field present but null counts as absent. The schema sent to OpenRouter + # is `strict`, so models routinely return every declared property and use + # null for the ones that do not apply to the action they chose; rejecting on + # mere presence threw away otherwise good output. if action == 'new': for field in ('title', 'labels', 'issue_type'): if field not in entry: errors.append(f"{path}: action='new' requires '{field}'") - if 'target_issue' in entry: + if entry.get('target_issue') is not None: errors.append(f"{path}: action='new' must not include 'target_issue'") labels = entry.get('labels') if labels is not None and ( @@ -724,7 +728,7 @@ def validate_entry(entry: Any, *, path: str, allow_also: bool = False) -> list[s elif not isinstance(entry['target_issue'], int) or entry['target_issue'] < 1: errors.append(f'{path}.target_issue: must be a positive integer') for field in ('title', 'labels', 'issue_type'): - if field in entry: + if entry.get(field) is not None: errors.append(f"{path}: action='comment' must not include '{field}'") return errors @@ -1190,7 +1194,14 @@ def main() -> int: except Exception as exc: # as above: degrade to "no candidates", don't crash. write_step_summary(f'Candidate search failed ({exc}); proceeding with no candidates.') open_candidates, closed_candidates = [], [] - open_candidates = [c for c in open_candidates if c.number != origin_issue] + if origin_kind == 'new': + # The placeholder this run just created is not a candidate to dedupe + # against. An issue the notifier *commented* on is a different matter: + # it already existed, the coarse search matched it, and it is the most + # likely duplicate -- dropping it left the model blind to the very + # issue it should have been comparing against, so it answered "new" + # and produced the duplicate this whole path exists to avoid. + open_candidates = [c for c in open_candidates if c.number != origin_issue] candidates_block = build_candidates_block( open_candidates, closed_candidates, datetime.datetime.now(datetime.timezone.utc) ) diff --git a/test/test_ai_failure_notifier.py b/test/test_ai_failure_notifier.py index c4e2c6909..548dc39bc 100644 --- a/test/test_ai_failure_notifier.py +++ b/test/test_ai_failure_notifier.py @@ -449,6 +449,61 @@ def test_genuinely_unknown_top_level_field_is_still_invalid(self): base['nonsense'] = 1 self.assertTrue(any('nonsense' in e for e in afn.validate_envelope(base))) + def test_new_envelope_with_null_target_issue_is_valid(self): + # The schema sent to OpenRouter is `strict`, so models return every + # declared property and null the inapplicable ones. Rejecting on mere + # presence discarded good output; only a real value is a conflict. + envelope: dict[str, Any] = { + 'action': 'new', + 'title': 't', + 'body': 'b', + 'labels': ['tests'], + 'issue_type': None, + 'target_issue': None, + 'dedup_reason': 'd', + 'confidence': 'low', + } + self.assertEqual(afn.validate_envelope(envelope), []) + + def test_new_envelope_with_a_real_target_issue_is_still_invalid(self): + envelope: dict[str, Any] = { + 'action': 'new', + 'title': 't', + 'body': 'b', + 'labels': [], + 'issue_type': None, + 'target_issue': 7, + 'dedup_reason': 'd', + 'confidence': 'low', + } + self.assertTrue( + any('target_issue' in e for e in afn.validate_envelope(envelope)), + ) + + def test_comment_envelope_with_null_new_only_fields_is_valid(self): + envelope: dict[str, Any] = { + 'action': 'comment', + 'target_issue': 7, + 'body': 'b', + 'title': None, + 'labels': None, + 'issue_type': None, + 'dedup_reason': 'd', + 'confidence': 'high', + } + self.assertEqual(afn.validate_envelope(envelope), []) + + def test_comment_envelope_with_a_real_title_is_still_invalid(self): + envelope: dict[str, Any] = { + 'action': 'comment', + 'target_issue': 7, + 'body': 'b', + 'title': 'nope', + 'dedup_reason': 'd', + 'confidence': 'high', + } + self.assertTrue(any('title' in e for e in afn.validate_envelope(envelope))) + def test_also_capped_at_two_entries(self): base = dict(FIXTURE_ENVELOPE) base['also'] = [dict(FIXTURE_ENVELOPE) for _ in range(3)] @@ -667,6 +722,64 @@ def test_existing_labels_requests_the_name_field(self): self.assertEqual(args[args.index('--json') + 1], 'name') +class CandidatePoolTests(unittest.TestCase): + """The issue the notifier commented on stays in the candidate pool. + + Found by dogfooding. The origin issue was excluded unconditionally. When + the notifier had commented on a pre-existing issue -- 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. It answered "new", + and a duplicate issue was opened: the exact outcome this path exists to + prevent. + """ + + def _run_main(self, *, origin_kind: str, candidates: list[afn.CandidateIssue]) -> str: + captured: dict[str, str] = {} + + def fake_build_prompt( + workflow_name: str, run_url: str, signature: Any, candidates_block: str + ) -> tuple[str, str]: + captured['block'] = candidates_block + return 'sys', 'user' + + env = { + 'REPO': 'canonical/operator', + 'RUN_ID': '28141163589', + 'WORKFLOW_NAME': 'Broad Charm Compatibility Tests', + 'RUN_URL': 'https://example.invalid/run', + 'OPENROUTER_API_KEY': 'test-key', + } + with ( + mock.patch.dict(os.environ, env, clear=True), + mock.patch.object(afn, 'locate_run_markers', return_value=(None, origin_kind, 9010)), + mock.patch.object(afn, 'fetch_failed_jobs', return_value=[]), + mock.patch.object(afn, 'fetch_run_meta', return_value={'createdAt': ''}), + mock.patch.object(afn, 'search_candidates', return_value=(candidates, [])), + mock.patch.object(afn, 'existing_labels', return_value=set()), + mock.patch.object(afn, 'build_prompt', side_effect=fake_build_prompt), + mock.patch.object(afn, 'call_openrouter', return_value={'action': 'bogus'}), + mock.patch.object(afn, 'gh'), + mock.patch.object(afn, 'write_step_summary'), + mock.patch.object(afn, 'set_output'), + ): + afn.main() + return captured['block'] + + def test_commented_origin_issue_is_offered_as_a_candidate(self): + candidate = afn.CandidateIssue( + number=9010, title='the tracked one', body='x', closed_at=None + ) + block = self._run_main(origin_kind='comment', candidates=[candidate]) + self.assertIn('#9010', block) + + def test_freshly_created_placeholder_is_not_offered_as_a_candidate(self): + candidate = afn.CandidateIssue( + number=9010, title='the placeholder', body='x', closed_at=None + ) + block = self._run_main(origin_kind='new', candidates=[candidate]) + self.assertNotIn('#9010', block) + + class BodyFooterTests(unittest.TestCase): """Every body carries the footer the notifier's coarse search matches on. From 2a4ea104f8fb40aa8c3d236b0afa7738e254558c Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Sat, 25 Jul 2026 13:40:28 +1200 Subject: [PATCH 05/37] fix: ignore fields that do not apply, rather than rejecting the response 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. --- .github/ai_failure_notifier.py | 50 ++++++++++++++++++++++ test/test_ai_failure_notifier.py | 72 ++++++++++++++++++++++++++++++++ 2 files changed, 122 insertions(+) diff --git a/.github/ai_failure_notifier.py b/.github/ai_failure_notifier.py index d76098bf9..282c87326 100644 --- a/.github/ai_failure_notifier.py +++ b/.github/ai_failure_notifier.py @@ -734,6 +734,48 @@ def validate_entry(entry: Any, *, path: str, allow_also: bool = False) -> list[s return errors +# Fields that only mean something for one of the two actions. The model is +# given a `strict` schema, so it tends to return every declared property and +# fill in the ones that do not apply to the action it chose. Those are dropped +# rather than treated as an error: we would not act on them either way, and +# rejecting the envelope threw away a usable body and fell back to the plain +# notice. +_ACTION_ONLY_FIELDS = { + 'comment': ('title', 'labels', 'issue_type'), + 'new': ('target_issue',), +} + + +def drop_inapplicable_fields(entry: Any) -> tuple[Any, list[str]]: + """Strip fields that do not apply to `entry`'s action; report what went.""" + if not isinstance(entry, dict): + return entry, [] + fields = _ACTION_ONLY_FIELDS.get(entry.get('action')) + if not fields: + return entry, [] + dropped = [f for f in fields if f in entry] + if not dropped: + return entry, [] + return {k: v for k, v in entry.items() if k not in dropped}, dropped + + +def normalise_envelope(envelope: Any) -> tuple[Any, list[str]]: + """Drop inapplicable fields from the envelope and each `also` entry.""" + if not isinstance(envelope, dict): + return envelope, [] + cleaned, dropped = drop_inapplicable_fields(envelope) + notes = [f'envelope: {f}' for f in dropped] + also = cleaned.get('also') + if isinstance(also, list): + entries: list[Any] = [] + for i, entry in enumerate(also): + entry, entry_dropped = drop_inapplicable_fields(entry) + notes += [f'envelope.also[{i}]: {f}' for f in entry_dropped] + entries.append(entry) + cleaned = {**cleaned, 'also': entries} + return cleaned, notes + + def validate_envelope(envelope: Any) -> list[str]: """Validate a top-level envelope (may carry `also`). @@ -1233,6 +1275,14 @@ def main() -> int: set_output('handled', 'true') return 0 + envelope, dropped_fields = normalise_envelope(envelope) + if dropped_fields: + write_step_summary( + 'Ignored fields that do not apply to the chosen action: ' + + ', '.join(dropped_fields) + + '.' + ) + errors = validate_envelope(envelope) if errors: write_step_summary( diff --git a/test/test_ai_failure_notifier.py b/test/test_ai_failure_notifier.py index 548dc39bc..43e837817 100644 --- a/test/test_ai_failure_notifier.py +++ b/test/test_ai_failure_notifier.py @@ -722,6 +722,78 @@ def test_existing_labels_requests_the_name_field(self): self.assertEqual(args[args.index('--json') + 1], 'name') +class NormalisationTests(unittest.TestCase): + """Fields that do not apply to the chosen action are dropped, not fatal. + + Found by dogfooding: the model is given a `strict` schema, so it returns + every declared property and fills the ones irrelevant to the action it + chose. Treating those as validation errors discarded a perfectly good + comment body and fell back to the plain notice. + """ + + def test_comment_loses_new_only_fields(self): + envelope: dict[str, Any] = { + 'action': 'comment', + 'target_issue': 9010, + 'body': 'Another occurrence.', + 'title': 'a title it should not have', + 'labels': ['tests'], + 'issue_type': 'bug', + 'dedup_reason': 'd', + 'confidence': 'high', + } + cleaned, dropped = afn.normalise_envelope(envelope) + self.assertEqual( + sorted(dropped), ['envelope: issue_type', 'envelope: labels', 'envelope: title'] + ) + self.assertNotIn('title', cleaned) + self.assertEqual(cleaned['body'], 'Another occurrence.') + self.assertEqual(afn.validate_envelope(cleaned), []) + + def test_new_loses_target_issue(self): + envelope: dict[str, Any] = { + 'action': 'new', + 'title': 't', + 'body': 'b', + 'labels': [], + 'issue_type': None, + 'target_issue': 7, + 'dedup_reason': 'd', + 'confidence': 'low', + } + cleaned, dropped = afn.normalise_envelope(envelope) + self.assertEqual(dropped, ['envelope: target_issue']) + self.assertEqual(afn.validate_envelope(cleaned), []) + + def test_also_entries_are_normalised_too(self): + inner: dict[str, Any] = { + 'action': 'comment', + 'target_issue': 1, + 'body': 'b', + 'title': 'nope', + 'dedup_reason': 'd', + 'confidence': 'low', + } + envelope: dict[str, Any] = { + 'action': 'new', + 'title': 't', + 'body': 'b', + 'labels': [], + 'issue_type': None, + 'dedup_reason': 'd', + 'confidence': 'low', + 'also': [inner], + } + cleaned, dropped = afn.normalise_envelope(envelope) + self.assertEqual(dropped, ['envelope.also[0]: title']) + self.assertEqual(afn.validate_envelope(cleaned), []) + + def test_nothing_dropped_leaves_the_envelope_alone(self): + cleaned, dropped = afn.normalise_envelope(FIXTURE_ENVELOPE) + self.assertEqual(dropped, []) + self.assertIs(cleaned, FIXTURE_ENVELOPE) + + class CandidatePoolTests(unittest.TestCase): """The issue the notifier commented on stays in the candidate pool. From 82f2df57e92a5d0dcba5fcb3e391a98b4ba71518 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Sat, 25 Jul 2026 14:10:04 +1200 Subject: [PATCH 06/37] fix: find the notifier's marker without waiting on the search index `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. --- .github/ai_failure_notifier.py | 56 ++++++++- test/test_ai_failure_notifier.py | 187 +++++++++++++++++++++++++------ 2 files changed, 206 insertions(+), 37 deletions(-) diff --git a/.github/ai_failure_notifier.py b/.github/ai_failure_notifier.py index 282c87326..17148a9ed 100644 --- a/.github/ai_failure_notifier.py +++ b/.github/ai_failure_notifier.py @@ -57,6 +57,10 @@ DEFAULT_MODEL = 'deepseek/deepseek-chat' # DeepSeek V3 on OpenRouter. CLOSED_CANDIDATE_WINDOW_DAYS = 14 MAX_CANDIDATES = 3 +# 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 # Colour escapes, which Actions logs are full of. Two alternatives, because # the logs contain both the real thing and a mangled form where the ESC byte @@ -951,8 +955,58 @@ def fetch_issue_texts(repo: str, number: int) -> list[str]: return texts +def recent_issue_texts(repo: str, limit: int = RECENT_ISSUE_SCAN) -> list[tuple[int, str]]: + """Return (number, text) pairs for the `limit` most recently updated issues. + + Bodies and comment bodies both, since a notifier marker with + `origin=comment` lives in a comment rather than the body. + """ + data = ( + gh_json( + 'issue', + 'list', + '--repo', + repo, + '--state', + 'all', + '--limit', + str(limit), + '--json', + 'number,body,comments', + ) + or [] + ) + texts: list[tuple[int, str]] = [] + for issue in data: + number = issue['number'] + texts.append((number, issue.get('body') or '')) + for comment in issue.get('comments') or []: + texts.append((number, comment.get('body') or '')) + return texts + + def locate_run_markers(repo: str, run_id: str) -> tuple[int | None, str | None, int | None]: - """Search `repo` for markers belonging to `run_id` and classify them.""" + """Find the markers belonging to `run_id` and classify them. + + Scans the most recently updated issues first, and only falls back to + `gh search issues` if that finds nothing. + + The ordering matters, and is the whole point of doing it this way. The + notifier stamps its marker moments before this workflow runs, and GitHub's + issue *search* index is not read-your-writes -- a marker that has not been + indexed yet reads as "no notifier marker found", and main() responds by + opening a *second* issue for a run that already has one. The issue *list* + endpoint has no such lag, and the artefact the notifier just touched is by + construction among the most recently updated issues in the repo. + + Search remains as a fallback for the one case the list cannot cover: a repo + busy enough that more than `RECENT_ISSUE_SCAN` issues were updated in + between, where a stale index still beats no lookup at all. + """ + markers = find_run_markers(recent_issue_texts(repo), run_id) + if markers != (None, None, None): + return markers + hits = search_issue_numbers(repo, f'{MARKER_PREFIX}:run={run_id}') texts: list[tuple[int, str]] = [] for number in hits: diff --git a/test/test_ai_failure_notifier.py b/test/test_ai_failure_notifier.py index 43e837817..dc5bd38b1 100644 --- a/test/test_ai_failure_notifier.py +++ b/test/test_ai_failure_notifier.py @@ -204,13 +204,15 @@ def test_strip_line_removes_timestamp_and_ansi(self): ) def test_parse_job_log_pytest_failures_and_summary_bound(self): - log = '\n'.join([ - '2026-06-25T01:40:15.0000000Z ============ short test summary info ============', - '2026-06-25T01:40:15.0000000Z FAILED tests/unit/test_x.py::test_a - AssertionError: x', - '2026-06-25T01:40:15.0000000Z ERROR tests/unit/test_x.py::test_b - PendingDeprecat...', - '2026-06-25T01:40:15.0000000Z ============ 2 failed in 1.23s ============', - '2026-06-25T01:40:15.0000000Z some trailing noise, not part of the summary', - ]) + log = '\n'.join( + [ + '2026-06-25T01:40:15.0000000Z ============ short test summary info ============', + '2026-06-25T01:40:15.0000000Z FAILED tests/unit/test_x.py::test_a - AssertionError: x', + '2026-06-25T01:40:15.0000000Z ERROR tests/unit/test_x.py::test_b - PendingDeprecat...', + '2026-06-25T01:40:15.0000000Z ============ 2 failed in 1.23s ============', + '2026-06-25T01:40:15.0000000Z some trailing noise, not part of the summary', + ] + ) pytest_failures, go_failures, _tb, _tail = afn.parse_job_log(log) self.assertEqual( pytest_failures, @@ -227,21 +229,25 @@ def test_parse_job_log_go_failures(self): self.assertEqual(go_failures, ['TestFoo', 'TestBar']) def test_parse_job_log_traceback_top_error_prefers_last_match(self): - log = '\n'.join([ - 'ValueError: first, ignored', - 'some other output', - 'AttributeError: the real one', - ]) + log = '\n'.join( + [ + 'ValueError: first, ignored', + 'some other output', + 'AttributeError: the real one', + ] + ) _, _, tb, _ = afn.parse_job_log(log) self.assertEqual(tb, 'AttributeError: the real one') def test_parse_job_log_tail_excerpt_stops_before_first_error_marker(self): - log = '\n'.join([ - 'line before 1', - 'line before 2', - '##[error]something broke', - 'line after (should not appear in tail)', - ]) + log = '\n'.join( + [ + 'line before 1', + 'line before 2', + '##[error]something broke', + 'line after (should not appear in tail)', + ] + ) _, _, _, tail = afn.parse_job_log(log) self.assertEqual(tail, ['line before 1', 'line before 2']) @@ -315,12 +321,14 @@ def test_empty_candidates_block(self): def test_recently_closed_candidate_is_labelled_and_capped_at_medium(self): now = datetime.datetime(2026, 6, 25, tzinfo=datetime.timezone.utc) closed = [ - afn.CandidateIssue.from_gh({ - 'number': 42, - 'title': 'old thing', - 'body': 'x', - 'closedAt': '2026-06-20T00:00:00Z', - }) + afn.CandidateIssue.from_gh( + { + 'number': 42, + 'title': 'old thing', + 'body': 'x', + 'closedAt': '2026-06-20T00:00:00Z', + } + ) ] block = afn.build_candidates_block([], closed, now) self.assertIn('#42', block) @@ -330,24 +338,28 @@ def test_recently_closed_candidate_is_labelled_and_capped_at_medium(self): def test_closed_candidate_outside_window_is_dropped(self): now = datetime.datetime(2026, 6, 25, tzinfo=datetime.timezone.utc) closed = [ - afn.CandidateIssue.from_gh({ - 'number': 42, - 'title': 'ancient', - 'body': 'x', - 'closedAt': '2026-01-01T00:00:00Z', - }) + afn.CandidateIssue.from_gh( + { + 'number': 42, + 'title': 'ancient', + 'body': 'x', + 'closedAt': '2026-01-01T00:00:00Z', + } + ) ] block = afn.build_candidates_block([], closed, now) self.assertEqual(block, '(no open issues found for this workflow)') def test_candidates_capped_at_three(self): opens = [ - afn.CandidateIssue.from_gh({ - 'number': n, - 'title': f'issue {n}', - 'body': 'x', - 'closedAt': None, - }) + afn.CandidateIssue.from_gh( + { + 'number': n, + 'title': f'issue {n}', + 'body': 'x', + 'closedAt': None, + } + ) for n in range(5) ] block = afn.build_candidates_block(opens, [], datetime.datetime.now(datetime.timezone.utc)) @@ -722,6 +734,109 @@ def test_existing_labels_requests_the_name_field(self): self.assertEqual(args[args.index('--json') + 1], 'name') +class MarkerLookupConsistencyTests(unittest.TestCase): + """The notifier's marker must be found without depending on the search index. + + `gh search issues` is not read-your-writes: the notifier stamps its marker + seconds before the enricher runs, and an unindexed marker reads as "no + notifier marker found", which makes main() open a *second* issue for a run + that already has one. The issue list endpoint has no such lag, so it is + consulted first and search is only a fallback. + """ + + def _gh(self, responses: list[str]) -> mock.Mock: + return mock.Mock( + side_effect=[mock.Mock(returncode=0, stdout=out, stderr='') for out in responses] + ) + + def test_marker_in_a_body_is_found_without_any_search_call(self): + listing = json.dumps( + [ + {'number': 2700, 'body': 'unrelated', 'comments': []}, + { + 'number': 2658, + 'body': 'placeholder\n\n', + 'comments': [], + }, + ] + ) + gh_calls = self._gh([listing]) + with mock.patch.object(afn, 'gh', side_effect=gh_calls): + enriched, kind, number = afn.locate_run_markers('canonical/operator', '999') + self.assertEqual((enriched, kind, number), (None, 'new', 2658)) + # Exactly one call, and it is the list endpoint -- not search. + self.assertEqual(gh_calls.call_count, 1) + args = gh_calls.call_args.args + self.assertEqual(args[:2], ('issue', 'list')) + self.assertEqual(args[args.index('--repo') + 1], 'canonical/operator') + self.assertEqual(args[args.index('--state') + 1], 'all') + self.assertEqual(args[args.index('--json') + 1], 'number,body,comments') + + def test_marker_in_a_comment_is_found_too(self): + comment = 'failed again\n\n' + listing = json.dumps( + [ + { + 'number': 2601, + 'body': 'an older failure thread', + 'comments': [{'body': comment}], + } + ] + ) + with mock.patch.object(afn, 'gh', side_effect=self._gh([listing])): + enriched, kind, number = afn.locate_run_markers('canonical/operator', '999') + self.assertEqual((enriched, kind, number), (None, 'comment', 2601)) + + def test_search_is_a_fallback_when_the_listing_misses(self): + listing = json.dumps([{'number': 2700, 'body': 'unrelated', 'comments': []}]) + search = json.dumps([{'number': 2658}]) + view = json.dumps( + { + 'body': 'placeholder\n\n', + 'comments': [], + } + ) + gh_calls = self._gh([listing, search, view]) + with mock.patch.object(afn, 'gh', side_effect=gh_calls): + enriched, kind, number = afn.locate_run_markers('canonical/operator', '999') + self.assertEqual((enriched, kind, number), (None, 'new', 2658)) + self.assertEqual(gh_calls.call_args_list[1].args[:2], ('search', 'issues')) + + def test_an_unindexed_marker_still_resolves(self): + """The regression this change exists for. + + Search returns nothing (the marker is not indexed yet) but the issue is + right there in the listing. Before the fix this returned all-None and + main() opened a duplicate issue. + """ + listing = json.dumps( + [ + { + 'number': 2658, + 'body': 'placeholder\n\n', + 'comments': [], + } + ] + ) + with mock.patch.object(afn, 'gh', side_effect=self._gh([listing, '[]'])): + enriched, kind, number = afn.locate_run_markers('canonical/operator', '999') + self.assertEqual((enriched, kind, number), (None, 'new', 2658)) + + def test_rung_zero_sig_marker_is_found_in_the_listing(self): + listing = json.dumps( + [ + { + 'number': 2658, + 'body': 'enriched body\n\n', + 'comments': [], + } + ] + ) + with mock.patch.object(afn, 'gh', side_effect=self._gh([listing])): + enriched, _, _ = afn.locate_run_markers('canonical/operator', '999') + self.assertEqual(enriched, 2658) + + class NormalisationTests(unittest.TestCase): """Fields that do not apply to the chosen action are dropped, not fatal. From e16680b6130b2205579bd59b2ad131ae24a81ee2 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Sat, 25 Jul 2026 17:47:56 +1200 Subject: [PATCH 07/37] test: keep the notifier test fixtures under the 99-column limit Co-Authored-By: Claude Opus 5 (1M context) --- test/test_ai_failure_notifier.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/test/test_ai_failure_notifier.py b/test/test_ai_failure_notifier.py index dc5bd38b1..fad39513e 100644 --- a/test/test_ai_failure_notifier.py +++ b/test/test_ai_failure_notifier.py @@ -204,13 +204,14 @@ def test_strip_line_removes_timestamp_and_ansi(self): ) def test_parse_job_log_pytest_failures_and_summary_bound(self): + ts = '2026-06-25T01:40:15.0000000Z ' log = '\n'.join( [ - '2026-06-25T01:40:15.0000000Z ============ short test summary info ============', - '2026-06-25T01:40:15.0000000Z FAILED tests/unit/test_x.py::test_a - AssertionError: x', - '2026-06-25T01:40:15.0000000Z ERROR tests/unit/test_x.py::test_b - PendingDeprecat...', - '2026-06-25T01:40:15.0000000Z ============ 2 failed in 1.23s ============', - '2026-06-25T01:40:15.0000000Z some trailing noise, not part of the summary', + f'{ts}============ short test summary info ============', + f'{ts}FAILED tests/unit/test_x.py::test_a - AssertionError: x', + f'{ts}ERROR tests/unit/test_x.py::test_b - PendingDeprecat...', + f'{ts}============ 2 failed in 1.23s ============', + f'{ts}some trailing noise, not part of the summary', ] ) pytest_failures, go_failures, _tb, _tail = afn.parse_job_log(log) @@ -827,7 +828,7 @@ def test_rung_zero_sig_marker_is_found_in_the_listing(self): [ { 'number': 2658, - 'body': 'enriched body\n\n', + 'body': 'enriched\n\n', 'comments': [], } ] From 239f8fc59d893bf99ff0e10114171f401affb56c Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Sat, 25 Jul 2026 17:50:02 +1200 Subject: [PATCH 08/37] test: apply ruff --preview formatting to the notifier tests Co-Authored-By: Claude Opus 5 (1M context) --- test/test_ai_failure_notifier.py | 160 +++++++++++++------------------ 1 file changed, 69 insertions(+), 91 deletions(-) diff --git a/test/test_ai_failure_notifier.py b/test/test_ai_failure_notifier.py index fad39513e..3c25ba98d 100644 --- a/test/test_ai_failure_notifier.py +++ b/test/test_ai_failure_notifier.py @@ -205,15 +205,13 @@ def test_strip_line_removes_timestamp_and_ansi(self): def test_parse_job_log_pytest_failures_and_summary_bound(self): ts = '2026-06-25T01:40:15.0000000Z ' - log = '\n'.join( - [ - f'{ts}============ short test summary info ============', - f'{ts}FAILED tests/unit/test_x.py::test_a - AssertionError: x', - f'{ts}ERROR tests/unit/test_x.py::test_b - PendingDeprecat...', - f'{ts}============ 2 failed in 1.23s ============', - f'{ts}some trailing noise, not part of the summary', - ] - ) + log = '\n'.join([ + f'{ts}============ short test summary info ============', + f'{ts}FAILED tests/unit/test_x.py::test_a - AssertionError: x', + f'{ts}ERROR tests/unit/test_x.py::test_b - PendingDeprecat...', + f'{ts}============ 2 failed in 1.23s ============', + f'{ts}some trailing noise, not part of the summary', + ]) pytest_failures, go_failures, _tb, _tail = afn.parse_job_log(log) self.assertEqual( pytest_failures, @@ -230,25 +228,21 @@ def test_parse_job_log_go_failures(self): self.assertEqual(go_failures, ['TestFoo', 'TestBar']) def test_parse_job_log_traceback_top_error_prefers_last_match(self): - log = '\n'.join( - [ - 'ValueError: first, ignored', - 'some other output', - 'AttributeError: the real one', - ] - ) + log = '\n'.join([ + 'ValueError: first, ignored', + 'some other output', + 'AttributeError: the real one', + ]) _, _, tb, _ = afn.parse_job_log(log) self.assertEqual(tb, 'AttributeError: the real one') def test_parse_job_log_tail_excerpt_stops_before_first_error_marker(self): - log = '\n'.join( - [ - 'line before 1', - 'line before 2', - '##[error]something broke', - 'line after (should not appear in tail)', - ] - ) + log = '\n'.join([ + 'line before 1', + 'line before 2', + '##[error]something broke', + 'line after (should not appear in tail)', + ]) _, _, _, tail = afn.parse_job_log(log) self.assertEqual(tail, ['line before 1', 'line before 2']) @@ -322,14 +316,12 @@ def test_empty_candidates_block(self): def test_recently_closed_candidate_is_labelled_and_capped_at_medium(self): now = datetime.datetime(2026, 6, 25, tzinfo=datetime.timezone.utc) closed = [ - afn.CandidateIssue.from_gh( - { - 'number': 42, - 'title': 'old thing', - 'body': 'x', - 'closedAt': '2026-06-20T00:00:00Z', - } - ) + afn.CandidateIssue.from_gh({ + 'number': 42, + 'title': 'old thing', + 'body': 'x', + 'closedAt': '2026-06-20T00:00:00Z', + }) ] block = afn.build_candidates_block([], closed, now) self.assertIn('#42', block) @@ -339,28 +331,24 @@ def test_recently_closed_candidate_is_labelled_and_capped_at_medium(self): def test_closed_candidate_outside_window_is_dropped(self): now = datetime.datetime(2026, 6, 25, tzinfo=datetime.timezone.utc) closed = [ - afn.CandidateIssue.from_gh( - { - 'number': 42, - 'title': 'ancient', - 'body': 'x', - 'closedAt': '2026-01-01T00:00:00Z', - } - ) + afn.CandidateIssue.from_gh({ + 'number': 42, + 'title': 'ancient', + 'body': 'x', + 'closedAt': '2026-01-01T00:00:00Z', + }) ] block = afn.build_candidates_block([], closed, now) self.assertEqual(block, '(no open issues found for this workflow)') def test_candidates_capped_at_three(self): opens = [ - afn.CandidateIssue.from_gh( - { - 'number': n, - 'title': f'issue {n}', - 'body': 'x', - 'closedAt': None, - } - ) + afn.CandidateIssue.from_gh({ + 'number': n, + 'title': f'issue {n}', + 'body': 'x', + 'closedAt': None, + }) for n in range(5) ] block = afn.build_candidates_block(opens, [], datetime.datetime.now(datetime.timezone.utc)) @@ -751,16 +739,14 @@ def _gh(self, responses: list[str]) -> mock.Mock: ) def test_marker_in_a_body_is_found_without_any_search_call(self): - listing = json.dumps( - [ - {'number': 2700, 'body': 'unrelated', 'comments': []}, - { - 'number': 2658, - 'body': 'placeholder\n\n', - 'comments': [], - }, - ] - ) + listing = json.dumps([ + {'number': 2700, 'body': 'unrelated', 'comments': []}, + { + 'number': 2658, + 'body': 'placeholder\n\n', + 'comments': [], + }, + ]) gh_calls = self._gh([listing]) with mock.patch.object(afn, 'gh', side_effect=gh_calls): enriched, kind, number = afn.locate_run_markers('canonical/operator', '999') @@ -775,15 +761,13 @@ def test_marker_in_a_body_is_found_without_any_search_call(self): def test_marker_in_a_comment_is_found_too(self): comment = 'failed again\n\n' - listing = json.dumps( - [ - { - 'number': 2601, - 'body': 'an older failure thread', - 'comments': [{'body': comment}], - } - ] - ) + listing = json.dumps([ + { + 'number': 2601, + 'body': 'an older failure thread', + 'comments': [{'body': comment}], + } + ]) with mock.patch.object(afn, 'gh', side_effect=self._gh([listing])): enriched, kind, number = afn.locate_run_markers('canonical/operator', '999') self.assertEqual((enriched, kind, number), (None, 'comment', 2601)) @@ -791,12 +775,10 @@ def test_marker_in_a_comment_is_found_too(self): def test_search_is_a_fallback_when_the_listing_misses(self): listing = json.dumps([{'number': 2700, 'body': 'unrelated', 'comments': []}]) search = json.dumps([{'number': 2658}]) - view = json.dumps( - { - 'body': 'placeholder\n\n', - 'comments': [], - } - ) + view = json.dumps({ + 'body': 'placeholder\n\n', + 'comments': [], + }) gh_calls = self._gh([listing, search, view]) with mock.patch.object(afn, 'gh', side_effect=gh_calls): enriched, kind, number = afn.locate_run_markers('canonical/operator', '999') @@ -810,29 +792,25 @@ def test_an_unindexed_marker_still_resolves(self): right there in the listing. Before the fix this returned all-None and main() opened a duplicate issue. """ - listing = json.dumps( - [ - { - 'number': 2658, - 'body': 'placeholder\n\n', - 'comments': [], - } - ] - ) + listing = json.dumps([ + { + 'number': 2658, + 'body': 'placeholder\n\n', + 'comments': [], + } + ]) with mock.patch.object(afn, 'gh', side_effect=self._gh([listing, '[]'])): enriched, kind, number = afn.locate_run_markers('canonical/operator', '999') self.assertEqual((enriched, kind, number), (None, 'new', 2658)) def test_rung_zero_sig_marker_is_found_in_the_listing(self): - listing = json.dumps( - [ - { - 'number': 2658, - 'body': 'enriched\n\n', - 'comments': [], - } - ] - ) + listing = json.dumps([ + { + 'number': 2658, + 'body': 'enriched\n\n', + 'comments': [], + } + ]) with mock.patch.object(afn, 'gh', side_effect=self._gh([listing])): enriched, _, _ = afn.locate_run_markers('canonical/operator', '999') self.assertEqual(enriched, 2658) From 4d958b7f037f99e46d1d9f9a789fc3a7a1661e5d Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Mon, 27 Jul 2026 11:22:55 +1200 Subject: [PATCH 09/37] fix: stop the workflow-level fallback opening a duplicate issue `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=:` 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) --- .github/workflows/ai-failure-enrich.yaml | 46 ++++++++++++++++++++---- 1 file changed, 39 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ai-failure-enrich.yaml b/.github/workflows/ai-failure-enrich.yaml index 5cb74a6c4..9f740fb3e 100644 --- a/.github/workflows/ai-failure-enrich.yaml +++ b/.github/workflows/ai-failure-enrich.yaml @@ -90,12 +90,19 @@ jobs: # `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 the simplest thing that - # can work: one `gh issue create`, no script, no secrets. + # 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. plain-fallback: needs: [enrich] if: >- @@ -106,14 +113,39 @@ jobs: permissions: issues: write steps: - - name: Create a plain issue (fallback) + - name: Comment on this run's issue, or create one (fallback) env: GH_TOKEN: ${{ github.token }} WORKFLOW_NAME: ${{ github.event.workflow_run.name }} REPO: ${{ github.event.workflow_run.repository.full_name }} + RUN_ID: ${{ github.event.workflow_run.id }} RUN_URL: ${{ github.event.workflow_run.html_url }} run: | - gh issue create \ - --repo "$REPO" \ - --title "Scheduled workflow '$WORKFLOW_NAME' failed" \ - --body "Scheduled workflow '$WORKFLOW_NAME' failed: $RUN_URL" + set -euo pipefail + + # The notifier runs inside the triggering run, so the run id it + # stamped is the one we are handed here. The trailing colon matches + # both the notifier's `:origin=` and the enricher's `:sig=`, so this + # also covers `enrich` having applied its result and then died before + # setting `handled`. + # + # Listing rather than `gh search issues` is the same read-your-writes + # point made at locate_run_markers() in ai_failure_notifier.py: the + # marker is minutes old and may not be indexed, and reading a stale + # index as "no issue exists" is precisely the duplicate this avoids. + # An issue the notifier just touched is by construction among the + # most recently updated ones. + marker="" gh issue comment "$match" --repo "$REPO" --body "$comment_body" + echo "issue=$match" >> "$GITHUB_OUTPUT" + echo "origin=comment" >> "$GITHUB_OUTPUT" else issue_body="Scheduled workflow '$WORKFLOW_NAME' failed: $RUN_URL"$'\n\n'"${marker}new -->" - gh issue create --repo "$REPO" \ + # `gh issue create` prints the new issue's URL; the number is its + # last path segment. + issue_url=$(gh issue create --repo "$REPO" \ --title "Scheduled workflow '$WORKFLOW_NAME' failed" \ - --body "$issue_body" + --body "$issue_body") + echo "issue=${issue_url##*/}" >> "$GITHUB_OUTPUT" + echo "origin=new" >> "$GITHUB_OUTPUT" fi enrich: @@ -73,4 +88,9 @@ jobs: # enrichment degrades to a plain notice with nothing failing loudly. secrets: OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} + # Empty when `notify` failed before it opened anything -- this job still + # runs (`always()`), and the script falls back to looking the issue up. + with: + issue: ${{ needs.notify.outputs.issue }} + origin: ${{ needs.notify.outputs.origin }} uses: ./.github/workflows/ai-failure-enrich.yaml From 351192bed6210c75a715daf829d212684dfb6ff3 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 26 Aug 2026 12:39:29 +1200 Subject: [PATCH 27/37] ci: don't enrich when the notification itself failed 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. --- .github/workflows/notify-scheduled-failure.yaml | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/.github/workflows/notify-scheduled-failure.yaml b/.github/workflows/notify-scheduled-failure.yaml index 7ee030812..532ad9b00 100644 --- a/.github/workflows/notify-scheduled-failure.yaml +++ b/.github/workflows/notify-scheduled-failure.yaml @@ -73,7 +73,6 @@ jobs: enrich: needs: [notify] - if: ${{ always() }} permissions: issues: write # A called workflow cannot hold a scope its caller did not grant, so @@ -88,8 +87,10 @@ jobs: # enrichment degrades to a plain notice with nothing failing loudly. secrets: OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} - # Empty when `notify` failed before it opened anything -- this job still - # runs (`always()`), and the script falls back to looking the issue up. + # `notify` has to have succeeded to get here, so these are always set for + # this repo. The script keeps a lookup fallback for the empty case anyway: + # other repositories adopt this at their own pace, and a caller that has + # not been migrated yet passes nothing. with: issue: ${{ needs.notify.outputs.issue }} origin: ${{ needs.notify.outputs.origin }} From ae5f5b014e5ac447b73ee65352143993be748689 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 26 Aug 2026 12:41:09 +1200 Subject: [PATCH 28/37] ci: drop the comments restating GitHub's permission model 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. --- .github/workflows/ai-failure-enrich.yaml | 6 ++---- .github/workflows/notify-scheduled-failure.yaml | 3 --- 2 files changed, 2 insertions(+), 7 deletions(-) diff --git a/.github/workflows/ai-failure-enrich.yaml b/.github/workflows/ai-failure-enrich.yaml index d614af400..950786a0f 100644 --- a/.github/workflows/ai-failure-enrich.yaml +++ b/.github/workflows/ai-failure-enrich.yaml @@ -29,10 +29,8 @@ jobs: runs-on: ubuntu-latest environment: ai-failure-triage # Required to access secrets.OPENROUTER_API_KEY permissions: - issues: write # Write to the workflow failure issue. - # Reading the failing run's job logs is an `actions: read` call, and a - # `permissions:` block sets every scope it does not name to `none`. - actions: read # Read the failing workflow's logs. + issues: write + actions: read steps: - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7.0.0 with: diff --git a/.github/workflows/notify-scheduled-failure.yaml b/.github/workflows/notify-scheduled-failure.yaml index 532ad9b00..68513bec6 100644 --- a/.github/workflows/notify-scheduled-failure.yaml +++ b/.github/workflows/notify-scheduled-failure.yaml @@ -75,9 +75,6 @@ jobs: needs: [notify] permissions: issues: write - # 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 # This passes an empty string, and is still required. Nothing in this # chain can read the OpenRouter key: it lives on stage 2's From 203e440aa817bbb51ae5b60b41b403f26025d56e Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 26 Aug 2026 12:42:33 +1200 Subject: [PATCH 29/37] ci: document the secret mechanics once, where the environment is 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. --- .github/workflows/notify-scheduled-failure.yaml | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/.github/workflows/notify-scheduled-failure.yaml b/.github/workflows/notify-scheduled-failure.yaml index 68513bec6..9bdfe453c 100644 --- a/.github/workflows/notify-scheduled-failure.yaml +++ b/.github/workflows/notify-scheduled-failure.yaml @@ -76,12 +76,8 @@ jobs: permissions: issues: write 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. + # Passes an empty string, and is still required -- see the note on + # OPENROUTER_API_KEY in ai-failure-enrich.yaml. secrets: OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} # `notify` has to have succeeded to get here, so these are always set for From b7fb74b4835036bb35e380030034092d69758312 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 26 Aug 2026 12:45:00 +1200 Subject: [PATCH 30/37] ci: trim the caller boilerplate to the part a caller needs 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. --- .github/workflows/example-charm-charmcraft-test.yaml | 11 ++--------- .../workflows/example-charm-integration-tests.yaml | 11 ++--------- .github/workflows/integration.yaml | 11 ++--------- .github/workflows/smoke.yaml | 11 ++--------- .github/workflows/tiobe.yaml | 11 ++--------- .github/workflows/update-best-practice-doc.yaml | 11 ++--------- .github/workflows/update-charm-tests.yaml | 11 ++--------- 7 files changed, 14 insertions(+), 63 deletions(-) diff --git a/.github/workflows/example-charm-charmcraft-test.yaml b/.github/workflows/example-charm-charmcraft-test.yaml index a8ef644c0..8183d5c9c 100644 --- a/.github/workflows/example-charm-charmcraft-test.yaml +++ b/.github/workflows/example-charm-charmcraft-test.yaml @@ -71,15 +71,8 @@ jobs: needs: [integration] permissions: issues: write - # 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 - # This passes an empty string, and is still required. The OpenRouter key - # lives on stage 2's `ai-failure-triage` environment, which no job here - # can read, but unless each call site names the secret, stage 2's own - # environment does not fill it in either and enrichment quietly degrades - # to a plain notice. + actions: read # for ai-failure-enrich.yaml secrets: + # For ai-failure-enrich.yaml -- can only be read in the ai-failure-triage environment. OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} uses: ./.github/workflows/notify-scheduled-failure.yaml diff --git a/.github/workflows/example-charm-integration-tests.yaml b/.github/workflows/example-charm-integration-tests.yaml index 9cf14e54a..6c50d1307 100644 --- a/.github/workflows/example-charm-integration-tests.yaml +++ b/.github/workflows/example-charm-integration-tests.yaml @@ -84,15 +84,8 @@ jobs: needs: [machine-examples-integration, k8s-examples-integration] permissions: issues: write - # 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 - # This passes an empty string, and is still required. The OpenRouter key - # lives on stage 2's `ai-failure-triage` environment, which no job here - # can read, but unless each call site names the secret, stage 2's own - # environment does not fill it in either and enrichment quietly degrades - # to a plain notice. + actions: read # for ai-failure-enrich.yaml secrets: + # For ai-failure-enrich.yaml -- can only be read in the ai-failure-triage environment. OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} uses: ./.github/workflows/notify-scheduled-failure.yaml diff --git a/.github/workflows/integration.yaml b/.github/workflows/integration.yaml index 1e46f31a0..54aa8c555 100644 --- a/.github/workflows/integration.yaml +++ b/.github/workflows/integration.yaml @@ -73,15 +73,8 @@ jobs: needs: [integration] permissions: issues: write - # 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 - # This passes an empty string, and is still required. The OpenRouter key - # lives on stage 2's `ai-failure-triage` environment, which no job here - # can read, but unless each call site names the secret, stage 2's own - # environment does not fill it in either and enrichment quietly degrades - # to a plain notice. + actions: read # for ai-failure-enrich.yaml secrets: + # For ai-failure-enrich.yaml -- can only be read in the ai-failure-triage environment. OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} uses: ./.github/workflows/notify-scheduled-failure.yaml diff --git a/.github/workflows/smoke.yaml b/.github/workflows/smoke.yaml index c4ccd7e49..63801c8ca 100644 --- a/.github/workflows/smoke.yaml +++ b/.github/workflows/smoke.yaml @@ -50,15 +50,8 @@ jobs: needs: [test] permissions: issues: write - # 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 - # This passes an empty string, and is still required. The OpenRouter key - # lives on stage 2's `ai-failure-triage` environment, which no job here - # can read, but unless each call site names the secret, stage 2's own - # environment does not fill it in either and enrichment quietly degrades - # to a plain notice. + actions: read # for ai-failure-enrich.yaml secrets: + # For ai-failure-enrich.yaml -- can only be read in the ai-failure-triage environment. OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} uses: ./.github/workflows/notify-scheduled-failure.yaml diff --git a/.github/workflows/tiobe.yaml b/.github/workflows/tiobe.yaml index 85b6cf957..d647b99c1 100644 --- a/.github/workflows/tiobe.yaml +++ b/.github/workflows/tiobe.yaml @@ -41,15 +41,8 @@ jobs: needs: [TICS] permissions: issues: write - # 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 - # This passes an empty string, and is still required. The OpenRouter key - # lives on stage 2's `ai-failure-triage` environment, which no job here - # can read, but unless each call site names the secret, stage 2's own - # environment does not fill it in either and enrichment quietly degrades - # to a plain notice. + actions: read # for ai-failure-enrich.yaml secrets: + # For ai-failure-enrich.yaml -- can only be read in the ai-failure-triage environment. OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} uses: ./.github/workflows/notify-scheduled-failure.yaml diff --git a/.github/workflows/update-best-practice-doc.yaml b/.github/workflows/update-best-practice-doc.yaml index 80316d7d2..460536620 100644 --- a/.github/workflows/update-best-practice-doc.yaml +++ b/.github/workflows/update-best-practice-doc.yaml @@ -62,15 +62,8 @@ jobs: needs: [update-docs] permissions: issues: write - # 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 - # This passes an empty string, and is still required. The OpenRouter key - # lives on stage 2's `ai-failure-triage` environment, which no job here - # can read, but unless each call site names the secret, stage 2's own - # environment does not fill it in either and enrichment quietly degrades - # to a plain notice. + actions: read # for ai-failure-enrich.yaml secrets: + # For ai-failure-enrich.yaml -- can only be read in the ai-failure-triage environment. OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} uses: ./.github/workflows/notify-scheduled-failure.yaml diff --git a/.github/workflows/update-charm-tests.yaml b/.github/workflows/update-charm-tests.yaml index 1fbce8077..29b0abd71 100644 --- a/.github/workflows/update-charm-tests.yaml +++ b/.github/workflows/update-charm-tests.yaml @@ -59,15 +59,8 @@ jobs: needs: [update-pins] permissions: issues: write - # 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 - # This passes an empty string, and is still required. The OpenRouter key - # lives on stage 2's `ai-failure-triage` environment, which no job here - # can read, but unless each call site names the secret, stage 2's own - # environment does not fill it in either and enrichment quietly degrades - # to a plain notice. + actions: read # for ai-failure-enrich.yaml secrets: + # For ai-failure-enrich.yaml -- can only be read in the ai-failure-triage environment. OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} uses: ./.github/workflows/notify-scheduled-failure.yaml From 52c63f6a83a05c408d15b7a8b516a8d5b8ef8374 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Wed, 26 Aug 2026 12:46:54 +1200 Subject: [PATCH 31/37] ci: shorten the workflow header to what a caller has to do 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. --- .../workflows/notify-scheduled-failure.yaml | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/.github/workflows/notify-scheduled-failure.yaml b/.github/workflows/notify-scheduled-failure.yaml index 9bdfe453c..27db3ef24 100644 --- a/.github/workflows/notify-scheduled-failure.yaml +++ b/.github/workflows/notify-scheduled-failure.yaml @@ -3,19 +3,13 @@ name: Notify on scheduled failure # Reusable workflow: opens (or comments on) an issue when a scheduled # workflow fails. Callers gate the invocation with # `if: failure() && github.event_name == 'schedule'` and pass -# `permissions: issues: write` plus `actions: read`, and pass -# `OPENROUTER_API_KEY` through. The last two are for stage 2, which this -# workflow calls in turn: a called workflow holds no scope its caller -# withheld, and cannot use a secret no call site named. +# `permissions: issues: write` plus `actions: read`, and grant access to +# `secrets.OPENROUTER_API_KEY`. # -# This is the first of two stages: a cheap, deterministic, always-works -# dedup pre-check, with no LLM dependency and no secrets. The `notify` -# job below is the whole of that guarantee -- once it has run, a -# notification exists. The second stage, -# .github/workflows/ai-failure-enrich.yaml, is called afterwards from here -# and rewrites what this one produced into a triaged artefact. It needs an -# API key and an environment, and if it is unprovisioned or broken the -# notification has already happened regardless. +# 1. `notify` opens or comments on an issue. No LLM and no secrets: once it +# has run, a notification exists. +# 2. `enrich` (ai-failure-enrich.yaml) rewrites that into a triaged +# artefact. If its key or environment is missing, 1 has already happened. on: workflow_call: From 2e79c77e12e93eceefc8c7b8eca6fc06f8aad76e Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 1 Sep 2026 09:14:06 +1200 Subject: [PATCH 32/37] ci: run the notifier from its own package, not a copy in this repo The script and its tests now live in canonical/charm-tech-code, where the other Charm Tech team tooling is going. This repo keeps the workflow half: the notifier changes, the caller `secrets:` lines, and the enrich workflow, which reaches the code through `uvx --from git+...` against a pinned SHA -- the same trust decision as every `actions/checkout@` pin above it. The `pyproject.toml` changes go with the script: `pythonpath`, the pyright include and extraPaths entries, and the ruff per-file ignore all existed only to let `scripts/test/` run here, and scripts/ is now empty. The pin is on `move-ai-failure-notifier`; it wants re-pointing at the merge commit once that branch lands. --- .github/workflows/ai-failure-enrich.yaml | 6 +- pyproject.toml | 13 +- scripts/README.md | 10 - scripts/ai_failure_notifier.py | 1461 ---------------------- scripts/test/test_ai_failure_notifier.py | 1231 ------------------ 5 files changed, 6 insertions(+), 2715 deletions(-) delete mode 100644 scripts/README.md delete mode 100755 scripts/ai_failure_notifier.py delete mode 100644 scripts/test/test_ai_failure_notifier.py diff --git a/.github/workflows/ai-failure-enrich.yaml b/.github/workflows/ai-failure-enrich.yaml index 950786a0f..a12fe7835 100644 --- a/.github/workflows/ai-failure-enrich.yaml +++ b/.github/workflows/ai-failure-enrich.yaml @@ -64,7 +64,9 @@ jobs: # # Each repo adopting this provisions its own. Absent key -> the # script uses the plain fallback body without calling OpenRouter - # (see scripts/ai_failure_notifier.py main()). + # (see the package's cli.main()). OPENROUTER_API_KEY: ${{ secrets.OPENROUTER_API_KEY }} OPENROUTER_MODEL: ${{ vars.OPENROUTER_MODEL }} - run: uv run scripts/ai_failure_notifier.py + run: >- + uvx --from "git+https://github.com/canonical/charm-tech-code@3efa0a710d469cc235dc2251b7903259ac4827c1#subdirectory=ai-failure-notifier" + ai-failure-notifier diff --git a/pyproject.toml b/pyproject.toml index d6fb4b917..04a54ce8b 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -108,11 +108,6 @@ include = ["ops", "ops._private", "ops.hookcmds", "ops.lib"] version = {attr = "ops.version.version"} # Testing tools configuration -[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"] - [tool.coverage.run] branch = true @@ -240,10 +235,6 @@ exclude = ["tracing/ops_tracing/vendor/*"] # exact-literal config values; pytest.approx isn't appropriate here. "RUF069", ] -"scripts/test/*" = [ - # All documentation linting. - "D", -] "testing/tests/*" = [ # All documentation linting. "D", @@ -293,11 +284,11 @@ convention = "google" builtins-ignorelist = ["id", "min", "map", "range", "type", "TimeoutError", "ConnectionError", "Warning", "input", "format"] [tool.pyright] -include = ["ops/*.py", "ops/_private/*.py", "test/*.py", "test/charms/*/src/*.py", "testing/src/*.py", "testing/src/scenario/*.py", "testing/tests/*.py", "testing/tests/test_e2e/*.py", "scripts/test/*.py"] +include = ["ops/*.py", "ops/_private/*.py", "test/*.py", "test/charms/*/src/*.py", "testing/src/*.py", "testing/src/scenario/*.py", "testing/tests/*.py", "testing/tests/test_e2e/*.py"] exclude = [ "tracing/*", ] -extraPaths = ["testing", "tracing", "scripts"] # scripts: workflow scripts under test +extraPaths = ["testing", "tracing"] pythonVersion = "3.10" # check no python > 3.10 features are used pythonPlatform = "All" typeCheckingMode = "strict" diff --git a/scripts/README.md b/scripts/README.md deleted file mode 100644 index e48e6f404..000000000 --- a/scripts/README.md +++ /dev/null @@ -1,10 +0,0 @@ -# Repository scripts - -Standalone scripts that support the repository itself rather than shipping as -part of `ops`: CI helpers, release tooling, and similar. They are plain -scripts, not a package, so tests in `scripts/test/` import them by bare module -name (`pythonpath = ["scripts"]` in `pyproject.toml`). - -Those tests are collected by the normal `tox -e unit` run. That is the point of -this directory: pytest skips dot-directories, so anything under `.github/` -never runs in CI. diff --git a/scripts/ai_failure_notifier.py b/scripts/ai_failure_notifier.py deleted file mode 100755 index 18761afb1..000000000 --- a/scripts/ai_failure_notifier.py +++ /dev/null @@ -1,1461 +0,0 @@ -#!/usr/bin/env python3 -# -# Copyright 2026 Canonical Ltd. -# -# Licensed under the Apache License, Version 2.0 (the "License"); -# you may not use this file except in compliance with the License. -# You may obtain a copy of the License at -# -# http://www.apache.org/licenses/LICENSE-2.0 -# -# Unless required by applicable law or agreed to in writing, software -# distributed under the License is distributed on an "AS IS" BASIS, -# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -# See the License for the specific language governing permissions and -# limitations under the License. - -"""ai-failure-notifications enrichment step. - -Invoked by `.github/workflows/ai-failure-enrich.yaml` after -`.github/workflows/notify-scheduled-failure.yaml` (the notifier) has already -created or commented on a placeholder issue for a failed scheduled workflow -run. This script: - -1. Finds the placeholder the notifier just touched (or, on a same-run - re-fire, the issue an earlier run of this script already enriched). -2. Fetches and parses the failing job logs into a deterministic failure - signature. -3. Builds a small candidate-issue pool (coarse title/body search). -4. Asks an LLM (via OpenRouter) to decide comment-vs-new and draft the text, - validates the response against the envelope schema, and applies it via - `gh`. -5. Falls back to a plain, generic issue/comment (still marker-stamped) if - OpenRouter is unreachable, misconfigured, or returns invalid JSON. - -The functions above the `--- I/O ---` marker are pure and unit-tested in -`scripts/test/test_ai_failure_notifier.py`. Everything below it talks to `gh` -or OpenRouter and is exercised only by mocking in tests. -""" - -from __future__ import annotations - -import dataclasses -import datetime -import hashlib -import json -import os -import re -import subprocess -import sys -import urllib.request -from typing import Any, Literal - -MARKER_PREFIX = 'ai-failure-notifications' -DEFAULT_MODEL = 'deepseek/deepseek-chat' # DeepSeek V3 on OpenRouter. -CLOSED_CANDIDATE_WINDOW_DAYS = 14 -MAX_CANDIDATES = 3 -# 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 - -# Colour escapes, which Actions logs are full of. Two alternatives, because -# the logs contain both the real thing and a mangled form where the ESC byte -# has already been stripped, leaving a bare "[32m". -ANSI = re.compile( - r""" - \x1b\[ [0-9;]* [A-Za-z] # a full escape: ESC [ params letter - | - \[ \d+ (?:;\d+)* m # ESC already stripped: [32m, [1;33m - """, - re.VERBOSE, -) - -# The timestamp Actions prefixes to every log line, for example -# "2026-07-21T16:17:04.8204062Z ". Stripped before anything else is matched. -TS = re.compile( - r""" - ^\d{4}-\d{2}-\d{2} # date: 2026-07-21 - T\d{2}:\d{2}:\d{2} # time: T16:17:04 - \.\d+Z[ ] # fractional seconds, zone, one trailing space - """, - re.VERBOSE, -) - -# Actions' own annotation for a failing step. -ERROR_MARKER = re.compile(r'##\[error\]') - -# The runner opens every step with "##[group]Run