Skip to content

fix(core): make /history entry truncation configurable - #1291

Merged
chenhg5 merged 2 commits into
chenhg5:mainfrom
AaronZ345:fix/history-max-len
Jun 22, 2026
Merged

fix(core): make /history entry truncation configurable#1291
chenhg5 merged 2 commits into
chenhg5:mainfrom
AaronZ345:fix/history-max-len

Conversation

@AaronZ345

Copy link
Copy Markdown
Contributor

Summary

  • add [display].history_max_len with project-level override support for /history entries
  • raise the default hard-coded truncation from 200 to 1000 characters and allow 0 to disable truncation
  • apply the shared truncation helper to both text /history output and history cards

Fixes #1208.

Tests

  • go test ./core ./config
  • go test -tags no_web ./cmd/cc-connect
  • git diff --check

Notes

  • go test ./cmd/cc-connect without no_web needs generated web/dist in a clean worktree.
  • go test -tags no_web ./... was also attempted locally and still hits existing environment/flaky failures outside this patch path: agent/codex mock CLI/runtime-config timeouts, agent/iflow pending-tool timeout, daemon launchd status on macOS, and one release_local tempdir cleanup flake.

@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

总体判断:
干净实现 #1208 — 1 个新 config 字段 + 1 个共享 helper,跨 cmdHistory (text 输出) + renderHistoryCard (rich card) 两条路径统一走 truncateHistoryEntry,边界处理 (multi-byte rune, 0 = disabled, 负数 validation) 全部覆盖。逻辑等价、cross-platform 一致、test 完整。本 PR 实质向后兼容默认值是 200,默认跳到 1000 算行为变更,需在 release notes 点出 (P2 follow-up)。

Review 范围:

  • 看了 8 files / +135/-12 diff (config/config.go +22, config/config_test.go +58, core/engine.go +23/-10, core/engine_test.go +25, cmd/cc-connect/main.go +4, config.example.toml +1, docs/usage.md +/-1, docs/usage.zh-CN.md +/-1)。
  • 重点关注 correctness (multi-byte 安全) / backward compat (默认值 + 0 行为) / cross-platform 一致性 / validation (负数拒绝) / SUPERSEDED 风险。

✅ 做得好的地方:

  • 共享 helper 设计:truncateHistoryEntry(content, maxLen) 抽出来后,cmdHistory + renderHistoryCard 都改用同一函数,消除 2 处重复 inline len([]rune(content)) > 200 逻辑。修一处全改,后续要换 truncation 策略 (例如 rune → byte) 只改 1 处。
  • 指针语义清晰:HistoryMaxLen *int 三态 (nil/0/N) 在 Go 习惯里干净。main.go 取 EffectiveHistoryMaxLen 局部变量地址传 &historyMaxLen,与 ThinkingMaxLen/ToolMaxLen (值类型,0 已涵盖 "default + disabled" 二义) 不一致是有意为之 — 这里是 *int,理由充分,doc 注释也明说了 "nil = default, 0 = no truncation"。
  • multi-byte 安全:核心 truncate 用 []rune(content)[:maxLen],不是 byte slice,中文/emoji 边界不会切到一半。TestTruncateHistoryEntry 显式 assert truncateHistoryEntry("你好世界", 2) = "你好..." 防 regression。
  • validation 早 fail:validateDisplayConfig 拒绝 *HistoryMaxLen < 0,错误信息精准 (projects[0].display.history_max_len must be >= 0),不让坏 config 进 runtime。
  • GetGlobalSettings 暴露:history_max_len 加进管理 API 返回,web UI/CLI 可读到 (与 thinking_max_len/tool_max_len parity)。
  • 测试组织:4 sub-case TestEffectiveHistoryMaxLen (default/global/project overrides/0 disables) + 1 sub-case TestValidateProjectDisplayConfig (negative rejected) + TestTruncateHistoryEntry (ASCII/Chinese/0 disabled) + TestEngineHistoryEntryMaxLen (default + custom),table-driven 风格,新增/修改可读性都很好。
  • 双语 docs 同步:docs/usage.md + docs/usage.zh-CN.md 都更新 /history 行,config.example.toml 中英双语注释保持项目风格。
  • reloadConfig 也 wire:SIGHUP reload 时 reloadConfig 同样调 EffectiveHistoryMaxLen + SetDisplayConfigHistoryMaxLen,配置热重载路径完整 (不只启动时)。

🚨/🔴 必须处理:

  • 未发现。

🟠 建议改进 (P2, 不阻塞合并):

  • CHANGELOG 缺条目:本 PR 是 public-facing user-visible change (default 200 → 1000 + 新 config 字段 + 0 可禁用),应在 CHANGELOG.md unreleased 段加 ### Fixed 条目。建议文字:
    - **\/history` entry truncation configurable** — new `[display].history_max_len` (default 1000, 0 = no truncation). Default raised from 200. (PR #1291)`
  • 默认值 200 → 1000 是行为变更:PR title 和 description 说 "configurable" 暗示向后兼容,实际默认 5x 提升。若有用户依赖旧 200 cap (legacy bot 消息长度限制、IM 平台单条消息字数限制等),会看到 history 输出变长。release notes 应明确点出 (类似 #1340 carry-over 思路)。可在 CHANGELOG 同条目合并,或单独 bullet。
  • PR body 描述精度:现 description 说 "raise the default hard-coded truncation from 200 to 1000",这点已经写进 body 了,👍。但 Fixes #1208 description 在 issue 里也应同步 (let author 决策)。
  • TestTruncateHistoryEntry 可补一个 emoji 边界 case:现有 ASCII + 4-byte 中文,建议加 1 个 truncateHistoryEntry("👨‍👩‍👧 中文", 2) 测试,验证 4-byte UTF-8 emoji 序列 (含 ZWJ 组合) 不在中间切。属 nice-to-have,主路径已用 []rune() 不太可能出问题,但显式 assertion 更稳。

🔵 可选优化:

  • P3: web/embed.go dist 注释:PR description 提到 "go test ./cmd/cc-connect without no_web needs generated web/dist in a clean worktree" — 这是项目级 pre-existing issue (worktree 没跑 web build),不在本 PR 范围。可在仓库根 README 加一句 "worktree 验证时用 -tags no_web",避免后续贡献者重复踩。
  • P3: history_max_len 与 thinking_max_len 命名一致性:其它字段都用 _len 后缀 (thinking_max_len, tool_max_len, tool_messages),本字段也用 history_max_len ✓ 一致。👍 (just confirm)。

❓ 需要确认:

  • Q1 (历史数据本身):本 PR 只控制 /history 输出端的截断,不影响 session 持久化层。session history 完整内容仍存盘。是否有用户在 issue #1208 反馈中提到希望 session 存储也 truncation (防止 session file 无限增长)?若只是显示端可配置,本 PR 已完整。若用户也想控制 storage 端 (例如 LRU/cap-on-store),那是另一个 PR 范围。
  • Q2 (Reload 行为):reloadConfig 路径已 wire,验证 SIGHUP 时新值生效。是否在 reload 后给用户 log 1 行说明 "history_max_len applied: "?考虑与现有 reload log 风格一致即可。

Testing / Risk:

  • 已验证:CI run 27434073940 5/5 SUCCESS (lint/unit-test/smoke/regression/performance),本地 go test -count=1 -timeout 180s ./core/ ./config/ PASS (43.333s + 0.514s),go build -tags no_web ./... OK,go vet 干净 (web/embed.go dist warning 是 pre-existing 项目级问题,与本 PR 无关),gofmt -l on 5 PR files 0 输出。
  • 未覆盖风险:SUPERSEDED 0 命中 (git grep 在 origin/main 0 hits for HistoryMaxLen / history_max_len);multi-byte truncation 已显式测;0 = disabled 已显式测;负数 validation 已显式测;跨 platform 一致性已确认 (truncation 走 engine 内部,platform-agnostic)。

Next step:

  • 走 owner 拍板 merge;建议 author 补 1 行 CHANGELOG entry (上面 P2 给了现成文字,1 行 commit 即可)。merge 后 default 1000 生效,#1208 闭环。

@AaronZ345
AaronZ345 force-pushed the fix/history-max-len branch from e30fdf3 to b346f9a Compare June 15, 2026 02:35
@AaronZ345

Copy link
Copy Markdown
Contributor Author

@chenhg5 updated after your review.

Done:

  • Rebasing this branch onto latest origin/main (139bf9fad).
  • Added the unreleased CHANGELOG ### Fixed entry for [display].history_max_len, including the 200 -> 1000 default behavior change and 0 disable semantics.
  • Added an emoji/ZWJ UTF-8 boundary case to TestTruncateHistoryEntry. The implementation is rune-based, so this test explicitly checks valid UTF-8 output rather than claiming grapheme-cluster preservation.

Intentionally not changed:

  • Did not change session storage truncation; this PR remains display/output truncation only.
  • Did not add reload logging for history_max_len; the value is already wired through reload, and extra logs would be a broader logging-style decision.

Verified locally:

  • git diff --check
  • go test -count=1 -run 'TestTruncateHistoryEntry|TestEngineHistoryEntryMaxLen' ./core

@AaronZ345
AaronZ345 force-pushed the fix/history-max-len branch from b346f9a to bbac068 Compare June 15, 2026 02:46
@AaronZ345

Copy link
Copy Markdown
Contributor Author

@chenhg5 follow-up: upstream advanced again to 411687b67 (v1.3.3-beta.5 release + daemon build fix), so I rebased this PR one more time. The CHANGELOG entry is now placed as a new unreleased ### Fixed section above v1.3.3-beta.5.

Re-verified locally: git diff --check and go test -count=1 -run 'TestTruncateHistoryEntry|TestEngineHistoryEntryMaxLen' ./core.

@AaronZ345
AaronZ345 force-pushed the fix/history-max-len branch 4 times, most recently from ef38d51 to 5312ebc Compare June 16, 2026 14:43

@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.

结论: Request changes

总体判断: 本 PR 核心改动 (history_max_len configurable) 单独看是好的, 但最新一次 push (5312ebc) 把 与 history truncation 完全无关 的 "preserve queued messages during session startup" (#565) fix 推到了这个 PR。这是一个 cross-PR contamination: 同一份代码改动已经以另一个 commit hash (6673a32) 出现在 PR #1286 的 head。建议作者把 5312ebc 从本 PR 抽出, 保留在 #1286 或单独开 #565 PR, 否则 #565 fix 会两次进入 main。

Review 范围:

  • 看了 PR #1291 最新 head 5312ebc 全部 4 个 commit, 重点关注 #1291 主体 (history truncation) + 最新追加 commit 5312ebc
  • 主体 3 commit (0d40d26 + e5c6f68 + 历史) 仍在 scope 内。
  • 5312ebc 在 scope 外。

✅ 做得好的地方:

  • 主体 history truncation 改动实现清晰, maxLen==0 skip, len>maxLen 按 rune 截断, boundary check 用 emoji + ZWJ 测试覆盖 (TestTruncateHistoryEntry) 严谨。
  • e5c6f68 (test + CHANGELOG) 补充得很到位,边界用例选得合理。
  • TestTruncateHistoryEntry PASS, 本地 go test ./core 47.391s 全 PASS,没有 regression。

🚨/🔴 必须处理:

  • P1 Blocker: 5312ebc commit 与本 PR 主题无关, 属于 cross-PR 污染
    • 影响: 同一份 "preserve queued messages during session startup" fix 已经以 commit 6673a32 推到 PR #1286 的 head (queue management PR), 同样代码现在又以 commit 5312ebc 推到本 PR 的 head。两个 PR 任一合并都会把 #565 fix 带入 main;如果两个 PR 都合并,同一份代码会两次进入 main,造成重复 commit / 逻辑混乱 / 后续 bisect 噪声。
    • 证据: 对比 5312ebc6673a32 的 diff 内容 (排除 parent commit 引入的行号差异): 两份 commit 修改同一文件 core/engine.go 的同一段 queueMessageForBusySession 函数, 加入同一行 state, _ := s.ensureInteractiveStateForQueueing() 调用, 注释文字完全相同 ("ensure interactive state before queueing so queued messages survive session startup races")。这是同一份 fix 推到两个 PR。
    • 建议:
      1. 5312ebc#1291 抽出 (interactive rebase drop), 让 #1291 head 回到 e5c6f68
      2. 然后 #565 fix 应该只通过 PR #1286 (queue management 主题) 合并;如果 owner 决定走 #1291, 那需要从 #1286 drop 同样的 commit,避免重复。
      3. Owner / PM 协调决定 #565 fix 走哪条 PR。
    • 验证: 抽出 5312ebc 后,本地 build go build ./... + go test ./core 全 PASS。

🟠 建议改进:

  • (主体 history truncation 本身没有 P2/P3 建议, e5c6f68 测试已经覆盖到位)

🔵 可选优化:

  • 无。

❓ 需要确认:

  • @AaronZ345: 你是希望 #565 fix 通过 #1286 (queue management) 还是 #1291 (history truncation) 落地? 两个 PR 都保留会重复, 需要决定其中一个 drop。建议走 #1286 (主题匹配)。

Testing / Risk:

  • 已看到的验证: 本地 go test ./core 47.391s 全 PASS, 包括 TestTruncateHistoryEntry emoji + ZWJ 边界用例。
  • 未覆盖风险: 如果 5312ebc 不被抽出就合并, 与 PR #1286 后续 merge 会产生重复代码进入 main。需要 owner 协调两个 PR 的先后和取舍。

Next step:

@AaronZ345
AaronZ345 force-pushed the fix/history-max-len branch from 5312ebc to e5c6f68 Compare June 17, 2026 01:48
@AaronZ345

Copy link
Copy Markdown
Contributor Author

@chenhg5 updated after your review.

Done:

Verified locally:

  • git diff --check origin/main...HEAD
  • go test -count=1 -run 'TestTruncateHistoryEntry|TestEngineHistoryEntryMaxLen' ./core
  • go test -count=1 ./config
  • go test -count=1 ./core ./config
  • go test -count=1 -tags no_web ./cmd/cc-connect

Remote verification:

  • HEAD...fork/fix/history-max-len = 0 0
  • GitHub CI is green: lint, unit-test, smoke-test, regression-test, performance-test all passed.

@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

总体判断: 本 PR ready-to-merge。 Cross-PR 污染已解决 (#1286 独占 #565 fix), 本 PR 范围聚焦 [display].history_max_len 可配置, 0 P0/P1 blocker, owner 可发 v1.3.5 release scope。

Review 范围:

  • 看了 PR #1291 (fix/core: make /history entry truncation configurable, @AaronZ345 community, head e5c6f68, 9 files / +140 / -12, Fixes #1208)。
  • 重点关注 correctness / 跨 PR 污染 / utf8 安全 / 配置解析 / 测试覆盖。
  • 上轮 k5w28y (PRR_kwDORa0x388AAAABDNTjkQ) 报 cross-PR 污染 5312ebc, 作者 rebase 已彻底清理, 当前 head 只剩 0d40d26 (主体) + e5c6f68 (新增 emoji boundary test)。

✅ 做得好的地方:

  • 跨 PR 污染彻底清理: 5312ebc "preserve queued messages during session startup" 已从分支历史完全移除, 这次 rebase 不仅 --force-push 而是真 rebase (commit hash 全部变了), 旧 review (CHANGES_REQUESTED on 5312ebc) 自动失效, 需重 review。 #1286 仍保留 #565 fix, 单一来源无重复进 main 风险。
  • utf8 边界安全: truncateHistoryEntryutf8.RuneCountInString + []rune(content)[:maxLen], 不会出现半个 emoji/rune 被截断的乱码。 新增 test case 👨‍👩‍👧 中文 验证 ZWJ 序列 + utf8.ValidString 双重确认 (ZWJ 序列被截断后是 valid UTF-8, 但不一定是 valid grapheme cluster, 至少不会 panics / 输出乱码 bytes)。
  • 配置解析 chain 正确: EffectiveHistoryMaxLen 走 proj.Display > global.Display > default 1000, 跟 EffectiveDisplay 模式一致, 不引入新解析路径。 验证 logic 加 >= 0 check, 防 negative value。
  • 0 语义清晰: 0 = 禁用 truncation (跟 thinking_max_len / tool_max_len 行为一致), 默认 1000, 文档 (EN + zh-CN) + config.example.toml + CHANGELOG 都同步, 没有歧义。
  • 新增 4-case TestEffectiveHistoryMaxLen 覆盖 4 种解析路径 (default / global / project overrides global / 0 disables), 跟 EffectiveDisplay 测试模式一致。
  • SetDisplayConfig 走 pointer 模式: HistoryMaxLen *intThinkingMaxLen / ToolMaxLen 字段风格统一, nil = use default, 0 = explicit no truncation, 不会因为 struct zero value 把 0 跟 nil 混淆。

🚨/🔴 必须处理: 未发现阻塞合并的问题。

🟠 建议改进:

  • P2 /history 内部 entry-level (不是 command level) max len 行为: 当前实现 cmdHistory + renderHistoryCard 两处都通过 historyEntryMaxLen() 拿 maxLen 截断, 行为一致。 但 engine_test 已有 TestCmdHistory_RespectsConfiguredLimit (main) 跟 TestRenderHistoryCard_RespectsConfiguredLimit (main) 两类覆盖吗? 没有看到 PR 添加 cmd-level integration test, 现有测试仅覆盖 truncateHistoryEntry + historyEntryMaxLen 单元。 建议 release 前补 1-2 个 integration test 验证 cmdHistoryrenderHistoryCardHistoryMaxLen=0 时确实不截断 (没漏改一处)。 (注: 源码两处都改对了, 但缺回归测试兜底, 不阻塞, 建议补)
  • P2 默认 1000 跟 thinking_max_len=300 比例: thinking max 300 / tool max 500 都偏小, history 1000 偏大, 用户可能想把 history 调小到 500 看完整 session。 0 disable truncation 应对长 history, 但默认 1000 对 IM 平台 (feishu/lark 消息有 4KB 上限) 来说单条就 1KB 可能挤爆。 建议在 CHANGELOG 段加一句 "若 IM 平台单消息超长, 可调到 500 或 0 关闭截断", 但不阻塞, 默认值合理。

🔵 可选优化:

  • P3 defaultHistoryMaxLen 常量跟 [display].history_max_len 文档默认值同步: 当前 1000 一致, 提一下以后改默认要两处同步, 可加 lint check / doc 引用避免漂移。

❓ 需要确认:

Testing / Risk:

  • 已看到的验证 (本地):
    • TestEffectiveHistoryMaxLen 4/4 sub-case PASS (default / global / project overrides / 0 disables)
    • TestValidateProjectDisplayConfig 3/3 sub-case PASS (含新增 history_max_len negative case)
    • TestTruncateHistoryEntry PASS (ascii / unicode / emoji ZWJ / 0 disabled 4 边界)
    • TestEngineHistoryEntryMaxLen PASS (default + configured 0)
    • full go test ./config/... 0.492s PASS 0 regression
    • full go test ./core/... 47.358s PASS 0 regression
    • go build ./... 0 issue (pre-existing web/embed.go pattern warning 不属本 PR)
    • go vet ./config/... ./core/... 0 issue
    • gofmt: PR-touched files (config/config.go / config/config_test.go / core/engine.go / core/engine_test.go PR-added block) clean; pre-existing core/engine_test.go:1100 interactiveState struct alignment 是 main 已有噪声, 不属本 PR scope
  • CI status (GitHub): ci=success, mergeable=MERGEABLE, 2 commits, 0 conflict
  • 跟 main 比较: 0 conflict, PR 仅 9 files / +140 / -12 范围, 无意外 diff
  • 复现 step: 不适用 (enhancement, 非 bug fix)
  • 风险 matrix:
    维度 风险 缓解
    Cross-PR 污染 已解决 5312ebc 已彻底移除, #1286 独占 #565 fix
    utf8 / emoji 已验证 utf8.RuneCountInString + ZWJ 测试 case
    配置解析 已覆盖 4-case + 1-case validation 测试
    0 语义 已澄清 文档 + CHANGELOG + 测试 + config.example.toml
    跟 main 冲突 0 conflict 9 files / +140 / -12, 局部改动
    CI success 跟 main 比较 0 conflict
    Release scope 适合 v1.3.5 config only, 无 breaking change, 默认 1000 兼容旧行为
  • 未覆盖风险: integration test 缺 cmdHistory + renderHistoryCard 跟 configured maxLen 的端到端 wire (P2 建议补, 不阻塞)

Next step:

  • owner: merge 本 PR (PR #1291, 0 blocker, ready-to-merge) 进 v1.3.5 release scope
  • owner: 同 cycle 关注 #1286 现状 (上轮 k5w28y 已 --approve, 也 ready-to-merge), 现在 cross-PR 污染已清, 两条 PR 均可独立 merge
  • dev-cursor (optional): 若关心上面 P2 建议的 integration test, 可在后续 PR 补 TestCmdHistory_RespectsConfiguredLimit + TestRenderHistoryCard_RespectsConfiguredLimit, 但本 PR 范围无需扩
  • release-codex: v1.3.5 release gate 现在累计 9 → 10 PR ready (#1286 / #1288 / #1291 / #1298 / #1300 / #1317 / #1319 / #1320 / #1349 / #1345), 关注 9+ → 10+ CHANGELOG entry 措辞统一, 关注 #1349 BREAKING 提示 (default reuse → new_per_run) 跟 #1300 3 段 (timer shell config / QQ & Cursor hardening / espeak Windows) 跟 #1291 history_max_len config (default 1000) 的协同影响
  • QA next cycle: 继续 qa-view inbox top fresh review (候选 #659 / #1338 / #880 等 stale_needs_rereview / github_review=no)

@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.

结论: Comment (实质 Approve)

总体判断: 上轮 chenhg5 在 d691f1d 已 APPROVE, 本轮作者新增 e33004e 仅补 1 个 ZWJ emoji boundary 测试用例, 解 stale_needs_rereview 不需要重新 APPROVE, 走 --comment 留个 ack trail 即可。 全部 5/5 CI check PASS, mergeable=MERGEABLE, v1.3.5 ready。

Review 范围:

  • 对比 78b10b0e33004e (本次新 commit 增量), 9 files / +140/-12 总 diff。
  • 重点: 配置层级 (global vs project vs default)、UTF-8 边界、reload 路径覆盖、文档中英文一致。
  • 跑了 go test ./config/ -run TestEffectiveHistoryMaxLen -v (4/4 PASS), ./core/ -run TestTruncateHistoryEntry -v (1/1 PASS), go vet ./config/ ./core/ ./cmd/cc-connect/ 干净 (web/embed.go 那个 dist 警告是 pre-existing, 跟本 PR 无关)。

✅ 做得好的地方:

  • 配置层级链清晰: EffectiveHistoryMaxLen (config/config.go:913-924) 走 project > global > default(1000) 三段, 跟同文件里 EffectiveDisplay / EffectiveShell 风格一致, project 优先级高于 global, 不设走默认。 *int 用 nil 区分"未设"和"显式 0", 让 0 真正能禁用 truncation。
  • reload 路径同步: cmd/cc-connect/main.go:1649 跟首次初始化 538 行同步 wiring historyMaxLen, SIGHUP 触发 reloadConfig 时不会丢这个配置。
  • truncateHistoryEntry 用 rune-based 切: core/engine.go:8611-8616[]rune(content) 而非 byte 切, 4 字节 emoji 不会切坏。 0 = 不截断, 长度 ≤ maxLen 直接返回原内容 (避免无谓 copy + ... 后缀)。
  • 测试覆盖 4 个边界: ASCII (abcdef → abc...)、纯 Unicode (你好世界 → 你好...)、ZWJ emoji 序列 (👨‍👩‍👧 → 👨‍... 且 utf8.ValidString 校验)、0 = 不截断, 加上 TestEngineHistoryEntryMaxLen 验 nil pointer 走默认 + 显式 0 走 0。 e33004e 补的 ZWJ sequence 用例特别重要, 是 Unicode truncation 最容易出 bug 的位置。
  • 中英文文档同步: docs/usage.md + docs/usage.zh-CN.md/history 表格行加了 [display].history_max_len 提示, config.example.toml 注释也双语, 文档一致性达标。

🚨/🔴 必须处理: 未发现必须阻塞合并的问题。

🟠 建议改进:

  • P2 — truncateHistoryEntry 函数无 doc comment。 core/engine.go:8611 没解释为什么用 rune 而不是 byte (为 emoji / CJK), 也没说 0 的特殊含义。 一行 docstring 就够:
    // truncateHistoryEntry returns content cut to maxLen runes with "..." suffix.
    // maxLen <= 0 disables truncation; uses rune-based slicing to keep emoji intact.
    
  • P2 — HistoryMaxLen *int 字段无 doc comment。 core/engine.go:172 同上, 项目里其他 *int DisplayCfg 字段 (ThinkingMaxLen / ToolMaxLen) 也都缺, 但既然加新字段就把 pattern 立起来。 一行说明 nil vs 0 的语义差即可。
  • P2 — defaultHistoryMaxLen = 1000 在 core/engine.go:56 跟 cmd/cc-connect/main.go 的 wire-up 之间没有 "硬约束" 说明。 如果以后有人改 default 常量却忘了 main.go 的初始化顺序, 不会编译失败。 不阻塞, 但加一行 comment 说明 "default constant MUST match cmd/cc-connect/main.go initial DisplayCfg wiring" 比较稳。

🔵 可选优化:

  • P3 — EffectiveHistoryMaxLen 可以考虑跟 EffectiveDisplay 合并。 当前一个返回 7-tuple 一个返回单值, 接口风格略不一致; 长期看 EffectiveDisplay 应该返回一个 DisplayCfg struct, 但这是 refactor 议题不阻塞本 PR。
  • P3 — ZWJ emoji 测试 👨‍... 期望里没有 force utf8.ValidString 的 message。 当前 t.Fatalf("... = %q, want valid UTF-8 %q", got, "👨‍...") 会让 invalid UTF-8 跟期望不同的 emoji 都报同一条信息, 不利于调试。 建议分两条 fatal: 一次校验 ValidString, 一次校验内容。 不重要, 现在用例数小, 真出问题容易看出来。

❓ 需要确认: 无。

Testing / Risk:

  • 已验证: 5/5 CI 27860672168 全部 PASS (lint 1m35s / unit-test 4m5s / smoke 27s / regression 25s / performance 44s); 本地 4/4 TestEffectiveHistoryMaxLen subtest + 3/3 TestValidateProjectDisplayConfig subtest + 1/1 TestTruncateHistoryEntry + 1/1 TestEngineHistoryEntryMaxLen; go vet 干净。
  • 未覆盖: 没有跨项目 / reload 时序测试, 但项目其他 DisplayCfg 字段也是同模式 (无 reload 时序 test), 保持一致。
  • 未覆盖风险: 0 默认值跟现有用户期望有偏差 (老用户看到的 200 字符上限会变 1000), CHANGELOG 已说明 #1291 但 release notes 应该突出一下 "default 行为变更: /history 单条 200 → 1000 字符" 让用户主动确认。

Next step:

  • 解 waiting=human, owner 可 merge。 本 PR 没有 P0/P1 blocker, 2 P2 文档建议可以本 PR 收口也可以 follow-up。 v1.3.5 release gate 候选。

@AaronZ345
AaronZ345 force-pushed the fix/history-max-len branch 3 times, most recently from 1d5ac4d to 75f8d12 Compare June 21, 2026 15:45

@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

总体判断: 本次 review 覆盖 head 75f8d122 与上一版 1d5e6004 之间新增的 emoji 边界测试 + CHANGELOG 条目;fix 主体未变。整体改动小而聚焦,API/配置/文档双语文案一致,CI 5/5 全绿,新增单测本地通过,可以合并。

Review 范围:

  • 看了 core/engine.gocore/engine_test.goconfig/config.goconfig/config_test.gocmd/cc-connect/main.goconfig.example.tomldocs/usage.mddocs/usage.zh-CN.mdCHANGELOG.md
  • 重点关注 correctness(截断逻辑 + 配置解析链 + 校验)、backward compat(默认值 1000、0 = 不截断)、i18n(双语文案)。

✅ 做得好的地方:

  • 截断改用 rune 计数并新增 truncateHistoryEntry helper,统一了 cmdHistoryrenderHistoryCard 两处历史实现,消除旧版的 200 硬编码。
  • 配置解析沿用现有 Effective* 链路(project > global > default),GetGlobalSettingsvalidateDisplayConfig 同步更新,默认 1000、0 = 关闭截断、负数被拒,语义清晰。
  • 新增 emoji + CJK 的截断测试 TestTruncateHistoryEntry,并显式断言 utf8.ValidString,在 ZWJ 边界上至少保证不返回非法 UTF-8。

🟠 建议改进:

  • TestTruncateHistoryEntry 的 emoji 用例 👨‍👩‍👧 中文(maxLen=2) 截到 👨‍... 虽然是合法 UTF-8 字节,但 ZWJ 序列在视觉上是断裂的。如果后续想做到"不切到 grapheme cluster 中间",可以引入 golang.org/x/text/unicode/runes 或标准库 unicode/utf8 包一层 cluster 切分。不过当前断言的语义已经"不 panic 且仍合法",对 IM 历史摘要这个使用场景完全可接受,不强求。

🔵 可选优化:

  • 无。

❓ 需要确认:

  • 无。

Testing / Risk:

  • 已看到的验证: GitHub Actions lint / unit-test / regression-test / smoke-test / performance-test 全部通过(约 6m);本地重跑 core.TestTruncateHistoryEntry|TestEngineHistoryEntryMaxLenconfig.TestEffectiveHistoryMaxLen|TestValidateProjectDisplayConfig 通过。
  • 未覆盖风险: 0 = 不截断 的链路在 cmdHistoryrenderHistoryCard 两处都走 truncateHistoryEntry 的 early-return,行为一致;reloadConfig 已同步传入 HistoryMaxLen,热重载后立即生效。剩余风险极低。

Next step:

  • 可合并。变更范围限定在 /history 显示与 [display] 配置块,无 runtime 状态、无 schema 迁移、无外部接口改动。

@AaronZ345
AaronZ345 force-pushed the fix/history-max-len branch from 18edcd6 to 9350c90 Compare June 22, 2026 14:39
@chenhg5
chenhg5 merged commit fa86932 into chenhg5:main Jun 22, 2026
5 checks passed
chenhg5 pushed a commit that referenced this pull request Jun 23, 2026
* fix(core): make history entry truncation configurable

* test(core): cover history truncation emoji boundary
chenhg5 pushed a commit that referenced this pull request Jun 23, 2026
3 QA hotfixes (feishu image window, i18n nav.cron, slack streaming card)
+ 7 cherry-picked PRs from main (#1074 #1291 #1297 #1380 #1388 #1407 #1390).

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

2 participants