From b84519a1690ddebeefd0cd56b888473794ce495d Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 11 Sep 2026 09:15:45 +1200 Subject: [PATCH 1/3] fix: a repo without the model's issue type gets one issue, not two `gh issue create --type ` creates the issue and only then fails on the type, so the retry-without-`--type` that followed a failure created a second, identical issue: same title, same body, same run marker. A fork, or any repo whose organisation has not enabled issue types, hit this on every enrichment that asked for one. Issue types are now resolved before the create, the same way labels already were, and there is nothing left to retry. The match ignores case and passes the repo's own spelling, because the model is asked for "bug" and GitHub's type is "Bug" - so the failure path was reachable even where types do exist. A type the repo doesn't have is dropped with a note in the step summary, which is what happens to an unknown label. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013trjz242SkZ2SxPami6XE7 --- .../ai_failure_notifier/_apply.py | 27 +++--- .../ai_failure_notifier/_github.py | 35 +++++++ .../tests/test_ai_failure_notifier.py | 92 +++++++++++++++++++ 3 files changed, 142 insertions(+), 12 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..f1a1e81 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 @@ -60,19 +60,22 @@ def apply_entry( 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 + # Resolved before the issue is created, never retried after it: a + # failed `gh issue create --type` has already created the issue, so a + # second attempt without the type is a duplicate rather than a repair. + wanted_type = entry.get('issue_type') + issue_type = ( + _github.match_issue_type(wanted_type, _github.existing_issue_types(repo)) + if wanted_type + else None + ) + if wanted_type and not issue_type: + _summary.write_step_summary( + f'Dropped issue type "{wanted_type}", which this repo does not have.' + ) if issue_type: - result = _github.gh(*args, '--type', issue_type, check=False) - if result.returncode != 0: - _summary.write_step_summary( - f'`gh issue create --type {issue_type}` failed ({result.stderr.strip()}); ' - 'retrying without --type.' - ) - result = None - if result is None: - result = _github.gh(*args) - return result.stdout.strip() + args += ['--type', issue_type] + return _github.gh(*args).stdout.strip() else: target = entry.get('target_issue', default_target) _github.gh('issue', 'comment', str(target), '--repo', repo, '--body', body) 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..2be8b2b 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 @@ -198,3 +198,38 @@ def existing_labels(repo: str) -> set[str]: 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 existing_issue_types(repo: str) -> set[str]: + """Return the issue type names enabled for `repo`, which may be none. + + Issue types come from the owning organisation, so a repo can have none at + all: a personal fork returns `null` here, and so does any repo whose org + has not enabled them. + """ + owner, _, name = repo.partition('/') + query = ( + 'query($owner: String!, $name: String!) { repository(owner: $owner, name: $name) ' + '{ issueTypes(first: 50) { nodes { name isEnabled } } } }' + ) + data = ( + gh_json( + 'api', 'graphql', '-f', f'query={query}', '-F', f'owner={owner}', '-F', f'name={name}' + ) + or {} + ) + repository = (data.get('data') or {}).get('repository') or {} + types = repository.get('issueTypes') or {} + return {node['name'] for node in types.get('nodes') or [] if node.get('isEnabled')} + + +def match_issue_type(issue_type: str | None, available: set[str]) -> str | None: + """Resolve `issue_type` to the repo's own spelling, or `None` if it has no such type. + + The model is asked for a lowercase name, and GitHub's are capitalised, so + the match ignores case and the repo's spelling is what gets passed on. + """ + if not issue_type: + return None + folded = issue_type.casefold() + return next((name for name in sorted(available) if name.casefold() == folded), None) diff --git a/ai-failure-notifier/tests/test_ai_failure_notifier.py b/ai-failure-notifier/tests/test_ai_failure_notifier.py index 885be20..dc0b3db 100644 --- a/ai-failure-notifier/tests/test_ai_failure_notifier.py +++ b/ai-failure-notifier/tests/test_ai_failure_notifier.py @@ -832,6 +832,31 @@ def test_existing_labels_requests_the_name_field(self): self.assertEqual(args[:2], ('label', 'list')) self.assertEqual(args[args.index('--json') + 1], 'name') + def test_existing_issue_types_returns_the_enabled_ones(self): + gh_calls = self._capture( + '{"data": {"repository": {"issueTypes": {"nodes": [' + '{"name": "Bug", "isEnabled": true}, ' + '{"name": "Task", "isEnabled": true}, ' + '{"name": "Epic", "isEnabled": false}]}}}}' + ) + with mock.patch.object(_github, 'gh', side_effect=gh_calls): + types = _github.existing_issue_types('example/repo') + self.assertEqual(types, {'Bug', 'Task'}) + args = gh_calls.call_args.args + self.assertEqual(args[:2], ('api', 'graphql')) + + def test_existing_issue_types_tolerates_a_repo_with_none(self): + """A personal fork, or any repo whose org has not enabled types.""" + gh_calls = self._capture('{"data": {"repository": {"issueTypes": null}}}') + with mock.patch.object(_github, 'gh', side_effect=gh_calls): + self.assertEqual(_github.existing_issue_types('example/repo'), set()) + + def test_match_issue_type_ignores_case_and_uses_the_repo_spelling(self): + self.assertEqual(_github.match_issue_type('bug', {'Bug', 'Task'}), 'Bug') + self.assertIsNone(_github.match_issue_type('bug', set())) + self.assertIsNone(_github.match_issue_type('chore', {'Bug', 'Task'})) + self.assertIsNone(_github.match_issue_type(None, {'Bug'})) + class NormalisationTests(unittest.TestCase): """Fields that do not apply to the chosen action are dropped, not fatal. @@ -987,6 +1012,73 @@ def test_applied_comment_body_has_the_footer(self): self.assertEqual(args[:3], ('issue', 'comment', '7')) self.assertIn('Workflow: ops Smoke Tests', args[args.index('--body') + 1]) + def test_a_missing_issue_type_creates_one_issue_and_not_two(self): + """The type is resolved before the create, so there is nothing to retry. + + `gh issue create --type` creates the issue and only then fails on the + type, so retrying without it opened a second, identical issue. + """ + 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': 'bug', + } + with ( + mock.patch.object(_github, 'gh', side_effect=gh_calls), + mock.patch.object(_github, 'existing_labels', return_value=set()), + mock.patch.object(_github, 'existing_issue_types', return_value=set()), + mock.patch.object(_summary, 'write_step_summary') as summary, + ): + _apply.apply_entry('example/repo', entry, '', 'ops Smoke Tests') + gh_calls.assert_called_once() + self.assertNotIn('--type', gh_calls.call_args.args) + self.assertIn('bug', summary.call_args.args[0]) + + def test_a_known_issue_type_is_passed_in_the_repo_spelling(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': 'bug', + } + with ( + mock.patch.object(_github, 'gh', side_effect=gh_calls), + mock.patch.object(_github, 'existing_labels', return_value=set()), + mock.patch.object(_github, 'existing_issue_types', return_value={'Bug', 'Task'}), + ): + _apply.apply_entry('example/repo', entry, '', 'ops Smoke Tests') + gh_calls.assert_called_once() + args = gh_calls.call_args.args + self.assertEqual(args[args.index('--type') + 1], 'Bug') + + def test_no_issue_type_asked_for_costs_no_lookup(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(_github, 'gh', side_effect=gh_calls), + mock.patch.object(_github, 'existing_labels', return_value=set()), + mock.patch.object(_github, 'existing_issue_types') as types, + ): + _apply.apply_entry('example/repo', entry, '', 'ops Smoke Tests') + types.assert_not_called() + 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='') From 4c5d46f68fd80c1f26e8af35719192ddcddaabf6 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 11 Sep 2026 09:27:35 +1200 Subject: [PATCH 2/3] fix: read a target issue the model wrote as "#44" Issues are written `#44` everywhere a person sees one, and the model returns that string often enough to matter. The schema wants an integer, so the whole envelope was rejected and a usable enrichment was thrown away over a `#`, falling back to the plain notice. Seen on a real run, against a candidate the notifier itself had just passed down. A `#`-prefixed or bare digit string is now read as the number it means, in the envelope and in each `also` entry. Anything else is left exactly as it is, for the schema to reject on its own terms. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_013trjz242SkZ2SxPami6XE7 --- .../ai_failure_notifier/_envelope.py | 21 ++++++- .../tests/test_ai_failure_notifier.py | 59 +++++++++++++++++++ 2 files changed, 78 insertions(+), 2 deletions(-) diff --git a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_envelope.py b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_envelope.py index d9f7c3d..60eedae 100644 --- a/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_envelope.py +++ b/ai-failure-notifier/src/charm_tech_code/ai_failure_notifier/_envelope.py @@ -52,17 +52,34 @@ def drop_inapplicable_fields(entry: Any) -> tuple[Any, list[str]]: return {k: v for k, v in entry.items() if k not in dropped}, dropped +def coerce_target_issue(entry: Any) -> Any: + """Turn a `target_issue` the model wrote as text into the integer it means. + + Issues are written `#44` everywhere a person sees them, and the model + returns that string often enough to matter: the schema wants an integer, so + the whole envelope was rejected and a usable body was thrown away for a `#`. + Anything that is not a plain issue reference is left exactly as it is, for + the schema to reject on its own terms. + """ + if not isinstance(entry, dict) or not isinstance(entry.get('target_issue'), str): + return entry + text = entry['target_issue'].strip().removeprefix('#') + if not text.isdigit(): + return entry + return {**entry, 'target_issue': int(text)} + + 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) + cleaned, dropped = drop_inapplicable_fields(coerce_target_issue(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) + entry, entry_dropped = drop_inapplicable_fields(coerce_target_issue(entry)) notes += [f'envelope.also[{i}]: {f}' for f in entry_dropped] entries.append(entry) cleaned = {**cleaned, 'also': entries} diff --git a/ai-failure-notifier/tests/test_ai_failure_notifier.py b/ai-failure-notifier/tests/test_ai_failure_notifier.py index dc0b3db..1e135ed 100644 --- a/ai-failure-notifier/tests/test_ai_failure_notifier.py +++ b/ai-failure-notifier/tests/test_ai_failure_notifier.py @@ -924,6 +924,65 @@ def test_also_entries_are_normalised_too(self): self.assertEqual(dropped, ['envelope.also[0]: title']) self.assertEqual(_envelope.validate_envelope(cleaned), []) + def test_a_hash_prefixed_target_issue_is_coerced(self): + """The model writes issues the way people do, and the schema wants an int.""" + envelope: dict[str, Any] = { + 'action': 'comment', + 'target_issue': '#44', + 'body': 'b', + 'dedup_reason': 'd', + 'confidence': 'low', + } + cleaned, dropped = _envelope.normalise_envelope(envelope) + self.assertEqual(cleaned['target_issue'], 44) + self.assertEqual(dropped, []) + self.assertEqual(_envelope.validate_envelope(cleaned), []) + + def test_a_bare_digit_string_target_issue_is_coerced(self): + envelope: dict[str, Any] = { + 'action': 'comment', + 'target_issue': ' 44 ', + 'body': 'b', + 'dedup_reason': 'd', + 'confidence': 'low', + } + cleaned, _ = _envelope.normalise_envelope(envelope) + self.assertEqual(cleaned['target_issue'], 44) + + def test_a_target_issue_that_is_not_a_reference_is_left_for_the_schema(self): + envelope: dict[str, Any] = { + 'action': 'comment', + 'target_issue': 'the loki one', + 'body': 'b', + 'dedup_reason': 'd', + 'confidence': 'low', + } + cleaned, _ = _envelope.normalise_envelope(envelope) + self.assertEqual(cleaned['target_issue'], 'the loki one') + self.assertNotEqual(_envelope.validate_envelope(cleaned), []) + + def test_also_entries_get_the_same_coercion(self): + inner: dict[str, Any] = { + 'action': 'comment', + 'target_issue': '#1', + 'body': 'b', + '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, _ = _envelope.normalise_envelope(envelope) + self.assertEqual(cleaned['also'][0]['target_issue'], 1) + self.assertEqual(_envelope.validate_envelope(cleaned), []) + def test_nothing_dropped_leaves_the_envelope_alone(self): cleaned, dropped = _envelope.normalise_envelope(FIXTURE_ENVELOPE) self.assertEqual(dropped, []) From 3702a80516503509a0472f62dcffbcf022654925 Mon Sep 17 00:00:00 2001 From: Tony Meyer Date: Fri, 11 Sep 2026 09:40:12 +1200 Subject: [PATCH 3/3] fix: say what OpenRouter objected to, not just that it did `str(HTTPError)` is only ever "HTTP Error 400: Bad Request", which doesn't say whether the model, the key or the schema was the problem. OpenRouter puts the reason in the response body, so a 400 cost a schema read to diagnose when the answer was one line away. The body is now read and appended to the message, JSON `error.message` where there is one and the raw text otherwise, truncated. Reading it is not allowed to raise: an unreadable explanation must not lose the status code that came with it. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01SEuiHk71P4R7hZD7dqfvGh --- .../ai_failure_notifier/_openrouter.py | 43 ++++++++++++++++--- .../tests/test_ai_failure_notifier.py | 40 +++++++++++++++-- 2 files changed, 73 insertions(+), 10 deletions(-) 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..26a8326 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 @@ -18,6 +18,7 @@ from __future__ import annotations import json +import urllib.error import urllib.request from typing import Any @@ -30,9 +31,9 @@ def call_openrouter( """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. + dependencies at all. A non-2xx response is raised as an error carrying + OpenRouter's own explanation, which main() treats the same as any other + OpenRouter failure: fall back to the plain body. """ payload = { 'model': model, @@ -55,8 +56,38 @@ 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 - body = json.loads(response.read().decode()) + try: + # 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()) + except urllib.error.HTTPError as exc: + # `str(exc)` is only ever "HTTP Error 400: Bad Request", which says + # nothing about which of the model, the key or the schema OpenRouter + # objected to. The reason is in the response body, and reading it is + # the difference between a glance and an afternoon. + raise RuntimeError(f'{exc} - {_error_detail(exc)}') from exc content = body['choices'][0]['message']['content'] return json.loads(content) + + +def _error_detail(exc: urllib.error.HTTPError) -> str: + """OpenRouter's own explanation of a non-2xx, as far as it can be read. + + The body is JSON in the ordinary case and can be anything at all when a + proxy answers instead, so nothing here is allowed to raise: an unreadable + explanation must not replace the status code that came with it. + """ + try: + raw = exc.read().decode(errors='replace').strip() + except Exception: # noqa: BLE001 - any read failure means no detail, not a crash. + return 'no response body' + if not raw: + return 'empty response body' + try: + parsed = json.loads(raw) + except ValueError: + return raw[:500] + error = parsed.get('error') if isinstance(parsed, dict) else None + if isinstance(error, dict) and error.get('message'): + return str(error['message'])[:500] + return raw[:500] diff --git a/ai-failure-notifier/tests/test_ai_failure_notifier.py b/ai-failure-notifier/tests/test_ai_failure_notifier.py index 1e135ed..9a63ac3 100644 --- a/ai-failure-notifier/tests/test_ai_failure_notifier.py +++ b/ai-failure-notifier/tests/test_ai_failure_notifier.py @@ -1283,21 +1283,53 @@ def test_posts_json_with_auth_and_schema(self): ) self.assertTrue(sent['response_format']['json_schema']['strict']) - def test_http_error_propagates_so_main_can_fall_back(self): + def _http_error(self, status: int, body: bytes) -> urllib.error.HTTPError: # 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, + status, 'boom', email.message.Message(), - io.BytesIO(b''), + io.BytesIO(body), ) self.addCleanup(error.close) + return error + + def test_http_error_raises_so_main_can_fall_back(self): + error = self._http_error(500, b'') + with mock.patch.object(_openrouter.urllib.request, 'urlopen', side_effect=error): + with self.assertRaises(RuntimeError): + _openrouter.call_openrouter('sys', 'user', 'm', 'k') + + def test_the_error_carries_openrouters_own_explanation(self): + """A 400 says only "Bad Request"; which of the model, key or schema is in the body.""" + body = json.dumps({ + 'error': {'code': 400, 'message': "Invalid schema: 'required' is missing 'also'"} + }).encode() + error = self._http_error(400, body) + with mock.patch.object(_openrouter.urllib.request, 'urlopen', side_effect=error): + with self.assertRaises(RuntimeError) as raised: + _openrouter.call_openrouter('sys', 'user', 'm', 'k') + message = str(raised.exception) + self.assertIn('HTTP Error 400', message) + self.assertIn("'required' is missing 'also'", message) + + def test_a_body_that_is_not_json_is_reported_as_it_came(self): + error = self._http_error(502, b'upstream is unwell') + with mock.patch.object(_openrouter.urllib.request, 'urlopen', side_effect=error): + with self.assertRaises(RuntimeError) as raised: + _openrouter.call_openrouter('sys', 'user', 'm', 'k') + self.assertIn('upstream is unwell', str(raised.exception)) + + def test_an_unreadable_body_still_leaves_the_status(self): + error = self._http_error(429, b'') + error.read = mock.Mock(side_effect=OSError('connection reset')) with mock.patch.object(_openrouter.urllib.request, 'urlopen', side_effect=error): - with self.assertRaises(urllib.error.HTTPError): + with self.assertRaises(RuntimeError) as raised: _openrouter.call_openrouter('sys', 'user', 'm', 'k') + self.assertIn('HTTP Error 429', str(raised.exception)) class ResolveOriginTests(unittest.TestCase):