[Codegen] Honor internal flag on session event types in Node codegen - #2177
Conversation
There was a problem hiding this comment.
Pull request overview
Updates TypeScript code generation to preserve internal schema types referenced by public declarations.
Changes:
- Adds reachability-based
@internaltagging. - Moves RPC tagging to a final assembled-file pass.
- Regenerates session-event and RPC types.
Show a summary per file
| File | Description |
|---|---|
scripts/codegen/typescript.ts |
Adds internal-type reachability analysis. |
nodejs/src/generated/session-events.ts |
Retains referenced session-event types. |
nodejs/src/generated/rpc.ts |
Retains referenced RPC types. |
Review details
Comments suppressed due to low confidence (2)
scripts/codegen/typescript.ts:109
- Reachability must be transitive across candidates that are retained.
HookInvokeRequestis kept becauseHooksHandlerpublicly references it, butHookTypeis referenced only byHookInvokeRequest; once that request is retained,HookTypemust be retained too. Computing each candidate independently against the original candidate set can therefore recreate a dangling type; use a fixed-point traversal from public roots.
const safe = new Set<string>();
for (const candidate of candidates) {
const referencedByPublic = blocks.some(
(block) =>
block.name !== candidate &&
!candidates.has(block.name) &&
new RegExp(`\\b${candidate}\\b`).test(block.text)
scripts/codegen/typescript.ts:109
- This treats every reference inside a non-candidate interface as public, even when
stripInternalremoves that member. For example,UsageCheckpointModelCacheStateis referenced only by the@internalmember atsession-events.ts:2257, yet its declaration is now retained. The same problem occurs for types used only by@internalexported functions, which this block parser does not model. Reachability must be computed from declarations and members that survivestripInternal.
const referencedByPublic = blocks.some(
(block) =>
block.name !== candidate &&
!candidates.has(block.name) &&
new RegExp(`\\b${candidate}\\b`).test(block.text)
- Files reviewed: 1/3 changed files
- Comments generated: 1
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
…olations 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>
Alternative fix pushedI've pushed a replacement commit (e46ff41) on top of this branch that takes a different approach. Summary of the findings and what changed: Root causeThe bug is in the Why the PR's fix is the wrong directionThe schema lint in the runtime ( What the replacement commit does
ValidationI compiled both the pre-PR
|
There was a problem hiding this comment.
Review details
Still open (1)
- 🟠 scripts/codegen/typescript.ts#L100 — These blocks start at the
exporttoken, so each declaration's leading JSDoc is assigned to the…
- Files reviewed: 2/4 changed files
- Comments generated: 4
- Review effort level: Balanced
| if (violations.length > 0) { | ||
| throw new Error( | ||
| `Codegen produced public declarations that reference internal types.\n` + | ||
| `This is a codegen bug — fix the generator so internal types are not referenced by public output:\n` + | ||
| violations.join("\n") | ||
| ); |
There was a problem hiding this comment.
I am intentionally not changing this in that direction. The runtime schema lint already enforces that public declarations must not reference internal types. If TypeScript codegen ever produces such a reference, that is a generator bug and we should fail hard, not silently keep the internal type public. That matches the repo invariant we want here, so I consider the hard failure the correct behavior.
| if (isSchemaInternal(variantSchema) || isSchemaInternal(resolvedVariant)) { | ||
| for (const ref of [variantSchema.$ref]) { | ||
| const match = ref?.match(/^#\/(?:definitions|\$defs)\/([^/]+)$/); | ||
| if (match) excludedDefinitionNames.add(match[1]); | ||
| } | ||
| return false; |
There was a problem hiding this comment.
I checked this point and I do not think it should block the fix. Those payload definitions only remain public today because the old generation path emitted schema byproducts that were reachable solely through internal event arms. Once the internal arms are correctly excluded, those payload-only exports disappear too. If we want them to stay public intentionally, the schema should reference them from a public declaration; otherwise keeping them would just preserve an accidental leak of internal-only shapes.
This comment has been minimized.
This comment has been minimized.
…face 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>
|
Follow-up to my previous comment: the CI failures I introduced were due to an incomplete fix. When I removed internal methods from the Commit
All 136 |
There was a problem hiding this comment.
Review details
Still open (5)
- 🔴 scripts/codegen/typescript.ts#L435 — Excluding the event arm makes
json-schema-to-typescriptstop emitting its otherwise-public… - 🔴 scripts/codegen/typescript.ts#L122 — This throws when a public declaration references an internal schema type, but the stated change is…
- 🔴 scripts/codegen/typescript.ts#L1099 — Filtering internal methods only from the handler interface leaves the generated registration…
- 🔴 scripts/codegen/typescript.ts#L106 — The body-removal regex also matches interface and object-type bodies, so the validator erases…
- 🟠 scripts/codegen/typescript.ts#L100 — These blocks start at the
exporttoken, so each declaration's leading JSDoc is assigned to the…
Suppressed comments (4)
scripts/codegen/typescript.ts:106
- This validator drops every interface/object-type body, not just function implementations. For example,
export interface Public { value: Hidden }becomesexport interface Public {}and therefore passes, so the new check cannot detect the dangling interface-member references it is intended to prevent. The later member cleanup also cannot work because all@internalcomments were already removed. Please inspect TypeScript declarations structurally (or emit declarations and inspect those) so only function implementations and stripped members are excluded.
let publicText = block.text
// Remove all block comments (JSDoc and otherwise).
.replace(/\/\*[\s\S]*?\*\//g, "")
// Remove function bodies (from the opening { to matching closing }).
.replace(/\{[^{}]*(?:\{[^{}]*\}[^{}]*)*\}/g, "{}")
scripts/codegen/typescript.ts:121
- This does not implement the PR's stated behavior of keeping a schema-internal type when public output references it. Every internal name is annotated before this call, and a detected reference aborts generation rather than declining the annotation and warning. A newly introduced public reference will therefore break codegen instead of preserving a valid
.d.ts; collect the referenced internal names first, skip@internalfor those names, and report the promised warning.
if (violations.length > 0) {
throw new Error(
`Codegen produced public declarations that reference internal types.\n` +
`This is a codegen bug — fix the generator so internal types are not referenced by public output:\n` +
violations.join("\n")
nodejs/test/client.test.ts:3176
- This now calls the private dispatcher directly, so the test named as JSON-RPC routing coverage still passes if the new
connection.onRequest("hooks.invoke", ...)registration is missing or misspelled. Exercise the request through the connection (or capture and invoke the registeredonRequestcallback) to cover the newly hand-written wire entry point as well as dispatch behavior.
const response = await (client as any).handleHooksInvoke({
nodejs/test/typescript-codegen.test.ts:111
- The test name says this arm is kept, but the assertions verify that it and both definitions are excluded. Rename it to describe the exclusion so failures and test reports communicate the actual contract.
it("keeps arms whose internal data sub-property is the only internal marker (legacy pattern)", () => {
- Files reviewed: 4/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
This comment has been minimized.
This comment has been minimized.
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>
There was a problem hiding this comment.
Review details
Still open (5)
- 🔴 scripts/codegen/typescript.ts#L435 — Excluding the event arm makes
json-schema-to-typescriptstop emitting its otherwise-public… - 🔴 scripts/codegen/typescript.ts#L122 — This throws when a public declaration references an internal schema type, but the stated change is…
- 🔴 scripts/codegen/typescript.ts#L1099 — Filtering internal methods only from the handler interface leaves the generated registration…
- 🔴 scripts/codegen/typescript.ts#L106 — The body-removal regex also matches interface and object-type bodies, so the validator erases…
- 🟠 scripts/codegen/typescript.ts#L100 — These blocks start at the
exporttoken, so each declaration's leading JSDoc is assigned to the…
Suppressed comments (4)
scripts/codegen/typescript.ts:109
- This body-removal regex also matches interface and object-type bodies, not just function implementations. As a result, references in public properties and methods are erased before validation; for example,
export interface Public { value: Hidden }becomesexport interface Public {}and passes. That is the exact shape of the dangling declarations this validator is meant to catch. Also, block comments are removed before the later@internal-member filter can use them. Please parse declarations (for example with the TypeScript AST) or otherwise distinguish function bodies and remove tagged members before comments.
let publicText = block.text
// Remove all block comments (JSDoc and otherwise).
.replace(/\/\*[\s\S]*?\*\//g, "")
// Remove function bodies (from the opening { to matching closing }).
.replace(/\{[^{}]*(?:\{[^{}]*\}[^{}]*)*\}/g, "{}")
// Remove any remaining @internal-tagged member lines that survived (e.g. a
// stripped comment leaves behind a bare property declaration on the next line).
.replace(/[ \t]+\S[^\n]*@internal[^\n]*/g, "");
scripts/codegen/typescript.ts:873
- This still tags every schema-internal type unconditionally and then fails generation if a public reference is detected. It does not implement the PR's stated behavior of retaining such declarations and warning about each declined strip. Once the validator catches interface members correctly, any remaining schema edge will break codegen rather than preserve valid
.d.tsoutput. Determine the referenced internal types first, skip@internalfor those types, and emit the documented warning.
for (const intType of internalTypes) {
rpcTs = rpcTs.replace(
new RegExp(`(^|\\n)(export (?:interface|type) ${intType}\\b)`, "m"),
`$1/** @internal */\n$2`
);
}
assertNoPublicInternalReferences(rpcTs, internalTypes);
nodejs/test/client.test.ts:3180
- Calling
handleHooksInvokedirectly bypasses the newly addedconnection.onRequest("hooks.invoke", ...)registration, so this test can pass even if the JSON-RPC route is missing or registered under the wrong method. Keep a wire-level or mocked-connection assertion that sendshooks.invokethrough the connection before checking hook dispatch.
This issue also appears on line 3253 of the same file.
const response = await (client as any).handleHooksInvoke({
sessionId: session.sessionId,
hookType: "postToolUseFailure",
input: failureInput,
});
nodejs/test/client.test.ts:3262
- This second routing test also invokes the private dispatcher directly, so it no longer covers the changed JSON-RPC registration path for
hooks.invoke. Exercise the request through the connection to ensure theagentStopwire method remains registered and routed correctly.
const response = await (client as any).handleHooksInvoke({
sessionId: session.sessionId,
hookType: "agentStop",
input: {
stopReason: "end_turn",
stop_hook_active: true,
timestamp: 1700000000000,
cwd: "/repo",
},
});
- Files reviewed: 4/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
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>
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>
There was a problem hiding this comment.
Review details
Still open (5)
- 🔴 scripts/codegen/typescript.ts#L435 — Excluding the event arm makes
json-schema-to-typescriptstop emitting its otherwise-public… - 🔴 scripts/codegen/typescript.ts#L122 — This throws when a public declaration references an internal schema type, but the stated change is…
- 🔴 scripts/codegen/typescript.ts#L1099 — Filtering internal methods only from the handler interface leaves the generated registration…
- 🔴 scripts/codegen/typescript.ts#L106 — The body-removal regex also matches interface and object-type bodies, so the validator erases…
- 🟠 scripts/codegen/typescript.ts#L100 — These blocks start at the
exporttoken, so each declaration's leading JSDoc is assigned to the…
Suppressed comments (2)
scripts/codegen/typescript.ts:112
- This removes every brace-delimited fragment in an exported function block, including object type literals in the public signature. Thus
export function f(arg: { value: Hidden }): void {}hasHiddenerased and incorrectly passes validation. Remove only the implementation body (for example via the TypeScript AST), preserving all parameter and return-type syntax.
""
)
// Remove all remaining block comments (JSDoc and otherwise).
scripts/codegen/typescript.ts:106
- This pattern can start at an earlier JSDoc and continue past its
*/until it finds@internalin a later member. For example, a documented publicvalue: Hiddenfollowed by an internal member is removed as one match, so the validator misses the dangling public reference. Constrain the match to a single JSDoc before removing its member.
This issue also appears on line 110 of the same file.
// Handles both simple members (`foo?: Hidden;`) and inline object-shaped members
- Files reviewed: 4/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
Cross-SDK Consistency Review ✅This PR modifies only Node.js/TypeScript files:
No cross-SDK consistency issues. The changes fix a TypeScript-specific codegen bug where The PR description confirms no public API surface changes:
|
There was a problem hiding this comment.
Review details
Resolved since last review (3)
- ✅ scripts/codegen/typescript.ts#L1099 — Resolved by Copilot - Filtering internal methods only from the handler interface leaves the generated registration…
- ✅ scripts/codegen/typescript.ts#L106 — Resolved by Copilot - The body-removal regex also matches interface and object-type bodies, so the validator erases…
- ✅ scripts/codegen/typescript.ts#L100 — Resolved by Copilot - These blocks start at the
exporttoken, so each declaration's leading JSDoc is assigned to the…
Still open (2)
- 🔴 scripts/codegen/typescript.ts#L435 — Excluding the event arm makes
json-schema-to-typescriptstop emitting its otherwise-public… - 🔴 scripts/codegen/typescript.ts#L122 — This throws when a public declaration references an internal schema type, but the stated change is…
Suppressed comments (2)
scripts/codegen/typescript.ts:117
- Restricting this replacement to function blocks still removes brace-delimited types in the function signature itself.
export function f(arg: { value: Hidden }): void {}is reduced to a signature with{}, so this validator passes even though declaration emit retains theHiddenreference. Function bodies can also determine inferred return types in emitted declarations. Parse the function and exclude only its implementation body, or validate the actual declaration emit instead of applying a brace regex.
if (block.kind === "function") {
// Remove function bodies (from the opening { to matching closing }).
publicText = publicText.replace(/\{[^{}]*(?:\{[^{}]*\}[^{}]*)*\}/g, "{}");
scripts/codegen/typescript.ts:109
- This pattern can cross JSDoc boundaries because
[\s\S]*?is not bounded by the first closing*/: an ordinary documented public member followed by an@internalmember is removed starting at the first comment. For example, a publicvisible: Hiddenmember between those comments disappears frompublicText, so the validator misses the dangling reference. Constrain the@internalsearch to one block comment.
/^[ \t]*\/\*\*[\s\S]*?@internal[\s\S]*?\*\/\s*\n(?:[ \t]*[^\n{;]+;\n?|[ \t]*[^\n{]+\{\n[\s\S]*?^[ \t]*\};\n?)/gm,
- Files reviewed: 4/6 changed files
- Comments generated: 0 new
- Review effort level: Balanced
SteveSandersonMS
changed the title
What
This PR fixes a TypeScript codegen bug that produced public
.d.tsdeclarations referencing types that were then removed bystripInternal.There were two implementation phases in this PR:
1) Original approach (superseded)
The initial fix changed
@internaltagging to be reachability-aware, so a type was tagged@internalonly when no public declaration still referenced it.This addressed the immediate dangling-reference symptom, but it was not the right long-term model because it could effectively "promote" internal types into public declarations when codegen leaked references.
2) Updated approach (current final implementation)
The branch now takes a stricter and more correct direction:
Fix the actual leak at generation time
SessionEventvariant filtering now excludes internal union arms correctly by checking internal visibility at the arm and resolved-definition levels (not only a nesteddataschema case).assistant.turn_retry,model.call_start) are removed from public union output instead of being left as public references.Fail hard if codegen ever regresses
assertNoPublicInternalReferencesin TypeScript codegen.Keep client-global internal methods out of public handler interfaces
emitClientGlobalApiRegistrationnow skips groups with no public methods.hooks.invokewiring remains handled internally inclient.tsvia direct JSON-RPC registration, not through publicClientGlobalApiHandlers.Why
A
.d.tsthat names a missing type is not just untidy; it breaks consumer typing. Onmain,SessionEventincluded names likeAssistantTurnRetryEvent/ModelCallStartEventwhile those declarations were stripped, causing TypeScript to degrade downstream typing (often silently whenskipLibCheckis enabled).The updated implementation is intentionally stricter than the original draft:
Scope / API impact
Validation
nodejs/test/typescript-codegen.test.tsand adjustednodejs/test/client.test.tsfor the internal hooks wiring path.