diff --git a/docs/noema-agent-sandbox-plan.md b/docs/noema-agent-sandbox-plan.md index f2e9bdff2..7324945ab 100644 --- a/docs/noema-agent-sandbox-plan.md +++ b/docs/noema-agent-sandbox-plan.md @@ -51,10 +51,17 @@ The driver returns JSON: "findings": [ { "severity": "critical | high | medium | low | info", + "priority": "P1 | P2 | P3", "path": "relative/path", "line": 1, - "evidence": "log, SARIF, test, or source reference", - "recommendation": "specific fix" + "check_name": "exact current-head failed check name | null", + "evidence": "log, SARIF, test, source, or other independently checkable reference", + "evidence_type": "nearby_implementation | matching_existing_example | cross_file_counterpart | current_official_docs | failed_check_or_log", + "observable_impact": "specific user or operator consequence", + "trigger": "concrete condition that exposes the issue", + "recommendation": "smallest specific fix", + "regression_command": "one exact single-line command or test target", + "suggested_diff": "optional replacement text | null" } ], "suggested_patch_ref": "optional artifact path or branch", @@ -63,6 +70,22 @@ The driver returns JSON: } ``` +`check_name` is optional for ordinary source, SARIF, dependency, and review-thread +findings. When a finding is offered as the causal RCA for a failed current-head +check, it must equal that exact check name. A failed check remains `blocked` +unless it has its own blocking-severity finding on a current-head changed path +with a positive source line; one finding cannot authorize multiple failed +checks. + +Every finding is actionable data rather than prose-only advice. Priority, +evidence type, observable impact, trigger, smallest fix, and an exact regression +command are required. A `regression_command` cannot contain a newline or Markdown +backtick. `suggested_diff` is optional, but when present it cannot contain a +Markdown fence and must anchor to a right-side line in the exact PR diff before +publication. Valid replacement text is published through GitHub's inline review +`comments` payload as a suggestion rather than only being displayed in the +top-level review body. + Noema-issued installation tokens are used only after the sandboxed agent has a bounded verdict to publish. The token scope is limited to the target repository and central review workflow permissions. @@ -150,6 +173,12 @@ failure and blocks strict approval. a failure came from missing evidence, dependency vulnerability, image verification, image vulnerability, CodeGraph failure, sandbox timeout, attestation creation/verification, model exhaustion, or GitHub API rejection. +- Each ordinary failed current-head check either has its own exact-name, + changed-path, positive-line blocking RCA or keeps the verdict `blocked`; + another failed check's finding cannot satisfy that evidence requirement. +- Each finding carries priority, evidence type, observable impact, trigger, + smallest fix, and one exact regression command; any proposed replacement text + must be fence-safe and exact-diff-anchorable before GitHub receives it. - Medium-or-higher dependency and sandbox-image findings from OSV, Trivy, and dependency-review are remediated by package/image bump or source change, not by gate weakening. @@ -186,10 +215,12 @@ privileged publication plane. The judgement plane is implemented as the Python package `reviewer/noema_reviewer` (a PydanticAI `ReviewAgent` driver). It returns the -JSON verdict contract above, enforces strict-evidence blocking and -MEDIUM-or-higher dependency downgrade around the model, preserves reviewed PR -comments and current check conclusions, records containerized CodeGraph status, -and publishes only against the live exact head after attested manifest -verification. The Noema Worker (`src/`) remains the token-exchange boundary -only. Reviewer code ships with 100% line and branch coverage and 100% docstring -coverage; the Worker release gate remains `npm run release:verify`. \ No newline at end of file +JSON verdict contract above, enforces strict-evidence blocking, exact per-check +failed-check RCA binding, actionable finding validation, exact-diff suggestion +anchoring, and MEDIUM-or-higher dependency downgrade around the model. It +preserves reviewed PR comments and current check conclusions, records +containerized CodeGraph status, and publishes only against the live exact head +after attested manifest verification. The Noema Worker (`src/`) remains the +token-exchange boundary only. Reviewer code is required to retain 100% line and +branch coverage and 100% docstring coverage; the Worker release gate remains +`npm run release:verify`. diff --git a/reviewer/README.md b/reviewer/README.md index 851a0a426..71a6eda00 100644 --- a/reviewer/README.md +++ b/reviewer/README.md @@ -17,19 +17,46 @@ Division of responsibility: ## Contract -The verdict shape is the JSON contract from the sandbox plan: +The verdict shape is the JSON contract from the sandbox plan. Each finding +carries structured actionability rather than relying on free-form prose: ```json { "verdict": "approve | request_changes | blocked", "summary": "…", - "findings": [{"severity": "critical|high|medium|low|info", "path": "…", "line": 1, "evidence": "…", "recommendation": "…"}], + "findings": [{ + "severity": "critical|high|medium|low|info", + "priority": "P1|P2|P3", + "path": "…", + "line": 1, + "check_name": "exact failed check name | null", + "evidence": "…", + "evidence_type": "nearby_implementation|matching_existing_example|cross_file_counterpart|current_official_docs|failed_check_or_log", + "observable_impact": "…", + "trigger": "…", + "recommendation": "smallest fix", + "regression_command": "one exact single-line command", + "suggested_diff": "optional replacement text | null" + }], "suggested_patch_ref": null, "blocked_reasons": [], "confidence": "high | medium | low" } ``` +`check_name` is optional for ordinary source, SARIF, dependency, and review-thread +findings. A finding offered as the RCA for a failed current-head check must bind +to that exact check name. The deterministic gate requires each ordinary failed +check to have its own blocking-severity finding on a current-head changed path +with a positive line; one unrelated or differently bound finding cannot clear +another failed check. + +`regression_command` cannot contain newlines or Markdown backticks. A +`suggested_diff` cannot contain a Markdown fence and is accepted only when its +`path:line` is a right-side anchor in the exact PR diff. Accepted replacement +text is sent through GitHub's inline review `comments` payload as a suggestion, +not merely printed in the top-level review body. + The following guarantees are enforced deterministically around the LLM (`gating.py`), so they hold regardless of what the model says: @@ -70,7 +97,7 @@ The following guarantees are enforced deterministically around the LLM a repository probe. The primary explore query preserves each selected changed path in full instead of truncating individual path identities; it admits at most 80 changed files and 24,079 aggregate characters. The manifest retains - bounded current-head file content for every selected file through that same + bounded current-head file context for every selected file through that same 80-file canonical scope; above 80 files both semantic scope and changed-file context fail closed rather than reviewing a historical 12-file prefix. Exceeding either exact-scope budget fails closed instead of querying a prefix. @@ -91,40 +118,46 @@ The following guarantees are enforced deterministically around the LLM unchanged lookalike path become a retrieval seed. The node output never counts as review evidence by itself; deleted, unresolved, symlinked-component, unindexed, or symbol-less paths leave the original empty result fail closed. - The local host-process CodeGraph fallback also builds a closed execution - environment instead of copying the parent environment: only `PATH` and locale - discovery variables may be propagated; `HOME`, `TEMP`, `TMP`, and `TMPDIR` - are replaced by one fresh per-command private temporary directory and - `NO_COLOR=1` is set explicitly. Process injection, host user configuration/ - credentials, ambient temporary-directory capabilities, credential-helper/ - socket, container/Kubernetes, proxy, arbitrary workflow, and provider - variables such as `NODE_OPTIONS`, `GIT_ASKPASS`, `SSH_AUTH_SOCK`, + The local host-process CodeGraph fallback builds a closed execution + environment instead of copying the parent environment: only `PATH` and + locale discovery variables may be propagated; `HOME`, `TEMP`, `TMP`, and + `TMPDIR` are replaced by one fresh per-command private temporary directory + and `NO_COLOR=1` is set explicitly. Process injection, host user + configuration/credentials, ambient temporary-directory capabilities, + credential-helper/socket, container/Kubernetes, proxy, arbitrary workflow, + and provider variables such as `NODE_OPTIONS`, `GIT_ASKPASS`, `SSH_AUTH_SOCK`, `DOCKER_CONFIG`, `KUBECONFIG`, and `HTTPS_PROXY` are not ambient CodeGraph - authority. Production central review still uses the separately attested no- - network sandbox; this host fallback does not replace that isolation boundary. - The production `DockerCodeGraphRunner` now owns the same semantic wrapper and - passes both the exact symbol probe and any symbol-seeded second `explore` - through its verified no-network container boundary. It extracts only the - trusted sandbox copy receipt and sole explore stdout section before semantic - classification, so setup/status bytes cannot satisfy the strict gate and an - empty production explore cannot silently fall back to a host CodeGraph + authority. Production central review still uses the separately attested + no-network sandbox; this host fallback does not replace that isolation + boundary. The production `DockerCodeGraphRunner` owns the same semantic + wrapper and passes both the exact symbol probe and any symbol-seeded second + `explore` through its verified no-network container boundary. It extracts + only the trusted sandbox copy receipt and sole explore stdout section before + semantic classification, so setup/status bytes cannot satisfy the strict gate + and an empty production explore cannot silently fall back to a host CodeGraph process. 2. **MEDIUM-or-higher dependency findings can't ride out on an approve.** An unresolved OSV/Trivy/dependency-review finding at MEDIUM+ downgrades an approval to `request_changes` with the finding attached — the org rule is "remediate by bump, not gate weakening". -3. **Current-head failures remain blocking.** Failed GitHub Checks and - MEDIUM-or-higher code-scanning/SARIF alerts deterministically downgrade an - approval and retain their exact job, rule, path, and bounded log evidence. -4. **Reviewer independence cannot deadlock.** The exact reviewer check names +3. **Current-head failures remain blocking until causally mapped.** Every + ordinary failed GitHub Check remains `blocked` unless its exact check name is + bound to its own current-head changed-file, positive-line blocking RCA. + Check-run names or workflow URLs are not synthesized into source findings. + MEDIUM-or-higher code-scanning/SARIF alerts remain deterministic findings. +4. **Suggestions must be executable review artifacts.** Suggested replacement + text is rejected before publication if GitHub cannot attach it to the exact + right side of the reviewed diff; fence injection and multiline regression + commands fail schema validation. +5. **Reviewer independence cannot deadlock.** The exact reviewer check names `noema-review` and `opencode-review`, plus the downstream - `metadata-only gate evaluation`, are excluded from Noema's deterministic - failed-check gate because they cannot be prerequisites for the review that - produces them. This cycle exception cannot satisfy strict evidence by itself: - at least one current-head check outside that reviewer-dependent set must be - observed. Similarly named checks remain blocking, as do every other failed - check and unresolved non-outdated inline thread. -5. **Long reviews stay useful.** The production provider request timeout + `metadata-only gate evaluation`, are excluded from Noema's failed-check RCA + gate because they cannot be prerequisites for the review that produces them. + This cycle exception cannot satisfy strict evidence by itself: at least one + current-head check outside that reviewer-dependent set must be observed. + Similarly named checks remain blocking, as do every other failed check and + unresolved non-outdated inline thread. +6. **Long reviews stay useful.** The production provider request timeout defaults to 5,400 seconds and provider 429/5xx responses receive bounded SDK retries. Production failover belongs inside `contextual-orchestrator`; Noema does not sequentially try the next model. Publication re-reads the live PR @@ -133,8 +166,11 @@ The following guarantees are enforced deterministically around the LLM The GitHub manifest fetch covers all inline review threads (including resolved and outdated state), submitted review bodies, conversation comments, failed current-head workflow logs, current-head code-scanning alerts, and open -Dependabot package advisories. Evidence-fetch errors are part of the manifest, -not silent empty lists. +Dependabot package advisories. Failed-check log collection derives an Actions +Job id only from an exact repository-bound GitHub `details_url`; a Check Run id +is never reused as a Job id. If the Actions log cannot be obtained, collection +falls back to the same Check Run's bounded annotations. Evidence-fetch errors +are part of the manifest, not silent empty lists. The driver sits behind the small `ReviewAgent` protocol, so the sandbox plan's "Codex, OpenCode, PydanticAI, or another driver" swap is a one-line change. diff --git a/reviewer/noema_reviewer/__init__.py b/reviewer/noema_reviewer/__init__.py index 02e6bb78f..36cdca00b 100644 --- a/reviewer/noema_reviewer/__init__.py +++ b/reviewer/noema_reviewer/__init__.py @@ -12,7 +12,7 @@ from .agent import PydanticAIReviewAgent, ReviewAgent, build_agent from .manifest import ReviewManifest -from .models import Confidence, Finding, ReviewVerdict, Severity, Verdict +from .models import Confidence, EvidenceType, Finding, Priority, ReviewVerdict, Severity, Verdict from .patch_image_validation import ( DockerPatchValidatorImageRunner, PatchValidatorImageProfile, @@ -35,6 +35,7 @@ "Confidence", "DockerPatchValidationRunner", "DockerPatchValidatorImageRunner", + "EvidenceType", "Finding", "PatchValidationProfile", "PatchValidationRequest", @@ -45,6 +46,7 @@ "PatchValidatorImageResult", "PatchValidatorImageStatus", "PydanticAIReviewAgent", + "Priority", "ReviewAgent", "ReviewManifest", "ReviewVerdict", diff --git a/reviewer/noema_reviewer/agent.py b/reviewer/noema_reviewer/agent.py index dc7d24b7a..7e49901b2 100644 --- a/reviewer/noema_reviewer/agent.py +++ b/reviewer/noema_reviewer/agent.py @@ -30,9 +30,16 @@ "regressions from that evidence only. Approve when no blocking issue is " "supported by the evidence. Use request_changes only for concrete, " "evidence-backed blocking issues, and cite the log, SARIF, test, or source " - "line for each finding. Use blocked when required evidence is missing rather " - "than guessing. Never approve while an unresolved MEDIUM-or-higher " - "dependency finding is present; require a package bump instead." + "line for each finding. For every failed check, read its current-head log or " + "annotation, trace the failure to an exact repository path and positive line, " + "set finding.check_name to that exact current-head check name, and state " + "P1/P2/P3 priority, evidence type, observable impact, trigger, smallest fix, " + "and an exact regression command in the finding. Include minimal replacement " + "text in suggested_diff when the cited line can be fixed directly; one finding " + "must not stand in for multiple failed checks. A check name, workflow URL, or " + "synthetic .github/checks path is not actionable. Use blocked when logs cannot " + "support that mapping rather than guessing. Never approve while an unresolved " + "MEDIUM-or-higher dependency finding is present; require a package bump instead." ) diff --git a/reviewer/noema_reviewer/gating.py b/reviewer/noema_reviewer/gating.py index 76dbc4ea7..b8f699fb6 100644 --- a/reviewer/noema_reviewer/gating.py +++ b/reviewer/noema_reviewer/gating.py @@ -1,23 +1,29 @@ """Deterministic safety gates applied around the LLM review. -The LLM driver produces a judgement, but two guarantees from the sandbox plan's -Acceptance Criteria must hold regardless of what the model says, so they are -enforced here in plain, testable code rather than trusted to the prompt: +The LLM driver produces a judgement, but repository guarantees from the sandbox +plan's Acceptance Criteria must hold regardless of what the model says, so they +are enforced here in plain, testable code rather than trusted to the prompt: 1. Manual **strict** runs fail (``blocked``) when required evidence is missing, naming exactly what was missing — never a silent pass. 2. An unresolved MEDIUM-or-higher dependency finding can never ride out on an ``approve``; it is downgraded to ``request_changes`` with the finding attached, because the org rule is "remediate by bump, not gate weakening". +3. Every ordinary failed current-head check needs its own source-bound RCA before + the reviewer may publish ``request_changes`` instead of ``blocked``. """ from __future__ import annotations +import re + from .manifest import ReviewManifest from .models import ( BLOCKING_SEVERITIES, Confidence, + EvidenceType, Finding, + Priority, ReviewVerdict, Severity, Verdict, @@ -33,6 +39,43 @@ REVIEW_DEPENDENT_CHECK_NAMES = frozenset( {"noema-review", "opencode-review", "metadata-only gate evaluation"} ) +HUNK_HEADER_RE = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@") + + +def _right_side_diff_lines(diff: str) -> set[tuple[str, int]]: + """Return right-side path/line anchors accepted by GitHub review comments.""" + anchors: set[tuple[str, int]] = set() + path: str | None = None + line_number: int | None = None + for line in diff.splitlines(): + if line.startswith("+++ b/"): + path = line[6:] + line_number = None + continue + hunk = HUNK_HEADER_RE.match(line) + if hunk: + line_number = int(hunk.group(1)) + continue + if path is None or line_number is None or not line: + continue + if line[0] in {" ", "+"}: + anchors.add((path, line_number)) + line_number += 1 + elif line[0] != "-": + line_number = None + return anchors + + +def invalid_suggestion_reasons(manifest: ReviewManifest, verdict: ReviewVerdict) -> list[str]: + """Reject suggestions GitHub cannot attach to this exact PR diff.""" + anchors = _right_side_diff_lines(manifest.diff) + return [ + "suggested diff is not anchored to a current-head right-side diff line: " + f"{finding.path}:{finding.line or 'missing'}" + for finding in verdict.findings + if finding.suggested_diff and (finding.path, finding.line) not in anchors + ] + CODEGRAPH_EXPLORE_MARKER = "## codegraph explore" RAW_CODEGRAPH_EXPLORE_MARKER = "[raw codegraph explore marker]" @@ -121,24 +164,12 @@ def missing_evidence(manifest: ReviewManifest) -> list[str]: token for line in classification_lines for token in line.split() ) if not codegraph_status: - # A blank/whitespace status is not evidence; treat it as missing so a - # malformed artifact cannot pass strict mode silently (mirrors the diff - # check above and the field's own "not supplied" default semantics). reasons.append("missing CodeGraph evidence") elif codegraph_status_lower.startswith("unavailable"): reasons.append(manifest.codegraph_status) elif explore_marker_count > 1: - # The production wrapper emits exactly one provenance marker. A second - # marker can only come from untrusted output or a malformed prepared - # manifest, so strict review cannot choose which section is authoritative. reasons.append("CodeGraph semantic query has ambiguous provenance") elif normalized_final_explore.startswith("no relevant code found"): - # Classify the explicit CodeGraph empty-result response only when it is - # the semantic response prefix after known lifecycle and wrapper - # annotations are removed. Source/code context may legitimately contain - # the same words and must not erase independently retained semantic bytes. - # Collapse every Unicode whitespace run first so formatting cannot - # disguise the actual empty-result response. reasons.append("CodeGraph semantic query returned no relevant code") elif not _has_semantic_codegraph_context(manifest): reasons.append("CodeGraph semantic query produced no review context") @@ -168,12 +199,17 @@ def dependency_findings_as_review(manifest: ReviewManifest) -> list[Finding]: findings.append( Finding( severity=dependency.severity, + priority=Priority.P1 if dependency.severity is Severity.CRITICAL else Priority.P2, path=dependency.package_name, evidence=( f"{dependency.tool} reported {dependency.package_name}" f"@{dependency.installed_version or 'current'}{identifier}" ), + evidence_type=EvidenceType.FAILED_CHECK, + observable_impact="The pull request would retain a known vulnerable dependency.", + trigger="Installing the dependency set recorded by the current lockfile.", recommendation=f"Bump {dependency.package_name} to {fixed} and refresh the lockfile.", + regression_command="uv run pip-audit", ) ) return findings @@ -188,31 +224,53 @@ def security_findings_as_review(manifest: ReviewManifest) -> list[Finding]: findings.append( Finding( severity=security.severity, + priority=(Priority.P1 if security.severity in {Severity.CRITICAL, Severity.HIGH} else Priority.P2), path=security.path or ".github/code-scanning", line=security.line, evidence=( f"{security.tool} reported {security.identifier}: {security.message}" + (f" ({security.url})" if security.url else "") ), + evidence_type=EvidenceType.FAILED_CHECK, + observable_impact="The current-head security gate remains failed.", + trigger=f"Running the {security.tool} scanner against the current head.", recommendation="Remediate the current-head scanner finding and rerun code scanning.", + regression_command="gh pr checks --watch", ) ) return findings -def failed_checks_as_review(manifest: ReviewManifest) -> list[Finding]: - """Convert every observed non-success current-head check into a review finding.""" - return [ - Finding( - severity=Severity.HIGH, - path=f".github/checks/{check.name}", - evidence=f"Current-head check concluded {check.conclusion}; see bounded workflow_logs.", - recommendation="Require terminal success for the current-head check before approval.", - ) +def failed_check_blockers( + manifest: ReviewManifest, + verdict: ReviewVerdict | None = None, +) -> list[str]: + """Return failed checks without their own actionable current-head source RCA.""" + failed = [ + check.name for check in manifest.check_conclusions if check.name not in REVIEW_DEPENDENT_CHECK_NAMES and check.conclusion.lower() != "success" ] + if verdict is None: + unresolved = failed + else: + changed_paths = {changed.path for changed in manifest.changed_files} + actionable_checks = { + finding.check_name + for finding in verdict.findings + if finding.check_name is not None + and finding.severity in BLOCKING_SEVERITIES + and finding.path in changed_paths + and isinstance(finding.line, int) + and not isinstance(finding.line, bool) + and finding.line > 0 + } + unresolved = [name for name in failed if name not in actionable_checks] + return [ + f"failed check {name} lacks an actionable current-head path:line finding" + for name in unresolved + ] def unresolved_threads_as_review(manifest: ReviewManifest) -> list[Finding]: @@ -220,10 +278,15 @@ def unresolved_threads_as_review(manifest: ReviewManifest) -> list[Finding]: return [ Finding( severity=Severity.HIGH, + priority=Priority.P1, path=comment.path or ".github/review-threads", line=comment.line, evidence=f"Unresolved review thread by {comment.author}: {comment.body}", + evidence_type=EvidenceType.NEARBY_IMPLEMENTATION, + observable_impact="The current head retains a reviewer-confirmed defect.", + trigger="Merging while the current inline review thread remains unresolved.", recommendation="Resolve the cited review thread with a current-head fix or response.", + regression_command="gh pr checks --watch", ) for comment in manifest.review_comments if comment.kind == "thread" and comment.state == "open" @@ -238,25 +301,10 @@ def _enforce_findings( """Merge distinct deterministic findings and prevent an approval from hiding them.""" if not findings or verdict.verdict is Verdict.BLOCKED: return verdict - existing = { - ( - finding.severity, - finding.path, - finding.line, - finding.evidence, - finding.recommendation, - ) - for finding in verdict.findings - } + existing = {finding.model_dump_json() for finding in verdict.findings} merged = list(verdict.findings) for finding in findings: - identity = ( - finding.severity, - finding.path, - finding.line, - finding.evidence, - finding.recommendation, - ) + identity = finding.model_dump_json() if identity not in existing: merged.append(finding) existing.add(identity) @@ -277,11 +325,7 @@ def enforce_security_and_check_gates( verdict: ReviewVerdict, ) -> ReviewVerdict: """Block approvals on current-head non-success checks or MEDIUM+ SARIF findings.""" - deterministic = ( - failed_checks_as_review(manifest) - + security_findings_as_review(manifest) - + unresolved_threads_as_review(manifest) - ) + deterministic = security_findings_as_review(manifest) + unresolved_threads_as_review(manifest) return _enforce_findings( verdict, deterministic, @@ -316,9 +360,15 @@ def apply_gates( The dependency gate always runs so an approval can never bury an unresolved MEDIUM-or-higher vulnerability. """ + suggestion_reasons = invalid_suggestion_reasons(manifest, verdict) + if suggestion_reasons: + return blocked_verdict(suggestion_reasons) if strict: reasons = missing_evidence(manifest) if reasons: return blocked_verdict(reasons) + failed_checks = failed_check_blockers(manifest, verdict) + if failed_checks: + return blocked_verdict(failed_checks) check_gated = enforce_security_and_check_gates(manifest, verdict) return enforce_dependency_gate(manifest, check_gated) diff --git a/reviewer/noema_reviewer/github_io.py b/reviewer/noema_reviewer/github_io.py index 557edfa5b..c2c5dccb2 100644 --- a/reviewer/noema_reviewer/github_io.py +++ b/reviewer/noema_reviewer/github_io.py @@ -15,7 +15,7 @@ import subprocess import tempfile from collections.abc import Callable, Sequence -from urllib.parse import quote +from urllib.parse import quote, urlparse from .manifest import ( ChangedFile, @@ -423,7 +423,7 @@ def _fetch_failed_workflow_logs(repo: str, head_sha: str, runner: GhRunner) -> s '.check_runs[] | select(.conclusion == "failure" or ' '.conclusion == "cancelled" or .conclusion == "timed_out" or ' '.conclusion == "action_required" or .conclusion == "startup_failure") ' - "| {id: .id, name: .name, conclusion: .conclusion}" + "| {id: .id, name: .name, conclusion: .conclusion, details_url: .details_url}" ), ], None, @@ -435,20 +435,52 @@ def _fetch_failed_workflow_logs(repo: str, head_sha: str, runner: GhRunner) -> s continue node = json.loads(line) check_id = node.get("id") - if not check_id: + if not isinstance(check_id, int) or isinstance(check_id, bool) or check_id <= 0: continue name = str(node.get("name") or "unnamed check") conclusion = str(node.get("conclusion") or "failure") + job_id = _github_actions_job_id(repo, node.get("details_url")) try: - log = runner(["gh", "api", f"repos/{repo}/actions/jobs/{check_id}/logs"], None) + if job_id is None: + raise RuntimeError("check details did not identify a repository-bound Actions job") + log = runner(["gh", "api", f"repos/{repo}/actions/jobs/{job_id}/logs"], None) except RuntimeError as exc: - log = f"[log unavailable: {_failure_reason(name, exc)}]" + try: + annotations = runner( + [ + "gh", + "api", + "--paginate", + f"repos/{repo}/check-runs/{check_id}/annotations?per_page=100", + "--jq", + r'.[] | "\(.path // \"\"):\(.start_line // 0): \(.annotation_level // \"failure\"): \(.message // \"\")"', + ], + None, + ) + except RuntimeError: + annotations = "" + log = annotations.strip() or f"[log unavailable: {_failure_reason(name, exc)}]" excerpts.append(f"## {name} ({conclusion})\n{_truncate(log, 8000)}") if not excerpts: return f"No failed GitHub Actions checks were reported for current head {head_sha}." return _truncate("\n\n".join(excerpts), MAX_WORKFLOW_LOG_CHARS) +def _github_actions_job_id(repo: str, details_url: object) -> int | None: + """Return the Actions job id from an exact repository-bound GitHub URL.""" + if not isinstance(details_url, str): + return None + parsed = urlparse(details_url) + if parsed.scheme != "https" or parsed.netloc.casefold() != "github.com": + return None + match = re.fullmatch( + rf"/{re.escape(repo)}/actions/runs/[1-9][0-9]*/job/([1-9][0-9]*)/?", + parsed.path, + flags=re.IGNORECASE, + ) + return int(match.group(1)) if match else None + + def _severity_from_github(raw: str) -> Severity: """Normalize GitHub and Dependabot severity labels conservatively.""" normalized = raw.strip().lower() @@ -684,12 +716,26 @@ def _fetch_codegraph_status( def render_review_body(verdict: ReviewVerdict, head_sha: str, token_source: str) -> str: """Render the PR review body, including the interop marker the central gate detects.""" - finding_lines = [ - f"- [{finding.severity.value}] {finding.path}" - + (f":{finding.line}" if finding.line else "") - + f": {finding.recommendation} ({finding.evidence})" - for finding in verdict.findings - ] or ["- No blocking findings."] + finding_lines: list[str] = [] + for finding in verdict.findings: + location = finding.path + (f":{finding.line}" if finding.line else "") + finding_lines.extend( + [ + f"#### [{finding.priority.value}] {location}", + f"- Severity: {finding.severity.value}", + f"- Evidence type: {finding.evidence_type.value}", + f"- Evidence: {finding.evidence}", + f"- Observable impact: {finding.observable_impact}", + f"- Trigger: {finding.trigger}", + f"- Smallest fix: {finding.recommendation}", + f"- Regression: `{finding.regression_command}`", + ] + ) + if finding.suggested_diff: + finding_lines.extend(["", "```suggestion", finding.suggested_diff, "```"]) + finding_lines.append("") + if not finding_lines: + finding_lines = ["- No blocking findings."] blocked_lines = [f"- {reason}" for reason in verdict.blocked_reasons] body = [ "## Noema PydanticAI review", @@ -752,6 +798,16 @@ def publish_verdict( "commit_id": head_sha, "event": event, "body": render_review_body(verdict, head_sha, token_source), + "comments": [ + { + "path": finding.path, + "line": finding.line, + "side": "RIGHT", + "body": f"```suggestion\n{finding.suggested_diff}\n```", + } + for finding in verdict.findings + if finding.suggested_diff and finding.line + ], } runner( ["gh", "api", "-X", "POST", f"repos/{repo}/pulls/{pr_number}/reviews", "--input", "-"], diff --git a/reviewer/noema_reviewer/models.py b/reviewer/noema_reviewer/models.py index 3962b9807..a054f2156 100644 --- a/reviewer/noema_reviewer/models.py +++ b/reviewer/noema_reviewer/models.py @@ -11,7 +11,7 @@ from enum import Enum -from pydantic import BaseModel, Field, model_validator +from pydantic import BaseModel, Field, field_validator, model_validator class Verdict(str, Enum): @@ -40,6 +40,24 @@ class Confidence(str, Enum): LOW = "low" +class Priority(str, Enum): + """Review priority compatible with actionable PR-review conventions.""" + + P1 = "P1" + P2 = "P2" + P3 = "P3" + + +class EvidenceType(str, Enum): + """The source that independently supports a finding.""" + + NEARBY_IMPLEMENTATION = "nearby_implementation" + MATCHING_EXAMPLE = "matching_existing_example" + CROSS_FILE_COUNTERPART = "cross_file_counterpart" + OFFICIAL_DOCS = "current_official_docs" + FAILED_CHECK = "failed_check_or_log" + + # Severities at or above which an unresolved dependency finding must block an # approval (the org rule: remediate MEDIUM-or-higher by bump, never by gate # weakening). Ordered worst-first for deterministic comparisons. @@ -54,17 +72,71 @@ class Finding(BaseModel): """A single reviewer-facing issue tied to concrete evidence.""" severity: Severity = Field(description="How serious the issue is.") + priority: Priority = Field(description="P1, P2, or P3 review priority.") path: str = Field(description="Repository-relative path the issue lives in.") line: int | None = Field( default=None, description="1-indexed line the issue anchors to, when known.", ) + check_name: str | None = Field( + default=None, + description=( + "Exact current-head failed check causally explained by this finding, " + "when the finding is a failed-check RCA." + ), + ) evidence: str = Field( + min_length=1, description="Log, SARIF, test, or source reference proving the issue is real.", ) + evidence_type: EvidenceType = Field(description="The kind of source evidence supporting the finding.") + observable_impact: str = Field( + min_length=1, + description="The user- or operator-visible failure caused by the issue.", + ) + trigger: str = Field( + min_length=1, + description="The concrete condition or workflow that exposes the issue.", + ) recommendation: str = Field( + min_length=1, description="The specific fix the author should apply.", ) + regression_command: str = Field( + min_length=1, + description="One exact command or test target that verifies the fix.", + ) + suggested_diff: str | None = Field( + default=None, + max_length=8000, + description="Minimal replacement text for a GitHub suggestion block, when possible.", + ) + + @field_validator("line", mode="before") + @classmethod + def require_exact_positive_integer_line(cls, value: object) -> int | None: + """Keep GitHub source identity 1-indexed and free from scalar coercion.""" + if value is None: + return None + if isinstance(value, bool) or not isinstance(value, int) or value <= 0: + raise ValueError("line must be an exact positive integer when supplied") + return value + + @field_validator("regression_command") + @classmethod + def require_single_line_command(cls, value: str) -> str: + """Keep the published command exact and safe inside inline-code markup.""" + if any(character in value for character in "\r\n`"): + raise ValueError("regression command must be one plain-text command") + return value + + @field_validator("suggested_diff") + @classmethod + def reject_suggestion_fence_injection(cls, value: str | None) -> str | None: + """Prevent model output from escaping the GitHub suggestion fence.""" + if value is not None and "```" in value: + raise ValueError("suggested diff cannot contain a Markdown fence") + return value class ReviewVerdict(BaseModel): diff --git a/reviewer/tests/test_agent.py b/reviewer/tests/test_agent.py index db624d5d2..14f873413 100644 --- a/reviewer/tests/test_agent.py +++ b/reviewer/tests/test_agent.py @@ -7,6 +7,7 @@ from noema_reviewer.agent import ( PydanticAIReviewAgent, ReviewAgent, + SYSTEM_PROMPT, build_agent, build_prompt, ) @@ -85,6 +86,9 @@ def test_build_prompt_includes_all_sections() -> None: assert "Dependency findings:" in prompt assert "SARIF summary:" in prompt assert "Workflow log excerpts:" in prompt + assert "exact repository path and positive line" in SYSTEM_PROMPT + assert "P1/P2/P3 priority" in SYSTEM_PROMPT + assert "exact regression command" in SYSTEM_PROMPT assert "Prior review comments:" in prompt assert "Changed-file context:" in prompt diff --git a/reviewer/tests/test_check_run_pagination.py b/reviewer/tests/test_check_run_pagination.py index ed41229d9..1d8c71924 100644 --- a/reviewer/tests/test_check_run_pagination.py +++ b/reviewer/tests/test_check_run_pagination.py @@ -21,7 +21,7 @@ def __init__(self, *, include_late_failure: bool = False) -> None: def __call__(self, args, stdin=None): """Return 101 checks or the log belonging to the late failed check.""" self.calls.append(list(args)) - if any("/actions/jobs/" in part for part in args): + if any("/actions/jobs/123456/logs" in part for part in args): return "late failure details" checks = [ @@ -30,7 +30,11 @@ def __call__(self, args, stdin=None): ] late_check = {"name": "check-100", "conclusion": "success"} if self.include_late_failure: - late_check.update({"id": 987654, "conclusion": "failure"}) + late_check.update({ + "id": 987654, + "conclusion": "failure", + "details_url": "https://github.com/ContextualWisdomLab/example/actions/runs/42/job/123456", + }) checks.append(late_check) return "\n".join(json.dumps(check) for check in checks) @@ -71,7 +75,7 @@ def test_failed_workflow_logs_retain_a_failure_after_the_first_page() -> None: assert "## check-100 (failure)" in logs assert "late failure details" in logs - assert any("/actions/jobs/987654/logs" in part for call in runner.calls for part in call) + assert any("/actions/jobs/123456/logs" in part for call in runner.calls for part in call) command = _check_runs_command(runner) _assert_complete_pagination(command) jq_filter = command[command.index("--jq") + 1] diff --git a/reviewer/tests/test_deterministic_finding_identity.py b/reviewer/tests/test_deterministic_finding_identity.py index c7965b7d4..64fc17037 100644 --- a/reviewer/tests/test_deterministic_finding_identity.py +++ b/reviewer/tests/test_deterministic_finding_identity.py @@ -2,11 +2,19 @@ from noema_reviewer.gating import enforce_security_and_check_gates from noema_reviewer.manifest import ReviewManifest, SecurityFinding -from noema_reviewer.models import Finding, ReviewVerdict, Severity, Verdict +from noema_reviewer.models import ( + EvidenceType, + Finding, + Priority, + ReviewVerdict, + Severity, + Verdict, +) def test_scanner_finding_is_not_hidden_by_model_finding_at_same_path_and_severity() -> None: """Distinct deterministic scanner evidence must survive a model path/severity collision.""" + path = "reviewer/noema_reviewer/github_io.py" manifest = ReviewManifest( repo="ContextualWisdomLab/noema", pr_number=1, @@ -16,7 +24,7 @@ def test_scanner_finding_is_not_hidden_by_model_finding_at_same_path_and_severit identifier="py/path-injection", severity=Severity.HIGH, message="Untrusted path reaches filesystem access", - path="reviewer/noema_reviewer/github_io.py", + path=path, line=42, url="https://example.invalid/alert/1", ) @@ -28,10 +36,15 @@ def test_scanner_finding_is_not_hidden_by_model_finding_at_same_path_and_severit findings=[ Finding( severity=Severity.HIGH, - path="reviewer/noema_reviewer/github_io.py", + priority=Priority.P1, + path=path, line=7, evidence="Model evidence for an unrelated boundary defect.", + evidence_type=EvidenceType.NEARBY_IMPLEMENTATION, + observable_impact="A separate review boundary is incorrect.", + trigger="Reviewing the unrelated boundary path.", recommendation="Repair the unrelated boundary defect.", + regression_command="python -m pytest reviewer/tests/test_gating.py", ) ], ) diff --git a/reviewer/tests/test_failed_check_causal_binding.py b/reviewer/tests/test_failed_check_causal_binding.py new file mode 100644 index 000000000..68a7ab33a --- /dev/null +++ b/reviewer/tests/test_failed_check_causal_binding.py @@ -0,0 +1,89 @@ +"""Regression tests for causal binding between failed checks and source findings.""" + +from __future__ import annotations + +from noema_reviewer.gating import apply_gates +from noema_reviewer.manifest import ChangedFile, CheckConclusion, ReviewManifest +from noema_reviewer.models import EvidenceType, Finding, Priority, ReviewVerdict, Severity, Verdict + + +def _manifest(*check_names: str) -> ReviewManifest: + """Build complete review evidence with the requested failed checks.""" + return ReviewManifest( + repo="o/r", + pr_number=1, + diff="diff --git a/a.py b/a.py\ndiff --git a/b.py b/b.py", + changed_files=[ + ChangedFile(path="a.py", content="raise RuntimeError('build')"), + ChangedFile(path="b.py", content="raise RuntimeError('lint')"), + ], + check_conclusions=[ + CheckConclusion(name=name, conclusion="failure") for name in check_names + ], + codegraph_status="## codegraph explore\na.py -> build_failure", + ) + + +def _finding(*, check_name: str | None) -> Finding: + """Build one otherwise-actionable source finding for failed-check tests.""" + return Finding( + severity=Severity.HIGH, + priority=Priority.P1, + path="a.py", + line=1, + check_name=check_name, + evidence="current-head log reports the failing assertion at a.py:1", + evidence_type=EvidenceType.FAILED_CHECK, + observable_impact="The current-head check fails.", + trigger="Running the bound check.", + recommendation="Fix the regression and retain this assertion as a test.", + regression_command="uv run pytest reviewer/tests/test_failed_check_causal_binding.py", + ) + + +def test_each_failed_check_requires_its_own_source_bound_rca() -> None: + """One actionable finding cannot clear a second failed check.""" + verdict = ReviewVerdict( + verdict=Verdict.REQUEST_CHANGES, + summary="The build check has an actionable source regression.", + findings=[_finding(check_name="build")], + ) + + gated = apply_gates(_manifest("build", "lint"), verdict, strict=False) + + assert gated.verdict is Verdict.BLOCKED + assert gated.blocked_reasons == [ + "failed check lint lacks an actionable current-head path:line finding" + ] + + +def test_unbound_actionable_finding_cannot_clear_failed_check() -> None: + """Path and line evidence without exact check identity remains blocked.""" + verdict = ReviewVerdict( + verdict=Verdict.REQUEST_CHANGES, + summary="A source regression exists, but it is not bound to the failed check.", + findings=[_finding(check_name=None)], + ) + + gated = apply_gates(_manifest("build"), verdict, strict=False) + + assert gated.verdict is Verdict.BLOCKED + assert gated.blocked_reasons == [ + "failed check build lacks an actionable current-head path:line finding" + ] + + +def test_wrong_check_identity_cannot_clear_failed_check() -> None: + """A finding bound to another check cannot stand in for the failed check.""" + verdict = ReviewVerdict( + verdict=Verdict.REQUEST_CHANGES, + summary="The finding names a different check.", + findings=[_finding(check_name="lint")], + ) + + gated = apply_gates(_manifest("build"), verdict, strict=False) + + assert gated.verdict is Verdict.BLOCKED + assert gated.blocked_reasons == [ + "failed check build lacks an actionable current-head path:line finding" + ] diff --git a/reviewer/tests/test_failed_check_coverage_edges.py b/reviewer/tests/test_failed_check_coverage_edges.py new file mode 100644 index 000000000..97d457cba --- /dev/null +++ b/reviewer/tests/test_failed_check_coverage_edges.py @@ -0,0 +1,82 @@ +"""Coverage contracts for reviewer fail-closed edge branches.""" + +from noema_reviewer.gating import invalid_suggestion_reasons +from noema_reviewer.github_io import _github_actions_job_id, render_review_body +from noema_reviewer.manifest import ChangedFile, ReviewManifest +from noema_reviewer.models import ( + EvidenceType, + Finding, + Priority, + ReviewVerdict, + Severity, + Verdict, +) + + +def _finding(*, line: int = 1, suggested_diff: str | None = None) -> Finding: + """Build one source-backed finding for rendering and anchoring edge tests.""" + return Finding( + severity=Severity.HIGH, + priority=Priority.P1, + path="a.py", + line=line, + evidence="current-head evidence", + evidence_type=EvidenceType.NEARBY_IMPLEMENTATION, + observable_impact="The current-head behavior is incorrect.", + trigger="Execute the affected path.", + recommendation="Apply the bounded source repair.", + regression_command="python -m pytest", + suggested_diff=suggested_diff, + ) + + +def test_diff_metadata_line_terminates_right_side_anchor_sequence() -> None: + """Unexpected diff metadata cannot leave a later suggestion line attachable.""" + manifest = ReviewManifest( + repo="o/r", + pr_number=1, + diff=( + "diff --git a/a.py b/a.py\n" + "--- a/a.py\n" + "+++ b/a.py\n" + "@@ -1 +1,2 @@\n" + "+first\n" + "\\ No newline at end of file\n" + "+second" + ), + changed_files=[ChangedFile(path="a.py", content="first\nsecond")], + ) + + verdict = ReviewVerdict( + verdict=Verdict.REQUEST_CHANGES, + summary="fix", + findings=[_finding(line=2, suggested_diff="replacement")], + ) + + assert invalid_suggestion_reasons(manifest, verdict) == [ + "suggested diff is not anchored to a current-head right-side diff line: a.py:2" + ] + + +def test_actions_job_id_rejects_non_https_github_url() -> None: + """Only repository-bound HTTPS GitHub job URLs can authorize log retrieval.""" + assert _github_actions_job_id( + "o/r", + "http://github.com/o/r/actions/runs/1/job/2", + ) is None + + +def test_review_body_renders_finding_without_inline_suggestion() -> None: + """A source finding without a suggestion renders without inventing a patch block.""" + body = render_review_body( + ReviewVerdict( + verdict=Verdict.REQUEST_CHANGES, + summary="current-head finding", + findings=[_finding()], + ), + "a" * 40, + "github-app", + ) + + assert "#### [P1] a.py:1" in body + assert "```suggestion" not in body diff --git a/reviewer/tests/test_finding_line_contract.py b/reviewer/tests/test_finding_line_contract.py new file mode 100644 index 000000000..4121eb938 --- /dev/null +++ b/reviewer/tests/test_finding_line_contract.py @@ -0,0 +1,37 @@ +"""Regression tests for exact GitHub review-line identity.""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError + +from noema_reviewer.models import EvidenceType, Finding, Priority, Severity + + +def _finding_payload(line: object) -> dict[str, object]: + """Build the smallest complete finding payload around one line candidate.""" + return { + "severity": Severity.HIGH, + "priority": Priority.P1, + "path": "src/example.py", + "line": line, + "evidence": "current-head regression", + "evidence_type": EvidenceType.NEARBY_IMPLEMENTATION, + "observable_impact": "GitHub cannot attach the review finding to an exact source line.", + "trigger": "Publishing a finding with a non-positive or coerced line value.", + "recommendation": "Require an exact positive integer review line at schema admission.", + "regression_command": "uv run pytest reviewer/tests/test_finding_line_contract.py", + } + + +@pytest.mark.parametrize("invalid_line", [0, -1, True, False, 1.0, "1"]) +def test_finding_rejects_non_exact_positive_integer_lines(invalid_line: object) -> None: + """Finding.line is a 1-indexed GitHub identity, not a coercible scalar.""" + with pytest.raises(ValidationError): + Finding.model_validate(_finding_payload(invalid_line)) + + +def test_finding_accepts_positive_integer_or_missing_line() -> None: + """Valid current-head line identities and intentionally absent lines remain supported.""" + assert Finding.model_validate(_finding_payload(1)).line == 1 + assert Finding.model_validate(_finding_payload(None)).line is None diff --git a/reviewer/tests/test_gating.py b/reviewer/tests/test_gating.py index 792719a16..3218b416a 100644 --- a/reviewer/tests/test_gating.py +++ b/reviewer/tests/test_gating.py @@ -7,7 +7,8 @@ blocked_verdict, enforce_dependency_gate, enforce_security_and_check_gates, - failed_checks_as_review, + failed_check_blockers, + invalid_suggestion_reasons, missing_evidence, security_findings_as_review, unresolved_threads_as_review, @@ -20,7 +21,15 @@ ReviewManifest, SecurityFinding, ) -from noema_reviewer.models import Confidence, Finding, ReviewVerdict, Severity, Verdict +from noema_reviewer.models import ( + Confidence, + EvidenceType, + Finding, + Priority, + ReviewVerdict, + Severity, + Verdict, +) def _full_manifest(**overrides) -> ReviewManifest: @@ -98,17 +107,69 @@ def test_evidence_collection_failure_blocks_strict_review() -> None: assert reasons == ["evidence collection failure: code scanning: HTTP 403"] -def test_failed_check_downgrades_approval_with_log_pointer() -> None: - """A current-head failed check becomes a deterministic HIGH finding.""" +def test_failed_check_without_source_mapping_blocks_publication() -> None: + """A check name alone cannot become a synthetic source-code finding.""" manifest = _full_manifest(check_conclusions=[CheckConclusion(name="build", conclusion="failure")]) - finding = failed_checks_as_review(manifest)[0] - assert finding.path.endswith("/build") - gated = enforce_security_and_check_gates( + assert failed_check_blockers(manifest) == [ + "failed check build lacks an actionable current-head path:line finding" + ] + gated = apply_gates( manifest, ReviewVerdict(verdict=Verdict.APPROVE, summary="looks good"), + strict=False, ) - assert gated.verdict is Verdict.REQUEST_CHANGES - assert "current-head checks" in gated.summary + assert gated.verdict is Verdict.BLOCKED + assert "path:line" in gated.blocked_reasons[0] + + +def test_failed_check_accepts_model_rca_at_changed_source_line() -> None: + """A source-backed failed-check RCA remains publishable as request changes.""" + manifest = _full_manifest(check_conclusions=[CheckConclusion(name="build", conclusion="failure")]) + verdict = ReviewVerdict( + verdict=Verdict.REQUEST_CHANGES, + summary="The current-head build proves a source regression.", + findings=[ + Finding( + severity=Severity.HIGH, + priority=Priority.P1, + path="a", + line=1, + check_name="build", + evidence="build log reports the failing assertion at a:1", + evidence_type=EvidenceType.FAILED_CHECK, + observable_impact="The current-head build fails.", + trigger="Running the build check.", + recommendation="Fix the branch and add the failing assertion as a regression test.", + regression_command="uv run pytest reviewer/tests/test_gating.py", + ) + ], + ) + assert apply_gates(manifest, verdict, strict=False).verdict is Verdict.REQUEST_CHANGES + + +def test_suggestion_must_target_current_right_side_diff_line() -> None: + """A suggestion outside the exact diff fails closed before GitHub publication.""" + manifest = _full_manifest( + diff="diff --git a/a b/a\n--- a/a\n+++ b/a\n@@ -1 +1 @@\n-old\n+new" + ) + finding = Finding( + severity=Severity.HIGH, + priority=Priority.P1, + path="a", + line=2, + evidence="current source", + evidence_type=EvidenceType.NEARBY_IMPLEMENTATION, + observable_impact="The request fails.", + trigger="Calling the affected path.", + recommendation="Replace the expression.", + regression_command="uv run pytest reviewer/tests/test_gating.py", + suggested_diff="fixed", + ) + verdict = ReviewVerdict(verdict=Verdict.REQUEST_CHANGES, summary="fix", findings=[finding]) + assert invalid_suggestion_reasons(manifest, verdict) + assert apply_gates(manifest, verdict, strict=False).verdict is Verdict.BLOCKED + anchored = verdict.model_copy(update={"findings": [finding.model_copy(update={"line": 1})]}) + assert invalid_suggestion_reasons(manifest, anchored) == [] def test_primary_opencode_check_does_not_deadlock_independent_noema() -> None: @@ -119,20 +180,20 @@ def test_primary_opencode_check_does_not_deadlock_independent_noema() -> None: CheckConclusion(name="build", conclusion="success"), ] ) - assert failed_checks_as_review(manifest) == [] + assert failed_check_blockers(manifest) == [] verdict = ReviewVerdict(verdict=Verdict.APPROVE, summary="independent evidence passed") assert enforce_security_and_check_gates(manifest, verdict).verdict is Verdict.APPROVE def test_noema_review_check_does_not_deadlock_its_own_current_run() -> None: - """The in-flight Noema check cannot become a deterministic finding against itself.""" + """The exact in-flight Noema check cannot become an RCA prerequisite for itself.""" manifest = _full_manifest( check_conclusions=[ CheckConclusion(name="noema-review", conclusion="pending"), CheckConclusion(name="build", conclusion="success"), ] ) - assert failed_checks_as_review(manifest) == [] + assert failed_check_blockers(manifest) == [] verdict = ReviewVerdict(verdict=Verdict.APPROVE, summary="independent evidence passed") assert enforce_security_and_check_gates(manifest, verdict).verdict is Verdict.APPROVE @@ -145,7 +206,7 @@ def test_review_dependent_metadata_gate_does_not_deadlock_independent_noema() -> CheckConclusion(name="build", conclusion="success"), ] ) - assert failed_checks_as_review(manifest) == [] + assert failed_check_blockers(manifest) == [] verdict = ReviewVerdict(verdict=Verdict.APPROVE, summary="independent evidence passed") assert enforce_security_and_check_gates(manifest, verdict).verdict is Verdict.APPROVE @@ -155,7 +216,7 @@ def test_similarly_named_failed_check_remains_blocking() -> None: manifest = _full_manifest( check_conclusions=[CheckConclusion(name="opencode-review-copy", conclusion="failure")] ) - assert failed_checks_as_review(manifest) + assert failed_check_blockers(manifest) def test_similarly_named_noema_check_remains_blocking() -> None: @@ -163,7 +224,7 @@ def test_similarly_named_noema_check_remains_blocking() -> None: manifest = _full_manifest( check_conclusions=[CheckConclusion(name="noema-review-copy", conclusion="failure")] ) - assert failed_checks_as_review(manifest) + assert failed_check_blockers(manifest) def test_similarly_named_metadata_check_remains_blocking() -> None: @@ -173,7 +234,7 @@ def test_similarly_named_metadata_check_remains_blocking() -> None: CheckConclusion(name="metadata-only gate evaluation copy", conclusion="failure") ] ) - assert failed_checks_as_review(manifest) + assert failed_check_blockers(manifest) def test_unresolved_current_thread_downgrades_approval() -> None: @@ -294,7 +355,7 @@ def test_dependency_gate_does_not_touch_blocked() -> None: def test_dependency_gate_deduplicates_exact_existing_finding() -> None: - """An exact pre-existing deterministic finding is not duplicated.""" + """An exact pre-existing dependency finding is not duplicated.""" manifest = _full_manifest( dependency_findings=[DependencyFinding(tool="osv", package_name="dup", severity=Severity.MEDIUM)] ) @@ -304,9 +365,14 @@ def test_dependency_gate_deduplicates_exact_existing_finding() -> None: findings=[ Finding( severity=Severity.MEDIUM, + priority=Priority.P2, path="dup", evidence="osv reported dup@current", + evidence_type=EvidenceType.FAILED_CHECK, + observable_impact="The pull request would retain a known vulnerable dependency.", + trigger="Installing the dependency set recorded by the current lockfile.", recommendation="Bump dup to a non-vulnerable release and refresh the lockfile.", + regression_command="uv run pip-audit", ) ], ) diff --git a/reviewer/tests/test_github_io.py b/reviewer/tests/test_github_io.py index 0158ff269..f2ea2c82e 100644 --- a/reviewer/tests/test_github_io.py +++ b/reviewer/tests/test_github_io.py @@ -25,7 +25,15 @@ publish_verdict, render_review_body, ) -from noema_reviewer.models import Confidence, Finding, ReviewVerdict, Severity, Verdict +from noema_reviewer.models import ( + Confidence, + EvidenceType, + Finding, + Priority, + ReviewVerdict, + Severity, + Verdict, +) REPO = "ContextualWisdomLab/example" HEAD_SHA = "a" * 40 @@ -44,10 +52,12 @@ def __init__(self, *, fail_contents: bool = False) -> None: """Record whether the contents endpoint should raise.""" self.fail_contents = fail_contents self.calls: list[list[str]] = [] + self.stdins: list[str | None] = [] def __call__(self, args, stdin=None): """Return canned responses keyed by the requested endpoint.""" self.calls.append(list(args)) + self.stdins.append(stdin) joined = " ".join(args) if "Accept: application/vnd.github.v3.diff" in joined: return "diff --git a/x b/x\n+new line" @@ -305,9 +315,9 @@ def test_failed_workflow_logs_include_exact_check_reason() -> None: def runner(args, stdin=None): joined = " ".join(args) - if "/check-runs" in joined: - return json.dumps({"id": 42, "name": "tests", "conclusion": "failure"}) - if "/jobs/42/logs" in joined: + if "/check-runs" in joined and "/annotations" not in joined: + return json.dumps({"id": 42, "name": "tests", "conclusion": "failure", "details_url": "https://github.com/o/r/actions/runs/10/job/99"}) + if "/jobs/99/logs" in joined: return "AssertionError: expected 1, got 2" return "" @@ -316,11 +326,31 @@ def runner(args, stdin=None): assert "AssertionError" in result +def test_failed_workflow_logs_never_treat_check_run_id_as_job_id() -> None: + """GitHub Check Run ids and Actions Job ids are separate namespaces.""" + calls: list[str] = [] + + def runner(args, stdin=None): + joined = " ".join(args) + calls.append(joined) + if "/check-runs" in joined and "/annotations" not in joined: + return json.dumps({"id": 42, "name": "tests", "conclusion": "failure", "details_url": "https://github.com/o/r/actions/runs/10/job/99"}) + if "/jobs/99/logs" in joined: + return "src/service.py:17: AssertionError" + return "" + + result = _fetch_failed_workflow_logs("o/r", "head", runner) + assert "src/service.py:17" in result + assert any("/jobs/99/logs" in call for call in calls) + assert not any("/jobs/42/logs" in call for call in calls) + + def test_failed_workflow_logs_explain_unavailable_job_log() -> None: """A job-log API error remains visible rather than disappearing.""" def runner(args, stdin=None): - if "/check-runs" in " ".join(args): + joined = " ".join(args) + if "/check-runs" in joined and "/annotations" not in joined: return json.dumps({"id": 42, "name": "tests", "conclusion": "failure"}) raise RuntimeError("HTTP 404") @@ -480,11 +510,25 @@ def test_render_review_body_marks_findings_and_marker() -> None: verdict = ReviewVerdict( verdict=Verdict.REQUEST_CHANGES, summary="please fix", - findings=[Finding(severity=Severity.HIGH, path="x.py", line=3, evidence="log", recommendation="bump")], + findings=[Finding( + severity=Severity.HIGH, + priority=Priority.P1, + path="x.py", + line=3, + evidence="log", + evidence_type=EvidenceType.FAILED_CHECK, + observable_impact="The build fails.", + trigger="Running the build check.", + recommendation="bump", + regression_command="uv run pytest reviewer/tests/test_github_io.py", + suggested_diff="fixed = True", + )], confidence=Confidence.MEDIUM, ) body = render_review_body(verdict, "headsha", "NOEMA_REVIEW_TOKEN") - assert "[high] x.py:3" in body + assert "[P1] x.py:3" in body + assert "Observable impact: The build fails." in body + assert "```suggestion\nfixed = True\n```" in body assert "" in body assert "Result: REQUEST_CHANGES" in body @@ -512,6 +556,36 @@ def test_publish_verdict_posts_review() -> None: assert post[:3] == ["gh", "api", "-X"] +def test_publish_verdict_posts_applyable_inline_suggestion() -> None: + """A source replacement is sent as a right-side GitHub suggestion comment.""" + runner = StubRunner() + verdict = ReviewVerdict( + verdict=Verdict.REQUEST_CHANGES, + summary="fix the line", + findings=[Finding( + severity=Severity.HIGH, + priority=Priority.P1, + path="x.py", + line=3, + evidence="current source", + evidence_type=EvidenceType.NEARBY_IMPLEMENTATION, + observable_impact="The request fails.", + trigger="Calling the affected endpoint.", + recommendation="Replace the faulty expression.", + regression_command="uv run pytest reviewer/tests/test_github_io.py", + suggested_diff="return fixed_value", + )], + ) + publish_verdict(REPO, 5, verdict, HEAD_SHA, runner=runner) + payload = json.loads(runner.stdins[-1] or "{}") + assert payload["comments"] == [{ + "path": "x.py", + "line": 3, + "side": "RIGHT", + "body": "```suggestion\nreturn fixed_value\n```", + }] + + def test_publish_verdict_rejects_invalid_metadata() -> None: """Publication rejects an out-of-scope repository before any GitHub call.""" verdict = ReviewVerdict(verdict=Verdict.APPROVE, summary="ok") diff --git a/reviewer/tests/test_models.py b/reviewer/tests/test_models.py index c97202694..9aab7c1ef 100644 --- a/reviewer/tests/test_models.py +++ b/reviewer/tests/test_models.py @@ -2,10 +2,15 @@ from __future__ import annotations +import pytest +from pydantic import ValidationError + from noema_reviewer.models import ( BLOCKING_SEVERITIES, Confidence, + EvidenceType, Finding, + Priority, ReviewVerdict, Severity, Verdict, @@ -40,10 +45,40 @@ def test_finding_roundtrips_optional_line() -> None: """A finding keeps an optional line and required evidence/recommendation.""" finding = Finding( severity=Severity.HIGH, + priority=Priority.P1, path="src/x.py", evidence="test log", + evidence_type=EvidenceType.FAILED_CHECK, + observable_impact="The tested behavior fails.", + trigger="Running the focused test.", recommendation="fix it", + regression_command="uv run pytest reviewer/tests/test_models.py", ) assert finding.line is None dumped = finding.model_dump() assert dumped["severity"] == "high" + assert { + "priority", "evidence_type", "observable_impact", "trigger", "regression_command" + } <= set(Finding.model_json_schema()["required"]) + + +@pytest.mark.parametrize( + ("field", "value"), + [("regression_command", "pytest\nrm -rf x"), ("suggested_diff", "```\nunsafe\n```")], +) +def test_finding_rejects_markdown_command_injection(field: str, value: str) -> None: + """Published commands and suggestions cannot escape their Markdown delimiters.""" + payload = { + "severity": Severity.HIGH, + "priority": Priority.P1, + "path": "src/x.py", + "evidence": "test log", + "evidence_type": EvidenceType.FAILED_CHECK, + "observable_impact": "The test fails.", + "trigger": "Running the test.", + "recommendation": "Fix it.", + "regression_command": "uv run pytest", + field: value, + } + with pytest.raises(ValidationError): + Finding.model_validate(payload) diff --git a/reviewer/tests/test_non_success_check_gate.py b/reviewer/tests/test_non_success_check_gate.py index 3f4649d5d..d87ae5e77 100644 --- a/reviewer/tests/test_non_success_check_gate.py +++ b/reviewer/tests/test_non_success_check_gate.py @@ -4,7 +4,7 @@ import pytest -from noema_reviewer.gating import enforce_security_and_check_gates, failed_checks_as_review +from noema_reviewer.gating import apply_gates, enforce_security_and_check_gates, failed_check_blockers from noema_reviewer.manifest import ChangedFile, CheckConclusion, ReviewManifest from noema_reviewer.models import ReviewVerdict, Verdict @@ -26,15 +26,13 @@ def test_observed_non_success_check_cannot_preserve_approval(conclusion: str) -> """Every observed ordinary check must be terminal-success before approval.""" manifest = _manifest_with_check("ci", conclusion) - findings = failed_checks_as_review(manifest) - assert len(findings) == 1 - assert conclusion in findings[0].evidence - - gated = enforce_security_and_check_gates( + assert failed_check_blockers(manifest) + gated = apply_gates( manifest, ReviewVerdict(verdict=Verdict.APPROVE, summary="model approved"), + strict=False, ) - assert gated.verdict is Verdict.REQUEST_CHANGES + assert gated.verdict is Verdict.BLOCKED def test_observed_success_check_remains_nonblocking() -> None: @@ -42,7 +40,7 @@ def test_observed_success_check_remains_nonblocking() -> None: manifest = _manifest_with_check("ci", "success") verdict = ReviewVerdict(verdict=Verdict.APPROVE, summary="model approved") - assert failed_checks_as_review(manifest) == [] + assert failed_check_blockers(manifest) == [] assert enforce_security_and_check_gates(manifest, verdict).verdict is Verdict.APPROVE @@ -55,5 +53,5 @@ def test_cycle_breaking_review_checks_remain_explicit_exceptions(name: str) -> N manifest = _manifest_with_check(name, "skipped") verdict = ReviewVerdict(verdict=Verdict.APPROVE, summary="independent evidence passed") - assert failed_checks_as_review(manifest) == [] + assert failed_check_blockers(manifest) == [] assert enforce_security_and_check_gates(manifest, verdict).verdict is Verdict.APPROVE diff --git a/reviewer/tests/test_verdict_invariants.py b/reviewer/tests/test_verdict_invariants.py index 355f826db..7d560a99d 100644 --- a/reviewer/tests/test_verdict_invariants.py +++ b/reviewer/tests/test_verdict_invariants.py @@ -5,16 +5,21 @@ import pytest from pydantic import ValidationError -from noema_reviewer.models import Finding, ReviewVerdict, Severity, Verdict +from noema_reviewer.models import EvidenceType, Finding, Priority, ReviewVerdict, Severity, Verdict def _finding(severity: Severity) -> Finding: """Build one concrete reviewer finding at the requested severity.""" return Finding( severity=severity, + priority=Priority.P1, path="src/example.py", evidence="current-head test evidence", + evidence_type=EvidenceType.NEARBY_IMPLEMENTATION, + observable_impact="The reviewed behavior fails.", + trigger="Running the affected code path.", recommendation="fix the defect", + regression_command="uv run pytest reviewer/tests/test_verdict_invariants.py", )