Skip to content

fix: accept legacy ACP optionIds for backward compatibility #436

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
RTCartist wants to merge 5 commits into from
+67 −3
Closed

fix: accept legacy ACP optionIds for backward compatibility #436

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1955dd9
fix: accept legacy ACP optionIds for backward compatibility
RTCartist Jun 4, 2026
0bbf492
chore: add changeset for legacy ACP optionId compatibility fix
RTCartist Jun 4, 2026
9bba513
fix: apply legacy optionId mapping only in the canonical branch
RTCartist Jun 4, 2026
8eb1847
chore(changeset): include @moonshot-ai/acp-adapter in legacy optionId…
RTCartist Jun 4, 2026
416f9b7
fix: preserve selectedLabel for legacy ACP optionIds
RTCartist Jun 4, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/legacy-acp-optionid-compat.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@moonshot-ai/kimi-code": patch
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 4, 2026

Choose a reason for hiding this comment

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

P2 Badge Include the changed ACP adapter in the changeset

The repo's changeset rules require source changes that affect a package's output to list that package, and to additionally list the CLI when an internal package change enters the CLI bundle. This commit changes packages/acp-adapter/src/approval.ts, but the changeset only bumps @moonshot-ai/kimi-code, so @moonshot-ai/acp-adapter will not be versioned/tracked for this fix; add "@moonshot-ai/acp-adapter": patch here as well.

Useful? React with 👍 / 👎.

---

Fix ACP custom client tools being auto-rejected when using legacy Python ACP SDK optionId values (`approve`, `approve_for_session`)
16 changes: 14 additions & 2 deletions packages/acp-adapter/src/approval.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,17 @@ export const APPROVE_ONCE_OPTION_ID = 'approve_once';
export const APPROVE_ALWAYS_OPTION_ID = 'approve_always';
export const REJECT_OPTION_ID = 'reject';

/**
* Legacy option ids sent by older ACP clients (e.g. Python ACP SDK 0.8.x).
* Mapped to the canonical ids so that clients built against the older SDK
* continue to work without modification.
*/
const LEGACY_OPTION_ID_MAP: Record<string, string> = {
approve: APPROVE_ONCE_OPTION_ID,
approve_for_session: APPROVE_ALWAYS_OPTION_ID,
reject: REJECT_OPTION_ID,
};

/**
* Phase 13.2 plan_review optionId namespace. Picked deliberately so the
* `plan_*` prefix never collides with the canonical `approve_*` /
Expand Down Expand Up @@ -151,10 +162,11 @@ export function permissionResponseToApprovalResponse(
if (response.outcome.outcome === 'cancelled') {
return { decision: 'cancelled' };
}
const optionId = response.outcome.optionId;
const rawOptionId = response.outcome.optionId;
if (req?.display.kind === 'plan_review') {
return mapPlanReviewOptionId(req.display, optionId);
return mapPlanReviewOptionId(req.display, rawOptionId);
}
const optionId = LEGACY_OPTION_ID_MAP[rawOptionId] ?? rawOptionId;
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot Jun 4, 2026

Choose a reason for hiding this comment

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

P3 Badge Preserve labels for legacy approvals

When a legacy client returns approve or approve_for_session, this normalization only affects the decision mapper; AcpSession.handleApproval still passes the original response into attachSelectedLabel, whose lookup compares the raw legacy id against the canonical options table and returns without adding a label. In the legacy-client path fixed here, approvals now execute but PermissionResult hooks/approval records lose the expected selectedLabel such as "Approve for this session", unlike the canonical path. Consider reusing the normalized id when attaching the label or teaching attachSelectedLabel the same legacy aliases.

Useful? React with 👍 / 👎.

switch (optionId) {
case APPROVE_ONCE_OPTION_ID:
return { decision: 'approved' };
Expand Down
15 changes: 15 additions & 0 deletions packages/acp-adapter/test/approval.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -175,6 +175,21 @@ describe('permissionResponseToApprovalResponse', () => {
expect(result).toEqual({ decision: 'rejected' });
});

it('maps legacy "approve" → { decision: approved } (Python ACP SDK compat)', () => {
const result = permissionResponseToApprovalResponse(undefined, {
outcome: { outcome: 'selected', optionId: 'approve' },
});
expect(result).toEqual({ decision: 'approved' });
expect(result.scope).toBeUndefined();
});

it('maps legacy "approve_for_session" → { decision: approved, scope: session } (Python ACP SDK compat)', () => {
const result = permissionResponseToApprovalResponse(undefined, {
outcome: { outcome: 'selected', optionId: 'approve_for_session' },
});
expect(result).toEqual({ decision: 'approved', scope: 'session' });
});

it('defensively maps an unknown optionId to { decision: rejected }', () => {
const result = permissionResponseToApprovalResponse(undefined, {
outcome: { outcome: 'selected', optionId: 'unknown_option_id' },
Expand Down