feat(platform): support multiple policy rules per plugin#1182
Merged
Conversation
Extend the command policy framework from single-Rule to multi-Rule semantics. A plugin (or policy.yml) may now contribute several scoped Rules; the engine combines them with OR -- a command is allowed when it satisfies every axis of at least one rule. This lets one integration apply different risk ceilings and identity restrictions to different command groups. The cross-plugin fail-closed boundary is preserved: two distinct plugins both calling Restrict still aborts startup (multiple_restrict_plugins). Single-Rule behaviour is fully backward compatible -- the rejection reason_code / rule_name / envelope shape are byte-for-byte unchanged; multi-rule rejection surfaces the aggregate reason_code no_matching_rule. - engine: New keeps single-rule compat, add NewSet for OR over rules - resolver: dedupe by owner (one plugin may contribute many rules), return []*Rule; yaml gains a top-level rules: list - registrar/builder/staging: Restrict may be called more than once; retire the double_restrict error - config policy show / config plugins show: emit a rules array - inventory: PluginEntry.Rules is now a slice (fixes last-rule-wins overwrite when a plugin contributes multiple rules)
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR converts the command-policy model from a single-rule to multi-rule system: YAML parsing and resolver return rule slices; the engine implements OR semantics and aggregates denials; staging/builder allow multiple Restrict calls; installation and inventory record plural rules; bootstrap applies multi-rule pruning; CLI outputs emit ChangesMulti-Rule Policy Framework
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 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
the
size/L
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1182 +/- ##
==========================================
+ Coverage 68.92% 68.95% +0.02%
==========================================
Files 629 629
Lines 58762 58841 +79
==========================================
+ Hits 40503 40575 +72
- Misses 14952 14959 +7
Partials 3307 3307 ☔ 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@b41cba9fc58281f094b253027e1e224523f9fbfd🧩 Skill updatenpx skills add larksuite/cli#feat/multi-rule-policy -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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/platform_bootstrap.go (1)
47-63:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let YAML parse failures disable plugin policy.
Resolvealready givesPluginRulesprecedence over YAML, but this code loads YAML first and bails out on any YAML error. If a plugin contributed restrictive rules and the user'spolicy.ymlis malformed, bootstrap now fail-opens and skips the plugin restrictions too.Suggested fix
- yamlRules, err := cmdpolicy.LoadYAMLPolicy(yamlPath) - if err != nil { - // Yaml-only failures are fail-OPEN at the caller (warn and - // continue), but the active-policy snapshot is process-global - // and may still carry data from a previous build in long-lived - // embedders / tests. Clear it explicitly so `config policy - // show` reports "no policy" instead of a stale rule that - // doesn't reflect the current command tree. - cmdpolicy.SetActive(nil) - return err - } + var yamlRules []*platform.Rule + if len(pluginRules) == 0 { + yamlRules, err = cmdpolicy.LoadYAMLPolicy(yamlPath) + if err != nil { + // Yaml-only failures are fail-OPEN at the caller (warn and + // continue), but the active-policy snapshot is process-global + // and may still carry data from a previous build in long-lived + // embedders / tests. Clear it explicitly so `config policy + // show` reports "no policy" instead of a stale rule that + // doesn't reflect the current command tree. + cmdpolicy.SetActive(nil) + return 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 `@cmd/platform_bootstrap.go` around lines 47 - 63, The YAML load error handling currently clears active plugin policies and returns early; instead, when cmdpolicy.LoadYAMLPolicy(yamlPath) returns an error, log or warn about the malformed YAML (including yamlPath and the error), do NOT call cmdpolicy.SetActive(nil) and do NOT return; set yamlRules to nil (or otherwise omit YAMLRules) and continue to call cmdpolicy.Resolve with PluginRules still present so plugin-contributed rules remain respected; ensure the function only returns an error if cmdpolicy.Resolve itself fails.
🧹 Nitpick comments (1)
cmd/config/policy_test.go (1)
60-96: ⚡ Quick winAdd a true multi-rule regression case.
This still only proves that
rulesexists for a single element. A bug that accidentally truncates output toactive.Rules[0]would still pass, even though this PR's contract is pluralrules[]. Please add a case with two rules and assert both are emitted.
🤖 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 `@extension/platform/builder.go`:
- Around line 130-137: Builder.Restrict currently appends the caller's *Rule
directly (b.rules = append(b.rules, rule)), which lets later mutations affect
earlier entries; change Restrict to append a fresh copy/clone of the rule
instead: after the nil check, create a new Rule instance that copies the
incoming rule (e.g. newRule := *rule or use rule.Clone() if a Clone method
exists) and append &newRule to b.rules so each call captures its own snapshot
and avoids shared-pointer mutations.
In `@internal/platform/inventory.go`:
- Around line 154-160: In BuildInventory, when constructing each RuleView (in
the entry.Rules append) you're reusing the source slices for Allow, Deny, and
Identities so later mutations to the original rules will affect the snapshot;
fix this by copying those slices into new slices before assigning to RuleView
(i.e. create new slices and copy values for Allow, Deny, and Identities when
building the RuleView in BuildInventory rather than relying on cloneInventory
later), referencing the RuleView struct and BuildInventory function to locate
where to perform the defensive copy.
---
Outside diff comments:
In `@cmd/platform_bootstrap.go`:
- Around line 47-63: The YAML load error handling currently clears active plugin
policies and returns early; instead, when cmdpolicy.LoadYAMLPolicy(yamlPath)
returns an error, log or warn about the malformed YAML (including yamlPath and
the error), do NOT call cmdpolicy.SetActive(nil) and do NOT return; set
yamlRules to nil (or otherwise omit YAMLRules) and continue to call
cmdpolicy.Resolve with PluginRules still present so plugin-contributed rules
remain respected; ensure the function only returns an error if cmdpolicy.Resolve
itself fails.
🪄 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: d0490c36-c3f9-4557-809f-90a9331d0d12
📒 Files selected for processing (20)
cmd/config/plugins.gocmd/config/policy.gocmd/config/policy_test.gocmd/platform_bootstrap.goextension/platform/README.mdextension/platform/builder.goextension/platform/registrar.gointernal/cmdpolicy/active.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/engine_test.gointernal/cmdpolicy/resolver.gointernal/cmdpolicy/resolver_test.gointernal/cmdpolicy/yaml/schema.gointernal/cmdpolicy/yaml/schema_test.gointernal/platform/error.gointernal/platform/host.gointernal/platform/host_test.gointernal/platform/inventory.gointernal/platform/inventory_test.gointernal/platform/staging.go
💤 Files with no reviewable changes (1)
- internal/platform/error.go
Address review feedback. Builder.Restrict stored the caller's *Rule directly, so reusing and mutating one Rule object across multiple Restrict calls collapsed entries to the last mutation; clone the rule and its slices on append, mirroring the staging registrar. BuildInventory likewise reused the source Allow/Deny/Identities slices; copy them when building the RuleView snapshot instead of relying on cloneInventory downstream. Add a regression test: reusing and mutating one Rule across two Restrict calls now yields two independent rules.
…s list
Two policy-config robustness fixes from review:
- A malformed ~/.lark-cli/policy.yml could abort a plugin-governed
binary. applyUserPolicyPruning read yaml before resolving, and
build.go fail-closes on any policy error when a plugin is present.
Plugin rules shadow yaml anyway, so skip reading yaml entirely when a
plugin contributed rules -- an unrelated broken file on the user's
machine can no longer lock the CLI.
- A present-but-empty "rules: []" collapsed to a single all-zero Rule
that allows every annotated command ("looks like policy, enforces
almost nothing"). yaml.Parse now distinguishes absent from
present-but-empty (Rules is a pointer) and rejects the empty list.
Add regression tests for both.
liangshuo-1
approved these changes
May 30, 2026
tuxedomm
pushed a commit
to zhumiaoxin/cli
that referenced
this pull request
Jun 6, 2026
) * feat(platform): support multiple policy rules per plugin Extend the command policy framework from single-Rule to multi-Rule semantics. A plugin (or policy.yml) may now contribute several scoped Rules; the engine combines them with OR -- a command is allowed when it satisfies every axis of at least one rule. This lets one integration apply different risk ceilings and identity restrictions to different command groups. The cross-plugin fail-closed boundary is preserved: two distinct plugins both calling Restrict still aborts startup (multiple_restrict_plugins). Single-Rule behaviour is fully backward compatible -- the rejection reason_code / rule_name / envelope shape are byte-for-byte unchanged; multi-rule rejection surfaces the aggregate reason_code no_matching_rule. - engine: New keeps single-rule compat, add NewSet for OR over rules - resolver: dedupe by owner (one plugin may contribute many rules), return []*Rule; yaml gains a top-level rules: list - registrar/builder/staging: Restrict may be called more than once; retire the double_restrict error - config policy show / config plugins show: emit a rules array - inventory: PluginEntry.Rules is now a slice (fixes last-rule-wins overwrite when a plugin contributes multiple rules) * fix(platform): clone rules in Builder.Restrict and inventory snapshot Address review feedback. Builder.Restrict stored the caller's *Rule directly, so reusing and mutating one Rule object across multiple Restrict calls collapsed entries to the last mutation; clone the rule and its slices on append, mirroring the staging registrar. BuildInventory likewise reused the source Allow/Deny/Identities slices; copy them when building the RuleView snapshot instead of relying on cloneInventory downstream. Add a regression test: reusing and mutating one Rule across two Restrict calls now yields two independent rules. * fix(platform): skip yaml when a plugin owns policy; reject empty rules list Two policy-config robustness fixes from review: - A malformed ~/.lark-cli/policy.yml could abort a plugin-governed binary. applyUserPolicyPruning read yaml before resolving, and build.go fail-closes on any policy error when a plugin is present. Plugin rules shadow yaml anyway, so skip reading yaml entirely when a plugin contributed rules -- an unrelated broken file on the user's machine can no longer lock the CLI. - A present-but-empty "rules: []" collapsed to a single all-zero Rule that allows every annotated command ("looks like policy, enforces almost nothing"). yaml.Parse now distinguishes absent from present-but-empty (Rules is a pointer) and rejects the empty list. Add regression tests for both.
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.
What
Extend the command policy framework (
internal/cmdpolicy+extension/platform) from single-Rule to multi-Rule semantics. A plugin — or~/.lark-cli/policy.yml— may now contribute several scopedRules. The engine combines them with OR: a command is allowed when it satisfies every axis (allow / deny / max_risk / identities) of at least one rule.Why
Real integrations need different policy per command group — e.g.
docs/**read-only whileim/**is writable, or some commands restricted to theuseridentity while others also allowbot. A single global Rule (onemax_risk, one identity set) cannot express this.Behaviour
denyis scoped to that rule and cannot veto another rule's allow — to block a command everywhere, deny it (or omit it from allow) in every rule.Restrictmultiple times, but two distinct plugins both contributing rules still aborts startup (multiple_restrict_plugins) — independent plugins cannot widen each other's policy.reason_code/rule_name/ envelope shape. Multi-rule "no rule matched" surfaces the new aggregateno_matching_rule.policy.ymlgains a top-levelrules:list (flat single-rule layout still supported). A present-but-emptyrules: []is rejected. When a plugin contributes rules, yaml is shadowed and not read at all (a broken user yaml can't abort a plugin-governed binary).API / diagnostic changes
cmdpolicy.New(*Rule)kept for compat; newNewSet([]*Rule)evaluates OR.config policy show/config plugins shownow emit arulesarray (was a singleruleobject). Scripts parsing.rulemust read.rules[].no_matching_rule; retireddouble_restrict.config plugins showpreviously overwrote down to the last rule when a plugin contributed several — now lists all.Testing
no_matching_rule/ single-rule compat), resolver (single-plugin multi-rule, cross-plugin abort), inventory (multi-rule, no last-rule overwrite), yaml (rules:list, flat+list mix rejection, empty-list rejection), builder/host (multipleRestrict), bootstrap (plugin rules shadow and skip a broken yaml).go build ./...+go vet+ full non-integrationgo testgreen;gofmtclean.no_matching_rule,config policy/plugins showrules array, single-rule backward-compat, cross-plugin abort, yaml multi-rule — all observed working. This was a development-time check against a throwaway embedder, not an in-repo regression suite.Known follow-up
A rule's
denyonly applies within that rule, so a later grant rule can re-expose a command an earlier rule denied. This is the OR contract (documented), but it is an easy operator foot-gun on a security surface. Tracked separately for a future safeguard (set-level lint / global deny /config policy explain); out of scope here.Summary by CodeRabbit
New Features
Documentation
Tests