mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-11 22:53:00 +00:00
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WqM6MiUp32js5YrbiW777k
This commit is contained in:
Bryan Helmkamp
parent
9827efaaa6
commit
aa09341f4e
5 changed files with 164 additions and 100 deletions
|
|
@ -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 [
|
||||
|
|
|
|||
|
|
@ -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:
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue