refactor(orchestration): give structured sessions an orchestration actor column - #22522
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (15)
Included review availability: Your plan provides up to 10 included reviews per hour; 5 remain after this review. 📝 WalkthroughWalkthroughThe change adds a session actor codec and actor columns for runs and dispatch contexts. Schema version 42 adds the columns, indexes, and coordinator triggers. A backfill derives actor values from structured-worker identity evidence and runs during database construction. Coordinator routing can use actor addresses, while run receipts omit the internal coordinator actor fields. Merge Risk: ⚪ Minimal · up to The actor migration and routing changes appear ready to merge after normal checks; no new blocking issue is established. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description is detailed and covers the change, rationale, testing, compatibility, risks, and checklist. However, the required Linked Issue section states "None," even though the template requires an issue link.
✨ 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: 593a1fe4-0bd6-4c12-b74d-07f530385ac2
📒 Files selected for processing (22)
src/main/runtime/orchestration/db/contract-constants.tssrc/main/runtime/orchestration/db/dispatch-depth.test.tssrc/main/runtime/orchestration/db/orchestration-db.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-lookup.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/structured-worker-actor-backfill.test.tssrc/main/runtime/orchestration/db/schema/structured-worker-actor-backfill.tssrc/main/runtime/orchestration/orchestration-actor-column-migration.test.tssrc/main/runtime/orchestration/orchestration-schema-version-skew.tssrc/main/runtime/orchestration/run-coordinator-actor-address.test.tssrc/main/runtime/orchestration/types.tssrc/main/runtime/rpc/methods/orchestration/runs/run-receipt.test.tssrc/main/runtime/rpc/methods/orchestration/runs/run-receipt.tssrc/main/runtime/structured-worker-identity.tssrc/shared/orchestration-actor.test.tssrc/shared/orchestration-actor.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 0 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — the codec, schema, migration, and open-time fill check out. Two carry-forward items for the next PRs in the stack; nothing to change in this one.
Reviewed changes
- Actor codec —
src/shared/orchestration-actor.tsmakessession:<id>both the stored column value and the mailbox address, parsing only the addressed spelling and validating every id throughisAgentSessionId. - Schema v42 — nullable
coordinator_actor/assignee_actor/creator_actorcolumns plus two partial indexes;migrate-v42.tsreplaces the coordinator-address triggers with aCOALESCE(coordinator_handle, coordinator_actor)form, while the staticcreateTablestriggers stay handle-only so the pre-v42 chain still prepares. - Open-time fill —
backfillStructuredWorkerActorsruns aftermigrateon every open and repairs only NULL actors, fail-closed on ambiguous evidence and never reading pane keys; it runs before theCOALESCEcache seed. - Writers and receipt —
bindRun/unbindOtherRunsForPaneclear the actor when they replace or clear a coordinator, andexposeRunstripscoordinator_actorfrom every run-receipt path.
I read the complete diff, the surrounding migration/skew/cache code, and independently checked both the wire publication and the migration/backfill mechanics against the tests. No consumer-facing breakage and no mis-assignment path; the notes below are about scope the PR hands to later PRs.
ℹ️ The later-PR obligation list omits the foreign-direct routing path
The body correctly flags that the routing trigger's self-dispatch exclusion and routeAllUnreadDirectMessagesToRunMailbox match assignee_handle / coordinator_handle only and must learn about actors. The same handle-only match also drives routeForeignDirectMessagesToOwnedMailboxes and its helper findActiveDispatchForDirectMessageOwner; once a session: recipient can also be an active assignee, mail addressed to it would land in run:<id> instead of dispatch:<id>.
Technical details
# Actor recipients need the same run-vs-dispatch routing in the foreign-direct path
## Affected sites
- `src/main/runtime/orchestration/db/messages/foreign-direct-mailbox-routing.ts:37` — branch 1 pulls undelivered mail whose `to_handle` equals a cached address in `run_coordinator_handles` into the Run mailbox.
- `src/main/runtime/orchestration/db/messages/foreign-direct-mailbox-routing.ts:10` — `findActiveDispatchForDirectMessageOwner` matches `assignee_handle` only, so an actor-addressed assignee is never found and its mail routes to `run:` rather than `dispatch:`.
- `src/main/runtime/orchestration/db/schema/migrate-v42.ts:50` — the trigger's self-dispatch exclusion compares `assignee_handle`; the PR already lists this trigger and `routeAllUnreadDirectMessagesToRunMailbox` as sites to extend.
## Required outcome
- The obligation list the resolver PR works from should include the foreign-direct routing path, so `session:` mail follows the same run-vs-dispatch mailbox rules as handle mail.
## Open questions for the human (optional)
- Is this deliberately part of the same deferral as the two named sites, or overlooked?ℹ️ A rolled-back rebind leaves a Run whose handle and actor name different coordinators, and reopen never repairs it
After a v41 binary rebinds a structured-coordinated Run, the Run carries the new PTY coordinator_handle and the old coordinator_actor. The open-time fill only touches coordinator_actor IS NULL rows, so the contradiction survives every v42 reopen. It is harmless while nothing reads actors, but the first reader (resolver PR) switching self-dispatch / run-current / active-dispatch checks to actor equality would treat a PTY-coordinated Run as session-coordinated.
Technical details
# Contradictory coordinator identity survives every v42 reopen
## Affected sites
- `src/main/runtime/orchestration/db/schema/structured-worker-actor-backfill.ts:98,102` — the coordinators pass fills `coordinator_actor IS NULL` only; it never clears a non-NULL actor.
- `src/main/runtime/orchestration/db/runs/run-binding.ts:140` — `bindRun` clears the actor, but a v41 binary's UPDATE (which does not know the column) cannot.
- `src/main/runtime/orchestration/orchestration-actor-column-migration.test.ts:344-346` — pins `coordinator_actor = SESSION_ACTOR` after v41 sets `coordinator_handle = 'term_taker'`; the roll-forward assertions at `:362-371` check a different Run, so the contradictory row's post-reopen state is not pinned.
## Required outcome
- Either a v42 reopen repairs a Run whose `coordinator_actor` no longer matches its handle evidence, or the resolver PR is explicitly named as owner and the intended outcome is pinned with a test.
## Suggested approach (careful)
- Do NOT repair as "clear actor whenever a handle is set": a structured-worker coordinator legitimately carries both (asserted at `orchestration-actor-column-migration.test.ts:200`). The discriminator is whether the handle is a `structworker_` handle recorded against the actor's session — the same evidence `actorFor` already computes.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta (0fa27df → 9d8e573): one commit hardening the actor codec and clearing assignee_actor on assignee-identity rewrites, plus tests. The two prior pullfrog carry-forward notes are unaffected and remain deferred to the resolver PR by design.
- Codec refuses terminal-handle-shaped ids —
orchestration-actor.tsvalidates session ids throughisOrchestrationSessionId, which rejects theterm_andstructworker_prefixes the runtime mints, so a handle handed to the codec by mistake can no longer become a durablesession:actor. - Assignee actor cleared on identity rewrites —
prepareStartingWorkerAuthorityandrecordFailedStartDispatchIdentitynow setassignee_actor = NULL.mintDispatchCapabilityis left untouched because it never changesassignee_handle, so the third site CodeRabbit asked for cannot leave a stale assignee. - Tests — a new
worker-dispatch-assignee-actor.test.tspins both clears (it fails if either is dropped), andorchestration-actor.test.tspins the prefix refusal against a PTY handle and a freshly minted structured-worker handle.
I read the full diff and independently checked the two rewritten writers and their call paths (local-worker-start.ts, worker-dispatch-outcome.ts): a starting Dispatch is created with no assignee, so the new clears are a defensive reset rather than a live wipe on the local worker path. The changed files and the adjacent orchestration suites pass locally.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Review status: ready to merge (no changes made in this review)Head reviewed: Summary. One full review loop found no defects. The pre-release checklist ran at the start and again at the end, and both passes found no blocking issues. The app was also launched on a copy of a real user database. It upgraded cleanly, filled in an actor for exactly the rows that belong to structured workers, and booted again without errors. Review loop
Pre-release checklist (start and end): PASS, no P0 or P1The P2 notes, and what was done about each:
One thing the next PRs should keep in mind: the final form of the Upgrading a real database (Electron, background launch, separate profile)The branch app was pointed at a copy of a real v41 orchestration database: 1,601 Runs, 8,929 Dispatches (917 of them structured-worker), and 56,826 messages. The real database was never opened, and it is still at v41.
First boot after the upgrade: Second boot on the upgraded database: These screenshots only show that the app boots, because this PR has no UI change. Not verified
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta (9d8e573 → f5aee23): one commit correcting a stale schema-version assertion in the cross-version delivery-downgrade contract test.
- Downgrade contract assertion now reads
SCHEMA_VERSION—tests/e2e/cross-version-wire/orchestration-delivery-downgrade.unit.test.tsimportsSCHEMA_VERSIONand asserts the pre-v41 reopen stamp equals it, replacing the hard-coded41. The pre-v41 (aac38d67, max schema 40) code leaves any newer stamp alone, so the correct post-reopen value is 42; the old41was stale after this PR bumped the schema to 42. I ran the file (pnpm vitest run --testTimeout=90000 …) and it passes.
I re-read the full PR diff and the migrate/skew path (resolveOrchestrationMigrationStartVersion returns storedVersion when it exceeds the old code's schemaVersion, so migrate early-returns), confirming the corrected assertion is the accurate one. The two prior pullfrog carry-forward notes are unaffected and remain deferred to the resolver PR by design.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
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 · Clear stale actor state on legacy unbinds. · run-coordinator-mail-routing.ts:15-20
src/main/runtime/orchestration/db/runs/run-coordinator-mail-routing.ts:15-20
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winClear stale actor state on legacy unbinds.
A v41 writer can clear
coordinator_handlewithout clearingcoordinator_actor. The v42 trigger and open-time backfill then register that stale actor inrun_coordinator_handles. A later current-delivery message addressed to that actor can be rewritten torun:<id>, even though the run is unbound.Add a v42 compatibility trigger that clears the actor and its cache entry when an older writer changes
coordinator_handlewithout changingcoordinator_actor. The coordinator update trigger must not register an unchanged actor from that legacy write.Suggested fix
DROP TRIGGER IF EXISTS trg_runs_remember_coordinator_insert; DROP TRIGGER IF EXISTS trg_runs_remember_coordinator_update; + DROP TRIGGER IF EXISTS trg_runs_clear_legacy_coordinator_actor; + CREATE TRIGGER trg_runs_clear_legacy_coordinator_actor + AFTER UPDATE OF coordinator_handle ON runs + WHEN NEW.legacy = 0 + AND NEW.coordinator_actor IS NOT NULL + AND NEW.coordinator_actor IS OLD.coordinator_actor + BEGIN + DELETE FROM run_coordinator_handles + WHERE run_id = NEW.id AND terminal_handle = NEW.coordinator_actor; + UPDATE runs SET coordinator_actor = NULL WHERE id = NEW.id; + END; CREATE TRIGGER trg_runs_remember_coordinator_insert AFTER INSERT ON runs WHEN NEW.legacy = 0 AND COALESCE(NEW.coordinator_handle, NEW.coordinator_actor) IS NOT NULL @@ CREATE TRIGGER trg_runs_remember_coordinator_update AFTER UPDATE OF coordinator_handle, coordinator_actor ON runs - WHEN NEW.legacy = 0 AND COALESCE(NEW.coordinator_handle, NEW.coordinator_actor) IS NOT NULL + WHEN NEW.legacy = 0 + AND ( + NEW.coordinator_handle IS NOT NULL + OR (NEW.coordinator_actor IS NOT NULL AND NEW.coordinator_actor IS NOT OLD.coordinator_actor) + ) BEGIN INSERT OR IGNORE INTO run_coordinator_handles (run_id, terminal_handle) - VALUES (NEW.id, COALESCE(NEW.coordinator_handle, NEW.coordinator_actor)); + VALUES ( + NEW.id, + CASE + WHEN NEW.coordinator_handle IS NOT NULL THEN NEW.coordinator_handle + ELSE NEW.coordinator_actor + END + ); END;
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: b7dc511c-39c6-40c5-bb3e-5a29ae7f6e60
📒 Files selected for processing (1)
tests/e2e/cross-version-wire/orchestration-delivery-downgrade.unit.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 6 remain after this review.
f5aee23 to
f06e716
Compare
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta (f06e716 → b183a5c): one commit that pairs a Run's coordinator actor with the consumer_generation it was written at, so an actor a rebind or unbind leaves behind stops counting with nothing needing to clear it.
- Generation column and rule —
runs.coordinator_actor_generation(nullable, registered in the v42 skew probe) pluscurrentRunCoordinatorActor/currentRunCoordinatorActorSqlindb/runs/run-coordinator-actor.tsas the single owner of "the actor counts only whencoordinator_actor_generation === consumer_generation". - Cache seed, triggers, and fill — the open-time cache seed, the
migrate-v42insert/update triggers (the update trigger now also fires on the generation column), and the coordinator pass ofbackfillStructuredWorkerActorsall read through that rule; the fill only targets rows with no current actor and writesgeneration = consumer_generation. - Writers and wire —
bindRunandunbindOtherRunsForPaneclear the generation alongside the actor, andexposeRunstrips the generation from every run-receipt path so the wire stays unchanged. - Tests — a v41 rebind-then-unbind of a chat-coordinated Run now pins that the leftover actor stops counting, a v42 database stamped before the column existed replays correctly, and the fill is shown to overwrite a stale actor while leaving a current one alone.
This resolves the earlier carry-forward concern that a v41 rebind left a Run whose coordinator_handle and coordinator_actor named different coordinators: every binary bumps consumer_generation on rebind or unbind, so the stale actor no longer reads as current. I confirmed by grep that only bindRun and unbindOtherRunsForPane bump runs.consumer_generation, which is the assumption the rule rests on, and ran the three new/changed suites (16 tests) green; ablating currentRunCoordinatorActor to always return the actor turns the two v41 rollback tests red, so they are not theatre.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…tor column Adds nullable session:<id> actor columns to runs (coordinator) and dispatch_contexts (assignee, creator) at schema v42, a shared codec, a fill for rows that provably belong to a structured worker, and a coordinator mail-address cache that remembers a handle-less coordinator by its actor address.
…/v41 upgrade paths
…ar the assignee actor on reassignment
…t in the delivery downgrade contract The contract asserted user_version 41 after old code reopens a database current code wrote, so the v42 bump failed it. Assert SCHEMA_VERSION so the next bump cannot strand it; the pre-v41 pin and its v40 stamp stay.
…ation it was written at A binary without the actor column rebinds and unbinds a Run by rewriting its handle and pane, which it cannot clear the actor beside. A rebind followed by an unbind leaves a row identical to a live chat binding. Both writes bump consumer_generation, which every binary already maintains, so the actor now carries the generation it was written at (coordinator_actor_generation, set in the same statement) and counts only while the two are equal. The coordinator cache, its triggers and the open-time fill read the actor through one rule in run-coordinator-actor; the fill also replaces an actor an older generation left behind. Still schema v42 (unreleased): the column joins migrate-v42 and the v42 skew-probe entries, so a database stamped v42 without it replays the chain.
b183a5c to
19218ab
Compare
…bare-id normalizer Nothing in this stack looks a Run up by coordinator_actor: callers load the Run and compare its current actor, so idx_runs_coordinator_actor would ship in every database with no reader. v42 is unreleased, so it leaves the migration rather than needing a later drop. normalizeOrchestrationActor had no caller outside its tests; bare session ids enter through sessionOrchestrationActor, and the handle-refusal cases stay covered there and in parseOrchestrationActor.
There was a problem hiding this comment.
ℹ️ No critical issues — a dead-code cleanup with one stale comment left behind, inline.
Reviewed changes
Reviewed the incremental delta (19218ab → bd3a8fe): one cleanup commit removing two pieces of unused actor infrastructure.
- Dropped the unused
idx_runs_coordinator_actorindex —migrate-v42.tsno longer creates the partial Runs index (nothing searched Runs by coordinator actor; the later PRs load a Run and compare its actor), and the migration test'sstripActorSchemahelper no longer drops it. - Removed
normalizeOrchestrationActor— the bare-id-accepting helper and its tests are deleted;parseOrchestrationActorremains the only reader andsessionOrchestrationActorthe builder.
No references to either symbol remain anywhere under src, tests, docs, or skills, and the version-skew probe only checks columns, so the index removal is inert. Change is deletion-only and carries no behavioral surface.
ℹ️ Nitpicks
- The PR description still lists both removed pieces in its inventory:
idx_runs_coordinator_actorunder "Database columns and indexes" andnormalizeOrchestrationActorunder "Terms this PR introduces". Both are gone at this head, so the description now overstates what the PR ships.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…p needs The next step finds a caller's Runs with one statement that ORs a pane-leaf match with `coordinator_actor = ?`. SQLite splits that OR across two indexes only when both sides have one; without idx_runs_coordinator_actor the plan falls back to scanning every Run on each lookup. v42 is unreleased, so the index returns to migrate-v42 rather than needing a later schema step.
There was a problem hiding this comment.
ℹ️ No critical issues — the re-added index is load-bearing and the new test pins its use. One non-anchored gap and one description nit below.
Reviewed changes
Reviewed the incremental delta (bd3a8fe → a38ebdd): one commit restoring the coordinator-actor index and pinning its use.
- Restored
idx_runs_coordinator_actor—migrate-v42.tsrecreates the partial index onruns(coordinator_actor)that the previous commit removed, with a comment explaining it exists for a Run lookup that ORs a pane-leaf match with an actor match. - Plan test —
orchestration-actor-column-migration.test.tsaddscoordinatorLookupPlanand asserts the query plan usesidx_runs_coordinator_actorand does notSCAN runs, on a fresh database and on one stripped to v41 and reopened.
I verified the index is genuinely load-bearing rather than speculative: with it, that OR query plans as MULTI-INDEX OR over idx_runs_coordinator_pane_leaf and idx_runs_coordinator_actor; after DROP INDEX idx_runs_coordinator_actor, the same query plans as SCAN runs. So the commit's rationale and the test's exact assertions are both accurate. No production query searches Runs by coordinator_actor yet, so this is forward-looking infrastructure for the resolver PR.
ℹ️ A v42 database that only lacks the index is never repaired
The index is created inside migrateV42, which returns early on a v42 stamp, and the version-skew probe does not treat this index as part of the completeness check. A database stamped v42 by the intermediate bd3a8fe build — which removed the index — therefore reopens without it, and the caller lookup the next PR adds would table-scan on that profile. It is dev-only and performance-only (v42 has not shipped, so no release build ever lacked the index), but the commit's stated goal is to keep the index the caller lookup needs.
Technical details
# A v42 database missing only `idx_runs_coordinator_actor` is never repaired
## Affected sites
- `src/main/runtime/orchestration/db/schema/migrate-v42.ts:36` — the `CREATE INDEX` runs only when `current < 42`; `migrateV42` returns early on a v42 stamp.
- `src/main/runtime/orchestration/orchestration-schema-version-skew.ts:162` — `hasCompletePostV6Schema` checks columns plus the fixed `POST_V6_INDEXES` list; this index is in neither, so a v42 stamp missing only the index passes and `migrate` early-returns.
- `src/main/runtime/orchestration/db/orchestration-db.ts:31` — `createTables` runs before `migrate` on every open, but it cannot create this index (the column may not exist on a pre-v42 database at that point), so nothing else ensures it.
## Required outcome
- Decide whether a v42 database missing only this index should be repaired on open (for example, create it after `migrate`, where the actor column is guaranteed to exist) or explicitly left unsupported.
## Open questions for the human (optional)
- The intermediate commit `bd3a8fe` removed the index; any dev profile opened with it is stamped v42 without the index. Is that covered by the "dev databases from earlier builds are unsupported" stance, or should it be repaired?ℹ️ Nitpicks
- The PR description's index inventory still says
idx_runs_coordinator_actoris "nothing queries it" and that "the next PRs ... never search Runs by actor", which now contradicts this commit's rationale that the caller lookup needs it. Update that row so the description matches the code.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
"Actor" read as a new concept when the columns only ever named a structured session. Rename them to what they hold: coordinator_orca_session_id (with its _generation), assignee_orca_session_id and creator_orca_session_id, plus the matching indexes, still added by migrate-v42 since v42 has not shipped. The columns now store the bare Orca session id rather than session:<id>. The session:<id> mail address is derived from ORCA_SESSION_ADDRESS_PREFIX where mail needs it: the coordinator address triggers and the cache refill share one SQL builder. isOrcaSessionId keeps refusing terminal-handle-shaped ids, and the generation rule and backfill evidence rules are unchanged. A dev database stamped v42 with the earlier *_actor columns replays the chain and gains the new ones.
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta (a38ebdd → 246cf56): one commit that renames the "actor" vocabulary to "Orca session id" and moves the stored representation from the addressed spelling to the bare id.
- Codec renamed and simplified —
src/shared/orchestration-actor.tsbecamesrc/shared/orca-session-address.ts. TheOrchestrationActor{kind, id}codec is gone, replaced byisOrcaSessionId,formatOrcaSessionAddress, andparseOrcaSessionAddress(which still requires thesession:prefix and returns the bare id, preserving the terminal-handle refusal). - Columns and indexes renamed —
coordinator_actor/assignee_actor/creator_actorbecamecoordinator_orca_session_id(+_generation) /assignee_orca_session_id/creator_orca_session_id, with matching index names; the v42 skew-probe entries, row-column lists, types, and receipt stripping all follow. - Stored value is now the bare id — the columns hold the bare Orca session id and the mailbox address
session:<id>is derived in one place,currentRunCoordinatorAddressSql, used by the cache seed and both coordinator-address triggers. The backfill validates throughisOrcaSessionIdand writes the bare id. - Tests renamed/updated — suites moved to their new names, the v41-simulation and dev-replay migration tests assert the new columns, and the prior stale-comment thread at
orchestration-actor.test.ts:21is addressed (the test moved and the comment now reads "A bare id is what the columns store, not an address").
I read the incremental and full diffs, traced the address derivation (COALESCE(handle, 'session:' || generation-gated id)), and confirmed that grep finds no remaining _actor / orchestration-actor references under src or tests outside the intentional legacy-name strings in the dev-replay test. The storage-format change is observable only through the mail cache and triggers, both of which derive the identical session:<id> address, so behavior is unchanged. All changed suites pass locally: the four renamed unit suites (45 tests), plus the column-migration, coordinator-address, receipt, and delivery-downgrade contract files (18 tests).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
… the handle first The v42 coordinator triggers and the on-open refill stored one address, COALESCE(handle, session address), so a structured worker coordinator was remembered by its handle only. Remember each address the coordinator has, its handle and its current session address, each where present, so this cache follows the same rule as bindRun and no precedence is persisted.
Review status (round 2): ready to merge at
|
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Reviewed the incremental delta (246cf56 → 72552cd): one commit that makes the coordinator-address cache remember every address a Run coordinator has, instead of picking the handle first.
- Cache stores both addresses —
rememberCurrentRunCoordinatorHandlesand themigrate-v42insert/update triggers now write the coordinator's handle and its derivedsession:<id>as two separate rows instead of a singleCOALESCE(coordinator_handle, 'session:' || id)value, so a coordinator that has both is reachable at either address. - Address derivation split out —
currentRunCoordinatorAddressSqlbecamecurrentRunCoordinatorSessionAddressSql(the session address only, still generation-gated); the handle is no longer folded into it. - Tests pin the split — the address suite asserts a structured-worker coordinator is remembered by both its handle and its session address across the insert, update and reopen-refill paths, that a stale-generation id adds no address, and that a PTY coordinator stays handle-only; the v41-rollback and migration assertions now expect both rows.
I read the incremental and full diffs, traced both write paths (the open-time refill and the v42 triggers) and the production reader path (getRunMailboxOwnerIdsForHandle, the coordinator-mail trigger, and routeForeignDirectMessagesToOwnedMailboxes), and confirmed the writes are idempotent (INSERT OR IGNORE) and generation-gated. The changed suites pass locally: 20 tests across the coordinator-address, handle-migration, and column-migration files. The two prior pullfrog threads are resolved.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…is change does not add The shared codec's comment named ORCA_AGENT_SESSION_ID, which nothing in this change defines, and ran one line past the wrap. It now says the stored id is the one the agent is addressed by (a /clear'd chat's lineage root), as the column comments do, and that PTY agents have none today rather than never. migrate-v42's note stated the lineage rule twice; it is folded into one sentence.
Review status (round 3): ready to merge at
|
1 participant
ELI5
Orchestration is Orca's system for one agent (the coordinator) handing out work to other agents. Each batch of coordinated work is a Run, and each individual assignment is a Dispatch. Today, orchestration writes down who each participant is by the terminal it runs in.
A native chat (Orca's own chat panel) has no terminal. So there is nowhere to write down who it is, and when a chat tries to coordinate work it is refused. This PR adds the missing place: the orchestration database can now also record a participant by its Orca session id, the id Orca already gives every chat session. It is Orca's own id, not the id Claude or Codex keep for their conversation.
This PR is the foundation only. Nothing writes the new id yet except a repair of existing rows that runs when the database opens, so almost nothing a user can see changes here. The next PRs in the stack (#22555, #22568, #22631, #22636) make the app and the
orcaCLI actually use it.What Changed
Orchestration skill: no changes in this PR
The instructions agents receive for using orchestration (
skills/orchestration,skill-guides/orchestration.md,skill-stubs/orchestration.md) are untouched. #22636, later in the stack, adds the guide text: a chat is reachable atsession:<id>(its Orca session id, never the provider's), andorca status --jsonreports the caller's address.What a user sees
From this PR alone: almost nothing. A native chat still cannot run orchestration, and terminal agents are completely unchanged. The observable effects:
orchestration.dispatchandorchestration.dispatchShow) return two extra optional fields.orca orchestration send --to session:<id>to a structured worker (a worker that runs as a native chat instead of a terminal) that coordinates a Run now reaches that Run's mailbox, the same as sending to its terminal handle. Before, it failed with "terminal not found".From the whole stack:
orca orchestration run-createis refused with "This chat session has no orchestration identity of its own…" and told to drive a worker terminal by hand instead.orcaCLI send that id, feat(orchestration): deliver worker results to a structured chat coordinator #22631 delivers worker results back to a chat coordinator, and feat(orchestration): tell each agent its own orchestration address #22636 tells each agent its own address.The new model
How orchestration names a participant:
Three new database columns hold it. Each is optional (empty for every terminal agent):
runs.coordinator_orca_session_id: the chat coordinating this Run. It is paired withruns.coordinator_orca_session_id_generation, and counts only while that equals the Run'sconsumer_generation(see Architecture review).dispatch_contexts.assignee_orca_session_id: the chat this Dispatch is assigned to.dispatch_contexts.creator_orca_session_id: the chat that created this Dispatch.Each column holds the id the agent is addressed by. For a chat that has been
/cleared, that is the id of the first session in its chain (its "lineage root"), which stays stable across/clear. It is not necessarily the id of the session that is live right now. The column comments say so.For agent-to-agent mail, a chat's address is
session:<id>, next to the existingrun:<id>anddispatch:<id>addresses. The columns store the bare id. The address is always built from it and never stored in these columns. The id holds only identity: host and workspace are looked up from the session record when a reader needs them.How agents refer to each other, and to chats, once the stack lands
Every participant in orchestration has exactly one address: the string you put after
--to. Agents copy addresses from command output; they never build one from parts.term_…session:<Orca session id>…_orca_session_idcolumns. The address is built from it.run:<run id>dispatch:<dispatch id>@all,@idle,@claude,@codex, …What
<Orca session id>is. It is the id Orca mints for the chat. It is not Claude's or Codex's own session id, which changes on/clear. For a chat that has been/cleared, it is the id of the first session in the chain, so the address never changes over the chat's life. A worker that runs as a chat issession:<id>too. Its internalstructworker_…key is never handed out as an address.How a chat finds its own address (#22636):
orca status --jsonreturns it ascaller.address:session:<id>in a chat, or the handle in a terminal.How a chat acts as itself (#22555, #22568):
ORCA_AGENT_SESSION_ID).orcaCLI sends that id, and the host checks it and treats the caller assession:<id>. The chat never passes--fromor--terminal, and naming another agent with them is refused.Three common flows:
orca orchestration run-create, and the Run records its coordinator inruns.coordinator_orca_session_id(this PR's column; refactor(orchestration): resolve every caller to one orchestration actor #22555 writes it).worker-startlaunches workers. A Claude or Codex worker runs as a chat or a terminal depending on the user's launch-mode setting.run:<id>.check --wait. Instead, when mail arrives, Orca starts a new turn in the chat ("You have n orchestration message(s)") and the chat runscheck(feat(orchestration): deliver worker results to a structured chat coordinator #22631).orca orchestration send --to dispatch:<dispatch id> …. This is the same whether the worker is a terminal or a chat.orca orchestration send --to session:<id> …. If that chat coordinates a Run, the mail lands in the Run's mailbox, the same as mail to a terminal coordinator's handle.What this PR contributes to that picture:
coordinator_orca_session_idand its generation,assignee_orca_session_id,creator_orca_session_id);session:<id>(src/shared/orca-session-address.ts);session:<id>so mail sent to it reaches the Run.Everything that uses them arrives in #22555 through #22636.
The pieces
src/shared/orca-session-address.ts). This is the one place that decides what a valid Orca session id is.isOrcaSessionIdrefuses any id shaped like a terminal handle (term_…,structworker_…). Handles use the same characters, so without this rule a handle passed in by mistake could become a permanent chat identity. A test mints a real worker handle and checks that it is refused. The file also builds and reads thesession:<id>address, from one exported prefix.migrate-v42.ts, schema version 41 → 42). A migration is the step that upgrades the database layout on launch. This one adds the four columns and two lookup indexes. The indexes only include rows that have an id, so they stay empty for terminal-only users. It also replaces two triggers (rules inside the database that run automatically when a row changes) so the coordinator address book remembers every address a coordinator has.structured-worker-orca-session-backfill.ts). Structured workers already exist, so some existing rows already belong to a session. Each time the database opens, rows that provably belong to exactly one such session get their id filled in. Ambiguous or invalid evidence leaves the row empty, and an id a writer recorded is never rewritten. Pane keys are never used as evidence, because a pane outlives the agent in it.db/runs/run-coordinator-orca-session.ts). A Run's coordinator id counts only at the generation it was written at. Every rebind or unbind bumps that generation, including one made by an older Orca that cannot see the new column, so a leftover id stops counting on its own. This file holds that rule, in TypeScript and in SQL. The writers in this PR that change who a row names also clear the id: rebinding or unbinding a Run's coordinator, reassigning a worker, and restoring identity after a failed start.run_coordinator_handles) remembers every address a Run's coordinator has had, so mail sent to any of them lands in the Run's mailbox. It now stores the handle and, when the coordinator's id is current,session:<id>, as separate rows. Neither takes precedence. All three existing readers match by exact string, so none of them changed.assignee_orca_session_idandcreator_orca_session_idas optional fields. Adding optional fields is safe when client and host run different versions. No released build ever published the fields' earlier names.Terms this PR introduces, and who uses them
In this PR the database layer only clears, fills and derives from these. The one real reader is the mail address book. "Next PRs" means #22555 through #22636. Files are under
src/main/runtime/orchestration/unless shown otherwise.The idea and how it is written
/cleared chat, the lineage root'sORCA_AGENT_SESSION_ID(#22568) so the host can tell who is callingsession:<id>(session address)isOrcaSessionIdORCA_SESSION_ADDRESS_PREFIX,formatOrcaSessionAddresssession:prefix, and building an address from an idparseOrcaSessionAddressDatabase columns and indexes (schema v42)
runs.coordinator_orca_session_idruns.coordinator_orca_session_id_generationconsumer_generationwhen the id was written; the id counts only while the two matchcurrentRunCoordinatorOrcaSessionId,…Sql,currentRunCoordinatorSessionAddressSqldispatch_contexts.assignee_orca_session_iddispatch_contexts.creator_orca_session_ididx_runs_coordinator_orca_session_ididx_dispatch_assignee_orca_session_idPlumbing
backfillStructuredWorkerOrcaSessionIdsdb/orchestration-db.ts)migrateV42,SCHEMA_VERSION = 42STRUCTURED_WORKER_HANDLE_PREFIX,STRUCTURED_WORKER_INCARNATION_PREFIX(now exported fromsrc/main/runtime/structured-worker-identity.ts)Why
Root cause. Orchestration's only vocabulary for "who is this" was terminal-shaped: a handle, a pane, a process. A chat has none of those. Every code path that needed the caller's identity had nothing to write down, so it refused. The fix gives the schema a second, first-class way to name a participant, instead of dressing chats up as terminals.
Where this is heading. The long-term plan is that every agent, terminal agents included, gets an Orca session id. The terminal handle, pane and process then become details of where the agent is running right now, not who it is. That is not built yet: today terminal launches get no session record. This PR is compatible with that plan. The columns are defined as "the Orca session id the agent is addressed by, when it has one (today only structured sessions)", so terminal agents can start filling them later with no schema change.
Why this over the alternatives:
coordinator_handleto older clients that treat it as a terminal. The renderer focusesassignee_handleas a terminal. And it would bake a session id into exactly the column the long-term plan demotes to "where the agent is running". Terminal identity works today, so it stays where it is.structworker_…handle). Every reader of a handle assumes a real terminal behind it that it can look up, deliver to or check. A made-up one fails those lookups, or every such reader grows a special case. It also blurs "this is a terminal" with "this is a chat", which is why the id check refuses handle-shaped ids.pane:<paneKey>, which refactor(orchestration): add actor principal columns and dual-write (1/6) #19943 did). A pane is a location, not a participant, and it outlives its occupant. A pane-keyed identity, and the mailbox that goes with it, would be inherited by the next agent that opens in that pane.session:<id>instead of the bare id. The address is derived from the id, so storing only the id leaves one spelling to validate and nothing that can disagree with it. A first version of this PR stored the prefixed form; it was changed.runsanddispatch_contexts. Validation lives inisOrcaSessionIdinstead.assignee_orca_session_idbecause it can be derived. Today it can: every assignee with an id is a structured worker, whose process record readsstructured:<id>. Once terminal agents get Orca session ids, that stops being true, and the next PRs look Dispatches up by this column through its index. Declined.This PR supersedes #19943 (not closed here). It keeps that PR's migration techniques and fixes its bug: #19943 registered its new columns at the wrong schema version, so every existing database failed a completeness check and replayed its whole migration chain from v6. This PR registers them at 42, and a test checks that a v41 database starts upgrading at 41.
Why the column is named for the Orca session id
The columns were first called
…_actor. A reviewer found that confusing, and it also said nothing about what the value is.…_agentwas out because "agent" already means several things here, such as which CLI (--agent codex) and the agent's terminal. A bare…_session_idis easily confused with Claude's or Codex's own session ids. The value is Orca's id: the session record id, and for a/cleared chat the lineage root's.…_orca_session_idsays exactly that. It also fits the plan for every agent to get one. Column names are permanent once v42 ships, so now is the cheap moment to object.Architecture review
An independent architecture review looked at the renamed design. Verdict: keep the design, adjust it. Both adjustments are in this head.
Accepted:
bindRunalready does.Declined: dropping
assignee_orca_session_idas derivable, for the reason under Why.Review passes: five in total: three before the rename, one over the rename and the address-book change, and a final rename-only pass that fixed two comments. The second found that a lookup index had been wrongly removed as unused. #22555's caller lookup needs it, so it was restored and a query-plan test now checks it on fresh and upgraded databases. The other three passes were clean. An unused helper that accepted either spelling was removed.
The generation rule. Without it,
runs.coordinator_orca_session_idwould be a stored value that nothing re-checks, and there is no other record in the orchestration database to check a chat coordinator against. Here is the failure it prevents. After a rollback, an older Orca (schema v41, the current release) rebinds a chat-coordinated Run to a terminal and later unbinds it. It cannot clear a column it doesn't know exists. The Run is left with no handle, no pane and the chat's id, which is exactly what a live chat binding looks like, so the chat would be treated as coordinator of a Run it no longer owns. Clearing the id in every writer can't fix this, because the writer that leaves it behind is the old binary. Instead the id is stored with the generation it was written at. Every Orca version, old or new, bumpsconsumer_generationon each rebind or unbind, so the leftover id stops counting.So any Orca version can make an id stop counting: v42 by rebinding or unbinding (it also clears the id), and v41 after a rollback by doing the same. That is safe because the rule can only fail in one direction. A stale id can never count. The worst an unexpected generation bump could do is make a valid id stop counting, and then the chat has to bind the Run again. Today only the two Run rebind/unbind writers bump
runs.consumer_generation, and the column comment asks that it stay that way. The address book triggers, their refill on open, and the fill all use the rule.Known edges, all inert today:
mintDispatchCapabilitymoves a Dispatch to a new pane and process without touchingassignee_orca_session_id. Its only caller mints on a Dispatch created in the same request, so the two cannot disagree. A future caller that mints on an existing Dispatch must write the id in the same statement.session:<id>whose session record was deleted leaves the address pointing nowhere, the same as a deadterm_handle does today. The address book still stores session addresses in a column namedterminal_handle.Linked Issue
None. Part of the structured-chat orchestration program. Supersedes #19943.
Visual Proof
N/A: no UI or interaction change; this PR is the orchestration database layer only. As evidence that the app upgrades and boots cleanly on a real database (details under Testing):
First boot after the upgrade:
Second boot on the upgraded database:
Testing
Checks at this head (
0bfebd79dc; the last commit changed comments only, and the suites were re-run on it):pnpm tc:node,pnpm tc:cliandpnpm tc:webpass.oxlint, the changed-code quality gate andoxfmtare clean.What the tests cover (8 test files). A fresh database holding a chat coordinator with no handle, and mail reaching it. Upgrades from v41 and v40 that start at their own version instead of replaying the chain, and one from before the address book existed. An older (v41) build reading and writing the upgraded database, and rebinding then unbinding a chat-coordinated Run. The old-row fill: exactly the provable rows, empty on ambiguous evidence, never rewriting, and repairing rows written after the version stamp. Writers not leaving a stale id behind. The id check never accepting a terminal handle. The address book remembering terminal coordinators by handle only, a structured worker by handle and session address, and nothing for a stale id. Dev databases from earlier builds. The lookup index on fresh and upgraded databases. The rename kept every test: 63 tests in 8 files before and after. The final commit added the address book tests for terminal coordinators, structured workers and stale ids.
Mutation checks. I deleted each mechanism in turn, re-ran the tests, and confirmed each deletion turns tests red:
session:+ id in the address derivationAn earlier round of 19 deletions at
9d8e5734de, before the rename, covered version bookkeeping, trigger replacement, the fill and its evidence rules, the handle refusal, the writer clears, the address book, the reply stripping and the optional column shape. All 19 turned red. The rename kept every one of those tests.Real-database upgrade (Electron, background launch, isolated profile), at
72552cd9b6(the only later commit changes comments). The branch app was pointed at a fresh copy of a real v41 orchestration database: 1,601 Runs, 8,929 Dispatches (917 of them structured-worker), 56,826 messages. After first boot:integrity_checkis ok, and every table's row count is unchanged.session:<id>rows, one per Run with a current coordinator id, and 0 existing rows lost.A second boot changed nothing. The real database was never opened and is still at v41.
Fill cost: about 25 ms per open on an extreme database (102,000 Dispatches), and about 10 ms on a 51,000-row benchmark earlier.
Not tested: Windows, Linux and mobile. The change is SQL and TypeScript only, with no platform branches and no mobile surface.
AI Disclosure
Review
Questions worth an explicit reviewer decision:
tasks.created_by_*, because a chat with no handle creates root tasks.runs.consumer_generationis bumped only when a Run's coordinator is rebound or unbound. That holds for every writer today, and the column comment says so. If something else bumped it, a valid id would stop counting until the chat binds again. It could never make a stale id count.assignee_orca_session_id, and the fill never rewrites an id. Is relying on the writers' clears acceptable, or should Dispatch ids get the same generation treatment?…_orca_session_idbecomes permanent once v42 ships.Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Rollback: a v41 Orca opening a v42 database keeps working. Its migration step sees the newer version and returns early. Its reads and writes work, and its inserts leave the new columns empty. Its Run list replies can show the new columns, which can't be prevented from this side. It cannot clear the coordinator id when it rebinds or unbinds a Run. It does bump the generation, so the id stops counting; this is tested with a v41 rebind followed by an unbind. On the next v42 open, the fill repairs structured-worker rows the old binary wrote, including a coordinator id left at an older generation.
Dev databases from earlier builds of this branch (stamped v42 with
*_actorcolumns) and from #19943 builds (*_principalcolumns) are unsupported. The completeness check sees the new columns are missing and replays the migration chain. That adds the new columns and leaves the old ones in place but unread, so a chat coordinator recorded only in an old column is not carried over (tested).Left for the next PRs, on purpose:
session:<id>the same way as mail addressed to its handle (refactor(orchestration): resolve every caller to one orchestration actor #22555).orcaCLI carry the chat's id, so a chat can run orchestration as itself (feat(orchestration): let a structured chat run orchestration as itself #22568)./clearchain to its lineage root (feat(orchestration): deliver worker results to a structured chat coordinator #22631).Environments:
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)