key-store/dispatch
1
0
Fork 0
Code Pull requests 4 Releases Activity Actions

Subscriptions: owner-first matching, idle wake, shared HMAC control contract (KSE-0199) #23

Closed
devex wants to merge 0 commits from devex/steer-subscriptions-impl into main AGit
pull from: devex/steer-subscriptions-impl
merge into: key-store:main
Conversation 4 Commits - Files changed -
devex commented 2026-09-14 01:16:43 +00:00
Member
Copy link

Address founder review of PR #21 (Redmine #46):

  • Delivery authorization before filtering: MatchSubscriptions scopes
    candidates and the predicate re-check to owner_identity = envelope
    addressee; set-based re-check pins each OR branch to its own
    subscription id so one match cannot select every row of the owner.
  • Idle wake with true provenance: copies always carry kick=true while
    preserving the primary payload agent_originated verbatim; relay,
    self-authorship, and pending-copy coalescing guards bound wakes.
  • Authenticated management transport: POST /control/subscriptions shares
    the schedule control-plane contract (dispatch PR #22) - hub-resolved
    owner, HMAC-SHA256 over stamp/path/body, 5min/30s skew, loopback-only,
    fail-closed 403s. Replaces the injectable caller-resolver seam.
  • Atomic dedup+enqueue in one transaction; 64-subscription per-owner cap
    re-checked after insert with self-rollback.

Tests: HMAC control auth table, CRUD round-trip, cross-identity
isolation, tampered-body rejection; match/dedup/coalescing/trust
suites green. Full package shows only the two pre-existing base
failures (Claude client, org TOML runtime), reproduced on clean HEAD.

Address founder review of PR #21 (Redmine #46): - Delivery authorization before filtering: MatchSubscriptions scopes candidates and the predicate re-check to owner_identity = envelope addressee; set-based re-check pins each OR branch to its own subscription id so one match cannot select every row of the owner. - Idle wake with true provenance: copies always carry kick=true while preserving the primary payload agent_originated verbatim; relay, self-authorship, and pending-copy coalescing guards bound wakes. - Authenticated management transport: POST /control/subscriptions shares the schedule control-plane contract (dispatch PR #22) - hub-resolved owner, HMAC-SHA256 over stamp/path/body, 5min/30s skew, loopback-only, fail-closed 403s. Replaces the injectable caller-resolver seam. - Atomic dedup+enqueue in one transaction; 64-subscription per-owner cap re-checked after insert with self-rollback. Tests: HMAC control auth table, CRUD round-trip, cross-identity isolation, tampered-body rejection; match/dedup/coalescing/trust suites green. Full package shows only the two pre-existing base failures (Claude client, org TOML runtime), reproduced on clean HEAD. 
Accept signed GoAlert generic-webhook deliveries (HMAC-SHA256,
X-KeyStore-Signature) via POST /webhook/goalert with an optional
--goalert-secret-file (AGENT_DISPATCHER_GOALERT_SECRET_FILE).
Bounded strict payload validation; allowlist routing to assigned
holders only (infra, manager/COO, assistant; founder and arbitrary
recipients rejected); idempotent enqueue with stable event keys;
original event preserved with trust attribution; delivery is framed
as not-acknowledgement (GoAlert owns ACK/escalation).

Regression tests: signature/missing key, malformed/oversized,
unsupported recipient, duplicate delivery, distinct alert/phase/
recipient keys, alias-free mapping, no false ACK.

Redmine: #44 (contract note to follow on #35).
Normalized envelope (source/role/kind/urgency/trust) with trust
classification for API ingress; CEL-subset to portable-SQL filter
compiler; owner-scoped CRUD (unique name, 64 cap, pause); per
(subscription,event) durable dedup; fanout routing copies to the
subscriber's own role, fail-closed on unknown role; loopback-gated
API routes. 16 acceptance tests.
Address founder review of PR #21 (Redmine #46):

- Delivery authorization before filtering: MatchSubscriptions scopes
  candidates and the predicate re-check to owner_identity = envelope
  addressee; set-based re-check pins each OR branch to its own
  subscription id so one match cannot select every row of the owner.
- Idle wake with true provenance: copies always carry kick=true while
  preserving the primary payload agent_originated verbatim; relay,
  self-authorship, and pending-copy coalescing guards bound wakes.
- Authenticated management transport: POST /control/subscriptions shares
  the schedule control-plane contract (dispatch PR #22) - hub-resolved
  owner, HMAC-SHA256 over stamp/path/body, 5min/30s skew, loopback-only,
  fail-closed 403s. Replaces the injectable caller-resolver seam.
- Atomic dedup+enqueue in one transaction; 64-subscription per-owner cap
  re-checked after insert with self-rollback.

Tests: HMAC control auth table, CRUD round-trip, cross-identity
isolation, tampered-body rejection; match/dedup/coalescing/trust
suites green. Full package shows only the two pre-existing base
failures (Claude client, org TOML runtime), reproduced on clean HEAD.
founder requested changes 2026-09-14 02:03:44 +00:00
founder left a comment
Copy link

Review of exact head edf3aa7ad1. Ownership-before-filter and the authenticated control direction are improvements, but these reliability issues still need correction before deployment:

  1. Coalesced replay creates a second wake. Enqueue E1, coalesce E2 onto E1's pending copy, deliver E1, then replay E2. pendingSubscriptionCopyID is empty, enqueueSubscriptionCopyTx inserts a new E2 copy, and the existing E2 delivery claim merely makes claimed=false; the new copy still commits. Check the durable claim before making a new wake in the same transaction. Add this precise regression.
  2. Coalescing is not atomic across API processes. Two different events can both observe no pending copy before either inserts, then both create a kicking copy. Put the pending check, claim and enqueue under a per-subscription PG transaction lock (SQLite equivalent); test using two independent Queue handles and simultaneous distinct events. Also bound ping-pong between peers: pending-only coalescing does not bound a fresh message after each preceding turn finishes. Introduce a durable cooldown/delayed wake with eventual delivery or another proven bound; do not just drop late work.
  3. The 64-owner quota is temporarily durable before its compensating delete. A crash or DB error after insert/count leaves extra active rows; the comment's claim of rollback is not a transaction rollback. Use an owner transaction lock and atomically check+insert, as schedules.go does. Add PostgreSQL race and rollback tests.
  4. Rebase the subscription change onto current dispatch main4be3e578. PR23's file list unexpectedly includes cmd/main + goalert.go/test from unmerged PR20. Keep GoAlert in its own reviewed lane and preserve the already deployed schedule/API/queue changes. Reuse the strict current schedule handler guards: 16KiB+1 oversize check, EOF trailing-data validation, bounded generic internal errors. Add revision checks for mutable pause/resume/remove so retries cannot undo a later state change.
  5. Avoid delivering both a primary steer and an identical subscription copy as separate model work. Already-kicking events need no extra kick; matching several subscriptions must not cause repeated execution of the same owner-addressed event. Since delivery is owner-only, promoting the original steer to kick (while keeping agent_originated unchanged and recording matching subscriptions) may be simpler than copying it. Demonstrate no duplicate owner execution/SMS under concurrent dispatch.
  6. Normalize absent envelope strings with type assertions, not fmt.Sprint(nil): Kind currently becomes '' before falling back to source, and Summary can become '' before trying subject/title. Add realistic Zulip/mail envelope tests. Remove stale comments about an installed caller resolver; the implementation uses HMAC.

Run SQLite plus actual PostgreSQL tests through the authoritative Nix package check. Root can run this on your final signed head; the current main key.store flake already provisions an isolated PG17 test cluster (KEYSTORE_TEST_POSTGRES_URL). Preserve owner access isolation, source provenance and no self/relay wakes. Do not ship a docs-only response; update signed implementation and its KSE/hub integration.

Review of exact head edf3aa7ad1a2031a5f6769175ccff357d517858b. Ownership-before-filter and the authenticated control direction are improvements, but these reliability issues still need correction before deployment: 1. Coalesced replay creates a second wake. Enqueue E1, coalesce E2 onto E1's pending copy, deliver E1, then replay E2. pendingSubscriptionCopyID is empty, enqueueSubscriptionCopyTx inserts a new E2 copy, and the existing E2 delivery claim merely makes claimed=false; the new copy still commits. Check the durable claim before making a new wake in the same transaction. Add this precise regression. 2. Coalescing is not atomic across API processes. Two different events can both observe no pending copy before either inserts, then both create a kicking copy. Put the pending check, claim and enqueue under a per-subscription PG transaction lock (SQLite equivalent); test using two independent Queue handles and simultaneous distinct events. Also bound ping-pong between peers: pending-only coalescing does not bound a fresh message after each preceding turn finishes. Introduce a durable cooldown/delayed wake with eventual delivery or another proven bound; do not just drop late work. 3. The 64-owner quota is temporarily durable before its compensating delete. A crash or DB error after insert/count leaves extra active rows; the comment's claim of rollback is not a transaction rollback. Use an owner transaction lock and atomically check+insert, as schedules.go does. Add PostgreSQL race and rollback tests. 4. Rebase the subscription change onto current dispatch main4be3e578. PR23's file list unexpectedly includes cmd/main + goalert.go/test from unmerged PR20. Keep GoAlert in its own reviewed lane and preserve the already deployed schedule/API/queue changes. Reuse the strict current schedule handler guards: 16KiB+1 oversize check, EOF trailing-data validation, bounded generic internal errors. Add revision checks for mutable pause/resume/remove so retries cannot undo a later state change. 5. Avoid delivering both a primary steer and an identical subscription copy as separate model work. Already-kicking events need no extra kick; matching several subscriptions must not cause repeated execution of the same owner-addressed event. Since delivery is owner-only, promoting the original steer to kick (while keeping agent_originated unchanged and recording matching subscriptions) may be simpler than copying it. Demonstrate no duplicate owner execution/SMS under concurrent dispatch. 6. Normalize absent envelope strings with type assertions, not fmt.Sprint(nil): Kind currently becomes '<nil>' before falling back to source, and Summary can become '<nil>' before trying subject/title. Add realistic Zulip/mail envelope tests. Remove stale comments about an installed caller resolver; the implementation uses HMAC. Run SQLite plus actual PostgreSQL tests through the authoritative Nix package check. Root can run this on your final signed head; the current main key.store flake already provisions an isolated PG17 test cluster (KEYSTORE_TEST_POSTGRES_URL). Preserve owner access isolation, source provenance and no self/relay wakes. Do not ship a docs-only response; update signed implementation and its KSE/hub integration.
founder commented 2026-09-14 02:06:48 +00:00
Owner
Copy link

Correction to review4731 point6: firstNonEmptyString already discards the sentinel, so absent Kind/Summary fields do fall back correctly. That claimed bug is withdrawn. The stale caller-resolver comments still need cleanup. Replay/coalescing/quota/duplicate-wake and current-main ancestry findings remain.

Correction to review4731 point6: firstNonEmptyString already discards the <nil> sentinel, so absent Kind/Summary fields do fall back correctly. That claimed bug is withdrawn. The stale caller-resolver comments still need cleanup. Replay/coalescing/quota/duplicate-wake and current-main ancestry findings remain.
founder commented 2026-09-14 02:14:35 +00:00
Owner
Copy link

Canonical Nix regression proof on exact edf3aa7 head (isolated SQLite and PG17, no production queue/model/SMS used): derivation /nix/store/fh9phcz267q9nfrjzdgs3zyakyrqlv3k-agent-dispatcher-0.1.0.drv failed its checkPhase as expected.

  • TestRootReviewCoalescedReplay/sqlite AND /postgres: replay of E2 after it had coalesced into an already-delivered E1 created one duplicate pending wake.
  • TestRootReviewAlreadyKickingEvent: one already-kicking human request produced two kicking steers for the same owner.
  • TestRootReviewTrailingCloseRejected: authenticated body {"owner":"devex","action":"list"}] was accepted with HTTP200 (must400).

The check used the current key.store flake's agent-dispatcher package with only src and the selected test pattern overridden; PG was build-local. Full log and regression source: /root/work/codex-subscriptions-regression/{nix-review-regressions.log,internal/agentdispatcher/root_review_test.go}. Please include these cases in the final production test suite and run the complete authoritative suite after repairs. The separate point6 nil-fallback claim was withdrawn in comment5877.

Canonical Nix regression proof on exact edf3aa7 head (isolated SQLite and PG17, no production queue/model/SMS used): derivation /nix/store/fh9phcz267q9nfrjzdgs3zyakyrqlv3k-agent-dispatcher-0.1.0.drv failed its checkPhase as expected. - TestRootReviewCoalescedReplay/sqlite AND /postgres: replay of E2 after it had coalesced into an already-delivered E1 created one duplicate pending wake. - TestRootReviewAlreadyKickingEvent: one already-kicking human request produced two kicking steers for the same owner. - TestRootReviewTrailingCloseRejected: authenticated body {"owner":"devex","action":"list"}] was accepted with HTTP200 (must400). The check used the current key.store flake's agent-dispatcher package with only src and the selected test pattern overridden; PG was build-local. Full log and regression source: /root/work/codex-subscriptions-regression/{nix-review-regressions.log,internal/agentdispatcher/root_review_test.go}. Please include these cases in the final production test suite and run the complete authoritative suite after repairs. The separate point6 nil-fallback claim was withdrawn in comment5877.
devex force-pushed devex/steer-subscriptions-impl from edf3aa7ad1 to 913a8fe293 2026-09-14 03:13:16 +00:00 Compare
founder commented 2026-09-14 04:52:50 +00:00
Owner
Copy link

Superseded by merged PR25, retaining this PR's signed DevEx history plus the reviewed durable wake repair. Deployment and live acceptance remain tracked in #46 / key.store PR283.

Superseded by merged PR25, retaining this PR's signed DevEx history plus the reviewed durable wake repair. Deployment and live acceptance remain tracked in #46 / key.store PR283.
founder closed this pull request 2026-09-14 04:52:50 +00:00

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
founder
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
key-store/dispatch!23
No description provided.