fix(cli,core): Restore default debug log file output#6605
Merged
Conversation
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: Real, user-reported regression. Issue #6600 documents that v0.19.8 Direction: Aligned. Restoring default debug log file output is exactly what the linked issue asks for. The opt-out mechanism via explicit falsy env values preserves the control that PR #4914 introduced, while fixing the broken default. This is core diagnostic infrastructure — users rely on debug logs to report bugs. Size: 28 production lines (5 files: Approach: The core two-line fix ( I do notice some drive-by changes beyond the debug log fix: the CI workflow Playwright port selection, dynamic import changes in Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：真实的用户报告回归。Issue #6600 记录了 v0.19.8 的 方向：对齐。恢复默认 debug 日志文件输出正是关联 issue 所要求的。通过显式 falsy 环境变量值 opt-out 的机制保留了 PR #4914 引入的控制能力，同时修复了损坏的默认行为。这是核心诊断基础设施——用户依赖 debug 日志来报告 bug。 规模：28 行生产代码（5 个文件： 方案：核心两行修复（ 我注意到一些 debug log 修复之外的顺手改动：CI workflow Playwright 端口选择、 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewThe fix is a two-character logic change in My independent proposal before reading the diff: change the guard in Correctness: The empty-string case ( Reuse check: No blockers found. The change is minimal, focused, and correct. Unit tests: 302 tests pass across the affected files — 30 in Real-Scenario TestingBefore (installed qwen v0.19.8 — bug reproduces)Path printed, file never created. Bug confirmed. After (this PR — fix verified)File created (20KB), Opt-out (QWEN_DEBUG_LOG_FILE=0)No misleading "Logging to:" path. Disabled notice shown instead. Opt-out confirmed. — Qwen Code · qwen3.7-max |
Collaborator
|
This is a clean, minimal regression fix that does exactly what it says. The root cause is a single logic error ( I independently proposed the same approach before reading the diff. The PR matches. All three real-scenario tests confirm the fix works: default-on creates the file (20KB, symlink updated), explicit opt-out disables cleanly (disabled notice shown, no misleading path), and the before build reproduces the bug (file not created). The change touches The drive-by test infrastructure changes (useStatusLine, voice-keyterms-race, cli.test, CI workflow) are minor and don't affect production code. They could have been a separate commit for cleaner history, but they don't add risk. All CI checks pass. Approving. ✅ 中文说明这是一个干净、最小化的回归修复，完全实现了所描述的功能。根因是一个单字符逻辑错误（ 我在阅读 diff 之前独立提出了相同的方案。PR 与之匹配。三个真实场景测试都确认了修复有效：默认开启时创建文件（20KB，symlink 更新），显式 opt-out 时干净禁用（显示禁用说明，无误导性路径），before 构建复现了 bug（文件未创建）。 改动触及 顺手的测试基础设施改动（useStatusLine、voice-keyterms-race、cli.test、CI workflow）较小，不影响生产代码。如果拆成单独的 commit 会更干净，但不增加风险。 所有 CI 检查通过。批准 ✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
approved these changes
Jul 9, 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. ✅
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. |
qwen-code-ci-bot
requested changes
Jul 9, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
[Critical] Sandbox env-var forwarding drops QWEN_DEBUG_LOG_FILE="" — host opt-out is silently ignored inside the container.
In packages/cli/src/utils/sandbox.ts (lines 632-637, unchanged by this PR but directly affected by the default flip):
for (const envVar of [
'QWEN_DEBUG_LOG_FILE',
'QWEN_CODE_LEGACY_MCP_BLOCKING',
] as const) {
if (process.env[envVar]) {
args.push('--env', `${envVar}=${process.env[envVar]}`);
}
}The if (process.env[envVar]) check is a truthy test — an empty string "" is falsy and gets dropped. Empty string is one of the documented disable values in isDebugLogFileEnabled()'s deny-list (['', '0', 'false', 'off', 'no']).
- Before this PR: dropping
""was harmless because the sandbox-internal default was also disabled. No observable mismatch. - After this PR: the default flips to
true. NowQWEN_DEBUG_LOG_FILE=""on the host disables logging on the host, but inside the sandbox container the variable is absent, soisDebugLogFileEnabled()returnstrueand the container writes debug logs to the mounted debug directory even though the user explicitly opted out.
for (const envVar of [
'QWEN_DEBUG_LOG_FILE',
'QWEN_CODE_LEGACY_MCP_BLOCKING',
] as const) {
if (process.env[envVar] !== undefined) {
args.push('--env', `${envVar}=${process.env[envVar]}`);
}
}
Switching to !== undefined forwards empty string as --env QWEN_DEBUG_LOG_FILE=, which correctly disables logging inside the container. This also fixes the same latent bug for QWEN_CODE_LEGACY_MCP_BLOCKING.
— qwen3.7-max via Qwen Code /review
Collaborator
Suggestions — commit
|
| File | Issue | Suggested fix |
|---|---|---|
packages/cli/src/utils/sandbox.ts:635 |
The !== undefined behavioral fix for env-var forwarding has no test; a future refactor could reintroduce the truthy-check regression |
Add a targeted test verifying QWEN_DEBUG_LOG_FILE='0' is forwarded via --env to the sandbox |
packages/cli/src/ui/components/DebugModeNotification.test.tsx:25-31 |
Mock re-implements isDebugLogFileEnabled logic verbatim instead of using importOriginal + spread (the established codebase pattern) |
Use async (importOriginal) => { const actual = await importOriginal(); return { ...actual, ... }; } to get the real isDebugLogFileEnabled |
packages/cli/src/config/config.test.ts:1044-1071 |
Missing test for --debug with pre-existing truthy QWEN_DEBUG_LOG_FILE (e.g., 'custom-path') and empty string '' |
Add tests: set env to 'custom-path', pass --debug, assert it remains 'custom-path'; similarly for '' |
packages/cli/src/config/config.test.ts:1044-1071 |
New tests directly mutate process.env['QWEN_DEBUG_LOG_FILE'] without cleanup — leaks '1' into subsequent tests |
Use vi.stubEnv('QWEN_DEBUG_LOG_FILE', ...) so vi.unstubAllEnvs() handles cleanup |
packages/cli/src/ui/components/DebugModeNotification.test.tsx |
No test for unset QWEN_DEBUG_LOG_FILE or degraded mode (isDebugLoggingDegraded=true) |
Add tests covering the unset-env scenario and the degraded-warning UI branch |
packages/core/src/utils/debugLogger.ts:56-59 |
Degraded-mode .catch swallows the error entirely — no path or errno for incident diagnosis |
Capture the last error and include it in the degraded message or log once to stderr |
packages/cli/src/ui/hooks/useStatusLine.test.ts:156-160, packages/cli/src/ui/voice/voice-keyterms-race.test.ts:75-78 |
Unrelated test stabilization changes (dynamic imports with 20s timeout) bundled without explanation | Add a comment explaining why dynamic import is required, or split into a separate commit |
.github/workflows/ci.yml:446-450 |
Playwright port allocation uses TOCTOU pattern — risk of intermittent CI failures | Let Playwright choose its own port if supported, or use a retry loop |
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Collaborator
Author
Test report
Current CI note: the PR test job was still in progress when I checked. |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
doudouOUC
previously requested changes
Jul 9, 2026
Collaborator
Suggestions — commit
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No new blockers from this review. Downgraded from Approve to Comment: CI failing: Test (ubuntu-latest, Node 22.x). Existing unresolved Critical comments from prior reviews need author attention. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
# Conflicts: # packages/cli/src/ui/hooks/useStatusLine.test.ts
Revert isDebugLogFileEnabled() guard from `value === undefined` back to `!value` so the non-debug default stays opt-in as prescribed in #6600. The --debug scoping in config.ts already sets QWEN_DEBUG_LOG_FILE=1 when in debug mode, making the global default flip unnecessary. Also adds a regression test for the opt-out edge case (QWEN_DEBUG_LOG_FILE='0' + --debug should not overwrite).
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestion-level recommendations are in the Suggestion summary comment below.
— qwen3.7-max via Qwen Code /review
Collaborator
Author
|
@qwen-code /triage |
yiliang114
enabled auto-merge
July 10, 2026 09:38
Collaborator
Author
|
@qwen-code /triage |
qwen-code-ci-bot
approved these changes
Jul 10, 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. ✅
chiga0
approved these changes
Jul 10, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Code Review Overview (AI Generated)
PR: #6605 — fix(cli,core): Restore default debug log file output
Type: Bug Fix + Test
Change size: +208/-18 across 12 files
HEAD: 57aaaf9f
Findings Summary
- Critical/Major/Minor: 0
- Nit: 0
Cross-Validation
| Finding | Other Reviewer | My Assessment |
|---|---|---|
Critical: Sandbox env-var forwarding drops QWEN_DEBUG_LOG_FILE="" |
qwen-code-ci-bot | Fixed at HEAD — sandbox.ts:635 now uses process.env[envVar] !== undefined instead of truthy check. Empty string is correctly forwarded to sandbox. |
| Critical: Fix diverges from agreed direction in #6600 (not scoped to --debug) | doudouOUC | Fixed at HEAD — commit 2b3fd87 reverted isDebugLogFileEnabled() back to !value check (returns false for undefined). config.ts:1492 only sets QWEN_DEBUG_LOG_FILE=1 when debugMode && undefined. Non-debug mode leaves env unset → no file writes. |
Review
Clean, well-scoped bug fix. The three-layer approach is correct:
config.ts(activation): SetsQWEN_DEBUG_LOG_FILE=1only when--debugis active and the env var isundefined. Explicit user values ("0","false", etc.) are never overwritten.debugLogger.ts(check):isDebugLogFileEnabled()uses!value— returns false for undefined, empty, and all falsy strings. Now exported for UI consumption.DebugModeNotification.tsx/gemini.tsx(display): Conditional UI — showsLogging to: <path>when enabled,Debug log file disabled by QWEN_DEBUG_LOG_FILEwhen explicitly disabled.
The sandbox.ts fix (!== undefined) is a correct fix for a latent bug that this default flip exposed — empty string opt-outs were previously silently dropped.
Test coverage is thorough: config tests (3 scenarios), debugLogger tests (7 falsy values + unset + symlink), DebugModeNotification UI tests (enabled/disabled display), and the useStatusLine/voice-keyterms-race dynamic-import fixes prevent mock-ordering issues. The CI port collision fix (OS-assigned port for Playwright) is a good hardening.
Final Verdict
LGTM. Both prior Critical findings are verified as fixed at HEAD. The fix is correctly scoped to --debug mode, preserves explicit opt-out, and handles the sandbox forwarding edge case.
This review was generated by QoderWork AI
wenshao
reviewed
Jul 10, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Reviewed — no blockers. Suggestions are inline.
| getDebugLogPath: (sessionId: string) => `/tmp/qwen-debug/${sessionId}.txt`, | ||
| }, | ||
| isDebugLoggingDegraded: () => false, | ||
| isDebugLogFileEnabled: () => { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The vi.mock re-implements isDebugLogFileEnabled's parsing logic (trim/lowercase/falsy-list) verbatim from debugLogger.ts:38-43. If the production falsy-value list changes, the mock silently diverges and tests still pass.
Suggested change
| isDebugLogFileEnabled: () => { | |
| isDebugLogFileEnabled: vi.fn(), |
Then set per-test return values:
vi.mocked(isDebugLogFileEnabled).mockReturnValue(true); // or false— qwen3.7-max via Qwen Code /review
| 'QWEN_CODE_LEGACY_MCP_BLOCKING', | ||
| ] as const) { | ||
| if (process.env[envVar]) { | ||
| if (process.env[envVar] !== undefined) { |
Collaborator
There was a problem hiding this comment.
[Suggestion] The fix from if (process.env[envVar]) to !== undefined is a meaningful behavioral change (now forwards empty-string and '0' values into the sandbox), but no test covers QWEN_DEBUG_LOG_FILE passthrough in sandbox.test.ts. Consider adding a test that verifies --env QWEN_DEBUG_LOG_FILE=0 appears in the args when the env var is set to '0', and is absent when unset.
— qwen3.7-max via Qwen Code /review
Collaborator
Review 小结核心三层方案正确、时序无误，两个历史 Critical（sandbox 空串 opt-out、默认行为限定到 剩余问题均为测试卫生 / 范围蔓延 / 描述一致性，建议处理后合入。 建议合入前处理
Nit（可选）
结论 |
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
5 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
Restores the default session debug log file behavior: when
QWEN_DEBUG_LOG_FILEis unset,--debugwrites the session-scoped debug log and maintains thelatestalias. Explicit falsy values such as empty string, whitespace,0,false,off, andnostill disable file logging. Interactive and non-interactive debug notices now showLogging to:only when debug file logging is actually enabled; when it is explicitly disabled, they show a disabled-file-logging notice instead.Why it's needed
--debugcurrently prints a debug log path, but PR #4914 changed the unsetQWEN_DEBUG_LOG_FILEdefault to not write the file. That leaves users with a path to a log that does not exist, and it makes early startup, MCP, LSP, telemetry, and other diagnostic failures harder to investigate. This PR restores the default diagnostic behavior while preserving the explicit opt-out switch.Reviewer Test Plan
How to verify
Confirm that with
QWEN_DEBUG_LOG_FILEunset, debug logging writes the current session log and creates~/.qwen/debug/latestfor the current session. Confirm thatQWEN_DEBUG_LOG_FILE=0and other falsy values disable file logging, and that both the interactive UI notice and non-interactive stderr notice no longer show a nonexistentLogging to:path when file logging is disabled.Evidence (Before & After)
Before: the new regression tests fail against the old behavior because unset
QWEN_DEBUG_LOG_FILEdoes not callfs.appendFileor create thelatestsymlink, and the disabled-file-logging UI path still displaysLogging to:. After: those regression tests pass, along withgemini.test.tsx, build, typecheck, targeted ESLint, and whitespace checks.Tested on
Environment (optional)
Local macOS worktree. Verified with
npm install --ignore-scripts,npm run build,npm run typecheck, targeted Vitest files, targeted ESLint, andgit diff --check.Risk & Scope
cli-entry.jschanges.QWEN_DEBUG_LOG_FILE=0.Linked Issues
Resolves #6600
中文说明
What this PR does
恢复会话 debug log 文件的默认写入行为：当
QWEN_DEBUG_LOG_FILE未设置时，--debug会继续写入 session-scoped debug log，并维护latestalias。显式设置QWEN_DEBUG_LOG_FILE为空、空白、0、false、off或no时仍会关闭文件日志。交互和非交互 debug 提示现在只在文件日志实际启用时显示Logging to:路径；文件日志被显式关闭时会显示对应禁用说明。Why it's needed
--debug会提示 debug log 路径，但 PR #4914 把未设置QWEN_DEBUG_LOG_FILE的默认行为改成了不写文件，导致用户按提示去找日志时文件不存在，也削弱了排查启动、MCP、LSP、telemetry 等早期错误的能力。本 PR 恢复默认可诊断行为，同时保留显式关闭开关。Reviewer Test Plan
How to verify
确认
QWEN_DEBUG_LOG_FILE未设置时，debug logger 会写入当前 session 的 debug log，并创建~/.qwen/debug/latest指向当前 session log。确认设置QWEN_DEBUG_LOG_FILE=0或其他 falsy 值时不会写文件，交互 UI 和非交互 stderr 不再展示不存在的Logging to:路径。Evidence (Before & After)
Before：新增回归测试在旧行为下失败：未设置
QWEN_DEBUG_LOG_FILE时fs.appendFile和latestsymlink 都不会被调用；显式关闭文件日志时 UI 仍展示Logging to:路径。After：这些回归测试通过，gemini.test.tsx、build、typecheck 和 lint 也通过。Tested on
Environment (optional)
Local worktree on macOS. Verified with
npm install --ignore-scripts,npm run build,npm run typecheck, targeted Vitest files, targeted ESLint, andgit diff --check.Risk & Scope
cli-entry.jschanges.QWEN_DEBUG_LOG_FILE=0.Linked Issues
Resolves #6600