refactor: retire the Deep Research workflow - #5554
Conversation
Remove the specialized research runtime, protocol, tools, storage writer, desktop entry points and progress UI. Preserve legacy chats and report artifacts under their existing permission boundaries, and advance the Runtime Host compatibility epoch. Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
Generated-by: Codex
c339bea to
b3f7d78
Compare
jackwener
left a comment
There was a problem hiding this comment.
Seat: kabi-opus-review-orchestrator. Review of exact head b3f7d78b03e2b3dd17a8df9bec6a249c9daebe50, based on 1c18700a7 (behind main by 0). All 14 checks green, 1 skipped. Still a draft.
Verdict
No P0–P2. 1×P3. This is a careful retirement. The part that usually goes wrong in a 7,000-line deletion — something still reaching for what was removed — does not go wrong here, and I verified that by building rather than by reading.
The strongest evidence I can offer: a clean build from nothing
npm run clean && npm run build across all ten workspaces: 0 errors. That matters more than a passing test suite for a deletion, because a stale dist will happily resolve an import whose source no longer exists — the removed modules have to be genuinely absent from the emitted output before the compile means anything. They were.
Full workspace suite: desktop, cli, core, ui, mcp, computer-use, website, eval all passed. runtime, storage and runtime-host each reported failures — six in total, and all six reproduce on origin/main without this PR:
| Failing test | Workspace | On 1c18700a7 |
|---|---|---|
keeps direct-only tools out of the cell snapshot |
runtime | fails |
keeps provider-native tools out of the cell snapshot |
runtime | fails |
uses normal partial-execution semantics before an unknown tool failure |
runtime | fails |
does not dispatch tools inherited from Object.prototype |
runtime | fails |
rejects a second authority for the same storage root in another process |
storage | fails |
WorkHub v2 keeps its attachment and browser tool ceiling visible in direct and Code Mode |
runtime-host | fails |
I checked each one against main individually rather than assuming, because four of them are tool-registry tests and a tool-removing PR is exactly the thing that would break them. None are attributable to this change. They are pre-existing in my environment; CI is green, so they are likely local to it.
The migration claim is true, and I confirmed it by breaking it
The description says the retained historical table is what lets existing State Roots upgrade, and that the regression test "fails without the retained historical table definition." A test that would pass anyway proves nothing, so I removed the workflow_deep_research_events block from sqlite-workflow-schema.ts and rebuilt.
preserves retired research events through upgrade, reopen, and backup fails — along with four other migration tests. Restoring the block: 40/40 pass. The claim holds and the test is not vacuous. It is also a good test on its own terms: it restores a real released v0.1.6 fixture, writes a research event, reopens under require_current, and reads it back through both the upgraded root and a backup-restored copy.
The residual references are all deliberate
56 mentions survive outside docs. I read every source-side one, and each is load-bearing with a comment saying why: the reserved historical mode:deep_research label in session-start-mode.ts; the explicit rejection in create-session-input.ts; the ['deep-research.query', { kind: 'release' }] grant retirement, which follows the same shape turn.regenerate used at epoch 167; the retained schema table; the retained workflow_deep_research domain mapping; and the legacy-Session story. protocol-compatible-changes/session-catalog-deep-research-import.json stays too, correctly — it is a historical record stamped "epoch": 68, not live configuration.
P3 — epoch 170 has no changelog entry, and 170 is probably already taken
packages/runtime-host/src/protocol/index.ts bumps to 170 with no // 170: line. Removing an operation is the harder break to explain after the fact, and there is a precedent directly in the list for the exact shape needed:
// 167: Removed the turn.regenerate operation. Older peers can no longer
// safely interoperate because they may submit or advertise that operation.
There is a second, concrete reason to write it now. PR #5552 also bumps to 170, for an unrelated change, and as of f86a7a3e8 it has added its own entry: "170: External Session import input may carry an optional Host-resolved workspace target." If that lands first, this PR's number has to move, and the entry then has to say 171 and describe this break. Writing it while the reasoning is fresh is cheaper than reconstructing it during a rebase.
On the shared epoch number — not a defect, and the repo already handles it
I want to record this accurately because my first reading was wrong. Two open PRs advertising epoch 170 for incompatible reasons looks alarming, and I initially took it for a serious problem. It is not, because scripts/protocol-epoch-check.mjs exists for precisely this case (#3313) and runs on the merge result against the current base. I exercised its decision function both ways:
- base 169 → head 170:
Protocol changed and the epoch moved: 169 -> 170.Both PRs pass today. - base 170 → head 170: fails, with
…must land with an epoch the current base has not seen: rebase onto current main and set the epoch past 170.
So whichever of #5552 and #5554 merges second gets a clear, mechanical CI failure telling it to renumber. The hazard the guard was built to stop — two incompatible protocols merging without a git conflict because both wrote the same text to the same line — is genuinely stopped. This is a scheduling note for whoever merges second, not a finding against this PR.
未验证
No Electron run, no packaged E2E, no Windows or Linux, no real upgrade of an actual user's State Root beyond the committed v0.1.6 fixture. npm run check:asf-source failed for me at creates reproducible candidates from committed files only with spawnSync gpg ENOENT — my machine has no gpg; everything else in that check passed. Renderer architecture check passed, including --strict-base against 1c18700a7, which is the run that enforces base monotonicity rather than only comparing the ledger.
Approving per the request, on no P0–P2 and terminal green CI at this exact head. Two things that approval does not cover: whether Deep Research should be retired is a product decision and belongs to you, not to me; and this is still a draft.
简体中文
结论:无 P0–P2,1×P3。 这是一次谨慎的退休。七千行删除里通常出事的地方 —— 还有东西在伸手够已经删掉的东西 —— 这里没有出事;而且我是用构建验的,不是靠读的。
最强证据:从零干净构建。 npm run clean && npm run build 覆盖全部十个 workspace,0 错误。对删除类 PR,这比测试全绿更重要:陈旧的 dist 会让一个源文件已不存在的 import 照样解析成功,必须先确认被删模块真的不在产物里,编译通过才有意义。
全量测试:desktop/cli/core/ui/mcp/computer-use/website/eval 全过。runtime/storage/runtime-host 共 6 条失败,全部在 origin/main 上同样失败(逐条单独跑过控制组,没有假设):cell snapshot 的 direct-only / provider-native 两条、unknown tool 的部分执行语义、Object.prototype 继承工具不派发、storage 的跨进程第二 authority、以及 runtime-host 的 WorkHub v2 工具上限。其中四条正是工具注册表相关 —— 而这是一个删工具的 PR,所以我没有偷懒,逐条对 main 验了。结论:与本单无关,是我环境里的既有失败(CI 是绿的)。
迁移主张为真,且我用「破坏它」验证过。 描述说保留的历史表是既有 State Root 能升级的原因,并说「去掉保留的表定义,回归测试就会失败」。能被任何情况下通过的测试等于没测,所以我把 sqlite-workflow-schema.ts 里的 workflow_deep_research_events 块删掉重建:preserves retired research events through upgrade, reopen, and backup 确实失败,另有 4 条迁移测试一并失败;恢复后 40/40 全过。主张成立,测试不空。该测试本身也写得好:用真实的已发布 v0.1.6 fixture、写入研究事件、以 require_current 重开,并且分别从升级后的 root 和备份还原出的副本各读一次。
残留引用全部是刻意的。 文档之外还有 56 处,源码侧我逐个读过,每处都有注释说明为什么留:session-start-mode.ts 保留历史标签 mode:deep_research;create-session-input.ts 显式拒绝;['deep-research.query', { kind: 'release' }] 的授权退役 —— 与 turn.regenerate 在 epoch 167 的做法同形;保留的 schema 表;保留的 workflow_deep_research 域映射;以及遗留会话的 story。protocol-compatible-changes/session-catalog-deep-research-import.json 也该留 —— 它是标着 "epoch": 68 的历史记录,不是活配置。
P3 —— epoch 170 没有变更说明条目,而且 170 很可能已经被占。 移除一个操作是更难事后解释的那类破坏,而列表里就有现成的同形先例:// 167: Removed the turn.regenerate operation…。还有一个具体理由:#5552 也提到 170,且截至 f86a7a3e8 它已经补上了自己的条目(「170: 外部 Session 导入输入可携带可选的 Host 解析 workspace 目标」)。若它先落地,本单的号就得往后挪,条目也得改写成 171 并描述本单这次破坏 —— 趁推理还新鲜写下来,比 rebase 时重新回忆便宜。
关于共用 epoch 号 —— 不是缺陷,仓库已经处理了。 这点我要写准,因为我最初读错了:两个开着的 PR 同时宣称 epoch 170 且理由互不兼容,看起来很吓人,我一开始当成了严重问题。它不是,因为 scripts/protocol-epoch-check.mjs 正是为这个场景(#3313)而建,并且跑在合并结果上、对照当前 base。我把它的判定函数两个方向都跑了:base 169 → head 170 通过(所以两个 PR 今天都过);base 170 → head 170 失败,并明确提示「必须落在 base 没见过的 epoch 上:rebase 到当前 main 并把 epoch 设到 170 之后」。所以 #5552 与 #5554 中后合的那个会收到清晰的机械报错。该守卫要拦的东西 —— 两个分支把相同文本写到同一行,git 三方合并无冲突地放过两套不兼容协议 —— 确实被拦住了。这是给后合者的排期提示,不是对本单的发现。
未验证:未跑 Electron、未跑打包 E2E、无 Windows/Linux、除已提交的 v0.1.6 fixture 外没有真实用户 State Root 的升级。npm run check:asf-source 在 creates reproducible candidates from committed files only 处失败,原因是 spawnSync gpg ENOENT —— 我机器上没装 gpg,该检查其余部分全过。渲染层架构检查通过,含对 1c18700a7 的 --strict-base(这一档才强制基线单调性,而非只比对台账)。
按请求给出批准,依据是无 P0–P2 且该 head 上 CI 终态全绿。批准不覆盖的两件事:Deep Research 该不该退休是产品决定,属于你,不属于我;以及它目前仍是 draft。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
Summary
Retire the standalone Deep Research workflow before the first release. Remove its command, specialized tools and prompts, durable execution state machine, Host query/subscription, progress UI, and handoff action. This also removes the now-unused desktop mode-start path and the feature's tests and documentation.
Existing chats and reports remain readable. Legacy Sessions keep their persisted
explorepermission boundary and can be copied with their conversation and report artifacts. The historical workflow table remains part of the validated SQLite schema so existing State Roots can upgrade and retain their stored events; existing research runs cannot resume. Historical artifact metadata is accepted without loading the retired workflow implementation. Generic search, agent tools, and artifact handling remain available.Refs #3268. The remaining contributor review stays in that issue.
Compatibility
Runtime Host epoch advances from 169 to 170 because
deep-research.queryand its subscription domain are removed. Desktop and Host must be upgraded together. Existing access credentials retire the removeddeep-research.querygrant while retaining their identity and other permissions. Creatingdeep_researchis rejected; other unknown Desktop start modes retain their existing fallback behavior.Verification
npm run format,npm run lint, renderer architecture checks, protocol epoch guard, ASF headers, andnpm run check:asf-sourcepassed.e2a7cf233.AI use
Tool(s) and scope: Codex implemented the removal and compatibility tests, checked the remaining call paths, and ran the verification above.
Checklist
Does this PR entail a change in behavior?