fix: narrow npm publish surface to the module graph#1395
Conversation
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughUpdates the npm package publish allowlist in Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bd2aec48ed
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "files": [ | ||
| ".agents/", | ||
| ".claude-plugin/", | ||
| ".codex/", | ||
| ".codex-plugin/", |
There was a problem hiding this comment.
The new files allowlist drops schemas/, but runtime code in scripts/lib/install/config.js, scripts/lib/install-state.js, and scripts/lib/state-store/schema.js reads JSON schemas from that directory during normal CLI flows (install planning/apply, install-state validation, and state-store validation). With this commit’s publish surface, npm tarballs omit those schema files, so users installing from npm will hit file-read failures at runtime when these paths execute. Please add schemas/ back to the publish surface.
Useful? React with 👍 / 👎.
|
ECC bundle files are already tracked in this repository. Skipping generation of another bundle PR. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR narrows the Confidence Score: 5/5Safe to merge — the publish surface narrowing is correct and the regression test logic is sound. All findings are P2 style suggestions. No logic errors, missing assets, or correctness issues were identified in the module graph alignment or the isCoveredByAncestor deduplication logic. tests/scripts/npm-publish-surface.test.js — extraPaths coupling comment and JSON parse guard Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[manifests/install-modules.json] -->|flatMap paths| B[module graph paths]
C[extraPaths hardcoded in test] --> D[combined Set]
B --> D
D -->|filter isCoveredByAncestor| E[expectedPublishPaths sorted]
F[package.json files] -->|normalizePublishPath + sort| G[actualPublishPaths]
E -->|deepStrictEqual| H{Test 1: surface contract}
H -->|pass| I[✓ files match module graph]
H -->|fail| J[✗ drift detected]
F -->|npm pack dry-run| K[packaged file list]
K -->|assert includes| L{Test 2: required paths present}
K -->|assert excludes| M{Test 3: excluded paths absent}
L --> N[✓ runtime assets in tarball]
M --> O[✓ dev-only content excluded]
Reviews (1): Last reviewed commit: "fix: keep runtime schemas in npm package" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const extraPaths = [ | ||
| "manifests", | ||
| "scripts/ecc.js", | ||
| "scripts/catalog.js", | ||
| "scripts/claw.js", | ||
| "scripts/doctor.js", | ||
| "scripts/status.js", | ||
| "scripts/sessions-cli.js", | ||
| "scripts/install-apply.js", | ||
| "scripts/install-plan.js", | ||
| "scripts/list-installed.js", | ||
| "scripts/skill-create-output.js", | ||
| "scripts/repair.js", | ||
| "scripts/harness-audit.js", | ||
| "scripts/session-inspect.js", | ||
| "scripts/uninstall.js", | ||
| "scripts/gemini-adapt-agents.js", | ||
| "scripts/codex/merge-codex-config.js", | ||
| "scripts/codex/merge-mcp-config.js", | ||
| ".codex-plugin", | ||
| ".mcp.json", | ||
| "install.sh", | ||
| "install.ps1", | ||
| "schemas", | ||
| "agent.yaml", | ||
| "VERSION", | ||
| ] |
There was a problem hiding this comment.
extraPaths creates a third file developers must update (alongside package.json and manifests/install-modules.json) whenever a new standalone runtime script is added. When the deepStrictEqual on line 95 fails, the diff output lists path strings with no indication of where the fix belongs. A small comment here would make the coupling explicit for future contributors.
| const extraPaths = [ | |
| "manifests", | |
| "scripts/ecc.js", | |
| "scripts/catalog.js", | |
| "scripts/claw.js", | |
| "scripts/doctor.js", | |
| "scripts/status.js", | |
| "scripts/sessions-cli.js", | |
| "scripts/install-apply.js", | |
| "scripts/install-plan.js", | |
| "scripts/list-installed.js", | |
| "scripts/skill-create-output.js", | |
| "scripts/repair.js", | |
| "scripts/harness-audit.js", | |
| "scripts/session-inspect.js", | |
| "scripts/uninstall.js", | |
| "scripts/gemini-adapt-agents.js", | |
| "scripts/codex/merge-codex-config.js", | |
| "scripts/codex/merge-mcp-config.js", | |
| ".codex-plugin", | |
| ".mcp.json", | |
| "install.sh", | |
| "install.ps1", | |
| "schemas", | |
| "agent.yaml", | |
| "VERSION", | |
| ] | |
| // extraPaths: runtime assets that ship in the tarball but are not represented | |
| // in manifests/install-modules.json (e.g. top-level CLI entry-points, install | |
| // scripts, and config singletons). Keep this list in sync with package.json | |
| // "files" whenever a new standalone script is added. | |
| const extraPaths = [ | |
| "manifests", | |
| "scripts/ecc.js", | |
| "scripts/catalog.js", | |
| "scripts/claw.js", | |
| "scripts/doctor.js", | |
| "scripts/status.js", | |
| "scripts/sessions-cli.js", | |
| "scripts/install-apply.js", | |
| "scripts/install-plan.js", | |
| "scripts/list-installed.js", | |
| "scripts/skill-create-output.js", | |
| "scripts/repair.js", | |
| "scripts/harness-audit.js", | |
| "scripts/session-inspect.js", | |
| "scripts/uninstall.js", | |
| "scripts/gemini-adapt-agents.js", | |
| "scripts/codex/merge-codex-config.js", | |
| "scripts/codex/merge-mcp-config.js", | |
| ".codex-plugin", | |
| ".mcp.json", | |
| "install.sh", | |
| "install.ps1", | |
| "schemas", | |
| "agent.yaml", | |
| "VERSION", | |
| ] |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
|
||
| const packOutput = JSON.parse(result.stdout) | ||
| const packagedPaths = new Set(packOutput[0]?.files?.map((file) => file.path) ?? []) |
There was a problem hiding this comment.
JSON.parse(result.stdout) will throw an unguarded SyntaxError if npm emits anything non-JSON to stdout (e.g. a warning line in an older npm version, or output from prepack reaching stdout). The runTest wrapper will catch it, but the error message will be "Unexpected token …" with no clue that it is a parse failure. Wrapping the parse makes failures self-describing:
| const packOutput = JSON.parse(result.stdout) | |
| const packagedPaths = new Set(packOutput[0]?.files?.map((file) => file.path) ?? []) | |
| let packOutput | |
| try { | |
| packOutput = JSON.parse(result.stdout) | |
| } catch { | |
| assert.fail(`npm pack --dry-run did not produce valid JSON.\nstdout: ${result.stdout.slice(0, 500)}`) | |
| } |
1 participant
Summary\n- reduce the npm publish surface to the module-graph-backed runtime allowlist\n- add a dedicated publish-surface regression around package.json files and npm pack output\n- keep required runtime assets like scripts/catalog.js and shipped plugin manifests in the tarball\n\n## Testing\n- node tests/scripts/npm-publish-surface.test.js\n- node tests/scripts/build-opencode.test.js\n- node tests/scripts/ecc.test.js\n- npm test\n\nCloses #445
Summary by cubic
Narrows the npm package to the module-graph-backed runtime allowlist and required assets to prevent bloat and leaks. Adds a regression test to lock the publish surface via package.json and npm pack.
package.json.filesto the module graph plus an explicit runtime allowlist; keeps runtime schemas and assets (scripts likescripts/catalog.js, plugin manifests,.mcp.json,.gemini).tests/scripts/npm-publish-surface.test.jsto assertfilesmatches the module graph and thatnpm pack --dry-runincludes required paths (e.g.,schemas/install-state.schema.json) and excludes non-runtime content.tests/scripts/build-opencode.test.jsto expect.opencode/in the publish list.Written for commit c826305. Summary will update on new commits.
Summary by CodeRabbit
Chores
Tests