Repository navigation
fix(core): match one sed character for . and bracket expressions - #15701
2SinceJuly wants to merge 13 commits into
Conversation
The simulated sed path compiled patterns without the s and u flags, so `.` did not match `\r` in CRLF files and astral characters were matched as two UTF-16 code units, leaving a lone surrogate that was written as U+FFFD. Compile with su/gsu and escape a literal ] outside bracket expressions so the stricter u mode still accepts patterns such as [\[\]]. Closes QwenLM#15675
|
Verdict: merge-ready — 77/77 scripted assertions passed at head 中文摘要
Central claimThe simulated The oracle here is not hand-written expectations — it is the real A/B result
The cells are byte comparisons, not visual checks. Examples from the run (base → real
Both arms were bundled separately from their own worktree; the base bundle contains no Mutation matrix
The new test is not vacuous: reverting the whole apply-side change (M2) turns it red with the assertion it was written for, and each half of the bundled Survivors were adjudicated by building the mutant, not by reading it:
FindingsS1 — a zero-width global match over an astral character follows BSD, not GNU. S2 — nothing pins the gate's flags to the substitution's flags (M1). A one-line test would close it: S3 — the Observation (not a finding): head declines two patterns base simulated — CorrectionsNone. There was no earlier verification round or bot review on this PR describing the code, so there is nothing to correct; the two author claims I tested (the Not covered
MethodologyTwo detached worktrees, one at |
|
@qwen-code /triage |
|
Head is |
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6b": packages/core/src/tools/shell.test.ts sed-integration tests were not run — the file needs packages/core/dist/index.js , and this worktree is unbuilt; the uni…; "agent 6c": did not run the full packages/core suite (only the parser file plus targeted greps of shell.test.ts ), and did not run the mutation experiment by deleting th….
Test Plan (not a blocker): tests: 52 passed — this review observed 18646, 2035, 38776, 2278, 306, 13338 passed.
中文说明
未探索到全部深度(达到工具调用预算):"agent 6b":packages/core/src/tools/shell.test.ts sed-integration tests were not run — the file needs packages/core/dist/index.js , and this worktree is unbuilt; the uni…;"agent 6c":did not run the full packages/core suite (only the parser file plus targeted greps of shell.test.ts ), and did not run the mutation experiment by deleting th…。
Test Plan(非阻断):tests: 52 passed — this review observed 18646, 2035, 38776, 2278, 306, 13338 passed。
— deepseek-v4.1-flash-1017 via Qwen Code /review (v0.26.2)
|
Verdict: blocked — 289/297 scripted assertions passed at head 中文摘要
Follow-up round: previous-finding statusRound 1 (2026-10-27, head Correction to round 1 (my own earlier report): its merge-ready verdict is superseded. The r1 corpus (59 cells, groups A–E) did not include the u-only acceptance class — patterns that compile under
Central claim + A/BThe simulated
"Wrong" counts a cell against both oracles unless the cell is marked as deliberately following one (B5 follows BSD; A6/D4/D12–D14 follow GNU — all pre-existing choices, identical in both arms). Of the 5 head-wrong cells, F1/F2/F3/F8 are the PR regression and F5 is pre-existing (both arms wrong). All six issue rows flip base-wrong → head-match on both oracles. The corpus is a reconstruction: r1's harness no longer exists locally, so groups A–E were rebuilt from r1's group descriptions, the issue's six rows verbatim, and existing-test shapes (r1's A/B counts 5/6 and 8/10 reproduce exactly; E is 4/4 here vs r1's 3/4 because this round's fourth spelling is R1-1 regression (Critical, blocking)Under Measured bytes (input
Against BSD (the issue's own platform) every row is a clean match→wrong regression. Against GNU, F1 was already wrong at base (base simulated a pattern GNU rejects — pre-existing), and head changes the wrong bytes to a worse shape. BRE is safe: the translator emits Blast radius: every Reproduce: The proposed fix is measured, not eyeballed. Widening the guard to
The fix should ship with those assertions; the author's promised commit has not landed at this head. Mutation matrix12 rows, head's 52-test suite via JSON reporter, fresh scratch copy per row, every anchor asserted unique, kills attributed to named failing tests. Runner positive control: C1 (replaceAll forced false) kills with 15 failures; unmutated baseline 52/52.
The new test is not vacuous: transplanted onto base source it fails exactly once, on its first astral assertion ( Trial merge and sibling PRs
Findings
Not covered
MethodologyLocal maintainer round on macOS (Node v22.23.1, GNU sed 4.10 at Verification round 2 by @wenshao's local |
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Test Plan (not a blocker): tests: 52 passed — this review observed 19060, 2067, 39336, 2340, 306, 13620 passed.
Deferred under the convergence posture (round 2, not a blocker) — recorded, not requested in this round:
packages/core/src/utils/sedEditParser.test.ts:264 (+1 locations) — [review] the new s/x*/-/g assertion pins a result GNU sed does not produce for zero-width global matches over an astral character
中文说明
Test Plan(非阻断):tests: 52 passed — this review observed 19060, 2067, 39336, 2340, 306, 13620 passed。
收敛姿态下延后(第 2 轮,非阻断)——已记录,本轮不要求修改:共 1 条(原文未翻译,列表见上方英文部分)。
— deepseek-v4.1-flash-1017 via Qwen Code /review (v0.26.3)
|
Two Criticals at R1-1 (still standing). Confirmed on R1-2. Taking the decline assertions as written, R2-1 (new, accepted). Confirmed at Both fixes and their assertions go in one commit on top of |
`\p{…}`, `\P{…}` and `\u{…}` are literals to sed but Unicode property and
code-point escapes to a `u`-flagged regex, so the simulator accepted patterns
real sed rejects or leaves alone and wrote the simulated bytes to disk. Widen
the divergent-escape guard and pin the declines, plus the `u`-only rejection
class the flag introduced.
On a CRLF file, `s/.$//` consumes the trailing carriage return, but the write path restores CRLF, so the edit reported success while the bytes on disk were unchanged. Compare the prepared write content against the original instead of the parser's intermediate string.
Head branch was pushed to by a user without write access
|
All three are fixed. R1-1 — R1-2 — same commit. Added R2-1 — Evidence: |
qwen-code-review-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): "agent 6c": I did not run the two touched test files ( npx vitest run for shell.test.ts / sedEditParser.test.ts ), so the new assertions are reviewed by inspection only…; "agent 3a": none — no check was cut short..
Convergence: round 3 posted 4 inline comment(s), 4 of them reported for the first time; the previous round posted 3 (1 new). Findings keep coming back to the same files: packages/core/src/utils/sedEditParser.ts (findings in rounds 1, 2; 1 more now). The rate of new findings is not falling. A cluster that keeps producing siblings usually means the fixes are treating instances of a shared root cause — triaging that cause before the next round, or splitting an independent cluster into its own pull request, tends to end the loop faster than fixing them one at a time. Batching the remaining fixes and verifying them before the next push, or dropping this PR's reviews to --severity-floor critical, keeps the loop from re-deriving the same set. (Observation only — nothing was withheld from this review because of this observation.)
中文说明
未探索到全部深度(达到工具调用预算):"agent 6c":I did not run the two touched test files ( npx vitest run for shell.test.ts / sedEditParser.test.ts ), so the new assertions are reviewed by inspection only…;"agent 3a":none — no check was cut short.。
收敛情况:第 3 轮发布了 4 条行内评论,其中 4 条是首次提出;上一轮发布了 3 条(其中 1 条首次提出)。发现反复回到同一批文件:packages/core/src/utils/sedEditParser.ts(第 1、2 轮已出过发现,本轮又有 1 条)。新发现的产出速度没有下降。一个不断再生兄弟发现的簇,通常意味着逐条修复只在处理同一根因的实例——先定位并处理该根因,或把独立的簇拆成单独的 PR,通常比逐条修复更快结束循环。把剩余修复攒成一批、验证后再推送,或将本 PR 的评审降到 --severity-floor critical,可以避免循环反复推导同一组发现。(仅为观察——本轮评审未因此扣留任何内容。)
— deepseek-v4.1-flash-1017 via Qwen Code /review (v0.26.3)
|
Verdict: findings — 62/62 scripted assertions passed at head 中文摘要
Previous-finding statusRound 2 (2026-10-28, head
The bot's three corrected rulings (R1-1, R1-2, R2-1 "fixed by …") are accurate: each is confirmed by that fix's own witness going red when the fix is reverted. Central claim + A/BClaim unchanged: the simulated
Derived from those rows: head differs from base on four parser cells (A1, B1, F1, L1), all in the fixed direction; the shell matrix moves S2 into the fixed column and changes S7's report only. Mutation matrixRunner: the author's suites through a throwaway copy of the head tree, baseline green (53/53, 375/375) before any row, each anchor asserted unique, each revert replayed as that commit's production diff. The coverage claim is per-commit:
Both survivors were adjudicated by running their builds. Removing Correction to round 2 (my own earlier report)Round 2 said the widened guard left "0 of 59 non-F cells" with collateral damage, reasoning from FindingsR3-1 — Critical, blocking, reproduced. On a UTF-8-BOM CRLF file the read returns BOM-stripped R3-3 — Suggestion, reproduced in both directions. R3-2 — Suggestion, reproduced. The guard assumes R3-4 — Suggestion. Every positive-write assertion in the N-1 — note on the write path (pre-existing, not a regression of this PR). A CRLF→LF conversion simulated in-process never reaches the file: the write path restores CRLF ( Pre-existing, unchanged at every arm: B5 zero-width global over an astral character follows BSD (round 1's S1, still standing). Not covered
MethodologyLocal maintainer round: three arm sources exported with Verification round 3 by @wenshao's local |
The write-back prediction decoded the prepared bytes as UTF-8 and compared them with the read content, so a file carrying a BOM or a non-UTF-8 encoding never matched and the false `sed edit applied` this PR removes survived. Compare content strings instead, and only predict the CRLF normalization when the writer applies it: a client-side write stores the content verbatim, so `s/\r$//` is a real edit there and must not be suppressed.
`\0` is NUL to a `u`-flagged regex but a literal `0` to sed, so the simulator found no match and answered `sed edit made no changes` instead of falling back to the real binary.
|
Round 3 is addressed in two commits on top of R3-1 (Critical — BOM / non-UTF-8 made the comparison false). The guard no longer decodes the prepared bytes. It compares content strings against the content the read returned, and applies the CRLF normalization only when the active writer applies it, so a BOM or a non-UTF-8 encoding can no longer make an unchanged file look changed. R3-2 (a client-side write never restores CRLF). Added R3-3 ( R3-4 (only the skip branch was pinned).
Resolving the threads. @qwen-code /review |
|
Verdict: findings — 36/39 scripted assertions passed at head 中文摘要
Previous-finding statusRound 3 (2026-10-29, head
Central claim + A/BClaim unchanged: the simulated
The endpoint cells print in FindingsR4-1 — Critical, blocking, reproduced. Suggested fix: a capability flag says the client can write, not how it writes; the ACP service cannot distinguish a verbatim IDE from the in-repo daemon. Keep the conservative prediction (normalize) unless the writer proves verbatim — the r3 column of R4-2 — Suggestion, measured. R4-3 — Suggestion, measured. Dropping CorrectionsMy first control mutation ( Not covered
MethodologyLocal maintainer round, three detached worktrees at base / round-3 head / head; each arm runs the same probe file ( Verification round 4 by @wenshao's local |
|
Round 4 is addressed in R4-1 (Critical — a write capability cannot say how the client stores the content). I removed R4-2 ( R4-3 (Windows Verification at 中文说明第四轮已在
@qwen-code /review |
|
Verdict: merge-ready — 215/215 scripted assertions passed at head 中文摘要
Previous-finding statusRound 4 (2026-10-29, head
Central claim + A/BClaim unchanged: the simulated
The six rows from issue #15675, verbatim:
The Reviewer Test Plan's three bracket patterns ( Endpoint matrix (the round-5 delta)The changed predicate is judged at the endpoint, not by reading it. Real
Two things this fixes and one it costs. Fixed: the r4 head's Mutation matrixRunner: the author's two core suites through the head arm, baseline green before any row, each anchor asserted unique, each revert replayed and the tree confirmed clean.
Baseline before any row: 430/430 (53 sed parser + 377 shell). Every restore is byte-identical to the original; FindingsF1 — Suggestion, measured: the F2 — Suggestion, carried from R4-3: the Windows F3 — Suggestion, accepted trade-off: a verbatim ACP writer loses line-ending-only edits. Measured in No blocking finding. The false-success class that R2-1/R3-1/R4-1 chased is closed on every writer this round can reach. CorrectionsNone to an earlier round. Two oracle notes for readers comparing rounds: this container's GNU sed is 4.9 (rounds 3–4 used 4.5/4.10), and on 4.9 Not covered
MethodologyLocal maintainer round in a credential-free Verification round 5 by @wenshao's local |
|
Qwen Code review did not complete successfully. Qwen review exited with status 1. A transient error is retried automatically; if you are seeing this, retry with |
doudouOUC
left a comment
There was a problem hiding this comment.
EN
Thanks — I checked the central claim against a real sed instead of the new expectations: the head source of
sedEditParser.ts was driven through parseSedEditCommand + applySedSubstitution and the same expression was
run through /usr/bin/sed -i on the same bytes, over 54 targeted cells (.,.*,.\{1\} on astral and CRLF
input, [^a-z], [😀], [😀-😁], [^😀-😁], a], [\\], zero-width x*, \(.\)\1) and 649 randomised
ones. Head matches the real binary in all 18 cells where the base build was wrong, and nothing in the diff
regressed a cell that base got right. The ] escaping, the hasPosixBracketExpression(sedInfo.pattern) input
change at packages/core/src/utils/sedEditParser.ts:182, and the CRLF write-side guard at
packages/core/src/tools/shell.ts:1951 all hold up. I also tried to build a script this path would accept that
writes outside the named file (s///w, s///e, w file;…, two -e, s///p, -i.bak, pipelines and
redirects): every one is declined, so the write surface looks intact.
Two non-blocking notes:
packages/core/src/utils/sedEditParser.ts:204-206: feeding the raw pattern into this guard is the right fix,
but the regex only matches a POSIX class that closes the bracket expression itself. Negated and mixed forms
still pass and are then simulated wrongly rather than declined —s/[^[:space:]]/X/gonz]xwritesXx
where sed writesXXX,s/[[=a=]]/X/gona]writesXwhere sed writesX], and
s/[[:alpha:]abc]/X/gonaabc]writesXwhere sed writesXXXX]. Both arms of the diff do this, so it
is pre-existing rather than a regression, but it is the same "decline rather than guess" policy the comment at
:215-224states, and declining anything containing[:,[.or[=would close it.packages/core/src/utils/sedEditParser.ts:520: withu, a multibyte backreference now matches
(s/\(.\)\1/X/gon😀😀xwritesXx, while BSD sed leaves the file alone). The same split already
existed forééat base, so this is consistent with the GNU-oriented stance — just not covered by the
GNU/BSD note atsedEditParser.test.ts:278-279, which documents only thex*case.
中文
感谢——我没有只依赖新增断言,而是用真实 sed 复核了核心结论:把 head 的 sedEditParser.ts 原样驱动
parseSedEditCommand + applySedSubstitution,再用同一条表达式通过 /usr/bin/sed -i 处理同样的字节,跑了 54 个
定向用例(星平面与 CRLF 输入上的 ./.*/.\{1\}、[^a-z]、[😀]、[😀-😁]、[^😀-😁]、a]、[\\]、零宽
x*、\(.\)\1)和 649 个随机用例。base 答错的 18 格 head 全部与真实 sed 一致,且没有任何 base 答对的用例被本次
diff 改坏。] 转义、packages/core/src/utils/sedEditParser.ts:182 改用原始 pattern 的改动,以及
packages/core/src/tools/shell.ts:1951 的 CRLF 写回判据,都站得住。我还尝试构造一条会被这条路径接受、却写到指定
文件之外的脚本(s///w、s///e、w file;…、两个 -e、s///p、-i.bak、管道与重定向):全部被拒绝,写入口
看起来是完整的。
两条非阻断意见:
packages/core/src/utils/sedEditParser.ts:204-206:把原始 pattern 喂给这个 guard 是对的,但正则只匹配「自行闭合
整个括号表达式」的 POSIX 字符类。取反和混合写法仍会漏过,然后被错误模拟而不是回退——s/[^[:space:]]/X/g处理
z]x写出Xx(sed 是XXX),s/[[=a=]]/X/g处理a]写出X(sed 是X]),s/[[:alpha:]abc]/X/g处理
aabc]写出X(sed 是XXXX])。diff 的两侧行为一致,所以这是既有问题而非本次回归;但它正是函数上方注释里
「宁可回退也不要猜」的那条策略,只要在 pattern 含[:、[.或[=时一律回退即可关闭。packages/core/src/utils/sedEditParser.ts:520:加u之后多字节反向引用会命中(s/\(.\)\1/X/g处理😀😀x写出
Xx,而 BSD sed 不动这个文件)。同样的分歧在 base 上对éé已经存在,所以这与「以 GNU 为准」的取向一致——只是
sedEditParser.test.ts:278-279的 GNU/BSD 说明目前只写了x*一种,没有覆盖它。
|
Thanks for checking against the real binary rather than the new expectations — that is the evidence I could not produce from inside the simulation. Both notes are addressed in
No other changes; 53 sed-parser tests pass. |
|
Qwen Code review did not complete successfully. Qwen review exited with status 1. A transient error is retried automatically; if you are seeing this, retry with |













What this PR does
The shell tool's in-process sed simulation now compiles substitution patterns with the
sanduflags. This makes.match a carriage return and makes character classes and quantifiers operate on Unicode characters instead of UTF-16 code units. A literal]outside a bracket expression is escaped during translation so the stricter Unicode mode still accepts patterns such as[\[\]].Why it's needed
On CRLF files,
s/=.*$/=new/matched nothing and the tool reported that the edit made no changes. On files containing astral characters,s/^.//removed one half of an emoji and wrote the lone surrogate back as U+FFFD, silently corrupting the file. Real sed treats both cases as one character.Reviewer Test Plan
How to verify
Run the sed parser unit tests. The new regression case covers the six rows from the issue: astral characters with
., a negated class, a zero-widthx*match, and a CRLF substitution. Before the change it fails on the first astral row; after the change all cases pass. Also confirm that ordinary bracket patterns such as[abc],[^abc], and[\[\]]still take the simulation path rather than falling back.Evidence (Before & After)
N/A for UI. Unit tests: 53 passed, including new pins for negated, mixed and equivalence-class POSIX forms (
[^[:space:]],[[:alpha:]abc],[[=a=]]) and for the GNU/BSD multibyte-backreference split. The full core suite passed 39,351 tests; the singlebun-pty.test.tsfailure also reproduces on unmodified main. Typecheck, lint, and formatting pass.Tested on
Linux was verified in a Docker
node:22-bookwormarm64 container.Environment (optional)
Node 22.23.4; unit tests only.
Risk & Scope
uflag can reject patterns that previously compiled, so those commands now fall back to real sed. That is the existing fail-safe path and never writes a wrong file.x*iteration and multibyte backreferences are bytewise, while BSD sed and this simulation are characterwise. The POSIX-class guard now declines any pattern containing[:,[.or[=, so mixed and negated character classes also fall back to real sed instead of being simulated wrongly.Linked Issues
Closes #15675
中文说明
这个 PR 做了什么
shell 工具的进程内 sed 模拟现在会用
s和u标志编译替换模式。这样.能匹配回车符,字符类和量词也按 Unicode 字符而不是 UTF-16 code unit 工作。翻译时会把字符类外的字面量]转义,保证更严格的 Unicode 模式仍能接受[\[\]]这类模式。为什么需要
在 CRLF 文件上,
s/=.*$/=new/匹配不到任何内容,工具会报告没有产生修改。在包含星平面字符的文件上,s/^.//只删掉 emoji 的一半,并把孤立代理项写成 U+FFFD,静默损坏文件。真实 sed 在两种情况下都按一个字符处理。评审测试计划
如何验证
运行 sed 解析器单测。新增回归用例覆盖 issue 里的六行:星平面字符配合
.、否定字符类、零宽x*匹配,以及 CRLF 替换。修改前第一行星平面用例失败,修改后全部通过。同时确认[abc]、[^abc]、[\[\]]这类普通字符类仍然走模拟路径,而不是回退。证据(修改前后)
UI 变化:N/A。单测 53 通过,新增取反/混合/等价类 POSIX 形态(
[^[:space:]]、[[:alpha:]abc]、[[=a=]])以及 GNU/BSD 多字节反向引用分歧的钉子。core 全量 39,351 通过;唯一失败的bun-pty.test.ts在未修改的 main 上也能复现。类型检查、lint、格式化均通过。测试环境
Linux 在 Docker
node:22-bookwormarm64 容器中验证。运行环境(可选)
Node 22.23.4;仅单测。
风险与范围
u标志会拒绝一些以前能编译的模式,这些命令会回退到真实 sed。这是既有的 fail-safe 路径,不会写错文件。x*与多字节反向引用按字节推进,而 BSD sed 和本模拟按字符推进。POSIX 字符类 guard 现在对含[:、[.、[=的模式一律回退,混合与取反写法也不再被错误模拟。关联 issue
Closes #15675