Repository navigation
engine.sh: missing/placeholder Copilot token misclassified as rate-limit (status=1) — deterministic auth failure treated as retryable #1591
Description
Split out of review feedback on PR #1556 (implements #1546). Valid finding. #1546 fixes license/policy denial misclassification; this is the adjacent case it does not cover.
Defect
scripts/engine.sh (~line 297): a missing or placeholder Copilot token returns status 1. That is indistinguishable from a rate-limit result to run_writer_with_fallback, which then sets any_rate_limited=1 and can return exit 2 when Copilot is the only enabled engine, or when every other engine is unavailable.
Net effect: a deterministic authentication/configuration failure is classified as a retryable rate limit.
Why this matters more than it looks
- It is never going to succeed on retry. A retryable classification sends the retry cron back at a condition that cannot change without a human setting a secret. That burns cycles against the churn breaker and delays the real diagnosis.
- It very likely explains a standing trap. The fleet plan records: "the dev-lead/pr-review Copilot fallback engine is broken (fails in <1s every time, likely missing/invalid Copilot-scope PAT) — when Claude is unavailable there is currently no working fallback." A sub-second failure with a missing token is exactly this code path. That "no working fallback" is why the 7-day
CLAUDE_CODE_OAUTH_TOKENoutage (#1587) was a total engine outage rather than a degraded one. - It compounds
#1568.#1568tracksstatus=rate-limitedbeing emitted for non-quota holds; this is a second, independent source of the same false signal, in a different script. Fixing one will not fix the other, and both mislead the same standing diagnostic rule ("distinguish throttled from not-provided").
Acceptance criteria
- A missing/placeholder Copilot token yields a distinct
unavailable/unconfiguredresult, not status1. -
run_writer_with_fallbackdoes not setany_rate_limitedfor that result, and does not exit 2 on account of it. - The skip reason is tracked separately from quota exhaustion and surfaced (per
#1552). - Test: unset / placeholder the Copilot token with Copilot as the only enabled engine → assert the outcome is
unavailable, notrate-limited, and that no retry is scheduled. - Cross-check against
#1568so the two fixes agree on one reason-token vocabulary rather than inventing two.
Found by codeant-ai on #1556; kept out of that PR to avoid widening #1546's scope mid-review.
Activity
github-actions commentedon Aug 30, 2026
Thanks for the thorough write-up and for keeping this scoped out of #1556 — the separation makes both fixes much easier to reason about. The misclassification is clear and the acceptance criteria are well-defined. One question to unblock implementation: has any vocabulary for the reason token (e.g.
unavailable,unconfigured,auth-failure) been tentatively agreed upon in #1568, or should the implementer propose one here and cross-reference it there?don-petry commentedon Aug 31, 2026
Dev-Lead Implementation Plan
Issue: #1591 — engine.sh: missing/placeholder Copilot token misclassified as rate-limit (status=1) — deterministic auth failure treated as retryable
Scope
Introduce a distinct
unconfiguredreason token for a missing/placeholder Copilot credential so a deterministic configuration gap is classified separately from a retryable quota exhaustion (rate-limited), and route it to a human-escalation (no retry) path rather than a retryable failure.Implementation Checklist
scripts/engine.sh—run_writer_with_fallback: track a newany_unconfiguredaggregate when the Copilot config-gap guards skip the engine (missing/placeholder token, unsupported classic PAT); never setany_rate_limitedfor it; on a pure-config-gap outcome writereason=unconfiguredand return 1 (not 2). Priority:missing-binary>rate-limited>unconfigured>engine-error.scripts/dev-lead-fix-issue.sh—handle_engine_failure: add anunconfiguredbranch that escalates toneeds-human(terminal marker, no retryablestatus=failedmarker), mirroring themissing-binarybranch.Reason-token vocabulary (cross-check #1568)
Single shared vocabulary:
rate-limited(throttled/quota),unconfigured(credential not-provided/placeholder),missing-binary(engine binary absent),timeout,engine-error. This gives #1568 the "throttled vs not-provided" distinction one token set rather than two.Tests to Write
tests/dev-lead/unit/test_engine_fallback.bats— copilot-only + unset token → exit 1,reason=unconfigured, notrate-limited; placeholder-token variant; classic-PAT (ghp_) variant.tests/dev-lead/unit/test_fix_issue.bats—unconfigured→ escalates toneeds-human, applies the label, posts NO retryablestatus=failedmarker (no retry scheduled).Test Command
bats tests/dev-lead/unit/test_engine_fallback.bats tests/dev-lead/unit/test_fix_issue.bats(full:bats tests/**/*.bats), plusbash .dev-lead/scripts/dev-lead-lint.shdon-petry commentedon Aug 31, 2026
Dev-Lead: Progress (in progress — not yet durable)
Plan Execution (intended)
unconfiguredreason token for a missing/placeholder/unsupported Copilot credential, tracked separately from quota exhaustion (rate-limited).run_writer_with_fallbacknever feeds a config-gap intoany_rate_limited; when it is the sole cause it returns exit 1 withreason=unconfigured(priority: missing-binary > rate-limited > unconfigured > engine-error, so a genuine quota hit still wins).handle_engine_failureroutesunconfiguredto aneeds-humanescalation (terminal marker, no retryablestatus=failed), mirroringmissing-binary— so the retry cron does not requeue a state that cannot change without a human.Local check output (provisional — not the pushed run)
Files touched (this workspace, not yet pushed)
scripts/engine.sh: newany_unconfiguredaggregate + terminalunconfiguredclassification.scripts/dev-lead-fix-issue.sh:unconfigured→ needs-human escalation branch (no retry marker).tests/dev-lead/unit/test_engine_fallback.bats: copilot-only missing/placeholder/classic-PAT →unconfigured; genuine rate-limit still wins.tests/dev-lead/unit/test_fix_issue.bats:unconfigured→ needs-human, no retry marker.Notes
Reason-token vocabulary reconciled with #1568:
rate-limited= throttled,unconfigured= not-provided.check_provider_headroom's copilot arm (~line 297) is left as-is — its return-1 path is unreachable for the missing-token case becauserun_writer_with_fallbackguards the credential before probing; the classification fix lives in the caller where the misclassification actually occurred.don-petry commentedon Aug 31, 2026
Dev-Lead: Implementation Complete — PR #1597
The implementation for issue #1591 is durable: commits are pushed and a pull request is open. This record is backed by verifiable artifacts you can confirm in one click.
250e6ed6d4ea8aad5c43990631444e17cfe527efReview happens on the PR. Acceptance criteria and test results are evidenced by the PR's diff and CI checks — not asserted here. If a later stage fails, this claim is retracted in place.
feat: implement issue #1591 — engine.sh: missing/placeholder Copilot …