Repository navigation
De-emphasize codemode package invocation bridge - #472
Merged
Merged
Conversation
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
📝 WalkthroughWalkthroughThis PR refactors how package invoke functionality is exposed at runtime. Package invoke calls are moved from public ChangesPackage invoke runtime bridge refactor
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
kentcdodds
marked this pull request as ready for review
May 14, 2026 18:03
Contributor
|
🔎 Preview deployed: https://kody-pr-472.kentcdodds.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.
🧹 Nitpick comments (5)
packages/worker/src/mcp/run-codemode-registry.node.test.ts (1)
1557-1573: ⚡ Quick winAssert provider cardinality before indexing bridge provider.
The test reads
providers[1]directly; add an explicit length check so bridge registration regressions fail with a clearer contract-level message.Proposed tweak
async execute(wrapped, providers) { + expect(providers).toHaveLength(2) expect(wrapped).toContain('const packages = {') expect(wrapped).toContain('__kodyPackageInvokeRuntimeBridge') expect(wrapped).not.toContain('codemode.package_invoke') expect(wrapped).toContain( "packages: typeof packages === 'undefined' ? null : packages", )🤖 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/mcp/run-codemode-registry.node.test.ts` around lines 1557 - 1573, The test's execute(...) callback indexes providers[1] without asserting providers length first; add an explicit assertion on providers (e.g., expect(providers).toHaveLength(expected) or expect(providers.length).toBeGreaterThan(1)) before assigning providerFns and packageBridgeFns so a missing bridge provider fails with a clear contract-level message; update the block around execute(wrapped, providers) to assert providers' cardinality prior to using providers[0] and providers[1] (references: execute, providers, providerFns, packageBridgeFns).packages/worker/src/package-runtime/module-graph.workers.test.ts (4)
573-582: ⚡ Quick winConsider adding a comment explaining the expected rejection.
This code tests that direct calls to
codemode.package_invoke_checkedare rejected, which is a key behavioral change in this PR. A brief comment would help future maintainers understand why this test expects failure rather than success.📝 Suggested comment
+ // Direct codemode.package_invoke_checked calls should reject; packages.invokeChecked is the public API let directCodemodeInvokeChecked; try { await codemode.package_invoke_checked({🤖 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/module-graph.workers.test.ts` around lines 573 - 582, Add a short inline comment above the try/catch that calls codemode.package_invoke_checked explaining that the call is expected to reject (so the test asserts failure) due to the PR's behavioral change preventing direct invocation; reference the function name codemode.package_invoke_checked and the test variable directCodemodeInvokeChecked in the comment and briefly state the reason (e.g., direct package invoke is disallowed in this worker test) so future maintainers understand why the test expects an error.
660-662: ⚡ Quick winConsider making the failure expectation more explicit.
The assertion correctly verifies that the error message contains
'package_invoke_checked', but you could make it clearer that the call is expected to fail by also asserting the value is not'resolved'.🔍 More explicit assertion
directCodemodeInvokeChecked: expect.stringContaining( 'package_invoke_checked', ), + // Alternatively, be more explicit: + // expect(result.result.directCodemodeInvokeChecked).not.toBe('resolved') + // expect(result.result.directCodemodeInvokeChecked).toContain('package_invoke_checked')🤖 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/module-graph.workers.test.ts` around lines 660 - 662, The test currently asserts that directCodemodeInvokeChecked contains 'package_invoke_checked' but doesn't clearly express that the call failed; update the assertion for directCodemodeInvokeChecked to also assert the value is not 'resolved' (e.g., add an expectation using expect.not.toBe('resolved') or equivalent) so the test explicitly verifies the call did not succeed while still checking for the 'package_invoke_checked' text.
691-700: ⚡ Quick winConsider adding a comment explaining the expected rejection.
Same as the previous test: this code verifies that direct
codemode.package_invoke_checkedcalls are rejected. A brief comment would clarify the intent for future maintainers.📝 Suggested comment
+ // Direct codemode.package_invoke_checked calls should reject; packages.invokeChecked is the public API let directCodemodeInvokeChecked; try { await codemode.package_invoke_checked({🤖 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/module-graph.workers.test.ts` around lines 691 - 700, Add a short inline comment above the try/catch that explains the test expects codemode.package_invoke_checked to reject when invoked directly: reference the local variable directCodemodeInvokeChecked and the call codemode.package_invoke_checked({ kodyId: 'target-package', exportName: './run' }) so future maintainers understand that the try block should not resolve and the catch path captures the rejection message.
770-772: ⚡ Quick winConsider making the failure expectation more explicit.
Same suggestion as the previous test: the assertion is correct but could be clearer about expecting failure by also checking that the value is not
'resolved'.🔍 More explicit assertion
directCodemodeInvokeChecked: expect.stringContaining( 'package_invoke_checked', ), + // Alternatively, be more explicit: + // expect(result.result.directCodemodeInvokeChecked).not.toBe('resolved') + // expect(result.result.directCodemodeInvokeChecked).toContain('package_invoke_checked')🤖 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/module-graph.workers.test.ts` around lines 770 - 772, The assertion for directCodemodeInvokeChecked currently only checks for stringContaining('package_invoke_checked'); add an explicit negative check that the value is not the success marker 'resolved' to make the failure expectation clearer—locate the test using the directCodemodeInvokeChecked matcher in module-graph.workers.test.ts and add a companion assertion (e.g., expect(...).not.toBe('resolved') or expect(...).not.stringContaining('resolved')) targeting the same value so the test both expects the failure token and asserts it's not the successful 'resolved' token.
🤖 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.
Nitpick comments:
In `@packages/worker/src/mcp/run-codemode-registry.node.test.ts`:
- Around line 1557-1573: The test's execute(...) callback indexes providers[1]
without asserting providers length first; add an explicit assertion on providers
(e.g., expect(providers).toHaveLength(expected) or
expect(providers.length).toBeGreaterThan(1)) before assigning providerFns and
packageBridgeFns so a missing bridge provider fails with a clear contract-level
message; update the block around execute(wrapped, providers) to assert
providers' cardinality prior to using providers[0] and providers[1] (references:
execute, providers, providerFns, packageBridgeFns).
In `@packages/worker/src/package-runtime/module-graph.workers.test.ts`:
- Around line 573-582: Add a short inline comment above the try/catch that calls
codemode.package_invoke_checked explaining that the call is expected to reject
(so the test asserts failure) due to the PR's behavioral change preventing
direct invocation; reference the function name codemode.package_invoke_checked
and the test variable directCodemodeInvokeChecked in the comment and briefly
state the reason (e.g., direct package invoke is disallowed in this worker test)
so future maintainers understand why the test expects an error.
- Around line 660-662: The test currently asserts that
directCodemodeInvokeChecked contains 'package_invoke_checked' but doesn't
clearly express that the call failed; update the assertion for
directCodemodeInvokeChecked to also assert the value is not 'resolved' (e.g.,
add an expectation using expect.not.toBe('resolved') or equivalent) so the test
explicitly verifies the call did not succeed while still checking for the
'package_invoke_checked' text.
- Around line 691-700: Add a short inline comment above the try/catch that
explains the test expects codemode.package_invoke_checked to reject when invoked
directly: reference the local variable directCodemodeInvokeChecked and the call
codemode.package_invoke_checked({ kodyId: 'target-package', exportName: './run'
}) so future maintainers understand that the try block should not resolve and
the catch path captures the rejection message.
- Around line 770-772: The assertion for directCodemodeInvokeChecked currently
only checks for stringContaining('package_invoke_checked'); add an explicit
negative check that the value is not the success marker 'resolved' to make the
failure expectation clearer—locate the test using the
directCodemodeInvokeChecked matcher in module-graph.workers.test.ts and add a
companion assertion (e.g., expect(...).not.toBe('resolved') or
expect(...).not.stringContaining('resolved')) targeting the same value so the
test both expects the failure token and asserts it's not the successful
'resolved' token.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2d594418-a268-4c22-b09d-784ecab30f90
📒 Files selected for processing (3)
packages/worker/src/mcp/run-codemode-registry.node.test.tspackages/worker/src/mcp/run-codemode-registry.tspackages/worker/src/package-runtime/module-graph.workers.test.ts
Co-authored-by: Kent C. Dodds <me+github@kentcdodds.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
packages.check,packages.invoke, andpackages.invokeCheckedthrough a private runtime bridge provider instead of syntheticcodemode.package_invoke_*toolspackages.*still works while directcodemode.package_invoke_checkedrejectsTesting
npx vitest run --project node-unit "packages/worker/src/mcp/run-codemode-registry.node.test.ts"npx vitest run --project workers-unit "packages/worker/src/package-runtime/module-graph.workers.test.ts"npm run validate✅ Validate,🔎 Deploy Preview Resources, CodeRabbit, and Cursor Bugbot passingCloses #470
Summary by CodeRabbit
Tests
Refactor