perf(cli): replace comment-json settings parser - #7747
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
E2E and performance reportStatic closure
2-vCPU Linux benchmarkThe control and candidate were built from the same baseline with identical runtime assets. The host had 2 Intel Xeon Platinum vCPUs, approximately 3.5 GiB RAM, no swap, Linux 6.6.102, and Node.js 22.23.1. One warmup preceded 30 alternating paired cold starts and 30 alternating paired starts after a 3-second preheat.
Peak process-tree RSS was about 10.8 MiB higher during initialization. A separate 10-pair follow-up sampled after a 10-second idle period; the paired steady-state median delta was +0.55 MiB with a 95% bootstrap interval spanning zero. Per-process inspection showed no additional process or persistent footprint increase. Concurrent first sessions, telemetry-disabled mode, outfile telemetry mode, and legacy single-session mode all succeeded. Enabled profiles validated, telemetry-disabled mode emitted no telemetry records, cleanup found no residual PID, and the daemon port was unreachable after shutdown. Local verification
The full workspace typecheck still has the pre-existing untouched Web Shell errors at |
|
Thanks for the PR! Template looks good ✓ Problem: observed and well-measured. Linked to #7264 (ACP eager-closure audit), with 30 alternating paired cold starts showing 35–49 ms P50 improvements on Direction: aligned — removing the Size: touches core paths ( Approach: the scope feels right. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：已观测且充分量化。关联 #7264（ACP 静态闭包审计），30 组交替配对冷启动显示 方向：对齐——移除 ACP 静态闭包中的 规模：触及核心路径（ 方案：范围合理。 进入代码审查 🔍 — Qwen Code · qwen3.8-max-preview Reviewed at |
Code reviewIndependent proposal: I would have replaced Comparison with the diff: the implementation goes further than my baseline in good ways — duplicate-key normalization (renaming earlier occurrences before removing them, working around No critical blockers found. A few observations:
Conventions: kebab-case filename ✓, collocated tests ✓, ESM imports ✓, no Files changed (14 of 14 shown)
CI test evidenceAll PR CI workflow runs on this commit have completed. The primary test suite (ubuntu-latest, Node 22.x) passed in ~35 minutes. macOS and Windows test jobs were skipped (conditional trigger), as were integration tests. No failures.
Not verified: Windows runtime behavior (author notes CI is the cross-platform check; Windows test job was skipped on this commit). The author's cold-start benchmark numbers (35–49 ms P50 improvement) are self-reported from a controlled 2-vCPU Linux host — not independently reproduced here. 中文说明代码审查独立方案： 我会用 与 diff 的比较： 实现比我的基线更深入——重复键规范化（先重命名早期出现再删除，绕过 未发现关键阻塞问题。几个观察：
CI 测试证据此提交的所有 PR CI 工作流运行已完成。主测试套件（ubuntu-latest, Node 22.x）在约 35 分钟内通过。macOS 和 Windows 测试作业被跳过（条件触发），集成测试同样跳过。无失败。 未验证：Windows 运行时行为（作者注明 CI 是跨平台检查；此提交的 Windows 测试作业被跳过）。作者的冷启动基准数字（P50 改善 35–49 ms）来自受控 2-vCPU Linux 主机的自报告——未在此独立复现。 — Qwen Code · qwen3.8-max-preview Reviewed at |
|
Confidence: 3/5 — clean review, but the 500+ production-line core-path threshold requires a maintainer's sign-off. This is a well-executed dependency swap. The problem is real and measured (305 KB closure reduction, 35–49 ms P50 cold-start improvement across 30 paired runs), the implementation is thorough without being over-engineered, and every file in the diff serves the stated goal. The new My independent proposal matched the PR's approach; I didn't find a simpler path it missed. CI is green on ubuntu. The code follows project conventions throughout. Deferring only because the production-line count (616) on core paths ( 中文说明置信度：3/5 —— 审查干净，但 500+ 生产行核心路径阈值需要维护者签字。 这是一次执行良好的依赖替换。问题真实且已量化（305 KB 闭包缩减，30 组配对运行中 P50 冷启动改善 35–49 ms），实现详尽但不过度工程化，diff 中每个文件都服务于既定目标。新的 我的独立方案与 PR 方法一致；未发现更简路径。CI 在 ubuntu 上通过。代码全程遵循项目约定。 仅因生产行数（616）触及核心路径（ — Qwen Code · qwen3.8-max-preview Reviewed at |
|
⏸️ Deferring to @wenshao — the review is clean (no blockers, CI green, conventions followed), but this PR touches core paths ( |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
— qwen3.7-max via Qwen Code /review
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Review Summary
This is a well-designed and thoroughly implemented performance optimization that replaces comment-json with jsonc-parser for settings and trusted-folders writes. The PR demonstrates strong attention to edge cases, security, and test coverage.
✅ Strengths
1. Comprehensive Edge Case Handling
The implementation correctly handles:
- UTF-8 BOM preservation (
\uFEFF) - CRLF/LF line endings
- Tab vs space indentation
- Final newline preservation
- Inline comments on deleted properties
- Duplicate keys (removes earlier occurrences, keeps the effective last value)
- Prototype pollution protection (
__proto__,constructor,prototypefiltered)
2. Output Validation
The updateJsoncContent function re-parses and deep-compares the edited output against the intended target before returning. This is a critical safety check that prevents silent corruption.
3. Good Test Coverage
Tests cover the important cases including:
- Malformed input rejection
- Non-object roots (arrays, primitives)
- CRLF + tabs + BOM + final newline
- Duplicate key normalization
- Prototype-named properties (
__proto__,constructor) - No-op byte-for-byte unchanged detection
- Inline comment removal on deleted properties
4. Bundle Guard Extension
Properly extends the fast-path bundle guard to forbid comment-json, esprima, and the jsonc-parser UMD build from the ACP static closure.
5. Clean Dependency Changes
Correctly moves jsonc-parser from devDependencies to dependencies and removes the comment-json dependency chain.
🔍 Minor Observations
1. Duplicate Key Handling Complexity
The normalizeDuplicateProperties function uses a clever temporary key renaming approach. While it works, it iterates until no duplicates remain, which could theoretically be O(n²) for files with many duplicate keys. In practice, this is unlikely to be a problem since duplicate keys in config files are rare and files are small.
2. Structured Clone vs JSON Round-trip
In updateJsoncContent, the structuredClone followed by JSON.parse(JSON.stringify()) is intentional to handle undefined values. A brief comment explaining the two-step process would help future maintainers.
3. Trailing Comma in Test Expectation
The test expectation correctly accounts for the trailing comma that jsonc-parser.modify() produces given allowTrailingComma: true.
✅ Security Review
- Prototype Pollution: Properly filtered in
applyUpdatesloop - No
evalor dynamic code execution: Usesjsonc-parser's safe parse APIs - Atomic writes: Maintains the existing
writeWithBackupSyncpattern - Input validation: Rejects malformed JSONC before writing
✅ Correctness Verification
The implementation correctly:
- Strips BOM before parsing, re-adds after editing
- Detects indentation from actual property nodes (not comment contents)
- Removes inline comments when deleting properties (prevents orphaned comments)
- Validates output matches intended value before returning
📊 Performance Claims
The PR provides concrete benchmark data showing:
- Bundle size reduction: 304,770 bytes (2.45%)
- Cold start P50 improvements: 35-48ms across metrics
- Statistical significance: 95% bootstrap intervals, 28/30 paired wins
The measurement methodology appears sound.
Conclusion
This is a high-quality implementation that achieves its performance goals while maintaining correctness and adding comprehensive test coverage. The edge case handling for comment preservation, formatting, and duplicate keys is thorough.
Maintainer verification — local build + real end-to-end testingI ran an independent local verification of this change at head Verdict: the change does what it claims. The parser swap is behaviour-preserving on the settings values, the startup-closure reduction reproduces independently, and the security boundaries around Environment: macOS 24.6.0, Node v22.23.1, isolated worktrees for 1. Gates
2. Behavioural equivalence — old writer vs new writer, side by sideI imported the deleted 428 / 433 agree exactly. All five differences are intentional and are improvements:
3. Real end-to-end A/B with the actual bundled productTwo isolated Migration write-back (
Merge write path (
4. Formatting churn — the practical winFor a single one-value edit (
5. The performance claim reproducesI measured the ACP static closure myself — BFS over
I did not attempt to reproduce the 2-vCPU Linux latency numbers; the closure reduction that drives them is confirmed.
6.
|
| 检查项 | 结果 |
|---|---|
jsonc-editor.test.ts + settings.test.ts + trustedFolders.test.ts |
223 通过 |
整个 src/config + src/utils 面 |
86 文件 / 2094 测试通过 |
scripts/tests/serve-fast-path-bundle-check.test.js |
31 通过 |
tsc --noEmit（packages/cli） |
干净 |
eslint --max-warnings 0（全部 10 个改动文件） |
干净 |
prettier --check（改动文件 + 设计文档） |
干净 |
npm run check:serve-fast-path-bundle |
control 与 candidate 均通过 |
2. 行为等价性 —— 新旧写入实现并排对比
我把被删除的 commentJson.ts 与新的 jsonc-editor.ts 导入同一个进程，对每个语料文件的逐字节相同副本施加相同更新：33 个手写用例（各种位置的注释、CRLF、tab、BOM、尾逗号、重复键、原型同名键、深层嵌套、对象数组、Unicode、格式错误与非对象根）外加 400 个带种子的随机 JSONC 文档。
433 个用例中 428 个完全一致。 5 处差异全部是有意为之，且都是改进：
| 场景 | 旧（comment-json） |
新（jsonc-parser） |
|---|---|---|
settings.json 是 [1,2,3] |
返回 true，把文件重排成数组并静默丢弃本次更新 |
返回 false + stderr 提示，文件不动 |
settings.json 是 "hello" / 42 |
返回 true，静默丢弃更新 |
返回 false + stderr 提示，文件不动 |
sync 模式下的 "toString": "zombie" |
保留该僵尸键（'toString' in updates 经原型链为真） |
正确删除 —— 即 Object.hasOwn 修复 |
自有 __proto__ 键 |
直接删掉 | merge 模式保留、sync 模式删除；两者都不会污染 Object.prototype（已验证） |
3. 用真实产物做端到端 A/B
两个隔离的 HOME，逐字节相同、写满注释的 v1 settings.json，各自跑一次真实 bundled CLI —— 全程无 mock。
迁移回写（qwen mcp list 会触发 persistSettingsObject(sync=true)）：
- 迁移后的 settings 取值在 control 与 candidate 之间语义完全一致。
- candidate 保住了文件末尾换行和
main会破坏掉的尾部// ── end of file ──注释。 - candidate 丢了一条
main保住的行内注释 —— 见 F1。
merge 写入路径（qwen mcp add --scope user git uvx mcp-server-git）—— 这里 candidate 严格更优：在语义一致的前提下，保住了 main 会丢掉的前置注释和末尾换行。
4. 格式扰动 —— 实际收益所在
对一个 21 行的 settings 文件做单个值的修改（ui.theme: "Dracula" → "Default"）：
| 实现 | 修改前行数 | 修改后行数 | 逐字保留 | 被重写 |
|---|---|---|---|---|
comment-json（main） |
21 | 46 | 13 / 21 | 8 |
jsonc-parser（本 PR） |
21 | 21 | 20 / 21 | 1 |
main 每次写入都会把文件里所有紧凑对象和数组展开。本 PR 只动被修改的那一行。对手工维护 settings.json 的人来说，这才是最显著的行为变化，而 PR 描述里并没有提到。
5. 性能声明可复现
我自行测量了 ACP 静态闭包 —— 在各自生成的 dist/esbuild.json 中，从 ACP 入口 chunk 沿 import-statement 边做 BFS 并累加 output 字节：
| 闭包字节数 | |
|---|---|
control（main@27927c46d） |
12,528,737 |
| candidate（PR 7747） | 12,223,454 |
| 差值 | −305,283 字节（−2.44 %） |
| PR 自称 | −304,770 字节（−2.45 %） |
comment-json、esprima、array-timsort、repeat-string、has-own-prop 已从闭包中消失，也从整个 bundle 中消失（grep 计数为 0）。闭包内 jsonc-parser 解析到 ESM 构建，UMD 构建不存在，确认 esbuild alias 按设计生效。
我没有尝试复现 2-vCPU Linux 的时延数字；驱动这些数字的闭包缩减已得到确认。
6. trustedFolders 安全边界 —— 未变
用真实文件系统、无 mock 的 harness 对两个实现各跑 22 项检查：规则增删、0600 权限、原子写、通过持锁重读保留并发磁盘规则、拒绝符号链接配置路径且目标文件不受影响、失败后释放锁、拒绝非对象/格式错误根且不覆盖文件、拒绝磁盘上和拟写入的非法信任级别、新建文件。
两侧均 22 / 22 通过。 唯一差异是删除规则时的注释保留（3/4 对 4/4）—— 同属 F1。
7. 未 bundle 的编译产物在纯 Node 22 下运行
不用 tsx、不用 vitest、不用打包器 —— 即已发布 npm 包所走的路径。Node 把公开入口解析到 jsonc-parser/lib/umd/main.js，而 esbuild alias 到 ESM；两条路都能跑。8 项断言全部通过。
8. 对新增测试做变异验证
我每次只破坏 jsonc-editor.ts 中的一条保证，检查那 223 个定向测试是否变红。16 个变异体杀掉 11 个。
被杀掉的包括：BOM 剥离与回贴、行内注释移除、重复键归一化、Object.hasOwn 的 sync 修复、非对象根拒绝、解析错误拒绝、删除操作收集、insertSpaces、replacePath、原型键防护。对核心逻辑而言这个覆盖确实不错。
5 个存活变异体可分为 3 个等价变异体 + 2 个真实缺口 —— 我逐一做了验证而非臆断：
detectFormattingOptions里的insertFinalNewline和eol对modify()而言是惰性的。直接驱动jsonc-parser可证：insertFinalNewline取true与false输出完全相同；对 CRLF 文档设eol: '\n'仍然输出 CRLF。末尾换行与 CRLF 的保留是真实的，但来自jsonc-parser自身按文档归一化，而不是这两个字段。因此看起来在测试这段代码的那两条断言，实际上并没有。- 下面的 F3 与 F4 是真实的覆盖缺口。
后续跟进项
以下均不阻塞任何事 —— PR 已合并且行为正确，列出来只为存档。
F1 · 删除某个属性时，会连带删掉相邻存活属性的行内注释。（中低）
removeTrailingInlineComment() 处理了被删属性自身的注释，但 jsonc-parser 的删除区间会向前吃掉分隔逗号，从而把落在前一个属性行尾的行内注释一并吞掉；而删除第一个属性时，吞掉的是后一个属性的前置注释。已用三种独立方式复现：注释保真矩阵、真实 bundled CLI 迁移 A/B、trustedFolders harness。它会在 persistSettingsObject(sync=true)（升级后首次启动的迁移回写）以及删除受信目录规则时触发，而后者的逐条注释具有审计价值。
净效果是一种取舍而非纯粹损失：本 PR 同时保住了 comment-json 会破坏的尾部注释和末尾换行。在矩阵里，main 和本 PR 各自丢掉了对方保住的注释。
F2 · 重复键归一化会连带删掉描述存活那一份的注释。（低）
设计文档只承诺"随被移除的重复项一起删除其拥有的注释"。实测对于：
{
// comment for the ignored duplicate
"model": "a", // inline on ignored duplicate
// comment for the EFFECTIVE occurrence
"model": "b"
}comment-json 产出 { // comment for the EFFECTIVE occurrence\n "model": "c" ... }；新编辑器产出 { "model": "c" } —— 三条注释全没了。取值正确，但注释删得过头。settings.json 里出现重复键本身就很病态，故定为低。
F3 · tabSize 是承重逻辑却无测试覆盖。（测试缺口）
4 空格缩进确实被保留（已验证），但把 tabSize 强制为 2 不会让任何测试变红。这意味着 4 空格缩进的文件会悄悄多出 2 空格的键。值得补一条测试 —— 4 空格的 settings.json 很常见。
F4 · "输出与意图一致"这道保险没有任何测试。（测试缺口）
删掉 if (!isDeepStrictEqual(reparsed, target)) throw … —— 防止写出与调用方意图不符的 settings 的最后一道防线 —— 223 个测试无一变红。
F5 · 有一条测试并未测它名字所说的东西。（次要）
detects indentation from properties rather than quoted comments：该 fixture 里第一个属性键本就独占一行，缩进候选值已经是 "\t"，/^[ \t]*$/ 这道防护根本不会触发。只有当第一个键不在行首时才会触发 —— 例如单行的 { "a": 1 }。补上这个用例才能名副其实。
Code Review SummaryThis is a well-executed performance optimization that replaces ✅ Strengths1. Comprehensive Edge Case HandlingThe new
2. Correctness ImprovementsThe change from 3. Output Validation Safety NetThe const reparsed = parseJsoncObject(updated);
if (!isDeepStrictEqual(reparsed, target)) {
throw new Error('Edited JSONC does not match the intended settings.');
}4. Better Format PreservationA key behavioral improvement not emphasized in the PR description: 5. Proper Error Handling
6. Bundle Guard ExtensionsCorrectly extends the fast-path bundle guard to forbid 🔍 Observations1. Comment Retention Edge Case (low-medium)As noted in the maintainer verification, deleting a property can also remove inline comments belonging to neighboring surviving properties due to how 2. Duplicate Key Comment Handling (low)The duplicate key normalization removes all comments associated with duplicate keys, including comments describing the surviving (effective) occurrence. Since duplicate keys in 3. Test Coverage Gaps (test)
✅ Security Review
📊 Performance Claims VerificationThe bundle size reduction is reproducible:
The ConclusionApproved ✅ This is a high-quality implementation achieving meaningful performance improvements while maintaining correctness. The edge case handling for comment preservation, formatting, and duplicate keys is thorough. The follow-up items identified in the maintainer verification (F1-F5) are minor and do not block this change. 中文说明代码审查总结这是一次执行良好的性能优化，用 优点
观察
结论批准 ✅ 高质量实现，性能提升明确，正确性有保障。 |
|
Released in v0.21.1. |
|
|
4 participants
What this PR does
This PR replaces the synchronous
comment-json-based settings and trusted-folders writer with path-basedjsonc-parseredits. It preserves comments, indentation, line endings, final-newline state, UTF-8 BOMs, merge and sync behavior, exact-subtree replacement, trusted-folder locking, validation, and atomic-write guarantees while removing the legacy parser cluster from the ACP eager startup closure.It also makes the bundle resolve the public
jsonc-parserimport to its ESM build, while leaving unbundled compiled output on the public Node-compatible package entry, and extends the fast-path bundle guard to rejectcomment-json,esprima, or the incompatiblejsonc-parserUMD build from the ACP static closure.Why it's needed
ACP imports settings and trusted-folders writers before it can answer
initialize, although ordinary startup does not write either file. Removing the legacy JSONC parser cluster reduces the exact ACP static closure from 12,449,869 to 12,145,099 bytes, a reduction of 304,770 bytes (2.45%).On the established 2-vCPU Linux host, 30 alternating paired cold starts improved
channel.initializeP50 by 35.39 ms, process-to-first-session P50 by 38.00 ms, and process-to-first-session-complete P50 by 48.51 ms, with 28 of 30 paired wins for each primary metric and paired-mean 95% bootstrap intervals entirely below zero. Preheated process-to-session-complete behavior was statistically neutral.Reviewer Test Plan
How to verify
Update existing settings and trusted-folders JSONC containing leading, nested, inline, and trailing comments, then confirm only the requested values change and the original indentation, line endings, final newline, BOM, unrelated keys, and comments remain intact. Exercise merge, sync, and exact-subtree replacement and confirm malformed or non-object JSONC is rejected without overwriting the file. Confirm trusted-folders writes still re-read under lock, validate both disk and proposed state, preserve concurrent disk rules, write atomically with mode
0600, and release the lock after failures.Run the focused CLI tests, CLI typecheck, lint, clean CLI build and bundle, and the fast-path bundle guard. The generated ACP metafile closure should contain no
comment-json,esprima, orjsonc-parser/lib/umdinput, and both the unbundled compiled editor and bundled CLI should run under Node.js 22.Evidence (Before & After)
N/A — this is a non-UI startup and configuration-writing change.
Tested on
Environment (optional)
macOS with Node.js 22.22.3 for focused tests, typecheck, lint, clean build, bundle, unbundled runtime smoke, and bundle guard. Linux 6.6.102 with Node.js 22.23.1 on a 2-vCPU, approximately 3.5-GiB, no-swap host for 30 paired cold starts, 30 paired preheated starts, compatibility scenarios, and a separate 10-pair steady-memory follow-up.
Risk & Scope
Linked Issues
#7264
中文说明
此 PR 的改动
此 PR 使用基于路径的
jsonc-parser编辑替换同步的comment-jsonsettings 与 trusted-folders 写入实现。在移除 ACP 启动静态闭包中的旧解析器依赖簇的同时，保留注释、缩进、换行符、文件末尾换行状态、UTF-8 BOM、merge 与 sync 行为、精确子树替换、trusted-folders 锁、校验和原子写入保证。同时，bundle 构建会把公开的
jsonc-parserimport 解析到其 ESM 构建，而未 bundle 的编译产物仍使用 Node 可直接运行的公开包入口；fast-path bundle guard 也扩展为禁止comment-json、esprima或不兼容的jsonc-parserUMD 构建进入 ACP 静态闭包。为什么需要
ACP 必须在回答
initialize前导入 settings 与 trusted-folders 写入实现，尽管普通启动过程不会写入这两个文件。移除旧 JSONC 解析器依赖簇后，ACP 精确静态闭包从 12,449,869 字节降至 12,145,099 字节，减少 304,770 字节（2.45%）。在既有 2-vCPU Linux 主机上，30 组交替配对冷启动中，
channel.initializeP50 改善 35.39 ms，进程到首次 session 响应 P50 改善 38.00 ms，进程到首次 session 完成 P50 改善 48.51 ms；三个主要指标均为 30 组中 28 组胜出，配对均值的 95% bootstrap 区间全部低于零。预热后的进程到 session 完成行为在统计上持平。Reviewer 测试计划
如何验证
更新包含前置、嵌套、行内和尾部注释的现有 settings 与 trusted-folders JSONC，确认只有指定值发生变化，原有缩进、换行符、文件末尾换行、BOM、无关字段和注释均保持不变。覆盖 merge、sync 和精确子树替换，并确认格式错误或根节点不是对象的 JSONC 会被拒绝且不会覆盖文件。确认 trusted-folders 写入仍会在持锁后重新读取磁盘、校验磁盘状态与拟写入状态、保留并发写入的磁盘规则、以
0600权限原子写入，并在失败后释放锁。运行定向 CLI 测试、CLI typecheck、lint、干净的 CLI build 与 bundle，以及 fast-path bundle guard。生成的 ACP metafile 闭包不应包含任何
comment-json、esprima或jsonc-parser/lib/umd输入，并且未 bundle 的编译编辑器与 bundled CLI 均应能在 Node.js 22 下运行。证据（Before & After）
N/A — 这是非 UI 的启动和配置写入改动。
测试平台
环境（可选）
macOS、Node.js 22.22.3：定向测试、typecheck、lint、干净 build、bundle、未 bundle 运行时 smoke 和 bundle guard。Linux 6.6.102、Node.js 22.23.1、2-vCPU、约 3.5 GiB、无 swap：30 组配对冷启动、30 组配对预热启动、兼容性场景，以及单独的 10 组配对稳态内存复测。
风险与范围
关联 Issue
#7264