refactor(compass): SEA-1516 collapse the container hop and record agent placement

[seal-agent]

Collapses a redundant hop out of the SubscribeAgentSession authorization chain, and records the fact reattach recovery needs and nothing currently holds: which Runner each agent is on.

Two problems, one migration

1. agent_containers sat inside a security boundary carrying a fact that is not authorization.

The table (0003:24-27) was container_name PK + agent_account_id, and its only read anywhere was the authz JOIN at agent_sessions.go:99. But the hop could never do more than pass the account through — container_name is the PK and the FK to the account is NOT NULL, so it is strictly 1:1. It could neither drop nor multiply a row.

Worse, it stored a derivable string: spec.go:73 builds the name as NamePrefix + accountID, a pure function of the agent account.

2. Nothing recorded which Runner an agent runs on. No runner_id column exists anywhere. That matters because a Runner link drop kills the process while its containers survive (Launch sets no --rm), so after a restart the Server must re-drive Provision for exactly the agents that Runner held — a set it could not name.

What changed

agent_sessions points at agent_account_id directly, agent_containers is dropped, and agent_placements (agent_account_id PK, runner_id, container_name, timestamps) records placement as operational state, deliberately outside the authz path.

The chain shortens from session_id → container_name → agent_account_id → home_channel_id to session_id → agent_account_id → home_channel_id.

Why container_name lives on agent_placements rather than being recomputed. My first instinct was to derive the name, but NamePrefix is Runner-side config (cmd/compass-runner/main.go:112) that the Server never sees — it can neither compute nor invert it. The Server does, however, learn the name at Provision, where it holds every fact at once: agent_account_id is its own request field, container_name is the Runner's response, runner_id is the Runner it relayed to. Previously it dropped that triple into an in-memory binding and nowhere else.

So this closes a real durability window rather than just reorganizing tables: before it, a Server restart or Runner re-enroll between Provision and Start left StartAgentSession unable to say whose session it was recording. Start now resolves the owner from a durable read.

The authz query is the risk, so it got the most scrutiny

-- before                                    -- after
FROM agent_sessions se                       FROM agent_sessions se
JOIN agent_containers c                      JOIN agent_accounts ag
  ON c.container_name = se.container_name      ON ag.account_id = se.agent_account_id
JOIN agent_accounts ag                       JOIN channel_members cm
  ON ag.account_id = c.agent_account_id        ON cm.channel_id = ag.home_channel_id
JOIN channel_members cm                      AND cm.account_id = $2
  ON cm.channel_id = ag.home_channel_id     WHERE se.session_id = $1
 AND cm.account_id = $2
WHERE se.session_id = $1

The removed hop was provably 1:1, so the set of (session, caller) pairs authorized is identical. (The in-code comment originally justified this by the derived container name — a Runner-side convention the Server never enforces. It now rests on the schema fact, which holds regardless of how any Runner names things.) Still a single constant-shape EXISTS, so 0003's timing-parity argument survives. agent_placements is not in this query — placement is where an agent runs, not who may watch it, and keeping them apart is what stops the hop from growing back into the security boundary.

Every existing authz test passes unchanged, including the not-found/forbidden merge and the end-to-end parity test through the real interceptor chain. Added TestRequireAgentSessionSubscriberCollapseIsUniformAcrossRefusalCauses, which pins four structurally distinct refusals against each other — unknown session; a foreign session where the caller owns a different agent so the chain resolves fully and only membership fails; a caller belonging to nothing; an owner probing a rival's session — asserting the same sentinel and the same message, plus that the legitimate owner still authorizes, so uniform refusal is not universal refusal.

Mutation-proven, not assumed. Weakening the membership join to ON TRUE fails both the pre-existing collapse test and the new one. I re-ran that mutation myself rather than taking it on report.

Backfill proven separately, because a fresh-schema migration test never exercises it: a 0003-era schema seeded with 2 agents / 2 containers / 3 sessions, then the migration's agent_sessions half run verbatim — all 3 rows survived carrying the account the old JOIN resolved.

Deliberate choices worth flagging

• runner_id is not a FK. Runners enroll in memory under a token subject with no runners table, and a placement must outlive the Runner's attachment — that is the entire point of recording it.

• PK on the agent, not a surrogate. An agent is on at most one Runner under one name, so a re-provision replaces the row. A stale container_name beside a fresh runner_id would be worse than no row.

• container_name is UNIQUE, so a second agent can never claim a live container name — which would let Start record a session under the wrong owner and hand the authz JOIN the wrong home channel.

• Placements ARE backfilled, with an explicit sentinel. My first revision declined to backfill, reasoning that a pre-existing container's placement was "unknowable" since agent_containers recorded no Runner. Review caught that this was an upgrade-path break, and it was right: the same migration makes agent_placements a hard dependency of StartAgentSession, so every container provisioned before the upgrade became permanently un-Startable — and it failed after the Runner had already started a live agent, stranding a session the Server had no record of. It was only "unknowable" because the migration dropped the source table before anything could read it.

  Two of the three columns were sitting right there in scope, so the backfill inserts them with runner_id = '' as a documented sentinel. That value is exactly right in both directions: AgentForContainer resolves it so Start keeps working, while ListAgentPlacementsForRunner rejects an empty runner id and so never surfaces a backfilled row to reattach — correct, because we genuinely do not know which Runner held it. The next provision overwrites it with the real Runner through the existing upsert. runner_id stays NOT NULL; the empty string satisfies it, which is stronger than making the column nullable.

  Proven empirically: a 0003-era database seeded with a provisioned-but-never-started container — the exact broken case — now resolves through the literal query Start issues.

• StartAgentSession no longer strands a session it cannot record. hub.Start is an irreversible side effect. If the placement read or the ownership write then failed, the handler returned an error and discarded the session id, leaving a running agent that could not be stopped, reloaded, or subscribed to through any API. It now best-effort Stops the session on context.WithoutCancel (matching the failed-Launch cleanup pattern in internal/runtime/agent.go), logs at ERROR with the session and container, and names the session id in the returned error so a failed rollback still leaves an operator able to reap it.

  This is not symmetric with Provision, and the comments no longer imply it is: Provision's write is an idempotent upsert whose client retry dedups at the router via client_request_id, so it self-heals. StartAgentSessionRequest carries no client_request_id, so a retry issues a genuinely new Start — its partial state was unrecoverable.

• RecordAgentPlacement's docstring no longer oversells. It presented refusing a container name held by another agent as a live safety property, but nothing releases a name and names are derived from the account (spec.go:73), so the cross-agent conflict is unreachable in production. It now says placements are permanent for now, names what the unique index actually leans on, and states that changing name derivation makes the conflict reachable and needs a release path added alongside. No delete path was added.

Scope

This lands the record reattach recovery reads; it does not implement reattach (re-driving Provision on reattached), which is separate work in this lane. The placement accessors are real and tested, not stubs, and Provision genuinely writes the row.

The handler seam had zero coverage — that is where every defect lived

The store primitives were each individually correct and all their tests stayed green under both bugs; it was their composition in the handlers that was wrong, and no test drove either changed handler. Three pgtests now do, against a fake Runner dialing the real mounted RunnerService door, so a rollback Stop is observed as an actual wire command rather than a mock call:

• StartAgentSession succeeds for a backfilled placement (runner_id = ''), records the ownership row, and stays invisible to reattach.
• StartAgentSession on an unplaced container returns CodeInternal, names the session id, the Runner receives a Stop for that exact session, and agent_sessions is left empty.
• ProvisionAgentWorkspace records a placement naming the Runner that actually served the relay.

I mutation-tested the rollback myself: removing the Stop turns the second test red with "the started session is stranded".

Verification

go build, go vet -tags "pgtest unix", gofmt -l (empty), -race unit suite, golangci-lint zero findings in hand-written code. Real-Postgres pgtest run with -v against a live DSN showing genuine PASS lines — 6 placement tests, 5 authz tests, and the 3 new handler tests, no skips. (The pgtest suite silently skips when no container runtime is available, so a bare ok proves nothing; these were confirmed executing.)

Spec-impact: none

Internal persistence and one unchanged authz predicate; no externally-visible behavior changes.

Targets main for review; lands in sealedsecurity/compass after the port.

---

Ported from sealedsecurity/sealed#998, unchanged in content — Compass code now lives here, so this is where it ships. The sealed PR is closed in favour of this one.

Verification (this repo has no CI yet, so a local run is the only evidence there is): go build ./..., go vet -tags "pgtest unix" ./..., gofmt -l clean, go test -race ./internal/... ./server/..., and the pgtest suites for ./internal/store/, ./internal/comms/, ./server/ against a live postgres:17 — all green on this branch.

[linear-code]

SEA-1516

[seal-agent]

Cross-branch integration verified locally. This repo has no CI yet, so this is the only integration evidence there is — and it is the check that caught a real defect in the sealed copies of these same diffs (a deleted symbol still cited by a sibling branch, invisible to any single-diff review).

Merged all five of this lane's branches together onto main — #29, #30, #31, #32, #33 — in one tree:

merge boundary rc=0   merge drop rc=0   merge ask rc=0   merge t3 rc=0   merge schema rc=0

All five merge clean, no conflicts. On the combined tree, against a live postgres:17:

gofmt -l           clean
go build ./...     BUILD_OK
go vet -tags…      VET_OK
go test -race ./internal/... ./server/...        all pass
go test -tags pgtest ./internal/store/...        ok  39.358s
go test -tags pgtest ./internal/comms/...        ok  23.879s
go test -tags pgtest ./server/...                ok  20.286s

Worth stating plainly: ./server/ only goes green here because #29 is in the tree. On main today TestServeShutdownWithLiveCommsSubscriberReturnsClean fails against a live Postgres — pre-existing, confirmed on a clean main checkout — and #29 is the fix. The other four each show that same failure alone, and it is not theirs.

A per-package throwaway Postgres container also starves under this suite's parallelism (the store package hit its 600s timeout four times over); one shared database runs the same suite in ~30s. Relevant when CI is set up here.

[greptile-apps]

Greptile Summary

This PR replaces container-mediated session ownership with direct agent ownership and introduces durable agent-to-Runner placement records.

• Adds placement persistence and Runner-scoped placement lookup.
• Migrates session ownership and legacy container mappings.
• Records placement after provisioning and resolves ownership durably when starting sessions.
• Adds bounded rollback of sessions that start but cannot be recorded.
• Expands migration, authorization, placement, and handler integration coverage.

Confidence Score: 3/5

The PR is not safe to merge until the migration preserves every usable legacy container mapping.

The pre-upgrade schema permits multiple containers for one agent, but the placement backfill retains only one before deleting the source table; starting any discarded container then fails ownership resolution and rolls back the live session. The previously reported unbounded rollback wait is fixed by the new independent timeout.

Files Needing Attention: go/internal/store/migrations/0004_agent_placement.sql

Important Files Changed

Filename	Overview
go/internal/store/migrations/0004_agent_placement.sql	Migrates session ownership and creates placement state, but the legacy backfill still loses additional containers belonging to the same agent.
go/internal/store/agent_placements.go	Adds validated placement upserts and lookup methods for container ownership and Runner recovery.
go/internal/store/agent_sessions.go	Replaces container-based session ownership with direct agent-account ownership and preserves channel-membership authorization.
go/internal/runnerhub/commands.go	Returns the serving Runner identity from provision relays so placement records identify the actual Runner.
go/internal/runnerhub/hub.go	Extends router lookup to atomically return the attached Runner identifier with its router.
go/server/service.go	Persists placements, resolves session ownership from them, and bounds best-effort rollback Stops with an independent timeout.
go/server/service_placement_pgtest_test.go	Adds integration coverage for placement recording, legacy sentinel behavior, rollback, and rollback timeout handling.

_{Reviews (3): Last reviewed commit: "fix(server): bound the rollback Stop so ..." | Re-trigger Greptile}

[sealedsecurity-bot]

Review of record — BLOCKING on 1 finding (medium), parked as an Open Question for Matt

Ran the mandatory review (skill://review, Opus). One gating finding; Greptile's two P1s both adjudicated down.

OQ (medium, gating) — the rollback Stop can hang StartAgentSession unboundedly

abandonStartedSession (go/server/service.go:376) tears down a just-started-but-unrecorded session with s.hub.Stop(context.WithoutCancel(ctx), …). I traced the whole dispatch path (Hub.Stop → relay commands.go → commandRouter.dispatch → waitCall) and grep of the runnerhub production code found zero WithTimeout/WithDeadline/time.After — there is no independent deadline anywhere. context.WithoutCancel strips the caller's deadline and nothing replaces it, so a wedged-but-connected Runner that accepts Stop but never answers hangs StartAgentSession forever and stalls graceful-shutdown drain. The rollback is strictly worse than the normal relay path, which at least dies on client disconnect.

Note the code comment claims "The relay's own per-call bounds still apply" — that is false; there are no per-call bounds.

Compound precondition (store write fails post-relay AND Runner silent-but-connected) keeps it at medium, but it is above the gating floor. Fix shape is a self-contained control-flow change → Matt's call, parked here per rule://decision-authority:

1. (recommended) Fix in-port now — stopCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), <const>); defer cancel(); pass stopCtx to hub.Stop. Preserves the intent (survive caller cancellation) while guaranteeing StartAgentSession returns within a bound. Correct the false comment. Add a test: fake Runner withholds the Stop result → StartAgentSession returns CodeInternal within the bound, not hanging.
2. Defer — merge as-is, file a SEA follow-up I own for the bounded-rollback fix + test.

Adjudicated, non-gating

• Greptile P1a (migration drops legacy containers, 0004:143) — mechanism real but moot under the current invariant: container_name = NamePrefix + accountID is deterministic per agent and RecordAgentContainer used ON CONFLICT (container_name) DO NOTHING, so an agent yields exactly one row; multiple rows require a NamePrefix change between provisions, not a current path. The collapse to one-placement-per-agent is lossy by design. The migration's own comment reasons this correctly. Optional hardening: make the loss loud (RAISE EXCEPTION on a multi-row pre-check) for the day name-derivation changes.
• Greptile 'runner-unscoped attribution' — not an issue: ownership resolves by container_name (unique index), runner_id is operational state for reattach, never authz.
• Low: the non-responsive-rollback path is untested (the existing rollback test uses a Runner that answers Stop).

Migration is otherwise data-preserving, atomic (per-migration tx), FK-integral. Holding merge-ready pending Matt's ruling on the OQ.

[mattwilkinsonn]

• refactor(compass): SEA-1516 collapse the container hop and record agent placement #33 👈 (View in Graphite)
• main

This stack of pull requests is managed by Graphite. Learn more about stacking.