Skip to content

Commit bc42b10

Browse files
MRayermannMSFTCopilotCopilot
authored
[Codegen] Honor internal flag on session event types in Node codegen (#2177)
* [Codegen] Keep Internal Types That Public Declarations Reference * Fix @internal stripping: filter union arms correctly, fail hard on violations The root cause of dangling @internal types in the emitted .d.ts was in the publicVariants filter inside generateSessionEvents. The filter checked only the 'data' sub-property of each resolved variant for internal visibility, but missed the case where the arm object itself or its resolved definition is marked visibility:internal. Internal event types (AssistantTurnRetryEvent, ModelCallStartEvent) therefore passed through the filter and appeared in the generated SessionEvent union — while their declarations were separately tagged @internal and stripped — leaving dangling references that collapsed to 'any'. Fix the filter to also check isSchemaInternal on the arm object and on the resolved definition. With this, internal union arms are excluded from compilation entirely: no declaration, no union member, no dangling reference. For RPC, fix emitClientGlobalApiRegistration to filter internal methods from the generated handler interface (HooksHandler), so internal method param types (HookInvokeRequest, HookType, etc.) are no longer referenced by any public declaration. The registration function body still wires up the internal RPC handler internally, but function bodies are not emitted in declaration files. Replace the PR's strippableInternalTypes workaround (which silently promoted internal types to public rather than failing) with assertNoPublicInternalReferences, which throws hard if the codegen ever produces a public declaration referencing an internal type. The schema lint in the runtime already guarantees the schema is valid; a violation here means the codegen has a bug, not something to paper over. Export filterPublicSessionEventVariants and assertNoPublicInternalReferences and add unit tests covering: public arms kept, internal arms excluded (by arm visibility, by definition visibility, and by data-property visibility), the validator passing on @internal-tagged members and function bodies, and the validator throwing on direct public references to internal types. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: skip all-internal client global handler groups from public interface When all methods in a client-global RPC group are marked visibility:internal (currently: hooks.invoke), do not include the group in the generated HooksHandler interface or ClientGlobalApiHandlers. The registration body likewise only wires up public methods. The SDK handles hooks.invoke internally: client.ts registers the handler directly via connection.onRequest, bypassing ClientGlobalApiHandlers, so that HookInvokeRequest/HookType never appear in the public .d.ts surface. Tests that exercised clientGlobalHandlers.hooks.invoke are updated to call handleHooksInvoke directly — the same validation of the wire-format contract without depending on the now-removed public handler plumbing. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * style: apply prettier formatting to client hooks registration Fix the CI-only formatting failure in nodejs/src/client.ts introduced by the previous hooks.invoke plumbing change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * test: tighten internal reference validation and coverage Fix assertNoPublicInternalReferences so it only strips function bodies, preserving interface/type member signatures for validation. Also remove @internal-tagged members before scanning and add a regression test that a public interface member referencing an internal type fails validation. Add an explicit unit test that attachConnectionHandlers registers the hand-written hooks.invoke JSON-RPC entry point and routes it to handleHooksInvoke. Rename the legacy-pattern session-event test to match its actual assertions. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * fix: ignore internal object-valued members in TS validator The fail-hard validator correctly ignores simple @internal-tagged members, but it still treated inline object-shaped members like as public references. That caused false failures when public event payloads contained internal members whose types are removed by stripInternal. Extend the member-removal pass to drop both simple and inline object-valued @internal members before scanning for public references, and add a regression test covering that shape. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --------- Co-authored-by: Copilot <copilot@github.com> Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent e9a0f22 commit bc42b10

6 files changed

Lines changed: 432 additions & 159 deletions

File tree

nodejs/src/client.ts

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -831,11 +831,6 @@ export class CopilotClient {
831831

832832
private setupClientGlobalHandlers(): void {
833833
const handlers: import("./generated/rpc.js").ClientGlobalApiHandlers = {};
834-
// `hooks.invoke` is a client-global RPC method whose payload carries a
835-
// `sessionId`; route each invocation to the matching session's dispatcher.
836-
handlers.hooks = {
837-
invoke: async (params) => await this.handleHooksInvoke(params),
838-
};
839834
if (this.requestHandler) {
840835
handlers.llmInference = createCopilotRequestAdapter(this.requestHandler, () => {
841836
if (!this.connection) {
@@ -2850,6 +2845,17 @@ export class CopilotClient {
28502845
// — the runtime calls into a single handler for the whole connection.
28512846
registerClientGlobalApiHandlers(this.connection, this.clientGlobalHandlers);
28522847

2848+
// `hooks.invoke` is an internal RPC method: the runtime calls it to
2849+
// invoke a hook callback on the client. Route each call to the matching
2850+
// session's dispatcher. Not part of the public ClientGlobalApiHandlers
2851+
// interface because HookInvokeRequest/HookType are internal types.
2852+
this.connection.onRequest(
2853+
"hooks.invoke",
2854+
async (params: { sessionId: string; hookType: string; input: unknown }) => {
2855+
return await this.handleHooksInvoke(params);
2856+
}
2857+
);
2858+
28532859
this.connection.onClose(() => {
28542860
this.state = "disconnected";
28552861
});

nodejs/src/generated/rpc.ts

Lines changed: 0 additions & 19 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

nodejs/src/generated/session-events.ts

Lines changed: 0 additions & 100 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

nodejs/test/client.test.ts

Lines changed: 42 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -3139,16 +3139,47 @@ describe("CopilotClient", () => {
31393139
expect(failureCalls).toEqual(["fail-tool"]);
31403140
});
31413141

3142+
it("registers hooks.invoke on the JSON-RPC connection and routes it to handleHooksInvoke", async () => {
3143+
const client = new CopilotClient();
3144+
const handleHooksInvoke = vi
3145+
.spyOn(client as any, "handleHooksInvoke")
3146+
.mockResolvedValue({ output: { additionalContext: "ok" } });
3147+
3148+
const fakeConnection = {
3149+
onNotification: vi.fn(),
3150+
onRequest: vi.fn(),
3151+
onClose: vi.fn(),
3152+
onError: vi.fn(),
3153+
};
3154+
3155+
(client as any).connection = fakeConnection;
3156+
(client as any).attachConnectionHandlers();
3157+
3158+
const hooksRegistration = fakeConnection.onRequest.mock.calls.find(
3159+
([method]: [string, unknown]) => method === "hooks.invoke"
3160+
);
3161+
expect(hooksRegistration).toBeDefined();
3162+
3163+
const handler = hooksRegistration![1] as (params: {
3164+
sessionId: string;
3165+
hookType: string;
3166+
input: unknown;
3167+
}) => Promise<{ output?: unknown }>;
3168+
const payload = {
3169+
sessionId: "session-1",
3170+
hookType: "postToolUseFailure",
3171+
input: { toolName: "shell" },
3172+
};
3173+
3174+
await expect(handler(payload)).resolves.toEqual({
3175+
output: { additionalContext: "ok" },
3176+
});
3177+
expect(handleHooksInvoke).toHaveBeenCalledWith(payload);
3178+
});
3179+
31423180
it("routes hooks.invoke JSON-RPC requests to the SessionHooks handler", async () => {
3143-
// Validates the full JSON-RPC entry point used by the CLI:
3144-
// clientGlobalHandlers.hooks.invoke({sessionId, hookType, input})
3145-
// → CopilotSession._handleHooksInvoke(hookType, input)
3146-
// → SessionHooks.onPostToolUseFailure(normalizedInput, {sessionId})
3147-
//
3148-
// This guards the wire-format contract that the bundled Copilot
3149-
// CLI relies on: the hookType string "postToolUseFailure" and the
3150-
// input shape `{toolName, toolArgs, error, timestamp, cwd}`.
3151-
// The SDK maps that to public `{..., timestamp: Date, workingDirectory}`.
3181+
// Validates the dispatch behavior for the internal `hooks.invoke`
3182+
// payload after the JSON-RPC connection hands it to the SDK.
31523183
const client = new CopilotClient();
31533184
await client.start();
31543185
onTestFinished(() => stopClient(client));
@@ -3172,7 +3203,7 @@ describe("CopilotClient", () => {
31723203
cwd: "/tmp",
31733204
};
31743205

3175-
const response = await (client as any).clientGlobalHandlers.hooks.invoke({
3206+
const response = await (client as any).handleHooksInvoke({
31763207
sessionId: session.sessionId,
31773208
hookType: "postToolUseFailure",
31783209
input: failureInput,
@@ -3249,7 +3280,7 @@ describe("CopilotClient", () => {
32493280
},
32503281
});
32513282

3252-
const response = await (client as any).clientGlobalHandlers.hooks.invoke({
3283+
const response = await (client as any).handleHooksInvoke({
32533284
sessionId: session.sessionId,
32543285
hookType: "agentStop",
32553286
input: {

0 commit comments

Comments
 (0)