wip - #2064
Conversation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
cmd/event/bus.go-32-41 (1)
32-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMake the bus UAT sentinel errors typed.
cmd/event/bus.gois under the activecmd/scope oferrs-no-bare-wrap, soerrUATUserMismatch/errUATUnverifiablecreated with bareerrors.Newand line 75fmt.Errorf("%w", errUATUnverifiable)will fail the pinned lint. Use typed error constructors fromgithub.com/larksuite/cli/errsand preserve the cause with.WithCause(err).🤖 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/bus.go` around lines 32 - 41, Replace the bare errors.New definitions of errUATUserMismatch and errUATUnverifiable with typed constructors from github.com/larksuite/cli/errs. Update the error path around verifyUATBelongsToUser that currently uses fmt.Errorf("%w", errUATUnverifiable) to preserve the underlying cause via the typed error’s WithCause(err), while retaining the existing sentinel identities and messages.Source: Coding guidelines
internal/event/bus/bus.go-445-455 (1)
445-455: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winGuard the encrypted path on a non-empty
RemoteSubscriptionID.The completeness gate above only fires when
RemoteSubscriptionID != "". A Hello withIncludeResourceData=trueand an emptyRemoteSubscriptionIDtherefore reachesfetchAndSetwith an empty subscription id —protocol.Hello's own contract saysIncludeResourceDatais "Only meaningful alongside a non-empty RemoteSubscriptionID" (internal/event/protocol/messages.go lines 144-151). Fail closed on that combination rather than issuing aGetEncryptKey("").🛡️ Proposed guard
- if hello.IncludeResourceData && b.encryptKeyProvider != nil { + if hello.IncludeResourceData && bc.RemoteSubscriptionID() == "" { + b.logger.Printf("WARN: rejecting encrypted consumer pid=%d key=%q: include_resource_data without a remote_subscription_id", + hello.PID, hello.EventKey) + if werr := bc.writeFrame(protocol.NewHelloAckRejected("v1", protocol.RejectReasonIncompleteRefinedHello)); werr != nil { + b.logger.Printf("WARN: reject hello_ack (incomplete_refined_hello) write to pid=%d key=%q failed: %v", + hello.PID, hello.EventKey, werr) + } + bc.Close() + return + } + if hello.IncludeResourceData && b.encryptKeyProvider != 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/bus/bus.go` around lines 445 - 455, Update the encrypted-resource handling around b.encryptKeyProvider.fetchAndSet to require a non-empty bc.RemoteSubscriptionID() in addition to IncludeResourceData. When IncludeResourceData is true but the subscription ID is empty, reject the Hello and close the connection using the existing decrypt-key-unavailable response path, without calling fetchAndSet or issuing a key lookup with an empty ID.internal/event/bus/status_query_test.go-61-66 (1)
61-66: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the exact v2 markers, not just non-emptiness.
A regression that sets
ProtocolVersion = "v1"or dropsCapabilityHelloV2from the list still passes these checks.protocol.ProtocolVersionV2,protocol.CapabilityRefinedRouting, andprotocol.CapabilityHelloV2are exported constants — compare against them directly.As per coding guidelines, "contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
💚 Proposed assertions
- if resp.ProtocolVersion == "" { - t.Error("ProtocolVersion is empty, want a non-empty v2 marker") - } - if len(resp.Capabilities) == 0 { - t.Error("Capabilities is empty, want a non-empty capability list") - } + if resp.ProtocolVersion != protocol.ProtocolVersionV2 { + t.Errorf("ProtocolVersion = %q, want %q", resp.ProtocolVersion, protocol.ProtocolVersionV2) + } + for _, want := range []string{protocol.CapabilityRefinedRouting, protocol.CapabilityHelloV2} { + if !slices.Contains(resp.Capabilities, want) { + t.Errorf("Capabilities = %v, want it to contain %q", resp.Capabilities, want) + } + }🤖 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/bus/status_query_test.go` around lines 61 - 66, Update the response assertions in the status query test to compare ProtocolVersion directly with protocol.ProtocolVersionV2, and verify Capabilities contains both protocol.CapabilityRefinedRouting and protocol.CapabilityHelloV2. Replace the non-empty checks while preserving the existing failure reporting style.Source: Coding guidelines
cmd/event/consume.go-70-74 (1)
70-74: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHelp text cites refined EventKeys that aren't in the shipped catalog. The only registered refined base key is
im.message.created_v1(events/refined/refined_keys_mock.json), yet theeventcommand tree's help documents two other names; copy-pasted examples fail with "unknown EventKey".
cmd/event/consume.go#L70-L74: replaceim.message.example_v1withim.message.created_v1in the REFINED EVENTKEYS paragraph, and in theExampleblock at lines 102-105.cmd/event/event.go#L21-L24: replaceim.message.receive_v2/chat-id/oc_xxxwithim.message.created_v1/chat-id/oc_xxx, matching the same file's own example at line 45.🤖 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/consume.go` around lines 70 - 74, Update the refined EventKey examples to use the registered base key im.message.created_v1: change the REFINED EVENTKEYS paragraph and Example block in cmd/event/consume.go, and the example in cmd/event/event.go, replacing im.message.example_v1 and im.message.receive_v2 respectively while preserving the existing resource-selector format.cmd/event/subscription/subscription_test.go-230-237 (1)
230-237: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winIsolate config state for the
cmdutil.TestFactorycase.This test builds a real factory but doesn't pin
LARKSUITE_CLI_CONFIG_DIR, so it can read the developer's/CI host config dir.As per coding guidelines: "Use
cmdutil.TestFactory(t, config)for test factories and setLARKSUITE_CLI_CONFIG_DIRtot.TempDir()witht.Setenvto isolate configuration state."🛡️ Proposed fix
func TestResolveEffectiveIdentity_StrictModeRejectsCrossIdentity(t *testing.T) { + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) // Bot-only account -> strict mode bot (SupportedIdentities bit 2). A real🤖 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/subscription/subscription_test.go` around lines 230 - 237, Isolate the TestFactory configuration in TestResolveEffectiveIdentity_StrictModeRejectsCrossIdentity by setting LARKSUITE_CLI_CONFIG_DIR to a fresh t.TempDir() via t.Setenv before calling cmdutil.TestFactory. Keep the existing factory configuration and test behavior unchanged.Source: Coding guidelines
cmd/event/subscription/update.go-57-58 (1)
57-58: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExample won't parse: pflag boolean flags require
=.
--include-resource-data falsekeepsfalseas a second positional arg, socobra.ExactArgs(1)rejects the command with an arg-count error instead of the intended typed rejection.📝 Proposed fix
- Example: ` lark-cli event subscription update sub_xxx --include-resource-data false`, + Example: ` lark-cli event subscription update sub_xxx --include-resource-data=false`,🤖 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/subscription/update.go` around lines 57 - 58, Update the Example command for the subscription update Cobra command to use equals syntax for the boolean flag, ensuring false is parsed as the flag value and the command still receives exactly one positional argument under Args: cobra.ExactArgs(1).cmd/event/subscription/list.go-146-159 (1)
146-159: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winA negative
--page-sizeis silently dropped.
o.pageSize > 0quietly ignores negative input rather than reporting it. Reject it with a typed validation error inrunListso the user isn't silently given server-default paging.🛡️ Proposed fix (in `runList`, before building the request)
if o.pageSize < 0 { return errs.NewValidationError(errs.SubtypeInvalidArgument, "--page-size must not be negative"). WithParam("page-size"). WithHint("omit --page-size (or pass 0) to use the server default") }As per coding guidelines: "never silently coerce unsupported inputs, ignore unhonored options... Return a typed validation error when a requested behavior cannot be honored."
🤖 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/subscription/list.go` around lines 146 - 159, The runList validation flow must reject negative o.pageSize values before constructing the subscription request. Return the established typed invalid-argument validation error, identifying the page-size parameter and explaining that zero or omission uses the server default; preserve the existing request-building behavior for nonnegative values.Source: Coding guidelines
cmd/event/status.go-39-58 (1)
39-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the help-text grammar and trailing whitespace.
Line 47 ends with a trailing space, and "a missing scope silently fall back" should be "a missing scope silently falls back".
✏️ Proposed fix
SCOPE: the local view (bus in-memory state) needs no scope and is always shown. For a refined consumer, remote_state/expire_time/ include_resource_data are additionally supplemented from a live 'subscription -get' call, but ONLY as a weak, optional dependency: it requires event:subscription:read; -a missing scope silently fall back to the local-only view — this never fails the command. +get' call, but ONLY as a weak, optional dependency: it requires +event:subscription:read; a missing scope silently falls back to the +local-only view — this never fails the command.🤖 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/status.go` around lines 39 - 58, Update the `Long` help text in the event status command to remove the trailing whitespace after `event:subscription:read;` and correct “a missing scope silently fall back” to “a missing scope silently falls back.”cmd/event/subscription/delete_test.go-179-182 (1)
179-182: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winUnchecked type assertion can panic instead of failing the test.
If
local_impactis present butnoteis missing or non-string,localImpact["note"].(string)panics. Use the comma-ok form so the assertion fails with a readable message.🛡️ Proposed fix
- localImpact, ok := generic["local_impact"].(map[string]interface{}) - if !ok || !strings.Contains(localImpact["note"].(string), "not a substitute for stopping local consumers") { + localImpact, ok := generic["local_impact"].(map[string]interface{}) + note, noteOK := localImpact["note"].(string) + if !ok || !noteOK || !strings.Contains(note, "not a substitute for stopping local consumers") { t.Errorf(`"local_impact.note" = %v, want it to mention "not a substitute for stopping local consumers"`, generic["local_impact"]) }🤖 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/subscription/delete_test.go` around lines 179 - 182, Update the local_impact validation in the test to extract localImpact["note"] with a comma-ok string assertion before calling strings.Contains. Treat a missing or non-string note as a test failure and retain the existing readable error message instead of allowing a panic.cmd/event/subscription/get_test.go-124-134 (1)
124-134: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the typed metadata, not just "some typed error".
This error-path test only checks that
errs.ProblemOfsucceeds. PinCategoryandSubtype(invalid_response) so a subtype regression actually fails the test.💚 Proposed fix
- if _, ok := errs.ProblemOf(err); !ok { - t.Fatalf("expected a typed errs.* error, got %T: %v", err, err) - } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected a typed errs.* error, got %T: %v", err, err) + } + if p.Subtype != errs.SubtypeInvalidResponse { + t.Errorf("Subtype = %s, want %s", p.Subtype, errs.SubtypeInvalidResponse) + }As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam)".🤖 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/subscription/get_test.go` around lines 124 - 134, Update TestGetSubscription_SuccessWithNoData_ReturnsTypedInternalError to inspect the metadata returned by errs.ProblemOf(err) and assert Category and Subtype are both "invalid_response"; retain the existing non-nil and typed-error checks.Source: Coding guidelines
internal/event/consume/refined_test.go-58-59 (1)
58-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
gofmtwill reformat this line.
boolPtrhas extra padding before{; gofmt does not align consecutive one-line func declarations, sogofmt -l .will flag this file.As per coding guidelines: "Run
gofmt; Go code must be formatted with nogofmt -l .output."🎨 Formatting fix
func strPtr(s string) *string { return &s } -func boolPtr(b bool) *bool { return &b } +func boolPtr(b bool) *bool { return &b }🤖 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/refined_test.go` around lines 58 - 59, Run gofmt on the helper declarations near strPtr and boolPtr, removing the extra spacing before boolPtr’s opening brace so the file produces no gofmt output.Source: Coding guidelines
internal/event/bus/conn.go-437-469 (1)
437-469: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNo recovery path out of decrypt-degraded state.
Once
decryptFailCountreaches the threshold,degradedReasonstaysdecrypt_failedforever: a later successful decrypt callsSetDecryptState(decryptStateDecrypted), which touches neither the counter nordegradedReason. Three transient failures (e.g. a key rotation window) permanently mark the consumer degraded inevent status. Consider clearing both on a successful decrypt.🛠️ Proposed fix sketch
func (c *Conn) SetDecryptState(state string) { c.identityMu.Lock() defer c.identityMu.Unlock() c.decryptState = state + // A successful decrypt supersedes an earlier persistent-failure + // degradation: reset the streak and clear the decrypt-owned reason. + if state == decryptStateDecrypted { + c.decryptFailCount = 0 + if c.degradedReason == decryptStateFailed { + c.degradedReason = "" + } + } }🤖 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/bus/conn.go` around lines 437 - 469, Update SetDecryptState to restore healthy state when called with decryptStateDecrypted: reset decryptFailCount and clear degradedReason, while preserving the existing state update and locking. Leave failure recording in RecordDecryptFailure unchanged.internal/event/bus/identity.go-43-56 (1)
43-56: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse typed precondition error for missing app config
loadCurrentAppProfileshould return a typed precondition error for the missing app config case instead of a barefmt.Errorf, unless internal config sentinels are intentionally exempt.🤖 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/bus/identity.go` around lines 43 - 56, Update resolveCurrentIdentity’s missing-current-app branch to return the project’s typed precondition error, reusing the established constructor or sentinel rather than fmt.Errorf. Preserve the existing message/context and leave the no-user validation unchanged; only exempt this case if internal config sentinels are explicitly handled that way elsewhere.Source: Coding guidelines
internal/event/bus/lifecycle_test.go-217-223 (1)
217-223: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winPossible flake: this
waitForCalls(..., 1)can consume the first run's token.
startedis buffered, and the first Handle's token was already drained at line 207 — but the merged run's token is the only one left only if timing cooperates; on a slow worker the call at line 218 returns immediately after the queue re-send and the 50ms settle window is all that guards thecallCount() != 2assertion. Waiting explicitly for the second call (waitForCalls(t, action, 1)after release is fine, but assert with a bounded poll to 2) removes the timing dependency.🤖 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/bus/lifecycle_test.go` around lines 217 - 223, Make the post-release assertion in the lifecycle test wait explicitly for the merged second Handle invocation using a bounded poll until action.callCount() reaches 2, rather than relying on the fixed 50ms settle window. Keep the existing release and first-call synchronization, and retain the failure diagnostics from action.allCalls().cmd/event/subscription/reactivate_test.go-83-90 (1)
83-90: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winError-path tests only check that the error is typed, not which typed error it is. Both no-data tests copy the same
errs.ProblemOf(err); !okshape, so a subtype/category regression indoReactivateSubscription/doRenewSubscriptionwould go unnoticed.
cmd/event/subscription/reactivate_test.go#L83-L90: capture the*errs.Problemand assertSubtype == errs.SubtypeInvalidResponse(plus category).cmd/event/subscription/renew_test.go#L82-L89: apply the same assertion for the renew no-data path.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam)".🤖 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/subscription/reactivate_test.go` around lines 83 - 90, The no-data error tests only verify that an error is typed, not its metadata. In cmd/event/subscription/reactivate_test.go lines 83-90, capture the *errs.Problem returned by errs.ProblemOf(err) and assert the expected category, errs.SubtypeInvalidResponse subtype, and parameter; apply the same metadata assertions to the no-data test in cmd/event/subscription/renew_test.go lines 82-89.Source: Coding guidelines
internal/event/source/feishu.go-310-333 (1)
310-333: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
UnionIdis never consulted despite the doc promising a preference order.The comment says "OpenId is preferred over UnionId when a 'user' authority carries both", which implies a fallback; the code only reads
OpenId, so a user authority carrying onlyunion_idnormalizes to the bare"user". That value feeds the hub's authority cross-check, where a bare"user"will not match an owner keyed by open_id, and the event is silently dropped. Either add the fallback or reword the doc to state that a union_id-only authority is deliberately not resolvable here.🐛 Proposed fix (if the fallback is intended)
principalID := "" if a.OpenId != nil { principalID = *a.OpenId + } else if a.UnionId != nil { + principalID = *a.UnionId }🤖 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/source/feishu.go` around lines 310 - 333, Update FormatLifecycleAuthority to use UnionId as the principalID fallback when OpenId is absent, preserving OpenId’s precedence when both identifiers are present. Keep the existing nil handling and formatSubscriptionAuthority normalization unchanged.internal/event/subscription_client_test.go-181-192 (1)
181-192: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the typed metadata, not just "some typed error".
errs.ProblemOf(err)is checked only forokhere (and at Lines 205-207, 268-270, 344-346), so any typed error passes — an unresolved-identity failure classified as, say, a network error would keep this green. AssertCategoryandSubtypeon the returnedProblem(andParamviaerrors.Ason*errs.ValidationErrorwhere applicable), as the neighbouringTestNewSubscriptionClient_UserIdentity_MissingUAT_FailsClosedalready does.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings."💚 Proposed assertion shape
- if _, ok := errs.ProblemOf(err); !ok { - t.Fatalf("expected a typed errs.* error, got %T: %v", err, err) - } + p, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected a typed errs.* error, got %T: %v", err, err) + } + if p.Subtype != errs.SubtypeInvalidArgument { + t.Errorf("Subtype = %s, want %s", p.Subtype, errs.SubtypeInvalidArgument) + }🤖 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/subscription_client_test.go` around lines 181 - 192, Strengthen the error assertions in TestNewSubscriptionClient_UnresolvedIdentity_FailsClosedRatherThanGuessing and the corresponding cases around the other referenced tests. Extract the Problem from errs.ProblemOf(err) and assert the expected Category and Subtype, then use errors.As to validate the expected Param on *errs.ValidationError where applicable, following TestNewSubscriptionClient_UserIdentity_MissingUAT_FailsClosed. Also verify the underlying cause is preserved instead of accepting any typed error.Source: Coding guidelines
🧹 Nitpick comments (22)
internal/event/bus/hub_test.go (1)
128-134: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
mustNotReceive's 50ms window can hide a real delivery.Negative assertions here back the fail-closed drop paths (cross-check, dedup, identity gate). Since
Hub.Publishenqueues synchronously before returning, the wait can be replaced with a non-blocking drain — deterministic and faster than a 50ms sleep per call.♻️ Deterministic negative assertion
func mustNotReceive(t *testing.T, ch chan interface{}, label string) { t.Helper() - if _, _, msg := recvEvent(ch, 50*time.Millisecond); msg != nil { - t.Fatalf("%s: should not have received a message, got %#v", label, msg) - } + // Publish enqueues synchronously, so anything destined for ch is already + // there by the time Publish returns. + select { + case msg := <-ch: + t.Fatalf("%s: should not have received a message, got %#v", label, msg) + default: + } }🤖 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/bus/hub_test.go` around lines 128 - 134, Update mustNotReceive to perform a non-blocking receive check after Hub.Publish has synchronously enqueued any delivery, instead of waiting 50ms through recvEvent. Preserve the existing failure message and assert that no message is immediately available on ch.cmd/event/bus_test.go (1)
74-82: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a cause-preservation assertion (and preserve the cause in the source).
verifyUATBelongsToUserwraps the prove failure asfmt.Errorf("%w: %v", errUATUnverifiable, err)(cmd/event/bus.go line 77), so the originalproveErris flattened into text anderrors.Is(err, proveErr)is false. Diagnosing a real user_info failure then loses the typed/wrapped cause.As per coding guidelines, error-path tests must "verify cause preservation rather than relying only on message substrings."
♻️ Preserve both errors, then assert it
In
cmd/event/bus.go:- return fmt.Errorf("%w: %v", errUATUnverifiable, err) + return fmt.Errorf("%w: %w", errUATUnverifiable, err)In the test case:
{ name: "no stored token + verification failure is rejected (fail-closed)", userOpenID: "ou_alice", token: "tok-unknown", stored: nil, proveErr: errors.New("user_info API returned HTTP 401"), wantErr: true, wantSentinel: errUATUnverifiable, + wantCause: true, // assert errors.Is(err, tc.proveErr) },🤖 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/bus_test.go` around lines 74 - 82, Update verifyUATBelongsToUser so failures from the user-info verification preserve both errUATUnverifiable and the original proveErr as wrapped causes rather than formatting proveErr into plain text. Extend the “no stored token + verification failure is rejected (fail-closed)” test to assert errors.Is(err, proveErr) in addition to the existing sentinel assertion.Source: Coding guidelines
cmd/event/subscription/subscription_test.go (1)
365-375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDead
continueaftert.Fatalf, and a missing subcommand aborts the whole loop.
t.Fatalfnever returns, so thecontinueis unreachable; usingt.Errorf+continuealso lets the second subcommand still be checked.♻️ Proposed refactor
sub, ok := found[name] if !ok { - t.Fatalf("subscription command group missing %q subcommand", name) + t.Errorf("subscription command group missing %q subcommand", name) continue }🤖 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/subscription/subscription_test.go` around lines 365 - 375, In the subcommand validation loop, remove the unreachable continue after t.Fatalf and change the missing-subcommand handling so the test records an error without aborting the loop, allowing both “list” and “get” to be checked. Preserve the existing risk validation for found subcommands.cmd/event/subscription/create.go (1)
274-282: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReturning the shared package slice on the false path leaves an aliasing hazard.
createRequiredScopes(false)hands backsubscriptionMutationScopesitself, and that value is embedded intocreateDryRunResult.RequiredScopes. A futureappendon the result field would write into the shared backing array — the exact bug thetruebranch already defends against. Returning a copy on both paths makes the guarantee unconditional.♻️ Make both paths allocate
func createRequiredScopes(includeResourceData bool) []string { - if !includeResourceData { - return subscriptionMutationScopes - } scopes := make([]string, 0, len(subscriptionMutationScopes)+len(subscriptionEncryptKeyReadScopes)) scopes = append(scopes, subscriptionMutationScopes...) + if !includeResourceData { + return scopes + } scopes = append(scopes, subscriptionEncryptKeyReadScopes...) return scopes }🤖 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/subscription/create.go` around lines 274 - 282, Update createRequiredScopes so the !includeResourceData path returns a newly allocated copy of subscriptionMutationScopes rather than the shared package slice. Preserve the existing true-path combination and ensure both branches return independently owned slices, preventing later append operations on RequiredScopes from mutating shared backing storage.internal/event/consume/refined_test.go (1)
1359-1361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo config fixtures write with
os.WriteFileinside aninternal/test. Both sites bypass the repository's filesystem abstraction; the shared fix is to route them throughinternal/vfs.
internal/event/consume/refined_test.go#L1359-L1361: replaceos.WriteFile(filepath.Join(dir, "config.json"), ...)withvfs.WriteFile(...)and drop the now-unusedosimport if nothing else needs it.internal/event/consume/refined_test.go#L1435-L1437: apply the samevfs.WriteFilereplacement insiderunOnce.As per coding guidelines: "Use
internal/vfsfilesystem APIs instead ofosfilesystem APIs." Based on learnings: for any Go test file underinternal/, useinternal/vfsincluding fixture/setup helpers.🤖 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/refined_test.go` around lines 1359 - 1361, Replace both os.WriteFile calls in internal/event/consume/refined_test.go at lines 1359-1361 and 1435-1437, including the runOnce helper, with vfs.WriteFile using the existing path and contents; remove the os import if no other references remain.Sources: Coding guidelines, Learnings
internal/event/reconcile_test.go (1)
554-566: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the typed metadata on this fail-closed error.
The test only checks
err != nil, so it would still pass if the fail-closed branch degraded to an untyped error. Assertingerrs.ProblemOf(err)'s category/subtype (SubtypeUnknown, internal) locks the contract.As per coding guidelines: "Error-path tests must assert typed metadata through
errs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings."🤖 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/reconcile_test.go` around lines 554 - 566, Update TestReconcileExisting_EncryptedBothTrue_NoProberSupplied_FailsClosed to inspect errs.ProblemOf(err) and assert the fail-closed error’s internal category, SubtypeUnknown subtype, and expected param metadata; also verify the underlying cause is preserved. Keep the existing non-nil and no-reuse assertions.Source: Coding guidelines
internal/event/consume/refined.go (1)
623-637: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse typed error constructors instead of bare
fmt.Errorfhere.
resolveCurrentProfileIdentityreturns a rawfmt.Errorffor the "no current app config" case, and the caller at Line 182 wraps it with anotherfmt.Errorf. Both eventually land inside a typederrApplyOkHelloFailed, but the inner values carry no category/subtype of their own. Aerrs.NewValidationError(errs.SubtypeFailedPrecondition, ...)(missing profile) and.WithCause(err)on the caller side keeps the classification intact.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/consume/refined.go` around lines 623 - 637, Update resolveCurrentProfileIdentity to return errs.NewValidationError(errs.SubtypeFailedPrecondition, ...) when CurrentAppConfig returns nil instead of using fmt.Errorf. At the caller around the existing error wrapping, preserve the typed classification by attaching the returned error with .WithCause(err) rather than embedding it in another fmt.Errorf.Source: Coding guidelines
internal/event/reconcile.go (1)
98-109: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value"Mutually exclusive" options aren't actually enforced.
WithDeferredEncryptKeyConfirmation's doc says it is mutually exclusive withWithEncryptKeyProber, but supplying both silently takes the prober branch. Given the whole block is fail-closed by design, rejecting the ambiguous combination the same way thedefaultcase does would keep behavior and documentation aligned.Also applies to: 237-257
🤖 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/reconcile.go` around lines 98 - 109, The reconcile configuration must reject supplying both WithDeferredEncryptKeyConfirmation and WithEncryptKeyProber instead of silently selecting the prober path. Update the option validation or requestedIncludeResourceData=true handling in ReconcileExisting to detect the conflicting flags and route it through the existing fail-closed internal-error/default behavior.cmd/event/subscription/list_test.go (1)
176-190: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
errors.Isover message-string equality for the pass-through assertion.A sentinel makes the "unchanged, unwrapped" contract explicit and survives message rewording.
♻️ Proposed refactor
func TestListSubscriptions_TransportError_PropagatesTyped(t *testing.T) { - fake := &fakeListAPI{err: errors.New("boom: connection reset")} + wantErr := errors.New("boom: connection reset") + fake := &fakeListAPI{err: wantErr} _, err := listSubscriptions(context.Background(), fake, listOpts{}) if err == nil { t.Fatal("expected an error, got nil") } - if err.Error() != "boom: connection reset" { + if !errors.Is(err, wantErr) { // listSubscriptions must pass the error through unchanged — it is // *eventlib.SubscriptionClient's job (already tested) to // wrap it into a typed error; this command layer must not swallow // or double-wrap it. t.Errorf("err = %v, want it passed through unchanged", err) } }🤖 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/subscription/list_test.go` around lines 176 - 190, Update TestListSubscriptions_TransportError_PropagatesTyped to use a sentinel error for fakeListAPI and assert the returned error with errors.Is instead of comparing err.Error() to a message string, preserving the pass-through and unwrapped contract.internal/event/bus/identity_test.go (1)
376-381: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueFake panics with index-out-of-range if
resolveCurrentis ever called more than twice.A future change to the gate (e.g. an extra resolve) would fail as a panic rather than a readable assertion. Clamping to the last entry keeps the failure mode legible.
♻️ Proposed refactor
resolveCurrent := func() (currentIdentity, error) { - id := identities[resolveCalls] + idx := resolveCalls + if idx >= len(identities) { + idx = len(identities) - 1 + } + id := identities[idx] resolveCalls++ return id, 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/bus/identity_test.go` around lines 376 - 381, Update the resolveCurrent test helper to clamp resolveCalls to the final identities entry when resolving beyond the available entries, while preserving sequential selection for the initial calls. Keep the existing call-count tracking and returned identity behavior intact, but prevent index-out-of-range panics so later assertions report the actual failure.internal/event/protocol/messages_test.go (1)
195-236: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing the omitempty counterpart for
Event's v2 fields.
HelloandConsumerInfoboth get a "zero-valued v2 field must not reach the wire" test;Eventonly gets round-trip + old-frame decode. A legacy consumer parsing strictly would benefit from the same pin.💚 Proposed test
func TestEvent_V2FieldsOmittedWhenZero(t *testing.T) { e := NewEvent("im.message.receive_v1", "e1", "111", 1, json.RawMessage(`{}`)) data, err := json.Marshal(e) if err != nil { t.Fatalf("marshal: %v", err) } for _, key := range []string{ `"remote_subscription_id"`, `"target_resource"`, `"authority"`, `"subscription_event_id"`, } { if bytes.Contains(data, []byte(key)) { t.Errorf("zero-valued v2 Event field leaked onto wire: %s in %s", key, data) } } }🤖 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/protocol/messages_test.go` around lines 195 - 236, Add a TestEvent_V2FieldsOmittedWhenZero test alongside TestEvent_V2FieldsRoundTrip that marshals an Event created with NewEvent and verifies the zero-valued v2 fields remote_subscription_id, target_resource, authority, and subscription_event_id are absent from the JSON wire data. Use the existing event constructors and encoding conventions, and fail with the field name and serialized data when any key is present.internal/event/bus/conn.go (1)
258-268: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoff
degradedReasonis a single slot written by three independent producers.The identity gate (
SetBoundConnID/SetDegraded), the lifecycle action (ClearActionDegraded), and the decrypt path all write the same field, so a successful rebind silently erases a decrypt/lifecycle degradation (and vice versa) even though the underlying condition still holds. If status accuracy matters here, consider per-domain reason slots with a derived rollup.Also applies to: 297-305, 459-469
🤖 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/bus/conn.go` around lines 258 - 268, Replace the shared degradedReason slot in Conn with separate reason fields for identity, lifecycle action, and decrypt domains. Update SetBoundConnID, SetDegraded, ClearActionDegraded, and the decrypt-path writers to modify only their respective slots, then derive the exposed degradation reason from the active domain reasons so clearing or rebinding one condition does not erase others.internal/event/bus/conn_test.go (1)
333-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOrphaned doc comment: it documents
TestConn_IdentityGateState_ConcurrentAccessRace(defined at Line 570) but sits above the Task-17 section header.Move it down to the function it describes so the section banner reads cleanly.
♻️ Proposed fix
-// TestConn_IdentityGateState_ConcurrentAccessRace exercises every new -// identity-gate mutator/getter concurrently: in production this state is -// written from TWO independent goroutines (identity.go's onConnReady, driven -// by the WS ready/reconnect callback, and Hub.Publish's per-event delivery -// gate, driven by the source's emit goroutine) and read by a future status -// command from yet another — hence its own dedicated mutex rather than the -// zero-lock convention used for the write-once owner fields above. Run with -// -race (mirrors hub_publish_race_test.go's style for the Hub side). // --- Task 17: lifecycle summary state (spec §5.1/§5.5) --------------------and re-attach it directly above
func TestConn_IdentityGateState_ConcurrentAccessRace(Line 570).🤖 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/bus/conn_test.go` around lines 333 - 343, Move the multi-line documentation comment describing TestConn_IdentityGateState_ConcurrentAccessRace from above the Task-17 lifecycle section to directly above that test function. Leave the Task-17 section header and TestConn_LifecycleSummary_DefaultEmpty declaration together without the orphaned comment.internal/event/bus/hub_consumers_refined_test.go (1)
20-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDiscarded
net.Pipe()peers across the new Hub tests. Both files useconn, _ := net.Pipe()and close only one end, unlike the rest of the package's Hub/Conn tests which keep and close both.
internal/event/bus/hub_consumers_refined_test.go#L20-L21: bind the second return value anddeferitsClose()in all three tests.internal/event/bus/hub_lifecycle_test.go#L19-L20: same for everynet.Pipe()call site in this file (Lines 19, 25, 31, 64, 94, 119, 142, 176).🤖 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/bus/hub_consumers_refined_test.go` around lines 20 - 21, The Hub tests discard one peer returned by net.Pipe(), leaving resources unclosed. In internal/event/bus/hub_consumers_refined_test.go lines 20-21, update all three tests to retain and defer-close both pipe connections; apply the same change at every net.Pipe() call site in internal/event/bus/hub_lifecycle_test.go lines 19, 25, 31, 64, 94, 119, 142, and 176.internal/event/bus/lifecycle_dispatch_test.go (1)
1104-1111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPin the expected outcome instead of accepting both.
The
err == nil → t.Logbranch makes this assertion vacuous: any behavior passes. Decide the contract for "no subscription client configured" (typed error vs. nil) and assert it directly, keeping the summary-recording 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/bus/lifecycle_dispatch_test.go` around lines 1104 - 1111, The test around action.Handle must assert the defined contract for a missing subscription client instead of accepting either outcome. Update the err == nil branch to require the expected typed error or nil result, while preserving the LastLifecycleEvent assertion that verifies the summary is recorded.internal/event/bus/lifecycle_ports.go (1)
56-58: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePrefer a checked type assertion here.
The invariant holds today, but an unchecked assertion in a lifecycle worker goroutine turns any future non-
*Connimplementation into a daemon-killing panic; returning an error degrades just that consumer instead.♻️ Proposed change
func (g *identityGate) BindConsumer(ctx context.Context, c lifecycle.Conn) error { - return g.bindConsumer(ctx, c.(*Conn)) + conn, ok := c.(*Conn) + if !ok { + return fmt.Errorf("bind consumer: unexpected lifecycle.Conn type %T", c) + } + return g.bindConsumer(ctx, conn) }🤖 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/bus/lifecycle_ports.go` around lines 56 - 58, Update identityGate.BindConsumer to use a checked type assertion when converting the lifecycle.Conn argument to *Conn. If the assertion fails, return an appropriate error instead of panicking; otherwise preserve the existing bindConsumer flow.internal/event/bus/lifecycle/action.go (1)
187-208: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueUnsynchronized post-construction wiring read from the executor goroutine.
gate/newSubClient/encryptKeyRemoverare plain fields written by these setters and read byHandleon the executor's worker goroutine. Today all wiring happens inNewBus/SetIdentityProviders/SetSubscriptionClientbeforeRunstarts, so it's safe — but nothing enforces it, andHub.SetCurrentResolverguards the equivalent state withmu(internal/event/bus/hub.goLines 131-139). Consideratomic.Pointer/mutex, or at least a doc line stating these must be wired before the executor starts.🤖 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/bus/lifecycle/action.go` around lines 187 - 208, Synchronize access to the wiring fields used by the executor: gate, newSubClient, and encryptKeyRemover in SubscriptionAction, including reads from Handle and writes through SetIdentityGate, SetNewSubscriptionClient, and SetEncryptKeyRemover. Prefer the existing synchronization approach used by Hub.SetCurrentResolver, or document and enforce that all wiring completes before the executor starts if synchronization is intentionally not added.internal/event/bus/encrypt_key.go (2)
29-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc contradicts the declaration.
The comment says "A field on the provider (not a const)", but this is a
const. The tunable field isencryptKeyProvider.fetchTimeout; this const is only its default.♻️ Proposed comment fix
// defaultEncryptKeyFetchTimeout bounds the ONE Hello-time GetEncryptKey call. -// A field on the provider (not a const) so tests can shrink it; deliberately -// not env-configurable — this is control-plane housekeeping, run once per -// encrypted consumer registration, never a hot path. +// It only seeds encryptKeyProvider.fetchTimeout, which tests can shrink; +// deliberately not env-configurable — this is control-plane housekeeping, run +// once per encrypted consumer registration, never a hot path.🤖 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/bus/encrypt_key.go` around lines 29 - 33, Correct the comment above defaultEncryptKeyFetchTimeout to describe it as the default value for the provider’s encryptKeyProvider.fetchTimeout field, not as a provider field itself. Preserve the existing timeout, test-tuning, environment-configuration, and control-plane context.
100-106: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
gateis written unguarded whilenewClientis mutex-guarded.
setIdentityGatemutatesp.gatewith no lock, yetresolveOwnerIdentityreads it on thefetchAndSetpath — the exact concurrency windownewClientMuexists to cover fornewClient. Production wires both beforeRun(), so this is latent rather than live, but the asymmetry invites a race under-raceif a test (or future code) sets the gate concurrently. Either guard both or document that onlynewClientcan be set late.🔒 Proposed fix
- newClientMu sync.RWMutex - newClient func(as core.Identity, uat string) (encryptKeyClient, error) + // mu guards the post-construction wiring below (gate + newClient), which + // may be set slightly before Run() while a test drives fetchAndSet. + mu sync.RWMutex + gate *identityGate + newClient func(as core.Identity, uat string) (encryptKeyClient, error)func (p *encryptKeyProvider) setIdentityGate(g *identityGate) { p.mu.Lock() p.gate = g p.mu.Unlock() }🤖 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/bus/encrypt_key.go` around lines 100 - 106, Protect the gate assignment in setIdentityGate with the same synchronization used when resolveOwnerIdentity reads p.gate on the fetchAndSet path. Add or reuse the provider mutex around both the write and corresponding read, preserving the existing newClientMu handling separately.internal/event/bus/encrypt_key_test.go (1)
350-360: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winClassification table misses
errEncryptKeyBotUnsupported.
encryptKeyFailureClasshas aresource_data_requires_userbranch that no case covers, so a rename/removal of that token would go unnoticed.💚 Proposed test addition
{errEncryptKeyOwnerMismatch, "owner_mismatch"}, + {errEncryptKeyBotUnsupported, "resource_data_requires_user"}, {errEncryptKeyNoGate, "no_identity_gate"},🤖 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/bus/encrypt_key_test.go` around lines 350 - 360, Add errEncryptKeyBotUnsupported with the expected "resource_data_requires_user" classification to the cases table covering encryptKeyFailureClass. Keep the existing classifications unchanged so the test explicitly exercises every branch, including the resource-data-requires-user outcome.internal/event/registry_test.go (1)
314-332: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOnly one of the four new panic branches is covered.
validateRefinedSubscriptionalso panics on emptyPathSegment, emptySelectorKey, and out-of-setAuthTypes; none of those are asserted, so removing any of them would keep this test green.💚 Proposed additional cases
for _, tc := range []struct { name string tmpl KeyTemplate }{ {"empty path segment", KeyTemplate{SelectorKey: "z_id"}}, {"empty selector key", KeyTemplate{PathSegment: "z-id"}}, {"bad auth type", KeyTemplate{SelectorKey: "z_id", PathSegment: "z-id", AuthTypes: []string{"nope"}}}, } { t.Run(tc.name, func(t *testing.T) { def := KeyDefinition{Key: "x.y.bad_v1", EventType: "x.y.bad_v1", ResourceType: "x.y", Schema: nativeSchema(), RefinedSubscription: true, KeyTemplates: []KeyTemplate{tc.tmpl}} assertPanics(t, func() { RegisterKey(def) }) }) }🤖 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/registry_test.go` around lines 314 - 332, Expand TestRegisterKey_RefinedValidation to assert panics for the remaining validateRefinedSubscription branches: a template with an empty PathSegment, one with an empty SelectorKey, and one containing an out-of-set AuthTypes value. Add separate test cases using invalid refined KeyDefinitions and RegisterKey, while preserving the existing missing-template and valid-registration checks.internal/event/registry.go (1)
110-133: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick win
ResourceTypeis required for a refined key but not validated.
internal/event/resolve.goLine 143 buildsTargetResource: baseDef.ResourceType + "?" + tmpl.SelectorKey + "=" + .... A refined key registered withoutResourceTypetherefore yields a malformed"?chat_id=oc_x"that flows straight into subscription create/update, instead of failing loudly at registration like the other refined-key contract violations here.Template/Examplehave the same "declared required, silently optional" shape —Exampleis what the empty-value hint at resolve.go Line 133 renders.🛡️ Proposed validation
if len(def.KeyTemplates) == 0 { panic(fmt.Sprintf("EventKey %s: RefinedSubscription requires non-empty KeyTemplates", def.Key)) } + if def.ResourceType == "" { + panic(fmt.Sprintf("EventKey %s: RefinedSubscription requires a non-empty ResourceType", def.Key)) + } for i, tmpl := range def.KeyTemplates { + if tmpl.Example == "" { + panic(fmt.Sprintf("EventKey %s: KeyTemplates[%d] Example must not be empty", def.Key, i)) + } if tmpl.PathSegment == "" {🤖 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/registry.go` around lines 110 - 133, The validateRefinedSubscription function must also reject refined keys with an empty ResourceType, Template, or Example. Add registration-time validation for these required fields, using the existing def.Key context and consistent panic behavior so malformed refined definitions fail before resolution.
| @@ -0,0 +1,71 @@ | |||
| package refined | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔴 Critical | ⚡ Quick win
Missing license header — this is the CI license-header failure.
Every other new file in this PR carries the standard two-line header.
🐛 Proposed fix
+// Copyright (c) 2026 Lark Technologies Pte. Ltd.
+// SPDX-License-Identifier: MIT
+
package refined📝 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.
| package refined | |
| // Copyright (c) 2026 Lark Technologies Pte. Ltd. | |
| // SPDX-License-Identifier: MIT | |
| package refined |
🧰 Tools
🪛 GitHub Actions: CI / 2064 / 12_license-header.txt
[error] 1-1: license-eye header check failed: file does not have a valid license header.
🪛 GitHub Actions: CI / 2064 / license-header
[error] 1-1: license-eye header check failed: file does not have a valid license header.
🤖 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/refined/catalog_test.go` at line 1, Add the repository’s standard
two-line license header at the top of catalog_test.go, before the package
refined declaration, matching the format used by other new files in the pull
request.
Source: Pipeline failures
| func (p *CredentialProvider) VerifyUATOpenID(ctx context.Context, brand core.LarkBrand, uat string) (string, error) { | ||
| if p == nil || p.httpClient == nil { | ||
| return "", fmt.Errorf("credential: no HTTP client configured to verify a user access token") | ||
| } | ||
| hc, err := p.httpClient() | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| info, err := fetchUserInfo(ctx, hc, brand, uat) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| return info.OpenID, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Does the credential provider's httpClient factory set a Timeout?
rg -nP -C6 'httpClient\s*(:?=|func\(\))' internal/credential | rg -n -C6 'Timeout|http\.Client'
ast-grep run --pattern '&http.Client{$$$}' --lang go internal/credentialRepository: larksuite/cli
Length of output: 4963
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate usages of NewCredentialProvider / NewDefaultTokenProvider =="
rg -n "NewCredentialProvider|NewDefaultTokenProvider" --glob '*.go' .
echo
echo "== credential_provider relevant sections =="
sed -n '1,80p' internal/credential/http_client.go 2>/dev/null || true
sed -n '120,170p' internal/credential/credential_provider.go
sed -n '100,135p' internal/credential/default_provider.go
echo
echo "== ResolveUAT callers and timeout/ctx usages around bus =="
rg -n -C8 "ResolveUAT|verifyUATBelongsToUser|VerifyUATOpenID|handleHello|bindConsumer|context.Background" internal/event internal/credential --glob '*.go'
echo
echo "== client creation sites outside tests =="
rg -n -C5 'http.Client\{|Timeout:\s*time\.' --glob '*.go' --glob '!**/*_test.go' . | sed -n '1,220p'Repository: larksuite/cli
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== tracked files under internal/credential =="
git ls-files internal/credential | sed -n '1,120p'
echo
echo "== NewCredentialProvider/NewDefaultTokenProvider exact matches =="
rg -n -C4 'NewCredentialProvider\(|NewDefaultTokenProvider\(' --glob '*.go' .
echo
echo "== credential http client / request factory files and timeout mentions =="
git ls-files internal/credential | xargs rg -n -C4 'http\.Client|Timeout|user_info|openid|OpenID\(|VerifyUATOpenID|ResolveToken' || true
echo
echo "== identity gate resolveUAT implementation around live UAT resolution =="
rg -n -C12 'func .*ResolveUAT|verifyUATBelongsToUser|VerifyUATOpenID|fetchUserInfo\(' internal/event internal/credential --glob '*.go'Repository: larksuite/cli
Length of output: 50370
Don’t rely on the caller’s context to bound UAT verification.
VerifyUATOpenID makes a live HTTP round-trip over ctx; internal/cmdutil/factory_default.go wires the credential provider through deps.HttpClient, and deps.HttpClient is not set by the existing Client construction paths with a guaranteed Timeout. When a context.Background() Hello path reaches this call, a hung user_info request can block that goroutine/pipe until the socket closes. Add an explicit context.WithTimeout around the HTTP call, or establish a guaranteed default http.Client.Timeout at the injection point.
🤖 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/credential/user_info.go` around lines 64 - 77, The VerifyUATOpenID
flow must impose its own timeout instead of relying solely on the caller’s
context. Add an explicit bounded context around the fetchUserInfo call, or
guarantee a nonzero http.Client.Timeout where deps.HttpClient is injected, while
preserving the existing error propagation and OpenID return behavior.
| // subscriptionDecryptFailureRe matches the SDK's stable whole-envelope | ||
| // decrypt-failure error (event/dispatcher/dispatcher.go's | ||
| // decryptSubscriptionEnvelope: "subscription event decryption failed | ||
| // (subscription_id=%s): ...") and captures the subscription_id. This couples to | ||
| // the pinned SDK's error string — the only surface the WS path exposes for a | ||
| // per-message decrypt failure (Do returns the error; the ws client logs it via | ||
| // this logger). The capture stops at ')' so it never swallows the ": <detail>" | ||
| // tail (which carries no key, but also no useful routing info). | ||
| var subscriptionDecryptFailureRe = regexp.MustCompile(`subscription event decryption failed \(subscription_id=([^)]*)\)`) | ||
|
|
||
| // decryptFailureLogClass is the fixed classification used when a decrypt-failure | ||
| // SDK error line is rewritten before it reaches bus.log. The log must not carry | ||
| // the crypto/padding detail in the SDK's raw error tail, e.g. "illegal base64 | ||
| // data", "cipher too short", or "ciphertext is not a multiple of the block | ||
| // size", because those details could become a decryption oracle. The token | ||
| // mirrors Conn.decryptStateFailed's "decrypt_failed" state. | ||
| const decryptFailureLogClass = "decrypt_failed" | ||
|
|
||
| // redactDecryptFailureLine detects a decrypt-failure SDK Error line | ||
| // (subscriptionDecryptFailureRe) and truncates it right after the | ||
| // "(subscription_id=...)" it already carries, discarding everything from | ||
| // there to the end of the line — event/dispatcher/dispatcher.go's | ||
| // decryptSubscriptionEnvelope always appends the raw crypto/padding detail as | ||
| // the LAST component of the wrapped ws-client log line ("...: <detail>"), so | ||
| // this is the ONLY place that detail appears on this path — and replacing it | ||
| // with a fixed classification instead. Any harmless prefix context the ws | ||
| // client adds (message_type/message_id/trace_id, etc.) is kept verbatim. A | ||
| // line that is not a decrypt failure is returned byte-for-byte unchanged. | ||
| func redactDecryptFailureLine(msg string) string { | ||
| loc := subscriptionDecryptFailureRe.FindStringIndex(msg) | ||
| if loc == nil { | ||
| return msg | ||
| } | ||
| return msg[:loc[1]] + ": classification=" + decryptFailureLogClass | ||
| } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
oapi-sdk-go v3 subscription event decryption failed error message dispatcher
💡 Result:
The error message indicating a decryption failure in the Lark (Feishu) OpenAPI SDK for Go (v3) when handling event subscriptions is typically caused by a mismatch or incorrect configuration of the security credentials used by the EventDispatcher [1][2]. When you initialize the EventDispatcher in the Go SDK, you must provide the correct Verification Token and Event Encrypt Key if encryption is enabled in the Lark Developer Console [1][3][4]. If these values are missing, incorrect, or if encryption is enabled on the platform but the SDK is not configured to handle it, decryption will fail [1][2][5]. To resolve this issue, follow these steps: 1. Verify Encryption Settings: Go to the Lark Developer Console, navigate to your app, and check the Events and Callbacks > Encryption Strategy section [1][6][2]. Confirm whether Encryption is enabled [4]. 2. Update Dispatcher Initialization: When creating your EventDispatcher, ensure you are passing the exact Verification Token and Event Encrypt Key from the Developer Console [1][3][4]. Example configuration: handler:= dispatcher.NewEventDispatcher("your_verification_token", "your_event_encrypt_key") If encryption is enabled on the platform, passing empty strings or incorrect values will cause decryption errors [1][3][4]. 3. Check SDK Middleware: Ensure the http.HandleFunc used for receiving events is correctly wrapped with the handler using httpserverext.NewEventHandlerFunc [1][7][3]. This middleware handles the decryption and verification logic automatically [7][2]. 4. Debugging: If the error persists, you can enable debug logs by passing an option to the event handler to see more granular error details, which can help confirm if the issue is related to signature verification or the decryption process itself [3][8]: http.HandleFunc("/webhook/event", httpserverext.NewEventHandlerFunc(handler, larkevent.WithLogLevel(larkcore.LogLevelDebug))) If you have confirmed your keys are correct and are still encountering issues, verify that your server is correctly receiving the full, uncorrupted request body from the Lark platform, as any modification to the payload will cause signature and decryption verification to fail [2][5][9].
Citations:
- 1: https://open.larkoffice.com/document/server-side-sdk/golang-sdk-guide/handle-events
- 2: https://open.larksuite.com/document/server-docs/event-subscription/event-subscription-configure-/encrypt-key-encryption-configuration-case
- 3: https://github.com/larksuite/oapi-sdk-go/blob/1eeed3c60952f18272e9be8b03a831dea7ea6341/README.md
- 4: https://open.feishu.cn/document/server-side-sdk/golang-sdk-guide/handle-events
- 5: https://open.larksuite.com/document/ukTMukTMukTM/uYDNxYjL2QTM24iN0EjN/event-subscription-configure-/encrypt-key-encryption-configuration-case
- 6: https://wenjcsthqreward.ltgamesglobal.com/document/uAjLw4CM/ukTMukTMukTM/server-side-sdk/golang-sdk-guide/handle-events
- 7: https://context7.com/larksuite/oapi-sdk-go
- 8: https://github.com/larksuite/oapi-sdk-go/blob/v3_main/sample/event/event.go
- 9: https://open.larksuite.com/document/ukTMukTMukTM/uYDNxYjL2QTM24iN0EjN/event-subscription-configure-/choose-a-subscription-mode/send-notifications-to-developers-server
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
fd -i 'go.mod|go.sum|feishu.go' . | sed 's#^\./##' | sort
echo
echo "Relevant occurrences:"
rg -n "subscriptionDecryptFailureRe|decryptSubscriptionEnvelope|redactDecryptFailureLine|DecryptFailure|decrypt_failed|oapi-sdk-go|larksuite/oapi" -S .
echo
echo "Module versions:"
if [ -f go.mod ]; then awk '/oapi-sdk-go|larksuite\/oapi|github.com\/larksuite\/oapi-sdk-go|go 1\./{print}' go.mod; fi
if [ -f go.sum ]; then rg 'oapi-sdk-go|larksuite/oapi' go.sum | head -20; fiRepository: larksuite/cli
Length of output: 25442
🌐 Web query:
site:github.com/larksuite/oapi-sdk-go "subscription event decryption failed" v3.7.2 event dispatcher
💡 Result:
The error "subscription event decryption failed" (or related decryption/unmarshal errors) when using the Lark (Feishu) Open Platform Go SDK typically indicates a mismatch between the configuration in your application code and the settings in the Lark Developer Console [1]. To resolve this issue, ensure the following steps are taken: 1. Verify Encryption Configuration: In the Lark Developer Console under "Event Subscriptions," if "Encrypt Key" (Event Encrypt Key) is enabled, it must be correctly provided when initializing the EventDispatcher [1]. If the keys in your code do not match the keys configured in the developer console, decryption will fail [1]. Initialize your dispatcher as follows: dispatcher.NewEventDispatcher("your_verification_token", "your_event_encrypt_key") 2. Verify Token and Keys: Ensure both the "Verification Token" and "Event Encrypt Key" are copied exactly from the "Event Subscriptions" or "Credentials & Basic Info" section of your app in the developer console [1]. Even trailing or leading whitespace in these configuration strings can cause failures. 3. Check Event Protocol Version: Lark uses different event protocols (e.g., v1.0 vs v2.0). Ensure you are registering the correct event handler type (e.g., OnP1... for v1.0, OnP2... for v2.0) [1]. Using the wrong registration method can lead to unexpected unmarshaling errors or failure to process the decrypted payload [1]. 4. Inspect Request/Response Body: If you are getting JSON unmarshal errors (such as "invalid character"), inspect the raw body of the incoming request [2]. This often happens if the encryption key is incorrect (causing the SDK to attempt to decrypt garbage data) or if the server is receiving an unexpected format [2]. You can use the logger provided by the SDK to debug the incoming request: larkevent.WithLogLevel(larkcore.LogLevelDebug) [3][1] If the issue persists, ensure your SDK version is up to date, as issues regarding specific data type unmarshaling (e.g., string vs number for timestamps) have been addressed in later versions [4].
Citations:
- 1: https://github.com/larksuite/oapi-sdk-go/blob/1eeed3c60952f18272e9be8b03a831dea7ea6341/README.md
- 2: err: event json unmarshal, err: invalid character 'c' looking for beginning of value oapi-sdk-go#140
- 3: https://github.com/larksuite/oapi-sdk-go/blob/e5d4140690191a57247c0d5de35b57de140199e5/sample/event/event.go
- 4: 机器人自定义菜单事件 P2BotMenuV6Data.event.timestamp 为字符串，但是encrypt解析后为数字 oapi-sdk-go#97
🌐 Web query:
github.com larksuite oapi-sdk-go event dispatcher decryptSubscriptionEnvelope "subscription event decryption failed"
💡 Result:
The error "decryptSubscriptionEnvelope subscription event decryption failed" typically indicates a mismatch or misconfiguration in the event encryption settings when using the Lark/Feishu Open Platform SDK for Go [1][2][3]. When the Lark Open Platform pushes events with encryption enabled, the SDK must use the correct Encryption Key (AES Key) to decrypt the payload [4]. If decryption fails, it is almost always due to one of the following reasons: 1. Incorrect Encryption Key: Ensure the "Encrypt Key" configured in your application's "Event Subscriptions" page on the Lark/Feishu Developer Console matches exactly the key being passed to the SDK's dispatcher [4]. 2. Mismatch in Encryption Status: If you have enabled encryption in the Developer Console but are attempting to handle events in the SDK without providing the proper key, or vice-versa, decryption will fail [4]. 3. Handler Configuration: The EventDispatcher must be initialized with the correct verification token and encryption key if your app requires them [5][6]. If you are manually handling events (e.g., using OnCustomizedEvent), ensure you are passing the correct cipher data to the decryption methods [7]. Recommended troubleshooting steps: - Verify Encryption Settings: Go to the Lark/Feishu Developer Console, navigate to your app, and check the "Events & Callbacks" (or "Event Subscription") section. Confirm whether "Encrypt Key" is enabled and match that value against your code [5][4]. - Check SDK Initialization: When creating your event dispatcher, ensure you are passing the correct credentials [6]. For example: dispatcher.NewEventDispatcher("VERIFICATION_TOKEN", "ENCRYPTION_KEY") - Use Debug Logging: Enable debug logging in the SDK to inspect the incoming payload and the error details [5][6]. This will help determine if the payload itself is malformed or if the decryption process is rejecting it due to key mismatch. larkcore.WithLogLevel(larkcore.LogLevelDebug) If you are using the OnCustomizedEvent handler, ensure you are correctly parsing the event and calling handler.DecryptEvent(ctx, cipherEventJsonStr) as demonstrated in the official SDK samples [7].
Citations:
- 1: https://pkg.go.dev/github.com/larksuite/oapi-sdk-go/v3@v3.9.3/event
- 2: https://github.com/larksuite/oapi-sdk-go
- 3: https://open.larksuite.com/document/ukTMukTMukTM/uETO1YjLxkTN24SM5UjN
- 4: https://www.npmjs.com/package/@larksuiteoapi/node-sdk
- 5: https://open.larksuite.com/document/ukTMukTMukTM/uYDNxYjL2QTM24iN0EjN/event-subscription-configure-/use-websocket
- 6: https://context7.com/larksuite/oapi-sdk-go
- 7: https://github.com/larksuite/oapi-sdk-go/blob/v3_main/sample/event/event.go
🌐 Web query:
raw GitHub github.com larksuite oapi-sdk-go event dispatcher dispatcher.go decryptSubscriptionEnvelope subscription event decryption failed
💡 Result:
If you encounter a subscription event decryption failure when using the Lark/Feishu Go SDK (larksuite/oapi-sdk-go), it typically indicates a mismatch between the encryption configuration in your application's settings and the values provided to the SDK's EventDispatcher [1][2][3]. Key troubleshooting steps include: 1. Verify Encryption Configuration: Ensure that the Encrypt Key (or Encryption Key) used in your code matches exactly with the one configured in the Lark Developer Console under "Event & Callback > Encryption Strategy" [1][2][3]. 2. Verify Dispatcher Initialization: When initializing the EventDispatcher, both the Verification Token and Encrypt Key must be passed correctly if encryption is enabled in the developer console [2][3]. Example initialization: handler:= dispatcher.NewEventDispatcher("your_verification_token", "your_encrypt_key") 3. Check Request Data: Ensure the raw request body is being passed correctly to the dispatcher. Decryption will fail if the payload is malformed, truncated, or if the wrong part of the HTTP request is being parsed [4][5]. 4. SDK Versioning: Ensure you are using the latest version of the SDK, as older versions may have different handling logic or bugs related to event decryption [6][7]. For further debugging, you can use the low-level event.EventDecrypt function provided by the SDK to isolate whether the failure is in the dispatcher's automatic handling or the decryption logic itself [6][8]. func EventDecrypt(encrypt string, secret string) ([]byte, error) If the decryption continues to fail, verify that no middleware or framework (e.g., Gin, standard net/http) is consuming or modifying the request body before it reaches the SDK handler [4][5].
Citations:
- 1: https://open.larksuite.com/document/ukTMukTMukTM/uYDNxYjL2QTM24iN0EjN/event-subscription-configure-/encrypt-key-encryption-configuration-case
- 2: https://open.feishu.cn/document/server-side-sdk/golang-sdk-guide/handle-events
- 3: https://open.larkoffice.com/document/server-side-sdk/golang-sdk-guide/handle-events
- 4: https://www.cnblogs.com/epngllh/p/15789931.html
- 5: err: event json unmarshal, err: invalid character 'c' looking for beginning of value oapi-sdk-go#140
- 6: https://pkg.go.dev/gitee.com/larksuite/oapi-sdk-go/v3/event
- 7: https://pkg.go.dev/github.com/larksuite/oapi-sdk-go/v3@v3.9.3/event
- 8: https://context7.com/larksuite/oapi-sdk-go/llms.txt
🌐 Web query:
raw.githubusercontent.com/larksuite/oapi-sdk-go v3.7.2 event/dispatcher/dispatcher.go subscription event decryption failed decryption failed
💡 Result:
The "decryption failed" error when using the larksuite/oapi-sdk-go event dispatcher typically indicates a mismatch between the configuration on the Lark Developer Platform and the credentials provided to the SDK [1][2][3]. When encryption is enabled in the Lark Developer Platform (under Event & Callback > Encryption Strategy), the SDK must be initialized with both the correct Verification Token and the correct Encrypt Key [1][2][3]. Troubleshooting steps: 1. Verify Credentials: Ensure that the Verification Token and Encrypt Key passed to dispatcher.NewEventDispatcher(verificationToken, encryptKey) exactly match those provided in the Lark developer console [2][3]. Even a slight mismatch will cause decryption to fail [1]. 2. Confirm Encryption Status: If you have not enabled "Encrypt Key" in the developer backend, ensure you are not passing a key to the dispatcher that the SDK attempts to use for decryption [2][3]. 3. Check SDK Initialization: Ensure the dispatcher is initialized correctly. In older or specific versions, incorrect parameter ordering or empty strings for disabled encryption can lead to unexpected behavior [2][3]. 4. Debugging: Use larkcore.WithLogLevel(larkcore.LogLevelDebug) when registering the event handler to see more detailed logs about the request processing [2][3]. The oapi-sdk-go performs decryption internally using the provided Encrypt Key [1]. If your application receives the event request but fails to decrypt it, the issue is almost certainly due to the Encrypt Key not matching the key used by the Lark server to encrypt the payload [1][2]. If the data itself is malformed or truncated before reaching the SDK, standard base64 decoding errors may also occur, which are often related to improper handling of character encoding or padding in the request body [4][5].
Citations:
- 1: https://open.larksuite.com/document/ukTMukTMukTM/uYDNxYjL2QTM24iN0EjN/event-subscription-configure-/encrypt-key-encryption-configuration-case
- 2: https://open.feishu.cn/document/server-side-sdk/golang-sdk-guide/handle-events
- 3: https://open.feishu.cn/document/uAjLw4CM/ukTMukTMukTM/server-side-sdk/golang-sdk-guide/handle-events
- 4: https://cloud.tencent.cn/developer/article/1810782
- 5: https://ask.csdn.net/questions/1018224
🌐 Web query:
"subscription event decryption failed" "larksuite/oapi-sdk-go" "subscription_id"
💡 Result:
If you encounter a "subscription event decryption failed" error when using the Lark/Feishu Open API SDK for Go, it is almost certainly caused by a mismatch between the encryption settings on the Lark Developer Platform and the configuration passed to your SDK's event handler [1][2][3]. When encryption is enabled in the Developer Platform (under Event & Callback > Encryption Strategy), you must provide the correct Encrypt Key and Verification Token when initializing the EventDispatcher [1][4][3]. To resolve this, ensure the following steps are verified: 1. Validate Credentials: Confirm that the Encrypt Key and Verification Token in your code match exactly what is displayed on the Event & Callback > Encryption Strategy page of your application in the Lark Developer Console [2]. 2. Initialize Dispatcher Correctly: When creating your dispatcher, ensure both parameters are provided [1][4][3]: handler:= dispatcher.NewEventDispatcher("your_verification_token", "your_encrypt_key") 3. Check SDK Usage: If you are using the EventDispatcher, ensure you are not attempting to manually decrypt the event payload using a different method while the dispatcher is already configured to handle it automatically [1][5]. The SDK's handler automatically manages decryption if these keys are provided [1][3]. 4. Debugging: Enable debug logging in your event handler to inspect the raw request and identify if the decryption failure occurs during initial parsing or during the decryption phase [1][5][6]: http.HandleFunc("/webhook/event", httpserverext.NewEventHandlerFunc(handler, larkevent.WithLogLevel(larkcore.LogLevelDebug))) If you are using the low-level event.EventDecrypt function directly, ensure you are passing the correct encrypted string extracted from the request headers (often the X-Lark-Content-V1-Encrypted header) and the correct secret key [7][8]. Note that if you prefer to avoid the complexities of manual decryption and webhook configuration, you can use the SDK's persistent connection (WebSocket) mode, which encapsulates authentication and delivers plain text data directly, eliminating the need for decryption logic on your server [6].
Citations:
- 1: https://open.feishu.cn/document/server-side-sdk/golang-sdk-guide/handle-events
- 2: https://open.larksuite.com/document/ukTMukTMukTM/uYDNxYjL2QTM24iN0EjN/event-subscription-configure-/encrypt-key-encryption-configuration-case
- 3: https://open.larkoffice.com/document/server-side-sdk/golang-sdk-guide/handle-events
- 4: https://open.feishu.cn/document/uAjLw4CM/ukTMukTMukTM/server-side-sdk/golang-sdk-guide/handle-events
- 5: https://github.com/larksuite/oapi-sdk-go/blob/9494618e8cd35005d152819fff6749059f7a53d1/sample/event/event.go
- 6: https://open.feishu.cn/document/uAjLw4CM/ukTMukTMukTM/server-side-sdk/golang-sdk-guide/handle-callback
- 7: https://context7.com/larksuite/oapi-sdk-go/llms.txt
- 8: https://pkg.go.dev/gitee.com/larksuite/oapi-sdk-go/v3/event
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Feishu source relevant section:"
sed -n '520,650p' internal/event/source/feishu.go
echo
echo "Feishu tests around decrypt failure fixtures:"
sed -n '450,525p' internal/event/source/feishu_test.go
echo
echo "Check whether tests import/pull live SDK sources or just own fixtures:"
rg -n 'TryDecryptEnvelope|decryptSubscriptionEnvelope|subscription event decryption failed|DecryptSubscription|oapi-sdk-go.*v3' internal/event/source/internal/event/source/feishu_test.go internal/event/source/feishu.go || trueRepository: larksuite/cli
Length of output: 10929
Gate decrypt-failure logs on a positive SDK-error match.
redactDecryptFailureLine only passes the raw crypto/padding tail through when the pinned SDK wording no longer matches, creating fail-open drift on a security control. Make SDK upgrades fail loudly (for example, assert the current SDK error text) and require the encrypt-mismatch prefix for decryption failures instead of any error containing encrypt mismatch/invalid token.
🤖 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/source/feishu.go` around lines 556 - 590, Harden
redactDecryptFailureLine and subscriptionDecryptFailureRe so redaction occurs
only for the exact pinned SDK decrypt-failure format with the required
encrypt-mismatch prefix, not merely arbitrary “encrypt mismatch” or “invalid
token” text. Add an explicit assertion of the expected SDK error wording so SDK
changes fail loudly rather than silently passing raw crypto details through.
c3ee4a9 to
cfe76ad
Compare
leave330
changed the title
1 participant
Withdrawn; work moved out of this branch.