Repository navigation
Conversation
aviggiano
left a comment
There was a problem hiding this comment.
Verdict: REQUEST_CHANGES for 743c29ffea446b3aeded2c1ccdbdf33590cd753b.
Reviewed in a fresh detached worktree against freshly fetched origin/unstable (81aec3bdff8fa943118d9979a2a68eaa4c678466). This is a COMMENT review because the authenticated account also authored the PR.
-
[P1] Preserve the underlying scheduler controls when the 429 is detected from stderr/events. In
openRouterRecoveryBudgetError, lines 574–584,context.lastRateLimitErrorcan be the plainErrorcreated byterminalRateLimitError(error), with the structured provider error in its cause. Reading only the wrapper'scode/detailsthen producesAGENT_CLI_ERRORand dropsfailureQuota/quotaResetAtMs. The workflow normalizer only preserves top-level controls, so the scheduler can spend another attempt instead of retaining quota-wait authority.This is reproducible using the pinned Smithers
classifyQuotaError(): pass"provider diagnostic prefix ".repeat(20) + "HTTP 429 Too Many Requests; retry after 360 seconds". Smithers correctly returnsAGENT_QUOTA_EXCEEDEDwith a reset timestamp, but its 300-character message summary omits the HTTP status. Feed the complete text throughoptions.onStderr, then throw that real classified error at elapsed 9,500 ms with a 12,000 ms timeout. The reserve correctly refuses the first 1,000 ms wait, but the terminal result isAGENT_CLI_ERRORwithout the quota controls. Expected: preserveAGENT_QUOTA_EXCEEDED,failureQuota, and the original reset timestamp while adding the budget diagnostics. A separate structured-error probe also confirms that the ternary at line 581 downgradesAGENT_CONFIG_INVALIDtoAGENT_CLI_ERROR.Carry the original authoritative, allowlisted scheduler controls through the adapter's own diagnostic wrappers, and preserve applicable non-quota control codes rather than assigning
AGENT_CLI_ERRORunconditionally. Add coverage where the 429 signature is outside the provider error's summarized message; the existing direct-quota test does not cover this path. -
[P2] Recheck cancellation and the fixed deadline after releasing terminal callbacks. The new stop paths at lines 358–360 and 604–605 call
releaseRelayOrThrow()and immediately construct a budget error. Releasing buffered provider stderr/events invokes caller callbacks synchronously. If such a callback aborts the caller signal, the result still hasAGENT_QUOTA_EXCEEDED; if it consumes the remaining hard deadline, the result still hasAGENT_CLI_ERROR/finalization-reservedwithremainingMs: 0.Independent probes buffer
"HTTP 429 terminal provider output\n"at elapsed 9,500 ms of the same 12,000 ms timeout. The caller'sonStderreither aborts its signal or advances the deterministic clock to 12,000 ms when that buffered line is released. Both fail: expectedPROCESS_ABORTEDwith the cancellation cause, orPROCESS_TIMEOUT, respectively. Recheck the caller signal and monotonic total deadline after terminal relay release before returning the recovery-budget error. Keep the existing callback-error precedence, and launch no replacement attempt.
Validation completed:
- Runtime and dependency builds, runtime test compilation, strict changed-code lint, Prettier, and
git diff --check: passed. - Existing generated OpenRouter contracts: 22 passed. Adapter-boundary contracts: 7 passed.
- Independent deterministic provider-stub probes: 17 total, 13 passed and the four cases grouped above failed. Passing probes cover production cumulative-budget exhaustion, reserve boundaries, shrinking timeouts, productive-progress resets, delayed waits, partial-wait cancellation accounting, non-429 errors, replacement-error precedence, zero/subsecond timeouts, epoch-clock jumps, hard-deadline checkpoint rejection, conflicting sessions, replayed checkpoint timestamps, diagnostic payload exclusion, and frozen-result/terminal-telemetry behavior.
- CI at review time: Build gates, external static analysis, Semgrep, and both Socket checks passed; the full release-validation lanes were still running or queued.
Scope remains separate from #1300's backend-finalization grace and #955's dependency/durable-resume upgrade. No dependency pins, controller-restart checkpoint authority, or backend grace policy change in this diff. The PR remains draft pending the fixes and re-review.
aviggiano
left a comment
There was a problem hiding this comment.
Verdict: APPROVE for bed4bcb25ff7059b77385de9a7b76f95902a9e28.
Re-reviewed the exact follow-up diff from 743c29ff and the cumulative change against freshly fetched origin/unstable (81aec3bd) in a new detached worktree. Both grouped findings from the previous review are resolved:
- The adapter's diagnostic wrapper now carries the original allowlisted scheduler code and details. The real Smithers quota-summary case preserves
AGENT_QUOTA_EXCEEDEDand its reset controls; direct configuration/checkpoint controls are retained without promoting arbitrary causes. - Terminal relay delivery now preserves callback-error precedence, then rechecks caller cancellation and the monotonic hard deadline before returning a recovery-budget error. Both budget-stop paths return the expected cancellation/timeout authority without launching another attempt.
Independent verification at this exact head:
- Original 17-probe provider-stub harness: 17 passed, including all four previously failing probes. Only its worktree path was made configurable.
- Generated OpenRouter contracts: 25 passed. Adapter-boundary contracts: 7 passed.
- Runtime/dependency build, runtime test compilation, strict diff lint, Prettier, and
git diff --check: passed. - CI run 38741257989: passed at the reviewed head. All 16 PR checks are successful, including every full release lane,
release-gates, Semgrep, and both Socket checks.
The cumulative wait budget, fixed caller deadline, finalization reserve checks, checkpoint/session handling, and diagnostic payload boundaries remain intact. Scope remains separate from #1300's backend-finalization grace and #955's dependency/durable-resume upgrade. I found no remaining blocking issues in the reviewed diff.
This is an explicit APPROVE verdict submitted as a COMMENT review because the authenticated account also authored the PR. Draft status is left unchanged.
| #recordCheckpoint(): void { | ||
| if (this.#sawTerminalRateLimit || this.#isProvisionalStaging() || this.#isQuarantined()) return; |
There was a problem hiding this comment.
Buffered progress misses the checkpoint
#recordCheckpoint() skips progress while a possible 429 is unresolved. If stderr emits HTTP 429, Codex completes an action, and stderr then emits suffix, the relay releases the action because HTTP 429suffix is not a rate limit. But releasing those callbacks never records that progress.
If a real 429 follows without another action, providerRecovery.lastCheckpoint remains null or points to older work. Recovery still keeps the completed work, but operators get an incomplete progress report. Keep the event's original observation time while it is buffered, and record it once the possible rate limit is ruled out.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/runtime/src/templates/smithers/agents/openrouter.tsx
Line: 1347-1348
Comment:
**Buffered progress misses the checkpoint**
`#recordCheckpoint()` skips progress while a possible 429 is unresolved. If stderr emits `HTTP 429`, Codex completes an action, and stderr then emits `suffix`, the relay releases the action because `HTTP 429suffix` is not a rate limit. But releasing those callbacks never records that progress.
If a real 429 follows without another action, `providerRecovery.lastCheckpoint` remains null or points to older work. Recovery still keeps the completed work, but operators get an incomplete progress report. Keep the event's original observation time while it is buffered, and record it once the possible rate limit is ruled out.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Fixes #1305.
Sustained HTTP 429 recovery could spend the remaining task deadline sleeping. The OpenRouter adapter now refuses waits that would enter the existing finalization reserve and caps cumulative backoff at 120 seconds, including across recovery windows refreshed by productive activity. The original task deadline remains fixed, and authoritative scheduler codes/details survive the adapter's provider diagnostic wrappers. Terminal budget-stop callbacks preserve callback-error precedence and recheck caller cancellation and the monotonic hard deadline before returning a recovery-budget error.
Retry notices and terminal results/errors expose configured timeout/deadline, elapsed productive time and backoff, retry and rate-limit counts, remaining headroom, and the latest validated session checkpoint. Exact-session recovery retains completed work; replayed events do not refresh checkpoint time, and events at or beyond the hard deadline do not establish a new checkpoint. This checkpoint describes local session progress within an invocation. Existing portable checkpoint handling and stale-session protections continue to govern controller restarts.
Backend-finalization grace is tracked separately in #1300. This PR changes only the OpenRouter adapter, its deterministic regressions, and operator documentation.
Validation:
pnpm -w build— passed.pnpm -w typecheck— passed.pnpm --filter @ultrafuzz/runtime... buildandpnpm exec tsc -p packages/runtime/tsconfig.test.json --pretty false— passed after the final adapter/test changes.ULTRAFUZZ_STRICT_LINT=1 ESLINT_PLUGIN_DIFF_COMMIT=81aec3bd CI=1 pnpm exec eslint packages/runtime/src/templates/smithers/agents/openrouter.tsx packages/runtime/test/runtime.test.ts --max-warnings 0— passed.packages/runtime:bun test dist-test/test/runtime.test.js --test-name-pattern '^Bun adapter contract: generated OpenRouter'— 25 passed, including nine new deterministic provider-stub regressions for near-deadline recovery, sustained 429s, cumulative budgets, delayed callbacks/hard deadlines, aborted waits, real Smithers quota classification, direct scheduler controls, and terminal callback precedence.node --test packages/runtime/dist-test/test/agent-adapter-boundaries.test.js— 7 passed.git diff --check— passed.Draft pending independent review.
The PR appears safe to merge, with a non-blocking gap in checkpoint diagnostics for buffered progress.
Fix with agent prompt
Summary
The OpenRouter adapter now refuses recovery waits that would enter the finalization reserve and limits cumulative backoff to two minutes.
Diagram
%%{init: {'theme': 'neutral'}}%% flowchart TD A[Provider returns HTTP 429] --> B[Save scheduler controls and session progress] B --> C{Does the wait fit?} C -->|No| D[Release buffered callbacks] D --> E[Check callback error, cancellation, and hard deadline] E --> F[Return budget error with diagnostics] C -->|Yes| G[Wait and count actual elapsed backoff] G --> H[Recheck deadlines and budgets] H --> I[Retry fresh or resume the exact session]Reviews (1) · Last reviewed commit: "fix(runtime): preserve scheduler control..." · Reviewed by Greptile