feat(cli): add session path status command#4124
Conversation
| ): Promise<string | undefined> { | ||
| const files = await listLogFiles(logDir, (name) => | ||
| /^openai-.*\.json$/.test(name), | ||
| ); |
There was a problem hiding this comment.
[Critical] findLatestOpenAILogForSession iterates and fully JSON-parses every openai-*.json file in the log directory with no upper bound on the number of files scanned. Each file is loaded entirely into memory and parsed, only to check a single context.sessionId field.
Over weeks of usage, the OpenAI log directory can accumulate hundreds of large files (containing base64-encoded images, tool results, etc.), causing this interactive slash command to degrade to tens of seconds of blocking I/O. OpenAILogger.getLogFiles() in packages/core/src/utils/openaiLogger.ts already supports a limit parameter — this new code reimplements the same directory listing without it.
| ); | |
| const MAX_SCAN = 100; | |
| const files = await listLogFiles(logDir, (name) => | |
| /^openai-.*\.json$/.test(name), | |
| ); | |
| const recentFiles = files.slice(0, MAX_SCAN); | |
| for (const file of recentFiles) { |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in fe50141 by limiting the lookup to the 100 most recent OpenAI log files. I also added a regression test that asserts only 100 JSON files are read during the lookup.
| return []; | ||
| } | ||
| throw error; | ||
| } |
There was a problem hiding this comment.
[Suggestion] listLogFiles catches only ENOENT and re-throws all other filesystem errors (e.g., EACCES, EPERM). Since collectSessionPathInfo does not wrap the findLatestOpenAILogForSession call in a try/catch, a permission error on the OpenAI log directory will cause the entire /status paths command to fail — the user gets no output about any session paths, not just the log section.
OpenAILogger.getLogFiles() (same module, line ~190) handles all errors gracefully by returning [], which would be a more consistent approach for a diagnostic command.
| } | |
| } catch { | |
| return []; | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in fe50141. listLogFiles now degrades to an empty result for directory read failures, while logging non-ENOENT failures through the debug logger so /status paths can still render the rest of the session paths.
| const parsed: unknown = JSON.parse(raw); | ||
| if (hasContextSessionId(parsed, sessionId)) { | ||
| return file; | ||
| } |
There was a problem hiding this comment.
[Suggestion] The bare catch {} block swallows all error types — not just SyntaxError from malformed JSON, but also EACCES (permission denied), ENOMEM (out of memory reading large files), and EIO (disk I/O errors). The comment says "Ignore malformed or concurrently-written log files" but the implementation is far broader.
This silently drops potentially important signals while yielding no diagnostic output. If a log file is unreadable due to a configuration/permission issue, the user gets no indication.
| } | |
| } catch (err) { | |
| if ( | |
| err instanceof SyntaxError || | |
| (err as NodeJS.ErrnoException).code === 'ENOENT' | |
| ) { | |
| continue; | |
| } | |
| debugLogger.warn('Error reading log file %s: %o', file, err); | |
| } |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in fe50141. The per-file catch now only silently skips malformed JSON and concurrently missing files. Other read errors are logged via the SESSION_PATHS debug logger before continuing the scan.
| const parts = promptId.split('#'); | ||
| if (parts.length === 3 && parts[0]) { | ||
| return parts[0]; | ||
| } |
There was a problem hiding this comment.
[Suggestion] sessionIdFromPromptId uses parts.length === 3 as an exact match for subagent-format promptIds ({sessionId}#{subagentId}#{turnCounter}). If a subagent name ever contains a # character, the resulting 4+ segments will not match, and sessionId silently drops to undefined.
Since the first #-delimited segment is always the session UUID (which never contains #), the check can be made defensive:
| } | |
| if (parts.length >= 3 && parts[0]) { |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in fe50141. sessionIdFromPromptId now accepts 3+ hash-delimited segments and keeps the first segment as the session id, with coverage for extra separators in subagent prompt ids.
| function contextForPromptId( | ||
| promptId: string | undefined, | ||
| ): OpenAILogContext | null { | ||
| const trimmedPromptId = promptId?.trim(); |
There was a problem hiding this comment.
[Suggestion] contextForPromptId can produce log entries with only { promptId } and no sessionId when the promptId is a bare UUID (no ######## delimiter, no subagent #...#...# pattern). This occurs during session initialization before the first turn adds the ########counter suffix.
Logs written during this initialization window will be invisible to findLatestOpenAILogForSession, which searches by context.sessionId. /status paths will report "none yet" even though matching logs exist on disk.
Consider adding a UUID fallback in sessionIdFromPromptId: if promptId matches a UUID regex, return the whole promptId as the sessionId.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in fe50141. Bare UUID prompt ids now map back to the same value as sessionId, with OpenAILogger coverage for session-initialization style prompt ids.
| : undefined; | ||
| const transcriptPath = config?.getTranscriptPath() || ''; | ||
| const debugLogPath = | ||
| config?.getDebugMode() === true && sessionId !== 'unknown' |
There was a problem hiding this comment.
[Suggestion] config?.getDebugMode() === true uses strict boolean comparison, but the rest of the codebase consistently uses truthy checks (e.g., if (config.getDebugMode()) in Footer.tsx, KeypressContext.tsx, DebugModeNotification.tsx, gemini.tsx).
If getDebugMode() ever returns a truthy non-boolean value (e.g., if the config layer evolves), the debug log path will be silently omitted from /status paths output, while debug logs are still being written — causing silent diagnostic misdirection.
| config?.getDebugMode() === true && sessionId !== 'unknown' | |
| config?.getDebugMode() && sessionId !== 'unknown' |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Accepted in fe50141 for consistency with the rest of the codebase. getDebugMode currently returns boolean, but /status paths now uses the same truthy check style as the existing debug-mode call sites.
| if (!context || typeof context !== 'object') { | ||
| return false; | ||
| } | ||
| return (context as { sessionId?: unknown }).sessionId === sessionId; |
There was a problem hiding this comment.
[Suggestion] hasContextSessionId only checks context.sessionId, but contextForPromptId (introduced in the same PR) can produce log entries with only context.promptId and no sessionId field. When that happens, findLatestOpenAILogForSession will skip past a matching log file, and /status paths will report "none yet" even though the file exists.
Consider adding a context.promptId fallback match as defense-in-depth:
| return (context as { sessionId?: unknown }).sessionId === sessionId; | |
| const ctx = context as { sessionId?: unknown; promptId?: unknown }; | |
| return ctx.sessionId === sessionId || ctx.promptId === sessionId; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in fe50141. hasContextSessionId now also matches context.promptId against the current session id, and sessionPaths has regression coverage for logs that only contain promptId.
| const openAILoggingEnabled = | ||
| contentGeneratorConfig?.enableOpenAILogging === true; | ||
| const latestOpenAILog = | ||
| openAILoggingEnabled && sessionId !== 'unknown' |
There was a problem hiding this comment.
[Suggestion] The three sessionId === 'unknown' short-circuit branches (L48, L53, L56) that skip OpenAI log scanning, debug log path, and plan file path respectively — are untested. sessionPaths.test.ts always uses real session IDs. If getSessionId() returns empty and stats.sessionId is undefined (e.g. session not yet fully initialized when /status paths is called), the behavior is unverified.
Adding a test with getSessionId: vi.fn().mockReturnValue('') and session.stats.sessionId: undefined would cover this edge case.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Stepping back from the code: I'd like to align on the product surface before this lands. The PR body frames this as a support workflow — "maintainers can ask users to run one command and get the files needed to inspect a session." But the design seems to point at a different user, a developer working on qwen-code itself with all flags enabled by default.
Three places where the design contradicts the stated target:
- The command lives inside a running session, so an end user whose qwen-code just crashed — exactly when a maintainer would ask for artifacts — can't invoke it at all.
- Rows are hidden when the underlying feature flag is off. An end user reporting a bug doesn't know
QWEN_DEBUG_LOG_FILEorenableOpenAILoggingexist; the command quietly hides the very thing the maintainer would need them to turn on. - It lives under
/status(qwen-code's developer info dump), not under/exportwhere users naturally look when asked for artifacts to share.
If the real target is the end-user-reporting-a-bug flow, the primitive we want is closer to /export support-bundle — one command, one zip in CWD with transcript + debug log + recent OpenAI logs + system info, ready to attach to a GitHub issue. That keeps /export as the single place users go for session artifacts, and the user never has to know about feature flags or paste filesystem paths.
If the real target is "developers debugging qwen-code," that's a fine goal in itself, but then the PR should say so and we should talk about whether the slash command surface is the right home for developer-only diagnostics versus a clearly-namespaced developer surface.
Could you say more about who you had in mind and how they'd actually use this? I'd rather hold off merging until we've aligned on the target user — I have separate notes on code-correctness issues in the follow-up commits, but those only matter once we agree this is the right shape.
Verdict
REQUEST_CHANGES — blocking on the target-user question.
|
Thanks, this is a very fair point. I agree the PR body framed the motivation poorly. The target I had in mind is not really the "end user whose session crashed and needs to attach a complete support bundle to an issue" flow. For that case, I agree The use case here is closer to self-diagnosis and productivity for a Qwen Code user while the session is still running. Two concrete examples:
So I would reframe this as a read-only discoverability/diagnostic command for active-session self-service, not as the full support-reporting workflow. With that framing, hiding rows for artifacts that are not enabled/present is intentional: the command shows the concrete files that currently exist for this session, rather than guiding the user through enabling every possible diagnostic artifact. I can update the PR description/docs to make this target clearer and avoid presenting it as a maintainer support-bundle workflow. |
| ): Promise<SessionPathInfo> { | ||
| const config = context.services.config; | ||
| const sessionId = | ||
| config?.getSessionId() || context.session.stats.sessionId || 'unknown'; |
There was a problem hiding this comment.
[Critical] sessionId = 'unknown' 降级路径及 L48/L53/L58 三处短路分支均无测试覆盖。当 getSessionId() 返回 falsy 且 context.session.stats.sessionId 也为空时，sessionId 回退为 'unknown'，触发 OpenAI 日志扫描、debug 日志路径、plan 文件路径全部跳过。所有现有测试均 mock 了真实 UUID，此完整优雅降级路径完全未被验证，回归 bug 无法被检测到。
| config?.getSessionId() || context.session.stats.sessionId || 'unknown'; | |
| // 添加测试：构造 getSessionId() 返回 undefined、stats.sessionId 为空的 CommandContext， | |
| // 断言输出包含 "Session ID: unknown"，省略 "Debug log:" 和 "Plan file:"， | |
| // spy fs.readdir 确认日志目录从未被扫描。 |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in ff18330. Added regression coverage for the unknown fallback with empty getSessionId() and no stats.sessionId; it asserts Session ID: unknown, omits derived debug/plan paths, and verifies OpenAI log scanning (fs.readdir) plus plan existence probing (fs.access) are skipped.
| return entries | ||
| .filter((entry) => entry.isFile() && predicate(entry.name)) | ||
| .map((entry) => path.join(dir, entry.name)) | ||
| .sort() |
There was a problem hiding this comment.
[Suggestion] listLogFiles 对所有匹配文件完整排序（O(n log n)），但仅使用前 100 个。大型日志目录（数千文件）会浪费 CPU 和内存。
| .sort() | |
| .sort() | |
| .slice(-OPENAI_LOG_SCAN_LIMIT); |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in ff18330. listLogFiles now keeps a bounded descending list of the most recent 100 matching paths while walking directory entries, so it no longer materializes and sorts every matching log file before discarding all but 100.
| .sort() | ||
| .reverse(); | ||
| } catch (error) { | ||
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { |
There was a problem hiding this comment.
[Suggestion] listLogFiles 的 ENOENT catch 分支（静默返回 []）无测试覆盖。仅测试了 EACCES 权限拒绝路径；日志目录不存在是启用日志前的正常情况，此路径的回归会破坏整个 /status paths 命令。
| if ((error as NodeJS.ErrnoException).code !== 'ENOENT') { | |
| // 添加测试：mock fs.readdir 拒绝 { code: 'ENOENT' }， | |
| // 断言 collectSessionPathInfo 仍正常返回且无 debug 警告。 |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in ff18330. Added ENOENT regression coverage for the normal missing-log-directory path; /status paths still renders the session output and reports Latest for session: none yet, while the implementation keeps ENOENT silent.
| ); | ||
|
|
||
| const prompt_id = Math.random().toString(16).slice(2); | ||
| const prompt_id = randomUUID(); |
There was a problem hiding this comment.
[Suggestion] prompt_id = randomUUID() 产生裸 UUID，导致日志 context.sessionId 被设为 prompt UUID 而非真实 sessionId。/status paths 用 config.getSessionId() 搜索日志时无法匹配此路径的日志条目。其他所有执行路径（交互式、非交互式会话、ACP）使用 {sessionId}########{counter} 格式，可被正确匹配。
| const prompt_id = randomUUID(); | |
| const prompt_id = config.getSessionId() + '########0'; |
— DeepSeek/deepseek-v4-pro via Qwen Code /review
There was a problem hiding this comment.
Addressed in ff18330. The non-interactive prompt id now uses the current session id format ({sessionId}########0) via createNonInteractivePromptId(config.getSessionId()), with a focused unit test for the format. This keeps OpenAI log context.sessionId aligned with the /status paths lookup.
|
Follow-up update:
Validation run locally: cd packages/cli && npx vitest run src/utils/sessionPaths.test.ts src/ui/commands/aboutCommand.test.ts src/gemini.test.tsx
cd packages/core && npx vitest run src/utils/openaiLogger.test.ts src/agents/runtime/agent-core.test.ts
npm run build
npm run typecheck
npm run lint
git diff --check |
wenshao
left a comment
There was a problem hiding this comment.
Additional observations:
-
Subagent ID entropy regression (
agent-core.ts:275):randomUUID().replace(/-/g, '').slice(0, 6)yields 6 hex chars (24 bits, ~16.7M combinations). The oldMath.random().toString(36).slice(2, 8)yielded 6 base-36 chars (~31 bits, ~2.2B combinations). Crypto quality improved but collision resistance dropped ~130x. Considerslice(0, 8)(32 bits) to exceed the old entropy. -
formatSessionPathInfonot directly tested (sessionPaths.test.ts): The formatting logic (section titles, 2-space indentation, blank-line separators between sections) is only exercised indirectly throughcollectSessionPathInfo. A dedicated unit test with known input would catch indentation/separator regressions.
— mimo-v2.5-pro via Qwen Code /review
| } | ||
| } | ||
|
|
||
| export function createNonInteractivePromptId(sessionId: string): string { |
There was a problem hiding this comment.
[Suggestion] createNonInteractivePromptId is exported but always returns the same deterministic string ({sessionId}########0) for every call within a session. The function name implies uniqueness, and downstream log-analysis tools that group by prompt_id will merge distinct non-interactive requests into one.
Consider either renaming to createSessionCorrelationId to set correct expectations, or appending a monotonic counter:
| export function createNonInteractivePromptId(sessionId: string): string { | |
| let nonInteractivePromptCounter = 0; | |
| export function createNonInteractivePromptId(sessionId: string): string { | |
| return `${sessionId}########${nonInteractivePromptCounter++}`; | |
| } |
— mimo-v2.5-pro via Qwen Code /review
| } | ||
|
|
||
| export function createNonInteractivePromptId(sessionId: string): string { | ||
| return `${sessionId}########0`; |
There was a problem hiding this comment.
[Suggestion] The ######## delimiter is hardcoded here but defined as MAIN_SESSION_PROMPT_ID_DELIMITER (unexported) in openaiLogger.ts. If the delimiter changes in core, this CLI code will silently produce IDs that sessionIdFromPromptId cannot parse, breaking OpenAI log session correlation.
Consider exporting the constant from core and importing it here:
import { MAIN_SESSION_PROMPT_ID_DELIMITER } from '@qwen-code/qwen-code-core';— mimo-v2.5-pro via Qwen Code /review
| } | ||
|
|
||
| export function resolveOpenAILogDir( | ||
| customLogDir?: string, |
There was a problem hiding this comment.
[Suggestion] resolveOpenAILogDir is a newly exported public API (re-exported from core/index.ts) with non-obvious path-resolution semantics. Consider adding JSDoc:
| customLogDir?: string, | |
| /** | |
| * Resolve the OpenAI log directory path. | |
| * | |
| * - If `customLogDir` is omitted, defaults to `<cwd>/logs/openai`. | |
| * - Supports `~` expansion via `os.homedir()`. | |
| * - Relative paths are resolved against `cwd` (or `process.cwd()`). | |
| * | |
| * @param customLogDir Optional custom directory (absolute, relative, or `~/...`). | |
| * @param cwd Working directory for relative-path resolution. | |
| */ | |
| export function resolveOpenAILogDir( |
— mimo-v2.5-pro via Qwen Code /review
|
Addressed both observations in a472595:
Validation: cd packages/cli && npx vitest run src/utils/sessionPaths.test.ts
cd packages/core && npx vitest run src/agents/runtime/agent-core.test.ts
cd packages/cli && npx prettier --check src/utils/sessionPaths.test.ts
cd packages/core && npx prettier --check src/agents/runtime/agent-core.ts
git diff --check |
tanzhenxin
left a comment
There was a problem hiding this comment.
Review
Re-review after the pivot, and this clears my earlier block. That review was a design question — whether /status paths was a maintainer support-bundle workflow (wrong shape) or something else. You answered it: this is an active-session self-diagnosis and artifact-discoverability command for the user themselves, with /export support-bundle left as the separate primitive for support reporting. Under that framing the previously-suspect behaviors are correct by design — only surfacing artifacts that are present or that the user opted into is exactly right for a "what's relevant to this session" command, and /status is a reasonable home for read-only diagnostics.
The load-bearing change — making the non-interactive prompt id session-correlated so the OpenAI logs can be tied back to the current session — is correct end to end and consistent with the interactive and stream-json id formats. An independent review pass found no functional regressions. The debug-log row appearing only under --debug is intentional surfacing behavior, not a correctness issue. The remaining nits (a dead fallback branch, English-only output labels, one thin test) are low-severity polish and don't block.
Verdict
APPROVE — Design concern from the prior review is resolved and the session-correlation fix is solid. Minor polish items noted are non-blocking.
wenshao
left a comment
There was a problem hiding this comment.
Re-review Summary
All previously reported issues have been addressed in the latest commits. The one remaining suggestion — the ######## session delimiter being defined as an unexported constant in core while hardcoded in CLI — was already noted in a prior inline comment.
No new high-confidence findings. The implementation is solid: tests pass (80/80), CI is green, and error handling covers the main failure paths.
Verdict: LGTM from a code perspective. The open delimiter coupling suggestion is non-blocking.
— DeepSeek/deepseek-v4-pro via Qwen Code /review
|
Hi @wenshao, the requested changes and follow-up observations should now be addressed, and CI is green. Could you please re-review and approve when you have a chance? The PR is still blocked by the earlier changes-requested review. Thanks! |
✅ Local Verification Report — PASSVerified this PR by building and driving the actual CLI at its real surfaces (interactive TUI + non-interactive), not by re-running test suites. Environment
What was verified
Sample — interactive
|
Summary
/status pathsto print the current session transcript, debug log, plan file, and OpenAI log paths when those artifacts are enabled or present./statusis the right home for read-only active-session diagnostics.Validation
/status pathsreturns plain text paths for the current session artifacts.qwen-code, then run/status paths./status pathsto see those paths appear.Scope / Risk
Testing Matrix
Testing matrix notes:
Linked Issues / Bugs