Skip to content

feat(core): add memory recall delivery telemetry - #7393

Merged
wenshao merged 7 commits into
QwenLM:mainfrom
ZijianZhang989:codex/issue-7040-recall-delivery-telemetry
Jul 22, 2026
Merged

feat(core): add memory recall delivery telemetry#7393
wenshao merged 7 commits into
QwenLM:mainfrom
ZijianZhang989:codex/issue-7040-recall-delivery-telemetry

Conversation

@ZijianZhang989

Copy link
Copy Markdown
Collaborator

What this PR does

Adds terminal delivery telemetry for managed auto-memory recall. The existing recall selection telemetry tells us which memories were selected, but not whether those selected memories were actually delivered to the main model. This PR adds a new qwen-code.memory.recall.delivery event plus count and latency metrics, and records terminal outcomes when recall is injected into the initial prompt, injected at a ToolResult continuation point, discarded because no memory was selected, or cancelled by reset, shutdown, abort, new query, or another no-safe-delivery-point exit.

The current single auto-memory prefetch path is reported as phase: "refined" to preserve the current behavior and leave room for the planned Fast/Refined split in follow-up PRs. This PR does not change recall selection behavior or introduce Fast/Refined delivery logic.

Why it's needed

Issue #7040 needs reliable observability for auto-memory recall delivery. Today we can see that recall selected memories, but we cannot tell whether those memories reached the model prompt, arrived later at a safe continuation point, or were dropped because the turn ended first. That makes it hard to evaluate first-turn delivery, late delivery, and cancellation behavior before changing the recall pipeline.

This PR adds the missing delivery/discard signal with low-cardinality fields only. The event intentionally does not include query text, query hashes, memory content, file paths, project paths, session/message ids, raw errors, or secrets.

Reviewer Test Plan

How to verify

Run the focused telemetry and client tests. Confirm that delivery telemetry is emitted for initial prompt injection, ToolResult injection, empty recall results, reset cancellation, and new-query supersession, and that metrics only carry low-cardinality attributes.

cd packages/core && npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts
cd packages/core && mkdir -p coverage/.tmp && npx vitest run src/core/client.test.ts
npm run build && npm run typecheck

For E2E, run the CLI in a tmux session with QWEN_CODE_MEMORY_LOCAL=1 and telemetry file export enabled. Seed a local .qwen/memory/ file with a unique keyword such as ultraviolet parsnip, ask a prompt that recalls that memory, and inspect the telemetry outfile for qwen-code.memory.recall.delivery, qwen-code.memory.recall.delivery.count, and qwen-code.memory.recall.delivery.latency. Confirm each delivery event has one terminal outcome and does not include query, memory content, paths, session/message ids, raw errors, or secrets.

Evidence (Before & After)

Before: auto-memory recall telemetry reported selection outcome only, so a selected memory could not be distinguished from a delivered, late-delivered, or discarded memory.

After: tmux E2E on /tmp/pr1-memory-test produced qwen-code.memory.recall.delivery events and delivery metrics. Verified normal recall, ToolResult delivery, and abort discard. new_query and reset were timing-limited in real TUI because recall completed before cancellation, but those cancellation paths are covered by unit tests.

Tested on

OS Status
macOS tested
Windows not tested
Linux not tested

Environment (optional)

Local worktree on macOS, built bundle and ran focused Vitest tests plus tmux-based CLI E2E with telemetry outfile export.

Risk & Scope

  • Main risk or tradeoff: This adds telemetry to the recall lifecycle and cancellation paths, so the main risk is duplicate or missing terminal events. The implementation guards each prefetch with a terminal-logged flag and tests the key delivery/discard paths.
  • Not validated / out of scope: This PR does not implement Fast/Refined recall, bounded first-turn waiting, scorer fixes, multilingual recall, or the 200-cap candidate changes. Real TUI E2E could not reliably trigger new_query and reset discard reasons due to timing, but unit tests cover them.
  • Breaking changes / migration notes: None.

Linked Issues

Refs #7040

中文说明

这个 PR 做了什么

这个 PR 为 managed auto-memory recall 增加终态投递遥测。现有 recall selection telemetry 能说明选中了哪些 memory,但不能说明这些 memory 是否真的送进了主模型。这个 PR 新增 qwen-code.memory.recall.delivery 事件,以及 count 和 latency metrics,并在 recall 注入首轮 prompt、注入 ToolResult continuation、安全点缺失导致丢弃、reset、shutdown、abort、新 query 顶替等路径记录终态。

当前代码只有单一 auto-memory prefetch 路径,所以本 PR 统一记录为 phase: "refined",保持现有行为,并为后续 Fast/Refined 拆分预留字段。本 PR 不改变 recall selection 行为,也不实现 Fast/Refined 投递逻辑。

为什么需要

Issue #7040 需要可靠观察 auto-memory recall 是否真正交付。现在只能看到 recall 选中了 memory,但看不到它是否进入主模型 prompt、是否晚到后在安全 continuation 点投递,或者是否因为 turn 结束而被丢弃。缺少这个信号,就很难评估 first-turn delivery、late delivery 和 cancellation 行为。

这个 PR 补上 delivery/discard 信号,并且只记录低基数字段。事件不会包含 query 文本、query hash、memory 内容、文件路径、project path、session/message id、raw error 或 secret。

Reviewer Test Plan

如何验证

运行聚焦的 telemetry 和 client 测试。确认 initial prompt 注入、ToolResult 注入、空 recall 结果、reset 取消、新 query 顶替都会产生 delivery telemetry,并确认 metrics 只携带低基数字段。

cd packages/core && npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts
cd packages/core && mkdir -p coverage/.tmp && npx vitest run src/core/client.test.ts
npm run build && npm run typecheck

E2E 可以用 tmux 启动 CLI,设置 QWEN_CODE_MEMORY_LOCAL=1 并开启 telemetry outfile。准备一个本地 .qwen/memory/ 文件,包含 ultraviolet parsnip 这类唯一关键词,然后询问相关 memory。检查 telemetry outfile 中是否有 qwen-code.memory.recall.deliveryqwen-code.memory.recall.delivery.countqwen-code.memory.recall.delivery.latency。确认每条 delivery event 只有一个终态,并且不包含 query、memory 内容、路径、session/message id、raw error 或 secret。

证据 Before & After

Before:auto-memory recall telemetry 只记录 selection 结果,无法区分 selected memory 是已投递、晚投递,还是最终被丢弃。

After:在 /tmp/pr1-memory-test 的 tmux E2E 中观察到了 qwen-code.memory.recall.delivery 事件和 delivery metrics。已验证正常 recall、ToolResult delivery 和 abort discard。new_queryreset 在真实 TUI 中受时序限制,recall 完成太快,未稳定复现,但对应取消路径已有单测覆盖。

Tested on

OS Status
macOS tested
Windows not tested
Linux not tested

Environment

macOS 本地 worktree,构建 bundle 后运行聚焦 Vitest 测试,并用 tmux 进行 CLI E2E,telemetry 通过 outfile 落盘。

Risk & Scope

  • Main risk or tradeoff: 这个 PR 在 recall 生命周期和取消路径中增加 telemetry,主要风险是终态事件重复或遗漏。实现中通过 terminal-logged 标记保证同一个 prefetch 只记录一次终态,并用测试覆盖关键 delivery/discard 路径。
  • Not validated / out of scope: 本 PR 不实现 Fast/Refined recall、首轮 bounded wait、scorer 修复、多语言召回或 200 cap candidate 变更。真实 TUI E2E 未稳定触发 new_queryreset discard reason,因为 recall 完成太快,但单测已覆盖这些路径。
  • Breaking changes / migration notes: 无。

Linked Issues

Refs #7040

@ZijianZhang989

Copy link
Copy Markdown
Collaborator Author

E2E Test Report

Ran tmux-based CLI E2E against the rebased branch in isolated project /tmp/pr1-memory-test with QWEN_CODE_MEMORY_LOCAL=1 and telemetry file export enabled.

Overall result: PASS, with 3/5 scenarios fully verified and 2 scenarios timing-limited in real TUI.

Verified scenarios:

  • Normal recall produced one qwen-code.memory.recall.delivery terminal event with delivery_point: "tool_result".
  • ToolResult safe delivery produced one terminal delivery event and no duplicates.
  • Ctrl-C abort produced delivery_point: "discarded" with discard_reason: "abort", strategy: "none", and docs_selected: 0.

Timing-limited scenarios:

  • new_query cancellation and /clear reset were hard to reproduce in TUI because local heuristic recall completed before cancellation could win the race. These paths are covered by unit tests.

Telemetry checks:

  • qwen-code.memory.recall.delivery events were present.
  • qwen-code.memory.recall.delivery.count and qwen-code.memory.recall.delivery.latency metrics were present.
  • Each inspected scenario produced exactly one terminal delivery/discard event.
  • No forbidden high-cardinality or sensitive fields were observed: no query text/hash, memory content, memory file path, project path, session/message id, raw error, or secret.

Full local report: /tmp/pr1-memory-test/tmp/report.md.

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓

Problem: This is the first of three PRs from RFC #7040 (narrowed by the Core memory maintainer). The gap is real and observed: recall selection telemetry exists, but there's no signal for whether selected memories actually reached the model prompt, arrived late at a ToolResult continuation, or were silently dropped. That makes it impossible to evaluate delivery behavior before changing the recall pipeline.

Direction: Aligned — issue #7040 is on the roadmap/context-performance track, and the RFC explicitly lists "Add recall-delivery telemetry" as PR #1. CHANGELOG has no direct reference, but the area is actively being worked.

Size: 256 production lines (client.ts 126, loggers.ts 36, metrics.ts 51, types.ts 39, constants.ts 1, index.ts 3) vs. 837 test lines (client.test.ts 733, loggers.test.ts 78, metrics.test.ts 26). Well under the 500-line threshold — no escalation needed.

Approach: Scope feels right. Every edit serves the stated goal — new event type, logger, metrics, and wiring terminal outcomes into the existing prefetch lifecycle. The hasToolCalls tracking is a necessary behavioral addition (without it, prefetches on no-tool turns would never get a terminal event). No unrelated changes or drive-by refactors.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题: 这是 RFC #7040(由 Core memory 维护者收窄后)的三个 PR 中的第一个。差距是真实且已观察到的:recall selection telemetry 已存在,但没有信号表明选中的 memory 是否真正进入了模型 prompt、是否在 ToolResult continuation 点晚到投递、还是被静默丢弃。缺少这个信号就无法在修改 recall 管道之前评估投递行为。

方向: 对齐——issue #7040roadmap/context-performance 轨道上,RFC 明确将"添加 recall-delivery telemetry"列为 PR #1

规模: 256 行生产代码 vs. 837 行测试代码。远低于 500 行阈值,无需升级。

方案: 范围合理。每个编辑都服务于既定目标。hasToolCalls 跟踪是必要的行为补充(否则无工具调用的 turn 上的 prefetch 永远不会收到终态事件)。无无关改动。

进入代码审查 🔍

Qwen Code · qwen3.7-max

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Code Review

Independent proposal: Given the goal (add delivery/discard telemetry for auto-memory recall), I would: add a MemoryRecallDeliveryEvent type with low-cardinality fields in types.ts, a logMemoryRecallDelivery function in loggers.ts mirroring the existing logMemoryRecall pattern, count + latency metrics in metrics.ts, and wire terminal outcomes at every cancelPendingMemoryPrefetch() call site in client.ts plus the tryConsumeMemoryPrefetch() success path. A terminalLogged flag prevents duplicate terminal events.

Comparison with the diff: The PR matches this proposal almost exactly. The implementation follows existing telemetry patterns (MemoryRecallEventMemoryRecallDeliveryEvent, logMemoryRecalllogMemoryRecallDelivery, recordMemoryRecallMetricsrecordMemoryRecallDeliveryMetrics). No correctness bugs, security holes, or regressions found.

Key observations:

  • Terminal-event accounting is sound. The terminalLogged guard on the handle ensures exactly one terminal event per prefetch. All 13 cancelPendingMemoryPrefetch() call sites now pass a typed discard reason. The tryConsumeMemoryPrefetch() path logs delivery on success and no_relevant_results discard on empty recall.
  • hasToolCalls tracking (the one behavioral change beyond pure telemetry) is correct and necessary — without it, prefetches on no-tool turns would orphan indefinitely with no terminal event. The Retry/ModelFallback reset logic is right: those events invalidate prior tool calls, so hasToolCalls must reset.
  • Privacy: event attributes carry only low-cardinality fields (phase, delivery_point, discard_reason, strategy, docs_selected, latency_ms). No query text, memory content, file paths, session/message IDs, raw errors, or secrets.
  • Test coverage is thorough: 14 new client tests covering initial delivery, empty recall discard, tool_result delivery, no-tool turn discard, reset, shutdown, pre-aborted signal, new_query supersession, Retry/ModelFallback hasToolCalls reset (both directions), arena cancel, and terminal-event idempotency. Plus logger and metrics tests.

No blockers found.

E2E Testing

tmux is not available on this CI runner, so I ran a direct headless CLI test with telemetry outfile export.

Setup: seeded /tmp/triage-7393-test/.qwen/memory/user.md with keyword "ultraviolet parsnip", ran the bundled PR code with QWEN_TELEMETRY_ENABLED=true QWEN_TELEMETRY_OUTFILE=... QWEN_CODE_MEMORY_LOCAL=1.

CLI output (PR code):

$ QWEN_TELEMETRY_ENABLED=true QWEN_TELEMETRY_OUTFILE=/tmp/triage-7393-telemetry.jsonl \
  QWEN_CODE_MEMORY_LOCAL=1 node dist/cli.js -p 'What do I like to cook?' --output-format text

According to my memory, you like **ultraviolet parsnip recipes**.

Telemetry outfile — delivery event:

{
  "event.name": "qwen-code.memory.recall.delivery",
  "event.timestamp": "2026-07-22T01:27:58.038Z",
  "phase": "refined",
  "delivery_point": "tool_result",
  "strategy": "heuristic",
  "docs_selected": 1,
  "latency_ms": 5323
}

Telemetry outfile — delivery metrics:

qwen-code.memory.recall.delivery.count  (COUNTER)
  attributes: { phase: "refined", delivery_point: "tool_result", strategy: "heuristic" }

qwen-code.memory.recall.delivery.latency  (HISTOGRAM, ms)
  attributes: { phase: "refined", delivery_point: "tool_result", strategy: "heuristic" }

✅ Memory recalled and delivered correctly. Delivery event emitted with one terminal outcome (tool_result). Metrics carry only low-cardinality attributes. No query text, memory content, paths, or secrets in the event.

Unit tests:

✓ src/telemetry/loggers.test.ts (65 tests) 91ms
✓ src/telemetry/metrics.test.ts (44 tests) 121ms
✓ src/core/client.test.ts (281 tests) 5165ms

Test Files  3 passed (3)
     Tests  390 passed (390)

Build + typecheck: both pass.

中文说明

代码审查

独立方案: 给定目标(为 auto-memory recall 添加投递/丢弃遥测),我会:在 types.ts 中添加低基数字段的 MemoryRecallDeliveryEvent 类型,在 loggers.ts 中添加 logMemoryRecallDelivery 函数(镜像现有 logMemoryRecall 模式),在 metrics.ts 中添加 count + latency 指标,并在 client.ts 的每个 cancelPendingMemoryPrefetch() 调用点和 tryConsumeMemoryPrefetch() 成功路径接入终态。用 terminalLogged 标记防止重复终态事件。

与 diff 对比: PR 几乎完全匹配此方案。实现遵循现有遥测模式。未发现正确性 bug、安全漏洞或回归。

关键观察:

  • 终态事件记账可靠。terminalLogged 守卫确保每个 prefetch 恰好一个终态事件。所有 13 个 cancelPendingMemoryPrefetch() 调用点都传递了类型化的丢弃原因。
  • hasToolCalls 跟踪(唯一超越纯遥测的行为变更)正确且必要——否则无工具调用 turn 上的 prefetch 会无限孤立。Retry/ModelFallback 重置逻辑正确。
  • 隐私:事件属性仅携带低基数字段。无 query 文本、memory 内容、文件路径、session/message ID、原始错误或 secret。
  • 测试覆盖全面:14 个新 client 测试覆盖所有投递/丢弃路径。

无阻塞问题。

E2E 测试

此 CI 运行器无 tmux,改用直接无头 CLI 测试。CLI 正常响应,delivery 事件和指标正确发出,属性仅含低基数字段,无 PII 泄漏。

单元测试:3 个文件 390 个测试全部通过。构建和类型检查均通过。

Qwen Code · qwen3.7-max

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

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — clean implementation that mirrors existing telemetry patterns exactly, thorough test coverage, verified E2E; the only non-blocking nit is that phase: 'fast' is declared in the type union but never emitted (reserved for the follow-up Fast/Refined split, which is fine).

Honest take: this is a well-scoped, well-tested telemetry addition. The implementation does exactly what my independent proposal would have done — same event type, same logger pattern, same metrics structure, same terminal-event guard. The hasToolCalls tracking is the one piece that goes beyond pure instrumentation, and it's both correct and necessary: without it, prefetches on no-tool turns would silently orphan with no terminal event, making the telemetry incomplete. The autofix loop addressed the original CHANGES_REQUESTED (orphaned prefetch on no-tool turns) and four subsequent review rounds cleanly.

E2E confirms the event fires with the right attributes and no PII leakage. 390 unit tests pass, build and typecheck are green. Every change in the diff serves the stated goal — no scope creep, no drive-by refactors.

If I had to maintain this in six months, I'd thank the author: the terminal-event accounting is easy to follow, the typed discard reasons make debugging straightforward, and the test coverage means I can refactor the prefetch lifecycle without fear.

中文说明

置信度:4/5 —— 实现干净,完全镜像现有遥测模式,测试覆盖全面,E2E 已验证;唯一的非阻塞小问题是 phase: 'fast' 在类型联合中声明但从未发出(为后续 Fast/Refined 拆分预留,没问题)。

诚实评价:这是一个范围合理、测试充分的遥测补充。实现与我的独立方案完全一致。hasToolCalls 跟踪是唯一超越纯遥测的部分,正确且必要。autofix 循环干净地处理了原始 CHANGES_REQUESTED 和四轮后续审查。

E2E 确认事件以正确属性触发,无 PII 泄漏。390 个单元测试通过,构建和类型检查绿色。diff 中每个变更都服务于既定目标——无范围蔓延,无顺手重构。

Qwen Code · qwen3.7-max

Reviewed at 1fd8a2f5eaf63d38733b46b79067970e0b5bcb2c · 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.

Reviewed. Suggestions are inline. 1 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here.

— qwen3.7-max via Qwen Code /review

Comment on lines +667 to 668
this.cancelPendingMemoryPrefetch('shutdown');
}

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.

[Suggestion] requestShutdown() calls cancelPendingMemoryPrefetch('shutdown') — a new discard reason — but no test covers this path. The existing requestShutdown tests verify background memory task gating but none set up a pending prefetch handle, so the cancel path is never exercised in tests.

Failure scenario: A future refactor of requestShutdown() could remove or misplace the cancelPendingMemoryPrefetch('shutdown') call without any test detecting the regression. Operators would lose telemetry for this terminal outcome.

Suggested fix: Add a test that sets up a pending prefetch, calls requestShutdown(), and asserts logMemoryRecallDelivery was called with discard_reason: 'shutdown'. Pattern after the existing reset discard test.

— qwen3.7-max via Qwen Code /review

Comment on lines 2108 to 2110
if (signal.aborted) {
controller.abort();
} else {

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.

[Suggestion] When the parent signal is already aborted at handle creation, only controller.abort() fires — cancelPendingMemoryPrefetch('abort') is not called. The onParentAbort handler in the else branch includes both calls, but the pre-aborted path only mirrors the first.

Failure scenario: A caller enters with an already-aborted AbortSignal. The recall controller is aborted and the catch handler returns EMPTY_RELEVANT_AUTO_MEMORY_RESULT, but the delivery event won't carry discard_reason: 'abort' — operators cannot distinguish pre-abort discard from other empty-result discards.

Suggested fix: Mirror the onParentAbort handler's this.cancelPendingMemoryPrefetch('abort') call in the pre-aborted branch. Note the handle may not yet be installed on this.pendingMemoryPrefetch at this point, so this may require restructuring.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/telemetry/types.ts Outdated
export type MemoryRecallDeliveryPhase = 'fast' | 'refined';
export type MemoryRecallDeliveryPoint = 'initial' | 'tool_result' | 'discarded';
export type MemoryRecallDiscardReason =
| 'not_ready'

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.

[Suggestion] 'not_ready' is declared as a MemoryRecallDiscardReason variant and mirrored in metrics.ts, but no caller in client.ts ever passes this value — all cancel sites use 'shutdown', 'reset', 'new_query', 'abort', 'no_safe_delivery_point', or 'superseded'.

Failure scenario: The dead variant inflates the type union without exercising any code path. A future reader may think there's a code path that produces 'not_ready', or a linter exemption may accumulate around it.

Suggested fix: Remove 'not_ready' from MemoryRecallDiscardReason in both types.ts and metrics.ts, or add it when a caller needs it.

— qwen3.7-max via Qwen Code /review

Comment on lines +686 to +687
if (handle.terminalLogged) return;
handle.terminalLogged = true;

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.

[Suggestion] The terminalLogged idempotency guard is not directly tested. No test constructs a scenario where a handle's terminalLogged is set true before a second call to logMemoryPrefetchDelivery.

Failure scenario: A regression that removes the guard would go undetected by the test suite until a rare timing-dependent double-log surfaces in production (e.g., a race between cancelPendingMemoryPrefetch and tryConsumeMemoryPrefetch).

Suggested fix: Add a test that calls logMemoryPrefetchDelivery (or triggers both delivery and discard) twice on the same handle and asserts logMemoryRecallDelivery is called exactly once.

— qwen3.7-max via Qwen Code /review

Comment on lines +1413 to +1419
recordMemoryRecallDeliveryMetrics(config, event.latency_ms, {
phase: event.phase,
delivery_point: event.delivery_point,
discard_reason: event.discard_reason,
strategy: event.strategy,
docs_selected: event.docs_selected,
});

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.

[Suggestion] logMemoryRecallDelivery conditionally includes discard_reason in log attributes (if (event.discard_reason)) but unconditionally passes event.discard_reason (which may be undefined) to recordMemoryRecallDeliveryMetrics. The metrics function compensates internally with its own conditional spread, but the caller→callee contract is inconsistent.

Failure scenario: On every non-discard delivery (delivery_point: 'initial' or 'tool_result'), the metrics function receives { discard_reason: undefined }. If a future refactor removes the internal guard (relying on callers to omit the key), metric attributes would silently gain discard_reason: undefined as a tag value, polluting dashboard grouping.

Suggested change
recordMemoryRecallDeliveryMetrics(config, event.latency_ms, {
phase: event.phase,
delivery_point: event.delivery_point,
discard_reason: event.discard_reason,
strategy: event.strategy,
docs_selected: event.docs_selected,
});
recordMemoryRecallDeliveryMetrics(config, event.latency_ms, {
phase: event.phase,
delivery_point: event.delivery_point,
...(event.discard_reason ? { discard_reason: event.discard_reason } : {}),
strategy: event.strategy,
docs_selected: event.docs_selected,
});

— qwen3.7-max via Qwen Code /review

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

Reviewed the delivery/discard lifecycle independently — the terminal-event accounting is sound.

  • The terminalLogged guard plus the synchronous consumed = true / pendingMemoryPrefetch = undefined marking at the top of tryConsumeMemoryPrefetch (before the await) closes the consume↔cancel race. Traced consume→cancel, parent-abort→cancel, and the pre-aborted-signal paths; each lands exactly one terminal event. The finally belt-and-suspenders at the bottom of sendMessageStream covers the early-return sites.
  • No terminal event is lost on the normal path: a settled-but-unconsumed prefetch is deliberately preserved for the next ToolResult consume and is always closed out by the next query / reset / shutdown / abort.
  • Cardinality is right — docs_selected (unbounded) stays on the log event but is excluded from the metric attributes, and session.id stays opt-in.

The five suggestions already on the PR are valid non-blocking; nothing to add on top. The pre-aborted-signal one is real but narrow: the finally cleanup applies discard_reason: 'abort' unless an await between prefetch setup and the consume point lets the aborted recall settle and get consumed first.

@yiliang114

Copy link
Copy Markdown
Collaborator

Independently traced the delivery/discard lifecycle end-to-end — no new blockers, the terminal-event accounting holds up.

One non-blocking follow-up worth tightening: recordMemoryRecallDeliveryMetrics in metrics.ts re-declares the discard_reason / phase / strategy unions inline instead of reusing the exported MemoryRecallDiscardReason / MemoryRecallDeliveryPhase from types.ts. Right now the same enum lives in two places, so adding a reason means remembering to touch both — a passed-in new value would fail to compile, so it's guarded, but importing the shared types would drop the drift risk entirely. Same spot as the existing not_ready dead-variant note, so both can be cleaned up together.

Not blocking — fine to land as-is.

@ZijianZhang989
ZijianZhang989 force-pushed the codex/issue-7040-recall-delivery-telemetry branch from 8236edd to 69c925e Compare July 21, 2026 12:34
@github-actions

Copy link
Copy Markdown
Contributor

Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration.

中文

请勿对活跃的 PR 执行 rebase 或 force-push,因为这会使已有的评审评论失效。另外,供日后参考:作为集成流程的一部分,机器人始终会自动将所有改动压缩(squash)为单个提交。

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

Found one terminal-accounting issue that looks worth fixing before merge.

// future early-return sites that forget to call cancel.
if (!normalCompletion) {
this.cancelPendingMemoryPrefetch();
this.cancelPendingMemoryPrefetch(

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.

[P1] A slow recall can still miss terminal delivery telemetry on a normal no-tool turn. This cleanup only runs when normalCompletion is false; when recall is still pending at the initial consume point and the model finishes without tool calls or another continuation, the bottom-of-try path sets normalCompletion = true and preserves the handle for a future ToolResult. But no ToolResult will be scheduled for that no-tool turn, so the prefetch has no terminal delivery/discard event until a later unrelated new query/reset logs a misleading reason, or never if the session ends. That violates the PR's one-terminal-outcome goal; the no-future-delivery path should close the pending prefetch as no_safe_delivery_point (or equivalent) before returning.

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

[Critical] client.ts:3074 — Confirmed (verified): on a normal no-tool turn, normalCompletion = true preserves a still-pending memory prefetch for a future ToolResult that will never arrive. The finally cleanup (line 3089) only fires when !normalCompletion, so no terminal delivery/discard event is emitted. The handle is orphaned until the next unrelated cancel (new_query, reset) logs a misleading discard_reason with inflated latency, or never closed if the session ends. Violates the PR's one-terminal-outcome-per-prefetch goal. (Existing P1 from @yiliang114 at client.ts:3071, confirmed still-standing.)

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 21, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /takeover

@qwen-code-dev-bot qwen-code-dev-bot added the autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+) label Jul 21, 2026
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤝 Takeover engaged: the autofix loop now manages this PR — it will address new review feedback and resolve base conflicts until the label is removed or the round cap is reached. Remove the autofix/takeover label (or comment @qwen-code /takeover stop) to release.

中文说明

🤝 已接管:autofix 循环现在管理此 PR —— 将持续处理新的评审反馈与 base 冲突,直到移除标签或达到轮次上限。移除 autofix/takeover 标签(或评论 @qwen-code /takeover stop)即可释放。

On a normal turn where the model responds without tool calls, the
pending memory prefetch was preserved for a future ToolResult that
would never arrive. This orphaned the handle until an unrelated
cancel logged a misleading discard_reason with inflated latency,
or never closed it if the session ended.

Track whether the streaming loop saw any ToolCallRequest events and
cancel the prefetch with no_safe_delivery_point at normalCompletion
when none were requested. Also reuse the shared delivery/discard
type aliases in metrics.ts instead of re-declaring them inline.
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 1/100). What changed, and what I pushed back on:

Review Feedback Summary

Required fixes (Critical / P1)

Orphaned prefetch on no-tool turns ([Critical] @qwen-code-ci-bot at client.ts:3074, [P1] @yiliang114 at client.ts:3071)

  • Decision: Fixed.
  • Root cause: At the bottom of the try block, normalCompletion = true unconditionally preserved the pending memory prefetch for a future ToolResult turn. On a no-tool turn (model responds without requesting tool calls), no ToolResult will ever arrive, so the handle was orphaned until an unrelated new_query/reset cancel logged a misleading discard_reason with inflated latency — or never closed if the session ended.
  • Fix: Track whether the streaming loop observed any ToolCallRequest events via a local hasToolCalls flag. At normalCompletion, cancel the prefetch with no_safe_delivery_point when no tool calls were requested. When tool calls ARE pending, the handle is still preserved for the next ToolResult turn (the fire-and-forget design is unchanged for that path).
  • Tests: Added a new test "should discard pending prefetch with no_safe_delivery_point on a no-tool turn". Updated six existing tests that rely on the prefetch surviving past normalCompletion to include a tool_call_request event in the mock stream, accurately simulating a tool-call turn.

Suggestions implemented

Reuse shared type aliases in metrics.ts (@yiliang114 issue-level comment)

  • Decision: Implemented.
  • Change: recordMemoryRecallDeliveryMetrics in metrics.ts now imports MemoryRecallDeliveryPhase, MemoryRecallDeliveryPoint, and MemoryRecallDiscardReason from types.ts instead of re-declaring the same unions inline. This eliminates the drift risk of maintaining the same enum in two places.

Suggestions declined (with reasons)

[rc:3620184792] Add test for requestShutdown() cancel path (@doudouOUC)

  • Decision: Declined — already addressed.
  • Reason: The test "should log discard telemetry when pending auto-memory is shut down" (added in commit 69c925e) already covers this path: it sets up a pending prefetch, calls requestShutdown(), and asserts logMemoryRecallDelivery was called with discard_reason: 'shutdown'.

[rc:3620184796] Pre-aborted signal path missing cancelPendingMemoryPrefetch('abort') (@doudouOUC)

  • Decision: Declined — already handled.
  • Reason: The pre-aborted branch sets prefetchAbortReason = 'abort' and calls controller.abort(). After the handle is installed on this.pendingMemoryPrefetch, the if (prefetchAbortReason) block calls this.cancelPendingMemoryPrefetch(prefetchAbortReason), which emits the discard event with discard_reason: 'abort'. The test "should log abort discard telemetry when caller signal is already aborted" verifies this end-to-end.

[rc:3620184804] Remove dead 'not_ready' variant (@doudouOUC)

  • Decision: Declined — finding does not match the code.
  • Reason: MemoryRecallDiscardReason in types.ts does not contain a 'not_ready' variant. The union is 'no_safe_delivery_point' | 'new_query' | 'reset' | 'abort' | 'shutdown' | 'superseded'. The same applies to the (now-replaced) inline union in metrics.ts.

[rc:3620184810] Test the terminalLogged idempotency guard (@doudouOUC)

  • Decision: Declined — already addressed.
  • Reason: The test "should log only one terminal event for the same prefetch handle" (added in commit 69c925e) calls logMemoryPrefetchDelivery twice on the same handle and asserts logMemoryRecallDelivery is called exactly once.

[rc:3620184815] Inconsistent discard_reason passing to metrics (@doudouOUC)

  • Decision: Declined — already addressed.
  • Reason: logMemoryRecallDelivery in loggers.ts already uses a conditional spread: ...(event.discard_reason ? { discard_reason: event.discard_reason } : {}). The suggested code change matches what is already in the file.

Verification

  • npm run build
  • npm run typecheck
  • npm run lint (changed files) ✅
  • npx vitest run src/core/client.test.ts — 276/276 passed ✅
  • npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts — 109/109 passed ✅

Conflict notes

No conflicts (--conflict false).

中文说明

审查反馈总结

必须修复(Critical / P1)

无工具调用回合中孤立的预取句柄([Critical] @qwen-code-ci-bot 在 client.ts:3074,[P1] @yiliang114 在 client.ts:3071)

  • 决定: 已修复。
  • 根因:try 块底部,normalCompletion = true 无条件地保留待处理的内存预取句柄,等待未来的 ToolResult 回合消费。在无工具调用的回合中(模型响应未请求工具调用),不会有 ToolResult 到来,因此句柄被孤立,直到不相关的 new_query/reset 取消操作以误导性的 discard_reason 和膨胀的延迟记录——或者如果会话结束则永远不会关闭。
  • 修复: 通过局部变量 hasToolCalls 跟踪流式循环是否观察到任何 ToolCallRequest 事件。在 normalCompletion 处,当没有工具调用请求时,以 no_safe_delivery_point 取消预取。当有工具调用待处理时,句柄仍保留给下一个 ToolResult 回合(即发即弃设计在该路径上不变)。
  • 测试: 新增测试"should discard pending prefetch with no_safe_delivery_point on a no-tool turn"。更新了六个依赖预取在 normalCompletion 后存活的现有测试,在 mock 流中加入 tool_call_request 事件,准确模拟工具调用回合。

已实施的建议

在 metrics.ts 中复用共享类型别名@yiliang114 issue 级评论)

  • 决定: 已实施。
  • 变更: metrics.ts 中的 recordMemoryRecallDeliveryMetrics 现在从 types.ts 导入 MemoryRecallDeliveryPhaseMemoryRecallDeliveryPointMemoryRecallDiscardReason,而非内联重新声明相同的联合类型。这消除了在两处维护相同枚举的漂移风险。

已拒绝的建议(附原因)

[rc:3620184792] 为 requestShutdown() 取消路径添加测试@doudouOUC

  • 决定: 拒绝——已解决。
  • 原因: 测试"should log discard telemetry when pending auto-memory is shut down"(在提交 69c925e 中添加)已覆盖此路径:它设置待处理的预取,调用 requestShutdown(),并断言 logMemoryRecallDeliverydiscard_reason: 'shutdown' 被调用。

[rc:3620184796] 预中止信号路径缺少 cancelPendingMemoryPrefetch('abort')@doudouOUC

  • 决定: 拒绝——已处理。
  • 原因: 预中止分支设置 prefetchAbortReason = 'abort' 并调用 controller.abort()。在句柄安装到 this.pendingMemoryPrefetch 后,if (prefetchAbortReason) 块调用 this.cancelPendingMemoryPrefetch(prefetchAbortReason),发出带有 discard_reason: 'abort' 的丢弃事件。测试"should log abort discard telemetry when caller signal is already aborted"端到端验证了这一点。

[rc:3620184804] 移除死代码 'not_ready' 变体@doudouOUC

  • 决定: 拒绝——发现与代码不符。
  • 原因: types.ts 中的 MemoryRecallDiscardReason 不包含 'not_ready' 变体。联合类型为 'no_safe_delivery_point' | 'new_query' | 'reset' | 'abort' | 'shutdown' | 'superseded'metrics.ts 中(现已替换的)内联联合类型同理。

[rc:3620184810] 测试 terminalLogged 幂等性守卫@doudouOUC

  • 决定: 拒绝——已解决。
  • 原因: 测试"should log only one terminal event for the same prefetch handle"(在提交 69c925e 中添加)对同一句柄调用两次 logMemoryPrefetchDelivery,并断言 logMemoryRecallDelivery 仅被调用一次。

[rc:3620184815] discard_reason 传递给 metrics 不一致@doudouOUC

  • 决定: 拒绝——已解决。
  • 原因: loggers.ts 中的 logMemoryRecallDelivery 已使用条件展开:...(event.discard_reason ? { discard_reason: event.discard_reason } : {})。建议的代码变更与文件中已有的内容一致。

验证

  • npm run build
  • npm run typecheck
  • npm run lint(变更文件)✅
  • npx vitest run src/core/client.test.ts — 276/276 通过 ✅
  • npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts — 109/109 通过 ✅

冲突说明

无冲突(--conflict false)。

Base-conflict check: no conflict with main.

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human.


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

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

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.ts
Comment thread packages/core/src/telemetry/metrics.ts
Comment thread packages/core/src/core/client.ts
Comment thread packages/core/src/core/client.ts
doudouOUC
doudouOUC previously approved these changes Jul 21, 2026

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

No issues found. LGTM! ✅

— qwen3.7-max via Qwen Code /review

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 2/100). What changed, and what I pushed back on:

Review feedback addressed

All four suggestions from the automated reviewer have been implemented.

1. Add discard_reason for empty-recall discards (rc:3624547461)

Decision: Implemented.

Changes: Added 'no_relevant_results' to the MemoryRecallDiscardReason union type and passed it in the tryConsumeMemoryPrefetch else branch when recall settles with an empty prompt. This makes empty-recall discards distinguishable from external discards (reset, abort, shutdown) in telemetry dashboards without requiring operators to filter by discard_reason presence.

Files: types.ts, client.ts, client.test.ts

2. Remove unused docs_selected from metrics attrs (rc:3624547469)

Decision: Implemented — removed docs_selected from the recordMemoryRecallDeliveryMetrics attrs type and from the caller in loggers.ts.

Rationale: docs_selected was accepted by the metrics function but never included in metricAttributes — it was deliberately excluded from metric dimensions for cardinality control (the existing test validated its absence). The field is still recorded in the OTel log event attributes via logMemoryRecallDelivery, so no telemetry data is lost. Removing it from the metrics function signature eliminates the dead parameter that could mislead future developers.

Files: metrics.ts, loggers.ts, loggers.test.ts, metrics.test.ts

3. Reset hasToolCalls on retry/model_fallback (rc:3624547479)

Decision: Implemented — added an else if branch that resets hasToolCalls = false when Retry or ModelFallback events are seen in the stream.

Rationale: The reviewer suggested using turn.pendingToolCalls.length instead, which is the authoritative source in production. However, the test infrastructure mocks turn.run(), so turn.pendingToolCalls is never populated in tests — switching to it would require restructuring 7+ test cases. Resetting hasToolCalls on retry/fallback events directly fixes the described failure scenario (transient error clears pendingToolCalls but hasToolCalls stays true) with a minimal, test-compatible change.

Files: client.ts

4. Remove dead default parameter = 'superseded' (rc:3624547492)

Decision: Implemented — removed the default value, making discardReason a required parameter. Also replaced 'superseded' with 'no_relevant_results' in the MemoryRecallDiscardReason union (see item 1), since 'superseded' had no remaining use.

Files: client.ts, types.ts

Verification

  • npx tsc --noEmit — passes
  • npx eslint on all changed files — passes
  • npx vitest run src/core/client.test.ts — 276/276 tests pass
  • npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts — 109/109 tests pass
中文说明

已处理的审查反馈

自动化审查器提出的四条建议均已实现。

1. 为空召回丢弃添加 discard_reason(rc:3624547461)

决定: 已实现。

变更:MemoryRecallDiscardReason 联合类型中添加了 'no_relevant_results',并在 tryConsumeMemoryPrefetch 的 else 分支中(当召回结果为空时)传入该值。这使得空召回丢弃与外部丢弃(reset、abort、shutdown)在遥测仪表板中可以区分,无需运维人员通过 discard_reason 字段是否存在来手动过滤。

文件: types.tsclient.tsclient.test.ts

2. 从指标属性中移除未使用的 docs_selected(rc:3624547469)

决定: 已实现——从 recordMemoryRecallDeliveryMetrics 的属性类型和 loggers.ts 中的调用方移除了 docs_selected

理由: docs_selected 被指标函数接受但从未包含在 metricAttributes 中——它是为了基数控制而刻意排除在指标维度之外的(现有测试验证了它的缺失)。该字段仍然通过 logMemoryRecallDelivery 记录在 OTel 日志事件属性中,因此不会丢失遥测数据。从指标函数签名中移除它消除了可能误导未来开发者的无效参数。

文件: metrics.tsloggers.tsloggers.test.tsmetrics.test.ts

3. 在 retry/model_fallback 时重置 hasToolCalls(rc:3624547479)

决定: 已实现——添加了一个 else if 分支,在流中检测到 RetryModelFallback 事件时将 hasToolCalls 重置为 false

理由: 审查器建议改用 turn.pendingToolCalls.length,这在生产环境中是权威来源。然而,测试基础设施 mock 了 turn.run(),因此 turn.pendingToolCalls 在测试中永远不会被填充——切换到它需要重构 7 个以上的测试用例。在 retry/fallback 事件时重置 hasToolCalls 直接修复了描述的失败场景(瞬态错误清除了 pendingToolCallshasToolCalls 保持为 true),且变更最小、兼容测试。

文件: client.ts

4. 移除无效默认参数 = 'superseded'(rc:3624547492)

决定: 已实现——移除了默认值,使 discardReason 成为必需参数。同时在 MemoryRecallDiscardReason 联合类型中将 'superseded' 替换为 'no_relevant_results'(见第 1 项),因为 'superseded' 已无其他用途。

文件: client.tstypes.ts

验证

  • npx tsc --noEmit — 通过
  • npx eslint(所有变更文件)— 通过
  • npx vitest run src/core/client.test.ts — 276/276 测试通过
  • npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts — 109/109 测试通过

Base-conflict check: no conflict with main.

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human.


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

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

Reviewed. Not reviewed: You are review agent verify — Verification agent (round... — pointed at diff lines it never opened: it made tool calls, but none of them read the diff.

— qwen3.7-max via Qwen Code /review

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

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.ts
Comment thread packages/core/src/core/client.ts
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 3/100). What changed, and what I pushed back on:

Review feedback addressed

rc:3625266545 — hasToolCalls reset on Retry/ModelFallback lacks test coverage

Decision: Implemented.

Changes: Added two tests in packages/core/src/core/client.test.ts:

  1. "should discard prefetch when Retry resets hasToolCalls" — mock stream yields ToolCallRequest then Retry; verifies the prefetch is discarded with no_safe_delivery_point and the handle is cleared.
  2. "should preserve prefetch when ToolCallRequest follows Retry" — mock stream yields ToolCallRequest, Retry, then another ToolCallRequest; verifies the prefetch is preserved (handle still defined) and no no_safe_delivery_point discard telemetry was logged.

rc:3625266548 — Arena cancel path has no telemetry test with a live prefetch

Decision: Implemented.

Changes: Added one test in packages/core/src/core/client.test.ts:

  • "should log abort discard telemetry when arena cancels with a pending prefetch" — Turn 1 fires a prefetch and yields tool_call_request (preserving the handle past end-of-turn). Turn 2 (ToolResult) receives an arena cancel control signal before the turn runs. Verifies reportCancelled is called, logMemoryRecallDelivery receives discard_reason: 'abort' / delivery_point: 'discarded', and the handle is cleared.

Conflict

No conflict (--conflict false).

Verification

  • npm run build
  • npm run typecheck
  • npm run lint
  • npx vitest run src/core/client.test.ts — 279 tests passed ✅
中文说明

已处理的评审反馈

rc:3625266545 — hasToolCalls 在 Retry/ModelFallback 时重置缺少测试覆盖

决定: 已实现。

变更:packages/core/src/core/client.test.ts 中新增两个测试:

  1. "should discard prefetch when Retry resets hasToolCalls" — mock 流先产生 ToolCallRequest,然后产生 Retry;验证 prefetch 以 no_safe_delivery_point 原因被丢弃,且句柄被清除。
  2. "should preserve prefetch when ToolCallRequest follows Retry" — mock 流依次产生 ToolCallRequestRetry、再次 ToolCallRequest;验证 prefetch 被保留(句柄仍然存在),且没有记录 no_safe_delivery_point 丢弃遥测。

rc:3625266548 — Arena 取消路径在有活跃 prefetch 时缺少遥测测试

决定: 已实现。

变更:packages/core/src/core/client.test.ts 中新增一个测试:

  • "should log abort discard telemetry when arena cancels with a pending prefetch" — 第一轮触发 prefetch 并产生 tool_call_request(使句柄在轮次结束后保留)。第二轮(ToolResult)在轮次运行前收到 arena 取消控制信号。验证 reportCancelled 被调用、logMemoryRecallDelivery 收到 discard_reason: 'abort' / delivery_point: 'discarded',且句柄被清除。

冲突

无冲突(--conflict false)。

验证

  • npm run build
  • npm run typecheck
  • npm run lint
  • npx vitest run src/core/client.test.ts — 279 个测试全部通过 ✅

Base-conflict check: no conflict with main.

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human.


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

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

Reviewed. Suggestions are inline. 2 Suggestion-level finding(s) could not be anchored to a changed line and were dropped; nothing further to act on here. Not reviewed: issue-fidelity — Agent 0 ran and returned a substantive fidelity assessment but the harness did not match its launch prompt to the CLI-built brief. Not reviewed: build-and-test — Agent 7 ran build (16 workspaces passed) and tests (388 PR-changed tests passed) but the harness did not match its launch prompt to the CLI-built brief. Not reviewed: Agent 0: Issue fidelity & root-cause ownership, Agent 7: Build & test verification — its prompt was built, but no agent on record was launched with it.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/telemetry/loggers.ts
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 4/100). What changed, and what I pushed back on:

Review feedback addressed

[rc:3625927519] Add session.id to logMemoryRecallDelivery attributes — Implemented

Finding: logMemoryRecallDelivery was the only logger function (out of 47) that did not spread ...getCommonAttributes(config) into its log attributes, causing emitted events to lack session.id and breaking session-level log correlation.

Changes:

  • packages/core/src/telemetry/loggers.ts: Added ...getCommonAttributes(config) spread to the attributes object in logMemoryRecallDelivery, consistent with all other logger functions.
  • packages/core/src/telemetry/loggers.test.ts: Updated the test to positively assert session.id is present with the expected value, and removed session from the sensitive-data regex since session.id is a correlation UUID, not sensitive content.

Verification: npm run build, npm run typecheck, npm run lint all pass. Focused Vitest runs: loggers.test.ts (65 tests passed), client.test.ts (279 tests passed), metrics.test.ts (passed).

Conflict: None (--conflict false).

中文说明

已处理的评审反馈

[rc:3625927519] 为 logMemoryRecallDelivery 添加 session.id 属性 — 已实现

发现: logMemoryRecallDelivery 是 47 个日志函数中唯一没有展开 ...getCommonAttributes(config) 到日志属性的函数,导致发出的事件缺少 session.id,无法进行会话级别的日志关联。

变更:

  • packages/core/src/telemetry/loggers.ts:在 logMemoryRecallDelivery 的属性对象中添加了 ...getCommonAttributes(config) 展开,与所有其他日志函数保持一致。
  • packages/core/src/telemetry/loggers.test.ts:更新测试,正向断言 session.id 存在且值正确,并从敏感数据正则表达式中移除 session,因为 session.id 是关联 UUID,不属于敏感内容。

验证: npm run buildnpm run typechecknpm run lint 全部通过。聚焦 Vitest 运行:loggers.test.ts(65 个测试通过)、client.test.ts(279 个测试通过)、metrics.test.ts(通过)。

冲突: 无(--conflict false)。

Base-conflict check: no conflict with main.

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human.


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

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

Reviewed — no blockers. Suggestions are inline.

— qwen3.7-max via Qwen Code /review

Comment thread packages/core/src/core/client.ts
@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Addressed the latest review feedback (round 5/100). What changed, and what I pushed back on:

Review feedback addressed

[rc:3626340356] ModelFallback resetting hasToolCalls has no test coverage

Classification: Suggestion — valid, implemented.

Decision: Added two tests mirroring the existing Retry coverage:

  1. should discard prefetch when ModelFallback resets hasToolCalls — a ToolCallRequest sets hasToolCalls, then a model_fallback event resets it; the end-of-turn check discards the prefetch with no_safe_delivery_point.
  2. should preserve prefetch when ToolCallRequest follows ModelFallback — a ToolCallRequest after the model_fallback re-sets hasToolCalls; the prefetch is preserved and no discard telemetry is logged.

Both tests include the required fromModel, toModel, and fallbackIndex fields per ServerGeminiModelFallbackEvent.

Changes: packages/core/src/core/client.test.ts — 117 lines added (two new test cases).

Conflict notes: No conflicts (--conflict false).

Verification:

  • npm run build
  • npm run typecheck
  • npm run lint
  • npx vitest run src/core/client.test.ts — 281 tests passed ✅
中文说明

已处理的评审反馈

[rc:3626340356] ModelFallback 重置 hasToolCalls 缺少测试覆盖

分类: 建议 — 有效,已实现。

决定: 新增两个测试,与现有的 Retry 覆盖测试对称:

  1. should discard prefetch when ModelFallback resets hasToolCallsToolCallRequest 设置 hasToolCalls 后,model_fallback 事件将其重置;回合结束时检查以 no_safe_delivery_point 丢弃预取。
  2. should preserve prefetch when ToolCallRequest follows ModelFallbackmodel_fallback 之后再次出现 ToolCallRequest 重新设置 hasToolCalls;预取被保留,不记录丢弃遥测。

两个测试均按 ServerGeminiModelFallbackEvent 类型要求包含了 fromModeltoModelfallbackIndex 字段。

变更: packages/core/src/core/client.test.ts — 新增 117 行(两个新测试用例)。

冲突说明: 无冲突(--conflict false)。

验证:

  • npm run build
  • npm run typecheck
  • npm run lint
  • npx vitest run src/core/client.test.ts — 281 个测试全部通过 ✅

Base-conflict check: no conflict with main.

Re-review when you have a moment. After round 100 this bot stops and leaves the PR for a human.


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

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

⚠️ Downgraded from Approve to Comment: CI still running. Reviewed.

— qwen3.7-max via Qwen Code /review

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Reviewed the latest feedback — no changes needed. Why, point by point:

Autofix review triage — PR #7393

No code changes were made in this round.

Feedback reviewed

  • Automated review (@qwen-code-ci-bot, via Qwen Code /review) — status
    COMMENTED, downgraded from Approve to Comment with the note "CI still
    running. Reviewed." This is a status notice about CI timing, not a defect
    report or a change request. It names no bug, no broken build/test, no
    security issue, and is not a CHANGES_REQUESTED review.

Triage result

  • Inline comments: none.
  • Issue-level comments: none.
  • Failed checks: none reported in this feedback batch.
  • Required fixes: none — no correctness, build, test, or security issue
    was raised.
  • Optional suggestions: none to implement or defer.

Since there is nothing actionable, the PR head is left unchanged. No commit was
created and no review threads need resolving.

中文说明

Autofix 审查分诊 — PR #7393

本轮未做任何代码改动。

已审阅的反馈

  • 自动审查(@qwen-code-ci-bot,经由 Qwen Code /review —— 状态为
    COMMENTED,因“CI 仍在运行”而从 Approve 降级为 Comment,附注“CI still
    running. Reviewed.”。这是一条关于 CI 时序的状态提示,并非缺陷报告或修改
    请求。它没有指出任何 bug、构建/测试失败或安全问题,也不是
    CHANGES_REQUESTED 审查。

分诊结果

  • 行内评论: 无。
  • Issue 级评论: 无。
  • 失败的检查: 本批反馈中未报告任何失败检查。
  • 必须修复项: 无 —— 未提出任何正确性、构建、测试或安全问题。
  • 可选建议: 没有需要实现或延后的建议。

由于没有可处理的内容,PR 的 head 分支保持不变。未创建任何提交,也没有需要
解决的审查线程。

Base-conflict check: no conflict with main.


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

@wenshao

wenshao commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

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

@wenshao

wenshao commented Jul 22, 2026

Copy link
Copy Markdown
Collaborator

✅ Local real-build verification (Linux) — merge reference

Verified at HEAD 1fd8a2f5e. Built and ran everything on Linux — the PR's test matrix marks Linux not tested, so this closes that gap. Everything green; no issues found.

1. Reviewer test plan — unit tests ✅

packages/core $ npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts
  → 109 passed  (loggers 65, metrics 44)
packages/core $ npx vitest run src/core/client.test.ts
  → 281 passed

The 15 new delivery-telemetry cases (12 in client.test.ts, 2 in loggers.test.ts, 1 in metrics.test.ts) all pass.

2. Build + typecheck ✅

npm run build --workspace=packages/core → exit 0  ·  tsc --noEmit → exit 0.

3. Negative control (RED/GREEN) ✅

Reverting only client.ts to the merge-base (957a93f) while keeping the PR's tests flips exactly 12 tests RED — all of them the prefetch delivery-lifecycle cases (269 still pass); restoring → 281 pass. The tests genuinely discriminate the change; they are not vacuous.

4. Real end-to-end on Linux ✅ (the part the PR couldn't run)

Drove the real bundled CLI (dist/cli.js, v0.20.0) under a pseudo-terminal against a scripted fake OpenAI server, with a seeded .qwen/memory/ doc, managed auto-memory on (default), and --telemetry --telemetry-target local --telemetry-outfile. The events/metrics below were written by the real OpenTelemetry FileLogExporter / FileMetricExporter — not mocks.

Scenario (real run) delivery_point discard_reason strategy docs latency
Tool-call turn, memory selected tool_result model 1 186 ms
Plain turn, no tool call discarded no_safe_delivery_point none 0 69 ms
Tool-call turn, model picks nothing discarded no_relevant_results none 0 312 ms
  • Each run emitted exactly one terminal qwen-code.memory.recall.delivery log record + one .count + one .latency metric — no duplicate/missing terminal events. That is the PR's stated main risk, and the terminalLogged guard holds in a real run.
  • The no_safe_delivery_point row is the "close orphaned prefetch on no-tool turns" fix (2b83e6ab) firing for real: a plain turn with no tool call closes the prefetch instead of leaking it.
  • No-PII confirmed on the actual exported records — attributes are only session.id, event.name, event.timestamp, phase, delivery_point, strategy, docs_selected, latency_ms, [discard_reason]. No query text, memory content, file/project paths, message ids, raw errors, or secrets. Metrics carry an even smaller low-cardinality set.
  • phase is always refined, as documented (fast is never emitted yet — reserved for the follow-up split).

Not reproduced in a live TUI (timing, exactly as you noted): the initial delivery point (recall settles after the early consume poll, so the canonical success path in practice is tool_result — matching the fire-and-forget design), and the new_query / reset / shutdown / abort discard reasons. All are covered by the unit tests + the RED/GREEN above.

Screenshots

Real TUI run — tool_result delivery (model selected the seeded memory, shell tool ran under YOLO):

real TUI run

Delivery records + metrics from the real telemetry outfile (all three outcomes):

telemetry outfile evidence

Verdict: LGTM from a verification standpoint — behavior matches the description, the delivery/discard signal is low-cardinality and PII-free, and the terminal-event guard is solid on Linux end-to-end.

中文说明

✅ 本地真实构建验证(Linux)— 合并参考

在 HEAD 1fd8a2f5e 上验证。全部在 Linux 上构建并运行——PR 的测试矩阵里 Linux 标注为 not tested,本次正好补上这一项。全部通过,未发现问题。

1. Reviewer test plan — 单元测试 ✅

packages/core $ npx vitest run src/telemetry/loggers.test.ts src/telemetry/metrics.test.ts
  → 109 passed  (loggers 65, metrics 44)
packages/core $ npx vitest run src/core/client.test.ts
  → 281 passed

本 PR 新增的 15 个投递遥测用例(client.test.ts 12 个、loggers.test.ts 2 个、metrics.test.ts 1 个)全部通过。

2. 构建 + 类型检查 ✅

npm run build --workspace=packages/core → exit 0  ·  tsc --noEmit → exit 0。

3. 负对照(RED/GREEN)✅

client.ts 回退到 merge-base(957a93f)、保留 PR 的测试,恰好 12 个测试变红——全部是 prefetch 投递生命周期用例(其余 269 通过);恢复后 → 281 通过。说明这些测试确实能区分本次改动,并非空测。

4. Linux 上的真实端到端 ✅(PR 未能覆盖的部分)

在伪终端下驱动真实打包后的 CLIdist/cli.js,v0.20.0),对接一个脚本化的 fake OpenAI server,播种一个 .qwen/memory/ 文档,managed auto-memory 默认开启,并加上 --telemetry --telemetry-target local --telemetry-outfile。下表的事件/指标是由真实的 OpenTelemetry FileLogExporter / FileMetricExporter 写入的,非 mock。

场景(真实运行) delivery_point discard_reason strategy docs latency
工具调用轮、命中 memory tool_result model 1 186 ms
纯文本轮、无工具调用 discarded no_safe_delivery_point none 0 69 ms
工具调用轮、模型未选中 discarded no_relevant_results none 0 312 ms
  • 每次运行都恰好产生一条终态 qwen-code.memory.recall.delivery 日志 + 一条 .count + 一条 .latency 指标——无重复、无遗漏终态事件。这正是 PR 提到的主要风险,terminalLogged 守卫在真实运行中生效。
  • no_safe_delivery_point 那一行就是"无工具轮次关闭孤儿 prefetch"修复(2b83e6ab)的真实触发:纯文本轮无工具调用时,prefetch 被正确关闭而不是泄漏。
  • 在真实导出记录上确认无 PII——属性仅有 session.id, event.name, event.timestamp, phase, delivery_point, strategy, docs_selected, latency_ms, [discard_reason],不含 query 文本、memory 内容、文件/项目路径、message id、raw error 或 secret。指标携带的低基数字段更少。
  • phase 恒为 refined(符合文档说明,fast 目前从不发出,留给后续拆分)。

真实 TUI 中未复现(时序原因,正如你所说):initial 投递点(recall 在早期 consume 轮询之后才落定,所以实践中规范的成功路径是 tool_result,与 fire-and-forget 设计一致),以及 new_query / reset / shutdown / abort 这几个 discard reason。它们都已被单元测试 + 上面的 RED/GREEN 覆盖。

截图

真实 TUI 运行——tool_result 投递(模型选中了播种的 memory,YOLO 下执行了 shell 工具):

真实 TUI 运行

真实遥测 outfile 中的投递记录 + 指标(三种结局):

遥测 outfile 证据

结论: 从验证角度 LGTM——行为与描述一致,投递/丢弃信号低基数且无 PII,终态事件守卫在 Linux 端到端下稳固。


Verified locally with Claude Code (Opus 4.8, 1M context) — real bundled-CLI E2E + focused unit tests on Linux.

@qwen-code-dev-bot

Copy link
Copy Markdown
Collaborator

🤖 Reviewed the latest feedback — no changes needed. Why, point by point:

No action taken on PR #7393

The only new feedback since the last evaluation is an issue-level comment from
@wenshao: a local real-build verification report (Linux) that ends with an
explicit LGTM. It contains no change request, no named defect, and no
suggestion.

Triage

Feedback Type Decision
@wenshao — "✅ Local real-build verification (Linux)… Everything green; no issues found… LGTM from a verification standpoint" Positive verification / approval No action — confirms behavior matches the PR description, the delivery/discard signal is low-cardinality and PII-free, and the terminal-event guard holds on Linux end-to-end. It also closes the "Linux not tested" gap in the test matrix.

There are no reviews (no CHANGES_REQUESTED), no inline comments, no
failed checks
, and no still-red checks to address. Nothing requires a code
change, so no commit was made and the branch is unchanged at HEAD 1fd8a2f5e.

中文说明

PR #7393 未采取任何操作

自上次评估以来唯一的新反馈是 @wenshao 的一条 issue 级评论:一份本地真实构建验证报告(Linux),并以明确的 LGTM 结尾。该评论不包含任何改动请求、未指名任何缺陷,也没有任何建议。

分类

反馈 类型 决定
@wenshao — “✅ 本地真实构建验证(Linux)…… 全部通过,未发现问题…… 从验证角度 LGTM” 正面验证 / 认可 不采取行动——确认行为与 PR 描述一致,投递/丢弃信号为低基数且无 PII,终态事件守卫在 Linux 端到端下稳固。同时补上了测试矩阵中“Linux 未测试”的缺口。

没有 review(无 CHANGES_REQUESTED)、没有行内评论没有失败的检查,也没有持续飘红的检查需要处理。无需任何代码改动,因此未产生提交,分支保持在 HEAD 1fd8a2f5e 不变。

Base-conflict check: no conflict with main.


🧠 Handled by Qwen Code · model/模型 qwen3.8-max-preview

@wenshao
wenshao added this pull request to the merge queue Jul 22, 2026
Merged via the queue into QwenLM:main with commit fa2e72d Jul 22, 2026
60 checks passed
yiliang114 pushed a commit that referenced this pull request Aug 12, 2026
memory-system.md documented recall selection but never documented delivery, so
the delivery telemetry from #7393 was undocumented and the fast path had no
home in the canonical reference. Add a delivery section and the delivery event
table, and correct two docs that still described the single-path behaviour.
samuelhsin pushed a commit to samuelhsin/qwen-code that referenced this pull request Aug 18, 2026
…M#8716)

* fix(memory): improve recall delivery and multilingual fallback

* fix(memory): bound heuristic recall scoring

* test(memory): pin initial recall budget with fake timers

Rewrite the slow-recall test to assert with fake timers that the main
request is still held 1 ms inside the 100 ms initial budget and proceeds
without memory at expiry, so budget changes can no longer pass unnoticed.

* test(memory): pin recall budget and scoring contracts

Address review findings with mutation-verified pins:
- settle-early: bounded wait ends when recall settles, not at full budget
- Cron and ToolResult consume points stay zero-wait
- post-wait replacement guard refuses stale handles
- type boost flips the winner (tie-break no longer masks its removal)
- hiragana-only coverage for the CJK tokenizer
- design doc: RFC QwenLM#7040 sets no numeric overhead target; fix attribution

* fix(memory): preserve recall field weighting

* fix(memory): recall relevant topics beyond scan cap (QwenLM#8803)

* fix(memory): bound recall candidates after full scan

* test(memory): pin bounded selector inputs

* fix(memory): preserve bounded recall candidates

* fix(memory): preserve lexical recall candidates

* fix(memory): prioritize lexical model candidates

* fix(core): preserve UTF-16 manifest boundaries

* fix(memory): address recall review feedback

* test(memory): measure recall rollout gate against the pre-change scorer

RFC QwenLM#7040 gates the multilingual precision change on evidence that English
Recall@5 and no-result precision do not regress. Add a labeled 45-case
corpus and an evaluation harness that scores both the shipped deterministic
selector and a frozen copy of the pre-change scorer over it, so the gate is
reproducible rather than asserted.

* fix(memory): deliver a deterministic fast recall result on the initial turn

The initial-turn budget is 100 ms, but recall awaits the model selector,
which is a network side query with a 30 s ceiling. The budget therefore
expires on the common path and delivery falls through to the ToolResult
point — which a tool-free turn never reaches, so the result is discarded as
no_safe_delivery_point. That is the case memory matters most for.

Publish the deterministic candidates that selectModelCandidateDocuments
already computes, before blocking on the selector, and inject them when the
budget expires. The refined result still lands at ToolResult, with documents
the fast phase already delivered filtered out.

phase telemetry now carries both stages: phase is the delivery stage, strategy
is the selection method, and they are orthogonal.

* docs(memory): record the fast-path decision and phase/strategy split

* test(memory): report the mixed-language slice in the rollout gate

* docs(memory): align recall docs on the deterministic fast path

memory-system.md documented recall selection but never documented delivery, so
the delivery telemetry from QwenLM#7393 was undocumented and the fast path had no
home in the canonical reference. Add a delivery section and the delivery event
table, and correct two docs that still described the single-path behaviour.

* fix(memory): use Array<T> for the fast-path test doc lists

@typescript-eslint/array-type forbids T[] for non-simple types.

* docs(memory): clarify recall delivery telemetry

* fix(memory): report already-delivered recall count

* docs(memory): align recall delivery claims

* fix(memory): rank ties by recency and record fast-delivered discards

Three review follow-ups on the recall reliability change.

Tie-break: `selectRelevantAutoMemoryDocuments` broke score ties with
`type.localeCompare`, which orders feedback < project < reference < user.
That was tolerable while the result was five documents wide; the fast path
takes only MAX_FAST_RECALL_DOCS = 2, so a tied user-typed document was
dropped every time — the exact memory a tool-free turn exists to surface.
Ties now fall to recency, then to input order, which keeps the
project-before-user precedence the concatenation already establishes.

Corpus: the case labeled `semantic-no-lexical` had no relevant documents,
so it was a no-result case wearing the wrong label and nothing measured the
cost of "no lexical match, no score". Relabel it and add three genuine
answerable-but-lexically-disjoint cases. Both scorers return nothing for
them, so the slice sits outside the quality floor and is asserted
separately: the fast path closes the timing gap, not the matching gap.
Tool-free delivery is 92.3%, not 100%, and the residual is that slice.

Telemetry: a tool-free turn logs its terminal event from the discard path,
which did not apply the fast-phase exclusion. A turn whose every selected
document had already been fast-delivered was recorded as
`no_safe_delivery_point`, inflating the "memory never reached the model"
bucket with turns that got it. Apply the same rule the ToolResult consume
point uses; a partial overlap still reports the cancellation reason.

* docs(memory): state the candidate-cap trade and the per-turn document count

Two review follow-ups, documentation only. No behaviour change.

"Removes the 200-document cap" oversold the candidate change. What it does
is swap a per-scope, query-blind recency truncation for a global,
query-aware one, and the effect is not a uniform widening: at or under 200
documents nothing was excluded by count under either design, but the new
25,000-byte manifest budget is a ceiling the old path lacked; between 200
and 400 with neither scope over 200 the old path sent every document and
the new one sends at most 200, so fewer reach the model; only a scope over
200 is the case the change is actually for. Record all three, plus the fact
that the manifest budget packs rather than prefixes.

MAX_RELEVANT_DOCS = 5 bounds one prompt, not one turn. A fast delivery of
two plus a refined delivery of five disjoint documents puts seven in front
of the model; dedupe removes repeats, not the sum. This follows from
dropping combined fast/refined budget accounting, which was a deliberate
choice, but the number was never written down next to the constant that
reads like a hard cap.

* fix(memory): end the initial recall wait on the fast result, widen tokenization

The 100 ms initial budget was a fixed cost, and the evidence for it measured
the wrong thing. Deterministic *scoring* is microseconds, but the fast result
is only published once recall has enumerated, read, and parsed the memory
tree — and this branch removed the 200-document cap for recall, so that scan
grows with the tree. recall-scan-latency.test.ts adds that measurement
against a real temporary tree: ~29 ms at 200 topics, ~70 ms at 500, ~130 ms
at 1000.

So for any tree small enough to scan in time — the ordinary case — the fast
result was in hand tens of milliseconds before the budget expired, and the
rest of the budget was spent waiting on a model selector this design already
assumes will miss it. The wait now ends on whichever comes first: recall
settling, the fast result being published, cancellation, or the ceiling. The
preference order is unchanged, because the code after the wait still prefers
a settled recall. Past roughly a thousand topics the scan alone exceeds the
ceiling and the turn pays the full budget for nothing; that is recorded as a
known limitation rather than fixed, since the fix is a persistent catalog.

Tokenization kept only [a-z0-9]{3,} runs, so Cyrillic, Greek, Arabic, and
accented Latin produced no tokens at all and the deterministic path was
unconditionally silent for them. Keep whole runs of non-CJK letters, marks,
and digits instead. CJK is excluded per character rather than by alternation
order: \p{L} also matches Han, so a Latin-initial run would otherwise swallow
the CJK after it and turn abc漢字 into one token. Scripts without word
separators outside the CJK set still collapse to one run, which is recorded
rather than claimed as segmentation.

Two smaller follow-ups. The active-tool alias set is now derived once per
recall instead of once per scanned document, which mattered little under the
old 200-document cap and more without it. And the eval prints the Recall@5 a
query-blind random scorer would score on this corpus (20%), with a test
holding that floor at or below 25%, because a small corpus flatters every
design and the headline was unreadable without it.

* docs(memory): correct the initial-turn preference claim, pin it with a test

Local end-to-end verification on QwenLM#8716 found the claim added in 01ef7d7 —
"the preference order is unchanged: whatever ends the wait, a settled recall
is still delivered in preference to the fast result" — to be false in the
case that matters. `onFastResult` is published before recall issues the
selector request at all, so the recall promise cannot be settled when the
wait ends on the fast result. Measured against a selector settling in 15 ms,
comfortably inside the ceiling, the initial turn still delivers the
deterministic pair and discards the model's picks.

The behaviour is right and stays: a model side query does not return inside a
100 ms ceiling in production, so arbitrating would spend the rest of the
budget on every turn to win a race that does not happen, and the selector's
judgement still lands at ToolResult with the fast documents excluded. What
was wrong was the description. State it directly instead — on the initial
turn, once the deterministic scorer matches, the fast result wins regardless
of selector latency — and pin it with a test that fails when the early exit
is removed, so it reads as a decision rather than an accident.

Two measurements corrected while here. The scan crossover is machine-
dependent, not a fixed topic count: the same three sizes measure 9/21/46 ms
on faster hardware against 29/70/130 ms on the machine the tables were
written from, so the ceiling is not reached there at all. And
MAX_MODEL_CANDIDATE_DOCS = 200 is rarely the binding constraint —
MAX_MODEL_MANIFEST_BYTES is, at roughly 90-150 documents once absolute paths
and timestamps are counted. Measured runs sent 94 and 96 manifest lines where
the document cap would have allowed 200, which also explains why the recency
reserve has to be interleaved rather than appended.

---------

Co-authored-by: yiliang114 <jinjing.zzj@gmail.com>
Co-authored-by: Shaojin Wen <shaojin.wensj@alibaba-inc.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

autofix/takeover Summon the autofix loop to manage this PR (remove to release; needs triage+)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants