Skip to content

tooling-only typed projection for command owner metadata#1194

Closed
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
pr-1178
Closed

tooling-only typed projection for command owner metadata#1194
devin-ai-integration[bot] wants to merge 1 commit into
mainfrom
pr-1178

Conversation

@devin-ai-integration

Copy link
Copy Markdown
Contributor

Summary

Closes #1178.

PR #1174 colocated ownerFiles on every CommandDescriptor so explain:command had typed, completeness-enforced navigation metadata. Those strings were also kept on the runtime descriptor objects and were emitted in production bundles (e.g. sdk-batch.js).

This PR moves the owner metadata into a tooling-only typed projection that is still colocated with the declarations, still complete at compile time, still used by explain:command, and now dropped from production bundles by normal treeshaking.

Changes

  • CommandDescriptor no longer has ownerFiles.
  • RAW_COMMAND_DESCRIPTORS entries no longer carry ownerFiles.
  • A new commandDescriptorOwners map in src/core/command-descriptor/registry.ts is a separate Record<Command, readonly [string, ...string[]]> projection.
  • src/commands/command-explain.ts and src/commands/__tests__/command-explain.test.ts now merge commandDescriptorOwners when listing owner files.
  • DAEMON_ROUTE_HANDLERS in src/daemon/request-handler-chain.ts no longer takes ownerFile in defineDaemonRoute; a new DAEMON_ROUTE_OWNER_FILES record is the tooling-only source and getDaemonRouteOwnerFiles() returns a copy of it.
  • src/daemon/__tests__/request-handler-chain.test.ts parses the new owner-records structure.
// runtime descriptor
export const commandDescriptors = RAW_COMMAND_DESCRIPTORS.map((descriptor) => ({
  ...descriptor,
  mcpExposed: resolveMcpExposure(descriptor),
})) satisfies readonly CommandDescriptor[];

// tooling-only projection
export const commandDescriptorOwners = {
  ...
} as const satisfies Record<Command, readonly [string, ...string[]]>;

Verification

  • pnpm typecheck, pnpm lint, pnpm format:check, pnpm test:unit, pnpm check:layering, and pnpm test:output-economy all pass.
  • pnpm explain:command still lists owner files correctly.
  • Production bundle (node --experimental-strip-types node_modules/tsdown/dist/run.mjs --config-loader native) no longer contains ownerFiles strings (rg -n "ownerFiles" dist/src is empty); sdk-batch.js gzip drops from ~4.76 kB to ~4.47 kB.

Link to Devin session: https://app.devin.ai/sessions/c5f42b75391f471e96a6354577b5d9be
Requested by: @thymikee

@thymikee thymikee self-assigned this Jul 10, 2026
@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

🤖 Devin AI Engineer

I'll be helping with this pull request! Here's what you should know:

✅ I will automatically:

  • Address comments on this PR. Add '(aside)' to your comment to have me ignore it.
  • Look at CI failures and help fix them

Note: I can only respond to comments from users who have write access to this repository.

⚙️ Control Options:

  • Disable automatic comment, CI, and merge conflict monitoring

@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown

Size Report

Metric Base Current Diff
JS raw 1.6 MB 1.6 MB -4.0 kB
JS gzip 517.9 kB 517.5 kB -347 B
npm tarball 624.2 kB 623.7 kB -496 B
npm unpacked 2.2 MB 2.2 MB -4.0 kB

Startup median (7 runs, lower is better):

Scenario Base Current Diff
CLI --version 26.3 ms 26.1 ms -0.2 ms
CLI --help 54.5 ms 54.5 ms -0.0 ms

Top changed chunks:

Chunk Raw diff Gzip diff
dist/src/sdk-batch.js -3.7 kB -283 B
dist/src/internal/daemon.js -384 B -64 B

- Remove ownerFiles from CommandDescriptor and RAW_COMMAND_DESCRIPTORS.

- Add commandDescriptorOwners in registry.ts as a tooling-only Record<Command, ...>.

- Remove ownerFile from DAEMON_ROUTE_HANDLERS defineDaemonRoute.

- Add DAEMON_ROUTE_OWNER_FILES as tooling-only const in request-handler-chain.ts.

- Update explain:command and tests to merge projections.

- Refactor request-handler-chain test to keep fallow complexity low.

- Treeshaking drops owner metadata from production bundles.

Fixes #1178.

Co-Authored-By: Devin AI <158243242+devin-ai-integration[bot]@users.noreply.github.com>
@thymikee

Copy link
Copy Markdown
Member

Review — 🟡 Duplicate of #1192; recommend closing this one

Pure structural move (like #1192): strips ownerFiles off the runtime CommandDescriptor/RAW_COMMAND_DESCRIPTORS and daemon DAEMON_ROUTE_HANDLERS into satisfies-typed projection consts read only by the tooling path. The code is otherwise SHIP-WITH-NITS quality, but it directly duplicates #1192 (same bot, same #1178, same thesis) and #1192's mechanism is stronger.

Why #1192 is preferable:

  1. Boundary strength. refactor: keep command and daemon-route owner-file claims tooling-only (#1178) #1192 isolates the projection into a separate file (owner-files.ts) that production never imports — the strings drop regardless of DCE. This PR leaves commandDescriptorOwners as a named export inside the production-imported registry.ts and relies on named-export tree-shaking. Functional, but strictly more fragile: one accidental production reference re-ships the strings.
  2. No re-leak guard. refactor: keep command and daemon-route owner-file claims tooling-only (#1178) #1192 ships owner-files.test.ts asserting no runtime descriptor carries ownerFiles. This PR has only the compile-time satisfies (completeness, not leakage). Future code referencing commandDescriptorOwners/getDaemonRouteOwnerFiles re-enters the bundle with nothing to catch it.
  3. Misleading self-verification. The evidence rg "ownerFiles" dist/src is empty — but the new const is named commandDescriptorOwners with no ownerFiles key, so that grep could not detect a leak of the new object even if it occurred. Only the gzip delta is real evidence.

What this PR does better than #1192: it actually strips the ~8 daemon-route ownerFile strings that #1192 leaves in the bundle. #1192's stated reason for leaving them ("binds each route's production loader") is wrong — ownerFile is inert metadata read only by tooling, never used in load/run.

Recommendation: close this PR and fold its daemon-route stripping into #1192 (separate-file isolation + no-leak test). Don't merge both.

🤖 Generated with Claude Code

@devin-ai-integration

Copy link
Copy Markdown
Contributor Author

Agreed — the daemon-route owner-file metadata has been moved into the separate src/daemon/route-owner-files.ts in #1192, with a parity test against the production loaders and a guard that the production chain module doesn't carry ownerFile. The strings are no longer in dist/src (grep empty). Closing this one in favor of #1192.

@thymikee thymikee closed this Jul 10, 2026
@github-actions

Copy link
Copy Markdown
PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-10 11:11 UTC

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.

Keep command owner metadata tooling-only without duplicating registries

1 participant