fix(core): Normalize both sides of the session title echo comparison - #9809
Conversation
|
Thanks for the PR! Template looks good ✓ Problem: real but latent. Verified in Direction: aligned — closes a known hole in a just-merged guard, introduces no new scope, and changes no public contract (the helper is exported for tests following the existing Size: touches core paths ( Approach: minimal and exactly right — extract the existing normalization into Risk: no elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:真实存在但属潜在缺陷。已在 方向:对齐——补上刚合并守卫的已知漏洞,不引入新范围,不改变公共契约(辅助函数为测试而导出,沿用 规模:触及核心路径( 方案:最小且恰好正确——把既有归一化抽成 风险:无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewIndependent baseline first: given "the echo guard fully normalizes the candidate but only lowercases the examples", the minimal fix is to run both sides through one shared normalization and lock it in with a punctuated-example test. That is exactly what this PR does — no simpler path was missed. Verified by reading (this is an unattended run; PR code is never executed here):
The tests pin the change: revert the diff and the suite no longer compiles (it imports the helper), and the new cases cover ASCII punctuation, a quoted wrapper, and full-width Testing evidenceEvidence carried: the PR's own CI on the reviewed commit, fetched via the API — plus the static verification above. The macOS/Windows/integration jobs are
Nothing user-visible to drive in a real scenario — the change is provably inert on the current example set. 中文说明代码审查 先给独立基线:已知"回显守卫对候选标题做完整归一化、对示例只做小写化",最小修复就是让两侧共用同一套归一化,并用带标点的示例测试锁定。本 PR 正是这么做的——没有更简的路径被遗漏。 以静态阅读核实(无人值守运行,此处从不执行 PR 代码):
测试锁定了变更:回退 diff 后套件无法编译(它导入该辅助函数);新用例覆盖 ASCII 标点、引号包裹、全角 测试证据 本节证据为:经审查提交上 PR 自身的 CI(经 API 获取),加上上述静态核实。macOS/Windows/集成任务按设计仅在合并队列( — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — clean across every stage; the diff does exactly one thing, does it correctly, and cannot change behavior on the current example set. Reflection: my independent proposal for this problem was exactly what the PR does — pull the existing normalization out of the candidate branch, run both sides of the comparison through it, and add a punctuated-example test. Nothing simpler would close the hole. I verified the load-bearing fact myself rather than taking the PR's word: all four prompt examples start and end with letters, so the example-side strip is a no-op today — behavior is provably unchanged, zero regression surface. What the PR removes is a future silent bypass, the exact class of regression the guard introduced in #9709 exists to prevent. The scope is the minimal set: one helper, one call site, doc comments updated to match, and a test suite that cannot even compile without the change. One item remains open — CI: the ubuntu gate (lint + unit suite) was still running at the time of writing; everything already completed is green (desktop shell builds, dependency audit, secret scan), and the macOS/Windows/integration skips are by design (merge-queue only). Approval is therefore deferred until CI lands green on 中文说明置信度: 5/5 —— 各阶段均干净;diff 只做一件事,做得正确,且不可能改变当前示例集合下的行为。 回顾:我对这个问题的独立方案与 PR 完全一致——把既有归一化从候选分支中抽出,让比较两侧都走它,再补一个带标点示例的测试。没有更简的方案能补上这个洞。关键事实是我自己核实的,而非采信 PR 描述:四个提示词示例首尾均为字母,因此示例侧新增的剥离今天是空操作——行为可证明不变,回归面为零。PR 消除的是未来的静默绕过,正是 #9709 引入守卫要防的那类回归。范围恰为最小集:一个辅助函数、一个调用点、文档注释同步更新,外加一套缺少该变更便无法编译的测试。 唯一未决项是 CI:撰写时 ubuntu 关卡(lint + 单元测试)仍在运行;已完成的均为绿色(桌面壳构建、依赖审计、密钥扫描),macOS/Windows/集成跳过为设计使然(仅合并队列运行)。因此批准推迟到 CI 在 — Qwen Code · qwen3.8-max Reviewed at |
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Checked: normalizeForEchoCompare extracts all four transformations (trim → lowercase → strip leading non-alphanum → strip trailing non-alphanum) into a single helper and runs both sides of the echo comparison through it. Symmetry is now structural, not contingent on examples staying clean. Pre-existing sanitizeTitle behavior is unchanged. Export follows the sanitizeTitle precedent.
Tests: new normalizeForEchoCompare suite covers edge-punct equality (including full-width !), interior-punct non-equality, and the (WIP) Fix build case that exercises the bracket-stripping path. All 32 tests pass per the PR description.
Cross-checked against existing reviews: no prior comments on this PR.
CI: Desktop Shell (ubuntu + windows) pass. Test (ubuntu-latest, Node 22.x) was pending at review time — unit-only change, no behavioral risk on the running check.
Reviewed with AI assistance.
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
|
Released in v0.22.2. |
What this PR does
The session-title echo guard compares a model-generated candidate against the prompt's own example titles, but the two sides went through different normalization: the candidate was trimmed, lowercased, and stripped of leading/trailing non-letter/non-digit runs, while each example only got lowercased. This PR extracts that normalization into a single helper and runs both sides of the comparison through it, so the guard is symmetric by construction. A unit test locks in the symmetry, including an example with edge punctuation comparing equal to its stripped echo.
Why it's needed
The asymmetry is unreachable today because all four current example titles start and end with letters, but it becomes a silent bypass the moment an example with leading/trailing punctuation is added: the candidate's trailing punctuation is stripped during sanitization while the example side keeps it, the exact comparison misses, and the canned echo is accepted as a genuine title — the very regression the guard exists to prevent. Normalizing both sides with the same function removes the whole class instead of relying on future examples happening to be clean.
Reviewer Test Plan
How to verify
Run
cd packages/core && npx vitest run src/services/sessionTitle.test.ts— 32/32 pass, including the newnormalizeForEchoComparesuite: an example likeFix CI!now normalizes identically to the sanitized echoFix CI, while interior punctuation is preserved so distinct titles stay distinct. All pre-existing echo-guard tests (verbatim, case-variant, and wrapper-decorated echoes rejected; genuine titles like(WIP) Fix buildaccepted) still pass unchanged, confirming behavior on the current example set is identical.Evidence (Before & After)
N/A — internal normalization change with no user-visible behavior difference on the current example set.
Tested on
Environment (optional)
Unit tests only (
npx vitest runfrompackages/core); pluseslint,prettier --check, andtsc --noEmitclean on the changed files.Risk & Scope
sanitizeTitleprecedent.Linked Issues
Fixes #9772
Follow-up to the non-blocking observation by @wenshao on #9709 (echo guard introduced there for #9706).
中文说明
本 PR 做了什么
会话标题回显守卫会把模型生成的候选标题与提示词自带的示例标题做比较,但两侧走的归一化并不一致:候选标题会经过 trim、小写化、剥离首尾连续的非字母/非数字字符,而每个示例只做小写化。本 PR 把这套归一化抽成一个单独的辅助函数,让比较的两侧都走同一套逻辑,使守卫在结构上保持对称。并补充单元测试锁定这一对称性,包括带边缘标点的示例与其被剥离后的回显归一化结果相等。
为什么需要
当前四个示例标题首尾都是字母,所以这个不对称今天不可达;但一旦加入带首尾标点的示例就会变成静默绕过:候选标题的尾部标点在 sanitize 阶段被剥掉,示例侧却保留着,精确比较失配,罐头回显就会被当成真实标题接受——这正是守卫要防的回归。两侧走同一套归一化可以一次消除整类问题,而不是依赖将来的示例恰好都是干净的。
评审者测试计划
如何验证
运行
cd packages/core && npx vitest run src/services/sessionTitle.test.ts——32/32 通过,包含新增的normalizeForEchoCompare套件:Fix CI!这样的示例现在与被 sanitize 后的回显Fix CI归一化结果一致,同时内部标点被保留、不同标题仍可区分。所有既有回显守卫测试(原样/大小写变体/包裹符装饰的回显被拒,(WIP) Fix build等真实标题被接受)均不变通过,证明对当前示例集合的行为完全一致。前后对比证据
N/A——内部归一化变更,对当前示例集合无用户可见行为差异。
测试环境
环境说明(可选)
仅单元测试(
packages/core下npx vitest run);另对改动文件跑了eslint、prettier --check、tsc --noEmit,均干净。风险与范围
sanitizeTitle先例导出供测试使用。关联 Issue
Fixes #9772
对应 @wenshao 在 #9709 上提出的不阻塞观察(回显守卫即在该 PR 中为 #9706 引入)。