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/_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/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/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 885be20..9a63ac3 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. @@ -899,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, []) @@ -987,6 +1071,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='') @@ -1132,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):