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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01W5V4zmmAs92ngEsSKrvpAJ
This commit is contained in:
Bryan Helmkamp 2026-08-28 19:49:50 -04:00
parent 4a85a3e887
commit 71a544db19
No known key found for this signature in database
3 changed files with 85 additions and 81 deletions

View file

@ -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

View file

@ -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")

View file

@ -139,11 +139,17 @@ def base_tag_for(base: str) -> str:
return f"<!-- {BASE_TAG_PREFIX}:{base} -->"
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"<!-- " + re.escape(prefix) + r":([0-9a-f]{40}) -->", body
r"<!-- " + re.escape(prefix) + r":(" + pattern + 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"<!-- " + re.escape(RUN_TAG_PREFIX) + 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"<!-- " + re.escape(COMPLETED_TAG_PREFIX) + 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)