Skip to content
Merged
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
45 changes: 45 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,49 @@ describe('ConnectionRegistry.getSnapshot', () => {
registry.dispose();
}
});

it('finds 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.
// 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.
// findPendingPermission is a read-only locator; deletion is done by the
// owning connection on its own map key (AcpDispatcher.dropOwnPendingPermission).
connA.pending.delete(idA);
expect(registry.findPendingClientRequest(idA)).toBeUndefined();
} finally {
registry.dispose();
}
});
});
68 changes: 66 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,64 @@ export class ConnectionRegistry {
return conn;
}

findPendingClientRequest(id: string): PendingClientRequestRef | undefined {
Comment thread
chiga0 marked this conversation as resolved.
// Fast path: server-minted ids embed their originating connection
// (`_qwen_perm_<connectionId>_<counter>`, connectionId is a hyphenated
// `randomUUID()` with no underscores), so the owning connection is the
// substring before the last `_` — an O(1) lookup instead of scanning all
// connections on every client response.
if (id.startsWith('_qwen_perm_')) {
const body = id.slice('_qwen_perm_'.length);
const lastUnderscore = body.lastIndexOf('_');
if (lastUnderscore > 0) {
const conn = this.byId.get(body.slice(0, lastUnderscore));
const req = conn?.pending.get(id);
if (conn && req) return { conn, id, req };
}
}
// Fallback for client-chosen ids that don't match the server format.
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.
*
* 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, so this is a *read-only locator* for deriving a session /
* ownership from a wire `requestId`. To DELETE a resolved entry, act on the
* specific `conn`/map-key the caller already holds (see
* `AcpDispatcher.dropOwnPendingPermission`) — never delete by re-matching
* here, which could hit a sibling co-owner's entry.
*/
findPendingPermission(
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.
(sessionId === undefined || req.sessionId === sessionId)
) {
return { conn, id, req };
}
}
}
return undefined;
}

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