docs(server): include plugin setup in source startup - #692
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@codex review |
|
Codex Review: Didn't find any major issues. Nice work! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
|
|
||
| ```bash | ||
| pnpm install --frozen-lockfile | ||
| npm ci --prefix ../../plugins/codex-security/mcp-app --no-audit --no-fund |
There was a problem hiding this comment.
npm is correct for that line.
The commands intentionally use two package managers because they target two separate packages:
sdk/typescriptdeclares pnpm 11.9.0 as its package manager, so its install and build commands should use pnpm. sdk/typescript/package.jsonL15-L18 sdk/typescript/README.mdL951-L959plugins/codex-security/mcp-apphas an npmpackage-lock.jsonwith lockfile version 3, sonpm ci --prefix ../../plugins/codex-security/mcp-appinstalls that package’s locked dependencies correctly. plugins/codex-security/mcp-app/package-lock.jsonL1-L10- Existing SDK scripts also invoke npm for operations in that nested MCP app, confirming the package-manager boundary is intentional. sdk/typescript/package.jsonL57-L62
So the current documentation should remain:
pnpm install --frozen-lockfile
npm ci --prefix ../../plugins/codex-security/mcp-app --no-audit --no-fundNo repository change was necessary, so I did not create a commit or follow-up PR.
Checks
- ✅
find plugins/codex-security/mcp-app -maxdepth 2 \( -name package.json -o -name 'package-lock.json' -o -name 'pnpm-lock.yaml' \) -print - ✅
node -e "for (const p of ['sdk/typescript/package.json','plugins/codex-security/mcp-app/package.json']) {const j=require('./'+p); console.log(p, {packageManager:j.packageManager, scripts:j.scripts})}" - ✅
git status --short --branch(working tree clean)
There was a problem hiding this comment.
@codex what if the user only uses pnpm and not npm though? How does it make sense to mix package managers?
There was a problem hiding this comment.
Summary
- Clarified that the source build intentionally requires both package managers because the SDK is locked with pnpm, while the nested MCP app has its own npm lockfile. This avoids implying that the npm command is interchangeable with pnpm. sdk/typescript/README.mdL951-L954
- Preserved the reproducible installation commands for both packages and the existing build/start sequence. sdk/typescript/README.mdL956-L961
- Committed the change as
282b713(docs: clarify source build package managers) and prepared a follow-up PR titled “docs: clarify source build package managers.”
Testing
- ✅
pnpm exec prettier --check README.md - ✅
git diff --check - ✅
git status --short --branch(clean working tree after commit)
mldangelo-oai
left a comment
There was a problem hiding this comment.
Reviewed 002eeb7 against current main. This is now superseded: the source-start recipe on main already installs the MCP dependencies and runs build:plugin. It also uses pnpm consistently after #725. The npm command in this branch is stale because the MCP app now has a pnpm lockfile.
I recommend closing this PR as superseded instead of resolving the README conflict and restoring the old npm command. No additional implementation is needed for the original problem.
|
Closing as superseded. Main already includes the MCP dependency setup and plugin-build steps, and #725 updated the source-start recipe to use pnpm consistently. The original setup gap is covered; restoring this branch's npm command would be obsolete. |
Summary
The findings-server source-start recipe omits the generated plugin required by
storage initialization. Document the existing prerequisites before starting the
compiled server.
Changes
documented in the SDK testing guide.
pnpm run build:pluginbefore the existing package build and server start.Testing
prettier --check README.mdpassed with Prettier 3.2.5.git diff --checkpassed.dependencies reused read-only, with package scripts invoked through
node --run. The package build succeeded, but compiled startup failed beforereadiness with the missing-plugin error. After
node --run build:plugin, thesame compiled entrypoint served the expected HTTP 501 stubs and initialized
SQLite.
documentation-only change.
Risk and rollout
Documentation only. The added commands are existing source-build steps and apply
across supported platforms. No runtime, API, default, or published-package
behavior changes; no migration is required.
Public disclosure review