diff --git a/plugins/skill-set/commands/pr/ship.md b/plugins/skill-set/commands/pr/ship.md index 80ab579..4549b44 100644 --- a/plugins/skill-set/commands/pr/ship.md +++ b/plugins/skill-set/commands/pr/ship.md @@ -9,10 +9,10 @@ Supported flags and defaults: - `--max-cycles N` — resolver attempts, default 3 - `--ci-timeout MIN` — current-HEAD CI deadline, default 30 -- `--review-timeout MIN` — current-HEAD automated-review deadline, default 10 +- `--review-timeout MIN` — deprecated compatibility input with no completion-gate effect - `--no-create` — fail when the current branch has no PR -- `--required-only=BOOL` — select required checks only, default true +- `--required-only=BOOL` — enforce effective required checks only; false additionally selects observed optional checks, default true -Reviewer detection is always automatic for CodeRabbit, Claude, and `chatgpt-codex-connector`; no adapter flag is accepted. Invoking this command authorizes one automatic initial commit containing the complete inspected working-tree scope, publication of existing and resolver commits, PR creation when needed, and gated resolution feedback. Do not ask again before a normal commit or push; stop only for ambiguous fix decisions, stale/diverged state, or a failed publication gate. +Reviewer detection is automatic and reporting-only for CodeRabbit, Claude, and `chatgpt-codex-connector`; no adapter flag is accepted. Only effective required check contexts gate review completion. Invoking this command authorizes one automatic initial commit containing the complete inspected working-tree scope, publication of existing and resolver commits, PR creation when needed, and gated resolution feedback. Do not ask again before a normal commit or push; stop only for ambiguous fix decisions, stale/diverged state, or a failed publication gate. The workflow keeps state in the repository's Git common directory and rejects concurrent active runs. It binds publication to the live PR head repository/ref and the selected remote's canonical push URL, including fork remotes. Resume existing state instead of starting a second loop. Never include post-inspection changes, publish partial resolver work, force-push, merge, pull, or rebase. diff --git a/plugins/skill-set/evals/shipping-pr/state-machine-happy-path/fixtures/mock-gh b/plugins/skill-set/evals/shipping-pr/state-machine-happy-path/fixtures/mock-gh index 8b94cc2..58f4010 100755 --- a/plugins/skill-set/evals/shipping-pr/state-machine-happy-path/fixtures/mock-gh +++ b/plugins/skill-set/evals/shipping-pr/state-machine-happy-path/fixtures/mock-gh @@ -31,6 +31,16 @@ if [[ ${1:-} == pr && ${2:-} == checks ]]; then exit 0 fi +if [[ ${1:-} == api && ${2:-} == repos/*/rules/branches/* ]]; then + printf '%s\n' '[{"type":"required_status_checks","parameters":{"required_status_checks":[{"context":"verify","integration_id":null}]}}]' + exit 0 +fi + +if [[ ${1:-} == api && ${2:-} == repos/*/branches/*/protection/required_status_checks* ]]; then + printf '%s\n' 'gh: Branch not protected (HTTP 404)' >&2 + exit 1 +fi + if [[ ${1:-} == api && ${2:-} == graphql ]]; then graphql_id= graphql_body= diff --git a/plugins/skill-set/skills/shipping-pr/SKILL.md b/plugins/skill-set/skills/shipping-pr/SKILL.md index 6b8203b..ea37a98 100644 --- a/plugins/skill-set/skills/shipping-pr/SKILL.md +++ b/plugins/skill-set/skills/shipping-pr/SKILL.md @@ -29,9 +29,9 @@ Do not use for: |---|---:|---| | `--max-cycles` | 3 | Maximum resolver attempts | | `--ci-timeout` | 30 minutes | Current-HEAD check deadline | -| `--review-timeout` | 10 minutes | Persisted compatibility deadline; reviewer telemetry never delays a settled PR | +| `--review-timeout` | 10 minutes | Deprecated compatibility input; it does not gate completion | | `--no-create` | off | Refuse to create a missing PR | -| `--required-only` | true | Select required checks only | +| `--required-only` | true | Enforce effective required checks only; `false` additionally selects observed optional checks | ## Common Scenarios @@ -39,7 +39,7 @@ Do not use for: - “Ship this branch even though it is behind main” commits and publishes the current branch state, then lets the normal blocker cycle resolve any base conflict. - “Keep resolving without extra checkouts” reconciles and fixes the PR in the currently checked-out worktree and branch. - “Resume PR 42” loads the active run and branches on its persisted status/publication phase without starting a duplicate resolver. -- “Is this PR truly clean?” snapshots the same HEAD across checks, paginated threads, mergeability, and auto-detected reviewer telemetry. Pending reviewer telemetry is reported without delaying a settled PR. +- “Is this PR truly clean?” snapshots the same HEAD across effective required checks, paginated threads, and mergeability. Reviewer telemetry is reported without affecting the verdict. ## Workflow @@ -74,11 +74,10 @@ Convert minute flags to seconds, then run: --pr "$PR" --repo "$REPO" \ --max-cycles "$MAX_CYCLES" \ --ci-timeout-seconds "$CI_TIMEOUT_SECONDS" \ - --review-timeout-seconds "$REVIEW_TIMEOUT_SECONDS" \ --required-only "$REQUIRED_ONLY" ``` -Reviewer discovery is always automatic. The runner detects CodeRabbit, Claude, and `chatgpt-codex-connector` from recent merged-PR activity, then incorporates current-PR evidence on every snapshot for reporting and blocker fingerprints. Do not ask the user to select an adapter or pass reviewer-specific flags. Reviewer telemetry is not a separate completion gate: once selected checks are settled and no actionable review thread remains, an absent, pending, or failed reviewer signal must not keep the run polling or make it time out. A reviewer check still participates through the normal selected-check gate, and reviewer feedback still participates through review threads. +Reviewer discovery is always automatic and reporting-only. The runner detects CodeRabbit, Claude, and `chatgpt-codex-connector` from recent merged-PR activity, but their absent, pending, or failed telemetry never affects status, deadlines, or blocker fingerprints. Do not ask the user to select an adapter or pass reviewer-specific flags. A review-related status or check gates completion only when it is an effective required context; `--required-only false` intentionally broadens the selected set to every observed check. Existing actionable review threads still participate through the review-thread gate. Use `--resume` only when the runner reports an active run. State lives under the repository's Git common directory, so linked worktrees share one lock and one run. Branch on the returned status: snapshot only `polling`; handle `blocked`, `awaiting_user`, and `resolving` before polling again. A resumed `resolving` run must use its recorded `resolution` metadata, including `decision_requirements` and `decisions`, and must never dispatch a duplicate resolver. Resume a journaled `prepared`, `gate_passed`, or `commenting` publication by calling `publish` with the unchanged files; the runner reconciles the remote HEAD and hidden comment marker. If its phase is `pending`, report the recorded worktree/branch and treat the interrupted attempt as `partial-failure`. A resumed `awaiting_user` run remains paused with its saved result and recovery paths until the user explicitly decides every recorded ID. @@ -150,13 +149,15 @@ Every transition supplies `--from`, `--to`, and `--expected-run-id`. Treat a com Declare clean only when one snapshot confirms all of the following for the same HEAD at query start and finish: - no merge conflict; -- every selected check is `pass` or `skipping`; +- every effective required check context is present and `pass` or `skipping`; +- every additionally selected optional check is `pass` or `skipping`; - no fail, cancel, pending, or timeout result; +- GitHub mergeability is known; - no unresolved actionable review thread; -Auto-detected reviewer signals are telemetry only. Their absence, pending state, or standalone failure does not override this verdict; checks and actionable review threads remain authoritative. +An effective required context that has not appeared on the current HEAD is `pending`, not absent from the verdict. Auto-detected reviewer signals are telemetry only and never override this rule; a reviewer affects the verdict only through a required check context or an existing actionable thread. `mergeStateStatus=BLOCKED` is reported but does not independently gate completion because it can include approval requirements outside the required check set. -If HEAD changes, discard the old results. The runner resets the check deadline, review deadline, and check-registration grace before snapshotting the new HEAD. +If HEAD changes, discard the old results. The runner resets the check deadline and check-registration grace before snapshotting the new HEAD. ## Output diff --git a/plugins/skill-set/skills/shipping-pr/reference/polling.md b/plugins/skill-set/skills/shipping-pr/reference/polling.md index 0e6d279..e37037f 100644 --- a/plugins/skill-set/skills/shipping-pr/reference/polling.md +++ b/plugins/skill-set/skills/shipping-pr/reference/polling.md @@ -15,7 +15,7 @@ If the stored HEAD changed before the snapshot but the two current reads agree, ## Checks -The runner calls `gh pr checks --json bucket,name,state,link,workflow` and optionally `--required`. Buckets are classified as: +The runner queries all active rules that apply to the base branch, including inherited organization rulesets, and unions their required status-check contexts with legacy branch-protection contexts. It then calls `gh pr checks --json bucket,name,state,link,workflow` and optionally `--required`. A configured required context that is missing from the current HEAD is synthesized as `pending`. Buckets are classified as: | Bucket | Meaning | |---|---| @@ -23,9 +23,9 @@ The runner calls `gh pr checks --json bucket,name,state,link,workflow` and optio | `fail`, `cancel` | Blocked | | `pending` or unknown | Polling until the CI deadline | -Pending at the deadline becomes `timed_out`. For a new HEAD, zero selected checks remain `polling` for a 60-second registration grace so a fresh push cannot appear clean before workflows register. After that grace, a repository with genuinely no selected checks may satisfy the check condition. If checks were observed and later disappear, polling continues until the CI deadline. +Pending or missing required contexts at the deadline become `timed_out`. For a new HEAD with no configured or observed selected checks, zero selected checks remain `polling` for a 60-second registration grace so a fresh push cannot appear clean before workflows register. After that grace, a repository with genuinely no selected checks may satisfy the check condition. If checks were observed and later disappear, polling continues until the CI deadline. -Signal-gated review workflows, including `karrot-emu/signal-gated-review-action`, require no reviewer adapter. Treat the workflow job (for example, `Signal-Gated PR Review / review`) as a normal selected check and its inline findings as review threads. With the default `--required-only true`, configure that job as a required check on every target branch that must wait for it; otherwise use `--required-only false` intentionally to observe all checks. +Signal-gated review workflows require no reviewer adapter. A review workflow gates completion only through the exact status or check context configured as required on the base branch. Do not wait for a workflow job, reviewer identity, review object, reaction, or comment merely because it exists. With `--required-only false`, all currently observed checks are intentionally selected in addition to the required set. ## Review Threads @@ -37,16 +37,16 @@ Any unresolved actionable thread makes the snapshot `blocked`. ## Automated Reviewers -Reviewer discovery is always `auto`; there is no adapter-selection flag. Initialization detects CodeRabbit, Claude, and `chatgpt-codex-connector` from authors and apps found in the ten most recent merged PRs. Every snapshot unions that history with current-PR commit statuses, check-runs, reviews, comments, and reactions, so the report and blocker fingerprint include reviewer telemetry without requiring every repository to run those reviewers. +Reviewer discovery is always `auto`; there is no adapter-selection flag. Initialization detects CodeRabbit, Claude, and `chatgpt-codex-connector` from authors and apps found in the ten most recent merged PRs. Every snapshot unions that history with current-PR commit statuses, check-runs, reviews, comments, and reactions for reporting only. -CodeRabbit telemetry comes from its current-HEAD commit status or check-run. Claude telemetry comes from its current-HEAD check-run, status, or review. Codex telemetry comes from a current-HEAD review or, before any resolver push, the connector's `+1` reaction. These signals are observational and are never independently required. Their absence, pending state, or standalone failure cannot keep a snapshot `polling`, make it `timed_out`, or make it `blocked` after selected checks have settled and no actionable review thread remains. A telemetry query or normalization failure is reported as `telemetry_available:false` with `unavailable` provider states and does not change the snapshot verdict. +CodeRabbit telemetry comes from its current-HEAD commit status or check-run. Claude telemetry comes from its current-HEAD check-run, status, or review. Codex telemetry comes from a current-HEAD review or, before any resolver push, the connector's `+1` reaction. These signals are observational and are never independently required. Their absence, pending state, standalone failure, or change cannot affect polling, timeout, blocking, clean, or stalled decisions. A telemetry query or normalization failure is reported as `telemetry_available:false` with `unavailable` provider states. -Reviewer results that appear as selected PR checks still use the normal check classification. Actionable comments from every provider still use review-thread state. This makes checks and unresolved conversations authoritative while preserving reviewer evidence for reports and change fingerprints. +Reviewer results that match effective required contexts use the normal check classification. Actionable comments from every provider still use review-thread state. In the default required-only mode, optional reviewer evidence remains report-only; `--required-only false` intentionally makes every observed check part of the verdict. ## Mergeability -`CONFLICTING` or `DIRTY` is blocked. An unknown mergeability result is polling, never clean. +`CONFLICTING` or `DIRTY` is blocked. Unknown mergeability is polling and becomes `timed_out` at the current-HEAD check deadline when no actionable blocker is available. `mergeStateStatus=BLOCKED` is reported but does not gate the verdict because it can represent approval requirements outside the required status-check set. ## Fingerprint -The blocker fingerprint covers the observed HEAD; normalized check identities, states, and buckets; conflict state; unresolved thread IDs and latest-comment content; and all active reviewer states. After a resolver returns to polling, only a fresh snapshot can declare `stalled`, and only when both HEAD and fingerprint remain unchanged. +The blocker fingerprint covers the observed HEAD and base branch; effective required contexts; normalized check identities, states, and buckets; conflict or unknown-mergeability state; and unresolved thread IDs and latest-comment content. Reviewer telemetry and non-gating `mergeStateStatus` values never affect the fingerprint. After a resolver returns to polling, only a fresh snapshot can declare `stalled`, and only when both HEAD and fingerprint remain unchanged. diff --git a/plugins/skill-set/skills/shipping-pr/scripts/skill-set-pr b/plugins/skill-set/skills/shipping-pr/scripts/skill-set-pr index 7eb6adf..75f63ad 100755 --- a/plugins/skill-set/skills/shipping-pr/scripts/skill-set-pr +++ b/plugins/skill-set/skills/shipping-pr/scripts/skill-set-pr @@ -211,6 +211,7 @@ read_state() { (.github_host | type == "string" and test("^[A-Za-z0-9.-]+$")) and (.head_repo | type == "string" and test("^[A-Za-z0-9][A-Za-z0-9-]*/[A-Za-z0-9_.-]+$")) and (.head_branch | type == "string" and test("^[^\\n]+$")) and + ((.base_branch == null) or (.base_branch | type == "string" and test("^[^\\n]+$"))) and (.pr == $expected_pr) and (.head_sha | type == "string" and test("^[0-9a-fA-F]{40}$")) and (.cycle | type == "number" and . >= 0) and @@ -340,6 +341,28 @@ gh_json() { printf '%s\n' "$output" } +gh_optional_json_404() { + local output error_file error_output exit_code + error_file=$(mktemp "${TMPDIR:-/tmp}/skill-set-pr-gh.XXXXXX" 2>/dev/null) || \ + die temporary_file_failed "Unable to create a temporary gh error file." "Check temporary-directory permissions and retry." + set +e + output=$(gh "$@" 2>"$error_file") + exit_code=$? + set -e + error_output=$(<"$error_file") + rm -f -- "$error_file" + if [[ $exit_code -eq 0 ]]; then + OPTIONAL_JSON_FOUND=true + OPTIONAL_JSON=$output + elif [[ $exit_code -eq 1 && $error_output == *"(HTTP 404)"* ]]; then + OPTIONAL_JSON_FOUND=false + OPTIONAL_JSON=null + else + die github_query_failed "GitHub query failed: ${error_output:-exit $exit_code}" \ + "Verify gh authentication and repository access before retrying." + fi +} + gh_checks_json() { local output error_file error_output exit_code argument required=false for argument in "$@"; do @@ -602,9 +625,9 @@ command_init() { ' <<<"$recent_prs" 2>/dev/null) || \ die invalid_github_response "Unable to classify automated reviewer activity from gh pr list." "Update gh and retry." - local pr_json pr_state remote_head head_repo head_branch github_host pr_url + local pr_json pr_state remote_head head_repo head_branch base_branch github_host pr_url pr_json=$(gh_json pr view "$PR" --repo "$repo" \ - --json headRefOid,headRefName,headRepository,state,url) + --json headRefOid,headRefName,headRepository,baseRefName,state,url) validate_json_response "gh pr view" "$pr_json" jq -e ' type == "object" and @@ -612,6 +635,7 @@ command_init() { (.headRefName | type == "string" and length > 0) and (.headRepository | type == "object") and (.headRepository.nameWithOwner | type == "string" and length > 0) and + (.baseRefName | type == "string" and length > 0) and (.url | type == "string" and length > 0) and ((.state // "OPEN") | type == "string") ' >/dev/null 2>&1 <<<"$pr_json" || \ @@ -626,9 +650,11 @@ command_init() { validate_sha headRefOid "$head_sha" head_repo=$(jq -r .headRepository.nameWithOwner <<<"$pr_json") head_branch=$(jq -r .headRefName <<<"$pr_json") + base_branch=$(jq -r .baseRefName <<<"$pr_json") pr_url=$(jq -r .url <<<"$pr_json") validate_repo "$head_repo" validate_branch headRefName "$head_branch" + validate_branch baseRefName "$base_branch" parse_pr_host "$pr_url" github_host=$PR_HOST pr_state=$(jq -r '.state // "OPEN"' <<<"$pr_json") @@ -644,6 +670,7 @@ command_init() { new_state=$(jq -cn \ --argjson schema "$SCHEMA_VERSION" --arg run_id "$run_id" --arg repo "$repo" \ --arg github_host "$github_host" --arg head_repo "$head_repo" --arg head_branch "$head_branch" \ + --arg base_branch "$base_branch" \ --argjson pr "$PR" --arg head "$head_sha" --arg status "$initial_status" \ --argjson checks "$checks_deadline" --argjson review "$review_deadline" \ --argjson registration "$registration_deadline" \ @@ -651,7 +678,8 @@ command_init() { --argjson max_cycles "$max_cycles" --argjson required_only "$required_only" \ --argjson reviewers "$reviewers_active" \ '{schema_version:$schema,run_id:$run_id,repo:$repo,github_host:$github_host, - head_repo:$head_repo,head_branch:$head_branch,pr:$pr,head_sha:$head,cycle:0, + head_repo:$head_repo,head_branch:$head_branch,base_branch:$base_branch, + pr:$pr,head_sha:$head,cycle:0, deadlines:{checks_epoch:$checks,review_epoch:$review,registration_epoch:$registration}, blocker_fingerprint:"",checks_observed:false,resolver_attempt:null,resolution:null,status:$status, options:{ci_timeout_seconds:$ci_timeout,review_timeout_seconds:$review_timeout, @@ -661,6 +689,71 @@ command_init() { emit_state init "$new_state" false "$dry_run" } +collect_required_contexts() { + local repo=$1 + local base_branch=$2 + local encoded_branch page=1 response count + encoded_branch=$(jq -rn --arg branch "$base_branch" '$branch | @uri') + REQUIRED_CONTEXTS='[]' + + while [[ $page -le 1000 ]]; do + response=$(gh_json api "repos/$repo/rules/branches/$encoded_branch?per_page=100&page=$page") + validate_json_response "branch rules API" "$response" + jq -e ' + type == "array" and all(.[]; + type == "object" and + ((.type // "") | type == "string") and + ((.parameters // {}) | type == "object") and + (if .type == "required_status_checks" then + (.parameters.required_status_checks | type == "array") and + all(.parameters.required_status_checks[]; + type == "object" and + (.context | type == "string" and length > 0) and + ((.integration_id == null) or (.integration_id | type == "number"))) + else true end)) + ' >/dev/null 2>&1 <<<"$response" || \ + die invalid_github_response "Branch rules returned an invalid required-status-check shape." \ + "Verify GitHub API access and retry." + REQUIRED_CONTEXTS=$(jq -cn --argjson existing "$REQUIRED_CONTEXTS" --argjson rules "$response" ' + [$existing[], + $rules[]? | select(.type == "required_status_checks") + | .parameters.required_status_checks[]?.context] + | unique | sort + ') + count=$(jq 'length' <<<"$response") + [[ $count -eq 100 ]] || break + page=$((page + 1)) + done + [[ $page -le 1000 ]] || \ + die pagination_limit "Branch rule pagination exceeded 1000 pages." \ + "Inspect the effective rules for $base_branch before retrying." + + gh_optional_json_404 api \ + "repos/$repo/branches/$encoded_branch/protection/required_status_checks" + if [[ $OPTIONAL_JSON_FOUND == true ]]; then + validate_json_response "status-check protection API" "$OPTIONAL_JSON" + jq -e ' + type == "object" and + ((.contexts // []) | type == "array") and + all((.contexts // [])[]; type == "string" and length > 0) and + ((.checks // []) | type == "array") and + all((.checks // [])[]; + type == "object" and + (.context | type == "string" and length > 0) and + ((.app_id == null) or (.app_id | type == "number"))) + ' >/dev/null 2>&1 <<<"$OPTIONAL_JSON" || \ + die invalid_github_response "Status-check protection returned an invalid context shape." \ + "Verify GitHub API access and retry." + REQUIRED_CONTEXTS=$(jq -cn --argjson existing "$REQUIRED_CONTEXTS" \ + --argjson protection "$OPTIONAL_JSON" ' + [$existing[], + ($protection.contexts // [])[], + ($protection.checks // [])[]?.context] + | unique | sort + ') + fi +} + collect_threads() { local repo=$1 local pr=$2 @@ -950,15 +1043,17 @@ command_snapshot() { die invalid_snapshot_state "Snapshots are allowed only from polling, not $current_status." \ "Handle $current_status with the state transition workflow before polling again." - local repo prior_head prior_head_repo prior_head_branch prior_host required_only reviewers_active cycle + local repo prior_head prior_head_repo prior_head_branch prior_base_branch prior_host required_only reviewers_active cycle repo=$(jq -r .repo <<<"$STATE") prior_head=$(jq -r .head_sha <<<"$STATE") prior_head_repo=$(jq -r .head_repo <<<"$STATE") prior_head_branch=$(jq -r .head_branch <<<"$STATE") + prior_base_branch=$(jq -r '.base_branch // ""' <<<"$STATE") prior_host=$(jq -r .github_host <<<"$STATE") validate_repo "$repo" validate_repo "$prior_head_repo" validate_branch state.head_branch "$prior_head_branch" + [[ -z $prior_base_branch ]] || validate_branch state.base_branch "$prior_base_branch" validate_sha state.head_sha "$prior_head" required_only=$(jq -r ' if .options.required_only == null then true else .options.required_only end @@ -966,9 +1061,9 @@ command_snapshot() { reviewers_active=$(jq -c '.reviewers.active // []' <<<"$STATE") cycle=$(jq -r '.cycle // 0' <<<"$STATE") - local before_json start_head start_head_repo start_head_branch start_host pr_state mergeable merge_state + local before_json start_head start_head_repo start_head_branch start_base_branch start_host pr_state mergeable merge_state before_json=$(gh_json pr view "$PR" --repo "$repo" \ - --json headRefOid,headRefName,headRepository,mergeable,mergeStateStatus,state,url) + --json headRefOid,headRefName,headRepository,baseRefName,mergeable,mergeStateStatus,state,url) validate_json_response "gh pr view" "$before_json" jq -e ' type == "object" and @@ -976,6 +1071,7 @@ command_snapshot() { (.headRefName | type == "string" and length > 0) and (.headRepository | type == "object") and (.headRepository.nameWithOwner | type == "string" and length > 0) and + (.baseRefName | type == "string" and length > 0) and (.url | type == "string" and length > 0) and ((.mergeable // "UNKNOWN") | type == "string") and ((.mergeStateStatus // "UNKNOWN") | type == "string") and @@ -987,21 +1083,28 @@ command_snapshot() { validate_sha headRefOid "$start_head" start_head_repo=$(jq -r .headRepository.nameWithOwner <<<"$before_json") start_head_branch=$(jq -r .headRefName <<<"$before_json") + start_base_branch=$(jq -r .baseRefName <<<"$before_json") validate_repo "$start_head_repo" validate_branch headRefName "$start_head_branch" + validate_branch baseRefName "$start_base_branch" parse_pr_host "$(jq -r .url <<<"$before_json")" start_host=$PR_HOST pr_state=$(jq -r '.state // "OPEN"' <<<"$before_json") mergeable=$(jq -r '.mergeable // "UNKNOWN"' <<<"$before_json") merge_state=$(jq -r '.mergeStateStatus // "UNKNOWN"' <<<"$before_json") - local checks_json + local checks_json required_checks_json required_contexts missing_required required_summary + collect_required_contexts "$repo" "$start_base_branch" + required_contexts=$REQUIRED_CONTEXTS if [[ $required_only == true ]]; then checks_json=$(gh_checks_json pr checks "$PR" --repo "$repo" --required \ --json bucket,name,state,link,workflow) + required_checks_json=$checks_json else checks_json=$(gh_checks_json pr checks "$PR" --repo "$repo" \ --json bucket,name,state,link,workflow) + required_checks_json=$(gh_checks_json pr checks "$PR" --repo "$repo" --required \ + --json bucket,name,state,link,workflow) fi jq -e ' type == "array" and all(.[]; @@ -1013,9 +1116,28 @@ command_snapshot() { ((.state // "") | type == "string")) ' >/dev/null 2>&1 <<<"$checks_json" || \ die invalid_github_response "gh pr checks returned an invalid nested shape." "Update gh and retry." + jq -e 'type == "array" and all(.[]; type == "object" and ((.name // "") | type == "string"))' \ + >/dev/null 2>&1 <<<"$required_checks_json" || \ + die invalid_github_response "Required checks returned an invalid nested shape." "Update gh and retry." + missing_required=$(jq -cn --argjson required "$required_contexts" \ + --argjson observed "$required_checks_json" ' + ($observed | map(.name // "") | unique) as $observed_names + | [$required[] as $context + | select(($observed_names | index($context)) == null) + | $context] + ') + required_summary=$(jq -cn --argjson configured "$required_contexts" \ + --argjson observed "$required_checks_json" --argjson missing "$missing_required" ' + ($observed | map(.name // "") | unique) as $observed_names + | {configured:$configured, + observed:[$configured[] as $context + | select(($observed_names | index($context)) != null) + | $context], + missing:$missing} + ') local check_details check_counts check_total - check_details=$(jq -c ' - [ .[]? as $check + check_details=$(jq -cn --argjson checks "$checks_json" --argjson missing "$missing_required" ' + ([ $checks[]? as $check | (($check.bucket // "pending") | ascii_downcase) as $bucket | {name:($check.name // ""),workflow:($check.workflow // ""),link:($check.link // ""), state:($check.state // ""), @@ -1024,8 +1146,9 @@ command_snapshot() { elif $bucket == "fail" then "fail" elif ($bucket == "cancel" or $bucket == "cancelled") then "cancel" else "pending" end)} ] + + [$missing[] | {name:.,workflow:"",link:"",state:"MISSING",bucket:"pending"}]) | sort_by(.workflow,.name,.link,.bucket,.state) - ' <<<"$checks_json" 2>/dev/null) || \ + ' 2>/dev/null) || \ die invalid_github_response "Unable to normalize gh pr checks data." "Inspect the check response and retry." check_counts=$(jq -c ' reduce .[]? as $check ({pass:0,skipping:0,fail:0,cancel:0,pending:0}; @@ -1054,9 +1177,10 @@ command_snapshot() { ') coderabbit_state=$(jq -r .coderabbit <<<"$reviewer_states") - local after_json end_head end_head_repo end_head_branch end_host after_pr_state + local after_json end_head end_head_repo end_head_branch end_base_branch end_host after_pr_state + local end_mergeable end_merge_state after_json=$(gh_json pr view "$PR" --repo "$repo" \ - --json headRefOid,headRefName,headRepository,state,url) + --json headRefOid,headRefName,headRepository,baseRefName,mergeable,mergeStateStatus,state,url) validate_json_response "gh pr view" "$after_json" jq -e ' type == "object" and @@ -1064,7 +1188,10 @@ command_snapshot() { (.headRefName | type == "string" and length > 0) and (.headRepository | type == "object") and (.headRepository.nameWithOwner | type == "string" and length > 0) and + (.baseRefName | type == "string" and length > 0) and (.url | type == "string" and length > 0) and + ((.mergeable // "UNKNOWN") | type == "string") and + ((.mergeStateStatus // "UNKNOWN") | type == "string") and ((.state // "OPEN") | type == "string") ' >/dev/null 2>&1 <<<"$after_json" || \ die invalid_github_response "Final gh pr view returned an invalid HEAD/state shape." "Confirm that PR $PR exists." @@ -1073,11 +1200,15 @@ command_snapshot() { validate_sha headRefOid "$end_head" end_head_repo=$(jq -r .headRepository.nameWithOwner <<<"$after_json") end_head_branch=$(jq -r .headRefName <<<"$after_json") + end_base_branch=$(jq -r .baseRefName <<<"$after_json") validate_repo "$end_head_repo" validate_branch headRefName "$end_head_branch" + validate_branch baseRefName "$end_base_branch" parse_pr_host "$(jq -r .url <<<"$after_json")" end_host=$PR_HOST after_pr_state=$(jq -r '.state // "OPEN"' <<<"$after_json") + end_mergeable=$(jq -r '.mergeable // "UNKNOWN"' <<<"$after_json") + end_merge_state=$(jq -r '.mergeStateStatus // "UNKNOWN"' <<<"$after_json") local now head_changed=false discarded=false deadlines checks_observed resolver_attempt now=$(now_epoch) @@ -1086,7 +1217,9 @@ command_snapshot() { resolver_attempt=$(jq -c '.resolver_attempt // null' <<<"$STATE") if [[ $prior_head != "$start_head" || $start_head != "$end_head" || \ $(lowercase "$prior_head_repo") != "$(lowercase "$start_head_repo")" || \ - $prior_head_branch != "$start_head_branch" || $prior_host != "$start_host" || \ + $prior_head_branch != "$start_head_branch" || \ + ( -n $prior_base_branch && $prior_base_branch != "$start_base_branch" ) || \ + $prior_host != "$start_host" || \ $(lowercase "$start_head_repo") != "$(lowercase "$end_head_repo")" || \ $start_head_branch != "$end_head_branch" || $start_host != "$end_host" ]]; then head_changed=true @@ -1101,12 +1234,14 @@ command_snapshot() { resolver_attempt=null fi + merge_state=$end_merge_state local status conflict=false merge_pending=false registration_pending=false checks_missing=false if [[ $pr_state == CLOSED || $pr_state == MERGED || $after_pr_state == CLOSED || $after_pr_state == MERGED ]]; then status=closed elif [[ $start_head != "$end_head" || \ $(lowercase "$start_head_repo") != "$(lowercase "$end_head_repo")" || \ - $start_head_branch != "$end_head_branch" || $start_host != "$end_host" ]]; then + $start_head_branch != "$end_head_branch" || $start_base_branch != "$end_base_branch" || \ + $start_host != "$end_host" || $mergeable != "$end_mergeable" ]]; then status=polling discarded=true checks_observed=false @@ -1129,31 +1264,36 @@ command_snapshot() { if [[ $conflict == true || $fail_count -gt 0 || $cancel_count -gt 0 || \ $unresolved_count -gt 0 ]]; then status=blocked - elif [[ $pending_count -gt 0 || $merge_pending == true || $registration_pending == true || $checks_missing == true ]]; then + elif [[ $pending_count -gt 0 || $merge_pending == true || \ + $registration_pending == true || $checks_missing == true ]]; then if [[ $now -ge $checks_deadline ]]; then status=timed_out; else status=polling; fi else status=clean fi fi - - local fingerprint_material fingerprint effective_head effective_head_repo effective_head_branch effective_host last_snapshot new_state + local fingerprint_material fingerprint effective_head effective_head_repo effective_head_branch effective_base_branch effective_host last_snapshot new_state effective_head=$end_head effective_head_repo=$end_head_repo effective_head_branch=$end_head_branch + effective_base_branch=$end_base_branch effective_host=$end_host if [[ $discarded == true ]]; then fingerprint_material=$(jq -cn --arg head "$effective_head" --arg head_repo "$effective_head_repo" \ - --arg head_branch "$effective_head_branch" --arg host "$effective_host" \ - '{head:$head,head_repo:$head_repo,head_branch:$head_branch,github_host:$host,discarded:true}') + --arg head_branch "$effective_head_branch" --arg base_branch "$effective_base_branch" \ + --arg host "$effective_host" \ + '{head:$head,head_repo:$head_repo,head_branch:$head_branch,base_branch:$base_branch, + github_host:$host,discarded:true}') else fingerprint_material=$(jq -cn --arg head "$effective_head" --argjson checks "$check_details" \ --arg head_repo "$effective_head_repo" --arg head_branch "$effective_head_branch" \ - --arg host "$effective_host" \ + --arg base_branch "$effective_base_branch" --arg host "$effective_host" \ + --argjson required "$required_contexts" \ --argjson conflict "$conflict" --argjson merge_pending "$merge_pending" \ - --argjson threads "$THREAD_DETAILS" --argjson reviewers "$reviewer_states" \ - '{head:$head,head_repo:$head_repo,head_branch:$head_branch,github_host:$host, - checks:$checks,conflict:$conflict,merge_pending:$merge_pending, - threads:$threads,reviewers:$reviewers}') + --argjson threads "$THREAD_DETAILS" \ + '{head:$head,head_repo:$head_repo,head_branch:$head_branch,base_branch:$base_branch, + github_host:$host,required:$required,checks:$checks,conflict:$conflict, + merge_pending:$merge_pending, + threads:$threads}') fi fingerprint=$(printf '%s' "$fingerprint_material" | git hash-object --stdin) if [[ $status == blocked && $resolver_attempt != null && \ @@ -1162,27 +1302,33 @@ command_snapshot() { status=stalled fi last_snapshot=$(jq -cn --argjson checks "$check_counts" --argjson check_details "$check_details" \ - --argjson conflict "$conflict" --argjson threads "$THREAD_DETAILS" \ + --argjson required_checks "$required_summary" \ + --arg merge_state "$merge_state" --argjson conflict "$conflict" \ + --argjson threads "$THREAD_DETAILS" \ --argjson unresolved "$unresolved_count" --argjson pages "$THREAD_PAGES" \ --arg coderabbit "$coderabbit_state" --argjson reviewers "$reviewer_states" \ --argjson reviewer_required "$reviewer_required" --argjson observed_at "$now" \ --argjson reviewer_telemetry_available "$reviewer_telemetry_available" \ --argjson registration_pending "$registration_pending" --argjson checks_missing "$checks_missing" \ - '{checks:$checks,check_details:$check_details,conflict:$conflict,review_threads:$threads, + '{checks:$checks,check_details:$check_details,required_checks:$required_checks, + merge_state:$merge_state, + conflict:$conflict,review_threads:$threads, unresolved_actionable_threads:$unresolved,review_thread_pages:$pages,coderabbit:$coderabbit, reviewers:{states:$reviewers,required:$reviewer_required, telemetry_available:$reviewer_telemetry_available}, registration_pending:$registration_pending,checks_missing:$checks_missing,observed_at:$observed_at}') if [[ $discarded == true ]]; then last_snapshot=$(jq -cn --argjson observed_at "$now" \ - '{discarded:true,reason:"head_changed_during_snapshot",observed_at:$observed_at}') + '{discarded:true,reason:"snapshot_binding_changed",observed_at:$observed_at}') fi new_state=$(jq -c --arg head "$effective_head" --arg head_repo "$effective_head_repo" \ - --arg head_branch "$effective_head_branch" --arg github_host "$effective_host" --arg status "$status" \ + --arg head_branch "$effective_head_branch" --arg base_branch "$effective_base_branch" \ + --arg github_host "$effective_host" --arg status "$status" \ --argjson deadlines "$deadlines" --arg fingerprint "$fingerprint" \ --argjson snapshot "$last_snapshot" --argjson checks_observed "$checks_observed" \ --argjson reviewers_active "$reviewers_active" \ - '. + {head_sha:$head,head_repo:$head_repo,head_branch:$head_branch,github_host:$github_host, + '. + {head_sha:$head,head_repo:$head_repo,head_branch:$head_branch,base_branch:$base_branch, + github_host:$github_host, status:$status,deadlines:$deadlines, blocker_fingerprint:$fingerprint,checks_observed:$checks_observed, reviewers:{active:$reviewers_active}, @@ -1191,6 +1337,7 @@ command_snapshot() { jq -cn --argjson state "$new_state" --argjson checks "$check_counts" \ --argjson check_details "$check_details" --argjson threads "$THREAD_DETAILS" \ + --argjson required_checks "$required_summary" --arg merge_state "$merge_state" \ --argjson head_changed "$head_changed" --argjson discarded "$discarded" \ --argjson unresolved "$unresolved_count" --argjson pages "$THREAD_PAGES" \ --arg coderabbit "$coderabbit_state" --argjson reviewers_active "$reviewers_active" \ @@ -1198,6 +1345,7 @@ command_snapshot() { --argjson reviewer_telemetry_available "$reviewer_telemetry_available" \ --argjson dry_run "$dry_run" \ '$state + {ok:true,command:"snapshot",checks:$checks,check_details:$check_details, + required_checks:$required_checks,merge_state:$merge_state, review_threads:$threads,head_changed:$head_changed, discarded:$discarded,unresolved_actionable_threads:$unresolved, review_thread_pages:$pages,coderabbit:$coderabbit, diff --git a/plugins/skill-set/tests/fixtures/mock-gh-pr b/plugins/skill-set/tests/fixtures/mock-gh-pr index 8e9f75f..30fec4f 100755 --- a/plugins/skill-set/tests/fixtures/mock-gh-pr +++ b/plugins/skill-set/tests/fixtures/mock-gh-pr @@ -86,6 +86,8 @@ if [[ ${1:-} == pr && ${2:-} == view ]]; then if [[ $MOCK_GH_SCENARIO == conflict ]]; then mergeable=CONFLICTING merge_state=DIRTY + elif [[ $MOCK_GH_SCENARIO == merge-state-blocked ]]; then + merge_state=BLOCKED elif [[ $MOCK_GH_SCENARIO == closed ]]; then pr_state=CLOSED elif [[ $MOCK_GH_SCENARIO == close-race && $count -gt 2 ]]; then @@ -102,6 +104,40 @@ if [[ ${1:-} == pr && ${2:-} == view ]]; then exit 0 fi +if [[ ${1:-} == api && ${2:-} == repos/*/rules/branches/* ]]; then + if [[ $MOCK_GH_SCENARIO == malformed-rules ]]; then + printf '%s\n' '[{"type":"required_status_checks","parameters":{"required_status_checks":[{"context":42}]}}]' + elif [[ $MOCK_GH_SCENARIO == missing-required-review ]]; then + jq -cn '[{type:"required_status_checks",parameters:{required_status_checks:[ + {context:"verify",integration_id:null}, + {context:"signal-gated-review",integration_id:null} + ]}}]' + elif [[ $MOCK_GH_SCENARIO == no-required ]]; then + printf '%s\n' '[]' + elif [[ $MOCK_GH_SCENARIO == signal-gated-* ]]; then + jq -cn '[{type:"required_status_checks",parameters:{required_status_checks:[ + {context:"review",integration_id:null} + ]}}]' + else + jq -cn '[{type:"required_status_checks",parameters:{required_status_checks:[ + {context:"verify",integration_id:null} + ]}}]' + fi + exit 0 +fi + +if [[ ${1:-} == api && ${2:-} == repos/*/branches/*/protection/required_status_checks* ]]; then + if [[ $MOCK_GH_SCENARIO == legacy-missing-required ]]; then + printf '%s\n' '{"strict":false,"contexts":["legacy-review"],"checks":[]}' + exit 0 + elif [[ $MOCK_GH_SCENARIO == malformed-protection ]]; then + printf '%s\n' '{"strict":false,"contexts":[42],"checks":[]}' + exit 0 + fi + printf '%s\n' 'gh: Branch not protected (HTTP 404)' >&2 + exit 1 +fi + if [[ ${1:-} == pr && ${2:-} == comment ]]; then body_file= while [[ $# -gt 0 ]]; do @@ -127,6 +163,13 @@ if [[ ${1:-} == pr && ${2:-} == checks ]]; then printf "%s\n" "no required checks reported on the 'feature' branch" >&2 exit 1 fi + if [[ $MOCK_GH_SCENARIO == missing-required-review && $count -gt 1 ]]; then + jq -cn '[ + {bucket:"pass",name:"verify",state:"PASS",link:"https://example.test/check/verify",workflow:"CI"}, + {bucket:"pass",name:"signal-gated-review",state:"PASS",link:"https://example.test/check/signal-gated-review",workflow:"Signal-Gated PR Review"} + ]' + exit 0 + fi bucket=$MOCK_GH_SCENARIO check_name=verify workflow=CI @@ -138,6 +181,8 @@ if [[ ${1:-} == pr && ${2:-} == checks ]]; then bucket=pass check_name=review workflow='Signal-Gated PR Review' + elif [[ $MOCK_GH_SCENARIO == coderabbit-fingerprint ]]; then + bucket=pending elif [[ $MOCK_GH_SCENARIO == registration-delay ]]; then if [[ $count -eq 1 ]]; then printf "%s\n" "no required checks reported on the 'feature' branch" >&2 @@ -153,6 +198,10 @@ if [[ ${1:-} == pr && ${2:-} == checks ]]; then elif [[ $MOCK_GH_SCENARIO == fail-b ]]; then bucket=fail check_name='test' + elif [[ $MOCK_GH_SCENARIO == missing-required-review ]]; then + bucket=pass + elif [[ $MOCK_GH_SCENARIO == legacy-missing-required ]]; then + bucket=pass fi [[ $bucket == delayed && $count -eq 1 ]] && bucket=pending [[ $bucket == delayed && $count -gt 1 ]] && bucket=pass @@ -268,6 +317,10 @@ if [[ ${1:-} == api && ${2:-} == repos/*/commits/*/status ]]; then count=$(next_count coderabbit) state=pending [[ $count -gt 1 ]] && state=success + elif [[ $MOCK_GH_SCENARIO == coderabbit-fingerprint ]]; then + count=$(next_count coderabbit) + state=pending + [[ $count -gt 1 ]] && state=success elif [[ $MOCK_GH_SCENARIO == coderabbit-failure ]]; then state=failure elif [[ $MOCK_GH_SCENARIO == reviewers-active ]]; then diff --git a/plugins/skill-set/tests/test-pr-runner.sh b/plugins/skill-set/tests/test-pr-runner.sh index f3fe89d..c5c4dbe 100755 --- a/plugins/skill-set/tests/test-pr-runner.sh +++ b/plugins/skill-set/tests/test-pr-runner.sh @@ -159,9 +159,8 @@ export MOCK_GH_SCENARIO=fail init_case --required-only false >/dev/null all_checks=$(snapshot_case 101) assert_equals blocked "$(jq -r .status <<<"$all_checks")" "all-check failure state" -if grep -Eq '^pr checks .* --required( |$)' "$MOCK_GH_LOG"; then - fail "--required-only false still passed --required to gh pr checks" -fi +assert_equals 2 "$(count_log '^pr checks ')" "all-check and required-inventory query count" +assert_equals 1 "$(count_log '^pr checks .* --required( |$)')" "required-inventory query count" make_fixture skipping export MOCK_GH_SCENARIO=skipping @@ -187,6 +186,50 @@ assert_equals polling "$(jq -r .status <<<"$registered_pending")" "registered pe registered_pass=$(snapshot_case 120) assert_equals clean "$(jq -r .status <<<"$registered_pass")" "registered passing check" +make_fixture missing-required-review +export MOCK_GH_SCENARIO=missing-required-review +init_case >/dev/null +missing_required_review=$(snapshot_case 101) +jq -e ' + .status == "polling" and + .checks.pass == 1 and + .checks.pending == 1 and + .required_checks.configured == ["signal-gated-review","verify"] and + .required_checks.observed == ["verify"] and + .required_checks.missing == ["signal-gated-review"] +' <<<"$missing_required_review" >/dev/null +required_review_complete=$(snapshot_case 102) +jq -e ' + .status == "clean" and + .checks.pass == 2 and + .checks.pending == 0 and + .required_checks.observed == ["signal-gated-review","verify"] and + .required_checks.missing == [] +' <<<"$required_review_complete" >/dev/null + +make_fixture legacy-missing-required +export MOCK_GH_SCENARIO=legacy-missing-required +init_case >/dev/null +legacy_missing_required=$(snapshot_case 101) +jq -e ' + .status == "polling" and + .checks.pass == 1 and + .checks.pending == 1 and + .required_checks.configured == ["legacy-review","verify"] and + .required_checks.missing == ["legacy-review"] +' <<<"$legacy_missing_required" >/dev/null + +make_fixture merge-state-blocked +export MOCK_GH_SCENARIO=merge-state-blocked +init_case >/dev/null +merge_state_blocked=$(snapshot_case 101) +jq -e ' + .status == "clean" and + .checks.pass == 1 and + .checks.pending == 0 and + .merge_state == "BLOCKED" +' <<<"$merge_state_blocked" >/dev/null + make_fixture signal-gated-running export MOCK_GH_SCENARIO=signal-gated-running signal_gated_init=$(init_case) @@ -437,6 +480,19 @@ cr_timeout=$(snapshot_case 106) jq -e '.status == "clean" and .reviewers.states.coderabbit == "pending" and .reviewers.required.coderabbit == false' <<<"$cr_timeout" >/dev/null +make_fixture coderabbit-fingerprint +export MOCK_GH_SCENARIO=coderabbit-fingerprint +init_case >/dev/null +reviewer_pending=$(snapshot_case 101) +reviewer_complete=$(snapshot_case 102) +jq -e '.status == "polling" and .reviewers.states.coderabbit == "pending"' \ + <<<"$reviewer_pending" >/dev/null +jq -e '.status == "polling" and .reviewers.states.coderabbit == "success"' \ + <<<"$reviewer_complete" >/dev/null +assert_equals "$(jq -r .blocker_fingerprint <<<"$reviewer_pending")" \ + "$(jq -r .blocker_fingerprint <<<"$reviewer_complete")" \ + "optional reviewer telemetry does not affect blocker fingerprint" + make_fixture coderabbit-auto export MOCK_GH_SCENARIO=coderabbit-active auto_init=$(init_case) @@ -1002,7 +1058,7 @@ export MOCK_GH_SCENARIO=malformed-pr-list malformed_list=$(run_fail init --pr 17 --repo owner/repo) assert_equals invalid_github_response "$(jq -r .error.code <<<"$malformed_list")" "nested PR list response" -for scenario in malformed-checks malformed-threads; do +for scenario in malformed-checks malformed-threads malformed-rules malformed-protection; do make_fixture "$scenario" export MOCK_GH_SCENARIO=$scenario initialized=$(init_case)