Skip to content

feat(workflow): preserve source references across runs and resumes - #11931

Merged
qqqys merged 2 commits into
mainfrom
feat/workflow-source-references
Sep 15, 2026
Merged

qqqys merged 2 commits into
mainfrom
feat/workflow-source-references

Conversation

@qqqys

@qqqys qqqys commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

Adds an optional caller-supplied sourceRef: { id, revision } to the native Workflow tool. The reference is persisted before the first agent dispatch, retained in run results and live/historical task data, and restored from the journal when a run resumes after a restart. An explicit conflicting reference is rejected before execution.

Session capability discovery reports sourceRef: true, agentStepId: false, and workflowStepId: false. Existing Workflow availability and trust gates still apply. This is PR A of a two-part change; node and nested-call correlation is tracked in draft PR #11932.

Why it's needed

A caller that compiles or launches a versioned workflow definition needs to identify which definition produced a native run, including after a restart. A run ID or workflow display name alone cannot retain that attribution. Keeping the reference in the native execution and persistence path lets consumers reuse the existing workflow runtime and recovery behavior.

The reference is opaque caller metadata. It does not introduce a new workflow format, scheduler, source loader, or definition verification mechanism.

Reviewer Test Plan

How to verify

  1. Run an ordinary inline or saved workflow without a reference. It should retain its existing execution, results, and resume behavior.
  2. Start a workflow with an ID and revision, then inspect its tool result, task snapshot, and persisted history. They should report the same reference, and the first journal record should contain it before any business dispatch occurs.
  3. Restart the process and resume the run without passing a reference. It should restore the original reference and reuse eligible cached agent results. Supplying a different ID or revision, or attaching a new reference to a legacy journal, should fail before dispatch.
  4. Make the journal unavailable. An attributed run should fail before dispatch; ordinary callers should retain their existing best-effort persistence behavior.
  5. Query supported commands. Source references should be advertised while both step-ID features remain false. A disabled or untrusted Workflow session must remain unavailable.

Evidence (Before & After)

N/A for visual comparison: no layout or interaction changes. Before this change, a native run cannot retain caller definition attribution across recovery. After this change, the optional reference survives native journal recovery and history projections. Independent local validation and end-to-end evidence are recorded in the accompanying test report comment.

Tested on

OS Status
🍏 macOS ✅ tested
🪟 Windows ⚠️ not tested locally
🐧 Linux ⚠️ not tested locally

Environment

Node.js 22.17.0, isolated worktree dependencies and runtime directories, and a deterministic localhost OpenAI-compatible model fixture for actual CLI execution. This verifies protocol and runtime behavior, not real-model task quality or a deployed business environment. Based on current main including #11917 per-turn Workflow budgets and #11911 ACP child admission.

Risk & Scope

  • Main risk or tradeoff: opting into attribution requires a writable journal; startup fails if the initial source record cannot be persisted.
  • Not validated / out of scope: node IDs, nested-call traces, Data Agent UI/compiler changes, and real-model task quality.
  • Breaking changes / migration notes: all public fields are optional; no migration is required for existing workflows or historical records. An existing unbound journal cannot be relabeled during resume; start a new run to attach a reference.

Linked Issues

Replaces the source-attribution portion of the closed #11594 proposal. Reuses the native workflow support already on main, including #11805.

中文说明

改动内容

为原生 Workflow 工具增加可选的调用方来源 sourceRef: { id, revision }。在第一次 Agent 派发前持久化来源,在运行结果、实时和历史任务数据中保留来源,并在进程重启后恢复运行时从 journal 还原。调用方显式传入冲突来源时,在执行前拒绝。

会话能力探测返回 sourceRef: trueagentStepId: falseworkflowStepId: false,仍遵守现有 Workflow 可用性与信任门禁。这是两部分改动中的 PR A;节点及嵌套调用关联由 draft PR #11932 提供。

背景

调用方在编译或启动带版本的工作流定义后,需要知道具体哪份定义产生了原生运行,且进程重启后仍能追溯。仅靠运行 ID 或工作流显示名不能保留这层归属。把来源放在原生执行和持久化链路中,可让消费方复用已有运行时和恢复行为。

来源是调用方提供的不透明元数据,不引入新的工作流格式、调度器、来源加载器或定义校验机制。

Reviewer 验证计划

如何验证

  1. 不传来源运行普通 inline 或 saved workflow,确认执行、结果及恢复行为保持兼容。
  2. 使用 ID 和 revision 启动,检查工具结果、任务快照及持久化历史,确认来源一致,并且 journal 的第一条记录在业务派发前写入来源。
  3. 重启进程后不传来源恢复原运行,应继承原来源并复用可用的 Agent 缓存。显式传入不同 ID/revision,或为旧 journal 新增来源时,应在派发前失败。
  4. 让 journal 不可写。带来源的运行应在派发前失败,普通调用保留原有尽力持久化行为。
  5. 查询支持的命令,确认来源能力开启、两个节点 ID 能力均为 false。Workflow 被禁用或工作区不受信任时仍不可用。

改动前后证据

视觉对比不适用:没有布局或交互变化。改动前原生运行无法在恢复后保留调用方定义归属;改动后来源可通过原生 journal 恢复,并传递到历史视图的数据投影。独立本地验证与端到端证据见配套测试报告评论。

测试平台

macOS 已验证;Windows、Linux 未在本地验证。

环境

Node.js 22.17.0、独立工作树依赖与运行目录,以及确定性的本机 OpenAI 兼容模拟模型,实际执行 CLI。验证协议和运行时行为,不代表真实模型任务质量或业务部署环境。基线为包含 #11917 每轮 Workflow token budget 和 #11911 ACP 子进程准入的当前 main。

风险与范围

  • 主要取舍:启用来源关联需要可写 journal;初始来源记录无法持久化时启动失败。
  • 未验证或不包含:节点 ID、嵌套调用轨迹、Data Agent UI/编译器改造以及真实模型任务质量。
  • 兼容与迁移:公共字段均可选,现有工作流和历史记录无需迁移。不能在 resume 时给原本无来源的 journal 重新归属,需要新开运行。

关联

替代已关闭 #11594 方案中的来源归属部分,复用 main 已有的原生工作流能力,包括 #11805

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

⚠️ Deferred approval not posted — the PR head moved (or the PR closed) after the review of 9fbefab; approving now would attest to unreviewed code. Re-run @qwen-code /triage on the new head. finalize run

⚠️ 延迟审批未提交 —— 审查 9fbefab 之后 PR head 已变更(或 PR 已关闭),此时审批会为未审查的代码背书。请在新 head 上重新运行 @qwen-code /triage查看 finalize 运行

@qqqys

qqqys commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator Author

Local validation report

Validated independently on macOS with Node.js 22.17.0 and worktree-local dependencies. The complete Workflow validation ran on base e7d7ea84ba (including #11917). Before submission the branch was rebased onto 88347ccca3 (#11911); git range-diff confirms the feature patch is unchanged. Build, full typecheck, bundle and SDK compatibility checks were refreshed on that final base (533 targeted SDK tests passed).

  • Independent npm ci --prefer-offline, npm run build, npm run typecheck, and npm run bundle passed.
  • Package-local Vitest runs (--coverage.enabled=false --maxWorkers=2) passed 2,773 tests: Core Workflow/runtime/persistence/Tool/authoring 907; CLI including the full ACP Agent, task snapshots, history and saved-workflow loader 803; SDK daemon clients/UI/events 1,028; Web Shell Workflow consumers 35.
  • Changed-file ESLint, read-only Prettier checks and git diff --check passed for all 17 changed/new TypeScript files.
  • The actual built CLI ran against an isolated localhost OpenAI-compatible fixture with separate configuration, runtime and working directories. The existing globally installed CLI's ordinary native invocation was also checked as a baseline.

Actual CLI assertions passed:

  1. Ordinary native inline execution remains compatible and does not require source metadata.
  2. When the model receives the first Agent request, the journal already contains the source record as its first entry.
  3. Tool output and the terminal snapshot preserve the same reference. The A snapshot has no B-only step/call records.
  4. A fresh process resumes the same run with no supplied reference, inherits its original source and uses a new Tool invocation ID.
  5. The resumed dispatch is cached: the Agent model request count remains exactly one across both processes.
  6. An explicit conflicting revision fails before more Agent work and leaves the stored source unchanged.

Capability tests confirm A advertises source references while both step-ID features are false.

All model traffic was deterministic and local. This report is local runtime/protocol evidence, not hosted CI, review approval, interactive visual validation or real-model task-quality evidence.

中文验证说明

在 macOS / Node.js 22.17.0 上使用独立工作树依赖验证。完整 Workflow 验证基线为含 #11917e7d7ea84ba;提交前同步到 88347ccca3#11911),git range-diff 确认功能补丁完全等价,并在最终基线上刷新 build、全量 typecheck、bundle 和 SDK 兼容检查。

独立安装、构建、类型检查、bundle 均通过。定向 Vitest 共 2,773 个用例通过:Core 907、CLI 803(含完整 ACP Agent)、SDK 1,028、Web Shell 35。17 个变更文件的 ESLint、Prettier、diffcheck 通过。

实际 CLI 使用隔离本地模拟模型验证了:旧调用兼容;来源在首次 Agent 请求前已经作为 journal 首条落盘;工具结果与快照来源一致;A 没有 B 的节点/调用字段;跨进程 resume 不传来源仍继承原归属,并使用新的工具调用 ID;恢复命中缓存、两次执行合计仍只有一次 Agent 模型请求;冲突 revision 在新增派发前拒绝且不改原来源。能力探测确认来源开启、两个节点能力关闭。

以上是本地协议与运行时证据,不代表远端 CI、review、交互视觉回归或真实模型质量。

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

🖼️ web-shell visual preview

Rendered against a mock daemon (no real backend): the PR base vs this PR head 078d86c. Only screenshots that changed are shown (flows below, if any, are head-only) — refreshes on every push.

Screenshots · before / after

ℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:

  • packages/web-shell/client/components/messages/WorkflowExecutionView.tsx

Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to packages/web-shell/client/e2e/visuals/screenshots.spec.ts that seeds whatever state the UI is gated on; it then appears here as a head-only (NEW) capture.

Full-resolution recordings (.webm) are attached to the workflow run.

Qwen Code · web-shell visuals

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR — and for splitting #11594 rather than re-defending it. That one closed unmerged at 2,699 additions across 43 files; this is 446 additions across 17, with the step-ID and extension-context halves pulled out into PR B. That is the right cut, and it makes the provenance half reviewable on its own.

Template looks good ✓ — all nine headings, the OS table filled in honestly (macOS only), Evidence says N/A with a reason rather than being left blank, and Risk & Scope names the actual tradeoff instead of claiming there is none.

Problem. Real and code-verifiable, not theoretical hardening. At base, WorkflowParams carries script / scriptPath / args / resumeFromRunId and nothing at all about where the script came from. The run record carries toolUseId (which tool call launched it) and workflowName (a display name). Neither identifies a versioned definition, and neither survives a restart with that meaning intact — so a caller that compiles definitions externally has no way to answer "which revision produced this run?" from anything the native runtime persists.

Direction. Aligned. It threads the value through machinery that already exists — the resume journal, buildReplay, WorkflowRunHandle.locations, the optionalField helper in the ACP serializers — instead of adding a store or an endpoint. That is exactly the shape this gate asked for on #11594, where the dedicated execution endpoint and the separate run-state store were the expensive parts.

One thing I want on the record rather than buried: this does touch a public contract. ServeSessionSupportedCommandsStatus and ServeSessionWorkflowTaskStatus (acp-bridge), plus DaemonSessionSupportedCommandsStatus and DaemonSessionWorkflowTaskStatus (published SDK), all gain fields, and the journal gains a new versioned record type. I am not escalating it, because every addition is optional and additive and follows a pattern these exact interfaces already use — workflowsEnabled?, savedWorkflows?, and WorkflowSnapshot.toolUseId? ("Absent on legacy snapshots") were all added the same way, and the SDK file's own contract rule says only a v bump signals a breaking wire change. I traced every consumer: acpAgent.ts sets them, the SDK mirrors them by hand (documented convention, not duplication debt), tasksSnapshot.ts and workflowsCommand.ts project them, and web-shell reads them through @qwen-code/sdk/daemon with no local mirror to keep in sync. If a maintainer reads the contract surface differently, say so and this becomes a defer.

Size. Core paths are touched (packages/core/src/**, including tools/workflow/) and the change spans five packages. Breakdown: 201 production lines, 252 test lines, 0 generated/schema. Under the 500-line maintainer-awareness threshold and well under the 1000-line advisory, and the title is feat, so no size gate applies either way. The author has write access, so the two-tier core gate is exempt regardless — I'm reporting the numbers because they're the interesting part: there is more test code here than production code.

Approach. The scope feels right and I don't see a materially simpler version that still meets the stated requirement — dropping journal persistence would drop restart survival, which is the whole point. The one new module, workflow-correlation.ts (47 lines: a type, a predicate, a parse-and-freeze), earns its existence, because the journal, the runner, the tool, and the snapshot validator all need the same predicate and putting it in any one of them would create an import cycle or a copy. Three smaller things I'd genuinely ask about, none of them blocking:

  • Two of the three capability flags advertise features that do not exist yet — agentStepId: false and workflowStepId: false are PR B. I see the argument (a consumer can now distinguish "old daemon, field absent" from "new daemon, feature not implemented"), but it is public API surface for unimplemented behaviour. Would shipping sourceRef alone now, and adding the other two when they are real, cost the consumer anything?
  • toolUseId rides along in two places — workflowsCommand.ts snapshotToTask and the web-shell history export. It already existed on WorkflowSnapshot and on the daemon task type and was being dropped by those two projections, so both are real gap fixes; they are just not this PR's gap. One line each, so I would not die on this hill, but it is cheaper as its own commit.
  • No design doc. This introduces a versioned on-disk record format ({type:'source', version:1}) and two new public wire fields, across 17 files and 5 packages — which is where AGENTS.md asks for one in docs/design/, English plus zh-CN. The nearest existing doc, 2026-09-14-extension-workflow-distribution.md, covers distribution, not provenance.

Risk. Stage 1e matched: four files under packages/cli/src/acp-integration/ (acpAgent.ts, acpAgent.test.ts, session/tasksSnapshot.ts, session/tasksSnapshot.test.ts) hit the acp-integration high-risk pattern — the strongest revert-correlated signal in this repo. So I read the whole production diff at full depth rather than skimming it, and Stage 2 names the sandboxed lanes that would settle the behavioural claims.

Evidence gap worth closing. The body says independent local validation and E2E evidence are "recorded in the accompanying test report comment" — but this thread currently holds exactly one comment, the triage lifecycle marker. There is no test report here. On #11594 the equivalent report was posted. Since "Tested on" is macOS-only, that report is the only thing standing in for Windows and Linux coverage, so its absence is felt.

Moving on to code review. 🔍

中文说明

感谢贡献 —— 也感谢你把 #11594 拆开,而不是继续为它辩护。那个 PR 以 43 个文件、2699 行新增的状态关闭且未合并;这个是 17 个文件、446 行新增,节点 ID 和扩展上下文两半已经拆到 PR B。这个切法是对的,也让"来源归属"这一半可以独立评审。

模板完整 ✓ —— 九个标题齐全,测试平台表格如实填写(仅 macOS),Evidence 写了 N/A 并给出理由而不是留空,Risk & Scope 也如实说明了取舍而非声称没有风险。

问题。 真实且可在代码中验证,不是理论性加固。在 base 上,WorkflowParams 只有 script / scriptPath / args / resumeFromRunId,完全没有"脚本从哪来"的信息。运行记录里有 toolUseId(哪次工具调用启动了它)和 workflowName(展示名),两者都不能标识一份带版本的定义,重启后也保不住这层含义。所以在外部编译定义的调用方,无法从原生运行时持久化的任何内容中回答"这次运行是哪一版产生的?"

方向。 对齐。它把值串进已有的机制 —— resume journal、buildReplayWorkflowRunHandle.locations、ACP 序列化里的 optionalField —— 而不是新增存储或接口。这正是本 gate 在 #11594 上要求的形态:那里代价最高的部分就是专用执行接口和独立运行状态库。

有一点我要明确写出来而不是埋掉:这确实动了公共契约。acp-bridge 的 ServeSessionSupportedCommandsStatusServeSessionWorkflowTaskStatus,以及已发布 SDK 的 DaemonSessionSupportedCommandsStatusDaemonSessionWorkflowTaskStatus 都新增了字段,journal 也新增了一种带版本的记录类型。我升级处理,因为所有新增都是可选且向后兼容的,并且沿用了这些接口已有的做法 —— workflowsEnabled?savedWorkflows?WorkflowSnapshot.toolUseId?(注释写着 "Absent on legacy snapshots")都是同样方式加进来的,而 SDK 文件自己的契约规则也说明只有 v 号变化才代表破坏性 wire 改动。我把每个消费方都追了一遍:acpAgent.ts 负责写入,SDK 手工镜像(这是文件里写明的约定,不是重复代码债),tasksSnapshot.tsworkflowsCommand.ts 做投影,web-shell 通过 @qwen-code/sdk/daemon 读取、没有需要同步的本地镜像。如果 maintainer 对契约面的判断不同,请说明,我会改为 defer。

规模。 触及核心路径(packages/core/src/**,含 tools/workflow/),跨五个包。明细:生产代码 201 行测试 252 行生成/schema 0 行。低于 500 行的维护者关注阈值,也远低于 1000 行的大 PR 建议阈值;标题是 feat,因此任何规模门禁都不适用。作者具有 write 权限,两层核心门禁本身也豁免 —— 我仍然报数字,因为数字本身就是重点:测试代码比生产代码还多。

方案。 范围合理,我想不到在满足既定需求前提下明显更简的版本 —— 去掉 journal 持久化就等于去掉重启后可追溯,而那正是核心诉求。唯一的新模块 workflow-correlation.ts(47 行:一个类型、一个谓词、一个 parse-and-freeze)有存在价值:journal、runner、tool 和 snapshot 校验器都需要同一个谓词,放进其中任何一个都会造成循环依赖或复制。三个我更想问的小点,都不构成阻塞:

  • 三个能力开关里有两个在广告尚不存在的特性 —— agentStepId: falseworkflowStepId: false 属于 PR B。我理解其理由(消费方现在可以区分"旧 daemon,字段缺失"和"新 daemon,特性未实现"),但这毕竟是为未实现行为提前开放公共 API 面。现在只发 sourceRef,等另外两个真正实现时再加,对消费方有损失吗?
  • toolUseId 顺带出现在两处 —— workflowsCommand.tssnapshotToTask 和 web-shell 的历史导出。它在 WorkflowSnapshot 和 daemon task 类型上本来就有,只是被这两个投影丢掉了,所以两处都是真实的缺口修复;只是不属于本 PR 的缺口。各一行,我不会为此僵持,但单独一个 commit 成本更低。
  • 没有设计文档。这次引入了带版本的磁盘记录格式({type:'source', version:1})和两个新的公共 wire 字段,涉及 5 个包 17 个文件 —— 按 AGENTS.md,这正是应当在 docs/design/ 下补一份(英文 + zh-CN)的情形。现有最接近的 2026-09-14-extension-workflow-distribution.md 讲的是分发,不是来源归属。

风险。 Stage 1e 命中:packages/cli/src/acp-integration/ 下四个文件(acpAgent.tsacpAgent.test.tssession/tasksSnapshot.tssession/tasksSnapshot.test.ts)匹配 acp-integration 高风险模式 —— 这是本仓库与 revert 相关性最强的信号。因此我按完整深度读了全部生产代码 diff,而不是略读;Stage 2 也点名了能真正确认行为性结论的沙箱验证通道。

需要补上的证据缺口。 PR 正文写着独立本地验证与端到端证据"见配套测试报告评论"—— 但当前这个线程里只有一条评论,即 triage 生命周期标记,并没有测试报告。#11594 上是有对应报告的。由于"测试平台"只有 macOS,那份报告是 Windows 与 Linux 覆盖的唯一替代,所以它缺失是有影响的。

进入代码审查 🔍

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 9fbefabfb08c78a74e8576ead0e0f6acc62c1026 · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 15, 2026

Copy link
Copy Markdown
Collaborator

Code review

I read the whole production diff (201 lines, 13 files) at full depth, since Stage 1e flagged acp-integration. No merge blockers. The design is careful, the fail-closed posture is deliberate rather than accidental, and the 252 test lines pin behaviour instead of restating the implementation. What follows is one robustness gap in the new code, three smaller notes, and then what I checked and found clean — the clean list is long because the interesting question on a change like this is whether the ordering and aliasing claims actually hold.

First, my independent proposal, written before reading the diff: accept an optional validated {id, revision} on the tool param; persist it as the journal's first record so a crash after dispatch cannot lose attribution; carry it on the run handle and the registry task; restore it from the journal on resume and refuse to relabel a legacy journal; project it through the ACP/SDK/web-shell types; advertise it in capability discovery; and fail the start when an attributed run has no writable journal. The PR does all seven. It beat my proposal in two places I had not thought of: readWorkflowSourceRef returns a fresh Object.freezed copy, so a caller mutating its own object after start cannot corrupt attribution; and the runner cross-checks registry state against journal state (previousEntry?.sourceRef && !original), which catches a journal that lost its source record while the run record still claims one.

1. Non-blocking — one fail-open path in an otherwise fail-closed design

After a process restart the run registry is empty (WorkflowRunRegistry is not rehydrated from snapshots — I grepped for a load path and there isn't one; the new test simulates restart with registry.reset()), so previousEntry is undefined. If the journal is also gone or unreadable while the snapshot survives, WorkflowJournal.load() swallows the error and returns an empty replay, buildReplay([]) yields neither sourceRef nor sourceError, the registry cross-check has nothing to fire on, and the resumed run silently carries no attribution — then writes a snapshot without it.

Everywhere else this PR fails closed and says so: unwritable journal → throw before dispatch; conflicting id or revision → throw; corrupt, contradictory, or unsupported-version source record → sourceError → throw. This is the one path where attribution disappears without an error, so an external consumer cannot distinguish "this run was never attributed" from "attribution was lost". It is narrow — a resume with no journal already means full re-execution, since the cached results live in the same file — and it is not a regression, because base has no attribution at all. But it is a gap against the PR's own claim that the reference is "restored from the journal when a run resumes after a restart". Falling back to the snapshot's sourceRef, or throwing the same way the conflicting-reference path does, would close it.

2. Non-blocking — version !== 1 bricks resume, silently and permanently

buildReplay folds any record whose version is not exactly 1 into sourceError, and the runner throws on sourceError unconditionally. So a future release that writes version: 2 makes every run it journaled permanently unresumable on an older binary — stranding the cached agent results along with it — and the message ("Workflow journal contains invalid source metadata.") names neither the run nor the journal path. Failing closed on an unknown format is a defensible choice, and arguably the right one, but the alternative (ignore an unknown version, keep replaying results) is defensible too, and the tradeoff isn't recorded anywhere. Worth a code comment stating the intent, and worth putting the run id in the message.

3. Non-blocking — the new journal entry type breaks the file's own convention

Its three siblings are named exported interfaces (JournalStartedEntry, JournalResultEntry, JournalFailedEntry); the new one is an inline anonymous member, | { type: 'source'; version: 1; sourceRef: WorkflowSourceRef }. Naming it JournalSourceEntry matches the surrounding code and gives this on-disk record format something to point at from a migration note.

4. Non-blocking — the promised test report comment is missing

The body says independent local validation and E2E evidence are "recorded in the accompanying test report comment"; the thread holds only the lifecycle marker. I am not treating the author's claimed results as evidence and have not counted them anywhere below — but with "Tested on" at macOS-only, that report is the only substitute for Windows and Linux coverage, and Stage 1e put this PR in the elevated-review-depth bucket.

What I checked and found clean

  • No parser gap on the new record type. WorkflowJournal.load()read<JournalEntry>() in jsonl-utils does no runtime type filtering — it parses every non-empty line and pushes it — so source records reach buildReplay. This was the failure mode that would have made the whole feature silently no-op across restarts.
  • The workflow-orchestrator.test.ts change is a type fix, not a weakened assertion. A source entry has no key property, so entry.key === keyB was already false for it at runtime; the added entry.type !== 'source' guard only makes the property access legal under the widened union. For every non-source entry the behaviour is byte-identical to before.
  • The legacy replay shape is pinned. buildReplay([]) is asserted with toEqual({results, started, failed}) — the conditional spreads keep sourceRef/sourceError out entirely rather than adding undefined keys, so existing exact-shape assertions elsewhere don't rot.
  • Moving journal.ensureExists() ahead of inline-script persistence is safe. The catch block already calls journal.remove() for a fresh run, so a source record appended before a later failure is cleaned up and no orphan journal is left behind. On a resume nothing is appended (if (!options.resumeFromRunId)), so the original journal is never mutated.
  • No TDZ on previousEntry. It is declared at line 203 (const previousEntry = registry?.get(runId)), outside and before the try block that now reads it.
  • Write-before-dispatch is genuinely proven, not just claimed. The correlation test's dispatch mock reads the journal's first line from inside the dispatch and asserts it equals the source record. A regression that moved the append after dispatch would fail that test, not merely the PR description.
  • Aliasing is handled at every hop. readWorkflowSourceRef returns a frozen fresh copy, and toSnapshot, serializeWorkflowTask, serializeWorkflowSnapshot, and snapshotToTask all spread it rather than pass it through. tasksSnapshot.test.ts pins not.toBe(source.sourceRef); the correlation test mutates the caller's literal after start and asserts the handle still holds revision: 'r1'. That is the caller-mutation defence demonstrated, not asserted.
  • Double validation is correct, not redundant. validateToolParamValues guards the model-facing tool boundary; WorkflowRunner.start calls readWorkflowSourceRef again because the runner is reachable from paths that never go through tool-param validation. Both are system boundaries.
  • The SDK duplication is the documented convention. packages/sdk-typescript/src/daemon/types.ts states in its header that it mirrors packages/cli/src/serve shapes "defined SDK-side to avoid an SDK→CLI dependency". Hand-maintaining both copies is the existing rule, so this is not duplication debt.
  • The web-shell change typechecks against the extended type. downloadWorkflowHistory takes runs: readonly DaemonSessionWorkflowTaskStatus[] imported from @qwen-code/sdk/daemon — the type this PR extended — and toolUseId already existed on it. mapSupportedCommands returns a narrowed shape and does no exhaustive key mapping, so the new capability field cannot break it.
  • The barrel export is required, not speculative. WorkflowSourceRef now appears in the already-exported WorkflowTask and WorkflowSnapshot, so consumers must be able to name it; it sits beside the existing workflow-run-registry and workflow-snapshot exports.
  • Snapshot validation is fail-closed on disk. isWorkflowSnapshot now rejects a snapshot whose sourceRef is present but malformed, rather than passing it through to consumers.
  • No new daemon routes, so there is no route-ownership scope to classify and no repeat of the route-drift guard that feat(workflow): support prepared flows through native tools #11594 tripped.

Flow

sequenceDiagram
    participant P1 as Caller (Workflow tool)
    participant P2 as WorkflowRunner.start
    participant P3 as WorkflowJournal
    participant P4 as Orchestrator dispatch
    participant P5 as RunRegistry, Snapshot, ACP status
    P1->>P2: script, args, optional sourceRef (id, revision)
    P2->>P2: validate and freeze the reference
    P2->>P3: ensureExists, then append source record version 1
    Note over P2,P3: throws before dispatch when the journal is not writable
    P2->>P5: register the run task carrying sourceRef
    P2->>P4: start, first agent dispatch
    P4-->>P1: settlement, handle.sourceRef echoed into the tool result
    P1->>P2: later, resumeFromRunId after a process restart
    P2->>P3: load and buildReplay
    P3-->>P2: sourceRef restored, or sourceError
    Note over P2: a conflicting reference, or a new one on a legacy journal, throws
Loading
Files changed (17 of 17 shown)
File What changed
packages/core/src/agents/workflow-correlation.ts New 47-line module: the source-reference type, a strict shape predicate (exactly two own keys, non-empty, max 256, no surrounding whitespace, no control characters), and a parse-and-freeze that throws a message naming every constraint.
packages/core/src/agents/runtime/workflow-runner.ts The substance of the PR. Resolves and freezes the reference, enforces resume matching against the journal, cross-checks registry state against journal state, moves journal creation earlier so the source record lands before dispatch, and fails closed when an attributed run has no writable journal.
packages/core/src/agents/runtime/workflow-journal.ts Adds the source record to the entry union; replay now collects it, folds a contradictory or unsupported-version record into an error field, and keeps the legacy three-key shape when there is none.
packages/core/src/tools/workflow/workflow.ts The model-facing param and its JSON schema, boundary validation, pass-through to the runner, and echoing the reference into all four result shapes (background start, success, failure, display payload).
packages/core/src/agents/workflow-snapshot.ts Adds the field to the snapshot type, copies it when projecting a task, and rejects a malformed on-disk value in the snapshot type guard.
packages/core/src/agents/workflow-run-registry.ts Adds the optional field to the live workflow task type.
packages/core/src/index.ts One barrel export line, required because the new type appears in two already-exported types.
packages/cli/src/acp-integration/acpAgent.ts Hardcodes the three capability booleans in the supported-commands builder, and forwards a retried task's reference back into the workflow tool call.
packages/cli/src/acp-integration/session/tasksSnapshot.ts Projects the reference through both the live and historical workflow serializers, copying rather than aliasing, via the existing optional-field helper.
packages/cli/src/ui/commands/workflowsCommand.ts Snapshot-to-task conversion now carries the tool-use id and the reference instead of dropping them. The tool-use id half is the drive-by noted in Stage 1.
packages/acp-bridge/src/status.ts Adds the optional capability object to the serve status envelope and the reference to the workflow task status wire type.
packages/sdk-typescript/src/daemon/types.ts Hand-maintained mirror of the two acp-bridge additions, per that file's documented no-SDK-to-CLI-dependency convention.
packages/web-shell/client/components/messages/WorkflowExecutionView.tsx The history-download JSON now includes the tool-use id and the reference for each exported run.
packages/core/src/agents/workflow-correlation.test.ts New 222-line suite: write-before-dispatch ordering asserted from inside the dispatch, restart recovery with the snapshot deleted, durability required only of callers who opt in, no dispatch when the metadata write fails, no relabeling a legacy journal, and contradictory journal records.
packages/cli/src/acp-integration/session/tasksSnapshot.test.ts New test that historical snapshots retain attribution and that the projected reference is a copy, not the same object.
packages/cli/src/acp-integration/acpAgent.test.ts Extends the single assertion that pins the supported-commands envelope with the new capability object.
packages/core/src/agents/runtime/workflow-orchestrator.test.ts Type-narrowing fix on an existing assertion so the widened entry union still compiles; runtime semantics unchanged (see the clean list above).

Test evidence

This is an unattended CI run, so per the triage rules I did not build, run, or execute anything from this PR — no unit tests, no bundle, no dev CLI. Everything below is the PR's own CI, read through the API for the reviewed commit. I am carrying no independent behavioural evidence, and I have not counted the author's self-reported local results as evidence anywhere in this review.

Status at review time: 81 check-runs on this commit — 11 success, 10 in progress, 1 queued, 1 waiting, 58 skipped, and zero failures. Nothing is red. But the two checks that would actually catch a type error or a broken assertion in this diff — Test (ubuntu-latest, Node 22.x) and Lint & Static (ubuntu-latest, Node 22.x) — are both still running, so I cannot claim green and I have not polled for it. That matters more than usual here: #11594 was blocked precisely by a red Test (ubuntu-latest, Node 22.x), one failure in 2,447, in scripts/tests/cross-package-contracts.test.js. This PR touches the same class of cross-package mirrored surface, so that check is load-bearing for it. I read cross-package-contracts.test.js to see whether it pins anything this diff moves: its two cases are locale-resolution-vs-prompt-sanitization and the external subagent runtime subpath, neither of which this PR touches. That is a static read, not a passing run.

Not verified: Windows and Linux behaviour (the author tested macOS only, and the per-OS Test matrix entries for macOS and Windows are skipped on this commit); the restart-and-resume path against a real process exit rather than a registry.reset() in a unit test; and whether the missing test report comment would have shown anything the unit suite does not.

Final CI results for 9fbefab (auto-updated by the triage finalize job after CI completed):

Check Conclusion
Lint & Static (ubuntu-latest, Node 22.x) ❌ failure
Test (ubuntu-latest, Node 22.x) ❌ failure
web-shell E2E Smoke (ubuntu-latest, Node 22.x) 🚫 cancelled
Capture web-shell visuals (ubuntu-latest, Node 22.x) ✅ success
Classify PR ✅ success
Desktop Shell (ubuntu-22.04) ✅ success
Desktop Shell (windows-2022) ✅ success
Integration Tests (no-AK, No Sandbox) ✅ success
Live Host (macos-latest) ✅ success
macos-latest / Java 21 ✅ success
OpenTUI no-flicker gate ✅ success
Real daemon E2E / Java 11 ✅ success
TUI parity snapshots (ink vs opentui) ✅ success
ubuntu-latest / Java 11 ✅ success
ubuntu-latest / Java 17 ✅ success
ubuntu-latest / Java 21 ✅ success
windows-latest / Java 21 ✅ success

One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。

Sandboxed verification would settle what the diff and the pending suite cannot. The author has write access, so both lanes are available without sponsorship:

  • @qwen-code /verify — that the source record is durable before the first business dispatch and is restored from the journal across a real restart, and that a conflicting revision fails closed before any dispatch happens. The unit tests pin both against a mocked dispatch and a tmpdir journal, which is good but is the same process; an A/B against the base build is what would show base has no such guarantee. This is also the lane that would settle finding 1, since it can exercise a resume where the journal is missing but the snapshot is not.
  • @qwen-code /tmux — the /workflows surface. snapshotToTask now carries the reference and the tool-use id into the task list, and the web-shell history export gained both; whether a resumed run visibly retains attribution in the TUI is not observable from the diff at all.
中文说明

代码审查

Stage 1e 命中了 acp-integration,所以我按完整深度读了全部生产代码 diff(13 个文件、201 行)。没有合并阻塞项。 设计很谨慎,fail-closed 是刻意为之而非偶然,252 行测试钉住的是行为而不是对实现的复述。下面是一个健壮性缺口、三个较小的备注,然后是"我检查过且确认没问题"的清单 —— 这份清单很长,因为这类改动真正要回答的问题是:顺序性和引用别名方面的声明是否真的成立。

先说我在读 diff 之前写下的独立方案:在工具参数上接受一个可选且经过校验的 {id, revision};把它作为 journal 的第一条记录持久化,这样派发之后的崩溃也不会丢掉归属;把它带在 run handle 和 registry task 上;resume 时从 journal 还原,并拒绝给旧 journal 重新贴标签;把它投影到 ACP/SDK/web-shell 类型;在能力探测中声明;当归因运行没有可写 journal 时让启动失败。这个 PR 七条都做到了,并且在两处超出我的方案:readWorkflowSourceRef 返回一个新的 Object.freeze 副本,因此调用方在 start 之后修改自己的对象无法污染归因;runner 还会把 registry 状态与 journal 状态交叉校验(previousEntry?.sourceRef && !original),这能抓住"journal 丢了来源记录但运行记录仍声称有"的情况。

1. 非阻塞 —— 整体 fail-closed 的设计里有一条 fail-open 路径

进程重启后运行注册表是空的(WorkflowRunRegistry 不会从快照重建 —— 我搜过重建路径,没有;新测试是用 registry.reset() 模拟重启的),所以 previousEntryundefined。如果此时 journal 也丢失或不可读、而快照还在,WorkflowJournal.load() 会吞掉错误返回空 replay,buildReplay([]) 既不产出 sourceRef 也不产出 sourceError,注册表交叉校验无从触发,于是恢复出来的运行静默地不带归因 —— 然后写出一份同样不带归因的快照。

这个 PR 在其他所有地方都是 fail-closed 并且明确表达:journal 不可写 → 派发前抛错;id 或 revision 冲突 → 抛错;来源记录损坏、自相矛盾或版本不支持 → sourceError → 抛错。只有这一条路径上归因会无声消失,因此外部消费方无法区分"这次运行从未归因"和"归因丢了"。范围很窄 —— journal 缺失时的 resume 本来就等于完全重新执行,因为缓存结果就在同一个文件里 —— 而且这不是回归,因为 base 上根本没有归因。但它与 PR 自己的表述("进程重启后恢复运行时从 journal 还原")之间存在缺口。回退到快照里的 sourceRef,或者像冲突路径那样抛错,都能补上。

2. 非阻塞 —— version !== 1 会永久且无声地废掉 resume

buildReplay 把任何 version 不严格等于 1 的记录都归入 sourceError,而 runner 对 sourceError 无条件抛错。因此将来某个写 version: 2 的版本,会让它记录过的所有运行在旧二进制上永久无法 resume —— 连带缓存的 agent 结果一起报废 —— 而错误信息("Workflow journal contains invalid source metadata.")既不含 run id 也不含 journal 路径。对未知格式 fail-closed 是可以辩护的选择,甚至可能是对的,但另一种做法(忽略未知版本、继续 replay 结果)同样可以辩护,而这个取舍没有被记录在任何地方。建议加一行代码注释说明意图,并把 run id 放进错误信息。

3. 非阻塞 —— 新的 journal 记录类型破坏了该文件自身的约定

它的三个兄弟都是具名导出接口(JournalStartedEntryJournalResultEntryJournalFailedEntry),新增的这个却是内联匿名成员 | { type: 'source'; version: 1; sourceRef: WorkflowSourceRef }。命名为 JournalSourceEntry 既符合周围代码风格,也让这个磁盘记录格式在迁移说明里有个可指代的名字。

4. 非阻塞 —— 正文承诺的测试报告评论不存在

正文说独立本地验证与端到端证据"见配套测试报告评论",但线程里只有生命周期标记。我没有把作者自述的结果当作证据,本审查任何地方也没有采信它 —— 但既然"测试平台"只有 macOS,那份报告是 Windows 与 Linux 覆盖的唯一替代,而 Stage 1e 又把这个 PR 放进了更深审查的档位。

我检查过且确认没问题的部分

  • 新记录类型不存在解析器缺口。 WorkflowJournal.load()jsonl-utilsread<JournalEntry>() 不做运行时类型过滤,它解析每个非空行并全部推入结果,所以 source 记录能到达 buildReplay。这正是会让整个特性在重启后静默失效的失败模式。
  • workflow-orchestrator.test.ts 的改动是类型修复,不是弱化断言。 source 记录没有 key 属性,因此 entry.key === keyB 在运行时对本来就为 false;新增的 entry.type !== 'source' 只是让这个属性访问在放宽后的联合类型下合法。对所有非 source 记录,行为与之前完全一致。
  • 旧 replay 形状被钉住了。 buildReplay([])toEqual({results, started, failed}) 断言 —— 条件展开完全不放入 sourceRef/sourceError,而不是放入 undefined 键,因此别处已有的精确形状断言不会腐坏。
  • journal.ensureExists() 提前到内联脚本持久化之前是安全的。 catch 块本来就会对新运行调用 journal.remove(),所以在后续失败前追加的来源记录会被清理,不会留下孤儿 journal。resume 时不追加任何记录(if (!options.resumeFromRunId)),因此原 journal 永不被改写。
  • previousEntry 没有 TDZ 问题。 它在第 203 行声明(const previousEntry = registry?.get(runId)),位于现在读取它的 try 块之外且之前。
  • "派发前写入"是被真正证明的,不是被声称的。 关联测试的 dispatch mock 在派发内部读取 journal 第一行并断言它等于来源记录。任何把 append 挪到派发之后的回归都会让这个测试失败,而不只是让 PR 描述失真。
  • 每一跳都处理了引用别名。 readWorkflowSourceRef 返回冻结的新副本,toSnapshotserializeWorkflowTaskserializeWorkflowSnapshotsnapshotToTask 全部展开复制而非透传。tasksSnapshot.test.ts 钉住 not.toBe(source.sourceRef);关联测试在 start 之后修改调用方的字面量,并断言 handle 仍持有 revision: 'r1'。这是把防调用方修改的能力演示出来,而不是断言出来。
  • 双重校验是正确的,不是冗余。 validateToolParamValues 守的是面向模型的工具边界;WorkflowRunner.start 再调一次 readWorkflowSourceRef,是因为 runner 也可以从不经过工具参数校验的路径到达。两者都是系统边界。
  • SDK 的重复是文件写明的约定。 packages/sdk-typescript/src/daemon/types.ts 头部说明它镜像 packages/cli/src/serve 的形状、"定义在 SDK 侧以避免 SDK→CLI 依赖"。手工维护两份是既有规则,因此这不是重复代码债。
  • web-shell 改动对扩展后的类型可以通过类型检查。 downloadWorkflowHistory 的参数是 runs: readonly DaemonSessionWorkflowTaskStatus[],从 @qwen-code/sdk/daemon 导入 —— 正是本 PR 扩展的类型 —— 且 toolUseId 本来就在其上。mapSupportedCommands 返回收窄后的形状、不做穷举键映射,所以新增能力字段不会破坏它。
  • barrel 导出是必需的,不是投机。 WorkflowSourceRef 现在出现在已导出的 WorkflowTaskWorkflowSnapshot 中,消费方必须能指名它;它紧邻既有的 workflow-run-registryworkflow-snapshot 导出。
  • 快照校验对磁盘数据是 fail-closed 的。 isWorkflowSnapshot 现在会拒绝 sourceRef 存在但形状非法的快照,而不是把它透传给消费方。
  • 没有新增 daemon 路由,因此不存在需要归类的路由所有权范围,也不会重演 feat(workflow): support prepared flows through native tools #11594 触发的路由漂移守卫。

(流程图见上;中文摘要不重复图形内容:调用方把可选来源传给 runner,runner 校验并冻结,先确保 journal 存在再追加版本 1 的来源记录 —— journal 不可写时在派发前抛错 —— 然后注册携带来源的运行任务并启动首次派发,结果把来源回显给调用方;之后带 resumeFromRunId 恢复时从 journal 重放还原来源,冲突或在旧 journal 上新增来源都会抛错。)

测试证据

这是无人值守的 CI 运行,因此按 triage 规则我没有构建、运行或执行本 PR 的任何代码 —— 没跑单测、没打 bundle、没起 dev CLI。下面全部内容都是通过 API 读到的、该 PR 自身在所审查 commit 上的 CI 结果。我不携带任何独立的行为性证据,也没有在本审查的任何地方把作者自述的本地结果算作证据。

审查时状态:该 commit 上 81 个 check-run —— 11 成功、10 进行中、1 排队、1 等待、58 跳过,零失败。 没有红的。但真正能抓到本 diff 中类型错误或断言破损的两个检查 —— Test (ubuntu-latest, Node 22.x)Lint & Static (ubuntu-latest, Node 22.x) —— 都还在跑,所以我不能声称通过,也没有轮询等待。这一点在此处比平时更重要:#11594 恰恰是被红掉的 Test (ubuntu-latest, Node 22.x) 拦下的,2447 个测试中失败 1 个,位于 scripts/tests/cross-package-contracts.test.js。本 PR 动的是同一类跨包镜像面,所以那个检查对它是承重的。我读了 cross-package-contracts.test.js,确认它钉住的内容是否被本 diff 触及:它的两个用例分别是 locale 解析与 prompt 清洗的区分、以及外部 subagent 运行时的 subpath 导出,本 PR 都没有碰。这是静态阅读结论,不是一次通过的运行。

未验证:Windows 与 Linux 行为(作者仅在 macOS 测试,而该 commit 上 macOS 与 Windows 的分平台 Test 矩阵项被跳过);针对真实进程退出的重启恢复路径(而非单测里的 registry.reset());以及那份缺失的测试报告评论是否会展示单测覆盖不到的东西。

沙箱验证能确认 diff 与当前待完成测试套件都无法确认的部分。作者具有 write 权限,因此两条通道都无需赞助即可使用:

  • @qwen-code /verify —— 确认来源记录在首次业务派发之前已落盘、并能在真实重启后从 journal 还原;以及冲突的 revision 会在任何派发发生之前失败关闭。单测用 mock 派发和 tmpdir journal 钉住了这两点,这很好,但仍在同一进程内;与 base 构建做 A/B 才能显示 base 没有这类保证。这也是能确认第 1 条发现的通道,因为它可以演练"journal 缺失但快照存在"时的 resume。
  • @qwen-code /tmux —— /workflows 界面。snapshotToTask 现在把来源和 tool-use id 带进任务列表,web-shell 历史导出也新增了这两项;恢复的运行在 TUI 中是否可见地保留了归因,从 diff 完全看不出来。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 9fbefabfb08c78a74e8576ead0e0f6acc62c1026 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — solid, additive, and better tested than it needed to be; the four things I'd want are all non-blocking, and the only real reservation is that CI has not landed yet.

Where I landed

Going back to the proposal I wrote before reading the diff: I had sketched seven moves — a validated optional param, the reference persisted as the journal's first record, carried on the handle and the registry task, restored on resume with legacy journals un-relabelable, projected through the ACP/SDK/web-shell types, advertised in capability discovery, and a hard failure when an attributed run has no writable journal. The PR did all seven. It beat my proposal twice: freezing a fresh copy so caller mutation after start cannot corrupt attribution, and cross-checking registry state against journal state so a journal that lost its source record while the run record still claims one is caught rather than silently projected. Neither occurred to me, and both are the kind of thing that only shows up when someone has actually thought about how the value can go wrong rather than just how it flows.

The test suite is the part I'd point a future contributor at. 252 test lines against 201 production lines, and they pin the claims that matter rather than the ones that are easy: the write-before-dispatch ordering is asserted from inside the dispatch mock, reading the journal's first line off disk, so moving the append later fails the test instead of just the PR description. The aliasing defence is demonstrated by mutating the caller's object after start and asserting the handle still holds the original revision. The legacy replay shape is pinned with an exact toEqual, which is what stops the conditional spreads from quietly adding undefined keys and rotting assertions elsewhere. And the one change to an existing test — the entry.type !== 'source' guard in the orchestrator suite — is a type-narrowing fix, not a weakened assertion; I checked that the runtime semantics are identical, because a passing test that asserts less than it used to is exactly how a diff like this sneaks through.

On scope: this is #11594 cut from 43 files and 2,699 additions down to 17 and 446, with the two halves that carried the real architectural risk — the step-identity scheme and the extension-context loader — held back for PR B. If I had to maintain this in six months I would thank the author, mostly for that cut. The remaining surface is one 47-line module that four consumers genuinely share, and everything else is a field threaded through existing projections. Nothing here is trying too hard.

What keeps it at 4 rather than 5:

  • One fail-open path in an otherwise consistently fail-closed design (Stage 2, finding 1). Restart empties the registry, so if the journal is also gone while the snapshot survives, attribution is silently dropped and the new snapshot is written without it. Narrow, not a regression, and arguably out of scope — but it is the single place where this PR's own posture does not hold, and it contradicts the "restored from the journal after a restart" claim at the edge. Worth closing, cheaply, before or right after merge.
  • CI is not in. Test (ubuntu-latest, Node 22.x) and Lint & Static are both still running on the reviewed commit. feat(workflow): support prepared flows through native tools #11594 was blocked by exactly the first of these — one failure in 2,447 — so I am not treating "nothing is red yet" as "nothing is red". I read cross-package-contracts.test.js and neither of its two cases is touched by this diff, but that is a static read.
  • The test report the body promises is not in the thread. With "Tested on" at macOS-only and Stage 1e having put this in the elevated-depth bucket, that report was the only substitute for Windows and Linux coverage. I did not count the author's claimed results as evidence anywhere.
  • Two public-contract additions are forward declarations. agentStepId: false and workflowStepId: false advertise PR B. Defensible as capability discovery — an absent field means "old daemon", a false one means "new daemon, not implemented" — but it is API surface for code that does not exist, and the PR description does not say why the consumer needs it now rather than when PR B lands.

On the public-contract question I raised in Stage 1: I am not escalating it. Every addition is optional and additive, the two interfaces already carry workflowsEnabled?, savedWorkflows?, and WorkflowSnapshot.toolUseId? added the same way, the SDK file's own contract rule says only a v bump signals a breaking wire change, and I could name every downstream consumer — which is the bar. A maintainer who reads the contract surface differently should say so; that is a product call, not a code call.

Verdict

Approve, with the four notes above as non-blocking follow-ups rather than conditions. Nothing I found is a correctness bug, a security hole, or a regression, and the scope cut from #11594 is the right one.

CI is still running on the reviewed commit, so I am not approving in this run — approving now would attest to a result that does not exist yet. Approval is deferred until CI lands green on 9fbefabfb08c78a74e8576ead0e0f6acc62c1026; the finalize workflow posts the same commit-pinned approval once every check completes, and withholds it if anything lands red or the head moves.

中文说明

信心度:4/5 —— 扎实、纯增量、测试比必要程度更充分;我想提的四点都不构成阻塞,唯一真正的保留意见是 CI 还没跑完。

我的结论

回到我在读 diff 之前写下的方案:我勾了七步 —— 一个经过校验的可选参数、把来源作为 journal 的第一条记录持久化、带在 handle 与 registry task 上、resume 时还原且不允许给旧 journal 重新贴标签、投影到 ACP/SDK/web-shell 类型、在能力探测中声明、以及当归因运行没有可写 journal 时硬失败。这个 PR 七步全做到了,并且在两处超出我的方案:冻结一个新副本,使调用方在 start 之后的修改无法污染归因;以及把 registry 状态与 journal 状态交叉校验,从而抓住"journal 丢了来源记录但运行记录仍声称有"的情况并显式失败,而不是静默投影出去。这两点我都没想到,而它们正是那种只有在真正思考过"这个值会怎么出错"、而不只是"它怎么流动"之后才会出现的设计。

测试套件是我会推荐给后来贡献者的部分。252 行测试对 201 行生产代码,而且钉住的是要紧的断言而不是好写的断言:派发前写入的顺序是在 dispatch mock 内部断言的,直接从磁盘读 journal 第一行,所以把 append 挪后会让测试失败,而不只是让 PR 描述失真。别名防护是通过在 start 之后修改调用方对象、再断言 handle 仍持有原 revision 来演示的。旧 replay 形状用精确的 toEqual 钉住,这正是阻止条件展开悄悄加入 undefined 键、进而腐蚀别处断言的机制。而对既有测试的唯一改动 —— orchestrator 套件里的 entry.type !== 'source' 守卫 —— 是类型收窄修复,不是弱化断言;我专门核对了运行时语义完全一致,因为"通过但断言变少"正是这类 diff 蒙混过关的方式。

关于范围:这是把 #11594 从 43 个文件、2699 行新增削到 17 个文件、446 行,并把真正带架构风险的两半 —— 节点身份方案与扩展上下文加载器 —— 留给 PR B。如果六个月后要我维护这份代码,我会感谢作者,主要就是为了这一刀。剩下的面是一个 47 行、被四个消费方真实共享的模块,其余都是把字段串进既有投影。这里没有任何过度用力的地方。

让它停在 4 而不是 5 的原因:

  • 整体一致 fail-closed 的设计里有一条 fail-open 路径(Stage 2 第 1 条)。重启会清空注册表,因此如果 journal 也丢失而快照还在,归因会被静默丢弃,新写出的快照也不带归因。范围很窄、不是回归、也可以说超出本 PR 范围 —— 但这是本 PR 自身姿态唯一不成立的地方,并且在边界上与"重启后从 journal 还原"的表述相矛盾。值得以很低的成本补上,合并前或合并后立刻都行。
  • CI 还没跑完。 所审查 commit 上 Test (ubuntu-latest, Node 22.x)Lint & Static 都还在进行。feat(workflow): support prepared flows through native tools #11594 恰恰是被前者拦下的 —— 2447 个测试中失败 1 个 —— 所以我不把"目前没有红的"当作"没有红的"。我读了 cross-package-contracts.test.js,它的两个用例都不被本 diff 触及,但那是静态阅读结论。
  • 正文承诺的测试报告不在线程里。 在"测试平台"只有 macOS、且 Stage 1e 已把本 PR 归入更深审查档位的情况下,那份报告是 Windows 与 Linux 覆盖的唯一替代。我在任何地方都没有把作者自述的结果算作证据。
  • 两处公共契约新增属于提前声明。 agentStepId: falseworkflowStepId: false 在广告 PR B。作为能力探测是可以辩护的 —— 字段缺失意味着"旧 daemon",为 false 意味着"新 daemon,未实现" —— 但这毕竟是为尚不存在的代码开放 API 面,而 PR 描述没有说明消费方为什么现在就需要它、而不是等 PR B 落地时再加。

关于我在 Stage 1 提出的公共契约问题:我不升级处理。所有新增都是可选且向后兼容的,这两个接口上本来就有以同样方式加入的 workflowsEnabled?savedWorkflows?WorkflowSnapshot.toolUseId?,SDK 文件自己的契约规则也说明只有 v 号变化才代表破坏性 wire 改动,而且我能点名每一个下游消费方 —— 这就是判定门槛。如果 maintainer 对契约面的判断不同,请提出:那是产品判断,不是代码判断。

结论

批准,上述四点作为非阻塞的后续事项,而不是合并条件。我没有发现正确性缺陷、安全漏洞或回归,而相对 #11594 的范围收缩是正确的。

所审查 commit 上 CI 仍在运行,因此本次运行执行批准 —— 现在批准等于为一个尚不存在的结果背书。批准推迟到 CI 在 9fbefabfb08c78a74e8576ead0e0f6acc62c1026 上全绿之后;finalize 工作流会在所有检查完成后发布同样绑定该 commit 的批准,若有检查变红或 head 发生移动则不予批准。

Qwen Code · qwen3.8-max-2026-09-02

Reviewed at 9fbefabfb08c78a74e8576ead0e0f6acc62c1026 · re-run with @qwen-code /triage

@doudouOUC doudouOUC left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Threaded the new sourceRef through the whole path at the PR head. No blocking defect in the start/resume mainline — validation, ordering, the fail-closed guards and every read site behave as described. One finding on the restart path (inline), plus a coverage note I could not settle from code alone.

Checked and correct, so they do not need re-arguing: the ref is restored from the journal on every resume route regardless of what the caller passed (all of them funnel through WorkflowRunner.startClaimed); an explicit ref that conflicts with the journal is rejected before registration/dispatch, and a ref attached to a legacy journal is rejected too; older journals and snapshots without the field resume unattributed without error, and buildReplay skips entry types it does not know, so no migration concern; every added field has both a setter and a read site (no dead switch), and the display JSON already reaches the web-shell; previousEntry cannot misfire, because the registry ref and the journal ref both come from readRef's frozen copy; the ensureExists() reorder writes nothing new into <runsDir>/<runId>/ between the old and new positions, and the start-failure catch still removes the journal for fresh starts only (options.resumeFromRunId === undefined) — I verified that line, since it is what keeps the finding below narrow.

Coverage note (not filed inline, lower confidence). packages/cli/src/acp-integration/acpAgent.ts:12685-12686 is the only caller that supplies a ref and asks for a resume at the same time — the daemon retry path sets sourceRef from the task and resumeFromRunId: task.runId. On the state I describe in the inline comment (task carries a ref, journal yields none), the explicit-ref comparison at workflow-runner.ts:269-277 throws, so retry hard-fails, while the tool path with no explicit ref resumes silently on exactly the same state. If the child's registry is the hydration source I could not confirm, those two entry points disagree about the same run. A test pinning the retry pair would settle it; the new tests in workflow-correlation.test.ts drive WorkflowRunner.start directly and never reach this caller.

'Workflow sourceRef must match the original journal. Start a new run to use a different source.',
);
}
if (previousEntry?.sourceRef && !original) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After a restart this guard cannot fire, and the resumed settlement then overwrites the snapshot that still carried the reference — the loss becomes silent and destructive rather than merely ignored.

  • previousEntry is registry?.get(runId) on the in-process run registry, and nothing hydrates that registry from persisted snapshots, so it is undefined in exactly the situation the guard was written for.
  • journal.load() reports a missing or unreadable journal as "no entries" instead of an error (workflow-journal.ts:342-349, the catch returns empty maps), so original is undefined both for "no ref was ever recorded" and for "the journal that held the ref is gone", and the caller cannot tell the two apart.
  • With previousEntry === undefined and original === undefined the guard is skipped and this line clears the value. The resumed run then registers without a ref (the ...(sourceRef ? { sourceRef } : {}) spread at :349 contributes nothing), and settlement calls writeWorkflowSnapshot(config, entry)toSnapshot omits the field (workflow-snapshot.ts:92) → fs.writeFile replaces the file wholesale, so the ref that was on disk is erased.

Trigger: restart the CLI/daemon, then resume that run with resumeFromRunId while <runsDir>/<runId>/journal.jsonl is missing or corrupt and <runsDir>/<runId>.json is still there. The run completes unattributed and its snapshot is rewritten without the reference.

Suggested: fall back to the snapshot when the journal yields no source record — persisting the ref into the snapshot is what makes that possible — or refuse the resume when a snapshot with a ref is paired with a journal that has none, which is the disposition this in-process guard already takes. I am not filing it as blocking: the precondition is narrow, since the only journal deletion in this file is gated on options.resumeFromRunId === undefined (fresh starts), so it takes the journal being removed out from under an existing run by something else. But the outcome is destruction of already-recorded metadata rather than a missing field, so it is worth a deliberate decision.

@qqqys
qqqys enabled auto-merge September 15, 2026 11:57

@yiliang114 yiliang114 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM at 078d86c — no blocking issues. Bot review came back clean (no merge blockers, 4/5) and CI is green. One open thread from doudouOUC: after a restart the in-process registry guard can't fire and a missing/corrupt journal makes a resumed settlement erase a snapshot's source ref — narrow precondition (journal must be deleted out from under an existing run), explicitly filed as non-blocking, but it deserves a deliberate decision from the author: either fall back to the snapshot when the journal has no source record, or refuse the resume. Fine as a follow-up; not a merge blocker.

@chiga0 chiga0 left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking findings.

Checked: sourceRef persisted before first dispatch (journal.append in setup block before assertStartNotCancelled); resume correctly restores original sourceRef from journal; conflicting sourceRef rejected before dispatch; journal-unavailable case fails safely before dispatch; sourceRef input validation (exactly 2 keys, strings only, max 256 chars, no control chars, Object.freeze, additionalProperties:false); backward compatible (optional at every layer, legacy journals replay cleanly, omitting sourceRef takes original path); capability discovery correctly advertises sourceRef:true.

Reviewed with AI assistance.

@qqqys
qqqys added this pull request to the merge queue Sep 15, 2026
Merged via the queue into main with commit c074bc0 Sep 15, 2026
90 of 92 checks passed

@qwen-code-dev-bot qwen-code-dev-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

APPROVE

核对基线:head 078d86c9f1(base d313505f,17 个文件 +446/-7)。required 档全部完成且成功(Test (ubuntu-latest, Node 22.x)Lint & StaticIntegration Tests (no-AK, No Sandbox)web-shell E2E SmokeTUI parity snapshotsOpenTUI no-flicker gateReal daemon E2E / Java 11Live Host (macos-latest)Capture web-shell visuals);该 head 上 22 项非跳过检查全部 success,无 pending(本 PR 未触发 Serve A/B)。该 PR 已于 2026-09-15T11:59:40Z 由作者本人合入(合并提交 c074bc02ae),本票是补记,没有参与放行。

独立复核未发现新的 Critical。新增来源引用的入口只有 readWorkflowSourceRef:恰好两个键、每段 1–256 字符、去首尾空白、拒绝控制字符,且 isWorkflowSnapshot 在读回快照时用同一个判定复核 sourceRef,所以下游 tasksSnapshot.ts / workflowsCommand.ts / web-shell 导出这几处不再验证是安全的;daemon 侧以 workflowToolFeatures: {sourceRef: true, ...} 明确声明能力,老客户端不至于按不存在的字段行事。journal 侧把 source 记录作为唯一真源(写入在 dispatch 之前,写失败就不派发;冲突或非法的 source 记录变成 sourceError 并在 runner 直接抛错),workflow-correlation.test.ts 的六条用例(:50 先持久化再派发、:163 写失败不派发、:183 不给 legacy journal 补挂新归属、:206 冲突归属仍保留旧回放映射)都是能失败的见证。

遗留项(不阻塞,但这条线程在 GitHub 上仍是 unresolved 状态就合入了)workflow-runner.ts:279 的守卫依赖 previousEntry = registry?.get(runId):214,进程内注册表),全仓没有任何从快照回填注册表的路径,因此重启后它是 undefined;同时 workflow-journal.ts:342-349load() 把读不到的 journal 归成「没有条目」,于是 :279 跳过、sourceRef = original 取到 undefined,最终 writeWorkflowSnapshottoSnapshot(task) 整体覆写文件,把原本还留在快照里的 sourceRef 抹掉。触发面确实窄(要重启 + resumeFromRunId + journal 被外部删除或损坏,本 PR 自己不会在 resume 路径删 journal),而 workflow-correlation.test.ts:116「recovers source after restart without reading snapshots」说明「journal 才是真源、快照只是投影」是有意的取舍。建议按线程给出的两个方向之一收敛(journal 无 source 记录时回落到快照,或快照有、journal 无时拒绝 resume),并补一条覆盖该状态的用例。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants