Skip to content

fix(mesh): keep the existence check when filtering host sessions out of search - #11261

Closed
yiliang114 wants to merge 1 commit into
codex/multi-agent-mesh-foundationfrom
codex/mesh-fix-session-list-guard
Closed

fix(mesh): keep the existence check when filtering host sessions out of search#11261
yiliang114 wants to merge 1 commit into
codex/multi-agent-mesh-foundationfrom
codex/mesh-fix-session-list-guard

Conversation

@yiliang114

Copy link
Copy Markdown
Collaborator

What this PR does

One line: restores the existence check that #11225 folded into an optional chain in session-list.ts.

Why it's needed

It is why every build on the mesh branch is red. tsc --build fails on packages/cli:

src/serve/server/session-list.ts(1593,23): error TS2345:
Argument of type 'SessionListItem | undefined' is not assignable to parameter of type '{ sessionId: string; ... }'

Test, Lint & Static, and Integration Tests all fail at "Install dependencies", which runs the build, so nothing downstream of it has run since #11225 merged.

The change was if (item)if (item?.sourceType !== MESH_HOST_SESSION_SOURCE_TYPE). When the read finds nothing, item?.sourceType is undefined, and undefined !== 'mesh' is true — so the branch runs with no item at all. This would have been a regression even if it had compiled: a session that disappeared between the search hit and this read used to be skipped, and would now be added to the map as an undefined summary. The guard that was replaced existed for exactly that case.

Fix: both conditions, in this order.

Reviewer Test Plan

How to verify

npm run build --workspace=packages/cli

Expect it to pass. On the branch without this commit it fails at the line above.

Evidence (Before & After)

Before: tsc --build fails, so Test / Lint / Integration never start. After: the build proceeds.

Tested on

OS Status
🍏 macOS N/A
🪟 Windows N/A
🐧 Linux

Environment (optional)

Checked with tsc --noEmit on the single file; the full workspace build is CI's to confirm.

Risk & Scope

  • Main risk or tradeoff: none. It restores behaviour that existed before feat(mesh): add hidden host session launcher #11225 and adds the mesh filter beside it rather than instead of it.
  • Not validated / out of scope: no test covers a session vanishing between the search hit and the read. Worth adding, but this PR is deliberately the smallest thing that unblocks CI.
  • Breaking changes / migration notes: none.

Linked Issues

Parent delivery PR: #11206. Regression from #11225.

…of search

#11225 replaced 'if (item)' with 'if (item?.sourceType !== MESH_HOST...)'.
The optional chain yields undefined when the read found nothing, and
undefined !== 'mesh', so a session that vanished between the search hit and
this read now enters the branch and is added as an undefined summary. It
fails tsc, which is why every build on the branch is red, and it would have
been a real regression even if it had compiled: the guard it replaced existed
to drop exactly that case.

Both conditions, in this order.
@qwen-code-ci-bot

qwen-code-ci-bot commented Sep 7, 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: all required headings present ✓ (the 中文说明 details block is absent — not gating).

Problem: observed, and I confirmed it independently rather than taking the description's word for it. The base branch's own CI at 19e9138 fails inside Install dependencies with exactly the quoted error:

src/serve/server/session-list.ts(1593,23): error TS2345: Argument of type 'SessionListItem | undefined' is not assignable to parameter of type '{ sessionId: string; … }'

That is the only error TS in the entire job log, which is why Test, Lint & Static, Integration Tests and Serve A/B are all red or skipped on codex/multi-agent-mesh-foundation — the build dies in prepare, so nothing downstream ever starts. The types back the claim up: getSessionListItem returns Promise<SessionListItem | undefined> (ENOENT, an empty transcript, or a session belonging to another project each return undefined), while toSummary takes a non-nullable object. #11225 folded the existence check into item?.sourceType !== …, and that expression is true when item is undefined.

Direction: aligned. This isn't a style preference — the branch does not compile without it, and the parent delivery PR #11206 sits behind it. CHANGELOG: no direct reference, which is expected for an unreleased feature branch.

Size: not applicable. packages/cli/src/serve/server/session-list.ts is not a core-infrastructure path, and the diff is 5 additions / 1 deletion in one file — all production, no test or generated lines.

Approach: right scope. Both conditions, existence first, nothing else touched. I did consider whether the mesh-host filter belongs further down — inside the content search, so host-session hits never come back at all. But searchSessionContent only accepts { maxFiles, maxResults, signal }, and excludeSourceType exists solely on ListSessionsOptions, so pushing the filter down means a new core option plus a metadata read inside the walk. That is a much bigger change than the one that unblocks CI: good follow-up, wrong PR for it. The four-line comment is earned here — the optional-chain trap is exactly the sort of thing that gets "simplified" back later.

Risk: no elevated risk signals (no match against the revert-correlated path list).

Moving on to code review. 🔍

中文说明

感谢贡献!

**模板:**必需标题齐全 ✓(缺 中文说明 折叠块——不作为拦截项)。

**问题:**是已观测到的 bug,而且我独立核实过,没有只采信 PR 描述。目标分支自身在 19e9138 的 CI 就是在 Install dependencies 阶段挂掉的,报错与描述完全一致:

src/serve/server/session-list.ts(1593,23): error TS2345: Argument of type 'SessionListItem | undefined' is not assignable to parameter of type '{ sessionId: string; … }'

整个 job 日志里只有这一条 error TS,这也解释了为什么 codex/multi-agent-mesh-foundation 上 Test、Lint & Static、Integration Tests、Serve A/B 全红或被 skipped——构建在 prepare 阶段就死了,下游根本没机会启动。类型层面也对得上:getSessionListItem 返回 Promise<SessionListItem | undefined>(ENOENT、transcript 为空、session 属于其它项目都会返回 undefined),而 toSummary 的入参是非空对象。#11225 把存在性检查折进了 item?.sourceType !== …,而 itemundefined 时这个表达式恰好为 true

**方向:**对齐。这不是代码风格偏好——没有这一行分支根本编译不过,父交付 PR #11206 也被它挡住。CHANGELOG:无直接对应条目,未发布的特性分支属于正常情况。

**规模:**不适用。packages/cli/src/serve/server/session-list.ts 不属于核心基础设施路径,改动为单文件 +5/-1,全部是生产代码,不含测试或生成文件。

**方案:**范围合适。两个条件、存在性在前,其它一律没动。我确实考虑过把 mesh host 过滤下推到内容检索里,让 host session 的命中根本不返回;但 searchSessionContent 只接受 { maxFiles, maxResults, signal }excludeSourceType 仅存在于 ListSessionsOptions,下推意味着要在 core 新增一个选项、并在遍历里多读一次元数据。相比"先让 CI 通"这件事,那是大得多的改动:适合做后续项,不适合塞进这个 PR。那四行注释是值得留的——optional chain 这种坑,正是日后容易被再次"简化"回去的地方。

**风险:**无升级风险信号(未命中与 revert 相关的路径清单)。

进入代码审查 🔍

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code review

The change is correct and it is the minimal one. item && item.sourceType !== … narrows SessionListItem | undefined to SessionListItem before toSummary sees it, and it keeps all three cases distinct and right: an undefined read is dropped (the pre-#11225 behaviour, restored), a defined mesh-host session is dropped (#11225's intent, preserved), and everything else is added. Nothing downstream needs adjusting — the results loop already tolerates a missing entry (const session = bySessionId.get(hit.sessionId); if (session) results.push(…)), and the two sidecar enrichers just iterate whatever made it into the map. No blockers, no convention violations, and nothing new is introduced that could have reused something existing.

One correction to the description, which doesn't change the fix: a vanished session would not have been "added to the map as an undefined summary". toSummary reads item.sessionId in its first property access, so it would have thrown a TypeError. Moot in practice — it never compiled — but it makes the guard more necessary than the description suggests, not less.

Non-blocking, and pre-existing rather than this PR's doing: nothing pins the undefined branch. There is no session-list.test.ts on the branch at all, only persisted-session-list-cache.test.ts. The useful detail is that the branch is reachable without simulating a race — getSessionListItem returns undefined for ENOENT, for an empty transcript (records.length === 0), and for a session belonging to another project. So a test can stage an empty .jsonl, search for a needle that another session contains, and assert the empty one is absent from results. Worth doing in the follow-up that adds mesh search coverage, not here.

Testing

Unattended CI run — I did not build, run, or check out anything from this PR. Everything below is quoted from CI logs read through the API.

The load-bearing part is a lane that is easy to miss. ci.yml and serve-ab.yml both filter pull_request: branches: ['main', 'release/**'], and this PR's base is codex/multi-agent-mesh-foundation, so Test, Lint & Static, Integration Tests and Serve A/B never trigger on this commit — not pending, not queued, structurally out of scope. tui-parity.yml has an unfiltered pull_request trigger, and it runs npm ci followed by npm run build before its gates. That makes it the only lane on this PR that actually compiles the tree, and it happens to give a clean before/after on the same two jobs, forty minutes apart:

Before — branch head 19e91384 (without the fix), job 101621392108, conclusion failure:

##[error]src/serve/server/session-list.ts(1593,23): error TS2345: Argument of type
  'SessionListItem | undefined' is not assignable to parameter of type '{ sessionId: string; … }'.
npm error Lifecycle script `build` failed with error:
##[error]Process completed with exit code 1.

After — this PR's head e289b76 (with the fix), job 101628396431, conclusion success: > @qwen-code/qwen-code@0.23.0 buildnode ../../scripts/build_package.js ran to completion, and Run bash scripts/tui-parity/accept-noflicker.sh executed afterwards, which only happens once the build succeeds. The case-insensitive string error appears 0 times in that job's 868-line log.

Two attribution notes so the signal isn't over- or under-read. First, the red Test / Lint / Integration / Serve A/B results on 19e91384 belong to #11206 (base main, head codex/multi-agent-mesh-foundation) — I confirmed that against the run list for that SHA — not to this PR, which has no such checks. Second, error TS2345 at session-list.ts(1593,23) is the only TypeScript error in the entire base-branch build log, so this one line really is the whole blocker and nothing else is waiting behind it.

Not verified: lint and unit tests on e289b76. There is no signal for either on this PR, and by the trigger filters above there never will be — it arrives when the change reaches #11206. The diff is one conjunct and a comment, and the line is 66 characters inside its indent, so I don't expect Prettier or ESLint to object, but that is my reading, not a check result. Real-scenario tmux testing: N/A — this is an unattended CI run, and there is no TUI surface here regardless (daemon-side session search).

Sandboxed verification would settle the one thing CI cannot: @qwen-code /verify — that the restored existence check actually drops a hit whose transcript read comes back empty is not observable from the diff, and no test on the branch exercises that branch. Worth knowing that its A/B arm has no baseline here, since the base build does not compile; the value would be in a mock-free harness pinning the empty-transcript case described above.

Checks on the reviewed commit. ci.yml and serve-ab.yml are absent because both filter pull_request to main / release/**; the remaining entries other than tui-parity's two gates are bot orchestration.

Check Status Conclusion
OpenTUI no-flicker gate completed success
TUI parity snapshots (ink vs opentui) completed success
ack-review-request completed skipped
assign completed success
authorize completed/completed success
delay-automatic-review completed success
label completed success
precheck-pr completed/completed skipped
publish-resolution completed skipped
publish-tmux completed skipped
publish-verify completed skipped
resolve-pr completed skipped
review-config completed skipped
review-pr in_progress -
tmux-testing completed skipped
triage in_progress -
verify completed skipped
中文说明

代码审查

改动正确,而且已经是最小改动。item && item.sourceType !== …toSummary 拿到值之前把 SessionListItem | undefined 收窄成了 SessionListItem,三种情况各得其所:读到 undefined 就丢弃(恢复 #11225 之前的行为)、mesh host session 丢弃(保留 #11225 的意图)、其余照常加入。下游无需跟着改——结果循环本来就能容忍缺项(const session = bySessionId.get(hit.sessionId); if (session) results.push(…)),两个 sidecar enricher 也只是遍历进 map 的内容。没有阻塞项,没有违反项目约定,也没有新增可以复用既有实现的东西。

对描述做一处更正(不影响这个修法):session 消失时并不会"以 undefined summary 的形式进 map"。toSummary 第一次属性访问就是 item.sessionId,所以它会直接抛 TypeError。实际上这一点无从发生(根本编译不过),但这说明这个守卫比描述里讲的更必要,而不是更不必要。

非阻塞、且属于既有问题而非本 PR 引入:undefined 这条分支没有任何测试钉住。分支上根本没有 session-list.test.ts,只有 persisted-session-list-cache.test.ts。有用的信息是:这条分支不需要模拟竞态就能触发——getSessionListItem 在 ENOENT、transcript 为空(records.length === 0)、以及 session 属于其它项目这三种情况下都会返回 undefined。所以测试可以直接放一个空的 .jsonl,用另一个 session 里存在的关键词去搜,然后断言空的那个不出现在 results 里。这件事适合放进后续补 mesh 搜索覆盖的 PR,不适合塞在这里。

测试

本次是无人值守的 CI 运行——我没有构建、运行或 checkout 这个 PR 的任何东西。以下全部是通过 API 读到的 CI 日志原文。

关键在一个容易被忽略的 lane。ci.ymlserve-ab.yml 都带 pull_request: branches: ['main', 'release/**'] 过滤,而本 PR 的 base 是 codex/multi-agent-mesh-foundation,所以 Test、Lint & Static、Integration Tests、Serve A/B 在这个 commit 上根本不会触发——不是 pending,也不是排队中,而是从触发条件上就不在范围内。tui-parity.ymlpull_request 触发没有分支过滤,且它在跑自己的 gate 之前会先执行 npm cinpm run build。这让它成为本 PR 上唯一真正编译这棵树的 lane,并且刚好在同样的两个 job 上、相隔约 40 分钟,给出了一组干净的 before/after:

Before——分支头 19e91384(无此修复),job 101621392108,结论 failure

##[error]src/serve/server/session-list.ts(1593,23): error TS2345: Argument of type
  'SessionListItem | undefined' is not assignable to parameter of type '{ sessionId: string; … }'.
npm error Lifecycle script `build` failed with error:
##[error]Process completed with exit code 1.

After——本 PR 头 e289b76(含此修复),job 101628396431,结论 success> @qwen-code/qwen-code@0.23.0 buildnode ../../scripts/build_package.js 完整跑完,随后执行了 Run bash scripts/tui-parity/accept-noflicker.sh——而这一步只有在构建成功之后才会跑到。该 job 共 868 行日志,不区分大小写地搜 error,命中 0 次

两处归属说明,避免把信号读过头或读不够。第一,19e91384 上 Test / Lint / Integration / Serve A/B 的红属于 #11206(base main,head codex/multi-agent-mesh-foundation)——我按该 SHA 的 run 列表核对过——不属于本 PR,本 PR 没有这些检查。第二,整份基线构建日志里 error TS2345session-list.ts(1593,23))是唯一一条 TypeScript 错误,所以这一行确实就是全部阻塞点,后面没有别的在排队。

未验证:e289b76 上的 lint 与单元测试。本 PR 没有这两项信号,而且按上面的触发过滤永远不会有——它们要等改动进入 #11206 才会出现。这次 diff 是一个条件合取加一段注释,该行含缩进共 66 字符,所以我预计 Prettier 和 ESLint 不会有意见,但这是我的判读,不是检查结果。真实场景 tmux 测试:N/A——本次是无人值守 CI 运行,而且这里本来也没有 TUI 界面(属于 daemon 侧的 session 搜索)。

沙箱验证可以补上 CI 补不了的那一环:@qwen-code /verify——"恢复后的存在性检查确实会丢弃 transcript 读取为空的命中"这一点从 diff 上看不出来,分支上也没有测试走到这条分支。需要知道的是,它的 A/B 一环在这里没有基线可比,因为 base 构建编译不过;真正的价值在于用一个不依赖 mock 的 harness 把上面那个空 transcript 场景钉住。

CI 表格:被审 commit 上的检查项。ci.ymlserve-ab.yml 缺席,是因为两者都把 pull_request 过滤到了 main / release/**;除 tui-parity 的两个 gate 之外,其余条目均为机器人编排任务。

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

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

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — the fix is proven by a real before/after on the only lane that compiles this tree; what keeps it off 5 is two named non-blocking gaps, not doubt about the change.

My independent proposal from the title and the why-section alone was the same one-liner: put the existence check back and keep the mesh filter as a second conjunct. So there was nothing to argue about on approach. The alternative I went looking for — pushing the host-session filter down into searchSessionContent so those hits never surface — turned out to be a materially bigger change, since excludeSourceType only exists on ListSessionsOptions and search would need both a new core option and a metadata read inside its walk. The scope chosen here is the right one.

What actually convinced me is that the central claim was checkable and I could check it. tui-parity runs npm run build before its gates and has no branch filter on pull_request, which makes it the one lane that compiles a PR based on a feature branch. It was red with session-list.ts(1593,23): error TS2345 on the branch head and is green on this commit, with zero occurrences of the string error across the whole job log. That is a before/after, not an inference.

Reservations, none of them blocking:

  • The description's account of the runtime consequence is off — a vanished session would have thrown a TypeError inside toSummary, not been inserted as an undefined summary. Immaterial, since it never compiled, and it argues for the guard more strongly than the version in the PR body.
  • Nothing pins the undefined branch, and there is no session-list.test.ts on the branch at all. Pre-existing gap, explicitly scoped out here, and reachable without a race if someone wants to close it later.
  • Lint and unit tests have no signal on this commit and structurally cannot, because ci.yml filters pull_request to main / release/**. I read the diff as Prettier-clean (61 columns at its indent, brace-less body matching the surrounding style) but that is my reading, not a green check.

One observation for the mesh branch rather than for this PR. #11225 merged with both tui-parity checks red — its own head c1636a14 failed with this exact error TS2345 and Lifecycle script 'build' failed. The gate was there and it was visible. Nothing stops a merge onto a feature-branch base when it is red, because ci.yml never runs there and tui-parity is not a required check for that base. This PR is the cleanup for that; the gap that let it through is still open, and the next #11225 lands the same way. Worth deciding whether stacked mesh PRs should get a build gate before #11206 goes to main.

Six months from now I'd thank whoever wrote the four-line comment rather than curse them — that is precisely the guard a later cleanup pass would otherwise fold back into an optional chain. Approving.

中文说明

Confidence: 4/5 —— 在唯一会编译这棵树的 lane 上,这个修复有真实的 before/after 佐证;没给到 5 分是因为两个已点名的非阻塞缺口,而不是对改动本身有疑虑。

只看标题和"Why it's needed"时,我独立想到的方案就是这同一个一行改动:把存在性检查放回去,把 mesh 过滤作为第二个合取条件。所以方案层面没有可争的。我确实去找过另一条路——把 host session 过滤下推进 searchSessionContent,让这类命中根本不出现——但那是明显更大的改动:excludeSourceType 只存在于 ListSessionsOptions,search 既要新增 core 选项、又要在遍历里多读一次元数据。当前选定的范围是对的。

真正说服我的是:核心论断可核验,而且我核验了。tui-parity 在跑自己的 gate 之前会执行 npm run build,且它的 pull_request 触发没有分支过滤,因此它是唯一会编译"以特性分支为 base 的 PR"的 lane。它在分支头上因 session-list.ts(1593,23): error TS2345 而红,在这个 commit 上是绿的,整份 job 日志里 error 出现 0 次。这是 before/after,不是推断。

保留意见,均不阻塞:

  • 描述里对运行时后果的说明不准确——session 消失时会在 toSummary 内部抛 TypeError,而不是以 undefined summary 的形式被塞进去。这一点无关紧要(根本编译不过),而且它比 PR 正文里的版本更能说明这个守卫的必要性。
  • 没有任何测试钉住 undefined 这条分支,分支上也完全没有 session-list.test.ts。这是既有缺口,本 PR 已明确划出范围之外;而且日后若想补,不需要模拟竞态就能触发。
  • 这个 commit 上没有 lint 与单元测试信号,而且从机制上不可能有,因为 ci.ymlpull_request 过滤到了 main / release/**。我判读这个 diff 对 Prettier 是干净的(按其缩进为 61 列,无花括号的写法与周边风格一致),但这是我的判读,不是一个绿色的检查结果。

有一点是给 mesh 分支的,不是给这个 PR 的。#11225 是在两个 tui-parity 检查都红的情况下合入的——它自己的 head c1636a14 就是因为这条一模一样的 error TS2345Lifecycle script 'build' failed 挂掉的。门禁在那儿,而且是可见的。但在以特性分支为 base 的情况下,红了也没有任何东西阻止合并:ci.yml 在那里根本不跑,而 tui-parity 对该 base 不是必需检查。这个 PR 是在收拾后果;让事故发生的那个缺口还开着,下一个 #11225 会以同样方式落地。在 #11206main 之前,值得先决定要不要给堆叠式 mesh PR 配一道构建门禁。

半年之后,我会感谢写下那四行注释的人,而不是骂他——那正是日后一次清理会顺手折回 optional chain 里的守卫。批准。

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

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

@qwen-code-ci-bot qwen-code-ci-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.

LGTM, looks ready to ship. ✅

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Qwen Code review did not complete successfully. Qwen review aborted with an API error before posting comments. A transient error is retried automatically; if you are seeing this, retry with @qwen-code /review. See workflow logs.

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Converted to draft, not closed. Folded into #11294, which now carries this commit and is the single PR against the mesh branch. Kept open for its review history; please review #11294 instead.

@yiliang114

Copy link
Copy Markdown
Collaborator Author

Closing: superseded by work already on the mesh branch.

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.

2 participants