fix(daemon): fence pool-store transcripts on the owning org - #1075
Conversation
The daemon pool's shared store keyed transcripts on (channel, thread) alone, while the org-fenced pair the data plane declared was constructed and never read or written. Platform channel and thread ids are unique only inside one org, so on a pool store one org's INSERT OR IGNORE could swallow another org's message on the same coordinates, and a (channel, thread) read could serve one org's rows to another org's console. transcript and transcript_recipient gain orgId (NOT NULL): it joins the recipient primary key and prefixes every transcript index. Writes resolve it from the row's recipient, else its sender, else the agent that made the thread live; a store no pool shares owns one partition forever and behaves exactly as before, while a shared store resolves through the daemon's agent registry and refuses a row it cannot attribute. Reads key on (org, channel, thread), taking the org from the calling agent — the callers were already agent-scoped, and no placement-kind branch is involved. telegramThreadForMessage stays unfenced by design and says so: it runs before routing names an agent, its key is the physical-bot-scoped channel one org owns, and it returns a thread id rather than content. Schema version 11 adds the columns, rebuilds transcript_recipient for its new primary key, and drops the transcript indexes so the CREATE block re-emits them org-first. A store no pool shares keeps every row on its single partition; a shared store drops its transcript rows, because nothing in the store records an agent's org and only test data exists on any shared store today. The data plane's own transcript pair goes away, with a second data-plane migration dropping the tables and the sequence, so one store carries the fence.
There was a problem hiding this comment.
Approved — no blocking findings at 92d69c0c84f043e73a9c926e4be606a171d0577c.
The live LocalStore transcript tables now carry the org fence consistently through uniqueness/index keys, write attribution, content-bearing reads, agent-scoped pages/tails, revision cursors, and tool-body lookups. The pre-ingress CP registry barrier supplies the shared store's resolver, local single-owner stores retain their existing partition behavior, and the v9→v10 store migration plus removal of the unused data-plane transcript pair are coherent. The deliberately unfenced Telegram lookup remains limited to a physical-bot-scoped thread-ID locator and does not return transcript content.
Non-blocking rollout warning: if a v9 pool member remains live after a v10 member migrates the shared schema, that old member can still omit orgId and write into the column's default '' partition; v10 org-scoped reads will not see those rows. Given the stated active-development/test-only shared-store state, I do not consider this a blocker, but a coordinated pool drain/restart avoids the mixed-version window.
Verification: git diff --check passed; daemon typecheck passed; 197 focused store, migration, thread-context, and session-manager tests passed. GitHub's current Build, Check, Unit Test, both Integration test shards, and image/sandbox checks are green. A broader local daemon sweep was limited by this review sandbox's /dev/null and child-process execution denials, unrelated to the patch.
sent by review-bot (Codex · gpt-5.6-sol) · open in session
92d69c0 to
4b4a26f
Compare
|
The PR merged before the formal review could be submitted. One blocking issue remains in The ingress-owned org/agent should be carried into this path, or ambiguous fallback attribution should be rejected. Verification: daemon typecheck and 198 focused tests passed; GitHub checks were green. sent by |
Summary
Item 7 of #1041, the last one open: the daemon pool's shared store had an org-fenced
transcript pair in
agentconnect_data_planethat was constructed and never read orwritten, while every transcript actually went to the
LocalStoremirror tablestranscript/transcript_recipient, which carried noorg_idat all. On a pool storethose two tables are shared by every org the pool serves, and platform channel/thread ids
are unique only inside one org — so one org's
INSERT OR IGNOREcould swallow anotherorg's message on the same
(channel, thread, ts), and a(channel, thread)read couldserve one org's rows to another's console.
This is option B from the issue's decision comment: the live tables gain the fence, and
the never-used data-plane pair is deleted so exactly one store carries it.
The decision
transcriptandtranscript_recipientgainorgId(NOT NULL); it joins thetranscript_recipientprimary key and prefixes every transcript index.recipient, else itssender, else theagent that made the thread live. A store no pool shares holds one daemon's threads, so it
owns one partition forever (
'') and its behaviour is unchanged. A shared store resolvesthrough the daemon's agent registry (
orgForAgent, the same resolver the deleted storeused) and refuses a row it cannot attribute rather than filing it where anyone may read it
— a shared
LocalStorenow requires that resolver at construction.(channel, thread)key on(org, channel, thread). The org comesfrom the calling agent; the callers were already agent-scoped, so the only new parameters
are on the four reads that had dropped the agent on the floor (
transcriptSince,transcriptSinceRevision,threadTranscriptRevision,transcriptTextAt). Noplacement-kind branch anywhere.
telegramThreadForMessageruns before routing names an agent. Its key is the physical-bot-scoped transcript
channel — one integration owns that bot, one org owns that integration — and it returns a
thread id, never content.
TranscriptEntry.orgAgentIdis attribution only, never a column and never a delivery, forthe two writers whose rows carry a platform author and no recipient (an observed inbound,
and provider history replayed into a turn refresh).
Migration and backfill
SCHEMA_VERSION10 → 11, one newSCHEMA_MIGRATIONSstep appended after #1068's. It addsthe columns, drops the five transcript indexes so the
CREATEblock re-emits them org-first,and rebuilds
transcript_recipient(its primary key gains the org, whichALTER TABLEcannot do).
Backfill splits by store:
DEFAULT ''is the backfill, deliveries copy across, and reads see exactly what they sawbefore.
the daemon learns it from the CP at runtime — so the sessions → agent → org join resolves to
nothing at migration time, and a kept row would be readable by whichever org reused the
channel/thread ids. Only test data exists on any shared store today, which is why dropping
is the accepted price of the fence; the step's comment records that.
The step is plain SQL through the PostgreSQL rewrite path (
postgres-store-worker.js): nowindow functions, no aliased subqueries.
orgIdis added to the canonical column list inpostgres-sync-database.tsso read-back mapsorgidhome.Deleting the unused fence
PostgresDataPlane.transcriptsandPostgresTranscriptStoreare gone, together with theirunit and integration tests; the module is renamed
postgres-data-plane.ts, since what itstill holds is the pool member's store, not a transcript store. The data-plane schema gets a
second migration dropping
transcript,transcript_recipientandtranscript_revision_seq, so an installed data plane gives the tables back rather thankeeping them forever. The pg pool and
migrateDataPlaneSchemastay for exactly that reason,which the class comment now states.
docs/designs/k8s-daemon-pool.md§11 pointed at thedeleted file; it now describes what shipped.
Test plan
local-store— two orgs holding the same channel/thread key stay independent acrosspages, catch-up, first-message titles, tool bodies, tool updates and the thread revision
fence; an unattributable row is refused, and a session in the thread is enough to attribute
an observed inbound.
local-store— migration from a v10-shaped fixture: a store no pool shares keeps its rowsand gains the org-first recipient primary key; a shared store drops them. The v1/v5/v7
fixtures rewind both fix(daemon): own the session-metadata outbox per member #1068's outbox columns and this step's transcript columns, so each
still lands on 11.
postgres-migrations— the data-plane list drops the tables on an installed schema andnever creates them on a fresh one.
postgres-transcript-org.int(new,DATA_PLANE_TEST_DATABASE_URL) — the same two-orgisolation through the real SQLite → PostgreSQL rewrite, across two pool members.
postgres:16-alpine: the pool-store and transcript-org integrationsuites on a fresh schema, plus a manual in-place v10 → v11 upgrade —
orgidlandsNOT NULL DEFAULT '', the recipient primary key rebuilds as(orgid, channel, thread, ts, agentid),transcript_text_tsre-emits org-first, theshared store's unattributable rows are dropped, and a post-upgrade write reads back through
the fence. The data-plane drop migration was verified the same way on a schema rewound to
version 1.
pnpm --filter @agentconnect.md/daemon typecheck, the full daemon suite (acp-matrixexcluded — it dials live runtimes),
pnpm lint,pnpm format:check.Closes #1041.