Release v1.58.0 official plugin compatibility - #26
Conversation
PR Summary by QodoRelease v1.58.0: official plugin compatibility & hardened MCP safety
AI Description
Diagram
High-Level Assessment
Files changed (54)
|
![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: fb5dcd02ef
ℹ️ 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".
| const secretRuntimeArgumentFlag = /^--?(?:token|api[-_]?key|apikey|password|secret|credential|authorization|bearer)$/i; | ||
| const secretRuntimeArgumentInline = /^(--?(?:token|api[-_]?key|apikey|password|secret|credential|authorization|bearer))[=:].*$/i; |
There was a problem hiding this comment.
Credential flags containing qualifiers are not matched by these exact-name expressions, so common arguments such as --auth-token secret, --client-secret=secret, or --private-key secret remain unchanged in the runtime-plan JSON, Markdown, review bundle, and release evidence. Because these artifacts are intended to be portable and persisted, broaden the credential-key detection rather than limiting it to the listed single-word names.
Useful? React with 👍 / 👎.
| highRiskServerCount: 0, | ||
| findings: { fail: 1, warn: 0, total: 1 } | ||
| }, | ||
| servers: [], |
There was a problem hiding this comment.
When mcpServers is malformed or escapes the package, this helper returns a failed plan with no servers. buildDoctorRuntimePolicyReport derives deny, but buildRecommendation handles the zero-server case first and replaces it with an allow recommendation saying runtime probing is unnecessary. Thus the policy output simultaneously has fail status and recommends allow for an unsafe manifest; preserve a deny recommendation for failed zero-server plans.
Useful? React with 👍 / 👎.
Code Review by Qodo
1. Unhandled MCP path throw
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
| const containedDirectMcpPath = await resolveContainedPackagePath(rootPath, directMcpPath); | ||
|
|
||
| if (!containedDirectMcpPath) { | ||
| throw new Error("MCP config path resolves outside the package root."); |
There was a problem hiding this comment.
1. Unhandled mcp path throw 🐞 Bug ☼ Reliability
readMcpConfigPath now throws on unsafe .mcp.json containment, but readMcpServerNames awaits it outside a try/catch; an unsafe path can escape and abort or misclassify downstream compatibility checks that call readMcpServerNames.
Agent Prompt
### Issue description
`readMcpConfigPath()` now throws when the resolved MCP config escapes the package root. `readMcpServerNames()` calls `readMcpConfigPath()` without handling exceptions, so unsafe-path packages can throw out of `readMcpServerNames()` and disrupt compatibility checks.
### Issue Context
This behavior was introduced when `readMcpConfigPath()` started enforcing canonical containment by throwing on escapes.
### Fix Focus Areas
- src/compatibility/compatibility-matrix.ts[61-100]
- src/compatibility/compatibility-matrix.ts[158-173]
### Suggested fix
Wrap the `readMcpConfigPath()` call inside `readMcpServerNames()` in a try/catch and return `[]` (or a structured failure) when containment fails. This keeps “duplicate server name” checks deterministic even for unsafe packages, while preserving the thrown-error behavior for callers that explicitly want a failing result (e.g., `checkGenericMcp`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| if (configuredHooks === undefined) { | ||
| const defaultPath = "./hooks/hooks.json"; | ||
| const resolved = await resolveSafePackagePath(rootPath, defaultPath); | ||
| if (!resolved) return [invalidHookPath(manifestPath)]; | ||
| try { | ||
| if (!(await stat(resolved.path)).isFile()) return []; | ||
| } catch { | ||
| return []; |
There was a problem hiding this comment.
2. Default hooks errors ignored 🐞 Bug ≡ Correctness
When manifest.hooks is absent, validatePluginHooks returns [] for any stat() error on ./hooks/hooks.json, treating unreadable/broken paths the same as a missing optional file and skipping hook validation findings.
Agent Prompt
### Issue description
In the default hooks discovery path (`manifest.hooks === undefined`), any `stat()` error causes an early `return []`. This conflates “default hooks file does not exist” (intended to be optional) with other filesystem errors (permission denied, IO errors, broken symlinks, etc.), which should likely produce a failure finding because Doctor cannot validate the hooks file it attempted to discover.
### Issue Context
`validateHookFile()` already fail-closes on stat/read errors for explicitly configured hooks sources, but the default discovery path is currently more permissive.
### Fix Focus Areas
- src/core/plugin-hooks.ts[194-227]
- src/core/plugin-hooks.ts[229-244]
### Suggested fix
Handle `ENOENT` as “no default hooks” (return `[]`), but for other errors emit a failure finding (e.g., reuse `plugin.hook.missing_file` with evidence indicating unreadable/uninspectable), and consider failing when the path exists but is not a regular file instead of returning `[]`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ee4f6783d2
ℹ️ 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".
| const authorizationBearerHeader = /^authorization\s*:\s*bearer\s+\S+/i; | ||
| const inlineAuthorizationBearerHeader = /^(?:--header|-H)=authorization\s*:\s*bearer\s+\S+/i; |
There was a problem hiding this comment.
When an MCP command supplies a non-Bearer authorization scheme, such as --header "Authorization: Basic dXNlcjpwYXNz" or --header=Authorization: Basic ..., these Bearer-only expressions leave the complete credential in runtime-plan JSON, Markdown, review bundles, and release evidence. Match and redact credential-bearing Authorization headers regardless of authentication scheme while retaining the raw value only for digest calculation.
Useful? React with 👍 / 👎.
| status: !normalizedConfig.ok || highRiskServerCount > 0 || security.status === "fail" | ||
| ? "fail" as const | ||
| : security.status === "warn" | ||
| ? "warn" as const | ||
| : "pass" as const, | ||
| exitCode: (highRiskServerCount > 0 ? 1 : 0) as 0 | 1, | ||
| exitCode: (!normalizedConfig.ok || highRiskServerCount > 0 || security.status === "fail" ? 1 : 0) as 0 | 1, |
There was a problem hiding this comment.
This also downgrades failed plans that contain a valid nonzero server: for example, a package with a local server and plugin.security.prompt_injection_text is marked failed here, but buildRisk does not attach that package-wide finding to the server because its message lacks the server name, so buildDoctorRuntimePolicyReport derives review and a warning instead of deny. Propagate deny-level plan findings into the overall runtime-policy decision rather than deriving it solely from per-server risks.
Useful? React with 👍 / 👎.
1 participant
Summary\n\n- add official MCP layout normalization for direct maps, mcp_servers, and legacy mcpServers\n- validate official plugin metadata, apps, component paths, and lifecycle hooks without executing package content\n- enforce canonical package containment and fail-closed runtime planning across previews, Inspector, audit, and runtime flows\n- redact portable runtime credentials while preserving exact approval-digest binding\n- prepare synchronized v1.58.0 docs, changelog, package metadata, and release pins\n\n## Security\n\n- reject lexical and canonical link escapes before MCP config reads\n- reject malformed or ambiguous MCP declarations with structured failure and exit code 1\n- harden lifecycle-hook remote-pipe detection and finding precision\n- remove Windows shell execution from doctor size npm-pack measurement\n\n## Verification\n\n- npm run release-check\n- npm test: 1,090 passed; 6 expected skips\n- npx tsc -p tsconfig.json --noEmit\n- npm run build\n- npm pack --dry-run: 201 intended files\n- doctor corpus: 4/4 expectations passed\n- independent spec, quality/security, and final branch reviews: approved\n