[Refactor] Rebuild the work-execution data model: tasks, runs, and a first-class initiator - #45
Merged
Merged
Conversation
…rompts The ROOMOTE_APP_URL deploy-screen prompt is now configured on both published templates (verified on the live deploy screens). Corrections from doing it: Railway does have a template Duplicate action, and the prompt surfaces inside the api service's Configure step rather than as a standalone top-level input.
…y spec Both published templates already ship these; the spec and operator guide now match. SETUP_TOKEN is not optional anymore: the app refuses tokenless first-admin bootstrap outside local development, so the template must generate it. ROOMOTE_PING_BASE_URL points anonymous-analytics and version-check pings at the openmote ping service.
Stage 1 of the data-model simplification (see design doc). - Job tokens: user claim (r.u) now optional; absent = deployment service principal. JobTokenContext carries principal: 'user' | 'deployment' and a nullable userId. - Delete resolveUserIdForCloudJob and every first-user/placeholder-owner forge: scheduled scans (suggester, triage runners, merged-PR auditors, CI-failure webhook) now enqueue with userId null as explicit automation launches. - enqueueCloudTask: identity-less launches allowed for automation/maintenance launch classes (interim until the Stage 2 initiator union). - Token/job principal match: (cloudJob.userId ?? null) === token.userId; null == null is the valid automation case. - MCP proxies: deployment-principal jobs use deployment-scoped mcp_connections rows; human-required MCPs 403 naming the service principal; no more borrowed human credentials. - Thread nullability honestly through sdk/api/web/worker; update tests that asserted forged-owner behavior to the new principal semantics.
…ator stamp Stage 2 of the data-model simplification (see design doc). - tasks is now the durable unit of work: absorbs conversation cargo from cloud_jobs (channel bindings, title, prompts, requestedWorkKind, harnessInstructions, draftPrompt), gains workflow x surface x trigger x visibility x state classification, soft delete, and the immutable five-column initiator stamp (CHECK-enforced: user or automation, external actor context preserved). - cloud_jobs becomes task_runs: one sandbox execution attempt with a real FK to tasks, kind fresh|resume, sourceRunId, actingUserId as the only user column, payloadKind for runtime dispatch. Resume is a new run row; the four chain-walkers and cross-run field copying are gone. - 45 attribution columns collapse to 11: the initiator stamp plus a 5-column commit-author block (with an 'external' kind so unlinked GitHub authors keep noreply-email commit identity), evaluated unconditionally at enqueue. - enqueueCloudTask takes an explicit initiator union; all 47 call sites pass their real initiator (webhook reviews = automation with actor context, mentions = the human, scans = the automation, resumes never re-attribute). attributionOverride, inferSourceKind payload sniffing, resumePromptUserId, and the 732-line display cascade are deleted. - PR linkage writes at enqueue into extended task_pull_requests; PR-review dedup queries move to tasks JOIN task_pull_requests; Redis queue scope is explicit. - Read paths: task list/search/analytics/filters/manager-stats are single-table initiator reads; visibility is a column; task type filters use workflow; owner-scoped authz becomes deployment membership (fixes automation tasks being invisible/un-editable); deletion is soft delete. - CloudTaskType, deletedTasks, evalRuns, usage-table userId copies, and task-attribution.ts deleted. Satellites re-keyed to runId with real FKs.
…d legacy settings Stage 3 of the data-model simplification (see design doc). - One automations table (key text PK, targets as jsonb) replaces backgroundAutomations + backgroundAutomationTargets; rows seeded idempotently; tasks.initiatorAutomation is now a real FK to it. - backgroundAutomationRuns deleted: run history for task-launching automations is tasks WHERE initiator_automation = key; announcer and manager-stats history comes from the automations row's last* columns; dispatch failures land in lastError. - Manual Run-now is synchronous: the web tRPC command invokes the shared runner directly and returns the launched task id (or skip/error reason); apps/web's direct BullMQ producer coupling is deleted. - backgroundAgentSettings slimmed to genuinely global survivors; the ~20 write-dead persona columns, the ~1,250-line legacy normalize/merge layer, and the manager-channel triple fallback are deleted. Channel resolution is two levels: automation target, then manager channel. - One canonical snake_case automation key everywhere, including BullMQ job names (one-time scheduler cleanup for 12 retired names); the four-identifier regex conversion layers are deleted; coach is deleted end to end. - Runners relocated to packages/sdk/src/server/automations and shared by the scheduler and the synchronous trigger path. - Stage 2 follow-up included: worker consumes DequeuedTaskContext (requestedWorkKind + channel bindings) from dequeue/resume responses, and .agent-guidance docs updated for the Stage 1-2 model.
Stage 4 of the data-model simplification (see design doc). - work_items replaces taskSuggestions + automationWorkItems + setupNewQueuedTasks: one shape with a kind discriminator and ONE launch state machine (open/launching/launched/failed/dismissed, stale-claim recovery preserved). Every launchable surface now records launchedTaskId, fixing the dropped suggestion-to-task link for web launches. MCP recommendations get real work_items rows. - tracked_messages replaces agentSuggestionMessages + backgroundAutomationSlackThreads + mcpSetupManagerNotifications: a pure outbound-message registry with UNIQUE(kind, dedupeKey); launch state lives on work_items; the reaction-launch claim CAS moves to work_items. - fastAgentSessions renamed slack_quick_answers, deliberately outside the task spine; the automation-thread feedback loop keeps working against tracked_messages.
…ead-code sweep Stage 5 of the data-model simplification (see design doc). - All prior migrations collapse into a single baseline 0000 generated from the rebuilt schema; fresh-database apply verified on a scratch DB; schema and baseline are in sync (db:generate produces no diff). - 20 .agent-guidance docs updated to the new model (spine, automations, work_items, tracked_messages, queue naming); public docs corrected where the old forged-owner behavior was described. - Dead mocks and stale references removed; knip clean; full pnpm check green.
Found by driving a fresh database through setup, task launch, automations, history, and analytics in the browser. Four pre-existing bugs (masked until now by leftover GitHub state in dev databases) plus three refactor regressions: - Stamp payload.sourceControlProvider once at enqueue from the workspace's synced repositories (plus dequeue-time fallback and explicit stamps at the setup/onboarding launch sites). Non-GitHub deployments previously fell back to the GitHub default and failed source-control token creation or repo resolution. (pre-existing) - Worker git proxy: stop forwarding content-encoding/content-length/ transfer-encoding; fetch already decodes bodies, so gzip-serving hosts like gitlab.com produced 'incorrect header check' clone failures. (pre-existing) - Setup guard no longer requires a GitHub installation after setup completes; GitLab/Gitea/ADO-only deployments were bounced back to /setup forever. (pre-existing) - Allow graph.microsoft.com avatars in next/image (Teams sign-in). (pre-existing) - Deployment-scoped reads (repositories, MCP enablement) accept deployment-service-principal job tokens; automation runs failed workspace prep with 'Invalid authorization token'. (regression) - Suggestion posting no longer requires a human poster: automation scans with no user now post to Slack/Telegram/Teams, and suggestion work_items carry the initiating automationKey. (regression) - Every dequeue/enqueue-time cancel path now writes tasks.state='canceled', matching finishCloudJob; canceled-before-start tasks no longer stay active forever. (regression) - Suggester eligibility is provider-agnostic (any active repository) instead of requiring a GitHub App installation. (pre-existing)
|
No new code issues found. See task
|
…item claim lifecycle, delivery fallback, provider resolver Fixes the 15 findings from the max-effort review of this PR: - One shared task-state derivation (syncTaskStateFromRuns): any non-terminal run keeps the task active; otherwise the latest run that made progress wins. All six writers route through it (finishCloudJob, both dequeue-time cancels, queue supersede-eviction, enqueueSnapshotResume, sleep-check non-resumable shutdown, snapshot completion, and both user-facing direct cancels). Fixes: tasks stuck active after non-resumable shutdown or direct cancel; live resumes reported completed; failed-bootstrap resumes clobbering completed tasks; last-finisher-wins across siblings. The supersede-eviction also gains a status guard and skips when the Redis eviction lost the race. - work_items unique index now includes kind (regenerated baseline migration), fixing the guaranteed onboarding-queue collision with suggestion rows. One shared claim/release/finalize helper with a single stale window replaces the four hand-rolled CAS copies; stale 'launching' rows are recoverable everywhere, releases never revert a launched item, and claims are guarded on launched_task_id. - isVisibleTask() now also requires deletedAt IS NULL, closing the API/MCP read paths that served soft-deleted tasks; soft delete also removes the task's artifact rows in the same transaction. - Suggestion delivery precedence is now keyed on actual delivery: Slack returns a delivered boolean and Telegram/Teams fire only when the prior surface did not deliver, instead of self-suppressing on installation existence (which silently dropped suggestions). - One shared workspace provider resolver (repository/set/environment/ all_repositories) feeds the enqueue stamp, the dequeue fallback, and the web helpers, closing the Linear environment/all-repositories hole on non-GitHub deployments. - Automation tasks regain the env-var auto-stop RPC via deployment-principal sandbox tokens; manager-stats automation labels use the shared formatAutomationLabel.
Contributor
Author
Max-effort review completed; all 15 findings fixed in 8e714a1A 10-angle review (line-by-line, removed-behavior, cross-file tracing, language pitfalls, state machines, reuse, simplification, efficiency, altitude, conventions) with adversarial verification surfaced 15 findings (14 confirmed, 1 plausible). All are fixed: Task-state consistency family (7 findings) — introduced Work-items lifecycle (3 findings) — the unique index now includes Soft-delete integrity (2 findings) — Delivery + provider + misc (3 findings) — suggestion fallback precedence now keys on actual delivery (Slack returns a delivered boolean) instead of installation existence, so suggestions can no longer silently reach no surface; one shared workspace provider resolver closes the environment/all-repositories stamping hole (e.g. Linear launches on GitLab-only deployments) and replaces three divergent copies; automation tasks regain the env-var auto-stop RPC via deployment-principal sandbox tokens; manager-stats automation labels use the shared Full |
Resolves conflicts by porting develop's changes onto the new data-model contract: - setup-new onboarding kickoff adopts develop's web-only Teams fallback (#48) with the new enqueue contract (user initiator, setup_onboarding workflow, provider stamp) instead of attributionOverride - Telegram routing-confirmation launch helper (#22) ported to the new contract, keeping launchClass: 'human' for keepalive policy - snapshot refresh keeps the automation initiator; dropped the re-merged forged-owner fallback - setup redirect guard keeps the provider-agnostic completion check - drizzle baseline regenerated to fold develop's 0001 (nullable environments.created_by_user_id + declarative_source) into the single 0000 baseline - develop-added tests updated to the new enqueue/factory shapes
Finding 1: syncTaskStateFromRuns now takes a `SELECT ... FOR UPDATE` row lock on the owning tasks row before reading the sibling runs, serializing concurrent sibling-run syncs so two runs finishing at once can no longer each miss the other's terminal write and leave the task stuck 'active'. The doc comment records the lock and the must-run-inside-the-writing- transaction invariant. All call sites already pass a transaction `tx`. Finding 2: work-item launch claims are now fenced by the claim's `launchClaimedAt` timestamp. finalizeWorkItemLaunched and releaseWorkItemClaim require a `claimedAt` token and match it in their WHERE guard, so a slow launcher whose stale claim was reclaimed cannot finalize or release onto the new claimant's state. Every caller threads the claimed row's launchClaimedAt through. When finalize loses the guard after a task was already enqueued, the caller logs loudly with the work item id and orphaned task id. Adds fencing test coverage.
The Telegram suggestion button claimed the work item to 'launching' but
never finalized or released it: launched_task_id stayed NULL forever (so
after the 10-minute stale window a second click could relaunch a
suggestion that already produced a task), every launched task ran
unlinked from its work item, and a failed launch left the suggestion
dead for 10 minutes instead of immediately retryable.
claimTelegramSuggestionLaunch now returns the claim's launchClaimedAt
fencing token. The callback handler finalizes with the task id + token
on a successful launch (logging loudly with both ids if the fenced
finalize loses to a reclaim), and releases the claim with the token when
routing replies inline, when a confirmation path defers the launch, or
when the launch throws.
Teams needs no equivalent change: its suggestion surface posts plain
text ("start idea 2") and never claims a work item.
task_pull_requests rows now exist at enqueue with the owning task's workflow, so a PR joined to a pr_review task is one Roomote reviewed and everything else is one Roomote authored (authored wins when both rows exist; bot-authored PRs with no task row count as authored). The digest gains authoredPullRequests/reviewedPullRequests, the Slack headline shows the split, and merged-PR stats are now authored-only (a merged PR Roomote merely reviewed no longer counts as Roomote's merge outcome).
syncTaskStateFromRuns now issues select().for('update'); the mock chain
needs the method or every terminal-status test throws.
Teams posts its suggestion lists as one numbered markdown message and tells the user to reply "start idea 2", but the reply flowed through the generic task-entry path: no work-item claim (the same suggestion could be started twice), no launched_task_id link, and no fenced finalize. Add a conservative whole-message "start idea N" / "idea N" hook in the Teams webhook route. Matches resolve N against the newest tracked suggestion-card group in the conversation (bare conversation-id compare, work_items sort order = posted numbering) and drive the launch through the shared work_items claim state machine like Slack and Telegram: claimWorkItem CAS -> launch -> finalizeWorkItemLaunched with the claim's launchClaimedAt fencing token (loud warn with work item + orphaned task ids on a lost finalize), and releaseWorkItemClaim with the token on the inline-reply and failure paths so the suggestion is retryable immediately. Claim-CAS losses and out-of-range idea numbers get a visible reply; conversations with no tracked suggestion cards fall through to normal task entry unchanged. Runs before the snapshot resume so a suggestion start is never swallowed as a follow-up to the previous task.
…ites Part 1: when a fenced finalizeWorkItemLaunched loses to a reclaim, the already-enqueued run no longer keeps running unlinked. Extract the stop-task direct-cancel transaction into a shared @roomote/db helper (cancelTaskRunDirect: guarded pre-sandbox cancel + syncTaskStateFromRuns + parallel-count close; task-stop.ts now delegates to it) and have all four lost-finalize sites (Slack reactions x2, Telegram callback, Teams suggestion start, web implement) best-effort cancel the orphaned run by its enqueue run id. The cancel wrapper never throws; its outcome is appended to the loud lost-finalize warn either way. Part 2: the automation launch catch path updated work_items by id only, so a stale launcher's failure handling could fail or clear a fresh claimant's row. Both failure writes are now fenced: the terminal-failure stamp requires status='launching' AND our launchClaimedAt claim token, and the retry reopen requires status='launched' AND the task link we just finalized. A non-applying fenced write logs and leaves the current claimant's row untouched. Audited the other claim surfaces: no other raw post-claim work_items writes exist (the setup-new onboarding queue uses a separate non-reclaimable lifecycle, flagged for follow-up).
Ports develop's changes onto the new data model: - #50's enqueue-time provider inference is dropped in favor of our equivalent shared resolveWorkspaceSourceControlProvider (already stamps at enqueue and covers all four workspace shapes); its test coverage is ported to packages/db/src/lib/__tests__/source-control-provider.test.ts - #53's cancel-intent tracking lands on the new model: cancelRequestedAt on task_runs (stamped by cancelTaskRunDirect and the stop paths), failedAfterStopRequest suppression of Slack/Teams/Linear failure notifications and canceled GitHub-facing outcomes in finish-cloud-job, sleep-check finalizing stop-requested sweeps as Canceled - #53's new tests (task-stop, sleep-check, finish-cloud-job suppression) ported from the old cloudJobs/userId shapes to task_runs/actingUserId and the run/task split; develop's two review-summary outcome tests were dropped (no scaffolding for that path in our test file — the suppression mechanism is covered by the conflict/Slack/Teams/Linear cases) - baseline migration regenerated as a single 0000 (folds develop's 0002)
…aims (#56) Co-authored-by: Roomote <roomote@roocode.com>
A lost finalizeWorkItemLaunched means this launcher lost the claim and
its just-enqueued run was orphan-canceled, but every surface still
reported the launch as started and pointed the user at the canceled
orphan. Each surface now reports the claim-lose outcome instead, keeping
the existing loud warn and best-effort cancel:
- Slack reactions: both lost-finalize branches (happy path and
post-enqueue recovery) now mirror the claim-lose path — return
handled, never post the started thread message, and best-effort
delete the seeded root message so no dangling thread points at the
canceled orphan. The started post was already gated behind finalize
in both branches; the recovery branch no longer falls through to it.
- Web implement: throws the same "already been implemented" error the
claim-lose path throws instead of returning success with the orphaned
task ids; the catch-block release is fenced on the stale token so it
cannot touch the winner's claim.
- Teams suggestion start: returns a new 'already_started' outcome
(matching the claim-CAS-lose reply semantics) and posts a corrective
follow-up, since startNewTeamsTask posts its started acknowledgement
before the finalize.
- Telegram callback: posts a corrective reply to the triggering message
("already started elsewhere — duplicate canceled"), since the
callback was already answered "Starting:" and the started post
happens inside startNewTelegramTask, whose contract is shared with
other callers.
…h-logging gaps (#57) Co-authored-by: Roomote <roomote@roocode.com>
The sandbox-oidc refresh sweep's hand-written grouped SKIP LOCKED claim still referenced sandbox_oidc_targets.cloud_job_id after the column was renamed to run_id, so every bullmq refresh tick failed with 42703. Raw SQL is invisible to TypeScript and the sandbox-oidc tests mock db.execute, so no gate caught it. The claim is now extracted into claimDueSandboxOidcTargets with a real-database test that makes Postgres parse the statement, guarding all its identifiers against future drift.
- #60 (PR review notifications for everyone): kept develop's removal of the feature-flag gate, expressed with our task_runs naming - doc frontmatter and model-recommendation conflicts resolved trivially
finishCloudJob computed failedAfterStopRequest only AFTER the terminal
transaction, so a user-stopped task whose sandbox died mid-cancel was
REPORTED as canceled (notifications suppressed, GitHub outcomes mapped
to canceled) but PERSISTED as failed: the run row kept status=failed
with completedAt stamped, and syncTaskStateFromRuns derived
tasks.state='failed' — the canonical read for task history, analytics,
and unfurls showed a deliberate stop as a failure.
Normalize once at the top instead: an incoming Failed on a run with
cancelRequestedAt set becomes Canceled for everything downstream — the
persisted run status (canceledAt stamped, completedAt cleared, which is
the correct shape for a stop), the derived tasks.state, the lifecycle
event ('Cloud job was canceled.'), notifications, and GitHub-facing
outcomes. The sanitized error is still written to the run's error
column for debugging. This deletes the failedAfterStopRequest flag, its
!flag conditions in the Slack/Teams/Linear/conflict gates, and the
local recomputation in cleanupGithubPrReviewArtifacts.
Behavior notes: the stop-normalized run no longer triggers
refreshTaskTitleOnCompletion (Completed/Failed only), matching how
plain cancels always behaved; analytics only capture task_completed, so
nothing miscounts. sleep-check already normalizes at the caller and is
untouched; the controller's Failed-on-spawn-failure path now correctly
persists canceled when a stop was requested mid-spawn.
Tests: the four suppression tests now also assert the persisted run
write (status canceled, canceledAt set, completedAt null, error kept),
plus a new test that a plain Failed without a stop request still
persists failed and notifies.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Rebuilds the work-execution data model around six concepts, each answering exactly one question. There are no production users in this iteration, so this is a clean break: no compatibility shims, no dual writes, and migrations collapse to a fresh baseline
0000.Design doc with full rationale, audit evidence, and per-flow risk analysis: internal artifact (ask Matt).
The model
taskstask_runs(wascloud_jobs)tasks, CHECK-enforcedautomationswork_itemstracked_messagesHighlights
initiator_kind+initiator_user_id+initiator_automation+ external actor context, written once at enqueue from an explicit discriminated union at every launch site. The 45 attribution columns, the feature-flaggedeffectiveAuthor*blocks,resolveTaskAttributionDisplay(732 lines), and the analytics re-implementation of its cascade are deleted. Analytics now shows automations as first-class creator series.task_runs.acting_user_idor a deployment service principal in job tokens (r.uclaim optional).resolveUserIdForCloudJob, which silently made the first user in the users table the credential owner for all automation work, is gone along with all six placeholder-owner forgeries.tasks/task_pull_requests, so the four chain-walkers and cross-run field copying are deleted.CloudTaskTypesplits intotasks.workflow x surface x trigger+runs.kind, withpayloadKindkept for runtime dispatch only.tasks x task_pull_requestsinstead of reading job columns.backgroundAutomationRunsis deleted: run history for task-launching automations istasks WHERE initiator_automation = key, and manual Run-now is synchronous, returning the launched task id. The ~600-line legacy settings merge, the manager-channel triple fallback, and the coach automation are deleted.Verification
Every stage landed with the full
pnpm checkgreen (lint, types, ~5,600 tests across 24 packages, knip). Baseline migration verified against a scratch database;db:generateproduces no diff.End-to-end on a fresh database via the web UI (GitLab-only deployment, Local Docker sandboxes, mock Slack harness): setup onboarding, web task launch through completion, synchronous automation Run-now producing a hidden suggester scan, work items + tracked messages + Slack posts, task history with initiator display, and analytics with user and automation series.
The E2E run also surfaced and fixed four pre-existing GitHub-only bugs that leftover GitHub state in dev databases had been masking (provider stamping at launch, git-proxy gzip double-decode on gitlab.com clones, a setup guard requiring a GitHub installation forever, Microsoft avatar hosts) and three refactor regressions (service-principal tokens rejected by deployment-scoped reads, suggestion posts skipped without a human poster, dequeue-time cancels leaving tasks active).
Follow-ups (non-blocking)
background_agent_settingssurvivors intodeployment_settings.CloudJob/CloudTask*types toRun/Task*(tables are already renamed; type aliases marked TODO(stage5-rename)).Internal guidance (
.agent-guidance/) and public docs are updated in the same change.