fix(sdk): bound stalled model queries

[ahmadalguydi]

Summary

Fixes #96 by giving SDK model turns an optional, validated timeoutMs deadline. A timed-out query now aborts the active agent and stops waiting on a stalled provider response; query().abort() is also forwarded to the active agent instead of only aborting an unused controller. Chat-history summarization opts into a 30-second deadline.

The issue references an older runAgent path; current main exposes the same behavior through query() and chat-history.ts, so this patch applies the requested design at the current API boundary.

Validation

• npm run build
• npm test — 67 tests passed
• git diff --check
• Added validation for invalid timeout values and pre-aborted queries

This PR is ready for review.

[stealthwhizz]

Thanks for the implementation! I went through the changes and everything looks good overall. I only have a couple of minor suggestions before merge:

1. Double agent.abort() on timeout (non-blocking)
   abortQuery() calls ac.abort(), which synchronously triggers the forwardAbort listener and calls agent.abort(). Immediately after, abortQuery() also calls activeAgent?.abort(), so the agent ends up being aborted twice on timeout. This is likely harmless if Agent.abort() is idempotent, but it seems unintended. It may be cleaner to either:

   • let ac.abort() + forwardAbort handle the abort entirely, or
   • remove forwardAbort and let abortQuery() own both sides.

2. Pre-aborted controller test could be stronger (non-blocking)
   The current assertion:

   assert.ok(messages.some((m) => m.type === "system"));

   passes as long as any system message exists (for example, session_start). It doesn't actually verify that the pre-aborted signal prevented the query from running. A stronger assertion could check that no assistant messages were produced, or that the query completed without an agent_end/LLM execution.

3. Minor documentation clarification
   The timeoutMs JSDoc currently describes the timeout as applying to each model turn, but it may be worth explicitly stating that this is a per-turn timeout, not a timeout for the entire query, so callers don't misinterpret its behavior.

Other than those minor points, the implementation looks solid. The abort forwarding lifecycle is handled correctly, cleanup happens via finally, timeout validation is good, and I didn't notice any security concerns or dependency issues. Nothing here is blocking from my perspective.

[ahmadalguydi]

Addressed the requested follow-up in commit c220f4e:

• removed the duplicate Agent.abort() call; the abort controller forwarder is now the single owner of agent cancellation
• strengthened the pre-aborted query test to assert no deltas, an empty zero-token aborted assistant result, and no model content
• clarified in QueryOptions that timeoutMs applies per model turn, not to the entire query
• added an early signal guard before each prompt so a pre-aborted query never starts model execution

Validation: npm test (67 passed).

[ahmadalguydi]

Correction: the follow-up commit is c985244 (the prior comment's c220f4e was a typo).

[stealthwhizz]

After checking the current HEAD, I don't see any remaining issues. The implementation looks good to merge from my side.

[shreyas-lyzr]

Three commits reviewed in full. The implementation is correct and the tests are solid. A few minor points below — none are blocking.

Commit 1 — fix(sdk): bound model query duration

Core logic is sound. The timer is correctly scoped per promptWithTimeout call and cleared on every exit path via finally. abortQuery correctly routes through ac.abort() which then fires the forwardAbort listener to reach the agent.

One cosmetic issue: the validation block at line 135 is not indented to match the surrounding try body (everything else inside the try is indented one level deeper). Does not affect runtime behavior.

Commit 2 — test(sdk): cover query timeout and abort

The invalid-timeout test is clean. The pre-aborted-controller test is thorough but has a latent assumption: it asserts assistantMessages[0].stopReason === "aborted" and totalTokens === 0. That assertion depends on @mariozechner/pi-agent-core emitting a message_end event with stopReason: "aborted" and zero usage when aborted before the first LLM call. If the underlying agent were to emit no message_end at all on abort, the test would fail at assistantMessages.length === 1 — not at the right assertion. This is a narrow coupling to upstream behavior but not a bug in this PR.

Commit 3 — fix(sdk): honor pre-aborted query signals

This is the most important commit. Removing activeAgent and replacing the double-abort pattern with event-listener forwarding is cleaner and correct. The three new if (ac.signal.aborted) return guards cover the right call sites — before the single-turn prompt, at the start of each multi-turn iteration, and inside promptWithTimeout itself. No gap.

Security pass: no new dependencies, no hard-coded secrets or tokens, no injection vectors, no frontend exposure. Clean.

Overall: the implementation is correct on all realistic paths, teardown is idempotent, and the tests verify the key behaviors. Ready to merge.

[ahmadalguydi]

Follow-up commit ea4ef91 addresses the remaining cosmetic review note by aligning the timeoutMs validation block with the surrounding try body. npm test still passes all 67 tests.

For the pre-aborted test coupling noted in review: the test intentionally asserts the SDK's documented terminal assistant result (stopReason: aborted, zero usage, empty content), not merely that the stream settles. That makes a regression in the public abort contract visible; if the underlying agent changes its event contract, this test will correctly flag the SDK behavior that needs to be adapted rather than silently weakening coverage.

[stealthwhizz]

Two bugs on the abort path — pre-abort test fails locally

I checked out this branch at ea4ef91 and ran the suite from a clean state:

npm install
npm run build
npm test

Result: 66/67 tests pass.

The failing test is:

honors an already-aborted controller before starting the model

It times out after 2 seconds with:

aborted query did not settle

This does not match the PR description's claim of 67 tests passed.

Bug 1: agent.abort() does nothing before the first prompt

Line 330 calls agent.abort() when the signal is already aborted.

However, Agent.abort() in pi-agent-core is:

abort() {
    this.activeRun?.abortController.abort();
}

activeRun is only created once agent.prompt() has started a run. If abort() is called before that, activeRun is undefined, so the call is effectively a no-op. The abort signal never reaches the agent.

Bug 2: Early returns skip the only channel.finish() call

channel.finish() is called at line 584 inside the try block, after the prompt logic.

The four new abort guards (return at lines 526, 545, 549, and 569) are also inside the same try block, but before that call. Returning exits the try block immediately, so execution never reaches channel.finish().

The finally block performs cleanup and closes the session span, but it does not call channel.finish().

This causes the hang:

• createChannel().pull() only resolves when either:
  • a message is pushed, or
  • finish() is called.
• If neither happens, the consumer's for await loop waits forever on an unresolved promise.

That is exactly what the failing test is catching.

How they compound

Fixing only Bug 2 allows the query to settle, but the agent never receives the abort signal.

Fixing only Bug 1 correctly aborts the agent, but the consumer still hangs because the channel is never finished.

Both issues need to be addressed.

Suggested fix

• Move channel.finish() into the finally block so it executes on every exit path.
• For the pre-abort case, either:
  • restructure the flow so agent.prompt() still starts, allowing the normal abort path to emit the expected stopReason: "aborted" response, or
  • emit a synthetic assistant message with stopReason: "aborted" before the early return.

The test's assertions are correct. The current implementation does not satisfy them.

[stealthwhizz]

@ahmadalguydi Request for issue before sending a PR from next time . Thanks

[ahmadalguydi]

Fixed in commit 38da74b. The pre-aborted path now emits the documented zero-token aborted assistant result without attempting a model run, and the query channel is finished from the async-run finally block so every early-return path settles. Removed the ineffective pre-run agent.abort() call. Validation: npm run build, npm test (67/67), and git diff --check pass. I also noted the process feedback: I should request or confirm an issue before opening future PRs.

[ahmadalguydi]

The fix is now pushed at 38da74b; npm run build and all 67 tests pass. Please re-review the current HEAD when convenient. The PR addresses issue #96.