Skip to content

refactor(orchestration): resolve Run callers through one principal resolver (2/6) - #19949

Closed
brennanb2025 wants to merge 3 commits into
brennanb2025/principals-01-columnsfrom
brennanb2025/principals-02-resolver
Closed

brennanb2025 wants to merge 3 commits into
brennanb2025/principals-01-columnsfrom
brennanb2025/principals-02-resolver

Conversation

@brennanb2025

@brennanb2025 brennanb2025 commented Sep 11, 2026 •

Copy link
Copy Markdown
Contributor
Files Added Deleted Net
Test 5 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​623 0 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​623
Prod 10 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​429 $\color{#cf222e}{\Huge{\mathbf{−}}}$​73 $\color{#1a7f37}{\Huge{\mathbf{+}}}$​356

Summary

Second of six PRs replacing orchestration's terminal-shaped actor identity with a principal. PR 1 added the columns; this PR introduces the single place a credential becomes a principal, and converts the Run methods to it.

resolveCallerPrincipal (new caller-principal.ts) takes any credential form — a --from terminal handle with hook evidence, a structworker_ bearer, or declared session values — and returns one shape: principal, principalId, ownerGeneration, hostScope, workspaceId, an opaque binding for DB writers, and waiterHandles. Methods never inspect the principal's kind.

Why one resolver

The closed PR this stack supersedes branched per method on if (params.agentSessionId) at 21 call sites and shipped six defects that way — skipped prior-run waiter cancellation, a bypassed takeover guard, and changed error text among them. Here the only occurrence of that expression in the entire orchestration method tree is a comment naming it as the defect class the module prevents. runCreate / runUse / runCurrent execute an identical statement sequence for both principal kinds.

The security invariant

A declared agentSessionId / runtimeFence never authenticates. Session ids are public — structuredAgentSessionTabId returns structured-agent-session-${sessionId} and tab ids carry them in plain text — and a fence is a small integer. Accepting a declared pair would let any RPC caller impersonate any native session, and later PRs converting check / send / reply would inherit it as mailbox theft.

Session-kind resolution therefore requires possession of an unguessable bearer. Declared values are corroboration only: a mismatch against the bearer's real identity is a refusal, never an acceptance. A standalone declared session id resolves to consumer_fenced.

Pinned at resolver level and method level, and verified by ablation: re-introducing the declared-id acceptance and bypassing bearer possession reddens exactly two tests, one at each level. Note the defense is layered — removing the explicit refusal alone does not open the hole, because the handle-to-identity lookup is the real gate.

Second invariant: isEquivalentPrincipal never bridges pane↔session in either direction. A session id is recoverable from a structured pane key's tab half, so a fabricated pane:structured-agent-session-<realId>:<attacker-leaf> must not match session:<realId> — asserted in both argument orders. PR 1's backfill does derive session-from-pane, but only as a one-time server-side pass over rows the host wrote; the module doc states that derivation is legal exactly there and never at request time.

A latent bug fixed

The fence schema is z.number().int().min(0). The closed PR used .positive(), which refuses fence 0 — a legitimate value (agent-session-record.ts validates runtimeFence >= 0), so freshly leased sessions would have been silently locked out. Pinned by a test at both levels.

Notable deviation from the plan

The plan described attestDeclaredCaller as a no-op for sessions. It isn't: the session path runs the same evidence assertion as panes. A strict no-op would let a caller attested as term_X present a stolen structworker handle without triggering the mismatch throw, which is weaker than today's runUse. The implemented form is byte-compatible with current behavior and strictly safer, with a pinning test.

Validation

  • 39 new tests across 4 files; existing orchestration and RPC suites green.
  • Full sweep (orchestration + rpc + shared, 1093 files): 11,020 passed. One failure in remote-runtime-shared-control-connection is a known load-timing flake — its failure count varies between identical runs on an unchanged tree, it passes in isolation on both this branch and its base, and no import path connects it to anything here.
  • Typecheck clean on node, cli and web, run sequentially. Lint, format, code-quality and max-lines gates clean — no disables, no bumps.
  • pnpm-lock.yaml absent from every commit and from the branch range.

Stacking

Based on brennanb2025/principals-01-columns (#19943), not main. Review that first.

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • resolveCallerPrincipal resolver — new caller-principal.ts collapses every credential form (term handle, structworker_ bearer, declared session fields) into one shape and enforces the bearer-only session invariant.
  • Run method conversion — runCreate/runUse/runCurrent consume principalId + binding + waiterHandles whole and never branch on principal kind.
  • Principal-aware DB layer — isEquivalentPrincipal, the runCoordinatorBinding legacy normalizer, and principal-scoped bind/unbind/lookup (bindRun, createRun, runsBoundToPrincipal, unbindOtherRunsForPrincipal).
  • resolveRunScope re-routed — now resolves through resolveCallerPrincipal, threading dormant callerAgentSessionId/callerRuntimeFence fields for later PRs.
  • Tests — 595 added lines across 4 spec files pinning the cross-kind invariant, bearer-only authentication, fence-0 acceptance, and remint stability.

The resolver layer is sound: declared session fields never authenticate (bearer possession is the gate), the pane↔session non-bridging invariant holds in both argument orders, and the reversal of from from requiredString to OptionalString plus the two new optional fields is a Rule 1 wire-compatible change under docs/reference/remote-wire-compatibility.md.

ℹ️ resolveRunScope now routes non-Run methods through the session resolver

resolveRunScope is shared by check/send/reply/gate/dispatch and the message methods, and it now calls resolveCallerPrincipal with newly added but as-yet-unpopulated callerAgentSessionId/callerRuntimeFence fields. A structworker_ handle passed as --from to any of those methods today therefore resolves as a session principal (bearer-gated, no corroboration forwarded) instead of failing with stable_pane_required. That is safe — the bearer is the real gate and corroboration is only a mismatch refusal — but it is a behavior expansion on methods the PR title describes as out of scope, so it is worth confirming the sequencing is intended rather than landing inside the check/send/reply conversions.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@pullfrog pullfrog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No new issues found.

Reviewed changes

  • Index-backed principal lookups — runsBoundToPrincipal now serves pane principals from the exact idx_runs_coordinator_principal partial index plus the existing idx_runs_coordinator_pane_leaf expression index (dedup by run id, then rowid sort), and session/unparseable principals from the exact index only, replacing the prior full scan.
  • New partial index — idx_runs_coordinator_principal (coordinator_principal IS NOT NULL AND legacy = 0) is created idempotently in backfillPrincipalColumns, which runs after migration and on every open.
  • Query-plan pinning — a new test asserts both lookups resolve to an index (no SCAN runs), and the migration revert now drops the index before dropping the column.

The rewrite is equivalence-preserving: a pane: principal is only ever persisted alongside a non-null coordinator_pane_key sharing its leaf (every write path derives the principal from the pane key via principalFromPaneKey or the resolver's binding), so the IS NOT NULL pre-filter cannot drop a run the old full scan would have returned, and isEquivalentPrincipal remains the authoritative final gate for cross-kind and malformed values.

Pullfrog  | View workflow run | Using DeepSeek Pro (free via Pullfrog for OSS) | 𝕏

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Review status: PASS on pushed head eb6aee4.

  • The review found and fixed one P2 performance issue in runsBoundToPrincipal: historical nonlegacy runs were being loaded and filtered in JavaScript on frequent run-scope paths. The fix adds the partial principal index, uses exact indexed principal lookup, uses the existing pane-leaf index for pane remint matching, preserves isEquivalentPrincipal as the final authority check, and adds query-plan regression coverage.
  • Security and architecture checks are clean: declared agentSessionId/runtimeFence remain corroboration only, pane/session principals never compare equivalent, and the single caller-principal resolver remains the ownership boundary.
  • Validation: 188 targeted tests across 12 related files, sequential node typecheck, changed-file oxlint/formatting, diff checks, and remote head verification all passed. The unbounded full suite was not used because it produced unrelated failures under host-wide load; real SSH/WSL restart/drop exercises were not run, and no changed visible UI path exists for screenshot proof. The requested Grok Electron evidence lane was attempted but blocked by the provider's weekly quota; mobile QA is not applicable to this backend-only change.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Electron QA status (branch brennanb2025/principals-02-resolver, head eb6aee4d5f):

  • Background Orca launch and CDP attach succeeded on the intended principals-electron-qa worktree. The migration path was exercised separately using an isolated v40-shaped SQLite fixture; it opened cleanly and reached schema v41 (see the refactor(orchestration): add actor principal columns and dual-write (1/6) #19943 comment for exact details and screenshot).
  • I could not complete the live resolver flow from this dispatched worker: the isolated app has no live terminal panes, and orca orchestration run-create correctly refused the synthetic caller with stable_pane_required/consumer_fenced. Therefore this worker does not claim real-app run-create → task listing → orchestration check evidence, nor a structworker_ structured-chat resolution artifact. A live coordinator terminal is required to prove that path.

@brennanb2025

Copy link
Copy Markdown
Contributor Author

Superseded by #22555 (stacked on #22522), part of a fresh orchestration stack for structured-session coordinators.

The design changed after this PR was written. There is no bearer credential and no fence; all agents run on one machine as one user, so the identity is the Orca session id injected into the agent's environment.

#22555 resolves every caller once, at the dispatch entry, to one orchestration actor (session:<id>):

  • It accepts the actor on the local socket and Electron IPC.
  • It refuses it on the paired WebSocket route, naming the host boundary.
  • Methods never branch on actor kind.

This PR's one-resolver principle carries over. Closing in favour of #22555.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant