Add the Agent Factories authoring surface

[MRayermannMSFT]

What

Adds the Agent Factories authoring surface to the Node.js SDK. A factory is a trusted extension-authored closure that orchestrates a fleet of subagents and is invoked by name: an extension declares one with defineFactory, registers it through joinSession({ factories }), and runs it with session.factory.run(...). Only metadata crosses to the runtime — the closure stays in the extension process and is invoked back over a reverse RPC, where it gets primitives for spawning subagents, journaling results so a resumed run replays completed work for free, composing work in parallel or as a pipeline, and reporting progress.

Runs resolve with a durable envelope for every outcome rather than throwing, and waitForRun settles once a run reaches a terminal status, so callers stay correct when execution later moves to background-only. The one generator change narrows an over-broad opaque-JSON conversion; without it a factory result generates as an object alone, which a non-object or void result cannot satisfy.

Everything is dark behind the runtime's agent_factories flag plus its billing gate, and every public type is marked @experimental.

Why

The runtime half of Agent Factories already merged (github/copilot-agent-runtime#12953 and #13077), but the SDK half has never landed — the original PR was closed unmerged — so the shipped runtime currently has no authoring client and the feature cannot work end to end for any SDK consumer. Until this lands, the factory E2E suite can only run against a locally injected SDK build rather than the published package.

[Copilot]

Pull request overview

Adds the experimental Agent Factories authoring surface to the Node.js SDK, including registration, execution, orchestration, persistence, observability, and documentation.

Changes:

• Adds factory definition, registration, execution, resume, cancellation, and observability APIs.
• Generates supporting JSON-RPC and session-event types with recursive JsonValue support.
• Adds extensive factory tests and authoring documentation.

Show a summary per file

File	Description
scripts/codegen/typescript.ts	Generates recursive opaque JSON types.
nodejs/test/typescript-codegen.test.ts	Tests opaque JSON generation.
nodejs/test/session-event-types.test.ts	Checks factory and JSON exports.
nodejs/test/factory.test.ts	Tests factory behavior and APIs.
nodejs/test/extension.test.ts	Updates extension resume tests.
nodejs/src/types.ts	Adds factory metadata and limit types.
nodejs/src/sessionFsProvider.ts	Adds SQLite transaction support.
nodejs/src/session.ts	Implements factory execution and orchestration.
nodejs/src/index.ts	Exports factory APIs.
nodejs/src/generated/session-events.ts	Adds factory events and JSON types.
nodejs/src/generated/rpc.ts	Adds generated factory RPC contracts.
nodejs/src/factory.ts	Defines the public factory authoring surface.
nodejs/src/extension.ts	Registers factories through joinSession.
nodejs/src/client.ts	Sends registration metadata and installs handlers.
nodejs/docs/factories.md	Documents factory authoring and operation.
nodejs/docs/extensions.md	Links factory documentation.

Review details

• Files reviewed: 14/16 changed files
• Comments generated: 7
• Review effort level: Medium

[Copilot]

Review details

Still open (7)

• 🟠 nodejs/src/types.ts#L840 — This broadens elicitation responses beyond the values that the adjacent ElicitationSchemaField…
• 🟠 nodejs/src/extension.ts#L56 — The extension entry point has the same incomplete public surface: SessionFactoryApi.listRuns(),…
• 🟠 nodejs/src/index.ts#L181 — The new observability methods expose named DTOs (FactoryRunSummary, FactoryRunDetail,…
• 🟠 nodejs/src/factory.ts#L352 — The validated metadata is retained by reference and only the outer handle is frozen. Callers can…
• 🟠 nodejs/src/session.ts#L1939 — This strict validator accepts -0, but JSON serialization changes it to 0…
• 🟠 nodejs/src/session.ts#L274 — The RPC promise has already been created—and therefore the request already sent—before this…
• 🟠 nodejs/src/sessionFsProvider.ts#L56 — Making transaction required is a breaking change to the existing public SessionFsSqliteProvider…

Comments suppressed due to low confidence (2)

nodejs/src/sessionFsProvider.ts:56

• Making transaction required breaks every existing SessionFsProvider SQLite implementation in this repository: for example nodejs/test/session_fs_adapter.test.ts:62, nodejs/test/e2e/session_fs.e2e.test.ts:287, and nodejs/test/e2e/session_fs_sqlite.e2e.test.ts:203 implement only query and exists, so TypeScript compilation fails. Update all implementations (and transaction behavior tests), or make this capability optional with an adapter fallback to preserve compatibility.

    transaction(
        statements: SessionFsSqliteTransactionStatement[]
    ): Promise<SessionFsSqliteQueryResult[]>;

nodejs/src/types.ts:840

• This broadens elicitation responses beyond the primitive schema declared immediately above: ElicitationSchemaField can only produce strings, numbers, booleans, or string arrays, while JsonValue also permits null, objects, and mixed/nested arrays. Those values cannot satisfy the advertised MCP primitive form contract, so invalid handler responses now type-check. Keep the primitive value union here; factory JSON values should not alter this unrelated API.

 * JSON field value in an elicitation result.
 */
export type ElicitationFieldValue = JsonValue;

• Files reviewed: 15/17 changed files
• Comments generated: 4
• Review effort level: Medium

[Copilot]

Review details

Files not reviewed (6)

• go/rpc/zrpc.go: Generated file
• go/rpc/zrpc_encoding.go: Generated file
• go/rpc/zsession_encoding.go: Generated file
• go/rpc/zsession_events.go: Generated file
• go/zsession_events.go: Generated file
• nodejs/package-lock.json: Generated file

Resolved since last review (6)

• ✅ nodejs/src/session.ts#L182 — Resolved by Copilot - pipeline() similarly converts hard RPC failures from a stage's agent() call into null,…
• ✅ nodejs/src/session.ts#L154 — Resolved by Copilot - parallel() swallows every non-abort rejection, including ResponseError/ConnectionError from…
• ✅ scripts/codegen/typescript.ts#L293 — Resolved by Copilot - This blanket replacement collapses every marked schema to JsonValue, including already-structured…
• ✅ nodejs/src/extension.ts#L56 — Resolved by Copilot - The extension entry point has the same incomplete public surface: SessionFactoryApi.listRuns(),…
• ✅ nodejs/src/index.ts#L181 — Resolved by Copilot - The new observability methods expose named DTOs (FactoryRunSummary, FactoryRunDetail,…
• ✅ nodejs/src/sessionFsProvider.ts#L56 — Resolved by Copilot - Making transaction required is a breaking change to the existing public SessionFsSqliteProvider…

Still open (5)

• 🔴 nodejs/src/session.ts#L1192 — Indexing controllers only by runId loses the previous controller when overlapping execution…
• 🟠 nodejs/src/types.ts#L840 — This broadens elicitation responses beyond the values that the adjacent ElicitationSchemaField…
• 🟠 nodejs/src/factory.ts#L352 — The validated metadata is retained by reference and only the outer handle is frozen. Callers can…
• 🟠 nodejs/src/session.ts#L1939 — This strict validator accepts -0, but JSON serialization changes it to 0…
• 🟠 nodejs/src/session.ts#L274 — The RPC promise has already been created—and therefore the request already sent—before this…

Comments suppressed due to low confidence (6)

nodejs/src/session.ts:1215

• Overlapping factory.execute attempts can share a run ID (the new overlapping-attempts test explicitly exercises this), but this assignment overwrites the previous controller. A subsequent factory.abort therefore aborts only the newest attempt while the stale factory body can continue running and performing local side effects. Track all controllers per run ID and abort every active attempt.

                const controller = new AbortController();
                self.factoryAbortControllers.set(params.runId, controller);

nodejs/src/factory.ts:348

• Validation is only performed here, but the returned handle exposes the same mutable definition.meta object. A caller can mutate phases or limits after defineFactory and before joinSession, bypassing these checks; registration then serializes the invalid metadata. Snapshot/freeze the validated metadata, or revalidate it during registration.

    validateLimits(definition.meta);
    validatePhases(definition.meta);

    const stored: StoredFactory = {
        meta: definition.meta,
        run: definition.run,
    };
    const handle = Object.freeze({ meta: definition.meta }) as FactoryHandle<TArgs, TResult>;

nodejs/src/session.ts:1954

• The numeric branch accepts negative zero, but JSON serialization converts -0 to 0. That violates the stated lossless validation guarantee for factory results and journal replay. Detect Object.is(current, -0) and either reject it with a dedicated category or define an explicit normalization policy.

        if (typeof current === "number") {
            if (!Number.isFinite(current)) {

scripts/codegen/typescript.ts:326

• This replacement discards non-structural schema metadata such as description, deprecated, and visibility tags. The generated diff already loses documentation from opaque fields such as CitationReference.providerMetadata and HookStartData.input. Preserve the rewritten node and only replace its type semantics after removing the opaque marker.
  nodejs/src/factory.ts:197
• FactoryResumeErrorCode is exported as part of the new public authoring API but lacks the @experimental marker promised for every public factory type in the PR description. Add the same experimental notice used by the surrounding factory types.

/** Machine-readable pre-execution factory resume failure. */
export type FactoryResumeErrorCode =

nodejs/src/sessionFsProvider.ts:240

• The adapter now has several new behavioral branches—unsupported providers, parameter normalization, successful transactions, and three error classifications—but none are covered in the existing nodejs/test/session_fs_adapter.test.ts suite that tests the other adapter operations. Add focused tests, especially for busy/locked versus post-commit-ambiguous classification, to prevent unsafe retry behavior.

        sqliteTransaction: async ({ statements }) => {
            if (!provider.sqlite?.transaction) {
                return {
                    results: [],
                    error: {

• Files reviewed: 17/32 changed files
• Comments generated: 0 new
• Review effort level: Medium

[Copilot]

Review details

Files not reviewed (6)

• go/rpc/zrpc.go: Generated file
• go/rpc/zrpc_encoding.go: Generated file
• go/rpc/zsession_encoding.go: Generated file
• go/rpc/zsession_events.go: Generated file
• go/zsession_events.go: Generated file
• nodejs/package-lock.json: Generated file

Still open (5)

• 🔴 nodejs/src/session.ts#L1192 — Indexing controllers only by runId loses the previous controller when overlapping execution…
• 🟠 nodejs/src/types.ts#L840 — This broadens elicitation responses beyond the values that the adjacent ElicitationSchemaField…
• 🟠 nodejs/src/factory.ts#L352 — The validated metadata is retained by reference and only the outer handle is frozen. Callers can…
• 🟠 nodejs/src/session.ts#L1939 — This strict validator accepts -0, but JSON serialization changes it to 0…
• 🟠 nodejs/src/session.ts#L274 — The RPC promise has already been created—and therefore the request already sent—before this…

Comments suppressed due to low confidence (4)

nodejs/src/session.ts:1296

• Overlapping factory.execute calls for the same run are explicitly supported, but this assignment replaces the earlier controller. A later factory.abort (and session cleanup) therefore aborts only the newest attempt; an older extension body can keep running and performing side effects indefinitely. Track all controllers per run (or key them by execution token and abort every controller for the run), and remove only the completing attempt.

                const controller = new AbortController();
                self.factoryAbortControllers.set(params.runId, controller);

nodejs/src/sessionFsProvider.ts:296

• SQLite extended busy/locked result codes are misclassified as fatal. Standard codes such as SQLITE_BUSY_SNAPSHOT (numeric 517) and SQLITE_LOCKED_SHAREDCACHE (262) retain 5 or 6 only in the low byte, so providers exposing extended names or numbers will skip the intended retry path. Match the SQLITE_BUSY*/SQLITE_LOCKED* families and mask numeric extended codes to their base code.

    const code = "code" in err ? err.code : undefined;
    const errcode = "errcode" in err ? err.errcode : undefined;
    return code === "SQLITE_BUSY" ||
        code === "SQLITE_LOCKED" ||
        code === "EBUSY" ||
        errcode === 5 ||
        errcode === 6
        ? "busyOrLocked"
        : "fatal";

nodejs/src/factory.ts:223

• FactoryResumeErrorCode is a newly public Agent Factories type but is the only factory-specific public declaration here without @experimental, contrary to the PR's stated contract that every public type is marked experimental. Add the same experimental JSDoc used by the surrounding factory API types.

/** Machine-readable pre-execution factory resume failure. */
export type FactoryResumeErrorCode =

go/rpc/zrpc_encoding.go:1157

• Clear Failure when the property is absent. As written, unmarshalling a terminal without failure into a reused FactoryRunTerminal leaves the previous failure attached, while the other optional fields are reset from raw; callers can therefore observe a stale failure from an earlier payload. Please update scripts/codegen/go.ts to initialize this union field to nil before conditionally decoding it, then regenerate this file.

• Files reviewed: 17/32 changed files
• Comments generated: 0 new
• Review effort level: Medium

[Copilot]

Review details

Files not reviewed (6)

• go/rpc/zrpc.go: Generated file
• go/rpc/zrpc_encoding.go: Generated file
• go/rpc/zsession_encoding.go: Generated file
• go/rpc/zsession_events.go: Generated file
• go/zsession_events.go: Generated file
• nodejs/package-lock.json: Generated file

Resolved since last review (4)

• ✅ nodejs/src/session.ts#L1192 — Resolved by Copilot - Indexing controllers only by runId loses the previous controller when overlapping execution…
• ✅ nodejs/src/factory.ts#L352 — Resolved by Copilot - The validated metadata is retained by reference and only the outer handle is frozen. Callers can…
• ✅ nodejs/src/session.ts#L1939 — Resolved by Copilot - This strict validator accepts -0, but JSON serialization changes it to 0…
• ✅ nodejs/src/session.ts#L274 — Resolved by Copilot - The RPC promise has already been created—and therefore the request already sent—before this…

Still open (1)

• 🟠 nodejs/src/types.ts#L840 — This broadens elicitation responses beyond the values that the adjacent ElicitationSchemaField…

Comments suppressed due to low confidence (3)

scripts/codegen/typescript.ts:325

• Replacing an opaque node with a fresh object drops all schema metadata on that node, including its description and marker-derived JSDoc tags. This is already removing public field documentation such as CitationReference.providerMetadata and the @experimental text on ToolExecutionCompleteData.mcpMeta from the regenerated output. Preserve the rewritten node, remove only x-opaque-json, and add tsType so code generation retains those annotations.
  nodejs/src/sessionFsProvider.ts:236
• The new transaction adapter path is not exercised by nodejs/test/session_fs_adapter.test.ts, although that suite covers the other adapter operations. Please add cases for successful parameter normalization, unsupported providers, and each retry classification; a regression here can incorrectly retry a possibly committed transaction or mishandle a rolled-back busy error.

        sqliteTransaction: async ({ statements }) => {

nodejs/src/factory.ts:223

• This exported Agent Factories type is the only factory-specific public type here without the promised @experimental annotation, so generated API documentation will present it as stable while the related class and options are experimental.

/** Machine-readable pre-execution factory resume failure. */
export type FactoryResumeErrorCode =

• Files reviewed: 17/32 changed files
• Comments generated: 0 new
• Review effort level: Medium

[Copilot]

Review details

Files not reviewed (6)

• go/rpc/zrpc.go: Generated file
• go/rpc/zrpc_encoding.go: Generated file
• go/rpc/zsession_encoding.go: Generated file
• go/rpc/zsession_events.go: Generated file
• go/zsession_events.go: Generated file
• nodejs/package-lock.json: Generated file

Still open (1)

• 🟠 nodejs/src/types.ts#L840 — This broadens elicitation responses beyond the values that the adjacent ElicitationSchemaField…

Comments suppressed due to low confidence (3)

scripts/codegen/typescript.ts:324

• The new test exercises only the branch that collapses an unconstrained opaque node. The preservation branch here protects existing public contracts, but has no regression coverage. Add cases where x-opaque-json accompanies type/properties and anyOf, and assert those schemas retain their structure rather than becoming JsonValue.
  nodejs/src/factory.ts:249
• For a typed handle, args remains optional through RunOptions<TArgs>. A factory declared as defineFactory<{ files: string[] }, ...>(...) can therefore be invoked as session.factory.run(handle); the wrapper sends {}, while the factory body is typed as though args.files always exists. This makes the handle overload unsound and can turn an accepted call into a runtime failure. Require args for handle invocations unless TArgs explicitly permits the default empty object (the name-based overload can remain permissive).

    run<TArgs extends JsonValue>(
        factory: FactoryHandle<TArgs, JsonValue | void>,
        options?: RunOptions<TArgs>
    ): Promise<FactoryRunResult>;

nodejs/src/sessionFsProvider.ts:240

• The new transaction adapter path is untested even though this module already has SQLite adapter coverage. Add tests for successful statement/parameter forwarding, the no-transaction fallback, and error classification (busyOrLocked, postCommitAmbiguous, and fatal) so protocol errors cannot be silently misclassified.

        sqliteTransaction: async ({ statements }) => {
            if (!provider.sqlite?.transaction) {
                return {
                    results: [],
                    error: {

• Files reviewed: 20/35 changed files
• Comments generated: 0 new
• Review effort level: Medium

[Copilot]

Review details

Files not reviewed (6)

• go/rpc/zrpc.go: Generated file
• go/rpc/zrpc_encoding.go: Generated file
• go/rpc/zsession_encoding.go: Generated file
• go/rpc/zsession_events.go: Generated file
• go/zsession_events.go: Generated file
• nodejs/package-lock.json: Generated file

Still open (1)

• 🟠 nodejs/src/types.ts#L840 — This broadens elicitation responses beyond the values that the adjacent ElicitationSchemaField…

Comments suppressed due to low confidence (4)

scripts/codegen/typescript.ts:327

• Returning a new { tsType } object drops the original node's description and other annotations. As a result, this regeneration removes JSDoc from every unconstrained opaque field (for example, the arguments documentation in AssistantMessageToolRequest). Set tsType on rewritten and then strip the marker so the public documentation and annotations survive.
  nodejs/src/factory.ts:249
• The typed-handle overload permits omitting options.args, but the implementation then sends {}. For a factory declared with TArgs = { files: string[] }, session.factory.run(handle) therefore compiles while context.args is typed as that object and actually receives {}, causing ordinary typed factory bodies to fail at runtime. Require args when the handle's TArgs does not admit the documented {} default, or reflect that default in the context type.

    run<TArgs extends JsonValue>(
        factory: FactoryHandle<TArgs, JsonValue | void>,
        options?: RunOptions<TArgs>
    ): Promise<FactoryRunResult>;

go/rpc/zrpc_encoding.go:1163

• When failure is absent, this custom unmarshaller leaves any previous r.Failure value intact. Reusing a FactoryRunTerminal for successive decodes can therefore report a stale failure on a later successful/cancelled terminal value. Clear the destination union field before conditionally decoding it; the Go generator should be fixed and this file regenerated.
  nodejs/src/factory.ts:228
• This newly exported factory-specific type is the exception to the PR's stated rule that every public Agent Factories type is marked @experimental. Add the same experimental JSDoc used by the surrounding public factory types so generated API documentation does not present this error-code contract as stable.

export type FactoryResumeErrorCode =
    | "not_found"
    | "non_resumable"
    | "already_active"
    | "reapproval_declined"
    | "no_approval_provider";

• Files reviewed: 21/36 changed files
• Comments generated: 0 new
• Review effort level: Medium

[MRayermannMSFT]

Latest CCR pass — took one, disproved the other.

Taken (CCR 3) — FactoryMeta mutable while the handle is frozen. Correct. defineFactory deep-freezes the snapshot it stores, but FactoryHandle.meta was typed as a mutable FactoryMeta, so handle.meta.name = "..." and handle.meta.phases.push(...) compiled cleanly and then threw at runtime — the type promised something the object would not honour. meta is now typed deeply readonly on the handle, which turned out to cost exactly one cast inside defineFactory and nothing at any call site.

The test asserts both halves, since either alone would be weak:

expect(() => {
    // @ts-expect-error handle.meta is deeply readonly.
    handle.meta.name = "mutated";
}).toThrow(TypeError);

The @ts-expect-error fails the build if the mutation ever stops being a type error, and the toThrow fails if the object ever stops being frozen.

Declined — "{ output: undefined } → {} breaks client.test.ts:3271." This one is checkable, and it is not true: npm test -- --run client.test.ts is 136/136 green. The assertion is expect(response).toEqual({ output: undefined }), and toEqual treats an absent key and an explicit undefined as equal — only toStrictEqual would distinguish them. No change needed.

[Copilot]

Review details

Comments suppressed due to low confidence (8)

nodejs/src/factory.ts:162

• The pipeline implementation propagates abort and hard RPC/connection failures rather than always converting a thrown stage to null. The public JSDoc should state that exception.

    /** Run each item through every stage without barriers between stages. */
    pipeline(items: unknown[], ...stages: FactoryPipelineStage[]): Promise<unknown[]>;

nodejs/src/factory.ts:33

• This new public factory result type lacks the promised @experimental marker, so generated API documentation can present it as stable while the rest of the surface warns that it may change.

export type FactoryRunResult = Omit<WireFactoryRunResult, "result"> & {

nodejs/src/factory.ts:19

• FactoryRunResult is also returned by getRun() for pending and running runs, so describing it as terminal is inaccurate for consumers inspecting an in-flight run.

 * The terminal envelope describing a factory run's outcome (status, result,
 * reason). Re-exported so consumers can name the type returned by
 * {@link SessionFactoryApi} methods.

nodejs/src/factory.ts:76

• JsonValue is newly exported from both package entry points but is not marked @experimental, contrary to the stated contract that every public type in this surface carries that warning.

export type JsonValue =

nodejs/src/factory.ts:210

• The FactoryHandle experimental JSDoc is separated from the interface by DeepReadonly, so it attaches to that internal alias rather than the exported handle. Generated API docs therefore omit the experimental warning for this public type.

export interface FactoryHandle<

nodejs/src/factory.ts:156

• The runtime intentionally permits arbitrary values for { volatile: true } (the new test returns a function), but this signature always restricts producers and results to JsonValue. TypeScript authors therefore cannot use the supported volatile behavior without an unsafe cast. Add overloads that preserve a generic result for volatile: true while retaining the JSON constraint for journaled steps.

    step(
        key: string,
        producer: () => Promise<JsonValue> | JsonValue,
        options?: FactoryStepOptions
    ): Promise<JsonValue>;

nodejs/src/factory.ts:160

• This public contract says every thrown thunk becomes null, but runFactoryParallel rethrows abort, ResponseError, and ConnectionError. Document the distinction so callers do not treat hard runtime failures as per-item failures.

This issue also appears on line 161 of the same file.

    /** Run thunks concurrently, returning null for a thunk that throws. */
    parallel<TResult>(
        thunks: Array<() => Promise<TResult> | TResult>
    ): Promise<Array<TResult | null>>;

nodejs/docs/factories.md:58

• These bullets say thrown thunks/stages become null except for cancellation, but the implementation also propagates hard runtime ResponseError and ConnectionError failures. Documenting those as item-level null outcomes can cause authors to omit run-level error handling.

- `ctx.parallel(thunks)`: Runs thunks concurrently and awaits all of them (a barrier). A thunk that throws becomes `null` in the result array, except cooperative cancellation propagates. Rejects above 4096 items.
- `ctx.pipeline(items, ...stages)`: Flows each item through every stage without a barrier between stages, so one item can be in a later stage while another is still in an earlier one. Each stage is called as `(previous, item, index)`, where `previous` is the prior stage's result and `item` is the original input. A stage that throws drops that item to `null` and skips its remaining stages. Rejects above 4096 items.

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Medium

[MRayermannMSFT]

Good round — took six of eight, including one regression I introduced last commit.

Taken (CCR 4)

• parallel / pipeline contracts understated failure handling. Both said a thrown thunk or stage becomes null. That is only true for ordinary failures: cancellation and hard runtime failures (ResponseError, ConnectionError) propagate and reject the whole call, deliberately, because they mean the run is in trouble rather than one item. An author reading the old wording would have skipped run-level error handling. Corrected in the JSDoc and in factories.md.
• @experimental had detached from FactoryHandle. My own doing: the DeepReadonly alias I added in CCR 3 landed between the docblock and the interface, so the warning bound to the internal alias and the exported type lost it. Reordered.
• FactoryRunResult was described as "terminal". Inaccurate — getRun() returns the same envelope for pending and running runs, and that will be the common case once execution moves to background-only. Reworded, and given the @experimental marker the rest of the surface carries.

Declined (both previously argued)

• @experimental on JsonValue — it is a general "value representable on the JSON wire" alias, not part of the factory surface. Marking it experimental would signal that JSON representability itself is unstable.
• A generic step() overload for { volatile: true } — a real ergonomic gap, but a public-API redesign rather than a defect. The surface is @experimental precisely so that can be done deliberately.

Typecheck clean, 96 factory tests green, prettier clean.

[Copilot]

Review details

Comments suppressed due to low confidence (3)

nodejs/src/session.ts:1

• params.executionToken is used as a Map key and forwarded over RPC, but there’s no validation that it’s a non-empty string. If it’s missing/undefined, overlapping attempts can collide under the same key, and the forwarded RPC payload may be invalid depending on the wire schema. Recommend validating executionToken early (and throwing a ResponseError(ErrorCodes.InvalidParams, ...) when absent/invalid), or generating an internal unique token when missing while ensuring the RPC contract remains satisfied.

/*---------------------------------------------------------------------------------------------

nodejs/src/session.ts:1352

• params.executionToken is used as a Map key and forwarded over RPC, but there’s no validation that it’s a non-empty string. If it’s missing/undefined, overlapping attempts can collide under the same key, and the forwarded RPC payload may be invalid depending on the wire schema. Recommend validating executionToken early (and throwing a ResponseError(ErrorCodes.InvalidParams, ...) when absent/invalid), or generating an internal unique token when missing while ensuring the RPC contract remains satisfied.

                controllersForRun.set(params.executionToken, controller);
                const progress = new FactoryProgressBuffer(async (lines) => {
                    await self.rpc.factory.log({
                        runId: params.runId,
                        executionToken: params.executionToken,
                        lines,
                    });

nodejs/src/types.ts:450

• This narrows ToolTelemetry from unknown values to strictly JsonValue, which is an API-breaking type change for SDK consumers who may currently attach non-JSON values (e.g. Date, Error, bigint, class instances). If the intention is to guarantee telemetry is JSON-wire-safe, consider (mandatory) documenting this constraint prominently (and ideally validating/enforcing it at the serialization boundary), or (optional) keeping a broader input type and normalizing/serializing explicitly.

export type ToolTelemetry = Record<string, { [key: string]: JsonValue } | undefined>;

• Files reviewed: 15/15 changed files
• Comments generated: 3
• Review effort level: Low

[Copilot]

Review details

Resolved since last review (3)

• ✅ nodejs/src/session.ts#L1075 — Resolved by @​MRayermannMSFT - The tool-result path now deep clones via JSON.parse(JSON.stringify(rawResult)), which can throw…
• ✅ nodejs/src/client.ts#L3020 — Resolved by @​MRayermannMSFT - JSON.parse(JSON.stringify(output)) can (a) throw at runtime (e.g. BigInt/cycles), and (b)…
• ✅ nodejs/src/client.ts#L3005 — Resolved by @​MRayermannMSFT - JSON.parse(JSON.stringify(output)) can (a) throw at runtime (e.g. BigInt/cycles), and (b)…

Comments suppressed due to low confidence (2)

nodejs/src/session.ts:1461

• If definition.run(context) throws and progress.close() also throws (e.g., due to a prior flush failure), the finally block will mask the original factory error with the close/flush error. If preserving the factory failure is important for diagnostics, consider capturing the original exception, attempting progress.close() in finally, and then either rethrowing the original error (optionally logging/suppressing the close error) or combining both into an AggregateError so neither is lost.

                    const result = await definition.run(context);
                    if (result === undefined) {
                        return {};
                    }
                    assertFactoryResult(result);
                    return { result } as FactoryExecuteResult;
                } finally {
                    try {
                        await progress.close();
                    } finally {
                        const controllersForRun = self.factoryAbortControllers.get(params.runId);
                        if (controllersForRun?.get(params.executionToken) === controller) {
                            controllersForRun.delete(params.executionToken);
                            if (controllersForRun.size === 0) {
                                self.factoryAbortControllers.delete(params.runId);
                            }
                        }
                    }
                }

nodejs/src/factory.ts:85

• The JsonValue docstring claims “losslessly on the SDK JSON wire”, but the implementation enforces stricter runtime rules elsewhere (e.g., rejecting NaN/Infinity and -0). Since TypeScript cannot express “finite non-negative-zero numbers”, consider updating this comment (and any related docs) to explicitly note the additional constraints (finite numbers only; -0 rejected; no accessors/non-plain objects, etc.) so callers don’t assume any number is accepted.

/** A value that can be represented losslessly on the SDK JSON wire. */
export type JsonValue =
    | null
    | boolean
    | number
    | string
    | JsonValue[]
    | { [key: string]: JsonValue };

• Files reviewed: 15/15 changed files
• Comments generated: 2
• Review effort level: Low

[Copilot]

Review details

Resolved since last review (2)

• ✅ nodejs/src/session.ts#L1353 — Resolved by @​MRayermannMSFT - params.executionToken is used as a Map key and is forwarded over RPC, but this handler doesn’t…
• ✅ nodejs/src/session.ts#L330 — Resolved by @​MRayermannMSFT - operation() is invoked before the abort listener is registered, so an abort that lands after…

Comments suppressed due to low confidence (5)

nodejs/src/session.ts:2081

• Using ErrorCodes.InternalError (-32603) for “result/step is not JSON” validation failures makes these look like server faults rather than a contract violation in extension-provided data. Consider using ErrorCodes.InvalidParams (-32602) (or a dedicated code if you have one) so callers can reliably distinguish validation failures from runtime/transport failures.

function strictJsonValidationError(
    context: StrictJsonValidationContext,
    category: FactoryResultValidationCategory,
    message: string,
    path: string
): ResponseError<{ code: string; category: FactoryResultValidationCategory; path: string }> {
    return new ResponseError(ErrorCodes.InternalError, message, {
        code: context.code,
        category,
        path,
    });

nodejs/src/session.ts:158

• The thrown message is identical for two distinct invalid inputs and is also inaccurate when the value is neither an array nor a promise (or when an element is a non-function value that isn’t a promise). Suggest splitting these into more precise diagnostics (e.g., “expects an array” vs “array must contain functions”) and keeping the “not promises” guidance only for the “array elements are promises” case.

async function runFactoryParallel<TResult>(
    thunks: Array<() => Promise<TResult> | TResult>
): Promise<Array<TResult | null>> {
    if (!Array.isArray(thunks)) {
        throw new Error(
            "parallel() expects an array of functions, not promises. Wrap each call: () => agent(...)"
        );
    }
    assertFactoryFanoutSize("parallel", thunks.length);
    if (thunks.some((thunk) => typeof thunk !== "function")) {
        throw new Error(
            "parallel() expects an array of functions, not promises. Wrap each call: () => agent(...)"
        );
    }

nodejs/test/factory.test.ts:493

• Promise.withResolvers requires relatively new Node.js runtimes (Node 20+). If this repo/test matrix still supports older Node versions, these tests will fail at runtime; consider using a small local helper to create { promise, resolve, reject } via new Promise(...) (or a Vitest utility) to keep compatibility.

        const contextSeen = Promise.withResolvers<{
            runId: string;
            args: unknown;
            session: CopilotSession;
            signal: AbortSignal;
        }>();

nodejs/src/factory.ts:448

• structuredClone is only available in newer Node.js versions. If the SDK supports older Node runtimes, this will throw at runtime during factory registration. Consider either documenting a minimum Node version for the extension SDK, or adding a fallback cloning strategy (with known limitations) when structuredClone is unavailable.

export function defineFactory<
    TArgs extends JsonValue = JsonValue,
    TResult extends JsonValue | void = JsonValue | void,
>(definition: FactoryDefinition<TArgs, TResult>): FactoryHandle<TArgs, TResult> {
    // Snapshot before validating so post-registration mutation of the caller's
    // object cannot slip past the authoring-boundary checks.
    const meta = deepFreeze(structuredClone(definition.meta));
    validateLimits(meta);

nodejs/src/session.ts:2145

• This branch appears unreachable given the preceding type checks (null/boolean/string/number/function/symbol/bigint are all handled earlier). Keeping unreachable code with a misleading message makes the validator harder to reason about; consider removing it or converting it to an assertion with an “unreachable” message.

        if (typeof current !== "object") {
            throw strictJsonValidationError(
                context,
                "unsupported_type",
                `${context.label} contains a function, symbol, or BigInt at ${path}`,
                path
            );
        }

• Files reviewed: 15/15 changed files
• Comments generated: 0 new
• Review effort level: Low

[github-actions]

Cross-SDK Consistency Review

This PR adds the Agent Factories authoring surface exclusively to the Node.js/TypeScript SDK (nodejs/src/factory.ts, nodejs/src/session.ts, nodejs/src/extension.ts, etc.).

Findings

No inconsistency issues at this time. The feature is:

• Marked @experimental throughout all public types and APIs
• Gated behind the agent_factories runtime feature flag
• Clearly a new, intentional Node.js-first introduction (the PR description acknowledges the original multi-language PR was closed unmerged)

None of the other SDKs (Python, Go, .NET, Java, Rust) currently expose any Agent Factory authoring surface, so this PR doesn't create a regression — it introduces the first implementation.

Consistency suggestion (non-blocking)

Once the Node.js implementation stabilizes, consider tracking parity work for the remaining SDKs. The key public API surface to port would be:

Node.js	Equivalent pattern
defineFactory(meta, fn)	define_factory / DefineFactory / define_factory
joinSession({ factories })	add factories parameter to join_session / JoinSession
session.factory.run(...)	session.factory.run(...) / session.Factory.Run(...)
session.factory.waitForRun(...)	corresponding wait/poll helper
isFactoryRunTerminal(status)	is_factory_run_terminal / IsFactoryRunTerminal

No action required on this PR — just flagging for future parity tracking.

Generated by SDK Consistency Review Agent for #2114 · sonnet46 25.1 AIC · ⌖ 5.46 AIC · ⊞ 6.6K · ◷

[Copilot]

Review details

Comments suppressed due to low confidence (2)

nodejs/src/session.ts:158

• The thrown message is misleading for the !Array.isArray(thunks) case (the problem is “not an array”, not “promises”), and it’s also misleading for the “array contains non-functions” case (could be numbers/objects/etc., not necessarily promises). Consider splitting these into distinct diagnostics (e.g., “parallel() expects an array” vs “parallel() expects an array of functions”) so callers get actionable feedback.

    if (!Array.isArray(thunks)) {
        throw new Error(
            "parallel() expects an array of functions, not promises. Wrap each call: () => agent(...)"
        );
    }
    assertFactoryFanoutSize("parallel", thunks.length);
    if (thunks.some((thunk) => typeof thunk !== "function")) {
        throw new Error(
            "parallel() expects an array of functions, not promises. Wrap each call: () => agent(...)"
        );
    }

nodejs/src/session.ts:2145

• This branch appears to be unreachable given the preceding type checks (all other typeof cases are already handled), and the message is also inaccurate for a generic “not object” case. Either remove the dead branch or replace it with a message that reflects the actual unsupported type (e.g., include typeof current) to keep the validator easier to reason about and the diagnostics accurate.

        if (typeof current !== "object") {
            throw strictJsonValidationError(
                context,
                "unsupported_type",
                `${context.label} contains a function, symbol, or BigInt at ${path}`,
                path
            );
        }

• Files reviewed: 15/15 changed files
• Comments generated: 1
• Review effort level: Low