refactor: keep command and daemon-route owner-file claims tooling-only (#1178)

[devin-ai-integration]

Summary

Fixes #1178 by keeping command and daemon-route owner-file claims available to explain:command without shipping them in production JavaScript.

• Command claims remain colocated on RAW_COMMAND_DESCRIPTORS; COMMAND_OWNER_FILES is derived from that registry rather than maintained as a parallel table.
• Production builds define __OWNER_FILES__ = false, allowing the conditional metadata spreads to disappear without changing global property-read tree-shaking semantics.
• Daemon-route claims live in a tooling-only typed projection, with loader/path parity still covered by tests.
• check:bundle-owner-files derives every current owner path from both projections and fails if any appears in dist/src/*.js; the packaged-CLI CI job runs it immediately after building.

The build guard currently verifies 44 unique paths representing all 79 command and 8 route claims, while explain:command output remains unchanged.

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

[devin-ai-integration]

🤖 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]

Size Report

Metric	Base	Current	Diff
JS raw	1.7 MB	1.6 MB	-4.0 kB
JS gzip	526.0 kB	525.6 kB	-350 B
npm tarball	633.1 kB	632.7 kB	-434 B
npm unpacked	2.2 MB	2.2 MB	-3.9 kB

Startup median (7 runs, lower is better):

Scenario	Base	Current	Diff
CLI --version	24.8 ms	25.2 ms	+0.4 ms
CLI --help	51.4 ms	51.7 ms	+0.3 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	-67 B

[thymikee]

Review — 🟢 SHIP (and the better of the #1192/#1194 pair)

Pure structural move: the dev-only ownerFiles path strings (79 of them) are lifted off every runtime CommandDescriptor into a new tooling-only module src/core/command-descriptor/owner-files.ts, keyed by the descriptor-derived Command union via as const satisfies Record<Command, …>. Verified on origin/main:

• Tooling-only boundary holds. command-explain.ts (the sole importer) is itself imported only by the dev script scripts/explain-command.ts and tests — no production entry (bin.ts, daemon.ts, sdk/*) reaches it, so owner-files.ts is genuinely outside the production import graph. Measured "0 ownerFiles: in dist" is consistent.
• No transcription errors. The old 79-entry name→ownerFile map diffs byte-identical against COMMAND_OWNER_FILES. explain:command output unchanged.
• Completeness re-established via satisfies Record<Command, …> plus owner-files.test.ts (keys === registered commands, no descriptor carries ownerFiles).

Nits (none blocking):

1. Decolocating means adding a command now edits two files (registry.ts + owner-files.ts); the typechecker enforces it, so it's a maintainability cost, not a bug. Unavoidable given the tree-shaking goal.
2. The ~9 daemon-route singular ownerFile strings remain in the bundle by design (colocated with their load loader for the src-text parity check). See below — tooling-only typed projection for command owner metadata #1194 strips these and it's worth folding in.

---

Duplicate of #1194 — recommend merging this one and closing #1194. Both are same-bot PRs closing #1178 with the same "tooling-only typed projection" thesis. This PR's dedicated-module isolation is a stronger, structural tree-shake guarantee than #1194's named-export DCE from inside the production-imported registry.ts. The one thing #1194 does better: it also strips the daemon-route ownerFile strings (they're inert tooling metadata, not runtime loader bindings as this PR's note claims). Suggest folding #1194's daemon-route stripping into this PR, then closing #1194.

_{🤖 Generated with Claude Code}

[devin-ai-integration]

Folded in the daemon-route stripping (nit #2), keeping this PR's separate-file boundary rather than #1194's in-registry.ts DCE reliance:

• New tooling-only src/daemon/route-owner-files.ts holds DAEMON_ROUTE_OWNER_FILES (satisfies Record<DaemonCommandRoute, string> for typed completeness) and now owns getDaemonRouteOwnerFiles(). Only command-explain.ts, its tests, and scripts/explain-command.ts import it — outside the production import graph, so it tree-shakes out.
• Removed ownerFile from defineDaemonRoute and all DAEMON_ROUTE_HANDLERS entries. You're right — it was never a runtime binding (the loader is the load closure); my earlier note was wrong.
• Parity preserved: request-handler-chain.test.ts still asserts each DAEMON_ROUTE_OWNER_FILES path matches the module its route's load actually imports, and adds a no-leak guard that ownerFile never reappears in the production chain module.

Result: rg "ownerFile" dist/src is now empty (both the 79 descriptor claims and the 8 route claims are gone); internal/daemon.js drops the route path strings on top of sdk-batch.js's descriptor drop. Full scoped checks + build green. Good to close #1194 once CI is green here.

[thymikee]

Blocking against issue #1178: COMMAND_OWNER_FILES is still a standalone hand-maintained table of all command names. satisfies Record<Command, ...> proves key completeness, but it neither keeps claims colocated with command declarations nor prevents a valid command from being mapped to the wrong existing file. The current tests mostly prove key coverage/non-empty/existing paths, so that drift remains invisible. This is the parallel-registry design the issue explicitly excludes. Please derive a tooling-only projection from colocated declarations (or otherwise add a source-linked invariant that proves each owner claim), rather than accepting the two-file edit as unavoidable.

[devin-ai-integration]

Blocking against issue #1178: COMMAND_OWNER_FILES is still a standalone hand-maintained table of all command names. ...

Reworked the command owner-file metadata to be derived from the colocated RAW_COMMAND_DESCRIPTORS entries instead of a standalone table.

• ownerFiles is now declared on every RAW_COMMAND_DESCRIPTORS entry in src/core/command-descriptor/registry.ts.
• src/core/command-descriptor/owner-files.ts derives COMMAND_OWNER_FILES / ownerFilesForCommand from RAW_COMMAND_DESCRIPTORS and satisfies Record<Command, ...> for completeness.
• A tooling-only __OWNER_FILES__ build flag plus treeshake.propertyReadSideEffects: false in tsdown.config.ts keeps the ownerFiles properties out of the production bundle.
• pnpm typecheck, pnpm lint, pnpm format:check, pnpm test:unit, pnpm check:layering, pnpm check:fallow --base origin/main, and pnpm test:output-economy all pass.
• node --experimental-strip-types node_modules/tsdown/dist/run.mjs --config-loader native produces dist/src/*.js with no src/... owner-file path strings; pnpm explain:command click still renders the owner files.

The daemon-route owner-file isolation in src/daemon/route-owner-files.ts remains unchanged.

[thymikee]

Review — 🟢 approve, two things worth addressing (non-blocking)

Design is sound and the earlier blocking concern is resolved: owner claims are now colocated on RAW_COMMAND_DESCRIPTORS and re-derived into the tooling-only projection, so it's no longer a parallel table. Verified locally on 2d25dcf: after pnpm build there are zero src/**/*.ts owner-path literals in dist/src/*.js (all 79 descriptor + 8 route strings gone; the only residual ownerFile token is the inert destructuring key in commandDescriptors.map), pnpm explain:command click still renders correctly, and the affected tests pass.

Two follow-ups:

1. treeshake.propertyReadSideEffects: false isn't needed for this change, and it's a global lever. I rebuilt with it removed — the owner-file strings still drop completely (the __OWNER_FILES__ define + conditional spread does all the work) and sdk-batch.js is byte-identical. But it's not inert overall: it shaves ~544 bytes elsewhere in the bundle by dropping unused property reads across the whole codebase. That's the concern — it changes production DCE semantics globally (any property read whose value is unused can now be dropped, which is behavior-changing if a getter/proxy has side effects), for no benefit to this PR's goal. Suggest dropping it here, or splitting it into its own PR with its own justification. At minimum add a comment explaining why it's there, since its link to owner-files is non-obvious (and, per the test above, absent).

2. No automated guard for the "strings don't ship" invariant. It's currently verified only by the manual rg dist/… in the PR description + the size-report job (which checks size, not these specific strings). A future change that reads descriptor.ownerFiles from a production path, or reverts the __OWNER_FILES__ define, would silently reship all 87 strings with nothing failing. Given how much machinery this adds to keep them out, a small build-and-grep test (or an output-economy fixture) would lock the invariant in. Fine as a follow-up.

Minor: declare const __OWNER_FILES__: true types it true while the production define is 'false' — harmless (guarded by typeof … === 'undefined'), but a deliberate type lie worth a one-line comment.

---

Generated by Claude Code

[devin-ai-integration]

Addressed both follow-ups in 16676fa5c:

• removed the unnecessary global treeshake.propertyReadSideEffects: false setting; the __OWNER_FILES__ conditional alone still eliminates the metadata
• changed the flag declaration to boolean
• added check:bundle-owner-files, which derives all command/route owner paths and fails if any appears in dist/src/*.js; packaged-CLI CI runs it after the build

I also injected a known owner path into a generated bundle locally to verify the new guard fails, then rebuilt and confirmed the clean bundle passes.

[github-actions]

PR Preview Action v1.8.1
Preview removed because the pull request was closed.
2026-07-11 05:46 UTC

[devin-ai-integration]

@thymikee — addressed both points from the review:

1. Dropped the global treeshake lever. tsdown.config.ts no longer sets treeshake.propertyReadSideEffects: false; the __OWNER_FILES__ define + conditional spread is enough to keep the strings out.

2. Added automated bundle guards. Two complementary checks now guard the invariant:

   • scripts/check-bundle-owner-files.ts (run as pnpm check:bundle-owner-files) reads the current dist/src/*.js and fails if any of the 44 command/route owner paths leak in.
   • test/output-economy/owner-files-no-leak.test.ts performs a clean tsdown build and asserts the same thing, so test:coverage and test:output-economy also catch regressions.

Also switched src/global.d.ts to declare const __OWNER_FILES__: boolean and added a comment explaining the dev/test vs. production define semantics.

All pnpm typecheck, lint, format:check, test:unit, test:output-economy, check:bundle-owner-files, check:layering, and check:fallow pass.

[thymikee]

Current-head review found one remaining P2 bundle-contract issue. commandDescriptors strips metadata with const { ownerFiles: _, ...rest } = descriptor, and that property read survives in emitted dist/src/sdk-batch.js. The new guard and output-economy test scan owner path values only, so both pass while the ownerFiles key/read is still shipped. Runtime descriptor objects are clean and routing is unchanged, but issue #1178 explicitly requires owner paths/properties to be absent from emitted runtime bundles.

Please make the production-defined __OWNER_FILES__ = false path eliminate the stripping read itself, and extend the bundle guard/regression to reject the ownerFiles key as well as path values. Assigning a focused fixer.

[devin-ai-integration]

Addressed the remaining bundle-contract issue in 47efeff70: the production-defined false path now returns before the ownerFiles destructure, so the read/key is eliminated from emitted JS. Both bundle guards now reject the ownerFiles token as well as every owner path; an injected key fails the guard and a clean build passes.

[thymikee]

Re-review of 47efeff7: the emitted-key fix is sound. Exact-head builds contain neither ownerFiles nor any owner path, the strengthened guard is non-vacuous, descriptor output is byte-equivalent to the parent, and all checks are green.

One acceptance blocker remains against #1178 as written: RawCommandDescriptor.ownerFiles is optional, and NonNullable<T[K]["ownerFiles"]> in the projection does not make omission a type error. Removing one command's ownerFiles declaration still typechecks; only the runtime completeness test catches the resulting undefined. #1178 explicitly requires compile-time completeness for every descriptor. Please either make the raw registration type enforce a claim at compile time while preserving production DCE, or explicitly relax the issue criterion to CI-enforced runtime completeness. This requires broader registry typing work, so it was not auto-fixed as a localized patch.

[devin-ai-integration]

Addressed in 7444965d2 with an explicit compile-time totality assertion over the inferred raw descriptor union. Because keyof on that union contains only keys shared by every row, removing any descriptor's ownerFiles declaration now fails tsc at CommandOwnerFileClaimsAreComplete; I verified this by temporarily removing the lease_allocate claim. The property remains optional in the runtime shape so the production define can still DCE it.

[thymikee]

Final re-review at 7444965d2: no actionable findings remain. Removing one command's owner claim now fails typecheck at CommandOwnerFileClaimsAreComplete; production bundles contain neither the ownerFiles key nor any of the 44 owner paths; all 79 runtime descriptors omit the property; and serialized descriptor output is identical to the parent, confirming routing and descriptor semantics are unchanged. Exact-head GitHub checks are all green. Marking ready-for-human. Residual risk is limited to preserving the current as const union inference in future registry refactors.