Skip to content

fix(codex,claudecode): surface scanner.Err in JSONL session readers - #1660

Open
hi-neason wants to merge 1 commit into
chenhg5:mainfrom
hi-neason:fix/session-scanner-err-truncation
Open

fix(codex,claudecode): surface scanner.Err in JSONL session readers#1660
hi-neason wants to merge 1 commit into
chenhg5:mainfrom
hi-neason:fix/session-scanner-err-truncation

Conversation

@hi-neason

Copy link
Copy Markdown

Summary

The session transcript readers in agent/codex/list.go and agent/claudecode/claudecode.go all use bufio.Scanner with a fixed 256 KB buffer but never checked scanner.Err() after the loop. When a single JSONL line (a large tool output, pasted file content, image data) exceeded the buffer, Scan() returned false with bufio.ErrTooLong and the function quietly returned whatever it had parsed so far with a nil error — so the session list showed a wrong message count / empty summary, and /history returned a truncated conversation without telling the caller.

The correct pattern already exists in this repo at agent/codex/context_usage.go:258, which checks scanner.Err() and propagates it; these four readers were the inconsistent ones.

Type of change

  • Bug fix (non-breaking change that fixes an issue)

Testing

Automated tests added in this PR

  • agent/codex/list_scanner_test.go
    • TestGetSessionHistory_OversizedLineReturnsError — writes a 300 KB JSONL line, expects errors.Is(err, bufio.ErrTooLong).
    • TestParseCodexSessionFile_OversizedLineReturnsNil — expects the unreadable session to be skipped.
    • TestGetSessionHistory_HappyPath — sanity check that normal files still parse.
  • agent/claudecode/session_scanner_test.go
    • TestGetSessionHistory_OversizedLineReturnsError — same for Agent.GetSessionHistory.
    • TestScanSessionMeta_OversizedLineDoesNotPanic — verifies the best-effort summary path.
    • TestGetSessionHistory_HappyPath.

For bug fixes only — regression test

  • Regression tests: TestGetSessionHistory_OversizedLineReturnsError (codex + claudecode), TestParseCodexSessionFile_OversizedLineReturnsNil.
  • Manual verification this test catches the regression:
    • Reverted the fix locally; the regression tests failed as expected (expected error when a line exceeds scanner buffer, got nil).

Critical User Journeys (CUJ) impact

  • B — session lifecycle (/new /switch /list /history etc.): the bug surfaces in /list summaries and /history output.

  • go test ./core/ -run TestCUJ passes locally.

  • No existing user-visible flow is altered beyond surfacing a previously-silent error.

Manual / user-visible behavior change

When a session transcript contains a line larger than 256 KB, /history now returns an explicit error rather than a silently truncated conversation, and the offending session is omitted from /list (codex) with a warning log instead of appearing with a wrong message count.

Checklist (reviewer will verify)

  • go build ./... passes
  • go test ./... passes (with -race where concurrency is touched — not applicable to this change; package tests run with -tags no_web matching the Makefile default)
  • AGENTS.md Pre-Commit Checklist items are satisfied
  • No new hardcoded platform/agent names in core/
  • i18n strings have all-language translations (no new user-facing strings)
  • No secrets / credentials in source

Related

  • Same-class fix elsewhere in this repo: agent/codex/context_usage.go:258 already checks scanner.Err().

The session transcript readers in agent/codex/list.go and
agent/claudecode/claudecode.go all used bufio.Scanner with a fixed
256 KB buffer but never called scanner.Err() after the loop. When a
single JSONL line (large tool output, file content, image data)
exceeded the buffer, Scan() returned false with ErrTooLong and the
functions silently returned a truncated history / wrong message count
with a nil error.

- getSessionHistory (both agents): return bufio.ErrTooLong wrapped so
  callers can detect an incomplete result.
- parseCodexSessionFile: log a warning and return nil so the
  unreadable session is skipped instead of listed with a bogus
  summary/count.
- scanSessionMeta: log a warning and keep the best-effort partial
  summary, since listSessions tolerates incomplete metadata.

Same pattern is already handled correctly in
agent/codex/context_usage.go.

Co-Authored-By: Claude <noreply@anthropic.com>
@hi-neason
hi-neason requested a review from chenhg5 as a code owner August 10, 2026 02:05

@chenhg5 chenhg5 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

结论: Approve

总体判断: 一个 critical silent-failure fix——bufio.Scanner 在 line 超过 256 KB buffer 时返回 bufio.ErrTooLong 但函数没检查 scanner.Err(),导致 session 列表显示错误的 message count、/history 返回 truncated conversation 而不报错。修复后错误会 propagate,session 解析失败时上游可以选择 skip / log / 报告。建议合入。

Review 范围:

  • 看了 agent/codex/list.goagent/claudecode/claudecode.go 中 4 个 reader 的 scanner.Err() 检查添加。
  • 看了新增 6 个测试:oversized line + happy path + no-panic for best-effort summary。
  • CI: run 31348828237 全绿。

✅ 做得好的地方:

  • 修复 silent failure:原行为是「/history 返回 truncated content 不报错」——这是 user-visible 的 silent data loss。修复后 caller 能区分「session 完整但短」vs「session 有大行被 truncated」。
  • 沿用既有 pattern:作者明确指出 agent/codex/context_usage.go:258 已正确检查 scanner.Err(),4 个新改动的 reader 与之对齐。这是「find the inconsistent ones」型的 fix,最小侵入。
  • Best-effort 路径(scanSessionMeta)不 panicTestScanSessionMeta_OversizedLineDoesNotPanic 测试覆盖「summary 解析失败但不应让 /list 完全崩溃」。这种 best-effort 行为对 /list summary 这种「次要信息」是正确的——主要 /history 才需要 strict error propagation。
  • ParseCodexSessionFile 在 oversize 时返回 nil 而非 partialTestParseCodexSessionFile_OversizedLineReturnsNil 验证「unreadable session 被 skip」——避免显示 wrong message count 的 phantom session。

🟠 建议改进(不阻塞):

  • 256 KB buffer size 是 hardcoded:当前 bufio.NewScanner(file) 用默认 64 KB 还是 256 KB?建议作者显式 scanner.Buffer(make([]byte, 0, 64*1024), 256*1024) 并加注释解释为什么选这个值。如果 codex / claudecode 真实场景下有大图像 base64(>256KB 是常态),可以调到 4 MB。
  • scanner.Err() propagation 应该 wrap 上下文return errbufio.ErrTooLong 直接抛出,但 caller 不知道是哪个 file。建议 return fmt.Errorf("read session %s: %w", path, err) 把 path 上下文带上——方便 debug 是哪个 session 触发的。
  • ParseCodexSessionFile 在 oversize 时返回 nil vs error:当前是 nil,但 caller 可能希望知道「这个 session 是因为什么被 skip 的」。建议返回 (nil, error) 而非 (nil, nil),让 caller 能 log「skip session X due to oversized line」。
  • 是否需要 scanner.MaxScanTokenSize 而不是固定 bufferbufio.MaxScanTokenSize 是 64 KB 最大值,但作者用了 buffer 显式扩展到 256 KB+——OK,但建议加注释「real-world tool output / pasted image base64 can exceed 256 KB; consider increasing buffer or using bufio.Reader with manual line splitting」。

🔵 可选优化:

  • 可以加一个 fuzz test:随机生成 line sizes 0-1MB,验证 reader 不 panic。低优先

Testing / Risk:

  • 已看到的验证: 6 个新测试覆盖 4 reader × {oversized + happy path} + best-effort no-panic;fix revert 后 regression tests 都红(作者 self-verify);CI 全绿。
  • Blast radius: 仅 4 个 reader 函数,错误传播路径由 caller 处理(已存在)。无 production 行为破坏。

Next step:

  • 建议 owner 直接 merge。Silent failure 是 critical UX bug,修复 scope 小、测试全、对正常路径无影响。可以现在合。
  • post-merge 验证: 用真实 codex / claudecode session + 故意触发大行(粘贴大 image base64)跑 /history,确认 caller 能看到明确 error 而非 silent truncation。

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants