Skip to content

feat(automation): close PR1-5 backend gaps before frontend slice#1045

Merged
Astro-Han merged 13 commits into
devfrom
claude/pr5p5-automation-gaps
Jun 2, 2026
Merged

feat(automation): close PR1-5 backend gaps before frontend slice#1045
Astro-Han merged 13 commits into
devfrom
claude/pr5p5-automation-gaps

Conversation

@Astro-Han
Copy link
Copy Markdown
Owner

@Astro-Han Astro-Han commented Jun 1, 2026

Summary

Closes backend gaps left after PR1–5 so the PR6/PR7 frontend slice can build on a complete contract. Adds required model (and optional variant) on every AutomationDefinition, rejects stop.kind === "condition" with a structured 422 detail, persists derived scheduling fields (nextFireAt, nextFires, failureStreak) at create / update / after every terminal run, and extends the automate tool 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:

  • Runs depended on the runtime model fallback chain, which changes across restarts — automation definitions could silently target a different model after a process bounce.
  • stop.kind === "condition" was accepted at create time but canScheduleRecurring() returned false, so the UI could surface schedules that would never fire.
  • nextFireAt / nextFires / failureStreak were never persisted, so global-sync had nothing to render after a run completed.
  • The automate tool 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

Human Review Status

Approved by @Astro-Han

Review Focus

  • model / variant validation contract — create + update routes call ProviderTransform.variants(model) and return 422 invalid_automation with structured { field, message } details. Invariant: invalid model/variant never reaches the runner.
  • Stop condition rejection — schema layer still accepts { 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 the automate tool error path.
  • Derived field maintenance + self-loop guardrecordRunOutcome re-publishes automation.definition.updated after every terminal run; scheduler uses a selfPublishedDefinitionUpdates set keyed by id:revision to skip its own echoes. Check the race window and the ConflictError retry loop.
  • automate tool schema — picks up model / variant; description points the LLM at supported stop kinds; tool init now takes Provider.Interface so model existence is checked at create time.

Risk Notes

  • Migration drops pre-release rows. 20260601100000_automation_model_required deletes existing automation_definition / automation_run rows so the new NOT NULL model column is safe to add. The feature is still behind OPENCODE_ENABLE_AUTOMATE_TOOL with no user-visible entry point, so impact is limited to dev/QA installs.
  • Breaking type change. model becomes required on AutomationDefinition — any pre-PR5.5 caller that constructs a definition without it will fail type-check and (post-migration) the runtime parse.
  • Behavior change, not a type break: stop.kind === "condition" now returns 422 instead of being stored. The SDK AutomationStop union still includes the condition variant for future evaluator support.
  • Platform impact: none — backend-only, no packaging, signing, updater, or shell surface touched.
  • Skipped conditional checklist items (see below):
    • "manual UI / copy check" — no visible UI in this PR (feature still gated, sidebar/form land in PR6/PR7).
    • "macOS and Windows impact" — not touched; Windows-advisory CI jobs that ran are unrelated unit suites.

How To Verify

HEAD: 3af5bae6c1

bun test packages/opencode/test/server/automation-routes.test.ts
        + packages/opencode/test/server/automation-runner.test.ts
        + packages/opencode/test/server/automation-scheduler.test.ts
        + packages/opencode/test/server/automation-event-fixtures.test.ts
        + packages/opencode/test/tool/automate.test.ts
        + packages/opencode/test/automation/cron.test.ts
        → 129 pass / 0 fail
bun test packages/opencode/test/session/processor-effect.test.ts
        → 35 pass / 0 fail (runner change did not disturb session loop)
bun run --cwd packages/sdk/js build
        → SDK regen produces no diff vs the manually edited types.gen.ts (verified against 14fa9982aa; 3af5bae6c1 only touches test files, no schema diff)

CI on 3af5bae6c1: 28 SUCCESS / 0 FAILURE / 4 IN_PROGRESS.
  SUCCESS: ci/typecheck, ci/lint, ci/check, ci/frontend-architecture, ci/actionlint,
           ci/unit-app, ci/unit-ui, ci/unit-desktop, ci/unit-opencode,
           codeql/analyze-js-ts, dependency-review, desktop-smoke/smoke-macos-arm64,
           dev-dep-audit, e2e-artifacts, pr-triage (incl. unit results app/ui/desktop/opencode),
           windows-advisory/unit-windows-app, windows-advisory/unit-windows-desktop.
  IN_PROGRESS: CodeQL meta, plus 3 Windows-advisory shards
               (unit-windows-opencode-config-project / -session / -server-tools).
  Windows-advisory is non-blocking; the only prior Windows failure
  (unit-windows-opencode-server-tools on 14fa9982aa) was unrelated to
  automation code and that shard has restarted on 3af5bae6c1.

Screenshots or Recordings

N/A — backend-only PR. Feature is still gated behind OPENCODE_ENABLE_AUTOMATE_TOOL and has no user-visible entry point until PR6 / PR7.

Checklist

  • Type label — this PR carries exactly one of 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.
  • Routing labels — this PR carries at least one of 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.
  • Priority label — this PR carries exactly one of P0, P1, P2, P3. The priority-triage bot suggests one on PR open. Confirm or override, then tick this.
  • Human Review Status above is set to Pending, Approved by @<reviewer>, or Not required: <reason> (default is Pending; "not required" is restricted to bot-authored low-risk PRs).
  • I linked the related issue, or stated in Summary why there is no issue.
  • I described the review focus and any meaningful risks.
  • I replaced the example block in How To Verify with the real verification steps and the key result for each.
  • I did not introduce unrelated refactors, dependencies, generated files, or file changes beyond the stated scope.
  • (conditional) I manually checked visible UI or copy changes when needed, with screenshots or recordings. Leave unticked only if no visible UI or copy changed.
  • (conditional) I considered macOS and Windows impact for platform, packaging, updater, signing, paths, shell, or permissions changes. Leave unticked only if no platform/packaging surface was touched.
  • (conditional) I called out docs, release notes, dependencies, permissions, credentials, deletion behavior, generated content, or local file changes when relevant. Leave unticked only if none of those surfaces was touched.
  • I reviewed the final diff for unrelated changes and suspicious dependency changes.
  • I am targeting dev, and my PR title and commit messages use Conventional Commits in English.

Summary by CodeRabbit

  • New Features

    • Automations now require a model (provider+model ID) and optionally a variant; scheduler and runner use this selection.
    • Recurring automations compute and expose concrete nextFireAt/nextFires.
    • Stronger cron validation to reject unreachable schedules.
  • Bug Fixes / Validation

    • Creation/update API now validates model/variant and returns structured 422 errors for invalid selections.
    • Sending variant: null clears a previously set variant.
  • Chores

    • Migration cleared legacy persisted automations without a model.

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
@Astro-Han Astro-Han added the enhancement New feature or request label Jun 1, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jun 1, 2026

Review Change Stack

📝 Walkthrough

Walkthrough

This 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.

Changes

Automation Model Support & Scheduling

Layer / File(s) Summary
Cron Utilities & Database Cleanup
packages/opencode/migration/.../migration.sql, packages/opencode/src/automation/cron.ts, packages/opencode/test/automation/cron.test.ts
Migration deletes existing automation data to support required model field. Cron module parses 5-field expressions into numeric sets (minute/hour/day/month/weekday), validates ranges and step syntax, computes reachability of day/month combos, and provides cronMatches for DateTime membership.
Derived Scheduling Computation
packages/opencode/src/automation/derived.ts, packages/opencode/src/automation/fixtures.ts
computeDerivedFields generates nextFireAt and a bounded nextFires preview by parsing cron expressions (with minute-by-minute cursor advancement) or interval increments; early exits for paused automations or condition-based stops. Fixtures updated with concrete scheduled timestamps instead of null/empty.
Automation Model Schema & Persistence
packages/opencode/src/automation/index.ts
Introduces Automation.Model schema pairing providerID/modelID. Extends CreateInput/UpdateInput/Definition with model (required) and variant (optional/nullable). Updates field allowlists, enforces rejection of recurring automations with stop.kind === "condition", persists model/variant, and recomputes derived scheduling on create/update. recordRunOutcome rewritten to be selective and conflict-tolerant.
Model/Variant Validation & API Integration
packages/opencode/src/automation/validation.ts, packages/opencode/src/server/instance/automation.ts, packages/opencode/src/tool/automate.ts
validateModelAndVariantWith queries Provider for model availability and returns field-level validation errors (model_not_found, model_lookup_failed, invalid_variant_for_model). Routes POST/PUT and the automate tool validate model/variant and return 422 validation responses when invalid.
Scheduler Refactoring & Runner Integration
packages/opencode/src/automation/scheduler.ts, packages/opencode/src/automation/runner.ts
Scheduler imports cron utilities from the dedicated ./cron module and removes local cron helpers. RunUpdated handler records run outcomes, attempts derived-field refreshes, publishes refreshed definitions while avoiding self-published loops, and logs publication errors. Runner includes definition.model and optional definition.variant when invoking session prompts.
Comprehensive Test Coverage & Fixtures
packages/opencode/test/*, packages/opencode/test/fake/provider.ts, packages/sdk/js/src/v2/event-types.test-d.ts, .github/workflows/windows-advisory.yml
Test suites add fakeAutomationProvider() and a shared fixtureModel, update automation/tool/scheduler tests to include model/variant wiring, add cron validation and model/variant validation tests, update CI shard commands to run automation tests, and include model in event-type fixtures.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related PRs

  • Astro-Han/pawwork#1004: Related changes to session prompt execution/context wiring that interact with runner prompt parameters.
  • Astro-Han/pawwork#960: Baseline automation contract and lifecycle logic this PR extends with model/variant and scheduling derivation.
  • Astro-Han/pawwork#998: Prior Scheduler cron timing work related to the cron/next-fire semantics expanded here.

Suggested labels

windows

"🐰 I hopped through cron and schema lines,
Picked a model, matched the signs,
Next fires counted, fixtures set,
Provider checks — no tear or sweat.
Hooray — automations now align!"

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title 'feat(automation): close PR1-5 backend gaps before frontend slice' clearly and specifically summarizes the main change: completing automation backend work needed for frontend development.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description check ✅ Passed The pull request description comprehensively follows the required template, including all major sections: Summary, Why, Related Issue, Human Review Status, Review Focus, Risk Notes, How To Verify, Screenshots or Recordings, and Checklist.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/pr5p5-automation-gaps

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@github-actions github-actions Bot added harness Model harness, prompts, tool descriptions, and session mechanics P2 Medium priority labels Jun 1, 2026
Copy link
Copy Markdown

@github-actions github-actions Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested priority: P2 (includes non-doc, non-test paths outside the low-risk bucket).

P1/P0 are reserved for maintainer confirmation. Please relabel manually if this is a release blocker, security issue, data-loss risk, or updater/runtime failure.

@Astro-Han Astro-Han added the P1 High priority label Jun 1, 2026
Copy link
Copy Markdown

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread packages/opencode/src/automation/derived.ts
Comment thread packages/opencode/src/automation/index.ts Outdated
Comment thread packages/opencode/src/automation/index.ts
Comment thread packages/opencode/src/server/instance/automation.ts
@github-actions github-actions Bot removed the P2 Medium priority label Jun 1, 2026
Astro-Han added 2 commits June 1, 2026 17:44
…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.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (3)
packages/opencode/test/fake/provider.ts (1)

30-40: ⚡ Quick win

Wrap these fake provider methods with Effect.fn(...) or build this helper from ProviderTest.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 and Effect.fnUntraced for 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 win

Use testEffect(...) for this Effect-driven suite.

These cases run validateModelAndVariantWith(...) via Effect.runPromise, so they bypass the repo’s standard Effect test harness. Please move this file onto testEffect(...) with the test body inside Effect.gen(...).

As per coding guidelines "Use testEffect(...) from test/lib/effect.ts for 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 win

Reuse 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

📥 Commits

Reviewing files that changed from the base of the PR and between 3ade93a and ac04734.

⛔ Files ignored due to path filters (1)
  • packages/sdk/js/src/v2/gen/types.gen.ts is excluded by !**/gen/**
📒 Files selected for processing (17)
  • packages/opencode/migration/20260601100000_automation_model_required/migration.sql
  • packages/opencode/src/automation/cron.ts
  • packages/opencode/src/automation/derived.ts
  • packages/opencode/src/automation/fixtures.ts
  • packages/opencode/src/automation/index.ts
  • packages/opencode/src/automation/runner.ts
  • packages/opencode/src/automation/scheduler.ts
  • packages/opencode/src/automation/validation.ts
  • packages/opencode/src/server/instance/automation.ts
  • packages/opencode/src/tool/automate.ts
  • packages/opencode/test/automation/validation.test.ts
  • packages/opencode/test/fake/provider.ts
  • packages/opencode/test/server/automation-routes.test.ts
  • packages/opencode/test/server/automation-runner.test.ts
  • packages/opencode/test/server/automation-scheduler.test.ts
  • packages/opencode/test/tool/automate.test.ts
  • packages/sdk/js/src/v2/event-types.test-d.ts

Comment thread packages/opencode/test/server/automation-routes.test.ts Outdated
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.
@github-actions github-actions Bot added the ci Continuous integration / GitHub Actions label Jun 1, 2026
Astro-Han added 6 commits June 1, 2026 21:23
…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 }.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
packages/opencode/test/automation/validation.test.ts (1)

1-7: ⚡ Quick win

Use a file-local const it = testEffect(...) in packages/opencode/test/automation/validation.test.ts instead of importing the shared { it } export.
This file imports it from ../lib/effect, but the repo test guideline expects const it = testEffect(...) near the top of each test file for it.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

📥 Commits

Reviewing files that changed from the base of the PR and between ac04734 and 14fa998.

📒 Files selected for processing (12)
  • .github/workflows/windows-advisory.yml
  • packages/opencode/src/automation/cron.ts
  • packages/opencode/src/automation/index.ts
  • packages/opencode/src/automation/scheduler.ts
  • packages/opencode/src/tool/automate.ts
  • packages/opencode/test/automation/cron.test.ts
  • packages/opencode/test/automation/validation.test.ts
  • packages/opencode/test/fake/provider.ts
  • packages/opencode/test/github/ci-workflow.test.ts
  • packages/opencode/test/server/automation-routes.test.ts
  • packages/opencode/test/server/automation-scheduler.test.ts
  • packages/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

Comment thread packages/opencode/test/server/automation-routes.test.ts
Comment thread packages/opencode/test/server/automation-scheduler.test.ts
Astro-Han added 3 commits June 1, 2026 23:53
…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.
@Astro-Han Astro-Han merged commit 1ad8e16 into dev Jun 2, 2026
43 of 45 checks passed
@Astro-Han Astro-Han deleted the claude/pr5p5-automation-gaps branch June 2, 2026 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ci Continuous integration / GitHub Actions enhancement New feature or request harness Model harness, prompts, tool descriptions, and session mechanics P1 High priority

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant