refactor(orchestration): add actor principal columns and dual-write (1/6) - #19943
brennanb2025 wants to merge 6 commits into
Conversation
principalFromPaneKey is the single owner of both dual-write rules: NULL pane key means NULL principal, and a structured worker's minted pane key classifies to session:<sessionId> via parsePaneKey plus the new inverse structuredAgentSessionIdFromTabId. The tab-id pair moves to its own shared module (single source for the prefix) because the projection file sits at the max-lines cap; the projection re-exports both names.
Migration v41 adds the four nullable principal columns and owns the coordinator-cache trigger form: the static createTables SQL keeps the handle-only text because it must stay compilable against a pre-v41 runs table — a database stamped before the cache existed (< v30) would otherwise get triggers naming coordinator_principal before the ALTER runs, and the v40 backfill's INSERT INTO runs dies at prepare. The v41 triggers cache COALESCE(coordinator_handle, coordinator_principal): terminal_handle is a mailbox-address column (messages.to_handle already carries run:<id>), so a handle-less session coordinator's row matches every string-equality reader with zero read-side change, and the cache keeps its shape. The backfill classifies via principalFromPaneKey (a structured worker's row gets session:<id>, session values are fenced from repair) and reruns on every open, v40-style, to repair rows a rolled-back binary writes after user_version is already 41. coordinator_principal joins the internal receipt columns so it never ships on the wire.
…iter Every writer that sets or clears a coordinator/assignee/creator/resource handle or pane key now writes the classified principal alongside, through principalFromPaneKey — never a blind copy, so a structured worker's minted pane key lands as session:<sessionId>. Unbind and legacy adoption clear it. The open-time cache repopulate widens to COALESCE the same way as the triggers; a session coordinator's cached address reroutes its mail through the untouched reader SQL, which the migration test pins. Nothing reads the new columns; dispatch depth keeps its handle/pane path, and the new dispatch-depth test pins the equal-principals-imply-self-created invariant the resolver PR's read switch must preserve.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — one minor comment inline.
Reviewed changes
orchestration-principal.ts(new, shared) —format/parseat the first colon plusprincipalFromPaneKey, the single classification rule, with an explicit security boundary (session-derivation safe server-side only; request-time invariant deferred to PR 2). Correctly scoped.migrate-v41.ts+principal-column-backfill.ts—hasColumn-guardedALTER ADD COLUMNand a COALESCE-form trigger swap that keeps the staticcreateTablesSQL compilable against pre-v41runs; an every-open reconcile that fences outsession:rows and never mints a principal from a bare handle.- Dual-write fan-out —
coordinator_principal/assignee_principal/creator_principal/principalwritten at every creator/assignee/resource writer viaprincipalFromPaneKeyandrecordedCreatorIdentity, with the coordinator mail-address cache learning session coordinators viaCOALESCE. - Projection + receipt isolation —
row-column-lists.ts,types.ts, and version-skew columns stay in sync;exposeRunstripscoordinator_principalso no receipt shape changes. - Tests (613 lines) — migration chain from a seeded v40 file DB, rolled-back-binary repair, session-fence, handle-less coordinator cache seam, and dual-write assertions with exact expected values (not loose
>=/truthiness).
The migration reasoning held up under review: a fresh DB still runs migrateV41 because the skew resolver returns storedVersion when the post-v6 schema is complete, and the pre-v30 trigger-compilation hazard is avoided by keeping the handle-only form in static SQL. The one thing that gave me pause is a comment in the backfill that understates the every-open cost — see inline.
DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
| .prepare( | ||
| `SELECT id, ${paneColumn} AS pane_key, ${principalColumn} AS principal FROM ${table} | ||
| WHERE (${principalColumn} IS NULL AND ${paneColumn} IS NOT NULL) | ||
| OR ${principalColumn} LIKE 'pane:%'` |
There was a problem hiding this comment.
The comment above overstates the steady-state cheapness: principal LIKE 'pane:%' matches every ordinary pane-keyed row, not zero, so on every open this scans and re-classifies all principal-bearing rows (and dispatch_contexts twice, once per principal column). The v40 backfill this is modeled on (home_run_id = '') genuinely returns zero rows on a healthy DB; this one does not. Worth correcting the comment so future readers aren't misled about the recurring cost, and worth considering whether the stale-pane: detection can be narrowed in SQL rather than recomputed in JS for every row.
📝 WalkthroughWalkthroughThe change adds pane- and session-based orchestration principal serialization, parsing, and classification. Schema version 42 adds principal columns, trigger updates, and idempotent backfill logic. Run, dispatch, worker authority, and terminal resource paths now dual-write principal values. Coordinator routing uses principal fallback values. Run receipts exclude internal principal data. Tests cover classification, persistence, migration, routing, and receipt filtering. Priority: ➖ Normal Merge Risk: 🔵 Low · up to Upgrade handling for valid v41 databases can unnecessarily replay historical migrations, and a rare empty stored pane key can lose its derived identity. These are bounded migration and data-reconciliation risks that should be corrected before broad rollout. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description provides detailed technical context and validation results, but it does not follow the required template. It omits the required Linked Issue, Visual Proof, Testing, Review, Agent skill upstream boundary, Notes, and Checklist sections, and it contains conflicting statements about whether the principal columns are read. Resolution Restructure the description using all required template headings. Add a valid issue link, state N/A with a reason for Visual Proof if no UI change exists, complete the Testing checkboxes and platform coverage, provide the applicable AI Disclosure, Agent skill boundary, Notes, and Checklist entries, and remove or correct the conflicting claim that nothing reads the columns.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 350cf9d8-d4eb-486c-a049-aa25710f5f35
📒 Files selected for processing (31)
src/main/runtime/orchestration/db/contract-constants.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-capability.tssrc/main/runtime/orchestration/db/dispatch-depth.test.tssrc/main/runtime/orchestration/db/dispatch-depth.tssrc/main/runtime/orchestration/db/dispatch-row-writer.tssrc/main/runtime/orchestration/db/orchestration-db.tssrc/main/runtime/orchestration/db/principal-dual-write.test.tssrc/main/runtime/orchestration/db/row-column-lists.tssrc/main/runtime/orchestration/db/runs/run-binding.tssrc/main/runtime/orchestration/db/runs/run-coordinator-mail-routing.tssrc/main/runtime/orchestration/db/runs/run-create.tssrc/main/runtime/orchestration/db/runs/run-lookup.tssrc/main/runtime/orchestration/db/schema/adopt-legacy-run.tssrc/main/runtime/orchestration/db/schema/create-core-tables-sql.tssrc/main/runtime/orchestration/db/schema/create-graph-tables-sql.tssrc/main/runtime/orchestration/db/schema/migrate-v41.tssrc/main/runtime/orchestration/db/schema/migrate.tssrc/main/runtime/orchestration/db/schema/principal-column-backfill.tssrc/main/runtime/orchestration/db/schema/principal-column-migration.test.tssrc/main/runtime/orchestration/db/worker-dispatch/worker-dispatch-authority.tssrc/main/runtime/orchestration/db/worker-terminal/failed-start-dispatch-identity.tssrc/main/runtime/orchestration/db/worker-terminal/worker-terminal-resource-store.tssrc/main/runtime/orchestration/orchestration-schema-version-skew.tssrc/main/runtime/orchestration/types.tssrc/main/runtime/orchestration/worker-terminal-ownership.tssrc/main/runtime/rpc/methods/orchestration/runs/run-receipt.test.tssrc/main/runtime/rpc/methods/orchestration/runs/run-receipt.tssrc/shared/orchestration-principal.test.tssrc/shared/orchestration-principal.tssrc/shared/structured-agent-session-projection.tssrc/shared/structured-agent-session-tab-id.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 3 remain after this review.
| * backfill rule is NOT a general equivalence rule. | ||
| */ | ||
| export function principalFromPaneKey(paneKey: string | null | undefined): string | null { | ||
| if (!paneKey) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify that every pane-key column rejects empty strings before relying on the current falsy check.
rg -n -C 4 'CHECK|coordinator_pane_key|assignee_pane_key|creator_pane_key|pane_key' \
src/main/runtime/orchestration/db/schema src/main/runtime/orchestration/dbRepository: stablyai/orca
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- principal helper ---'
cat -n src/shared/orchestration-principal.ts | sed -n '1,130p'
printf '%s\n' '--- backfill consumer ---'
cat -n src/main/runtime/orchestration/db/schema/principal-column-backfill.ts | sed -n '1,90p'
printf '%s\n' '--- principal references and pane-key writers ---'
rg -n -C 3 'principalFromPaneKey|coordinator_principal|assignee_principal|creator_principal|\.principal|coordinator_pane_key|assignee_pane_key|creator_pane_key' \
src/main src/shared --glob '*.{ts,tsx}' | head -n 500Repository: stablyai/orca
Length of output: 50369
🤖 get_repo_knowledge executed:
get_repo_knowledge stablyai/orca /tmp/coderabbit-repo-knowledge/stablyai-orca-89dc44e4/learnings
Length of output: 31761
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relevant schema declarations ---'
for f in \
src/main/runtime/orchestration/db/schema/create-core-tables-sql.ts \
src/main/runtime/orchestration/db/schema/create-graph-tables-sql.ts \
src/main/runtime/orchestration/db/schema/migrate-v13-v30.ts \
src/main/runtime/orchestration/db/schema/migrate-v2-v12.ts
do
echo "FILE: $f"
rg -n -C 2 'coordinator_pane_key|assignee_pane_key|creator_pane_key|pane_key|principal' "$f" | head -n 160
done
printf '%s\n' '--- helper documentation and tests ---'
rg -n -C 5 'non-NULL|opaque|pane principal|session principal|principalFromPaneKey|pane:' \
src test tests docs --glob '*.{ts,tsx,md}' 2>/dev/null | head -n 400Repository: stablyai/orca
Length of output: 47889
Preserve or reject empty pane keys before principal backfill.
principalFromPaneKey maps '' to NULL, while backfillPrincipalColumns selects all non-NULL pane keys and writes that result to the principal column. The schema does not reject empty pane keys. If empty keys are supported, update both principalFromPaneKey and parseOrchestrationPrincipal so pane: round-trips; otherwise reject them before backfill and writes.
|
Electron QA evidence (branch
|
…migration to v42 Main took the v41 slot for the derived-delivery migration, so the principal-column migration moves to v42 and runs after it. - migrate-v41.ts is main's file unchanged; the principal columns, trigger recreation and backfill move to migrate-v42.ts with their version guards renumbered. - SCHEMA_VERSION is 42. The version-history comment keeps main's per-version lines and records v40 and v42, which the two sides had each dropped. - run-binding keeps the principal argument the four-placeholder UPDATE requires while taking main's renamed fenceUnacknowledgedMailboxDeliveries. - The migration test targets migrateV42 from v41; its real-chain case still seeds a v40 stamp, so it now exercises main's v41 before v42 as a real upgrade would.
There was a problem hiding this comment.
Important
The v42 renumber left the principal columns listed at version: 41 in the version-skew probe, so a database stamped 41 by main's build reads as skewed and the migration chain replays from the v6 floor. Details inline.
Reviewed changes
- Renumbered the principal migration to v42 — main's new v41 (derived outstanding deliveries) now precedes it:
migrate-v41.ts→migrate-v42.ts,SCHEMA_VERSION41 → 42, and the chain callsmigrateV41thenmigrateV42. - Merged main's derived-delivery schema —
DERIVED_DELIVERY_SCHEMA_SQLis applied at the end ofmigrate, and callers moved tofenceUnacknowledgedMailboxDeliveries. - Updated the migration test to seed a v40 database, so the real chain runs main's v41 before v42 — though it does not cover a database already stamped at 41.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| { version: 41, table: 'runs', column: 'coordinator_principal' }, | ||
| { version: 41, table: 'dispatch_contexts', column: 'assignee_principal' }, | ||
| { version: 41, table: 'dispatch_contexts', column: 'creator_principal' }, | ||
| { version: 41, table: 'worker_terminal_resources', column: 'principal' } |
There was a problem hiding this comment.
These four principal columns are declared at version: 41, but the migration that adds them is migrateV42. The completeness probe requires a column whenever storedVersion >= declaredVersion, so a database legitimately stamped 41 (main's current build, which has no principal columns) reads as version-skewed: resolveOrchestrationMigrationStartVersion returns the v6 floor instead of 41 and the entire v6..v42 chain replays, including the historical backfills the VERSIONED_POST_V6_COLUMNS list exists to keep from re-running. The declared version must match the migration that adds the column — exactly as home_run_id is version: 40 because migrateV40 adds it.
Technical details
# Bump the principal columns to version 42 in the version-skew list
## Affected sites
- `src/main/runtime/orchestration/orchestration-schema-version-skew.ts:47-50` — four entries carry `version: 41`; the columns are created by `migrateV42`.
- Tests miss it: `principal-column-migration.test.ts` seeds a v40 database (v40 short-circuits cleanly) and `orchestration-all-start-versions-migration.test.ts` stamps a fully-migrated database whose columns are already present, so neither exercises a v41 stamp that lacks the columns.
## Required outcome
- `resolveOrchestrationMigrationStartVersion(db, 41, 42)` returns 41 for a v41 database without principal columns, so only `migrateV42` runs on upgrade.
- A regression test that drops the four columns from a migrated database, stamps `user_version = 41`, and asserts the resolved start version is 41 (not 6).
## Evidence
- Reproduced locally: build a full-schema database, drop the four principal columns and the two `trg_runs_remember_coordinator_*` triggers, `PRAGMA user_version = 41`, then `resolveOrchestrationMigrationStartVersion(raw, 41, 42)` returned `6` where `41` is expected.| { version: 41, table: 'runs', column: 'coordinator_principal' }, | |
| { version: 41, table: 'dispatch_contexts', column: 'assignee_principal' }, | |
| { version: 41, table: 'dispatch_contexts', column: 'creator_principal' }, | |
| { version: 41, table: 'worker_terminal_resources', column: 'principal' } | |
| { version: 42, table: 'runs', column: 'coordinator_principal' }, | |
| { version: 42, table: 'dispatch_contexts', column: 'assignee_principal' }, | |
| { version: 42, table: 'dispatch_contexts', column: 'creator_principal' }, | |
| { version: 42, table: 'worker_terminal_resources', column: 'principal' } |
| * Reconciles principal columns from their pane-key columns — the v40 every-open pattern: a binary | ||
| * rolled back past v41 keeps writing after user_version is already 41, inserting rows with NULL | ||
| * principals and, worse, re-pointing pane keys without touching the principal, leaving stale | ||
| * `pane:` values. Runs from migrate-v41 and from every open; idempotent by construction (a second |
There was a problem hiding this comment.
The renumber left this docblock describing the old version: after a rollback the binary now writes while user_version is already 42, and this reconcile is invoked from migrateV42. Worth updating the v41 / migrate-v41 references so the invariant matches the migration that owns it.
| -- The principal-aware (COALESCE) trigger form is owned by migrate-v41, which every DB passes | ||
| -- through: the static SQL below must stay compilable against a pre-v41 runs table, where naming | ||
| -- NEW.coordinator_principal would break the first runs INSERT of the migration chain. |
There was a problem hiding this comment.
This comment still names migrate-v41 / pre-v41, but the COALESCE trigger form moved to migrateV42 in the renumber (main's v41 is the derived-delivery migration). Minor, but it points readers at the wrong migration.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Record the actor principal columns as version 42. · orchestration-schema-version-skew.ts:46-50
src/main/runtime/orchestration/orchestration-schema-version-skew.ts:46-50
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winRecord the actor principal columns as version 42.
migrateV42adds and backfills these four columns, buthasCompletePostV6Schemacurrently requires them foruser_version = 41. A valid v41 database without them therefore resolves to migration version 6, causing normal migration to replay earlier migrations and backfills before applying v42. Set all four entries to version 42.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 5440068c-5040-4186-ac5b-30af64aa75b9
📒 Files selected for processing (9)
src/main/runtime/orchestration/db/contract-constants.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-capability.tssrc/main/runtime/orchestration/db/runs/run-binding.tssrc/main/runtime/orchestration/db/runs/run-lookup.tssrc/main/runtime/orchestration/db/schema/migrate-v42.tssrc/main/runtime/orchestration/db/schema/migrate.tssrc/main/runtime/orchestration/db/schema/principal-column-migration.test.tssrc/main/runtime/orchestration/db/worker-dispatch/worker-dispatch-authority.tssrc/shared/structured-agent-session-projection.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 2 remain after this review.
…role The rename to revertToV41Shape tripped the naming audit — "shape" describes structure rather than what the helper does, which is undo the principal migration.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 9f62b8a9-a87d-497f-bc52-bdd8dd62cf4f
📒 Files selected for processing (31)
src/main/runtime/orchestration/db/contract-constants.tssrc/main/runtime/orchestration/db/dispatch-context/dispatch-capability.tssrc/main/runtime/orchestration/db/dispatch-depth.test.tssrc/main/runtime/orchestration/db/dispatch-depth.tssrc/main/runtime/orchestration/db/dispatch-row-writer.tssrc/main/runtime/orchestration/db/orchestration-db.tssrc/main/runtime/orchestration/db/principal-dual-write.test.tssrc/main/runtime/orchestration/db/row-column-lists.tssrc/main/runtime/orchestration/db/runs/run-binding.tssrc/main/runtime/orchestration/db/runs/run-coordinator-mail-routing.tssrc/main/runtime/orchestration/db/runs/run-create.tssrc/main/runtime/orchestration/db/runs/run-lookup.tssrc/main/runtime/orchestration/db/schema/adopt-legacy-run.tssrc/main/runtime/orchestration/db/schema/create-core-tables-sql.tssrc/main/runtime/orchestration/db/schema/create-graph-tables-sql.tssrc/main/runtime/orchestration/db/schema/migrate-v42.tssrc/main/runtime/orchestration/db/schema/migrate.tssrc/main/runtime/orchestration/db/schema/principal-column-backfill.tssrc/main/runtime/orchestration/db/schema/principal-column-migration.test.tssrc/main/runtime/orchestration/db/worker-dispatch/worker-dispatch-authority.tssrc/main/runtime/orchestration/db/worker-terminal/failed-start-dispatch-identity.tssrc/main/runtime/orchestration/db/worker-terminal/worker-terminal-resource-store.tssrc/main/runtime/orchestration/orchestration-schema-version-skew.tssrc/main/runtime/orchestration/types.tssrc/main/runtime/orchestration/worker-terminal-ownership.tssrc/main/runtime/rpc/methods/orchestration/runs/run-receipt.test.tssrc/main/runtime/rpc/methods/orchestration/runs/run-receipt.tssrc/shared/orchestration-principal.test.tssrc/shared/orchestration-principal.tssrc/shared/structured-agent-session-projection.tssrc/shared/structured-agent-session-tab-id.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| { version: 40, table: 'remote_dispatch_attachments', column: 'home_run_id' }, | ||
| { version: 41, table: 'runs', column: 'coordinator_principal' }, | ||
| { version: 41, table: 'dispatch_contexts', column: 'assignee_principal' }, | ||
| { version: 41, table: 'dispatch_contexts', column: 'creator_principal' }, | ||
| { version: 41, table: 'worker_terminal_resources', column: 'principal' } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,130p' src/main/runtime/orchestration/orchestration-schema-version-skew.ts
sed -n '1,100p' src/main/runtime/orchestration/db/schema/migrate-v42.ts
rg -n -C 4 "hasCompletePostV6Schema|VERSIONED_POST_V6_COLUMNS|orchestration-schema-version-skew" src/main --glob '*.ts'Repository: stablyai/orca
Length of output: 21881
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- skew resolver ---'
cat -n src/main/runtime/orchestration/orchestration-schema-version-skew.ts | sed -n '150,225p'
printf '%s\n' '--- migration runner and version constants ---'
cat -n src/main/runtime/orchestration/db/schema/migrate.ts | sed -n '1,180p'
cat -n src/main/runtime/orchestration/db/contract-constants.ts | sed -n '1,80p'
printf '%s\n' '--- v42 and principal migration references ---'
rg -n -C 5 'migrateV42|applySchemaMigrations|SCHEMA_VERSION|resolveOrchestrationMigrationStartVersion|coordinator_principal|assignee_principal|creator_principal' src/main/runtime/orchestration --glob '*.ts' | head -n 500
printf '%s\n' '--- focused tests ---'
rg -n -C 8 'v41|v42|principal.*column|schema.*skew|incomplete|start version' src/main/runtime/orchestration/*test.ts src/main/runtime/orchestration/db/schema/*test.ts 2>/dev/null | head -n 500Repository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- migration entrypoints and replay-sensitive operations ---'
rg -n 'export function|function apply|function migrate|INSERT INTO|UPDATE |DELETE FROM|DROP TABLE|DROP COLUMN|ALTER TABLE|CREATE UNIQUE INDEX|CREATE INDEX' \
src/main/runtime/orchestration/db/schema/migrate-v2-v12.ts \
src/main/runtime/orchestration/db/schema/migrate-v13-v30.ts \
src/main/runtime/orchestration/db/schema/migrate-mailbox-pointer-enter-v33.ts \
src/main/runtime/orchestration/db/schema/migrate-role-mailbox-delivery-v34.ts \
src/main/runtime/orchestration/db/schema/migrate-v35.ts \
src/main/runtime/orchestration/db/schema/migrate-v36.ts \
src/main/runtime/orchestration/db/schema/migrate-v37.ts \
src/main/runtime/orchestration/db/schema/migrate-v38.ts \
src/main/runtime/orchestration/db/schema/migrate-v39.ts \
src/main/runtime/orchestration/db/schema/migrate-v40.ts \
src/main/runtime/orchestration/db/schema/migrate-v41.ts \
src/main/runtime/orchestration/db/schema/migrate-v42.ts
printf '%s\n' '--- all-start-versions test ---'
cat -n src/main/runtime/orchestration/orchestration-all-start-versions-migration.test.ts | sed -n '1,110p'
printf '%s\n' '--- migration path assertions near v41/v42 ---'
rg -n -C 12 '41|42|principal|all prior|replay|replay.*chain|start version' \
src/main/runtime/orchestration/orchestration-all-start-versions-migration.test.ts \
src/main/runtime/orchestration/orchestration-version-skew-migration.test.ts \
src/main/runtime/orchestration/*migration.test.ts | head -n 350Repository: stablyai/orca
Length of output: 28193
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- v23 resource migration block ---'
cat -n src/main/runtime/orchestration/db/schema/migrate-v13-v30.ts | sed -n '105,145p'
printf '%s\n' '--- related resource migration tests and assertions ---'
rg -n -C 10 'resource.*backfill|backfill.*resource|worker_terminal_resources|resource ownership|v23' \
src/main/runtime/orchestration --glob '*.ts' | head -n 300Repository: stablyai/orca
Length of output: 36993
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 20 'backfillWorkerTerminalResources' src/main/runtime/orchestration --glob '*.ts'Repository: stablyai/orca
Length of output: 14120
🏁 Script executed:
#!/bin/bash
set -euo pipefail
cat -n src/main/runtime/orchestration/db/worker-terminal/worker-terminal-resource-store.ts | sed -n '14,75p'Repository: stablyai/orca
Length of output: 2703
Register the principal columns as v42 requirements. migrateV42 creates all four principal columns. With the current version 41 entries, hasCompletePostV6Schema marks a valid v41 database without those columns as incomplete, so the resolver returns 6. The migration runner can then replay the full v6-to-v42 chain, including earlier backfills, instead of starting at v41. Mark all four requirements as version 42.
|
Superseded by #22522, the first PR of a fresh orchestration stack for structured-session coordinators. The design changed after this PR was written, so almost no line of it survives a rework:
#22522 ports this PR's hard-won migration reasoning (createTables before migrate, the trigger DROP+CREATE, the real-chain tests). It also fixes a defect found here: the skew probe registered the new columns at version 41 while migrating at 42, so a v41 database replayed the whole chain from v6. Closing in favour of #22522. |
1 participant
Merge status
Ready to merge on its own. Not blocked on any other PR, branch or lane.
What reads these columns: one internal path, dormant in effect.
orchestration-principal.tsdescribes itself aswrite-only until the resolver PRs, and that is true at the RPC boundary —run-receipt.tsstill excludes the column. But it is not true inside this PR:run-coordinator-mail-routing.ts:20-21selectsCOALESCE(coordinator_handle, coordinator_principal)to populate the coordinator mail-address cache. It returns nothing different today, because a handle-less coordinator cannot exist until a later PR creates one — but the read path is live, so this is a migration with one internal reader, not a free rename. (Correcting an earlier revision of this description, which claimed nothing reads them.)What is hooked up to it:
mainonce this lands.Execution scope is missing, and that is a follow-up in this stack, not a deferral to another lane. A
session:<sessionId>principal does not record where that session runs. That is harmless today only becausestructuredWorkerHostScoperefuses any structured session that is not local-and-non-WSL, so there is exactly one host it can be on. The moment structured chat ships on WSL or SSH that guard relaxes and the key becomes ambiguous — in a durable column, silently. So the session arm should carry the four scope fields (executionHostId,wslDistro,workspaceId,workspaceKind), and the cheap window is now, while the only reader is the internal one above.Note for whoever does that: the fields are on
AgentSessionRecord.location, but the resolver's output shape discards them —structuredWorkerHostScopecollapses host and distro into{kind:'local'} | null, andworkspaceKindappears nowhere in that path. Widening it is real work, not a field copy.The
pane:arm should not ship. It is wrong by construction — a pane outlives the agent in it, so a coordinator bound topane:<paneKey>has its Run binding inherited by whatever occupies that pane next. And it is unnecessary: PTY agents already coordinate orchestration fine on terminal handles. The bug this stack exists to fix is structured-only.So the intended end state for this PR is the session arm alone, with PTY keeping
coordinator_handle/coordinator_pane_keyunchanged. A single reference covering both lanes remains the goal, but it arrives when PTY has a durable agent id — which is separate work, in a separate program.Rebase, 2026-09-16
Rebased onto
mainafter 438 commits. Four conflicts, two of which were not mechanical:maintook the v41 slot for the derived-delivery migration.migrate-v41.tshere ismain's file unchanged; the principal columns, trigger recreation and backfill moved tomigrate-v42.tswith their guards renumbered, running after v41.SCHEMA_VERSIONnever conflicted. Only the version-history comment did, so the constant auto-merged to41and would have shipped a migration that never runs. Now42.run-binding.tsneeded both sides. This branch adds the principal argument;mainrenamedfenceOutstandingDeliverytofenceUnacknowledgedMailboxDeliveries. Takingmain's side alone would have bound three arguments to a four-placeholderUPDATE— the SQL carryingcoordinator_principal = ?auto-merged outside the conflict region — which typecheck cannot see.migrateV42from v41. Its real-chain case still seeds a v40 stamp, so it now runsmain's v41 before v42, which is the ordering a real upgrade sees.Post-rebase validation:
pnpm tc:nodeclean; 953 passed acrosssrc/main/runtime/orchestrationplus the principal unit tests; 777 passed acrosssrc/main/runtime/rpc/methods/orchestration; anti-slop audit clean;pnpm-lock.yamlabsent from every non-merge commit (main's arrived via the merge parent, byte-identical toorigin/main).Note for reviewers on the changed-code quality gate: merging 438 commits moves its baseline, so it reports findings across ~3900 files. Every finding was checked against the six files this rebase touched — the only hits are two pre-existing
consistent-type-assertionslines in the migration test that this diff does not touch.Summary
Orchestration identifies every actor — Run coordinator, dispatch assignee, worker — by a terminal handle plus a pane key. A native chat session has neither, so today it mints a fake handle and a fake pane key to participate. This is the first of six PRs replacing that key with a principal:
pane:<paneKey>orsession:<sessionId>, one tagged string per role.This PR only adds the columns and writes them. Nothing reads them, and there is no behavior change — that is the acceptance bar.
What's here
maintook that slot):runs.coordinator_principal,dispatch_contexts.assignee_principal,dispatch_contexts.creator_principal,worker_terminal_resources.principal.src/shared/orchestration-principal.ts— format/parse (split at the first colon; pane keys contain one) plusprincipalFromPaneKey, the single place encoding both rules: NULL pane key yields NULL principal, and a structured worker's pane key classifies tosession:<id>rather thanpane:<key>.pane:value, not just a NULL — a rolled-back binary can update a pane key without touching the principal.session:values are never overwritten.principalFromPaneKey/recordedCreatorIdentityso the tab-id prefix string is never re-derived.COALESCE(coordinator_handle, coordinator_principal)in the two remember-triggers. Dormant until a handle-less coordinator exists (PR 5).Why classification matters
A structured worker's pane key is
structured-agent-session-<sessionId>:<randomUUID>— the session id is in the tab half in plain text. Copying it into apane:principal would mint the wrong identity: harmless here, but it strands every live structured coordinator's bound Run the moment PR 2 flips reads.That derivation is legal only in this server-side backfill, over rows the host itself wrote. At request time it is an attack: a caller who learns a session id can fabricate
structured-agent-session-<id>:<anything>, and the random leaf is the only real credential. The code carries that asymmetry as a comment; the request-time invariant belongs to PR 2.Three defects in the original plan, fixed here
createTablesruns beforemigrate, and v40's backfill inserts intoruns. A trigger namingNEW.coordinator_principalin the static SQL compiles against a pre-ALTERrunsand throws before v41 adds the column — orchestration fails to open for anyone upgrading from an older build. Confirmed with sqlite3.createTablesnow keeps the handle-only trigger text and migrate-v41 solely owns the final form; a fresh DB still reaches v41 because the skew resolver returnsstoredVersion, notSCHEMA_VERSION.principalcolumn with a(run_id, principal)partial unique index. Two handles legitimately share one pane principal, soINSERT OR IGNOREdropped the cache row for a reminted handle and its mail went undelivered. The COALESCE shape removes the whole class — one address column, keyed by the existing primary key — along with the table rebuild, its CHECK, and both indexes.Validation
tsc --noEmitclean on node, cli and web, run sequentially.max-linesdisables —structured-agent-session-projection.tswas at 294/300, so the tab-id pair moved to its own module with re-exports keeping every importer unchanged.pnpm-lock.yamlabsent from every commit and from the full branch range.Notes for review
principalcolumn with a v41 table rebuild, then simplified to the COALESCE shape below; the branch was rewritten into three coherent commits, so that detour is not in the history. It is recorded here because defect 2 is only reachable under the shape that was dropped.exposeDispatchContextdeliberately does not expose principals — no receipt shape changes.isSelfCreatedDispatchandresolveCreatorDepthkeep their handle/pane logic, so nesting-depth outcomes are identical to main. The principal-equality switch is owned by a later PR in the stack.