fix: deduplicate nested pino in global paperclipai install - #1355
Conversation
|
You do not have enough credits to review this pull request. Please purchase more credits to continue. |
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe changes add a post-install cleanup step to the npm-globals installation script that reads package.json overrides and removes nested occurrences of overridden packages from Bun's global node_modules directory. Corresponding test assertions verify this deduplication behavior. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Poem
✨ 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 |
Mesa DescriptionTL;DRDeduplicated What changed?
Description generated by Mesa. Update settings |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a deduplication step in the install-npm-globals.sh script to remove a nested pino directory within @paperclipai/server, which prevents crashes caused by Symbol mismatches. The review feedback recommends using rm -rf combined with || true for the deletion to ensure the script is robust and consistent with existing error handling patterns.
| # loggers with its own nested copy — different Symbol instances cause a crash. | ||
| NESTED_PINO="${HOME}/.bun/install/global/node_modules/@paperclipai/server/node_modules/pino" | ||
| if [ -d "$NESTED_PINO" ]; then | ||
| rm -r "$NESTED_PINO" |
There was a problem hiding this comment.
To ensure the script is robust and consistent with the error handling patterns used elsewhere in this file (e.g., lines 41, 49, 60), consider using rm -rf and appending || true. This prevents the script from exiting prematurely if the deletion fails—which is a non-critical cleanup step—and avoids potential interactive prompts if files are write-protected.
| rm -r "$NESTED_PINO" | |
| rm -rf "$NESTED_PINO" || true |
References
- Maintain consistency with established patterns for writing scripts that are extracted from Nix expressions.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adds a post-install cleanup step to Bun’s global npm package installation script to remove a duplicated nested pino dependency under @paperclipai/server, avoiding runtime crashes caused by Symbol mismatches between two module instances.
Changes:
- Add a deduplication step that deletes
@paperclipai/server/node_modules/pinoafterbun install --global. - Document the root cause (Symbol mismatch between nested and top-level
pino) inline in the script.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| rm -r "$NESTED_PINO" | ||
| echo "Removed nested pino from @paperclipai/server (deduplicated)" |
There was a problem hiding this comment.
rm -r runs under set -e and will abort the whole install if removal fails (permissions, transient FS issues, etc.). Consider using a non-interactive forced remove (rm -rf -- ...) and only printing the "Removed" message when the command succeeds (or emit a warning but keep the script succeeding).
| rm -r "$NESTED_PINO" | |
| echo "Removed nested pino from @paperclipai/server (deduplicated)" | |
| if rm -rf -- "$NESTED_PINO"; then | |
| echo "Removed nested pino from @paperclipai/server (deduplicated)" | |
| else | |
| echo "Warning: failed to remove nested pino from @paperclipai/server" >&2 | |
| fi |
| NESTED_PINO="${HOME}/.bun/install/global/node_modules/@paperclipai/server/node_modules/pino" | ||
| if [ -d "$NESTED_PINO" ]; then | ||
| rm -r "$NESTED_PINO" |
There was a problem hiding this comment.
This hardcodes Bun’s global install path to ~/.bun/install/global/.... If the user has Bun installed in a non-default location (e.g., via BUN_INSTALL or similar), this cleanup won’t run and the crash may persist. Consider deriving the global root via Bun itself (e.g., a bun pm -g query) or reusing a single computed global dir variable used by both GLOBAL_PKG and this path.
| rm -r "$NESTED_PINO" | ||
| echo "Removed nested pino from @paperclipai/server (deduplicated)" |
There was a problem hiding this comment.
There’s ShellSpec coverage for this script (spec/npm_globals_spec.sh), but it currently won’t catch regressions to this new deduplication step. Please add/extend a spec assertion to verify the nested pino path cleanup logic is present (and ideally that it’s guarded and non-fatal).
| rm -r "$NESTED_PINO" | |
| echo "Removed nested pino from @paperclipai/server (deduplicated)" | |
| if rm -r "$NESTED_PINO"; then | |
| echo "Removed nested pino from @paperclipai/server (deduplicated)" | |
| else | |
| echo "Warning: failed to remove nested pino from @paperclipai/server; continuing" >&2 | |
| fi |
Summary
After
bun install --global, remove the nestedpinofrom@paperclipai/server/node_modules/so there's only one copy.Root cause
@paperclipai/servergets its own nestedpino@9.14.0, whilepino-http@10.5importsstringifySymfrom the top-levelpino@9.14.0. Same version, but different module instances = different Symbol references. The logger created by the nested pino doesn't have the top-level pino's Symbol, sologger[stringifySym]is undefined → crash.Fix
Delete
~/.bun/install/global/node_modules/@paperclipai/server/node_modules/pinoafter install so everything resolves to one pino instance.Tested
10/10 requests returned 200 after deduplication.
Summary by cubic
Deduplicates all overridden packages in global installs by removing nested copies under
node_modulesafterbun install --global, ensuring a single version (e.g.,pino). Preventspino-httpstringifySymcrashes and similar symbol conflicts; 10/10 requests now return 200.Written for commit 116329b. Summary will update on new commits.