mirror of
https://github.com/fabro-sh/fabro.git
synced 2026-09-13 23:14:17 +00:00
Merge 31a10144f9 into d6fc85b9a8
This commit is contained in:
Bryan Helmkamp
GitHub
commit
999fd25f6e
8 changed files with 1581 additions and 81 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 78d239edb68be8e3db983445a786b9dca7eed6044ff4430e5581ae4c4c8466d3 .fabro/workflows/code-review/scripts/git_readonly.py 29cee508724f7bee8d73317d82fe94d0d830361476b367e4a012b70071a0e841 .fabro/workflows/code-review/scripts/publish_pr.py 35cde9006c9d079f468228498704c7c6079bff52fc790d7e2cee9505d3de9a06 .fabro/workflows/code-review/scripts/render_report.py fe432d8a53e1294b8030a54ca17b412312338e761ee751ab770e221450b53b8c .fabro/workflows/code-review/scripts/review_contract.py 8917fe7ae046cfda547f4f1240570fa295e110fd5e8d0c84c2a98719137db6fa .fabro/workflows/code-review/scripts/rule_loader.py eaa7258e5cf7b231a7a1192c9738eb2a0486480cd04ebe2059a79295040fc66e .fabro/workflows/code-review/specs/report-spec.md 7a54f72ee46f09218d18854d184a1f36875f9011877e94779c6b1f0d5dd118a9 .fabro/workflows/code-review/templates/report.html 5def570da34ca186da31781378367d70fb9c58e82f7aeec4aaf420fd348a8e61 .fabro/workflows/code-review/schemas/findings.schema.json 2f4d0a9052d5af0dad92db12a1e9d49cc91a282c4dddda495791352bf1559ed8 .fabro/workflows/code-review/schemas/verdict.schema.json de13ce02c5fd0c088640542831cc732e35dee3ddb38f89d4412f6a46fea75567 .fabro/workflows/code-review/schemas/file-groups.schema.json b53c4e1c0bbd07bbf70e83f4f3b35fd96cb880c621c7c424e95b9aea34e13d7c .fabro/workflows/code-review/prompts/finder.md.j2 86c2e6a032f7c54c1bbab1c12496a8f0d6bf48703abe6017eb175330608cf223 .fabro/workflows/code-review/prompts/verify.md.j2 cb3866240077d1bc8993b2f012a8d66a6ea61d4a9f2e88a1fefc6ef375f630e2 .fabro/workflows/code-review/prompts/sweep.md.j2 e6f89b47b11c57030a6ef7d5896ccb37dbd2a2982e9fa7eb7f2e3df73acab82c .fabro/workflows/code-review/prompts/group-files.md.j2 5b291313a1266d1d658f80ea7989cdefcd8609b6893b7197b17914d553dab041 .fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 a81ee5b0ac134eb121dbf503025387c64126d3276e4673ebc836cfb62a3689fb .fabro/workflows/code-review/prompts/partials/guidance.md.j2 53bc0c40bb917288708bed1f9ba478fbd89b9790c92497762224cc752f40bef5 .fabro/workflows/code-review/prompts/partials/output-schema.md.j2 811994bb357739f2562d84f66dc05075ebe3c7f8d58034f8c25ee1c36bee996b .fabro/workflows/code-review/prompts/partials/read-only-explorer.md.j2 44a0244e7aa62fdb0dbbfdbadcffbfb640af249bae3e96895dedd5c7a33bad10 .fabro/workflows/code-review/prompts/partials/review-target.md.j2 abffeeff0e16b89a0754cd53f1833b3744494cd54ff761798b782a80467446ea .fabro/workflows/code-review/prompts/partials/safe-git-history.md.j2 4ddd8d36d5c51d7e166a6b7f1dff51b72cce0e64108cc7e892002ca909af8b3a .fabro/workflows/code-review/rules/builtin-manifest.json ecd1d77ad8c77cae153280cb775e5e5f7fa9b68925900473e7d2331af377bb49 && python3 .fabro/workflows/code-review/scripts/code_review.py prepare --review-id-stdin --mode {{ inputs.mode }} --effort {{ inputs.effort }} --scope {{ inputs.scope }} --base {{ inputs.base }} --commit {{ inputs.commit }} --range {{ inputs.range }} --model {{ inputs.model }} --guidance {{ inputs.guidance }}"
|
||||
script="python3 -c \"import hashlib,sys; pairs=list(zip(sys.argv[1::2],sys.argv[2::2])); sys.exit(0 if pairs and all(hashlib.sha256(open(path,'rb').read()).hexdigest()==expected for path,expected in pairs) else 91)\" .fabro/workflows/code-review/scripts/code_review.py 38f8fdb8a79b929fdcefc4e38a193f4221d05a827914ea5cadf93a6f8f86a13a .fabro/workflows/code-review/scripts/git_readonly.py 29cee508724f7bee8d73317d82fe94d0d830361476b367e4a012b70071a0e841 .fabro/workflows/code-review/scripts/publish_pr.py 48416e9a4fcaafb251a17dc0923666d2d548992f48c811d4f1c08d2646331e46 .fabro/workflows/code-review/scripts/render_report.py 1eb5076b1697d734eb053ab0a941290c9c0e93c0ea1a3dfbf8a62c102defb935 .fabro/workflows/code-review/scripts/review_contract.py 8917fe7ae046cfda547f4f1240570fa295e110fd5e8d0c84c2a98719137db6fa .fabro/workflows/code-review/scripts/rule_loader.py eaa7258e5cf7b231a7a1192c9738eb2a0486480cd04ebe2059a79295040fc66e .fabro/workflows/code-review/specs/report-spec.md 4bbc85934ae8d6109a2f07f274fa6806828189db1f7695c565e8e14e9621b572 .fabro/workflows/code-review/templates/report.html 5def570da34ca186da31781378367d70fb9c58e82f7aeec4aaf420fd348a8e61 .fabro/workflows/code-review/schemas/findings.schema.json 2f4d0a9052d5af0dad92db12a1e9d49cc91a282c4dddda495791352bf1559ed8 .fabro/workflows/code-review/schemas/verdict.schema.json de13ce02c5fd0c088640542831cc732e35dee3ddb38f89d4412f6a46fea75567 .fabro/workflows/code-review/schemas/file-groups.schema.json b53c4e1c0bbd07bbf70e83f4f3b35fd96cb880c621c7c424e95b9aea34e13d7c .fabro/workflows/code-review/prompts/finder.md.j2 86c2e6a032f7c54c1bbab1c12496a8f0d6bf48703abe6017eb175330608cf223 .fabro/workflows/code-review/prompts/verify.md.j2 47c47946e41b2738d52b0a59b13d088f055dee9aa79b40f7b7b6f1614ecf8925 .fabro/workflows/code-review/prompts/sweep.md.j2 e6f89b47b11c57030a6ef7d5896ccb37dbd2a2982e9fa7eb7f2e3df73acab82c .fabro/workflows/code-review/prompts/group-files.md.j2 5b291313a1266d1d658f80ea7989cdefcd8609b6893b7197b17914d553dab041 .fabro/workflows/code-review/prompts/partials/finding-fields.md.j2 a81ee5b0ac134eb121dbf503025387c64126d3276e4673ebc836cfb62a3689fb .fabro/workflows/code-review/prompts/partials/guidance.md.j2 53bc0c40bb917288708bed1f9ba478fbd89b9790c92497762224cc752f40bef5 .fabro/workflows/code-review/prompts/partials/output-schema.md.j2 811994bb357739f2562d84f66dc05075ebe3c7f8d58034f8c25ee1c36bee996b .fabro/workflows/code-review/prompts/partials/read-only-explorer.md.j2 44a0244e7aa62fdb0dbbfdbadcffbfb640af249bae3e96895dedd5c7a33bad10 .fabro/workflows/code-review/prompts/partials/review-target.md.j2 abffeeff0e16b89a0754cd53f1833b3744494cd54ff761798b782a80467446ea .fabro/workflows/code-review/prompts/partials/safe-git-history.md.j2 4ddd8d36d5c51d7e166a6b7f1dff51b72cce0e64108cc7e892002ca909af8b3a .fabro/workflows/code-review/rules/builtin-manifest.json ecd1d77ad8c77cae153280cb775e5e5f7fa9b68925900473e7d2331af377bb49 && python3 .fabro/workflows/code-review/scripts/code_review.py prepare --review-id-stdin --mode {{ inputs.mode }} --effort {{ inputs.effort }} --scope {{ inputs.scope }} --base {{ inputs.base }} --commit {{ inputs.commit }} --range {{ inputs.range }} --model {{ inputs.model }} --guidance {{ inputs.guidance }} --incremental '{{ inputs.incremental }}' --incremental-overlap-threshold '{{ inputs.incremental_overlap_threshold }}' --pr-repo '{{ inputs.pr_repo }}' --pr-number '{{ inputs.pr_number }}' --bot-login '{{ inputs.bot_login }}'"
|
||||
]
|
||||
|
||||
grouping [
|
||||
|
|
@ -206,7 +206,7 @@ digraph CodeReview {
|
|||
label="Publish findings to the reviewed PR (opt-in)",
|
||||
timeout="900s",
|
||||
output_schema="routing",
|
||||
script="python3 .fabro/workflows/code-review/scripts/code_review.py publish-pr --post-pr '{{ inputs.post_pr }}' --pr-repo '{{ inputs.pr_repo }}' --pr-number '{{ inputs.pr_number }}' --route-severity-below '{{ inputs.route_severity_below }}' --route-categories '{{ inputs.route_categories }}' --run-url '{{ inputs.run_url }}'"
|
||||
script="python3 .fabro/workflows/code-review/scripts/code_review.py publish-pr --post-pr '{{ inputs.post_pr }}' --pr-repo '{{ inputs.pr_repo }}' --pr-number '{{ inputs.pr_number }}' --route-severity-below '{{ inputs.route_severity_below }}' --route-categories '{{ inputs.route_categories }}' --run-url '{{ inputs.run_url }}' --bot-login '{{ inputs.bot_login }}'"
|
||||
]
|
||||
|
||||
start -> prepare
|
||||
|
|
|
|||
|
|
@ -27,7 +27,10 @@ contract, or review scope.
|
|||
`siblings` lists other candidates in the same file (id, line, category,
|
||||
short summary). Judge the claim on its own. If it describes the same defect
|
||||
as a sibling -- one root cause, not merely nearby lines -- also return
|
||||
`duplicate_of` with that sibling's id.
|
||||
`duplicate_of` with that sibling's id. A sibling whose id starts with `H`
|
||||
is a historical sibling: a finding already reported on this pull request
|
||||
by an earlier review; if the claim describes that same defect, return
|
||||
`duplicate_of` with its id the same way.
|
||||
|
||||
{% include "partials/review-target.md.j2" %}
|
||||
Return exactly one verdict:
|
||||
|
|
|
|||
|
|
@ -109,9 +109,17 @@ CONFIDENCE_RANK = SEVERITY_RANK
|
|||
SAFE_REV_RE = re.compile(r"^[A-Za-z0-9@][A-Za-z0-9._/@{}^~:+-]{0,399}$")
|
||||
REVIEW_ID_RE = re.compile(r"^[A-Za-z0-9][A-Za-z0-9_.:-]{0,127}$")
|
||||
CANDIDATE_ID_RE = re.compile(r"^[FS][1-9][0-9]*$")
|
||||
# A historical sibling: a finding already posted on the reviewed PR by an
|
||||
# earlier run, addressed by its comment ID (P3 item 5). The grammar is
|
||||
# disjoint from same-run candidate IDs so a verdict's duplicate_of target
|
||||
# is never ambiguous.
|
||||
HISTORICAL_ID_RE = re.compile(r"^H[1-9][0-9]*$")
|
||||
# Other candidates in the same file a verifier is shown, nearest first, so it
|
||||
# can mark its claim a duplicate of one that describes the same defect.
|
||||
SIBLING_CAP = 6
|
||||
# Historical siblings append under their own bound so they can never
|
||||
# displace same-run siblings and starve the in-run fold.
|
||||
HISTORICAL_SIBLING_CAP = 4
|
||||
|
||||
# Lines of context kept on each side of a finding's anchor line.
|
||||
CODE_FRAME_CONTEXT = 4
|
||||
|
|
@ -1084,6 +1092,138 @@ def import_rule_loader() -> Any:
|
|||
return rule_loader
|
||||
|
||||
|
||||
def import_publisher() -> Any:
|
||||
"""The publisher module (stdlib-only), for its shared P3 contracts."""
|
||||
import publish_pr
|
||||
|
||||
return publish_pr
|
||||
|
||||
|
||||
# --- Incremental re-review (P3) ----------------------------------------------
|
||||
|
||||
|
||||
HISTORY_FILE_NAME = "pr-history.json"
|
||||
|
||||
|
||||
def run_publisher(
|
||||
publisher_path: Path,
|
||||
arguments: List[str],
|
||||
environment: Optional[Dict[str, str]] = None,
|
||||
) -> subprocess.CompletedProcess:
|
||||
return subprocess.run(
|
||||
[sys.executable, str(publisher_path), *arguments],
|
||||
cwd=root(),
|
||||
env=environment,
|
||||
capture_output=True,
|
||||
)
|
||||
|
||||
|
||||
def prepare_incremental(args: argparse.Namespace, mode: str) -> Dict[str, Any]:
|
||||
"""Fetch per-PR history and resolve the incremental posture (items 1-2).
|
||||
|
||||
Runs before range resolution: the delta needs the last reviewed head.
|
||||
The fetch itself is `publish_pr.py history` -- pinned code holding the
|
||||
token, the P3 sibling of apply. A fetch failure degrades open (full
|
||||
review, no suppression, declared downstream; decided Q2), but a
|
||||
snapshot that disagrees with the local HEAD fails the run: it was
|
||||
taken around a racing push and every decision built on it would be
|
||||
wrong from the outset.
|
||||
"""
|
||||
publisher = import_publisher()
|
||||
state: Dict[str, Any] = {
|
||||
"requested": True,
|
||||
"enabled": False,
|
||||
"from": None,
|
||||
"commits": None,
|
||||
"history_unavailable": None,
|
||||
"history_comments": [],
|
||||
}
|
||||
if mode != "changes":
|
||||
raise WorkflowDataError(
|
||||
"incremental review requires mode=changes (a PR review)"
|
||||
)
|
||||
if not inside_git_worktree():
|
||||
raise WorkflowDataError("incremental review requires a Git worktree")
|
||||
repo = args.pr_repo.strip()
|
||||
pr = args.pr_number.strip()
|
||||
if not repo or not pr:
|
||||
raise WorkflowDataError(
|
||||
"incremental is enabled but pr_repo/pr_number do not name the "
|
||||
"pull request whose history to read"
|
||||
)
|
||||
try:
|
||||
# The parsed value rides in the state record so the publish node
|
||||
# reuses this validated decision instead of re-reading the input.
|
||||
state["overlap_threshold"] = publisher.parse_overlap_threshold(
|
||||
args.incremental_overlap_threshold
|
||||
)
|
||||
except publisher.PublishError as error:
|
||||
raise WorkflowDataError(str(error)) from error
|
||||
|
||||
publisher_path = resolve_workflow_script(PUBLISHER_PATH, "PR publisher")
|
||||
history_path = CONTROL_DIR / HISTORY_FILE_NAME
|
||||
fetched = run_publisher(
|
||||
publisher_path,
|
||||
[
|
||||
"history",
|
||||
"--repo", repo,
|
||||
"--pr", pr,
|
||||
"--api-base", args.api_base,
|
||||
"--bot-login", args.bot_login,
|
||||
"--output", str(history_path),
|
||||
],
|
||||
)
|
||||
if fetched.returncode != 0:
|
||||
reason = one_line(
|
||||
fetched.stderr.decode("utf-8", "replace").strip()
|
||||
or "the history fetch failed",
|
||||
500,
|
||||
)
|
||||
state["history_unavailable"] = reason
|
||||
print(f"Incremental history unavailable, degrading open: {reason}")
|
||||
return state
|
||||
stdout_text = fetched.stdout.decode("utf-8", "replace").strip()
|
||||
if stdout_text:
|
||||
print(stdout_text)
|
||||
|
||||
try:
|
||||
history = publisher.validate_history_document(
|
||||
read_json(history_path)
|
||||
)
|
||||
except publisher.PublishError as error:
|
||||
raise WorkflowDataError(f"the fetched history is invalid: {error}")
|
||||
live_head = history["live_head"]
|
||||
local_head = resolve_commit("HEAD", "HEAD")
|
||||
if live_head != local_head:
|
||||
raise WorkflowDataError(
|
||||
f"the PR head moved during preparation: the history snapshot "
|
||||
f"describes {live_head[:12]} but the checkout is at "
|
||||
f"{local_head[:12]}; re-run the review"
|
||||
)
|
||||
state["enabled"] = True
|
||||
state["history_comments"] = list(history.get("comments") or [])
|
||||
|
||||
last_head = (history.get("summary") or {}).get("head")
|
||||
if isinstance(last_head, str):
|
||||
ancestry = git("merge-base", "--is-ancestor", last_head, "HEAD")
|
||||
if ancestry.returncode == 0:
|
||||
# Delta review (item 2): two-dot, literal base. A same-head
|
||||
# re-run resolves to an empty range and exits quietly (Q3).
|
||||
state["from"] = last_head
|
||||
commits = git_text("rev-list", "--count", f"{last_head}..HEAD")
|
||||
state["commits"] = (
|
||||
int(commits) if commits and commits.isdigit() else None
|
||||
)
|
||||
else:
|
||||
# A force-push or rebase: cold start for the range, but the
|
||||
# history still suppresses where mapping succeeded (item 1).
|
||||
print(
|
||||
f"Last reviewed head {last_head[:12]} is not an ancestor "
|
||||
"of HEAD (force-push?); reviewing the full range"
|
||||
)
|
||||
return state
|
||||
|
||||
|
||||
def repo_rule_revision(state: Mapping[str, Any]) -> Optional[str]:
|
||||
"""The revision repository rules are read from.
|
||||
|
||||
|
|
@ -1528,6 +1668,29 @@ def prepare(args: argparse.Namespace) -> None:
|
|||
base_input = args.base.strip()
|
||||
commit_input = args.commit.strip()
|
||||
range_input = args.range.strip()
|
||||
publisher = import_publisher()
|
||||
try:
|
||||
incremental_requested = publisher.parse_incremental_flag(
|
||||
args.incremental
|
||||
)
|
||||
except publisher.PublishError as error:
|
||||
raise WorkflowDataError(str(error)) from error
|
||||
incremental_state: Dict[str, Any] = {"requested": False}
|
||||
if incremental_requested:
|
||||
if range_input or commit_input:
|
||||
raise WorkflowDataError(
|
||||
"incremental review resolves its own range; leave commit "
|
||||
"and range empty (base is allowed as the cold-start "
|
||||
"fallback)"
|
||||
)
|
||||
incremental_state = prepare_incremental(args, mode)
|
||||
if incremental_state.get("from"):
|
||||
# The delta wins; base is only the cold-start fallback. A
|
||||
# sandbox clone's default base resolves to the PR branch's
|
||||
# own upstream (an empty range), so PR runs pass the base
|
||||
# branch explicitly for the first, untagged review.
|
||||
range_input = f"{incremental_state['from']}..HEAD"
|
||||
base_input = ""
|
||||
model = one_line(args.model, 120)
|
||||
# Guidance reaches the finder and sweep prompts through their MiniJinja
|
||||
# templates; the engine only records it so the report says what steering
|
||||
|
|
@ -1668,6 +1831,14 @@ def prepare(args: argparse.Namespace) -> None:
|
|||
"scope_files": scope_file_count,
|
||||
"empty_diff": empty_diff,
|
||||
"empty_scope": empty_scope,
|
||||
"incremental": {
|
||||
key: value
|
||||
for key, value in incremental_state.items()
|
||||
if key != "history_comments"
|
||||
},
|
||||
"history_comments": list(
|
||||
incremental_state.get("history_comments") or []
|
||||
),
|
||||
"use_verify": bool(cell["verify"]),
|
||||
"verify_bias": cell["bias"],
|
||||
"use_sweep": bool(cell["sweep"]),
|
||||
|
|
@ -1703,6 +1874,16 @@ def prepare(args: argparse.Namespace) -> None:
|
|||
evidence_dir.mkdir()
|
||||
metadata_dir.mkdir()
|
||||
(products_dir / ".gitignore").write_text("*\n", encoding="utf-8")
|
||||
if incremental_state.get("enabled"):
|
||||
# Ship the history snapshot as a run artifact: every skip stays
|
||||
# replayable from the products directory.
|
||||
history_source = CONTROL_DIR / HISTORY_FILE_NAME
|
||||
(products_dir / HISTORY_FILE_NAME).write_bytes(
|
||||
history_source.read_bytes()
|
||||
)
|
||||
state["incremental"]["history_rel"] = (
|
||||
f"{products_rel}/{HISTORY_FILE_NAME}"
|
||||
)
|
||||
state["products_dir"] = products_dir.as_posix()
|
||||
state["products_rel"] = products_rel
|
||||
state["evidence_dir"] = evidence_dir.as_posix()
|
||||
|
|
@ -2087,10 +2268,14 @@ def normalize_verdict(value: Any) -> Optional[Dict[str, Any]]:
|
|||
"reasoning": clean_text(value.get("reasoning"), 4000),
|
||||
}
|
||||
duplicate_of = value.get("duplicate_of")
|
||||
if isinstance(duplicate_of, str) and CANDIDATE_ID_RE.fullmatch(
|
||||
duplicate_of.strip()
|
||||
):
|
||||
result["duplicate_of"] = duplicate_of.strip()
|
||||
if isinstance(duplicate_of, str):
|
||||
token = duplicate_of.strip()
|
||||
# Same-run candidates (F/S) and historical siblings (H, P3 item
|
||||
# 5) are both addressable; anything else is silently dropped.
|
||||
if CANDIDATE_ID_RE.fullmatch(token) or HISTORICAL_ID_RE.fullmatch(
|
||||
token
|
||||
):
|
||||
result["duplicate_of"] = token
|
||||
suggestion_valid = value.get("suggestion_valid")
|
||||
if isinstance(suggestion_valid, bool):
|
||||
result["suggestion_valid"] = suggestion_valid
|
||||
|
|
@ -2433,6 +2618,46 @@ def sibling_claims(
|
|||
]
|
||||
|
||||
|
||||
def historical_sibling_claims(
|
||||
candidate: Mapping[str, Any],
|
||||
state: Optional[Mapping[str, Any]],
|
||||
) -> List[Dict[str, Any]]:
|
||||
"""Previously posted findings on this PR, as extra verifier siblings.
|
||||
|
||||
Same-file only, nearest by live-mapped line first, under their own
|
||||
bound (HISTORICAL_SIBLING_CAP) so they never displace same-run
|
||||
siblings. A comment without a mapped span or a parseable meta tag
|
||||
(a pre-P3 comment) is skipped.
|
||||
"""
|
||||
comments = (state or {}).get("history_comments") or []
|
||||
line = int(candidate.get("line") or 0)
|
||||
entries: List[Dict[str, Any]] = []
|
||||
for comment in comments:
|
||||
if not isinstance(comment, dict):
|
||||
continue
|
||||
span = comment.get("mapped_span")
|
||||
meta = comment.get("meta")
|
||||
if not isinstance(span, dict) or not isinstance(meta, dict):
|
||||
continue
|
||||
if span.get("path") != candidate.get("file"):
|
||||
continue
|
||||
entries.append(
|
||||
{
|
||||
"id": f"H{comment.get('id')}",
|
||||
"line": span.get("end_line"),
|
||||
"category": meta.get("category"),
|
||||
"short_summary": one_line(meta.get("short_summary"), 200),
|
||||
}
|
||||
)
|
||||
entries.sort(
|
||||
key=lambda entry: (
|
||||
abs(int(entry.get("line") or 0) - line),
|
||||
str(entry.get("id")),
|
||||
)
|
||||
)
|
||||
return entries[:HISTORICAL_SIBLING_CAP]
|
||||
|
||||
|
||||
def verification_claim(
|
||||
candidate: Mapping[str, Any],
|
||||
state: Optional[Mapping[str, Any]] = None,
|
||||
|
|
@ -2462,7 +2687,8 @@ def verification_claim(
|
|||
"summary": candidate.get("summary"),
|
||||
"failure_scenario": candidate.get("failure_scenario"),
|
||||
"reports": int(candidate.get("reports") or 1),
|
||||
"siblings": sibling_claims(candidate, pool or []),
|
||||
"siblings": sibling_claims(candidate, pool or [])
|
||||
+ historical_sibling_claims(candidate, state),
|
||||
}
|
||||
suggestion_code = str(candidate.get("suggestion_code") or "")
|
||||
if suggestion_code and location["existing_code"]:
|
||||
|
|
@ -2999,6 +3225,8 @@ def reportable_finding(
|
|||
candidate["file"], start_line, end_line, state
|
||||
),
|
||||
}
|
||||
if isinstance(candidate.get("duplicate_of_posted"), dict):
|
||||
finding["duplicate_of_posted"] = dict(candidate["duplicate_of_posted"])
|
||||
suggestion_code = str(candidate.get("suggestion_code") or "")
|
||||
if (
|
||||
suggestion_code
|
||||
|
|
@ -3228,12 +3456,35 @@ def fold_duplicates(
|
|||
candidate_id = folded[candidate_id]
|
||||
return candidate_id
|
||||
|
||||
history_by_hid = {
|
||||
f"H{comment.get('id')}": comment
|
||||
for comment in state.get("history_comments") or []
|
||||
if isinstance(comment, dict)
|
||||
}
|
||||
|
||||
for record in ordered:
|
||||
candidate_id = str(record["candidate"].get("id"))
|
||||
target = (record.get("verdict") or {}).get("duplicate_of")
|
||||
if not target:
|
||||
continue
|
||||
if HISTORICAL_ID_RE.fullmatch(str(target)):
|
||||
# A historical target (P3 item 5): the posted finding is not
|
||||
# in kept_records, so the shown-sibling check stays but the
|
||||
# survivor check does not apply. The candidate is not folded
|
||||
# away -- it is marked, kept in the bundle, and the publisher
|
||||
# places it as skipped (reason duplicate-of-posted).
|
||||
comment = history_by_hid.get(str(target))
|
||||
if (
|
||||
target in allowed.get(candidate_id, set())
|
||||
and comment is not None
|
||||
):
|
||||
record["candidate"]["duplicate_of_posted"] = {
|
||||
"comment_id": int(comment["id"]),
|
||||
"html_url": str(comment.get("html_url") or ""),
|
||||
}
|
||||
continue
|
||||
if (
|
||||
not target
|
||||
or target == candidate_id
|
||||
target == candidate_id
|
||||
or target not in allowed.get(candidate_id, set())
|
||||
or target not in by_id
|
||||
or rank_index[target] > rank_index[candidate_id]
|
||||
|
|
@ -3350,6 +3601,10 @@ def final_tally() -> None:
|
|||
entry["verdict"] = verdict["verdict"]
|
||||
if disposition == "duplicate":
|
||||
entry["duplicate_of"] = folded.get(str(candidate.get("id")))
|
||||
if isinstance(candidate.get("duplicate_of_posted"), dict):
|
||||
entry["duplicate_of_posted"] = dict(
|
||||
candidate["duplicate_of_posted"]
|
||||
)
|
||||
if candidate.get("anchors"):
|
||||
entry["anchors"] = list(candidate["anchors"])
|
||||
return entry
|
||||
|
|
@ -3363,6 +3618,8 @@ def final_tally() -> None:
|
|||
if str(candidate.get("id")) in folded:
|
||||
return "duplicate"
|
||||
if candidate_key(candidate) in reported_keys and record["kept"]:
|
||||
if isinstance(candidate.get("duplicate_of_posted"), dict):
|
||||
return "duplicate-of-posted"
|
||||
return "reportable"
|
||||
if record["kept"]:
|
||||
return "deferred-by-cap"
|
||||
|
|
@ -3532,6 +3789,21 @@ def final_tally() -> None:
|
|||
"verification": {"status": verification_status},
|
||||
"canonical_files": list(CANONICAL_FILES),
|
||||
}
|
||||
incremental_state = (
|
||||
state.get("incremental")
|
||||
if isinstance(state.get("incremental"), dict)
|
||||
else {}
|
||||
)
|
||||
if incremental_state.get("requested"):
|
||||
manifest["incremental"] = {
|
||||
"requested": True,
|
||||
"enabled": bool(incremental_state.get("enabled")),
|
||||
"from": incremental_state.get("from"),
|
||||
"commits": incremental_state.get("commits"),
|
||||
"history_unavailable": incremental_state.get(
|
||||
"history_unavailable"
|
||||
),
|
||||
}
|
||||
if rule_mapped:
|
||||
rules_state = (
|
||||
state.get("rules") if isinstance(state.get("rules"), dict) else {}
|
||||
|
|
@ -3680,17 +3952,6 @@ def publish_pr_command(args: argparse.Namespace) -> None:
|
|||
plan_rel = f"{products_rel}/pr-publish-plan.json"
|
||||
outcome_rel = f"{products_rel}/pr-publish-outcome.json"
|
||||
|
||||
def run_publisher(
|
||||
arguments: List[str],
|
||||
environment: Optional[Dict[str, str]] = None,
|
||||
) -> subprocess.CompletedProcess:
|
||||
return subprocess.run(
|
||||
[sys.executable, str(publisher), *arguments],
|
||||
cwd=root(),
|
||||
env=environment,
|
||||
capture_output=True,
|
||||
)
|
||||
|
||||
def publisher_error(
|
||||
prefix: str, failed: subprocess.CompletedProcess
|
||||
) -> WorkflowDataError:
|
||||
|
|
@ -3707,32 +3968,50 @@ def publish_pr_command(args: argparse.Namespace) -> None:
|
|||
if key in ("PATH", "HOME", "TZ", "USER", "LOGNAME", "SHELL")
|
||||
or key.startswith(("LANG", "LC_", "PYTHON", "TMP", "TEMP"))
|
||||
}
|
||||
result = run_publisher(
|
||||
[
|
||||
"plan",
|
||||
"--evidence-dir", evidence_rel,
|
||||
"--repo", repo,
|
||||
"--pr", pr_text,
|
||||
"--route-severity-below", args.route_severity_below,
|
||||
"--route-categories", args.route_categories,
|
||||
"--run-url", one_line(args.run_url, 2000),
|
||||
"--output", plan_rel,
|
||||
],
|
||||
plan_environment,
|
||||
plan_arguments = [
|
||||
"plan",
|
||||
"--evidence-dir", evidence_rel,
|
||||
"--repo", repo,
|
||||
"--pr", pr_text,
|
||||
"--route-severity-below", args.route_severity_below,
|
||||
"--route-categories", args.route_categories,
|
||||
"--run-url", one_line(args.run_url, 2000),
|
||||
"--output", plan_rel,
|
||||
]
|
||||
incremental_state = (
|
||||
state.get("incremental")
|
||||
if isinstance(state.get("incremental"), dict)
|
||||
else {}
|
||||
)
|
||||
if incremental_state.get("requested"):
|
||||
# Prepare parsed and recorded the threshold; the node reuses that
|
||||
# decision rather than re-reading the raw workflow input.
|
||||
plan_arguments.extend(
|
||||
[
|
||||
"--incremental", "true",
|
||||
"--incremental-overlap-threshold",
|
||||
str(incremental_state.get("overlap_threshold") or ""),
|
||||
]
|
||||
)
|
||||
history_rel = incremental_state.get("history_rel")
|
||||
if incremental_state.get("enabled") and isinstance(history_rel, str):
|
||||
plan_arguments.extend(["--history", history_rel])
|
||||
result = run_publisher(publisher, plan_arguments, plan_environment)
|
||||
if result.returncode != 0:
|
||||
raise publisher_error("the publication plan failed: ", result)
|
||||
print(result.stdout.decode("utf-8", "replace").strip())
|
||||
|
||||
result = run_publisher(
|
||||
publisher,
|
||||
[
|
||||
"apply",
|
||||
"--plan", plan_rel,
|
||||
"--repo", repo,
|
||||
"--pr", pr_text,
|
||||
"--api-base", args.api_base,
|
||||
"--bot-login", args.bot_login,
|
||||
"--outcome", outcome_rel,
|
||||
]
|
||||
],
|
||||
)
|
||||
value = read_json(root() / outcome_rel, required=False)
|
||||
outcome: Dict[str, Any] = value if isinstance(value, dict) else {}
|
||||
|
|
@ -3902,6 +4181,12 @@ def build_parser() -> argparse.ArgumentParser:
|
|||
prepare_parser.add_argument("--range", default="")
|
||||
prepare_parser.add_argument("--model", default="")
|
||||
prepare_parser.add_argument("--guidance", default="")
|
||||
prepare_parser.add_argument("--incremental", default="")
|
||||
prepare_parser.add_argument("--incremental-overlap-threshold", default="")
|
||||
prepare_parser.add_argument("--pr-repo", default="")
|
||||
prepare_parser.add_argument("--pr-number", default="")
|
||||
prepare_parser.add_argument("--bot-login", default="")
|
||||
prepare_parser.add_argument("--api-base", default="https://api.github.com")
|
||||
prepare_parser.add_argument("--review-id-stdin", action="store_true")
|
||||
|
||||
merge_parser = subparsers.add_parser("merge")
|
||||
|
|
@ -3917,6 +4202,7 @@ def build_parser() -> argparse.ArgumentParser:
|
|||
publish_parser.add_argument("--route-severity-below", default="")
|
||||
publish_parser.add_argument("--route-categories", default="")
|
||||
publish_parser.add_argument("--run-url", default="")
|
||||
publish_parser.add_argument("--bot-login", default="")
|
||||
publish_parser.add_argument("--api-base", default="https://api.github.com")
|
||||
|
||||
expectations_parser = subparsers.add_parser("verify-expectations")
|
||||
|
|
|
|||
File diff suppressed because it is too large
Load diff
|
|
@ -48,6 +48,10 @@ DISPOSITIONS = (
|
|||
"verification-incomplete",
|
||||
"deferred-by-cap",
|
||||
"duplicate",
|
||||
# P3 item 5: kept in the bundle, but the verifier judged it the same
|
||||
# defect as a finding already posted on the reviewed PR; the
|
||||
# publisher places it as skipped instead of re-posting.
|
||||
"duplicate-of-posted",
|
||||
)
|
||||
VERIFICATION_STATUSES = ("complete", "partial", "skipped-low-effort")
|
||||
COMPLETION_STATUSES = ("complete", "partial")
|
||||
|
|
@ -178,6 +182,15 @@ def validate_manifest(value: object) -> Dict[str, Any]:
|
|||
verification = as_map(manifest.get("verification"))
|
||||
if verification.get("status") not in VERIFICATION_STATUSES:
|
||||
die("manifest verification.status is invalid")
|
||||
incremental = manifest.get("incremental")
|
||||
if incremental is not None:
|
||||
incremental = as_map(incremental)
|
||||
delta_from = incremental.get("from")
|
||||
if delta_from is not None and (
|
||||
not isinstance(delta_from, str)
|
||||
or not re.fullmatch(r"[0-9a-f]{40}", delta_from)
|
||||
):
|
||||
die("manifest incremental.from must be a commit SHA or null")
|
||||
rules = manifest.get("rules")
|
||||
if rules is not None:
|
||||
rules = as_map(rules)
|
||||
|
|
@ -359,6 +372,19 @@ def validate_finding(value: object, index: int) -> Dict[str, Any]:
|
|||
}
|
||||
if normalized_suggestion is not None:
|
||||
normalized["suggestion"] = normalized_suggestion
|
||||
duplicate_of_posted = finding.get("duplicate_of_posted")
|
||||
if duplicate_of_posted is not None:
|
||||
record = as_map(duplicate_of_posted)
|
||||
normalized["duplicate_of_posted"] = {
|
||||
"comment_id": positive_int(
|
||||
record.get("comment_id"),
|
||||
f"{field}.duplicate_of_posted.comment_id",
|
||||
),
|
||||
"html_url": safe_text(
|
||||
record.get("html_url", ""),
|
||||
f"{field}.duplicate_of_posted.html_url",
|
||||
),
|
||||
}
|
||||
return normalized
|
||||
|
||||
|
||||
|
|
@ -503,8 +529,13 @@ def validate_relationships(
|
|||
counts = manifest["counts"]
|
||||
if counts["reported"] != len(findings):
|
||||
die("manifest counts.reported does not match findings.json")
|
||||
# A duplicate-of-posted finding stays in findings.json (the bundle
|
||||
# never drops a finding); its ledger disposition names the earlier
|
||||
# comment it defers to.
|
||||
reportable = [
|
||||
record for record in ledger if record["disposition"] == "reportable"
|
||||
record
|
||||
for record in ledger
|
||||
if record["disposition"] in ("reportable", "duplicate-of-posted")
|
||||
]
|
||||
if len(reportable) != len(findings):
|
||||
die("reportable ledger records do not match findings.json")
|
||||
|
|
@ -720,6 +751,14 @@ def finding_markdown(finding: Mapping[str, Any]) -> List[str]:
|
|||
)
|
||||
+ " -- judged the same defect and folded in."
|
||||
)
|
||||
duplicate_of_posted = finding.get("duplicate_of_posted")
|
||||
if isinstance(duplicate_of_posted, dict):
|
||||
url = str(duplicate_of_posted.get("html_url") or "").strip()
|
||||
lines.append(
|
||||
"Already reported on the pull request"
|
||||
+ (f": {url}" if url else "")
|
||||
+ " -- not re-posted."
|
||||
)
|
||||
if finding["summary"].strip() != finding["short_summary"].strip():
|
||||
lines.extend(["", escape_markdown(finding["summary"])])
|
||||
lines.extend(
|
||||
|
|
@ -958,6 +997,8 @@ def jsonl_line(finding: Mapping[str, Any]) -> str:
|
|||
}
|
||||
if finding.get("suggestion") is not None:
|
||||
record["suggestion"] = finding["suggestion"]
|
||||
if finding.get("duplicate_of_posted") is not None:
|
||||
record["duplicate_of_posted"] = finding["duplicate_of_posted"]
|
||||
return json.dumps(record, ensure_ascii=False, separators=(",", ":"))
|
||||
|
||||
|
||||
|
|
@ -1116,6 +1157,10 @@ def sarif_result(
|
|||
)
|
||||
},
|
||||
}
|
||||
if finding.get("duplicate_of_posted") is not None:
|
||||
result["properties"]["duplicate_of_posted"] = finding[
|
||||
"duplicate_of_posted"
|
||||
]
|
||||
suggestion = finding.get("suggestion")
|
||||
if isinstance(suggestion, dict):
|
||||
result["fixes"] = [
|
||||
|
|
|
|||
|
|
@ -14,18 +14,27 @@ The canonical bundle is schema version 4.
|
|||
tiers (every tier above `low`) it also carries a `rules` block: the
|
||||
compiled rule layers, the rule configuration SHA-256, the built-in rule
|
||||
manifest SHA-256, the repository rule revision, and pack/check counts for
|
||||
both layers.
|
||||
both layers. An incremental PR re-review adds an `incremental` record:
|
||||
whether it was requested and enabled, the last reviewed head the delta
|
||||
started from (`from`, with the commit count), and any declared
|
||||
history-fetch degradation.
|
||||
- `candidate-ledger.jsonl` contains every unique candidate after
|
||||
deduplication, plus every sweep candidate. Each record has one disposition:
|
||||
`reportable`, `refuted`, `verification-incomplete`, `deferred-by-cap`, or
|
||||
`duplicate` (folded into the finding named by `duplicate_of`), and
|
||||
carries the candidate's applicable `rule_ids` (empty outside the
|
||||
rule-mapped tiers).
|
||||
`reportable`, `refuted`, `verification-incomplete`, `deferred-by-cap`,
|
||||
`duplicate` (folded into the finding named by `duplicate_of`), or
|
||||
`duplicate-of-posted` (judged the same defect as a finding already
|
||||
posted on the reviewed PR; `duplicate_of_posted` names the covering
|
||||
comment), and carries the candidate's applicable `rule_ids` (empty
|
||||
outside the rule-mapped tiers).
|
||||
- `findings.json` contains only the reportable subset. It is the authoritative
|
||||
finding list. Each reported finding carries its orthogonal `issue_type`, an
|
||||
engine-derived `location` with the exact original code and start/end lines,
|
||||
a highlighted `code` excerpt, and its `rule_ids`. A verified replacement is
|
||||
stored as optional `suggestion.replacement_code`.
|
||||
stored as optional `suggestion.replacement_code`. A finding the verifier
|
||||
judged already posted on the reviewed PR stays in the list (the bundle
|
||||
never drops a finding) with an optional `duplicate_of_posted` record
|
||||
naming the covering comment; the PR publisher skips it instead of
|
||||
re-posting.
|
||||
- `coverage.json` records what the review dispatched, what returned, what was
|
||||
rejected for failing the finding contract, and what a cap dropped. At the
|
||||
rule-mapped tiers it also records the authoritative target-file list, the
|
||||
|
|
|
|||
|
|
@ -29,6 +29,16 @@ route_severity_below = ""
|
|||
route_categories = ""
|
||||
# Optional run-report URL included in the sticky summary.
|
||||
run_url = ""
|
||||
# Incremental re-review (P3): incremental = "true" makes prepare fetch the
|
||||
# PR's prior-review history, review only the commits since the last
|
||||
# reviewed head, and skip findings already covered by an earlier comment.
|
||||
# incremental_overlap_threshold is the span IoU bound, strictly inside
|
||||
# (0, 1); empty means 0.6. bot_login names the login whose comments count
|
||||
# as ours (required in practice on GitHub App servers, where GET /user is
|
||||
# unavailable to installation tokens; e.g. "fabro-sh-fabro[bot]").
|
||||
incremental = ""
|
||||
incremental_overlap_threshold = ""
|
||||
bot_login = ""
|
||||
|
||||
# Full history, for arbitrary base and range inputs.
|
||||
[run.clone]
|
||||
|
|
@ -86,6 +96,7 @@ include = [
|
|||
"CODE-REVIEW-*/metadata/review-meta.json",
|
||||
"CODE-REVIEW-*/pr-publish-plan.json",
|
||||
"CODE-REVIEW-*/pr-publish-outcome.json",
|
||||
"CODE-REVIEW-*/pr-history.json",
|
||||
]
|
||||
|
||||
[environments.code-review]
|
||||
|
|
|
|||
35
tools/code-review-canary/canary.py
Normal file
35
tools/code-review-canary/canary.py
Normal file
|
|
@ -0,0 +1,35 @@
|
|||
"""Deliberately flawed fixture for the P3 incremental live acceptance.
|
||||
|
||||
Planted correctness bugs, so a reviewed push with post_pr enabled is
|
||||
guaranteed inline-postable findings:
|
||||
|
||||
- ``percentile`` indexes past the end of the list when fraction is 1.0.
|
||||
- ``moving_average`` divides every window by the full window size, so
|
||||
the tail averages are too small.
|
||||
|
||||
This file exists only on the calibration draft PR and is never merged.
|
||||
"""
|
||||
|
||||
# Calibration cycle 3: these two lines shift every function down.
|
||||
# They exist to outdate and re-anchor the earlier review comments.
|
||||
|
||||
def percentile(values, fraction):
|
||||
"""Return the value at the given fraction of the sorted input."""
|
||||
ordered = sorted(values)
|
||||
index = int(len(ordered) * fraction)
|
||||
return ordered[index] # still off the end at fraction == 1.0
|
||||
|
||||
|
||||
def moving_average(values, window):
|
||||
"""Average each window of the input, including the shorter tail."""
|
||||
averages = []
|
||||
for start in range(len(values)):
|
||||
chunk = values[start:start + window]
|
||||
averages.append(sum(chunk) / window)
|
||||
return averages
|
||||
|
||||
|
||||
def collect_values(values, collected=[]):
|
||||
"""Collect values for one independent operation."""
|
||||
collected.extend(values)
|
||||
return collected
|
||||
Loading…
Add table
Reference in a new issue