diff --git a/Scripts/review-gate.sh b/Scripts/review-gate.sh index 45d99f5cc..064849ced 100755 --- a/Scripts/review-gate.sh +++ b/Scripts/review-gate.sh @@ -10,7 +10,8 @@ Run the pre-PR review gate for the current branch. Exit codes: 0 Local review gate ran and passed. 2 Remote review gate required; PR must wait for GitHub claude-review. - 1 Local review gate ran and failed, or repository state is invalid. + 1 Local review gate ran and failed (a CONFIRMED finding, a review that did + not complete, or one without a verdict), or repository state is invalid. Options: --require-local Treat unavailable local review tooling as a failure. @@ -53,8 +54,21 @@ if ! git rev-parse --verify origin/master >/dev/null 2>&1; then exit 1 fi -changed_summary=$(git diff --stat origin/master...HEAD) -if [[ -z "$changed_summary" ]] && [[ -z "$(git diff --stat)" ]]; then +# Review everything the branch would ship: commits since the merge base, plus +# uncommitted tracked changes, plus untracked files. A diff of commits only +# reviews nothing for a branch whose work is still in the working tree. +base=$(git merge-base origin/master HEAD) +# BSD mktemp replaces only trailing X's, so the template must end in them; +# "….XXXXXX.patch" made one fixed filename and a second run collided. +diff_file=$(mktemp "${TMPDIR:-/tmp}/keypath-review-diff.XXXXXX") +output_file="" +trap 'rm -f "$diff_file" ${output_file:+"$output_file"}' EXIT +git diff --find-renames "$base" > "$diff_file" +while IFS= read -r -d '' untracked; do + git diff --no-index -- /dev/null "$untracked" >> "$diff_file" || true +done < <(git ls-files -z --others --exclude-standard) + +if [[ ! -s "$diff_file" ]]; then echo "review-gate: no branch or working-tree diff to review" exit 0 fi @@ -66,21 +80,36 @@ if command -v thermo-nuclear-swift-review >/dev/null 2>&1; then fi if command -v claude >/dev/null 2>&1; then - echo "review-gate: running claude review prompt against origin/master...HEAD" - diff_file=$(mktemp "${TMPDIR:-/tmp}/keypath-review-diff.XXXXXX.patch") - trap 'rm -f "$diff_file"' EXIT - git diff --find-renames origin/master...HEAD > "$diff_file" - claude <&2 + exit 1 + fi + # The exit code says the CLI ran, not what the review found: gate on the verdict. + # `|| true`: under pipefail a grep that finds no verdict would otherwise end + # the script with status 1 and no message — a silent failure. + verdict=$(grep -E '^REVIEW-VERDICT: (PASS|FAIL)[[:space:]]*$' "$output_file" | tail -1 | awk '{print $2}' || true) + case "$verdict" in + PASS) echo "review-gate: review passed"; exit 0 ;; + FAIL) echo "review-gate: review found CONFIRMED issues" >&2; exit 1 ;; + *) echo "review-gate: the review gave no REVIEW-VERDICT line; cannot tell whether it passed" >&2; exit 1 ;; + esac fi message="review-gate: remote review gate selected; GitHub claude-review must pass before merge" diff --git a/Tests/KeyPathTests/BuildScripts/ReviewGateScriptTests.swift b/Tests/KeyPathTests/BuildScripts/ReviewGateScriptTests.swift new file mode 100644 index 000000000..a50a1a2e0 --- /dev/null +++ b/Tests/KeyPathTests/BuildScripts/ReviewGateScriptTests.swift @@ -0,0 +1,128 @@ +import Foundation +@preconcurrency import XCTest + +/// Scripts/review-gate.sh must review what the branch would ship and pass only +/// on an explicit PASS verdict. Runs the real script in a scratch repository +/// with a stub `claude` on PATH. +final class ReviewGateScriptTests: XCTestCase { + private var repo: URL! + private var stubs: URL! + + private static var repositoryRoot: URL { + var url = URL(fileURLWithPath: #filePath) + while url.path != "/", !FileManager.default.fileExists(atPath: url.appendingPathComponent("Package.swift").path) { + url.deleteLastPathComponent() + } + return url + } + + @discardableResult + private func run(_ executable: String, _ arguments: [String], env extra: [String: String] = [:]) throws -> (code: Int32, out: String, err: String) { + let process = Process() + process.executableURL = URL(fileURLWithPath: executable) + process.arguments = arguments + process.currentDirectoryURL = repo + var env = ["PATH": "\(stubs.path):/usr/bin:/bin:/opt/homebrew/bin", "HOME": NSTemporaryDirectory(), + "TMPDIR": NSTemporaryDirectory(), "GIT_AUTHOR_NAME": "t", "GIT_AUTHOR_EMAIL": "t@t", + "GIT_COMMITTER_NAME": "t", "GIT_COMMITTER_EMAIL": "t@t", "GIT_CONFIG_NOSYSTEM": "1"] + env.merge(extra) { $1 } + process.environment = env + let out = Pipe(), err = Pipe() + process.standardOutput = out + process.standardError = err + try process.run() + let outData = out.fileHandleForReading.readDataToEndOfFile() + let errData = err.fileHandleForReading.readDataToEndOfFile() + process.waitUntilExit() + return (process.terminationStatus, String(decoding: outData, as: UTF8.self), String(decoding: errData, as: UTF8.self)) + } + + private func git(_ args: String...) throws { + try run("/usr/bin/env", ["git"] + args) + } + + private func gate(_ env: [String: String] = [:]) throws -> (code: Int32, out: String, err: String) { + try run("/bin/bash", ["Scripts/review-gate.sh"], env: env) + } + + private var reviewed: String { + (try? String(contentsOf: stubs.appendingPathComponent("stdin.txt"), encoding: .utf8)) ?? "" + } + + override func setUpWithError() throws { + let root = FileManager.default.temporaryDirectory.appendingPathComponent("review-gate-\(UUID().uuidString)") + repo = root.appendingPathComponent("repo") + stubs = root.appendingPathComponent("stubs") + try FileManager.default.createDirectory(at: repo.appendingPathComponent("Scripts"), withIntermediateDirectories: true) + try FileManager.default.createDirectory(at: stubs, withIntermediateDirectories: true) + try FileManager.default.copyItem(at: Self.repositoryRoot.appendingPathComponent("Scripts/review-gate.sh"), + to: repo.appendingPathComponent("Scripts/review-gate.sh")) + // The stub records what it was asked to review and answers as told. + let stub = stubs.appendingPathComponent("claude") + try """ + #!/bin/bash + cat > "\(stubs.path)/stdin.txt" + echo "Findings: none worth noting." + [[ -n "${STUB_VERDICT:-}" ]] && echo "REVIEW-VERDICT: $STUB_VERDICT" + exit "${STUB_EXIT:-0}" + """.write(to: stub, atomically: true, encoding: .utf8) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: stub.path) + try git("init", "-q", "-b", "master") + try "base\n".write(to: repo.appendingPathComponent("README"), atomically: true, encoding: .utf8) + try git("add", "-A") + try git("commit", "-q", "-m", "base") + try git("update-ref", "refs/remotes/origin/master", "HEAD") + try git("checkout", "-q", "-b", "feature") + } + + override func tearDownWithError() throws { + try? FileManager.default.removeItem(at: repo.deletingLastPathComponent()) + } + + private func commitChange() throws { + try "committed change\n".write(to: repo.appendingPathComponent("committed.txt"), atomically: true, encoding: .utf8) + try git("add", "-A") + try git("commit", "-q", "-m", "change") + } + + func testPassesOnlyOnAnExplicitPassVerdictAndRunsTwice() throws { + try commitChange() + for _ in 0 ..< 2 { // the old fixed mktemp name collided on the second run + let result = try gate(["STUB_VERDICT": "PASS"]) + XCTAssertEqual(result.code, 0, result.err) + } + XCTAssertTrue(reviewed.contains("committed change"), reviewed) + } + + func testAConfirmedFindingFailsTheGate() throws { + try commitChange() + XCTAssertEqual(try gate(["STUB_VERDICT": "FAIL"]).code, 1) + } + + func testAReviewWithoutAVerdictFailsTheGate() throws { + try commitChange() + let result = try gate() + XCTAssertEqual(result.code, 1) + XCTAssertTrue(result.err.contains("no REVIEW-VERDICT line"), result.err) + } + + func testAReviewThatDidNotCompleteFailsTheGate() throws { + try commitChange() + XCTAssertEqual(try gate(["STUB_VERDICT": "PASS", "STUB_EXIT": "3"]).code, 1) + } + + func testUncommittedAndUntrackedWorkIsReviewed() throws { + try "edited\n".write(to: repo.appendingPathComponent("README"), atomically: true, encoding: .utf8) + try "brand new\n".write(to: repo.appendingPathComponent("new.txt"), atomically: true, encoding: .utf8) + XCTAssertEqual(try gate(["STUB_VERDICT": "PASS"]).code, 0) + XCTAssertTrue(reviewed.contains("edited"), reviewed) + XCTAssertTrue(reviewed.contains("brand new"), reviewed) + } + + func testNothingToReviewNeverAsksTheReviewer() throws { + let result = try gate(["STUB_VERDICT": "FAIL"]) + XCTAssertEqual(result.code, 0) + XCTAssertTrue(result.out.contains("no branch or working-tree diff"), result.out) + XCTAssertEqual(reviewed, "") + } +} diff --git a/docs/process/agent-pr-workflow.md b/docs/process/agent-pr-workflow.md index 36a97baf6..8116255d5 100644 --- a/docs/process/agent-pr-workflow.md +++ b/docs/process/agent-pr-workflow.md @@ -30,7 +30,9 @@ End-to-end process for shepherding code from initial request through to a clean 6b. **Run `./Scripts/review-gate.sh`** — before creating the PR, run the review gate against the branch diff. If local review tooling is available, the -script runs it and returns 0 only when it passes. If local review tooling is +script runs it over the branch's commits, uncommitted changes and untracked +files, and returns 0 only when the review's `REVIEW-VERDICT` line says PASS +(a CONFIRMED finding, a failed run or a missing verdict is exit 1). If local review tooling is unavailable, exit 2 is the expected remote-review path: record `remote review gate selected` in the PR and do not merge until the GitHub `claude-review` check passes. Do not report missing local review tools as a PR issue; that is