Skip to content

fix(daemon): stop reading our own empty-output fallback as a hard quota - #6261

Open
maxmilian wants to merge 3 commits into
nexu-io:mainfrom
maxmilian:fix/quota-misclassify-empty-output
Open

fix(daemon): stop reading our own empty-output fallback as a hard quota#6261
maxmilian wants to merge 3 commits into
nexu-io:mainfrom
maxmilian:fix/quota-misclassify-empty-output

Conversation

@maxmilian

Copy link
Copy Markdown
Contributor

Refs #6143 — item (1) only, as scoped in the thread.

The misclassification is self-inflicted

apps/daemon/src/server.ts emits a generic fallback when a run produces no output:

Agent completed without producing any output. … then try re-authenticating the agent, checking quota, or switching models.

isHardQuotaText carried a bare quota alternative, so it matched that sentence. And the quota branch is evaluated before isEmptyOutputText, so empty_output was unreachable for this text.

Downstream, run-retry-policy.ts does if (failure?.failure_detail === 'hard_quota') return suppress('hard_quota') — which is why the reporter saw it stick permanently rather than retry, on providers that had quota left. inputTokens/outputTokens being 0 with no upstream status line fits exactly.

The fix follows a pattern already in this repo

apps/daemon/src/integrations/vela-errors.ts already gets this right — it requires a corroborating word:

return value.includes('quota') && /\b(wallet|balance|credit|billing|funds?)\b/.test(value);

isHardQuotaText now does the same: quota on its own is not a signal, it needs one of wallet/balance/credits/billing/funds/payment/plan. Phrases that are unambiguous on their own keep matching directly — session limit, usage limit, insufficient quota, exceeded your current quota, out of credits, DAILY_LIMIT_EXCEEDED, and the Chinese vela pre-charge texts.

Tests

Red-then-green in apps/daemon/tests/run-failure-classification.test.ts, feeding the exact server.ts fallback string:

  • the fallback now lands on empty_output / empty_output (was rate_limit / hard_quota)
  • corroborated quota messages still classify hard_quota, retryable: false — so this doesn't trade one silent misclassification for another
  • 用户额度不足 keeps its more specific amr_insufficient_balance detail

run-failure-classification.test.ts: 102 passed. pnpm typecheck: clean.

Full @open-design/daemon suite: 6104 passed, 10 failed — all 10 pre-exist on unmodified upstream/main (tests/amr-session-resume.test.ts, tests/langfuse-trace.test.ts; verified with a stash A/B on the same machine: 10 failed / 89 passed either way).

Scope

Deliberately item (1) only. Items (2)/(3) from the thread (the BYOK path itself and the diagnostics wording) are untouched, so the issue should stay open. Happy to take the retest offer on openai.mozhevip.top once this is buildable.

`isHardQuotaText` carried a bare `quota` alternative, so it matched the
daemon's own generic empty-output fallback (server.ts): "...then try
re-authenticating the agent, checking quota, or switching models."

The quota branch runs before `isEmptyOutputText`, so every output-less run —
for any reason — was classified `hard_quota`, and run-retry-policy suppresses
retries outright on that detail. That is why third-party API runs failed
permanently while the provider itself still had quota.

`quota` alone is no longer a signal; it needs a corroborating word. This
mirrors the detector that already got it right — integrations/vela-errors.ts
requires wallet/balance/credit/billing/funds alongside `quota`. Phrases that
are unambiguous on their own (session limit, insufficient quota, exceeded
your current quota, the Chinese vela pre-charge texts) keep matching directly.

Regression test feeds the exact server.ts fallback string and asserts it lands
on `empty_output`, plus a guard that corroborated quota messages still
classify as `hard_quota` and that the Chinese pre-charge text keeps its more
specific `amr_insufficient_balance` detail.

Refs nexu-io#6143
@lefarcen

Copy link
Copy Markdown
Contributor

Thanks @maxmilian — scoping this to item (1) from #6143 and testing against the exact daemon fallback string is the right shape. I've routed it into review now, and I'll handle the PR-body / validation follow-up on this thread while pool review picks up the code pass.

@lefarcen
lefarcen requested a review from PerishCode July 30, 2026 01:13
@lefarcen lefarcen added size/S PR changes 20-100 lines risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps type/bugfix Bug fix labels Jul 30, 2026
@lefarcen

Copy link
Copy Markdown
Contributor

Thanks for scoping this tightly to item (1) and for grounding it in the exact #6143 fallback path. Before pool review goes further, could you add the missing PR-template pieces — a short Why, What users will see, Surface area, and Validation section? The substance is mostly already in your write-up; this is mainly to make the reviewer-facing summary explicit.

@lefarcen lefarcen added the needs-validation Runtime change detected; needs human or /explore agent validation. label Jul 30, 2026
@lefarcen

Copy link
Copy Markdown
Contributor

🧪 Queued for QA validation — this PR has changes that need a manual QA pass before it's merged. Nothing needed from you; we'll update here once it's validated. Thanks for the contribution! 🙏

@PerishCode PerishCode 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.

The empty-output regression is correctly reproduced, but the replacement hard-quota predicate drops common unambiguous exhaustion forms. This needs a small matcher/test adjustment before the fix is safe to merge.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

return /\bquota\b/i.test(text) &&
/\b(wallet|balance|credits?|billing|funds?|payment|plan)\b/i.test(text);
}

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.

Preserve explicit exhaustion phrases such as quota exhausted and quota exceeded as hard quota signals. This new conjunction only recognizes quota when one of wallet/balance/credit/billing/funds/payment/plan also appears, so classify('AGENT_EXECUTION_FAILED', 'quota exhausted') now falls through to the generic retryable execution-failure path; with RATE_LIMITED, the same text becomes retryable rate_limit_429. That changes a genuine terminal quota failure into a retry candidate, contrary to the existing hard_quota suppression contract. The repository already contains the exact upstream phrase quota exhausted in apps/daemon/tests/byok-tools.test.ts, and quota exceeded is also a common provider form. Add tightly scoped alternatives such as quota (?:exceeded|exhausted|depleted|reached) (without restoring bare quota) and include these terse forms in the fixture matrix alongside the empty-output fallback.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen
lefarcen requested a review from PerishCode July 30, 2026 01:15
@lefarcen

Copy link
Copy Markdown
Contributor

Hey @maxmilian — PerishCode has the current blocker covered on this head: the quota matcher fix now misses terse terminal exhaustion forms like quota exhausted / quota exceeded, so the next step is to tighten that matcher and extend the regression matrix around those phrases before re-check.

💡 To drive this PR to merge hands-free, paste this to your AI coding agent (Claude Code / Codex / opencode / Cursor …):
Take over nexu-io/open-design#6261 until it merges — read https://raw.githubusercontent.com/nexu-io/looper/main/skills/pr-takeover/SKILL.md and follow it.

Review caught a recall regression in the previous commit: requiring a
corroborating word for `quota` also dropped phrases that are unambiguous on
their own. `quota exhausted` is a real upstream payload in this repo
(tests/byok-tools.test.ts: `status_msg: 'quota exhausted'`), and under
`RATE_LIMITED` it would have become a retryable `rate_limit_429` — turning a
terminal quota failure into a retry candidate, the exact inverse of the bug
this PR fixes.

`quota` adjacent to an exhaustion word (exceeded / exhausted / depleted /
reached / used up, in either order) now matches directly, without needing
wallet/balance/billing. The daemon's own fallback ("...checking quota, or
switching models.") still doesn't match, which is the invariant that started
this PR.

Tests cover both directions and both error codes, since RATE_LIMITED is where
the suppression contract actually breaks.
@maxmilian

Copy link
Copy Markdown
Contributor Author

Good catch — that's a real recall regression, and I verified the evidence you cited before fixing: apps/daemon/tests/byok-tools.test.ts:332 does carry status_msg: 'quota exhausted' as a real upstream payload. Fixed in 55ef79c29.

What was wrong: requiring a corroborating word for quota also dropped phrases that are unambiguous on their own. quota exhausted fell through to the generic execution-failure path, and — as you noted — under RATE_LIMITED it became a retryable rate_limit_429. That inverts the very contract this PR exists to protect.

Fix: quota adjacent to an exhaustion word now matches directly, in either order and without corroboration:

/\bquota\s+(?:exceeded|exhausted|depleted|reached|used\s+up)\b/i
/\b(?:exceeded|exhausted|out\s+of|ran\s+out\s+of|no\s+remaining)\s+(?:\w+\s+){0,3}quota\b/i

The corroboration rule now only governs a bare quota with no exhaustion context — which is what the daemon's own fallback has ("...then try re-authenticating the agent, checking quota, or switching models."), so the original invariant still holds.

Tests cover quota exhausted, quota exceeded, Quota depleted…, exceeded your monthly quota, and out of quota, each asserted under both AGENT_EXECUTION_FAILED and RATE_LIMITED — the second one deliberately, since that's the path where the suppression contract breaks rather than just the detail string being wrong.

run-failure-classification.test.ts 102 passed; run-failure-detail-status, run-failure-detail-persisted-message, run-retry-policy and byok-tools 95 passed; pnpm typecheck clean.

@PerishCode PerishCode 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.

The explicit exhaustion-phrase regression is fixed, but the fallback can still be reclassified by an unrelated event fragment because corroboration is applied across the full aggregated failure text. This remains a retry-suppression risk on the empty-output path.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

Comment on lines 181 to 182
}

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.

Keep the corroborating term in the same message as quota. collectFailureText joins the status error and up to 24 event fragments with newlines, but these two independent regexes search that whole aggregate. Therefore the exact empty-output fallback supplies quota, while an unrelated event such as Failed to load plan supplies plan; together they still return hard_quota, and run-retry-policy suppresses the retry this PR intends to restore. Match corroborators within the same line or a tightly bounded phrase around quota (or classify each collected fragment separately), and add a regression fixture combining EMPTY_OUTPUT_FALLBACK with an unrelated event containing plan or payment that must remain empty_output.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen

Copy link
Copy Markdown
Contributor

Hey @maxmilian — thanks for turning the first blocker around quickly. PerishCode’s current blocker on this head is narrower now: keep the corroborating term in the same collected fragment as quota, and add the mixed-fragment regression case (EMPTY_OUTPUT_FALLBACK + unrelated plan / payment) so the empty-output path can’t get reclassified back into hard_quota.

collectFailureText joins up to 24 unrelated messages with '\n', so applying
the corroboration rule to the joined string let any fragment mentioning a
plan or a payment vouch for the bare `quota` inside our own empty-output
fallback — putting the run back on the retry-suppressing hard_quota path.

Evaluate the predicate per collected fragment instead. This also stops the
`\s+` in the exhaustion-adjacency patterns from bridging two messages across
the join.
@maxmilian

Copy link
Copy Markdown
Contributor Author

Fixed in f15819003. You're right that the fragment is the actual boundary — collectFailureText joins up to 24 unrelated messages with \n, so corroborating over the joined string meant any fragment mentioning a plan or a payment could vouch for the bare quota sitting in our own fallback.

collectFailureParts now returns the parts and the predicate runs per fragment (parts.some(isHardQuotaFragment)).

While testing this I hit a second instance of the same root cause that isn't in your note: \s in /\bquota\s+(?:exceeded|exhausted|…)/ matches the join character itself, so a fragment ending in quota pairs up with the next fragment starting in exceeded. Fragment scope closes that too, and there's a regression case for it rather than leaving it implicit.

New tests in run-failure-classification.test.ts:

  • EMPTY_OUTPUT_FALLBACK alongside an unrelated fragment (…fallback plan for this workspace, Your payment details were updated…, …previous billing period snapshot) stays empty_output / empty_output
  • the split adjacency case (Reported usage against the quota + exceeded the configured step budget as separate fragments) stays empty_output
  • a fragment that corroborates itself still classifies hard_quota, retryable: false, even when it arrives in the same run as the fallback — so this doesn't buy the empty-output path back by breaking real quota detection

Red-verified before the change: with the previous run-failure-classification.ts the first case returns rate_limit / hard_quota.

run-failure-classification, run-retry-policy, run-failure-detail-status, byok-tools: 197 passed. pnpm typecheck: clean. The 10 pre-existing amr-session-resume / langfuse-trace failures are unchanged on this head (verified by running those two files alone).

@PerishCode PerishCode 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.

@maxmilian The current head correctly scopes hard-quota detection to individual collected fragments, preserves explicit exhaustion phrases under both generic and rate-limited codes, and adds regression coverage for unrelated-fragment and cross-newline false positives. I also verified the live PR head and successful required checks. Nicely handled—the follow-up closes both prior retry-suppression blockers with focused coverage.

🔁 Powered by Looper · runner=reviewer · agent=codex · An autonomous AI dev team for your GitHub repos.

@lefarcen
lefarcen requested a review from AmyShang-alt July 30, 2026 15:19
@lefarcen

Copy link
Copy Markdown
Contributor

Hey @maxmilian — PerishCode’s current head is clear, so I’ve now queued the manual QA pass for this revision. We’ll update here once that validation comes back.

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

Labels

needs-validation Runtime change detected; needs human or /explore agent validation. risk/high High risk: apps/desktop, daemon, auth, migration, workflows, package deps size/S PR changes 20-100 lines type/bugfix Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants