[Codegen] Keep Internal Types That Public Declarations Reference - #2177
[Codegen] Keep Internal Types That Public Declarations Reference#2177MRayermannMSFT wants to merge 1 commit into
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
| const blocks = starts.map((start, i) => ({ | ||
| name: start.name, | ||
| text: generatedTs.slice(start.index, i + 1 < starts.length ? starts[i + 1].index : generatedTs.length), |
Cross-SDK Consistency Review ✅This PR modifies:
No cross-SDK consistency issues found. The change fixes a TypeScript-specific problem: The other SDKs are unaffected and need no changes.
|
What
Fixes the TypeScript codegen so a type is only tagged
@internalwhen nothing public still references it.@internaldrivesstripInternal, which deletes the whole declaration from the emitted.d.ts, and the codegen applied it purely from schemavisibility, without checking reachability from the public surface. Seventeen types were being stripped while public declarations still named them.The
@internaltagging for RPC types also moves to a final pass over the assembled file. The client and server method signatures that reference these types are emitted after the per-schema type chunks, so a per-chunk check cannot see them -- that is whyHookInvokeRequest/HookInvokeResponse, referenced only byhooks.invoke, were affected.No public API surface changes:
nodejs/src/index.tsandnodejs/src/extension.tsare untouched, and the package root re-exports an explicit list rather than a wildcard. Member-level@internaltags still strip as before; only whole-type stripping is now reachability-checked.Why
A
.d.tsthat references a type it does not declare is not merely untidy -- it is silently destructive. The dangling reference makes the referring declaration an error type, which TypeScript degrades toany, so the damage spreads far beyond the stripped type.Concretely,
SessionEventlistedAssistantTurnRetryEventandModelCallStartEventwhile both were stripped, so every consumer'sSessionEventtyping collapsed toany-- narrowing byevent.typesilently stopped type-checking.hooks.invokehad the same problem viaHookInvokeRequest/HookInvokeResponse. BecauseskipLibCheck: trueis a common consumer setting, the underlyingTS2304s insidenode_modulesare suppressed, so this surfaces only as scattered implicit-anyerrors that look like trivial annotation gaps. It went unnoticed from1.0.8onward.A comment in the RPC generator asserted that "the runtime schema generator enforces that no public method references an internal type, so there's no transitive propagation to do here." That invariant does not hold in practice, so the codegen now verifies it instead of assuming it, and logs a warning naming each type it declines to strip.