fix(core): improve debug txt diagnostics - #6277
Merged
Merged
Conversation
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. |
wenshao
marked this pull request as ready for review
July 3, 2026 15:59
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: the debug txt files had two real issues — API errors logged only the raw error message string without structured metadata (model, status code, duration, request ID), making it hard to triage failures from the log alone. Separately, Direction: aligned. Debug infrastructure should carry structured diagnostics on failure and should never dump raw request payloads. Related to #4421 but deliberately scoped to improving the existing artifact rather than adding new commands. No CHANGELOG reference needed — this is internal diagnostic tooling. Approach: three focused changes, all minimal. Since the last triage, three commits were pushed addressing maintainer review feedback: the null guard fix, preserving pre-summarized context via the new options flag, and simplifying the error report serialization path. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题：debug txt 文件有两个真实问题——API 错误日志只记录了原始错误字符串，没有结构化元数据（模型、状态码、耗时、请求 ID），导致无法从日志快速排查故障。另外 方向：对齐。debug 基础设施应该在失败时携带结构化诊断信息，且不应转储原始请求内容。关联 #4421 但刻意收窄为改进现有产物而非新增命令。内部诊断工具，不需要 CHANGELOG 条目。 方案：三处聚焦的改动，全部最小化。 上次 triage 后推送了三个 commit，处理了 maintainer 的 review 反馈：null 守卫修复、通过新选项保留预摘要上下文、简化 error report 序列化路径。 进入代码审查 🔍 — Qwen Code · qwen3.7-max |
Collaborator
Code ReviewIndependent proposal: Given "improve debug txt diagnostics," I'd expect: (1) add structured metadata to API error debug entries using existing error utility functions, (2) replace raw context serialization in error reports with a type/shape summary, (3) prevent non-user session IDs from overwriting the The PR matches this exactly. No surprises in the diff. Correctness: All four changes are sound. Reuse check: All diagnostic data extraction delegates to existing shared utilities in Post-review commits: Three commits since the original triage. (1) Null guard in Issues found: None. No critical blockers, no AGENTS.md violations. The diff is focused — 10 files, +290/-56, all directly related to the stated goal. Test ResultsCI: Local unit tests (worktree, PR branch): All 161 tests pass across the 5 affected test files, including new tests for structured diagnostics, context summarization, UUID gating, null error object handling, and pre-summarized context passthrough. Tmux test: This PR is non-UI logging behavior. Debug log output writes to 中文说明代码审查独立方案： 根据标题"improve debug txt diagnostics"，预期：(1) 使用现有错误工具函数为 API 错误日志添加结构化元数据，(2) 用类型/结构摘要替代 error report 中的原始上下文序列化，(3) 防止非用户 session ID 覆盖 PR 完全吻合。 diff 没有意外。 正确性： 四处改动都正确。 复用检查： 所有诊断数据提取都委托给已有的共享工具函数。没有新的并行实现。 Review 后提交： 原始 triage 后三个 commit。(1) 问题： 无。没有关键阻塞，没有违反 AGENTS.md。diff 聚焦——10 个文件，+290/-56，全部与目标直接相关。 测试结果CI： 本地单测（worktree，PR 分支）： 5 个受影响文件共 161 个测试全部通过。 Tmux 测试： 此 PR 为非 UI 日志行为。Debug 日志写入 — Qwen Code · qwen3.7-max |
Collaborator
|
This is a re-triage after the author pushed three commits addressing maintainer review feedback. The changes since the original triage are all improvements: the null guard in Stepping back: this PR makes debug infrastructure meaningfully better without making it more complicated. The structured diagnostics give you the exact data you need when an API call fails (which model, what status, how long, which request ID) without having to parse raw error messages. The context summarization is a privacy fix that also happens to simplify the code — the old stringify-failure fallback is gone because it's no longer needed. The UUID gating is a one-liner that prevents a real footgun where test sessions could hijack the The implementation is tight: 290 additions, 56 deletions across 10 files, all directly serving the stated goal. It reuses existing utilities instead of duplicating parsing logic. 161 tests pass across 5 affected test files. No reservations. Ships cleanly. ✅ 中文说明这是在作者推送三个 commit 处理 maintainer review 反馈后的重新 triage。自上次 triage 以来的改动都是改进： 退一步看：这个 PR 让 debug 基础设施显著改善，同时没有增加复杂度。结构化诊断在 API 调用失败时提供你需要的确切数据（哪个模型、什么状态码、耗时多久、哪个 request ID），而不需要解析原始错误消息。上下文摘要既是隐私修复也简化了代码——旧的 stringify 失败兜底路径消失了，因为不再需要。UUID 限制是一行代码，防止测试 session 劫持 实现紧凑：10 个文件 +290/-56，全部服务于目标。复用现有工具函数而非重复解析逻辑。5 个受影响文件共 161 个测试全部通过。没有顾虑。 干净交付。✅ — Qwen Code · qwen3.7-max |
qwen-code-ci-bot
previously approved these changes
Jul 3, 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. ✅
yiliang114
changed the title
wenshao
previously approved these changes
Jul 3, 2026
| durationMs: Date.now() - context.startTime, | ||
| errorType: getErrorType(error), | ||
| ...(getErrorStatus(error) !== undefined | ||
| ? { statusCode: getErrorStatus(error) } |
Collaborator
There was a problem hiding this comment.
[Suggestion] getErrorStatus(error) is called twice in this conditional spread (once for the guard, once for the value) and a third time inside getRateLimitErrorDetails. Since it's a pure function, consider computing once and reusing:
Suggested change
| ? { statusCode: getErrorStatus(error) } | |
| ...(details.statusCode !== undefined | |
| ? { statusCode: details.statusCode } | |
| : {}), |
Or compute it once before the return:
const statusCode = getErrorStatus(error);— qwen3.7-max via Qwen Code /review
| model: context.model, | ||
| durationMs: Date.now() - context.startTime, | ||
| errorType: getErrorType(error), | ||
| ...(getErrorStatus(error) !== undefined |
Collaborator
There was a problem hiding this comment.
[Suggestion] getErrorStatus(error) is called twice here — once for the conditional check and once for the value. The function does non-trivial work (property traversal and regex matching on error.message). Extract to a local variable:
Suggested change
| ...(getErrorStatus(error) !== undefined | |
| ...(getErrorStatus(error) !== undefined | |
| ? { statusCode: getErrorStatus(error) } | |
| : {}), |
→
const statusCode = getErrorStatus(error);
// ...then in the return:
...(statusCode !== undefined ? { statusCode } : {}),— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
previously approved these changes
Jul 3, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No issues found. LGTM! ✅
— 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.
No review findings. Downgraded from Approve to Comment: CI still running.
— qwen3.7-max via Qwen Code /review
wenshao
requested changes
Jul 4, 2026
Collaborator
|
@qwen-code /triage |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
No critical issues found. Build passes, all 70 tests pass, tsc and eslint clean. Downgraded from Approve to Comment: CI still running.
Minor observations for the author's consideration:
- Test coverage gap: The "should use custom suppression function" test (
errorHandler.test.ts:167) does not assertexpect(debugLoggerSpy.error).not.toHaveBeenCalled(), even though the PR introduces the spy. Adding this assertion would verify logging is actually suppressed. - Robustness: The
JSON.stringifytry/catch fallback was removed fromerrorReporting.ts. ThecontextAlreadySummarized: truepath passes raw context through — current callers are safe, but the implicit contract is fragile for future callers. - Test gap:
isApiErrorgained a null guard inquotaErrorDetection.tsbutquotaErrorDetection.test.tshas no direct test for{ error: null }.
— qwen3.7-max via Qwen Code /review
qwen-code-ci-bot
approved these changes
Jul 4, 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. ✅
wenshao
approved these changes
Jul 4, 2026
wenshao
left a comment
wenshao
left a comment
Collaborator
There was a problem hiding this comment.
Both prior Critical findings are resolved in this revision: isApiError now guards against error.error === null (verified via new errorHandler.test.ts case that passes { error: null } through the handler), and the debug-log symlink update is gated by an anchored UUID regex that rejects path-traversal payloads.
Build passes; all tests in the four files directly exercising the PR (errorHandler, debugLogger, errorReporting, telemetry SDK) pass (129 tests). The turn.test.ts failure (@xterm/headless not installed) reproduces on main and is unrelated to this PR.
The Suggestion-level findings still open on the PR (silent UUID rejection, getRequestId direct test, ContextSummary type contract, buildDiagnostics absent-branch test, isApiError null test) are non-blocking and can be addressed in follow-ups.
— qwen3.7-max via Qwen Code /review
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
3 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
This PR makes the existing debug txt files more useful and safer for local troubleshooting. API failures now add a structured diagnostic object to the debug log with the model, duration, error type, HTTP status, provider code/message, request id, and transport when those fields are available. Error reports now summarize supplied context instead of serializing prompts, tool output, or other raw request context into the debug log. The
latestdebug-log alias now only tracks UUID-shaped user session logs, so internal test/sink sessions cannot replace the pointer to the latest real session.Why it's needed
Issue #4421 originally explored a broader diagnostic command flow, but the immediate useful artifact already exists: the per-session debug txt file. The remaining problem is that these files can miss concise API/SSE metadata while also containing too much raw context. This narrows the fix to improving the existing local artifact instead of adding a new command surface.
Reviewer Test Plan
How to verify
Trigger or unit-test an OpenAI-compatible API error and confirm the debug logger receives a structured diagnostic object without request contents. Trigger
reportErrorwith context and confirm the debug log contains onlycontextSummary, not raw prompt text. Set a debug log session id likelog-to-span-sink-testand confirm it does not update thelatestalias, while a UUID session still does.Evidence (Before & After)
N/A; this is non-UI logging behavior covered by unit tests.
Tested on
Environment (optional)
Local Node.js v22.22.0 / npm 10.9.4 in a dedicated git worktree.
Risk & Scope
ERROR_REPORTno longer includes raw context in debug txt files by default; this intentionally favors privacy and shareability over full inline payload dumps./debug,/debug copy,/bug collect, or diagnostic bundle command is added here. Larger command UX and redacted copy workflows remain future work.Linked Issues
Related to #4421
中文说明
这个 PR 做了什么
这个 PR 让现有 debug txt 文件更适合本地问题排查，也更安全。API 失败现在会在 debug log 里额外写入结构化诊断对象，包括可获得的 model、duration、error type、HTTP status、provider code/message、request id 和 transport。Error report 现在只记录传入 context 的摘要，不再把 prompt、tool output 或其他原始请求上下文序列化进 debug log。
latestdebug-log alias 现在只跟踪 UUID 形态的用户 session log，避免内部测试或 sink session 覆盖真实最近 session 的指针。为什么需要
#4421 最初讨论的是更大的诊断命令流程，但眼下最有用的本地材料已经存在：每个 session 的 debug txt 文件。现在的问题是这些文件缺少足够简洁的 API/SSE 元数据，同时又会包含过多原始上下文。这个 PR 将范围收窄为改进现有本地文件，而不是增加新的命令入口。
Reviewer Test Plan
How to verify
触发或用单测覆盖 OpenAI-compatible API error，确认 debug logger 收到结构化诊断对象，并且不包含请求内容。用带 context 的
reportError触发错误，确认 debug log 只有contextSummary，没有原始 prompt 文本。设置log-to-span-sink-test这样的 debug log session id，确认它不会更新latestalias；UUID session 仍会更新。Evidence (Before & After)
N/A；这是非 UI 日志行为，由单元测试覆盖。
Tested on
Environment (optional)
本地 Node.js v22.22.0 / npm 10.9.4，使用独立 git worktree。
Risk & Scope
ERROR_REPORT默认不再把 raw context 写入 debug txt；这是为了优先保证隐私和可分享性。/debug、/debug copy、/bug collect或诊断 bundle 命令；更大的命令交互和 redacted copy workflow 留给后续。Linked Issues
Related to #4421