test: isolate registry package state in tests#280
Merged
liangshuo-1 merged 2 commits intolarksuite:mainfrom Apr 7, 2026
Merged
test: isolate registry package state in tests#280liangshuo-1 merged 2 commits intolarksuite:mainfrom
liangshuo-1 merged 2 commits intolarksuite:mainfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded per-test registry reinitialization and environment setup in tests; expanded Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
niuchong0523
force-pushed
the
fix/registry-test-isolation
branch
from
bd334b1 to
7562961
Compare
github-actions
Bot
added
size/S
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1df146f4f355838132d59bade5f3c50b343bafe0🧩 Skill updatenpx skills add niuchong0523/cli#fix/registry-test-isolation -y -g |
Greptile SummaryThis PR improves test isolation for the Confidence Score: 5/5This PR is safe to merge; the prior P1 inverted-assertion issue is correctly fixed and all remaining findings are P2. The prior review thread's P1 concern (inverted assertion in TestOverlayMergedServicesDoesNotPolluteFollowingInit) is correctly resolved — the test now asserts absence of polluted services. The two remaining findings are P2 style issues about sleep-based goroutine synchronization and one test leaving dirty global state. These do not affect correctness and the PR's -race/-shuffle stress run demonstrates the tests pass as written. No files require special attention; minor P2 notes in remote_test.go are non-blocking. Important Files Changed
Sequence DiagramsequenceDiagram
participant T as Test
participant R as resetInit()
participant E as ensureFreshRegistry()
participant I as Init()
participant G as background goroutine
T->>R: resetInit()
R-->>T: clears initOnce, mergedServices,
embeddedVersion, all caches, refreshOnce
Note over T,E: Optional helper for sensitive tests
T->>E: ensureFreshRegistry(t)
E->>R: resetInit()
E->>I: Init() with REMOTE_META=off
I-->>E: loads embedded data only
E-->>T: fresh registry ready
Note over T,G: Tests enabling remote may trigger background refresh
T->>G: triggerBackgroundRefresh() via Init()
G-->>T: HTTP request to mock server
G-->>T: fetched channel signal
T-->>T: time.Sleep(50ms) [fragile guard]
Note over T,R: Next test cleanup
T->>R: resetInit() in next test
Reviews (5): Last reviewed commit: "Merge upstream/main into registry isolat..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
internal/registry/remote_test.go (1)
20-36:⚠️ Potential issue | 🔴 Critical
resetInit()is still racing with background refresh.This helper rewrites
embeddedVersion,configuredBrand,testMetaURL, and the caches without waiting for the goroutine started by earlierInit()calls. The attached-raceoutput is already showing reads fromdoBackgroundRefresh()/remoteMetaURL()against these writes, so the helper is still not safe to use as the package-wide reset point. Please add a test-visible way to drain or disable async refresh before clearing package state.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/remote_test.go` around lines 20 - 36, The resetInit() helper races with the background goroutine started by Init(); before resetting package-level vars (embeddedVersion, configuredBrand, testMetaURL, cached* and refreshOnce) ensure the async refresh is stopped/drained: add a test-visible control to disable or wait for doBackgroundRefresh() (e.g., export a test-only function or variable like DisableBackgroundRefresh(bool) or WaitForRefreshDone() that the tests can call) and use it in resetInit() to either disable remote refresh (set enableRemoteMeta=false and ensure any running goroutine exits) or block until the current background refresh completes (use a channel or sync.WaitGroup signaled by doBackgroundRefresh()). Update resetInit() to call that drain/disable function before clearing refreshOnce and other globals so remoteMetaURL()/doBackgroundRefresh() cannot read those values concurrently.shortcuts/event/router.go (1)
73-80:⚠️ Potential issue | 🟠 MajorDeduplicate matched directories before fan-out.
Match()returns every regex hit. If two routes resolve to the samedir,outputRouter.WriteRecord()will write the same record there twice with different sequence numbers. Preserve route order, but collapse duplicate directories before returning them.Suggested fix
func (r *EventRouter) Match(eventType string) []string { var dirs []string + seen := make(map[string]struct{}, len(r.routes)) for _, route := range r.routes { if route.pattern.MatchString(eventType) { + if _, ok := seen[route.dir]; ok { + continue + } + seen[route.dir] = struct{}{} dirs = append(dirs, route.dir) } } return dirs }Also applies to: 110-115
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/router.go` around lines 73 - 80, EventRouter.Match currently appends every route.dir for regex hits which can produce duplicate dirs; change Match to preserve route order but collapse duplicates by tracking seen directory strings (e.g., with a map[string]bool) and only append a dir the first time it is encountered. Apply the same de-duplication pattern to the other matching block in the same file that builds a dirs slice so no duplicate directories are returned for fan-out.shortcuts/event/processor_generic.go (1)
40-54:⚠️ Potential issue | 🟠 MajorHandle
nullevent payloads in GenericProcessor.Transform().When
raw.Eventis JSONnull,json.UnmarshalleaveseventMapnil, and the subsequent assignmenteventMap["type"] = ...panics. The current test suite (TestGenericProcessor_Compact,TestGenericProcessor_CompactUnmarshalError) does not cover this case, but it is a valid input that should be normalized to an empty map.Suggested fix
func (p *GenericProcessor) Transform(_ context.Context, raw *RawEvent, mode TransformMode) interface{} { if mode == TransformRaw { return raw } // Compact: parse event as flat map, inject envelope metadata so AI // can always identify the event type regardless of which processor ran. var eventMap map[string]interface{} if err := json.Unmarshal(raw.Event, &eventMap); err != nil { return raw } + if eventMap == nil { + eventMap = map[string]interface{}{} + } eventMap["type"] = raw.Header.EventType🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/processor_generic.go` around lines 40 - 54, genericCompactMap currently panics when raw.Event is JSON null because json.Unmarshal leaves eventMap nil; update genericCompactMap to check if eventMap == nil after unmarshalling and, if so, initialize eventMap = make(map[string]interface{}) before assigning eventMap["type"], eventMap["event_id"], and eventMap["timestamp"]; preserve the existing behavior of returning raw on unmarshal error (the err branch) and only normalize null payloads to an empty map so GenericProcessor.Transform handles JSON null safely.
🧹 Nitpick comments (8)
shortcuts/event/matcher.go (1)
6-22: Consider:Matchedfield appears redundant with theokreturn value.Both
RawEventMatch.Matchedand theokreturn value always have the same boolean state. If this is intentional for API consistency or future extensibility, consider documenting that intent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/matcher.go` around lines 6 - 22, The RawEventMatch.Matched field is redundant with the boolean ok return from MatchRawEventType; remove Matched from the RawEventMatch struct and return only the EventType in MatchRawEventType (i.e., construct RawEventMatch{EventType: evt.EventType}) and rely on the bool return to indicate success; update all callers of MatchRawEventType and any uses of RawEventMatch.Matched to use the function's ok return instead and adjust any tests or serialization that referenced Matched.shortcuts/event/subscribe.go (1)
226-226: Remove unnecessary blank assignment.
_ = jsonFlagappears to be dead code sincejsonFlagis already used on Line 244 inpipelineConfigFor(jsonFlag, compactFlag). This blank assignment serves no purpose.Remove the unused blank assignment
- _ = jsonFlag hasRoutes := len(routeSpecs) > 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/subscribe.go` at line 226, Remove the unnecessary blank assignment `_ = jsonFlag` in subscribe.go: delete that line since `jsonFlag` is already used later by `pipelineConfigFor(jsonFlag, compactFlag)` and the `_ =` no-op serves no purpose; ensure there are no other silent uses of `jsonFlag` before removal and run tests/build to confirm no unused-variable warnings remain.shortcuts/event/processor_im_message.go (1)
95-101: Directos.Stderrwrite bypasses configured error output.Line 99 writes directly to
os.Stderr, but the pipeline and handlers elsewhere use an injectederrOutwriter for testability and consistency. This hint message won't be captured in tests and won't respect the--quietflag.Consider passing the error writer through or removing this debug hint, since the
"interactive_fallback"reason in theHandlerResultalready communicates this information to downstream consumers (as shown inpipeline.goLine 163-164 where reason is included in output).Option: Remove the hint since reason is already captured
func buildIMMessageCompactOutput(eventType, headerCreateTime string, ev imMessagePayload) (map[string]interface{}, bool) { // Card messages (interactive) are not yet supported for compact conversion; // return raw event data directly. if ev.Message.MessageType == "interactive" { - fmt.Fprintf(os.Stderr, "%s[hint]%s card message (interactive) compact conversion is not yet supported, returning raw event data\n", output.Dim, output.Reset) return nil, false }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/processor_im_message.go` around lines 95 - 101, The function buildIMMessageCompactOutput currently writes a hint directly to os.Stderr when ev.Message.MessageType == "interactive" (via fmt.Fprintf), which bypasses the configured error writer; remove that direct stderr write and simply return nil, false (or otherwise rely on the existing "interactive_fallback" reason in the HandlerResult) so the message flows through the pipeline handlers that use the injected errOut; locate the fmt.Fprintf call in buildIMMessageCompactOutput and delete it (do not add a new global writer), relying on the existing downstream reason reporting instead.shortcuts/event/processor_im_message_read.go (1)
60-68: Silent error swallowing may mask data issues.Lines 64-66 ignore marshal/unmarshal errors with blank assignments. If
evt.Payload.Datais malformed, the output will contain only what was successfully parsed from a zero-valueimMessageReadPayload, potentially producing incomplete or misleading records.Consider logging or returning an error indicator, or at minimum adding a comment explaining why errors are intentionally ignored here.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/processor_im_message_read.go` around lines 60 - 68, The function imMessageReadCompactOutput silently ignores JSON Marshal/Unmarshal errors for evt.Payload.Data which can mask malformed payloads; update imMessageReadCompactOutput to check and handle errors from json.Marshal and json.Unmarshal (for the imMessageReadPayload type): on marshal/unmarshal failure either log the error via the existing logger or include an explicit error indicator in the returned map (e.g., add an "error" field) before calling buildIMMessageReadCompactOutput, and/or add a clear comment explaining why errors are acceptable if you intentionally choose to ignore them.shortcuts/event/registry.go (1)
78-98: Silent error handling inNewBuiltinHandlerRegistrycould mask registration issues.Lines 94 and 96 ignore errors from
RegisterEventHandlerandSetFallbackHandler. If a built-in handler has a duplicate ID (due to a coding error), registration will silently fail and the handler won't be available at runtime.Since these are compile-time-known handlers, consider either:
- Panicking on registration failure (fail fast during initialization)
- Using
must-style helper that panics- At minimum, logging the error
Option: Panic on registration failure for built-in handlers
func NewBuiltinHandlerRegistry() *HandlerRegistry { r := NewHandlerRegistry() for _, h := range []EventHandler{ NewIMMessageReceiveHandler(), NewIMMessageReadHandler(), NewIMReactionCreatedHandler(), NewIMReactionDeletedHandler(), NewIMChatUpdatedHandler(), NewIMChatDisbandedHandler(), NewIMChatMemberBotAddedHandler(), NewIMChatMemberBotDeletedHandler(), NewIMChatMemberUserAddedHandler(), NewIMChatMemberUserWithdrawnHandler(), NewIMChatMemberUserDeletedHandler(), } { - _ = r.RegisterEventHandler(h) + if err := r.RegisterEventHandler(h); err != nil { + panic(fmt.Sprintf("failed to register builtin handler %s: %v", h.ID(), err)) + } } - _ = r.SetFallbackHandler(NewGenericFallbackHandler()) + if err := r.SetFallbackHandler(NewGenericFallbackHandler()); err != nil { + panic(fmt.Sprintf("failed to set fallback handler: %v", err)) + } return r }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/registry.go` around lines 78 - 98, NewBuiltinHandlerRegistry currently ignores errors from HandlerRegistry.RegisterEventHandler and HandlerRegistry.SetFallbackHandler which can silently drop handlers; modify NewBuiltinHandlerRegistry to handle errors returned by RegisterEventHandler (called for each NewIM...Handler) and SetFallbackHandler(NewGenericFallbackHandler()) by panicking (or using a must-style helper that panics) on non-nil error so initialization fails fast; update the loop to check the error from r.RegisterEventHandler(h) and call panic(fmt.Sprintf("failed to register builtin handler %T: %v", h, err)) (or similar), and likewise panic if r.SetFallbackHandler(...) returns an error.shortcuts/event/pipeline.go (1)
115-145: Early return on write error may cause partial processing.Lines 129 and 141 return immediately on write error, which stops processing remaining dispatch results. If an event has multiple matching handlers, a write failure on the first result will silently skip the remaining handlers' outputs.
Consider whether to:
- Continue processing and accumulate errors
- Log the skipped count
- Document this as intentional fail-fast behavior
This is likely acceptable for the current use case (CLI streaming output), but worth noting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/pipeline.go` around lines 115 - 145, The current dispatch method returns immediately on write errors (in the p.recordWriter branch and the p.writeRecord branch), which aborts processing remaining result records; modify EventPipeline.dispatch so that on a write error it does not return immediately but instead records/accumulates the error (e.g., append to a local error slice or increment an error counter), logs the failure via output.PrintError with context (include evt.EventType and the record/reason), continues processing the rest of result.Results, and after the loop either return a consolidated error or log a summary (including skipped count) so callers like dispatch, rawModeRecord, writeRecord, p.recordWriter and p.dispatchedN still reflect all processed records.shortcuts/event/subscribe_test.go (1)
167-183: Consider usingt.Setenvpattern or ensuring test doesn't run in parallel.The
os.Chdircall modifies global process state. While thedefercorrectly restores the original directory, this test will conflict with any parallel tests that rely on the working directory.If this test runs with
t.Parallel()elsewhere in the suite, it could cause flaky failures. The current implementation is safe as long as tests in this package don't uset.Parallel().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/subscribe_test.go` around lines 167 - 183, The test TestEventSubscribeExecuteRouteWritesEventFile currently calls os.Chdir which mutates global process state and can cause flakes when tests run in parallel; update the test to avoid changing the global working directory: either make the runtime created by newSubscribeRuntimeForTest accept a working directory parameter (or set an explicit output path on the runtime/command so files are created under tmpDir), or ensure restoration via t.Cleanup and mark the test as non-parallel (do not call t.Parallel anywhere); modify TestEventSubscribeExecuteRouteWritesEventFile to use the tmpDir explicitly for file operations (or pass tmpDir into newSubscribeRuntimeForTest) and remove the os.Chdir/os.Getwd sequence so the test is safe for parallel execution.shortcuts/event/processor_im_chat_member.go (1)
153-160: Avoid the JSON round-trip in the handler path.Both helpers marshal
evt.Payload.Databack to JSON, unmarshal it again, and ignore both errors. That adds avoidable allocations on every dispatch and turns payload-shape drift into partially emptyhandledoutput. A small shared decode helper that returns an error would make this path cheaper and safer.Also applies to: 186-193
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/event/processor_im_chat_member.go` around lines 153 - 160, The handler currently does a JSON marshal/unmarshal on evt.Payload.Data in imChatBotCompactOutput (and the similar block at lines 186-193), ignoring errors and causing extra allocations; replace that round-trip with a shared decoding helper (e.g., decodeToImChatMemberPayload(data interface{}) (imChatMemberPayload, error)) that attempts a type-safe conversion/unmarshal once and returns an error if decoding fails, then call that helper from imChatBotCompactOutput and the other handler, check the error, and only call buildIMChatBotCompactOutput when decoding succeeds (or return the minimal map with type on error); reference imChatBotCompactOutput, imChatMemberPayload, buildIMChatBotCompactOutput and the evt.Payload.Data value when implementing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/registry/meta_data_default.json`:
- Line 1: The embedded baseline JSON only marks methods with
accessTokens:["user"], so the tenant-scope loader filters them out and yields an
empty tenant minimum-scope set; update the default metadata (the "services"
entry for "calendar" and at least one method such as "events.create" or
"calendars.create") to include a tenant-capable access token (e.g., add "tenant"
to the accessTokens array for one or more methods) so that tenant flows have a
non-empty minimum-scope set on fresh init.
In `@internal/registry/registry_test.go`:
- Around line 12-16: ensureFreshRegistry still leaves Init() able to read real
machine state; modify ensureFreshRegistry to set the test config dir and seed
deterministic registry data before calling Init(): in ensureFreshRegistry(t
*testing.T) call t.TempDir(), set the LARKSUITE_CLI_CONFIG_DIR env var to that
temp dir, disable remote fetching (so Init() won't load remote_meta.json), write
the specific registry fixture file(s) into that temp dir (the exact
remote_meta.json or embedded baseline the tests expect), then call resetInit()
and Init(); reference the functions ensureFreshRegistry, resetInit, Init and the
env var LARKSUITE_CLI_CONFIG_DIR when making these changes.
In `@internal/registry/remote_test.go`:
- Around line 327-340: The test
TestOverlayMergedServicesDoesNotPolluteFollowingInit should be tightened to
assert that the sentinel service injected via overlayMergedServices is absent
after resetInit()+Init(): after calling
overlayMergedServices(&MergedRegistry{Services:
[]map[string]interface{}{{"name":"brand_new","version":"v1"}}}) and then
resetInit(); Init(), explicitly check that ListFromMetaProjects() (or the
registry lookup function that would return "brand_new") does NOT contain an
entry with name "brand_new"; also make the test use a temporary config directory
and disable remote fetching during Init (e.g., set the config/remote flags or
call the Init variant that accepts a config dir/remote-off) so the assertion
cannot be satisfied by ambient cache data or remote backends.
In `@shortcuts/event/concurrency_test.go`:
- Around line 29-50: The test TestOutputRouterWriteRecordConcurrent currently
only checks for WriteRecord errors; after wg.Wait() read the files in
router.defaultDir (e.g., using os.ReadDir(router.defaultDir) or
filepath.Glob(filepath.Join(router.defaultDir, "*"))) and assert that the number
of records equals workers; if ReadDir returns an error fail the test, otherwise
compare len(entries) == workers and call t.Fatalf/t.Errorf with a clear message
if not equal to ensure concurrent writes produced exactly workers files.
In `@shortcuts/event/processor_im_chat_member.go`:
- Around line 41-48: The current ImChatBotProcessor.ID() and Domain() advertise
handlers even when eventType is unset or unknown; change ID() to return an empty
string unless p.eventType is explicitly one of the supported values (e.g.,
contains "deleted" -> "builtin.im.chat.member.bot.deleted", contains "added" ->
"builtin.im.chat.member.bot.added"), and change Domain() to return "im" only
when p.eventType is set to a known value; otherwise return "". Also apply the
same validation pattern to the other analogous processor methods referenced
(lines 111-123) so zero-values do not register as catch-all handlers.
In `@shortcuts/event/processor_test.go`:
- Around line 617-620: The test currently splits TrimSpace(out.String()) into
lines which returns one element for empty stdout and hides regressions; before
splitting, explicitly check whether strings.TrimSpace(out.String()) is empty and
fail the test if so (use the existing t.Fatalf), then proceed to split into
lines and assert len(lines) == 1; reference the test variables out and lines in
shortcuts/event/processor_test.go to locate where to add the guard.
In `@shortcuts/event/router.go`:
- Around line 150-177: The file-naming currently ignores the explicit eventType
argument and only reads record["event_type"], causing compact-mode events that
set record["type"] to be misnamed; fix by using the explicit eventType when
building the filename: change dirRecordWriter.WriteRecord to accept and use the
eventType parameter (replace the blank identifier with eventType) and update
nextFileName to take eventType as an argument (e.g., nextFileName(eventType
string, record map[string]interface{})), then prefer the passed eventType when
computing base (fall back to record["event_type"] or record["type"] if the
argument is empty) and keep sanitizeRouteFilePart, seq handling, and file
formatting the same.
---
Outside diff comments:
In `@internal/registry/remote_test.go`:
- Around line 20-36: The resetInit() helper races with the background goroutine
started by Init(); before resetting package-level vars (embeddedVersion,
configuredBrand, testMetaURL, cached* and refreshOnce) ensure the async refresh
is stopped/drained: add a test-visible control to disable or wait for
doBackgroundRefresh() (e.g., export a test-only function or variable like
DisableBackgroundRefresh(bool) or WaitForRefreshDone() that the tests can call)
and use it in resetInit() to either disable remote refresh (set
enableRemoteMeta=false and ensure any running goroutine exits) or block until
the current background refresh completes (use a channel or sync.WaitGroup
signaled by doBackgroundRefresh()). Update resetInit() to call that
drain/disable function before clearing refreshOnce and other globals so
remoteMetaURL()/doBackgroundRefresh() cannot read those values concurrently.
In `@shortcuts/event/processor_generic.go`:
- Around line 40-54: genericCompactMap currently panics when raw.Event is JSON
null because json.Unmarshal leaves eventMap nil; update genericCompactMap to
check if eventMap == nil after unmarshalling and, if so, initialize eventMap =
make(map[string]interface{}) before assigning eventMap["type"],
eventMap["event_id"], and eventMap["timestamp"]; preserve the existing behavior
of returning raw on unmarshal error (the err branch) and only normalize null
payloads to an empty map so GenericProcessor.Transform handles JSON null safely.
In `@shortcuts/event/router.go`:
- Around line 73-80: EventRouter.Match currently appends every route.dir for
regex hits which can produce duplicate dirs; change Match to preserve route
order but collapse duplicates by tracking seen directory strings (e.g., with a
map[string]bool) and only append a dir the first time it is encountered. Apply
the same de-duplication pattern to the other matching block in the same file
that builds a dirs slice so no duplicate directories are returned for fan-out.
---
Nitpick comments:
In `@shortcuts/event/matcher.go`:
- Around line 6-22: The RawEventMatch.Matched field is redundant with the
boolean ok return from MatchRawEventType; remove Matched from the RawEventMatch
struct and return only the EventType in MatchRawEventType (i.e., construct
RawEventMatch{EventType: evt.EventType}) and rely on the bool return to indicate
success; update all callers of MatchRawEventType and any uses of
RawEventMatch.Matched to use the function's ok return instead and adjust any
tests or serialization that referenced Matched.
In `@shortcuts/event/pipeline.go`:
- Around line 115-145: The current dispatch method returns immediately on write
errors (in the p.recordWriter branch and the p.writeRecord branch), which aborts
processing remaining result records; modify EventPipeline.dispatch so that on a
write error it does not return immediately but instead records/accumulates the
error (e.g., append to a local error slice or increment an error counter), logs
the failure via output.PrintError with context (include evt.EventType and the
record/reason), continues processing the rest of result.Results, and after the
loop either return a consolidated error or log a summary (including skipped
count) so callers like dispatch, rawModeRecord, writeRecord, p.recordWriter and
p.dispatchedN still reflect all processed records.
In `@shortcuts/event/processor_im_chat_member.go`:
- Around line 153-160: The handler currently does a JSON marshal/unmarshal on
evt.Payload.Data in imChatBotCompactOutput (and the similar block at lines
186-193), ignoring errors and causing extra allocations; replace that round-trip
with a shared decoding helper (e.g., decodeToImChatMemberPayload(data
interface{}) (imChatMemberPayload, error)) that attempts a type-safe
conversion/unmarshal once and returns an error if decoding fails, then call that
helper from imChatBotCompactOutput and the other handler, check the error, and
only call buildIMChatBotCompactOutput when decoding succeeds (or return the
minimal map with type on error); reference imChatBotCompactOutput,
imChatMemberPayload, buildIMChatBotCompactOutput and the evt.Payload.Data value
when implementing.
In `@shortcuts/event/processor_im_message_read.go`:
- Around line 60-68: The function imMessageReadCompactOutput silently ignores
JSON Marshal/Unmarshal errors for evt.Payload.Data which can mask malformed
payloads; update imMessageReadCompactOutput to check and handle errors from
json.Marshal and json.Unmarshal (for the imMessageReadPayload type): on
marshal/unmarshal failure either log the error via the existing logger or
include an explicit error indicator in the returned map (e.g., add an "error"
field) before calling buildIMMessageReadCompactOutput, and/or add a clear
comment explaining why errors are acceptable if you intentionally choose to
ignore them.
In `@shortcuts/event/processor_im_message.go`:
- Around line 95-101: The function buildIMMessageCompactOutput currently writes
a hint directly to os.Stderr when ev.Message.MessageType == "interactive" (via
fmt.Fprintf), which bypasses the configured error writer; remove that direct
stderr write and simply return nil, false (or otherwise rely on the existing
"interactive_fallback" reason in the HandlerResult) so the message flows through
the pipeline handlers that use the injected errOut; locate the fmt.Fprintf call
in buildIMMessageCompactOutput and delete it (do not add a new global writer),
relying on the existing downstream reason reporting instead.
In `@shortcuts/event/registry.go`:
- Around line 78-98: NewBuiltinHandlerRegistry currently ignores errors from
HandlerRegistry.RegisterEventHandler and HandlerRegistry.SetFallbackHandler
which can silently drop handlers; modify NewBuiltinHandlerRegistry to handle
errors returned by RegisterEventHandler (called for each NewIM...Handler) and
SetFallbackHandler(NewGenericFallbackHandler()) by panicking (or using a
must-style helper that panics) on non-nil error so initialization fails fast;
update the loop to check the error from r.RegisterEventHandler(h) and call
panic(fmt.Sprintf("failed to register builtin handler %T: %v", h, err)) (or
similar), and likewise panic if r.SetFallbackHandler(...) returns an error.
In `@shortcuts/event/subscribe_test.go`:
- Around line 167-183: The test TestEventSubscribeExecuteRouteWritesEventFile
currently calls os.Chdir which mutates global process state and can cause flakes
when tests run in parallel; update the test to avoid changing the global working
directory: either make the runtime created by newSubscribeRuntimeForTest accept
a working directory parameter (or set an explicit output path on the
runtime/command so files are created under tmpDir), or ensure restoration via
t.Cleanup and mark the test as non-parallel (do not call t.Parallel anywhere);
modify TestEventSubscribeExecuteRouteWritesEventFile to use the tmpDir
explicitly for file operations (or pass tmpDir into newSubscribeRuntimeForTest)
and remove the os.Chdir/os.Getwd sequence so the test is safe for parallel
execution.
In `@shortcuts/event/subscribe.go`:
- Line 226: Remove the unnecessary blank assignment `_ = jsonFlag` in
subscribe.go: delete that line since `jsonFlag` is already used later by
`pipelineConfigFor(jsonFlag, compactFlag)` and the `_ =` no-op serves no
purpose; ensure there are no other silent uses of `jsonFlag` before removal and
run tests/build to confirm no unused-variable warnings remain.
🪄 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
Run ID: fd115d65-6d55-43c6-a470-48d29787c711
📒 Files selected for processing (32)
.gitignoreinternal/registry/loader.gointernal/registry/loader_embedded.gointernal/registry/meta_data_default.jsoninternal/registry/registry_test.gointernal/registry/remote_test.goshortcuts/event/concurrency_test.goshortcuts/event/deduper.goshortcuts/event/deduper_test.goshortcuts/event/dispatcher.goshortcuts/event/dispatcher_test.goshortcuts/event/domain_resolver.goshortcuts/event/domain_resolver_test.goshortcuts/event/envelope.goshortcuts/event/handlers.goshortcuts/event/matcher.goshortcuts/event/normalizer.goshortcuts/event/normalizer_test.goshortcuts/event/pipeline.goshortcuts/event/processor_generic.goshortcuts/event/processor_im_chat.goshortcuts/event/processor_im_chat_member.goshortcuts/event/processor_im_message.goshortcuts/event/processor_im_message_reaction.goshortcuts/event/processor_im_message_read.goshortcuts/event/processor_im_test.goshortcuts/event/processor_test.goshortcuts/event/registry.goshortcuts/event/router.goshortcuts/event/router_permissions_test.goshortcuts/event/subscribe.goshortcuts/event/subscribe_test.go
There was a problem hiding this comment.
♻️ Duplicate comments (1)
internal/registry/remote_test.go (1)
327-342:⚠️ Potential issue | 🟡 MinorTest isolation incomplete: missing config dir and remote isolation.
The test now properly asserts that injected services don't survive
resetInit(), which is an improvement. However, the test still lacks environment isolation:
- No temp config dir: Without setting
LARKSUITE_CLI_CONFIG_DIR,Init()may read from ambient cache.- No remote disabled: Without
LARKSUITE_CLI_REMOTE_META=off,Init()could fetch remote data, making assertions depend on network state.- Generic service names: "existing" and "brand_new" could theoretically collide with real service names in embedded/remote data.
Suggested fix for proper isolation
func TestOverlayMergedServicesDoesNotPolluteFollowingInit(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + t.Setenv("LARKSUITE_CLI_REMOTE_META", "off") resetInit() mergedServices = map[string]map[string]interface{}{ - "existing": {"name": "existing", "version": "v1"}, + "zz_test_overlay_existing": {"name": "zz_test_overlay_existing", "version": "v1"}, } - overlayMergedServices(&MergedRegistry{Services: []map[string]interface{}{{"name": "brand_new", "version": "v1"}}}) + overlayMergedServices(&MergedRegistry{Services: []map[string]interface{}{{"name": "zz_test_overlay_new", "version": "v1"}}}) resetInit() Init() for _, name := range ListFromMetaProjects() { - if name == "existing" || name == "brand_new" { + if name == "zz_test_overlay_existing" || name == "zz_test_overlay_new" { t.Fatalf("polluted service %q survived resetInit", name) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/registry/remote_test.go` around lines 327 - 342, Update the test TestOverlayMergedServicesDoesNotPolluteFollowingInit to fully isolate environment: before calling resetInit()/Init(), create a temp directory and set LARKSUITE_CLI_CONFIG_DIR to it (and unset/restore after), and set LARKSUITE_CLI_REMOTE_META=off so Init() cannot fetch remote data; use unique test-only service names (e.g., "test_existing_..." and "test_brand_new_...") when calling overlayMergedServices and ListFromMetaProjects to avoid collisions, and ensure you clean up the temp dir and restore original environment variables at the end of the test so resetInit(), Init(), overlayMergedServices and ListFromMetaProjects run in a deterministic, isolated environment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@internal/registry/remote_test.go`:
- Around line 327-342: Update the test
TestOverlayMergedServicesDoesNotPolluteFollowingInit to fully isolate
environment: before calling resetInit()/Init(), create a temp directory and set
LARKSUITE_CLI_CONFIG_DIR to it (and unset/restore after), and set
LARKSUITE_CLI_REMOTE_META=off so Init() cannot fetch remote data; use unique
test-only service names (e.g., "test_existing_..." and "test_brand_new_...")
when calling overlayMergedServices and ListFromMetaProjects to avoid collisions,
and ensure you clean up the temp dir and restore original environment variables
at the end of the test so resetInit(), Init(), overlayMergedServices and
ListFromMetaProjects run in a deterministic, isolated environment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0dbab268-a2e2-48e3-b63a-30cea709d42b
📒 Files selected for processing (2)
internal/registry/registry_test.gointernal/registry/remote_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/registry/registry_test.go
niuchong0523
force-pushed
the
fix/registry-test-isolation
branch
from
325e9a7 to
3ddd148
Compare
Reset registry test globals more completely, tighten the overlay pollution regressions, and ensure tenant scope coverage tests rebuild a fresh isolated registry before asserting.
niuchong0523
force-pushed
the
fix/registry-test-isolation
branch
from
3ddd148 to
05e3522
Compare
liangshuo-1
approved these changes
Apr 7, 2026
tuxedomm
pushed a commit
that referenced
this pull request
Apr 8, 2026
Reset registry test globals more completely, tighten the overlay pollution regressions, and ensure tenant scope coverage tests rebuild a fresh isolated registry before asserting.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit