feat(channels): support natural memory references - #6952
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: This is a feature extension, not a bug fix. The PR adds natural-language channel memory lookup, update, and removal planning — a logical next phase after the existing exact-ID channel memory (PRs #6051, #6376, #6617 in CHANGELOG). The user problem is real: requiring opaque IDs for every follow-up is cumbersome. Direction: Aligned. Channel memory is an established feature with a clear phase progression, and natural references are the obvious next step. CHANGELOG confirms the trajectory ( Size: Not applicable — no core infrastructure paths matched. Production logic: 458 lines (ChannelBase.ts +235/-32, channel-memory-intent.ts +10/-1, types.ts +17/-7, memory-intent-classifier.ts +139/-17). Test: 754 lines. Docs: 20 lines. All well within thresholds. Approach: Scope feels right for the stated goal. The design preserves exact-ID as the deterministic fast path and only invokes the classifier for non-exact requests. CAS protection via Re-run note: No new commits since the prior pass. Acknowledging @wenshao's human review — all six suggestions are non-blocking follow-up material. 中文说明感谢贡献! 模板完整 ✓ 问题:这是功能扩展,不是 bug 修复。PR 增加了自然语言的频道记忆查询、更新和删除规划——在现有精确 ID 频道记忆(CHANGELOG 中的 PR #6051、#6376、#6617)之后的合理下一步。用户痛点真实存在:每次后续操作都需要记住不透明 ID 很不方便。 方向:对齐。频道记忆是已有功能,有清晰的阶段演进,自然引用是显而易见的下一步。CHANGELOG 确认了发展轨迹(#6376 中的 规模:不适用——未触及核心基础设施路径。生产逻辑 458 行,测试 754 行,文档 20 行,均在阈值内。 方案:范围与目标匹配。设计保留了精确 ID 作为确定性快速路径,仅在非精确请求时调用分类器。通过 重跑说明: 自上次审查以来没有新提交。确认 @wenshao 的人工审查——六条建议均为非阻塞的后续改进项。 — Qwen Code · qwen3.7-max Reviewed at |
Code ReviewNo critical issues found. The implementation is clean and security-conscious. Independent proposal comparison: My approach would have been nearly identical — extend the classifier to accept current entries, return structured plans with Notable strengths:
Minor observations (non-blocking):
@wenshao's review alignment: Their six suggestions are all follow-up material — trigger broadening cost monitoring, manifest budget floor for extreme entry counts, unpaginated candidate lists, carrying previews in the resolved intent, CAS-conflict user messaging, and prompt wording for multi-target plans. None block the merge. Real-Scenario TestingChannel memory operations require a running messaging bot (Telegram, DingTalk, etc.) — this feature can't be exercised via Unit tests re-verified from the PR branch in the worktree (all passing): Key test scenarios covered:
中文说明代码审查未发现关键问题。实现干净且安全意识强。 独立方案对比: 我的方案几乎一致——扩展分类器接收当前条目,返回带 显著优点:
次要观察(非阻塞):
@wenshao 审查一致性: 其六条建议均为后续改进项——触发正则开销监控、极端条目数下的清单预算下限、未分页候选列表、在解析意图中携带预览、CAS 冲突用户提示、以及多目标规划的提示措辞。均不阻塞合并。 真实场景测试频道记忆操作需要运行中的消息机器人(Telegram、钉钉等)——此功能无法通过 已从 PR 分支在工作树中重新验证单元测试(全部通过): — Qwen Code · qwen3.7-max Reviewed at |
|
Confidence: 4/5 — solid implementation of a well-scoped feature in an established area; the only gap is live bot E2E which neither the author nor I could run. This PR extends channel memory with natural-language references — a clear next phase after the existing exact-ID operations. The implementation is careful in all the right places: CAS protection prevents stale classifier output from mutating the wrong entry, ambiguity is handled conservatively (return candidates, never mutate), and classifier output is treated as untrusted data with thorough revalidation. The exact-ID fast path is preserved and verified by tests to bypass the classifier entirely. The code is straightforward — the @wenshao's human review (LGTM with six non-blocking suggestions) corroborates the automated assessment. Their points about trigger broadening cost, manifest budget floor, and CAS-conflict user messaging are all good follow-up material. The one thing I'd flag: real E2E with a live messaging bot (Telegram, DingTalk) would give extra confidence, but this requires disposable bot credentials that neither the author nor the test environment has. The unit test coverage is thorough enough to ship. Approval already pinned to 中文说明信心度:4/5 — 在成熟领域中实现良好的功能扩展;唯一的差距是作者和我都无法运行实时机器人 E2E 测试。 此 PR 为频道记忆增加了自然语言引用——在现有精确 ID 操作之后的清晰下一阶段。实现在所有关键位置都很谨慎:CAS 保护防止过期分类器输出修改错误条目,歧义保守处理(返回候选项,永不修改),分类器输出被视为不可信数据并进行彻底重新验证。精确 ID 快速路径保持不变,测试验证其完全绕过分类器。 代码简洁—— @wenshao 的人工审查(LGTM 加六条非阻塞建议)印证了自动评估。其关于触发正则开销、清单预算下限和 CAS 冲突用户提示的观点都是好的后续改进项。 唯一要指出的:使用实时消息机器人(Telegram、钉钉)的真实 E2E 测试会增加信心,但这需要作者和测试环境都没有的可丢弃机器人凭证。单元测试覆盖足够全面可以发布。 批准已锚定在 — Qwen Code · qwen3.7-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not reviewed: coverage — could not read the agents' transcripts (no subagent transcripts at /home/github-runner/actions-runner-5/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-5--work-qwen-code-qwen-code/subagents/820bc888-48c8-444e-97bd-7eebb9e9fde1 (ENOENT: no such file or directory, scandir '/home/github-runner/actions-runner-5/_work/_temp/qwen-home/projects/-home-github-runner-actions-runner-5--work-qwen-code-qwen-code/subagents/820bc888-48c8-444e-97bd-7eebb9e9fde1'). The harness writes one per agent; if there are none, either no agents ran or the harness could not write them.), so this run cannot show that any of the diff was read.
— qwen3.7-max via Qwen Code /review
| if (intent.kind === 'ambiguous') { | ||
| const channelMemory = await this.getChannelMemory(envelope); | ||
| if (!channelMemory) return; | ||
| let entries: ChannelMemoryEntry[]; | ||
| try { | ||
| entries = await channelMemory.listChannelMemoryEntries( |
There was a problem hiding this comment.
[Suggestion] listChannelMemoryEntries is fetched again here, but classifyChannelMemoryIntent already fetched entries and resolved intent.ids against that snapshot. Re-fetching creates both redundant I/O and a TOCTOU gap: if entries change between the two fetches (e.g., concurrent memory mutation), intent.ids resolved against snapshot A may not match snapshot B — an entry could silently vanish from the rendered candidate list. — Concrete cost: every ambiguous/list_matches/inspect resolution triggers two reads of the memory store, doubling latency for high-latency backends.
Consider threading the already-fetched entries through the resolved intent (e.g., add an optional entries field to ambiguous, list_matches, and inspect variants) so the handler renders from the classifier's snapshot.
— qwen3.7-max via Qwen Code /review
ReviewOverviewThis extends channel memory with natural-language references: the classifier now receives the current entry manifest and can plan What I verified
Edge-case coverage is genuinely good (NaN/∞ confidence, duplicate/unknown IDs, unknown JSON fields, 500-entry manifest budget stress, lone-surrogate metadata, group-mention gating, planner-read failures). SuggestionsNothing blocking; in rough priority order:
One user-visible behavior change worth being aware of: VerdictWell-scoped, security-conscious implementation with strong tests; the suggestions above are all minor or follow-up material. LGTM. 🤖 Generated with Claude Code — Claude Fable 5 |
|
@qwen-code /triage |
What this PR does
Adds natural-language channel-memory lookup, update, and removal planning while preserving exact-ID commands as the deterministic fast path. Natural requests can return a filtered list, resolve one current entry, or return stateless candidate IDs when ambiguous. Classifier output is bounded, treated as untrusted, and revalidated before any memory operation.
Why it's needed
People can refer to a recent preference by its content or context, but requiring a remembered opaque ID for every follow-up is cumbersome. This change adds natural references without allowing ambiguous selections or stale planner output to mutate another entry.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Focused suites passed: core 64 tests, channel base 439 tests, and CLI 107 tests. Repository build, bundle, typecheck, lint, changed-file Prettier, and whitespace checks passed on macOS. An independent whole-branch review found one filtered-list routing gap; the TDD fix was re-reviewed with zero remaining Critical, Important, or Minor findings and a Ready to merge verdict.
Live Telegram/model E2E was not observed because no disposable
PHASE3B1_*credentials were already available to the verifier. No normal user credentials or unrelated daemon were inspected or used.Tested on
Environment (optional)
macOS, Node.js v25.9.0, package-local Vitest suites, repository build and static checks.
Risk & Scope
Linked Issues
N/A
中文说明
此 PR 的内容
增加自然语言的频道记忆查询、更新和删除规划,同时保留精确 ID 命令作为确定性快速路径。自然请求可以返回过滤后的列表、解析出一个当前条目,或者在存在歧义时返回无状态的候选 ID。分类器输出有长度限制,被视为不可信数据,并且在任何记忆操作前都会再次校验。
为什么需要
用户可能通过内容或上下文引用最近的偏好,但每次后续操作都要求记住不透明 ID 会很麻烦。此改动增加自然引用,同时不允许歧义选择或过期的规划输出修改其他条目。
审阅者测试计划
如何验证
证据(前后对比)
聚焦测试套件已通过:core 64 个测试、channel base 439 个测试、CLI 107 个测试。仓库构建、bundle、typecheck、lint、改动文件 Prettier 和空白检查已在 macOS 上通过。独立整分支审查发现了一个过滤列表路由缺口;TDD 修复经复审后 Critical、Important、Minor 均为 0,结论为 Ready to merge。
由于验证者没有已安全提供的可丢弃
PHASE3B1_*凭证,实时 Telegram/model E2E 未被观察。未检查或使用普通用户凭证或无关 daemon。测试平台
环境(可选)
macOS、Node.js v25.9.0、包级 Vitest 测试套件、仓库构建和静态检查。
风险与范围
关联 Issue
不适用