Clean, well-structured fix for the envs_size_too_large blocker. The root cause analysis is solid and the two-pronged approach (per-PR teardown + one-time bulk cleanup) is the right call. One issue worth addressing around API pagination in the cleanup script.

  ^{｜ Fix all ➔ ｜ Fix 👍s ➔ ｜ View workflow run ｜ Using Claude Opus ｜ 𝕏}

PR Review Summary

(0) Total Issues | Risk: Low

This PR correctly addresses the root cause of envs_size_too_large errors by adding cleanup mechanisms for branch-scoped Vercel preview env vars. The implementation is well-structured and follows established patterns from existing preview scripts.

💭 Consider (2) 💭

💭 1) cleanup-vercel-stale-env.sh:76 / teardown-vercel-preview-env.sh:60 Capture API errors for debugging

Issue: Both scripts suppress curl DELETE output (>/dev/null 2>&1), so when deletions fail the warning message doesn't include the actual Vercel API error response.

Why: Makes it harder to diagnose issues like rate limiting, auth problems, or API changes. The teardown script runs on every PR close, so silent failures could accumulate.

Fix: Capture stderr to a temp file and include the API error in warning output, similar to how the list operation already captures and reports errors:

local err_file
err_file="$(mktemp)"
if ! curl ... -X DELETE ... 2>"${err_file}"; then
  echo "Warning: failed to delete ${env_key}: $(cat "${err_file}")" >&2
  rm -f "${err_file}"
fi

Refs:

• cleanup-vercel-stale-env.sh:70-81
• teardown-vercel-preview-env.sh:54-62

💭 2) cleanup-vercel-stale-env.sh Document exit behavior on partial failures

Issue: The cleanup script continues after individual env var deletion failures and always exits 0, even when some deletions fail.

Why: For a one-time cleanup tool this is reasonable (partial progress is better than failing early), but operators should be aware that the workflow job will succeed even with partial failures.

Fix: Add a comment documenting this intentional behavior, or optionally return non-zero when failed > 0 if strict failure semantics are preferred.

Refs:

• cleanup-vercel-stale-env.sh:85-87

Summary: Clean fix for a real production issue. The scripts follow established patterns (sourcing common.sh, using set -euo pipefail, proper input validation, timeouts), and the workflow conditions are correctly scoped. Ship it! 🚀

PR Review Summary

(0) Total Issues | Risk: Low

This re-review covers the delta since the last automated review: commit 3ee8363 ("fix(ci): safeguard Vercel env teardown to only delete preview-target vars").

Changes Reviewed

The delta adds a good safety improvement:

1. New shared helper vercel_list_preview_only_env_vars() in common.sh:

   • Centralizes env var listing logic
   • Safety check: Refuses to proceed if any branch-scoped vars target production or development (not just preview)
   • Returns JSON for callers to filter

2. Script refactoring:

   • Both cleanup and teardown scripts now use the shared helper
   • Additional filter ensures only target == ["preview"] vars are deleted

The safety check is intentionally conservative — it protects against accidentally deleting vars that might affect production or development environments, even if they're branch-scoped.

🕐 Pending Recommendations (1)

• 🟡 teardown-vercel-preview-env.sh:56 Deletion count reports attempted vs actual deletions (pullfrog)

Summary: The delta adds a well-designed safety layer to prevent accidental deletion of non-preview env vars. The conservative "fail-safe" approach is appropriate for infrastructure automation. The only outstanding item is the minor count accuracy issue raised in the prior review. Ship it! 🚀

PR Review Summary

(1) Total Issues | Risk: Low

This re-review covers the delta since the last automated review (commit 3ee8363). The 3 new commits address prior feedback well:

• ✅ Pagination: common.sh now correctly paginates Vercel API with cursor-based iteration
• ✅ Delete count: Teardown script now tracks actual successful deletions vs attempted
• ✅ Branch regex: Now includes + and = characters for dependabot-style branches
• ✅ Open PR check: Cleanup script now queries GitHub before deleting (smart addition)
• ✅ Local-only cleanup: Removed workflow dispatch input — bulk cleanup is now a local script only (good design choice)

🟠⚠️ Major (1) 🟠⚠️

🟠 1) cleanup-vercel-stale-env.sh:25-30 gh CLI failure silently treats branches as stale

Issue: The branch_has_open_pr() function uses || echo "0" to handle gh pr list failures, which means any failure (network issues, rate limiting, expired auth) returns 0, causing the function to return false (no open PR). This classifies the branch as "stale" when it might actually have an open PR.

Why: If gh fails for all branches (e.g., auth expired), every branch would be incorrectly classified as stale, leading to deletion of env vars for branches that actually have open PRs. While this script is now local-only (not CI), it's a one-time cleanup tool where silent failure modes are particularly risky.

Fix: Distinguish between "zero PRs found" (legitimate) and "command failed" (should abort). Options:

branch_has_open_pr() {
  local branch="$1"
  local output=""
  local exit_code=0
  
  output="$(gh pr list --head "${branch}" --state open --json number --jq 'length' 2>&1)" || exit_code=$?
  
  if [ "${exit_code}" -ne 0 ]; then
    echo "Error: gh pr list failed for branch ${branch}: ${output}" >&2
    return 2  # Distinct exit code for "failed to check"
  fi
  
  [ "${output}" -gt 0 ]
}

Then in the caller, treat exit code 2 as "skip this branch" or "abort entirely" rather than "no open PR".

Alternatively, run gh auth status once at script start to fail fast if auth is invalid.

Refs:

• cleanup-vercel-stale-env.sh:25-30

💭 Consider (1) 💭

💭 1) cleanup-vercel-stale-env.sh:71-73 Empty array expansion edge case in older bash

Issue: With set -u, ${open_branches[@]} on an empty array can cause "unbound variable" errors in bash < 4.4.

Why: The printf is inside an if block checking array length, so it's guarded. This is a very edge-case issue since GitHub Actions runners and most dev machines use recent bash. Since this is a local-only script, the risk is minimal.

Fix: If you want to be defensive, use "${open_branches[@]+"${open_branches[@]}"}" pattern, but this is optional.

Refs:

• cleanup-vercel-stale-env.sh:71-73

Summary: The delta addresses all prior feedback cleanly. The pagination implementation is correct, delete counts are now accurate, and removing the workflow dispatch was the right call. The one remaining issue (silent gh failure) is worth fixing since this is a cleanup tool where silent failure modes can cause unintended deletions. That said, it's a local-only script with DRY_RUN support, so the overall risk is manageable. Ship it once the gh error handling is tightened! 🚀

PR Review Summary

(1) Total Issues | Risk: Low

This re-review covers the delta since the last automated review (commit 5bcdce3b9d9c). The new commit (f712904bb) is an empty CI re-trigger commit with no code changes.

Delta Analysis

✅ No code changes — The single commit since the last review (ci: re-trigger CI (flaky SpiceDB E2E test)) contains no file modifications. This is a CI re-trigger for a flaky E2E test.

🕐 Pending Recommendations (1)

• 🟠 cleanup-vercel-stale-env.sh:60-65 gh CLI failure silently treats branches as stale — raised in prior review, still unaddressed

The branch_has_open_pr() function uses || echo "0" which treats any gh CLI failure (network, rate limiting, expired auth) as "no open PR", potentially causing deletion of env vars for branches that actually have open PRs.

Risk context: This is a local-only cleanup script (not CI), so the blast radius is limited to manual operator runs. The DRY_RUN mode also provides a safety check before committing to deletions.

Summary: No code changes in this delta — just a CI re-trigger commit. The implementation from prior commits remains solid. The one pending issue (silent gh CLI failure handling) is worth addressing but not blocking given the script is local-only with DRY_RUN support. Ship it! 🚀

PR Review Summary

(1) Total Issues | Risk: Low

This re-review covers the delta since the last automated review (commit f712904bb). The new commit (e2bd0e36e) adds a SpiceDB schema write step before E2E tests.

Delta Analysis

✅ Clean, correct change — The step:

• Installs the zed CLI and writes the SpiceDB schema before E2E tests
• Placement is correct (after migrations, before E2E tests)
• Token matches the SpiceDB container's preshared key
• Resolves the flaky E2E test issue where schema wasn't initialized

💭 Consider (1) 💭

💭 1) .github/workflows/ci.yml:380 Unpinned zed version

Issue: go install github.com/authzed/zed/cmd/zed@latest pulls the latest version on every CI run, which can cause non-deterministic behavior if zed introduces breaking changes.

Why: Minor reliability concern for CI reproducibility. However, this is consistent with the existing pattern — the authzed/spicedb Docker image is also unpinned (line 356).

Fix: Optionally pin to a specific version: go install github.com/authzed/zed/cmd/[email protected] (or current stable). Not blocking.

Refs:

• authzed/zed releases
• ci.yml:356 — unpinned spicedb image

🕐 Pending Recommendations (1)

• 🟠 cleanup-vercel-stale-env.sh:60-65 gh CLI failure silently treats branches as stale — raised in prior review, still unaddressed

Summary: The delta is a straightforward CI fix that correctly initializes the SpiceDB schema before E2E tests. The one Consider item (version pinning) is consistent with existing patterns in the workflow. The prior pending issue (gh CLI silent failure) remains from earlier reviews but is outside this delta's scope and affects a local-only script with DRY_RUN support. Ship it! 🚀

PR Review Summary

(0) Total Issues | Risk: Low

This re-review covers the delta since the last automated review (commit e2bd0e36ea98). The single commit (e0147135e) adds GOPATH/bin to PATH for zed CLI.

Delta Analysis

✅ Trivial, correct fix — The change adds:

export PATH="$(go env GOPATH)/bin:${PATH}"

This ensures the zed binary installed by go install is available in the subsequent zed schema write command. Without this, the shell wouldn't find zed since go install places binaries in GOPATH/bin which isn't in PATH by default.

Correctness:

• $(go env GOPATH) correctly retrieves the Go workspace path
• Adding to beginning of PATH ensures the newly installed binary takes precedence
• The export happens in the same run: block so it's available for the next command in the script

🕐 Pending Recommendations (1)

• 🟠 cleanup-vercel-stale-env.sh:60-65 gh CLI failure silently treats branches as stale — raised in prior review, still unaddressed

The branch_has_open_pr() function uses || echo "0" which treats any gh CLI failure (network, rate limiting, expired auth) as "no open PR", potentially causing deletion of env vars for branches that actually have open PRs.

Risk context: This is a local-only cleanup script (not CI), so the blast radius is limited to manual operator runs. The DRY_RUN mode also provides a safety check before committing to deletions.

Summary: The delta is a minimal, correct fix that ensures the zed CLI is available after go install. No new issues. The prior pending issue (gh CLI silent failure) remains from earlier reviews but affects a local-only script with DRY_RUN support — not blocking. Ship it! 🚀