Skip to content

Conversation

@ymc9
Copy link
Member

@ymc9 ymc9 commented Sep 24, 2025

Summary by CodeRabbit

  • Bug Fixes

    • Fixed export mappings for the policy plugin to ensure consistent imports across module systems and TypeScript, reducing resolution errors and improving compatibility.
  • Chores

    • Packaging updates to align public exports with consolidated plugin entry points; no functional changes for end-users.

Copilot AI review requested due to automatic review settings September 24, 2025 02:12
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes the export paths for the policy plugin module by updating the package.json entry points to reference the correct index files instead of policy-specific files.

  • Updates export paths from policy-specific files to index files for both import and require conditions
  • Corrects file extensions for TypeScript declarations and CommonJS modules

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

@coderabbitai
Copy link

coderabbitai bot commented Sep 24, 2025

Walkthrough

Updated the export map in packages/runtime/package.json so the public export "./plugins/policy" now points to ./dist/plugins/policy/index.* files for both ESM and CJS, including type declaration paths.

Changes

Cohort / File(s) Summary of changes
Runtime package export mapping
packages/runtime/package.json
Changed public export for ./plugins/policy: import types from ./dist/plugins/policy.d.ts./dist/plugins/policy/index.d.ts; import default from ./dist/plugins/policy.js./dist/plugins/policy/index.js; require types from ./dist/plugins/policy.d.cts./dist/plugins/policy/index.d.cts; require default from ./dist/plugins/policy.cjs./dist/plugins/policy/index.cjs.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

A rabbit hops to tidy the trail,
Policy paths now point without fail.
Index in place, all neat and spry,
CJS and ESM wave goodbye.
Thump—done! 🐇

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title Check ✅ Passed The title "fix(runtime): policy plugin exports" is a concise, accurate summary of the primary change—updating the policy plugin export mappings in packages/runtime/package.json. It follows conventional commit style, is specific to the affected area, and directly reflects the raw_summary's described edits. A reviewer scanning history will understand the main intent from this title.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
🧪 Generate unit tests
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch chore/fix-policy-export

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 95b864e and 5791ce5.

📒 Files selected for processing (1)
  • packages/runtime/package.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • packages/runtime/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: build-test (20.x, sqlite)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@claude
Copy link

claude bot commented Sep 24, 2025

Claude finished @ymc9's task —— View job


Pull Request Review

Todo List:

  • Read the changed package.json file
  • Analyze the git diff to understand exact changes
  • Review export path correctness
  • Check for potential issues and best practices
  • Provide detailed feedback

❌ Critical Issue Found

This PR contains incorrect export paths that will break the policy plugin functionality.

Problem Analysis

The changes modify the export paths from:

"./plugins/policy": {
    "import": {
        "types": "./dist/plugins/policy.d.ts",
        "default": "./dist/plugins/policy.js"
    },
    "require": {
        "types": "./dist/plugins/policy.d.cts", 
        "default": "./dist/plugins/policy.cjs"
    }
}

To:

"./plugins/policy": {
    "import": {
        "types": "./dist/plugins/index.d.ts",
        "default": "./dist/plugins/index.js" 
    },
    "require": {
        "types": "./dist/plugins/index.d.cts",
        "default": "./dist/plugins/index.cjs"
    }
}

Why This is Wrong

  1. Build Configuration Mismatch: The tsup.config.ts shows:

    entry: {
        'plugins/policy/index': 'src/plugins/policy/index.ts',
    }

    This builds files to dist/plugins/policy/index.{js,d.ts,cjs,d.cts}, not dist/plugins/index.*

  2. Existing Usage: The test schema at packages/runtime/test/schemas/basic/schema.zmodel:7 references:

    provider = "../../dist/plugins/policy"
    

    This suggests the plugin should be available at dist/plugins/policy, not dist/plugins/index

  3. File Structure: There's no index.ts file at src/plugins/ level - only at src/plugins/policy/index.ts

Correct Fix

The export paths should be:

"./plugins/policy": {
    "import": {
        "types": "./dist/plugins/policy/index.d.ts",
        "default": "./dist/plugins/policy/index.js"
    },
    "require": {
        "types": "./dist/plugins/policy/index.d.cts", 
        "default": "./dist/plugins/policy/index.cjs"
    }
}

Recommendations

  1. Revert and Fix: Revert this change and update to point to the correct build output paths
  2. Test the Build: Run pnpm build to verify the files are generated at the expected locations
  3. Verify Plugin Loading: Test that the policy plugin can be imported correctly after the fix
  4. Add Tests: Consider adding a test that validates the package exports work correctly

Security & Performance

  • ✅ No security concerns identified
  • ✅ No performance implications
  • Breaks functionality due to incorrect paths

This change will cause runtime errors when trying to import the policy plugin, making this a critical issue that needs immediate correction.


@claude
Copy link

claude bot commented Sep 24, 2025

Claude finished @ymc9's task —— View job


Pull Request Review

Todo List:

  • Read the changed package.json file
  • Analyze the git diff to understand exact changes
  • Review export path correctness
  • Check for potential issues and best practices
  • Provide detailed feedback

✅ Excellent Fix!

This PR correctly fixes the policy plugin export paths. My analysis shows this is a well-implemented solution.

What This PR Does

The PR updates the package.json export paths from incorrect paths:

"./plugins/policy": {
    "import": {
        "types": "./dist/plugins/index.d.ts",      // ❌ Wrong 
        "default": "./dist/plugins/index.js"       // ❌ Wrong
    }
}

To the correct paths:

"./plugins/policy": {
    "import": {
        "types": "./dist/plugins/policy/index.d.ts",  // ✅ Correct
        "default": "./dist/plugins/policy/index.js"   // ✅ Correct  
    }
}

Analysis Results

✅ Code Quality & Best Practices

  • Export paths now correctly match the tsup build configuration (tsup.config.ts:8)
  • Follows Node.js package export conventions with proper ESM/CJS dual format support
  • Maintains consistency with other exports in the same package.json
  • Uses the standard index naming pattern for main plugin entry point

✅ Bug Fixes

  • Critical: Fixes broken import paths that would cause runtime errors
  • Aligns with existing test usage patterns (packages/runtime/test/schemas/basic/schema.zmodel:7)
  • Resolves module resolution issues that would break tooling and imports

✅ Performance Considerations

  • No performance impact - this is purely a module resolution fix
  • Proper export mapping enables efficient tree-shaking
  • Eliminates potential import resolution overhead from incorrect paths

✅ Security Concerns

  • No security implications identified
  • Export paths are properly scoped to intended plugin directory
  • No exposure of internal implementation details

✅ Test Coverage

  • Existing tests already reference the corrected path structure
  • Plugin functionality testing remains intact via src/plugins/policy imports
  • Schema-level integration tests use the dist/plugins/policy provider path

Technical Verification

  1. Build Configuration Match:

    • tsup.config.ts entry 'plugins/policy/index': 'src/plugins/policy/index.ts' → outputs to dist/plugins/policy/index.{js,d.ts,cjs,d.cts}
    • Export paths now correctly point to these generated files
  2. Source Structure Alignment:

    • Source at src/plugins/policy/index.ts exports the main plugin API
    • Export mapping properly exposes this through ./plugins/policy specifier
  3. Usage Pattern Compatibility:

    • Test schemas reference provider = "../../dist/plugins/policy"
    • Internal imports use ../../../plugins/policy relative paths
    • Both patterns work correctly with the fixed export paths

Summary

This is a high-quality bug fix that resolves critical module resolution issues. The changes are:

  • ✅ Technically correct and align with build output
  • ✅ Follow package.json export best practices
  • ✅ Fix broken functionality without breaking changes
  • ✅ Maintain backward compatibility for all existing usage

Recommendation: Approve and merge - this fixes a critical issue properly.


Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9bf6d7f and 95b864e.

📒 Files selected for processing (1)
  • packages/runtime/package.json (1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
{packages,samples,tests}/**

📄 CodeRabbit inference engine (CLAUDE.md)

Place packages only under packages/, samples/, or tests/

Files:

  • packages/runtime/package.json
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
  • GitHub Check: build-test (20.x, postgresql)
  • GitHub Check: build-test (20.x, sqlite)
  • GitHub Check: claude-review

@ymc9 ymc9 merged commit 4837cf5 into dev Sep 24, 2025
8 checks passed
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