feat(apps): gate apps domain off on Lark brand#1025
Merged
Conversation
The Miaoda apps OpenAPI is Feishu-only. On Lark brand: - shortcut subtree is registered + hidden, RunE returns a structured brand-restriction error so users see a clear message instead of cobra's generic "unknown command" - auth login `--domain apps` is treated as unknown; `--domain all` skips apps; help text omits it - scope collection skips apps shortcuts so spark:* scopes are never requested The leaf-stub pattern mirrors internal/cmdpolicy/apply.go::installDenyStub (DisableFlagParsing + ArbitraryArgs + leaf-level PersistentPreRunE override) so cobra can't short-circuit the stub with a missing-flag or parent-PreRunE detour. Change-Id: I5817e87ae6fedabdb5faf05d0d32ea988f7effc9
github-actions
Bot
added
the
size/L
|
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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAuth login derives the CLI brand and uses it to filter known domains, shortcut services, and scope collection; shortcut registration enforces brand-restricted services (hiding and blocking execution). Tests exercise both domain/scope filtering and the runtime guard behavior. ChangesBrand-aware domain filtering and service restrictions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 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]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
license-eye has checked 1614 files.
| Valid | Invalid | Ignored | Fixed |
|---|---|---|---|
| 1158 | 1 | 455 | 0 |
Click to see the invalid file list
- cmd/auth/login_brand_filter_test.go
Use this command to fix any missing license headers
```bash
docker run -it --rm -v $(pwd):/github/workspace apache/skywalking-eyes header fix
</details>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1025 +/- ##
==========================================
+ Coverage 67.75% 67.77% +0.02%
==========================================
Files 590 590
Lines 55245 55291 +46
==========================================
+ Hits 37432 37475 +43
- Misses 14693 14696 +3
Partials 3120 3120 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@8a9cddd58ef47eaf1997b819c0b364913605b8b7🧩 Skill updatenpx skills add larksuite/cli#feat/apps-brand-gate-lark -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/auth/login_interactive.go (1)
109-125:⚠️ Potential issue | 🟠 Major | ⚡ Quick winInteractive picker still surfaces brand-restricted services (e.g.
apps) on Lark.
runInteractiveLoginthreadsbrandonly intocollectScopesForDomains(Line 169), butgetDomainMetadata(lang)at Line 110 is brand-unaware and iteratesshortcuts.AllShortcuts()withoutshortcuts.IsShortcutServiceAvailable. Net effect on Lark brand:
- The multi-select shows
appsas a selectable option.- If the user selects only
apps, scope collection returns empty and the caller surfaces a generic"no matching scopes found"validation error.- If the user selects
appsalongside other domains, no error is raised but theappsselection is silently dropped from the requested scope set.This contradicts the PR objective ("Prevent the CLI from presenting …
apps… on Lark") and is inconsistent with the brand-filtered--help(sortedKnownDomains(brand)) and--domainvalidation (allKnownDomains(brand)) inlogin.go.♻️ Proposed fix: thread brand into domain metadata
-func getDomainMetadata(lang string) []domainMeta { +func getDomainMetadata(lang string, brand core.LarkBrand) []domainMeta { seen := make(map[string]bool) var domains []domainMeta // 1. Domains from from_meta projects (skip domains with auth_domain) for _, project := range registry.ListFromMetaProjects() { if registry.HasAuthDomain(project) { seen[project] = true continue } dm := buildDomainMeta(project, lang) domains = append(domains, dm) seen[project] = true } // 2. Shortcut-only domains shortcutOnlyNames := getShortcutOnlyDomainNames() for _, name := range shortcutOnlyNames { - if !seen[name] { + if !seen[name] && shortcuts.IsShortcutServiceAvailable(name, brand) { dm := buildDomainMeta(name, lang) domains = append(domains, dm) seen[name] = true } } // 3. Auto-discover remaining shortcut services... shortcutOnlySet := make(map[string]bool) for _, n := range shortcutOnlyNames { shortcutOnlySet[n] = true } for _, sc := range shortcuts.AllShortcuts() { + if !shortcuts.IsShortcutServiceAvailable(sc.Service, brand) { + continue + } if !seen[sc.Service] { if shortcutOnlySet[sc.Service] && !registry.HasAuthDomain(sc.Service) { dm := buildDomainMeta(sc.Service, lang) domains = append(domains, dm) } seen[sc.Service] = true } }And update the caller:
- allDomains := getDomainMetadata(lang) + allDomains := getDomainMetadata(lang, brand)The corresponding
TestGetDomainMetadata_*tests incmd/auth/login_test.goalso need to be updated to pass a brand argument.🤖 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_interactive.go` around lines 109 - 125, runInteractiveLogin currently calls getDomainMetadata(lang) which is brand-unaware and lets brand-restricted services (e.g. "apps") appear; change getDomainMetadata to accept a core.LarkBrand (or similar) and filter domain list using shortcuts.IsShortcutServiceAvailable when iterating shortcuts.AllShortcuts(), then update runInteractiveLogin to pass the brand into getDomainMetadata and any other callers (e.g. tests and the places that call sortedKnownDomains/allKnownDomains) so the interactive picker, --help and --domain validation all use the same brand-aware domain set; also update TestGetDomainMetadata_* tests to pass the brand argument.
🧹 Nitpick comments (1)
shortcuts/register_brand_guard_test.go (1)
18-24: ⚡ Quick winUse
cmdutil.TestFactoryhere.This helper manually assembles
cmdutil.Factory, which skips the repo’s standard unit-test factory setup. Please build it withcmdutil.TestFactory(t, &core.CliConfig{Brand: brand})instead and threadtthrough the helper.As per coding guidelines,
**/*_test.go: Usecmdutil.TestFactory(t, config)for test factories in unit tests.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shortcuts/register_brand_guard_test.go` around lines 18 - 24, The test helper newFactoryWithBrand currently constructs a cmdutil.Factory manually; change it to accept testing.T (thread t through the helper) and return cmdutil.TestFactory(t, &core.CliConfig{Brand: brand}) instead of assembling cmdutil.Factory directly so the repo's standard test factory setup is used (update the helper signature and all call sites to pass t).
🤖 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.
Outside diff comments:
In `@cmd/auth/login_interactive.go`:
- Around line 109-125: runInteractiveLogin currently calls
getDomainMetadata(lang) which is brand-unaware and lets brand-restricted
services (e.g. "apps") appear; change getDomainMetadata to accept a
core.LarkBrand (or similar) and filter domain list using
shortcuts.IsShortcutServiceAvailable when iterating shortcuts.AllShortcuts(),
then update runInteractiveLogin to pass the brand into getDomainMetadata and any
other callers (e.g. tests and the places that call
sortedKnownDomains/allKnownDomains) so the interactive picker, --help and
--domain validation all use the same brand-aware domain set; also update
TestGetDomainMetadata_* tests to pass the brand argument.
---
Nitpick comments:
In `@shortcuts/register_brand_guard_test.go`:
- Around line 18-24: The test helper newFactoryWithBrand currently constructs a
cmdutil.Factory manually; change it to accept testing.T (thread t through the
helper) and return cmdutil.TestFactory(t, &core.CliConfig{Brand: brand}) instead
of assembling cmdutil.Factory directly so the repo's standard test factory setup
is used (update the helper signature and all call sites to pass t).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3f3da1f8-d801-4e8e-8358-c62cc8739fa9
📒 Files selected for processing (6)
cmd/auth/login.gocmd/auth/login_brand_filter_test.gocmd/auth/login_interactive.gocmd/auth/login_test.goshortcuts/register.goshortcuts/register_brand_guard_test.go
Change-Id: Ic65b4eb17b982dab6a7c5382fb26e4e678da18fd
AI consumers parsed 'not currently available' as transient (retry-worthy). 'not yet supported' makes it categorical: feature isn't done on this brand, no point retrying or switching params. Change-Id: I5a757700a9a242c065db4a53101dc4babc101c7c
…k brand Without this, an AI consumer on Lark brand reads the skill description, sees a match on 'deploy HTML' intent, runs through the end-to-end flow, and only fails at step 1 when the CLI returns the brand error. Surface the limitation up front: in the description (so the skill router sees it) and as the first body section (so any consumer reading SKILL.md hits it before reading the operational guidance). Change-Id: Ie316178fe361cc570756568aabea4c64d1ff80be
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 `@skills/lark-apps/SKILL.md`:
- Around line 16-20: The docs require jq for the command `lark-cli config show
--format json | jq -r '.brand'` but the package metadata only declares
`lark-cli` in `metadata.requires.bins`; either add `jq` to
`metadata.requires.bins` or add a no-`jq` fallback command (e.g., parse JSON in
a POSIX-tool-safe way or use `lark-cli`’s own flags) and document both options
in SKILL.md so environments that install only declared prerequisites won't fail;
update references to `metadata.requires.bins` and the example command
accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Trim per Anthropic skills convention (anthropics/skills repo):
- description: capability-first opener, no CLI path in parens, no
keyword soup, no execution policy duplication (already covered in
body 端到端流程 + 快速决策). Adds explicit negative boundaries vs
lark-doc / lark-drive / lark-slides — the slides boundary avoids
routing conflict on 'PPT'/'演示文稿'.
- cliHelp: single 'lark-cli apps --help' (matches 8/10 other skills;
the 5-entry enumeration was a one-off).
- Body: replace the ⛔ banner + config-show precheck with a one-line
prereq under its own '品牌可用性（先做）' H2. Detection is semantic
('若提示暂未支持') rather than substring match, and the user-facing
phrasing is left to the AI rather than a verbatim template.
Change-Id: Ib3bbfc5bcbb31ef15033c883d224d8de9686ec16
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
1 participant
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
The Miaoda apps OpenAPI is Feishu-only. This PR makes both the CLI and the
lark-appsskill fail cleanly on Lark brand instead of presenting commands and scopes that can't possibly work.appsshortcuts stay registered but Hidden on Lark; every node under the subtree returns a structuredtype=validationerror (the "apps" feature is not yet supported on the Lark brand) instead of cobra'sunknown command. Wording avoids "currently unavailable" so AI consumers don't misread it as a transient failure worth retrying.lark-cli help appsreaches cobra's help renderer (bypassing RunE) and shows the same note viaLong.--domain appsis rejected as unknown;--domain allskips apps; help-text domain list omits it; scope collection skips apps shortcuts sospark:*scopes never get requested.lark-appsskill: brand-restriction flagged in both the frontmatterdescription(so skill-routing sees it before triggering) and a prominent banner at the top ofSKILL.md(so any consumer hits it before reading the end-to-end flow). The banner tells the AI to runlark-cli config show --format json | jq -r '.brand'first, stop immediately onlark, and not to suggest brand-switching.internal/cmdpolicy/apply.go::installDenyStub—DisableFlagParsing+ArbitraryArgs+ leaf-levelPersistentPreRunEso cobra can't short-circuit with a missing-required-flag error or a parent-PreRunE detour before our stub runs.brandRestrictedServicesinshortcuts/register.go— adding future brand restrictions is one map entry, no per-shortcut changes.Behavior matrix (Lark brand)
DisableFlagParsing=truecauses--help/-h/ unknown subcommands to fall through to our RunE stub as positional args. The only path that still hits cobra's help renderer is the explicithelpsubcommand.lark-cli --helplark-cli appstype=validation, brand errorlark-cli apps --help/-hlark-cli apps +create --name xlark-cli apps +create --helplark-cli apps +nonexistentlark-cli help appsLongbrand note — only entry that bypasses RunElark-cli auth login --domain appsunknown domain "apps"lark-cli auth login --domain allFeishu brand behavior is unchanged (verified via built binary —
apps --helpstill lists all 6 subcommands).Test plan
go test ./shortcuts/... ./cmd/auth/... -count=1— all greengo vetcleanshortcuts/register_brand_guard_test.go— 4 cases: hidden on Lark, dispatch returns brand error, untouched on Feishu, cobra-end-to-end dispatchcmd/auth/login_brand_filter_test.go—--domain apps/ scope collection filtered on Larkapps +createreturns the validation envelope;lark-cli apps --helpreturns validation error (not help text);lark-cli help appsshows the Long noteSummary by CodeRabbit
New Features
Tests
Documentation