Skip to content

feat: mutlti root workspace support#1333

Open
faga295 wants to merge 3 commits into
MoonshotAI:mainfrom
faga295:feat/multi_root_workspace_support
Open

feat: mutlti root workspace support#1333
faga295 wants to merge 3 commits into
MoonshotAI:mainfrom
faga295:feat/multi_root_workspace_support

Conversation

@faga295

@faga295 faga295 commented Jul 3, 2026

Copy link
Copy Markdown

Related Issue

Resolve #(issue_number)
#1287

Problem

See linked issue

What changed

Implemented ACP multi-root workspace support for Zed and other ACP clients.
The adapter now advertises sessionCapabilities.additionalDirectories, so clients know Kimi Code supports multi-root workspaces during initialize. It also maps ACP additionalDirectories to Kimi Code SDK/core additionalDirs for session/new, session/load, and session/resume.

Checklist

  • I have read the CONTRIBUTING document.
  • I have linked a related issue, or explained the problem above.
  • I have added tests that prove my feature works.
  • Ran gen-changesets skill, or this PR needs no changeset.
  • Ran gen-docs skill, or this PR needs no doc update.

@changeset-bot

changeset-bot Bot commented Jul 3, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: 5b49c73

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@moonshot-ai/kimi-code Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 8718f5bdd2

ℹ️ About Codex in GitHub

Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".

Comment thread packages/acp-adapter/src/server.ts
@luckzylp

luckzylp commented Jul 3, 2026

Copy link
Copy Markdown

@faga295 Thanks a lot for putting this PR together! I was looking into the additionalDirectories feature and testing things out, and I noticed a couple of edge cases we might need to handle to fully align with the ACP specification https://agentclientprotocol.com/protocol/v1/session-setup#additional-workspace-roots.

I found two specific requirements in the spec that might need a bit more enforcement in the current implementation:

  1. Absolute Path Validation & Fail-Closed
    The spec mentions: "each additionalDirectories entry MUST be an absolute path". Right now, if a client sends a relative path or invalid data, it gets passed directly to the harness.
    It might be safer to validate this at the adapter layer and return a JSON-RPC invalid_params error if it's not an absolute path.

  2. Enforcing File System Boundaries
    The spec also mentions: "The session's effective root set is [cwd, ...additionalDirectories] . This root set SHOULD serve as a boundary for tool operations". It looks like
    maybeBuildAcpKaos currently doesn't receive the additionalDirectories to inject them into AcpKaos , which means the sandbox might not know about these new boundaries.

I've actually experimented with a way to handle both of these! We could add a validateAdditionalDirectories helper and pass the resolved roots into AcpKaos . Something like this:

   // 1. Validation helper
    export function validateAdditionalDirectories(dirs: unknown): string[] | undefined {
      if (dirs === undefined || dirs === null) return undefined;
      if (!Array.isArray(dirs)) throw RequestError.invalidParams({ additionalDirectories: dirs }, 'must be an array');
  
      for (let i = 0; i < dirs.length; i++) {
        const entry = dirs[i];
        if (typeof entry !== 'string' || entry === '') {
          throw RequestError.invalidParams({ additionalDirectories: dirs }, `entry at [${i}] must be a non-empty string`);
         }
         if (!path.isAbsolute(entry)) {
           throw RequestError.invalidParams({ additionalDirectories: dirs }, `entry at [${i}] must be an absolute path`);
         }
       }
       return dirs;
     }
  
     // 2. Injecting into AcpKaos (inside maybeBuildAcpKaos)
     const roots = [cwd, ...(additionalDirs ?? [])];
     const canonical = await resolveCanonicalRoots(roots);
     return new AcpKaos(this.conn, sessionId, innerKaos, canonical); // Passing canonical roots here

Would you be open to incorporating something like this into your PR? I'd be happy to help out or submit a patch to your branch if that makes it easier. Thanks again for driving this feature! 🚀

@faga295

faga295 commented Jul 3, 2026

Copy link
Copy Markdown
Author

@luckzylp Thanks a lot for your suggestions. You’re welcome to help fix this issue with us. I’ve granted you access to my fork repo, so feel free to push your changes there.

@faga295

faga295 commented Jul 3, 2026

Copy link
Copy Markdown
Author

@luckzylp For your second point, I’m not sure this is the right approach. Rather than passing additionalDirectories into kaos, I think we should update the workspace scope in core when an active session is matched.

@luckzylp

luckzylp commented Jul 3, 2026

Copy link
Copy Markdown

@faga295 This is merely my understanding of the ACP specification.

Updating the workspace scope in core is exactly what we are doing via harness.resumeSession({additionalDirs }) , which translates to active.setAdditionalDirs() in core-impl.ts .

However, we also must pass additionalDirectories into AcpKaos . AcpKaos serves as the strict security boundary for reverse-RPCs sent to the client (like Zed). It intercepts FS operations and runs assertPathInRoots before asking the editor to read/write files to prevent sandbox escapes.

Because AcpKaos is instantiated and injected before the session is created, it cannot dynamically query core for the current workspace scope. If we don't pass additionalDirectories to maybeBuildAcpKaos , its internal effectiveRoots will only contain [cwd] . Consequently, AcpKaos will forcefully reject any legal file reads to additionalDirectories with a KaosError , even if core considers them within the scope.

Therefore, injecting it into Kaos at creation time is necessary to keep the underlying FS abstraction's boundaries aligned with core 's scope.

Currently, I have attempted to push the code directly to your repository (but failed). I have submitted a pull request to your repository and I sincerely welcome everyone to review and discuss.

@faga295

faga295 commented Jul 3, 2026

Copy link
Copy Markdown
Author

If we don't pass additionalDirectories to maybeBuildAcpKaos , its internal effectiveRoots will only contain [cwd] . Consequently, AcpKaos will forcefully reject any legal file reads to additionalDirectories with a KaosError , even if core considers them within the scope.

@luckzylp Since AcpKaos doesn’t currently have effectiveRoots, I don’t think we should introduce the concept of roots just yet. Passing additionalDirs expands the file read/write scope to [workDir, ...additionalDirs], and I believe that behavior is expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants