diff --git a/README.md b/README.md index 8b2d0efc86..e7af773f6d 100644 --- a/README.md +++ b/README.md @@ -70,8 +70,8 @@ Connect Kandev to the tools your team already uses — pull issues into the kanb | Agent | Launch | |:-------:|:----------:| -| **Claude Code** | `npx -y @agentclientprotocol/claude-agent-acp` | -| **Codex** | `npx -y @agentclientprotocol/codex-acp` | +| **Claude Code** | `npx -y @agentclientprotocol/claude-agent-acp@0.61.0` | +| **Codex** | `npx -y @agentclientprotocol/codex-acp@1.1.5` | | **GitHub Copilot** | `npx -y @github/copilot --acp` | | **Gemini CLI** | `npx -y @google/gemini-cli --acp` | | **Amp** | `npx -y amp-acp` | diff --git a/apps/backend/cmd/mock-agent/main.go b/apps/backend/cmd/mock-agent/main.go index 69d3860953..69c6f4fe7c 100644 --- a/apps/backend/cmd/mock-agent/main.go +++ b/apps/backend/cmd/mock-agent/main.go @@ -40,6 +40,8 @@ type mockAgent struct { mu sync.Mutex } +var _ acp.Agent = (*mockAgent)(nil) + func main() { model := parseModelFlag() @@ -325,6 +327,14 @@ func (a *mockAgent) CloseSession(_ context.Context, req acp.CloseSessionRequest) return acp.CloseSessionResponse{}, nil } +// DeleteSession is not supported by the mock agent. +func (a *mockAgent) DeleteSession( + _ context.Context, + _ acp.DeleteSessionRequest, +) (acp.DeleteSessionResponse, error) { + return acp.DeleteSessionResponse{}, acp.NewMethodNotFound(acp.AgentMethodSessionDelete) +} + // ListSessions is not supported by the mock and returns an empty list. func (a *mockAgent) ListSessions(_ context.Context, _ acp.ListSessionsRequest) (acp.ListSessionsResponse, error) { return acp.ListSessionsResponse{}, nil diff --git a/apps/backend/go.mod b/apps/backend/go.mod index 0cc43cd7c6..9ecf327016 100644 --- a/apps/backend/go.mod +++ b/apps/backend/go.mod @@ -119,4 +119,4 @@ require ( gopkg.in/ini.v1 v1.67.0 // indirect ) -replace github.com/coder/acp-go-sdk => github.com/kdlbs/acp-go-sdk v0.13.6-0.20260625174839-e37105a28386 +replace github.com/coder/acp-go-sdk => github.com/kdlbs/acp-go-sdk v0.13.6-0.20260722160645-1ce4653527f6 diff --git a/apps/backend/go.sum b/apps/backend/go.sum index f364fe03f6..aec989bf53 100644 --- a/apps/backend/go.sum +++ b/apps/backend/go.sum @@ -223,8 +223,8 @@ github.com/json-iterator/go v1.1.12 h1:PV8peI4a0ysnczrg+LtxykD8LfKY9ML6u2jnxaEnr github.com/json-iterator/go v1.1.12/go.mod h1:e30LSqwooZae/UwlEbR2852Gd8hjQvJoHmT4TnhNGBo= github.com/jstemmer/go-junit-report v0.0.0-20190106144839-af01ea7f8024/go.mod h1:6v2b51hI/fHJwM22ozAgKL4VKDeJcHhJFhtBdhmNjmU= github.com/jstemmer/go-junit-report v0.9.1/go.mod h1:Brl9GWCQeLvo8nXZwPNNblvFj/XSXhF0NWZEnDohbsk= -github.com/kdlbs/acp-go-sdk v0.13.6-0.20260625174839-e37105a28386 h1:zUFkN1/+o3vEYqrkshpykUBAstk6NqiitMuh+dR+Tik= -github.com/kdlbs/acp-go-sdk v0.13.6-0.20260625174839-e37105a28386/go.mod h1:yKzM/3R9uELp4+nBAwwtkS0aN1FOFjo11CNPy37yFko= +github.com/kdlbs/acp-go-sdk v0.13.6-0.20260722160645-1ce4653527f6 h1:PIUaLxC1PZ4Y1ESMzucH2ThVq4yOAH8x8VfCKRSX5pc= +github.com/kdlbs/acp-go-sdk v0.13.6-0.20260722160645-1ce4653527f6/go.mod h1:yKzM/3R9uELp4+nBAwwtkS0aN1FOFjo11CNPy37yFko= github.com/kevinburke/ssh_config v1.6.0 h1:J1FBfmuVosPHf5GRdltRLhPJtJpTlMdKTBjRgTaQBFY= github.com/kevinburke/ssh_config v1.6.0/go.mod h1:q2RIzfka+BXARoNexmF9gkxEX7DmvbW9P4hIVx2Kg4M= github.com/kisielk/gotool v1.0.0/go.mod h1:XhKaO+MFFWcvkIS/tQcRk01m1F5IRFswLeQ+oQHNcck= diff --git a/apps/backend/internal/agent/agents/ACP_BRIDGE_VERSIONS.md b/apps/backend/internal/agent/agents/ACP_BRIDGE_VERSIONS.md new file mode 100644 index 0000000000..43a0b85fbc --- /dev/null +++ b/apps/backend/internal/agent/agents/ACP_BRIDGE_VERSIONS.md @@ -0,0 +1,30 @@ +# ACP bridge versions + +Kandev tests these npm-provided ACP runtimes with the following package pins: + +| Agent | Package | Tested version | ACP runtime selection | +| --- | --- | --- | --- | +| Claude | `@agentclientprotocol/claude-agent-acp` | `0.61.0` | Exact package spec through `npx` | +| Codex | `@agentclientprotocol/codex-acp` | `1.1.5` | Exact package spec through `npx` | +| OpenCode | `opencode-ai` | `1.18.4` | Installed `opencode` binary | + +Claude and Codex use their exact package specs for normal sessions, container +commands, one-shot inference, and remote installation. The resolved spec is +part of `AgentCommand`, which existing lifecycle diagnostics log as +`agent_command`. + +OpenCode intentionally uses the direct `opencode acp` command for discovery, +normal sessions, container commands, and one-shot inference. This keeps startup +offline-compatible and ensures discovery validates the executable actually +launched. Its installer remains pinned to `opencode-ai@1.18.4`, and discovery +runs that executable with `--version` before reporting it as supported. Missing, +malformed, failing, or mismatched version output reports the agent as unavailable +with the pinned install command as remediation. The ACP initialize response +separately records the runtime-reported agent name and version. + +To update a bridge, change one version constant at a time, confirm the exact +version exists in the configured npm registry, capture only sanitized ACP wire +fixtures, and run the agent command-surface and ACP dialect tests before +changing the documented tested version. For OpenCode, also install the candidate +globally and confirm `opencode --version` before capturing fixtures. Do not add +prompts, file contents, credentials, or other user data to protocol fixtures. diff --git a/apps/backend/internal/agent/agents/claude_acp.go b/apps/backend/internal/agent/agents/claude_acp.go index 82735eca08..254bf1f42a 100644 --- a/apps/backend/internal/agent/agents/claude_acp.go +++ b/apps/backend/internal/agent/agents/claude_acp.go @@ -16,7 +16,11 @@ var claudeACPLogoLight []byte //go:embed logos/claude_code_dark.svg var claudeACPLogoDark []byte -const claudeACPPkg = "@agentclientprotocol/claude-agent-acp" +const ( + claudeACPPackage = "@agentclientprotocol/claude-agent-acp" + claudeACPVersion = "0.61.0" + claudeACPPackageSpec = claudeACPPackage + "@" + claudeACPVersion +) // claudeACPPermSettings maps the profile DangerouslySkipPermissions toggle to // the Claude Code CLI's --dangerously-skip-permissions flag in passthrough mode. @@ -106,7 +110,7 @@ func (a *ClaudeACP) IsInstalled(ctx context.Context) (*DiscoveryResult, error) { } func (a *ClaudeACP) BuildCommand(opts CommandOptions) Command { - return Cmd("npx", "-y", claudeACPPkg).Build() + return Cmd("npx", "-y", claudeACPPackageSpec).Build() } func (a *ClaudeACP) Runtime() *RuntimeConfig { @@ -114,7 +118,7 @@ func (a *ClaudeACP) Runtime() *RuntimeConfig { return &RuntimeConfig{ Image: "kandev/multi-agent", Tag: "latest", - Cmd: Cmd("npx", "-y", claudeACPPkg).Build(), + Cmd: Cmd("npx", "-y", claudeACPPackageSpec).Build(), WorkingDir: "{workspace}", RequiredEnv: []string{}, // Auth via ANTHROPIC_API_KEY or OAuth credentials file (see RemoteAuth) Env: map[string]string{ @@ -169,7 +173,7 @@ func (a *ClaudeACP) LoginCommand() *LoginCommand { func (a *ClaudeACP) InstallScript() string { // Install both the user-facing Anthropic CLI (which IsInstalled probes for // and which `claude /login` runs against) and the ACP bridge package. - return "npm install -g @anthropic-ai/claude-code " + claudeACPPkg + return "npm install -g @anthropic-ai/claude-code " + claudeACPPackageSpec } func (a *ClaudeACP) BillingType() usage.BillingType { return claudeBillingType() } @@ -182,6 +186,6 @@ func (a *ClaudeACP) PermissionSettings() map[string]PermissionSetting { func (a *ClaudeACP) InferenceConfig() *InferenceConfig { return &InferenceConfig{ Supported: true, - Command: NewCommand("npx", "-y", claudeACPPkg), + Command: NewCommand("npx", "-y", claudeACPPackageSpec), } } diff --git a/apps/backend/internal/agent/agents/claude_acp_test.go b/apps/backend/internal/agent/agents/claude_acp_test.go index a2b16f2978..bf105f56bf 100644 --- a/apps/backend/internal/agent/agents/claude_acp_test.go +++ b/apps/backend/internal/agent/agents/claude_acp_test.go @@ -1,26 +1,27 @@ package agents import ( - "reflect" + "slices" "strings" "testing" ) -func TestClaudeACPCommandsUseUnversionedBridgePackage(t *testing.T) { - want := []string{"npx", "-y", "@agentclientprotocol/claude-agent-acp"} - agent := NewClaudeACP() +func TestClaudeACPUsesPinnedBridge(t *testing.T) { + a := NewClaudeACP() + want := []string{"npx", "-y", "@agentclientprotocol/claude-agent-acp@0.61.0"} - if got := agent.BuildCommand(CommandOptions{}).Args(); !reflect.DeepEqual(got, want) { - t.Errorf("BuildCommand() = %q, want %q", got, want) + if got := a.BuildCommand(CommandOptions{}).Args(); !slices.Equal(got, want) { + t.Fatalf("BuildCommand = %#v, want %#v", got, want) } - if got := agent.Runtime().Cmd.Args(); !reflect.DeepEqual(got, want) { - t.Errorf("Runtime().Cmd = %q, want %q", got, want) + if got := a.Runtime().Cmd.Args(); !slices.Equal(got, want) { + t.Fatalf("Runtime Cmd = %#v, want %#v", got, want) } - if got := agent.InferenceConfig().Command.Args(); !reflect.DeepEqual(got, want) { - t.Errorf("InferenceConfig().Command = %q, want %q", got, want) + if got := a.InferenceConfig().Command.Args(); !slices.Equal(got, want) { + t.Fatalf("Inference Command = %#v, want %#v", got, want) } - if got := agent.InstallScript(); got != "npm install -g @anthropic-ai/claude-code @agentclientprotocol/claude-agent-acp" { - t.Errorf("InstallScript() = %q, want unversioned Claude ACP package", got) + wantInstall := "npm install -g @anthropic-ai/claude-code @agentclientprotocol/claude-agent-acp@0.61.0" + if got := a.InstallScript(); got != wantInstall { + t.Fatalf("InstallScript = %q, want %q", got, wantInstall) } } diff --git a/apps/backend/internal/agent/agents/codex_acp.go b/apps/backend/internal/agent/agents/codex_acp.go index 25311befc1..5952a10d6d 100644 --- a/apps/backend/internal/agent/agents/codex_acp.go +++ b/apps/backend/internal/agent/agents/codex_acp.go @@ -16,7 +16,11 @@ var codexACPLogoLight []byte //go:embed logos/codex_dark.svg var codexACPLogoDark []byte -const codexACPPkg = "@agentclientprotocol/codex-acp" +const ( + codexACPPackage = "@agentclientprotocol/codex-acp" + codexACPVersion = "1.1.5" + codexACPPackageSpec = codexACPPackage + "@" + codexACPVersion +) var ( _ Agent = (*CodexACP)(nil) @@ -95,7 +99,7 @@ func (a *CodexACP) IsInstalled(ctx context.Context) (*DiscoveryResult, error) { } func (a *CodexACP) BuildCommand(opts CommandOptions) Command { - return Cmd("npx", "-y", codexACPPkg).Build() + return Cmd("npx", "-y", codexACPPackageSpec).Build() } func (a *CodexACP) Runtime() *RuntimeConfig { @@ -103,7 +107,7 @@ func (a *CodexACP) Runtime() *RuntimeConfig { return &RuntimeConfig{ Image: "kandev/multi-agent", Tag: "latest", - Cmd: Cmd("npx", "-y", codexACPPkg).Build(), + Cmd: Cmd("npx", "-y", codexACPPackageSpec).Build(), WorkingDir: "{workspace}", RequiredEnv: []string{"OPENAI_API_KEY"}, Env: map[string]string{}, @@ -162,7 +166,7 @@ func (a *CodexACP) LoginCommand() *LoginCommand { // Install both the user-facing OpenAI codex CLI (which `codex login` runs // against) and the ACP bridge package used for chat sessions. func (a *CodexACP) InstallScript() string { - return "npm install -g @openai/codex " + codexACPPkg + return "npm install -g @openai/codex " + codexACPPackageSpec } func (a *CodexACP) BillingType() usage.BillingType { return codexBillingType() } @@ -175,6 +179,6 @@ func (a *CodexACP) PermissionSettings() map[string]PermissionSetting { func (a *CodexACP) InferenceConfig() *InferenceConfig { return &InferenceConfig{ Supported: true, - Command: NewCommand("npx", "-y", codexACPPkg), + Command: NewCommand("npx", "-y", codexACPPackageSpec), } } diff --git a/apps/backend/internal/agent/agents/codex_acp_test.go b/apps/backend/internal/agent/agents/codex_acp_test.go index c9703ac666..5801c24f25 100644 --- a/apps/backend/internal/agent/agents/codex_acp_test.go +++ b/apps/backend/internal/agent/agents/codex_acp_test.go @@ -25,7 +25,7 @@ func TestCodexACP_PermissionSettings_NoBridgeCLIFlags(t *testing.T) { } func TestCodexACP_BuildCommand_NoCodexCLIFlags(t *testing.T) { - want := []string{"npx", "-y", codexACPPkg} + want := []string{"npx", "-y", codexACPPackageSpec} cmd := NewCodexACP().BuildCommand(CommandOptions{ PermissionValues: map[string]bool{PermissionKeyAutoApprove: true}, }) @@ -36,7 +36,7 @@ func TestCodexACP_BuildCommand_NoCodexCLIFlags(t *testing.T) { func TestCodexACP_UsesAgentClientProtocolBridge(t *testing.T) { a := NewCodexACP() - want := []string{"npx", "-y", "@agentclientprotocol/codex-acp"} + want := []string{"npx", "-y", "@agentclientprotocol/codex-acp@1.1.5"} if got := a.BuildCommand(CommandOptions{}).Args(); !slices.Equal(got, want) { t.Fatalf("BuildCommand = %#v, want %#v", got, want) @@ -47,8 +47,9 @@ func TestCodexACP_UsesAgentClientProtocolBridge(t *testing.T) { if got := a.InferenceConfig().Command.Args(); !slices.Equal(got, want) { t.Fatalf("Inference Command = %#v, want %#v", got, want) } - if got := a.InstallScript(); !strings.Contains(got, "@agentclientprotocol/codex-acp") { - t.Fatalf("InstallScript = %q, want agentclientprotocol bridge package", got) + wantInstall := "npm install -g @openai/codex @agentclientprotocol/codex-acp@1.1.5" + if got := a.InstallScript(); got != wantInstall { + t.Fatalf("InstallScript = %q, want %q", got, wantInstall) } } diff --git a/apps/backend/internal/agent/agents/opencode_acp.go b/apps/backend/internal/agent/agents/opencode_acp.go index 71b1762550..d8ee5cf9a4 100644 --- a/apps/backend/internal/agent/agents/opencode_acp.go +++ b/apps/backend/internal/agent/agents/opencode_acp.go @@ -3,6 +3,10 @@ package agents import ( "context" _ "embed" + "fmt" + "os/exec" + "regexp" + "strings" "time" "github.com/kandev/kandev/internal/agent/mcpconfig" @@ -16,7 +20,16 @@ var opencodeACPLogoLight []byte //go:embed logos/opencode_dark.svg var opencodeACPLogoDark []byte -const opencodeACPPkg = "opencode-ai" +const ( + opencodeACPPackage = "opencode-ai" + opencodeACPVersion = "1.18.4" + opencodeACPPackageSpec = opencodeACPPackage + "@" + opencodeACPVersion + opencodeVersionTimeout = 5 * time.Second +) + +var opencodeVersionPattern = regexp.MustCompile( + `(?:^|[^0-9A-Za-z])[vV]?([0-9]+\.[0-9]+\.[0-9]+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?)(?:$|[^0-9A-Za-z.+-])`, +) var ( _ Agent = (*OpenCodeACP)(nil) @@ -73,9 +86,22 @@ func (a *OpenCodeACP) IsInstalled(ctx context.Context) (*DiscoveryResult, error) // Check for the opencode CLI on PATH. Auth state is surfaced later by // the ACP probe, not by scanning ~/.opencode. result, err := Detect(ctx, WithCommand("opencode")) - if err != nil { + if err != nil || !result.Available { return result, err } + version, err := probeOpenCodeVersion(ctx, result.MatchedPath) + if err != nil { + return unsupportedOpenCodeResult(result.MatchedPath), fmt.Errorf( + "cannot verify OpenCode %s at %q: %w; reinstall with `%s`", + opencodeACPVersion, result.MatchedPath, err, a.InstallScript(), + ) + } + if version != opencodeACPVersion { + return unsupportedOpenCodeResult(result.MatchedPath), fmt.Errorf( + "unsupported OpenCode version %s at %q; Kandev requires %s; reinstall with `%s`", + version, result.MatchedPath, opencodeACPVersion, a.InstallScript(), + ) + } result.SupportsMCP = true result.Capabilities = DiscoveryCapabilities{ SupportsSessionResume: true, @@ -83,6 +109,28 @@ func (a *OpenCodeACP) IsInstalled(ctx context.Context) (*DiscoveryResult, error) return result, nil } +func probeOpenCodeVersion(ctx context.Context, path string) (string, error) { + probeCtx, cancel := context.WithTimeout(ctx, opencodeVersionTimeout) + defer cancel() + + output, err := exec.CommandContext(probeCtx, path, "--version").CombinedOutput() + if err != nil { + if probeCtx.Err() != nil { + return "", probeCtx.Err() + } + return "", fmt.Errorf("run --version: %w", err) + } + match := opencodeVersionPattern.FindStringSubmatch(strings.TrimSpace(string(output))) + if len(match) != 2 { + return "", fmt.Errorf("parse --version output %q", strings.TrimSpace(string(output))) + } + return match[1], nil +} + +func unsupportedOpenCodeResult(path string) *DiscoveryResult { + return &DiscoveryResult{MatchedPath: path} +} + func (a *OpenCodeACP) BuildCommand(opts CommandOptions) Command { return Cmd("opencode", "acp").Build() } @@ -127,7 +175,7 @@ func (a *OpenCodeACP) RemoteAuth() *RemoteAuth { } func (a *OpenCodeACP) InstallScript() string { - return "npm install -g " + opencodeACPPkg + return "npm install -g " + opencodeACPPackageSpec } func (a *OpenCodeACP) BillingType() usage.BillingType { return defaultBillingType() } diff --git a/apps/backend/internal/agent/agents/opencode_acp_test.go b/apps/backend/internal/agent/agents/opencode_acp_test.go index 20174510f7..9713b8022f 100644 --- a/apps/backend/internal/agent/agents/opencode_acp_test.go +++ b/apps/backend/internal/agent/agents/opencode_acp_test.go @@ -1,10 +1,154 @@ package agents import ( + "context" + "errors" + "os" + "path/filepath" "slices" + "strings" "testing" ) +func TestOpenCodeACPUsesInstalledBinaryAndPinnedInstaller(t *testing.T) { + a := NewOpenCodeACP() + want := []string{"opencode", "acp"} + + if got := a.BuildCommand(CommandOptions{}).Args(); !slices.Equal(got, want) { + t.Fatalf("BuildCommand = %#v, want %#v", got, want) + } + if got := a.Runtime().Cmd.Args(); !slices.Equal(got, want) { + t.Fatalf("Runtime Cmd = %#v, want %#v", got, want) + } + if got := a.InferenceConfig().Command.Args(); !slices.Equal(got, want) { + t.Fatalf("Inference Command = %#v, want %#v", got, want) + } + if got, wantInstall := a.InstallScript(), "npm install -g opencode-ai@1.18.4"; got != wantInstall { + t.Fatalf("InstallScript = %q, want %q", got, wantInstall) + } +} + +func TestOpenCodeACPDiscoveryMatchesRuntimeExecutable(t *testing.T) { + binaryPath := writeOpenCodeTestBinary(t, "printf '1.18.4\\n'") + + a := NewOpenCodeACP() + result, err := a.IsInstalled(context.Background()) + if err != nil { + t.Fatalf("IsInstalled() error = %v", err) + } + if !result.Available { + t.Fatal("IsInstalled() Available = false, want true") + } + if result.MatchedPath != binaryPath { + t.Fatalf("IsInstalled() MatchedPath = %q, want %q", result.MatchedPath, binaryPath) + } + if got := a.BuildCommand(CommandOptions{}).Args()[0]; got != filepath.Base(binaryPath) { + t.Fatalf("runtime executable = %q, discovered executable = %q", got, filepath.Base(binaryPath)) + } +} + +func TestOpenCodeACPDiscoveryAcceptsSupportedVersionOutput(t *testing.T) { + tests := []struct { + name string + output string + }{ + {name: "exact", output: "1.18.4"}, + {name: "prefixed and whitespace", output: " OpenCode version v1.18.4 "}, + {name: "slash prefix", output: "opencode/1.18.4 linux-x64"}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + writeOpenCodeTestBinary(t, "printf '%s\\n' \"$OPEN_CODE_VERSION_OUTPUT\"") + t.Setenv("OPEN_CODE_VERSION_OUTPUT", tt.output) + + result, err := NewOpenCodeACP().IsInstalled(context.Background()) + if err != nil { + t.Fatalf("IsInstalled() error = %v", err) + } + if !result.Available { + t.Fatal("IsInstalled() Available = false, want true") + } + }) + } +} + +func TestOpenCodeACPDiscoveryRejectsUnsupportedVersion(t *testing.T) { + binaryPath := writeOpenCodeTestBinary(t, "printf '1.16.2\\n'") + + result, err := NewOpenCodeACP().IsInstalled(context.Background()) + if err == nil { + t.Fatal("IsInstalled() error = nil, want version mismatch") + } + if result.Available { + t.Fatal("IsInstalled() Available = true, want false") + } + if result.MatchedPath != binaryPath { + t.Fatalf("MatchedPath = %q, want %q", result.MatchedPath, binaryPath) + } + for _, want := range []string{"1.16.2", "requires 1.18.4", "npm install -g opencode-ai@1.18.4"} { + if !strings.Contains(err.Error(), want) { + t.Errorf("error %q does not contain %q", err, want) + } + } +} + +func TestOpenCodeACPDiscoveryRejectsMalformedVersion(t *testing.T) { + writeOpenCodeTestBinary(t, "printf 'OpenCode development build\\n'") + + result, err := NewOpenCodeACP().IsInstalled(context.Background()) + if err == nil { + t.Fatal("IsInstalled() error = nil, want parse failure") + } + if result.Available { + t.Fatal("IsInstalled() Available = true, want false") + } + if !strings.Contains(err.Error(), "npm install -g opencode-ai@1.18.4") { + t.Fatalf("error %q does not contain pinned remediation", err) + } +} + +func TestOpenCodeACPDiscoveryHandlesVersionCommandFailure(t *testing.T) { + writeOpenCodeTestBinary(t, "exit 7") + + result, err := NewOpenCodeACP().IsInstalled(context.Background()) + if err == nil { + t.Fatal("IsInstalled() error = nil, want command failure") + } + if result.Available { + t.Fatal("IsInstalled() Available = true, want false") + } + if !strings.Contains(err.Error(), "npm install -g opencode-ai@1.18.4") { + t.Fatalf("error %q does not contain pinned remediation", err) + } +} + +func TestOpenCodeACPDiscoveryHonorsCancellation(t *testing.T) { + writeOpenCodeTestBinary(t, "/bin/sleep 10") + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + result, err := NewOpenCodeACP().IsInstalled(ctx) + if !errors.Is(err, context.Canceled) { + t.Fatalf("IsInstalled() error = %v, want context.Canceled", err) + } + if result.Available { + t.Fatal("IsInstalled() Available = true, want false") + } +} + +func writeOpenCodeTestBinary(t *testing.T, body string) string { + t.Helper() + dir := t.TempDir() + binaryPath := filepath.Join(dir, "opencode") + contents := "#!/bin/sh\n" + body + "\n" + if err := os.WriteFile(binaryPath, []byte(contents), 0o755); err != nil { + t.Fatalf("write fake opencode: %v", err) + } + t.Setenv("PATH", dir) + return binaryPath +} + // TestOpenCodeACPRuntime_RequiresProcessKill is the regression test for GH // issue #1247: opencode acp keeps its HTTP server + MCP child tree alive // when stdin closes, so its RuntimeConfig must signal that the process diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_events.go b/apps/backend/internal/agent/runtime/lifecycle/manager_events.go index 14f8fb25a7..122d1e689f 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_events.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_events.go @@ -24,6 +24,16 @@ func (m *Manager) handleMessageChunkEvent(execution *AgentExecution, event agent if event.Role == "user" || event.Text == "" { return } + // ACP message chunks do not carry the lifecycle prompt generation. The + // messageMu acquisition is therefore the only reliable turn boundary: a + // chunk observed before an atomic reset is detached with the old turn, + // while one observed after reset is retained for the replacement turn. + m.appendAssistantHistoryChunk(execution, event.Text) + if event.ProtocolMessageID != "" { + m.flushPendingLegacyMessage(execution) + m.publishProtocolMessage(execution, event.ProtocolMessageID, event.Text) + return + } execution.messageMu.Lock() execution.messageBuffer.WriteString(event.Text) bufferLenAfterWrite := execution.messageBuffer.Len() @@ -55,6 +65,11 @@ func (m *Manager) handleReasoningEvent(execution *AgentExecution, event agentctl if event.ReasoningText == "" { return } + if event.ProtocolMessageID != "" { + m.flushPendingLegacyThinking(execution) + m.publishProtocolThinking(execution, event.ProtocolMessageID, event.ReasoningText) + return + } execution.messageMu.Lock() execution.thinkingBuffer.WriteString(event.ReasoningText) @@ -155,9 +170,10 @@ func handleCompleteEventSignal(execution *AgentExecution, event *agentctl.AgentE } select { case execution.promptDoneCh <- PromptCompletionSignal{ - StopReason: stopReason, - IsError: isError, - Error: errorMsg, + StopReason: stopReason, + IsError: isError, + Error: errorMsg, + PromptGeneration: event.PromptGeneration, }: default: // Channel full or no one waiting — that's fine (e.g., initial prompt in goroutine) @@ -296,14 +312,13 @@ func (m *Manager) handleCompleteEvent(execution *AgentExecution, event *agentctl // Flush the message buffer to publish any remaining content as a streaming message. flushedText := m.flushMessageBuffer(execution) + execution.messageMu.Lock() + execution.clearProtocolMessageCorrelationLocked() + execution.messageMu.Unlock() if flushedText != "" { event.Text = flushedText - if m.historyManager != nil && execution.historyEnabled && execution.SessionID != "" { - if err := m.historyManager.AppendAgentMessage(execution.SessionID, flushedText); err != nil { - m.logger.Warn("failed to store final agent message to history", zap.Error(err)) - } - } } + m.flushAssistantHistory(execution) m.logger.Info("complete event processed", zap.String("execution_id", execution.ID), @@ -334,6 +349,7 @@ func (m *Manager) handleToolCallEvent(execution *AgentExecution, event agentctl. // the streaming path itself and always returns "". m.flushMessageBuffer(execution) } + m.flushAssistantHistory(execution) if m.historyManager != nil && execution.historyEnabled && execution.SessionID != "" { if err := m.historyManager.AppendToolCall(execution.SessionID, event); err != nil { m.logger.Warn("failed to store tool call to history", zap.Error(err)) @@ -349,6 +365,7 @@ func (m *Manager) handleToolCallEvent(execution *AgentExecution, event agentctl. // handleToolUpdateEvent stores completed tool results in session history. func (m *Manager) handleToolUpdateEvent(execution *AgentExecution, event agentctl.AgentEvent) { if m.historyManager != nil && execution.historyEnabled && execution.SessionID != "" && event.ToolStatus == toolStatusComplete { + m.flushAssistantHistory(execution) if err := m.historyManager.AppendToolResult(execution.SessionID, event); err != nil { m.logger.Warn("failed to store tool result to history", zap.Error(err)) } @@ -489,18 +506,66 @@ func (m *Manager) recordActivity(execution *AgentExecution, event agentctl.Agent // handleStreamDisconnect handles unexpected updates stream disconnections. // It proactively updates execution status and publishes an error event so the // orchestrator can transition the session state without waiting for a future prompt. -func (m *Manager) handleStreamDisconnect(execution *AgentExecution, err error) { +func (m *Manager) handleStreamDisconnect( + execution *AgentExecution, + err error, + promptGeneration uint64, +) { m.logger.Warn("agent updates stream disconnected", zap.String("execution_id", execution.ID), zap.String("session_id", execution.SessionID), + zap.Uint64("prompt_generation", promptGeneration), zap.Error(err)) + if promptGeneration != 0 { + execution.promptLifecycleMu.Lock() + defer execution.promptLifecycleMu.Unlock() + + var claimed bool + var updated *AgentExecution + statusErr := m.executionStore.WithLock(execution.ID, func(current *AgentExecution) { + if current != execution || current.promptGeneration != promptGeneration { + return + } + current.Status = v1.AgentStatusFailed + updated = current + claimed = true + }) + if statusErr != nil { + m.logger.Warn("failed to persist stream disconnect failed status", + zap.String("execution_id", execution.ID), + zap.Error(statusErr)) + return + } + if !claimed { + m.logger.Debug("ignoring stream disconnect for superseded prompt generation", + zap.String("execution_id", execution.ID), + zap.Uint64("disconnect_prompt_generation", promptGeneration)) + return + } + + m.flushAssistantHistory(execution) + m.persistExecutorRunning(context.Background(), updated) + m.publishStreamDisconnectError(execution, err) + return + } + + // The stream callback may race with the caller starting another prompt + // after promptDoneCh is signaled. Drain the partial assistant transcript + // here as well as at prompt setup; the shared buffer lock makes either + // path the single owner and prevents a later reset from dropping it. + m.flushAssistantHistory(execution) + if err := m.UpdateStatus(execution.ID, v1.AgentStatusFailed); err != nil { m.logger.Warn("failed to persist stream disconnect failed status", zap.String("execution_id", execution.ID), zap.Error(err)) } + m.publishStreamDisconnectError(execution, err) +} + +func (m *Manager) publishStreamDisconnectError(execution *AgentExecution, err error) { m.eventPublisher.PublishAgentctlEvent( context.Background(), events.AgentctlError, execution, "agent stream disconnected: "+err.Error(), diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_events_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_events_test.go index 9fc16dd300..7445134976 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_events_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_events_test.go @@ -2,6 +2,8 @@ package lifecycle import ( "context" + "errors" + "runtime" "sync" "testing" "time" @@ -116,6 +118,713 @@ func TestHandleAgentEvent_UserMessageChunkNotBufferedAsAssistant(t *testing.T) { } } +func TestHandleAgentEvent_ProtocolMessageResumesAcrossToolCall(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "message_chunk", + Text: "before tool", + ProtocolMessageID: "acp-message-1", + }) + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "tool_call", + ToolCallID: "tool-1", + ToolName: "read_file", + }) + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "message_chunk", + Text: "after tool", + ProtocolMessageID: "acp-message-1", + }) + + messageEvents := streamEventsOfType(eventBus, "message_streaming") + if len(messageEvents) != 2 { + t.Fatalf("message event count = %d, want 2", len(messageEvents)) + } + first, second := messageEvents[0].Data, messageEvents[1].Data + if first.MessageID == "" || second.MessageID != first.MessageID { + t.Fatalf("Kandev message IDs = (%q, %q), want same non-empty ID", first.MessageID, second.MessageID) + } + if first.IsAppend || !second.IsAppend { + t.Fatalf("append flags = (%t, %t), want (false, true)", first.IsAppend, second.IsAppend) + } +} + +func TestHandleAgentEvent_InterleavedProtocolMessagesRemainDistinct(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + for _, event := range []agentctl.AgentEvent{ + {Type: "message_chunk", Text: "a1", ProtocolMessageID: "message-a"}, + {Type: "message_chunk", Text: "b1", ProtocolMessageID: "message-b"}, + {Type: "message_chunk", Text: "a2", ProtocolMessageID: "message-a"}, + } { + mgr.handleAgentEvent(execution, event) + } + + messageEvents := streamEventsOfType(eventBus, "message_streaming") + if len(messageEvents) != 3 { + t.Fatalf("message event count = %d, want 3", len(messageEvents)) + } + first, second, third := messageEvents[0].Data, messageEvents[1].Data, messageEvents[2].Data + if first.Text != "a1" || second.Text != "b1" || third.Text != "a2" { + t.Fatalf("event order = (%q, %q, %q), want (a1, b1, a2)", first.Text, second.Text, third.Text) + } + if first.MessageID == second.MessageID || third.MessageID != first.MessageID { + t.Fatalf("Kandev message IDs = (%q, %q, %q), want A != B and A stable", + first.MessageID, second.MessageID, third.MessageID) + } + if first.IsAppend || second.IsAppend || !third.IsAppend { + t.Fatalf("append flags = (%t, %t, %t), want (false, false, true)", + first.IsAppend, second.IsAppend, third.IsAppend) + } +} + +func TestHandleAgentEvent_ProtocolThoughtAndMessageIDsUseSeparateNamespaces(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "message_chunk", + Text: "answer", + ProtocolMessageID: "shared-source-id", + }) + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "reasoning", + ReasoningText: "thought", + ProtocolMessageID: "shared-source-id", + }) + + messageEvents := streamEventsOfType(eventBus, "message_streaming") + thinkingEvents := streamEventsOfType(eventBus, "thinking_streaming") + if len(messageEvents) != 1 || len(thinkingEvents) != 1 { + t.Fatalf("event counts = message:%d thinking:%d, want 1 each", len(messageEvents), len(thinkingEvents)) + } + if messageEvents[0].Data.MessageID == thinkingEvents[0].Data.MessageID { + t.Fatalf("assistant and thought mapped to the same Kandev ID %q", messageEvents[0].Data.MessageID) + } +} + +func TestHandleAgentEvent_CompleteClearsProtocolMessageCorrelation(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "message_chunk", + Text: "turn one", + ProtocolMessageID: "reused-source-id", + }) + mgr.handleAgentEvent(execution, agentctl.AgentEvent{Type: "complete"}) + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "message_chunk", + Text: "turn two", + ProtocolMessageID: "reused-source-id", + }) + + messageEvents := streamEventsOfType(eventBus, "message_streaming") + if len(messageEvents) != 2 { + t.Fatalf("message event count = %d, want 2", len(messageEvents)) + } + if messageEvents[0].Data.MessageID == messageEvents[1].Data.MessageID { + t.Fatalf("protocol correlation leaked across completion: Kandev ID %q was reused", + messageEvents[0].Data.MessageID) + } + if messageEvents[1].Data.IsAppend { + t.Fatal("first chunk after completion was marked as append") + } +} + +func TestHandleAgentEvent_LegacyAssistantFlushesBeforeProtocolMessage(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + mgr.handleAgentEvent(execution, agentctl.AgentEvent{Type: "message_chunk", Text: "legacy first"}) + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "message_chunk", + Text: "protocol second", + ProtocolMessageID: "protocol-message", + }) + + events := streamEventsOfType(eventBus, "message_streaming") + if len(events) != 2 { + t.Fatalf("message event count = %d, want 2", len(events)) + } + if events[0].Data.Text != "legacy first" || events[1].Data.Text != "protocol second" { + t.Fatalf("message order = (%q, %q), want legacy then protocol", + events[0].Data.Text, events[1].Data.Text) + } + if events[0].Data.MessageID == events[1].Data.MessageID { + t.Fatalf("mixed legacy and protocol chunks shared Kandev ID %q", events[0].Data.MessageID) + } +} + +func TestHandleAgentEvent_ProtocolToLegacyTransitionCreatesBoundary(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + for _, event := range []agentctl.AgentEvent{ + {Type: "message_chunk", Text: "protocol first", ProtocolMessageID: "protocol-message"}, + {Type: "message_chunk", Text: "legacy second\n"}, + {Type: "message_chunk", Text: "protocol third", ProtocolMessageID: "protocol-message"}, + {Type: "message_chunk", Text: "legacy fourth\n"}, + } { + mgr.handleAgentEvent(execution, event) + } + + events := streamEventsOfType(eventBus, "message_streaming") + if len(events) != 4 { + t.Fatalf("message event count = %d, want 4", len(events)) + } + texts := []string{events[0].Data.Text, events[1].Data.Text, events[2].Data.Text, events[3].Data.Text} + wantTexts := []string{"protocol first", "legacy second\n", "protocol third", "legacy fourth\n"} + for i := range wantTexts { + if texts[i] != wantTexts[i] { + t.Fatalf("event %d text = %q, want %q", i, texts[i], wantTexts[i]) + } + } + protocolID := events[0].Data.MessageID + if events[2].Data.MessageID != protocolID || !events[2].Data.IsAppend { + t.Fatalf("resumed protocol event = id:%q append:%t, want id:%q append:true", + events[2].Data.MessageID, events[2].Data.IsAppend, protocolID) + } + if events[1].Data.MessageID == protocolID || events[3].Data.MessageID == protocolID { + t.Fatal("legacy chunks merged into protocol record") + } + if events[1].Data.MessageID == events[3].Data.MessageID { + t.Fatal("legacy chunks on opposite sides of a protocol chunk shared a record") + } +} + +func TestHandleAgentEvent_LegacyThinkingFlushesBeforeProtocolThinking(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + mgr.handleAgentEvent(execution, agentctl.AgentEvent{Type: "reasoning", ReasoningText: "legacy thought"}) + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "reasoning", + ReasoningText: "protocol thought", + ProtocolMessageID: "thought-message", + }) + + events := streamEventsOfType(eventBus, "thinking_streaming") + if len(events) != 2 { + t.Fatalf("thinking event count = %d, want 2", len(events)) + } + if events[0].Data.Text != "legacy thought" || events[1].Data.Text != "protocol thought" { + t.Fatalf("thinking order = (%q, %q), want legacy then protocol", + events[0].Data.Text, events[1].Data.Text) + } + if events[0].Data.MessageID == events[1].Data.MessageID { + t.Fatalf("mixed legacy and protocol thinking shared Kandev ID %q", events[0].Data.MessageID) + } +} + +func TestHandleAgentEvent_ProtocolAssistantHistoryPersistedOnceInWireOrder(t *testing.T) { + mgr, _ := createTestManagerWithTracking() + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + mgr.historyManager = history + execution := createTestExecution("exec-1", "task-1", "session-1") + execution.historyEnabled = true + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + for _, event := range []agentctl.AgentEvent{ + {Type: "message_chunk", Text: "first", ProtocolMessageID: "message-a"}, + {Type: "message_chunk", Text: " second", ProtocolMessageID: "message-b"}, + {Type: "message_chunk", Text: " third", ProtocolMessageID: "message-a"}, + } { + mgr.handleAgentEvent(execution, event) + } + mgr.handleAgentEvent(execution, agentctl.AgentEvent{Type: "complete"}) + mgr.handleAgentEvent(execution, agentctl.AgentEvent{Type: "complete"}) + + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + var agentEntries []HistoryEntry + for _, entry := range entries { + if entry.Type == "agent_message" { + agentEntries = append(agentEntries, entry) + } + } + if len(agentEntries) != 1 { + t.Fatalf("agent history entry count = %d, want 1", len(agentEntries)) + } + if agentEntries[0].Content != "first second third" { + t.Fatalf("agent history content = %q, want wire-order transcript", agentEntries[0].Content) + } + execution.messageMu.Lock() + historyBufferLen := execution.assistantHistoryBuffer.Len() + execution.messageMu.Unlock() + if historyBufferLen != 0 { + t.Fatalf("assistant history accumulator length = %d after completion, want 0", historyBufferLen) + } +} + +func TestStreamDisconnect_PersistsPartialAssistantHistoryExactlyOnce(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + mgr.historyManager = history + execution := createTestExecution("exec-1", "task-1", "session-1") + execution.historyEnabled = true + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "message_chunk", + Text: "partial response", + ProtocolMessageID: "message-a", + }) + mgr.handleStreamDisconnect(execution, errors.New("connection lost"), 0) + + // A later prompt or reset must not discard or duplicate the segment. + flushAssistantHistory(execution, history, mgr.logger) + execution.messageMu.Lock() + execution.resetStreamingStateLocked() + execution.messageMu.Unlock() + mgr.flushAssistantHistory(execution) + + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + if len(entries) != 1 || entries[0].Type != "agent_message" || + entries[0].Content != "partial response" { + t.Fatalf("disconnect history = %+v, want one partial assistant message", entries) + } + + messageEvents := streamEventsOfType(eventBus, "message_streaming") + if len(messageEvents) != 1 || messageEvents[0].Data.Text != "partial response" { + t.Fatalf("visible messages = %+v, want the original chunk exactly once", messageEvents) + } +} + +func TestPromptResetWinsDisconnectRaceWithoutDroppingHistory(t *testing.T) { + mgr, _ := createTestManagerWithTracking() + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + mgr.historyManager = history + execution := createTestExecution("exec-1", "task-1", "session-1") + execution.historyEnabled = true + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + mgr.handleAgentEvent(execution, agentctl.AgentEvent{ + Type: "message_chunk", + Text: "response before disconnect callback", + }) + + // connectUpdatesStream signals promptDoneCh before invoking the disconnect + // callback. Model a next prompt claiming the buffer first. + flushAssistantHistory(execution, history, mgr.logger) + execution.messageMu.Lock() + execution.resetStreamingStateLocked() + execution.messageMu.Unlock() + mgr.handleStreamDisconnect(execution, errors.New("connection lost"), 0) + + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + if len(entries) != 1 || entries[0].Content != "response before disconnect callback" { + t.Fatalf("disconnect-race history = %+v, want one partial assistant message", entries) + } +} + +func TestDelayedDisconnectCannotAffectReplacementGeneration(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + mgr.historyManager = history + execution := createTestExecution("exec-1", "task-1", "session-1") + execution.historyEnabled = true + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + originalGeneration, err := mgr.executionStore.BeginPrompt(execution.ID) + if err != nil { + t.Fatalf("begin original prompt: %v", err) + } + mgr.handleAgentEvent(execution, agentctl.AgentEvent{Type: "message_chunk", Text: "old partial"}) + + callbackReached := make(chan struct{}) + releaseCallback := make(chan struct{}) + callbackDone := make(chan struct{}) + streamManager := NewStreamManager(mgr.logger, StreamCallbacks{ + OnStreamDisconnect: func(exec *AgentExecution, disconnectErr error, generation uint64) { + close(callbackReached) + <-releaseCallback + mgr.handleStreamDisconnect(exec, disconnectErr, generation) + close(callbackDone) + }, + }, nil, nil) + go streamManager.handleUpdatesDisconnect(execution, errors.New("old stream lost")) + + <-callbackReached + signal := <-execution.promptDoneCh + if signal.PromptGeneration != originalGeneration { + t.Fatalf("disconnect signal generation = %d, want %d", signal.PromptGeneration, originalGeneration) + } + + replacementGeneration, err := mgr.executionStore.BeginPrompt(execution.ID) + if err != nil { + t.Fatalf("begin replacement prompt: %v", err) + } + if replacementGeneration == originalGeneration { + t.Fatal("replacement prompt did not advance generation") + } + resetStreamingStateWithHistory(execution, history, mgr.logger) + mgr.handleAgentEvent(execution, agentctl.AgentEvent{Type: "message_chunk", Text: "replacement text"}) + + close(releaseCallback) + <-callbackDone + + if execution.Status != v1.AgentStatusRunning { + t.Fatalf("replacement status = %s, want %s", execution.Status, v1.AgentStatusRunning) + } + execution.messageMu.Lock() + assistantHistory := execution.assistantHistoryBuffer.String() + messageBuffer := execution.messageBuffer.String() + execution.messageMu.Unlock() + if assistantHistory != "replacement text" || messageBuffer != "replacement text" { + t.Fatalf("replacement buffers changed: history=%q message=%q", assistantHistory, messageBuffer) + } + + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + if len(entries) != 1 || entries[0].Content != "old partial" { + t.Fatalf("history = %+v, want only the original partial response", entries) + } + for _, published := range eventBus.PublishedEvents { + if published.Subject == events.AgentctlError { + t.Fatal("superseded disconnect published AgentctlError") + } + } +} + +func TestResetStreamingStateWithHistory_HasNoDrainResetGap(t *testing.T) { + mgr, _ := createTestManagerWithTracking() + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + execution := createTestExecution("exec-1", "task-1", "session-1") + execution.historyEnabled = true + execution.assistantHistoryBuffer.WriteString("old partial") + execution.messageBuffer.WriteString("old partial") + + // Hold persistence so the test can force a chunk to arrive after history + // detaches. With split drain/reset locks, that chunk was accepted in the + // gap and then erased by reset. The atomic helper resets before blocking + // on history I/O, so the late chunk belongs to the post-reset buffer. + history.mu.Lock() + historyLocked := true + defer func() { + if historyLocked { + history.mu.Unlock() + } + }() + resetDone := make(chan struct{}) + go func() { + resetStreamingStateWithHistory(execution, history, mgr.logger) + close(resetDone) + }() + + deadline := time.Now().Add(2 * time.Second) + for { + execution.messageMu.Lock() + detached := execution.assistantHistoryBuffer.Len() == 0 + execution.messageMu.Unlock() + if detached { + break + } + if time.Now().After(deadline) { + t.Fatal("assistant history did not detach") + } + runtime.Gosched() + } + mgr.handleMessageChunkEvent(execution, agentctl.AgentEvent{Type: "message_chunk", Text: "late chunk"}) + + history.mu.Unlock() + historyLocked = false + <-resetDone + + execution.messageMu.Lock() + assistantHistory := execution.assistantHistoryBuffer.String() + messageBuffer := execution.messageBuffer.String() + execution.messageMu.Unlock() + if assistantHistory != "late chunk" || messageBuffer != "late chunk" { + t.Fatalf("late chunk lost across reset: history=%q message=%q", assistantHistory, messageBuffer) + } + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + if len(entries) != 1 || entries[0].Content != "old partial" { + t.Fatalf("detached history = %+v, want only old partial", entries) + } +} + +func TestStreamDisconnect_DiscardsHistoryWhenRecordingUnavailable(t *testing.T) { + tests := []struct { + name string + historyManager bool + historyEnabled bool + sessionID string + }{ + {name: "history manager unavailable", historyEnabled: true, sessionID: "session-1"}, + {name: "history disabled", historyManager: true, sessionID: "session-1"}, + {name: "session unavailable", historyManager: true, historyEnabled: true}, + } + + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + mgr, _ := createTestManagerWithTracking() + mgr.historyManager = nil + if test.historyManager { + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + mgr.historyManager = history + } + execution := createTestExecution("exec-1", "task-1", test.sessionID) + execution.historyEnabled = test.historyEnabled + execution.assistantHistoryBuffer.WriteString("discard me") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + mgr.handleStreamDisconnect(execution, errors.New("connection lost"), 0) + + execution.messageMu.Lock() + buffered := execution.assistantHistoryBuffer.Len() + execution.messageMu.Unlock() + if buffered != 0 { + t.Fatalf("assistant history buffer length = %d, want discarded", buffered) + } + }) + } +} + +func TestHandleAgentEvent_ProtocolAssistantAndToolHistoryFollowWireOrder(t *testing.T) { + mgr, _ := createTestManagerWithTracking() + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + mgr.historyManager = history + execution := createTestExecution("exec-1", "task-1", "session-1") + execution.historyEnabled = true + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + for _, event := range []agentctl.AgentEvent{ + {Type: "message_chunk", Text: "before tool", ProtocolMessageID: "message-a"}, + {Type: "tool_call", ToolCallID: "tool-1", ToolName: "read_file", ToolStatus: "started"}, + {Type: "message_chunk", Text: "during tool", ProtocolMessageID: "message-a"}, + {Type: "tool_update", ToolCallID: "tool-1", ToolName: "read_file", ToolStatus: toolStatusComplete}, + {Type: "message_chunk", Text: "after result", ProtocolMessageID: "message-a"}, + {Type: "complete"}, + } { + mgr.handleAgentEvent(execution, event) + } + + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + if len(entries) != 5 { + t.Fatalf("history entry count = %d, want 5: %+v", len(entries), entries) + } + wantTypes := []string{"agent_message", historyEntryTypeToolCall, "agent_message", "tool_result", "agent_message"} + wantContent := []string{"before tool", "", "during tool", "", "after result"} + for i := range wantTypes { + if entries[i].Type != wantTypes[i] || entries[i].Content != wantContent[i] { + t.Fatalf("entry %d = type:%q content:%q, want type:%q content:%q", + i, entries[i].Type, entries[i].Content, wantTypes[i], wantContent[i]) + } + } +} + +func TestHandleAgentEvent_MixedAssistantHistoryPreservesAllTextAroundTool(t *testing.T) { + mgr, eventBus := createTestManagerWithTracking() + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + mgr.historyManager = history + execution := createTestExecution("exec-1", "task-1", "session-1") + execution.historyEnabled = true + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + for _, event := range []agentctl.AgentEvent{ + {Type: "message_chunk", Text: "legacy before "}, + {Type: "message_chunk", Text: "protocol before", ProtocolMessageID: "message-a"}, + {Type: "tool_call", ToolCallID: "tool-1", ToolName: "read_file"}, + {Type: "message_chunk", Text: "legacy after "}, + {Type: "message_chunk", Text: "protocol after", ProtocolMessageID: "message-a"}, + {Type: "complete"}, + } { + mgr.handleAgentEvent(execution, event) + } + + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + if len(entries) != 3 { + t.Fatalf("history entry count = %d, want 3: %+v", len(entries), entries) + } + if entries[0].Type != "agent_message" || entries[0].Content != "legacy before protocol before" { + t.Fatalf("first history segment = %+v, want complete mixed pre-tool text", entries[0]) + } + if entries[1].Type != historyEntryTypeToolCall { + t.Fatalf("second history entry type = %q, want %q", entries[1].Type, historyEntryTypeToolCall) + } + if entries[2].Type != "agent_message" || entries[2].Content != "legacy after protocol after" { + t.Fatalf("final history segment = %+v, want complete mixed post-tool text", entries[2]) + } + + messageEvents := streamEventsOfType(eventBus, "message_streaming") + if len(messageEvents) != 4 { + t.Fatalf("visible message event count = %d, want 4", len(messageEvents)) + } + wantVisible := []string{"legacy before", "protocol before", "legacy after", "protocol after"} + for i, want := range wantVisible { + if messageEvents[i].Data.Text != want { + t.Fatalf("visible event %d text = %q, want %q", i, messageEvents[i].Data.Text, want) + } + } +} + +func TestHandleAgentEvent_HistoryDisabledDoesNotPersistOrLeakAssistantText(t *testing.T) { + mgr, _ := createTestManagerWithTracking() + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + mgr.historyManager = history + execution := createTestExecution("exec-1", "task-1", "session-1") + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + for _, event := range []agentctl.AgentEvent{ + {Type: "message_chunk", Text: "legacy"}, + {Type: "message_chunk", Text: " protocol", ProtocolMessageID: "message-a"}, + {Type: "tool_call", ToolCallID: "tool-1", ToolName: "read_file"}, + {Type: "message_chunk", Text: "after", ProtocolMessageID: "message-a"}, + {Type: "complete"}, + } { + mgr.handleAgentEvent(execution, event) + } + + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + if len(entries) != 0 { + t.Fatalf("history-disabled execution persisted entries: %+v", entries) + } + execution.messageMu.Lock() + historyBufferLen := execution.assistantHistoryBuffer.Len() + execution.messageMu.Unlock() + if historyBufferLen != 0 { + t.Fatalf("assistant history accumulator length = %d after completion, want 0", historyBufferLen) + } +} + +func TestHandleAgentEvent_EmptyHistoryBoundariesDoNotDuplicateAssistant(t *testing.T) { + mgr, _ := createTestManagerWithTracking() + history, err := NewSessionHistoryManager(t.TempDir(), "", newTestLogger()) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + mgr.historyManager = history + execution := createTestExecution("exec-1", "task-1", "session-1") + execution.historyEnabled = true + if err := mgr.executionStore.Add(execution); err != nil { + t.Fatalf("add execution: %v", err) + } + + for _, event := range []agentctl.AgentEvent{ + {Type: "message_chunk", Text: "only once", ProtocolMessageID: "message-a"}, + {Type: "tool_call", ToolCallID: "tool-1", ToolName: "read_file"}, + {Type: "tool_update", ToolCallID: "tool-1", ToolName: "read_file", ToolStatus: toolStatusComplete}, + {Type: "complete"}, + {Type: "complete"}, + } { + mgr.handleAgentEvent(execution, event) + } + + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + if len(entries) != 3 { + t.Fatalf("history entry count = %d, want assistant/tool/result only: %+v", len(entries), entries) + } + if entries[0].Type != "agent_message" || entries[0].Content != "only once" { + t.Fatalf("assistant history entry = %+v, want one exact segment", entries[0]) + } + if entries[1].Type != historyEntryTypeToolCall || entries[2].Type != "tool_result" { + t.Fatalf("history boundary entries = (%q, %q), want tool_call/tool_result", + entries[1].Type, entries[2].Type) + } +} + +func streamEventsOfType(eventBus *MockEventBusWithTracking, eventType string) []AgentStreamEventPayload { + var matching []AgentStreamEventPayload + for _, event := range eventBus.getStreamEvents() { + if event.Data != nil && event.Data.Type == eventType { + matching = append(matching, event) + } + } + return matching +} + // TestHandleAgentEvent_StreamingThenComplete tests the normal flow: // message_chunk events followed by complete event - should NOT create duplicate func TestHandleAgentEvent_StreamingThenComplete(t *testing.T) { diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go index 09a119f76d..2abb103dae 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction.go @@ -216,8 +216,9 @@ func (m *Manager) escalateStuckCancel(ctx context.Context, execution *AgentExecu select { case execution.promptDoneCh <- PromptCompletionSignal{ - IsError: true, - Error: "cancel escalated: agent did not complete turn within timeout", + IsError: true, + Error: "cancel escalated: agent did not complete turn within timeout", + PromptGeneration: execution.promptGenerationSnapshot(), }: default: // Channel already has a pending signal; SendPrompt will pick that up instead. @@ -457,12 +458,7 @@ func (m *Manager) ResetAgentContext(ctx context.Context, executionID string) err exec.needsResumeContext = false exec.resumeContextInjected = false - exec.messageMu.Lock() - exec.messageBuffer.Reset() - exec.thinkingBuffer.Reset() - exec.currentMessageID = "" - exec.currentThinkingID = "" - exec.messageMu.Unlock() + m.resetStreamingStateWithHistory(exec) // Drain any stale prompt completion signal select { @@ -737,12 +733,7 @@ func (m *Manager) resetAgentRestartState(executionID string, commands agentComma exec.ContinueCommand = commands.continue_ exec.AgentArgs = commands.args exec.ContinueArgs = commands.continueArgs - exec.messageMu.Lock() - exec.messageBuffer.Reset() - exec.thinkingBuffer.Reset() - exec.currentMessageID = "" - exec.currentThinkingID = "" - exec.messageMu.Unlock() + m.resetStreamingStateWithHistory(exec) select { case <-exec.promptDoneCh: default: diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go index 07fb11500c..ae7b33965b 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_interaction_test.go @@ -233,6 +233,9 @@ func TestManager_RestartAgentProcess_Success(t *testing.T) { exec.thinkingBuffer.WriteString("old-thinking") exec.currentMessageID = "msg-1" exec.currentThinkingID = "th-1" + exec.protocolMessageIDs = map[string]string{"source-message": "msg-1"} + exec.protocolThinkingIDs = map[string]string{"source-thought": "th-1"} + exec.assistantHistoryBuffer.WriteString("old response") exec.needsResumeContext = true exec.resumeContextInjected = true exec.promptDoneCh <- PromptCompletionSignal{StopReason: "stale"} @@ -258,6 +261,12 @@ func TestManager_RestartAgentProcess_Success(t *testing.T) { if exec.currentMessageID != "" || exec.currentThinkingID != "" { t.Fatalf("expected streaming message IDs to be reset") } + if exec.protocolMessageIDs != nil || exec.protocolThinkingIDs != nil { + t.Fatalf("expected protocol message correlation to be reset") + } + if exec.assistantHistoryBuffer.Len() != 0 { + t.Fatalf("expected assistant history accumulator to be reset") + } if exec.needsResumeContext || exec.resumeContextInjected { t.Fatalf("expected resume context flags to be reset") } @@ -659,6 +668,9 @@ func TestManager_ResetAgentContext_ReappliesSessionMode(t *testing.T) { CurrentModeID: "acceptEdits", AvailableModes: []streams.SessionModeInfo{{ID: "default"}, {ID: "acceptEdits"}}, }) + exec.protocolMessageIDs = map[string]string{"source-message": "msg-1"} + exec.protocolThinkingIDs = map[string]string{"source-thought": "th-1"} + exec.assistantHistoryBuffer.WriteString("old response") require.NoError(t, mgr.executionStore.Add(exec)) require.NoError(t, mgr.ResetAgentContext(ctx, exec.ID)) @@ -669,6 +681,9 @@ func TestManager_ResetAgentContext_ReappliesSessionMode(t *testing.T) { "fast-path reset must re-apply the previously-active session mode") require.NotNil(t, exec.GetModeState()) require.Equal(t, "acceptEdits", exec.GetModeState().CurrentModeID) + require.Nil(t, exec.protocolMessageIDs) + require.Nil(t, exec.protocolThinkingIDs) + require.Zero(t, exec.assistantHistoryBuffer.Len()) } // TestManager_RestartAgentProcess_PrefersPersistedModeOverStaleCache is the diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go index 2f67bfea3d..be9ca367ce 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_launch_test.go @@ -129,6 +129,39 @@ func TestBuildAgentCommand_ResumeFlag(t *testing.T) { }) } +func TestBuildAgentCommand_PreservesPinnedACPBridgeSpecs(t *testing.T) { + mgr := newTestManager(t) + tests := []struct { + name string + agent agents.Agent + want string + }{ + { + name: "claude", + agent: agents.NewClaudeACP(), + want: "npx -y @agentclientprotocol/claude-agent-acp@0.61.0", + }, + { + name: "codex", + agent: agents.NewCodexACP(), + want: "npx -y @agentclientprotocol/codex-acp@1.1.5", + }, + { + name: "opencode", + agent: agents.NewOpenCodeACP(), + want: "opencode acp", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + cmds, err := mgr.buildAgentCommand(&LaunchRequest{}, nil, tt.agent, false) + require.NoError(t, err) + require.Equal(t, tt.want, cmds.initial) + }) + } +} + // cliFlagTestAgent is a minimal BuildCommand that produces a stable prefix // so tests can assert CLI flag tokens are appended after the agent's own // argv by CommandBuilder.BuildCommand (not by the agent itself). diff --git a/apps/backend/internal/agent/runtime/lifecycle/manager_streaming.go b/apps/backend/internal/agent/runtime/lifecycle/manager_streaming.go index 49a8ef582c..b98e0fb64e 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/manager_streaming.go +++ b/apps/backend/internal/agent/runtime/lifecycle/manager_streaming.go @@ -5,9 +5,208 @@ import ( "time" "github.com/google/uuid" + "github.com/kandev/kandev/internal/common/logger" "go.uber.org/zap" ) +const thinkingStreamingEventType = "thinking_streaming" + +func (e *AgentExecution) clearProtocolMessageCorrelationLocked() { + e.protocolMessageIDs = nil + e.protocolThinkingIDs = nil +} + +func (e *AgentExecution) resetStreamingStateLocked() { + e.messageBuffer.Reset() + e.thinkingBuffer.Reset() + e.assistantHistoryBuffer.Reset() + e.currentMessageID = "" + e.currentThinkingID = "" + e.clearProtocolMessageCorrelationLocked() +} + +func protocolRecordID(ids *map[string]string, protocolMessageID string) (string, bool) { + if *ids == nil { + *ids = make(map[string]string) + } + if messageID, ok := (*ids)[protocolMessageID]; ok { + return messageID, true + } + messageID := uuid.New().String() + (*ids)[protocolMessageID] = messageID + return messageID, false +} + +func (m *Manager) publishProtocolMessage( + execution *AgentExecution, + protocolMessageID string, + content string, +) { + execution.messageMu.Lock() + messageID, isAppend := protocolRecordID(&execution.protocolMessageIDs, protocolMessageID) + execution.messageMu.Unlock() + + m.publishStreamingContent(execution, "message_streaming", messageID, content, isAppend) +} + +func (m *Manager) appendAssistantHistoryChunk(execution *AgentExecution, content string) { + execution.messageMu.Lock() + execution.assistantHistoryBuffer.WriteString(content) + execution.messageMu.Unlock() +} + +// flushAssistantHistory persists the assistant text observed since the prior +// history boundary. It is independent from visible-stream flushing so a +// subagent tool can retain UI nesting while history still follows wire order. +func (m *Manager) flushAssistantHistory(execution *AgentExecution) { + flushAssistantHistory(execution, m.historyManager, m.logger) +} + +func (m *Manager) resetStreamingStateWithHistory(execution *AgentExecution) { + resetStreamingStateWithHistory(execution, m.historyManager, m.logger) +} + +// flushAssistantHistory drains the pending assistant segment exactly once. +// Draining also deliberately discards the segment when history recording is +// unavailable or disabled, so stale text cannot leak into a later prompt. +func flushAssistantHistory( + execution *AgentExecution, + historyManager *SessionHistoryManager, + log *logger.Logger, +) { + execution.messageMu.Lock() + content := execution.detachAssistantHistoryLocked() + execution.messageMu.Unlock() + + persistAssistantHistory(content, execution, historyManager, log) +} + +// resetStreamingStateWithHistory atomically detaches the prior assistant +// transcript and resets every streaming buffer/correlation under messageMu. +// Persistence happens afterward so filesystem I/O never blocks incoming chunks. +func resetStreamingStateWithHistory( + execution *AgentExecution, + historyManager *SessionHistoryManager, + log *logger.Logger, +) { + execution.messageMu.Lock() + content := execution.detachAssistantHistoryLocked() + execution.resetStreamingStateLocked() + execution.messageMu.Unlock() + + persistAssistantHistory(content, execution, historyManager, log) +} + +func (e *AgentExecution) detachAssistantHistoryLocked() string { + content := e.assistantHistoryBuffer.String() + e.assistantHistoryBuffer.Reset() + return content +} + +func persistAssistantHistory( + content string, + execution *AgentExecution, + historyManager *SessionHistoryManager, + log *logger.Logger, +) { + if content == "" || historyManager == nil || !execution.historyEnabled || execution.SessionID == "" { + return + } + if err := historyManager.AppendAgentMessage(execution.SessionID, content); err != nil { + log.Warn("failed to store agent message to history", zap.Error(err)) + } +} + +// flushPendingLegacyMessage closes only the ID-less assistant stream. It is +// used when an explicit protocol message begins so buffered legacy content is +// published first and a later ID-less chunk cannot append across that boundary. +func (m *Manager) flushPendingLegacyMessage(execution *AgentExecution) { + execution.messageMu.Lock() + content := execution.messageBuffer.String() + execution.messageBuffer.Reset() + messageID := execution.currentMessageID + execution.currentMessageID = "" + execution.messageMu.Unlock() + + trimmed := strings.TrimSpace(content) + if trimmed == "" { + return + } + if messageID != "" { + m.publishStreamingMessageFinal(execution, messageID, trimmed) + return + } + m.publishStreamingMessage(execution, trimmed) + execution.messageMu.Lock() + execution.currentMessageID = "" + execution.messageMu.Unlock() +} + +// flushPendingLegacyThinking is the reasoning-stream counterpart of +// flushPendingLegacyMessage. +func (m *Manager) flushPendingLegacyThinking(execution *AgentExecution) { + execution.messageMu.Lock() + content := execution.thinkingBuffer.String() + execution.thinkingBuffer.Reset() + messageID := execution.currentThinkingID + execution.currentThinkingID = "" + execution.messageMu.Unlock() + + trimmed := strings.TrimSpace(content) + if trimmed == "" { + return + } + if messageID != "" { + m.publishStreamingThinkingFinal(execution, messageID, trimmed) + return + } + m.publishStreamingThinking(execution, trimmed) + execution.messageMu.Lock() + execution.currentThinkingID = "" + execution.messageMu.Unlock() +} + +func (m *Manager) publishProtocolThinking( + execution *AgentExecution, + protocolMessageID string, + content string, +) { + execution.messageMu.Lock() + messageID, isAppend := protocolRecordID(&execution.protocolThinkingIDs, protocolMessageID) + execution.messageMu.Unlock() + + m.publishStreamingContent(execution, thinkingStreamingEventType, messageID, content, isAppend) +} + +func (m *Manager) publishStreamingContent( + execution *AgentExecution, + eventType string, + messageID string, + content string, + isAppend bool, +) { + event := AgentStreamEventData{ + Type: eventType, + Text: content, + MessageID: messageID, + IsAppend: isAppend, + } + if eventType == thinkingStreamingEventType { + event.MessageType = "thinking" + } + + payload := &AgentStreamEventPayload{ + Type: "agent/event", + Timestamp: time.Now().UTC().Format(time.RFC3339Nano), + AgentID: execution.ID, + ExecutionID: execution.ID, + TaskID: execution.TaskID, + SessionID: execution.SessionID, + Data: &event, + } + m.eventPublisher.PublishAgentStreamEventPayload(payload) +} + // publishStreamingMessage publishes a streaming message event for real-time text updates. // It creates a new message on first call (currentMessageID empty) or appends to existing. // The message ID is generated and set synchronously to avoid race conditions. @@ -153,7 +352,7 @@ func (m *Manager) publishStreamingThinking(execution *AgentExecution, content st execution.messageMu.Unlock() event := AgentStreamEventData{ - Type: "thinking_streaming", + Type: thinkingStreamingEventType, Text: content, MessageID: thinkingID, IsAppend: isAppend, @@ -179,7 +378,7 @@ func (m *Manager) publishStreamingThinking(execution *AgentExecution, content st // This is called during flush to append any remaining buffered thinking content. func (m *Manager) publishStreamingThinkingFinal(execution *AgentExecution, thinkingID, content string) { event := AgentStreamEventData{ - Type: "thinking_streaming", + Type: thinkingStreamingEventType, Text: content, MessageID: thinkingID, IsAppend: true, diff --git a/apps/backend/internal/agent/runtime/lifecycle/session.go b/apps/backend/internal/agent/runtime/lifecycle/session.go index 2fadb38f54..f971d909da 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/session.go +++ b/apps/backend/internal/agent/runtime/lifecycle/session.go @@ -560,13 +560,26 @@ func (sm *SessionManager) buildEffectivePrompt(execution *AgentExecution, prompt } // waitForPromptDone waits for the prompt to complete, checking for stalls periodically. -func (sm *SessionManager) waitForPromptDone(ctx context.Context, execution *AgentExecution) (*PromptResult, error) { +func (sm *SessionManager) waitForPromptDone( + ctx context.Context, + execution *AgentExecution, + promptGeneration uint64, +) (*PromptResult, error) { stallTicker := time.NewTicker(30 * time.Second) defer stallTicker.Stop() for { select { case signal := <-execution.promptDoneCh: + if signal.PromptGeneration != 0 && + promptGeneration != 0 && + signal.PromptGeneration != promptGeneration { + sm.logger.Debug("ignoring completion signal for superseded prompt generation", + zap.String("execution_id", execution.ID), + zap.Uint64("signal_prompt_generation", signal.PromptGeneration), + zap.Uint64("active_prompt_generation", promptGeneration)) + continue + } if signal.IsError { sm.logger.Error("prompt completed with error", zap.String("execution_id", execution.ID), @@ -703,7 +716,7 @@ func (sm *SessionManager) sendPrompt( if err := sm.triggerPrompt(preparedCtx, execution, effectivePrompt, attachments, promptGeneration); err != nil { return nil, err } - return sm.finishAcceptedPrompt(preparedCtx, execution, dispatchOnly, onDispatched) + return sm.finishAcceptedPrompt(preparedCtx, execution, dispatchOnly, onDispatched, promptGeneration) } func (sm *SessionManager) preparePrompt( @@ -745,12 +758,11 @@ func (sm *SessionManager) preparePrompt( promptGeneration = beginExecutionPrompt(execution) } - execution.messageMu.Lock() - execution.messageBuffer.Reset() - execution.thinkingBuffer.Reset() - execution.currentMessageID = "" - execution.currentThinkingID = "" - execution.messageMu.Unlock() + // A disconnect can release the prior SendPrompt before its disconnect + // callback has persisted a partial assistant response. Drain that history + // segment before resetting the streaming state; the callback uses the same + // locked drain, so the segment is persisted at most once. + resetStreamingStateWithHistory(execution, sm.historyManager, sm.logger) effectivePrompt := sm.buildEffectivePrompt(execution, prompt) sm.logger.Info("sending prompt to agent", @@ -807,6 +819,7 @@ func (sm *SessionManager) finishAcceptedPrompt( execution *AgentExecution, dispatchOnly bool, onDispatched func(), + promptGeneration uint64, ) (*PromptResult, error) { if dispatchOnly { execution.dispatchedPromptPending = true @@ -818,7 +831,7 @@ func (sm *SessionManager) finishAcceptedPrompt( return &PromptResult{StopReason: PromptStopReasonDispatched}, nil } // Wait for completion signal from handleAgentEvent(complete) or stream disconnect. - return sm.waitForPromptDone(ctx, execution) + return sm.waitForPromptDone(ctx, execution, promptGeneration) } func (sm *SessionManager) dispatchPrompt( diff --git a/apps/backend/internal/agent/runtime/lifecycle/session_test.go b/apps/backend/internal/agent/runtime/lifecycle/session_test.go index c942d1d4bf..3a24b24d00 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/session_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/session_test.go @@ -1204,6 +1204,60 @@ func TestSendPrompt_DispatchOnlyReturnsWithoutWaiting(t *testing.T) { } } +func TestSendPrompt_PersistsPartialAssistantHistoryBeforeReset(t *testing.T) { + mock := newMockAgentServer(t) + t.Cleanup(mock.Close) + + log := newSessionTestLogger() + history, err := NewSessionHistoryManager(t.TempDir(), "", log) + if err != nil { + t.Fatalf("create history manager: %v", err) + } + sm := NewSessionManager(log, make(chan struct{})) + sm.SetDependencies(nil, nil, nil, history) + + client := createTestClient(t, mock.server.URL) + t.Cleanup(client.Close) + ctx := context.Background() + if err := client.StreamUpdates(ctx, func(agentctl.AgentEvent) {}, nil, nil); err != nil { + t.Fatalf("connect stream: %v", err) + } + waitForWSConnected(t, mock) + + execution := &AgentExecution{ + ID: "test-exec", + TaskID: "test-task", + SessionID: "test-session", + WorkspacePath: "/workspace", + Status: v1.AgentStatusReady, + agentctl: client, + promptDoneCh: make(chan PromptCompletionSignal, 1), + historyEnabled: true, + } + execution.assistantHistoryBuffer.WriteString("partial prior response") + + if _, err := sm.SendPrompt(ctx, execution, "next prompt", true, nil, true); err != nil { + t.Fatalf("SendPrompt: %v", err) + } + + entries, err := history.ReadHistory(execution.SessionID) + if err != nil { + t.Fatalf("read history: %v", err) + } + if len(entries) != 2 { + t.Fatalf("history entry count = %d, want assistant then user: %+v", len(entries), entries) + } + if entries[0].Type != "agent_message" || entries[0].Content != "partial prior response" { + t.Fatalf("first history entry = %+v, want partial assistant response", entries[0]) + } + if entries[1].Type != "user_message" || entries[1].Content != "next prompt" { + t.Fatalf("second history entry = %+v, want next user prompt", entries[1]) + } + if execution.assistantHistoryBuffer.Len() != 0 { + t.Fatal("assistant history buffer was not reset after prompt dispatch") + } +} + func TestSendPrompt_DispatchOnlyBlocksNextPromptUntilItsCompletion(t *testing.T) { mock := newMockAgentServer(t) defer mock.Close() @@ -1520,7 +1574,7 @@ func TestWaitForPromptDone_TreatsPromptAbandonedAfterCancelAsCancelEscalated(t * Error: "prompt abandoned after cancel", } - _, err := sm.waitForPromptDone(context.Background(), execution) + _, err := sm.waitForPromptDone(context.Background(), execution, 0) if !errors.Is(err, ErrCancelEscalated) { t.Fatalf("expected ErrCancelEscalated, got: %v", err) } @@ -1529,6 +1583,31 @@ func TestWaitForPromptDone_TreatsPromptAbandonedAfterCancelAsCancelEscalated(t * } } +func TestWaitForPromptDone_IgnoresSupersededGenerationSignal(t *testing.T) { + sm := NewSessionManager(newSessionTestLogger(), make(chan struct{})) + execution := &AgentExecution{ + ID: "test-exec", + promptDoneCh: make(chan PromptCompletionSignal, 2), + } + execution.promptDoneCh <- PromptCompletionSignal{ + IsError: true, + Error: "old stream disconnected", + PromptGeneration: 1, + } + execution.promptDoneCh <- PromptCompletionSignal{ + StopReason: "end_turn", + PromptGeneration: 2, + } + + result, err := sm.waitForPromptDone(context.Background(), execution, 2) + if err != nil { + t.Fatalf("waitForPromptDone: %v", err) + } + if result == nil || result.StopReason != "end_turn" { + t.Fatalf("result = %+v, want replacement completion", result) + } +} + func TestSendPrompt_TriggerTimeCancelReleaseReturnsErrCancelEscalated(t *testing.T) { mock := newMockAgentServer(t) defer mock.Close() diff --git a/apps/backend/internal/agent/runtime/lifecycle/streams.go b/apps/backend/internal/agent/runtime/lifecycle/streams.go index e914987f34..353c38aff8 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/streams.go +++ b/apps/backend/internal/agent/runtime/lifecycle/streams.go @@ -14,7 +14,7 @@ import ( // StreamCallbacks defines callbacks for stream events type StreamCallbacks struct { OnAgentEvent func(execution *AgentExecution, event agentctl.AgentEvent) - OnStreamDisconnect func(execution *AgentExecution, err error) + OnStreamDisconnect func(execution *AgentExecution, err error, promptGeneration uint64) OnGitStatus func(execution *AgentExecution, update *agentctl.GitStatusUpdate) OnGitCommit func(execution *AgentExecution, commit *agentctl.GitCommitNotification) OnGitReset func(execution *AgentExecution, reset *agentctl.GitResetNotification) @@ -287,20 +287,8 @@ func (sm *StreamManager) connectUpdatesStream(execution *AgentExecution, ready c sm.callbacks.OnAgentEvent(execution, event) } }, sm.mcpHandlerFor(execution), func(disconnectErr error) { - // WebSocket dropped — signal promptDoneCh so SendPrompt doesn't hang forever. - // Only signal on unexpected errors (not normal close). if disconnectErr != nil { - select { - case execution.promptDoneCh <- PromptCompletionSignal{ - IsError: true, - Error: "agent stream disconnected: " + disconnectErr.Error(), - }: - default: - } - // Notify lifecycle manager so it can proactively update execution status - if sm.callbacks.OnStreamDisconnect != nil { - sm.callbacks.OnStreamDisconnect(execution, disconnectErr) - } + sm.handleUpdatesDisconnect(execution, disconnectErr) } }) @@ -318,6 +306,23 @@ func (sm *StreamManager) connectUpdatesStream(execution *AgentExecution, ready c } } +func (sm *StreamManager) handleUpdatesDisconnect(execution *AgentExecution, disconnectErr error) { + // Capture prompt ownership before signaling the waiter. A replacement may + // advance the generation before the lifecycle callback runs. + promptGeneration := execution.promptGenerationSnapshot() + select { + case execution.promptDoneCh <- PromptCompletionSignal{ + IsError: true, + Error: "agent stream disconnected: " + disconnectErr.Error(), + PromptGeneration: promptGeneration, + }: + default: + } + if sm.callbacks.OnStreamDisconnect != nil { + sm.callbacks.OnStreamDisconnect(execution, disconnectErr, promptGeneration) + } +} + // connectMCPStream opens the agent updates WebSocket for a PASSTHROUGH session // purely to drain the MCP request channel: the agentctl instance serves /mcp and // proxies tool calls to the backend over this stream, so without it kandev MCP diff --git a/apps/backend/internal/agent/runtime/lifecycle/types.go b/apps/backend/internal/agent/runtime/lifecycle/types.go index f11833e0d9..c06944b6de 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/types.go +++ b/apps/backend/internal/agent/runtime/lifecycle/types.go @@ -107,11 +107,21 @@ type AgentExecution struct { thinkingBuffer strings.Builder messageMu sync.Mutex - // Streaming message tracking - IDs of the current in-progress messages being streamed - // These are set when we create a streaming message and cleared on tool_call/complete + // Legacy streaming message tracking for agents that omit protocol message IDs. + // These are set when we create a streaming message and cleared on tool_call/complete. currentMessageID string currentThinkingID string + // Protocol message correlation maps source-protocol IDs to Kandev message + // record IDs. Assistant and thought IDs use separate namespaces so an agent + // reusing the same source ID cannot merge visible and reasoning content. + protocolMessageIDs map[string]string + protocolThinkingIDs map[string]string + // assistantHistoryBuffer accumulates assistant chunks in wire order for + // history-context injection. Tool and completion boundaries persist it as + // one segment before recording the boundary event. + assistantHistoryBuffer strings.Builder + // History-based context injection for agents without native session resume (e.g. Auggie). // historyEnabled gates recording and injection; set from SessionConfig.HistoryContextInjection. // needsResumeContext is set to true when the session has history that should be injected. @@ -181,9 +191,16 @@ func (e *AgentExecution) officeProfileID() string { // PromptCompletionSignal carries the result from a complete event or disconnect. type PromptCompletionSignal struct { - StopReason string - IsError bool - Error string + StopReason string + IsError bool + Error string + PromptGeneration uint64 +} + +func (e *AgentExecution) promptGenerationSnapshot() uint64 { + e.promptLifecycleMu.Lock() + defer e.promptLifecycleMu.Unlock() + return e.promptGeneration } // GetAgentCtlClient returns the agentctl client for this execution diff --git a/apps/backend/internal/agentctl/AGENTS.md b/apps/backend/internal/agentctl/AGENTS.md index 3ab2282d60..4615c51cfe 100644 --- a/apps/backend/internal/agentctl/AGENTS.md +++ b/apps/backend/internal/agentctl/AGENTS.md @@ -71,9 +71,9 @@ Env knobs (all optional): `KANDEV_DEBUG_ACP_MAX_FILES` (default 200), `KANDEV_DE **PRIVACY / PERF:** frames carry the full prompt, file, and tool-call content. Keep this strictly behind the debug flag and local-dev-scoped — never enable in a shared/production deployment. When agentctl runs inside a Docker executor the files land *inside the container*, so this is meant for standalone/dev. -### Recognizing claude-acp meta-tagged tools +### Recognizing agent-specific ACP meta-tagged tools -`claude-agent-acp` tags certain tool calls with `_meta.claudeCode.toolName` (e.g. `Monitor`, `ScheduleWakeup`, `Agent` for subagents) and may carry results in `_meta.claudeCode.toolResponse`. These are normalized into typed `streams.NormalizedPayload` kinds in `server/adapter/transport/acp/`. The established pattern is **one file per recognized tool** (`monitor.go`, `wakeup.go`, `subagent.go`), each with a defensive untyped-map recognizer (`recognize*`/`is*Meta`/`extract*`), a typed payload, and a sibling `*_test.go`. `convertToolCallUpdate` stashes `title`/`Meta` into the normalizer args; result enrichment happens in `convertToolCallResultUpdate`. To add another claude-acp meta-tool, copy that shape — don't inline detection in `adapter.go`. Detection can also be cross-agent: subagent recognition keys off Claude's `_meta`, OpenCode's tool `title`, and Cursor's `rawInput._toolName`. +`claude-agent-acp` tags certain tool calls with `_meta.claudeCode.toolName` (e.g. `Monitor`, `ScheduleWakeup`, `Agent` for subagents) and may carry results in `_meta.claudeCode.toolResponse`. `codex-acp` reports collaboration and subagent lifecycle through `_meta.codex`. These are normalized into typed `streams.NormalizedPayload` kinds in `server/adapter/transport/acp/`. The established pattern is a defensive untyped-map recognizer, typed normalized data, and sibling tests. Shared recognized tools live in files such as `monitor.go`, `wakeup.go`, and `subagent.go`; agent-scoped wire translation belongs in `dialect_.go` (for example `dialect_codex.go`). `convertToolCallUpdate` stashes `title`/`Meta` into the normalizer args; result enrichment happens in `convertToolCallResultUpdate`. Don't inline detection in `adapter.go`. Detection can also be cross-agent: common subagent recognition keys off Claude's `_meta`, OpenCode's tool `title`, Cursor's `rawInput._toolName`, and Auggie's title prefix. ### Subagent tool-call nesting: what each agent emits @@ -82,10 +82,11 @@ Kandev renders subagents (the `Task` tool) as cards and *wants* to nest each sub | Agent | Subagent-internal tool calls | Nestable? | |---|---|---| | **Claude** | emitted on the parent session, each tagged with **`_meta.claudeCode.parentToolUseId`** = the parent Task tool_call's id | **Yes** — `parentToolUseID()` reads it in `adapter_tools.go` and sets `AgentEvent.ParentToolCallID`, which becomes the message's `parent_tool_call_id` and nests under the card | +| **Codex** | **not emitted over ACP**; `codex-acp` emits top-level collaboration and lifecycle tool calls with `_meta.codex.collaboration` / `_meta.codex.subagent` | No — spawn/start signals render through `subagent_task` and preserve the child thread ID, but there are no child internal tool calls to attach | | **Cursor** | **not emitted over ACP at all** (Task `tool_call_update` carries only `{durationMs, isBackground}`) | No — no child data exists | | **OpenCode** | emitted into a **separate child ACP session** (the `metadata.sessionId` we store as `SubagentTaskPayload.ChildSessionID`) | Not yet — they never reach the parent stream; would require merging that child session via the stored `child_session_id` | -Claude is the one that works today: `claude-agent-acp` (since PR #341) sets `_meta.claudeCode.parentToolUseId` on a subagent's internal calls, and its value already equals the parent Task tool_call id — so it maps straight onto our `parent_tool_call_id`. Cursor exposes nothing to nest. OpenCode needs a kandev-side child-session merge. (Top-level `parentToolCallId` is NOT in the ACP schema — `_meta` is the spec-compliant carrier.) +Claude is the one that supports nested child tools today: `claude-agent-acp` (since PR #859) sets `_meta.claudeCode.parentToolUseId` on a subagent's internal calls, and its value already equals the parent Task tool_call id — so it maps straight onto our `parent_tool_call_id`. Codex reports child lifecycle but not child tools. Cursor exposes nothing to nest. OpenCode needs a Kandev-side child-session merge. (Top-level `parentToolCallId` is NOT in the ACP schema — `_meta` is the spec-compliant carrier.) ## Process-group cleanup and `RequiresProcessKill` diff --git a/apps/backend/internal/agentctl/server/acp/authmethod.go b/apps/backend/internal/agentctl/server/acp/authmethod.go index 5446f651bd..cf62de7715 100644 --- a/apps/backend/internal/agentctl/server/acp/authmethod.go +++ b/apps/backend/internal/agentctl/server/acp/authmethod.go @@ -9,11 +9,11 @@ import "github.com/coder/acp-go-sdk" func AuthMethodFields(m acp.AuthMethod) (id, name string, description *string, meta map[string]any) { switch { case m.Agent != nil: - return m.Agent.Id, m.Agent.Name, m.Agent.Description, m.Agent.Meta + return string(m.Agent.Id), m.Agent.Name, m.Agent.Description, m.Agent.Meta case m.Terminal != nil: - return m.Terminal.Id, m.Terminal.Name, m.Terminal.Description, m.Terminal.Meta + return string(m.Terminal.Id), m.Terminal.Name, m.Terminal.Description, m.Terminal.Meta case m.EnvVar != nil: - return m.EnvVar.Id, m.EnvVar.Name, m.EnvVar.Description, m.EnvVar.Meta + return string(m.EnvVar.Id), m.EnvVar.Name, m.EnvVar.Description, m.EnvVar.Meta } return "", "", nil, nil } diff --git a/apps/backend/internal/agentctl/server/acp/client.go b/apps/backend/internal/agentctl/server/acp/client.go index ac644dbce7..55e8f6665c 100644 --- a/apps/backend/internal/agentctl/server/acp/client.go +++ b/apps/backend/internal/agentctl/server/acp/client.go @@ -390,16 +390,17 @@ func (c *Client) CreateTerminal(ctx context.Context, p acp.CreateTerminalRequest span.RecordError(err) return acp.CreateTerminalResponse{}, err } - return acp.CreateTerminalResponse{TerminalId: id}, nil + return acp.CreateTerminalResponse{TerminalId: acp.TerminalId(id)}, nil } // KillTerminal sends SIGTERM to a terminal's process. func (c *Client) KillTerminal(ctx context.Context, p acp.KillTerminalRequest) (acp.KillTerminalResponse, error) { _, span := shared.TraceProtocolRequest(ctx, shared.ProtocolACP, "", "request.kill_terminal") defer span.End() - span.SetAttributes(attribute.String("terminal_id", p.TerminalId)) + terminalID := string(p.TerminalId) + span.SetAttributes(attribute.String("terminal_id", terminalID)) - if err := c.terminals.Kill(p.TerminalId); err != nil { + if err := c.terminals.Kill(terminalID); err != nil { span.RecordError(err) return acp.KillTerminalResponse{}, err } @@ -410,9 +411,10 @@ func (c *Client) KillTerminal(ctx context.Context, p acp.KillTerminalRequest) (a func (c *Client) TerminalOutput(ctx context.Context, p acp.TerminalOutputRequest) (acp.TerminalOutputResponse, error) { _, span := shared.TraceProtocolRequest(ctx, shared.ProtocolACP, "", "request.terminal_output") defer span.End() - span.SetAttributes(attribute.String("terminal_id", p.TerminalId)) + terminalID := string(p.TerminalId) + span.SetAttributes(attribute.String("terminal_id", terminalID)) - output, truncated, exitCode, signal, err := c.terminals.Output(p.TerminalId) + output, truncated, exitCode, signal, err := c.terminals.Output(terminalID) if err != nil { span.RecordError(err) return acp.TerminalOutputResponse{}, err @@ -435,9 +437,10 @@ func (c *Client) TerminalOutput(ctx context.Context, p acp.TerminalOutputRequest func (c *Client) ReleaseTerminal(ctx context.Context, p acp.ReleaseTerminalRequest) (acp.ReleaseTerminalResponse, error) { _, span := shared.TraceProtocolRequest(ctx, shared.ProtocolACP, "", "request.release_terminal") defer span.End() - span.SetAttributes(attribute.String("terminal_id", p.TerminalId)) + terminalID := string(p.TerminalId) + span.SetAttributes(attribute.String("terminal_id", terminalID)) - _ = c.terminals.Release(p.TerminalId) + _ = c.terminals.Release(terminalID) return acp.ReleaseTerminalResponse{}, nil } @@ -445,9 +448,10 @@ func (c *Client) ReleaseTerminal(ctx context.Context, p acp.ReleaseTerminalReque func (c *Client) WaitForTerminalExit(ctx context.Context, p acp.WaitForTerminalExitRequest) (acp.WaitForTerminalExitResponse, error) { _, span := shared.TraceProtocolRequest(ctx, shared.ProtocolACP, "", "request.wait_for_terminal_exit") defer span.End() - span.SetAttributes(attribute.String("terminal_id", p.TerminalId)) + terminalID := string(p.TerminalId) + span.SetAttributes(attribute.String("terminal_id", terminalID)) - exitCode, signal, err := c.terminals.WaitForExit(p.TerminalId) + exitCode, signal, err := c.terminals.WaitForExit(terminalID) if err != nil { span.RecordError(err) return acp.WaitForTerminalExitResponse{}, err diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go index 4d400fa886..e8d7ee1843 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter.go @@ -170,6 +170,16 @@ type Adapter struct { // Tool call tracking for result normalization // Maps toolCallId -> NormalizedPayload so we can update with results activeToolCalls map[string]*streams.NormalizedPayload + // codexSubagentCorrelations deduplicates the collaboration and activity + // tool_call frames codex-acp emits for one logical child, keyed by session, + // wire tool-call ID, and child session ID. Incomplete entries are retained + // until completion or session cleanup so delayed lifecycle frames cannot + // split a live card; only completed tombstones are bounded. Emitted-ID + // reservations are intentionally retained for the session so a pruned + // tombstone's ID cannot be reused by a later card. + codexSubagentCorrelations map[codexSubagentCorrelationKey]*codexSubagentCorrelation + codexEmittedToolCallIDs map[string]map[string]*codexSubagentCorrelation + codexSubagentSequence uint64 // Active Monitor tools, keyed by sessionID -> taskID -> toolCallID. // Claude-acp's Monitor tool runs a background script that streams events @@ -202,13 +212,12 @@ type Adapter struct { // Used by SetModel to validate the requested model exists. availableModels []modelInfo - // usageDelta tracks the running cumulative `usage_update.used` and - // the most recent USD cost reported per session. codex-acp emits no - // per-turn usage frame; the prompt-complete handler consumes the - // delta here when resp.Usage is empty and flags the row estimated. + // usageBySession tracks the latest and previously consumed cumulative + // `usage_update` samples. codex-acp emits no per-turn usage frame, so the + // prompt-complete handler uses nonnegative context-occupancy growth as an + // estimated input count and derives true deltas from cumulative USD cost. // claude-acp / opencode-acp report a real `result.usage` so this - // cache is only ever read for codex-acp turns. Reset to 0 once - // consumed so the next turn starts from a fresh delta. + // cache contributes only their provider-reported cost delta. usageBySession map[string]*usageTracker // Available auth methods captured from the ACP initialize response. @@ -532,6 +541,10 @@ func (a *Adapter) Close() error { // handleACPUpdate may call sendUpdate, so updatesCh must remain open // until the worker is gone. a.workerWg.Wait() + a.mu.Lock() + a.clearCodexSubagentCorrelationsLocked("") + clear(a.usageBySession) + a.mu.Unlock() // Clean up any saved attachments a.attachMgr.Cleanup() diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_helpers.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_helpers.go index 2e11baa537..3b610a1034 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_helpers.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_helpers.go @@ -8,10 +8,11 @@ import ( v1 "github.com/kandev/kandev/pkg/api/v1" ) -// derefStr safely dereferences a string pointer, returning empty string if nil. -func derefStr(s *string) string { +// derefStr safely dereferences a pointer to a string or named string type, +// returning empty string if nil. +func derefStr[T ~string](s *T) string { if s != nil { - return *s + return string(*s) } return "" } diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_prompt.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_prompt.go index 16fdeddd7b..a0b0559b34 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_prompt.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_prompt.go @@ -184,11 +184,11 @@ func (a *Adapter) sendPrompt( zap.String("stop_reason", stopReason)) a.cancelAsyncTurnComplete(sessionID) usage := a.dialect.promptUsage(extractUsage(&resp), resp.Meta) - // codex-acp emits no per-turn usage frame, only cumulative - // usage_update.used. Fall back to the inferred delta so the office - // cost subscriber sees at least an approximate input count. The - // resulting cost is off (no input/output split) — flagged via - // PromptUsage.Estimated so downstream rows carry estimated=true. + // codex-acp emits no per-turn usage frame, only cumulative context + // occupancy. Fall back to nonnegative occupancy growth so the office cost + // subscriber sees an approximate input count. It has no input/output split, + // so Estimated remains true. usage_update cost is cumulative session cost; + // consumeUsageDelta converts it to the current turn's nonnegative delta. delta, costSubcents := a.consumeUsageDelta(sessionID) if usage == nil { if delta > 0 || costSubcents > 0 { @@ -199,8 +199,8 @@ func (a *Adapter) sendPrompt( } } } else if costSubcents > 0 { - // claude-acp: usage_update.cost.amount carries the authoritative - // USD cost — attach it to the typed usage frame so Layer A wins + // claude-acp: usage_update.cost.amount carries authoritative cumulative + // USD cost — attach the derived turn delta so Layer A wins // downstream and the office cost subscriber stores the row // verbatim instead of falling back to models.dev. claude-acp's // model id is a logical alias (sonnet / haiku) that won't match diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session.go index 75e8a07c17..38d05cfa1f 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session.go @@ -31,6 +31,8 @@ func (a *Adapter) NewSession(ctx context.Context, mcpServers []types.McpServer) // a stale entry between the two operations. a.mu.Lock() a.pendingWakeups = make(map[string]*pendingWakeup) + a.clearCodexSubagentCorrelationsLocked("") + clear(a.usageBySession) a.wakeup.cancel() a.mu.Unlock() a.cancelAllAsyncTurnCompletes() @@ -45,6 +47,12 @@ func (a *Adapter) NewSession(ctx context.Context, mcpServers []types.McpServer) McpServers: toACPMcpServers(filteredServers), }) if err != nil { + // An agent may have emitted provisional usage before returning an RPC + // error. Clear at the FIFO boundary so an already queued notification + // cannot repopulate the tracker after this failure cleanup. + if !a.syncNotifQueueThen(a.clearUsageTrackers) { + a.clearUsageTrackers() + } span.RecordError(err) if a.maybeEmitAuthRequired(err) { return "", fmt.Errorf("authentication required: %w", err) @@ -52,9 +60,22 @@ func (a *Adapter) NewSession(ctx context.Context, mcpServers []types.McpServer) return "", fmt.Errorf("failed to create session: %w", err) } + sessionID := string(resp.SessionId) + if !a.syncNotifQueueThen(func() { + a.mu.Lock() + a.retainConsumedUsageBaselineLocked(sessionID) + a.mu.Unlock() + }) { + a.clearUsageTrackers() + barrierErr := context.Cause(a.lifetimeCtx) + if barrierErr == nil { + barrierErr = context.Canceled + } + return "", fmt.Errorf("failed to synchronize new session notifications: %w", barrierErr) + } + a.mu.Lock() - a.sessionID = string(resp.SessionId) - sessionID := a.sessionID + a.sessionID = sessionID a.configGeneration++ clear(a.contextSamples) // Reset session-scoped model caches before computing the new session's @@ -282,6 +303,7 @@ func (a *Adapter) LoadSession(ctx context.Context, sessionID string, mcpServers // accumulating stale pendingWakeups entries across reloads. a.mu.Lock() a.pendingWakeups = make(map[string]*pendingWakeup) + a.clearCodexSubagentCorrelationsLocked("") a.wakeup.cancel() a.mu.Unlock() a.cancelAllAsyncTurnCompletes() @@ -299,6 +321,8 @@ func (a *Adapter) LoadSession(ctx context.Context, sessionID string, mcpServers // The flag will be cleared when we send the next prompt (see Prompt method). a.mu.Lock() a.isLoadingSession = true + a.loadReplayPlan = nil + delete(a.usageBySession, sessionID) a.mu.Unlock() resp, err := conn.LoadSession(ctx, acp.LoadSessionRequest{ @@ -308,14 +332,39 @@ func (a *Adapter) LoadSession(ctx context.Context, sessionID string, mcpServers }) if err != nil { + // Notifications sent before the failed RPC response may still be queued + // on the adapter worker. Clear replay suppression only at a FIFO barrier, + // otherwise those frames can escape as live messages, tools, or plans + // after LoadSession returns its error. + clearFailedLoad := func() { + a.mu.Lock() + a.isLoadingSession = false + a.loadReplayPlan = nil + a.mu.Unlock() + } + if !a.syncNotifQueueThen(clearFailedLoad) { + // Close cancels lifetimeCtx and drops the remaining queue. The + // barrier callback will not run in that case, so clean up directly. + clearFailedLoad() + } span.RecordError(err) return fmt.Errorf("failed to load session: %w", err) } + // The SDK may finish the load RPC while replay notifications are still + // queued in the adapter worker. Keep suppression active until a FIFO barrier + // proves every replay frame has been processed, then mark replayed cumulative + // usage/cost as the baseline for the first new prompt. + a.syncNotifQueue() + a.mu.Lock() a.sessionID = sessionID a.configGeneration++ clear(a.contextSamples) + a.consumeUsageBaselineLocked(sessionID) + replayPlan := a.loadReplayPlan + a.loadReplayPlan = nil + a.isLoadingSession = false // Reset session-scoped model caches so a load that lands on a session // without a model surface can't reuse the previous session's data. a.availableModels = nil @@ -341,17 +390,6 @@ func (a *Adapter) LoadSession(ctx context.Context, sessionID string, mcpServers a.emitSessionModels(sessionID, initialModels, resp.Meta, resp.ConfigOptions) } - // Re-emit plan captured during history replay and clear the loading flag. - // The ACP SDK guarantees all replay notifications are processed before - // LoadSession returns (via notificationWg.Wait), so captured state is complete. - // Clearing isLoadingSession here allows post-replay notifications (e.g. - // AvailableCommandsUpdate "ready" signals) to pass through normally. - a.mu.Lock() - replayPlan := a.loadReplayPlan - a.loadReplayPlan = nil - a.isLoadingSession = false - a.mu.Unlock() - // Any Monitor still tracked at this point was running in pre-restart history // but has no live process to back it now — emit synthetic cancellations so // the frontend doesn't render a stuck "watching" card. @@ -1022,7 +1060,7 @@ func (a *Adapter) Authenticate(ctx context.Context, methodID string) error { } _, err := conn.Authenticate(ctx, acp.AuthenticateRequest{ - MethodId: methodID, + MethodId: acp.AuthMethodId(methodID), }) if err != nil { return fmt.Errorf("authenticate failed: %w", err) diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session_test.go index 98150d5c3e..6dca031d90 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_session_test.go @@ -40,6 +40,10 @@ func (*sessionRequestCaptureAgent) CloseSession(context.Context, acpsdk.CloseSes return acpsdk.CloseSessionResponse{}, nil } +func (*sessionRequestCaptureAgent) DeleteSession(context.Context, acpsdk.DeleteSessionRequest) (acpsdk.DeleteSessionResponse, error) { + return acpsdk.DeleteSessionResponse{}, nil +} + func (*sessionRequestCaptureAgent) ListSessions(context.Context, acpsdk.ListSessionsRequest) (acpsdk.ListSessionsResponse, error) { return acpsdk.ListSessionsResponse{}, nil } diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_tools.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_tools.go index 3d2e99e9b5..c3fdd5ed27 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_tools.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_tools.go @@ -1,12 +1,36 @@ package acp import ( + "encoding/base64" + "strconv" + "github.com/coder/acp-go-sdk" "github.com/kandev/kandev/internal/agentctl/server/adapter/transport/shared" "github.com/kandev/kandev/internal/agentctl/types/streams" "go.uber.org/zap" ) +const ( + maxCodexCompletedSubagentCorrelations = 256 + codexSubagentRunningStatus = "running" +) + +type codexSubagentCorrelationKey struct { + sessionID string + toolCallID string + childSessionID string + occurrence uint64 +} + +type codexSubagentCorrelation struct { + emittedToolCallID string + parentToolCallID string + payload *streams.NormalizedPayload + collaborationSeen bool + activitySeen bool + lastSeen uint64 +} + // convertToolCallContents converts ACP ToolCallContent items to our protocol-agnostic type. func (a *Adapter) convertToolCallContents(contents []acp.ToolCallContent) []streams.ToolCallContentItem { if len(contents) == 0 { @@ -33,7 +57,7 @@ func (a *Adapter) convertToolCallContents(contents []acp.ToolCallContent) []stre case c.Terminal != nil: items = append(items, streams.ToolCallContentItem{ Type: "terminal", - TerminalID: c.Terminal.TerminalId, + TerminalID: string(c.Terminal.TerminalId), }) } } @@ -106,9 +130,13 @@ func (a *Adapter) convertToolCallUpdate(sessionID string, tc *acp.SessionUpdateT normalizedPayload := a.normalizer.NormalizeToolCall(toolKind, args) toolCallID := string(tc.ToolCallId) - a.mu.Lock() - a.activeToolCalls[toolCallID] = normalizedPayload - a.mu.Unlock() + normalizedPayload, eventType, codexSignal, emittedToolCallID, codexParentToolCallID := a.trackToolCallPayload( + sessionID, + toolCallID, + normalizedPayload, + tc.Meta, + string(tc.Status), + ) // ScheduleWakeup tracking: meta carries `_meta.claudeCode.toolName` // on the initial tool_call; rawInput is usually empty here but record @@ -123,12 +151,19 @@ func (a *Adapter) convertToolCallUpdate(sessionID string, tc *acp.SessionUpdateT if status == "" { status = toolStatusInProgress } + if codexSignal == codexSubagentSignalActivity { + status = codexActivityToolStatus(normalizedPayload) + } + parentToolCallID := parentToolUseID(tc.Meta) + if codexParentToolCallID != "" { + parentToolCallID = codexParentToolCallID + } return &AgentEvent{ - Type: streams.EventTypeToolCall, + Type: eventType, SessionID: sessionID, - ToolCallID: toolCallID, - ParentToolCallID: parentToolUseID(tc.Meta), + ToolCallID: emittedToolCallID, + ParentToolCallID: parentToolCallID, ToolName: toolKind, // Kind is effectively the tool name ToolTitle: tc.Title, ToolStatus: status, @@ -137,6 +172,405 @@ func (a *Adapter) convertToolCallUpdate(sessionID string, tc *acp.SessionUpdateT } } +func (a *Adapter) trackToolCallPayload( + sessionID string, + toolCallID string, + payload *streams.NormalizedPayload, + meta map[string]any, + status string, +) (*streams.NormalizedPayload, string, codexSubagentSignal, string, string) { + eventType := streams.EventTypeToolCall + signal := codexSubagentSignalNone + if a.agentID == codexAgentID { + signal = codexSubagentSignalFromMeta(meta) + } + if signal == codexSubagentSignalCollaboration && payload != nil && payload.SubagentTask() != nil && payload.SubagentTask().Status == "" { + fillCodexStatus(payload.SubagentTask(), status) + } + + a.mu.Lock() + defer a.mu.Unlock() + if signal == codexSubagentSignalNone { + a.activeToolCalls[toolCallID] = payload + return payload, eventType, signal, toolCallID, "" + } + + correlation, duplicate := a.correlateCodexSubagentToolCallLocked( + sessionID, + toolCallID, + payload, + signal, + codexSenderThreadID(meta), + ) + if !duplicate { + a.activeToolCalls[correlation.emittedToolCallID] = correlation.payload + return cloneSubagentPayload(correlation.payload), eventType, signal, + correlation.emittedToolCallID, correlation.parentToolCallID + } + if active := a.activeToolCalls[correlation.emittedToolCallID]; sameCodexSubagentChild(active, correlation.payload) { + a.activeToolCalls[correlation.emittedToolCallID] = correlation.payload + } + return cloneSubagentPayload(correlation.payload), streams.EventTypeToolUpdate, signal, + correlation.emittedToolCallID, correlation.parentToolCallID +} + +func (a *Adapter) correlateCodexSubagentToolCallLocked( + sessionID string, + toolCallID string, + candidate *streams.NormalizedPayload, + signal codexSubagentSignal, + parentThreadID string, +) (*codexSubagentCorrelation, bool) { + if a.codexSubagentCorrelations == nil { + a.codexSubagentCorrelations = make(map[codexSubagentCorrelationKey]*codexSubagentCorrelation) + } + childSessionID := codexSubagentChildID(candidate) + key, correlation, found := a.findCodexSubagentCorrelationLocked(sessionID, toolCallID, childSessionID) + if found { + mergeCodexSubagentPayload(correlation.payload, candidate) + a.touchCodexSubagentCorrelationLocked(correlation, signal) + if childSessionID != "" && key.childSessionID == "" { + delete(a.codexSubagentCorrelations, key) + key.childSessionID = childSessionID + key.occurrence = 0 + a.codexSubagentCorrelations[key] = correlation + } + if correlation.parentToolCallID == "" { + correlation.parentToolCallID = a.codexParentToolCallIDLocked(sessionID, parentThreadID) + } + a.pruneCodexCompletedCorrelationsLocked() + return correlation, true + } + key = codexSubagentCorrelationKey{ + sessionID: sessionID, + toolCallID: toolCallID, + childSessionID: childSessionID, + } + correlation = &codexSubagentCorrelation{payload: cloneSubagentPayload(candidate)} + if _, occupied := a.codexSubagentCorrelations[key]; occupied { + key.occurrence = a.nextCodexCorrelationOccurrenceLocked(key) + } + correlation.emittedToolCallID = a.allocateCodexEmittedToolCallIDLocked( + sessionID, + toolCallID, + childSessionID, + correlation, + ) + correlation.parentToolCallID = a.codexParentToolCallIDLocked(sessionID, parentThreadID) + a.touchCodexSubagentCorrelationLocked(correlation, signal) + a.codexSubagentCorrelations[key] = correlation + a.pruneCodexCompletedCorrelationsLocked() + return correlation, false +} + +func (a *Adapter) findCodexSubagentCorrelationLocked( + sessionID string, + toolCallID string, + childSessionID string, +) (codexSubagentCorrelationKey, *codexSubagentCorrelation, bool) { + exactKey := codexSubagentCorrelationKey{ + sessionID: sessionID, + toolCallID: toolCallID, + childSessionID: childSessionID, + } + if exact, ok := a.codexSubagentCorrelations[exactKey]; ok { + if childSessionID != "" { + return exactKey, exact, true + } + if a.codexCorrelationSiblingCountLocked(sessionID, toolCallID) == 1 { + return exactKey, exact, true + } + return codexSubagentCorrelationKey{}, nil, false + } + + // A frame with a known child may adopt one earlier child-less frame only + // when it is the sole, incomplete correlation for this tool ID. A + // child-less frame may match the sole known child even after completion: + // codex-acp can replay or overlap one side of the pair without its child + // ID. Additional siblings make that identity ambiguous, so the caller + // creates a conservative standalone card. + var candidateKey codexSubagentCorrelationKey + var candidate *codexSubagentCorrelation + count := 0 + for key, correlation := range a.codexSubagentCorrelations { + if key.sessionID != sessionID || key.toolCallID != toolCallID { + continue + } + count++ + candidateKey = key + candidate = correlation + } + if count == 1 { + if childSessionID == "" { + return candidateKey, candidate, true + } + if candidateKey.childSessionID == "" && !codexCorrelationComplete(candidate) { + return candidateKey, candidate, true + } + } + return codexSubagentCorrelationKey{}, nil, false +} + +func (a *Adapter) codexCorrelationSiblingCountLocked(sessionID, toolCallID string) int { + count := 0 + for key := range a.codexSubagentCorrelations { + if key.sessionID == sessionID && key.toolCallID == toolCallID { + count++ + } + } + return count +} + +func codexCorrelationComplete(correlation *codexSubagentCorrelation) bool { + return correlation != nil && correlation.collaborationSeen && correlation.activitySeen +} + +func codexSubagentChildID(payload *streams.NormalizedPayload) string { + if payload == nil || payload.SubagentTask() == nil { + return "" + } + return payload.SubagentTask().ChildSessionID +} + +func sameCodexSubagentChild(left, right *streams.NormalizedPayload) bool { + leftID, rightID := codexSubagentChildID(left), codexSubagentChildID(right) + return left != nil && right != nil && leftID == rightID +} + +func (a *Adapter) touchCodexSubagentCorrelationLocked( + correlation *codexSubagentCorrelation, + signal codexSubagentSignal, +) { + a.codexSubagentSequence++ + correlation.lastSeen = a.codexSubagentSequence + switch signal { + case codexSubagentSignalCollaboration: + correlation.collaborationSeen = true + case codexSubagentSignalActivity: + correlation.activitySeen = true + } +} + +func (a *Adapter) evictCodexSubagentCorrelationLocked() { + key, found := a.oldestCodexSubagentCorrelationLocked() + if found { + delete(a.codexSubagentCorrelations, key) + } +} + +func (a *Adapter) oldestCodexSubagentCorrelationLocked() (codexSubagentCorrelationKey, bool) { + var oldestKey codexSubagentCorrelationKey + var oldestSequence uint64 + found := false + for key, correlation := range a.codexSubagentCorrelations { + if !codexCorrelationComplete(correlation) { + continue + } + if !found || correlation.lastSeen < oldestSequence { + oldestKey = key + oldestSequence = correlation.lastSeen + found = true + } + } + return oldestKey, found +} + +func (a *Adapter) pruneCodexCompletedCorrelationsLocked() { + for a.codexCompletedCorrelationCountLocked() > maxCodexCompletedSubagentCorrelations { + a.evictCodexSubagentCorrelationLocked() + } +} + +func (a *Adapter) codexCompletedCorrelationCountLocked() int { + count := 0 + for _, correlation := range a.codexSubagentCorrelations { + if codexCorrelationComplete(correlation) { + count++ + } + } + return count +} + +func (a *Adapter) nextCodexCorrelationOccurrenceLocked(key codexSubagentCorrelationKey) uint64 { + occurrence := a.codexSubagentSequence + 1 + for { + key.occurrence = occurrence + if _, occupied := a.codexSubagentCorrelations[key]; !occupied { + return occurrence + } + occurrence++ + } +} + +func (a *Adapter) allocateCodexEmittedToolCallIDLocked( + sessionID string, + wireToolCallID string, + childSessionID string, + correlation *codexSubagentCorrelation, +) string { + if a.codexEmittedToolCallIDs == nil { + a.codexEmittedToolCallIDs = make(map[string]map[string]*codexSubagentCorrelation) + } + reserved := a.codexEmittedToolCallIDs[sessionID] + if reserved == nil { + reserved = make(map[string]*codexSubagentCorrelation) + a.codexEmittedToolCallIDs[sessionID] = reserved + } + if _, exists := reserved[wireToolCallID]; !exists { + reserved[wireToolCallID] = correlation + return wireToolCallID + } + + encodedChild := base64.RawURLEncoding.EncodeToString([]byte(childSessionID)) + base := wireToolCallID + "~codex-subagent~" + strconv.Itoa(len(childSessionID)) + "-" + encodedChild + candidate := base + for suffix := 2; reserved[candidate] != nil; suffix++ { + candidate = base + "~" + strconv.Itoa(suffix) + } + reserved[candidate] = correlation + return candidate +} + +func (a *Adapter) codexParentToolCallIDLocked(sessionID, parentThreadID string) string { + if parentThreadID == "" { + return "" + } + var emittedID string + matches := 0 + for key, correlation := range a.codexSubagentCorrelations { + if key.sessionID != sessionID || codexSubagentChildID(correlation.payload) != parentThreadID { + continue + } + emittedID = correlation.emittedToolCallID + matches++ + } + if matches == 1 { + return emittedID + } + return "" +} + +func (a *Adapter) codexSubagentUpdateTargetLocked( + sessionID string, + wireToolCallID string, + meta map[string]any, + title string, + rawInput any, +) (*streams.NormalizedPayload, string, string, bool) { + payload := a.activeToolCalls[wireToolCallID] + if a.agentID != codexAgentID { + return payload, wireToolCallID, "", false + } + signal := codexSubagentSignalFromMeta(meta) + if signal == codexSubagentSignalNone { + return payload, wireToolCallID, "", false + } + frame, ok := parseCodexSubagentFrame(meta, title, rawInput) + if !ok { + return nil, "", "", true + } + _, correlation, found := a.findCodexSubagentCorrelationLocked( + sessionID, + wireToolCallID, + frame.result.ChildSessionID, + ) + if !found { + // A recognized Codex subagent update must never fall back to the bare + // wire ID. codex-acp may reuse that ID for several sibling children; + // without a unique child identity, mutating the active wire entry would + // arbitrarily patch or complete the first sibling. Suppress the frame + // until it can be correlated coherently. + return nil, "", "", true + } + a.touchCodexSubagentCorrelationLocked(correlation, signal) + a.pruneCodexCompletedCorrelationsLocked() + return correlation.payload, correlation.emittedToolCallID, correlation.parentToolCallID, false +} + +func (a *Adapter) clearCodexSubagentCorrelationsLocked(sessionID string) { + if sessionID == "" { + a.codexSubagentCorrelations = make(map[codexSubagentCorrelationKey]*codexSubagentCorrelation) + a.codexEmittedToolCallIDs = make(map[string]map[string]*codexSubagentCorrelation) + a.codexSubagentSequence = 0 + return + } + for key := range a.codexSubagentCorrelations { + if key.sessionID == sessionID { + delete(a.codexSubagentCorrelations, key) + } + } + delete(a.codexEmittedToolCallIDs, sessionID) +} + +func cloneSubagentPayload(payload *streams.NormalizedPayload) *streams.NormalizedPayload { + if payload == nil || payload.SubagentTask() == nil { + return payload + } + src := payload.SubagentTask() + clone := streams.NewSubagentTask(src.Description, src.Prompt, src.SubagentType) + dst := clone.SubagentTask() + dst.Status = src.Status + dst.AgentID = src.AgentID + dst.Model = src.Model + dst.ChildSessionID = src.ChildSessionID + dst.DurationMs = src.DurationMs + dst.TotalTokens = src.TotalTokens + if src.ToolUseCount != nil { + count := *src.ToolUseCount + dst.ToolUseCount = &count + } + dst.ResultText = src.ResultText + dst.IsAsync = src.IsAsync + dst.OutputFile = src.OutputFile + dst.CanReadOutputFile = src.CanReadOutputFile + dst.SetIsAuggie(src.IsAuggie()) + return clone +} + +func mergeCodexSubagentPayload(current, candidate *streams.NormalizedPayload) { + if current == nil || candidate == nil { + return + } + dst, src := current.SubagentTask(), candidate.SubagentTask() + if dst == nil || src == nil { + return + } + fillIfEmpty(&dst.Description, src.Description) + fillIfEmpty(&dst.Prompt, src.Prompt) + fillIfEmpty(&dst.SubagentType, src.SubagentType) + fillIfEmpty(&dst.Model, src.Model) + fillIfEmpty(&dst.ChildSessionID, src.ChildSessionID) + if codexSubagentStatusRank(src.Status) >= codexSubagentStatusRank(dst.Status) { + fillCodexStatus(dst, src.Status) + } +} + +func fillCodexStatus(payload *streams.SubagentTaskPayload, status string) { + if status != "" { + payload.Status = status + } +} + +func codexSubagentStatusRank(status string) int { + switch status { + case toolStatusCompleted, toolStatusComplete, "errored", "error", "interrupted", "shutdown", "notFound", toolStatusCancelled: + return 3 + case codexSubagentRunningStatus, "inProgress", toolStatusInProgress: + return 2 + case codexSubagentStarted, "pendingInit": + return 1 + default: + return 0 + } +} + +func codexActivityToolStatus(payload *streams.NormalizedPayload) string { + if payload != nil && payload.SubagentTask() != nil && codexSubagentStatusRank(payload.SubagentTask().Status) == 3 { + return toolStatusComplete + } + return toolStatusInProgress +} + // convertToolCallResultUpdate converts a ToolCallUpdate notification to an AgentEvent. // //nolint:gocognit,cyclop,funlen // pre-existing complexity preserved from adapter.go file split @@ -178,7 +612,21 @@ func (a *Adapter) convertToolCallResultUpdate(sessionID string, tcu *acp.Session supplemental := toolCallUpdateSupplemental(tcu) a.mu.Lock() - payload := a.activeToolCalls[toolCallID] + updateTitle := "" + if tcu.Title != nil { + updateTitle = *tcu.Title + } + payload, emittedToolCallID, codexParentToolCallID, suppressCodexUpdate := a.codexSubagentUpdateTargetLocked( + sessionID, + toolCallID, + tcu.Meta, + updateTitle, + tcu.RawInput, + ) + if suppressCodexUpdate { + a.mu.Unlock() + return nil + } monitorCommand := "" if payload != nil && (tcu.RawInput != nil || len(tcu.Locations) > 0) { a.normalizer.UpdatePayloadInput(payload, tcu.RawInput, supplemental) @@ -275,7 +723,7 @@ func (a *Adapter) convertToolCallResultUpdate(sessionID string, tcu *acp.Session } if isTerminal { - delete(a.activeToolCalls, toolCallID) + delete(a.activeToolCalls, emittedToolCallID) // Also drop tracked Monitor: this terminal update is the // agent-emitted close, so the prompt-end sweep must not re-emit a // "Monitor exited" event for this same toolCallID. @@ -283,6 +731,7 @@ func (a *Adapter) convertToolCallResultUpdate(sessionID string, tcu *acp.Session a.dropMonitorByToolCallIDLocked(sessionID, toolCallID) } } + emittedPayload := cloneSubagentPayload(payload) a.mu.Unlock() // ScheduleWakeup tracking: tool_call_update is where rawInput.prompt and @@ -325,15 +774,19 @@ func (a *Adapter) convertToolCallResultUpdate(sessionID string, tcu *acp.Session if tcu.Title != nil { title = *tcu.Title } + parentToolCallID := parentToolUseID(tcu.Meta) + if codexParentToolCallID != "" { + parentToolCallID = codexParentToolCallID + } return &AgentEvent{ Type: streams.EventTypeToolUpdate, SessionID: sessionID, - ToolCallID: toolCallID, - ParentToolCallID: parentToolUseID(tcu.Meta), + ToolCallID: emittedToolCallID, + ParentToolCallID: parentToolCallID, ToolTitle: title, ToolStatus: status, - NormalizedPayload: payload, + NormalizedPayload: emittedPayload, ToolCallContents: convertedContents, } } diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_updates.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_updates.go index 9eaf9f9ec7..df104139c2 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_updates.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/adapter_updates.go @@ -10,14 +10,14 @@ import ( "go.uber.org/zap" ) -// notifWork is the item type carried on notifQueue. Exactly one of the two -// fields is populated: notif for a real SDK notification (the common case), -// sync for a barrier posted by syncNotifQueue. The worker closes sync when -// it pops the barrier off the queue, releasing the waiter once everything -// queued ahead of it has been processed. +// notifWork is the item type carried on notifQueue. notif is populated for a +// real SDK notification (the common case); sync identifies a barrier and may +// carry afterBarrier state finalization. The worker closes sync after the +// callback, releasing the waiter once everything queued ahead has completed. type notifWork struct { notif acp.SessionNotification sync chan struct{} + afterBarrier func() promptGeneration uint64 } @@ -57,16 +57,25 @@ func (a *Adapter) enqueueACPUpdate(n acp.SessionNotification) { // reintroduce the very race this primitive exists to prevent (sweeps and // EventTypeComplete running while the worker still has queued frames). // Only adapter shutdown via lifetimeCtx can release the wait early. -func (a *Adapter) syncNotifQueue() { +func (a *Adapter) syncNotifQueue() bool { + return a.syncNotifQueueThen(nil) +} + +// syncNotifQueueThen runs afterBarrier on the update worker at the FIFO +// barrier boundary. This is useful when state must be finalized after all +// preceding notifications but before the worker can process later ones. +func (a *Adapter) syncNotifQueueThen(afterBarrier func()) bool { ch := make(chan struct{}) select { case <-a.lifetimeCtx.Done(): - return - case a.notifQueue <- notifWork{sync: ch}: + return false + case a.notifQueue <- notifWork{sync: ch, afterBarrier: afterBarrier}: } select { case <-a.lifetimeCtx.Done(): + return false case <-ch: + return true } } @@ -93,6 +102,9 @@ func (a *Adapter) runUpdateWorker() { return case item := <-a.notifQueue: if item.sync != nil { + if item.afterBarrier != nil && a.lifetimeCtx.Err() == nil { + item.afterBarrier() + } close(item.sync) continue } @@ -160,15 +172,11 @@ func (a *Adapter) handleACPUpdate( var event, leadingEvent *AgentEvent if !suppressed { event = a.convertNotification(n) - } - if event == nil && !suppressed { - // Try untyped updates not yet supported by the ACP SDK. A derived - // lifecycle event (session_info/usage frames that also imply a - // foreground-idle or background-complete transition) can carry a - // leading context-window event for the same provider frame; both - // follow the same log/trace/send path below instead of the context - // event being sent out-of-band inside the helper. - leadingEvent, event = a.tryConvertUntypedUpdate(rawData, sessionID, promptGeneration) + if n.Update.UsageUpdate != nil { + if lifecycleEvent := usageLifecycleEvent(sessionID, n.Update.UsageUpdate.Meta, promptGeneration); lifecycleEvent != nil { + leadingEvent, event = event, lifecycleEvent + } + } } if leadingEvent != nil { shared.LogNormalizedEvent(shared.ProtocolACP, a.agentID, sessionID, leadingEvent) @@ -241,17 +249,28 @@ func (a *Adapter) convertNotification(n acp.SessionNotification) *AgentEvent { switch { case u.AgentMessageChunk != nil: - return a.convertMessageChunk(sessionID, u.AgentMessageChunk.Content, "assistant") + return a.convertMessageChunkWithProtocolID( + sessionID, + u.AgentMessageChunk.Content, + "assistant", + derefStr(u.AgentMessageChunk.MessageId), + ) case u.UserMessageChunk != nil: - return a.convertMessageChunk(sessionID, u.UserMessageChunk.Content, "user") + return a.convertMessageChunkWithProtocolID( + sessionID, + u.UserMessageChunk.Content, + "user", + derefStr(u.UserMessageChunk.MessageId), + ) case u.AgentThoughtChunk != nil: if u.AgentThoughtChunk.Content.Text != nil { return &AgentEvent{ - Type: streams.EventTypeReasoning, - SessionID: sessionID, - ReasoningText: u.AgentThoughtChunk.Content.Text.Text, + Type: streams.EventTypeReasoning, + SessionID: sessionID, + ProtocolMessageID: derefStr(u.AgentThoughtChunk.MessageId), + ReasoningText: u.AgentThoughtChunk.Content.Text.Text, } } @@ -306,51 +325,32 @@ func (a *Adapter) convertNotification(n acp.SessionNotification) *AgentEvent { Data: map[string]any{"config_options_source": "provider_update"}, } } + + case u.SessionInfoUpdate != nil: + return &AgentEvent{ + Type: streams.EventTypeSessionInfo, + SessionID: sessionID, + SessionTitle: derefStr(u.SessionInfoUpdate.Title), + SessionUpdatedAt: derefStr(u.SessionInfoUpdate.UpdatedAt), + SessionMeta: u.SessionInfoUpdate.Meta, + } + + case u.UsageUpdate != nil: + return a.convertUsageUpdate(sessionID, u.UsageUpdate) } return nil } -// acpUsageUpdate represents the ACP "usage_update" session notification. -// TODO: Replace with acp.SessionUsageUpdate when the ACP SDK adds native support. -type acpUsageUpdate struct { - SessionUpdate string `json:"sessionUpdate"` - Size int64 `json:"size"` - Used int64 `json:"used"` - Meta struct { - ClaudeOrigin struct { - Kind string `json:"kind"` - } `json:"_claude/origin"` - } `json:"_meta,omitempty"` - Cost *struct { - Amount float64 `json:"amount"` - Currency string `json:"currency"` - } `json:"cost,omitempty"` -} - -// acpSessionInfoUpdate represents the ACP "session_info_update" notification. -// TODO: Replace with the SDK type when acp-go-sdk exposes it. -type acpSessionInfoUpdate struct { - SessionUpdate string `json:"sessionUpdate"` - Title string `json:"title,omitempty"` - UpdatedAt string `json:"updatedAt,omitempty"` - Meta map[string]any `json:"_meta,omitempty"` -} - -// usageTracker carries the running cumulative usage state for one ACP -// session — used to infer codex-acp's per-turn deltas. Reset when the -// prompt-complete handler consumes it; lastUsed sticks so subsequent -// updates compute deltas from the new baseline. -// -// lastCostSubcents is the most recent USD cost (Layer A from spec) in -// hundredths of a cent; pumped through to PromptUsage. -// ProviderReportedCostSubcents at consume time. The actual claude-acp -// frames carry a cumulative number, but we treat the most-recent value -// as the per-turn cost because claude-code already emits one cost -// snapshot per session/prompt cycle. +// usageTracker carries the latest cumulative ACP usage sample and the sample +// consumed at the previous prompt boundary. `used` is current context +// occupancy, so its nonnegative growth is only an estimate of turn input. Cost +// is cumulative session cost and is converted to a true per-turn delta. type usageTracker struct { - lastUsed int64 - lastCostSubcents int64 + latestUsed int64 + consumedUsed int64 + latestCostSubcents int64 + consumedCostSubcents int64 // maxSize is the largest context-window size reported for this session. // claude-acp's default model emits a 200K turn-start frame and a 1M // turn-end frame; sticky-max prevents the stale 200K from shrinking @@ -369,20 +369,49 @@ func (a *Adapter) ensureUsageTracker(sessionID string) *usageTracker { return tr } +func (a *Adapter) clearUsageTrackers() { + a.mu.Lock() + clear(a.usageBySession) + a.mu.Unlock() +} + +// retainConsumedUsageBaselineLocked drops stale session trackers while +// retaining the new session's sticky context size and latest sample. The +// retained sample is marked consumed so session-creation telemetry is not +// attributed to the first prompt. The caller must hold a.mu for writing. +func (a *Adapter) retainConsumedUsageBaselineLocked(sessionID string) { + tracker := a.usageBySession[sessionID] + clear(a.usageBySession) + if tracker == nil { + return + } + a.usageBySession[sessionID] = tracker + tracker.consumedUsed = tracker.latestUsed + tracker.consumedCostSubcents = tracker.latestCostSubcents +} + // recordUsageAndMaxSize updates per-session usage/cost tracking and returns -// the sticky-max context window size for the session. -func (a *Adapter) recordUsageAndMaxSize(sessionID string, size, used, costSubcents int64) int64 { +// the sticky-max context window size for the session. A decreasing cumulative +// value means compaction or a provider-side reset; advance both baselines so a +// later consume never emits a negative or stale delta. +func (a *Adapter) recordUsageAndMaxSize(sessionID string, size, used int64, cost *acp.Cost) int64 { a.mu.Lock() defer a.mu.Unlock() tr := a.ensureUsageTracker(sessionID) if size > tr.maxSize { tr.maxSize = size } - if used > tr.lastUsed { - tr.lastUsed = used + used = max(used, 0) + if used < tr.latestUsed { + tr.consumedUsed = used } - if costSubcents > 0 { - tr.lastCostSubcents = costSubcents + tr.latestUsed = used + if cost != nil && strings.EqualFold(cost.Currency, "USD") { + costSubcents := max(int64(cost.Amount*10000), 0) + if costSubcents < tr.latestCostSubcents { + tr.consumedCostSubcents = costSubcents + } + tr.latestCostSubcents = costSubcents } return tr.maxSize } @@ -398,9 +427,9 @@ func (a *Adapter) resetContextWindowMaxSize(sessionID string) { delete(a.contextSamples, sessionID) } -// consumeUsageDelta returns the delta of cumulative `used` since the -// last consumption + the most recent USD cost (subcents). Both fields -// are reset to zero after read so the next turn starts fresh. +// consumeUsageDelta returns nonnegative growth in context occupancy since the +// previous prompt boundary and the delta in cumulative USD session cost. Both +// consumed baselines advance to the latest observed sample. func (a *Adapter) consumeUsageDelta(sessionID string) (int64, int64) { a.mu.Lock() defer a.mu.Unlock() @@ -408,96 +437,54 @@ func (a *Adapter) consumeUsageDelta(sessionID string) (int64, int64) { if tr == nil { return 0, 0 } - delta := tr.lastUsed - cost := tr.lastCostSubcents - tr.lastUsed = 0 - tr.lastCostSubcents = 0 + delta := max(tr.latestUsed-tr.consumedUsed, 0) + cost := max(tr.latestCostSubcents-tr.consumedCostSubcents, 0) + tr.consumedUsed = tr.latestUsed + tr.consumedCostSubcents = tr.latestCostSubcents return delta, cost } -// tryConvertUntypedUpdate handles ACP session update types not yet supported by the SDK. -// When the SDK adds native support, move the handling into convertNotification and delete this. -// -// The second return value is a leading context-window event derived from the -// same provider frame as the primary (first) return value — populated only -// when a usage_update frame also implies a lifecycle transition (see -// usage.Meta.ClaudeOrigin.Kind below). Callers must log/trace/send both -// returned events on the normal handleACPUpdate path instead of this helper -// emitting the leading event itself, out of band. -func (a *Adapter) tryConvertUntypedUpdate( - rawNotification []byte, - sessionID string, - promptGeneration uint64, -) (*AgentEvent, *AgentEvent) { - var envelope struct { - Update json.RawMessage `json:"update"` - } - if err := json.Unmarshal(rawNotification, &envelope); err != nil { - return nil, nil +// consumeUsageBaselineLocked marks replayed usage and cost as historical so +// the first prompt after session/load starts from the restored cumulative +// sample. The caller must hold a.mu for writing. +func (a *Adapter) consumeUsageBaselineLocked(sessionID string) { + if tr := a.usageBySession[sessionID]; tr != nil { + tr.consumedUsed = tr.latestUsed + tr.consumedCostSubcents = tr.latestCostSubcents } +} - var sessionInfo acpSessionInfoUpdate - if err := json.Unmarshal(envelope.Update, &sessionInfo); err == nil && - sessionInfo.SessionUpdate == "session_info_update" { - return nil, &AgentEvent{ - Type: streams.EventTypeSessionInfo, - SessionID: sessionID, - SessionTitle: sessionInfo.Title, - SessionUpdatedAt: sessionInfo.UpdatedAt, - SessionMeta: sessionInfo.Meta, - } +func (a *Adapter) convertUsageUpdate(sessionID string, usage *acp.SessionUsageUpdate) *AgentEvent { + if usage == nil || usage.Size <= 0 { + return nil } - - var usage acpUsageUpdate - if err := json.Unmarshal(envelope.Update, &usage); err != nil { - return nil, nil - } - if usage.SessionUpdate != "usage_update" || usage.Size <= 0 { - return nil, nil - } - - // Forward usage_update.cost to the prompt-complete handler via the - // per-session tracker. claude-acp emits a USD float; we convert to - // hundredths-of-a-cent (subcents). The actual cost frame might be - // "cumulative-this-session"; the prompt-complete handler currently - // treats the value as the turn's cost — close enough for the - // budget+display surface today and revisited if claude-acp's - // semantics change. - var costSubcents int64 - if usage.Cost != nil { - costSubcents = int64(usage.Cost.Amount * 10000) - } - effectiveSize := a.recordUsageAndMaxSize(sessionID, usage.Size, usage.Used, costSubcents) - - remaining := max(effectiveSize-usage.Used, 0) - contextEvent := &AgentEvent{ + size := int64(usage.Size) + used := max(int64(usage.Used), 0) + effectiveSize := a.recordUsageAndMaxSize(sessionID, size, used, usage.Cost) + remaining := max(effectiveSize-used, 0) + return &AgentEvent{ Type: streams.EventTypeContextWindow, SessionID: sessionID, ContextWindowSize: effectiveSize, - ContextWindowUsed: usage.Used, + ContextWindowUsed: used, ContextWindowRemaining: remaining, - ContextEfficiency: float64(usage.Used) / float64(effectiveSize) * 100, + ContextEfficiency: float64(used) / float64(effectiveSize) * 100, } +} - var lifecycleType string - switch usage.Meta.ClaudeOrigin.Kind { +func usageLifecycleEvent(sessionID string, meta map[string]any, promptGeneration uint64) *AgentEvent { + origin, _ := meta["_claude/origin"].(map[string]any) + kind, _ := origin["kind"].(string) + var eventType string + switch kind { case "human": - lifecycleType = streams.EventTypeForegroundIdle + eventType = streams.EventTypeForegroundIdle case "task-notification": - lifecycleType = streams.EventTypeBackgroundComplete - } - if lifecycleType == "" { - return nil, contextEvent - } - // Preserve the context-window update carried by the same provider frame: - // return it as the leading event so the caller sends it first, ahead of - // the derived lifecycle event, maintaining provider order without - // overloading either type. - return contextEvent, &AgentEvent{ - Type: lifecycleType, - SessionID: sessionID, - PromptGeneration: promptGeneration, + eventType = streams.EventTypeBackgroundComplete + default: + return nil } + return &AgentEvent{Type: eventType, SessionID: sessionID, PromptGeneration: promptGeneration} } // convertMessageChunk converts an ACP ContentBlock to an AgentEvent, handling multimodal content. @@ -506,9 +493,19 @@ func (a *Adapter) tryConvertUntypedUpdate( // //nolint:nestif // pre-existing complexity preserved from adapter.go file split func (a *Adapter) convertMessageChunk(sessionID string, content acp.ContentBlock, role string) *AgentEvent { + return a.convertMessageChunkWithProtocolID(sessionID, content, role, "") +} + +func (a *Adapter) convertMessageChunkWithProtocolID( + sessionID string, + content acp.ContentBlock, + role string, + protocolMessageID string, +) *AgentEvent { event := &AgentEvent{ - Type: streams.EventTypeMessageChunk, - SessionID: sessionID, + Type: streams.EventTypeMessageChunk, + SessionID: sessionID, + ProtocolMessageID: protocolMessageID, } // Only set Role for user messages (assistant is the default) diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/conversion_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/conversion_test.go index 4826f5acbb..5ce3bbfb6d 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/conversion_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/conversion_test.go @@ -2,7 +2,6 @@ package acp import ( "encoding/json" - "fmt" "testing" "github.com/coder/acp-go-sdk" @@ -112,7 +111,7 @@ func drainEvents(a *Adapter) []AgentEvent { // --- derefStr --- func TestDerefStr_NilPointer(t *testing.T) { - result := derefStr(nil) + result := derefStr[string](nil) if result != "" { t.Errorf("derefStr(nil) = %q, want empty string", result) } @@ -512,6 +511,63 @@ func TestConvertToolCallContents_MixedItems(t *testing.T) { // --- convertMessageChunk --- +func TestConvertNotification_PreservesProtocolMessageIDs(t *testing.T) { + a := newTestAdapter() + tests := []struct { + name string + update acp.SessionUpdate + text string + role string + }{ + { + name: "assistant", + update: acp.SessionUpdate{AgentMessageChunk: &acp.SessionUpdateAgentMessageChunk{ + Content: acp.TextBlock("assistant text"), + MessageId: acp.Ptr(acp.MessageId("assistant-message")), + }}, + text: "assistant text", + }, + { + name: "user", + update: acp.SessionUpdate{UserMessageChunk: &acp.SessionUpdateUserMessageChunk{ + Content: acp.TextBlock("user text"), + MessageId: acp.Ptr(acp.MessageId("user-message")), + }}, + text: "user text", + role: "user", + }, + { + name: "thought", + update: acp.SessionUpdate{AgentThoughtChunk: &acp.SessionUpdateAgentThoughtChunk{ + Content: acp.TextBlock("thought text"), + MessageId: acp.Ptr(acp.MessageId("thought-message")), + }}, + text: "thought text", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + event := a.convertNotification(acp.SessionNotification{ + SessionId: "session-1", + Update: tt.update, + }) + if event == nil { + t.Fatal("expected converted event") + } + if event.ProtocolMessageID != tt.name+"-message" { + t.Errorf("ProtocolMessageID = %q, want %q", event.ProtocolMessageID, tt.name+"-message") + } + if event.Role != tt.role { + t.Errorf("Role = %q, want %q", event.Role, tt.role) + } + if event.Text != tt.text && event.ReasoningText != tt.text { + t.Errorf("converted text = (%q, %q), want %q", event.Text, event.ReasoningText, tt.text) + } + }) + } +} + func TestConvertMessageChunk_TextAssistant(t *testing.T) { a := newTestAdapter() cb := acp.TextBlock("Hello from assistant") @@ -868,13 +924,20 @@ func TestConvertAvailableCommands_NoDuplicates(t *testing.T) { } } -// --- tryConvertUntypedUpdate --- +// --- generated usage/session-info updates --- -func TestTryConvertUntypedUpdate_UsageUpdate(t *testing.T) { +func TestConvertNotification_UsageUpdateFixture(t *testing.T) { a := newTestAdapter() raw := []byte(`{"sessionId":"s1","update":{"sessionUpdate":"usage_update","size":200000,"used":56047,"cost":{"amount":9.76,"currency":"USD"}}}`) + var notification acp.SessionNotification + if err := json.Unmarshal(raw, ¬ification); err != nil { + t.Fatalf("unmarshal fixture: %v", err) + } + if notification.Update.UsageUpdate == nil { + t.Fatal("generated SDK did not decode usage_update") + } - _, result := a.tryConvertUntypedUpdate(raw, "s1", 0) + result := a.convertNotification(notification) if result == nil { t.Fatal("expected non-nil result for usage_update") @@ -900,54 +963,19 @@ func TestTryConvertUntypedUpdate_UsageUpdate(t *testing.T) { } } -func TestTryConvertUntypedUpdate_HumanOriginSignalsForegroundIdle(t *testing.T) { - a := newTestAdapter() - raw := []byte(`{"sessionId":"s1","update":{"sessionUpdate":"usage_update","size":1000000,"used":23638,"_meta":{"_claude/origin":{"kind":"human"}}}}`) - - _, event := a.tryConvertUntypedUpdate(raw, "s1", 0) - if event == nil { - t.Fatal("expected foreground-idle event") - } - if event.Type != "foreground_idle" { - t.Fatalf("event type = %q, want foreground_idle", event.Type) - } -} - -func TestTryConvertUntypedUpdate_HumanOriginPreservesPromptGeneration(t *testing.T) { - a := newTestAdapter() - raw := []byte(`{"sessionId":"s1","update":{"sessionUpdate":"usage_update","size":1000000,"used":23638,"_meta":{"_claude/origin":{"kind":"human"}}}}`) - - _, event := a.tryConvertUntypedUpdate(raw, "s1", 42) - if event == nil { - t.Fatal("expected foreground-idle event") - } - if event.PromptGeneration != 42 { - t.Fatalf("prompt generation = %d, want 42", event.PromptGeneration) - } -} - -func TestTryConvertUntypedUpdate_TaskNotificationSignalsOneBackgroundCompletion(t *testing.T) { - a := newTestAdapter() - raw := []byte(`{"sessionId":"s1","update":{"sessionUpdate":"usage_update","size":1000000,"used":24892,"_meta":{"_claude/origin":{"kind":"task-notification"}}}}`) - - _, event := a.tryConvertUntypedUpdate(raw, "s1", 0) - if event == nil { - t.Fatal("expected background-complete event") - } - if event.Type != "background_complete" { - t.Fatalf("event type = %q, want background_complete", event.Type) - } - if event.ToolCallID != "" { - t.Fatalf("task-notification unexpectedly invented work identity %q", event.ToolCallID) - } -} - -func TestTryConvertUntypedUpdate_SessionInfoUpdate(t *testing.T) { +func TestConvertNotification_SessionInfoUpdateFixture(t *testing.T) { a := newTestAdapter() t.Cleanup(func() { _ = a.Close() }) raw := []byte(`{"sessionId":"s1","update":{"sessionUpdate":"session_info_update","title":"Linux File Guide","updatedAt":"2026-06-13T19:37:46Z","_meta":{"cursor":{"requestId":"req-1"}}}}`) + var notification acp.SessionNotification + if err := json.Unmarshal(raw, ¬ification); err != nil { + t.Fatalf("unmarshal fixture: %v", err) + } + if notification.Update.SessionInfoUpdate == nil { + t.Fatal("generated SDK did not decode session_info_update") + } - _, result := a.tryConvertUntypedUpdate(raw, "s1", 0) + result := a.convertNotification(notification) if result == nil { t.Fatal("expected non-nil result for session_info_update") @@ -969,11 +997,10 @@ func TestTryConvertUntypedUpdate_SessionInfoUpdate(t *testing.T) { } } -func TestTryConvertUntypedUpdate_ZeroUsed(t *testing.T) { +func TestConvertUsageUpdate_ZeroUsed(t *testing.T) { a := newTestAdapter() - raw := []byte(`{"sessionId":"s1","update":{"sessionUpdate":"usage_update","size":200000,"used":0}}`) - _, result := a.tryConvertUntypedUpdate(raw, "s1", 0) + result := a.convertUsageUpdate("s1", usageUpdate(200_000, 0)) if result == nil { t.Fatal("expected non-nil result") @@ -986,11 +1013,10 @@ func TestTryConvertUntypedUpdate_ZeroUsed(t *testing.T) { } } -func TestTryConvertUntypedUpdate_UsedExceedsSize(t *testing.T) { +func TestConvertUsageUpdate_UsedExceedsSize(t *testing.T) { a := newTestAdapter() - raw := []byte(`{"sessionId":"s1","update":{"sessionUpdate":"usage_update","size":100000,"used":110000}}`) - _, result := a.tryConvertUntypedUpdate(raw, "s1", 0) + result := a.convertUsageUpdate("s1", usageUpdate(100_000, 110_000)) if result == nil { t.Fatal("expected non-nil result") @@ -1000,55 +1026,34 @@ func TestTryConvertUntypedUpdate_UsedExceedsSize(t *testing.T) { } } -func TestTryConvertUntypedUpdate_UnknownUpdateType(t *testing.T) { - a := newTestAdapter() - raw := []byte(`{"sessionId":"s1","update":{"sessionUpdate":"something_else","foo":"bar"}}`) - - _, result := a.tryConvertUntypedUpdate(raw, "s1", 0) - - if result != nil { - t.Errorf("expected nil for unknown update type, got %+v", result) - } -} - -func TestTryConvertUntypedUpdate_InvalidJSON(t *testing.T) { +func TestConvertUsageUpdate_ZeroSize(t *testing.T) { a := newTestAdapter() - _, result := a.tryConvertUntypedUpdate([]byte(`{invalid`), "s1", 0) - - if result != nil { - t.Errorf("expected nil for invalid JSON, got %+v", result) - } -} - -func TestTryConvertUntypedUpdate_ZeroSize(t *testing.T) { - a := newTestAdapter() - raw := []byte(`{"sessionId":"s1","update":{"sessionUpdate":"usage_update","size":0,"used":0}}`) - - _, result := a.tryConvertUntypedUpdate(raw, "s1", 0) + result := a.convertUsageUpdate("s1", usageUpdate(0, 0)) if result != nil { t.Errorf("expected nil for zero size (division by zero guard), got %+v", result) } } -func usageUpdateRaw(size, used int64) []byte { - return []byte(fmt.Sprintf( - `{"sessionId":"s1","update":{"sessionUpdate":"usage_update","size":%d,"used":%d}}`, - size, used, - )) +func usageUpdate(size, used int64) *acp.SessionUsageUpdate { + return &acp.SessionUsageUpdate{ + SessionUpdate: "usage_update", + Size: int(size), + Used: int(used), + } } -func TestTryConvertUntypedUpdate_StickyMaxSize(t *testing.T) { +func TestConvertUsageUpdate_StickyMaxSize(t *testing.T) { t.Run("default turn raises max from 200K to 1M", func(t *testing.T) { a := newTestAdapter() - _, first := a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 5_000), "s1", 0) + first := a.convertUsageUpdate("s1", usageUpdate(200_000, 5_000)) if first == nil || first.ContextWindowSize != 200_000 { t.Fatalf("first size = %d, want 200000", sizeOrZero(first)) } - _, second := a.tryConvertUntypedUpdate(usageUpdateRaw(1_000_000, 5_000), "s1", 0) + second := a.convertUsageUpdate("s1", usageUpdate(1_000_000, 5_000)) if second == nil || second.ContextWindowSize != 1_000_000 { t.Fatalf("second size = %d, want 1000000", sizeOrZero(second)) } @@ -1056,10 +1061,9 @@ func TestTryConvertUntypedUpdate_StickyMaxSize(t *testing.T) { t.Run("stale 200K start-frame cannot shrink after 1M end-frame", func(t *testing.T) { a := newTestAdapter() - _, _ = a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 5_000), "s1", 0) - _, _ = a.tryConvertUntypedUpdate(usageUpdateRaw(1_000_000, 5_000), "s1", 0) + _, _ = a.convertUsageUpdate("s1", usageUpdate(200_000, 5_000)), a.convertUsageUpdate("s1", usageUpdate(1_000_000, 5_000)) - _, stale := a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 233_900), "s1", 0) + stale := a.convertUsageUpdate("s1", usageUpdate(200_000, 233_900)) if stale == nil { t.Fatal("expected non-nil stale frame result") } @@ -1077,8 +1081,8 @@ func TestTryConvertUntypedUpdate_StickyMaxSize(t *testing.T) { t.Run("sonnet stays at 200K", func(t *testing.T) { a := newTestAdapter() - _, first := a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 5_000), "s1", 0) - _, second := a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 7_000), "s1", 0) + first := a.convertUsageUpdate("s1", usageUpdate(200_000, 5_000)) + second := a.convertUsageUpdate("s1", usageUpdate(200_000, 7_000)) if first.ContextWindowSize != 200_000 || second.ContextWindowSize != 200_000 { t.Fatalf("sizes = %d, %d; want 200000 both", first.ContextWindowSize, second.ContextWindowSize) } @@ -1086,7 +1090,7 @@ func TestTryConvertUntypedUpdate_StickyMaxSize(t *testing.T) { t.Run("sonnet[1m] is 1M from first frame", func(t *testing.T) { a := newTestAdapter() - _, result := a.tryConvertUntypedUpdate(usageUpdateRaw(1_000_000, 5_000), "s1", 0) + result := a.convertUsageUpdate("s1", usageUpdate(1_000_000, 5_000)) if result == nil || result.ContextWindowSize != 1_000_000 { t.Fatalf("size = %d, want 1000000", sizeOrZero(result)) } @@ -1094,10 +1098,9 @@ func TestTryConvertUntypedUpdate_StickyMaxSize(t *testing.T) { t.Run("sessions track max independently", func(t *testing.T) { a := newTestAdapter() - _, _ = a.tryConvertUntypedUpdate(usageUpdateRaw(1_000_000, 5_000), "s1", 0) - _, _ = a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 5_000), "s2", 0) - _, s1 := a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 10_000), "s1", 0) - _, s2 := a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 10_000), "s2", 0) + _, _ = a.convertUsageUpdate("s1", usageUpdate(1_000_000, 5_000)), a.convertUsageUpdate("s2", usageUpdate(200_000, 5_000)) + s1 := a.convertUsageUpdate("s1", usageUpdate(200_000, 10_000)) + s2 := a.convertUsageUpdate("s2", usageUpdate(200_000, 10_000)) if s1.ContextWindowSize != 1_000_000 { t.Errorf("s1 size = %d, want 1000000", s1.ContextWindowSize) } @@ -1108,11 +1111,10 @@ func TestTryConvertUntypedUpdate_StickyMaxSize(t *testing.T) { t.Run("reset after model switch allows downshift to 200K", func(t *testing.T) { a := newTestAdapter() - _, _ = a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 5_000), "s1", 0) - _, _ = a.tryConvertUntypedUpdate(usageUpdateRaw(1_000_000, 5_000), "s1", 0) + _, _ = a.convertUsageUpdate("s1", usageUpdate(200_000, 5_000)), a.convertUsageUpdate("s1", usageUpdate(1_000_000, 5_000)) a.resetContextWindowMaxSize("s1") - _, afterSwitch := a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 26_000), "s1", 0) + afterSwitch := a.convertUsageUpdate("s1", usageUpdate(200_000, 26_000)) if afterSwitch == nil || afterSwitch.ContextWindowSize != 200_000 { t.Fatalf("after switch size = %d, want 200000", sizeOrZero(afterSwitch)) } diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect.go index 6a1d4042df..e1acea0ba6 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect.go @@ -24,6 +24,7 @@ type acpDialect struct { suppressNotification func(acp.SessionNotification) bool contextWindow func(map[string]any, []modelInfo, []streams.ConfigOption) (contextWindowSample, bool) normalizePromptUsage func(*streams.PromptUsage, map[string]any) *streams.PromptUsage + subagentFrame func(map[string]any, string, any) (subagentFrame, bool) } type dialectConfigChange struct { @@ -45,12 +46,22 @@ type contextWindowSample struct { } func newACPDialect(agentID string) acpDialect { - if agentID == grokAgentID { + switch agentID { + case grokAgentID: return newGrokACPDialect() + case codexAgentID: + return newCodexACPDialect() } return acpDialect{} } +func (d acpDialect) parseSubagentFrame(meta map[string]any, title string, rawInput any) (subagentFrame, bool) { + if d.subagentFrame == nil { + return subagentFrame{}, false + } + return d.subagentFrame(meta, title, rawInput) +} + func (d acpDialect) sessionConfigOptions( meta map[string]any, options []acp.SessionConfigOption, diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_codex.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_codex.go new file mode 100644 index 0000000000..f09fd86f3a --- /dev/null +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_codex.go @@ -0,0 +1,198 @@ +package acp + +import ( + "path" + "strings" +) + +const ( + codexAgentID = "codex-acp" + codexCollaborationSpawnAgent = "spawnAgent" + codexSubagentStarted = "started" +) + +type codexSubagentSignal uint8 + +const ( + codexSubagentSignalNone codexSubagentSignal = iota + codexSubagentSignalCollaboration + codexSubagentSignalActivity +) + +// newCodexACPDialect translates codex-acp's implementation metadata into the +// provider-neutral subagent payload. codex-acp reports collaboration control +// operations through the same envelope, so only spawnAgent and the matching +// "started" activity are creation signals. +func newCodexACPDialect() acpDialect { + return acpDialect{subagentFrame: parseCodexSubagentFrame} +} + +func parseCodexSubagentFrame(meta map[string]any, _ string, rawInput any) (subagentFrame, bool) { + codex, ok := nestedMap(meta, "codex") + if !ok { + return subagentFrame{}, false + } + if collaboration, ok := nestedMap(codex, "collaboration"); ok && codexCollaborationSignal(collaboration) { + return parseCodexCollaboration(collaboration, rawInput) + } + if activity, ok := nestedMap(codex, "subagent"); ok && codexActivitySignal(activity) { + return parseCodexSubagentActivity(activity, rawInput) + } + return subagentFrame{}, false +} + +func codexSubagentSignalFromMeta(meta map[string]any) codexSubagentSignal { + codex, ok := nestedMap(meta, "codex") + if !ok { + return codexSubagentSignalNone + } + if collaboration, ok := nestedMap(codex, "collaboration"); ok && codexCollaborationSignal(collaboration) { + return codexSubagentSignalCollaboration + } + if activity, ok := nestedMap(codex, "subagent"); ok && codexActivitySignal(activity) { + return codexSubagentSignalActivity + } + return codexSubagentSignalNone +} + +func codexSenderThreadID(meta map[string]any) string { + codex, ok := nestedMap(meta, "codex") + if !ok { + return "" + } + collaboration, ok := nestedMap(codex, "collaboration") + if !ok { + return "" + } + senderThreadID, _ := collaboration["senderThreadId"].(string) + return senderThreadID +} + +func codexCollaborationSignal(collaboration map[string]any) bool { + tool, _ := collaboration["tool"].(string) + return tool == codexCollaborationSpawnAgent +} + +func codexActivitySignal(activity map[string]any) bool { + activityKind, _ := activity["activity"].(string) + return activityKind == codexSubagentStarted +} + +func parseCodexCollaboration(collaboration map[string]any, rawInput any) (subagentFrame, bool) { + if !codexCollaborationSignal(collaboration) { + return subagentFrame{}, false + } + + input, _ := rawInput.(map[string]any) + receivers := stringSlice(collaboration["receiverThreadIds"]) + if len(receivers) == 0 { + receivers = stringSlice(input["receiverThreadIds"]) + } + childID := firstString(receivers) + prompt, _ := input["prompt"].(string) + description, status := codexAgentState(input, childID) + if description == "" { + description = prompt + } + if status == "" { + status, _ = input["status"].(string) + } + model, _ := input["model"].(string) + + return subagentFrame{ + description: description, + prompt: prompt, + result: SubagentTaskResult{ + Status: status, + Model: model, + ChildSessionID: childID, + }, + }, true +} + +func parseCodexSubagentActivity(activity map[string]any, rawInput any) (subagentFrame, bool) { + activityKind, _ := activity["activity"].(string) + if !codexActivitySignal(activity) { + return subagentFrame{}, false + } + input, _ := rawInput.(map[string]any) + childID, _ := activity["threadId"].(string) + if childID == "" { + childID, _ = input["agentThreadId"].(string) + } + agentPath, _ := activity["path"].(string) + if agentPath == "" { + agentPath, _ = input["agentPath"].(string) + } + name := codexSubagentName(agentPath) + + return subagentFrame{ + description: name, + subagentType: name, + result: SubagentTaskResult{ + Status: activityKind, + ChildSessionID: childID, + }, + }, true +} + +func codexAgentState(input map[string]any, childID string) (message, status string) { + if input == nil || childID == "" { + return "", "" + } + states, ok := input["agentsStates"].(map[string]any) + if !ok { + return "", "" + } + state, ok := states[childID].(map[string]any) + if !ok { + return "", "" + } + message, _ = state["message"].(string) + status, _ = state["status"].(string) + return message, status +} + +func codexSubagentName(agentPath string) string { + cleaned := strings.TrimSpace(agentPath) + if cleaned == "" { + return "" + } + name := path.Base(cleaned) + if name == "." || name == "/" { + return "" + } + return name +} + +func nestedMap(parent map[string]any, key string) (map[string]any, bool) { + if parent == nil { + return nil, false + } + value, ok := parent[key].(map[string]any) + return value, ok +} + +func stringSlice(value any) []string { + switch values := value.(type) { + case []string: + return values + case []any: + result := make([]string, 0, len(values)) + for _, value := range values { + if item, ok := value.(string); ok && item != "" { + result = append(result, item) + } + } + return result + default: + return nil + } +} + +func firstString(values []string) string { + if len(values) == 0 { + return "" + } + return values[0] +} diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_codex_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_codex_test.go new file mode 100644 index 0000000000..f7210ae1b8 --- /dev/null +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/dialect_codex_test.go @@ -0,0 +1,139 @@ +package acp + +import "testing" + +func TestParseCodexSubagentFrameCollaboration(t *testing.T) { + meta := codexCollaborationMeta(codexCollaborationSpawnAgent, []any{"thread-child"}) + rawInput := map[string]any{ + "prompt": "Inspect the flaky test", + "model": "gpt-5.2-codex", + "reasoningEffort": "high", + "agentsStates": map[string]any{ + "thread-child": map[string]any{ + "status": "running", + "message": "Reading the test suite", + }, + }, + } + + frame, ok := parseCodexSubagentFrame(meta, "spawnAgent", rawInput) + if !ok { + t.Fatal("expected spawnAgent metadata to be recognized") + } + if frame.description != "Reading the test suite" { + t.Errorf("description = %q", frame.description) + } + if frame.prompt != "Inspect the flaky test" { + t.Errorf("prompt = %q", frame.prompt) + } + if frame.result.ChildSessionID != "thread-child" { + t.Errorf("ChildSessionID = %q", frame.result.ChildSessionID) + } + if frame.result.Status != "running" { + t.Errorf("Status = %q", frame.result.Status) + } + if frame.result.Model != "gpt-5.2-codex" { + t.Errorf("Model = %q", frame.result.Model) + } +} + +func TestParseCodexSubagentFrameSpawnFallbacks(t *testing.T) { + meta := codexCollaborationMeta(codexCollaborationSpawnAgent, nil) + rawInput := map[string]any{ + "prompt": "Review the adapter", + "receiverThreadIds": []string{"thread-from-input"}, + "status": "inProgress", + } + frame, ok := parseCodexSubagentFrame(meta, "", rawInput) + if !ok { + t.Fatal("expected partial spawnAgent metadata to be recognized") + } + if frame.description != "Review the adapter" { + t.Errorf("description = %q, want prompt fallback", frame.description) + } + if frame.result.ChildSessionID != "thread-from-input" { + t.Errorf("ChildSessionID = %q", frame.result.ChildSessionID) + } + if frame.result.Status != "inProgress" { + t.Errorf("Status = %q", frame.result.Status) + } +} + +func TestParseCodexSubagentFrameStartedActivity(t *testing.T) { + meta := map[string]any{"codex": map[string]any{"subagent": map[string]any{ + "threadId": "thread-child", + "path": "/root/review_agent", + "activity": codexSubagentStarted, + }}} + frame, ok := parseCodexSubagentFrame(meta, "Start subagent review_agent", nil) + if !ok { + t.Fatal("expected started activity to be recognized") + } + if frame.result.ChildSessionID != "thread-child" { + t.Errorf("ChildSessionID = %q", frame.result.ChildSessionID) + } + if frame.result.Status != codexSubagentStarted { + t.Errorf("Status = %q", frame.result.Status) + } + if frame.subagentType != "review_agent" || frame.description != "review_agent" { + t.Errorf("type/description = %q/%q", frame.subagentType, frame.description) + } +} + +func TestParseCodexSubagentFrameRejectsControlsAndMalformedMeta(t *testing.T) { + tests := []struct { + name string + meta map[string]any + }{ + {"interact", codexCollaborationMeta("interact", []any{"thread-child"})}, + {"wait", codexCollaborationMeta("wait", []any{"thread-child"})}, + {"close", codexCollaborationMeta("close", []any{"thread-child"})}, + {"interacted activity", codexActivityMeta("interacted")}, + {"interrupted activity", codexActivityMeta("interrupted")}, + {"missing codex", map[string]any{"other": map[string]any{}}}, + {"wrong codex type", map[string]any{"codex": "invalid"}}, + {"wrong collaboration type", map[string]any{"codex": map[string]any{"collaboration": 42}}}, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + if _, ok := parseCodexSubagentFrame(test.meta, "", nil); ok { + t.Fatal("metadata must not create a subagent") + } + }) + } +} + +func TestParseCodexSubagentFrameMalformedCollectionsAreSafe(t *testing.T) { + meta := codexCollaborationMeta(codexCollaborationSpawnAgent, "not-an-array") + rawInput := map[string]any{ + "receiverThreadIds": []any{42, nil}, + "agentsStates": []any{"not-an-object"}, + } + frame, ok := parseCodexSubagentFrame(meta, "", rawInput) + if !ok { + t.Fatal("the explicit spawnAgent signal should still be recognized") + } + if frame.result.ChildSessionID != "" || frame.result.Status != "" { + t.Errorf("malformed fields leaked into result: %+v", frame.result) + } +} + +func codexCollaborationMeta(tool string, receivers any) map[string]any { + return codexCollaborationMetaFrom(tool, "thread-main", receivers) +} + +func codexCollaborationMetaFrom(tool, sender string, receivers any) map[string]any { + return map[string]any{"codex": map[string]any{"collaboration": map[string]any{ + "tool": tool, + "senderThreadId": sender, + "receiverThreadIds": receivers, + }}} +} + +func codexActivityMeta(activity string) map[string]any { + return map[string]any{"codex": map[string]any{"subagent": map[string]any{ + "threadId": "thread-child", + "path": "/root/review_agent", + "activity": activity, + }}} +} diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/load_burst_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/load_burst_test.go index 9cbac1a60a..a20d9fe2c4 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/load_burst_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/load_burst_test.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "io" + "strings" "sync" "testing" "time" @@ -21,6 +22,7 @@ const ( var ( errReplayBurstProducerStalled = errors.New("replay burst producer stalled") errReplayBurstProducerBeforeWriteStalled = errors.New("replay burst producer stalled before writer entry") + errReplayLoadFailed = errors.New("replay load failed") ) // burstAgent is a minimal acp.Agent stub used by the load-replay regression @@ -40,6 +42,10 @@ func (burstAgent) CloseSession(context.Context, acp.CloseSessionRequest) (acp.Cl return acp.CloseSessionResponse{}, nil } +func (burstAgent) DeleteSession(context.Context, acp.DeleteSessionRequest) (acp.DeleteSessionResponse, error) { + return acp.DeleteSessionResponse{}, acp.NewMethodNotFound(acp.AgentMethodSessionDelete) +} + func (burstAgent) ListSessions(context.Context, acp.ListSessionsRequest) (acp.ListSessionsResponse, error) { return acp.ListSessionsResponse{}, nil } @@ -67,6 +73,47 @@ func (burstAgent) SetSessionMode(context.Context, acp.SetSessionModeRequest) (ac return acp.SetSessionModeResponse{}, nil } +type replayLoadAgent struct { + burstAgent + conn *acp.AgentSideConnection + loadErr error +} + +func (a *replayLoadAgent) Initialize(_ context.Context, req acp.InitializeRequest) (acp.InitializeResponse, error) { + return acp.InitializeResponse{ + ProtocolVersion: req.ProtocolVersion, + AgentCapabilities: acp.AgentCapabilities{ + LoadSession: true, + }, + }, nil +} + +func (a *replayLoadAgent) LoadSession(ctx context.Context, req acp.LoadSessionRequest) (acp.LoadSessionResponse, error) { + usage := usageUpdateWithCost(200_000, 100, 1.23, "USD") + if err := a.conn.SessionUpdate(ctx, acp.SessionNotification{ + SessionId: req.SessionId, + Update: acp.SessionUpdate{UsageUpdate: usage}, + }); err != nil { + return acp.LoadSessionResponse{}, err + } + if err := a.conn.SessionUpdate(ctx, acp.SessionNotification{ + SessionId: req.SessionId, + Update: acp.SessionUpdate{Plan: &acp.SessionUpdatePlan{ + Entries: []acp.PlanEntry{{Content: "replayed plan", Status: "in_progress"}}, + }}, + }); err != nil { + return acp.LoadSessionResponse{}, err + } + if a.loadErr != nil { + for i := range 3 { + if err := a.conn.SessionUpdate(ctx, makeReplayNotification(string(req.SessionId), i)); err != nil { + return acp.LoadSessionResponse{}, err + } + } + } + return acp.LoadSessionResponse{}, a.loadErr +} + // burstClient is a minimal acp.Client stub that forwards SessionUpdate // notifications to the adapter's real handler. type burstClient struct { @@ -308,6 +355,230 @@ func TestBurstProducerWatchdog_ClosesPipesBeforeAgentToClientWrite(t *testing.T) } } +func TestLoadSession_DrainsBackedUpReplayBeforeClearingSuppression(t *testing.T) { + a := newTestAdapter() + c2aR, c2aW := io.Pipe() + a2cR, a2cW := io.Pipe() + t.Cleanup(func() { + _ = a.Close() + _ = c2aR.Close() + _ = c2aW.Close() + _ = a2cR.Close() + _ = a2cW.Close() + }) + if err := a.Connect(c2aW, a2cR); err != nil { + t.Fatalf("Connect: %v", err) + } + fake := &replayLoadAgent{} + fake.conn = acp.NewAgentSideConnection(fake, a2cW, c2aR) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + if err := a.Initialize(ctx); err != nil { + t.Fatalf("Initialize: %v", err) + } + drainEvents(a) + + workerBlocked := make(chan struct{}) + releaseWorker := make(chan struct{}) + workerReleased := false + defer func() { + if !workerReleased { + close(releaseWorker) + } + }() + a.dialect.suppressNotification = func(n acp.SessionNotification) bool { + if n.Update.UsageUpdate != nil { + close(workerBlocked) + <-releaseWorker + } + return false + } + + loadDone := make(chan error, 1) + go func() { loadDone <- a.LoadSession(ctx, "replay-session", nil) }() + select { + case <-workerBlocked: + case <-ctx.Done(): + t.Fatal("replay usage did not reach blocked update worker") + } + select { + case err := <-loadDone: + t.Fatalf("LoadSession returned before replay queue drained: %v", err) + case <-time.After(50 * time.Millisecond): + } + close(releaseWorker) + workerReleased = true + if err := <-loadDone; err != nil { + t.Fatalf("LoadSession: %v", err) + } + + a.mu.RLock() + loading := a.isLoadingSession + a.mu.RUnlock() + if loading { + t.Fatal("load suppression remained active after replay barrier") + } + if delta, cost := a.consumeUsageDelta("replay-session"); delta != 0 || cost != 0 { + t.Fatalf("replay baseline leaked into first turn: delta=%d cost=%d", delta, cost) + } + a.convertUsageUpdate("replay-session", usageUpdateWithCost(200_000, 120, 1.25, "USD")) + if delta, cost := a.consumeUsageDelta("replay-session"); delta != 20 || cost != 200 { + t.Fatalf("post-load turn delta = (%d, %d), want (20, 200)", delta, cost) + } + + events := drainEvents(a) + var replayPlanSeen bool + for _, event := range events { + if event.Type == streams.EventTypePlan && len(event.PlanEntries) == 1 && + event.PlanEntries[0].Description == "replayed plan" { + replayPlanSeen = true + } + } + if !replayPlanSeen { + t.Fatal("captured replay plan was not re-emitted after queue drain") + } +} + +func TestLoadSession_FailureDrainsReplayBeforeClearingSuppression(t *testing.T) { + a, fake, ctx := setupReplayLoadTest(t) + fake.loadErr = errReplayLoadFailed + + workerBlocked, releaseWorker := blockReplayWorker(t, a) + loadDone := make(chan error, 1) + go func() { loadDone <- a.LoadSession(ctx, "failed-replay-session", nil) }() + + select { + case <-workerBlocked: + case <-ctx.Done(): + t.Fatal("failed-load replay did not reach blocked update worker") + } + select { + case err := <-loadDone: + t.Fatalf("LoadSession returned before failed replay queue drained: %v", err) + case <-time.After(50 * time.Millisecond): + } + + releaseWorker() + err := <-loadDone + if err == nil || !strings.Contains(err.Error(), errReplayLoadFailed.Error()) { + t.Fatalf("LoadSession error = %v, want original RPC error %v", err, errReplayLoadFailed) + } + + a.mu.RLock() + loading := a.isLoadingSession + replayPlan := a.loadReplayPlan + a.mu.RUnlock() + if loading { + t.Fatal("load suppression remained active after failed-load replay barrier") + } + if replayPlan != nil { + t.Fatal("failed load retained replay plan after queue drain") + } + + for _, event := range drainEvents(a) { + switch event.Type { + case streams.EventTypeMessageChunk, streams.EventTypeToolCall, streams.EventTypePlan: + t.Fatalf("failed-load replay leaked as live %q event: %+v", event.Type, event) + } + } +} + +func TestLoadSession_FailureBarrierUnblocksOnClose(t *testing.T) { + a, fake, ctx := setupReplayLoadTest(t) + fake.loadErr = errReplayLoadFailed + + workerBlocked, releaseWorker := blockReplayWorker(t, a) + loadDone := make(chan error, 1) + go func() { loadDone <- a.LoadSession(ctx, "closing-replay-session", nil) }() + + select { + case <-workerBlocked: + case <-ctx.Done(): + t.Fatal("failed-load replay did not reach blocked update worker") + } + + closeDone := make(chan error, 1) + go func() { closeDone <- a.Close() }() + + select { + case err := <-loadDone: + if err == nil || !strings.Contains(err.Error(), errReplayLoadFailed.Error()) { + t.Fatalf("LoadSession error = %v, want original RPC error %v", err, errReplayLoadFailed) + } + case <-ctx.Done(): + t.Fatal("LoadSession failure barrier did not unblock during Close") + } + + a.mu.RLock() + loading := a.isLoadingSession + replayPlan := a.loadReplayPlan + a.mu.RUnlock() + if loading || replayPlan != nil { + t.Fatalf("failed load state after Close = loading:%t plan:%v, want cleared", loading, replayPlan) + } + + releaseWorker() + select { + case err := <-closeDone: + if err != nil { + t.Fatalf("Close: %v", err) + } + case <-ctx.Done(): + t.Fatal("Close did not finish after blocked worker was released") + } +} + +func setupReplayLoadTest(t *testing.T) (*Adapter, *replayLoadAgent, context.Context) { + t.Helper() + + a := newTestAdapter() + c2aR, c2aW := io.Pipe() + a2cR, a2cW := io.Pipe() + t.Cleanup(func() { + _ = a.Close() + _ = c2aR.Close() + _ = c2aW.Close() + _ = a2cR.Close() + _ = a2cW.Close() + }) + if err := a.Connect(c2aW, a2cR); err != nil { + t.Fatalf("Connect: %v", err) + } + fake := &replayLoadAgent{} + fake.conn = acp.NewAgentSideConnection(fake, a2cW, c2aR) + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + t.Cleanup(cancel) + if err := a.Initialize(ctx); err != nil { + t.Fatalf("Initialize: %v", err) + } + drainEvents(a) + return a, fake, ctx +} + +func blockReplayWorker(t *testing.T, a *Adapter) (<-chan struct{}, func()) { + t.Helper() + + workerBlocked := make(chan struct{}) + releaseWorker := make(chan struct{}) + var releaseOnce sync.Once + release := func() { + releaseOnce.Do(func() { + close(releaseWorker) + }) + } + t.Cleanup(release) + a.dialect.suppressNotification = func(n acp.SessionNotification) bool { + if n.Update.UsageUpdate != nil { + close(workerBlocked) + <-releaseWorker + } + return false + } + return workerBlocked, release +} + // TestLoadReplayBurst_HandlesLargeReplay is a regression test for the // "notification queue overflow" failure we hit on a 304-exchange auggie // session load. The acp-go-sdk exposes a hardcoded 1024-slot inbound channel diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/new_session_usage_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/new_session_usage_test.go new file mode 100644 index 0000000000..3bfe19971b --- /dev/null +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/new_session_usage_test.go @@ -0,0 +1,331 @@ +package acp + +import ( + "context" + "errors" + "fmt" + "io" + "strings" + "sync" + "testing" + "time" + + acpsdk "github.com/coder/acp-go-sdk" + "github.com/kandev/kandev/internal/agentctl/types/streams" +) + +type sessionCreationAgent struct { + burstAgent + conn *acpsdk.AgentSideConnection + mu sync.Mutex + calls int + onNew func(context.Context, acpsdk.NewSessionRequest, int) (acpsdk.NewSessionResponse, error) +} + +func (a *sessionCreationAgent) Initialize( + _ context.Context, + req acpsdk.InitializeRequest, +) (acpsdk.InitializeResponse, error) { + return acpsdk.InitializeResponse{ProtocolVersion: req.ProtocolVersion}, nil +} + +func (a *sessionCreationAgent) NewSession( + ctx context.Context, + req acpsdk.NewSessionRequest, +) (acpsdk.NewSessionResponse, error) { + a.mu.Lock() + a.calls++ + call := a.calls + onNew := a.onNew + a.mu.Unlock() + if onNew != nil { + return onNew(ctx, req, call) + } + return acpsdk.NewSessionResponse{SessionId: acpsdk.SessionId(fmt.Sprintf("new-%d", call))}, nil +} + +func (a *sessionCreationAgent) sendUsage( + ctx context.Context, + sessionID string, + used int64, + costUSD float64, +) error { + return a.conn.SessionUpdate(ctx, acpsdk.SessionNotification{ + SessionId: acpsdk.SessionId(sessionID), + Update: acpsdk.SessionUpdate{ + UsageUpdate: usageUpdateWithCost(200_000, used, costUSD, "USD"), + }, + }) +} + +func setupSessionCreationAdapter(t *testing.T) (*Adapter, *sessionCreationAgent) { + t.Helper() + a := newTestAdapter() + c2aR, c2aW := io.Pipe() + a2cR, a2cW := io.Pipe() + t.Cleanup(func() { + _ = a.Close() + _ = c2aR.Close() + _ = c2aW.Close() + _ = a2cR.Close() + _ = a2cW.Close() + }) + if err := a.Connect(c2aW, a2cR); err != nil { + t.Fatalf("Connect: %v", err) + } + fake := &sessionCreationAgent{} + fake.conn = acpsdk.NewAgentSideConnection(fake, a2cW, c2aR) + if err := a.Initialize(context.Background()); err != nil { + t.Fatalf("Initialize: %v", err) + } + drainEvents(a) + return a, fake +} + +func TestNewSession_NoUsageNotificationLeavesNoTracker(t *testing.T) { + a, _ := setupSessionCreationAdapter(t) + a.convertUsageUpdate("stale-session", usageUpdate(200_000, 900)) + + sessionID, err := a.NewSession(context.Background(), nil) + if err != nil { + t.Fatalf("NewSession: %v", err) + } + if sessionID != "new-1" { + t.Fatalf("sessionID = %q, want new-1", sessionID) + } + a.mu.RLock() + trackerCount := len(a.usageBySession) + a.mu.RUnlock() + if trackerCount != 0 { + t.Fatalf("usage tracker count = %d, want 0", trackerCount) + } +} + +func TestNewSession_ConsumesCreationUsageAndCostAtBarrier(t *testing.T) { + a, fake := setupSessionCreationAdapter(t) + fake.onNew = func(ctx context.Context, _ acpsdk.NewSessionRequest, _ int) (acpsdk.NewSessionResponse, error) { + if err := fake.sendUsage(ctx, "created-session", 100, 1.23); err != nil { + return acpsdk.NewSessionResponse{}, err + } + return acpsdk.NewSessionResponse{SessionId: "created-session"}, nil + } + + if _, err := a.NewSession(context.Background(), nil); err != nil { + t.Fatalf("NewSession: %v", err) + } + if delta, cost := a.consumeUsageDelta("created-session"); delta != 0 || cost != 0 { + t.Fatalf("creation baseline = (%d, %d), want (0, 0)", delta, cost) + } + a.mu.RLock() + maxSize := a.usageBySession["created-session"].maxSize + a.mu.RUnlock() + if maxSize != 200_000 { + t.Fatalf("sticky max size = %d, want 200000", maxSize) + } + + a.convertUsageUpdate("created-session", usageUpdateWithCost(200_000, 125, 1.25, "USD")) + if delta, cost := a.consumeUsageDelta("created-session"); delta != 25 || cost != 200 { + t.Fatalf("first-turn delta = (%d, %d), want (25, 200)", delta, cost) + } + + var contextEventSeen bool + for _, event := range drainEvents(a) { + if event.Type == streams.EventTypeContextWindow && event.ContextWindowUsed == 100 { + contextEventSeen = true + } + } + if !contextEventSeen { + t.Fatal("session-creation context window was not displayed") + } +} + +func TestNewSession_WaitsForBackedUpCreationUsage(t *testing.T) { + a, fake := setupSessionCreationAdapter(t) + fake.onNew = func(ctx context.Context, _ acpsdk.NewSessionRequest, _ int) (acpsdk.NewSessionResponse, error) { + if err := fake.sendUsage(ctx, "blocked-session", 100, 1.23); err != nil { + return acpsdk.NewSessionResponse{}, err + } + return acpsdk.NewSessionResponse{SessionId: "blocked-session"}, nil + } + + workerBlocked := make(chan struct{}) + releaseWorker := make(chan struct{}) + released := false + defer func() { + if !released { + close(releaseWorker) + } + }() + a.dialect.suppressNotification = func(n acpsdk.SessionNotification) bool { + if n.Update.UsageUpdate != nil { + close(workerBlocked) + <-releaseWorker + } + return false + } + + done := make(chan error, 1) + go func() { _, err := a.NewSession(context.Background(), nil); done <- err }() + select { + case <-workerBlocked: + case <-time.After(5 * time.Second): + t.Fatal("creation usage did not reach blocked worker") + } + select { + case err := <-done: + t.Fatalf("NewSession returned before creation barrier drained: %v", err) + case <-time.After(50 * time.Millisecond): + } + close(releaseWorker) + released = true + if err := <-done; err != nil { + t.Fatalf("NewSession: %v", err) + } + if delta, cost := a.consumeUsageDelta("blocked-session"); delta != 0 || cost != 0 { + t.Fatalf("blocked creation baseline = (%d, %d), want (0, 0)", delta, cost) + } +} + +func TestNewSession_RPCFailureAndCancellationClearTrackers(t *testing.T) { + tests := []struct { + name string + ctx func() context.Context + }{ + { + name: "failure", + ctx: context.Background, + }, + { + name: "cancellation", + ctx: func() context.Context { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + return ctx + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + a, fake := setupSessionCreationAdapter(t) + if tt.name == "failure" { + fake.onNew = func(ctx context.Context, _ acpsdk.NewSessionRequest, _ int) (acpsdk.NewSessionResponse, error) { + if err := fake.sendUsage(ctx, "failed-session", 100, 1.23); err != nil { + return acpsdk.NewSessionResponse{}, err + } + return acpsdk.NewSessionResponse{}, errors.New("creation failed") + } + } else { + fake.onNew = func(context.Context, acpsdk.NewSessionRequest, int) (acpsdk.NewSessionResponse, error) { + return acpsdk.NewSessionResponse{}, context.Canceled + } + } + a.convertUsageUpdate("stale-session", usageUpdate(200_000, 900)) + + if _, err := a.NewSession(tt.ctx(), nil); err == nil { + t.Fatal("NewSession unexpectedly succeeded") + } + a.mu.RLock() + trackerCount := len(a.usageBySession) + a.mu.RUnlock() + if trackerCount != 0 { + t.Fatalf("usage tracker count after failure = %d, want 0", trackerCount) + } + }) + } +} + +func TestResetSession_ConsumesEachCreationBaseline(t *testing.T) { + a, fake := setupSessionCreationAdapter(t) + fake.onNew = func(ctx context.Context, _ acpsdk.NewSessionRequest, call int) (acpsdk.NewSessionResponse, error) { + sessionID := fmt.Sprintf("reset-%d", call) + if err := fake.sendUsage(ctx, sessionID, int64(call*100), float64(call)+0.25); err != nil { + return acpsdk.NewSessionResponse{}, err + } + return acpsdk.NewSessionResponse{SessionId: acpsdk.SessionId(sessionID)}, nil + } + + if _, err := a.NewSession(context.Background(), nil); err != nil { + t.Fatalf("NewSession: %v", err) + } + if _, err := a.ResetSession(context.Background(), nil); err != nil { + t.Fatalf("ResetSession: %v", err) + } + if delta, cost := a.consumeUsageDelta("reset-2"); delta != 0 || cost != 0 { + t.Fatalf("reset baseline = (%d, %d), want (0, 0)", delta, cost) + } + a.mu.RLock() + _, oldTracker := a.usageBySession["reset-1"] + a.mu.RUnlock() + if oldTracker { + t.Fatal("ResetSession retained the previous session tracker") + } + a.convertUsageUpdate("reset-2", usageUpdateWithCost(200_000, 230, 2.50, "USD")) + if delta, cost := a.consumeUsageDelta("reset-2"); delta != 30 || cost != 2500 { + t.Fatalf("first post-reset delta = (%d, %d), want (30, 2500)", delta, cost) + } +} + +func TestNewSession_BarrierShutdownClearsProvisionalTracker(t *testing.T) { + a, fake := setupSessionCreationAdapter(t) + fake.onNew = func(ctx context.Context, _ acpsdk.NewSessionRequest, _ int) (acpsdk.NewSessionResponse, error) { + if err := fake.sendUsage(ctx, "shutdown-session", 100, 1.23); err != nil { + return acpsdk.NewSessionResponse{}, err + } + return acpsdk.NewSessionResponse{SessionId: "shutdown-session"}, nil + } + + workerBlocked := make(chan struct{}) + releaseWorker := make(chan struct{}) + a.dialect.suppressNotification = func(n acpsdk.SessionNotification) bool { + if n.Update.UsageUpdate != nil { + close(workerBlocked) + <-releaseWorker + } + return false + } + done := make(chan error, 1) + go func() { _, err := a.NewSession(context.Background(), nil); done <- err }() + select { + case <-workerBlocked: + case <-time.After(5 * time.Second): + t.Fatal("creation usage did not reach blocked worker") + } + waitForQueuedBarrier(t, a) + closeDone := make(chan struct{}) + go func() { _ = a.Close(); close(closeDone) }() + + err := <-done + if err == nil || !strings.Contains(err.Error(), "synchronize new session notifications") { + t.Fatalf("NewSession error = %v, want barrier synchronization error", err) + } + close(releaseWorker) + select { + case <-closeDone: + case <-time.After(5 * time.Second): + t.Fatal("Close did not finish after releasing worker") + } + a.mu.RLock() + trackerCount := len(a.usageBySession) + a.mu.RUnlock() + if trackerCount != 0 { + t.Fatalf("usage tracker count after barrier shutdown = %d, want 0", trackerCount) + } +} + +func waitForQueuedBarrier(t *testing.T, a *Adapter) { + t.Helper() + deadline := time.NewTimer(5 * time.Second) + defer deadline.Stop() + ticker := time.NewTicker(time.Millisecond) + defer ticker.Stop() + for { + if len(a.notifQueue) > 0 { + return + } + select { + case <-deadline.C: + t.Fatal("session/new barrier was not queued behind blocked notification") + case <-ticker.C: + } + } +} diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go index 1c0c71ef4b..a8498d4954 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/normalize.go @@ -80,12 +80,13 @@ func DetectToolOperationType(toolKind string, args map[string]any) string { // Normalizer converts ACP protocol tool data to NormalizedPayload. type Normalizer struct { agentID string + dialect acpDialect } // NewNormalizer creates a new ACP normalizer. agentID selects per-agent enrichers // (e.g. "codex-acp"); pass "" for common-layer-only normalization in tests. func NewNormalizer(agentID string) *Normalizer { - return &Normalizer{agentID: agentID} + return &Normalizer{agentID: agentID, dialect: newACPDialect(agentID)} } // NormalizeToolCall converts ACP tool call data to NormalizedPayload. @@ -382,9 +383,30 @@ func (n *Normalizer) EnrichFromToolCallUpdate( rawInput any, supplemental map[string]any, ) { + n.enrichDialectSubagent(payload, meta, rawInput) applyAgentEnrichment(n.agentID, payload, enrichFrameFromUpdate(title, meta, rawInput, supplemental)) } +func (n *Normalizer) enrichDialectSubagent( + payload *streams.NormalizedPayload, + meta map[string]any, + rawInput any, +) { + if payload == nil || payload.Kind() != streams.ToolKindSubagentTask { + return + } + frame, ok := n.dialect.parseSubagentFrame(meta, "", rawInput) + if !ok { + return + } + updateSubagentTaskInput(payload.SubagentTask(), map[string]any{ + subagentKeyDescription: frame.description, + subagentKeyPrompt: frame.prompt, + subagentKeySubagentType: frame.subagentType, + }) + applySubagentResult(payload.SubagentTask(), frame.result) +} + // normalizeEdit converts ACP edit tool data. func (n *Normalizer) normalizeEdit(args map[string]any) *streams.NormalizedPayload { rawInput, _ := args["raw_input"].(map[string]any) @@ -547,6 +569,11 @@ func (n *Normalizer) normalizeSubagent(args map[string]any) (*streams.Normalized meta, _ := args[argKeyMeta].(map[string]any) title, _ := args[argKeyTitle].(string) rawInput := args["raw_input"] + if frame, ok := n.dialect.parseSubagentFrame(meta, title, rawInput); ok { + payload := streams.NewSubagentTask(frame.description, frame.prompt, frame.subagentType) + applySubagentResult(payload.SubagentTask(), frame.result) + return payload, true + } desc, prompt, subagentType, ok := recognizeSubagent(meta, title, rawInput) if !ok { return nil, false diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/subagent.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/subagent.go index 6ac1e36f07..9177eb2c56 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/subagent.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/subagent.go @@ -6,8 +6,9 @@ import ( "github.com/kandev/kandev/internal/agentctl/types/streams" ) -// Subagent (Task) tool detection constants. These are the wire-level tool -// identifiers four real agents use to spawn a child agent ("subagent"): +// Subagent (Task) tool detection constants. These are the common wire-level +// identifiers four agents use to spawn a child agent ("subagent"). Codex's +// agent-scoped `_meta.codex` shapes are parsed by dialect_codex.go instead: // - Claude-acp tags it via `_meta.claudeCode.toolName == "Agent"`. // - OpenCode reports tool title == "task" (case-insensitive). // - Cursor reports `rawInput._toolName == "task"` (title "Task: Subagent task"). @@ -57,6 +58,16 @@ type SubagentTaskResult struct { ResultText string } +// subagentFrame is the provider-neutral result of parsing one agent-specific +// subagent metadata frame. The common normalizer owns payload construction; +// dialects only describe the data observed on the wire. +type subagentFrame struct { + description string + prompt string + subagentType string + result SubagentTaskResult +} + // subagentAsyncLaunchedStatus is the Claude Code marker that the Task tool // dispatched a background subagent. It appears at // `_meta.claudeCode.toolResponse.status` with `_meta.claudeCode.toolResponse.isAsync:true`. @@ -112,7 +123,8 @@ func recognizeSubagent(meta map[string]any, title string, rawInput any) (descrip return description, prompt, subagentType, true } -// isSubagentSignal implements the four detection rules. +// isSubagentSignal implements the four common detection rules. Agent-scoped +// dialect signals (currently Codex) are checked by Normalizer first. func isSubagentSignal(meta map[string]any, title string, rawInput any) bool { if isClaudeAgentMeta(meta) { return true diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/subagent_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/subagent_test.go index bc01cc2dde..7b9f3edbad 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/subagent_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/subagent_test.go @@ -2,9 +2,11 @@ package acp import ( "encoding/json" + "strconv" "strings" "testing" + acp "github.com/coder/acp-go-sdk" "github.com/kandev/kandev/internal/agentctl/types/streams" ) @@ -270,6 +272,743 @@ func TestNormalizeToolCall_CursorSubagent(t *testing.T) { } } +func TestNormalizeToolCall_CodexSpawnSubagent(t *testing.T) { + n := NewNormalizer(codexAgentID) + payload := n.NormalizeToolCall("other", map[string]any{ + "kind": toolTypeGeneric, + "meta": codexCollaborationMeta(codexCollaborationSpawnAgent, []any{"thread-child"}), + "raw_input": map[string]any{ + "prompt": "Audit the ACP adapter", + "model": "gpt-5.2-codex", + "agentsStates": map[string]any{ + "thread-child": map[string]any{"status": "running"}, + }, + }, + }) + if payload.Kind() != streams.ToolKindSubagentTask { + t.Fatalf("Kind = %q, want subagent_task", payload.Kind()) + } + sa := payload.SubagentTask() + if sa.ChildSessionID != "thread-child" || sa.Prompt != "Audit the ACP adapter" { + t.Errorf("child/prompt = %q/%q", sa.ChildSessionID, sa.Prompt) + } + if sa.Model != "gpt-5.2-codex" || sa.Status != "running" { + t.Errorf("model/status = %q/%q", sa.Model, sa.Status) + } +} + +func TestNormalizeToolCall_CodexStartedActivity(t *testing.T) { + n := NewNormalizer(codexAgentID) + payload := n.NormalizeToolCall("other", map[string]any{ + "kind": toolTypeGeneric, + "meta": codexActivityMeta(codexSubagentStarted), + }) + if payload.Kind() != streams.ToolKindSubagentTask { + t.Fatalf("Kind = %q, want subagent_task", payload.Kind()) + } + sa := payload.SubagentTask() + if sa.ChildSessionID != "thread-child" || sa.Status != "started" { + t.Errorf("child/status = %q/%q", sa.ChildSessionID, sa.Status) + } +} + +func TestNormalizeToolCall_CodexControlsStayGeneric(t *testing.T) { + for _, tool := range []string{"interact", "wait", "close"} { + t.Run(tool, func(t *testing.T) { + n := NewNormalizer(codexAgentID) + payload := n.NormalizeToolCall("other", map[string]any{ + "kind": toolTypeGeneric, + "meta": codexCollaborationMeta(tool, []any{"thread-child"}), + }) + if payload.Kind() != streams.ToolKindGeneric { + t.Fatalf("Kind = %q, want generic", payload.Kind()) + } + }) + } +} + +func TestNormalizeToolCall_CodexMetaIsDialectScoped(t *testing.T) { + payload := NewNormalizer("cursor-acp").NormalizeToolCall("other", map[string]any{ + "kind": toolTypeGeneric, + "meta": codexCollaborationMeta(codexCollaborationSpawnAgent, []any{"thread-child"}), + }) + if payload.Kind() != streams.ToolKindGeneric { + t.Fatalf("Kind = %q, want generic for a non-Codex agent", payload.Kind()) + } +} + +func TestCodexSubagentSequenceUsesImmutableSnapshots(t *testing.T) { + a := newTestAdapter() + a.agentID = codexAgentID + a.normalizer = NewNormalizer(codexAgentID) + meta := codexCollaborationMeta(codexCollaborationSpawnAgent, []any{"thread-child"}) + start := &acp.SessionUpdateToolCall{ + ToolCallId: "call-spawn", + Kind: "other", + Title: codexCollaborationSpawnAgent, + Status: toolStatusInProgress, + RawInput: map[string]any{ + "prompt": "Audit the adapter", + "agentsStates": map[string]any{ + "thread-child": map[string]any{"status": "running"}, + }, + }, + Meta: meta, + } + startEvent := a.convertToolCallUpdate("session-1", start) + if startEvent.NormalizedPayload.Kind() != streams.ToolKindSubagentTask { + t.Fatalf("start kind = %q", startEvent.NormalizedPayload.Kind()) + } + startSnapshot := startEvent.NormalizedPayload + + completeStatus := acp.ToolCallStatus(toolStatusCompleted) + complete := &acp.SessionToolCallUpdate{ + ToolCallId: "call-spawn", + Status: &completeStatus, + RawInput: map[string]any{ + "prompt": "Audit the adapter", + "model": "gpt-5.2-codex", + "agentsStates": map[string]any{ + "thread-child": map[string]any{"status": "completed"}, + }, + }, + Meta: meta, + } + completeEvent := a.convertToolCallResultUpdate("session-1", complete) + if completeEvent.ToolCallID != startEvent.ToolCallID { + t.Fatalf("tool call IDs differ: %q/%q", startEvent.ToolCallID, completeEvent.ToolCallID) + } + sa := completeEvent.NormalizedPayload.SubagentTask() + if sa.Status != "completed" || sa.Model != "gpt-5.2-codex" { + t.Errorf("status/model = %q/%q", sa.Status, sa.Model) + } + if got := startSnapshot.SubagentTask(); got.Status != "running" || got.Model != "" { + t.Errorf("start event snapshot mutated after completion: status/model = %q/%q", got.Status, got.Model) + } + if _, active := a.activeToolCalls["call-spawn"]; active { + t.Error("terminal completion must remove the active tool call") + } + + // codex-acp may deliver the matching started activity after the terminal + // collaboration update. It must update the existing card, not create a + // second one or regress the terminal state back to running. + activityEvent := a.convertToolCallUpdate("session-1", codexStartedActivityToolCall("call-spawn")) + if activityEvent.Type != streams.EventTypeToolUpdate { + t.Fatalf("reordered activity type = %q, want tool_update", activityEvent.Type) + } + if activityEvent.ToolStatus != toolStatusComplete || activityEvent.NormalizedPayload.SubagentTask().Status != "completed" { + t.Errorf("reordered activity regressed status: event=%q payload=%q", activityEvent.ToolStatus, activityEvent.NormalizedPayload.SubagentTask().Status) + } + if got := completeEvent.NormalizedPayload.SubagentTask(); got.SubagentType != "" { + t.Errorf("completion event snapshot mutated after activity: subagent_type = %q", got.SubagentType) + } + if got := activityEvent.NormalizedPayload.SubagentTask(); got.SubagentType != "review_agent" { + t.Errorf("activity snapshot subagent_type = %q, want review_agent", got.SubagentType) + } +} + +func TestCloneSubagentPayloadDeepCopiesMutableFields(t *testing.T) { + payload := streams.NewSubagentTask("description", "prompt", "reviewer") + count := 7 + source := payload.SubagentTask() + source.Status = "completed" + source.AgentID = "agent-1" + source.Model = "gpt-5.2-codex" + source.ChildSessionID = "child-1" + source.DurationMs = 123 + source.TotalTokens = 456 + source.ToolUseCount = &count + source.ResultText = "done" + source.IsAsync = true + source.OutputFile = "/tmp/result" + source.CanReadOutputFile = true + source.SetIsAuggie(true) + + clone := cloneSubagentPayload(payload).SubagentTask() + source.Description = "mutated" + *source.ToolUseCount = 99 + if clone.Description != "description" || clone.ToolUseCount == nil || *clone.ToolUseCount != 7 { + t.Fatalf("clone changed with source: description/count = %q/%v", clone.Description, clone.ToolUseCount) + } + if !clone.IsAuggie() || !clone.IsAsync || clone.OutputFile != "/tmp/result" || !clone.CanReadOutputFile { + t.Fatalf("clone lost adapter fields: %+v", clone) + } +} + +func TestCodexSubagentActivityDeduplicatesWhileRunning(t *testing.T) { + a := newTestAdapter() + a.agentID = codexAgentID + a.normalizer = NewNormalizer(codexAgentID) + meta := codexCollaborationMeta(codexCollaborationSpawnAgent, []any{"thread-child"}) + start := &acp.SessionUpdateToolCall{ + ToolCallId: "call-spawn", + Kind: "other", + Title: codexCollaborationSpawnAgent, + Status: toolStatusInProgress, + RawInput: map[string]any{ + "prompt": "Audit the adapter", + "agentsStates": map[string]any{ + "thread-child": map[string]any{"status": "running"}, + }, + }, + Meta: meta, + } + startEvent := a.convertToolCallUpdate("session-1", start) + activityEvent := a.convertToolCallUpdate("session-1", codexStartedActivityToolCall("call-spawn")) + if startEvent.Type != streams.EventTypeToolCall || activityEvent.Type != streams.EventTypeToolUpdate { + t.Fatalf("event types = %q/%q, want tool_call/tool_update", startEvent.Type, activityEvent.Type) + } + if activityEvent.ToolStatus != toolStatusInProgress { + t.Errorf("activity ToolStatus = %q, want in_progress", activityEvent.ToolStatus) + } + if got := activityEvent.NormalizedPayload.SubagentTask().Status; got != "running" { + t.Errorf("payload status = %q, want running (started must not downgrade it)", got) + } + if got := startEvent.NormalizedPayload.SubagentTask().SubagentType; got != "" { + t.Errorf("start event snapshot mutated after activity: subagent_type = %q", got) + } + if got := activityEvent.NormalizedPayload.SubagentTask().SubagentType; got != "review_agent" { + t.Errorf("activity subagent_type = %q, want review_agent", got) + } +} + +func TestCodexSubagentCorrelationPressureRetainsDelayedMatch(t *testing.T) { + a := newTestAdapter() + a.agentID = codexAgentID + a.normalizer = NewNormalizer(codexAgentID) + delayed := codexCollaborationToolCall("delayed", "delayed-child", "running") + if event := a.convertToolCallUpdate("session-1", delayed); event.Type != streams.EventTypeToolCall { + t.Fatalf("delayed first event type = %q", event.Type) + } + for i := 0; i < maxCodexCompletedSubagentCorrelations+20; i++ { + id := "live-" + strconv.Itoa(i) + a.convertToolCallUpdate("session-1", codexCollaborationToolCall(id, "child-"+strconv.Itoa(i), "running")) + } + wantLive := maxCodexCompletedSubagentCorrelations + 21 + if got := len(a.codexSubagentCorrelations); got != wantLive { + t.Fatalf("live correlation count = %d, want %d retained until completion", got, wantLive) + } + delayedEvent := a.convertToolCallUpdate("session-1", codexStartedActivityToolCallForChild("delayed", "delayed-child")) + if delayedEvent.Type != streams.EventTypeToolUpdate { + t.Fatalf("delayed matching event type = %q, want tool_update after cache pressure", delayedEvent.Type) + } + key := codexSubagentCorrelationKey{ + sessionID: "session-1", + toolCallID: "delayed", + childSessionID: "delayed-child", + } + correlation := a.codexSubagentCorrelations[key] + if correlation == nil || !correlation.collaborationSeen || !correlation.activitySeen { + t.Fatalf("delayed correlation signal state = %+v", correlation) + } +} + +func TestCodexCompletedTombstonesAreBoundedOldestFirst(t *testing.T) { + a := newCodexCorrelationTestAdapter() + total := maxCodexCompletedSubagentCorrelations + 20 + for i := 0; i < total; i++ { + id := "paired-" + strconv.Itoa(i) + childID := "child-" + strconv.Itoa(i) + a.convertToolCallUpdate("session-1", codexCollaborationToolCall(id, childID, "running")) + a.convertToolCallUpdate("session-1", codexStartedActivityToolCallForChild(id, childID)) + } + if got := a.codexCompletedCorrelationCountLocked(); got != maxCodexCompletedSubagentCorrelations { + t.Fatalf("completed tombstones = %d, want %d", got, maxCodexCompletedSubagentCorrelations) + } + oldest := codexSubagentCorrelationKey{sessionID: "session-1", toolCallID: "paired-0", childSessionID: "child-0"} + if a.codexSubagentCorrelations[oldest] != nil { + t.Fatal("oldest completed tombstone survived deterministic pruning") + } + newest := codexSubagentCorrelationKey{ + sessionID: "session-1", + toolCallID: "paired-" + strconv.Itoa(total-1), + childSessionID: "child-" + strconv.Itoa(total-1), + } + if a.codexSubagentCorrelations[newest] == nil { + t.Fatal("newest completed tombstone was pruned") + } +} + +func TestCodexCorrelationSameToolIDDifferentChildCreatesNewCard(t *testing.T) { + a := newCodexCorrelationTestAdapter() + first := a.convertToolCallUpdate("session-1", codexCollaborationToolCall("reused", "child-a", "running")) + firstActivity := a.convertToolCallUpdate("session-1", codexStartedActivityToolCallForChild("reused", "child-a")) + second := a.convertToolCallUpdate("session-1", codexCollaborationToolCall("reused", "child-b", "running")) + if first.Type != streams.EventTypeToolCall || firstActivity.Type != streams.EventTypeToolUpdate { + t.Fatalf("first child event types = %q/%q", first.Type, firstActivity.Type) + } + if second.Type != streams.EventTypeToolCall { + t.Fatalf("different child event type = %q, want tool_call", second.Type) + } + if first.ToolCallID == second.ToolCallID { + t.Fatalf("sibling emitted IDs collide at %q", first.ToolCallID) + } + if first.ToolCallID != "reused" { + t.Fatalf("first child ID = %q, want unchanged wire ID", first.ToolCallID) + } + if got := second.NormalizedPayload.SubagentTask().ChildSessionID; got != "child-b" { + t.Fatalf("different child ID = %q", got) + } + + lateFirst := a.convertToolCallUpdate("session-1", codexStartedActivityToolCallForChild("reused", "child-a")) + if lateFirst.Type != streams.EventTypeToolUpdate { + t.Fatalf("same-child replay type = %q, want tool_update", lateFirst.Type) + } + if lateFirst.ToolCallID != first.ToolCallID { + t.Fatalf("late first child ID = %q, want %q", lateFirst.ToolCallID, first.ToolCallID) + } + secondActivity := a.convertToolCallUpdate("session-1", codexStartedActivityToolCallForChild("reused", "child-b")) + if secondActivity.ToolCallID != second.ToolCallID { + t.Fatalf("second activity ID = %q, want %q", secondActivity.ToolCallID, second.ToolCallID) + } + for id, childID := range map[string]string{first.ToolCallID: "child-a", second.ToolCallID: "child-b"} { + if active := a.activeToolCalls[id]; codexSubagentChildID(active) != childID { + t.Fatalf("active payload %q child = %q, want %q", id, codexSubagentChildID(active), childID) + } + } + if got := a.codexCorrelationSiblingCountLocked("session-1", "reused"); got != 2 { + t.Fatalf("correlation sibling count = %d, want 2", got) + } + + firstResult := a.convertToolCallResultUpdate( + "session-1", + codexCollaborationResultUpdate("reused", "child-a", toolStatusCompleted), + ) + secondResult := a.convertToolCallResultUpdate( + "session-1", + codexCollaborationResultUpdate("reused", "child-b", toolStatusCompleted), + ) + if firstResult.ToolCallID != first.ToolCallID || secondResult.ToolCallID != second.ToolCallID { + t.Fatalf( + "result IDs = %q/%q, want %q/%q", + firstResult.ToolCallID, + secondResult.ToolCallID, + first.ToolCallID, + second.ToolCallID, + ) + } + if a.activeToolCalls[first.ToolCallID] != nil || a.activeToolCalls[second.ToolCallID] != nil { + t.Fatal("terminal sibling results did not clear their independent active entries") + } +} + +func TestCodexAmbiguousChildlessResultsAreSuppressed(t *testing.T) { + a := newCodexCorrelationTestAdapter() + first := a.convertToolCallUpdate("session-1", codexCollaborationToolCall("reused", "child-a", "running")) + second := a.convertToolCallUpdate("session-1", codexCollaborationToolCall("reused", "child-b", "running")) + if first.ToolCallID == second.ToolCallID { + t.Fatalf("persistence-facing sibling IDs collide at %q", first.ToolCallID) + } + firstBefore := mustMarshalPayload(t, a.activeToolCalls[first.ToolCallID]) + secondBefore := mustMarshalPayload(t, a.activeToolCalls[second.ToolCallID]) + + running := a.convertToolCallResultUpdate( + "session-1", + codexChildlessCollaborationResultUpdate("reused", toolStatusInProgress), + ) + if running != nil { + t.Fatalf("ambiguous running update emitted persistence event %+v", running) + } + terminal := a.convertToolCallResultUpdate( + "session-1", + codexChildlessCollaborationResultUpdate("reused", toolStatusCompleted), + ) + if terminal != nil { + t.Fatalf("ambiguous terminal update emitted persistence event %+v", terminal) + } + lateReplay := a.convertToolCallResultUpdate( + "session-1", + codexChildlessCollaborationResultUpdate("reused", toolStatusCompleted), + ) + if lateReplay != nil { + t.Fatalf("ambiguous late replay emitted persistence event %+v", lateReplay) + } + + assertActivePayloadUnchanged(t, a, first.ToolCallID, firstBefore) + assertActivePayloadUnchanged(t, a, second.ToolCallID, secondBefore) + + firstResult := a.convertToolCallResultUpdate( + "session-1", + codexCollaborationResultUpdate("reused", "child-a", toolStatusCompleted), + ) + if firstResult == nil || firstResult.ToolCallID != first.ToolCallID { + t.Fatalf("known child-a result = %+v, want emitted ID %q", firstResult, first.ToolCallID) + } + if a.activeToolCalls[first.ToolCallID] != nil { + t.Fatal("known terminal child-a result did not delete child-a active state") + } + assertActivePayloadUnchanged(t, a, second.ToolCallID, secondBefore) + + // Both sibling correlations still exist, so a childless replay remains + // ambiguous even though only child-b is active. It must not complete or + // delete child-b by falling back to the bare wire ID. + lateAfterFirstCompletion := a.convertToolCallResultUpdate( + "session-1", + codexChildlessCollaborationResultUpdate("reused", toolStatusCompleted), + ) + if lateAfterFirstCompletion != nil { + t.Fatalf("post-completion ambiguous replay emitted event %+v", lateAfterFirstCompletion) + } + assertActivePayloadUnchanged(t, a, second.ToolCallID, secondBefore) + + secondResult := a.convertToolCallResultUpdate( + "session-1", + codexCollaborationResultUpdate("reused", "child-b", toolStatusCompleted), + ) + if secondResult == nil || secondResult.ToolCallID != second.ToolCallID { + t.Fatalf("known child-b result = %+v, want emitted ID %q", secondResult, second.ToolCallID) + } + if a.activeToolCalls[second.ToolCallID] != nil { + t.Fatal("known terminal child-b result did not delete child-b active state") + } + if got := mustMarshalPayload(t, first.NormalizedPayload); got != firstBefore { + t.Fatalf("persisted first event snapshot mutated:\nbefore: %s\nafter: %s", firstBefore, got) + } + if got := mustMarshalPayload(t, second.NormalizedPayload); got != secondBefore { + t.Fatalf("persisted second event snapshot mutated:\nbefore: %s\nafter: %s", secondBefore, got) + } +} + +func TestCodexSoleChildlessResultReplayUsesCompletedCorrelation(t *testing.T) { + a := newCodexCorrelationTestAdapter() + start := a.convertToolCallUpdate("session-1", codexCollaborationToolCall("sole", "child-a", "running")) + a.convertToolCallUpdate("session-1", codexStartedActivityToolCallForChild("sole", "child-a")) + completed := a.convertToolCallResultUpdate( + "session-1", + codexCollaborationResultUpdate("sole", "child-a", toolStatusCompleted), + ) + if completed == nil || completed.ToolCallID != start.ToolCallID { + t.Fatalf("known completion = %+v, want ID %q", completed, start.ToolCallID) + } + completedSnapshot := mustMarshalPayload(t, completed.NormalizedPayload) + + replay := a.convertToolCallResultUpdate( + "session-1", + codexChildlessCollaborationResultUpdate("sole", toolStatusCompleted), + ) + if replay == nil || replay.ToolCallID != start.ToolCallID { + t.Fatalf("sole childless replay = %+v, want ID %q", replay, start.ToolCallID) + } + if got := mustMarshalPayload(t, completed.NormalizedPayload); got != completedSnapshot { + t.Fatalf("completed event snapshot mutated:\nbefore: %s\nafter: %s", completedSnapshot, got) + } +} + +func TestCodexCorrelationMissingChildFallbackIsUnambiguousOnly(t *testing.T) { + a := newCodexCorrelationTestAdapter() + a.convertToolCallUpdate("session-1", codexCollaborationToolCall("fallback", "child-a", "running")) + missingActivity := a.convertToolCallUpdate("session-1", codexStartedActivityToolCallForChild("fallback", "")) + if missingActivity.Type != streams.EventTypeToolUpdate { + t.Fatalf("single incomplete fallback type = %q, want tool_update", missingActivity.Type) + } + if got := missingActivity.NormalizedPayload.SubagentTask().ChildSessionID; got != "child-a" { + t.Fatalf("fallback lost known child ID: %q", got) + } + + // The correlation is now fully matched. A later overlapping or replayed + // child-less frame still belongs to the sole compatible child. + replayed := a.convertToolCallUpdate("session-1", codexChildlessCollaborationToolCall("fallback")) + if replayed.Type != streams.EventTypeToolUpdate { + t.Fatalf("child-less frame after completed tombstone = %q, want tool_update", replayed.Type) + } + if replayed.ToolCallID != missingActivity.ToolCallID { + t.Fatalf("replayed child-less ID = %q, want %q", replayed.ToolCallID, missingActivity.ToolCallID) + } + + // The reverse ordering is also safe: a sole incomplete child-less spawn + // adopts the child identity from its matching activity and is re-keyed. + childless := a.convertToolCallUpdate("session-1", codexChildlessCollaborationToolCall("reverse")) + reverse := a.convertToolCallUpdate("session-1", codexStartedActivityToolCallForChild("reverse", "child-c")) + if reverse.Type != streams.EventTypeToolUpdate { + t.Fatalf("reverse fallback type = %q, want tool_update", reverse.Type) + } + if reverse.ToolCallID != childless.ToolCallID { + t.Fatalf("re-keyed child ID changed from %q to %q", childless.ToolCallID, reverse.ToolCallID) + } + knownKey := codexSubagentCorrelationKey{ + sessionID: "session-1", + toolCallID: "reverse", + childSessionID: "child-c", + } + if a.codexSubagentCorrelations[knownKey] == nil { + t.Fatal("reverse fallback was not re-keyed to the discovered child") + } +} + +func TestCodexSiblingNestedChildrenUseEmittedParentIDs(t *testing.T) { + a := newCodexCorrelationTestAdapter() + first := a.convertToolCallUpdate("session-1", codexCollaborationToolCall("reused", "child-a", "running")) + second := a.convertToolCallUpdate("session-1", codexCollaborationToolCall("reused", "child-b", "running")) + + nestedA := codexCollaborationToolCallFrom("nested-a", "child-a", "grandchild-a", "running") + nestedAEvent := a.convertToolCallUpdate("session-1", nestedA) + if nestedAEvent.ParentToolCallID != first.ToolCallID { + t.Fatalf("nested child-a parent = %q, want %q", nestedAEvent.ParentToolCallID, first.ToolCallID) + } + nestedB := codexCollaborationToolCallFrom("nested-b", "child-b", "grandchild-b", "running") + nestedBEvent := a.convertToolCallUpdate("session-1", nestedB) + if nestedBEvent.ParentToolCallID != second.ToolCallID { + t.Fatalf("nested child-b parent = %q, want %q", nestedBEvent.ParentToolCallID, second.ToolCallID) + } + + nestedBActivity := a.convertToolCallUpdate( + "session-1", + codexStartedActivityToolCallForChild("nested-b", "grandchild-b"), + ) + if nestedBActivity.ToolCallID != nestedBEvent.ToolCallID { + t.Fatalf("nested child update ID = %q, want %q", nestedBActivity.ToolCallID, nestedBEvent.ToolCallID) + } + if nestedBActivity.ParentToolCallID != second.ToolCallID { + t.Fatalf("nested child update parent = %q, want %q", nestedBActivity.ParentToolCallID, second.ToolCallID) + } +} + +func TestCodexCorrelationMissingChildDoesNotChooseAmongChildren(t *testing.T) { + a := newCodexCorrelationTestAdapter() + a.convertToolCallUpdate("session-1", codexCollaborationToolCall("ambiguous", "child-a", "running")) + second := a.convertToolCallUpdate("session-1", codexCollaborationToolCall("ambiguous", "child-b", "running")) + if second.Type != streams.EventTypeToolCall { + t.Fatalf("second known child type = %q, want tool_call", second.Type) + } + missing := a.convertToolCallUpdate("session-1", codexStartedActivityToolCallForChild("ambiguous", "")) + if missing.Type != streams.EventTypeToolCall { + t.Fatalf("ambiguous child-less activity type = %q, want tool_call", missing.Type) + } + if got := a.codexCorrelationSiblingCountLocked("session-1", "ambiguous"); got != 3 { + t.Fatalf("ambiguous correlation count = %d, want 3", got) + } +} + +func TestCodexSiblingEmittedIDsAvoidEncodedAndArbitraryCollisions(t *testing.T) { + a := newCodexCorrelationTestAdapter() + first := a.convertToolCallUpdate( + "session-1", + codexCollaborationToolCall("wire~codex-subagent~", "child/雪", "running"), + ) + second := a.convertToolCallUpdate( + "session-1", + codexCollaborationToolCall("wire~codex-subagent~", "child~two", "running"), + ) + if first.ToolCallID == second.ToolCallID { + t.Fatalf("encoded sibling IDs collide at %q", first.ToolCallID) + } + + // A later arbitrary ACP wire ID may equal an ID generated for an earlier + // sibling. The session reservation prevents the two persisted cards from + // sharing an emitted ID, and that fallback remains stable for updates. + collidingWireID := second.ToolCallID + third := a.convertToolCallUpdate( + "session-1", + codexCollaborationToolCall(collidingWireID, "child-three", "running"), + ) + if third.ToolCallID == collidingWireID || third.ToolCallID == first.ToolCallID { + t.Fatalf("arbitrary wire collision was not escaped: %q", third.ToolCallID) + } + thirdUpdate := a.convertToolCallUpdate( + "session-1", + codexStartedActivityToolCallForChild(collidingWireID, "child-three"), + ) + if thirdUpdate.ToolCallID != third.ToolCallID { + t.Fatalf("escaped ID changed on update: %q/%q", third.ToolCallID, thirdUpdate.ToolCallID) + } +} + +func TestCodexSubagentCorrelationCleanup(t *testing.T) { + a := newTestAdapter() + a.agentID = codexAgentID + a.normalizer = NewNormalizer(codexAgentID) + a.convertToolCallUpdate("session-1", codexCollaborationToolCall("first", "child-1", "running")) + + a.convertToolCallUpdate("session-2", &acp.SessionUpdateToolCall{ + ToolCallId: "other-session", + Kind: "other", + Status: toolStatusInProgress, + Meta: codexCollaborationMeta(codexCollaborationSpawnAgent, []any{"other-child"}), + }) + a.mu.Lock() + a.clearCodexSubagentCorrelationsLocked("session-1") + remaining := len(a.codexSubagentCorrelations) + a.mu.Unlock() + if remaining != 1 { + t.Fatalf("session cleanup left %d entries, want only the other session", remaining) + } + if err := a.Close(); err != nil { + t.Fatalf("Close: %v", err) + } + if got := len(a.codexSubagentCorrelations); got != 0 { + t.Fatalf("Close left %d correlation entries", got) + } +} + +func TestCodexEmittedPayloadIsImmutableDuringQueuedSerialization(t *testing.T) { + a := newTestAdapter() + a.agentID = codexAgentID + a.normalizer = NewNormalizer(codexAgentID) + first := a.convertToolCallUpdate("session-1", codexCollaborationToolCall("call-race", "child", "running")) + before, err := json.Marshal(first.NormalizedPayload) + if err != nil { + t.Fatalf("marshal before updates: %v", err) + } + + errs := make(chan error, 1) + done := make(chan struct{}) + go func() { + defer close(done) + for i := 0; i < 2000; i++ { + if _, marshalErr := json.Marshal(first.NormalizedPayload); marshalErr != nil { + errs <- marshalErr + return + } + } + }() + for i := 0; i < 2000; i++ { + status := "completed" + if i%2 == 0 { + status = "errored" + } + a.convertToolCallUpdate("session-1", codexCollaborationToolCall("call-race", "child", status)) + } + <-done + select { + case marshalErr := <-errs: + t.Fatalf("concurrent marshal: %v", marshalErr) + default: + } + after, err := json.Marshal(first.NormalizedPayload) + if err != nil { + t.Fatalf("marshal after updates: %v", err) + } + if string(after) != string(before) { + t.Errorf("queued event payload mutated:\nbefore: %s\nafter: %s", before, after) + } +} + +func TestCodexCorrelationDoesNotChangeOtherAgents(t *testing.T) { + a := newTestAdapter() + tc := &acp.SessionUpdateToolCall{ + ToolCallId: "same-id", + Kind: "other", + Status: toolStatusInProgress, + Meta: codexCollaborationMeta(codexCollaborationSpawnAgent, []any{"child"}), + } + first := a.convertToolCallUpdate("session-1", tc) + second := a.convertToolCallUpdate("session-1", tc) + if first.Type != streams.EventTypeToolCall || second.Type != streams.EventTypeToolCall { + t.Fatalf("non-Codex event types = %q/%q, want unchanged tool_call behavior", first.Type, second.Type) + } + if first.ToolCallID != "same-id" || second.ToolCallID != "same-id" { + t.Fatalf("non-Codex tool call IDs changed: %q/%q", first.ToolCallID, second.ToolCallID) + } + if len(a.codexSubagentCorrelations) != 0 { + t.Fatal("non-Codex tool calls must not populate Codex correlation state") + } +} + +func codexStartedActivityToolCall(toolCallID string) *acp.SessionUpdateToolCall { + return codexStartedActivityToolCallForChild(toolCallID, "thread-child") +} + +func codexStartedActivityToolCallForChild(toolCallID, childID string) *acp.SessionUpdateToolCall { + return &acp.SessionUpdateToolCall{ + ToolCallId: acp.ToolCallId(toolCallID), + Kind: "other", + Title: "Start subagent review_agent", + Status: toolStatusCompleted, + RawInput: map[string]any{ + "agentThreadId": childID, + "agentPath": "/root/review_agent", + "activityKind": codexSubagentStarted, + }, + Meta: map[string]any{"codex": map[string]any{"subagent": map[string]any{ + "threadId": childID, + "path": "/root/review_agent", + "activity": codexSubagentStarted, + }}}, + } +} + +func codexCollaborationToolCall(toolCallID, childID, status string) *acp.SessionUpdateToolCall { + return codexCollaborationToolCallFrom(toolCallID, "thread-main", childID, status) +} + +func codexCollaborationToolCallFrom(toolCallID, senderID, childID, status string) *acp.SessionUpdateToolCall { + return &acp.SessionUpdateToolCall{ + ToolCallId: acp.ToolCallId(toolCallID), + Kind: "other", + Title: codexCollaborationSpawnAgent, + Status: toolStatusInProgress, + RawInput: map[string]any{ + "prompt": "Audit the adapter", + "agentsStates": map[string]any{ + childID: map[string]any{"status": status}, + }, + }, + Meta: codexCollaborationMetaFrom(codexCollaborationSpawnAgent, senderID, []any{childID}), + } +} + +func codexCollaborationResultUpdate(toolCallID, childID, status string) *acp.SessionToolCallUpdate { + toolStatus := acp.ToolCallStatus(status) + return &acp.SessionToolCallUpdate{ + ToolCallId: acp.ToolCallId(toolCallID), + Status: &toolStatus, + RawInput: map[string]any{ + "agentsStates": map[string]any{ + childID: map[string]any{"status": status}, + }, + }, + Meta: codexCollaborationMeta(codexCollaborationSpawnAgent, []any{childID}), + } +} + +func codexChildlessCollaborationResultUpdate(toolCallID, status string) *acp.SessionToolCallUpdate { + toolStatus := acp.ToolCallStatus(status) + return &acp.SessionToolCallUpdate{ + ToolCallId: acp.ToolCallId(toolCallID), + Status: &toolStatus, + RawInput: map[string]any{"prompt": "Unknown child"}, + Meta: codexCollaborationMeta(codexCollaborationSpawnAgent, nil), + } +} + +func mustMarshalPayload(t *testing.T, payload *streams.NormalizedPayload) string { + t.Helper() + encoded, err := json.Marshal(payload) + if err != nil { + t.Fatalf("marshal payload: %v", err) + } + return string(encoded) +} + +func assertActivePayloadUnchanged(t *testing.T, a *Adapter, emittedID, want string) { + t.Helper() + payload := a.activeToolCalls[emittedID] + if payload == nil { + t.Fatalf("active payload %q was deleted", emittedID) + } + if got := mustMarshalPayload(t, payload); got != want { + t.Fatalf("active payload %q mutated:\nbefore: %s\nafter: %s", emittedID, want, got) + } +} + +func codexChildlessCollaborationToolCall(toolCallID string) *acp.SessionUpdateToolCall { + return &acp.SessionUpdateToolCall{ + ToolCallId: acp.ToolCallId(toolCallID), + Kind: "other", + Title: codexCollaborationSpawnAgent, + Status: toolStatusInProgress, + RawInput: map[string]any{"prompt": "Unknown child"}, + Meta: codexCollaborationMeta(codexCollaborationSpawnAgent, nil), + } +} + +func newCodexCorrelationTestAdapter() *Adapter { + a := newTestAdapter() + a.agentID = codexAgentID + a.normalizer = NewNormalizer(codexAgentID) + return a +} + func TestNormalizeToolCall_PlainBashNotSubagent(t *testing.T) { n := NewNormalizer("") args := map[string]any{ diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/usage_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/usage_test.go index b4331a8b74..e39399f54f 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/usage_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/usage_test.go @@ -1,7 +1,7 @@ package acp import ( - "fmt" + "context" "testing" "github.com/coder/acp-go-sdk" @@ -174,15 +174,14 @@ func TestExtractUsage_DoesNotInterpretGrokPrivateReasoningTokens(t *testing.T) { } // TestUsageTracker_CumulativeDelta asserts the codex-acp fallback path: -// usage_update updates the cumulative used counter via tryConvertUntypedUpdate; -// consumeUsageDelta returns the running total and resets to zero so the next -// turn starts fresh. +// usage_update reports current context occupancy; consumeUsageDelta returns +// nonnegative growth since the previously consumed baseline. func TestUsageTracker_CumulativeDelta(t *testing.T) { a := newTestAdapter() const sess = "sess-codex" - a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 100), sess, 0) - a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 350), sess, 0) + a.convertUsageUpdate(sess, usageUpdate(200_000, 100)) + a.convertUsageUpdate(sess, usageUpdate(200_000, 350)) delta, cost := a.consumeUsageDelta(sess) if delta != 350 { @@ -192,37 +191,70 @@ func TestUsageTracker_CumulativeDelta(t *testing.T) { t.Errorf("first consume cost = %d, want 0 (no cost reported)", cost) } - a.tryConvertUntypedUpdate(usageUpdateRaw(200_000, 200), sess, 0) + // Compaction decreases occupancy. It resets both baselines instead of + // emitting a negative delta or recounting the restored context. + a.convertUsageUpdate(sess, usageUpdate(200_000, 200)) delta, _ = a.consumeUsageDelta(sess) - if delta != 200 { - t.Errorf("second consume delta = %d, want 200 (reset baseline)", delta) + if delta != 0 { + t.Errorf("post-compaction delta = %d, want 0", delta) + } + a.convertUsageUpdate(sess, usageUpdate(200_000, 260)) + delta, _ = a.consumeUsageDelta(sess) + if delta != 60 { + t.Errorf("growth after compaction = %d, want 60", delta) } } -// TestUsageTracker_ForwardsCost mirrors claude-acp where usage_update.cost.amount -// is the authoritative per-turn USD figure. The tracker stores the most -// recent value; consume returns it and resets. -func TestUsageTracker_ForwardsCost(t *testing.T) { +// TestUsageTracker_CumulativeUSDCost mirrors claude-acp where +// usage_update.cost.amount is authoritative cumulative USD session cost. +func TestUsageTracker_CumulativeUSDCost(t *testing.T) { a := newTestAdapter() const sess = "sess-claude" - a.tryConvertUntypedUpdate(usageUpdateCostRaw(200_000, 25_068, 0.06156125), sess, 0) + a.convertUsageUpdate(sess, usageUpdateWithCost(200_000, 25_068, 0.06156125, "USD")) _, cost := a.consumeUsageDelta(sess) if cost != 615 { t.Errorf("cost = %d, want 615 (subcents)", cost) } + a.convertUsageUpdate(sess, usageUpdateWithCost(200_000, 25_500, 0.07156125, "USD")) + _, cost = a.consumeUsageDelta(sess) + if cost != 100 { + t.Errorf("cumulative cost delta = %d, want 100", cost) + } + _, cost = a.consumeUsageDelta(sess) if cost != 0 { t.Errorf("second consume cost = %d, want 0", cost) } } -func usageUpdateCostRaw(size, used int64, costUSD float64) []byte { - return []byte(fmt.Sprintf( - `{"sessionId":"s1","update":{"sessionUpdate":"usage_update","size":%d,"used":%d,"cost":{"amount":%g,"currency":"USD"}}}`, - size, used, costUSD, - )) +func TestUsageTracker_IgnoresNonUSDCostAndResetsDecreases(t *testing.T) { + a := newTestAdapter() + const sess = "sess-cost-currency" + + a.convertUsageUpdate(sess, usageUpdateWithCost(200_000, 100, 2.5, "EUR")) + _, cost := a.consumeUsageDelta(sess) + if cost != 0 { + t.Fatalf("EUR cost = %d, want 0", cost) + } + + a.convertUsageUpdate(sess, usageUpdateWithCost(200_000, 110, 0.05, "USD")) + _, cost = a.consumeUsageDelta(sess) + if cost != 500 { + t.Fatalf("initial USD cost = %d, want 500", cost) + } + a.convertUsageUpdate(sess, usageUpdateWithCost(200_000, 120, 0.01, "USD")) + _, cost = a.consumeUsageDelta(sess) + if cost != 0 { + t.Fatalf("decreased USD cumulative cost = %d, want 0", cost) + } +} + +func usageUpdateWithCost(size, used int64, amount float64, currency string) *acp.SessionUsageUpdate { + update := usageUpdate(size, used) + update.Cost = &acp.Cost{Amount: amount, Currency: currency} + return update } // TestConsumeUsageDelta_UnknownSession returns zero for sessions that @@ -235,3 +267,18 @@ func TestConsumeUsageDelta_UnknownSession(t *testing.T) { t.Errorf("unknown session = (%d, %d), want (0, 0)", d, c) } } + +func TestNewSession_ClearsUsageTrackers(t *testing.T) { + a, _ := setupConcurrencyFakeAgent(t) + if err := a.Initialize(context.Background()); err != nil { + t.Fatalf("Initialize: %v", err) + } + a.convertUsageUpdate("old-session", usageUpdate(200_000, 500)) + + if _, err := a.NewSession(context.Background(), nil); err != nil { + t.Fatalf("NewSession: %v", err) + } + if got := len(a.usageBySession); got != 0 { + t.Fatalf("usage tracker count = %d, want 0", got) + } +} diff --git a/apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go b/apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go index 7c1fa974d7..3458e56729 100644 --- a/apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go +++ b/apps/backend/internal/agentctl/server/adapter/transport/acp/wakeup_concurrency_test.go @@ -56,6 +56,9 @@ func (f *concurrencyFakeAgent) Cancel(_ context.Context, _ acp.CancelNotificatio func (f *concurrencyFakeAgent) CloseSession(_ context.Context, _ acp.CloseSessionRequest) (acp.CloseSessionResponse, error) { return acp.CloseSessionResponse{}, nil } +func (f *concurrencyFakeAgent) DeleteSession(_ context.Context, _ acp.DeleteSessionRequest) (acp.DeleteSessionResponse, error) { + return acp.DeleteSessionResponse{}, acp.NewMethodNotFound(acp.AgentMethodSessionDelete) +} func (f *concurrencyFakeAgent) ListSessions(_ context.Context, _ acp.ListSessionsRequest) (acp.ListSessionsResponse, error) { return acp.ListSessionsResponse{}, nil } diff --git a/apps/backend/internal/agentctl/types/streams/agent.go b/apps/backend/internal/agentctl/types/streams/agent.go index 725761a2e9..95d12c84ea 100644 --- a/apps/backend/internal/agentctl/types/streams/agent.go +++ b/apps/backend/internal/agentctl/types/streams/agent.go @@ -110,6 +110,10 @@ type AgentEvent struct { // Text contains streaming text content from the agent. Text string `json:"text,omitempty"` + // ProtocolMessageID identifies the source-protocol message this chunk belongs to. + // It is distinct from the Kandev message record ID used by downstream streaming. + ProtocolMessageID string `json:"protocol_message_id,omitempty"` + // --- Reasoning fields (for "reasoning" type) --- // ReasoningText contains full reasoning/chain-of-thought content. diff --git a/apps/desktop/src-tauri/src/backend.rs b/apps/desktop/src-tauri/src/backend.rs index b93076a21a..e86e0eea48 100644 --- a/apps/desktop/src-tauri/src/backend.rs +++ b/apps/desktop/src-tauri/src/backend.rs @@ -916,15 +916,10 @@ mod tests { } #[test] - fn pick_available_loopback_port_returns_preferred_port() { - let listener = TcpListener::bind((LOOPBACK_HOST, 0)).expect("reserve candidate"); - let port = listener.local_addr().expect("candidate address").port(); - drop(listener); + fn pick_available_loopback_port_accepts_kernel_assigned_port() { + let port = pick_available_loopback_port(0).expect("kernel-assigned port"); - assert_eq!( - pick_available_loopback_port(port).expect("preferred port"), - port - ); + assert_ne!(port, 0); } #[test] diff --git a/apps/web/app/office/tasks/[id]/advanced-panels/chat-panel.tsx b/apps/web/app/office/tasks/[id]/advanced-panels/chat-panel.tsx index 4949426fb1..099c87840a 100644 --- a/apps/web/app/office/tasks/[id]/advanced-panels/chat-panel.tsx +++ b/apps/web/app/office/tasks/[id]/advanced-panels/chat-panel.tsx @@ -8,6 +8,7 @@ import { Input } from "@kandev/ui/input"; import { useSessionMessages } from "@/hooks/domains/session/use-session-messages"; import { useSession } from "@/hooks/domains/session/use-session"; import { useSessionLaunch } from "@/hooks/domains/session/use-session-launch"; +import { useSessionTurn } from "@/hooks/domains/session/use-session-turn"; import { useAppStore } from "@/components/state-provider"; import { useFileEditors } from "@/hooks/use-file-editors"; import { getWebSocketClient } from "@/lib/ws/connection"; @@ -63,6 +64,7 @@ function MessageList({ sessionId, worktreePath, onOpenFile, + activeTurnId, scrollRef, }: { messages: Message[]; @@ -71,6 +73,7 @@ function MessageList({ sessionId: string | null; worktreePath?: string; onOpenFile?: (path: string) => void; + activeTurnId: string | null; scrollRef: React.RefObject; }) { if (isLoading && messages.length === 0) { @@ -94,6 +97,7 @@ function MessageList({ sessionId={sessionId ?? undefined} worktreePath={worktreePath} onOpenFile={onOpenFile} + isContainingTurnActive={Boolean(activeTurnId && msg.turn_id === activeTurnId)} /> ))} @@ -140,6 +144,7 @@ export function AdvancedChatPanel({ taskId, sessionId, hideInput }: AdvancedChat const scrollRef = useRef(null); const { session } = useSession(sessionId); + const { activeTurnId } = useSessionTurn(sessionId); const { messages, isLoading } = useSessionMessages(sessionId); const { openFile } = useFileEditors(); const agentProfiles = useAppStore((s) => s.agentProfiles.items ?? []); @@ -207,6 +212,7 @@ export function AdvancedChatPanel({ taskId, sessionId, hideInput }: AdvancedChat sessionId={sessionId} worktreePath={session?.worktree_path} onOpenFile={openFile} + activeTurnId={activeTurnId} scrollRef={scrollRef} /> {!hideInput && ( diff --git a/apps/web/components/task/chat/message-list-native.tsx b/apps/web/components/task/chat/message-list-native.tsx index c96a1a6478..0a10de6bbb 100644 --- a/apps/web/components/task/chat/message-list-native.tsx +++ b/apps/web/components/task/chat/message-list-native.tsx @@ -5,6 +5,7 @@ import { SessionPanelContent } from "@kandev/ui/pannel-session"; import { useDockviewStore } from "@/lib/state/dockview-store"; import type { Message } from "@/lib/types/http"; import { useLazyLoadMessages } from "@/hooks/use-lazy-load-messages"; +import { useSessionTurn } from "@/hooks/domains/session/use-session-turn"; import { MessageListFooter } from "./message-list-footer"; import { type MessageListProps, @@ -12,7 +13,7 @@ import { MessageItem, getItemKey, getConversationLoadingState, - getSessionRunningState, + getEffectiveActiveTurnId, getLastTurnGroupId, getStreamingAgentMessageId, } from "./message-list-shared"; @@ -194,7 +195,8 @@ export const NativeMessageList = memo(function NativeMessageList({ sessionState, }); const { loadMore, hasMore, isLoading: isLoadingMore } = useLazyLoadMessages(sessionId); - const isRunning = getSessionRunningState(sessionState); + const { activeTurnId } = useSessionTurn(sessionId); + const effectiveActiveTurnId = getEffectiveActiveTurnId(activeTurnId, isWorking); const streamingMessageId = getStreamingAgentMessageId(messages); const lastTurnGroupId = useMemo(() => getLastTurnGroupId(items), [items]); const handleScrollToMessage = useScrollToMessage(); @@ -247,7 +249,7 @@ export const NativeMessageList = memo(function NativeMessageList({ worktreePath={worktreePath} onOpenFile={onOpenFile} isLastGroup={item.type === "turn_group" && item.id === lastTurnGroupId} - isTurnActive={isRunning} + activeTurnId={effectiveActiveTurnId} streamingMessageId={streamingMessageId} onScrollToMessage={handleScrollToMessage} /> diff --git a/apps/web/components/task/chat/message-list-shared.test.tsx b/apps/web/components/task/chat/message-list-shared.test.tsx index 64a8f36ce0..d3d4914b58 100644 --- a/apps/web/components/task/chat/message-list-shared.test.tsx +++ b/apps/web/components/task/chat/message-list-shared.test.tsx @@ -6,6 +6,7 @@ import type { RenderItem } from "@/hooks/use-processed-messages"; import type { Message } from "@/lib/types/http"; const rendererSpy = vi.fn(); +const turnGroupSpy = vi.fn(); const mockStoreState = vi.hoisted(() => ({ taskSessions: { items: { @@ -31,7 +32,10 @@ vi.mock("@/components/task/chat/message-renderer", () => ({ }, })); vi.mock("@/components/task/chat/messages/turn-group-message", () => ({ - TurnGroupMessage: () =>
, + TurnGroupMessage: (props: unknown) => { + turnGroupSpy(props); + return
; + }, })); vi.mock("@/components/session/prepare-progress", () => ({ PrepareProgress: () =>
, @@ -65,6 +69,7 @@ import { MessageItem, MessageListStatus, getConversationLoadingState, + getEffectiveActiveTurnId, getStreamingAgentMessageId, } from "./message-list-shared"; @@ -73,6 +78,16 @@ const noop = () => {}; const perm = new Map(); const kids = new Map(); +describe("getEffectiveActiveTurnId", () => { + it("preserves the active turn while the session is working", () => { + expect(getEffectiveActiveTurnId("turn-active", true)).toBe("turn-active"); + }); + + it("ignores a stale active turn after the session settles", () => { + expect(getEffectiveActiveTurnId("turn-stale", false)).toBeNull(); + }); +}); + function row(onOpenFile: (p: string) => void) { return ( void) { worktreePath="/wt" onOpenFile={onOpenFile} isLastGroup={false} - isTurnActive={false} + activeTurnId={null} onScrollToMessage={noop} /> ); @@ -103,6 +118,7 @@ function Harness({ onOpenFile }: { onOpenFile: (p: string) => void }) { describe("MessageItem memo boundary", () => { afterEach(() => { rendererSpy.mockClear(); + turnGroupSpy.mockClear(); }); it("does not re-render the row when the parent re-renders with stable props", () => { @@ -139,7 +155,7 @@ describe("MessageItem agent error notice", () => { childrenByParentToolCallId={kids} taskId="t1" isLastGroup={false} - isTurnActive={false} + activeTurnId={null} onScrollToMessage={noop} />, ); @@ -149,6 +165,69 @@ describe("MessageItem agent error notice", () => { }); }); +describe("MessageItem containing turn activity", () => { + afterEach(() => { + rendererSpy.mockClear(); + turnGroupSpy.mockClear(); + }); + + it("does not reactivate a historical message while another turn is active", () => { + const historicalItem: RenderItem = { + type: "message", + message: { id: "historical", turn_id: "turn-old" } as Message, + }; + render( + , + ); + + expect(rendererSpy).toHaveBeenLastCalledWith( + expect.objectContaining({ isContainingTurnActive: false }), + ); + }); + + it("marks only the matching turn group active", () => { + const historicalGroup: RenderItem = { + type: "turn_group", + id: "group-old", + turnId: "turn-old", + messages: [], + }; + const { rerender } = render( + , + ); + expect(turnGroupSpy).toHaveBeenLastCalledWith(expect.objectContaining({ isTurnActive: false })); + + rerender( + , + ); + expect(turnGroupSpy).toHaveBeenLastCalledWith(expect.objectContaining({ isTurnActive: true })); + }); +}); + describe("getConversationLoadingState", () => { it("shows loading while conversation history is still fetching with initial content rendered", () => { expect( diff --git a/apps/web/components/task/chat/message-list-shared.tsx b/apps/web/components/task/chat/message-list-shared.tsx index 4a4362ce06..81f9881126 100644 --- a/apps/web/components/task/chat/message-list-shared.tsx +++ b/apps/web/components/task/chat/message-list-shared.tsx @@ -43,8 +43,11 @@ export function getItemKey(item: RenderItem): string { return item.message.id; } -export function getSessionRunningState(sessionState: string | null | undefined) { - return sessionState === "CREATED" || sessionState === "STARTING" || sessionState === "RUNNING"; +export function getEffectiveActiveTurnId( + activeTurnId: string | null, + isWorking: boolean, +): string | null { + return isWorking ? activeTurnId : null; } /** Only the latest ordinary agent row in the active turn can be the streaming reply. */ @@ -240,7 +243,7 @@ export const MessageItem = memo(function MessageItem({ worktreePath, onOpenFile, isLastGroup, - isTurnActive, + activeTurnId, streamingMessageId, onScrollToMessage, }: { @@ -252,7 +255,7 @@ export const MessageItem = memo(function MessageItem({ worktreePath?: string; onOpenFile?: (path: string) => void; isLastGroup: boolean; - isTurnActive: boolean; + activeTurnId?: string | null; streamingMessageId?: string | null; onScrollToMessage: (id: string) => void; }) { @@ -263,6 +266,7 @@ export const MessageItem = memo(function MessageItem({ return ; } if (item.type === "turn_group") { + const isContainingTurnActive = Boolean(activeTurnId && item.turnId === activeTurnId); return ( message.id === streamingMessageId) - : false) - } + isTurnActive={isContainingTurnActive} streamingMessageId={streamingMessageId} onScrollToMessage={onScrollToMessage} /> ); } + const isContainingTurnActive = Boolean(activeTurnId && item.message.turn_id === activeTurnId); return ( diff --git a/apps/web/components/task/chat/message-list-virtuoso.tsx b/apps/web/components/task/chat/message-list-virtuoso.tsx index c3747dbc02..20734ea1cb 100644 --- a/apps/web/components/task/chat/message-list-virtuoso.tsx +++ b/apps/web/components/task/chat/message-list-virtuoso.tsx @@ -7,6 +7,7 @@ import { SessionPanelContent } from "@kandev/ui/pannel-session"; import type { RenderItem } from "@/hooks/use-processed-messages"; import type { Message, TaskSessionState } from "@/lib/types/http"; import { useLazyLoadMessages } from "@/hooks/use-lazy-load-messages"; +import { useSessionTurn } from "@/hooks/domains/session/use-session-turn"; import { MessageListFooter } from "./message-list-footer"; import { type MessageListProps, @@ -14,7 +15,7 @@ import { MessageItem, getItemKey, getConversationLoadingState, - getSessionRunningState, + getEffectiveActiveTurnId, getLastTurnGroupId, getStreamingAgentMessageId, } from "./message-list-shared"; @@ -28,7 +29,7 @@ const debugFirstIndex = createDebugLogger("chat:virtuoso:firstIndex"); type VirtuosoBodyProps = MessageListProps & { scrollParent: HTMLDivElement; - isRunning: boolean; + activeTurnId: string | null; lastTurnGroupId: string | null; hasMore: boolean; isLoadingMore: boolean; @@ -105,7 +106,7 @@ function useVirtuosoCallbacks(props: VirtuosoBodyProps) { childrenByParentToolCallId, taskId, } = props; - const { worktreePath, onOpenFile, lastTurnGroupId, isRunning } = props; + const { worktreePath, onOpenFile, lastTurnGroupId, activeTurnId } = props; const { hasMore, isLoadingMore, loadMore } = props; const virtuosoRef = useRef(null); const itemCount = items.length; @@ -162,7 +163,7 @@ function useVirtuosoCallbacks(props: VirtuosoBodyProps) { worktreePath={worktreePath} onOpenFile={onOpenFile} isLastGroup={item.type === "turn_group" && item.id === lastTurnGroupId} - isTurnActive={isRunning} + activeTurnId={activeTurnId} streamingMessageId={streamingMessageId} onScrollToMessage={handleScrollToMessage} /> @@ -179,7 +180,7 @@ function useVirtuosoCallbacks(props: VirtuosoBodyProps) { worktreePath, onOpenFile, lastTurnGroupId, - isRunning, + activeTurnId, streamingMessageId, handleScrollToMessage, ], @@ -209,10 +210,10 @@ function VirtuosoBody(props: VirtuosoBodyProps) { firstItemIndex, initialTopMostItemIndex: itemCount - 1, hasMore: props.hasMore, - isRunning: props.isRunning, + activeTurnId: props.activeTurnId, lastTurnGroupId: props.lastTurnGroupId ?? "-", }); - }, [itemCount, firstItemIndex, props.hasMore, props.isRunning, props.lastTurnGroupId]); + }, [itemCount, firstItemIndex, props.hasMore, props.activeTurnId, props.lastTurnGroupId]); return ( getLastTurnGroupId(items), [items]); // Track which render branch fires and how itemCount/messageCount transition. @@ -522,7 +524,7 @@ export const VirtuosoMessageList = memo(function VirtuosoMessageList(props: Mess void; onScrollToMessage?: (messageId: string) => void; isTurnActive?: boolean; + isContainingTurnActive?: boolean; }; function TaskDescriptionStartButton({ taskId, sessionId }: { taskId: string; sessionId: string }) { @@ -243,6 +244,7 @@ const adapters: MessageAdapter[] = [ void; onScrollToMessage?: (messageId: string) => void; isTurnActive?: boolean; + isContainingTurnActive?: boolean; }; export const MessageRenderer = memo(function MessageRenderer({ @@ -386,6 +389,7 @@ export const MessageRenderer = memo(function MessageRenderer({ onOpenFile, onScrollToMessage, isTurnActive = false, + isContainingTurnActive = false, }: MessageRendererProps) { const ctx = { isTaskDescription, @@ -397,6 +401,7 @@ export const MessageRenderer = memo(function MessageRenderer({ onOpenFile, onScrollToMessage, isTurnActive, + isContainingTurnActive, }; const adapter = adapters.find((entry) => entry.matches(comment, ctx)) ?? adapters[adapters.length - 1]; diff --git a/apps/web/components/task/chat/messages/tool-subagent-message.test.tsx b/apps/web/components/task/chat/messages/tool-subagent-message.test.tsx index 6fdc265fe6..919c4a3a83 100644 --- a/apps/web/components/task/chat/messages/tool-subagent-message.test.tsx +++ b/apps/web/components/task/chat/messages/tool-subagent-message.test.tsx @@ -1,128 +1,288 @@ -import { describe, it, expect, afterEach } from "vitest"; -import { render, screen, fireEvent, cleanup } from "@testing-library/react"; -import { ToolSubagentMessage } from "@/components/task/chat/messages/tool-subagent-message"; -import type { Message } from "@/lib/types/http"; - -// Guards the subagent card's expand/collapse behavior. Regression coverage for -// "silent" subagents (a result_text with no child tool calls) rendering -// permanently expanded: auto-expand must key on the running state alone, not on -// result_text, so completed cards collapse to their header + metadata row like -// subagents that stream child tool calls. - -function subagentMessage(opts: { - status: string; - result_text?: string; - description?: string; -}): Message { - return { - id: "m1", - content: "Subagent", - metadata: { - status: opts.status, - normalized: { - kind: "subagent_task", - subagent_task: { - description: opts.description ?? "Summarize the repo", - subagent_type: "general-purpose", - status: opts.status, - result_text: opts.result_text, - }, - }, - }, - } as unknown as Message; -} - -const noopRenderChild = () => null; -const SILENT_RESULT = "Found 42 files across 7 packages."; -const RESULT_TEXT = "subagent-result-text"; +import { cleanup, fireEvent, render, screen } from "@testing-library/react"; +import { afterEach, describe, expect, it } from "vitest"; +import { sessionId as toSessionId, taskId as toTaskId, type Message } from "@/lib/types/http"; +import type { ToolCallMetadata } from "@/components/task/chat/types"; +import { isSubagentEffectivelyActive, ToolSubagentMessage } from "./tool-subagent-message"; afterEach(cleanup); -describe("ToolSubagentMessage expansion", () => { - it("collapses a completed subagent with child tool calls to its header", () => { - const child = { id: "c1", content: "sleep 30", metadata: {} } as unknown as Message; - render( -
sleep 30
} - />, - ); - expect(screen.queryByTestId("child-body")).toBeNull(); - }); +const COMPLETE = "complete"; +const SUBAGENT_CHEVRON = "subagent-chevron"; +const IN_PROGRESS = "in_progress"; +const STARTED = "started"; +const SUBAGENT_WORKING = "Subagent working..."; - it("collapses a completed silent subagent (result_text, no children)", () => { - render( - , - ); - // Completed and no longer running -> collapsed: result text stays hidden - // until the user opens the card. The metadata row remains visible. - expect(screen.queryByTestId(RESULT_TEXT)).toBeNull(); - expect(screen.getByTestId("subagent-type")).toBeTruthy(); - }); +function subagentMessage({ + metadataStatus = "in_progress", + payloadStatus = "started", + prompt, + resultText, + durationMs, +}: { + metadataStatus?: ToolCallMetadata["status"]; + payloadStatus?: string; + prompt?: string; + resultText?: string; + durationMs?: number; +} = {}): Message { + return { + id: "codex-subagent-1", + session_id: toSessionId("session-1"), + task_id: toTaskId("task-1"), + author_type: "agent", + type: "tool_call", + content: "ten_second_probe", + created_at: "2026-07-23T12:00:00Z", + metadata: { + status: metadataStatus, + tool_call_id: "codex-subagent-tool-1", + normalized: { + kind: "subagent_task", + subagent_task: { + description: "ten_second_probe", + subagent_type: "subagent", + status: payloadStatus, + child_session_id: "child-session-123456", + prompt, + result_text: resultText, + duration_ms: durationMs, + }, + }, + }, + }; +} - it("reveals the silent subagent's result text when expanded", () => { - render( - , - ); - fireEvent.click(screen.getByRole("button")); - expect(screen.getByTestId(RESULT_TEXT).textContent).toContain(SILENT_RESULT); - }); +function childTool(id: string, content: string): Message { + return { + id, + session_id: toSessionId("session-1"), + task_id: toTaskId("task-1"), + author_type: "agent", + type: "tool_call", + content, + created_at: "2026-07-23T12:00:01Z", + metadata: { status: "complete", tool_call_id: id }, + }; +} - it("auto-expands while the subagent is running, then auto-collapses on completion", () => { - const running = subagentMessage({ status: "running" }); - const { rerender } = render( - , - ); - // Running -> auto-expanded working indicator is shown. - expect(screen.queryByText("Subagent working...")).not.toBeNull(); +function renderSubagent( + comment: Message, + { + childMessages = [], + isContainingTurnActive = false, + }: { childMessages?: Message[]; isContainingTurnActive?: boolean } = {}, +) { + return render( + {message.content}} + />, + ); +} - rerender( - , - ); - // Completed without a manual override -> auto-collapsed. - expect(screen.queryByTestId(RESULT_TEXT)).toBeNull(); - }); +describe("isSubagentEffectivelyActive", () => { + it.each<{ + name: string; + metadataStatus: ToolCallMetadata["status"]; + payloadStatus: string; + isContainingTurnActive: boolean; + expected: boolean; + }>([ + { + name: "in-progress metadata is active during its turn without a started payload", + metadataStatus: IN_PROGRESS, + payloadStatus: "queued", + isContainingTurnActive: true, + expected: true, + }, + { + name: "in-progress metadata settles with its turn without a started payload", + metadataStatus: IN_PROGRESS, + payloadStatus: "queued", + isContainingTurnActive: false, + expected: false, + }, + { + name: "started payload with pending metadata is active during its turn", + metadataStatus: "pending", + payloadStatus: STARTED, + isContainingTurnActive: true, + expected: true, + }, + { + name: "started payload with pending metadata settles with its turn", + metadataStatus: "pending", + payloadStatus: STARTED, + isContainingTurnActive: false, + expected: false, + }, + { + name: "started payload without metadata status is active during its turn", + metadataStatus: undefined, + payloadStatus: STARTED, + isContainingTurnActive: true, + expected: true, + }, + { + name: "started payload without metadata status settles with its turn", + metadataStatus: undefined, + payloadStatus: STARTED, + isContainingTurnActive: false, + expected: false, + }, + { + name: "running metadata stays active without a containing-turn signal", + metadataStatus: "running", + payloadStatus: "queued", + isContainingTurnActive: false, + expected: true, + }, + { + name: "terminal metadata overrides a started payload in an active turn", + metadataStatus: COMPLETE, + payloadStatus: STARTED, + isContainingTurnActive: true, + expected: false, + }, + ])("$name", ({ metadataStatus, payloadStatus, isContainingTurnActive, expected }) => { + const message = subagentMessage({ metadataStatus, payloadStatus }); + const metadata = message.metadata as ToolCallMetadata; + if (metadataStatus === undefined) delete metadata.status; - it("keeps a manual collapse after the subagent completes", () => { - const { rerender } = render( - , - ); - // User collapses the running card. - fireEvent.click(screen.getByRole("button")); - expect(screen.queryByText("Subagent working...")).toBeNull(); - - // Completion emits result_text; the manual collapse must survive (it used - // to be wiped, forcing the card back open). - rerender( - , - ); - expect(screen.queryByTestId(RESULT_TEXT)).toBeNull(); + expect(isSubagentEffectivelyActive(metadata, isContainingTurnActive)).toBe(expected); + }); +}); + +describe("ToolSubagentMessage", () => { + it("does not expose a toggle for a settled contentless Codex subagent", () => { + renderSubagent(subagentMessage()); + + expect(screen.getByTestId("subagent-type").textContent).toContain("subagent"); + expect(screen.getByTestId("subagent-meta-session")).toBeTruthy(); + expect(screen.queryByTestId(SUBAGENT_CHEVRON)).toBeNull(); + expect(screen.queryByRole("button")).toBeNull(); + + fireEvent.click(screen.getByTestId("subagent-header")); + expect(screen.queryByText(SUBAGENT_WORKING)).toBeNull(); + }); + + it("shows work only while a stale Codex lifecycle is in an active turn", () => { + const comment = subagentMessage(); + const { rerender } = renderSubagent(comment, { isContainingTurnActive: true }); + + expect(screen.getByRole("status", { name: "Loading" })).toBeTruthy(); + expect(screen.getByText(SUBAGENT_WORKING)).toBeTruthy(); + + rerender( + {message.content}} + />, + ); + + expect(screen.queryByRole("status", { name: "Loading" })).toBeNull(); + expect(screen.queryByText(SUBAGENT_WORKING)).toBeNull(); + expect(screen.queryByRole("button")).toBeNull(); + }); +}); + +describe("ToolSubagentMessage expansion", () => { + it("expands nested child tools and keeps their count", () => { + const childMessages = [ + childTool("child-1", "first child"), + childTool("child-2", "second child"), + ]; + renderSubagent(subagentMessage({ metadataStatus: COMPLETE, payloadStatus: COMPLETE }), { + childMessages, + }); + + expect(screen.getByTestId("subagent-child-count").textContent).toBe("2 tool calls"); + expect(screen.getByTestId(SUBAGENT_CHEVRON)).toBeTruthy(); + expect(screen.queryByText("first child")).toBeNull(); + + fireEvent.click(screen.getByRole("button")); + expect(screen.getByText("first child")).toBeTruthy(); + expect(screen.getByText("second child")).toBeTruthy(); + }); + + it("keeps completed result-only subagents collapsed but expandable", () => { + renderSubagent( + subagentMessage({ + metadataStatus: COMPLETE, + payloadStatus: COMPLETE, + resultText: "Probe completed successfully", + }), + ); + + expect(screen.getByRole("button").getAttribute("aria-expanded")).toBe("false"); + expect(screen.queryByTestId("subagent-result-text")).toBeNull(); + + fireEvent.click(screen.getByRole("button")); + expect(screen.getByTestId("subagent-result-text").textContent).toBe( + "Probe completed successfully", + ); + }); + + it("keeps a manual collapse after an active subagent completes", () => { + const activeComment = subagentMessage({ + metadataStatus: "running", + payloadStatus: STARTED, + }); + const { rerender } = renderSubagent(activeComment); + + expect(screen.getByText(SUBAGENT_WORKING)).toBeTruthy(); + fireEvent.click(screen.getByRole("button")); + expect(screen.queryByText(SUBAGENT_WORKING)).toBeNull(); + + rerender( + {message.content}} + />, + ); + + expect(screen.queryByTestId("subagent-result-text")).toBeNull(); + }); + + it("keeps prompt-only subagents expandable", () => { + renderSubagent( + subagentMessage({ + metadataStatus: COMPLETE, + payloadStatus: COMPLETE, + prompt: "Inspect the lifecycle events", + }), + ); + + expect(screen.getByTestId(SUBAGENT_CHEVRON)).toBeTruthy(); + expect(screen.queryByText("Inspect the lifecycle events")).toBeNull(); + + fireEvent.click(screen.getByRole("button")); + expect(screen.getByText("Inspect the lifecycle events")).toBeTruthy(); + }); + + it("renders a completed contentless card as settled metadata", () => { + renderSubagent( + subagentMessage({ + metadataStatus: COMPLETE, + payloadStatus: COMPLETE, + durationMs: 2500, + }), + ); + + expect(screen.getByTestId("subagent-meta-session")).toBeTruthy(); + expect(screen.getByTestId("subagent-meta-duration").textContent).toBe("2.5s"); + expect(screen.queryByRole("status", { name: "Loading" })).toBeNull(); + expect(screen.queryByTestId(SUBAGENT_CHEVRON)).toBeNull(); + expect(screen.queryByRole("button")).toBeNull(); }); }); diff --git a/apps/web/components/task/chat/messages/tool-subagent-message.tsx b/apps/web/components/task/chat/messages/tool-subagent-message.tsx index ead4eaecd4..4899560566 100644 --- a/apps/web/components/task/chat/messages/tool-subagent-message.tsx +++ b/apps/web/components/task/chat/messages/tool-subagent-message.tsx @@ -8,22 +8,10 @@ import type { Message } from "@/lib/types/http"; import type { SubagentTaskPayload, ToolCallMetadata } from "@/components/task/chat/types"; import { SubagentMetaRow } from "@/components/task/chat/messages/subagent-meta-row"; -// deriveSubagentBody picks which secondary block (result text or prompt) the -// card should render below the header. Result text wins because it carries the -// silent-subagent summary that prompt placeholders cannot substitute for. -function deriveSubagentBody( - childCount: number, - payload: SubagentTaskPayload | undefined, -): { resultText?: string; prompt?: string } { - if (childCount > 0) return {}; - if (payload?.result_text) return { resultText: payload.result_text }; - if (payload?.prompt) return { prompt: payload.prompt }; - return {}; -} - type ToolSubagentMessageProps = { comment: Message; childMessages: Message[]; + isContainingTurnActive?: boolean; worktreePath?: string; onOpenFile?: (path: string) => void; renderChild: (message: Message) => React.ReactNode; @@ -37,17 +25,66 @@ function arePropsEqual( return ( prevProps.comment === nextProps.comment && prevProps.childMessages === nextProps.childMessages && + prevProps.isContainingTurnActive === nextProps.isContainingTurnActive && prevProps.worktreePath === nextProps.worktreePath && prevProps.onOpenFile === nextProps.onOpenFile ); } +const TERMINAL_TOOL_STATUSES = new Set([ + "complete", + "completed", + "success", + "error", + "failed", + "cancelled", +]); + +export function isSubagentEffectivelyActive( + metadata: ToolCallMetadata | undefined, + isContainingTurnActive: boolean, +): boolean { + const status = metadata?.status; + if (status && TERMINAL_TOOL_STATUSES.has(status)) return false; + if (status === "running") return true; + const payloadStatus = metadata?.normalized?.subagent_task?.status; + return isContainingTurnActive && (status === "in_progress" || payloadStatus === "started"); +} + +function deriveSubagentBody( + childCount: number, + subagentTask: SubagentTaskPayload | undefined, +): { resultText?: string; prompt?: string } { + if (childCount > 0) return {}; + if (subagentTask?.result_text) return { resultText: subagentTask.result_text }; + if (subagentTask?.prompt) return { prompt: subagentTask.prompt }; + return {}; +} + +function deriveSubagentDisplay( + metadata: ToolCallMetadata | undefined, + childCount: number, + isContainingTurnActive: boolean, +) { + const subagentTask = metadata?.normalized?.subagent_task; + const isActive = isSubagentEffectivelyActive(metadata, isContainingTurnActive); + const { resultText, prompt } = deriveSubagentBody(childCount, subagentTask); + return { + subagentTask, + isActive, + resultText, + prompt, + hasExpandableContent: childCount > 0 || Boolean(resultText) || Boolean(prompt) || isActive, + }; +} + type SubagentHeaderProps = { isExpanded: boolean; subagentType: string; description: string; isActive: boolean; childCount: number; + hasExpandableContent: boolean; onToggle: () => void; }; @@ -57,22 +94,23 @@ function SubagentHeader({ description, isActive, childCount, + hasExpandableContent, onToggle, }: SubagentHeaderProps) { - return ( - ); } @@ -160,6 +221,7 @@ function SubagentContent({ export const ToolSubagentMessage = memo(function ToolSubagentMessage({ comment, childMessages, + isContainingTurnActive = false, // eslint-disable-next-line @typescript-eslint/no-unused-vars worktreePath, // eslint-disable-next-line @typescript-eslint/no-unused-vars @@ -167,17 +229,11 @@ export const ToolSubagentMessage = memo(function ToolSubagentMessage({ renderChild, }: ToolSubagentMessageProps) { const metadata = comment.metadata as ToolCallMetadata | undefined; - const status = metadata?.status; - const normalized = metadata?.normalized; - const subagentTask = normalized?.subagent_task; - + const childCount = childMessages.length; + const { subagentTask, isActive, resultText, prompt, hasExpandableContent } = + deriveSubagentDisplay(metadata, childCount, isContainingTurnActive); const description = subagentTask?.description || comment.content || "Subagent"; const subagentType = subagentTask?.subagent_type || "Task"; - const childCount = childMessages.length; - - const isActive = status === "running"; - const showMeta = !isActive; - const { resultText, prompt } = deriveSubagentBody(childCount, subagentTask); // Track manual override state - null means "use auto behavior" const [manualExpandState, setManualExpandState] = useState(null); @@ -191,7 +247,7 @@ export const ToolSubagentMessage = memo(function ToolSubagentMessage({ const autoExpanded = isActive; // Derive expanded state: manual override takes precedence, otherwise use auto - const isExpanded = manualExpandState ?? autoExpanded; + const isExpanded = hasExpandableContent && (manualExpandState ?? autoExpanded); const handleToggle = useCallback(() => { setManualExpandState((prev) => !(prev ?? autoExpanded)); @@ -205,9 +261,10 @@ export const ToolSubagentMessage = memo(function ToolSubagentMessage({ description={description} isActive={isActive} childCount={childCount} + hasExpandableContent={hasExpandableContent} onToggle={handleToggle} /> - {showMeta && } + {!isActive && } , + ); +} + describe("TurnGroupMessage repeated tool compaction", () => { it("summarizes the middle of a long run of identical terminal commands", () => { const messages = Array.from({ length: 6 }, (_, i) => toolExecute(`tool-${i + 1}`)); @@ -109,3 +152,15 @@ describe("TurnGroupMessage repeated tool compaction", () => { expect(html).not.toContain('aria-label="Loading"'); }); }); + +describe("TurnGroupMessage Codex subagent activity", () => { + it("runs the group only while the stale lifecycle belongs to an active turn", () => { + const activeHtml = renderCodexSubagentGroup(true); + const settledHtml = renderCodexSubagentGroup(false); + + expect(activeHtml).toContain('aria-label="Loading"'); + expect(activeHtml).toContain("Subagent working..."); + expect(settledHtml).not.toContain('aria-label="Loading"'); + expect(settledHtml).not.toContain("Subagent working..."); + }); +}); diff --git a/apps/web/components/task/chat/messages/turn-group-message.tsx b/apps/web/components/task/chat/messages/turn-group-message.tsx index 24d56bf8ea..600e98b45a 100644 --- a/apps/web/components/task/chat/messages/turn-group-message.tsx +++ b/apps/web/components/task/chat/messages/turn-group-message.tsx @@ -8,6 +8,7 @@ import type { Message } from "@/lib/types/http"; import type { TurnGroup } from "@/hooks/use-processed-messages"; import type { ToolCallMetadata } from "@/components/task/chat/types"; import { MessageRenderer } from "@/components/task/chat/message-renderer"; +import { isSubagentEffectivelyActive } from "@/components/task/chat/messages/tool-subagent-message"; type TurnGroupMessageProps = { group: TurnGroup; @@ -92,12 +93,16 @@ const TOOL_MESSAGE_TYPES = new Set([ * Check if any tool or subagent in the group is still running. * A tool/subagent is considered running if it's not in a terminal state. */ -function hasRunningTool(messages: Message[]): boolean { +function hasRunningTool(messages: Message[], isContainingTurnActive: boolean): boolean { for (const msg of messages) { const metadata = msg.metadata as ToolCallMetadata | undefined; const isToolMessage = msg.type && TOOL_MESSAGE_TYPES.has(msg.type); const isSubagent = metadata?.normalized?.kind === "subagent_task"; if (!isToolMessage && !isSubagent) continue; + if (isSubagent) { + if (isSubagentEffectivelyActive(metadata, isContainingTurnActive)) return true; + continue; + } const status = metadata?.status; if (status !== "complete" && status !== "error" && status !== "cancelled") return true; } @@ -280,6 +285,7 @@ function renderMessageEntry(message: Message, props: MessageRenderProps) { worktreePath={props.worktreePath} sessionId={props.sessionId ?? undefined} isTurnActive={props.isTurnActive && message.id === props.streamingMessageId} + isContainingTurnActive={props.isTurnActive} onOpenFile={props.onOpenFile} onScrollToMessage={props.onScrollToMessage} /> @@ -374,7 +380,7 @@ export const TurnGroupMessage = memo(function TurnGroupMessage({ streamingMessageId, onScrollToMessage, }: TurnGroupMessageProps) { - const isGroupRunning = hasRunningTool(group.messages); + const isGroupRunning = hasRunningTool(group.messages, isTurnActive); const hasPending = hasPendingPermission(group.messages, permissionsByToolCallId); const [manualExpandState, setManualExpandState] = useState(null); diff --git a/apps/web/components/task/simple/components/agent-turn-panel.tsx b/apps/web/components/task/simple/components/agent-turn-panel.tsx index 4041f36b0c..2dc5b3c1bb 100644 --- a/apps/web/components/task/simple/components/agent-turn-panel.tsx +++ b/apps/web/components/task/simple/components/agent-turn-panel.tsx @@ -98,6 +98,7 @@ export function AgentTurnPanel({ isTaskDescription={idx === 0 && msg.author_type === "user"} taskId={taskId} sessionId={sessionId} + isContainingTurnActive={Boolean(isLive)} /> ))}
diff --git a/apps/web/e2e/tests/chat/mobile-repeated-tool-activity.spec.ts b/apps/web/e2e/tests/chat/mobile-repeated-tool-activity.spec.ts index 11013f1c17..7b7e86c16e 100644 --- a/apps/web/e2e/tests/chat/mobile-repeated-tool-activity.spec.ts +++ b/apps/web/e2e/tests/chat/mobile-repeated-tool-activity.spec.ts @@ -49,9 +49,11 @@ test.describe("mobile: repeated tool activity", () => { const chat = session.activeChat(); const groupToggle = chat.getByRole("button", { name: /6\s+tool calls/i }); await expect(groupToggle).toBeVisible({ timeout: 15_000 }); + const repeatedToolSummary = chat.getByTestId("repeated-tool-summary"); + await expect(repeatedToolSummary).toHaveCount(0); await groupToggle.click(); - await expect(chat.getByTestId("repeated-tool-summary")).toContainText( + await expect(repeatedToolSummary).toContainText( "4 repeated identical terminal commands hidden", ); }); diff --git a/apps/web/hooks/domains/session/use-session-turn.ts b/apps/web/hooks/domains/session/use-session-turn.ts index 727bff7b7e..deb525494a 100644 --- a/apps/web/hooks/domains/session/use-session-turn.ts +++ b/apps/web/hooks/domains/session/use-session-turn.ts @@ -81,6 +81,7 @@ export function useSessionTurn(sessionId: string | null) { return { lastCompletedTurn, lastTurnDuration, + activeTurnId, isActive, sessionModel, }; diff --git a/docs/decisions/0034-agentclientprotocol-codex-acp.md b/docs/decisions/0034-agentclientprotocol-codex-acp.md index ccfac62fb8..cf95045f73 100644 --- a/docs/decisions/0034-agentclientprotocol-codex-acp.md +++ b/docs/decisions/0034-agentclientprotocol-codex-acp.md @@ -10,7 +10,7 @@ Kandev's `codex-acp` agent previously launched `@zed-industries/codex-acp`. That ## Decision -Kandev's `codex-acp` agent launches `npx -y @agentclientprotocol/codex-acp` for ACP chat and one-shot inference sessions. The install script still installs `@openai/codex` for `codex login`, then installs the ACP bridge package for Kandev sessions. +Kandev's `codex-acp` agent launches `npx -y @agentclientprotocol/codex-acp@1.1.5` for ACP chat and one-shot inference sessions. The install script still installs `@openai/codex` for `codex login`, then installs the same pinned ACP bridge package for Kandev sessions. No product spec update is required: the user-facing agent remains `codex-acp`; only the package that supplies the ACP bridge changes. diff --git a/docs/specs/office/costs.md b/docs/specs/office/costs.md index d351e442f2..5f7578dc75 100644 --- a/docs/specs/office/costs.md +++ b/docs/specs/office/costs.md @@ -76,10 +76,10 @@ The `models.dev` dataset is fetched once per day to a workspace-local disk cache ### Per-CLI usage shapes (ACP wire) -- **claude-acp**: `result.usage` (camelCase, `cachedRead`/`cachedWriteTokens`), plus `usage_update.cost.amount` in USD which is preferred when present. claude-acp uses logical aliases (`sonnet`, `haiku`); provider-reported cost is the only accurate signal. +- **claude-acp**: `result.usage` (camelCase, `cachedRead`/`cachedWriteTokens`), plus cumulative `usage_update.cost.amount` in USD; the adapter emits its nonnegative turn delta when present. claude-acp uses logical aliases (`sonnet`, `haiku`); provider-reported cost is the only accurate signal. - **opencode-acp**: `result.usage` with `inputTokens`/`outputTokens`/`thoughtTokens` (no cached tokens). Optional `usage_update.cost.amount` (often `0` on BYOK). - **gemini**: `result._meta.quota.token_count.{input_tokens, output_tokens}` (snake_case, no cached, no cost). -- **codex-acp**: cumulative `usage_update.used` only; adapter infers per-turn deltas and flags rows `estimated=true`. Input vs output cannot be split on the wire. +- **codex-acp**: current context occupancy in `usage_update.used`; adapter uses nonnegative occupancy growth as a per-turn estimate and flags rows `estimated=true`. Input vs output cannot be split on the wire. Compaction or other decreases reset the estimate baseline. - **auggie**, **copilot-acp**: not tracked. `_meta.copilotUsage` is a billing multiplier; Copilot `/usage` would require scraping. ### Disk-runner provider coverage @@ -93,7 +93,7 @@ The `models.dev` dataset is fetched once per day to a workspace-local disk cache ### Cost resolution (two-layer lookup) -1. **Provider-reported cost.** If the CLI emits a USD amount on `usage_update`, store `int64(amount * 10000)` hundredths-of-a-cent and skip pricing lookup. Only accurate path for claude-acp. +1. **Provider-reported cost.** If the CLI emits cumulative USD session cost on `usage_update`, subtract the previously consumed session baseline and store the nonnegative turn delta as `int64(amount * 10000)` hundredths-of-a-cent. Ignore non-USD cost values. This is the only accurate path for claude-acp. 2. **`models.dev` lookup.** For CLIs reporting tokens but no cost (gemini, opencode BYOK, codex fallback), resolve pricing against the cached dataset. When both miss (first-boot, no network, model unknown, proprietary id), the row records `cost_subcents=0` and `estimated=true`. UI shows "pricing unavailable". Users can override pricing per model in workspace settings. @@ -187,7 +187,7 @@ No TTL or retention is applied to `office_cost_events`; rows accumulate for the ## Scenarios -- **GIVEN** an ACP agent session processing a turn, **WHEN** the `context_window` event arrives with token counts, **THEN** a cost event is created with the estimated cost looked up from models.dev pricing. The session's cumulative `cost_subcents` is updated. +- **GIVEN** an ACP agent session processing a turn, **WHEN** its `complete` event carries normalized usage, **THEN** a cost event is created from the provider-reported USD delta or estimated via models.dev pricing. The session's cumulative `cost_subcents` is updated. - **GIVEN** a model not found in the models.dev dataset and no user override configured, **WHEN** a cost event is recorded, **THEN** token counts are stored but `cost_subcents` is zero. The cost explorer shows "pricing unavailable" for that model.