diff --git a/ariadne/services/hermes_code_repair.py b/ariadne/services/hermes_code_repair.py index 545dc97..cb64f3d 100644 --- a/ariadne/services/hermes_code_repair.py +++ b/ariadne/services/hermes_code_repair.py @@ -26,6 +26,9 @@ _COMMIT_IDENTITY = {"name": "Hermes Agent", "email": "hermes@bstein.dev"} _COMMIT_OK_STATUSES = {HTTP_OK, HTTP_CREATED} # The Hermes console reopens a finished run from its id at this route. RUN_PATH = "/chat?resume=" +# Every proposal title is built from this, so the incident an open pull +# request belongs to can be read back without storing a second index. +PR_TITLE_PREFIX = "fix(hermes): repair " _BRANCH_RETRY_STATUSES = {HTTP_NOT_FOUND, HTTP_UNPROCESSABLE} _BRANCH_UNSAFE = re.compile(r"[^A-Za-z0-9._-]+") @@ -86,6 +89,45 @@ def find_open_proposal(cfg: dict) -> dict[str, Any]: return _oldest_repair_pull(response, _base_branch(cfg)) +def open_proposal_incidents(cfg: dict) -> tuple[list[str], str | None]: + """List the incidents that already have an open repair pull request. + + Inputs: `cfg` as for `fetch_file`. Outputs: (incident_ids, error). + + Read back from the pull request titles rather than from a stored index, + because the pull requests are the thing that actually exists - an index + could disagree with them, and the whole point of the check is to not + propose something a reviewer is already looking at. + + Fails open: on any error the list is empty, so a lookup failure produces a + duplicate rather than silently suppressing real work. + """ + + base_url = _base_url(cfg) + if not base_url: + return [], "gitea base url is empty" + try: + with httpx.Client(timeout=_timeout(cfg)) as client: + response = client.get( + f"{base_url}/api/v1/repos/{_owner(cfg)}/{_repo(cfg)}/pulls", + headers=_headers(cfg), + params={"state": "open", "limit": _OPEN_PULLS_LIMIT}, + ) + if response.status_code != HTTP_OK: + return [], f"open proposal lookup http {response.status_code}" + payload = response.json() + except Exception as exc: + return [], f"open proposal lookup failed: {exc}" + if not isinstance(payload, list): + return [], "open proposal payload is not a list" + found = [] + for pull in payload: + title = str(pull.get("title") or "") if isinstance(pull, dict) else "" + if title.startswith(PR_TITLE_PREFIX): + found.append(title[len(PR_TITLE_PREFIX) :].strip()) + return found, None + + def push_branch( cfg: dict, incident_id: str, ref: Any, patch: Any, patched_contents: str ) -> dict[str, Any]: @@ -137,7 +179,7 @@ def open_pull_request( # noqa: PLR0913 - the body needs the full proposal prove payload = { "head": branch, "base": _base_branch(cfg), - "title": f"fix(hermes): repair {incident_id}", + "title": f"{PR_TITLE_PREFIX}{incident_id}", "body": _pr_body(incident_id, patch, analysis, run_id, cfg), } try: diff --git a/ariadne/services/hermes_incident_body.py b/ariadne/services/hermes_incident_body.py index 943e696..05d6c89 100644 --- a/ariadne/services/hermes_incident_body.py +++ b/ariadne/services/hermes_incident_body.py @@ -38,7 +38,7 @@ _AUDIT_NOTE = ( "event types `hermes_autotriage_incident` and `hermes_autotriage_diagnosis`." ) _FOOTER_TEMPLATE = ( - "Filed automatically by Ariadne from a Hermes Agent diagnosis (run `{run_id}`). " + "Filed automatically by Ariadne from a Hermes Agent diagnosis ({run}). " "Hermes has no write access to this repository; no files or infrastructure were changed." ) # Some escalations never reach a model at all - a build still running has no @@ -168,7 +168,11 @@ def _footer(context: dict) -> str: run_id = str(context.get("run_id") or "") if not run_id or run_id == "unknown": return _NO_RUN_FOOTER - return _FOOTER_TEMPLATE.format(run_id=run_id) + # A bare id is something to copy; a link is a page to open, and that page + # is the whole answer to who decided this. + run_url = str(context.get("run_url") or "") + run = f"run [{run_id}]({run_url})" if run_url else f"run `{run_id}`" + return _FOOTER_TEMPLATE.format(run=run) def _human_section(context: dict) -> str: diff --git a/ariadne/services/hermes_incident_issue.py b/ariadne/services/hermes_incident_issue.py index 78702e9..b8d5f5a 100644 --- a/ariadne/services/hermes_incident_issue.py +++ b/ariadne/services/hermes_incident_issue.py @@ -22,7 +22,7 @@ from typing import Any import httpx from ..utils.logging import get_logger -from . import hermes_incident_body as body +from . import hermes_code_repair, hermes_incident_body as body logger = get_logger(__name__) @@ -171,7 +171,9 @@ def issue_config(config: Any) -> dict[str, Any]: } -def issue_context(base: dict[str, Any], diagnosis: dict[str, Any]) -> dict[str, Any]: +def issue_context( + base: dict[str, Any], diagnosis: dict[str, Any], hermes_ui_url: str = "" +) -> dict[str, Any]: """Flatten the incident, evidence bundle, and outcome into body inputs. Inputs: the incident identity fields and the diagnosis dict passed to @@ -212,6 +214,7 @@ def issue_context(base: dict[str, Any], diagnosis: dict[str, Any]) -> dict[str, # console text is the whole explanation and must not be dropped. "observation": _observation(jenkins) if decision is None else "", "run_id": diagnosis.get("run_id"), + "run_url": hermes_code_repair.run_url(hermes_ui_url, str(diagnosis.get("run_id") or "")), "code_proposal_url": str(proposal.get("url") or "") if isinstance(proposal, dict) else "", } @@ -241,7 +244,7 @@ def _file_issue( if not cfg["enabled"] or target is None or _filed_count(tick_state) >= _max_per_tick(cfg): return None repo_cfg = {**cfg, "owner": target[0], "repo": target[1]} - context = issue_context(base, diagnosis) + context = issue_context(base, diagnosis, str(getattr(config, "hermes_ui_url", "") or "")) existing = find_open_incident_issue( repo_cfg, job, str(context["classification"]), str(context["incident_id"]) ) diff --git a/ariadne/services/hermes_sonar_client.py b/ariadne/services/hermes_sonar_client.py index ed19903..5f9a114 100644 --- a/ariadne/services/hermes_sonar_client.py +++ b/ariadne/services/hermes_sonar_client.py @@ -171,7 +171,11 @@ def issue_url(incident_id: str, ui_url: str) -> str: text = str(incident_id or "") if not base or not text.startswith(INCIDENT_PREFIX): return "" - project, separator, key = text[len(INCIDENT_PREFIX) :].partition("/") + # `sonar///`; the rule sits between them so the + # project is read from the front and the key from the back. + remainder = text[len(INCIDENT_PREFIX) :] + project, separator, rest = remainder.partition("/") + _, _, key = rest.rpartition("/") if "/" in rest else ("", "", rest) if not separator or not project.strip() or not key.strip(): return "" return base + _ISSUE_PATH.format(project=project.strip(), key=key.strip()) diff --git a/ariadne/services/hermes_sonar_sweep.py b/ariadne/services/hermes_sonar_sweep.py index de19a1c..23d431c 100644 --- a/ariadne/services/hermes_sonar_sweep.py +++ b/ariadne/services/hermes_sonar_sweep.py @@ -27,7 +27,7 @@ from typing import Any from ..settings import settings from ..utils.logging import get_logger -from . import hermes_code_flow, hermes_sonar_client +from . import hermes_code_flow, hermes_code_repair, hermes_sonar_client logger = get_logger(__name__) @@ -131,9 +131,10 @@ def _propose_for_project( # noqa: PLR0913 - one project needs every config the issues, error = hermes_sonar_client.fetch_issues(sonar_cfg, project) if error: return {"proposed": 0, "skipped": [f"{project}: {error}"]} - chosen = select_issue(issues, repo_cfg, config) + open_rules = proposed_rules(repo_cfg, project) + chosen = select_issue(issues, repo_cfg, config, open_rules) if chosen is None: - return {"proposed": 0, "skipped": [f"{project}: no mechanically fixable finding"]} + return {"proposed": 0, "skipped": [f"{project}: no new mechanically fixable finding"]} result = hermes_code_flow.propose_code_fix( storage, incident_id=incident_id(project, chosen), @@ -148,11 +149,43 @@ def _propose_for_project( # noqa: PLR0913 - one project needs every config the return {"proposed": 0, "skipped": [f"{project}: {result.get('reason')}"]} -def select_issue(issues: list[dict], repo_cfg: dict, config: Any) -> dict[str, Any] | None: +def proposed_rules(repo_cfg: dict, project: str) -> set[str]: + """Return the rules this project already has an open proposal for. + + Inputs: the resolved per-repo cfg and the SonarQube project key. Outputs: + the rule ids, read back from the open pull requests' incident ids. + + One rule is usually one root cause spread across many files: S2208 appears + in three Ariadne modules, and the cognitive-complexity rule in dozens. One + finding at a time with no memory would open a near-identical pull request + for every instance, and a reviewer facing thirty of those reads none of + them. Until the open one is dealt with, the rest of that rule waits. + + Fails open, like every other duplicate check here: an unreadable list + yields an empty set, so the cost of a lookup failure is one extra proposal + rather than a whole rule silently going unreported. + """ + + incidents, _error = hermes_code_repair.open_proposal_incidents(repo_cfg) + prefix = f"{hermes_sonar_client.INCIDENT_PREFIX}{project}/" + rules = set() + for incident in incidents: + if not incident.startswith(prefix): + continue + rule, separator, _key = incident[len(prefix) :].rpartition("/") + if separator and rule: + rules.add(rule) + return rules + + +def select_issue( + issues: list[dict], repo_cfg: dict, config: Any, open_rules: set[str] | None = None +) -> dict[str, Any] | None: """Pick the single most mechanically fixable finding for one project. Inputs: the normalized findings, the resolved per-repo cfg (whose write - allowlist decides what is patchable at all), and the settings object. + allowlist decides what is patchable at all), the settings object, and the + rules that already have an open proposal. Outputs: one finding, or None when none qualify. Ordered by effort, then severity, then key. Effort leads because it is the @@ -163,10 +196,12 @@ def select_issue(issues: list[dict], repo_cfg: dict, config: Any) -> dict[str, A """ ceiling = _max_effort_minutes(config) + seen = open_rules or set() eligible = [ issue for issue in issues - if _is_writable(str(issue.get("path") or ""), repo_cfg) + if str(issue.get("rule") or "") not in seen + and _is_writable(str(issue.get("path") or ""), repo_cfg) and effort_minutes(issue.get("effort")) is not None and (effort_minutes(issue.get("effort")) or 0) <= ceiling ] @@ -236,9 +271,14 @@ def bundle_for(project: str, issue: dict[str, Any]) -> dict[str, Any]: def incident_id(project: str, issue: dict[str, Any]) -> str: - """Name the incident for one finding, stable across sweeps.""" + """Name the incident for one finding, stable across sweeps. - return f"sonar/{project}/{issue.get('key')}" + Carries the rule as well as the key so an open proposal announces which + root cause is already being reviewed. The alternative was a second index + of proposed rules, which could disagree with the pull requests themselves. + """ + + return f"sonar/{project}/{issue.get('rule')}/{issue.get('key')}" def client_config(config: Any) -> dict[str, Any]: diff --git a/tests/test_hermes_code_repair.py b/tests/test_hermes_code_repair.py index 2a662b4..9d13436 100644 --- a/tests/test_hermes_code_repair.py +++ b/tests/test_hermes_code_repair.py @@ -3,6 +3,8 @@ from __future__ import annotations import base64 import json +import httpx + from ariadne.services import hermes_code_repair as module from ariadne.services.hermes_code_patch import ProposedPatch @@ -430,3 +432,48 @@ def test_a_build_driven_proposal_has_no_finding_line(monkeypatch) -> None: ) assert "SonarQube finding" not in calls["requests"][0][2]["json"]["body"] + + +def test_open_proposal_incidents_reads_them_from_the_titles(monkeypatch) -> None: + """The pull requests are the thing that exists; an index could disagree.""" + + calls = _install_http( + monkeypatch, + [ + FakeResponse( + 200, + [ + {"title": "fix(hermes): repair sonar/ariadne/python:S2208/AZ1"}, + {"title": "fix(hermes): repair ariadne/408"}, + {"title": "chore: something a person opened"}, + {"title": ""}, + "not-a-dict", + ], + ) + ], + ) + incidents, error = module.open_proposal_incidents(_cfg()) + + assert error is None + assert incidents == ["sonar/ariadne/python:S2208/AZ1", "ariadne/408"] + assert calls["requests"][0][2]["params"]["state"] == "open" + + +def test_open_proposal_incidents_fails_open(monkeypatch) -> None: + """A duplicate is noise; a suppressed rule is lost work.""" + + _install_http(monkeypatch, [FakeResponse(500, None)]) + assert module.open_proposal_incidents(_cfg()) == ([], "open proposal lookup http 500") + + _install_http(monkeypatch, [FakeResponse(200, {"not": "a list"})]) + assert module.open_proposal_incidents(_cfg()) == ([], "open proposal payload is not a list") + + _install_http(monkeypatch, [httpx.ConnectError("refused")]) + incidents, error = module.open_proposal_incidents(_cfg()) + assert incidents == [] + assert "open proposal lookup failed" in error + + assert module.open_proposal_incidents({**_cfg(), "gitea_base_url": ""}) == ( + [], + "gitea base url is empty", + ) diff --git a/tests/test_hermes_incident_attribution.py b/tests/test_hermes_incident_attribution.py index bffdd3b..10fa041 100644 --- a/tests/test_hermes_incident_attribution.py +++ b/tests/test_hermes_incident_attribution.py @@ -7,6 +7,23 @@ from types import SimpleNamespace from ariadne.services import hermes_incident_body as body from ariadne.services import hermes_incident_issue as module + +_BASE = {"incident_id": "ariadne/408", "job": "ariadne", "build_number": 408} + + +def _diagnosis(**overrides): + """A completed diagnosis, as maybe_file_issue receives it.""" + + diagnosis = { + "bundle": {"jenkins": {"url": "https://ci.example/job/ariadne/408/"}}, + "outcome": None, + "authorize_reason": "human_required", + "run_id": "run_a5af87af", + } + diagnosis.update(overrides) + return diagnosis + + def test_body_does_not_claim_a_diagnosis_that_never_happened() -> None: """A hung build is escalated without any model call. @@ -99,3 +116,21 @@ def test_explicit_classification_is_used_only_without_a_decision() -> None: "classification": "build_exceeded_time_cap", "run_id": "run_x"}, ) assert ctx["classification"] == "pytest_test_failure" + + +def test_the_issue_footer_links_to_the_hermes_run() -> None: + """A bare id is something to copy; a link is a page to open.""" + + context = module.issue_context(_BASE, _diagnosis(), "https://agent.bstein.dev") + rendered = body.issue_body(context) + + assert context["run_url"].startswith("https://agent.bstein.dev/chat?resume=") + assert f"run [{context['run_id']}]({context['run_url']})" in rendered + + +def test_the_footer_falls_back_to_a_bare_id_without_a_console() -> None: + context = module.issue_context(_BASE, _diagnosis()) + rendered = body.issue_body(context) + + assert context["run_url"] == "" + assert f"run `{context['run_id']}`" in rendered diff --git a/tests/test_hermes_incident_issue.py b/tests/test_hermes_incident_issue.py index 70fd599..aad252c 100644 --- a/tests/test_hermes_incident_issue.py +++ b/tests/test_hermes_incident_issue.py @@ -493,3 +493,4 @@ def test_broken_repo_map_entries_file_nothing(monkeypatch) -> None: is None ) assert calls["requests"] == [] + diff --git a/tests/test_hermes_sonar_sweep.py b/tests/test_hermes_sonar_sweep.py index c823261..45a737a 100644 --- a/tests/test_hermes_sonar_sweep.py +++ b/tests/test_hermes_sonar_sweep.py @@ -72,6 +72,10 @@ def wiring(monkeypatch): return calls["result"] monkeypatch.setattr(module.hermes_code_flow, "propose_code_fix", _propose) + monkeypatch.setattr( + module.hermes_code_repair, "open_proposal_incidents", + lambda cfg: (calls.get("open_incidents", []), None), + ) return calls @@ -146,7 +150,7 @@ def test_a_sweep_opens_one_proposal_for_the_chosen_finding(wiring) -> None: assert result == {"proposed": 1, "skipped": [], "projects": 1} proposal = wiring["proposals"][0] - assert proposal["incident_id"] == "sonar/ariadne/AZ-1" + assert proposal["incident_id"] == "sonar/ariadne/python:S1172/AZ-1" assert proposal["job"] == "ariadne" assert proposal["build_number"] == "sonar-AZ-1" @@ -159,7 +163,7 @@ def test_the_bundle_offers_only_the_file_the_finding_names() -> None: assert bundle["sonarqube"] == {"project": "ariadne", "issues": [_issue()]} assert bundle["jenkins"]["console_failures"] == [] assert bundle["jenkins"]["console_tail"] == "" - assert bundle["incident_id"] == "sonar/ariadne/AZ-1" + assert bundle["incident_id"] == "sonar/ariadne/python:S1172/AZ-1" def test_the_sweep_budget_stops_after_its_quota(wiring) -> None: @@ -216,7 +220,7 @@ def test_a_project_with_nothing_mechanical_is_reported(wiring) -> None: result = module.sweep(_Storage(), _config(), {}) - assert result["skipped"] == ["ariadne: no mechanically fixable finding"] + assert result["skipped"] == ["ariadne: no new mechanically fixable finding"] def test_a_sweep_never_takes_down_the_scheduler(monkeypatch) -> None: @@ -360,3 +364,62 @@ def test_a_bundles_origin_decides_the_framing(bundle, expected) -> None: from ariadne.services import hermes_code_prompt assert hermes_code_prompt.is_quality_sweep(bundle) is expected + + +def test_a_rule_already_under_review_is_not_proposed_again(wiring) -> None: + """One rule is one root cause; thirty near-identical PRs get read as none.""" + + wiring["open_incidents"] = ["sonar/ariadne/python:S1172/AZ-OTHER"] + wiring["issues"] = [ + _issue(key="a", rule="python:S1172"), + _issue(key="b", rule="python:S2208", effort="10min"), + ] + + result = module.sweep(_Storage(), _config(), {}) + + assert result["proposed"] == 1 + assert wiring["proposals"][0]["incident_id"] == "sonar/ariadne/python:S2208/b" + + +def test_every_rule_under_review_means_nothing_new_to_propose(wiring) -> None: + wiring["open_incidents"] = ["sonar/ariadne/python:S1172/AZ-OTHER"] + wiring["issues"] = [_issue(rule="python:S1172")] + + result = module.sweep(_Storage(), _config(), {}) + + assert result["proposed"] == 0 + assert result["skipped"] == ["ariadne: no new mechanically fixable finding"] + + +def test_another_projects_open_proposal_does_not_block_this_one(wiring) -> None: + wiring["open_incidents"] = ["sonar/metis/python:S1172/AZ-X", "ariadne/408"] + + assert module.sweep(_Storage(), _config(), {})["proposed"] == 1 + + +def test_the_rules_under_review_are_read_from_the_open_proposals(monkeypatch) -> None: + monkeypatch.setattr( + module.hermes_code_repair, "open_proposal_incidents", + lambda cfg: ( + [ + "sonar/ariadne/python:S2208/AZ1", + "sonar/ariadne/python:S3776/AZ2", + "sonar/other/python:S1172/AZ3", + "ariadne/408", + "sonar/ariadne/malformed", + ], + None, + ), + ) + + assert module.proposed_rules(REPO_CFG, "ariadne") == {"python:S2208", "python:S3776"} + + +def test_an_unreadable_proposal_list_costs_a_duplicate_not_a_silent_rule(monkeypatch) -> None: + """Failing open here means one extra PR; failing closed loses a whole rule.""" + + monkeypatch.setattr( + module.hermes_code_repair, "open_proposal_incidents", lambda cfg: ([], "http 500") + ) + + assert module.proposed_rules(REPO_CFG, "ariadne") == set()