From aa09341f4e591a578670e75f9d85459b37e93390 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Fri, 28 Aug 2026 10:04:40 -0400 Subject: [PATCH] Refresh the code-review workflow (acceptance-run fixes) Syncs the post-acceptance state from lithoscomputer/code-review: the wiring simplification pass, the smoke-variant inputs the shared graph's publish_pr node now requires, the hunk-header parsing hardening, the root-commit diff base, and the verified-missing retry. These include the fixes for what the publisher's own first live run reported on PR #815. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WqM6MiUp32js5YrbiW777k --- .../workflows/code-review/code-review.fabro | 2 +- .../code-review/scripts/code_review.py | 73 ++++---- .../code-review/scripts/publish_pr.py | 169 ++++++++++++------ .../workflows/code-review/verify-xhigh.toml | 10 ++ .fabro/workflows/code-review/verify.toml | 10 ++ 5 files changed, 164 insertions(+), 100 deletions(-) diff --git a/.fabro/workflows/code-review/code-review.fabro b/.fabro/workflows/code-review/code-review.fabro index 01de9a984..0672be340 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 263fe507bc3bff9341b302305435af39ef95637f3b05e2ca5681daa812237ca1 .fabro/workflows/code-review/scripts/git_readonly.py bcd4364ba3aca2ee1e12d5909204f645c16bdf22e3753a39d74c79d8d37cf73e .fabro/workflows/code-review/scripts/publish_pr.py efb1afa9c0dac874da6c27d856adc5a7a5679acaa6307122b7c8afa5c0ddd4e2 .fabro/workflows/code-review/scripts/render_report.py fd8f5eb237a75e3d1e946279b27dabf7d08757e2a6aac8ff1732b504ee3d2655 .fabro/workflows/code-review/scripts/rule_loader.py f885409c64c631075d7e31fcb6e7a100592430c5eba9a6e989222006061a9f52 .fabro/workflows/code-review/specs/report-spec.md 006898b9e84951237b3fe862bd07f9be77070fa8d9cc8d28ce528c5ccf270fda .fabro/workflows/code-review/templates/report.html 04e8bffbbec4c4848646fc5ff3261aa06dd3fa95b1dfa21474f7dec1b91abd22 .fabro/workflows/code-review/schemas/findings.schema.json 6fdf7c63fb6183c8bef64a874cd56f805d92c2aa3c011a513ee07f07b0797b6d .fabro/workflows/code-review/schemas/verdict.schema.json 4cb752e624f1b88809017ac5db472d59850ffe09ec5a93d3b63ba636364b8b19 .fabro/workflows/code-review/schemas/file-groups.schema.json b53c4e1c0bbd07bbf70e83f4f3b35fd96cb880c621c7c424e95b9aea34e13d7c .fabro/workflows/code-review/prompts/finder.md.j2 86c2e6a032f7c54c1bbab1c12496a8f0d6bf48703abe6017eb175330608cf223 .fabro/workflows/code-review/prompts/verify.md.j2 6528cffd1bf199f27a32c19547aebf6f0c78a546698aaef81c6a89441b80f7da .fabro/workflows/code-review/prompts/sweep.md.j2 e6f89b47b11c57030a6ef7d5896ccb37dbd2a2982e9fa7eb7f2e3df73acab82c .fabro/workflows/code-review/prompts/group-files.md.j2 5b291313a1266d1d658f80ea7989cdefcd8609b6893b7197b17914d553dab041 .fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 b91ddeb86dc7f552323f7ac04823984da9365d013bfe7965c3eb6f51fa8abe0f .fabro/workflows/code-review/prompts/partials/guidance.md.j2 53bc0c40bb917288708bed1f9ba478fbd89b9790c92497762224cc752f40bef5 .fabro/workflows/code-review/prompts/partials/output-schema.md.j2 811994bb357739f2562d84f66dc05075ebe3c7f8d58034f8c25ee1c36bee996b .fabro/workflows/code-review/prompts/partials/read-only-explorer.md.j2 44a0244e7aa62fdb0dbbfdbadcffbfb640af249bae3e96895dedd5c7a33bad10 .fabro/workflows/code-review/prompts/partials/review-target.md.j2 abffeeff0e16b89a0754cd53f1833b3744494cd54ff761798b782a80467446ea .fabro/workflows/code-review/prompts/partials/safe-git-history.md.j2 4ddd8d36d5c51d7e166a6b7f1dff51b72cce0e64108cc7e892002ca909af8b3a .fabro/workflows/code-review/rules/builtin-manifest.json 47e3123fb25c560e36216dc9b8323c86e8301f1868a4fa3219bfd59bda3a533a && python3 .fabro/workflows/code-review/scripts/code_review.py prepare --review-id-stdin --mode {{ inputs.mode }} --effort {{ inputs.effort }} --scope {{ inputs.scope }} --base {{ inputs.base }} --commit {{ inputs.commit }} --range {{ inputs.range }} --model {{ inputs.model }} --guidance {{ inputs.guidance }}" + script="python3 -c \"import hashlib,sys; pairs=list(zip(sys.argv[1::2],sys.argv[2::2])); sys.exit(0 if pairs and all(hashlib.sha256(open(path,'rb').read()).hexdigest()==expected for path,expected in pairs) else 91)\" .fabro/workflows/code-review/scripts/code_review.py e05d017c71a16c8418c1cf941ae134695520ccd6db62186f251706d7e635eafa .fabro/workflows/code-review/scripts/git_readonly.py bcd4364ba3aca2ee1e12d5909204f645c16bdf22e3753a39d74c79d8d37cf73e .fabro/workflows/code-review/scripts/publish_pr.py 0372308e1dc9ecbedef18fe7898fd40c73f0b90dcbd062ca99f9f38f833278d3 .fabro/workflows/code-review/scripts/render_report.py fd8f5eb237a75e3d1e946279b27dabf7d08757e2a6aac8ff1732b504ee3d2655 .fabro/workflows/code-review/scripts/rule_loader.py f885409c64c631075d7e31fcb6e7a100592430c5eba9a6e989222006061a9f52 .fabro/workflows/code-review/specs/report-spec.md 006898b9e84951237b3fe862bd07f9be77070fa8d9cc8d28ce528c5ccf270fda .fabro/workflows/code-review/templates/report.html 04e8bffbbec4c4848646fc5ff3261aa06dd3fa95b1dfa21474f7dec1b91abd22 .fabro/workflows/code-review/schemas/findings.schema.json 6fdf7c63fb6183c8bef64a874cd56f805d92c2aa3c011a513ee07f07b0797b6d .fabro/workflows/code-review/schemas/verdict.schema.json 4cb752e624f1b88809017ac5db472d59850ffe09ec5a93d3b63ba636364b8b19 .fabro/workflows/code-review/schemas/file-groups.schema.json b53c4e1c0bbd07bbf70e83f4f3b35fd96cb880c621c7c424e95b9aea34e13d7c .fabro/workflows/code-review/prompts/finder.md.j2 86c2e6a032f7c54c1bbab1c12496a8f0d6bf48703abe6017eb175330608cf223 .fabro/workflows/code-review/prompts/verify.md.j2 6528cffd1bf199f27a32c19547aebf6f0c78a546698aaef81c6a89441b80f7da .fabro/workflows/code-review/prompts/sweep.md.j2 e6f89b47b11c57030a6ef7d5896ccb37dbd2a2982e9fa7eb7f2e3df73acab82c .fabro/workflows/code-review/prompts/group-files.md.j2 5b291313a1266d1d658f80ea7989cdefcd8609b6893b7197b17914d553dab041 .fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 b91ddeb86dc7f552323f7ac04823984da9365d013bfe7965c3eb6f51fa8abe0f .fabro/workflows/code-review/prompts/partials/guidance.md.j2 53bc0c40bb917288708bed1f9ba478fbd89b9790c92497762224cc752f40bef5 .fabro/workflows/code-review/prompts/partials/output-schema.md.j2 811994bb357739f2562d84f66dc05075ebe3c7f8d58034f8c25ee1c36bee996b .fabro/workflows/code-review/prompts/partials/read-only-explorer.md.j2 44a0244e7aa62fdb0dbbfdbadcffbfb640af249bae3e96895dedd5c7a33bad10 .fabro/workflows/code-review/prompts/partials/review-target.md.j2 abffeeff0e16b89a0754cd53f1833b3744494cd54ff761798b782a80467446ea .fabro/workflows/code-review/prompts/partials/safe-git-history.md.j2 4ddd8d36d5c51d7e166a6b7f1dff51b72cce0e64108cc7e892002ca909af8b3a .fabro/workflows/code-review/rules/builtin-manifest.json 47e3123fb25c560e36216dc9b8323c86e8301f1868a4fa3219bfd59bda3a533a && python3 .fabro/workflows/code-review/scripts/code_review.py prepare --review-id-stdin --mode {{ inputs.mode }} --effort {{ inputs.effort }} --scope {{ inputs.scope }} --base {{ inputs.base }} --commit {{ inputs.commit }} --range {{ inputs.range }} --model {{ inputs.model }} --guidance {{ inputs.guidance }}" ] grouping [ diff --git a/.fabro/workflows/code-review/scripts/code_review.py b/.fabro/workflows/code-review/scripts/code_review.py index b8470f078..d2d84fae3 100644 --- a/.fabro/workflows/code-review/scripts/code_review.py +++ b/.fabro/workflows/code-review/scripts/code_review.py @@ -895,7 +895,8 @@ def assert_workspace_unchanged(state: Mapping[str, Any]) -> None: Tamper evidence behind the read-only tool guard: an agent that finds a way to write could shape what the verifiers and the report see. Checked only at - the publication gates (final-tally and render-report). + the publication gates: final-tally, render-report, and publish-pr (the + last immediately before anything leaves for GitHub). """ expected = state.get("workspace_digest") actual = workspace_digest() @@ -3357,12 +3358,15 @@ def final_tally() -> None: # --- Rendering and expectations ---------------------------------------------- -def load_renderer() -> Any: - path = (root() / RENDERER_PATH).resolve() +def resolve_workflow_script(rel_path: Path, description: str) -> Path: + path = (root() / rel_path).resolve() if not path.is_file(): - raise WorkflowDataError( - f"the deterministic renderer is missing: {RENDERER_PATH}" - ) + raise WorkflowDataError(f"the {description} is missing: {rel_path}") + return path + + +def load_renderer() -> Any: + path = resolve_workflow_script(RENDERER_PATH, "deterministic renderer") spec = importlib.util.spec_from_file_location( "code_review_render_report", path, @@ -3417,12 +3421,7 @@ def publish_pr_command(args: argparse.Namespace) -> None: GITHUB_TOKEN. The plan and outcome files land in the products directory as replayable evidence, peers of the canonical bundle. """ - requested = str(args.post_pr or "").strip().lower() in ( - "true", - "1", - "yes", - "on", - ) + requested = args.post_pr.strip().lower() in ("true", "1", "yes", "on") if not requested: print("PR publishing not requested (post_pr is off)") emit(publish_pr={"requested": False}) @@ -3434,26 +3433,20 @@ def publish_pr_command(args: argparse.Namespace) -> None: "final-tally and render-report" ) assert_workspace_unchanged(state) - repo = str(args.pr_repo or "").strip() - pr_text = str(args.pr_number or "").strip() + repo = args.pr_repo.strip() + pr_text = args.pr_number.strip() if not repo or not pr_text: raise WorkflowDataError( "post_pr is enabled but pr_repo/pr_number do not name the " "target pull request" ) - if not pr_text.isdigit() or int(pr_text) < 1: - raise WorkflowDataError( - f"pr_number must be a positive integer, got {pr_text!r}" - ) if not os.environ.get("GITHUB_TOKEN"): raise WorkflowDataError( "publish-pr needs GITHUB_TOKEN in the environment; the " "workflow's [run.integrations.github.permissions] makes Fabro " "inject one when its GitHub integration is configured" ) - publisher = (root() / PUBLISHER_PATH).resolve() - if not publisher.is_file(): - raise WorkflowDataError(f"the PR publisher is missing: {PUBLISHER_PATH}") + publisher = resolve_workflow_script(PUBLISHER_PATH, "PR publisher") products_rel = str(state["products_rel"]) evidence_rel = str(state["evidence_rel"]) plan_rel = f"{products_rel}/pr-publish-plan.json" @@ -3470,11 +3463,21 @@ def publish_pr_command(args: argparse.Namespace) -> None: capture_output=True, ) - # The plan step needs no credentials and runs without any (R18). + def publisher_error( + prefix: str, failed: subprocess.CompletedProcess + ) -> WorkflowDataError: + detail = failed.stderr.decode("utf-8", "replace").strip() + return WorkflowDataError(prefix + one_line(detail, 2000)) + + # The plan step needs no credentials and runs with none (R18). Its + # environment is rebuilt from a benign allowlist, so a credential + # injected under any name -- not just the ones Fabro uses today -- + # never reaches the plan. plan_environment = { key: value for key, value in os.environ.items() - if key not in ("GITHUB_TOKEN", "GH_TOKEN") + if key in ("PATH", "HOME", "TZ", "USER", "LOGNAME", "SHELL") + or key.startswith(("LANG", "LC_", "PYTHON", "TMP", "TEMP")) } result = run_publisher( [ @@ -3482,18 +3485,15 @@ def publish_pr_command(args: argparse.Namespace) -> None: "--evidence-dir", evidence_rel, "--repo", repo, "--pr", pr_text, - "--route-severity-below", str(args.route_severity_below or ""), - "--route-categories", str(args.route_categories or ""), + "--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, ) if result.returncode != 0: - detail = result.stderr.decode("utf-8", "replace").strip() - raise WorkflowDataError( - "the publication plan failed: " + one_line(detail, 2000) - ) + raise publisher_error("the publication plan failed: ", result) print(result.stdout.decode("utf-8", "replace").strip()) result = run_publisher( @@ -3502,16 +3502,12 @@ def publish_pr_command(args: argparse.Namespace) -> None: "--plan", plan_rel, "--repo", repo, "--pr", pr_text, - "--api-base", str(args.api_base), + "--api-base", args.api_base, "--outcome", outcome_rel, ] ) - outcome_path = root() / outcome_rel - outcome: Dict[str, Any] = {} - if outcome_path.is_file(): - value = read_json(outcome_path) - if isinstance(value, dict): - outcome = value + value = read_json(root() / outcome_rel, required=False) + outcome: Dict[str, Any] = value if isinstance(value, dict) else {} updates: Dict[str, Any] = {"requested": True} if outcome: updates["counts"] = outcome.get("counts") @@ -3522,10 +3518,7 @@ def publish_pr_command(args: argparse.Namespace) -> None: if stdout_text: print(stdout_text) if result.returncode != 0: - detail = result.stderr.decode("utf-8", "replace").strip() - raise WorkflowDataError( - "posting to the PR failed: " + one_line(detail, 2000) - ) + raise publisher_error("posting to the PR failed: ", result) def lint_rules() -> None: diff --git a/.fabro/workflows/code-review/scripts/publish_pr.py b/.fabro/workflows/code-review/scripts/publish_pr.py index 985f3248b..7aaabedd9 100644 --- a/.fabro/workflows/code-review/scripts/publish_pr.py +++ b/.fabro/workflows/code-review/scripts/publish_pr.py @@ -126,11 +126,36 @@ def resolve_commit(token: str, field: str) -> str: return resolved +def resolve_diff_base(token: str) -> str: + """The diff base as a commit, or a bare tree for a root commit. + + A root commit's reviewed range starts at the empty tree, which is + tree-ish but not a commit; ``git diff`` accepts it as a base. + """ + result = run_git("rev-parse", "--verify", "--quiet", token + "^{commit}") + resolved = result.stdout.decode("utf-8", "replace").strip() + if result.returncode == 0 and SHA_RE.fullmatch(resolved): + return resolved + result = run_git("rev-parse", "--verify", "--quiet", token + "^{tree}") + resolved = result.stdout.decode("utf-8", "replace").strip() + if result.returncode == 0 and SHA_RE.fullmatch(resolved): + return resolved + fail( + f"range base {token!r} does not resolve to a commit or tree in " + "this repository; plan must run inside the reviewed checkout" + ) + + def right_side_hunks(base: str, head: str) -> Dict[str, List[Tuple[int, int]]]: """RIGHT-side hunk line ranges of ``git diff -U3 base head`` (R2). Hunks include context lines; a pure-deletion hunk has no RIGHT-side - lines and is skipped. + lines and is skipped. A ``+++ `` target line counts as a file header + only inside a file's preamble (between its ``diff --git`` boundary + and its first hunk): an added body line whose content starts with + ``++ `` renders as ``+++ `` but cannot reach the preamble, because + every hunk body line carries a +/-/space marker while a real file + boundary starts bare. """ result = run_git( "diff", "--no-color", "--no-ext-diff", "--find-renames", "-U3", @@ -141,8 +166,12 @@ def right_side_hunks(base: str, head: str) -> Dict[str, List[Tuple[int, int]]]: fail(f"git diff over the reviewed range failed: {detail}") ranges: Dict[str, List[Tuple[int, int]]] = {} current: Optional[str] = None + in_preamble = False for line in result.stdout.decode("utf-8", "replace").splitlines(): - if line.startswith("+++ "): + if line.startswith("diff --git "): + in_preamble = True + current = None + elif in_preamble and line.startswith("+++ "): target = line[4:] if target == "/dev/null" or target.startswith('"'): current = None @@ -150,7 +179,10 @@ def right_side_hunks(base: str, head: str) -> Dict[str, List[Tuple[int, int]]]: current = target[2:] else: current = target - elif line.startswith("@@ ") and current is not None: + elif line.startswith("@@ "): + in_preamble = False + if current is None: + continue match = HUNK_HEADER_RE.match(line) if not match: continue @@ -211,6 +243,13 @@ def parse_batch_size(raw: str) -> int: return value if value >= 1 else DEFAULT_BATCH_SIZE +def parse_pr_number(raw: str) -> int: + text = str(raw).strip() + if not text.isdigit() or int(text) < 1: + fail(f"pr must be a positive integer, got {raw!r}") + return int(text) + + def routing_detail( finding: Mapping[str, Any], threshold: Optional[str], @@ -515,7 +554,7 @@ def resolve_reviewed_range(manifest: Mapping[str, Any]) -> Tuple[str, str, str]: fail(f"the manifest range is not two-sided: {range_text!r}") if not left_token: fail(f"the manifest range has no base side: {range_text!r}") - left_sha = resolve_commit(left_token, "range base") + left_sha = resolve_diff_base(left_token) resolved_head = resolve_commit(head, "reviewed head") if resolved_head != head: fail("the reviewed head commit is not present in this repository") @@ -533,9 +572,7 @@ def command_plan(args: argparse.Namespace) -> int: 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 = int(args.pr) - if pr < 1: - fail("pr must be a positive integer") + pr = parse_pr_number(args.pr) # Fail-closed routing policy (R5): a malformed configuration fails the # plan before anything can be posted. threshold = parse_severity_threshold(args.route_severity_below) @@ -1016,12 +1053,18 @@ class BatchPoster: def is_server_failure(status: Optional[int]) -> bool: return status is None or status == 408 or (status >= 500) - def mark_unverified(self, finding_ids: Sequence[str]) -> None: + UNVERIFIED_DETAIL = ( + "a server error interrupted the write and the result could not " + "be verified" + ) + DROPPED_DETAIL = ( + "a server error dropped the write; the comment was verified " + "missing and the retry also failed" + ) + + def mark_failed(self, finding_ids: Sequence[str], detail: str) -> None: for finding_id in finding_ids: - self.failed[finding_id] = ( - "a server error interrupted the write and the result could " - "not be verified" - ) + self.failed[finding_id] = detail def reconcile(self, finding_ids: Sequence[str]) -> List[str]: """After a possibly-landed failure: absorb what landed, return what @@ -1029,7 +1072,7 @@ class BatchPoster: return nothing (never risk a duplicate, R14).""" landed = self.landed_ids() if landed is None: - self.mark_unverified(finding_ids) + self.mark_failed(finding_ids, self.UNVERIFIED_DETAIL) return [] self.posted.update(landed & set(self.by_id)) return [fid for fid in finding_ids if fid not in landed] @@ -1047,7 +1090,20 @@ class BatchPoster: elif self.is_server_failure(status): missing = self.reconcile([finding_id]) if missing: - self.mark_unverified(missing) + # Verified missing, so one retry cannot duplicate (R14). + retry_status = self.post_review([finding_id]) + if retry_status in (200, 201): + self.posted.add(finding_id) + elif retry_status == 422: + self.failed[finding_id] = ( + "GitHub could not resolve the diff position (422)" + ) + else: + still_missing = self.reconcile([finding_id]) + if still_missing: + self.mark_failed( + still_missing, self.DROPPED_DETAIL + ) else: self.failed[finding_id] = f"GitHub refused the comment (HTTP {status})" @@ -1071,7 +1127,9 @@ class BatchPoster: else: still_missing = self.reconcile(missing) if still_missing: - self.mark_unverified(still_missing) + self.mark_failed( + still_missing, self.DROPPED_DETAIL + ) else: for finding_id in to_send: self.failed[finding_id] = ( @@ -1088,9 +1146,7 @@ def command_apply(args: argparse.Namespace) -> int: 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 = int(args.pr) - if pr < 1: - fail("pr must be a positive integer") + pr = parse_pr_number(args.pr) try: raw_plan = json.loads(Path(args.plan).read_text(encoding="utf-8")) except (OSError, UnicodeError, json.JSONDecodeError) as error: @@ -1128,49 +1184,46 @@ def command_apply(args: argparse.Namespace) -> int: ) # Sticky-summary discovery (R7): only a marker comment authored by our - # own token identity is ever updated; the newest owned one wins. + # own token identity is ever updated; the newest owned one wins. While + # the login is still unknown, no comment counts as owned. def find_owned_summary( comments: Sequence[Mapping[str, Any]], - author: Optional[str], ) -> Optional[Dict[str, Any]]: - if author is None: + if login is None: return None owned = [ comment for comment in comments if SUMMARY_MARKER in str(comment.get("body") or "") - and (comment.get("user") or {}).get("login") == author + and (comment.get("user") or {}).get("login") == login and isinstance(comment.get("id"), int) ] if not owned: return None return max(owned, key=lambda comment: comment["id"]) - def create_anchor() -> Tuple[Optional[int], Optional[Dict[str, Any]]]: - return client.request( - "POST", - f"/repos/{repo}/issues/{pr}/comments", - {"body": summary["anchor_body"]}, - ) - prior_comments = client.list_all(f"/repos/{repo}/issues/{pr}/comments") summary_comment_id: Optional[int] = None summary_url = "" stale_skip = False anchor_failed = False - if login is not None: - existing = find_owned_summary(prior_comments, login) - if existing is not None: - stamp = completed_stamp(str(existing.get("body") or "")) - if is_newer_stamp(stamp, summary["completed_at"]): - # Stale-run guard (R14): never overwrite a newer review's - # summary. - stale_skip = True - summary_url = str(existing.get("html_url") or "") - else: - summary_comment_id = existing["id"] - summary_url = str(existing.get("html_url") or "") + # Take over an owned summary comment as the one to update -- unless it + # carries a newer review's completed tag: the stale-run guard (R14) + # never overwrites a newer review's summary. + def adopt(existing: Mapping[str, Any]) -> None: + nonlocal stale_skip, summary_comment_id, summary_url + summary_url = str(existing.get("html_url") or "") + stamp = completed_stamp(str(existing.get("body") or "")) + if is_newer_stamp(stamp, summary["completed_at"]): + stale_skip = True + summary_comment_id = None + else: + summary_comment_id = existing["id"] + + existing = find_owned_summary(prior_comments) + if existing is not None: + adopt(existing) # Anchor before review on a cold start (R8). A failed anchor does not # abort the run; the final summary write settles the outcome (R16). @@ -1180,31 +1233,30 @@ def command_apply(args: argparse.Namespace) -> int: # one summary (R7); if the delete fails, the probe is the newest owned # comment and later runs converge on it. if not stale_skip and summary_comment_id is None: - status, created = create_anchor() + status, created = client.request( + "POST", + f"/repos/{repo}/issues/{pr}/comments", + {"body": summary["anchor_body"]}, + ) if status in (200, 201) and isinstance(created, dict) and isinstance( created.get("id"), int ): - anchor_id = created["id"] - anchor_url = str(created.get("html_url") or "") - summary_comment_id = anchor_id - summary_url = anchor_url + summary_comment_id = created["id"] + summary_url = str(created.get("html_url") or "") if login is None: login = (created.get("user") or {}).get("login") - prior = find_owned_summary(prior_comments, login) + prior = find_owned_summary(prior_comments) if prior is not None: delete_status, _ = client.request( "DELETE", - f"/repos/{repo}/issues/comments/{anchor_id}", + f"/repos/{repo}/issues/comments/{summary_comment_id}", ) deleted = delete_status in (200, 204) - stamp = completed_stamp(str(prior.get("body") or "")) - if is_newer_stamp(stamp, summary["completed_at"]): - stale_skip = True - summary_comment_id = None - summary_url = str(prior.get("html_url") or "") - elif deleted: - summary_comment_id = prior["id"] - summary_url = str(prior.get("html_url") or "") + if deleted or is_newer_stamp( + completed_stamp(str(prior.get("body") or "")), + summary["completed_at"], + ): + adopt(prior) else: anchor_failed = True print( @@ -1268,13 +1320,12 @@ def command_apply(args: argparse.Namespace) -> int: review_id, plan["config"].get("run_url") or "", ) - if summary_comment_id is None and anchor_failed: + 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 # before choosing create over update (R14). try: landed = find_owned_summary( - client.list_all(f"/repos/{repo}/issues/{pr}/comments"), - login, + client.list_all(f"/repos/{repo}/issues/{pr}/comments") ) except PublishError: landed = None diff --git a/.fabro/workflows/code-review/verify-xhigh.toml b/.fabro/workflows/code-review/verify-xhigh.toml index 6c5bdf0f5..02d73b822 100644 --- a/.fabro/workflows/code-review/verify-xhigh.toml +++ b/.fabro/workflows/code-review/verify-xhigh.toml @@ -25,6 +25,16 @@ expected_min_findings = "2" expected_file = ".fabro/workflows/code-review/fixtures/rules_probe.py" expected_min_rule_findings = "1" +# The shared graph's publish_pr node interpolates these inputs, so every +# workflow.toml that runs the graph must define them; smoke runs never +# post, so they stay empty. +post_pr = "" +pr_repo = "" +pr_number = "" +route_severity_below = "" +route_categories = "" +run_url = "" + [run.run_branch] enabled = false diff --git a/.fabro/workflows/code-review/verify.toml b/.fabro/workflows/code-review/verify.toml index a5cfd52a6..77d5f39cb 100644 --- a/.fabro/workflows/code-review/verify.toml +++ b/.fabro/workflows/code-review/verify.toml @@ -24,6 +24,16 @@ expected_min_findings = "1" expected_file = ".fabro/workflows/code-review/fixtures/inventory_utils.py" expected_min_rule_findings = "1" +# The shared graph's publish_pr node interpolates these inputs, so every +# workflow.toml that runs the graph must define them; smoke runs never +# post, so they stay empty. +post_pr = "" +pr_repo = "" +pr_number = "" +route_severity_below = "" +route_categories = "" +run_url = "" + [run.run_branch] enabled = false