Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
6 changes: 6 additions & 0 deletions .changeset/legacy-acp-optionid-compat.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@moonshot-ai/acp-adapter": patch
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P3 Badge Drop the private package from the changeset

This changeset selects @moonshot-ai/acp-adapter, but that workspace is marked private in packages/acp-adapter/package.json and the repository release guidance says not to add release changesets for private internal packages, only the publishable surface packages. Leaving this entry causes the release PR to version/report an internal adapter package instead of only bumping the user-visible package affected by the fix.

Useful? React with 👍 / 👎.

"@moonshot-ai/kimi-code": patch
Copy link
Copy Markdown

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`)
25 changes: 22 additions & 3 deletions packages/acp-adapter/src/approval.ts
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

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 Expand Up @@ -310,7 +322,14 @@ export function attachSelectedLabel(
) {
return approval;
}
const matched = options.find((o) => o.optionId === outcome.optionId);
// Normalize legacy ids (Python ACP SDK 0.8.x sends `approve` /
// `approve_for_session` / `reject`) to their canonical equivalents
// before the lookup. Without this, legacy approvals are correctly
// routed by `permissionResponseToApprovalResponse` but drop the
// human-readable `selectedLabel` (e.g. "Approve for this session"),
// so PermissionResult hooks and approval records lose context.
const normalizedOptionId = LEGACY_OPTION_ID_MAP[outcome.optionId] ?? outcome.optionId;
const matched = options.find((o) => o.optionId === normalizedOptionId);
if (!matched) return approval;
return { ...approval, selectedLabel: matched.name };
}
24 changes: 24 additions & 0 deletions packages/acp-adapter/test/approval-display.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,30 @@ describe('attachSelectedLabel', () => {
expect(result).toEqual({ decision: 'rejected', selectedLabel: 'Reject' });
});

it('attaches "Approve once" when the legacy "approve" optionId is selected', () => {
const approval: ApprovalResponse = { decision: 'approved' };
const result = attachSelectedLabel(
{ outcome: { outcome: 'selected', optionId: 'approve' } },
approval,
options,
);
expect(result).toEqual({ decision: 'approved', selectedLabel: 'Approve once' });
});

it('attaches "Approve for this session" when the legacy "approve_for_session" optionId is selected', () => {
const approval: ApprovalResponse = { decision: 'approved', scope: 'session' };
const result = attachSelectedLabel(
{ outcome: { outcome: 'selected', optionId: 'approve_for_session' } },
approval,
options,
);
expect(result).toEqual({
decision: 'approved',
scope: 'session',
selectedLabel: 'Approve for this session',
});
});

it('returns the input unchanged when the optionId is unknown', () => {
const approval: ApprovalResponse = { decision: 'rejected' };
const result = attachSelectedLabel(
Expand Down
15 changes: 15 additions & 0 deletions packages/acp-adapter/test/approval.test.ts
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