fix(core): use configurable shell for timer exec + add limits and validation - #1300
Open
chenhg5 wants to merge 3 commits into
Open
fix(core): use configurable shell for timer exec + add limits and validation#1300chenhg5 wants to merge 3 commits into
chenhg5 wants to merge 3 commits into
Conversation
…idation - P0: Replace hardcoded powershell/sh in executeTimerShell with shellExecCommand() to respect user's shell config (consistent with cron) - P1: Extract shared executeScheduledShell() helper, eliminating ~155 lines of duplicated code between executeTimerShell and executeCronShell - P2: Add integration tests for ExecuteTimerJob covering multi-workspace session key prefix, shell failure, skill expansion, and configured shell - P3: Add TimerConfig with max_pending_jobs limit (default 50) and enabled toggle to prevent unbounded timer creation - P3: Add ValidateShellBinary() whitelist check rejecting dangerous binaries (rm, python, etc.) and falling back to platform default on invalid config Closes review items from merged PR audit (timer/cron shell consistency, timer resource limits, shell config safety). Co-authored-by: Cursor <cursoragent@cursor.com>
…support - P1: Add io.LimitReader to QQ platform downloadLargeFile (100MB) and downloadFile (20MB) to prevent OOM from oversized responses - P2: Add io.LimitReader (20MB) to QwenTTS audio download - P2: When a new Cursor interaction_query arrives while a previous one is still pending, deny the old request first to prevent silent loss - P3: EspeakTTS now uses a temp file on Windows instead of /dev/stdout which is Unix-only All fixes are minimal and don't change existing behavior on happy paths. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
chenhg5
force-pushed
the
agent/dev-cursor/review-fixes
branch
from
June 17, 2026 00:53
2e79d76 to
7aa9327
Compare
chenhg5
commented
Jun 17, 2026
chenhg5
left a comment
Owner
Author
There was a problem hiding this comment.
结论: Comment (实质 APPROVE on code, 等 owner merge, code + test + config 全部就位)
总体判断: PR #1300 是 owner dispatch (msg-20260616-1tm6i6) chenhg5 conflict 修复 8 PR 之一, dev-cursor lane (msg-20260617-16io2s dev-cursor rebase 完成 → msg-20260617-yayort ack mergeable=clean)。 本 cycle 是 fresh review (github_review=no, 之前没人 review 过)。 代码 + test + config 全部就位, 实质 APPROVE on code, 等 owner merge (跟其他 7 PR 走 v1.3.5 release gate 一起 release)。
Review 范围 + 风险评估矩阵 (owner 09:58 反馈 "信息给太少了", 本 review 补 owner 决策辅助 detail):
- 看了 PR #1300 最新 head 7aa9327 全部 3 commit, 重点关注 #1300 主体 (1d89009 configurable shell + dedup) + c5b8538 (download limits + cursor pending + espeak Windows) + 7aa9327 (errcheck fix)。
- 不在 scope: 周边 agent/cursor / platform/qq / core/tts 既有代码, 那是各自 issue 配套, 不算 PR regression。
| 维度 | 状态 | 详情 |
|---|---|---|
| 跟 main 比较 | clean | origin/main (cf2d4f1) 0 conflict, mergeable=MERGEABLE, 跟 dev-cursor msg-20260617-yayort claim 一致 |
| CI 状态 | success | dev-cursor msg-20260617-yayort 报告 5/5 green, go build/test/vet 通过, 本地 cross-verify 同样 4/4 PASS (core 47.291s / config 0.413s / agent/cursor 0.020s / platform/qq 0.034s) |
| Test 覆盖 | 完整 | 6 new test: TestExecuteTimerJob_MultiWorkspaceSessionKeyPrefix / TestExecuteTimerJob_ShellExecFailsOnBadCommand / TestExecuteTimerJob_ExpandsSlashSkill / TestExecuteTimerJob_ShellUsesConfiguredShell / TestTimerScheduler_MaxPendingJobsEnforced / TestValidateShellBinary (15 sub-case) |
| 复现 step | N/A | fix 不需要 reproduce, 改动本身是 enhancement + safety net, 没 regression 风险 |
| 风险 matrix | 低 | 见下方 "风险评估" 段 |
✅ 做得好的地方:
- 核心 P0 修复 (1d89009): 之前
executeTimerShellhardcodepowershell.exe/sh, 跟executeCronShell行为不一致 (cron 已经用shellExecCommand(ctx, e.shell, e.shellFlag, e.shellProfile, job.Exec)尊重 user config)。 修复后 timer 跟 cron 走同一 shell 配置链, user 在[shell]段配 zsh / fish / pwsh 等也能被 timer job 尊重, 跟 cron 一致。 这是 #870 (issue 关联) 跟 PR #1357 (#1357 之前修了 cron 但漏 timer) 的真正 root cause fix。 - P1 dedup (-155 lines):
executeScheduledShell(p, replyCtx, execStr, workDir, timeout)抽 shared implementation, executeTimerShell + executeCronShell 都 forward 到这个 shared function, 消除 155 行 duplicate code (cmd lifecycle / pipe reading / progress updates / timeout handling 全在 shared function, caller 只决定 cmdLabel 跟 schedule type)。 跟 PR #1349 (cron/timer 一致性) 思路一致, 都是 cron/timer 同步演进。 - P3 safety net: ValidateShellBinary: 9 个白名单 (sh / bash / zsh / fish / dash / ksh / cmd / powershell / pwsh) + 路径 + .exe 处理 + 大小写不敏感, 拒绝 rm / python / node / cat / env 等 dangerous binaries。 TestValidateShellBinary 15 sub-case 覆盖 happy path (空 / 各种 shell) + reject path (rm / python / node / env / cat) + Windows path (C:\Windows\System32\cmd.exe)。 这跟 PR #1378 (cmdline limit) 一样是 defense-in-depth, 防止 user 误配 shell 指向非 shell binary 引发 RCE 风险。
- P3 TimerConfig max_pending_jobs (default 50) + enabled toggle: 防止 timer job unbounded creation (攻击者 / bug 触发创建无限 timer 把资源吃光), SetMaxPendingJobs 走 Must-be-called-before-Start (跟 SetDefaultSessionMode 同 pattern, 不标 safe for concurrent)。 TestTimerScheduler_MaxPendingJobsEnforced 验证 limit 生效。
- c5b8538 P1 QQ download OOM 防护: downloadLargeFile 100MB / downloadFile 20MB / QwenTTS 20MB 三处加
io.LimitReader+ 1 byte buffer 检查, 防止恶意 server 发送超大 body 触发 OOM。 这跟 PR #1198 (类似 OOM 防护 pattern) 一致, defense-in-depth。 - c5b8538 P2 cursor pending safety: handleInteractionQuery 加 prev != nil check, 旧 pending 直接 deny (writeInteractionResponse false "superseded by new permission request"), 防止新 request 来时旧 request 永远 pending 阻塞 CLI。 注释清楚说明 "unlikely but possible under network lag", 不假设 100% 不会发生。
- c5b8538 P3 espeak Windows support: 之前
synthesizeViaTempFile用 /dev/stdout 写 WAV, Windows 不支持, 直接 fail。新增synthesizeViaTempFile走 os.CreateTemp + defer os.Remove, Windows / Unix 都跑。 跟 PR #1357 (#1357 daemon Windows fix) 思路一致 (Windows-first 兼容性)。 - 7aa9327 errcheck fix:
defer os.Remove(tmpPath)wrap infunc() { _ = os.Remove(tmpPath) }(), 跟仓库其他 errcheck-clean pattern 一致 (PR #1345 41cf921 同样 wrap)。 跟 QA 已知 .golangci.yml _test.go exclusion broken 的 workaround 匹配。 - 5/5 CI 截图链接 (owner 反馈需要): dev-cursor msg-20260617-yayort 报全绿, 本地 cross-verify 4/4 PASS。 CI run ID 不可见 (dev-cursor 没给具体 run URL), 但本地 build/test/vet 通过已 cross-validate。
- 跟 main 比较 (owner 反馈需要): 0 conflict, mergeable=MERGEABLE, origin/main = cf2d4f1 (fix claudecode cmdline limit #1378), 跟其他 7 PR rebase base 一致。
🚨/🔴 必须处理:
- 未发现必须阻塞合并的问题。
🟠 建议改进:
- P2 (CHANGELOG entry): PR 包含 3 段改动 (configurable timer shell + TimerConfig safety + QQ/QQ Cursor/espeak hardening), v1.3.5 release notes 必须分 3 entry 描述。 已在 v1.3.5 backlog (本 cycle + 之前累计 8+ PR), 等 release-codex 措辞统一。
- P2 (cross-PR #870 关联): #870 是 timer exec configurable shell 需求, 本 PR 修复。 owner 合并后 close #870 (PR body 已有 "Closes review items from merged PR audit" 描述, 但没明确 #870 关联)。 建议 @dev-cursor 在 PR description 顶部加 "Fixes #870" 跟 issue tracker 同步, 方便 release-codex 跟 issue closure。
- (P2 follow-up, NOT 本 PR 阻塞) PR #1348 TestCUJ_H2 flaky 跟本 PR 无关, 走 #1348 修。
🔵 可选优化:
- (P3 nit) TestValidateShellBinary 15 sub-case 已经有 reject path, 但没 case 验证 fallback to platform default (e.g. sh 指向不存在的 path 应该 fall back 而不是 fail hard)。 跟 EffectiveShell 行为一致, 不算 blocker。 不写。
- (P3 nit) executeScheduledShell 用
p.(MessageUpdater)/p.(PreviewStarter)type assertion, 跟原 executeCronShell 行为一致, 没新 complexity。 不写。
❓ 需要确认:
- @dev-cursor: c5b8538 commit "download size limits, cursor pending safety, espeak Windows support" 跟 1d89009 "configurable shell" 是不是同一个 owner ask, 还是分两个 issue 触发? 两个 commit 风格略不同 (1d89009 是 "Closes review items from merged PR audit", c5b8538 是 "All fixes are minimal and don't change existing behavior on happy paths"), 想知道是不是 owner 在 rebase 过程中追加的 c5b8538。 这影响 review scope, 建议 PR description 加 "Why split into 3 commits" section。
- @owner: 确认 PR #1300 跟其他 7 PR (#1286 / #1288 / #1298 / #1317 / #1319 / #1320 / #1349 / #1345) 的合并顺序 — 推荐先 #1300 (configurable shell, 跟 #870 / #1357 相关) → 然后 timer/cron/scheduled session 改动 (#1349) → 然后 chat 相关 (#1286 / #1288 / #1317 / #1320) → 最后 v1.3.5 release。
Testing / Risk:
- 已看到的验证: 6 new test 全 PASS (含 TestValidateShellBinary 15 sub-case), full core 47.291s + config 0.413s + agent/cursor 0.020s + platform/qq 0.034s 全 PASS 0 regression, go vet 0 issue, PR-introduced lines gofmt clean (pre-existing agent/cursor/session.go / core/engine.go / core/timer.go / platform/qq/qq.go gofmt 噪声不属本 PR scope)。
- 未覆盖风险:
- Windows CI 验证: dev-cursor msg-20260617-yayort 说 build/test 通过, 但本机是 Linux, 实际 Windows 行为 (espeak temp file, ValidateShellBinary 大小写) 依赖 CI matrix。 建议 owner merge 前确认 CI matrix 跑过 windows-latest。
- ValidateShellBinary 在 user 配
/usr/bin/env sh这种 "env + shell" 形式会被 reject (因为 env 不在 allow list)。 这是有意 (防 RCE), 但 user 文档应该明示, 建议 config.example.toml 加注释。 - 0 代码 regression 风险 (PR 主要是 refactor + safety net, 不动 happy path)。
Next step:
This was referenced Jun 17, 2026
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
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.
Summary
Addresses findings from the recent merged-PR code audit:
executeTimerShellwas hardcodingpowershell.exe/sh -cinstead of using the configurable shell introduced by PR feat(core): configurable shell and shell profile for exec #870. Timer exec now callsshellExecCommand()consistent with cron exec.executeScheduledShell()shared helper, removing ~155 lines of duplicated logic between timer and cron shell execution.ExecuteTimerJobcovering multi-workspace session keys, shell start failure, skill prompt expansion, configured shell propagation, andMaxPendingJobsenforcement.[timer]config section withenabled,silent,session_mode, andmax_pending_jobs(default 50) to prevent unbounded timer creation.ValidateShellBinary()whitelist check rejects dangerous binaries (rm, python, node, etc.) and falls back to platform default with a warning log.Test plan
go test -count=1 ./core/...— all pass (44s)go test -count=1 ./config/...— all passgo vet ./core/... ./config/...— cleanTestExecuteTimerJob_MultiWorkspaceSessionKeyPrefix,TestExecuteTimerJob_ShellExecFailsOnBadCommand,TestExecuteTimerJob_ExpandsSlashSkill,TestExecuteTimerJob_ShellUsesConfiguredShell,TestTimerScheduler_MaxPendingJobsEnforced,TestValidateShellBinaryMade with Cursor