feat(comms): coordination-channel auto-provision from tree edges (SEA-1722) - #166
Merged
Merged
Conversation
seal-agent
force-pushed
the
compass-comms-1722-t5-coordination
branch
2 times, most recently
from
August 5, 2026 21:14
cfba1aa to
ffea6c2
Compare
seal-agent
force-pushed
the
compass-comms-1722-t4-channel-policy
branch
from
August 5, 2026 21:14
4074868 to
bfccd5f
Compare
…-1722) Auto-provision and reconcile a manager's coordination channel from the agent tree's parent edges. The store's two parent-edge writers (CreateAgent, ReparentAgent) invoke a store-registered hook on their own tx right after writing agent_accounts.parent_agent_id; the comms-owned closure runs the in-tx reconcile — channel upsert + membership rows + D2 cursor seeds — on that same tx, so coordination state commits atomically with the tree edge. ChannelChanged is emitted post-commit best-effort (a lost emit self-heals on the next reconcile / D1 sweep). Design realizations (both confirmed against the frozen record, design.md 530-592): - Per-owner group get-or-create: one `__coordination__` VisibilityOwner group per owner hosts every coordination channel under that owner, resolved by a deterministic SELECT-then-INSERT under a per-owner advisory lock (LockOwnerCoordinationTx, keyed `coordination:`+owner, namespaced off ReparentAgent's tree lock). All of an owner's coordination channels share one group, the invariant the same-owner collision analysis depends on (design.md:570-585). - No savepoint: the reconcile runs directly on the parent-edge writer's tx. Its only expected failure — a same-owner name collision — is resolved WITHOUT erroring by the advisory-lock + SELECT-guided suffix search in UpsertCoordinationChannelTx (a user's manually-named channel is suffixed around, never adopted, never a unique-violation). Normal operation cannot poison the tx; only a genuine store fault returns an error, which correctly rolls back the whole parent-edge write (design.md:554 in-tx vs 591-592 never-wedges-report-creation). ReparentAgent reconciles BOTH affected managers: the new parent gains the report (reparent-in adds it) and the old parent loses it (reparent-out removes it, carried in ChannelChanged.removed_account_ids), captured before the UPDATE overwrites the edge. Membership seeds each agent member's D2 cursor in-tx so a mandatory channel never mints an un-seeded delivery target. No P3 relay-arm change (coordination is provisioned from the tree, not an agent comms-call variant). The migrations/0007 edit is a comment-only INVARIANT note on the parent_agent_id column; the migration runner is version-tracked, not checksummed, so editing an applied migration's comment is runtime-safe. Red-first proven: neutering invokeCoordinationHook turns the 4 hook-dependent store tests + 3 comms tests red with sensible messages (empty fired-list, 0 channels provisioned, member not moved, no suffix), restoring turns them green. Stacked on the T4 channel-policy branch (SEA-1722); base tip 8d2cfd6 (born-mandatory cursor seed). Full gate green on the rebased tree: build/vet clean, CI-exact lint (--config .golangci.yml ./..., no pgtest tag) 0 issues, store + comms pgtest suites green under -race. Refs SEA-1722 Co-authored-by: Matt Wilkinson <matt@sealedsecurity.com>
Review-fix for the T5 coordination PR (mandatory skill://review, 2 medium + 3 low). Both mediums are the same root cause — the `__coordination__` namespace was claimed-reserved in docstrings but not enforced — fixed at the store/reconcile layer without touching the user RPC surface, honoring all four frozen invariants (owner-private, never-wedge, never-adopt, no-savepoint). - EnsureOwnerCoordinationGroupTx: the get-half SELECT matched a group by (owner_user_id, name, top-level) with no visibility filter, so a user-planted top-level VisibilityShared group named `__coordination__` was adopted and the OWNER_ONLY coordination channel inserted into it — making an owner-private channel visible to every account (cross-tenant metadata leak). Discriminate the SELECT on `visibility = VisibilityOwner` so a mis-visibility planted group is never adopted; the create-half then mints the correct owner-visible group. - UpsertCoordinationChannelTx: the reconcile runs on the parent-edge writer's tx with no savepoint, and its plain INSERT could raise a unique-violation on channels_group_name_key when a user's concurrent CreateChannel committed the same (group, name) between our SELECT and INSERT — poisoning the tx and wedging report creation (the exact outcome the design forbids). Make the INSERT poison-free: `ON CONFLICT (group_id, name) WHERE group_id IS NOT NULL DO NOTHING RETURNING id`. A concurrent commit now yields zero rows (not a raised violation); the loop re-resolves the name and suffixes it (never-adopt), and the parent-edge write never wedges. Both docstrings rewritten from their prior false claims to the true-by-construction invariant. - comms EnsureCoordinationChannel: move the len==0 internal-invariant guard ahead of emitCoordChanges (surface the fault before the no-op emit). - Test-doc honesty: correct the coordChannels helper comment. Red-first: TestReconcileIgnoresMisVisibilityUserGroup (drop the visibility discriminator -> the shared group is adopted, channel leaks to a third account); TestUpsertConcurrentUserInsertSuffixesWithoutWedge (deterministic two-tx race, a committed concurrent user insert -> without ON CONFLICT the reconcile raises 23505 and would wedge the parent-edge write); TestCreateAgentSuffixesAroundUser ChannelWithoutWedge (drives the real registered closure: manager's channel suffixes to -2, CreateAgent still succeeds). Each proven by reverting the fix. Co-authored-by: Matt Wilkinson <matt@sealedsecurity.com>
seal-agent
force-pushed
the
compass-comms-1722-t5-coordination
branch
from
August 6, 2026 00:42
ffea6c2 to
07e05ff
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Auto-provision and reconcile a manager's coordination channel from the agent tree's parent edges (SEA-1722 T5, design.md:530-592). When an agent gains or moves a parent, the manager's
<handle>-coordinationchannel (OWNER_ONLY, mandatory-subscription, owner = manager) is get-or-created and its membership resynced to the manager plus its direct reports — atomically with the tree edge.How
The store's two parent-edge writers (
CreateAgent,ReparentAgent) invoke a store-registeredCoordinationHookon their own tx, right after writingagent_accounts.parent_agent_id. The comms-owned closure (reconcileCoordinationTx) runs the in-tx reconcile — channel upsert + membership rows + D2 cursor seeds — on that same tx, so coordination state commits atomically with the tree edge. The store never imports comms types; the hook is an injected callback.ChannelChangedis emitted post-commit, best-effort: the in-tx reconcile records touched channels into a ctx-scoped buffer that the parent-edge RPC drains and emits after the store confirms the commit. A lost emit self-heals on the next reconcile / D1 sweep, so a dropped event never leaves the tree edge and channel state divergent.Design realizations (both confirmed against the frozen record)
__coordination__VisibilityOwnergroup per owner hosts every coordination channel under that owner, resolved by a deterministicSELECT-then-INSERTunder a per-owner advisory lock (LockOwnerCoordinationTx, keyedcoordination:+owner, namespaced offReparentAgent's per-owner tree lock so the two never spuriously serialize). All of an owner's coordination channels sharing one group is the invariant the same-owner collision analysis depends on.SELECT-guided suffix search inUpsertCoordinationChannelTx(a user's manually-named channel is suffixed around-2,-3, …, never adopted, never a unique-violation). Normal operation cannot poison the tx; only a genuine store fault returns an error, which correctly rolls back the whole parent-edge write (fail-loud beats a half-provisioned tree).Reparent reconciles both managers
ReparentAgentcaptures the OLD parent before theUPDATEoverwrites the edge, then reconciles BOTH affected managers: the new parent gains the report (reparent-in adds it) and the old parent loses it (reparent-out removes it, carried inChannelChanged.removed_account_ids). Empty sides (promote-to-root / former-root move) skip;old == new(no-op move) skips the redundant second resync.Notes
seedDeliveryCursoris agent-only + idempotent, so a human member yields no cursor and a re-run touches nothing.migrations/0007_agent_tree.sqledit is a comment-only INVARIANT note on theparent_agent_idcolumn (documenting the hook contract). The migration runner is version-tracked, not checksummed (store.gomigrate), so editing an applied migration's comment is runtime-safe.Tests (red-first proven)
9 pgtest suites: 6 store (
coordination_pgtest_test.go) + 3 comms (coordination_pgtest_test.go), covering hook-fires-for-parent, first-report-provisions-once, reparent-moves-membership (both managers), despawned-report-keeps-membership, collision-manager-owned-resumes, collision-user-owned-suffixes (never-adopt + parent-edge-never-wedged), and the manualEnsureCoordinationChannelbackfill entrypoint.Red-first: neutering
invokeCoordinationHookturns the 4 hook-dependent store tests red with sensible messages (empty fired-list, 0 channels provisioned, member not moved, no suffix); restoring turns them green.Verification
go build ./...clean;go vet -tags pgtest(store + comms) clean.golangci-lint run --config .golangci.yml ./...(no pgtest tag — the authoritative gate) → 0 issues.-race(store 83s / comms 23s), including T4's born-mandatory tests — no regression.Stacking / collect notes
Stacked on the T4 channel-policy branch (#161, base tip
8d2cfd6cborn-mandatory). GitHub auto-repoints this base →mainwhen #161 merges.Collect overlap with T6 (#165): both add an identical
store.GetChannel(ctx, id) (Channel, error)exported wrapper overgetChannel(T5 for the post-commit emit read, T6 for the pinned-board authz read). Byte-identical body, different doc comment — a clean union fold whichever merges second (keep one method), not a conflict.Refs SEA-1722