feat: add lazy API catalog routing - #2232
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (133)
💤 Files with no reviewable changes (27)
🚧 Files skipped from review as they are similar to previous changes (105)
📝 WalkthroughWalkthroughThis PR replaces fetched runtime metadata with a validated embedded API Catalog Snapshot. It adds target-aware command assembly, explicit catalog propagation, exact JSON-number handling, catalog publication checks, and selected shortcut registration. ChangesCatalog Snapshot Migration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 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/calendar
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@be38883817c0eb75dfb8756be2726f31bffa584f🧩 Skill updatenpx skills add larksuite/cli#feat/catalog-lazy-routing -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: 4
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (17)
cmd/schema/schema_test.go-72-78 (1)
72-78: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert catalog-scoped execution directly.
driveresolves with both the drive-only catalog and the full catalog. This assertion can pass if schema execution ignoresf.APICatalog.Run
imand assert an unknown-service validation error, or run without arguments and assert that every emitted envelope belongs todrive. As per coding guidelines, “Every behavior change must have an accompanying contract test that fails if the implementation is reverted and asserts the changed field or behavior directly.”🤖 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/schema/schema_test.go` around lines 72 - 78, Strengthen the schema command contract test around cmd.Execute so it verifies APICatalog scoping directly: execute the command with “im” and assert the expected unknown-service validation error, or execute without arguments and confirm every emitted envelope belongs to “drive.” Keep the assertion focused on the catalog-selected behavior rather than merely checking for drive text in the output.Source: Coding guidelines
tests/plugin_e2e/degrade_subsystem_test.go-40-41 (1)
40-41: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert the schema service field structurally.
strings.Containsaccepts malformed JSON and unrelated nested text. Parseres.stdoutand assert that the returned schema service name is exactlyim.As per coding guidelines, “Every behavior change must have an accompanying contract test that fails if the implementation is reverted and asserts the changed field or behavior directly.”
🤖 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 `@tests/plugin_e2e/degrade_subsystem_test.go` around lines 40 - 41, Replace the substring check in the schema output test with JSON parsing of res.stdout, then assert the parsed schema’s service name field equals exactly "im". Keep the test failure reporting tied to malformed output or an incorrect field value so it directly validates the committed service.Source: Coding guidelines
internal/cmdutil/fileupload_test.go-381-387 (1)
381-387: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAssert generic typed metadata with
errs.ProblemOf.Lines 381-387 inspect
Subtypeonly through*errs.ValidationError. AssertSubtypethrougherrs.ProblemOf(err). Keeperrors.Asfor theParamassertion.Proposed test update
+ problem, ok := errs.ProblemOf(err) + if !ok { + t.Fatalf("expected typed problem, got %T: %v", err, err) + } + if problem.Subtype != errs.SubtypeInvalidArgument { + t.Fatalf("problem subtype = %q, want %q", problem.Subtype, errs.SubtypeInvalidArgument) + } var validationErr *errs.ValidationError if !errors.As(err, &validationErr) { t.Fatalf("expected typed validation error, got %T: %v", err, err) } - if validationErr.Subtype != errs.SubtypeInvalidArgument || validationErr.Param != "--data" { - t.Fatalf("validation error = %#v, want invalid_argument for --data", validationErr) + if validationErr.Param != "--data" { + t.Fatalf("validation error param = %q, want --data", validationErr.Param) }As per coding guidelines,
**/*_test.go: Error-path tests must assert typed metadata viaerrs.ProblemOfand 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/cmdutil/fileupload_test.go` around lines 381 - 387, Update the validation-error assertions in the affected test to obtain generic typed metadata from errs.ProblemOf(err) and assert the expected invalid-argument subtype through that result. Retain errors.As with *errs.ValidationError specifically to verify Param is "--data", and preserve the existing failure diagnostics.Source: Coding guidelines
internal/qualitygate/publiccontent/catalog_test.go-7-8 (1)
7-8: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUse
internal/vfsfor the catalog read.Line 23 uses
os.ReadFilein an internal package test. Replace the directosfilesystem call with the repositoryvfsAPI. This keeps filesystem behavior consistent with the internal test policy.As per coding guidelines, “Use
internal/vfsfilesystem APIs instead ofos.” Based on learnings, “for any Go test file underinternal/… useinternal/vfsfor filesystem access.”Also applies to: 23-25
🤖 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/publiccontent/catalog_test.go` around lines 7 - 8, Replace the direct os.ReadFile call in the catalog test with the repository’s internal/vfs file-reading API, updating imports accordingly and removing the now-unused os import. Preserve the existing catalog test behavior and error handling.Sources: Coding guidelines, Learnings
internal/qualitygate/publiccontent/collect.go-41-43 (1)
41-43: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReturn a typed error for the catalog read failure.
Line 43 returns the raw
gitOutputerror. IfreadErris already typed, return it unchanged. Otherwise wrap it witherrs.NewInternalError(errs.SubtypeUnknown, ...).WithCause(readErr). This preserves the cause and the quality-gate error contract.As per coding guidelines, “Command-facing failures must use typed
errs.*errors” and “Preserve lower-layer typed errors, attach causes 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 `@internal/qualitygate/publiccontent/collect.go` around lines 41 - 43, Update the gitOutput error handling in the catalog collection flow around readErr: return readErr unchanged when it is already a typed errs error; otherwise wrap it with errs.NewInternalError(errs.SubtypeUnknown, ...) and attach readErr via WithCause(readErr), preserving the quality-gate error contract.Source: Coding guidelines
internal/registry/catalog/services/attendance.json-383-383 (1)
383-383: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
riskiswritefor a read-only query.
user_task.queryreads attendance results. The entry sets"risk": "write". See the consolidated comment for the shared cause.🤖 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/registry/catalog/services/attendance.json` at line 383, Update the attendance service entry for user_task.query to set its risk value to read instead of write, while leaving the query definition unchanged.internal/registry/catalog/services/contact.json-166-167 (1)
166-167: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
danger: trueis set on a read method.
user_profiles.batch_queryreads profile data and declares"risk": "read". It also sets"danger": true. See the consolidated comment for the shared cause across shards.🤖 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/registry/catalog/services/contact.json` around lines 166 - 167, Update the user_profiles.batch_query entry in the service catalog to remove danger: true while preserving risk: "read" and the method’s existing read behavior.internal/registry/catalog/services/drive.json-770-771 (1)
770-771: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
danger: trueis set on read methods.
file.comments.batch_querydeclares"risk": "read"and also"danger": true.metas.batch_queryat lines 3742-3743 has the same combination. See the consolidated comment.🤖 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/registry/catalog/services/drive.json` around lines 770 - 771, Update the metadata for the read-only batch_query methods, including file.comments.batch_query and metas.batch_query, to remove the danger: true flag while preserving risk: "read".internal/registry/catalog/services/calendar.json-6442-6444 (1)
6442-6444: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSame
danger/riskcontradiction and emptydocUrl.
calendar.event.share_inforeads a share link. It sets"danger": truewith"risk": "read"and an emptydocUrl. See the consolidated comment for the shared cause and the fix.🤖 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/registry/catalog/services/calendar.json` around lines 6442 - 6444, Update the calendar.event.share_info catalog entry so its danger and risk values consistently represent a read-only share-link operation, and replace the empty docUrl with the appropriate documentation URL used by the related calendar tools.internal/registry/catalog/services/approval.json-95-95 (1)
95-95: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConfirm the
riskvalue for a read-only search.
approvals.searchcallsapprovals/search_launchable, which only lists launchable approval definitions. The entry sets"risk": "write". Every other read operation in this file uses"risk": "read". The value looks derived from thePOSTverb rather than from the effect. If risk drives confirmation prompts or policy gating, this misclassifies a read as a write.The same pattern exists in
internal/registry/catalog/services/attendance.jsonforuser_task.query.🔧 Proposed change
- "risk": "write", + "risk": "read",🤖 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/registry/catalog/services/approval.json` at line 95, Update the risk classification for the read-only approvals.search operation to "read" instead of "write", and apply the same correction to attendance.json's user_task.query entry. Keep the operation definitions and HTTP methods unchanged.internal/registry/catalog/services/drive.json-452-452 (1)
452-452: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a description for
files.patch.The
descriptionfield is empty. Every other method in this shard has a description. The catalog description supplies the command summary in help output, sodrive files patchwill show no summary.🔧 Proposed change
- "description": "", + "description": "修改文件标题",🤖 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/registry/catalog/services/drive.json` at line 452, Add a concise, user-facing description to the files.patch catalog entry’s description field, matching the style of neighboring drive file methods and summarizing the command’s purpose for help output.internal/registry/catalog/services/calendar.json-1652-1652 (1)
1652-1652: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRemove the
<redacted>marker from the parameter description.The description of
instance_start_time_admincontains the literal text`<redacted>_1713168000`. This is a redaction artifact, not an example value. The catalog descriptions are rendered in command help, so users see the marker. Replace it with a placeholder event ID.🔧 Proposed change
- "description": "The instance to be modified when accessing as an administrator.; Note:; - This parameter is only used to modify a event instance in a repeating event. This field does not need to be filled in for non-repeating events.;- You can call the [Get repeating event instance](https://open.feishu.cn/document/uAjLw4CM/ukTMukTMukTM/reference/calendar-v4/calendar-event/instances) interface to obtain the event_id of a event instance in the repeating event. The value of this parameter is the timestamp suffix of event_id. For example, the queried event instance ID is `<redacted>_1713168000`, then the current `instance_start_time_admin` value is `1713168000`.; Default: empty", + "description": "The instance to be modified when accessing as an administrator.; Note:; - This parameter is only used to modify a event instance in a repeating event. This field does not need to be filled in for non-repeating events.;- You can call the [Get repeating event instance](https://open.feishu.cn/document/uAjLw4CM/ukTMukTMukTM/reference/calendar-v4/calendar-event/instances) interface to obtain the event_id of a event instance in the repeating event. The value of this parameter is the timestamp suffix of event_id. For example, the queried event instance ID is `xxxxxxxxx_1713168000`, then the current `instance_start_time_admin` value is `1713168000`.; Default: empty",🤖 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/registry/catalog/services/calendar.json` at line 1652, Update the instance_start_time_admin description in the calendar catalog to replace the literal `<redacted>` marker in the example event ID with a neutral placeholder event ID, while preserving the surrounding explanation and timestamp suffix.internal/registry/catalog/services/okr.json-1372-1375 (1)
1372-1375: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winCorrect the
itemsdescription.
itemsis the Objective list forcycle.objectives.list. The description says "Error code, non-zero means failure". Every sibling list endpoint in this file describesitemsas the returned list, for example line 3037 and line 4541.📝 Proposed fix
"items": { "type": "array", - "description": "Error code, non-zero means failure", + "description": "Objective list", "required": 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/registry/catalog/services/okr.json` around lines 1372 - 1375, Update the description of the items property in the cycle.objectives.list response schema to describe it as the returned Objective list, matching the wording pattern used by sibling list endpoints in the catalog; remove the unrelated error-code description.internal/registry/catalog/services/mail.json-5819-5821 (1)
5819-5821: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAlign the template delete risk level with the other delete operations.
user_mailbox.template.deleteuses"risk": "write". Every other permanent delete in this file uses"high-risk-write": draft delete (line 722), folder delete (line 1607), label delete (line 2006), and mail contact delete (line 2415). The description states the deletion cannot be undone.🔧 Proposed fix
"danger": true, - "risk": "write", + "risk": "high-risk-write", "docUrl": "https://open.feishu.cn/api-explorer?from=op_doc_tab&apiName=delete&project=mail&resource=user_mailbox.template&version=v1"🤖 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/registry/catalog/services/mail.json` around lines 5819 - 5821, Update the risk value for the user_mailbox.template.delete entry from "write" to "high-risk-write", matching the other permanent delete operations while preserving its existing danger flag and docUrl.internal/registry/catalog/services/okr.json-1286-1355 (1)
1286-1355: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the shifted parameter descriptions in
cycle.objectives.list.The descriptions are offset by one parameter across this whole block:
cycle_id(line 1290) describes a department ID.page_size(line 1296) describes the user cycle ID, and its example7342342398472398471is a cycle ID, not a page size.department_id_type(line 1338) describes page size, its example is10, and its two option descriptions describe a page token and a department ID type.Users read these strings as flag help for
okr cycle.objectives list.📝 Proposed fix
"cycle_id": { "type": "string", "location": "path", "required": true, - "description": "Identify department by department_id. The ID type must match the value of the department_id_type parameter" + "description": "User cycle ID. Obtain by calling the \"Get user OKR Cycle list\" API. The ID type must match the value of the user_id_type parameter" }, "page_size": { "type": "integer", "location": "query", "required": false, - "description": "User cycle ID. Obtain by calling the \"Get user OKR Cycle list\" API. The ID type must match the value of the user_id_type parameter", + "description": "Page size, maximum 100", "default": "10", - "example": "7342342398472398471", + "example": "10", "max": "100", "min": "1" },"department_id_type": { "type": "string", "location": "query", "required": false, - "description": "Page size, maximum 100", + "description": "Department ID type used in this call", "default": "open_department_id", - "example": "10", + "example": "open_department_id", "options": [ { "value": "department_id", - "description": "Page token. Leave empty for the first request to start from the beginning. When there are more items, a new page_token will be returned. Use this page_token for the next request to get more results." + "description": "use department_id for department" }, { "value": "open_department_id", - "description": "Department ID type used in this call" + "description": "use open_department_id for department" } ],🤖 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/registry/catalog/services/okr.json` around lines 1286 - 1355, Correct the parameter metadata for cycle.objectives.list: make cycle_id describe the user cycle ID, make page_size describe page size with example 10, and make department_id_type describe the department ID type. Replace the department_id_type option descriptions with accurate descriptions for department_id and open_department_id, removing the misplaced page-token text and cycle-ID example.internal/registry/catalog/services/im.json-505-515 (1)
505-515: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winSwap the
group_message_typeandchat_modedescriptions.The descriptions are exchanged.
group_message_typedescribes group editing permissions, andchat_modedescribes the group message type. Users see these strings inschemaoutput and help.📝 Proposed fix
"group_message_type": { "type": "string", - "description": "Group editing permissions; Optional values are:;- `only_owner`: only group owner and administrators;- `all_members`: all members", + "description": "Group message type; Optional values are:;- `chat`: chat;- `thread`: thread", "required": false }, "chat_mode": { "type": "string", - "description": "group message type; Optional values are:;- `chat`: chat;- `thread`: thread", + "description": "Group mode; Optional values are:;- `group`: A group", "required": false, "example": "group" },🤖 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/registry/catalog/services/im.json` around lines 505 - 515, Correct the descriptions for the group_message_type and chat_mode schema properties: assign the group message type description to group_message_type and the group editing permissions description to chat_mode, while leaving their types, required flags, and example unchanged.cmd/auth/login.go-93-101 (1)
93-101: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse the same domain source for completion.
allKnownDomainsincludes brand-eligible shortcut services, butcompleteDomainonly uses catalog services. For example, Feishu acceptsapps, while completion cannot suggest it. Pass the brand intocompleteDomainand build candidates fromsortedKnownDomains.Proposed fix
- return completeDomain(f.APICatalog, toComplete), cobra.ShellCompDirectiveNoFileComp + return completeDomain(f.APICatalog, helpBrand, toComplete), cobra.ShellCompDirectiveNoFileComp -func completeDomain(catalog apicatalog.Catalog, toComplete string) []string { - allDomains := catalogServiceNames(catalog) +func completeDomain(catalog apicatalog.Catalog, brand core.LarkBrand, toComplete string) []string { + allDomains := sortedKnownDomains(catalog, brand)Add completion tests for
appson Feishu and its absence on Lark.🤖 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/auth/login.go` around lines 93 - 101, The completeDomain function currently derives candidates only from catalog services, omitting brand-specific shortcut domains. Pass the brand through its callers into completeDomain, use sortedKnownDomains to generate completion candidates, and add tests confirming apps is suggested for Feishu but not Lark.
🧹 Nitpick comments (18)
cmd/assembly_plan_fuzz_test.go (1)
56-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the exclusion from registered global flags.
Line 57 excludes only
profile.PlanAssemblyrecognizes every long flag fromregisteredGlobalFlagArities. Keep this fuzz property limited to unknown flags when global flags change.Proposed change
func FuzzPlanAssemblyUnknownFlagValueIsNotDomain(f *testing.F) { + longGlobals, _ := registeredGlobalFlagArities() for _, seed := range []string{"tenant", "profiled", "配置", "\x00", "x=y"} { f.Add(seed) } f.Fuzz(func(t *testing.T, flagName string) { - if flagName == "" || flagName == "profile" || strings.ContainsAny(flagName, "=\t\r\n ") { + if flagName == "" || strings.ContainsAny(flagName, "=\t\r\n ") { t.Skip() } + if _, known := longGlobals[flagName]; known { + t.Skip() + } plan := PlanAssembly([]string{"--" + flagName, "drive"}, []string{"drive"}, []string{"drive"})🤖 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/assembly_plan_fuzz_test.go` around lines 56 - 63, Update the fuzz input filter in the PlanAssembly test to exclude every long flag registered in registeredGlobalFlagArities rather than hard-coding only "profile". Preserve the existing empty-name and whitespace/equal-sign exclusions, and keep the assertion focused on flags unknown to the current global-flag registry.internal/qualitygate/publiccontent/catalog.go (2)
362-404: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffThe shape heuristic for
smtp_message_idis brittle.
safeTechnicalMessageIDaccepts a local part only when it is exactly 16 characters, has no two consecutive digits, has at least two digits, and has at least two uppercase runs of at most three characters. This encodes the exact shape of one current catalog fixture. If the upstream catalog changes that example by one character, the quality gate reports apublic_content_catalog_piifinding and CI fails with a misleading "email address" message.Consider replacing the shape heuristics with an explicit allowlist keyed by file path plus JSON path. That makes each suppression auditable and makes a catalog change produce a clear diff instead of a false PII finding. The same concern applies to
technicalReferenceLocalPartandstrongTechnicalHeaderLocalPart.🤖 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/publiccontent/catalog.go` around lines 362 - 404, Replace the shape-based checks in safeTechnicalMessageID with an explicit allowlist keyed by catalog file path and JSON path, and apply the same approach to technicalReferenceLocalPart and strongTechnicalHeaderLocalPart. Update their callers and data flow to provide those identifiers, allowing only documented entries while rejecting all other values.
243-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
walkCatalogJSONduplicates the allowlist traversal and is effectively unreachable.
catalogUnsafeEmailsruns only fromscanCatalogRawSafety.scanCatalogRawSafetyruns only whenparseCatalogJSONreturns an error at line 34.catalogUnsafeEmailsthen callsjson.Unmarshalon the same bytes at line 246. Both parsers reject the same inputs, so thejson.Unmarshalbranch at line 246 almost always fails too andwalkCatalogJSONnever runs. The result is a second copy of the key, parent key, and description traversal that carries thesafeCatalogEmailallowlist but is not exercised.Consider deleting
walkCatalogJSONand treating every email in the raw fallback as unsafe, because the fallback only applies to input that is not valid JSON.🤖 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/publiccontent/catalog.go` around lines 243 - 274, Simplify catalogUnsafeEmails by removing the JSON unmarshal and walkCatalogJSON traversal; the raw fallback receives invalid JSON, so treat every email found by catalogEmailRE as unsafe and return the lowercase-email map. Delete the now-unused walkCatalogJSON function and retain safeCatalogEmail filtering only in the valid-JSON parsing path.internal/qualitygate/cmd/manifest-export/main_test.go (1)
60-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth isolation tests assert only a non-empty
LARKSUITE_CLI_CONFIG_DIR. Each test clears the variable, calls the configure function, and then checks that the value is not empty. Neither test proves that the configured directory is isolated from the real user configuration directory, so a regression that points either tool at the user's real config still passes.
internal/qualitygate/cmd/manifest-export/main_test.go#L60-L68: assert thatconfigureManifestExportEnvironmentsets a process-local temporary directory and that the directory exists.internal/qualitygate/cmd/quality-gate/main_test.go#L16-L24: apply the same assertion toconfigureQualityGateEnvironment.As per coding guidelines: "Every behavior change must have an accompanying contract test that fails if the implementation is reverted and asserts the changed field or behavior directly."
🤖 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/cmd/manifest-export/main_test.go` around lines 60 - 68, Strengthen both isolation tests: in internal/qualitygate/cmd/manifest-export/main_test.go lines 60-68, update TestConfigureManifestExportEnvironmentIsolatesConfig to assert that configureManifestExportEnvironment sets LARKSUITE_CLI_CONFIG_DIR to a process-local temporary directory and that the directory exists; apply the same direct assertions in internal/qualitygate/cmd/quality-gate/main_test.go lines 16-24 for configureQualityGateEnvironment, rather than checking only that the variable is non-empty.Source: Coding guidelines
internal/affordance/affordance.go (1)
66-76: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
DomainSkillparses the service markdown a second time.
DomainSkillkeys onapicatalog.Catalog{}. WhenForalready loaded the same service under a real catalog key, this forces a secondfs.ReadFileand markdown parse for the same file, and stores a second cache entry.The returned value stays correct, because
skilldoes not depend on command mappings. The cost is one extra parse per service per process.🤖 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.go` around lines 66 - 76, Update DomainSkill to reuse the already-loaded service data from For instead of calling loadService with the empty catalog key. Locate the cached entry by the service identity across existing catalog entries, return its skill and non-empty status, and avoid creating another tried/byCatalog entry or rereading and reparsing the markdown.internal/schema/assembler.go (1)
219-221: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark the empty-catalog wrappers as deprecated.
EnvelopeOfandEnvelopesnow resolve affordances against an empty catalog. Command-form headings that need catalog resolution fall back toheadingToKey, so a caller on these wrappers can silently lose an overlay thatEnvelopeOfCatalogwould return. Production code already uses the catalog-aware functions.Add a
Deprecated:doc line to both wrappers so new callers choose the catalog-aware form.♻️ Proposed doc change
+// Deprecated: use EnvelopeOfCatalog with the selected catalog. This wrapper +// resolves affordances against an empty catalog and can miss command-form +// overlays. func EnvelopeOf(ref apicatalog.MethodRef) Envelope { return EnvelopeOfCatalog(apicatalog.Catalog{}, ref) }+// Deprecated: use EnvelopesCatalog with the selected catalog. func Envelopes(refs []apicatalog.MethodRef) []Envelope { return EnvelopesCatalog(apicatalog.Catalog{}, refs) }Also applies to: 238-240
🤖 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/schema/assembler.go` around lines 219 - 221, Add a Go doc comment with a Deprecated: notice to both empty-catalog wrappers, EnvelopeOf and Envelopes, directing callers to use EnvelopeOfCatalog and the corresponding catalog-aware function instead. Keep their existing implementations unchanged.internal/affordance/im_source_test.go (1)
249-260: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueOpen the IM catalog once per test helper.
imAffordanceCatalogcurrently loads and verifies the embedded snapshot on every call, including the loop-invokedparsedIMAffordance. Memoize the catalog with a safe helper or tests-localsync.OnceValuesvariable to avoid repeated integrity verification during one test run.🤖 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/im_source_test.go` around lines 249 - 260, Update imAffordanceCatalog to memoize the “im” catalog using a tests-local sync.OnceValues or equivalent safe helper, so registry.OpenSnapshot and snapshot.Catalog run only once while preserving existing error handling and returned catalog behavior.internal/registry/snapshot.go (2)
306-313: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider a recovery hint on service integrity errors.
catalogAccessErrorattachesWithHint("run lark-cli update to restore the embedded catalog").serviceIntegrityErrorandmanifestIntegrityErrordescribe the same class of failure — a corrupted embedded catalog — but carry no hint. A user who hits a checksum mismatch gets no recovery guidance.The coding guidelines require recovery guidance in
.WithHint(...).As per coding guidelines: "put recovery guidance in
.WithHint(...)".♻️ Proposed change
func serviceIntegrityError(name, reason string, cause error) error { return errs.NewInternalError( errs.SubtypeCatalogIntegrity, `embedded catalog service "%s" failed integrity validation: %s`, safeServiceName(name), reason, - ).WithCause(cause) + ).WithHint("run lark-cli update to restore the embedded catalog").WithCause(cause) }🤖 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/registry/snapshot.go` around lines 306 - 313, Update serviceIntegrityError and the corresponding manifestIntegrityError helper to attach the recovery guidance via WithHint("run lark-cli update to restore the embedded catalog"), while preserving their existing error messages and causes.Source: Coding guidelines
246-252: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
errors.Isfor theio.EOFcheck.
decoder.Decodecurrently returns a bareio.EOFat end of input, so the direct comparison works today.errors.Isis the idiomatic form and stays correct if the decoder ever wraps the sentinel.♻️ Proposed change
var trailing any - if err := decoder.Decode(&trailing); err != io.EOF { + if err := decoder.Decode(&trailing); !errors.Is(err, io.EOF) { if err == nil { return catalogIntegrityCause("JSON contains multiple values") } return err }Add
"errors"to the import block.🤖 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/registry/snapshot.go` around lines 246 - 252, Update the trailing-value Decode check in the snapshot validation flow to use errors.Is(err, io.EOF) instead of directly comparing err to io.EOF, and add the errors import. Preserve the existing handling for multiple JSON values and other decode errors.internal/registry/registry_test.go (1)
24-34: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low value
scopeTestCatalogre-reads and re-parses the whole snapshot on each call.
Snapshot.Catalogdeliberately never caches service bodies, as documented ininternal/registry/snapshot.goline 49.FullCatalogtherefore reads, checksums, and unmarshals all 15 shards on every call. Several tests callscopeTestCatalog(t)two or three times, for example lines 480-481 and 540-541.Build the catalog once per package with
sync.OnceValuesand reuse it.apicatalog.Catalogis read-only, so sharing it across tests is safe.🤖 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/registry/registry_test.go` around lines 24 - 34, Update scopeTestCatalog to initialize and retain the apicatalog.Catalog once per package using sync.OnceValues, then return the shared read-only catalog on subsequent calls. Preserve existing error handling during the one-time OpenSnapshot and FullCatalog initialization, and remove repeated snapshot parsing from each test invocation.internal/registry/snapshot_test.go (1)
137-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTwo integrity branches have no test coverage.
The table covers a missing file and an extra file. Two rejection paths in
internal/registry/snapshot.goremain untested:
- Line 224-227: a nested directory inside
services/is rejected withservice file set does not match manifest.- Line 249: trailing JSON after the manifest object is rejected with
JSON contains multiple values.Both guard the integrity boundary. Add cases so a regression in either check fails the suite.
♻️ Proposed table entries
{ name: "extra file", mutate: func(fsys fstest.MapFS) { fsys["services/extra.json"] = &fstest.MapFile{Data: []byte(`{}`)} }, reason: "service file set does not match manifest", }, + { + name: "nested directory in services", + mutate: func(fsys fstest.MapFS) { + fsys["services/nested/inner.json"] = &fstest.MapFile{Data: []byte(`{}`)} + }, + reason: "service file set does not match manifest", + }, + { + name: "trailing manifest value", + mutate: func(fsys fstest.MapFS) { + data := append(slices.Clone(fsys["manifest.json"].Data), []byte(`{}`)...) + fsys["manifest.json"] = &fstest.MapFile{Data: data} + }, + reason: "invalid JSON", + },
fstest.MapFSsynthesizesservices/nestedas a directory entry from the nested path, sofs.ReadDirreports it and theentry.IsDir()branch runs.🤖 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/registry/snapshot_test.go` around lines 137 - 151, Add two table-driven cases to the snapshot integrity tests: mutate the MapFS with a nested path under services/ so ReadDir encounters a directory and assert the existing “service file set does not match manifest” reason, and provide manifest JSON with trailing data after the object to assert “JSON contains multiple values.” Keep the cases aligned with the existing missing-file and extra-file entries.internal/registry/catalog/services/wiki.json (1)
1219-1221: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueRemove the empty annotation entry.
target_parent_tokendeclares"annotations": [""]. Every other annotation list in the catalog holds meaningful tokens such asreadOnlyoropen:x_user_id. Consumers that iterate annotations receive one empty token here. Drop the field.♻️ Proposed change
"description": "Target parent node token;- Target Wiki space ID and target parent token cannot be empty at the same time", - "required": false, - "annotations": [ - "" - ] + "required": 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/registry/catalog/services/wiki.json` around lines 1219 - 1221, Remove the empty annotation entry and its annotations field from the target_parent_token definition in the wiki catalog, leaving only meaningful annotation tokens on catalog entries.internal/registry/scopes.go (2)
278-284: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
catalogServiceNamesis duplicated in three places.The same loop over
catalog.Services()collectingservice.Namealso exists assnapshotServiceNamesininternal/registry/registry_test.golines 14-22 and astestCatalogServiceNamesincmd/auth/login_test.go. Move the helper tointernal/apicatalogas an exportedCatalog.ServiceNames()method and let all three call sites use it. That keeps the name-extraction rule with the type that owns the services.#!/bin/bash # Find every place that extracts service names from a catalog. rg -nP -C4 'range .*\.Services\(\)' --type=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/registry/scopes.go` around lines 278 - 284, The service-name extraction loop is duplicated across production and test helpers. Add an exported Catalog.ServiceNames() method in internal/apicatalog, move the logic from catalogServiceNames into it, and update catalogServiceNames, snapshotServiceNames, and testCatalogServiceNames call sites to use the method while preserving their existing results.
263-265: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueAvoid the name slice and match map for the all-services case.
GetReadOnlyScopesandComputeMinimumScopeSetbuild a slice of every service name, thenmethodsForProjectsbuilds amap[string]boolfrom that slice and tests every service against it. The filter accepts everything. Add a variant that walks the catalog directly for the all-services case.Also applies to: 274-276
🤖 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/registry/scopes.go` around lines 263 - 265, Update GetReadOnlyScopes and ComputeMinimumScopeSet to use a catalog-wide scope collection path that walks services directly, rather than creating catalogServiceNames slices and service-name maps through methodsForProjects. Add or reuse a methodsForProjects variant accepting the catalog for the all-services case, while preserving each function’s existing scope filtering and minimum-set behavior.cmd/build.go (2)
187-208: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the catalog-failure argument handling consistent with the success path.
Line 197 copies
cfg.invocationArgsbeforeSetArgs. Line 193 passes the slice directly. Both slices originate from the same field, so the two calls should use the same handling. Either copy in both places or in neither.WithInvocationArgsalready owns a defensive copy, so a second copy at line 197 is redundant.♻️ Proposed refactor
root := attachExecutionState(result) - root.SetArgs(append(make([]string, 0, len(cfg.invocationArgs)), cfg.invocationArgs...)) + root.SetArgs(cfg.invocationArgs) return root🤖 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/build.go` around lines 187 - 208, Update the SetArgs call in the successful buildForArgsWithConfig path within Build to pass cfg.invocationArgs directly, matching the catalog-failure path; remove the redundant defensive copy while preserving the existing argument ordering and behavior.
240-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the duplicated default resolution.
resolveBuildConfigalready applies thestreamsandsnapshotOpenerdefaults (lines 295-308). BothbuildForArgsWithConfigandbuildInternalWithConfigrepeat the same defaults. Every current caller passes a config that came fromresolveBuildConfig, so the repeated blocks never take effect. Three copies of the same default set will drift when a new default is added.Keep one nil-config guard and delegate to
resolveBuildConfig.♻️ Proposed refactor
cfg *buildConfig, ) (*buildResult, error) { if cfg == nil { - cfg = &buildConfig{} - } - if cfg.streams == nil { - cfg.streams = cmdutil.SystemIO() - } - if cfg.snapshotOpener == nil { - cfg.snapshotOpener = func() (catalogSnapshot, error) { return registry.OpenSnapshot() } + cfg = resolveBuildConfig(nil) }func buildInternalWithConfig(ctx context.Context, inv cmdutil.InvocationContext, cfg *buildConfig) (*buildRuntime, *cobra.Command, *hook.Registry) { if cfg == nil { cfg = resolveBuildConfig(nil) } - if cfg.streams == nil { - cfg.streams = cmdutil.SystemIO() - } - if cfg.snapshotOpener == nil { - cfg.snapshotOpener = func() (catalogSnapshot, error) { return registry.OpenSnapshot() } - } catalog, err := fullCatalog(cfg)Also applies to: 333-341
🤖 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/build.go` around lines 240 - 248, Remove the duplicated streams and snapshotOpener default assignments from buildForArgsWithConfig and buildInternalWithConfig. Keep a single nil-config guard in each entry point as needed, then delegate configuration defaulting to resolveBuildConfig so it remains the sole owner of these defaults.cmd/build_target_test.go (2)
460-493: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe late-registration guard does not pin the freeze point.
frozenPluginsruns at the start ofbuildForArgsWithConfig, before the snapshot opener is called. TheafterSnapshotOpenhook therefore fires after the plugin list is already frozen, solate.installs == 0holds regardless of where the freeze happens between those two points. The test still guards against a regression that moves plugin collection after catalog selection, but it cannot detect a regression that moves it anywhere earlier in the same window.Consider registering the late plugin from the
snapshotOpeneritself, or add a second hook that fires before the opener, to bracket the freeze point from both sides.🤖 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/build_target_test.go` around lines 460 - 493, Strengthen TestBuildForArgsLatePluginRegistrationDoesNotChangeFrozenTarget by registering the late plugin from the snapshot opener itself, or otherwise adding a pre-opener hook, so the test brackets the frozenPlugins collection point. Keep the existing assertions verifying the drive-only catalog, zero late-plugin installs, and unchanged command tree.
962-967: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport which field differs.
compareContractreturns a constant string.TestFullTargetCommandContractcompares full and target trees across every catalog and shortcut domain. When it fails, the message names the command path but not the differing field, so the cause must be found by hand.Compare the fields and name the first mismatch.
♻️ Proposed refactor
func compareContract(want, got commandContract) string { if reflect.DeepEqual(want, got) { return "" } - return "metadata or flags changed" + wantValue := reflect.ValueOf(want) + gotValue := reflect.ValueOf(got) + for i := 0; i < wantValue.NumField(); i++ { + if !reflect.DeepEqual(wantValue.Field(i).Interface(), gotValue.Field(i).Interface()) { + return fmt.Sprintf("field %s: want %#v, got %#v", + wantValue.Type().Field(i).Name, + wantValue.Field(i).Interface(), + gotValue.Field(i).Interface()) + } + } + return "metadata or flags changed" }Add
"fmt"to the import block.🤖 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/build_target_test.go` around lines 962 - 967, Update compareContract to compare commandContract fields individually in a stable order, returning a descriptive fmt-based message naming the first mismatched field; retain an empty string when all fields match and add the required fmt import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 3685e38c-c3a9-4629-a333-a5e19dbf28c7
📒 Files selected for processing (131)
.github/workflows/arch-audit.yml.github/workflows/ci.yml.gitignoreAGENTS.mdMakefilebuild.shcmd/assembly_plan.gocmd/assembly_plan_fuzz_test.gocmd/assembly_plan_test.gocmd/auth/auth_test.gocmd/auth/catalog_test_helpers_test.gocmd/auth/login.gocmd/auth/login_brand_filter_test.gocmd/auth/login_interactive.gocmd/auth/login_test.gocmd/auth/testmain_test.gocmd/build.gocmd/build_benchmark_test.gocmd/build_memstats_test.gocmd/build_target_test.gocmd/build_test.gocmd/cmdexample_test.gocmd/diagnose_scope_test.gocmd/error_auth_hint.gocmd/error_presenter_test.gocmd/global_flags.gocmd/global_flags_test.gocmd/platform_bootstrap.gocmd/plugin_integration_test.gocmd/root.gocmd/root_risk_help_test.gocmd/root_test.gocmd/schema/schema.gocmd/schema/schema_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/paramflags_test.gocmd/service/paramhelp.gocmd/service/service.gocmd/service/service_test.gocmd/service/testmain_test.gocmd/startup_brand.gocmd/startup_brand_test.gocmd/testmain_test.gocontent_embed_affordance_test.goerrs/subtypes.gointernal/affordance/affordance.gointernal/affordance/affordance_test.gointernal/affordance/application_source_test.gointernal/affordance/im_source_test.gointernal/apicatalog/catalog.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/fileupload.gointernal/cmdutil/fileupload_test.gointernal/cmdutil/json.gointernal/cmdutil/json_test.gointernal/cmdutil/testmain_test.gointernal/meta/normalize.gointernal/meta/normalize_test.gointernal/qualitygate/cmd/manifest-export/collect.gointernal/qualitygate/cmd/manifest-export/main.gointernal/qualitygate/cmd/manifest-export/main_test.gointernal/qualitygate/cmd/quality-gate/main.gointernal/qualitygate/cmd/quality-gate/main_test.gointernal/qualitygate/diff/diff.gointernal/qualitygate/diff/diff_test.gointernal/qualitygate/publiccontent/catalog.gointernal/qualitygate/publiccontent/catalog_test.gointernal/qualitygate/publiccontent/collect.gointernal/qualitygate/publiccontent/collect_test.gointernal/qualitygate/publiccontent/rules.gointernal/qualitygate/publiccontent/scan.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/run_test.gointernal/registry/catalog.gointernal/registry/catalog/manifest.jsoninternal/registry/catalog/services/approval.jsoninternal/registry/catalog/services/attendance.jsoninternal/registry/catalog/services/calendar.jsoninternal/registry/catalog/services/contact.jsoninternal/registry/catalog/services/drive.jsoninternal/registry/catalog/services/im.jsoninternal/registry/catalog/services/mail.jsoninternal/registry/catalog/services/mindnotes.jsoninternal/registry/catalog/services/minutes.jsoninternal/registry/catalog/services/okr.jsoninternal/registry/catalog/services/sheets.jsoninternal/registry/catalog/services/slides.jsoninternal/registry/catalog/services/task.jsoninternal/registry/catalog/services/vc.jsoninternal/registry/catalog/services/wiki.jsoninternal/registry/catalog_test.gointernal/registry/loader.gointernal/registry/loader_embedded.gointernal/registry/loader_test.gointernal/registry/meta_data_default.jsoninternal/registry/registry_test.gointernal/registry/registrytest/fixture_meta.jsoninternal/registry/registrytest/registrytest.gointernal/registry/registrytest/registrytest_test.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/registry/scope_hint_test.gointernal/registry/scopes.gointernal/registry/snapshot.gointernal/registry/snapshot_test.gointernal/registry/testmain_test.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/catalog_test_helpers_test.gointernal/schema/lint.gointernal/schema/lint_test.gointernal/schema/types.goscripts/build-pkg-pr-new.shscripts/ci-workflow.test.shscripts/fetch_meta.pyshortcuts/application/slash_command_common.goshortcuts/common/skill_references.goshortcuts/im/affordance_migration_test.goshortcuts/register.goshortcuts/register_brand_guard_test.goshortcuts/register_test.goshortcuts/slides/slides_replace_slide.goskills/lark-calendar/SKILL.mdtests/plugin_e2e/degrade_subsystem_test.gotests/plugin_e2e/harness.gotests/plugin_e2e/restrict_test.gotests/sidecar_e2e/roundtrip_test.go
💤 Files with no reviewable changes (26)
- internal/qualitygate/cmd/quality-gate/main.go
- internal/qualitygate/cmd/manifest-export/main.go
- internal/registry/loader_embedded.go
- build.sh
- internal/qualitygate/rules/dryrun.go
- scripts/build-pkg-pr-new.sh
- cmd/cmdexample_test.go
- .gitignore
- internal/registry/meta_data_default.json
- internal/registry/catalog.go
- cmd/startup_brand.go
- .github/workflows/arch-audit.yml
- internal/qualitygate/rules/dryrun_test.go
- internal/cmdutil/factory_default.go
- internal/registry/testmain_test.go
- internal/registry/registrytest/fixture_meta.json
- internal/registry/remote_test.go
- scripts/fetch_meta.py
- internal/registry/registrytest/registrytest.go
- internal/registry/loader.go
- internal/registry/loader_test.go
- cmd/startup_brand_test.go
- internal/registry/scope_hint_test.go
- internal/registry/remote.go
- internal/registry/catalog_test.go
- internal/registry/registrytest/registrytest_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2232 +/- ##
==========================================
+ Coverage 76.33% 76.42% +0.09%
==========================================
Files 993 991 -2
Lines 106633 107275 +642
==========================================
+ Hits 81400 81987 +587
- Misses 19032 19068 +36
- Partials 6201 6220 +19 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
21fba26 to
70bf027
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cmd/build.go (1)
240-248: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider centralizing default resolution in
resolveBuildConfig.Lines 240-248 re-apply the same defaults that
resolveBuildConfigalready applies at lines 295-307, andbuildInternalWithConfigrepeats them again at lines 333-341. Three copies of the samestreamsandsnapshotOpenerfallbacks can drift.Extract one
applyDefaults(cfg *buildConfig) *buildConfighelper and call it from all three sites.🤖 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/build.go` around lines 240 - 248, Centralize build configuration fallback logic by extracting an applyDefaults(cfg *buildConfig) *buildConfig helper that initializes nil configs, streams, and snapshotOpener. Replace the duplicated default-resolution blocks in the current build flow, resolveBuildConfig, and buildInternalWithConfig with calls to this helper, preserving the existing defaults and return behavior.
🤖 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.
Nitpick comments:
In `@cmd/build.go`:
- Around line 240-248: Centralize build configuration fallback logic by
extracting an applyDefaults(cfg *buildConfig) *buildConfig helper that
initializes nil configs, streams, and snapshotOpener. Replace the duplicated
default-resolution blocks in the current build flow, resolveBuildConfig, and
buildInternalWithConfig with calls to this helper, preserving the existing
defaults and return behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: aaa11a9f-c8a4-4ab2-8ab1-2d6786bd096e
📒 Files selected for processing (131)
.github/workflows/arch-audit.yml.github/workflows/ci.yml.gitignoreAGENTS.mdMakefilebuild.shcmd/assembly_plan.gocmd/assembly_plan_fuzz_test.gocmd/assembly_plan_test.gocmd/auth/auth_test.gocmd/auth/catalog_test_helpers_test.gocmd/auth/login.gocmd/auth/login_brand_filter_test.gocmd/auth/login_interactive.gocmd/auth/login_test.gocmd/auth/testmain_test.gocmd/build.gocmd/build_benchmark_test.gocmd/build_memstats_test.gocmd/build_target_test.gocmd/build_test.gocmd/cmdexample_test.gocmd/diagnose_scope_test.gocmd/error_auth_hint.gocmd/error_presenter_test.gocmd/global_flags.gocmd/global_flags_test.gocmd/platform_bootstrap.gocmd/plugin_integration_test.gocmd/root.gocmd/root_risk_help_test.gocmd/root_test.gocmd/schema/schema.gocmd/schema/schema_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/paramflags_test.gocmd/service/paramhelp.gocmd/service/service.gocmd/service/service_test.gocmd/service/testmain_test.gocmd/startup_brand.gocmd/startup_brand_test.gocmd/testmain_test.gocontent_embed_affordance_test.goerrs/subtypes.gointernal/affordance/affordance.gointernal/affordance/affordance_test.gointernal/affordance/application_source_test.gointernal/affordance/im_source_test.gointernal/apicatalog/catalog.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/fileupload.gointernal/cmdutil/fileupload_test.gointernal/cmdutil/json.gointernal/cmdutil/json_test.gointernal/cmdutil/testmain_test.gointernal/meta/normalize.gointernal/meta/normalize_test.gointernal/qualitygate/cmd/manifest-export/collect.gointernal/qualitygate/cmd/manifest-export/main.gointernal/qualitygate/cmd/manifest-export/main_test.gointernal/qualitygate/cmd/quality-gate/main.gointernal/qualitygate/cmd/quality-gate/main_test.gointernal/qualitygate/diff/diff.gointernal/qualitygate/diff/diff_test.gointernal/qualitygate/publiccontent/catalog.gointernal/qualitygate/publiccontent/catalog_test.gointernal/qualitygate/publiccontent/collect.gointernal/qualitygate/publiccontent/collect_test.gointernal/qualitygate/publiccontent/rules.gointernal/qualitygate/publiccontent/scan.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/run_test.gointernal/registry/catalog.gointernal/registry/catalog/manifest.jsoninternal/registry/catalog/services/approval.jsoninternal/registry/catalog/services/attendance.jsoninternal/registry/catalog/services/calendar.jsoninternal/registry/catalog/services/contact.jsoninternal/registry/catalog/services/drive.jsoninternal/registry/catalog/services/im.jsoninternal/registry/catalog/services/mail.jsoninternal/registry/catalog/services/mindnotes.jsoninternal/registry/catalog/services/minutes.jsoninternal/registry/catalog/services/okr.jsoninternal/registry/catalog/services/sheets.jsoninternal/registry/catalog/services/slides.jsoninternal/registry/catalog/services/task.jsoninternal/registry/catalog/services/vc.jsoninternal/registry/catalog/services/wiki.jsoninternal/registry/catalog_test.gointernal/registry/loader.gointernal/registry/loader_embedded.gointernal/registry/loader_test.gointernal/registry/meta_data_default.jsoninternal/registry/registry_test.gointernal/registry/registrytest/fixture_meta.jsoninternal/registry/registrytest/registrytest.gointernal/registry/registrytest/registrytest_test.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/registry/scope_hint_test.gointernal/registry/scopes.gointernal/registry/snapshot.gointernal/registry/snapshot_test.gointernal/registry/testmain_test.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/catalog_test_helpers_test.gointernal/schema/lint.gointernal/schema/lint_test.gointernal/schema/types.goscripts/build-pkg-pr-new.shscripts/ci-workflow.test.shscripts/fetch_meta.pyshortcuts/application/slash_command_common.goshortcuts/common/skill_references.goshortcuts/im/affordance_migration_test.goshortcuts/register.goshortcuts/register_brand_guard_test.goshortcuts/register_test.goshortcuts/slides/slides_replace_slide.goskills/lark-calendar/SKILL.mdtests/plugin_e2e/degrade_subsystem_test.gotests/plugin_e2e/harness.gotests/plugin_e2e/restrict_test.gotests/sidecar_e2e/roundtrip_test.go
💤 Files with no reviewable changes (26)
- internal/qualitygate/cmd/quality-gate/main.go
- cmd/cmdexample_test.go
- cmd/startup_brand.go
- internal/registry/registrytest/registrytest.go
- internal/qualitygate/rules/dryrun.go
- build.sh
- internal/registry/registrytest/fixture_meta.json
- internal/registry/registrytest/registrytest_test.go
- internal/registry/loader_test.go
- internal/qualitygate/rules/dryrun_test.go
- internal/registry/catalog_test.go
- internal/registry/loader.go
- internal/registry/scope_hint_test.go
- internal/registry/meta_data_default.json
- internal/registry/testmain_test.go
- .gitignore
- scripts/fetch_meta.py
- internal/registry/remote.go
- internal/registry/remote_test.go
- cmd/startup_brand_test.go
- internal/registry/loader_embedded.go
- internal/registry/catalog.go
- .github/workflows/arch-audit.yml
- internal/cmdutil/factory_default.go
- internal/qualitygate/cmd/manifest-export/main.go
- scripts/build-pkg-pr-new.sh
🚧 Files skipped from review as they are similar to previous changes (103)
- internal/meta/normalize_test.go
- skills/lark-calendar/SKILL.md
- internal/qualitygate/publiccontent/scan.go
- cmd/global_flags.go
- cmd/auth/testmain_test.go
- internal/qualitygate/rules/run.go
- AGENTS.md
- cmd/service/paramflags_test.go
- cmd/auth/catalog_test_helpers_test.go
- cmd/build_memstats_test.go
- internal/schema/lint.go
- internal/qualitygate/publiccontent/collect.go
- cmd/build_test.go
- internal/qualitygate/publiccontent/rules.go
- shortcuts/slides/slides_replace_slide.go
- internal/registry/catalog/manifest.json
- cmd/root_risk_help_test.go
- internal/cmdutil/factory.go
- internal/cmdutil/fileupload_test.go
- internal/schema/types.go
- cmd/testmain_test.go
- shortcuts/common/skill_references.go
- internal/meta/normalize.go
- cmd/service/service.go
- cmd/global_flags_test.go
- internal/qualitygate/diff/diff_test.go
- cmd/platform_bootstrap.go
- tests/sidecar_e2e/roundtrip_test.go
- cmd/build_benchmark_test.go
- cmd/service/testmain_test.go
- cmd/service/paramhelp.go
- internal/qualitygate/publiccontent/catalog.go
- cmd/error_presenter_test.go
- errs/subtypes.go
- internal/schema/assembler.go
- internal/registry/catalog/services/attendance.json
- cmd/build_target_test.go
- tests/plugin_e2e/harness.go
- scripts/ci-workflow.test.sh
- internal/schema/catalog_test_helpers_test.go
- tests/plugin_e2e/restrict_test.go
- cmd/root.go
- internal/registry/catalog/services/contact.json
- cmd/plugin_integration_test.go
- internal/qualitygate/cmd/quality-gate/main_test.go
- cmd/error_auth_hint.go
- internal/registry/catalog/services/mindnotes.json
- internal/cmdutil/fileupload.go
- internal/affordance/im_source_test.go
- .github/workflows/ci.yml
- internal/registry/catalog/services/wiki.json
- cmd/auth/login_brand_filter_test.go
- internal/qualitygate/diff/diff.go
- cmd/auth/auth_test.go
- cmd/root_test.go
- shortcuts/application/slash_command_common.go
- cmd/service/service_test.go
- cmd/service/affordance.go
- cmd/schema/schema.go
- tests/plugin_e2e/degrade_subsystem_test.go
- internal/registry/catalog/services/slides.json
- internal/cmdutil/json.go
- internal/apicatalog/catalog.go
- internal/affordance/affordance_test.go
- internal/registry/snapshot.go
- internal/qualitygate/cmd/manifest-export/collect.go
- shortcuts/im/affordance_migration_test.go
- internal/qualitygate/rules/run_test.go
- cmd/auth/login_interactive.go
- internal/registry/catalog/services/approval.json
- internal/affordance/application_source_test.go
- internal/qualitygate/cmd/manifest-export/main_test.go
- internal/registry/catalog/services/okr.json
- internal/qualitygate/publiccontent/catalog_test.go
- internal/registry/catalog/services/calendar.json
- internal/registry/catalog/services/sheets.json
- cmd/auth/login.go
- shortcuts/register_test.go
- internal/qualitygate/publiccontent/collect_test.go
- internal/registry/catalog/services/drive.json
- internal/affordance/affordance.go
- shortcuts/register_brand_guard_test.go
- cmd/diagnose_scope_test.go
- internal/cmdutil/json_test.go
- Makefile
- cmd/assembly_plan_test.go
- internal/schema/lint_test.go
- internal/registry/catalog/services/minutes.json
- content_embed_affordance_test.go
- cmd/service/affordance_test.go
- internal/registry/catalog/services/task.json
- internal/registry/snapshot_test.go
- shortcuts/register.go
- cmd/assembly_plan.go
- internal/registry/catalog/services/mail.json
- cmd/schema/schema_test.go
- internal/cmdutil/testmain_test.go
- internal/registry/catalog/services/im.json
- internal/registry/scopes.go
- cmd/auth/login_test.go
- internal/schema/assembler_test.go
- cmd/assembly_plan_fuzz_test.go
- internal/registry/catalog/services/vc.json
70bf027 to
b486e7a
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
cmd/build_target_test.go (3)
609-632: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the typed problem metadata on both guard paths.
Lines 594-600 assert the cause and the subtype for the build error. The two guard paths at lines 613-616 and 629-632 assert only the cause and the exit code. Add a subtype assertion so a regression that keeps the cause but changes the problem classification still fails.
♻️ Proposed assertion helper
+ assertCatalogGuard := func(t *testing.T, err error) { + t.Helper() + if !errors.Is(err, cause) { + t.Fatalf("guard error = %v, want preserved cause", err) + } + problem, ok := errs.ProblemOf(err) + if !ok || problem.Subtype != errs.SubtypeCatalogIntegrity { + t.Fatalf("guard problem = %#v, %v", problem, ok) + } + if output.ExitCodeOf(err) != output.ExitInternal { + t.Fatalf("guard exit = %d, want %d", output.ExitCodeOf(err), output.ExitInternal) + } + }Then call
assertCatalogGuard(t, err)after eachExecute().Based on the coding guideline "Use typed error metadata and cause preservation in error-path tests, asserting
errs.ProblemOffields and causes rather than message substrings alone."🤖 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/build_target_test.go` around lines 609 - 632, The two guard-path assertions after root.Execute and targetRoot.Execute only verify the cause and exit code; update both to also validate typed problem metadata using the existing assertCatalogGuard helper. Call assertCatalogGuard(t, err) after each Execute while preserving the current cause and exit-code checks.Source: Coding guidelines
963-968: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReport the differing fields instead of a fixed string.
compareContractreturns the same message for every mismatch. When this contract test fails, the output at line 871 does not identify which field diverged between the full tree and the target tree. Include the differing field names so a failure is actionable.♻️ Proposed diff detail
func compareContract(want, got commandContract) string { if reflect.DeepEqual(want, got) { return "" } - return "metadata or flags changed" + wantValue := reflect.ValueOf(want) + gotValue := reflect.ValueOf(got) + var fields []string + for i := range wantValue.NumField() { + if !reflect.DeepEqual(wantValue.Field(i).Interface(), gotValue.Field(i).Interface()) { + fields = append(fields, wantValue.Type().Field(i).Name) + } + } + return "fields differ: " + strings.Join(fields, ", ") }As per the coding guideline "Design CLI errors, output formats, and flags as machine-consumable: errors must be structured, actionable, and specific", the same specificity helps here for contract-regression diagnosis.
🤖 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/build_target_test.go` around lines 963 - 968, Update compareContract to inspect the differing fields of want and got and return an actionable message listing each field that differs, rather than the fixed “metadata or flags changed” string. Preserve the empty string result for equal commandContract values and ensure the reported names identify the specific contract fields used by the test.Source: Coding guidelines
815-826: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace
compactTestStringswithslices.Compact.Keep the explicit
appendonly if the caller needs a separate backing array; otherwise avoidout := values[:1]to prevent compacting into the caller’s slice. The Go 1.23 module enablesslices.Compact.🤖 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/build_target_test.go` around lines 815 - 826, Replace the compactTestStrings helper implementation with the standard library slices.Compact function, preserving its duplicate-removal behavior and nil result for empty input. Remove the manual out := values[:1] logic; retain an explicit append copy only if callers require isolation from the input slice.
🤖 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.
Nitpick comments:
In `@cmd/build_target_test.go`:
- Around line 609-632: The two guard-path assertions after root.Execute and
targetRoot.Execute only verify the cause and exit code; update both to also
validate typed problem metadata using the existing assertCatalogGuard helper.
Call assertCatalogGuard(t, err) after each Execute while preserving the current
cause and exit-code checks.
- Around line 963-968: Update compareContract to inspect the differing fields of
want and got and return an actionable message listing each field that differs,
rather than the fixed “metadata or flags changed” string. Preserve the empty
string result for equal commandContract values and ensure the reported names
identify the specific contract fields used by the test.
- Around line 815-826: Replace the compactTestStrings helper implementation with
the standard library slices.Compact function, preserving its duplicate-removal
behavior and nil result for empty input. Remove the manual out := values[:1]
logic; retain an explicit append copy only if callers require isolation from the
input slice.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1bd073a3-22c5-4d40-8176-2d02a4682650
📒 Files selected for processing (131)
.github/workflows/arch-audit.yml.github/workflows/ci.yml.gitignoreAGENTS.mdMakefilebuild.shcmd/assembly_plan.gocmd/assembly_plan_fuzz_test.gocmd/assembly_plan_test.gocmd/auth/auth_test.gocmd/auth/catalog_test_helpers_test.gocmd/auth/login.gocmd/auth/login_brand_filter_test.gocmd/auth/login_interactive.gocmd/auth/login_test.gocmd/auth/testmain_test.gocmd/build.gocmd/build_benchmark_test.gocmd/build_memstats_test.gocmd/build_target_test.gocmd/build_test.gocmd/cmdexample_test.gocmd/diagnose_scope_test.gocmd/error_auth_hint.gocmd/error_presenter_test.gocmd/global_flags.gocmd/global_flags_test.gocmd/platform_bootstrap.gocmd/plugin_integration_test.gocmd/root.gocmd/root_risk_help_test.gocmd/root_test.gocmd/schema/schema.gocmd/schema/schema_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/paramflags_test.gocmd/service/paramhelp.gocmd/service/service.gocmd/service/service_test.gocmd/service/testmain_test.gocmd/startup_brand.gocmd/startup_brand_test.gocmd/testmain_test.gocontent_embed_affordance_test.goerrs/subtypes.gointernal/affordance/affordance.gointernal/affordance/affordance_test.gointernal/affordance/application_source_test.gointernal/affordance/im_source_test.gointernal/apicatalog/catalog.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/fileupload.gointernal/cmdutil/fileupload_test.gointernal/cmdutil/json.gointernal/cmdutil/json_test.gointernal/cmdutil/testmain_test.gointernal/meta/normalize.gointernal/meta/normalize_test.gointernal/qualitygate/cmd/manifest-export/collect.gointernal/qualitygate/cmd/manifest-export/main.gointernal/qualitygate/cmd/manifest-export/main_test.gointernal/qualitygate/cmd/quality-gate/main.gointernal/qualitygate/cmd/quality-gate/main_test.gointernal/qualitygate/diff/diff.gointernal/qualitygate/diff/diff_test.gointernal/qualitygate/publiccontent/catalog.gointernal/qualitygate/publiccontent/catalog_test.gointernal/qualitygate/publiccontent/collect.gointernal/qualitygate/publiccontent/collect_test.gointernal/qualitygate/publiccontent/rules.gointernal/qualitygate/publiccontent/scan.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/run_test.gointernal/registry/catalog.gointernal/registry/catalog/manifest.jsoninternal/registry/catalog/services/approval.jsoninternal/registry/catalog/services/attendance.jsoninternal/registry/catalog/services/calendar.jsoninternal/registry/catalog/services/contact.jsoninternal/registry/catalog/services/drive.jsoninternal/registry/catalog/services/im.jsoninternal/registry/catalog/services/mail.jsoninternal/registry/catalog/services/mindnotes.jsoninternal/registry/catalog/services/minutes.jsoninternal/registry/catalog/services/okr.jsoninternal/registry/catalog/services/sheets.jsoninternal/registry/catalog/services/slides.jsoninternal/registry/catalog/services/task.jsoninternal/registry/catalog/services/vc.jsoninternal/registry/catalog/services/wiki.jsoninternal/registry/catalog_test.gointernal/registry/loader.gointernal/registry/loader_embedded.gointernal/registry/loader_test.gointernal/registry/meta_data_default.jsoninternal/registry/registry_test.gointernal/registry/registrytest/fixture_meta.jsoninternal/registry/registrytest/registrytest.gointernal/registry/registrytest/registrytest_test.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/registry/scope_hint_test.gointernal/registry/scopes.gointernal/registry/snapshot.gointernal/registry/snapshot_test.gointernal/registry/testmain_test.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/catalog_test_helpers_test.gointernal/schema/lint.gointernal/schema/lint_test.gointernal/schema/types.goscripts/build-pkg-pr-new.shscripts/ci-workflow.test.shscripts/fetch_meta.pyshortcuts/application/slash_command_common.goshortcuts/common/skill_references.goshortcuts/im/affordance_migration_test.goshortcuts/register.goshortcuts/register_brand_guard_test.goshortcuts/register_test.goshortcuts/slides/slides_replace_slide.goskills/lark-calendar/SKILL.mdtests/plugin_e2e/degrade_subsystem_test.gotests/plugin_e2e/harness.gotests/plugin_e2e/restrict_test.gotests/sidecar_e2e/roundtrip_test.go
💤 Files with no reviewable changes (26)
- scripts/build-pkg-pr-new.sh
- cmd/cmdexample_test.go
- internal/registry/registrytest/registrytest.go
- cmd/startup_brand_test.go
- scripts/fetch_meta.py
- internal/registry/catalog_test.go
- .gitignore
- internal/registry/loader.go
- cmd/startup_brand.go
- internal/registry/loader_embedded.go
- internal/registry/meta_data_default.json
- internal/registry/registrytest/fixture_meta.json
- internal/registry/remote.go
- internal/registry/loader_test.go
- internal/registry/testmain_test.go
- build.sh
- internal/qualitygate/cmd/quality-gate/main.go
- internal/cmdutil/factory_default.go
- internal/qualitygate/rules/dryrun.go
- internal/qualitygate/cmd/manifest-export/main.go
- internal/qualitygate/rules/dryrun_test.go
- internal/registry/catalog.go
- internal/registry/remote_test.go
- internal/registry/scope_hint_test.go
- internal/registry/registrytest/registrytest_test.go
- .github/workflows/arch-audit.yml
🚧 Files skipped from review as they are similar to previous changes (103)
- skills/lark-calendar/SKILL.md
- cmd/auth/login_brand_filter_test.go
- shortcuts/slides/slides_replace_slide.go
- cmd/service/paramhelp.go
- scripts/ci-workflow.test.sh
- cmd/build_benchmark_test.go
- shortcuts/application/slash_command_common.go
- cmd/service/service.go
- cmd/global_flags_test.go
- cmd/assembly_plan_test.go
- internal/cmdutil/testmain_test.go
- cmd/assembly_plan_fuzz_test.go
- cmd/root_test.go
- tests/plugin_e2e/restrict_test.go
- internal/registry/catalog/services/mindnotes.json
- internal/qualitygate/rules/run.go
- internal/qualitygate/publiccontent/scan.go
- internal/qualitygate/diff/diff.go
- cmd/global_flags.go
- errs/subtypes.go
- cmd/assembly_plan.go
- internal/registry/catalog/services/slides.json
- internal/registry/catalog/services/attendance.json
- AGENTS.md
- internal/schema/catalog_test_helpers_test.go
- internal/cmdutil/json_test.go
- cmd/schema/schema.go
- cmd/error_presenter_test.go
- cmd/build_memstats_test.go
- cmd/root.go
- internal/meta/normalize.go
- cmd/diagnose_scope_test.go
- internal/registry/catalog/services/vc.json
- internal/qualitygate/rules/run_test.go
- tests/sidecar_e2e/roundtrip_test.go
- internal/affordance/application_source_test.go
- tests/plugin_e2e/harness.go
- cmd/auth/testmain_test.go
- internal/schema/types.go
- shortcuts/common/skill_references.go
- shortcuts/register_brand_guard_test.go
- internal/qualitygate/publiccontent/collect_test.go
- internal/qualitygate/cmd/manifest-export/collect.go
- internal/qualitygate/publiccontent/rules.go
- cmd/service/service_test.go
- cmd/platform_bootstrap.go
- tests/plugin_e2e/degrade_subsystem_test.go
- shortcuts/im/affordance_migration_test.go
- internal/registry/catalog/manifest.json
- internal/cmdutil/factory.go
- internal/qualitygate/cmd/quality-gate/main_test.go
- internal/cmdutil/fileupload.go
- content_embed_affordance_test.go
- shortcuts/register_test.go
- internal/registry/catalog/services/mail.json
- internal/registry/catalog/services/sheets.json
- internal/cmdutil/json.go
- cmd/auth/catalog_test_helpers_test.go
- cmd/error_auth_hint.go
- internal/qualitygate/publiccontent/catalog.go
- shortcuts/register.go
- internal/qualitygate/diff/diff_test.go
- cmd/auth/auth_test.go
- internal/affordance/im_source_test.go
- cmd/root_risk_help_test.go
- internal/registry/catalog/services/minutes.json
- internal/affordance/affordance.go
- internal/registry/catalog/services/drive.json
- internal/schema/lint.go
- cmd/service/paramflags_test.go
- cmd/service/affordance.go
- cmd/plugin_integration_test.go
- internal/registry/registry_test.go
- .github/workflows/ci.yml
- internal/registry/catalog/services/approval.json
- Makefile
- cmd/schema/schema_test.go
- internal/schema/assembler.go
- internal/qualitygate/cmd/manifest-export/main_test.go
- internal/qualitygate/publiccontent/collect.go
- cmd/build_test.go
- internal/registry/snapshot.go
- internal/registry/catalog/services/task.json
- cmd/testmain_test.go
- internal/qualitygate/publiccontent/catalog_test.go
- internal/registry/catalog/services/contact.json
- cmd/service/testmain_test.go
- internal/registry/snapshot_test.go
- internal/schema/assembler_test.go
- internal/registry/catalog/services/okr.json
- internal/registry/catalog/services/im.json
- cmd/auth/login_test.go
- cmd/auth/login_interactive.go
- internal/cmdutil/fileupload_test.go
- internal/apicatalog/catalog.go
- internal/schema/lint_test.go
- internal/meta/normalize_test.go
- cmd/auth/login.go
- internal/registry/catalog/services/calendar.json
- internal/registry/catalog/services/wiki.json
- cmd/build.go
- internal/registry/scopes.go
- cmd/service/affordance_test.go
b486e7a to
eb7f343
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/apicatalog/catalog.go`:
- Around line 68-69: Update the documentation comment for Catalog.Source to use
source-neutral wording rather than describing embedded versus runtime, while
leaving the method implementation unchanged.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9945f228-84cf-4709-9c82-e49b97272662
📒 Files selected for processing (132)
.github/workflows/arch-audit.yml.github/workflows/ci.yml.gitignoreAGENTS.mdMakefilebuild.shcmd/assembly_plan.gocmd/assembly_plan_fuzz_test.gocmd/assembly_plan_test.gocmd/auth/auth_test.gocmd/auth/catalog_test_helpers_test.gocmd/auth/login.gocmd/auth/login_brand_filter_test.gocmd/auth/login_interactive.gocmd/auth/login_test.gocmd/auth/testmain_test.gocmd/build.gocmd/build_benchmark_test.gocmd/build_memstats_test.gocmd/build_target_test.gocmd/build_test.gocmd/cmdexample_test.gocmd/diagnose_scope_test.gocmd/error_auth_hint.gocmd/error_presenter_test.gocmd/global_flags.gocmd/global_flags_test.gocmd/platform_bootstrap.gocmd/plugin_integration_test.gocmd/root.gocmd/root_risk_help_test.gocmd/root_test.gocmd/schema/schema.gocmd/schema/schema_test.gocmd/service/affordance.gocmd/service/affordance_test.gocmd/service/paramflags_test.gocmd/service/paramhelp.gocmd/service/service.gocmd/service/service_test.gocmd/service/testmain_test.gocmd/startup_brand.gocmd/startup_brand_test.gocmd/testmain_test.gocontent_embed_affordance_test.goerrs/subtypes.gointernal/affordance/affordance.gointernal/affordance/affordance_test.gointernal/affordance/application_source_test.gointernal/affordance/im_source_test.gointernal/apicatalog/catalog.gointernal/apicatalog/catalog_test.gointernal/cmdutil/factory.gointernal/cmdutil/factory_default.gointernal/cmdutil/fileupload.gointernal/cmdutil/fileupload_test.gointernal/cmdutil/json.gointernal/cmdutil/json_test.gointernal/cmdutil/testmain_test.gointernal/meta/normalize.gointernal/meta/normalize_test.gointernal/qualitygate/cmd/manifest-export/collect.gointernal/qualitygate/cmd/manifest-export/main.gointernal/qualitygate/cmd/manifest-export/main_test.gointernal/qualitygate/cmd/quality-gate/main.gointernal/qualitygate/cmd/quality-gate/main_test.gointernal/qualitygate/diff/diff.gointernal/qualitygate/diff/diff_test.gointernal/qualitygate/publiccontent/catalog.gointernal/qualitygate/publiccontent/catalog_test.gointernal/qualitygate/publiccontent/collect.gointernal/qualitygate/publiccontent/collect_test.gointernal/qualitygate/publiccontent/rules.gointernal/qualitygate/publiccontent/scan.gointernal/qualitygate/rules/dryrun.gointernal/qualitygate/rules/dryrun_test.gointernal/qualitygate/rules/run.gointernal/qualitygate/rules/run_test.gointernal/registry/catalog.gointernal/registry/catalog/manifest.jsoninternal/registry/catalog/services/approval.jsoninternal/registry/catalog/services/attendance.jsoninternal/registry/catalog/services/calendar.jsoninternal/registry/catalog/services/contact.jsoninternal/registry/catalog/services/drive.jsoninternal/registry/catalog/services/im.jsoninternal/registry/catalog/services/mail.jsoninternal/registry/catalog/services/mindnotes.jsoninternal/registry/catalog/services/minutes.jsoninternal/registry/catalog/services/okr.jsoninternal/registry/catalog/services/sheets.jsoninternal/registry/catalog/services/slides.jsoninternal/registry/catalog/services/task.jsoninternal/registry/catalog/services/vc.jsoninternal/registry/catalog/services/wiki.jsoninternal/registry/catalog_test.gointernal/registry/loader.gointernal/registry/loader_embedded.gointernal/registry/loader_test.gointernal/registry/meta_data_default.jsoninternal/registry/registry_test.gointernal/registry/registrytest/fixture_meta.jsoninternal/registry/registrytest/registrytest.gointernal/registry/registrytest/registrytest_test.gointernal/registry/remote.gointernal/registry/remote_test.gointernal/registry/scope_hint_test.gointernal/registry/scopes.gointernal/registry/snapshot.gointernal/registry/snapshot_test.gointernal/registry/testmain_test.gointernal/schema/assembler.gointernal/schema/assembler_test.gointernal/schema/catalog_test_helpers_test.gointernal/schema/lint.gointernal/schema/lint_test.gointernal/schema/types.goscripts/build-pkg-pr-new.shscripts/ci-workflow.test.shscripts/fetch_meta.pyshortcuts/application/slash_command_common.goshortcuts/common/skill_references.goshortcuts/im/affordance_migration_test.goshortcuts/register.goshortcuts/register_brand_guard_test.goshortcuts/register_test.goshortcuts/slides/slides_replace_slide.goskills/lark-calendar/SKILL.mdtests/plugin_e2e/degrade_subsystem_test.gotests/plugin_e2e/harness.gotests/plugin_e2e/restrict_test.gotests/sidecar_e2e/roundtrip_test.go
💤 Files with no reviewable changes (26)
- scripts/build-pkg-pr-new.sh
- .gitignore
- internal/cmdutil/factory_default.go
- cmd/cmdexample_test.go
- .github/workflows/arch-audit.yml
- internal/registry/catalog.go
- internal/registry/registrytest/registrytest.go
- internal/registry/loader_test.go
- build.sh
- internal/registry/catalog_test.go
- internal/registry/remote.go
- cmd/startup_brand.go
- internal/qualitygate/rules/dryrun_test.go
- internal/registry/registrytest/fixture_meta.json
- internal/qualitygate/cmd/manifest-export/main.go
- cmd/startup_brand_test.go
- internal/registry/meta_data_default.json
- internal/registry/scope_hint_test.go
- internal/registry/testmain_test.go
- internal/registry/remote_test.go
- internal/qualitygate/cmd/quality-gate/main.go
- scripts/fetch_meta.py
- internal/registry/loader_embedded.go
- internal/registry/registrytest/registrytest_test.go
- internal/qualitygate/rules/dryrun.go
- internal/registry/loader.go
🚧 Files skipped from review as they are similar to previous changes (101)
- internal/qualitygate/diff/diff.go
- cmd/global_flags_test.go
- internal/meta/normalize_test.go
- internal/qualitygate/publiccontent/scan.go
- errs/subtypes.go
- AGENTS.md
- cmd/platform_bootstrap.go
- cmd/service/paramhelp.go
- cmd/service/paramflags_test.go
- shortcuts/common/skill_references.go
- cmd/auth/catalog_test_helpers_test.go
- cmd/auth/login_brand_filter_test.go
- internal/cmdutil/factory.go
- cmd/build_test.go
- scripts/ci-workflow.test.sh
- cmd/build_memstats_test.go
- internal/registry/catalog/manifest.json
- internal/registry/catalog/services/minutes.json
- shortcuts/register_brand_guard_test.go
- internal/qualitygate/rules/run_test.go
- skills/lark-calendar/SKILL.md
- shortcuts/application/slash_command_common.go
- shortcuts/slides/slides_replace_slide.go
- cmd/assembly_plan_test.go
- internal/cmdutil/json.go
- cmd/service/service_test.go
- cmd/root_test.go
- cmd/error_auth_hint.go
- internal/qualitygate/rules/run.go
- cmd/plugin_integration_test.go
- cmd/assembly_plan_fuzz_test.go
- internal/schema/lint_test.go
- internal/schema/catalog_test_helpers_test.go
- tests/sidecar_e2e/roundtrip_test.go
- cmd/service/testmain_test.go
- cmd/service/service.go
- internal/qualitygate/cmd/quality-gate/main_test.go
- internal/qualitygate/cmd/manifest-export/main_test.go
- cmd/global_flags.go
- cmd/assembly_plan.go
- internal/cmdutil/json_test.go
- cmd/service/affordance.go
- internal/registry/catalog/services/mail.json
- cmd/build_benchmark_test.go
- tests/plugin_e2e/restrict_test.go
- internal/qualitygate/publiccontent/collect_test.go
- cmd/auth/testmain_test.go
- internal/qualitygate/publiccontent/catalog.go
- internal/schema/assembler.go
- cmd/root_risk_help_test.go
- internal/affordance/affordance.go
- internal/cmdutil/fileupload_test.go
- shortcuts/register.go
- internal/qualitygate/publiccontent/collect.go
- .github/workflows/ci.yml
- cmd/auth/auth_test.go
- cmd/schema/schema.go
- internal/registry/snapshot.go
- internal/affordance/affordance_test.go
- cmd/auth/login_interactive.go
- internal/qualitygate/cmd/manifest-export/collect.go
- internal/affordance/application_source_test.go
- shortcuts/im/affordance_migration_test.go
- cmd/testmain_test.go
- internal/registry/scopes.go
- internal/registry/catalog/services/drive.json
- internal/registry/catalog/services/slides.json
- cmd/auth/login_test.go
- internal/registry/catalog/services/attendance.json
- internal/cmdutil/testmain_test.go
- internal/registry/catalog/services/calendar.json
- internal/registry/catalog/services/contact.json
- internal/schema/types.go
- cmd/auth/login.go
- cmd/service/affordance_test.go
- content_embed_affordance_test.go
- cmd/root.go
- internal/registry/catalog/services/approval.json
- internal/qualitygate/publiccontent/catalog_test.go
- internal/registry/catalog/services/wiki.json
- tests/plugin_e2e/harness.go
- internal/registry/catalog/services/vc.json
- internal/qualitygate/diff/diff_test.go
- internal/schema/lint.go
- shortcuts/register_test.go
- internal/qualitygate/publiccontent/rules.go
- internal/affordance/im_source_test.go
- internal/registry/catalog/services/okr.json
- cmd/build_target_test.go
- internal/schema/assembler_test.go
- internal/registry/registry_test.go
- internal/meta/normalize.go
- cmd/error_presenter_test.go
- tests/plugin_e2e/degrade_subsystem_test.go
- internal/registry/catalog/services/task.json
- internal/registry/catalog/services/im.json
- internal/registry/snapshot_test.go
- internal/registry/catalog/services/mindnotes.json
- cmd/schema/schema_test.go
- cmd/diagnose_scope_test.go
- cmd/build.go
eb7f343 to
356f69e
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
356f69e to
be38883
Compare
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
1 participant
Summary
Large API catalogs make every CLI startup pay the cost of assembling the full command tree, even when an agent invokes only one domain. This PR introduces an embedded, sharded catalog snapshot and invocation-aware assembly so standard execution loads only the reachable API and shortcut domains while preserving full-tree behavior for callers that use
Buildwithout invocation arguments.Changes
internal/registry/catalog.PlanAssembly,BuildForArgs, andWithInvocationArgsto select None, Target, or Full assembly from one defensive copy of the invocation arguments.cmd.Execute()or the distributed binary require no integration changes, while customcmd.Build()wrappers can opt into lazy assembly withcmd.WithInvocationArgs(args).Test Plan
make unit-testpassed with the race detectorgo vet ./...,gofmt -l ., andgo mod tidypassedgolangci-lint v2.1.6 --new-from-rev=origin/mainpassed with 0 issues./lark-cli drive +search --query quarterly --page-size 1 --as usercompleted a real read-only request with exit code 0Related Issues
N/A
Summary by CodeRabbit