Routines: standing instructions a Bot runs on a schedule - #278
Conversation
main took idx 20 while this branch was built, so 0020_routines becomes 0021_routines: regenerated with drizzle-kit (SQL byte-identical to the original), snapshot chained off main's 0020, journal appended. The last_run_at column comment now says what the sweep actually writes there — the stamp advanced past, fired or drained — rather than implying a run history the routine_runs table owns.
main inserted attentionStore into createApp's positional signature before pageFrames; the endpoint test's hand-built tuple put the runner one slot early, so the route never mounted and all seven cases 404ed.
…ation Reverts e8aa344 (#255) code on guido/routines only: the API routes, store, view, schema, app wiring, sidebar entry and /attention page go; the 0020 migration, journal entry and snapshot stay, because the local database has already applied them and an unused table is cheaper than a broken chain. The routine endpoint test loses the positional attentionStore slot it had gained for main's signature.
…inish Four lifecycle fixes from review. The 15-minute floor now walks the expression's cycle instead of sampling the next two occurrences, so a cron like '45,55 8 * * *' is refused whenever it is created rather than accepted at 08:50 and wedged after its first firing; a refusal that still surfaces at sweep time switches the routine off with its reason on a run row instead of silently burning a due slot every pass forever. The enabled cap counts and writes under a per-owner advisory lock, so two racing creates at nineteen admit exactly one. A stale clock is drained current in one pass by computing the next occurrence from now while the CAS still compares the old stamp. And failOpenRuns — whose only caller was unreachable at the shipped five-minute cadence, and which would have closed a genuinely in-flight run as failed — is replaced by an age-scoped reaper that closes abandoned rows as skipped, which also mops up runs stranded open by a server dying mid-turn.
The attention revert kept migration 0020 while removing the schema declaration, which left drizzle's latest snapshot claiming a table the schema files no longer knew: the next unrelated db:generate silently emitted DROP TABLE attention_resolutions CASCADE inside whatever migration somebody happened to be writing. Migration 0022 makes that drop explicit and reviewable — the feature never shipped in a release — and restores the invariant that the latest snapshot matches the schema. Migration 0023 gives routines the (owner_user_id, enabled) index that listFor, countEnabled, the owner-scoped writes and the users cascade were all sequential-scanning without.
start.sh leaves an answering server alone, and that philosophy kept a server started before WORKER_SHARED_SECRET existed: the worker then got 401 for every handoff and routines never fired, with nothing at start time saying why. Now the script probes /internal/routines/run with this run's secret — 400 means the secret was accepted and only the empty body refused, 401 or 404 means the server cannot take handoffs and is restarted into this run's environment.
# Conflicts: # server/src/app.ts # server/src/attention/view.ts # server/src/db/schema/attention.ts # server/tests/attention-view.test.ts
davidmckayv
left a comment
davidmckayv
left a comment
There was a problem hiding this comment.
Reviewed the full branch closely across three angles (governed-path/security, horizontal scale, and the runtime-copy plus template-fit), reading the actual code rather than the description. This is strong work. Approving.
Governed path and worker auth: sound
- The internal handoff is authenticated constant-time (timingSafeEqual via sameToken), fail-closed when WORKER_SHARED_SECRET is unset (byte-identical 401), and the refusal is itself audited as routines.dispatch_refused, so a stale secret cannot silently stop every routine.
- A firing runs under the creator's real resolved identity through the same grant -> CEL policy -> audit -> act tool boundary as an interactive turn. Enforcement lives in the tool closure baked into the agent, so the headless path cannot skip it. No escalation, and firings are audited.
- No identity spoofing: owner/agent/channel are read server-side from the stored routine row; the handoff body carries only routineRunId, and an ownerUserId in the model's args is never read. Error columns are code-point-capped with no secret material.
Horizontal scale: multi-worker-safe
- Exactly-once firing rides three independent Postgres guarantees: work_items (kind,key) PK with onConflictDoNothing keyed on routineId:minute, a CAS advance on next_run_at, and SELECT ... FOR UPDATE SKIP LOCKED claims. No in-process lease anywhere in the firing path, and there are real race tests.
- The sub-floor schedule wedge is fixed by cycle-walking from a fixed leap-year start (tested at multiple clocks), and a sweep-time refusal disables the routine visibly instead of wedging.
- The reaper closes only status IS NULL rows older than 2x the turn timeout as skipped, so it never closes live work and keeps infra deaths out of the fatigue streak. Stale-clock catch-up happens in one pass. The 20-enabled cap is counted and written under a per-owner advisory lock with a real concurrency test. Migrations 0021/0023 are lock-safe on the new/empty table and correctly chained.
Template-fit: appropriate
The builtin in-process catalogue entry is a genuinely generic seam, not a Routines special-case: it reuses the existing VendorTransport interface and the closed TransportKind union, so a forker adds a second in-process capability the same way (a new kind string plus a module exporting listTools/callTool). The worker package already existed; the Helm CronJob mirrors the culler pattern; routines.enabled defaults off behind a required secret. No new product surface.
One follow-up worth tracking (not blocking)
run-turn.ts hand-copies ~660 lines of the runtime's private turn engine. I diffed it line-for-line against 1.69.0: it is faithful, the dep is exactly pinned (not a caret range), and it is covered by the 882-line test. The live risk is upgrade-time drift: a renamed runtime method fails loudly at compile, but a renamed field would drift silently. The right long-term fix is the one already noted here, a headless-turn API exported from @copilotkit/runtime; until then, treat a runtime bump as requiring a re-read of this file against the package. The chunk-collector fallback refutation is correct: those chunks come from the durably-acknowledged event stream, so the fallback does not violate the report-only-what-persisted invariant.
The other deferred items listed in the description (shared createRoutineSweep, constructed transport registry, createApp options object, the sweep N+1s, a scheduled_for column) are all reasonable as follow-ups.
On the Attention inbox removal
Acknowledged and accepted. The revert is mechanically clean and complete (no dangling references), and migration 0022 is correctly chained; it also defuses the silent DROP that a schema-only deletion would have armed in the next unrelated migration. Good call to make it explicit.
LGTM.
2 participants
What this is
A Bot can now be asked to do something on a schedule — "every weekday at nine, post the standup notes here" — and the routine runs under its creator's grants, replying into the channel as an ordinary Bot message. The feature spans the server (store, cron scheduling, sweep, headless turn runner, internal handoff endpoint), a worker process that fires due routines, the app's Routines page, tool surface via the plugin catalogue (Routines is the first builtin, in-process catalogue entry), Helm chart wiring, and
start.sh.Two tables arrive via migration
0021. A new process fires due routines:scripts/start.shruns it locally, the Helm chart schedules it withroutines.enabled, andWORKER_SHARED_SECRETis the credential it presents.Deliberate and discussed: the revert of e8aa344 rides in this branch, plus migration
0022droppingattention_resolutionsexplicitly (the feature never shipped in a release; the alternative was a snapshot/schema divergence that armed a silentDROP TABLEinside the next unrelated migration). If the team wants Attention back, that conversation should happen on this PR.Review
A high-effort multi-angle review ran over the full branch diff (8 finder angles, adversarial verification of every correctness candidate). Confirmed and fixed here:
45,55 8 * * *created at 08:50 was accepted, fired once, then wedged forever onScheduleRefusedErrorinsideadvanceNextRun(reproduced by execution). Validation now walks the expression's cycle, and a sweep-time refusal switches the routine off visibly instead of wedging.failOpenRunswas unreachable at shipped Helm cadence (5-min sweep vs 10-min grace vs 5 attempts), so dispatch failures leakedstatus IS NULLrun rows rendering as "Running…" forever — and where reachable it would have closed a genuinely in-flight run as failed. Replaced with an age-scoped reaper that closes abandoned rows asskipped, which also reaps runs stranded by a server dying mid-turn and keeps infra failures out of the fatigue streak.routineshad no owner index;0023adds(owner_user_id, enabled).start.shkept a surviving pre-routines server that could never accept worker handoffs (401 on every dispatch, invisibly). It now probes the handoff route and restarts on 401/404.relative-timemodule in the app,RoutineToolsasPick<RoutineStore, …>, the store'sMAX_RUN_ERRORreused, one dead compat branch removed, timezone validation memoized off the sweep hot path.One reviewer claim was refuted and deliberately not applied: the run-turn chunk-collector fallback is load-bearing (chunks are durably acknowledged before the fallback can run; real empty-diff-with-chunks paths exist).
Known follow-ups (found in review, out of scope here)
run-turn.tshand-copies the runtime's private turn engine (~660 lines pinned to 1.69.0 dist paths) — the real fix is exporting a headless-turn API from@copilotkit/runtime.fire-routines.tsduplicate the dispatch/owner/purge wiring; extract a sharedcreateRoutineSweep.actorId?) — wants a constructed registry and a requiredCallContext.createAppis at 13 trailing positional optionals; convert to an options object.advanceNextRunre-SELECT, per-itemroutineForFiring),describeWrittenfull-list read, truncation-helper and MCP result-cap consolidation.routine_runswould benefit from ascheduled_forcolumn (identity-scoped closing) and a partial index on open runs.Verification
Format, lint, typecheck (server/app/worker) all green; 1893 tests pass, 0 fail; full build succeeds; migrations
0022/0023applied cleanly to a dev database holding0000–0021and re-run as a no-op.