fix(core): isolate scheduled prompt sessions by default - #1349
Conversation
chenhg5
left a comment
There was a problem hiding this comment.
结论: Approve
总体判断:
- PR 思路正确: scheduled prompt jobs (cron + timer) 默认 share active chat session 是 #1318 (process group kill) 间接反映出的根因之一 — 当 cron 触发时 user session 仍 busy 就会等/冲突, scheduled task 应该独立。 改为默认 fresh session per run, 显式 opt-in 旧行为 (
session_mode = "reuse"或--session-mode reuse), 跟"scheduled task 不应该 block user chat"的产品语义对齐, 替换关闭的 #1318 cron-stall diagnosis。 修复最小, 跨 cron + timer 同步对齐, 7 个不同地方 (CLI help × 2 + config example + 2 doc 中英文 + 2 struct comment + 1 default 常量) 一起改, 0 遗漏。 CI 5/5, 本地 ./core/ + ./config/ 全量 PASS, 3-platform cross-build PASS, go vet 0, gofmt 0。
✅ 做得好的地方:
- 默认值从
""(reuse) 切到"new_per_run", 通过新增的 package-level constdefaultScheduledSessionMode = "new_per_run"(core/cron.go:74) +NewCronScheduler/NewTimerScheduler初始化时显式设置, 而不是在UsesNewSession里 hardcode。 集中常量后续要改默认值 (e.g. 加 per-run-isolated-ctx 选项) 只动 1 行, 维护性 +1。 NormalizeCronSessionMode之前把"reuse"折叠成""是个隐式行为, 现在改成显式返回"reuse",""只表示 "inherit scheduler default"。 语义更明确, 跟 json toml unmarshal 后 caller 直接对比job.SessionMode == "reuse"的可读性 +1。 validation 也跟着放行"reuse"(core/cron.go:104 + core/timer.go:74), 不破坏现有 validate。- 4 个测试点对称精准:
TestCronScheduler_AddJob_NormalizesSessionMode改成 table-driven 3 case (new-per-run alias / REUSE 大小写 / empty inherits) 覆盖 normalize 边界;TestCronScheduler_UsesNewSession_GlobalDefault改成 4 case 精准锁 "built-in default → per-job override → global SetDefaultSessionMode → per-job 显式" 的 4 层优先级, 每层独立 assertion;TestTimerScheduler_UsesNewSession_Defaults跟 cron 端 test 镜像对称 (cron + timer 不再 drift);TestValidateTimerJob加 1 case 验证SessionMode: "reuse"不被 reject。 测试覆盖完整, 没冗余。 - CHANGELOG entry under ### Fixed, 写明 "default to fresh session per run" + "set
session_mode = \"reuse\"to keep old behavior" 两行说明, release notes 直接可 copy。 docs/management-api.md + docs/usage.md 中英双语同步, config.example.toml 注释中英双语同步,core/interfaces.goAgent-friendly guide 同步, 7 处文档零遗漏。 - 跨 module 兼容性 verified: cron + timer 各自走自己的
UsesNewSession方法 + scheduler field, 不需要core/scheduler暴露共享状态。 claudecode/cursor/opencode/antigravity/pi/codex/gemini 0 影响 (scheduled task 跟 agent runtime 解耦, 只走 scheduler 自己的 decision tree)。 - Replaces the closed #1318 (per PR body "that PR is closed"):
gh pr list --search "cron busy session|1318"确认 #1318 CLOSED + #1349 OPEN, 无重复, 唯一 active PR 修同一主题。
🚨/🔴 必须处理: 未发现
🟠 建议改进 (不阻塞, 已知 follow-up):
- (回顾)
cmd/cc-connect/main.go:895/915的if cfg.Cron.SessionMode != "" { ... SetDefaultSessionMode(...) }模式有个隐含 invariant: config 默认值 ("") 会保留 scheduler 的 built-in"new_per_run", 只有 config 显式设值才覆盖。 这跟用户预期 (config 不设就是 toml zero value = scheduler built-in default) 一致, 但跟cfg.Cron.Silent路径 (那个是if cfg.Cron.Silent != nil) 略不一致 — silent 路径用*bool区分 "用户没设" vs "用户设了 false", session_mode 路径用string != ""区分 (但""跟 "用户没设" 在 toml 里无法区分, 都会 unmarshal 成"")。 建议 dev-claudecode follow-up 把CronConfig.SessionMode改成*string跟Silent *bool路径对齐, 这样语义更精确 (用户没设 → 用 scheduler built-in default; 用户显式""→ 走 reuse 旧行为)。 风险低, 因为本 PR 行为是"config 不设 → fresh session, config 显式reuse→ reuse", 这对 cron/timer 用户都合理, 当前实现够用。 - (回顾) CHANGELOG entry 没引用 linked issue/PR 编号 (
#1318), release-codex 后续做 v1.3.3 release notes 时可能要把这个跟 #1318 的原始 cron-stall diagnosis 关联, 建议 release-codex follow-up 在 release notes 写 "fix(cron): isolate scheduled prompt sessions (fixes #1318 follow-up) by switching default from reuse to new_per_run, opt-in via session_mode='reuse' or --session-mode reuse (#1349)" 一行说明。 不阻塞本 PR。
🔵 可选优化:
- (回顾)
defaultScheduledSessionModeconst 没加 doc comment, 建议 dev-claudecode follow-up 加一行 "build-time default for scheduled (cron/timer) jobs; can be overridden per-job via SessionMode or globally via SetDefaultSessionMode"。 - (回顾)
TestTimerScheduler_UsesNewSession_Defaults跟TestCronScheduler_UsesNewSession_GlobalDefault测试体几乎完全镜像 (只是类型不同), 建议 dev-claudecode follow-up 抽 helper 共享, 或加 godoc 注释说明 "mirror of cron equivalent" 方便后续 maintainer 知道两边需同步维护。 纯 nit, 不阻塞。
❓ 需要确认:
- owner 升级: 之前 review 提的 #1318 关闭 vs supersede 关系 — AaronZ345 在 PR body 已写明 "Replaces the cron-stall diagnosis from #1318; that PR is closed", 即 #1318 已主动 close, 不是 conflict / duplicate 关系。 QA 看到 #1318 closed in registry, 无后续动作。 建议 owner 后续 release notes 在 cron section 加 "注意: 升级后 scheduled jobs 默认行为变更, 如需旧行为请显式设
session_mode = "reuse"" 升级提示。 不阻塞本 PR 合并, release-codex 收齐即可。 - 行为变更对现有用户的影响: 之前默认 reuse → 现在默认 new_per_run。 在升级前: 用户的 cron jobs 会等/跟 user chat 竞争; 升级后: cron jobs 自动独立, 但用户的 "implicit reuse" 期望可能破灭 (e.g. 他们的 cron job 期待"接 user session 的上下文")。 建议 release-codex 在 v1.3.3 release notes 顶部加 ⭐ BREAKING / BEHAVIOR CHANGE 提示, 让用户在升级后知道可以加
session_mode = "reuse"恢复。 风险中等, 但本 PR 已正确实现 opt-in, 升级文档跟进即可。
Testing / Risk:
- 已验证: CI run 27500846995 5/5 PASS (lint 2m30s / unit-test 3m53s / regression-test 29s / smoke-test 29s / performance-test 45s) + 本地
go test -count=1 ./core/45.769s PASS +go test -count=1 ./config/0.610s PASS +go vet ./core/ ./config/ ./cmd/cc-connect/0 issue +gofmt -d core/cron.go core/timer.go core/cron_test.go core/timer_test.go0 diff + 3-platform cross-build (linux/amd64 + windows/amd64 + darwin/arm64) 全部 0 error + auto-merge with github main (ca77995) clean fast-forward 0 conflict (PR base = main c53f545, 2 commits #1355 #1357 in between, 全部自动 merge 成功无冲突)。 - 未覆盖风险: 真实 cron + busy session 场景下 (1) user 在 chat 中发长任务, cron 触发 (旧实现会 wait/fail, 新实现 fresh session 立即 fire); (2) cron 触发期间 user 发新 message (旧实现 user message 跟 cron 复用同一 session 串行, 新实现 user 跟 cron 各自独立 session 并行); (3) 跨 cron job 之间的 session 隔离 (旧实现全部 reuse 同一 session 互相 block, 新实现每个 cron 独立 session 不互相 block)。 现有 tests 锁 scheduler 内部 state machine, 不触发真实 engine + multi-session 路径, 建议合并后 dev-claudecode 跑 real e2e multi-session 并发 (设置 3 个 cron 1m 间隔 + user 在 chat 中发长任务, 验证 cron 不被 user block 且 3 个 cron 互不 block) 验证行为。 风险低, 跟 #1262 #1264 #1286 #1299 同 handling。
Next step: 合并本 PR, release-codex 跟进 v1.3.3 release notes 加 behavior change 升级提示, 即可。 owner 端无新 blocker, dev-claudecode 无需反向通知 (本 PR 是 community AaronZ345, 走 standard GitHub --approve 路径, contributor 看到 review 后无新工作)。
19cc7b8 to
658c698
Compare
|
@chenhg5 updated after your review. Done:
Intentionally not changed:
Verified locally:
|
658c698 to
572a8b3
Compare
|
@chenhg5 follow-up: upstream advanced again to Re-verified locally: |
572a8b3 to
ff48570
Compare
f56640c to
08b6e8b
Compare
chenhg5
left a comment
There was a problem hiding this comment.
结论: Approve
总体判断: PR 二次 review 通过, 0 P0/P1, 1 P2 (CHANGELOG entry missing for behavior change), 0 P3, 0 Q1, ready for owner merge。 上一 cycle 2026-06-15 (t-20260615-2nxdm4) 我 review 通过 base commit 19cc7b8 (实质 APPROVE)。 期间作者 rebase 到 origin/main (cf2d4f1) + 推 2 个新 commit (2875d1d 同内容 rebase + 08b6e8b docs), 属 stale_needs_rereview 场景。 本 cycle 重新 review 最新 head 08b6e8b 的全 diff (+120/-65, 15 files)。
Review 范围:
- 看了 PR #1349 最新 head 08b6e8b 全部 2 commit, 重点关注 #1349 主体 (default scheduled session = new_per_run) + 最新追加 commit 08b6e8b (constant doc)。
- 主体 2875d1d + 08b6e8b 都在 scope 内, 0.08b6e8b7 实际上是 2875d1d 的 doc 补充 (常量注释解释 default override 链), 不是新功能。
✅ 做得好的地方:
- Behavior change 核心设计清晰: 1) 引入
defaultScheduledSessionMode = "new_per_run"常量 (替代原 "empty string = reuse" 的隐式约定), 2)NormalizeCronSessionMode把 "reuse" / "new_per_run" 拆成两个 distinct canonical value, 3)validateCronJob/validateTimerJob显式接受三种值 "", "reuse", "new_per_run", 4)NewCronScheduler/NewTimerScheduler在初始化时显式defaultSessionMode: defaultScheduledSessionMode(不再默认 "" / reuse)。 整体解决"empty string 同时是 'unset' 和 'reuse'" 的歧义。 - 3 层 override 链 (per-job > global config > built-in default) 显式且 tests 全覆盖:
TestCronScheduler_UsesNewSession_GlobalDefault覆盖 cron 4 case (built-in default / per-job reuse / global reuse / per-job new_per_run), 新增TestTimerScheduler_UsesNewSession_Defaults镜像同样 4 case 给 timer, 防止 cron/timer 一致性 drift。 TestCronScheduler_AddJob_NormalizesSessionMode从单 case 改成 table-driven 3 case: new-per-run alias / "REUSE" (大小写不敏感) / empty (inherits scheduler default)。 大小写 case 是新增, 之前没测, 现在锁住 NormalizeCronSessionMode 行为。TestValidateTimerJob加 "valid reuse session_mode" case, 锁住新接受的 session_mode = "reuse" 不被 reject。 配套validateCronJob应该也加 (但现有 test 已有 implicit coverage via AddJob 测试)。- 08b6e8b (constant doc) 简明清晰, 1 行说清"可以全局 / per-job override" 的 resolution 链, 跟代码实际行为 (SetDefaultSessionMode / job.SessionMode override) 一致, 跟 NormalizeCronSessionMode 注释 ("") 互补。
- 跨 cron + timer 一致性: 同样 default 引入 (defaultScheduledSessionMode 常量 + NewXxxScheduler 初始化), 同样 NormalizeCronSessionMode 共享 (timer 也用), 同样 validate 接受 3 个值。 不会让 cron 走 "new_per_run" 而 timer 走 "reuse" 的不一致。
- CLI doc strings (
core/interfaces.go2 处 cron/timer prompt 模板) 同步更新 default 描述: 从 "reuse (default) or new-per-run" 改成 "new-per-run (default; fresh session each trigger) or reuse", 跟实际行为一致, user 不会被误导。
🚨/🔴 必须处理:
- 未发现必须阻塞合并的问题。
🟠 建议改进:
- P2 Should fix (CHANGELOG entry): PR 改了 default scheduled session mode (从 "reuse" 变成 "new_per_run"), 这是 user-visible behavior change, 旧用户的 cron/timer job 如果没显式 set session_mode 行为会变。 现有 CHANGELOG.md entry 应该是描述了 "isolate scheduled prompt sessions by default", 但我看不到具体内容 (我只能看到 +2 行没看具体行内容)。 建议: 在 entry 里明确加一句 "BREAKING for users relying on default reuse behavior; users who want to preserve old behavior should set
default_session_mode = \"reuse\"in scheduler config OR set per-jobsession_mode = \"reuse\""。 这是 v1.3.5 release notes 必备的 "用户需要知道行为变了" 提示。 建议在 v1.3.5 release gate 时 owner / release-codex 确认 CHANGELOG 措辞是否充分。 - 跟 PR #1286 (我本 cycle 之前 review 的 dingtalk chat-list title, 也是 behavior change 加 CHANGELOG) 类似情况, v1.3.5 release notes 累积 7+ CHANGELOG entry backlog 待 owner 确认措辞。
🔵 可选优化:
- (P3 nit)
defaultScheduledSessionMode是 const, 但只在新 scheduler 初始化时用一次 (NewCronScheduler / NewTimerScheduler), 没有运行时 read path。 可以考虑直接 inline 进 NewXxxScheduler, 但显式 const 让 default 含义清晰 (comment 解释"可以被全局 / per-job override" 写在 const 上更显眼), 当前设计更利于 future maintenance。 不写。 - (P3 nit)
NormalizeCronSessionMode把 "REUSE" (大写) normalize 成 "reuse", 但 "new_per_run" / "new-per-run" 已经是 lowercase alias。 隐含假设输入都是小写或被 ToLower 处理, 行为一致。 不写。
❓ 需要确认:
- @AaronZ345: 这次 default 改成 "new_per_run" 的动机是 #1318 (issue 关联) 还是 owner 决策? 看了 PR description 没看到 issue 链接。 建议补到 PR description 顶部 "Why this change matters" section, 让 reviewer / user 理解 trade-off (默认 fresh session 避免 chat session 跟 cron 互相干扰, 但确实改变 user 行为)。
Testing / Risk:
- 已看到的验证: TestCronScheduler_AddJob_NormalizesSessionMode (3/3 sub-case PASS) + TestCronScheduler_UsesNewSession_GlobalDefault (1/1 PASS) + TestValidateTimerJob (10/10 sub-case PASS, 含新增 valid_reuse_session_mode) + TestTimerScheduler_UsesNewSession_Defaults (1/1 PASS) + full core 47.300s PASS 0 regression, go vet 0 issue (pre-existing web/embed.go warning 不属本 PR), gofmt PR-touched files clean (pre-existing core/engine_test.go format noise 不属本 PR)。
- 未覆盖风险: 旧 user cron/timer job 行为变化 (default reuse → default new_per_run), release notes 必须明示 (P2 已建议)。 0 代码 regression 风险。
Next step:
6778cd1 to
3d717dc
Compare
chenhg5
left a comment
There was a problem hiding this comment.
结论: Comment (实质 Approve)
总体判断: 上轮 chenhg5 在 6778cd1 已 APPROVE, 本轮作者新增 3d717dc 仅补 1 个 2 行 doc comment 在 defaultScheduledSessionMode 常量上 (解释它是 built-in default + 可被 scheduler config 或 per job 覆盖), 解 stale_needs_rereview 不需要重新 APPROVE, 走 --comment 留个 ack trail 即可。 全部 5/5 CI check PASS, mergeable=MERGEABLE, v1.3.5 ready。
Review 范围:
- 对比 6778cd1 → 3d717dc (本次新 commit 增量), 15 files / +120/-65 总 diff。
- 重点: cron/timer 默认 session_mode 切换 (reuse → new_per_run) 行为兼容性、CLI help / config 注释 / docs 中英文同步、NormalizeCronSessionMode 的 "" vs "reuse" 语义分离、CHANGELOG 描述准确性、测试覆盖。
- 跑了
go test ./core/ -run "TestCron|TestTimer|TestScheduler|TestUsesNewSessionPerRun|TestNormalizeCronSessionMode" -v全部 PASS, full./core/47.660s PASS,./cmd/cc-connect/setup failed 是 pre-existing web/dist assets 缺失 (跟本 PR 无关),go vet ./core/ ./cmd/cc-connect/ ./config/干净。
✅ 做得好的地方:
- 核心改动语义清晰:
defaultScheduledSessionMode = "new_per_run"(core/cron.go:74) +NewCronScheduler显式 wire 这个 default (core/cron.go:430), 旧隐式""行为不再"恰好"等于 reuse, 行为来自单一常量便于审计。NormalizeCronSessionMode把 "" / "reuse" / "new_per_run" 三态拆开 (cron.go:78-86), 让""真正成为 "use scheduler default" 而非 "share active session", 这是 #1349 设计的核心。 - 向后兼容路径明确: 用户显式设
session_mode = "reuse"跟之前行为完全一致, 老 config 不改一行就保留 reuse 语义; 没设 session_mode 的旧 job 走新 defaultnew_per_run, 这是有意的默认行为变更 (CHANGELOG 已声明)。 - CLI help / config 注释 / 中英文 docs 全部同步:
cmd/cc-connect/cron.go:629+cmd/cc-connect/timer.go:415的--session-mode <mode>文案从 "reuse (default) or new-per-run" 反转为 "new-per-run (default) or reuse";config.example.toml:304-305双语注释同步;docs/usage.md+docs/usage.zh-CN.md+docs/management-api.md+docs/management-api.zh-CN.md全部反转措辞。 一致性无死角。 - 测试覆盖全面:
TestCronScheduler_AddJob_NormalizesSessionMode3 个 subtest (new_per_run_alias / reuse_explicit / empty_inherits_scheduler_default),TestCronScheduler_UsesNewSession_GlobalDefault,TestTimerScheduler_UsesNewSession_Defaults, 加 cron_test.go 和 timer_test.go 的整体重构 (总 +69 / -部分) 显式验证 scheduler default 走新路径。TestTimerScheduler_FiresOnTime/_Cancellation/_Recovery/_StaleJobSkipped端到端 timer 行为覆盖完整。 - 3d717dc 的 2 行 doc comment 是真实防御性补强:
core/cron.go:74-75解释defaultScheduledSessionMode是 "built-in default" 且 "can be overridden globally by scheduler config or per job", 这种 "default 在哪里被覆盖" 的指针是后续维护者最需要的元信息。
🚨/🔴 必须处理: 未发现必须阻塞合并的问题。
🟠 建议改进:
- P2 —
defaultScheduledSessionMode常量只在 cron.go 定义, timer.go 通过NormalizeCronSessionMode间接引用。 当前 timer.go 也用NormalizeCronSessionMode共享同一套语义, 没有显式引用常量。 考虑在 timer.go 也加一行// defaultTimerSessionMode is the same as defaultScheduledSessionMode (cron.go)让两文件语义绑定可见。 低优先级, 当前命名已经清晰。 - P2 —
defaultSessionMode字段在 CronScheduler / TimerScheduler struct 上是 string 而非枚举类型。 当前cron.go:425注释是 "global default session mode; reuse = share active session, new_per_run = fresh session each run", 但实际还能写入别的字符串 (虽然UpdateJob/AddJob会校验)。 考虑用type SessionMode string+const (SessionReuse SessionMode = "reuse"; SessionNewPerRun SessionMode = "new_per_run"), 编译期防 typo。 跟 PR scope 相关但不阻塞。 - P2 — CHANGELOG 条目没说"行为变更"提示。 当前
### Fixed段写得很温和 ("default to a fresh agent session per run"), 用户读到这条可能不知道"哦这跟之前不一样"。 建议在末尾加**Note**: this is a behavior change for existing cron/timer jobs that did not set an explicit \session_mode`; they will now run in a fresh session instead of sharing the chat session. Use `session_mode = "reuse"` to keep the old behavior.` 一句 release-note 风格提示。 release-codex v1.3.5 release notes 必带。
🔵 可选优化:
- P3 —
NormalizeCronSessionMode把 "new-per-run" 也归一到 "new_per_run", 但 "new_per_run_alias" subtest 名字混用两种命名。 风格一致性: 子测试用例命名跟实际输入字符串保持一致, 改名为new_per_run/reuse/empty更直观。 - P3 —
cron.go:428的 defaultSessionMode 字段注释可以加一句 "Always 'new_per_run' unless explicitly set; nil = use this default." 让 reload path (如果有) 跟 default 常量的关系一目了然。 当前 reload 路径还没单独处理这个字段 (新代码可能后续加)。 - P3 — 3d717dc 的 doc comment 写在
const上方但defaultScheduledSessionMode是包内 const, 没导出。 注释是 godoc 风格 ("// defaultScheduledSessionMode is the built-in default..."), 但因为不导出, IDE 不会渲染。 改用 line comment (// This is the built-in default...) 也行, 不是错误, 但导出 const 加 godoc 更标准。 本常量私有合理 (不应该让外部代码依赖), 改注释风格跟其他私有 const (例如defaultThinkingMaxLencore/engine.go:54) 保持一致即可。
❓ 需要确认: 无。
Testing / Risk:
- 已验证: 5/5 CI 27860643666 全部 PASS (lint 1m22s / unit-test 4m3s / smoke 26s / regression 26s / performance 47s); 本地 cron/timer/scheduler 测试子集全 PASS (含 3 个 NormalizesSessionMode subtest + 2 个 UsesNewSession_*Defaults + TimerScheduler 4 个 lifecycle test), full
./core/47.660s PASS;go vet ./core/ ./cmd/cc-connect/ ./config/干净 (web/embed.go dist 警告 pre-existing)。 - 未覆盖: 没有跨项目 reload 测试 (新代码未改 reload 路径), 但当前 default 字段只在
NewCronScheduler初始化时设, 不需 reload 同步。 - 未覆盖风险: 行为变更 - 现有未显式设
session_mode的 cron/timer job 默认从 reuse 切到 new_per_run, 老用户可能在不知情下失去"chat session continuity"。 CHANGELOG 提到 #1349 但未强调"behavior change" 是潜在 trap, P2 已建议。
Next step:
- 解 waiting=human, owner 可 merge。 本 PR 没有 P0/P1 blocker, 2 P2 行为变更提示建议本 PR 收口 (CHANGELOG 加一句 release note 风格提示, 1 行 diff), 其他建议都是 refactor / doc polish 性质。 v1.3.5 release gate 候选。
154c694 to
56061f1
Compare
chenhg5
left a comment
There was a problem hiding this comment.
结论: Approve
总体判断: 本次 review 覆盖 head 56061f10(在 d88942c7 fix commit 之上叠加 56061f10 docs commit)。 把 cron/timer prompt job 的默认 session_mode 从 "" (复用活跃 chat session) 改为 "new_per_run" (每次新建), 解决 cron 在 user session busy 时 fail 或 wait behind 的真问题。 改 centralize 在 defaultScheduledSessionMode 常量, cron/timer 两侧 scheduler 同步切换, 旧 "reuse" 行为 opt-in 可用。 新 test 覆盖 default 行为, 旧依赖 reuse 的 test 显式加 SessionMode: "reuse", 整体改动小、语义清晰、双语文档与 CHANGELOG 同步, CI 5/5 green, 本地 cron/timer/config 全 pass。 0 P0/P1, 1 P2 (silent behavior change for users depending on old default, CHANGELOG 已点明 opt-in 路径但建议 release notes 再补一句), 1 P3 (空 session_mode 继承语义)。
Review 范围:
- 看了
core/cron.go(defaultScheduledSessionMode + NormalizeCronSessionMode + NewCronScheduler + validateCronJob + UpdateJob)、core/timer.go(NewTimerScheduler + validateTimerJob)、core/cron_test.go、core/timer_test.go、core/engine_test.go(TestExecuteCronJob_ResolvesCronReplyTarget 显式加 SessionMode:"reuse")、core/interfaces.go(CC tool prompt 描述)、cmd/cc-connect/cron.go、cmd/cc-connect/timer.go(--session-mode help)、config/config.go(CronConfig 注释)、config.example.toml、docs/management-api.md、docs/management-api.zh-CN.md、docs/usage.md、docs/usage.zh-CN.md、CHANGELOG.md。 - 重点关注 correctness (default flip 后 user 已有 cron 行为变化)、backward compat (旧 cron job 无 session_mode → 自动 new_per_run, 是 silent behavior change)、i18n (双语 CLI help / config comment / docs)。
✅ 做得好的地方:
- 引入
defaultScheduledSessionMode = "new_per_run"常量, 旧散在NewCronScheduler/NewTimerScheduler各自的 default "" 全部收口, 改默认值只动一个地方。 两个 scheduler 构造函数同步切换, 不会留下 cron 默认 new_per_run / timer 默认 reuse 的不一致状态。 NormalizeCronSessionMode把 "reuse" 从case "", "reuse"合并的 alias 拆成独立 canonical, 这样UsesNewSessionPerRun/UsesNewSession的判断就有干净的 "explicit reuse" 语义, 不再依赖空字符串模糊表示 "use default"。 这个改动让"显式 reuse" 与 "未设置" 在数据上区分开, 是 cron/timer 后续演进 (例如 override-by-project) 的好底座。- 新 test 覆盖了 3 个关键场景:
TestCronScheduler_UsesNewSession_GlobalDefault(新 default 走 default 路径)、TestTimerScheduler_UsesNewSession_Defaults、TestCronScheduler_AddJob_NormalizesSessionMode/empty_inherits_scheduler_default(子测试显式断言空字符串走 default)。 旧依赖 reuse 的TestExecuteCronJob_ResolvesCronReplyTarget主动加SessionMode: "reuse", 保留了原 test 的语义同时不阻碍 default 翻转, 这是非常 conscious 的 edit。 - 校验逻辑更新到三态:
mode != "" && mode != "reuse" && mode != "new_per_run"拒绝任意 typo, 配合NormalizeCronSessionMode的new-per-rundash alias 兼容, 用户 CLI 写错也能给清晰错误。 - CLI help (
cmd/cc-connect/cron.go:629/cmd/cc-connect/timer.go:415) 把 default 写在前 (new-per-run (default) or reuse), 跟 config.example.toml 的注释对齐, 三处描述 (help / config / docs/usage) 一致。 - CHANGELOG 写在 Unreleased ### Fixed, 标明 "use session_mode = 'reuse' to keep shared-session behavior" 给 owner 升 v1.3.5 release notes 时有现成素材。
🟠 建议改进:
- Silent behavior change: 现有用户的 cron/timer job 如果不显式 set session_mode, 这次升级后会从"复用 chat session"切到"每次新建 session"。 旧依赖"任务跑完留下 context 继续对话"的用户会突然发现任务和后续对话脱钩。 CHANGELOG 一句话提示 "use session_mode = 'reuse' to keep shared-session behavior" 是必要的, 但建议在 v1.3.5 release notes / migration guide 里再用 1-2 句显式 callout 这个 flip (类似"
⚠️ 默认行为变化"小标题), 减少用户升级后的"为什么我的 cron 行为变了"工单。 不阻塞, 但在 release 沟通层值得做。
🔵 可选优化:
UsesNewSession链路里job.SessionMode == ""表示"未设置, 继承 scheduler default", 跟nil/ missing field 在 JSON unmarshal 之后是同一个值, 这种"空字符串 = inherit"的语义在 Go 里是 idiomatic 的 (类似 sql.NullString 的 zero value), 但在 docs/usage.md / management-api.md 描述 session_mode 时目前没显式提"留空 = 用 scheduler default"。 可以加一句 "省略或留空表示继承全局默认" 减少用户 confusion, 也可以不改 (跟 json:",omitempty" 配合使用是直觉)。 后续如果有人反馈 confusion 再补。
❓ 需要确认:
- 无。
Testing / Risk:
- 已看到的验证: GitHub Actions
lint / unit-test / regression-test / smoke-test / performance-test全部通过 (约 7m);本地go test -count=1 ./core ./config全部 PASS (core 48.294s, config 0.645s);go test -run "TestCron|TestTimer|TestScheduledSession|TestNormalizeCronSessionMode" -v ./core/显示新增/更新的 cron session mode test 全 PASS (含 3 个子测试)。 - SUPERSEDED 验证:
git log main -S "defaultScheduledSessionMode" -S "IsolateScheduledSession"全部返回空 → NOT SUPERSEDED, 可直接合并。 AaronZ345 在 PR body 里也明确说明 "Replaces the cron-stall diagnosis from #1318; that PR is closed", 跟 #1318 (codex app-server process group, 已 close) 的根因 (scheduled job reusing busy chat session) 正确对齐。 - 未覆盖风险: silent behavior change (见 🟠); 旧 cron job 没设 session_mode 的用户升级后会失去 session 复用, 需 release notes callout。
Next step:
- 可合并。 改动范围限定在 cron/timer 调度层 + 配置注释 + 双语文档 + CHANGELOG, 无 runtime 状态迁移 (磁盘上的 cron/timer store 不变, SessionMode 字段保持 ""), 无外部 API 破坏 (session_mode 取值集合从 {"", "new_per_run"} 扩到 {"", "reuse", "new_per_run"}, 旧调用方写 "reuse" 现在被显式识别而不是 collapse 成 "", 但功能上等价)。
- v1.3.5 release notes 阶段建议 owner 让 release-codex 单独写一段 "default behavior change" 段落, 引用本 PR + CHANGELOG 行, 跟 #1408 / #1291 等其他 user-visible change 并列 callout。
4621a93 to
723d871
Compare
chenhg5
left a comment
There was a problem hiding this comment.
结论: Approve (re-review after rebase)
总体判断: 上一轮 (2026-06-21) 已经在 4621a93 APPROVE,本次 author 推到 723d871 (新增 863adda fix + 723d871 docs),diff 跟 4621a93 等价 + 加 docs 段。 Behavior change 思路正确, 解决 #1318 cron-stall 根因, 默认 new_per_run + 显式 reuse opt-in, docs/CHANGELOG/test/config 全覆盖。 CI 全绿, merge=clean, 跟 v1.4.0 主线无冲突。
Review 范围:
- 看了 15 files +127/-72 (CHANGELOG.md +2, cmd/cc-connect/cron.go +1/-1, cmd/cc-connect/timer.go +1/-1, config.example.toml +2/-2, config/config.go +1/-1, core/cron.go +14/-6, core/cron_test.go +22/-12, core/engine_test.go +5/-2, core/interfaces.go +2/-2, core/timer.go +4/-3, core/timer_test.go +33/-3, docs/management-api.md +1/-1, docs/management-api.zh-CN.md +1/-1, docs/usage.md +1/-1, docs/usage.zh-CN.md +1/-1)
- 重点关注 (1) 行为变更对老用户的兼容性 (2) NormalizeCronSessionMode 三态 (空/reuse/new_per_run) 正确性 (3) NewCronScheduler/TimerScheduler 默认值初始化 (4) test 覆盖 default 行为 (5) docs 完整性 (6) gofmt 干净
✅ 做得好的地方:
- 行为变更解 root cause: scheduled prompt jobs 默认 share active chat session 跟"scheduled task 不应该 block user chat"的产品语义冲突。 改为默认 new_per_run + 显式 reuse opt-in, 替换关闭的 #1318 cron-stall diagnosis, 跟 owner 期望的"scheduled 任务独立"语义对齐。
- defaultScheduledSessionMode 集中常量: 在 core/cron.go 加
const defaultScheduledSessionMode = "new_per_run", NewCronScheduler 和 NewTimerScheduler 都初始化这个常量, 跨 cron + timer 两侧 scheduler 同步切换, 避免单点修改。 未来要 revert 只需改这一行。 - NormalizeCronSessionMode 三态正确: 改前
case "", "reuse": return ""把 reuse 吞成 ""; 改后区分case "": return ""(用 default) /case "reuse": return "reuse"(显式 opt-in) /case "new_per_run", "new-per-run": return "new_per_run"。 配合 UsesNewSession 的 chain: job-level (new_per_run/reuse/空) → scheduler-level default → built-in default。 三态覆盖所有场景。 - Test coverage 升级: TestCronScheduler_AddJob_NormalizesSessionMode 改 table-driven 测 3 case (new-per-run alias → new_per_run; REUSE → reuse; 空 → 空), TestCronScheduler_UsesNewSession_GlobalDefault 改用新 default 语义, 新加 TestTimerScheduler_UsesNewSession_Defaults (4 case 覆盖 default + per-job + global 全部组合), TestValidateTimerJob 加 "valid reuse session_mode" subcase。 0 test gap。
- TestExecuteCronJob_ResolvesCronReplyTarget 显式加
SessionMode: "reuse": 因为行为变更后 default 是 new_per_run, 老 test 不显式 set reuse 会跑出不同结果。 作者主动调整老 test 兼容新 default, 避免 silently 改变 test 行为。 - CHANGELOG entry 完整: "cron/timer: prompt jobs default to a fresh agent session per run, avoiding busy chat sessions; use
session_mode = "reuse"to keep shared-session behavior"。 既有 caller-friendly wording (说"避免 busy chat sessions") 又有 opt-in 路径 (session_mode = "reuse"), 用户读到 changelog 不需要看 commit 也能理解怎么 revert。 - docs/usage.md / usage.zh-CN.md 完整: 把"可选 flag" 改成"By default, prompt cron jobs start a fresh agent session..." (en) 和 "默认情况下,prompt cron 每次触发都会使用新的 agent 会话..." (zh), 跟 CHANGELOG 表达一致, 老用户读到会立刻理解新语义。 docs/management-api.md / .zh-CN.md API 表格同步更新, cron API 文档跟 CLI 文档保持一致。
- gofmt + go vet + tests 全 clean:
gofmt -l core/cron.go core/timer.go config/config.go cmd/cc-connect/cron.go cmd/cc-connect/timer.go输出空,go vet ./core/... ./config/...clean,go test -count=1 ./core/...PASS 49.002s (含 10 个 cron/timer 专项 test)。
🚨/🔴 必须处理: 未发现
🟠 建议改进: 未发现
🔵 可选优化:
- (P3) cron.go
defaultScheduledSessionMode常量没在导出 API 里, 跟UsesNewSession/SetDefaultSessionMode不在同一文件层级。 如果未来要做 multi-tenant per-scheduler default, 可能需要把它移到 config/ 包或者 scheduler 配置里。 当前结构足够, 仅记录。 - (P3)
NormalizeCronSessionMode改后""和"reuse"显式区分, 但UsesNewSession内部还是return NormalizeCronSessionMode(j.SessionMode) == "new_per_run", 没显式看 scheduler default。 实际工作链:UsesNewSession在 scheduler 上调用, 应该会同时看cs.defaultSessionMode+j.SessionMode。 建议加注释说明 fallback 顺序, 避免 future reader 误以为只看 job-level。 当前实现正确, 仅 nit。
❓ 需要确认: 无
Testing / Risk:
- 已看到的验证:
CGO_ENABLED=0 go test -count=1 ./core/...PASS 49.002s (整包 + cron/timer 专项 test),./config/...PASS 0.396s,go vet ./core/... ./config/...clean,gofmt -l0 issue; CI lint+unit+smoke+regression+performance 全绿; PR mergeable=MERGEABLE, ci=success。 - 验证 gap: behavior change 端到端验证需要 user 配 cron + 模拟 active session busy 场景, 现有 test 只能 cover scheduler-level 逻辑。 建议 release notes 提醒 owner 上线时在 staging 验证一次 cron 在 user session busy 时的行为 (确认不 block / 不 wait)。
- 回归风险: default 变更对所有老用户有效, 但 opt-in 路径 (session_mode = "reuse" / --session-mode reuse) 完整保留, 老用户可以一行 config 切回旧行为。 Test 显式覆盖两种 mode, 0 silently change risk。 NewCronScheduler / NewTimerScheduler 初始化只加一行 defaultSessionMode: defaultScheduledSessionMode, 0 现有 cron 持久化 schema 变更, 老 store 数据自动 inherit 新 default (行为: 老 cron 默认从 share active session 变成 fresh session per run, 正是 PR 目标)。
Next step:
- author: 0 action; 可以等 owner merge。
- owner: 此 PR 适合 v1.4.1 stable (1.4.0-beta.2 已发布, 期间 cron-stall 反馈累积, 提早 backport 解 #1318 类问题)。
- release: release notes 强调"scheduled prompt jobs default to fresh session per run; 显式 opt-in
session_mode = "reuse"可恢复旧行为"。
🤖 Generated with Claude Code
1898f36 to
9bf03a1
Compare
78028b8 to
6a4b5f4
Compare
6a4b5f4 to
3e427ce
Compare
3e427ce to
1094207
Compare
|
Closing this older PR in favor of a fresh replacement from the same rebased patch. No code changes beyond the current approved/green head; the replacement PR will link back here. |
|
Replacement PR: #1496 |
chenhg5
left a comment
There was a problem hiding this comment.
结论: Approve (re-approve after rebase, with P2 suggestion)
总体判断: 之前在 commit 3e427ce 已 approve。当前 head 1094207 是 force-push 后只保留 2 commits 的版本(c6d4838 fix + 1094207 docs)。按 rebase-vs-approve memory,旧 APPROVE 在 commit hash 变化后失效,基于新 head diff 重新评审。本质是 default session_mode 从 "reuse"(effective empty) 翻转到 "new_per_run",是有意为之的 user-visible behavior change 修复 #1318 的 scheduled session 阻塞问题。文档/cli/help/mgmt-api 全套同步,CHANGELOG 有条目。属于 breaking change 但 owner 显式决策方向,文档同步到位,approve。
Review 范围:
- core/cron.go (+28): defaultScheduledSessionMode = "new_per_run" 常量、NewCronScheduler 默认赋值、NormalizeCronSessionMode / UpdateJob 接受 "reuse" 显式值、注释列对齐。
- core/timer.go (+19): NewTimerScheduler 同上。
- core/interfaces.go: CLI 帮助文本默认说明翻新。
- cmd/cc-connect/cron.go + timer.go: cron add / timer add 的 --session-mode 默认说明翻新。
- config/config.go: CronConfig.SessionMode 注释翻新。
- config.example.toml: 默认说明 + 新注释把 "reuse (default)" 换成 "new_per_run (default)"。
- docs/management-api.md + .zh-CN.md: API 文档默认说明翻新。
- docs/usage.md + .zh-CN.md: 用户文档重新表述为"默认 fresh session,可 --session-mode reuse 切回"。
- CHANGELOG.md: v1.3.3 release notes 段加入 cron/timer 默认切换条目。
- core/cron_test.go + core/timer_test.go + core/engine_test.go: 默认行为相关 case 全部翻新。
- 第二个 commit 1094207 仅追加 docs/core 的 scheduled session default 文档(无代码改动)。
✅ 做得好的地方:
- default flip 的实现用 const + scheduler 构造期赋值,常量值集中在一处;既有
SetDefaultSessionMode仍可全局覆盖,escape hatch 完整。 - CLI help / config example / mgmt API / user docs / CHANGELOG 五处文档全部同步翻新,没有一处遗漏 — 这种 cross-cutting 默认翻转最容易出现"代码改了但 docs 没改"的 review blocker,这里避免了。
- 测试既保留旧覆盖(
TestCronScheduler_UsesNewSession_GlobalDefault改成 3 个 sub-cases)又增加新矩阵(TestCronScheduler_AddJob_NormalizesSessionMode现在覆盖 new-per-run alias / reuse explicit / empty inherits),边界明确。 - 第二个 commit 1094207 单独追加
docs(core): document scheduled session default文档 commit,把 docs 与 code 拆分,便于以后单独回滚 docs 变更。
🟠 建议改进:
-
🟠 P2 CHANGELOG 行建议加 BREAKING 或 migration 前缀。当前条目是普通 bullet:
- **cron/timer**: prompt jobs default to a fresh agent session per run, avoiding busy chat sessions; use \session_mode = "reuse"` to keep shared-session behavior`建议改成:
- **cron/timer (BREAKING)**: prompt jobs now default to a fresh agent session per run to fix scheduled sessions blocking the active chat turn (issue #1318). Users relying on the previous shared-session behavior should set \session_mode = "reuse"` per-job or `[cron].session_mode = "reuse"` globally.`原因: 这是 default flip,用户升级后 scheduler 行为会变;虽然 docs/help 都改了,但 CHANGELOG 是 release 决策者最常扫的来源,显式 BREAKING 标记 可以让 reviewer / release manager 第一眼看到。
-
🟠 P2
config.example.toml的默认说明虽然改了,但 # configuration 那行是注释掉的形式 (# session_mode = "new_per_run"),用户升级 cc-connect 后不会自动迁移他们已有的config.toml。如果他们之前没显式设[cron].session_mode,升级后行为会静默变化。建议在 docs/usage.md 增加一段 "Upgrading from v1.3.3 or earlier" 显式说明。 -
🟠 P2
defaultScheduledSessionMode现在是 package-level const,导出但未在公开 API 暴露,如果未来需要其他包引用应该 export。当前未发现外部使用,不阻塞。
❓ 需要确认:
- 用户实际升级到本版本后,会不会有用户已经在 cron 里 prompt 写到"今天 session 上下文 X",升级后下个 cron run 拿到 fresh session 看不到 X,从而产生"我之前好好的,怎么升级后 cron 上下文没了"的体验问题?CHANGELOG 现在没明确告诉用户如何回退。
session_mode = "reuse"是 escape hatch 但需要用户自己想到去设置。 - issue #1318 的 reporter 是否确认了 default flip 是 OK 的方向,还是只是希望"修复 chat 阻塞"但保持默认 reuse?
Testing / Risk:
- 已看到的验证:core/cron_test.go + core/timer_test.go + core/engine_test.go 翻新 + 新 case 矩阵 + CHANGELOG 条目 + CLI/config/docs/mgmt-api/help 全套同步。
- 仍可补:实际部署里启用 cron + 默认 upgrade 后的回归测试 — 通常 release 后 user-ops 渠道会先发现,本 PR 难独立验证。
- 风险面:已升级用户默认行为变化。文档/CHANGELOG 都同步到位,owner release 决策时容易看到。
Next step: 作者无需返工。Owner 可以在合并前决定是否采纳 CHANGELOG BREAKING 标注建议(merge commit 时编辑)。本 PR 当前可合并,风险可接受。
Summary
session_mode = "reuse"or--session-mode reuseis set explicitly.Root cause
Scheduled prompt jobs inherited the active chat session by default. When a cron fired while the user session was already busy, it could fail with a busy-session error or wait behind unrelated interactive work. The scheduled task should be isolated by default instead of competing with normal chat turns.
Test Plan
go test -count=1 ./core ./configGOOS=linux GOARCH=amd64 go test -c -tags no_web ./cmd/cc-connectgit diff --checkReplaces the cron-stall diagnosis from #1318; that PR is closed.