fix(desktop): preserve policy-less plugin discovery - #4019
Conversation
Signed-off-by: Dash <125997726+dashhuang@users.noreply.github.com>
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. |
|
| Filename | Overview |
|---|---|
| apps/desktop/src/main/mcp-integrations/codexBuiltinToolPolicy.ts | Changes missing or malformed frozen policy handling from fail-open to fail-closed, but also denies the policy-less provider-discovery phase. |
| apps/desktop/src/main/mcp-integrations/tests/codexBuiltinToolPolicy.test.ts | Updates focused helper tests for fail-closed behavior, but does not cover provider registration before thread policy injection. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Codex or Pi provider discovery] --> B["Context uses vendorOptions: {}"]
B --> C[Provider isEnabled]
C --> D[isFrozenBuiltinPluginAllowed]
D --> E[No allowlist or disabled list]
E --> F[Return false]
F --> G[Skip provider registration]
G --> H[Tool remains unavailable after thread policy is injected]
Prompt To Fix All With AI
### Issue 1
apps/desktop/src/main/mcp-integrations/codexBuiltinToolPolicy.ts:41
**Provider registration is blocked**
During Codex and Pi startup, provider discovery calls `isEnabled` with empty `vendorOptions` before a thread-specific capability snapshot exists. This new fallback returns `false`, so discovery skips every controlled built-in before its server is registered. Those tools remain unavailable even after a valid per-thread policy is supplied, contrary to the existing design that registers ordinary plugins first and enforces their policy at call time.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Reviews (1): Last reviewed commit: "fix(desktop): fail closed for missing pl..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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: c8087e664a
ℹ️ 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".
|
感谢 review。该问题成立：当前改动把 discovery 阶段的空 vendorOptions 与调用阶段的缺失快照混为一谈，确实会导致 provider 在策略注入前被永久跳过。我们会改为显式的 capability snapshot/discovery 语义，保留 discovery 注册与调用前 fail-closed，并补 provider 注册回归测试。当前 PR 暂不 resolve 该反馈。 |
|
@dashhuang 👋 这个 PR 还有 2 条 review conversation 没 resolve(apps/desktop/src/main/mcp-integrations/codexBuiltinToolPolicy.ts),auto-review 因此暂时跳过、没法继续审查 / 合并。 如果你已经按评论改完或回应了,请到对应 thread 上点 Resolve conversation;全部 resolve 后,下一轮 auto-review 会自动重新审查这个 PR。 |
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
格式门未通过
Description 缺仓库要求的三段标题（中文段落名需原样出现）：
- 这次改了什么
- 怎么验证的
- 风险
当前正文用的是 Problem / Change / Validation / Risk，格式门按标题匹配，不会把英文小标题当成这三段。
请按 .github/PULL_REQUEST_TEMPLATE.md 改成上述三段后再推（或编辑 PR 描述）。本轮不进入代码审查。
另外：CI 当前是红的（Linux/Windows unit tests、verify、client-ci），修描述时一并处理。
Signed-off-by: Dash <125997726+dashhuang@users.noreply.github.com>
|
已处理该 review：provider discovery 和普通任务在没有冻结策略时继续保持可用；冻结策略仍只在显式 allowlist/disabled list 存在时生效。当前 PR 已收缩为保持既有策略语义，完整 capability snapshot 重构不在本 PR 内。对应提交：1a763a985。 |
|
Codex Review: Didn't find any major issues. Another round soon, please! 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". |
MagicLizi
left a comment
MagicLizi
left a comment
There was a problem hiding this comment.
[P1] apps/desktop/src/main/mcp-integrations/__tests__/codexBuiltinToolPolicy.test.ts:19
标题仍是 fail closed，当前 head 只改了用例名，断言仍是 isFrozenBuiltinPluginAllowed(undefined, 'collab') → true，畸形 disabled 列表 ['collab', 1] 同样 true。共享实现 !readDisabledBuiltinPluginIds(...)?.includes(pluginId) 在缺/畸形 policy 时放行。正文已改口「保持既有策略语义」，与标题不符。
请二选一并改到一致：
- 若本 PR 仍是 fail-closed：调用期缺 snapshot / 畸形列表应
toBe(false)；discovery / 普通任务无冻结策略若要保持注册，应与调用期闸门拆开。 - 若本 PR 收缩为保持 fail-open：改掉标题和「fail closed」表述，不要把缺 policy 放行伪装成已完成的能力快照重构。
dashhuang
changed the title
这些问题已在当前 head 的后续 commit 中修复(格式门/安全门已通过),自动 dismiss 旧的 CHANGES_REQUESTED 以解除合并阻塞。
3 participants
这次改了什么
保持普通任务和 provider discovery 在缺少冻结策略时的现有可用性；显式 Bot Profile allowlist / disabled list 继续生效。本 PR 不宣称完成通用 capability snapshot 重构。
怎么验证的
风险
本 PR 不改变缺少冻结策略时的默认放行语义，避免普通任务和 provider discovery 回归。通用插件能力快照仍需后续专门改动。