feat(deployments): 使用 River 实现定时调度 - #219
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change adds cron-based scheduled deployments with River-backed execution, persistent schedule revisions and cursors, transactional webhook delivery, typed trigger responses, startup wiring, and integration coverage. ChangesScheduled deployment scheduling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant API
participant Database
participant River
participant Scheduler
participant WebhookOutbox
API->>Database: persist schedule and enqueue occurrence
River->>Scheduler: execute scheduled occurrence
Scheduler->>Database: validate state and apply run
Database->>WebhookOutbox: enqueue lifecycle events
Scheduler->>River: advance or retry occurrence
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 64c21deb1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
main.go (1)
56-62: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winDocument the
cmd/migraterequirement whenAutoMigrateis disabled.When
database.auto_migrateis off,cmd/migrate upstill runs Goose migrations anddeployments.MigrateRiverbeforeoma-serverstarts. If an operator starts the server withauto_migrate: falsebefore applying migrations,deploymentScheduler.Start(ctx)fails with missing River tables and only reportsstart deployment scheduler. Add this ordering to the runbook or run both migration steps unconditionally before starting the scheduler.🤖 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 `@main.go` around lines 56 - 62, The AutoMigrate-disabled startup path must ensure both Goose and River migrations run before deploymentScheduler.Start(ctx). Either document in the runbook that operators must run cmd/migrate up before starting oma-server, or move database.Migrate and deployments.MigrateRiver outside the cfg.Database.AutoMigrate guard so both execute unconditionally.
🧹 Nitpick comments (22)
tests/deployments_api_test.go (1)
720-768: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the success subtest before the failure subtests, or move the failure subtests after it.
The added block places
failure auto pause rolls back...,failure scheduled root agent archive..., andfailure agent archive rolls back...beforeagent API archives deployments with webhook outbox. The coding guideline requires failure scenarios first and success scenarios after. Reorder the new subtests so allfailure ...subtests precede this success subtest.As per coding guidelines: "Order tests with failure scenarios before success scenarios".
🤖 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 `@tests/deployments_api_test.go` around lines 720 - 768, Reorder the subtests in the test suite so the failure scenarios “failure auto pause rolls back...”, “failure scheduled root agent archive...”, and “failure agent archive rolls back...” appear before the successful “agent API archives deployments with webhook outbox” subtest. Keep each subtest’s implementation unchanged.Source: Coding guidelines
docs/design/be/deployments-api-contract.md (1)
88-88: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReference the source of the 14 paused-reason error types.
The count
14 类 paused-reason errorwill drift when the code list changes. Name the Go constant or slice that holds the list so readers can verify it.🤖 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 `@docs/design/be/deployments-api-contract.md` at line 88, Update the deployment API contract documentation to reference the Go constant or slice that defines the 14 public paused-reason error types instead of relying only on the numeric count. Keep the existing behavior description unchanged, and use the exact source symbol name so readers can verify the list.web/src/features/managed-agents/ManagedAgentsPage.resources.suite.tsx (1)
2187-2187: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueAlign the deployment schedule with the real response shape.
internal/deployments/cron_test.gousesexpression, while this fixture usesQuickstartDeploymentInput’scron_expression;DeploymentApiResponse.schedulehas no typed shape. Use the response format or remove this fixture assignment if it is not exercised.🤖 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 `@web/src/features/managed-agents/ManagedAgentsPage.resources.suite.tsx` at line 2187, Update the fixture assignment near api.resources.deployments in the managed agents resources suite to match the real DeploymentApiResponse schedule shape by using the response’s expression field instead of QuickstartDeploymentInput’s cron_expression; if this schedule is not exercised, remove the assignment.internal/webhooks/enqueuer_test.go (2)
16-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove this success-scenario test after the failure-scenario test.
TestPrepareDeliveryEventPreservesOutboxDataasserts the success path. The existing test that usesfailingEnqueueStoreat Line 62 asserts a failure path. Place the failure-scenario test first.As per coding guidelines: "Order tests with failure scenarios before success scenarios".
🤖 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/webhooks/enqueuer_test.go` around lines 16 - 37, Reorder the tests in the relevant test file so the existing failure-scenario test using failingEnqueueStore appears before TestPrepareDeliveryEventPreservesOutboxData. Do not change either test’s implementation or assertions.Source: Coding guidelines
22-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtend the assertions to cover the fields the test name claims.
The test name states that outbox data is preserved. The assertions cover
FallbackEnabled,EventType,CreatedAt, andData.IDonly.event.ID,event.Data.Type,event.Data.WorkspaceID, andevent.Data.OrganizationIDare the fields that the scheduler's transactional outbox writes depend on, and they are not checked.Assert those fields as well, and split the combined condition so a failure names the field.
💚 Proposed additional assertions
- if !deliveryEvent.FallbackEnabled || deliveryEvent.EventType != "deployment_run.started" || - event.CreatedAt != "2026-08-07T01:02:03Z" || event.Data.ID != "drun_test" { - t.Fatalf("PrepareDeliveryEvent() = %+v, event = %+v", deliveryEvent, event) - } + if !deliveryEvent.FallbackEnabled { + t.Errorf("FallbackEnabled = false, want true") + } + if deliveryEvent.EventType != "deployment_run.started" { + t.Errorf("EventType = %q, want %q", deliveryEvent.EventType, "deployment_run.started") + } + if event.CreatedAt != "2026-08-07T01:02:03Z" { + t.Errorf("CreatedAt = %q, want %q", event.CreatedAt, "2026-08-07T01:02:03Z") + } + if !strings.HasPrefix(event.ID, "wevt_") { + t.Errorf("ID = %q, want prefix %q", event.ID, "wevt_") + } + if event.Data.ID != "drun_test" || event.Data.Type != "deployment_run.started" || + event.Data.WorkspaceID != "workspace_test" || event.Data.OrganizationID != "org-uuid" { + t.Errorf("Data = %+v", event.Data) + }Add
"strings"to the imports.🤖 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/webhooks/enqueuer_test.go` around lines 22 - 36, Extend the assertions in the PrepareDeliveryEvent test to verify event.ID, event.Data.Type, event.Data.WorkspaceID, and event.Data.OrganizationID alongside the existing fields. Split the combined condition into field-specific assertions so failures identify the mismatched field, and add the strings import only if needed for the expected workspace or organization value checks.internal/deployments/scheduler.go (3)
217-220: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winA permanently invalid stored schedule blocks the deployment forever.
nextAfterScheduledreturns an error for a schedule that cannot be parsed.Workreturns that error, so River retries the job until it is discarded. The schedule cursor never advances, andreconcileskips the same deployment on every pass becausejitteredTriggerAtalso fails. The deployment then staysactivewith a stalenext_scheduled_atand no operator-visible run.Record a failure run and auto-pause the deployment for this case, in the same way as other non-retryable preparation failures.
🤖 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/deployments/scheduler.go` around lines 217 - 220, Update the invalid-schedule handling in Work around nextAfterScheduled so an unparseable stored schedule is treated as a non-retryable preparation failure: record a failure run and automatically pause the deployment, matching the existing handling for other non-retryable preparation failures. Ensure Work does not return the parse error for River retry, and keep reconcile from repeatedly skipping the unchanged active deployment.
263-288: 🩺 Stability & Availability | 🔵 Trivial | 🏗️ Heavy liftThe next occurrence is enqueued only by the 30-second reconcile loop.
After
ApplyScheduledOccurrenceadvancesnext_scheduled_at, no job is inserted for the new occurrence. The next job appears on the following reconcile tick. This adds up toscheduleReconcileIntervalof latency to every occurrence, and it makes minute-level cron schedules imprecise.Insert the follow-up job directly after a successful apply, and keep the reconcile loop as the recovery path.
♻️ Sketch of the follow-up insert
if err != nil { return err } + if nextScheduledAt != nil { + deployment.ScheduleRevision = args.ScheduleRevision + deployment.NextScheduledAt = nextScheduledAt + if err := w.enqueueNext(ctx, deployment); err != nil { + w.logger.ErrorContext(ctx, "enqueue next deployment occurrence", + "deployment_id", deployment.ExternalID, "error", err) + } + } return nil
enqueueNextneeds access to the River client, so pass the client or a small inserter interface intoscheduledDeploymentWorker.🤖 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/deployments/scheduler.go` around lines 263 - 288, After a successful ApplyScheduledOccurrence call in the scheduledDeploymentWorker flow, immediately enqueue the newly computed nextScheduledAt occurrence instead of waiting for reconciliation. Update scheduledDeploymentWorker or its dependencies to provide enqueueNext with the River client or a minimal inserter interface, while preserving the reconcile loop as the recovery path and existing error handling.
82-94: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low value
Startblocks application startup on a full reconcile.
backfillNextScheduledAtandreconcileiterate every scheduled deployment and perform one database write or job insert per deployment. Both run synchronously beforeclient.Start. With a large number of scheduled deployments, this delays process startup, and a single insert error aborts startup througherrors.Join.Consider running the initial reconcile in the background loop, and treating per-deployment errors as logged failures instead of startup failures.
🤖 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/deployments/scheduler.go` around lines 82 - 94, The DeploymentScheduler.Start method currently blocks startup and propagates per-deployment failures from backfillNextScheduledAt and reconcile. Move the initial backfill/reconcile work into the background reconciliation flow so client.Start executes without waiting, and handle individual deployment errors by logging them while allowing the loop to continue rather than returning them as startup errors.main.go (1)
136-142: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winThe stop timeout is not aligned with the River client timeouts.
Stopgets 10 seconds.internal/deployments/scheduler.gosetsSoftStopTimeout: 10 * time.SecondandJobTimeout: 2 * time.Minute. The soft-stop phase alone consumes the whole budget, so the hard-stop phase never gets time, andStopreports a timeout error on every shutdown that has an in-flight job.Set the shutdown context longer than
SoftStopTimeout, for example 20 seconds.In-flight work is safe because
ApplyScheduledOccurrenceruns in one transaction and the job is retried, but the recurring error log is misleading.♻️ Proposed change
defer func() { - stopCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + stopCtx, cancel := context.WithTimeout(context.Background(), 20*time.Second) defer cancel() if err := deploymentScheduler.Stop(stopCtx); err != nil { logger.Error("stop deployment scheduler", "error", 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 `@main.go` around lines 136 - 142, Increase the shutdown context timeout in the deferred cleanup around deploymentScheduler.Stop from 10 seconds to a value longer than the scheduler’s SoftStopTimeout, such as 20 seconds, so the hard-stop phase can complete without recurring timeout errors.internal/deployments/execution.go (1)
31-43: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPreparation errors are reported with a misleading error type.
sessionEventsFromInitialEvents,sessionResourcesFromDeployment, andhttpapi.MarshalRawreturn plain errors. Ininternal/deployments/scheduler.goat Line 257, every non-retryable error fromprepareDeploymentRunis recorded assession_resource_not_found_error. A malformedInitialEventspayload or a marshal failure then produces a wrong error type in the run record, andshouldAutoPausepauses the deployment with that wrong reason.Return a classified error from
prepareDeploymentRun, or wrap each failure source with its own error type before it reachesrecordFailure.🤖 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/deployments/execution.go` around lines 31 - 43, The prepareDeploymentRun error paths for sessionEventsFromInitialEvents, sessionResourcesFromDeployment, and httpapi.MarshalRaw currently return unclassified errors that scheduler.go records as session_resource_not_found_error. Update prepareDeploymentRun to return a classified error, or wrap each failure with an appropriate distinct error type, so recordFailure and shouldAutoPause receive the correct failure reason for malformed events, resource preparation, and marshal failures.internal/deployments/handler.go (4)
620-638: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winCompute the next occurrence from the row that the transaction unpauses.
Line 620 reads the deployment outside the transaction. Line 625 derives
nextfrom that stale copy. A concurrent update toschedulebetween the read andUnpauseDeploymentTxmakes the persistednext_scheduled_atinconsistent with the stored schedule, and the enqueued River job then targets the wrong occurrence. Move the read and the calculation inside the transaction callback, or haveUnpauseDeploymentTxderive the next occurrence from the locked row.🤖 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/deployments/handler.go` around lines 620 - 638, Move the deployment read and nextScheduledAt calculation into the transaction callback used by the unpause flow, ensuring they operate on the transaction’s current or locked row before calling UnpauseDeploymentTx and enqueueScheduledOccurrenceTx. Keep error logging and HTTP error responses consistent, and ensure the persisted next_scheduled_at and enqueued job derive from the same schedule.
775-862: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSplit
validateRunReferencesinto per-resource validators.The function validates six reference kinds in one body. Its cyclomatic complexity is close to the 30 limit for Go functions, so any later branch pushes it over. Extract
validateAgentReferences,validateSkillReferences,validateVaultReferences, andvalidateResourceReferences, then call them in sequence.As per coding guidelines: "Respect complexity budgets: Go functions must remain at cyclomatic complexity 30 or below".
🤖 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/deployments/handler.go` around lines 775 - 862, Split validateRunReferences into the requested helpers: validateAgentReferences, validateSkillReferences, validateVaultReferences, and validateResourceReferences. Move each corresponding validation block into its helper, preserving existing error classification, ordering, and return behavior, then have validateRunReferences invoke them sequentially and continue only when each succeeds.Source: Coding guidelines
289-297: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the local variable to avoid shadowing the
nextScheduledAtfunction.Line 289 declares a local variable with the same name as the package-level function. The function becomes unreachable for the rest of
create. Rename the variable, for examplenextRunAt.♻️ Proposed rename
- nextScheduledAt, err := nextScheduledAt(schedule, now) + nextRunAt, err := nextScheduledAt(schedule, now) if err != nil { writeBadRequest(w, r, err) return } revision := int64(0) - if nextScheduledAt != nil { + if nextRunAt != nil { revision = 1 }Update the struct field assignment at line 319 as well:
NextScheduledAt: nextRunAt,🤖 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/deployments/handler.go` around lines 289 - 297, Rename the local result variable in create from nextScheduledAt to nextRunAt so it does not shadow the nextScheduledAt function, update its nil check and revision logic, and use nextRunAt in the NextScheduledAt struct field assignment.
822-861: 🚀 Performance & Scalability | 🔵 Trivial | 🏗️ Heavy liftBatch the vault, file, and memory-store lookups.
Lines 828-836 and 843-861 run one database query per reference, in sequence, on the request thread. The deployment resource limit permits up to 500 file resources, so a single
POST /v1/deployments/{id}/runcan issue hundreds of sequential round trips before any work starts. Add batch lookups by external ID for vaults, files, and memory stores, then validate the returned set.🤖 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/deployments/handler.go` around lines 822 - 861, Replace the sequential lookups in the vault and resources validation flow with batch-by-external-ID queries for vaults, files, and memory stores, grouping resource IDs by type before querying. Validate the returned records as a set, preserving missing-reference and archived-record handling through classifyReferenceFailure, and keep the existing invalid JSON behavior unchanged.internal/deployments/scheduler_test.go (1)
1-9: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the worker success path.
This file covers failure classification, auto-pause selection, and failure webhook inputs. It does not cover the successful scheduled execution path, which creates the session, the deployment run, and the outbox rows, and then advances the schedule cursor. The PR description lists this path as a review focus. Do you want me to draft the test?
🤖 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/deployments/scheduler_test.go` around lines 1 - 9, Add a test in the scheduler test suite covering the successful scheduled execution path: verify session, deployment run, and outbox rows are created and the schedule cursor advances. Reuse the existing test fixtures, helpers, and success-path symbols in the scheduler implementation, while preserving the current failure, auto-pause, and webhook tests.internal/db/deployment_mapper_test.go (1)
168-177: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert that
AdvanceScheduleleavesschedule_revisionunchanged.The distinction between
AdvanceScheduleandPauseAfterScheduledRunis that only the pause statement bumpsschedule_revision. If a future edit adds the increment toAdvanceSchedule, every pending River job is invalidated after each successful run and the schedule stalls until the next reconciliation pass. The current fragments do not catch that.Add a negative assertion in the sub-test block below the table, matching the existing "include archived omits archived filter" pattern.
💚 Proposed test
t.Run("include archived omits archived filter", func(t *testing.T) { page.IncludeArchived = true bound := buildDeploymentMapperListPage(yourbatis.DialectPostgres, page) if containsSQL(bound.SQL, "archived_at IS NULL") { t.Fatalf("SQL unexpectedly filters archived deployments: %q", bound.SQL) } }) + + t.Run("advance schedule preserves the schedule revision", func(t *testing.T) { + bound := buildDeploymentMapperAdvanceSchedule(yourbatis.DialectPostgres, advance) + if containsSQL(bound.SQL, "schedule_revision = schedule_revision + 1") { + t.Fatalf("AdvanceSchedule unexpectedly bumps the schedule revision: %q", bound.SQL) + } + })🤖 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/db/deployment_mapper_test.go` around lines 168 - 177, Extend the “advance schedule” test case for buildDeploymentMapperAdvanceSchedule with a negative SQL-fragment assertion, following the existing “include archived omits archived filter” pattern. Verify the generated statement does not contain any schedule_revision increment or update, while preserving the existing positive fragments and argument expectations.internal/db/migrations/00049_schedule_deployments_with_river.sql (1)
13-31: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffPlan the lock impact on
deployment_runs.Static analysis flags several blocking operations on this table:
add constraint ... checkperforms a full table scan and blocks writes.NOT VALIDplus a laterVALIDATE CONSTRAINTavoids that.- Both
create indexstatements block writes.CONCURRENTLYavoids that, but requires-- +goose NO TRANSACTIONbecause goose wraps migrations in a transaction.drop column trigger_contextis metadata-only and fast, but it is irreversible for any context field other thanscheduled_at.If
deployment_runsis small in every deployed environment, the current form is acceptable. State that decision, or split the migration.🤖 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/db/migrations/00049_schedule_deployments_with_river.sql` around lines 13 - 31, Address the lock impact in migration 00049 by either explicitly confirming that deployment_runs is small in every deployed environment and keeping the current operations, or split the migration to use NOT VALID followed by later VALIDATE CONSTRAINT and concurrent index creation with the required goose NO TRANSACTION directive. Preserve the metadata-only trigger_context removal, acknowledging its irreversible behavior.Source: Linters/SAST tools
internal/deployments/cron_test.go (2)
9-72: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrder failure scenarios before success scenarios.
TestNormalizeOptionalScheduleRejectsUnsupportedSyntaxis the failure scenario. Move it aboveTestNextScheduledTimesHandlesLeapDayandTestNextScheduledTimesHandlesDST. The coding guidelines require failure tests first in*_test.gofiles.As per coding guidelines: "Order tests with failure scenarios before success scenarios".
🤖 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/deployments/cron_test.go` around lines 9 - 72, Reorder the test functions so TestNormalizeOptionalScheduleRejectsUnsupportedSyntax appears before the successful nextScheduledTimes tests TestNextScheduledTimesHandlesLeapDay and TestNextScheduledTimesHandlesDST. Keep each test’s implementation unchanged and retain TestNormalizeOptionalScheduleAcceptsSundaySeven with the success scenarios.Source: Coding guidelines
54-65: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for an unsatisfiable cron expression.
0 0 30 2 *parses successfully but never matches a date. Add it to this rejection table once the parser rejects it. See the related comment oninternal/deployments/cron.golines 114-158.💚 Proposed test case
tests := []string{ `{"type":"cron","expression":"`@daily`","timezone":"UTC"}`, `{"type":"cron","expression":"0 0 L * *","timezone":"UTC"}`, `{"type":"cron","expression":"0 0 0 * * *","timezone":"UTC"}`, + `{"type":"cron","expression":"0 0 30 2 *","timezone":"UTC"}`, }🤖 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/deployments/cron_test.go` around lines 54 - 65, Add the unsatisfiable expression `0 0 30 2 *` to the `tests` table in `TestNormalizeOptionalScheduleRejectsUnsupportedSyntax`, preserving the existing assertion that `normalizeOptionalSchedule` returns an error for every listed expression. Ensure the parser and normalization logic reject this expression before enabling the test.internal/db/deployment_mapper.xml (1)
224-233: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winThis query has no supporting index and runs on every reconciliation tick.
The partial index
deployments_pending_schedule_idxadded in migration00049requiresnext_scheduled_at IS NOT NULL. This query requires the opposite, so PostgreSQL falls back to a sequential scan ofdeployments. The scheduler runs the query every 30 seconds across all tenants.Add a matching partial index in the migration.
⚡ Proposed index for `internal/db/migrations/00049_schedule_deployments_with_river.sql`
create index deployments_pending_schedule_idx on deployments (next_scheduled_at) where status = 'active' and archived_at is null and deleted_at is null and next_scheduled_at is not null; +create index deployments_uninitialized_schedule_idx + on deployments (uuid) + where status = 'active' + and archived_at is null + and deleted_at is null + and schedule is not null + and next_scheduled_at is null; +Add the matching
drop index deployments_uninitialized_schedule_idx;to the Down section.🤖 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/db/deployment_mapper.xml` around lines 224 - 233, Add a partial index in migration 00049 matching the ListSchedulesMissingNextScheduledAt predicates, covering active, non-archived, non-deleted deployments with a non-null schedule and null next_scheduled_at; add the corresponding deployments_uninitialized_schedule_idx drop statement to the migration’s Down section.internal/db/webhooks.go (1)
70-98: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache the endpoint lookup per event type.
ListActiveForEventruns once per event inside the loop. The agent-archive cascade ininternal/db/agents.gobuilds onedeployment.archivedevent per archived deployment, so the same query repeats for every deployment while holding the transaction open. This is an N+1 query on a write transaction.
hasEndpointsis also loop-invariant, so the fallback branch can move outside the loop.♻️ Proposed refactor
jobMapper := NewWebhookDeliveryJobMapper(executor) + if !hasEndpoints { + for _, event := range events { + if !event.FallbackEnabled { + continue + } + payload, err := webhookDeliveryJobPayloadJSON(event.EventType, event.Event, "") + if err != nil { + return err + } + if err := jobMapper.Insert(ctx, workspaceUUID, payload); err != nil { + return err + } + } + return nil + } + + endpointsByEventType := make(map[string][]webhookEndpointMapperRow, len(events)) for _, event := range events { - if !hasEndpoints { - if !event.FallbackEnabled { - continue - } - payload, err := webhookDeliveryJobPayloadJSON(event.EventType, event.Event, "") - if err != nil { - return err - } - if err := jobMapper.Insert(ctx, workspaceUUID, payload); err != nil { - return err - } - continue - } - - endpoints, err := endpointMapper.ListActiveForEvent(ctx, workspaceUUID, event.EventType) - if err != nil { - return err + endpoints, cached := endpointsByEventType[event.EventType] + if !cached { + var err error + endpoints, err = endpointMapper.ListActiveForEvent(ctx, workspaceUUID, event.EventType) + if err != nil { + return err + } + endpointsByEventType[event.EventType] = endpoints } for _, endpoint := range endpoints {Adjust the map value type to the actual row type returned by
ListActiveForEvent.🤖 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/db/webhooks.go` around lines 70 - 98, Refactor the event processing loop around ListActiveForEvent and hasEndpoints to cache active endpoints by event type, using the actual row type returned by endpointMapper.ListActiveForEvent for the map values. Reuse cached results for repeated event types so each type is queried once, and move the loop-invariant fallback handling for !hasEndpoints outside the endpoint lookup path while preserving existing payload and insertion behavior.internal/deployments/cron.go (1)
68-72: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAdd an explicit default or make
schedule.timezonerequired.An empty
schedule.timezonepasses validation andtime.LoadLocation("")treats it as UTC, but the API does not document this fallback. Set an explicit UTC option before loading the location, or reject an omitted/blank timezone if the user must always choose a timezone.🤖 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/deployments/cron.go` around lines 68 - 72, Update the timezone validation in the schedule parsing flow around config.Timezone and time.LoadLocation: either assign an explicit UTC default when the trimmed value is blank, or reject blank values as invalid if timezone selection is required. Ensure the chosen behavior is explicit and consistent with the API contract before loading the location.
🤖 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 `@go.mod`:
- Line 84: Upgrade the google.golang.org/grpc dependency from v1.80.0 to at
least v1.82.1 in go.mod, preserving it as an explicit requirement if needed, and
regenerate go.sum so the updated module checksums are recorded.
In `@internal/db/deployment_mapper.xml`:
- Around line 144-148: Update the schedule-change detection in
UpdateDeploymentTx before it calls UpdateByExternalID so ScheduleChanged is true
only when the normalized new schedule differs from the existing normalized
schedule. Reuse the same normalization/serialization semantics used for
persistence, then pass the comparison result into the existing deployment update
flow so schedule_revision is not incremented for equivalent schedules.
In `@internal/db/deployments.go`:
- Around line 319-326: Update SetInitialDeploymentNextScheduledAt to preserve
the rows-affected result from SetInitialNextScheduledAt and return
ErrStaleSchedule when the guarded update affects zero rows, while returning nil
for a successful update and propagating database errors. Ensure the scheduler
handles ErrStaleSchedule by skipping the River enqueue, consistent with the
worker’s existing stale-schedule handling.
In `@internal/db/migrations/00049_schedule_deployments_with_river.sql`:
- Around line 9-21: Before updating deployment_runs, add explicit validation for
existing schedule rows in the migration: fail with a clear error when
trigger_context.scheduled_at is missing or invalid, and when duplicate
deployment_uuid/scheduled_at occurrences would violate the new unique index.
Only perform the backfill and add deployment_runs_scheduled_at_check and
deployment_runs_schedule_occurrence_idx after these validations pass.
In `@internal/deployments/cron.go`:
- Around line 114-158: The cron schedule handling must reject unsatisfiable
expressions instead of persisting or returning zero times. In
internal/deployments/cron.go lines 114-158, update parseDeploymentSchedule to
reject a zero first occurrence, make nextScheduledAt and nextAfterScheduled
return nil when cron.Schedule.Next yields zero, and update jitteredTriggerAt at
lines 165-176 to handle a nil result before dereferencing next. In
internal/deployments/cron_test.go lines 54-65, add the 0 0 30 2 * UTC schedule
to TestNormalizeOptionalScheduleRejectsUnsupportedSyntax’s rejection table.
In `@web/src/features/managed-agents/resources/detail.tsx`:
- Line 680: Guard the trigger context access in the runs table so missing
trigger_context does not throw and preserves the prior fallback behavior.
Restore or reuse triggerLabel for user-facing formatting instead of rendering
raw enum values, unless the intended copy explicitly requires manual and
schedule.
---
Outside diff comments:
In `@main.go`:
- Around line 56-62: The AutoMigrate-disabled startup path must ensure both
Goose and River migrations run before deploymentScheduler.Start(ctx). Either
document in the runbook that operators must run cmd/migrate up before starting
oma-server, or move database.Migrate and deployments.MigrateRiver outside the
cfg.Database.AutoMigrate guard so both execute unconditionally.
---
Nitpick comments:
In `@docs/design/be/deployments-api-contract.md`:
- Line 88: Update the deployment API contract documentation to reference the Go
constant or slice that defines the 14 public paused-reason error types instead
of relying only on the numeric count. Keep the existing behavior description
unchanged, and use the exact source symbol name so readers can verify the list.
In `@internal/db/deployment_mapper_test.go`:
- Around line 168-177: Extend the “advance schedule” test case for
buildDeploymentMapperAdvanceSchedule with a negative SQL-fragment assertion,
following the existing “include archived omits archived filter” pattern. Verify
the generated statement does not contain any schedule_revision increment or
update, while preserving the existing positive fragments and argument
expectations.
In `@internal/db/deployment_mapper.xml`:
- Around line 224-233: Add a partial index in migration 00049 matching the
ListSchedulesMissingNextScheduledAt predicates, covering active, non-archived,
non-deleted deployments with a non-null schedule and null next_scheduled_at; add
the corresponding deployments_uninitialized_schedule_idx drop statement to the
migration’s Down section.
In `@internal/db/migrations/00049_schedule_deployments_with_river.sql`:
- Around line 13-31: Address the lock impact in migration 00049 by either
explicitly confirming that deployment_runs is small in every deployed
environment and keeping the current operations, or split the migration to use
NOT VALID followed by later VALIDATE CONSTRAINT and concurrent index creation
with the required goose NO TRANSACTION directive. Preserve the metadata-only
trigger_context removal, acknowledging its irreversible behavior.
In `@internal/db/webhooks.go`:
- Around line 70-98: Refactor the event processing loop around
ListActiveForEvent and hasEndpoints to cache active endpoints by event type,
using the actual row type returned by endpointMapper.ListActiveForEvent for the
map values. Reuse cached results for repeated event types so each type is
queried once, and move the loop-invariant fallback handling for !hasEndpoints
outside the endpoint lookup path while preserving existing payload and insertion
behavior.
In `@internal/deployments/cron_test.go`:
- Around line 9-72: Reorder the test functions so
TestNormalizeOptionalScheduleRejectsUnsupportedSyntax appears before the
successful nextScheduledTimes tests TestNextScheduledTimesHandlesLeapDay and
TestNextScheduledTimesHandlesDST. Keep each test’s implementation unchanged and
retain TestNormalizeOptionalScheduleAcceptsSundaySeven with the success
scenarios.
- Around line 54-65: Add the unsatisfiable expression `0 0 30 2 *` to the
`tests` table in `TestNormalizeOptionalScheduleRejectsUnsupportedSyntax`,
preserving the existing assertion that `normalizeOptionalSchedule` returns an
error for every listed expression. Ensure the parser and normalization logic
reject this expression before enabling the test.
In `@internal/deployments/cron.go`:
- Around line 68-72: Update the timezone validation in the schedule parsing flow
around config.Timezone and time.LoadLocation: either assign an explicit UTC
default when the trimmed value is blank, or reject blank values as invalid if
timezone selection is required. Ensure the chosen behavior is explicit and
consistent with the API contract before loading the location.
In `@internal/deployments/execution.go`:
- Around line 31-43: The prepareDeploymentRun error paths for
sessionEventsFromInitialEvents, sessionResourcesFromDeployment, and
httpapi.MarshalRaw currently return unclassified errors that scheduler.go
records as session_resource_not_found_error. Update prepareDeploymentRun to
return a classified error, or wrap each failure with an appropriate distinct
error type, so recordFailure and shouldAutoPause receive the correct failure
reason for malformed events, resource preparation, and marshal failures.
In `@internal/deployments/handler.go`:
- Around line 620-638: Move the deployment read and nextScheduledAt calculation
into the transaction callback used by the unpause flow, ensuring they operate on
the transaction’s current or locked row before calling UnpauseDeploymentTx and
enqueueScheduledOccurrenceTx. Keep error logging and HTTP error responses
consistent, and ensure the persisted next_scheduled_at and enqueued job derive
from the same schedule.
- Around line 775-862: Split validateRunReferences into the requested helpers:
validateAgentReferences, validateSkillReferences, validateVaultReferences, and
validateResourceReferences. Move each corresponding validation block into its
helper, preserving existing error classification, ordering, and return behavior,
then have validateRunReferences invoke them sequentially and continue only when
each succeeds.
- Around line 289-297: Rename the local result variable in create from
nextScheduledAt to nextRunAt so it does not shadow the nextScheduledAt function,
update its nil check and revision logic, and use nextRunAt in the
NextScheduledAt struct field assignment.
- Around line 822-861: Replace the sequential lookups in the vault and resources
validation flow with batch-by-external-ID queries for vaults, files, and memory
stores, grouping resource IDs by type before querying. Validate the returned
records as a set, preserving missing-reference and archived-record handling
through classifyReferenceFailure, and keep the existing invalid JSON behavior
unchanged.
In `@internal/deployments/scheduler_test.go`:
- Around line 1-9: Add a test in the scheduler test suite covering the
successful scheduled execution path: verify session, deployment run, and outbox
rows are created and the schedule cursor advances. Reuse the existing test
fixtures, helpers, and success-path symbols in the scheduler implementation,
while preserving the current failure, auto-pause, and webhook tests.
In `@internal/deployments/scheduler.go`:
- Around line 217-220: Update the invalid-schedule handling in Work around
nextAfterScheduled so an unparseable stored schedule is treated as a
non-retryable preparation failure: record a failure run and automatically pause
the deployment, matching the existing handling for other non-retryable
preparation failures. Ensure Work does not return the parse error for River
retry, and keep reconcile from repeatedly skipping the unchanged active
deployment.
- Around line 263-288: After a successful ApplyScheduledOccurrence call in the
scheduledDeploymentWorker flow, immediately enqueue the newly computed
nextScheduledAt occurrence instead of waiting for reconciliation. Update
scheduledDeploymentWorker or its dependencies to provide enqueueNext with the
River client or a minimal inserter interface, while preserving the reconcile
loop as the recovery path and existing error handling.
- Around line 82-94: The DeploymentScheduler.Start method currently blocks
startup and propagates per-deployment failures from backfillNextScheduledAt and
reconcile. Move the initial backfill/reconcile work into the background
reconciliation flow so client.Start executes without waiting, and handle
individual deployment errors by logging them while allowing the loop to continue
rather than returning them as startup errors.
In `@internal/webhooks/enqueuer_test.go`:
- Around line 16-37: Reorder the tests in the relevant test file so the existing
failure-scenario test using failingEnqueueStore appears before
TestPrepareDeliveryEventPreservesOutboxData. Do not change either test’s
implementation or assertions.
- Around line 22-36: Extend the assertions in the PrepareDeliveryEvent test to
verify event.ID, event.Data.Type, event.Data.WorkspaceID, and
event.Data.OrganizationID alongside the existing fields. Split the combined
condition into field-specific assertions so failures identify the mismatched
field, and add the strings import only if needed for the expected workspace or
organization value checks.
In `@main.go`:
- Around line 136-142: Increase the shutdown context timeout in the deferred
cleanup around deploymentScheduler.Stop from 10 seconds to a value longer than
the scheduler’s SoftStopTimeout, such as 20 seconds, so the hard-stop phase can
complete without recurring timeout errors.
In `@tests/deployments_api_test.go`:
- Around line 720-768: Reorder the subtests in the test suite so the failure
scenarios “failure auto pause rolls back...”, “failure scheduled root agent
archive...”, and “failure agent archive rolls back...” appear before the
successful “agent API archives deployments with webhook outbox” subtest. Keep
each subtest’s implementation unchanged.
In `@web/src/features/managed-agents/ManagedAgentsPage.resources.suite.tsx`:
- Line 2187: Update the fixture assignment near api.resources.deployments in the
managed agents resources suite to match the real DeploymentApiResponse schedule
shape by using the response’s expression field instead of
QuickstartDeploymentInput’s cron_expression; if this schedule is not exercised,
remove the assignment.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 468a4729-af4c-4a0c-9e8f-6cd499959c24
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (35)
cmd/migrate/main.godocs/design/be/deployments-api-contract.mdgo.modinternal/agents/handler.gointernal/api/server.gointernal/config/defaults.gointernal/db/agents.gointernal/db/db.gointernal/db/deployment_mapper.gointernal/db/deployment_mapper.xmlinternal/db/deployment_mapper_test.gointernal/db/deployment_run_mapper.gointernal/db/deployment_run_mapper.xmlinternal/db/deployments.gointernal/db/migrations/00049_schedule_deployments_with_river.sqlinternal/db/migrations_test.gointernal/db/webhooks.gointernal/deployments/cron.gointernal/deployments/cron_test.gointernal/deployments/execution.gointernal/deployments/handler.gointernal/deployments/handler_contract_test.gointernal/deployments/resources.gointernal/deployments/scheduler.gointernal/deployments/scheduler_test.gointernal/webhooks/enqueuer.gointernal/webhooks/enqueuer_test.gomain.gotests/deployments_api_test.gotests/uuid_boundary_postgres_test.goweb/src/features/managed-agents/ManagedAgentsPage.resources.suite.tsxweb/src/features/managed-agents/ManagedAgentsPage.test-utils.tsxweb/src/features/managed-agents/resources/detail.tsxweb/src/features/managed-agents/resources/model.tsxweb/src/features/managed-agents/types.ts
💤 Files with no reviewable changes (1)
- web/src/features/managed-agents/resources/model.tsx
|
这轮只修了 4 个正确性问题:Workspace 归档、无解 Cron、unpause 旧快照和执行配置 revision。30 秒 reconcile 和 Worker 全链路测试没动,按之前结论另算。 麻烦再看一轮。 @codex review |
|
Codex Review: Didn't find any major issues. Swish! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/deployments_api_test.go (1)
557-568: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the lock-contention assertions deterministic.
The tests use elapsed time without proving that the competing operation reached the database lock boundary. A delayed goroutine can make either test pass without testing the required blocking behavior.
tests/deployments_api_test.go#L557-L568: Synchronize afterUpdateDeploymentreaches its row-lock attempt before committingworkerTx.tests/deployments_api_test.go#L658-L680: Signal after the unpause request reaches its row-lock attempt, not beforeapp.client.Do(req).Use a test hook or observable database lock state. Do not use a fixed sleep as the synchronization condition.
🤖 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 `@tests/deployments_api_test.go` around lines 557 - 568, Make the lock-contention assertions deterministic in tests/deployments_api_test.go at lines 557-568 and 658-680: add a test hook or observable database lock-state signal so the worker synchronizes only after UpdateDeployment reaches its row-lock attempt before committing workerTx, and the unpause test signals only after app.client.Do(req) reaches its row-lock attempt. Replace the fixed time-based synchronization with these signals; do not use sleeps or elapsed-time checks.docs/design/be/deployments-api-contract.md (1)
95-95: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win定义
deployment_runwebhook 的顺序和幂等处理规则。
succeeded/failed与started是当前独立jobs记录。订单号相同的jobs只能按run_after/created_at排序领取,当前实现不会按 Run 串行投递;重试或并发交付 may 导致终态事件先到达或重复送达。在合同中补充顺序键、重复记录规则,或实现/约束 Run 级别的 webhook delivery worker 串行性。🤖 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 `@docs/design/be/deployments-api-contract.md` at line 95, 补充 deployment_run webhook 的顺序与幂等契约:明确以同一 Run ID 作为顺序键,started 必须先于 succeeded/failed 投递,并规定重试或并发导致的重复事件及终态先到达时的处理规则;若无法仅通过合同保证,则约束对应 webhook delivery worker 按 Run 串行投递。internal/db/deployment_mapper.xml (1)
23-24: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winRun the Yourbatis generator for
DeploymentMapper.
internal/db/deployment_mapper.godeclaresdeployment_mapper.sqlmap.gen.goas generator output, and the XML now addsschedule_revision,next_scheduled_at, and the schedule-related statements. Regenerate matching generated code rather than shipping stale SQL-map output.🤖 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/db/deployment_mapper.xml` around lines 23 - 24, Regenerate the MyBatis output for DeploymentMapper so deployment_mapper.sqlmap.gen.go reflects the XML additions, including schedule_revision, next_scheduled_at, and the schedule-related statements. Do not manually patch the generated file; run the repository’s established generator and include the resulting synchronized output.Source: Coding guidelines
🧹 Nitpick comments (1)
internal/db/deployment_mapper.xml (1)
224-243: 🚀 Performance & Scalability | 🔵 TrivialBound the reconciliation reads before scale-up.
Both queries return all matching deployments without a
LIMIT. With a 30-second reconciliation loop, each cycle can materialize the full active schedule set. Add keyset pagination or bounded batches, or validate that the expected schedule count and indexes keep this cost acceptable.This follows the PR objective of a 30-second reconciliation loop.
🤖 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/db/deployment_mapper.xml` around lines 224 - 243, The ListActiveSchedules and ListSchedulesMissingNextScheduledAt queries currently read unbounded result sets each reconciliation cycle. Add bounded batching with keyset pagination using stable ordering keys, or otherwise enforce an appropriate LIMIT and continuation mechanism, while preserving their existing filters and ordering so reconciliation remains scalable.
🤖 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 `@docs/design/be/deployments-api-contract.md`:
- Line 65: 明确文档中的终态 Run 规则:每个 scheduled occurrence 都必须创建对应的 deployment_run,并为其生成
Run ID 和 webhook;创建 Session、Deployment Run 与 outbox 必须保持原子性。更新游标规则,使提交任一终态
Run(成功或非自动暂停的最终失败)后都推进到下一个 occurrence,仅自动暂停的失败停止推进。
---
Outside diff comments:
In `@docs/design/be/deployments-api-contract.md`:
- Line 95: 补充 deployment_run webhook 的顺序与幂等契约:明确以同一 Run ID 作为顺序键,started 必须先于
succeeded/failed 投递,并规定重试或并发导致的重复事件及终态先到达时的处理规则;若无法仅通过合同保证,则约束对应 webhook
delivery worker 按 Run 串行投递。
In `@internal/db/deployment_mapper.xml`:
- Around line 23-24: Regenerate the MyBatis output for DeploymentMapper so
deployment_mapper.sqlmap.gen.go reflects the XML additions, including
schedule_revision, next_scheduled_at, and the schedule-related statements. Do
not manually patch the generated file; run the repository’s established
generator and include the resulting synchronized output.
In `@tests/deployments_api_test.go`:
- Around line 557-568: Make the lock-contention assertions deterministic in
tests/deployments_api_test.go at lines 557-568 and 658-680: add a test hook or
observable database lock-state signal so the worker synchronizes only after
UpdateDeployment reaches its row-lock attempt before committing workerTx, and
the unpause test signals only after app.client.Do(req) reaches its row-lock
attempt. Replace the fixed time-based synchronization with these signals; do not
use sleeps or elapsed-time checks.
---
Nitpick comments:
In `@internal/db/deployment_mapper.xml`:
- Around line 224-243: The ListActiveSchedules and
ListSchedulesMissingNextScheduledAt queries currently read unbounded result sets
each reconciliation cycle. Add bounded batching with keyset pagination using
stable ordering keys, or otherwise enforce an appropriate LIMIT and continuation
mechanism, while preserving their existing filters and ordering so
reconciliation remains scalable.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 087e8caa-7ffe-48d3-bea5-f1ab2dac9ca2
📒 Files selected for processing (11)
docs/design/be/deployments-api-contract.mdinternal/db/db.gointernal/db/deployment_mapper.gointernal/db/deployment_mapper.xmlinternal/db/deployment_mapper_test.gointernal/db/deployments.gointernal/deployments/cron.gointernal/deployments/cron_test.gointernal/deployments/handler.gointernal/deployments/scheduler.gotests/deployments_api_test.go
🚧 Files skipped from review as they are similar to previous changes (7)
- internal/db/db.go
- internal/db/deployment_mapper.go
- internal/db/deployment_mapper_test.go
- internal/deployments/scheduler.go
- internal/deployments/cron.go
- internal/db/deployments.go
- internal/deployments/handler.go
|
@codex review |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/db/migrations/00050_schedule_deployments_with_river.sql`:
- Around line 14-28: Split the constraint and index changes into subsequent
numbered migrations: add deployment_runs_scheduled_at_check as NOT VALID, then
validate it in a later migration; create deployment_runs_schedule_occurrence_idx
and deployments_pending_schedule_idx with CREATE INDEX CONCURRENTLY in NO
TRANSACTION migrations. Update each Down path to drop the corresponding indexes,
preserving the existing constraint semantics.
🪄 Autofix
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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b95686de-1cbb-42bd-bd64-4adc46f73f4a
📒 Files selected for processing (11)
internal/api/server.gointernal/config/reference_test.gointernal/db/agents.gointernal/db/db.gointernal/db/deployments.gointernal/db/migrations/00050_schedule_deployments_with_river.sqlinternal/db/migrations_test.gointernal/db/webhooks.gomain.gotests/uuid_boundary_postgres_test.goweb/src/features/managed-agents/resources/model.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- web/src/features/managed-agents/resources/model.tsx
- internal/db/db.go
- internal/db/migrations_test.go
- main.go
- internal/db/agents.go
- internal/api/server.go
- tests/uuid_boundary_postgres_test.go
- internal/db/webhooks.go
- internal/db/deployments.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dade2a3a66
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
| func updateDeployment(ctx context.Context, executor yourbatis.Executor, workspaceUUID string, externalID string, input UpdateDeploymentInput) (Deployment, error) { | ||
| deploymentMapper := NewDeploymentMapper(executor) | ||
| current, err := deploymentMapper.LockByExternalID(ctx, workspaceUUID, externalID) |
There was a problem hiding this comment.
不要 lock,不用那么严格的事务,这个 lock 的语义太大,整个都 lock 住了,同下。 为了性能考虑,可以牺牲一些东西
There was a problem hiding this comment.
对齐一下当前状态:
- 组织级配额锁已经去掉,改成 best-effort
COUNT。 UpdateDeployment/ApplyScheduledOccurrence/ manual run 仍保留 deployment 行级FOR UPDATE,用来做 archived/active/schedule/执行配置校验,避免和并发更新交错。
如果你希望 Update / manual run 也去掉行锁、只保留 Apply 路径的锁(或全部去掉、靠唯一索引兜底),说一下偏好我按那个改。
There was a problem hiding this comment.
对齐确认:
- 组织配额锁已去掉,改 best-effort
COUNT。 UpdateDeployment/ApplyScheduledOccurrence/ manual run 目前仍保留 deployment 行级FOR UPDATE,用来做 archived/active/schedule/执行配置校验。
如果你希望 Update / manual run 也去掉行锁、只保留 Apply,或全部去掉靠唯一索引兜底,直接说偏好我按那个改。
| current.AgentUUID != next.AgentUUID || | ||
| current.AgentExternalID != next.AgentExternalID || | ||
| current.AgentVersion != next.AgentVersion || | ||
| !sameJSON(current.AgentSnapshot, next.AgentSnapshot) || |
There was a problem hiding this comment.
对齐一下:schedule 本身已经有命名结构 deploymentSchedule;执行配置比较里的 agent_snapshot / initial_events / resources / vault_ids 等目前仍是边界 JSON + sameJSON。
如果这轮就要把这些字段收成命名 DTO 再比较,我可以跟;也可以留到后续独立清理 PR。你更倾向哪种?
There was a problem hiding this comment.
对齐确认:schedule 已有命名结构 deploymentSchedule;执行配置比较里的 agent_snapshot / initial_events / resources / vault_ids 等目前仍是边界 JSON + sameJSON。
这轮如果要一起收成命名 DTO 再比较可以说一声;否则我先留着,后续单独清理。
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b981fad07
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/pullfrog.yml (3)
20-22: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winLimit
pullfrogcredentials to the selected provider.Every active secret in this step is available to
pullfrog/pullfrog@v0, not only the provider selected by the caller. Pass only the selected provider key. Dropid-token: writeunless this workflow intentionally enables GitHub OIDC for this action.🤖 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 @.github/workflows/pullfrog.yml around lines 20 - 22, Update the pullfrog workflow permissions and action configuration so pullfrog receives only the credential for the provider selected by the caller, rather than all active secrets. Remove id-token: write unless pullfrog explicitly requires GitHub OIDC, while retaining only the minimum contents permission needed.Source: MCP tools
25-29: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winPin both actions to full-length commit SHAs.
The workflow uses mutable tags for
actions/checkout@v6andpullfrog/pullfrog@v0. Pin bothuses: ...entries to verified full-length commit SHAs, and keep the version in a trailing comment for maintainability.🤖 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 @.github/workflows/pullfrog.yml around lines 25 - 29, Update the workflow’s actions/checkout and pullfrog/pullfrog uses entries to verified full-length commit SHAs, retaining their current v6 and v0 versions in trailing comments for maintainability.Source: MCP tools
24-27: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winDisable checkout credential persistence.
actions/checkoutpersists its checkout credentials by default. Since the workflow only reads the repository contents and no later step runs authenticated Git commands, addpersist-credentials: falseto the checkout step.Proposed fix
with: fetch-depth: 1 + persist-credentials: false🤖 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 @.github/workflows/pullfrog.yml around lines 24 - 27, Update the actions/checkout step to set persist-credentials to false alongside fetch-depth, while preserving the existing checkout behavior.Sources: MCP tools, Linters/SAST tools
🤖 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.
Outside diff comments:
In @.github/workflows/pullfrog.yml:
- Around line 20-22: Update the pullfrog workflow permissions and action
configuration so pullfrog receives only the credential for the provider selected
by the caller, rather than all active secrets. Remove id-token: write unless
pullfrog explicitly requires GitHub OIDC, while retaining only the minimum
contents permission needed.
- Around line 25-29: Update the workflow’s actions/checkout and
pullfrog/pullfrog uses entries to verified full-length commit SHAs, retaining
their current v6 and v0 versions in trailing comments for maintainability.
- Around line 24-27: Update the actions/checkout step to set persist-credentials
to false alongside fetch-depth, while preserving the existing checkout behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 33c29678-c4cc-4311-ad5f-6a57b4a954e8
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (8)
.github/workflows/pullfrog.ymlgo.modinternal/db/db.gotests/deployments_api_test.gotests/uuid_boundary_postgres_test.goweb/src/features/managed-agents/ManagedAgentsPage.resources.suite.tsxweb/src/features/managed-agents/resources/detail.tsxweb/src/features/managed-agents/resources/model.tsx
🚧 Files skipped from review as they are similar to previous changes (6)
- web/src/features/managed-agents/resources/model.tsx
- web/src/features/managed-agents/ManagedAgentsPage.resources.suite.tsx
- tests/uuid_boundary_postgres_test.go
- web/src/features/managed-agents/resources/detail.tsx
- go.mod
- tests/deployments_api_test.go
…duler-river # Conflicts: # internal/api/server.go
…duler-river # Conflicts: # internal/deployments/handler.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 241c4708cf
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4b2ccffd4b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…duler-river # Conflicts: # internal/agents/handler.go # internal/deployments/handler.go
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dcdb618516
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: daaa3e570d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: afa6e89ef3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f518b2b6e7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
Preview deployment for your docs. Learn more about Mintlify Previews.
💡 Tip: Enable Workflows to automatically generate PRs for you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ccd25798c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
|
|
||
| type deploymentContentBlock struct { | ||
| Type string `json:"type"` | ||
| Text string `json:"text,omitempty"` |
There was a problem hiding this comment.
Preserve content-block citation settings
When an initial event contains a document block with the supported citations option, normalizeMessageContent now decodes it into this closed struct and later re-encodes the normalized events, silently discarding that field. Before this change, Content remained a json.RawMessage, so accepted citation configuration reached the created session event unchanged; both manual and scheduled runs now lose the requested citation behavior. Include the contract field in the DTO or preserve validated boundary fields.
AGENTS.md reference: AGENTS.md:L116-L116
Useful? React with 👍 / 👎.
| _, err = deploymentMapper.UpdateLastRun( | ||
| ctx, deployment.WorkspaceUUID, deployment.ExternalID, input.Now, | ||
| ) |
There was a problem hiding this comment.
Prevent last_run_at from moving backward
When two occurrences for the same deployment overlap—for example after the queue is backlogged—the workers capture input.Now before acquiring the deployment lock. A newer occurrence can therefore commit first, followed by an older occurrence that overwrites both last_run_at and updated_at with its earlier timestamp, making the API report a stale last run. Compare against the locked value or update these timestamps monotonically, and cover overlapping occurrence order.
AGENTS.md reference: AGENTS.md:L117-L117
Useful? React with 👍 / 👎.
River overwrites river_job.scheduled_at on retry, so the worker must not use that column as the schedule identity. Stamp the Cron time into job args at insert and key deployment runs on that value.
Dismissed the prior Cursor Approval Agent approval because it applied to an older commit and current review state still requires human attention. Cursor Bugbot was not present after the initial check poll, so that signal was skipped. No additional reviewers were assigned.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2b2c4a4857
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| err = w.applyOccurrence(ctx, db.ApplyScheduledOccurrenceInput{ | ||
| Deployment: deployment, ScheduledAt: scheduledAt, | ||
| Session: &preparedRun.Session, Events: preparedRun.Events, |
There was a problem hiding this comment.
Emit lifecycle webhooks for scheduled sessions
When a webhook endpoint subscribes to the existing session lifecycle events, sessions created by a Cron occurrence never emit session.created, session.pending, session.status_idled, session.thread_created, session.thread_idled, or the initial outcome event: the manual-run path explicitly enqueues them after creation, but this worker commits the session directly and neither it nor ApplyScheduledOccurrence writes webhook outbox records. Downstream integrations therefore cannot discover or initialize scheduled sessions; include these lifecycle events in the occurrence transaction.
AGENTS.md reference: AGENTS.md:L117-L117
Useful? React with 👍 / 👎.
Keep official 0-7 DOW compatibility with a minimal 7→0 rewrite for cron/v3. Merge cron compatibility tests and document why River job args store the Cron occurrence.



目标
为 Deployment 增加计划执行能力:配置 Cron 后,由 River 自动创建 scheduled Deployment Run 和 Session。
方案
一致性
Scheduled Worker 使用 River Job 的 scheduled_at 作为 occurrence:
部分唯一索引
(deployment_uuid, scheduled_at) WHERE trigger_type = 'schedule'防止 River at-least-once 投递产生重复 Run。数据库和进程级错误交给 River 重试;确定性的业务失败记录失败 Run,并按公开 allowlist 自动暂停 Deployment。Root Agent 归档与其 Deployment 级联归档在同一个数据库事务中完成。Webhook 变更不在本 PR 范围,后续统一实现。
数据与 API
验证
参考