zero cron: dep-free file-backed scheduled agents (foreground runner) - #155
Conversation
…e-fire, +) - Next: fix infinite loop on DST spring-forward gaps (advance hours by absolute addition + a forward-progress guard); widen the search window to 9 years so a Feb-29 schedule across a century non-leap year (2096->2104) isn't reported as impossible. - store: reject path-traversal job ids (Get/Update/Remove/AppendRun) so 'cron rm ../..' can't delete outside the store; List now surfaces corrupt jobs as a warning instead of silently dropping them. - runner: reject impossible schedules at add even with --run-now; auto-pause a job whose schedule can no longer advance (was re-firing every tick); only skip-reschedule STRICTLY-overdue jobs on startup (keep exactly-due ones); capture stderr into RunRecord.Error on non-zero exit; pass the prompt via the inline --prompt= form so dash-leading prompts aren't misparsed. - Regression tests for each (DST, leap gap, traversal, corrupt-list, pause, reconcile, dash-prompt).
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughAdds a top-level ChangesCron Scheduling Feature
Sequence DiagramsequenceDiagram
participant User
participant CLI as "zero cron"
participant Store
participant Schedule
participant Exec as "zero exec"
User->>CLI: add / run / list / resume
CLI->>Store: Add/Get/List/Update
CLI->>Schedule: Parse / Next(now)
CLI->>Exec: run job (prompt,args)
Exec-->>CLI: exit code + stderr
CLI->>Store: AppendRun + Update(job)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
internal/cron/store_test.go (1)
72-90: ⚡ Quick winAdd regression coverage for
Runsunsafe IDs.Given the unsafe-ID protections elsewhere, add a
Runs("../x")/Runs("/abs")rejection test so traversal protections stay enforced for history reads too.Also applies to: 92-106
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cron/store_test.go` around lines 72 - 90, Update the unsafe-ID test to also assert that the Runs method rejects traversal/absolute IDs: in TestStoreRejectsUnsafeID add cases like "../x" and "/abs" (or similar unsafe strings) to the loop and call s.Runs(id) expecting a non-nil error, just as you do for s.Remove and s.Get; repeat the same addition for the other related test covering lines 92-106 so history reads are covered too, and keep the existing check that the sibling directory still exists after these calls.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/cron_run.go`:
- Line 121: The call to store.Update(j) currently ignores returned errors (seen
where store.Update is invoked with job variable j), risking stale NextRunAt and
lost run history; change these calls to check the error returned from
store.Update (and any similar store.Write/Save invocations) and handle failures
by logging the error via the existing logger and returning or propagating the
error (or retrying as appropriate) so the job state persistence cannot silently
fail; update the code paths that update j.NextRunAt and j.RunHistory to ensure
they only proceed after a successful store.Update and that failures surface to
the caller.
In `@internal/cli/cron.go`:
- Around line 119-121: The command currently silently ignores extra positional
arguments by taking only positional[0] into expr; update the cron add argument
parsing to reject extra positionals: if more than one positional token is
provided (i.e., len(positional) > 1) return a user-facing error (or print usage)
explaining that only a single expression/token is allowed. Locate the logic
around the variables positional and expr in the cron add handler (the block
containing "if len(positional) > 0 && expr == \"\" { expr = positional[0] }")
and add the validation there so additional tokens are not accepted silently.
- Around line 232-235: In cronResume, don't mark jobs active when schedule
parsing fails: only set job.Status = cron.StatusActive and update job.NextRunAt
= sched.Next(now()) inside the successful cron.Parse(job.Expr) branch (perr ==
nil); if cron.Parse returns an error, leave the job.Status unchanged (or set to
a failed/inactive state) and surface/log the parse error instead of resuming the
job so corrupted/impossible schedules are not reactivated.
In `@internal/cron/store.go`:
- Around line 210-220: The AppendRun write path currently defers f.Close() and
ignores its error; change it to capture and propagate Close() errors so buffered
write failures aren't dropped: in AppendRun (the block opening runs.jsonl and
using f, err, rec) remove the simple defer f.Close(), perform the write as
shown, then call cerr := f.Close() and if err == nil { err = cerr } before
returning so Close errors are returned when write succeeded but flush fails;
alternatively make the function use a named error return and a defer that sets
the named err to cerr if err == nil.
- Around line 223-229: The Runs method accepts an unvalidated id and joins it
into a filesystem path, enabling path traversal; validate/sanitize id before
calling s.jobDir(id) by cleaning it (e.g., filepath.Clean or filepath.Base) and
rejecting absolute paths or any cleaned id that equals "." or that begins with
"..", or alternatively build the full path and verify it is inside the cron root
with filepath.Rel against s.root; update Runs (and its use of s.jobDir) to
perform this check and return an error for invalid ids so callers cannot escape
the cron root.
---
Nitpick comments:
In `@internal/cron/store_test.go`:
- Around line 72-90: Update the unsafe-ID test to also assert that the Runs
method rejects traversal/absolute IDs: in TestStoreRejectsUnsafeID add cases
like "../x" and "/abs" (or similar unsafe strings) to the loop and call
s.Runs(id) expecting a non-nil error, just as you do for s.Remove and s.Get;
repeat the same addition for the other related test covering lines 92-106 so
history reads are covered too, and keep the existing check that the sibling
directory still exists after these calls.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 144fba40-e047-4516-8da1-e55bed81b84a
📒 Files selected for processing (13)
internal/cli/app.gointernal/cli/cron.gointernal/cli/cron_run.gointernal/cli/cron_run_test.gointernal/cli/cron_test.gointernal/cron/loopprompt.gointernal/cron/loopprompt_test.gointernal/cron/next_test.gointernal/cron/recipes.gointernal/cron/schedule.gointernal/cron/schedule_test.gointernal/cron/store.gointernal/cron/store_test.go
…traversal, +) - fireJob/reconcile/cronRun now surface AppendRun/Update failures to stderr instead of discarding them (a failed write no longer silently re-fires/loses history); fireJob takes stderr. - cron add rejects extra positional arguments instead of silently ignoring them. - cronResume rejects an unparseable/impossible schedule instead of reactivating a job with a stale/zero NextRunAt. - store.AppendRun returns the Close() error (buffered-write failures surface on close); store.Runs now validates the id (path-traversal guard, matching the other store methods). - tests: Runs traversal rejection, cron add extra-args, resume-impossible.
|
@coderabbitai review — all 5 findings addressed in dc073a3 (store-error surfacing, extra-arg rejection, resume schedule guard, AppendRun Close error, Runs traversal guard). gofmt/vet/build + Windows + |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get PR details and commits
gh pr view 155 --repo Gitlawb/zero --json commits,files 2>/dev/null | jq '.commits[-3:], .files[].path' 2>/dev/null || true
# Get the diff for the specific commit dc073a3
gh api repos/Gitlawb/zero/commits/dc073a3 2>/dev/null | jq '.files[] | {filename: .filename, patch: .patch}' 2>/dev/null || trueLength of output: 9905
All five are correct, targeted, and have regression tests. LGTM. [approve] |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/cli/cron_test.go (2)
25-25:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCheck store operation errors in tests.
Multiple store operation errors are ignored in this test (lines 25, 44, 51). If
List()orGet()fail, the test continues with zero-value returns, causing misleading assertions or false-positive passes.🛡️ Proposed fix for error handling
- jobs, _ := store.List() + jobs, err := store.List() + if err != nil { + t.Fatalf("list failed: %v", err) + }Apply similar fixes at lines 44 and 51:
- if j, _ := store.Get(jobs[0].ID); j.Status != cron.StatusPaused { + j, err := store.Get(jobs[0].ID) + if err != nil { + t.Fatalf("get failed: %v", err) + } + if j.Status != cron.StatusPaused {- if jobs, _ := store.List(); len(jobs) != 0 { + jobs, err = store.List() + if err != nil { + t.Fatalf("list after rm failed: %v", err) + } + if len(jobs) != 0 {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/cron_test.go` at line 25, The test ignores errors from store operations (store.List() and store.Get()), which can mask failures; update the calls in cron_test.go to capture the error return (e.g., jobs, err := store.List() and job, err := store.Get(id)) and assert/fail on error (use t.Fatalf or require.NoError(t, err)) before using the returned values so the test fails immediately when the store operations fail.
75-75:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCheck store.List() error.
If
List()fails, the test continues with a zero-value (empty slice), causing a misleading assertion or false-positive pass.🛡️ Proposed fix
- jobs, _ := store.List() + jobs, err := store.List() + if err != nil { + t.Fatalf("list failed: %v", err) + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/cron_test.go` at line 75, The test currently ignores the return error from store.List(), which can mask failures; update the cron_test.go test to capture the error from store.List() (e.g., jobs, err := store.List()) and immediately fail the test if err != nil (use t.Fatalf or require.NoError(t, err) consistent with the test suite) before asserting on jobs so a List() failure doesn't produce a false-positive.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/cli/cron_test.go`:
- Line 88: The test currently ignores the error return from store.List(), which
can mask failures; update the test to capture and assert the error from
store.List() (e.g., err := store.List(); if err != nil { t.Fatalf("List failed:
%v", err) } or use require.NoError(t, err)) before asserting len(jobs),
referencing the existing call to store.List() and the jobs variable so the test
fails loudly on List() errors instead of treating an error as an empty slice.
- Line 96: The test currently ignores errors from store.Add and store.Get which
can produce misleading results; update the test around the call to
store.Add(cron.Job{...}) and the subsequent store.Get(job.ID) to check their
returned errors and fail the test immediately (e.g., t.Fatalf or equivalent)
when err != nil so you don't proceed with a zero-value job or an empty job.ID;
reference the Add and Get calls and the job variable as the locations to add
these checks and ensure any subsequent use (including invoking the resume
command) only runs when Add and Get succeeded.
---
Outside diff comments:
In `@internal/cli/cron_test.go`:
- Line 25: The test ignores errors from store operations (store.List() and
store.Get()), which can mask failures; update the calls in cron_test.go to
capture the error return (e.g., jobs, err := store.List() and job, err :=
store.Get(id)) and assert/fail on error (use t.Fatalf or require.NoError(t,
err)) before using the returned values so the test fails immediately when the
store operations fail.
- Line 75: The test currently ignores the return error from store.List(), which
can mask failures; update the cron_test.go test to capture the error from
store.List() (e.g., jobs, err := store.List()) and immediately fail the test if
err != nil (use t.Fatalf or require.NoError(t, err) consistent with the test
suite) before asserting on jobs so a List() failure doesn't produce a
false-positive.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c795bd74-51ea-427e-8138-40dc90b73ae3
📒 Files selected for processing (5)
internal/cli/cron.gointernal/cli/cron_run.gointernal/cli/cron_test.gointernal/cron/store.gointernal/cron/store_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/cli/cron_run.go
- internal/cron/store_test.go
- internal/cron/store.go
- internal/cli/cron.go
CodeRabbit (critical): the cron CLI tests ignored errors from store.List/Add/Get, so a store failure would surface as a misleading assertion on a zero-value job/slice instead of a clear failure. Check every store op's error across the test file (not only the two flagged sites).
|
Fixed (critical): the cron CLI tests now check every |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Get the latest commits on the PR
gh pr view 155 --repo Gitlawb/zero --json commits | jq '.commits[-3:] | .[] | {sha: .oid, msg: .messageHeadline}'
# Get the full diff of the latest commit
LATEST=$(gh pr view 155 --repo Gitlawb/zero --json commits | jq -r '.commits[-1].oid')
echo "Latest commit: $LATEST"
gh api repos/Gitlawb/zero/commits/$LATEST 2>/dev/null | jq -r '.files[] | "=== \(.filename) ===\n\(.patch)"'Length of output: 3984
No remaining [approve] |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: request changes.
Finding:
- P1 internal/cron/store.go:65: DefaultRoot does not actually mirror sessions.DefaultRoot. It reads only XDG_DATA_HOME/HOME from envMap, and if HOME is unset it falls through to filepath.Join("", ".local", "share"), which is a relative .local/share path. On Windows/PowerShell or other environments without HOME, zero cron add/list/run will store jobs under the caller's current working directory instead of a stable user data directory, so jobs disappear when running from a different cwd and may create repo-local .local data. sessions.DefaultRoot falls back to os.UserHomeDir() when HOME is empty; cron should do the same, and the test should cover empty HOME.
Validation in an isolated PR worktree passed before this review: git diff --check origin/main...HEAD, go test ./internal/cron ./internal/cli, and go build ./cmd/zero. The scheduler/parser path otherwise looks solid.
# Conflicts: # internal/cli/app.go
Vasanth (P1): cron's DefaultRoot mirrored neither sessions.DefaultRoot nor reality — with no XDG_DATA_HOME and no HOME it produced a RELATIVE ".local/share/zero/cron" under the caller's cwd, so on Windows/restricted shells cron jobs scattered per working directory and could write repo-local data. Now falls back to os.UserHomeDir() like sessions.DefaultRoot. Adds TestDefaultRootEmptyHomeFallsBackToUserHome (asserts the path stays absolute).
|
@Vasanthdev2004 fixed (P1): |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Verdict: approve.
Reviewed latest head 8a2cbe2. The previous blocker around cron.DefaultRoot is fixed: it now mirrors the sessions store behavior and falls back to os.UserHomeDir() when HOME is unset, so cron data will not scatter into a relative .local/share path under the caller's cwd.
Validation run locally:
go test ./internal/cron ./internal/cligo test ./...go vet ./...go run ./cmd/zero-release buildgo run ./cmd/zero-release smokegit diff --check origin/main...HEAD- manual:
go run ./cmd/zero cron --help
Notes: the cron parser/store/runner boundaries look coherent, the foreground runner uses the existing zero exec path, unsafe IDs are guarded, loop prompt loading skips symlinks and caps size, and corrupt-job listing degrades without hiding healthy jobs. No remaining blockers from my pass.
2 participants
Summary
Adds
zero cron— define file-backed scheduled agent jobs (standard 5-field cron) and a foregroundzero cron runthat fires due jobs by reusing the existingzero execpath. Dep-free, additive (newinternal/cronpackage + new command).This is the network-free, no-daemon residue of the upstream
05-daemon-cronmodule — the entireTuiDaemonAdapter/daemon/IPC, the second "automations" job system, the TUI leave-warning, and locale time handling are intentionally not ported.Commands
minute hour day-of-month month day-of-week— a small dep-free parser (tokens*, lists, ranges, steps, month/weekday names,7=Sun) with standard Vixie DOM/DOW OR semantics, evaluated in local time.run: foreground loop (Ctrl+C/SIGTERM to stop);--oncefires currently-due jobs and exits (wire it under an external scheduler);--catch-upfires overdue jobs on start, otherwise the default is skip-and-reschedule.zero exec --output-format stream-json --session-title cron:<id> --prompt=<job prompt>, reusing the agent + session persistence; the exit code is recorded toruns.jsonl.$XDG_DATA_HOME/zero/cron/<id>/(metadata.json+ append-onlyruns.jsonl), mirroringsessions.Store.--recipes +loop.mdprompt resolution (25KB cap, symlink-reject).Design
internal/cron(pure, stdlib-only):Schedule(Parse/Next/String), file-backedStore, recipes, loop.md resolution.Now/RootDirinjectable for tests.internal/cli/cron.go+cron_run.go: command group + foreground runner behind an injectable exec seam (defaultcli.Run) for testability. Dispatch case + help inapp.go.Built with an adversarial workflow
Implemented via a multi-agent workflow (6 sequential TDD tasks, each verified) followed by a 3-lens adversarial panel (parser correctness, runner semantics, arg-safety). The panel found and this PR fixes 9 real issues, including:
Next()infinite loop on a DST spring-forward gap (30 2 * * *on the transition day) — fixed (absolute-time hour advance + forward-progress guard). Regression-tested againstAmerica/New_York.cron rm ../..couldRemoveAlloutside the root) — job ids now validated.--run-nowbypassed the impossible-spec guard) — impossible specs rejected at add; unadvanceable jobs auto-pause.RunRecord.Errorcapture, and dash-leading prompts via the inline--prompt=form.Test Plan
go build ./.../GOOS=windows GOARCH=amd64 go build ./...cleango vet ./...clean;gofmtcleango test ./...green (parser table,Nextrollovers/DST/leap/impossible, store round-trip + traversal + corrupt, runner once/catch-up/overlap/pause/reconcile, CLI)go test -race ./internal/{cron,cli}/...greenReviewer focus
Next(local time, Vixie OR, DST gap, leap window).cron runis active (documented); skip-and-reschedule default vs--catch-up.add); unattended fires inherit the normal autonomy/sandbox ceiling.Summary by CodeRabbit
New Features
Tests