fix(agents): detect empty provider responses as failures, improve Telegram error routing#60830
fix(agents): detect empty provider responses as failures, improve Telegram error routing#60830garnetlyx wants to merge 2 commits intoopenclaw:mainfrom
Conversation
…egram error routing
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fb3b7e10a6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| // Re-throw dispatcher crashes after fallback/cleanup so callers can log/alert | ||
| if (caughtDispatchError) { | ||
| throw caughtDispatchError; |
There was a problem hiding this comment.
Re-throw dispatch crashes before early no-response return
When dispatchReplyWithBufferedBlockDispatcher throws and caughtDispatchError is set, the function can still exit via the !hasFinalResponse early return (for example if fallback delivery also fails or only partial/non-final output was sent), so the new re-throw block is never reached. This silently swallows dispatcher crashes that previously propagated, which can hide production failures from upstream logging/alerting and make incident triage harder.
Useful? React with 👍 / 👎.
Greptile SummaryThis PR introduces two improvements: a typed Key changes:
Confidence Score: 4/5Safe to merge with minor observability and test-coverage gaps; no runtime correctness issues found. The core logic is sound: EmptyResponseError propagates correctly through the fallback loop and into the Telegram dispatch routing in all scenarios. Abort/timeout re-throw prevents spurious fallback replies. The only concerns are: (1) the meta-field heuristic in runFallbackAttempt is not type-enforced and could silently break if either EmbeddedPiRunResult or raw provider response shapes change; (2) EmptyResponseError from the last/only candidate bypasses attempts.push and params.onError, reducing observability; (3) the single-candidate empty-content path is not covered by tests. None of these are correctness bugs in the current code. src/agents/model-fallback.ts (meta-field guard and last-attempt observability); src/agents/model-fallback.test.ts (single-candidate empty-content test coverage)
|
| // Check for empty raw provider response content array (treat as failure) | ||
| // Do NOT check EmbeddedPiRunResult payloads here - that should be handled upstream | ||
| // to avoid breaking valid use cases like memory flush which intentionally return empty | ||
| const result = runResult.result; | ||
| if ( | ||
| result && | ||
| typeof result === "object" && | ||
| "content" in result && | ||
| Array.isArray(result.content) && | ||
| result.content.length === 0 && | ||
| // Only apply to raw provider responses (not wrapped in EmbeddedPiRunResult) | ||
| !("meta" in result) | ||
| ) { |
There was a problem hiding this comment.
meta-field discriminant for EmbeddedPiRunResult
The !("meta" in result) guard is load-bearing for correctness: if a raw provider ever adds a meta field to its empty-content response (or if EmbeddedPiRunResult ever drops its meta field), the detection will silently misfire in the wrong direction. Since this heuristic is also completely invisible in the type system, future callers of runWithModelFallback have no way to know their result type needs to satisfy the guard.
A more robust approach would be to either:
- Pass an explicit
isRawProviderResulttype predicate intorunFallbackAttempt, or - Require callers to mark which result types should trigger the empty-content check (e.g. a wrapper type or a config flag)
The current approach is an improvement over the prior string-matched check, but it introduces a new latent coupling between runFallbackAttempt's heuristic and the shape of EmbeddedPiRunResult that isn't enforced anywhere in the type system.
Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-fallback.ts
Line: 163-175
Comment:
**Fragile `meta`-field discriminant for EmbeddedPiRunResult**
The `!("meta" in result)` guard is load-bearing for correctness: if a raw provider ever adds a `meta` field to its empty-content response (or if `EmbeddedPiRunResult` ever drops its `meta` field), the detection will silently misfire in the wrong direction. Since this heuristic is also completely invisible in the type system, future callers of `runWithModelFallback` have no way to know their result type needs to satisfy the guard.
A more robust approach would be to either:
1. Pass an explicit `isRawProviderResult` type predicate into `runFallbackAttempt`, or
2. Require callers to mark which result types should trigger the empty-content check (e.g. a wrapper type or a config flag)
The current approach is an improvement over the prior string-matched check, but it introduces a new latent coupling between `runFallbackAttempt`'s heuristic and the shape of `EmbeddedPiRunResult` that isn't enforced anywhere in the type system.
How can I resolve this? If you propose a fix, please make it concise.| it("treats empty content array (no meta) as failure and falls back to next candidate", async () => { | ||
| const cfg = makeCfg({ | ||
| agents: { | ||
| defaults: { | ||
| model: { primary: "openai/gpt-4.1-mini", fallbacks: ["fallback/ok-model"] }, | ||
| }, | ||
| }, | ||
| }); | ||
| const run = vi | ||
| .fn() | ||
| .mockResolvedValueOnce({ content: [] }) // primary: empty, no meta → EmptyResponseError | ||
| .mockResolvedValueOnce("ok"); // fallback succeeds | ||
| const result = await runWithModelFallback({ | ||
| cfg, | ||
| provider: "openai", | ||
| model: "gpt-4.1-mini", | ||
| run, | ||
| }); | ||
| expect(result.result).toBe("ok"); | ||
| expect(run).toHaveBeenCalledTimes(2); | ||
| }); |
There was a problem hiding this comment.
The new test only covers the two-candidate case (primary returns { content: [] }, fallback returns "ok"). The single-candidate path is meaningfully different: isKnownFailover is false and i === candidates.length - 1, so EmptyResponseError is thrown directly (before attempts.push and onError), bypassing throwFallbackFailureSummary.
It would be good to add a test asserting that when the only candidate returns { content: [] }, the call rejects with an EmptyResponseError (not a generic "All models failed" wrapper):
it("throws EmptyResponseError when the only candidate returns empty content", async () => {
const cfg = makeCfg({
agents: { defaults: { model: { primary: "openai/gpt-4.1-mini" } } },
});
const run = vi.fn().mockResolvedValueOnce({ content: [] });
await expect(
runWithModelFallback({ cfg, provider: "openai", model: "gpt-4.1-mini", run }),
).rejects.toBeInstanceOf(EmptyResponseError);
expect(run).toHaveBeenCalledTimes(1);
});Prompt To Fix With AI
This is a comment left during a code review.
Path: src/agents/model-fallback.test.ts
Line: 1210-1230
Comment:
**Missing test for single-candidate empty-content scenario**
The new test only covers the two-candidate case (primary returns `{ content: [] }`, fallback returns `"ok"`). The single-candidate path is meaningfully different: `isKnownFailover` is false and `i === candidates.length - 1`, so `EmptyResponseError` is thrown directly (before `attempts.push` and `onError`), bypassing `throwFallbackFailureSummary`.
It would be good to add a test asserting that when the only candidate returns `{ content: [] }`, the call rejects with an `EmptyResponseError` (not a generic "All models failed" wrapper):
```ts
it("throws EmptyResponseError when the only candidate returns empty content", async () => {
const cfg = makeCfg({
agents: { defaults: { model: { primary: "openai/gpt-4.1-mini" } } },
});
const run = vi.fn().mockResolvedValueOnce({ content: [] });
await expect(
runWithModelFallback({ cfg, provider: "openai", model: "gpt-4.1-mini", run }),
).rejects.toBeInstanceOf(EmptyResponseError);
expect(run).toHaveBeenCalledTimes(1);
});
```
How can I resolve this? If you propose a fix, please make it concise.…mpty-result predicate - Re-throw caughtDispatchError in Telegram early-return path (P1) - Treat EmptyResponseError as known failover for observability (P2) - Replace fragile meta-field heuristic with isRawProviderResult option (P2) - Add single-candidate empty-content test (P2)
|
Codex review: needs changes before merge. Summary Reproducibility: yes. The blocking PR issue is reproducible by source inspection: current main has no tracked Next step before merge Security Review findings
Review detailsBest possible solution: Adapt the contribution onto current main by using the existing classifier/failover seam for empty results, moving Telegram message selection into Do we have a high-confidence way to reproduce the issue? Yes. The blocking PR issue is reproducible by source inspection: current main has no tracked Is this the best way to solve the issue? No. The intended behavior is useful, but the submitted patch is not the best current implementation because it targets an obsolete Telegram path and bypasses the classifier-based fallback contract already present on main. Full review comments:
Overall correctness: patch is incorrect Acceptance criteria:
What I checked:
Likely related people:
Remaining risk / open question:
Codex review notes: model gpt-5.5, reasoning high; reviewed against 4407c317f38e. |
Summary
EmptyResponseErrortyped class inmodel-fallback.ts; replaces the fragile string-matchednew Error("Empty response content received from provider")runFallbackAttempt: treats a raw provider response withcontent: []and nometafield as a failure, triggering fallback to the next candidate (benefits all channels)lastErrorviaonErrorand an outer try/catch; routes the user-visible fallback message by error type (rate limit / empty response / general); re-throws aborts and timeouts so they don't produce spurious fallback replies; re-throws other dispatcher crashes after cleanup so callers still see the failure for logging/alertingmetafield guard (accepted as valid)Related
Follow-up work tracked in:
Test plan
pnpm test src/agents/model-fallback.test.ts— all tests pass including 3 new onesbunx oxlint src/agents/model-fallback.ts src/telegram/bot-message-dispatch.ts— 0 errors/stopor timeout → no spurious fallback reply sent