fix(core): hide scheduler background sessions - #1417
Conversation
ab7a90f to
35e863e
Compare
35e863e to
7852ab6
Compare
chenhg5
left a comment
There was a problem hiding this comment.
结论: Approve
总体判断: 这个 PR 干净地解决了 #1077 — cron/timer 在 session_mode=new_per_run 下产生的后台 session 不再泄漏到普通 /list、/switch、/delete 视图。改动面小、语义清晰、测试覆盖到位,可合并。
Review 范围:
- 看了
core/session.go、core/engine.go、core/engine_test.go。 - 重点关注 correctness(后台标记的传播路径)、compatibility(默认行为变化)、testing(新 test 是否覆盖关键边界)。
✅ 做得好的地方:
Background字段的语义明确、序列化进 JSON,跨重启仍生效,和现有KnownAgentSessionIDs()的 lock 习惯保持一致。BackgroundAgentSessionIDs()与KnownAgentSessionIDs()解耦得很干净:前者是后台 session 的"反向集合",后者是"非后台且被追踪的"集合,二者职责清晰,不会互相干扰。filterHiddenSessions优先于filterExternalSessions应用,语义正确:用户视角的"看不到"先于"哪些是我的"判断,这样filter_external_sessions=true模式下后台 session 既不会被隐藏也不会被错误地算成 known。ListSessions在读取Background时加s.mu.Lock()、读后释放,与现有KnownAgentSessionIDs的并发模型保持一致,避免 race detector 报警。- 新增
TestCmdList_HidesBackgroundSessionsByDefault直接覆盖核心场景(默认模式仍显示 external,但隐藏 cron 后台),覆盖完整。
🚨/🔴 必须处理: 未发现。
🟠 建议改进:
- 无。
🔵 可选优化:
filterHiddenSessions的len(hidden) == 0短路是一个微小但正确的优化;可以考虑加一行注释说明 hidden 来自BackgroundAgentSessionIDs()而不是某个用户输入 map,这样未来读者不会误以为这是性能 hack。但这是 nit,不强求。
❓ 需要确认:
- 无。
Testing / Risk:
- 已看到的验证:
go test ./core、go test -tags no_web ./cmd/cc-connect,本地都过;CI 5/5 绿。 - 回归测试
TestCmdList_HidesBackgroundSessionsByDefault直接覆盖"/list不再泄漏 cron session";TestCmdList_DefaultShowsAllSessions等先前的 /list 测试继续覆盖默认行为。 - 未覆盖风险:
cmdSwitch/cmdDelete走相同的applySessionFilter路径,新逻辑确实覆盖到了,但这两个 handler 自身没有针对"后台 session 不可达"的明确断言。考虑到分支相同,风险低;如果后续要补TestCmdSwitch_BackgroundSessionInaccessible之类的负向断言也可以,但本 PR 范围内不必阻塞。
Next step: 可合并。
7dd2d4f to
f38b559
Compare
chenhg5
left a comment
There was a problem hiding this comment.
结论: Request changes(P1: breaking without opt-out)
总体判断:
- 修复方向是对的(#1077 里
/list累积 cron 后台 session 确实是 bug),实现干净、测试到位、Maintainer 已 APPROVE。但目前的实现是硬编码的默认行为变更、没有任何配置开关能回退,属于 breaking change。合并前建议加一个 per-project bool 开关允许 opt-out。
Review 范围:
core/session.go、core/engine.go、及配套测试- 重点关注 correctness(后台标记的传播路径)、compatibility(默认行为变化 + opt-out 缺失)、testing 覆盖
✅ 做得好的地方:
Background字段的语义清晰、序列化进 JSON 持久化到 saved.json、跨重启仍生效NewBackgroundSession与NewSideSession通过内部newSideSession(bool)共享代码,语义拆分干净BackgroundAgentSessionIDs()覆盖了AgentSessionID+PastAgentSessionIDs,考虑到了 session 内部 rotate agent 的情况,比较周到applySessionFilter无条件先跑filterHiddenSessions,语义正交(不受filter_external_sessions影响),设计清晰- 5+ 个 regression test 覆盖 background 新建 / list 过滤 / 持久化 / KnownAgentSessionIDs 剔除
🔴 必须处理:
-
P1: 默认行为变更缺 opt-out 开关
影响:
- 有用户依赖
/list查 cron/timer 是否跑成功 → 升级后突然看不见 - 依赖
/list输出做监控/统计的自定义脚本 → 输出条目突然减少 - 想
/switch到某次 cron 的对话上下文追问 → 找不到入口 - IM 里没有其他官方方式查看跑完的 background session(Codex transcript 在
~/.codex/sessions磁盘上,但 IM 用户看不到)
证据:
applySessionFilter(core/engine.go) 里filterHiddenSessions无条件运行,没有 config gateListSessions()(core/session.go) 硬编码if background { continue }config/config.go、config.example.toml、CHANGELOG.md全部未改动- 用户无法通过任何配置回到旧行为
建议修复方向:
在
ProjectConfig加一个可选布尔字段,默认true(继续修 #1077),允许用户显式设false保留旧行为:// config/config.go // HideSchedulerSessions: true (default) hides cron/timer new_per_run // background sessions from /list, /switch, /delete. Set to false to keep // them visible (pre-#1417 behavior). HideSchedulerSessions *bool \`toml:\"hide_scheduler_sessions,omitempty\"\`
# config.example.toml [[projects]] # hide_scheduler_sessions = true # 默认 true:从 /list、/switch、/delete 中隐藏 cron/timer 的一次性后台 session(#1417) # 设为 false 可恢复旧行为,把后台 run 也当普通 session 显示
在 engine 里把 hide flag 传给
applySessionFilter,filterHiddenSessions只在 flag=true 时跑。验证: 加两条 test case:
- 默认(未配置)→ background sessions 被隐藏(保当前行为)
hide_scheduler_sessions = false→ background sessions 仍然在 /list 里出现
- 有用户依赖
🟠 建议改进:
-
P2: CHANGELOG 未提及行为变化: 这次改的是用户可见行为的默认值,建议在
## Unreleased下加一条### Changed或### Fixed,明确说明"cron/timernew_per_run后台 session 不再出现在 /list",让升级用户不至于摸不到头脑。 -
P2: 老数据迁移提示: 升级前已经跑过的 cron session 在
saved.json里Background字段为 false(默认反序列化),升级后仍然会出现在/list。建议在 PR body 或 CHANGELOG 里提示用户可用/delete清理历史,或在 release note 里说明。
❓ 需要确认:
- 未来是否考虑加
/list --all或/scheduler-history命令给用户查看被隐藏的 background session?(本 PR 不用做,可 follow-up)
Testing / Risk:
- 已看到的验证: 5+ 个 regression test 覆盖到位
- 未覆盖风险: 没有针对"opt-out 回退到旧行为"的 test(因为 opt-out 目前不存在);也没有针对"未标记 background 的老数据升级后的行为"做测试
Next step:
- 建议在合并前加
hide_scheduler_sessions开关(默认 true 保当前修复效果,允许 opt-out)+ CHANGELOG 条目。加完之后我可以再复审一轮。
f38b559 to
52b10a2
Compare
52b10a2 to
03f83bd
Compare
|
Closing this older PR in favor of a fresh replacement to avoid the stale CHANGES_REQUESTED review state after the compatibility opt-out was added. Replacement PR will link back here. |
|
Replacement PR: #1497 |
chenhg5
left a comment
There was a problem hiding this comment.
结论: Approve (re-review after author addressed P1)
总体判断: 上轮 (commit f38b559, 2026-07-04) 我提了 P1 "breaking without opt-out"。本轮作者在 head 03f83bd 上 force-push / 重新组织 commit,新增 per-project hide_scheduler_sessions config (default true, 可显式 = false opt-out) + SessionManager 完整 opt-out 实现 + main.go 初始/reload 双路径 wire + 4 个新 tests 覆盖 hide/show 两方向 + CHANGELOG + config.example.toml + 2 个 config 解析 test。P1 完整闭环,approve。
Review 范围:
- config/config.go: 新增 ProjectConfig.HideSchedulerSessions *bool toml 字段 + 注释说明 default true,set false 回退 pre-#1417。
- config/config_test.go: TestLoad_HideSchedulerSessionsDefault (nil) + TestLoad_HideSchedulerSessionsFalse (显式 false 解析)。
- config.example.toml:
[[projects]]注释段双语说明,default true,示例# hide_scheduler_sessions = true,跟现有 filter_external_sessions 注释风格对齐。 - cmd/cc-connect/main.go: 两处
engine.SetHideSchedulerSessions(...)wire,初始 load + reloadConfig 路径都覆盖(注意默认值处理:HideSchedulerSessions == nil || *HideSchedulerSessions,默认 true 走第一个 nil 分支)。 - core/engine.go: 新字段
hideSchedulerSessions bool+SetHideSchedulerSessions(v bool)setter 同步下推 SessionManager +applySessionFilter新增if e.hideSchedulerSessions { sessions = filterHiddenSessions(...) }+ 新 helperfilterHiddenSessions(sessions, hidden)。 - core/session.go: Session struct 新增
Background bool字段(omitempty,json 不影响老 snapshot 兼容);SessionManager 新增hideBackgroundSessions bool默认 true +SetHideBackgroundSessions(v)setter +BackgroundAgentSessionIDs()访问器。 - core/engine.go 内部: cron + timer 的
NewSideSession→NewBackgroundSession重命名(语义更清晰:这些都是 background 而不是 side);改动保留旧NewSideSession仍在,向后兼容。 - core/engine_test.go + core/session_test.go: 4 个新 tests 覆盖 hide/show 两个方向 + active session 不被 background 替换 + KnownAgentSessionIDs 不暴露 background agent。
- CHANGELOG.md: 在
## Unreleased/### Fixed段第一条添加core条目,明确提到"per-project hide_scheduler_sessions = false opt-out"。
✅ 做得好的地方:
- P1 完整闭环: 用户现在可以
[projects] hide_scheduler_sessions = false一行恢复 pre-#1417 行为,不需要改任何其他东西 — escape hatch 完整且 atomic。 - 默认值处理用
HideSchedulerSessions == nil || *HideSchedulerSessions表达式很到位 — nil (用户未设置) = true,显式 true = true,显式 false = false。布尔三态都覆盖了。 Background bool字段加在 Session struct 而不是单独 map,持久化到 JSON 自动纳入(omitempty 不破坏老 snapshot),不需要额外 migration。NewSideSession→NewBackgroundSession重命名跟概念对齐,既保留旧名(向后兼容)又提供语义更清晰的新名 — call sites (cron + timer) 同步替换。- 测试矩阵对成:
- TestCmdList_HidesBackgroundSessionsByDefault (新默认)
- TestCmdList_ShowsBackgroundSessionsWhenHideSchedulerSessionsDisabled (opt-out)
- TestSessionManager_NewBackgroundSessionHiddenFromUserList (session 级别过滤)
- TestSessionManager_BackgroundSessionsVisibleWhenDisabled (sm 级别过滤)
- main.go 同时 wire 初始 + reload,跟既有 filter_external_sessions 处理完全对称,reload 时
SetHideSchedulerSessions(...)会被立即 apply,不需要 restart。 - CHANGELOG 条目写得清晰,提到"with per-project hide_scheduler_sessions = false opt-out for users who want the old visible scheduler history behavior" — 升级用户能在 release notes 看到 escape hatch。
🟠 建议改进:
-
🟠 P2
NewSideSession现在跟NewBackgroundSession共存,如果新代码都用 background,建议下个版本把 NewSideSession 标 deprecated 并附// Deprecated: use NewBackgroundSession注释,避免后续误用。当前 call sites 已经全切到 background,风险低。 -
🟠 P2 CHANGELOG 行建议加
hide_scheduler_sessions这个 config 名字的具体配置示例(类似其他条目附set session_mode = "reuse"),便于用户搜配置名时第一眼定位:
- **core**: hide cron/timer \new_per_run` background sessions from normal `/list`, `/switch`, and `/delete` views by default, with per-project `hide_scheduler_sessions = false` opt-out for users who want the old visible scheduler history behavior (#1077, #1417).`现在 config 名字已经在描述里但没突出 bold。
-
🟠 P2
Background字段加到 Session struct 是个好选择,但没有 helper methodIsBackground() bool/MarkBackground(),如果其他地方需要区分 background vs main session 会重复访问 struct 字段。建议加 helper 增强可读性。 -
🟠 P2 docs/usage.md + docs/usage.zh-CN.md 没有更新 — 用户文档没提到
hide_scheduler_sessions这个新配置。CHANGELOG 提到但用户读 docs 时找不到。可以顺手在 /list 或 /switch 段补一句。
❓ 需要确认:
BackgroundAgentSessionIDs()这个 access 在新隐藏模式下,会不会跟旧的KnownAgentSessionIDs()产生数据不一致?e.g. 用户用旧 CLI (pre-#1417) 加载一个 background session,会不会被 KnownAgentSessionIDs 暴露?建议确认下BackgroundAgentSessionIDs在新默认 (true) 模式下与 KnownAgentSessionIDs 完全 disjoint,且 reload / restart 后状态恢复正确。tests 覆盖了 in-memory 路径,持久化路径希望已有老 snapshot 不会让 background leak。- 升级到 v1.4 时,已经存在的"pre-#1417 时代" 用户配置里如果没显式 hide_scheduler_sessions,默认 true 会让他们的 background sessions 突然从 /list 消失 — 这是有意的 break (跟 CHANGELOG 描述一致),但用户群里可能会先报告"我的 cron session 跑哪去了",需要 release notes 提前覆盖。
Testing / Risk:
- 已看到的验证:4 unit tests + 2 config tests 双向覆盖;CI 5/5 green,merge=clean,head=03f83bd8 force-pushed over f38b559。
- 仍可补:实际 cron 触发后,用户用 /list 看到的列表对比升级前后;不在本 PR 阻塞,release 后 user-ops 渠道可观察。
- 风险面:default flip 对未升级 docs/usage 的用户是隐形行为变化,CHANGELOG 已经标注。本 PR 已提供完整 escape hatch,owner release 决策时容易看到。
Next step: 作者无需返工。Owner 可以合并。本 PR 当前可合并,风险可接受。
Summary
new_per_runsessions as background sessions/list,/switch, and/deletefiltering even whenfilter_external_sessionsis offFixes #1077
Tests
go test ./corego test -tags no_web ./cmd/cc-connect