feat(automation): close PR1-5 backend gaps before frontend slice#1045
Conversation
Adds the AutomationDefinition contract pieces missed by PR1-5 so the
PR6/PR7 frontend can build on a complete backend surface:
- `model: { providerID, modelID }` required on every definition; passed
through to `SessionPrompt.promptWithAutomationContext` so runs stop
depending on the runtime model fallback chain.
- `variant?: string` optional reasoning/effort selection; validated
against `ProviderTransform.variants(model)` so traffic that's not
valid for the chosen model returns 422 instead of failing late in
the run.
- `stop.kind === "condition"` is now rejected with
`unsupported_stop_condition` at create/update — the scheduler never
scheduled it, and the silent-accept behaviour leaked UI surface area.
- `nextFireAt`, `nextFires`, `failureStreak` are now maintained
on create, on update, and after every terminal run; the scheduler
publishes `automation.definition.updated` so global-sync sees the
derived state.
- `automate` tool schema picks up `model`/`variant`, gets a fake
Provider for tests, and the description spells out the expected
shape and an example so the LLM submits valid inputs.
A SQL migration drops any pre-release automation rows because the
existing payloads cannot be parsed against the new required `model`
field; the feature is still gated behind
`OPENCODE_ENABLE_AUTOMATE_TOOL` so no user-visible data is lost.
Out of scope (deferred to follow-ups):
- `needs_user_input` / `loop_gate` real-world production. Both are
reserved in the run error code enum; producing them requires
changing prompt loop semantics (unattended question abort + loop
gate failure surfacing) which is too big for PR5.5.
- `Session.automationID?` reverse lookup. That's a session contract
migration; tracked separately and not needed by PR6.
Verification:
- `bun typecheck` (repo) clean
- `bun test test/server/automation-*.test.ts test/tool/automate.test.ts`
118 pass / 0 fail
- `bun test test/session/processor-effect.test.ts` 35 pass / 0 fail
- SDK regen produces no diff against manually-edited types
📝 WalkthroughWalkthroughThis PR requires a model selection (providerID/modelID) and optional variant for automations, adds cron/interval-derived next-fire computation, validates model/variant via Provider lookups on create/update and via the automate tool, refactors scheduler/runner to use the cron module and to publish refreshed definitions after run outcomes, and updates tests/CI/fixtures accordingly. ChangesAutomation Model Support & Scheduling
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
🚥 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Code Review
This pull request introduces a required model field and an optional variant field to the automation definitions, updating schemas, validation logic, database migrations, and tests accordingly. It also implements derived field calculations for recurring automations. The review feedback highlights a critical performance issue in the brute-force cron lookahead search of nextCronFires that could block the event loop, and suggests an optimized jumping search algorithm. Additionally, the feedback recommends allowing the variant field to be null in update inputs to support clearing it, along with the necessary handling in the update and validation paths.
…ng variant - nextCronFires: jumping search (months → days → hours), capped lookahead. Brute-force minute-by-minute over 5y blocked the event loop. - update(): only recompute nextFireAt/nextFires when rhythm/stop/timezone/paused actually changes. Title/prompt edits no longer postpone pending interval runs. - UpdateInput.variant: accept null to clear a previously set effort when the new model has no compatible variant.
…runs - Extract cron parser/matcher into automation/cron.ts; scheduler and derived now share one implementation (removes ~70 lines of duplication). - recordRunOutcome: stopped runs keep previous nextFireAt/nextFires. Manual cancel, writer conflicts, and missed-schedule stops must not re-anchor the recurring preview — only succeeded/failed advance it. - Add unit coverage for validateModelAndVariantWith with real provider interface so HTTP route 422 details (model_not_found / model_lookup_failed / invalid_variant_for_model) are exercised end-to-end, not just bypassed via env flag. - Assert scheduler-stop + manual-stop both leave failureStreak / nextFireAt / nextFires / revision intact.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/opencode/test/fake/provider.ts (1)
30-40: ⚡ Quick winWrap these fake provider methods with
Effect.fn(...)or build this helper fromProviderTest.fake(...).Right now
fakeAutomationProvider()open-codes raw Effect lambdas, so these calls lose the naming/tracing the repo expects and can drift from the existing fake-provider behavior defined lower in this file.As per coding guidelines "Use
Effect.fn("Domain.method")for named/traced effects andEffect.fnUntracedfor internal helpers."🤖 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 `@packages/opencode/test/fake/provider.ts` around lines 30 - 40, The provider fake currently returns raw Effect lambdas losing tracing; update the Provider.Interface implementation (the object assigned to iface used by fakeAutomationProvider) so each method (list, getProvider, getModel, getLanguage, closest, getSmallModel, defaultModel) is wrapped with Effect.fn("Provider.<methodName>") (or replace the whole helper by calling ProviderTest.fake(...) if available) instead of returning unwrapped Effect.succeed/Effect.die—this preserves naming/tracing per guidelines (use Effect.fnUntraced only for true internal helpers).packages/opencode/test/automation/validation.test.ts (1)
1-10: ⚡ Quick winUse
testEffect(...)for this Effect-driven suite.These cases run
validateModelAndVariantWith(...)viaEffect.runPromise, so they bypass the repo’s standard Effect test harness. Please move this file ontotestEffect(...)with the test body insideEffect.gen(...).As per coding guidelines "Use
testEffect(...)fromtest/lib/effect.tsfor tests that exercise Effect services or Effect-based workflows."🤖 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 `@packages/opencode/test/automation/validation.test.ts` around lines 1 - 10, Tests are calling validateModelAndVariantWith(...) via Effect.runPromise which bypasses the repo's Effect test harness; replace those with testEffect(...) (imported from test/lib/effect.ts) and move each test body into Effect.gen(...) so the Effect is executed under the harness. Update the helper runDetails or inline calls so they return an Effect (use Effect.gen to yield validateModelAndVariantWith) instead of calling Effect.runPromise, and ensure tests use testEffect(() => Effect.gen(function*() { ... yield* ... })) referencing validateModelAndVariantWith and Provider.Interface accordingly.packages/opencode/test/tool/automate.test.ts (1)
12-46: ⚡ Quick winReuse
fakeAutomationProvider()here instead of maintaining a second provider stub.This fixture duplicates the provider/model setup from
packages/opencode/test/fake/provider.ts, so variant/default-model behavior can drift between automate tests and the other automation suites.🤖 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 `@packages/opencode/test/tool/automate.test.ts` around lines 12 - 46, The test duplicates provider/model setup (fakeProviderID, fakeModelID, fakeModel, fakeInfo, fakeProviderInterface, fixtureModel) that already exists in fakeAutomationProvider(); replace this block by importing and using fakeAutomationProvider() from the shared test helper and remove the duplicated declarations. Specifically, drop the manual uses of ProviderID.make, ModelID.make, ProviderTest.model, ProviderTest.info and the custom fakeProviderInterface, and instead call fakeAutomationProvider() (or destructure its returned provider interface, providerID/modelID and fixture) and use those symbols throughout the test so variant/default-model behavior stays consistent across suites.
🤖 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 `@packages/opencode/test/server/automation-routes.test.ts`:
- Line 16: The test file currently sets
process.env.OPENCODE_SKIP_AUTOMATION_MODEL_VALIDATION at module scope which
leaks state; modify the file to save the original value into a variable (e.g.,
const _prevSkip = process.env.OPENCODE_SKIP_AUTOMATION_MODEL_VALIDATION), then
set process.env.OPENCODE_SKIP_AUTOMATION_MODEL_VALIDATION = "1" inside a
beforeAll (or at module start) and restore it in an afterAll by assigning
process.env.OPENCODE_SKIP_AUTOMATION_MODEL_VALIDATION = _prevSkip (or delete it
if undefined). Reference the environment key
OPENCODE_SKIP_AUTOMATION_MODEL_VALIDATION and use Jest lifecycle hooks
beforeAll/afterAll (or teardown/afterEach) to ensure the original value is
restored after the suite.
---
Nitpick comments:
In `@packages/opencode/test/automation/validation.test.ts`:
- Around line 1-10: Tests are calling validateModelAndVariantWith(...) via
Effect.runPromise which bypasses the repo's Effect test harness; replace those
with testEffect(...) (imported from test/lib/effect.ts) and move each test body
into Effect.gen(...) so the Effect is executed under the harness. Update the
helper runDetails or inline calls so they return an Effect (use Effect.gen to
yield validateModelAndVariantWith) instead of calling Effect.runPromise, and
ensure tests use testEffect(() => Effect.gen(function*() { ... yield* ... }))
referencing validateModelAndVariantWith and Provider.Interface accordingly.
In `@packages/opencode/test/fake/provider.ts`:
- Around line 30-40: The provider fake currently returns raw Effect lambdas
losing tracing; update the Provider.Interface implementation (the object
assigned to iface used by fakeAutomationProvider) so each method (list,
getProvider, getModel, getLanguage, closest, getSmallModel, defaultModel) is
wrapped with Effect.fn("Provider.<methodName>") (or replace the whole helper by
calling ProviderTest.fake(...) if available) instead of returning unwrapped
Effect.succeed/Effect.die—this preserves naming/tracing per guidelines (use
Effect.fnUntraced only for true internal helpers).
In `@packages/opencode/test/tool/automate.test.ts`:
- Around line 12-46: The test duplicates provider/model setup (fakeProviderID,
fakeModelID, fakeModel, fakeInfo, fakeProviderInterface, fixtureModel) that
already exists in fakeAutomationProvider(); replace this block by importing and
using fakeAutomationProvider() from the shared test helper and remove the
duplicated declarations. Specifically, drop the manual uses of ProviderID.make,
ModelID.make, ProviderTest.model, ProviderTest.info and the custom
fakeProviderInterface, and instead call fakeAutomationProvider() (or destructure
its returned provider interface, providerID/modelID and fixture) and use those
symbols throughout the test so variant/default-model behavior stays consistent
across suites.
🪄 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: 4e9f840e-6e5f-446b-b2ff-24dbae86a776
⛔ Files ignored due to path filters (1)
packages/sdk/js/src/v2/gen/types.gen.tsis excluded by!**/gen/**
📒 Files selected for processing (17)
packages/opencode/migration/20260601100000_automation_model_required/migration.sqlpackages/opencode/src/automation/cron.tspackages/opencode/src/automation/derived.tspackages/opencode/src/automation/fixtures.tspackages/opencode/src/automation/index.tspackages/opencode/src/automation/runner.tspackages/opencode/src/automation/scheduler.tspackages/opencode/src/automation/validation.tspackages/opencode/src/server/instance/automation.tspackages/opencode/src/tool/automate.tspackages/opencode/test/automation/validation.test.tspackages/opencode/test/fake/provider.tspackages/opencode/test/server/automation-routes.test.tspackages/opencode/test/server/automation-runner.test.tspackages/opencode/test/server/automation-scheduler.test.tspackages/opencode/test/tool/automate.test.tspackages/sdk/js/src/v2/event-types.test-d.ts
ci-workflow sentinel requires every opencode test file to be covered exactly once across the three Windows shards; the new test/automation/ directory was missing, breaking unit-opencode > check > unit results (opencode). Also tighten env-bypass lifecycle in automation-routes.test.ts (CodeRabbit): restore OPENCODE_SKIP_AUTOMATION_MODEL_VALIDATION in afterAll instead of leaking module-scope mutation across the suite.
…422 wiring scheduler-owned stopped (manual conflict, missed_schedule, writer block) now refreshes nextFireAt so the UI never shows a fire time in the past; non-owned stopped still freezes derived fields. Adds route-level 422 tests with provider validation enabled to exercise the production path that the env-bypassed suite skips. Threads Effect.fn tracing through the fake provider and lifts validation/automate fixtures onto it.
…heck cast scheduler now tags DefinitionUpdated events it emits from recordRunOutcome's stopped-run refresh and skips reschedule on receipt, so a refresh → publish → reschedule cycle cannot self-pump under an adversarial clock. Drops the OversleepClock-based recurring missed schedule test that exposed the issue: that clock pins time at a fixed oversleptAt, so any subsequent recurring scheduleNextInterval picks a fireAt strictly above clock.now() and waitUntil starves the loop with microtasks. Manual-conflict coverage already exercises the scheduler-owned advance path. Also restore the dev-style 'as never' cast pattern on the three provider stubs in validation.test.ts so typecheck stops rejecting the non-overlapping Effect.fail/Effect.die narrowing.
…tion - Skip variant:null updates when previous variant is unset to avoid no-op revision bumps and stray DefinitionUpdated events. - Split Stop into SupportedCreateStop (no condition) for CreateInput / UpdateInput / automate tool so the input contract no longer advertises the kind the API rejects. - Collapse cron validation to a single isValidCronExpression in automation/cron.ts; index.ts now re-exports the shared helper so validate / tool / derived / scheduler share one source of truth. - Key scheduler's self-published DefinitionUpdated guard by id:revision and clear it when publish rejects, so a real concurrent update racing ahead of the self event isn't swallowed.
Previously a ConflictError from a concurrent definition write caused the run outcome (failureStreak / nextFireAt / nextFires) to be silently dropped; the next definition.updated event then reflected the racing edit but lost the run's contribution. Retry the read+compute+replace loop up to three times so a race against an unrelated update doesn't strand the user on stale derived fields.
…conflict retry
Stop schema previously split into SupportedCreateStop (no condition)
which dropped the structured `{ field: "stop", message: "unsupported_stop_condition" }`
422 detail that the PR description promised. Restore the unified Stop
schema and the validate-layer rejection; the agent-facing automate tool
keeps condition out of its own schema separately so LLMs do not try it.
Also add an internal __testBeforeReplace hook to recordRunOutcome so a
test can deterministically force a ConflictError between the read and
the replace, exercising the retry path end-to-end and asserting the
racing edit's title is preserved alongside the run's failureStreak.
….Stop The Stop schema rollback in c6c7b5b restored condition in Automation.CreateInput but the automate tool's Effect Schema still excluded it, breaking the type relationship between the zod recurring input fed into tool.execute and the tool's parameters. Restore condition in the tool schema and add a description line steering the LLM away from it; validate-time rejection still surfaces { stop, unsupported_stop_condition }.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/opencode/test/automation/validation.test.ts (1)
1-7: ⚡ Quick winUse a file-local
const it = testEffect(...)inpackages/opencode/test/automation/validation.test.tsinstead of importing the shared{ it }export.
This file importsitfrom../lib/effect, but the repo test guideline expectsconst it = testEffect(...)near the top of each test file forit.effect(...)usage.🤖 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 `@packages/opencode/test/automation/validation.test.ts` around lines 1 - 7, Replace the shared import of it from "../lib/effect" with a file-local declaration; remove the imported it and add a local const it = testEffect(...) near the top of the test file so tests use the file-local it.effect(...) helper; specifically, stop importing the symbol it and instead call testEffect to create a local const named it (matching existing usage of it.effect and keeping other imports like describe/expect unchanged).
🤖 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 `@packages/opencode/test/server/automation-routes.test.ts`:
- Around line 146-167: The test "create rejects with 422 invalid_automation
details when provider lookup fails" depends on an environment-specific model;
modify the POST body to force an impossible model so the provider lookup always
fails: call recurringInput(projectID) then overwrite its model property with a
guaranteed-invalid pair (e.g., provider: "nonexistent", model: "no-such-model")
before sending the request; update the test that uses recurringInput(projectID)
in this test block so the response is deterministically 422 and the existing
assertions against body.error and body.details still apply.
In `@packages/opencode/test/server/automation-scheduler.test.ts`:
- Around line 1004-1014: The test uses a fixed Bun.sleep(10) after
Automation.runNowExecuting which races on CI; replace the fixed sleep with
polling helpers like waitForRunStates or waitForRunCount to wait until the run
reaches the terminal "failed" state. Specifically, after calling
Automation.runNowExecuting(... executor ...) (and similarly in the second
occurrence noted), call waitForRunStates or waitForRunCount to poll
Automation.runs({ automationID: created.id }) until a run.state === "failed" is
observed, rather than relying on Bun.sleep; keep the existing
Automation.markRunStarted and Automation.publishRunUpdated calls inside the
executor unchanged.
---
Nitpick comments:
In `@packages/opencode/test/automation/validation.test.ts`:
- Around line 1-7: Replace the shared import of it from "../lib/effect" with a
file-local declaration; remove the imported it and add a local const it =
testEffect(...) near the top of the test file so tests use the file-local
it.effect(...) helper; specifically, stop importing the symbol it and instead
call testEffect to create a local const named it (matching existing usage of
it.effect and keeping other imports like describe/expect unchanged).
🪄 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: 0532a3ea-008f-411b-85df-d8de882db3e0
📒 Files selected for processing (12)
.github/workflows/windows-advisory.ymlpackages/opencode/src/automation/cron.tspackages/opencode/src/automation/index.tspackages/opencode/src/automation/scheduler.tspackages/opencode/src/tool/automate.tspackages/opencode/test/automation/cron.test.tspackages/opencode/test/automation/validation.test.tspackages/opencode/test/fake/provider.tspackages/opencode/test/github/ci-workflow.test.tspackages/opencode/test/server/automation-routes.test.tspackages/opencode/test/server/automation-scheduler.test.tspackages/opencode/test/tool/automate.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/opencode/test/fake/provider.ts
- packages/opencode/src/automation/scheduler.ts
- packages/opencode/src/tool/automate.ts
- packages/opencode/test/tool/automate.test.ts
…lling
- automation-routes.test.ts: 422 create-path now sends an explicitly
invalid model { providerID: "nonexistent", modelID: "missing-model" }
instead of relying on the suite-wide fixture model being unavailable in
the test environment. Mirrors the update-path test below it.
- automation-scheduler.test.ts: add waitForFailedRunCount polling helper
and use it in both recordRunOutcome tests in place of `Bun.sleep(10)`
after `runNowExecuting`. Fixes a latent CI flake where the failed run
may not have been persisted before the assertion reads it.
…ptions
The conflict-retry test injected its concurrent write through an
__testBeforeReplace field on recordRunOutcome's options object, which
leaked a test-only seam into the function's exported type signature.
Move the hook to a module-level Automation.__testHooks seam that
production code never touches; the test now sets
__testHooks.beforeReplaceDefinition and clears it in finally, while
recordRunOutcome's options shrink to { now?, refreshOnStopped? }.
…ation API The previous attempt parked the ConflictError test seam at Automation.__testHooks, which kept it inside the public namespace export and still appeared on the Automation.* API surface. Move it to a sibling module src/automation/__test_hooks.ts that only the ConflictError retry path in recordRunOutcome and the matching test import. The Automation namespace no longer exposes a writable test hook; consumers reading the public type surface see no test-only fields.
Summary
Closes backend gaps left after PR1–5 so the PR6/PR7 frontend slice can build on a complete contract. Adds required
model(and optionalvariant) on everyAutomationDefinition, rejectsstop.kind === "condition"with a structured 422 detail, persists derived scheduling fields (nextFireAt,nextFires,failureStreak) at create / update / after every terminal run, and extends theautomatetool schema + Provider-backed validation so LLM-submitted automations are checked at create time instead of failing inside the run.Why
PR1–5 stood up the automation contract, scheduler, persistence and worktree surface, but left several pieces the frontend would have to handle at runtime:
stop.kind === "condition"was accepted at create time butcanScheduleRecurring()returnedfalse, so the UI could surface schedules that would never fire.nextFireAt/nextFires/failureStreakwere never persisted, so global-sync had nothing to render after a run completed.automatetool schema had no model/variant slots and no Provider-backed validation, so invalid LLM submissions only failed deep inside the run.PR6 (sidebar panel) and PR7 (form / templates / NL card) need this surface complete and stable before they can start.
Related Issue
docs/architecture/2026-05-27-issue-950-automation-pr-plan.md)Human Review Status
Approved by @Astro-Han
Review Focus
model/variantvalidation contract — create + update routes callProviderTransform.variants(model)and return422 invalid_automationwith structured{ field, message }details. Invariant: invalid model/variant never reaches the runner.{ kind: "condition" }(kept for SDK shape parity with future evaluator support) but the validate layer rejects with{ field: "stop", message: "unsupported_stop_condition" }. Check that the structured detail is preserved across both HTTP and theautomatetool error path.recordRunOutcomere-publishesautomation.definition.updatedafter every terminal run; scheduler uses aselfPublishedDefinitionUpdatesset keyed byid:revisionto skip its own echoes. Check the race window and theConflictErrorretry loop.automatetool schema — picks upmodel/variant; description points the LLM at supportedstopkinds; tool init now takesProvider.Interfaceso model existence is checked at create time.Risk Notes
20260601100000_automation_model_requireddeletes existingautomation_definition/automation_runrows so the new NOT NULLmodelcolumn is safe to add. The feature is still behindOPENCODE_ENABLE_AUTOMATE_TOOLwith no user-visible entry point, so impact is limited to dev/QA installs.modelbecomes required onAutomationDefinition— any pre-PR5.5 caller that constructs a definition without it will fail type-check and (post-migration) the runtime parse.stop.kind === "condition"now returns 422 instead of being stored. The SDKAutomationStopunion still includes the condition variant for future evaluator support.How To Verify
Screenshots or Recordings
N/A — backend-only PR. Feature is still gated behind
OPENCODE_ENABLE_AUTOMATE_TOOLand has no user-visible entry point until PR6 / PR7.Checklist
bug,enhancement,task,documentation. Type labels are author-added; the labeler bot does NOT assign them. Add the label in the GitHub UI, then tick this.app,ui,platform,harness,ci. The labeler bot assigns these on PR open based on changed paths. Confirm the bot's choice (or override if wrong), then tick this.P0,P1,P2,P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.Pending,Approved by @<reviewer>, orNot required: <reason>(default isPending; "not required" is restricted to bot-authored low-risk PRs).dev, and my PR title and commit messages use Conventional Commits in English.Summary by CodeRabbit
New Features
Bug Fixes / Validation
Chores