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@4d15abb5473db450b77ce2f924b22d94f202642b🧩 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 75.19% 75.33% +0.14%
==========================================
Files 914 928 +14
Lines 96789 98139 +1350
==========================================
+ Hits 72778 73934 +1156
- Misses 18413 18534 +121
- Partials 5598 5671 +73 ☔ 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
July 9, 2026 09:41
9cba7bd to
419d7df
Compare
evandance
force-pushed
the
feat/plugin-trim
branch
2 times, most recently
from
July 10, 2026 06:23
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
July 10, 2026 06:53
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
11 times, most recently
from
July 11, 2026 12:28
9198dbe to
aeaed08
Compare
evandance
force-pushed
the
feat/plugin-trim
branch
4 times, most recently
from
July 28, 2026 06:36
5199518 to
fce977d
Compare
evandance
force-pushed
the
feat/plugin-trim
branch
2 times, most recently
from
July 28, 2026 11:48
8e33cc0 to
e149df8
Compare
evandance
force-pushed
the
feat/plugin-trim
branch
from
July 29, 2026 04:26
e149df8 to
fac65d4
Compare
evandance
force-pushed
the
feat/plugin-trim
branch
from
July 29, 2026 07:54
fac65d4 to
542673c
Compare
github-actions
Bot
added
domain/calendar
evandance
force-pushed
the
feat/plugin-trim
branch
3 times, most recently
from
July 29, 2026 09:40
d37d1ae to
05d26a5
Compare
evandance
removed
enhancement
evandance
force-pushed
the
feat/plugin-trim
branch
from
July 30, 2026 08:08
05d26a5 to
691e819
Compare
github-actions
Bot
added
domain/calendar
evandance
force-pushed
the
feat/plugin-trim
branch
from
July 30, 2026 08:41
691e819 to
87fe9bd
Compare
Add host-level opt-in concealment for plugin-restricted commands while keeping Restrict enforcement and the established legacy and YAML behavior unchanged. Compose external wrapper-owned embedded skill trees with fail-closed Base, Allow, Remove, Overlay, and structured reference remaps. Retire CLI-owned dead affordances across help, completion, flags, notices, diagnostics, and recovery. Keep presentation and skill state build-local and single-sourced, preserve typed producer errors, and cover source compatibility, multi-build isolation, failure paths, and real external fork integrations.
evandance
force-pushed
the
feat/plugin-trim
branch
from
July 30, 2026 08:53
87fe9bd to
4d15abb
Compare
liuxinyanglxy
approved these changes
Jul 30, 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
3 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
Let integrators ship a reduced lark-cli distribution without exposing plugin-policy details or leaving unusable command, recovery, and skill affordances behind. Existing
Restrictbehavior remains compatible; absence-style presentation is an explicit wrapper-host choice throughExecuteWithOptions.Changes
Execute() intand addExecuteWithOptions(opts ...BuildOption) intwithConcealRestrictedCommands,UnavailableMessage, andHidePolicyDiagnostics.validation/command_unavailable; legacy Restrict and YAML-policy denials retain their existing envelopes.api,schema,calendar, andmailexamples without leaving an empty examples heading; every declared target is contract-tested against the real default command tree.help <child>whenever any ancestor is concealed, closing the Cobra help traversal that could reveal descendants of an unavailable group.identity,missing_scopes,subtype, andconsole_urlfacts. Framework-owned auth outputs and typed-error recovery no longer need distribution knowledge or handwrittenauth loginhints; ordinary business-domain Tips remain outside this migration boundary.auth login,config bind,profile add) outside the structured recovery model.SkillsOverlay{Base, Allow, Remove, Overlay, ReferenceRemaps}. Missing wrapper base content, invalid trees, conflicting owners, dangling remaps, install failures, and plugin-FS panics become controlled build/runtime errors instead of silently restoring host defaults or crashing.SkillRefresolution andplatform.RemapSkillRef. Whole-skill remaps preserve relative paths, exact remaps take precedence, and unresolved references remove their complete help fragment.affordance/docs.md. Domain and shortcut help now use the same generic affordance + skill resolver path, including replacement/removal/concealment.cmd.SetEmbeddedSkillContentowns the runtime skill tree andcmd.SetEmbeddedAffordanceContentowns command guidance. The external-module E2E builds wrappers exactly this way.Compatibility
auth loginonly when that command is referenceable; bot recovery points to app/developer configuration; reduced builds receive a command-independent authorization fallback.skills readsections. The docs commands and exit codes are unchanged.Execute() int, existingCapabilities/Ruleusage, and the public two-argument help preparation functions retain source compatibility.Test Plan
make unit-test(race-enabled supported packages)go test ./tests/plugin_e2e -count=1(real external wrapper modules)go vet ./...gofmt -l .produces no outputgo mod tidyleavesgo.modandgo.sumunchangedgo test -C lint ./errscontractpython3 skills/lark-slides/scripts/xml_text_overlap_lint_test.py(124 tests from rebasedmain)origin/main:0 behind / 1 aheadorigin/main/final-head black-box A/B: representative default help paths are byte-identical, includingconfig init --help; only the declared docs-help presentation differsauth loginrecovery and project it to a target-free fallback only when that command is concealeddrive +search --mine's existing default wireExternal fork E2E covers wrapper asset wiring, Allow/Remove, complete Base replacement, whole-skill and exact-reference remapping, docs pointer rendering/removal, root-help and generated-method schema projection, concealed-parent help rejection, framework-owned and typed-error target-free authorization recovery, retained
profile remove/config initrecovery after concealingprofile add/config bind, concealed skills commands, legacy Restrict reason compatibility, fail-closed installation, and plugin-FS runtime panic containment.Related Issues