diff --git a/src/pr_af/hitl/__init__.py b/src/pr_af/hitl/__init__.py index 30c71a9..1595c78 100644 --- a/src/pr_af/hitl/__init__.py +++ b/src/pr_af/hitl/__init__.py @@ -17,8 +17,10 @@ ACTION_POST, ACTION_REJECT, ACTION_RERUN, + HAX_REVIEW_TEMPLATE, ReviewDecision, - build_review_form, + build_review_payload, + clean_intent, parse_review_decision, request_review_approval, ) @@ -27,10 +29,12 @@ "ACTION_POST", "ACTION_REJECT", "ACTION_RERUN", + "HAX_REVIEW_TEMPLATE", "ReviewDecision", "approval_webhook_url", "build_hax_client_from_env", - "build_review_form", + "build_review_payload", + "clean_intent", "create_hax_form_request_with_timeout", "extract_values_from_raw", "parse_review_decision", diff --git a/src/pr_af/hitl/client.py b/src/pr_af/hitl/client.py index d989df1..1c5ceb2 100644 --- a/src/pr_af/hitl/client.py +++ b/src/pr_af/hitl/client.py @@ -63,7 +63,8 @@ async def create_hax_form_request_with_timeout( *, app: Any, hax_client: HaxClient, - form: Any, + payload: dict[str, Any], + request_type: str, title: str, description: str | None, expires_in_seconds: int, @@ -72,21 +73,24 @@ async def create_hax_form_request_with_timeout( metadata: dict[str, Any] | None, timeout_seconds: float = HAX_CREATE_REQUEST_TIMEOUT_SECONDS, ) -> Any: - """Submit a hax form-builder request with a hard timeout. + """Submit a hax request of ``request_type`` with a hard timeout. Runs the synchronous ``hax_client.create_request`` in a worker thread under ``asyncio.wait_for`` so a wedged hax-sdk fails fast (``RuntimeError``) instead of silently burning the reasoner's active-time budget. Returns the ``CreatedRequest``; the caller passes ``.id`` / ``.url`` to ``app.pause``. + + ``payload`` must satisfy the registered hax template's schema — the hax + service validates it server-side and rejects unknown ``request_type`` values. """ app.note( - f"hitl: submitting hax form request ({title!r})", + f"hitl: submitting hax request ({request_type}: {title!r})", tags=["hitl", "hax", "create_request"], ) kwargs: dict[str, Any] = { - "type": "form-builder", - "payload": form.to_payload(), + "type": request_type, + "payload": payload, "title": title, "expires_in_seconds": expires_in_seconds, } diff --git a/src/pr_af/hitl/review_gate.py b/src/pr_af/hitl/review_gate.py index b82e04b..a78a3b8 100644 --- a/src/pr_af/hitl/review_gate.py +++ b/src/pr_af/hitl/review_gate.py @@ -14,6 +14,7 @@ from __future__ import annotations +import re from dataclasses import dataclass, field from typing import TYPE_CHECKING, Any @@ -28,18 +29,26 @@ from ..schemas.output import ScoredFinding -# Action chosen by the reviewer in the form's "action" radio. +# The hax template this gate renders into. Registered in hax-sdk under +# src/templates/pr-af-review (id "pr-af-review-v1"). The template emits the same +# ``{template, values: {...}}`` response envelope form-builder uses, so the +# decision parser below is template-agnostic. +HAX_REVIEW_TEMPLATE = "pr-af-review-v1" + +# Action chosen by the reviewer in the template's action buttons. These string +# values are the contract with the hax template's response `values.action`. ACTION_POST = "post_selected" ACTION_RERUN = "rerun" ACTION_REJECT = "reject" _VALID_ACTIONS = {ACTION_POST, ACTION_RERUN, ACTION_REJECT} -_SEVERITY_EMOJI = { - "critical": "🔴", - "important": "🟠", - "suggestion": "🔵", - "nitpick": "⚪", -} +# Longest PR-intent blurb shown in the form. The raw PR body (e.g. a Dependabot +# changelog) is stripped of HTML and truncated to this so the form stays legible. +_MAX_INTENT_CHARS = 700 + +_HTML_TAG_RE = re.compile(r"<[^>]+>") +_WS_RE = re.compile(r"[ \t]+") +_BLANKLINES_RE = re.compile(r"\n{3,}") @dataclass @@ -65,101 +74,91 @@ def is_reject(self) -> bool: return self.action == ACTION_REJECT -def _finding_option(finding: ScoredFinding) -> dict[str, str]: - """One checkbox option per finding: ``id`` is the value, label is human.""" - emoji = _SEVERITY_EMOJI.get(finding.severity, "•") - loc = finding.file_path or "(no file)" - if finding.line_start and finding.line_start > 0: - loc = f"{loc}:{finding.line_start}" - return { - "value": finding.id, - "label": f"{emoji} [{finding.severity}] {loc} — {finding.title}", - } - - -def _build_description( - pr_intent: str, - findings: list[ScoredFinding], - revision_iter: int, - revision_history: list[str], -) -> str: - """Markdown blurb shown above the form: PR intent + what's being asked.""" - counts: dict[str, int] = {} - for f in findings: - counts[f.severity] = counts.get(f.severity, 0) + 1 - count_str = ", ".join(f"{n} {sev}" for sev, n in counts.items()) or "no findings" +def clean_intent(text: str | None, max_chars: int = _MAX_INTENT_CHARS) -> str: + """Turn a raw PR body into a short, legible intent blurb. - lines = [] - if pr_intent: - lines.append("**PR intent:** " + pr_intent.strip()) - lines.append("") - lines.append( - f"PR-AF found **{len(findings)}** finding(s) ({count_str}). " - "Check the ones to post, or request a re-review with instructions." - ) - if revision_iter > 0: - lines.append("") - lines.append(f"_Revision round {revision_iter}._") - if revision_history: - lines.append("") - lines.append("Prior instructions:") - for idx, instr in enumerate(revision_history, start=1): - if instr: - lines.append(f"{idx}. {instr}") - return "\n".join(lines) - - -def build_review_form( + PR bodies (especially bot-authored ones like Dependabot) are often a wall of + HTML — ``
``, ````, changelog tables. The hax template + renders the intent as markdown, where raw HTML shows up literally, so we + strip tags, collapse whitespace, and truncate. + """ + if not text: + return "" + stripped = _HTML_TAG_RE.sub(" ", text) + stripped = _WS_RE.sub(" ", stripped) + stripped = _BLANKLINES_RE.sub("\n\n", stripped) + stripped = "\n".join(line.strip() for line in stripped.splitlines()) + stripped = stripped.strip() + if len(stripped) > max_chars: + stripped = stripped[:max_chars].rstrip() + "…" + return stripped + + +def _finding_payload(finding: ScoredFinding) -> dict[str, Any]: + """One finding entry for the hax template (camelCase per its zod schema).""" + entry: dict[str, Any] = { + "id": finding.id, + "severity": finding.severity, + "title": finding.title, + # All findings start checked, matching the prior "submit posts all" default. + "defaultSelected": True, + } + if finding.file_path: + entry["filePath"] = finding.file_path + if finding.line_start and finding.line_start > 0: + entry["lineStart"] = finding.line_start + if finding.line_end and finding.line_end > 0: + entry["lineEnd"] = finding.line_end + if finding.body: + entry["body"] = finding.body + if finding.suggestion: + entry["suggestion"] = finding.suggestion + if finding.dimension_name: + entry["dimension"] = finding.dimension_name + if finding.confidence is not None: + entry["confidence"] = finding.confidence + return entry + + +def build_review_payload( *, pr_intent: str, findings: list[ScoredFinding], title: str, + pr_meta: dict[str, Any] | None = None, revision_iter: int = 0, revision_history: list[str] | None = None, -) -> Any: - """Translate the review into a ``hax.FormBuilder`` (imported lazily).""" - from hax import FormBuilder - - all_ids = [f.id for f in findings] - options = [_finding_option(f) for f in findings] - - form = ( - FormBuilder() - .title(title) - .description( - _build_description(pr_intent, findings, revision_iter, revision_history or []) - ) - .submit_label("Submit decision") - ) +) -> dict[str, Any]: + """Build the ``pr-af-review-v1`` request payload (validated server-side). - # checkbox_group requires at least one option; skip it when there are no - # findings (an empty review still lets the reviewer approve/reject). - if options: - form.checkbox_group( - "findings_to_post", - label="Findings to post", - description="Only the checked findings are posted to the PR.", - options=options, - default_value=all_ids, - ) + Shape matches ``prAfReviewPayloadSchema`` in the hax-sdk template. + """ + counts: dict[str, int] = {} + for f in findings: + counts[f.severity] = counts.get(f.severity, 0) + 1 + count_str = ", ".join(f"{n} {sev}" for sev, n in counts.items()) or "no findings" - form.radio_group( - "action", - label="Action", - options=[ - {"value": ACTION_POST, "label": "Post selected findings"}, - {"value": ACTION_RERUN, "label": "Re-review with instructions below"}, - {"value": ACTION_REJECT, "label": "Reject — post nothing"}, - ], - default_value=ACTION_POST, - ) - form.textarea( - "instructions", - label="Re-review instructions (optional)", - description="Used when action is 'Re-review'. E.g. 'too aggressive, tone it down'.", - required=False, - ) - return form + payload: dict[str, Any] = { + "title": title, + "intent": clean_intent(pr_intent), + "reviewSummary": f"PR-AF found {len(findings)} finding(s) ({count_str}).", + "findings": [_finding_payload(f) for f in findings], + "postLabel": "Post selected", + "rerunLabel": "Re-review with instructions", + "rejectLabel": "Reject", + "instructionsPlaceholder": "e.g. too aggressive, tone it down and drop the nitpicks", + } + if pr_meta: + # Drop empties so optional zod fields stay absent rather than "". + cleaned = {k: v for k, v in pr_meta.items() if v not in (None, "")} + if cleaned: + payload["pr"] = cleaned + if revision_iter > 0 or revision_history: + payload["revision"] = { + "iteration": revision_iter, + "priorInstructions": [i for i in (revision_history or []) if i and i.strip()], + } + return payload def _coerce_str(value: Any) -> str: @@ -229,11 +228,12 @@ async def request_review_approval( webhook_url: str | None, user_id: str | None, expires_in_hours: int, + pr_meta: dict[str, Any] | None = None, revision_iter: int = 0, revision_history: list[str] | None = None, metadata: dict[str, Any] | None = None, ) -> ReviewDecision: - """Build the form, create the hax request, pause, return the decision. + """Build the payload, create the hax request, pause, return the decision. Any failure to create the request or pause is surfaced as a reject so the pipeline never posts an unreviewed review when the gate is enabled. @@ -245,25 +245,27 @@ async def request_review_approval( title = f"{title} — {pr_label}" try: - form = build_review_form( + payload = build_review_payload( pr_intent=pr_intent, findings=findings, title=title, + pr_meta=pr_meta, revision_iter=revision_iter, revision_history=revision_history, ) except Exception as exc: app.note( - f"hitl: failed to build review form: {exc}", - tags=["hitl", "form", "error"], + f"hitl: failed to build review payload: {exc}", + tags=["hitl", "payload", "error"], ) - return ReviewDecision(action=ACTION_REJECT, instructions=f"form build failed: {exc}") + return ReviewDecision(action=ACTION_REJECT, instructions=f"payload build failed: {exc}") try: created = await create_hax_form_request_with_timeout( app=app, hax_client=hax_client, - form=form, + payload=payload, + request_type=HAX_REVIEW_TEMPLATE, title=title, description=None, expires_in_seconds=expires_in_hours * 3600, diff --git a/src/pr_af/orchestrator.py b/src/pr_af/orchestrator.py index 38b6035..c9d8451 100644 --- a/src/pr_af/orchestrator.py +++ b/src/pr_af/orchestrator.py @@ -161,12 +161,23 @@ async def run(self) -> ReviewResult: print("[PR-AF] Phase 8: OUTPUT (direct post)", flush=True) return await self._finish(scored_findings, intake, anatomy, plan, post=True) + # Nothing to triage: don't bother a human (and don't auto-post an + # empty "approved" review to a public repo). Complete silently. + if not scored_findings: + self.app.note( + "hitl: no findings — skipping review gate, posting nothing", + tags=["hitl", "no-post", "no-findings"], + ) + print("[PR-AF] HITL: no findings — skipping gate, not posting", flush=True) + return await self._finish(scored_findings, intake, anatomy, plan, post=False) + decision = await request_review_approval( app=self.app, hax_client=hax_client, pr_intent=intake.pr_summary, findings=scored_findings, pr_label=self._pr_label(), + pr_meta=self._pr_meta(), webhook_url=approval_webhook_url(self.app), user_id=self.config.hitl.approval_user_id, expires_in_hours=self.config.hitl.approval_expires_in_hours, @@ -273,6 +284,28 @@ def _pr_label(self) -> str: return f"{self.pr_data.owner}/{self.pr_data.repo}#{self.pr_data.number}" return "" + def _pr_meta(self) -> dict[str, Any]: + """PR metadata block for the hax review template (camelCase keys).""" + pr = self.pr_data + if not pr: + return {} + url = self.input.pr_url or "" + if not url and pr.owner and pr.repo and pr.number: + url = f"https://github.com/{pr.owner}/{pr.repo}/pull/{pr.number}" + repo = f"{pr.owner}/{pr.repo}" if pr.owner and pr.repo else (pr.repo or "") + meta: dict[str, Any] = { + "title": pr.title or "", + "number": pr.number or None, + "url": url, + "repo": repo, + "author": pr.author or "", + } + if pr.changed_files: + meta["filesChangedCount"] = len(pr.changed_files) + meta["additionsCount"] = sum(f.additions for f in pr.changed_files) + meta["deletionsCount"] = sum(f.deletions for f in pr.changed_files) + return meta + def _merge_feedback(self, revision_history: list[str]) -> str: """Collapse accumulated reviewer instructions into one guidance string.""" items = [instr.strip() for instr in revision_history if instr and instr.strip()] diff --git a/tests/test_hitl_review_gate.py b/tests/test_hitl_review_gate.py index ad281e3..0e8d8f6 100644 --- a/tests/test_hitl_review_gate.py +++ b/tests/test_hitl_review_gate.py @@ -16,8 +16,10 @@ ACTION_POST, ACTION_REJECT, ACTION_RERUN, + HAX_REVIEW_TEMPLATE, build_hax_client_from_env, - build_review_form, + build_review_payload, + clean_intent, parse_review_decision, ) from pr_af.hitl.client import create_hax_form_request_with_timeout @@ -58,32 +60,51 @@ def test_build_hax_client_returns_client_with_api_key(monkeypatch): assert client is not None -# --- form shape ---------------------------------------------------------- +# --- payload shape ------------------------------------------------------- -def test_form_has_one_checkbox_option_per_finding_plus_action_and_textarea(): +def test_payload_has_one_finding_entry_per_finding_all_default_selected(): findings = [_finding("f1"), _finding("f2", severity="nitpick"), _finding("f3")] - form = build_review_form(pr_intent="adds caching", findings=findings, title="t") - payload = form.to_payload() - fields = {f["id"]: f for f in payload["fields"]} + payload = build_review_payload(pr_intent="adds caching", findings=findings, title="t") + + assert [f["id"] for f in payload["findings"]] == ["f1", "f2", "f3"] + # All findings pre-checked by default so "post selected" posts everything. + assert all(f["defaultSelected"] is True for f in payload["findings"]) + # Severity + location are carried through for the rich UI. + assert payload["findings"][0]["severity"] == "important" + assert payload["findings"][0]["filePath"] == "src/foo.py" + assert payload["findings"][0]["lineStart"] == 10 + assert payload["intent"] == "adds caching" + + +def test_payload_carries_pr_meta_and_revision(): + payload = build_review_payload( + pr_intent="adds caching", + findings=[_finding("f1")], + title="t", + pr_meta={"repo": "o/r", "number": 7, "author": "", "url": ""}, + revision_iter=1, + revision_history=["tone it down", ""], + ) + # Empty meta values are dropped so optional zod fields stay absent. + assert payload["pr"] == {"repo": "o/r", "number": 7} + assert payload["revision"]["iteration"] == 1 + assert payload["revision"]["priorInstructions"] == ["tone it down"] + - assert set(fields) == {"findings_to_post", "action", "instructions"} - options = fields["findings_to_post"]["options"] - assert [o["value"] for o in options] == ["f1", "f2", "f3"] - # All findings pre-checked by default so "submit as-is" posts everything. - assert fields["findings_to_post"]["defaultValue"] == ["f1", "f2", "f3"] - assert {o["value"] for o in fields["action"]["options"]} == { - ACTION_POST, - ACTION_RERUN, - ACTION_REJECT, - } +def test_payload_handles_no_findings(): + payload = build_review_payload(pr_intent="docs only", findings=[], title="t") + assert payload["findings"] == [] + assert "no findings" in payload["reviewSummary"] -def test_form_omits_checkbox_group_when_no_findings(): - form = build_review_form(pr_intent="docs only", findings=[], title="t") - field_ids = {f["id"] for f in form.to_payload()["fields"]} - assert "findings_to_post" not in field_ids - assert "action" in field_ids # reviewer can still approve/reject +def test_clean_intent_strips_html_and_truncates(): + raw = "
Release notes

Bumps pkg

" + cleaned = clean_intent(raw) + assert "<" not in cleaned and ">" not in cleaned + assert "Release notes" in cleaned + assert clean_intent("x" * 5000) .endswith("…") + assert len(clean_intent("x" * 5000)) <= 701 # max_chars + ellipsis # --- decision parsing ---------------------------------------------------- @@ -161,7 +182,8 @@ def create_request(self, **kwargs): await create_hax_form_request_with_timeout( app=_FakeApp(), hax_client=_WedgedHax(), - form=SimpleNamespace(to_payload=lambda: {"fields": []}), + payload={"findings": []}, + request_type=HAX_REVIEW_TEMPLATE, title="t", description=None, expires_in_seconds=3600, diff --git a/tests/test_orchestrator_hitl.py b/tests/test_orchestrator_hitl.py index b6a60e8..b921e6d 100644 --- a/tests/test_orchestrator_hitl.py +++ b/tests/test_orchestrator_hitl.py @@ -12,8 +12,6 @@ from types import SimpleNamespace -import pytest - import pr_af.orchestrator as orch_mod from pr_af.hitl import ACTION_POST, ACTION_REJECT, ACTION_RERUN, ReviewDecision from pr_af.orchestrator import ReviewOrchestrator @@ -138,6 +136,15 @@ async def test_rerun_then_post_threads_feedback(monkeypatch): assert captured["post"] is True +async def test_hitl_on_zero_findings_skips_gate(monkeypatch): + orch, captured, feedbacks, calls = _make_orchestrator( + monkeypatch, findings=[], hitl_on=True, decisions=[] + ) + await orch.run() + assert captured["post"] is False # nothing posted to a public repo + assert calls["n"] == 0 # human never bothered for an empty review + + async def test_reject_posts_nothing(monkeypatch): findings = [_finding("f1")] decision = ReviewDecision(action=ACTION_REJECT, decision_raw="rejected")