Skip to content

fix(test): make chat recovery fixture replay completed turns - #4250

Merged
huangruiteng merged 1 commit into
mainfrom
codex/chat-recovery-fixture-replay
Sep 11, 2026
Merged

huangruiteng merged 1 commit into
mainfrom
codex/chat-recovery-fixture-replay

Conversation

@huangruiteng

Copy link
Copy Markdown
Collaborator

The browser fixture could return an empty SSE stream to a reconnecting client once the original request cleared the active Turn. This makes reload acceptance timing-dependent and differs from the durable completion replay of the real store.

Both fixture event routes now share one completion record: persist the answer and completion before sending, replay the same terminal events after completion, and leave interrupted or superseded Turns untouched. Session snapshots also expose their active Turn. The browser acceptance checks two reconnecting clients receive identical completion events without duplicating the answer, and reports the caught failure detail in CI.

This changes only the browser fixture and its assertions. Production frontend, backend and packaged assets are unchanged; no companion product UI or configuration migration is needed.

Validation: the full development browser smoke with CI coverage enabled passed all acceptance criteria. A local negative control that disables completed-event replay fails only criterion 6, specifically on missing identical terminal replay; it is excluded from the PR. JavaScript syntax and diff checks passed. This addresses the fixture flaw identified while following the repeated reload check failure on #4244; the original generic CI log does not establish which sub-step timed out, so the exact caught error is now retained.

Signed-off-by: huangruiteng <huangrt01@163.com>
@huangruiteng

Copy link
Copy Markdown
Collaborator Author

No blocking findings after independent diff review. This is a fixture-only correction: both SSE routes now persist/replay one completion before responding, without overwriting a different active Turn. The production frontend, backend, permission and configuration behavior are unchanged; no rebuilt product bundle is needed.

Validation: complete CI-mode instrumented browser acceptance passed; the negative control that removes completed-event replay fails only the new criterion 6 assertion. JavaScript syntax, diff and public-boundary checks passed. The temporary negative control, dependencies and generated results are excluded.

The bounded refactor removes duplicate completion behavior from the two fixture routes. Mid-stream after/Last-Event-ID replay remains an existing fixture limitation, outside this late-reconnect correction. The original remote generic failure is not retroactively claimed to expose a precise sub-step; failures now include that detail.

Self-merge under the owner's standing authorization for validated manager fixes. DCO passed; full remote CI has not yet completed. The completed local fixture acceptance and negative control are the risk-based validation for this test-only change.

@huangruiteng huangruiteng left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Approval conclusion (author-owned PR; GitHub blocks formal self-approval)

审阅对象:PR #4250,exact head 3fb80e78dd55172d6cfcd81eccb7daaa1d23a88a,合并提交 6c0a43c340fe4b58e8ba477d25d7250508ddd4f0。本记录是该 exact head 的合并后审计,只覆盖这一 head 的完整增量(1 file, +34/-31)。

动机

这个 PR 修的是浏览器 smoke fixture 的一个真实缺陷,而不是"为了让 CI 变绿而放宽断言"。fixture 的两条 turn 事件路由各自构造完成体,并且当会话的 active_turn_id 已经不再匹配请求的 turn 时,会对重连客户端返回空的 SSE 流。这会产生两个后果:一是 reload 验收变成时序相关的(原始请求先清掉 active turn,重连就拿到空流并超时);二是它与真实 store 的"完成事件可持久重放"语义不一致——也就是说,这个 fixture 在过去几次 CI 里报出的正是 Reload did not restore the active Goal conversation and reconnect its active Turn within 10 seconds.,而我上一轮审 #4246、#4244 时已经用父提交复现确认:这两次合并的 checks 红项属于既有 fixture 缺陷、不是那两个 PR 引入。这个 PR 就是那条线索的收尾。

价值因此很直接:它把一个反复消耗合并周期的假阴性修掉,让 reload/重连验收重新具备"区分产品回归与 fixture 抖动"的能力,同时没有新增任何 smoke 文件或生产改动。

改动思路

思路是"让 fixture 与真实 store 的语义一致,并把断言做在真正的契约上":

  • 抽出共享的 finishTurn(sessionId, turnId, answer, protectedAction):首次完成时先持久化 assistant 消息与完成体、清掉 active_turn_id,并把该 (session, turn) 的终态事件缓存下来;之后的任何重连请求都返回同一份终态事件,而不是空流。中断或被取代的 turn 记录保持原样。
  • 两条事件路由(恢复路径与 operator 路径)都改为调用这一个 helper,替换掉两段重复的完成体构造逻辑——这是"压缩而不是追加"。
  • 会话快照(GET /api/chat/sessions/<id>)在存在活跃 turn 时返回 active_turn: {turn_id, status: "running", response: null},与真实接口一致,这样 reload 走的"恢复活跃 turn"分支才是被真正执行的路径,而不是被 null 短路。
  • 验收断言从"10 秒内恢复"升级为可诊断的契约断言:两次重连请求都必须包含 event: turn.completed 且逐字节一致,并且持久化的回答只能存在一份;失败信息改为携带捕获到的真实错误。

具体改动

唯一改动文件是 examples/personal-workspace-browser-smoke.mjs(+34/-31):新增 finishTurn 并让两条 POST .../turns/<turn>/events 路由复用它;GET .../sessions/<id> 增加 active_turn 字段;criterion 6 改为两次 fetch 的终态事件一致性与"回答不重复"断言,fail(6, ...) 现在输出 error.message。

关键点讲解:

  1. finishTurn:既是持久化点也是重放点。只有在真实完成请求到来时才写入缓存,因此不会对"从未完成的 turn"伪造完成事件;返回空流的分支仍保留给"会话不在该 turn 且没有完成记录"的场景。
  2. 快照 active_turn:这是让 reload 走恢复路径的关键一行。此前 fixture 恒返回 null,等于把"恢复活跃 turn"的入口从 fixture 层面关掉了。
  3. criterion 6 的新断言:turn.completed 必须存在且两次响应一致,回答计数必须为 1。前者保证"重连能看到终态",后者保证"重连不会把回答渲染两遍"——后者才是产品级的去重守卫。
  4. 失败信息:把捕获到的错误写进 criterion 6 的 note,并在失败时截图与记录 observations。这个改动立刻产生了效果:本轮审阅时该 head 上的 checks 失败信息已经从原来笼统的超时文本变成 locator.waitFor: Error: strict mode violation: getByText('正在整理…') resolved to 2 elements,也就是说被修的那个缺陷不再复现,暴露的是另一个无关的、脆弱的定位器(当日由 #4251 修复)。

对主干的风险

这是纯 fixture 变更:不涉及生产前端、后端、打包产物、配置或迁移,因此主干运行时代码的风险为零。需要审视的是"fixture 是否掩盖产品回归":

  • 缓存只在真实完成请求后建立,未完成的 turn 仍返回空流,不会伪造终态。
  • "两次响应一致"这一条本质上更多是 fixture 确定性自检(两次都打到 fixture 路由);产品级守护来自同一 criterion 里的"重连恢复历史/恢复活跃 turn"与"回答只存在一份"两条断言,二者都在同一处保留。
  • 失败信息携带真实错误后,定位器歧义之类的产品侧脆弱点更容易被发现——本例正是如此(#4251)。

关于该 head 上仍为红的 checks/pytest/merge-gate:这不是本 PR 的缺陷。本 PR 针对的空流失败已不再复现;随后红项先是上面那条定位器歧义(#4251 修),再是与此无关的 mypy 回归(#4257 修)。最近 main 上承载 checks 的 Python Tests 工作流是绿的,我前两轮审 #4246/#4244 时也用父提交复现证明了当时的红项属于既有 fixture 问题。

未验证项如实标注:本次审计没有在本地复跑 Playwright smoke(需要浏览器与已构建的 dashboard),证据来自该 PR 记录的开发模式全通过、被排除的负向对照(禁用完成事件重放时只让 criterion 6 失败),以及 CI 日志中失败信息的实际变化。

我的整体评价

APPROVE。这个 PR 修的是一个已经反复消耗合并周期的假阴性,做法克制且方向正确:让 fixture 与真实 store 的持久重放语义一致、把重复的完成体构造合并成一个 helper、并顺势把验收断言收紧到"终态事件可重放且回答不重复"。它没有新增 smoke 文件、没有放宽任何断言、没有触碰生产代码,而且失败信息现在会带上真实原因——这正是后续那条定位器歧义能被迅速定位并修复的原因。

无阻断发现。唯一值得记下的建议(非阻断):replayed[0] !== replayed[1] 这条更多是 fixture 自检,真正守产品行为的是同组的"重连恢复"与"回答恰好一份"断言;保留两者即可,但后续若再扩展 criterion 6,应优先增加产品可观测行为的断言而不是 fixture 内部一致性断言。残余风险:本轮未本地复跑 smoke;被排除的负向对照脚本必须继续留在仓库之外(PR 正文已声明)。

English verdict: APPROVE - #4250 at exact head 3fb80e7 (merged as 6c0a43c) is a fixture-only repair that stops the browser smoke from answering a reconnecting client with an empty SSE stream after the active Turn was cleared: both event routes now share one finishTurn helper that persists and replays the identical terminal events, session snapshots expose the real active_turn, and criterion 6 asserts replayed turn.completed identity plus a single persisted answer with the caught error reported on failure. It removes two merged bodies of duplicated fixture logic rather than adding coverage, changes no production frontend/backend/packaged asset, and is verified by the CI acceptance itself: at this head the old Reload did not restore ... failure no longer reproduces and criterion 6 instead surfaces an unrelated strict-mode locator fragility that #4251 repaired the same afternoon (a later mypy regression was repaired by #4257, and recent main checks runs are green). Durable-value check: it guards the reload/reconnect journey of the packaged personal workspace, is consolidated rather than duplicated, is public-safe and synthetic, and the #4250/#4251 pair are distinct diagnosed repairs rather than same-shape farming. No blocking finding; one advisory that the identical-body assertion is largely a fixture self-check while the reconnect/no-duplicate pair is the real product guard, and this audit did not re-run the Playwright smoke locally.

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.

1 participant