feat(extension): present restricted commands as absent and trim skills#1837
Open
evandance wants to merge 1 commit into
Open
feat(extension): present restricted commands as absent and trim skills#1837evandance wants to merge 1 commit into
evandance wants to merge 1 commit into
Conversation
|
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 adds embedded skill overlays for callers and plugins, validates and composes skill filesystems, introduces plugin-specific unavailable errors, and applies plugin-denied policy state to CLI help, flags, notices, diagnostics, and recovery hints. ChangesEmbedded skills and plugin integration
Policy-aware command presentation
Estimated code review effort: 5 (Critical) | ~120 minutes 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 |
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@9cf835d56f5dea2617ce5f8a8479d1e053d9ba48🧩 Skill updatenpx skills add larksuite/cli#feat/plugin-trim -y -g |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #1837 +/- ##
==========================================
+ Coverage 74.45% 74.54% +0.09%
==========================================
Files 860 865 +5
Lines 89906 90466 +560
==========================================
+ Hits 66940 67439 +499
- Misses 17780 17818 +38
- Partials 5186 5209 +23 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (7)
extension/platform/registrar.go (1)
39-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment should start with
EmbeddedSkills, notSkills.Go doc convention expects the first word to match the symbol name so
go docproduces a correct synopsis. Currently it reads "Skills contributes a SkillsOverlay..." instead of "EmbeddedSkills contributes a SkillsOverlay...".📝 Proposed fix
- // Skills contributes a SkillsOverlay customizing the CLI's embedded skill + // EmbeddedSkills contributes a SkillsOverlay customizing the CLI's embedded skill🤖 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 `@extension/platform/registrar.go` around lines 39 - 51, Update the doc comment on EmbeddedSkills in registrar.go so the first word matches the method name; change the opening sentence from “Skills contributes...” to “EmbeddedSkills contributes...” while keeping the rest of the comment unchanged. This ensures go doc generates the correct synopsis for the EmbeddedSkills symbol.internal/skillpolicy/resolver_test.go (1)
107-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider adding a test for Remove+Overlay targeting the same skill.
The composition order (Base → Allow → Remove → Overlay) means an Overlay entry should win over a Remove entry for the same skill name.
effectiveRemovedocuments this ("Overlay entries are never folded in — the overlay FS resolves them from the upper layer before the whiteout applies"), but no test explicitly verifies this interaction. A same-name Remove+Overlay test would lock in this precedence behavior.📝 Suggested test
// Overlay wins over Remove for the same skill name: the overlay // entry is served from the upper layer before the whiteout applies. func TestResolve_OverlayWinsOverRemove(t *testing.T) { overlay := skillFS(map[string]string{"lark-a/SKILL.md": "overlaid a"}) got := mustResolve(t, baseTree(), &platform.SkillsOverlay{ Remove: []string{"lark-a"}, Overlay: overlay, }) if want := []string{"lark-a", "lark-b", "lark-shared"}; !equalStrings(topLevel(t, got), want) { t.Errorf("top level = %v, want %v (overlay restores removed skill)", topLevel(t, got), want) } if c := readFile(t, got, "lark-a/SKILL.md"); c != "overlaid a" { t.Errorf("lark-a content = %q, want overlaid a (overlay wins over remove)", c) } }Also applies to: 168-176
🤖 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/skillpolicy/resolver_test.go` around lines 107 - 119, Add a test in resolver_test.go that covers Remove and Overlay targeting the same skill name, since the current tests only verify same-name Overlay replacing the base skill. Use mustResolve with platform.SkillsOverlay on baseTree, setting both Remove and Overlay for lark-a, and assert that Overlay wins by restoring lark-a in topLevel and serving lark-a/SKILL.md from the overlay. This should lock in the precedence behavior documented by effectiveRemove and the Base → Allow → Remove → Overlay composition.extension/platform/README.md (1)
117-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify
OverlayvsBasedistinction in the safety contract.Line 121 reads "
Overlayadds/replaces ones, or swap the wholeBase" — the verb "swap" breaks the parallel structure ("keeps", "drops", "adds/replaces") and conflates theOverlayfield with theBasefield. Consider separating them for clarity.📝 Suggested rewording
- `Remove` wins over `Allow`, and `Overlay` entries are exempt), `Remove` - drops skills, `Overlay` adds/replaces ones, or swap the whole `Base` — - layered over the CLI default. + `Remove` wins over `Allow`, and `Overlay` entries are exempt), `Remove` + drops skills, `Overlay` adds/replaces ones, and `Base` swaps the + entire base tree — all layered over the CLI default.🤖 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 `@extension/platform/README.md` around lines 117 - 145, Clarify the safety-contract wording in the EmbeddedSkills docs: the current sentence in the README blurs `Overlay` and `Base` by using “swap,” which breaks the parallel list of actions. Update the text around `EmbeddedSkills()`, `Allow`, `Remove`, `Overlay`, and `Base` so `Overlay` is described as adding/replacing entries and `Base` is described separately as replacing the whole tree, keeping the phrasing parallel and unambiguous.internal/skillpolicy/overlay.go (1)
37-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument why the
fs.ReadDir(upper, ".")error is intentionally ignored.If the upper FS root is unreadable,
upperNamesstays empty androute()silently falls through to lower for all paths, whilemergedRoot()(line 140) will still attempt the same read and propagate the error. The resolver likely validates the upper FS before callingnewOverlayFS, but a one-line comment would prevent future readers from mistaking this for a bug.📝 Suggested comment
if upper != nil { - if entries, err := fs.ReadDir(upper, "."); err == nil { + // upperNames is populated eagerly so route() can decide + // without per-call ReadDir. A read failure here is benign: + // the resolver validates the overlay FS before calling + // newOverlayFS, and mergedRoot() will surface the error + // if it ever occurs at read time. + if entries, err := fs.ReadDir(upper, "."); err == nil {🤖 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/skillpolicy/overlay.go` around lines 37 - 57, Add a brief comment in newOverlayFS explaining that the fs.ReadDir(upper, ".") error is intentionally ignored because an unreadable upper root just leaves upperNames empty and lets route() fall back to lower, while mergedRoot() handles and surfaces the error separately. Keep the note near the upper != nil / fs.ReadDir block so future readers understand the intended behavior.cmd/presentation.go (1)
104-124: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
sampledenial can be picked from the wrong plugin when multiple plugins restrict the same domain group.
sampleis chosen by matching only path-prefix +IsPluginPolicySource, without pinning to a single owningPolicySource. If two different plugins each deny distinct sub-paths under the same top-level group (e.g. one restrictsconfig/policy, anotherconfig/plugins), the aggregate denial installed on the group inheritsRuleName/DeniedMessagefrom whichever entry the (randomized) map iteration happens to hit first, giving inconsistent attribution across runs.Given this is a narrower edge case (only surfaces with multiple plugins denying disjoint paths under one diagnostic group), please confirm whether that scenario is reachable in practice; if so, consider deterministically selecting
sample(e.g. sort candidates or require a single denying source per group) rather than relying on map iteration order.🤖 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/presentation.go` around lines 104 - 124, The aggregate group denial in presentation logic is attributing metadata from an arbitrary plugin because `sample` is taken from the first matching entry in `denied`, which depends on map iteration order. Update the selection in `cmd/presentation.go` around the `sample`/`Apply` block to deterministically choose a single owning `PolicySource` for the group (or otherwise enforce one source per group), and only then copy `RuleName`, `DeniedMessage`, and related fields into the synthesized denial.internal/cmdpolicy/apply.go (1)
133-249: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
BuildUnavailableErrorduplicatesmessageOf's resolution logic.
messageOf(lines 144-149) andBuildUnavailableError(lines 156-159) implement the identical "DeniedMessage or DefaultUnavailableMessage" fallback independently.installDenyStubeven callsmessageOf(d)for the annotation just above whereBuildUnavailableErrorre-derives the same value. Consolidating avoids future divergence if the fallback logic changes.♻️ Proposed fix
func BuildUnavailableError(path string, d Denial) *errs.ValidationError { - msg := d.DeniedMessage - if msg == "" { - msg = DefaultUnavailableMessage - } - return errs.NewValidationError(errs.SubtypeCommandUnavailable, "%s", msg). + return errs.NewValidationError(errs.SubtypeCommandUnavailable, "%s", messageOf(d)). WithCause(CommandDeniedFromDenial(path, d)) }🤖 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/cmdpolicy/apply.go` around lines 133 - 249, BuildUnavailableError duplicates the denied-message fallback already handled by messageOf, so the two paths can drift. Update BuildUnavailableError to reuse messageOf(d) instead of re-deriving the default message, keeping the existing behavior for plugin sources and the Denial capture in installDenyStub consistent. This change should be made in the BuildUnavailableError and messageOf helpers so there is a single source of truth for the unavailable-message resolution.internal/cmdpolicy/diagnostic.go (1)
35-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSort
DiagnosticPaths()output for deterministic ordering.Map iteration in Go is non-deterministic, so the returned slice order varies between calls. While the current downstream consumer (
cmd/presentation.go) only iterates to hide paths—making order irrelevant for correctness—any future test that asserts the slice directly will be flaky. Sorting is a one-liner that makes the exported API contract deterministic.♻️ Proposed fix: sort output
func DiagnosticPaths() []string { out := make([]string, 0, len(diagnosticPaths)) for p := range diagnosticPaths { out = append(out, p) } + sort.Strings(out) return out }You'll also need to add
"sort"to the import block.🤖 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/cmdpolicy/diagnostic.go` around lines 35 - 41, DiagnosticPaths() currently returns map keys in non-deterministic order because it iterates diagnosticPaths directly, so make the exported result deterministic by sorting the slice before returning it. Update DiagnosticPaths() in diagnostic.go to collect the keys as it does now, then apply a sort step using the standard sort package, and add sort to the imports so any callers and tests get stable ordering.
🤖 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/show.go`:
- Around line 59-64: Add test coverage for the profile hint gate in
configShowRun: the existing no-active-profile case in config_test.go should be
expanded to verify both outcomes of policystate.DomainDeniedByPlugin("profile").
Add one test that stubs the domain as allowed and asserts the returned
ConfigError from configShowRun includes the “run: lark-cli profile list” hint,
and another that stubs it as denied and asserts the hint is omitted. Use the no
active profile path and configShowRun as the main symbols to locate the logic.
In `@cmd/presentation.go`:
- Around line 52-72: The message selection in retireDiagnostics is picking any
plugin-sourced denial instead of the one owned by
hideDiagnosticsOwner(installResult.Plugins), which can make the diagnostics
message nondeterministic. Update the loop in retireDiagnostics to match on the
computed source value as well as cmdpolicy.IsPluginPolicySource, so only the
denial from the actual HideDiagnostics owner can populate message before
cmdpolicy.Apply builds the diagnostics map.
In `@cmd/prune.go`:
- Around line 109-116: Add tests for the config-denied hint branch in
strictModeStubFrom, since cmd/prune_test.go currently only covers the default
stub envelope. Extend the existing prune tests to exercise both outcomes of
policystate.DomainDeniedByPlugin("config"): one case where it returns true and
the returned validation error hint omits stubHint, and one case where it returns
false and the hint includes it. Use strictModeStubFrom, DomainDeniedByPlugin,
and the validation error/hint assertions to keep the conditional behavior locked
in.
In `@cmd/skill_customization_test.go`:
- Around line 144-167: In TestBuildInternal_invalidSkillsOverlayGuard, replace
the brittle Hint substring check on errs.ValidationError with typed metadata
assertions using errs.ProblemOf, mirroring
TestBuildInternal_multipleSkillPluginsGuard. Verify the returned error preserves
the expected category/subtype/param for invalid_skills_overlay and keep the
existing errors.As check so the test validates structured error data instead of
message text alone.
- Around line 198-221: The conflict test for buildInternal only checks
verr.Hint, so update TestBuildInternal_withSkillsPlusPluginConflicts to assert
typed error metadata as well. Use errs.ProblemOf on the returned
*errs.ValidationError from leaf.RunE and verify the expected
category/subtype/param (matching the pattern already used in
TestBuildInternal_multipleSkillPluginsGuard), instead of relying on a Hint
substring alone.
In `@internal/platform/inventory_test.go`:
- Line 39: Add a focused BuildInventory test that passes a non-nil
SkillsInventorySource instead of nil so the EmbeddedSkills path is exercised.
Update the existing internalplatform.BuildInventory call in inventory_test.go
and assert the resulting inventory includes the embedded skills behavior, using
BuildInventory, EmbeddedSkills, and SkillsInventorySource as the key symbols to
locate the change.
In `@internal/platform/inventory.go`:
- Around line 188-194: In BuildInventory, the EmbeddedSkills view is only
cloning View.Remove while leaving View.Allow shared, which is inconsistent with
cloneInventory and stagingRegistrar.EmbeddedSkills. Update the code that builds
v from sk.View to defensively clone both Allow and Remove before assigning
entry.EmbeddedSkills, using the same pattern already used elsewhere so later
mutations to the source skill view cannot affect the inventory data.
---
Nitpick comments:
In `@cmd/presentation.go`:
- Around line 104-124: The aggregate group denial in presentation logic is
attributing metadata from an arbitrary plugin because `sample` is taken from the
first matching entry in `denied`, which depends on map iteration order. Update
the selection in `cmd/presentation.go` around the `sample`/`Apply` block to
deterministically choose a single owning `PolicySource` for the group (or
otherwise enforce one source per group), and only then copy `RuleName`,
`DeniedMessage`, and related fields into the synthesized denial.
In `@extension/platform/README.md`:
- Around line 117-145: Clarify the safety-contract wording in the EmbeddedSkills
docs: the current sentence in the README blurs `Overlay` and `Base` by using
“swap,” which breaks the parallel list of actions. Update the text around
`EmbeddedSkills()`, `Allow`, `Remove`, `Overlay`, and `Base` so `Overlay` is
described as adding/replacing entries and `Base` is described separately as
replacing the whole tree, keeping the phrasing parallel and unambiguous.
In `@extension/platform/registrar.go`:
- Around line 39-51: Update the doc comment on EmbeddedSkills in registrar.go so
the first word matches the method name; change the opening sentence from “Skills
contributes...” to “EmbeddedSkills contributes...” while keeping the rest of the
comment unchanged. This ensures go doc generates the correct synopsis for the
EmbeddedSkills symbol.
In `@internal/cmdpolicy/apply.go`:
- Around line 133-249: BuildUnavailableError duplicates the denied-message
fallback already handled by messageOf, so the two paths can drift. Update
BuildUnavailableError to reuse messageOf(d) instead of re-deriving the default
message, keeping the existing behavior for plugin sources and the Denial capture
in installDenyStub consistent. This change should be made in the
BuildUnavailableError and messageOf helpers so there is a single source of truth
for the unavailable-message resolution.
In `@internal/cmdpolicy/diagnostic.go`:
- Around line 35-41: DiagnosticPaths() currently returns map keys in
non-deterministic order because it iterates diagnosticPaths directly, so make
the exported result deterministic by sorting the slice before returning it.
Update DiagnosticPaths() in diagnostic.go to collect the keys as it does now,
then apply a sort step using the standard sort package, and add sort to the
imports so any callers and tests get stable ordering.
In `@internal/skillpolicy/overlay.go`:
- Around line 37-57: Add a brief comment in newOverlayFS explaining that the
fs.ReadDir(upper, ".") error is intentionally ignored because an unreadable
upper root just leaves upperNames empty and lets route() fall back to lower,
while mergedRoot() handles and surfaces the error separately. Keep the note near
the upper != nil / fs.ReadDir block so future readers understand the intended
behavior.
In `@internal/skillpolicy/resolver_test.go`:
- Around line 107-119: Add a test in resolver_test.go that covers Remove and
Overlay targeting the same skill name, since the current tests only verify
same-name Overlay replacing the base skill. Use mustResolve with
platform.SkillsOverlay on baseTree, setting both Remove and Overlay for lark-a,
and assert that Overlay wins by restoring lark-a in topLevel and serving
lark-a/SKILL.md from the overlay. This should lock in the precedence behavior
documented by effectiveRemove and the Base → Allow → Remove → Overlay
composition.
🪄 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: 26da75ec-40e3-4e44-b400-0933e3775fb9
📒 Files selected for processing (44)
cmd/build.gocmd/config/plugins.gocmd/config/plugins_test.gocmd/config/show.gocmd/flag_gate.gocmd/platform_bootstrap.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/presentation.gocmd/presentation_test.gocmd/prune.gocmd/root.gocmd/root_risk_help_test.gocmd/service/affordance_test.gocmd/skill_customization_test.goerrs/subtypes.goextension/platform/README.mdextension/platform/builder.goextension/platform/builder_test.goextension/platform/capabilities.goextension/platform/registrar.goextension/platform/rule.goextension/platform/skillsoverlay.gointernal/auth/errors.gointernal/auth/hint_gate_test.gointernal/client/client.gointernal/client/hint_gate_test.gointernal/cmdpolicy/active.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/denial.gointernal/cmdpolicy/diagnostic.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/source_label_test.gointernal/platform/error.gointernal/platform/host.gointernal/platform/host_test.gointernal/platform/inventory.gointernal/platform/inventory_test.gointernal/platform/staging.gointernal/policystate/policystate.gointernal/skillpolicy/overlay.gointernal/skillpolicy/resolver.gointernal/skillpolicy/resolver_test.go
evandance
force-pushed
the
feat/plugin-trim
branch
from
9cba7bd to
419d7df
Compare
evandance
force-pushed
the
feat/plugin-trim
branch
2 times, most recently
from
efa636f to
965c771
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
internal/skillpolicy/resolver_test.go (2)
69-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid calling
topLevel(t, got)twice in error assertions.The pattern
!equalStrings(topLevel(t, got), want)followed byt.Errorf("top level = %v, want %v", topLevel(t, got), want)invokestopLevel(and itsfs.ReadDir+ sort) twice. Extract to a local variable for clarity and to avoid redundant I/O:- if want := []string{"lark-a", "lark-b", "lark-shared"}; !equalStrings(topLevel(t, got), want) { - t.Errorf("top level = %v, want %v", topLevel(t, got), want) + gotNames := topLevel(t, got) + if want := []string{"lark-a", "lark-b", "lark-shared"}; !equalStrings(gotNames, want) { + t.Errorf("top level = %v, want %v", gotNames, want)This applies to all 9 test functions using this pattern.
Also applies to: 80-94, 96-106, 108-120, 122-132, 134-148, 153-166, 169-177, 181-190
🤖 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/skillpolicy/resolver_test.go` around lines 69 - 78, Store the result of topLevel(t, got) in a local variable before the equality check, then reuse that variable in the failure message. Apply this change to all nine affected test functions, including TestResolve_NoSpecs_ReturnsBaseUnchanged, to avoid duplicate directory reads and sorting.
263-273: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
slices.EqualhereequalStringsis redundant in Go 1.23; replace the call sites withslices.Equaland drop the 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/skillpolicy/resolver_test.go` around lines 263 - 273, Replace all call sites of equalStrings in resolver_test.go with slices.Equal, adding the slices import if needed, then remove the redundant equalStrings helper function.
🤖 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/skillpolicy/resolver_test.go`:
- Around line 69-78: Store the result of topLevel(t, got) in a local variable
before the equality check, then reuse that variable in the failure message.
Apply this change to all nine affected test functions, including
TestResolve_NoSpecs_ReturnsBaseUnchanged, to avoid duplicate directory reads and
sorting.
- Around line 263-273: Replace all call sites of equalStrings in
resolver_test.go with slices.Equal, adding the slices import if needed, then
remove the redundant equalStrings helper function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e579f44f-b98a-4a34-b87d-0bf9532afc68
📒 Files selected for processing (46)
cmd/build.gocmd/config/config_test.gocmd/config/plugins.gocmd/config/plugins_test.gocmd/config/show.gocmd/flag_gate.gocmd/platform_bootstrap.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/presentation.gocmd/presentation_test.gocmd/prune.gocmd/prune_test.gocmd/root.gocmd/root_risk_help_test.gocmd/service/affordance_test.gocmd/skill_customization_test.goerrs/subtypes.goextension/platform/README.mdextension/platform/builder.goextension/platform/builder_test.goextension/platform/capabilities.goextension/platform/registrar.goextension/platform/rule.goextension/platform/skillsoverlay.gointernal/auth/errors.gointernal/auth/hint_gate_test.gointernal/client/client.gointernal/client/hint_gate_test.gointernal/cmdpolicy/active.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/denial.gointernal/cmdpolicy/diagnostic.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/source_label_test.gointernal/platform/error.gointernal/platform/host.gointernal/platform/host_test.gointernal/platform/inventory.gointernal/platform/inventory_test.gointernal/platform/staging.gointernal/policystate/policystate.gointernal/skillpolicy/overlay.gointernal/skillpolicy/resolver.gointernal/skillpolicy/resolver_test.go
✅ Files skipped from review due to trivial changes (1)
- extension/platform/skillsoverlay.go
🚧 Files skipped from review as they are similar to previous changes (42)
- internal/auth/hint_gate_test.go
- cmd/config/show.go
- internal/platform/error.go
- cmd/prune.go
- cmd/config/plugins.go
- internal/cmdpolicy/source_label_test.go
- internal/client/client.go
- internal/policystate/policystate.go
- cmd/config/plugins_test.go
- extension/platform/capabilities.go
- internal/cmdpolicy/engine.go
- internal/auth/errors.go
- internal/cmdpolicy/active.go
- internal/cmdpolicy/diagnostic.go
- cmd/config/config_test.go
- cmd/root_risk_help_test.go
- internal/platform/inventory_test.go
- internal/platform/host_test.go
- errs/subtypes.go
- internal/platform/staging.go
- internal/client/hint_gate_test.go
- cmd/prune_test.go
- extension/platform/registrar.go
- cmd/skill_customization_test.go
- cmd/platform_guards.go
- internal/platform/host.go
- extension/platform/builder.go
- internal/cmdpolicy/apply.go
- cmd/platform_bootstrap.go
- cmd/service/affordance_test.go
- extension/platform/README.md
- cmd/platform_bootstrap_test.go
- extension/platform/builder_test.go
- internal/cmdpolicy/denial.go
- cmd/presentation_test.go
- internal/platform/inventory.go
- extension/platform/rule.go
- cmd/presentation.go
- internal/skillpolicy/resolver.go
- internal/cmdpolicy/aggregation_test.go
- cmd/build.go
- internal/skillpolicy/overlay.go
evandance
force-pushed
the
feat/plugin-trim
branch
from
965c771 to
547a35a
Compare
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 `@internal/errclass/hint_gate_test.go`:
- Around line 21-27: Remove the st != errs.SubtypeUserUnauthorized exemption
from the condition in the PermissionHint default-build test, so every listed
subtype must produce a non-empty hint containing "auth login"; simplify the
assertion while preserving the existing diagnostic error message.
In `@internal/hook/install.go`:
- Around line 110-114: Add a regression test in install_test.go covering Install
with a command marked by cmdpolicy.AnnotationFrameworkMeta; verify the
framework-meta command bypasses the plugin wrapper chain while preserving the
underlying result.
🪄 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: 887f2f85-ae3c-4ad6-ba7a-94b64a01a5f8
📒 Files selected for processing (52)
cmd/build.gocmd/config/config_test.gocmd/config/plugins.gocmd/config/plugins_test.gocmd/config/show.gocmd/flag_gate.gocmd/platform_bootstrap.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/presentation.gocmd/presentation_test.gocmd/prune.gocmd/prune_test.gocmd/root.gocmd/root_risk_help_test.gocmd/service/affordance_test.gocmd/skill_customization_test.goerrs/subtypes.goextension/platform/README.mdextension/platform/builder.goextension/platform/builder_test.goextension/platform/capabilities.goextension/platform/registrar.goextension/platform/registrar_compat_test.goextension/platform/rule.goextension/platform/skillsoverlay.gointernal/auth/errors.gointernal/auth/hint_gate_test.gointernal/client/client.gointernal/client/hint_gate_test.gointernal/cmdpolicy/active.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/denial.gointernal/cmdpolicy/diagnostic.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/source_label_test.gointernal/core/notconfigured.gointernal/core/notconfigured_hint_gate_test.gointernal/errclass/classify.gointernal/errclass/hint_gate_test.gointernal/hook/install.gointernal/platform/error.gointernal/platform/host.gointernal/platform/host_test.gointernal/platform/inventory.gointernal/platform/inventory_test.gointernal/platform/staging.gointernal/policystate/policystate.gointernal/skillpolicy/overlay.gointernal/skillpolicy/resolver.gointernal/skillpolicy/resolver_test.go
✅ Files skipped from review due to trivial changes (1)
- internal/cmdpolicy/diagnostic.go
🚧 Files skipped from review as they are similar to previous changes (45)
- cmd/root_risk_help_test.go
- extension/platform/skillsoverlay.go
- internal/auth/hint_gate_test.go
- internal/cmdpolicy/denial.go
- cmd/config/show.go
- internal/policystate/policystate.go
- cmd/platform_guards.go
- errs/subtypes.go
- internal/cmdpolicy/engine.go
- cmd/prune.go
- extension/platform/rule.go
- extension/platform/registrar.go
- internal/cmdpolicy/active.go
- internal/auth/errors.go
- cmd/config/plugins.go
- internal/client/hint_gate_test.go
- internal/platform/error.go
- cmd/prune_test.go
- internal/client/client.go
- cmd/platform_bootstrap_test.go
- extension/platform/builder_test.go
- cmd/service/affordance_test.go
- extension/platform/builder.go
- cmd/flag_gate.go
- internal/cmdpolicy/aggregation_test.go
- internal/platform/inventory.go
- internal/cmdpolicy/source_label_test.go
- cmd/config/config_test.go
- cmd/config/plugins_test.go
- internal/platform/staging.go
- cmd/presentation.go
- internal/cmdpolicy/apply.go
- internal/platform/host_test.go
- cmd/platform_bootstrap.go
- extension/platform/README.md
- internal/skillpolicy/overlay.go
- cmd/build.go
- internal/platform/host.go
- extension/platform/capabilities.go
- internal/platform/inventory_test.go
- internal/skillpolicy/resolver_test.go
- cmd/root.go
- internal/skillpolicy/resolver.go
- cmd/presentation_test.go
- cmd/skill_customization_test.go
evandance
force-pushed
the
feat/plugin-trim
branch
8 times, most recently
from
25c5311 to
bc94f30
Compare
Integrators that build their own lark-cli distribution can now ship a package where everything they cut out truly disappears, instead of lingering as half-disabled stubs that mislead users and agents. Commands an integrator plugin restricts no longer show up in help or completion, and invoking one (including asking for its help) answers "command not included in this build" (subtype command_unavailable) with no policy vocabulary and no dead-end recovery steering. Integrators can replace that message with their own product wording via Rule.DeniedMessage. Cutting a whole domain also retires everything that points at it: --profile hides and rejects use when the profile domain goes, the skills-setup footer, update notices, and the auth-login recovery hint stop rendering when their domains go. The policy self-inspection commands (config policy show / plugins show) stay executable by default so operators can still see which rule locked the build, hidden from help but available on request. Fully-managed distributions can retire even those with HideDiagnostics(). Plugins can also trim the embedded skills to match: EmbeddedSkills removes, overlays, or replaces entries over the CLI's built-in set, and skills list/read plus every help pointer serve the same trimmed tree. Users who write their own ~/.lark-cli/policy.yml see no change: their denials stay visible and keep explaining how to adjust the policy they own. Default builds are byte-for-byte unchanged.
evandance
force-pushed
the
feat/plugin-trim
branch
from
bc94f30 to
9cf835d
Compare
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
Integrators building their own lark-cli distribution with
Restrict(rule)now ship builds where restricted capabilities are truly absent. Invoking a restricted command, including asking for its help, answerscommand_unavailable("command not included in this build") with no policy vocabulary and no dead-end recovery steering, and restricted commands are absent from normal help and command-name completion. Cutting a whole domain also retires what points at it:--profilehides and is rejected when the profile domain goes; the skills-setup footer, update notices, and the auth-login recovery hint stop rendering when their domains go. Plugins can additionally trim the embedded skill set. User-authored~/.lark-cli/policy.ymldenials and default builds are unchanged.Changes
validation/command_unavailable(exit 2) on execution,--help, andhelp <cmd>; they no longer appear in normal help or command-name shell completion. yaml-source denials keep the existingcommand_deniedpresentation.--profilehides and is rejected as an unknown flag (same shape as an unregistered flag) when the profile domain is restricted, on every dispatch path.EmbeddedSkills(&SkillsOverlay{Base, Allow, Remove, Overlay})composes the embedded skill tree in fixed order (Base, then Allow, then Remove, then Overlay), with Remove taking precedence over Allow, Overlay exempt from Allow, and fail-fast validation for malformed skill trees or names absent from the base.skills list/skills readand every help pointer serve the same resolved tree.Rule.DeniedMessagereplaces the default message build-wide;HideDiagnostics()retiresconfig policy show/config plugins show;config plugins showreports the new capability summary.Test Plan
--profileflag gating, strict-mode precedence, diagnostics, fixed hints, builder ownership, and plugin inventory rendering.fstest.TestFSconformance.--profilerejection, skill list/read trimming, default-build parity, and unchanged yaml-policy behavior.Related Issues
None