194 lines
6.0 KiB
Markdown
194 lines
6.0 KiB
Markdown
# Agent loop follow-up issues
|
|
|
|
## Issue 1: Fix dangling control tool calls across movement transitions
|
|
|
|
### Problem
|
|
|
|
`transition` and `complete` tool calls can be appended to the live conversation
|
|
without a matching tool result message. When the same `Conversation` is reused by
|
|
the next movement, strict OpenAI-compatible providers may reject the next request
|
|
because every assistant `tool_call` must have a corresponding `tool` message.
|
|
|
|
### Evidence
|
|
|
|
- `src/engine/agent-loop.ts`: assistant messages are recorded with all pending
|
|
tool calls, including control calls.
|
|
- `src/engine/agent-loop/terminal-control.ts`: valid `transition` returns a
|
|
`MovementResult` without a tool result message.
|
|
- `src/engine/agent-loop.ts`: valid `complete` returns immediately without a
|
|
tool result message.
|
|
- `src/engine/context/conversation.ts`: `replayableTurns()` sanitizes persisted
|
|
transcript replay, but live `conversation.messages` shared across movements is
|
|
not sanitized.
|
|
|
|
### Expected behavior
|
|
|
|
After a movement exits via `transition` or `complete`, the next model request
|
|
must not contain unresolved control `tool_call`s.
|
|
|
|
### Suggested fix
|
|
|
|
Normalize live conversation state before entering the next movement, or avoid
|
|
recording terminal/control tool calls without corresponding tool messages.
|
|
|
|
### Acceptance criteria
|
|
|
|
- Add a regression test that uses a shared `Conversation` across a real
|
|
`transition`.
|
|
- The second movement's model input contains no dangling `transition` or
|
|
`complete` tool calls.
|
|
- Existing agent-loop tests continue to pass.
|
|
|
|
---
|
|
|
|
## Issue 2: Make context overflow terminal defaults consistently abort
|
|
|
|
### Problem
|
|
|
|
Context overflow handling has two different policies:
|
|
|
|
- `buildContextOverflowResult()` converts terminal defaults (`COMPLETE`, `ASK`)
|
|
to `ABORT`.
|
|
- `applyContextManagerUpdate()` with `force_transition` uses
|
|
`movement.defaultNext ?? 'ABORT'` directly.
|
|
|
|
This means context pressure can falsely complete or ask from a movement whose
|
|
default next step is terminal, even though the safer policy is to abort on
|
|
context loss.
|
|
|
|
### Evidence
|
|
|
|
- `src/engine/agent-loop/context-control.ts`: `buildContextOverflowResult()`
|
|
normalizes `COMPLETE` and `ASK` to `ABORT`.
|
|
- `src/engine/agent-loop/context-control.ts`: `applyContextManagerUpdate()`
|
|
force transition uses `movement.defaultNext` directly.
|
|
|
|
### Expected behavior
|
|
|
|
All context-overflow forced exits should use the same terminal-default policy.
|
|
When context is compromised, terminal defaults should not be treated as
|
|
successful completion.
|
|
|
|
### Suggested fix
|
|
|
|
Share one helper for context-overflow movement results, or apply the same
|
|
terminal-default normalization in `applyContextManagerUpdate()`.
|
|
|
|
### Acceptance criteria
|
|
|
|
- Add a regression test for `force_transition` with `defaultNext: "COMPLETE"`.
|
|
- The result is an abort-style movement result, not a successful completion.
|
|
- Existing context overflow tests continue to pass.
|
|
|
|
---
|
|
|
|
## Issue 3: Enforce `why_no_default` for `needs_user_input`
|
|
|
|
### Problem
|
|
|
|
The prompt and tool description require `why_no_default` when calling
|
|
`complete({ status: "needs_user_input" })`, but runtime validation only requires
|
|
`missing_info`.
|
|
|
|
This lets the agent ask the user without documenting why it could not choose a
|
|
reasonable default, which weakens the intended "avoid unnecessary user
|
|
questions" behavior.
|
|
|
|
### Evidence
|
|
|
|
- `src/engine/agent-loop/prompt.ts`: instructs the model to provide
|
|
`why_no_default`.
|
|
- `src/engine/agent-loop/terminal-control.ts`: tool description says
|
|
`why_no_default` is required.
|
|
- `src/engine/agent-loop/terminal-control.ts`: `validateCompleteArgs()` only
|
|
checks `missing_info`.
|
|
- `src/engine/agent-loop/terminal-control.ts`: movement output ignores
|
|
`why_no_default`.
|
|
|
|
### Expected behavior
|
|
|
|
`needs_user_input` should be rejected unless both `missing_info` and
|
|
`why_no_default` are non-empty strings.
|
|
|
|
### Suggested fix
|
|
|
|
Validate `why_no_default` in `validateCompleteArgs()` and include it in the
|
|
result/debug output if useful.
|
|
|
|
### Acceptance criteria
|
|
|
|
- Add a negative test for missing `why_no_default`.
|
|
- Add a positive test for `needs_user_input` with both fields.
|
|
- Existing `complete` behavior remains unchanged for other statuses.
|
|
|
|
---
|
|
|
|
## Issue 4: Validate interactive browse waiting-human session id
|
|
|
|
### Problem
|
|
|
|
`parseInteractiveBrowseWaitingHuman()` casts `sessionId` to `string` without
|
|
checking that it is present and actually a string.
|
|
|
|
If the tool returns malformed `waiting_human` output, the movement result can
|
|
carry `browserSessionId: undefined` despite the type expecting a string.
|
|
|
|
### Evidence
|
|
|
|
- `src/engine/agent-loop/tool-dispatcher.ts`: `sessionId` is read via
|
|
`parsed["sessionId"] as string`.
|
|
- The function validates `action` and `waitReason`, but not `sessionId`.
|
|
|
|
### Expected behavior
|
|
|
|
Malformed waiting-human output should not produce a typed waiting-human movement
|
|
result with an undefined session id.
|
|
|
|
### Suggested fix
|
|
|
|
Require `typeof parsed["sessionId"] === "string"` and a non-empty value before
|
|
returning a waiting-human result.
|
|
|
|
### Acceptance criteria
|
|
|
|
- Add a test for malformed `waiting_human` output without `sessionId`.
|
|
- Add a test for valid `waiting_human` output.
|
|
- Existing interactive browse behavior remains unchanged for valid tool output.
|
|
|
|
---
|
|
|
|
## Issue 5: Split `tool-dispatcher.ts` before it becomes the next agent-loop
|
|
|
|
### Problem
|
|
|
|
`tool-dispatcher.ts` now owns several distinct responsibilities:
|
|
|
|
- cache routing
|
|
- cache hit/miss event logging
|
|
- tool execution
|
|
- memory checkpoint behavior
|
|
- waiting-human parsing
|
|
- batched result recording
|
|
|
|
This file is becoming the next high-coupling module after the agent-loop split.
|
|
|
|
### Expected behavior
|
|
|
|
Tool dispatch should be decomposed into focused modules so bugs in cache
|
|
routing, execution, and special tool outputs are easier to test independently.
|
|
|
|
### Suggested fix
|
|
|
|
Split along behavioral boundaries, for example:
|
|
|
|
- `tool-cache-routing.ts`
|
|
- `tool-execution.ts`
|
|
- `tool-result-recorder.ts`
|
|
- `interactive-browse-result.ts`
|
|
|
|
### Acceptance criteria
|
|
|
|
- No behavior change.
|
|
- Existing agent-loop and tool-loop tests pass.
|
|
- New modules expose narrow, testable functions.
|