Skip to content

fix(combo): do not treat credits-exhausted 401 as auth skip - #12449

Merged
diegosouzapw merged 3 commits into
diegosouzapw:release/v3.8.51from
RaviTharuma:fix/12441-quota-not-auth-skip
Sep 4, 2026
Merged

diegosouzapw merged 3 commits into
diegosouzapw:release/v3.8.51from
RaviTharuma:fix/12441-quota-not-auth-skip

Conversation

@RaviTharuma

Copy link
Copy Markdown
Contributor

Summary

Related Issues

Validation

  • Change type: routing
  • Focused tests and category gates from the golden path
  • npm run lint (CI)
  • Reconciled with the current active release base; focused checks rerun afterward
  • Production-code changes include a new or updated automated test in this PR

Focused command:

node --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test tests/unit/12441-quota-not-auth-skip.test.ts tests/unit/chat-helpers.test.ts

Result: 32 pass, 0 fail.

Tests Added Or Updated

  • tests/unit/12441-quota-not-auth-skip.test.ts
  • tests/unit/chat-helpers.test.ts

Coverage Notes

Reviewer Notes

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The new quota/credits detection can miss quota signals when structured errors include a non-quota code, and the embeddings 402 mapping lacks a direct regression test.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes combo exhaustion classification so quota/credits exhaustion that surfaces as HTTP 401/403 does not take the auth-skip path (#8133), and surfaces credits_exhausted as HTTP 402 (Payment Required) from chat + embeddings to align with quota semantics.

Changes:

  • Map credits_exhausted terminal credential state to HTTP 402 (chat + embeddings) instead of 401.
  • Add isQuotaOrCreditsError() and use it to prevent misclassified quota/credits bodies from triggering auth-level combo exhaustion.
  • Add unit tests for the #12441 combo skip regression and the chat helper HTTP status mapping.

Commands / Coverage (custom):

  • Commands run (per PR description): node --import tsx/esm --import ./open-sse/utils/setupPolyfill.ts --import ./tests/_setup/isolateDataDir.ts --test tests/unit/12441-quota-not-auth-skip.test.ts tests/unit/chat-helpers.test.ts
  • Changed test files: tests/unit/12441-quota-not-auth-skip.test.ts, tests/unit/chat-helpers.test.ts
  • Coverage result (custom): Not provided/verified in this review (no npm run test:coverage output included).
File summaries
File Description
open-sse/services/combo/targetExhaustion.ts Adds quota/credits detection helper and blocks auth-skip for misclassified quota/credits error bodies.
src/sse/handlers/chatHelpers.ts Maps credits_exhausted terminal state to HTTP 402 instead of 401 in handleNoCredentials.
src/lib/embeddings/service.ts Maps embeddings credential exhaustion with credits_exhausted to HTTP 402.
tests/unit/chat-helpers.test.ts Adds regression test asserting handleNoCredentials returns 402 for credits_exhausted.
tests/unit/12441-quota-not-auth-skip.test.ts New unit tests covering quota/credits bodies not triggering auth-level combo skips.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 3
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread open-sse/services/combo/targetExhaustion.ts Outdated
Comment thread src/lib/embeddings/service.ts
Comment thread tests/unit/12441-quota-not-auth-skip.test.ts
@cursor
cursor Bot force-pushed the fix/12441-quota-not-auth-skip branch from 3388a6b to 363ded3 Compare September 2, 2026 10:20
@diegosouzapw

Copy link
Copy Markdown
Owner

sweep-reds / babysit: reds on this PR are inherited from base-red #12581, not this quota-skip diff. Skipping a code change; did not merge origin/release/v3.8.51.

  • ESLint: CodeQL ratchet 12 open > baseline 11 (repo-wide Security alerts). No unused/@eloqnt in this diff.
  • FQG mutation-test-coverage: missing tests/unit/video-bridge-memory-suppression.test.ts on open-sse/handlers/chatCore/memoryExtraction.ts — already listed on the current origin/release/v3.8.51 tip; not a file this PR touches.
  • Unit Tests 3/4: tests/unit/build/npm-ci-retry-composite.test.ts (fixTlsClientNodeBinary.mjs vanished / wreqJsNative.mjs omitted). Not in this PR's files; already corrected on the release tip.

@diegosouzapw

Copy link
Copy Markdown
Owner

sweep-reds round 6 / babysit: re-verified — every remaining red is inherited from base-red #12581, not this quota-skip diff. Skipping a code change; did not merge origin/release/v3.8.51.

  • ESLint: eslintWarnings=0; job fails CodeQL ratchet 12 open > baseline 11.
  • FQG: mutation-test-coverage missing tests/unit/video-bridge-memory-suppression.test.ts covering memoryExtraction.ts — not this PR (targetExhaustion.ts / embeddings / chatHelpers.ts + 12441 tests). Complexity new-code was OK (3 files).
  • Units 3/4: fixTlsClientNodeBinary.mjs vanished / wreqJsNative.mjs cache-key — postinstall helpers this PR does not touch.

STOP. Resume after #12581 drains.

RaviTharuma and others added 3 commits September 3, 2026 23:25
…-quota

Keep credits-exhausted 401/403 off the auth-skip path when the quota
signal lives in the message or errorText. Add embeddings 402 coverage
for allExpired + credits_exhausted.

Co-authored-by: Ravi Tharuma <RaviTharuma@users.noreply.github.com>
@RaviTharuma
RaviTharuma force-pushed the fix/12441-quota-not-auth-skip branch from 76a9341 to 4763ffd Compare September 3, 2026 23:26
@diegosouzapw
diegosouzapw merged commit 8c1dfc4 into diegosouzapw:release/v3.8.51 Sep 4, 2026
15 of 16 checks passed
@RaviTharuma
RaviTharuma deleted the fix/12441-quota-not-auth-skip branch September 23, 2026 19:36
muhamadgalihsaputra pushed a commit to niyatna/NiyatnaRoute that referenced this pull request Sep 27, 2026
…zapw#12449)

Validado em lote numa worktree combinada com os 10 PRs desta leva sobre o tip de `release/v3.8.51`: `typecheck:core` limpo, `check-file-size` OK e **241/242** nos 29 arquivos de teste que os PRs tocam.

A única "falha" não é falha: `tests/unit/autoCombo/strict-zero-cost-filter.test.ts` é um teste em estilo Vitest que eu incluí por engano na invocação do runner nativo do Node — ele quebra no import (`@vitest/runner`), não numa asserção. Ao investigar, descobri que esse arquivo não roda em nenhum dos dois runners hoje (o glob do `test:unit` não lista `autoCombo` e o `include` do Vitest só pega `.tsx` nessa pasta); é um problema pré-existente do repositório, sem relação com esta leva, e vou registrá-lo separadamente.

O diegosouzapw#12636 conflitava apenas na lista de testes do `@omniroute/opencode-plugin/package.json`, de forma aditiva: o tip já tinha `models-fetcher.test.ts` (do diegosouzapw#12607, irmão desta mesma leva) e o diegosouzapw#12636 acrescenta `telemetry.test.ts`. Fiz a união dos dois lados (25 arquivos contra 24 de cada) em vez de escolher um, o que teria removido um arquivo da suíte do plugin em silêncio.

Obrigado, @RaviTharuma.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

4 participants