diff --git a/.github/workflows/initiative-planner.yml b/.github/workflows/initiative-planner.yml index 7a2969d48..cbe7fc9f6 100644 --- a/.github/workflows/initiative-planner.yml +++ b/.github/workflows/initiative-planner.yml @@ -48,44 +48,11 @@ concurrency: cancel-in-progress: false jobs: - # ── discussion [labeled] bridge ─────────────────────────────────────────────── - # claude-code-action aborts on `discussion` event contexts ("Unsupported event - # type: discussion"), so we never plan inline on a discussion event. When a - # maintainer adds `idea:approved`, this job resolves the discussion number and - # re-invokes THIS workflow via workflow_dispatch — under which the action runs. - # The label stays the human gate; the re-dispatch is an invisible bridge. - redispatch: - if: github.event_name == 'discussion' && github.event.label.name == 'idea:approved' - runs-on: ubuntu-latest - timeout-minutes: 5 - permissions: - contents: read # actions/checkout; the workflow_dispatch is fired with the PAT - steps: - - name: Checkout agent repo - uses: actions/checkout@df4cb1c069e1874edd31b4311f1884172cec0e10 # v6.0.3 - with: - persist-credentials: false - - - name: Guard — PAT present - # PAT required: a workflow_dispatch fired with GITHUB_TOKEN is accepted but - # NEVER starts a run (loop prevention) — the planner would silently no-op. - env: - GH_TOKEN: ${{ secrets.GH_PAT_WORKFLOWS }} - run: | - if [ -z "${GH_TOKEN}" ]; then - echo "::error::GH_PAT_WORKFLOWS is required — a workflow_dispatch fired with GITHUB_TOKEN will not start a run." - exit 1 - fi - - - name: Re-dispatch under workflow_dispatch - env: - GH_TOKEN: ${{ secrets.GH_PAT_WORKFLOWS }} - REPO: ${{ github.repository }} - DISCUSSION_NUMBER: ${{ github.event.discussion.number }} - run: bash scripts/initiative-planner/redispatch.sh - plan: - if: github.event_name == 'workflow_dispatch' + # Skip discussion-label events unless the label added is exactly idea:approved. + if: >- + github.event_name == 'workflow_dispatch' || + (github.event_name == 'discussion' && github.event.label.name == 'idea:approved') runs-on: ubuntu-latest timeout-minutes: 30 permissions: @@ -94,11 +61,9 @@ jobs: discussions: write # post the plan summary back to the idea id-token: write # claude-code-action OIDC env: - # This job only runs on workflow_dispatch (the discussion path re-dispatches - # here via the `redispatch` job), so inputs are always present. REPO: ${{ github.repository }} - DISCUSSION_NUMBER: ${{ github.event.inputs.discussion }} - DRY_RUN: ${{ inputs.dry_run == true && '1' || '0' }} + DISCUSSION_NUMBER: ${{ github.event.inputs.discussion || github.event.discussion.number }} + DRY_RUN: ${{ (github.event_name == 'workflow_dispatch' && inputs.dry_run == true) && '1' || '0' }} TOOLING_DIR: ${{ github.workspace }}/scripts/initiative-planner steps: - name: Checkout agent repo diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 83da9c441..7dbe41145 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -52,8 +52,8 @@ jobs: tests/test_cut_release.bats \ tests/test_gitleaks_config.bats \ tests/test_initiative_planner.bats \ - tests/test_initiative_planner_redispatch.bats \ - tests/test_idea_triage.bats + tests/test_idea_triage.bats \ + tests/test_oversized_pr.bats validate-agent-profiles: runs-on: ubuntu-latest diff --git a/AGENTS.md b/AGENTS.md index 306ba29fa..99a861c4a 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -56,6 +56,20 @@ This is the `.github-private` org infrastructure repo for `petry-projects`. It c - Scripts must be POSIX-compatible shell (`#!/usr/bin/env bash` with `set -euo pipefail`). - No hardcoded tokens or secrets — use `$GITHUB_TOKEN` from the environment. +### Oversized PRs (> 300 changed files) + +GitHub's unified-diff endpoint (`gh pr diff`) is hard-capped at 300 changed files and returns +HTTP 406 above that limit. + +- **PR-size guideline:** PRs should stay under 300 changed files to receive a full automated review. + PRs exceeding 300 files are reviewed with a truncated diff and should be split where practical. + This guideline is advisory — no CI gate enforces it. +- **Agent behavior on 406:** When `gh pr diff` returns the 406 "diff exceeded the maximum number of + files" error, the cascade falls back to the per-file REST API (`/pulls/{n}/files`), assembles a + truncated diff up to the configured line limit, and continues the review with a note that the diff + is partial. The agent **never** exits with a fatal error (code 1) on the 406 — it always produces + an actionable review or comment. + ### Cost reporting - **All surfaced USD amounts are rounded to 2 decimals (cents).** Render every diff --git a/scripts/engine.sh b/scripts/engine.sh index b953cbb62..d6c94421d 100644 --- a/scripts/engine.sh +++ b/scripts/engine.sh @@ -293,6 +293,16 @@ is_cli_error() { "(invalid command format|invalid (flag|argument|option|command)|unknown (flag|command|option|argument)|command not found|no such command|did you mean:|unrecognized (command|flag|argument|option)|bad (flag|argument|option))" } +# is_diff_too_large +# Returns 0 (true) if the text indicates that the PR diff exceeded GitHub's +# hard 300-file unified-diff cap (HTTP 406). Distinct from rate-limit (429/529) +# and CLI invocation errors — callers should fall back to the per-file REST API +# rather than exiting 1 (per-PR failure) or 2 (engine rate-limit). +is_diff_too_large() { + local text="$1" + printf '%s\n' "$text" | grep -qiE "(HTTP 406|exceeded the maximum number of files|diff exceeded the maximum)" +} + # is_transient_failure # Returns 0 (true) for exit codes suggesting a flaky network/process state: # 124 (GNU timeout) and 137/143 (signal kills). JSON parse failures and diff --git a/scripts/initiative-planner/README.md b/scripts/initiative-planner/README.md index 2027c6d90..9b73f39fb 100644 --- a/scripts/initiative-planner/README.md +++ b/scripts/initiative-planner/README.md @@ -13,14 +13,13 @@ create-story template, which `plan.schema.json` mirrors field-for-field. | File | Role | |------|------| -| `redispatch.sh` | Bridge the `discussion [labeled]` trigger to `workflow_dispatch` (claude-code-action rejects `discussion` event contexts). Fired with a PAT so the dispatch actually starts a run. | | `gather-context.sh` | Fetch the approved Discussion + repo context → `$CONTEXT_PATH`; export `DISCUSSION_NODE_ID`. | | `plan.schema.json` | The plan contract Bob must emit (epic + stories + `blocked_by`). | | `validate-plan.py` | Schema + semantic checks: unique ids, acyclic DAG, an entry point, no dangling edges. | | `lib/mutations.sh` | DRY_RUN-aware GitHub helpers (create issue, link sub-issue, add `blocked_by`, comment on Discussion). | | `apply-plan.sh` | Materialize a validated plan. Creates issues labelled `initiative` only — **never** `initiative:auto`. | -Tests: `tests/test_initiative_planner.bats`, `tests/test_initiative_planner_redispatch.bats` (run via `lint.yml`). +Tests: `tests/test_initiative_planner.bats` (run via `lint.yml`). ## Local dry run diff --git a/scripts/initiative-planner/redispatch.sh b/scripts/initiative-planner/redispatch.sh deleted file mode 100755 index 52118d44a..000000000 --- a/scripts/initiative-planner/redispatch.sh +++ /dev/null @@ -1,58 +0,0 @@ -#!/usr/bin/env bash -# redispatch.sh — bridge a `discussion [labeled]` event to a supported event. -# -# The initiative planner's documented primary trigger is a maintainer adding -# `idea:approved` to an Ideas Discussion, which fires `on: discussion [labeled]`. -# But the planner's "Plan the initiative" step uses claude-code-action, which -# only recognizes issues / pull_request / issue_comment / workflow_dispatch event -# contexts and aborts on a `discussion` event: -# -# Action failed with error: Unsupported event type: discussion -# -# So on the discussion event we DON'T plan inline; we resolve the discussion -# number and re-invoke the SAME workflow via workflow_dispatch, under which the -# action runs cleanly. The `idea:approved` label stays the human gate; the -# re-dispatch is an invisible bridge. -# -# IMPORTANT — the re-dispatch requires a PAT, not GITHUB_TOKEN: -# GitHub does not start new workflow runs from events created with the default -# GITHUB_TOKEN (loop prevention). A workflow_dispatch fired with GITHUB_TOKEN -# would be accepted but would NEVER start a run, so the caller MUST provide a -# PAT (GH_PAT_WORKFLOWS) as GH_TOKEN. This is the same constraint documented in -# scripts/initiative-driver.sh for the dev-lead label trigger. -# -# Env: -# REPO owner/repo (required) -# DISCUSSION_NUMBER Ideas Discussion number to plan (required) -# WORKFLOW_FILE workflow to dispatch (default: initiative-planner.yml) -# GH_TOKEN a PAT with workflow scope (required at the call site) -set -euo pipefail - -REPO="${REPO:?REPO required}" -DISCUSSION_NUMBER="${DISCUSSION_NUMBER:?DISCUSSION_NUMBER required}" -WORKFLOW_FILE="${WORKFLOW_FILE:-initiative-planner.yml}" - -# Validate before the value reaches the gh command line. -[[ "$DISCUSSION_NUMBER" =~ ^[0-9]+$ ]] || { - echo "::error::DISCUSSION_NUMBER must be a positive integer, got: '$DISCUSSION_NUMBER'" >&2 - exit 1 -} - -# Map DRY_RUN to a boolean string for the workflow input -if [[ "${DRY_RUN:-}" == "1" || "${DRY_RUN:-}" == "true" ]]; then - DISPATCH_DRY_RUN="true" -else - DISPATCH_DRY_RUN="false" -fi - -# Use GITHUB_REF if it is a branch or tag to support testing on feature branches -REF_FLAGS=() -if [[ -n "${GITHUB_REF:-}" && ( "$GITHUB_REF" =~ ^refs/heads/ || "$GITHUB_REF" =~ ^refs/tags/ ) ]]; then - REF_FLAGS=(--ref "$GITHUB_REF") -fi - -echo "Discussion event → re-dispatching ${WORKFLOW_FILE} for discussion #${DISCUSSION_NUMBER} via workflow_dispatch." -gh workflow run "$WORKFLOW_FILE" --repo "$REPO" "${REF_FLAGS[@]}" \ - -f discussion="$DISCUSSION_NUMBER" \ - -f dry_run="$DISPATCH_DRY_RUN" -echo "Dispatched ${WORKFLOW_FILE} (discussion=${DISCUSSION_NUMBER}, dry_run=${DISPATCH_DRY_RUN})." diff --git a/scripts/review-one-pr.sh b/scripts/review-one-pr.sh index 8d4018aaa..f4c9f321f 100644 --- a/scripts/review-one-pr.sh +++ b/scripts/review-one-pr.sh @@ -429,10 +429,36 @@ else echo "{\"pr\":\"$PR_URL\",\"sha\":\"$PR_HEAD_SHA\",\"decision\":\"skip\",\"reason\":\"gh-rate-limited\"}" exit 100 fi - echo "::error::gh pr diff failed during prefetch for $PR_URL" - [ -n "$_gh_diff_err_content" ] && echo " $_gh_diff_err_content" - rm -f "$_gh_meta_err" "$_gh_diff_err" "$_gh_diff_tmp" - exit 1 + if is_diff_too_large "$_gh_diff_err_content"; then + # GitHub's unified-diff endpoint is hard-capped at 300 changed files (HTTP 406). + # Fall back to the per-file REST API which works above the cap: each entry carries + # its own patch, so we assemble a truncated diff and continue the review normally. + _changed_files=$(echo "$PR_METADATA" | jq -r '.changedFiles // "unknown"' 2>/dev/null || echo "unknown") + echo " diff too large (${_changed_files} files exceeds GitHub 300-file unified-diff cap) — assembling diff from per-file REST API" + _owner_repo_fallback=$(echo "$PR_URL" | sed -E 's|https://github.com/([^/]+/[^/]+)/pull/.*|\1|') + _pr_num_fallback=${PR_URL##*/} + _fallback_tmp=$(mktemp 2>/dev/null || echo "/tmp/cascade/fallback-diff-$$.txt") + _fallback_err=$(mktemp 2>/dev/null || echo "/tmp/cascade/fallback-err-$$.txt") + { + printf '# NOTE: diff assembled from per-file REST API (%s changed files exceeded\n' "$_changed_files" + printf "# GitHub's 300-file unified-diff cap; patches may be partial or absent for large files).\n" + gh api "repos/$_owner_repo_fallback/pulls/$_pr_num_fallback/files" \ + --paginate --slurp \ + | jq -r 'add | .[]? | if .patch then "diff --git a/\(.filename) b/\(.filename)\n--- a/\(.filename)\n+++ b/\(.filename)\n\(.patch)" else "# \(.status): \(.filename) (no patch — binary or large file)" end' \ + 2>"$_fallback_err" || { + echo "::warning::REST API fallback also failed — review will have partial diff" >&2 + cat "$_fallback_err" >&2 || true + } + } > "$_fallback_tmp" + PR_DIFF=$(head -"$_diff_limit" "$_fallback_tmp") + rm -f "$_fallback_tmp" "$_fallback_err" + unset _changed_files _owner_repo_fallback _pr_num_fallback _fallback_tmp _fallback_err + else + echo "::error::gh pr diff failed during prefetch for $PR_URL" + [ -n "$_gh_diff_err_content" ] && echo " $_gh_diff_err_content" + rm -f "$_gh_meta_err" "$_gh_diff_err" "$_gh_diff_tmp" + exit 1 + fi fi rm -f "$_gh_meta_err" "$_gh_diff_err" "$_gh_diff_tmp" unset _gh_meta_err _gh_diff_err _gh_diff_tmp _gh_meta_err_content _gh_diff_err_content diff --git a/tests/test_initiative_planner_redispatch.bats b/tests/test_initiative_planner_redispatch.bats deleted file mode 100644 index 5b780015e..000000000 --- a/tests/test_initiative_planner_redispatch.bats +++ /dev/null @@ -1,106 +0,0 @@ -#!/usr/bin/env bats -# Unit tests for scripts/initiative-planner/redispatch.sh -# -# The planner's primary trigger is a `discussion [labeled]` event, but -# claude-code-action aborts on `discussion` event contexts. redispatch.sh bridges -# that gap by re-invoking the workflow via workflow_dispatch. These tests mock the -# gh CLI to avoid network calls and assert on the dispatched command. -# -# Run with: bats tests/test_initiative_planner_redispatch.bats - -SCRIPT="$(cd "$(dirname "$BATS_TEST_FILENAME")/.." && pwd)/scripts/initiative-planner/redispatch.sh" - -setup() { - MOCK_BIN="$(mktemp -d)" - GH_LOG="$(mktemp)" - export GH_LOG - export PATH="$MOCK_BIN:$PATH" - export REPO="petry-projects/.github-private" - export DISCUSSION_NUMBER="572" - export GH_TOKEN="pat" - - cat > "$MOCK_BIN/gh" <<'EOF' -#!/usr/bin/env bash -printf '%s\n' "$*" >> "$GH_LOG" -exit 0 -EOF - chmod +x "$MOCK_BIN/gh" -} - -teardown() { - rm -rf "${MOCK_BIN:-}" - rm -f "${GH_LOG:-}" -} - -@test "re-dispatches the planner workflow via workflow_dispatch" { - run bash "$SCRIPT" - [ "$status" -eq 0 ] - grep -qF "workflow run initiative-planner.yml" "$GH_LOG" -} - -@test "threads the discussion number and forces dry_run=false" { - run bash "$SCRIPT" - [ "$status" -eq 0 ] - grep -qF -- "-f discussion=572" "$GH_LOG" - grep -qF -- "-f dry_run=false" "$GH_LOG" -} - -@test "targets the correct repo" { - run bash "$SCRIPT" - [ "$status" -eq 0 ] - grep -qF -- "--repo petry-projects/.github-private" "$GH_LOG" -} - -@test "fails when DISCUSSION_NUMBER is unset" { - unset DISCUSSION_NUMBER - run bash "$SCRIPT" - [ "$status" -ne 0 ] - [ ! -s "$GH_LOG" ] -} - -@test "rejects a non-numeric DISCUSSION_NUMBER without dispatching" { - export DISCUSSION_NUMBER="572; rm -rf /" - run bash "$SCRIPT" - [ "$status" -ne 0 ] - [[ "$output" == *"positive integer"* ]] - [ ! -s "$GH_LOG" ] -} - -@test "fails when REPO is unset" { - unset REPO - run bash "$SCRIPT" - [ "$status" -ne 0 ] -} - -@test "honors a WORKFLOW_FILE override" { - export WORKFLOW_FILE="other-planner.yml" - run bash "$SCRIPT" - [ "$status" -eq 0 ] - grep -qF "workflow run other-planner.yml" "$GH_LOG" -} - -@test "honors DRY_RUN=1 or DRY_RUN=true" { - export DRY_RUN="1" - run bash "$SCRIPT" - [ "$status" -eq 0 ] - grep -qF -- "-f dry_run=true" "$GH_LOG" - - export DRY_RUN="true" - run bash "$SCRIPT" - [ "$status" -eq 0 ] - grep -qF -- "-f dry_run=true" "$GH_LOG" -} - -@test "passes GITHUB_REF if it is a branch or tag" { - export GITHUB_REF="refs/heads/feature-branch" - run bash "$SCRIPT" - [ "$status" -eq 0 ] - grep -qF -- "--ref refs/heads/feature-branch" "$GH_LOG" -} - -@test "ignores GITHUB_REF if it is a pull request ref" { - export GITHUB_REF="refs/pull/123/merge" - run bash "$SCRIPT" - [ "$status" -eq 0 ] - ! grep -qF -- "--ref" "$GH_LOG" -} diff --git a/tests/test_oversized_pr.bats b/tests/test_oversized_pr.bats new file mode 100644 index 000000000..e38043662 --- /dev/null +++ b/tests/test_oversized_pr.bats @@ -0,0 +1,72 @@ +#!/usr/bin/env bats +# Unit tests for is_diff_too_large in scripts/engine.sh. +# Validates that the 406 "diff too large" detection function correctly +# identifies GitHub's hard 300-file unified-diff cap error — distinct from +# rate-limit (429/529) and CLI invocation errors. +# +# Run with: bats tests/test_oversized_pr.bats + +setup() { + export REVIEW_ENGINE="claude" + # Capture (and discard) the "engine: ..." echo that fires on source. + source "$(dirname "$BATS_TEST_FILENAME")/../scripts/engine.sh" >/dev/null 2>&1 || true +} + +# --------------------------------------------------------------------------- +# is_diff_too_large: must return TRUE (exit 0) for 406 "too many files" errors +# --------------------------------------------------------------------------- + +@test "is_diff_too_large: 'HTTP 406' in error text returns true" { + run is_diff_too_large "could not find pull request diff: HTTP 406" + [ "$status" -eq 0 ] +} + +@test "is_diff_too_large: 'exceeded the maximum number of files' returns true" { + run is_diff_too_large "Sorry, the diff exceeded the maximum number of files (300)." + [ "$status" -eq 0 ] +} + +@test "is_diff_too_large: full GitHub 406 error message returns true" { + run is_diff_too_large "could not find pull request diff: HTTP 406: Sorry, the diff exceeded the maximum number of files (300). Consider using 'List pull requests files' API or locally cloning the repository instead." + [ "$status" -eq 0 ] +} + +@test "is_diff_too_large: 'diff exceeded the maximum' returns true" { + run is_diff_too_large "the diff exceeded the maximum size limit" + [ "$status" -eq 0 ] +} + +# --------------------------------------------------------------------------- +# is_diff_too_large: must return FALSE (exit 1) to avoid false positives +# --------------------------------------------------------------------------- + +@test "is_diff_too_large: rate-limit error returns false" { + run is_diff_too_large "rate limit exceeded" + [ "$status" -eq 1 ] +} + +@test "is_diff_too_large: HTTP 429 error returns false" { + run is_diff_too_large "Error: too many requests (429)" + [ "$status" -eq 1 ] +} + +@test "is_diff_too_large: CLI invocation error returns false" { + run is_diff_too_large "error: Invalid command format." + [ "$status" -eq 1 ] +} + +@test "is_diff_too_large: generic diff failure returns false" { + run is_diff_too_large "gh pr diff: diff not available for this PR" + [ "$status" -eq 1 ] +} + +@test "is_diff_too_large: empty string returns false" { + run is_diff_too_large "" + [ "$status" -eq 1 ] +} + +@test "is_diff_too_large: unrelated 406 text without diff context returns false" { + # Guard: a bare '406' not from the diff endpoint should not fire. + run is_diff_too_large "request returned 406 for some other API" + [ "$status" -eq 1 ] +}