feat(event): compile the catalog and harden the consume pipeline - #2142
Merged
Conversation
The whiteboard schema golden gains the subscription-key marker on whiteboard_id; that visible diff is the intended outcome of this fix.
…shot The runtime registry is gone: declarations aggregate through events.All, compile once at the command tree's assembly point, and every reader (list, schema, consume, suggestions, bus) works from the immutable snapshot. Byte-identical golden output pins the migration.
…tion seam The stream host keeps its connect/prepare/stream sequence and panic-safe cleanup exactly as before; what changed is who supplies the preparation: the application layer injects the decided strategy, and the declaration's own hook remains the fallback for direct library callers.
…ctors Kernel packages are derived from the directory tree so new packages are gated the day they appear; host adapter imports are pinned by a two-way allowlist (unexpected imports fail, stale entries fail); the detectors prove themselves on synthetic violations. Header re-parsing in domain packages is pinned by a shrink-only baseline until the residue is gone.
…d headers Domain processors now take event id, type, source time, and tenant identity from the canonical event the pipeline restored; the per-file header envelope blocks are gone and the header re-parse gate is zero-tolerance. The per-key stdout baseline is byte-identical.
leave330
commented
Aug 3, 2026
Extending the default generic-api-key rule appends the allowlist to that rule alone, and the AND condition additionally bounds it to the event catalog golden fixtures, so the dotted-identifier exemption can no longer mask a match from any other rule or any other path.
leave330
commented
Aug 3, 2026
The unapproved-domain guard rejects cdn.example.com and open.feishu.cn.example.com in internal/cmdutil and internal/core unit tests. Both are RFC 2606 example subdomains used only as fixtures, and the reserved-name exemption covers the bare example.com names only, so they belong in the fixture list next to the attacker/evil entries. The tests predate the guard and their last green run predates it too, so every pull request whose diff window includes them currently fails lint on files it did not touch.
The vc and minutes PreConsume implementations were byte-identical copies, so the register/unregister pair moved to a shared helper. Deleting both domain files to get there read as if those domains had lost their PreConsume, and it took away the place a reader looks for how a domain subscribes. Each domain keeps its own preconsume.go and its own documented subscription semantics, delegating only the shared OAPI dance. The key declarations are unchanged. Also covers the shared helper directly: nine keys across three domains now depend on it, including the bounded cleanup that lets an exiting consumer unsubscribe under a cancelled consume context.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@events/internal/subscribeprep/subscribeprep_test.go`:
- Around line 117-157: Strengthen the error-path tests for Hook and
SubscribeWithCleanup by using a sentinel client error and asserting the returned
API failure’s typed metadata via errs.ProblemOf, while also verifying errors.Is
preserves the sentinel cause; ensure the stub/API failure is classified through
errclass.BuildAPIError or runtime.CallAPITyped. In
TestHook_RejectsMissingAPIClient, assert the typed internal error category and
errs.SubtypeUnknown in addition to the existing cleanup checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 190d97e7-e080-407b-a29c-5a76effdb750
📒 Files selected for processing (3)
events/internal/subscribeprep/subscribeprep_test.goevents/minutes/preconsume.goevents/vc/preconsume.go
The error paths only checked for a non-nil error, so a rewrap that dropped the client's category, subtype or retryable flag would have passed. They now assert the client's typed problem reaches the caller unchanged, and that a missing API client yields the typed internal error this package documents.
leave330
commented
Aug 3, 2026
Decoding the payload header into typed strings fails as soon as one field carries the wrong JSON type, and the arbiter treated that error as "no conflict". Because encoding/json still populates the fields it can decode, a header could pair one type-flipped field with forged app_id and tenant_key values and reach a native key's stdout unchallenged -- the opposite of the fail-closed contract this check documents. Header values are now decoded one at a time, so a badly-typed field no longer discards the claims beside it. The envelope declares every one of these facts as a string, so a non-string assertion is itself a conflict; JSON null still means absent and stays silent.
Scoping the subscription per whiteboard made the server-side setup run for every board, but the local bus still fans out by event type, so each consumer received every subscribed board's events -- including the first consumer, which saw only its own board before. The key now declares a Match filter on the payload's whiteboard_id, the existing pre-Process hook the bus already calls. An event whose board cannot be read is dropped rather than delivered: a consumer that asked for one board must not be handed an event that cannot be attributed to it. Routing per board on the bus would additionally save the IPC hop and stays open as a follow-up; it needs the handshake to carry the filter and the hub to read payload contents.
…data Refusing every key on an older bus left no upgrade path. The bus is a long-lived daemon on the user's machine and its 30-second idle exit only applies while it has no connections, so one supervised consumer keeps an older bus alive indefinitely; the refusal also shares an exit code with "bad parameters", which reads to an orchestrator as "do not retry". Together that turns an upgrade into a silent stall rather than the one recovery round the notes promised. A connection whose handshake advertises no canonical metadata now enters a compatibility mode, decided once from the ack and read-only after that. A per-event fallback was rejected deliberately: it would let a forged header override canonical facts on a bus that does supply them. The older frame already carries event_id, event_type and source_time, so those stay authoritative and stay arbitrated. Only app_id and tenant_key are restored, from the payload header the older ingress parsed them out of. app_id is checked against the app this consumer is configured for, so it cannot be forged; tenant_key has no second source on such a connection and is the one accepted cost. Both are refused unless the header spells them as strings. Keys with a SubscriptionKey param keep refusing: their scope is hashed here and bare on an older bus, so old and new consumers of the same resource would each act as first and last for their own scope and unsubscribe one another, and the older bus can never grow a guard. That narrows the break from every key to one. The refusal still happens before any preparation runs. A per-key matrix over the shipped catalog asserts every key has a defined answer and that a restored legacy event matches what a current frame carries; the frozen output baseline then pins the rendered bytes.
The compatibility tests asserted on the restored canonical event and argued the rendered output followed from that plus the frozen baseline. The argument did not hold: a legacy frame has no observation clock, so its canonical event legitimately differs from a current one, and the control was built from another legacy frame -- so the one field the two formats cannot agree on was never compared. Any future handler reading that field would have kept both tests green while the output diverged. Output equivalence is now asserted directly: an old-format frame and an old hello_ack are replayed through the real consumer -- handshake, frame decode, compatibility restore, arbitration, Match, Process, sink -- and stdout is compared with the frozen baseline for every Processed key. The key that refuses a legacy bus is asserted on the same path. The field-level matrix keeps its place, now comparing against a frame that does carry an observation clock and stating that difference as the single recorded divergence. The refusal regression is also bounded: it ran on a background context, so a negotiation that wrongly degraded entered the consume loop and hung until the package timeout, reporting a timeout instead of the missing refusal. It now fails in about two seconds with the real reason.
The ingress accepted event bodies up to 1 MiB and the frame limit was the same 1 MiB, but a frame carries the canonical metadata on top of its payload. A body in the top few hundred bytes of the accepted range framed into something over the limit: the bus wrote it, the consumer refused to read it and warned, and the event was gone. Long transcripts reach that band. The frame limit is now the payload budget plus a stated overhead allowance, so anything the ingress accepts can be framed and read back. A boundary test frames a maximum-size payload with long metadata and round trips it, and a second test pins the ingress cap against the wire format's budget -- they are declared in separate packages because the platform ingress must not depend on the local framing, so a test is what keeps them in step.
The stdin EOF watcher was armed before the decision was executed, so a run refused for an unmet precondition still announced "stdin closed -- shutting down" on stderr. For a subprocess caller that reads stderr to find out why a consumer exited, that names the wrong cause: nothing had closed stdin, and the run never reached the stream. It now lives inside the runner, which a blocked decision never invokes. The blocked path also gains the tests it lacked. One pins the preview contract an orchestrator reads: ok and dry_run stay true and the exit code stays 0 because the preview itself succeeded, so status and the named blocked precondition with its detail are the only things that say a real run would refuse. The other pins the invariant that makes the watcher move correct -- executing a blocked decision returns the preflight's own error, hint intact, and never starts the stream.
Two properties lived only in the wiring, where each side looks correct alone and only the pair is wrong. The normalizer must run exactly once per consumer: deciding runs it to compute the subscription identity and tells the host to skip its own call. Both directions of that flag failed silently -- a second run for a hook that is not idempotent, or a host normalizing values the bus was never told about -- and nothing asserted the count across the two layers. The preparation the decision chose must be the one that runs. The injected path and the declaration's own hook call the same function today, so a broken injection would have produced identical behavior; the test counts a spy the declaration cannot see, which distinguishes them. Both were verified by mutation: dropping the skip makes the normalizer run twice, and ignoring the injected preparation drops its count to zero.
A blocked precondition rendered only the error's sentence. The preview exists to be read before acting -- it is the surface an agent consults to decide whether to run at all -- yet it was the one surface with no way forward: the classification and the recovery hint the same failure puts in a real run's error envelope were dropped on the floor. Each precondition now carries subtype, hint and, where the failure is a missing grant, the concrete scopes, all omitted when the check passed so a healthy preview stays as terse as before. The blocking error already held all of it; only the view discarded it. The renderer converts the view struct directly, so the exported model and the JSON model cannot drift apart without breaking the build.
The seam tests asserted that the stream host honours the normalized-params flag and the injected preparation, but they built those options themselves. Deleting either line from the command left every test green -- verified by deleting them -- so the wiring the tests were meant to protect was not covered at all. The three assignments the command alone can get wrong now live in a small named function, and the tests call that function. Each of the three was confirmed to turn the suite red when removed. The fixture key takes a parameter and the incoming options carry a value the decision must overwrite; with a parameterless key the parameter assignment had nothing to observe and stayed unpinned on the first attempt. The function does one thing: transfer the decided parts onto the options. Signal handling, the stdin watcher, the transport and the runtime stay where they were.
The bus stub has to live in a non-test file so other packages can import it, which subjects it to the framework bans that test files are exempt from. It now goes through vfs for its temp directory, like the rest of internal/, so it needs no exemption at all. A redundant conversion of an aliased function type is gone too.
8 tasks
The skill no longer documents --dry-run: the preview earns little for an agent this cycle, and describing a path agents rarely take costs budget in a document that has to stay short. --domain now lists the eight domains inline instead of leaving the agent to guess one and learn from the rejection, matching what the flag's own help already prints.
# Conflicts: # cmd/build.go # cmd/event/consume.go
liangshuo-1
approved these changes
Aug 5, 2026
zkh-bytedance
pushed a commit
that referenced
this pull request
Aug 6, 2026
Co-authored-by: TRAE CLI <traecli@bytedance.com>
6 tasks
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
2 participants
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
This PR pays down long-standing technical debt in the event module without rewriting it: it gives event metadata a single source of truth, turns EventKey registration into a compile-time step, separates the consume decision from its side effects, draws an explicit kernel/host/adapter boundary, and locks all existing behavior behind compatibility tests before any of that happened.
The resulting main line: the platform WebSocket ingress parses each envelope once into a canonical event; the local bus carries it in full; EventKey declarations compile at startup into an immutable catalog snapshot; the consume command forms a structured decision first and only then acts on it; domain processors touch only their business payload and public output.
The strategy throughout: strict contracts on the shared pipeline, restraint on legacy domain code; byte-identical normal output, explicit failure for error and conflict cases.
What was wrong
event_id/event_type/create_time, but consumers restored only a subset from the IPC frame, so 13 domain processors re-parsedpayload.headerthemselves — multiple copies of the same facts with no authority when they disagreed.init()side effects; list/schema/consume/bus each read global state; validation ran per-registration, so whole-catalog invariants were never checked.KeyDefinitioncarried display fields, schema, delivery knobs, and function hooks all at once; a malformed payload could push the raw V2 envelope into a processed key's stdout — a shape its schema never described.Changes
The work is staged in phases, so every migration lands behind a gate that proves it changed nothing it should not have.
Architecture
internal/event/{model,catalog,processing,application/consume}events/<domain>internal/event/{bus,consume}internal/event/adapter/{lark,localbus}cmd/eventArchitecture tests pin the direction: kernel packages cannot import hosts, adapters, cobra, or the platform SDK; host→adapter imports sit behind an exact two-way allowlist (unexpected imports fail, stale entries fail); the detectors verify themselves against synthetic violations.
Key mechanisms
event_id,event_type,create_time,app_id,tenant_key); the IPC frame carries every field (additive, withobserved_atas a fixed RFC3339Nano string), consumers restore them in full, and a table-driven arbiter drops any event whose payload header contradicts the canonical facts — including the case where a canonical fact went missing.events.All()aggregates declarations explicitly;catalog.Compilecanonicalizes defaults, validates the whole catalog (duplicate keys, schema placeholders, dangling field overrides, processless custom schemas, unresolvable strategies), resolves output schemas, and projects each key into Descriptor / OutputContract / Capability / RuntimeBinding inside an immutable snapshot. The runtime registry (RegisterKey/Lookup/ListAll) is gone.event consumenow forms an immutable decision (identity, normalized params, scope, precondition statuses, would-read/would-write sets) before anything is started, registered, prepared or written; a gate proves that separation across all 25 keys. Execution then acts on that same decision, and existing PreConsume hooks run through a compatibility strategy with unchanged first/last semantics.(nil, nil)business-filter convention is unchanged.canonical_metadata_v1on the real delivery handshake rather than decoding missing fields as empty values. The answer is decided once per connection: a key whose subscription identity is one-dimensional degrades into the compatibility mode described in the upgrade notes, while a key that subscribes per resource is refused explicitly, before any preparation side effect, with a recovery hint.New user-facing capabilities
event list --domain <d>— filter the catalog by domain at the snapshot query layer; unknown domains fail with the valid set listed.event consume <key> --dry-run— reports the decision instead of acting on it, with three-state preconditions (ok/unknown/blocked) and sensitive parameter values redacted. The skill documentation deliberately does not cover it; it is a diagnostic surface, not part of the agent-facing flow.Behavior changes and upgrade notes
Normal, well-formed event output is byte-identical (verified by frozen goldens, per-key stdout baselines, and a dual-binary byte-for-byte comparison across all 25 keys). The changes below tighten error semantics only:
Malformed processed payloads and metadata conflicts are dropped with a stderr diagnostic instead of passing the raw envelope through to stdout.
A missing upstream
create_timestays empty instead of being backfilled from the local clock; the local observation time travels separately asobserved_at.board.whiteboard.updated_v1consumers are scoped perwhiteboard_id(bug fix: distinct whiteboards previously shared one subscription scope, so a second consumer's server-side subscription never happened and an exiting consumer could unsubscribe a live one). Its schema now marks the parameter as a subscription key, and each consumer's stdout is filtered to the whiteboard it asked for — scoping the subscription alone would have left every consumer receiving every subscribed board's events, since the local bus fans out by event type. An event whose whiteboard cannot be read is dropped rather than attributed to the wrong board. Upgrade note: restart all whiteboard consumers after upgrading and avoid mixing old and new CLIs on the same whiteboard during the transition window.Attaching to a bus started by an older CLI: the connection enters a compatibility mode instead of failing.
app_idandtenant_keyare then read from the event payload — the same bytes the older ingress parsed them from — and a one-line notice on stderr names the mode. The payload'sapp_idis checked against the app this consumer is configured for, so it cannot be forged;tenant_keyhas no second source on such a connection, which is the one accepted cost. Everything the older frame does carry stays authoritative and stays arbitrated.board.whiteboard.updated_v1is the exception and still exits withfailed_preconditionplus a recovery hint: it subscribes per whiteboard, and its scope is hashed here but bare on an older bus, so old and new consumers of the same board would each act as first and last for their own scope and unsubscribe one another. Stopping the consumers attached to the older bus and runningevent stopis a prerequisite for that key. Note that the bus's 30-second idle exit only applies while it has no connections — one long-running consumer keeps it alive indefinitely.Error precedence in combined failure cases is now validation-first: invalid parameters are reported before missing scopes (exit 2 rather than 3 when both apply). Single-error outputs are unchanged.
An event body close to the 1 MiB ingress limit is now delivered instead of dropped. The frame limit used to equal the payload limit, so the metadata a frame adds pushed the top of the accepted range over it: the bus wrote such a frame and the consumer refused to read it, warning and moving on. Long transcripts reach that band.
A run refused for an unmet precondition no longer prints
stdin closed — shutting downon stderr. The watcher that emits it now starts with the stream, so the line means what it says instead of naming the wrong cause for the exit.Known limitation —
--quietand dropped events.--quietsuppresses per-event drop diagnostics along with the rest of stderr, so a run that loses events to backpressure, an oversized frame, a metadata conflict or a malformed payload produces no signal at all: stdout is simply short. This is pre-existing behaviour, not a change in this PR, and it is now stated in the flag's help text and in the skill documentation so the trade-off is an informed one. Unattended runs that must notice data loss should omit the flag. Giving those drops a channel--quietdoes not silence is deliberately left to a follow-up: doing it properly means correcting how the drop paths count (sequence reordering, fragmented frames, backpressure attribution) and touches the bus daemon, which does not belong in this PR.Unrelated CI fix carried along
internal/qualitygate/config/allowlists/fixture-domains.txtgains two test-only hostnames (cdn.example.com,open.feishu.cn.example.com) used by pre-existing unit tests ininternal/cmdutilandinternal/core. The unapproved-domain guard landed after those tests' last green run, so lint currently fails on every pull request whose diff window includes them — including this one, on files it does not touch. Verified on the merge tree: the guard passes with the two entries present and reports six rejections with them removed.Deliberate non-goals
Legacy EventKeys are not rewritten (public output types, field order, and JSON tags untouched; deduplication only in private helpers); Bus/Conn/Hub internals are not redesigned; compatibility aliases (
KeyDefinition,RawEvent,APIClient) keep declaration sites unchanged; heavier lifecycle models were intentionally left out.Test Plan
make unit-testpassedlark-cli event list --domain vc,lark-cli event list --domain bogus,lark-cli event consume vc.note.generated_v1 --dry-run— filter, typed error with the valid domain set, and the side-effect-free preview verified against a live tenantVerification depth behind those boxes:
go build,go vet, full unit/integration suites, andgo test -raceacross all event packages; incremental golangci-lint clean;go mod tidyproduces no changes (zero new dependencies).Related Issues
N/A