This is an automated email from the ASF dual-hosted git repository. shuke987 pushed a commit to branch codex/review-receipt-hardening in repository https://gitbox.apache.org/repos/asf/doris-skills.git
commit 6fb7266dd0cc3e9a964e4b9ad33c58d0341e023d Author: shuke <[email protected]> AuthorDate: Wed Aug 19 19:34:23 2026 +0800 fix: verify local review results before posting --- README.md | 4 +- skills/doris-repo-review/SKILL.md | 26 +-- .../references/pr-comment-format.md | 8 +- .../doris-repo-review/scripts/post-pass-comment.sh | 71 +++++-- .../scripts/record-review-runtime.sh | 6 +- skills/doris-repo-review/scripts/verify-anchors.py | 194 ++++++++++++++++-- verify/repo-review/run.sh | 225 +++++++++++++++++---- 7 files changed, 446 insertions(+), 88 deletions(-) diff --git a/README.md b/README.md index 3224c21..353d599 100644 --- a/README.md +++ b/README.md @@ -111,8 +111,8 @@ deterministic review-receipt contract suite: - **L3 — behavior**: the skills *behave* as written — evidence-first and safety guardrails, an end-to-end advisor→DDL→live-cluster loopback, and skill-router triggering — exercised through a nested `claude -p`. -- **Review receipt**: a mocked-`gh` suite checks the qualified-runtime gate, review-run and commit - binding, automatic create/update behavior, and core refusal paths without writing to GitHub. +- **Review receipt**: a mocked-`gh` suite checks qualified-runtime binding, exact commit matching, + automatic create/update behavior, and every important refusal path without writing to GitHub. A skill that makes no DDL or doriscli claim (`doris-debug`, `doris-profile-reader`, `doris-repo-review`) has no L1/L2 coverage. `doris-repo-review` instead has a deterministic diff --git a/skills/doris-repo-review/SKILL.md b/skills/doris-repo-review/SKILL.md index 7385396..3b38eaa 100644 --- a/skills/doris-repo-review/SKILL.md +++ b/skills/doris-repo-review/SKILL.md @@ -43,7 +43,7 @@ $S/post-pass-comment.sh --ctx "$CTX" ... # step 10: |---|---| | `scripts/align-to-pr.sh` | Resolve the PR, diagnose how the current directory relates to it, align it to the PR head | | `scripts/prepare-review-context.sh` | Produce the authoritative diff, new-side line ranges, required AGENTS.md list, existing comments, ledger skeleton | -| `scripts/verify-anchors.py` | Check that every `path:line` anchor exists and that both documents expose the same finding IDs | +| `scripts/verify-anchors.py` | Check anchors and EN/ZH agreement, then bind verdict/counts/rounds and document hashes to this review run | | `scripts/record-review-runtime.sh` | Validate and bind the explicitly selected qualified reviewer runtime to the reviewed head | | `scripts/post-pass-comment.sh` | Render and post the machine-readable PASS comment; refuses everything that is not a pass | | `references/prompts.md` | Subagent prompt templates (CI wording, carried over verbatim) | @@ -368,8 +368,10 @@ python3 $S/verify-anchors.py --ctx "$CTX" \ ``` It checks that anchor paths exist, that line numbers are inside the file, that every finding has at -least one anchor, and that the EN and ZH finding-ID sets match; it also flags anchors pointing at -unchanged context lines - which is usually where a miscomputed line number shows up. +least one anchor, and that the EN and ZH finding IDs, severities, verdict, rounds, and convergence +match. On success it atomically writes the run-and-commit-bound `review-result.env` consumed by the +poster. It also flags anchors pointing at unchanged context lines - which is usually where a +miscomputed line number shows up. --- @@ -389,8 +391,6 @@ cat > "$CTX/pr-comment-notes.md" <<'EOF' EOF $S/post-pass-comment.sh --ctx "$CTX" \ - --findings <blocker>,<major>,<minor>,<nit> \ - --rounds <r> --converged true \ --notes-file "$CTX/pr-comment-notes.md" ``` @@ -398,8 +398,10 @@ $S/post-pass-comment.sh --ctx "$CTX" \ environment fallbacks, `unknown`, low efforts, and unapproved models are rejected. `--dry-run` remains available for maintainers testing the script, but it is not part of the normal skill flow. -- Findings counts and rounds must match the accepted findings and convergence result in the two - documents. They are supplied only after `verify-anchors.py` passes. +- Verdict, findings counts, rounds, convergence, document paths, and document hashes come only + from `review-result.env`, which `verify-anchors.py` replaces after successfully checking the + standard EN/ZH documents. Any later document edit invalidates the receipt until verification is + rerun. - The script refuses to post when: any `Blocker`/`Major` is present; the live PR head no longer equals the reviewed commit; the runtime record names another commit; model/effort/source is not eligible; the PR is not open (`--allow-closed` overrides); rounds are outside 1-3; the review did @@ -409,8 +411,8 @@ $S/post-pass-comment.sh --ctx "$CTX" \ the whole skill instead of posting. - An earlier v1 comment from the same account for the **same** commit is edited in place; a new commit gets a new comment, so each push leaves exactly one record. -- The comment is automatic after a qualified, converged pass. `--dry-run` is available only for - maintainer testing and is not a confirmation step in the normal flow. +- Never pass verdict, finding counts, rounds, or convergence to the poster by hand; it accepts only + the verifier's result for the current run and commit. - `references/pr-comment-format.md` holds the schema, the field meanings, and the parser snippet. **Never hand-write or hand-edit this comment** - the format is a contract other programs read. @@ -485,6 +487,6 @@ Tell the user: - **A pass is not a human merge approval.** Repository policy may accept the receipt for its exact commit, but it carries no Apache sign-off. The `<sub>` disclaimer line says exactly that - keep it. -- **Counts drift between the documents and the comment.** `--findings` must be the accepted - findings of step 9, not the candidate count from the ledger; re-count from the written documents - before posting. +- **Counts drift between the documents and the comment.** Never supply or edit counts separately. + The verifier derives them from the accepted findings in both step-9 documents, and the poster + re-verifies those documents immediately before posting. diff --git a/skills/doris-repo-review/references/pr-comment-format.md b/skills/doris-repo-review/references/pr-comment-format.md index 1db6cf3..8a7c6cc 100644 --- a/skills/doris-repo-review/references/pr-comment-format.md +++ b/skills/doris-repo-review/references/pr-comment-format.md @@ -60,9 +60,9 @@ receipt for the matching commit; it is not a human Apache approval.</sub> | `reviewer` | GitHub login | `gh api user`, falling back to GraphQL `viewer` and `gh auth status` | | `model` | exact eligible model id | review-run-and-commit-bound `review-runtime.env`, created before source review | | `effort` | `xhigh`, `max`, or `ultra` | review-run-and-commit-bound `review-runtime.env`; no environment or `unknown` fallback | -| `findings` | inline map | accepted EN/ZH document counts per severity; `blocker` and `major` are 0 in a PASS | -| `rounds` | int | convergence rounds actually run (1-3) | -| `converged` | bool | the producer posts only `true` | +| `findings` | inline map | verified EN/ZH document counts per severity; `blocker` and `major` are 0 in a PASS | +| `rounds` | int | verified EN/ZH convergence rounds (1-3) | +| `converged` | bool | verified EN/ZH result; the producer posts only `true` | Rules the script enforces, so they cannot drift: @@ -74,6 +74,8 @@ Rules the script enforces, so they cannot drift: independently rechecks the model allowlist, `xhigh`-or-higher effort, runtime source, run id, and recorded head. A runtime record cannot be reused by a later review of the same commit. - **The review must have converged in 1-3 rounds.** A non-converged run never posts a PASS receipt. +- **The documents must remain unchanged after verification.** The poster recomputes both SHA-256 + hashes from `review-result.env` before every create or update. - **At most 5 notes**, each a `- ` bullet (2-space indented continuation lines allowed). Longer material belongs in the local review documents. - **Same commit ⇒ update in place.** An earlier v1 comment by the same account carrying the same diff --git a/skills/doris-repo-review/scripts/post-pass-comment.sh b/skills/doris-repo-review/scripts/post-pass-comment.sh index 26624f9..d753934 100755 --- a/skills/doris-repo-review/scripts/post-pass-comment.sh +++ b/skills/doris-repo-review/scripts/post-pass-comment.sh @@ -5,10 +5,7 @@ # post-pass-comment.sh --ctx <dir> [options] # # --ctx <dir> review context directory (must contain meta.env and a -# qualified runtime record) -# --findings b,m,mi,n blocker,major,minor,nit counts (default 0,0,0,0) -# --rounds <n> convergence rounds actually run (default 1) -# --converged true|false (default true) +# qualified runtime record; standard EN/ZH docs must exist) # --notes-file <f> markdown bullet list for "Notes for maintainers" # --dry-run run every precondition, render the body, post nothing # --force-new always create a new comment, never update in place @@ -22,9 +19,6 @@ set -euo pipefail CTX="" -FINDINGS="0,0,0,0" -ROUNDS="1" -CONVERGED="true" NOTES_FILE="" DRY_RUN=0 FORCE_NEW=0 @@ -33,9 +27,6 @@ ALLOW_CLOSED=0 while [ $# -gt 0 ]; do case "$1" in --ctx) CTX="$2"; shift 2 ;; - --findings) FINDINGS="$2"; shift 2 ;; - --rounds) ROUNDS="$2"; shift 2 ;; - --converged) CONVERGED="$2"; shift 2 ;; --notes-file) NOTES_FILE="$2"; shift 2 ;; --dry-run) DRY_RUN=1; shift ;; --force-new) FORCE_NEW=1; shift ;; @@ -58,6 +49,7 @@ RUNTIME_FILE="$CTX/review-runtime.env" echo "ERROR: $RUNTIME_FILE not found - record an explicitly selected qualified reviewer first." >&2 exit 2 } +RESULT_FILE="$CTX/review-result.env" command -v gh >/dev/null 2>&1 || { echo "ERROR: gh CLI is required." >&2; exit 1; } command -v jq >/dev/null 2>&1 || { echo "ERROR: jq is required." >&2; exit 1; } command -v python3 >/dev/null 2>&1 || { echo "ERROR: python3 is required." >&2; exit 1; } @@ -71,6 +63,7 @@ BASE_SHA="$(read_meta BASE_SHA)" HEAD_SHA="$(read_meta HEAD_SHA)" REVIEW_RUN_ID="$(read_meta REVIEW_RUN_ID)" REPO_ROOT="$(read_meta REPO_ROOT)" +DOCS_ROOT="$(read_meta DOCS_ROOT)" MODEL="$(read_value "$RUNTIME_FILE" REVIEW_MODEL)" EFFORT="$(read_value "$RUNTIME_FILE" REVIEW_EFFORT)" RUNTIME_HEAD_SHA="$(read_value "$RUNTIME_FILE" REVIEW_HEAD_SHA)" @@ -108,12 +101,62 @@ if ! is_valid_review_run_id "$REVIEW_RUN_ID" \ exit 2 fi -case "$CONVERGED" in true) ;; *) echo "ERROR: a pipeline-equivalent PASS requires --converged true." >&2; exit 2 ;; esac -[[ "$ROUNDS" =~ ^[0-9]+$ ]] || { echo "ERROR: --rounds takes a number." >&2; exit 2; } -[ "$ROUNDS" -ge 1 ] && [ "$ROUNDS" -le 3 ] || { echo "ERROR: --rounds must be between 1 and 3." >&2; exit 2; } +# Re-run document verification immediately before rendering. This overwrites any stale or edited +# result file, so verdict/counts/rounds cannot be supplied by hand. +[ -n "$DOCS_ROOT" ] || DOCS_ROOT="$REPO_ROOT" +EXPECTED_EN_DOC="$DOCS_ROOT/review-docs/pr-${PR_NUMBER}-review.en.md" +EXPECTED_ZH_DOC="$DOCS_ROOT/review-docs/pr-${PR_NUMBER}-review.zh.md" +EXPECTED_EN_DOC="$(python3 -c 'import pathlib, sys; print(pathlib.Path(sys.argv[1]).resolve())' "$EXPECTED_EN_DOC")" +EXPECTED_ZH_DOC="$(python3 -c 'import pathlib, sys; print(pathlib.Path(sys.argv[1]).resolve())' "$EXPECTED_ZH_DOC")" +python3 "$SCRIPT_DIR/verify-anchors.py" --ctx "$CTX" \ + --doc "$EXPECTED_EN_DOC" --doc "$EXPECTED_ZH_DOC" || { + echo "ERROR: review documents did not pass final verification. Nothing was posted." >&2 + exit 2 +} +[ -f "$RESULT_FILE" ] || { echo "ERROR: verifier did not produce $RESULT_FILE." >&2; exit 2; } + +RESULT_RUN_ID="$(read_value "$RESULT_FILE" REVIEW_RUN_ID)" +RESULT_HEAD_SHA="$(read_value "$RESULT_FILE" REVIEW_HEAD_SHA)" +VERDICT="$(read_value "$RESULT_FILE" VERDICT)" +FINDINGS="$(read_value "$RESULT_FILE" FINDINGS)" +ROUNDS="$(read_value "$RESULT_FILE" ROUNDS)" +CONVERGED="$(read_value "$RESULT_FILE" CONVERGED)" +EN_DOC="$(read_value "$RESULT_FILE" EN_DOC)" +EN_DOC_SHA256="$(read_value "$RESULT_FILE" EN_DOC_SHA256)" +ZH_DOC="$(read_value "$RESULT_FILE" ZH_DOC)" +ZH_DOC_SHA256="$(read_value "$RESULT_FILE" ZH_DOC_SHA256)" + +[ "$RESULT_RUN_ID" = "$REVIEW_RUN_ID" ] && [ "$RESULT_HEAD_SHA" = "$HEAD_SHA" ] || { + echo "ERROR: verified review result belongs to a different run or commit." >&2 + exit 2 +} +[ "$EN_DOC" = "$EXPECTED_EN_DOC" ] && [ "$ZH_DOC" = "$EXPECTED_ZH_DOC" ] || { + echo "ERROR: verifier result names unexpected review documents." >&2 + exit 2 +} +[ "$VERDICT" = "APPROVE" ] || { + echo "ERROR: verified review verdict is '$VERDICT', not APPROVE. Nothing was posted." >&2 + exit 1 +} + +document_sha256() { + python3 -c 'import hashlib, pathlib, sys; print(hashlib.sha256(pathlib.Path(sys.argv[1]).read_bytes()).hexdigest())' "$1" +} +for doc in "$EN_DOC" "$ZH_DOC"; do + [ -f "$doc" ] || { echo "ERROR: verified review document no longer exists: $doc" >&2; exit 2; } +done +[ "$(document_sha256 "$EN_DOC")" = "$EN_DOC_SHA256" ] \ + && [ "$(document_sha256 "$ZH_DOC")" = "$ZH_DOC_SHA256" ] || { + echo "ERROR: a review document changed after anchor verification; verify both documents again." >&2 + exit 2 +} + +case "$CONVERGED" in true) ;; *) echo "ERROR: the verified review did not converge." >&2; exit 2 ;; esac +[[ "$ROUNDS" =~ ^[0-9]+$ ]] || { echo "ERROR: verified rounds must be numeric." >&2; exit 2; } +[ "$ROUNDS" -ge 1 ] && [ "$ROUNDS" -le 3 ] || { echo "ERROR: verified rounds must be between 1 and 3." >&2; exit 2; } IFS=',' read -r F_BLOCKER F_MAJOR F_MINOR F_NIT <<<"$FINDINGS" for v in "$F_BLOCKER" "$F_MAJOR" "$F_MINOR" "$F_NIT"; do - [[ "$v" =~ ^[0-9]+$ ]] || { echo "ERROR: --findings takes blocker,major,minor,nit as four numbers." >&2; exit 2; } + [[ "$v" =~ ^[0-9]+$ ]] || { echo "ERROR: verified findings must be blocker,major,minor,nit as four numbers." >&2; exit 2; } done # The verdict rule: any Blocker or Major means REQUEST_CHANGES, and a diff --git a/skills/doris-repo-review/scripts/record-review-runtime.sh b/skills/doris-repo-review/scripts/record-review-runtime.sh index dc921ab..d6efbca 100755 --- a/skills/doris-repo-review/scripts/record-review-runtime.sh +++ b/skills/doris-repo-review/scripts/record-review-runtime.sh @@ -42,8 +42,10 @@ is_valid_review_run_id "$REVIEW_RUN_ID" || { } RUNTIME_FILE="$CTX/review-runtime.env" -# A rejected replacement must not leave a previously qualified runtime available for reuse. -rm -f "$RUNTIME_FILE" "$CTX/pr-comment.md" "$CTX/pr-comment.url" +RESULT_FILE="$CTX/review-result.env" +# Every recording attempt starts a new attestation for this run. A rejected replacement must not +# leave a previously qualified runtime or verified result available for accidental reuse. +rm -f "$RUNTIME_FILE" "$RESULT_FILE" "$CTX/pr-comment.md" "$CTX/pr-comment.url" is_eligible_review_model "$MODEL" || { echo "ERROR: model '$MODEL' is not eligible for pipeline-equivalent review." >&2 diff --git a/skills/doris-repo-review/scripts/verify-anchors.py b/skills/doris-repo-review/scripts/verify-anchors.py index 8eefd3d..321f596 100755 --- a/skills/doris-repo-review/scripts/verify-anchors.py +++ b/skills/doris-repo-review/scripts/verify-anchors.py @@ -15,12 +15,15 @@ Anchors must be written inside backticks as `path:line` or `path:start-end`. Usage: verify-anchors.py --ctx <context-dir> --doc <en.md> --doc <zh.md> -Exits non-zero when an anchor is broken or the two documents disagree. +Exits non-zero when an anchor is broken or the two documents disagree. On success for a PR, it +also replaces `<ctx>/review-result.env` with the run id, head, verdict, severity counts, rounds, +convergence, standard document paths, and SHA-256 hashes consumed by the PASS poster. """ from __future__ import annotations import argparse +import hashlib import re import sys from collections import defaultdict @@ -29,6 +32,19 @@ from pathlib import Path ANCHOR_RE = re.compile(r"`([A-Za-z0-9_][A-Za-z0-9_./+-]*\.[A-Za-z0-9_+-]+):(\d+)(?:-(\d+))?`") FINDING_RE = re.compile(r"^#{2,4}\s+(?:\[)?(F-\d+)(?:\])?\b") CODE_FENCE_RE = re.compile(r"^\s*```") +UUID_RE = re.compile( + r"[0-9a-fA-F]{8}-[0-9a-fA-F]{4}-[1-5][0-9a-fA-F]{3}-" + r"[89abAB][0-9a-fA-F]{3}-[0-9a-fA-F]{12}" +) +SEVERITY_RE = re.compile( + r"^-\s+\*\*(?:Severity|等级)\*\*\s*[:：]\s*(Blocker|Major|Minor|Nit)\s*$", + re.IGNORECASE, +) +VERDICT_RE = re.compile( + r"^\|\s*(?:Verdict|结论)\s*\|\s*\*\*(APPROVE|REQUEST_CHANGES)\*\*\s*\|" +) +ROUNDS_ROW_RE = re.compile(r"^\|\s*(?:Rounds|轮次)\s*\|(.*?)\|\s*$") +SEVERITIES = ("Blocker", "Major", "Minor", "Nit") def parse_args() -> argparse.Namespace: @@ -62,11 +78,14 @@ def load_changed_ranges(ctx: Path) -> dict[str, list[tuple[int, int]]]: return ranges -def scan_document(doc: Path) -> tuple[list[str], dict[str, list[tuple[int, int, int | None]]], list[tuple[int, int, int | None]]]: +Anchor = tuple[str, int, int | None, int] + + +def scan_document(doc: Path) -> tuple[list[str], dict[str, list[Anchor]], list[Anchor]]: """Return (finding ids in order, anchors per finding, all anchors).""" findings: list[str] = [] - per_finding: dict[str, list[tuple[int, int, int | None]]] = defaultdict(list) - all_anchors: list[tuple[str, int, int | None, int]] = [] + per_finding: dict[str, list[Anchor]] = defaultdict(list) + all_anchors: list[Anchor] = [] current: str | None = None in_fence = False @@ -74,13 +93,13 @@ def scan_document(doc: Path) -> tuple[list[str], dict[str, list[tuple[int, int, if CODE_FENCE_RE.match(line): in_fence = not in_fence continue + if in_fence: + continue heading = FINDING_RE.match(line) if heading: current = heading.group(1) if current not in findings: findings.append(current) - if in_fence: - continue for match in ANCHOR_RE.finditer(line): path, start, end = match.group(1), int(match.group(2)), match.group(3) anchor = (path, start, int(end) if end else None, lineno) @@ -95,16 +114,136 @@ def in_changed_range(ranges: list[tuple[int, int]], start: int, end: int | None) return any(not (last < lo or start > hi) for lo, hi in ranges) +def parse_review_summary( + doc: Path, +) -> tuple[str | None, int | None, bool | None, dict[str, str], list[str]]: + """Return verdict, rounds, convergence, per-finding severity, and format errors.""" + current: str | None = None + in_fence = False + verdict: str | None = None + rounds: int | None = None + converged: bool | None = None + severities: dict[str, str] = {} + errors: list[str] = [] + + for lineno, line in enumerate(doc.read_text(encoding="utf-8").splitlines(), start=1): + if CODE_FENCE_RE.match(line): + in_fence = not in_fence + continue + if in_fence: + continue + heading = FINDING_RE.match(line) + if heading: + current = heading.group(1) + continue + severity_match = SEVERITY_RE.match(line) + if severity_match: + if current is None: + errors.append(f"{doc}:{lineno}: severity is not under a finding heading") + elif current in severities: + errors.append(f"{doc}:{lineno}: duplicate severity for {current}") + else: + severities[current] = severity_match.group(1).title() + verdict_match = VERDICT_RE.match(line) + if verdict_match: + if verdict is not None: + errors.append(f"{doc}:{lineno}: duplicate verdict row") + verdict = verdict_match.group(1) + rounds_match = ROUNDS_ROW_RE.match(line) + if rounds_match: + if rounds is not None: + errors.append(f"{doc}:{lineno}: duplicate rounds row") + continue + value = rounds_match.group(1) + number = re.search(r"\d+", value) + if number is None: + errors.append(f"{doc}:{lineno}: rounds row has no number") + else: + rounds = int(number.group(0)) + lowered = value.casefold() + if "not converged" in lowered or "未收敛" in value: + converged = False + elif "converged" in lowered or "已收敛" in value: + converged = True + else: + errors.append(f"{doc}:{lineno}: rounds row does not state convergence") + + if verdict is None: + errors.append(f"{doc}: missing Verdict/结论 row") + if rounds is None: + errors.append(f"{doc}: missing Rounds/轮次 row") + elif not 1 <= rounds <= 3: + errors.append(f"{doc}: rounds must be between 1 and 3") + return verdict, rounds, converged, severities, errors + + +def sha256_file(path: Path) -> str: + digest = hashlib.sha256() + with path.open("rb") as handle: + for chunk in iter(lambda: handle.read(1024 * 1024), b""): + digest.update(chunk) + return digest.hexdigest() + + +def write_review_result( + ctx: Path, + meta: dict[str, str], + docs: dict[str, Path], + verdict: str, + findings: tuple[int, int, int, int], + rounds: int, + converged: bool, +) -> Path: + result = ctx / "review-result.env" + temporary = ctx / ".review-result.env.tmp" + lines = [ + f"REVIEW_RUN_ID={meta['REVIEW_RUN_ID']}", + f"REVIEW_HEAD_SHA={meta['HEAD_SHA']}", + f"VERDICT={verdict}", + f"FINDINGS={','.join(str(value) for value in findings)}", + f"ROUNDS={rounds}", + f"CONVERGED={'true' if converged else 'false'}", + f"EN_DOC={docs['en']}", + f"EN_DOC_SHA256={sha256_file(docs['en'])}", + f"ZH_DOC={docs['zh']}", + f"ZH_DOC_SHA256={sha256_file(docs['zh'])}", + ] + temporary.write_text("\n".join(lines) + "\n", encoding="utf-8") + temporary.replace(result) + return result + + def main() -> int: args = parse_args() ctx = args.ctx.resolve() meta = read_meta(ctx) + result_path = ctx / "review-result.env" + try: + result_path.unlink() + except FileNotFoundError: + pass repo_root = (args.repo_root or Path(meta.get("REPO_ROOT", "."))).resolve() changed_ranges = load_changed_ranges(ctx) errors: list[str] = [] warnings: list[str] = [] finding_sets: dict[Path, list[str]] = {} + summaries: dict[Path, tuple[str | None, int | None, bool | None, dict[str, str]]] = {} + + if len(args.doc) != 2: + errors.append("exactly two review documents (EN and ZH) are required") + + pr_number = meta.get("PR_NUMBER", "") + docs_root = Path(meta.get("DOCS_ROOT", repo_root)).resolve() + expected_docs: dict[str, Path] = {} + if pr_number: + expected_docs = { + "en": (docs_root / "review-docs" / f"pr-{pr_number}-review.en.md").resolve(), + "zh": (docs_root / "review-docs" / f"pr-{pr_number}-review.zh.md").resolve(), + } + actual_docs = {doc.resolve() for doc in args.doc} + if actual_docs != set(expected_docs.values()): + errors.append("review documents must use the standard PR-specific EN/ZH paths") for doc in args.doc: doc = doc.resolve() @@ -113,6 +252,9 @@ def main() -> int: continue findings, per_finding, anchors = scan_document(doc) finding_sets[doc] = findings + verdict, rounds, converged, severities, summary_errors = parse_review_summary(doc) + summaries[doc] = (verdict, rounds, converged, severities) + errors.extend(summary_errors) print(f"\n=== {doc} ===") print(f"findings: {len(findings)} anchors: {len(anchors)}") @@ -139,18 +281,27 @@ def main() -> int: for finding in findings: if not per_finding.get(finding): errors.append(f"{doc}: finding {finding} has no `path:line` anchor") + if finding not in severities: + errors.append(f"{doc}: finding {finding} has no severity") docs = list(finding_sets) if len(docs) >= 2: - base_doc, base_ids = docs[0], set(finding_sets[docs[0]]) + base_doc, base_ids = docs[0], finding_sets[docs[0]] for other in docs[1:]: - other_ids = set(finding_sets[other]) - missing = sorted(base_ids - other_ids) - extra = sorted(other_ids - base_ids) - if missing: - errors.append(f"{other}: missing finding IDs present in {base_doc.name}: {', '.join(missing)}") - if extra: - errors.append(f"{other}: has finding IDs absent from {base_doc.name}: {', '.join(extra)}") + other_ids = finding_sets[other] + if other_ids != base_ids: + errors.append(f"{other}: finding IDs or order differ from {base_doc.name}") + if summaries[other][3] != summaries[base_doc][3]: + errors.append(f"{other}: finding severities differ from {base_doc.name}") + if summaries[other][:3] != summaries[base_doc][:3]: + errors.append(f"{other}: verdict, rounds, or convergence differs from {base_doc.name}") + + if docs: + verdict, rounds, converged, severities = summaries[docs[0]] + counts = tuple(sum(1 for value in severities.values() if value == name) for name in SEVERITIES) + expected_verdict = "REQUEST_CHANGES" if counts[0] or counts[1] else "APPROVE" + if verdict is not None and verdict != expected_verdict: + errors.append(f"{docs[0]}: verdict {verdict} conflicts with finding severities") if warnings: print("\n--- notes ---") @@ -161,10 +312,23 @@ def main() -> int: print("\n--- errors ---") for error in errors: print(f" ERROR: {error}") - print(f"\nFAILED: {len(errors)} anchor problem(s).") + print(f"\nFAILED: {len(errors)} review document problem(s).") return 1 + if not pr_number or set(expected_docs.values()) != set(docs): + print("\nOK: every anchor resolves and both documents expose the same findings.") + print("No PR-bound review result was written.") + return 0 + + run_id = meta.get("REVIEW_RUN_ID", "") + head_sha = meta.get("HEAD_SHA", "") + if not UUID_RE.fullmatch(run_id) or not re.fullmatch(r"[0-9a-fA-F]{40}", head_sha): + print("\nFAILED: meta.env lacks a valid REVIEW_RUN_ID or HEAD_SHA.") + return 1 + assert verdict is not None and rounds is not None and converged is not None + result = write_review_result(ctx, meta, expected_docs, verdict, counts, rounds, converged) print("\nOK: every anchor resolves and both documents expose the same findings.") + print(f"review result: {result}") return 0 diff --git a/verify/repo-review/run.sh b/verify/repo-review/run.sh index 545b764..cd5c9d0 100755 --- a/verify/repo-review/run.sh +++ b/verify/repo-review/run.sh @@ -1,13 +1,14 @@ #!/usr/bin/env bash -# Core contract tests for qualified doris-repo-review PASS receipts. No network writes. +# Deterministic producer tests for doris-repo-review PASS receipts. No network writes. set -euo pipefail ROOT="$(cd "$(dirname "$0")/../.." && pwd)" S="$ROOT/skills/doris-repo-review/scripts" TMP_ROOT="$(mktemp -d "${TMPDIR:-/tmp}/doris-repo-review-test.XXXXXX")" trap 'rm -rf "$TMP_ROOT"' EXIT -mkdir -p "$TMP_ROOT/bin" "$TMP_ROOT/repo" +mkdir -p "$TMP_ROOT/bin" "$TMP_ROOT/repo/src" "$TMP_ROOT/repo/review-docs" +FIXTURE_REPO="$TMP_ROOT/repo" HEAD_SHA="1111111111111111111111111111111111111111" OTHER_SHA="3333333333333333333333333333333333333333" BASE_SHA="2222222222222222222222222222222222222222" @@ -15,6 +16,8 @@ RUN_ID="aaaaaaaa-aaaa-4aaa-8aaa-aaaaaaaaaaaa" OTHER_RUN_ID="bbbbbbbb-bbbb-4bbb-8bbb-bbbbbbbbbbbb" PASS_COUNT=0 +printf 'int answer = 42;\n' > "$FIXTURE_REPO/src/Foo.java" + fail() { echo "FAIL: $*" >&2 exit 1 @@ -64,18 +67,62 @@ PR_URL=https://github.com/apache/doris/pull/123 BASE_SHA=$BASE_SHA HEAD_SHA=$head REVIEW_RUN_ID=$run_id -REPO_ROOT=$TMP_ROOT/repo +REPO_ROOT=$FIXTURE_REPO +DOCS_ROOT=$FIXTURE_REPO +EOF + printf 'src/Foo.java\t1\t1\n' > "$ctx/changed_line_ranges.tsv" +} + +write_docs() { + local verdict="$1" + local severity="$2" + local rounds="$3" + local convergence="$4" + local zh_convergence="已收敛" + [ "$convergence" = "converged" ] || zh_convergence="未收敛" + cat > "$FIXTURE_REPO/review-docs/pr-123-review.en.md" <<EOF +# Code Review — PR #123: fixture + +| | | +|---|---| +| Verdict | **$verdict** | +| Rounds | $rounds of max 3, $convergence | + +## Findings + +### F-01 · fixture finding + +- **Severity**: $severity +- **Where**: \`src/Foo.java:1\` +EOF + cat > "$FIXTURE_REPO/review-docs/pr-123-review.zh.md" <<EOF +# 代码评审 — PR #123：fixture + +| | | +|---|---| +| 结论 | **$verdict** | +| 轮次 | 共 $rounds 轮（上限 3），$zh_convergence | + +## 问题清单 + +### F-01 · fixture finding + +- **等级**：$severity +- **位置**：\`src/Foo.java:1\` EOF } record_codex_runtime() { - "$S/record-review-runtime.sh" --ctx "$1" \ + local ctx="$1" + "$S/record-review-runtime.sh" --ctx "$ctx" \ --model gpt-5.6-sol --effort xhigh --source codex-explicit-spawn } -post_pass() { - "$S/post-pass-comment.sh" --ctx "$1" \ - --findings 0,0,1,0 --rounds 2 --converged true "${@:2}" +verify_docs() { + local ctx="$1" + python3 "$S/verify-anchors.py" --ctx "$ctx" \ + --doc "$FIXTURE_REPO/review-docs/pr-123-review.en.md" \ + --doc "$FIXTURE_REPO/review-docs/pr-123-review.zh.md" } cat > "$TMP_ROOT/bin/gh" <<'EOF' @@ -108,8 +155,11 @@ export MOCK_LIVE_HEAD="$HEAD_SHA" CTX="$TMP_ROOT/ctx" write_meta "$CTX" "$HEAD_SHA" + expect_failure "poster requires a runtime record" "review-runtime.env not found" \ - post_pass "$CTX" --dry-run + "$S/post-pass-comment.sh" --ctx "$CTX" --dry-run +expect_failure "poster rejects command-line runtime overrides" "unknown argument: --model" \ + "$S/post-pass-comment.sh" --ctx "$CTX" --model gpt-5.6-sol --effort xhigh --dry-run record_codex_runtime "$CTX" >"$TMP_ROOT/record.out" assert_file_contains "$CTX/review-runtime.env" "REVIEW_MODEL=gpt-5.6-sol" @@ -117,42 +167,119 @@ assert_file_contains "$CTX/review-runtime.env" "REVIEW_EFFORT=xhigh" assert_file_contains "$CTX/review-runtime.env" "REVIEW_HEAD_SHA=$HEAD_SHA" assert_file_contains "$CTX/review-runtime.env" "REVIEW_RUN_ID=$RUN_ID" assert_file_contains "$CTX/review-runtime.env" "REVIEW_RUNTIME_SOURCE=codex-explicit-spawn" -pass "qualified runtime is bound to the review run and commit" +pass "qualified Codex runtime is bound to the review run and commit" + +expect_failure "poster requires complete review documents" "did not pass final verification" \ + "$S/post-pass-comment.sh" --ctx "$CTX" --dry-run +write_docs APPROVE Minor 2 converged +verify_docs "$CTX" >"$TMP_ROOT/verify.out" +assert_file_contains "$CTX/review-result.env" "VERDICT=APPROVE" +assert_file_contains "$CTX/review-result.env" "FINDINGS=0,0,1,0" +assert_file_contains "$CTX/review-result.env" "ROUNDS=2" +assert_file_contains "$CTX/review-result.env" "CONVERGED=true" +pass "verifier binds document result and hashes to the review run" + +python3 - "$CTX/review-result.env" <<'PY' +from pathlib import Path +import sys -post_pass "$CTX" --dry-run >"$TMP_ROOT/dry-run.out" +path = Path(sys.argv[1]) +path.write_text(path.read_text().replace("FINDINGS=0,0,1,0", "FINDINGS=0,0,99,0")) +PY +"$S/post-pass-comment.sh" --ctx "$CTX" --dry-run >"$TMP_ROOT/dry-run.out" assert_file_contains "$CTX/pr-comment.md" "commit: $HEAD_SHA" assert_file_contains "$CTX/pr-comment.md" "model: gpt-5.6-sol" assert_file_contains "$CTX/pr-comment.md" "effort: xhigh" assert_file_contains "$CTX/pr-comment.md" "findings: {blocker: 0, major: 0, minor: 1, nit: 0}" +assert_file_contains "$TMP_ROOT/dry-run.out" "dry run - nothing was posted." +ACTUAL_FIELDS="$(awk ' + /^```yaml$/ { inside=1; next } + inside && /^```$/ { exit } + inside { sub(/:.*/, ""); print } +' "$CTX/pr-comment.md")" +EXPECTED_FIELDS="$(printf '%s\n' schema status pr commit base reviewed_at reviewer model effort findings rounds converged)" +[ "$ACTUAL_FIELDS" = "$EXPECTED_FIELDS" ] || fail "receipt fields or ordering drifted from v1" [ -z "${RECEIPT_OUTPUT:-}" ] || cp "$CTX/pr-comment.md" "$RECEIPT_OUTPUT" -pass "qualified dry run renders a pipeline-compatible receipt" +pass "verified qualified dry run renders a pipeline-compatible receipt" : > "$MOCK_GH_LOG" -MOCK_SAME_COMMENT=0 post_pass "$CTX" >"$TMP_ROOT/create.out" +MOCK_SAME_COMMENT=0 "$S/post-pass-comment.sh" --ctx "$CTX" >"$TMP_ROOT/create.out" assert_file_contains "$MOCK_GH_LOG" "--method POST repos/apache/doris/issues/123/comments --input -" -pass "qualified PASS automatically creates a comment" +assert_file_contains "$CTX/pr-comment.url" "#issuecomment-test" +pass "converged qualified PASS automatically creates the comment" : > "$MOCK_GH_LOG" -MOCK_SAME_COMMENT=1 post_pass "$CTX" >"$TMP_ROOT/update.out" +MOCK_SAME_COMMENT=1 "$S/post-pass-comment.sh" --ctx "$CTX" >"$TMP_ROOT/update.out" assert_file_contains "$MOCK_GH_LOG" "--method PATCH repos/apache/doris/issues/comments/987 --input -" -pass "same-commit rerun updates the existing comment" +pass "same-commit rerun automatically updates the existing comment" -REPLACE_CTX="$TMP_ROOT/replace-ctx" -write_meta "$REPLACE_CTX" "$HEAD_SHA" -record_codex_runtime "$REPLACE_CTX" >"$TMP_ROOT/replace-record.out" +FAILED_REPLACE_CTX="$TMP_ROOT/failed-replace-ctx" +write_meta "$FAILED_REPLACE_CTX" "$HEAD_SHA" +write_docs APPROVE Minor 2 converged +record_codex_runtime "$FAILED_REPLACE_CTX" >"$TMP_ROOT/replace-record.out" +verify_docs "$FAILED_REPLACE_CTX" >"$TMP_ROOT/replace-verify.out" expect_failure "high effort is below the required floor" "use xhigh or higher" \ - "$S/record-review-runtime.sh" --ctx "$REPLACE_CTX" \ + "$S/record-review-runtime.sh" --ctx "$FAILED_REPLACE_CTX" \ --model gpt-5.6-sol --effort high --source codex-explicit-spawn -assert_file_absent "$REPLACE_CTX/review-runtime.env" -pass "failed replacement invalidates the old runtime record" +assert_file_absent "$FAILED_REPLACE_CTX/review-runtime.env" +assert_file_absent "$FAILED_REPLACE_CTX/review-result.env" +pass "failed runtime replacement invalidates old credentials" expect_failure "unapproved model is rejected" "is not eligible" \ - "$S/record-review-runtime.sh" --ctx "$REPLACE_CTX" \ + "$S/record-review-runtime.sh" --ctx "$FAILED_REPLACE_CTX" \ --model gpt-5.6-terra --effort xhigh --source codex-explicit-spawn expect_failure "model/source mismatch is rejected" "is not valid" \ - "$S/record-review-runtime.sh" --ctx "$REPLACE_CTX" \ + "$S/record-review-runtime.sh" --ctx "$FAILED_REPLACE_CTX" \ --model gpt-5.6-sol --effort xhigh --source claude-explicit-session +TAMPERED_CTX="$TMP_ROOT/tampered-ctx" +write_meta "$TAMPERED_CTX" "$HEAD_SHA" +write_docs APPROVE Minor 2 converged +record_codex_runtime "$TAMPERED_CTX" >"$TMP_ROOT/tampered-record.out" +verify_docs "$TAMPERED_CTX" >"$TMP_ROOT/tampered-verify.out" +cat > "$TAMPERED_CTX/review-runtime.env" <<EOF +REVIEW_MODEL=gpt-5.6-sol +REVIEW_EFFORT=high +REVIEW_HEAD_SHA=$HEAD_SHA +REVIEW_RUN_ID=$RUN_ID +REVIEW_RUNTIME_SOURCE=codex-explicit-spawn +EOF +expect_failure "poster independently rejects a tampered low effort" "use xhigh or higher" \ + "$S/post-pass-comment.sh" --ctx "$TAMPERED_CTX" --dry-run + +REUSED_CTX="$TMP_ROOT/reused-ctx" +write_meta "$REUSED_CTX" "$HEAD_SHA" +write_docs APPROVE Minor 2 converged +record_codex_runtime "$REUSED_CTX" >"$TMP_ROOT/reused-record.out" +verify_docs "$REUSED_CTX" >"$TMP_ROOT/reused-verify.out" +write_meta "$REUSED_CTX" "$HEAD_SHA" "$OTHER_RUN_ID" +expect_failure "runtime record cannot be reused by a later review" "different review run" \ + "$S/post-pass-comment.sh" --ctx "$REUSED_CTX" --dry-run + +HASH_CTX="$TMP_ROOT/hash-ctx" +write_meta "$HASH_CTX" "$HEAD_SHA" +write_docs APPROVE Minor 2 converged +record_codex_runtime "$HASH_CTX" >"$TMP_ROOT/hash-record.out" +verify_docs "$HASH_CTX" >"$TMP_ROOT/hash-verify.out" +cat >> "$FIXTURE_REPO/review-docs/pr-123-review.en.md" <<'EOF' + +### F-02 · added after verification + +- **Severity**: Minor +- **Where**: `src/Foo.java:1` +EOF +expect_failure "invalid document changes are caught by final re-verification" "did not pass final verification" \ + "$S/post-pass-comment.sh" --ctx "$HASH_CTX" --dry-run + +CLAUDE_CTX="$TMP_ROOT/claude-ctx" +write_meta "$CLAUDE_CTX" "$HEAD_SHA" +"$S/record-review-runtime.sh" --ctx "$CLAUDE_CTX" \ + --model 'claude-opus-5[1m]' --effort max --source claude-explicit-session \ + >"$TMP_ROOT/claude-record.out" +assert_file_contains "$CLAUDE_CTX/review-runtime.env" "REVIEW_MODEL=claude-opus-5[1m]" +pass "eligible Claude runtime is accepted" + +# Keep this list explicit: a family-prefix match would silently admit future or lower-tier models. # shellcheck source=/dev/null . "$S/review-runtime-policy.sh" for model in claude-opus-5 'claude-opus-5[1m]' claude-fable-5 'claude-fable-5[1m]' gpt-5.6-sol; do @@ -173,33 +300,51 @@ for effort in minimal low medium high unknown; do done pass "model and effort allowlists are exact" -REUSED_CTX="$TMP_ROOT/reused-ctx" -write_meta "$REUSED_CTX" "$HEAD_SHA" -record_codex_runtime "$REUSED_CTX" >"$TMP_ROOT/reused-record.out" -write_meta "$REUSED_CTX" "$HEAD_SHA" "$OTHER_RUN_ID" -expect_failure "runtime record cannot be reused by a later review" "different review run" \ - post_pass "$REUSED_CTX" --dry-run +NONCONVERGED_CTX="$TMP_ROOT/nonconverged-ctx" +write_meta "$NONCONVERGED_CTX" "$HEAD_SHA" +write_docs APPROVE Minor 3 "not converged" +record_codex_runtime "$NONCONVERGED_CTX" >"$TMP_ROOT/nonconverged-record.out" +verify_docs "$NONCONVERGED_CTX" >"$TMP_ROOT/nonconverged-verify.out" +expect_failure "non-converged review cannot post PASS" "verified review did not converge" \ + "$S/post-pass-comment.sh" --ctx "$NONCONVERGED_CTX" --dry-run + +INVALID_ROUNDS_CTX="$TMP_ROOT/invalid-rounds-ctx" +write_meta "$INVALID_ROUNDS_CTX" "$HEAD_SHA" +write_docs APPROVE Minor 0 converged +expect_failure "round zero is rejected by the document verifier" "between 1 and 3" \ + verify_docs "$INVALID_ROUNDS_CTX" +assert_file_absent "$INVALID_ROUNDS_CTX/review-result.env" +write_docs APPROVE Minor 4 converged +expect_failure "round four is rejected by the document verifier" "between 1 and 3" \ + verify_docs "$INVALID_ROUNDS_CTX" + +CHANGES_CTX="$TMP_ROOT/changes-ctx" +write_meta "$CHANGES_CTX" "$HEAD_SHA" +write_docs REQUEST_CHANGES Major 2 converged +record_codex_runtime "$CHANGES_CTX" >"$TMP_ROOT/changes-record.out" +verify_docs "$CHANGES_CTX" >"$TMP_ROOT/changes-verify.out" +expect_failure "REQUEST_CHANGES result cannot post PASS" "not APPROVE" \ + "$S/post-pass-comment.sh" --ctx "$CHANGES_CTX" --dry-run MISMATCH_CTX="$TMP_ROOT/mismatch-ctx" write_meta "$MISMATCH_CTX" "$HEAD_SHA" -record_codex_runtime "$MISMATCH_CTX" >"$TMP_ROOT/mismatch-record.out" +write_docs APPROVE Minor 2 converged +"$S/record-review-runtime.sh" --ctx "$MISMATCH_CTX" \ + --model gpt-5.6-sol --effort xhigh --source codex-explicit-task \ + >"$TMP_ROOT/mismatch-record.out" +verify_docs "$MISMATCH_CTX" >"$TMP_ROOT/mismatch-verify.out" write_meta "$MISMATCH_CTX" "$OTHER_SHA" MOCK_LIVE_HEAD="$OTHER_SHA" expect_failure \ "runtime record is bound to one commit" "recorded for a different commit" \ - post_pass "$MISMATCH_CTX" --dry-run + "$S/post-pass-comment.sh" --ctx "$MISMATCH_CTX" --dry-run MOVED_CTX="$TMP_ROOT/moved-ctx" write_meta "$MOVED_CTX" "$HEAD_SHA" +write_docs APPROVE Minor 2 converged record_codex_runtime "$MOVED_CTX" >"$TMP_ROOT/moved-record.out" +verify_docs "$MOVED_CTX" >"$TMP_ROOT/moved-verify.out" MOCK_LIVE_HEAD="$OTHER_SHA" expect_failure \ "moved PR head invalidates the review" "PR head moved since the review" \ - post_pass "$MOVED_CTX" --dry-run - -expect_failure "non-converged review cannot post PASS" "requires --converged true" \ - "$S/post-pass-comment.sh" --ctx "$CTX" \ - --findings 0,0,0,0 --rounds 3 --converged false --dry-run -expect_failure "Blocker result cannot post PASS" "mean REQUEST_CHANGES" \ - "$S/post-pass-comment.sh" --ctx "$CTX" \ - --findings 1,0,0,0 --rounds 2 --converged true --dry-run + "$S/post-pass-comment.sh" --ctx "$MOVED_CTX" --dry-run -echo "$PASS_COUNT review-runtime tests passed" +echo "$PASS_COUNT review-receipt tests passed" --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
