-
Notifications
You must be signed in to change notification settings - Fork 714
Stage #9269
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
#9268) * fix: use the correct rebuild patterns for all native packages * Update .deploy/mcp/Dockerfile Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com> * fix: add review suggestions --------- Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
) * chore(deps): bump @modelcontextprotocol/sdk from 1.15.1 to 1.24.0 Bumps [@modelcontextprotocol/sdk](https://github.com/modelcontextprotocol/typescript-sdk) from 1.15.1 to 1.24.0. - [Release notes](https://github.com/modelcontextprotocol/typescript-sdk/releases) - [Commits](modelcontextprotocol/typescript-sdk@1.15.1...1.24.0) --- updated-dependencies: - dependency-name: "@modelcontextprotocol/sdk" dependency-version: 1.24.0 dependency-type: direct:production ... Signed-off-by: dependabot[bot] <[email protected]> * fix: add commands for native nodejs modules * fix: add typos spelling * fix: add typos spelling --------- Signed-off-by: dependabot[bot] <[email protected]> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Co-authored-by: rolandm99 <[email protected]>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the ✨ 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 |
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No issues found across 4 files
Greptile OverviewGreptile SummaryThis PR merges changes from develop to stage, including a dependency bump for Key Changes:
Critical Issue Found: Impact: Confidence Score: 1/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant Developer
participant Docker as Docker Build
participant Yarn as Yarn Install
participant PostInstall as postinstall.js
participant NPM as npm rebuild
participant NativeModules as Native Modules (bcrypt, better-sqlite3)
Developer->>Docker: Build Docker image
Docker->>Yarn: yarn install --frozen-lockfile --ignore-scripts
Yarn-->>Docker: Dependencies installed (scripts skipped)
Docker->>PostInstall: yarn postinstall.manual (ts-node .scripts/postinstall.js)
PostInstall->>PostInstall: Find postinstall scripts in node_modules
PostInstall->>PostInstall: Run found postinstall scripts
Note over PostInstall,NativeModules: ISSUE: postinstall.js attempts to rebuild native modules<br/>using "yarn add {package} --force" from inside<br/>node_modules/{package} directory - INCORRECT!
PostInstall->>NativeModules: yarn add bcrypt --force (from wrong directory)
NativeModules-->>PostInstall: Rebuild fails or incomplete
Note over Docker,NPM: MISSING: Explicit npm rebuild command<br/>that was removed in this PR
Docker->>Docker: Continue build (with potentially broken native modules)
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
3 files reviewed, 4 comments
|
|
||
| RUN yarn install --network-timeout 1000000 --frozen-lockfile --ignore-scripts \ | ||
| && npm rebuild bcrypt better-sqlite3 --build-from-source \ | ||
| && yarn postinstall.manual \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The postinstall.js script (line 96) runs yarn add ${packageName} --force from inside node_modules/{package}, which is incorrect. This will fail to properly rebuild native modules. The previous approach using npm rebuild bcrypt better-sqlite3 --build-from-source was correct and should be restored.
| && yarn postinstall.manual \ | |
| && node ./.scripts/postinstall.js && npm rebuild bcrypt better-sqlite3 --build-from-source \ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .deploy/mcp-auth/Dockerfile
Line: 68:68
Comment:
**logic:** The postinstall.js script (line 96) runs `yarn add ${packageName} --force` from inside `node_modules/{package}`, which is incorrect. This will fail to properly rebuild native modules. The previous approach using `npm rebuild bcrypt better-sqlite3 --build-from-source` was correct and should be restored.
```suggestion
&& node ./.scripts/postinstall.js && npm rebuild bcrypt better-sqlite3 --build-from-source \
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| RUN yarn install --network-timeout 1000000 --frozen-lockfile --ignore-scripts --production \ | ||
| && npm rebuild bcrypt better-sqlite3 --build-from-source \ | ||
| && yarn postinstall.manual \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Same issue in production dependencies stage - the postinstall.js script has incorrect rebuild logic. Restore the explicit npm rebuild command.
| && yarn postinstall.manual \ | |
| && node ./.scripts/postinstall.js && npm rebuild bcrypt better-sqlite3 --build-from-source \ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .deploy/mcp-auth/Dockerfile
Line: 119:119
Comment:
**logic:** Same issue in production dependencies stage - the postinstall.js script has incorrect rebuild logic. Restore the explicit `npm rebuild` command.
```suggestion
&& node ./.scripts/postinstall.js && npm rebuild bcrypt better-sqlite3 --build-from-source \
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| RUN yarn install --network-timeout 1000000 --frozen-lockfile --ignore-scripts \ | ||
| && npm rebuild bcrypt --build-from-source \ | ||
| && yarn postinstall.manual \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: The postinstall.js script cannot properly rebuild native modules - it runs yarn add from inside the package directory instead of running npm rebuild from the root. Restore the explicit rebuild command.
| && yarn postinstall.manual \ | |
| && node ./.scripts/postinstall.js && npm rebuild bcrypt --build-from-source \ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .deploy/mcp/Dockerfile
Line: 114:114
Comment:
**logic:** The postinstall.js script cannot properly rebuild native modules - it runs `yarn add` from inside the package directory instead of running `npm rebuild` from the root. Restore the explicit rebuild command.
```suggestion
&& node ./.scripts/postinstall.js && npm rebuild bcrypt --build-from-source \
```
How can I resolve this? If you propose a fix, please make it concise.|
|
||
| RUN yarn install --network-timeout 1000000 --frozen-lockfile --ignore-scripts --production \ | ||
| && npm rebuild bcrypt --build-from-source \ | ||
| && yarn postinstall.manual \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
logic: Same issue in production dependencies - restore the explicit npm rebuild command to properly build native modules.
| && yarn postinstall.manual \ | |
| && node ./.scripts/postinstall.js && npm rebuild bcrypt --build-from-source \ |
Prompt To Fix With AI
This is a comment left during a code review.
Path: .deploy/mcp/Dockerfile
Line: 164:164
Comment:
**logic:** Same issue in production dependencies - restore the explicit `npm rebuild` command to properly build native modules.
```suggestion
&& node ./.scripts/postinstall.js && npm rebuild bcrypt --build-from-source \
```
How can I resolve this? If you propose a fix, please make it concise.
PR
Please note: we will close your PR without comment if you do not check the boxes above and provide ALL requested information.
Summary by cubic
Fixes native Node.js module rebuilds in MCP and MCP Auth Docker images using a manual postinstall, improving build reliability on Alpine. Updates @modelcontextprotocol/sdk to 1.24.0 and aligns related dependencies; adds a few cspell dictionary entries.
Bug Fixes
Dependencies
Written for commit 4a6ba91. Summary will update automatically on new commits.