ACP session creation missing required cwd parameter causing "Improperly formed request" error - #253
Conversation
…properly formed request" error
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Fixes ACP protocol compliance for session creation by ensuring cwd (absolute working directory) is included in session/new requests, addressing the “Improperly formed request” failure in the planning tab.
Changes:
- Added
Cwdto ACPConnectionConfig, with defaulting and validation to ensure it is non-empty and absolute. - Updated both streaming and non-streaming
NewSessionrequests to includeCwd. - Initialized ACP client configuration in the planning tab to set/pass a working directory.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| internal/tui/planning_tab.go | Sets ACP client Cwd when creating the planner ACP client. |
| internal/acp/types.go | Adds ConnectionConfig.Cwd, defaults it, and validates it as required + absolute. |
| internal/acp/client.go | Passes Cwd into both NewSession call sites and logs it. |
| .kiro-krew/specs/issue-251-acp-session-missing-cwd-parameter.md | Adds design specification documenting the fix and acceptance criteria. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| 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 |
| 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 | ||
| } | ||
| } |
- Use os.TempDir() as absolute fallback when os.Getwd() fails - Make absolute path conversion unconditional with safety fallback - Change planning tab cwd logging from Info to Debug level Addresses Copilot review feedback on PR #253: - Prevents validation failures when working directory is unavailable - Avoids exposing local paths in normal logs - Ensures Connect() remains possible in edge cases Signed-off-by: Joseph Brinkman <joe.brinkman@improving.com>
Summary
Fixes the ACP session creation error in the planning tab by adding the required
cwd(current working directory) parameter toNewSessionRequestcalls.Problem
When sending a prompt in the planning tab, ACP session creation fails with an "Improperly formed request" error because the
NewSessionRequestis missing the requiredcwdparameter as specified in the ACP protocol.Solution
Added
cwdparameter to ACP connection configuration and ensured it's passed to allNewSessioncalls (both streaming and non-streaming).Changes Made
Files Modified
internal/acp/types.go- AddedCwd stringfield to ConnectionConfig struct with validationinternal/acp/client.go- Updated both streaming and non-streaming NewSession calls to include Cwd parameterinternal/tui/planning_tab.go- Initialize working directory when creating ACP client in planning tabKey Implementation Details
Cwd stringfield to ConnectionConfig struct with proper JSON tagDefaultConnectionConfig()to initialize Cwd with absolute path to current working directoryValidateConnectionConfig()to validate Cwd is non-empty and absolute pathNewSessioncalls (lines 320 and 407) in client.go to includeCwd: c.config.Cwdos.Getwd()with proper error handlingTesting
Validation
Verified that the implementation addresses all acceptance criteria:
Cwdfield to ConnectionConfigos.Getwd()as specifiedCloses #251