feat: implement issue #881 — pr-review engine: COPILOT_API_MODEL unbound var silently disables the cross-engine rubber-duck tier (engine.sh:431)

[don-petry]

Closes #881

Implemented by dev-lead agent. Please review.

Summary by CodeRabbit

• Bug Fixes

  • Improved reliability of engine configuration handling to prevent script failures in cross-engine scenarios

• Tests

  • Added regression tests to ensure engine configuration variables remain properly bound across different engine types

[chatgpt-codex-connector]

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]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 66152d13-039b-4dd3-b0b1-7c05429895aa

📥 Commits

Reviewing files that changed from the base of the PR and between e634fdb and 4434754.

📒 Files selected for processing (2)

• scripts/engine.sh
• tests/dev-lead/unit/test_engine_duck_model.bats

---

📝 Walkthrough

Walkthrough

COPILOT_API_MODEL default assignment is moved from the copilot branch of set_engine_config() to an unconditional block after the case statement in scripts/engine.sh. A new Bats test file adds regression coverage asserting the variable is bound across claude, copilot, and gemini engine configurations, including a set -euo pipefail subprocess test.

Changes

COPILOT_API_MODEL unconditional default fix

Layer / File(s)	Summary
Unconditional COPILOT_API_MODEL default in engine.sh scripts/engine.sh	Removes COPILOT_API_MODEL default and export from the copilot branch body (lines 144–146) and adds a new unconditional default block after the case statement (lines 165–177), ensuring the variable is always exported for any engine that invokes the Copilot duck subprocess.
Bats regression tests tests/dev-lead/unit/test_engine_duck_model.bats	New test file with setup/teardown hooks that unset COPILOT_API_MODEL per test, a _source_engine helper, and five tests covering: default binding under claude, a set -euo pipefail subprocess regression asserting no unbound-variable abort, explicit override preservation under claude, and default binding under both copilot and gemini.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

• petry-projects/.github-private#111: Modifies the claude branch in set_engine_config to configure DUCK_ENGINE="copilot" and DUCK_MODEL, directly creating the code path this PR fixes by requiring COPILOT_API_MODEL to be set outside the copilot branch.
• petry-projects/.github-private#227: Refactors scripts/engine.sh's engine configuration and Copilot model binding around set_engine_config, overlapping with the exact section this PR modifies.

Suggested labels

needs-human-review

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main change: moving COPILOT_API_MODEL default out of the copilot-only branch to fix cross-engine rubber-duck failures on claude-primary reviews.
Linked Issues check	✅ Passed	The PR implements both suggested fixes from #881: hoisting COPILOT_API_MODEL default to unconditional exports and adding regression tests for claude-primary configurations.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to issue #881: scripts/engine.sh unconditionally exports COPILOT_API_MODEL, and test_engine_duck_model.bats adds regression coverage for the fix.

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

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch dev-lead/issue-881-20260621-1313

---

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

• X
• Mastodon
• Reddit
• LinkedIn

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

[gemini-code-assist]

Code Review

This pull request moves the default initialization of COPILOT_API_MODEL in scripts/engine.sh to be unconditional, ensuring it is always set even when Copilot is not the primary engine. This prevents unbound variable errors under set -u during cross-engine rubber-duck execution. A new suite of bats regression tests has been added to verify this behavior. Feedback is provided to correct a minor typo with unbalanced parentheses in the newly added comments.

[don-petry]

Dev-Lead — review-changes (applied)

Changes committed and pushed.

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-06-21T14:23:38Z.

[don-petry]

Dev-Lead — fix-reviews (no-changes)

Agent reasoning

Addressed 0 threads:
(no open review threads)
Test verification: shellcheck passes (no new issues); bats unavailable locally but CI review job passed
Files changed: none (no threads required fixes)
```

[don-petry]

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

PR: #894
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-21T15:32:07Z

[don-petry]

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-21T15:32:07Z

[donpetry-bot]

Advisory bots were rate-limited; auto-approval is withheld until they recover. pr-review-sweep will re-review this PR after 2026-06-21T16:05:28Z.

[donpetry-bot]

Automated review — APPROVED ✓

Risk: LOW
Reviewed commit: 44347541a4ced17e68a669b3fa2acbf0241e9f46
Review mode: triage-approved (single reviewer)

Summary

Hoists the COPILOT_API_MODEL default out of the copilot) branch of set_engine_config() to an unconditional block after the case statement, so the variable is always bound for the cross-engine rubber-duck path. Adds a Bats regression suite covering claude/copilot/gemini primaries plus a set -euo pipefail subprocess test. +71/-7 across 2 files.

Linked issue analysis

Closes #881. Verified the root cause holds: the claude) branch sets DUCK_ENGINE=copilot (engine.sh:93) and copilot_chat references a bare $COPILOT_API_MODEL (engine.sh:455,460) under set -u, so leaving the default inside the copilot-only branch aborted the duck subprocess on a claude-primary review and silently skipped tier-2. The fix defaults it unconditionally (engine.sh:176) and it remains exported (engine.sh:181: export DUCK_ENGINE DUCK_MODEL COPILOT_API_MODEL). Both fixes requested in the issue — unconditional default + regression tests — are implemented and scoped.

Findings

No blocking findings. The change is minimal, behavior-preserving for the copilot-primary case (regression test confirms), and adds explicit coverage for the previously-broken claude/gemini paths. Non-blocking nit (pre-existing, not introduced here): the comment at engine.sh:434 says 'default: openai/gpt-4o' while the actual default is openai/o4-mini — worth aligning in a future touch. No auth/secrets/migration/CI-security concerns. Secret-scanning MCP tool not exposed in this environment; relied on the passing gitleaks CI check instead.

CI status

All required checks green (shellcheck, ShellCheck, bats, unit, unit-tests, CodeQL, Analyze (actions/python), agent-shield, gitleaks secret scan, SonarCloud quality gate passed with 0 new issues). Skipped checks are dependabot/dependency-audit stubs (expected). CodeRabbit reported no actionable comments. mergeStateStatus is BLOCKED only on the required human/automated review (REVIEW_REQUIRED), which this verdict addresses.

---

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

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[donpetry-bot]

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

[donpetry-bot]

Automated review — APPROVED ✓

Risk: MEDIUM
Reviewed commit: a419c991371d5d71cf1b4ab7b17fc2912742aa23
Review mode: triage-approved (single reviewer)

Summary

Small, well-scoped fix for #881: moves the COPILOT_API_MODEL default assignment out of the copilot-only case branch to an unconditional default after the case statement (engine.sh:176), and keeps it exported (engine.sh:181). This resolves the unbound-variable abort under set -u when a non-copilot primary (e.g. claude, which wires DUCK_ENGINE=copilot at engine.sh:93) invokes copilot_chat's bare $COPILOT_API_MODEL (engine.sh:455/460) for the cross-engine rubber-duck tier. Adds 5 bats regression tests covering claude/copilot/gemini primaries, set -u reproduction, and override honoring.

Linked issue analysis

#881 reports that COPILOT_API_MODEL was defaulted only inside the copilot) branch, so under claude-primary reviews the duck subprocess silently aborted and tier-2 was skipped. The fix directly addresses the root cause: the default is now applied for every engine and exported unconditionally (an improvement over the prior copilot-only export). Verified the claim against source — claude branch sets DUCK_ENGINE=copilot (engine.sh:93) and copilot_chat references a bare $COPILOT_API_MODEL (engine.sh:455/460). Substantively addressed.

Findings

No blocking findings. Non-blocking nit (from gemini-code-assist): the explanatory comment uses "claude) branch" shorthand which reads as an unbalanced paren; it intentionally refers to the claude) case label and is harmless. No secrets — COPILOT_API_MODEL is a public model identifier, not a credential. Export is preserved (engine.sh:181), so no regression in subprocess visibility. shellcheck SC2015/SC1091 infos at engine.sh:281 are pre-existing and outside this diff.

CI status

All required checks green (Lint, shellcheck/ShellCheck, unit-tests/unit, bats, CodeQL, Analyze actions+python, SonarCloud, agent-shield, holdout-guard, validate-fixtures, guard, secret scan gitleaks, plus structure/permission guards). CANCELLED entries are the dev-lead agent's own dispatch/ci-relay workflows; SKIPPED entries are dependency-audit jobs for ecosystems not present. reviewDecision=APPROVED (coderabbitai, donpetry-bot at prior SHA). Local bats unavailable on runner; relied on CI's green bats/unit checks.

---

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