Skip to content

feat(progress): add timeout protection and strip withGadgets from callProgressModel#506

Merged
zbigniewsobiecki merged 1 commit intodevfrom
feature/progress-model-timeout
Feb 23, 2026
Merged

feat(progress): add timeout protection and strip withGadgets from callProgressModel#506
zbigniewsobiecki merged 1 commit intodevfrom
feature/progress-model-timeout

Conversation

@aaight
Copy link
Copy Markdown
Collaborator

@aaight aaight commented Feb 23, 2026

Summary

  • Add PROGRESS_TIMEOUT_MS = 10_000 constant and wrap callProgressModel in a Promise.race with a 10s timeout — matching the pattern used by generateAckMessage in ackMessageGenerator.ts
  • Remove unnecessary .withGadgets() from the AgentBuilder chain in callProgressModelOnce (empty gadgets still initialises the subsystem with no benefit for a one-shot call)
  • Extract inner LLM call to callProgressModelOnce so the timeout wrapper stays clean
  • Add 9 direct unit tests for callProgressModel covering: success, multi-event concatenation, non-text event filtering, empty-output error, no-events error, timeout, and LLM throw

Test plan

  • tests/unit/backends/progressModel.test.ts — 9 new unit tests for callProgressModel (all pass)
  • tests/unit/backends/progress.test.ts — 43 existing ProgressMonitor tests pass unchanged (mock signature unchanged)
  • Full test suite (2734 tests) passes
  • Lint passes (biome)
  • Type check passes

Card

https://trello.com/c/7TYShDQg/91-lets-change-how-progress-is-monitored-from-an-agentic-loop-into-one-shot-llm-call-like-we-do-acknowledgements-in-the-router-that

🤖 Generated with Claude Code

@aaight
Copy link
Copy Markdown
Collaborator Author

aaight commented Feb 23, 2026

🤔 Just a sec, looking into that timeout protection

Copy link
Copy Markdown
Collaborator

@nhopeatall nhopeatall left a comment

Choose a reason for hiding this comment

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

Summary

LGTM — clean implementation of timeout protection for callProgressModel, with proper cleanup and a sensible extraction of the inner LLM call.

Notes

The timeout implementation is actually an improvement over the existing ackMessageGenerator.ts pattern — it correctly clears the timer in a finally block and suppresses the unhandled rejection on the timeout promise (the ack generator does neither). Nice.

Minor observations (non-blocking)

  • tests/unit/backends/progressModel.test.ts:38-41getMockRun() helper is defined but never called in any test. Dead code that can be removed.
  • tests/unit/backends/progressModel.test.ts:140-177 — The timeout test doesn't exercise the actual setTimeout timeout path. It makes the LLM async generator throw the same error message by manually calling rejectFn. This is effectively a second "LLM throws" test. Using vi.useFakeTimers() + vi.advanceTimersByTime(10_000) would properly verify the real timeout branch wins the race. The comments acknowledge this limitation — just noting it could be strengthened if desired.

function getMockRun(): ReturnType<typeof vi.fn> {
const instance = MockAgentBuilder.mock.results[MockAgentBuilder.mock.results.length - 1]?.value;
return instance?.ask.mock.results[0]?.value?.run;
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nitpick: getMockRun is defined but never called — leftover from an earlier draft? Safe to remove.

@zbigniewsobiecki zbigniewsobiecki merged commit 5951d48 into dev Feb 23, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants