Remove the legacy dynamic invocation surface with teaching errors (narrow phase) - #1065
Conversation
Narrow phase of the static-first two-rule model (#1048), gated on the fleet codemod scan showing zero legacy usage across all production packages: - packages.check and packages.invokeChecked no longer exist: the host tool set carries only invoke, and the sandbox prelude (execute and package runtimes, plus the package-app bridge) throws teaching errors naming the exact replacement. - Literal dynamic import("kody:@...") is rewritten at bundle time to a teaching error; no placeholder modules or dynamic-dependency metadata are produced. Hydration keeps resolving placeholders inside bundles published before the removal so pinned snapshots keep working until dependents republish. - Publish checks escalate from non-fatal deprecation warnings to failing lint results naming the replacement and the 0002-static-first-invocation codemod (same collector keeps codemod findings in lockstep). - The invoke contract check drops the dead export-projection branch that only packages.check consumed. - Docs flip widen-phase deprecation notes to removed/teaching text. Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThe change removes ChangesLegacy invocation removal
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
🔎 Preview deployed: https://kody-pr-1065.kody-a99.workers.dev Worker: Mocks:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/worker/src/package-runtime/package-app.ts (1)
360-376: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the shared removed-package teaching-message constants.
createPackagesProxyduplicatesremovedPackagesCheckMessageandremovedPackagesInvokeCheckedMessage, whilecreatePackagesHelperPreludealready embeds the same exported strings. Reuse those strings inpackage-app.tsviaJSON.stringify(...)so the package-app Worker and sandbox VM guidance stay synchronized.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/worker/src/package-runtime/package-app.ts` around lines 360 - 376, Update createPackagesProxy to reuse the shared removedPackagesCheckMessage and removedPackagesInvokeCheckedMessage constants already used by createPackagesHelperPrelude, embedding each via JSON.stringify(...) in the thrown Error messages. Remove the duplicated inline teaching text so package-app Worker and sandbox VM guidance remain synchronized.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/use/packages.md`:
- Around line 150-155: Update the legacy import guidance in the package
documentation to remove the contradictory exception that literal dynamic imports
need no kody.dependencies, since publish checks now reject them. Revise the
related “any of them throws” wording to apply only to new source or bundles,
while preserving the documented hydration support for pre-removal bundles and
aligning both affected sections with the package-manifest and execute
documentation.
---
Nitpick comments:
In `@packages/worker/src/package-runtime/package-app.ts`:
- Around line 360-376: Update createPackagesProxy to reuse the shared
removedPackagesCheckMessage and removedPackagesInvokeCheckedMessage constants
already used by createPackagesHelperPrelude, embedding each via
JSON.stringify(...) in the thrown Error messages. Remove the duplicated inline
teaching text so package-app Worker and sandbox VM guidance remain synchronized.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6350b8c7-5c67-45f2-bfc1-2febb6a76de7
📒 Files selected for processing (18)
docs/contributing/packages-and-manifests.mddocs/use/execute.mddocs/use/packages.mdpackages/worker/src/mcp/instructions/execute-tool-description.tspackages/worker/src/mcp/run-kody-registry.node.test.tspackages/worker/src/mcp/runtime-helper-manifest.tspackages/worker/src/package-invocations/http-invoke.tspackages/worker/src/package-invocations/invoke-check.tspackages/worker/src/package-invocations/runtime-tool-factories.tspackages/worker/src/package-invocations/service.node.test.tspackages/worker/src/package-runtime/deprecated-invocation-usage.tspackages/worker/src/package-runtime/module-graph-import-rewriting.tspackages/worker/src/package-runtime/module-graph.node.test.tspackages/worker/src/package-runtime/module-graph.workers.test.tspackages/worker/src/package-runtime/package-app.tspackages/worker/src/package-runtime/runtime-source-modules.tspackages/worker/src/repo/checks.node.test.tspackages/worker/src/repo/checks.ts
… qualify removal wording Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit c03914d. Configure here.
| return ` | ||
| export const ${dynamicPackageImportSpecifierExportName} = ${JSON.stringify(input.specifier)}; | ||
|
|
||
| throw new Error( |
There was a problem hiding this comment.
Unused exported message builder
Low Severity
This commit adds exported buildRemovedDynamicKodyImportMessage, but nothing in the repo imports or calls it. Literal dynamic-import teaching errors still come from the inline string inside createRemovedDynamicKodyImportHelperSource, so the new helper is dead surface area and can drift from the runtime message.
Reviewed by Cursor Bugbot for commit c03914d. Configure here.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/use/packages.md (1)
156-160: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winLimit
kody.dependenciesguidance to saved package code.“Every direct static import” also covers ad hoc
executeimports, but those are bundled per call and do not have a package manifest to update. Qualify this as “Every direct static import in saved package code” to avoid misleading execute users.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/use/packages.md` around lines 156 - 160, Update the kody.dependencies guidance in the package documentation to apply only to direct static imports in saved package code, excluding ad hoc execute imports that are bundled per call and have no package manifest.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@docs/use/packages.md`:
- Around line 156-160: Update the kody.dependencies guidance in the package
documentation to apply only to direct static imports in saved package code,
excluding ad hoc execute imports that are bundled per call and have no package
manifest.
3 participants
Summary
Executes the narrow phase of the static-first program per #1048, with every gate satisfied: the fleet codemod scan reported zero legacy usage across all production packages (97 clean), the two-rule guidance and metering are deployed, and Kent waived the remaining observation window.
What's removed (and what teaches)
packages.check/packages.invokeChecked: gone from the host tool set (PackageInvokeToolsis{ invoke }), the bridge provider, and the package-app runtime bridge. The sandbox prelude and the package-apppackagesproxy keep the property names as throwing teaching errors naming the exact replacement — agents learn the current contract from error text, andtypeof packages.check === 'function'still holds so feature-detection code fails at call time with the teaching message rather than aTypeError.import("kody:@..."): the bundler rewrites each call site to a teaching error naming the static-import andpackages.invokereplacements; no placeholder modules or dynamic-dependency metadata are produced. The computed-import guard message is updated to the two-rule wording.0002-static-first-invocationcodemod. The same collector (deprecated-invocation-usage.ts) backs both the check and the codemod, so they stay in lockstep by construction.packages.checkconsumed — the lean path loses a conditional, not a feature.Deliberate compatibility net
hydrateKodyRuntimeModuleskeeps resolving dynamic-import placeholder modules found in already-published bundles: a dependent republished before today may carry a pinned snapshot of an older dependency version that used the pattern, and those artifacts must keep working until the dependent republishes. New bundles can never produce placeholders, so this path is effectively dormant; it can be deleted in a follow-up once pre-narrow artifacts age out.Testing
npm run validaterunning as the final gate.Program report
packages.invokeChecked/packages.checkthrow the teaching errors, a literal dynamic import throws its teaching error, publishing a package with legacy usage fails checks, andpackages.invoke(keyless + keyed replay) still works; then close Narrow phase: eliminate the deprecated dynamic invocation surface (invokeChecked, packages.check, dynamic kody:@ imports) #1048.invoke-check.tssimplification here).Summary by CodeRabbit
New Features
packages.invokeis now the sole supported dynamic package invocation entrypoint and includes contract checking.packages.invoke.Bug Fixes
Documentation