Skip to content

[Codegen] Keep Internal Types That Public Declarations Reference - #2177

Open
MRayermannMSFT wants to merge 1 commit into
mainfrom
dev/mrayermannmsft/other/fix-stripinternal-dangling-types
Open

[Codegen] Keep Internal Types That Public Declarations Reference#2177
MRayermannMSFT wants to merge 1 commit into
mainfrom
dev/mrayermannmsft/other/fix-stripinternal-dangling-types

Conversation

@MRayermannMSFT

Copy link
Copy Markdown
Contributor

What

Fixes the TypeScript codegen so a type is only tagged @internal when nothing public still references it. @internal drives stripInternal, which deletes the whole declaration from the emitted .d.ts, and the codegen applied it purely from schema visibility, without checking reachability from the public surface. Seventeen types were being stripped while public declarations still named them.

The @internal tagging 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 why HookInvokeRequest/HookInvokeResponse, referenced only by hooks.invoke, were affected.

No public API surface changes: nodejs/src/index.ts and nodejs/src/extension.ts are untouched, and the package root re-exports an explicit list rather than a wildcard. Member-level @internal tags still strip as before; only whole-type stripping is now reachability-checked.

Why

A .d.ts that 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 to any, so the damage spreads far beyond the stripped type.

Concretely, SessionEvent listed AssistantTurnRetryEvent and ModelCallStartEvent while both were stripped, so every consumer's SessionEvent typing collapsed to any -- narrowing by event.type silently stopped type-checking. hooks.invoke had the same problem via HookInvokeRequest/HookInvokeResponse. Because skipLibCheck: true is a common consumer setting, the underlying TS2304s inside node_modules are suppressed, so this surfaces only as scattered implicit-any errors that look like trivial annotation gaps. It went unnoticed from 1.0.8 onward.

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.

Copilot AI review requested due to automatic review settings July 30, 2026 23:45

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Updates TypeScript code generation to preserve internal schema types referenced by public declarations.

Changes:

  • Adds reachability-based @internal tagging.
  • 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. HookInvokeRequest is kept because HooksHandler publicly references it, but HookType is referenced only by HookInvokeRequest; once that request is retained, HookType must 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 stripInternal removes that member. For example, UsageCheckpointModelCacheState is referenced only by the @internal member at session-events.ts:2257, yet its declaration is now retained. The same problem occurs for types used only by @internal exported functions, which this block parser does not model. Reachability must be computed from declarations and members that survive stripInternal.
        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

Comment on lines +98 to +100
const blocks = starts.map((start, i) => ({
name: start.name,
text: generatedTs.slice(start.index, i + 1 < starts.length ? starts[i + 1].index : generatedTs.length),
@github-actions

Copy link
Copy Markdown
Contributor

Cross-SDK Consistency Review ✅

This PR modifies:

  • scripts/codegen/typescript.ts — TypeScript codegen logic
  • nodejs/src/generated/rpc.ts — auto-generated (reflects codegen fix)
  • nodejs/src/generated/session-events.ts — auto-generated (reflects codegen fix)

No cross-SDK consistency issues found.

The change fixes a TypeScript-specific problem: @internal drives TypeScript's stripInternal compiler option, which deletes whole type declarations from emitted .d.ts files. When types were stripped while still referenced by public declarations, TypeScript silently degraded those references to any. This is purely a TypeScript/Node.js toolchain concern — the concept has no equivalent in Python, Go, .NET, Java, or Rust.

The other SDKs are unaffected and need no changes.

Generated by SDK Consistency Review Agent for #2177 · sonnet46 17.3 AIC · ⌖ 8.16 AIC · ⊞ 6.6K ·

@MRayermannMSFT
MRayermannMSFT marked this pull request as ready for review July 30, 2026 23:53
@MRayermannMSFT
MRayermannMSFT requested a review from a team as a code owner July 30, 2026 23:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants