fix(core): skip abbreviations in multiple_sentences filter (#6077) - #6193
Merged
Conversation
The follow-up suggestion generator was falsely flagging suggestions containing common abbreviations like 'vs.' or 'Dr.' as multiple sentences, because the filter regex matched any period followed by a capitalized word. Replace the inline regex with a named MULTIPLE_SENTENCES_RE that uses a negative lookbehind to skip common honorifics (Mr, Mrs, Dr, Ms, Prof, Sr, Jr, St), abbreviations (vs, etc), and Latin shorthands (e.g., i.e.). Add vitest coverage for the issue's reported case, the common abbreviations, and an explicit multi-sentence case so the filter still catches real run-on suggestions. Fixes #6077
Collaborator
Author
E2E Report — Issue #6077SummaryFix the Reproduction (before fix)Standalone reproducer Vitest unit testsWith fix appliedWith fix temporarily reverted (inline regex restored)The new Compiled-module E2E (post-build)Imported True Static checks
Fix shapeSingle constant |
Collaborator
|
Re-triage after three rounds of review feedback and code changes. Template: complete ✓ Problem: real, observed bug — #6077 has debug-log evidence showing Direction: straightforward bug fix. The old inline regex Approach: the implementation has evolved significantly through three rounds of review. The original single-regex lookbehind approach was replaced with a per-boundary Moving on to code review. 🔍 中文说明在三轮 review 反馈和代码变更后重新审查。 模板：完整 ✓ 问题：真实、已观测到的 bug——#6077 有调试日志证据表明 方向：直接的 bug 修复。旧的内联正则 方案：实现经过三轮 review 已大幅演进。最初的单正则后顾断言方案被替换为逐边界检查的 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Contributor
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. |
Collaborator
Code ReviewThe implementation evolved through three review rounds from a single regex with negative lookbehind to a Key observations:
No correctness bugs, no security issues, no AGENTS.md violations. Real-Scenario TestingUnit Tests (vitest)Before/After Behavioral ComparisonBefore: 7 false positives (all abbreviation cases incorrectly filtered). After: 12/12 correct. 中文说明代码审查实现经过三轮 review 从一个带否定后顾断言的单正则演变为 关键观察：
无正确性 bug、无安全问题、无 AGENTS.md 违规。 真实场景测试单元测试（vitest）改动前/改动后行为对比改动前：7 个误判（所有缩写用例被错误过滤）。改动后：12/12 正确。 — Qwen Code · qwen3.7-max |
Collaborator
|
This PR has been through a thorough review cycle — three rounds of feedback, each addressed with substantive code changes. The implementation is materially better than the original submission: the per-boundary function catches cases the initial regex lookbehind couldn't (abbreviation-then-real-boundary), and the test suite now exercises every code path including the The The one known edge case (abbreviation exemption applying to All 20 unit tests pass. The before/after comparison is conclusive: 7 false positives eliminated, 0 regressions. Approving. ✅ 中文说明这个 PR 经历了完整的 review 周期——三轮反馈，每轮都有实质性的代码改进。实现比初始提交有质的提升：逐边界检查函数能捕获初始正则后顾断言无法处理的情况（缩写后跟真正的句子边界），测试套件现在覆盖了每条代码路径，包括
一个已知的边界情况（缩写豁免对 全部 20 个单元测试通过。改动前/改动后对比具有决定性：7 个误判消除，0 个回归。 批准。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jul 2, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
wenshao
requested changes
Jul 3, 2026
| * not mistaken for two sentences. | ||
| */ | ||
| const MULTIPLE_SENTENCES_RE = | ||
| /(?<!\b(?:vs|Mr|Mrs|Dr|Ms|Prof|Sr|Jr|St|etc|eg|ie|e\.g|i\.e))[.!?]\s+[A-Z]/; |
Collaborator
There was a problem hiding this comment.
[Critical] This lookbehind skips every sentence-boundary candidate that follows an allowlisted abbreviation, even when that abbreviation is the end of a real sentence. I verified getFilterReason('Review options etc. Then commit.') returns null on this branch, so a two-sentence suggestion can now be surfaced instead of being rejected as multiple_sentences.
Please add regression coverage for terminal-abbreviation cases and avoid a blanket left-token exemption. For example, scan [.!?]\\s+[A-Z] candidates and skip only known-safe continuations (Dr. Smith, St. Petersburg, vs. Wildflowers, including case variants like E.g. Docker), while still rejecting Review options etc. Then commit..
— GPT-5 via Qwen Code /review
…check (#6077) The negative lookbehind in MULTIPLE_SENTENCES_RE exempted every abbreviation (including etc, eg, ie) from sentence-boundary detection, so 'Review options etc. Then commit.' incorrectly passed the filter. Replace the regex with hasSentenceBoundary(), which checks each potential boundary individually: honorifics (Dr., Mr., etc.) and vs. are recognized as safe continuations, while etc. at the end of a real sentence is still correctly detected. Latin shorthands (e.g., i.e.) are handled by checking the two-part pattern before the period.
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 1/3). What changed, and what I pushed back on: Address Summary — PR #6193 (issue #6077)Conflict resolutionCONFLICT_WITH_BASE: false — no merge needed. Feedback triage1.
|
qwen-code-ci-bot
requested changes
Jul 4, 2026
| 'no', | ||
| ]); | ||
|
|
||
| const ABBREV_HONORIFICS = new Set([ |
Collaborator
There was a problem hiding this comment.
[Critical] etc is missing from this set, despite the PR description listing etc. as one of the abbreviations being fixed. The companion test 'Review the etc. items' passes vacuously — items is lowercase, so /[.!?]\s+[A-Z]/ never matches. But etc. followed by a capitalized word (e.g., "Check etc. Tasks remaining") is still falsely filtered as multiple_sentences.
Also, the name ABBREV_HONORIFICS is misleading — vs and St are not honorifics. Consider renaming to KNOWN_ABBREVIATIONS.
Suggested change
| const ABBREV_HONORIFICS = new Set([ | |
| const KNOWN_ABBREVIATIONS = new Set([ | |
| 'Mr', | |
| 'Mrs', | |
| 'Dr', | |
| 'Ms', | |
| 'Prof', | |
| 'Sr', | |
| 'Jr', | |
| 'St', | |
| 'vs', | |
| 'etc', | |
| ]); |
— qwen3.7-max via Qwen Code /review
| expect(shouldFilterSuggestion('Visit St. Petersburg office')).toBe(false); | ||
| expect(shouldFilterSuggestion('Review the etc. items')).toBe(false); | ||
| // Latin shorthands with an internal period | ||
| expect(shouldFilterSuggestion('Use e.g. Docker to build')).toBe(false); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The i.e. code path (word === 'e' && /i\.e$/.test(before)) is implemented but has no test. If a refactor breaks the i.e. branch, no test would catch it.
Suggested change
| expect(shouldFilterSuggestion('Use e.g. Docker to build')).toBe(false); | |
| expect(shouldFilterSuggestion('Use e.g. Docker to build')).toBe(false); | |
| expect(shouldFilterSuggestion('Use i.e. Docker to build')).toBe(false); |
— qwen3.7-max via Qwen Code /review
| @@ -157,6 +157,30 @@ describe('shouldFilterSuggestion', () => { | |||
|
|
|||
| it('filters multiple sentences', () => { | |||
| expect(shouldFilterSuggestion('Run the tests. Then commit.')).toBe(true); | |||
Collaborator
There was a problem hiding this comment.
[Suggestion] No test exercises the continue-then-detect iteration in hasSentenceBoundary — the core new control flow where an abbreviation match is skipped and a subsequent non-abbreviation match triggers the filter. A regression (e.g., continue changed to return false) would pass all existing tests silently.
Add a test combining a skipped abbreviation with a real sentence boundary:
Suggested change
| expect(shouldFilterSuggestion('Run the tests. Then commit.')).toBe(true); | |
| expect(shouldFilterSuggestion('Run the tests. Then commit.')).toBe(true); | |
| expect(shouldFilterSuggestion('Hello! How are you?')).toBe(true); | |
| expect(shouldFilterSuggestion('Do this. Then do that.')).toBe(true); | |
| expect(shouldFilterSuggestion('Review options etc. Then commit.')).toBe( | |
| true, | |
| ); | |
| expect( | |
| shouldFilterSuggestion('Check Dr. Smith. Then commit.'), | |
| ).toBe(true); |
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 2/3). What changed, and what I pushed back on: Address Summary — PR #6193 (issue #6077)Feedback triage1. [Critical]
|
| if (!wordMatch) return true; | ||
| const word = wordMatch[1]; | ||
| if (KNOWN_ABBREVIATIONS.has(word)) continue; | ||
| if ( |
Collaborator
There was a problem hiding this comment.
[Suggestion] The e.g./i.e. check is case-sensitive — word === 'g' and /e\.g$/ are lowercase-only. Capitalized variants like E.g. and I.e. (valid in many style guides) would be incorrectly filtered. For example, "Consider E.g. Docker for this" would be dropped as multiple_sentences.
Suggested change
| if ( | |
| if ( | |
| (word === 'g' && /e\.g$/i.test(before)) || | |
| (word === 'e' && /i\.e$/i.test(before)) | |
| ) |
— qwen3.7-max via Qwen Code /review
| const i = m.index!; | ||
| const before = suggestion.slice(0, i); | ||
| const wordMatch = before.match(/(\w+)$/); | ||
| if (!wordMatch) return true; |
Collaborator
There was a problem hiding this comment.
[Suggestion] The if (!wordMatch) return true; branch — which fires when the text before the punctuation ends with a non-word character (e.g., closing paren, quote mark) — has no test coverage. An input like "Run (see docs). Then deploy" would exercise this path and guard against future regressions.
— qwen3.7-max via Qwen Code /review
- Make e.g./i.e. abbreviation check case-insensitive to handle capitalized variants (E.g., I.e.) - Add test for non-word-char-before-punctuation branch in hasSentenceBoundary
Collaborator
Author
|
🤖 Addressed the latest review feedback (round 3/3). What changed, and what I pushed back on: Address Summary — PR #6193 (issue #6077)Merge conflictNone. CONFLICT_WITH_BASE=false; branch merges cleanly with Inline comment:
|
| function hasSentenceBoundary(suggestion: string): boolean { | ||
| for (const m of suggestion.matchAll(SENTENCE_BOUNDARY_RE)) { | ||
| const i = m.index!; | ||
| const before = suggestion.slice(0, i); |
Collaborator
There was a problem hiding this comment.
[Suggestion] The abbreviation exemption is applied to all three punctuation types in SENTENCE_BOUNDARY_RE ([.!?]), but abbreviations only end with periods. A string like "Wow Dr! That's great" would match on ! T, find Dr in KNOWN_ABBREVIATIONS, and incorrectly shield the exclamation boundary.
Suggested change
| const before = suggestion.slice(0, i); | |
| function hasSentenceBoundary(suggestion: string): boolean { | |
| for (const m of suggestion.matchAll(SENTENCE_BOUNDARY_RE)) { | |
| const i = m.index!; | |
| if (suggestion[i] !== '.') return true; | |
| const before = suggestion.slice(0, i); |
— qwen3.7-max via Qwen Code /review
wenshao
approved these changes
Jul 4, 2026
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 4, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this PR does
The
multiple_sentencesfilter in the follow-up suggestion generator no longer treats the period after a common abbreviation (vs., Dr., Mr., e.g., etc.) as a sentence boundary. The filter still rejects real run-on suggestions — it only stops tripping over cases where the period is part of an abbreviation followed by a capitalized word.Why it's needed
Follow-up suggestions containing abbreviations were being silently dropped. The reported case was
"Let's start with the Weeds vs. Wildflowers audit.", which the LLM produced as a perfectly reasonable next-step hint, but the old inline regex/[.!?]\s+[A-Z]/matched the period aftervsand classified it asmultiple_sentences. The same false-positive applies to honorifics (Dr. Smith,Mr. Jones,Prof. Lee), common abbreviations (St.,etc.), and Latin shorthands (e.g.,i.e.). Surfacing these suggestions is a user-visible improvement — the model is already generating them, they were just being swallowed by the filter.Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/followup/suggestionGenerator.test.ts. Expect 20/20 passing, including the newdoes not filter abbreviations as multiple sentencestest coveringvs.,Dr.,Mr.,Ms.,Prof.,St.,etc., ande.g..filters multiple sentencestest now also coversHello! How are you?andDo this. Then do that..Evidence (Before & After)
N/A — the change is in the suggestion filter logic, not a user-visible TUI surface.
Before (old inline regex):
After (new
MULTIPLE_SENTENCES_RE):Tested on
Environment (optional)
Linux CI sandbox, Node.js 22.
npm run build && npm run bundle && npm run typecheck && npm run lintall green; full compiled-module E2E (importingshouldFilterSuggestionfrompackages/core/dist/src/followup/suggestionGenerator.js) confirms all six behavioral cases.Risk & Scope
Capt.,Ltd.) appears, the filter will still false-positive; adding new entries is a one-line change to the lookbehind alternation.Linked Issues
Fixes #6077
中文说明
此 PR 做了什么
在后续建议生成器中，
multiple_sentences过滤器不再将常见缩写（如vs.、Dr.、Mr.、e.g.、etc.）后面的句号视为句子边界。该过滤器仍然会拒绝真正的连句建议，只是不再误判缩写后跟大写单词的情况。为什么需要
包含缩写的后续建议此前会被静默丢弃。报告中的典型案例是
"Let's start with the Weeds vs. Wildflowers audit."，模型已生成了一个非常合理的下一步提示，但旧的内联正则/[.!?]\s+[A-Z]/会匹配vs后的句号并将其归类为multiple_sentences。同样的误判也出现在称谓（Dr. Smith、Mr. Jones、Prof. Lee）、常见缩写（St.、etc.）以及拉丁语简写（e.g.、i.e.）上。让这些建议能够正常展示，是一个用户可感知的改进——模型本来就在生成它们，只是被过滤器吞掉了。评审测试计划
如何验证
cd packages/core && npx vitest run src/followup/suggestionGenerator.test.ts。期望 20/20 通过，包括新增的does not filter abbreviations as multiple sentences测试，覆盖vs.、Dr.、Mr.、Ms.、Prof.、St.、etc.和e.g.。filters multiple sentences测试现在也覆盖了Hello! How are you?和Do this. Then do that.。证据（改动前后对比）
N/A —— 变更位于建议过滤逻辑中，并非用户可见的 TUI 界面。
改动前（旧的内联正则）：
改动后（新的
MULTIPLE_SENTENCES_RE）：测试环境
环境（可选）
Linux CI 沙箱，Node.js 22。
npm run build && npm run bundle && npm run typecheck && npm run lint全部通过；完整的编译后模块 E2E（从packages/core/dist/src/followup/suggestionGenerator.js导入shouldFilterSuggestion）确认全部六个行为用例通过。风险与范围
Capt.、Ltd.），过滤器仍会误判；增加新条目只需修改 lookbehind 分支的一行代码。关联 Issue
Fixes #6077