fix: cron 观测缺口 — 费用记账 + 超预算不再静默成功

[lishuceo]

背景

定时任务「Watchlist 每日股票追踪」报 Reached maximum budget ($15)。排查出三个独立问题，本 PR 修其中两个观测缺口 + 清一个死字段。

先说清根因：$18.61 不是跨天累计，noResume 是生效的（日志 resume: false，全新 sessionId）。就是 08-02 这一次 26 分钟真的花了 $18.61。反推 opus-5 单价（用 $18.6066 精确拟合）后的成本构成：

项	用量	花费	占比
cache 写入	118 万	$7.36	40%
output	21 万	$5.25	28%
cache 读取	926 万	$4.63	25%
input	27 万	$1.37	7%

降成本（换模型、收窄 pm 的 29 skills / 51 tools、拆任务）不在本 PR 范围。

修的问题

① 错误型 result 的费用记成 0

SDK 对 error_max_budget_usd 不填顶层 usage（四个 token 计数全 0），calculateCostFromUsage 照它算就是 0。真实花费只在 total_cost_usd / modelUsage 里 —— 超预算恰恰是最该记账的场景。

新增 isUsageEmpty()，顶层 usage 为空且 total_cost_usd > 0 时回退到 SDK 累计值，日志加 costFallbackToSessionTotal 标明口径。usage 有值时行为不变，仍按单次用量算，避免 resume 首次 query 混入历史累计。

② cron 把超预算的空跑记成成功

scheduler 只靠「executeTask 有没有抛异常」判断成败，而 executeDirectTask / executeClaudeTask 都返回 void。SDK 拒绝执行时不抛异常，于是 run 记 status='ok' 且 cost_usd 为空。

08-01 那次 run 18 秒就"完成"（零 turn，什么都没干）却记成 ok —— 任务连续多天空跑，记录显示一切正常。cron_runs.cost_usd 字段一直存在、updateRun 一直支持写入，只是从来没人传值。

• 新增 TaskOutcome（success/costUsd/error/numTurns），从 ClaudeResult 提炼
• 两个 execute 函数覆盖全部出口返回 outcome（正常、workspace restart、resume 失败、catch）
• CronTaskExecutor 返回 TaskOutcome | undefined | void，返回 undefined 时保持旧行为
• executeJob 按 outcome.success 判定，失败复用既有退避重试分支；两条路径都写 cost_usd
• 零轮执行在 lastError 附诊断提示，不只丢一个 subtype

③ cron 的 maxBudgetUsd 死字段

cron_jobs 表和 CronJob 类型都有 maxBudgetUsd（出问题那个 job 存的是 5.0），但 scheduler.ts 调 executeTask 时从未传下去。预算实际一直由 agent 级配置决定，通过 cron 层设预算完全无效。

已从 types/store 移除。老库遗留物理列保留不 DROP（有 DEFAULT 5，省略插入不报错），避免对运行中库做不可逆改动。

测试

新增 6 个回归测试：

• 费用回退 3 例：usage 全 0 时回退（用线上真实 $18.61 字段）；usage 有值时不回退；真没花钱时保持 0
• cron 记账 3 例：成功 run 写入 cost；失败 outcome 记 error 且照样记账并触发退避；executor 返回 void 时保持旧行为
• 死字段 2 例：job 不再暴露 maxBudgetUsd；老库带 NOT NULL DEFAULT 5 遗留列时 add/update 仍正常

npx vitest run — 1784 passed / 1 failed。唯一失败是 memory/quality.test.ts > 当前工作重点，已 stash 后在干净树上复现，属既有失败，与本 PR 无关。

npm run typecheck 干净；npm run lint 0 errors。

未处理

• 降成本：换 sonnet / 收窄 pm 的 skills 与工具 / 拆任务 — 待定
• pm agent 配置：config/agents.json 里 "maxBudgetUsd": 15 未改（config 不入 git，热重载生效）
• cron 其余死字段：scheduler.ts 同样没传 model、timeoutSeconds、workingDir、repoUrl

🤖 Generated with Claude Code

[github-actions]

Claude finished @lishuceo's task in 2m 20s —— View job

---

Claude Code Review

• Check previous review comments (none found)
• Read CLAUDE.md & PR intent
• Review src/cron/types.ts
• Review src/cron/store.ts
• Review src/cron/__tests__/store.test.ts
• Verify no remaining maxBudgetUsd / max_budget_usd references
• Post review summary

---

✅ Approved — no blocking issues

The PR's core claim checks out. I verified at src/cron/scheduler.ts:190-199 that executeTask is called with only prompt/chatId/userId/messageId/rootId/threadId/agentId/accountId — maxBudgetUsd was indeed never threaded through, so removing it changes no runtime behavior. Budget is resolved from agent config at src/feishu/event-handler.ts:2899,3050,3475.

Cross-module verification:

• grep for maxBudgetUsd/max_budget_usd across src/ finds zero remaining cron-side reads — only comments and unrelated agent-config/pipeline call sites.
• src/cron/tool.ts never exposed the field (it doesn't expose model or timeoutSeconds either), so no MCP tool contract breaks and no agent-visible schema changes.
• All reads go through SELECT * (store.ts:307-315), so on legacy DBs the extra column comes back in the row and rowToJob (which builds an explicit object, no spread) simply ignores it. Narrowing CronJobRow without the field is a type-vs-runtime mismatch on a private interface only — harmless.
• The legacy-DB test using the stricter NOT NULL DEFAULT 5 form is a good call: it proves the omitted-column INSERT is safe under the tightest constraint the real column could have had.

🟡 Minor notes (non-blocking)

1. Stale schema in docs/plans/plan-7-cron-scheduler.md:124 (confidence: 85)

That plan's DDL snippet still lists max_budget_usd REAL DEFAULT 5, -- 单次执行预算上限. Its front matter has read_when: 修改 cron 相关代码, so per CLAUDE.md's agent workflow a future agent reading it before touching cron would be told this field exists and is meaningful — reintroducing exactly the misconception this PR removes. Worth a one-line fix in the same PR.

Fix this →

2. Rollback direction is one-way (confidence: 80, low impact)

Existing DBs keep the physical column, but DBs created after this change won't have it. If the service is ever rolled back to pre-PR code against such a DB, stmtInsertJob/stmtUpdateJob reference max_budget_usd, so db.prepare() throws in the CronStore constructor → startup failure, not a graceful degradation. In practice this only bites a brand-new deployment that then rolls back, so it may be fine to accept. If you want that closed cheaply, an idempotent migration in the same try/catch style as the neighbors also makes the schema uniform across old and new DBs:

try {
  this.db.exec(`ALTER TABLE cron_jobs ADD COLUMN max_budget_usd REAL DEFAULT 5`);
} catch { /* column already exists — ignore */ }

Your call — the current "keep the comment, don't touch the column" approach is defensible and clearly documented at store.ts:243-245.

Notes on scope

The PR's own "后续" section is accurate: model, timeoutSeconds, workingDir, and repoUrl are dead in the same way (confirmed at scheduler.ts:190-199). Leaving them for a follow-up is reasonable — timeoutSeconds/model at least still round-trip through the store, so removing them is a slightly larger surface. Worth filing as an issue so it doesn't get lost.

Testing

I could not execute npm run typecheck or npx vitest run in this environment (commands require approval that wasn't granted), so I reviewed statically and relied on the PR's reported results. Nothing in the diff suggests a type break: no other file constructs a CronJob literal or reads job.maxBudgetUsd, and excess-property checking would have surfaced any fixture that did.

Overall quality is good — tight scope, the reversible-migration choice is the right one for a running DB, and the regression tests target the two failure modes that actually matter.
· Branch feat/claude-session-764fc0

[github-actions]

Claude finished @lishuceo's task in 5m 24s —— View job

---

Claude Code Review

• Check previous review comments (no inline comments; 2 prior non-blocking notes re-checked)
• Read PR intent & CLAUDE.md
• Review new commits (cron 记账 / 费用回退 / 超预算空跑)
• Review src/claude/executor.ts + types.ts
• Review src/cron/scheduler.ts / store.ts / types.ts
• Review src/feishu/event-handler.ts, src/index.ts
• Trace all callers of the changed signatures
• Post review summary

---

✅ Approved — no blocking issues

The three fixes are independently correct and each is backed by a targeted regression test.

1. Dead-field removal (unchanged since my last pass) — verified again at src/cron/scheduler.ts:211-219: executeTask never received maxBudgetUsd, so removing it is behavior-neutral. Reads go through SELECT * + explicit rowToJob, so legacy rows with the extra column are simply ignored.

2. Cost fallback on error results (executor.ts:1351-1355) — logic checks out:

• usage absent entirely → already fell through to total_cost_usd before and after; unchanged.
• usage present but all-zero and total_cost_usd > 0 → uses SDK total. This is the real over-budget case (error_max_budget_usd reports zero top-level usage while modelUsage holds the actual $18.61).
• usage populated → still self-computes, so the resume-cumulative inflation guard the original code existed for is preserved. The third test (total_cost_usd: 99 vs 1500 real tokens) pins exactly that.
• Genuinely-free run stays at 0 because of the > 0 guard.

The "错误型 result 下为会话累计口径" caveat is honest and documented in both the code comment and TaskOutcome.

3. Silent-success fix (scheduler.ts:222-228) — routing the failure through CronTaskFailure into the existing catch is the right call: retry/backoff, run.error, and job.state stay in one place, and deleteAfterRun still cleans up one-shots on the failure path. outcome && outcome.success === false (rather than !outcome?.success) correctly preserves the "executor returned nothing → legacy behavior" contract, which the third scheduler test locks in.

Caller-side type safety verified: taskQueue/processQueue only does .then(() => ...) (event-handler.ts:722-724) and registerTask(promise: Promise<unknown>) (executor.ts:1584), so widening the return type breaks nothing; runWithAccountId is a generic passthrough over AsyncLocalStorage.run, so index.ts:148-176 propagates the outcome correctly.

🟡 Minor notes (non-blocking)

1. Two remaining silent-success holes in executeClaudeTask (confidence: 85, low frequency)

The new contract is "return undefined = can't determine = treat as ok," but two pre-existing bare return; paths inside the restart branch now fall into that bucket:

• event-handler.ts:2964 — newWorkingDir doesn't exist: the code sends the user a failure card (error: '工作区准备失败，目录不存在') and then returns undefined, so cron records the run as ok. This is exactly the class of bug this PR set out to kill.
• event-handler.ts:2937 — restart cancelled because the session is no longer busy (/stop). Arguably ok-ish, but cron will report a successful run for a task that never executed.

Both are reachable for a thread-mode cron job whose agent calls setup_workspace. One-liners:

// 2964
return { success: false, costUsd: result.costUsd, error: '工作区准备失败，目录不存在', numTurns: result.numTurns };

Fix this →

2. Restart branch reports only the second query's numTurns (confidence: 80, cosmetic)

event-handler.ts:3118-3123 sums cost (totalCostUsd = result.costUsd + restartResult.costUsd) but passes numTurns: restartResult.numTurns. Since CronTaskFailure keys its diagnostic hint on numTurns === 0 || 1 (scheduler.ts:39-41), a restart-then-fail run can get the "几乎零轮执行，通常是 SDK 直接拒绝了请求" hint even though the first query ran many turns. Summing turns like cost would keep the hint truthful.

3. Still outstanding from my previous pass — docs/plans/plan-7-cron-scheduler.md:124 still lists max_budget_usd REAL DEFAULT 5, -- 单次执行预算上限. Its front matter says read_when: 修改 cron 相关代码, so a future agent reading it before touching cron is told this field exists and is meaningful — reintroducing precisely the misconception this PR removes. The rollback-direction note (new DBs won't have the physical column; a rollback to pre-PR code would throw in the CronStore constructor) also still stands and is still a reasonable thing to accept.

Testing

I could not execute npm run typecheck or npx vitest run in this environment — both were denied by the permission layer, so I relied on the PR's reported results (1778 passed / 1 pre-existing failure) plus static tracing. Nothing in the diff suggests a type break: I checked every caller of the two widened signatures and every consumer of CronRunResult.costUsd (store.ts:539-548, which already accepts optional costUsd and maps undefined → null).

Overall: three tight, well-motivated fixes with genuinely good regression tests — the "executor returns nothing → legacy ok" test and the legacy-DB NOT NULL DEFAULT 5 test are the right paranoia. The PR body's stated scope no longer matches the commits (it only describes the dead-field removal); worth updating the description before merge so the cost-accounting fixes are discoverable in history.
· Branch feat/claude-session-764fc0