feat(rewrite): add durable jobs and worker foundation - #401
Conversation
|
Warning Review limit reached
Next review available in: 13 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis change adds durable jobs and schedules with SQLite persistence, validated contracts, worker coordination, leases, retries, realtime events, authenticated procedures, application wiring, and production lifecycle verification. ChangesDurable jobs platform
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant DashboardServer
participant JobService
participant JobRepository
participant WorkerRuntime
participant JobWorkerCoordinator
participant JobActionRegistry
DashboardServer->>JobService: reconcile registered schedules
JobService->>JobRepository: persist schedule records
WorkerRuntime->>JobWorkerCoordinator: initialize worker
JobWorkerCoordinator->>JobRepository: claim due run
JobWorkerCoordinator->>JobActionRegistry: execute registered action
JobWorkerCoordinator->>JobRepository: settle run and append events
WorkerRuntime->>JobWorkerCoordinator: dispose with termination signal
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
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 |
mira-2026
added
status: needs-review
mira-2026
changed the title
mira-2026
added
area: backend
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (24)
greenfield/src/contracts/jobModel.ts (1)
505-512: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueParenthesize the mixed relational and equality comparison.
Line 509 reads
hasStarted !== run.attemptCount > 0. The relational operator binds first, so the expression is correct. Explicit parentheses remove the ambiguity for readers.♻️ Proposed readability fix
- hasStarted !== (run.lastAttemptStartedAtMs !== undefined) || - hasStarted !== run.attemptCount > 0 + hasStarted !== (run.lastAttemptStartedAtMs !== undefined) || + hasStarted !== run.attemptCount > 0Apply the same shape as the neighboring comparison:
hasStarted !== (run.attemptCount > 0)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@greenfield/src/contracts/jobModel.ts` around lines 505 - 512, In the validation logic around hasStarted, parenthesize the relational comparison in the mixed expression so it explicitly reads as hasStarted compared with (run.attemptCount > 0), matching the neighboring comparison’s shape without changing behavior.greenfield/src/contracts/jobModel.test.ts (1)
217-264: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd positive cases for
drainingandstoppedworkers.The worker tests assert only one accepted projection (
online). Alldrainingandstoppedsamples are negative. A regression that rejects every non-onlineworker would pass this suite.♻️ Suggested extra assertions
+ expect( + v.parse(jobWorkerSummarySchema, { + activeRunCount: 1, + capacity: 2, + drainingAtMs: 2500, + heartbeatAtMs: 2600, + id: workerId, + releaseId: "a".repeat(40), + startedAtMs: 1000, + state: "draining", + }).state + ).toBe("draining"); + expect( + v.parse(jobWorkerSummarySchema, { + activeRunCount: 0, + capacity: 2, + drainingAtMs: 2500, + heartbeatAtMs: 2600, + id: workerId, + releaseId: "a".repeat(40), + startedAtMs: 1000, + state: "stopped", + stoppedAtMs: 3000, + }).state + ).toBe("stopped");🤖 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 `@greenfield/src/contracts/jobModel.test.ts` around lines 217 - 264, Add accepted test cases in “validates worker and schedule projections across state boundaries” for both “draining” and “stopped” worker states, using valid field combinations and asserting successful parsing. Keep the existing negative boundary cases, including invalid stopped projections, so the suite verifies non-online states are accepted without weakening validation.greenfield/src/contracts/jobs.ts (1)
290-310: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared job-domain access and transport policy. Both contract files declare identical
queryTransportandmutationTransportobjects and identicaljobs:read/jobs:writeaccess objects. One policy change must then be edited in two places, and the two files can drift.
greenfield/src/contracts/jobs.ts#L290-L310: movejobReadAccess,jobSessionWriteAccess,queryTransport, andmutationTransportinto a shared module, for example./jobAccess.ts, and import them here.greenfield/src/contracts/schedules.ts#L181-L206: import the shared constants instead of redefining them. KeepscheduleRunAccesslocal, because it intentionally omitsprincipalKinds.🤖 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 `@greenfield/src/contracts/jobs.ts` around lines 290 - 310, Extract the shared job-domain constants jobReadAccess, jobSessionWriteAccess, queryTransport, and mutationTransport from greenfield/src/contracts/jobs.ts lines 290-310 into a shared module such as ./jobAccess.ts, then import and reuse them in jobs.ts. In greenfield/src/contracts/schedules.ts lines 181-206, replace the duplicate access and transport definitions with imports from that module, while keeping scheduleRunAccess local because its principalKinds behavior differs.greenfield/src/server/database/schema/jobRuns.ts (1)
121-124: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the event-budget constants with the repository.
The budget values
1000,967, and1048576are written as literals here, again in the hand-written triggers in the migration file (Lines 1259, 1284, 1291, 1296), and again as named constants ingreenfield/src/server/domains/jobs/repository.ts(jobRunEventMaximum,jobRunPayloadEventMaximum,jobPayloadEventByteMaximum). Three copies can drift, and drift shows up only as an aborted transaction at runtime. Import the shared constants here and interpolate them.🤖 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 `@greenfield/src/server/database/schema/jobRuns.ts` around lines 121 - 124, Update the job_runs_event_budget_check definition to import and interpolate jobRunEventMaximum, jobRunPayloadEventMaximum, and jobPayloadEventByteMaximum from the jobs repository instead of using the literals 1000, 967, and 1048576; preserve the existing event-count and payload constraints.greenfield/src/server/database/schema/jobChecks.ts (1)
57-81: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winWrap the
ORhelpers in parentheses.
optionalJobMessageCheck,optionalJobTerminalCodeCheck, andoptionalBoundedJobTextCheckreturn a top-levelORexpression without enclosing parentheses. Every current caller uses them standalone, so the generated SQL is correct today. If a later caller composes one of them withAND, SQLite appliesANDbeforeORand the constraint silently weakens.jobActorCheckalready wraps its result on Line 50. Apply the same rule to these three helpers.♻️ Proposed change
- return sql`${column} IS NULL OR (${boundedControlSafeTextCheck(column, maximumCodePoints)} AND length(CAST(${column} AS BLOB)) <= ${maximumBytesSql})`; + return sql`(${column} IS NULL OR (${boundedControlSafeTextCheck(column, maximumCodePoints)} AND length(CAST(${column} AS BLOB)) <= ${maximumBytesSql}))`;- return sql`${column} IS NULL OR (length(${column}) BETWEEN 1 AND ${maximumLengthSql} AND ${nulFreeTextCheck(column)} AND ${column} = lower(${column}) AND substr(${column}, 1, 1) GLOB '[a-z0-9]' AND ${column} NOT GLOB '*[^a-z0-9._/-]*')`; + return sql`(${column} IS NULL OR (length(${column}) BETWEEN 1 AND ${maximumLengthSql} AND ${nulFreeTextCheck(column)} AND ${column} = lower(${column}) AND substr(${column}, 1, 1) GLOB '[a-z0-9]' AND ${column} NOT GLOB '*[^a-z0-9._/-]*'))`;- return sql`${column} IS NULL OR (${boundedNonBlankTextCheck(column, maximumLength)})`; + return sql`(${column} IS NULL OR (${boundedNonBlankTextCheck(column, maximumLength)}))`;Note: this changes the emitted SQL text, so the migration file and the checksums in
greenfield/src/shared/databaseMigrationManifest.tsmust be regenerated.🤖 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 `@greenfield/src/server/database/schema/jobChecks.ts` around lines 57 - 81, Wrap the complete SQL expressions returned by optionalJobMessageCheck, optionalJobTerminalCodeCheck, and optionalBoundedJobTextCheck in parentheses, matching jobActorCheck so future AND composition preserves the intended OR grouping. Regenerate the migration file and update checksums in databaseMigrationManifest.ts to reflect the emitted SQL changes.greenfield/src/server/database/schema/workerInstances.ts (1)
28-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
lowercaseHexTextCheckforrelease_id.This predicate repeats the lowercase-hex rule that
lowercaseHexTextCheckalready provides.greenfield/src/server/database/schema/jobRuns.tsLine 119 uses that helper forenqueue_sha256. Use the helper here so both hex columns share one definition.♻️ Proposed change
import { - nulFreeTextCheck, + lowercaseHexTextCheck, timestampMillisecondsCheck, uuidV7TextCheck, } from "./checks.ts";check( "worker_instances_release_id_check", - sql`length(${table.releaseId}) = 40 AND ${nulFreeTextCheck(table.releaseId)} AND ${table.releaseId} = lower(${table.releaseId}) AND ${table.releaseId} NOT GLOB '*[^0-9a-f]*'` + lowercaseHexTextCheck(table.releaseId, 40) ),Verify that the helper emits the same SQL text before you regenerate the migration and the manifest checksums.
🤖 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 `@greenfield/src/server/database/schema/workerInstances.ts` around lines 28 - 31, Update the worker_instances release_id constraint to reuse the existing lowercaseHexTextCheck helper, matching the usage for enqueue_sha256 in jobRuns.ts, instead of duplicating the lowercase-hex predicate inline. Verify the helper emits equivalent SQL, then regenerate the migration and manifest checksums.greenfield/src/server/database/schema/scheduledJobs.ts (1)
96-99: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMove
timeZonevalidation into the database check.The application schema already enforces
scheduleTimeZoneSchemaand rejects values such asUS/Eastern,GMT,+01:00, andlocal. The SQLite check is still the only source of truth for direct SQL writes and can still persist unparseabletime_zonevalues. Add a matching canonical/check constraint or make this check consistent with the contract validation.🤖 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 `@greenfield/src/server/database/schema/scheduledJobs.ts` around lines 96 - 99, Update scheduled_jobs_time_zone_check so database validation matches scheduleTimeZoneSchema rather than only checking length and NUL characters. Enforce the canonical accepted time-zone format for direct SQL writes, reject values such as US/Eastern, GMT, +01:00, and local, and preserve the existing allowance for NULL.greenfield/src/server/database/validation/jobWorkerControl.ts (1)
71-77: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
nonnegativeDateSchemafor the update timestamp.
greenfield/src/server/database/validation/jobDisableIntents.tsline 147 callsnonnegativeDateSchema(v.date())for the same purpose. Reusing the helper keeps one definition of the timestamp rule and the same error message across tables.♻️ Proposed fix
- updatedAt: v.pipe( - v.date("Worker control timestamp is invalid"), - v.check( - (value) => Number.isFinite(value.getTime()) && value.getTime() >= 0, - "Worker control timestamp is invalid" - ) - ), + updatedAt: nonnegativeDateSchema(v.date("Worker control timestamp is invalid")),🤖 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 `@greenfield/src/server/database/validation/jobWorkerControl.ts` around lines 71 - 77, Replace the inline validation pipeline for updatedAt with the shared nonnegativeDateSchema helper, passing v.date() as used by jobDisableIntents. Preserve the existing field validation while centralizing the nonnegative timestamp rule and error message.greenfield/src/server/database/validation/jobRuns.ts (1)
156-161: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd parentheses to the mixed comparison.
hasStarted !== run.attemptCount > 0parses ashasStarted !== (run.attemptCount > 0)because relational operators bind tighter than equality operators. The behavior is correct, but the expression is hard to read next to the explicit parentheses on the line above.♻️ Proposed readability fix
if ( hasStarted !== (run.lastAttemptStartedAt !== null) || - hasStarted !== run.attemptCount > 0 + hasStarted !== run.attemptCount > 0 === false ) {Use the explicit form instead:
if ( hasStarted !== (run.lastAttemptStartedAt !== null) || hasStarted !== (run.attemptCount > 0) ) { return 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 `@greenfield/src/server/database/validation/jobRuns.ts` around lines 156 - 161, In the validation condition, update the comparison involving run.attemptCount so the relational check is explicitly parenthesized as (run.attemptCount > 0), matching the existing parentheses around run.lastAttemptStartedAt and preserving behavior.greenfield/src/server/database/validation/jobDisableIntents.ts (1)
22-28: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider extracting the duplicated
actorIsValidhelper.The same function exists in
greenfield/src/server/database/validation/jobRuns.ts(lines 43-49), and a two-kind variant exists ingreenfield/src/server/database/validation/jobWorkerControl.ts(controlActorIsValid). A shared helper in one validation module keeps the actor-identity rules in one place.🤖 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 `@greenfield/src/server/database/validation/jobDisableIntents.ts` around lines 22 - 28, Extract the shared actor-identity validation logic from actorIsValid in jobDisableIntents.ts and the equivalent helper in jobRuns.ts into a common validation helper, then update both callers to reuse it. Incorporate the compatible automation, system, and user handling from controlActorIsValid without changing the existing validation schemas or behavior.greenfield/src/server/database/validation/rowSchemas.test.ts (1)
540-601: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNegative tests do not pin the expected failure message. Across both test files, several rejection assertions use a bare
.toThrow(). Any failure satisfies them, including a failure that the test does not target, such as a renamed column trippingv.strictObjector a foreign-key violation replacing a trigger violation. The shared root cause is one missing expected-message argument on each assertion.
greenfield/src/server/database/validation/rowSchemas.test.ts#L540-L601: pass the specific Valibot message to each.toThrow(), for example"Stored job resource keys are not canonical"and"Stored job event count is invalid".greenfield/src/server/database/migrations/jobsSchema.test.ts#L781-L789: pass the expected event-count constraint message, matching the style of the neighboring assertions.greenfield/src/server/database/migrations/jobsSchema.test.ts#L1082-L1087: pass the expected identity-update trigger message forresource_leases.🤖 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 `@greenfield/src/server/database/validation/rowSchemas.test.ts` around lines 540 - 601, Replace every bare toThrow() in greenfield/src/server/database/validation/rowSchemas.test.ts lines 540-601 with the specific expected Valibot message for its validation case, including the canonical resource-keys and invalid event-count messages. Update the event-count rejection in greenfield/src/server/database/migrations/jobsSchema.test.ts lines 781-789 with its expected constraint message, and update the resource_leases identity-update rejection in lines 1082-1087 with the expected trigger message, matching neighboring assertions.greenfield/src/server/database/migrations/jobsSchema.test.ts (1)
265-275: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStatic analysis flags the interpolated query text here.
The helper builds
EXPLAIN QUERY PLAN ${query}by interpolation. The statement text cannot be parameterized, so interpolation is required for the plan prefix. All callers pass literals that are defined in this test file, so there is no injection path. The finding is a false positive for this helper.The embedded identifiers at Line 1158 and Line 1185 can still be bound as parameters, which removes the pattern that trips the scanner.
🤖 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 `@greenfield/src/server/database/migrations/jobsSchema.test.ts` around lines 265 - 275, Keep the required query interpolation in expectUsesIndexWithoutTemporarySort, but update the SQL statements containing embedded identifiers at the two flagged call sites to bind those values as parameters. Preserve the existing query plans and assertions while removing the interpolated identifier pattern that triggers static analysis.Source: Linters/SAST tools
greenfield/src/server/domains/jobs/service.ts (2)
766-778: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReconciliation emits a realtime event for one schedule id only.
The realtime payload uses
jobActionRegistrations[0]?.scheduleId, but reconciliation can insert or update every registered schedule. Subscribers that refresh a single schedule from this event will miss the other rows. Consider emitting one realtime entry per reconciled schedule, or a directory-level identity that clients interpret as "refresh the schedule list".🤖 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 `@greenfield/src/server/domains/jobs/service.ts` around lines 766 - 778, The reconciliation side effect currently identifies only the first schedule via jobActionRegistrations[0]?.scheduleId. Update the realtime payload in mutationSideEffects for jobs.schedule.reconcile to represent all reconciled schedules, either by emitting an entry per schedule or by using a directory-level identity that clients interpret as refreshing the schedule list; preserve the successful reconciliation metadata.
311-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare one schedule-insert builder with the coordinator.
scheduleInsertShapeduplicatesscheduleInsertingreenfield/src/server/domains/jobs/coordinator.ts(lines 245-278). The two builders produce the same durable row but differ in the failure mode: this one throwsJobValidationError, the coordinator throwsRangeError. Any future column addition must be applied twice, and a missed update writes inconsistent schedule rows.Extract one builder and let the caller map the missing-occurrence case to its own error.
🤖 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 `@greenfield/src/server/domains/jobs/service.ts` around lines 311 - 348, Extract the shared durable-row construction from scheduleInsertShape and coordinator’s scheduleInsert into one reusable builder, including all schedule columns and nextRunAt calculation. Have the builder expose the missing-occurrence case without choosing a caller-specific error, then let scheduleInsertShape map it to JobValidationError and the coordinator map it to RangeError while preserving each caller’s existing behavior.greenfield/src/server/domains/jobs/repository.ts (3)
972-996: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winBind the supplied queued event to the inserted run.
#insertSuppliedEventwritesinput.queuedEventwithout checking thatqueuedEvent.jobRunIdequalsrun.id. A caller mistake writes the event onto a different run, and the run row then reports an event it does not own. Add one guard before the insert.♻️ Proposed guard
const inserted = this.#transaction.insert(jobRuns).values(run).returning().get(); const record = parseRun(requiredRow(inserted, "manual run insert")); + if (input.queuedEvent.jobRunId !== record.id) { + throw new TypeError("Queued run event must reference the inserted run"); + } this.#insertSuppliedEvent(input.queuedEvent);🤖 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 `@greenfield/src/server/domains/jobs/repository.ts` around lines 972 - 996, In enqueueManualRun, validate that input.queuedEvent.jobRunId matches the newly inserted run.id before calling `#insertSuppliedEvent`; reject the operation on mismatch so the supplied event can only be written to the run created by this method.
1064-1070: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDo not return a conflict result after intent rows are written.
Lines 1023-1045 insert or close disable-intent rows before this guarded update. If the guarded update returns no row, this branch returns
version-changed, and the caller treats it as a normal conflict. The transaction still commits the intent writes. The immediate-transaction guard at line 1003 makes this branch unreachable today, so this is defensive hardening only. Throw here instead, so the transaction rolls back if the invariant ever breaks.♻️ Proposed change
if (row === undefined) { - const observed = requiredRow( - this.#findScheduleRecord(input.id), - "schedule conflict read" - ); - return { kind: "version-changed", schedule: observed }; + throw new Error("Jobs repository schedule update lost its version guard"); }🤖 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 `@greenfield/src/server/domains/jobs/repository.ts` around lines 1064 - 1070, In the guarded-update handling within the repository method containing the disable-intent writes, replace the row === undefined branch’s version-changed return with an exception. Preserve the existing schedule lookup only if needed for the thrown error, and ensure the exception causes the transaction to roll back rather than committing intent rows as a normal conflict.
2154-2157: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unnecessary
as unknown ascast ondatabase.transaction.
drizzle-orm@1.0.0-rc.4accepts an optional config argument for the Bun SQLitetransactionAPI, so bind a typed wrapper instead. This keeps the callback and{ behavior }option type-checked.🤖 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 `@greenfield/src/server/domains/jobs/repository.ts` around lines 2154 - 2157, Update the runTransaction definition around database.transaction to remove the as unknown as cast and create a typed wrapper that binds database.transaction while preserving the generic callback and optional { behavior: "deferred" | "immediate" } configuration typing.greenfield/src/server/domains/jobs/workerSystem.test.ts (1)
19-30: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExit early when the run reaches a non-success terminal state.
waitForTerminalonly stops on"succeeded"or on the 2000 ms deadline. If the coordinator settles the run as"failed","cancelled", or"timed-out", the loop keeps polling a state that can no longer change. The test then spends the full deadline before it reports the failure.Break out as soon as the state is terminal.
♻️ Proposed refactor for a fast, explicit failure
+const terminalStates = new Set(["cancelled", "failed", "succeeded", "timed-out"]); + async function waitForTerminal( readState: () => string | undefined ): Promise<string | undefined> { const deadline = Date.now() + 2000; let state = readState(); - while (state !== "succeeded") { + while (state === undefined || !terminalStates.has(state)) { if (Date.now() >= deadline) return state; await Bun.sleep(2); state = readState(); } return state; }🤖 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 `@greenfield/src/server/domains/jobs/workerSystem.test.ts` around lines 19 - 30, Update waitForTerminal so its polling loop exits immediately for any terminal state, including failed, cancelled, and timed-out, while retaining the existing deadline handling for undefined or non-terminal states and returning the observed state.greenfield/src/server/domains/jobs/coordinator.test.ts (1)
284-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the forced-shutdown path.
No test passes a
forceSignaltodispose, andcoordinatorOptionsomitsforceDrainMs. As a resultwaitForActiveExecutionalways takes theforceSignal === undefinedbranch in coordinator.ts Line 448.The forced branch on coordinator.ts Lines 452-474 contains the escalation logic and the forced-drain timeout. That logic is the reason
jobWorkerForceDrainMsexists, and it is currently untested.Add a test that runs a non-cancellable action, calls
dispose(controller.signal), and aborts the controller. Set a smallforceDrainMsso the test stays fast.Also applies to: 605-641
🤖 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 `@greenfield/src/server/domains/jobs/coordinator.test.ts` around lines 284 - 292, Add coverage in the coordinator disposal tests for the forced-shutdown path: configure a small forceDrainMs in coordinatorOptions, run a non-cancellable action, invoke dispose with an AbortController signal, and abort the controller to exercise waitForActiveExecution’s forceSignal branch and timeout escalation. Assert the forced-drain behavior completes as expected while keeping existing graceful-disposal coverage unchanged.greenfield/scripts/documentation/jsonSchema.ts (1)
680-690: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueDocument cron aliases in the generated schema.
normalizeScheduleCronExpressionruns beforescheduleCronExpressionIsValid, so inputs likeJANandMONare accepted and normalized before the published regex pattern checks. Add a$commentnoting the monthly and weekday alias forms, or avoid applying this transform rule when the schema should document only accepted raw input.🤖 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 `@greenfield/scripts/documentation/jsonSchema.ts` around lines 680 - 690, Update the schema generation branch for normalizeScheduleCronExpression so the generated schedule cron schema documents the accepted JAN–DEC monthly and SUN–SAT weekday aliases via a $comment alongside the normalized regex pattern. Preserve the existing transform handling for the other operations.greenfield/src/server/domains/jobs/coordinator.ts (1)
718-720: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the explicit
try/catchform inheartbeatLoopandscheduleLoop.
claimLoopalready usesEffect.tryPromise({ catch: error => error, try: ... }), while the other two use the single-argument form. With Effect 4.0.0-beta.104, the single-argument form wraps Promise rejections in Effect’s error boundary, socompletioncan reject with different shaped failures depending on the loop. Keep all three loops using the object form so rejects preserve the original error.🤖 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 `@greenfield/src/server/domains/jobs/coordinator.ts` around lines 718 - 720, The heartbeatLoop and scheduleLoop definitions use the single-argument Effect.tryPromise form, unlike claimLoop, causing rejected promises to receive inconsistent error wrapping. Update both loops to use the explicit object form with try invoking the existing tracked pass and catch returning the original error, preserving the current loop behavior.greenfield/src/server/domains/jobs/workerRuntime.ts (2)
170-173: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the factory with the
DashboardWorkerRuntimereturn type.
createDashboardWorkerRuntimereturns an inferred shape.greenfield/src/app/worker.tsconsumes it through theDashboardWorkerRuntimeinterface ingreenfield/src/worker/runtime.ts. An explicit return type documents that contract and reports drift at the definition instead of at the call site.🤖 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 `@greenfield/src/server/domains/jobs/workerRuntime.ts` around lines 170 - 173, Annotate the createDashboardWorkerRuntime factory with an explicit DashboardWorkerRuntime return type, importing or referencing the existing interface from the worker runtime module as appropriate. Preserve the current implementation and dependency handling while ensuring the factory is checked against that contract.
129-144: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReplace the substring test with an explicit action mapping.
input.action.includes("enqueue")infers the realtimeoperationfrom an action name. Any future action name that containsenqueuebut does not create a run emitscreated, and realtime consumers then insert instead of update. An explicit set of creating actions keeps the contract stable.♻️ Proposed refactor
+const runCreatingActions: ReadonlySet<string> = new Set([ + "jobs.run.enqueue-scheduled", +]); + export function createSystemJobWorkerSideEffects(realtime: { id: input.targetId, kind: "run", - operation: input.action.includes("enqueue") ? "created" : "updated", + operation: runCreatingActions.has(input.action) + ? "created" + : "updated", },🤖 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 `@greenfield/src/server/domains/jobs/workerRuntime.ts` around lines 129 - 144, Update forRun in createJobMutationSideEffects to derive realtime.operation from an explicit set of known run-creating actions, returning "created" only for those actions and "updated" for all others. Remove the input.action.includes("enqueue") substring check and preserve the existing realtime payload structure.greenfield/src/app/worker.ts (1)
67-79: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftThe default runtime factory drops the structured logger.
createRuntimereceivesloggerbutcreateDashboardWorkerRuntimeaccepts no logger. The durable loop therefore emits no structured records for claims, retries, lease loss, or drain. Only the process-levelruntime.started,runtime.stopped, andruntime.start_failedrecords exist. Consider passing the logger into the runtime options so worker execution is observable in production.🤖 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 `@greenfield/src/app/worker.ts` around lines 67 - 79, The createRuntime factory currently ignores its logger when constructing the dashboard worker runtime. Update createRuntime and the createDashboardWorkerRuntime options to accept and pass through the structured logger, preserving it for durable-loop claim, retry, lease-loss, and drain records.
🤖 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 `@greenfield/src/contracts/jobModel.ts`:
- Around line 494-504: Update the validation logic around hasScheduleIdentity so
manual runs are not required to have scheduledJobId and scheduledJobVersion.
Keep schedule runs requiring both schedule identity fields and scheduledForAtMs,
while preserving the existing consistency validation for schedule-backed data.
In `@greenfield/src/contracts/jobRealtime.ts`:
- Around line 101-125: Update each schema in jobRealtimeChangeSchemas to add a
cross-field validation requiring payload.id to equal entityId, while preserving
the existing field validations. In greenfield/src/contracts/jobRealtime.test.ts
lines 96-135, add negative tests covering mismatched entityId and payload.id
values for each change type.
In `@greenfield/src/contracts/jobs.ts`:
- Around line 180-191: Update listRuns to read repository.listRuns(input) and
repository.readQueueState() within the same database transaction, ensuring the
queue summary is validated against a consistent snapshot. Preserve
jobQueueSummaryIsConsistent and jobQueueSummarySchema validation, and use the
repository’s existing transaction mechanism rather than disabling validation.
In `@greenfield/src/server/database/schema/resourceLeases.ts`:
- Line 23: Implement an expiry reaper for resource_leases that removes rows
whose lease expiration has passed, and clear the corresponding lease fields in
job_runs so retry-safe jobs can be claimed again. Reuse the existing resource
lease and job run schema symbols and ensure cleanup is scoped to expired
orphaned leases without affecting active leases.
In `@greenfield/src/server/domains/jobs/coordinator.ts`:
- Around line 524-536: Validate and parse run.payloadJson before registering
lifecycleAbort or creating the timeout in the coordinator flow. Catch
parseJsonText or v.parse failures, settle the run as a non-retryable closed
failure using the existing unknown-action handling path around the action
lookup, and return without propagating the error. Pass the validated parsed
payload into registration.execute instead of parsing it there, while preserving
normal cleanup for valid payloads.
- Around line 922-933: Update the initialization-failure path in dispose,
specifically the catch around initializePromise, to call rejectCompletion with
the normalized coordinator failure before rethrowing it. Preserve the existing
abort behavior and ensure completion is rejected when initialization fails,
including failures from registerWorker or reconcileSchedules.
- Around line 469-474: Update the forced-drain race around execution to create
an AbortController, pass its signal to waitFor(forceDrainMs), and abort it in a
finally block after Promise.race settles. Preserve the timeout error when the
drain timer wins and ensure the timer is cancelled when execution completes
first.
In `@greenfield/src/server/domains/jobs/routes.ts`:
- Around line 29-56: Update runJobEffect so failures from Effect.runPromise are
unwrapped before checking JobNotFoundError, JobConflictError, and
JobValidationError, or move the mappings into the Effect pipeline using
catchTag/cause handling. Preserve the existing tRPC mappings to NOT_FOUND,
CONFLICT, and BAD_REQUEST, and rethrow unrelated failures.
---
Nitpick comments:
In `@greenfield/scripts/documentation/jsonSchema.ts`:
- Around line 680-690: Update the schema generation branch for
normalizeScheduleCronExpression so the generated schedule cron schema documents
the accepted JAN–DEC monthly and SUN–SAT weekday aliases via a $comment
alongside the normalized regex pattern. Preserve the existing transform handling
for the other operations.
In `@greenfield/src/app/worker.ts`:
- Around line 67-79: The createRuntime factory currently ignores its logger when
constructing the dashboard worker runtime. Update createRuntime and the
createDashboardWorkerRuntime options to accept and pass through the structured
logger, preserving it for durable-loop claim, retry, lease-loss, and drain
records.
In `@greenfield/src/contracts/jobModel.test.ts`:
- Around line 217-264: Add accepted test cases in “validates worker and schedule
projections across state boundaries” for both “draining” and “stopped” worker
states, using valid field combinations and asserting successful parsing. Keep
the existing negative boundary cases, including invalid stopped projections, so
the suite verifies non-online states are accepted without weakening validation.
In `@greenfield/src/contracts/jobModel.ts`:
- Around line 505-512: In the validation logic around hasStarted, parenthesize
the relational comparison in the mixed expression so it explicitly reads as
hasStarted compared with (run.attemptCount > 0), matching the neighboring
comparison’s shape without changing behavior.
In `@greenfield/src/contracts/jobs.ts`:
- Around line 290-310: Extract the shared job-domain constants jobReadAccess,
jobSessionWriteAccess, queryTransport, and mutationTransport from
greenfield/src/contracts/jobs.ts lines 290-310 into a shared module such as
./jobAccess.ts, then import and reuse them in jobs.ts. In
greenfield/src/contracts/schedules.ts lines 181-206, replace the duplicate
access and transport definitions with imports from that module, while keeping
scheduleRunAccess local because its principalKinds behavior differs.
In `@greenfield/src/server/database/migrations/jobsSchema.test.ts`:
- Around line 265-275: Keep the required query interpolation in
expectUsesIndexWithoutTemporarySort, but update the SQL statements containing
embedded identifiers at the two flagged call sites to bind those values as
parameters. Preserve the existing query plans and assertions while removing the
interpolated identifier pattern that triggers static analysis.
In `@greenfield/src/server/database/schema/jobChecks.ts`:
- Around line 57-81: Wrap the complete SQL expressions returned by
optionalJobMessageCheck, optionalJobTerminalCodeCheck, and
optionalBoundedJobTextCheck in parentheses, matching jobActorCheck so future AND
composition preserves the intended OR grouping. Regenerate the migration file
and update checksums in databaseMigrationManifest.ts to reflect the emitted SQL
changes.
In `@greenfield/src/server/database/schema/jobRuns.ts`:
- Around line 121-124: Update the job_runs_event_budget_check definition to
import and interpolate jobRunEventMaximum, jobRunPayloadEventMaximum, and
jobPayloadEventByteMaximum from the jobs repository instead of using the
literals 1000, 967, and 1048576; preserve the existing event-count and payload
constraints.
In `@greenfield/src/server/database/schema/scheduledJobs.ts`:
- Around line 96-99: Update scheduled_jobs_time_zone_check so database
validation matches scheduleTimeZoneSchema rather than only checking length and
NUL characters. Enforce the canonical accepted time-zone format for direct SQL
writes, reject values such as US/Eastern, GMT, +01:00, and local, and preserve
the existing allowance for NULL.
In `@greenfield/src/server/database/schema/workerInstances.ts`:
- Around line 28-31: Update the worker_instances release_id constraint to reuse
the existing lowercaseHexTextCheck helper, matching the usage for enqueue_sha256
in jobRuns.ts, instead of duplicating the lowercase-hex predicate inline. Verify
the helper emits equivalent SQL, then regenerate the migration and manifest
checksums.
In `@greenfield/src/server/database/validation/jobDisableIntents.ts`:
- Around line 22-28: Extract the shared actor-identity validation logic from
actorIsValid in jobDisableIntents.ts and the equivalent helper in jobRuns.ts
into a common validation helper, then update both callers to reuse it.
Incorporate the compatible automation, system, and user handling from
controlActorIsValid without changing the existing validation schemas or
behavior.
In `@greenfield/src/server/database/validation/jobRuns.ts`:
- Around line 156-161: In the validation condition, update the comparison
involving run.attemptCount so the relational check is explicitly parenthesized
as (run.attemptCount > 0), matching the existing parentheses around
run.lastAttemptStartedAt and preserving behavior.
In `@greenfield/src/server/database/validation/jobWorkerControl.ts`:
- Around line 71-77: Replace the inline validation pipeline for updatedAt with
the shared nonnegativeDateSchema helper, passing v.date() as used by
jobDisableIntents. Preserve the existing field validation while centralizing the
nonnegative timestamp rule and error message.
In `@greenfield/src/server/database/validation/rowSchemas.test.ts`:
- Around line 540-601: Replace every bare toThrow() in
greenfield/src/server/database/validation/rowSchemas.test.ts lines 540-601 with
the specific expected Valibot message for its validation case, including the
canonical resource-keys and invalid event-count messages. Update the event-count
rejection in greenfield/src/server/database/migrations/jobsSchema.test.ts lines
781-789 with its expected constraint message, and update the resource_leases
identity-update rejection in lines 1082-1087 with the expected trigger message,
matching neighboring assertions.
In `@greenfield/src/server/domains/jobs/coordinator.test.ts`:
- Around line 284-292: Add coverage in the coordinator disposal tests for the
forced-shutdown path: configure a small forceDrainMs in coordinatorOptions, run
a non-cancellable action, invoke dispose with an AbortController signal, and
abort the controller to exercise waitForActiveExecution’s forceSignal branch and
timeout escalation. Assert the forced-drain behavior completes as expected while
keeping existing graceful-disposal coverage unchanged.
In `@greenfield/src/server/domains/jobs/coordinator.ts`:
- Around line 718-720: The heartbeatLoop and scheduleLoop definitions use the
single-argument Effect.tryPromise form, unlike claimLoop, causing rejected
promises to receive inconsistent error wrapping. Update both loops to use the
explicit object form with try invoking the existing tracked pass and catch
returning the original error, preserving the current loop behavior.
In `@greenfield/src/server/domains/jobs/repository.ts`:
- Around line 972-996: In enqueueManualRun, validate that
input.queuedEvent.jobRunId matches the newly inserted run.id before calling
`#insertSuppliedEvent`; reject the operation on mismatch so the supplied event can
only be written to the run created by this method.
- Around line 1064-1070: In the guarded-update handling within the repository
method containing the disable-intent writes, replace the row === undefined
branch’s version-changed return with an exception. Preserve the existing
schedule lookup only if needed for the thrown error, and ensure the exception
causes the transaction to roll back rather than committing intent rows as a
normal conflict.
- Around line 2154-2157: Update the runTransaction definition around
database.transaction to remove the as unknown as cast and create a typed wrapper
that binds database.transaction while preserving the generic callback and
optional { behavior: "deferred" | "immediate" } configuration typing.
In `@greenfield/src/server/domains/jobs/service.ts`:
- Around line 766-778: The reconciliation side effect currently identifies only
the first schedule via jobActionRegistrations[0]?.scheduleId. Update the
realtime payload in mutationSideEffects for jobs.schedule.reconcile to represent
all reconciled schedules, either by emitting an entry per schedule or by using a
directory-level identity that clients interpret as refreshing the schedule list;
preserve the successful reconciliation metadata.
- Around line 311-348: Extract the shared durable-row construction from
scheduleInsertShape and coordinator’s scheduleInsert into one reusable builder,
including all schedule columns and nextRunAt calculation. Have the builder
expose the missing-occurrence case without choosing a caller-specific error,
then let scheduleInsertShape map it to JobValidationError and the coordinator
map it to RangeError while preserving each caller’s existing behavior.
In `@greenfield/src/server/domains/jobs/workerRuntime.ts`:
- Around line 170-173: Annotate the createDashboardWorkerRuntime factory with an
explicit DashboardWorkerRuntime return type, importing or referencing the
existing interface from the worker runtime module as appropriate. Preserve the
current implementation and dependency handling while ensuring the factory is
checked against that contract.
- Around line 129-144: Update forRun in createJobMutationSideEffects to derive
realtime.operation from an explicit set of known run-creating actions, returning
"created" only for those actions and "updated" for all others. Remove the
input.action.includes("enqueue") substring check and preserve the existing
realtime payload structure.
In `@greenfield/src/server/domains/jobs/workerSystem.test.ts`:
- Around line 19-30: Update waitForTerminal so its polling loop exits
immediately for any terminal state, including failed, cancelled, and timed-out,
while retaining the existing deadline handling for undefined or non-terminal
states and returning the observed state.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 79e0eae7-c561-4eea-a8c7-371905d76128
⛔ Files ignored due to path filters (31)
greenfield/docs/generated/procedures.mdis excluded by!**/generated/**and included by**/*greenfield/docs/generated/realtime-events.mdis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/automationSecurity.createPrincipal.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/automationSecurity.createPrincipal.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/automationSecurity.disablePrincipal.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/automationSecurity.listPrincipals.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/automationSecurity.replaceCapabilities.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/automationSecurity.replaceCapabilities.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/events.stream.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/events.stream.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/jobs.cancelRun.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/jobs.cancelRun.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/jobs.getRun.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/jobs.getRun.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/jobs.listRuns.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/jobs.listRuns.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/jobs.runs.realtime.payload.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/jobs.setClaimingPaused.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/jobs.setClaimingPaused.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.get.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.get.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.list.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.list.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.listRuns.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.listRuns.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.records.realtime.payload.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.run.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.run.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.update.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.update.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/securityAudit.listEvents.output.schema.jsonis excluded by!**/generated/**and included by**/*
📒 Files selected for processing (85)
greenfield/docs/architecture/greenfield-rewrite/application-architecture.mdgreenfield/docs/architecture/greenfield-rewrite/data-and-security.mdgreenfield/docs/architecture/greenfield-rewrite/progress.mdgreenfield/docs/architecture/greenfield-rewrite/runtime-and-delivery.mdgreenfield/migrations/20260804022252_dashboard-foundation/migration.sqlgreenfield/migrations/20260804022252_dashboard-foundation/snapshot.jsongreenfield/scripts/documentation/artifacts.test.tsgreenfield/scripts/documentation/jsonSchema.test.tsgreenfield/scripts/documentation/jsonSchema.tsgreenfield/scripts/sourceBoundaries/externalAuthorityPolicy.tsgreenfield/scripts/sourceBoundaries/policy.test.tsgreenfield/scripts/sourceBoundaries/sourceTopologyPolicy.tsgreenfield/src/app/dashboardServer.test.tsgreenfield/src/app/dashboardServer.tsgreenfield/src/app/server.tsgreenfield/src/app/trpcHttpHandler.tsgreenfield/src/app/worker.test.tsgreenfield/src/app/worker.tsgreenfield/src/browser/api/trpcClient.test.tsgreenfield/src/browser/api/trpcClient.tsgreenfield/src/contracts/contractRegistry.tsgreenfield/src/contracts/events.test.tsgreenfield/src/contracts/events.tsgreenfield/src/contracts/jobModel.test.tsgreenfield/src/contracts/jobModel.tsgreenfield/src/contracts/jobRealtime.test.tsgreenfield/src/contracts/jobRealtime.tsgreenfield/src/contracts/jobs.test.tsgreenfield/src/contracts/jobs.tsgreenfield/src/contracts/schedules.test.tsgreenfield/src/contracts/schedules.tsgreenfield/src/contracts/security.test.tsgreenfield/src/contracts/security.tsgreenfield/src/server/database/migrations/jobsSchema.test.tsgreenfield/src/server/database/migrations/migrationGraph.test.tsgreenfield/src/server/database/schema/automationPrincipalCapabilities.tsgreenfield/src/server/database/schema/drizzleSchema.tsgreenfield/src/server/database/schema/jobChecks.tsgreenfield/src/server/database/schema/jobDisableIntents.tsgreenfield/src/server/database/schema/jobRunEvents.tsgreenfield/src/server/database/schema/jobRuns.tsgreenfield/src/server/database/schema/jobWorkerControl.tsgreenfield/src/server/database/schema/resourceLeases.tsgreenfield/src/server/database/schema/scheduledJobs.tsgreenfield/src/server/database/schema/workerInstances.tsgreenfield/src/server/database/validation/jobDisableIntents.tsgreenfield/src/server/database/validation/jobRunEvents.tsgreenfield/src/server/database/validation/jobRuns.tsgreenfield/src/server/database/validation/jobWorkerControl.tsgreenfield/src/server/database/validation/resourceLeases.tsgreenfield/src/server/database/validation/rowSchemas.test.tsgreenfield/src/server/database/validation/scheduledJobs.tsgreenfield/src/server/database/validation/workerInstances.tsgreenfield/src/server/domains/jobs/actionRegistry.test.tsgreenfield/src/server/domains/jobs/actionRegistry.tsgreenfield/src/server/domains/jobs/coordinator.test.tsgreenfield/src/server/domains/jobs/coordinator.tsgreenfield/src/server/domains/jobs/errors.tsgreenfield/src/server/domains/jobs/procedures.test.tsgreenfield/src/server/domains/jobs/procedures.tsgreenfield/src/server/domains/jobs/records.tsgreenfield/src/server/domains/jobs/repository.test.tsgreenfield/src/server/domains/jobs/repository.tsgreenfield/src/server/domains/jobs/routes.tsgreenfield/src/server/domains/jobs/scheduleTime.test.tsgreenfield/src/server/domains/jobs/scheduleTime.tsgreenfield/src/server/domains/jobs/service.test.tsgreenfield/src/server/domains/jobs/service.tsgreenfield/src/server/domains/jobs/sideEffects.test.tsgreenfield/src/server/domains/jobs/sideEffects.tsgreenfield/src/server/domains/jobs/testSupport/service.tsgreenfield/src/server/domains/jobs/workerRuntime.test.tsgreenfield/src/server/domains/jobs/workerRuntime.tsgreenfield/src/server/domains/jobs/workerSystem.test.tsgreenfield/src/server/test/support/requestContext.tsgreenfield/src/server/test/system/serverGatewayCredentialVerification.test.tsgreenfield/src/server/trpc/appRouter.tsgreenfield/src/server/trpc/context.test.tsgreenfield/src/server/trpc/context.tsgreenfield/src/server/trpc/procedureErrorPolicy.tsgreenfield/src/shared/databaseMigrationManifest.tsgreenfield/src/test/integration/delivery/productionReleaseLifecycle.test.tsgreenfield/src/test/parity/fixtures/greenfield-contracts.jsongreenfield/src/test/parity/fixtures/legacy-endpoints.jsongreenfield/src/worker/runtime.ts
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 319283756a
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Review follow-up is pushed in cecdfaf, with the final incremental review fixes in af5b7bc.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
greenfield/src/server/database/validation/jobActors.ts (1)
15-24: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
systembranch explicit.The final
returnhandlessystemimplicitly. If thekindunion gains a new member later, the new kind silently validates againstjobActionKeySchemaand passes. An explicit branch with a closingreturn falsefails closed instead.The reuse of
jobActionKeySchemafor system actor ids also reads as accidental. The explicit branch documents that the shape is intentionally the same as the SQLjob_runs_requested_actor_checksystem rule.♻️ Proposed change
if (kind === "user") return v.safeParse(securityRecordIdSchema, id).success; - return v.safeParse(jobActionKeySchema, id).success; + // System actor ids share the bounded lowercase key shape used by action keys. + if (kind === "system") return v.safeParse(jobActionKeySchema, id).success; + return 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 `@greenfield/src/server/database/validation/jobActors.ts` around lines 15 - 24, Update jobActorIdentityIsValid to add an explicit kind === "system" branch that validates with jobActionKeySchema, then replace the implicit final validation with return false so future kind values fail closed.greenfield/src/contracts/jobRealtime.ts (1)
137-159: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the repeated identity-check wrapper.
The same
v.pipe(..., v.check(...))wrapper with the same message repeats three times. A small helper removes the repetition and keeps the message defined once.♻️ Proposed helper
+const identityMessage = "Job realtime entity identity is inconsistent"; + +function withMatchingIdentity< + TSchema extends v.GenericSchema< + unknown, + { readonly entityId: string; readonly payload: { readonly id: string } } + >, +>(schema: TSchema) { + return v.pipe( + schema, + v.check<v.InferOutput<TSchema>, typeof identityMessage>( + jobRealtimeIdentityMatches, + identityMessage + ) + ); +} + /** Topic-specific client change schemas built from the producer routing policy. */ export const jobRealtimeChangeSchemas = [ - v.pipe( - jobRunRealtimeChangeObjectSchema, - v.check< - v.InferOutput<typeof jobRunRealtimeChangeObjectSchema>, - "Job realtime entity identity is inconsistent" - >(jobRealtimeIdentityMatches, "Job realtime entity identity is inconsistent") - ), + withMatchingIdentity(jobRunRealtimeChangeObjectSchema),🤖 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 `@greenfield/src/contracts/jobRealtime.ts` around lines 137 - 159, Extract the repeated v.pipe/v.check identity-validation pattern from jobRealtimeChangeSchemas into a small helper that accepts a realtime change schema and applies jobRealtimeIdentityMatches with the shared inconsistency message. Replace all three inline wrappers with calls to this helper, preserving the existing schema order and validation behavior.greenfield/src/server/domains/jobs/service.test.ts (1)
213-219: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStrengthen the pre-change cursor assertion.
Line 214 asserts that
enabled.nextRunAtMsis not the post-change value. The assertion passes whenenabled.nextRunAtMsisundefined, so it does not prove that the enabled schedule carried the original 60-second cadence cursor. Assert the expected original value instead.🤖 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 `@greenfield/src/server/domains/jobs/service.test.ts` around lines 213 - 219, Update the enabled schedule assertion in the relevant test to verify enabled.nextRunAtMs equals the original 60-second cadence timestamp based on authenticationTestNow, rather than merely asserting it differs from the post-change value; keep the existing repository nextRunAt assertion 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 `@greenfield/src/server/domains/jobs/repository.test.ts`:
- Around line 170-181: Sequence the rollback state assertion after the rejected
enqueue operation has settled: retain the existing non-awaited rejects matcher
for repository.enqueueManualRun, then await a separate promise or otherwise
synchronize on the same operation before calling repository.findRun. Ensure
findRun verifies the post-rollback state rather than observing the database
before the deferred write executes.
---
Nitpick comments:
In `@greenfield/src/contracts/jobRealtime.ts`:
- Around line 137-159: Extract the repeated v.pipe/v.check identity-validation
pattern from jobRealtimeChangeSchemas into a small helper that accepts a
realtime change schema and applies jobRealtimeIdentityMatches with the shared
inconsistency message. Replace all three inline wrappers with calls to this
helper, preserving the existing schema order and validation behavior.
In `@greenfield/src/server/database/validation/jobActors.ts`:
- Around line 15-24: Update jobActorIdentityIsValid to add an explicit kind ===
"system" branch that validates with jobActionKeySchema, then replace the
implicit final validation with return false so future kind values fail closed.
In `@greenfield/src/server/domains/jobs/service.test.ts`:
- Around line 213-219: Update the enabled schedule assertion in the relevant
test to verify enabled.nextRunAtMs equals the original 60-second cadence
timestamp based on authenticationTestNow, rather than merely asserting it
differs from the post-change value; keep the existing repository nextRunAt
assertion unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ca07075d-6b78-4fae-806e-ed3dd7f94d96
⛔ Files ignored due to path filters (5)
greenfield/docs/generated/schemas/events.stream.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.get.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.list.output.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.update.input.schema.jsonis excluded by!**/generated/**and included by**/*greenfield/docs/generated/schemas/schedules.update.output.schema.jsonis excluded by!**/generated/**and included by**/*
📒 Files selected for processing (40)
greenfield/migrations/20260804022252_dashboard-foundation/migration.sqlgreenfield/migrations/20260804022252_dashboard-foundation/snapshot.jsongreenfield/scripts/documentation/artifacts.test.tsgreenfield/scripts/documentation/jsonSchema.test.tsgreenfield/scripts/documentation/jsonSchema.tsgreenfield/src/contracts/events.test.tsgreenfield/src/contracts/events.tsgreenfield/src/contracts/jobModel.test.tsgreenfield/src/contracts/jobModel.tsgreenfield/src/contracts/jobProcedurePolicies.tsgreenfield/src/contracts/jobRealtime.test.tsgreenfield/src/contracts/jobRealtime.tsgreenfield/src/contracts/jobs.tsgreenfield/src/contracts/scheduleTimeZones.tsgreenfield/src/contracts/schedules.tsgreenfield/src/server/database/migrations/jobsSchema.test.tsgreenfield/src/server/database/schema/checks.tsgreenfield/src/server/database/schema/jobChecks.tsgreenfield/src/server/database/schema/jobRuns.tsgreenfield/src/server/database/schema/scheduledJobs.tsgreenfield/src/server/database/schema/workerInstances.tsgreenfield/src/server/database/validation/jobActors.tsgreenfield/src/server/database/validation/jobDisableIntents.tsgreenfield/src/server/database/validation/jobRuns.tsgreenfield/src/server/database/validation/jobWorkerControl.tsgreenfield/src/server/database/validation/rowSchemas.test.tsgreenfield/src/server/database/validation/scalars.tsgreenfield/src/server/domains/jobs/coordinator.test.tsgreenfield/src/server/domains/jobs/coordinator.tsgreenfield/src/server/domains/jobs/registeredSchedule.tsgreenfield/src/server/domains/jobs/repository.test.tsgreenfield/src/server/domains/jobs/repository.tsgreenfield/src/server/domains/jobs/service.test.tsgreenfield/src/server/domains/jobs/service.tsgreenfield/src/server/domains/jobs/workerRuntime.test.tsgreenfield/src/server/domains/jobs/workerRuntime.tsgreenfield/src/server/domains/jobs/workerSystem.test.tsgreenfield/src/shared/databaseMigrationManifest.tsgreenfield/src/shared/workerRuntime.tsgreenfield/src/worker/runtime.ts
🚧 Files skipped from review as they are similar to previous changes (21)
- greenfield/src/shared/databaseMigrationManifest.ts
- greenfield/src/contracts/jobRealtime.test.ts
- greenfield/scripts/documentation/artifacts.test.ts
- greenfield/src/server/database/validation/rowSchemas.test.ts
- greenfield/src/server/database/schema/workerInstances.ts
- greenfield/src/server/database/validation/jobDisableIntents.ts
- greenfield/src/server/database/migrations/jobsSchema.test.ts
- greenfield/src/server/database/schema/jobRuns.ts
- greenfield/src/server/database/validation/jobWorkerControl.ts
- greenfield/src/contracts/schedules.ts
- greenfield/scripts/documentation/jsonSchema.ts
- greenfield/src/server/database/validation/jobRuns.ts
- greenfield/src/contracts/events.ts
- greenfield/src/server/domains/jobs/workerRuntime.ts
- greenfield/src/contracts/jobModel.test.ts
- greenfield/src/server/domains/jobs/service.ts
- greenfield/src/contracts/jobs.ts
- greenfield/src/server/database/schema/jobChecks.ts
- greenfield/src/contracts/jobModel.ts
- greenfield/src/server/domains/jobs/coordinator.ts
- greenfield/src/server/domains/jobs/repository.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: dashboard-checks
- GitHub Check: frontend-checks
- GitHub Check: Analyze JavaScript and TypeScript
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-08-07T18:47:49.639Z
Learnt from: mira-2026
Repo: rajohan/Mira-Dashboard PR: 398
File: greenfield/src/server/domains/monitoring/catalogErrors.ts:3-3
Timestamp: 2026-08-07T18:47:49.639Z
Learning: In the greenfield TypeScript application, use the pinned Effect version 4.0.0-beta.104 API. Preserve `Schema.Literals` for readonly literal tuples and arrays, and do not replace it with variadic `Schema.Literal(...)` unless the replacement has been validated against the pinned Effect version.
Applied to files:
greenfield/src/server/database/schema/checks.tsgreenfield/src/contracts/jobProcedurePolicies.tsgreenfield/src/server/domains/jobs/registeredSchedule.tsgreenfield/src/shared/workerRuntime.tsgreenfield/src/server/database/validation/scalars.tsgreenfield/src/worker/runtime.tsgreenfield/src/contracts/scheduleTimeZones.tsgreenfield/scripts/documentation/jsonSchema.test.tsgreenfield/src/server/domains/jobs/workerSystem.test.tsgreenfield/src/server/database/schema/scheduledJobs.tsgreenfield/src/contracts/events.test.tsgreenfield/src/server/database/validation/jobActors.tsgreenfield/src/server/domains/jobs/repository.test.tsgreenfield/src/server/domains/jobs/workerRuntime.test.tsgreenfield/src/server/domains/jobs/service.test.tsgreenfield/src/server/domains/jobs/coordinator.test.tsgreenfield/src/contracts/jobRealtime.ts
📚 Learning: 2026-08-07T17:05:36.638Z
Learnt from: mira-2026
Repo: rajohan/Mira-Dashboard PR: 397
File: greenfield/src/server/domains/agents/service.test.ts:225-239
Timestamp: 2026-08-07T17:05:36.638Z
Learning: In Bun test files, write rejection assertions as `expect(promise).rejects...` without `await`. The repository's installed matcher types return `void`, and ESLint's `typescript(await-thenable)` rule rejects awaiting these matcher assertions.
Applied to files:
greenfield/scripts/documentation/jsonSchema.test.tsgreenfield/src/server/domains/jobs/workerSystem.test.tsgreenfield/src/contracts/events.test.tsgreenfield/src/server/domains/jobs/repository.test.tsgreenfield/src/server/domains/jobs/workerRuntime.test.tsgreenfield/src/server/domains/jobs/service.test.tsgreenfield/src/server/domains/jobs/coordinator.test.ts
🔇 Additional comments (20)
greenfield/src/server/domains/jobs/workerRuntime.test.ts (1)
116-138: LGTM!greenfield/src/server/domains/jobs/coordinator.test.ts (2)
1-1: LGTM!Also applies to: 145-160, 162-271, 330-375
698-857: LGTM!Also applies to: 890-892, 905-907
greenfield/src/shared/workerRuntime.ts (1)
1-6: LGTM!greenfield/src/worker/runtime.ts (1)
1-1: 🩺 Stability & AvailabilityNo change needed.
greenfield/src/worker/runtime.tsis a type-only re-export, and current importers use type-only imports.greenfield/scripts/documentation/jsonSchema.test.ts (1)
26-27: LGTM!Also applies to: 106-129
greenfield/src/contracts/events.test.ts (2)
8-8: LGTM!Also applies to: 22-22
94-128: 🎯 Functional CorrectnessNo change needed. The matching variants parse successfully in
greenfield/src/contracts/jobRealtime.test.ts, and this test covers only the mismatched entities.greenfield/src/contracts/jobProcedurePolicies.ts (1)
1-28: LGTM!greenfield/src/contracts/scheduleTimeZones.ts (1)
1-453: LGTM!greenfield/src/contracts/jobRealtime.ts (1)
100-109: LGTM!greenfield/migrations/20260804022252_dashboard-foundation/migration.sql (2)
616-634: LGTM!Also applies to: 636-705, 740-782, 784-800
929-951: 🗄️ Data Integrity & IntegrationNo change needed. The
job_disable_intents_active_schedule_uniqueindex and the single-intent update path prevent multiple non-ended intents for one scheduled job, so the replacement exemption cannot match multiple active intents.greenfield/src/server/database/schema/checks.ts (1)
23-26: LGTM!greenfield/src/server/database/schema/scheduledJobs.ts (1)
12-16: LGTM!Also applies to: 103-106
greenfield/src/server/database/validation/scalars.ts (1)
53-57: LGTM!greenfield/src/server/domains/jobs/workerSystem.test.ts (1)
18-31: LGTM!Also applies to: 34-45
greenfield/src/server/domains/jobs/repository.test.ts (1)
450-531: LGTM!greenfield/src/server/domains/jobs/registeredSchedule.ts (1)
11-43: LGTM!greenfield/src/server/domains/jobs/service.test.ts (1)
141-165: LGTM!Also applies to: 225-293, 347-407
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: af5b7bca3f
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@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 (1)
greenfield/src/server/domains/jobs/repository.ts (1)
297-312: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winHandle the new
left-disabledresult before continuing the schedule pass.
expireDisableIntents()can now returnkind: "left-disabled", but the coordinator only branches onnext-occurrence-unavailableand continues tolistDueSchedules. That can still attempt to schedule the same disabled intent again or treat it the same as a successfully re-enabled/disabled-to-expired path. Add an explicitleft-disabledbranch that preserves the disabled schedule state.🤖 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 `@greenfield/src/server/domains/jobs/repository.ts` around lines 297 - 312, The coordinator handling expireDisableIntents() must explicitly handle the "left-disabled" result before continuing to listDueSchedules. Add a branch keyed on ExpireDisableIntentResult.kind that preserves the returned disabled schedule state and prevents it from being processed as re-enabled or passed through the normal scheduling path.
🧹 Nitpick comments (4)
greenfield/src/server/domains/jobs/service.ts (2)
543-545: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueRead only the worker control row here.
readQueueStateruns the state-count aggregate, the distinct resource-class query, the worker query, and the active-count query. This path needs onlycontrol. Add a narrowreadWorkerControlreader to the repository and call it here. The pause and resume mutations then avoid four unnecessary queries.🤖 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 `@greenfield/src/server/domains/jobs/service.ts` around lines 543 - 545, Replace the readQueueState call in the job service flow with a narrow repository.readWorkerControl reader that accepts the existing minimumHeartbeatAt filter and returns only the control row. Implement readWorkerControl in the repository using only the worker control query, then preserve the existing .control consumers and pause/resume behavior.
441-450: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSeparate the registry check from the exposure check.
Lines 441-444 collapse two distinct causes into one reason. A schedule that is no longer in the code-owned registry now reports
action-not-manually-exposed.updateSchedulereportsaction-unavailablefor the same registry condition at Line 583. Split the branches so both procedures report the same reason for the same cause.♻️ Proposed change
- if ( - !isRegisteredJobSchedule(schedule.id, schedule.actionKey) || - registration?.manualExposure !== "jobs-write" - ) { + if (!isRegisteredJobSchedule(schedule.id, schedule.actionKey)) { + throw new JobConflictError({ + id: input.id, + reason: "action-unavailable", + resource: "schedule", + }); + } + if (registration?.manualExposure !== "jobs-write") { throw new JobConflictError({ id: input.id, reason: "action-not-manually-exposed", resource: "schedule", }); }Note: this changes an observable error reason. Update
greenfield/src/server/domains/jobs/service.test.tsLines 259-266 if you apply 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 `@greenfield/src/server/domains/jobs/service.ts` around lines 441 - 450, Split the combined condition in the schedule validation around isRegisteredJobSchedule into separate branches: throw the registry-specific action-unavailable reason when the schedule is not registered, and retain action-not-manually-exposed only for registered schedules whose manualExposure is not jobs-write. Update the corresponding service test expectation in service.test.ts.greenfield/src/server/domains/jobs/repository.test.ts (1)
970-978: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive the stale-worker count from the summary bound.
The loop registers 33 stale workers. That number only proves "filter before limit" while
workerSummaryMaximumstays at 32. If the constant changes, the test still passes but no longer exercises the bound. Import the constant and size the loop from it, so the intent survives a limit change.🤖 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 `@greenfield/src/server/domains/jobs/repository.test.ts` around lines 970 - 978, Update the stale-worker registration loop around workerSummaryMaximum so its iteration count is derived from the imported summary limit rather than the hardcoded 33-worker range. Import and use workerSummaryMaximum, registering one more stale worker than the limit to preserve the filter-before-limit assertion when the bound changes.greenfield/src/server/domains/jobs/repository.ts (1)
1514-1550: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared intent-closure block.
Lines 1520-1541 and Lines 1569-1590 perform the same guarded
jobDisableIntentsupdate, the samev.parseclosure payload, and the samerequiredRowhandling. Only the failure label differs. Extract one private helper so the fencing predicate cannot drift between the two expiry paths.♻️ Proposed helper
`#closeExpiredIntent`( intent: JobDisableIntentRecord, transitionAt: Date, systemActorId: string, context: string ): JobDisableIntentRecord { const row = this.#transaction .update(jobDisableIntents) .set( v.parse(jobDisableIntentCloseSchema, { endedAt: transitionAt, endedById: systemActorId, endedByKind: "system", endedReason: "expired", }) ) .where( and( eq(jobDisableIntents.id, intent.id), isNull(jobDisableIntents.endedAt), lte(jobDisableIntents.expiresAt, transitionAt) ) ) .returning() .get(); return parseDisableIntent(requiredRow(row, context)); }Note: keep the original
input.atbound in the predicate if you extract it; pass it explicitly rather than reusingtransitionAt.🤖 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 `@greenfield/src/server/domains/jobs/repository.ts` around lines 1514 - 1550, Extract the duplicated jobDisableIntents closure logic into a private `#closeExpiredIntent` helper, including the parsed closure payload, guarded update, and requiredRow/parseDisableIntent handling. Update both expiry paths to call it with their transition time, system actor ID, and distinct failure context; preserve input.at as the fencing predicate bound rather than substituting transitionAt.
🤖 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 `@greenfield/src/server/domains/jobs/repository.test.ts`:
- Around line 258-284: Sequence the assertions in the reconcileSchedules test by
storing its returned promise, asserting rejection with
expect(...).rejects.toThrow() without await, and only then reading
findSchedule("system.worker-smoke"). Follow the established pattern from the
test block around lines 175-186 so the deferred transaction settles before
validating the unchanged schedule state.
---
Outside diff comments:
In `@greenfield/src/server/domains/jobs/repository.ts`:
- Around line 297-312: The coordinator handling expireDisableIntents() must
explicitly handle the "left-disabled" result before continuing to
listDueSchedules. Add a branch keyed on ExpireDisableIntentResult.kind that
preserves the returned disabled schedule state and prevents it from being
processed as re-enabled or passed through the normal scheduling path.
---
Nitpick comments:
In `@greenfield/src/server/domains/jobs/repository.test.ts`:
- Around line 970-978: Update the stale-worker registration loop around
workerSummaryMaximum so its iteration count is derived from the imported summary
limit rather than the hardcoded 33-worker range. Import and use
workerSummaryMaximum, registering one more stale worker than the limit to
preserve the filter-before-limit assertion when the bound changes.
In `@greenfield/src/server/domains/jobs/repository.ts`:
- Around line 1514-1550: Extract the duplicated jobDisableIntents closure logic
into a private `#closeExpiredIntent` helper, including the parsed closure payload,
guarded update, and requiredRow/parseDisableIntent handling. Update both expiry
paths to call it with their transition time, system actor ID, and distinct
failure context; preserve input.at as the fencing predicate bound rather than
substituting transitionAt.
In `@greenfield/src/server/domains/jobs/service.ts`:
- Around line 543-545: Replace the readQueueState call in the job service flow
with a narrow repository.readWorkerControl reader that accepts the existing
minimumHeartbeatAt filter and returns only the control row. Implement
readWorkerControl in the repository using only the worker control query, then
preserve the existing .control consumers and pause/resume behavior.
- Around line 441-450: Split the combined condition in the schedule validation
around isRegisteredJobSchedule into separate branches: throw the
registry-specific action-unavailable reason when the schedule is not registered,
and retain action-not-manually-exposed only for registered schedules whose
manualExposure is not jobs-write. Update the corresponding service test
expectation in service.test.ts.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 97224325-908e-4d30-a88c-ff81284e735f
📒 Files selected for processing (18)
greenfield/src/app/worker.test.tsgreenfield/src/app/worker.tsgreenfield/src/contracts/jobModel.tsgreenfield/src/contracts/jobRealtime.tsgreenfield/src/server/database/validation/jobActors.tsgreenfield/src/server/domains/jobs/actionRegistry.test.tsgreenfield/src/server/domains/jobs/actionRegistry.tsgreenfield/src/server/domains/jobs/coordinator.test.tsgreenfield/src/server/domains/jobs/coordinator.tsgreenfield/src/server/domains/jobs/errors.tsgreenfield/src/server/domains/jobs/repository.test.tsgreenfield/src/server/domains/jobs/repository.tsgreenfield/src/server/domains/jobs/service.test.tsgreenfield/src/server/domains/jobs/service.tsgreenfield/src/server/domains/jobs/sideEffects.test.tsgreenfield/src/server/domains/jobs/sideEffects.tsgreenfield/src/server/domains/jobs/workerRuntime.test.tsgreenfield/src/server/domains/jobs/workerRuntime.ts
🚧 Files skipped from review as they are similar to previous changes (11)
- greenfield/src/server/domains/jobs/actionRegistry.test.ts
- greenfield/src/server/database/validation/jobActors.ts
- greenfield/src/server/domains/jobs/workerRuntime.test.ts
- greenfield/src/server/domains/jobs/errors.ts
- greenfield/src/app/worker.ts
- greenfield/src/contracts/jobModel.ts
- greenfield/src/server/domains/jobs/workerRuntime.ts
- greenfield/src/server/domains/jobs/actionRegistry.ts
- greenfield/src/server/domains/jobs/coordinator.test.ts
- greenfield/src/server/domains/jobs/coordinator.ts
- greenfield/src/contracts/jobRealtime.ts
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Analyze JavaScript and TypeScript
- GitHub Check: frontend-checks
- GitHub Check: dashboard-checks
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2026-08-07T17:05:36.638Z
Learnt from: mira-2026
Repo: rajohan/Mira-Dashboard PR: 397
File: greenfield/src/server/domains/agents/service.test.ts:225-239
Timestamp: 2026-08-07T17:05:36.638Z
Learning: In Bun test files, write rejection assertions as `expect(promise).rejects...` without `await`. The repository's installed matcher types return `void`, and ESLint's `typescript(await-thenable)` rule rejects awaiting these matcher assertions.
Applied to files:
greenfield/src/server/domains/jobs/service.test.tsgreenfield/src/server/domains/jobs/sideEffects.test.tsgreenfield/src/app/worker.test.tsgreenfield/src/server/domains/jobs/repository.test.ts
📚 Learning: 2026-08-07T18:47:49.639Z
Learnt from: mira-2026
Repo: rajohan/Mira-Dashboard PR: 398
File: greenfield/src/server/domains/monitoring/catalogErrors.ts:3-3
Timestamp: 2026-08-07T18:47:49.639Z
Learning: In the greenfield TypeScript application, use the pinned Effect version 4.0.0-beta.104 API. Preserve `Schema.Literals` for readonly literal tuples and arrays, and do not replace it with variadic `Schema.Literal(...)` unless the replacement has been validated against the pinned Effect version.
Applied to files:
greenfield/src/server/domains/jobs/service.test.tsgreenfield/src/server/domains/jobs/sideEffects.test.tsgreenfield/src/server/domains/jobs/sideEffects.tsgreenfield/src/app/worker.test.tsgreenfield/src/server/domains/jobs/repository.test.tsgreenfield/src/server/domains/jobs/service.tsgreenfield/src/server/domains/jobs/repository.ts
🔇 Additional comments (25)
greenfield/src/app/worker.test.ts (4)
45-58: LGTM!
84-123: LGTM!
152-159: LGTM!
233-255: LGTM!greenfield/src/server/domains/jobs/repository.test.ts (5)
175-186: LGTM!
285-349: LGTM!
351-443: LGTM!
854-886: LGTM!Also applies to: 1054-1100
145-165: LGTM!Also applies to: 507-507, 560-562, 584-584, 606-606, 635-635, 644-644, 682-684, 710-710
greenfield/src/server/domains/jobs/repository.ts (6)
796-808: LGTM!
1083-1086: LGTM!
1920-1937: LGTM!
2340-2349: LGTM!
705-712: LGTM!
1014-1042: 🩺 Stability & AvailabilityNo change needed.
Queued runs with an unregistered action key are settled by the worker with the
failedoutcome and terminal codeaction-unavailable.greenfield/src/server/domains/jobs/service.test.ts (3)
406-442: LGTM!Also applies to: 461-477, 497-503
5-5: LGTM!Also applies to: 14-14, 215-216
245-278: 🎯 Functional CorrectnessNo change needed.
registeredcomes fromsystem.worker-smoke, whose default schedule is disabled; the clonedenabledvalue is not a hidden dependency here.> Likely an incorrect or invalid review comment.greenfield/src/server/domains/jobs/service.ts (3)
198-206: LGTM!Also applies to: 298-300, 419-424
575-586: LGTM!Also applies to: 676-677
789-806: LGTM!greenfield/src/server/domains/jobs/sideEffects.test.ts (1)
3-6: LGTM!Also applies to: 78-99
greenfield/src/server/domains/jobs/sideEffects.ts (3)
52-57: LGTM!
98-117: LGTM!
140-149: LGTM!
|
Addressed the complete CodeRabbit review in 9a3ccd9: explicit left-disabled validation and same-pass skip, a narrow worker-control read, distinct action-unavailable mapping, the canonical worker-summary bound, one fenced intent-closure helper, and settled rollback promises before persisted-state assertions. Final local gate: 61 jobs/worker tests, both typechecks, lint, format, docs, db check, and source boundaries all green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9a3ccd9833
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@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: 5b54fd8bf4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@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: 064e0cfa48
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@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: 4c4be994a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@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: 3b31df4fd5
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
2 participants
Summary
system.worker-smokeactionBehavior and regression coverage
jobs.getRunreads run state and its event page from one SQLite snapshot, and worker-produced audit/realtime timestamps use the repository's effective clamped transition timesystem.worker-smoke, observes the separate bundled worker complete it, and proves web-first shutdown with exit code 0 for both processes and no forced stopVerification
cd greenfield && bun run lintcd greenfield && bun run format:checkcd greenfield && bun run typecheckcd greenfield && bun run check:boundariescd greenfield && bun run db:checkcd greenfield && bun run docs:checkRisk checklist
.envfiles, database dumps, or runtime state committedjobs:read/jobs:write, manual action exposure, and mutation error policies were reviewed at contract and procedure boundariesDeployment / operations
Notes for reviewers
openClawCron.*, the/jobsbrowser route, cache/metrics/overview, privileged actions, and Gateway-backed work remain deliberately deferred1dc765dfc9753cadfb3cd246a4daa7ae02c3fee3