feat(agent-engine): add phase 2 orchestration and runtime provisioning - #493
Conversation
| conversationKey: agentengine.ConversationKey(binding.ConversationKey), | ||
| turnID: agentengine.TurnID(responseID), | ||
| } | ||
| h.addSessionTurn(turnKey, turn) |
There was a problem hiding this comment.
The turn is exposed to the cancel endpoint before Engine.Run has registered it. A cancellation in this window makes Cancel return success without finding the turn, after which the request proceeds normally. Consider making registration and publication atomic, or retaining a request-level cancel function until Engine admission completes, and add a regression test for this race.
There was a problem hiding this comment.
Fixed in a569f45. The Session turn index now stores the request cancel function before publishing the turn. The cancel endpoint cancels that context before exact Engine cancellation, so a cancel in the pre-registration window prevents Run from being admitted or dispatched. A deterministic regression test verifies the endpoint returns 204 and the Engine records zero calls.
| return result | ||
| } | ||
| case event, ok := <-events: | ||
| if !ok { |
There was a problem hiding this comment.
Once events is closed, this receive case remains permanently ready and the loop spins until PromptTurn returns or the context ends. If the subscription closes while the prompt is still blocked, one turn can consume a CPU core for the entire timeout. Set events = nil after observing closure (or return an explicit runtime failure) so the select blocks on the remaining signals.
There was a problem hiding this comment.
Fixed in a569f45. When the Runtime event subscription closes, the adapter now sets the channel to nil, so select waits on PromptTurn or context instead of spinning. A regression test uses a closed event stream and blocked prompt to verify Run remains pending and completes normally after the prompt returns.
6976bf8 to
a569f45
Compare
| } | ||
| currentRuntime := previous.RuntimeConfig() | ||
| desiredRuntime := createAgentSpec(spec).RuntimeConfig() | ||
| replacesRuntime := currentRuntime != desiredRuntime || previous.Role != string(spec.Role) |
There was a problem hiding this comment.
Role changes are routed through Service.Create(... Replace: true), but that service treats any replacement involving a manager as an operation on the singleton manager. Updating a worker to manager can therefore return/update the manager while leaving the requested worker unchanged; updating the manager to worker also remains on the manager path. Since Update promises to replace the complete AgentSpec, please either implement role transitions for the addressed resource or reject role changes explicitly, and cover both directions in the shared contract tests.
There was a problem hiding this comment.
Fixed in 7363282. Agent Engine Update now rejects role changes before any Skill or Runtime mutation, and MemoryClient exposes the same behavior. The shared contract covers both worker-to-manager and manager-to-worker attempts.
| if c.agentID == "" || request.ID == "" || request.ConversationKey == "" || len(request.Input) == 0 { | ||
| return failed(agentengine.ErrorInvalidRequest, "agent ID, turn ID, conversation key, and input are required") | ||
| } | ||
| for _, part := range request.Input { |
There was a problem hiding this comment.
The MemoryClient accepts requests that the real Engine rejects: file inputs are not checked for ID/path/name/media type/size/SHA-256, and unsupported continuation or interaction policies are not rejected. An adapter can therefore pass the mock-backed contract suite but fail after switching to the real Engine. Please share the normalization/validation logic or add equivalent checks and contract cases so both implementations expose the same request boundary.
There was a problem hiding this comment.
Fixed in 7363282. MemoryClient now normalizes request identity and policies, rejects unsupported continuation and interaction values, and validates the complete Runtime-neutral file shape. The same shared contract cases now run against MemoryClient and the real Engine.
| return Agent{}, err | ||
| } | ||
| } | ||
| if err := f.service.ReplaceSkills(ctx, created.ID, spec.Skills); err != nil { |
There was a problem hiding this comment.
A manager Create is backed by EnsureManager, so created may be the pre-existing singleton manager rather than a resource created by this call. If skill validation/staging then fails (for example, an unknown skill name), this cleanup deletes that existing manager, its runtime, and its home directory. Please validate/stage skills before creating anything, or track whether this call actually created the resource and never delete a pre-existing manager during rollback; add a regression test starting with an existing manager.
There was a problem hiding this comment.
Fixed in 7363282. Manager Create records whether the singleton existed before the call and only performs destructive rollback for a newly created resource. A regression test starts with an existing manager, forces Skill staging failure, and verifies the manager and Runtime are preserved.
| if err != nil { | ||
| return nil, cleanup, &TurnError{Code: ErrorFileUnavailable, Message: err.Error()} | ||
| } | ||
| root := filepath.Join(workspace, ".csgclaw", "engine-inputs") |
There was a problem hiding this comment.
The destination is inside the Runtime-controlled workspace, but these host-side path operations follow symlinked parent components. A Runtime can replace .csgclaw or engine-inputs with a symlink and make the host copy an authorized input outside this workspace; racing the deferred RemoveAll can also redirect cleanup outside the intended root. Please anchor creation, writes, and cleanup with os.Root (as the credential provisioning code does) or otherwise reject symlink components, and cover a malicious destination-symlink case.
There was a problem hiding this comment.
Fixed in 7363282. Runtime-local input staging is now anchored to the selected workspace with os.Root; directory creation, file writes, rename, and cleanup all use root-relative operations. A malicious .csgclaw destination symlink test verifies no file is created outside the workspace.
| return result | ||
| } | ||
| case event, ok := <-events: | ||
| if !ok { |
There was a problem hiding this comment.
Disabling the closed channel fixes the spin, but the turn is still reported as successful once PromptTurn returns even though all remaining output events were lost. Because TurnResult.Output is built only from this stream, a subscriber failure can become a successful empty response. Treat an unexpected event-stream closure before PromptCompleted as a Runtime failure (while still waiting for Runtime cleanup), and update the new closure test to assert that terminal result.
There was a problem hiding this comment.
Fixed in 7363282. Premature event-stream closure now disables the channel, waits for PromptTurn cleanup, and returns a dispatched runtime_failed result instead of successful empty output. The closure regression test asserts the terminal failure.
| <-promptDone | ||
| promptReturned = true | ||
| } | ||
| result.Dispatched = dispatched |
There was a problem hiding this comment.
dispatched is updated only when the select consumes accepted (or promptDone), so an accepted turn can take an event-failure or context-cancellation branch first and reach stopPrompt with this stale value still false. That violates the documented rule that every outcome after Runtime submission retains Dispatched=true, and can make a caller retry a turn that already ran. Reload dispatchedState in stopPrompt after waiting for prompt cleanup, and add a test where OnAccepted fires before a failing event/cancellation but the accepted case is not consumed first.
There was a problem hiding this comment.
Fixed in 0d05fc4. Terminal cleanup now reloads the atomic acceptance state after waiting for PromptTurn cleanup instead of using the potentially stale local dispatched value. A 100-iteration accepted-then-failed-event regression verifies every post-submission failure retains Dispatched=true.
0d05fc4 to
0fb19c2
Compare
|
|
admission 目前固定 reject;如果飞书需要排队、supersede,建议在 TurnRequest 加显式 AdmissionPolicy,而不是让每个渠道各自实现执行队列。 |
|
Looks good to merge as the Phase 2 baseline. We can refine the remaining details as we integrate the Channel and Runtime adapters. |
0fb19c2 to
623348a
Compare
|
感谢反馈,当前处理如下:
第二个单独的评论: 已在 TurnRequest 增加 AdmissionPolicy,并实现 |
Summary