diff --git a/apps/backend/cmd/agentctl/kandev_task.go b/apps/backend/cmd/agentctl/kandev_task.go index 389e52ca37..59b2c56b50 100644 --- a/apps/backend/cmd/agentctl/kandev_task.go +++ b/apps/backend/cmd/agentctl/kandev_task.go @@ -7,9 +7,14 @@ import ( "strings" ) +const ( + decisionApproved = "approved" + decisionRejected = "rejected" +) + func runTaskCmd(args []string) int { if len(args) == 0 { - cliError("usage: agentctl kandev task [flags]") + cliError("usage: agentctl kandev task [flags]") return 1 } switch args[0] { @@ -19,12 +24,51 @@ func runTaskCmd(args []string) int { return taskUpdate(args[1:]) case subcmdCreate: return taskCreate(args[1:]) + case "decision": + return taskDecision(args[1:]) default: cliError("unknown task subcommand: %s", args[0]) return 1 } } +// taskDecision records the caller's workflow-step verdict through the signed +// Office runtime API. The runtime derives every identity field from the token. +func taskDecision(args []string) int { + fs := flag.NewFlagSet("task decision", flag.ContinueOnError) + decision := fs.String("decision", "", fmt.Sprintf("Verdict: %s or %s (required)", decisionApproved, decisionRejected)) + reason := fs.String("reason", "", "Reason for the verdict (required)") + if err := fs.Parse(args); err != nil { + cliError("parse flags: %v", err) + return 1 + } + if fs.NArg() > 0 { + cliError("unexpected arguments: %s", strings.Join(fs.Args(), " ")) + return 1 + } + switch *decision { + case decisionApproved, decisionRejected: + default: + cliError("--decision must be %q or %q", decisionApproved, decisionRejected) + return 1 + } + if strings.TrimSpace(*reason) == "" { + cliError("--reason is required") + return 1 + } + + client, err := newKandevClient() + if err != nil { + cliError("%v", err) + return 1 + } + body, status, err := client.do(http.MethodPost, "/api/v1/office/runtime/task/decision", map[string]string{ + "decision": *decision, + "reason": *reason, + }) + return handleResponse(body, status, err) +} + // taskGet fetches a task by ID. Defaults to $KANDEV_TASK_ID when --id is omitted. func taskGet(args []string) int { fs := flag.NewFlagSet("task get", flag.ContinueOnError) diff --git a/apps/backend/cmd/agentctl/kandev_task_decision_test.go b/apps/backend/cmd/agentctl/kandev_task_decision_test.go new file mode 100644 index 0000000000..23bff84fb7 --- /dev/null +++ b/apps/backend/cmd/agentctl/kandev_task_decision_test.go @@ -0,0 +1,55 @@ +package main + +import ( + "encoding/json" + "net/http" + "testing" +) + +func TestTaskDecision_PostsOnlyDecisionAndReasonToRuntimeEndpoint(t *testing.T) { + captured := setupMockTransport(t, http.StatusOK, `{"decision":"approved"}`) + t.Setenv("KANDEV_API_URL", "http://kandev.test") + t.Setenv("KANDEV_API_KEY", "signed-office-run-token") + t.Setenv("KANDEV_RUN_ID", "run-456") + t.Setenv("KANDEV_TASK_ID", "task-from-environment") + t.Setenv("KANDEV_AGENT_ID", "agent-from-environment") + + code := runKandevCLI([]string{ + "task", "decision", "--decision", "approved", "--reason", "looks good", + }) + if code != 0 { + t.Fatalf("task decision exit = %d, want 0", code) + } + if captured.Method != http.MethodPost || captured.Path != "/api/v1/office/runtime/task/decision" { + t.Fatalf("request = %s %s, want POST /api/v1/office/runtime/task/decision", captured.Method, captured.Path) + } + assertRunIDHeader(t, captured, "run-456") + + var body map[string]any + if err := json.Unmarshal([]byte(captured.Body), &body); err != nil { + t.Fatalf("unmarshal request body: %v", err) + } + if len(body) != 2 || body["decision"] != "approved" || body["reason"] != "looks good" { + t.Fatalf("request body = %#v, want only decision and reason", body) + } +} + +func TestTaskDecision_RejectsInvalidInputBeforeRequest(t *testing.T) { + captured := setupMockTransport(t, http.StatusOK, `{}`) + t.Setenv("KANDEV_API_URL", "http://kandev.test") + t.Setenv("KANDEV_API_KEY", "signed-office-run-token") + + if code := runKandevCLI([]string{"task", "decision", "--decision", "approve", "--reason", "ok"}); code == 0 { + t.Fatal("expected invalid decision to fail") + } + if captured.Method != "" { + t.Fatalf("invalid decision contacted server: %s %s", captured.Method, captured.Path) + } + + if code := runKandevCLI([]string{"task", "decision", "--decision", "approved", "--reason", " "}); code == 0 { + t.Fatal("expected blank reason to fail") + } + if captured.Method != "" { + t.Fatalf("blank reason contacted server: %s %s", captured.Method, captured.Path) + } +} diff --git a/apps/backend/config/prompts/office-context.md b/apps/backend/config/prompts/office-context.md index eb50f65831..51fcad868e 100644 --- a/apps/backend/config/prompts/office-context.md +++ b/apps/backend/config/prompts/office-context.md @@ -16,7 +16,6 @@ Available tools: - list_task_documents_kandev: List documents on an accessible related task. Required: task_id. - get_task_document_kandev: Read one document on an accessible related task. Required: task_id, document_key. - write_task_document_kandev: Create or replace a document on the current task or an ancestor. Required: task_id, document_key, content. Optional: title, type. -- record_step_decision_kandev: Record an approve/reject decision for the current workflow step, as a reviewer or approver. Required: decision ("approved" or "rejected"), reason. Not idempotent: a second call supersedes the first. - step_complete_kandev: Signal that every requirement for the current workflow step is satisfied when the step requires an explicit signal. Required: summary. {step_complete_instruction} diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go b/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go index 022a5e1457..88c7664ed8 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_backend.go @@ -259,21 +259,23 @@ const ( MetadataKeyOfficeAgentProfileID = "office_agent_profile_id" // SSH runtime metadata keys (per-session, except SSHWorkdirRoot which is per-profile). - MetadataKeySSHHostAlias = "ssh_host_alias" - MetadataKeySSHHost = "ssh_host" - MetadataKeySSHPort = "ssh_port" - MetadataKeySSHUser = "ssh_user" - MetadataKeySSHHostFingerprint = "ssh_host_fingerprint" - MetadataKeySSHRemoteTaskDir = "ssh_remote_task_dir" - MetadataKeySSHRemoteSessionDir = "ssh_remote_session_dir" - MetadataKeySSHRemoteAgentctlPort = "ssh_remote_agentctl_port" - MetadataKeySSHRemoteAgentctlPID = "ssh_remote_agentctl_pid" - MetadataKeySSHLocalForwardPort = "ssh_local_forward_port" - MetadataKeySSHRemoteAgentctlURL = "ssh_remote_agentctl_url" - MetadataKeySSHWorkdirRoot = "ssh_workdir_root" - MetadataKeySSHProxyJump = "ssh_proxy_jump" - MetadataKeySSHIdentitySource = "ssh_identity_source" - MetadataKeySSHIdentityFile = "ssh_identity_file" + MetadataKeySSHHostAlias = "ssh_host_alias" + MetadataKeySSHHost = "ssh_host" + MetadataKeySSHPort = "ssh_port" + MetadataKeySSHUser = "ssh_user" + MetadataKeySSHHostFingerprint = "ssh_host_fingerprint" + MetadataKeySSHRemoteTaskDir = "ssh_remote_task_dir" + MetadataKeySSHRemoteSessionDir = "ssh_remote_session_dir" + MetadataKeySSHRemoteAgentctlPort = "ssh_remote_agentctl_port" + MetadataKeySSHRemoteAgentctlPID = "ssh_remote_agentctl_pid" + MetadataKeySSHLocalForwardPort = "ssh_local_forward_port" + MetadataKeySSHRemoteAgentctlURL = "ssh_remote_agentctl_url" + MetadataKeySSHRuntimeAPILocalURL = "ssh_runtime_api_local_url" + MetadataKeySSHRuntimeAPIRemotePort = "ssh_runtime_api_remote_port" + MetadataKeySSHWorkdirRoot = "ssh_workdir_root" + MetadataKeySSHProxyJump = "ssh_proxy_jump" + MetadataKeySSHIdentitySource = "ssh_identity_source" + MetadataKeySSHIdentityFile = "ssh_identity_file" // MetadataKeySSHShell names the login shell used when running commands // over SSH on the remote (probe, agentctl launch, install, setup // scripts). Empty / unset falls back to "bash" at runtime — see @@ -309,22 +311,24 @@ var persistentMetadataKeys = map[string]bool{ MetadataKeyLocalPort: true, // SSH runtime - MetadataKeySSHHost: true, - MetadataKeySSHPort: true, - MetadataKeySSHUser: true, - MetadataKeySSHHostFingerprint: true, - MetadataKeySSHRemoteTaskDir: true, - MetadataKeySSHRemoteSessionDir: true, - MetadataKeySSHRemoteAgentctlPort: true, - MetadataKeySSHRemoteAgentctlPID: true, - MetadataKeySSHLocalForwardPort: true, - MetadataKeySSHRemoteAgentctlURL: true, - MetadataKeySSHWorkdirRoot: true, - MetadataKeySSHProxyJump: true, - MetadataKeySSHIdentitySource: true, - MetadataKeySSHIdentityFile: true, - MetadataKeySSHShell: true, - MetadataKeySSHReclaimTaskDir: true, + MetadataKeySSHHost: true, + MetadataKeySSHPort: true, + MetadataKeySSHUser: true, + MetadataKeySSHHostFingerprint: true, + MetadataKeySSHRemoteTaskDir: true, + MetadataKeySSHRemoteSessionDir: true, + MetadataKeySSHRemoteAgentctlPort: true, + MetadataKeySSHRemoteAgentctlPID: true, + MetadataKeySSHLocalForwardPort: true, + MetadataKeySSHRemoteAgentctlURL: true, + MetadataKeySSHRuntimeAPILocalURL: true, + MetadataKeySSHRuntimeAPIRemotePort: true, + MetadataKeySSHWorkdirRoot: true, + MetadataKeySSHProxyJump: true, + MetadataKeySSHIdentitySource: true, + MetadataKeySSHIdentityFile: true, + MetadataKeySSHShell: true, + MetadataKeySSHReclaimTaskDir: true, // Kubernetes connection and exact resource inventory. MetadataKeyKubernetesAuthMode: true, @@ -390,7 +394,8 @@ var persistentMetadataPrefixes = []string{ // runtime resources (process PIDs, allocated ports, session directories on // the remote). These keys ARE persisted across a SAME-session resume — that's // how a backend restart reattaches to a still-running remote agent — but they -// MUST NOT be carried across SIBLING sessions on the same task. If they were, +// MUST NOT be carried across SIBLING sessions on the same task. This includes +// the SSH runtime API reverse-tunnel URL and port. If they were, // the second session would try to attach to the first session's agentctl // process and end up sharing its ACP session and instance port. var sessionScopedMetadataKeys = map[string]bool{ @@ -403,6 +408,8 @@ var sessionScopedMetadataKeys = map[string]bool{ MetadataKeySSHRemoteAgentctlPID: true, MetadataKeySSHLocalForwardPort: true, MetadataKeySSHRemoteAgentctlURL: true, + MetadataKeySSHRuntimeAPILocalURL: true, + MetadataKeySSHRuntimeAPIRemotePort: true, MetadataKeyKubernetesAuthMode: true, MetadataKeyKubernetesKubeconfigPath: true, MetadataKeyKubernetesKubeContext: true, diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_backend_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_backend_test.go index c528eba08e..445bd44f9f 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_backend_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_backend_test.go @@ -80,6 +80,16 @@ func TestOfficeAgentIdentityMetadataIsSessionScoped(t *testing.T) { "Office identity must not leak to a sibling session") } +func TestSSHRuntimeAPIMetadataIsPersistentAndSessionScoped(t *testing.T) { + for _, key := range []string{ + MetadataKeySSHRuntimeAPILocalURL, + MetadataKeySSHRuntimeAPIRemotePort, + } { + require.True(t, ShouldPersistMetadataKey(key), "%s must survive same-session restart", key) + require.True(t, IsSessionScopedMetadataKey(key), "%s must not leak to sibling sessions", key) + } +} + func TestFilterPersistentMetadata(t *testing.T) { t.Run("nil input returns nil", func(t *testing.T) { require.Nil(t, FilterPersistentMetadata(nil)) diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go index 7d65d35f20..5d23f5e9f0 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh.go @@ -40,18 +40,19 @@ const ( // when many short-lived channels race the long-lived stream channels), and // the local port forwarder. type sshSessionState struct { - target *SSHTarget - client *ssh.Client - forwarder *SSHPortForwarder - agentctlClient *agentctl.Client - pid int - remoteDir string - remoteTaskDir string - authToken string - reusingProcess bool - metadata map[string]interface{} - prepareEnv map[string]string - platform SSHRemotePlatform + target *SSHTarget + client *ssh.Client + forwarder *SSHPortForwarder + agentctlClient *agentctl.Client + pid int + remoteDir string + remoteTaskDir string + authToken string + reusingProcess bool + metadata map[string]interface{} + prepareEnv map[string]string + platform SSHRemotePlatform + runtimeAPITunnel *sshRuntimeAPITunnel } // SSHExecutor implements ExecutorBackend for SSH-reachable Linux and macOS hosts. @@ -117,6 +118,7 @@ func (r *SSHExecutor) Close() error { } r.mu.Unlock() for _, s := range states { + _ = s.runtimeAPITunnel.Close() if s.forwarder != nil { _ = s.forwarder.Close() } @@ -216,9 +218,11 @@ func (r *SSHExecutor) CreateInstance(ctx context.Context, req *ExecutorCreateReq if err != nil { return nil, fmt.Errorf("ssh: connect to %s@%s: %w", target.User, target.Host, err) } + var runtimeAPITunnel *sshRuntimeAPITunnel released := false defer func() { if !released { + _ = runtimeAPITunnel.Close() _ = client.Close() } }() @@ -231,6 +235,10 @@ func (r *SSHExecutor) CreateInstance(ctx context.Context, req *ExecutorCreateReq if err != nil { return nil, err } + runtimeAPITunnel, _, err = openSSHRuntimeAPITunnelForRequest(client, req) + if err != nil { + return nil, err + } workdir := r.workdirRoot(req.Metadata) taskDir := "" @@ -277,16 +285,17 @@ func (r *SSHExecutor) CreateInstance(ctx context.Context, req *ExecutorCreateReq r.mu.Lock() r.sessions[req.InstanceID] = &sshSessionState{ - target: target, - client: client, - forwarder: fwd, - pid: pid, - remoteDir: sessionDir, - remoteTaskDir: taskDir, - authToken: authToken, - metadata: cloneSSHMetadata(req.Metadata), - prepareEnv: sshRemoteContributionEnv(req, agentctlBin), - platform: platform, + target: target, + client: client, + forwarder: fwd, + pid: pid, + remoteDir: sessionDir, + remoteTaskDir: taskDir, + authToken: authToken, + metadata: cloneSSHMetadata(req.Metadata), + prepareEnv: sshRemoteContributionEnv(req, agentctlBin), + platform: platform, + runtimeAPITunnel: runtimeAPITunnel, } r.mu.Unlock() released = true // ownership transferred to session state; released on StopInstance @@ -406,6 +415,9 @@ func (r *SSHExecutor) startAndForwardAgentctl( req *ExecutorCreateRequest, platform SSHRemotePlatform, ) (int, int, *SSHPortForwarder, string, error) { + if err := uploadSSHSkillManifest(ctx, client, taskDir, req.Metadata); err != nil { + return 0, 0, nil, "", fmt.Errorf("ssh: upload skill manifest: %w", err) + } shell := sshShellForRemote(req.Metadata, platform) controlPort, pid, authToken, err := r.startAgentctlAndHandshake(ctx, client, shell, agentctlBin, taskDir, sessionDir, req) if err != nil { @@ -531,6 +543,20 @@ func (r *SSHExecutor) buildInstance( port, pid int, workdir, authToken string, ) *ExecutorInstance { + metadata := map[string]interface{}{ + MetadataKeySSHHost: target.Host, + MetadataKeySSHPort: strconv.Itoa(target.Port), + MetadataKeySSHUser: target.User, + MetadataKeySSHHostFingerprint: target.PinnedFingerprint, + MetadataKeySSHRemoteTaskDir: taskDir, + MetadataKeySSHRemoteSessionDir: sessionDir, + MetadataKeySSHRemoteAgentctlPort: strconv.Itoa(port), + MetadataKeySSHRemoteAgentctlPID: strconv.Itoa(pid), + MetadataKeySSHLocalForwardPort: strconv.Itoa(fwd.LocalPort()), + MetadataKeySSHWorkdirRoot: workdir, + MetadataKeyIsRemote: true, + } + copySSHRuntimeAPIMetadata(metadata, req.Metadata) return &ExecutorInstance{ InstanceID: req.InstanceID, TaskID: req.TaskID, @@ -541,19 +567,7 @@ func (r *SSHExecutor) buildInstance( agentctl.WithSessionID(req.SessionID), agentctl.WithAuthToken(authToken)), WorkspacePath: taskDir, AuthToken: authToken, - Metadata: map[string]interface{}{ - MetadataKeySSHHost: target.Host, - MetadataKeySSHPort: strconv.Itoa(target.Port), - MetadataKeySSHUser: target.User, - MetadataKeySSHHostFingerprint: target.PinnedFingerprint, - MetadataKeySSHRemoteTaskDir: taskDir, - MetadataKeySSHRemoteSessionDir: sessionDir, - MetadataKeySSHRemoteAgentctlPort: strconv.Itoa(port), - MetadataKeySSHRemoteAgentctlPID: strconv.Itoa(pid), - MetadataKeySSHLocalForwardPort: strconv.Itoa(fwd.LocalPort()), - MetadataKeySSHWorkdirRoot: workdir, - MetadataKeyIsRemote: true, - }, + Metadata: metadata, } } @@ -572,6 +586,21 @@ func (r *SSHExecutor) buildResumedInstance(req *ExecutorCreateRequest, state *ss agentctl.WithExecutionID(resumedSSHAgentctlInstanceID(req)), agentctl.WithSessionID(req.SessionID), agentctl.WithAuthToken(state.authToken)) } + metadata := map[string]interface{}{ + MetadataKeySSHHost: state.target.Host, + MetadataKeySSHPort: strconv.Itoa(state.target.Port), + MetadataKeySSHUser: state.target.User, + MetadataKeySSHHostFingerprint: state.target.PinnedFingerprint, + MetadataKeySSHRemoteTaskDir: taskDir, + MetadataKeySSHRemoteSessionDir: state.remoteDir, + MetadataKeySSHRemoteAgentctlPort: strconv.Itoa(port), + MetadataKeySSHRemoteAgentctlPID: strconv.Itoa(state.pid), + MetadataKeySSHLocalForwardPort: strconv.Itoa(state.forwarder.LocalPort()), + MetadataKeySSHWorkdirRoot: workdir, + MetadataKeyIsRemote: true, + "reuse_existing_process": state.reusingProcess, + } + copySSHRuntimeAPIMetadata(metadata, req.Metadata) return &ExecutorInstance{ InstanceID: req.InstanceID, TaskID: req.TaskID, @@ -580,20 +609,18 @@ func (r *SSHExecutor) buildResumedInstance(req *ExecutorCreateRequest, state *ss Client: client, WorkspacePath: taskDir, AuthToken: state.authToken, - Metadata: map[string]interface{}{ - MetadataKeySSHHost: state.target.Host, - MetadataKeySSHPort: strconv.Itoa(state.target.Port), - MetadataKeySSHUser: state.target.User, - MetadataKeySSHHostFingerprint: state.target.PinnedFingerprint, - MetadataKeySSHRemoteTaskDir: taskDir, - MetadataKeySSHRemoteSessionDir: state.remoteDir, - MetadataKeySSHRemoteAgentctlPort: strconv.Itoa(port), - MetadataKeySSHRemoteAgentctlPID: strconv.Itoa(state.pid), - MetadataKeySSHLocalForwardPort: strconv.Itoa(state.forwarder.LocalPort()), - MetadataKeySSHWorkdirRoot: workdir, - MetadataKeyIsRemote: true, - "reuse_existing_process": state.reusingProcess, - }, + Metadata: metadata, + } +} + +func copySSHRuntimeAPIMetadata(dst, src map[string]interface{}) { + for _, key := range []string{ + MetadataKeySSHRuntimeAPILocalURL, + MetadataKeySSHRuntimeAPIRemotePort, + } { + if value, ok := src[key]; ok { + dst[key] = value + } } } @@ -617,6 +644,7 @@ func (r *SSHExecutor) StopInstance(ctx context.Context, instance *ExecutorInstan zap.String("instance_id", instance.InstanceID)) return nil } + _ = state.runtimeAPITunnel.Close() if state.forwarder != nil { _ = state.forwarder.Close() } @@ -744,14 +772,21 @@ func (r *SSHExecutor) ResumeRemoteInstance(ctx context.Context, req *ExecutorCre clearSSHResumeRuntimeMetadata(req.Metadata) return nil } + runtimeAPITunnel, _, err := openSSHRuntimeAPITunnelForRequest(client, req) + if err != nil { + _ = client.Close() + return err + } remotePort, _ := strconv.Atoi(portStr) fwd, err := StartPortForward(client, remotePort, r.logger) if err != nil { + _ = runtimeAPITunnel.Close() _ = client.Close() return fmt.Errorf("ssh resume: port forward: %w", err) } if err := waitAgentctlHealthy(ctx, fwd.LocalPort(), sshAgentctlHealthTimeout); err != nil { + _ = runtimeAPITunnel.Close() _ = fwd.Close() _ = client.Close() return fmt.Errorf("ssh resume: agentctl health: %w", err) @@ -762,17 +797,18 @@ func (r *SSHExecutor) ResumeRemoteInstance(ctx context.Context, req *ExecutorCre r.mu.Lock() r.sessions[req.InstanceID] = &sshSessionState{ - target: target, - client: client, - forwarder: fwd, - agentctlClient: instanceClient, - pid: pid, - remoteDir: sessionDir, - remoteTaskDir: taskDir, - authToken: req.AuthToken, - reusingProcess: reusingProcess, - metadata: cloneSSHMetadata(req.Metadata), - prepareEnv: sshRemoteContributionEnv(req, agentctlBin), + target: target, + client: client, + forwarder: fwd, + agentctlClient: instanceClient, + pid: pid, + remoteDir: sessionDir, + remoteTaskDir: taskDir, + authToken: req.AuthToken, + reusingProcess: reusingProcess, + metadata: cloneSSHMetadata(req.Metadata), + prepareEnv: sshRemoteContributionEnv(req, agentctlBin), + runtimeAPITunnel: runtimeAPITunnel, } r.mu.Unlock() @@ -896,6 +932,7 @@ func clearSSHResumeRuntimeMetadata(metadata map[string]interface{}) { MetadataKeySSHRemoteAgentctlPID, MetadataKeySSHLocalForwardPort, MetadataKeySSHRemoteAgentctlURL, + MetadataKeySSHRuntimeAPIRemotePort, } { delete(metadata, key) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_control_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_control_test.go index e2cf3b16b8..33db6a154a 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_control_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_control_test.go @@ -249,6 +249,9 @@ func TestBuildSSHCreateInstanceRequestMapsEveryField(t *testing.T) { if got.Env[envKeyOpenAIAPIKey] != "sk-1" { t.Fatalf("Env = %+v, want the allowlisted key", got.Env) } + if got.Env[envKeyKandevCLI] != "/remote/agentctl" { + t.Fatalf("KANDEV_CLI = %q, want the remote agentctl path", got.Env[envKeyKandevCLI]) + } if _, leaked := got.Env["UNRELATED"]; leaked { t.Fatalf("Env leaked a non-allowlisted key: %+v", got.Env) } @@ -300,6 +303,23 @@ func TestSSHRemoteAgentEnvForwardsOnlyScopedCredentials(t *testing.T) { } }) + t.Run("signed runtime contract is forwarded", func(t *testing.T) { + env := sshRemoteAgentEnv(&ExecutorCreateRequest{Env: map[string]string{ + envKeyKandevAPIURL: "http://127.0.0.1:38429/api/v1", + envKeyKandevAPIKey: "signed-key", + envKeyKandevRunToken: "signed-token", + envKeyKandevAgentID: "agent-1", + envKeyKandevWorkspaceID: "workspace-1", + envKeyKandevRunID: "run-1", + envKeyKandevTaskID: "task-1", + envKeyKandevCLI: "/host/agentctl", + }}) + if env[envKeyKandevAPIURL] == "" || env[envKeyKandevAPIKey] != "signed-key" || + env[envKeyKandevRunToken] != "signed-token" || env[envKeyKandevTaskID] != "task-1" { + t.Fatalf("runtime contract = %+v", env) + } + }) + t.Run("managed broker env and indexed git config ride along", func(t *testing.T) { env := sshRemoteAgentEnv(&ExecutorCreateRequest{Env: map[string]string{ githubauth.CredentialBrokerURLEnv: "http://127.0.0.1:9/broker", diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_credentials.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_credentials.go index a07dc94019..a4c8f519e2 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_credentials.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_credentials.go @@ -8,6 +8,7 @@ import ( "io" "io/fs" "os" + "path" "regexp" "strings" @@ -386,6 +387,47 @@ func (u *sshFileUploader) WriteFile(_ context.Context, path string, data []byte, return nil } +func (u *sshFileUploader) RemoveAll(ctx context.Context, target string) error { + c, err := newSFTPClientContext(ctx, u.client) + if err != nil { + return fmt.Errorf("sftp: new client: %w", err) + } + defer func() { _ = c.Close() }() + if err := removeSFTPPath(ctx, c, target); err != nil && !isSFTPNotExist(err) && !errors.Is(err, os.ErrNotExist) { + return fmt.Errorf("sftp: remove %s: %w", target, err) + } + return nil +} + +func removeSFTPPath(ctx context.Context, client *sftp.Client, target string) error { + if err := ctx.Err(); err != nil { + return err + } + info, err := client.Lstat(target) + if err != nil { + return err + } + if info.Mode()&os.ModeSymlink != 0 { + return client.Remove(target) + } + if !info.IsDir() { + return client.Remove(target) + } + entries, err := client.ReadDirContext(ctx, target) + if err != nil { + return err + } + for _, entry := range entries { + if err := removeSFTPPath(ctx, client, path.Join(target, entry.Name())); err != nil { + if isSFTPNotExist(err) || errors.Is(err, os.ErrNotExist) { + continue + } + return err + } + } + return client.RemoveDirectory(target) +} + // sshMkdirAll mimics `mkdir -p` over SFTP. Walks every prefix of dir and // creates segments that don't exist; treats "already exists" as success. func sshMkdirAll(c *sftp.Client, dir string) error { diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_operations.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_operations.go index 29416d5c71..a3a6db4b12 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_operations.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_operations.go @@ -929,6 +929,18 @@ const ( envKeyKandevGitLabHost = "KANDEV_GITLAB_HOST" envKeyMCPTimeout = "MCP_TIMEOUT" envKeyMCPToolTimeout = "MCP_TOOL_TIMEOUT" + envKeyKandevAPIURL = "KANDEV_API_URL" + envKeyKandevAPIKey = "KANDEV_API_KEY" + envKeyKandevRunToken = "KANDEV_RUN_TOKEN" + envKeyKandevCLI = "KANDEV_CLI" + envKeyKandevAgentID = "KANDEV_AGENT_ID" + envKeyKandevAgentName = "KANDEV_AGENT_NAME" + envKeyKandevWorkspaceID = "KANDEV_WORKSPACE_ID" + envKeyKandevRunID = "KANDEV_RUN_ID" + envKeyKandevTaskID = "KANDEV_TASK_ID" + envKeyKandevWakeReason = "KANDEV_WAKE_REASON" + envKeyKandevWakeCommentID = "KANDEV_WAKE_COMMENT_ID" + envKeyKandevWakePayload = "KANDEV_WAKE_PAYLOAD_JSON" ) var sshRemoteAgentCredentialEnvKeys = []string{ @@ -944,11 +956,23 @@ var sshRemoteAgentCredentialEnvKeys = []string{ envKeyKandevGitLabHost, } -// sshRemoteAgentRuntimeEnvKeys are non-secret runtime controls that must reach -// the remote agent process after profile and agent precedence has been resolved. +// sshRemoteAgentRuntimeEnvKeys are resolved runtime contract values that must +// reach the remote agent process after profile and agent precedence is applied. var sshRemoteAgentRuntimeEnvKeys = []string{ envKeyMCPTimeout, envKeyMCPToolTimeout, + envKeyKandevAPIURL, + envKeyKandevAPIKey, + envKeyKandevRunToken, + envKeyKandevCLI, + envKeyKandevAgentID, + envKeyKandevAgentName, + envKeyKandevWorkspaceID, + envKeyKandevRunID, + envKeyKandevTaskID, + envKeyKandevWakeReason, + envKeyKandevWakeCommentID, + envKeyKandevWakePayload, } // sshRemoteAgentEnv builds the env map sent to the remote agent instance. Each diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_remote_contribution.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_remote_contribution.go index 64050bfc8f..65033ef8e0 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_remote_contribution.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_remote_contribution.go @@ -66,6 +66,12 @@ func sshRemoteContributionEnv(req *ExecutorCreateRequest, agentctlBin string) ma if req == nil { return env } + if agentctlBin != "" { + env[envKeyKandevCLI] = agentctlBin + } + if agentctlBin != "" { + env["KANDEV_CLI"] = agentctlBin + } count := sshRemoteGitConfigCount(env) managedBroker := hasManagedGitHubBrokerEnv(req.Env) foundManagedHelper := rewriteSSHManagedGitCredentialHelpers(env, count, managedBroker, agentctlBin) diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_resume_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_resume_test.go index 8bb8f1837b..4951970700 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_resume_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_resume_test.go @@ -32,8 +32,10 @@ func TestSSHBuildResumedInstanceRecordsProcessReuse(t *testing.T) { TaskID: "task-1", SessionID: "session-1", Metadata: map[string]interface{}{ - MetadataKeySSHRemoteAgentctlPort: "43123", - MetadataKeySSHRemoteTaskDir: "/remote/task", + MetadataKeySSHRemoteAgentctlPort: "43123", + MetadataKeySSHRemoteTaskDir: "/remote/task", + MetadataKeySSHRuntimeAPILocalURL: "http://127.0.0.1:38429/api/v1", + MetadataKeySSHRuntimeAPIRemotePort: "41234", }, }, &sshSessionState{ target: &SSHTarget{Host: "ssh.example", Port: 22, User: "agent"}, @@ -48,10 +50,36 @@ func TestSSHBuildResumedInstanceRecordsProcessReuse(t *testing.T) { if !ok || reusingProcess != tc.reusingProcess { t.Fatalf("reuse_existing_process = (%v, %v), want (%v, true)", reusingProcess, ok, tc.reusingProcess) } + if got := instance.Metadata[MetadataKeySSHRuntimeAPILocalURL]; got != "http://127.0.0.1:38429/api/v1" { + t.Fatalf("local API URL metadata = %v", got) + } + if got := instance.Metadata[MetadataKeySSHRuntimeAPIRemotePort]; got != "41234" { + t.Fatalf("remote API port metadata = %v", got) + } }) } } +func TestSSHBuildInstanceCarriesRuntimeAPITunnelMetadata(t *testing.T) { + executor := &SSHExecutor{logger: newNopLogger(t)} + instance := executor.buildInstance( + &ExecutorCreateRequest{InstanceID: "instance-1", Metadata: map[string]interface{}{ + MetadataKeySSHRuntimeAPILocalURL: "http://127.0.0.1:38429/api/v1", + MetadataKeySSHRuntimeAPIRemotePort: "41234", + }}, + &SSHTarget{Host: "ssh.example", Port: 22, User: "agent"}, + &SSHPortForwarder{localPort: 43124}, + "/remote/task", "/remote/session", 43123, 99, "/home/agent/.kandev", "token", + ) + + if got := instance.Metadata[MetadataKeySSHRuntimeAPILocalURL]; got != "http://127.0.0.1:38429/api/v1" { + t.Fatalf("local API URL metadata = %v", got) + } + if got := instance.Metadata[MetadataKeySSHRuntimeAPIRemotePort]; got != "41234" { + t.Fatalf("remote API port metadata = %v", got) + } +} + func TestSSHResumedAgentctlClientChecksExistingProcess(t *testing.T) { for _, tc := range []struct { name string diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_runtime_api.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_runtime_api.go new file mode 100644 index 0000000000..7e3ac3fbdb --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_runtime_api.go @@ -0,0 +1,195 @@ +package lifecycle + +import ( + "fmt" + "io" + "net" + "net/url" + "strconv" + "strings" + "sync" + "time" + + "golang.org/x/crypto/ssh" +) + +const sshRuntimeAPIDialTimeout = 10 * time.Second + +const urlSchemeHTTP = "http" + +// sshRuntimeAPITunnel exposes a control-plane loopback API through the SSH +// connection. The remote agent can then use the same API URL shape as a local +// launch without assuming that the SSH host can route to the developer's +// machine. +type sshRuntimeAPITunnel struct { + listener net.Listener + localTarget string + remotePort string + once sync.Once +} + +func openSSHRuntimeAPITunnelForRequest( + client *ssh.Client, + req *ExecutorCreateRequest, +) (*sshRuntimeAPITunnel, string, error) { + if req == nil { + return nil, "", fmt.Errorf("ssh runtime API tunnel: request is required") + } + rawURL := getMetadataString(req.Metadata, MetadataKeySSHRuntimeAPILocalURL) + if rawURL == "" && req.Env != nil { + rawURL = req.Env[envKeyKandevAPIURL] + } + requestedPort := getMetadataString(req.Metadata, MetadataKeySSHRuntimeAPIRemotePort) + tunnel, runtimeURL, err := openSSHRuntimeAPITunnel(client, rawURL, requestedPort) + if err != nil || tunnel == nil { + return tunnel, runtimeURL, err + } + if req.Env == nil { + req.Env = make(map[string]string) + } + req.Env[envKeyKandevAPIURL] = runtimeURL + if req.Metadata == nil { + req.Metadata = make(map[string]interface{}) + } + req.Metadata[MetadataKeySSHRuntimeAPILocalURL] = rawURL + req.Metadata[MetadataKeySSHRuntimeAPIRemotePort] = tunnel.RemotePort() + return tunnel, runtimeURL, nil +} + +func openSSHRuntimeAPITunnel(client *ssh.Client, rawURL string, requestedRemotePort ...string) (*sshRuntimeAPITunnel, string, error) { + parsed, err := url.Parse(strings.TrimSpace(rawURL)) + if err != nil { + return nil, rawURL, fmt.Errorf("ssh runtime API URL: %w", err) + } + if !isLoopbackURL(parsed) { + return nil, rawURL, nil + } + if client == nil { + return nil, rawURL, fmt.Errorf("ssh runtime API tunnel: SSH client is required") + } + localPort := parsed.Port() + if localPort == "" { + localPort = defaultURLPort(parsed.Scheme) + } + if localPort == "" { + return nil, rawURL, fmt.Errorf("ssh runtime API tunnel: URL must include a port") + } + remotePort := "0" + if len(requestedRemotePort) > 0 && strings.TrimSpace(requestedRemotePort[0]) != "" { + remotePort = strings.TrimSpace(requestedRemotePort[0]) + if _, err := parseSSHRuntimeAPIPort(remotePort, true); err != nil { + return nil, rawURL, err + } + } + remoteAddress := net.JoinHostPort("127.0.0.1", remotePort) + listener, err := client.Listen("tcp", remoteAddress) + if err != nil { + return nil, rawURL, fmt.Errorf("ssh runtime API tunnel: listen on remote %s: %w", remoteAddress, err) + } + actualRemotePort, err := sshRuntimeAPIPortFromAddr(listener.Addr()) + if err != nil { + _ = listener.Close() + return nil, rawURL, err + } + tunnel := &sshRuntimeAPITunnel{ + listener: listener, + localTarget: net.JoinHostPort(parsed.Hostname(), localPort), + remotePort: actualRemotePort, + } + go tunnel.acceptLoop() + + parsed.Host = net.JoinHostPort("127.0.0.1", actualRemotePort) + return tunnel, parsed.String(), nil +} + +func parseSSHRuntimeAPIPort(raw string, allowZero bool) (int, error) { + port, err := strconv.Atoi(raw) + if err != nil || port < 0 || port > 65535 || (!allowZero && port == 0) { + return 0, fmt.Errorf("ssh runtime API tunnel: invalid remote port %q", raw) + } + return port, nil +} + +func sshRuntimeAPIPortFromAddr(addr net.Addr) (string, error) { + if addr == nil { + return "", fmt.Errorf("ssh runtime API tunnel: listener has no address") + } + _, port, err := net.SplitHostPort(addr.String()) + if err != nil { + return "", fmt.Errorf("ssh runtime API tunnel: parse listener address %q: %w", addr.String(), err) + } + if _, err := parseSSHRuntimeAPIPort(port, false); err != nil { + return "", err + } + return port, nil +} + +func isLoopbackURL(parsed *url.URL) bool { + if parsed == nil || parsed.Hostname() == "" { + return false + } + host := strings.ToLower(parsed.Hostname()) + if host == "localhost" || host == "::1" { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + +func defaultURLPort(scheme string) string { + switch strings.ToLower(scheme) { + case urlSchemeHTTP: + return "80" + case "https": + return "443" + default: + return "" + } +} + +func (t *sshRuntimeAPITunnel) acceptLoop() { + for { + remote, err := t.listener.Accept() + if err != nil { + return + } + go t.proxy(remote) + } +} + +func (t *sshRuntimeAPITunnel) proxy(remote net.Conn) { + local, err := net.DialTimeout("tcp", t.localTarget, sshRuntimeAPIDialTimeout) + if err != nil { + _ = remote.Close() + return + } + done := make(chan struct{}, 2) + go func() { + _, _ = io.Copy(local, remote) + done <- struct{}{} + }() + go func() { + _, _ = io.Copy(remote, local) + done <- struct{}{} + }() + <-done + _ = local.Close() + _ = remote.Close() + <-done +} + +func (t *sshRuntimeAPITunnel) RemotePort() string { + if t == nil { + return "" + } + return t.remotePort +} + +func (t *sshRuntimeAPITunnel) Close() error { + if t == nil { + return nil + } + var err error + t.once.Do(func() { err = t.listener.Close() }) + return err +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_runtime_api_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_runtime_api_test.go new file mode 100644 index 0000000000..136c5746d8 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_runtime_api_test.go @@ -0,0 +1,160 @@ +package lifecycle + +import ( + "io" + "net" + "net/http" + "net/http/httptest" + "net/url" + "strconv" + "sync" + "testing" +) + +func TestOpenSSHRuntimeAPITunnel_LeavesReachableURLsUnchanged(t *testing.T) { + const rawURL = "https://kandev.example.test/api/v1" + + tunnel, gotURL, err := openSSHRuntimeAPITunnel(nil, rawURL) + if err != nil { + t.Fatalf("openSSHRuntimeAPITunnel: %v", err) + } + if tunnel != nil { + t.Fatal("reachable URL unexpectedly created a tunnel") + } + if gotURL != rawURL { + t.Fatalf("URL = %q, want %q", gotURL, rawURL) + } +} + +func TestOpenSSHRuntimeAPITunnel_RequiresSSHClientForLoopbackURL(t *testing.T) { + _, _, err := openSSHRuntimeAPITunnel(nil, "http://127.0.0.1:38429/api/v1") + if err == nil { + t.Fatal("loopback URL without SSH client unexpectedly succeeded") + } +} + +func TestOpenSSHRuntimeAPITunnel_ConcurrentSameHostSessionsGetUniqueRemotePorts(t *testing.T) { + backendListener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("reserve backend port: %v", err) + } + backendPort := backendListener.Addr().(*net.TCPAddr).Port + _ = backendListener.Close() + rawURL := "http://127.0.0.1:" + strconv.Itoa(backendPort) + "/api/v1" + + server := newFakeSSHServer(t, nil) + client := server.dial(t) + start := make(chan struct{}) + type result struct { + tunnel *sshRuntimeAPITunnel + url string + err error + } + results := make(chan result, 2) + var wg sync.WaitGroup + for range 2 { + wg.Add(1) + go func() { + defer wg.Done() + <-start + tunnel, rewritten, openErr := openSSHRuntimeAPITunnel(client, rawURL) + results <- result{tunnel: tunnel, url: rewritten, err: openErr} + }() + } + close(start) + wg.Wait() + close(results) + + seenPorts := make(map[string]struct{}, 2) + for outcome := range results { + if outcome.err != nil { + t.Fatalf("concurrent tunnel open failed: %v", outcome.err) + } + if outcome.tunnel == nil { + t.Fatal("loopback URL did not create a tunnel") + } + parsed, parseErr := url.Parse(outcome.url) + if parseErr != nil { + t.Fatalf("parse rewritten URL %q: %v", outcome.url, parseErr) + } + seenPorts[parsed.Port()] = struct{}{} + if parsed.Port() == strconv.Itoa(backendPort) { + t.Fatalf("rewritten URL still uses backend port %d: %q", backendPort, outcome.url) + } + _ = outcome.tunnel.Close() + } + if len(seenPorts) != 2 { + t.Fatalf("concurrent tunnels used %d remote ports, want 2", len(seenPorts)) + } +} + +func TestOpenSSHRuntimeAPITunnel_ResumeRebindsPersistedRemotePort(t *testing.T) { + server := newFakeSSHServer(t, nil) + client := server.dial(t) + const rawURL = "http://127.0.0.1:38429/api/v1" + firstReq := &ExecutorCreateRequest{Env: map[string]string{envKeyKandevAPIURL: rawURL}} + + firstTunnel, firstURL, err := openSSHRuntimeAPITunnelForRequest(client, firstReq) + if err != nil { + t.Fatalf("first tunnel: %v", err) + } + firstPort := firstTunnel.RemotePort() + if firstReq.Metadata[MetadataKeySSHRuntimeAPILocalURL] != rawURL { + t.Fatalf("local URL metadata = %v, want %q", firstReq.Metadata[MetadataKeySSHRuntimeAPILocalURL], rawURL) + } + if firstReq.Metadata[MetadataKeySSHRuntimeAPIRemotePort] != firstPort { + t.Fatalf("remote port metadata = %v, want %q", firstReq.Metadata[MetadataKeySSHRuntimeAPIRemotePort], firstPort) + } + if err := firstTunnel.Close(); err != nil { + t.Fatalf("close first tunnel: %v", err) + } + + resumedReq := &ExecutorCreateRequest{ + Env: map[string]string{envKeyKandevAPIURL: firstURL}, + Metadata: cloneSSHMetadata(firstReq.Metadata), + } + secondTunnel, secondURL, err := openSSHRuntimeAPITunnelForRequest(client, resumedReq) + if err != nil { + t.Fatalf("resume tunnel: %v", err) + } + defer func() { _ = secondTunnel.Close() }() + if secondTunnel.RemotePort() != firstPort { + t.Fatalf("resume remote port = %q, want persisted %q", secondTunnel.RemotePort(), firstPort) + } + if secondURL != firstURL { + t.Fatalf("resume URL = %q, want the original rewritten URL %q", secondURL, firstURL) + } + if resumedReq.Env[envKeyKandevAPIURL] != secondURL { + t.Fatalf("resume API URL = %q, want %q", resumedReq.Env[envKeyKandevAPIURL], secondURL) + } +} + +func TestOpenSSHRuntimeAPITunnel_ProxiesRoundTripAndTearsDown(t *testing.T) { + localServer := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Connection", "close") + _, _ = io.WriteString(w, "runtime-api-round-trip") + })) + t.Cleanup(localServer.Close) + + server := newFakeSSHServer(t, nil) + client := server.dial(t) + tunnel, rewrittenURL, err := openSSHRuntimeAPITunnel(client, localServer.URL) + if err != nil { + t.Fatalf("open tunnel: %v", err) + } + t.Cleanup(func() { _ = tunnel.Close() }) + + requestClient := &http.Client{Transport: &http.Transport{DisableKeepAlives: true}} + response, err := requestClient.Get(rewrittenURL) + if err != nil { + t.Fatalf("GET through SSH runtime API tunnel: %v", err) + } + t.Cleanup(func() { _ = response.Body.Close() }) + body, err := io.ReadAll(response.Body) + if err != nil { + t.Fatalf("read tunneled response: %v", err) + } + if string(body) != "runtime-api-round-trip" { + t.Fatalf("tunneled response = %q", body) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_skills.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_skills.go new file mode 100644 index 0000000000..bbc2460058 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_skills.go @@ -0,0 +1,124 @@ +package lifecycle + +import ( + "context" + "encoding/json" + "fmt" + "os" + "path" + "strings" + + "golang.org/x/crypto/ssh" + + "github.com/kandev/kandev/internal/agent/runtime/lifecycle/skill" +) + +type sshSkillWorkspace interface { + WriteFile(context.Context, string, []byte, os.FileMode) error + RemoveAll(context.Context, string) error +} + +// uploadSSHSkillManifest materializes launch-scoped skills after the remote +// task workspace exists and before the remote agent starts. The lifecycle +// deployer cannot write the host worktree because SSH agents run elsewhere. +func uploadSSHSkillManifest( + ctx context.Context, + client *ssh.Client, + taskDir string, + metadata map[string]interface{}, +) error { + raw := getMetadataString(metadata, MetadataKeySkillManifestJSON) + if raw == "" { + return nil + } + return materializeSSHSkillManifest(ctx, &sshFileUploader{client: client}, taskDir, raw) +} + +func materializeSSHSkillManifest( + ctx context.Context, + workspace sshSkillWorkspace, + taskDir string, + raw string, +) error { + if workspace == nil { + return fmt.Errorf("SSH skill workspace is required") + } + taskRoot := path.Clean(strings.TrimSpace(taskDir)) + if taskRoot == "." || !path.IsAbs(taskRoot) { + return fmt.Errorf("SSH task directory must be absolute") + } + var manifest skill.Manifest + if err := json.Unmarshal([]byte(raw), &manifest); err != nil { + return fmt.Errorf("unmarshal SSH skill manifest: %w", err) + } + projectDir := manifest.ProjectSkillDir + if projectDir == "" { + projectDir = skill.DefaultProjectSkillDir + } + projectDir, ok := sshSafeRelativePath(projectDir) + if !ok { + return fmt.Errorf("SSH skill manifest project directory is unsafe") + } + projectRoot := path.Join(taskRoot, projectDir) + decisionRoot := path.Join(projectRoot, skill.DirName(skill.ReservedDecisionSkillSlug)) + if err := workspace.RemoveAll(ctx, decisionRoot); err != nil { + return fmt.Errorf("remove stale SSH decision skill: %w", err) + } + return materializeSSHManifestSkills(ctx, workspace, projectRoot, manifest.Skills) +} + +func materializeSSHManifestSkills( + ctx context.Context, + workspace sshSkillWorkspace, + projectRoot string, + skills []skill.Skill, +) error { + claimed := make(map[string]string, len(skills)) + for _, item := range skills { + if !validSlugRe.MatchString(item.Slug) { + continue + } + dirName := skill.DirName(item.Slug) + if _, exists := claimed[dirName]; exists { + continue + } + claimed[dirName] = item.Slug + if err := writeSSHManifestSkill(ctx, workspace, projectRoot, item); err != nil { + return err + } + } + return nil +} + +func writeSSHManifestSkill( + ctx context.Context, + workspace sshSkillWorkspace, + projectRoot string, + item skill.Skill, +) error { + root := path.Join(projectRoot, skill.DirName(item.Slug)) + if err := workspace.WriteFile(ctx, path.Join(root, "SKILL.md"), []byte(item.Content), 0o644); err != nil { + return fmt.Errorf("write SSH skill %q: %w", item.Slug, err) + } + for _, support := range item.Files { + relative, safe := sshSafeRelativePath(support.Path) + if !safe || relative == "SKILL.md" { + continue + } + if err := workspace.WriteFile(ctx, path.Join(root, relative), []byte(support.Content), 0o644); err != nil { + return fmt.Errorf("write SSH skill support file %q: %w", support.Path, err) + } + } + return nil +} + +func sshSafeRelativePath(value string) (string, bool) { + if value == "" || strings.Contains(value, "\\") || strings.ContainsRune(value, '\x00') || path.IsAbs(value) { + return "", false + } + cleaned := path.Clean(value) + if cleaned == "." || cleaned == ".." || strings.HasPrefix(cleaned, "../") { + return "", false + } + return cleaned, true +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_skills_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_skills_test.go new file mode 100644 index 0000000000..a787f433e8 --- /dev/null +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_skills_test.go @@ -0,0 +1,109 @@ +package lifecycle + +import ( + "context" + "encoding/json" + "os" + "path/filepath" + "testing" + + "github.com/kandev/kandev/internal/agent/runtime/lifecycle/skill" +) + +func TestUploadSSHSkillManifest_WritesDecisionSkillToRemoteWorkspace(t *testing.T) { + remoteWorkspace := t.TempDir() + server := newFakeSSHServer(t, nil) + server.enableSFTP() + manifest, err := json.Marshal(skill.Manifest{ + ProjectSkillDir: ".claude/skills", + Skills: []skill.Skill{{ + Slug: skill.ReservedDecisionSkillSlug, + Content: "---\nname: kandev-step-decision\ndescription: decision\n---\n# decision", + }}, + }) + if err != nil { + t.Fatalf("marshal manifest: %v", err) + } + + if err := uploadSSHSkillManifest(context.Background(), server.dial(t), remoteWorkspace, + map[string]interface{}{MetadataKeySkillManifestJSON: string(manifest)}); err != nil { + t.Fatalf("uploadSSHSkillManifest: %v", err) + } + + path := filepath.Join(remoteWorkspace, ".claude", "skills", "kandev-step-decision", "SKILL.md") + data, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read remote decision skill: %v", err) + } + if string(data) != "---\nname: kandev-step-decision\ndescription: decision\n---\n# decision" { + t.Fatalf("remote decision skill = %q", string(data)) + } +} + +func TestUploadSSHSkillManifest_NoSeatRemovesStaleDecisionSkill(t *testing.T) { + remoteWorkspace := t.TempDir() + server := newFakeSSHServer(t, nil) + server.enableSFTP() + client := server.dial(t) + + seatManifest, err := json.Marshal(skill.Manifest{ + ProjectSkillDir: ".claude/skills", + Skills: []skill.Skill{{Slug: skill.ReservedDecisionSkillSlug, Content: "# decision"}}, + }) + if err != nil { + t.Fatalf("marshal seat manifest: %v", err) + } + metadata := map[string]interface{}{MetadataKeySkillManifestJSON: string(seatManifest)} + if err := uploadSSHSkillManifest(context.Background(), client, remoteWorkspace, metadata); err != nil { + t.Fatalf("seat uploadSSHSkillManifest: %v", err) + } + decisionPath := filepath.Join(remoteWorkspace, ".claude", "skills", "kandev-step-decision", "SKILL.md") + if _, err := os.Stat(decisionPath); err != nil { + t.Fatalf("seat decision skill missing: %v", err) + } + + noSeatManifest, err := json.Marshal(skill.Manifest{ProjectSkillDir: ".claude/skills"}) + if err != nil { + t.Fatalf("marshal no-seat manifest: %v", err) + } + metadata[MetadataKeySkillManifestJSON] = string(noSeatManifest) + if err := uploadSSHSkillManifest(context.Background(), client, remoteWorkspace, metadata); err != nil { + t.Fatalf("no-seat uploadSSHSkillManifest: %v", err) + } + if _, err := os.Stat(decisionPath); !os.IsNotExist(err) { + t.Fatalf("no-seat upload left stale decision skill at %s", decisionPath) + } +} + +func TestUploadSSHSkillManifest_NoSeatDoesNotFollowDecisionSkillSymlink(t *testing.T) { + remoteWorkspace := t.TempDir() + outsideWorkspace := t.TempDir() + marker := filepath.Join(outsideWorkspace, "keep.txt") + if err := os.WriteFile(marker, []byte("must survive"), 0o600); err != nil { + t.Fatalf("seed outside marker: %v", err) + } + decisionRoot := filepath.Join(remoteWorkspace, ".claude", "skills", skill.ReservedDecisionSkillSlug) + if err := os.MkdirAll(filepath.Dir(decisionRoot), 0o755); err != nil { + t.Fatalf("create decision skill parent: %v", err) + } + if err := os.Symlink(outsideWorkspace, decisionRoot); err != nil { + t.Skipf("symlinks are unavailable: %v", err) + } + + server := newFakeSSHServer(t, nil) + server.enableSFTP() + manifest, err := json.Marshal(skill.Manifest{ProjectSkillDir: ".claude/skills"}) + if err != nil { + t.Fatalf("marshal no-seat manifest: %v", err) + } + if err := uploadSSHSkillManifest(context.Background(), server.dial(t), remoteWorkspace, + map[string]interface{}{MetadataKeySkillManifestJSON: string(manifest)}); err != nil { + t.Fatalf("uploadSSHSkillManifest: %v", err) + } + if _, err := os.Stat(marker); err != nil { + t.Fatalf("outside marker was removed: %v", err) + } + if _, err := os.Lstat(decisionRoot); !os.IsNotExist(err) { + t.Fatalf("decision symlink still exists or returned unexpected error: %v", err) + } +} diff --git a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_stop_resume_test.go b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_stop_resume_test.go index c39f380334..f91e0fa34e 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_stop_resume_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/executor_ssh_stop_resume_test.go @@ -522,21 +522,26 @@ func TestSSHTaskDirName(t *testing.T) { func TestClearSSHResumeRuntimeMetadata(t *testing.T) { metadata := map[string]interface{}{ - MetadataKeySSHRemoteSessionDir: "/remote/session", - MetadataKeySSHRemoteAgentctlPort: "41234", - MetadataKeySSHRemoteAgentctlPID: "4242", - MetadataKeySSHLocalForwardPort: "5000", - MetadataKeySSHRemoteAgentctlURL: "http://127.0.0.1:5000", - MetadataKeySSHRemoteTaskDir: "/remote/task", - MetadataKeySSHHost: "build.example", + MetadataKeySSHRemoteSessionDir: "/remote/session", + MetadataKeySSHRemoteAgentctlPort: "41234", + MetadataKeySSHRemoteAgentctlPID: "4242", + MetadataKeySSHLocalForwardPort: "5000", + MetadataKeySSHRemoteAgentctlURL: "http://127.0.0.1:5000", + MetadataKeySSHRuntimeAPILocalURL: "http://127.0.0.1:3456/api/v1", + MetadataKeySSHRuntimeAPIRemotePort: "45678", + MetadataKeySSHRemoteTaskDir: "/remote/task", + MetadataKeySSHHost: "build.example", } clearSSHResumeRuntimeMetadata(metadata) - if len(metadata) != 2 { - t.Fatalf("remaining metadata = %+v, want only the durable keys", metadata) + if len(metadata) != 3 { + t.Fatalf("remaining metadata = %+v, want durable keys plus the local API URL", metadata) } if metadata[MetadataKeySSHRemoteTaskDir] != "/remote/task" || metadata[MetadataKeySSHHost] != "build.example" { t.Fatalf("durable metadata was cleared: %+v", metadata) } + if metadata[MetadataKeySSHRuntimeAPILocalURL] != "http://127.0.0.1:3456/api/v1" { + t.Fatalf("local API URL was cleared: %+v", metadata) + } clearSSHResumeRuntimeMetadata(nil) // must not panic } diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill/delivery.go b/apps/backend/internal/agent/runtime/lifecycle/skill/delivery.go index 4c10335cb5..157c241072 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill/delivery.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill/delivery.go @@ -23,7 +23,9 @@ func (d *Deployer) deliver(_ context.Context, manifest *Manifest, executorType, return d.deliverSprites(manifest) case "k8s": return d.deliverKubernetes(manifest) - case "", "local", "local_pc", "worktree", "local_docker", "remote_docker", "ssh", "mock_remote": + case "ssh": + return d.deliverSSH(manifest) + case "", "local", "local_pc", "worktree", "local_docker", "remote_docker", "mock_remote": // local_pc and local_docker share the same delivery: the // worktree IS the agent's CWD inside the executor (Docker // bind-mounts it; local_pc runs the agent in it directly), so @@ -68,6 +70,16 @@ func (d *Deployer) deliverKubernetes(manifest *Manifest) DeployResult { return d.deliverRemote(manifest, dir, "k8s") } +// deliverSSH keeps the manifest in launch metadata. The SSH executor uploads +// its files after the remote task workspace and agentctl instance exist; the +// host-side worktree is not the SSH agent's workspace. +func (d *Deployer) deliverSSH(manifest *Manifest) DeployResult { + result := d.deliverRemote(manifest, "", "ssh") + result.InstructionsDir = instructionsDirHost(d.basePath, manifest.WorkspaceSlug, manifest.AgentID) + d.writeInstructionFiles(manifest, result.InstructionsDir) + return result +} + func (d *Deployer) deliverRemote(manifest *Manifest, dir, executorType string) DeployResult { rewriteManifestRefs(manifest, dir) normalizeManifestSkills(manifest) diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill/deployer.go b/apps/backend/internal/agent/runtime/lifecycle/skill/deployer.go index 6607f11317..61e192efbe 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill/deployer.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill/deployer.go @@ -80,11 +80,12 @@ func New(cfg Config) (*Deployer, error) { // lifecycle.SkillDeployRequest but is decoupled from the lifecycle // package to keep the import boundary one-way. type Request struct { - Profile *settingsmodels.AgentProfile - WorkspacePath string - ExecutorType string - WorkspaceID string - SessionID string + Profile *settingsmodels.AgentProfile + WorkspacePath string + ExecutorType string + WorkspaceID string + SessionID string + AdditionalSkillSlugs []string // OfficeRuntime reports whether backend selected Office mode and the // finalized launch env contains a non-empty KANDEV_CLI. Office launch // validation checks the remaining runtime variables before this hook runs. @@ -100,7 +101,7 @@ func (d *Deployer) Deploy(ctx context.Context, req Request) (DeployResult, error if req.Profile == nil { return DeployResult{}, errors.New("skill deploy: profile is required") } - manifest := d.buildManifest(ctx, req.Profile, d.workspaceSlugFn(req.WorkspaceID), req.OfficeRuntime) + manifest := d.buildManifest(ctx, req.Profile, d.workspaceSlugFn(req.WorkspaceID), req.AdditionalSkillSlugs, req.OfficeRuntime) result := d.deliver(ctx, manifest, req.ExecutorType, req.WorkspacePath) return result, nil } diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill/deployer_officeruntime_test.go b/apps/backend/internal/agent/runtime/lifecycle/skill/deployer_officeruntime_test.go index 980b85ecda..161ee4a619 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill/deployer_officeruntime_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill/deployer_officeruntime_test.go @@ -55,9 +55,10 @@ func TestDeploy_OfficeRuntimeGatesSystemSkills(t *testing.T) { ID: "routine-agent", SkillIDs: string(skillIDsJSON), }, - ExecutorType: "worktree", - WorkspacePath: worktree, - OfficeRuntime: officeRuntime, + ExecutorType: "worktree", + WorkspacePath: worktree, + AdditionalSkillSlugs: []string{skill.ReservedDecisionSkillSlug}, + OfficeRuntime: officeRuntime, }); err != nil { t.Fatalf("Deploy: %v", err) } diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill/deployer_test.go b/apps/backend/internal/agent/runtime/lifecycle/skill/deployer_test.go index a9320ef999..ba70655c48 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill/deployer_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill/deployer_test.go @@ -344,6 +344,122 @@ func TestDeploy_KanbanProfile_NoSkillsNoOp(t *testing.T) { } } +func TestDeploy_AdditionalSkillSlugMaterializesForEmptyProfile(t *testing.T) { + base := t.TempDir() + worktree := t.TempDir() + reader := &fakeSkillReader{skills: map[string]*skill.Skill{ + "kandev-step-decision": {Slug: "kandev-step-decision", Content: "# decision"}, + }} + d := newDeployer(t, base, reader, &fakeInstructionLister{}) + + _, err := d.Deploy(context.Background(), skill.Request{ + Profile: &settingsmodels.AgentProfile{ + ID: "office-p1", + AgentID: "claude-acp", + }, + AdditionalSkillSlugs: []string{"kandev-step-decision"}, + ExecutorType: "local_pc", + WorkspacePath: worktree, + }) + if err != nil { + t.Fatalf("Deploy: %v", err) + } + + skillPath := filepath.Join(worktree, ".claude", "skills", "kandev-step-decision", "SKILL.md") + if _, err := os.Stat(skillPath); err != nil { + t.Fatalf("additional decision skill was not materialized: %v", err) + } +} + +func TestDeploy_SSHReturnsManifestWithoutWritingControlPlaneWorktree(t *testing.T) { + base := t.TempDir() + worktree := t.TempDir() + reader := &fakeSkillReader{skills: map[string]*skill.Skill{ + "kandev-step-decision": {Slug: "kandev-step-decision", Content: "# decision"}, + }} + d := newDeployer(t, base, reader, &fakeInstructionLister{}) + + res, err := d.Deploy(context.Background(), skill.Request{ + Profile: &settingsmodels.AgentProfile{ID: "ssh-p1", AgentID: "claude-acp"}, + ExecutorType: "ssh", WorkspacePath: worktree, + AdditionalSkillSlugs: []string{"kandev-step-decision"}, + }) + if err != nil { + t.Fatalf("Deploy: %v", err) + } + raw, ok := res.Metadata[skill.MetadataKeySkillManifestJSON].(string) + if !ok || raw == "" { + t.Fatalf("SSH delivery missing skill manifest: %#v", res.Metadata) + } + var manifest skill.Manifest + if err := json.Unmarshal([]byte(raw), &manifest); err != nil { + t.Fatalf("unmarshal SSH manifest: %v", err) + } + if len(manifest.Skills) != 1 || manifest.Skills[0].Slug != "kandev-step-decision" { + t.Fatalf("SSH manifest skills = %+v", manifest.Skills) + } + path := filepath.Join(worktree, ".claude", "skills", "kandev-step-decision", "SKILL.md") + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("SSH delivery wrote control-plane worktree file %s", path) + } +} + +func TestDeploy_DurableDecisionSkillIsNotMaterialized(t *testing.T) { + base := t.TempDir() + worktree := t.TempDir() + decision := &skill.Skill{Slug: "kandev-step-decision", Content: "# decision"} + reader := &fakeSkillReader{skills: map[string]*skill.Skill{ + "kandev-step-decision": decision, + "decision-id": decision, + }} + d := newDeployer(t, base, reader, &fakeInstructionLister{}) + + _, err := d.Deploy(context.Background(), skill.Request{ + Profile: &settingsmodels.AgentProfile{ + ID: "kanban-p1", AgentID: "claude-acp", + SkillIDs: `["decision-id"]`, + DesiredSkills: `["kandev-step-decision"]`, + }, + ExecutorType: "local_pc", WorkspacePath: worktree, + }) + if err != nil { + t.Fatalf("Deploy: %v", err) + } + path := filepath.Join(worktree, ".claude", "skills", "kandev-step-decision", "SKILL.md") + if _, err := os.Stat(path); !os.IsNotExist(err) { + t.Fatalf("durably selected decision skill was materialized at %s", path) + } +} + +func TestDeploy_SeatThenNonSeatRemovesDecisionSkill(t *testing.T) { + base := t.TempDir() + worktree := t.TempDir() + reader := &fakeSkillReader{skills: map[string]*skill.Skill{ + "kandev-step-decision": {Slug: "kandev-step-decision", Content: "# decision"}, + "sk-review": {Slug: "sk-review", Content: "# review"}, + }} + d := newDeployer(t, base, reader, &fakeInstructionLister{}) + profile := &settingsmodels.AgentProfile{ID: "p1", AgentID: "claude-acp"} + if _, err := d.Deploy(context.Background(), skill.Request{ + Profile: profile, ExecutorType: "local_pc", WorkspacePath: worktree, + AdditionalSkillSlugs: []string{"kandev-step-decision"}, + }); err != nil { + t.Fatalf("seat Deploy: %v", err) + } + decisionPath := filepath.Join(worktree, ".claude", "skills", "kandev-step-decision", "SKILL.md") + if _, err := os.Stat(decisionPath); err != nil { + t.Fatalf("seat decision skill missing: %v", err) + } + if _, err := d.Deploy(context.Background(), skill.Request{ + Profile: profile, ExecutorType: "local_pc", WorkspacePath: worktree, + }); err != nil { + t.Fatalf("non-seat Deploy: %v", err) + } + if _, err := os.Stat(decisionPath); !os.IsNotExist(err) { + t.Fatalf("non-seat deployment left decision skill at %s", decisionPath) + } +} + // TestDeploy_KanbanProfileWithSkill_DeploysFiles verifies that a // kanban-flavoured profile (no DesiredSkills, no Role) that the user // later enriched with SkillIDs gets the same delivery treatment as diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill/manifest.go b/apps/backend/internal/agent/runtime/lifecycle/skill/manifest.go index 3051b45501..f39700c81f 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill/manifest.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill/manifest.go @@ -10,6 +10,11 @@ import ( settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" ) +// ReservedDecisionSkillSlug is launch-scoped. It is admitted only through +// Request.AdditionalSkillSlugs so a seat can receive decision instructions +// without making them part of a durable agent profile. +const ReservedDecisionSkillSlug = "kandev-step-decision" + // buildManifest resolves a profile's desired skills + instruction files // into a Manifest the delivery strategies can consume. It is pure data // — no filesystem side effects. @@ -17,8 +22,10 @@ import ( // The agent profile is the unified ADR 0005 row. We read both // SkillIDs (the new merged column) and DesiredSkills (legacy office // column) and union the two into a single list of slugs / IDs. Empty -// slots are dropped before lookup. -func (d *Deployer) buildManifest(ctx context.Context, profile *settingsmodels.AgentProfile, workspaceSlug string, officeRuntime bool) *Manifest { +// slots and launch-reserved skill slugs are dropped before lookup. +// System skills are included only for Office launches, whose runtime env +// provides the protocol they depend on. +func (d *Deployer) buildManifest(ctx context.Context, profile *settingsmodels.AgentProfile, workspaceSlug string, additionalSkillSlugs []string, officeRuntime bool) *Manifest { // Profile.AgentID IS the agent type ID after ADR 0005 — the // agent_profiles row's agent_id column points at the agents // table (claude-acp, codex-acp, ...). No extra resolver needed. @@ -29,28 +36,53 @@ func (d *Deployer) buildManifest(ctx context.Context, profile *settingsmodels.Ag AgentID: profile.ID, ProjectSkillDir: d.resolveProjectSkillDir(agentTypeID), } - d.appendSkills(ctx, manifest, profile, officeRuntime) + d.appendSkills(ctx, manifest, profile, additionalSkillSlugs, officeRuntime) d.appendInstructions(ctx, manifest, profile.ID) return manifest } // appendSkills resolves every desired slug / id on the profile to a // runtime Skill record. Lookups that fail are logged at debug level -// and dropped — a missing skill must never abort a launch. System -// skills (bundled Office protocol/task-ops skills) are skipped unless -// officeRuntime is true: their instructions depend on Office runtime -// env that only the office scheduler launch path provides. -func (d *Deployer) appendSkills(ctx context.Context, manifest *Manifest, profile *settingsmodels.AgentProfile, officeRuntime bool) { +// and dropped — a missing skill must never abort a launch. +// System skills (bundled Office protocol/task-ops skills) are skipped unless +// officeRuntime is true: their instructions depend on Office runtime env +// that only the Office scheduler launch path provides. +func (d *Deployer) appendSkills(ctx context.Context, manifest *Manifest, profile *settingsmodels.AgentProfile, additionalSkillSlugs []string, officeRuntime bool) { if d.skillReader == nil { return } - for _, key := range mergedSkillKeys(profile) { + keys := mergedSkillKeys(profile) + additionalKeys := make(map[string]struct{}, len(additionalSkillSlugs)) + seen := make(map[string]struct{}, len(keys)+len(additionalSkillSlugs)) + for _, key := range keys { + seen[key] = struct{}{} + } + for _, key := range additionalSkillSlugs { + key = strings.TrimSpace(key) + if key == "" { + continue + } + additionalKeys[key] = struct{}{} + if _, exists := seen[key]; exists { + continue + } + seen[key] = struct{}{} + keys = append(keys, key) + } + for _, key := range keys { skill, err := d.skillReader.GetSkillFromConfig(ctx, key) if err != nil || skill == nil { d.logger.Debug("skip skill in manifest", zap.String("key", key), zap.Error(err)) continue } + if isReservedSkillSlug(skill.Slug) { + if _, launchScoped := additionalKeys[key]; !launchScoped { + d.logger.Debug("skip reserved durable skill", + zap.String("key", key), zap.String("slug", skill.Slug)) + continue + } + } if skill.IsSystem && !officeRuntime { d.logger.Debug("skip system skill: no office runtime env", zap.String("key", key)) @@ -96,9 +128,11 @@ func (d *Deployer) resolveProjectSkillDir(agentTypeID string) string { } // mergedSkillKeys returns the union of profile.SkillIDs and -// profile.DesiredSkills, with empties trimmed. Duplicates collapse. +// profile.DesiredSkills, with empties and launch-reserved skill slugs +// removed. Duplicates collapse. // The order is SkillIDs first (the canonical post-ADR field), then // DesiredSkills (legacy office input that callers still populate). +// Launch-reserved skills are admitted only through AdditionalSkillSlugs. // // Both fields are JSON-array TEXT columns; non-empty values are unmarshalled // into a list of slugs and concatenated. Malformed JSON is treated as an @@ -114,7 +148,7 @@ func mergedSkillKeys(profile *settingsmodels.AgentProfile) []string { out := make([]string, 0, len(skillIDs)+len(desired)) for _, k := range skillIDs { k = strings.TrimSpace(k) - if k == "" || seen[k] { + if k == "" || isReservedSkillSlug(k) || seen[k] { continue } seen[k] = true @@ -122,7 +156,7 @@ func mergedSkillKeys(profile *settingsmodels.AgentProfile) []string { } for _, k := range desired { k = strings.TrimSpace(k) - if k == "" || seen[k] { + if k == "" || isReservedSkillSlug(k) || seen[k] { continue } seen[k] = true @@ -131,6 +165,10 @@ func mergedSkillKeys(profile *settingsmodels.AgentProfile) []string { return out } +func isReservedSkillSlug(slug string) bool { + return strings.TrimSpace(slug) == ReservedDecisionSkillSlug +} + // decodeSkillSlugs parses a JSON-array string into a []string. Empty input // or non-array input collapses to nil so callers can treat "no entries" // uniformly without checking for malformed JSON. diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill/types.go b/apps/backend/internal/agent/runtime/lifecycle/skill/types.go index 699c9fe1b0..e603969db1 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill/types.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill/types.go @@ -1,8 +1,8 @@ // Package skill is the runtime-tier implementation of per-profile skill // and instruction-file deployment introduced in ADR 0005 Wave A. The // package owns the manifest builder and the per-executor delivery -// strategies (local filesystem symlinks, Docker bind-mount, Sprites -// upload). Wave E moved this code out of internal/office into the +// strategies (local filesystem, Docker bind-mount, Sprites upload, +// SSH/SFTP upload). Wave E moved this code out of internal/office into the // runtime so every launch (kanban or office) goes through the same // path. // diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill_deploy.go b/apps/backend/internal/agent/runtime/lifecycle/skill_deploy.go index 4b6e4b1bce..8994325a78 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill_deploy.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill_deploy.go @@ -15,9 +15,10 @@ import ( const MetadataKeyInstructionsDir = "kandev_instructions_dir" // runSkillDeploy resolves the agent profile (now including the office -// enrichment fields from ADR 0005 Wave A), short-circuits when there's -// nothing to deploy, and otherwise hands off to the configured -// SkillDeployer. Any metadata patches the deployer returns are merged +// enrichment fields from ADR 0005 Wave A) and hands it off to the configured +// SkillDeployer. Empty profiles still reach the deployer so it can reconcile +// launch-scoped files left by a previous seat run. Any metadata patches the +// deployer returns are merged // onto the prepared LaunchRequest so executor backends see them // (e.g. Docker bind-mount, Sprites manifest upload). Errors are logged // and swallowed: an in-flight launch must not be aborted because skill @@ -36,18 +37,14 @@ func (m *Manager) runSkillDeploy(ctx context.Context, original, prepared *Launch zap.Error(err)) return } - if len(profile.SkillIDs) == 0 && len(profile.DesiredSkills) == 0 { - // Fast path: shallow / kanban-flavour profiles with no enrichment to - // deploy. Most launches today land here. - return - } req := SkillDeployRequest{ - Profile: profile, - WorkspacePath: prepared.WorkspacePath, - ExecutorType: prepared.ExecutorType, - WorkspaceID: profile.WorkspaceID, - SessionID: original.SessionID, - OfficeRuntime: prepared.McpMode == mcpmode.Office && strings.TrimSpace(prepared.Env["KANDEV_CLI"]) != "", + Profile: profile, + WorkspacePath: prepared.WorkspacePath, + ExecutorType: prepared.ExecutorType, + WorkspaceID: profile.WorkspaceID, + SessionID: original.SessionID, + AdditionalSkillSlugs: append([]string(nil), original.AdditionalSkillSlugs...), + OfficeRuntime: prepared.McpMode == mcpmode.Office && strings.TrimSpace(prepared.Env["KANDEV_CLI"]) != "", } result, err := m.skillDeployer.DeploySkills(ctx, req) if err != nil { diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill_deploy_test.go b/apps/backend/internal/agent/runtime/lifecycle/skill_deploy_test.go index 64fd01e402..ffe93930de 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill_deploy_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill_deploy_test.go @@ -3,9 +3,12 @@ package lifecycle import ( "context" "errors" + "os" + "path/filepath" "sync/atomic" "testing" + runtimeskill "github.com/kandev/kandev/internal/agent/runtime/lifecycle/skill" settingsmodels "github.com/kandev/kandev/internal/agent/settings/models" "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/common/mcpmode" @@ -31,6 +34,17 @@ type recordingDeployer struct { err error } +type lifecycleSkillReader struct { + skill *runtimeskill.Skill +} + +func (r *lifecycleSkillReader) GetSkillFromConfig(_ context.Context, key string) (*runtimeskill.Skill, error) { + if key == r.skill.Slug { + return r.skill, nil + } + return nil, nil +} + func (r *recordingDeployer) DeploySkills(_ context.Context, req SkillDeployRequest) (SkillDeployResult, error) { r.called.Add(1) r.last = req @@ -63,10 +77,10 @@ func TestRunSkillDeploy_NoOpWithoutReader(t *testing.T) { } } -// TestRunSkillDeploy_FastPathEmptyProfile verifies that a shallow / kanban -// profile (empty skill_ids, empty desired_skills) short-circuits before -// invoking the deployer. -func TestRunSkillDeploy_FastPathEmptyProfile(t *testing.T) { +// TestRunSkillDeploy_EmptyProfileInvokesDeployerForCleanup verifies that a +// shallow / kanban profile still reaches the deployer so it can remove a +// launch-scoped skill left by an earlier seat run. +func TestRunSkillDeploy_EmptyProfileInvokesDeployerForCleanup(t *testing.T) { mgr := newSkillDeployTestManager(t) rec := &recordingDeployer{} mgr.skillDeployer = rec @@ -78,8 +92,70 @@ func TestRunSkillDeploy_FastPathEmptyProfile(t *testing.T) { &LaunchRequest{AgentProfileID: "p1"}, &LaunchRequest{WorkspacePath: "/tmp/ws", ExecutorType: "local_pc"}) - if rec.called.Load() != 0 { - t.Errorf("shallow profile should fast-path, deployer was called %d times", rec.called.Load()) + if rec.called.Load() != 1 { + t.Errorf("empty profile should invoke cleanup, deployer was called %d times", rec.called.Load()) + } +} + +func TestRunSkillDeploy_AdditionalSkillSlugInvokesDeployerForEmptyProfile(t *testing.T) { + mgr := newSkillDeployTestManager(t) + rec := &recordingDeployer{} + mgr.skillDeployer = rec + mgr.agentProfileReader = &fakeProfileReader{ + profile: &settingsmodels.AgentProfile{ID: "p1", AgentID: "a1"}, + } + + mgr.runSkillDeploy(context.Background(), + &LaunchRequest{ + AgentProfileID: "p1", + AdditionalSkillSlugs: []string{"kandev-step-decision"}, + }, + &LaunchRequest{WorkspacePath: "/tmp/ws", ExecutorType: "local_pc"}) + + if rec.called.Load() != 1 { + t.Fatalf("expected deployer once, got %d", rec.called.Load()) + } + if len(rec.last.AdditionalSkillSlugs) != 1 || rec.last.AdditionalSkillSlugs[0] != "kandev-step-decision" { + t.Fatalf("additional skill slugs = %v", rec.last.AdditionalSkillSlugs) + } +} + +func TestRunSkillDeploy_SeatThenNonSeatCleansMaterializedDecisionSkill(t *testing.T) { + base := t.TempDir() + worktree := t.TempDir() + log, _ := logger.NewLogger(logger.LoggingConfig{Level: "error", Format: "console"}) + deployer, err := runtimeskill.New(runtimeskill.Config{ + Logger: log, + BasePath: base, + SkillReader: &lifecycleSkillReader{skill: &runtimeskill.Skill{ + Slug: runtimeskill.ReservedDecisionSkillSlug, Content: "# decision", + }}, + ProjectSkillDirResolver: func(string) string { return ".claude/skills" }, + }) + if err != nil { + t.Fatalf("skill.New: %v", err) + } + mgr := newSkillDeployTestManager(t) + mgr.skillDeployer = NewSkillDeployerAdapter(deployer) + mgr.agentProfileReader = &fakeProfileReader{ + profile: &settingsmodels.AgentProfile{ID: "seat-profile", AgentID: "claude-acp"}, + } + prepared := &LaunchRequest{WorkspacePath: worktree, ExecutorType: "local_pc"} + + mgr.runSkillDeploy(context.Background(), &LaunchRequest{ + AgentProfileID: "seat-profile", + AdditionalSkillSlugs: []string{ + runtimeskill.ReservedDecisionSkillSlug, + }, + }, prepared) + decisionPath := filepath.Join(worktree, ".claude", "skills", "kandev-step-decision", "SKILL.md") + if _, err := os.Stat(decisionPath); err != nil { + t.Fatalf("seat launch did not materialize decision skill: %v", err) + } + + mgr.runSkillDeploy(context.Background(), &LaunchRequest{AgentProfileID: "seat-profile"}, prepared) + if _, err := os.Stat(decisionPath); !os.IsNotExist(err) { + t.Fatalf("non-seat launch left decision skill at %s", decisionPath) } } diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill_deployer.go b/apps/backend/internal/agent/runtime/lifecycle/skill_deployer.go index 1972427f8b..30d0cda137 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill_deployer.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill_deployer.go @@ -27,11 +27,12 @@ type SkillDeployer interface { // the agent will run inside; ExecutorType is the executor backend // (local_pc / local_docker / sprites) so the deployer can pick a strategy. type SkillDeployRequest struct { - Profile *settingsmodels.AgentProfile - WorkspacePath string - ExecutorType string - WorkspaceID string - SessionID string + Profile *settingsmodels.AgentProfile + WorkspacePath string + ExecutorType string + WorkspaceID string + SessionID string + AdditionalSkillSlugs []string // OfficeRuntime reports whether backend selected Office mode and the // finalized launch env contains a non-empty KANDEV_CLI. Office launch // validation checks the remaining runtime variables before this hook runs. diff --git a/apps/backend/internal/agent/runtime/lifecycle/skill_deployer_adapter.go b/apps/backend/internal/agent/runtime/lifecycle/skill_deployer_adapter.go index b043173034..edfaf4fcae 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/skill_deployer_adapter.go +++ b/apps/backend/internal/agent/runtime/lifecycle/skill_deployer_adapter.go @@ -35,12 +35,13 @@ func (a *skillDeployerAdapter) DeploySkills(ctx context.Context, req SkillDeploy return SkillDeployResult{}, nil } res, err := a.inner.Deploy(ctx, skill.Request{ - Profile: req.Profile, - WorkspacePath: req.WorkspacePath, - ExecutorType: req.ExecutorType, - WorkspaceID: req.WorkspaceID, - SessionID: req.SessionID, - OfficeRuntime: req.OfficeRuntime, + Profile: req.Profile, + WorkspacePath: req.WorkspacePath, + ExecutorType: req.ExecutorType, + WorkspaceID: req.WorkspaceID, + SessionID: req.SessionID, + AdditionalSkillSlugs: append([]string(nil), req.AdditionalSkillSlugs...), + OfficeRuntime: req.OfficeRuntime, }) if err != nil { return SkillDeployResult{}, err diff --git a/apps/backend/internal/agent/runtime/lifecycle/ssh_fake_server_test.go b/apps/backend/internal/agent/runtime/lifecycle/ssh_fake_server_test.go index 5b9321cfa2..ab3ac67c08 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/ssh_fake_server_test.go +++ b/apps/backend/internal/agent/runtime/lifecycle/ssh_fake_server_test.go @@ -6,6 +6,7 @@ import ( "errors" "io" "net" + "strconv" "strings" "sync" "testing" @@ -55,6 +56,9 @@ type fakeSSHServer struct { calls []sshExecCall handler sshExecHandler + reverseMu sync.Mutex + reverseForwards map[string]net.Listener + // sftpEnabled serves the "sftp" subsystem against the real filesystem, so // tests point remote paths at a t.TempDir() and assert on real bytes. sftpEnabled bool @@ -81,7 +85,10 @@ func newFakeSSHServer(t *testing.T, handler sshExecHandler) *fakeSSHServer { if err != nil { t.Fatalf("listen: %v", err) } - s := &fakeSSHServer{t: t, listener: listener, signer: signer, handler: handler} + s := &fakeSSHServer{ + t: t, listener: listener, signer: signer, handler: handler, + reverseForwards: make(map[string]net.Listener), + } t.Cleanup(s.Close) s.wg.Add(1) go s.acceptLoop() @@ -140,6 +147,12 @@ func (s *fakeSSHServer) lastCommandContaining(substr string) (sshExecCall, bool) func (s *fakeSSHServer) Close() { s.closeOnce.Do(func() { _ = s.listener.Close() + s.reverseMu.Lock() + for key, listener := range s.reverseForwards { + _ = listener.Close() + delete(s.reverseForwards, key) + } + s.reverseMu.Unlock() }) s.wg.Wait() } @@ -184,10 +197,7 @@ func (s *fakeSSHServer) serveConn(conn net.Conn) { defer func() { _ = serverConn.Close() }() s.wg.Add(1) - go func() { - defer s.wg.Done() - ssh.DiscardRequests(reqs) - }() + go s.serveGlobalRequests(serverConn, reqs) for newChan := range chans { switch newChan.ChannelType() { @@ -203,6 +213,134 @@ func (s *fakeSSHServer) serveConn(conn net.Conn) { } } +func (s *fakeSSHServer) serveGlobalRequests(conn *ssh.ServerConn, requests <-chan *ssh.Request) { + defer s.wg.Done() + for req := range requests { + switch req.Type { + case "tcpip-forward": + s.handleTCPIPForward(conn, req) + case "cancel-tcpip-forward": + s.handleCancelTCPIPForward(req) + default: + if req.WantReply { + _ = req.Reply(false, nil) + } + } + } +} + +func (s *fakeSSHServer) handleTCPIPForward(conn *ssh.ServerConn, req *ssh.Request) { + var payload struct { + Addr string + Port uint32 + } + if err := ssh.Unmarshal(req.Payload, &payload); err != nil { + _ = req.Reply(false, nil) + return + } + address := net.JoinHostPort(payload.Addr, strconv.FormatUint(uint64(payload.Port), 10)) + listener, err := net.Listen("tcp", address) + if err != nil { + _ = req.Reply(false, nil) + return + } + actualPort := uint32(listener.Addr().(*net.TCPAddr).Port) + key := net.JoinHostPort(payload.Addr, strconv.FormatUint(uint64(actualPort), 10)) + s.reverseMu.Lock() + s.reverseForwards[key] = listener + s.reverseMu.Unlock() + if payload.Port == 0 { + _ = req.Reply(true, ssh.Marshal(struct{ Port uint32 }{Port: actualPort})) + } else { + _ = req.Reply(true, nil) + } + s.wg.Add(1) + go s.serveReverseForward(conn, key, listener) +} + +func (s *fakeSSHServer) handleCancelTCPIPForward(req *ssh.Request) { + var payload struct { + Addr string + Port uint32 + } + if err := ssh.Unmarshal(req.Payload, &payload); err != nil { + _ = req.Reply(false, nil) + return + } + key := net.JoinHostPort(payload.Addr, strconv.FormatUint(uint64(payload.Port), 10)) + s.reverseMu.Lock() + listener, ok := s.reverseForwards[key] + if ok { + delete(s.reverseForwards, key) + } + s.reverseMu.Unlock() + if ok { + _ = listener.Close() + } + _ = req.Reply(true, nil) +} + +func (s *fakeSSHServer) serveReverseForward(conn *ssh.ServerConn, key string, listener net.Listener) { + defer s.wg.Done() + defer func() { + s.reverseMu.Lock() + delete(s.reverseForwards, key) + s.reverseMu.Unlock() + }() + for { + incoming, err := listener.Accept() + if err != nil { + return + } + s.serveReverseForwardConnection(conn, incoming) + } +} + +func (s *fakeSSHServer) serveReverseForwardConnection(conn *ssh.ServerConn, incoming net.Conn) { + defer func() { _ = incoming.Close() }() + originAddr, originPort := tcpOrigin(incoming.RemoteAddr()) + channel, requests, err := conn.OpenChannel("forwarded-tcpip", ssh.Marshal(struct { + Addr string + Port uint32 + OriginAddr string + OriginPort uint32 + }{ + Addr: "127.0.0.1", + Port: uint32(incoming.LocalAddr().(*net.TCPAddr).Port), + OriginAddr: originAddr, + OriginPort: originPort, + })) + if err != nil { + return + } + s.wg.Add(1) + go func() { + defer s.wg.Done() + ssh.DiscardRequests(requests) + }() + + var copies sync.WaitGroup + copies.Add(2) + go func() { + defer copies.Done() + _, _ = io.Copy(channel, incoming) + }() + go func() { + defer copies.Done() + _, _ = io.Copy(incoming, channel) + }() + copies.Wait() + _ = channel.Close() +} + +func tcpOrigin(addr net.Addr) (string, uint32) { + tcpAddr, ok := addr.(*net.TCPAddr) + if !ok { + return "", 0 + } + return tcpAddr.IP.String(), uint32(tcpAddr.Port) +} + func (s *fakeSSHServer) serveSession(newChan ssh.NewChannel) { defer s.wg.Done() channel, requests, err := newChan.Accept() diff --git a/apps/backend/internal/agent/runtime/lifecycle/types.go b/apps/backend/internal/agent/runtime/lifecycle/types.go index 7ecc19b05d..7c86458041 100644 --- a/apps/backend/internal/agent/runtime/lifecycle/types.go +++ b/apps/backend/internal/agent/runtime/lifecycle/types.go @@ -923,6 +923,9 @@ type LaunchRequest struct { TaskDescription string // Task description to send via ACP prompt Attachments []MessageAttachment // Attachments (images/files) for the initial prompt Env map[string]string // Additional env vars + // AdditionalSkillSlugs are materialized for this launch in addition to the + // durable profile selection. + AdditionalSkillSlugs []string // ApprovedSecretEnvKeys contains repository binding keys that SSH may // forward in addition to its managed credential allowlist. ApprovedSecretEnvKeys []string diff --git a/apps/backend/internal/backendapp/adapters.go b/apps/backend/internal/backendapp/adapters.go index 87bfb6bda1..b11974c82b 100644 --- a/apps/backend/internal/backendapp/adapters.go +++ b/apps/backend/internal/backendapp/adapters.go @@ -424,6 +424,7 @@ func buildLifecycleLaunchRequest( TaskDescription: req.TaskDescription, Attachments: convertToLifecycleAttachments(req.Attachments), Env: req.Env, + AdditionalSkillSlugs: append([]string(nil), req.AdditionalSkillSlugs...), ApprovedSecretEnvKeys: append([]string(nil), req.ApprovedSecretEnvKeys...), EnvironmentDefinitions: append([]runtimeenv.Definition(nil), req.EnvironmentDefinitions...), EnvironmentResolutionRequired: req.EnvironmentResolutionRequired, diff --git a/apps/backend/internal/backendapp/canvas_edit_test.go b/apps/backend/internal/backendapp/canvas_edit_test.go index 8a6ebb7c0a..8fd9604371 100644 --- a/apps/backend/internal/backendapp/canvas_edit_test.go +++ b/apps/backend/internal/backendapp/canvas_edit_test.go @@ -220,32 +220,32 @@ func stringContains(value, want string) bool { func newCanvasEditTestDependencies() (*fakeCanvasEditTaskStore, *fakeCanvasEditCanvasStore, *fakeCanvasEditReleaseStore, *fakeCanvasEditArtifactStore) { return &fakeCanvasEditTaskStore{ - workspace: &models.Workspace{ - ID: "workspace-a", - DefaultAgentProfileID: stringPointer("profile-default"), - DefaultExecutorID: stringPointer("executor-default"), - }, - }, &fakeCanvasEditCanvasStore{canvas: &canvas.Canvas{ - ID: "canvas-a", - PluginInstanceID: "instance-a", - WorkspaceID: "workspace-a", - ScopeKind: canvas.ScopeWorkspace, - Status: instances.StatusActive, - ActiveReleaseID: "release-a", - ActiveReleaseStatus: instances.ValidationValid, - EffectiveGrants: []canvas.GrantProjection{{ - PermissionKind: "api_read", Resource: "tasks", ScopeCeiling: instances.ScopeWorkspace, - }}, - }}, &fakeCanvasEditReleaseStore{release: instances.Release{ - ID: "release-a", - InstanceID: "instance-a", - PackageDigest: "digest-a", - ArtifactPath: "releases/digest-a", - ValidationStatus: instances.ValidationValid, - }}, &fakeCanvasEditArtifactStore{files: map[string][]byte{ - "manifest.yaml": []byte("manifest"), - "index.html": []byte("index"), - }} + workspace: &models.Workspace{ + ID: "workspace-a", + DefaultAgentProfileID: stringPointer("profile-default"), + DefaultExecutorID: stringPointer("executor-default"), + }, + }, &fakeCanvasEditCanvasStore{canvas: &canvas.Canvas{ + ID: "canvas-a", + PluginInstanceID: "instance-a", + WorkspaceID: "workspace-a", + ScopeKind: canvas.ScopeWorkspace, + Status: instances.StatusActive, + ActiveReleaseID: "release-a", + ActiveReleaseStatus: instances.ValidationValid, + EffectiveGrants: []canvas.GrantProjection{{ + PermissionKind: "api_read", Resource: "tasks", ScopeCeiling: instances.ScopeWorkspace, + }}, + }}, &fakeCanvasEditReleaseStore{release: instances.Release{ + ID: "release-a", + InstanceID: "instance-a", + PackageDigest: "digest-a", + ArtifactPath: "releases/digest-a", + ValidationStatus: instances.ValidationValid, + }}, &fakeCanvasEditArtifactStore{files: map[string][]byte{ + "manifest.yaml": []byte("manifest"), + "index.html": []byte("index"), + }} } func stringPointer(value string) *string { return &value } diff --git a/apps/backend/internal/backendapp/helpers.go b/apps/backend/internal/backendapp/helpers.go index 60eee1a3ed..8f553ee6a4 100644 --- a/apps/backend/internal/backendapp/helpers.go +++ b/apps/backend/internal/backendapp/helpers.go @@ -1878,10 +1878,6 @@ func registerMCPAndDebugRoutes( if p.services.GitLab != nil { mcpHandlers.SetTaskMRAutomationService(p.services.GitLab) } - if p.services.OfficeSvcs != nil && p.services.OfficeSvcs.Dashboard != nil { - mcpHandlers.SetDashboardService(p.services.OfficeSvcs.Dashboard) - } - // Reuse the cross-task handoff service constructed in registerRoutes — // the same instance backs the MCP path and the HTTP Kanban path so // workspace-group state stays consistent across both surfaces. diff --git a/apps/backend/internal/backendapp/main.go b/apps/backend/internal/backendapp/main.go index f636356358..197d4428ed 100644 --- a/apps/backend/internal/backendapp/main.go +++ b/apps/backend/internal/backendapp/main.go @@ -81,7 +81,6 @@ import ( "github.com/kandev/kandev/internal/office/configloader" officeservice "github.com/kandev/kandev/internal/office/service" "github.com/kandev/kandev/internal/orchestrator" - v1 "github.com/kandev/kandev/pkg/api/v1" // Office feature packages office "github.com/kandev/kandev/internal/office" @@ -2056,17 +2055,16 @@ func backfillAgentDefaultSkills( } } -// newOfficeTaskStarter wraps orchestratorSvc.StartTaskWithEnv in the -// officeservice.TaskStarterWithEnvFunc adapter. Extracted from +// newOfficeTaskStarter wraps orchestratorSvc.StartTaskWithEnvAndSkills in the +// officeservice.TaskStarterWithLaunchContextFunc adapter. Extracted from // initOfficeServices to keep that function under the funlen cap. func newOfficeTaskStarter(orchestratorSvc *orchestrator.Service) officeservice.TaskStarter { - return officeservice.TaskStarterWithEnvFunc( - func(ctx context.Context, taskID, agentProfileID, executorID, - executorProfileID string, priority string, prompt, workflowStepID string, - planMode bool, attachments []v1.MessageAttachment, env map[string]string) error { - _, err := orchestratorSvc.StartTaskWithEnv(ctx, taskID, agentProfileID, - executorID, executorProfileID, priority, prompt, - workflowStepID, planMode, false, attachments, env) + return officeservice.TaskStarterWithLaunchContextFunc( + func(ctx context.Context, taskID, agentProfileID string, launch officeservice.LaunchContext) error { + _, err := orchestratorSvc.StartTaskWithEnvAndSkills(ctx, taskID, agentProfileID, + launch.ExecutorID, launch.ExecutorProfileID, launch.Priority, launch.Prompt, + launch.WorkflowStepID, launch.PlanMode, false, launch.Attachments, launch.Env, + launch.AdditionalSkillSlugs) return err }, ) diff --git a/apps/backend/internal/mcp/handlers/agent_decision_handlers.go b/apps/backend/internal/mcp/handlers/agent_decision_handlers.go deleted file mode 100644 index 68fb5e66b9..0000000000 --- a/apps/backend/internal/mcp/handlers/agent_decision_handlers.go +++ /dev/null @@ -1,97 +0,0 @@ -package handlers - -import ( - "context" - "encoding/json" - "errors" - - "go.uber.org/zap" - - "github.com/kandev/kandev/internal/office/dashboard" - "github.com/kandev/kandev/internal/office/shared" - "github.com/kandev/kandev/internal/task/models" - ws "github.com/kandev/kandev/pkg/websocket" -) - -// SetDashboardService wires the office dashboard service used to back the -// record_step_decision_kandev MCP tool. Optional — when nil, the handler -// reports a configuration error instead of registering and returning a -// broken endpoint. -func (h *Handlers) SetDashboardService(svc *dashboard.DashboardService) { - h.dashboardSvc = svc -} - -// handleRecordStepDecision dispatches mcp.record_step_decision -// (record_step_decision_kandev). Per the tool contract, task/step/role are -// never caller-supplied: the task comes from the calling session (msg -// carries session_id/task_id set by the MCP server from its own bound -// s.sessionID/s.taskID, matching every other Office tool), and role/seat -// are resolved server-side by DashboardService.RecordAgentDecision via the -// engine's AC-4a slate. -func (h *Handlers) handleRecordStepDecision(ctx context.Context, msg *ws.Message) (*ws.Message, error) { - if h.dashboardSvc == nil { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "dashboard service not configured", nil) - } - var req struct { - TaskID string `json:"task_id"` - SessionID string `json:"session_id"` - Decision string `json:"decision"` - Reason string `json:"reason"` - } - if err := json.Unmarshal(msg.Payload, &req); err != nil { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeBadRequest, "Invalid payload: "+err.Error(), nil) - } - if req.TaskID == "" || req.SessionID == "" { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "task_id and session_id are required", nil) - } - - session, err := h.sessionRepo.GetTaskSession(ctx, req.SessionID) - if err != nil { - if errors.Is(err, models.ErrTaskSessionNotFound) { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeNotFound, "session not found", nil) - } - h.logger.Error("record_step_decision: failed to load session", - zap.String("session_id", req.SessionID), zap.Error(err)) - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "failed to load session", nil) - } - if session.TaskID != req.TaskID { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, "session does not belong to task", nil) - } - - result, err := h.dashboardSvc.RecordAgentDecision(ctx, dashboard.RecordAgentDecisionInput{ - TaskID: req.TaskID, - AgentProfileID: session.AgentProfileID, - Decision: req.Decision, - Reason: req.Reason, - SessionID: req.SessionID, - }) - if err != nil { - if !errors.Is(err, shared.ErrForbidden) && !dashboard.IsAgentDecisionValidationError(err) { - h.logger.Error("record_step_decision: failed to record decision", zap.String("task_id", req.TaskID), zap.Error(err)) - } - return mapRecordStepDecisionError(msg, err) - } - - return ws.NewResponse(msg.ID, msg.Action, map[string]interface{}{ - "decision": result.Decision, - "role": result.Role, - "step_id": result.StepID, - "decision_id": result.DecisionID, - "decided_at": result.DecidedAt, - "transition_applied": result.TransitionApplied, - "guards": result.Guards, - }) -} - -// mapRecordStepDecisionError classifies RecordAgentDecision failures into -// WS error codes. Only typed validation failures are safe to expose. Raw -// repository and engine failures become an opaque internal error. -func mapRecordStepDecisionError(msg *ws.Message, err error) (*ws.Message, error) { - if errors.Is(err, shared.ErrForbidden) { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeForbidden, err.Error(), nil) - } - if dashboard.IsAgentDecisionValidationError(err) { - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeValidation, err.Error(), nil) - } - return ws.NewError(msg.ID, msg.Action, ws.ErrorCodeInternalError, "failed to record step decision", nil) -} diff --git a/apps/backend/internal/mcp/handlers/agent_decision_handlers_test.go b/apps/backend/internal/mcp/handlers/agent_decision_handlers_test.go deleted file mode 100644 index ff9cd3d522..0000000000 --- a/apps/backend/internal/mcp/handlers/agent_decision_handlers_test.go +++ /dev/null @@ -1,157 +0,0 @@ -package handlers - -import ( - "context" - "errors" - "testing" - "time" - - "github.com/stretchr/testify/require" - - "github.com/kandev/kandev/internal/office/dashboard" - "github.com/kandev/kandev/internal/office/shared" - "github.com/kandev/kandev/internal/task/models" - sqliterepo "github.com/kandev/kandev/internal/task/repository/sqlite" - v1 "github.com/kandev/kandev/pkg/api/v1" - ws "github.com/kandev/kandev/pkg/websocket" -) - -// This file covers the record_step_decision_kandev MCP transport boundary -// (handleRecordStepDecision / mapRecordStepDecisionError). Review round 1 -// flagged this file at zero coverage. RecordAgentDecision's own business -// logic (role resolution, verdict/reason validation, engine re-evaluation) -// is already covered at the dashboard-service tier in -// internal/office/dashboard/agent_decisions_test.go — these tests instead -// exercise what's unique to this transport: payload validation, session -// lookup, the session/task ownership check, and error-code mapping. - -// seedRecordStepDecisionSession seeds a minimal workspace/task/session chain -// so handleRecordStepDecision's h.sessionRepo.GetTaskSession lookup and the -// session.TaskID ownership check have something real to read. -func seedRecordStepDecisionSession(t *testing.T, repo *sqliterepo.Repository, workspaceID, taskID, sessionID string) { - t.Helper() - ctx := context.Background() - now := time.Now().UTC() - require.NoError(t, repo.CreateWorkspace(ctx, &models.Workspace{ - ID: workspaceID, Name: "Decision WS", CreatedAt: now, UpdatedAt: now, - })) - require.NoError(t, repo.CreateTask(ctx, &models.Task{ - ID: taskID, WorkspaceID: workspaceID, Title: "Decision Task", - State: v1.TaskStateInProgress, CreatedAt: now, UpdatedAt: now, - })) - require.NoError(t, repo.CreateTaskSession(ctx, &models.TaskSession{ - ID: sessionID, TaskID: taskID, State: models.TaskSessionStateRunning, - StartedAt: now, UpdatedAt: now, - })) -} - -// newRecordStepDecisionHandler builds a *Handlers whose dashboardSvc is a -// real, minimally-constructed *dashboard.DashboardService. Every test case -// here returns before RecordAgentDecision ever touches its repo/dispatcher -// dependencies, so a nil Repository is safe — dashboardSvc only needs to be -// non-nil to pass handleRecordStepDecision's first guard. -func newRecordStepDecisionHandler(t *testing.T, repo *sqliterepo.Repository, svc *dashboard.DashboardService) *Handlers { - t.Helper() - return &Handlers{ - sessionRepo: repo, - dashboardSvc: svc, - logger: testLogger(t).WithFields(), - } -} - -func TestHandleRecordStepDecision_NotConfigured(t *testing.T) { - h := &Handlers{logger: testLogger(t).WithFields()} - msg := makeWSMessage(t, ws.ActionMCPRecordStepDecision, map[string]interface{}{ - "task_id": "t1", "session_id": "s1", "decision": "approved", "reason": "lgtm", - }) - resp, err := h.handleRecordStepDecision(context.Background(), msg) - require.NoError(t, err) - assertWSError(t, resp, ws.ErrorCodeInternalError) -} - -func TestHandleRecordStepDecision_InvalidPayload(t *testing.T) { - svc := dashboard.NewDashboardService(nil, testLogger(t), nil, nil, nil) - h := newRecordStepDecisionHandler(t, nil, svc) - msg := &ws.Message{ - ID: "test-id", Type: ws.MessageTypeRequest, - Action: ws.ActionMCPRecordStepDecision, Payload: []byte(`{"task_id":`), - } - resp, err := h.handleRecordStepDecision(context.Background(), msg) - require.NoError(t, err) - assertWSError(t, resp, ws.ErrorCodeBadRequest) -} - -func TestHandleRecordStepDecision_MissingFields(t *testing.T) { - svc := dashboard.NewDashboardService(nil, testLogger(t), nil, nil, nil) - cases := []struct { - name string - payload map[string]interface{} - }{ - { - name: "missing task_id", - payload: map[string]interface{}{"session_id": "s1", "decision": "approved", "reason": "lgtm"}, - }, - { - name: "missing session_id", - payload: map[string]interface{}{"task_id": "t1", "decision": "approved", "reason": "lgtm"}, - }, - } - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - h := newRecordStepDecisionHandler(t, nil, svc) - msg := makeWSMessage(t, ws.ActionMCPRecordStepDecision, tc.payload) - resp, err := h.handleRecordStepDecision(context.Background(), msg) - require.NoError(t, err) - assertWSError(t, resp, ws.ErrorCodeValidation) - }) - } -} - -func TestHandleRecordStepDecision_SessionNotFound(t *testing.T) { - svc := dashboard.NewDashboardService(nil, testLogger(t), nil, nil, nil) - _, repo := newTestTaskService(t) - h := newRecordStepDecisionHandler(t, repo, svc) - - msg := makeWSMessage(t, ws.ActionMCPRecordStepDecision, map[string]interface{}{ - "task_id": "missing-task", "session_id": "missing-session", "decision": "approved", "reason": "lgtm", - }) - resp, err := h.handleRecordStepDecision(context.Background(), msg) - require.NoError(t, err) - assertWSError(t, resp, ws.ErrorCodeNotFound) -} - -func TestHandleRecordStepDecision_SessionDoesNotBelongToTask(t *testing.T) { - svc := dashboard.NewDashboardService(nil, testLogger(t), nil, nil, nil) - _, repo := newTestTaskService(t) - seedRecordStepDecisionSession(t, repo, "ws-rd1", "task-rd1", "sess-rd1") - h := newRecordStepDecisionHandler(t, repo, svc) - - msg := makeWSMessage(t, ws.ActionMCPRecordStepDecision, map[string]interface{}{ - "task_id": "some-other-task", "session_id": "sess-rd1", "decision": "approved", "reason": "lgtm", - }) - resp, err := h.handleRecordStepDecision(context.Background(), msg) - require.NoError(t, err) - assertWSError(t, resp, ws.ErrorCodeValidation) -} - -func TestMapRecordStepDecisionError_ForbiddenMapsTo403(t *testing.T) { - msg := makeWSMessage(t, ws.ActionMCPRecordStepDecision, map[string]interface{}{}) - resp, err := mapRecordStepDecisionError(msg, shared.ErrForbidden) - require.NoError(t, err) - assertWSError(t, resp, ws.ErrorCodeForbidden) -} - -func TestMapRecordStepDecisionError_UnexpectedFailureMapsToInternal(t *testing.T) { - msg := makeWSMessage(t, ws.ActionMCPRecordStepDecision, map[string]interface{}{}) - resp, err := mapRecordStepDecisionError(msg, errors.New("task has no workflow step bound")) - require.NoError(t, err) - assertWSError(t, resp, ws.ErrorCodeInternalError) -} - -func TestMapRecordStepDecisionError_ValidationMapsToValidation(t *testing.T) { - msg := makeWSMessage(t, ws.ActionMCPRecordStepDecision, map[string]interface{}{}) - validationErr := &dashboard.AgentDecisionValidationError{Err: errors.New("reason is required")} - resp, err := mapRecordStepDecisionError(msg, validationErr) - require.NoError(t, err) - assertWSError(t, resp, ws.ErrorCodeValidation) -} diff --git a/apps/backend/internal/mcp/handlers/handlers.go b/apps/backend/internal/mcp/handlers/handlers.go index 3a16296840..ce1872bb08 100644 --- a/apps/backend/internal/mcp/handlers/handlers.go +++ b/apps/backend/internal/mcp/handlers/handlers.go @@ -21,7 +21,6 @@ import ( "github.com/kandev/kandev/internal/events" "github.com/kandev/kandev/internal/events/bus" mcpscope "github.com/kandev/kandev/internal/mcp/scope" - "github.com/kandev/kandev/internal/office/dashboard" "github.com/kandev/kandev/internal/orchestrator" "github.com/kandev/kandev/internal/orchestrator/executor" "github.com/kandev/kandev/internal/orchestrator/messagequeue" @@ -285,10 +284,6 @@ type Handlers struct { // MCP tools introduced in office task handoffs phase 2. handoffSvc *service.HandoffService - // Office dashboard service (optional, set via SetDashboardService). - // Wires the record_step_decision_kandev MCP tool. - dashboardSvc *dashboard.DashboardService - // Optional PR lister (set via SetTaskPRLister) used to enrich // task-listing responses with associated pull requests. taskPRLister TaskPRLister @@ -545,9 +540,6 @@ func (h *Handlers) registerConfigModeHandlers(d *guardedMCPDispatcher) { d.RegisterFunc(ws.ActionMCPGetTaskDocument, h.handleGetTaskDocument) d.RegisterFunc(ws.ActionMCPWriteTaskDocument, h.handleWriteTaskDocument) } - if h.dashboardSvc != nil { - d.RegisterFunc(ws.ActionMCPRecordStepDecision, h.handleRecordStepDecision) - } if h.taskSvc != nil { h.registerTaskConfigMutationHandlers(d) } diff --git a/apps/backend/internal/mcp/server/agent_decision_tool.go b/apps/backend/internal/mcp/server/agent_decision_tool.go deleted file mode 100644 index c225b29725..0000000000 --- a/apps/backend/internal/mcp/server/agent_decision_tool.go +++ /dev/null @@ -1,53 +0,0 @@ -package mcp - -import ( - "context" - "encoding/json" - "strings" - - "github.com/mark3labs/mcp-go/mcp" - "github.com/mark3labs/mcp-go/server" - - ws "github.com/kandev/kandev/pkg/websocket" -) - -// registerRecordStepDecisionTool registers record_step_decision_kandev, the -// Office quorum decision-recording tool. Per the tool contract in -// docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md, it takes no -// task_id — task/step/role are resolved server-side from the calling -// session, matching every other Office tool. -func (s *Server) registerRecordStepDecisionTool() { - s.mcpServer.AddTool( - mcp.NewTool("record_step_decision_kandev", - mcp.WithDescription(`Record an approve/reject decision for the current workflow step, as a reviewer or approver. Call this once you have finished evaluating the step's output. The task, step, and your role are resolved from your session — you cannot record a decision on another task. Not idempotent: a second call supersedes the first.`), - mcp.WithString("decision", mcp.Required(), mcp.Enum("approved", "rejected"), mcp.Description("The verdict: \"approved\" or \"rejected\".")), - mcp.WithString("reason", mcp.Required(), mcp.Description("Non-empty explanation for the verdict. Recorded in the task timeline.")), - ), - s.wrapHandler("record_step_decision_kandev", s.recordStepDecisionHandler()), - ) -} - -func (s *Server) recordStepDecisionHandler() server.ToolHandlerFunc { - return func(ctx context.Context, req mcp.CallToolRequest) (*mcp.CallToolResult, error) { - if s.taskID == "" || s.sessionID == "" { - return mcp.NewToolResultError("record_step_decision_kandev requires a bound task and session"), nil - } - decision := strings.TrimSpace(req.GetString("decision", "")) - reason := strings.TrimSpace(req.GetString("reason", "")) - if reason == "" { - return mcp.NewToolResultError("reason is required"), nil - } - payload := map[string]interface{}{ - "task_id": s.taskID, - "session_id": s.sessionID, - "decision": decision, - "reason": reason, - } - var result map[string]interface{} - if err := s.backend.RequestPayload(ctx, ws.ActionMCPRecordStepDecision, payload, &result); err != nil { - return mcp.NewToolResultError(err.Error()), nil - } - data, _ := json.MarshalIndent(result, "", " ") - return mcp.NewToolResultText(string(data)), nil - } -} diff --git a/apps/backend/internal/mcp/server/agent_decision_tool_test.go b/apps/backend/internal/mcp/server/agent_decision_tool_test.go deleted file mode 100644 index f25920e22a..0000000000 --- a/apps/backend/internal/mcp/server/agent_decision_tool_test.go +++ /dev/null @@ -1,88 +0,0 @@ -package mcp - -import ( - "testing" - - ws "github.com/kandev/kandev/pkg/websocket" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// This file covers the record_step_decision_kandev tool-registration/dispatch -// half of the MCP transport (registerRecordStepDecisionTool / -// recordStepDecisionHandler). Review round 1 flagged this file at zero -// coverage. The WS-handler half (handleRecordStepDecision, -// mapRecordStepDecisionError) is covered separately in -// internal/mcp/handlers/agent_decision_handlers_test.go; these tests instead -// exercise what's unique to this transport boundary: the bound-session guard, -// trimming/validation before dispatch, the exact payload shape sent to -// s.backend.RequestPayload, and surfacing a backend error as a tool error. - -func newOfficeModeServer(t *testing.T, backend BackendClient, sessionID, taskID string) *Server { - t.Helper() - return New(backend, sessionID, taskID, 10005, newTestLogger(t), "", false, ModeOffice) -} - -func TestRecordStepDecision_RequiresBoundTaskAndSession(t *testing.T) { - backend := &testBackend{} - s := newOfficeModeServer(t, backend, "", "") - - result := callTool(t, s, "record_step_decision_kandev", map[string]interface{}{ - "decision": "approved", - "reason": "looks good", - }) - - require.True(t, result.IsError, "unbound task/session must be rejected") - assert.Empty(t, backend.lastAction, "backend must not be called without a bound session") -} - -func TestRecordStepDecision_RejectsEmptyReason(t *testing.T) { - backend := &testBackend{} - s := newOfficeModeServer(t, backend, "test-session", "task-current") - - result := callTool(t, s, "record_step_decision_kandev", map[string]interface{}{ - "decision": "approved", - "reason": " ", - }) - - require.True(t, result.IsError, "whitespace-only reason must be rejected") - assert.Empty(t, backend.lastAction, "backend must not be called with an empty reason") -} - -func TestRecordStepDecision_SendsExpectedPayload(t *testing.T) { - backend := &testBackend{ - response: map[string]interface{}{ - "decision": "approved", - "role": "reviewer", - "step_id": "step-1", - }, - } - s := newOfficeModeServer(t, backend, "test-session", "task-current") - - result := callTool(t, s, "record_step_decision_kandev", map[string]interface{}{ - "decision": "approved", - "reason": " looks good ", - }) - - require.False(t, result.IsError) - assert.Equal(t, ws.ActionMCPRecordStepDecision, backend.lastAction) - - payload, ok := backend.lastPayload.(map[string]interface{}) - require.True(t, ok) - assert.Equal(t, "task-current", payload["task_id"]) - assert.Equal(t, "test-session", payload["session_id"]) - assert.Equal(t, "approved", payload["decision"]) - assert.Equal(t, "looks good", payload["reason"], "reason should be trimmed before dispatch") -} - -func TestRecordStepDecision_SurfacesBackendErrorAsToolError(t *testing.T) { - backend := &testBackend{err: assert.AnError} - s := newOfficeModeServer(t, backend, "test-session", "task-current") - - result := callTool(t, s, "record_step_decision_kandev", map[string]interface{}{ - "decision": "rejected", - "reason": "needs work", - }) - - require.True(t, result.IsError, "a backend error must surface as a tool error, not panic or succeed") -} diff --git a/apps/backend/internal/mcp/server/server.go b/apps/backend/internal/mcp/server/server.go index e601353f77..360bb0eb7c 100644 --- a/apps/backend/internal/mcp/server/server.go +++ b/apps/backend/internal/mcp/server/server.go @@ -1094,7 +1094,6 @@ func (s *Server) profileToolGroups() []profileToolGroup { {name: "review", enabled: kanban, register: func(s *Server) { s.registerReviewTools() }}, {name: "related-tasks", enabled: func(ctx mcpprofile.Context) bool { return kanban(ctx) || office(ctx) }, register: func(s *Server) { s.registerRelatedTasksTool() }}, {name: "office-documents", enabled: office, register: func(s *Server) { s.registerTaskDocumentTools() }}, - {name: "office-decisions", enabled: office, register: func(s *Server) { s.registerRecordStepDecisionTool() }}, {name: "task-branch-sources", enabled: kanban, register: func(s *Server) { s.registerAddBranchToTaskTool() s.registerAddWorkspaceSourcesTool() diff --git a/apps/backend/internal/mcp/server/server_test.go b/apps/backend/internal/mcp/server/server_test.go index d27c5a8bef..8096d55342 100644 --- a/apps/backend/internal/mcp/server/server_test.go +++ b/apps/backend/internal/mcp/server/server_test.go @@ -1051,11 +1051,11 @@ func TestServerModeOffice_ToolCount(t *testing.T) { s := New(backend, "test-session", "test-task", 10005, log, "", false, ModeOffice) tools := getRegisteredToolNames(s) // 4 plan + 1 interaction + 1 related-tasks + 3 task-documents - // + 1 rich-output + 1 decisions + 1 step_complete (ADR 0015) = 12. + // + 1 rich-output + 1 step_complete (ADR 0015) = 11. // (delegate_task_kandev retired in favour of `agentctl kandev task create …`). // (list_task_comments_kandev retired in favour of `agentctl kandev comment list …`). assert.Contains(t, tools, "step_complete_kandev", "office mode must register the ADR 0015 completion signal") - assert.Equal(t, 12, len(tools)) + assert.Equal(t, 11, len(tools)) } func TestServerModeOffice_DisableAskQuestion(t *testing.T) { @@ -1073,9 +1073,9 @@ func TestServerModeOffice_DisableAskQuestion(t *testing.T) { // the agentctl CLI as `agentctl kandev task create --parent …`). assert.NotContains(t, tools, "delegate_task_kandev") // 4 plan + 1 related-tasks + 3 task-documents + 1 rich-output - // + 1 decisions + 1 step_complete (ADR 0015) = 11 + // + 1 step_complete (ADR 0015) = 10 // (no ask_user_question, no delegate, no list_task_comments) - assert.Equal(t, 11, len(tools)) + assert.Equal(t, 10, len(tools)) } func TestServerModeConstants(t *testing.T) { diff --git a/apps/backend/internal/mcp/server/sysprompt_sync_test.go b/apps/backend/internal/mcp/server/sysprompt_sync_test.go index 91120c3690..dfcd1a135f 100644 --- a/apps/backend/internal/mcp/server/sysprompt_sync_test.go +++ b/apps/backend/internal/mcp/server/sysprompt_sync_test.go @@ -274,6 +274,23 @@ func TestSyspromptToolNames_ExactlyMatchMCPOfficeMode(t *testing.T) { assert.Equal(t, registered, referenced, "Office first-turn context must advertise exactly the ModeOffice tool inventory") + assert.Len(t, registered, 11, "Office MCP must expose the replacement 11-tool catalog") +} + +func TestDecisionMCPTransportIsAbsentFromOfficeAndKanban(t *testing.T) { + log := newTestLogger(t) + backend := NewChannelBackendClient(log) + defer backend.Close() + + office := New(backend, "office-session", "office-task", 10005, log, "", false, ModeOffice) + kanban := New(backend, "task-session", "task", 10005, log, "", false, ModeTask) + require.NotNil(t, office) + require.NotNil(t, kanban) + + assert.NotContains(t, office.mcpServer.ListTools(), "record_step_decision_kandev") + assert.NotContains(t, sysprompt.OfficeContext(), "record_step_decision_kandev") + assert.NotContains(t, kanban.mcpServer.ListTools(), "record_step_decision_kandev") + assert.NotContains(t, sysprompt.KandevContext(), "record_step_decision") } // TestSyspromptToolNames_NoBareToolReferences catches the opposite drift: a diff --git a/apps/backend/internal/office/configloader/skills/kandev-step-decision/SKILL.md b/apps/backend/internal/office/configloader/skills/kandev-step-decision/SKILL.md new file mode 100644 index 0000000000..989356cd53 --- /dev/null +++ b/apps/backend/internal/office/configloader/skills/kandev-step-decision/SKILL.md @@ -0,0 +1,43 @@ +--- +name: kandev-step-decision +description: Record the current workflow step decision through the task-bound Office CLI. +kandev: + system: true + version: "0.42.0" +--- + +# Workflow step decisions + +Use the task-bound CLI command when you hold the current review or approval +seat and the run prompt advertises the decision action. + +## Record the decision + +The only accepted verdicts are `approved` and `rejected`. Use a non-empty reason +that explains the evidence for the verdict. + +```bash +$KANDEV_CLI kandev task decision --decision approved --reason "..." +``` + +Use `approved` as shown, or replace it with `rejected` when rejecting. Replace +the reason with the evidence for the current workflow step. Make this command +the final action for the turn, then stop. A repeated +decision supersedes the previous decision for that participant and step. + +The command returns structured JSON with these fields: + +```json +{ + "decision": "approved", + "role": "reviewer", + "step_id": "step-1", + "decision_id": "decision-1", + "decided_at": "2026-01-01T00:00:00Z", + "transition_applied": false, + "guards": [] +} +``` + +Posting comments do not count as a workflow step decision. Approval-inbox commands do not count +either. Do not use either operation instead of the task decision command. diff --git a/apps/backend/internal/office/dashboard/agent_decisions.go b/apps/backend/internal/office/dashboard/agent_decisions.go index 3108360b7a..ef99494b93 100644 --- a/apps/backend/internal/office/dashboard/agent_decisions.go +++ b/apps/backend/internal/office/dashboard/agent_decisions.go @@ -16,7 +16,7 @@ import ( const agentDecisionReasonRequiredErr = "reason is required" -// RecordAgentDecisionInput bundles the agent decision tool's caller-supplied +// RecordAgentDecisionInput bundles the runtime decision endpoint's caller-supplied // fields. Per the tool contract, task/step/role are never caller-supplied — // they are resolved server-side from the calling session and the AC-50 // slate. @@ -28,9 +28,9 @@ type RecordAgentDecisionInput struct { // SessionID, when features.officeSessionIdentity is on, names the // decider's own calling session so RecordDecision re-evaluates against // it instead of the task's most-recently-started ("active") session. - // Populated unconditionally by the MCP handler; gated here because the - // flag decision belongs with the rest of this service's behavior, not - // the transport layer. + // The runtime handler derives it from the signed RunContext and passes it + // unconditionally; gated here because the flag decision belongs with the + // rest of this service's behavior, not the transport layer. SessionID string } @@ -57,7 +57,7 @@ func (e *AgentDecisionValidationError) Unwrap() error { } // IsAgentDecisionValidationError reports whether err is safe to expose as a -// validation response at the MCP boundary. +// validation response at the runtime boundary. func IsAgentDecisionValidationError(err error) bool { var target *AgentDecisionValidationError return errors.As(err, &target) @@ -67,7 +67,7 @@ func agentDecisionValidation(err error) error { return &AgentDecisionValidationError{Err: err} } -// RecordAgentDecisionResult is the AC-64 tool-contract return shape. +// RecordAgentDecisionResult is the AC-64 runtime contract return shape. type RecordAgentDecisionResult struct { Decision string Role string @@ -92,8 +92,8 @@ type quorumEvaluatingDispatcher interface { EvaluateStepQuorum(ctx context.Context, taskID string) (engine.QuorumSnapshot, error) } -// RecordAgentDecision is the agent/MCP counterpart to ApproveTask / -// RequestTaskChanges (`record_step_decision_kandev`). Unlike the human +// RecordAgentDecision is the agent-runtime counterpart to ApproveTask / +// RequestTaskChanges. Unlike the human // path — whose resolveDeciderRole/resolveParticipantID stay unchanged and // step-scoped per AC-57b — this path resolves role and seat over the // AC-50 population via the engine's ResolveParticipantRole (AC-4a): a new diff --git a/apps/backend/internal/office/dashboard/quorum_dto.go b/apps/backend/internal/office/dashboard/quorum_dto.go index 7f8b72d62d..6582d1b54b 100644 --- a/apps/backend/internal/office/dashboard/quorum_dto.go +++ b/apps/backend/internal/office/dashboard/quorum_dto.go @@ -4,7 +4,7 @@ import "github.com/kandev/kandev/internal/workflow/engine" // GuardStateDTO is the direct JSON projection of a single AC-57d guard // entry (`engine.QuorumGuardState`), shared verbatim by the AC-24b HTTP -// quorum endpoint and the AC-64 agent decision tool response so the two +// quorum endpoint and the AC-64 runtime decision response so the two // surfaces can never report different arithmetic for the same step. type GuardStateDTO struct { TargetStepID string `json:"target_step_id"` diff --git a/apps/backend/internal/office/decision_adapter.go b/apps/backend/internal/office/decision_adapter.go new file mode 100644 index 0000000000..4f6d5375dd --- /dev/null +++ b/apps/backend/internal/office/decision_adapter.go @@ -0,0 +1,68 @@ +package office + +import ( + "context" + "fmt" + + "github.com/kandev/kandev/internal/office/dashboard" + officeruntime "github.com/kandev/kandev/internal/office/runtime" +) + +type dashboardDecisionRecorder interface { + RecordAgentDecision(ctx context.Context, input dashboard.RecordAgentDecisionInput) (*dashboard.RecordAgentDecisionResult, error) +} + +type runtimeDecisionRecorder struct { + dashboard dashboardDecisionRecorder +} + +func newRuntimeDecisionRecorder(svc dashboardDecisionRecorder) officeruntime.DecisionRecorder { + if svc == nil { + return nil + } + return &runtimeDecisionRecorder{dashboard: svc} +} + +func (r *runtimeDecisionRecorder) RecordAgentDecision( + ctx context.Context, + input officeruntime.RecordAgentDecisionInput, +) (officeruntime.RecordAgentDecisionResult, error) { + result, err := r.dashboard.RecordAgentDecision(ctx, dashboard.RecordAgentDecisionInput{ + TaskID: input.TaskID, + AgentProfileID: input.AgentProfileID, + SessionID: input.SessionID, + Decision: input.Decision, + Reason: input.Reason, + }) + if err != nil { + if dashboard.IsAgentDecisionValidationError(err) { + return officeruntime.RecordAgentDecisionResult{}, officeruntime.NewDecisionValidationError(err) + } + return officeruntime.RecordAgentDecisionResult{}, err + } + if result == nil { + return officeruntime.RecordAgentDecisionResult{}, fmt.Errorf("record agent decision returned no result") + } + guards := make([]officeruntime.DecisionGuard, 0, len(result.Guards)) + for _, guard := range result.Guards { + guards = append(guards, officeruntime.DecisionGuard{ + TargetStepID: guard.TargetStepID, + Role: guard.Role, + Threshold: guard.Threshold, + RequiredCount: guard.RequiredCount, + ReceivedCount: guard.ReceivedCount, + Satisfied: guard.Satisfied, + Reason: guard.Reason, + Error: guard.Error, + }) + } + return officeruntime.RecordAgentDecisionResult{ + Decision: result.Decision, + Role: result.Role, + StepID: result.StepID, + DecisionID: result.DecisionID, + DecidedAt: result.DecidedAt, + TransitionApplied: result.TransitionApplied, + Guards: guards, + }, nil +} diff --git a/apps/backend/internal/office/decision_adapter_test.go b/apps/backend/internal/office/decision_adapter_test.go new file mode 100644 index 0000000000..6b926bdc5f --- /dev/null +++ b/apps/backend/internal/office/decision_adapter_test.go @@ -0,0 +1,79 @@ +package office + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/kandev/kandev/internal/office/dashboard" + officeruntime "github.com/kandev/kandev/internal/office/runtime" +) + +type fakeDashboardDecisionRecorder struct { + input dashboard.RecordAgentDecisionInput + result *dashboard.RecordAgentDecisionResult + err error +} + +func (f *fakeDashboardDecisionRecorder) RecordAgentDecision( + _ context.Context, + input dashboard.RecordAgentDecisionInput, +) (*dashboard.RecordAgentDecisionResult, error) { + f.input = input + return f.result, f.err +} + +func TestRuntimeDecisionRecorderForwardsIdentityAndMapsResult(t *testing.T) { + decidedAt := time.Date(2026, 9, 7, 20, 0, 0, 0, time.UTC) + fake := &fakeDashboardDecisionRecorder{result: &dashboard.RecordAgentDecisionResult{ + Decision: "approved", + Role: "approver", + StepID: "step-1", + DecisionID: "decision-1", + DecidedAt: decidedAt, + TransitionApplied: true, + Guards: []dashboard.GuardStateDTO{{ + TargetStepID: "step-2", + Role: "approver", + Threshold: "all_approve", + RequiredCount: 1, + ReceivedCount: 1, + Satisfied: true, + }}, + }} + recorder := newRuntimeDecisionRecorder(fake) + + got, err := recorder.RecordAgentDecision(context.Background(), officeruntime.RecordAgentDecisionInput{ + TaskID: "task-1", + AgentProfileID: "agent-1", + SessionID: "session-1", + Decision: "approved", + Reason: "ready", + }) + if err != nil { + t.Fatalf("RecordAgentDecision: %v", err) + } + if fake.input.TaskID != "task-1" || fake.input.AgentProfileID != "agent-1" || fake.input.SessionID != "session-1" || + fake.input.Decision != "approved" || fake.input.Reason != "ready" { + t.Fatalf("dashboard input = %+v", fake.input) + } + if got.Decision != "approved" || got.Role != "approver" || got.StepID != "step-1" || + got.DecisionID != "decision-1" || !got.DecidedAt.Equal(decidedAt) || !got.TransitionApplied || len(got.Guards) != 1 { + t.Fatalf("runtime result = %+v", got) + } + if got.Guards[0].TargetStepID != "step-2" || got.Guards[0].RequiredCount != 1 || !got.Guards[0].Satisfied { + t.Fatalf("runtime guard = %+v", got.Guards[0]) + } +} + +func TestRuntimeDecisionRecorderClassifiesDashboardValidation(t *testing.T) { + fake := &fakeDashboardDecisionRecorder{err: &dashboard.AgentDecisionValidationError{Err: errors.New("reason is required")}} + recorder := newRuntimeDecisionRecorder(fake) + + _, err := recorder.RecordAgentDecision(context.Background(), officeruntime.RecordAgentDecisionInput{}) + var validation *officeruntime.DecisionValidationError + if !errors.As(err, &validation) { + t.Fatalf("error = %v, want runtime validation error", err) + } +} diff --git a/apps/backend/internal/office/engine_dispatcher/dispatcher.go b/apps/backend/internal/office/engine_dispatcher/dispatcher.go index 75c0c98787..96f8edac16 100644 --- a/apps/backend/internal/office/engine_dispatcher/dispatcher.go +++ b/apps/backend/internal/office/engine_dispatcher/dispatcher.go @@ -55,7 +55,7 @@ type EngineHandle interface { RecordParticipantDecision(ctx context.Context, sessionID string, in engine.DecisionInfo) (engine.RecordDecisionResult, error) EvaluateStepQuorum(ctx context.Context, taskID, sessionID string) (engine.QuorumSnapshot, error) // ResolveParticipantRole is the AC-2/3/4/4a role-and-seat resolution - // entry point the agent decision tool needs. Reached the same way as + // entry point the runtime decision path needs. Reached the same way as // the two methods above: via Dispatcher.ResolveParticipantRole plus a // narrow caller-side type assertion. ResolveParticipantRole(ctx context.Context, taskID, stepID, agentProfileID string) (role, participantID string, err error) diff --git a/apps/backend/internal/office/repository/sqlite/step_decisions.go b/apps/backend/internal/office/repository/sqlite/step_decisions.go index b0d1a79200..e5323fe7c5 100644 --- a/apps/backend/internal/office/repository/sqlite/step_decisions.go +++ b/apps/backend/internal/office/repository/sqlite/step_decisions.go @@ -9,7 +9,7 @@ import ( // HasActiveStepDecision reports whether an active (non-superseded) row exists // in workflow_step_decisions for the given task, step, and decider. It backs // the review/approval completion check that flags a reviewer or approver -// turn that ended without recording a verdict via record_step_decision_kandev. +// turn that ended without recording a workflow-step verdict. func (r *Repository) HasActiveStepDecision(ctx context.Context, taskID, stepID, deciderID string) (bool, error) { var exists int err := r.ro.QueryRowxContext(ctx, r.ro.Rebind(` diff --git a/apps/backend/internal/office/routes.go b/apps/backend/internal/office/routes.go index 12bd144ed8..20ecbdc883 100644 --- a/apps/backend/internal/office/routes.go +++ b/apps/backend/internal/office/routes.go @@ -42,6 +42,8 @@ func RegisterAllRoutes(router *gin.RouterGroup, svcs *Services, handoff *taskser }), svcs.Skills, svcs.Workspaces, + newRuntimeDecisionRecorder(svcs.Dashboard), + log, )) skillsHandler := skills.NewHandler(svcs.Skills) diff --git a/apps/backend/internal/office/runtime/capabilities.go b/apps/backend/internal/office/runtime/capabilities.go index 65741eaed4..3b3ec6425b 100644 --- a/apps/backend/internal/office/runtime/capabilities.go +++ b/apps/backend/internal/office/runtime/capabilities.go @@ -27,8 +27,9 @@ const ( CapabilityDeleteSkills = "delete_skills" ) -// AvailableActionRecordStepDecision identifies the MCP decision tool that a -// run prompt may advertise when the agent holds the current workflow seat. +// AvailableActionRecordStepDecision identifies the advisory runtime decision +// action that a run prompt may advertise when the agent holds the current +// workflow seat. const AvailableActionRecordStepDecision = "record_step_decision" // Allows reports whether the named runtime capability is granted. diff --git a/apps/backend/internal/office/runtime/context_builder.go b/apps/backend/internal/office/runtime/context_builder.go index 625ce3c4a3..f151a0da05 100644 --- a/apps/backend/internal/office/runtime/context_builder.go +++ b/apps/backend/internal/office/runtime/context_builder.go @@ -24,7 +24,7 @@ type RunSnapshotStore interface { // seat (reviewer or approver) at a task's current workflow step. Implemented // by *service.Service via HoldsDecisionSeat, mirroring the authorization // RecordAgentDecision itself applies. The result is advisory prompt metadata; -// the decision tool performs live authorization when it is called. +// the runtime decision endpoint performs live authorization when it is called. type DecisionSeatResolver interface { HoldsDecisionSeat(ctx context.Context, taskID, agentProfileID string) (bool, error) } diff --git a/apps/backend/internal/office/runtime/decision.go b/apps/backend/internal/office/runtime/decision.go new file mode 100644 index 0000000000..c1ce047c59 --- /dev/null +++ b/apps/backend/internal/office/runtime/decision.go @@ -0,0 +1,74 @@ +package runtime + +import ( + "context" + "fmt" + "time" +) + +// DecisionRecorder is the composition-boundary dependency for the signed +// Office task decision endpoint. +type DecisionRecorder interface { + RecordAgentDecision(ctx context.Context, input RecordAgentDecisionInput) (RecordAgentDecisionResult, error) +} + +// RecordAgentDecisionInput contains only identity resolved from the signed run +// and the two caller-controlled decision fields. +type RecordAgentDecisionInput struct { + TaskID string + AgentProfileID string + SessionID string + Decision string + Reason string +} + +// DecisionGuard is the JSON shape shared by the runtime decision response and +// the Office quorum diagnostic response. +type DecisionGuard struct { + TargetStepID string `json:"target_step_id"` + Role string `json:"role"` + Threshold string `json:"threshold"` + RequiredCount int `json:"required_count"` + ReceivedCount int `json:"received_count"` + Satisfied bool `json:"satisfied"` + Reason string `json:"reason,omitempty"` + Error string `json:"error,omitempty"` +} + +// RecordAgentDecisionResult is the seven-field task decision contract. +type RecordAgentDecisionResult struct { + Decision string `json:"decision"` + Role string `json:"role"` + StepID string `json:"step_id"` + DecisionID string `json:"decision_id"` + DecidedAt time.Time `json:"decided_at"` + TransitionApplied bool `json:"transition_applied"` + Guards []DecisionGuard `json:"guards"` +} + +// DecisionValidationError identifies a caller-correctable decision request. +type DecisionValidationError struct { + Err error +} + +func (e *DecisionValidationError) Error() string { + if e == nil || e.Err == nil { + return "invalid agent decision" + } + return e.Err.Error() +} + +func (e *DecisionValidationError) Unwrap() error { + if e == nil { + return nil + } + return e.Err +} + +// NewDecisionValidationError preserves validation classification at the +// runtime/composition boundary without importing the dashboard package here. +func NewDecisionValidationError(err error) error { + return &DecisionValidationError{Err: err} +} + +var errDecisionRecorderMissing = fmt.Errorf("%w: decision recorder", ErrRuntimeDependencyMissing) diff --git a/apps/backend/internal/office/runtime/handler.go b/apps/backend/internal/office/runtime/handler.go index 26874d6f23..7e4a206be5 100644 --- a/apps/backend/internal/office/runtime/handler.go +++ b/apps/backend/internal/office/runtime/handler.go @@ -4,11 +4,14 @@ import ( "context" "encoding/json" "errors" + "io" "net/http" "strings" "github.com/gin-gonic/gin" + "go.uber.org/zap" + commonlogger "github.com/kandev/kandev/internal/common/logger" "github.com/kandev/kandev/internal/office/agents" "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/shared" @@ -20,6 +23,8 @@ type Handler struct { actions *Actions skillLister SkillLister runEvents RunEventAppender + decisions DecisionRecorder + logger *commonlogger.Logger } // RunEventAppender records runtime behavior against a run. @@ -33,18 +38,26 @@ func NewHandler( actions *Actions, skillLister SkillLister, runEvents RunEventAppender, + decisions DecisionRecorder, + log *commonlogger.Logger, ) *Handler { + if log == nil { + log = commonlogger.Default() + } return &Handler{ agentSvc: agentSvc, actions: actions, skillLister: skillLister, runEvents: runEvents, + decisions: decisions, + logger: log, } } // RegisterRoutes mounts runtime syscall routes. func RegisterRoutes(group *gin.RouterGroup, h *Handler) { group.POST("/runtime/comments", h.postComment) + group.POST("/runtime/task/decision", h.recordAgentDecision) group.POST("/runtime/tasks/:id/status", h.updateTaskStatus) group.POST("/runtime/tasks/:id/subtasks", h.createSubtask) group.POST("/runtime/tasks", h.createTask) @@ -60,6 +73,43 @@ func RegisterRoutes(group *gin.RouterGroup, h *Handler) { group.DELETE("/runtime/skills/:id", h.deleteSkill) } +type recordAgentDecisionRequest struct { + Decision string `json:"decision"` + Reason string `json:"reason"` +} + +func (h *Handler) recordAgentDecision(c *gin.Context) { + runCtx, _, ok := h.contextFromRequest(c) + if !ok { + return + } + if runCtx.TaskID == "" { + c.JSON(http.StatusBadRequest, gin.H{"error": "decision requires a task-bound runtime"}) + return + } + var req recordAgentDecisionRequest + if !bindClosedJSON(c, &req) { + return + } + if h.decisions == nil { + h.respondRuntimeError(c, runCtx, "record_agent_decision", "task", runCtx.TaskID, errDecisionRecorderMissing) + return + } + result, err := h.decisions.RecordAgentDecision(c.Request.Context(), RecordAgentDecisionInput{ + TaskID: runCtx.TaskID, + AgentProfileID: runCtx.AgentID, + SessionID: runCtx.SessionID, + Decision: req.Decision, + Reason: req.Reason, + }) + if err != nil { + h.respondRuntimeError(c, runCtx, "record_agent_decision", "task", runCtx.TaskID, err) + return + } + h.appendActionRunEvent(c.Request.Context(), runCtx, "record_agent_decision", "task", runCtx.TaskID) + c.JSON(http.StatusOK, result) +} + func (h *Handler) createTask(c *gin.Context) { runCtx, _, ok := h.contextFromRequest(c) if !ok { @@ -392,6 +442,24 @@ func bindJSON(c *gin.Context, target any) bool { return true } +func bindClosedJSON(c *gin.Context, target any) bool { + decoder := json.NewDecoder(c.Request.Body) + decoder.DisallowUnknownFields() + if err := decoder.Decode(target); err != nil { + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return false + } + var extra any + if err := decoder.Decode(&extra); err != io.EOF { + if err == nil { + err = errors.New("request body must contain one JSON value") + } + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return false + } + return true +} + func bearerToken(header string) string { const prefix = "Bearer " if !strings.HasPrefix(header, prefix) { @@ -413,12 +481,26 @@ func (h *Handler) respondRuntimeError( c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) return } + var decisionValidation *DecisionValidationError + if errors.As(err, &decisionValidation) { + h.appendDeniedRunEvent(c.Request.Context(), runCtx, action, targetType, targetID, err) + c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) + return + } if errors.Is(err, shared.ErrForbidden) { h.appendDeniedRunEvent(c.Request.Context(), runCtx, action, targetType, targetID, err) c.JSON(http.StatusForbidden, gin.H{"error": err.Error()}) return } - c.JSON(http.StatusInternalServerError, gin.H{"error": err.Error()}) + h.logger.Error("office runtime action failed", + zap.String("action", action), + zap.String("target_type", targetType), + zap.String("target_id", targetID), + zap.String("run_id", runCtx.RunID), + zap.String("agent_id", runCtx.AgentID), + zap.Error(err), + ) + c.JSON(http.StatusInternalServerError, gin.H{"error": "internal runtime error"}) } func (h *Handler) respondTaskStatusError(c *gin.Context, runCtx RunContext, taskID string, err error) { diff --git a/apps/backend/internal/office/runtime/handler_decision_test.go b/apps/backend/internal/office/runtime/handler_decision_test.go new file mode 100644 index 0000000000..c56122c555 --- /dev/null +++ b/apps/backend/internal/office/runtime/handler_decision_test.go @@ -0,0 +1,158 @@ +package runtime + +import ( + "context" + "encoding/json" + "errors" + "net/http" + "strings" + "testing" + "time" + + "github.com/kandev/kandev/internal/office/shared" +) + +type recordingDecisionRecorder struct { + inputs []RecordAgentDecisionInput + result RecordAgentDecisionResult + err error +} + +func (r *recordingDecisionRecorder) RecordAgentDecision( + _ context.Context, + input RecordAgentDecisionInput, +) (RecordAgentDecisionResult, error) { + r.inputs = append(r.inputs, input) + return r.result, r.err +} + +func TestRuntimeHandler_RecordAgentDecisionRejectsCallerIdentityFields(t *testing.T) { + h := newRuntimeHandlerHarness(t, Capabilities{}) + + resp := h.request(t, http.MethodPost, "/runtime/task/decision", map[string]string{ + "decision": "approved", + "reason": "looks good", + "task_id": "forged-task", + }) + if resp.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body=%s", resp.Code, http.StatusBadRequest, resp.Body.String()) + } +} + +func TestRuntimeHandler_RecordAgentDecisionUsesSignedRunIdentity(t *testing.T) { + h := newRuntimeHandlerHarness(t, Capabilities{}) + h.decisions.result = RecordAgentDecisionResult{ + Decision: "approved", + Role: "reviewer", + StepID: "step-1", + DecisionID: "decision-1", + DecidedAt: mustDecisionTime(t, "2026-09-07T20:00:00Z"), + TransitionApplied: true, + Guards: []DecisionGuard{}, + } + + resp := h.request(t, http.MethodPost, "/runtime/task/decision", map[string]string{ + "decision": "approved", + "reason": "looks good", + }) + if resp.Code != http.StatusOK { + t.Fatalf("status = %d, want %d; body=%s", resp.Code, http.StatusOK, resp.Body.String()) + } + if len(h.decisions.inputs) != 1 { + t.Fatalf("decision inputs = %d, want 1", len(h.decisions.inputs)) + } + input := h.decisions.inputs[0] + if input.TaskID != "task-1" || input.AgentProfileID != "agent-1" || input.SessionID != "sess-1" || + input.Decision != "approved" || input.Reason != "looks good" { + t.Fatalf("decision input = %+v", input) + } + + var body RecordAgentDecisionResult + if err := json.Unmarshal(resp.Body.Bytes(), &body); err != nil { + t.Fatalf("decode decision response: %v", err) + } + if body.Decision != "approved" || body.Role != "reviewer" || body.StepID != "step-1" || + body.DecisionID != "decision-1" || !body.TransitionApplied || body.Guards == nil { + t.Fatalf("decision response = %+v", body) + } + assertActionRunEvent(t, h.runEvents, "record_agent_decision", "task", "task-1") +} + +func TestRuntimeHandler_RecordAgentDecisionRejectsTasklessRun(t *testing.T) { + h := newRuntimeHandlerHarness(t, Capabilities{}) + token, err := h.agentSvc.MintRuntimeJWT("agent-1", "", "ws-1", "run-1", "sess-1", "") + if err != nil { + t.Fatalf("mint taskless runtime token: %v", err) + } + h.token = token + + resp := h.request(t, http.MethodPost, "/runtime/task/decision", map[string]string{ + "decision": "approved", + "reason": "looks good", + }) + if resp.Code != http.StatusBadRequest { + t.Fatalf("status = %d, want %d; body=%s", resp.Code, http.StatusBadRequest, resp.Body.String()) + } + if !strings.Contains(resp.Body.String(), "decision requires a task-bound runtime") { + t.Fatalf("body = %q, want task-bound runtime error", resp.Body.String()) + } + if len(h.decisions.inputs) != 0 { + t.Fatalf("decision inputs = %d, want 0", len(h.decisions.inputs)) + } +} + +func TestRuntimeHandler_RecordAgentDecisionMapsValidationAndPermissionErrors(t *testing.T) { + tests := []struct { + name string + err error + wantStatus int + wantEvent string + }{ + {name: "validation", err: NewDecisionValidationError(errors.New("invalid decision")), wantStatus: http.StatusBadRequest, wantEvent: "runtime.denied"}, + {name: "permission", err: shared.ErrForbidden, wantStatus: http.StatusForbidden, wantEvent: "runtime.denied"}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + h := newRuntimeHandlerHarness(t, Capabilities{}) + h.decisions.err = tc.err + resp := h.request(t, http.MethodPost, "/runtime/task/decision", map[string]string{ + "decision": "approved", + "reason": "looks good", + }) + if resp.Code != tc.wantStatus { + t.Fatalf("status = %d, want %d; body=%s", resp.Code, tc.wantStatus, resp.Body.String()) + } + if len(h.runEvents.events) != 1 || h.runEvents.events[0].eventType != tc.wantEvent { + t.Fatalf("run events = %#v, want one %s event", h.runEvents.events, tc.wantEvent) + } + }) + } +} + +func TestRuntimeHandler_RecordAgentDecisionHidesUnexpectedErrors(t *testing.T) { + h := newRuntimeHandlerHarness(t, Capabilities{}) + h.decisions.err = errors.New("database password leaked") + + resp := h.request(t, http.MethodPost, "/runtime/task/decision", map[string]string{ + "decision": "approved", + "reason": "looks good", + }) + if resp.Code != http.StatusInternalServerError { + t.Fatalf("status = %d, want %d; body=%s", resp.Code, http.StatusInternalServerError, resp.Body.String()) + } + if strings.Contains(resp.Body.String(), "database password leaked") { + t.Fatalf("body leaked internal error: %q", resp.Body.String()) + } + if !strings.Contains(resp.Body.String(), "internal runtime error") { + t.Fatalf("body = %q, want opaque runtime error", resp.Body.String()) + } +} + +func mustDecisionTime(t *testing.T, value string) time.Time { + t.Helper() + parsed, err := time.Parse(time.RFC3339, value) + if err != nil { + t.Fatalf("parse decision time: %v", err) + } + return parsed +} diff --git a/apps/backend/internal/office/runtime/handler_test.go b/apps/backend/internal/office/runtime/handler_test.go index 7321fb7d26..71f4fb253b 100644 --- a/apps/backend/internal/office/runtime/handler_test.go +++ b/apps/backend/internal/office/runtime/handler_test.go @@ -30,6 +30,7 @@ type handlerHarness struct { projects *recordingProjectManager tasks *handlerTaskCreator runEvents *recordingRunEvents + decisions *recordingDecisionRecorder agentSvc *agents.AgentService repository *sqlite.Repository } @@ -808,6 +809,7 @@ func newRuntimeHandlerHarnessWithProjectManager( projectManager = projectManagerFactory(repo) } runEvents := &recordingRunEvents{} + decisions := &recordingDecisionRecorder{} router := gin.New() RegisterRoutes(router.Group(""), NewHandler( agentSvc, @@ -821,6 +823,8 @@ func newRuntimeHandlerHarnessWithProjectManager( }), nil, runEvents, + decisions, + logger.Default(), )) return &handlerHarness{ router: router, @@ -830,6 +834,7 @@ func newRuntimeHandlerHarnessWithProjectManager( projects: projects, tasks: tasks, runEvents: runEvents, + decisions: decisions, agentSvc: agentSvc, repository: repo, } diff --git a/apps/backend/internal/office/service/decision_seat.go b/apps/backend/internal/office/service/decision_seat.go index 065414e102..6d91ad7d78 100644 --- a/apps/backend/internal/office/service/decision_seat.go +++ b/apps/backend/internal/office/service/decision_seat.go @@ -27,7 +27,7 @@ type decisionSeatDispatcher interface { // HoldsDecisionSeat reports whether agentProfileID currently holds a // decision seat (reviewer or approver) at the task's current workflow step. // It resolves the seat the same way RecordAgentDecision authorizes a real -// record_step_decision_kandev call, so runtime.ContextBuilder can grant the +// runtime decision call, so runtime.ContextBuilder can grant the // matching capability without over- or under-stating who can actually // decide. func (s *Service) HoldsDecisionSeat(ctx context.Context, taskID, agentProfileID string) (bool, error) { diff --git a/apps/backend/internal/office/service/event_subscribers.go b/apps/backend/internal/office/service/event_subscribers.go index 38e2c49e3d..8bbe92a8c5 100644 --- a/apps/backend/internal/office/service/event_subscribers.go +++ b/apps/backend/internal/office/service/event_subscribers.go @@ -448,7 +448,7 @@ func (s *Service) handleAgentCompleted(ctx context.Context, event *bus.Event) er } // warnIfReviewDecisionMissing flags a review or approval run that finished -// without the agent ever calling record_step_decision_kandev. A reviewer can +// without the agent ever recording a workflow decision. A reviewer can // post a full critique and reject the work in a comment, but if that comment // never becomes a recorded decision the workflow engine has nothing to act // on and the task strands in its current step forever. This does not fix diff --git a/apps/backend/internal/office/service/prompt_builder.go b/apps/backend/internal/office/service/prompt_builder.go index 4ae3d99cf9..f7df0a79bb 100644 --- a/apps/backend/internal/office/service/prompt_builder.go +++ b/apps/backend/internal/office/service/prompt_builder.go @@ -290,7 +290,9 @@ func buildReviewStagePrompt(pc *PromptContext) string { } b.WriteString("\nReview the implementation carefully. Check for correctness, edge cases, and code quality.\n") b.WriteString("Submit your verdict: approve if the work is satisfactory, or reject with specific feedback on what needs to change.") - writeDecisionContract(&b) + if hasDecisionAction(pc.AllowedActions) { + writeDecisionContract(&b) + } return b.String() } @@ -305,19 +307,19 @@ func buildApprovalStagePrompt(pc *PromptContext) string { } b.WriteString("\nConfirm that the approval requirements are met for this workflow.\n") b.WriteString("Submit your verdict: approve if the requirements are met, or reject with specific feedback on what needs to change.") - writeDecisionContract(&b) + if hasDecisionAction(pc.AllowedActions) { + writeDecisionContract(&b) + } return b.String() } -// writeDecisionContract appends the explicit record_step_decision_kandev contract shared by the -// review and approval stage prompts: a verdict must be recorded via the tool call, which the agent -// must treat as its final action for the turn, since posting a comment alone is not a decision and -// leaves the task stranded in review. The tool call itself does not halt the agent mid-turn (it -// returns an ordinary result), so the prompt must not claim otherwise — it instructs the agent to -// stop on its own after calling it. +// writeDecisionContract appends the task-bound CLI contract shared by the +// review and approval stage prompts. The command must be the final action for +// the turn because a comment alone does not advance the workflow. func writeDecisionContract(b *strings.Builder) { - b.WriteString("\n\nYou must call the record_step_decision_kandev tool with decision (\"approved\" or \"rejected\") and reason to record your verdict. Make this your final tool call for the turn, then stop.") - b.WriteString(" Posting a comment alone is not a decision and will not advance the task.") + b.WriteString("\n\nUse this command as your final action for the turn:\n") + b.WriteString(`$KANDEV_CLI kandev task decision --decision approved --reason "..."`) + b.WriteString("\nUse `approved` as shown, or replace it with `rejected` when rejecting. Both require a non-empty reason, then stop. Posting a comment or using an approval-inbox command alone does not count as a workflow step decision.") } func buildShipStagePrompt(pc *PromptContext) string { diff --git a/apps/backend/internal/office/service/prompt_builder_test.go b/apps/backend/internal/office/service/prompt_builder_test.go index 14e739c098..6536812d61 100644 --- a/apps/backend/internal/office/service/prompt_builder_test.go +++ b/apps/backend/internal/office/service/prompt_builder_test.go @@ -87,14 +87,10 @@ func TestBuildPrompt_LegacyRunReasonsRemainCompatible(t *testing.T) { {name: "review started", reason: "review_started", contains: []string{ "You are reviewing", "Legacy workflow task", - "record_step_decision_kandev", - "Posting a comment alone is not a decision", }}, {name: "approval started", reason: "approval_started", contains: []string{ "You are approving", "Legacy workflow task", - "record_step_decision_kandev", - "Posting a comment alone is not a decision", }}, } @@ -120,13 +116,14 @@ func TestBuildPrompt_TaskReviewRequestedUsesStageType(t *testing.T) { StageType: "approval", TaskIdentifier: "KAN-review", TaskTitle: "Approve release", + AllowedActions: []string{officeruntime.AvailableActionRecordStepDecision}, }) if !strings.HasPrefix(prompt, "You are approving") { t.Errorf("task_review_requested approver prompt = %q, want approver framing", prompt) } - if !strings.Contains(prompt, "record_step_decision_kandev") { - t.Errorf("task_review_requested prompt missing decision tool contract: %q", prompt) + if !strings.Contains(prompt, `$KANDEV_CLI kandev task decision --decision approved --reason "..."`) { + t.Errorf("task_review_requested prompt missing decision CLI contract: %q", prompt) } } @@ -433,8 +430,8 @@ func TestBuildPrompt_ReviewStageAllowedActionsIncludeRecordStepDecision(t *testi } prompt := service.BuildPrompt(pc) - if !strings.Contains(prompt, "You must call the record_step_decision_kandev tool") { - t.Fatalf("expected the decision contract sentence:\n%s", prompt) + if !strings.Contains(prompt, `$KANDEV_CLI kandev task decision --decision approved --reason "..."`) { + t.Fatalf("expected the decision CLI contract:\n%s", prompt) } allowedIdx := strings.Index(prompt, "- Allowed actions:") if allowedIdx == -1 { @@ -446,6 +443,54 @@ func TestBuildPrompt_ReviewStageAllowedActionsIncludeRecordStepDecision(t *testi } } +func TestBuildPrompt_DecisionContractUsesCLIAsFinalAction(t *testing.T) { + for _, stageType := range []string{"review", "approval"} { + t.Run(stageType, func(t *testing.T) { + prompt := service.BuildPrompt(&service.PromptContext{ + Reason: service.RunReasonTaskAssigned, + TaskIdentifier: "KAN-decision", + TaskTitle: "Decision task", + StageType: stageType, + AllowedActions: []string{officeruntime.AvailableActionRecordStepDecision}, + }) + + if !strings.Contains(prompt, `$KANDEV_CLI kandev task decision --decision approved --reason "..."`) { + t.Fatalf("%s prompt missing CLI decision contract:\n%s", stageType, prompt) + } + if !strings.Contains(prompt, "final action") { + t.Fatalf("%s prompt must make the CLI decision the final action:\n%s", stageType, prompt) + } + if strings.Contains(prompt, "record_step_decision_kandev") { + t.Fatalf("%s prompt still names the retired MCP decision tool:\n%s", stageType, prompt) + } + }) + } +} + +func TestBuildPrompt_WithoutDecisionActionOmitsOfficeDecisionContract(t *testing.T) { + for _, stageType := range []string{"review", "approval"} { + t.Run(stageType, func(t *testing.T) { + prompt := service.BuildPrompt(&service.PromptContext{ + Reason: service.RunReasonTaskAssigned, + TaskIdentifier: "KAN-kanban", + TaskTitle: "Kanban task", + StageType: stageType, + AllowedActions: []string{officeruntime.CapabilityPostComment}, + }) + + for _, fragment := range []string{ + "$KANDEV_CLI kandev task decision", + "record_step_decision", + "decision_id", + } { + if strings.Contains(prompt, fragment) { + t.Fatalf("%s prompt must not contain Office decision metadata %q:\n%s", stageType, fragment, prompt) + } + } + }) + } +} + func TestBuildPrompt_ApprovalStageUsesNeutralLifecycleLanguage(t *testing.T) { prompt := service.BuildPrompt(&service.PromptContext{ Reason: service.RunReasonTaskAssigned, diff --git a/apps/backend/internal/office/service/scheduler_integration.go b/apps/backend/internal/office/service/scheduler_integration.go index 347db6fa59..5e78eb707a 100644 --- a/apps/backend/internal/office/service/scheduler_integration.go +++ b/apps/backend/internal/office/service/scheduler_integration.go @@ -278,15 +278,6 @@ func (si *SchedulerIntegration) prepareAndLaunch( ctx context.Context, run *models.Run, agent *models.AgentInstance, taskID string, execCfg *ExecutorConfig, ) { - // ADR 0005 Wave E: skill + instruction file delivery moved into the - // runtime (internal/agent/runtime/lifecycle/skill). We still build - // the manifest here to extract AGENTS.md content for the prompt and - // to compute the deterministic instructionsDir path the runtime - // will write to. No filesystem side effects from this call. - manifest := si.buildSkillManifest(ctx, agent, defaultWorkspaceName) - instructionsDir, agentsMD := si.resolveInstructionsForPrompt(manifest, execCfg.Type) - si.snapshotRunSkills(ctx, run.ID, manifest, instructionsDir) - runCtx, err := (&officeruntime.ContextBuilder{ Agents: si.svc, Runs: si.svc.repo, @@ -307,6 +298,14 @@ func (si *SchedulerIntegration) prepareAndLaunch( "workspace_id": runCtx.WorkspaceID, "wake_reason": runCtx.Reason, }) + // ADR 0005 Wave E: skill + instruction file delivery moved into the + // runtime (internal/agent/runtime/lifecycle/skill). We still build + // the manifest here to extract AGENTS.md content for the prompt and + // to compute the deterministic instructionsDir path the runtime + // will write to. No filesystem side effects from this call. + manifest := si.buildSkillManifest(ctx, agent, defaultWorkspaceName, runCtx.AvailableActions...) + instructionsDir, agentsMD := si.resolveInstructionsForPrompt(manifest, execCfg.Type) + si.snapshotRunSkills(ctx, run.ID, manifest, instructionsDir) token, err := si.mintRuntimeToken(run, agent, runCtx) if err != nil { @@ -333,9 +332,10 @@ func (si *SchedulerIntegration) prepareAndLaunch( zap.Int("env_count", len(env))) launchCtx := LaunchContext{ - Prompt: prompt, - Env: env, - ProfileID: profileID, + Prompt: prompt, + Env: env, + ProfileID: profileID, + AdditionalSkillSlugs: decisionSkillSlugs(runCtx.AvailableActions), } // launchAgent returns true only when the adapter was actually invoked. // When it was, leave the run `claimed` and let the AgentCompleted/ @@ -591,7 +591,9 @@ func (si *SchedulerIntegration) launchAgent( return launched } var err error - if starter, ok := si.svc.taskStarter.(TaskStarterWithEnv); ok { + if starter, ok := si.svc.taskStarter.(TaskStarterWithLaunchContext); ok { + err = starter.StartTaskWithLaunchContext(ctx, taskID, launch.ProfileID, launch) + } else if starter, ok := si.svc.taskStarter.(TaskStarterWithEnv); ok { err = starter.StartTaskWithEnv(ctx, taskID, launch.ProfileID, "", "", "", launch.Prompt, "", false, nil, launch.Env) } else { diff --git a/apps/backend/internal/office/service/scheduler_integration_routing_test.go b/apps/backend/internal/office/service/scheduler_integration_routing_test.go index b95424779e..cbdd70ad6e 100644 --- a/apps/backend/internal/office/service/scheduler_integration_routing_test.go +++ b/apps/backend/internal/office/service/scheduler_integration_routing_test.go @@ -11,6 +11,7 @@ import ( "github.com/kandev/kandev/internal/agentctl/types/streams" "github.com/kandev/kandev/internal/office/models" "github.com/kandev/kandev/internal/office/service" + "github.com/kandev/kandev/internal/workflow/engine" ) // captureDispatcher implements service.RoutingDispatcher and records the @@ -136,13 +137,24 @@ func TestSchedulerIntegration_RoutingReceivesBuiltPromptAndEnv(t *testing.T) { } } -func TestSchedulerIntegration_SeatActionFlowsToPromptOnly(t *testing.T) { +func TestSchedulerIntegration_SeatActionFlowsToPromptAndLaunch(t *testing.T) { mock := &mockTaskStarter{} svc := newTestService(t, service.ServiceOptions{TaskStarter: mock}) dispatcher := &captureDispatcher{} svc.SetRoutingDispatcher(dispatcher) svc.SetWorkflowEngineDispatcher(&seatSpyDispatcher{}) ctx := context.Background() + if err := svc.CreateSkill(ctx, &models.Skill{ + ID: "decision-skill", + WorkspaceID: "ws-1", + Name: "Step decision", + Slug: "kandev-step-decision", + Content: "decision skill", + Version: "0.42.0", + ContentHash: "decision-skill-hash", + }); err != nil { + t.Fatalf("create decision skill: %v", err) + } agent := &models.AgentInstance{ ID: "decision-agent-1", @@ -174,6 +186,10 @@ func TestSchedulerIntegration_SeatActionFlowsToPromptOnly(t *testing.T) { if allowedIdx == -1 || !containsIgnoreCase(prompt[allowedIdx:], "record_step_decision") { t.Fatalf("prompt must advertise the seat-derived action: %s", dispatcher.lastCall().Prompt) } + launch := dispatcher.lastCall() + if len(launch.AdditionalSkillSlugs) != 1 || launch.AdditionalSkillSlugs[0] != "kandev-step-decision" { + t.Fatalf("launch skill additions = %v, want decision skill", launch.AdditionalSkillSlugs) + } runs, err := svc.ListRuns(ctx, "ws-1") if err != nil { @@ -203,6 +219,78 @@ func TestSchedulerIntegration_SeatActionFlowsToPromptOnly(t *testing.T) { if actions, ok := snapshot["available_actions"].([]any); !ok || len(actions) != 1 || actions[0] != "record_step_decision" { t.Fatalf("persisted snapshot missing advisory action: %#v", snapshot["available_actions"]) } + + snapshots, err := svc.ListRunSkillSnapshotsForTest(ctx, run.ID) + if err != nil { + t.Fatalf("list run skill snapshots: %v", err) + } + if len(snapshots) != 1 || snapshots[0].SkillID != "decision-skill" { + t.Fatalf("decision seat run snapshots = %#v, want decision skill", snapshots) + } +} + +func TestSchedulerIntegration_NonSeatRunDoesNotReceiveDecisionSkill(t *testing.T) { + mock := &mockTaskStarter{} + svc := newTestService(t, service.ServiceOptions{TaskStarter: mock}) + dispatcher := &captureDispatcher{} + svc.SetRoutingDispatcher(dispatcher) + svc.SetWorkflowEngineDispatcher(&seatSpyDispatcher{err: engine.ErrParticipantNotFound}) + ctx := context.Background() + + if err := svc.CreateSkill(ctx, &models.Skill{ + ID: "decision-skill", + WorkspaceID: "ws-1", + Name: "Step decision", + Slug: "kandev-step-decision", + Content: "decision skill", + Version: "0.42.0", + ContentHash: "decision-skill-hash", + }); err != nil { + t.Fatalf("create decision skill: %v", err) + } + agent := &models.AgentInstance{ + ID: "non-seat-decision-agent", + WorkspaceID: "ws-1", + Name: "non-seat-reviewer", + Role: models.AgentRoleWorker, + Status: models.AgentStatusIdle, + ExecutorPreference: `{"type":"worktree"}`, + } + if err := svc.CreateAgentInstance(ctx, agent); err != nil { + t.Fatalf("create agent: %v", err) + } + svc.ExecSQL(t, `INSERT INTO workflow_steps (id, stage_type) VALUES (?, ?)`, "step-non-seat", "review") + svc.ExecSQL(t, `INSERT INTO tasks (id, workspace_id, workflow_step_id, title, description, created_at, updated_at) + VALUES (?, ?, ?, ?, ?, CURRENT_TIMESTAMP, CURRENT_TIMESTAMP)`, + "task-non-seat", "ws-1", "step-non-seat", "Non-seat task", "Review the change") + if err := svc.QueueRun(ctx, agent.ID, service.RunReasonTaskAssigned, + `{"task_id":"task-non-seat"}`, ""); err != nil { + t.Fatalf("queue: %v", err) + } + + service.RunSchedulerTick(svc, ctx) + + runs, err := svc.ListRuns(ctx, "ws-1") + if err != nil { + t.Fatalf("list runs: %v", err) + } + var run *models.Run + for _, candidate := range runs { + if candidate.AgentProfileID == agent.ID && candidate.Reason == service.RunReasonTaskAssigned { + run = candidate + break + } + } + if run == nil { + t.Fatal("missing non-seat decision run") + } + snapshots, err := svc.ListRunSkillSnapshotsForTest(ctx, run.ID) + if err != nil { + t.Fatalf("list run skill snapshots: %v", err) + } + if len(snapshots) != 0 { + t.Fatalf("non-seat run snapshots = %#v, want no decision skill", snapshots) + } } // TestSchedulerIntegration_RoutingFallThrough_FallsBackToLegacy asserts diff --git a/apps/backend/internal/office/service/service.go b/apps/backend/internal/office/service/service.go index ad62afffbe..bd49809fb0 100644 --- a/apps/backend/internal/office/service/service.go +++ b/apps/backend/internal/office/service/service.go @@ -45,6 +45,12 @@ type TaskStarterWithEnv interface { planMode bool, attachments []v1.MessageAttachment, env map[string]string) error } +// TaskStarterWithLaunchContext optionally carries the complete Office launch +// context into the agent runtime, including per-run skill additions. +type TaskStarterWithLaunchContext interface { + StartTaskWithLaunchContext(ctx context.Context, taskID string, agentProfileID string, launch LaunchContext) error +} + // LaunchContext mirrors scheduler.LaunchContext so the office.service // package can carry the Office-built launch context (prompt, env, // workflow step, attachments, plan-mode, profile) into the routing @@ -53,15 +59,16 @@ type TaskStarterWithEnv interface { // The scheduler.RoutingDispatcher implementation translates this to // the scheduler-side LaunchContext when calling StartTaskWithRoute. type LaunchContext struct { - ExecutorID string - ExecutorProfileID string - Priority string - Prompt string - WorkflowStepID string - PlanMode bool - Attachments []v1.MessageAttachment - Env map[string]string - ProfileID string + ExecutorID string + ExecutorProfileID string + Priority string + Prompt string + WorkflowStepID string + PlanMode bool + Attachments []v1.MessageAttachment + Env map[string]string + ProfileID string + AdditionalSkillSlugs []string } // RoutingDispatcher is the seam the office scheduler integration uses to @@ -169,6 +176,46 @@ func (f TaskStarterWithEnvFunc) StartTaskWithEnv(ctx context.Context, taskID, ag priority, prompt, workflowStepID, planMode, attachments, env) } +// TaskStarterWithLaunchContextFunc adapts a complete launch-context function +// to the TaskStarter interfaces used by the Office scheduler. +type TaskStarterWithLaunchContextFunc func(ctx context.Context, taskID, agentProfileID string, launch LaunchContext) error + +// StartTask implements TaskStarter. +func (f TaskStarterWithLaunchContextFunc) StartTask(ctx context.Context, taskID, agentProfileID, executorID, + executorProfileID string, priority string, prompt, workflowStepID string, + planMode bool, attachments []v1.MessageAttachment) error { + return f(ctx, taskID, agentProfileID, LaunchContext{ + ExecutorID: executorID, + ExecutorProfileID: executorProfileID, + Priority: priority, + Prompt: prompt, + WorkflowStepID: workflowStepID, + PlanMode: planMode, + Attachments: attachments, + }) +} + +// StartTaskWithEnv implements TaskStarterWithEnv. +func (f TaskStarterWithLaunchContextFunc) StartTaskWithEnv(ctx context.Context, taskID, agentProfileID, executorID, + executorProfileID string, priority string, prompt, workflowStepID string, + planMode bool, attachments []v1.MessageAttachment, env map[string]string) error { + return f(ctx, taskID, agentProfileID, LaunchContext{ + ExecutorID: executorID, + ExecutorProfileID: executorProfileID, + Priority: priority, + Prompt: prompt, + WorkflowStepID: workflowStepID, + PlanMode: planMode, + Attachments: attachments, + Env: env, + }) +} + +// StartTaskWithLaunchContext implements TaskStarterWithLaunchContext. +func (f TaskStarterWithLaunchContextFunc) StartTaskWithLaunchContext(ctx context.Context, taskID, agentProfileID string, launch LaunchContext) error { + return f(ctx, taskID, agentProfileID, launch) +} + // WorkspaceCreator creates a DB workspace row for kanban compatibility. // Implemented by the task service or its repository. type WorkspaceCreator interface { diff --git a/apps/backend/internal/office/service/skill_manifest.go b/apps/backend/internal/office/service/skill_manifest.go index 70832da4ff..4d877fa2e5 100644 --- a/apps/backend/internal/office/service/skill_manifest.go +++ b/apps/backend/internal/office/service/skill_manifest.go @@ -6,8 +6,11 @@ import ( "go.uber.org/zap" "github.com/kandev/kandev/internal/office/models" + officeruntime "github.com/kandev/kandev/internal/office/runtime" ) +const decisionSkillSlug = "kandev-step-decision" + // SkillManifest holds the resolved skills and instructions for an agent session. // It is a pure data structure built before executor selection, so the delivery // strategy can adapt to the target executor type. @@ -40,6 +43,7 @@ type ManifestInstruction struct { // returning a manifest that can be delivered to any executor type. func (si *SchedulerIntegration) buildSkillManifest( ctx context.Context, agent *models.AgentInstance, workspaceSlug string, + availableActions ...string, ) *SkillManifest { // Wave G: AgentInstance.ID == agent_profiles.id under the unified model. agentTypeID := si.svc.resolveAgentType(agent.ID) @@ -50,22 +54,18 @@ func (si *SchedulerIntegration) buildSkillManifest( ProjectSkillDir: si.svc.resolveProjectSkillDir(agentTypeID), } + decisionAvailable := hasDecisionAction(availableActions) + // Load desired skills. slugs := ParseDesiredSlugs(agent.DesiredSkills) for _, slug := range slugs { - skill, err := si.svc.GetSkillFromConfig(ctx, slug) - if err != nil { - si.logger.Debug("skip skill in manifest", - zap.String("slug", slug), zap.Error(err)) + if slug == decisionSkillSlug && !decisionAvailable { continue } - manifest.Skills = append(manifest.Skills, ManifestSkill{ - ID: skill.ID, - Slug: skill.Slug, - Content: skill.Content, - Version: skill.Version, - ContentHash: skill.ContentHash, - }) + si.appendConfiguredSkill(ctx, manifest, slug) + } + if decisionAvailable && !manifestHasSkill(manifest, decisionSkillSlug) { + si.appendConfiguredSkill(ctx, manifest, decisionSkillSlug) } // Load instruction files from DB. @@ -85,3 +85,46 @@ func (si *SchedulerIntegration) buildSkillManifest( return manifest } + +func (si *SchedulerIntegration) appendConfiguredSkill( + ctx context.Context, manifest *SkillManifest, slug string, +) { + skill, err := si.svc.GetSkillFromConfig(ctx, slug) + if err != nil { + si.logger.Debug("skip skill in manifest", + zap.String("slug", slug), zap.Error(err)) + return + } + manifest.Skills = append(manifest.Skills, ManifestSkill{ + ID: skill.ID, + Slug: skill.Slug, + Content: skill.Content, + Version: skill.Version, + ContentHash: skill.ContentHash, + }) +} + +func manifestHasSkill(manifest *SkillManifest, slug string) bool { + for _, skill := range manifest.Skills { + if skill.Slug == slug { + return true + } + } + return false +} + +func hasDecisionAction(actions []string) bool { + for _, action := range actions { + if action == officeruntime.AvailableActionRecordStepDecision { + return true + } + } + return false +} + +func decisionSkillSlugs(actions []string) []string { + if !hasDecisionAction(actions) { + return nil + } + return []string{decisionSkillSlug} +} diff --git a/apps/backend/internal/office/service/skill_manifest_decision_test.go b/apps/backend/internal/office/service/skill_manifest_decision_test.go new file mode 100644 index 0000000000..4795931744 --- /dev/null +++ b/apps/backend/internal/office/service/skill_manifest_decision_test.go @@ -0,0 +1,76 @@ +package service_test + +import ( + "context" + "testing" + + "github.com/kandev/kandev/internal/office/models" + officeruntime "github.com/kandev/kandev/internal/office/runtime" + "github.com/kandev/kandev/internal/office/service" +) + +func TestBuildSkillManifest_DecisionSkillSeatBranches(t *testing.T) { + tests := []struct { + name string + desiredSkills string + available []string + wantSkillCount int + }{ + { + name: "desired without decision seat", + desiredSkills: `["kandev-step-decision"]`, + wantSkillCount: 0, + }, + { + name: "decision seat auto injects skill", + available: []string{officeruntime.AvailableActionRecordStepDecision}, + wantSkillCount: 1, + }, + { + name: "decision seat keeps desired skill once", + desiredSkills: `["kandev-step-decision"]`, + available: []string{officeruntime.AvailableActionRecordStepDecision}, + wantSkillCount: 1, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + svc := newTestService(t) + ctx := context.Background() + if err := svc.CreateSkill(ctx, &models.Skill{ + ID: "decision-skill", + WorkspaceID: "ws-1", + Name: "Step decision", + Slug: "kandev-step-decision", + Content: "decision skill", + Version: "0.42.0", + ContentHash: "decision-skill-hash", + }); err != nil { + t.Fatalf("create skill: %v", err) + } + + manifest := service.BuildSkillManifestForTest( + service.NewSchedulerIntegration(svc, 0), + ctx, + &models.AgentInstance{ + ID: "agent-1", + WorkspaceID: "ws-1", + DesiredSkills: tt.desiredSkills, + }, + "ws-1", + tt.available..., + ) + + count := 0 + for _, skill := range manifest.Skills { + if skill.Slug == "kandev-step-decision" { + count++ + } + } + if count != tt.wantSkillCount { + t.Fatalf("decision skill count = %d, want %d; manifest = %+v", count, tt.wantSkillCount, manifest.Skills) + } + }) + } +} diff --git a/apps/backend/internal/office/service/test_helpers.go b/apps/backend/internal/office/service/test_helpers.go index ded774aaaf..c6d59d63d5 100644 --- a/apps/backend/internal/office/service/test_helpers.go +++ b/apps/backend/internal/office/service/test_helpers.go @@ -193,8 +193,9 @@ func BuildSkillManifestForTest( ctx context.Context, agent *models.AgentInstance, workspaceSlug string, + availableActions ...string, ) *SkillManifest { - return si.buildSkillManifest(ctx, agent, workspaceSlug) + return si.buildSkillManifest(ctx, agent, workspaceSlug, availableActions...) } // Skill delivery test helpers were removed in ADR 0005 Wave E along diff --git a/apps/backend/internal/office/skills/system_sync_test.go b/apps/backend/internal/office/skills/system_sync_test.go index b54442452a..1214dd5aa0 100644 --- a/apps/backend/internal/office/skills/system_sync_test.go +++ b/apps/backend/internal/office/skills/system_sync_test.go @@ -345,6 +345,44 @@ func TestBundledSkillCLIExamplesAvoidKnownUnsupportedOperations(t *testing.T) { } } +func TestBundledDecisionSkillContract(t *testing.T) { + specs, err := skills.LoadBundledSystemSkills() + if err != nil { + t.Fatalf("LoadBundledSystemSkills: %v", err) + } + + for _, spec := range specs { + if spec.Slug != "kandev-step-decision" { + continue + } + if len(spec.DefaultForRoles) != 0 { + t.Fatalf("decision skill default roles = %v, want none", spec.DefaultForRoles) + } + for _, fragment := range []string{ + `$KANDEV_CLI kandev task decision --decision approved --reason "..."`, + "approved", + "rejected", + "non-empty reason", + "supersedes", + "decision", + "role", + "step_id", + "decision_id", + "decided_at", + "transition_applied", + "guards", + "comments do not count", + "Approval-inbox commands do not count", + } { + if !strings.Contains(spec.Content, fragment) { + t.Errorf("decision skill missing %q:\n%s", fragment, spec.Content) + } + } + return + } + t.Fatal("kandev-step-decision bundled system skill not found") +} + func TestBundledProtocolSkillDocumentsScopedTaskMessages(t *testing.T) { specs, err := skills.LoadBundledSystemSkills() if err != nil { diff --git a/apps/backend/internal/orchestrator/executor/executor.go b/apps/backend/internal/orchestrator/executor/executor.go index a5ea5971ca..c0709f818c 100644 --- a/apps/backend/internal/orchestrator/executor/executor.go +++ b/apps/backend/internal/orchestrator/executor/executor.go @@ -431,6 +431,8 @@ type LaunchAgentRequest struct { Priority string Metadata map[string]interface{} Env map[string]string + // AdditionalSkillSlugs are launch-scoped skills selected by Office. + AdditionalSkillSlugs []string // ApprovedSecretEnvKeys contains repository binding keys that SSH may // forward in addition to its managed credential allowlist. Values are // still taken only from Env; the key list is the explicit repository grant. @@ -594,6 +596,9 @@ type LaunchOptions struct { McpProfile *mcpprofile.Context Attachments []v1.MessageAttachment Env map[string]string + // AdditionalSkillSlugs are materialized for this launch in addition to the + // durable profile selection. + AdditionalSkillSlugs []string // RouteOverride carries a provider-routing override resolved by the // office scheduler. When nil, launch behavior is identical to today. RouteOverride *RouteOverride @@ -617,14 +622,15 @@ type RouteOverride struct { // preserve the Office-built prompt and configuration that the legacy // path receives via StartTaskWithEnv. type LaunchContext struct { - ExecutorID string - ExecutorProfileID string - Priority string - Prompt string - WorkflowStepID string - PlanMode bool - Attachments []v1.MessageAttachment - Env map[string]string + ExecutorID string + ExecutorProfileID string + Priority string + Prompt string + WorkflowStepID string + PlanMode bool + Attachments []v1.MessageAttachment + Env map[string]string + AdditionalSkillSlugs []string } // LaunchAgentResponse contains the result of launching an agent diff --git a/apps/backend/internal/orchestrator/executor/executor_environment_test.go b/apps/backend/internal/orchestrator/executor/executor_environment_test.go index 9f89b1d38b..d36926e251 100644 --- a/apps/backend/internal/orchestrator/executor/executor_environment_test.go +++ b/apps/backend/internal/orchestrator/executor/executor_environment_test.go @@ -1068,6 +1068,8 @@ func TestApplyExecutorRunningMetadata_SkipsSessionScopedKeys(t *testing.T) { lifecycle.MetadataKeySSHRemoteAgentctlPID, lifecycle.MetadataKeySSHLocalForwardPort, lifecycle.MetadataKeySSHRemoteAgentctlURL, + lifecycle.MetadataKeySSHRuntimeAPILocalURL, + lifecycle.MetadataKeySSHRuntimeAPIRemotePort, lifecycle.MetadataKeyOfficeAgentProfileID, } for _, k := range sessionScoped { diff --git a/apps/backend/internal/orchestrator/executor/executor_execute.go b/apps/backend/internal/orchestrator/executor/executor_execute.go index a5e7f85f4d..9e844600b2 100644 --- a/apps/backend/internal/orchestrator/executor/executor_execute.go +++ b/apps/backend/internal/orchestrator/executor/executor_execute.go @@ -1305,6 +1305,7 @@ func (e *Executor) LaunchPreparedSession(ctx context.Context, task *v1.Task, ses } req.StartAgent = startAgent mergeEnv(req, opts.Env) + req.AdditionalSkillSlugs = append([]string(nil), opts.AdditionalSkillSlugs...) if opts.RouteOverride != nil { req.RouteOverride = opts.RouteOverride if opts.RouteOverride.ExecutionProfileID == "" { diff --git a/apps/backend/internal/orchestrator/task_operations.go b/apps/backend/internal/orchestrator/task_operations.go index 683eda91eb..7316550410 100644 --- a/apps/backend/internal/orchestrator/task_operations.go +++ b/apps/backend/internal/orchestrator/task_operations.go @@ -1029,6 +1029,15 @@ func (s *Service) StartTaskWithEnv(ctx context.Context, taskID string, agentProf return s.startTask(ctx, taskID, agentProfileID, executorID, executorProfileID, priority, prompt, workflowStepID, planMode, autoStart, attachments, startTaskOptions{Env: env}) } +// StartTaskWithEnvAndSkills starts a task with launch-scoped environment and +// skills selected by the Office run rather than by the durable profile. +func (s *Service) StartTaskWithEnvAndSkills(ctx context.Context, taskID string, agentProfileID string, executorID string, executorProfileID string, priority string, prompt string, workflowStepID string, planMode, autoStart bool, attachments []v1.MessageAttachment, env map[string]string, additionalSkillSlugs []string) (*executor.TaskExecution, error) { + return s.startTask(ctx, taskID, agentProfileID, executorID, executorProfileID, priority, prompt, workflowStepID, planMode, autoStart, attachments, startTaskOptions{ + Env: env, + AdditionalSkillSlugs: append([]string(nil), additionalSkillSlugs...), + }) +} + // startTaskOptions carries the optional, server-only launch inputs that only // some callers supply. Keeping them in one struct avoids growing startTask's // already long positional parameter list for every new orthogonal concern. @@ -1039,6 +1048,9 @@ type startTaskOptions struct { ProfileExplicit bool // Env holds launch-scoped environment variables for the agent runtime. Env map[string]string + // AdditionalSkillSlugs are materialized for this launch in addition to the + // profile's durable skill selection. + AdditionalSkillSlugs []string // Route pins a concrete execution profile chosen by Office provider routing. Route *executor.RouteOverride // SpawnOrigin is set when another agent session spawned this launch; it @@ -1064,7 +1076,11 @@ func (s *Service) StartTaskWithRoute( _, err := s.startTask(ctx, taskID, agentProfileID, launch.ExecutorID, launch.ExecutorProfileID, launch.Priority, launch.Prompt, launch.WorkflowStepID, launch.PlanMode, false, - launch.Attachments, startTaskOptions{Env: launch.Env, Route: &route}) + launch.Attachments, startTaskOptions{ + Env: launch.Env, + AdditionalSkillSlugs: append([]string(nil), launch.AdditionalSkillSlugs...), + Route: &route, + }) return err } @@ -1354,6 +1370,7 @@ func (s *Service) startTask(ctx context.Context, taskID string, agentProfileID s McpMode: mcpMode, Attachments: attachments, Env: env, + AdditionalSkillSlugs: append([]string(nil), opts.AdditionalSkillSlugs...), RouteOverride: route, }) if err != nil { diff --git a/apps/backend/pkg/websocket/actions.go b/apps/backend/pkg/websocket/actions.go index bd10d227a0..e2f6ab6042 100644 --- a/apps/backend/pkg/websocket/actions.go +++ b/apps/backend/pkg/websocket/actions.go @@ -472,9 +472,6 @@ const ( ActionMCPGetCanvasState = "mcp.get_canvas_state" ActionMCPSetCanvasState = "mcp.set_canvas_state" - // Office quorum decision recording. - ActionMCPRecordStepDecision = "mcp.record_step_decision" - // Config-mode MCP actions (agent-native configuration) ActionMCPCreateWorkflow = "mcp.create_workflow" ActionMCPUpdateWorkflow = "mcp.update_workflow" diff --git a/docs/decisions/2026-09-07-separate-runtime-permissions-and-advisory-actions.md b/docs/decisions/2026-09-07-separate-runtime-permissions-and-advisory-actions.md index 26109791bb..a474004c62 100644 --- a/docs/decisions/2026-09-07-separate-runtime-permissions-and-advisory-actions.md +++ b/docs/decisions/2026-09-07-separate-runtime-permissions-and-advisory-actions.md @@ -1,4 +1,4 @@ -# ADR-2026-09-07-separate-runtime-permissions-and-advisory-actions: Keep MCP availability separate from runtime permissions +# ADR-2026-09-07-separate-runtime-permissions-and-advisory-actions: Keep live advisory actions separate from runtime permissions **Status:** accepted **Date:** 2026-09-07 @@ -9,22 +9,33 @@ Office runtime capabilities are enforced permissions for the runtime HTTP action surface. They are persisted on a run and copied into the agent JWT. -The `record_step_decision_kandev` MCP tool is different. It is always exposed -to the bound session, and the handler performs live workflow-seat -authorization when the agent calls it. A run-launch seat lookup can therefore -describe prompt availability, but it cannot grant or revoke the MCP action. +Workflow-step decision recording is different. It is a task-bound Office CLI +command backed by the runtime HTTP surface, and the handler performs live +workflow-seat authorization when the agent calls it. A run-launch seat lookup +can therefore select prompt and skill guidance, but it cannot grant or revoke +the decision action. -Putting the MCP action in `runtime.Capabilities` makes `Allows` and JWT data -look like authorization even though the MCP handler does not enforce them. +Putting the decision action in `runtime.Capabilities` makes `Allows` and JWT +data look like authorization even though the runtime handler must resolve the +current reviewer or approver seat on every call. ## Decision -Keep `runtime.Capabilities` limited to permissions enforced by the runtime HTTP -handler. Store seat-derived MCP affordances in `RunContext.AvailableActions`. +Keep `runtime.Capabilities` limited to static permissions enforced directly by +the runtime HTTP handlers. Store the seat-derived decision affordance in +`RunContext.AvailableActions`. The scheduler may merge `AvailableActions` with runtime capability keys when it -renders the prompt. It must not serialize those actions into runtime JWT -capability claims. The MCP handler remains the live authorization boundary. +renders the prompt and may use it to inject a narrow Kandev skill. It must not +serialize those actions into runtime JWT capability claims. The task, session, +and agent come only from the signed run context; the runtime handler resolves +the current workflow step and participant seat live before it delegates to +`DashboardService.RecordAgentDecision`. + +The Office MCP registry does not expose `record_step_decision_kandev`. Decision +recording uses `$KANDEV_CLI kandev task decision`, which calls the narrow Office +runtime endpoint. The CLI and endpoint accept only `decision` and `reason`; they +do not accept task, step, role, participant, session, or agent identifiers. If the seat lookup fails, context construction fails and the scheduler uses its existing retry path. A missing resolver, missing step, or absent seat produces @@ -32,23 +43,30 @@ no advertised action. ## Consequences -Prompts can advertise the decision tool without changing the meaning of the -runtime permission model. Run input snapshots retain the launch-time advisory -state for inspection, while JWT claims remain limited to enforced permissions. +Prompts and injected skills can advertise the decision command without changing +the meaning of the runtime permission model. Run input snapshots retain the +launch-time advisory state for inspection, while JWT claims remain limited to +static enforced permissions. Seat changes after launch can make the prompt stale. This is safe because the -decision handler resolves the current step and participant seat again. +decision handler resolves the current step and participant seat again. Moving +the action out of MCP also removes its tool schema from every Office turn. Transient seat lookup failures delay a run through the normal retry policy instead of launching a prompt that requires an unavailable action. ## Alternatives Considered +- Keep `record_step_decision_kandev` in the Office MCP registry: rejected + because every Office turn pays for a decision schema even when the caller + does not occupy a decision seat, while the established Office CLI already + provides a task-bound authenticated transport. - Keep `record_step_decision` in `Capabilities`: rejected because a false flag - does not block the MCP call and a true flag does not guarantee live authority. -- Enforce the MCP action with the runtime JWT capability: rejected because it - would duplicate workflow-seat authorization and couple the generic runtime - permission model to one tool transport. + would not represent the live participant slate and a true flag would not + guarantee current authority. +- Enforce the CLI action only with the runtime JWT capability: rejected because + it would duplicate workflow-seat authorization and turn a launch-time + snapshot into an authority source. - Continue on lookup errors and omit the action: rejected because review and approval prompts still require the decision call, which would create a contradictory launch state. diff --git a/docs/plans/office-decision-cli-transport/plan.md b/docs/plans/office-decision-cli-transport/plan.md new file mode 100644 index 0000000000..6f4a59675c --- /dev/null +++ b/docs/plans/office-decision-cli-transport/plan.md @@ -0,0 +1,104 @@ +--- +created: 2026-09-07 +status: in_progress +requirements: + - REQ-TASKS-QUORUM-RECORDING-001 +system_design: + - ../../specs/tasks/system-design/workflow-quorum-decision-recording.md +legacy_specs: [] +--- + +# Implementation Plan: Office Decision CLI Transport + +## Overview + +Move Office workflow-step decisions from the Office MCP catalog to the signed, +task-bound Office runtime CLI. Delivery is sequential: establish and verify the +CLI/API path, teach eligible seat holders through an injected skill and prompt, +then remove the old MCP transport and update its inventories. + +## Scope + +### In scope + +- Add `$KANDEV_CLI kandev task decision` and a closed Office runtime endpoint. +- Derive task, session, and agent identity from the signed run context. +- Delegate through `DashboardService.RecordAgentDecision` and the existing + workflow-engine decision path. +- Inject decision guidance for reviewer and approver seat holders. +- Remove `record_step_decision_kandev` from Office MCP registration, context, + websocket transport, and transport-only tests. +- Update internal and public Office MCP/CLI inventories. + +### Out of scope + +- Changes to quorum thresholds, seat canonicalization, persistence, reactivity, + or transition semantics. +- Changes to the separate Office approval-inbox command. +- A Kanban decision command or any Kanban MCP profile change. + +## Technical approach + +### Signed runtime API and CLI + +Add `task decision` to `apps/backend/cmd/agentctl/kandev_task.go`. It accepts +only `--decision` and `--reason` and posts to +`/api/v1/office/runtime/task/decision` using the existing runtime client. + +The runtime handler derives the task, session, and agent from the validated JWT. +Its request body is closed over `decision` and `reason`, so identity-shaped or +unknown fields fail instead of being ignored. A small adapter at the Office +composition boundary translates runtime DTOs to +`dashboard.RecordAgentDecisionInput` and maps the existing structured result +back to JSON. Validation, live role/seat resolution, persistence, events, +reactivity, and quorum advancement remain in `DashboardService` and the engine. + +### Seat-specific skill and prompt + +Add the bundled `kandev-step-decision` system skill without assigning it as a +static role default. When `RunContext.AvailableActions` contains the +launch-time decision affordance, the scheduler adds that skill to the run's +manifest. Review and approval prompts name the CLI command as the final action. +The endpoint still performs live authorization, so a stale launch-time seat +snapshot cannot grant access. + +### MCP retirement + +After the first two work orders pass, remove the Office decision tool group, +tool implementation, websocket action and handler wiring, and transport-only +tests. Keep the service and engine regression suites. Reduce the documented +Office tool count from 12 to 11 and remove the decision tool from first-turn and +public inventories. Add negative assertions for both Office MCP schema absence +and Kanban instruction/metadata isolation. + +## Tests + +| Acceptance criterion | Evidence | +| --- | --- | +| `AC-TASKS-QUORUM-RECORDING-001.1` | CLI request and seat-specific skill/prompt tests | +| `AC-TASKS-QUORUM-RECORDING-001.2`–`.5` | Runtime identity forwarding plus existing dashboard seat/persistence/quorum tests | +| `AC-TASKS-QUORUM-RECORDING-001.6`–`.8` | CLI/API validation and existing no-write dashboard tests | +| `AC-TASKS-QUORUM-RECORDING-001.9` | Kanban prompt/context isolation regression | +| `AC-TASKS-QUORUM-RECORDING-001.10`–`.11` | Runtime adapter and existing dashboard structured-result tests | +| `AC-TASKS-QUORUM-RECORDING-001.12` | Office MCP inventory and exact first-turn context tests | + +## Work orders + +- [done] [Task 01: Establish the task-bound decision transport](task-01-task-bound-decision-transport.md) +- [done] [Task 02: Inject the decision skill](task-02-inject-decision-skill.md) +- [done] [Task 03: Retire the decision MCP transport](task-03-retire-decision-mcp.md) + +## Verification results + +- Task 01: `cd apps/backend && rtk go test ./cmd/agentctl ./internal/office/runtime ./internal/office/dashboard -run 'Test(TaskDecision|RuntimeHandler_RecordAgentDecision|RecordAgentDecision)' -count=1` — passed (18 tests). +- Task 02: `cd apps/backend && rtk go test ./internal/office/configloader ./internal/office/skills ./internal/office/service -run 'Test.*(DecisionSkill|DecisionContract|ReviewStage|ApprovalStage|SkillManifest|SystemSkills)' -count=1` — passed (36 tests); full affected packages passed (564 tests). +- Task 03: `cd apps/backend && rtk go test ./internal/mcp/server ./internal/mcp/handlers ./internal/backendapp -run 'Test.*(ModeOffice|ModeTask|Sysprompt|Decision|HandlerRegistration)' -count=1` — passed (22 tests); public docs tests and validator passed (61 tests, 46 pages). + +## Risks + +- Importing `office/dashboard` from `office/runtime` would create a package + cycle; the composition adapter must keep DTO translation at `internal/office`. +- A permissive JSON binder could silently ignore a forged `task_id`; the new + endpoint needs a closed request decoder. +- Static role-based skill assignment cannot represent live participant seats; + injection must follow the seat-derived advisory action and retain live checks. diff --git a/docs/plans/office-decision-cli-transport/task-01-task-bound-decision-transport.md b/docs/plans/office-decision-cli-transport/task-01-task-bound-decision-transport.md new file mode 100644 index 0000000000..97924d9ca5 --- /dev/null +++ b/docs/plans/office-decision-cli-transport/task-01-task-bound-decision-transport.md @@ -0,0 +1,96 @@ +--- +id: "01-task-bound-decision-transport" +title: "Establish the task-bound decision transport" +status: done +wave: 1 +depends_on: [] +plan: "plan.md" +requirements: + - REQ-TASKS-QUORUM-RECORDING-001 +acceptance_criteria: + - AC-TASKS-QUORUM-RECORDING-001.2 + - AC-TASKS-QUORUM-RECORDING-001.3 + - AC-TASKS-QUORUM-RECORDING-001.4 + - AC-TASKS-QUORUM-RECORDING-001.5 + - AC-TASKS-QUORUM-RECORDING-001.6 + - AC-TASKS-QUORUM-RECORDING-001.7 + - AC-TASKS-QUORUM-RECORDING-001.8 + - AC-TASKS-QUORUM-RECORDING-001.10 + - AC-TASKS-QUORUM-RECORDING-001.11 +system_design: + - ../../specs/tasks/system-design/workflow-quorum-decision-recording.md +--- + +# Task 01: Establish the Task-Bound Decision Transport + +## Summary + +Add the singular `task decision` CLI command and its signed Office runtime API. +Keep the handler thin and route the trusted run identity into the existing +dashboard and workflow-engine decision path. + +## In scope + +- Add CLI flag validation and JSON request handling. +- Add the closed runtime endpoint with no caller-selectable identity fields. +- Add an acyclic Office composition adapter for `DashboardService`. +- Cover trusted identity forwarding, cross-task input rejection, + non-participant denial, structured results, and no-write validation failures. + +## Out of scope + +- Prompt, skill, or MCP registry changes. +- Any change to dashboard or engine quorum semantics. + +## Acceptance + +- The CLI accepts only `approved|rejected` plus a non-empty reason and posts no + task, step, role, participant, session, or agent identifier. +- The API derives all identity from the signed run and rejects forged identity + fields and callers without a live decision seat. +- Reviewer and approver decisions still persist under the canonical live seat, + return all seven fields, and can advance quorum through the existing service. + +## Verification + +```bash +cd apps/backend && rtk go test ./cmd/agentctl ./internal/office/runtime ./internal/office/dashboard -run 'Test(TaskDecision|RuntimeHandler_RecordAgentDecision|RecordAgentDecision)' -count=1 +``` + +## Files likely touched + +- `apps/backend/cmd/agentctl/kandev_task.go` +- `apps/backend/cmd/agentctl/kandev_test.go` +- `apps/backend/internal/office/runtime/handler.go` +- `apps/backend/internal/office/runtime/handler_test.go` +- `apps/backend/internal/office/runtime/errors.go` +- `apps/backend/internal/office/routes.go` +- `apps/backend/internal/office/decision_adapter.go` +- `apps/backend/internal/office/dashboard/agent_decisions_test.go` + +## Dependencies + +None. + +## Risks + +- The runtime/dashboard package cycle requires DTO translation at the Office + composition root. +- Unknown JSON fields must fail closed for cross-task regression coverage. + +## Parallelism + +`sequential` + +## Inputs + +- `REQ-TASKS-QUORUM-RECORDING-001` and the agent command contract. +- Existing runtime handler, CLI client, and dashboard decision tests. + +## Results + +- Added the signed `kandev task decision` CLI command with local verdict/reason validation. +- Added the closed `POST /api/v1/office/runtime/task/decision` endpoint. +- Added the Office composition adapter to `DashboardService.RecordAgentDecision`. +- Verified signed task/session/agent identity forwarding, forged identity rejection, structured results, validation and permission errors, and dashboard decision regressions. +- Verification passed: `cd apps/backend && rtk go test ./cmd/agentctl ./internal/office/runtime ./internal/office/dashboard -run 'Test(TaskDecision|RuntimeHandler_RecordAgentDecision|RecordAgentDecision)' -count=1` (18 tests). diff --git a/docs/plans/office-decision-cli-transport/task-02-inject-decision-skill.md b/docs/plans/office-decision-cli-transport/task-02-inject-decision-skill.md new file mode 100644 index 0000000000..fc97e6d049 --- /dev/null +++ b/docs/plans/office-decision-cli-transport/task-02-inject-decision-skill.md @@ -0,0 +1,101 @@ +--- +id: "02-inject-decision-skill" +title: "Inject the decision skill" +status: done +wave: 2 +depends_on: + - "01-task-bound-decision-transport" +plan: "plan.md" +requirements: + - REQ-TASKS-QUORUM-RECORDING-001 +acceptance_criteria: + - AC-TASKS-QUORUM-RECORDING-001.1 + - AC-TASKS-QUORUM-RECORDING-001.3 + - AC-TASKS-QUORUM-RECORDING-001.9 +system_design: + - ../../specs/tasks/system-design/workflow-quorum-decision-recording.md +--- + +# Task 02: Inject the Decision Skill + +## Summary + +Teach decision-seat holders the CLI contract through a bundled system skill. +Select that skill from the seat-derived advisory action and make the review and +approval prompts require the command as their final action. + +## In scope + +- Add `kandev-step-decision/SKILL.md` with no static role default. +- Append it to the run manifest only when the launch-time seat lookup grants + `record_step_decision` as an advisory action. +- Update review and approval prompts from the MCP tool to the CLI command. +- Prove normal Kanban sessions receive no decision CLI skill or instruction. + +## Out of scope + +- Treating launch-time `AvailableActions` or JWT claims as authorization. +- Removing the MCP transport before the replacement guidance is green. + +## Acceptance + +- A reviewer or approver seat run receives the decision skill and final-action + CLI prompt; a non-seat Office run does not receive the skill. +- The skill explains accepted verdicts, required reason, superseding repeats, + structured output, and that comments or approval-inbox commands do not count. +- Kanban context and prompts contain no Office decision CLI instruction or + decision metadata. + +## Verification + +```bash +cd apps/backend && rtk go test ./internal/office/configloader ./internal/office/skills ./internal/office/service -run 'Test.*(DecisionSkill|DecisionContract|ReviewStage|ApprovalStage|SkillManifest|SystemSkills)' -count=1 +``` + +## Files likely touched + +- `apps/backend/internal/office/configloader/skills/kandev-step-decision/SKILL.md` +- `apps/backend/internal/office/skills/system_sync_test.go` +- `apps/backend/internal/office/service/skill_manifest.go` +- `apps/backend/internal/office/service/scheduler_integration.go` +- `apps/backend/internal/office/service/prompt_builder.go` +- `apps/backend/internal/office/service/prompt_builder_test.go` +- `apps/backend/internal/office/service/scheduler_integration_routing_test.go` +- `apps/backend/internal/mcp/server/sysprompt_sync_test.go` + +## Dependencies + +- Task 01 provides the command that the skill teaches. + +## Risks + +- Static role defaults do not match workflow participant seats; selection must + use the existing live-seat advisory lookup at launch. +- Prompt wording must not imply that launch-time skill availability grants + authority after the participant slate changes. + +## Parallelism + +`sequential` + +## Inputs + +- The accepted advisory-actions ADR. +- Existing bundled skill synchronization and scheduler manifest pipeline. + +## Results + +Implemented the bundled `kandev-step-decision` skill without static role +defaults. The scheduler now builds the signed run context before resolving the +manifest, injects the skill only when the launch-time advisory decision action +is present, and filters the skill from non-seat runs. Review and approval +prompts use the task-bound CLI command as the final action and omit the Office +decision contract when the action is absent. + +Verification: + +```text +cd apps/backend && rtk go test ./internal/office/configloader ./internal/office/skills ./internal/office/service -run 'Test.*(DecisionSkill|DecisionContract|ReviewStage|ApprovalStage|SkillManifest|SystemSkills)' -count=1 +``` + +Passed: 36 tests. The full affected package run also passed 564 tests. diff --git a/docs/plans/office-decision-cli-transport/task-03-retire-decision-mcp.md b/docs/plans/office-decision-cli-transport/task-03-retire-decision-mcp.md new file mode 100644 index 0000000000..a89d08699d --- /dev/null +++ b/docs/plans/office-decision-cli-transport/task-03-retire-decision-mcp.md @@ -0,0 +1,107 @@ +--- +id: "03-retire-decision-mcp" +title: "Retire the decision MCP transport" +status: done +wave: 3 +depends_on: + - "02-inject-decision-skill" +plan: "plan.md" +requirements: + - REQ-TASKS-QUORUM-RECORDING-001 +acceptance_criteria: + - AC-TASKS-QUORUM-RECORDING-001.9 + - AC-TASKS-QUORUM-RECORDING-001.12 +system_design: + - ../../specs/tasks/system-design/workflow-quorum-decision-recording.md +--- + +# Task 03: Retire the Decision MCP Transport + +## Summary + +Remove the now-replaced Office MCP schema and websocket transport. Preserve the +normal Kanban catalog and update internal and public Office inventories to the +11-tool surface plus the task-bound CLI command. + +## In scope + +- Remove the Office decision tool group and implementation. +- Remove its websocket action, handler dependency/wiring, and transport-only + test files. +- Update exact Office context/tool-count tests and add negative schema checks. +- Update the public Office MCP/runtime CLI reference and coverage inventory. + +## Out of scope + +- Removing `DashboardService.RecordAgentDecision` or its engine tests. +- Changing `step_complete_kandev` or any normal Kanban MCP registration. +- Reusing or changing `agentctl kandev approvals decide`. + +## Acceptance + +- Office `tools/list` and first-turn context omit + `record_step_decision_kandev`; the Office tool count is 11. +- Normal Kanban tool registration is byte-for-byte unchanged in intent and has + no Office decision metadata or instructions. +- No dead websocket decision action, handler, dashboard dependency, or + transport-only test remains. + +## Verification + +```bash +cd apps/backend && rtk go test ./internal/mcp/server ./internal/mcp/handlers ./internal/backendapp -run 'Test.*(ModeOffice|ModeTask|Sysprompt|Decision|HandlerRegistration)' -count=1 +cd ../.. && rtk node --test scripts/validate-public-docs.test.mjs && rtk node scripts/validate-public-docs.mjs +``` + +## Files likely touched + +- `apps/backend/config/prompts/office-context.md` +- `apps/backend/internal/mcp/server/server.go` +- `apps/backend/internal/mcp/server/server_test.go` +- `apps/backend/internal/mcp/server/sysprompt_sync_test.go` +- `apps/backend/internal/mcp/server/agent_decision_tool.go` +- `apps/backend/internal/mcp/server/agent_decision_tool_test.go` +- `apps/backend/internal/mcp/handlers/handlers.go` +- `apps/backend/internal/mcp/handlers/agent_decision_handlers.go` +- `apps/backend/internal/mcp/handlers/agent_decision_handlers_test.go` +- `apps/backend/internal/backendapp/helpers.go` +- `apps/backend/pkg/websocket/actions.go` +- `docs/public/automation-and-mcp.md` +- `docs/public/coverage.json` + +## Dependencies + +- Task 02 verifies the replacement skill and prompt path before deletion. + +## Risks + +- Exact context-inventory tests must change with the registry so documentation + cannot drift from `tools/list`. +- Removing shared handler wiring must not affect other Office or Kanban tools. + +## Parallelism + +`sequential` + +## Inputs + +- The verified outputs of Tasks 01 and 02. +- Current Office MCP registry, prompt inventory, and public CLI reference. + +## Results + +Removed the Office decision MCP tool, websocket action, handler dependency and +composition wiring, plus their transport-only tests. The Office context and +public reference now describe the 11-tool catalog and the task-bound CLI +decision command. Kanban registration remains unchanged and negative tests +cover the absence of Office decision metadata from both surfaces. + +Verification: + +```text +cd apps/backend && rtk go test ./internal/mcp/server ./internal/mcp/handlers ./internal/backendapp -run 'Test.*(ModeOffice|ModeTask|Sysprompt|Decision|HandlerRegistration)' -count=1 +cd ../.. && rtk node --test scripts/validate-public-docs.test.mjs && rtk node scripts/validate-public-docs.mjs +``` + +Passed: 22 backend tests, 61 public-doc tests, and validation of 46 published +docs pages. diff --git a/docs/public/automation-and-mcp.md b/docs/public/automation-and-mcp.md index 1e37faf15f..245e9c0ecc 100644 --- a/docs/public/automation-and-mcp.md +++ b/docs/public/automation-and-mcp.md @@ -582,12 +582,11 @@ Office runs use a smaller MCP surface than regular task-mode sessions. The built - `ask_user_question_kandev`; - `create_task_plan_kandev`, `get_task_plan_kandev`, `update_task_plan_kandev`, and `delete_task_plan_kandev`; - `list_related_tasks_kandev`; -- `list_task_documents_kandev`, `get_task_document_kandev`, and `write_task_document_kandev`. +- `list_task_documents_kandev`, `get_task_document_kandev`, and `write_task_document_kandev`; - `show_rich_output_kandev`; -- `record_step_decision_kandev` records an `approved` or `rejected` verdict for the current workflow step. It requires a non-empty reason, and a later verdict supersedes the earlier one. - `step_complete_kandev`, per ADR 0015: Kandev includes its completion instruction, and acts on its signal, only on Office steps whose auto-advance action explicitly requires that signal (office-default's `work` step is one such step). -These tools cover human questions, the current task plan, related-task discovery, task documents, quorum decisions, and the step-completion signal. Office state changes use the injected `$KANDEV_CLI kandev ...` commands instead. An Office agent should not search for additional Kandev MCP tools: Kanban/configuration tools are task-mode only and are not registered in Office mode. +These tools cover human questions, the current task plan, related-task discovery, task documents, and the step-completion signal. Office state changes use the injected `$KANDEV_CLI kandev ...` commands instead. An Office agent should not search for additional Kandev MCP tools: Kanban/configuration tools are task-mode only and are not registered in Office mode. ### Runtime credentials @@ -603,6 +602,19 @@ missing, do not set either variable yourself. A regular task session should use its injected Kandev MCP tools. An Office-owned task must be started or woken through Office so the scheduler can supply its signed runtime context. +Reviewers and approvers record a workflow-step verdict through the task-bound +runtime CLI. The command accepts only `approved` or `rejected` and requires a +non-empty reason: + +```bash +$KANDEV_CLI kandev task decision --decision approved --reason "..." +``` + +The runtime derives the task, session, and agent identity from the signed run +context. A repeated decision supersedes the earlier decision for that +participant and step. Comments and approval-inbox commands do not record a +workflow-step verdict. + An Office run can inspect the projects in its current workspace: ```bash diff --git a/docs/public/coverage.json b/docs/public/coverage.json index 41f54be343..1f61567772 100644 --- a/docs/public/coverage.json +++ b/docs/public/coverage.json @@ -130,16 +130,17 @@ "tasks-and-workflows" ], "sources": [ - "apps/backend/internal/mcp/server/agent_decision_tool.go", + "apps/backend/cmd/agentctl/kandev_task.go", + "apps/backend/internal/office/configloader/skills/kandev-step-decision/SKILL.md", + "apps/backend/internal/office/runtime/handler.go", "apps/backend/internal/office/dashboard/agent_decisions.go" ], "tests": [ - "apps/backend/internal/mcp/server/agent_decision_tool_test.go", + "apps/backend/cmd/agentctl/kandev_task_decision_test.go", + "apps/backend/internal/office/runtime/handler_decision_test.go", "apps/backend/internal/office/dashboard/agent_decisions_test.go" ], - "mcpTools": [ - "record_step_decision_kandev" - ] + "mcpTools": [] }, { "id": "task-lifecycle", diff --git a/docs/specs/office/system-design/agents-02.md b/docs/specs/office/system-design/agents-02.md index 711da9dac0..227290e57e 100644 --- a/docs/specs/office/system-design/agents-02.md +++ b/docs/specs/office/system-design/agents-02.md @@ -246,7 +246,7 @@ Three MCP modes coexist: |---|---|---|---| | `ModeTask` | 27 (kanban + plans + walkthroughs + coordination + completion) | ~3-5K | Interactive kanban sessions | | `ModeConfig` | 29 (workflows + agents + executors) | ~8-10K | Config setup sessions | -| `ModeOffice` | 12 (plans + tasks + rich output + decisions + completion) | ~1-2K | Office agent sessions | +| `ModeOffice` | 11 (plans + tasks + rich output + completion) | ~1-2K, without the decision schema | Office agent sessions | Agent routing and Office ownership are independent. Workflow-level defaults, per-step agent profiles, and `runner` participants select the execution identity only; they never make a Kanban task Office-owned. A task is Office-owned only when it is linked to an Office project or its workflow matches the workspace's Office workflow. @@ -255,13 +255,17 @@ Agent routing and Office ownership are independent. Workflow-level defaults, per - `ask_user_question_kandev` (only meaningful when the user opens the task in advanced mode). - `list_related_tasks_kandev`. - 3 task-document tools (`list_task_documents_kandev`, `get_task_document_kandev`, `write_task_document_kandev`). -- `show_rich_output_kandev`, `record_step_decision_kandev`, and gated `step_complete_kandev`. +- `show_rich_output_kandev` and gated `step_complete_kandev`. -`ModeOffice` excludes kanban/config tools and workspace/workflow listing tools. Its first-turn context lists only registered tools, and Office mutations use `$KANDEV_CLI`. +`ModeOffice` excludes kanban/config tools, workspace/workflow listing tools, and +the workflow-step decision action. Its first-turn context lists only registered +tools. Reviewer and approver runs receive an injected Kandev skill for +`$KANDEV_CLI kandev task decision`; other Office mutations also use +`$KANDEV_CLI`. ### Skills are preferred over MCP tools -Skills are the preferred pattern for teaching agents office capabilities. A skill provides instructions in `SKILL.md` and the agent calls API endpoints via `$KANDEV_CLI`. This is cheaper than MCP tools: instructions read once per session, shell calls thereafter; MCP tool definitions add per-call overhead (tool schemas in context, structured I/O parsing on every invocation). The `kandev-protocol` system skill teaches CLI usage and replaces the earlier curl-based version. New office capabilities expose API endpoints, ship a skill that teaches the agent how to call them, and assign the skill to agents that need it. +Skills are the preferred pattern for teaching agents office capabilities. A skill provides instructions in `SKILL.md` and the agent calls API endpoints via `$KANDEV_CLI`. This is cheaper than MCP tools: instructions read once per session, shell calls thereafter; MCP tool definitions add per-turn schema cost and structured I/O parsing on every invocation. The `kandev-protocol` system skill teaches general CLI usage and replaces the earlier curl-based version. The narrow `kandev-step-decision` system skill is injected when the launch-time seat lookup finds a reviewer or approver seat; the runtime endpoint still authorizes the seat live on every call. New office capabilities expose API endpoints, ship a skill that teaches the agent how to call them, and assign or inject the skill only where it is useful. ## UI diff --git a/docs/specs/office/system-design/runtime-01.md b/docs/specs/office/system-design/runtime-01.md index b5fc310fc2..00b3bdd12d 100644 --- a/docs/specs/office/system-design/runtime-01.md +++ b/docs/specs/office/system-design/runtime-01.md @@ -261,7 +261,7 @@ Recovery and runtime-action authorization splits across three actors: human user - **Mark fixed (inbox dismiss + retry)** and **Resume session**: any workspace user. There is no agent-role gating on these affordances; they are explicit human-driven recovery actions surfaced in the inbox and per-task chat. - **Reassignment** (`PATCH /tasks/:id` with new `assignee_agent_instance_id`): workspace user. Agent JWTs cannot reassign tasks via the runtime action surface in v1; there is no `CapabilityReassignTask`. - **Manual unpause** (clearing `pause_reason` from the agent detail page): workspace admin / CEO role only, enforced by `isAdminRole` in `internal/office/agents/handler.go`. -- **Runtime action surface capabilities**: derived from `agent_instances.role` / `permissions` via `runtime.FromAgent`. The agent JWT carries the serialized `Capabilities` snapshot taken at run-claim time; subsequent permission revocations on the agent profile do not affect a JWT already in flight (it expires after `DefaultTokenDuration = 4h`). The seat-derived `record_step_decision` MCP action is not a runtime capability. `runtime.ContextBuilder.Build` stores it in `RunContext.AvailableActions` when the agent holds the current workflow participant seat. The scheduler adds this advisory action to the prompt, but not to the JWT capability claims. `handleRecordStepDecision` performs live authorization with `ResolveParticipantRole`. +- **Runtime action surface capabilities**: derived from `agent_instances.role` / `permissions` via `runtime.FromAgent`. The agent JWT carries the serialized `Capabilities` snapshot taken at run-claim time; subsequent permission revocations on the agent profile do not affect a JWT already in flight (it expires after `DefaultTokenDuration = 4h`). The seat-derived `record_step_decision` CLI affordance is not a runtime capability. `runtime.ContextBuilder.Build` stores it in `RunContext.AvailableActions` when the agent holds the current workflow participant seat. The scheduler uses this advisory action to inject `kandev-step-decision` and add the command to the prompt, but does not add it to JWT capability claims. The task-bound runtime decision handler derives task, session, and agent identity from the signed run context and performs live authorization through `DashboardService.RecordAgentDecision` and `ResolveParticipantRole`. - **Task scope**: `RunContext.CanMutateTask(taskID)` returns true only when `taskID == runCtx.TaskID` or `taskID` (or wildcard `*`) appears in `Capabilities.AllowedTaskIDs`. Out-of-scope mutations return `ErrTaskOutOfScope`. - **Workspace scope**: every action that touches another entity (target agent, target skill, target task) is rejected with `ErrWorkspaceOutOfScope` when the target's `workspace_id` does not match the run's. - **Memory namespaces**: `CanAccessMemory` enforces workspace match plus, for `kind=agent`, that the namespace ID matches `runCtx.AgentID`. Agents cannot read or write another agent's memory. diff --git a/docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md b/docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md index e4920c7f32..893432d78f 100644 --- a/docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md +++ b/docs/specs/tasks/requirements/workflow-quorum-decision-recording-agent-surface.md @@ -32,9 +32,11 @@ An Office participant can submit a validated verdict with a reason, and the resu - **AC-TASKS-QUORUM-RECORDING-001.1:** WHEN an Office agent session is attached to the task's current step with role `reviewer` or `approver` and `decision_required = 1`, THE SYSTEM - SHALL expose an MCP tool that records a decision, accepting a verdict of - `approved` or `rejected` and a required non-empty reason. -- **AC-TASKS-QUORUM-RECORDING-001.2:** WHEN that tool is called with a valid verdict, THE SYSTEM SHALL write + SHALL inject a Kandev skill and stage prompt that require the task-bound + `$KANDEV_CLI kandev task decision` command as the agent's final action. The + command SHALL accept a verdict of `approved` or `rejected` and a required + non-empty reason. +- **AC-TASKS-QUORUM-RECORDING-001.2:** WHEN that command is called with a valid verdict, THE SYSTEM SHALL write one row to `workflow_step_decisions` with `task_id` = the calling session's task, `step_id` = that task's current `workflow_step_id`, `role` = the caller's resolved role, `decider_type` = `agent`, `decider_id` = the caller's @@ -71,16 +73,18 @@ An Office participant can submit a validated verdict with a reason, and the resu an approver attached at an earlier step is currently unresolvable at the step where the approval is due; role resolution and slate membership SHALL NOT disagree about who participates. -- **AC-TASKS-QUORUM-RECORDING-001.6:** WHEN the verdict is any value other than `approved` or `rejected`, +- **AC-TASKS-QUORUM-RECORDING-001.6:** WHEN the command or its Office API receives a verdict other than `approved` or `rejected`, THE SYSTEM SHALL reject the call with a validation error and write no row. -- **AC-TASKS-QUORUM-RECORDING-001.7:** WHEN the reason is empty or whitespace-only, THE SYSTEM SHALL reject +- **AC-TASKS-QUORUM-RECORDING-001.7:** WHEN the command or its Office API receives an empty or whitespace-only reason, THE SYSTEM SHALL reject the call with a validation error and write no row. - **AC-TASKS-QUORUM-RECORDING-001.8:** WHEN the task has no `workflow_step_id` bound, THE SYSTEM SHALL reject the call with an error naming the unbound step and write no row. -- **AC-TASKS-QUORUM-RECORDING-001.9:** THE SYSTEM SHALL NOT expose the decision tool to sessions whose - profile does not enable `SurfaceOfficeTask`. +- **AC-TASKS-QUORUM-RECORDING-001.9:** THE SYSTEM SHALL NOT expose decision MCP + metadata, the Office decision skill, or Office decision instructions to a + normal Kanban session. The task-bound command SHALL be available only inside + a scheduler-prepared, signed Office run context. -- **AC-TASKS-QUORUM-RECORDING-001.10:** THE SYSTEM SHALL validate the decision tool's preconditions in this +- **AC-TASKS-QUORUM-RECORDING-001.10:** THE SYSTEM SHALL validate the Office decision API's preconditions in this order: (1) the task has a bound `workflow_step_id` (AC-TASKS-QUORUM-RECORDING-001.8), (2) the caller resolves to a participant with `decision_required = 1` for `reviewer` or `approver` (AC-TASKS-QUORUM-RECORDING-001.3), (3) the verdict is `approved` or `rejected` (AC-TASKS-QUORUM-RECORDING-001.6), (4) the @@ -93,8 +97,8 @@ An Office participant can submit a validated verdict with a reason, and the resu both preconditions are false at once in that case and the two errors differ in diagnostic value. -- **AC-TASKS-QUORUM-RECORDING-001.11:** WHEN the decision tool returns successfully, THE SYSTEM SHALL return - the seven fields enumerated in `## Tool contract` — `decision`, `role`, +- **AC-TASKS-QUORUM-RECORDING-001.11:** WHEN the decision command returns successfully, THE SYSTEM SHALL return + the seven fields enumerated in the system design's agent command contract: `decision`, `role`, `step_id`, `decision_id`, `decided_at`, `transition_applied` and `guards` — and SHALL NOT return a scalar `required_count` or `received_count`. `guards` SHALL equal the AC-TASKS-QUORUM-REEVALUATION-001.14 snapshot's `Guards` for the validated step, @@ -111,6 +115,11 @@ An Office participant can submit a validated verdict with a reason, and the resu and `transition_applied`, via AC-TASKS-QUORUM-BINDING-001.2 and AC-TASKS-QUORUM-CONCURRENCY-001.4 — were observable by any acceptance criterion, so a wrong `role` or a wrong count could ship untested. +- **AC-TASKS-QUORUM-RECORDING-001.12:** WHEN an Office session lists its MCP + tools after the CLI migration, THE SYSTEM SHALL NOT include + `record_step_decision_kandev` or its JSON schema. Normal Kanban MCP profiles + SHALL remain unchanged by this migration. + ## Out of scope diff --git a/docs/specs/tasks/system-design/workflow-quorum-decision-recording.md b/docs/specs/tasks/system-design/workflow-quorum-decision-recording.md index cb0396f798..e403c9c574 100644 --- a/docs/specs/tasks/system-design/workflow-quorum-decision-recording.md +++ b/docs/specs/tasks/system-design/workflow-quorum-decision-recording.md @@ -22,7 +22,7 @@ requirements: -The task system owns quorum semantics, participant canonicalization, decision persistence, guarded transition application, and diagnostic projections. Office and MCP surfaces are transports over that engine-owned contract. +The task system owns quorum semantics, participant canonicalization, decision persistence, guarded transition application, and diagnostic projections. The task-bound Office CLI and runtime API are transports over that engine-owned contract. @@ -70,18 +70,19 @@ The engine's evaluation is therefore correct **given its inputs**. With zero decision rows, `all_approve` and `any_reject` both correctly evaluate false and the card correctly stays put. The defects are all on the input side. -### Defect 1 — an agent has no tool to record a decision +### Transport follow-up — remove the per-turn decision schema -The MCP tool registry (`apps/backend/internal/mcp/server/server.go:892-923`) -enables exactly four groups for `SurfaceOfficeTask`: `plan` (4 tools), -`related-tasks` (1), `office-documents` (3), and the capability-gated -`user-question` (1). Nine tools, none of which records a decision. +The first agent surface added `record_step_decision_kandev` to +`SurfaceOfficeTask`. That proved the server-owned decision and quorum path, but +it also makes every Office turn carry a decision JSON schema, including turns +whose agents do not occupy a reviewer or approver seat. -`POST /api/v1/office/tasks/:id/approve` and `.../request-changes` exist -(`office/dashboard/handler.go:91-92`) and are reachable from the human UI -(`apps/web/components/task/simple/components/approval-action-bar.tsx:122-123`), -but no agent can reach either. A reviewer agent's only means of expressing a -verdict is a free-text comment, which nothing parses. +Office already uses a signed, task-bound runtime CLI for its mutation surface. +The final transport moves decision recording to that established path while +retaining `DashboardService.RecordAgentDecision` and the workflow engine as the +only business and transition path. The MCP registration, websocket transport, +and transport-only tests are removed only after the CLI, runtime endpoint, and +injected skill are covered. ### Defect 2 — the human path writes a verdict the evaluator cannot read @@ -183,14 +184,19 @@ which `ConfigTransitionGuard` accepts (`workflow/engine/types.go:533`): Approval is the same shape with `role: approver`. -## Tool contract +## Agent command contract Pinned here because Build would otherwise invent it and Review would relitigate it. -**Name.** `record_step_decision_kandev`, registered under a new -`office-decisions` group in the profile registry -(`apps/backend/internal/mcp/server/server.go:892-923`), enabled for -`SurfaceOfficeTask` only. +**Command.** `$KANDEV_CLI kandev task decision --decision approved|rejected +--reason "..."`. The command is taught through the injected +`kandev-step-decision` system skill and repeated in the review and approval +stage prompts as the required final action. + +**HTTP endpoint.** `POST /api/v1/office/runtime/task/decision` with a closed JSON +body containing only `decision` and `reason`. The route uses the existing +Office runtime bearer-token middleware and does not expose a task id in its +path. Identity-shaped body fields are rejected rather than ignored. **Arguments.** @@ -200,16 +206,27 @@ Pinned here because Build would otherwise invent it and Review would relitigate | `reason` | yes | non-empty string | No idempotency argument. A retry is a new verdict that supersedes the previous -one under AC-TASKS-QUORUM-CONCURRENCY-001.2; the tool is deliberately NOT idempotent across calls, and +one under AC-TASKS-QUORUM-CONCURRENCY-001.2; the command is deliberately NOT idempotent across calls, and AC-TASKS-QUORUM-CONCURRENCY-001.7 states this as contract so a builder does not invent a client-supplied key. Idempotency in this feature is confined to transition application (AC-TASKS-QUORUM-CONCURRENCY-001.6, AC-TASKS-QUORUM-CONCURRENCY-001.4). -**Scoping.** The tool takes no `task_id`. It resolves the task and step from the -calling session, matching every other Office tool. An agent cannot record a -decision on a task other than the one its session is bound to. +**Scoping.** The command and endpoint accept no task, step, role, participant, +session, or agent identifier. The runtime handler derives `task_id`, +`session_id`, and `agent_profile_id` from the validated signed Office run +context. `DashboardService.RecordAgentDecision` resolves the task's current +workflow step and the caller's canonical reviewer or approver seat live on +every call. A token bound to one task cannot address another task, and a caller +that no longer holds a decision seat is rejected without a write. -**Return.** On success the tool returns exactly these seven fields, observed by +**Transport boundary.** The runtime handler remains thin. A small composition +adapter translates the runtime request and response types and calls +`DashboardService.RecordAgentDecision`; the dashboard service continues to own +validation order, decision publication, activity, reactivity, and delegation +to the workflow engine's decision path. The runtime layer does not implement a +second quorum or persistence path. + +**Return.** On success the command returns exactly these seven fields, observed by AC-TASKS-QUORUM-RECORDING-001.11: | Field | Meaning | @@ -264,6 +281,11 @@ approver precedence, as required by AC-TASKS-QUORUM-RECORDING-001.4a. The two paths can therefore persist different roles for the same caller, task, and step. Future implementations must preserve this boundary. +**MCP boundary.** `record_step_decision_kandev` is absent from the +`SurfaceOfficeTask` registry and first-turn MCP inventory after this command is +wired. Normal Kanban profiles remain unchanged and never receive the Office +decision skill or CLI instruction. + **Reason column.** The reason is written to `workflow_step_decisions.comment`, the column every Office reader already projects (`office/dashboard/decisions.go:141,428`). It is NOT written to `note`, which `Engine.RecordParticipantDecision` currently @@ -277,7 +299,7 @@ task timeline — this is the concrete bug this paragraph exists to prevent. | Condition | Behavior | |---|---| | Agent not a participant | Permission error; no row written (AC-TASKS-QUORUM-RECORDING-001.3) | -| Verdict outside `approved`/`rejected` on the tool | Validation error; no row (AC-TASKS-QUORUM-RECORDING-001.6) | +| Verdict outside `approved`/`rejected` on the command or API | Validation error; no row (AC-TASKS-QUORUM-RECORDING-001.6) | | Empty reason | Validation error; no row (AC-TASKS-QUORUM-RECORDING-001.7) | | Task has no bound step | Error naming the unbound step; no row (AC-TASKS-QUORUM-RECORDING-001.8) | | Decision store not wired | Guard does not fire; reason recorded (AC-TASKS-QUORUM-DIAGNOSTICS-001.1) | @@ -312,7 +334,7 @@ task timeline — this is the concrete bug this paragraph exists to prevent. | AC-TASKS-QUORUM-DIAGNOSTICS-001.4 read while the engine dispatcher is not wired | Endpoint errors; no office-side fallback evaluation (AC-TASKS-QUORUM-REEVALUATION-001.14, AC-TASKS-QUORUM-REEVALUATION-001.13) | | One guard's store read errors, siblings healthy | That entry alone reports `evaluation_error`; the others are still returned (AC-TASKS-QUORUM-REEVALUATION-001.14, AC-TASKS-QUORUM-DIAGNOSTICS-001.1) | | Decision recorded at a step that also configures a side-effect action on `on_turn_complete` | Only guarded transitions are evaluated; no callback re-fires (AC-TASKS-QUORUM-REEVALUATION-001.8) | -| Tool called at a step with no guarded transition | `guards` is an empty list, not an error (AC-TASKS-QUORUM-RECORDING-001.11) | +| Command called at a step with no guarded transition | `guards` is an empty list, not an error (AC-TASKS-QUORUM-RECORDING-001.11) | | Decision row id needed for the AC-TASKS-QUORUM-CONCURRENCY-001.6 operation id | Pre-generated caller-side into `DecisionInfo.ID`; never read back after the write (AC-TASKS-QUORUM-CONCURRENCY-001.8) | | Office needs `decision_id` / `created_at` to publish | Both returned by the AC-TASKS-QUORUM-REEVALUATION-001.10 entry point (AC-TASKS-QUORUM-REEVALUATION-001.11) | @@ -340,7 +362,8 @@ Each of the following is a deliberate exclusion, not an oversight. - **Workspace `approvals` subsystem.** `POST /approvals/:id/decide` and its `"approved" | "rejected"` shape (`apps/web/lib/api/domains/office-api.ts:534`) - are a separate feature from task step decisions. Untouched. + are a separate feature from task step decisions. The + `agentctl kandev approvals decide` command remains untouched and is not reused. - **New thresholds.** `all_approve`, `all_decide`, `any_reject`, `majority_approve`, `n_approve:` are the complete set. No new threshold. - **Guard variants beyond `wait_for_quorum`.** `TransitionGuard` stays @@ -359,10 +382,11 @@ Each of the following is a deliberate exclusion, not an oversight. - **Timeouts and escalation.** A quorum that is never reached waits indefinitely. No auto-advance, no reminder, no deadline. - **Delegating or reassigning a pending decision.** Not addressed. -- **Kanban-workflow decisions.** The decision tool is Office-surface only - (AC-TASKS-QUORUM-RECORDING-001.9); Kanban keeps `step_complete_kandev`. +- **Kanban-workflow decisions.** The decision command is Office-run only + (AC-TASKS-QUORUM-RECORDING-001.9); Kanban keeps `step_complete_kandev` and + its existing MCP profile. - **Backfilling the zero existing decision rows.** There are none to migrate. -- **Client-supplied idempotency keys on the decision tool.** Excluded by AC-TASKS-QUORUM-CONCURRENCY-001.7. +- **Client-supplied idempotency keys on the decision command.** Excluded by AC-TASKS-QUORUM-CONCURRENCY-001.7. Transition application is idempotent (AC-TASKS-QUORUM-CONCURRENCY-001.6); recording is not. - **Giving the singleton human user a participant row.** AC-TASKS-QUORUM-VERDICT-001.3 makes a human rejection count without one. Seating the user in the slate would change who