feat(im): make IM operations agent-reliable - #2106
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:
📝 WalkthroughWalkthroughThe PR adds contract-aware IM execution, completion and recovery metadata, shared pagination, structured mentions, idempotency handling, governed media behavior, contract-aware help, quality gates, and expanded IM documentation and coverage. ChangesIM contract execution
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
github-actions
Bot
added
domain/im
5da4257 to
60dc511
Compare
09b52a9 to
20a5f50
Compare
60dc511 to
24d926e
Compare
20a5f50 to
89af6d1
Compare
a03b388 to
2e7007c
Compare
…pdates Replay the seeded IM failure cases. Most already give an agent enough signal to recover; deterministic search coverage requires stable fixtures, while the --head/--tail conflict lacked a concrete next action. Add the missing hint, record the inventory, and fold the search fixture prerequisite and dry-run coverage into coverage.md.
2e7007c to
c45fad0
Compare
github-actions
Bot
added
domain/base
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2106 +/- ##
==========================================
+ Coverage 75.46% 75.75% +0.29%
==========================================
Files 928 946 +18
Lines 98479 100984 +2505
==========================================
+ Hits 74314 76499 +2185
- Misses 18511 18670 +159
- Partials 5654 5815 +161 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
BREAKING CHANGE: im +chat-create now requires --idempotency-key.
c45fad0 to
ac8c577
Compare
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@67d71b569751642103a40d4b69cda0d3f4b3caae🧩 Skill updatenpx skills add larksuite/cli#feat/im-agent-reliability-closeout -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
shortcuts/im/im_messages_search.go (1)
368-377: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the computed page limit in the pagination helper.
messagesSearchPaginationConfig()returns 40 when--page-allis set without--page-limit, butpaginateIMWithMode()readsruntime.Int("page-limit")instead, which is the--page-limitflag default. The dry-run description and “stopped after fetching” warning can report 40 while pagination stops at the default page limit.🤖 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 `@shortcuts/im/im_messages_search.go` around lines 368 - 377, The pagination flow must use the pageLimit computed by messagesSearchPaginationConfig(), including the messagesSearchMaxPageLimit value selected for page-all. Update paginateIMWithMode() to consume and propagate that returned limit for pagination and related dry-run or warning messages instead of rereading runtime.Int("page-limit").
🧹 Nitpick comments (28)
internal/qualitygate/rules/run.go (1)
47-47: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo mechanisms protect the IM coverage diagnostics; one is unreachable.
RunappendsimContractDiagsat Line 115, afterfilterPRDiagnosticsruns at Line 114. So IM coverage diagnostics never reach the filter from this path. The rule bypass added infilterPRDiagnostics(Lines 218-221) is therefore dead for production callers, and onlyTestIMContractCoverageDiagnosticIsNotChangedFileFilteredexercises it.Keep one mechanism. Either append the IM diagnostics before filtering and rely on the rule bypass, or remove the bypass and keep the post-filter append.
♻️ Option A: rely on the rule bypass in the filter
- diags = filterPRDiagnostics(opts.Repo, opts.ChangedFrom, scope, m, diags) - diags = append(diags, imContractDiags...) + diags = append(diags, imContractDiags...) + diags = filterPRDiagnostics(opts.Repo, opts.ChangedFrom, scope, m, diags)Also applies to: 115-115
🤖 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/qualitygate/rules/run.go` at line 47, Consolidate IM coverage diagnostic protection in Run and filterPRDiagnostics by choosing one mechanism: either append imContractDiags before filtering so the existing bypass handles them, or remove the bypass and retain the post-filter append. Eliminate the redundant or unreachable path while preserving IM coverage diagnostics for production callers and existing tests.internal/affordance/affordance_im_test.go (1)
8-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an
internal/vfs-backed FS root for this internal test.
internal/affordance/affordance_im_test.go:40usesos.DirFSunderinternal/, but the guideline forinternal/tests is to avoid standard-library filesystem APIs. Add or use aninternal/vfshelper that builds anio/fs.FSfor the repository tree, then pass that root toSetSource.🤖 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/affordance/affordance_im_test.go` at line 8, Replace the os.DirFS-based repository root in the affordance test with an internal/vfs helper that returns an io/fs.FS for the repository tree, then pass that vfs-backed root to SetSource. Remove the direct os filesystem usage while preserving the test’s existing source path and behavior.Source: Learnings
internal/imcontract/materialization.go (1)
9-16: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider adding explicit tags to the remaining fields.
RequestedIDs,ResolvedIDs, andCausecarryjson:"-". The other three fields carry no tag. Serialization happens throughledger(), so behavior is correct today. The mixed tagging suggests that direct marshaling is supported, which would emit Go field names instead of the snake_case keys used byledger(). Add explicit tags or drop all tags and rely only onledger().🤖 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/imcontract/materialization.go` around lines 9 - 16, Add explicit JSON tags to MissingMessageIDs, UnresolvedHitCount, and UnexpectedMessageCount in MaterializationStatus, using the snake_case keys emitted by ledger(), while preserving json:"-" for RequestedIDs, ResolvedIDs, and Cause.internal/imcontract/http_status_test.go (1)
30-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case for a 4xx status with an existing error.
The test covers 503, 429, 404 without an error, and 200. It does not cover the branch that returns the incoming error unchanged for a 4xx status, and it does not cover the generic 4xx mapping to
errs.SubtypeUnknown. Add both cases so that a change in that branch fails the test.💚 Proposed additional cases
notFound := NormalizeHTTPError(404, "", nil) notFoundProblem, ok := errs.ProblemOf(notFound) if !ok || notFoundProblem.Subtype != errs.SubtypeNotFound || notFoundProblem.Code != 404 || notFoundProblem.Retryable { t.Fatalf("not-found problem = %#v, err=%T %v", notFoundProblem, notFound, notFound) } + if kept := NormalizeHTTPError(400, "log-id", original); kept != original { + t.Fatalf("typed lower-layer error was replaced: %T %v", kept, kept) + } + + generic := NormalizeHTTPError(400, "", nil) + genericProblem, ok := errs.ProblemOf(generic) + if !ok || genericProblem.Category != errs.CategoryAPI || + genericProblem.Subtype != errs.SubtypeUnknown || + genericProblem.Code != 400 || genericProblem.Retryable { + t.Fatalf("generic 4xx problem = %#v", genericProblem) + } + if unchanged := NormalizeHTTPError(200, "", original); unchanged != original {As per coding guidelines: "Every behavior change must have an accompanying test, and contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
🤖 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/imcontract/http_status_test.go` around lines 30 - 39, Add test cases for NormalizeHTTPError covering a generic 4xx status mapping to errs.SubtypeUnknown and a 4xx status with an existing error returning that same error unchanged. Assert the relevant subtype and object identity so regressions in both branches fail the test.Source: Coding guidelines
internal/imcontract/write.go (1)
140-141: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low value
root["completion"]can overwrite a server field.
finalizeBatchwrites the ledger into the decoded response map.finalizeAssertiondoes the same at Line 193. If a server response ever contains acompletionfield, the CLI discards it without notice. Consider rejecting the response with a typed internal error whenrootalready has acompletionkey, so that server data is never silently replaced.As per coding guidelines: "When transcribing input or transforming requests, preserve values faithfully; never silently coerce unsupported inputs, ignore unhonored options, default missing identities, or discard writes."
🤖 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/imcontract/write.go` around lines 140 - 141, Update finalizeBatch and finalizeAssertion to detect whether the decoded response map already contains the completion key before assigning the ledger. If present, reject the response with the established typed internal error mechanism; otherwise preserve the existing ledger assignment and result construction without overwriting server data.Source: Coding guidelines
internal/output/envelope_success_test.go (1)
239-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the partial result payload.
The test name states that the envelope carries the partial result, but the assertions cover only
ok,hint, anderror.type. TheDatapayload withcompletion.status == "partial"is not asserted. A regression that dropsDataon the partial-failure path would still pass.💚 Proposed additional assertion
if env["error"].(map[string]any)["type"] != "api" { t.Fatalf("typed error missing: %#v", env) } + data, ok := env["data"].(map[string]any) + if !ok { + t.Fatalf("partial result data missing: %#v", env) + } + completion, ok := data["completion"].(map[string]any) + if !ok || completion["status"] != "partial" { + t.Fatalf("partial completion missing: %#v", data) + } }As per coding guidelines: "contract tests must assert the changed field or behavior directly so reverting the implementation causes failure."
🤖 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/output/envelope_success_test.go` around lines 239 - 248, Extend the assertions in the partial-failure envelope test to validate the Data payload directly, including completion.status == "partial". Reuse the decoded env structure and assert the nested payload is present and has the expected status, so dropping Data causes the test to fail.Source: Coding guidelines
internal/imcontract/write_test.go (2)
400-405: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert
Paramon the validation error.This test asserts
CategoryandSubtypeonly.ObserveRequestreturns a validation error, and validation errors carryParam, which identifies the offending field. Useerrors.As(err, &ve)with*errs.ValidationErrorand assertve.Paramso that a change in the reported field breaks the test.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."Based on learnings:
errs.ProblemOfdoes not exposeParam; read it from*errs.ValidationErrorthrougherrors.As.🤖 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/imcontract/write_test.go` around lines 400 - 405, Update the ObserveRequest error assertion to extract the error with errors.As into *errs.ValidationError, while retaining the existing errs.ProblemOf category and subtype checks. Assert ve.Param matches the offending request field, and verify the wrapped cause is preserved as required by the error-path test guidelines.Sources: Coding guidelines, Learnings
41-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the
ObserveRequesterror in these tests.
ObserveRequestreturns an error, and several tests discard it (Lines 41, 61, 75, 108, 135, 151, 365, 378, 473, 494). If request-evidence validation starts to fail,s.requestedstays empty. Tests such asTestBatchRejectsUnmappableFailureEvidencethen still fail on the response, so they pass for the wrong reason. Assert the error, asTestBatchPartialRecoveryMatrixdoes at Line 295.💚 Example fix
- s.ObserveRequest(tc.request) + if err := s.ObserveRequest(tc.request); err != nil { + t.Fatalf("ObserveRequest() error = %v", err) + }Also applies to: 365-366
🤖 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/imcontract/write_test.go` around lines 41 - 42, Update every listed test call to ObserveRequest in write_test.go to capture and assert its returned error, matching the pattern used by TestBatchPartialRecoveryMatrix. Ensure request-evidence validation completes successfully before FinalizeSuccess or other response assertions, including the occurrences around lines 41, 61, 75, 108, 135, 151, 365, 378, 473, and 494.internal/imcontract/registry.go (1)
20-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMove the unused production helper out of
registry.go.
stringsFromis not referenced by production code and is unrelated to registry lookup. Move it next to the evidence extraction code ininternal/imcontract/ledger.go.🤖 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/imcontract/registry.go` around lines 20 - 22, Move the unused stringsFrom helper from registry.go to ledger.go alongside the evidence extraction code, preserving its signature and behavior; do not otherwise change registry lookup logic.internal/imcontract/message_mentions_test.go (1)
22-114: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding two uncovered builder cases.
The table omits two reachable branches of
BuildMessageMentionResult:
All: truetogether with confirmableIDs, which must yieldstatus = "accepted_unverified"and non-emptyConfirmed(lines 108-112 ofinternal/imcontract/message_mentions.go).- A non-slice
response, for examplemap[string]any{}, which setsambiguousinparseResponseMentionsand must yieldpartial_unattributed.🤖 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/imcontract/message_mentions_test.go` around lines 22 - 114, The table for BuildMessageMentionResult is missing coverage for two reachable branches. Add a case with All true and confirmable IDs, asserting accepted_unverified with a non-empty Confirmed result, and add a case using a non-slice response such as map[string]any, asserting partial_unattributed from the ambiguous parseResponseMentions path.internal/imcontract/catalog/registry.go (2)
43-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider passing materialization as a parameter instead of matching the key.
searchcompareskeywith the literal"im +messages-search"to decideRequiresMaterialization. The builder then depends on one specific catalog entry. A boolean parameter, or an explicit assignment at the call site on line 127, keeps the builder generic.🤖 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/imcontract/catalog/registry.go` around lines 43 - 55, Make search generic by removing its key-specific "im +messages-search" comparison and accepting materialization explicitly, either through a boolean parameter or assignment at the relevant call site. Ensure the catalog entry for "im +messages-search" still sets Strategy.RequiresMaterialization to true while other search contracts retain the default.
259-274: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider rejecting duplicate contract keys while building the map.
buildContractswrites each entry intooutkeyed byc.Key. A duplicate key silently overwrites the earlier entry.ValidateRegistryiterates the map, so it cannot detect the loss. The count and key-list assertions ininternal/imcontract/registry_test.gowould fail, but the failure message would not name the duplicate.♻️ Proposed fix to detect duplicates at build time
out := make(map[ContractKey]Contract, len(all)) + duplicates := make([]ContractKey, 0) for _, c := range all { + if _, exists := out[c.Key]; exists { + duplicates = append(duplicates, c.Key) + } if c.PartialRecovery == "" &&Then report
duplicatesfromValidateRegistry, or panic in aninitguard if the catalog must be correct by construction.🤖 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/imcontract/catalog/registry.go` around lines 259 - 274, Update buildContracts to detect repeated c.Key values before assigning to out, and reject them instead of silently overwriting the existing contract. Propagate the duplicate keys to ValidateRegistry so its validation error names the conflicting keys, or use the project’s initialization guard to fail immediately if catalog correctness is required at startup.internal/imcontract/catalog/types.go (1)
114-131: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider decoupling the acceptance-only text from one command.
HelpAcceptanceOnlyreturns a message that namesim chat.moderation get. Today onlyim chat.moderation updateusesAcceptanceOnlyKind, so the text is correct. If another acceptance-only contract is added, the text will point to the wrong verification command. Storing the verification command on theContract(or onStrategy.ReadHint, asim reactions batch_queryalready does) removes that risk.🤖 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/imcontract/catalog/types.go` around lines 114 - 131, Decouple HelpAcceptanceOnly from the hard-coded moderation command by storing the applicable verification command on each Contract or reusing Strategy.ReadHint, consistent with im reactions batch_query. Update the acceptance-only help generation around HelpPolicy.Text and its callers so each contract supplies its own verification guidance, while preserving the existing moderation update message.internal/imcontract/identity_notice_test.go (1)
8-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a nil-base case.
WithIdentityDefaultedNoticehas a dedicated branch for a nilbasemap, becausemaps.Clone(nil)returns nil. No test covers that branch. Add a case that passesniland asserts the returned map contains the identity notice.🤖 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/imcontract/identity_notice_test.go` around lines 8 - 31, Add a nil-base test case for WithIdentityDefaultedNotice, passing nil and asserting the returned map contains IdentityDefaultedNoticeKey with the expected resolved identity and message. Cover the dedicated nil handling branch while preserving the existing non-mutating merge test.internal/output/emitter_contract_test.go (1)
152-181: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the redaction property, not only the failure shape.
The test proves
RedactedFallbackbypasses the blocking scan. It does not prove the emitted document contains only allowlisted fields. Add an assertion that the decodeddata.completioncontains the expected keys and no payload text from the blocked result. The assertion then fails if a future change routes the original payload through this 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/output/emitter_contract_test.go` around lines 152 - 181, The test TestEmitterRedactedFallbackSkipsBlockedPresentationScan must verify the redacted payload contents, not just the failure envelope. After decoding env, inspect data.completion and assert it contains only the expected allowlisted keys, with no payload text from the blocked result; keep the existing env.OK and env.Error assertions unchanged.internal/output/emitter.go (1)
102-124: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider routing the hint to stderr automatically for non-envelope formats.
emitHintwrites nothing unlessHintToStderris true. Forpretty,table, and other non-envelope formats the envelope is not written, so a caller that setsHintand forgetsHintToStderrloses the hint with no signal. Deriving the stderr routing from the selected format would remove that failure mode.Also applies to: 351-359
🤖 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/output/emitter.go` around lines 102 - 124, The Success method should automatically route hints to stderr when emitting non-envelope formats such as pretty, table, or other formatted output. Before calling emitHint, derive the hint options from the selected output format while preserving explicit HintToStderr behavior and leaving JSON/JQ envelope output unchanged.internal/imcontract/read_test.go (1)
385-401: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
strings.Containsinstead of the hand-rolled substring scan.The file already imports
strings.stringContainsduplicatesstrings.Contains.♻️ Proposed simplification
func containsAny(s string, values ...string) bool { for _, value := range values { - if value != "" && stringContains(s, value) { + if value != "" && strings.Contains(s, value) { return true } } return false } - -func stringContains(s, substr string) bool { - for i := 0; i+len(substr) <= len(s); i++ { - if s[i:i+len(substr)] == substr { - return true - } - } - return 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 `@internal/imcontract/read_test.go` around lines 385 - 401, Replace the hand-rolled scan in stringContains with the existing strings.Contains helper, preserving the current boolean result and leaving containsAny’s empty-value filtering unchanged. Remove stringContains if it is no longer needed and update its call site accordingly.internal/client/pagination_status_test.go (1)
279-315: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert typed metadata and cause preservation for the late-error cases.
Both subtests assert only the Go error type. The repository guideline requires error-path tests to assert
category,subtype, andparamthrougherrs.ProblemOf, and to verify cause preservation. Add those assertions, including the wrapped*net.DNSErrorfor the transport case and the upstream API failure for the API case.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."♻️ Example assertion for the transport case
var networkErr *errs.NetworkError if !errors.As(err, &networkErr) { t.Fatalf("error = %T %v, want typed NetworkError", err, err) } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Category != errs.CategoryNetwork { + t.Fatalf("problem = %#v, want network category", problem) + } + var dnsErr *net.DNSError + if !errors.As(err, &dnsErr) { + t.Fatalf("error = %v, want preserved DNS cause", 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 `@internal/client/pagination_status_test.go` around lines 279 - 315, Extend the late transport-error and API-error subtests around PaginateAllWithStatus to validate errs.ProblemOf(err) metadata for category, subtype, and param, not just errors.As type checks. Also assert cause preservation: the transport error must retain the wrapped *net.DNSError, and the API error must retain the upstream API failure as its cause, while preserving the existing partial-page and status assertions.Source: Coding guidelines
shortcuts/im/im_chat_members_list.go (1)
53-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueName the non-default page limit.
imPaginationFlags(10)hides why this command differs from the other IM reads, which passimReadDefaultPageLimit. Declare a named constant next tochatMembersListDefaultPageSize, for examplechatMembersListDefaultPageLimit = 10, and pass it here.🤖 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 `@shortcuts/im/im_chat_members_list.go` at line 53, Declare a named chat-members page-limit constant next to chatMembersListDefaultPageSize, then replace the literal 10 passed to imPaginationFlags in the chat members list command with that constant; keep the existing pagination behavior unchanged.shortcuts/common/runner_partial_failure_test.go (3)
108-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCheck the
ObserveRequesterror here, as the test does at Line 135.Line 110 discards the error from
ObserveRequest. If request observation fails, the ledger has no requested items and the later completion assertions become misleading. Line 135 already uses the checked form; apply it here too.💚 Proposed fix
- rt.contractSession.ObserveRequest(map[string]any{"user_id_list": []any{"ou_a", "ou_b"}}) + if err := rt.contractSession.ObserveRequest(map[string]any{"user_id_list": []any{"ou_a", "ou_b"}}); err != nil { + t.Fatal(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 `@shortcuts/common/runner_partial_failure_test.go` around lines 108 - 110, Update the setup around rt.contractSession.ObserveRequest in the test to capture and check its returned error, matching the checked form already used at the later ObserveRequest call around line 135; fail the test immediately when observation fails instead of discarding the error.
91-92: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
imcontract.Lookupresult in the new contract tests. Every new test discards theokvalue fromimcontract.Lookup. If a contract key is renamed or removed,NewSessionreceives a zeroContractand the tests stop covering the intended contract without failing. Add a small helper in the package, for examplemustLookupContract(t, key), and use it at each site.
shortcuts/common/runner_partial_failure_test.go#L91-L92: use the helper here and at the other lookups in this file (Line 108, Line 133, Line 176, Line 302, Line 321, Line 362, Line 414).shortcuts/common/call_api_typed_test.go#L166-L177: use the helper forim messages urgent_app.shortcuts/common/runner_contentsafety_test.go#L110-L111: use the helper forim chat.moderation update.shortcuts/common/runner_jq_test.go#L182-L183: use the helper forim +messages-send.🤖 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 `@shortcuts/common/runner_partial_failure_test.go` around lines 91 - 92, Add a package-level mustLookupContract(t, key) helper that asserts the success result from imcontract.Lookup and returns the contract. Replace every unchecked lookup in shortcuts/common/runner_partial_failure_test.go at lines 91-92, 108, 133, 176, 302, 321, 362, and 414, plus the lookup in shortcuts/common/call_api_typed_test.go lines 166-177, shortcuts/common/runner_contentsafety_test.go lines 110-111, and shortcuts/common/runner_jq_test.go lines 182-183; each site must use the helper so missing or renamed contracts fail the tests.
320-323: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFail the test when
NewReadSessionreturns an error.The assignment discards the error. If construction fails,
rt.readSessionis nil and the test then asserts legacy non-contract output instead of the read contract. The same pattern appears at Line 363 and Line 415.💚 Proposed fix
- rt.readSession, _ = imcontract.NewReadSession(contract, imcontract.ReadOptions{FullRead: true}) + readSession, err := imcontract.NewReadSession(contract, imcontract.ReadOptions{FullRead: true}) + if err != nil { + t.Fatal(err) + } + rt.readSession = readSession🤖 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 `@shortcuts/common/runner_partial_failure_test.go` around lines 320 - 323, Update the `imcontract.NewReadSession` assignments near the `rt.readSession` setup, including the corresponding cases around lines 363 and 415, to capture and assert the returned error immediately. Make each test fail when session construction fails before using `rt.readSession`, while preserving the existing contract-based assertions.cmd/service/service.go (1)
497-507: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the write-side
--page-allrejection next to the read-side guard.The read guard runs at Line 391, before identity resolution, config load, and scope checks. The write guard runs here, after
buildServiceRequestand aftercontractSession.ObserveRequest. Both are pure flag validations. Reject--page-allfor contract-managed writes together with the read guard so the command fails before credential and request work.♻️ Proposed relocation
contractManagedWrite := contractFound && contract.Strategy.Kind.IsWrite() contractManagedRead := contractFound && contract.Strategy.Kind.IsRead() + if contractManagedWrite && opts.PageAll { + return errs.NewValidationError(errs.SubtypeInvalidArgument, + "--page-all is not valid for an IM write command").WithParam("--page-all") + } if contractManagedRead && opts.PageAll &&if opts.PageAll { - if contractSession != nil { - return errs.NewValidationError(errs.SubtypeInvalidArgument, - "--page-all is not valid for an IM write command").WithParam("--page-all") - } if readSession != 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 `@cmd/service/service.go` around lines 497 - 507, Move the contract-managed write rejection for --page-all from the PageAll branch near servicePaginate into the existing early flag-validation guard alongside the read-side --page-all check. Ensure contract writes are rejected before identity resolution, configuration loading, scope checks, buildServiceRequest, and contractSession.ObserveRequest, while preserving the existing read pagination and non-contract pagination behavior.shortcuts/common/runner.go (1)
507-520: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the shared idempotency-key merge into
internal/imcontract. Both runtimes rebuild the observed write payload by copying the request body and adding theuuidquery value. The rule is one contract-evidence rule, so a change in one place will silently diverge from the other.
shortcuts/common/runner.go#L507-L520: replace the inline clone with a call to a shared helper, for exampleimcontract.RequestEvidence(body, uuid), and passquery["uuid"].cmd/service/service.go#L473-L488: replace the inline clone with the same helper, and passrequest.Params["uuid"].🤖 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 `@shortcuts/common/runner.go` around lines 507 - 520, Extract the shared request-evidence merge into an imcontract helper, such as RequestEvidence, that copies the request body and applies the uuid value. Update shortcuts/common/runner.go lines 507-520 to use the helper with query["uuid"], and cmd/service/service.go lines 473-488 to use the same helper with request.Params["uuid"], removing both inline clone implementations.shortcuts/im/mentions.go (1)
399-412: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueGuard the slice capacity against an empty request.
len(targets)*2-1evaluates to-1whentargetsis empty, andmakepanics on a negative capacity. Today every caller passes a requested mention, so the path is unreachable. Compute the capacity so a future caller cannot panic.♻️ Proposed defensive fix
- paragraph := make([]map[string]interface{}, 0, len(targets)*2-1) + capacity := 0 + if len(targets) > 0 { + capacity = len(targets)*2 - 1 + } + paragraph := make([]map[string]interface{}, 0, capacity)🤖 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 `@shortcuts/im/mentions.go` around lines 399 - 412, Update postMentionParagraph to compute a non-negative paragraph capacity when targets is empty, while retaining the existing len(targets)*2-1 capacity for non-empty requests. Ensure make cannot panic for an empty request and preserve the current paragraph construction behavior.shortcuts/im/helpers.go (1)
26-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused legacy inline at-tag helper.
normalizeAtMentionshas no callers outside its declaration, so deletenormalizeAtMentionsandmentionFixRewith this change.🤖 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 `@shortcuts/im/helpers.go` around lines 26 - 33, Remove the unused normalizeAtMentions helper and its associated mentionFixRe declaration from shortcuts/im/helpers.go, leaving the surrounding imports and helper functionality intact.internal/imcontract/output_fallback_test.go (1)
130-140: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
errs.ProblemOffor typed metadata assertions.This test asserts
Category,Subtype, andMessagethrough a direct type assertionenv.Error.(*errs.Problem). As per path instructions for**/*_test.go, "Error-path tests must assert typed metadata througherrs.ProblemOf(category,subtype, andparam) and verify cause preservation rather than relying only on message substrings." Useerrs.ProblemOf(env.Error)instead of the direct type assertion. This also avoids an unhandled panic ifenv.Errorever holds a different concrete type.♻️ Proposed fix
- problem := env.Error.(*errs.Problem) - if problem.Category != errs.CategoryPolicy || problem.Subtype != errs.SubtypeContentSafety || + problem, ok := errs.ProblemOf(env.Error) + if !ok { + t.Fatalf("env.Error is not a typed problem: %#v", env.Error) + } + if problem.Category != errs.CategoryPolicy || problem.Subtype != errs.SubtypeContentSafety || problem.Message != "Output blocked after the IM write completed" { t.Fatalf("problem = %#v", problem) }🤖 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/imcontract/output_fallback_test.go` around lines 130 - 140, Update TestContentSafetyOutputFallbackUsesFixedPublicProblem to obtain the typed problem through errs.ProblemOf(env.Error) instead of directly asserting *errs.Problem. Assert the returned category, subtype, and param metadata as required, and verify the original cause is preserved alongside the existing exit-code validation.Source: Path instructions
internal/imcontract/session.go (1)
124-126: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd named constants for the message-mention contract keys.
internal/imcontract/session.gocheckskey == "im +messages-send"orkey == "im +messages-reply"while the catalog registers those keys asrequired(...)literals. Add catalogContractKeyconstants for these operations and use them fromsupportsMessageMentionResultto keep the selector and catalog together.🤖 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/imcontract/session.go` around lines 124 - 126, Add named ContractKey constants for the “im +messages-send” and “im +messages-reply” catalog entries, then update supportsMessageMentionResult to compare against those constants instead of string literals. Replace the corresponding required(...) literals in the catalog with the same constants so registration and selection share a single source of truth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@affordance/im.md`:
- Around line 146-178: Add an `### Examples` block to each of `messages
urgent_app`, `messages urgent_phone`, and `messages urgent_sms`, using a
copyable command consistent with the proposed `lark-cli im messages urgent_*`
invocation and each section’s command name. Place the block after `###
Prerequisites` and preserve the existing guidance and prerequisite text.
In `@internal/client/pagination_status.go`:
- Around line 121-124: Update PaginateAllWithStatus and its related pagination
loop to treat PaginationOptions.PageLimit == 0 as the documented default of 10,
matching PaginateAll and StreamPages rather than fetching unlimited pages.
Preserve explicit nonzero PageLimit values and keep the existing page-delay
handling unchanged.
In `@internal/imcontract/http_status.go`:
- Around line 15-32: Update the 5xx and 429 handling in the HTTP status
normalization function to attach the incoming err via WithCause(err) whenever
err is non-nil, while preserving the existing HTTP-driven classifications and
metadata. Keep nil errors unattached and align this behavior with the existing
4xx branch.
In `@internal/imcontract/message_mentions_test.go`:
- Around line 155-159: Update the error-path table cases “unknown status” and
“replay scope cannot authorize replay” with expected category and subtype
metadata, then extend the test assertions to inspect the typed error via
errs.ProblemOf and verify category, subtype, param, and cause preservation.
Replace the existing err-only checks while keeping the current successful-case
behavior unchanged.
In `@internal/imcontract/output_fallback.go`:
- Around line 42-92: Update allowlistedCompletion to use a conservative fallback
status instead of seeding status as "complete"; use the established
"accepted_unverified" value (and avoid implying retry_scope "none" when the
source status is unknown). Preserve copyCompletionStatus behavior for recognized
statuses while ensuring unrecognized completion statuses cannot be reported as
complete and bypass recovery.
In `@internal/output/envelope_success.go`:
- Around line 55-88: Update WriteEnvelope and its EmitOptions construction to
propagate env.ContentSafetyAlert into the emitter, preserving the supplied
envelope value through emission. Use the existing ContentSafetyAlert field and
avoid silently discarding it.
In `@shortcuts/common/runner.go`:
- Around line 802-808: Update RuntimeContext.OutPartialFailure to return the
contract-provided resultExit after emitFinalized when that result is present,
while preserving the existing outputErr handling and
output.PartialFailure(output.ExitAPI) fallback when no contract result exists.
In `@shortcuts/im/im_chat_messages_list.go`:
- Around line 124-132: Preserve the user-supplied page_token in the
first-request branches of the paginateIM callbacks in
shortcuts/im/im_chat_messages_list.go (lines 124-132) and
shortcuts/im/im_chat_search.go (lines 106-114) instead of deleting it when
pageToken is empty; alternatively, return the established typed validation error
when pagination cannot continue from that requested start token.
In `@shortcuts/im/im_feed_group_item_test.go`:
- Line 426: Extend the error-path assertions in
shortcuts/im/im_feed_group_item_test.go:426-426 for the “list bad page-limit”
case to require problem.Category == errs.CategoryValidation alongside subtype
and parameter metadata. In shortcuts/im/builders_test.go:421-437 and 679-695,
after errs.ProblemOf(err), add the same CategoryValidation assertion while
preserving the existing subtype and param checks.
In `@shortcuts/im/im_feed_shortcut_test.go`:
- Around line 494-495: Validate the contract lookups before creating sessions in
both affected sites: shortcuts/im/im_feed_shortcut_test.go lines 494-495 for “im
+feed-shortcut-create” and lines 703-704 for “im +feed-shortcut-remove”. Capture
the ok result from imcontract.Lookup and call t.Fatal with a clear
missing-contract message when it is false; only pass the validated contract to
imcontract.NewSession.
In `@shortcuts/im/im_flag_cancel.go`:
- Around line 157-167: Update the error branches in the feed-layer resolution
flow around getMessageChatID and resolveThreadFeedItemType to preserve the
specific unknown-feed-layer skip reason in the structured completion/result
payload or tracked stderr entry. Keep recording FactFlagFeedLayerPending, but
ensure finalization no longer relies solely on that fact kind to synthesize a
generic “feed” pending item.
In `@shortcuts/im/im_flag_list.go`:
- Around line 217-219: Update executeListAllPages in
shortcuts/im/im_flag_list.go at lines 217-219 and searchMessages in
shortcuts/im/im_messages_search.go at lines 391-393 to propagate or record
pageErr even when pages were successfully fetched, using the shared pagination
status mechanism such as RecordPagination. Ensure any post-first-page pagination
failure marks the read incomplete and causes callers to fail closed rather than
returning partial data as success.
In `@shortcuts/im/im_messages_send.go`:
- Around line 226-233: Make mention post-processing non-failing after a
successful write by parsing the mention request once before the write in
shortcuts/im/im_messages_send.go lines 226-233 and passing that mentionRequest
into the result builder instead of calling addMessageMentionResult to re-parse
it; apply the same change in shortcuts/im/im_messages_reply.go lines 202-209,
reusing the mention request computed before sending the reply.
In `@skills/lark-im/references/lark-im-chat-create.md`:
- Around line 145-146: Update the chat creation command in Scenario 3 to include
the explicit --as bot option, matching the identity used by the subsequent
messages-send command.
In `@skills/lark-im/references/lark-im-chat-identity.md`:
- Line 43: Pin bot identity consistently in the documented chat workflows:
update the group-creation command in
skills/lark-im/references/lark-im-chat-identity.md:43-43 to run as bot, and
update the preceding +chat-search command in
skills/lark-im/references/lark-im-chat-search.md:107-107 to run as bot as well,
keeping both operations under the same identity.
In `@skills/lark-im/references/lark-im-feed-shortcut-list.md`:
- Around line 9-13: Make the exhaustive-read contract explicit across all listed
references: in skills/lark-im/references/lark-im-feed-shortcut-list.md lines
9-13, document meta.complete in the response structure and JSON example; in
skills/lark-im/references/lark-im-feed-group-list.md lines 3-5 and 26-37, state
that merging covers fetched pages only and require meta.complete=true beside
pagination and output guidance; in
skills/lark-im/references/lark-im-feed-group-list-item.md lines 31-39,
distinguish merged pages from a complete dataset and document the
meta.complete=true gate; in skills/lark-im/references/lark-im-feed-groups.md
lines 33, 245, and 329, add the same completion condition to the overview, list,
and list_item documentation; and in
skills/lark-im/references/lark-im-flag-list.md line 9, replace the vague
completion wording with the exact meta.complete=true check and its output path.
In `@skills/lark-im/references/lark-im-messages-send.md`:
- Line 267: Align the remote Markdown-image upload failure contract in both
skills/lark-im/references/lark-im-messages-send.md:267 and
skills/lark-im/references/lark-im-messages-reply.md:267 with the existing rules
at Lines 72/277 and 73/276: remove the failed image, continue sending the
message, and issue the documented warning. Ensure both references describe the
same behavior and do not claim that nothing is sent or that the command fails.
---
Outside diff comments:
In `@shortcuts/im/im_messages_search.go`:
- Around line 368-377: The pagination flow must use the pageLimit computed by
messagesSearchPaginationConfig(), including the messagesSearchMaxPageLimit value
selected for page-all. Update paginateIMWithMode() to consume and propagate that
returned limit for pagination and related dry-run or warning messages instead of
rereading runtime.Int("page-limit").
---
Nitpick comments:
In `@cmd/service/service.go`:
- Around line 497-507: Move the contract-managed write rejection for --page-all
from the PageAll branch near servicePaginate into the existing early
flag-validation guard alongside the read-side --page-all check. Ensure contract
writes are rejected before identity resolution, configuration loading, scope
checks, buildServiceRequest, and contractSession.ObserveRequest, while
preserving the existing read pagination and non-contract pagination behavior.
In `@internal/affordance/affordance_im_test.go`:
- Line 8: Replace the os.DirFS-based repository root in the affordance test with
an internal/vfs helper that returns an io/fs.FS for the repository tree, then
pass that vfs-backed root to SetSource. Remove the direct os filesystem usage
while preserving the test’s existing source path and behavior.
In `@internal/client/pagination_status_test.go`:
- Around line 279-315: Extend the late transport-error and API-error subtests
around PaginateAllWithStatus to validate errs.ProblemOf(err) metadata for
category, subtype, and param, not just errors.As type checks. Also assert cause
preservation: the transport error must retain the wrapped *net.DNSError, and the
API error must retain the upstream API failure as its cause, while preserving
the existing partial-page and status assertions.
In `@internal/imcontract/catalog/registry.go`:
- Around line 43-55: Make search generic by removing its key-specific "im
+messages-search" comparison and accepting materialization explicitly, either
through a boolean parameter or assignment at the relevant call site. Ensure the
catalog entry for "im +messages-search" still sets
Strategy.RequiresMaterialization to true while other search contracts retain the
default.
- Around line 259-274: Update buildContracts to detect repeated c.Key values
before assigning to out, and reject them instead of silently overwriting the
existing contract. Propagate the duplicate keys to ValidateRegistry so its
validation error names the conflicting keys, or use the project’s initialization
guard to fail immediately if catalog correctness is required at startup.
In `@internal/imcontract/catalog/types.go`:
- Around line 114-131: Decouple HelpAcceptanceOnly from the hard-coded
moderation command by storing the applicable verification command on each
Contract or reusing Strategy.ReadHint, consistent with im reactions batch_query.
Update the acceptance-only help generation around HelpPolicy.Text and its
callers so each contract supplies its own verification guidance, while
preserving the existing moderation update message.
In `@internal/imcontract/http_status_test.go`:
- Around line 30-39: Add test cases for NormalizeHTTPError covering a generic
4xx status mapping to errs.SubtypeUnknown and a 4xx status with an existing
error returning that same error unchanged. Assert the relevant subtype and
object identity so regressions in both branches fail the test.
In `@internal/imcontract/identity_notice_test.go`:
- Around line 8-31: Add a nil-base test case for WithIdentityDefaultedNotice,
passing nil and asserting the returned map contains IdentityDefaultedNoticeKey
with the expected resolved identity and message. Cover the dedicated nil
handling branch while preserving the existing non-mutating merge test.
In `@internal/imcontract/materialization.go`:
- Around line 9-16: Add explicit JSON tags to MissingMessageIDs,
UnresolvedHitCount, and UnexpectedMessageCount in MaterializationStatus, using
the snake_case keys emitted by ledger(), while preserving json:"-" for
RequestedIDs, ResolvedIDs, and Cause.
In `@internal/imcontract/message_mentions_test.go`:
- Around line 22-114: The table for BuildMessageMentionResult is missing
coverage for two reachable branches. Add a case with All true and confirmable
IDs, asserting accepted_unverified with a non-empty Confirmed result, and add a
case using a non-slice response such as map[string]any, asserting
partial_unattributed from the ambiguous parseResponseMentions path.
In `@internal/imcontract/output_fallback_test.go`:
- Around line 130-140: Update
TestContentSafetyOutputFallbackUsesFixedPublicProblem to obtain the typed
problem through errs.ProblemOf(env.Error) instead of directly asserting
*errs.Problem. Assert the returned category, subtype, and param metadata as
required, and verify the original cause is preserved alongside the existing
exit-code validation.
In `@internal/imcontract/read_test.go`:
- Around line 385-401: Replace the hand-rolled scan in stringContains with the
existing strings.Contains helper, preserving the current boolean result and
leaving containsAny’s empty-value filtering unchanged. Remove stringContains if
it is no longer needed and update its call site accordingly.
In `@internal/imcontract/registry.go`:
- Around line 20-22: Move the unused stringsFrom helper from registry.go to
ledger.go alongside the evidence extraction code, preserving its signature and
behavior; do not otherwise change registry lookup logic.
In `@internal/imcontract/session.go`:
- Around line 124-126: Add named ContractKey constants for the “im
+messages-send” and “im +messages-reply” catalog entries, then update
supportsMessageMentionResult to compare against those constants instead of
string literals. Replace the corresponding required(...) literals in the catalog
with the same constants so registration and selection share a single source of
truth.
In `@internal/imcontract/write_test.go`:
- Around line 400-405: Update the ObserveRequest error assertion to extract the
error with errors.As into *errs.ValidationError, while retaining the existing
errs.ProblemOf category and subtype checks. Assert ve.Param matches the
offending request field, and verify the wrapped cause is preserved as required
by the error-path test guidelines.
- Around line 41-42: Update every listed test call to ObserveRequest in
write_test.go to capture and assert its returned error, matching the pattern
used by TestBatchPartialRecoveryMatrix. Ensure request-evidence validation
completes successfully before FinalizeSuccess or other response assertions,
including the occurrences around lines 41, 61, 75, 108, 135, 151, 365, 378, 473,
and 494.
In `@internal/imcontract/write.go`:
- Around line 140-141: Update finalizeBatch and finalizeAssertion to detect
whether the decoded response map already contains the completion key before
assigning the ledger. If present, reject the response with the established typed
internal error mechanism; otherwise preserve the existing ledger assignment and
result construction without overwriting server data.
In `@internal/output/emitter_contract_test.go`:
- Around line 152-181: The test
TestEmitterRedactedFallbackSkipsBlockedPresentationScan must verify the redacted
payload contents, not just the failure envelope. After decoding env, inspect
data.completion and assert it contains only the expected allowlisted keys, with
no payload text from the blocked result; keep the existing env.OK and env.Error
assertions unchanged.
In `@internal/output/emitter.go`:
- Around line 102-124: The Success method should automatically route hints to
stderr when emitting non-envelope formats such as pretty, table, or other
formatted output. Before calling emitHint, derive the hint options from the
selected output format while preserving explicit HintToStderr behavior and
leaving JSON/JQ envelope output unchanged.
In `@internal/output/envelope_success_test.go`:
- Around line 239-248: Extend the assertions in the partial-failure envelope
test to validate the Data payload directly, including completion.status ==
"partial". Reuse the decoded env structure and assert the nested payload is
present and has the expected status, so dropping Data causes the test to fail.
In `@internal/qualitygate/rules/run.go`:
- Line 47: Consolidate IM coverage diagnostic protection in Run and
filterPRDiagnostics by choosing one mechanism: either append imContractDiags
before filtering so the existing bypass handles them, or remove the bypass and
retain the post-filter append. Eliminate the redundant or unreachable path while
preserving IM coverage diagnostics for production callers and existing tests.
In `@shortcuts/common/runner_partial_failure_test.go`:
- Around line 108-110: Update the setup around rt.contractSession.ObserveRequest
in the test to capture and check its returned error, matching the checked form
already used at the later ObserveRequest call around line 135; fail the test
immediately when observation fails instead of discarding the error.
- Around line 91-92: Add a package-level mustLookupContract(t, key) helper that
asserts the success result from imcontract.Lookup and returns the contract.
Replace every unchecked lookup in
shortcuts/common/runner_partial_failure_test.go at lines 91-92, 108, 133, 176,
302, 321, 362, and 414, plus the lookup in
shortcuts/common/call_api_typed_test.go lines 166-177,
shortcuts/common/runner_contentsafety_test.go lines 110-111, and
shortcuts/common/runner_jq_test.go lines 182-183; each site must use the helper
so missing or renamed contracts fail the tests.
- Around line 320-323: Update the `imcontract.NewReadSession` assignments near
the `rt.readSession` setup, including the corresponding cases around lines 363
and 415, to capture and assert the returned error immediately. Make each test
fail when session construction fails before using `rt.readSession`, while
preserving the existing contract-based assertions.
In `@shortcuts/common/runner.go`:
- Around line 507-520: Extract the shared request-evidence merge into an
imcontract helper, such as RequestEvidence, that copies the request body and
applies the uuid value. Update shortcuts/common/runner.go lines 507-520 to use
the helper with query["uuid"], and cmd/service/service.go lines 473-488 to use
the same helper with request.Params["uuid"], removing both inline clone
implementations.
In `@shortcuts/im/helpers.go`:
- Around line 26-33: Remove the unused normalizeAtMentions helper and its
associated mentionFixRe declaration from shortcuts/im/helpers.go, leaving the
surrounding imports and helper functionality intact.
In `@shortcuts/im/im_chat_members_list.go`:
- Line 53: Declare a named chat-members page-limit constant next to
chatMembersListDefaultPageSize, then replace the literal 10 passed to
imPaginationFlags in the chat members list command with that constant; keep the
existing pagination behavior unchanged.
In `@shortcuts/im/mentions.go`:
- Around line 399-412: Update postMentionParagraph to compute a non-negative
paragraph capacity when targets is empty, while retaining the existing
len(targets)*2-1 capacity for non-empty requests. Ensure make cannot panic for
an empty request and preserve the current paragraph construction behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| ## messages urgent_app | ||
| Send an in-app urgent notification for an existing bot-sent message. | ||
|
|
||
| ### Avoid when | ||
| - The user asked for a phone call → use [[messages urgent_phone]] | ||
| - The user asked for SMS → use [[messages urgent_sms]] | ||
| - The message has not been sent yet → send it first with [[+messages-send]] | ||
|
|
||
| ### Prerequisites | ||
| - message_id of a message sent by the calling bot | ||
| - bot identity; the bot must still be in the conversation | ||
|
|
||
| ## messages urgent_phone | ||
| Send a phone urgent notification for an existing bot-sent message. | ||
|
|
||
| ### Avoid when | ||
| - The user asked only for an in-app prompt → use [[messages urgent_app]] | ||
| - The user asked for SMS → use [[messages urgent_sms]] | ||
|
|
||
| ### Prerequisites | ||
| - message_id of a message sent by the calling bot | ||
| - bot identity; the bot must still be in the conversation | ||
|
|
||
| ## messages urgent_sms | ||
| Send an SMS urgent notification for an existing bot-sent message. | ||
|
|
||
| ### Avoid when | ||
| - The user asked only for an in-app prompt → use [[messages urgent_app]] | ||
| - The user asked for a phone call → use [[messages urgent_phone]] | ||
|
|
||
| ### Prerequisites | ||
| - message_id of a message sent by the calling bot | ||
| - bot identity; the bot must still be in the conversation |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add example commands to the three messages urgent_* sections.
Every other section in this file ends with an ### Examples block that contains a copyable command. The messages urgent_app, messages urgent_phone, and messages urgent_sms sections stop after ### Prerequisites. Agents reading the rendered affordance get no invocation form for these three commands.
📝 Proposed addition for each urgent section
### Prerequisites
- message_id of a message sent by the calling bot
- bot identity; the bot must still be in the conversation
+
+### Examples
+
+**Send an in-app urgent notification**
+```bash
+lark-cli im messages urgent_app --message-id <message_id> --user-id-type open_id --data '{"user_id_list":["<open_id>"]}' --as bot
+```🤖 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 `@affordance/im.md` around lines 146 - 178, Add an `### Examples` block to each
of `messages urgent_app`, `messages urgent_phone`, and `messages urgent_sms`,
using a copyable command consistent with the proposed `lark-cli im messages
urgent_*` invocation and each section’s command name. Place the block after `###
Prerequisites` and preserve the existing guidance and prerequisite text.
| pageDelay := opts.PageDelay | ||
| if pageDelay == 0 { | ||
| pageDelay = 200 | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare page-limit defaulting between the legacy and status pagination loops, and list callers.
set -euo pipefail
fd -t f 'pagination.*\.go' internal/client --exec sh -c 'echo "== $1"; rg -n "PageLimit|pageLimit|PageDelay" "$1"' sh {}
echo "== callers of the status APIs"
rg -n -C4 'PaginateAllWithStatus|StreamPagesWithStatus' --glob '!internal/client/**'Repository: larksuite/cli
Length of output: 1080
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate pagination files"
fd -t f 'pagination.*\.go' internal/client
echo "== pagination.go relevant lines"
nl -ba internal/client/pagination.go | sed -n '1,140p'
echo "== pagination_status.go relevant ranges"
nl -ba internal/client/pagination_status.go | sed -n '1,280p'
echo "== internal references to PaginationOptions and status APIs"
rg -n "PaginationOptions|PaginateAllWithStatus|StreamPagesWithStatus|PageLimit\s*:" internal/clientRepository: larksuite/cli
Length of output: 362
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pagination.go relevant lines"
awk '{printf "%6d\t%s\n", NR, $0}' internal/client/pagination.go | sed -n '1,140p'
echo "== pagination_status.go relevant range"
awk '{printf "%6d\t%s\n", NR, $0}' internal/client/pagination_status.go | sed -n '1,280p'
echo "== status test relevant page-limit/default coverage"
awk '{printf "%6d\t%s\n", NR, $0}' internal/client/pagination_status_test.go | sed -n '100,230p'
echo "== internal usages"
rg -n "PaginationOptions|PaginateAllWithStatus|StreamPagesWithStatus|PageLimit\s*:" internal/client || trueRepository: larksuite/cli
Length of output: 21830
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== client.go paginate functions"
awk '{printf "%6d\t%s\n", NR, $0}' internal/client/client.go | sed -n '320,460p'
echo "== client.go existing callers"
rg -n -C5 'PaginateAll\(|StreamPages\(|PaginationOptions{' internal/client/client_test.go internal/client/client.go || trueRepository: larksuite/cli
Length of output: 5880
Align PageLimit == 0 with the documented default.
PaginationOptions.PageLimit documents 0 = unlimited (default: 10), PaginateAll()/StreamPages() default to 10 pages, and PaginateAllWithStatus() currently fetches all pages when PageLimit is zero. Apply the same default in both loops or remove/change the conflicting field documentation.
🤖 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/client/pagination_status.go` around lines 121 - 124, Update
PaginateAllWithStatus and its related pagination loop to treat
PaginationOptions.PageLimit == 0 as the documented default of 10, matching
PaginateAll and StreamPages rather than fetching unlimited pages. Preserve
explicit nonzero PageLimit values and keep the existing page-delay handling
unchanged.
| if status >= 500 { | ||
| normalized := errs.NewNetworkError( | ||
| errs.SubtypeNetworkServer, | ||
| "HTTP %d server error", | ||
| status, | ||
| ).WithCode(status).WithRetryable() | ||
| if logID != "" { | ||
| normalized.WithLogID(logID) | ||
| } | ||
| return normalized | ||
| } | ||
| if status == 429 { | ||
| normalized := errs.NewAPIError(errs.SubtypeRateLimit, "HTTP 429 rate limit").WithCode(status) | ||
| if logID != "" { | ||
| normalized.WithLogID(logID) | ||
| } | ||
| return normalized | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Preserve the original error as the cause.
The 5xx branch and the 429 branch drop the incoming err. The classification result must stay HTTP-driven, but the lower-layer business error is then lost for diagnostics and for errors.Is/errors.As checks by callers. The 4xx branch at Line 33 keeps the typed lower-layer error, so the handling is also inconsistent inside this function.
Attach the original error with .WithCause(err) when err != nil.
As per coding guidelines: "Preserve typed lower-layer errors unchanged and preserve causes with .WithCause(err)."
🐛 Proposed fix
if status >= 500 {
normalized := errs.NewNetworkError(
errs.SubtypeNetworkServer,
"HTTP %d server error",
status,
).WithCode(status).WithRetryable()
+ if err != nil {
+ normalized.WithCause(err)
+ }
if logID != "" {
normalized.WithLogID(logID)
}
return normalized
}
if status == 429 {
normalized := errs.NewAPIError(errs.SubtypeRateLimit, "HTTP 429 rate limit").WithCode(status)
+ if err != nil {
+ normalized.WithCause(err)
+ }
if logID != "" {
normalized.WithLogID(logID)
}
return normalized
}📝 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.
| if status >= 500 { | |
| normalized := errs.NewNetworkError( | |
| errs.SubtypeNetworkServer, | |
| "HTTP %d server error", | |
| status, | |
| ).WithCode(status).WithRetryable() | |
| if logID != "" { | |
| normalized.WithLogID(logID) | |
| } | |
| return normalized | |
| } | |
| if status == 429 { | |
| normalized := errs.NewAPIError(errs.SubtypeRateLimit, "HTTP 429 rate limit").WithCode(status) | |
| if logID != "" { | |
| normalized.WithLogID(logID) | |
| } | |
| return normalized | |
| } | |
| if status >= 500 { | |
| normalized := errs.NewNetworkError( | |
| errs.SubtypeNetworkServer, | |
| "HTTP %d server error", | |
| status, | |
| ).WithCode(status).WithRetryable() | |
| if err != nil { | |
| normalized.WithCause(err) | |
| } | |
| if logID != "" { | |
| normalized.WithLogID(logID) | |
| } | |
| return normalized | |
| } | |
| if status == 429 { | |
| normalized := errs.NewAPIError(errs.SubtypeRateLimit, "HTTP 429 rate limit").WithCode(status) | |
| if err != nil { | |
| normalized.WithCause(err) | |
| } | |
| if logID != "" { | |
| normalized.WithLogID(logID) | |
| } | |
| return normalized | |
| } |
🤖 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/imcontract/http_status.go` around lines 15 - 32, Update the 5xx and
429 handling in the HTTP status normalization function to attach the incoming
err via WithCause(err) whenever err is non-nil, while preserving the existing
HTTP-driven classifications and metadata. Keep nil errors unattached and align
this behavior with the existing 4xx branch.
Source: Coding guidelines
| {name: "unknown status", mention: validMentionResult("mystery"), wantErr: true}, | ||
| {name: "replay scope cannot authorize replay", mention: MessageMentionResult{ | ||
| Status: "partial", Requested: []string{"ou_a"}, Confirmed: []MessageMentionConfirmation{}, | ||
| Missing: []string{"ou_a"}, All: "not_requested", RetryScope: "whole_request", | ||
| }, wantErr: true}, |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert typed error metadata on the two error-path cases.
The unknown status and replay scope cannot authorize replay cases only check that err is non-nil. The repository test guideline requires error-path tests to assert the typed metadata through errs.ProblemOf and to verify cause preservation. Add expected category and subtype fields to the table and assert them, so a change from invalidEvidence to another error class fails the test.
💚 Proposed test change
tests := []struct {
name string
mention any
wantOK bool
wantExit int
wantErr bool
+ wantCategory errs.Category
+ wantSubtype errs.Subtype
}{Then in the body:
if (err != nil) != tt.wantErr {
t.Fatalf("FinalizeSuccess() error = %v, wantErr %v", err, tt.wantErr)
}
if err != nil {
+ problem, ok := errs.ProblemOf(err)
+ if !ok {
+ t.Fatalf("error is not typed: %v", err)
+ }
+ if problem.Category != tt.wantCategory || problem.Subtype != tt.wantSubtype {
+ t.Fatalf("problem = %#v", problem)
+ }
return
}Based on coding guidelines: "Error-path tests must assert typed metadata through errs.ProblemOf (category, subtype, and param) and verify cause preservation rather than relying only on message substrings."
Also applies to: 169-174
🤖 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/imcontract/message_mentions_test.go` around lines 155 - 159, Update
the error-path table cases “unknown status” and “replay scope cannot authorize
replay” with expected category and subtype metadata, then extend the test
assertions to inspect the typed error via errs.ProblemOf and verify category,
subtype, param, and cause preservation. Replace the existing err-only checks
while keeping the current successful-case behavior unchanged.
Source: Coding guidelines
| func allowlistedCompletion(data any) map[string]any { | ||
| summary := map[string]any{ | ||
| "status": "complete", | ||
| "retry_scope": "none", | ||
| } | ||
| root, ok := data.(map[string]any) | ||
| if !ok { | ||
| return summary | ||
| } | ||
| completionValue, hasCompletion := root["completion"] | ||
| switch completion := completionValue.(type) { | ||
| case Completion: | ||
| copyCompletionStatus(summary, completion.Status) | ||
| summary["requested_count"] = completion.RequestedCount | ||
| summary["succeeded_count"] = completion.SucceededCount | ||
| summary["failed_count"] = completion.FailedCount | ||
| summary["pending_count"] = completion.PendingCount | ||
| copyCompletionRetryScope(summary, completion.RetryScope) | ||
| return summary | ||
| case map[string]any: | ||
| if value, ok := completion["status"].(string); ok { | ||
| copyCompletionStatus(summary, value) | ||
| } | ||
| copyCompletionCount(summary, completion, "requested_count") | ||
| copyCompletionCount(summary, completion, "succeeded_count") | ||
| copyCompletionCount(summary, completion, "failed_count") | ||
| copyCompletionCount(summary, completion, "pending_count") | ||
| if value, exists := completion["final_state_verified"]; exists { | ||
| if verified, valid := value.(bool); valid { | ||
| summary["final_state_verified"] = verified | ||
| } | ||
| } | ||
| if value, ok := completion["retry_scope"].(string); ok { | ||
| copyCompletionRetryScope(summary, value) | ||
| } | ||
| } | ||
| if !hasCompletion { | ||
| if mention, ok := root["mention_result"].(MessageMentionResult); ok { | ||
| copyCompletionStatus(summary, mention.Status) | ||
| copyCompletionRetryScope(summary, mention.RetryScope) | ||
| } | ||
| } | ||
| return summary | ||
| } | ||
|
|
||
| func copyCompletionStatus(dst map[string]any, value string) { | ||
| switch value { | ||
| case "complete", "partial", "accepted_unverified", "partial_unattributed": | ||
| dst["status"] = value | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Do not default the fallback status to complete when the real status is unrecognized.
allowlistedCompletion seeds status: "complete" and retry_scope: "none". copyCompletionStatus only overwrites the status for four known values. If the completion carries any other status (for example a future status string), the fallback reports complete with retry_scope: "none" while the counts can still show failures. An agent then skips recovery. Prefer a conservative default, such as accepted_unverified, or omit the fields when the source status is unknown.
🛡️ Proposed conservative default
func allowlistedCompletion(data any) map[string]any {
summary := map[string]any{
- "status": "complete",
- "retry_scope": "none",
+ "status": "accepted_unverified",
+ "retry_scope": "whole_request",
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| func allowlistedCompletion(data any) map[string]any { | |
| summary := map[string]any{ | |
| "status": "complete", | |
| "retry_scope": "none", | |
| } | |
| root, ok := data.(map[string]any) | |
| if !ok { | |
| return summary | |
| } | |
| completionValue, hasCompletion := root["completion"] | |
| switch completion := completionValue.(type) { | |
| case Completion: | |
| copyCompletionStatus(summary, completion.Status) | |
| summary["requested_count"] = completion.RequestedCount | |
| summary["succeeded_count"] = completion.SucceededCount | |
| summary["failed_count"] = completion.FailedCount | |
| summary["pending_count"] = completion.PendingCount | |
| copyCompletionRetryScope(summary, completion.RetryScope) | |
| return summary | |
| case map[string]any: | |
| if value, ok := completion["status"].(string); ok { | |
| copyCompletionStatus(summary, value) | |
| } | |
| copyCompletionCount(summary, completion, "requested_count") | |
| copyCompletionCount(summary, completion, "succeeded_count") | |
| copyCompletionCount(summary, completion, "failed_count") | |
| copyCompletionCount(summary, completion, "pending_count") | |
| if value, exists := completion["final_state_verified"]; exists { | |
| if verified, valid := value.(bool); valid { | |
| summary["final_state_verified"] = verified | |
| } | |
| } | |
| if value, ok := completion["retry_scope"].(string); ok { | |
| copyCompletionRetryScope(summary, value) | |
| } | |
| } | |
| if !hasCompletion { | |
| if mention, ok := root["mention_result"].(MessageMentionResult); ok { | |
| copyCompletionStatus(summary, mention.Status) | |
| copyCompletionRetryScope(summary, mention.RetryScope) | |
| } | |
| } | |
| return summary | |
| } | |
| func copyCompletionStatus(dst map[string]any, value string) { | |
| switch value { | |
| case "complete", "partial", "accepted_unverified", "partial_unattributed": | |
| dst["status"] = value | |
| } | |
| } | |
| func allowlistedCompletion(data any) map[string]any { | |
| summary := map[string]any{ | |
| "status": "accepted_unverified", | |
| "retry_scope": "whole_request", | |
| } | |
| root, ok := data.(map[string]any) | |
| if !ok { | |
| return summary | |
| } | |
| completionValue, hasCompletion := root["completion"] | |
| switch completion := completionValue.(type) { | |
| case Completion: | |
| copyCompletionStatus(summary, completion.Status) | |
| summary["requested_count"] = completion.RequestedCount | |
| summary["succeeded_count"] = completion.SucceededCount | |
| summary["failed_count"] = completion.FailedCount | |
| summary["pending_count"] = completion.PendingCount | |
| copyCompletionRetryScope(summary, completion.RetryScope) | |
| return summary | |
| case map[string]any: | |
| if value, ok := completion["status"].(string); ok { | |
| copyCompletionStatus(summary, value) | |
| } | |
| copyCompletionCount(summary, completion, "requested_count") | |
| copyCompletionCount(summary, completion, "succeeded_count") | |
| copyCompletionCount(summary, completion, "failed_count") | |
| copyCompletionCount(summary, completion, "pending_count") | |
| if value, exists := completion["final_state_verified"]; exists { | |
| if verified, valid := value.(bool); valid { | |
| summary["final_state_verified"] = verified | |
| } | |
| } | |
| if value, ok := completion["retry_scope"].(string); ok { | |
| copyCompletionRetryScope(summary, value) | |
| } | |
| } | |
| if !hasCompletion { | |
| if mention, ok := root["mention_result"].(MessageMentionResult); ok { | |
| copyCompletionStatus(summary, mention.Status) | |
| copyCompletionRetryScope(summary, mention.RetryScope) | |
| } | |
| } | |
| return summary | |
| } | |
| func copyCompletionStatus(dst map[string]any, value string) { | |
| switch value { | |
| case "complete", "partial", "accepted_unverified", "partial_unattributed": | |
| dst["status"] = value | |
| } | |
| } |
🤖 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/imcontract/output_fallback.go` around lines 42 - 92, Update
allowlistedCompletion to use a conservative fallback status instead of seeding
status as "complete"; use the established "accepted_unverified" value (and avoid
implying retry_scope "none" when the source status is unknown). Preserve
copyCompletionStatus behavior for recognized statuses while ensuring
unrecognized completion statuses cannot be reported as complete and bypass
recovery.
| result := map[string]interface{}{ | ||
| "message_id": resData["message_id"], | ||
| "chat_id": resData["chat_id"], | ||
| "create_time": common.FormatTimeWithSeconds(resData["create_time"]), | ||
| }, nil) | ||
| } | ||
| if err := addMessageMentionResult(runtime, resData, result); err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Mention post-processing can report failure for a completed write. Both shortcuts call addMessageMentionResult after the message was accepted by the server, and that function re-parses the mention flags and can return a validation error. The command then exits with an error even though the message was sent. The mention request is already parsed and validated inside buildMessageRequestBody before the write, so the failure is unreachable today; make the post-write path unable to fail instead of relying on that.
shortcuts/im/im_messages_send.go#L226-L233: parse the mention request once before the write and pass the resultingmentionRequestinto the result builder, so no error can be returned afterDoWriteAPIJSONTypedsucceeds.shortcuts/im/im_messages_reply.go#L202-L209: apply the same change, reusing the mention request computed before the reply is sent.
📍 Affects 2 files
shortcuts/im/im_messages_send.go#L226-L233(this comment)shortcuts/im/im_messages_reply.go#L202-L209
🤖 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 `@shortcuts/im/im_messages_send.go` around lines 226 - 233, Make mention
post-processing non-failing after a successful write by parsing the mention
request once before the write in shortcuts/im/im_messages_send.go lines 226-233
and passing that mentionRequest into the result builder instead of calling
addMessageMentionResult to re-parse it; apply the same change in
shortcuts/im/im_messages_reply.go lines 202-209, reusing the mention request
computed before sending the reply.
| CHAT_ID=$(lark-cli im +chat-create --name "New Group" --idempotency-key <generated_uuid> --format json | jq -r '.data.chat_id') | ||
| lark-cli im +messages-send --chat-id "$CHAT_ID" --text "Welcome, everyone!" --as bot |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 5 'DefaultAs|default.*(identity|as)|im \+chat-create|chat-create' --glob '*.go' --glob '*.md'Repository: larksuite/cli
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Tracked files matching chat-create/lark-cli references:\n'
git ls-files | rg '(^skills/lark-im/references/lark-im-chat-create\.md|chat-create|lark-cli|cli)' | head -200
printf '\nDocument excerpt:\n'
sed -n '130,155p' skills/lark-im/references/lark-im-chat-create.md
printf '\nBroader search for --as / bot / identity default:\n'
rg -n --glob '*.go' --glob '*.md' '('\''--as'\''|\b--as\b|\bdefault[ _-]?[A-Za-z]*(identity|user|bot)|Default[A-Za-z]*(Identity|User)|chat-create|messages-send|messages-send --as|DefaultAs)' | head -300Repository: larksuite/cli
Length of output: 10768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Files containing --as:\n'
rg -n --glob '*.go' --glob '*.md' --glob '*.sh' '\b--as\b' . | head -300
printf '\nFiles containing Default[A-Za-z]+User / Default[A-Za-z]+Identity / Default[A-Za-z]+Bot:\n'
rg -n --glob '*.go' --glob '*.md' 'Default[A-Za-z]+(User|Identity|Bot|As)|default.*as=default|as.*default' . | head -300
printf '\nChat create command implementation candidates:\n'
git ls-files | rg '(^.*chat.*\.go$|chat.*\.go$)' | xargs -r rg -n 'chat-create|ChatCreate|CreateChat|messages-send|MessageSend|\\b-as\\b|--as|Bot|bot' | head -400Repository: larksuite/cli
Length of output: 174
Make the bot identity explicit in the create command.
Scenario 3 creates the chat without --as, then sends the welcome message with --as bot. Use --as bot for the create command too so the same identity both creates the chat and sends the message.
🤖 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 `@skills/lark-im/references/lark-im-chat-create.md` around lines 145 - 146,
Update the chat creation command in Scenario 3 to include the explicit --as bot
option, matching the identity used by the subsequent messages-send command.
| If a bot creates a group and `--users` includes users who are mutually invisible to the bot, the entire request fails with 232043. Use two steps instead: | ||
|
|
||
| 1. Create the group with the bot first, excluding invisible users: `lark-cli im +chat-create --name "Group Name"` | ||
| 1. Generate a UUID once with a UUID library or tool, then create the group with the bot first, excluding invisible users: `lark-cli im +chat-create --name "Group Name" --idempotency-key <generated_uuid>` |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pin bot identity throughout bot workflows.
The examples describe bot actions but do not consistently use bot identity. A default user identity can create or discover a chat that the bot cannot access.
skills/lark-im/references/lark-im-chat-identity.md#L43-L43: Add--as botto the group-creation command.skills/lark-im/references/lark-im-chat-search.md#L107-L107: Add--as botto the preceding+chat-searchcommand, or run both commands as user.
Proposed fix
-lark-cli im +chat-create --name "Group Name" --idempotency-key <generated_uuid>
+lark-cli im +chat-create --as bot --name "Group Name" --idempotency-key <generated_uuid>
-CHAT_ID=$(lark-cli im +chat-search --query "daily report" --format json | jq -r '.data.chats[0].chat_id')
+CHAT_ID=$(lark-cli im +chat-search --as bot --query "daily report" --format json | jq -r '.data.chats[0].chat_id')📍 Affects 2 files
skills/lark-im/references/lark-im-chat-identity.md#L43-L43(this comment)skills/lark-im/references/lark-im-chat-search.md#L107-L107
🤖 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 `@skills/lark-im/references/lark-im-chat-identity.md` at line 43, Pin bot
identity consistently in the documented chat workflows: update the
group-creation command in
skills/lark-im/references/lark-im-chat-identity.md:43-43 to run as bot, and
update the preceding +chat-search command in
skills/lark-im/references/lark-im-chat-search.md:107-107 to run as bot as well,
keeping both operations under the same identity.
| Lists the **current user's** feed shortcuts. | ||
|
|
||
| - Only **CHAT-type** shortcuts are exposed via OpenAPI today (others in the IDL are not yet whitelisted). | ||
| - The shortcut is a **thin one-page wrapper** — there is no built-in auto-pagination. Callers drive their own loop when they actually need to paginate. | ||
| - Server-side page size is controlled by the service; in normal use one page usually covers the list. | ||
| - Pagination tokens are opaque. If a token is rejected because the shortcut list changed, restart by omitting `--page-token`. | ||
| - Pagination controls are defined by the concrete command's `--help`. | ||
| - The default call is bounded. For an exhaustive task, inspect the leaf help and require `meta.complete=true` before reporting a complete list. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Make the exhaustive-read contract explicit in every paginated IM reference.
The documentation either requires completion metadata without documenting its location or describes page merging without stating the completion gate. A bounded result can therefore be mistaken for a complete result.
skills/lark-im/references/lark-im-feed-shortcut-list.md#L9-L13: documentmeta.completein the response structure and JSON example.skills/lark-im/references/lark-im-feed-group-list.md#L3-L5: describe merging as covering fetched pages and requiremeta.complete=truefor an exhaustive list.skills/lark-im/references/lark-im-feed-group-list.md#L26-L37: document the completion check beside pagination controls and output.skills/lark-im/references/lark-im-feed-group-list-item.md#L31-L39: documentmeta.complete=trueand distinguish merged pages from a complete dataset.skills/lark-im/references/lark-im-feed-groups.md#L33-L33: add the completion condition to the shortcut overview.skills/lark-im/references/lark-im-feed-groups.md#L245-L245: add the completion condition tolist.skills/lark-im/references/lark-im-feed-groups.md#L329-L329: add the completion condition tolist_item.skills/lark-im/references/lark-im-flag-list.md#L9-L9: replace “result reports complete” with the exactmeta.complete=truecheck and document its output path.
📍 Affects 5 files
skills/lark-im/references/lark-im-feed-shortcut-list.md#L9-L13(this comment)skills/lark-im/references/lark-im-feed-group-list.md#L3-L5skills/lark-im/references/lark-im-feed-group-list.md#L26-L37skills/lark-im/references/lark-im-feed-group-list-item.md#L31-L39skills/lark-im/references/lark-im-feed-groups.md#L33-L33skills/lark-im/references/lark-im-feed-groups.md#L245-L245skills/lark-im/references/lark-im-feed-groups.md#L329-L329skills/lark-im/references/lark-im-flag-list.md#L9-L9
🤖 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 `@skills/lark-im/references/lark-im-feed-shortcut-list.md` around lines 9 - 13,
Make the exhaustive-read contract explicit across all listed references: in
skills/lark-im/references/lark-im-feed-shortcut-list.md lines 9-13, document
meta.complete in the response structure and JSON example; in
skills/lark-im/references/lark-im-feed-group-list.md lines 3-5 and 26-37, state
that merging covers fetched pages only and require meta.complete=true beside
pagination and output guidance; in
skills/lark-im/references/lark-im-feed-group-list-item.md lines 31-39,
distinguish merged pages from a complete dataset and document the
meta.complete=true gate; in skills/lark-im/references/lark-im-feed-groups.md
lines 33, 245, and 329, add the same completion condition to the overview, list,
and list_item documentation; and in
skills/lark-im/references/lark-im-flag-list.md line 9, replace the vague
completion wording with the exact meta.complete=true check and its output path.
| - `--content` must be valid JSON | ||
| - When using `--content`, you are responsible for making the JSON structure match the effective `msg_type` | ||
| - `--image`/`--file`/`--video`/`--audio` support existing keys, URLs, and cwd-relative local file paths; the shortcut uploads local paths and URLs first, then sends the message; both the upload and send steps use the same identity (UAT when `--as user`, TAT when `--as bot`) | ||
| - If an upload fails (URL media or a markdown image), **nothing is sent** — the command fails with a recovery hint. The CLI never downgrades content on its own (e.g. replacing a failed image with a text link); any degraded form must be shown to the user and re-sent explicitly after their approval |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use one remote Markdown-image failure contract across both references.
Both references say that a failed remote image upload sends nothing, while their earlier rules say that the image is removed and the message continues with a warning. Align all statements with the CLI behavior to prevent duplicate sends.
skills/lark-im/references/lark-im-messages-send.md#L267-L267: align Line 267 with Lines 72 and 277, and document the selected behavior consistently.skills/lark-im/references/lark-im-messages-reply.md#L267-L267: align Line 267 with Lines 73 and 276, and document the selected behavior consistently.
📍 Affects 2 files
skills/lark-im/references/lark-im-messages-send.md#L267-L267(this comment)skills/lark-im/references/lark-im-messages-reply.md#L267-L267
🤖 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 `@skills/lark-im/references/lark-im-messages-send.md` at line 267, Align the
remote Markdown-image upload failure contract in both
skills/lark-im/references/lark-im-messages-send.md:267 and
skills/lark-im/references/lark-im-messages-reply.md:267 with the existing rules
at Lines 72/277 and 73/276: remove the failed image, continue sending the
message, and issue the documented warning. Ensure both references describe the
same behavior and do not claim that nothing is sent or that the command fails.
1 participant
Summary
IM commands could report success without enough evidence for an agent to know whether a write completed, a read was exhaustive, or a retry was safe. This standalone PR includes and supersedes #1880, makes completion contracts mandatory across IM commands, makes defaulted write identity observable, and keeps IM help guidance consistent without changing non-IM behavior.
Changes
internal/imcontractinternal/output.Emitter, including structured error, hint, completeness metadata, and redacted presentation fallbackim +chat-createcallers to provide a caller-owned--idempotency-key; add structured individual and all-member mentions toim +messages-sendandim +messages-replyidentity_defaultednotice and stderr warning when a dual-identity contract-managed IM write omits--asaffordance/im.md, andskills/lark-imTest Plan
go vetandgit diff --checkpassedmainmention_resultandretry_scope)Related PRs
Summary by CodeRabbit