feat(event): refined event subscription support (management, consume, encryption) - #2064
Draft
leave330 wants to merge 210 commits into
Draft
feat(event): refined event subscription support (management, consume, encryption)#2064leave330 wants to merge 210 commits into
leave330 wants to merge 210 commits into
Conversation
Add additive KeyDefinition fields (RefinedSubscription, ResourceType,
KeyTemplates) and the new KeyTemplate type, plus RegisterKey validation:
when RefinedSubscription is true, KeyTemplates must be non-empty, each
template's PathSegment/SelectorKey must be non-empty, and each
template's AuthTypes must be a subset of {"user","bot"}. Panics on
violation, matching the existing RegisterKey panic style. Existing
KeyDefinition fields and RegisterKey validation are unchanged.
Adds a placeholder refined-subscription EventKey catalog for
im.message.created_v1 (chat-id/{chat_id} and owner/me templates, per
design spec section 2.2), registered through the normal events/register.go
aggregation alongside the other domain packages. internal/event stays
types/resolver/registry only. meta-swap: replace the embedded mock JSON
with the fetch_meta-driven catalog once available; consumers unchanged.
TestRefinedKeys only asserted RefinedSubscription==true and len(KeyTemplates)==2, so a wrong template string/selector/auth_types in refined_keys_mock.json would pass silently. Assert the im.message.created_v1 key's ResourceType, AuthTypes, and both KeyTemplates' Template/PathSegment/ SelectorKey/FixedValue/AuthTypes against design spec section 2.2, since this mock is the contract the refined-key parser (a later task) consumes.
Adds internal/event/resolve.go with ResolveEventKey(input) and ResolvedEventKey, the sole refined-aware EventKey entry point per design spec section 2.3. Exact Lookup runs first (legacy keys exact-only; a bare refined base key is rejected under R1); only on a miss does it split base/path-segment/value, requiring the base to itself be a refined definition. Selector values go through url.PathUnescape exactly once, validated (malformed % escape rejected; a literal unescaped '/' rejected rather than silently changing segment count), then re-escaped with PathEscape/QueryEscape so %2F round-trips without double-encoding. Lookup and RegisterKey are unchanged.
ResolveEventKey's ValidationError.Param carried the literal usage placeholder <EventKey> instead of a canonical snake_case name. errs/ERROR_CONTRACT.md's Validation parameters section requires positional arguments to use the canonical name without dashes (e.g. target_user_id), so rename the constant's value to event_key; this propagates to every error site through paramEventKey.
Adds additive `event schema --json` output for RefinedSubscription keys per design spec section 2.6/2.7: conditional_scopes (event:encrypt_key:read when --include-resource-data + --as user), a static risk disclosure (ordinary_event_key=read, effective=write, dry_run_recommended=true) since the command framework only tracks static per-command risk, subscription payload_options + dry_run contract info, and a next_action pointing at a concrete dry-run command anchored on the first KeyTemplate example. refined_subscription/resource_type/key_templates/auth_types/scopes were already flowing for free via the KeyDefinition embed; only conditional_scopes/risk/subscription/next_action are new fields, gated on RefinedSubscription and using pointer/slice types so omitempty actually omits them for legacy keys. Legacy key JSON output (im.message.receive_v1) is verified unchanged.
Wire ResolveEventKey into the event consume entry: R1 (bare refined base key), legacy+suffix, and unknown-base all get a typed invalid_argument; a materialized refined key gets a typed failed_precondition (refined consume runtime is Phase C, not built yet -- never starts the bus or performs a remote write for it). Legacy exact-match keys are unaffected (same *KeyDefinition pointer, same flow). The pre-existing "unknown EventKey: <key>" message/hint contract is preserved by mapping ResolveEventKey's differently-worded unknown-base error back onto it.
Add internal/event/subscription_client.go, a thin adapter over the candidate SDK's typed client.Event.V1.Subscription service, aliased larkeventv1 to avoid the larkevent package-name collision with the core oapi-sdk-go/v3/event package. SubscriptionClient binds one already-resolved CLI identity (user or bot) to the correct per-call option for its lifetime: user appends larkcore.WithUserAccessToken(uat); bot appends none, letting the SDK mint its own tenant token from the appID/secret already on the lark.Client. Construction fails closed with a typed error on an unresolved identity or a missing UAT instead of guessing. Exposes Create/Get/List/Patch/Renew/Reactivate/Delete for the upcoming event subscription command tasks. Errors are classified through the existing errs/errclass contract: SDK-boundary transport errors go through client.WrapDoAPIError; a non-success response is decoded and classified via errclass.BuildAPIError, so Subscription never gets bespoke error codes. A small subscriptionService interface, satisfied structurally by lark.Client Event.V1.Subscription, lets tests capture exactly which RequestOptionFunc values are passed per identity, with no real network call.
Add cmd/event/subscription/ with the event subscription command group root plus the read-only list/get pair, wired into cmd/event/event.go. Both call through Task 7 SubscriptionClient, resolve --as per spec 2.8 (no per-template AuthTypes check), and run a local best-effort event:subscription:read scope pre-check before constructing the SDK client. The local field is reserved but always omitted pending the Phase-C bus runtime.
resolveEffectiveIdentity (cmd/event/subscription/subscription.go) called f.ResolveAs but never f.CheckStrictMode, so an explicit --as user/bot could bypass an administrator's configured strict-mode identity policy whenever a credential of the disallowed type was still resolvable (e.g. strict-mode: bot configured, plus a cached user token, plus event subscription list --as user would resolve and call the API as user). Call f.CheckStrictMode right after f.ResolveAs, before CheckIdentity, mirroring cmd/api/api.go's apiRun, cmd/service/service.go's serviceMethodRun, and cmd/whoami/whoami.go's whoamiRun. Both list and get route through this one helper, so the single change covers both. cmd/event/consume.go has the same pre-existing gap but is intentionally left untouched here; it is tracked as a separate follow-up. Add TestResolveEffectiveIdentity_StrictModeRejectsCrossIdentity, mirroring cmd/whoami/whoami_test.go's TestWhoami_StrictModeRejectsCrossIdentity: a bot-only strict-mode account configured via cmdutil.TestFactory rejects an explicit --as user with a typed *errs.ValidationError.
Implements `event subscription create <refined EventKey>`: resolves the key via ResolveEventKey, enforces template-level identity AuthTypes on top of the usual --as checks, hard-requires both subscription:read and subscription:write scopes, gates --include-resource-data=true (E), and reconciles against remote state (List) before writing - idempotent reuse on an active+compatible match, typed failed_precondition on a conflicting or suspended match, and a single bounded re-List reconcile if Create itself fails (duplicate/timeout). --dry-run previews the plan without ever writing. No confirmation/--yes: create is additive.
…/suspended branches Add TestRunCreate_MissingWriteScope_ReturnsPermissionError, which the existing doc comment already claimed existed alongside the missing-read test, locking spec section 3.5's read/write independence invariant from both sides. Wire suspendedDetail's reason param into Suspension.Code instead of hardcoding "authority_revoked", and tighten the suspended-branch reconcile test to use a distinct reason and assert it flows through suspendedError's message, proving the extraction is dynamic rather than coincidentally matching a hardcoded value. Add dry-run coverage for the conflict and suspended reconcile plan shapes (previously only create/reuse were covered), asserting --dry-run reports each state informationally per spec section 3.4 and never calls Create.
Add the remaining four event subscription management commands, closing Phase B's management plane: update (SDK Patch, payload_options only, with a suspended-state guard guiding reactivate), renew, reactivate, and delete. All four hard-require both event:subscription:read and event:subscription:write, always read remote state first via Get, and support --dry-run (parse/identity/scope/remote-read/impact-analysis only, never a write). update and delete are high-risk writes on a shared remote resource and return a typed ConfirmationRequiredError (exit code 10) unless --yes is passed; renew and reactivate do not expose --yes.
Add optional, omitempty fields to Hello/Event/StatusResponse per spec §4.2 so later Phase-C tasks can carry refined-subscription context between the consume client and bus daemon over the existing newline-JSON IPC. No logic changes (routing/identity/negotiation are later tasks) — struct fields only, with doc comments. - Hello: consumer_scope_id, remote_subscription_id, identity, profile, user_open_id, capabilities (v2 capability marker, symmetric with StatusResponse.Capabilities). - Event: remote_subscription_id, target_resource, authority, subscription_event_id (normalized push-envelope subscription context). - StatusResponse: protocol_version, capabilities, registered_event_types, for a future ProbeBusEligibility to read bus capability over status_query/status_response. The existing wire subscription_id stays the local parameter fingerprint (frozen meaning) — remote_subscription_id is a new, distinct field. Codec is unchanged; old frames without the new keys still decode with the new fields left zero-valued.
Move the subscription create/reuse/conflict/suspended reconcile classification (state table + authority matching) out of cmd/event/subscription/create.go into exported internal/event/reconcile.go (ReconcileExisting/ReconcilePlan/AuthorityMatchesIdentity/PlanAction*), alongside subscription_client.go, so it can be shared with the refined `event consume` startup chain's PlanRemoteSubscription stage (Task 15b) instead of staying package-private to `create`. create.go keeps its own reconcilePlan/planAction*/reconcileExisting names as aliases/a forwarding wrapper to the new shared implementation, so its behavior is unchanged and create_test.go passes with zero modification — the correctness gate for this extraction. Also defines SubscriptionLister (List-only) and SubscriptionCreateAPI (List+Create) seam interfaces in internal/event, which *SubscriptionClient satisfies structurally, for 15b's future write-path use.
Populate protocol.StatusResponse's v2 fields (ProtocolVersion, Capabilities, RegisteredEventTypes), which existed on the wire shape but were never set. handleStatusQuery now reports ProtocolVersion v2 and a Capabilities list (refined_routing, hello_v2) via new consts in protocol/messages.go (the one place both the bus and a later prober can reference the same identifiers), plus a new Hub.RegisteredEventTypes() aggregator over currently registered subscribers' EventTypes(). An old (pre-this-change) bus still reports all three fields empty; that absence is itself the incompatibility signal a future probe (Task 15b's ProbeBusEligibility) checks for. NewStatusResponse's own signature and call sites are untouched - the fields are set on the constructed value inside handleStatusQuery instead.
Implement Task 15b: the refined event consume <EventKey> startup chain (design spec section 4.1/4.2/4.9). In strict order: ProbeBusEligibility (read-only) -> PlanRemoteSubscription (List/Get) -> [--dry-run exits here] -> ApplyRemoteSubscriptionPlan (the ONLY remote write: Create/Reactivate) -> StartOrConnectBus -> HelloV2 -> consumeLoop. - internal/event/consume/probe.go (new): ProbeBusEligibility reuses CheckRemoteConnections unchanged for the single-bus guard, plus a new local-bus capability check via status_query/status_response. An old (pre-refined) bus has an absent protocol_version/capabilities and fails closed with a prompt to run event stop; no local bus yet is fine. - internal/event/consume/refined.go (new): RunRefined plus the injectable refinedDeps seam (probe/plan/apply/startBus/hello) so order and the dry-run short-circuit are assertable with fakes, no network. Apply (create/reactivate/reuse) is the only write; a conflicting plan fails closed before Apply; an Apply-ok-but-bus-start-fail never auto-deletes the remote subscription. HelloV2 populates Identity/Profile/UserOpenID/ RemoteSubscriptionID/ConsumerScopeID (a SHA-256 hash of base key, materialized key, authority type, app_id, and user_open_id). - internal/event/consume/handshake.go: extract doHello's wire dance into a shared sendHello helper; add doHelloV2 for the v2-populated Hello. doHello's own signature and behavior are unchanged. - cmd/event/consume.go: add --dry-run; replace the refined-key stub with runRefinedConsume, which resolves identity, the SubscriptionClient, and signal handling, then calls consume.RunRefined. The legacy (non-refined) path is untouched. - cmd/event/consume_test.go: update the one pre-existing test that locked the now-removed "runtime not yet available" stub to instead lock that a refined key is routed into the real chain and fails safely offline.
when a local bus already answered CheckRemoteConnections counts ALL of this app's active WebSocket connections, including the local bus's own connection once it is up. ProbeBusEligibility was calling it unconditionally, ahead of the local bus reachability check, so every second (and later) refined consumer attaching to an already-healthy, already-running local bus would see online_instance_cnt=1 (that bus's own connection) and fail closed as if "another event bus" were connected -- breaking the ordinary N-consumers-share-one-bus model. EnsureBus (startup.go) already avoids exactly this: it calls CheckRemoteConnections only inside the branch where probeAndDialBus failed (no local bus dialable at all) -- see its own "local bus not found; checking remote connections..." log line. ProbeBusEligibility now mirrors that same condition: check the local bus first, and only fall through to the remote check when no local bus answered. A local bus that does answer is checked for its v2 capability markers only. Caught via a design-review question about the apparent duplication between startup.go and probe.go; added TestProbeBusEligibility_HealthyLocalBusAlreadyOnline_RemoteCheckNotAppliedToItsOwnConnection as a regression test (RED against the previous implementation, GREEN after the fix) that also asserts the remote check makes zero calls once a local bus already answered.
Review fixes for the refined event consume startup chain (Task 15b): 1. (write-safety) runRefinedConsume only checked the base EventKey's AuthTypes, never the matched KeyTemplate's own narrower AuthTypes, so "event consume im.message.created_v1/owner/me --as bot" reached Plan/Apply (a real remote write) for a user-only template, a write that subscription create already rejects client-side. Moved checkTemplateAuthTypes out of cmd/event/subscription/create.go into the new exported internal/event.CheckTemplateAuthTypes (create.go keeps a thin same-named wrapper; create_test.go is unchanged) and call it in runRefinedConsume before any client construction or Plan/Apply. 2. (correctness) prodRefinedDeps's hello closure fed a bot consumer's real user_open_id into computeConsumerScopeID even though buildHelloV2 correctly drops it from the wire for a bot, making a bot's consumer_scope_id vary with whoever is currently logged in. Now forces an empty string for a bot identity's scope-id computation; user identity behavior is unchanged. 3. (correctness) A post-Apply HelloV2 transport error or bus-rejected hello_ack returned an error without the remote_subscription_id, created_by_this_attempt, and next_action recovery hint that the startBus-fail path already carries, even though both leave the same remote-subscription-behind situation. Extracted the shared hint builder and reused it in both paths, appending (not replacing) the bus-rejection path's own single-consumer guidance. TDD throughout: a RED test was added per fix before each implementation change. Full report in .superpowers/sdd/task-15b-report.md.
Additive, strictly read-only enhancement to `event status` for refined
(remote-subscription-backed) consumers, spec §4.6. Legacy consumer output
is unchanged; every new field is omitempty.
- protocol.ConsumerInfo gains refined_subscription, remote_subscription_id,
owner_{identity,app_id,user_open_id}, stale_identity, degraded_reason,
remote_state, a remote_subscription{state,expire_time,
include_resource_data} sub-struct, and last_lifecycle_event (added now,
populated later by Task 18). TestDecode_OldStatusResponse_BackwardCompat
extended to pin all of them zero-valued on an old wire frame.
- Hub.Consumers() populates the new fields: RemoteSubscriptionID/OwnerAppID/
OwnerUserOpenID come straight off the Subscriber interface (so OwnerAppID
is populated for every consumer on a Phase-C bus, not only refined ones);
OwnerIdentity/StaleIdentity/DegradedReason use the same s.(*Conn)
type-assert the Publish delivery gate already relies on, so the
Subscriber interface and its mocks are untouched.
- cmd/event/status.go: runStatus now threads cmd (needed to resolve its own
identity). Local current_profile_match is computed against a freshly
loaded (never Factory.Config()-cached) current identity, mirroring
bus/identity.go's resolveCurrentIdentity, and is guarded to consumers with
a real owner user (OwnerUserOpenID != "") so bot/legacy consumers are
never flagged. Text output gets a per-consumer advisory sub-line under
each refined row (stale/degraded are informational only, never "dead");
JSON gets the same via a consumerView wrapper that embeds ConsumerInfo.
A weak, optional remote supplement (SubscriptionClient.Get) fills
remote_subscription/remote_state for the current app's refined consumers
only when identity resolves, a valid unrefreshed token is on disk
(auth.GetStoredToken, never f.Credential.ResolveToken), and
event:subscription:read is held — any failed precondition or remote
error silently falls back to local-only. status never refreshes a UAT,
binds a user, or writes remotely.
TDD throughout, bottom-up (protocol -> bus -> cmd), RED confirmed at each
layer before implementing. Full report in .superpowers/sdd/task-16-report.md
(gitignored, local artifact only).
Address Task 16 code review findings for `event status`'s refined
additive fields (spec §4.6), display-only:
- Derive a degraded advisory from the remote-supplement's
remote_state for the two grounded values ("suspended"/"expired");
any other (open-vocabulary/healthy) state stays advisory-free.
Appended alongside any existing bus-side degraded_reason/
stale_identity advisory, never clobbering it.
- Add `,omitempty` to RemoteSubscriptionInfo.IncludeResourceData for
consistency with its siblings.
- Reframe the stale_identity advisory as an earlier-snapshot note
(instead of asserting a current mismatch) when the freshly
computed current_profile_match is true, resolving a
textually-contradictory pair of adjacent lines.
Strictly read-only/display-only: no bus write, no Conn mutation, no
new remote call. Legacy consumer output unchanged.
Task 17 (infra only, spec section 5.1/5.2): FeishuSource registers the 6 typed event.subscription.*_v1 handlers on the same dispatcher, before cli.Start, diverting them away from business-consumer delivery into a new bounded, single-shot in-memory executor (worker=2/slots=32, dedup via a third DedupFilter domain, in-flight/pending merge, full-to-degraded, cancel/timeout). The executor's action is summary-only for this task: it records lastLifecycleEvent/remoteState on the matched consumer via a new Hub.connsByRemoteSubscriptionID, behind a pluggable action seam that a later task will swap remote actions into.
…stone Task 18 (spec section 5.3/5.4/5.5/8): replaces Task 17's summary-only lifecycleAction with the real per-event behavior, plugged into the same bounded executor unchanged. - New subscriptionLifecycleAction (internal/event/bus/lifecycle.go): per-event switch for activated/updated/suspended/expiration_reminder/ expired/deleted, a single owner==current eligibility gate (spec 8) reused by every remote action (Reactivate/Renew/Get) and by BindUser, at-most-one remote call per event, and a TTL in-memory tombstone so a deleted_v1 blocks a later activated_v1/updated_v1 resurrection for the same remote_subscription_id. Only the confirmed suspension.code value (authority_revoked) auto-Reactivates; any other value takes section 5.4's default branch (single Get reconcile, never a guessed recovery action). - identityGate (identity.go) now memoizes the latest connID/bindUser from onConnReady and exposes bindConsumer(ctx, c), factored out of onConnReady's own loop body, so the lifecycle action can (re)bind a specific consumer independently of a fresh WS ready/reconnect event. - Bus gains SetSubscriptionClient(sdk *lark.Client), mirroring SetIdentityProviders's shape; cmd/event/bus.go wires it via f.LarkClient() with a non-fatal (summary-only) fallback. NewBus now constructs subscriptionLifecycleAction instead of summaryLifecycleAction. - Conn gains suspensionReason/lastAction/lastActionError/nextAction (spec section 5.5), surfaced via Hub.Consumers()/protocol.ConsumerInfo (new omitempty fields). Fixes 2 stale Task-17 doc comments that said these fields were populated by a future Task 18 when Task 17 already populated them (review Minor 1), and has the executor's queue-full path also set an explicit next_action (review Minor 2). The section 8 red line (never recover/BindUser for a historical/non-current identity) is covered explicitly: an owner-mismatched consumer triggers zero Reactivate/Renew/Get/BindUser/resolveUAT calls across every event type, verified in internal/event/bus/lifecycle_dispatch_test.go. TDD throughout; go test ./internal/event/bus/ -race -count=1 passes (154 tests, 0 races). Full report in .superpowers/sdd/task-18-report.md (gitignored, local artifact only).
Task 19 (final task) of the refined event-subscription feature: document what Tasks 1-18 shipped, with no product-logic changes. - Add skills/lark-event/references/refined-subscription.md: cross-cutting reference for key_templates/materialization/--dry-run, TTL/renew, the --as identity gate (owner vs current, stale_identity), the 6-event lifecycle control plane, the event subscription management plane (scope/confirmation table keyed by remote_subscription_id), the stop chain (local consume vs remote delete), and the E-deferred boundary on --include-resource-data. - Modify skills/lark-event/SKILL.md: add a refined-subscription routing section (list/schema as source of truth -> key_templates -> --dry-run -> event subscription management -> stop chain) linking to the new reference; additive, existing content unchanged. - Modify cobra Long/Example/Short help text (no flag/logic changes) for the event root, event consume, event status, and the event subscription group + its 7 subcommands, giving each the USAGE/allowed values/identity semantics/Scope/EXAMPLES/OUTPUT/NEXT STEP/SAFETY structure, verified against the actual implemented flags and behavior.
event consume shipped with no --include-resource-data flag at all (Task 19's own report flagged this as a concern): passing it produced cobra's generic unknown-flag error instead of the spec-required typed rejection (design spec section 4.7:325/328/330, section 9:403). Adds the flag (default false, no behavior change) and gates it before any side effect: true on a refined EventKey returns the same typed failed_precondition (reason resource_data_encryption_deferred) that subscription create/update already return; true on an ordinary EventKey returns a typed invalid_argument instead, since the flag has no remote-Subscription concept to apply to there. Corrects the consume --help text and skills/lark-event/references/refined-subscription.md, both of which documented the flag as absent.
runRefinedConsume resolved identity via resolveIdentity (ResolveAs + CheckIdentity) but never called CheckStrictMode, unlike every sibling --as write command (subscription.go's resolveEffectiveIdentity, api.go's apiRun, service.go's serviceMethodRun, whoami.go's whoamiRun). Since ResolveAs deliberately preserves an explicit --as through strict mode so the caller can reject it, an explicit --as of the disallowed type on a strict-mode account could slip past both AuthTypes tiers and reach PlanRemoteSubscription/ApplyRemoteSubscriptionPlan under the wrong authority, bypassing the configured identity policy on a remote-write path. Adds one CheckStrictMode call in runRefinedConsume, mirrored exactly from resolveEffectiveIdentity, right after identity resolution and before any remote write. Scoped to the refined write path only; resolveIdentity and the legacy consume path are untouched.
Locks the red line that matters: a domain must not re-derive event_id, event_type or create_time from the shared push-envelope header, because the ingress already resolved them onto the canonical event and the consumer boundary fails closed if the two disagree. An AST check (not a grep) flags those tags nested inside a json:"header" block, so a domain body owning its own create_time is unaffected. Deliberately scoped to those three facts rather than banning header access outright: events/ carries long-running legacy projections, and forcing every one of them off the header wholesale would be a wide, risky change with no correctness gain.
…d bool 'Refined' bundles several independent facts that happen to move together today: a remote Subscription must be provisioned, events are addressed by that subscription's id instead of broadcast by event_type, and the EventKey must carry a resource selector. Threading one boolean through catalog, consume and status means a third delivery mode has to edit all of them. Introduce a derived SubscriptionCapability naming those axes and repoint the two genuine decision sites at it: the resolver asks 'does this key require a selector?' and DescribeConsume asks 'how is delivery provisioned?'. The resolver populates the capability on its result, so it is the single interpreter of the declaration and no reader has to guess a default for a missing definition. Derived from the existing declaration rather than added as a field, so no domain under events/ changes and no JSON/wire shape gains a member — the list/schema goldens are untouched. Only two values exist today; the win is that the next mode is additive, which the extension test pins.
…consume Domain handlers no longer re-parse the envelope header, so every consumer now depends on the bus carrying the full canonical metadata on each delivered frame: the identity facts, BOTH time facts kept separate, and the tenant identity. A bus from an older build omits those fields, so a new consumer talking to it would emit events with event_id / event time / tenant identity silently empty instead of failing. The bus advertises canonical_metadata_v1, and BOTH the classic and the refined consume path validate it on the ack of the REAL delivery connection (a separate status probe reads a different connection and would leave a TOCTOU). Missing it is a typed failed_precondition telling the user to run 'event stop' and retry; the classic path fails before PreConsume, and the refined path carries the apply-ok receipt because the remote Subscription may already exist. Tests decode real old-bus ack frames, and one gate pins that what this bus advertises satisfies what this consumer requires.
The header-vs-canonical check compared event_id, event_type and create_time and silently ignored the rest: the tenant identity and the entire refined delivery tuple (remote_subscription_id / target_resource / authority / subscription_event_id) could disagree without being noticed. Fixing that by adding more if-branches would keep the same failure mode, so the comparison is now a table covering every canonical fact, plus a reflection test that fails when a RawEvent field is neither verified nor explicitly declared unverifiable — with a reason. Authority needs both sides in one vocabulary, so the formatter moves into the kernel and the ingress delegates to it; otherwise a formatting difference would masquerade as a data conflict. A separate case drives a real mismatch through every row so none can be present-but-inert.
…ugh a provisioner registry Reworks the capability boundary so it is trustworthy rather than merely tidy. The capability is now COMPILED AND VALIDATED at registration and stored immutably on the registry entry, instead of being derived lazily at each call site. Its zero value is invalid on every axis and the mode sets are closed, so an unset or unimplemented mode fails registration rather than reaching runtime. The exported accessor that could be handed a nil definition is gone — with it the silent 'unknown means classic' fallback, which was the dangerous direction because classic reads as no-remote-write and read-level risk. Callers get a capability from the compiled entry or from ResolveEventKey; an unknown key reports not-found. Dispatch moves from an interface with one method per mode to a ProvisioningKind -> Provisioner registry. Adding a delivery mode now means adding the value to the catalog's declarable set and registering a Provisioner; the use case's dispatch does not grow a branch and existing provisioners are untouched. Each Provisioner receives the descriptor including DryRun, so preview semantics belong to the mode rather than to a separate no-setup pseudo-mode. Unknown or unregistered modes fail closed at both ends, and Risk reports write for anything not KNOWN to be write-free. The catalog owns which modes may be DECLARED; the registry owns which are IMPLEMENTED. Dispatch checks the latter, so a mode can be implemented without editing the dispatcher. The open-closed test now registers a third mode and asserts the existing use case routes to it — replacing a test that only compared unused string constants.
…side effect Registration was an init() in the events package: importing it — even blank — mutated a global registry, and the freeze happened separately in main. Nothing stated where the catalog came from, and any entry point that forgot the import got an empty one. events.All() now returns the declarations and catalog.Compile registers and freezes them. Compile is called from cmd.buildInternal, the one assembly function every entry point funnels through (main, the plugin harness, Build callers), so no entry point can miss it and the build step is explicit. Compile is idempotent, so a process assembling several root commands is fine. Test binaries that need the real catalog now say so in a TestMain, via CompileForTest — which registers without freezing, because tests legitimately add synthetic keys that a frozen catalog would rightly reject. That is a separate, explicitly named entry point so production cannot reach the unfrozen path by accident. The blank imports that no longer registered anything are removed.
…omain The event domain suggests a "next step" on almost every failure it can produce, and it spells those suggestions as `lark-cli ...` command lines. That couples the domain to one specific front end: the domain cannot be reused behind another caller without lying about how to recover, and a renamed flag or relocated command silently rots hint text spread across nine files. Introduce the vocabulary the domain will state instead: a Kind plus the operands a presenter needs, built through constructors so an action can never be half-populated, and a Context carrying the renderer a caller injects. With no renderer the action presents itself semantically rather than guessing at a command line — a library caller must see the action it was handed, not a command for a front end that may not exist. Kinds() is the enumeration a presenter's completeness test drives over, and an AST check keeps it in step with the declared constants, so a new action cannot ship with no presentation at all. This commit only adds the package; nothing depends on it yet.
… in the CLI Every `lark-cli ...` command the event domain used to spell is now a semantic recovery.Action, and cmd/event/render is the single place that turns one into a runnable command line. A recovery suggestion is a decision about WHAT to do next, which the domain owns; how to type it is the front end's business, which the domain had been guessing at in nine different files. The renderer is injected, not imported: CommandContext gained the render seam it already had the profile/identity for, and the domain entry points that produce suggestions without one (the catalog resolver, filter validation, bus startup, the bus-eligibility probe, the subscription planner) now take that seam as an argument. cmd/event builds it once per invocation through render.NewCommandContext, so a suggestion still targets the same app and identity the failure happened under. The rendered text is unchanged, deliberately: the renderer reproduces the exact strings the domain used to build, including which forms carry the --profile head and --as and which are bare app-independent lookups. The tests that assert that operator-facing text now compose the domain with the production renderer, so they still assert what a user sees rather than a copy of it. Two sites keep the semantic form because neither of their callers is a front end: the platform adapter's missing-user-token error (constructed by both the CLI and the bus daemon) and the subscription controller the bus's lifecycle action builds, whose output goes to a log. Threading a renderer into a transport adapter to spell one command would be the coupling this change removes.
The recovery split is only worth its churn if it cannot silently regress: adding one `lark-cli ...` string back into a hint is the easiest thing in the world to do while fixing an unrelated bug. Two checks. A source scan over every production file under internal/event fails on the binary name, with one allowlisted exception (a Windows named-pipe address, which is an IPC endpoint, not something a user can run) that has to state its reason. And a seam test in cmd/event asserts the exact command text an operator sees for the resolver and filter rejections, which is where the byte-for-byte claim about this refactor actually lives — the domain names the action, the render layer spells it, and the two together must still produce what they produced before. The scan is a source scan rather than an AST/string-literal scan on purpose: a command line can be assembled from a raw string, a concatenation, or documented in a comment, and all of those are the same violation.
The recovery seam threads a renderer in per invocation, but a handful of adapter constructors are reached from several entry points and are handed no context at all: platform/lark's subscription client is built from eleven call sites by both the CLI and the bus daemon it forks. Its missing-UAT hint therefore fell back to the semantic form, degrading shipped operator text from `lark-cli auth login` to a bare `auth_login` token. Give the domain a process-wide fallback renderer that a front end installs once at composition time, next to the event catalog compile, and resolve those context-less sites through it. The per-invocation renderer still wins wherever one was threaded in -- it is the only one that knows the profile and identity the invocation resolved. A process that installs none (a library embedding the domain, and any test that does not opt in) still gets the semantic form, so the domain remains free of command text. Covered both ways: the unit tests pin fallback, precedence and restore; cmd asserts that building the root command makes those actions render as the shipped commands, and the assertion was mutation-checked against a build that skips the install.
…tore The facts that describe one remote Subscription -- the lifecycle summary, the lifecycle action outcome, the decryption accounting, and the subscription and decryption health dimensions -- are shared by every consumer connection bound to that subscription. They were copied into each connection, so every writer had to walk the connection list and update each copy, and any path that missed one left two consumers of the same subscription disagreeing about its state. Give them one owner. binding.DeliveryBinding holds them behind named mutators, reads leave through an immutable Snapshot, and no mutable pointer -- in particular no *health.Facts -- crosses the package boundary. binding.Store owns every live binding keyed by remote_subscription_id and reference-counts it, so it outlives any single connection but is dropped once the last one goes; an empty id is never a key, since a classic consumer has no remote Subscription to share. health gains a value-typed Record so a consumer's dimensions can have more than one owner: each projects exactly the dimensions it owns (RecordOf) and Merge combines them, leaving the ordered Snapshot/NextAction projections identical to the single-owner case. Facts.Snapshot/NextAction/Any now go through it. Nothing references the aggregate yet -- this commit only adds it.
Conn was four objects in one. The third group -- the facts about the remote
Subscription rather than about the connection -- is now referenced instead of
copied: Conn keeps its transport, consumer-session and identity state and points
at the shared DeliveryBinding for its remote_subscription_id. Every read of a
subscription-scoped fact projects that binding, and the connection stores none of
them.
Health is split by ownership rather than duplicated: identity, source and
delivery stay on the connection; subscription and decryption move to the binding.
healthRecord merges the two owners back into one dimension-ordered view, so
HealthSnapshot and the next_action projection behave exactly as they did when one
record held everything -- a per-connection identity rebind still outranks a
shared subscription recovery.
Every writer now writes that one binding instead of iterating connections:
- the lifecycle reducer and its effect runners (summary, suspension code,
action outcome, subscription health). lifecycle.Conn is narrowed to the
connection's OWN identity state and the new lifecycle.Binding carries the
rest, so a lifecycle action can no longer reach a per-connection copy at all;
- the executor's queue-full degrade;
- the management-plane MarkLocalUpdate signal;
- the decrypt accounting, which now counts one upstream failure ONCE for the
subscription rather than once per watching consumer.
A reduction's scope no longer selects which copies to write -- there is one fact
-- but it still decides whether the decision applies: an eligible-scoped
decision needs an owner==current consumer, an all-scoped one needs any consumer.
Lifetime is reference counted by the Hub's store: the Hello-time attach and the
registration attach are idempotent, and Conn.shutdown -- the one funnel every
close path including a handshake reject goes through -- gives the reference back,
so the binding outlives any single connection but not all of them. A classic
consumer has no remote_subscription_id, so it gets no binding: none is
fabricated, its per-connection health is untouched, and the subscription-scoped
reads report the zero values they always reported.
Behavior is unchanged: event status text and --json, the ready marker, exit
codes, typed errors and the lifecycle semantics (terminal protection,
tombstones, per-id FIFO) all stay as they were. Only WHERE the state lives moves.
Three invariants the refactor stands or falls on, none of which the existing
suite could state before because there was no aggregate to state them about:
- two consumers of one remote_subscription_id share ONE binding -- stored once,
referenced twice, the same object -- so a single lifecycle reduction and a
single management-plane signal are both visible in BOTH consumers' status
projections, and one upstream decrypt failure counts once rather than twice.
This is the anti-drift gate: with a binding per connection the reduction
lands on one of them and the two consumers disagree;
- a classic consumer (no remote_subscription_id) gets NO binding: none is
fabricated for the empty id, its per-connection identity health still
round-trips through the next_action projection, its subscription-scoped reads
stay at their zero values, and it shares nothing with a refined consumer
registered alongside it or with a second classic consumer;
- the binding is released when its LAST referencing connection closes. It
survives an earlier close with its state intact, a repeated close cannot
double-release and drop an unrelated subscription's binding, a connection
rejected mid-handshake still gives its reference back, and the store returns
to empty once every connection has closed.
internal/event presented 20 sibling directories with no visible layering, so nothing in the tree said which of them are kernel domain code and which speak to the outside world. Five of them implement the local-bus IPC: the wire types and codec (protocol), the per-OS socket/named-pipe mechanics (transport), the control operations a client issues over it (busctl), the bus-endpoint discovery and orphan detection (busdiscover), and the local-bus read model (buslocal). Move all five under internal/event/adapter/localbus/ so the parent directory states the boundary. This is a path change only: each package keeps its name, its files and every exported identifier, and the dependency graph is unchanged (protocol/transport/busdiscover stay leaves, busctl depends on protocol and transport, buslocal on all four). They stay five packages rather than one flat localbus package on purpose: the names document a real internal layering, and merging them would risk identifier collisions for no dependency-graph benefit. The visible adapter boundary is what this step is after, and a parent directory delivers it.
The two packages that talk to Lark itself sat in the same flat namespace as the domain: source drives the WebSocket long connection through the SDK's ws client, and platform/lark owns the remote Subscription management plane. Move them to internal/event/adapter/lark/websocket and internal/event/adapter/lark/subscription so the tree names them as the outbound edge they are, and so adapter/ collects every package that leaves the process. Path change only — files, package names and exported identifiers are untouched, and the dependency direction is unchanged (websocket depends on the subscription gateway, never the reverse). Doc comments that cited a moved package by its old import path are retargeted so they still point at real code. Because the package clauses are unchanged, the two directory names no longer match their package identifiers (websocket declares package source, subscription declares package lark). Every import site names that identifier explicitly, so the binding is visible where it is used. Renaming the clauses is deliberately not part of this step: package subscription would collide with the kernel's own internal/event/subscription in the files that import both, and it would rewrite every call-site qualifier in a change that is otherwise a pure relocation.
The relocation left 44 comments across 32 files naming platform/lark, a
directory that no longer exists, so a reader following the reference found
nothing. Point them at lark/subscription instead, and spell the two
fully-qualified citations with their new import path.
Comment text only — no code, no string literal and no test expectation is
touched. Two sentences that read badly after the substitution ("a
lark/subscription type", "lark/subscription's subscription client") are reworded
to say what they mean.
The adapter/ grouping makes the boundary visible, but nothing yet stops the dependency direction from inverting: a domain package importing the local-bus wire format or the Lark gateway still compiles, still passes every test, and quietly welds the domain to one transport. Add an AST gate that fails instead. The gated set is derived from the directory tree — every directory under internal/event except the four documented outward-facing ones (adapter itself, the bus daemon, the consume client, and the transport test double) — so a package added tomorrow is covered the day it appears rather than when someone remembers to enrol it. That currently means the eight kernel packages the design names plus app, binding, delivery and the event facade. Two tripwires keep the gate from decaying into a no-op: the design-named kernel list must still exist in the tree (so a rename, or an exemption added to silence a failure, fails loudly instead of shrinking the gated set), and the adapter detector is run against a synthetic violating file (so a path constant that stops matching after a future move is caught). Production files only. A kernel package's own tests may drive a real adapter on purpose — internal/event's integration tests start the WebSocket source — and a test edge is not what ships.
The events/<domain> dependency gate named the forbidden packages by their old top-level directory (protocol / transport / busctl / buslocal / busdiscover / source). Those all moved under internal/event/adapter, so the check's leaf lookup started resolving to "adapter" and matched nothing: a domain importing the local-bus wire format passed the gate silently. Verified — the same violating import is green against the old list and red against this one. Key on "adapter" instead, which covers the whole outward edge in one entry and extends to any adapter added later, and keep bus / consume. Comment updated to describe the layer rather than enumerate packages that will move again.
…tories The relocation moved the packages but left their package clauses behind: adapter/lark/websocket declared `package source` and adapter/lark/subscription declared `package lark`. Opening either directory showed a package name that no longer described it, which is most of what the grouping was meant to fix. Renaming them costs nothing at the call sites: every importer already aliases these two, and a Go import alias is independent of the declared name, so the existing aliases keep compiling untouched. The only substantive edit is inside the websocket adapter, which reached its sibling through a now-misleading `lark` alias and now names it directly. Verified: build clean for darwin, windows and linux; vet clean; 33 packages green; -race clean; both goldens untouched; gofmt clean.
The check required BOTH sides to be non-empty before comparing, so it only caught contradiction, never loss. A header claiming app_id whose canonical counterpart arrived empty compared nothing, passed, and the event was emitted with app_id silently blank -- the exact outcome the canonical-metadata authority exists to prevent, reached through the check meant to enforce it. The asymmetry must run one way only: header silence is still no claim (a classic payload carries no subscription block, and treating that absence as a mismatch would drop every classic event), but once the header claims a fact the canonical value must MATCH it, empty included. An empty canonical against a claiming header is not "nothing to compare" -- it means the fact was lost between the ingress that wrote it and this consumer. Gated in the opposite direction from the existing mismatch test: instead of corrupting the header, blank the canonical value and assert the drop, driven over all nine rows. Mutation-checked -- restoring the old condition fails every one of the nine with the field named.
The kernel gate says the domain must not know its adapters. It says nothing about the hosts that legitimately do: "bus and consume may import adapters" read as "any adapter, and more of them over time", so the exemption could widen indefinitely while the kernel gate stayed green. That is how a boundary decays into a bucket -- a runtime host accumulating adapters is a composition root that was never collected. Pin each exempt host to exactly the adapters it imports today. The list is a ceiling recorded so growth is a decision made here with a reason, not a side effect of an import statement. It fails in both directions: an unexpected import names the file and points at injecting from cmd/event instead, and an entry nobody imports any more is reported as a ceiling drifted above reality -- an allowlist wider than the truth permits a re-introduction nobody reviews. The consume entry carries the sharpest case: it lists only localbus, so a consumer growing a lark/* import fails. A consumer reaching Lark directly has stopped going through the bus, which is the reason the bus exists. Mutation-checked both ways: adding adapter/lark/subscription to consume fails with the file named; adding an unused entry fails as stale.
…ient
SubscriptionPlan is a claim that a specific remote state was observed and
classified. With all fields exported, anyone could write
SubscriptionPlan{Action: ActionReuse, Before: someRemote} and Apply would reuse a
subscription nothing had checked. Unexport the fields, read the classification
through accessors, and route every path through one constructor so no plan can
name an action without the facts behind it.
Apply took (plan, policy, req) as three arguments that could disagree. The
refined consume path re-derived its Request at the call site, so a plan
classified against one request could be applied with another -- the plan's own
include_resource_data and filter conflict checks would then not describe the
write they gated. The plan now carries the policy and request it was classified
against, and Apply(ctx, plan) takes nothing else, so the classified thing and the
written thing cannot come apart.
Two guards, both mutation-checked:
- Apply rejects a plan it did not produce. Unexported fields stop most forgery at
compile time, but a zero value inside the package still gets there, and an
empty Action must not read as a default.
- NewPlanForTest exists for fakes in other packages that have no observation to
classify. An arch test fails if production code calls it -- verified red by
adding a call, and it counts the files it walked, because its own first draft
resolved the repo root to ".." and silently walked nothing.
The test rewrite preserved each call's original policy and request rather than
defaulting them: passing Request{} made the encrypted-create tests build a plan
whose request contradicted the assertion, which is the same defect one layer up.
… land
Giving the shared subscription facts one owner removed the drift between
connections, but left two gaps inside the aggregate itself.
A multi-fact write was a sequence of independently-locked setters, so a reader
could catch it half-applied. A lifecycle reduction records what happened, the
resulting remote state, the suspension code, and whether the subscription is
degraded -- and `event status` landing mid-sequence reported a combination that
never held: degraded with no reason yet, or a new remote state still paired with
the previous suspension code. Mutation now applies a group under one lock, health
dimensions included (the lock order is mu then health, never the reverse), and
Snapshot is taken as one consistent cut so the revision it carries describes every
fact in it.
Worse, a reduction that consults the remote before writing had no way to notice
that newer state arrived while it waited. runReconcileGet reads the state, issues
a Get that can take arbitrarily long, then writes what it concluded -- so a
suspended_v1 landing mid-flight was silently overwritten by a conclusion drawn
before it. It now reads the revision before the round trip and writes through
CompareAndApply; losing the race drops the conclusion rather than applying it,
and is not retried, because the correct response to losing it is to re-observe.
The action result is folded into that same mutation rather than written first.
That keeps the write atomic AND keeps the revision test free of arithmetic -- an
earlier draft guessed "beforeGet+1" and was already wrong, since markActionResult
wrote two facts.
Every setter now routes through Apply, so no write can leave the revision behind
and let a later CompareAndApply succeed against a value that no longer describes
the binding; a test drives all ten write paths to hold that. The atomicity test
is mutation-checked: restoring the split write makes it report the exact tear
("state=suspended but degraded=false").
Compile registered declarations into a package-level map, set a frozen flag, and returned nothing. There was no compiled artifact anyone could hold, and the whole-catalog Validate — which exists precisely for the defects a per-declaration check cannot see — was never called by anything. Compile now returns a Snapshot: the compiled catalog, projected per key onto a Descriptor (the declaration/display facts), a RuntimeBinding (the four hooks as the same function values, plus the delivery knobs), and the already compiled SubscriptionCapability. KeyDefinition stays exactly as it is — `events/` keeps declaring it and the render path keeps consuming it — but it is now a projection of the compiled model rather than the model itself, and Entry.Definition reassembles it so the split cannot silently drop a fact. Compile also runs the FULL Validate before freezing and panics on failure. A duplicate template path segment, a schema that parses but declares no fields, an event_type colliding with a reserved lifecycle meta-event, a filter meta naming an operator its event_type does not publish: each of those was reaching runtime as quietly wrong behaviour instead of a startup error. CompileForTest stays lenient on purpose, because test binaries register deliberately malformed keys to prove a gate bites. A Snapshot is immutable from outside: unexported map, accessors that hand back values or independent deep copies, never a pointer into the snapshot — the same ownership discipline as internal/event/binding. The package-level registry is untouched; every existing reader still goes through Lookup. Moving them onto the snapshot would touch 15+ files for no correctness gain and is a separate step.
ResolveEventKey and CompiledCapability reached into KeyDefinition's unexported capability field, so two call sites were interpreting a declaration alongside the compiler that already had. Both now read the compiled Entry. Only the compiler writes that field and only the projector reads it, which is what makes it an artefact of the compile step rather than a member of the declaration. ResolveEventKey also takes its Definition and its Capability from the same entry, so the pair it returns is one projection of one declaration and cannot come apart. No behaviour change: entryFor derives from the live registry, so a synthetic key registered by a test after CompileForTest still resolves.
…leak `event list --json` and `event schema --json` are an external contract: AI callers and scripts parse them. The only thing standing between that contract and a new field was the golden files, and a golden cannot carry this guarantee — the moment someone adds a field and regenerates the golden, the comparison agrees with the new output. The gate reflects over the two structs the commands actually marshal and requires every exported member to be either named in a written-out declaration allowlist or explicitly `json:"-"`. Embedded structs are followed, because that is how KeyDefinition's members reach the output, and the walk asserts it actually reached them — a traversal that inspected only the four wrapper fields would pass while a leak sat in the promotion. The two marshalled shapes move from function-local types to named ones so they can be reflected over. Same fields, same tags, byte-identical output.
…onsume path PreConsume is how a classic EventKey arranges delivery it cannot arrange locally — a server-side subscribe — and its cleanup is the matching unsubscribe. Both are gated on facts only the bus knows: setup on the handshake's FirstForKey, cleanup on the pre-shutdown ack's LastForKey. Both gates had only unit coverage of the pieces, and neither gate's timing was asserted anywhere. The consequences of getting either wrong are remote and shared. Lark keys a subscription record by (app, user, event_type) and overwrites rather than reference-counts it, so a second setup subscribes twice over one record, and a cleanup that fires while co-consumers are attached unsubscribes the record they are still receiving on — with no local symptom at all. Two consumers now attach to a real bus over a fake transport and are detached in a controlled order, asserting exactly one setup and one cleanup. Calling the hook directly would only prove the closure counts; what needs proving is when production invokes it. The second scenario is separate on purpose: in the first ordering the consumer that leaves early holds no cleanup, so deleting the LastForKey check outright leaves that test green. There the setup owner is the one leaving early, and the bus's "you are not last" answer is the only thing between it and a remote unsubscribe. Each bus gets a unique app id: bus.alive.lock is keyed by app id and held on an fd the OS releases only at process exit, so sharing one across buses in a test binary fails a `-count=N` run as a bogus "bus did not come up".
Making Mutation atomic only made an atomic write possible; both lifecycle callers still issued the pair as two setters. degradeSubscription and applySubDecision(subSet) each wrote SetSubscriptionDegraded then SetSubscriptionNextAction, so a status query landing between them saw a degraded subscription with no next_action -- a failure printed with no step beside it, which is the exact state pairing them exists to prevent. Both now write one binding.Mutation through a shared degradeMutation helper. Take the two setters off the lifecycle.Binding port as well. They are one fact in two fields, and offering them separately is an invitation to sequence them again; production lifecycle code can no longer do so, while the concrete binding keeps them for tests that set arbitrary prior state. Gated three ways, all mutation-checked against the restored two-setter form: a concurrent sampler per caller (the defect is a window, so asserting the final state would pass against the old code), plus a deterministic check that one degrade advances the binding's revision by exactly one -- two writes advance it by two, the same defect without needing a race to catch it.
All four were reachable, and three were introduced by the work that was meant to prevent exactly this class of defect. Seed ran after publication. Acquire inserted the binding into the store and the caller seeded it afterwards, but Seed overwrites every scalar, so it is only correct on a binding nobody else can reach. In between, a second Acquire could take it and BindingFor -- the lifecycle and management-plane write path -- could resolve it out of the same map; the bus handles connections concurrently and a refined key allows several consumers, so anything either wrote was then erased. The store now seeds under its own lock before publishing, and the caller hands in the seed value instead of applying it. Reactivate and Renew cleared a degradation newer than their own conclusion. Both clear on success, but that conclusion describes the state the remote call started from, and MarkLocalUpdate can write remote_subscription_conflict/get while the call is in flight. Both now capture the revision before the call and land the action result and the clear as one CompareAndApply, dropping the conclusion when something else got there first. A plan could be edited after it was made. Before() returned the internal *RemoteSubscription, so a caller could change the ID and have Apply reactivate a different subscription than the plan was a claim about -- verified: the test reactivates "sub_ATTACKER" without the fix. Request.Filter was shared by the struct copy, so mutating the tree after planning made create() send a filter the plan never validated while still reporting it as compatible. newPlan now deep copies every mutable input, and the accessors return copies; Filter and RemoteSubscription grew Clone methods in the model package that owns them. One status response could mix binding revisions. The row was assembled from a dozen per-field getters, each taking its own snapshot, so a concurrent write between two of them put facts from different revisions in one response. Conn.Status projects the whole row from ONE snapshot, with NextAction derived from the same Health value so it cannot disagree with its reason. On the write side Handle folded its summary, suspension and health decision into one mutation instead of three. Each fix has a regression test driving the real path -- the reactivate one uses a mid-call hook on the fake client so the conflict lands exactly where the race is, rather than asserting the binding's CAS in isolation -- and each was shown red against the restored production behaviour and green after.
Three follow-ons from the previous round, each a place the mechanism was right and a path did not use it. Only the SUCCESS path was conditional. Reactivate, Renew and reconcile-Get all kept two unconditional writes on failure -- the action result, then the degradation -- so a MarkLocalUpdate conflict arriving during the call was overwritten by a conclusion drawn before it, replacing the real reason the subscription is unusable with a recovery that cannot help. A failure conclusion is no less stale than a success one. All six outcome paths now go through one applyActionOutcome that folds the result and the health effect into a single CompareAndApply, and the revision is captured above the client build as well as the call, since identity resolution can block too. The conditional clear was a genuine TOCTOU, and the "idempotent" comment I wrote defending it was wrong. mergeSubDecision read the current reason, decided, and left the caller to Apply later; a conflict landing in between was erased by a clear that had never been evaluated against it. A conditional clear is not idempotent under a concurrent writer -- the condition and the act have to be atomic with EACH OTHER, not merely each atomic alone. The test now travels with the mutation as ClearSubscriptionDegradedIfReason and is evaluated inside the binding's lock. The receipt was a second route into the plan. reuseReceipt returned plan.before as both Before and After -- one object, aliased twice and shared with the plan -- so editing the receipt edited the plan, and re-applying it acted on a different subscription. Verified: without the fix a second Apply of the same plan resolves to "sub_ATTACKER". Receipts now carry independent deep copies. Each fix has a test on the real path -- the two lifecycle ones drive Handle with a mid-call hook on the fake client -- and each was shown red against the restored behaviour and green after.
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
1 participant
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.
Summary
Adds refined (fine-grained) event subscription support to lark-cli end to end — managing server-side event Subscriptions, consuming events through the refined-subscription lifecycle, server-side event Filter, and encrypted resource data — built on a hexagonal event-subsystem architecture with a single owner per domain fact. Draft for review; CI is red pending an upstream
oapi-sdk-gorelease (see Test Plan).Feature changes
cmd/event/subscription/):create,get,list,renew,reactivate,delete,update,status— full lifecycle, each with--dry-run, identity resolution, and scope preflight; state-changing mutations read-before-write, destructive ones confirmation-gated.cmd/event/consume.go,internal/event/consume/): reconcile against the remote Subscription, conflict detection with typed recovery hints, the Hello/registration handshake.--filter/--clear-filteron create/consume/update, Filter as a reconcile-reuse conflict dimension, remote-filter display in reads, and Filter as anupdated_v1bus-compat dimension.header.subscription).--include-resource-datahandling and the decrypt-key path.internal/event/bus/): the local event bus, subscription lifecycle events, and the requested-vs-remote cross-check.eventcommand help and thelark-eventskill reference.Architecture
The event subsystem follows a hexagonal / clean layering — the domain is inner, adapters implement inward, the command layer is thin. The boundary is enforced by tests, not convention:
internal/event/model— SDK-free value objects (RemoteSubscription,Filter,OwnerRef, …), each with a deepCloneso a value crossing a decision boundary stops being the caller's to change.internal/event/catalog— the EventKey registry.Compile(events.All())is the one explicit build step: it registers every business declaration, validates the whole catalog (cross-declaration defects — a shadowed template path, a business event_type colliding with a reserved lifecycle meta-event — are only visible once every declaration is present), freezes it, and returns an immutableSnapshotprojecting each key into its declaration view, its runtime hooks and its delivery capability. It is called from the single assembly function every entry point funnels through, so no entry point can run on an empty or unvalidated catalog.internal/event/subscription— the domain owner: it defines the outboundGatewayport and its request specs, and ownsObserve → Plan → Apply(the only remote-write path). ASubscriptionPlanis unforgeable and self-sufficient: its fields are unexported, it carries the policy and request it was classified against, andApply(ctx, plan)takes nothing else — so what gets written and what was validated cannot come apart.internal/event/binding— theDeliveryBindingaggregate: the facts belonging to one remote Subscription, which every consumer bound to it shares. They have one writable home instead of a copy per connection, mutations apply as one versioned step, and a decision made from an earlier read is rejected rather than allowed to overwrite newer state.internal/event/adapter/**— the outward edge, grouped so the dependency direction is visible:adapter/lark/{websocket,subscription}for the platform,adapter/localbus/**for the local-bus IPC. No kernel package imports an adapter, and each host that legitimately does is pinned to an explicit allowlist.internal/event/app— consume/status/subscription use-cases;internal/event/session— the fail-closed owner/current identity gate; plushealth,routing,delivery,recovery.cmd/event/renderis the single place that turns one into a runnablelark-clicommand carrying the resolved--profile+--as. Renaming a flag or moving a command is one edit there instead of a hunt through the domain. Status recovery is owner-first: it maps a consumer's owning app → profile and only emits an executable command when that profile currently resolves to the consumer's owner (else a reason-only hint), never falling back to the invocation's profile.Correctness / hardening
updaterequires--yeswhen a filter change affects — or, if the local-bus scan/query is incomplete, cannot be confirmed not to affect — a running local consumer. Subscription listing fails closed on a broken pagination chain rather than silently truncating.statusread can never observe a lifecycle reduction half-applied — no failure without the recovery beside it, and no new remote state still paired with the previous suspension code. Any conclusion drawn before a blocking remote call is applied conditionally on nothing having changed since, and dropped if it lost the race; re-observing is left to the next lifecycle event. Conditional clears are evaluated inside the aggregate's lock, so the condition and the act cannot come apart.--dry-runmirrors the real outcome (would-block vs runnable),--quietkeeps the preview,scopes_oknever claims an unverified green light, and renew/reactivate/consume previews plan from observed remote state.subscription_event_id; ordered per-id lifecycle reduction; per-dimension health facts + recovery actions.Test Plan
go test ./internal/event/... ./cmd/event/... ./events/... ./cmd, 34 packages), race-clean under-race.go buildclean for darwin,GOOS=linuxandGOOS=windows;go vet ./...clean.event list/event schema, JSON and text) byte-identical — never regenerated. A separate reflection test asserts no runtime-only field can reach that output, because a field withomitemptyand an empty value would leave the golden unchanged.Related Issues
🤖 Generated with Claude Code