Skip to content
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
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
44 changes: 44 additions & 0 deletions packages/cli/src/serve/acp-http/connection-registry.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -859,4 +859,48 @@ describe('ConnectionRegistry.getSnapshot', () => {
registry.dispose();
}
});

it('finds and deletes pending permissions across connections', () => {
const registry = new ConnectionRegistry();
try {
const connA = registry.create(false);
const connB = registry.create(false);
expect(connA).toBeDefined();
expect(connB).toBeDefined();
if (!connA || !connB) return;

const idA = connA.nextId();
const idB = connB.nextId();
expect(idA).not.toBe(idB);
Comment thread
chiga0 marked this conversation as resolved.
Resolved
// Pin the connection-qualified format `_qwen_perm_<connectionId>_<N>` —
// it's the collision-prevention guarantee of this change, so a
// regression to the old `_qwen_perm_<N>` format must fail here, not just
// an "ids differ" check that the old format would also pass.
expect(idA).toMatch(/^_qwen_perm_.+_1$/);
expect(idA).toContain(connA.connectionId);
expect(idB).toContain(connB.connectionId);

connA.pending.set(idA, {
sessionId: 'sess-1',
bridgeRequestId: 'perm-1',
kind: 'permission',
});

expect(registry.findPendingClientRequest(idA)?.conn).toBe(connA);
expect(registry.findPendingPermission('perm-1', 'sess-1')?.id).toBe(idA);
// The `sessionId === undefined` branch (relied on by dispatch's
// `session/permission` handler when no sessionId is supplied) matches on
// requestId alone, while a mismatched sessionId must not match.
expect(registry.findPendingPermission('perm-1')?.id).toBe(idA);
expect(
registry.findPendingPermission('perm-1', 'wrong-session'),
).toBeUndefined();

Comment thread
chiga0 marked this conversation as resolved.
Resolved
registry.deletePendingPermission('perm-1', 'sess-1');

expect(registry.findPendingClientRequest(idA)).toBeUndefined();
} finally {
registry.dispose();
}
});
});
62 changes: 60 additions & 2 deletions packages/cli/src/serve/acp-http/connection-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -145,6 +145,12 @@ export interface PendingClientRequest {
kind: 'permission';
}

export interface PendingClientRequestRef {
conn: AcpConnection;
id: string;
req: PendingClientRequest;
}

export interface AcpConnectionDiagnostic {
connectionIdPrefix: string;
fromLoopback: boolean;
Expand Down Expand Up @@ -254,13 +260,13 @@ export class AcpConnection {

/**
* Allocate a fresh JSON-RPC id for an agent→client request. STRING-typed
* (`_qwen_perm_N`) so it can never collide with a client-originated id —
* (`_qwen_perm_<conn>_N`) so it can never collide with a client-originated id —
* JSON-RPC 2.0 permits clients to use any number (incl. negatives) or
* string, so a numeric namespace wasn't actually safe.
*/
nextId(): string {
this.idCounter += 1;
return `_qwen_perm_${this.idCounter}`;
return `_qwen_perm_${this.connectionId}_${this.idCounter}`;
}

touch(): void {
Expand Down Expand Up @@ -901,6 +907,58 @@ export class ConnectionRegistry {
return conn;
}

findPendingClientRequest(id: string): PendingClientRequestRef | undefined {
Comment thread
chiga0 marked this conversation as resolved.
Resolved
for (const conn of this.byId.values()) {
const req = conn.pending.get(id);
if (req) return { conn, id, req };
}
return undefined;
}

/**
* Locate a pending permission entry matching `requestId` (a bridge
* `bridgeRequestId`, i.e. a per-request `randomUUID()`) and optionally
* `sessionId`, returning the first match. {@link deletePendingPermission}
* delegates here so the matching predicate lives in one place.
*
* NOTE: `requestId` is unique per *request*, not per *pending entry*. The
* per-entry unique id is the `conn.pending` map key
* (`_qwen_perm_<connectionId>_N`), which is NOT what is matched here. A
* `permission_request` is delivered to every live subscriber of its session,
* so when connections co-own a session (multi-client attach) each mints its
* own entry sharing the same `bridgeRequestId`. More than one entry can
* therefore match; callers that must act on a *specific* entry (e.g. the
* resolve site) should delete by the `conn`/map-key they already hold rather
* than re-matching here.
*/
findPendingPermission(
Comment thread
chiga0 marked this conversation as resolved.
Resolved
requestId: string,
sessionId?: string,
): PendingClientRequestRef | undefined {
for (const conn of this.byId.values()) {
for (const [id, req] of conn.pending) {
if (
req.kind === 'permission' &&
req.bridgeRequestId === requestId &&
Comment thread
chiga0 marked this conversation as resolved.
Resolved
(sessionId === undefined || req.sessionId === sessionId)
) {
return { conn, id, req };
}
}
}
return undefined;
}

// Argument order matches `findPendingPermission` (requestId first) so the two
// can never be called with swapped string args — a swap would silently match
// nothing and leak the entry until teardown, with no type error to catch it.
deletePendingPermission(requestId: string, sessionId: string): void {
Comment thread
chiga0 marked this conversation as resolved.
Resolved
Comment thread
chiga0 marked this conversation as resolved.
Resolved
const match = this.findPendingPermission(requestId, sessionId);
if (match) {
match.conn.pending.delete(match.id);
}
}

delete(connectionId: string): boolean {
const conn = this.byId.get(connectionId);
if (!conn) return false;
Expand Down
Loading
Loading