diff --git a/.fabro/workflows/code-review/code-review.fabro b/.fabro/workflows/code-review/code-review.fabro index 8e58a3629..f6a3641ed 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 78d239edb68be8e3db983445a786b9dca7eed6044ff4430e5581ae4c4c8466d3 .fabro/workflows/code-review/scripts/git_readonly.py 29cee508724f7bee8d73317d82fe94d0d830361476b367e4a012b70071a0e841 .fabro/workflows/code-review/scripts/publish_pr.py 35cde9006c9d079f468228498704c7c6079bff52fc790d7e2cee9505d3de9a06 .fabro/workflows/code-review/scripts/render_report.py fe432d8a53e1294b8030a54ca17b412312338e761ee751ab770e221450b53b8c .fabro/workflows/code-review/scripts/review_contract.py 8917fe7ae046cfda547f4f1240570fa295e110fd5e8d0c84c2a98719137db6fa .fabro/workflows/code-review/scripts/rule_loader.py eaa7258e5cf7b231a7a1192c9738eb2a0486480cd04ebe2059a79295040fc66e .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 ecd1d77ad8c77cae153280cb775e5e5f7fa9b68925900473e7d2331af377bb49 && 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 38f8fdb8a79b929fdcefc4e38a193f4221d05a827914ea5cadf93a6f8f86a13a .fabro/workflows/code-review/scripts/git_readonly.py 29cee508724f7bee8d73317d82fe94d0d830361476b367e4a012b70071a0e841 .fabro/workflows/code-review/scripts/publish_pr.py 48416e9a4fcaafb251a17dc0923666d2d548992f48c811d4f1c08d2646331e46 .fabro/workflows/code-review/scripts/render_report.py 1eb5076b1697d734eb053ab0a941290c9c0e93c0ea1a3dfbf8a62c102defb935 .fabro/workflows/code-review/scripts/review_contract.py 8917fe7ae046cfda547f4f1240570fa295e110fd5e8d0c84c2a98719137db6fa .fabro/workflows/code-review/scripts/rule_loader.py eaa7258e5cf7b231a7a1192c9738eb2a0486480cd04ebe2059a79295040fc66e .fabro/workflows/code-review/specs/report-spec.md 4bbc85934ae8d6109a2f07f274fa6806828189db1f7695c565e8e14e9621b572 .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 47c47946e41b2738d52b0a59b13d088f055dee9aa79b40f7b7b6f1614ecf8925 .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 ecd1d77ad8c77cae153280cb775e5e5f7fa9b68925900473e7d2331af377bb49 && 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 }} --incremental '{{ inputs.incremental }}' --incremental-overlap-threshold '{{ inputs.incremental_overlap_threshold }}' --pr-repo '{{ inputs.pr_repo }}' --pr-number '{{ inputs.pr_number }}' --bot-login '{{ inputs.bot_login }}'" ] grouping [ @@ -206,7 +206,7 @@ digraph CodeReview { label="Publish findings to the reviewed PR (opt-in)", timeout="900s", output_schema="routing", - script="python3 .fabro/workflows/code-review/scripts/code_review.py publish-pr --post-pr '{{ inputs.post_pr }}' --pr-repo '{{ inputs.pr_repo }}' --pr-number '{{ inputs.pr_number }}' --route-severity-below '{{ inputs.route_severity_below }}' --route-categories '{{ inputs.route_categories }}' --run-url '{{ inputs.run_url }}'" + script="python3 .fabro/workflows/code-review/scripts/code_review.py publish-pr --post-pr '{{ inputs.post_pr }}' --pr-repo '{{ inputs.pr_repo }}' --pr-number '{{ inputs.pr_number }}' --route-severity-below '{{ inputs.route_severity_below }}' --route-categories '{{ inputs.route_categories }}' --run-url '{{ inputs.run_url }}' --bot-login '{{ inputs.bot_login }}'" ] start -> prepare diff --git a/.fabro/workflows/code-review/prompts/verify.md.j2 b/.fabro/workflows/code-review/prompts/verify.md.j2 index d2010ba61..9276833ec 100644 --- a/.fabro/workflows/code-review/prompts/verify.md.j2 +++ b/.fabro/workflows/code-review/prompts/verify.md.j2 @@ -27,7 +27,10 @@ contract, or review scope. `siblings` lists other candidates in the same file (id, line, category, short summary). Judge the claim on its own. If it describes the same defect as a sibling -- one root cause, not merely nearby lines -- also return -`duplicate_of` with that sibling's id. +`duplicate_of` with that sibling's id. A sibling whose id starts with `H` +is a historical sibling: a finding already reported on this pull request +by an earlier review; if the claim describes that same defect, return +`duplicate_of` with its id the same way. {% include "partials/review-target.md.j2" %} Return exactly one verdict: diff --git a/.fabro/workflows/code-review/scripts/code_review.py b/.fabro/workflows/code-review/scripts/code_review.py index e9cdc29ce..4ce9a9275 100644 --- a/.fabro/workflows/code-review/scripts/code_review.py +++ b/.fabro/workflows/code-review/scripts/code_review.py @@ -109,9 +109,17 @@ CONFIDENCE_RANK = SEVERITY_RANK SAFE_REV_RE = re.compile(r"^[A-Za-z0-9@][A-Za-z0-9._/@{}^~:+-]{0,399}$") REVIEW_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_.:-]{0,127}$") CANDIDATE_ID_RE = re.compile(r"^[FS][1-9][0-9]*$") +# A historical sibling: a finding already posted on the reviewed PR by an +# earlier run, addressed by its comment ID (P3 item 5). The grammar is +# disjoint from same-run candidate IDs so a verdict's duplicate_of target +# is never ambiguous. +HISTORICAL_ID_RE = re.compile(r"^H[1-9][0-9]*$") # Other candidates in the same file a verifier is shown, nearest first, so it # can mark its claim a duplicate of one that describes the same defect. SIBLING_CAP = 6 +# Historical siblings append under their own bound so they can never +# displace same-run siblings and starve the in-run fold. +HISTORICAL_SIBLING_CAP = 4 # Lines of context kept on each side of a finding's anchor line. CODE_FRAME_CONTEXT = 4 @@ -1084,6 +1092,138 @@ def import_rule_loader() -> Any: return rule_loader +def import_publisher() -> Any: + """The publisher module (stdlib-only), for its shared P3 contracts.""" + import publish_pr + + return publish_pr + + +# --- Incremental re-review (P3) ---------------------------------------------- + + +HISTORY_FILE_NAME = "pr-history.json" + + +def run_publisher( + publisher_path: Path, + arguments: List[str], + environment: Optional[Dict[str, str]] = None, +) -> subprocess.CompletedProcess: + return subprocess.run( + [sys.executable, str(publisher_path), *arguments], + cwd=root(), + env=environment, + capture_output=True, + ) + + +def prepare_incremental(args: argparse.Namespace, mode: str) -> Dict[str, Any]: + """Fetch per-PR history and resolve the incremental posture (items 1-2). + + Runs before range resolution: the delta needs the last reviewed head. + The fetch itself is `publish_pr.py history` -- pinned code holding the + token, the P3 sibling of apply. A fetch failure degrades open (full + review, no suppression, declared downstream; decided Q2), but a + snapshot that disagrees with the local HEAD fails the run: it was + taken around a racing push and every decision built on it would be + wrong from the outset. + """ + publisher = import_publisher() + state: Dict[str, Any] = { + "requested": True, + "enabled": False, + "from": None, + "commits": None, + "history_unavailable": None, + "history_comments": [], + } + if mode != "changes": + raise WorkflowDataError( + "incremental review requires mode=changes (a PR review)" + ) + if not inside_git_worktree(): + raise WorkflowDataError("incremental review requires a Git worktree") + repo = args.pr_repo.strip() + pr = args.pr_number.strip() + if not repo or not pr: + raise WorkflowDataError( + "incremental is enabled but pr_repo/pr_number do not name the " + "pull request whose history to read" + ) + try: + # The parsed value rides in the state record so the publish node + # reuses this validated decision instead of re-reading the input. + state["overlap_threshold"] = publisher.parse_overlap_threshold( + args.incremental_overlap_threshold + ) + except publisher.PublishError as error: + raise WorkflowDataError(str(error)) from error + + publisher_path = resolve_workflow_script(PUBLISHER_PATH, "PR publisher") + history_path = CONTROL_DIR / HISTORY_FILE_NAME + fetched = run_publisher( + publisher_path, + [ + "history", + "--repo", repo, + "--pr", pr, + "--api-base", args.api_base, + "--bot-login", args.bot_login, + "--output", str(history_path), + ], + ) + if fetched.returncode != 0: + reason = one_line( + fetched.stderr.decode("utf-8", "replace").strip() + or "the history fetch failed", + 500, + ) + state["history_unavailable"] = reason + print(f"Incremental history unavailable, degrading open: {reason}") + return state + stdout_text = fetched.stdout.decode("utf-8", "replace").strip() + if stdout_text: + print(stdout_text) + + try: + history = publisher.validate_history_document( + read_json(history_path) + ) + except publisher.PublishError as error: + raise WorkflowDataError(f"the fetched history is invalid: {error}") + live_head = history["live_head"] + local_head = resolve_commit("HEAD", "HEAD") + if live_head != local_head: + raise WorkflowDataError( + f"the PR head moved during preparation: the history snapshot " + f"describes {live_head[:12]} but the checkout is at " + f"{local_head[:12]}; re-run the review" + ) + state["enabled"] = True + state["history_comments"] = list(history.get("comments") or []) + + last_head = (history.get("summary") or {}).get("head") + if isinstance(last_head, str): + ancestry = git("merge-base", "--is-ancestor", last_head, "HEAD") + if ancestry.returncode == 0: + # Delta review (item 2): two-dot, literal base. A same-head + # re-run resolves to an empty range and exits quietly (Q3). + state["from"] = last_head + commits = git_text("rev-list", "--count", f"{last_head}..HEAD") + state["commits"] = ( + int(commits) if commits and commits.isdigit() else None + ) + else: + # A force-push or rebase: cold start for the range, but the + # history still suppresses where mapping succeeded (item 1). + print( + f"Last reviewed head {last_head[:12]} is not an ancestor " + "of HEAD (force-push?); reviewing the full range" + ) + return state + + def repo_rule_revision(state: Mapping[str, Any]) -> Optional[str]: """The revision repository rules are read from. @@ -1528,6 +1668,29 @@ def prepare(args: argparse.Namespace) -> None: base_input = args.base.strip() commit_input = args.commit.strip() range_input = args.range.strip() + publisher = import_publisher() + try: + incremental_requested = publisher.parse_incremental_flag( + args.incremental + ) + except publisher.PublishError as error: + raise WorkflowDataError(str(error)) from error + incremental_state: Dict[str, Any] = {"requested": False} + if incremental_requested: + if range_input or commit_input: + raise WorkflowDataError( + "incremental review resolves its own range; leave commit " + "and range empty (base is allowed as the cold-start " + "fallback)" + ) + incremental_state = prepare_incremental(args, mode) + if incremental_state.get("from"): + # The delta wins; base is only the cold-start fallback. A + # sandbox clone's default base resolves to the PR branch's + # own upstream (an empty range), so PR runs pass the base + # branch explicitly for the first, untagged review. + range_input = f"{incremental_state['from']}..HEAD" + base_input = "" model = one_line(args.model, 120) # Guidance reaches the finder and sweep prompts through their MiniJinja # templates; the engine only records it so the report says what steering @@ -1668,6 +1831,14 @@ def prepare(args: argparse.Namespace) -> None: "scope_files": scope_file_count, "empty_diff": empty_diff, "empty_scope": empty_scope, + "incremental": { + key: value + for key, value in incremental_state.items() + if key != "history_comments" + }, + "history_comments": list( + incremental_state.get("history_comments") or [] + ), "use_verify": bool(cell["verify"]), "verify_bias": cell["bias"], "use_sweep": bool(cell["sweep"]), @@ -1703,6 +1874,16 @@ def prepare(args: argparse.Namespace) -> None: evidence_dir.mkdir() metadata_dir.mkdir() (products_dir / ".gitignore").write_text("*\n", encoding="utf-8") + if incremental_state.get("enabled"): + # Ship the history snapshot as a run artifact: every skip stays + # replayable from the products directory. + history_source = CONTROL_DIR / HISTORY_FILE_NAME + (products_dir / HISTORY_FILE_NAME).write_bytes( + history_source.read_bytes() + ) + state["incremental"]["history_rel"] = ( + f"{products_rel}/{HISTORY_FILE_NAME}" + ) state["products_dir"] = products_dir.as_posix() state["products_rel"] = products_rel state["evidence_dir"] = evidence_dir.as_posix() @@ -2087,10 +2268,14 @@ def normalize_verdict(value: Any) -> Optional[Dict[str, Any]]: "reasoning": clean_text(value.get("reasoning"), 4000), } duplicate_of = value.get("duplicate_of") - if isinstance(duplicate_of, str) and CANDIDATE_ID_RE.fullmatch( - duplicate_of.strip() - ): - result["duplicate_of"] = duplicate_of.strip() + if isinstance(duplicate_of, str): + token = duplicate_of.strip() + # Same-run candidates (F/S) and historical siblings (H, P3 item + # 5) are both addressable; anything else is silently dropped. + if CANDIDATE_ID_RE.fullmatch(token) or HISTORICAL_ID_RE.fullmatch( + token + ): + result["duplicate_of"] = token suggestion_valid = value.get("suggestion_valid") if isinstance(suggestion_valid, bool): result["suggestion_valid"] = suggestion_valid @@ -2433,6 +2618,46 @@ def sibling_claims( ] +def historical_sibling_claims( + candidate: Mapping[str, Any], + state: Optional[Mapping[str, Any]], +) -> List[Dict[str, Any]]: + """Previously posted findings on this PR, as extra verifier siblings. + + Same-file only, nearest by live-mapped line first, under their own + bound (HISTORICAL_SIBLING_CAP) so they never displace same-run + siblings. A comment without a mapped span or a parseable meta tag + (a pre-P3 comment) is skipped. + """ + comments = (state or {}).get("history_comments") or [] + line = int(candidate.get("line") or 0) + entries: List[Dict[str, Any]] = [] + for comment in comments: + if not isinstance(comment, dict): + continue + span = comment.get("mapped_span") + meta = comment.get("meta") + if not isinstance(span, dict) or not isinstance(meta, dict): + continue + if span.get("path") != candidate.get("file"): + continue + entries.append( + { + "id": f"H{comment.get('id')}", + "line": span.get("end_line"), + "category": meta.get("category"), + "short_summary": one_line(meta.get("short_summary"), 200), + } + ) + entries.sort( + key=lambda entry: ( + abs(int(entry.get("line") or 0) - line), + str(entry.get("id")), + ) + ) + return entries[:HISTORICAL_SIBLING_CAP] + + def verification_claim( candidate: Mapping[str, Any], state: Optional[Mapping[str, Any]] = None, @@ -2462,7 +2687,8 @@ def verification_claim( "summary": candidate.get("summary"), "failure_scenario": candidate.get("failure_scenario"), "reports": int(candidate.get("reports") or 1), - "siblings": sibling_claims(candidate, pool or []), + "siblings": sibling_claims(candidate, pool or []) + + historical_sibling_claims(candidate, state), } suggestion_code = str(candidate.get("suggestion_code") or "") if suggestion_code and location["existing_code"]: @@ -2999,6 +3225,8 @@ def reportable_finding( candidate["file"], start_line, end_line, state ), } + if isinstance(candidate.get("duplicate_of_posted"), dict): + finding["duplicate_of_posted"] = dict(candidate["duplicate_of_posted"]) suggestion_code = str(candidate.get("suggestion_code") or "") if ( suggestion_code @@ -3228,12 +3456,35 @@ def fold_duplicates( candidate_id = folded[candidate_id] return candidate_id + history_by_hid = { + f"H{comment.get('id')}": comment + for comment in state.get("history_comments") or [] + if isinstance(comment, dict) + } + for record in ordered: candidate_id = str(record["candidate"].get("id")) target = (record.get("verdict") or {}).get("duplicate_of") + if not target: + continue + if HISTORICAL_ID_RE.fullmatch(str(target)): + # A historical target (P3 item 5): the posted finding is not + # in kept_records, so the shown-sibling check stays but the + # survivor check does not apply. The candidate is not folded + # away -- it is marked, kept in the bundle, and the publisher + # places it as skipped (reason duplicate-of-posted). + comment = history_by_hid.get(str(target)) + if ( + target in allowed.get(candidate_id, set()) + and comment is not None + ): + record["candidate"]["duplicate_of_posted"] = { + "comment_id": int(comment["id"]), + "html_url": str(comment.get("html_url") or ""), + } + continue if ( - not target - or target == candidate_id + target == candidate_id or target not in allowed.get(candidate_id, set()) or target not in by_id or rank_index[target] > rank_index[candidate_id] @@ -3350,6 +3601,10 @@ def final_tally() -> None: entry["verdict"] = verdict["verdict"] if disposition == "duplicate": entry["duplicate_of"] = folded.get(str(candidate.get("id"))) + if isinstance(candidate.get("duplicate_of_posted"), dict): + entry["duplicate_of_posted"] = dict( + candidate["duplicate_of_posted"] + ) if candidate.get("anchors"): entry["anchors"] = list(candidate["anchors"]) return entry @@ -3363,6 +3618,8 @@ def final_tally() -> None: if str(candidate.get("id")) in folded: return "duplicate" if candidate_key(candidate) in reported_keys and record["kept"]: + if isinstance(candidate.get("duplicate_of_posted"), dict): + return "duplicate-of-posted" return "reportable" if record["kept"]: return "deferred-by-cap" @@ -3532,6 +3789,21 @@ def final_tally() -> None: "verification": {"status": verification_status}, "canonical_files": list(CANONICAL_FILES), } + incremental_state = ( + state.get("incremental") + if isinstance(state.get("incremental"), dict) + else {} + ) + if incremental_state.get("requested"): + manifest["incremental"] = { + "requested": True, + "enabled": bool(incremental_state.get("enabled")), + "from": incremental_state.get("from"), + "commits": incremental_state.get("commits"), + "history_unavailable": incremental_state.get( + "history_unavailable" + ), + } if rule_mapped: rules_state = ( state.get("rules") if isinstance(state.get("rules"), dict) else {} @@ -3680,17 +3952,6 @@ def publish_pr_command(args: argparse.Namespace) -> None: plan_rel = f"{products_rel}/pr-publish-plan.json" outcome_rel = f"{products_rel}/pr-publish-outcome.json" - def run_publisher( - arguments: List[str], - environment: Optional[Dict[str, str]] = None, - ) -> subprocess.CompletedProcess: - return subprocess.run( - [sys.executable, str(publisher), *arguments], - cwd=root(), - env=environment, - capture_output=True, - ) - def publisher_error( prefix: str, failed: subprocess.CompletedProcess ) -> WorkflowDataError: @@ -3707,32 +3968,50 @@ def publish_pr_command(args: argparse.Namespace) -> None: if key in ("PATH", "HOME", "TZ", "USER", "LOGNAME", "SHELL") or key.startswith(("LANG", "LC_", "PYTHON", "TMP", "TEMP")) } - result = run_publisher( - [ - "plan", - "--evidence-dir", evidence_rel, - "--repo", repo, - "--pr", pr_text, - "--route-severity-below", args.route_severity_below, - "--route-categories", args.route_categories, - "--run-url", one_line(args.run_url, 2000), - "--output", plan_rel, - ], - plan_environment, + plan_arguments = [ + "plan", + "--evidence-dir", evidence_rel, + "--repo", repo, + "--pr", pr_text, + "--route-severity-below", args.route_severity_below, + "--route-categories", args.route_categories, + "--run-url", one_line(args.run_url, 2000), + "--output", plan_rel, + ] + incremental_state = ( + state.get("incremental") + if isinstance(state.get("incremental"), dict) + else {} ) + if incremental_state.get("requested"): + # Prepare parsed and recorded the threshold; the node reuses that + # decision rather than re-reading the raw workflow input. + plan_arguments.extend( + [ + "--incremental", "true", + "--incremental-overlap-threshold", + str(incremental_state.get("overlap_threshold") or ""), + ] + ) + history_rel = incremental_state.get("history_rel") + if incremental_state.get("enabled") and isinstance(history_rel, str): + plan_arguments.extend(["--history", history_rel]) + result = run_publisher(publisher, plan_arguments, plan_environment) if result.returncode != 0: raise publisher_error("the publication plan failed: ", result) print(result.stdout.decode("utf-8", "replace").strip()) result = run_publisher( + publisher, [ "apply", "--plan", plan_rel, "--repo", repo, "--pr", pr_text, "--api-base", args.api_base, + "--bot-login", args.bot_login, "--outcome", outcome_rel, - ] + ], ) value = read_json(root() / outcome_rel, required=False) outcome: Dict[str, Any] = value if isinstance(value, dict) else {} @@ -3902,6 +4181,12 @@ def build_parser() -> argparse.ArgumentParser: prepare_parser.add_argument("--range", default="") prepare_parser.add_argument("--model", default="") prepare_parser.add_argument("--guidance", default="") + prepare_parser.add_argument("--incremental", default="") + prepare_parser.add_argument("--incremental-overlap-threshold", default="") + prepare_parser.add_argument("--pr-repo", default="") + prepare_parser.add_argument("--pr-number", default="") + prepare_parser.add_argument("--bot-login", default="") + prepare_parser.add_argument("--api-base", default="https://api.github.com") prepare_parser.add_argument("--review-id-stdin", action="store_true") merge_parser = subparsers.add_parser("merge") @@ -3917,6 +4202,7 @@ def build_parser() -> argparse.ArgumentParser: publish_parser.add_argument("--route-severity-below", default="") publish_parser.add_argument("--route-categories", default="") publish_parser.add_argument("--run-url", default="") + publish_parser.add_argument("--bot-login", default="") publish_parser.add_argument("--api-base", default="https://api.github.com") expectations_parser = subparsers.add_parser("verify-expectations") diff --git a/.fabro/workflows/code-review/scripts/publish_pr.py b/.fabro/workflows/code-review/scripts/publish_pr.py index 56fa54b30..94cefa0e6 100644 --- a/.fabro/workflows/code-review/scripts/publish_pr.py +++ b/.fabro/workflows/code-review/scripts/publish_pr.py @@ -12,11 +12,17 @@ Posts a completed review's findings to the reviewed GitHub PR in two steps: PR state) is confined here. The plan file is untrusted input: apply re-validates it before any write. +A third subcommand, ``history``, is the token-holding sibling of apply +for incremental re-review (P3): it snapshots the PR's prior-review state +(our earlier comments and the sticky summary's identity tags) into a +validated ``pr-history.json`` that ``plan`` consumes as a file. + The canonical ``lithoscomputer/code-review`` source repository keeps the -requirements register at ``.ai/plans/p1-pr-publisher-requirements.md`` and -the executable specification at ``tests/test_pr_publisher.py``. Packaged -workflow installs do not need to copy those development files. R-numbers -below refer to that canonical requirements register. +requirements registers at ``.ai/plans/p1-pr-publisher-requirements.md`` +(R-numbers) and ``.ai/plans/p3-incremental-re-review-requirements.md`` +(P-numbers), with the executable specifications at +``tests/test_pr_publisher.py`` and ``tests/test_incremental_rereview.py``. +Packaged workflow installs do not need to copy those development files. Python 3.9-compatible. Standard library only. """ @@ -24,6 +30,8 @@ Python 3.9-compatible. Standard library only. from __future__ import annotations import argparse +import base64 +import functools import hashlib import json import os @@ -42,12 +50,32 @@ import render_report as renderer # noqa: E402 (the bundle validators) from review_contract import FINDING_ID_RE # noqa: E402 -PLAN_VERSION = 1 +PLAN_VERSION = 2 +HISTORY_VERSION = 1 SUMMARY_MARKER = "" COMMENT_TAG_PREFIX = "fabro-code-review-comment" RUN_TAG_PREFIX = "fabro-code-review-run" COMPLETED_TAG_PREFIX = "fabro-code-review-completed" +HEAD_TAG_PREFIX = "fabro-code-review-head" +BASE_TAG_PREFIX = "fabro-code-review-base" +META_TAG_PREFIX = "fabro-code-review-meta" DEFAULT_BATCH_SIZE = 50 +# OCR parity: the incremental overlap threshold defaults to 0.6 and must be +# strictly inside (0, 1) -- the predicate is IoU > threshold and an identical +# span has IoU exactly 1.0, so a threshold of 1.0 could never match anything. +DEFAULT_OVERLAP_THRESHOLD = 0.6 +# Skipped-placement reasons (the plan's third placement kind, P3 item 3/5). +SKIP_REASONS = ("overlap", "duplicate-of-posted") +# Partial-span mapping policy (P3 item 4, decided Q6): a history comment +# whose span was partly rewritten by a later push is treated as covering the +# rewritten site's replacement lines and still suppresses -- the aggressive +# choice, recorded here as an explicit constant. +PARTIAL_MAPPED_SPANS_SUPPRESS = True +# Only canonical GitHub comment URLs are carried into plan details. +GITHUB_URL_PREFIX = "https://github.com/" +# The encoded meta-tag payload is bounded; the short summary is truncated +# deterministically until the encoding fits. +META_TAG_PAYLOAD_CAP = 1000 # GitHub caps a comment body at 65,536 characters; the summary is assembled # under this budget so a write can never fail on size (R19). SUMMARY_BUDGET = 65000 @@ -68,6 +96,15 @@ REPO_RE = re.compile( ) SHA_RE = re.compile(r"^[0-9a-f]{40}$") HUNK_HEADER_RE = re.compile(r"^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@") +HUNK_PAIR_RE = re.compile(r"^@@ -(\d+)(?:,(\d+))? \+(\d+)(?:,(\d+))? @@") +COMMENT_TAG_RE = re.compile( + r"^" +) +META_TAG_RE = re.compile( + r"" +) PLAUSIBLE_WARNING = ( "> **Needs confirmation:** The verifier could not fully confirm this " "finding from the available evidence." @@ -94,6 +131,89 @@ def completed_tag_for(completed_at: str) -> str: return f"" +def head_tag_for(head: str) -> str: + return f"" + + +def base_tag_for(base: str) -> str: + return f"" + + +def tag_value( + body: str, prefix: str, pattern: str = r"[^>]+" +) -> Optional[str]: + match = re.search( + r"", body + ) + return match.group(1).strip() if match else None + + +def tagged_sha(body: str, prefix: str) -> Optional[str]: + return tag_value(body, prefix, r"[0-9a-f]{40}") + + +def meta_tag_for(finding: Mapping[str, Any]) -> str: + """The hidden metadata tag an inline comment carries (P3 item 5). + + Base64url keeps the payload safe inside an HTML comment: its alphabet + cannot produce the ``-->`` terminator, so a short summary containing + it (or any markup) cannot break out of the tag. The short summary is + truncated deterministically until the encoding fits the cap. + """ + short_summary = str(finding["short_summary"]) + while True: + payload = json.dumps( + { + "category": finding["category"], + "severity": finding["severity"], + "short_summary": short_summary, + }, + ensure_ascii=True, + sort_keys=True, + separators=(",", ":"), + ) + encoded = base64.urlsafe_b64encode(payload.encode("utf-8")).decode( + "ascii" + ) + if len(encoded) <= META_TAG_PAYLOAD_CAP or not short_summary: + return f"" + short_summary = short_summary[:-10] + + +def parse_meta_tag(body: str) -> Optional[Dict[str, str]]: + match = META_TAG_RE.search(body) + if not match: + return None + try: + payload = json.loads( + base64.urlsafe_b64decode(match.group(1).encode("ascii")) + ) + except (ValueError, UnicodeError): + return None + if not isinstance(payload, dict): + return None + category = payload.get("category") + severity = payload.get("severity") + short_summary = payload.get("short_summary") + if ( + category not in renderer.CATEGORIES + or severity not in SEVERITY_RANK + or not isinstance(short_summary, str) + ): + return None + return { + "category": category, + "severity": severity, + "short_summary": short_summary, + } + + +def parse_comment_identity(body: str) -> Optional[Tuple[str, str]]: + """(review_id, finding_id) from a body's leading identity tag.""" + match = COMMENT_TAG_RE.match(body) + return (match.group(1), match.group(2)) if match else None + + # --- Git arithmetic (plan) --------------------------------------------------- @@ -130,6 +250,13 @@ def resolve_commit(token: str, field: str) -> str: return resolved +@functools.lru_cache(maxsize=None) +def commit_exists(sha: str) -> bool: + """True when the SHA resolves to a commit in the local repository.""" + result = run_git("rev-parse", "--verify", "--quiet", sha + "^{commit}") + return result.returncode == 0 + + def resolve_diff_base(token: str) -> str: """The diff base as a commit, or a bare tree for a root commit. @@ -213,6 +340,189 @@ def has_diff_range( ) +# --- Span mapping between commits (P3 item 4) -------------------------------- +# +# Pure git arithmetic shared by `history` (mapping an outdated comment's +# original span onto the live head) and `apply` (forward-mapping a plan's +# spans onto a head that moved after the review started). + + +@functools.lru_cache(maxsize=64) +def pair_diff_map( + old_rev: str, new_rev: str +) -> Mapping[str, Tuple[Optional[str], Tuple[Tuple[int, int, int, int], ...]]]: + """Per old path: (new path, or None when deleted; -U0 hunk quadruples). + + Hunks are (old_start, old_count, new_start, new_count). A file absent + from the mapping is unchanged between the revisions. Renames are + followed. The same preamble discipline as ``right_side_hunks`` keeps a + body line that renders as ``--- ``/``+++ `` from being read as a file + boundary. + """ + result = run_git( + "diff", "--no-color", "--no-ext-diff", "--no-textconv", + "--find-renames", "-U0", + old_rev, new_rev, + ) + if result.returncode != 0: + detail = result.stderr.decode("utf-8", "replace").strip() + fail(f"git diff for span mapping failed: {detail}") + mapping: Dict[str, Tuple[Optional[str], Tuple[Tuple[int, int, int, int], ...]]] = {} + old_path: Optional[str] = None + new_path: Optional[str] = None + hunks: List[Tuple[int, int, int, int]] = [] + in_preamble = False + + def flush() -> None: + if old_path is not None: + mapping[old_path] = (new_path, tuple(hunks)) + + for line in result.stdout.decode("utf-8", "replace").splitlines(): + if line.startswith("diff --git "): + flush() + old_path = None + new_path = None + hunks = [] + in_preamble = True + elif in_preamble and line.startswith("rename from "): + # A pure rename has no ---/+++ lines at all; the rename + # preamble is the only record of the path pair. + token = line[len("rename from "):] + old_path = None if token.startswith('"') else token + elif in_preamble and line.startswith("rename to "): + token = line[len("rename to "):] + if token.startswith('"'): + old_path = None + else: + new_path = token + elif in_preamble and line.startswith("--- "): + token = line[4:] + if token == "/dev/null" or token.startswith('"'): + old_path = None + elif token.startswith("a/"): + old_path = token[2:] + else: + old_path = token + elif in_preamble and line.startswith("+++ "): + token = line[4:] + if token == "/dev/null": + new_path = None + elif token.startswith('"'): + old_path = None + elif token.startswith("b/"): + new_path = token[2:] + else: + new_path = token + elif line.startswith("@@ "): + in_preamble = False + match = HUNK_PAIR_RE.match(line) + if not match: + continue + old_start = int(match.group(1)) + old_count = 1 if match.group(2) is None else int(match.group(2)) + new_start = int(match.group(3)) + new_count = 1 if match.group(4) is None else int(match.group(4)) + hunks.append((old_start, old_count, new_start, new_count)) + flush() + return mapping + + +def map_line( + hunks: Sequence[Tuple[int, int, int, int]], line: int +) -> Optional[int]: + """The line's image in the new revision, or None inside a removal.""" + delta = 0 + for old_start, old_count, _new_start, new_count in hunks: + # A pure insertion (-a,0) sits after old line a; line a itself is + # untouched by it. + boundary = old_start if old_count > 0 else old_start + 1 + if line < boundary: + return line + delta + if old_count > 0 and line <= old_start + old_count - 1: + return None + delta += new_count - old_count + return line + delta + + +def map_span( + old_rev: str, new_rev: str, path: str, start_line: int, end_line: int +) -> Optional[Dict[str, Any]]: + """The span's image at new_rev: path, lines, and a ``partial`` flag. + + Both endpoints mapping cleanly is a full mapping. A span partly inside + a rewritten region maps onto the rewrite's replacement lines and is + flagged ``partial`` (policy: PARTIAL_MAPPED_SPANS_SUPPRESS). A deleted + file or a fully deleted span has no image and returns None. + """ + entry = pair_diff_map(old_rev, new_rev).get(path) + if entry is None: + return { + "path": path, + "start_line": start_line, + "end_line": end_line, + "partial": False, + } + new_path, hunks = entry + if new_path is None: + return None + points: List[int] = [] + partial = False + for line in (start_line, end_line): + image = map_line(hunks, line) + if image is None: + partial = True + else: + points.append(image) + # Any removal intersecting the span means span lines changed; the + # replacement region is part of the image (the same site, edited). + for old_start, old_count, new_start, new_count in hunks: + if old_count < 1: + continue + if old_start > end_line or old_start + old_count - 1 < start_line: + continue + partial = True + if new_count > 0: + points.extend((new_start, new_start + new_count - 1)) + if not points: + return None + return { + "path": new_path, + "start_line": min(points), + "end_line": max(points), + "partial": partial, + } + + +# --- Overlap predicate (P3 item 3, OCR parity) -------------------------------- + + +def span_overlap_iou( + new_start: int, + new_end: int, + old_start: int, + old_end: int, + threshold: float, +) -> Optional[float]: + """The IoU when the spans match under OCR's predicate, else None. + + Single-line vs single-line matches on the same line (IoU 1.0); + multi-line vs multi-line matches when overlap/union is strictly above + the threshold; single vs multi never matches, in either direction. + """ + new_single = new_start == new_end + old_single = old_start == old_end + if new_single != old_single: + return None + if new_single: + return 1.0 if new_start == old_start else None + overlap = min(new_end, old_end) - max(new_start, old_start) + 1 + if overlap <= 0: + return None + union = max(new_end, old_end) - min(new_start, old_start) + 1 + iou = overlap / union + return iou if iou > threshold else None + + # --- Routing configuration (R3-R5, fail-closed) ------------------------------ @@ -262,6 +572,42 @@ def parse_pr_number(raw: str) -> int: return int(text) +def parse_incremental_flag(raw: str) -> bool: + """Fail-closed boolean for the incremental input.""" + text = str(raw or "").strip().lower() + if text in ("", "false", "0", "no", "off"): + return False + if text in ("true", "1", "yes", "on"): + return True + fail(f"incremental must be true or false (or empty), got {raw!r}") + + +def parse_overlap_threshold(raw: str) -> float: + """The overlap threshold, strictly inside (0, 1) (fail-closed). + + The predicate is strict ``IoU > threshold`` and an identical span has + IoU exactly 1.0, so a threshold of 1.0 could never match anything; + dead configuration is refused rather than accepted (unlike OCR, which + documents (0, 1] and silently inherits the quirk). + """ + text = str(raw or "").strip() + if not text: + return DEFAULT_OVERLAP_THRESHOLD + try: + value = float(text) + except ValueError: + fail( + "incremental-overlap-threshold must be a number strictly " + f"between 0 and 1, got {raw!r}" + ) + if not (0.0 < value < 1.0): + fail( + "incremental-overlap-threshold must be strictly between 0 and " + f"1 (exclusive: 1.0 could never match anything), got {raw!r}" + ) + return value + + def routing_detail( finding: Mapping[str, Any], threshold: Optional[str], @@ -365,6 +711,7 @@ def inline_comment_body(finding: Mapping[str, Any], review_id: str) -> str: issue_type = str(finding["issue_type"]).lower().capitalize() lines = [ f"", + meta_tag_for(finding), "", f"**{SEVERITY_EMOJI[finding['severity']]} {issue_type}** — " + renderer.escape_markdown(finding["short_summary"]), @@ -443,18 +790,28 @@ def summary_section( def counts_line( - total: int, inline: int, no_position: int, routed: int, failed: int + total: int, + inline: int, + no_position: int, + routed: int, + failed: int, + skipped: int, ) -> str: if total == 0: return ( "**No findings.** The review completed with nothing to report; " "this summary supersedes any earlier run." ) - return ( + text = ( f"**{total} finding(s)** — posted inline: {inline} · " f"no diff position: {no_position} · routed by policy: {routed} · " f"could not be posted: {failed}" ) + # Skipped findings render as the count alone (P3, decided Q4); the + # plan's skipped placements are the audit trail. + if skipped: + text += f" · already reported: {skipped}" + return text def rules_coverage_line( @@ -482,6 +839,35 @@ def rules_coverage_line( ) +def incremental_context_lines(manifest: Mapping[str, Any]) -> List[str]: + """The sticky summary's incremental identity and degrade notes (P3).""" + record = manifest.get("incremental") + if not isinstance(record, dict) or not record.get("requested"): + return [] + lines: List[str] = [] + delta_from = record.get("from") + if isinstance(delta_from, str) and SHA_RE.fullmatch(delta_from): + commits = record.get("commits") + counted = ( + f"{commits} commit(s)" + if isinstance(commits, int) and not isinstance(commits, bool) + else "the commits" + ) + lines.append( + f"Incremental review: reviewed {counted} since " + + renderer.code_span(delta_from[:12]) + + "; findings outside this delta were not re-reviewed." + ) + reason = record.get("history_unavailable") + if isinstance(reason, str) and reason.strip(): + lines.append( + "History unavailable (" + + renderer.escape_markdown(reason.strip()) + + "); earlier comments may be repeated." + ) + return lines + + def summary_context_lines( manifest: Mapping[str, Any], coverage: Mapping[str, Any], @@ -495,6 +881,7 @@ def summary_context_lines( lines.append(renderer.partial_review_warning(reasons)) if manifest["effort"] == "low": lines.append(renderer.LOW_EFFORT_REVIEW_NOTE) + lines.extend(incremental_context_lines(manifest)) rules_text = rules_coverage_line(coverage, findings) if rules_text: lines.append(rules_text) @@ -525,15 +912,20 @@ def assemble_summary_body( sections: Sequence[str], review_id: str, run_url: str, + identity_tags: Sequence[str] = (), ) -> str: """Assemble the sticky summary under the size budget (R9, R19). Sections render in full in ranking order; when the next section would overflow the budget, it and every later section are replaced by one elision line. Elision affects rendering only, never counts. + ``identity_tags`` carries the reviewed-head (and base) tags, written + only into the final summary body -- never the anchor -- so a crash + after the anchor write cannot leave a tag claiming this head was + fully reviewed (P3 item 1). """ - head_parts = [marker, run_tag, completed_tag, "", "## Code review", "", - counts_text] + head_parts = [marker, run_tag, completed_tag, *identity_tags, "", + "## Code review", "", counts_text] for line in context_lines: head_parts.extend(["", line]) if sections: @@ -554,6 +946,159 @@ def assemble_summary_body( return body +# --- PR history (P3 item 1) -------------------------------------------------- + + +def optional_positive_line(value: Any, field: str) -> Optional[int]: + if value is None: + return None + if isinstance(value, bool) or not isinstance(value, int) or value < 1: + fail(f"history {field} must be a positive integer or null") + return value + + +def validate_history_document(value: Any) -> Dict[str, Any]: + """Validate a pr-history.json document (fail-closed, R5).""" + if not isinstance(value, dict): + fail("the history file is not a JSON object") + if value.get("version") != HISTORY_VERSION: + fail( + "the history file has an unsupported version " + f"(expected {HISTORY_VERSION}, got {value.get('version')!r})" + ) + target = value.get("target") + if ( + not isinstance(target, dict) + or not isinstance(target.get("repo"), str) + or not REPO_RE.fullmatch(target["repo"]) + or isinstance(target.get("pr"), bool) + or not isinstance(target.get("pr"), int) + or target["pr"] < 1 + ): + fail("the history target is invalid") + live_head = value.get("live_head") + if not isinstance(live_head, str) or not SHA_RE.fullmatch(live_head): + fail("the history live_head is not a commit SHA") + summary = value.get("summary") + if summary is not None: + if not isinstance(summary, dict): + fail("the history summary must be an object or null") + if isinstance(summary.get("comment_id"), bool) or not isinstance( + summary.get("comment_id"), int + ): + fail("the history summary.comment_id must be an integer") + for field in ("head", "base"): + sha = summary.get(field) + if sha is not None and ( + not isinstance(sha, str) or not SHA_RE.fullmatch(sha) + ): + fail(f"the history summary.{field} must be a SHA or null") + comments = value.get("comments") + if not isinstance(comments, list): + fail("the history comments must be an array") + seen_ids: Set[int] = set() + for index, comment in enumerate(comments): + field = f"comments[{index}]" + if not isinstance(comment, dict): + fail(f"history {field} must be an object") + comment_id = comment.get("id") + if ( + isinstance(comment_id, bool) + or not isinstance(comment_id, int) + or comment_id < 1 + or comment_id in seen_ids + ): + fail(f"history {field}.id must be a unique positive integer") + seen_ids.add(comment_id) + try: + renderer.safe_repo_path(comment.get("path"), f"history {field}.path") + except renderer.RenderError as error: + fail(str(error)) + if comment.get("side") not in ("LEFT", "RIGHT"): + fail(f"history {field}.side is invalid") + for line_field in ( + "line", + "start_line", + "original_line", + "original_start_line", + ): + optional_positive_line( + comment.get(line_field), f"{field}.{line_field}" + ) + original_commit = comment.get("original_commit_id") + if original_commit is not None and ( + not isinstance(original_commit, str) + or not SHA_RE.fullmatch(original_commit) + ): + fail(f"history {field}.original_commit_id must be a SHA or null") + span = comment.get("mapped_span") + if span is not None: + if not isinstance(span, dict): + fail(f"history {field}.mapped_span must be an object or null") + try: + renderer.safe_repo_path( + span.get("path"), f"history {field}.mapped_span.path" + ) + except renderer.RenderError as error: + fail(str(error)) + start = optional_positive_line( + span.get("start_line"), f"{field}.mapped_span.start_line" + ) + end = optional_positive_line( + span.get("end_line"), f"{field}.mapped_span.end_line" + ) + if start is None or end is None or start > end: + fail(f"history {field}.mapped_span lines are invalid") + if not isinstance(span.get("partial"), bool): + fail(f"history {field}.mapped_span.partial must be a boolean") + meta = comment.get("meta") + if meta is not None: + if ( + not isinstance(meta, dict) + or meta.get("category") not in renderer.CATEGORIES + or meta.get("severity") not in SEVERITY_RANK + or not isinstance(meta.get("short_summary"), str) + or len(meta["short_summary"]) > 8000 + ): + fail(f"history {field}.meta is invalid") + html_url = comment.get("html_url") + if html_url is not None and ( + not isinstance(html_url, str) or len(html_url) > 2048 + ): + fail(f"history {field}.html_url is invalid") + return value + + +def load_history_file( + path: str, repo: str, pr: int, reviewed_head: str +) -> Tuple[Dict[str, Any], str]: + """Read, validate, and digest a history file for this plan's target. + + The snapshot must describe exactly the reviewed head: a push landing + between the checkout and the history fetch would make every decision + built on it wrong from the outset, so a mismatch fails the plan. + """ + try: + raw = Path(path).read_bytes() + except OSError as error: + fail(f"could not read the history file: {error}") + try: + value = json.loads(raw.decode("utf-8")) + except (UnicodeError, json.JSONDecodeError) as error: + fail(f"the history file is not valid JSON: {error}") + history = validate_history_document(value) + if history["target"] != {"repo": repo, "pr": pr}: + fail("the history file's target does not match --repo/--pr") + if history["live_head"] != reviewed_head: + fail( + f"the history snapshot describes head " + f"{history['live_head'][:12]}, not the reviewed head " + f"{reviewed_head[:12]}; the PR moved between the checkout and " + "the history fetch -- re-run the review" + ) + return history, hashlib.sha256(raw).hexdigest() + + # --- plan -------------------------------------------------------------------- @@ -637,6 +1182,114 @@ def resolve_reviewed_range(manifest: Mapping[str, Any]) -> Tuple[str, str, str]: return base, head, range_text +def duplicate_of_posted_detail( + finding: Mapping[str, Any], +) -> Optional[Dict[str, Any]]: + """The skipped-placement detail for a verifier-marked duplicate (item 5).""" + record = finding.get("duplicate_of_posted") + if not isinstance(record, dict): + return None + comment_id = record.get("comment_id") + if isinstance(comment_id, bool) or not isinstance(comment_id, int): + fail( + f"finding {finding.get('id')} carries an invalid " + "duplicate_of_posted record" + ) + detail: Dict[str, Any] = {"comment_id": comment_id} + html_url = record.get("html_url") + if isinstance(html_url, str) and html_url.startswith(GITHUB_URL_PREFIX): + detail["html_url"] = html_url + return detail + + +def suppression_spans( + history: Optional[Mapping[str, Any]], +) -> List[Dict[str, Any]]: + """Our history comments' spans at the live head, for overlap tests. + + A comment suppresses only through a RIGHT-side mapped span (item 4 + supplies it: live API fields while the comment is live, git-mapped + original fields once GitHub marks it outdated). A comment with no + usable span never suppresses. + """ + if history is None: + return [] + spans: List[Dict[str, Any]] = [] + for comment in history["comments"]: + if comment.get("side") != "RIGHT": + continue + span = comment.get("mapped_span") + if not isinstance(span, dict): + continue + if span.get("partial") and not PARTIAL_MAPPED_SPANS_SUPPRESS: + continue + spans.append( + { + "comment_id": comment["id"], + "review_id": comment.get("review_id"), + "finding_id": comment.get("finding_id"), + "html_url": comment.get("html_url"), + "path": span["path"], + "start_line": span["start_line"], + "end_line": span["end_line"], + "partial": bool(span.get("partial")), + } + ) + return spans + + +def overlap_match( + finding: Mapping[str, Any], + spans: Sequence[Mapping[str, Any]], + threshold: float, +) -> Optional[Dict[str, Any]]: + """The best history comment covering this finding's span, or None. + + Deterministic: the highest IoU wins; ties resolve to the lowest + comment ID. + """ + location = finding["location"] + best: Optional[Dict[str, Any]] = None + for span in spans: + if span["path"] != finding["file"]: + continue + iou = span_overlap_iou( + location["start_line"], + location["end_line"], + span["start_line"], + span["end_line"], + threshold, + ) + if iou is None: + continue + if ( + best is None + or iou > best["iou"] + or (iou == best["iou"] and span["comment_id"] < best["comment_id"]) + ): + best = {**span, "iou": iou} + if best is None: + return None + detail: Dict[str, Any] = { + "comment_id": best["comment_id"], + "iou": round(best["iou"], 4), + "mapped_span": { + "path": best["path"], + "start_line": best["start_line"], + "end_line": best["end_line"], + "partial": best["partial"], + }, + } + if isinstance(best.get("review_id"), str): + detail["review_id"] = best["review_id"] + if isinstance(best.get("finding_id"), str): + detail["finding_id"] = best["finding_id"] + html_url = best.get("html_url") + if isinstance(html_url, str) and html_url.startswith(GITHUB_URL_PREFIX): + detail["html_url"] = html_url + return detail + + def command_plan(args: argparse.Namespace) -> int: repo = args.repo.strip() if not REPO_RE.fullmatch(repo) or ".." in repo: @@ -647,6 +1300,12 @@ def command_plan(args: argparse.Namespace) -> int: threshold = parse_severity_threshold(args.route_severity_below) categories = parse_route_categories(args.route_categories) batch_size = parse_batch_size(args.batch_size) + incremental = parse_incremental_flag(args.incremental) + overlap_threshold = parse_overlap_threshold( + args.incremental_overlap_threshold + ) + if args.history and not incremental: + fail("--history requires --incremental true") run_url = (args.run_url or "").strip() if len(run_url) > 2048: fail("run-url exceeds 2048 characters") @@ -658,12 +1317,24 @@ def command_plan(args: argparse.Namespace) -> int: if not isinstance(completed_at, str) or not completed_at.strip(): fail("the manifest has no completion timestamp") + history: Optional[Dict[str, Any]] = None + history_digest: Optional[str] = None + if args.history: + history, history_digest = load_history_file( + args.history, repo, pr, head + ) + spans = suppression_spans(history) + hunks = right_side_hunks(base, head) # Exhaustive partition (R1): every finding gets exactly one placement. - # Diff position decides first (R2); routing applies only to otherwise - # inline-eligible findings, so a finding matching both carries the - # no-position reason and routing can never hide a placement. + # A verifier-marked duplicate of a posted comment skips first (the + # defect already has a visible comment, wherever it sits); then diff + # position decides (R2); then overlap suppression (a finding already + # posted must not be routed to the summary as new); routing applies + # last, only to otherwise inline-eligible findings, so a finding + # matching several policies carries the earliest reason and nothing + # can hide a placement. placements: List[Dict[str, Any]] = [] for finding in findings: location = finding["location"] @@ -676,6 +1347,17 @@ def command_plan(args: argparse.Namespace) -> int: "start_line": start_line, "end_line": end_line, } + posted_duplicate = duplicate_of_posted_detail(finding) + if posted_duplicate is not None: + placements.append( + { + **base_entry, + "placement": "skipped", + "reason": "duplicate-of-posted", + "detail": posted_duplicate, + } + ) + continue if not has_diff_range( hunks, finding["file"], start_line, end_line ): @@ -690,6 +1372,17 @@ def command_plan(args: argparse.Namespace) -> int: } ) continue + overlap = overlap_match(finding, spans, overlap_threshold) + if overlap is not None: + placements.append( + { + **base_entry, + "placement": "skipped", + "reason": "overlap", + "detail": overlap, + } + ) + continue detail = routing_detail(finding, threshold, categories) if detail is not None: placements.append( @@ -727,6 +1420,9 @@ def command_plan(args: argparse.Namespace) -> int: for entry in placements if entry["placement"] == "summary" and entry["reason"] == "routed" ) + skipped = sum( + 1 for entry in placements if entry["placement"] == "skipped" + ) # Deterministic batching (R13): sorted (path, line, finding ID), then # contiguous chunks of at most batch_size. Routed findings never enter @@ -746,6 +1442,8 @@ def command_plan(args: argparse.Namespace) -> int: run_tag = run_tag_for(review_id) completed_tag = completed_tag_for(completed_at) + head_tag = head_tag_for(head) + base_tag = base_tag_for(base) context = summary_context_lines( manifest, coverage, findings, reasons, head, run_url ) @@ -756,12 +1454,22 @@ def command_plan(args: argparse.Namespace) -> int: SUMMARY_MARKER, run_tag, completed_tag, - counts_line(len(findings), len(inline_entries), no_position, routed, 0), + counts_line( + len(findings), + len(inline_entries), + no_position, + routed, + 0, + skipped, + ), context, sections, review_id, run_url, + identity_tags=(head_tag, base_tag), ) + # The anchor never carries the head tag: a crash after the anchor + # write must not leave a tag claiming this head was fully reviewed. anchor_body = "\n".join( [ SUMMARY_MARKER, @@ -774,6 +1482,13 @@ def command_plan(args: argparse.Namespace) -> int: ] ) + incremental_record = manifest.get("incremental") + history_unavailable = "" + if isinstance(incremental_record, dict): + reason = incremental_record.get("history_unavailable") + if isinstance(reason, str): + history_unavailable = reason.strip() + plan = { "version": PLAN_VERSION, "review_id": review_id, @@ -784,11 +1499,15 @@ def command_plan(args: argparse.Namespace) -> int: "head": head, "range": range_text, "bundle_digest": bundle_digest(args.evidence_dir), + "history_digest": history_digest, "config": { "batch_size": batch_size, "route_severity_below": threshold.lower() if threshold else "", "route_categories": categories, "run_url": run_url, + "incremental": incremental, + "incremental_overlap_threshold": overlap_threshold, + "history_unavailable": history_unavailable, }, "placements": placements, "batches": batches, @@ -797,6 +1516,8 @@ def command_plan(args: argparse.Namespace) -> int: "run_tag": run_tag, "completed_tag": completed_tag, "completed_at": completed_at, + "head_tag": head_tag, + "base_tag": base_tag, "anchor_body": anchor_body, "body": summary_body, "context_lines": context, @@ -806,7 +1527,7 @@ def command_plan(args: argparse.Namespace) -> int: "planned_inline": len(inline_entries), "no_position": no_position, "routed": routed, - "skipped": 0, + "skipped": skipped, }, } output = Path(args.output) @@ -819,7 +1540,7 @@ def command_plan(args: argparse.Namespace) -> int: print( f"Planned {len(findings)} placement(s): {len(inline_entries)} inline " f"in {len(batches)} batch(es), {no_position} without a diff " - f"position, {routed} routed" + f"position, {routed} routed, {skipped} skipped (already reported)" ) return 0 @@ -840,7 +1561,11 @@ def validate_plan_document( if not isinstance(value, dict): fail("the plan is not a JSON object") if value.get("version") != PLAN_VERSION: - fail("the plan has an unsupported version") + fail( + "the plan has an unsupported version: this apply understands " + f"version {PLAN_VERSION}, the plan carries " + f"{value.get('version')!r}" + ) review_id = value.get("review_id") if not isinstance(review_id, str) or not REVIEW_ID_RE.fullmatch(review_id): fail("the plan review_id is invalid") @@ -855,12 +1580,32 @@ def validate_plan_document( sha = value.get(field) if not isinstance(sha, str) or not SHA_RE.fullmatch(sha): fail(f"the plan {field} is not a commit SHA") + history_digest = value.get("history_digest") + if history_digest is not None and ( + not isinstance(history_digest, str) + or not re.fullmatch(r"[0-9a-f]{64}", history_digest) + ): + fail("the plan history_digest is invalid") config = value.get("config") if not isinstance(config, dict): fail("the plan config is missing") run_url = config.get("run_url", "") if not isinstance(run_url, str) or len(run_url) > 2048: fail("the plan run_url is invalid") + if not isinstance(config.get("incremental"), bool): + fail("the plan config.incremental must be a boolean") + overlap_threshold = config.get("incremental_overlap_threshold") + if ( + isinstance(overlap_threshold, bool) + or not isinstance(overlap_threshold, (int, float)) + or not (0.0 < float(overlap_threshold) < 1.0) + ): + fail( + "the plan config.incremental_overlap_threshold must be a " + "number strictly between 0 and 1" + ) + if not isinstance(config.get("history_unavailable", ""), str): + fail("the plan config.history_unavailable must be a string") placements = value.get("placements") if not isinstance(placements, list): @@ -868,6 +1613,7 @@ def validate_plan_document( seen_ids: Set[str] = set() inline_ids: List[str] = [] reason_counts = {"no-position": 0, "routed": 0} + skipped_count = 0 for index, entry in enumerate(placements): field = f"placements[{index}]" if not isinstance(entry, dict): @@ -905,8 +1651,43 @@ def validate_plan_document( 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 == "skipped": + # The P3 placement kind: a finding already covered by one of + # our earlier comments. It carries no body and never posts; + # its detail names the covering comment (the audit trail). + if entry.get("reason") not in SKIP_REASONS: + fail(f"plan {field}.reason is invalid") + skipped_count += 1 + detail = entry.get("detail") + if not isinstance(detail, dict): + fail(f"plan {field}.detail must name the covering comment") + comment_id = detail.get("comment_id") + if isinstance(comment_id, bool) or not isinstance( + comment_id, int + ): + fail(f"plan {field}.detail.comment_id must be an integer") + html_url = detail.get("html_url") + if html_url is not None and ( + not isinstance(html_url, str) + or len(html_url) > 2048 + or not html_url.startswith(GITHUB_URL_PREFIX) + ): + fail( + f"plan {field}.detail.html_url must be an " + "https://github.com/ URL or absent" + ) + iou = detail.get("iou") + if iou is not None and ( + isinstance(iou, bool) + or not isinstance(iou, (int, float)) + or not (0.0 <= float(iou) <= 1.0) + ): + fail(f"plan {field}.detail.iou is invalid") + if "body" in entry: + fail(f"plan {field} is skipped and must carry no body") + continue + body = require_text(entry.get("body"), f"{field}.body", GITHUB_BODY_CAP) if placement == "inline": expected_tag = comment_tag(review_id, finding_id) if entry.get("comment_id") != expected_tag: @@ -952,12 +1733,22 @@ def validate_plan_document( ) if summary.get("completed_tag") != completed_tag_for(completed_at): fail("the plan summary completed_tag is inconsistent") + if summary.get("head_tag") != head_tag_for(value["head"]): + fail("the plan summary head_tag is not the reviewed head's tag") + if summary.get("base_tag") != base_tag_for(value["base"]): + fail("the plan summary base_tag is not the reviewed base's tag") for field in ("anchor_body", "body"): body = require_text( summary.get(field), f"summary.{field}", GITHUB_BODY_CAP ) if SUMMARY_MARKER not in body: fail(f"the plan summary.{field} does not embed the marker") + if summary["head_tag"] not in summary["body"]: + fail("the plan summary.body does not embed the head tag") + # The head tag claims this head was fully reviewed; only the final + # summary body may carry it, never the anchor (P3 item 1). + if HEAD_TAG_PREFIX in summary["anchor_body"]: + fail("the plan summary.anchor_body must not carry a head tag") context = summary.get("context_lines") if not isinstance(context, list) or len(context) > 100 or not all( isinstance(line, str) and len(line) <= GITHUB_BODY_CAP @@ -977,7 +1768,7 @@ def validate_plan_document( or counts["planned_inline"] != len(inline_ids) or counts["no_position"] != reason_counts["no-position"] or counts["routed"] != reason_counts["routed"] - or counts["skipped"] != 0 + or counts["skipped"] != skipped_count ): fail("the plan counts do not reconcile with its placements (R1)") return value @@ -1023,11 +1814,12 @@ class GitHubClient: except json.JSONDecodeError: return status, None - def list_all(self, path: str) -> List[Dict[str, Any]]: + def list_all(self, path: str, params: str = "") -> List[Dict[str, Any]]: results: List[Dict[str, Any]] = [] + suffix = f"&{params}" if params else "" for page in range(1, 51): status, value = self.request( - "GET", f"{path}?per_page=100&page={page}" + "GET", f"{path}?per_page=100&page={page}{suffix}" ) if status != 200 or not isinstance(value, list): fail(f"could not list {path} (HTTP {status})") @@ -1054,11 +1846,202 @@ def extract_posted_ids( return posted -def completed_stamp(body: str) -> Optional[str]: - match = re.search( - r"", body +# --- history (P3 item 1: the token-holding sibling of apply) ----------------- + + +def resolve_token_login( + client: GitHubClient, bot_login: str +) -> Optional[str]: + """The login whose comments count as ours. + + The bot_login input wins when set; otherwise GET /user works on + PAT-mode servers. On App-mode servers (where /user returns 403) this + returns None: history then degrades to no history -- its write-probe + cannot be reused because the probe is a write -- while apply learns + the login from its own first write (the anchor response). + """ + login = (bot_login or "").strip() + if login: + return login + status, user = client.request("GET", "/user") + if status == 401: + fail("GitHub rejected the token (HTTP 401)") + if status == 200 and isinstance(user, dict) and user.get("login"): + return str(user["login"]) + return None + + +def comment_line_field(comment: Mapping[str, Any], field: str) -> Optional[int]: + value = comment.get(field) + if isinstance(value, bool) or not isinstance(value, int) or value < 1: + return None + return value + + +def history_mapped_span( + comment: Mapping[str, Any], live_head: str +) -> Optional[Dict[str, Any]]: + """The comment's span at the live head (P3 item 4). + + While the comment is live its ``line``/``start_line`` already track + the current diff -- the live head. Once a push outdates it those go + null and the creation-time ``original_*`` fields are mapped forward + from ``original_commit_id`` with pure git arithmetic. An original + commit missing from local history (a force-push) cannot be mapped. + """ + path = comment.get("path") + if not isinstance(path, str) or not path: + return None + line = comment_line_field(comment, "line") + if line is not None: + start = comment_line_field(comment, "start_line") or line + return { + "path": path, + "start_line": min(start, line), + "end_line": line, + "partial": False, + } + original_line = comment_line_field(comment, "original_line") + original_commit = comment.get("original_commit_id") + if original_line is None or not isinstance(original_commit, str) or not ( + SHA_RE.fullmatch(original_commit) + ): + return None + if not commit_exists(original_commit): + return None + original_start = ( + comment_line_field(comment, "original_start_line") or original_line ) - return match.group(1).strip() if match else None + try: + return map_span( + original_commit, + live_head, + path, + min(original_start, original_line), + original_line, + ) + except PublishError: + return None + + +def command_history(args: argparse.Namespace) -> int: + token = os.environ.get("GITHUB_TOKEN", "") + if not token: + fail("history requires GITHUB_TOKEN in the environment, never argv") + repo = args.repo.strip() + if not REPO_RE.fullmatch(repo) or ".." in repo: + fail(f"repo must look like owner/name, got {args.repo!r}") + pr = parse_pr_number(args.pr) + client = GitHubClient(args.api_base, token) + + status, pull = client.request("GET", f"/repos/{repo}/pulls/{pr}") + if status != 200 or not isinstance(pull, dict): + fail(f"could not read the PR (HTTP {status})") + live_head = (pull.get("head") or {}).get("sha") + if not isinstance(live_head, str) or not SHA_RE.fullmatch(live_head): + fail("the PR head is not a commit SHA") + + login = resolve_token_login(client, args.bot_login) + if login is None: + # History drives range selection and suppression, so ownership + # must be attributable; without an identity the run degrades to + # no history (decided, Q2). + fail( + "no identity for the history fetch: GET /user is unavailable " + "(a GitHub App installation token?) and the bot_login input " + "is not set; degrading to no history" + ) + + review_comments = client.list_all( + f"/repos/{repo}/pulls/{pr}/comments", + params="sort=created&direction=desc", + ) + issue_comments = client.list_all(f"/repos/{repo}/issues/{pr}/comments") + + # Only comments that carry our identity tag AND were authored by our + # resolved login count: the tag alone is never sufficient -- a forged + # tag from another author could shrink the delta or hide a finding. + comments: List[Dict[str, Any]] = [] + for comment in review_comments: + body = str(comment.get("body") or "") + identity = parse_comment_identity(body) + if identity is None: + continue + if (comment.get("user") or {}).get("login") != login: + continue + comment_id = comment.get("id") + if isinstance(comment_id, bool) or not isinstance(comment_id, int): + continue + record: Dict[str, Any] = { + "id": comment_id, + "review_id": identity[0], + "finding_id": identity[1], + "path": comment.get("path"), + "side": comment.get("side") or "RIGHT", + "line": comment_line_field(comment, "line"), + "start_line": comment_line_field(comment, "start_line"), + "original_line": comment_line_field(comment, "original_line"), + "original_start_line": comment_line_field( + comment, "original_start_line" + ), + "original_commit_id": comment.get("original_commit_id"), + "html_url": str(comment.get("html_url") or "")[:2048], + "meta": parse_meta_tag(body), + "mapped_span": history_mapped_span(comment, live_head), + } + comments.append(record) + comments.sort(key=lambda record: record["id"]) + + summary_record: Optional[Dict[str, Any]] = None + owned_summaries = [ + comment + for comment in issue_comments + if SUMMARY_MARKER in str(comment.get("body") or "") + and (comment.get("user") or {}).get("login") == login + and isinstance(comment.get("id"), int) + ] + if owned_summaries: + newest = max(owned_summaries, key=lambda comment: comment["id"]) + body = str(newest.get("body") or "") + summary_record = { + "comment_id": newest["id"], + "review_id": tag_value(body, RUN_TAG_PREFIX), + "completed_at": completed_stamp(body), + "head": tagged_sha(body, HEAD_TAG_PREFIX), + "base": tagged_sha(body, BASE_TAG_PREFIX), + } + + document = { + "version": HISTORY_VERSION, + "target": {"repo": repo, "pr": pr}, + "live_head": live_head, + "bot_login": login, + "summary": summary_record, + "comments": comments, + } + validate_history_document(document) + output = Path(args.output) + temporary = output.with_name(output.name + ".tmp") + temporary.write_text( + json.dumps(document, ensure_ascii=True, indent=2, sort_keys=True) + + "\n", + encoding="utf-8", + ) + os.replace(temporary, output) + print( + f"Fetched PR history: {len(comments)} owned inline comment(s), " + + ( + "summary head " + + (summary_record.get("head") or "untagged")[:12] + if summary_record + else "no owned summary" + ) + ) + return 0 + + +def completed_stamp(body: str) -> Optional[str]: + return tag_value(body, COMPLETED_TAG_PREFIX) def is_newer_stamp(existing: Optional[str], ours: str) -> bool: @@ -1072,7 +2055,11 @@ def is_newer_stamp(existing: Optional[str], ours: str) -> bool: def strip_identity_tag(body: str) -> str: return re.sub( - r"^\n*", + r"^(?:\n*)+", "", body, ) @@ -1091,11 +2078,12 @@ class BatchPoster: pr: int, plan: Mapping[str, Any], already_posted: Set[str], + commit_id: str, ) -> None: self.client = client self.repo = repo self.pr = pr - self.head = plan["head"] + self.head = commit_id self.by_id = { entry["finding_id"]: entry for entry in plan["placements"] @@ -1160,8 +2148,11 @@ class BatchPoster: ) def mark_failed(self, finding_ids: Sequence[str], detail: str) -> None: + # A finding that already landed is never also failed: posted and + # failed staying disjoint is what keeps the outcome counts honest. for finding_id in finding_ids: - self.failed[finding_id] = detail + if finding_id not in self.posted: + self.failed[finding_id] = detail def reconcile(self, finding_ids: Sequence[str]) -> List[str]: """After a possibly-landed failure: absorb what landed, return what @@ -1212,7 +2203,13 @@ class BatchPoster: self.mark_failed(still_missing, self.DROPPED_DETAIL) def post_batch(self, batch_ids: Sequence[str]) -> None: - to_send = [fid for fid in batch_ids if fid not in self.posted] + # A finding pre-marked failed (a span that did not survive drift + # mapping) is never attempted; it routes to the summary (R15). + to_send = [ + fid + for fid in batch_ids + if fid not in self.posted and fid not in self.failed + ] if to_send: self.batches_attempted += 1 status = self.post_review(to_send) @@ -1243,6 +2240,67 @@ class BatchPoster: self.batches_succeeded += 1 +def ensure_commit_local(sha: str) -> bool: + """True when the commit is available locally, fetching if needed.""" + if commit_exists(sha): + return True + # The sandbox clone's HTTPS credentials are ambient; a fetch failure + # falls back to the R20 refusal. + fetched = run_git("fetch", "origin", sha) + if fetched.returncode != 0: + return False + commit_exists.cache_clear() + return commit_exists(sha) + + +def forward_map_placements( + plan: Mapping[str, Any], live_head: str +) -> Dict[str, str]: + """Map every planned inline span from plan.head onto the live head. + + Mutates the (validated) placement entries in place and returns the + finding IDs whose span did not survive the mapping, with the reason; + those route to the summary through the failed-comment path (R15). + Raises PublishError when the drift cannot be trusted at all, and the + caller falls back to the R20 refusal. + """ + if not ensure_commit_local(live_head): + fail( + f"the live head {live_head[:12]} could not be fetched for " + "drift mapping" + ) + ancestry = run_git( + "merge-base", "--is-ancestor", plan["head"], live_head + ) + if ancestry.returncode != 0: + fail( + f"the plan head {plan['head'][:12]} is not an ancestor of the " + f"live head {live_head[:12]} (a force-push?)" + ) + unmapped: Dict[str, str] = {} + for entry in plan["placements"]: + if entry["placement"] != "inline": + continue + mapped = map_span( + plan["head"], + live_head, + entry["path"], + entry["start_line"], + entry["end_line"], + ) + if mapped is None or mapped["partial"]: + unmapped[entry["finding_id"]] = ( + "the PR head moved past the reviewed head and this " + "comment's lines did not survive the move" + ) + continue + entry["path"] = mapped["path"] + entry["start_line"] = mapped["start_line"] + entry["end_line"] = mapped["end_line"] + entry["line"] = mapped["end_line"] + return unmapped + + def command_apply(args: argparse.Namespace) -> int: token = os.environ.get("GITHUB_TOKEN", "") if not token: @@ -1260,27 +2318,36 @@ def command_apply(args: argparse.Namespace) -> int: summary = plan["summary"] client = GitHubClient(args.api_base, token) - # Head drift check before the first write (R20). + # Head drift check before the first write (R20, revised by P3 item 4): + # when the live head has moved past the plan's head, apply forward-maps + # every planned span onto the live head instead of refusing; spans that + # do not survive the mapping route to the summary (R15). When the + # drift cannot be trusted (fetch failure, force-push, mapping error), + # the original refusal stands: exit nonzero before the first write. status, pull = client.request("GET", f"/repos/{repo}/pulls/{pr}") if status != 200 or not isinstance(pull, dict): fail(f"could not read the PR (HTTP {status})") live_head = (pull.get("head") or {}).get("sha") + drift_failures: Dict[str, str] = {} + posting_head = plan["head"] if live_head != plan["head"]: - fail( + refusal = ( f"the live PR head {str(live_head)[:12]} does not match the " f"plan's reviewed head {plan['head'][:12]}; refusing to post " "against a drifted head" ) + if not isinstance(live_head, str) or not SHA_RE.fullmatch(live_head): + fail(refusal) + try: + drift_failures = forward_map_placements(plan, live_head) + except PublishError as error: + fail(f"{refusal} ({error})") + posting_head = live_head - # Token identity (R7). A PAT answers /user; a GitHub App installation - # token gets 403 there, so its login is learned from apply's own first - # write (the anchor response) below. - status, user = client.request("GET", "/user") - login: Optional[str] = None - if status == 200 and isinstance(user, dict) and user.get("login"): - login = str(user["login"]) - elif status == 401: - fail("GitHub rejected the token (HTTP 401)") + # Token identity (R7). A GitHub App installation token resolves to + # None here and its login is learned from apply's own first write + # (the anchor response) below. + login: Optional[str] = resolve_token_login(client, args.bot_login) # Reconcile against comments already carrying this review's tags (R14). already_posted = extract_posted_ids( @@ -1368,7 +2435,13 @@ def command_apply(args: argparse.Namespace) -> int: file=sys.stderr, ) - poster = BatchPoster(client, repo, pr, plan, already_posted) + poster = BatchPoster( + client, repo, pr, plan, already_posted, commit_id=posting_head + ) + # A drift-unmapped span is failed before any write: it never enters a + # posted batch and lands in the summary with its reason (R15). + for finding_id, detail in drift_failures.items(): + poster.mark_failed([finding_id], detail) for batch in plan["batches"]: poster.post_batch(batch) @@ -1418,11 +2491,13 @@ def command_apply(args: argparse.Namespace) -> int: outcome_counts["no_position"], outcome_counts["routed"], outcome_counts["failed_inline"], + outcome_counts["skipped"], ), summary["context_lines"], sections, review_id, plan["config"].get("run_url") or "", + identity_tags=(summary["head_tag"], summary["base_tag"]), ) if summary_comment_id is None and anchor_failed and login is not None: # The anchor write may have landed despite its error; re-read @@ -1471,7 +2546,28 @@ def command_apply(args: argparse.Namespace) -> int: {"finding_id": finding_id, "detail": detail} for finding_id, detail in sorted(poster.failed.items()) ], + # Skip telemetry (P3): which earlier comment each finding + # deferred to, replayable from the plan artifact. + "skipped": [ + { + "finding_id": entry["finding_id"], + "reason": entry["reason"], + "detail": entry["detail"], + } + for entry in plan["placements"] + if entry["placement"] == "skipped" + ], } + if posting_head != plan["head"]: + outcome["head_drift"] = { + "plan_head": plan["head"], + "posted_against": posting_head, + "unmapped": sorted(drift_failures), + } + if plan["config"].get("history_unavailable"): + outcome["history_unavailable"] = plan["config"][ + "history_unavailable" + ] if stale_skip: outcome["summary_skipped"] = ( "a newer review's summary is already posted" @@ -1516,14 +2612,29 @@ def build_parser() -> argparse.ArgumentParser: plan.add_argument("--route-categories", default="") plan.add_argument("--batch-size", default=str(DEFAULT_BATCH_SIZE)) plan.add_argument("--run-url", default="") + plan.add_argument("--incremental", default="") + plan.add_argument("--incremental-overlap-threshold", default="") + plan.add_argument("--history", default="") plan.add_argument("--output", required=True) plan.set_defaults(handler=command_plan) + history = commands.add_parser( + "history", + help="fetch this PR's prior-review state into pr-history.json", + ) + history.add_argument("--repo", required=True) + history.add_argument("--pr", required=True, type=int) + history.add_argument("--api-base", default="https://api.github.com") + history.add_argument("--bot-login", default="") + history.add_argument("--output", required=True) + history.set_defaults(handler=command_history) + apply_ = commands.add_parser("apply", help="execute a publication plan") apply_.add_argument("--plan", required=True) apply_.add_argument("--repo", required=True) apply_.add_argument("--pr", required=True, type=int) apply_.add_argument("--api-base", required=True) + apply_.add_argument("--bot-login", default="") apply_.add_argument("--outcome", required=True) apply_.set_defaults(handler=command_apply) return parser diff --git a/.fabro/workflows/code-review/scripts/render_report.py b/.fabro/workflows/code-review/scripts/render_report.py index 170270c00..6063e762d 100644 --- a/.fabro/workflows/code-review/scripts/render_report.py +++ b/.fabro/workflows/code-review/scripts/render_report.py @@ -48,6 +48,10 @@ DISPOSITIONS = ( "verification-incomplete", "deferred-by-cap", "duplicate", + # P3 item 5: kept in the bundle, but the verifier judged it the same + # defect as a finding already posted on the reviewed PR; the + # publisher places it as skipped instead of re-posting. + "duplicate-of-posted", ) VERIFICATION_STATUSES = ("complete", "partial", "skipped-low-effort") COMPLETION_STATUSES = ("complete", "partial") @@ -178,6 +182,15 @@ def validate_manifest(value: object) -> Dict[str, Any]: verification = as_map(manifest.get("verification")) if verification.get("status") not in VERIFICATION_STATUSES: die("manifest verification.status is invalid") + incremental = manifest.get("incremental") + if incremental is not None: + incremental = as_map(incremental) + delta_from = incremental.get("from") + if delta_from is not None and ( + not isinstance(delta_from, str) + or not re.fullmatch(r"[0-9a-f]{40}", delta_from) + ): + die("manifest incremental.from must be a commit SHA or null") rules = manifest.get("rules") if rules is not None: rules = as_map(rules) @@ -359,6 +372,19 @@ def validate_finding(value: object, index: int) -> Dict[str, Any]: } if normalized_suggestion is not None: normalized["suggestion"] = normalized_suggestion + duplicate_of_posted = finding.get("duplicate_of_posted") + if duplicate_of_posted is not None: + record = as_map(duplicate_of_posted) + normalized["duplicate_of_posted"] = { + "comment_id": positive_int( + record.get("comment_id"), + f"{field}.duplicate_of_posted.comment_id", + ), + "html_url": safe_text( + record.get("html_url", ""), + f"{field}.duplicate_of_posted.html_url", + ), + } return normalized @@ -503,8 +529,13 @@ def validate_relationships( counts = manifest["counts"] if counts["reported"] != len(findings): die("manifest counts.reported does not match findings.json") + # A duplicate-of-posted finding stays in findings.json (the bundle + # never drops a finding); its ledger disposition names the earlier + # comment it defers to. reportable = [ - record for record in ledger if record["disposition"] == "reportable" + record + for record in ledger + if record["disposition"] in ("reportable", "duplicate-of-posted") ] if len(reportable) != len(findings): die("reportable ledger records do not match findings.json") @@ -720,6 +751,14 @@ def finding_markdown(finding: Mapping[str, Any]) -> List[str]: ) + " -- judged the same defect and folded in." ) + duplicate_of_posted = finding.get("duplicate_of_posted") + if isinstance(duplicate_of_posted, dict): + url = str(duplicate_of_posted.get("html_url") or "").strip() + lines.append( + "Already reported on the pull request" + + (f": {url}" if url else "") + + " -- not re-posted." + ) if finding["summary"].strip() != finding["short_summary"].strip(): lines.extend(["", escape_markdown(finding["summary"])]) lines.extend( @@ -958,6 +997,8 @@ def jsonl_line(finding: Mapping[str, Any]) -> str: } if finding.get("suggestion") is not None: record["suggestion"] = finding["suggestion"] + if finding.get("duplicate_of_posted") is not None: + record["duplicate_of_posted"] = finding["duplicate_of_posted"] return json.dumps(record, ensure_ascii=False, separators=(",", ":")) @@ -1116,6 +1157,10 @@ def sarif_result( ) }, } + if finding.get("duplicate_of_posted") is not None: + result["properties"]["duplicate_of_posted"] = finding[ + "duplicate_of_posted" + ] suggestion = finding.get("suggestion") if isinstance(suggestion, dict): result["fixes"] = [ diff --git a/.fabro/workflows/code-review/specs/report-spec.md b/.fabro/workflows/code-review/specs/report-spec.md index 28b213945..7828664d7 100644 --- a/.fabro/workflows/code-review/specs/report-spec.md +++ b/.fabro/workflows/code-review/specs/report-spec.md @@ -14,18 +14,27 @@ The canonical bundle is schema version 4. tiers (every tier above `low`) it also carries a `rules` block: the compiled rule layers, the rule configuration SHA-256, the built-in rule manifest SHA-256, the repository rule revision, and pack/check counts for - both layers. + both layers. An incremental PR re-review adds an `incremental` record: + whether it was requested and enabled, the last reviewed head the delta + started from (`from`, with the commit count), and any declared + history-fetch degradation. - `candidate-ledger.jsonl` contains every unique candidate after deduplication, plus every sweep candidate. Each record has one disposition: - `reportable`, `refuted`, `verification-incomplete`, `deferred-by-cap`, or - `duplicate` (folded into the finding named by `duplicate_of`), and - carries the candidate's applicable `rule_ids` (empty outside the - rule-mapped tiers). + `reportable`, `refuted`, `verification-incomplete`, `deferred-by-cap`, + `duplicate` (folded into the finding named by `duplicate_of`), or + `duplicate-of-posted` (judged the same defect as a finding already + posted on the reviewed PR; `duplicate_of_posted` names the covering + comment), and 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 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`. + stored as optional `suggestion.replacement_code`. A finding the verifier + judged already posted on the reviewed PR stays in the list (the bundle + never drops a finding) with an optional `duplicate_of_posted` record + naming the covering comment; the PR publisher skips it instead of + re-posting. - `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 diff --git a/.fabro/workflows/code-review/workflow.toml b/.fabro/workflows/code-review/workflow.toml index 8eb0c43b9..01271975f 100644 --- a/.fabro/workflows/code-review/workflow.toml +++ b/.fabro/workflows/code-review/workflow.toml @@ -29,6 +29,16 @@ route_severity_below = "" route_categories = "" # Optional run-report URL included in the sticky summary. run_url = "" +# Incremental re-review (P3): incremental = "true" makes prepare fetch the +# PR's prior-review history, review only the commits since the last +# reviewed head, and skip findings already covered by an earlier comment. +# incremental_overlap_threshold is the span IoU bound, strictly inside +# (0, 1); empty means 0.6. bot_login names the login whose comments count +# as ours (required in practice on GitHub App servers, where GET /user is +# unavailable to installation tokens; e.g. "fabro-sh-fabro[bot]"). +incremental = "" +incremental_overlap_threshold = "" +bot_login = "" # Full history, for arbitrary base and range inputs. [run.clone] @@ -86,6 +96,7 @@ include = [ "CODE-REVIEW-*/metadata/review-meta.json", "CODE-REVIEW-*/pr-publish-plan.json", "CODE-REVIEW-*/pr-publish-outcome.json", + "CODE-REVIEW-*/pr-history.json", ] [environments.code-review] diff --git a/tools/code-review-canary/canary.py b/tools/code-review-canary/canary.py new file mode 100644 index 000000000..958538d6a --- /dev/null +++ b/tools/code-review-canary/canary.py @@ -0,0 +1,35 @@ +"""Deliberately flawed fixture for the P3 incremental live acceptance. + +Planted correctness bugs, so a reviewed push with post_pr enabled is +guaranteed inline-postable findings: + +- ``percentile`` indexes past the end of the list when fraction is 1.0. +- ``moving_average`` divides every window by the full window size, so + the tail averages are too small. + +This file exists only on the calibration draft PR and is never merged. +""" + +# Calibration cycle 3: these two lines shift every function down. +# They exist to outdate and re-anchor the earlier review comments. + +def percentile(values, fraction): + """Return the value at the given fraction of the sorted input.""" + ordered = sorted(values) + index = int(len(ordered) * fraction) + return ordered[index] # still off the end at fraction == 1.0 + + +def moving_average(values, window): + """Average each window of the input, including the shorter tail.""" + averages = [] + for start in range(len(values)): + chunk = values[start:start + window] + averages.append(sum(chunk) / window) + return averages + + +def collect_values(values, collected=[]): + """Collect values for one independent operation.""" + collected.extend(values) + return collected