feat(agent): persist unresolved manual tool calls as awaiting_client_tools#64
Conversation
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
|
Reviewed Verified: flag resets after first save (multi-turn safe); loaded state object no longer mutated (good catch — Suite verified: 27 files / 304 tests green. |
![perry-the-pr-reviewer[bot]](https://avatars.githubusercontent.com/in/3222961?s=60&v=4){kind=small}
There was a problem hiding this comment.
Perry's Review
Verdict: 💬 Comments / questions
(Posting as COMMENT, not APPROVE: the maintainer GitHub App lacks pull_requests:write on OpenRouterTeam, so Perry cannot record a formal approval here. Content-wise this is an LGTM.)
Details
Clean, additive feature. Unresolved manual tool calls now persist to pendingToolCalls with a new distinct awaiting_client_tools status at all three stop paths (first-round all-manual, mid-loop all-manual, mixed auto+manual), mirroring persistHitlPause exactly. The resume semantics are the careful part and they're right: resumingFromClientTools defers clearing the stale pendings until after the resumed request produces a response (cleared atomically in saveResponseToState), so a failed resume leaves the paused state intact — and there's a test for exactly that. HITL (awaiting_hitl) is untouched, requiresApproval() now also reports the new status, and the type + README doc comments are updated. Suite green locally (7/7), including a JSON round-trip and a HITL no-regression case.
Risk is Medium only because this adds a new public ConversationStatus value and changes the terminal state of a previously-"complete" (well, previously-mislabeled) run — consumers switching exhaustively on status will need the new arm. That's the intended behavior and it's a minor changeset, so it's fine; flagging for reviewer awareness. Two non-blocking notes inline.
| if ( | ||
| status === 'awaiting_approval' || | ||
| status === 'awaiting_hitl' || | ||
| status === 'awaiting_client_tools' |
There was a problem hiding this comment.
Adding awaiting_client_tools to the public ConversationStatus union is a breaking-ish surface change for any consumer doing an exhaustive switch on status (the repo's own style mandates default: value satisfies never). Please confirm no in-repo or known downstream consumer switches exhaustively on ConversationStatus without a default, and that the changeset being minor (not major) is the intended call for adding an enum member.
▶ Prompt for agents: Grep consumers for exhaustive switches over ConversationStatus and confirm they either have a default arm or are updated to handle awaiting_client_tools. Confirm the minor changeset bump is correct for adding a public status value.
There was a problem hiding this comment.
Checked: no in-repo consumer switches exhaustively on ConversationStatus — the only status checks are equality comparisons (requiresApproval(), resume gating in initStream, and tests), all updated to include awaiting_client_tools. Grepped every switch in packages/; none discriminate on ConversationStatus (they're all on stream event / item / block types). On the changeset: package is pre-1.0 (0.7.2), so minor is the breaking-allowed bump under 0.x semver and matches prior surface additions in this repo — intended call.
| ): Promise<void> { | ||
| this.finalResponse = currentResponse; | ||
|
|
||
| if (!this.stateAccessor || unresolvedCalls.length === 0) { |
There was a problem hiding this comment.
persistClientToolsPause early-returns without persisting when !this.stateAccessor, so a manual-tool run with no state accessor now silently ends with status left as-is (not complete, since the old markStateComplete() path was replaced) and no pendings surfaced. That's arguably better than the old false complete, but confirm the no-accessor manual path is acceptable — the caller can still read getPendingToolCalls() off the in-memory result, but nothing is durable. Worth a one-line test or doc note on the no-accessor behavior.
▶ Prompt for agents: Add a test for the manual-tool stop path with no StateAccessor to pin the intended behavior (result exposes pending calls in-memory; nothing persisted), or document that manual tools require a StateAccessor to be recoverable.
There was a problem hiding this comment.
Done in 07ac7f6 — added a test pinning the no-accessor path: the loop still stops after the unresolved manual call (no orphan follow-up request), getPendingToolCalls() returns [], and the caller reads the unresolved function_call items off getResponse().output. Also documented on persistClientToolsPause that this mirrors persistHitlPause and that manual tools need a StateAccessor to be recoverable across processes. Note the in-memory result is still fully usable — nothing is silently lost within the process; it's only durability that requires the accessor.
…tools Manual tools (execute: false) stopped the loop without persisting anything; getPendingToolCalls() returned [] and cold-start consumers could not see the calls in serialized state. Persist unresolved manual calls to pendingToolCalls with new status 'awaiting_client_tools' at all three stop paths. Resume with new input clears pendings. HITL (awaiting_hitl) untouched.
…quirement Addresses Perry's review: without a StateAccessor nothing is persisted (mirroring persistHitlPause) — getPendingToolCalls() returns [] and the caller reads unresolved function_call items off getResponse().output. Added a test pinning this and a doc note on persistClientToolsPause.
PR #63 hardened the no-tool exit to reject empty final outputs (validateFinalResponse), which this test's mock tripped after rebase. A real resumed response carries assistant content; fixture updated.
07ac7f6 to
ef7c4da
Compare
1 participant
Problem
Manual tools (
execute: false) stop the loop correctly (#59) but persist nothing:getPendingToolCalls()→[], status staysin_progress, and serialized state carries no trace of the calls. Serverless/cold-start consumers must scrapegetNewMessagesStream()to capture proposals (agent-monorepo wikillm FRICTION #3).Fix
Persist unresolved manual calls to
state.pendingToolCallswith newConversationStatus'awaiting_client_tools'at all three stop paths (first-round all-manual, mid-loop all-manual, mixed unresolved). Resume with new input clears pendings (manual calls aren't approved/rejected by id — callers execute externally and continue withfunction_call_output). HITLawaiting_hitluntouched.Tests
4 new cases in
manual-tool-pending-state.test.tsincl. JSON round-trip + HITL no-regression; suite 301 green, zero existing tests adjusted. Changeset: minor (additive status).