fix: route brand-sensitive endpoints through the resolver#1836
Merged
Conversation
evandance
added
domain/core
|
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:
📝 WalkthroughWalkthroughThis PR centralizes brand parsing and endpoint resolution across configuration, authentication, self-update, registry, and event flows. It adds brand-mismatch registration retries, domain-contract lint coverage, and validated npm registry URL and redirect handling. ChangesBrand resolution centralization
Domain contract lint rule
Dynamic npm registry URL resolution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1836 +/- ##
==========================================
+ Coverage 74.45% 74.58% +0.13%
==========================================
Files 860 861 +1
Lines 89906 89971 +65
==========================================
+ Hits 66940 67106 +166
+ Misses 17780 17669 -111
- Partials 5186 5196 +10 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1756d39a81a895e8068dc0ea645ab161d13cc105🧩 Skill updatenpx skills add larksuite/cli#fix/endpoint-resolver-consolidation -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: 2
🤖 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 `@cmd/config/init_interactive_test.go`:
- Around line 25-30: The test in retryBrand only validates the brand when gotR
is true, so false-retry cases can hide an incorrect brand result. Update the
assertion in init_interactive_test to always compare both gotB and gotR against
c.wantBrand and c.wantRetry for every case, using retryBrand as the reference
point.
In `@cmd/config/init_interactive.go`:
- Around line 225-237: Use core.ParseBrand when computing finalBrand in
init_interactive.go so the saved brand is derived with the same normalization as
retryBrand. The current exact string matching can leave finalBrand on the
original value when result.UserInfo.TenantBrand is mixed-case or padded, so
update the final-brand selection logic to normalize TenantBrand through
core.ParseBrand and keep it consistent with the retry endpoint choice.
🪄 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: c6efd731-adbc-4a6c-8865-1d757c9b4cfa
📒 Files selected for processing (22)
cmd/config/bind_test.gocmd/config/binder.gocmd/config/init_interactive.gocmd/config/init_interactive_test.gocmd/update/update.goextension/credential/env/env.goextension/credential/sidecar/provider.gointernal/auth/app_registration.gointernal/auth/app_registration_test.gointernal/core/types.gointernal/core/types_test.gointernal/registry/remote.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.gointernal/update/update.gointernal/update/update_test.golint/README.mdlint/domaincontract/scan.golint/domaincontract/scan_test.golint/main.goshortcuts/event/subscribe.goshortcuts/event/subscribe_test.go
💤 Files with no reviewable changes (1)
- cmd/config/bind_test.go
evandance
force-pushed
the
fix/endpoint-resolver-consolidation
branch
3 times, most recently
from
July 10, 2026 06:24
a2f4f56 to
6d2f2b4
Compare
github-actions
Bot
added
size/XL
There was a problem hiding this comment.
🧹 Nitpick comments (1)
lint/domaincontract/scan_test.go (1)
87-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
net/urlfor host extraction instead of prefix stripping.
strings.TrimPrefix(v, "https://")yields the full URL path (e.g.,open.feishu.cn/open-apis), not just the host. Ifinternal/core/types.goever defines a resolver URL with a path component, the extracted key won't match the bare host inforbiddenHosts, causing a false test failure. The scanner usesstrings.Contains(substring match), so this inconsistency is latent today but fragile against future resolver changes.♻️ Proposed fix using net/url
import ( "go/ast" "go/parser" "go/token" + "net/url" "os" "path/filepath" "strconv" "strings" "testing" )v, err := strconv.Unquote(lit.Value) - if err != nil || !strings.HasPrefix(v, "https://") { + if err != nil { return true } - resolverHosts[strings.TrimPrefix(v, "https://")] = true + u, err := url.Parse(v) + if err != nil || u.Host == "" { + return true + } + resolverHosts[u.Host] = true return true🤖 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 `@lint/domaincontract/scan_test.go` around lines 87 - 91, In the resolver host extraction logic, replace strings.TrimPrefix(v, "https://") with net/url parsing so only the URL hostname is stored in resolverHosts. Parse the unquoted value, handle parse errors alongside the existing validation, and use the parsed Hostname when populating resolverHosts; update imports accordingly.
🤖 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 `@lint/domaincontract/scan_test.go`:
- Around line 87-91: In the resolver host extraction logic, replace
strings.TrimPrefix(v, "https://") with net/url parsing so only the URL hostname
is stored in resolverHosts. Parse the unquoted value, handle parse errors
alongside the existing validation, and use the parsed Hostname when populating
resolverHosts; update imports accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 47f9aec6-f3b4-4a0d-b887-8e7aba1c8acc
📒 Files selected for processing (23)
.github/workflows/ci.ymlcmd/config/bind_test.gocmd/config/binder.gocmd/config/init_interactive.gocmd/config/init_interactive_test.gocmd/update/update.goextension/credential/env/env.goextension/credential/sidecar/provider.gointernal/auth/app_registration.gointernal/auth/app_registration_test.gointernal/core/types.gointernal/core/types_test.gointernal/registry/remote.gointernal/selfupdate/updater.gointernal/selfupdate/updater_test.gointernal/update/update.gointernal/update/update_test.golint/README.mdlint/domaincontract/scan.golint/domaincontract/scan_test.golint/main.goshortcuts/event/subscribe.goshortcuts/event/subscribe_test.go
💤 Files with no reviewable changes (1)
- cmd/config/bind_test.go
✅ Files skipped from review due to trivial changes (1)
- lint/README.md
🚧 Files skipped from review as they are similar to previous changes (15)
- shortcuts/event/subscribe_test.go
- internal/core/types_test.go
- internal/selfupdate/updater_test.go
- cmd/update/update.go
- extension/credential/env/env.go
- shortcuts/event/subscribe.go
- cmd/config/init_interactive_test.go
- lint/main.go
- extension/credential/sidecar/provider.go
- internal/registry/remote.go
- internal/auth/app_registration_test.go
- internal/core/types.go
- cmd/config/binder.go
- cmd/config/init_interactive.go
- internal/selfupdate/updater.go
evandance
force-pushed
the
fix/endpoint-resolver-consolidation
branch
8 times, most recently
from
July 11, 2026 04:18
dfd63f3 to
f6bdf6a
Compare
evandance
changed the title
evandance
force-pushed
the
fix/endpoint-resolver-consolidation
branch
3 times, most recently
from
July 11, 2026 08:27
5c37be7 to
274f08a
Compare
Lark users' API metadata, skills index and source, event WebSocket, and app-registration endpoints now follow the configured brand through one resolver, starting from the very first catalog access at process startup. App registration keeps its protocol-defined Feishu bootstrap, switches to Lark the moment the tenant reports it — even while authorization is still pending — and always saves the brand the credentials were issued on, within a single expiry budget that also cancels in-flight requests. Brand values are normalized wherever configuration enters the runtime, and a CI-enforced source guard keeps outbound hosts from bypassing the resolver again.
evandance
force-pushed
the
fix/endpoint-resolver-consolidation
branch
from
July 11, 2026 09:55
274f08a to
1756d39
Compare
PR Quality SummaryThe semantic review system could not produce a fully trusted result. This is not reported as a code defect. System status
|
liangshuo-1
approved these changes
Jul 11, 2026
This was referenced Jul 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Route the brand-sensitive, resolver-owned Go CLI endpoints touched here through the single
core.ResolveEndpointssource of truth. Feishu remains the default; Lark uses the corresponding resolver endpoints while app registration preserves its protocol-defined Feishu bootstrap and switches to Lark only after tenant discovery.This PR intentionally leaves package-manager version lookup, npm registry behavior, update caching, and general URL data-flow analysis unchanged.
Changes
core.ParseBrandat every runtime ingress (config resolution, credential accounts) and defensively at theResolveEndpointsboundaryinternal/authstate machine with Feishu begin and first poll, immediate tenant-signaled Lark switching (even when the signal accompanies authorization_pending), one expiry budget with no attempt cap, context-bound requests, and the issuing brand as the only saved brand (a contradictory final tenant report is a protocol error)Test Plan
user_info, no attempt cap, and rejection of contradictory final tenant reportslark, bot credentials verified, and the Lark Open and MCP endpoints were reachable; temporary local config and Keychain data were removed afterwardRelated Issues
N/A