diff --git a/.fabro/workflows/code-review/code-review.fabro b/.fabro/workflows/code-review/code-review.fabro index 35cd784d5..e577ecd13 100644 --- a/.fabro/workflows/code-review/code-review.fabro +++ b/.fabro/workflows/code-review/code-review.fabro @@ -33,7 +33,7 @@ digraph CodeReview { timeout="300s", output_schema="routing", stdin_source="context.internal.run_id", - script="python3 -c \"import hashlib,sys; pairs=list(zip(sys.argv[1::2],sys.argv[2::2])); sys.exit(0 if pairs and all(hashlib.sha256(open(path,'rb').read()).hexdigest()==expected for path,expected in pairs) else 91)\" .fabro/workflows/code-review/scripts/code_review.py e05d017c71a16c8418c1cf941ae134695520ccd6db62186f251706d7e635eafa .fabro/workflows/code-review/scripts/git_readonly.py bcd4364ba3aca2ee1e12d5909204f645c16bdf22e3753a39d74c79d8d37cf73e .fabro/workflows/code-review/scripts/publish_pr.py 2ba754164e91524f0b4b0dd6bb761d3ba64876ed21ed4f59a1b64ebd0ec52080 .fabro/workflows/code-review/scripts/render_report.py fd8f5eb237a75e3d1e946279b27dabf7d08757e2a6aac8ff1732b504ee3d2655 .fabro/workflows/code-review/scripts/rule_loader.py f885409c64c631075d7e31fcb6e7a100592430c5eba9a6e989222006061a9f52 .fabro/workflows/code-review/specs/report-spec.md 006898b9e84951237b3fe862bd07f9be77070fa8d9cc8d28ce528c5ccf270fda .fabro/workflows/code-review/templates/report.html 04e8bffbbec4c4848646fc5ff3261aa06dd3fa95b1dfa21474f7dec1b91abd22 .fabro/workflows/code-review/schemas/findings.schema.json 6fdf7c63fb6183c8bef64a874cd56f805d92c2aa3c011a513ee07f07b0797b6d .fabro/workflows/code-review/schemas/verdict.schema.json 4cb752e624f1b88809017ac5db472d59850ffe09ec5a93d3b63ba636364b8b19 .fabro/workflows/code-review/schemas/file-groups.schema.json b53c4e1c0bbd07bbf70e83f4f3b35fd96cb880c621c7c424e95b9aea34e13d7c .fabro/workflows/code-review/prompts/finder.md.j2 86c2e6a032f7c54c1bbab1c12496a8f0d6bf48703abe6017eb175330608cf223 .fabro/workflows/code-review/prompts/verify.md.j2 6528cffd1bf199f27a32c19547aebf6f0c78a546698aaef81c6a89441b80f7da .fabro/workflows/code-review/prompts/sweep.md.j2 e6f89b47b11c57030a6ef7d5896ccb37dbd2a2982e9fa7eb7f2e3df73acab82c .fabro/workflows/code-review/prompts/group-files.md.j2 5b291313a1266d1d658f80ea7989cdefcd8609b6893b7197b17914d553dab041 .fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 b91ddeb86dc7f552323f7ac04823984da9365d013bfe7965c3eb6f51fa8abe0f .fabro/workflows/code-review/prompts/partials/guidance.md.j2 53bc0c40bb917288708bed1f9ba478fbd89b9790c92497762224cc752f40bef5 .fabro/workflows/code-review/prompts/partials/output-schema.md.j2 811994bb357739f2562d84f66dc05075ebe3c7f8d58034f8c25ee1c36bee996b .fabro/workflows/code-review/prompts/partials/read-only-explorer.md.j2 44a0244e7aa62fdb0dbbfdbadcffbfb640af249bae3e96895dedd5c7a33bad10 .fabro/workflows/code-review/prompts/partials/review-target.md.j2 abffeeff0e16b89a0754cd53f1833b3744494cd54ff761798b782a80467446ea .fabro/workflows/code-review/prompts/partials/safe-git-history.md.j2 4ddd8d36d5c51d7e166a6b7f1dff51b72cce0e64108cc7e892002ca909af8b3a .fabro/workflows/code-review/rules/builtin-manifest.json 47e3123fb25c560e36216dc9b8323c86e8301f1868a4fa3219bfd59bda3a533a && python3 .fabro/workflows/code-review/scripts/code_review.py prepare --review-id-stdin --mode {{ inputs.mode }} --effort {{ inputs.effort }} --scope {{ inputs.scope }} --base {{ inputs.base }} --commit {{ inputs.commit }} --range {{ inputs.range }} --model {{ inputs.model }} --guidance {{ inputs.guidance }}" + script="python3 -c \"import hashlib,sys; pairs=list(zip(sys.argv[1::2],sys.argv[2::2])); sys.exit(0 if pairs and all(hashlib.sha256(open(path,'rb').read()).hexdigest()==expected for path,expected in pairs) else 91)\" .fabro/workflows/code-review/scripts/code_review.py 961fe81afa6a5c80432d8e8a05791d6c12ccc724e9f945e7190bbac77a6121d2 .fabro/workflows/code-review/scripts/git_readonly.py bcd4364ba3aca2ee1e12d5909204f645c16bdf22e3753a39d74c79d8d37cf73e .fabro/workflows/code-review/scripts/publish_pr.py fe65b5c710e6f486bf0b4a0da4933b53b954c32481313efd8d60d34b3dcc3a44 .fabro/workflows/code-review/scripts/render_report.py 91ea86428ed4759fbb368b4fa90ca8f54f1ab05f09f18bc8a4e9ffa188ed2295 .fabro/workflows/code-review/scripts/rule_loader.py f885409c64c631075d7e31fcb6e7a100592430c5eba9a6e989222006061a9f52 .fabro/workflows/code-review/specs/report-spec.md 7a54f72ee46f09218d18854d184a1f36875f9011877e94779c6b1f0d5dd118a9 .fabro/workflows/code-review/templates/report.html 5def570da34ca186da31781378367d70fb9c58e82f7aeec4aaf420fd348a8e61 .fabro/workflows/code-review/schemas/findings.schema.json 2f4d0a9052d5af0dad92db12a1e9d49cc91a282c4dddda495791352bf1559ed8 .fabro/workflows/code-review/schemas/verdict.schema.json de13ce02c5fd0c088640542831cc732e35dee3ddb38f89d4412f6a46fea75567 .fabro/workflows/code-review/schemas/file-groups.schema.json b53c4e1c0bbd07bbf70e83f4f3b35fd96cb880c621c7c424e95b9aea34e13d7c .fabro/workflows/code-review/prompts/finder.md.j2 86c2e6a032f7c54c1bbab1c12496a8f0d6bf48703abe6017eb175330608cf223 .fabro/workflows/code-review/prompts/verify.md.j2 cb3866240077d1bc8993b2f012a8d66a6ea61d4a9f2e88a1fefc6ef375f630e2 .fabro/workflows/code-review/prompts/sweep.md.j2 e6f89b47b11c57030a6ef7d5896ccb37dbd2a2982e9fa7eb7f2e3df73acab82c .fabro/workflows/code-review/prompts/group-files.md.j2 5b291313a1266d1d658f80ea7989cdefcd8609b6893b7197b17914d553dab041 .fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 a81ee5b0ac134eb121dbf503025387c64126d3276e4673ebc836cfb62a3689fb .fabro/workflows/code-review/prompts/partials/guidance.md.j2 53bc0c40bb917288708bed1f9ba478fbd89b9790c92497762224cc752f40bef5 .fabro/workflows/code-review/prompts/partials/output-schema.md.j2 811994bb357739f2562d84f66dc05075ebe3c7f8d58034f8c25ee1c36bee996b .fabro/workflows/code-review/prompts/partials/read-only-explorer.md.j2 44a0244e7aa62fdb0dbbfdbadcffbfb640af249bae3e96895dedd5c7a33bad10 .fabro/workflows/code-review/prompts/partials/review-target.md.j2 abffeeff0e16b89a0754cd53f1833b3744494cd54ff761798b782a80467446ea .fabro/workflows/code-review/prompts/partials/safe-git-history.md.j2 4ddd8d36d5c51d7e166a6b7f1dff51b72cce0e64108cc7e892002ca909af8b3a .fabro/workflows/code-review/rules/builtin-manifest.json 47e3123fb25c560e36216dc9b8323c86e8301f1868a4fa3219bfd59bda3a533a && python3 .fabro/workflows/code-review/scripts/code_review.py prepare --review-id-stdin --mode {{ inputs.mode }} --effort {{ inputs.effort }} --scope {{ inputs.scope }} --base {{ inputs.base }} --commit {{ inputs.commit }} --range {{ inputs.range }} --model {{ inputs.model }} --guidance {{ inputs.guidance }}" ] grouping [ diff --git a/.fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 b/.fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 index b0ddd97e4..716fdc2aa 100644 --- a/.fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 +++ b/.fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 @@ -1,7 +1,9 @@ Report each candidate finding with: - `file`: the repository-relative path; -- `line`: the line in the reviewed revision the finding anchors to; +- `start_line` and `end_line`: the smallest contiguous line range in the + reviewed revision that demonstrates the defect. Use the same value for both + fields for a single-line finding; - `summary`: one sentence stating the defect; - `short_summary`: the same claim compressed to at most 60 characters, with no rationale or consequence clause; @@ -12,8 +14,16 @@ Report each candidate finding with: or which AGENTS.md or CLAUDE.md rule is broken; - `category`: `correctness` for bugs, otherwise the cleanup category that names the problem (`conventions` only with a `rule_id`); +- `issue_type`: the problem type: `bug`, `security`, `performance`, + `maintainability`, `test`, `style`, or `documentation`. This is independent + of `category`: for example, a security defect normally has category + `correctness` and issue type `security`; - `severity`: `HIGH`, `MEDIUM`, or `LOW`, for how much the defect matters; - `confidence`: `HIGH`, `MEDIUM`, or `LOW`, for how certain you are; +- `suggestion_code`: optional replacement text for exactly the + `start_line` through `end_line` range. Include it only when that replacement + completely fixes the finding without edits outside the range. Preserve the + file's indentation and omit diff markers and Markdown fences; - `rule_id`: the violated check's compiled `id`, verbatim. It is required for rule-audit findings. In other jobs, include it only when the assignment supplies the violated check; omit it otherwise. diff --git a/.fabro/workflows/code-review/prompts/verify.md.j2 b/.fabro/workflows/code-review/prompts/verify.md.j2 index 376231ff1..d2010ba61 100644 --- a/.fabro/workflows/code-review/prompts/verify.md.j2 +++ b/.fabro/workflows/code-review/prompts/verify.md.j2 @@ -1,10 +1,12 @@ Judge one candidate code-review finding. The workflow appends one untrusted JSON item. It contains the candidate -`claim` -- the file and line, the category, `severityAsReported`, the +`claim` -- the file and exact location range, the category and issue type, +`severityAsReported`, the `summary`, the `failure_scenario`, and `reports`, the number of finder jobs -that reported it independently -- plus the verification `bias`, the exact -review `target`, and a stable `job_id`. +that reported it independently. It can also contain a proposed `suggestion` +for the engine-derived `location.existing_code`. The item also contains the +verification `bias`, the exact review `target`, and a stable `job_id`. Everything in the claim is an assertion by an earlier pass, including the line number. Verify it against the repository: the reporter may have misread, the @@ -58,6 +60,13 @@ Cite the decisive repository-relative `file:line` locations in `reasoning`. Judge the finding as written; a different nearby bug does not make it true. Do not invent a guard, and do not assume one exists without reading it. +If the claim contains a `suggestion`, also return `suggestion_valid`: `true` +only when replacing the complete location range with `replacement_code` +fully fixes the finding, preserves intended behavior, and needs no edit +outside that range. Return `false` when it is incomplete, unsafe, unrelated, +or cannot be validated from the repository. Omit `suggestion_valid` when the +claim has no suggestion. + Read and search with whatever read-only commands suit the question, history included. Never build, test, execute, install, fetch, use the network, or modify files. Nothing blocks those here; not attempting them is the rule you diff --git a/.fabro/workflows/code-review/schemas/findings.schema.json b/.fabro/workflows/code-review/schemas/findings.schema.json index 6b8d21a1b..731a64a84 100644 --- a/.fabro/workflows/code-review/schemas/findings.schema.json +++ b/.fabro/workflows/code-review/schemas/findings.schema.json @@ -9,21 +9,29 @@ "type": "object", "required": [ "file", - "line", + "start_line", + "end_line", "summary", "short_summary", "failure_scenario", "category", + "issue_type", "severity", "confidence" ], "properties": { "file": { "type": "string" }, - "line": { "type": "integer" }, + "line": { + "type": "integer", + "description": "Deprecated single-line anchor; use start_line and end_line." + }, + "start_line": { "type": "integer", "minimum": 1 }, + "end_line": { "type": "integer", "minimum": 1 }, "rule_id": { "type": "string" }, "summary": { "type": "string" }, "short_summary": { "type": "string", "maxLength": 60 }, "failure_scenario": { "type": "string" }, + "suggestion_code": { "type": "string", "maxLength": 8000 }, "category": { "type": "string", "enum": [ @@ -36,6 +44,18 @@ "test-coverage" ] }, + "issue_type": { + "type": "string", + "enum": [ + "bug", + "security", + "performance", + "maintainability", + "test", + "style", + "documentation" + ] + }, "severity": { "type": "string", "enum": ["HIGH", "MEDIUM", "LOW"] diff --git a/.fabro/workflows/code-review/schemas/verdict.schema.json b/.fabro/workflows/code-review/schemas/verdict.schema.json index 60331f367..f6d256867 100644 --- a/.fabro/workflows/code-review/schemas/verdict.schema.json +++ b/.fabro/workflows/code-review/schemas/verdict.schema.json @@ -7,6 +7,7 @@ "enum": ["CONFIRMED", "PLAUSIBLE", "REFUTED"] }, "reasoning": { "type": "string" }, - "duplicate_of": { "type": "string" } + "duplicate_of": { "type": "string" }, + "suggestion_valid": { "type": "boolean" } } } diff --git a/.fabro/workflows/code-review/scripts/code_review.py b/.fabro/workflows/code-review/scripts/code_review.py index d2d84fae3..3155e6e49 100644 --- a/.fabro/workflows/code-review/scripts/code_review.py +++ b/.fabro/workflows/code-review/scripts/code_review.py @@ -91,6 +91,15 @@ CATEGORIES = ( "conventions", "test-coverage", ) +ISSUE_TYPES = ( + "bug", + "security", + "performance", + "maintainability", + "test", + "style", + "documentation", +) # Correctness bugs always outrank cleanup findings when a cap forces a cut. CLEANUP_CATEGORIES = frozenset(CATEGORIES) - {"correctness"} # Policy filters drop well-formed findings the review does not want; unlike a @@ -116,6 +125,8 @@ SIBLING_CAP = 6 CODE_FRAME_CONTEXT = 4 CODE_FRAME_MAX_LINE_LENGTH = 400 CODE_FRAME_MAX_BYTES = 2 * 1024 * 1024 +LOCATION_MAX_LINES = 50 +SUGGESTION_CODE_MAX_LENGTH = 8000 CODE_FRAME_LANGUAGES = { "c": "C", "cc": "C++", @@ -150,7 +161,7 @@ CODE_FRAME_LANGUAGES = { "yml": "YAML", } -CANONICAL_SCHEMA_VERSION = 3 +CANONICAL_SCHEMA_VERSION = 4 CANONICAL_FILES = ( "review-manifest.json", "candidate-ledger.jsonl", @@ -1013,6 +1024,19 @@ def verify_schema_sources() -> None: f"{FINDINGS_SCHEMA_PATH} category enum does not match this " "engine's category list" ) + try: + schema_issue_types = findings_schema["properties"]["findings"]["items"][ + "properties" + ]["issue_type"]["enum"] + except (KeyError, TypeError) as error: + raise WorkflowDataError( + f"{FINDINGS_SCHEMA_PATH} has no issue_type enum" + ) from error + if list(schema_issue_types) != list(ISSUE_TYPES): + raise WorkflowDataError( + f"{FINDINGS_SCHEMA_PATH} issue_type enum does not match this " + "engine's issue type list" + ) verdict_schema = read_json(root() / VERDICT_SCHEMA_PATH) try: schema_verdicts = verdict_schema["properties"]["verdict"]["enum"] @@ -1870,27 +1894,69 @@ def finding_or_rejection( if not isinstance(value, dict): return None, "the finding is not a JSON object" path = normalize_repo_path(value.get("file")) - line = value.get("line") + legacy_line = value.get("line") + start_line = value.get("start_line", legacy_line) + end_line = value.get("end_line", legacy_line) summary = one_line(value.get("summary"), 600).strip() short_summary = one_line(value.get("short_summary"), 200).strip()[:60] failure_scenario = clean_text(value.get("failure_scenario"), 4000).strip() category = one_line(value.get("category"), 40).strip().lower() + issue_type = one_line(value.get("issue_type"), 40).strip().lower() severity = one_line(value.get("severity"), 20).upper() confidence = one_line(value.get("confidence"), 20).upper() + raw_suggestion = value.get("suggestion_code", "") for failed, reason in ( (path is None or path == ".", "file does not name a repository file"), ( - isinstance(line, bool) or not isinstance(line, int) or line < 1, - "line is not a positive integer", + isinstance(start_line, bool) + or not isinstance(start_line, int) + or start_line < 1, + "start_line is not a positive integer", + ), + ( + isinstance(end_line, bool) + or not isinstance(end_line, int) + or end_line < 1, + "end_line is not a positive integer", + ), + ( + isinstance(start_line, int) + and isinstance(end_line, int) + and start_line > end_line, + "start_line is after end_line", + ), + ( + isinstance(start_line, int) + and isinstance(end_line, int) + and end_line - start_line + 1 > LOCATION_MAX_LINES, + f"location spans more than {LOCATION_MAX_LINES} lines", ), (not summary, "summary is empty"), (not failure_scenario, "failure_scenario is empty"), (category not in CATEGORIES, "category is not in the closed list"), + (issue_type not in ISSUE_TYPES, "issue_type is not in the closed list"), (severity not in SEVERITY_RANK, "severity is not HIGH, MEDIUM, or LOW"), ( confidence not in CONFIDENCE_RANK, "confidence is not HIGH, MEDIUM, or LOW", ), + ( + not isinstance(raw_suggestion, str), + "suggestion_code is not a string", + ), + ( + isinstance(raw_suggestion, str) + and len(raw_suggestion) > SUGGESTION_CODE_MAX_LENGTH, + f"suggestion_code exceeds {SUGGESTION_CODE_MAX_LENGTH} characters", + ), + ( + isinstance(raw_suggestion, str) + and any( + character not in "\n\t" and ord(character) < 0x20 + for character in raw_suggestion + ), + "suggestion_code contains control characters", + ), ): if failed: return None, reason @@ -1922,13 +1988,20 @@ def finding_or_rejection( return { "file": path, - "line": line, + # ``line`` remains the stable end-line alias used by ranking, + # deduplication, and older consumers. The canonical finding also + # carries the complete range. + "line": end_line, + "start_line": start_line, + "end_line": end_line, "summary": summary, "short_summary": short_summary, "failure_scenario": failure_scenario, "category": category, + "issue_type": issue_type, "severity": severity, "confidence": confidence, + "suggestion_code": raw_suggestion if raw_suggestion.strip() else "", "rule_ids": rule_ids, }, None @@ -1975,7 +2048,7 @@ def state_rule_context( } -def normalize_verdict(value: Any) -> Optional[Dict[str, str]]: +def normalize_verdict(value: Any) -> Optional[Dict[str, Any]]: if not isinstance(value, dict): return None verdict = value.get("verdict") @@ -1992,6 +2065,9 @@ def normalize_verdict(value: Any) -> Optional[Dict[str, str]]: duplicate_of.strip() ): result["duplicate_of"] = duplicate_of.strip() + suggestion_valid = value.get("suggestion_valid") + if isinstance(suggestion_valid, bool): + result["suggestion_valid"] = suggestion_valid return result @@ -2338,16 +2414,26 @@ def verification_claim( about applicability, and the verifier judges only violation. ``pool`` supplies the same-file siblings the verifier may name as duplicates. """ + start_line = int(candidate.get("start_line") or candidate.get("line") or 0) + end_line = int(candidate.get("end_line") or candidate.get("line") or 0) + location = resolved_location( + str(candidate.get("file") or ""), start_line, end_line + ) claim: Dict[str, Any] = { "file": candidate.get("file"), "line": candidate.get("line"), + "location": location, "category": candidate.get("category"), + "issue_type": candidate.get("issue_type"), "severityAsReported": candidate.get("severity"), "summary": candidate.get("summary"), "failure_scenario": candidate.get("failure_scenario"), "reports": int(candidate.get("reports") or 1), "siblings": sibling_claims(candidate, pool or []), } + suggestion_code = str(candidate.get("suggestion_code") or "") + if suggestion_code and location["existing_code"]: + claim["suggestion"] = {"replacement_code": suggestion_code} rules_state = (state or {}).get("rules") if isinstance(rules_state, dict) and rules_state.get("enabled"): catalog = rules_state.get("catalog") or {} @@ -2487,6 +2573,24 @@ def plan_verify() -> None: set(existing.get("rule_ids") or []) | set(report.get("rule_ids") or []) ) + # A fix is publishable only when reporting passes agree on its exact + # range and replacement. A pass that offers no fix does not veto an + # otherwise consistent proposal. + existing_suggestion = str(existing.get("suggestion_code") or "") + report_suggestion = str(report.get("suggestion_code") or "") + if not existing_suggestion and report_suggestion: + existing["start_line"] = report["start_line"] + existing["end_line"] = report["end_line"] + existing["line"] = report["end_line"] + existing["suggestion_code"] = report_suggestion + elif existing_suggestion and report_suggestion and ( + existing_suggestion != report_suggestion + or existing.get("start_line") != report.get("start_line") + or existing.get("end_line") != report.get("end_line") + ): + existing["suggestion_code"] = "" + if report.get("issue_type") == "security": + existing["issue_type"] = "security" if ( SEVERITY_RANK[report["severity"]] > SEVERITY_RANK[existing["severity"]] @@ -2712,46 +2816,84 @@ def safe_code_text(value: str) -> str: return text -def code_frame(file_path: str, line: int) -> Dict[str, Any]: - """Read the lines around a finding's anchor from the reviewed tree. +def reviewed_source_lines(file_path: str) -> Optional[List[str]]: + """Read one UTF-8 source file from the unchanged reviewed tree.""" + target = root() / file_path + try: + if target.is_symlink() or not target.is_file(): + return None + if target.stat().st_size > CODE_FRAME_MAX_BYTES: + return None + raw = target.read_bytes() + except OSError: + return None + if b"\0" in raw: + return None + try: + return raw.decode("utf-8").splitlines() + except UnicodeError: + return None + + +def resolved_location( + file_path: str, start_line: int, end_line: int +) -> Dict[str, Any]: + """Build an engine-derived exact anchor for a finding.""" + existing_code = "" + source_lines = reviewed_source_lines(file_path) + if ( + source_lines is not None + and 1 <= start_line <= end_line <= len(source_lines) + ): + candidate = "\n".join(source_lines[start_line - 1:end_line]) + if ( + len(candidate) <= SUGGESTION_CODE_MAX_LENGTH + and not any( + character not in "\n\t" and ord(character) < 0x20 + for character in candidate + ) + ): + existing_code = candidate + return { + "start_line": start_line, + "end_line": end_line, + "existing_code": existing_code, + } + + +def code_frame( + file_path: str, start_line: int, end_line: Optional[int] = None +) -> Dict[str, Any]: + """Read the lines around a finding's anchor range from the reviewed tree. The excerpt shown in the report is read here, so its line numbers are the tree's own and no agent transcribes them. An unreadable, binary, oversized, or out-of-range target yields an empty excerpt. """ + end_line = start_line if end_line is None else end_line language = code_frame_language(file_path) empty: Dict[str, Any] = { "language": language, - "label": f"{file_path}:{line}", + "label": f"{file_path}:{start_line}-{end_line}", "lines": [], } - target = root() / file_path - try: - if target.is_symlink() or not target.is_file(): - return empty - if target.stat().st_size > CODE_FRAME_MAX_BYTES: - return empty - raw = target.read_bytes() - except OSError: + source_lines = reviewed_source_lines(file_path) + if ( + source_lines is None + or start_line < 1 + or end_line < start_line + or end_line > len(source_lines) + ): return empty - if b"\0" in raw: - return empty - try: - text = raw.decode("utf-8") - except UnicodeError: - return empty - source_lines = text.splitlines() - if line > len(source_lines): - return empty - start = max(1, line - CODE_FRAME_CONTEXT) - end = min(len(source_lines), line + CODE_FRAME_CONTEXT) + start = max(1, start_line - CODE_FRAME_CONTEXT) + end = min(len(source_lines), end_line + CODE_FRAME_CONTEXT) lines: List[Dict[str, Any]] = [] for number in range(start, end + 1): entry: Dict[str, Any] = { "number": number, "text": safe_code_text(source_lines[number - 1]), } - if number == line: + if start_line <= number <= end_line: entry["highlight"] = True lines.append(entry) return { @@ -2767,14 +2909,19 @@ def reportable_finding( ) -> Dict[str, Any]: candidate = record["candidate"] verdict = record.get("verdict") - return { + start_line = int(candidate.get("start_line") or candidate["line"]) + end_line = int(candidate.get("end_line") or candidate["line"]) + location = resolved_location(candidate["file"], start_line, end_line) + finding = { "id": display_id, "file": candidate["file"], - "line": candidate["line"], + "line": end_line, + "location": location, "summary": candidate["summary"], "short_summary": candidate["short_summary"], "failure_scenario": candidate["failure_scenario"], "category": candidate["category"], + "issue_type": candidate["issue_type"], "severity": candidate["severity"], "confidence": candidate["confidence"], "reports": int(candidate.get("reports") or 1), @@ -2785,8 +2932,18 @@ def reportable_finding( "source": candidate.get("source", "finder"), "verdict": verdict["verdict"] if verdict else "UNVERIFIED", "verdict_reasoning": verdict["reasoning"] if verdict else "", - "code": code_frame(candidate["file"], int(candidate["line"])), + "code": code_frame(candidate["file"], start_line, end_line), } + suggestion_code = str(candidate.get("suggestion_code") or "") + if ( + suggestion_code + and location["existing_code"] + and verdict is not None + and verdict.get("suggestion_valid") is True + and suggestion_code != location["existing_code"] + ): + finding["suggestion"] = {"replacement_code": suggestion_code} + return finding def finding_reports(state: Mapping[str, Any], key: str) -> List[str]: @@ -2825,6 +2982,8 @@ def vote_records( entry["reasoning"] = verdict["reasoning"] if verdict.get("duplicate_of"): entry["duplicate_of"] = verdict["duplicate_of"] + if "suggestion_valid" in verdict: + entry["suggestion_valid"] = verdict["suggestion_valid"] records.append(entry) return records @@ -3103,7 +3262,10 @@ def final_tally() -> None: "id": candidate.get("id"), "file": candidate.get("file"), "line": candidate.get("line"), + "start_line": candidate.get("start_line"), + "end_line": candidate.get("end_line"), "category": candidate.get("category"), + "issue_type": candidate.get("issue_type"), "severity": candidate.get("severity"), "confidence": candidate.get("confidence"), "reports": int(candidate.get("reports") or 1), @@ -3114,6 +3276,8 @@ def final_tally() -> None: "failure_scenario": candidate.get("failure_scenario"), "disposition": disposition, } + if candidate.get("suggestion_code"): + entry["suggestion_code"] = candidate["suggestion_code"] if verdict is not None: entry["verdict"] = verdict["verdict"] if disposition == "duplicate": diff --git a/.fabro/workflows/code-review/scripts/publish_pr.py b/.fabro/workflows/code-review/scripts/publish_pr.py index 86f6e9a02..03fefca84 100644 --- a/.fabro/workflows/code-review/scripts/publish_pr.py +++ b/.fabro/workflows/code-review/scripts/publish_pr.py @@ -49,6 +49,7 @@ DEFAULT_BATCH_SIZE = 50 SUMMARY_BUDGET = 65000 GITHUB_BODY_CAP = 65536 SEVERITY_RANK = {"LOW": 0, "MEDIUM": 1, "HIGH": 2} +SEVERITY_EMOJI = {"LOW": "🟡", "MEDIUM": "🟠", "HIGH": "🔴"} CANONICAL_FILE_NAMES = ( "review-manifest.json", "candidate-ledger.jsonl", @@ -68,6 +69,14 @@ UNVERIFIED_NOTE = ( "_This finding comes from a low-effort single-pass review and was not " "independently verified._" ) +PLAUSIBLE_WARNING = ( + "> **Needs confirmation:** The verifier could not fully confirm this " + "finding from the available evidence." +) +UNVERIFIED_WARNING = ( + "> **Not verified:** This finding comes from a low-effort single-pass " + "review and was not independently verified." +) class PublishError(RuntimeError): @@ -201,6 +210,19 @@ def has_diff_position( return any(start <= line <= end for start, end in hunks.get(path, ())) +def has_diff_range( + hunks: Mapping[str, Sequence[Tuple[int, int]]], + path: str, + start_line: int, + end_line: int, +) -> bool: + """True when one RIGHT-side hunk contains the complete range.""" + return any( + hunk_start <= start_line <= end_line <= hunk_end + for hunk_start, hunk_end in hunks.get(path, ()) + ) + + # --- Routing configuration (R3-R5, fail-closed) ------------------------------ @@ -296,6 +318,11 @@ def safe_code_block(code: Mapping[str, Any]) -> List[str]: return body +def raw_code_block(text: str, language: str = "text") -> List[str]: + fence = backtick_fence([text]) + return [fence + language, text, fence] + + def finding_detail_lines(finding: Mapping[str, Any]) -> List[str]: lines: List[str] = [] if finding["summary"].strip() != finding["short_summary"].strip(): @@ -318,29 +345,99 @@ def finding_detail_lines(finding: Mapping[str, Any]) -> List[str]: return lines +def inline_more_lines(finding: Mapping[str, Any]) -> List[str]: + verdict = str(finding["verdict"]).lower().capitalize() + confidence = str(finding["confidence"]).lower().capitalize() + lines = [ + "", + "
", + f"More · {verdict} · {confidence} confidence", + "", + "**Impact:** " + + renderer.escape_markdown(finding["failure_scenario"]), + "", + ] + reasoning = str(finding.get("verdict_reasoning") or "").strip() + if reasoning: + evidence = renderer.escape_markdown(reasoning) + elif finding["verdict"] == "UNVERIFIED": + evidence = "No independent verification ran at this effort level." + else: + evidence = "No verifier reasoning was recorded." + lines.append("**Evidence:** " + evidence) + + metadata: List[str] = [] + reports = finding.get("reports") + reporters = finding.get("reporters") or [] + if isinstance(reports, int) and reports > 1: + report_text = f"- Reported by {reports} review passes" + if reporters: + report_text += ": " + renderer.escape_markdown( + ", ".join(str(reporter) for reporter in reporters) + ) + metadata.append(report_text) + + rule_ids = finding.get("rule_ids") or [] + if rule_ids: + label = "Rule" if len(rule_ids) == 1 else "Rules" + metadata.append( + f"- {label}: " + + ", ".join(renderer.code_span(rule_id) for rule_id in rule_ids) + ) + + for anchor in finding.get("anchors") or []: + location = f"{anchor['file']}:{anchor['line']}" + metadata.append( + f"- Related location: {renderer.code_span(location)} " + f"({renderer.escape_markdown(anchor['category'])}, " + f"{renderer.escape_markdown(anchor['id'])})" + ) + + if metadata: + lines.extend(["", *metadata]) + lines.extend(["", "
"]) + return lines + + def inline_comment_body(finding: Mapping[str, Any], review_id: str) -> str: tag = comment_tag(review_id, finding["id"]) + issue_type = str(finding["issue_type"]).lower().capitalize() lines = [ f"", "", - f"**{finding['severity']} · {finding['category']}** — " + f"**{SEVERITY_EMOJI[finding['severity']]} {issue_type}** — " + renderer.escape_markdown(finding["short_summary"]), - *finding_detail_lines(finding), ] - meta = f"Verdict {finding['verdict']} · confidence {finding['confidence']}" - rule_ids = finding.get("rule_ids") or [] - if rule_ids: - meta += " · rule " + ", ".join( - renderer.code_span(rule_id) for rule_id in rule_ids + if finding["summary"].strip() != finding["short_summary"].strip(): + lines.extend(["", renderer.escape_markdown(finding["summary"])]) + if finding["verdict"] == "PLAUSIBLE": + lines.extend(["", PLAUSIBLE_WARNING]) + elif finding["verdict"] == "UNVERIFIED": + lines.extend(["", UNVERIFIED_WARNING]) + suggestion = finding.get("suggestion") + if isinstance(suggestion, dict): + lines.extend( + [ + "", + *raw_code_block(suggestion["replacement_code"], "suggestion"), + ] ) - lines.extend(["", f"_{meta}_"]) + lines.extend(inline_more_lines(finding)) return "\n".join(lines) def summary_section( finding: Mapping[str, Any], reason_text: Optional[str] ) -> str: - location = renderer.code_span(f"{finding['file']}:{finding['line']}") + location_data = finding["location"] + start_line = location_data["start_line"] + end_line = location_data["end_line"] + location_text = ( + f"{finding['file']}:{start_line}" + if start_line == end_line + else f"{finding['file']}:{start_line}-{end_line}" + ) + location = renderer.code_span(location_text) facts = [location] if reason_text: facts.append(reason_text) @@ -353,7 +450,8 @@ def summary_section( + ", ".join(renderer.code_span(rule_id) for rule_id in rule_ids) ) lines = [ - f"### {finding['id']} · {finding['severity']} {finding['category']} — " + f"### {finding['id']} · {finding['severity']} " + f"{finding['issue_type']} / {finding['category']} — " + renderer.escape_markdown(finding["short_summary"]), "", " · ".join(facts), @@ -362,6 +460,22 @@ def summary_section( excerpt = safe_code_block(finding["code"]) if excerpt: lines.extend(["", *excerpt]) + suggestion = finding.get("suggestion") + if isinstance(suggestion, dict): + lines.extend( + [ + "", + "
Suggested change", + "", + "**Before:**", + *raw_code_block(location_data["existing_code"]), + "", + "**After:**", + *raw_code_block(suggestion["replacement_code"]), + "", + "
", + ] + ) return "\n".join(lines) @@ -597,12 +711,19 @@ def command_plan(args: argparse.Namespace) -> int: # no-position reason and routing can never hide a placement. placements: List[Dict[str, Any]] = [] for finding in findings: + location = finding["location"] + start_line = location["start_line"] + end_line = location["end_line"] base_entry = { "finding_id": finding["id"], "path": finding["file"], - "line": finding["line"], + "line": end_line, + "start_line": start_line, + "end_line": end_line, } - if not has_diff_position(hunks, finding["file"], finding["line"]): + if not has_diff_range( + hunks, finding["file"], start_line, end_line + ): placements.append( { **base_entry, @@ -811,6 +932,24 @@ def validate_plan_document( line = entry.get("line") if isinstance(line, bool) or not isinstance(line, int) or line < 1: fail(f"plan {field}.line must be a positive integer") + start_line = entry.get("start_line") + end_line = entry.get("end_line") + if ( + isinstance(start_line, bool) + or not isinstance(start_line, int) + or start_line < 1 + ): + fail(f"plan {field}.start_line must be a positive integer") + if ( + isinstance(end_line, bool) + or not isinstance(end_line, int) + or end_line < start_line + or end_line != line + ): + fail( + f"plan {field}.end_line must end at its line and not precede " + "start_line" + ) body = require_text(entry.get("body"), f"{field}.body", GITHUB_BODY_CAP) placement = entry.get("placement") if placement == "inline": @@ -1014,15 +1153,19 @@ class BatchPoster: self.batches_succeeded = 0 def post_review(self, finding_ids: Sequence[str]) -> Optional[int]: - comments = [ - { - "path": self.by_id[finding_id]["path"], - "line": self.by_id[finding_id]["line"], + comments: List[Dict[str, Any]] = [] + for finding_id in finding_ids: + entry = self.by_id[finding_id] + comment: Dict[str, Any] = { + "path": entry["path"], + "line": entry["end_line"], "side": "RIGHT", - "body": self.by_id[finding_id]["body"], + "body": entry["body"], } - for finding_id in finding_ids - ] + if entry["start_line"] != entry["end_line"]: + comment["start_line"] = entry["start_line"] + comment["start_side"] = "RIGHT" + comments.append(comment) # No review body: the run tag lives in the sticky summary and in # every inline comment's identity tag, so body text here would # only add a noise bubble to the PR timeline (R12). diff --git a/.fabro/workflows/code-review/scripts/render_report.py b/.fabro/workflows/code-review/scripts/render_report.py index 796f654ac..6a9f5d3c5 100644 --- a/.fabro/workflows/code-review/scripts/render_report.py +++ b/.fabro/workflows/code-review/scripts/render_report.py @@ -14,6 +14,7 @@ Python 3.9-compatible. Standard library only. from __future__ import annotations import json +import hashlib import os import re import sys @@ -22,7 +23,7 @@ from typing import Any, Dict, List, Mapping, NoReturn, Sequence, Tuple from urllib.parse import quote -CANONICAL_SCHEMA_VERSION = 3 +CANONICAL_SCHEMA_VERSION = 4 TEMPLATE_RELATIVE_PATH = ("..", "templates", "report.html") PAYLOAD_PLACEHOLDER = "__CODE_REVIEW_PAYLOAD__" @@ -35,6 +36,15 @@ CATEGORIES = ( "conventions", "test-coverage", ) +ISSUE_TYPES = ( + "bug", + "security", + "performance", + "maintainability", + "test", + "style", + "documentation", +) SEVERITIES = ("HIGH", "MEDIUM", "LOW") FINDING_VERDICTS = ("CONFIRMED", "PLAUSIBLE", "UNVERIFIED") VOTE_VERDICTS = ("CONFIRMED", "PLAUSIBLE", "REFUTED") @@ -58,6 +68,7 @@ COMPILED_RULE_ID_RE = re.compile( ) MAX_TEXT = 8000 MAX_RULE_IDS_PER_FINDING = 50 +MAX_LOCATION_LINES = 50 class RenderError(RuntimeError): @@ -211,8 +222,8 @@ def validate_code(value: object, field: str) -> Dict[str, Any]: line["highlight"] = True highlighted += 1 normalized.append(line) - if normalized and highlighted != 1: - die(f"{field} must highlight exactly one line") + if normalized and highlighted < 1: + die(f"{field} must highlight at least one line") return { "language": code["language"], "label": code["label"], @@ -230,12 +241,49 @@ def validate_finding(value: object, index: int) -> Dict[str, Any]: line = positive_int(finding.get("line"), f"{field}.line") if finding.get("category") not in CATEGORIES: die(f"{field}.category is not in the closed list") + if finding.get("issue_type") not in ISSUE_TYPES: + die(f"{field}.issue_type is not in the closed list") if finding.get("severity") not in SEVERITIES: die(f"{field}.severity is invalid") if finding.get("confidence") not in SEVERITIES: die(f"{field}.confidence is invalid") if finding.get("verdict") not in FINDING_VERDICTS: die(f"{field}.verdict is invalid") + location = as_map(finding.get("location")) + start_line = positive_int( + location.get("start_line"), f"{field}.location.start_line" + ) + end_line = positive_int( + location.get("end_line"), f"{field}.location.end_line" + ) + if start_line > end_line: + die(f"{field}.location starts after it ends") + if end_line - start_line + 1 > MAX_LOCATION_LINES: + die(f"{field}.location spans more than {MAX_LOCATION_LINES} lines") + if end_line != line: + die(f"{field}.line must equal location.end_line") + normalized_location = { + "start_line": start_line, + "end_line": end_line, + "existing_code": safe_text( + location.get("existing_code"), + f"{field}.location.existing_code", + ), + } + suggestion = finding.get("suggestion") + normalized_suggestion: Optional[Dict[str, str]] = None + if suggestion is not None: + suggestion_record = as_map(suggestion) + replacement = safe_text( + suggestion_record.get("replacement_code"), + f"{field}.suggestion.replacement_code", + allow_empty=False, + ) + if not normalized_location["existing_code"]: + die(f"{field}.suggestion has no exact existing code anchor") + if replacement == normalized_location["existing_code"]: + die(f"{field}.suggestion does not change the anchored code") + normalized_suggestion = {"replacement_code": replacement} reporters = finding.get("reporters") if not isinstance(reporters, list) or not all( isinstance(item, str) for item in reporters @@ -270,10 +318,21 @@ def validate_finding(value: object, index: int) -> Dict[str, Any]: "category": record["category"], } ) - return { + normalized_code = validate_code(finding.get("code"), f"{field}.code") + if normalized_code["lines"]: + highlighted = { + entry["number"] + for entry in normalized_code["lines"] + if entry.get("highlight") + } + expected = set(range(start_line, end_line + 1)) + if highlighted != expected: + die(f"{field}.code highlights do not match its location") + normalized = { "id": display_id, "file": path, "line": line, + "location": normalized_location, "summary": safe_text( finding.get("summary"), f"{field}.summary", allow_empty=False ), @@ -288,6 +347,7 @@ def validate_finding(value: object, index: int) -> Dict[str, Any]: allow_empty=False, ), "category": finding["category"], + "issue_type": finding["issue_type"], "severity": finding["severity"], "confidence": finding["confidence"], "reports": positive_int(finding.get("reports"), f"{field}.reports"), @@ -299,8 +359,11 @@ def validate_finding(value: object, index: int) -> Dict[str, Any]: "verdict_reasoning": safe_text( finding.get("verdict_reasoning"), f"{field}.verdict_reasoning" ), - "code": validate_code(finding.get("code"), f"{field}.code"), + "code": normalized_code, } + if normalized_suggestion is not None: + normalized["suggestion"] = normalized_suggestion + return normalized def validate_findings(value: object) -> List[Dict[str, Any]]: @@ -333,6 +396,8 @@ def validate_ledger(records: Sequence[Mapping[str, Any]]) -> List[Dict[str, Any] positive_int(record.get("line"), f"{field}.line") if record.get("category") not in CATEGORIES: die(f"{field}.category is not in the closed list") + if record.get("issue_type") not in ISSUE_TYPES: + die(f"{field}.issue_type is not in the closed list") if record.get("disposition") not in DISPOSITIONS: die(f"{field}.disposition is invalid") validated.append(dict(record)) @@ -351,6 +416,10 @@ def validate_votes(records: Sequence[Mapping[str, Any]]) -> List[Dict[str, Any]] if record.get("verdict") not in VOTE_VERDICTS: die(f"{field}.verdict is invalid") safe_text(record.get("reasoning"), f"{field}.reasoning") + if "suggestion_valid" in record and not isinstance( + record.get("suggestion_valid"), bool + ): + die(f"{field}.suggestion_valid must be a boolean") validated.append(dict(record)) return validated @@ -582,6 +651,12 @@ def code_block(code: Mapping[str, Any]) -> List[str]: return body +def fenced_text(value: str, language: str = "text") -> List[str]: + longest = max((len(run) for run in re.findall(r"`+", value)), default=0) + fence = "`" * max(4, longest + 1) + return [fence + language, value, fence] + + def describe_target(manifest: Mapping[str, Any]) -> str: mode = str(manifest.get("mode")) if mode == "files": @@ -601,11 +676,19 @@ def revision_summary(manifest: Mapping[str, Any]) -> str: def finding_markdown(finding: Mapping[str, Any]) -> List[str]: - location = f"{finding['file']}:{finding['line']}" + location_data = finding["location"] + start_line = location_data["start_line"] + end_line = location_data["end_line"] + location = ( + f"{finding['file']}:{start_line}" + if start_line == end_line + else f"{finding['file']}:{start_line}-{end_line}" + ) rule_ids = finding.get("rule_ids") or [] lines = [ f"### {finding['id']} · {finding['severity']} " - f"{finding['category']} — {escape_markdown(finding['short_summary'])}", + f"{finding['issue_type']} / {finding['category']} — " + f"{escape_markdown(finding['short_summary'])}", "", f"{code_span(location)} · verdict {finding['verdict']} · " f"confidence {finding['confidence']} · reported by " @@ -636,6 +719,22 @@ def finding_markdown(finding: Mapping[str, Any]) -> List[str]: reasoning = str(finding.get("verdict_reasoning") or "").strip() if reasoning: lines.extend(["", f"**Verifier.** {escape_markdown(reasoning)}"]) + suggestion = finding.get("suggestion") + if isinstance(suggestion, dict): + lines.extend( + [ + "", + "
Suggested change", + "", + "**Before:**", + *fenced_text(location_data["existing_code"]), + "", + "**After:**", + *fenced_text(suggestion["replacement_code"]), + "", + "
", + ] + ) excerpt = code_block(finding["code"]) if excerpt: lines.extend(["", *excerpt]) @@ -841,7 +940,9 @@ def jsonl_line(finding: Mapping[str, Any]) -> str: "id", "file", "line", + "location", "category", + "issue_type", "severity", "confidence", "verdict", @@ -854,6 +955,8 @@ def jsonl_line(finding: Mapping[str, Any]) -> str: "source", ) } + if finding.get("suggestion") is not None: + record["suggestion"] = finding["suggestion"] return json.dumps(record, ensure_ascii=False, separators=(",", ":")) @@ -889,14 +992,19 @@ def sarif_uri(path: str) -> str: return quote(path, safe="/") -def sarif_location(path: str, line: int) -> Dict[str, Any]: +def sarif_location( + path: str, start_line: int, end_line: Optional[int] = None +) -> Dict[str, Any]: + region: Dict[str, Any] = {"startLine": start_line} + if end_line is not None and end_line != start_line: + region["endLine"] = end_line return { "physicalLocation": { "artifactLocation": { "uri": sarif_uri(path), "uriBaseId": "%SRCROOT%", }, - "region": {"startLine": line}, + "region": region, } } @@ -970,22 +1078,36 @@ def sarif_result( ) if finding["verdict"] == "UNVERIFIED": message += "\n\n" + SARIF_UNVERIFIED_NOTE + location = finding["location"] + start_line = location["start_line"] + end_line = location["end_line"] + fingerprint_anchor = ( + location["existing_code"] + or f"{start_line}:{end_line}" + ) + fingerprint = hashlib.sha256( + ( + f"{finding['file']}\0{finding['issue_type']}\0" + f"{fingerprint_anchor}" + ).encode("utf-8") + ).hexdigest() result: Dict[str, Any] = { "ruleId": primary, "ruleIndex": rule_indices[primary], "level": SARIF_LEVELS[finding["severity"]], "message": {"text": message}, - "locations": [sarif_location(finding["file"], finding["line"])], + "locations": [ + sarif_location(finding["file"], start_line, end_line) + ], "partialFingerprints": { - "codeReviewIdentity/v1": ( - f"{finding['file']}:{finding['line']}:{finding['category']}" - ) + "codeReviewIdentity/v2": fingerprint }, "properties": { key: finding[key] for key in ( "id", "category", + "issue_type", "severity", "confidence", "verdict", @@ -997,6 +1119,32 @@ def sarif_result( ) }, } + suggestion = finding.get("suggestion") + if isinstance(suggestion, dict): + result["fixes"] = [ + { + "description": {"text": "Apply the verified suggested change"}, + "artifactChanges": [ + { + "artifactLocation": { + "uri": sarif_uri(finding["file"]), + "uriBaseId": "%SRCROOT%", + }, + "replacements": [ + { + "deletedRegion": { + "startLine": start_line, + "endLine": end_line, + }, + "insertedContent": { + "text": suggestion["replacement_code"] + }, + } + ], + } + ], + } + ] anchors = finding.get("anchors") or [] if anchors: result["relatedLocations"] = [ diff --git a/.fabro/workflows/code-review/specs/report-spec.md b/.fabro/workflows/code-review/specs/report-spec.md index 7c22c5f8d..28b213945 100644 --- a/.fabro/workflows/code-review/specs/report-spec.md +++ b/.fabro/workflows/code-review/specs/report-spec.md @@ -7,7 +7,7 @@ report. ## Canonical files -The canonical bundle is schema version 3. +The canonical bundle is schema version 4. - `review-manifest.json` identifies the review, target, revision, request, completion status, counts, and canonical file set. At the rule-mapped @@ -22,8 +22,10 @@ The canonical bundle is schema version 3. carries the candidate's applicable `rule_ids` (empty outside the rule-mapped tiers). - `findings.json` contains only the reportable subset. It is the authoritative - finding list. Each reported finding also carries a `code` excerpt, which the - ledger's candidate records do not, and its `rule_ids`. + finding list. Each reported finding carries its orthogonal `issue_type`, an + engine-derived `location` with the exact original code and start/end lines, + a highlighted `code` excerpt, and its `rule_ids`. A verified replacement is + stored as optional `suggestion.replacement_code`. - `coverage.json` records what the review dispatched, what returned, what was rejected for failing the finding contract, and what a cap dropped. At the rule-mapped tiers it also records the authoritative target-file list, the @@ -39,9 +41,10 @@ The canonical bundle is schema version 3. and cap drops -- also emitted into the workflow context so calibration across many runs can read it from the event log. - `votes.jsonl` contains one record for each dispatched verification, with the - exact claim shown to the verifier (including its claimed `rule_ids` and the - file's effective checks at the rule-mapped tiers), plus its verdict and - reasoning when it completed. + exact claim shown to the verifier (including its location, proposed + replacement, claimed `rule_ids`, and the file's effective checks at the + rule-mapped tiers), plus its verdict and reasoning when it completed. A vote + over a proposed replacement also carries `suggestion_valid`. ## Derived files @@ -117,6 +120,12 @@ verifier's instructions, never the arithmetic. At `low`, verification is skipped by design: findings carry `verdict: "UNVERIFIED"` and the reports say so. +A proposed replacement is independent of the keep verdict. The verifier must +return `suggestion_valid: true`, the engine must be able to read the exact +original range from the unchanged reviewed tree, and the replacement must +differ from it. Low-effort findings never carry suggestions because they have +no independent verification. + ## Deduplication and ranking A candidate's identity is its normalized file, line, and category. Two angles @@ -146,12 +155,16 @@ categories (`reuse`, `simplification`, `efficiency`, `altitude`, report count, then confidence, then file and line. The report cap cuts from the bottom, and everything cut is in the ledger as `deferred-by-cap`. -## Source excerpts +## Locations, source excerpts, and suggestions -A finding's `code` excerpt is not agent-quoted: `final-tally` reads the lines -around the finding from the reviewed tree, so the line numbers are the tree's -own and no agent transcribes them. The excerpt is omitted when the file is -unreadable, binary, oversized, or the line is out of range. +A finder supplies a bounded `start_line`/`end_line` range. `final-tally` reads +that range from the reviewed tree and records its exact text as +`location.existing_code`; the agent never supplies the canonical original +text. The adjacent `code` excerpt is read the same way and highlights the +complete range. Exact text and the excerpt are omitted when the file is +unreadable, binary, oversized, or the range is invalid. A proposed +`suggestion_code` becomes canonical only after the verifier approves it and +the exact original text is available. ## HTML rendering @@ -174,11 +187,14 @@ validated bundle: covers every check ID any result cites. - Severity maps to level: `HIGH` is `error`, `MEDIUM` is `warning`, `LOW` is `note`. -- Each result's location is the finding's file and line relative to +- Each result's location is the finding's file and line range relative to `%SRCROOT%`; anchors become related locations. The finding's identity, - category, severity, confidence, verdict, reports, reporters, rule IDs, - anchors, and source are result properties, and the file, line, and - category form a stable partial fingerprint. + category, issue type, severity, confidence, verdict, reports, reporters, + rule IDs, anchors, and source are result properties. File, issue type, and + exact original code form a stable hashed partial fingerprint, falling back + to the line range when source text is unavailable. +- A verified suggestion becomes a SARIF `fix` that replaces the complete + location range. - An `UNVERIFIED` finding (the `low` tier) says so in its result message and carries the verdict in its properties. - The run's automation ID is `code-review/`, and the run properties diff --git a/.fabro/workflows/code-review/templates/report.html b/.fabro/workflows/code-review/templates/report.html index 49de0cf6d..463fc1bd8 100644 --- a/.fabro/workflows/code-review/templates/report.html +++ b/.fabro/workflows/code-review/templates/report.html @@ -203,6 +203,19 @@ function renderCode(code) { return details; } +function renderSuggestedChange(finding) { + if (!finding.suggestion || !finding.location || + !finding.location.existing_code) return null; + const details = el("details"); + details.appendChild(el("summary", null, "Suggested change")); + details.appendChild(el("p", "label", "Before")); + details.appendChild(el("pre", "code", finding.location.existing_code)); + details.appendChild(el("p", "label", "After")); + details.appendChild(el("pre", "code", + finding.suggestion.replacement_code || "")); + return details; +} + function renderFinding(finding) { const card = el("article", "finding"); const head = el("div", "finding-head"); @@ -213,6 +226,7 @@ function renderFinding(finding) { chips.appendChild(el("span", "chip sev-" + finding.severity, finding.severity + " severity")); chips.appendChild(el("span", "chip", finding.category)); + chips.appendChild(el("span", "chip", finding.issue_type)); if ((finding.rule_ids || []).length) { chips.appendChild(el("span", "chip", "rule " + finding.rule_ids.join(", "))); @@ -222,7 +236,11 @@ function renderFinding(finding) { chips.appendChild(el("span", "chip", finding.reports + " report(s): " + (finding.reporters || []).join(", "))); card.appendChild(chips); - card.appendChild(el("p", "location", finding.file + ":" + finding.line)); + const loc = finding.location || {}; + const span = loc.start_line && loc.end_line && loc.start_line !== loc.end_line + ? loc.start_line + "-" + loc.end_line + : (loc.end_line || finding.line); + card.appendChild(el("p", "location", finding.file + ":" + span)); if ((finding.anchors || []).length) { card.appendChild(el("p", "location", "Also reported at " + finding.anchors.map(a => a.file + ":" + a.line + " (" + a.category + @@ -242,6 +260,8 @@ function renderFinding(finding) { verifier.appendChild(el("p", null, finding.verdict_reasoning)); card.appendChild(verifier); } + const suggestion = renderSuggestedChange(finding); + if (suggestion) card.appendChild(suggestion); const code = renderCode(finding.code); if (code) card.appendChild(code); return card;