Conversation
helsome
left a comment
There was a problem hiding this comment.
这组 #41 → #42 → #43 的方向是对的,但当前还不建议把协议定型进 main,有两个架构阻塞需要先解决:
messageId === runId不应作为 v1 固化。#34 已明确要求 message / generation / run 是不同身份:message 有稳定 id,assistant generation 绑定具体 run。现在把两者合并会直接给 edit/regenerate/fork 留下错误语义。建议让 message 级事件携带真实 messageId;run 级事件可只依赖 runId,或 messageId 可选,但不要把两者定义成同一身份。- ADR 写了 reconnect=
lastSequencereplay,但当前 stack 只有 parallel event channel + renderer log/dedupe,没有 replay source/API/buffer,也没有“明确无法恢复”的失败路径。也就是说 contract 文档承诺了实现尚不存在的行为。
另外 #43 自己说明 #41 的 StreamEvent 不是正确 discriminated union,要到 #43 才修,所以不要单独 merge #41/#42 后在 main 留一个已知有缺陷的协议类型。
建议:在本 stack 顶部统一修掉 identity contract,并至少实现一个明确的 reconnect 行为(内存 replay 也可以,或明确 terminal failure),然后补 Issue #27 要求的真实 Copilot E2E:model + tool + citation,分别验证 cancel 和 transport interruption。完成后这组三层可以按依赖顺序合。
Addresses helsome#43 review: messageId is no longer fused with runId (issue helsome#34 - message/generation/run are distinct identities). message-level events carry the real assistant messageId, pre-assigned per run in RunManager; run-level events omit it; idempotency key is now runId + sequence. Adds StreamEventHistory: in-memory per-run tail used by RunManager.replayStream(runId, lastSequence) for reconnect resume, with an explicit recoverable:false path when the run is unknown or the tail is non-contiguous (eviction).
|
Thank you for the detailed review. Both blockers are now addressed at the top of this stack (new commits just pushed): 1. Identity contract (#34) —
2. Reconnect — implemented a concrete in-memory replay plus an explicit unrecoverable path:
Verification: adapter (7, incl. messageId injection) + history (5) + kernel E2E regression (6) all pass; electron/core/ui/shared tsc show no new errors beyond the pre-existing local On the real Copilot E2E (model + tool + citation, cancel and transport interruption): I'll wire this next against the eval fixture infrastructure so it runs without external API keys — will follow up in this thread before we merge. |
作者已解决原 review 的两个实质 blocker(message/run identity 与 reconnect replay)。重新评估后,真实完整 E2E 不再作为当前合入前置条件;剩余仅需解决与最新 main 的 Git 冲突并通过基础 CI。
helsome
left a comment
There was a problem hiding this comment.
重新审查后通过代码方向。你已经解决了上一轮的两个核心 blocker:messageId / runId 身份拆分,以及明确的 in-memory replay / unrecoverable reconnect 路径。完整真实 Copilot E2E 可以作为后续增强,不再作为当前合入前置条件。现在只剩 Git 层面的 dirty:请把最终 #43 rebase 到最新 main,保留 Run Budget / Provider Router 等主干后续改动,并让基础 CI(unit/typecheck/secret scan)跑通;完成后可直接合。
f345099 to
d953606
Compare
Addresses helsome#43 review: messageId is no longer fused with runId (issue helsome#34 - message/generation/run are distinct identities). message-level events carry the real assistant messageId, pre-assigned per run in RunManager; run-level events omit it; idempotency key is now runId + sequence. Adds StreamEventHistory: in-memory per-run tail used by RunManager.replayStream(runId, lastSequence) for reconnect resume, with an explicit recoverable:false path when the run is unknown or the tail is non-contiguous (eviction).
d953606 to
05ae328
Compare
Addresses helsome#43 review: messageId is no longer fused with runId (issue helsome#34 - message/generation/run are distinct identities). message-level events carry the real assistant messageId, pre-assigned per run in RunManager; run-level events omit it; idempotency key is now runId + sequence. Adds StreamEventHistory: in-memory per-run tail used by RunManager.replayStream(runId, lastSequence) for reconnect resume, with an explicit recoverable:false path when the run is unknown or the tail is non-contiguous (eviction).
517434d to
b9e2cc5
Compare
审核标准已更新:Ready PR 在 APPROVE 前必须补齐可复现测试报告(至少 Bun 版本、OS、实际命令与结果)。原审批基于旧标准,先撤销,代码方向本身仍认可。
helsome
left a comment
There was a problem hiding this comment.
代码方向仍然认可,上一轮 identity/reconnect blocker 已修;这次 Changes Requested 只是同步当前统一验收规则。请在 clean rebase 最新 main 时,把测试报告补成可复现格式:Bun 版本、OS/平台、实际 focused test/typecheck 命令与 pass/fail;现有 protocol/renderer/kernel 测试范围够了,不需要补完整真实 Copilot E2E。rebase + 基础 CI + 报告字段齐全后即可恢复 APPROVE。
Addresses helsome#43 review: messageId is no longer fused with runId (issue helsome#34 - message/generation/run are distinct identities). message-level events carry the real assistant messageId, pre-assigned per run in RunManager; run-level events omit it; idempotency key is now runId + sequence. Adds StreamEventHistory: in-memory per-run tail used by RunManager.replayStream(runId, lastSequence) for reconnect resume, with an explicit recoverable:false path when the run is unknown or the tail is non-contiguous (eviction).
b9e2cc5 to
e0d46c4
Compare
|
这轮复查有两个更新:
合并策略调整为:#43 clean 后直接以 stack 顶部合入 #41→#42→#43 的完整协议增量,不再分别 squash #41/#42,避免重复历史;随后再处理 #76 的 persistence-only delta。 |
749da22 to
eb75fc7
Compare
|
已按你的两点执行完毕: 1. clean rebase:整个 2. 跑现有 PR checks:已重跑,全部 pass —— Typecheck / Focused tests / Full unit tests (advisory) / Secret scan,未扩大测试范围。 顺带按 #34 与 ADR 收敛了三处协议边界(都属于本 PR 已声明契约的落地,没有新增功能面):
合并策略保持你定的那份:以本 PR 栈顶一次性带入 |
当前 PR 描述已补齐 Bun 1.4.2 / Windows 11、focused 44/0 与 CI 四项结果;identity/replay 架构 blocker 也早已修复。原验收材料 blocker 已解决。
helsome
left a comment
There was a problem hiding this comment.
当前栈顶最终状态可以批准:message/run identity 已拆开,in-memory replay 与 unrecoverable reconnect 路径明确,取消时 partial answer 保留;测试报告也已满足 Bun/OS/命令/44 pass/CI 要求。不要单独合 #41/#42,继续以 #43 栈顶承载完整协议增量。现在唯一阻塞是 main 已明显前进,请 clean rebase 当前 main,保留新近 RunManager/Citation 等主干改动后重跑这 6 个 focused suites + typecheck/basic CI;不需要再补完整真实 Copilot E2E。
eb75fc7 to
584bb56
Compare
Addresses helsome#43 review: messageId is no longer fused with runId (issue helsome#34 - message/generation/run are distinct identities). message-level events carry the real assistant messageId, pre-assigned per run in RunManager; run-level events omit it; idempotency key is now runId + sequence. Adds StreamEventHistory: in-memory per-run tail used by RunManager.replayStream(runId, lastSequence) for reconnect resume, with an explicit recoverable:false path when the run is unknown or the tail is non-contiguous (eviction).
|
已按你的要求完成 clean rebase,现在只剩等你点合并: 1. clean rebase 到最新 main
2. 基础 CI 已复绿(新 commit 上重跑) 3. 测试报告补齐可复现字段 补一句如实说明: 没有新增功能、没有扩大测试范围、没有动 |
helsome
left a comment
There was a problem hiding this comment.
已复审当前完整 #41→#42→#43 栈顶 diff、测试报告及该 head 的成功 CI。并行事件通道、message/run 身份、序列去重、内存 replay、不可恢复返回和取消 partial 均可作为独立增量落地;不要求切换全部 renderer 或完成 #27 真实产品 E2E,#27 继续保持 open。
#76 的恢复时重复写回 JSONL 缺陷已经单独反馈;它不在本 PR 内,不阻塞当前内存协议层。本次优先保留栈祖先的 merge commit,避免先 squash #41/#42 后引入重复历史。若与刚合入 #85 的 RunManager 产生实际 Git 冲突,只需保留两边语义解决冲突并跑基础 CI,无需扩大功能范围。
Introduce the pure-type foundation for the structured streaming event protocol (issue helsome#27): versioned envelope, 12 typed events, and the idempotency/cancel/reconnect contracts as types. Zero runtime change. Adds ADR 0001 documenting context, decision, migration path and open questions for maintainer review.
Emit Stream Event Protocol v1 events alongside the existing AgentEvent stream (issue helsome#27, ADR 0001 migration step 2). Adds toStreamEvents mapping (8 AgentEvent types -> 12 protocol events, cancel normalized to 'cancelled') and RunManager.subscribeStream. Existing AgentEvent consumers are untouched; the parallel channel only activates when a stream subscriber is registered.
StreamEvent was a single indexed-union instantiation (Tagged StreamEventEnvelope<StreamEventType>), so payload could not be narrowed by type in switch/if. Rewrite as a distributive mapped union; on-disk type shape is unchanged. Adjusts adapter unit-test helper accordingly.
KernelHost subscribes RunManager.subscribeStream (issue helsome#27) and forwards { sessionId, event } over IPC channel 'agent:stream'; preload exposes electronAPI.kernel.onStreamEvent. Legacy 'agent:event' delivery untouched. Transport only; renderer consumption follows.
Renderer-side data layer for issue helsome#27: KernelBridge subscribes client.kernel.onStreamEvent into a parallel StreamEvent log. reduceStreamLog keeps per-run events ordered by sequence, dedupes replays (drops), and flags gaps/out-of-order (anomalies) as a protocol health signal. FinagentClient and preload.cjs wire onStreamEvent; FinagentClient adds onStreamEvent contract with fallback noop. No visual change: existing AgentEvent rendering untouched.
Addresses helsome#43 review: messageId is no longer fused with runId (issue helsome#34 - message/generation/run are distinct identities). message-level events carry the real assistant messageId, pre-assigned per run in RunManager; run-level events omit it; idempotency key is now runId + sequence. Adds StreamEventHistory: in-memory per-run tail used by RunManager.replayStream(runId, lastSequence) for reconnect resume, with an explicit recoverable:false path when the run is unknown or the tail is non-contiguous (eviction).
Adds 'runs:stream-replay' handler (KernelHost.streamReplay -> RunManager.replayStream), preload (ts + cjs) streamReplay, finagentClient wiring and FinagentClient contract with fallback noop.
…plementation Envelope messageId is now optional (message-level events only); idempotency key runId+sequence; Reconnect row documents implemented StreamEventHistory + IPC with explicit unrecoverable path; open question 3 resolved.
run_started(seq 1)与 runtime 自产的首个事件(同样 seq 1)冲突, 导致 replay(runId, 0) 被判为不连续(recoverable: false),renderer 的 幂等去重也会误丢事件。RunManager 现为每个 run 持有 RunProtocol 计数器, 扇出前把所有 AgentEvent 统一重排为 1..N;messageId 随该对象传递, 崩溃/取消兜底合成的 terminal 事件也保住 helsome#34 的身份契约。replay 游标 超出已知最大 sequence 时,由静默视为已同步改为明确不可恢复。
kernel 级(stream-replay.e2e.test.ts):真实 LocalRuntimeAdapter + RunManager + 持久化。六个用例:完整 run 后全量补发、无实时订阅者时 事件仍入历史、按 lastSequence 中途断线补发、取消路径产生可补发的显式 cancelled 事件、带工具 run 的全量流、未知 run 与伪造游标返回明确的 不可恢复路径。 app 级(e2e/stream-replay.mjs):真实 Electron + preload IPC(CDP)。 四个用例:replay(runId, 0) 与实时投递逐字节一致、断线补发拼接还原 完整流、未知 run 跨 IPC 返回不可恢复、非法 lastSequence 被拒以 INVALID_ARGUMENT。
AgentKernelHost.attach wires the Stream Event v1 channel through kernel.runs.subscribeStream; the fake kernel in kernelHost.test.ts lacked the method, so the transport test crashed with TypeError before asserting. Align the fake with the kernel surface (subscribeStream + replayStream) so focused CI runs green again.
The Stream Event v1 IPC surface (streamReplay / onStreamEvent) landed in the kernel channel types; the test kernel client was not updated, which broke the ui + i18n + electron typecheck gates. Align the stub with the channel so typecheck is green again.
streamReplay returned an inferred StreamReplayResult that TS could not name portably across the @finagent/shared boundary (error TS2742). Export the type from the shared package and annotate the kernelHost surface so every package typecheck gate passes.
- stream-event-adapter:run_failed 按 error.code 归一为 cancelled(user/budget/runtime), 并把已生成的 partial 文本带进 cancelled.partial.text(对齐 ADR 0001) - run-manager:run 全程保留 assistant 文本快照,供 cancelled 事件使用 - kernelHost:streamReplay 游标限定为非负整数;dispose 时一并清理 stream 订阅 - ui/streamAtoms:幂等去重仅在 sequence 回退时扫描,避免长 run 下的 O(n²) - 补齐 adapter / streamAtoms / kernelHost 边界测试,electron E2E 增加非法游标用例
584bb56 to
e67969b
Compare
背景与动机
copilot 的回答流目前走隐式
AgentEvent(8 事件)+ 单一 IPC 通道agent:event,缺少版本号、单调sequence契约、显式取消事件与重连补发能力(issue #27)。#41 落协议类型、#42 在RunManager并行产出协议事件;本 PR 完成 transport 与 renderer 数据层接线,并收敛取消/重连的边界行为。完成了什么(scope)
第三步只做"并行接线 + 边界收敛":不改变现有
agent:event路径,无可见 UI 变化(因此不附截图)。Transport
apps/electron/src/main/kernelHost.ts:attach()订阅RunManager.subscribeStream并把{ sessionId, event }转发到新通道agent:stream;dispose()一并清理该订阅;新增streamReplay(input)(IPCruns:stream-replay)暴露重连补发,游标限定为非负整数(负数/小数/NaN/非数字一律INVALID_ARGUMENT)。preload/index.ts+preload/index.cjs:onStreamEvent(cb) -> unsubscribe与streamReplay,与既有onAgentEvent对称。renderer/finagentClient.ts+packages/ui/src/client.tsx:FinagentClient契约与 fallback client 同步。Renderer 数据层
packages/ui/src/atoms/streamAtoms.ts(新增):纯函数reduceStreamLog按 run 维护有序事件、按runId + sequence幂等去重、把 gap/乱序计为anomalies;幂等扫描只在 sequence 回退时触发,避免长 run 的 O(n²)。暴露streamLogAtom/applyStreamEventAtom。KernelBridge订阅onStreamEvent写入日志;旧agent:event→applyAgentEventAtom路径未改动。协议边界收敛(对齐 #34 身份模型与 ADR 0001)
stream-event-adapter:run_failed按error.code归一 ——RUN_CANCELLED→cancelled{reason:'user'}、BUDGET_EXHAUSTED→{reason:'budget'}、RETRY_STORM/LOOP_DETECTED→{reason:'runtime'},其余保持error;cancelled.payload.partial.text带出该 run 已生成的文本(ADR 的 "partial answer preserved" 由此落地)。run-manager:run 全程保留 assistant 文本快照供cancelled使用;sequence在唯一汇聚点统一重排(runtime 与本管理器都自产事件、都会从 1 计数)。packages/core/src/stream-events.ts:StreamEvent改成真正的可判别联合(feat(core): add Stream Event Protocol v1 types (ADR 0001) #41 的写法无法按type收窄payload);形状不变。向后兼容
AgentEvent、agent:event通道与applyAgentEventAtom全部保持原样;协议事件是并行通道,没有订阅者时不做额外工作。UI 渲染仍由旧路径驱动(text_delta渲染切换是后续 PR)。关联 Issue
Refs #27(第三步,不关闭)· #34(message/run 双身份)· #18(终态一致)· #19(security 约束)。依赖 #41 → #42。
验证
环境:Bun 1.4.2 / Windows 11 (NT 10.0.26200)
CI(本 PR 的 checks,全部 pass)
本地聚焦测试(改动后的代码)
本地工作区测试(rebase 后于新栈顶复跑,依赖已按
--linker=isolated装全)bun test packages/shared首次在沙箱内运行时,PiRuntimeAdapter > removes the session conversation file on disposeSession因/tmp在本机映射到不可写路径报EPERM(非断言失败、与协议无关);在具备写权限的同一工作区复跑该套件为 936 passed / 0 failed,故不计为本 PR 引入的失败。说明:
apps/electron/e2e/stream-replay.mjs需真实 Electron 构建,由 release/nightly 流程覆盖。已知限制 / 后续
text_delta增量渲染与显式cancelled状态尚未接到 UI(本 PR 只到数据层),建议在 ADR open questions 收敛后单独跟进。error.retryable目前恒为false(v1 不做重试分类)。StreamEventHistory(每 run 2000 事件 / 32 run),跨重启持久化在 feat(shared): persist stream event log for cross-restart replay (#75) #76。Rebase 说明
已 clean rebase 到最新
main(6405b7b,含 #89 / #88 / #94 等新提交),14 个 commit 顺序重放、零冲突;main 侧的research-prompts抽取与citations导出均完整保留。栈为 #41 → #42 → #43(
e2a59f5→82837e6→584bb56);按维护者指示,本 PR 的 diff 收敛为纯协议增量,合并时以本栈顶一次性带入 #41/#42/#43,不再分别 squash #41/#42。合并前请以当前 approval 为准,无需再改代码。