diff --git a/.kiro-krew/specs/issue-251-acp-session-missing-cwd-parameter.md b/.kiro-krew/specs/issue-251-acp-session-missing-cwd-parameter.md new file mode 100644 index 0000000..acc2a38 --- /dev/null +++ b/.kiro-krew/specs/issue-251-acp-session-missing-cwd-parameter.md @@ -0,0 +1,389 @@ +# Design Specification: Fix ACP Session Creation Missing Required `cwd` Parameter + +**Issue**: #251 +**Title**: ACP session creation missing required cwd parameter causing "Improperly formed request" error +**Repository**: jbrinkman/kiro-krew +**Closes**: #251 + +## Problem Statement + +The planning tab's ACP session creation fails with an "Improperly formed request" error because the `NewSessionRequest` is missing the required `cwd` (current working directory) parameter as specified in the ACP protocol specification. + +### Error Manifestation + +``` +2026/07/15 07:21:24 ERRO streaming prompt failed session_id=ef5b2781-c9e0-41a8-936d-e87004e4ac33 error="{\"code\":-32603,\"message\":\"Internal error\",\"data\":\"Encountered an error in the response stream: Improperly formed request. (request_id: 332a05f2-73ca-46df-91e3-2d99069066c2)\"}" +``` + +### Root Cause + +According to the [ACP protocol documentation](https://agentclientprotocol.com/protocol/v1/session-setup#creating-a-session), the `session/new` method requires: +- `cwd`: The working directory for the session (must be an absolute path) +- `mcpServers`: List of MCP servers (can be empty slice) + +The current implementation at `internal/acp/client.go` lines 320 and 407 only provides `McpServers` and omits the required `Cwd` field. + +## Solution Approach + +Add the `cwd` parameter to the ACP connection configuration and ensure it's passed to all `NewSession` calls (both streaming and non-streaming). The solution follows the pattern established in the reference implementation at `/Users/jbrinkman/projects/kiro-acp-client/main.go`. + +### Design Principles + +1. **Protocol Compliance**: Strictly follow ACP protocol specification requirements +2. **Minimal Changes**: Add only what's required without refactoring unrelated code +3. **Consistency**: Apply the fix uniformly to both streaming and non-streaming paths +4. **Validation**: Ensure the `cwd` is an absolute path before use +5. **Backward Compatibility**: Default to current working directory if not explicitly set + +## Relevant Files + +### Files to Modify + +1. **`internal/acp/types.go`** (line ~88) + - Add `Cwd string` field to `ConnectionConfig` struct + - Update `DefaultConnectionConfig()` to initialize `Cwd` with current working directory + - Update `ValidateConnectionConfig()` to validate `Cwd` is absolute path + +2. **`internal/acp/client.go`** (lines 320, 407) + - Update `NewSession` call on line 320 (non-streaming) to include `Cwd: c.config.Cwd` + - Update `NewSession` call on line 407 (streaming) to include `Cwd: c.config.Cwd` + +3. **`internal/tui/planning_tab.go`** (line ~131) + - When creating ACP client, set `config.Cwd` to project root (use `os.Getwd()`) + - Add logging to record the working directory being used + +### Files for Reference + +- `/Users/jbrinkman/projects/kiro-acp-client/main.go` - Reference implementation showing correct usage + +## Team Orchestration + +This is a straightforward bug fix with minimal dependencies: + +1. **Task 1**: Update `ConnectionConfig` type and validation (types.go) + - **Dependencies**: None + - Can proceed immediately + +2. **Task 2**: Update both `NewSession` calls in ACP client (client.go) + - **Dependencies**: Task 1 (needs updated `ConnectionConfig` structure) + - Sequential after Task 1 + +3. **Task 3**: Initialize `Cwd` in planning tab (planning_tab.go) + - **Dependencies**: Task 1 (needs updated `ConnectionConfig` structure) + - Can run in parallel with Task 2 + +All tasks contribute to complete issue resolution in one PR. Tasks 2 and 3 can be parallelized after Task 1 completes. + +## Step-by-Step Task Breakdown + +### Task 1: Update ConnectionConfig Structure + +**File**: `internal/acp/types.go` + +**Changes Required**: + +1. Add `Cwd` field to `ConnectionConfig` struct (around line 88): +```go +type ConnectionConfig struct { + // KiroCLIPath is the path to the Kiro CLI executable + KiroCLIPath string `json:"kiro_cli_path,omitempty"` + + // Agent is the name of the agent to connect to + Agent string `json:"agent,omitempty"` + + // Cwd is the working directory for the ACP session (must be absolute path) + Cwd string `json:"cwd,omitempty"` + + // MaxRetries is the maximum number of retry attempts + MaxRetries int `json:"max_retries,omitempty"` + + // ... rest of fields +} +``` + +2. Update `DefaultConnectionConfig()` function to initialize `Cwd`: +```go +func DefaultConnectionConfig() *ConnectionConfig { + cwd, err := os.Getwd() + if err != nil { + cwd = "." // Fallback to current directory + } + // Convert to absolute path + if !filepath.IsAbs(cwd) { + if absCwd, err := filepath.Abs(cwd); err == nil { + cwd = absCwd + } + } + + return &ConnectionConfig{ + KiroCLIPath: "kiro-cli", + Agent: "", // Must be set explicitly by caller + Cwd: cwd, + MaxRetries: 3, + RetryDelay: 1 * time.Second, + ConnectionTimeout: 30 * time.Second, + RequestTimeout: 60 * time.Second, + } +} +``` + +3. Update `ValidateConnectionConfig()` to validate `Cwd`: +```go +func ValidateConnectionConfig(config *ConnectionConfig) error { + if config == nil { + return ErrInvalidConfig + } + + if config.Agent == "" { + return ErrMissingConfigAgent + } + + if config.KiroCLIPath == "" { + return errors.New("kiro-cli path is required") + } + + if config.Cwd == "" { + return errors.New("cwd (working directory) is required") + } + + // Validate Cwd is absolute path + if !filepath.IsAbs(config.Cwd) { + return errors.New("cwd must be an absolute path") + } + + if config.ConnectionTimeout <= 0 || config.RequestTimeout <= 0 { + return errors.New("timeouts must be positive") + } + + return nil +} +``` + +**Required Imports**: Add `path/filepath` and `os` if not already imported + +**Acceptance Criteria**: +- `ConnectionConfig` struct has `Cwd string` field +- `DefaultConnectionConfig()` initializes `Cwd` with absolute path to current working directory +- `ValidateConnectionConfig()` validates `Cwd` is non-empty and absolute path +- All validations pass with appropriate error messages + +### Task 2: Update NewSession Calls in ACP Client + +**File**: `internal/acp/client.go` + +**Changes Required**: + +1. Update non-streaming `NewSession` call (line ~320): +```go +// Before: +sessionResp, err := conn.NewSession(ctx, acp.NewSessionRequest{ + McpServers: []acp.McpServer{}, +}) + +// After: +logging.Debug("creating ACP session", "cwd", c.config.Cwd) +sessionResp, err := conn.NewSession(ctx, acp.NewSessionRequest{ + Cwd: c.config.Cwd, + McpServers: []acp.McpServer{}, +}) +``` + +2. Update streaming `NewSession` call (line ~407): +```go +// Before: +sessionResp, err := conn.NewSession(streamCtx, acp.NewSessionRequest{ + McpServers: []acp.McpServer{}, +}) + +// After: +logging.Debug("creating streaming ACP session", "cwd", c.config.Cwd) +sessionResp, err := conn.NewSession(streamCtx, acp.NewSessionRequest{ + Cwd: c.config.Cwd, + McpServers: []acp.McpServer{}, +}) +``` + +**Acceptance Criteria**: +- Both `NewSession` calls (streaming and non-streaming) include `Cwd` field +- Debug logging includes the working directory being used +- No changes to surrounding error handling or session management logic +- All existing tests pass + +### Task 3: Initialize Cwd in Planning Tab + +**File**: `internal/tui/planning_tab.go` + +**Changes Required**: + +Update the ACP client creation section (around line 131 in `NewPlanningTabWithSession`): + +```go +// Initialize ACP client with provided client or create default +if acpClient == nil { + logging.Debug("creating default ACP client for planner agent", "tab_id", id) + + // Get current working directory for ACP session + cwd, err := os.Getwd() + if err != nil { + logging.Warn("failed to get current working directory, using fallback", "tab_id", id, "error", err) + cwd = "." + } + + // Ensure absolute path + if !filepath.IsAbs(cwd) { + if absCwd, err := filepath.Abs(cwd); err == nil { + cwd = absCwd + } + } + + logging.Info("initializing ACP client with working directory", "tab_id", id, "cwd", cwd) + + config := acp.DefaultConnectionConfig() + config.Agent = "planner" + config.Cwd = cwd + acpClient = acp.NewClient(config) +} else { + logging.Debug("using provided ACP client", "tab_id", id) +} +``` + +**Required Imports**: Add `os` and `path/filepath` if not already imported + +**Acceptance Criteria**: +- Planning tab sets `Cwd` when creating ACP client +- Working directory is logged for debugging +- Absolute path is ensured before setting +- Error handling for `os.Getwd()` failure with appropriate fallback +- Existing ACP client functionality remains unchanged + +## Validation Commands + +### Unit Testing + +```bash +# Run ACP-related tests +go test ./internal/acp/... -v + +# Run planning tab tests +go test ./internal/tui/... -run TestPlanning -v +``` + +### Integration Testing + +```bash +# Build the project +task build + +# Run the TUI and test planning tab +./kiro-krew + +# In the REPL: +# 1. Press Ctrl+Alt+P to switch to planning mode +# 2. Type a simple prompt like "hello" and press Enter +# 3. Verify no "Improperly formed request" error appears +# 4. Verify the planner responds successfully +``` + +### Manual Verification Checklist + +1. **Start kiro-krew and open planning tab** + - No errors in logs during ACP connection + - `cwd` appears in debug logs during session creation + +2. **Send a prompt in planning tab** + - No "Improperly formed request" error + - Session ID appears in logs + - Streaming response received successfully + - No JSON-RPC protocol errors + +3. **Check logs for working directory** + - Look for log entries containing `cwd` parameter + - Verify path is absolute + - Verify path matches current working directory + +### Expected Log Output + +``` +INFO initializing ACP client with working directory tab_id=planning-1 cwd=/Users/username/projects/kiro-krew +DEBUG creating ACP session cwd=/Users/username/projects/kiro-krew +INFO streaming ACP session created session_id= +DEBUG sending prompt for streaming session_id= +INFO streaming completed session_id= +``` + +### Regression Testing + +Verify existing functionality still works: +- Non-streaming message requests work correctly +- Session reuse across multiple prompts works +- Error handling for connection failures remains functional +- Planning tab state management unaffected + +## Constraints + +1. **Protocol Compliance**: Must match ACP protocol specification exactly +2. **No Breaking Changes**: Existing ACP functionality must continue working +3. **Path Safety**: `cwd` must always be an absolute path +4. **Error Handling**: Gracefully handle failures to determine working directory +5. **Logging**: Add appropriate debug/info logging without cluttering logs + +## Technical Notes + +### Why This Fix Works + +The ACP protocol requires `cwd` for session creation because: +1. Agents need context about where to resolve relative file paths +2. The working directory affects tool execution and file operations +3. The protocol enforces this as a mandatory field for session context + +### Reference Implementation Analysis + +The working example at `/Users/jbrinkman/projects/kiro-acp-client/main.go` shows: +```go +cwd, err := filepath.Abs(os.Args[1]) +if err != nil { + log.Fatal(err) +} + +// ... + +sess, err := conn.NewSession(ctx, acp.NewSessionRequest{ + Cwd: cwd, // ✅ Required field included + McpServers: []acp.McpServer{}, +}) +``` + +Our implementation follows the same pattern but uses `os.Getwd()` instead of requiring it as a parameter. + +### Potential Edge Cases + +1. **Symbolic Links**: `filepath.Abs()` resolves symlinks, which is the correct behavior +2. **Permission Issues**: If `os.Getwd()` fails due to permissions, we fall back to "." and let `filepath.Abs()` resolve it +3. **Windows Paths**: `filepath.IsAbs()` and `filepath.Abs()` are cross-platform and handle Windows paths correctly + +## Success Criteria + +The fix is complete when: + +1. ✅ All three files are updated with the required changes +2. ✅ Unit tests pass for modified code +3. ✅ Planning tab can send prompts without protocol errors +4. ✅ Debug logs show the `cwd` parameter being used +5. ✅ Both streaming and non-streaming requests work correctly +6. ✅ No regression in existing ACP functionality +7. ✅ Code follows existing patterns and style conventions + +## Risk Assessment + +**Risk Level**: Low + +**Rationale**: +- Small, focused change to well-defined types and call sites +- Following established pattern from reference implementation +- No complex logic or state management changes +- Clear validation and error handling + +**Mitigation**: +- Comprehensive testing of both streaming and non-streaming paths +- Debug logging to verify correct behavior +- Validation to catch configuration errors early diff --git a/internal/acp/client.go b/internal/acp/client.go index c684511..97de8c2 100644 --- a/internal/acp/client.go +++ b/internal/acp/client.go @@ -316,8 +316,9 @@ func (c *KiroACPClient) SendMessage(ctx context.Context, req *MessageRequest) (* if sessionID == "" { c.mu.Lock() if c.sessionID == "" { - logging.Debug("creating new ACP session") + logging.Debug("creating ACP session", "cwd", c.config.Cwd) sessionResp, err := conn.NewSession(ctx, acp.NewSessionRequest{ + Cwd: c.config.Cwd, McpServers: []acp.McpServer{}, }) if err != nil { @@ -403,8 +404,9 @@ func (c *KiroACPClient) StreamMessage(ctx context.Context, req *MessageRequest) if sessionID == "" { c.mu.Lock() if c.sessionID == "" { - logging.Debug("creating new ACP session for streaming") + logging.Debug("creating streaming ACP session", "cwd", c.config.Cwd) sessionResp, err := conn.NewSession(streamCtx, acp.NewSessionRequest{ + Cwd: c.config.Cwd, McpServers: []acp.McpServer{}, }) if err != nil { diff --git a/internal/acp/types.go b/internal/acp/types.go index d42dcd9..87ff320 100644 --- a/internal/acp/types.go +++ b/internal/acp/types.go @@ -5,6 +5,8 @@ import ( "encoding/json" "errors" "io" + "os" + "path/filepath" "time" ) @@ -92,6 +94,9 @@ type ConnectionConfig struct { // Agent is the name of the agent to connect to Agent string `json:"agent,omitempty"` + // Cwd is the working directory for the ACP session (must be absolute path) + Cwd string `json:"cwd,omitempty"` + // MaxRetries is the maximum number of retry attempts MaxRetries int `json:"max_retries,omitempty"` @@ -148,9 +153,27 @@ type AuthProvider interface { // DefaultConnectionConfig returns default connection configuration func DefaultConnectionConfig() *ConnectionConfig { + cwd, err := os.Getwd() + if err != nil { + // Use absolute fallback when Getwd fails (e.g., dir deleted, permission issues) + cwd = os.TempDir() + } + + // Ensure absolute path unconditionally + if !filepath.IsAbs(cwd) { + absCwd, err := filepath.Abs(cwd) + if err != nil { + // Final fallback to temp dir if Abs fails + cwd = os.TempDir() + } else { + cwd = absCwd + } + } + return &ConnectionConfig{ KiroCLIPath: "kiro-cli", Agent: "", // Must be set explicitly by caller + Cwd: cwd, MaxRetries: 3, RetryDelay: 1 * time.Second, ConnectionTimeout: 30 * time.Second, @@ -172,6 +195,14 @@ func ValidateConnectionConfig(config *ConnectionConfig) error { return errors.New("kiro-cli path is required") } + if config.Cwd == "" { + return errors.New("working directory (cwd) is required") + } + + if !filepath.IsAbs(config.Cwd) { + return errors.New("working directory (cwd) must be an absolute path") + } + if config.ConnectionTimeout <= 0 || config.RequestTimeout <= 0 { return errors.New("timeouts must be positive") } diff --git a/internal/tui/planning_tab.go b/internal/tui/planning_tab.go index 8c55d4d..01887ab 100644 --- a/internal/tui/planning_tab.go +++ b/internal/tui/planning_tab.go @@ -3,6 +3,8 @@ package tui import ( "context" "fmt" + "os" + "path/filepath" "strings" "time" @@ -126,8 +128,32 @@ func NewPlanningTabWithSession(id, title string, styles *Styles, contextTracker // Initialize ACP client with provided client or create default if acpClient == nil { logging.Debug("creating default ACP client for planner agent", "tab_id", id) + + // Get current working directory for ACP session + cwd, err := os.Getwd() + if err != nil { + logging.Warn("failed to get current working directory, using fallback", "tab_id", id, "error", err) + // Use absolute fallback when Getwd fails + cwd = os.TempDir() + } + + // Ensure absolute path unconditionally + if !filepath.IsAbs(cwd) { + absCwd, err := filepath.Abs(cwd) + if err != nil { + // Final fallback to temp dir if Abs fails + logging.Warn("failed to convert to absolute path, using temp dir", "tab_id", id, "error", err) + cwd = os.TempDir() + } else { + cwd = absCwd + } + } + + logging.Debug("initializing ACP client with working directory", "tab_id", id, "cwd", cwd) + config := acp.DefaultConnectionConfig() config.Agent = "planner" + config.Cwd = cwd acpClient = acp.NewClient(config) } else { logging.Debug("using provided ACP client", "tab_id", id)