feat(event): compile the catalog and harden the consume pipeline - #2142
feat(event): compile the catalog and harden the consume pipeline#2142leave330 wants to merge 51 commits into
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.
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@b8ccd4d53cb3417d318eccea291f6141ca69ecde🧩 Skill updatenpx skills add larksuite/cli#feat/event-arch-refactor -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2142 +/- ##
==========================================
+ Coverage 75.69% 75.70% +0.01%
==========================================
Files 942 956 +14
Lines 100079 100369 +290
==========================================
+ Hits 75750 75986 +236
- Misses 18537 18572 +35
- Partials 5792 5811 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Nitpick comments (11)
internal/event/catalog/snapshot.go (1)
118-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
Snapshotdoc comment withResolveandEntries.Lines 118-119 state that accessors never return pointers into the snapshot's own state.
ResolveandEntriesreturn*Entryvalues that point intos.entries. The immutability property still holds, because everyEntryfield is unexported and everyEntryaccessor copies. The comment as written can lead a future maintainer inside packagecatalogto mutate through the returned pointer. State the actual rule instead.♻️ Proposed wording
-// Snapshot is the compiled, immutable catalog. Accessors return values or -// fresh copies — never pointers into the snapshot's own state. +// Snapshot is the compiled, immutable catalog. Resolve and Entries hand out +// *Entry handles into the snapshot; every Entry field is unexported and every +// Entry accessor returns a value or a fresh copy, so no caller outside this +// package can mutate compiled state. Code inside this package must not write +// through a returned *Entry.🤖 Prompt for 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. In `@internal/event/catalog/snapshot.go` around lines 118 - 147, Update the `Snapshot` type comment to accurately describe pointer-returning accessors: `Resolve` and `Entries` may return pointers to entries stored in the immutable snapshot, while `Entry` keeps its fields unexported and accessors return copies. Remove the claim that accessors never return pointers into snapshot state.events/im/catalog_helper_test.go (1)
38-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated
lookupCompiledDeftest helper. Three domain packages define the identical helper: compile the package's ownKeys()withcatalog.StrategyRefs{catalog.StrategyNone, catalog.StrategyLegacyPreConsume}, then resolve one key.internal/event/testutil/testutil.goalready exists in this PR as a shared test-support package and is the natural place to hold one generic version of this helper.
events/im/catalog_helper_test.go#L38-L54: replace thislookupCompiledDefwith a call to a shared helper ininternal/event/testutilthat acceptsdefs []event.KeyDefinitionandkey string, so this file only suppliesim.Keys().events/minutes/catalog_helper_test.go#L13-L29: replace thislookupCompiledDefwith the same shared helper, supplyingminutes.Keys().events/vc/catalog_helper_test.go#L13-L29: replace thislookupCompiledDefwith the same shared helper, supplyingvc.Keys().🤖 Prompt for 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. In `@events/im/catalog_helper_test.go` around lines 38 - 54, Consolidate the duplicated lookupCompiledDef helpers by adding one generic helper to internal/event/testutil that accepts []event.KeyDefinition and a key, compiles with the existing StrategyRefs, and resolves the definition. Update events/im/catalog_helper_test.go:38-54 to use it with im.Keys(), events/minutes/catalog_helper_test.go:13-29 with minutes.Keys(), and events/vc/catalog_helper_test.go:13-29 with vc.Keys(), removing each local lookupCompiledDef implementation.cmd/event/schema_test.go (1)
395-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove catalog-compile tests to the catalog package.
TestCompile_EmptySpecIsRejectedandTestCompile_InvalidBaseWithOverridesIsRejectedexerciseinternal/event/catalog.Compiledirectly. They do not callrunSchemaor exercise anycmd/eventbehavior.internal/event/catalog/compile_test.goalready exists in this PR and is the natural home for catalog-compilation unit tests.Move both tests to
internal/event/catalog/compile_test.goto keepcmd/event/schema_test.gofocused on command-layer behavior.🤖 Prompt for 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. In `@cmd/event/schema_test.go` around lines 395 - 424, Move TestCompile_EmptySpecIsRejected and TestCompile_InvalidBaseWithOverridesIsRejected from cmd/event/schema_test.go into internal/event/catalog/compile_test.go, preserving their assertions and catalog.Compile coverage. Keep cmd/event/schema_test.go focused on runSchema and other command-layer behavior.events/vc/test_helpers_test.go (1)
20-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
fillCanonicalFromHeaderhelper acrossevents/vcandevents/minutestest packages. Both copies parse the identical envelope header shape and copy the identical three fields onto*event.RawEvent; the shared root cause is the lack of a common test helper for this canonical-field synchronization.
events/vc/test_helpers_test.go#L20-L37: keep this as the canonical implementation, or move it intointernal/event/testutilso other event-domain test packages can import it directly.events/minutes/minute_generated_test.go#L330-L352: remove this copy and import the shared helper instead of redefining it.🤖 Prompt for 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. In `@events/vc/test_helpers_test.go` around lines 20 - 37, Centralize the canonical-field synchronization implemented by fillCanonicalFromHeader. Keep events/vc/test_helpers_test.go:20-37 as the shared implementation or move it to internal/event/testutil for import; remove the duplicate helper from events/minutes/minute_generated_test.go:330-352 and update its callers to use the shared helper.internal/event/catalog/compile.go (2)
109-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a typed validation error from
Compile.
Compilereports every declaration problem througherrors.New. Callers therefore receive an untyped error, and the command layer cannot classify it.cmd/eventcompiles the catalog at startup, so this error reaches a user-facing exit path.Wrap the joined problem list in the prescribed typed constructor for validation failures.
🛡️ Proposed fix
if len(problems) > 0 { - return nil, errors.New("event catalog rejected:\n " + strings.Join(problems, "\n ")) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, + "event catalog rejected:\n %s", strings.Join(problems, "\n ")) }
errorsstays in use forrenderSpec.Confirm that
internal/event/catalogmay importgithub.com/larksuite/cli/errs.internal/event/catalog/params.goalready imports it, andTestArchKernelPuritydoes not ban it, so the layering gate stays green.As per coding guidelines: "Use the prescribed typed error constructors for validation, failed preconditions, API failures, network failures, file I/O failures, and unknown lower-layer errors."
🤖 Prompt for 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. In `@internal/event/catalog/compile.go` around lines 109 - 111, Update Compile’s problems-return path to use the prescribed validation-error constructor from github.com/larksuite/cli/errs around the joined problem list, allowing callers to classify the failure. Keep the existing errors import and usage for renderSpec unchanged.Source: Coding guidelines
171-186: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject duplicate param names and out-of-set defaults at compile time.
The param loop validates the type and the
Valueslist, but it does not check two contracts that the runtime depends on:
- Duplicate
Nameentries.ValidateParamsininternal/event/catalog/params.gobuildsknownandvalidNamesfrom the same slice, so a duplicated name silently produces a duplicated hint list and an ambiguous default.- A
Defaultthat is not present inValuesforParamEnumandParamMulti.ValidateParamsinjectsDefaultbefore the required check, so an invalid default becomes an accepted parameter value.Whole-catalog validation is the right place for both checks.
♻️ Proposed addition
+ seen := make(map[string]bool, len(def.Params)) for _, p := range def.Params { + if seen[p.Name] { + fail("EventKey %s: duplicate param %q", def.Key, p.Name) + } + seen[p.Name] = true switch p.Type { case "", ParamString, ParamBool, ParamInt: case ParamEnum, ParamMulti: if len(p.Values) == 0 { fail("EventKey %s: param %q type %q requires Values", def.Key, p.Name, p.Type) } for _, v := range p.Values { if v.Desc == "" { fail("EventKey %s: param %q value %q requires non-empty Desc", def.Key, p.Name, v.Value) } } + if p.Default != "" && !slices.ContainsFunc(p.Values, func(v ParamValue) bool { return v.Value == p.Default }) { + fail("EventKey %s: param %q Default %q is not one of its Values", def.Key, p.Name, p.Default) + } default: fail("EventKey %s: param %q has unknown type %q", def.Key, p.Name, p.Type) } }Add
"slices"to the imports.For
ParamMulti, confirm whetherDefaultmay hold a multi-value list before applying the membership check.🤖 Prompt for 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. In `@internal/event/catalog/compile.go` around lines 171 - 186, Extend the parameter validation loop in the catalog compiler to reject duplicate non-empty names using a seen-name set, and validate enum defaults against the declared Values. For ParamMulti, first use the existing type definition and runtime handling to determine whether Default is a list; validate every default value against Values accordingly, preserving the intended empty-default behavior. Add the slices import only if the chosen membership check requires it, and emit compile-time failures through the existing fail function.cmd/event/suggestions_test.go (1)
100-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert typed metadata for the unknown-key error.
unknownEventKeyErrreturns a validation error withSubtypeInvalidArgument, so the test should asserterrs.ProblemOf(err).Categoryand.Subtypeinstead of only checkingerr.Error()substrings.♻️ Proposed addition
err := unknownEventKeyErr(compileCatalog(), "im.message.recieve_v1") if err == nil { t.Fatal("expected error") } + if p, ok := errs.ProblemOf(err); !ok { + t.Fatal("unknownEventKeyErr must return a typed error") + } else if p.Category != errs.CategoryValidation || p.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("category/subtype = %q/%q", p.Category, p.Subtype) + } msg := err.Error()🤖 Prompt for 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. In `@cmd/event/suggestions_test.go` around lines 100 - 113, Update the unknownEventKeyErr test to inspect errs.ProblemOf(err), asserting its Category and Subtype equal the expected validation metadata, including SubtypeInvalidArgument. Keep the existing message-content assertions for the unknown key and suggestion.Source: Coding guidelines
internal/event/application/consume/strategy.go (1)
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a typed error for an unregistered strategy.
getreturns a barefmt.Errorf. This error reaches the command layer, where it loses category and subtype metadata. An unregistered strategy reference is an internal invariant failure, so use the prescribed typed constructor.As per coding guidelines: "Use the prescribed typed error constructors for validation, failed preconditions, API failures, network failures, file I/O failures, and unknown lower-layer errors".
♻️ Proposed change
func (r *Registry) get(ref catalog.StrategyRef) (PreparationStrategy, error) { s, ok := r.strategies[ref] if !ok { - return nil, fmt.Errorf("preparation strategy %q is not registered", ref) + return nil, errs.NewInternalError(errs.SubtypeFailedPrecondition, + fmt.Sprintf("preparation strategy %q is not registered", ref)) } return s, nil }Match the exact
errsconstructor and subtype used elsewhere ininternal/event.🤖 Prompt for 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. In `@internal/event/application/consume/strategy.go` around lines 60 - 66, Update Registry.get for missing strategy references to use the established errs typed constructor and subtype already used elsewhere in internal/event, instead of fmt.Errorf. Preserve the existing unregistered-reference message and return behavior while ensuring the error retains its category and subtype metadata.Source: Coding guidelines
events/im/message_receive_test.go (1)
236-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the empty
SourceTimefallback.
runReceivealways callsfillCanonicalFromHeader, soraw.SourceTimeis always populated. The new fallback inprocessImMessageReceive(timestamp = msg.CreateTimewhenraw.SourceTimeis empty) is therefore never executed by these tests. Removing the fallback would not fail the suite.Add one case that builds a
RawEventwithoutSourceTimeand assertstimestampequals the messagecreate_time.As per coding guidelines: "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
💚 Proposed test
func TestProcessImMessageReceive_TimestampFallsBackToMessageCreateTime(t *testing.T) { raw := &event.RawEvent{ EventID: "ev_no_source_time", EventType: "im.message.receive_v1", Payload: json.RawMessage(`{"event":{"message":{"message_id":"om_1","create_time":"1776409468987"}}}`), Timestamp: time.Now(), } got, err := processImMessageReceive(context.Background(), nil, raw, nil) if err != nil { t.Fatalf("Process error: %v", err) } var out ImMessageReceiveOutput if err := json.Unmarshal(got, &out); err != nil { t.Fatalf("invalid output JSON: %v", err) } if out.Timestamp != "1776409468987" { t.Errorf("Timestamp = %q, want message create_time", out.Timestamp) } }🤖 Prompt for 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. In `@events/im/message_receive_test.go` around lines 236 - 254, Add a dedicated test for processImMessageReceive that constructs an event.RawEvent without SourceTime, supplies a message create_time in its payload, and asserts the decoded ImMessageReceiveOutput.Timestamp equals that create_time. Do not use runReceive, since it populates SourceTime via fillCanonicalFromHeader.Source: Coding guidelines
internal/event/consume/capability_gate_test.go (1)
86-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
t.TempDir()instead of manualos.MkdirTemp/os.RemoveAll.
startLegacyBusStubcreates a temp directory withos.MkdirTempand manually cleans it up withos.RemoveAllint.Cleanup.t.TempDir()does this automatically and is already the pattern used inpidfile_test.goin this same cohort.♻️ Proposed simplification
func startLegacyBusStub(t *testing.T, rawAck string) transport.IPC { t.Helper() - dir, err := os.MkdirTemp("", "capgate-*") - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { os.RemoveAll(dir) }) + dir := t.TempDir()🤖 Prompt for 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. In `@internal/event/consume/capability_gate_test.go` around lines 86 - 99, Update startLegacyBusStub to use t.TempDir() for temporary directory creation, removing the os.MkdirTemp error handling and manual os.RemoveAll cleanup while preserving the existing socket setup.internal/event/adapter/localbus/busdiscover/pidfile_test.go (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
internal/vfsfor fixture setup in thisinternal/test file.This file uses
os.ReadFile(Line 25),os.MkdirAll(Lines 94, 126), andos.WriteFile(Line 134) to set up test fixtures. A retrieved learning from a prior PR in this repository states that test files underinternal/should useinternal/vfsfor filesystem access, including fixture/setup helpers, instead ofos.*. Note this differs from an older, more general learning that recommendsos.*for fixture setup in*_test.gofiles broadly; the internal/-specific learning is newer and targets this exact path pattern.If the newer convention holds for this package, replace the
os.*calls with the correspondingvfs.*functions already imported inpidfile.go(vfs.ReadFile,vfs.WriteFile,vfs.MkdirAll).Also applies to: 94-96, 126-136
🤖 Prompt for 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. In `@internal/event/adapter/localbus/busdiscover/pidfile_test.go` at line 25, Replace the filesystem fixture calls in the pidfile tests—os.ReadFile, os.MkdirAll, and os.WriteFile—with the corresponding internal/vfs functions vfs.ReadFile, vfs.MkdirAll, and vfs.WriteFile, reusing the package’s existing VFS convention and preserving the current test behavior.Source: Learnings
🤖 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/approval/preconsume.go`:
- Line 16: Update approvalSubscriptionPreConsume so its eventType and
subscribePath parameters use distinct named string types, such as
approvalEventType and approvalSubscribePath, and adjust relevant callers or
conversions to pass those typed values. Preserve the existing callback behavior
while ensuring the compiler rejects accidental argument swaps.
In `@events/schema_instance_test.go`:
- Around line 96-100: Update the `flipped` assignment in the test to assert the
decoded value is a `map[string]any` and fail immediately on assertion failure,
matching the existing checked assertion near line 87. Reuse that
already-validated map for the `message_id` mutation instead of discarding the
assertion error and risking a nil-map panic.
In `@internal/event/adapter/lark/websocket/source.go`:
- Around line 4-6: Update the package comment above the package declaration in
the websocket source file to begin with “Package websocket” instead of “Package
source,” keeping the rest of the documentation unchanged.
In `@internal/event/adapter/localbus/protocol/canonical_fields_test.go`:
- Around line 48-51: Extend the decoded frame assertions in the test around the
existing EventID/EventType checks to compare frame.Payload with the original
event payload from ev. Keep the assertion direct and ensure payload loss or
mutation causes the test to fail.
In `@internal/event/adapter/localbus/protocol/codec_test.go`:
- Around line 65-72: Update TestEncodeAddsNewline to capture and assert the
error returned by Encode before inspecting the buffer; report the encoding
failure through the test and stop execution so the newline check cannot index an
empty buffer.
In `@internal/event/adapter/localbus/protocol/codec.go`:
- Around line 51-58: Update the nil-error branch of the ReadSlice handling to
check len(chunk) against MaxFrameBytes before returning the first chunk,
returning ErrFrameTooLarge when it exceeds the limit; retain the existing buf
accumulation checks for subsequent chunks. Add coverage using
bufio.NewReaderSize with a buffer larger than MaxFrameBytes to verify an
oversized first frame is rejected before Decode.
- Around line 17-105: Update the protocol boundary functions Encode,
EncodeWithDeadline, ReadFrame, and Decode to return the repository’s typed
errs.* errors instead of raw errors.New or fmt.Errorf values. Classify oversized
frames, deadline failures, malformed JSON, and downstream unmarshalling failures
with the appropriate errs.* type, preserving the underlying cause through
wrapping where applicable. Keep existing error context and behavior unchanged.
In `@internal/event/adapter/localbus/protocol/messages.go`:
- Around line 160-176: The NewEvent function currently formats ev.Timestamp
directly, allowing local timezone offsets in ObservedAt. Normalize the event
timestamp to UTC before formatting, using the event’s UTC-normalization
behavior, while preserving the empty value for zero timestamps.
In `@internal/event/adapter/localbus/transport/transport_test.go`:
- Line 10: Replace the os.Stat and os.Remove calls in the transport tests with
the corresponding internal/vfs filesystem APIs, and remove the now-unused os
import. Keep the existing test behavior unchanged while routing all filesystem
access through internal/vfs.
- Around line 91-92: Update the listener setup in the test around tr.Listen to
capture and check its error, failing the test immediately with the testing
helper if setup fails; only call ln.Close after a successful Listen.
In `@internal/event/adapter/localbus/transport/transport_unix.go`:
- Around line 26-34: Update unixTransport.Listen and unixTransport.Dial to wrap
every MkdirAll, net.Listen, and net.DialTimeout failure with the prescribed
typed transport error constructor and chain the original error via
WithCause(err) before returning. Preserve successful return behavior and use the
existing constructor conventions in the surrounding transport package.
In `@internal/event/application/consume/service.go`:
- Around line 78-81: Update both strategy lookup error paths in the service
method—where Strategies.get returns an error near the referenced locations—to
attach the original registry error using WithCause(err) on the constructed
internal error, preserving errors.Is and errors.As behavior without changing the
existing subtype or message.
- Around line 106-119: The Decide precondition loop must ensure every
PreconditionBlocked result has a non-nil d.blockErr: reuse the provided BlockErr
when present, otherwise synthesize a failed_precondition error. Update the
blocked-decision execution path so Execute does not run the stream when the
decision remains blocked without an error, while preserving existing status
precedence.
In `@internal/event/catalog/params.go`:
- Around line 40-54: Update the parameter validation logic around the map
iteration in the catalog parser to collect unknown parameter names, sort them
deterministically, and report the first sorted name while preserving the
existing no-valid-params and valid-params error messages. Ensure the validation
result no longer depends on Go map iteration order.
- Around line 17-32: Update ValidateParams to reject empty strings for required
parameters before SubscriptionScope is computed, including CLI values such as
whiteboard_id="". Enforce each parameter’s declared type: validate ParamEnum and
ParamMulti against p.Values, and parse ParamInt and ParamBool, returning the
existing validation error style for invalid values. Preserve default application
and optional-parameter behavior.
In `@internal/event/catalog/snapshot_test.go`:
- Around line 36-44: Update the test setup around validDef, compiledFixture, and
Resolve to use the returned definition’s def.Key instead of the hard-coded key,
and assert the Resolve success result before dereferencing entry. Before
accessing def.Schema.Custom.Raw[0], validate that the schema is custom and the
raw collection is non-empty, reporting a test failure rather than panicking when
validDef changes.
In `@internal/event/consume/loop.go`:
- Around line 286-306: Update restoreCanonicalEvent to surface a WARN diagnostic
through the same mechanism used by checkCanonicalConflict when a non-empty
evt.ObservedAt fails RFC3339Nano parsing, while preserving the zero timestamp
and no-warning behavior for an empty value. Propagate the required diagnostic or
logger context through its call site so malformed timestamps are reported
without dropping the event.
---
Nitpick comments:
In `@cmd/event/schema_test.go`:
- Around line 395-424: Move TestCompile_EmptySpecIsRejected and
TestCompile_InvalidBaseWithOverridesIsRejected from cmd/event/schema_test.go
into internal/event/catalog/compile_test.go, preserving their assertions and
catalog.Compile coverage. Keep cmd/event/schema_test.go focused on runSchema and
other command-layer behavior.
In `@cmd/event/suggestions_test.go`:
- Around line 100-113: Update the unknownEventKeyErr test to inspect
errs.ProblemOf(err), asserting its Category and Subtype equal the expected
validation metadata, including SubtypeInvalidArgument. Keep the existing
message-content assertions for the unknown key and suggestion.
In `@events/im/catalog_helper_test.go`:
- Around line 38-54: Consolidate the duplicated lookupCompiledDef helpers by
adding one generic helper to internal/event/testutil that accepts
[]event.KeyDefinition and a key, compiles with the existing StrategyRefs, and
resolves the definition. Update events/im/catalog_helper_test.go:38-54 to use it
with im.Keys(), events/minutes/catalog_helper_test.go:13-29 with minutes.Keys(),
and events/vc/catalog_helper_test.go:13-29 with vc.Keys(), removing each local
lookupCompiledDef implementation.
In `@events/im/message_receive_test.go`:
- Around line 236-254: Add a dedicated test for processImMessageReceive that
constructs an event.RawEvent without SourceTime, supplies a message create_time
in its payload, and asserts the decoded ImMessageReceiveOutput.Timestamp equals
that create_time. Do not use runReceive, since it populates SourceTime via
fillCanonicalFromHeader.
In `@events/vc/test_helpers_test.go`:
- Around line 20-37: Centralize the canonical-field synchronization implemented
by fillCanonicalFromHeader. Keep events/vc/test_helpers_test.go:20-37 as the
shared implementation or move it to internal/event/testutil for import; remove
the duplicate helper from events/minutes/minute_generated_test.go:330-352 and
update its callers to use the shared helper.
In `@internal/event/adapter/localbus/busdiscover/pidfile_test.go`:
- Line 25: Replace the filesystem fixture calls in the pidfile
tests—os.ReadFile, os.MkdirAll, and os.WriteFile—with the corresponding
internal/vfs functions vfs.ReadFile, vfs.MkdirAll, and vfs.WriteFile, reusing
the package’s existing VFS convention and preserving the current test behavior.
In `@internal/event/application/consume/strategy.go`:
- Around line 60-66: Update Registry.get for missing strategy references to use
the established errs typed constructor and subtype already used elsewhere in
internal/event, instead of fmt.Errorf. Preserve the existing
unregistered-reference message and return behavior while ensuring the error
retains its category and subtype metadata.
In `@internal/event/catalog/compile.go`:
- Around line 109-111: Update Compile’s problems-return path to use the
prescribed validation-error constructor from github.com/larksuite/cli/errs
around the joined problem list, allowing callers to classify the failure. Keep
the existing errors import and usage for renderSpec unchanged.
- Around line 171-186: Extend the parameter validation loop in the catalog
compiler to reject duplicate non-empty names using a seen-name set, and validate
enum defaults against the declared Values. For ParamMulti, first use the
existing type definition and runtime handling to determine whether Default is a
list; validate every default value against Values accordingly, preserving the
intended empty-default behavior. Add the slices import only if the chosen
membership check requires it, and emit compile-time failures through the
existing fail function.
In `@internal/event/catalog/snapshot.go`:
- Around line 118-147: Update the `Snapshot` type comment to accurately describe
pointer-returning accessors: `Resolve` and `Entries` may return pointers to
entries stored in the immutable snapshot, while `Entry` keeps its fields
unexported and accessors return copies. Remove the claim that accessors never
return pointers into snapshot state.
In `@internal/event/consume/capability_gate_test.go`:
- Around line 86-99: Update startLegacyBusStub to use t.TempDir() for temporary
directory creation, removing the os.MkdirTemp error handling and manual
os.RemoveAll cleanup while preserving the existing socket setup.
🪄 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: ea6f2b0e-30fc-4988-b6bd-7334a845c67d
📒 Files selected for processing (159)
cmd/build.gocmd/event/bus.gocmd/event/bus_test.gocmd/event/consume.gocmd/event/consume_dryrun_test.gocmd/event/event.gocmd/event/format_helpers_test.gocmd/event/golden_test.gocmd/event/list.gocmd/event/list_domain_test.gocmd/event/list_test.gocmd/event/preconditions_test.gocmd/event/preflight_test.gocmd/event/render/decision.gocmd/event/render/decision_test.gocmd/event/render/redaction_guard_test.gocmd/event/render_contract_test.gocmd/event/schema.gocmd/event/schema_test.gocmd/event/service_adapters.gocmd/event/status.gocmd/event/status_orphan_test.gocmd/event/stop.gocmd/event/stop_discover_test.gocmd/event/stop_integration_test.gocmd/event/suggestions.gocmd/event/suggestions_test.gocmd/event/testdata/golden/list_json.goldencmd/event/testdata/golden/list_text.goldencmd/event/testdata/golden/schema_board_whiteboard_json.goldencmd/event/testdata/golden/schema_board_whiteboard_text.goldencmd/event/testdata/golden/schema_card_action_trigger_json.goldencmd/event/testdata/golden/schema_card_action_trigger_text.goldencmd/event/testdata/golden/schema_im_chat_updated_json.goldencmd/event/testdata/golden/schema_im_chat_updated_text.goldencmd/event/testdata/golden/schema_im_message_receive_json.goldencmd/event/testdata/golden/schema_im_message_receive_text.goldencmd/event/wiring.goevents/all.goevents/application/menu.goevents/application/menu_test.goevents/approval/preconsume.goevents/approval/register.goevents/approval/register_test.goevents/arch_test.goevents/catalog_helper_test.goevents/compile_test.goevents/expected_keys_test.goevents/im/card_action.goevents/im/card_action_test.goevents/im/catalog_helper_test.goevents/im/message_receive.goevents/im/message_receive_test.goevents/internal/subscribeprep/subscribeprep.goevents/lint_test.goevents/minutes/catalog_helper_test.goevents/minutes/minute_generated.goevents/minutes/minute_generated_test.goevents/minutes/preconsume.goevents/minutes/register.goevents/output_baseline_test.goevents/schema_closure_test.goevents/schema_instance_test.goevents/task/register_test.goevents/testdata/output_baseline.jsonevents/vc/catalog_helper_test.goevents/vc/internal_helpers.goevents/vc/note_generated.goevents/vc/note_generated_test.goevents/vc/participant_meeting_ended.goevents/vc/participant_meeting_ended_test.goevents/vc/participant_meeting_joined.goevents/vc/participant_meeting_lifecycle_test.goevents/vc/participant_meeting_started.goevents/vc/preconsume.goevents/vc/recording_ended.goevents/vc/recording_started.goevents/vc/recording_test.goevents/vc/recording_transcript_generated.goevents/vc/register.goevents/vc/test_helpers_test.goevents/whiteboard/preconsume.goevents/whiteboard/register.goevents/whiteboard/subscription_scope_test.gointernal/event/adapter/lark/websocket/feishu.gointernal/event/adapter/lark/websocket/feishu_ingress_test.gointernal/event/adapter/lark/websocket/feishu_log_test.gointernal/event/adapter/lark/websocket/feishu_test.gointernal/event/adapter/lark/websocket/sdk_log_patterns.gointernal/event/adapter/lark/websocket/sdk_log_patterns_test.gointernal/event/adapter/lark/websocket/source.gointernal/event/adapter/lark/websocket/source_test.gointernal/event/adapter/lark/websocket/state_pinning_test.gointernal/event/adapter/localbus/busctl/busctl.gointernal/event/adapter/localbus/busdiscover/busdiscover.gointernal/event/adapter/localbus/busdiscover/pidfile.gointernal/event/adapter/localbus/busdiscover/pidfile_test.gointernal/event/adapter/localbus/protocol/canonical_fields_test.gointernal/event/adapter/localbus/protocol/codec.gointernal/event/adapter/localbus/protocol/codec_test.gointernal/event/adapter/localbus/protocol/messages.gointernal/event/adapter/localbus/protocol/messages_test.gointernal/event/adapter/localbus/transport/transport.gointernal/event/adapter/localbus/transport/transport_test.gointernal/event/adapter/localbus/transport/transport_unix.gointernal/event/adapter/localbus/transport/transport_windows.gointernal/event/application/consume/decision.gointernal/event/application/consume/service.gointernal/event/application/consume/service_test.gointernal/event/application/consume/strategy.gointernal/event/arch_layering_test.gointernal/event/bus/bus.gointernal/event/bus/bus_shutdown_test.gointernal/event/bus/conn.gointernal/event/bus/conn_test.gointernal/event/bus/handle_hello_test.gointernal/event/bus/hub.gointernal/event/bus/hub_observability_test.gointernal/event/bus/hub_test.gointernal/event/bus/source_port.gointernal/event/catalog/canonicalize.gointernal/event/catalog/compile.gointernal/event/catalog/compile_test.gointernal/event/catalog/definition.gointernal/event/catalog/params.gointernal/event/catalog/scope.gointernal/event/catalog/snapshot.gointernal/event/catalog/snapshot_test.gointernal/event/catalog/strategy.gointernal/event/consume/canonical_conflict.gointernal/event/consume/canonical_conflict_test.gointernal/event/consume/capability_gate_test.gointernal/event/consume/consume.gointernal/event/consume/consume_test.gointernal/event/consume/diagnostics_redaction_test.gointernal/event/consume/fingerprint.gointernal/event/consume/fingerprint_scope_test.gointernal/event/consume/handshake.gointernal/event/consume/loop.gointernal/event/consume/loop_seq_test.gointernal/event/consume/loop_test.gointernal/event/consume/reject_test.gointernal/event/consume/shutdown.gointernal/event/consume/shutdown_test.gointernal/event/consume/startup.gointernal/event/consume/startup_guard_test.gointernal/event/consume/startup_probe_test.gointernal/event/integration_test.gointernal/event/model/event.gointernal/event/preconsume_contract_test.gointernal/event/processing/result.gointernal/event/registry.gointernal/event/registry_test.gointernal/event/source/source.gointernal/event/testutil/testutil.gointernal/event/types.golint/domaincontract/unapproved_test.goskills/lark-event/SKILL.mdskills/lark-event/references/lark-event-vc.md
💤 Files with no reviewable changes (6)
- events/minutes/preconsume.go
- internal/event/registry.go
- internal/event/source/source.go
- events/vc/preconsume.go
- internal/event/registry_test.go
- cmd/build.go
| } | ||
|
|
||
| func approvalSubscriptionPreConsume(cfg approvalSubscriptionConfig) func(context.Context, event.APIClient, map[string]string) (func() error, error) { | ||
| func approvalSubscriptionPreConsume(eventType, subscribePath string) func(context.Context, event.APIClient, map[string]string) (func() error, error) { |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Two adjacent string parameters can be swapped silently.
approvalSubscriptionPreConsume(eventType, subscribePath string) takes two string parameters with different semantics. The compiler does not stop a caller from passing them in the wrong order. The previous wrapper types prevented this class of mistake at compile time.
Use distinct named string types (for example type approvalEventType string and type approvalSubscribePath string) for these two parameters, so the compiler catches an accidental swap.
As per coding guidelines, "prefer distinct types when same-typed values could be silently swapped."
♻️ Proposed fix using distinct named types
-func approvalSubscriptionPreConsume(eventType, subscribePath string) func(context.Context, event.APIClient, map[string]string) (func() error, error) {
+type approvalEventType string
+type approvalSubscribePath string
+
+func approvalSubscriptionPreConsume(eventType approvalEventType, subscribePath approvalSubscribePath) func(context.Context, event.APIClient, map[string]string) (func() error, error) {
return func(ctx context.Context, rt event.APIClient, params map[string]string) (func() error, error) {
if rt == nil {
return nil, errs.NewInternalError(errs.SubtypeUnknown,
"runtime API client is required for pre-consume subscription")
}
- subscriptionTypes, err := approvalSubscriptionTypes(eventType, params)
+ subscriptionTypes, err := approvalSubscriptionTypes(string(eventType), params)
if err != nil {
return nil, err
}
registered := make([]string, 0, len(subscriptionTypes))
for _, subscriptionType := range subscriptionTypes {
body := map[string]string{"subscription_type": subscriptionType}
- if _, err := rt.CallAPI(ctx, "POST", subscribePath, body); err != nil {
- return nil, approvalSubscriptionRegistrationError(eventType, registered, subscriptionType, err)
+ if _, err := rt.CallAPI(ctx, "POST", string(subscribePath), body); err != nil {
+ return nil, approvalSubscriptionRegistrationError(string(eventType), registered, subscriptionType, err)
}
registered = append(registered, subscriptionType)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func approvalSubscriptionPreConsume(eventType, subscribePath string) func(context.Context, event.APIClient, map[string]string) (func() error, error) { | |
| type approvalEventType string | |
| type approvalSubscribePath string | |
| func approvalSubscriptionPreConsume(eventType approvalEventType, subscribePath approvalSubscribePath) func(context.Context, event.APIClient, map[string]string) (func() error, error) { | |
| return func(ctx context.Context, rt event.APIClient, params map[string]string) (func() error, error) { | |
| if rt == nil { | |
| return nil, errs.NewInternalError(errs.SubtypeUnknown, | |
| "runtime API client is required for pre-consume subscription") | |
| } | |
| subscriptionTypes, err := approvalSubscriptionTypes(string(eventType), params) | |
| if err != nil { | |
| return nil, err | |
| } | |
| registered := make([]string, 0, len(subscriptionTypes)) | |
| for _, subscriptionType := range subscriptionTypes { | |
| body := map[string]string{"subscription_type": subscriptionType} | |
| if _, err := rt.CallAPI(ctx, "POST", string(subscribePath), body); err != nil { | |
| return nil, approvalSubscriptionRegistrationError(string(eventType), registered, subscriptionType, err) | |
| } | |
| registered = append(registered, subscriptionType) | |
| } |
🤖 Prompt for 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.
In `@events/approval/preconsume.go` at line 16, Update
approvalSubscriptionPreConsume so its eventType and subscribePath parameters use
distinct named string types, such as approvalEventType and
approvalSubscribePath, and adjust relevant callers or conversions to pass those
typed values. Preserve the existing callback behavior while ensuring the
compiler rejects accidental argument swaps.
Source: Coding guidelines
| func ValidateParams(def *KeyDefinition, params map[string]string) error { | ||
| for _, p := range def.Params { | ||
| if _, ok := params[p.Name]; !ok && p.Default != "" { | ||
| params[p.Name] = p.Default | ||
| } | ||
| } | ||
| for _, p := range def.Params { | ||
| if p.Required { | ||
| if _, ok := params[p.Name]; !ok { | ||
| return errs.NewValidationError(errs.SubtypeInvalidArgument, | ||
| "required param %q missing for EventKey %s", p.Name, def.Key). | ||
| WithParam("--param"). | ||
| WithHint("pass it as --param %s=<value>; run `lark-cli event schema %s` for details", p.Name, def.Key) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether any other layer already validates param values against the declared type or Values.
rg -nP --type=go -C4 'ParamEnum|ParamMulti|ParamInt|ParamBool' internal/event cmd/event | rg -v '_test\.go'Repository: larksuite/cli
Length of output: 2233
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)internal/event/catalog/params\.go$|(^|/)internal/event/catalog/definition\.go$|(^|/)internal/event/catalog/compile\.go$|internal/event/|cmd/event/' | head -200
echo
echo "== params.go =="
cat -n internal/event/catalog/params.go
echo
echo "== definition.go =="
cat -n internal/event/catalog/definition.go
echo
echo "== compile.go outline and relevant =="
ast-grep outline internal/event/catalog/compile.go || true
sed -n '1,240p' internal/event/catalog/compile.go | cat -n
echo
echo "== usages of ValidateParams =="
rg -n --type=go -C3 'ValidateParams|params\[(p\.Name)|p\.Type|ParamEnum|ParamMulti' internal/event cmd/event | rg -v '_test\.go' | head -300Repository: larksuite/cli
Length of output: 31453
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== consume.go validation callers and options =="
sed -n '1,120p' internal/event/consume/consume.go | cat -n
sed -n '120,270p' internal/event/consume/consume.go | cat -n
echo
echo "== params parsing in app and consumers =="
rg -n --type=go -C4 'param|--param|params\s*:' cmd internal -g '*.go' | rg -v '_test\.go' | head -400
echo
echo "== scope.go =="
cat -n internal/event/catalog/scope.go
echo
echo "== service options path =="
cat -n internal/event/application/consume/service.go | sed -n '1,120p'
echo
echo "== validation tests around params =="
sed -n '1,220p' internal/event/consume/validate_params_test.go | cat -nRepository: larksuite/cli
Length of output: 34798
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== consume.go full parseParams area =="
sed -n '180,230p' cmd/event/consume.go | cat -n
sed -n '432,446p' cmd/event/consume.go | cat -n
echo
echo "== application/consume/service.go =="
cat -n internal/event/application/consume/service.go | sed -n '1,150p'
rg -n --type=go -C3 'ValidateParams\(' internal/event/application/consume/service.go cmd/event/consume.go internal/event/consume/consume.go internal/event/catalog/snapshot.go internal/event/catalog/scope.go
echo
echo "== scope.go full =="
cat -n internal/event/catalog/scope.go
echo
echo "== validate_params_test.go =="
wc -l internal/event/consume/validate_params_test.go
cat -n internal/event/consume/validate_params_test.go | sed -n '1,240p'
echo
echo "== param parse behavior probe =="
python3 - <<'PY'
for kv in ["whiteboard_id=", "whiteboard_id", "a=b=c"]:
if "=" in kv:
k, v = kv.split("=", 1)
print(f"{kv!r} -> key={k!r}, value={v!r}, value_empty={v==''}")
else:
print(f"{kv!r} -> parse_failed")
PYRepository: larksuite/cli
Length of output: 13912
Validate param values, not only their presence.
ValidateParams is used by svc.Decide, and the resulting params feed catalog.SubscriptionScope. Reject empty values for required params before computing the subscription scope, and enforce the declared type so ParamEnum / ParamMulti are checked against p.Values and ParamInt / ParamBool fail on non-parseable strings. The CLI currently passes --param whiteboard_id= as {"whiteboard_id": ""}, so the empty string currently creates a distinct scope.
🤖 Prompt for 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.
In `@internal/event/catalog/params.go` around lines 17 - 32, Update ValidateParams
to reject empty strings for required parameters before SubscriptionScope is
computed, including CLI values such as whiteboard_id="". Enforce each
parameter’s declared type: validate ParamEnum and ParamMulti against p.Values,
and parse ParamInt and ParamBool, returning the existing validation error style
for invalid values. Preserve default application and optional-parameter
behavior.
Source: Coding guidelines
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 17
🧹 Nitpick comments (11)
internal/event/catalog/snapshot.go (1)
118-147: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAlign the
Snapshotdoc comment withResolveandEntries.Lines 118-119 state that accessors never return pointers into the snapshot's own state.
ResolveandEntriesreturn*Entryvalues that point intos.entries. The immutability property still holds, because everyEntryfield is unexported and everyEntryaccessor copies. The comment as written can lead a future maintainer inside packagecatalogto mutate through the returned pointer. State the actual rule instead.♻️ Proposed wording
-// Snapshot is the compiled, immutable catalog. Accessors return values or -// fresh copies — never pointers into the snapshot's own state. +// Snapshot is the compiled, immutable catalog. Resolve and Entries hand out +// *Entry handles into the snapshot; every Entry field is unexported and every +// Entry accessor returns a value or a fresh copy, so no caller outside this +// package can mutate compiled state. Code inside this package must not write +// through a returned *Entry.🤖 Prompt for 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. In `@internal/event/catalog/snapshot.go` around lines 118 - 147, Update the `Snapshot` type comment to accurately describe pointer-returning accessors: `Resolve` and `Entries` may return pointers to entries stored in the immutable snapshot, while `Entry` keeps its fields unexported and accessors return copies. Remove the claim that accessors never return pointers into snapshot state.events/im/catalog_helper_test.go (1)
38-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate the duplicated
lookupCompiledDeftest helper. Three domain packages define the identical helper: compile the package's ownKeys()withcatalog.StrategyRefs{catalog.StrategyNone, catalog.StrategyLegacyPreConsume}, then resolve one key.internal/event/testutil/testutil.goalready exists in this PR as a shared test-support package and is the natural place to hold one generic version of this helper.
events/im/catalog_helper_test.go#L38-L54: replace thislookupCompiledDefwith a call to a shared helper ininternal/event/testutilthat acceptsdefs []event.KeyDefinitionandkey string, so this file only suppliesim.Keys().events/minutes/catalog_helper_test.go#L13-L29: replace thislookupCompiledDefwith the same shared helper, supplyingminutes.Keys().events/vc/catalog_helper_test.go#L13-L29: replace thislookupCompiledDefwith the same shared helper, supplyingvc.Keys().🤖 Prompt for 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. In `@events/im/catalog_helper_test.go` around lines 38 - 54, Consolidate the duplicated lookupCompiledDef helpers by adding one generic helper to internal/event/testutil that accepts []event.KeyDefinition and a key, compiles with the existing StrategyRefs, and resolves the definition. Update events/im/catalog_helper_test.go:38-54 to use it with im.Keys(), events/minutes/catalog_helper_test.go:13-29 with minutes.Keys(), and events/vc/catalog_helper_test.go:13-29 with vc.Keys(), removing each local lookupCompiledDef implementation.cmd/event/schema_test.go (1)
395-424: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove catalog-compile tests to the catalog package.
TestCompile_EmptySpecIsRejectedandTestCompile_InvalidBaseWithOverridesIsRejectedexerciseinternal/event/catalog.Compiledirectly. They do not callrunSchemaor exercise anycmd/eventbehavior.internal/event/catalog/compile_test.goalready exists in this PR and is the natural home for catalog-compilation unit tests.Move both tests to
internal/event/catalog/compile_test.goto keepcmd/event/schema_test.gofocused on command-layer behavior.🤖 Prompt for 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. In `@cmd/event/schema_test.go` around lines 395 - 424, Move TestCompile_EmptySpecIsRejected and TestCompile_InvalidBaseWithOverridesIsRejected from cmd/event/schema_test.go into internal/event/catalog/compile_test.go, preserving their assertions and catalog.Compile coverage. Keep cmd/event/schema_test.go focused on runSchema and other command-layer behavior.events/vc/test_helpers_test.go (1)
20-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated
fillCanonicalFromHeaderhelper acrossevents/vcandevents/minutestest packages. Both copies parse the identical envelope header shape and copy the identical three fields onto*event.RawEvent; the shared root cause is the lack of a common test helper for this canonical-field synchronization.
events/vc/test_helpers_test.go#L20-L37: keep this as the canonical implementation, or move it intointernal/event/testutilso other event-domain test packages can import it directly.events/minutes/minute_generated_test.go#L330-L352: remove this copy and import the shared helper instead of redefining it.🤖 Prompt for 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. In `@events/vc/test_helpers_test.go` around lines 20 - 37, Centralize the canonical-field synchronization implemented by fillCanonicalFromHeader. Keep events/vc/test_helpers_test.go:20-37 as the shared implementation or move it to internal/event/testutil for import; remove the duplicate helper from events/minutes/minute_generated_test.go:330-352 and update its callers to use the shared helper.internal/event/catalog/compile.go (2)
109-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a typed validation error from
Compile.
Compilereports every declaration problem througherrors.New. Callers therefore receive an untyped error, and the command layer cannot classify it.cmd/eventcompiles the catalog at startup, so this error reaches a user-facing exit path.Wrap the joined problem list in the prescribed typed constructor for validation failures.
🛡️ Proposed fix
if len(problems) > 0 { - return nil, errors.New("event catalog rejected:\n " + strings.Join(problems, "\n ")) + return nil, errs.NewValidationError(errs.SubtypeInvalidArgument, + "event catalog rejected:\n %s", strings.Join(problems, "\n ")) }
errorsstays in use forrenderSpec.Confirm that
internal/event/catalogmay importgithub.com/larksuite/cli/errs.internal/event/catalog/params.goalready imports it, andTestArchKernelPuritydoes not ban it, so the layering gate stays green.As per coding guidelines: "Use the prescribed typed error constructors for validation, failed preconditions, API failures, network failures, file I/O failures, and unknown lower-layer errors."
🤖 Prompt for 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. In `@internal/event/catalog/compile.go` around lines 109 - 111, Update Compile’s problems-return path to use the prescribed validation-error constructor from github.com/larksuite/cli/errs around the joined problem list, allowing callers to classify the failure. Keep the existing errors import and usage for renderSpec unchanged.Source: Coding guidelines
171-186: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winReject duplicate param names and out-of-set defaults at compile time.
The param loop validates the type and the
Valueslist, but it does not check two contracts that the runtime depends on:
- Duplicate
Nameentries.ValidateParamsininternal/event/catalog/params.gobuildsknownandvalidNamesfrom the same slice, so a duplicated name silently produces a duplicated hint list and an ambiguous default.- A
Defaultthat is not present inValuesforParamEnumandParamMulti.ValidateParamsinjectsDefaultbefore the required check, so an invalid default becomes an accepted parameter value.Whole-catalog validation is the right place for both checks.
♻️ Proposed addition
+ seen := make(map[string]bool, len(def.Params)) for _, p := range def.Params { + if seen[p.Name] { + fail("EventKey %s: duplicate param %q", def.Key, p.Name) + } + seen[p.Name] = true switch p.Type { case "", ParamString, ParamBool, ParamInt: case ParamEnum, ParamMulti: if len(p.Values) == 0 { fail("EventKey %s: param %q type %q requires Values", def.Key, p.Name, p.Type) } for _, v := range p.Values { if v.Desc == "" { fail("EventKey %s: param %q value %q requires non-empty Desc", def.Key, p.Name, v.Value) } } + if p.Default != "" && !slices.ContainsFunc(p.Values, func(v ParamValue) bool { return v.Value == p.Default }) { + fail("EventKey %s: param %q Default %q is not one of its Values", def.Key, p.Name, p.Default) + } default: fail("EventKey %s: param %q has unknown type %q", def.Key, p.Name, p.Type) } }Add
"slices"to the imports.For
ParamMulti, confirm whetherDefaultmay hold a multi-value list before applying the membership check.🤖 Prompt for 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. In `@internal/event/catalog/compile.go` around lines 171 - 186, Extend the parameter validation loop in the catalog compiler to reject duplicate non-empty names using a seen-name set, and validate enum defaults against the declared Values. For ParamMulti, first use the existing type definition and runtime handling to determine whether Default is a list; validate every default value against Values accordingly, preserving the intended empty-default behavior. Add the slices import only if the chosen membership check requires it, and emit compile-time failures through the existing fail function.cmd/event/suggestions_test.go (1)
100-113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert typed metadata for the unknown-key error.
unknownEventKeyErrreturns a validation error withSubtypeInvalidArgument, so the test should asserterrs.ProblemOf(err).Categoryand.Subtypeinstead of only checkingerr.Error()substrings.♻️ Proposed addition
err := unknownEventKeyErr(compileCatalog(), "im.message.recieve_v1") if err == nil { t.Fatal("expected error") } + if p, ok := errs.ProblemOf(err); !ok { + t.Fatal("unknownEventKeyErr must return a typed error") + } else if p.Category != errs.CategoryValidation || p.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("category/subtype = %q/%q", p.Category, p.Subtype) + } msg := err.Error()🤖 Prompt for 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. In `@cmd/event/suggestions_test.go` around lines 100 - 113, Update the unknownEventKeyErr test to inspect errs.ProblemOf(err), asserting its Category and Subtype equal the expected validation metadata, including SubtypeInvalidArgument. Keep the existing message-content assertions for the unknown key and suggestion.Source: Coding guidelines
internal/event/application/consume/strategy.go (1)
60-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn a typed error for an unregistered strategy.
getreturns a barefmt.Errorf. This error reaches the command layer, where it loses category and subtype metadata. An unregistered strategy reference is an internal invariant failure, so use the prescribed typed constructor.As per coding guidelines: "Use the prescribed typed error constructors for validation, failed preconditions, API failures, network failures, file I/O failures, and unknown lower-layer errors".
♻️ Proposed change
func (r *Registry) get(ref catalog.StrategyRef) (PreparationStrategy, error) { s, ok := r.strategies[ref] if !ok { - return nil, fmt.Errorf("preparation strategy %q is not registered", ref) + return nil, errs.NewInternalError(errs.SubtypeFailedPrecondition, + fmt.Sprintf("preparation strategy %q is not registered", ref)) } return s, nil }Match the exact
errsconstructor and subtype used elsewhere ininternal/event.🤖 Prompt for 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. In `@internal/event/application/consume/strategy.go` around lines 60 - 66, Update Registry.get for missing strategy references to use the established errs typed constructor and subtype already used elsewhere in internal/event, instead of fmt.Errorf. Preserve the existing unregistered-reference message and return behavior while ensuring the error retains its category and subtype metadata.Source: Coding guidelines
events/im/message_receive_test.go (1)
236-254: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the empty
SourceTimefallback.
runReceivealways callsfillCanonicalFromHeader, soraw.SourceTimeis always populated. The new fallback inprocessImMessageReceive(timestamp = msg.CreateTimewhenraw.SourceTimeis empty) is therefore never executed by these tests. Removing the fallback would not fail the suite.Add one case that builds a
RawEventwithoutSourceTimeand assertstimestampequals the messagecreate_time.As per coding guidelines: "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
💚 Proposed test
func TestProcessImMessageReceive_TimestampFallsBackToMessageCreateTime(t *testing.T) { raw := &event.RawEvent{ EventID: "ev_no_source_time", EventType: "im.message.receive_v1", Payload: json.RawMessage(`{"event":{"message":{"message_id":"om_1","create_time":"1776409468987"}}}`), Timestamp: time.Now(), } got, err := processImMessageReceive(context.Background(), nil, raw, nil) if err != nil { t.Fatalf("Process error: %v", err) } var out ImMessageReceiveOutput if err := json.Unmarshal(got, &out); err != nil { t.Fatalf("invalid output JSON: %v", err) } if out.Timestamp != "1776409468987" { t.Errorf("Timestamp = %q, want message create_time", out.Timestamp) } }🤖 Prompt for 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. In `@events/im/message_receive_test.go` around lines 236 - 254, Add a dedicated test for processImMessageReceive that constructs an event.RawEvent without SourceTime, supplies a message create_time in its payload, and asserts the decoded ImMessageReceiveOutput.Timestamp equals that create_time. Do not use runReceive, since it populates SourceTime via fillCanonicalFromHeader.Source: Coding guidelines
internal/event/consume/capability_gate_test.go (1)
86-99: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
t.TempDir()instead of manualos.MkdirTemp/os.RemoveAll.
startLegacyBusStubcreates a temp directory withos.MkdirTempand manually cleans it up withos.RemoveAllint.Cleanup.t.TempDir()does this automatically and is already the pattern used inpidfile_test.goin this same cohort.♻️ Proposed simplification
func startLegacyBusStub(t *testing.T, rawAck string) transport.IPC { t.Helper() - dir, err := os.MkdirTemp("", "capgate-*") - if err != nil { - t.Fatal(err) - } - t.Cleanup(func() { os.RemoveAll(dir) }) + dir := t.TempDir()🤖 Prompt for 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. In `@internal/event/consume/capability_gate_test.go` around lines 86 - 99, Update startLegacyBusStub to use t.TempDir() for temporary directory creation, removing the os.MkdirTemp error handling and manual os.RemoveAll cleanup while preserving the existing socket setup.internal/event/adapter/localbus/busdiscover/pidfile_test.go (1)
25-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider
internal/vfsfor fixture setup in thisinternal/test file.This file uses
os.ReadFile(Line 25),os.MkdirAll(Lines 94, 126), andos.WriteFile(Line 134) to set up test fixtures. A retrieved learning from a prior PR in this repository states that test files underinternal/should useinternal/vfsfor filesystem access, including fixture/setup helpers, instead ofos.*. Note this differs from an older, more general learning that recommendsos.*for fixture setup in*_test.gofiles broadly; the internal/-specific learning is newer and targets this exact path pattern.If the newer convention holds for this package, replace the
os.*calls with the correspondingvfs.*functions already imported inpidfile.go(vfs.ReadFile,vfs.WriteFile,vfs.MkdirAll).Also applies to: 94-96, 126-136
🤖 Prompt for 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. In `@internal/event/adapter/localbus/busdiscover/pidfile_test.go` at line 25, Replace the filesystem fixture calls in the pidfile tests—os.ReadFile, os.MkdirAll, and os.WriteFile—with the corresponding internal/vfs functions vfs.ReadFile, vfs.MkdirAll, and vfs.WriteFile, reusing the package’s existing VFS convention and preserving the current test behavior.Source: Learnings
🤖 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/approval/preconsume.go`:
- Line 16: Update approvalSubscriptionPreConsume so its eventType and
subscribePath parameters use distinct named string types, such as
approvalEventType and approvalSubscribePath, and adjust relevant callers or
conversions to pass those typed values. Preserve the existing callback behavior
while ensuring the compiler rejects accidental argument swaps.
In `@events/schema_instance_test.go`:
- Around line 96-100: Update the `flipped` assignment in the test to assert the
decoded value is a `map[string]any` and fail immediately on assertion failure,
matching the existing checked assertion near line 87. Reuse that
already-validated map for the `message_id` mutation instead of discarding the
assertion error and risking a nil-map panic.
In `@internal/event/adapter/lark/websocket/source.go`:
- Around line 4-6: Update the package comment above the package declaration in
the websocket source file to begin with “Package websocket” instead of “Package
source,” keeping the rest of the documentation unchanged.
In `@internal/event/adapter/localbus/protocol/canonical_fields_test.go`:
- Around line 48-51: Extend the decoded frame assertions in the test around the
existing EventID/EventType checks to compare frame.Payload with the original
event payload from ev. Keep the assertion direct and ensure payload loss or
mutation causes the test to fail.
In `@internal/event/adapter/localbus/protocol/codec_test.go`:
- Around line 65-72: Update TestEncodeAddsNewline to capture and assert the
error returned by Encode before inspecting the buffer; report the encoding
failure through the test and stop execution so the newline check cannot index an
empty buffer.
In `@internal/event/adapter/localbus/protocol/codec.go`:
- Around line 51-58: Update the nil-error branch of the ReadSlice handling to
check len(chunk) against MaxFrameBytes before returning the first chunk,
returning ErrFrameTooLarge when it exceeds the limit; retain the existing buf
accumulation checks for subsequent chunks. Add coverage using
bufio.NewReaderSize with a buffer larger than MaxFrameBytes to verify an
oversized first frame is rejected before Decode.
- Around line 17-105: Update the protocol boundary functions Encode,
EncodeWithDeadline, ReadFrame, and Decode to return the repository’s typed
errs.* errors instead of raw errors.New or fmt.Errorf values. Classify oversized
frames, deadline failures, malformed JSON, and downstream unmarshalling failures
with the appropriate errs.* type, preserving the underlying cause through
wrapping where applicable. Keep existing error context and behavior unchanged.
In `@internal/event/adapter/localbus/protocol/messages.go`:
- Around line 160-176: The NewEvent function currently formats ev.Timestamp
directly, allowing local timezone offsets in ObservedAt. Normalize the event
timestamp to UTC before formatting, using the event’s UTC-normalization
behavior, while preserving the empty value for zero timestamps.
In `@internal/event/adapter/localbus/transport/transport_test.go`:
- Line 10: Replace the os.Stat and os.Remove calls in the transport tests with
the corresponding internal/vfs filesystem APIs, and remove the now-unused os
import. Keep the existing test behavior unchanged while routing all filesystem
access through internal/vfs.
- Around line 91-92: Update the listener setup in the test around tr.Listen to
capture and check its error, failing the test immediately with the testing
helper if setup fails; only call ln.Close after a successful Listen.
In `@internal/event/adapter/localbus/transport/transport_unix.go`:
- Around line 26-34: Update unixTransport.Listen and unixTransport.Dial to wrap
every MkdirAll, net.Listen, and net.DialTimeout failure with the prescribed
typed transport error constructor and chain the original error via
WithCause(err) before returning. Preserve successful return behavior and use the
existing constructor conventions in the surrounding transport package.
In `@internal/event/application/consume/service.go`:
- Around line 78-81: Update both strategy lookup error paths in the service
method—where Strategies.get returns an error near the referenced locations—to
attach the original registry error using WithCause(err) on the constructed
internal error, preserving errors.Is and errors.As behavior without changing the
existing subtype or message.
- Around line 106-119: The Decide precondition loop must ensure every
PreconditionBlocked result has a non-nil d.blockErr: reuse the provided BlockErr
when present, otherwise synthesize a failed_precondition error. Update the
blocked-decision execution path so Execute does not run the stream when the
decision remains blocked without an error, while preserving existing status
precedence.
In `@internal/event/catalog/params.go`:
- Around line 40-54: Update the parameter validation logic around the map
iteration in the catalog parser to collect unknown parameter names, sort them
deterministically, and report the first sorted name while preserving the
existing no-valid-params and valid-params error messages. Ensure the validation
result no longer depends on Go map iteration order.
- Around line 17-32: Update ValidateParams to reject empty strings for required
parameters before SubscriptionScope is computed, including CLI values such as
whiteboard_id="". Enforce each parameter’s declared type: validate ParamEnum and
ParamMulti against p.Values, and parse ParamInt and ParamBool, returning the
existing validation error style for invalid values. Preserve default application
and optional-parameter behavior.
In `@internal/event/catalog/snapshot_test.go`:
- Around line 36-44: Update the test setup around validDef, compiledFixture, and
Resolve to use the returned definition’s def.Key instead of the hard-coded key,
and assert the Resolve success result before dereferencing entry. Before
accessing def.Schema.Custom.Raw[0], validate that the schema is custom and the
raw collection is non-empty, reporting a test failure rather than panicking when
validDef changes.
In `@internal/event/consume/loop.go`:
- Around line 286-306: Update restoreCanonicalEvent to surface a WARN diagnostic
through the same mechanism used by checkCanonicalConflict when a non-empty
evt.ObservedAt fails RFC3339Nano parsing, while preserving the zero timestamp
and no-warning behavior for an empty value. Propagate the required diagnostic or
logger context through its call site so malformed timestamps are reported
without dropping the event.
---
Nitpick comments:
In `@cmd/event/schema_test.go`:
- Around line 395-424: Move TestCompile_EmptySpecIsRejected and
TestCompile_InvalidBaseWithOverridesIsRejected from cmd/event/schema_test.go
into internal/event/catalog/compile_test.go, preserving their assertions and
catalog.Compile coverage. Keep cmd/event/schema_test.go focused on runSchema and
other command-layer behavior.
In `@cmd/event/suggestions_test.go`:
- Around line 100-113: Update the unknownEventKeyErr test to inspect
errs.ProblemOf(err), asserting its Category and Subtype equal the expected
validation metadata, including SubtypeInvalidArgument. Keep the existing
message-content assertions for the unknown key and suggestion.
In `@events/im/catalog_helper_test.go`:
- Around line 38-54: Consolidate the duplicated lookupCompiledDef helpers by
adding one generic helper to internal/event/testutil that accepts
[]event.KeyDefinition and a key, compiles with the existing StrategyRefs, and
resolves the definition. Update events/im/catalog_helper_test.go:38-54 to use it
with im.Keys(), events/minutes/catalog_helper_test.go:13-29 with minutes.Keys(),
and events/vc/catalog_helper_test.go:13-29 with vc.Keys(), removing each local
lookupCompiledDef implementation.
In `@events/im/message_receive_test.go`:
- Around line 236-254: Add a dedicated test for processImMessageReceive that
constructs an event.RawEvent without SourceTime, supplies a message create_time
in its payload, and asserts the decoded ImMessageReceiveOutput.Timestamp equals
that create_time. Do not use runReceive, since it populates SourceTime via
fillCanonicalFromHeader.
In `@events/vc/test_helpers_test.go`:
- Around line 20-37: Centralize the canonical-field synchronization implemented
by fillCanonicalFromHeader. Keep events/vc/test_helpers_test.go:20-37 as the
shared implementation or move it to internal/event/testutil for import; remove
the duplicate helper from events/minutes/minute_generated_test.go:330-352 and
update its callers to use the shared helper.
In `@internal/event/adapter/localbus/busdiscover/pidfile_test.go`:
- Line 25: Replace the filesystem fixture calls in the pidfile
tests—os.ReadFile, os.MkdirAll, and os.WriteFile—with the corresponding
internal/vfs functions vfs.ReadFile, vfs.MkdirAll, and vfs.WriteFile, reusing
the package’s existing VFS convention and preserving the current test behavior.
In `@internal/event/application/consume/strategy.go`:
- Around line 60-66: Update Registry.get for missing strategy references to use
the established errs typed constructor and subtype already used elsewhere in
internal/event, instead of fmt.Errorf. Preserve the existing
unregistered-reference message and return behavior while ensuring the error
retains its category and subtype metadata.
In `@internal/event/catalog/compile.go`:
- Around line 109-111: Update Compile’s problems-return path to use the
prescribed validation-error constructor from github.com/larksuite/cli/errs
around the joined problem list, allowing callers to classify the failure. Keep
the existing errors import and usage for renderSpec unchanged.
- Around line 171-186: Extend the parameter validation loop in the catalog
compiler to reject duplicate non-empty names using a seen-name set, and validate
enum defaults against the declared Values. For ParamMulti, first use the
existing type definition and runtime handling to determine whether Default is a
list; validate every default value against Values accordingly, preserving the
intended empty-default behavior. Add the slices import only if the chosen
membership check requires it, and emit compile-time failures through the
existing fail function.
In `@internal/event/catalog/snapshot.go`:
- Around line 118-147: Update the `Snapshot` type comment to accurately describe
pointer-returning accessors: `Resolve` and `Entries` may return pointers to
entries stored in the immutable snapshot, while `Entry` keeps its fields
unexported and accessors return copies. Remove the claim that accessors never
return pointers into snapshot state.
In `@internal/event/consume/capability_gate_test.go`:
- Around line 86-99: Update startLegacyBusStub to use t.TempDir() for temporary
directory creation, removing the os.MkdirTemp error handling and manual
os.RemoveAll cleanup while preserving the existing socket setup.
🪄 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: ea6f2b0e-30fc-4988-b6bd-7334a845c67d
📒 Files selected for processing (159)
cmd/build.gocmd/event/bus.gocmd/event/bus_test.gocmd/event/consume.gocmd/event/consume_dryrun_test.gocmd/event/event.gocmd/event/format_helpers_test.gocmd/event/golden_test.gocmd/event/list.gocmd/event/list_domain_test.gocmd/event/list_test.gocmd/event/preconditions_test.gocmd/event/preflight_test.gocmd/event/render/decision.gocmd/event/render/decision_test.gocmd/event/render/redaction_guard_test.gocmd/event/render_contract_test.gocmd/event/schema.gocmd/event/schema_test.gocmd/event/service_adapters.gocmd/event/status.gocmd/event/status_orphan_test.gocmd/event/stop.gocmd/event/stop_discover_test.gocmd/event/stop_integration_test.gocmd/event/suggestions.gocmd/event/suggestions_test.gocmd/event/testdata/golden/list_json.goldencmd/event/testdata/golden/list_text.goldencmd/event/testdata/golden/schema_board_whiteboard_json.goldencmd/event/testdata/golden/schema_board_whiteboard_text.goldencmd/event/testdata/golden/schema_card_action_trigger_json.goldencmd/event/testdata/golden/schema_card_action_trigger_text.goldencmd/event/testdata/golden/schema_im_chat_updated_json.goldencmd/event/testdata/golden/schema_im_chat_updated_text.goldencmd/event/testdata/golden/schema_im_message_receive_json.goldencmd/event/testdata/golden/schema_im_message_receive_text.goldencmd/event/wiring.goevents/all.goevents/application/menu.goevents/application/menu_test.goevents/approval/preconsume.goevents/approval/register.goevents/approval/register_test.goevents/arch_test.goevents/catalog_helper_test.goevents/compile_test.goevents/expected_keys_test.goevents/im/card_action.goevents/im/card_action_test.goevents/im/catalog_helper_test.goevents/im/message_receive.goevents/im/message_receive_test.goevents/internal/subscribeprep/subscribeprep.goevents/lint_test.goevents/minutes/catalog_helper_test.goevents/minutes/minute_generated.goevents/minutes/minute_generated_test.goevents/minutes/preconsume.goevents/minutes/register.goevents/output_baseline_test.goevents/schema_closure_test.goevents/schema_instance_test.goevents/task/register_test.goevents/testdata/output_baseline.jsonevents/vc/catalog_helper_test.goevents/vc/internal_helpers.goevents/vc/note_generated.goevents/vc/note_generated_test.goevents/vc/participant_meeting_ended.goevents/vc/participant_meeting_ended_test.goevents/vc/participant_meeting_joined.goevents/vc/participant_meeting_lifecycle_test.goevents/vc/participant_meeting_started.goevents/vc/preconsume.goevents/vc/recording_ended.goevents/vc/recording_started.goevents/vc/recording_test.goevents/vc/recording_transcript_generated.goevents/vc/register.goevents/vc/test_helpers_test.goevents/whiteboard/preconsume.goevents/whiteboard/register.goevents/whiteboard/subscription_scope_test.gointernal/event/adapter/lark/websocket/feishu.gointernal/event/adapter/lark/websocket/feishu_ingress_test.gointernal/event/adapter/lark/websocket/feishu_log_test.gointernal/event/adapter/lark/websocket/feishu_test.gointernal/event/adapter/lark/websocket/sdk_log_patterns.gointernal/event/adapter/lark/websocket/sdk_log_patterns_test.gointernal/event/adapter/lark/websocket/source.gointernal/event/adapter/lark/websocket/source_test.gointernal/event/adapter/lark/websocket/state_pinning_test.gointernal/event/adapter/localbus/busctl/busctl.gointernal/event/adapter/localbus/busdiscover/busdiscover.gointernal/event/adapter/localbus/busdiscover/pidfile.gointernal/event/adapter/localbus/busdiscover/pidfile_test.gointernal/event/adapter/localbus/protocol/canonical_fields_test.gointernal/event/adapter/localbus/protocol/codec.gointernal/event/adapter/localbus/protocol/codec_test.gointernal/event/adapter/localbus/protocol/messages.gointernal/event/adapter/localbus/protocol/messages_test.gointernal/event/adapter/localbus/transport/transport.gointernal/event/adapter/localbus/transport/transport_test.gointernal/event/adapter/localbus/transport/transport_unix.gointernal/event/adapter/localbus/transport/transport_windows.gointernal/event/application/consume/decision.gointernal/event/application/consume/service.gointernal/event/application/consume/service_test.gointernal/event/application/consume/strategy.gointernal/event/arch_layering_test.gointernal/event/bus/bus.gointernal/event/bus/bus_shutdown_test.gointernal/event/bus/conn.gointernal/event/bus/conn_test.gointernal/event/bus/handle_hello_test.gointernal/event/bus/hub.gointernal/event/bus/hub_observability_test.gointernal/event/bus/hub_test.gointernal/event/bus/source_port.gointernal/event/catalog/canonicalize.gointernal/event/catalog/compile.gointernal/event/catalog/compile_test.gointernal/event/catalog/definition.gointernal/event/catalog/params.gointernal/event/catalog/scope.gointernal/event/catalog/snapshot.gointernal/event/catalog/snapshot_test.gointernal/event/catalog/strategy.gointernal/event/consume/canonical_conflict.gointernal/event/consume/canonical_conflict_test.gointernal/event/consume/capability_gate_test.gointernal/event/consume/consume.gointernal/event/consume/consume_test.gointernal/event/consume/diagnostics_redaction_test.gointernal/event/consume/fingerprint.gointernal/event/consume/fingerprint_scope_test.gointernal/event/consume/handshake.gointernal/event/consume/loop.gointernal/event/consume/loop_seq_test.gointernal/event/consume/loop_test.gointernal/event/consume/reject_test.gointernal/event/consume/shutdown.gointernal/event/consume/shutdown_test.gointernal/event/consume/startup.gointernal/event/consume/startup_guard_test.gointernal/event/consume/startup_probe_test.gointernal/event/integration_test.gointernal/event/model/event.gointernal/event/preconsume_contract_test.gointernal/event/processing/result.gointernal/event/registry.gointernal/event/registry_test.gointernal/event/source/source.gointernal/event/testutil/testutil.gointernal/event/types.golint/domaincontract/unapproved_test.goskills/lark-event/SKILL.mdskills/lark-event/references/lark-event-vc.md
💤 Files with no reviewable changes (6)
- events/minutes/preconsume.go
- internal/event/registry.go
- internal/event/source/source.go
- events/vc/preconsume.go
- internal/event/registry_test.go
- cmd/build.go
🛑 Comments failed to post (6)
internal/event/adapter/localbus/protocol/codec_test.go (1)
65-72: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Check the
Encodeerror inTestEncodeAddsNewline.This test discards the error from
Encode(&buf, msg). Every otherEncodecall in this file checks the error. IfEncodefails here,bufcan end up empty, andbuf.Bytes()[buf.Len()-1]then indexes an empty slice instead of reporting a clear encode failure.🐛 Proposed fix
func TestEncodeAddsNewline(t *testing.T) { msg := &Bye{Type: MsgTypeBye} var buf bytes.Buffer - Encode(&buf, msg) + if err := Encode(&buf, msg); err != nil { + t.Fatalf("encode: %v", err) + } if buf.Bytes()[buf.Len()-1] != '\n' { t.Error("encoded message should end with newline") } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.func TestEncodeAddsNewline(t *testing.T) { msg := &Bye{Type: MsgTypeBye} var buf bytes.Buffer if err := Encode(&buf, msg); err != nil { t.Fatalf("encode: %v", err) } if buf.Bytes()[buf.Len()-1] != '\n' { t.Error("encoded message should end with newline") } }🤖 Prompt for 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. In `@internal/event/adapter/localbus/protocol/codec_test.go` around lines 65 - 72, Update TestEncodeAddsNewline to capture and assert the error returned by Encode before inspecting the buffer; report the encoding failure through the test and stop execution so the newline check cannot index an empty buffer.internal/event/adapter/localbus/protocol/codec.go (2)
17-105: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Locate repository error constructors and cause-preservation patterns. rg -n --glob '*.go' \ 'errs\.(New|Build)|WithCause\(|Subtype(Network|Validation|Unknown|FileIO)' \ internal cmd | head -n 240 # Locate all protocol error return sites and their callers. rg -n -C 3 --glob '*.go' \ 'ErrFrameTooLarge|protocol (encode|decode)|EncodeWithDeadline|ReadFrame\(' \ internal/eventRepository: larksuite/cli
Length of output: 43912
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== candidate error package files ==" fd -a '.*\.go$' internal/errclass internal/event/adapter/internal 2>/dev/null || true rg -n 'package errs|type (NetworkError|ValidationError|InternalError|APIError|Problem)|func New(Network|Validation|Internal|API|Authentication|Permission|Config)Error|func Subtype' internal/errclass internal/client internal/keychain internal/output | head -n 280 echo echo "== codec.go ==" cat -n internal/event/adapter/localbus/protocol/codec.go echo echo "== messages.go outline ==" ast-grep outline internal/event/adapter/localbus/protocol/messages.go --view compact || true sed -n '1,120p' internal/event/adapter/localbus/protocol/messages.go echo echo "== errclass package files ==" fd -a '*.go' internal/errclass | sort | while read -r f; do echo "--- $f ---" sed -n '1,220p' "$f" done echo echo "== call sites context ==" sed -n '20,130p' internal/event/adapter/localbus/protocol/messages.go sed -n '220,235p' internal/event/bus/bus.go sed -n '120,170p' internal/event/bus/conn.go sed -n '180,215p' internal/event/consume/consume_test.go sed -n '75,135p' internal/event/consume/loop_test.goRepository: larksuite/cli
Length of output: 1078
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== package declarations and typed constructors ==" sed -n '1,220p' internal/errclass/classify.go sed -n '1,80p' internal/errclass/codemeta.go rg -n 'func .*Build|func New.*Error|type .*Error|Subtype|WithCause|WithParam|Package errs' internal/errclass internal/keychain internal/oauth internal/client | head -n 260 echo echo "== codec.go ==" cat -n internal/event/adapter/localbus/protocol/codec.go echo echo "== protocol tests around errors ==" cat -n internal/event/adapter/localbus/protocol/messages_test.go | sed -n '60,125p'Repository: larksuite/cli
Length of output: 44726
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== locate errs package ==" fd -a '.*\.go$' . 2>/dev/null | rg '/errs/|errs_[^/]\.go$' | head -n 80 echo echo "== package errs files ==" fd -a '.*\.go$' . 2>/dev/null | rg '/errs/' | while read -r f; do echo "--- $f ---" sed -n '1,220p' "$f" done echo echo "== imports of localbus protocol with errors ==" rg -n --glob '*.go' 'internal/event/adapter/localbus/protocol|protocol\.' internal/event internal/client | head -n 200Repository: larksuite/cli
Length of output: 50369
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "== errs constructors ==" rg -n --glob '*.go' 'func (New|Wrap)(Network|Validation|Internal|API|Authentication|Config|Permission|ContentSafety|SecurityPolicy|ConfirmationRequired)Error|func WrapInternal|func WithCause|func NewValidationError|func NewNetworkError|func NewInternalError|func NewAPIError' errs/internal_carrier.go errs/raw.go errs/types.go | head -n 160 rg -n --glob '*.go' 'func (New|Wrap)(Network|Validation|Internal|API|Authentication|Config|Permission|ContentSafety|SecurityPolicy|ConfirmationRequired)Error|func WrapInternal|func WithCause|func NewValidationError|func NewNetworkError|func NewInternalError|func NewAPIError' | head -n 160 echo echo "== codec.go ==" cat -n internal/event/adapter/localbus/protocol/codec.go echo echo "== current import usages in codec ==" rg -n 'github.com/larksuite/cli/errs|protocol:|errors\.Is\(err, ErrFrameTooLarge\)|ErrFrameTooLarge' internal/event/adapter/internal internal/event/adapter/localbus/protocol errors.go 2>/dev/null || trueRepository: larksuite/cli
Length of output: 1242
Classify protocol boundary failures with typed errors.
codec.goreturns rawerrors.Newandfmt.Errorfvalues for malformed JSON, oversized frames, failed deadlines, and downstreamjson.Unmarshalfailures. Returnerrs.*errors from this boundary and preserve the original cause where applicable.🤖 Prompt for 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. In `@internal/event/adapter/localbus/protocol/codec.go` around lines 17 - 105, Update the protocol boundary functions Encode, EncodeWithDeadline, ReadFrame, and Decode to return the repository’s typed errs.* errors instead of raw errors.New or fmt.Errorf values. Classify oversized frames, deadline failures, malformed JSON, and downstream unmarshalling failures with the appropriate errs.* type, preserving the underlying cause through wrapping where applicable. Keep existing error context and behavior unchanged.Source: Coding guidelines
51-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce
MaxFrameBytesbefore returning the first chunk.When
brhas a buffer larger thanMaxFrameBytes,ReadSlicecan return a complete oversized frame on the first call. Lines 52-53 return that frame without a size check. This bypasses the protocol limit and sends oversized untrusted JSON toDecode.Check
len(chunk)before thelen(buf) == 0return path. Add a test that usesbufio.NewReaderSizewith a buffer larger thanMaxFrameBytes.Proposed fix
case nil: + if len(buf)+len(chunk) > MaxFrameBytes { + return nil, ErrFrameTooLarge + } if len(buf) == 0 { return chunk, nil } - if len(buf)+len(chunk) > MaxFrameBytes { - return nil, ErrFrameTooLarge - } return append(buf, chunk...), nil📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.case nil: if len(buf)+len(chunk) > MaxFrameBytes { return nil, ErrFrameTooLarge } if len(buf) == 0 { return chunk, nil } return append(buf, chunk...), nil🤖 Prompt for 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. In `@internal/event/adapter/localbus/protocol/codec.go` around lines 51 - 58, Update the nil-error branch of the ReadSlice handling to check len(chunk) against MaxFrameBytes before returning the first chunk, returning ErrFrameTooLarge when it exceeds the limit; retain the existing buf accumulation checks for subsequent chunks. Add coverage using bufio.NewReaderSize with a buffer larger than MaxFrameBytes to verify an oversized first frame is rejected before Decode.internal/event/adapter/localbus/transport/transport_test.go (2)
10-10: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Use
internal/vfsfor filesystem access.Lines 95 and 105 call
os.Statandos.Remove. Replace these calls with theinternal/vfsequivalents and remove theosimport.As per coding guidelines, use
internal/vfsfilesystem APIs instead ofosfilesystem APIs. Based on learnings, tests underinternal/must route filesystem interactions throughinternal/vfs.Also applies to: 95-105
🤖 Prompt for 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. In `@internal/event/adapter/localbus/transport/transport_test.go` at line 10, Replace the os.Stat and os.Remove calls in the transport tests with the corresponding internal/vfs filesystem APIs, and remove the now-unused os import. Keep the existing test behavior unchanged while routing all filesystem access through internal/vfs.Sources: Coding guidelines, Learnings
91-92: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Handle listener setup failure.
Line 91 ignores the
tr.Listenerror. If setup fails,ln.Close()dereferences a nil listener and hides the cause. Fail the test immediately whenListenreturns an error.Proposed fix
- ln, _ := tr.Listen(addr) + ln, err := tr.Listen(addr) + if err != nil { + t.Fatalf("listen: %v", err) + } ln.Close()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.ln, err := tr.Listen(addr) if err != nil { t.Fatalf("listen: %v", err) } ln.Close()🤖 Prompt for 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. In `@internal/event/adapter/localbus/transport/transport_test.go` around lines 91 - 92, Update the listener setup in the test around tr.Listen to capture and check its error, failing the test immediately with the testing helper if setup fails; only call ln.Close after a successful Listen.internal/event/adapter/localbus/transport/transport_unix.go (1)
26-34: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Return typed transport errors.
Lines 27-34 return raw file I/O and network errors. Wrap each error with the prescribed typed constructor and
.WithCause(err)before returning it. This preserves error classification for bus startup failures.As per coding guidelines, “Use the prescribed typed error constructors for validation, failed preconditions, API failures, network failures, file I/O failures, and unknown lower-layer errors.”
🤖 Prompt for 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. In `@internal/event/adapter/localbus/transport/transport_unix.go` around lines 26 - 34, Update unixTransport.Listen and unixTransport.Dial to wrap every MkdirAll, net.Listen, and net.DialTimeout failure with the prescribed typed transport error constructor and chain the original error via WithCause(err) before returning. Preserve successful return behavior and use the existing constructor conventions in the surrounding transport package.Source: Coding guidelines
EventKey names like minutes.minute.generated_v1 in the catalog golden fixtures trip the generic-api-key entropy heuristic. Allowlist secrets that are exactly dotted lowercase identifiers; real credentials do not take that shape, so detection strength is unchanged.
The wire frame formatted observed_at with the emitting host's local offset; normalize to UTC so frame bytes do not depend on where the bus runs. A non-empty observed_at that fails RFC3339Nano parsing on restore now surfaces a stderr diagnostic instead of silently zeroing, matching how canonical-metadata conflicts are reported.
Strategy-lookup failures now carry their cause for errors.Is/As. A decision blocked by a precondition that supplied no error synthesizes a failed_precondition error, so Execute can never no-op with nil on a blocked decision.
Map iteration order made the reported name vary when several unknown params were passed at once; sort and report the first.
Check the discarded type assertion in the schema-instance flip test, guard the snapshot fixture's key and raw-schema assumptions with clear failures instead of panics, and assert the payload round trip in the canonical-fields frame test.
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 @.gitleaks.toml:
- Around line 6-10: The global allowlist in the Gitleaks configuration is too
broad because it suppresses matching secrets from every detection rule. Scope
the EventKey pattern specifically to the generic-api-key rule using the
supported [[allowlists]] and targetRules configuration, or replace the regex
with exact known EventKey values; verify the pinned GitHub Actions Gitleaks
version supports the selected syntax.
🪄 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: 6b22f4f9-9a11-4fef-baf5-5438257f03ed
📒 Files selected for processing (11)
.gitleaks.tomlevents/schema_instance_test.gointernal/event/adapter/lark/websocket/source.gointernal/event/adapter/lark/websocket/source_test.gointernal/event/adapter/localbus/protocol/canonical_fields_test.gointernal/event/adapter/localbus/protocol/messages.gointernal/event/application/consume/service.gointernal/event/bus/hub_observability_test.gointernal/event/catalog/params.gointernal/event/catalog/snapshot_test.gointernal/event/consume/loop.go
💤 Files with no reviewable changes (1)
- internal/event/adapter/lark/websocket/source_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- internal/event/adapter/lark/websocket/source.go
- internal/event/catalog/params.go
- internal/event/adapter/localbus/protocol/canonical_fields_test.go
- events/schema_instance_test.go
- internal/event/catalog/snapshot_test.go
- internal/event/consume/loop.go
- internal/event/bus/hub_observability_test.go
- internal/event/application/consume/service.go
- internal/event/adapter/localbus/protocol/messages.go
| } | ||
|
|
||
| func approvalSubscriptionPreConsume(cfg approvalSubscriptionConfig) func(context.Context, event.APIClient, map[string]string) (func() error, error) { | ||
| func approvalSubscriptionPreConsume(eventType, subscribePath string) func(context.Context, event.APIClient, map[string]string) (func() error, error) { |
There was a problem hiding this comment.
🔍 Not actionable: this internal helper has exactly two call sites, both passing adjacent named constants declared next to the key definitions, so a silent swap is not a realistic failure mode here. Dedicated string types would add API surface without buying compile-time safety anyone needs yet; worth revisiting if the helper gains external callers.
| t.Errorf("an undeclared field must produce exactly one finding, got: %v", problems) | ||
| } | ||
|
|
||
| flipped, ok := decodeInstance(t, key, frozen).(map[string]any) |
There was a problem hiding this comment.
✅ Fixed in 626f5d2: the primitive-flip check now asserts the type assertion and fails clearly, matching the undeclared-field check above it.
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | ||
| // SPDX-License-Identifier: MIT | ||
|
|
||
| // Package websocket adapts the platform WebSocket connection into a pluggable |
There was a problem hiding this comment.
✅ Fixed in 115c1b6: the package comment now names websocket.
| if frame.EventID != ev.EventID || frame.EventType != ev.EventType || | ||
| frame.SourceTime != ev.SourceTime || frame.AppID != ev.AppID || | ||
| frame.TenantKey != ev.TenantKey || frame.Seq != 7 { | ||
| t.Errorf("canonical facts drifted across the wire: %+v", frame) |
There was a problem hiding this comment.
✅ Fixed in 626f5d2: the round-trip test now asserts the payload survives the wire verbatim.
| // UTC-normalized so the wire never carries the emitting host's local | ||
| // offset; consumers parse RFC3339Nano either way, but the frame bytes | ||
| // should not depend on where the bus happens to run. | ||
| observedAt = ev.Timestamp.UTC().Format(time.RFC3339Nano) |
There was a problem hiding this comment.
✅ Fixed in aa4a682: observed_at is now formatted from the UTC-normalized timestamp, so frame bytes no longer depend on the emitting host's local offset. The consumer-side restore also gained a WARN diagnostic for non-empty values that fail to parse.
| strategy, err := s.Strategies.get(strategyRef) | ||
| if err != nil { | ||
| return nil, errs.NewInternalError(errs.SubtypeUnknown, "%s", err).WithCause(err) | ||
| } |
There was a problem hiding this comment.
✅ Both findings on this file fixed in 585ec87: strategy-lookup errors now carry their cause via .WithCause(err) at both sites, and a decision blocked by a precondition that supplied no error synthesizes a failed_precondition error, so Execute can never return nil for a blocked decision.
| return errs.NewValidationError(errs.SubtypeInvalidArgument, | ||
| "unknown param %q: EventKey %s accepts no params", k, def.Key). | ||
| WithParam("--param"). | ||
| WithHint("run `lark-cli event schema %s` for details", def.Key) |
There was a problem hiding this comment.
✅ Deterministic reporting fixed in 11d9535: unknown names are sorted before the first is reported, so the message no longer varies with map iteration order.
🔍 Value/type validation (empty required values, enum/multi membership, int/bool parsing) is deliberately not added in this PR: it introduces new rejection paths beyond the declared behavior changes, which are locked by compatibility tests. Tracked as a follow-up with its own test locks.
|
|
||
| d := entry.Descriptor() | ||
| d.Params[0].Name = "tampered" | ||
| d.Params[0].Values[0].Value = "tampered" |
There was a problem hiding this comment.
✅ Fixed in 626f5d2: the test now resolves the fixture's own key with an explicit ok-check and guards the raw-custom-schema assumption, so a changed fixture fails with a clear message instead of a nil-pointer panic.
| } | ||
| } | ||
| return &event.RawEvent{ | ||
| EventID: evt.EventID, |
There was a problem hiding this comment.
✅ Fixed in aa4a682: a non-empty observed_at that fails RFC3339Nano parsing now emits a WARN naming only the event id and type, consistent with canonical-conflict reporting; the event is still delivered and an empty value stays silent by design.
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.
| # allowlist is appended to that rule only, leaving every other rule untouched. | ||
| [[rules]] | ||
| id = "generic-api-key" | ||
|
|
There was a problem hiding this comment.
✅ Fixed in f70f06f: the allowlist now extends the default generic-api-key rule via [[rules.allowlists]] and is additionally bounded with condition = "AND" to cmd/event/testdata/golden/, so the dotted-identifier exemption applies to that rule and that path only. The pinned Gitleaks version does not support global [[allowlists]]/targetRules, but its config merge keeps the base rule's regex and appends extension allowlists (with condition/paths support), so rule extension is the supported scoping mechanism. Verified locally: the fixture finding stays suppressed, the identical line outside the golden directory is still flagged, and removing the block brings the original finding back.
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.
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.
| } | ||
| if len(rt.calls) != 1 { | ||
| t.Errorf("calls = %d, want only the failed subscribe", len(rt.calls)) | ||
| } |
There was a problem hiding this comment.
✅ Assertion strengthening applied in 2822d8e: the failure paths now use a typed sentinel and assert the caller still sees it — errors.Is for cause survival plus errs.ProblemOf for category, subtype and the retryable flag — and the missing-client case asserts the typed internal error with errs.SubtypeUnknown. Verified by mutation: rewrapping the client error with fmt.Errorf now fails the test, where before it passed.
🔍 Not actionable — classifying inside this helper: consumeRuntime.CallAPI (the production client behind this port) already classifies every failure path into a typed problem before returning, including OAPI business errors via CheckResponse. Adding errclass.BuildAPIError here would wrap an already-typed error a second time and flatten the network/invalid-response distinction the client established. This helper's contract is pass-through, which is what the strengthened tests now lock.
1 participant
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 either renders it (
--dry-run) or executes 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).--dry-runrenders it and exits — provably without starting a bus, registering a consumer, running preparation, or creating files (spy-verified across all 25 keys). The real path executes the same decision; 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 and refuse an older bus explicitly — before any preparation side effect — with a recovery hint, instead of silently decoding missing fields as empty values.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— structured, side-effect-free consume preview on the standard envelope (top-leveldry_run: true, decision underdata.decision), honest three-state preconditions (ok/unknown/blocked), sensitive parameter values redacted.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:
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. Upgrade note: restart all whiteboard consumers after upgrading and avoid mixing old and new CLIs on the same whiteboard during the transition window.failed_preconditionand a recovery hint (stop old consumers,event stop, retry). The bus's 30-second idle auto-exit keeps this window small, but headless orchestrators should expect one recovery round.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
Summary by CodeRabbit
--dry-runpreviews for event consumption, including readiness decisions and preconditions without side effects.