Skip to content

fix(cli): submit exact OpenTUI slash commands from live input - #10987

Merged
yiliang114 merged 1 commit into
mainfrom
codex/fix-opentui-live-slash-submit-10976
Sep 4, 2026
Merged

fix(cli): submit exact OpenTUI slash commands from live input#10987
yiliang114 merged 1 commit into
mainfrom
codex/fix-opentui-live-slash-submit-10976

Conversation

@yiliang114

@yiliang114 yiliang114 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

What this PR does

This change makes OpenTUI decide whether Enter should submit an exact slash command from the editor's current text instead of completion state produced by the previous React render. It also adds regression coverage for both stale-state directions: an exact /quit must submit, while a partial command must not be dispatched as an old exact match.

Why it's needed

The main-branch OpenTUI E2E job repeatedly failed because the final t in /quit and Enter can arrive in the same React frame. The editor already contains /quit, but the completion state can still describe /qui, so Enter accepts the stale suggestion instead of submitting the command. The input remains on screen and the process never reaches the already-correct interrupt and exit path. The stale Enter path originated in the OpenTUI input batch from #10368. The same job's bare exit case succeeds because it does not enter slash completion. This is the OpenTUI counterpart of the stale completion race fixed for Ink by #10929.

Failing job: https://github.com/QwenLM/qwen-code/actions/runs/33820259657/job/100861214441

Reviewer Test Plan

How to verify

  1. Hold a model response after its first streamed chunk, type /quit, and press Enter immediately after the final character. The active response should be interrupted and the CLI should exit with code 0 without waiting for the held stream.
  2. Type a partial slash command while completion state still reflects a previous exact match. Enter should accept completion rather than dispatching the partial command.
  3. Confirm the existing OpenTUI mid-turn submit E2E and the focused composer unit tests pass.

Evidence (Before & After)

Before: the linked main-branch job retried the /quit case three times; each attempt timed out after 30 seconds while the input and completion menu remained visible. The bare exit case completed in 3.7 seconds.

After: the regression test sends the final t and Enter in one React frame and expects /quit to be submitted and the editor to clear. Standard PR CI remains the authoritative OpenTUI runtime verification because the isolated local worktree did not have the newly required @opentui/react package installed.

Tested on

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

Environment (optional)

Focused ESLint and Prettier checks passed. The focused Vitest file could not be collected in the isolated worktree because its dependency snapshot predates the OpenTUI packages; no full dependency installation or broad build was run locally.

Risk & Scope

Linked Issues

Fixes #10976

中文说明

本 PR 做了什么

本改动让 OpenTUI 在按下 Enter 时直接依据编辑器当前文本判断是否为完整 slash command,不再依赖上一次 React 渲染产生的补全状态。同时增加两个方向的回归覆盖:完整的 /quit 必须提交,而部分命令不能因为旧状态曾经是完整匹配就被错误执行。

为什么需要

main 分支的 OpenTUI E2E 反复失败,是因为 /quit 最后的 t 和 Enter 可能在同一个 React frame 内到达。此时编辑器已经是 /quit,但补全状态仍可能停留在 /qui,Enter 因而接受旧补全项而不是提交命令。输入内容继续留在界面上,进程也没有进入本来已经正确的中断和退出路径。这个使用旧状态的 Enter 路径最初由 OpenTUI 输入批次 #10368 引入。同一 job 中裸 exit 能成功,是因为它不会进入 slash 补全。这是 #10929 已为 Ink 修复的补全状态竞态在 OpenTUI 中的对应问题。

失败 job:https://github.com/QwenLM/qwen-code/actions/runs/33820259657/job/100861214441

Reviewer Test Plan

如何验证

  1. 在模型响应输出第一个流式片段后保持流不结束,输入 /quit,并在最后一个字符后立即按 Enter。活动响应应被中断,CLI 应以退出码 0 结束,不等待被保持的响应流。
  2. 当补全状态仍记录着之前的完整匹配时输入部分 slash command。Enter 应接受补全,而不是执行这个部分命令。
  3. 确认现有 OpenTUI mid-turn submit E2E 和聚焦的 composer 单元测试通过。

前后证据

改动前:链接中的 main 分支 job 对 /quit case 重试三次,每次都在 30 秒后超时,输入和补全菜单仍留在屏幕上;裸 exit case 在 3.7 秒内完成。

改动后:回归测试在同一个 React frame 中发送最后的 t 和 Enter,并断言 /quit 被提交且编辑器被清空。由于隔离的本地 worktree 没有安装新要求的 @opentui/react 包,标准 PR CI 是 OpenTUI 运行时的权威验证。

测试平台

OS 状态
🍏 macOS ⚠️ 未测试
🪟 Windows ⚠️ 未测试
🐧 Linux ⚠️ 未测试

环境(可选)

聚焦的 ESLint 和 Prettier 检查已通过。隔离 worktree 的依赖快照早于 OpenTUI 包,因此无法收集聚焦的 Vitest 文件;本地没有执行全量依赖安装或整仓构建。

风险与范围

关联 Issue

Fixes #10976

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@qwen-code-ci-bot

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

Copy link
Copy Markdown
Collaborator

Qwen Triage finishedview run. See the stage comments in this thread for the result.

Qwen Triage 已完成 —— 查看运行。结果见本线程中的各阶段评论。

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — every section filled in, and the Chinese body is a real translation rather than a stub.

Problem: observed, not theoretical. #10976 was auto-filed from a main-branch E2E Tests failure (E2E Interactive - OpenTUI renderer (bun)), the description links the exact failing job, and the mechanism is concrete: the editor mutates synchronously while completion state is only recomputed from a useEffect, so it trails the buffer by one React frame. I checked that against the code — refreshCompletion has no synchronous call site anywhere in input-prompt.tsx, only the two effects. The detail that the same job's bare exit case passes is consistent with the diagnosis, since that input never enters slash completion.

Direction: aligned. This is the OpenTUI counterpart of the Ink-side race already fixed in #10929, and it targets a lane that is red on main. Fixing the input path is the right call — adding a wait or a retry to the E2E would have made the job green while leaving a race real users can hit.

Size: the core-module gate does not apply. Both files sit under packages/cli/src/ui/opentui/, which is not a protected core path, and the change stays inside a single package. For the record: 25 production lines (21 +/4 −) in input-prompt.tsx, 52 test lines, no generated or schema files.

Approach: the scope feels right, and it is close to the minimal version of this fix. Instead of adding a new mechanism it re-derives isPerfectMatch inside the Enter handler using the same expression refreshCompletion was already computing (isPerfectSlashMatch(parseSlashCommandQuery(target.query, slashCommandPool(target, commandsRef.current)))), only fed from the live buffer — then deletes the now-dead perfect field rather than leaving it behind. I grepped for other readers of that field: there are none, so the removal is safe. The obvious alternative I considered first, calling refreshCompletion() synchronously before deciding, is worse — it would push state updates into a key handler. One small thing worth thinking about, not a blocker: this is now the third copy of the seven-argument detectCompletionTarget(...) call in this file, the other two being in refreshCompletion and acceptSuggestion. A tiny local helper would stop the three from drifting apart later.

Risk: no elevated risk signals — neither file matches the revert-correlated path list. One thing to know up front, which I pick up again in the review: the E2E Tests workflow triggers only on push: main, schedule, and workflow_dispatch, so the lane that demonstrated this bug does not run on PR CI at all. It re-verifies post-merge.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ —— 各小节都填写了,中文正文也是完整翻译而非占位。

问题: 是已观测到的 bug,不是理论性加固。#10976 由 main 分支 E2E Tests 失败(E2E Interactive - OpenTUI renderer (bun))自动创建,PR 描述里给出了具体失败 job 链接,成因也很明确:编辑器是同步变更的,而补全状态只在 useEffect 里重算,因此会比缓冲区落后一个 React frame。我对照代码确认过——input-prompt.tsxrefreshCompletion 没有任何同步调用点,只有那两个 effect。同一 job 里裸 exit 能通过也与该判断一致,因为那个输入不会进入 slash 补全。

方向: 对齐。这是 #10929 已在 Ink 侧修复的补全竞态在 OpenTUI 的对应问题,且针对的是 main 上已经变红的 lane。修输入路径是正确的选择——如果改成在 E2E 里加等待或重试,job 会绿,但真实用户仍会碰到这个竞态。

规模: 核心模块门禁不适用。两个文件都在 packages/cli/src/ui/opentui/ 下,不属于受保护的核心路径,改动也只在单个 package 内。记录一下:input-prompt.tsx 生产代码 25 行(+21 / −4),测试 52 行,无生成文件或 schema 文件。

方案: 范围合理,基本就是这个问题最小的修法。它没有引入新机制,而是在 Enter 处理里用 refreshCompletion 原本就在算的同一个表达式isPerfectSlashMatch(parseSlashCommandQuery(target.query, slashCommandPool(target, commandsRef.current))))重新推导 isPerfectMatch,只是数据源换成实时缓冲区——随后删掉了失去用途的 perfect 字段。我 grep 过该字段的其他读取点:没有,所以删除是安全的。我最初想到的另一个方案(在判断前同步调用 refreshCompletion())更差,那会把 state 更新塞进按键处理里。有一点可以想想,但不是阻塞项:这已经是该文件中第三处七参数的 detectCompletionTarget(...) 调用,另外两处在 refreshCompletionacceptSuggestion。抽一个很小的本地 helper 可以防止三者以后各自漂移。

风险: 无升级风险信号——两个文件都不匹配与 revert 相关的路径清单。有一点先说明,我在 review 里还会再提:E2E Tests workflow 只在 push: mainscheduleworkflow_dispatch 触发,因此暴露这个 bug 的那条 lane 在 PR CI 上根本不会跑,只能合并后再验证。

进入代码审查 🔍

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

Reviewed at 77b10bd244828023a65d89383ebe7d50685e8490 · re-run with @qwen-code /triage

@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.

Reviewed at head 77b10bd2.

  • The OpenTUI counterpart of #10929, and done the tighter way: instead of re-implementing the exact-match predicate, Enter re-runs the render path's own detectCompletionTarget + parseSlashCommandQuery + isPerfectSlashMatch against the live editor (cursor-mapped, multi-line aware), so the live decision is equivalent-by-construction to what the next render would compute — no grammar copy to drift. slashStateRef sheds only its perfect field; suggestion acceptance still uses the effect-published range, and the mode check moved from the render's completionModeRef to the live target?.mode (undefined target → not perfect → accepts, same as before for AT mode and non-commands). No dangling perfect references at this head; cost is one synchronous parse per Enter.
  • Both race directions are pinned by new tests in the same frame the failure needs: final char + Enter in one React frame must submit /quit, and a live partial under stale perfect-match must still autocomplete — exactly the Ink-side pair from #10929 adapted to OpenTUI.
  • No review history or threads to reconcile; nothing in my own pass found worth blocking.
  • CI facts: 11 checks pass, zero assertion failures; Test/Lint/triage are still in flight (the authoritative run of this very spec, since local re-execution was not possible in my environment — the @opentui/react snapshot here predates the packages). The lone red is Dependency CVE audit, whose job died on npm error audit endpoint returned an error (registry 503) with production showing found 0 vulnerabilities — the same endpoint flake that reddened the identical check on #10979 this morning, and this PR touches no manifest or lockfile. Per the channel convention the call is on the review itself; a re-run clears that leg.

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

Read statically against 77b10bd — I did not build, run, or check out any of this PR's code.

The core of the change is right. isPerfectMatch used to be assembled from completionModeRef and slashStateRef.current.perfect, both of which are only ever written by refreshCompletion, and refreshCompletion has no synchronous call site — it runs from two useEffects. So the value describing the buffer was always at least one React frame behind the buffer. Re-deriving it from el.plainText / el.logicalCursor inside the Enter handler removes that lag, and it does so with the identical expression the effect was already using (isPerfectSlashMatch(parseSlashCommandQuery(target.query, slashCommandPool(target, commandsRef.current)))), so the semantics of "perfect match" are unchanged — only the freshness of the inputs is. That is the smallest correct version of this fix. Dropping the now-unused perfect field is clean too; I grepped for other readers and there are none, and completionModeRef itself is still live in four other places, so nothing was orphaned.

Both new tests are load-bearing rather than decorative — I checked each one against the pre-change logic:

  • submits an exact command from the live editor when completion is stale: await typeText('/qui') flushes effects, so the old code entered Enter with perfect: false and /quit sitting in suggestions; firing t and return inside a single act would take the accept branch and leave submitted empty, failing the assertion. It only passes with the live re-parse.
  • does not submit a partial command from stale perfect-match state: editor.setText('/qui') is deliberately not wrapped in act, so the refs still hold perfect: true from the preceding /quit. The old code would have submitted /qui outright; the new code accepts the completion into /quit . This pins the opposite direction of the race, which is the part a one-sided fix usually forgets.

They also reuse the existing harness helpers (baseKeyEvent, lastKeyboardHandler, currentEditor, typeText, renderWithCommands) instead of adding parallel ones, and follow the surrounding G-13 conventions including the action: () => undefined that isPerfectSlashMatch requires.

One divergence worth a look (non-blocking)

The fix makes isPerfectMatch live, but the guard it feeds is still half-stale: showing comes from suggestions, which is React state and therefore still a frame behind. Mixing a live signal into a stale gate opens a narrow window that the old code happened to get right.

History recall is the reachable path. The navigationUp / navigationDown handlers replace the buffer and set historyRestoredTextRef, but they do not synchronously clear suggestions or reset completionModeRef. So with the dropdown open on an already-exact command (/quit typed in full), recall a non-slash entry such as npm run build and press Enter inside the same frame:

  • the live detectCompletionTarget returns null — the cursor is set to (0, 0), there is no @ or mid-input / token, and the line-led branch needs cursorRow === 0 on a slash command;
  • so isPerfectMatch is false while stale showing is still true, and Enter takes the accept branch;
  • key.preventDefault() runs, then acceptSuggestion recomputes the target from the live buffer and early-returns on if (!target) return;.

Net effect: the keystroke is swallowed. Before this PR the stale perfect: true made isPerfectMatch true, so it fell through to decideSubmit and submitted the recalled entry.

I want to be clear about the weight here: this needs an exact-match dropdown, a history recall, and both keys in one React frame, and the symptom is a no-op Enter that a second press recovers from — not corruption. It is also the mirror image of a case this PR genuinely improves (backspacing /quit to /qui and pressing Enter now accepts the completion instead of submitting a half-typed command off stale perfect: true, which is exactly what test 2 pins). So I am raising it as something to consider, not as a reason to hold the PR.

If you do want to close it, the shape is roughly one word — require a live target before taking the accept branch, since a null live target means the open dropdown is stale by definition and Enter should fall through to decideSubmit:

if (showing && target && (!isPerfectMatch || suggestionNavigatedRef.current)) {

Not verified: I could not execute this to confirm the window is reachable in the real OpenTUI runtime — running PR code is out of bounds on this path, and the unit harness drives a fake editor rather than real frame timing. The /tmux and /verify lanes below would settle it.

Nit

The comment above slashStateRef still describes the field this PR removed — it reads "the query-relative replacement range and whether the input already names a runnable command exactly". The second clause was perfect. Worth trimming while the context is fresh, so the ref's contract matches its type.

Test evidence

This is an unattended CI run, so the evidence below is the PR's own CI read through the API — real check names and conclusions for 77b10bd244828023a65d89383ebe7d50685e8490, fetched once. Nothing was re-run locally, and nothing user-visible was driven in tmux (that path is local-invocation only). No check was red at fetch time: 12 success, 13 skipped, 6 still in progress out of 31.

Check Conclusion
TUI parity snapshots (ink vs opentui) success
OpenTUI no-flicker gate success
Desktop Shell (ubuntu-22.04) success
Desktop Shell (windows-2022) success
Secret scan (TruffleHog) success
Test (ubuntu-latest, Node 22.x) in_progress
Lint & Static (ubuntu-latest, Node 22.x) in_progress
Integration Tests (no-AK, No Sandbox) in_progress
Dependency CVE audit in_progress
Test (macos-latest / windows-latest, Node 22.x) skipped
verify, tmux-testing skipped (maintainer-triggered)

The two lanes that matter most for this diff were still running when I fetched, so I am reporting them as pending rather than guessing an outcome — the finalize job rewrites the table above once CI settles. Test (ubuntu-latest, Node 22.x) is what actually executes the two new input-prompt.test.tsx cases, and Lint & Static carries typecheck, which is the lane that would catch a narrowing problem in the new target?.mode === CompletionMode.SLASH && ... expression (statically it narrows fine, but that is typecheck's call, not mine). The 13 skips are bot-orchestration and gated lanes, not suppressed test jobs.

For attribution: the description reports focused ESLint and Prettier passing locally, and says the focused Vitest file could not be collected in the author's isolated worktree because its dependency snapshot predates the OpenTUI packages. That is the author's account of their own environment, not evidence I can vouch for — which is precisely why the pending Test lane above is the signal that counts here.

Sandboxed verification would settle what static review cannot. @qwen-code /tmux — that typing /quit and pressing Enter immediately after the final t, against a response held open after its first streamed chunk, actually interrupts and exits 0 instead of leaving the input and completion menu on screen; the unit tests pin the composer-level decision but run against a fake editor, and the lane that demonstrated this bug never runs on PR CI (see below). @qwen-code /verify — that the new live-parse branch is load-bearing rather than incidentally green, i.e. the two added tests fail with the production change reverted, plus an A/B read on the history-recall divergence above. The author has write access, so both lanes are available directly rather than as a sponsored run.

The gap worth naming explicitly: E2E Tests — the workflow containing E2E Interactive - OpenTUI renderer (bun), the job that failed on main and produced #10976 — triggers only on push: main, schedule, and workflow_dispatch. It is deliberately not in the merge queue. So a fully green PR CI here cannot demonstrate the original failure is gone; that only re-verifies post-merge.

中文说明

代码审查

以下是针对 77b10bd 的静态审查——我没有构建、运行或 checkout 本 PR 的任何代码。

改动的核心是对的。isPerfectMatch 原先由 completionModeRefslashStateRef.current.perfect 组合而来,而这两者只由 refreshCompletion 写入,refreshCompletion 又没有任何同步调用点——它只从两个 useEffect 里跑。所以描述缓冲区的那个值,永远比缓冲区本身落后至少一个 React frame。在 Enter 处理里改用 el.plainText / el.logicalCursor 重新推导,消除了这个延迟;而且用的是 effect 原本完全相同的表达式(isPerfectSlashMatch(parseSlashCommandQuery(target.query, slashCommandPool(target, commandsRef.current)))),因此"完整匹配"的语义没变,变的只是输入的新鲜度。这是该问题最小的正确修法。顺手删掉不再使用的 perfect 字段也很干净:我 grep 过没有其他读取点,而 completionModeRef 自身在另外四处仍然在用,没有留下孤儿。

两个新测试都是真正有承重作用的,我逐个对照改动前的逻辑验证过:

  • completion 状态过期时从实时编辑器提交完整命令await typeText('/qui') 会 flush effect,所以旧代码进入 Enter 时 perfect: falsesuggestions 里已有 /quit;在同一个 act 里连发 treturn 会走 accept 分支、submitted 保持为空,断言失败。只有实时重解析才能通过。
  • 不因过期的完整匹配状态提交部分命令editor.setText('/qui') 故意没有包在 act 里,因此 ref 仍保留前一次 /quitperfect: true。旧代码会直接把 /qui 提交出去;新代码则接受补全得到 /quit 。这钉住了竞态的另一个方向,正是单向修复通常会漏掉的部分。

它们也复用了既有 harness helper(baseKeyEventlastKeyboardHandlercurrentEditortypeTextrenderWithCommands),没有另起一套,并遵循周围 G-13 的写法,包括 isPerfectSlashMatch 所需的 action: () => undefined

一处值得看看的行为分歧(非阻塞)

这个修复让 isPerfectMatch 变成实时的,但它所参与的那个判断条件仍然半新半旧:showing 来自 suggestions,那是 React state,因此仍然落后一帧。把实时信号混进过期门禁,会打开一个旧代码恰好做对的窄窗口。

可达路径是历史回溯。navigationUp / navigationDown 会替换缓冲区并设置 historyRestoredTextRef,但不会同步清空 suggestions,也不会重置 completionModeRef。于是当下拉框停在一个已经完整的命令上(完整输入了 /quit),回溯到一条非 slash 的历史(例如 npm run build),并在同一个 frame 内按 Enter:

  • 实时 detectCompletionTarget 返回 null——光标被设到 (0, 0),没有 @ 或行中 / token,而 line-led 分支要求 cursorRow === 0 且是 slash 命令;
  • 因此 isPerfectMatchfalse,而过期的 showing 仍为 true,Enter 走进 accept 分支;
  • 先执行 key.preventDefault(),随后 acceptSuggestion 用实时缓冲区重算 target,在 if (!target) return; 处提前返回。

最终结果:这次按键被吞掉。改动前,过期的 perfect: true 会让 isPerfectMatch 为真,从而落到 decideSubmit 并把回溯出来的内容提交。

我想把权重说清楚:这需要"完整匹配的下拉框 + 历史回溯 + 两个按键落在同一个 React frame",症状是一次空操作的 Enter,再按一次即可恢复——不是数据损坏。而且它是本 PR 确实改好的那个场景的镜像(从 /quit 退格到 /qui 再按 Enter,现在会接受补全,而不是凭过期的 perfect: true 提交半个命令,这正是测试 2 钉住的行为)。所以我把它作为一个可以考虑的点提出,而不是压住 PR 的理由。

如果确实想收掉它,大致只需一个词——在走 accept 分支前要求存在实时 target,因为实时 target 为 null 就意味着当前下拉框按定义已过期,Enter 应当落到 decideSubmit

if (showing && target && (!isPerfectMatch || suggestionNavigatedRef.current)) {

未验证:我无法执行代码来确认这个窗口在真实 OpenTUI 运行时中确实可达——在此路径上运行 PR 代码是被禁止的,而单元测试 harness 驱动的是 fake editor,不是真实帧时序。下面的 /tmux/verify 可以判定。

小问题

slashStateRef 上方的注释仍在描述本 PR 删掉的字段——它写着"query 相对的替换范围以及输入是否已经精确指名一个可运行命令"。后半句说的就是 perfect。趁上下文还热的时候顺手修掉,让该 ref 的契约与它的类型一致。

测试证据

这是无人值守的 CI 运行,因此下面的证据是通过 API 读到的本 PR 自己的 CI——针对 77b10bd244828023a65d89383ebe7d50685e8490 的真实 check 名称与结论,一次性获取。没有在本地重跑任何东西,也没有在 tmux 里驱动任何用户可见行为(那条路径仅限本地调用)。获取时没有红色 check:31 项中 12 项 success、13 项 skipped、6 项仍在进行。

对本 diff 最重要的两条 lane 在我获取时仍在运行,所以我如实标为 pending,而不是猜测结果——CI 落定后 finalize job 会重写上面的表格。Test (ubuntu-latest, Node 22.x) 才是真正执行两个新 input-prompt.test.tsx 用例的 lane,而 Lint & Static 承载 typecheck,也就是能捕捉新表达式 target?.mode === CompletionMode.SLASH && ... 收窄问题的 lane(静态看收窄是成立的,但那该由 typecheck 判定,不该由我判定)。13 项 skip 是 bot 编排与被门禁的 lane,不是被抑制的测试 job。

关于归属:PR 描述称本地聚焦的 ESLint 与 Prettier 通过,并说聚焦的 Vitest 文件在作者的隔离 worktree 中无法收集,因为其依赖快照早于 OpenTUI 包。这是作者对自己环境的陈述,不是我能背书的证据——这也正是上面那条 pending 的 Test lane 才是关键信号的原因。

沙箱化验证可以判定静态审查判定不了的部分。@qwen-code /tmux——在模型响应输出第一个流式片段后保持流不结束的情况下,输入 /quit 并在最后一个 t 之后立即按 Enter,是否真的中断并以退出码 0 结束,而不是把输入和补全菜单留在屏幕上;单元测试钉住的是 composer 层的决策,跑的是 fake editor,而暴露这个 bug 的那条 lane 在 PR CI 上根本不会运行(见下)。@qwen-code /verify——新的实时解析分支是否真正承重,而非碰巧变绿,即回滚生产改动后那两个新增测试是否失败,同时对上面那处历史回溯分歧做 A/B 判读。作者有 write 权限,因此两条 lane 都可以直接触发,而非 sponsored run。

需要点明的缺口:E2E Tests——即包含 E2E Interactive - OpenTUI renderer (bun)(在 main 上失败并产生 #10976 的那个 job)的 workflow——只在 push: mainscheduleworkflow_dispatch 触发,是刻意不放进 merge queue 的。所以这里即便 PR CI 全绿,也无法证明原始失败已消失;只能合并后再验证。

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

Reviewed at 77b10bd244828023a65d89383ebe7d50685e8490 · re-run with @qwen-code /triage

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — correct diagnosis, minimal fix, and two regression tests that actually fail without it; the nits below are non-blocking.

Stepping back: before I read the diff I wrote down what I would do from the title and the "why" alone — re-derive the submit-vs-complete decision from the live editor buffer inside the Enter handler, using the same parse the completion effect already uses, and delete the cached flag rather than trying to keep it fresh. That is what this PR does. The alternatives I came up with were all worse: calling the refresh synchronously from the key handler pushes state updates into an event and still leaves suggestions a frame behind; storing the buffer text alongside the flag and invalidating on mismatch keeps state that a cheap synchronous recompute makes unnecessary; and loosening the E2E to send the final character and Enter in separate frames would have turned the job green while leaving a race that real fast typists hit. So the approach here matches my independent proposal and then goes one better, by covering the opposite direction of the race in test 2 — the half a one-sided fix usually forgets.

The part that convinced me this is a fix and not a plausible-looking edit is that the mechanism is checkable in the code without running anything. refreshCompletion genuinely has no synchronous call site, so the flag describing the buffer was structurally guaranteed to lag it. That is not a timing coincidence to be papered over; it is a stale-cache bug, and removing the cache is the honest repair. Six months from now nobody has to wonder whether perfect is current, because perfect no longer exists.

What holds it at 4 rather than 5 is that the fix is only half-applied to its own guard: isPerfectMatch is now live but showing is still stale React state, and mixing the two opens the narrow history-recall window I traced in the review comment — exact-match dropdown, recall a non-slash entry, Enter in the same frame, keystroke swallowed where the old code submitted. I am deliberately not treating that as a blocker: it needs three things to line up inside one frame, a second Enter recovers from it, and it is the mirror image of a case this PR genuinely fixes. But it is a real divergence rather than a style preference, and the one-word hardening is right there if you want it. The stale slashStateRef comment describing the removed field is the other loose end — small, but it is the kind of thing that misleads the next reader precisely when they are trying to understand this race.

On the evidence: I have to be straight about what is and is not verified. No check was red when I fetched, and the two OpenTUI-adjacent lanes that did finish (TUI parity snapshots, OpenTUI no-flicker gate) are green — but the unit lane that runs the new tests and the typecheck lane were still in flight, so I am not claiming they passed. More importantly, E2E Tests does not trigger on pull_request at all, so the job that failed on main and produced #10976 will never run against this PR; green PR CI here cannot demonstrate the original failure is gone. That is a property of the workflow, not a shortcoming of this PR, but it means the central behavioural claim rests on unit tests over a fake editor plus the author's account of their local runs. @qwen-code /tmux or @qwen-code /verify would close that, and I would read the merge either way as safe given the fix's shape — I just do not want the gap left unsaid.

Approving on the merits. CI is still running on this commit, so approval is deferred until it lands green on 77b10bd244828023a65d89383ebe7d50685e8490 rather than posted against a result that does not exist yet; if anything goes red or the head moves, the deferred approval is withheld and the status comment will say so.

中文说明

Confidence: 4/5 —— 诊断准确、修法最小,且两个回归测试在没有该改动时确实会失败;下面几点均为非阻塞。

退一步看:在读 diff 之前,我只凭标题和"为什么需要"写下了自己会怎么做——在 Enter 处理里用补全 effect 本来就在用的同一套解析、从实时编辑器缓冲区重新推导"提交还是补全"的决策,并删掉缓存标志而不是设法让它保持新鲜。这正是本 PR 所做的。我想到的其他方案都更差:在按键处理里同步调用刷新,会把 state 更新塞进事件里,而且 suggestions 仍然落后一帧;把缓冲区文本与标志一起存下来、不一致时失效,则保留了本可用一次廉价的同步重算取代的状态;而放宽 E2E、让最后一个字符与 Enter 分处不同 frame,只会让 job 变绿,却留下真实快速输入用户会撞上的竞态。所以这里的方案与我的独立提议一致,并且更进一步——测试 2 覆盖了竞态的另一个方向,而那正是单向修复通常漏掉的一半。

真正让我确信这是修复而非"看起来合理"的改动,是它的成因无需运行任何代码就能在源码里核实。refreshCompletion 确实没有任何同步调用点,因此那个描述缓冲区的标志在结构上就注定落后于缓冲区。这不是可以糊过去的时序巧合,而是一个过期缓存 bug,而删掉缓存才是诚实的修法。六个月后没有人需要再纠结 perfect 是否是最新的,因为 perfect 已经不存在了。

之所以停在 4 分而不是 5 分,是因为这个修复对它自己的判断条件只做了一半:isPerfectMatch 现在是实时的,但 showing 仍是过期的 React state,两者混用打开了我在审查评论里推演过的那个窄窗口——历史回溯:下拉框停在完整匹配上,回溯到一条非 slash 的历史,同一个 frame 内按 Enter,按键被吞掉,而旧代码会提交。我刻意不把它当作阻塞项:它需要三件事在一个 frame 内对齐,再按一次 Enter 即可恢复,而且它是本 PR 确实修好的那个场景的镜像。但它是一个真实的行为分歧,而非风格偏好;如果你想收掉它,那个一个词的加固就在那里。另一个收尾问题是 slashStateRef 上方仍在描述已删除字段的注释——很小,但恰恰会在下一个读者试图理解这个竞态时误导他。

关于证据:我必须把已验证与未验证分清楚。我获取时没有红色 check,已完成的两条与 OpenTUI 相关的 lane(TUI parity snapshotsOpenTUI no-flicker gate)是绿的——但运行新测试的单元 lane 与 typecheck lane 当时仍在进行,所以我不会声称它们通过了。更重要的是,E2E Tests 根本不在 pull_request 上触发,因此在 main 上失败并产生 #10976 的那个 job 永远不会针对本 PR 运行;这里 PR CI 全绿也无法证明原始失败已消失。这是该 workflow 的属性,不是本 PR 的缺陷,但意味着核心的行为主张目前依靠的是基于 fake editor 的单元测试,加上作者对其本地运行的陈述。@qwen-code /tmux@qwen-code /verify 可以补上这一点;考虑到这个修复的形态,无论哪种方式我都会认为合并是安全的——我只是不想让这个缺口没人说出来。

按实质表现批准。CI 仍在该 commit 上运行,因此批准将推迟到它在 77b10bd244828023a65d89383ebe7d50685e8490 上变绿之后,而不是针对一个尚不存在的结果先行给出;若出现红色或 head 发生移动,该推迟的批准将被撤回,状态评论中会说明。

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

Reviewed at 77b10bd244828023a65d89383ebe7d50685e8490 · re-run with @qwen-code /triage

@yiliang114
yiliang114 added this pull request to the merge queue Sep 4, 2026
Merged via the queue into main with commit 7a3b6cb Sep 4, 2026
67 of 70 checks passed
@chiga0

chiga0 commented Sep 4, 2026

Copy link
Copy Markdown
Collaborator

Post-merge review (merged while this review was running; findings are for follow-up, not a merge gate)

No blocking findings. Tier: Standard.


Findings

Minor — stale slashStateRef comment
packages/cli/src/ui/opentui/input-prompt.tsx:268–270

The three-line comment immediately above slashStateRef still reads "… and whether the input already names a runnable command exactly (Enter then submits instead of accepting a suggestion)." — but the perfect field that stored that flag was removed by this PR. Only range remains in the ref. One option is to trim the comment to "SLASH-completion state for the current buffer: the query-relative replacement range."


What I checked

  • slashStateRef read sites at head: only range is read (line 507), and the two null-reset sites. No remaining .perfect consumer — removal is clean.
  • detectCompletionTarget + parseSlashCommandQuery + isPerfectSlashMatch are pure synchronous functions; calling them in the keyboard handler on every Enter is negligible cost.
  • matchesCommandName does strict case-insensitive equality — a partial token like qui never matches quit, so the live re-parse correctly returns isPerfectMatch = false for any partial query.
  • Both new tests are load-bearing (verified against pre-change control flow — test 1 would exit the if branch and accept the stale suggestion rather than submitting; test 2 would submit /qui outright).
  • applyCompletion tolerates a stale slashRange.end past the current line length — lineCodePoints.slice(end) returns [], so the replacement is still correct.

Cross-check with ci-bot stage-3:

  • Stale slashStateRef comment → confirmed.
  • Mixed freshness (showing stale, isPerfectMatch live) opening a narrow history-recall window → confirmed as a real theoretical case (exact-match dropdown + history recall + Enter in one frame → keystroke swallowed instead of submitted). Non-blocking: requires three things in one frame, a second Enter recovers. No new action needed here beyond what was already noted.

Not covered: no local build / unit-test run (OpenTUI deps absent in this environment); E2E lane does not trigger on pull_request by design.

Reviewed with AI assistance.

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.

Main CI failed: E2E Tests on 60161cb64a2b

4 participants