fix(cli): degrade gracefully when messageRewrite.promptFile cannot be read - #9753
Conversation
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug with solid evidence. Linked issue #9752 carries a deterministic reproduction ( Direction: aligned — this restores the rewrite feature's own safe-degradation contract. A missing prompt file already falls back to the built-in default with a warning; an existing-but-unreadable path should degrade the same way instead of crashing the session. No product-direction concerns. Size: not core paths ( Approach: right-sized. Wrapping the read in try/catch and reusing the existing fallback is the simplest fix that covers all read failures (EISDIR, EACCES, EPERM) at once — a Risk: the changed path matches Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,证据充分。关联 issue #9752 提供了确定性复现( 方向:对齐——这是恢复 rewrite 功能自身的安全降级契约。缺失的 prompt 文件本来就会回退到内置默认值并打印警告;存在但不可读的路径也应同样降级,而不是让整个会话崩溃。无产品方向顾虑。 规模:非核心路径( 方案:范围合理。用 try/catch 包住读取并复用既有回退路径,是能一次覆盖所有读取失败(EISDIR、EACCES、EPERM)的最简修复—— 风险:改动路径命中 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewBefore reading the diff I wrote down my own baseline: wrap the read in try/catch, keep the existing missing-file branch untouched, add a real-filesystem regression test. The PR matches that exactly — no simpler path was missed, and nothing in the diff goes beyond it. No blockers:
Not blocking, just noted: Test evidence — the PR's own CI (this triage run never executes PR code)The unit suite is still running at review time; the snapshot below will be refreshed by the finalize job once CI settles. Everything completed so far is green (authorize/label/classify, secret scan, CVE audit, Desktop Shell on both OSes, the full Java matrix); no red check to analyze yet. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 Sandboxed verification would settle the remaining end-to-end claim: 中文说明代码审查读 diff 之前我先写下了自己的方案基线:用 try/catch 包住读取、保持缺失文件分支不动、补一个真实文件系统的回归测试。PR 与此完全一致——没有遗漏更简路径,diff 也没有超出这个范围。 无阻塞项:
非阻塞备注: 测试证据——来自 PR 自身的 CI(本次 triage 不执行 PR 代码)审查时单元测试仍在运行;上方快照会在 CI 落定后由 finalize 任务刷新。目前已完成的检查全部为绿(授权/标签/分类、密钥扫描、CVE 审计、双平台 Desktop Shell、完整 Java 矩阵);暂无红色检查需要分析。 沙箱验证可以落定剩余的端到端结论: — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 5/5 — a real, reproduced crash fixed with exactly the minimal change, pinned by a mock-free regression test. Stepping back: the problem is genuine — issue #9752 reproduces it deterministically, and I traced the throw path in the pre-fix code myself rather than taking the PR's word for it. The fix is the smallest thing that works: one try/catch around the single throw site, reusing the fallback that already existed for missing files, with the missing-file branch left untouched. The new test pins the behavior — a real directory, no mocks, and it fails with the exact reported error pre-fix. Nothing unrelated rode along, and the one comment added explains a genuinely non-obvious constraint. If I read this in six months it reads as an obvious, well-guarded line of defense — no curses. Approving, but CI hasn't settled on the reviewed commit yet — 2 中文说明置信度:5/5 —— 一个真实、可复现的崩溃,用恰好最小的改动修复,并由无 mock 的回归测试钉住。 整体来看:问题真实存在——issue #9752 可确定性复现,且我亲自在修复前代码中追踪了抛错路径,而非仅凭 PR 的描述。修复是可行的最小改动:在唯一抛错点外包一层 try/catch,复用缺失文件时既有的回退逻辑,缺失文件分支保持不动。新测试真正钉住了行为——真实目录、无 mock,且在修复前代码上会以报告中的原始错误失败。diff 没有夹带无关改动,新增的唯一注释解释了一个确实不明显的关键约束。六个月后再看这段代码,也只是一道显而易见、防护得当的防线。 结论是批准,但 CI 尚未在被审提交上落定——2 个 — Qwen Code · qwen3.8-max Reviewed at |
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. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): src/acp-integration/session/rewrite/LlmRewriter.test.ts — no such file or directory.
中文说明
仅完成部分审查,审查缺口已披露。 建议见行内评论。
未审查:build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally。
Test Plan(非阻断):src/acp-integration/session/rewrite/LlmRewriter.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Closeout update: documented the unreadable or missing |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
No issues found. LGTM! ✅
Test Plan (not a blocker): src/acp-integration/session/rewrite/LlmRewriter.test.ts — no such file or directory.
中文说明
未发现问题。LGTM!✅
Test Plan(非阻断):src/acp-integration/session/rewrite/LlmRewriter.test.ts — no such file or directory。
— qwen3.8-max via Qwen Code /review (v0.22.0)
|
Local runtime verification: PASS (ACP degradation path)
Before excerpt (ACP stdio): {"jsonrpc":"2.0","id":2,"error":{"code":-32603,"message":"Internal error","data":{"details":"EISDIR: illegal operation on a directory, read"}}}After excerpt (debug log, same trigger): i.e. the rewriter now degrades to Visual evidence (terminal screenshots, same trigger): Before (merge-base After (PR head |
1 similar comment
|
Local runtime verification: PASS (ACP degradation path)
Before excerpt (ACP stdio): {"jsonrpc":"2.0","id":2,"error":{"code":-32603,"message":"Internal error","data":{"details":"EISDIR: illegal operation on a directory, read"}}}After excerpt (debug log, same trigger): i.e. the rewriter now degrades to Visual evidence (terminal screenshots, same trigger): Before (merge-base After (PR head |
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings.
Scope: Standard tier — 3 files, 133 lines diff.
Checked:
LlmRewriter.tsconstructor:existsSyncpasses for directories;readFileSyncthen throws EISDIR. Fix wraps the read intry/catchand falls back toDEFAULT_REWRITE_PROMPT— mirrors the existing missing-file path. All code paths setthis.prompt.- Test efficacy: three tests use
mkdtempSyncto create a real temp dir and real file. Deleting thecatchblock would fail the directory test with EISDIR. The missing-file test and readable-file test cover the two surviving branches. - README doc update: accurate — matches the implementation.
- No credential/auth surface touched.
CI: precheck-pr SKIPPED — fork PR security model; build/test did not run from GitHub Actions. The three new unit tests are the executable evidence.
Cross-check: Prior review (2026-08-22) flagged debugLogger.warn as the only signal path — not a blocker, and the README update documents the QWEN_DEBUG_LOG_FILE escape hatch. No prior criticals outstanding.
Reviewed with AI assistance.
|
Released in v0.22.2. |


What this PR does
Wraps the
messageRewrite.promptFileread in theLlmRewriterconstructor in error handling. When the configured path exists but cannot be read as a file — a directory (EISDIR) is the deterministic example, unreadable files (EACCES/EPERM) behave the same — it now logs a warning and falls back to the built-inDEFAULT_REWRITE_PROMPT, the same degradation path already used for a missing file. Previously the barereadFileSync()threw out of the constructor.Why it's needed
Before this change,
existsSync()passed for a directory, thenreadFileSync()threw synchronously with no error handling anywhere between the constructor and the session-creation caller:Session.installRewriter()runs inside thecreateAndStoreSessiontry block, whose catch disposes the half-built session and rethrows. The result was that a misconfiguredpromptFilecrashed ACP session startup entirely, bypassing the rewrite feature's intended safe-degradation behavior. After this change, the session opens normally with the default rewrite prompt and a debug warning.Reviewer Test Plan
How to verify
Unit level:
npx vitest run src/acp-integration/session/rewrite/LlmRewriter.test.tsinpackages/cli. The newpromptFiledescribe block covers three cases: a real temp directory aspromptFile(the #9752 regression), a missing file (degradation unchanged), and a readable file (custom prompt still loaded and trimmed). On the pre-fix code the directory test fails with the exact error from the issue:After the fix all 15 tests in the file pass. End-to-end shape: set
"messageRewrite": { "enabled": true, "target": "all", "promptFile": "." }, open an ACP session — pre-fix the session fails to open withEISDIR, post-fix it opens and rewriting uses the default prompt.Evidence (Before & After)
N/A — no TUI/UI change; this is the ACP session startup path. The failing/passing test output above is the observable evidence.
Tested on
Environment (optional)
Unit tests only (
vitest), Node v24 on Linux. The failing path is platform-independent (synchronousfsread).Risk & Scope
EACCES/EPERMvariants go through the same catch but are not tested directly, since chmod-based permission tests are unreliable when CI runs as root; the directory case is the deterministic one. No config schema changes, no changes to other rewrite middleware.Linked Issues
Fixes #9752
中文说明
本 PR 做了什么
给
LlmRewriter构造函数中读取messageRewrite.promptFile的逻辑加上了错误处理。当配置的路径存在但无法按文件读取时——目录(EISDIR)是确定性例子,不可读文件(EACCES/EPERM)行为相同——现在会打印警告并回退到内置的DEFAULT_REWRITE_PROMPT,与文件缺失分支已有的降级路径完全一致。此前裸的readFileSync()会直接从构造函数抛错。为什么需要
改动之前,目录能通过
existsSync(),随后readFileSync()同步抛错,而从构造函数到会话创建调用方之间没有任何错误处理:Session.installRewriter()运行在createAndStoreSession的 try 块内,其 catch 会销毁半初始化的会话并重新抛出。结果就是配置错误的promptFile会让整个 ACP 会话启动崩溃,绕过了 rewrite 功能本应有的安全降级行为。改动之后,会话可以正常打开,使用默认 rewrite prompt,并打印一条 debug 警告。评审验证方案
如何验证
单元测试层:在
packages/cli下执行npx vitest run src/acp-integration/session/rewrite/LlmRewriter.test.ts。新增的promptFiledescribe 块覆盖三个用例:真实临时目录作为promptFile(#9752 回归)、文件缺失(降级行为不变)、可读文件(自定义 prompt 仍正常加载并 trim)。在修复前的代码上,目录用例会以 issue 中报告的原始错误失败:修复后该文件全部 15 个测试通过。端到端形态:设置
"messageRewrite": { "enabled": true, "target": "all", "promptFile": "." }后打开 ACP 会话——修复前会话因EISDIR打不开,修复后正常打开且改写使用默认 prompt。前后对比证据
N/A——无 TUI/UI 变化;这是 ACP 会话启动路径。上面失败/通过的测试输出即为可观测证据。
测试环境
运行环境(可选)
仅单元测试(
vitest),Linux 上 Node v24。失败路径与平台无关(同步fs读取)。风险与范围
EACCES/EPERM变体走同一个 catch,但未直接测试,因为 CI 以 root 运行时基于 chmod 的权限测试不可靠;目录用例是确定性的。不改配置 schema,不改其他 rewrite 中间件。关联 Issue
Fixes #9752