feat(core): support custom brands via extraBrands in config.json #1149
Open
fengb3 wants to merge 4 commits into
Open
feat(core): support custom brands via extraBrands in config.json #1149fengb3 wants to merge 4 commits into
fengb3 wants to merge 4 commits into
Conversation
Add brandRegistry map and RegisterBrand/MergeEndpointOverrides to types.go. LoadMultiAppConfig registers extraBrands entries on load. ResolveEndpoints signature unchanged — all callers zero-modified.
Add brandRegistry map and RegisterBrand/MergeEndpointOverrides to types.go. LoadMultiAppConfig registers extraBrands entries on load. ResolveEndpoints signature unchanged — all callers zero-modified.
…into feat/extra-brands
|
|
📝 WalkthroughWalkthroughThe PR refactors endpoint resolution from hardcoded switch logic to a registry-based system. ChangesCustom Brand Endpoint Registry
Sequence DiagramsequenceDiagram
participant ConfigLoader as Config Loader
participant RegisterFn as RegisterBrand
participant Registry as brandRegistry
participant Resolver as ResolveEndpoints
ConfigLoader->>RegisterFn: RegisterBrand(name, overrides)
RegisterFn->>Registry: Merge overrides onto Feishu defaults
Resolver->>Registry: Lookup brand by name
Registry-->>Resolver: Return registered endpoints or Feishu fallback
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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
the
size/M
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
internal/core/types_test.go (1)
93-116: ⚡ Quick winIsolate
brandRegistrymutations in tests.These tests mutate package-global registry entries (
staging,proxy) and do not clean them up, which can introduce order-coupling with future tests.Proposed fix
func TestRegisterBrand(t *testing.T) { + t.Cleanup(func() { delete(brandRegistry, "staging") }) RegisterBrand("staging", Endpoints{Open: "https://open-staging.feishu.cn"}) ep := ResolveEndpoints("staging") @@ func TestRegisterBrand_Full(t *testing.T) { + t.Cleanup(func() { delete(brandRegistry, "proxy") }) RegisterBrand("proxy", Endpoints{🤖 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/core/types_test.go` around lines 93 - 116, The tests TestRegisterBrand and TestRegisterBrand_Full mutate the package-global brandRegistry via RegisterBrand and don't restore it; modify each test to capture the current brandRegistry (or relevant entries) before calling RegisterBrand and defer restoring the original registry state at the end of the test so registry mutations are isolated; use ResolveEndpoints to verify behavior as before but ensure brandRegistry is reset (restoring removed or previous Endpoints for keys like "staging" and "proxy") in a deferred cleanup to avoid test order coupling.
🤖 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/core/config.go`:
- Around line 203-205: The loop over multi.ExtraBrands may dereference nil
pointers (ep) and panic; update the loop that iterates name, ep in
multi.ExtraBrands to guard against nil ep before calling RegisterBrand — e.g.,
if ep is nil then skip (or log a warning) and continue, otherwise call
RegisterBrand(name, *ep); ensure you reference the existing loop and
RegisterBrand call so only non-nil endpoints get dereferenced.
In `@internal/core/types.go`:
- Around line 137-140: The current fallback always returns
brandRegistry[string(BrandFeishu)] for any unknown brand, which hides typos;
change the logic so brandRegistry[string(BrandFeishu)] is returned only when
brand is empty (string(brand) == ""); if the brand is non-empty and not found in
brandRegistry, do not silently return Feishu — return a nil/zero endpoint (or an
explicit error/marker) so callers can detect and handle an unknown brand; update
the code around brandRegistry, BrandFeishu and the brand variable accordingly.
- Around line 84-133: There are duplicate top-level declarations for
brandRegistry, RegisterBrand and MergeEndpointOverrides; remove the redundant
copies and keep one consolidated implementation (keep the MergeEndpointOverrides
logic in the single RegisterBrand/brandRegistry file) so the package compiles.
Also update ResolveEndpoints to validate lookups instead of silently falling
back to Feishu: when a requested brand (e.g., LarkBrand or any name) is not
found in brandRegistry return an error (or a boolean ok) alongside the Endpoints
so callers can handle unknown-brand cases rather than implicitly defaulting to
Feishu.
---
Nitpick comments:
In `@internal/core/types_test.go`:
- Around line 93-116: The tests TestRegisterBrand and TestRegisterBrand_Full
mutate the package-global brandRegistry via RegisterBrand and don't restore it;
modify each test to capture the current brandRegistry (or relevant entries)
before calling RegisterBrand and defer restoring the original registry state at
the end of the test so registry mutations are isolated; use ResolveEndpoints to
verify behavior as before but ensure brandRegistry is reset (restoring removed
or previous Endpoints for keys like "staging" and "proxy") in a deferred cleanup
to avoid test order coupling.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 086c041b-7695-468e-a481-12e0e65d950a
📒 Files selected for processing (3)
internal/core/config.gointernal/core/types.gointernal/core/types_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/core/types_test.go (1)
122-134: ⚡ Quick winStrengthen built-in immutability assertion for
lark.Line 131 only checks
Open, so mutation ofAccounts/MCP/AppLinkcould slip through. Snapshot and compare the fullEndpointsstruct forlark(same as feishu), and optionally assert empty brand key is not inserted.🧪 Suggested test hardening
func TestRegisterBrand_IgnoresBuiltIn(t *testing.T) { - original := brandRegistry[string(BrandFeishu)] + originalFeishu := brandRegistry[string(BrandFeishu)] + originalLark := brandRegistry[string(BrandLark)] RegisterBrand("feishu", Endpoints{Open: "https://malicious.example.com"}) RegisterBrand("lark", Endpoints{Open: "https://malicious.example.com"}) RegisterBrand("", Endpoints{Open: "https://malicious.example.com"}) - if brandRegistry[string(BrandFeishu)] != original { + if brandRegistry[string(BrandFeishu)] != originalFeishu { t.Error("RegisterBrand should not overwrite built-in feishu brand") } - if brandRegistry[string(BrandLark)].Open == "https://malicious.example.com" { + if brandRegistry[string(BrandLark)] != originalLark { t.Error("RegisterBrand should not overwrite built-in lark brand") } + if _, ok := brandRegistry[""]; ok { + t.Error("RegisterBrand should ignore empty brand name") + } }🤖 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/core/types_test.go` around lines 122 - 134, Update TestRegisterBrand_IgnoresBuiltIn to assert the entire Endpoints struct for lark is unchanged: capture the original endpoints (like originalFeishu := brandRegistry[string(BrandFeishu)] and originalLark := brandRegistry[string(BrandLark)]) before calling RegisterBrand, call RegisterBrand("", ...) and RegisterBrand("feishu", ...)/RegisterBrand("lark", ...), then compare brandRegistry[string(BrandLark)] to originalLark (not just the .Open field) using equality on the Endpoints value; also add an assertion that brandRegistry does not contain an entry for the empty string key (e.g., brandRegistry[""] is absent or zero value) to ensure empty brand keys were not inserted.
🤖 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 `@internal/core/types_test.go`:
- Around line 122-134: Update TestRegisterBrand_IgnoresBuiltIn to assert the
entire Endpoints struct for lark is unchanged: capture the original endpoints
(like originalFeishu := brandRegistry[string(BrandFeishu)] and originalLark :=
brandRegistry[string(BrandLark)]) before calling RegisterBrand, call
RegisterBrand("", ...) and RegisterBrand("feishu", ...)/RegisterBrand("lark",
...), then compare brandRegistry[string(BrandLark)] to originalLark (not just
the .Open field) using equality on the Endpoints value; also add an assertion
that brandRegistry does not contain an entry for the empty string key (e.g.,
brandRegistry[""] is absent or zero value) to ensure empty brand keys were not
inserted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0a812edd-8309-4708-8cfc-1da2683dcde2
📒 Files selected for processing (3)
internal/core/config.gointernal/core/types.gointernal/core/types_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- internal/core/config.go
- internal/core/types.go
Collaborator
|
Thanks for the PR 🙏 One thing to flag upfront: just exposing the endpoint baseURL isn't enough to make lark-cli work against a private deployment. Auth, the OpenAPI command surface, and most upper-layer capabilities don't have parity with the public cloud yet — so even with Private deployment support needs to be tackled as a whole, not via an endpoint override. So I'd rather not land this under the "private deployment" framing and set an expectation we can't meet. If you're trying to reach a specific internal environment, happy to look at that case separately. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
支持在 config.json 中通过 extraBrands 字段配置自定义brand（私有化部署），让 lark-cli 能够连接非
feishu/lark 的自建飞书平台。
Changes
MergeEndpointOverrides），支持运行时注册自定义品牌；为 Endpoints 字段添加 json tag 修复反序列化问题
中的自定义品牌
Usage
在
~/.lark-cli/config.json中添加extraBrands字段，设置自定义 endpoint base url。未指定的字段会自动继承feishu的默认值。{ "extraBrands": { "your-brand-name": { "open": "https://your-private-hosted-url", // "accounts": "", // "mcp": "", // "applink": "", } }, "apps": [ { "appId": "cli_xxx", "appSecret": "xxx", "brand": "your-brand-name" } ] } // brand 字段引用 extraBrands 中定义的 key，CLI 会自动使用该品牌对应的 endpoint 进行 API 调用。Test Plan
Related Issues
Summary by CodeRabbit
New Features
Tests