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
52 changes: 52 additions & 0 deletions packages/sdk-typescript/src/daemon/DaemonHttpError.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,3 +23,55 @@ export class DaemonHttpError extends Error {
this.body = body;
}
}

// Kept local (instead of reusing `isRecord` from `acpTransportUtils.ts` or
// `ui/utils.ts`) so this leaf module stays dependency-free: those modules
// pull the ACP route table / UI helpers into the budgeted browser bundles.
function getErrorBodyRecord(
body: unknown,
): Record<string, unknown> | undefined {
return typeof body === 'object' && body !== null && !Array.isArray(body)
? (body as Record<string, unknown>)
: undefined;
}
Comment on lines +30 to +36

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] getErrorBodyRecord duplicates existing isRecord type guard in the same package — Concrete cost: the codebase now has two implementations of the same non-null-non-array-object check in the same module hierarchy. If the check ever needs updating, the private getErrorBodyRecord in DaemonHttpError.ts will be missed.

Suggested change
function getErrorBodyRecord(
body: unknown,
): Record<string, unknown> | undefined {
return typeof body === 'object' && body !== null && !Array.isArray(body)
? (body as Record<string, unknown>)
: undefined;
}
import { isRecord } from './acpTransportUtils.js';
// ...
// Replace getErrorBodyRecord(error.body) with:
isRecord(error.body) ? error.body : undefined
中文说明

getErrorBodyRecord 是同一包中已有 isRecord 类型守卫的重复实现。建议删除 getErrorBodyRecord 并导入 acpTransportUtils.ts 中的 isRecord

— deepseek-v4-flash via Qwen Code /review (v0.21.8)

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Declined: the duplication is deliberate. DaemonHttpError.ts is an intentionally dependency-free leaf module (see its header comment) — RestSseTransport imports it, and it sits in the graph of both budgeted browser bundles that packages/sdk-typescript/scripts/build.js byte-limits (MAX_DAEMON_BROWSER_BUNDLE_BYTES and the opt-in daemon/transports bundle). Importing isRecord from acpTransportUtils.js would transitively pull in the ~1000-line ACP route table (acpTransportUtils imports ROUTE_TABLE from acpRouteTable.js), and the ui/utils.js copy would pull in the UI rendering helpers — each is excluded from one of those bundles today precisely to keep them out. A 4-line non-null-non-array-object check is stable enough that a local private copy is the bundle-correct choice; a comment on getErrorBodyRecord now records this constraint so the next person weighing dedup sees the trade-off.

中文说明

拒绝:该重复是有意为之。DaemonHttpError.ts 是一个刻意保持零依赖的叶子模块(见其头部注释)——RestSseTransport 导入它,且它位于两个受字节预算约束的浏览器 bundle 的模块图中(packages/sdk-typescript/scripts/build.js 中的 MAX_DAEMON_BROWSER_BUNDLE_BYTES 与可选的 daemon/transports bundle)。从 acpTransportUtils.js 导入 isRecord 会连带引入约 1000 行的 ACP 路由表(acpTransportUtilsacpRouteTable.js 导入 ROUTE_TABLE);而 ui/utils.js 中的副本则会引入 UI 渲染辅助函数——这两者今天正是为了不进这些 bundle 而被隔离的。4 行的"非 null、非数组对象"判断足够稳定,本地私有副本是符合 bundle 预算的正确选择;现已在 getErrorBodyRecord 上添加注释记录该约束,便于后续考虑去重的人了解权衡。


/**
* Type guard for the daemon's `GET /session/:id/subagents/:toolCallId` 404
* contract: `{ code: 'session_not_found', sessionId, toolCallId? }`. Pass
* `toolCallId` to require the body to identify that specific missing agent
* (a session-level 404 carries no identifying `toolCallId`); omit it to
* accept both.
*/
export function isSubagentSessionNotFound(
error: unknown,
toolCallId?: string,
): boolean {
if (!(error instanceof DaemonHttpError) || error.status !== 404) {
return false;
}
const body = getErrorBodyRecord(error.body);
if (body?.['code'] !== 'session_not_found') return false;
return toolCallId === undefined || body['toolCallId'] === toolCallId;
}

/**
* Type guard for the session-level variant of that same 404 contract: the
* daemon could not find the parent session itself, so the body carries
* `code: 'session_not_found'` with no identifying `toolCallId` (an
* explicitly `null` id is treated the same as an absent one).
*
* A missing parent session is not the only producer: a multi-workspace
* daemon answers this same shape while the owning workspace entry is
* merely not active (for example draining before removal, or transitioning
* to a replacement runtime), which the daemon treats as reversible. Treat
* this error as recoverable, not as proof the session is permanently gone.
*/
export function isSessionLevelNotFound(error: unknown): boolean {
if (!(error instanceof DaemonHttpError) || error.status !== 404) {
return false;
}
const body = getErrorBodyRecord(error.body);
if (body?.['code'] !== 'session_not_found') return false;
const toolCallId = body['toolCallId'];
return toolCallId === undefined || toolCallId === null;
}
4 changes: 4 additions & 0 deletions packages/sdk-typescript/src/daemon/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,10 @@ export {
type RestoreSessionRequest,
type SubscribeOptions,
} from './DaemonClient.js';
export {
isSessionLevelNotFound,
isSubagentSessionNotFound,
} from './DaemonHttpError.js';
// Transport abstraction layer
export { DaemonTransportClosedError } from './DaemonTransport.js';
export type {
Expand Down
161 changes: 161 additions & 0 deletions packages/sdk-typescript/test/unit/isSubagentSessionNotFound.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,161 @@
/**
* @license
* Copyright 2025 Qwen Team
* SPDX-License-Identifier: Apache-2.0
*/

import { describe, expect, it } from 'vitest';
import {
DaemonHttpError,
isSessionLevelNotFound,
isSubagentSessionNotFound,
} from '../../src/daemon/DaemonHttpError.js';

const missingAgentBody = {
code: 'session_not_found',
sessionId: 'session-1',
toolCallId: 'call-1',
};

describe('isSubagentSessionNotFound', () => {
it('matches a 404 whose body identifies the missing agent', () => {
expect(
isSubagentSessionNotFound(
new DaemonHttpError(404, missingAgentBody, 'not found'),
'call-1',
),
).toBe(true);
});

it('matches a session-level 404 when no toolCallId is required', () => {
expect(
isSubagentSessionNotFound(
new DaemonHttpError(
404,
{ code: 'session_not_found', sessionId: 'session-1' },
'not found',
),
),
).toBe(true);
});

it.each([
['non-DaemonHttpError', new Error('not found'), 'call-1'],
[
'non-404 status',
new DaemonHttpError(500, missingAgentBody, 'server error'),
'call-1',
],
[
'missing code',
new DaemonHttpError(404, { toolCallId: 'call-1' }, 'not found'),
'call-1',
],
[
'wrong code',
new DaemonHttpError(
404,
{ ...missingAgentBody, code: 'workspace_not_found' },
'not found',
),
'call-1',
],
[
'missing toolCallId in body',
new DaemonHttpError(
404,
{ code: 'session_not_found', sessionId: 'session-1' },
'not found',
),
'call-1',
],
[
'null toolCallId in body',
new DaemonHttpError(
404,
{ code: 'session_not_found', sessionId: 'session-1', toolCallId: null },
'not found',
),
'call-1',
],
[
'mismatched toolCallId',
new DaemonHttpError(404, missingAgentBody, 'not found'),
'call-other',
],
])('rejects %s', (_label, error, toolCallId) => {
expect(isSubagentSessionNotFound(error, toolCallId as string)).toBe(false);
});

it('rejects non-object bodies', () => {
expect(
isSubagentSessionNotFound(
new DaemonHttpError(404, 'session_not_found', 'not found'),
'call-1',
),
).toBe(false);
});
});

describe('isSessionLevelNotFound', () => {
it('matches a 404 whose body has no toolCallId', () => {
expect(
isSessionLevelNotFound(
new DaemonHttpError(
404,
{ code: 'session_not_found', sessionId: 'session-1' },
'not found',
),
),
).toBe(true);
});

it('matches a 404 whose body carries a null toolCallId', () => {
expect(
isSessionLevelNotFound(
new DaemonHttpError(
404,
{
code: 'session_not_found',
sessionId: 'session-1',
toolCallId: null,
},
'not found',
),
),
).toBe(true);
});

it('rejects an agent-level 404', () => {
expect(
isSessionLevelNotFound(
new DaemonHttpError(404, missingAgentBody, 'not found'),
),
).toBe(false);
});

it('rejects a 404 whose body carries a different code', () => {
expect(
isSessionLevelNotFound(
new DaemonHttpError(
404,
{ code: 'workspace_not_found', sessionId: 'session-1' },
'not found',
),
),
).toBe(false);
});

it('rejects non-404 and non-matching errors', () => {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

[Suggestion] isSessionLevelNotFound has no negative case for a 404 whose body carries a different code and no toolCallId; its sibling guard has a 'wrong code' row, but this guard does not. The mutation return !('toolCallId' in body) (dropping the code check) keeps all three existing tests green — verified on this commit. The distinguishing input new DaemonHttpError(404, {code:'workspace_not_found', sessionId:'session-1'}, 'x') is false today, true under that mutant. — Failure scenario: downstream in useMessages, that branch is the no-grace immediate-terminal path, so an over-matching guard would flip pending background-agent cards to failed on any future/variant 404 shape lacking toolCallId. Suggested fix: add one rejection case to the isSessionLevelNotFound describe:

expect(
  isSessionLevelNotFound(
    new DaemonHttpError(
      404,
      { code: 'workspace_not_found', sessionId: 'session-1' },
      'not found',
    ),
  ),
).toBe(false);
中文说明

isSessionLevelNotFound 缺少针对"404 且响应体携带其他 code、没有 toolCallId"的负例;它的姊妹守卫有 'wrong code' 用例行,本守卫没有。变异 return !('toolCallId' in body)(去掉 code 检查)能让现有 3 个测试全部保持绿色——已在本提交上验证。区分性输入 new DaemonHttpError(404, {code:'workspace_not_found', sessionId:'session-1'}, 'x') 今天返回 false,在该变异下返回 true。— 失败场景:在下游 useMessages 中,该分支是无宽限的立即终态路径,过度匹配的守卫会让任何未来/变体的、不带 toolCallId 的 404 形状把 pending 的后台 agent 卡片翻转为 failed。建议修复:在 isSessionLevelNotFound 的 describe 中补充上面的拒绝用例。

— qwen3.8-max via Qwen Code /review (v0.21.10)

expect(
isSessionLevelNotFound(
new DaemonHttpError(
500,
{ code: 'session_not_found', sessionId: 'session-1' },
'server error',
),
),
).toBe(false);
expect(isSessionLevelNotFound(new Error('not found'))).toBe(false);
});
});
Loading
Loading