feat(cli): add session path status command#4124
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| ): 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
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
| 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
| .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
| ); | ||
|
|
||
| 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
4 participants
Summary
/status pathsto print the current session transcript, debug log, plan file, and OpenAI log paths when those artifacts are enabled or present.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