From 9b2925e50fb5ffaecd3518a906fcabf81521be0e Mon Sep 17 00:00:00 2001 From: imccyu <276526105+imccyu@users.noreply.github.com> Date: Tue, 11 Aug 2026 23:01:20 +0800 Subject: [PATCH] fix: restore the credential-rejected branch and admit SRC absence by key Review follow-ups that are logic rather than documentation: - rpc.schema.ts had lost the credential-rejected branch while api-proxy.ts still returns that code, so a legitimate business error failed the client's response parse. Restore the branch and assert it. - rpc-schemas.spec.ts had lost the workspace list, archiveSession and insertSessionBefore cases along with the command schemas; those routes still ship, so restore their coverage. - An omitted SRC field is now recognized by an absent key instead of an undefined value, which makes the allowance assertExactArguments already granted reachable; an explicitly undefined field stays invalid input. A weak descriptor's undefined result rides the wire as an absent value, matching the envelope removal. - The chooser unmounts an already-created backend when the surface entry fails to load, and no longer reverses the captured id array in place. --- packages/api/gateway/src/index.ts | 9 +++++++ packages/api/gateway/tests/gateway.spec.ts | 13 ++++++++++ packages/host/apiproxy/src/api/rpc.schema.ts | 1 + .../host/apiproxy/tests/rpc-schemas.spec.ts | 25 ++++++++++++++++++- .../host/directory-picker-auto/src/index.ts | 19 ++++++++++---- .../tests/loader-composition.spec.ts | 19 +++++++++++++- 6 files changed, 79 insertions(+), 7 deletions(-) diff --git a/packages/api/gateway/src/index.ts b/packages/api/gateway/src/index.ts index 9b7ba6f2b1..1a060979a0 100644 --- a/packages/api/gateway/src/index.ts +++ b/packages/api/gateway/src/index.ts @@ -176,6 +176,10 @@ export class TypertGatewayService extends Service implements TypertGateway { if (request.signal?.aborted === true) throw new RemoteInvocationCancelled(endpoint, error) throw error } + // A weak descriptor declares no return type, so nothing returned is a void + // result and rides the wire as an absent value field. A strict descriptor + // keeps its schema: there, undefined has to be a declared result. + if (result === undefined && descriptor.result.mode !== 'strict') return result return decode(descriptor.result, result, 'result-invalid', endpoint, 'result') } @@ -405,6 +409,11 @@ export class TypertGatewayService extends Service implements TypertGateway { args: Readonly>, endpoint: string, ): Promise { + // An absent field reached assertExactArguments' allowance, so this parameter + // takes undefined; a present-but-undefined field is not JSON-safe input and + // still fails decode. Lookup ids are never omissible, so absence here only + // ever belongs to a json parameter. + if (!Object.hasOwn(args, parameter.wire)) return undefined const value = decode(parameter.codec, args[parameter.wire], 'input-invalid', endpoint, parameter.wire) if (parameter.source === 'json') return value const key = parameter.lookup diff --git a/packages/api/gateway/tests/gateway.spec.ts b/packages/api/gateway/tests/gateway.spec.ts index 8bc7817c95..00914d971f 100644 --- a/packages/api/gateway/tests/gateway.spec.ts +++ b/packages/api/gateway/tests/gateway.spec.ts @@ -788,6 +788,19 @@ describe('TypertGatewayService', () => { }), 'input-invalid') }) + it('admits an omitted SRC field and hands the Host method undefined', async () => { + const { ctx, service } = await setup() + // A weak descriptor reads parameter names from the JavaScript signature and + // cannot see which are optional, so an absent field is admitted; the case + // above keeps an explicitly undefined field rejected. + await expect(ctx.typertGateway.invoke({ + namespace: 'goals', + method: 'passthrough', + args: {}, + })).resolves.toBeUndefined() + expect(service.calls).toContain('passthrough') + }) + it('rejects cyclic SRC input and non-JSON SRC results', async () => { const { ctx, service } = await setup() const cyclic: { self?: unknown } = {} diff --git a/packages/host/apiproxy/src/api/rpc.schema.ts b/packages/host/apiproxy/src/api/rpc.schema.ts index 53f3c34ec2..177f0ffd29 100644 --- a/packages/host/apiproxy/src/api/rpc.schema.ts +++ b/packages/host/apiproxy/src/api/rpc.schema.ts @@ -61,6 +61,7 @@ export const rpcErrorSchema: z.ZodType = z.discriminatedUnion('code', z.object({ code: z.literal('settings-rejected'), message: z.string(), details: z.object({ ns: z.string() }) }), z.object({ code: z.literal('settings-not-exposed'), message: z.string(), details: z.object({ ns: z.string() }) }), z.object({ code: z.literal('settings-conflict'), message: z.string(), details: z.object({ ns: z.string(), expected: z.number(), actual: z.number() }) }), + z.object({ code: z.literal('credential-rejected'), message: z.string(), details: z.object({ ref: z.string() }) }), z.object({ code: z.literal('model-discovery-failed'), message: z.string(), details: z.object({ settingsNs: z.string(), baseURL: z.string().optional() }) }), z.object({ code: z.literal('title-invalid'), message: z.string(), details: z.object({ sessionId: z.string() }) }), z.object({ code: z.literal('fork-unavailable'), message: z.string(), details: z.object({ sessionId: z.string() }) }), diff --git a/packages/host/apiproxy/tests/rpc-schemas.spec.ts b/packages/host/apiproxy/tests/rpc-schemas.spec.ts index 0e8ab81ec2..c8e81964b9 100644 --- a/packages/host/apiproxy/tests/rpc-schemas.spec.ts +++ b/packages/host/apiproxy/tests/rpc-schemas.spec.ts @@ -20,8 +20,11 @@ import { hostListDirectoryRequestSchema, hostListDirectoryValueSchema, } from '../src/api/host.schema.ts' import { + workspaceArchiveSessionRequestSchema, workspaceArchiveSessionValueSchema, workspaceCreateRequestSchema, workspaceCreateValueSchema, workspaceIdSchema, workspaceDeleteRequestSchema, workspaceDeleteValueSchema, + workspaceInsertSessionBeforeRequestSchema, workspaceInsertSessionBeforeValueSchema, + workspaceListRequestSchema, workspaceListValueSchema, workspaceRenameRequestSchema, workspaceRenameValueSchema, workspaceViewSchema, } from '../src/api/workspace.schema.ts' import { skillEntrySchema, skillListRequestSchema, skillListValueSchema } from '../src/api/skills.schema.ts' @@ -73,6 +76,8 @@ describe('rpcErrorSchema', () => { expect(rpcErrorSchema.parse({ code: 'command-error', message: 'm', details: {} }).code).toBe('command-error') expect(rpcErrorSchema.parse({ code: 'unknown-command', message: 'm', details: {} }).code).toBe('unknown-command') expect(rpcErrorSchema.parse({ code: 'title-invalid', message: 'm', details: { sessionId: 's' } }).code).toBe('title-invalid') + // The credentials producer still emits this code, so the branch has to stay. + expect(rpcErrorSchema.parse({ code: 'credential-rejected', message: 'm', details: { ref: 'r' } }).code).toBe('credential-rejected') expect(rpcErrorSchema.parse({ code: 'internal', message: 'm', details: {} }).code).toBe('internal') }) @@ -344,11 +349,29 @@ describe('workspace domain schemas', () => { createdAt: '2026-07-25T00:00:00.000Z', updatedAt: '2026-07-25T00:00:00.000Z', } - it('validates ids and the view row', () => { + it('validates ids, the view row, and list request/value', () => { expect(workspaceIdSchema.parse('w1')).toBe('w1') expect(() => workspaceIdSchema.parse('')).toThrow() expect(workspaceViewSchema.parse(view).sessionIds).toEqual(['s1']) expect(() => workspaceViewSchema.parse({ ...view, sessionIds: 's1' })).toThrow() + expect(workspaceListRequestSchema.parse({})).toEqual({}) + expect(workspaceListValueSchema.parse({ items: [view], archivedSessionIds: ['s1'] }).items).toHaveLength(1) + expect(() => workspaceListValueSchema.parse({ items: [view] })).toThrow() + }) + + it('archiveSession request/value carry the id and the full updated set', () => { + expect(workspaceArchiveSessionRequestSchema.parse({ sessionId: 's1' }).sessionId).toBe('s1') + expect(() => workspaceArchiveSessionRequestSchema.parse({})).toThrow() + expect(workspaceArchiveSessionValueSchema.parse({ archivedSessionIds: ['s1', 's2'] }).archivedSessionIds) + .toEqual(['s1', 's2']) + expect(() => workspaceArchiveSessionValueSchema.parse({ archivedSessionIds: 's1' })).toThrow() + }) + + it('insertSessionBefore accepts an anchored and an anchorless move', () => { + expect(workspaceInsertSessionBeforeRequestSchema.parse({ workspaceId: 'w1', sessionId: 's1', beforeSessionId: 's2' }).beforeSessionId).toBe('s2') + expect(workspaceInsertSessionBeforeRequestSchema.parse({ workspaceId: 'w1', sessionId: 's1' }).beforeSessionId).toBeUndefined() + expect(() => workspaceInsertSessionBeforeRequestSchema.parse({ workspaceId: 'w1' })).toThrow() + expect(workspaceInsertSessionBeforeValueSchema.parse({ workspace: view }).workspace.workspaceId).toBe('w1') }) it('create requires a path', () => { diff --git a/packages/host/directory-picker-auto/src/index.ts b/packages/host/directory-picker-auto/src/index.ts index 764af27cdc..4f1ee77b98 100644 --- a/packages/host/directory-picker-auto/src/index.ts +++ b/packages/host/directory-picker-auto/src/index.ts @@ -72,11 +72,8 @@ export async function apply(ctx: Context): Promise { // backend lands first: the surface's browser half drives the capability // the backend registers. const ids: string[] = [] - for (const name of [BACKEND_PACKAGES[backend], SURFACE_PACKAGES[backend]]) { - ids.push(await ctx.loader.create({ name })) - } - return async () => { - for (const id of ids.reverse()) { + const unmount = async () => { + for (const id of [...ids].reverse()) { // Tree teardown (group.stop) can have removed the entry already; // nothing is left to unmount or await then. if (ctx.loader.store[id] === undefined) continue @@ -85,5 +82,17 @@ export async function apply(ctx: Context): Promise { await ctx.loader.remove(id) } } + try { + for (const name of [BACKEND_PACKAGES[backend], SURFACE_PACKAGES[backend]]) { + ids.push(await ctx.loader.create({ name })) + } + } catch (cause) { + // Setup owns the entries it created until it returns the disposer: leaving + // the backend mounted would make a retry collide with its own + // directoryPicker registration. + await unmount() + throw cause + } + return unmount }, 'directory-picker-auto: interaction entries') } diff --git a/packages/host/directory-picker-auto/tests/loader-composition.spec.ts b/packages/host/directory-picker-auto/tests/loader-composition.spec.ts index dc39c2ae77..4e19936300 100644 --- a/packages/host/directory-picker-auto/tests/loader-composition.spec.ts +++ b/packages/host/directory-picker-auto/tests/loader-composition.spec.ts @@ -88,7 +88,10 @@ afterEach(async () => { }) /** Write a two-row cordis.yml (webserver + chooser), then boot it through the real Loader. */ -async function loadComposition(bindHost: '127.0.0.1' | '0.0.0.0'): Promise<{ ctx: Context; configPath: string }> { +async function loadComposition( + bindHost: '127.0.0.1' | '0.0.0.0', + options: { failSurface?: boolean } = {}, +): Promise<{ ctx: Context; configPath: string }> { root = await mkdtemp(join(tmpdir(), 'dsh-directory-picker-auto-')) const configPath = join(root, 'cordis.yml') await writeFile(configPath, [ @@ -115,6 +118,9 @@ async function loadComposition(bindHost: '127.0.0.1' | '0.0.0.0'): Promise<{ ctx context.loader.internal = { version: 'v2', async import(specifier: string) { + if (options.failSurface === true && (specifier === NATIVE_SURFACE || specifier === BROWSE_SURFACE)) { + throw new Error(`surface import failed: ${specifier}`) + } if (!modules.has(specifier)) throw new Error(`unexpected Loader import: ${specifier}`) return modules.get(specifier) }, @@ -209,6 +215,17 @@ describe('real Loader composition', () => { expect(entryNames(ctx)).not.toContain(NATIVE_SURFACE) }) + it('unmounts the backend when the surface entry fails to load', { timeout: 60_000 }, async () => { + stubAttendedHost() + await expect(loadComposition('127.0.0.1', { failSurface: true })).rejects.toThrow(/surface import failed/) + + // Setup owns both entries until it returns its disposer, so a failed surface + // must take the mounted backend with it: otherwise a retry collides with the + // directoryPicker registration this backend already made. + expect(entryNames(context!)).not.toContain(NATIVE) + expect(context!.get('directoryPicker')).toBeUndefined() + }) + it('tolerates the mounted entry being removed by the tree before the chooser unloads', { timeout: 60_000 }, async () => { stubAttendedHost() const { ctx, configPath } = await loadComposition('127.0.0.1')