Conversation
202a471 to
4925527
Compare
Review summaryReviewed the full diff (52 files, +2742/−151) and ran the affected suites locally. The extension-point design is well-scoped, but there is one reproducible CI-breaking failure plus a few consistency issues. Verdict: not ready to merge until P0 is fixed. P0 — Blocker (CI failure + broken behavior)New plugin-executor Sessions drop the selected
return {
executorId: input.executorId,
connectionSlug: `executor:${input.executorId}`,
model: input.executorModel ?? input.executorId,
}; // ← thinkingLevel silently droppedSince The PR's own test catches it: I built Fix: mirror the native branch and echo P1 — Should address before mergeThe WorkHub
P2 — RecommendedInconsistent
P3 — Optional
What looks good
Fix the P0 |
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
1. Blocking — Preserve reasoning effort when creating executor sessions
session-catalog-coordinator.ts:1361–1365
The executor branch of #resolveCreateExecution() drops input.thinkingLevel. Both regular session creation and WorkHub creation persist the returned value, so selecting high creates a session without that setting; the executor subsequently receives reasoningEffort: null.
The PR’s own test, plugin executor creation bypasses model resolution and persists the executor route, fails with actual undefined, expected high.
Please preserve the requested thinking level in this branch. The existing test already covers the regression.
2. Important — Keep WorkHub defaults across connection replacement
workhub-new-work-defaults.ts:24–34
Defaults are keyed by the transient client object. Reconnecting creates a new client, so execution reads {}. Meanwhile, the controller reloads defaults only when services or sessionId changes; a same-host reconnect preserves both.
A targeted reproduction using the actual controller and store leaves the picker displaying Codex while execution defaults are empty. Subsequent work creation therefore falls back to the native default model.
Please scope these defaults to a stable host/profile identity, not the transport client. Verify that reconnecting preserves agreement between the displayed selection and the execution target.
3. Important — Preserve native reasoning controls when the plugin is enabled
The plugin claims the model-selection slot whenever onExecutorTargetChange exists. That slot replaces both the native model picker and its thinking selector, but this replacement renders reasoning controls only for Codex models.
Consequently, enabling the plugin removes the reasoning selector for native models, even when they support multiple levels. Rendering the shipped bundle reproduces this: native selection has no reasoning control; Codex selection does.
Please retain the native thinking control when a native model is selected, preferably under the existing host-side owner rather than duplicating its behavior in the plugin.
Simplification worth doing now
Remove the newly added executorTarget alternative from coordinator model configuration. The stated design keeps the coordinator native and applies executor selection only to new work defaults. No production caller found needs this new alternative.
Keeping the existing native-only contract removes an unnecessary protocol branch and preserves that invariant. Retain native coordinator configuration and executor selection for newly created work.
Verdicts: correctness — not acceptable as-is; design — not acceptable as-is because defaults have the wrong lifecycle owner.
Validation: reviewed commit 4925527baa103b8d756116a10c5f8547d233f2c4. Local Codex 0.155.0 with gpt-5.6-luna passed model discovery, two-turn context retention, and cancellation. Plugin tests: 11/11 passed. Targeted repository tests: 144 passed, 1 failed. Two additional probes reproduced findings 2 and 3. Full Electron UI and Windows execution were not tested. CI is also red on architecture-inventory checks, independently of finding 1.
中文
1. Blocking — 创建 executor 会话时丢失推理强度
session-catalog-coordinator.ts:1361–1365
#resolveCreateExecution() 的 executor 分支没有返回 input.thinkingLevel。普通会话和 WorkHub 新建会话均持久化这里的返回值，因此用户选择 high 后，保存的会话仍不含该设置，executor 最终收到 reasoningEffort: null。
PR 自带的 plugin executor creation bypasses model resolution and persists the executor route 测试已经失败：实际为 undefined，预期为 high。
请在该分支保留请求的推理强度；现有测试已能覆盖此回归。
2. Important — WorkHub 重连后显示的选择与实际执行目标不一致
workhub-new-work-defaults.ts:24–34
默认选择以临时 client 对象为键。重连创建新 client 后，执行端读到 {}；但 controller 只在 services 或 sessionId 变化时重新读取，同一 Host 重连不会改变这两个依赖。
使用真实 controller 和 store 的定向复现确认：界面仍显示 Codex，执行端默认选择却已清空，后续新任务因而退回原生默认模型。
请将默认选择归属于稳定的 Host/profile 身份，而非连接对象，并验证重连后界面选择与实际执行目标一致。
3. Important — 启用插件会移除原生模型的推理强度入口
只要存在 onExecutorTargetChange，插件就接管 model-selection slot。这个 slot 替换的是原生模型选择器及推理强度选择器两者，但插件仅为 Codex 模型渲染推理控件。
因此，启用插件后，即使原生模型支持多个推理等级，其选择入口也会消失。直接渲染 PR 的客户端 bundle 已复现：原生模型没有推理控件，Codex 模型有。
请选择原生模型时保留原生推理控件，优先继续由宿主侧维护，避免插件再复制一套行为。
值得现在做的简化
删除 coordinator 模型配置中新加的 executorTarget 分支。PR 的目标是协调会话保持原生执行，仅对新任务应用 executor 默认选择；当前也未找到需要该新增分支的生产调用方。
保留原生 coordinator 配置，以及新任务的 executor 选择即可。这样能删除不必要的协议分支，并保住协调会话必须原生执行的约束。
结论： correctness — 当前不可接受；design — 当前不可接受，默认选择的生命周期归属有误。
验证： 审查提交为 4925527baa103b8d756116a10c5f8547d233f2c4。本机 Codex 0.155.0、gpt-5.6-luna 已通过模型发现、两轮上下文保持和取消测试。插件测试 11/11 通过；仓库定向测试 144 通过、1 失败；另有两个探针复现问题 2、3。未验证完整 Electron GUI 和 Windows 执行。CI 另因架构清单检查失败，与问题 1 独立。
github-actions
Bot
added
effort/XXL
4925527 to
3d8cedb
Compare
Generated-by: Codex
3d8cedb to
3e4caa4
Compare
M4n5ter
left a comment
M4n5ter
left a comment
There was a problem hiding this comment.
English
Re-reviewed 3e4caa463 with deep-code-review + simplify-audit. The three previous findings are resolved: executor creation retains thinking level, same-Host client replacement preserves new-work defaults, and native reasoning controls remain available. The unused coordinator executor configuration path has also been removed; no further material simplification is needed.
Important — align the WorkHub E2E with the new-work-only contract.
CI still fails at workhub-layout.spec.ts:248: it expects the coordination Session's thinking level to become high, but the picker now correctly updates new-work defaults. The later model-selection assertions around lines 279–283 and 323 also expect coordinator mutation. Update/remove these obsolete assertions, preserving the native-window coverage and relying on the existing controller/integration tests for new-work-only semantics. Do not restore coordinator mutation to satisfy the old test.
Validation: rebuilt affected workspace dependencies and desktop main; 159 targeted tests and 12 plugin tests passed. Additional probes confirmed reconnect/default consistency and both reasoning controls. Real Codex gpt-5.6-luna model discovery, two-turn context retention, and cancellation passed. I did not rerun the full Electron E2E locally; its failure above is from CI.
Correctness: acceptable for the reviewed implementation. Design: acceptable. No new product defect confirmed; please resolve the stale E2E failure before merge.
中文
按 deep-code-review + simplify-audit 复审了 3e4caa463。上轮三个问题均已解决：executor 创建保留 thinking level、同 Host 更换 client 后保留新建 Work 默认值、原生 reasoning 控件恢复。协调器多余的 executor 配置路径也已删除，没有新的高价值简化建议。
Important — 将 WorkHub E2E 对齐到“仅影响新建 Work”的契约。
CI 仍然失败：workhub-layout.spec.ts:248 要求协调会话的 thinking level 变成 high，但选择器现在正确地修改新建 Work 默认值。后面的 279–283、323 行也仍然要求修改协调会话模型。请更新或删除这些过时断言，保留原生窗口覆盖，并复用已有 controller/integration 测试验证新建 Work 语义；不要为了旧测试恢复协调会话修改行为。
验证：重新构建相关 workspace 依赖及 desktop main；159 项定向测试、12 项插件测试通过。补充探针验证了重连前后默认值一致，以及两类 reasoning 控件。真实 Codex gpt-5.6-luna 的模型发现、两轮上下文保持、取消均通过。本地未重跑完整 Electron E2E，上述失败依据来自 CI。
Correctness：本轮审查的实现可接受。Design：可接受。未确认新的产品缺陷；合并前仍需解决过时 E2E 导致的失败。
3 participants
Summary
scripts/pluginsscripts/pluginsfrom ASF source archives and desktop packagingThe integration does not bundle Codex, OpenAI credentials, protocol schemas, or brand assets. Codex-specific model loading and UI behavior remain inside the plugin directory; Maka exposes only reusable executor/model extension points and the Session data path needed by any executor plugin.
Behavior
Verification
@maka/desktoptypecheck@maka/desktopproduction build, including Renderer entry and third-party notice checksCompatibility
main