fix: point permission-apply link at official /page/scope-apply entry#1722
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughConsole URL construction for scope-apply flows now uses ChangesScope-apply console URL construction
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/registry/scope_hint.go (1)
58-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating with
errclass.ConsoleURL.Both
BuildConsoleScopeURLhere anderrclass.ConsoleURLnow independently build the same/page/scope-apply?clientID=...&scopes=...URL shape on top ofcore.ResolveOpenBaseURL. Now that brand-to-host resolution is unified, consider extracting the shared "build scope-apply URL from base+appID+scopes" logic intointernal/core(or another shared package) to avoid future drift between the two implementations (e.g., differing escaping or query-param ordering).♻️ Example shared helper
// in internal/core func BuildScopeApplyURL(brand LarkBrand, appID string, scopes []string) string { if appID == "" { return "" } base := fmt.Sprintf("%s/page/scope-apply?clientID=%s", ResolveOpenBaseURL(brand), url.QueryEscape(appID)) if len(scopes) == 0 { return base } return base + "&scopes=" + url.QueryEscape(strings.Join(scopes, ",")) }Both
errclass.ConsoleURL(parsing a raw brand string) andregistry.BuildConsoleScopeURL(single scope) could then delegate to this helper.🤖 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/scope_hint.go` around lines 58 - 68, BuildConsoleScopeURL currently duplicates the same scope-apply console URL construction that errclass.ConsoleURL also performs, so consolidate this shared logic into a common helper in internal/core (or another shared package). Add a reusable helper that takes the resolved brand/base, appID, and scopes, then have BuildConsoleScopeURL delegate to it so escaping, query param formatting, and future changes stay consistent with errclass.ConsoleURL.
🤖 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/registry/scope_hint.go`:
- Around line 58-68: BuildConsoleScopeURL currently duplicates the same
scope-apply console URL construction that errclass.ConsoleURL also performs, so
consolidate this shared logic into a common helper in internal/core (or another
shared package). Add a reusable helper that takes the resolved brand/base,
appID, and scopes, then have BuildConsoleScopeURL delegate to it so escaping,
query param formatting, and future changes stay consistent with
errclass.ConsoleURL.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 24f2fc03-8378-41c4-801f-8ff83624b1fa
📒 Files selected for processing (3)
internal/errclass/classify.gointernal/errclass/classify_test.gointernal/registry/scope_hint.go
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1126d1711511b1731ec73f4a81266b0c91d147b9🧩 Skill updatenpx skills add larksuite/cli#fix/permission-apply-scope-apply-url -y -g |
The console_url carried on app-scope-not-applied errors (Lark
99991672/99991679) built the historical internal /app/{appID}/auth?q=
entry, which the open-platform open-pages spec explicitly warns against.
Switch it to the official /page/scope-apply?clientID=...&scopes=... entry;
clientID and the comma-joined scopes are query-escaped so hostile values
cannot inject extra parameters.
Also converge the brand->host mapping in both scope-apply URL builders
(errclass.ConsoleURL and registry.BuildConsoleScopeURL) onto
core.ResolveOpenBaseURL instead of re-deriving open.feishu.cn /
open.larksuite.com inline, so the open-platform base URL has a single
source of truth. errclass now imports internal/core (verified: no import
cycle); the stale comment claiming a cycle is removed.
2bd5b0c to
1126d17
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1722 +/- ##
==========================================
- Coverage 74.52% 74.40% -0.13%
==========================================
Files 851 854 +3
Lines 87155 88375 +1220
==========================================
+ Hits 64952 65752 +800
- Misses 17231 17552 +321
- Partials 4972 5071 +99 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
The
console_urlwe surface on app-scope-not-applied errors (Lark99991672/99991679) was built against the historical internal/app/{appID}/auth?q=entry, which the open-platform open-pages spec explicitly warns developers not to depend on. This switches it to the official application-scope apply entry,/page/scope-apply?clientID=...&scopes=....Changes
errclass.ConsoleURLnow emitshttps://<open-host>/page/scope-apply?clientID=<appID>&scopes=<comma-joined>;clientIDand the comma-joined scopes are query-escaped so hostile values cannot inject extra query parameters.errclass.ConsoleURLandregistry.BuildConsoleScopeURL) ontocore.ResolveOpenBaseURL, instead of re-derivingopen.feishu.cn/open.larksuite.cominline, so the open-platform base URL has a single source of truth.errclassnow importsinternal/core(verified: no import cycle —coredoes not depend onerrclass); the stale comment claiming a cycle is removed.classify_test.goassertions to the new entry format.Test Plan
Unit tests pass (
errclass,registry,client,cmd/api,shortcuts/common)Manual local verification against the live server (same app, same missing scopes; only the URL format changed):
https://open.feishu.cn/app/cli_xxx/auth?q=okr:okr.period:readonly,...https://open.feishu.cn/page/scope-apply?clientID=cli_xxx&scopes=okr:okr.period:readonly,...The host convergence is a zero-behavior-change refactor: existing tests assert the exact
open.feishu.cn/open.larksuite.comhosts and output is byte-identical.Related Issues
Summary by CodeRabbit
New Features
Bug Fixes
Tests