feat: implement issue #947 — P1 (#860 follow-up): don't re-invoke rate-limited advisory engines — one exhaustion notice per engine per run

[don-petry]

Closes #947

Implemented by dev-lead agent. Please review.

Summary by CodeRabbit

• Bug Fixes

  • Improved fallback handling to avoid re-invoking engines that become rate-limited or unavailable within the same run.
  • When a provider is at/above the headroom threshold, affected engines are treated as exhausted for the remainder of the run.
  • Reduced duplicate warnings by logging an exhaustion notice only once per engine per run, while preserving existing overall return behavior when all options are exhausted.

• Tests

  • Added coverage for run-scoped engine exhaustion, warning behavior, and reset of exhaustion state.

[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: d527e73b-1b3c-4b98-8a30-6b96221e8ee6

📥 Commits

Reviewing files that changed from the base of the PR and between a4a7501 and b311ee4.

📒 Files selected for processing (2)

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

---

📝 Walkthrough

Walkthrough

Adds a run-scoped engine exhaustion registry to scripts/engine.sh, wires fallback handling to skip exhausted engines and mark new exhaustion states, and adds bats coverage for repeated calls, notice deduplication, reset behavior, and headroom-triggered exhaustion.

Changes

Engine Exhaustion Tracking

Layer / File(s)	Summary
Exhaustion registry and helpers scripts/engine.sh	Adds _ENGINE_EXHAUSTED_REASON plus helper functions to check, mark, and reset exhausted engines within a run.
Fallback loop wiring scripts/engine.sh	run_writer_with_fallback skips already-exhausted engines, preserves aggregate exit-state tracking, and marks engines exhausted on headroom or writer failure paths.
Bats regression coverage tests/dev-lead/unit/test_engine_fallback.bats	Adds multi-call tests for skipped re-invocation, single exhaustion notices, reset behavior, all-engines-exhausted return codes, and headroom-triggered exhaustion.

Estimated code review effort: 2 (Simple) | ~15 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Caller
  participant run_writer_with_fallback
  participant _engine_is_exhausted
  participant run_writer
  participant _mark_engine_exhausted

  Caller->>run_writer_with_fallback: invoke with engine list
  loop for each engine
    run_writer_with_fallback->>_engine_is_exhausted: check(engine)
    alt engine already exhausted
      _engine_is_exhausted-->>run_writer_with_fallback: true
      run_writer_with_fallback->>run_writer_with_fallback: skip, fold reason into aggregate flags
    else engine not exhausted
      run_writer_with_fallback->>run_writer: run_writer(engine)
      run_writer-->>run_writer_with_fallback: exit code (2, 127, or success)
      alt exit code 2 or 127
        run_writer_with_fallback->>_mark_engine_exhausted: mark(engine, reason)
      end
    end
  end

Loading

Possibly related PRs

• petry-projects/.github-private#226: Both PRs modify scripts/engine.sh fallback behavior around run_writer_with_fallback and exit code 2/127 handling.
• petry-projects/.github-private#227: Both PRs modify run_writer_with_fallback's per-engine attempt logic in scripts/engine.sh.
• petry-projects/.github-private#899: Both PRs change run_writer_with_fallback/rate-limit handling logic in scripts/engine.sh.

Suggested labels: needs-human-review

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title is specific and matches the main change: preventing re-invocation of exhausted advisory engines with deduped notices.
Linked Issues check	✅ Passed	The change marks exhausted engines, skips re-invocation within the run, and adds bats coverage for single-notice behavior, matching #947.
Out of Scope Changes check	✅ Passed	The changes stay within the issue scope: engine fallback logic and regression tests, with no unrelated feature work.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✨ 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-947-20260702-1615

---

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.}

[don-petry]

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

No changes were needed for this PR.

[donpetry-bot]

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

[don-petry]

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

Agent reasoning

Issues addressed: 0
Files changed: N/A (no issues to fix)
Skipped (informational): Coverage and duplication metrics (0% — expected for minimal/non-code changes)
```

[gemini-code-assist]

Code Review

This pull request introduces a run-scoped engine-exhaustion registry to prevent re-invoking rate-limited or unavailable engines during the same process run, thereby avoiding unnecessary quota consumption. It also adds comprehensive unit tests to verify this behavior. The review feedback highlights potential unbound variable errors under set -u when accessing positional parameters in the new helper functions, and suggests utilizing the automatically cleaned-up STUB_BIN_DIR for temporary files in the unit tests to prevent orphaned files upon test failures.

[Copilot]

Pull request overview

Implements run-scoped “engine exhaustion” in the review engine fallback cascade so that once an advisory engine is detected as rate-limited/unavailable during a run, it won’t be re-invoked on subsequent run_writer_with_fallback calls in the same process, preventing repeated quota burns and repeated limit notices (issue #947 / incident #860 class).

Changes:

• Add a process-scoped exhaustion registry in scripts/engine.sh and skip already-exhausted engines while preserving the aggregate final exit code behavior.
• Emit a one-time exhaustion warning per engine per run when first marked exhausted.
• Add Bats unit tests asserting (1) no re-invocation, (2) single exhaustion notice emission across multiple calls, and (3) reset behavior for test isolation.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
scripts/engine.sh	Adds run-scoped exhaustion tracking and integrates it into run_writer_with_fallback fallback logic.
tests/dev-lead/unit/test_engine_fallback.bats	Adds regression tests for run-scoped engine exhaustion (no re-invoke, single notice, reset).

[don-petry]

Dev-Lead — rate-limited (intent: fix-reviews)

PR: #1022
The retry cron will re-attempt automatically.

[donpetry-bot]

Automated review — APPROVED ✓

Risk: MEDIUM
Reviewed commit: 64c7acbd8228fe961739ce4a93a139ca76ebcfd0
Cascade: triage → deep (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5)

Summary

PR #1022 adds a run-scoped engine-exhaustion registry to scripts/engine.sh (issue #947) so a rate-limited/unavailable engine is not re-invoked later in the same run, fixing the #860 fix-ci-cycle quota re-burn; it preserves the aggregate exit code and emits at most one notice per engine, with four bats tests covering skip, single-notice, all-exhausted return code, and reset. This is a MEDIUM logic change to shell orchestration with no security surface (no auth/secrets/crypto, no DB, no injection/eval, no Actions changes); all CI is green (CodeQL, gitleaks, Agent Security Scan, shellcheck, bats, unit-tests). The two gemini 'HIGH' set -u findings are defensive-robustness nits rather than live bugs — both helpers are always called with an explicit engine argument and the registry is an associative array (empty-subscript safe) — so no security escalation is warranted; downstream impact is (none). MCP run_secret_scanning was unavailable, so the gitleaks CI check stands in its place.

Findings

• minor: gemini flagged _mark_engine_exhausted (local engine="$1") and _engine_is_exhausted ([ -n "${_ENGINE_EXHAUSTED_REASON[$1]:-}" ]) as unsafe under set -u. Assessed as non-blocking: every call site passes an explicit "$engine" (never zero-arg), and _ENGINE_EXHAUSTED_REASON is an associative array so an empty subscript would not error. No live crash path; shellcheck and bats pass under set -euo pipefail. Adopting the ${1:-} guards would still be a cheap defensive improvement.
• info: gemini LOW: three new tests create temp files via mktemp in /tmp (claude_record, gemini_record, errlog). Creating them inside STUB_BIN_DIR would guarantee teardown cleanup even if an assertion fails, avoiding orphaned files. Non-blocking test hygiene.
• info: MCP run_secret_scanning tool was not available in this environment; did not fabricate a result. The gitleaks CI secret-scan check passed (SUCCESS) for this PR.

---

Reviewed by the PR-review cascade (triage: haiku 4.5 → deep: opus 4.8 + duck: o4-mini → audit: fable 5). Reply if you need a human review.

[don-petry]

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

Agent reasoning

Issues addressed: 0
Files changed: none
Skipped (informational): N/A — Quality Gate passed with 0 new issues
```
The PR is in good shape with all automated checks passing. No fixes are required for the SonarCloud comment. The PR is ready to merge pending any other review steps.

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@scripts/engine.sh`:
- Around line 1335-1352: Add a bats test for the headroom-threshold exhaustion
path in the engine fallback flow: mock check_provider_headroom in
scripts/engine.sh so it returns nonzero for a specific engine, then verify the
engine is marked exhausted for the rest of the run, skipped on a second call,
and only one exhaustion notice is emitted. Use the existing exhaustion/skip
assertions around _engine_is_exhausted, _mark_engine_exhausted, and
check_provider_headroom to cover the new permanent-exhaustion behavior without
changing the current rc=2/127 tests.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 37db4669-63cc-4f21-afbd-51c0c6a2ed2c

📥 Commits

Reviewing files that changed from the base of the PR and between e713192 and a4a7501.

📒 Files selected for processing (2)

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

[don-petry]

Dev-Lead — fix-reviews (applied)

Changes committed and pushed.

[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

[don-petry]

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

PR: #1022
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-07-02T19:52:59Z

[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-07-02T19:52:59Z

[don-petry]

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

No changes were needed for this PR.

[donpetry-bot]

Automated review — APPROVED ✓

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

Summary

Adds a run-scoped engine-exhaustion registry to run_writer_with_fallback in scripts/engine.sh: engines found rate-limited (exit 2) or missing (exit 127), or failing the headroom check, are marked exhausted and never re-invoked in the same run, with exactly one exhaustion notice per engine per run. Return-code semantics are preserved by folding recorded reasons into the existing any_rate_limited/any_missing aggregates. 5 new bats tests cover skip-on-re-invoke, single-notice, all-exhausted rc=2, reset_engine_exhaustion, and headroom-breach paths.

Linked issue analysis

Closes #947 (P1 follow-up to the #860 runaway, tracker #926). All three acceptance criteria are substantively met: (1) at most one exhaustion notice per engine per run — enforced by the no-op-on-repeat _mark_engine_exhausted; (2) an exhausted engine is not re-invoked within the same run — skipped via the _ENGINE_EXHAUSTED_REASON registry, including the headroom-threshold path; (3) bats regression coverage added for the rate-limited/unavailable paths asserting single-notice behavior.

Findings

No blocking findings.

• Correctness verified: the rc==2/else split in the fallback loop sits inside the existing 'rc==2 || rc==127' guard, so the else branch is exactly the missing-binary (127) case — prior semantics preserved.
• The registry declaration is guarded against re-sourcing engine.sh in the same process, so live exhaustion state survives (e.g. review-batch copilot pre-flight).
• Associative-array subscripts are quoted throughout (earlier Copilot nits addressed); exhaustion reasons are hardcoded literals, no injection surface.
• No secrets in the diff (test stubs use obvious placeholders); gitleaks CI check passed. The run_secret_scanning MCP tool was unavailable in this environment — noted, non-blocking.
• All 9 review threads (gemini-code-assist, copilot-pull-request-reviewer, coderabbitai) are resolved; CodeRabbit dismissed its changes-requested review and approved the head SHA.
• Triage assessment confirmed: change is well-scoped to the fallback path with no behavior change outside exhaustion tracking.

CI status

All checks green on head SHA b311ee4: shellcheck, ShellCheck, Lint, bats, unit, unit-tests, CodeQL (actions+python), SonarCloud, Secret scan (gitleaks), agent-shield, holdout-guard, validate-fixtures, and all structural/permission guards SUCCESS. CANCELLED entries (dev-lead/dispatch, review/review, dev-lead/ci-relay) are superseded duplicate runs — each has a later SUCCESS or SKIPPED run of the same check.

---

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