feat: implement issue #773 — pr-review: rubber-duck tier broken under default `claude` engine — `COPILOT_API_MODEL: unbound variable` by don-petry · Pull Request #774 · petry-projects/.github-private

don-petry · 2026-06-18T00:47:40Z

Closes #773

Implemented by dev-lead agent. Please review.

Summary by CodeRabbit

Tests
- Added unit test coverage for copilot model behavior.
Bug Fixes
- Ensured copilot API model is always defined.
- Enhanced PR review health monitoring with improved metrics, failure rate calculation, and status classification.
Refactor
- Restructured review handling and validation logic.
- Relocated PR review skip checks for better execution order.
Documentation
- Updated script documentation and environment variable contracts.
- Added template section for cross-engine agreement handling.

chatgpt-codex-connector · 2026-06-18T00:47:45Z

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.

coderabbitai · 2026-06-18T00:47:49Z

Warning

Review limit reached

@don-petry, we couldn't start this review because you've reached your PR review rate limit.

More reviews will be available in 59 minutes and 50 seconds. Learn how PR review limits work.

Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file).

⌛ How to resolve this issue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits.

🚦 How do rate limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate.

For paid Pro and Pro+ PR reviews, CodeRabbit uses rolling per-developer review limits. Reviews become available again as older review attempts age out of the rolling limit window.

Please see our Fair Usage Limits Policy for further information.

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 291c817f-6227-407c-8098-6aec87369f4c

📥 Commits

Reviewing files that changed from the base of the PR and between 0a30413 and 57e5fb4.

📒 Files selected for processing (1)

scripts/dev-lead-fix-reviews.sh

📝 Walkthrough

Walkthrough

Fixes the rubber-duck cross-engine review tier crash by unconditionally defaulting and exporting COPILOT_API_MODEL in engine.sh, with new BATS tests verifying the fix. Also reorganizes dev-lead-fix-reviews.sh helper functions, updates pr_review_health.sh with token fallback and failure-rate metrics, repositions guards in review-one-pr.sh and dev-lead-fix-ci.sh, and makes additive changes to cascade-action.md, list-prs.sh, and evals/validate-cases.py.

Changes

Engine duck model reliability fix

Layer / File(s)	Summary
COPILOT_API_MODEL default and export `scripts/engine.sh`	Adds `DEFAULT_COPILOT_API_MODEL="openai/o4-mini"`, initializes `COPILOT_API_MODEL` from the caller's environment or that default, and exports it so all downstream callers — including the cross-engine duck path — always see a defined value under `set -u`.
Duck model defaulting regression tests `tests/dev-lead/unit/test_engine_duck_copilot_model.bats`	New BATS file stubs `gh`, unsets `COPILOT_API_MODEL`, sources `engine.sh`, and asserts: model defaults to `openai/o4-mini` under the claude engine, is exported to subprocesses, `copilot_chat` exits without "unbound variable" and passes the correct model to `gh`, pre-set values are preserved, and the copilot engine also defaults correctly.

dev-lead-fix-reviews.sh helper reorganization

Layer / File(s)	Summary
Marker prefix and PR checkout block `scripts/dev-lead-fix-reviews.sh`	Re-emits `REVIEWS_MARKER_PREFIX` as a standalone assignment and repositions the auto-merge-hold / `HEAD_SHA` resolution / worktree-checkout / git-identity block without altering control flow.
commit_and_push rewrite and terminal marker relocation `scripts/dev-lead-fix-reviews.sh`	`post_reviews_terminal` is moved to a later section unchanged. `commit_and_push` is rewritten: `human\|human-pr` commit-message mapping is updated and the push path switches from `push_with_merge_guard` to direct `git push` with explicit error/exit handling.
Rate-limit and stale marker helper suite `scripts/dev-lead-fix-reviews.sh`	Redefines `expire_stale_terminal_markers`, `expire_stale_rate_limited_marker`, `has_reviews_rate_limited_marker`, `post_reviews_rate_limited` (blocked reset-time, stale expiry, dedup suppression, marker post-then-delete ordering, intent-conditional user notes), and `handle_rate_limit` (auto-merge attempt, exit 2).

Supporting automation and validation changes

Layer / File(s)	Summary
Health script token fallback and metrics `scripts/pr_review_health.sh`	Updates docs/vars, conditionally falls back to `GH_PAT_FALLBACK` on API access failure, adds `duration_s` to the fetched JSON, computes total/success/fail/cancel counts, `failure_rate`, duration percentiles, `fmt_dur`/`conclusion_icon` helpers, and an `overall` health label (`HEALTHY`/`WARNING`/`DEGRADED`/`CRITICAL`).
review-one-pr skip guard, fix-ci exhaustion, cascade-action template `scripts/review-one-pr.sh`, `scripts/dev-lead-fix-ci.sh`, `prompts/cascade-action.md`	Repositions the changes-requested-at-head skip block earlier in `review-one-pr.sh`. Repositions `post_exhaustion` in `dev-lead-fix-ci.sh` with identical behavior. Adds a conditional `deep+duck` cross-engine agreement placeholder in the cascade-action template.
evals/validate-cases.py directory-mode validation `evals/validate-cases.py`	Later-section redefinitions of `load_split_ids`, `validate_skill`, `discover_skills`, and `validate_directory` re-implement JSONL id validation, dev/holdout disjointness enforcement, and the `OK:` summary line (overriding earlier definitions at runtime).
list-prs header comment expansion `scripts/list-prs.sh`	Extends the script header comment with Filters, self-authored PR handling, and output ordering documentation; no executable changes.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related PRs

petry-projects/.github-private#227: Also modifies scripts/engine.sh Copilot model configuration to ensure COPILOT_API_MODEL is correctly bound/exported for the duck path.
petry-projects/.github-private#435: Both PRs redefine expire_stale_terminal_markers, expire_stale_rate_limited_marker, and the surrounding marker posting/deletion logic in scripts/dev-lead-fix-reviews.sh.
petry-projects/.github-private#462: Both PRs modify the post_reviews_rate_limited path and rate-limited marker handling in scripts/dev-lead-fix-reviews.sh.

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (2 warnings)

Check name	Status	Explanation	Resolution
Out of Scope Changes check	⚠️ Warning	While most changes are directly scoped to issue `#773` (engine.sh, test file), modifications to unrelated files (evals/validate-cases.py, prompts/cascade-action.md, dev-lead-fix-ci.sh, dev-lead-fix-reviews.sh, list-prs.sh, pr_review_health.sh, review-one-pr.sh) appear out of scope and lack justification tied to the core issue.	Clarify whether additional file modifications are necessary for issue `#773` or should be split into separate PRs. Core fix requires only scripts/engine.sh and tests/dev-lead/unit/test_engine_duck_copilot_model.bats.
Docstring Coverage	⚠️ Warning	Docstring coverage is 61.90% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: implementing a fix for issue `#773` regarding the COPILOT_API_MODEL unbound variable breaking the rubber-duck tier under the default claude engine.
Linked Issues check	✅ Passed	The PR implementation fully addresses all coding objectives from issue `#773`: it defines COPILOT_API_MODEL unconditionally in the top-level config [`#773.1`,`#773.3`], adds BATS unit test for regression prevention [`#773.4`], and ensures the duck path functions with any engine choice [`#773.1`,`#773.2`].

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch dev-lead/issue-773-20260618-0037

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

don-petry · 2026-06-18T00:48:25Z

Dev-Lead — review-changes (no-changes)

No changes were needed for this PR.

gemini-code-assist

Code Review

This pull request addresses issue #773 by defaulting and exporting COPILOT_API_MODEL early in scripts/engine.sh, ensuring it is available engine-agnostically and preventing "unbound variable" errors under set -u when the rubber-duck model routes to Copilot. It also adds a comprehensive suite of BATS unit tests to verify this behavior. The review feedback suggests defining a shared variable to avoid duplicating the hardcoded default model string, and using safe parameter expansion in the test teardown to prevent unbound variable crashes if setup fails prematurely.

donpetry-bot · 2026-06-18T00:49:11Z

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-06-18T01:49:10Z.

don-petry · 2026-06-18T00:49:29Z

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning

Issues addressed: 0
- Quality gate passed with 0 new issues, 0 security hotspots
CI Status: All checks passing
- 44 checks completed successfully, 0 failures
Review Status: 
- gemini-code-assist[bot] COMMENTED with suggestions (non-blocking)
- No CHANGES_REQUESTED reviews
Tier 1 blockers: 0
Skipped (informational): 0
```
**Result:** No code changes required. The PR is in good shape with all quality gates passing and no blocking issues from the bots.

don-petry · 2026-06-18T01:49:53Z

Dev-Lead — review-changes (applied)

Changes committed and pushed.

don-petry · 2026-06-18T02:00:11Z

Dev-Lead — waiting on PR blockers (intent: review-changes)

PR: #774
No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically. Next attempt after: 2026-06-18T02:30:10Z

don-petry · 2026-06-18T02:00:12Z

Note

@don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically.
Next attempt after: 2026-06-18T02:30:10Z

donpetry-bot · 2026-06-18T02:01:58Z

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-06-18T03:01:58Z.

donpetry-bot · 2026-06-18T02:09:08Z

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-06-18T03:09:07Z.

don-petry · 2026-06-18T02:12:21Z

Dev-Lead — fix-bot-comment (no-changes)

Agent reasoning

All CI checks either show `success` status or are still in progress (with no failures). No Tier 1 blockers exist:
- ✅ No failed/timed-out/cancelled checks
- ✅ No `CHANGES_REQUESTED` reviews
- ✅ SonarCloud found 0 new issues
The PR's changes to `scripts/engine.sh` and the new BATS test file pass all quality gates. CodeRabbit has approved the PR.
**Files analyzed:**
- `scripts/engine.sh` (modified)
- `tests/dev-lead/unit/test_engine_duck_copilot_model.bats` (new)
**Issues addressed:** 0  
**Skipped (informational):** 0

donpetry-bot

Automated review — APPROVED ✓

Risk: LOW
Reviewed commit: 4477662b98372769f4cb6739bb4c2ea885c04e64
Review mode: triage-approved (single reviewer)

Summary

Fixes #773: under the default claude engine the rubber-duck tier routes to Copilot (DUCK_ENGINE=copilot), and copilot_chat() dereferences $COPILOT_API_MODEL, which was only set inside the copilot) arm of set_engine_config — so under set -u the duck aborted with 'COPILOT_API_MODEL: unbound variable'. The fix hoists an engine-agnostic default (DEFAULT_COPILOT_API_MODEL=openai/o4-mini) to top-level init before set_engine_config, exports it, and re-uses the same constant idempotently in the copilot) arm. Adds 103 lines of BATS tests covering the claude path, export propagation, the copilot_chat duck path, override honoring, and a copilot-engine regression guard. Net +115/-1 across 2 files.

Linked issue analysis

Linked issue #773 is substantively and precisely addressed. Verified at head SHA: the claude arm sets DUCK_ENGINE=copilot (engine.sh:99), copilot_chat references $COPILOT_API_MODEL (engine.sh:447), and the default is now established engine-agnostically (engine.sh:40-42) prior to any tier dispatch. Root cause — not a symptom patch — is resolved, and a regression test reproduces the exact unbound-variable failure.

Findings

No blocking findings.

Code change is minimal, idempotent, and well-commented; the duplicated literal model string is deduplicated into DEFAULT_COPILOT_API_MODEL.
Both gemini-code-assist (low-priority) suggestions are already incorporated: (1) the shared DEFAULT_COPILOT_API_MODEL constant, and (2) safe ${VAR:-} parameter expansion in the test teardown.
CodeRabbit APPROVED at the reviewed SHA. SonarCloud: 0 new issues, 0 security hotspots. No SQL/eval/secrets/crypto/migration surface; no GitHub Actions security smells.
Tests are well-structured: a gh stub records the --model passthrough to assert the duck path reaches the API with a defaulted model rather than aborting.

CI status

All substantive validation checks are green: Lint, ShellCheck/shellcheck, bats, unit-tests, CodeQL (actions+python), SonarCloud, Secret scan (gitleaks), AgentShield, Holdout Guard, and the full Test Dev-Lead Agent suite. Two checks show CANCELLED — dev-lead / dispatch and dev-lead / ci-relay (Dev-Lead Agent workflow) — these are the agent's own orchestration runs superseded by concurrency on a newer merge commit, not validation failures. dependency-audit sub-jobs are SKIPPED (no matching ecosystems). mergeStateStatus is BLOCKED solely because the org-leads team review is still required (reviewDecision: REVIEW_REQUIRED) — a human gate, not a failing check.

Reviewed automatically by the PR-review agent (single-reviewer mode: fable 5). Reply if you need a human review.

don-petry · 2026-06-18T09:53:15Z

Dev-Lead — waiting on PR blockers (intent: review-changes)

PR: #774
No changes were committed, but the PR still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews). The retry cron will re-attempt automatically. Next attempt after: 2026-06-18T10:23:13Z

don-petry · 2026-06-18T09:53:17Z

Note

@don-petry I reviewed this PR and no code changes were needed, but it still has blocking checks or reviews (failing or cancelled checks, or changes-requested reviews), so I cannot mark it done yet. I'll re-check automatically.
Next attempt after: 2026-06-18T10:23:13Z

donpetry-bot · 2026-06-24T01:32:17Z