Repository navigation
feat(harness): migrate runtime suspension to DATA snapshots - #3165
supreme-gg-gg wants to merge 12 commits into
Conversation
This comment has been minimized.
This comment has been minimized.
a920ed4 to
3666151
Compare
|
Simple demo with tracing Screen.Recording.2026-10-12.at.5.40.35.PM.mov |
8e02d7a to
f946b63
Compare
f946b63 to
0f54e08
Compare
0f54e08 to
d6293e6
Compare
| if err := json.Unmarshal(message.Params, ¶ms); err != nil { | ||
| return nil, false, fmt.Errorf("decode Codex MCP approval request: %w", err) | ||
| } | ||
| if params.ThreadID != translator.threadID || params.TurnID != nil && *params.TurnID != translator.turnID { |
There was a problem hiding this comment.
Can we keep handling requests from known spawned threads here? Shared-agent callbacks use the child's thread and turn, so this check rejects even a single protected MCP approval and consume fails the parent A2A task. approvalTool already matches these calls by thread.
If durable child continuations are out of scope, can we explicitly decline that callback instead of aborting the parent, and document the restriction?
There was a problem hiding this comment.
I think child continuations are out of scope and I'll decline them here. I believe that our compiler doesn't even allow tools in subagents right now. I think supporting them would be somewhat difficult since there's no client-executed tool flow for subagents (might only be doable with the native approval flow, which means we need FULL snapshot...), but either way, it should definitely be out of scope for this PR.
| } | ||
| if r.Phase == phaseResult { | ||
| if decision, ok := response.(*runtime.ApprovalDecision); ok && !reflect.DeepEqual(r.Decision, decision) { | ||
| return runtime.Outcome{}, fmt.Errorf("Codex approval decision changed after execution") |
There was a problem hiding this comment.
This returns before the abandonment defer is installed. If a tool result is cached, native startup fails before dispatch, and the user changes their decision on the republished request, the executor marks the task FAILED but the journal stays in result. ReservedTaskID() then restores that failed task as pending and blocks new tasks.
There's a similar path in Claude's response handling: an empty answer selection passes public validation but fails journal validation before cleanup is installed, leaving the failed task in waiting.
Can we reject these responses before starting the task, or keep it input-required? If we return a terminal failure, the journal needs to be marked abandoned on these paths too.
| } else { | ||
| err = w.Pause(runtimeCtx, work.Session) | ||
| } | ||
| snapshot, err := w.Quiesce(runtimeCtx, work.Session) |
There was a problem hiding this comment.
Can we add E2E coverage that waits for a verified DATA snapshot and suspended Actor before answering pending input? The approval helper replies immediately after INPUT_REQUIRED, so it can supersede unclaimed quiescence and pass without replacing the harness. The ask-user suspension test also skips Claude and Codex.
Approval, denial, cancellation, and native ask-user responses should exercise that restore boundary.
There was a problem hiding this comment.
Extended E2E coverage
| scopes again. A Data snapshot resumes by starting fresh containers from the OCI | ||
| image with the saved durable directories; Substrate still restores one as Data when | ||
| the Actor's template changed after the snapshot was taken. | ||
| Agent and standalone sandbox templates use DATA. Resume starts fresh containers |
There was a problem hiding this comment.
Small docs correction: can we also update the automatic-quiescence section and its sequence diagram? They still say waiting tasks pause on-node while only terminal tasks suspend externally, which contradicts the DATA behavior described here.
| b.mu.Lock() | ||
| defer b.mu.Unlock() | ||
| r, err := loadDeferred(b.directory, int(b.maxBody)) | ||
| if err == nil && r != nil && r.Phase == "executing" && input != nil && b.protects(input.ToolName) && input.ToolUseID == r.Tool.ID && input.ToolName == r.Tool.Name { |
There was a problem hiding this comment.
Can we handle these errors explicitly and log the underlying cause? A journal read or write failure currently falls through to the same denial as an unmatched request, including the suggestion to retry as a single tool call. handleHook also discards the error from resolveHook.
Early error branches would make this easier to follow and distinguish persistence failures from unsupported calls. We should keep the successful HTTP denial response that Claude's hook protocol requires.
| return saveDeferred(b.directory, d.config.MaxEventBytes, r) | ||
| } | ||
|
|
||
| func (p *deferredTurn) Resume(ctx context.Context, response runtime.InputResponse, sink runtime.EventSink) (outcome runtime.Outcome, returnErr error) { |
There was a problem hiding this comment.
Optional cleanup: can we extract the opening lock/load/check/save block into a named transition helper that owns its lock? Resume and Cancel currently mix journal transitions, native execution, and deferred cleanup, which makes it hard to follow which errors leave the input retryable and which abandon it.
A scoped lock would allow early returns without the repeated err == nil checks, while keeping process execution outside the lock. Private phase constants, like the Codex driver uses, would also make the transitions easier to trace.
| } | ||
|
|
||
| func yieldCancellation(ctx context.Context, reqCtx *a2asrv.ExecutorContext, done <-chan struct{}, yield func(a2atype.Event, error) bool) { | ||
| func yieldCancellation(ctx context.Context, reqCtx *a2asrv.ExecutorContext, done <-chan struct{}, knownErr error, yield func(a2atype.Event, error) bool) { |
There was a problem hiding this comment.
Small cleanup: do we still need done in this helper? All three callers already know it's closed: the active/canceling paths have received from it, and the parked path closes it before calling here.
We can keep that synchronization at the callers, where it needs to happen before reading cancelErr/err, and have this helper emit the resolved result. That removes the second wait while keeping request-context cancellation handling explicit.
| directory := t.TempDir() | ||
| executable := filepath.Join(directory, "codex") | ||
| capture := filepath.Join(directory, "requests") | ||
| script := `#!/bin/sh |
There was a problem hiding this comment.
Optional test cleanup: can we share this fake App Server setup with TestDeferredTurnExitsBeforePublishingPendingAndReusesSavedResult? Most of the handshake, approval, interruption, and continuation script is duplicated, so protocol changes need updating in both places and the actual test scenarios are harder to see.
A shared fixture could keep the assertions here. We'd still want to preserve the different interrupt/completion orderings and the cached-result test's graceful-exit check.
Verify with pinned DSH that session resume records an interrupted tool outcome without restoring its pending approval or running the command. Document the difference between this safety regression and durable approval continuation, and clarify adoption conditions for kagent kagent-dev#3010, kagent-dev#3011 and kagent-dev#3165. AI assistance: implementation and validation assisted by OpenAI Codex. Signed-off-by: Junfeng Yu <64075563+Jacob953@users.noreply.github.com>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
d6293e6 to
b74541f
Compare
This comment has been minimized.
This comment has been minimized.
Signed-off-by: Jet Chiang <pokyuen.jetchiang-ext@solo.io>
Move runtime suspension from memory snapshots to DATA snapshots for HITL events like approvals and questions.
Motivations:
Limitations:
unfinished; cross-boundary subagent nesting is not restored. Invocation links preserve task continuity. Deferred span durations include harness setup overhead.