From 84e54ab264c3ddbb13b98c9acc12426e89799673 Mon Sep 17 00:00:00 2001 From: Peter Steinberger Date: Sun, 19 Jul 2026 02:09:55 -0700 Subject: [PATCH] =?UTF-8?q?chore(pr):=20landing-UX=20=E2=80=94=20aggregate?= =?UTF-8?q?d=20validation,=20lock=20classes,=20ci-dispatch,=20committer=20?= =?UTF-8?q?guard=20(#111287)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(pr): landing-UX — aggregate artifact validation, lock classes, ci-dispatch, committer worktree guard * fix(pr): CI lane cleanup for landing-ux — lint causes, hermetic tests, private helpers * chore(deadcode): register pr-lib CLI entries in knip config --- config/knip.config.ts | 2 + scripts/AGENTS.md | 2 +- scripts/committer | 59 ++- scripts/pr | 11 +- scripts/pr-lib/ci-dispatch.mjs | 165 ++++++ scripts/pr-lib/gates.sh | 63 ++- scripts/pr-lib/merge.sh | 2 + scripts/pr-lib/operation-lock.sh | 29 ++ scripts/pr-lib/prepare-core.sh | 4 + scripts/pr-lib/process-group-runner.mjs | 31 +- scripts/pr-lib/review-artifacts.mjs | 474 ++++++++++++++++++ scripts/pr-lib/review.sh | 336 +------------ test/scripts/committer.test.ts | 66 ++- test/scripts/pr-ci-dispatch.test.ts | 141 ++++++ test/scripts/pr-operation-lock.test.ts | 100 ++++ test/scripts/pr-prepare-gates.test.ts | 40 +- .../pr-review-artifact-validation.test.ts | 45 ++ test/scripts/pr-wrappers.test.ts | 2 + 18 files changed, 1245 insertions(+), 327 deletions(-) create mode 100644 scripts/pr-lib/ci-dispatch.mjs create mode 100644 scripts/pr-lib/review-artifacts.mjs create mode 100644 test/scripts/pr-ci-dispatch.test.ts diff --git a/config/knip.config.ts b/config/knip.config.ts index 75a55c58c406..7dffa0d7caab 100644 --- a/config/knip.config.ts +++ b/config/knip.config.ts @@ -69,6 +69,8 @@ const repositoryScriptEntries = [ "scripts/oxlint-boundary-guards.mjs!", "scripts/plugin-prerelease-liveish-matrix.mjs!", "scripts/pr-gates-lock.mjs!", + "scripts/pr-lib/ci-dispatch.mjs!", + "scripts/pr-lib/review-artifacts.mjs!", "scripts/pr-lib/process-group-runner.mjs!", "scripts/pre-commit/filter-staged-files.mjs!", "scripts/qa-coverage-report.ts!", diff --git a/scripts/AGENTS.md b/scripts/AGENTS.md index 033c89b4ae17..b9e815d83ab9 100644 --- a/scripts/AGENTS.md +++ b/scripts/AGENTS.md @@ -20,7 +20,7 @@ This directory owns local tooling, script wrappers, and generated-artifact helpe ## PR Prepare Gates -- `scripts/pr` serializes review, prepare, and merge operations per PR across linked worktrees; `scripts/pr gc` skips active or indeterminate locks. A successful command return is the trusted synchronous-completion contract: every PR-state-mutating child must be joined before returning, and such work must never daemonize or explicitly escape both the operation group and lock-notification FD. Failed, interrupted, or controller-lost operations stay locked because detached children cannot be disproved; after verifying no child tools remain, use the reported exact-OID `scripts/pr lock-recover` command. Never bypass or delete these refs manually. +- `scripts/pr` serializes review, prepare, and merge operations per PR across linked worktrees; `scripts/pr gc` skips active or indeterminate locks. A successful command return is the trusted synchronous-completion contract: every PR-state-mutating child must be joined before returning, and such work must never daemonize or explicitly escape both the operation group and lock-notification FD. A failed command auto-releases only while its explicit pre-side-effect validation marker remains active; failures after mutation/tool launch, interruptions, and controller loss stay locked because detached children cannot be disproved. After verifying no child tools remain, use the reported exact-OID `scripts/pr lock-recover` command. Never bypass or delete these refs manually. - `scripts/pr prepare-gates` holds the heavy-check lock for its whole local gate block (`scripts/pr-gates-lock.mjs`), so concurrent gate runs across `.worktrees` queue as units instead of dying on child lock timeouts or vitest no-output watchdog kills. - `OPENCLAW_PR_GATES_REMOTE=testbox` runs the full-suite `pnpm test` gate on a Blacksmith Testbox through `scripts/crabbox-wrapper.mjs` (same delegation as `check:changed`); `pnpm build`/`pnpm check` stay local. The `tbx_` lease id and Actions run URL land in `.local/gates.env` (`REMOTE_GATES_*`) and `.local/prep.md`. Use it for reviewed trusted code when a loaded host makes the local 88-shard run stall-kill; contributor/fork code stays on secretless CI or sanitized AWS unless a maintainer explicitly approves credentialed execution. diff --git a/scripts/committer b/scripts/committer index 1fbf4eac22ee..d21942820d09 100755 --- a/scripts/committer +++ b/scripts/committer @@ -6,9 +6,9 @@ set -f usage() { local exit_code=${1:-2} if [ "$exit_code" -eq 0 ]; then - printf 'Usage: %s [--force] [--fast] "commit message" "file" ["file" ...]\n' "$(basename "$0")" + printf 'Usage: %s [--force] [--fast] [--no-verify-formatted] "commit message" "file" ["file" ...]\n' "$(basename "$0")" else - printf 'Usage: %s [--force] [--fast] "commit message" "file" ["file" ...]\n' "$(basename "$0")" >&2 + printf 'Usage: %s [--force] [--fast] [--no-verify-formatted] "commit message" "file" ["file" ...]\n' "$(basename "$0")" >&2 fi exit "$exit_code" } @@ -23,6 +23,7 @@ fi force_delete_lock=false fast_commit=false +no_verify_formatted=false while [[ "${1:-}" == --* ]]; do case "${1:-}" in --force) @@ -33,6 +34,10 @@ while [[ "${1:-}" == --* ]]; do fast_commit=true shift ;; + --no-verify-formatted) + no_verify_formatted=true + shift + ;; --help|-h) usage 0 ;; @@ -207,6 +212,51 @@ for file in "${files[@]}"; do fi done +committer_requires_repo_formatter() { + local root="$1" + local filter="$root/scripts/pre-commit/filter-staged-files.mjs" + [ -f "$filter" ] || return 1 + + local filter_output selected + filter_output=$(mktemp "${TMPDIR:-/tmp}/openclaw-committer-filter.XXXXXX") || { + echo "Unable to create temporary output for formatter applicability check." >&2 + return 2 + } + if ! node "$filter" format -- "${files[@]}" >"$filter_output"; then + rm -f "$filter_output" + echo "Unable to determine formatter applicability: filter-staged-files.mjs failed." >&2 + return 2 + fi + if IFS= read -r -d '' selected <"$filter_output"; then + rm -f "$filter_output" + return 0 + fi + rm -f "$filter_output" + return 1 +} + +repo_root=$(git rev-parse --show-toplevel) +if [ "$fast_commit" = false ] && [ "$no_verify_formatted" = false ] && \ + [ -f "$repo_root/pnpm-lock.yaml" ] && \ + command -v pnpm >/dev/null 2>&1 && \ + [ ! -e "$repo_root/node_modules" ] +then + if committer_requires_repo_formatter "$repo_root"; then + formatter_requirement=0 + else + formatter_requirement=$? + fi + if [ "$formatter_requirement" = 2 ]; then + exit 1 + fi + if [ "$formatter_requirement" = 0 ]; then + echo "Missing repo dependencies: cannot run oxfmt without node_modules." >&2 + echo "Run pnpm install in a normal checkout, or bypass the hook only after separate formatting proof." >&2 + echo "After that proof, pass --no-verify-formatted to commit explicitly with --no-verify." >&2 + exit 1 + fi +fi + run_git_with_lock_retry "unstaging files" git restore --staged :/ run_git_with_lock_retry "staging files" git add --all --force -- "${files[@]}" @@ -221,6 +271,11 @@ if [ "$fast_commit" = true ]; then if run_git_with_lock_retry "commit" env "${commit_env[@]}" git commit --no-verify -m "$commit_message"; then committed=true fi +elif [ "$no_verify_formatted" = true ]; then + echo "Notice: --no-verify-formatted asserts separate formatting proof; committing with --no-verify." + if run_git_with_lock_retry "commit" git commit --no-verify -m "$commit_message"; then + committed=true + fi else if run_git_with_lock_retry "commit" git commit -m "$commit_message"; then committed=true diff --git a/scripts/pr b/scripts/pr index 374d8347653f..7566cbca5fd9 100755 --- a/scripts/pr +++ b/scripts/pr @@ -70,7 +70,7 @@ fi is_locked_pr_command() { case "$1" in - review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | review-tests | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | merge-verify | merge-run) return 0 ;; + review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | review-tests | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | ci-dispatch | merge-verify | merge-run) return 0 ;; *) return 1 ;; esac } @@ -124,6 +124,7 @@ Usage: scripts/pr prepare-push scripts/pr prepare-sync-head scripts/pr prepare-run + scripts/pr ci-dispatch scripts/pr merge-verify scripts/pr merge-run OPENCLAW_PR_MERGE_METHOD=merge|rebase preserves the PR commit series. @@ -213,7 +214,7 @@ main() { review-tests) [ "$#" -ge 2 ] || { usage; exit 2; } ;; - review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | merge-verify | merge-run) + review-init | review-checkout-main | review-checkout-pr | review-claim | review-guard | review-artifacts-init | review-validate-artifacts | prepare-init | prepare-validate-commit | prepare-gates | prepare-push | prepare-sync-head | prepare-run | ci-dispatch | merge-verify | merge-run) [ "$#" -ge 1 ] || { usage; exit 2; } ;; *) @@ -231,6 +232,7 @@ main() { if is_locked_pr_command "$cmd"; then local locked_pr="${1-}" acquire_pr_operation_lock "$locked_pr" + begin_pr_operation_validation_phase trap 'exit 129' HUP trap 'exit 130' INT trap 'exit 131' QUIT @@ -319,6 +321,11 @@ main() { [ -n "$pr" ] || { usage; exit 2; } prepare_run "$pr" ;; + ci-dispatch) + local pr="${1-}" + [ -n "$pr" ] || { usage; exit 2; } + ci_dispatch "$pr" + ;; merge-verify) local pr="${1-}" [ -n "$pr" ] || { usage; exit 2; } diff --git a/scripts/pr-lib/ci-dispatch.mjs b/scripts/pr-lib/ci-dispatch.mjs new file mode 100644 index 000000000000..2f380e1062f9 --- /dev/null +++ b/scripts/pr-lib/ci-dispatch.mjs @@ -0,0 +1,165 @@ +#!/usr/bin/env node + +import { isDirectRunUrl } from "../lib/direct-run.mjs"; +import { execPlainGh } from "../lib/plain-gh.mjs"; + +const SHA_PATTERN = /^(?:[0-9a-f]{40}|[0-9a-f]{64})$/u; + +function requirePrRecord({ pr, headRefName, headRefOid, isCrossRepository }) { + if (!Number.isSafeInteger(pr) || pr <= 0) { + throw new Error("Expected a positive PR number."); + } + if (typeof headRefName !== "string" || headRefName.length === 0 || headRefName.startsWith("-")) { + throw new Error("Expected a non-empty PR headRefName."); + } + if (!SHA_PATTERN.test(headRefOid)) { + throw new Error("Expected a full PR headRefOid."); + } + if (isCrossRepository === true) { + throw new Error( + `PR #${pr} comes from a fork; release-gate workflow dispatch requires a branch in the base repository at ${headRefOid}.`, + ); + } +} + +function buildCiDispatchArgs(record) { + requirePrRecord(record); + return [ + "workflow", + "run", + "ci.yml", + "--ref", + record.headRefName, + "-f", + `target_ref=${record.headRefOid}`, + "-f", + "release_gate=true", + "-f", + `pull_request_number=${record.pr}`, + ]; +} + +function listCiRuns(headRefOid) { + return JSON.parse( + execPlainGh( + [ + "run", + "list", + "--commit", + headRefOid, + "--workflow", + "ci.yml", + "--event", + "workflow_dispatch", + "--limit", + "20", + "--json", + "databaseId,url,headSha,createdAt,status", + ], + { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }, + ), + ); +} + +function readCurrentPrHeadOid(pr) { + return execPlainGh(["pr", "view", String(pr), "--json", "headRefOid", "--jq", ".headRefOid"], { + encoding: "utf8", + stdio: ["ignore", "pipe", "pipe"], + }).trim(); +} + +function delay(milliseconds) { + return new Promise((resolve) => { + setTimeout(resolve, milliseconds); + }); +} + +async function dispatchCiForPr( + record, + { + pollAttempts = 10, + pollIntervalMs = 1500, + listRuns = listCiRuns, + runDispatch = (args) => + execPlainGh(args, { encoding: "utf8", stdio: ["ignore", "pipe", "pipe"] }), + readHeadOid = readCurrentPrHeadOid, + wait = delay, + } = {}, +) { + requirePrRecord(record); + const priorRunIds = new Set(listRuns(record.headRefOid).map((run) => run.databaseId)); + const headBeforeDispatch = readHeadOid(record.pr); + if (headBeforeDispatch !== record.headRefOid) { + throw new Error( + `PR #${record.pr} head changed before CI dispatch (expected ${record.headRefOid}, got ${headBeforeDispatch}).`, + ); + } + runDispatch(buildCiDispatchArgs(record)); + + for (let attempt = 1; attempt <= pollAttempts; attempt += 1) { + const run = listRuns(record.headRefOid).find( + (candidate) => + candidate.headSha === record.headRefOid && + !priorRunIds.has(candidate.databaseId) && + typeof candidate.url === "string" && + candidate.url.length > 0, + ); + if (run) { + const headAtObservation = readHeadOid(record.pr); + if (headAtObservation !== record.headRefOid) { + throw new Error( + `PR #${record.pr} head changed before an exact-SHA CI run became visible (expected ${record.headRefOid}, got ${headAtObservation}); verify the run before retrying.`, + ); + } + return run; + } + if (attempt < pollAttempts) { + await wait(pollIntervalMs); + } + } + const headAfterDispatch = readHeadOid(record.pr); + if (headAfterDispatch !== record.headRefOid) { + throw new Error( + `PR #${record.pr} head changed while CI dispatch was being indexed (expected ${record.headRefOid}, got ${headAfterDispatch}); verify the run before retrying.`, + ); + } + return undefined; +} + +async function main(argv = process.argv.slice(2)) { + if (argv.length !== 4 || !["true", "false"].includes(argv[3])) { + console.error("Usage: ci-dispatch.mjs "); + process.exitCode = 2; + return; + } + const record = { + pr: Number(argv[0]), + headRefName: argv[1], + headRefOid: argv[2], + isCrossRepository: argv[3] === "true", + }; + const run = await dispatchCiForPr(record); + if (run) { + console.log( + `GitHub accepted CI dispatch for PR #${record.pr} at unchanged remote head ${record.headRefOid} (${record.headRefName}).`, + ); + console.log( + "Observed a new exact-SHA manual run after dispatch; GitHub does not expose a dispatch correlation ID, so concurrent requests cannot be distinguished.", + ); + console.log(`observed_run_url=${run.url}`); + } else { + console.log( + `Requested CI for PR #${record.pr} at unchanged remote head ${record.headRefOid} (${record.headRefName}).`, + ); + console.log( + "run_url=pending (GitHub accepted the dispatch, but Actions has not indexed it yet)", + ); + console.log( + `inspect_with=gh run list --commit ${record.headRefOid} --workflow ci.yml --event workflow_dispatch`, + ); + } +} + +if (isDirectRunUrl(process.argv[1], import.meta.url)) { + await main(); +} diff --git a/scripts/pr-lib/gates.sh b/scripts/pr-lib/gates.sh index 7a481b875986..861b945b47ad 100644 --- a/scripts/pr-lib/gates.sh +++ b/scripts/pr-lib/gates.sh @@ -3,8 +3,11 @@ run_hosted_prepare_gates() { local current_head="$2" local changelog_only="$3" local recent_sha="" - local remote_head - remote_head=$(gh pr view "$pr" --json headRefOid --jq .headRefOid) + local remote_record remote_head remote_head_ref remote_is_cross_repository + remote_record=$(gh pr view "$pr" --json headRefName,headRefOid,isCrossRepository) + remote_head=$(printf '%s\n' "$remote_record" | jq -r .headRefOid) + remote_head_ref=$(printf '%s\n' "$remote_record" | jq -r .headRefName) + remote_is_cross_repository=$(printf '%s\n' "$remote_record" | jq -r .isCrossRepository) if [ "$remote_head" != "$current_head" ]; then echo "PR head changed before hosted gate verification (expected $current_head, got $remote_head). Re-run prepare-init." return 1 @@ -40,7 +43,60 @@ run_hosted_prepare_gates() { if [ "$changelog_only" = "true" ]; then args+=(--changelog-only) fi - run_quiet_logged "hosted CI/Testbox gates" ".local/gates-hosted-checks.log" node "${args[@]}" + if run_quiet_logged "hosted CI/Testbox gates" ".local/gates-hosted-checks.log" node "${args[@]}"; then + return 0 + fi + + if rg -F -q "Missing successful recent CI workflow for $current_head. Observed: none" \ + .local/gates-hosted-checks.log + then + if [ "$remote_is_cross_repository" = "true" ]; then + cat <&2 + return 1 + fi + if [ "$is_cross_repository" = "true" ]; then + echo "PR #$pr comes from a fork; release-gate workflow dispatch requires a base-repository branch at $head_sha." >&2 + return 1 + fi + + mark_pr_operation_side_effects_if_available + node "$script_parent_dir/pr-lib/ci-dispatch.mjs" "$pr" "$head_ref" "$head_sha" false +} + +mark_pr_operation_side_effects_if_available() { + # scripts/pr sources operation-lock.sh first. Policy tests may source this + # library alone, where advancing a lock phase is neither possible nor needed. + if declare -F mark_pr_operation_side_effects_started >/dev/null; then + mark_pr_operation_side_effects_started + fi } pin_worktree_bundled_plugins_dir() { @@ -313,6 +369,7 @@ prepare_gates() { enter_worktree "$pr" false + mark_pr_operation_side_effects_if_available checkout_prep_branch "$pr" require_artifact .local/pr-meta.env # shellcheck disable=SC1091 diff --git a/scripts/pr-lib/merge.sh b/scripts/pr-lib/merge.sh index 79e1481c613c..bf9c9c7fe3ad 100644 --- a/scripts/pr-lib/merge.sh +++ b/scripts/pr-lib/merge.sh @@ -115,6 +115,7 @@ merge_verify() { echo "Re-run prepare to refresh prep artifacts and gates: scripts/pr-prepare run $pr" echo "Note: docs/changelog-only follow-ups reuse prior gate results automatically." + mark_pr_operation_side_effects_started git fetch origin "pull/$pr/head" >/dev/null 2>&1 || true if git cat-file -e "${PREP_HEAD_SHA}^{commit}" 2>/dev/null && git cat-file -e "${pr_head_sha}^{commit}" 2>/dev/null; then echo "HEAD delta (expected...current):" @@ -125,6 +126,7 @@ merge_verify() { exit 1 fi + mark_pr_operation_side_effects_started gh pr checks "$pr" --required --watch --fail-fast >.local/merge-checks-watch.log 2>&1 || true local checks_json local checks_err_file diff --git a/scripts/pr-lib/operation-lock.sh b/scripts/pr-lib/operation-lock.sh index e2a1de6c5df1..e4586547ea12 100644 --- a/scripts/pr-lib/operation-lock.sh +++ b/scripts/pr-lib/operation-lock.sh @@ -5,6 +5,9 @@ PR_OPERATION_LOCK_CANDIDATE_PR="" PR_OPERATION_LOCK_CANDIDATE_OID="" PR_OPERATION_LOCK_BLOCKED_OID="" PR_OPERATION_LOCK_BLOCKED_REASON="" +# This is monotonic for one supervised command. Once side effects begin, a +# descendant must not be able to reopen the auto-release validation window. +PR_OPERATION_VALIDATION_PHASE_STATE=unannounced is_canonical_pr_number() { local pr="$1" @@ -121,6 +124,32 @@ clear_pr_operation_lock_state() { PR_OPERATION_LOCK_CANDIDATE_OID="" PR_OPERATION_LOCK_BLOCKED_OID="" PR_OPERATION_LOCK_BLOCKED_REASON="" + PR_OPERATION_VALIDATION_PHASE_STATE=unannounced +} + +notify_pr_operation_phase() { + local phase="$1" + if [ -z "${OPENCLAW_PR_LOCK_NOTIFY_FD:-}" ]; then + return 0 + fi + case "$OPENCLAW_PR_LOCK_NOTIFY_FD" in ''|*[!0-9]*) return 1 ;; esac + printf 'phase\t%s\n' "$phase" >&"$OPENCLAW_PR_LOCK_NOTIFY_FD" +} + +begin_pr_operation_validation_phase() { + if [ "$PR_OPERATION_VALIDATION_PHASE_STATE" != "unannounced" ]; then + return 0 + fi + notify_pr_operation_phase validation-started || return 1 + PR_OPERATION_VALIDATION_PHASE_STATE=validation +} + +mark_pr_operation_side_effects_started() { + if [ "$PR_OPERATION_VALIDATION_PHASE_STATE" != "validation" ]; then + return 0 + fi + notify_pr_operation_phase side-effects-started || return 1 + PR_OPERATION_VALIDATION_PHASE_STATE=side_effects } pr_operation_lock_owner_is_current() { diff --git a/scripts/pr-lib/prepare-core.sh b/scripts/pr-lib/prepare-core.sh index 77343690134d..21f313ffccc0 100644 --- a/scripts/pr-lib/prepare-core.sh +++ b/scripts/pr-lib/prepare-core.sh @@ -49,6 +49,7 @@ verify_prep_branch_matches_prepared_head() { prepare_init() { local pr="$1" + mark_pr_operation_side_effects_started enter_worktree "$pr" true require_artifact .local/pr-meta.env @@ -105,6 +106,7 @@ prepare_validate_commit() { enter_worktree "$pr" false require_artifact .local/pr-meta.env + mark_pr_operation_side_effects_started checkout_prep_branch "$pr" # shellcheck disable=SC1091 @@ -135,6 +137,7 @@ prepare_push() { require_artifact .local/prep-context.env require_artifact .local/gates.env + mark_pr_operation_side_effects_started checkout_prep_branch "$pr" # shellcheck disable=SC1091 @@ -222,6 +225,7 @@ prepare_sync_head() { require_artifact .local/pr-meta.env require_artifact .local/prep-context.env + mark_pr_operation_side_effects_started checkout_prep_branch "$pr" # shellcheck disable=SC1091 diff --git a/scripts/pr-lib/process-group-runner.mjs b/scripts/pr-lib/process-group-runner.mjs index 05ee3223bc6b..0f0db3055128 100644 --- a/scripts/pr-lib/process-group-runner.mjs +++ b/scripts/pr-lib/process-group-runner.mjs @@ -53,6 +53,7 @@ let lingeringGroupProcesses = []; let drainFailure; let drainFailureGroupStatus; let drainFailureNotificationOpen = false; +let validationPhaseState = "unannounced"; function delay(ms) { return new Promise((resolveDelay) => { @@ -172,6 +173,18 @@ if (killDeadline) { } function consumeNotificationLine(line) { + if (line === "phase\tvalidation-started") { + // The FD is inherited by descendants, so phase messages are monotonic: + // no later writer may reopen validation after side effects have started. + if (validationPhaseState === "unannounced") { + validationPhaseState = "validation"; + } + return; + } + if (line === "phase\tside-effects-started") { + validationPhaseState = "side-effects"; + return; + } const [lockRef, ownerOid, extra] = line.split("\t"); if ( extra !== undefined || @@ -445,17 +458,29 @@ for (const [signal, handler] of signalHandlers) { } // PR commands must join all state-mutating children before returning. A clean -// exit is that trusted completion signal; abnormal exits retain the lock because -// an escaped child can outlive both the recorded group and notification pipe. +// exit is the normal completion signal. A nonzero exit may also release while +// the child explicitly remains in its pre-side-effect validation phase; every +// other abnormal exit retains because an escaped child can outlive the group. const completedCleanly = childResult.code === 0 && !receivedSignal && !childResult.signal && !notificationFailure && !hadLingeringGroup; +const failedDuringValidation = + validationPhaseState === "validation" && + childResult.code !== null && + childResult.code > 0 && + // Shells encode signal termination as 128+signal. Retain conservatively for + // every such status, including signals scripts/pr does not trap itself. + childResult.code < 128 && + !receivedSignal && + !childResult.signal && + !notificationFailure && + !hadLingeringGroup; const retainedLocks = []; const releaseFailures = new Set(); -if (drained && completedCleanly) { +if (drained && (completedCleanly || failedDuringValidation)) { for (const lock of locks.values()) { try { releaseLock(lock); diff --git a/scripts/pr-lib/review-artifacts.mjs b/scripts/pr-lib/review-artifacts.mjs new file mode 100644 index 000000000000..f51711f97d84 --- /dev/null +++ b/scripts/pr-lib/review-artifacts.mjs @@ -0,0 +1,474 @@ +#!/usr/bin/env node + +import { readFileSync } from "node:fs"; +import { isDirectRunUrl } from "../lib/direct-run.mjs"; + +const REVIEW_ARTIFACT_ENUMS = Object.freeze({ + recommendation: Object.freeze([ + "READY FOR /prepare-pr", + "NEEDS WORK", + "NEEDS DISCUSSION", + "NOT USEFUL (CLOSE)", + ]), + findingSeverity: Object.freeze(["BLOCKER", "IMPORTANT", "NIT"]), + nitSweepStatus: Object.freeze(["none", "has_nits"]), + issueValidationSource: Object.freeze(["linked_issue", "pr_body", "both"]), + issueValidationStatus: Object.freeze(["valid", "unclear", "invalid", "already_fixed_on_main"]), + behavioralSweepStatus: Object.freeze(["pass", "needs_work", "not_applicable"]), + behavioralSweepRisk: Object.freeze(["none", "present", "unknown"]), + testsResult: Object.freeze(["pass", "fail", "not_run"]), + docs: Object.freeze(["up_to_date", "missing", "not_applicable"]), + changelog: Object.freeze(["required", "not_required"]), +}); + +function reviewArtifactEnumHint(enumName, initialValue) { + const allowed = REVIEW_ARTIFACT_ENUMS[enumName]; + if (!allowed?.includes(initialValue)) { + throw new Error(`Invalid initial value ${initialValue} for review enum ${enumName}.`); + } + return `${initialValue} (allowed: ${allowed.join("|")})`; +} + +function createReviewArtifactTemplate() { + return { + recommendation: reviewArtifactEnumHint("recommendation", "NEEDS WORK"), + findings: [], + nitSweep: { + performed: true, + status: reviewArtifactEnumHint("nitSweepStatus", "none"), + summary: "No optional nits identified.", + }, + behavioralSweep: { + performed: true, + status: reviewArtifactEnumHint("behavioralSweepStatus", "not_applicable"), + summary: "No runtime branch-level behavior changes require sweep evidence.", + silentDropRisk: reviewArtifactEnumHint("behavioralSweepRisk", "none"), + branches: [], + }, + issueValidation: { + performed: true, + source: reviewArtifactEnumHint("issueValidationSource", "pr_body"), + status: reviewArtifactEnumHint("issueValidationStatus", "unclear"), + summary: "Review not completed yet.", + }, + tests: { + ran: [], + gaps: [], + result: reviewArtifactEnumHint("testsResult", "pass"), + }, + docs: reviewArtifactEnumHint("docs", "not_applicable"), + changelog: reviewArtifactEnumHint("changelog", "not_required"), + }; +} + +function isObject(value) { + return value !== null && typeof value === "object" && !Array.isArray(value); +} + +function isNonEmptyString(value) { + return typeof value === "string" && value.trim().length > 0; +} + +function jsonValue(value) { + return JSON.stringify(value === undefined ? null : value); +} + +function validateReviewArtifacts({ review, reviewMarkdown, prMeta }) { + const violations = []; + const add = (message) => { + if (!violations.includes(message)) { + violations.push(message); + } + }; + const requireType = (valid, message) => { + if (!valid) { + add(message); + } + return valid; + }; + const requireEnum = (value, enumName, messagePrefix) => { + const allowed = REVIEW_ARTIFACT_ENUMS[enumName]; + if (!allowed.includes(value)) { + add(`${messagePrefix}: ${jsonValue(value)} (allowed: ${allowed.join("|")})`); + return false; + } + return true; + }; + + const reviewIsObject = requireType( + isObject(review), + "Invalid .local/review.json: top-level value must be an object", + ); + const value = reviewIsObject ? review : {}; + const recommendationIsString = requireType( + typeof value.recommendation === "string", + "Invalid recommendation in .local/review.json: recommendation must be a string", + ); + const findingsAreArray = requireType( + Array.isArray(value.findings), + "Invalid findings in .local/review.json: findings must be an array", + ); + const findings = findingsAreArray ? value.findings : []; + requireType( + findings.every(isObject), + "Invalid finding entry in .local/review.json: each finding must be an object", + ); + const nitSweepIsObject = requireType( + isObject(value.nitSweep), + "Invalid nit sweep in .local/review.json: nitSweep must be an object", + ); + const issueValidationIsObject = requireType( + isObject(value.issueValidation), + "Invalid issue validation in .local/review.json: issueValidation must be an object", + ); + const behavioralSweepIsObject = requireType( + isObject(value.behavioralSweep), + "Invalid behavioral sweep in .local/review.json: behavioralSweep must be an object", + ); + const testsIsObject = requireType( + isObject(value.tests), + "Invalid tests in .local/review.json: tests must be an object", + ); + + for (const section of ["A)", "B)", "C)", "D)", "E)", "F)", "G)", "H)", "I)", "J)"]) { + if (!reviewMarkdown.split("\n").some((line) => line.startsWith(section))) { + add(`Missing section header in .local/review.md: ${section}`); + } + } + + if (recommendationIsString) { + requireEnum( + value.recommendation, + "recommendation", + "Invalid recommendation in .local/review.json", + ); + } + + const invalidSeverity = findings.find( + (finding) => + isObject(finding) && !REVIEW_ARTIFACT_ENUMS.findingSeverity.includes(finding.severity), + ); + if (invalidSeverity) { + add( + `Invalid finding severity in .local/review.json: ${jsonValue(invalidSeverity.severity)} (allowed: ${REVIEW_ARTIFACT_ENUMS.findingSeverity.join("|")})`, + ); + } + if ( + findings.some( + (finding) => + !isObject(finding) || + typeof finding.id !== "string" || + typeof finding.title !== "string" || + typeof finding.area !== "string" || + typeof finding.fix !== "string", + ) + ) { + add("Invalid finding shape in .local/review.json (id/title/area/fix must be strings)"); + } + const nitFindingsCount = findings.filter( + (finding) => isObject(finding) && finding.severity === "NIT", + ).length; + + const nitSweep = nitSweepIsObject ? value.nitSweep : {}; + const nitSweepPerformedIsBoolean = requireType( + typeof nitSweep.performed === "boolean", + "Invalid nit sweep in .local/review.json: nitSweep.performed must be a boolean", + ); + if (nitSweepPerformedIsBoolean && nitSweep.performed !== true) { + add("Invalid nit sweep in .local/review.json: nitSweep.performed must be true"); + } + const nitSweepStatusIsString = requireType( + typeof nitSweep.status === "string", + "Invalid nit sweep status in .local/review.json: nitSweep.status must be a string", + ); + if (nitSweepStatusIsString) { + const validStatus = requireEnum( + nitSweep.status, + "nitSweepStatus", + "Invalid nit sweep status in .local/review.json", + ); + if (validStatus && nitSweep.status === "none" && nitFindingsCount > 0) { + add( + "Invalid nit sweep in .local/review.json: nitSweep.status is none but NIT findings exist", + ); + } + if (validStatus && nitSweep.status === "has_nits" && nitFindingsCount < 1) { + add( + "Invalid nit sweep in .local/review.json: nitSweep.status is has_nits but no NIT findings exist", + ); + } + } + requireType( + typeof nitSweep.summary === "string", + "Invalid nit sweep summary in .local/review.json: nitSweep.summary must be a string", + ); + if (typeof nitSweep.summary === "string" && !isNonEmptyString(nitSweep.summary)) { + add( + "Invalid nit sweep summary in .local/review.json: nitSweep.summary must be a non-empty string", + ); + } + + const issueValidation = issueValidationIsObject ? value.issueValidation : {}; + const issuePerformedIsBoolean = requireType( + typeof issueValidation.performed === "boolean", + "Invalid issue validation in .local/review.json: issueValidation.performed must be a boolean", + ); + if (issuePerformedIsBoolean && issueValidation.performed !== true) { + add("Invalid issue validation in .local/review.json: issueValidation.performed must be true"); + } + const issueSourceIsString = requireType( + typeof issueValidation.source === "string", + "Invalid issue validation source in .local/review.json: issueValidation.source must be a string", + ); + if (issueSourceIsString) { + requireEnum( + issueValidation.source, + "issueValidationSource", + "Invalid issue validation source in .local/review.json", + ); + } + const issueStatusIsString = requireType( + typeof issueValidation.status === "string", + "Invalid issue validation status in .local/review.json: issueValidation.status must be a string", + ); + if (issueStatusIsString) { + requireEnum( + issueValidation.status, + "issueValidationStatus", + "Invalid issue validation status in .local/review.json", + ); + } + requireType( + typeof issueValidation.summary === "string", + "Invalid issue validation summary in .local/review.json: issueValidation.summary must be a string", + ); + if (typeof issueValidation.summary === "string" && !isNonEmptyString(issueValidation.summary)) { + add( + "Invalid issue validation summary in .local/review.json: issueValidation.summary must be a non-empty string", + ); + } + + const prMetaIsValid = + isObject(prMeta) && + Array.isArray(prMeta.files) && + prMeta.files.every((file) => isObject(file) && typeof file.path === "string"); + if (!prMetaIsValid) { + add("Invalid .local/pr-meta.json: files must be an array of objects with string path"); + } + const runtimeFileCount = prMetaIsValid + ? prMeta.files.filter( + ({ path }) => + /^(src|extensions|apps)\//u.test(path) && + !/(^|\/)__tests__\/|\.test\.|\.spec\./u.test(path) && + !/\.(md|mdx)$/u.test(path), + ).length + : 0; + const runtimeReviewRequired = runtimeFileCount > 0; + + const behavioralSweep = behavioralSweepIsObject ? value.behavioralSweep : {}; + const behavioralPerformedIsBoolean = requireType( + typeof behavioralSweep.performed === "boolean", + "Invalid behavioral sweep in .local/review.json: behavioralSweep.performed must be a boolean", + ); + if (behavioralPerformedIsBoolean && behavioralSweep.performed !== true) { + add("Invalid behavioral sweep in .local/review.json: behavioralSweep.performed must be true"); + } + const behavioralStatusIsString = requireType( + typeof behavioralSweep.status === "string", + "Invalid behavioral sweep status in .local/review.json: behavioralSweep.status must be a string", + ); + const behavioralStatusIsValid = + behavioralStatusIsString && + requireEnum( + behavioralSweep.status, + "behavioralSweepStatus", + "Invalid behavioral sweep status in .local/review.json", + ); + const behavioralRiskIsString = requireType( + typeof behavioralSweep.silentDropRisk === "string", + "Invalid behavioral sweep risk in .local/review.json: behavioralSweep.silentDropRisk must be a string", + ); + const behavioralRiskIsValid = + behavioralRiskIsString && + requireEnum( + behavioralSweep.silentDropRisk, + "behavioralSweepRisk", + "Invalid behavioral sweep risk in .local/review.json", + ); + requireType( + typeof behavioralSweep.summary === "string", + "Invalid behavioral sweep summary in .local/review.json: behavioralSweep.summary must be a string", + ); + if (typeof behavioralSweep.summary === "string" && !isNonEmptyString(behavioralSweep.summary)) { + add( + "Invalid behavioral sweep summary in .local/review.json: behavioralSweep.summary must be a non-empty string", + ); + } + const branchesAreArray = Array.isArray(behavioralSweep.branches); + if (!branchesAreArray) { + add( + "Invalid behavioral sweep in .local/review.json: behavioralSweep.branches must be an array", + ); + } + const branches = branchesAreArray ? behavioralSweep.branches : []; + if ( + branches.some( + (branch) => + !isObject(branch) || + typeof branch.path !== "string" || + typeof branch.decision !== "string" || + typeof branch.outcome !== "string", + ) + ) { + add( + "Invalid behavioral sweep branch entry in .local/review.json: each entry must be an object with string path/decision/outcome", + ); + } + + if ( + behavioralStatusIsValid && + runtimeReviewRequired && + behavioralSweep.status === "not_applicable" + ) { + add( + "Invalid behavioral sweep in .local/review.json: runtime file changes require behavioralSweep.status=pass|needs_work", + ); + } + if (runtimeReviewRequired && branches.length < 1) { + add( + "Invalid behavioral sweep in .local/review.json: runtime file changes require at least one branch entry", + ); + } + if ( + behavioralStatusIsValid && + behavioralSweep.status === "not_applicable" && + branches.length > 0 + ) { + add( + "Invalid behavioral sweep in .local/review.json: not_applicable cannot include branch entries", + ); + } + if ( + behavioralStatusIsValid && + behavioralRiskIsValid && + behavioralSweep.status === "pass" && + behavioralSweep.silentDropRisk !== "none" + ) { + add("Invalid behavioral sweep in .local/review.json: status=pass requires silentDropRisk=none"); + } + + if (value.recommendation === "READY FOR /prepare-pr" && issueValidation.status !== "valid") { + add( + "Invalid recommendation in .local/review.json: READY FOR /prepare-pr requires issueValidation.status=valid", + ); + } + if (value.recommendation === "READY FOR /prepare-pr" && behavioralSweep.status === "needs_work") { + add( + "Invalid recommendation in .local/review.json: READY FOR /prepare-pr requires behavioralSweep.status!=needs_work", + ); + } + if ( + value.recommendation === "READY FOR /prepare-pr" && + runtimeReviewRequired && + behavioralSweep.status !== "pass" + ) { + add( + "Invalid recommendation in .local/review.json: READY FOR /prepare-pr on runtime changes requires behavioralSweep.status=pass", + ); + } + if ( + value.recommendation === "READY FOR /prepare-pr" && + behavioralSweep.silentDropRisk === "present" + ) { + add( + "Invalid recommendation in .local/review.json: READY FOR /prepare-pr is not allowed when behavioralSweep.silentDropRisk=present", + ); + } + + const tests = testsIsObject ? value.tests : {}; + const testsRanAreArray = requireType( + Array.isArray(tests.ran), + "Invalid tests in .local/review.json: tests.ran must be an array of strings", + ); + if (testsRanAreArray && !tests.ran.every((entry) => typeof entry === "string")) { + add("Invalid tests in .local/review.json: tests.ran must be an array of strings"); + } + const testsGapsAreArray = requireType( + Array.isArray(tests.gaps), + "Invalid tests in .local/review.json: tests.gaps must be an array of strings", + ); + if (testsGapsAreArray && !tests.gaps.every((entry) => typeof entry === "string")) { + add("Invalid tests in .local/review.json: tests.gaps must be an array of strings"); + } + const testsResultIsString = requireType( + typeof tests.result === "string", + "Invalid tests result in .local/review.json: tests.result must be a string", + ); + if (testsResultIsString) { + requireEnum(tests.result, "testsResult", "Invalid tests result in .local/review.json"); + } + + const docsIsString = requireType( + typeof value.docs === "string", + "Invalid docs status in .local/review.json: docs must be a string", + ); + if (docsIsString) { + requireEnum(value.docs, "docs", "Invalid docs status in .local/review.json"); + } + const changelogIsString = requireType( + typeof value.changelog === "string", + "Invalid changelog status in .local/review.json: changelog must be a string", + ); + if (changelogIsString) { + requireEnum(value.changelog, "changelog", "Invalid changelog status in .local/review.json"); + } + + return violations; +} + +function readJson(filePath) { + try { + return JSON.parse(readFileSync(filePath, "utf8")); + } catch (error) { + const message = error instanceof Error ? error.message : String(error); + throw new Error(`Invalid JSON in ${filePath}: ${message}`, { cause: error }); + } +} + +function main(argv = process.argv.slice(2)) { + const [command, ...args] = argv; + if (command === "template" && args.length === 0) { + process.stdout.write(`${JSON.stringify(createReviewArtifactTemplate(), null, 2)}\n`); + return; + } + if (command === "validate" && args.length === 3) { + const [reviewPath, reviewMarkdownPath, prMetaPath] = args; + const violations = validateReviewArtifacts({ + review: readJson(reviewPath), + reviewMarkdown: readFileSync(reviewMarkdownPath, "utf8"), + prMeta: readJson(prMetaPath), + }); + if (violations.length > 0) { + for (const violation of violations) { + console.log(violation); + } + console.log(`${violations.length} artifact violations`); + process.exitCode = 1; + } + return; + } + console.error( + "Usage: review-artifacts.mjs template | validate ", + ); + process.exitCode = 2; +} + +if (isDirectRunUrl(process.argv[1], import.meta.url)) { + try { + main(); + } catch (error) { + console.log(error instanceof Error ? error.message : String(error)); + console.log("1 artifact violations"); + process.exitCode = 1; + } +} diff --git a/scripts/pr-lib/review.sh b/scripts/pr-lib/review.sh index 6c9fd36db5e7..f77cee920081 100644 --- a/scripts/pr-lib/review.sh +++ b/scripts/pr-lib/review.sh @@ -7,8 +7,17 @@ set_review_mode() { > .local/review-mode.env } +review_artifacts_helper_path() { + local scripts_dir="${script_parent_dir:-}" + if [ -z "$scripts_dir" ]; then + scripts_dir=$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd) + fi + printf '%s/pr-lib/review-artifacts.mjs\n' "$scripts_dir" +} + review_claim() { local pr="$1" + mark_pr_operation_side_effects_started local root root=$(repo_root) cd "$root" @@ -64,6 +73,7 @@ review_claim() { review_checkout_main() { local pr="$1" enter_worktree "$pr" false + mark_pr_operation_side_effects_started git fetch origin main git checkout --detach origin/main set_review_mode main @@ -76,6 +86,7 @@ review_checkout_main() { review_checkout_pr() { local pr="$1" enter_worktree "$pr" false + mark_pr_operation_side_effects_started git fetch origin "pull/$pr/head:pr-$pr" --force git checkout --detach "pr-$pr" set_review_mode pr @@ -90,6 +101,7 @@ review_guard() { enter_worktree "$pr" false require_artifact .local/review-mode.env require_artifact .local/pr-meta.env + # shellcheck disable=SC1091 source .local/review-mode.env # shellcheck disable=SC1091 @@ -136,6 +148,8 @@ review_artifacts_init() { enter_worktree "$pr" false require_artifact .local/pr-meta.env + mark_pr_operation_side_effects_started + if [ ! -f .local/review.md ]; then cat > .local/review.md <<'EOF_MD' A) TL;DR recommendation @@ -161,52 +175,13 @@ EOF_MD fi if [ ! -f .local/review.json ]; then - cat > .local/review.json <<'EOF_JSON' -{ - "recommendation": "NEEDS WORK", - "findings": [], - "nitSweep": { - "performed": true, - "status": "none", - "summary": "No optional nits identified." - }, - "behavioralSweep": { - "performed": true, - "status": "not_applicable", - "summary": "No runtime branch-level behavior changes require sweep evidence.", - "silentDropRisk": "none", - "branches": [] - }, - "issueValidation": { - "performed": true, - "source": "pr_body", - "status": "unclear", - "summary": "Review not completed yet." - }, - "tests": { - "ran": [], - "gaps": [], - "result": "pass" - }, - "docs": "not_applicable", - "changelog": "not_required" -} -EOF_JSON + node "$(review_artifacts_helper_path)" template > .local/review.json fi echo "review artifact templates are ready" echo "files=.local/review.md .local/review.json" } -review_json_require() { - local expression="$1" - local message="$2" - if ! jq -e "$expression" .local/review.json >/dev/null 2>&1; then - echo "$message" - exit 1 - fi -} - review_validate_artifacts() { local pr="$1" enter_worktree "$pr" false @@ -217,281 +192,14 @@ review_validate_artifacts() { review_guard "$pr" - jq . .local/review.json >/dev/null - review_json_require 'type == "object"' "Invalid .local/review.json: top-level value must be an object" - review_json_require '(.recommendation | type) == "string"' "Invalid recommendation in .local/review.json: recommendation must be a string" - review_json_require '(.findings | type) == "array"' "Invalid findings in .local/review.json: findings must be an array" - review_json_require 'all(.findings[]; type == "object")' "Invalid finding entry in .local/review.json: each finding must be an object" - review_json_require '(.nitSweep | type) == "object"' "Invalid nit sweep in .local/review.json: nitSweep must be an object" - review_json_require '(.issueValidation | type) == "object"' "Invalid issue validation in .local/review.json: issueValidation must be an object" - review_json_require '(.behavioralSweep | type) == "object"' "Invalid behavioral sweep in .local/review.json: behavioralSweep must be an object" - review_json_require '(.tests | type) == "object"' "Invalid tests in .local/review.json: tests must be an object" - - local section - for section in "A)" "B)" "C)" "D)" "E)" "F)" "G)" "H)" "I)" "J)"; do - awk -v s="$section" 'index($0, s) == 1 { found=1; exit } END { exit(found ? 0 : 1) }' .local/review.md || { - echo "Missing section header in .local/review.md: $section" - exit 1 - } - done - - local recommendation - recommendation=$(jq -r '.recommendation // ""' .local/review.json) - case "$recommendation" in - "READY FOR /prepare-pr"|"NEEDS WORK"|"NEEDS DISCUSSION"|"NOT USEFUL (CLOSE)") - ;; - *) - printf 'Invalid recommendation in .local/review.json: %s (allowed: READY FOR /prepare-pr|NEEDS WORK|NEEDS DISCUSSION|NOT USEFUL (CLOSE))\n' "$(jq -c '.recommendation' .local/review.json)" - exit 1 - ;; - esac - - local invalid_severity_count - invalid_severity_count=$(jq '[.findings[]? | select((.severity // "") != "BLOCKER" and (.severity // "") != "IMPORTANT" and (.severity // "") != "NIT")] | length' .local/review.json) - if [ "$invalid_severity_count" -gt 0 ]; then - printf 'Invalid finding severity in .local/review.json: %s (allowed: BLOCKER|IMPORTANT|NIT)\n' "$(jq -c 'first(.findings[] | select((.severity // "") != "BLOCKER" and (.severity // "") != "IMPORTANT" and (.severity // "") != "NIT")).severity' .local/review.json)" - exit 1 + if ! node "$(review_artifacts_helper_path)" validate \ + .local/review.json \ + .local/review.md \ + .local/pr-meta.json + then + return 1 fi - local invalid_findings_count - invalid_findings_count=$(jq '[.findings[]? | select((.id|type)!="string" or (.title|type)!="string" or (.area|type)!="string" or (.fix|type)!="string")] | length' .local/review.json) - if [ "$invalid_findings_count" -gt 0 ]; then - echo "Invalid finding shape in .local/review.json (id/title/area/fix must be strings)" - exit 1 - fi - - local nit_findings_count - nit_findings_count=$(jq '[.findings[]? | select((.severity // "") == "NIT")] | length' .local/review.json) - - local nit_sweep_performed - review_json_require '(.nitSweep.performed | type) == "boolean"' "Invalid nit sweep in .local/review.json: nitSweep.performed must be a boolean" - nit_sweep_performed=$(jq -r '.nitSweep.performed // empty' .local/review.json) - if [ "$nit_sweep_performed" != "true" ]; then - echo "Invalid nit sweep in .local/review.json: nitSweep.performed must be true" - exit 1 - fi - - local nit_sweep_status - review_json_require '(.nitSweep.status | type) == "string"' "Invalid nit sweep status in .local/review.json: nitSweep.status must be a string" - nit_sweep_status=$(jq -r '.nitSweep.status // ""' .local/review.json) - case "$nit_sweep_status" in - "none") - if [ "$nit_findings_count" -gt 0 ]; then - echo "Invalid nit sweep in .local/review.json: nitSweep.status is none but NIT findings exist" - exit 1 - fi - ;; - "has_nits") - if [ "$nit_findings_count" -lt 1 ]; then - echo "Invalid nit sweep in .local/review.json: nitSweep.status is has_nits but no NIT findings exist" - exit 1 - fi - ;; - *) - printf 'Invalid nit sweep status in .local/review.json: %s (allowed: none|has_nits)\n' "$(jq -c '.nitSweep.status' .local/review.json)" - exit 1 - ;; - esac - - local invalid_nit_summary_count - review_json_require '(.nitSweep.summary | type) == "string"' "Invalid nit sweep summary in .local/review.json: nitSweep.summary must be a string" - invalid_nit_summary_count=$(jq '[.nitSweep.summary | select((type != "string") or (gsub("^\\s+|\\s+$";"") | length == 0))] | length' .local/review.json) - if [ "$invalid_nit_summary_count" -gt 0 ]; then - echo "Invalid nit sweep summary in .local/review.json: nitSweep.summary must be a non-empty string" - exit 1 - fi - - local issue_validation_performed - review_json_require '(.issueValidation.performed | type) == "boolean"' "Invalid issue validation in .local/review.json: issueValidation.performed must be a boolean" - issue_validation_performed=$(jq -r '.issueValidation.performed // empty' .local/review.json) - if [ "$issue_validation_performed" != "true" ]; then - echo "Invalid issue validation in .local/review.json: issueValidation.performed must be true" - exit 1 - fi - - local issue_validation_source - review_json_require '(.issueValidation.source | type) == "string"' "Invalid issue validation source in .local/review.json: issueValidation.source must be a string" - issue_validation_source=$(jq -r '.issueValidation.source // ""' .local/review.json) - case "$issue_validation_source" in - "linked_issue"|"pr_body"|"both") - ;; - *) - printf 'Invalid issue validation source in .local/review.json: %s (allowed: linked_issue|pr_body|both)\n' "$(jq -c '.issueValidation.source' .local/review.json)" - exit 1 - ;; - esac - - local issue_validation_status - review_json_require '(.issueValidation.status | type) == "string"' "Invalid issue validation status in .local/review.json: issueValidation.status must be a string" - issue_validation_status=$(jq -r '.issueValidation.status // ""' .local/review.json) - case "$issue_validation_status" in - "valid"|"unclear"|"invalid"|"already_fixed_on_main") - ;; - *) - printf 'Invalid issue validation status in .local/review.json: %s (allowed: valid|unclear|invalid|already_fixed_on_main)\n' "$(jq -c '.issueValidation.status' .local/review.json)" - exit 1 - ;; - esac - - local invalid_issue_summary_count - review_json_require '(.issueValidation.summary | type) == "string"' "Invalid issue validation summary in .local/review.json: issueValidation.summary must be a string" - invalid_issue_summary_count=$(jq '[.issueValidation.summary | select((type != "string") or (gsub("^\\s+|\\s+$";"") | length == 0))] | length' .local/review.json) - if [ "$invalid_issue_summary_count" -gt 0 ]; then - echo "Invalid issue validation summary in .local/review.json: issueValidation.summary must be a non-empty string" - exit 1 - fi - - local runtime_file_count - if ! jq -e 'type == "object" and (.files | type) == "array" and all(.files[]; type == "object" and (.path | type) == "string")' .local/pr-meta.json >/dev/null 2>&1; then - echo "Invalid .local/pr-meta.json: files must be an array of objects with string path" - exit 1 - fi - runtime_file_count=$(jq '[.files[]? | (.path // "") | select(test("^(src|extensions|apps)/")) | select(test("(^|/)__tests__/|\\.test\\.|\\.spec\\.") | not) | select(test("\\.(md|mdx)$") | not)] | length' .local/pr-meta.json) - - local runtime_review_required="false" - if [ "$runtime_file_count" -gt 0 ]; then - runtime_review_required="true" - fi - - local behavioral_sweep_performed - review_json_require '(.behavioralSweep.performed | type) == "boolean"' "Invalid behavioral sweep in .local/review.json: behavioralSweep.performed must be a boolean" - behavioral_sweep_performed=$(jq -r '.behavioralSweep.performed // empty' .local/review.json) - if [ "$behavioral_sweep_performed" != "true" ]; then - echo "Invalid behavioral sweep in .local/review.json: behavioralSweep.performed must be true" - exit 1 - fi - - local behavioral_sweep_status - review_json_require '(.behavioralSweep.status | type) == "string"' "Invalid behavioral sweep status in .local/review.json: behavioralSweep.status must be a string" - behavioral_sweep_status=$(jq -r '.behavioralSweep.status // ""' .local/review.json) - case "$behavioral_sweep_status" in - "pass"|"needs_work"|"not_applicable") - ;; - *) - printf 'Invalid behavioral sweep status in .local/review.json: %s (allowed: pass|needs_work|not_applicable)\n' "$(jq -c '.behavioralSweep.status' .local/review.json)" - exit 1 - ;; - esac - - local behavioral_sweep_risk - review_json_require '(.behavioralSweep.silentDropRisk | type) == "string"' "Invalid behavioral sweep risk in .local/review.json: behavioralSweep.silentDropRisk must be a string" - behavioral_sweep_risk=$(jq -r '.behavioralSweep.silentDropRisk // ""' .local/review.json) - case "$behavioral_sweep_risk" in - "none"|"present"|"unknown") - ;; - *) - printf 'Invalid behavioral sweep risk in .local/review.json: %s (allowed: none|present|unknown)\n' "$(jq -c '.behavioralSweep.silentDropRisk' .local/review.json)" - exit 1 - ;; - esac - - local invalid_behavioral_summary_count - review_json_require '(.behavioralSweep.summary | type) == "string"' "Invalid behavioral sweep summary in .local/review.json: behavioralSweep.summary must be a string" - invalid_behavioral_summary_count=$(jq '[.behavioralSweep.summary | select((type != "string") or (gsub("^\\s+|\\s+$";"") | length == 0))] | length' .local/review.json) - if [ "$invalid_behavioral_summary_count" -gt 0 ]; then - echo "Invalid behavioral sweep summary in .local/review.json: behavioralSweep.summary must be a non-empty string" - exit 1 - fi - - local behavioral_branches_is_array - behavioral_branches_is_array=$(jq -r 'if (.behavioralSweep.branches | type) == "array" then "true" else "false" end' .local/review.json) - if [ "$behavioral_branches_is_array" != "true" ]; then - echo "Invalid behavioral sweep in .local/review.json: behavioralSweep.branches must be an array" - exit 1 - fi - - local invalid_behavioral_branch_count - review_json_require 'all(.behavioralSweep.branches[]; type == "object")' "Invalid behavioral sweep branch entry in .local/review.json: each entry must be an object with string path/decision/outcome" - invalid_behavioral_branch_count=$(jq '[.behavioralSweep.branches[]? | select((.path|type)!="string" or (.decision|type)!="string" or (.outcome|type)!="string")] | length' .local/review.json) - if [ "$invalid_behavioral_branch_count" -gt 0 ]; then - echo "Invalid behavioral sweep branch entry in .local/review.json: each entry must be an object with string path/decision/outcome" - exit 1 - fi - - local behavioral_branch_count - behavioral_branch_count=$(jq '[.behavioralSweep.branches[]?] | length' .local/review.json) - - if [ "$runtime_review_required" = "true" ] && [ "$behavioral_sweep_status" = "not_applicable" ]; then - echo "Invalid behavioral sweep in .local/review.json: runtime file changes require behavioralSweep.status=pass|needs_work" - exit 1 - fi - - if [ "$runtime_review_required" = "true" ] && [ "$behavioral_branch_count" -lt 1 ]; then - echo "Invalid behavioral sweep in .local/review.json: runtime file changes require at least one branch entry" - exit 1 - fi - - if [ "$behavioral_sweep_status" = "not_applicable" ] && [ "$behavioral_branch_count" -gt 0 ]; then - echo "Invalid behavioral sweep in .local/review.json: not_applicable cannot include branch entries" - exit 1 - fi - - if [ "$behavioral_sweep_status" = "pass" ] && [ "$behavioral_sweep_risk" != "none" ]; then - echo "Invalid behavioral sweep in .local/review.json: status=pass requires silentDropRisk=none" - exit 1 - fi - - if [ "$recommendation" = "READY FOR /prepare-pr" ] && [ "$issue_validation_status" != "valid" ]; then - echo "Invalid recommendation in .local/review.json: READY FOR /prepare-pr requires issueValidation.status=valid" - exit 1 - fi - - if [ "$recommendation" = "READY FOR /prepare-pr" ] && [ "$behavioral_sweep_status" = "needs_work" ]; then - echo "Invalid recommendation in .local/review.json: READY FOR /prepare-pr requires behavioralSweep.status!=needs_work" - exit 1 - fi - - if [ "$recommendation" = "READY FOR /prepare-pr" ] && [ "$runtime_review_required" = "true" ] && [ "$behavioral_sweep_status" != "pass" ]; then - echo "Invalid recommendation in .local/review.json: READY FOR /prepare-pr on runtime changes requires behavioralSweep.status=pass" - exit 1 - fi - - if [ "$recommendation" = "READY FOR /prepare-pr" ] && [ "$behavioral_sweep_risk" = "present" ]; then - echo "Invalid recommendation in .local/review.json: READY FOR /prepare-pr is not allowed when behavioralSweep.silentDropRisk=present" - exit 1 - fi - - review_json_require '(.tests.ran | type) == "array"' "Invalid tests in .local/review.json: tests.ran must be an array of strings" - review_json_require 'all(.tests.ran[]; type == "string")' "Invalid tests in .local/review.json: tests.ran must be an array of strings" - review_json_require '(.tests.gaps | type) == "array"' "Invalid tests in .local/review.json: tests.gaps must be an array of strings" - review_json_require 'all(.tests.gaps[]; type == "string")' "Invalid tests in .local/review.json: tests.gaps must be an array of strings" - - local tests_result - review_json_require '(.tests.result | type) == "string"' "Invalid tests result in .local/review.json: tests.result must be a string" - tests_result=$(jq -r '.tests.result // ""' .local/review.json) - case "$tests_result" in - "pass"|"fail"|"not_run") - ;; - *) - printf 'Invalid tests result in .local/review.json: %s (allowed: pass|fail|not_run)\n' "$(jq -c '.tests.result' .local/review.json)" - exit 1 - ;; - esac - - local docs_status - review_json_require '(.docs | type) == "string"' "Invalid docs status in .local/review.json: docs must be a string" - docs_status=$(jq -r '.docs // ""' .local/review.json) - case "$docs_status" in - "up_to_date"|"missing"|"not_applicable") - ;; - *) - printf 'Invalid docs status in .local/review.json: %s (allowed: up_to_date|missing|not_applicable)\n' "$(jq -c '.docs' .local/review.json)" - exit 1 - ;; - esac - - local changelog_status - review_json_require '(.changelog | type) == "string"' "Invalid changelog status in .local/review.json: changelog must be a string" - changelog_status=$(jq -r '.changelog // ""' .local/review.json) - case "$changelog_status" in - "required"|"not_required") - ;; - *) - printf 'Invalid changelog status in .local/review.json: %s (allowed: required|not_required)\n' "$(jq -c '.changelog' .local/review.json)" - exit 1 - ;; - esac - echo "review artifacts validated" print_review_stdout_summary } @@ -515,6 +223,7 @@ review_tests() { fi done + mark_pr_operation_side_effects_started bootstrap_deps_if_needed local run_log=".local/review-tests-run.log" @@ -546,6 +255,7 @@ review_tests() { review_init() { local pr="$1" + mark_pr_operation_side_effects_started enter_worktree "$pr" true local json pr_url diff --git a/test/scripts/committer.test.ts b/test/scripts/committer.test.ts index 5c71d9b7269b..1d6c95811f86 100644 --- a/test/scripts/committer.test.ts +++ b/test/scripts/committer.test.ts @@ -1,5 +1,5 @@ // Committer tests cover committer script behavior. -import { execFileSync } from "node:child_process"; +import { execFileSync, spawnSync } from "node:child_process"; import { cpSync, mkdirSync, mkdtempSync, rmSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import path from "node:path"; @@ -63,6 +63,10 @@ function commitWithHelperArgs(repo: string, ...args: string[]) { return run(repo, "bash", [scriptPath, ...args]); } +function commitWithHelperFailure(repo: string, ...args: string[]) { + return spawnSync("bash", [scriptPath, ...args], { cwd: repo, encoding: "utf8" }); +} + function committedPaths(repo: string) { const output = git(repo, "diff-tree", "--no-commit-id", "--name-only", "-r", "HEAD"); const paths: string[] = []; @@ -172,6 +176,64 @@ describe("scripts/committer", () => { expect(committedPaths(repo)).toEqual(["note.txt"]); }); + it("fails before staging when formatting dependencies are missing", () => { + const repo = createRepo(); + writeRepoFile(repo, "pnpm-lock.yaml", "lockfileVersion: '9.0'\n"); + writeRepoFile( + repo, + "scripts/pre-commit/filter-staged-files.mjs", + "for (const file of process.argv.slice(4)) { if (file.endsWith('.ts')) process.stdout.write(file + '\\0'); }\n", + ); + writeRepoFile(repo, "note.ts", "export const note = true;\n"); + + const result = commitWithHelperFailure(repo, "test: missing formatter", "note.ts"); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("cannot run oxfmt without node_modules"); + expect(result.stderr).toContain("--no-verify-formatted"); + expect(git(repo, "diff", "--cached", "--name-only")).toBe(""); + expect(git(repo, "log", "-1", "--pretty=%s")).toBe("seed"); + }); + + it("commits dependency-less formatted work only with the explicit assertion", () => { + const repo = createRepo(); + writeRepoFile(repo, "pnpm-lock.yaml", "lockfileVersion: '9.0'\n"); + writeRepoFile( + repo, + "scripts/pre-commit/filter-staged-files.mjs", + "for (const file of process.argv.slice(4)) { if (file.endsWith('.ts')) process.stdout.write(file + '\\0'); }\n", + ); + writeRepoFile(repo, "note.ts", "export const note = true;\n"); + + const output = commitWithHelperArgs( + repo, + "--no-verify-formatted", + "test: formatted assertion", + "note.ts", + ); + + expect(output).toContain("asserts separate formatting proof; committing with --no-verify"); + expect(committedPaths(repo)).toEqual(["note.ts"]); + }); + + it("fails before staging when formatter applicability cannot be determined", () => { + const repo = createRepo(); + writeRepoFile(repo, "pnpm-lock.yaml", "lockfileVersion: '9.0'\n"); + writeRepoFile( + repo, + "scripts/pre-commit/filter-staged-files.mjs", + "process.stderr.write('fixture filter failure\\n'); process.exit(7);\n", + ); + writeRepoFile(repo, "note.ts", "export const note = true;\n"); + + const result = commitWithHelperFailure(repo, "test: failed formatter filter", "note.ts"); + + expect(result.status).toBe(1); + expect(result.stderr).toContain("Unable to determine formatter applicability"); + expect(git(repo, "diff", "--cached", "--name-only")).toBe(""); + expect(git(repo, "log", "-1", "--pretty=%s")).toBe("seed"); + }); + it("bypasses git hooks when using --fast", () => { const repo = createRepo(); installHook(repo, ".githooks/pre-commit", "#!/usr/bin/env bash\nset -euo pipefail\nexit 91\n"); @@ -211,7 +273,7 @@ describe("scripts/committer", () => { const output = commitWithHelperArgs(repo, "--help"); expect(output).toContain( - 'Usage: committer [--force] [--fast] "commit message" "file" ["file" ...]', + 'Usage: committer [--force] [--fast] [--no-verify-formatted] "commit message" "file" ["file" ...]', ); }); }); diff --git a/test/scripts/pr-ci-dispatch.test.ts b/test/scripts/pr-ci-dispatch.test.ts new file mode 100644 index 000000000000..1a9315f73fe8 --- /dev/null +++ b/test/scripts/pr-ci-dispatch.test.ts @@ -0,0 +1,141 @@ +import { spawnSync } from "node:child_process"; +import { chmodSync, existsSync, readFileSync, writeFileSync } from "node:fs"; +import { join } from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; +import { useAutoCleanupTempDirTracker } from "../helpers/temp-dir.js"; + +const tempDirs = useAutoCleanupTempDirTracker(afterEach); +const dispatchScript = join(process.cwd(), "scripts/pr-lib/ci-dispatch.mjs"); +const sha = "0123456789abcdef0123456789abcdef01234567"; +const changedSha = "fedcba9876543210fedcba9876543210fedcba98"; +const describePosix = process.platform === "win32" ? describe.skip : describe; + +function createFakeGh() { + const tempDir = tempDirs.make("openclaw-pr-ci-dispatch-"); + const fakeGh = join(tempDir, "gh"); + const calls = join(tempDir, "calls.log"); + const dispatched = join(tempDir, "dispatched"); + const seenRunList = join(tempDir, "seen-run-list"); + writeFileSync( + fakeGh, + `#!/usr/bin/env bash +set -euo pipefail +printf '%s\\n' "$*" >> "$OPENCLAW_TEST_GH_CALLS" +case "$1 $2" in + "run list") + if [ "\${OPENCLAW_TEST_GH_MODE:-}" = "pending-head-change" ]; then + printf '[]\\n' + elif [ -e "$OPENCLAW_TEST_GH_SEEN_RUN_LIST" ]; then + printf '[{"databaseId":99,"url":"https://github.com/openclaw/openclaw/actions/runs/99","headSha":"%s","createdAt":"2026-01-01T00:00:00Z","status":"queued"}]\\n' "$OPENCLAW_TEST_HEAD_SHA" + else + : > "$OPENCLAW_TEST_GH_SEEN_RUN_LIST" + printf '[]\\n' + fi + ;; + "pr view") + if [ -e "$OPENCLAW_TEST_GH_DISPATCHED" ] && [ -n "\${OPENCLAW_TEST_GH_MODE:-}" ]; then + printf '%s\\n' "$OPENCLAW_TEST_CHANGED_HEAD_SHA" + else + printf '%s\\n' "$OPENCLAW_TEST_HEAD_SHA" + fi + ;; + "workflow run") : > "$OPENCLAW_TEST_GH_DISPATCHED" ;; + *) echo "unexpected gh invocation: $*" >&2; exit 2 ;; +esac +`, + ); + chmodSync(fakeGh, 0o755); + return { calls, dispatched, fakeGh, seenRunList }; +} + +function runDispatch( + fakeGh: ReturnType, + options: { + mode?: "observed-head-change" | "pending-head-change"; + immediateTimers?: boolean; + } = {}, +) { + let nodeOptions = process.env.NODE_OPTIONS ?? ""; + if (options.immediateTimers) { + const preload = join(tempDirs.make("openclaw-pr-ci-dispatch-timers-"), "immediate-timers.cjs"); + writeFileSync(preload, "global.setTimeout = (callback) => { callback(); return 0; };\n"); + nodeOptions = `${nodeOptions} --require ${preload}`.trim(); + } + return spawnSync( + process.execPath, + [dispatchScript, "12345", "contributor/fix-hosted-gates", sha, "false"], + { + encoding: "utf8", + env: { + ...process.env, + NODE_OPTIONS: nodeOptions, + OPENCLAW_GH_BIN: fakeGh.fakeGh, + OPENCLAW_TEST_CHANGED_HEAD_SHA: changedSha, + OPENCLAW_TEST_GH_CALLS: fakeGh.calls, + OPENCLAW_TEST_GH_DISPATCHED: fakeGh.dispatched, + OPENCLAW_TEST_GH_MODE: options.mode ?? "", + OPENCLAW_TEST_GH_SEEN_RUN_LIST: fakeGh.seenRunList, + OPENCLAW_TEST_HEAD_SHA: sha, + }, + }, + ); +} + +describePosix("scripts/pr ci-dispatch", () => { + it("dispatches the exact CI workflow for the remote PR head", () => { + const fakeGh = createFakeGh(); + const result = runDispatch(fakeGh); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(result.stdout).toContain( + "observed_run_url=https://github.com/openclaw/openclaw/actions/runs/99", + ); + expect(readFileSync(fakeGh.calls, "utf8")).toContain( + `workflow run ci.yml --ref contributor/fix-hosted-gates -f target_ref=${sha} -f release_gate=true -f pull_request_number=12345`, + ); + }); + + it("refuses a fork-local branch name before invoking GitHub", () => { + const fakeGh = createFakeGh(); + const result = spawnSync( + process.execPath, + [dispatchScript, "12345", "fix-hosted-gates", sha, "true"], + { + encoding: "utf8", + env: { + ...process.env, + OPENCLAW_GH_BIN: fakeGh.fakeGh, + OPENCLAW_TEST_GH_CALLS: fakeGh.calls, + OPENCLAW_TEST_GH_DISPATCHED: fakeGh.dispatched, + OPENCLAW_TEST_GH_SEEN_RUN_LIST: fakeGh.seenRunList, + OPENCLAW_TEST_HEAD_SHA: sha, + }, + }, + ); + + expect(result.status).not.toBe(0); + expect(`${result.stdout}\n${result.stderr}`).toMatch(/comes from a fork/u); + expect(existsSync(fakeGh.calls)).toBe(false); + }); + + it("fails closed if the remote head changes while CI run indexing is pending", () => { + const result = runDispatch(createFakeGh(), { + immediateTimers: true, + mode: "pending-head-change", + }); + + expect(result.status).not.toBe(0); + expect(`${result.stdout}\n${result.stderr}`).toMatch( + /head changed while CI dispatch was being indexed/u, + ); + }); + + it("rechecks the remote head before returning an observed exact-SHA run", () => { + const result = runDispatch(createFakeGh(), { mode: "observed-head-change" }); + + expect(result.status).not.toBe(0); + expect(`${result.stdout}\n${result.stderr}`).toMatch( + /head changed before an exact-SHA CI run became visible/u, + ); + }); +}); diff --git a/test/scripts/pr-operation-lock.test.ts b/test/scripts/pr-operation-lock.test.ts index 8e9156630aa2..606d057ff419 100644 --- a/test/scripts/pr-operation-lock.test.ts +++ b/test/scripts/pr-operation-lock.test.ts @@ -129,8 +129,10 @@ function installPrCliFixture(repoDir: string) { "scripts/pr-lib/common.sh", "scripts/pr-lib/changelog.sh", "scripts/pr-lib/gates.sh", + "scripts/pr-lib/ci-dispatch.mjs", "scripts/pr-lib/push.sh", "scripts/pr-lib/review.sh", + "scripts/pr-lib/review-artifacts.mjs", "scripts/pr-lib/prepare-core.sh", "scripts/pr-lib/merge.sh", ]; @@ -1009,6 +1011,41 @@ describePosix("scripts/pr per-PR operation lock", () => { expect(result.stderr).not.toContain("Retaining the operation lock"); }); + it("releases a failed lock while the child is still in validation phase", async () => { + const repoDir = createRepo(); + const fixture = writeOperationFixture(repoDir, "failed-validation.sh", [ + "acquire_pr_operation_lock 42", + "begin_pr_operation_validation_phase", + "exit 3", + ]); + const result = await runSupervisedFixture(repoDir, fixture); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(3); + expect(refExists(repoDir)).toBe(false); + expect(result.stderr).not.toContain("Retaining the operation lock"); + }); + + it("retains a failed lock after the child leaves validation phase", async () => { + const repoDir = createRepo(); + const fixture = writeOperationFixture(repoDir, "failed-after-side-effects.sh", [ + "acquire_pr_operation_lock 42", + "begin_pr_operation_validation_phase", + "mark_pr_operation_side_effects_started", + "exit 3", + ]); + const result = await runSupervisedFixture(repoDir, fixture); + const ownerOid = refOid(repoDir); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(3); + expect(result.stderr).toContain("reason: child exited with code 3"); + expect(refOid(repoDir)).toBe(ownerOid); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + }); + it("reports the child exit code when retaining a failed operation", async () => { const repoDir = createRepo(); const fixture = writeOperationFixture(repoDir, "failed-operation.sh", [ @@ -1029,6 +1066,69 @@ describePosix("scripts/pr per-PR operation lock", () => { expect(refExists(repoDir)).toBe(false); }); + it("does not re-enter validation after side effects have started", async () => { + const repoDir = createRepo(); + const fixture = writeOperationFixture(repoDir, "failed-after-forged-validation.sh", [ + "acquire_pr_operation_lock 42", + "begin_pr_operation_validation_phase", + "mark_pr_operation_side_effects_started", + "notify_pr_operation_phase validation-started", + "exit 3", + ]); + const result = await runSupervisedFixture(repoDir, fixture); + const ownerOid = refOid(repoDir); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(3); + expect(result.stderr).toContain("reason: child exited with code 3"); + expect(refOid(repoDir)).toBe(ownerOid); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + }); + + it("retains a validation-phase lock when the child exits through a trapped signal", async () => { + const repoDir = createRepo(); + const fixture = writeOperationFixture(repoDir, "signaled-validation.sh", [ + "trap 'exit 143' TERM", + "acquire_pr_operation_lock 42", + "begin_pr_operation_validation_phase", + "kill -TERM $$", + ]); + const result = await runSupervisedFixture(repoDir, fixture); + const ownerOid = refOid(repoDir); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(143); + expect(result.stderr).toContain("reason: child exited with code 143"); + expect(refOid(repoDir)).toBe(ownerOid); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + }); + + it("retains a validation-phase lock for untrapped signal exit statuses", async () => { + const repoDir = createRepo(); + const fixture = writeOperationFixture(repoDir, "killed-validation.sh", [ + "acquire_pr_operation_lock 42", + "begin_pr_operation_validation_phase", + "exit 137", + ]); + const result = await runSupervisedFixture(repoDir, fixture); + const ownerOid = refOid(repoDir); + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(137); + expect(result.stderr).toContain("reason: child exited with code 137"); + expect(refOid(repoDir)).toBe(ownerOid); + + const recovered = runLockShell(repoDir, [ + `recover_pr_operation_lock 42 '${ownerOid}' --confirmed-no-running-tools`, + ]); + expect(recovered.status, `${recovered.stdout}\n${recovered.stderr}`).toBe(0); + }); + it("releases the lock after the operation deletes its runner worktree", async () => { const repoDir = createRepo(); const doomedDir = tempDirs.make("openclaw-pr-self-deleting-runner-"); diff --git a/test/scripts/pr-prepare-gates.test.ts b/test/scripts/pr-prepare-gates.test.ts index 685a21f4c891..97c74aac26bb 100644 --- a/test/scripts/pr-prepare-gates.test.ts +++ b/test/scripts/pr-prepare-gates.test.ts @@ -63,6 +63,7 @@ function runGatesBash( `script_parent_dir='${repoRoot}/scripts'`, `source '${repoRoot}/scripts/pr-lib/common.sh'`, `source '${repoRoot}/scripts/pr-lib/gates.sh'`, + "mark_pr_operation_side_effects_started() { :; }", ...(options.sourcePush ? [`source '${repoRoot}/scripts/pr-lib/push.sh'`] : []), ...(options.sourcePrepareCore ? [`source '${repoRoot}/scripts/pr-lib/prepare-core.sh'`] @@ -696,7 +697,7 @@ describe("prepare gate stamp transitions", () => { }).stdout.trim(); const result = runGatesBash( [ - `gh() { if [ "$1" = pr ]; then printf '${currentHead}\\n'; else printf 'openclaw/openclaw\\n'; fi; }`, + `gh() { if [ "$1" = pr ]; then printf '{"headRefName":"topic","headRefOid":"${currentHead}","isCrossRepository":false}\\n'; else printf 'openclaw/openclaw\\n'; fi; }`, "run_quiet_logged() { printf 'ARG:%s\\n' \"$@\"; }", `run_hosted_prepare_gates 100606 ${currentHead} false`, ].join("\n"), @@ -711,6 +712,43 @@ describe("prepare gate stamp transitions", () => { } }); + it("prints the exact recovery command when hosted CI is missing", () => { + const { repoDir, headSha } = makeRetryRepo(); + const result = runGatesBash( + [ + `gh() { if [ "$1" = pr ]; then printf '{"headRefName":"topic","headRefOid":"${headSha}","isCrossRepository":false}\\n'; else printf 'openclaw/openclaw\\n'; fi; }`, + 'rg() { command grep -F -q "$3" "$4"; }', + `run_quiet_logged() { printf 'Missing successful recent CI workflow for ${headSha}. Observed: none\\n' > "$2"; return 1; }`, + `run_hosted_prepare_gates 100606 ${headSha} false`, + ].join("\n"), + { cwd: repoDir }, + ); + + expect(result.status).toBe(1); + expect(result.stdout).toContain("scripts/pr ci-dispatch 100606"); + expect(result.stdout).toContain( + `gh workflow run ci.yml --ref topic -f target_ref=${headSha} -f release_gate=true -f pull_request_number=100606`, + ); + }); + + it("does not advertise an unusable dispatch command for fork PRs", () => { + const { repoDir, headSha } = makeRetryRepo(); + const result = runGatesBash( + [ + `gh() { if [ "$1" = pr ]; then printf '{"headRefName":"topic","headRefOid":"${headSha}","isCrossRepository":true}\\n'; else printf 'openclaw/openclaw\\n'; fi; }`, + 'rg() { command grep -F -q "$3" "$4"; }', + `run_quiet_logged() { printf 'Missing successful recent CI workflow for ${headSha}. Observed: none\\n' > "$2"; return 1; }`, + `run_hosted_prepare_gates 100606 ${headSha} false`, + ].join("\n"), + { cwd: repoDir }, + ); + + expect(result.status).toBe(1); + expect(result.stdout).toContain("scripts/pr ci-dispatch 100606"); + expect(result.stdout).toContain("unavailable: PR #100606 comes from a fork"); + expect(result.stdout).not.toContain("gh workflow run"); + }); + it("clears remote stamps when fresh docs-only gates do not reuse prior proof", () => { const { repoDir } = makeRetryRepo(); spawnSync("git", ["update-ref", "refs/remotes/origin/main", "HEAD"], { cwd: repoDir }); diff --git a/test/scripts/pr-review-artifact-validation.test.ts b/test/scripts/pr-review-artifact-validation.test.ts index c67351e48043..1ebf541ff8f2 100644 --- a/test/scripts/pr-review-artifact-validation.test.ts +++ b/test/scripts/pr-review-artifact-validation.test.ts @@ -6,6 +6,7 @@ import { useAutoCleanupTempDirTracker } from "../helpers/temp-dir.js"; const tempDirs = useAutoCleanupTempDirTracker(afterEach); const reviewScript = join(process.cwd(), "scripts/pr-lib/review.sh"); +const reviewArtifactsScript = join(process.cwd(), "scripts/pr-lib/review-artifacts.mjs"); const describePosix = process.platform === "win32" ? describe.skip : describe; function validReview() { @@ -106,4 +107,48 @@ describePosix("scripts/pr review artifact validation", () => { 'Invalid behavioral sweep status in .local/review.json: "performed" (allowed: pass|needs_work|not_applicable)', ); }); + + it("reports every artifact violation before exiting", () => { + const review = validReview(); + review.behavioralSweep.status = "performed"; + review.behavioralSweep.branches = "src/example.ts" as unknown as unknown[]; + review.docs = "todo"; + const result = runValidation(review); + + expect(result.status).toBe(1); + expect(result.stdout).toContain( + 'Invalid behavioral sweep status in .local/review.json: "performed" (allowed: pass|needs_work|not_applicable)', + ); + expect(result.stdout).toContain( + "Invalid behavioral sweep in .local/review.json: behavioralSweep.branches must be an array", + ); + expect(result.stdout).toContain( + 'Invalid docs status in .local/review.json: "todo" (allowed: up_to_date|missing|not_applicable)', + ); + expect(result.stdout).toContain("3 artifact violations"); + }); + + it("derives template enum hints from the validation table", () => { + const result = spawnSync(process.execPath, [reviewArtifactsScript, "template"], { + encoding: "utf8", + }); + const template = JSON.parse(result.stdout) as ReturnType; + + expect(result.status, `${result.stdout}\n${result.stderr}`).toBe(0); + expect(template.recommendation).toBe( + "NEEDS WORK (allowed: READY FOR /prepare-pr|NEEDS WORK|NEEDS DISCUSSION|NOT USEFUL (CLOSE))", + ); + expect(template.nitSweep.status).toBe("none (allowed: none|has_nits)"); + expect(template.behavioralSweep.status).toBe( + "not_applicable (allowed: pass|needs_work|not_applicable)", + ); + expect(template.behavioralSweep.silentDropRisk).toBe("none (allowed: none|present|unknown)"); + expect(template.issueValidation.source).toBe("pr_body (allowed: linked_issue|pr_body|both)"); + expect(template.issueValidation.status).toBe( + "unclear (allowed: valid|unclear|invalid|already_fixed_on_main)", + ); + expect(template.tests.result).toBe("pass (allowed: pass|fail|not_run)"); + expect(template.docs).toBe("not_applicable (allowed: up_to_date|missing|not_applicable)"); + expect(template.changelog).toBe("not_required (allowed: required|not_required)"); + }); }); diff --git a/test/scripts/pr-wrappers.test.ts b/test/scripts/pr-wrappers.test.ts index 3127012102fb..b0aade6e31ae 100644 --- a/test/scripts/pr-wrappers.test.ts +++ b/test/scripts/pr-wrappers.test.ts @@ -20,9 +20,11 @@ describe("scripts/pr wrappers", () => { expect(script).toContain("gh_plain"); expect(script).toContain("scripts/pr review-init "); expect(script).toContain("scripts/pr prepare-run "); + expect(script).toContain("scripts/pr ci-dispatch "); expect(script).toContain("scripts/pr merge-run "); expect(script).toContain('review_init "$pr"'); expect(script).toContain('prepare_run "$pr"'); + expect(script).toContain('ci_dispatch "$pr"'); expect(script).toContain('merge_run "$pr"'); expect(script).toContain('require_main_target_pr "${1-}"'); expect(script).toContain("only support PRs targeting main");