From 71a544db1977671f4e6a3551680d9c925e8a9b9a Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 28 Aug 2026 19:49:50 -0400 Subject: [PATCH] Sync workflow: incremental cold-start base fallback and simplify pass From lithoscomputer/code-review fb12258: base is allowed with incremental as the cold-start fallback (the first run here quiet-exited on an empty upstream-based range), plus the P3 seam simplifications. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01W5V4zmmAs92ngEsSKrvpAJ --- .../workflows/code-review/code-review.fabro | 4 +- .../code-review/scripts/code_review.py | 79 +++++++++--------- .../code-review/scripts/publish_pr.py | 83 +++++++++---------- 3 files changed, 85 insertions(+), 81 deletions(-) diff --git a/.fabro/workflows/code-review/code-review.fabro b/.fabro/workflows/code-review/code-review.fabro index 816e7bbaf..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 882a5690ba9b4b60bc18f7bf38c13ef4773886ef38928ec36520d1fe20aff642 .fabro/workflows/code-review/scripts/git_readonly.py 29cee508724f7bee8d73317d82fe94d0d830361476b367e4a012b70071a0e841 .fabro/workflows/code-review/scripts/publish_pr.py 3a3ae944e3d643caf4bc4f91dce97d96c81fbfa11449b1412bb77342d8791b95 .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 }}'" + 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 }}' --incremental '{{ inputs.incremental }}' --incremental-overlap-threshold '{{ inputs.incremental_overlap_threshold }}' --bot-login '{{ inputs.bot_login }}'" + 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/scripts/code_review.py b/.fabro/workflows/code-review/scripts/code_review.py index 1e4e6f869..4ce9a9275 100644 --- a/.fabro/workflows/code-review/scripts/code_review.py +++ b/.fabro/workflows/code-review/scripts/code_review.py @@ -1105,14 +1105,16 @@ def import_publisher() -> Any: HISTORY_FILE_NAME = "pr-history.json" -def parse_incremental_input(raw: str) -> bool: - text = (raw or "").strip().lower() - if text in ("", "false", "0", "no", "off"): - return False - if text in ("true", "1", "yes", "on"): - return True - raise WorkflowDataError( - f"incremental must be true or false (or empty), got {raw!r}" +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, ) @@ -1135,7 +1137,6 @@ def prepare_incremental(args: argparse.Namespace, mode: str) -> Dict[str, Any]: "commits": None, "history_unavailable": None, "history_comments": [], - "range": None, } if mode != "changes": raise WorkflowDataError( @@ -1151,23 +1152,26 @@ def prepare_incremental(args: argparse.Namespace, mode: str) -> Dict[str, Any]: "pull request whose history to read" ) try: - publisher.parse_overlap_threshold(args.incremental_overlap_threshold) + # 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 = subprocess.run( + fetched = run_publisher( + publisher_path, [ - sys.executable, str(publisher_path), "history", + "history", "--repo", repo, "--pr", pr, "--api-base", args.api_base, "--bot-login", args.bot_login, "--output", str(history_path), ], - cwd=root(), - capture_output=True, ) if fetched.returncode != 0: reason = one_line( @@ -1206,7 +1210,6 @@ def prepare_incremental(args: argparse.Namespace, mode: str) -> Dict[str, Any]: # 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 - state["range"] = f"{last_head}..HEAD" commits = git_text("rev-list", "--count", f"{last_head}..HEAD") state["commits"] = ( int(commits) if commits and commits.isdigit() else None @@ -1665,17 +1668,29 @@ def prepare(args: argparse.Namespace) -> None: base_input = args.base.strip() commit_input = args.commit.strip() range_input = args.range.strip() - incremental_requested = parse_incremental_input(args.incremental) + 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 base_input or range_input or commit_input: + if range_input or commit_input: raise WorkflowDataError( - "incremental review resolves its own range; leave base, " - "commit, and range empty" + "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("range"): - range_input = str(incremental_state["range"]) + 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 @@ -3937,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: @@ -3980,22 +3984,25 @@ def publish_pr_command(args: argparse.Namespace) -> None: 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", - args.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(plan_arguments, plan_environment) + 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, @@ -4004,7 +4011,7 @@ def publish_pr_command(args: argparse.Namespace) -> None: "--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 {} @@ -4195,8 +4202,6 @@ 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("--incremental", default="") - publish_parser.add_argument("--incremental-overlap-threshold", default="") publish_parser.add_argument("--bot-login", default="") publish_parser.add_argument("--api-base", default="https://api.github.com") diff --git a/.fabro/workflows/code-review/scripts/publish_pr.py b/.fabro/workflows/code-review/scripts/publish_pr.py index bfd1a57b7..94cefa0e6 100644 --- a/.fabro/workflows/code-review/scripts/publish_pr.py +++ b/.fabro/workflows/code-review/scripts/publish_pr.py @@ -139,11 +139,17 @@ def base_tag_for(base: str) -> str: return f"" -def tagged_sha(body: str, prefix: str) -> Optional[str]: +def tag_value( + body: str, prefix: str, pattern: str = r"[^>]+" +) -> Optional[str]: match = re.search( - r"", body + r"", body ) - return match.group(1) if match else None + 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: @@ -244,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. @@ -782,7 +795,7 @@ def counts_line( no_position: int, routed: int, failed: int, - skipped: int = 0, + skipped: int, ) -> str: if total == 0: return ( @@ -1836,15 +1849,16 @@ def extract_posted_ids( # --- history (P3 item 1: the token-holding sibling of apply) ----------------- -def resolve_history_login( +def resolve_token_login( client: GitHubClient, bot_login: str ) -> Optional[str]: - """The login whose comments count as ours, for this read-only fetch. + """The login whose comments count as ours. The bot_login input wins when set; otherwise GET /user works on - PAT-mode servers. Apply's write-probe cannot be reused here -- the - probe is a write -- so on App-mode servers (where /user returns 403) - bot_login is effectively required for incremental to work at all. + 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: @@ -1893,10 +1907,7 @@ def history_mapped_span( SHA_RE.fullmatch(original_commit) ): return None - result = run_git( - "rev-parse", "--verify", "--quiet", original_commit + "^{commit}" - ) - if result.returncode != 0: + if not commit_exists(original_commit): return None original_start = ( comment_line_field(comment, "original_start_line") or original_line @@ -1930,7 +1941,7 @@ def command_history(args: argparse.Namespace) -> int: if not isinstance(live_head, str) or not SHA_RE.fullmatch(live_head): fail("the PR head is not a commit SHA") - login = resolve_history_login(client, args.bot_login) + 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 @@ -1992,12 +2003,9 @@ def command_history(args: argparse.Namespace) -> int: if owned_summaries: newest = max(owned_summaries, key=lambda comment: comment["id"]) body = str(newest.get("body") or "") - run_match = re.search( - r"", body - ) summary_record = { "comment_id": newest["id"], - "review_id": run_match.group(1).strip() if run_match else None, + "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), @@ -2033,10 +2041,7 @@ def command_history(args: argparse.Namespace) -> int: def completed_stamp(body: str) -> Optional[str]: - match = re.search( - r"", body - ) - return match.group(1).strip() if match else None + return tag_value(body, COMPLETED_TAG_PREFIX) def is_newer_stamp(existing: Optional[str], ours: str) -> bool: @@ -2073,12 +2078,12 @@ class BatchPoster: pr: int, plan: Mapping[str, Any], already_posted: Set[str], - commit_id: Optional[str] = None, + commit_id: str, ) -> None: self.client = client self.repo = repo self.pr = pr - self.head = commit_id or plan["head"] + self.head = commit_id self.by_id = { entry["finding_id"]: entry for entry in plan["placements"] @@ -2143,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 @@ -2234,16 +2242,15 @@ class BatchPoster: def ensure_commit_local(sha: str) -> bool: """True when the commit is available locally, fetching if needed.""" - check = run_git("rev-parse", "--verify", "--quiet", sha + "^{commit}") - if check.returncode == 0: + 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 - check = run_git("rev-parse", "--verify", "--quiet", sha + "^{commit}") - return check.returncode == 0 + commit_exists.cache_clear() + return commit_exists(sha) def forward_map_placements( @@ -2337,17 +2344,10 @@ def command_apply(args: argparse.Namespace) -> int: fail(f"{refusal} ({error})") posting_head = live_head - # Token identity (R7). The bot_login input names it directly when - # set; otherwise a PAT answers /user, while a GitHub App installation - # token gets 403 there and its login is learned from apply's own - # first write (the anchor response) below. - login: Optional[str] = (args.bot_login or "").strip() or None - if login is None: - status, user = client.request("GET", "/user") - 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( @@ -2441,8 +2441,7 @@ def command_apply(args: argparse.Namespace) -> int: # 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(): - if finding_id not in poster.posted: - poster.failed[finding_id] = detail + poster.mark_failed([finding_id], detail) for batch in plan["batches"]: poster.post_batch(batch)