From fa039b1686fa201d92588bc998a1e58876fd615f Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 15 Sep 2026 11:48:58 +1200 Subject: [PATCH 1/2] style: clear ai-failure-notifier's findings under the team's rule set `style/python.md` names seventeen ruff rule groups; the monorepo root turns on eight. Turning on the rest finds eight things here, which is what has been keeping the standard out of the root config. None of them is a behaviour change. * Two docstring summaries in the imperative mood (`D401`). * `fetch_issue_texts` builds its list with `extend` rather than a loop that appends (`PERF401`). * `resolve_origin` discards `find_run_markers`' third value deliberately -- the passed-in issue wins -- so it is `_` rather than a name nothing reads (`RUF059`). * One nested `with` in a test becomes one `with` and two contexts (`SIM117`). * The two `subprocess` findings are marked rather than changed: `gh` really is called by name and resolved from PATH, and the argv really is a list this module builds, so `S603`/`S607` are answered with a `noqa` and the reason next to it, the same way `S607` already was. The test conftest imports `subprocess` only to patch it out, which is what its `S404` says. --- .../charm_tech_code/ai_failure_notifier/_apply.py | 2 +- .../ai_failure_notifier/_github.py | 15 ++++++++++----- .../ai_failure_notifier/_models.py | 2 +- ai-failure-notifier/tests/conftest.py | 2 +- .../tests/test_ai_failure_notifier.py | 8 +++++--- 5 files changed, 18 insertions(+), 11 deletions(-) diff --git a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_apply.py b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_apply.py index d30fbf5..a4b60c2 100644 --- a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_apply.py +++ b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_apply.py @@ -23,7 +23,7 @@ def plain_fallback_body(workflow_name: str, run_url: str) -> str: - """The plain, generic body text used whenever enrichment is unavailable.""" + """Render the generic body used whenever enrichment is unavailable.""" return f"Scheduled workflow '{workflow_name}' failed: {run_url}" diff --git a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_github.py b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_github.py index 16d0922..418fe8a 100644 --- a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_github.py +++ b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_github.py @@ -18,7 +18,7 @@ from __future__ import annotations import json -import subprocess +import subprocess # noqa: S404 -- every call below is a fixed `gh` argv, never a shell string from typing import Any from . import _summary @@ -29,7 +29,13 @@ 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 + # S603: the argv is a list this module builds; nothing is a shell string. + return subprocess.run( # noqa: S603 + ['gh', *args], # noqa: S607 + text=True, + capture_output=True, + check=check, + ) def gh_json(*args: str) -> Any: @@ -111,8 +117,7 @@ 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 '') + texts.extend(comment.get('body') or '' for comment in data.get('comments') or []) return texts @@ -138,7 +143,7 @@ def resolve_origin( issue keeps that lookup to reading the one issue we were handed. """ texts = [(notify_issue, text) for text in fetch_issue_texts(repo, notify_issue)] - enriched_issue, origin_kind, origin_issue = find_run_markers(texts, run_id) + enriched_issue, origin_kind, _ = find_run_markers(texts, run_id) # The passed-in values win: a marker we failed to find on the issue does # not make the issue the wrong one. return enriched_issue, notify_origin or origin_kind, notify_issue diff --git a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_models.py b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_models.py index 56f68c2..0897332 100644 --- a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_models.py +++ b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_models.py @@ -93,6 +93,6 @@ def from_gh(cls, data: dict[str, Any]) -> CandidateIssue: ) def excerpt(self) -> str: - """The first line of the body, bounded, for the candidate block.""" + """Return 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)' diff --git a/ai-failure-notifier/tests/conftest.py b/ai-failure-notifier/tests/conftest.py index 8bc6488..d16cb7d 100644 --- a/ai-failure-notifier/tests/conftest.py +++ b/ai-failure-notifier/tests/conftest.py @@ -24,7 +24,7 @@ from __future__ import annotations -import subprocess +import subprocess # noqa: S404 -- patched out, never run, see `_no_subprocess` import urllib.request import pytest diff --git a/ai-failure-notifier/tests/test_ai_failure_notifier.py b/ai-failure-notifier/tests/test_ai_failure_notifier.py index 885be20..6364801 100644 --- a/ai-failure-notifier/tests/test_ai_failure_notifier.py +++ b/ai-failure-notifier/tests/test_ai_failure_notifier.py @@ -1144,9 +1144,11 @@ def test_http_error_propagates_so_main_can_fall_back(self): io.BytesIO(b''), ) self.addCleanup(error.close) - with mock.patch.object(_openrouter.urllib.request, 'urlopen', side_effect=error): - with self.assertRaises(urllib.error.HTTPError): - _openrouter.call_openrouter('sys', 'user', 'm', 'k') + with ( + mock.patch.object(_openrouter.urllib.request, 'urlopen', side_effect=error), + self.assertRaises(urllib.error.HTTPError), + ): + _openrouter.call_openrouter('sys', 'user', 'm', 'k') class ResolveOriginTests(unittest.TestCase): From 8f324649cf22645844336f4093d94202f3bd9233 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Tue, 15 Sep 2026 12:01:06 +1200 Subject: [PATCH 2/2] style: suppress by rule name with ruff's own directive, per review `# ruff: ignore[start-process-with-partial-path]` says what is being allowed; `# noqa: S607` needs a lookup. Ruff reads the name as well as the code, matches on it (naming a different rule does not suppress), and `RUF100` reports an unnecessary one as an unused suppression, so nothing is lost by the change. Also converts `_openrouter`'s pre-existing `S310`, which this branch did not otherwise touch: one package with two suppression syntaxes is worse than either. One thing to know: the directive is preview-gated. Under `preview = false` it is ignored and the findings come back. The root config sets `preview = true` and its comment already calls that load-bearing, so this is a second thing depending on it. Co-authored-by: James Garner --- .../src/charm_tech_code/ai_failure_notifier/_github.py | 10 +++++----- .../charm_tech_code/ai_failure_notifier/_openrouter.py | 4 ++-- ai-failure-notifier/tests/conftest.py | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_github.py b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_github.py index 418fe8a..2efba8e 100644 --- a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_github.py +++ b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_github.py @@ -18,7 +18,7 @@ from __future__ import annotations import json -import subprocess # noqa: S404 -- every call below is a fixed `gh` argv, never a shell string +import subprocess # ruff: ignore[suspicious-subprocess-import] -- see `gh` below from typing import Any from . import _summary @@ -28,10 +28,10 @@ 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. - # S603: the argv is a list this module builds; nothing is a shell string. - return subprocess.run( # noqa: S603 - ['gh', *args], # noqa: S607 + # The argv is a list this module builds, and `gh` is deliberately called by + # name so the runner's PATH resolves it. + return subprocess.run( # ruff: ignore[subprocess-without-shell-equals-true] + ['gh', *args], # ruff: ignore[start-process-with-partial-path] text=True, capture_output=True, check=check, diff --git a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_openrouter.py b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_openrouter.py index 9d57e5e..f56e181 100644 --- a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_openrouter.py +++ b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_openrouter.py @@ -55,8 +55,8 @@ def call_openrouter( 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 + # The URL is a literal https endpoint, not caller-controlled. + with urllib.request.urlopen(request, timeout=60) as response: # ruff: ignore[suspicious-url-open-usage] body = json.loads(response.read().decode()) content = body['choices'][0]['message']['content'] return json.loads(content) diff --git a/ai-failure-notifier/tests/conftest.py b/ai-failure-notifier/tests/conftest.py index d16cb7d..a0ff215 100644 --- a/ai-failure-notifier/tests/conftest.py +++ b/ai-failure-notifier/tests/conftest.py @@ -24,7 +24,7 @@ from __future__ import annotations -import subprocess # noqa: S404 -- patched out, never run, see `_no_subprocess` +import subprocess # ruff: ignore[suspicious-subprocess-import] -- see `_no_subprocess` import urllib.request import pytest