Repository navigation
Conversation
|
Thanks for the PR — and for the bug report behind it. #15675 is unusually well researched, which made this gate a lot easier to run. Template looks good ✓ Problem: observed, not theoretical. The issue carries a six-row table comparing the simulation against BSD sed with UTF-8 hex output, a named root cause (the translated Direction: aligned. The in-process simulation only earns its place if it is indistinguishable from real sed; every divergence either blocks an edit the user asked for or rewrites a file wrongly. It also follows the philosophy this file already applies elsewhere — POSIX bracket expressions, leading Size: core path ( Approach: minimal, and it matches what I arrived at independently before reading the diff. One shared flag constant feeding both construction sites, one escape branch, one argument switch. I looked for a smaller version and don't think there is one: One ask, because it currently reads like a drive-by and isn't: switching the POSIX-bracket check from the translated pattern to the raw sed pattern is load-bearing. Once Risk: no elevated risk signals — none of the high-revert-correlation paths are touched. Two notes for later, neither a blocker: #14428 is still open and also edits this file, so the ordering is worth a deliberate choice; and your test evidence is Windows-only, which I come back to in the review comment. Moving on to code review. 🔍 中文说明感谢这个 PR,也感谢背后的 issue。#15675 的调研非常扎实,让这次准入检查轻松很多。 模板完整 ✓ 问题: 是已观测到的 bug,不是理论性加固。issue 里有一张六行的对照表,用 UTF-8 十六进制列出了模拟结果与 BSD sed 的差异,指出了根因(翻译出的 方向: 对齐。进程内模拟只有在与真实 sed 无法区分时才值得存在;任何偏差要么挡住用户想要的编辑,要么错误地改写文件。这也符合该文件已有的处理哲学——POSIX 字符类、开头的 规模: 触及核心路径( 方案: 足够精简,和我在读 diff 之前独立想到的做法一致:一个共享的标志常量供两处构造点使用、一个转义分支、一个参数切换。我找过更小的写法,认为没有:只加 有一点想请你说明,因为它现在看起来像顺手改动,其实不是:把 POSIX 字符类检查的输入从翻译后的 pattern 换成原始 sed pattern 是关键改动。一旦 风险: 无升级风险信号——未触及与回滚高度相关的路径。 两点后续说明,都不是阻塞项:#14428 仍处于开放状态且同样修改该文件,合并顺序值得有意识地安排;另外你的测试证据只在 Windows 上取得,这一点我会在代码审查评论里再提。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewStatic review only — no PR code was built or executed. I read the diff against No critical blockers found. The change is correct as far as I can establish by reading it, and the reasoning hangs together:
I hand-traced all four new parser assertions and the new shell assertion against the translation and the engine semantics, and each expected value is right — including One thing I could not settle, and it is the gap that matters. The Non-blocking nit: the new shell test spells its write expectation inline instead of reusing Test evidenceThere is none, and that is the substance of this comment. Every one of this PR's own CI workflows completed as
For completeness, and clearly attributed: the author reports in the PR body that Sandboxed verification would settle this, and two different gaps need two different lanes:
Real-scenario testing (2c): N/A — unattended CI run, never driven locally. 中文说明代码审查仅做静态审查——没有构建或执行任何 PR 代码。我把 diff 与 未发现阻塞性缺陷。 就我能通过阅读确认的范围而言,改动是正确的,推理也自洽:
我按翻译逻辑和引擎语义手工推演了四条新增 parser 断言和一条新增 shell 断言,每个期望值都是对的——包括 有一件我无法确认的事,而它恰恰是关键缺口。 上面的 非阻塞小项:新增的 shell 测试把写入期望直接内联写出,而没有复用 测试证据没有证据,这正是本条评论的实质。 本 PR 自身的每一个 CI 工作流都以 为完整起见并明确归属:作者在 PR 描述中称 沙箱验证可以厘清此事,而两个不同的缺口需要两条不同的通道:
真实场景测试(2c):不适用——本次为无人值守 CI 运行,未在本地驱动产品。 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 3/5 — I could not find a defect in this diff, but a core-path change with zero executed tests is not something I should sign off on alone. Going back to what I proposed before reading the diff: the PR matches it almost exactly — one shared flag constant feeding both construction sites, What I like: the diff is 17 production lines that fix two distinct user-visible divergences, both of which are documented with measured hex output against real sed. Every new assertion I traced by hand is correct. Every way the change can go wrong that I could find goes wrong in the safe direction — Why I am not approving:
Stage 0's bar for core paths is 100% confidence, and "the reasoning is sound but nothing has run" is not that. The fork- @yiliang114 — deferring to you, and you are already the assigned owner. Two things would unblock this: authorize the pending CI runs so the suite actually executes, and give the unicode-mode change a maintainer's eye, since it changes how every simulated sed pattern in the tool is compiled. Once CI is green on this commit, Worth deciding deliberately: #14428 is still open and also edits 中文说明信心度:3/5 —— 我在这份 diff 里找不到缺陷,但一个核心路径的改动在完全没有执行过测试的情况下,不该由我单独放行。 回到我在读 diff 之前自己写的方案:这个 PR 与它几乎完全一致——一个供两处构造点共用的标志常量、用 我认可的地方:17 行生产代码修掉了两处用户可见的语义偏差,两处都有对照真实 sed 的实测十六进制输出作为依据。我手工推演过的每一条新增断言都是正确的。我能想到的所有出错方式,都朝着安全的方向出错—— 我不批准的原因:
Stage 0 对核心路径的要求是 100% 确信,而"推理成立但什么都没跑过"不是 100%。fork @yiliang114 —— 转交给你,你也已经是本 PR 的负责人。有两件事可以解锁:授权待处理的 CI 运行,让测试套件真正执行;并以维护者视角看一下这个 unicode 模式改动,因为它改变了工具中每一个被模拟的 sed pattern 的编译方式。这个 commit 上 CI 变绿之后, 有一件事值得有意识地决定:#14428 仍然开放,且同样修改 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Verdict: findings — 87/91 scripted checks passed; verified head 中文摘要结论:findings(87/91 条断言通过)。被验证 head 为
修复建议:在 Central claim and A/BThe PR claims the in-process sed simulation now matches real sed for (a) CRLF input where
The three rows the PR is about ( The broader corpus exercises the sibling shapes the fix's mechanism admits: CRLF anchors ( Byte-level note. 8 of 41 simulated head cells differ from real sed in the bytes written to disk: six are the CRLF cells where the match consumes FindingsF1 — Medium — ERE-only escapes are silently applied where both real seds leave the file untouched
The in-class control agrees because the ERE branch doubles a backslash inside a bracket expression ( Reproduce directly against the head source: const info = parseSedEditCommand("sed -E -i 's/\\p{L}/X/g' f.txt")!;
applySedSubstitution('abc\n', info); // "XXX\n"Real Measured suggested fix (one guard, no collateral) hasSedJavascriptDivergentEscape(jsPattern) ||
+ hasUnicodeModeOnlyEscape(jsPattern) ||
hasBracketCharacterEscape(sedInfo.pattern)
@@
+function hasUnicodeModeOnlyEscape(pattern: string): boolean {
+ return /(?<!\\)\\[pPu]\{/u.test(pattern);
+}Applied to a third worktree at the same head: parser suite 53/53, corpus 51/51 cells as expected (the three hostile cells now decline and run real sed), and 38 benign cells byte-identical to head. The negative lookbehind is what keeps the in-class F2 — Suggestion — the parse-time half of
|
| Mutant | Result | Witness |
|---|---|---|
M1 runtime flags → g |
killed (51/53) | the two new parser tests |
M2 parse flags → g |
survived (53/53) | see F2 |
M3 drop s |
killed (52/53) | CRLF dot test |
M4 remove ] escape |
killed (52/53) | existing [\[\]] ERE test |
M5 POSIX check on jsPattern |
killed (52/53) | existing POSIX-decline test |
| M6 control: remove divergent-escape check | killed (52/53) | \d reject test |
| M7 control: neuter leading-bracket check | killed (51/53) | []a] / [^]] tests |
The two controls prove the runner can see red on lines the suite already pins; without them the survivor would be indistinguishable from a dead harness.
Gates
| Arm | sedEditParser.test.ts |
shell.test.ts -t "simulated sed edit" |
|---|---|---|
head 0ae72eba |
53/53 | 25/25 |
base 51c59e69 |
51/51 | 24/24 |
trial merge 46c1347b |
53/53 | 25/25 |
The trial merge is origin/main (5cb982da) plus the PR head; main touched none of the PR's files since the base, and the merge was conflict-free. The focused suites ran under a minimal vitest config because the repository's global setup refuses to run without a built packages/core/dist; these two suites use only relative imports, and the config omits the guard (the guard's own concern, workspace dist staleness, is unrelated to them).
Not covered
- Windows — the author tested there; this round is macOS only.
- Linux CI — all three of the PR's workflows are
action_required(first-time fork contributor), so no CI lane has executed this commit. The trial merge is the closest substitute for "what lands". - The full
shell.test.ts— only the 25 simulated-sed tests were run; the full 375-test file was not. - A real model-driven CLI turn — the harness drives
parseSedEditCommand,applySedSubstitution, and the realprepareTextFileContentwrite-layer function directly, not through a model tool call. - PR fix(core): keep sed anchors inside groups and alternations #14428 — still open and also edits
sedEditParser.ts; whichever lands second needs a rebase. This round did not evaluate the pair. - Replacement-side GNU escapes (
\U,\L,\uin the replacement) — the parser's replacement filter already declines unsupported escapes; they were not swept here.
Methodology
macOS (darwin), Node 24.18.1, vitest 3.2.7, BSD /usr/bin/sed and GNU gsed 4.10 with LC_ALL=en_US.UTF-8. Four worktrees off the same repository: base 51c59e69, head 0ae72eba, trial merge 46c1347b (origin/main 5cb982da + head), and a fix arm at head plus the one-line guard. The corpus harness imports each arm's sedEditParser.ts through tsx, compares the substitution string to both real sed binaries, and computes the bytes the product would write through head's exported prepareTextFileContent (a file the PR does not touch). Mutation rows use exact-text replacement plus vitest's JSON reporter, with restore verified by sha256. Raw logs and harnesses live in tmp/pr15694-verify-20261027-192900/.
What I would do as a reviewer: the central fix is sound and I would merge it. F1 is narrow (ES-only escapes in ERE) but it is a silent edit where sed either no-ops or rejects, so I would either land the measured one-line guard in this PR or file it as a follow-up with the table above as the fixture; F2 is a one-assertion follow-up at most.



What this PR does
The in-process sed edit simulation now treats text as Unicode characters instead of UTF-16 code units and lets
.consume the carriage return retained from CRLF input. It keeps the parse-time and execution-time regular-expression flags consistent and escapes a literal closing bracket where Unicode mode requires it, so the existing supported sed patterns continue to use the simulation safely.Why it's needed
Issue #15675 reports two user-visible divergences from real sed. In CRLF files,
s/=.*$/=new/can report that no changes were made because.does not consume\r. For non-BMP characters, patterns such ass/^.//can remove half of an emoji and write the orphaned surrogate as U+FFFD. Both behaviors make the simulated edit disagree with the shell command and can either block an edit or silently corrupt content.Reviewer Test Plan
How to verify
Run the focused parser and shell regression suites:
The first suite covers CRLF dot matching and non-BMP handling for
.,[^...], and zero-width quantifiers. The second confirms that a CRLF substitution is written withlineEnding: crlfmetadata and that all simulated-sed scenarios still pass.Evidence (Before & After)
Before: a CRLF substitution could report no changes, and
s/^.//on😀 Titlecould leave U+FFFD in the file. After: both cases follow real sed semantics, with the substitution producing LF-normalized content and the shell write retaining the source file's CRLF metadata. This is an internal behavior change, so there is no UI screenshot.Tested on
Local verification used Windows and Node.js 24.18.0.
Risk & Scope
Linked Issues
Fixes #15675
中文说明
本 PR 做了什么
进程内的 sed 编辑模拟现在按 Unicode 字符而不是 UTF-16 码元处理文本,并允许
.匹配 CRLF 输入拆分后保留的回车符。它让解析阶段和执行阶段使用同一组正则标志,并在 Unicode 模式要求时转义字符类外的字面量],从而让现有受支持的 sed 模式继续安全地使用模拟路径。为什么需要这个改动
Issue #15675 报告了两处与真实 sed 不一致的用户可见行为。在 CRLF 文件中,
s/=.*$/=new/可能因为.不匹配\r而错误地报告没有发生修改。对于非 BMP 字符,s/^.//这类模式可能只删除 emoji 的一半,并在写回时把孤立代理项变成 U+FFFD。这两种行为都会让模拟编辑与 shell 命令不一致,可能阻止编辑,也可能静默破坏内容。Reviewer 验证计划
如何验证
运行聚焦的 parser 和 shell 回归测试:
第一组测试覆盖 CRLF 下的点号匹配,以及
.、[^...]和零宽量词对非 BMP 字符的处理。第二组测试确认 CRLF 替换会以lineEnding: crlf元数据写回,并确认所有模拟 sed 场景仍然通过。证据(修改前与修改后)
修改前:CRLF 替换可能报告没有修改;
s/^.//作用于😀 Title时可能在文件中留下 U+FFFD。修改后:两种情况都符合真实 sed 语义,替换层输出 LF 归一化内容,shell 写回时保留源文件的 CRLF 元数据。这是内部行为修复,因此没有 UI 截图。测试环境
本地使用 Windows 和 Node.js 24.18.0 验证;macOS 和 Linux 未在本地执行。
风险与范围
关联 Issue
Fixes #15675