diff --git a/cmd/bootstrap_test.go b/cmd/bootstrap_test.go index aa5fd3de7..60bcf2d20 100644 --- a/cmd/bootstrap_test.go +++ b/cmd/bootstrap_test.go @@ -3,7 +3,10 @@ package cmd -import "testing" +import ( + "errors" + "testing" +) func TestBootstrapInvocationContext_ProfileFlag(t *testing.T) { inv, err := BootstrapInvocationContext([]string{"--profile", "target", "auth", "status"}) @@ -70,3 +73,18 @@ func TestBootstrapInvocationContext_HelpWithProfile(t *testing.T) { t.Fatalf("profile = %q, want %q", inv.Profile, "target") } } + +func TestIsDeferredBootstrapProfileError(t *testing.T) { + if !isDeferredBootstrapProfileError(errors.New("flag needs an argument: --profile")) { + t.Fatal("missing --profile value must be deferred to the completed Cobra tree") + } + for _, err := range []error{ + nil, + errors.New("flag needs an argument: --future"), + errors.New("invalid argument for --profile"), + } { + if isDeferredBootstrapProfileError(err) { + t.Fatalf("unexpected deferred bootstrap error: %v", err) + } + } +} diff --git a/cmd/build.go b/cmd/build.go index 55f89304c..bfb130ae6 100644 --- a/cmd/build.go +++ b/cmd/build.go @@ -28,7 +28,10 @@ import ( "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/hook" "github.com/larksuite/cli/internal/keychain" + internalplatform "github.com/larksuite/cli/internal/platform" + "github.com/larksuite/cli/internal/policystate" "github.com/larksuite/cli/internal/registry" + "github.com/larksuite/cli/internal/skillpolicy" "github.com/larksuite/cli/shortcuts" "github.com/spf13/cobra" ) @@ -167,13 +170,20 @@ func buildInternal(ctx context.Context, inv cmdutil.InvocationContext, opts ...B if cfg.streams == nil { cfg.streams = cmdutil.SystemIO() } - // Initialize the registry brand before anything touches the runtime // catalog (its sync.Once would otherwise lock onto the Feishu default). if cfg.startupBrand != "" { registry.InitWithBrand(cfg.startupBrand) } + // Reset every process-global snapshot up front, not only inside + // applyUserPolicyPruning: the skipPlugins and install-failure paths + // return before pruning runs, and a previous build's state must not + // leak into this one (long-lived embedders, test sequences). + policystate.SetPluginDeniedDomains(nil) + cmdpolicy.SetActive(nil) + internalplatform.SetActiveInventory(nil) + f := cmdutil.NewDefault(cfg.streams, inv) if cfg.keychain != nil { f.Keychain = cfg.keychain @@ -195,7 +205,16 @@ func buildInternal(ctx context.Context, inv cmdutil.InvocationContext, opts ...B // rootUsageTemplate. rootCmd.SetUsageTemplate(rootUsageTemplate) - installTipsHelpFunc(rootCmd) + // The skill-content getter also gates on the skills command domain: + // every pointer it feeds renders as `lark-cli skills read ...`, so with + // that domain plugin-denied the pointers would all be dead ends even + // though the content itself still exists. + installTipsHelpFunc(rootCmd, func() fs.FS { + if policystate.DomainDeniedByPlugin("skills") { + return nil + } + return f.SkillContent + }) rootCmd.SilenceErrors = true // SilenceUsage as a static field (not only in PersistentPreRun) so it also // covers flag-parse errors, which fail before PreRun runs — otherwise cobra @@ -243,6 +262,8 @@ func buildInternal(ctx context.Context, inv cmdutil.InvocationContext, opts ...B } if cfg.skipPlugins { + installHelpCommand(rootCmd) + f.SkillContent = embeddedSkillContent recordInventory(nil) return f, rootCmd, nil } @@ -253,23 +274,52 @@ func buildInternal(ctx context.Context, inv cmdutil.InvocationContext, opts ...B return f, rootCmd, nil } var pluginRules []cmdpolicy.PluginRule + var pluginSkills []skillpolicy.PluginSkill var registry *hook.Registry if installResult != nil { pluginRules = installResult.PluginRules + pluginSkills = installResult.PluginSkills registry = installResult.Registry } // Policy errors fail-CLOSED when a plugin contributed (security // intent must not be silently dropped); yaml-only errors fail-OPEN // with a warning so a typo can't lock the user out. - if err := applyUserPolicyPruning(rootCmd, pluginRules); err != nil { + denied, policyErr := applyUserPolicyPruning(rootCmd, pluginRules) + if policyErr != nil { if len(pluginRules) > 0 { - installPluginConflictGuard(rootCmd, err) + installPluginConflictGuard(rootCmd, policyErr) return f, rootCmd, nil } - warnPolicyError(cfg.streams.ErrOut, err) + warnPolicyError(cfg.streams.ErrOut, policyErr) } + // The custom help command attaches AFTER policy evaluation on purpose: + // it is a framework meta command, and inside the evaluated tree an + // allow-list rule (Allow: ["im/**"]) would deny it as + // domain_not_allowed — cobra's stock help command is likewise attached + // only at Execute time and never evaluated. + installHelpCommand(rootCmd) + + // Presentation passes: a capability an integrator plugin denied + // presents as absent — retired global flags (--profile), no skills + // footer, diagnostics hidden or retired. Enforcement stays with + // cmdpolicy.Apply above; these only shape help and fixed hints. + applyPluginPresentation(rootCmd, installResult, denied) + + // Resolve the embedded skill tree BEFORE wiring hooks: an invalid + // SkillsOverlay must fail fast, before wireHooks emits Startup, so a Startup + // side effect is never stranded without its Shutdown. Both skill readers + // -- `skills list`/`read` and the --help guidance -- then read this one + // f.SkillContent. Fails closed: never silently ship defaults once a + // customization is declared. + resolvedSkills, skillErr := skillpolicy.Resolve(embeddedSkillContent, pluginSkills) + if skillErr != nil { + installPluginSkillErrorGuard(rootCmd, skillErr) + return f, rootCmd, nil + } + f.SkillContent = resolvedSkills + if registry != nil { if err := wireHooks(ctx, rootCmd, registry); err != nil { installPluginLifecycleErrorGuard(rootCmd, err) diff --git a/cmd/config/config_test.go b/cmd/config/config_test.go index 9f0f3da62..0690a6e51 100644 --- a/cmd/config/config_test.go +++ b/cmd/config/config_test.go @@ -20,6 +20,7 @@ import ( "github.com/larksuite/cli/internal/i18n" "github.com/larksuite/cli/internal/keychain" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/policystate" ) type noopConfigKeychain struct{} @@ -564,3 +565,54 @@ func TestPrintLangPreferenceConfirmation(t *testing.T) { } }) } + +// The "no active profile" hint points at `lark-cli profile list`; that pointer +// must be gated on the profile domain still being present. When a plugin denies +// the profile domain, the hint would be a dead end, so it is omitted — the error +// itself is unchanged. +func TestConfigShowRun_ProfileHintGatedByPluginDenial(t *testing.T) { + multi := &core.MultiAppConfig{ + CurrentApp: "missing", + Apps: []core.AppConfig{{ + Name: "default", + AppId: "app-default", + AppSecret: core.PlainSecret("secret-default"), + Brand: core.BrandFeishu, + }}, + } + + hintOf := func(t *testing.T) string { + t.Helper() + t.Setenv("LARKSUITE_CLI_CONFIG_DIR", t.TempDir()) + if err := core.SaveMultiAppConfig(multi); err != nil { + t.Fatalf("SaveMultiAppConfig() error = %v", err) + } + f, _, _, _ := cmdutil.TestFactory(t, nil) + err := configShowRun(&ConfigShowOptions{Factory: f}) + var cfgErr *errs.ConfigError + if !errors.As(err, &cfgErr) { + t.Fatalf("expected *errs.ConfigError, got %T %v", err, err) + } + if cfgErr.Subtype != errs.SubtypeNotConfigured { + t.Fatalf("subtype = %q, want not_configured", cfgErr.Subtype) + } + return cfgErr.Hint + } + + t.Run("profile domain present: hint points at profile list", func(t *testing.T) { + policystate.ResetForTesting() + t.Cleanup(policystate.ResetForTesting) + if h := hintOf(t); !strings.Contains(h, "lark-cli profile list") { + t.Errorf("hint = %q, want it to reference `lark-cli profile list`", h) + } + }) + + t.Run("profile domain plugin-denied: hint omitted", func(t *testing.T) { + policystate.ResetForTesting() + t.Cleanup(policystate.ResetForTesting) + policystate.SetPluginDeniedDomains(map[string]bool{"profile": true}) + if h := hintOf(t); h != "" { + t.Errorf("hint = %q, want empty when the profile domain is plugin-denied", h) + } + }) +} diff --git a/cmd/config/plugins.go b/cmd/config/plugins.go index fc1738337..ed98315ca 100644 --- a/cmd/config/plugins.go +++ b/cmd/config/plugins.go @@ -85,6 +85,9 @@ func runConfigPluginsShow(f *cmdutil.Factory) error { if len(p.Rules) > 0 { entry["rules"] = p.Rules } + if p.EmbeddedSkills != nil { + entry["embedded_skills"] = p.EmbeddedSkills + } entry["hooks"] = map[string]any{ "observers": p.Observers, "wrappers": p.Wrappers, diff --git a/cmd/config/plugins_test.go b/cmd/config/plugins_test.go new file mode 100644 index 000000000..e4fed17fb --- /dev/null +++ b/cmd/config/plugins_test.go @@ -0,0 +1,93 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package config + +import ( + "bytes" + "encoding/json" + "testing" + + "github.com/larksuite/cli/internal/cmdutil" + internalplatform "github.com/larksuite/cli/internal/platform" +) + +// config plugins show must surface a plugin's EmbeddedSkills contribution in +// the rendered JSON, not only in the internal inventory struct: this command is +// the operator's window into what a fork trimmed, so the Allow/Remove/Overlay/ +// Base summary has to reach stdout. Guards the render layer, which asserting the +// inventory struct alone does not exercise. +func TestConfigPluginsShow_RendersEmbeddedSkills(t *testing.T) { + internalplatform.SetActiveInventory(&internalplatform.Inventory{ + Plugins: []internalplatform.PluginEntry{{ + Name: "acme", + Version: "1.0", + Capabilities: internalplatform.CapabilitiesView{Restricts: true, FailurePolicy: "fail-closed"}, + EmbeddedSkills: &internalplatform.SkillsOverlayView{ + Allow: []string{"lark-im"}, + Remove: []string{"lark-a"}, + Overlay: true, + Base: true, + }, + }}, + }) + t.Cleanup(func() { internalplatform.SetActiveInventory(nil) }) + + out := &bytes.Buffer{} + f := &cmdutil.Factory{IOStreams: cmdutil.NewIOStreams(nil, out, &bytes.Buffer{})} + if err := runConfigPluginsShow(f); err != nil { + t.Fatalf("show: %v", err) + } + + var got struct { + Plugins []struct { + EmbeddedSkills *internalplatform.SkillsOverlayView `json:"embedded_skills"` + } `json:"plugins"` + } + if err := json.Unmarshal(out.Bytes(), &got); err != nil { + t.Fatalf("not json: %v\n%s", err, out.String()) + } + if len(got.Plugins) != 1 { + t.Fatalf("want 1 plugin, got %d", len(got.Plugins)) + } + es := got.Plugins[0].EmbeddedSkills + if es == nil { + t.Fatalf("embedded_skills missing from rendered output:\n%s", out.String()) + } + if len(es.Allow) != 1 || es.Allow[0] != "lark-im" || + len(es.Remove) != 1 || es.Remove[0] != "lark-a" || + !es.Overlay || !es.Base { + t.Errorf("embedded_skills summary mismatch: %+v", es) + } +} + +// A plugin that did not customize embedded skills must not emit an +// embedded_skills key, so the field's presence is a reliable signal that a fork +// trimmed the tree. +func TestConfigPluginsShow_OmitsEmbeddedSkillsWhenAbsent(t *testing.T) { + internalplatform.SetActiveInventory(&internalplatform.Inventory{ + Plugins: []internalplatform.PluginEntry{{ + Name: "acme", + Version: "1.0", + Capabilities: internalplatform.CapabilitiesView{Restricts: true, FailurePolicy: "fail-closed"}, + }}, + }) + t.Cleanup(func() { internalplatform.SetActiveInventory(nil) }) + + out := &bytes.Buffer{} + f := &cmdutil.Factory{IOStreams: cmdutil.NewIOStreams(nil, out, &bytes.Buffer{})} + if err := runConfigPluginsShow(f); err != nil { + t.Fatalf("show: %v", err) + } + var raw map[string]any + if err := json.Unmarshal(out.Bytes(), &raw); err != nil { + t.Fatalf("not json: %v", err) + } + plugins, ok := raw["plugins"].([]any) + if !ok || len(plugins) != 1 { + t.Fatalf("want 1 plugin in output, got: %s", out.String()) + } + if _, ok := plugins[0].(map[string]any)["embedded_skills"]; ok { + t.Errorf("embedded_skills must be omitted when the plugin customized no skills; got:\n%s", out.String()) + } +} diff --git a/cmd/config/show.go b/cmd/config/show.go index 5526f0254..b85292a88 100644 --- a/cmd/config/show.go +++ b/cmd/config/show.go @@ -13,6 +13,7 @@ import ( "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/policystate" "github.com/spf13/cobra" ) @@ -55,7 +56,12 @@ func configShowRun(opts *ConfigShowOptions) error { } app := config.CurrentAppConfig(f.Invocation.Profile) if app == nil { - return errs.NewConfigError(errs.SubtypeNotConfigured, "no active profile").WithHint("run: lark-cli profile list") + e := errs.NewConfigError(errs.SubtypeNotConfigured, "no active profile") + // No recovery hint when the profile domain is absent from this build. + if !policystate.DomainDeniedByPlugin("profile") { + e = e.WithHint("run: lark-cli profile list") + } + return e } users := "(no logged-in users)" if len(app.Users) > 0 { diff --git a/cmd/flag_gate.go b/cmd/flag_gate.go new file mode 100644 index 000000000..7d67f5de4 --- /dev/null +++ b/cmd/flag_gate.go @@ -0,0 +1,67 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmd + +import ( + "errors" + + "github.com/spf13/cobra" + "github.com/spf13/pflag" + + "github.com/larksuite/cli/internal/cmdpolicy" +) + +// globalFlagDomains maps each root persistent flag to the command domain +// it belongs to. A new domain-tied global flag must add a row. +var globalFlagDomains = map[string]string{ + "profile": "profile", +} + +// flagGateAnnotation distinguishes a policy-retired flag from one hidden +// cosmetically (single-app mode force-shows the latter in root help). +const flagGateAnnotation = "lark:policy_denied_flag" + +// applyPluginFlagGate hides and rejects the global flags whose whole +// domain a plugin denied. yaml denials do not gate flags. Must run after +// RegisterGlobalFlags and cmdpolicy.Apply. +func applyPluginFlagGate(root *cobra.Command, denied map[string]cmdpolicy.Denial) { + for flagName, domain := range globalFlagDomains { + d, ok := denied[domain] + if !ok || !cmdpolicy.IsPluginPolicySource(d.PolicySource) { + continue + } + fl := root.PersistentFlags().Lookup(flagName) + if fl == nil { + continue + } + fl.Hidden = true + if fl.Annotations == nil { + fl.Annotations = map[string][]string{} + } + fl.Annotations[flagGateAnnotation] = []string{"true"} + fl.Value = &gatedFlagValue{name: flagName, inner: fl.Value} + } +} + +func isPolicyGatedFlag(fl *pflag.Flag) bool { + return fl != nil && fl.Annotations[flagGateAnnotation] != nil +} + +// gatedFlagValue rejects at parse time, before cobra's help/version fast +// paths (which never reach PersistentPreRunE). Its Set error carries +// cobra's own unknown-flag wording so the root FlagErrorFunc renders the +// same envelope an unregistered flag produces. +type gatedFlagValue struct { + name string + inner pflag.Value +} + +func (g *gatedFlagValue) String() string { return g.inner.String() } +func (g *gatedFlagValue) Type() string { return g.inner.Type() } +func (g *gatedFlagValue) Set(string) error { + // Intermediate parse error, not a final envelope: pflag wraps it and + // the root FlagErrorFunc (flagDidYouMean) converts it to the typed + // unknown-flag validation error. + return errors.New("unknown flag: --" + g.name) //nolint:forbidigo // intermediate parse error; flagDidYouMean emits the typed envelope +} diff --git a/cmd/platform_bootstrap.go b/cmd/platform_bootstrap.go index 9b1b57ac3..51d23fe39 100644 --- a/cmd/platform_bootstrap.go +++ b/cmd/platform_bootstrap.go @@ -17,6 +17,7 @@ import ( "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/hook" internalplatform "github.com/larksuite/cli/internal/platform" + "github.com/larksuite/cli/internal/policystate" "github.com/larksuite/cli/internal/vfs" ) @@ -35,7 +36,15 @@ const userPolicyFileName = "policy.yml" // // pluginRules carries Plugin.Restrict() contributions collected from // the InstallAll phase; nil/empty is fine. -func applyUserPolicyPruning(rootCmd *cobra.Command, pluginRules []cmdpolicy.PluginRule) error { +// +// The returned denied map (nil when no rule denied anything) feeds the +// post-pruning presentation passes in build.go: the global-flag gate and +// the fixed-hint filter both key off which domains a plugin denied. +func applyUserPolicyPruning(rootCmd *cobra.Command, pluginRules []cmdpolicy.PluginRule) (map[string]cmdpolicy.Denial, error) { + // Reset up front so every early return leaves the process-global + // snapshot clean; the success path re-populates it. + policystate.SetPluginDeniedDomains(nil) + // Plugin rules shadow the yaml source entirely (Resolve: plugin > // yaml). When a plugin contributed rules we therefore do NOT even // read ~/.lark-cli/policy.yml: build.go fail-CLOSES on any policy @@ -65,7 +74,7 @@ func applyUserPolicyPruning(rootCmd *cobra.Command, pluginRules []cmdpolicy.Plug // show` reports "no policy" instead of a stale rule that // doesn't reflect the current command tree. cmdpolicy.SetActive(nil) - return lerr + return nil, lerr } yamlRules = loaded } @@ -77,11 +86,11 @@ func applyUserPolicyPruning(rootCmd *cobra.Command, pluginRules []cmdpolicy.Plug }) if err != nil { cmdpolicy.SetActive(nil) - return err + return nil, err } if len(rules) == 0 { cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{Source: source}) - return nil + return nil, nil } // RuleName attributes a denial to a specific rule in the envelope. @@ -94,17 +103,39 @@ func applyUserPolicyPruning(rootCmd *cobra.Command, pluginRules []cmdpolicy.Plug ruleName = rules[0].Name } + // DeniedMessage is build-level: the first non-empty message across + // the single owner's rules speaks for all of them. + deniedMessage := "" + for _, r := range rules { + if r.DeniedMessage != "" { + deniedMessage = r.DeniedMessage + break + } + } + engine := cmdpolicy.NewSet(rules) decisions := engine.EvaluateAll(rootCmd) - denied := cmdpolicy.BuildDeniedByPath(rootCmd, decisions, source, ruleName) + denied := cmdpolicy.BuildDeniedByPath(rootCmd, decisions, source, ruleName, deniedMessage) cmdpolicy.Apply(rootCmd, denied) cmdpolicy.SetActive(&cmdpolicy.ActivePolicy{ - Rules: rules, - Source: source, - DeniedPaths: len(denied), + Rules: rules, + Source: source, + DeniedPaths: len(denied), + DeniedByPath: denied, }) - return nil + + // Whole-domain denials surface as aggregate entries keyed by the + // bare domain name (no slash); record them for render-time hint + // emitters (internal/auth, internal/client, the notice provider). + pluginDomains := map[string]bool{} + for path, d := range denied { + if !strings.Contains(path, "/") && cmdpolicy.IsPluginPolicySource(d.PolicySource) { + pluginDomains[path] = true + } + } + policystate.SetPluginDeniedDomains(pluginDomains) + return denied, nil } // installPluginsAndHooks runs the InstallAll phase on the globally- @@ -156,7 +187,22 @@ func recordInventory(installResult *internalplatform.InstallResult) { AllowUnannotated: r.Rule.AllowUnannotated, }) } - internalplatform.SetActiveInventory(internalplatform.BuildInventory(pluginSrcs, installResult.Registry, ruleSrcs)) + skillSrcs := make([]internalplatform.SkillsInventorySource, 0, len(installResult.PluginSkills)) + for _, ps := range installResult.PluginSkills { + if ps.SkillsOverlay == nil { + continue + } + skillSrcs = append(skillSrcs, internalplatform.SkillsInventorySource{ + PluginName: ps.PluginName, + View: internalplatform.SkillsOverlayView{ + Allow: ps.SkillsOverlay.Allow, + Remove: ps.SkillsOverlay.Remove, + Overlay: ps.SkillsOverlay.Overlay != nil, + Base: ps.SkillsOverlay.Base != nil, + }, + }) + } + internalplatform.SetActiveInventory(internalplatform.BuildInventory(pluginSrcs, installResult.Registry, ruleSrcs, skillSrcs)) } // wireHooks installs Observer/Wrapper hooks onto every runnable command diff --git a/cmd/platform_bootstrap_test.go b/cmd/platform_bootstrap_test.go index f033d3fb5..7e4f9e477 100644 --- a/cmd/platform_bootstrap_test.go +++ b/cmd/platform_bootstrap_test.go @@ -116,7 +116,7 @@ max_risk: write `) root := fakeTree(t) - if err := applyUserPolicyPruning(root, nil); err != nil { + if _, err := applyUserPolicyPruning(root, nil); err != nil { t.Fatalf("apply policy: %v", err) } @@ -175,7 +175,7 @@ func TestApplyUserPolicyPruning_missingFileIsSilent(t *testing.T) { tmpHome(t) // home set but no policy.yml written root := fakeTree(t) - if err := applyUserPolicyPruning(root, nil); err != nil { + if _, err := applyUserPolicyPruning(root, nil); err != nil { t.Fatalf("missing policy should not error, got %v", err) } @@ -196,7 +196,7 @@ func TestApplyUserPolicyPruning_malformedYamlReturnsError(t *testing.T) { writePolicy(t, cfgDir, "::: not yaml :::") root := fakeTree(t) - err := applyUserPolicyPruning(root, nil) + _, err := applyUserPolicyPruning(root, nil) if err == nil { t.Fatalf("malformed yaml should produce an error") } @@ -221,7 +221,7 @@ func TestApplyUserPolicyPruning_pluginRulesSkipBrokenYaml(t *testing.T) { }}, } root := fakeTree(t) - if err := applyUserPolicyPruning(root, pluginRules); err != nil { + if _, err := applyUserPolicyPruning(root, pluginRules); err != nil { t.Fatalf("plugin rules must shadow (and skip reading) yaml; broken yaml should not error, got %v", err) } @@ -243,7 +243,7 @@ func TestApplyUserPolicyPruning_invalidRuleReturnsError(t *testing.T) { writePolicy(t, cfgDir, "max_risk: nukem\n") root := fakeTree(t) - err := applyUserPolicyPruning(root, nil) + _, err := applyUserPolicyPruning(root, nil) if err == nil { t.Fatalf("invalid MaxRisk should produce an error") } diff --git a/cmd/platform_guards.go b/cmd/platform_guards.go index a7c99e60d..0a398b204 100644 --- a/cmd/platform_guards.go +++ b/cmd/platform_guards.go @@ -12,6 +12,7 @@ import ( "github.com/larksuite/cli/internal/cmdpolicy" "github.com/larksuite/cli/internal/hook" internalplatform "github.com/larksuite/cli/internal/platform" + "github.com/larksuite/cli/internal/skillpolicy" ) // installFatalGuard wires a fail-closed guard at every cobra dispatch @@ -110,6 +111,27 @@ func installPluginConflictGuard(rootCmd *cobra.Command, err error) { installFatalGuard(rootCmd, makeErr) } +// installPluginSkillErrorGuard surfaces a plugin SkillsOverlay configuration +// error before any command runs. Two failure modes, split by reason code: +// +// - "invalid_skills_overlay" - a Remove/Overlay that cannot compose +// - "multiple_skills_overlay_plugins" - two plugins each customizing skills +// +// The CLI must NOT silently fall back to default skills once an +// integrator has declared a customization. +func installPluginSkillErrorGuard(rootCmd *cobra.Command, err error) { + makeErr := func() error { + reasonCode := internalplatform.ReasonInvalidSkillsOverlay + if errors.Is(err, skillpolicy.ErrMultipleSkillsOverlays) { + reasonCode = internalplatform.ReasonMultipleSkillsOverlays + } + return errs.NewValidationError(errs.SubtypeFailedPrecondition, "%s", err.Error()). + WithHint("skill customization is broken (reason_code %s); fix the plugin's EmbeddedSkills configuration or remove the conflicting plugin", reasonCode). + WithCause(err) + } + installFatalGuard(rootCmd, makeErr) +} + // installPluginLifecycleErrorGuard surfaces a Startup lifecycle handler // failure as a typed validation error (failed_precondition). The hint's // reason code splits returned-error vs panic so consumers (audit / diff --git a/cmd/presentation.go b/cmd/presentation.go new file mode 100644 index 000000000..0d13ccaf3 --- /dev/null +++ b/cmd/presentation.go @@ -0,0 +1,199 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmd + +import ( + "strings" + + "github.com/spf13/cobra" + + "github.com/larksuite/cli/internal/cmdpolicy" + internalplatform "github.com/larksuite/cli/internal/platform" + "github.com/larksuite/cli/internal/policystate" +) + +// applyPluginPresentation makes plugin-denied capabilities present as +// absent: retired flags, no skills footer, diagnostics hidden or retired. +// Presentation only — enforcement happened in cmdpolicy.Apply. +func applyPluginPresentation(rootCmd *cobra.Command, installResult *internalplatform.InstallResult, denied map[string]cmdpolicy.Denial) { + applyPluginFlagGate(rootCmd, denied) + + if domainDeniedByPlugin(denied, "skills") { + rootCmd.SetUsageTemplate(strings.Replace(rootUsageTemplate, skillsSetupFooter, "", 1)) + } + + if installResult != nil && hideDiagnosticsOwner(installResult.Plugins) != "" { + retireDiagnostics(rootCmd, installResult, denied) + } + // Group convergence runs on both paths: with the exemptions retired + // (or merely concealed), nothing keeps their domain group alive, and + // it must present as absent like any fully-denied domain. + concealDiagnostics(rootCmd, denied) +} + +// domainDeniedByPlugin reads the freshly-built denied map, unlike +// policystate.DomainDeniedByPlugin which serves render-time consumers. +func domainDeniedByPlugin(denied map[string]cmdpolicy.Denial, domain string) bool { + d, ok := denied[domain] + return ok && cmdpolicy.IsPluginPolicySource(d.PolicySource) +} + +// hideDiagnosticsOwner returns the plugin that declared HideDiagnostics, +// or "". The host already enforced that it also Restricts. +func hideDiagnosticsOwner(plugins []internalplatform.PluginInfo) string { + for _, p := range plugins { + if p.Capabilities.HideDiagnostics { + return p.Name + } + } + return "" +} + +// retireDiagnostics installs the unavailable presentation on the +// diagnostic exemptions, same as any other plugin-denied command. +func retireDiagnostics(rootCmd *cobra.Command, installResult *internalplatform.InstallResult, denied map[string]cmdpolicy.Denial) { + source := "plugin:" + hideDiagnosticsOwner(installResult.Plugins) + message := "" + for _, d := range denied { + if cmdpolicy.IsPluginPolicySource(d.PolicySource) && d.DeniedMessage != "" { + message = d.DeniedMessage + break + } + } + // A rule can declare DeniedMessage yet deny no command of its own; + // the synthesized diagnostics denial still speaks in its voice. + if message == "" { + if ap := cmdpolicy.GetActive(); ap != nil && ap.Source.Kind == cmdpolicy.SourcePlugin { + for _, r := range ap.Rules { + if r.DeniedMessage != "" { + message = r.DeniedMessage + break + } + } + } + } + diag := map[string]cmdpolicy.Denial{} + for _, path := range cmdpolicy.DiagnosticPaths() { + // The whole exemption chain retires: the leaf and every group + // between it and the top-level domain answer unavailable, not + // bare help at exit 0. + for p := path; strings.Contains(p, "/"); p = p[:strings.LastIndex(p, "/")] { + diag[p] = cmdpolicy.Denial{ + Layer: cmdpolicy.LayerPolicy, + PolicySource: source, + ReasonCode: "diagnostics_hidden", + Reason: "policy self-inspection hidden by the integrator", + DeniedMessage: message, + } + } + } + cmdpolicy.Apply(rootCmd, diag) + cmdpolicy.AppendActiveDenials(diag) +} + +// concealDiagnostics hides the diagnostic exemptions from help — without +// touching their RunE, so they stay dispatchable — once every non-exempt +// sibling in their domain is already hidden. The group itself then gets +// the unavailable stub: it escaped denial aggregation only because the +// exemptions kept a runnable descendant alive, and its unknown-subcommand +// guard RunE would otherwise keep it listed in help. +func concealDiagnostics(rootCmd *cobra.Command, denied map[string]cmdpolicy.Denial) { + for _, group := range diagnosticDomainGroups(rootCmd) { + if !pluginDeniedUnder(denied, cmdpolicy.CanonicalPath(group)) { + continue + } + exemptAncestors := map[*cobra.Command]bool{} + for _, path := range cmdpolicy.DiagnosticPaths() { + for c := findByPath(rootCmd, path); c != nil && c != group; c = c.Parent() { + exemptAncestors[c] = true + } + } + allOthersHidden := true + for _, child := range group.Commands() { + if child.Name() == "help" || exemptAncestors[child] { + continue + } + if !child.Hidden { + allOthersHidden = false + break + } + } + if !allOthersHidden { + continue + } + for c := range exemptAncestors { + c.Hidden = true + } + var sample cmdpolicy.Denial + for path, d := range denied { + if strings.HasPrefix(path, cmdpolicy.CanonicalPath(group)+"/") && cmdpolicy.IsPluginPolicySource(d.PolicySource) { + sample = d + break + } + } + groupDenial := cmdpolicy.Denial{ + Layer: cmdpolicy.LayerPolicy, + PolicySource: sample.PolicySource, + RuleName: sample.RuleName, + ReasonCode: "all_children_denied", + Reason: "all child commands are denied", + DeniedMessage: sample.DeniedMessage, + } + cmdpolicy.Apply(rootCmd, map[string]cmdpolicy.Denial{ + cmdpolicy.CanonicalPath(group): groupDenial, + }) + // The bootstrap snapshot could not see this whole-domain denial + // (the exemptions were still alive then); record it now so + // render-time hints and policy introspection stop pointing into + // the converged domain. + policystate.AddPluginDeniedDomain(cmdpolicy.CanonicalPath(group)) + cmdpolicy.AppendActiveDenials(map[string]cmdpolicy.Denial{ + cmdpolicy.CanonicalPath(group): groupDenial, + }) + } +} + +// diagnosticDomainGroups returns the top-level groups containing a +// diagnostic exemption (today just `config`). +func diagnosticDomainGroups(rootCmd *cobra.Command) []*cobra.Command { + seen := map[*cobra.Command]bool{} + var out []*cobra.Command + for _, path := range cmdpolicy.DiagnosticPaths() { + top, _, _ := strings.Cut(path, "/") + if c := findByPath(rootCmd, top); c != nil && !seen[c] { + seen[c] = true + out = append(out, c) + } + } + return out +} + +func pluginDeniedUnder(denied map[string]cmdpolicy.Denial, prefix string) bool { + for path, d := range denied { + if strings.HasPrefix(path, prefix+"/") && cmdpolicy.IsPluginPolicySource(d.PolicySource) { + return true + } + } + return false +} + +// findByPath resolves a canonical slash path (e.g. "config/policy/show") +// to the command node, or nil. +func findByPath(rootCmd *cobra.Command, path string) *cobra.Command { + cur := rootCmd + for _, seg := range strings.Split(path, "/") { + var next *cobra.Command + for _, child := range cur.Commands() { + if child.Name() == seg { + next = child + break + } + } + if next == nil { + return nil + } + cur = next + } + return cur +} diff --git a/cmd/presentation_test.go b/cmd/presentation_test.go new file mode 100644 index 000000000..acb64c89f --- /dev/null +++ b/cmd/presentation_test.go @@ -0,0 +1,892 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmd + +import ( + "bytes" + "context" + "errors" + "os" + "strings" + "testing" + + "github.com/spf13/cobra" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/extension/platform" + "github.com/larksuite/cli/internal/cmdpolicy" + "github.com/larksuite/cli/internal/core" + internalplatform "github.com/larksuite/cli/internal/platform" + "github.com/larksuite/cli/internal/policystate" + "github.com/larksuite/cli/internal/update" +) + +// restrictingPlugin registers a plugin that denies the given globs; extra +// customizes the builder further (nil for none). +func restrictingPlugin(t *testing.T, deny []string, extra func(*platform.Builder) *platform.Builder) { + t.Helper() + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + t.Cleanup(func() { policystate.ResetForTesting() }) + b := platform.NewPlugin("acme", "1.0").Restrict(&platform.Rule{Deny: deny}) + if extra != nil { + b = extra(b) + } + platform.Register(b.MustBuild()) +} + +// runnableUnder returns the first runnable non-exempt descendant under +// the named top-level group of the real command tree (the diagnostic +// exemptions keep their original RunE and would not exercise the stub). +func runnableUnder(t *testing.T, root *cobra.Command, group string) *cobra.Command { + t.Helper() + g := findByPath(root, group) + if g == nil { + t.Fatalf("group %q not found in command tree", group) + } + var find func(c *cobra.Command) *cobra.Command + find = func(c *cobra.Command) *cobra.Command { + if c.RunE != nil && len(c.Commands()) == 0 && !cmdpolicy.IsDiagnosticPath(cmdpolicy.CanonicalPath(c)) { + return c + } + for _, child := range c.Commands() { + if leaf := find(child); leaf != nil { + return leaf + } + } + return nil + } + leaf := find(g) + if leaf == nil { + t.Fatalf("no runnable non-exempt leaf under %q", group) + } + return leaf +} + +// A plugin-denied command answers with command_unavailable: default +// message, no hint, no policy vocabulary. +func TestBuildInternal_pluginDenyPresentsUnavailable(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, nil) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + leaf := runnableUnder(t, root, "config") + + err := leaf.RunE(leaf, nil) + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("expected *errs.ValidationError, got %T %v", err, err) + } + if ve.Subtype != errs.SubtypeCommandUnavailable { + t.Errorf("subtype = %q, want command_unavailable", ve.Subtype) + } + if ve.Message != cmdpolicy.DefaultUnavailableMessage || ve.Hint != "" { + t.Errorf("message=%q hint=%q, want default message and empty hint", ve.Message, ve.Hint) + } +} + +// Rule.DeniedMessage speaks in the integrator's product voice. +func TestBuildInternal_deniedMessageOverride(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + t.Cleanup(func() { policystate.ResetForTesting() }) + platform.Register(platform.NewPlugin("acme", "1.0"). + Restrict(&platform.Rule{Deny: []string{"config/**"}, DeniedMessage: "not part of acme cli"}). + MustBuild()) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + leaf := runnableUnder(t, root, "config") + + err := leaf.RunE(leaf, nil) + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("expected *errs.ValidationError, got %T", err) + } + if ve.Message != "not part of acme cli" { + t.Errorf("message = %q, want the integrator's DeniedMessage", ve.Message) + } +} + +// Explicit help on a plugin-denied command must not render the original +// usage; it answers with the same unavailable envelope. +func TestBuildInternal_pluginDenyHelpIntercepted(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, nil) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + leaf := runnableUnder(t, root, "config") + + var buf bytes.Buffer + leaf.SetOut(&buf) + leaf.SetErr(&buf) + root.HelpFunc()(leaf, nil) + out := buf.String() + if !strings.Contains(out, "command_unavailable") { + t.Errorf("explicit help must answer command_unavailable, got:\n%s", out) + } + if strings.Contains(out, "Usage:") { + t.Errorf("explicit help must not render the original usage, got:\n%s", out) + } + + // yaml-source presentation is untouched: a command outside the denied + // domain still renders normal help. + var normal bytes.Buffer + alive := findByPath(root, "skills") + alive.SetOut(&normal) + alive.SetErr(&normal) + root.HelpFunc()(alive, nil) + if strings.Contains(normal.String(), "command_unavailable") { + t.Errorf("non-denied command help must render normally, got:\n%s", normal.String()) + } +} + +// `lark-cli help ` must fail with the typed +// unavailable error (exit non-zero), not print an envelope and exit 0. +func TestBuildInternal_helpCommandReturnsUnavailable(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, nil) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + helpCmd := findByPath(root, "help") + if helpCmd == nil || helpCmd.RunE == nil { + t.Fatal("custom help command not installed") + } + + err := helpCmd.RunE(helpCmd, []string{"config"}) + var ve *errs.ValidationError + if !errors.As(err, &ve) || ve.Subtype != errs.SubtypeCommandUnavailable { + t.Errorf("help on a restricted command must return command_unavailable, got %v", err) + } + + // A live target renders normally and returns nil. + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + if err := helpCmd.RunE(helpCmd, []string{"skills"}); err != nil { + t.Errorf("help on a live command must succeed, got %v", err) + } +} + +// help is a framework meta command: an allow-list rule must not deny it. +// `help ` renders; `help ` returns unavailable. +func TestBuildInternal_helpSurvivesAllowList(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + t.Cleanup(func() { policystate.ResetForTesting() }) + platform.Register(platform.NewPlugin("acme", "1.0"). + Restrict(&platform.Rule{Allow: []string{"im/**"}, AllowUnannotated: true}). + MustBuild()) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + helpCmd := findByPath(root, "help") + if helpCmd == nil || helpCmd.Annotations[cmdpolicy.AnnotationDenialLayer] != "" { + t.Fatalf("help must not be policy-denied under an allow-list; annotations=%v", helpCmd.Annotations) + } + + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + if err := helpCmd.RunE(helpCmd, []string{"im"}); err != nil { + t.Errorf("help on an allowed domain must render, got %v", err) + } + + err := helpCmd.RunE(helpCmd, []string{"config"}) + var ve *errs.ValidationError + if !errors.As(err, &ve) || ve.Subtype != errs.SubtypeCommandUnavailable { + t.Errorf("help on a denied domain must return command_unavailable, got %v", err) + } +} + +// The plugin inventory surfaces the new contributions so `config plugins +// show` can answer "what did this build customize". +func TestBuildInternal_inventoryCoversNewCapabilities(t *testing.T) { + tmpHome(t) + withBaseSkills(t, map[string]string{"lark-a/SKILL.md": "---\ndescription: a\n---\n"}) + restrictingPlugin(t, []string{"profile/**"}, func(b *platform.Builder) *platform.Builder { + return b.HideDiagnostics(). + EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-a"}}) + }) + + buildInternal(context.Background(), buildInvocationForTest(t)) + inv := internalplatform.GetActiveInventory() + if inv == nil || len(inv.Plugins) != 1 { + t.Fatalf("inventory = %+v, want 1 plugin", inv) + } + p := inv.Plugins[0] + if !p.Capabilities.HideDiagnostics { + t.Error("inventory must surface HideDiagnostics") + } + if p.EmbeddedSkills == nil || len(p.EmbeddedSkills.Remove) != 1 || p.EmbeddedSkills.Remove[0] != "lark-a" { + t.Errorf("inventory must summarise EmbeddedSkills, got %+v", p.EmbeddedSkills) + } +} + +// Denying the whole profile domain retires --profile: hidden from help, +// and setting it fails like an unknown flag. +func TestBuildInternal_profileFlagGate(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"profile", "profile/**"}, nil) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + fl := root.PersistentFlags().Lookup("profile") + if fl == nil { + t.Fatal("--profile not registered") + } + if !fl.Hidden || !isPolicyGatedFlag(fl) { + t.Errorf("flag should be hidden and policy-gated; hidden=%v gated=%v", fl.Hidden, isPolicyGatedFlag(fl)) + } + + // The gate rejects at the Value level: cobra's parse (help/version + // fast paths included) can never set a gated flag. + err := root.PersistentFlags().Set("profile", "prod") + if err == nil || !strings.Contains(err.Error(), "unknown flag: --profile") { + t.Errorf("setting a gated flag must fail as unknown flag at parse time, got %v", err) + } +} + +// The gate must also fire on the real dispatch path. cobra parses a persistent +// flag on the leaf command's merged flagset, so a gate that walks +// root.PersistentFlags()'s changed table is a no-op once the flag is passed to +// a subcommand. Drive root.Execute end to end and confirm the gated --profile +// is rejected before the command body runs. +func TestBuildInternal_profileFlagGate_RejectsOnDispatch(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"profile", "profile/**"}, nil) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + ranBody := false + root.AddCommand(&cobra.Command{ + Use: "gateprobe", + RunE: func(*cobra.Command, []string) error { ranBody = true; return nil }, + }) + + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"--profile", "prod", "gateprobe"}) + + err := root.Execute() + var ve *errs.ValidationError + if !errors.As(err, &ve) || !strings.Contains(ve.Message, "unknown flag") { + t.Errorf("a gated --profile passed on the dispatch path must fail as unknown flag, got %v", err) + } + if ranBody { + t.Error("gated --profile must be rejected before the command body runs") + } +} + +// Without the profile domain denied, the gate stays inert. +func TestBuildInternal_profileFlagUntouchedWithoutDenial(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, nil) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + if fl := root.PersistentFlags().Lookup("profile"); isPolicyGatedFlag(fl) { + t.Error("--profile must not be gated when its domain is not denied") + } +} + +// Denying the skills domain drops the root-help skills-setup footer. +func TestBuildInternal_skillsFooterSuppressed(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"skills", "skills/**"}, nil) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + if strings.Contains(root.UsageTemplate(), "Skills setup") { + t.Error("skills-setup footer must be dropped when the skills domain is denied") + } + + // Control: without the denial the footer stays. + platform.ResetForTesting() + policystate.ResetForTesting() + _, root2, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + if !strings.Contains(root2.UsageTemplate(), "Skills setup") { + t.Error("skills-setup footer must stay in the default build") + } +} + +// Denying the skills command domain kills every `skills read` pointer in +// domain help, even when the skill content itself is still embedded — the +// command the pointers name is absent, so they would all be dead ends. +func TestBuildInternal_skillsDomainDenyKillsHelpPointers(t *testing.T) { + tmpHome(t) + withBaseSkills(t, map[string]string{"lark-im/SKILL.md": "---\ndescription: im\n---\n"}) + restrictingPlugin(t, []string{"skills", "skills/**"}, nil) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + im := findByPath(root, "im") + if im == nil { + t.Fatal("im domain not in tree") + } + var buf bytes.Buffer + im.SetOut(&buf) + im.SetErr(&buf) + root.HelpFunc()(im, nil) + if strings.Contains(buf.String(), "skills read") { + t.Errorf("domain help must not point at the denied skills command, got:\n%s", buf.String()) + } +} + +// Denying the update domain silences the _notice providers that would +// steer the caller to `lark-cli update`. +func TestComposePendingNotice_updateDomainDenied(t *testing.T) { + update.SetPending(&update.UpdateInfo{Current: "1.0.0", Latest: "1.0.1"}) + t.Cleanup(func() { update.SetPending(nil) }) + t.Cleanup(policystate.ResetForTesting) + + policystate.SetPluginDeniedDomains(map[string]bool{"update": true}) + if got := composePendingNotice(); got != nil { + t.Errorf("notices must be silenced with the update domain denied, got %+v", got) + } + + policystate.SetPluginDeniedDomains(nil) + if got := composePendingNotice(); got == nil { + t.Error("notices must render without the denial") + } +} + +// HideDiagnostics retires the exemptions — and with them gone, nothing keeps +// the config group alive: it must converge exactly like any fully-denied +// domain (absent from help, bare invocation and explicit help both answer +// command_unavailable). Guards the group level, which the leaf-only test +// below does not. +func TestBuildInternal_hideDiagnosticsConvergesDomainGroup(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, func(b *platform.Builder) *platform.Builder { + return b.HideDiagnostics() + }) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + cfg := findByPath(root, "config") + if cfg == nil { + t.Fatal("config group not in tree") + } + if cfg.IsAvailableCommand() { + t.Error("config group must leave help/completion when HideDiagnostics retires its last live descendants") + } + + if cfg.RunE == nil { + t.Fatal("converged config group must carry an unavailable stub RunE") + } + err := cfg.RunE(cfg, nil) + var ve *errs.ValidationError + if !errors.As(err, &ve) || ve.Subtype != errs.SubtypeCommandUnavailable { + t.Errorf("bare `config` must answer command_unavailable, got %v", err) + } + + var buf bytes.Buffer + cfg.SetOut(&buf) + cfg.SetErr(&buf) + root.HelpFunc()(cfg, nil) + out := buf.String() + if !strings.Contains(out, "command_unavailable") { + t.Errorf("`config --help` must answer command_unavailable, got:\n%.200s", out) + } + if strings.Contains(out, "Usage:") { + t.Errorf("`config --help` must not render the original usage, got:\n%.200s", out) + } + + // The converged domain must also reach the render-time snapshot, so + // fixed hints (e.g. the strict-mode stub's `config strict-mode` + // pointer) stop pointing at a domain this build presents as absent. + if !policystate.DomainDeniedByPlugin("config") { + t.Error("converged config domain must be recorded for render-time hint gating") + } +} + +// Default: diagnostics stay executable but leave help when their whole +// domain is denied — cobra then drops the empty config group entirely. +func TestBuildInternal_concealDiagnostics(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, nil) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + show := findByPath(root, "config/policy/show") + if show == nil { + t.Fatal("config policy show not in tree") + } + if !show.Hidden { + t.Error("exempt diagnostic should be hidden from help when its domain is denied") + } + // Still dispatchable: its RunE is the original, not an unavailable stub. + if show.Annotations[cmdpolicy.AnnotationDenialLayer] != "" { + t.Error("exempt diagnostic must not carry a denial stub by default") + } + if cfg := findByPath(root, "config"); cfg.IsAvailableCommand() { + t.Error("config group with no visible children must drop from help") + } +} + +// Strict-mode discovery resolves credentials before plugin policy is applied +// and caches the not-configured error. The final envelope guard must remove its +// now-dead config-init hint after presentation converges the config domain. +func TestBuildInternal_cachedNotConfiguredHintFollowsPluginPresentation(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config", "config/**"}, nil) + + var stdout, stderr bytes.Buffer + f, _, _ := buildInternal(context.Background(), buildInvocationForTest(t), + WithIO(strings.NewReader(""), &stdout, &stderr)) + _, err := f.Config() + p, ok := errs.ProblemOf(err) + if !ok || p.Subtype != errs.SubtypeNotConfigured { + t.Fatalf("Config() error = %T %v, want typed not_configured", err, err) + } + if p.Hint == "" { + t.Fatal("test precondition failed: early cached error has no config hint") + } + if !policystate.DomainDeniedByPlugin("config") { + t.Fatal("test precondition failed: config domain did not converge") + } + + if code := handleRootError(f, err); code != 3 { + t.Fatalf("handleRootError() exit = %d, want 3", code) + } + if p.Hint != "" { + t.Fatalf("final recovery hint = %q, want empty for retired config domain", p.Hint) + } + if out := stderr.String(); !strings.Contains(out, `"subtype": "not_configured"`) || strings.Contains(out, "config init") { + t.Fatalf("final envelope did not apply recovery-hint gate:\n%s", out) + } +} + +func TestApplyPolicyRecoveryHintGate_preservesUnrelatedAndRawHints(t *testing.T) { + policystate.ResetForTesting() + t.Cleanup(policystate.ResetForTesting) + previousWorkspace := core.CurrentWorkspace() + core.SetCurrentWorkspace(core.WorkspaceLocal) + t.Cleanup(func() { core.SetCurrentWorkspace(previousWorkspace) }) + + canonical := core.NotConfiguredError() + raw := errs.MarkRaw(core.NotConfiguredError()) + nonCanonical := errs.NewConfigError(errs.SubtypeNotConfigured, "profile missing"). + WithHint("available profiles: production") + otherSubtype := errs.NewConfigError(errs.SubtypeInvalidConfig, "malformed"). + WithHint("repair the JSON file") + otherCategory := errs.NewAuthenticationError(errs.SubtypeTokenMissing, "missing token"). + WithHint("use an external credential provider") + + policystate.SetPluginDeniedDomains(map[string]bool{"config": true}) + applyPolicyRecoveryHintGate(canonical) + if p, _ := errs.ProblemOf(canonical); p.Hint != "" { + t.Fatalf("canonical config-command hint = %q, want empty", p.Hint) + } + for name, err := range map[string]error{ + "raw": raw, + "non-canonical": nonCanonical, + "other subtype": otherSubtype, + "other category": otherCategory, + } { + p, ok := errs.ProblemOf(err) + if !ok || p.Hint == "" { + t.Fatalf("%s test precondition failed: %+v", name, err) + } + want := p.Hint + applyPolicyRecoveryHintGate(err) + if p.Hint != want { + t.Errorf("%s hint changed from %q to %q", name, want, p.Hint) + } + } + + policystate.SetPluginDeniedDomains(nil) + withoutDenial := core.NotConfiguredError() + p, _ := errs.ProblemOf(withoutDenial) + want := p.Hint + applyPolicyRecoveryHintGate(withoutDenial) + if p.Hint != want { + t.Fatalf("canonical hint changed without config denial: %q -> %q", want, p.Hint) + } +} + +// HideDiagnostics retires the exemptions like any other denied command. +func TestBuildInternal_hideDiagnostics(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, func(b *platform.Builder) *platform.Builder { + return b.HideDiagnostics() + }) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + show := findByPath(root, "config/policy/show") + if show == nil { + t.Fatal("config policy show not in tree") + } + err := show.RunE(show, nil) + var ve *errs.ValidationError + if !errors.As(err, &ve) || ve.Subtype != errs.SubtypeCommandUnavailable { + t.Errorf("hidden diagnostic must answer command_unavailable, got %v", err) + } +} + +// A plugin Wrapper that swallows errors (returns nil without calling next) +// must not defeat the help meta command's unavailable interception: `help +// ` is framework presentation, not business dispatch, so it stays +// outside the Wrap chain and keeps answering command_unavailable. +func TestBuildInternal_helpCommandImmuneToWrapperSwallow(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, func(b *platform.Builder) *platform.Builder { + return b.Wrap("swallow", platform.All(), func(platform.Handler) platform.Handler { + return func(context.Context, platform.Invocation) error { return nil } + }) + }) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + helpCmd := findByPath(root, "help") + if helpCmd == nil || helpCmd.RunE == nil { + t.Fatal("custom help command not installed") + } + + err := helpCmd.RunE(helpCmd, []string{"config"}) + var ve *errs.ValidationError + if !errors.As(err, &ve) || ve.Subtype != errs.SubtypeCommandUnavailable { + t.Errorf("help on a restricted command must answer command_unavailable even under a swallowing Wrapper, got %v", err) + } +} + +// A gated --profile must be rejected at parse time, so cobra's help/version +// fast paths (which never reach PersistentPreRunE) cannot slip past the gate. +func TestBuildInternal_gatedProfileRejectedOnHelpFastPath(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"profile", "profile/**"}, nil) + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + for _, args := range [][]string{ + {"--profile", "prod", "--help"}, + {"--profile", "prod", "--version"}, + {"--profile", "prod", "skills", "--help"}, + } { + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs(args) + err := root.Execute() + var ve *errs.ValidationError + if !errors.As(err, &ve) || !strings.Contains(ve.Message, `unknown flag "--profile"`) { + t.Errorf("%v: gated --profile must fail as unknown flag on the fast path, got %v", args, err) + } + } + + // Un-gated flags parse normally (no over-rejection). + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"--help"}) + if err := root.Execute(); err != nil { + t.Errorf("bare --help must still work, got %v", err) + } +} + +// With HideDiagnostics, every group on the exemption chain answers +// unavailable — bare `config policy` / `config plugins` included, not only +// the show leaves. +func TestBuildInternal_retireDiagnosticsCoversIntermediates(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, func(b *platform.Builder) *platform.Builder { + return b.HideDiagnostics() + }) + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + for _, path := range []string{"config/policy", "config/plugins", "config/policy/show", "config/plugins/show"} { + c := findByPath(root, path) + if c == nil || c.RunE == nil { + t.Fatalf("%s missing or without RunE", path) + } + err := c.RunE(c, nil) + var ve *errs.ValidationError + if !errors.As(err, &ve) || ve.Subtype != errs.SubtypeCommandUnavailable { + t.Errorf("%s must answer command_unavailable, got %v", path, err) + } + } +} + +// Rule.DeniedMessage reaches the synthesized diagnostics denial even when +// the rule itself denies no command. +func TestBuildInternal_deniedMessageAppliesToRetiredDiagnostics(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + t.Cleanup(func() { policystate.ResetForTesting() }) + platform.Register(platform.NewPlugin("acme", "1.0"). + Restrict(&platform.Rule{Deny: []string{"zz-not-a-cmd/**"}, DeniedMessage: "not part of acme cli"}). + HideDiagnostics(). + MustBuild()) + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + c := findByPath(root, "config/policy/show") + err := c.RunE(c, nil) + var ve *errs.ValidationError + if !errors.As(err, &ve) { + t.Fatalf("expected validation error, got %T %v", err, err) + } + if ve.Message != "not part of acme cli" { + t.Errorf("retired diagnostic message = %q, want the rule's DeniedMessage", ve.Message) + } +} + +// The custom help command keeps stock cobra fidelity: subcommand completion +// works (denied commands, being hidden, never appear), and its own help +// carries no Risk line the stock command lacks. +func TestBuildInternal_helpCompletionMatchesStock(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, nil) + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + h := findByPath(root, "help") + if h.ValidArgsFunction == nil { + t.Fatal("help must offer subcommand completion like stock cobra") + } + comps, _ := h.ValidArgsFunction(h, nil, "") + joined := strings.Join(comps, "\n") + if !strings.Contains(joined, "im\t") { + t.Errorf("help completion must offer live commands, got:\n%s", joined) + } + if strings.Contains(joined, "config\t") { + t.Errorf("help completion must not offer a denied domain, got:\n%s", joined) + } + + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"help", "help"}) + if err := root.Execute(); err != nil { + t.Fatalf("help help: %v", err) + } + if strings.Contains(buf.String(), "Risk:") { + t.Errorf("help's own help must not carry a Risk line, got:\n%s", buf.String()) + } +} + +// A plugin-denied command's local flags leave shell completion: the command +// presents as absent, so its flag surface must not be enumerable. +func TestBuildInternal_deniedCommandFlagsLeaveCompletion(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"skills/read"}, nil) + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"__complete", "skills", "read", "--"}) + _ = root.Execute() + if strings.Contains(buf.String(), "--json") { + t.Errorf("denied command's local flags must not complete, got:\n%s", buf.String()) + } +} + +// Presentation-time denials (retired diagnostics, converged groups) reach the +// recorded ActivePolicy, so `config policy show` reflects the shipped tree. +func TestBuildInternal_activePolicyIncludesRetiredDiagnostics(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"config/**"}, func(b *platform.Builder) *platform.Builder { + return b.HideDiagnostics() + }) + buildInternal(context.Background(), buildInvocationForTest(t)) + + ap := cmdpolicy.GetActive() + if ap == nil { + t.Fatal("no active policy recorded") + } + for _, path := range []string{"config", "config/policy", "config/policy/show", "config/plugins", "config/plugins/show"} { + if _, ok := ap.DeniedByPath[path]; !ok { + t.Errorf("ActivePolicy.DeniedByPath missing %q (presentation-time denial not recorded)", path) + } + } +} + +// A bare gated --profile (no value) presents as unknown, same as a set one: +// pflag's "needs an argument" path must not leak a different shape. +func TestBuildInternal_gatedProfileBareFlagPresentsUnknown(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"profile", "profile/**"}, nil) + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"--profile"}) + err := root.Execute() + var ve *errs.ValidationError + if !errors.As(err, &ve) || !strings.Contains(ve.Message, `unknown flag "--profile"`) { + t.Errorf("bare gated --profile must present as unknown flag, got %v", err) + } +} + +// Execute's bootstrap parser runs before plugin installation. It must defer a +// missing-value error to Cobra, otherwise a bare retired --profile escapes as +// plain text before the policy gate exists. +func TestExecute_profileBootstrapErrorsDeferToCobra(t *testing.T) { + t.Setenv("LARKSUITE_CLI_NO_UPDATE_NOTIFIER", "1") + t.Setenv("LARKSUITE_CLI_NO_SKILLS_NOTIFIER", "1") + + t.Run("retired flag is unknown", func(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + policystate.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + t.Cleanup(policystate.ResetForTesting) + platform.Register(platform.NewPlugin("acme", "1.0"). + Restrict(&platform.Rule{Deny: []string{"profile", "profile/**"}}). + MustBuild()) + + code, _, stderr := executeWithCapturedOS(t, "--profile") + if code != 2 || !strings.Contains(stderr, `"subtype": "invalid_argument"`) || + !strings.Contains(stderr, `unknown flag \"--profile\"`) { + t.Fatalf("retired bare --profile: exit=%d stderr=%s", code, stderr) + } + }) + + t.Run("ordinary flag keeps needs-argument error", func(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + policystate.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + t.Cleanup(policystate.ResetForTesting) + + code, _, stderr := executeWithCapturedOS(t, "--profile") + if code != 2 || !strings.Contains(stderr, "flag needs an argument") || strings.Contains(stderr, "unknown flag") { + t.Fatalf("ordinary bare --profile: exit=%d stderr=%s", code, stderr) + } + }) + + t.Run("valid profile still reaches help", func(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + policystate.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + t.Cleanup(policystate.ResetForTesting) + + code, stdout, stderr := executeWithCapturedOS(t, "--profile", "prod", "--help") + if code != 0 || !strings.Contains(stdout, "Usage:") { + t.Fatalf("valid profile help: exit=%d stdout=%s stderr=%s", code, stdout, stderr) + } + }) +} + +func executeWithCapturedOS(t *testing.T, args ...string) (int, string, string) { + t.Helper() + oldArgs, oldStdout, oldStderr := os.Args, os.Stdout, os.Stderr + stdout, err := os.CreateTemp(t.TempDir(), "stdout") + if err != nil { + t.Fatal(err) + } + stderr, err := os.CreateTemp(t.TempDir(), "stderr") + if err != nil { + t.Fatal(err) + } + restored := false + restore := func() { + if restored { + return + } + restored = true + os.Args, os.Stdout, os.Stderr = oldArgs, oldStdout, oldStderr + } + defer restore() + os.Args = append([]string{"e2e-cli"}, args...) + os.Stdout, os.Stderr = stdout, stderr + code := Execute() + restore() + if err := stdout.Close(); err != nil { + t.Fatal(err) + } + if err := stderr.Close(); err != nil { + t.Fatal(err) + } + stdoutData, err := os.ReadFile(stdout.Name()) + if err != nil { + t.Fatal(err) + } + stderrData, err := os.ReadFile(stderr.Name()) + if err != nil { + t.Fatal(err) + } + return code, string(stdoutData), string(stderrData) +} + +// A plugin-denied command offers no positional completion either. +func TestBuildInternal_deniedCommandPositionalsLeaveCompletion(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"skills/read"}, nil) + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"__complete", "skills", "read", ""}) + _ = root.Execute() + if strings.Contains(buf.String(), "lark-") { + t.Errorf("denied command must not complete positional args, got:\n%s", buf.String()) + } +} + +// A denied command that offers static ValidArgs (completion's shell names) +// must not complete them either: cobra serves ValidArgs through a separate +// channel from ValidArgsFunction, and both leave with the denial. +func TestBuildInternal_deniedCommandStaticValidArgsLeaveCompletion(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, []string{"completion", "completion/**"}, nil) + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"__complete", "completion", ""}) + _ = root.Execute() + if strings.Contains(buf.String(), "bash") { + t.Errorf("denied completion command must not offer shell names, got:\n%s", buf.String()) + } +} + +// A fresh `lark-cli help` must render root usage with --version, exactly like +// stock cobra (whose help command inits the version flag before rendering). +// Fresh root + single invocation: an earlier --help run would register the +// flag as a side effect and mask the regression. +func TestBuildInternal_helpShowsVersionFlagFresh(t *testing.T) { + tmpHome(t) + restrictingPlugin(t, nil, nil) + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + + var buf bytes.Buffer + root.SetOut(&buf) + root.SetErr(&buf) + root.SetArgs([]string{"help"}) + if err := root.Execute(); err != nil { + t.Fatalf("help: %v", err) + } + if !strings.Contains(buf.String(), "--version") { + t.Errorf("fresh `help` must show --version like stock cobra, got:\n%.300s", buf.String()) + } +} + +// spec'd precedence: a command restricted by BOTH the user's strict-mode and a +// plugin Rule keeps the strict-mode identity error — strict-mode is a +// user-side security boundary and is never re-labelled as absent. +func TestApply_strictStubWinsOverPluginDenial(t *testing.T) { + root := newTestTree() + pruneForStrictMode(root, core.StrictModeBot) + stub := findCmd(root, "auth", "login") + if stub == nil { + t.Fatal("auth/login strict stub missing") + } + + n := cmdpolicy.Apply(root, map[string]cmdpolicy.Denial{ + "auth/login": {Layer: cmdpolicy.LayerPolicy, PolicySource: "plugin:acme"}, + }) + _ = n + + if got := stub.Annotations[cmdpolicy.AnnotationDenialLayer]; got != cmdpolicy.LayerStrictMode { + t.Fatalf("denial layer = %q, want strict_mode preserved", got) + } + err := stub.RunE(stub, nil) + if err == nil || !strings.Contains(err.Error(), "strict mode") { + t.Errorf("double-restricted command must keep the strict-mode error, got %v", err) + } +} diff --git a/cmd/prune.go b/cmd/prune.go index 49979423f..c67ce16b1 100644 --- a/cmd/prune.go +++ b/cmd/prune.go @@ -13,6 +13,7 @@ import ( "github.com/larksuite/cli/internal/cmdpolicy" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/policystate" ) // pruneForStrictMode removes commands incompatible with the active strict mode. @@ -105,8 +106,14 @@ func strictModeStubFrom(child *cobra.Command, mode core.StrictMode) *cobra.Comma }, RunE: func(c *cobra.Command, _ []string) error { cd := cmdpolicy.CommandDeniedFromDenial(cmdpolicy.CanonicalPath(c), denial) + hint := fmt.Sprintf("denied by %s policy (reason_code %s)", cd.Layer, cd.ReasonCode) + // The switch-policy pointer names `config strict-mode`; with + // the config domain plugin-denied it would be a dead end. + if !policystate.DomainDeniedByPlugin("config") { + hint += "; " + stubHint + } return errs.NewValidationError(errs.SubtypeFailedPrecondition, "%s", stubMessage). - WithHint("denied by %s policy (reason_code %s); %s", cd.Layer, cd.ReasonCode, stubHint). + WithHint("%s", hint). WithCause(cd) }, } diff --git a/cmd/prune_test.go b/cmd/prune_test.go index aee11177b..5f6d65596 100644 --- a/cmd/prune_test.go +++ b/cmd/prune_test.go @@ -14,6 +14,7 @@ import ( "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/policystate" "github.com/spf13/cobra" ) @@ -379,3 +380,41 @@ func TestStrictModeStub_PreservesOriginalMetadata(t *testing.T) { t.Errorf("denial annotation overwritten or missing") } } + +// The strict-mode stub's RunE appends a `config strict-mode` switch-policy +// pointer to its hint — but only when the config domain is still present. With +// the config domain plugin-denied that pointer is a dead end, so it is omitted; +// the denial itself (message, subtype) is unchanged. +func TestStrictModeStub_ConfigHintGatedByPluginDenial(t *testing.T) { + hintOf := func(t *testing.T) string { + t.Helper() + child := &cobra.Command{Use: "search", RunE: func(*cobra.Command, []string) error { return nil }} + stub := strictModeStubFrom(child, core.StrictModeBot) + err := stub.RunE(stub, nil) + var verr *errs.ValidationError + if !errors.As(err, &verr) { + t.Fatalf("expected *errs.ValidationError, got %T %v", err, err) + } + if verr.Subtype != errs.SubtypeFailedPrecondition { + t.Fatalf("subtype = %q, want failed_precondition", verr.Subtype) + } + return verr.Hint + } + + t.Run("config domain present: switch-policy pointer included", func(t *testing.T) { + policystate.ResetForTesting() + t.Cleanup(policystate.ResetForTesting) + if h := hintOf(t); !strings.Contains(h, "config strict-mode") { + t.Errorf("hint = %q, want it to reference `config strict-mode`", h) + } + }) + + t.Run("config domain plugin-denied: switch-policy pointer omitted", func(t *testing.T) { + policystate.ResetForTesting() + t.Cleanup(policystate.ResetForTesting) + policystate.SetPluginDeniedDomains(map[string]bool{"config": true}) + if h := hintOf(t); strings.Contains(h, "config strict-mode") { + t.Errorf("hint = %q, want no `config strict-mode` pointer when config is plugin-denied", h) + } + }) +} diff --git a/cmd/root.go b/cmd/root.go index 7e8244fd3..a16597bc3 100644 --- a/cmd/root.go +++ b/cmd/root.go @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "io/fs" "os" "sort" "strings" @@ -18,9 +19,11 @@ import ( "github.com/larksuite/cli/internal/cmdmeta" "github.com/larksuite/cli/internal/cmdpolicy" "github.com/larksuite/cli/internal/cmdutil" + "github.com/larksuite/cli/internal/core" "github.com/larksuite/cli/internal/deprecation" "github.com/larksuite/cli/internal/hook" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/policystate" "github.com/larksuite/cli/internal/skillscheck" "github.com/larksuite/cli/internal/suggest" "github.com/larksuite/cli/internal/update" @@ -80,11 +83,16 @@ Global Flags: Additional help topics:{{range .Commands}}{{if .IsAdditionalHelpTopicCommand}} {{rpad .CommandPath .CommandPathPadding}} {{.Short}}{{end}}{{end}}{{end}}{{if .HasAvailableSubCommands}} -Use "{{.CommandPath}} [command] --help" for more information about a command.{{end}}{{if not .HasParent}} - -Skills setup (one-time, humans): npx skills add larksuite/cli -g -y — https://github.com/larksuite/cli#agent-skills{{end}} +Use "{{.CommandPath}} [command] --help" for more information about a command.{{end}}` + skillsSetupFooter + ` ` +// skillsSetupFooter is the root-help pointer at the human one-time skills +// setup. Split out so the presentation pass can drop it from the template +// when an integrator plugin denies the skills domain. +const skillsSetupFooter = `{{if not .HasParent}} + +Skills setup (one-time, humans): npx skills add larksuite/cli -g -y — https://github.com/larksuite/cli#agent-skills{{end}}` + // Execute runs the root command and returns the process exit code. // rawInvocationArgs holds os.Args[1:] captured at Execute() entry. cobra's // UnknownFlags whitelist (installUnknownSubcommandGuard) swallows unknown flags @@ -95,9 +103,13 @@ var rawInvocationArgs []string func Execute() int { rawInvocationArgs = os.Args[1:] - inv, err := BootstrapInvocationContext(os.Args[1:]) - if err != nil { - fmt.Fprintln(os.Stderr, "Error:", err) + // Bootstrap parsing is best-effort: Cobra must render any flag error after + // the command tree and plugin presentation gates are installed. Returning + // here would leak a plain-text error for a policy-retired global flag before + // its gate can present the flag as absent. + inv, bootstrapErr := BootstrapInvocationContext(os.Args[1:]) + if bootstrapErr != nil && !isDeferredBootstrapProfileError(bootstrapErr) { + fmt.Fprintln(os.Stderr, "Error:", bootstrapErr) return 1 } configureFlagCompletions(os.Args) @@ -131,6 +143,16 @@ func Execute() int { return 0 } +// isDeferredBootstrapProfileError identifies the one bootstrap parse failure +// the completed Cobra tree must render. Bootstrap only registers --profile; +// deferring its missing-value error lets a plugin-retired flag present as +// absent, while an ordinary build still emits Cobra's typed needs-argument +// envelope. Any future bootstrap error keeps the existing early-fail behavior +// until its full-tree equivalence is explicitly established. +func isDeferredBootstrapProfileError(err error) bool { + return err != nil && err.Error() == "flag needs an argument: --profile" +} + // setupNotices wires both the binary update notice and the skills // staleness notice into output.PendingNotice as a composed function. // Each provider populates an independent key under _notice; either @@ -168,6 +190,10 @@ func setupNotices() { // Extracted from Execute so the composition is unit-testable. func composePendingNotice() map[string]interface{} { notice := map[string]interface{}{} + // All three notices steer the caller to `lark-cli update`. + if policystate.DomainDeniedByPlugin("update") { + return nil + } if info := update.GetPending(); info != nil { notice["update"] = map[string]interface{}{ "current": info.Current, @@ -254,6 +280,7 @@ func handleRootError(f *cmdutil.Factory, err error) int { // local shortcut/service metadata — it never depends on server state. if !errs.IsRaw(err) { applyNeedAuthorizationHint(f, err) + applyPolicyRecoveryHintGate(err) } // Staged dispatch: capture the typed exit code BEFORE attempting the @@ -303,6 +330,26 @@ func handleRootError(f *cmdutil.Factory, err error) int { return output.ExitCodeOf(fallback) } +// applyPolicyRecoveryHintGate is the final presentation guard for typed errors +// created before plugin policy is installed. Credential resolution runs early +// to determine strict mode and caches its result; a not-configured error can +// therefore retain a config-init hint that was valid at construction time but +// points at a command the completed build retired. Gate on typed metadata at +// the envelope boundary instead of parsing or rewriting prose. +func applyPolicyRecoveryHintGate(err error) { + if errs.IsRaw(err) { + return + } + p, ok := errs.ProblemOf(err) + if !ok || p.Hint == "" { + return + } + if p.Category == errs.CategoryConfig && p.Subtype == errs.SubtypeNotConfigured && + core.HasConfigCommandRecoveryTarget(err) && policystate.DomainDeniedByPlugin("config") { + p.Hint = "" + } +} + // cobraUsageErrorMarkers are the stable error-text fragments cobra / pflag // (pinned at v1.10.2) emit for usage mistakes — missing required flag, unknown // command / flag, wrong argument count. Cobra surfaces these as plain errors, @@ -601,6 +648,15 @@ func isLarkDomain(c *cobra.Command) bool { func flagDidYouMean(c *cobra.Command, ferr error) error { name, isUnknown := unknownFlagName(ferr) if !isUnknown { + // A policy-gated flag invoked bare ("flag needs an argument") + // never reaches its rejecting Value; it still presents as + // unregistered, exactly like a set one. + if gated, ok := gatedFlagFromNeedsArg(c, ferr); ok { + return errs.NewValidationError(errs.SubtypeInvalidArgument, + "unknown flag %q for %q", "--"+gated, c.CommandPath()). + WithParams(errs.InvalidParam{Name: "--" + gated, Reason: "unknown flag"}). + WithHint("run `%s --help` to see valid flags", c.CommandPath()) + } return errs.NewValidationError(errs.SubtypeInvalidArgument, "%s", ferr.Error()). WithHint("run `%s --help` for valid flags", c.CommandPath()) } @@ -623,6 +679,25 @@ func flagDidYouMean(c *cobra.Command, ferr error) error { WithHint("%s", hint) } +// gatedFlagFromNeedsArg reports whether ferr is pflag's "flag needs an +// argument: --name" for a policy-gated flag on this command's flag set. +func gatedFlagFromNeedsArg(c *cobra.Command, ferr error) (string, bool) { + const p = "flag needs an argument: --" + msg := ferr.Error() + i := strings.Index(msg, p) + if i < 0 { + return "", false + } + name := msg[i+len(p):] + if j := strings.IndexAny(name, " \t"); j >= 0 { + name = name[:j] + } + if fl := c.Root().PersistentFlags().Lookup(name); isPolicyGatedFlag(fl) { + return name, true + } + return "", false +} + // unknownFlagName extracts the offending long-flag name from cobra's flag-parse // error text ("unknown flag: --query" → "query"). Returns ok=false for anything // else (missing argument, invalid value, unknown shorthand) so the caller keeps @@ -659,16 +734,104 @@ func visibleFlagNames(c *cobra.Command) []string { return names } +// installHelpCommand replaces cobra's default help command so that +// `lark-cli help ` returns a typed error (exit 2) +// instead of printing an envelope and exiting 0 — cobra's stock help +// command has no error channel. +func installHelpCommand(root *cobra.Command) { + helpCmd := &cobra.Command{ + Use: "help [command]", + Short: "Help about any command", + Long: "Help provides help for any command in the application.\n" + + "Simply type " + root.DisplayName() + " help [path to command] for full details.", + ValidArgsFunction: func(c *cobra.Command, args []string, toComplete string) ([]string, cobra.ShellCompDirective) { + // Mirrors cobra's stock help completion: available (non-hidden) + // subcommands of the resolved path — a denied command is + // Hidden and therefore never offered. + cmd, _, e := root.Find(args) + if e != nil || cmd == nil { + return nil, cobra.ShellCompDirectiveNoFileComp + } + var comps []string + for _, sub := range cmd.Commands() { + if !sub.IsAvailableCommand() && sub.Name() != "help" { + continue + } + if strings.HasPrefix(sub.Name(), toComplete) { + comps = append(comps, sub.Name()+"\t"+sub.Short) + } + } + return comps, cobra.ShellCompDirectiveNoFileComp + }, + RunE: func(c *cobra.Command, args []string) error { + target, _, err := root.Find(args) + if err != nil || target == nil { + c.Printf("Unknown help topic %#q\n", args) + return root.Usage() + } + if msg, ok := unavailableHelpMessage(target); ok { + return errs.NewValidationError(errs.SubtypeCommandUnavailable, "%s", msg) + } + target.InitDefaultHelpFlag() + target.InitDefaultVersionFlag() + return target.Help() + }, + } + // help attaches after policy evaluation (framework meta command, never + // policy-evaluated). No risk annotation: it would render a "Risk:" + // line that stock cobra help output does not carry. + cmdutil.DisableAuthCheck(helpCmd) + if helpCmd.Annotations == nil { + helpCmd.Annotations = map[string]string{} + } + helpCmd.Annotations[cmdpolicy.AnnotationFrameworkMeta] = "true" + root.SetHelpCommand(helpCmd) + // SetHelpCommand alone defers attachment to Execute's + // InitDefaultHelpCmd; add it now so the built tree is complete + // (InitDefaultHelpCmd re-adds idempotently). + root.AddCommand(helpCmd) +} + +// unavailableHelpMessage returns the message to render in place of help +// for a plugin-restricted command. yaml-source denials keep original help. +func unavailableHelpMessage(cmd *cobra.Command) (string, bool) { + if cmd == nil || cmd.Annotations == nil { + return "", false + } + if cmd.Annotations[cmdpolicy.AnnotationDenialLayer] != cmdpolicy.LayerPolicy || + !cmdpolicy.IsPluginPolicySource(cmd.Annotations[cmdpolicy.AnnotationDenialSource]) { + return "", false + } + if msg := cmd.Annotations[cmdpolicy.AnnotationDenialMessage]; msg != "" { + return msg, true + } + return cmdpolicy.DefaultUnavailableMessage, true +} + // installTipsHelpFunc wraps the default help function to append a TIPS section // when a command has tips set via cmdutil.SetTips. It also force-shows global // flags that are normally hidden in single-app mode (currently --profile) // when rendering the root command's own help, so users discovering the CLI // still see them at `lark-cli --help`. -func installTipsHelpFunc(root *cobra.Command) { +// +// skillContent is read lazily at help-render time (not captured up front) so +// the domain-guide pointer reflects the resolved skill tree -- the same +// f.SkillContent that `skills list`/`read` serve -- even though plugin skill +// customization is applied after this help func is installed. +func installTipsHelpFunc(root *cobra.Command, skillContent func() fs.FS) { defaultHelp := root.HelpFunc() root.SetHelpFunc(func(cmd *cobra.Command, args []string) { + // Explicit help on a plugin-restricted command answers the same + // unavailable envelope as its RunE stub, not the original usage. + if msg, ok := unavailableHelpMessage(cmd); ok { + output.WriteTypedErrorEnvelope(cmd.ErrOrStderr(), + errs.NewValidationError(errs.SubtypeCommandUnavailable, "%s", msg), "") + return + } if cmd == root { - if f := root.PersistentFlags().Lookup("profile"); f != nil && f.Hidden { + // Force-show flags hidden by single-app mode; never a + // policy-retired one. + if f := root.PersistentFlags().Lookup("profile"); f != nil && f.Hidden && !isPolicyGatedFlag(f) { f.Hidden = false defer func() { f.Hidden = true }() } @@ -676,15 +839,15 @@ func installTipsHelpFunc(root *cobra.Command) { // Domain and method commands compose their agent guidance into Long lazily // here (shortcuts attach after service registration); both skip the generic // bottom-of-help append below. - if service.PrepareDomainHelp(cmd, embeddedSkillContent) { + if service.PrepareDomainHelp(cmd, skillContent()) { defaultHelp(cmd, args) return } - if service.PrepareMethodHelp(cmd, embeddedSkillContent) { + if service.PrepareMethodHelp(cmd, skillContent()) { defaultHelp(cmd, args) return } - if service.PrepareShortcutHelp(cmd, embeddedSkillContent) { + if service.PrepareShortcutHelp(cmd, skillContent()) { defaultHelp(cmd, args) return } diff --git a/cmd/root_risk_help_test.go b/cmd/root_risk_help_test.go index 2556bfdac..efaf62c59 100644 --- a/cmd/root_risk_help_test.go +++ b/cmd/root_risk_help_test.go @@ -5,6 +5,7 @@ package cmd import ( "bytes" + "io/fs" "strings" "testing" @@ -12,6 +13,10 @@ import ( "github.com/spf13/cobra" ) +// nilSkills is the skill-content getter used by help-func tests that do +// not exercise the domain-guide pointer. +func nilSkills() fs.FS { return nil } + // rendersHelp runs the wrapped help func and returns stdout. func rendersHelp(t *testing.T, cmd *cobra.Command) string { t.Helper() @@ -24,7 +29,7 @@ func rendersHelp(t *testing.T, cmd *cobra.Command) string { func TestHelpFunc_RendersRiskLineWhenAnnotated(t *testing.T) { root := &cobra.Command{Use: "lark-cli"} - installTipsHelpFunc(root) + installTipsHelpFunc(root, nilSkills) child := &cobra.Command{Use: "delete", Short: "delete a file"} cmdutil.SetRisk(child, "high-risk-write") @@ -38,7 +43,7 @@ func TestHelpFunc_RendersRiskLineWhenAnnotated(t *testing.T) { func TestHelpFunc_NoRiskLineWhenUnannotated(t *testing.T) { root := &cobra.Command{Use: "lark-cli"} - installTipsHelpFunc(root) + installTipsHelpFunc(root, nilSkills) child := &cobra.Command{Use: "list", Short: "list items"} root.AddCommand(child) @@ -51,7 +56,7 @@ func TestHelpFunc_NoRiskLineWhenUnannotated(t *testing.T) { func TestHelpFunc_RiskLinePrecedesTips(t *testing.T) { root := &cobra.Command{Use: "lark-cli"} - installTipsHelpFunc(root) + installTipsHelpFunc(root, nilSkills) child := &cobra.Command{Use: "delete", Short: "delete a file"} cmdutil.SetRisk(child, "high-risk-write") diff --git a/cmd/service/affordance_test.go b/cmd/service/affordance_test.go index 3202ee432..c79f2ee33 100644 --- a/cmd/service/affordance_test.go +++ b/cmd/service/affordance_test.go @@ -297,6 +297,26 @@ func TestPrepareDomainHelp_PreservesHandAuthoredLong(t *testing.T) { } // A service domain carries only a Short at help time; it seeds the base. +// The domain-guide pointer is likewise gated: removing the domain's skill +// drops the pointer instead of leaving it dangling. +func TestPrepareDomainHelp_GatesGuidePointerOnFS(t *testing.T) { + present := domainCmd("Consume and manage real-time events", "") + if !PrepareDomainHelp(present, fstest.MapFS{"lark-event/SKILL.md": &fstest.MapFile{Data: []byte("x")}}) { + t.Fatal("PrepareDomainHelp returned false for a domain-tagged command") + } + if !strings.Contains(present.Long, "lark-cli skills read lark-event") { + t.Errorf("skill present should emit the domain-guide pointer; got:\n%s", present.Long) + } + + removed := domainCmd("Consume and manage real-time events", "") + if !PrepareDomainHelp(removed, fstest.MapFS{}) { + t.Fatal("PrepareDomainHelp returned false for a domain-tagged command") + } + if strings.Contains(removed.Long, "skills read lark-event") { + t.Errorf("removed skill must leave no domain-guide pointer; got:\n%s", removed.Long) + } +} + func TestPrepareDomainHelp_FallsBackToShort(t *testing.T) { dom := domainCmd("Message and group chat management", "") if !PrepareDomainHelp(dom, nil) { diff --git a/cmd/skill_customization_test.go b/cmd/skill_customization_test.go new file mode 100644 index 000000000..5f64db782 --- /dev/null +++ b/cmd/skill_customization_test.go @@ -0,0 +1,170 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package cmd + +import ( + "context" + "errors" + "strings" + "testing" + "testing/fstest" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/extension/platform" + "github.com/larksuite/cli/internal/policystate" + "github.com/larksuite/cli/internal/skillcontent" +) + +// withBaseSkills swaps the process-global embedded skill tree for the +// duration of a test, restoring it afterward. +func withBaseSkills(t *testing.T, files map[string]string) { + t.Helper() + base := fstest.MapFS{} + for p, content := range files { + base[p] = &fstest.MapFile{Data: []byte(content)} + } + saved := embeddedSkillContent + t.Cleanup(func() { embeddedSkillContent = saved }) + embeddedSkillContent = base +} + +// A plugin's SkillsOverlay must reshape the tree the factory serves: skills +// list/read read f.SkillContent, so a resolved removal/overlay shows up here. +// (The --help pointers are gated on the same f.SkillContent; that gating is +// covered by the PrepareDomainHelp/PrepareMethodHelp tests in cmd/service.) +func TestBuildInternal_appliesPluginSkillsOverlay(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + withBaseSkills(t, map[string]string{ + "lark-a/SKILL.md": "---\ndescription: a\n---\n", + "lark-b/SKILL.md": "---\ndescription: b\n---\n", + "lark-shared/SKILL.md": "---\ndescription: shared\n---\n", + }) + + overlay := fstest.MapFS{ + "lark-new/SKILL.md": &fstest.MapFile{Data: []byte("---\ndescription: new\n---\n")}, + } + platform.Register(platform.NewPlugin("acme", "1.0"). + EmbeddedSkills(&platform.SkillsOverlay{ + Remove: []string{"lark-shared"}, + Overlay: overlay, + }).MustBuild()) + + f, _, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + if f.SkillContent == nil { + t.Fatal("f.SkillContent is nil after skill resolution") + } + + skills, err := skillcontent.New(f.SkillContent).List() + if err != nil { + t.Fatalf("List: %v", err) + } + var names []string + for _, s := range skills { + names = append(names, s.Name) + } + if got := strings.Join(names, ","); got != "lark-a,lark-b,lark-new" { + t.Errorf("skills = %q, want lark-a,lark-b,lark-new (shared removed, new added)", got) + } +} + +// Two plugins each customizing skills must abort at dispatch with a +// structured envelope carrying reason_code multiple_skills_overlay_plugins, not +// silently fall back to the default tree. +func TestBuildInternal_multipleSkillPluginsGuard(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + withBaseSkills(t, map[string]string{"lark-a/SKILL.md": "---\ndescription: a\n---\n"}) + + platform.Register(platform.NewPlugin("acme", "1.0"). + EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-a"}}).MustBuild()) + platform.Register(platform.NewPlugin("globex", "1.0"). + EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-a"}}).MustBuild()) + + _, root, reg := buildInternal(context.Background(), buildInvocationForTest(t)) + if reg != nil { + t.Errorf("skill conflict guard path should yield nil registry") + } + + leaf := findRunnableLeaf(root) + if leaf == nil { + t.Fatal("no runnable leaf in command tree") + } + err := leaf.RunE(leaf, nil) + var verr *errs.ValidationError + if !errors.As(err, &verr) { + t.Fatalf("expected *errs.ValidationError, got %T %+v", err, err) + } + if verr.Subtype != errs.SubtypeFailedPrecondition { + t.Errorf("subtype = %q, want failed_precondition", verr.Subtype) + } + if !strings.Contains(verr.Hint, "multiple_skills_overlay_plugins") { + t.Errorf("hint should surface reason_code multiple_skills_overlay_plugins, got %q", verr.Hint) + } +} + +// Allow keeps only the listed skills from the base — a CLI upgrade adding +// new embedded skills cannot widen an allow-listed build. +func TestBuildInternal_appliesAllowList(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + t.Cleanup(func() { policystate.ResetForTesting() }) + + withBaseSkills(t, map[string]string{ + "lark-a/SKILL.md": "---\ndescription: a\n---\n", + "lark-b/SKILL.md": "---\ndescription: b\n---\n", + "lark-c/SKILL.md": "---\ndescription: c\n---\n", + }) + platform.Register(platform.NewPlugin("acme", "1.0"). + EmbeddedSkills(&platform.SkillsOverlay{Allow: []string{"lark-a", "lark-c"}}). + MustBuild()) + + f, _, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + skills, err := skillcontent.New(f.SkillContent).List() + if err != nil { + t.Fatalf("List: %v", err) + } + var names []string + for _, s := range skills { + names = append(names, s.Name) + } + if got := strings.Join(names, ","); got != "lark-a,lark-c" { + t.Errorf("skills = %q, want lark-a,lark-c (allow-list)", got) + } +} + +// A plugin whose SkillsOverlay cannot compose (Remove naming a skill absent +// from the base) must abort with reason_code invalid_skills_overlay. +func TestBuildInternal_invalidSkillsOverlayGuard(t *testing.T) { + tmpHome(t) + platform.ResetForTesting() + t.Cleanup(platform.ResetForTesting) + + withBaseSkills(t, map[string]string{"lark-a/SKILL.md": "---\ndescription: a\n---\n"}) + + platform.Register(platform.NewPlugin("acme", "1.0"). + EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-does-not-exist"}}).MustBuild()) + + _, root, _ := buildInternal(context.Background(), buildInvocationForTest(t)) + leaf := findRunnableLeaf(root) + if leaf == nil { + t.Fatal("no runnable leaf in command tree") + } + err := leaf.RunE(leaf, nil) + var verr *errs.ValidationError + if !errors.As(err, &verr) { + t.Fatalf("expected *errs.ValidationError, got %T %+v", err, err) + } + if verr.Subtype != errs.SubtypeFailedPrecondition { + t.Errorf("subtype = %q, want failed_precondition", verr.Subtype) + } + if !strings.Contains(verr.Hint, "invalid_skills_overlay") { + t.Errorf("hint should surface reason_code invalid_skills_overlay, got %q", verr.Hint) + } +} diff --git a/errs/subtypes.go b/errs/subtypes.go index df0cf8ab8..a3968ac2d 100644 --- a/errs/subtypes.go +++ b/errs/subtypes.go @@ -14,6 +14,7 @@ const ( const ( SubtypeInvalidArgument Subtype = "invalid_argument" // user-supplied flag / arg failed validation (gRPC INVALID_ARGUMENT alignment) SubtypeFailedPrecondition Subtype = "failed_precondition" // request is valid but the system/resource state is not in the state required to execute; caller must change state (not retry) — e.g. ambiguous remote mapping (gRPC FAILED_PRECONDITION alignment) + SubtypeCommandUnavailable Subtype = "command_unavailable" // command not included in this build (integrator-restricted distribution); absent, not gated ) // CategoryAuthentication subtypes diff --git a/extension/platform/README.md b/extension/platform/README.md index 9f9eb56da..430a67a05 100644 --- a/extension/platform/README.md +++ b/extension/platform/README.md @@ -60,6 +60,7 @@ You should see `audit` in the plugin list. | `Wrap` | Around each command's RunE | Yes (return `*AbortError`) | | `On(Startup/Shutdown)` | Process lifecycle | N/A | | `Restrict(Rule)` | Bootstrap-time, ≥1 per plugin | Denies whole subtrees | +| `EmbeddedSkills(SkillsOverlay)` | Bootstrap-time, ≤1 per plugin | No (customizes embedded skills) | ### Plugin lifecycle @@ -79,7 +80,7 @@ sequenceDiagram Host->>SDK: InstallAll() SDK->>Plugin: Capabilities() SDK->>Plugin: Install(Registrar) - Plugin->>SDK: Observe / Wrap / Restrict / On(Startup,Shutdown) + Plugin->>SDK: Observe / Wrap / Restrict / EmbeddedSkills / On(Startup,Shutdown) SDK->>Plugin: On(Startup) fire Note over Host,Plugin: Each command dispatch @@ -113,6 +114,43 @@ the rejected dispatch. widen another's policy). YAML policy at `~/.lark-cli/policy.yml` (which may itself list several rules under `rules:`) is shadowed by any plugin Restrict. +- A plugin may call `EmbeddedSkills()` at most once to customize the embedded + skill tree — `Allow` keeps only the listed skills (the allow-list + counterpart of `Rule.Allow`, so a CLI upgrade cannot widen the build; + `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. Unlike `Restrict()` + it is NOT a security boundary and does not imply `FailClosed`: it + shapes fail-open guidance content. Removing a skill only drops its + `skills read` / `--help` guidance — it does NOT disable the matching + commands (use `Restrict()` for that). Only ONE plugin per binary may + contribute a `SkillsOverlay`; two DISTINCT plugins is a deliberate + `multiple_skills_overlay_plugins` error. The top-level skill set and + each skill's owning FS are snapshotted during CLI build; files inside an + owned skill directory remain live. Both `Base` and `Overlay` must + contain only valid skill directories with `SKILL.md`. +- A command denied by a **plugin** Rule presents as absent, not as + forbidden: it leaves normal help and command-name completion, explicit + help on it is intercepted, and invoking it answers + `subtype=command_unavailable` with no policy vocabulary and no + recovery hint. Set + `Rule.DeniedMessage` to replace the default + "command not included in this build" with your product's own wording. + yaml-source denials keep the classic `command_denied` presentation — + the user owns that policy and needs to see how to adjust it. One + carve-out: a command already retired by the user's strict-mode setting + keeps its strict-mode identity error even when a plugin Rule also + matches it — strict-mode is a user-side security boundary and is never + re-labelled as absent. Denying a + whole domain also retires what points at it: `--profile` (profile + domain) hides and rejects use, the root-help skills footer (skills), + update notices (update), and the `auth login` recovery hint (auth) + stop rendering. +- `config policy show` / `config plugins show` stay executable under any + plugin policy (hidden from help when their domain is denied) so an + operator can still inspect the rule that locked the build. An + integrator shipping a fully-managed distribution opts out with + `HideDiagnostics()` — requires `Restrict()` on the same plugin. - The `Wrap` factory runs **once per command dispatch**, not at install time. Long-lived state (clients, caches, metrics counters) must live on the Plugin struct or in package-level variables. @@ -153,6 +191,8 @@ messages are localised and may change between releases. | `invalid_hook_registration` | Hook factory returns nil / Wrap chain re-entry / etc. | Yes | | `invalid_rule` | Rule fails ValidateRule (malformed glob, bad MaxRisk, unknown Identity) | Yes | | `multiple_restrict_plugins` | Two or more DISTINCT plugins each contributed Restrict (one plugin may contribute several rules) | Yes | +| `invalid_skills_overlay` | Staging fault (`EmbeddedSkills(nil)` / second call in one plugin) honours FailurePolicy; a `SkillsOverlay` that can't compose (`Remove` names a skill absent from the base, `Overlay` entry lacks `SKILL.md`) always aborts via a fatal dispatch guard | Mixed — see left | +| `multiple_skills_overlay_plugins` | Two or more DISTINCT plugins each contributed a `SkillsOverlay` (only one may own skill content) | No — always aborts (dispatch guard) | | `install_failed` | `Plugin.Install` returned a non-nil error | Yes | | `install_panic` | `Plugin.Install` panicked | Yes | @@ -187,8 +227,8 @@ should additionally check `detail.layer == "policy"`. - [Runnable example: audit observer](./examples/audit-observer/) - [Runnable example: read-only policy](./examples/readonly-policy/) - Builder API: see [`builder.go`](./builder.go) for the full DSL - (`NewPlugin`, `Observer`, `Wrap`, `Restrict`, `FailOpen`/`FailClosed`, - `MustBuild`). + (`NewPlugin`, `Observer`, `Wrap`, `Restrict`, `EmbeddedSkills`, + `HideDiagnostics`, `FailOpen`/`FailClosed`, `MustBuild`). - Inventory diagnostic: run `lark-cli config plugins show` after installing your plugin to see hooks/rules attributed to your plugin name. diff --git a/extension/platform/builder.go b/extension/platform/builder.go index 479fea824..aeaddd3ce 100644 --- a/extension/platform/builder.go +++ b/extension/platform/builder.go @@ -36,8 +36,9 @@ type Builder struct { version string caps Capabilities - actions []func(Registrar) - rules []*Rule + actions []func(Registrar) + rules []*Rule + skillsOverlay *SkillsOverlay hookNames map[string]bool errs []error @@ -81,6 +82,15 @@ func (b *Builder) FailClosed() *Builder { return b } +// HideDiagnostics retires the policy self-inspection commands +// (`config policy show`, `config plugins show`), which otherwise stay +// executable under a plugin policy as the operator's escape hatch. +// Requires Restrict() on the same plugin; Build fails otherwise. +func (b *Builder) HideDiagnostics() *Builder { + b.caps.HideDiagnostics = true + return b +} + // Observer registers an Observer. Multiple calls accumulate. func (b *Builder) Observer(when When, hookName string, sel Selector, fn Observer) *Builder { if !b.validateHookName(hookName, "observer") { @@ -145,6 +155,35 @@ func (b *Builder) Restrict(rule *Rule) *Builder { return b } +// EmbeddedSkills contributes a SkillsOverlay (see SkillsOverlay) +// customizing the CLI's embedded skill content. Unlike Restrict it does +// NOT imply FailClosed: skill customization is guidance content, not a +// security boundary. A plugin owns at most one SkillsOverlay, so calling +// EmbeddedSkills more than once is a build error. +func (b *Builder) EmbeddedSkills(spec *SkillsOverlay) *Builder { + if spec == nil { + b.errs = append(b.errs, errors.New("EmbeddedSkills(nil): spec must not be nil")) + return b + } + if b.skillsOverlay != nil { + b.errs = append(b.errs, errors.New("EmbeddedSkills() called more than once; a plugin owns at most one SkillsOverlay")) + return b + } + b.skillsOverlay = cloneSkillsOverlay(spec) + return b +} + +// cloneSkillsOverlay snapshots the caller's spec so a later mutation of the +// same *SkillsOverlay cannot alter the staged copy. The Remove slice is +// copied; Overlay/Base are fs.FS handles retained by reference (an fs.FS +// is a read-only view, not caller-mutable state). +func cloneSkillsOverlay(spec *SkillsOverlay) *SkillsOverlay { + cp := *spec + cp.Allow = append([]string(nil), spec.Allow...) + cp.Remove = append([]string(nil), spec.Remove...) + return &cp +} + // Build returns the configured Plugin, or an error if any builder // step found a fault. MustBuild panics on the same error. // @@ -155,15 +194,20 @@ func (b *Builder) Build() (Plugin, error) { b.errs = append(b.errs, errors.New( "Restrict() requires FailClosed; do not call FailOpen() after Restrict()")) } + if b.caps.HideDiagnostics && len(b.rules) == 0 { + b.errs = append(b.errs, errors.New( + "HideDiagnostics() requires Restrict(): there is no integrator policy to hide")) + } if len(b.errs) > 0 { return nil, errors.Join(b.errs...) } return &builtPlugin{ - name: b.name, - version: b.version, - caps: b.caps, - actions: b.actions, - rules: b.rules, + name: b.name, + version: b.version, + caps: b.caps, + actions: b.actions, + rules: b.rules, + skillsOverlay: b.skillsOverlay, }, nil } @@ -202,11 +246,12 @@ func (b *Builder) validateHookName(hookName, kind string) bool { // builtPlugin is the Plugin implementation the builder emits. type builtPlugin struct { - name string - version string - caps Capabilities - actions []func(Registrar) - rules []*Rule + name string + version string + caps Capabilities + actions []func(Registrar) + rules []*Rule + skillsOverlay *SkillsOverlay } func (p *builtPlugin) Name() string { return p.name } @@ -216,6 +261,15 @@ func (p *builtPlugin) Install(r Registrar) error { for _, rule := range p.rules { r.Restrict(rule) } + if p.skillsOverlay != nil { + sr, ok := r.(EmbeddedSkillsRegistrar) + if !ok { + // Fail closed: a declared skill customization must never be + // silently dropped by a host that cannot honour it. + return errors.New("host registrar does not support EmbeddedSkills") + } + sr.EmbeddedSkills(p.skillsOverlay) + } for _, action := range p.actions { action(r) } diff --git a/extension/platform/builder_test.go b/extension/platform/builder_test.go index 7a813665f..4172859d4 100644 --- a/extension/platform/builder_test.go +++ b/extension/platform/builder_test.go @@ -14,11 +14,13 @@ import ( // recorder Registrar captures everything a builder schedules so the // test can assert what Install produced without involving the host. type recorder struct { - observers int - wrappers int - lifecycles int - rule *platform.Rule // last rule (existing single-rule assertions) - rules []*platform.Rule // every rule, in Restrict order + observers int + wrappers int + lifecycles int + rule *platform.Rule // last rule (existing single-rule assertions) + rules []*platform.Rule // every rule, in Restrict order + skillsOverlay *platform.SkillsOverlay + skillCalls int } func (r *recorder) Observe(platform.When, string, platform.Selector, platform.Observer) { @@ -30,6 +32,10 @@ func (r *recorder) Restrict(rule *platform.Rule) { r.rule = rule r.rules = append(r.rules, rule) } +func (r *recorder) EmbeddedSkills(spec *platform.SkillsOverlay) { + r.skillsOverlay = spec + r.skillCalls++ +} // Restrict must snapshot each rule: a caller that reuses and mutates the // same *Rule object across two Restrict calls must still get two distinct @@ -211,3 +217,78 @@ func TestBuilder_failOpenThenRestrictOK(t *testing.T) { t.Errorf("FailurePolicy = %v, want FailClosed", p.Capabilities().FailurePolicy) } } + +// EmbeddedSkills() must snapshot Remove: a caller that mutates the same slice +// after the call must still get the value staged at call time. +func TestBuilder_skillsInstalledAndCloned(t *testing.T) { + remove := []string{"lark-shared"} + b := platform.NewPlugin("p", "0").EmbeddedSkills(&platform.SkillsOverlay{Remove: remove}) + remove[0] = "mutated" + + p, err := b.Build() + if err != nil { + t.Fatalf("Build: %v", err) + } + r := &recorder{} + if err := p.Install(r); err != nil { + t.Fatalf("Install: %v", err) + } + if r.skillCalls != 1 { + t.Fatalf("Skills calls = %d, want 1", r.skillCalls) + } + if r.skillsOverlay == nil || len(r.skillsOverlay.Remove) != 1 || r.skillsOverlay.Remove[0] != "lark-shared" { + t.Errorf("staged Remove leaked later mutation: %+v", r.skillsOverlay) + } +} + +func TestBuilder_skillsNilRejected(t *testing.T) { + _, err := platform.NewPlugin("p", "0").EmbeddedSkills(nil).Build() + if err == nil { + t.Fatal("EmbeddedSkills(nil) must produce error") + } +} + +func TestBuilder_skillsTwiceRejected(t *testing.T) { + _, err := platform.NewPlugin("p", "0"). + EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-a"}}). + EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-b"}}). + Build() + if err == nil { + t.Fatal("calling EmbeddedSkills() twice must produce error") + } +} + +// EmbeddedSkills() customizes guidance content, not a security boundary, so +// unlike Restrict() it must NOT force FailClosed. +func TestBuilder_skillsDoesNotForceFailClosed(t *testing.T) { + p, err := platform.NewPlugin("p", "0").EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-a"}}).Build() + if err != nil { + t.Fatalf("Build: %v", err) + } + caps := p.Capabilities() + if caps.Restricts { + t.Error("EmbeddedSkills() must not set Restricts") + } + if caps.FailurePolicy != platform.FailOpen { + t.Errorf("FailurePolicy = %v, want FailOpen (default)", caps.FailurePolicy) + } +} + +// HideDiagnostics only makes sense alongside Restrict: without a rule +// there is no integrator policy whose inspection could be hidden. +func TestBuilder_hideDiagnosticsRequiresRestrict(t *testing.T) { + _, err := platform.NewPlugin("p", "0").HideDiagnostics().Build() + if err == nil { + t.Fatal("HideDiagnostics() without Restrict() must fail Build") + } + p, err := platform.NewPlugin("p", "0"). + Restrict(&platform.Rule{Deny: []string{"config/**"}}). + HideDiagnostics(). + Build() + if err != nil { + t.Fatalf("HideDiagnostics()+Restrict() must build: %v", err) + } + if !p.Capabilities().HideDiagnostics { + t.Error("Capabilities.HideDiagnostics must be set") + } +} diff --git a/extension/platform/capabilities.go b/extension/platform/capabilities.go index fc517c426..d434344c1 100644 --- a/extension/platform/capabilities.go +++ b/extension/platform/capabilities.go @@ -47,4 +47,9 @@ type Capabilities struct { // constants above; the framework requires FailClosed whenever // Restricts=true. FailurePolicy FailurePolicy + + // HideDiagnostics retires the policy self-inspection commands + // (`config policy show`, `config plugins show`) from the build. + // Requires Restricts=true; the install fails otherwise. + HideDiagnostics bool } diff --git a/extension/platform/doc.go b/extension/platform/doc.go index 8897876c2..dc9ef35ef 100644 --- a/extension/platform/doc.go +++ b/extension/platform/doc.go @@ -32,7 +32,12 @@ // gives a comparable rank for the read < write < high-risk-write ordering // - CommandDeniedError - structured error returned to denied callers // -// Stability: every exported symbol here is part of the contract. Internal +// Stability: every exported symbol here is part of the contract. Exported +// structs (Rule, Capabilities, SkillsOverlay) may gain new fields across +// minor versions -- construct them with keyed literals, per standard Go +// module-compatibility practice. Interfaces never gain methods; new host +// capability surfaces arrive as optional extension interfaces (see +// EmbeddedSkillsRegistrar). Internal // orchestration (staging, validation, RunE wrapping, denial guard) lives // under internal/platform, internal/hook and internal/cmdpolicy and is not // importable by third parties. diff --git a/extension/platform/registrar.go b/extension/platform/registrar.go index 36df85f45..277bf136c 100644 --- a/extension/platform/registrar.go +++ b/extension/platform/registrar.go @@ -36,3 +36,22 @@ type Registrar interface { // plugins both calling Restrict abort startup. Restrict(r *Rule) } + +// EmbeddedSkillsRegistrar is the optional extension a host registrar +// implements to accept embedded-skill customization. It is deliberately NOT +// part of Registrar: every exported symbol in this package is a stability +// contract, and widening Registrar would break existing third-party +// implementations (fakes, decorators, custom hosts). A Builder-built plugin +// type-asserts for this interface at Install time and fails closed when the +// host lacks it -- a declared customization is never silently dropped. +// +// Skill content has a single owner: a second customizing plugin, or a +// SkillsOverlay that cannot compose, aborts startup unconditionally. Unlike +// Restrict, EmbeddedSkills is not a security boundary -- it shapes fail-open +// guidance content. Removing a skill drops its guidance but does not disable +// any command; use Restrict to actually block a command. +type EmbeddedSkillsRegistrar interface { + // EmbeddedSkills contributes a SkillsOverlay customizing the CLI's + // embedded skill content (see SkillsOverlay). + EmbeddedSkills(spec *SkillsOverlay) +} diff --git a/extension/platform/registrar_compat_test.go b/extension/platform/registrar_compat_test.go new file mode 100644 index 000000000..ca97892ac --- /dev/null +++ b/extension/platform/registrar_compat_test.go @@ -0,0 +1,63 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package platform + +import ( + "context" + "testing" +) + +// legacyRegistrarFake is a downstream Registrar implementation written +// BEFORE EmbeddedSkills existed. It must keep compiling: doc.go declares +// every exported symbol a stability contract, so Registrar can never widen. +// If this file goes red, a method was added to Registrar -- move it to an +// optional extension interface instead (see EmbeddedSkillsRegistrar). +type legacyRegistrarFake struct{} + +func (legacyRegistrarFake) Observe(When, string, Selector, Observer) {} +func (legacyRegistrarFake) Wrap(string, Selector, Wrapper) {} +func (legacyRegistrarFake) On(LifecycleEvent, string, LifecycleHandler) {} +func (legacyRegistrarFake) Restrict(*Rule) {} + +var _ Registrar = legacyRegistrarFake{} + +// skillsRegistrarFake opts into the optional extension. +type skillsRegistrarFake struct { + legacyRegistrarFake + got *SkillsOverlay +} + +func (f *skillsRegistrarFake) EmbeddedSkills(spec *SkillsOverlay) { f.got = spec } + +var _ EmbeddedSkillsRegistrar = (*skillsRegistrarFake)(nil) + +// Drive the legacy surface through the interface so the fake stays a +// faithful stand-in for downstream usage (and none of it reads as dead code). +func TestLegacyRegistrarFake_ImplementsContractSurface(t *testing.T) { + var r Registrar = legacyRegistrarFake{} + r.Observe(Before, "x.obs", All(), func(context.Context, Invocation) {}) + r.Wrap("x.wrap", All(), func(next Handler) Handler { return next }) + r.On(Startup, "x.boot", func(context.Context, *LifecycleContext) error { return nil }) + r.Restrict(&Rule{Deny: []string{"config/**"}}) +} + +// A plugin that declared EmbeddedSkills fails closed against a host whose +// registrar lacks the optional extension, and succeeds against one that has it. +func TestBuiltPlugin_embeddedSkillsRequiresOptionalInterface(t *testing.T) { + p := NewPlugin("acme", "1.0"). + EmbeddedSkills(&SkillsOverlay{Remove: []string{"lark-a"}}). + MustBuild() + + if err := p.Install(legacyRegistrarFake{}); err == nil { + t.Error("Install must fail closed when the host cannot honour EmbeddedSkills") + } + + host := &skillsRegistrarFake{} + if err := p.Install(host); err != nil { + t.Fatalf("Install: %v", err) + } + if host.got == nil || len(host.got.Remove) != 1 || host.got.Remove[0] != "lark-a" { + t.Errorf("SkillsOverlay must reach the opted-in host, got %+v", host.got) + } +} diff --git a/extension/platform/rule.go b/extension/platform/rule.go index cf5ecebaf..4e3005667 100644 --- a/extension/platform/rule.go +++ b/extension/platform/rule.go @@ -6,8 +6,9 @@ package platform // Rule is the declarative policy rule data structure. yaml files and // Plugin.Restrict() both produce the same Rule. // -// At any moment there is at most one effective Rule -- the resolver decides -// which source wins (Plugin > yaml > none). This package only defines the +// At any moment there is at most one effective SOURCE of rules -- the +// resolver decides which wins (Plugin > yaml > none); the winning source +// may contribute several scoped rules. This package only defines the // shape; selection lives in internal/cmdpolicy. // // The four filter fields are joined by AND. See the engine's Evaluate for @@ -57,4 +58,11 @@ type Rule struct { // No yaml tag: yaml decoding lives in internal/cmdpolicy/yaml so // platform stays free of a yaml library dependency. AllowUnannotated bool `json:"allow_unannotated,omitempty"` + + // DeniedMessage replaces the default "command not included in this + // build" message for plugin-denied commands. The message is + // build-level: the first non-empty DeniedMessage across the owning + // plugin's rules applies to every denial, so declare it once. yaml + // rules ignore it (they keep the command-denied presentation). + DeniedMessage string `json:"denied_message,omitempty"` } diff --git a/extension/platform/skillsoverlay.go b/extension/platform/skillsoverlay.go new file mode 100644 index 000000000..172153e92 --- /dev/null +++ b/extension/platform/skillsoverlay.go @@ -0,0 +1,55 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package platform + +import "io/fs" + +// SkillsOverlay declares how a plugin customizes the CLI's embedded +// skill content, contributed via Builder.EmbeddedSkills. At most one +// source may own skill content; two customizing plugins abort startup. +// +// Allow / Remove mirror Rule's Allow / Deny: an allow-list keeps only +// what it names, a remove-list drops what it names, and Remove wins +// over Allow. Composition order is fixed: Base (or the CLI default) -> +// Allow -> Remove -> Overlay, a same-named skill resolving to Overlay. +// +// Skills are addressed by exact name (a directory carrying SKILL.md, +// e.g. "lark-doc"), not by command path and not by glob — the skill +// list is flat, so misspellings abort startup instead of silently +// matching nothing. Removing a skill only drops its guidance; it does +// not disable any command (use Restrict for that). +// +// The top-level skill set and each skill's owning FS are snapshotted when +// the CLI builds. Later additions or removals of top-level directories do +// not change the manifest; files within an owned skill directory are read +// live. Base and Overlay must contain only valid skill directories. +type SkillsOverlay struct { + // Allow, when non-empty, keeps only these skills (by name) from the + // base tree — the allow-list counterpart of Rule.Allow. Skills the + // CLI adds in future versions stay out of the build until listed + // here, which a Remove-only spec cannot guarantee. A name not + // present in the base aborts startup. Overlay entries are exempt: + // content the integrator explicitly ships needs no allow-listing. + Allow []string + + // Remove hides these skills, by name (e.g. "lark-shared"), from the + // base tree; it wins over Allow, mirroring Rule's Deny-over-Allow. A + // name not present in the base aborts startup rather than being + // silently ignored. + Remove []string + + // Overlay contributes skills laid over the base: a same-named skill + // replaces the base's entirely, a new name adds one. It is rooted at + // the skill list (entries like "my-skill/SKILL.md"); each top-level + // entry must be a "/" directory containing SKILL.md. Any fs.FS + // works (embed.FS, os.DirFS, fstest.MapFS); embed.FS is not required. + Overlay fs.FS + + // Base replaces the entire base skill tree instead of layering over + // the CLI default. nil keeps the CLI default. Every top-level entry + // must be a valid skill directory containing SKILL.md. Rare: most + // integrators leave it nil and use Remove/Overlay so unchanged skills + // follow the CLI version with no copy to maintain. + Base fs.FS +} diff --git a/internal/auth/errors.go b/internal/auth/errors.go index eb326d547..613ff607c 100644 --- a/internal/auth/errors.go +++ b/internal/auth/errors.go @@ -9,6 +9,7 @@ import ( "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/policystate" ) const ( @@ -41,11 +42,15 @@ func (e *NeedAuthorizationError) Error() string { // legacy *NeedAuthorizationError sentinel is preserved in the Cause chain for // errors.As / errors.Is traversal. func NewNeedUserAuthorizationError(userOpenID string) *errs.AuthenticationError { - return errs.NewAuthenticationError(errs.SubtypeTokenMissing, + e := errs.NewAuthenticationError(errs.SubtypeTokenMissing, "%s (user: %s)", needUserAuthorizationMarker, userOpenID). WithUserOpenID(userOpenID). - WithHint("run: lark-cli auth login to re-authorize"). WithCause(&NeedAuthorizationError{UserOpenId: userOpenID}) + // No recovery hint when the auth domain is absent from this build. + if !policystate.DomainDeniedByPlugin("auth") { + e = e.WithHint("run: lark-cli auth login to re-authorize") + } + return e } // IsNeedUserAuthorizationError reports whether err represents a missing-UAT diff --git a/internal/auth/hint_gate_test.go b/internal/auth/hint_gate_test.go new file mode 100644 index 000000000..c35f63765 --- /dev/null +++ b/internal/auth/hint_gate_test.go @@ -0,0 +1,28 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package auth + +import ( + "strings" + "testing" + + "github.com/larksuite/cli/internal/policystate" +) + +// The auth-login recovery hint points into the auth domain; when an +// integrator plugin denied that whole domain the hint would be a dead +// end, so it stays off the error. +func TestNeedUserAuthorization_hintFollowsAuthDomain(t *testing.T) { + t.Cleanup(policystate.ResetForTesting) + + policystate.SetPluginDeniedDomains(nil) + if e := NewNeedUserAuthorizationError("ou_x"); !strings.Contains(e.Hint, "auth login") { + t.Errorf("default build must keep the auth login hint, got %q", e.Hint) + } + + policystate.SetPluginDeniedDomains(map[string]bool{"auth": true}) + if e := NewNeedUserAuthorizationError("ou_x"); e.Hint != "" { + t.Errorf("auth-denied build must not steer to auth login, got %q", e.Hint) + } +} diff --git a/internal/client/client.go b/internal/client/client.go index f40a1057b..fb5c5fe00 100644 --- a/internal/client/client.go +++ b/internal/client/client.go @@ -23,6 +23,7 @@ import ( "github.com/larksuite/cli/internal/credential" "github.com/larksuite/cli/internal/errclass" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/policystate" "github.com/larksuite/cli/internal/util" ) @@ -71,10 +72,14 @@ func (c *APIClient) resolveAccessToken(ctx context.Context, as core.Identity) (s // for the defensive empty-token branch) and is preserved for errors.Is / // errors.Unwrap traversal without being serialized on the wire. func newTokenMissingError(as core.Identity, cause error) error { - return errs.NewAuthenticationError(errs.SubtypeTokenMissing, + e := errs.NewAuthenticationError(errs.SubtypeTokenMissing, "no access token available for %s", as). - WithHint("run: lark-cli auth login to re-authorize"). WithCause(cause) + // No recovery hint when the auth domain is absent from this build. + if !policystate.DomainDeniedByPlugin("auth") { + e = e.WithHint("run: lark-cli auth login to re-authorize") + } + return e } // buildApiReq converts a RawApiRequest into SDK types and collects diff --git a/internal/client/hint_gate_test.go b/internal/client/hint_gate_test.go new file mode 100644 index 000000000..fdfb51b12 --- /dev/null +++ b/internal/client/hint_gate_test.go @@ -0,0 +1,31 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package client + +import ( + "errors" + "strings" + "testing" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/policystate" +) + +// Same gate as internal/auth: the token-missing recovery hint points into +// the auth domain and stays off the error when a plugin denied it. +func TestTokenMissing_hintFollowsAuthDomain(t *testing.T) { + t.Cleanup(policystate.ResetForTesting) + + policystate.SetPluginDeniedDomains(nil) + var ae *errs.AuthenticationError + if err := newTokenMissingError(core.AsUser, nil); !errors.As(err, &ae) || !strings.Contains(ae.Hint, "auth login") { + t.Errorf("default build must keep the auth login hint, got %v", err) + } + + policystate.SetPluginDeniedDomains(map[string]bool{"auth": true}) + if err := newTokenMissingError(core.AsUser, nil); !errors.As(err, &ae) || ae.Hint != "" { + t.Errorf("auth-denied build must not steer to auth login, got %v", err) + } +} diff --git a/internal/cmdpolicy/active.go b/internal/cmdpolicy/active.go index 7a766e29d..bb806c07e 100644 --- a/internal/cmdpolicy/active.go +++ b/internal/cmdpolicy/active.go @@ -23,6 +23,9 @@ type ActivePolicy struct { Rules []*platform.Rule Source ResolveSource DeniedPaths int // number of commands the engine marked as denied (post-aggregation) + + // DeniedByPath is the full post-aggregation denial map. + DeniedByPath map[string]Denial } var ( @@ -47,6 +50,27 @@ func SetActive(p *ActivePolicy) { activePolicy = cloneActivePolicy(p) } +// AppendActiveDenials merges presentation-time denials (retired or +// converged diagnostics) into the recorded snapshot, so `config policy show` +// and other introspection reflect the tree that actually shipped rather +// than only the bootstrap-time aggregate. +func AppendActiveDenials(extra map[string]Denial) { + activeMu.Lock() + defer activeMu.Unlock() + if activePolicy == nil || len(extra) == 0 { + return + } + if activePolicy.DeniedByPath == nil { + activePolicy.DeniedByPath = map[string]Denial{} + } + for path, d := range extra { + if _, dup := activePolicy.DeniedByPath[path]; !dup { + activePolicy.DeniedPaths++ + } + activePolicy.DeniedByPath[path] = d + } +} + // GetActive returns a deep copy of the recorded policy, or nil if // bootstrap has not finished or no rule applied. Callers can freely // mutate the result — including the embedded Rule slices — without @@ -81,6 +105,12 @@ func cloneActivePolicy(in *ActivePolicy) *ActivePolicy { cp.Rules[i] = &rule } } + if in.DeniedByPath != nil { + cp.DeniedByPath = make(map[string]Denial, len(in.DeniedByPath)) + for k, v := range in.DeniedByPath { + cp.DeniedByPath[k] = v + } + } return &cp } diff --git a/internal/cmdpolicy/aggregation_test.go b/internal/cmdpolicy/aggregation_test.go index 9f1cab61b..3d3ee8bb3 100644 --- a/internal/cmdpolicy/aggregation_test.go +++ b/internal/cmdpolicy/aggregation_test.go @@ -70,7 +70,7 @@ func TestBuildDeniedByPath_parentAggregationAllChildrenDenied(t *testing.T) { } denied := cmdpolicy.BuildDeniedByPath(root, decisions, - cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML, Name: "/policy.yml"}, "agent") + cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML, Name: "/policy.yml"}, "agent", "") // Both leaves denied. if _, ok := denied["im/+send"]; !ok { @@ -110,7 +110,7 @@ func TestBuildDeniedByPath_partialDenialKeepsParent(t *testing.T) { Deny: []string{"docs/+delete"}, }) denied := cmdpolicy.BuildDeniedByPath(root, e.EvaluateAll(root), - cmdpolicy.ResolveSource{Kind: cmdpolicy.SourcePlugin, Name: "secaudit"}, "secaudit-policy") + cmdpolicy.ResolveSource{Kind: cmdpolicy.SourcePlugin, Name: "secaudit"}, "secaudit-policy", "") if _, ok := denied["docs"]; ok { t.Errorf("parent 'docs' must NOT be denied when some children are allowed") @@ -129,7 +129,7 @@ func TestBuildDeniedByPath_rootNeverDenied(t *testing.T) { root := buildTree() e := cmdpolicy.New(&platform.Rule{Allow: []string{"nonexistent/**"}}) denied := cmdpolicy.BuildDeniedByPath(root, e.EvaluateAll(root), - cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML, Name: "/p.yml"}, "") + cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML, Name: "/p.yml"}, "", "") // Every leaf should be denied. We do not assert on the root entry // because Apply skips the root regardless; the contract is "root @@ -156,7 +156,7 @@ func TestBuildDeniedByPath_hybridParentOwnAllowedKeepsAlive(t *testing.T) { Allow: []string{"docs"}, }) denied := cmdpolicy.BuildDeniedByPath(root, e.EvaluateAll(root), - cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML, Name: ""}, "") + cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML, Name: ""}, "", "") // docs/+delete denied (path doesn't match Allow=["docs"]). if _, ok := denied["docs/+delete"]; !ok { @@ -170,13 +170,13 @@ func TestBuildDeniedByPath_hybridParentOwnAllowedKeepsAlive(t *testing.T) { // Apply returns a typed *errs.ValidationError that exposes BOTH paths // consumers rely on: -// 1. cmd/root.go's envelope writer (errs.ProblemOf / failed_precondition -// subtype + exit code 2) +// 1. cmd/root.go's envelope writer (errs.ProblemOf; plugin-source +// denials use subtype command_unavailable + exit code 2) // 2. in-process consumers extracting the platform.CommandDeniedError as // the typed error's Cause via errors.As // -// The policy metadata (layer / policy_source / rule_name / reason_code) -// is folded into the Hint text rather than a separate detail map. +// Plugin-source denials keep the policy metadata OFF the wire (no hint, +// no source / rule vocabulary); it stays reachable on the Cause only. func TestApply_runEReturnsExitErrorAndCommandDeniedError(t *testing.T) { root := buildTree() denied := map[string]cmdpolicy.Denial{ @@ -196,29 +196,31 @@ func TestApply_runEReturnsExitErrorAndCommandDeniedError(t *testing.T) { t.Fatalf("denied command should return error") } - // Path 1: typed-envelope view. The denial is a failed_precondition - // ValidationError so cmd/root.go renders the structured envelope and - // the process exits 2 (ExitValidation). + // Path 1: typed-envelope view. A plugin-source denial presents as + // "command unavailable": the capability is absent from this build, so + // the envelope carries no policy metadata and no recovery hint. var ve *errs.ValidationError if !errors.As(err, &ve) { t.Fatalf("error chain must contain *errs.ValidationError, got %T", err) } - if ve.Subtype != errs.SubtypeFailedPrecondition { - t.Errorf("subtype = %q, want %q", ve.Subtype, errs.SubtypeFailedPrecondition) + if ve.Subtype != errs.SubtypeCommandUnavailable { + t.Errorf("subtype = %q, want %q", ve.Subtype, errs.SubtypeCommandUnavailable) } if code := output.ExitCodeOf(err); code != output.ExitValidation { t.Errorf("exit code = %d, want ExitValidation (%d)", code, output.ExitValidation) } - // The policy metadata is folded into the Hint text: reason_code, - // policy_source, and rule_name must all be discoverable there. - if !strings.Contains(ve.Hint, "write_not_allowed") { - t.Errorf("hint must carry reason_code write_not_allowed, got %q", ve.Hint) + if ve.Message != cmdpolicy.DefaultUnavailableMessage { + t.Errorf("message = %q, want default unavailable message", ve.Message) } - if !strings.Contains(ve.Hint, "plugin:secaudit") { - t.Errorf("hint must carry policy_source plugin:secaudit, got %q", ve.Hint) + // No hint, no policy vocabulary: the wire must not steer the caller + // toward a policy the integrator locked into the build. + if ve.Hint != "" { + t.Errorf("plugin-source denial must carry no hint, got %q", ve.Hint) } - if !strings.Contains(ve.Hint, "secaudit-policy") { - t.Errorf("hint must carry rule_name secaudit-policy, got %q", ve.Hint) + for _, leak := range []string{"policy", "plugin:secaudit", "secaudit-policy", "write_not_allowed"} { + if strings.Contains(ve.Message, leak) { + t.Errorf("message leaks %q: %q", leak, ve.Message) + } } // Path 2: in-process typed-error view -- the *platform.CommandDeniedError @@ -301,7 +303,7 @@ func TestHasRunnableDescendant_ignoresAnnotatedPureGroup(t *testing.T) { e := cmdpolicy.New(&platform.Rule{MaxRisk: "read"}) decisions := e.EvaluateAll(root) - denied := cmdpolicy.BuildDeniedByPath(root, decisions, cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML}, "") + denied := cmdpolicy.BuildDeniedByPath(root, decisions, cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML}, "", "") if _, ok := denied["docs"]; !ok { t.Fatalf("docs should be aggregated as fully denied (pure-group children excluded from live count); map=%+v", denied) @@ -332,7 +334,7 @@ func TestBuildDeniedByPath_aggregatesAnnotatedPureGroup(t *testing.T) { e := cmdpolicy.New(&platform.Rule{MaxRisk: "read"}) decisions := e.EvaluateAll(root) - denied := cmdpolicy.BuildDeniedByPath(root, decisions, cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML}, "") + denied := cmdpolicy.BuildDeniedByPath(root, decisions, cmdpolicy.ResolveSource{Kind: cmdpolicy.SourceYAML}, "", "") if _, ok := denied["drive"]; !ok { t.Fatalf("aggregator must install drive denial when all children denied; map=%+v", denied) diff --git a/internal/cmdpolicy/apply.go b/internal/cmdpolicy/apply.go index c253ca798..252ca0848 100644 --- a/internal/cmdpolicy/apply.go +++ b/internal/cmdpolicy/apply.go @@ -4,7 +4,10 @@ package cmdpolicy import ( + "strings" + "github.com/spf13/cobra" + "github.com/spf13/pflag" "github.com/larksuite/cli/errs" "github.com/larksuite/cli/extension/platform" @@ -73,6 +76,16 @@ const ( AnnotationDenialLayer = "lark:policy_denied_layer" AnnotationDenialSource = "lark:policy_denied_source" + // AnnotationDenialMessage carries the resolved unavailable message + // for the cmd layer's help interceptor (plugin-source denials only). + AnnotationDenialMessage = "lark:policy_denied_message" + + // AnnotationFrameworkMeta marks a framework meta command (help): its + // RunE is framework presentation, dispatched outside the plugin Wrap + // chain so a Wrapper cannot swallow or rewrite its result. Observers + // still see the invocation. + AnnotationFrameworkMeta = "lark:framework_meta" + // AnnotationPureGroup marks a cobra.Command that is logically a // parent-only group but had a RunE attached by the bootstrap-time // unknown-subcommand guard. The engine treats annotated commands @@ -124,6 +137,37 @@ func BuildDenialError(path string, d Denial) *errs.ValidationError { WithCause(cd) } +// IsPluginPolicySource reports whether a policy source names a plugin. +// Plugin sources select the "command unavailable" presentation. +func IsPluginPolicySource(source string) bool { + return strings.HasPrefix(source, "plugin:") +} + +// DefaultUnavailableMessage is the message shown when a plugin-restricted +// command is invoked and the integrator supplied no Rule.DeniedMessage. +const DefaultUnavailableMessage = "command not included in this build" + +// messageOf resolves the effective unavailable message for a denial. +func messageOf(d Denial) string { + if d.DeniedMessage != "" { + return d.DeniedMessage + } + return DefaultUnavailableMessage +} + +// BuildUnavailableError is the plugin-source counterpart of +// BuildDenialError: no hint, no policy vocabulary on the wire. The +// *platform.CommandDeniedError stays reachable as the Cause for +// in-process consumers; Cause is never serialized. +func BuildUnavailableError(path string, d Denial) *errs.ValidationError { + msg := d.DeniedMessage + if msg == "" { + msg = DefaultUnavailableMessage + } + return errs.NewValidationError(errs.SubtypeCommandUnavailable, "%s", msg). + WithCause(CommandDeniedFromDenial(path, d)) +} + // installDenyStub mutates a cobra.Command in place. Unlike cmd/prune.go // which does RemoveCommand+AddCommand (changing the pointer), we modify // the existing node so any external reference (snapshots, alias targets) @@ -158,6 +202,19 @@ func installDenyStub(cmd *cobra.Command, path string, d Denial) bool { } cmd.Hidden = true cmd.DisableFlagParsing = true + // A plugin-denied command presents as absent: its local flags leave + // shell completion too. Hidden, not removed — yaml denials keep the + // full usage render, and the plugin help path never renders usage. + if IsPluginPolicySource(d.PolicySource) { + cmd.Flags().VisitAll(func(f *pflag.Flag) { f.Hidden = true }) + // Both positional-completion channels: the static ValidArgs list + // (cobra serves it before consulting the function) and the + // dynamic ValidArgsFunction. + cmd.ValidArgs = nil + cmd.ValidArgsFunction = func(*cobra.Command, []string, string) ([]string, cobra.ShellCompDirective) { + return nil, cobra.ShellCompDirectiveNoFileComp + } + } // Bypass cobra's pre-RunE gates that would otherwise short-circuit // before the wrapped RunE (= where observers + denial guard live): @@ -194,11 +251,19 @@ func installDenyStub(cmd *cobra.Command, path string, d Denial) bool { cmd.Annotations[AnnotationDenialSource] = d.PolicySource denial := d // capture by value for the closure - cmd.RunE = func(c *cobra.Command, args []string) error { - // The typed message carries the user-facing semantic ("a command - // was denied"); the hint carries the layer / source / rule - // distinction ("policy" vs "strict_mode") for debugging. - return BuildDenialError(path, denial) + if IsPluginPolicySource(d.PolicySource) { + // The message annotation feeds the cmd layer's help interceptor. + cmd.Annotations[AnnotationDenialMessage] = messageOf(d) + cmd.RunE = func(c *cobra.Command, args []string) error { + return BuildUnavailableError(path, denial) + } + } else { + cmd.RunE = func(c *cobra.Command, args []string) error { + // The typed message carries the user-facing semantic ("a command + // was denied"); the hint carries the layer / source / rule + // distinction ("policy" vs "strict_mode") for debugging. + return BuildDenialError(path, denial) + } } // Clear any pre-existing Run hook: cobra prefers RunE when both are // set, but leaving a stale Run around is a foot-gun for future diff --git a/internal/cmdpolicy/denial.go b/internal/cmdpolicy/denial.go index 3411984d0..56974e377 100644 --- a/internal/cmdpolicy/denial.go +++ b/internal/cmdpolicy/denial.go @@ -25,6 +25,10 @@ type Denial struct { RuleName string // matched Rule.Name (if any) ReasonCode string // closed enum, see docs/extension/reason-codes.md Reason string // human-readable + + // DeniedMessage is Rule.DeniedMessage for the plugin-source + // unavailable presentation; empty means the default message. + DeniedMessage string } // ChildDenial is what AggregateChildren consumes — it pairs a Denial diff --git a/internal/cmdpolicy/diagnostic.go b/internal/cmdpolicy/diagnostic.go index 9b2393248..c6e3d9c43 100644 --- a/internal/cmdpolicy/diagnostic.go +++ b/internal/cmdpolicy/diagnostic.go @@ -27,3 +27,15 @@ var diagnosticPaths = map[string]bool{ func IsDiagnosticPath(path string) bool { return diagnosticPaths[path] } + +// DiagnosticPaths returns the exempt self-inspection command paths, for +// the presentation layer: an integrator's HideDiagnostics retires exactly +// this set, and the help-concealment pass hides it from listings when the +// surrounding domain is plugin-denied. +func DiagnosticPaths() []string { + out := make([]string, 0, len(diagnosticPaths)) + for p := range diagnosticPaths { + out = append(out, p) + } + return out +} diff --git a/internal/cmdpolicy/engine.go b/internal/cmdpolicy/engine.go index 2624f0017..990cb3186 100644 --- a/internal/cmdpolicy/engine.go +++ b/internal/cmdpolicy/engine.go @@ -269,8 +269,9 @@ func mergeDenials(rules []*platform.Rule, denials []Decision) Decision { // so `--help` and similar remain available. // // source / ruleName populate PolicySource and RuleName on the produced -// Denial values, so envelope output can attribute denials. -func BuildDeniedByPath(root *cobra.Command, decisions map[string]Decision, source ResolveSource, ruleName string) map[string]Denial { +// Denial values, so envelope output can attribute denials. deniedMessage +// is build-level and applies uniformly, aggregates included. +func BuildDeniedByPath(root *cobra.Command, decisions map[string]Decision, source ResolveSource, ruleName string, deniedMessage string) map[string]Denial { out := map[string]Denial{} sourceLabel := policySourceLabel(source) @@ -287,6 +288,13 @@ func BuildDeniedByPath(root *cobra.Command, decisions map[string]Decision, sourc } aggregateParents(root, out) + + if deniedMessage != "" { + for path, d := range out { + d.DeniedMessage = deniedMessage + out[path] = d + } + } return out } diff --git a/internal/cmdpolicy/source_label_test.go b/internal/cmdpolicy/source_label_test.go index 4c32c3f40..a873344f5 100644 --- a/internal/cmdpolicy/source_label_test.go +++ b/internal/cmdpolicy/source_label_test.go @@ -34,7 +34,7 @@ func TestEnvelope_yamlPolicySourceDoesNotLeakHomePath(t *testing.T) { cmdpolicy.ResolveSource{ Kind: cmdpolicy.SourceYAML, Name: "/Users/alice/.lark-cli/policy.yml", // simulate an absolute path - }, "my-readonly-rule") + }, "my-readonly-rule", "") cmdpolicy.Apply(root, denied) err := leaf.RunE(leaf, nil) @@ -77,7 +77,7 @@ func TestEnvelope_pluginPolicySourceCarriesName(t *testing.T) { }) denied := cmdpolicy.BuildDeniedByPath(root, e.EvaluateAll(root), cmdpolicy.ResolveSource{Kind: cmdpolicy.SourcePlugin, Name: "secaudit"}, - "secaudit-policy") + "secaudit-policy", "") cmdpolicy.Apply(root, denied) err := leaf.RunE(leaf, nil) @@ -85,10 +85,17 @@ func TestEnvelope_pluginPolicySourceCarriesName(t *testing.T) { if !errors.As(err, &ve) { t.Fatalf("expected *errs.ValidationError, got %T", err) } - // The plugin name IS surfaced (in-binary, part of the contract): it - // must appear in the Hint so an integrator debugging a denial knows - // which plugin fired. - if !strings.Contains(ve.Hint, "plugin:secaudit") { - t.Errorf("hint must carry policy_source plugin:secaudit, got %q", ve.Hint) + // A plugin-source denial presents as absent: no hint, no plugin name + // on the wire. Plugin attribution moves to the in-process Cause + // (*platform.CommandDeniedError) for integrators debugging a denial. + if ve.Subtype != errs.SubtypeCommandUnavailable { + t.Errorf("subtype = %q, want %q", ve.Subtype, errs.SubtypeCommandUnavailable) + } + if ve.Hint != "" || strings.Contains(ve.Message, "plugin:secaudit") { + t.Errorf("wire must not expose the plugin source; hint=%q message=%q", ve.Hint, ve.Message) + } + var cd *platform.CommandDeniedError + if !errors.As(err, &cd) || cd.PolicySource != "plugin:secaudit" { + t.Errorf("in-process Cause must carry plugin:secaudit, got %+v", cd) } } diff --git a/internal/core/notconfigured.go b/internal/core/notconfigured.go index 068ea86cf..08c46224d 100644 --- a/internal/core/notconfigured.go +++ b/internal/core/notconfigured.go @@ -8,6 +8,7 @@ import ( "os" "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/policystate" ) // isMalformedConfigError reports whether a config load failure indicates a @@ -66,6 +67,19 @@ const ( agentBindHint = "read `lark-cli config bind --help`, then ask the user to confirm intent and identity preset (bot-only or user-default); only after both are confirmed, run `lark-cli config bind`" ) +// errConfigCommandRecoveryHint marks canonical recovery guidance whose target +// is a config command. It stays in the cause chain (never on the wire) so the +// root dispatcher can remove a hint created before plugin presentation when +// the completed build no longer ships that recovery command. Other errors that +// share the not_configured subtype keep their independent hints. +var errConfigCommandRecoveryHint = errors.New("recovery hint targets a config command") + +// HasConfigCommandRecoveryTarget reports whether err's canonical recovery +// action targets config init/bind. The hint itself may already be hidden. +func HasConfigCommandRecoveryTarget(err error) bool { + return errors.Is(err, errConfigCommandRecoveryHint) +} + // NotConfiguredError returns the canonical "not configured" error, with a // hint that depends on the active workspace: // @@ -81,14 +95,24 @@ const ( func NotConfiguredError() error { ws := CurrentWorkspace() if ws.IsLocal() { - return errs.NewConfigError(errs.SubtypeNotConfigured, "not configured"). - WithHint("%s", localInitHint) + e := errs.NewConfigError(errs.SubtypeNotConfigured, "not configured"). + WithCause(errConfigCommandRecoveryHint) + // With the config domain absent from this build the init/bind + // commands do not exist; no hint beats a dead-end hint. + if !policystate.DomainDeniedByPlugin("config") { + e = e.WithHint("%s", localInitHint) + } + return e } // Agent workspace: the workspace name appears only in the message, never // in the wire subtype, which stays not_configured. - return errs.NewConfigError(errs.SubtypeNotConfigured, + e := errs.NewConfigError(errs.SubtypeNotConfigured, "%s context detected but lark-cli is not bound to it", ws.Display()). - WithHint("%s", agentBindHint) + WithCause(errConfigCommandRecoveryHint) + if !policystate.DomainDeniedByPlugin("config") { + e = e.WithHint("%s", agentBindHint) + } + return e } // reconfigureHint returns the workspace-aware "fix it from scratch" hint @@ -111,9 +135,11 @@ func NoActiveProfileError() error { ws := CurrentWorkspace() if ws.IsLocal() { return errs.NewConfigError(errs.SubtypeNotConfigured, "no active profile"). - WithHint("%s", localInitHint) + WithHint("%s", localInitHint). + WithCause(errConfigCommandRecoveryHint) } return errs.NewConfigError(errs.SubtypeNotConfigured, "no active profile in %s workspace", ws.Display()). - WithHint("%s", agentBindHint) + WithHint("%s", agentBindHint). + WithCause(errConfigCommandRecoveryHint) } diff --git a/internal/core/notconfigured_hint_gate_test.go b/internal/core/notconfigured_hint_gate_test.go new file mode 100644 index 000000000..bf349e8bc --- /dev/null +++ b/internal/core/notconfigured_hint_gate_test.go @@ -0,0 +1,38 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package core + +import ( + "errors" + "strings" + "testing" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/policystate" +) + +// The not-configured recovery hint points at `config init`/`config bind`; +// with the config domain absent from the build it must not render. The error +// itself (subtype, message) is unchanged. +func TestNotConfiguredError_hintFollowsConfigDomain(t *testing.T) { + policystate.ResetForTesting() + t.Cleanup(policystate.ResetForTesting) + + var ce *errs.ConfigError + if err := NotConfiguredError(); !errors.As(err, &ce) || ce.Hint == "" || !strings.Contains(ce.Hint, "config") { + t.Fatalf("default build must hint at a config command, got %+v", err) + } + + policystate.SetPluginDeniedDomains(map[string]bool{"config": true}) + if err := NotConfiguredError(); !errors.As(err, &ce) { + t.Fatalf("expected *errs.ConfigError, got %T", err) + } else { + if ce.Subtype != errs.SubtypeNotConfigured { + t.Errorf("subtype = %q, want not_configured (gate must not change the error)", ce.Subtype) + } + if ce.Hint != "" { + t.Errorf("config-denied build must omit the recovery hint, got %q", ce.Hint) + } + } +} diff --git a/internal/core/notconfigured_test.go b/internal/core/notconfigured_test.go index d546fbe4a..a8f928dad 100644 --- a/internal/core/notconfigured_test.go +++ b/internal/core/notconfigured_test.go @@ -42,6 +42,9 @@ func TestNotConfiguredError_Local(t *testing.T) { if strings.Contains(cfgErr.Hint, "config bind") { t.Errorf("local hint must not mention config bind; got %q", cfgErr.Hint) } + if !HasConfigCommandRecoveryTarget(err) { + t.Error("canonical config recovery hint marker is missing") + } } func TestNotConfiguredError_OpenClaw(t *testing.T) { @@ -104,6 +107,17 @@ func TestNoActiveProfileError_Local(t *testing.T) { if cfgErr.Message != "no active profile" { t.Errorf("message = %q, want %q", cfgErr.Message, "no active profile") } + if !HasConfigCommandRecoveryTarget(err) { + t.Error("no-active-profile config recovery hint marker is missing") + } +} + +func TestHasConfigCommandRecoveryTarget_RejectsUnrelatedNotConfigured(t *testing.T) { + err := errs.NewConfigError(errs.SubtypeNotConfigured, "profile missing"). + WithHint("available profiles: production") + if HasConfigCommandRecoveryTarget(err) { + t.Fatal("unrelated not_configured hint was marked as a config command recovery") + } } func TestNoActiveProfileError_AgentSuggestsBind(t *testing.T) { diff --git a/internal/errclass/classify.go b/internal/errclass/classify.go index bc200b4de..525dc97c4 100644 --- a/internal/errclass/classify.go +++ b/internal/errclass/classify.go @@ -11,6 +11,7 @@ import ( "github.com/larksuite/cli/errs" "github.com/larksuite/cli/internal/core" + "github.com/larksuite/cli/internal/policystate" ) // ClassifyContext is the contextual data BuildAPIError uses to populate @@ -370,13 +371,24 @@ func PermissionHint(missing []string, identity string, subtype errs.Subtype, con } return "the app developer must apply for the required scope(s) at the developer console" case errs.SubtypeMissingScope: + // With the auth domain absent from this build there is no login + // command to point at; no hint beats a dead-end hint. + if policystate.DomainDeniedByPlugin("auth") { + return "" + } if len(missing) > 0 { return fmt.Sprintf("run `lark-cli auth login --scope \"%s\"` to re-authorize the user with the updated scope set", strings.Join(missing, " ")) } return "run `lark-cli auth login` to re-authorize the user with the updated scope set" case errs.SubtypeTokenScopeInsufficient: + if policystate.DomainDeniedByPlugin("auth") { + return "check the token's granted scopes" + } return "check the token's granted scopes; run `lark-cli auth login` to refresh if the scope was added after the token was issued" case errs.SubtypeUserUnauthorized: + if policystate.DomainDeniedByPlugin("auth") { + return "the operation may be blocked by external-chat or admin policy" + } return "run `lark-cli auth login` to re-authorize this user; if re-auth does not help, the operation may be blocked by external-chat or admin policy" case errs.SubtypeAppUnavailable: return "ask the tenant admin to check the app's install status in the Lark admin console" diff --git a/internal/errclass/hint_gate_test.go b/internal/errclass/hint_gate_test.go new file mode 100644 index 000000000..164f89d72 --- /dev/null +++ b/internal/errclass/hint_gate_test.go @@ -0,0 +1,37 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package errclass + +import ( + "strings" + "testing" + + "github.com/larksuite/cli/errs" + "github.com/larksuite/cli/internal/policystate" +) + +// Permission recovery hints point at `auth login`; with the auth domain +// absent from the build those pointers must not render. The tenant-admin and +// console-side hints are not auth-login recoveries and stay. +func TestPermissionHint_followsAuthDomain(t *testing.T) { + policystate.ResetForTesting() + t.Cleanup(policystate.ResetForTesting) + + for _, st := range []errs.Subtype{errs.SubtypeMissingScope, errs.SubtypeTokenScopeInsufficient, errs.SubtypeUserUnauthorized} { + if h := PermissionHint([]string{"im:message"}, "user", st, ""); !strings.Contains(h, "auth login") { + t.Errorf("%s: default build hint should mention auth login, got %q", st, h) + } + } + + policystate.SetPluginDeniedDomains(map[string]bool{"auth": true}) + for _, st := range []errs.Subtype{errs.SubtypeMissingScope, errs.SubtypeTokenScopeInsufficient, errs.SubtypeUserUnauthorized} { + if h := PermissionHint([]string{"im:message"}, "user", st, ""); strings.Contains(h, "auth login") { + t.Errorf("%s: auth-denied build must not point at auth login, got %q", st, h) + } + } + // Non-auth-login recoveries are untouched. + if h := PermissionHint(nil, "bot", errs.SubtypeAppScopeNotApplied, "https://example.com"); !strings.Contains(h, "developer console") { + t.Errorf("console hint must survive the auth gate, got %q", h) + } +} diff --git a/internal/hook/install.go b/internal/hook/install.go index 4e741b46b..86ddc5891 100644 --- a/internal/hook/install.go +++ b/internal/hook/install.go @@ -107,7 +107,11 @@ func wrapRunE(cmd *cobra.Command, reg *Registry, snapshot CommandViewSource) { // If denied, run the originalRunE directly (it is the denyStub // installed by cmdpolicy.Apply). The Wrap chain is bypassed. var err error - if inv.DeniedByPolicy() { + if inv.DeniedByPolicy() || isFrameworkMeta(c) { + // Framework meta commands (help) are presentation, not + // business dispatch: a plugin Wrapper must not swallow or + // rewrite their result (e.g. the command_unavailable answer + // for a restricted target). Observers above still ran. err = invokeOriginal(ctx, c, args, originalRunE, originalRun) } else { // Compose matching Wrappers around the originalRunE. Each @@ -334,6 +338,13 @@ var stderr = func() interface{ Write(p []byte) (int, error) } { // // This indirection lets us avoid an import cycle between hook and // pruning packages. +// isFrameworkMeta reports whether the command is a framework meta command +// (stamped by cmd's installHelpCommand). Key duplicated by value, matching +// the denial keys below: hook sits beneath cmdpolicy in the import graph. +func isFrameworkMeta(c *cobra.Command) bool { + return c.Annotations["lark:framework_meta"] == "true" +} + func populateInvocationDenial(inv *invocation, c *cobra.Command) { const layerKey = "lark:policy_denied_layer" const sourceKey = "lark:policy_denied_source" diff --git a/internal/hook/install_test.go b/internal/hook/install_test.go index b33398f7b..ed493a036 100644 --- a/internal/hook/install_test.go +++ b/internal/hook/install_test.go @@ -412,3 +412,46 @@ func equalStrings(a, b []string) bool { } return true } + +// A framework meta command (help, stamped lark:framework_meta) dispatches +// outside the Wrap chain: a swallowing Wrapper must not intercept it, and its +// own RunE result must come through unchanged. +func TestInstall_frameworkMetaBypassesWrapChain(t *testing.T) { + root := &cobra.Command{Use: "root"} + metaRan := false + sentinel := errors.New("meta result") + leaf := &cobra.Command{ + Use: "help", + RunE: func(*cobra.Command, []string) error { + metaRan = true + return sentinel + }, + Annotations: map[string]string{"lark:framework_meta": "true"}, + } + root.AddCommand(leaf) + + reg := hook.NewRegistry() + wrapCalled := false + reg.AddWrapper(hook.WrapperEntry{ + Name: "swallow", Selector: platform.All(), + Fn: func(next platform.Handler) platform.Handler { + return func(context.Context, platform.Invocation) error { + wrapCalled = true + return nil + } + }, + }) + + hook.Install(root, reg, fakeViewSource{view: fakeView{path: "help"}}) + + err := leaf.RunE(leaf, nil) + if wrapCalled { + t.Error("Wrap chain must not run for a framework meta command") + } + if !metaRan { + t.Error("meta command's own RunE must run") + } + if !errors.Is(err, sentinel) { + t.Errorf("meta result must pass through unchanged, got %v", err) + } +} diff --git a/internal/platform/error.go b/internal/platform/error.go index 0934a6bed..35f770bff 100644 --- a/internal/platform/error.go +++ b/internal/platform/error.go @@ -53,4 +53,12 @@ const ( ReasonInstallPanic = "install_panic" ReasonDuplicatePluginName = "duplicate_plugin_name" ReasonMultipleRestricts = "multiple_restrict_plugins" + // ReasonInvalidSkillsOverlay flags a plugin's SkillsOverlay that cannot + // compose -- EmbeddedSkills() called twice, a Remove naming a skill absent + // from the base, or an Overlay entry missing SKILL.md. + ReasonInvalidSkillsOverlay = "invalid_skills_overlay" + // ReasonMultipleSkillsOverlays flags two or more plugins each contributing + // a SkillsOverlay; only one may own skill content (mirrors + // ReasonMultipleRestricts). + ReasonMultipleSkillsOverlays = "multiple_skills_overlay_plugins" ) diff --git a/internal/platform/host.go b/internal/platform/host.go index b8265ce60..b7030933c 100644 --- a/internal/platform/host.go +++ b/internal/platform/host.go @@ -11,6 +11,7 @@ import ( "github.com/larksuite/cli/extension/platform" "github.com/larksuite/cli/internal/cmdpolicy" "github.com/larksuite/cli/internal/hook" + "github.com/larksuite/cli/internal/skillpolicy" ) // PluginInfo is the metadata of a successfully-installed plugin, @@ -29,9 +30,10 @@ type PluginInfo struct { // every plugin that committed successfully (FailOpen-skipped plugins // are absent), for downstream diagnostics. type InstallResult struct { - Registry *hook.Registry - PluginRules []cmdpolicy.PluginRule - Plugins []PluginInfo + Registry *hook.Registry + PluginRules []cmdpolicy.PluginRule + PluginSkills []skillpolicy.PluginSkill + Plugins []PluginInfo } // InstallAll runs every registered plugin through the staging @@ -142,6 +144,16 @@ func installOne(name string, p platform.Plugin, result *InstallResult) error { } } + // The Builder rejects this at Build time; hand-written plugins are + // caught here (authoring error, aborts unconditionally). + if caps.HideDiagnostics && !caps.Restricts { + return &PluginInstallError{ + PluginName: name, + ReasonCode: ReasonInvalidCapability, + Reason: "HideDiagnostics=true requires Restricts=true", + } + } + // Version compatibility check. Two distinct failure modes: // // 1. Parse error (constraint is malformed, e.g. ">=abc") @@ -207,6 +219,12 @@ func installOne(name string, p platform.Plugin, result *InstallResult) error { Rule: rule, }) } + if staging.skillsOverlay != nil { + result.PluginSkills = append(result.PluginSkills, skillpolicy.PluginSkill{ + PluginName: name, + SkillsOverlay: staging.skillsOverlay, + }) + } // Record the plugin in the inventory. Version is fetched here under // a recover-wrapped helper so a plugin's Version() panic does not diff --git a/internal/platform/host_test.go b/internal/platform/host_test.go index c00a7868a..194ed00a2 100644 --- a/internal/platform/host_test.go +++ b/internal/platform/host_test.go @@ -431,3 +431,84 @@ func TestInstallAll_multipleRestrictPerPlugin(t *testing.T) { result.PluginRules[0].Rule.Name, result.PluginRules[1].Rule.Name) } } + +// skillPlugin contributes a SkillsOverlay via r.EmbeddedSkills; the install pipeline +// must capture it into PluginSkills for the skill resolver. +type skillPlugin struct{} + +func (skillPlugin) Name() string { return "skiller" } +func (skillPlugin) Version() string { return "1.0.0" } +func (skillPlugin) Capabilities() platform.Capabilities { + return platform.Capabilities{FailurePolicy: platform.FailOpen} +} +func (skillPlugin) Install(r platform.Registrar) error { + sr, ok := r.(platform.EmbeddedSkillsRegistrar) + if !ok { + return errors.New("host registrar does not support EmbeddedSkills") + } + sr.EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-shared"}}) + return nil +} + +func TestInstallAll_skillsCommitted(t *testing.T) { + result, err := internalplatform.InstallAll([]platform.Plugin{skillPlugin{}}, nil) + if err != nil { + t.Fatalf("InstallAll: %v", err) + } + if len(result.PluginSkills) != 1 { + t.Fatalf("PluginSkills = %d, want 1", len(result.PluginSkills)) + } + ps := result.PluginSkills[0] + if ps.PluginName != "skiller" { + t.Errorf("PluginName = %q, want skiller", ps.PluginName) + } + if ps.SkillsOverlay == nil || len(ps.SkillsOverlay.Remove) != 1 || ps.SkillsOverlay.Remove[0] != "lark-shared" { + t.Errorf("Spec = %+v, want Remove=[lark-shared]", ps.SkillsOverlay) + } +} + +// doubleSkillPlugin calls r.EmbeddedSkills twice; staging must reject it. Declared +// FailClosed so the staging error aborts InstallAll deterministically. +type doubleSkillPlugin struct{} + +func (doubleSkillPlugin) Name() string { return "double-skiller" } +func (doubleSkillPlugin) Version() string { return "1.0.0" } +func (doubleSkillPlugin) Capabilities() platform.Capabilities { + return platform.Capabilities{FailurePolicy: platform.FailClosed} +} +func (doubleSkillPlugin) Install(r platform.Registrar) error { + sr := r.(platform.EmbeddedSkillsRegistrar) + sr.EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-a"}}) + sr.EmbeddedSkills(&platform.SkillsOverlay{Remove: []string{"lark-b"}}) + return nil +} + +func TestInstallAll_skillsCalledTwice_aborts(t *testing.T) { + _, err := internalplatform.InstallAll([]platform.Plugin{doubleSkillPlugin{}}, nil) + if err == nil { + t.Fatal("calling r.EmbeddedSkills twice must abort a FailClosed plugin") + } + var pi *internalplatform.PluginInstallError + if !errors.As(err, &pi) || pi.ReasonCode != internalplatform.ReasonInvalidSkillsOverlay { + t.Errorf("err = %v, want PluginInstallError reason_code %s", err, internalplatform.ReasonInvalidSkillsOverlay) + } +} + +// hideDiagnosticsOnlyPlugin declares HideDiagnostics without Restricts — +// an authoring error the host rejects unconditionally. +type hideDiagnosticsOnlyPlugin struct{} + +func (hideDiagnosticsOnlyPlugin) Name() string { return "hider" } +func (hideDiagnosticsOnlyPlugin) Version() string { return "1.0.0" } +func (hideDiagnosticsOnlyPlugin) Capabilities() platform.Capabilities { + return platform.Capabilities{HideDiagnostics: true, FailurePolicy: platform.FailOpen} +} +func (hideDiagnosticsOnlyPlugin) Install(platform.Registrar) error { return nil } + +func TestInstallAll_hideDiagnosticsRequiresRestricts(t *testing.T) { + _, err := internalplatform.InstallAll([]platform.Plugin{hideDiagnosticsOnlyPlugin{}}, nil) + var pi *internalplatform.PluginInstallError + if !errors.As(err, &pi) || pi.ReasonCode != internalplatform.ReasonInvalidCapability { + t.Fatalf("HideDiagnostics without Restricts must abort with invalid_capability, got %v", err) + } +} diff --git a/internal/platform/inventory.go b/internal/platform/inventory.go index 063cb3747..ba4cfb85c 100644 --- a/internal/platform/inventory.go +++ b/internal/platform/inventory.go @@ -29,6 +29,10 @@ type PluginEntry struct { // plugin did not call r.Restrict. Rules []*RuleView + // EmbeddedSkills summarises the plugin's EmbeddedSkills contribution; + // nil when the plugin did not customize embedded skills. + EmbeddedSkills *SkillsOverlayView `json:"embedded_skills,omitempty"` + Observers []HookEntry Wrappers []HookEntry Lifecycles []HookEntry @@ -41,6 +45,7 @@ type CapabilitiesView struct { Restricts bool `json:"restricts"` FailurePolicy string `json:"failure_policy"` RequiredCLIVersion string `json:"required_cli_version,omitempty"` + HideDiagnostics bool `json:"hide_diagnostics,omitempty"` } // NewCapabilitiesView converts a platform.Capabilities value into the @@ -50,6 +55,7 @@ func NewCapabilitiesView(c platform.Capabilities) CapabilitiesView { Restricts: c.Restricts, FailurePolicy: failurePolicyLabel(c.FailurePolicy), RequiredCLIVersion: c.RequiredCLIVersion, + HideDiagnostics: c.HideDiagnostics, } } @@ -74,6 +80,22 @@ type RuleView struct { AllowUnannotated bool `json:"allow_unannotated"` } +// SkillsOverlayView is the displayable summary of an EmbeddedSkills +// contribution. Overlay / Base report presence only (an fs.FS has no +// display form). +type SkillsOverlayView struct { + Allow []string `json:"allow,omitempty"` + Remove []string `json:"remove,omitempty"` + Overlay bool `json:"overlay"` + Base bool `json:"base"` +} + +// SkillsInventorySource pairs a plugin name with its overlay summary. +type SkillsInventorySource struct { + PluginName string + View SkillsOverlayView +} + // Inventory is the full snapshot. type Inventory struct { Plugins []PluginEntry @@ -108,7 +130,7 @@ type RuleInventorySource struct { // Hooks are attributed to plugins by the namespaced name convention: // each entry's Name starts with ".", and we group by the // leading segment up to the first dot. -func BuildInventory(plugins []PluginInventorySource, registry *hook.Registry, rules []RuleInventorySource) *Inventory { +func BuildInventory(plugins []PluginInventorySource, registry *hook.Registry, rules []RuleInventorySource, skills []SkillsInventorySource) *Inventory { byPlugin := make(map[string]*PluginEntry, len(plugins)) out := &Inventory{Plugins: make([]PluginEntry, 0, len(plugins))} for _, p := range plugins { @@ -162,6 +184,15 @@ func BuildInventory(plugins []PluginInventorySource, registry *hook.Registry, ru }) } } + + for _, sk := range skills { + if entry := byPlugin[sk.PluginName]; entry != nil { + v := sk.View + v.Allow = append([]string(nil), sk.View.Allow...) + v.Remove = append([]string(nil), sk.View.Remove...) + entry.EmbeddedSkills = &v + } + } return out } @@ -263,6 +294,12 @@ func cloneInventory(in *Inventory) *Inventory { entry.Rules[j] = &rv } } + if p.EmbeddedSkills != nil { + sv := *p.EmbeddedSkills + sv.Allow = append([]string(nil), p.EmbeddedSkills.Allow...) + sv.Remove = append([]string(nil), p.EmbeddedSkills.Remove...) + entry.EmbeddedSkills = &sv + } entry.Observers = append([]HookEntry(nil), p.Observers...) entry.Wrappers = append([]HookEntry(nil), p.Wrappers...) entry.Lifecycles = append([]HookEntry(nil), p.Lifecycles...) diff --git a/internal/platform/inventory_test.go b/internal/platform/inventory_test.go index 992722cff..7f147f018 100644 --- a/internal/platform/inventory_test.go +++ b/internal/platform/inventory_test.go @@ -36,7 +36,7 @@ func TestBuildInventory_groupsByPluginName(t *testing.T) { {PluginName: "a", RuleName: "a-rule", Allow: []string{"docs/**"}, MaxRisk: "read"}, } - inv := internalplatform.BuildInventory(plugins, r, rules) + inv := internalplatform.BuildInventory(plugins, r, rules, nil) if got := len(inv.Plugins); got != 2 { t.Fatalf("Plugins len = %d, want 2", got) @@ -89,7 +89,7 @@ func TestBuildInventory_multipleRulesPerPlugin(t *testing.T) { {PluginName: "a", RuleName: "im-rw", Allow: []string{"im/**"}, MaxRisk: "write"}, } - inv := internalplatform.BuildInventory(plugins, nil, rules) + inv := internalplatform.BuildInventory(plugins, nil, rules, nil) a := findPlugin(inv, "a") if a == nil { t.Fatalf("missing entry a") @@ -103,12 +103,45 @@ func TestBuildInventory_multipleRulesPerPlugin(t *testing.T) { } func TestBuildInventory_empty(t *testing.T) { - inv := internalplatform.BuildInventory(nil, nil, nil) + inv := internalplatform.BuildInventory(nil, nil, nil, nil) if got := len(inv.Plugins); got != 0 { t.Errorf("Plugins len = %d, want 0", got) } } +// A non-nil SkillsInventorySource must surface on the owning plugin's entry as +// an EmbeddedSkills summary, and BuildInventory must clone the Allow/Remove +// slices so a later mutation of the source cannot corrupt the recorded view. +func TestBuildInventory_populatesAndClonesEmbeddedSkills(t *testing.T) { + plugins := []internalplatform.PluginInventorySource{{Name: "acme", Version: "1.0"}} + allow := []string{"lark-im"} + remove := []string{"lark-shared"} + skills := []internalplatform.SkillsInventorySource{ + {PluginName: "acme", View: internalplatform.SkillsOverlayView{ + Allow: allow, Remove: remove, Overlay: true, Base: false, + }}, + } + + inv := internalplatform.BuildInventory(plugins, nil, nil, skills) + entry := findPlugin(inv, "acme") + if entry == nil || entry.EmbeddedSkills == nil { + t.Fatalf("acme entry missing EmbeddedSkills: %+v", entry) + } + es := entry.EmbeddedSkills + if len(es.Allow) != 1 || es.Allow[0] != "lark-im" || + len(es.Remove) != 1 || es.Remove[0] != "lark-shared" || + !es.Overlay || es.Base { + t.Errorf("EmbeddedSkills summary mismatch: %+v", es) + } + + // Mutating the source slices must not leak into the recorded view. + allow[0] = "MUTATED" + remove[0] = "MUTATED" + if es.Allow[0] != "lark-im" || es.Remove[0] != "lark-shared" { + t.Errorf("EmbeddedSkills slices not cloned; source mutation leaked: %+v", es) + } +} + func findPlugin(inv *internalplatform.Inventory, name string) *internalplatform.PluginEntry { for i := range inv.Plugins { if inv.Plugins[i].Name == name { diff --git a/internal/platform/staging.go b/internal/platform/staging.go index e3bb48295..91d271644 100644 --- a/internal/platform/staging.go +++ b/internal/platform/staging.go @@ -41,6 +41,13 @@ type stagingRegistrar struct { // can detect the call. actuallyRestricted bool + // skillsOverlay holds the staged Skills contribution, captured for the + // host to feed into the skill resolver later. nil means the plugin + // did not call r.EmbeddedSkills. overlaySet records that the call happened so a + // second call in the same plugin can be rejected. + skillsOverlay *platform.SkillsOverlay + overlaySet bool + // seenHookNames detects duplicate hookName within this plugin's // Install call. seenHookNames map[string]bool @@ -144,6 +151,25 @@ func (r *stagingRegistrar) Restrict(rule *platform.Rule) { r.rules = append(r.rules, &cp) } +func (r *stagingRegistrar) EmbeddedSkills(spec *platform.SkillsOverlay) { + if r.overlaySet { + r.bufferErr(ReasonInvalidSkillsOverlay, "EmbeddedSkills() called more than once in the same plugin") + return + } + r.overlaySet = true + if spec == nil { + r.bufferErr(ReasonInvalidSkillsOverlay, "EmbeddedSkills(nil)") + return + } + // Defensive clone: freeze Remove so a plugin cannot mutate it after + // Install returns. Overlay/Base are read-only fs.FS views retained by + // reference. + cp := *spec + cp.Allow = append([]string(nil), spec.Allow...) + cp.Remove = append([]string(nil), spec.Remove...) + r.skillsOverlay = &cp +} + // --- helpers --- func (r *stagingRegistrar) namespaced(name string) string { diff --git a/internal/policystate/policystate.go b/internal/policystate/policystate.go new file mode 100644 index 000000000..74d66ef55 --- /dev/null +++ b/internal/policystate/policystate.go @@ -0,0 +1,60 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +// Package policystate answers "did an integrator plugin deny this whole +// command domain?" for render-time hint emitters. It is dependency-free +// because internal/auth and internal/client sit below internal/cmdpolicy +// in the import graph and cannot ask it directly. Written once by the +// bootstrap after policy pruning; read-only thereafter. +package policystate + +import "sync" + +var ( + mu sync.RWMutex + pluginDeniedDomains map[string]bool +) + +// SetPluginDeniedDomains records the plugin-denied top-level domains. +// nil clears. +func SetPluginDeniedDomains(domains map[string]bool) { + mu.Lock() + defer mu.Unlock() + if domains == nil { + pluginDeniedDomains = nil + return + } + cp := make(map[string]bool, len(domains)) + for d, v := range domains { + cp[d] = v + } + pluginDeniedDomains = cp +} + +// AddPluginDeniedDomain records one more plugin-denied domain after the +// bootstrap snapshot: presentation-time convergence (a domain whose last +// live descendants were retired) discovers whole-domain denials the +// bootstrap aggregate cannot see. +func AddPluginDeniedDomain(domain string) { + mu.Lock() + defer mu.Unlock() + if pluginDeniedDomains == nil { + pluginDeniedDomains = map[string]bool{} + } + pluginDeniedDomains[domain] = true +} + +// DomainDeniedByPlugin reports whether the whole top-level domain was +// denied by an integrator plugin. yaml denials never register here. +func DomainDeniedByPlugin(domain string) bool { + mu.RLock() + defer mu.RUnlock() + return pluginDeniedDomains[domain] +} + +// ResetForTesting clears the recorded state. +func ResetForTesting() { + mu.Lock() + defer mu.Unlock() + pluginDeniedDomains = nil +} diff --git a/internal/skillpolicy/overlay.go b/internal/skillpolicy/overlay.go new file mode 100644 index 000000000..5d531db2b --- /dev/null +++ b/internal/skillpolicy/overlay.go @@ -0,0 +1,190 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillpolicy + +import ( + "io" + "io/fs" + "sort" + "strings" + "time" +) + +// overlayFS layers an upper skill tree over a lower one at skill (top +// path segment) granularity: a skill present in upper is served wholly +// from upper, a skill named in removed is hidden, and everything else +// falls through to lower. It is the runtime form of a SkillsOverlay's +// Base -> Remove -> Overlay composition. +// +// It implements the io/fs fast-path interfaces (ReadDir/Stat/ReadFile) +// so the io/fs helpers route through the merge instead of hitting a +// single underlying tree. +type overlayFS struct { + // The manifest: top-level skill name -> owning tree, snapshotted once + // at composition. Routing and listing consult the same snapshot, so a + // top-level directory added later cannot appear through only one of + // those surfaces. Contents WITHIN a skill directory are still read live. + owner map[string]fs.FS + entries []fs.DirEntry // manifest listing, sorted by name +} + +var ( + _ fs.FS = (*overlayFS)(nil) + _ fs.ReadDirFS = (*overlayFS)(nil) + _ fs.StatFS = (*overlayFS)(nil) + _ fs.ReadFileFS = (*overlayFS)(nil) +) + +func newOverlayFS(lower, upper skillTreeSnapshot, remove, allow []string) *overlayFS { + removed := make(map[string]bool, len(remove)) + for _, r := range remove { + removed[r] = true + } + var allowed map[string]bool + if len(allow) > 0 { + allowed = make(map[string]bool, len(allow)) + for _, a := range allow { + allowed[a] = true + } + } + + o := &overlayFS{owner: map[string]fs.FS{}} + for _, e := range upper.entries { + o.owner[e.Name()] = upper.source + o.entries = append(o.entries, e) + } + for _, e := range lower.entries { + name := e.Name() + if removed[name] || o.owner[name] != nil { + continue + } + if allowed != nil && !allowed[name] { + continue + } + o.owner[name] = lower.source + o.entries = append(o.entries, e) + } + sort.Slice(o.entries, func(i, j int) bool { return o.entries[i].Name() < o.entries[j].Name() }) + return o +} + +// route picks the tree owning name by its top path segment via the +// composition-time manifest. whiteout is true when the manifest does not +// carry the skill (removed, filtered by Allow, or added after composition). +func (o *overlayFS) route(name string) (target fs.FS, whiteout bool) { + top := name + if i := strings.IndexByte(name, '/'); i >= 0 { + top = name[:i] + } + if t, ok := o.owner[top]; ok { + return t, false + } + return nil, true +} + +func (o *overlayFS) Open(name string) (fs.File, error) { + if !fs.ValidPath(name) { + return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrInvalid} + } + if name == "." { + entries, err := o.ReadDir(".") + if err != nil { + return nil, err + } + return &rootDir{entries: entries}, nil + } + target, whiteout := o.route(name) + if whiteout || target == nil { + return nil, &fs.PathError{Op: "open", Path: name, Err: fs.ErrNotExist} + } + return target.Open(name) +} + +func (o *overlayFS) Stat(name string) (fs.FileInfo, error) { + if !fs.ValidPath(name) { + return nil, &fs.PathError{Op: "stat", Path: name, Err: fs.ErrInvalid} + } + if name == "." { + return rootInfo{}, nil + } + target, whiteout := o.route(name) + if whiteout || target == nil { + return nil, &fs.PathError{Op: "stat", Path: name, Err: fs.ErrNotExist} + } + return fs.Stat(target, name) +} + +func (o *overlayFS) ReadFile(name string) ([]byte, error) { + if !fs.ValidPath(name) || name == "." { + return nil, &fs.PathError{Op: "readfile", Path: name, Err: fs.ErrInvalid} + } + target, whiteout := o.route(name) + if whiteout || target == nil { + return nil, &fs.PathError{Op: "readfile", Path: name, Err: fs.ErrNotExist} + } + return fs.ReadFile(target, name) +} + +func (o *overlayFS) ReadDir(name string) ([]fs.DirEntry, error) { + if !fs.ValidPath(name) { + return nil, &fs.PathError{Op: "readdir", Path: name, Err: fs.ErrInvalid} + } + if name != "." { + target, whiteout := o.route(name) + if whiteout || target == nil { + return nil, &fs.PathError{Op: "readdir", Path: name, Err: fs.ErrNotExist} + } + return fs.ReadDir(target, name) + } + return o.mergedRoot() +} + +// mergedRoot serves the composition-time manifest listing. +func (o *overlayFS) mergedRoot() ([]fs.DirEntry, error) { + out := make([]fs.DirEntry, len(o.entries)) + copy(out, o.entries) + return out, nil +} + +// rootInfo is the synthetic FileInfo for the overlay's "." directory, +// which has no single backing entry in either tree. +type rootInfo struct{} + +func (rootInfo) Name() string { return "." } +func (rootInfo) Size() int64 { return 0 } +func (rootInfo) Mode() fs.FileMode { return fs.ModeDir | 0o555 } +func (rootInfo) ModTime() time.Time { return time.Time{} } +func (rootInfo) IsDir() bool { return true } +func (rootInfo) Sys() any { return nil } + +// rootDir is the synthetic directory file returned by Open("."). +type rootDir struct { + entries []fs.DirEntry + off int +} + +func (d *rootDir) Stat() (fs.FileInfo, error) { return rootInfo{}, nil } +func (d *rootDir) Close() error { return nil } + +func (d *rootDir) Read([]byte) (int, error) { + return 0, &fs.PathError{Op: "read", Path: ".", Err: fs.ErrInvalid} +} + +func (d *rootDir) ReadDir(n int) ([]fs.DirEntry, error) { + if n <= 0 { + rest := d.entries[d.off:] + d.off = len(d.entries) + return rest, nil + } + if d.off >= len(d.entries) { + return nil, io.EOF + } + end := d.off + n + if end > len(d.entries) { + end = len(d.entries) + } + part := d.entries[d.off:end] + d.off = end + return part, nil +} diff --git a/internal/skillpolicy/resolver.go b/internal/skillpolicy/resolver.go new file mode 100644 index 000000000..a284ad4e1 --- /dev/null +++ b/internal/skillpolicy/resolver.go @@ -0,0 +1,170 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +// Package skillpolicy composes the CLI's effective embedded skill tree +// from a base skill FS and at most one plugin-supplied SkillsOverlay. It is +// the skill-side analogue of internal/cmdpolicy: plugins contribute a +// delta over a base, and one resolver produces the single tree that both +// skill readers consume -- `skills list`/`read` and the --help +// domain-guide pointer. +package skillpolicy + +import ( + "errors" + "fmt" + "io/fs" + "strings" + + "github.com/larksuite/cli/extension/platform" +) + +// PluginSkill pairs a plugin name with the SkillsOverlay it contributed, so a +// conflict can be attributed to specific owners. Mirrors +// cmdpolicy.PluginRule. +type PluginSkill struct { + PluginName string + SkillsOverlay *platform.SkillsOverlay +} + +// ErrMultipleSkillsOverlays reports that more than one plugin tried to +// customize skill content. Mirrors cmdpolicy.ErrMultipleRestricts: only +// one owner is allowed so independent plugins cannot silently overwrite +// each other's skill tree. +var ErrMultipleSkillsOverlays = errors.New("multiple plugins customized skills; only one plugin may own skill content") + +// Resolve composes the effective skill tree from base and the supplied +// specs. base is the CLI's embedded skill FS (nil when the build embeds +// none). With no spec it returns base unchanged. With exactly one spec it +// applies, in fixed order, Base override -> Allow -> Remove -> Overlay, +// returning an overlay FS in which a same-named skill resolves to +// Overlay. Two or more distinct owners is a configuration error. +func Resolve(base fs.FS, specs []PluginSkill) (fs.FS, error) { + owners := distinctOwners(specs) + if len(owners) > 1 { + return nil, fmt.Errorf("%w: %v", ErrMultipleSkillsOverlays, owners) + } + if len(specs) == 0 || specs[0].SkillsOverlay == nil { + return base, nil + } + + owner, spec := specs[0].PluginName, specs[0].SkillsOverlay + lower := base + if spec.Base != nil { + lower = spec.Base + } + lowerSnapshot, err := scanSkillTree("Base", lower) + if err != nil { + return nil, fmt.Errorf("plugin %q skill spec: %w", owner, err) + } + upperSnapshot, err := scanSkillTree("Overlay", spec.Overlay) + if err != nil { + return nil, fmt.Errorf("plugin %q skill spec: %w", owner, err) + } + if err := validateSelection(lowerSnapshot, spec); err != nil { + return nil, fmt.Errorf("plugin %q skill spec: %w", owner, err) + } + if lower == nil && spec.Overlay == nil { + return nil, nil + } + return newOverlayFS(lowerSnapshot, upperSnapshot, spec.Remove, spec.Allow), nil +} + +// distinctOwners returns the unique contributing plugin names in +// first-seen order. Mirrors cmdpolicy.distinctOwners. +func distinctOwners(specs []PluginSkill) []string { + seen := map[string]bool{} + owners := make([]string, 0, len(specs)) + for _, s := range specs { + if !seen[s.PluginName] { + seen[s.PluginName] = true + owners = append(owners, s.PluginName) + } + } + return owners +} + +type skillTreeSnapshot struct { + source fs.FS + entries []fs.DirEntry + names map[string]bool +} + +// scanSkillTree validates and snapshots a skill tree's top level in one +// pass. The returned entries are the exact entries used by the overlay +// manifest, so a mutable FS cannot swap unvalidated names in between +// validation and composition. +func scanSkillTree(label string, source fs.FS) (skillTreeSnapshot, error) { + snapshot := skillTreeSnapshot{source: source, names: map[string]bool{}} + if source == nil { + return snapshot, nil + } + entries, err := fs.ReadDir(source, ".") + if err != nil { + return snapshot, fmt.Errorf("%s: cannot read root: %w", label, err) + } + for _, e := range entries { + if !e.IsDir() { + return snapshot, fmt.Errorf("%s: %q is not a directory; every %s entry must be a / dir", label, e.Name(), label) + } + if !isSkillName(e.Name()) { + return snapshot, fmt.Errorf("%s: %q is not a valid skill name", label, e.Name()) + } + ok, err := skillExists(source, e.Name()) + if err != nil { + return snapshot, fmt.Errorf("%s: probing skill %q: %w", label, e.Name(), err) + } + if !ok { + return snapshot, fmt.Errorf("%s: skill %q is missing SKILL.md", label, e.Name()) + } + snapshot.names[e.Name()] = true + } + snapshot.entries = entries + return snapshot, nil +} + +// validateSelection rejects allow/remove entries that cannot compose against +// the already-validated base snapshot. +func validateSelection(lower skillTreeSnapshot, spec *platform.SkillsOverlay) error { + for _, name := range spec.Allow { + if !isSkillName(name) { + return fmt.Errorf("Allow: %q is not a valid skill name", name) + } + if !lower.names[name] { + return fmt.Errorf("Allow: skill %q is not in the base tree", name) + } + } + for _, name := range spec.Remove { + if !isSkillName(name) { + return fmt.Errorf("Remove: %q is not a valid skill name", name) + } + if !lower.names[name] { + return fmt.Errorf("Remove: skill %q is not in the base tree", name) + } + } + return nil +} + +// skillExists reports whether fsys holds a skill named name -- a +// directory carrying SKILL.md, the shape internal/skillcontent treats as +// a skill. +func skillExists(fsys fs.FS, name string) (bool, error) { + if fsys == nil { + return false, nil + } + info, err := fs.Stat(fsys, name+"/SKILL.md") + switch { + case err == nil: + return !info.IsDir(), nil + case errors.Is(err, fs.ErrNotExist) || errors.Is(err, fs.ErrInvalid): + return false, nil + default: + // A permission or I/O fault is a real cause, not absence. + return false, err + } +} + +// isSkillName rejects empty, dotted, or path-bearing names so Remove +// cannot smuggle a traversal or match outside the top level. +func isSkillName(name string) bool { + return name != "" && name != "." && name != ".." && !strings.ContainsAny(name, `/\`) +} diff --git a/internal/skillpolicy/resolver_test.go b/internal/skillpolicy/resolver_test.go new file mode 100644 index 000000000..bcd9e05e8 --- /dev/null +++ b/internal/skillpolicy/resolver_test.go @@ -0,0 +1,449 @@ +// Copyright (c) 2026 Lark Technologies Pte. Ltd. +// SPDX-License-Identifier: MIT + +package skillpolicy + +import ( + "errors" + "io/fs" + "slices" + "sort" + "strings" + "testing" + "testing/fstest" + + "github.com/larksuite/cli/extension/platform" +) + +// skillFS builds an in-memory skill tree; each map entry is a path -> content. +func skillFS(files map[string]string) fstest.MapFS { + m := fstest.MapFS{} + for p, content := range files { + m[p] = &fstest.MapFile{Data: []byte(content)} + } + return m +} + +// baseTree is a representative base: three skills, one with a reference file. +func baseTree() fstest.MapFS { + return skillFS(map[string]string{ + "lark-a/SKILL.md": "base a", + "lark-a/references/x.md": "base a ref", + "lark-b/SKILL.md": "base b", + "lark-shared/SKILL.md": "base shared", + }) +} + +// topLevel returns the sorted top-level skill names of fsys. +func topLevel(t *testing.T, fsys fs.FS) []string { + t.Helper() + entries, err := fs.ReadDir(fsys, ".") + if err != nil { + t.Fatalf("ReadDir(.): %v", err) + } + var names []string + for _, e := range entries { + names = append(names, e.Name()) + } + sort.Strings(names) + return names +} + +func readFile(t *testing.T, fsys fs.FS, name string) string { + t.Helper() + data, err := fs.ReadFile(fsys, name) + if err != nil { + t.Fatalf("ReadFile(%q): %v", name, err) + } + return string(data) +} + +func mustResolve(t *testing.T, base fs.FS, spec *platform.SkillsOverlay) fs.FS { + t.Helper() + got, err := Resolve(base, []PluginSkill{{PluginName: "acme", SkillsOverlay: spec}}) + if err != nil { + t.Fatalf("Resolve: unexpected error: %v", err) + } + return got +} + +func TestResolve_NoSpecs_ReturnsBaseUnchanged(t *testing.T) { + base := baseTree() + got, err := Resolve(base, nil) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + gotNames := topLevel(t, got) + if want := []string{"lark-a", "lark-b", "lark-shared"}; !slices.Equal(gotNames, want) { + t.Errorf("top level = %v, want %v", gotNames, want) + } +} + +func TestResolve_Remove_HidesSkill(t *testing.T) { + got := mustResolve(t, baseTree(), &platform.SkillsOverlay{Remove: []string{"lark-shared"}}) + + gotNames := topLevel(t, got) + if want := []string{"lark-a", "lark-b"}; !slices.Equal(gotNames, want) { + t.Errorf("top level = %v, want %v", gotNames, want) + } + // The removed skill is gone for the affordance reader too (Stat gates it). + if _, err := fs.Stat(got, "lark-shared/SKILL.md"); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("Stat removed skill: err = %v, want ErrNotExist", err) + } + // Surviving skills still read from base. + if c := readFile(t, got, "lark-a/SKILL.md"); c != "base a" { + t.Errorf("lark-a content = %q, want %q", c, "base a") + } +} + +func TestResolve_Overlay_AddsNewSkill(t *testing.T) { + overlay := skillFS(map[string]string{"lark-new/SKILL.md": "new skill"}) + got := mustResolve(t, baseTree(), &platform.SkillsOverlay{Overlay: overlay}) + + gotNames := topLevel(t, got) + if want := []string{"lark-a", "lark-b", "lark-new", "lark-shared"}; !slices.Equal(gotNames, want) { + t.Errorf("top level = %v, want %v", gotNames, want) + } + if c := readFile(t, got, "lark-new/SKILL.md"); c != "new skill" { + t.Errorf("lark-new content = %q, want %q", c, "new skill") + } +} + +func TestResolve_Overlay_ReplacesSameNameWholeSkill(t *testing.T) { + overlay := skillFS(map[string]string{"lark-a/SKILL.md": "overlaid a"}) + got := mustResolve(t, baseTree(), &platform.SkillsOverlay{Overlay: overlay}) + + // Same-named skill resolves to the overlay (upper wins). + if c := readFile(t, got, "lark-a/SKILL.md"); c != "overlaid a" { + t.Errorf("lark-a content = %q, want overlaid", c) + } + // Replacement is whole-skill: the base's reference file is shadowed, not merged. + if _, err := fs.Stat(got, "lark-a/references/x.md"); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("base reference should be shadowed by overlay skill; err = %v, want ErrNotExist", err) + } +} + +func TestResolve_Base_ReplacesEntireTree(t *testing.T) { + replacement := skillFS(map[string]string{"lark-only/SKILL.md": "only"}) + got := mustResolve(t, baseTree(), &platform.SkillsOverlay{Base: replacement}) + + gotNames := topLevel(t, got) + if want := []string{"lark-only"}; !slices.Equal(gotNames, want) { + t.Errorf("top level = %v, want %v", gotNames, want) + } + if _, err := fs.Stat(got, "lark-a/SKILL.md"); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("base skill should be gone after Base replace; err = %v", err) + } +} + +func TestResolve_Base_WithRemoveAndOverlay(t *testing.T) { + replacement := skillFS(map[string]string{ + "lark-p/SKILL.md": "p", + "lark-q/SKILL.md": "q", + }) + overlay := skillFS(map[string]string{"lark-r/SKILL.md": "r"}) + got := mustResolve(t, baseTree(), &platform.SkillsOverlay{ + Base: replacement, + Remove: []string{"lark-q"}, + Overlay: overlay, + }) + gotNames := topLevel(t, got) + if want := []string{"lark-p", "lark-r"}; !slices.Equal(gotNames, want) { + t.Errorf("top level = %v, want %v", gotNames, want) + } +} + +// Allow keeps only the listed base skills — the allow-list counterpart +// of Rule.Allow, so a CLI upgrade adding new embedded skills cannot +// widen an allow-listed build. +func TestResolve_Allow_KeepsOnlyListed(t *testing.T) { + got := mustResolve(t, baseTree(), &platform.SkillsOverlay{Allow: []string{"lark-a"}}) + + gotNames := topLevel(t, got) + if want := []string{"lark-a"}; !slices.Equal(gotNames, want) { + t.Errorf("top level = %v, want %v", gotNames, want) + } + if _, err := fs.Stat(got, "lark-b/SKILL.md"); !errors.Is(err, fs.ErrNotExist) { + t.Errorf("non-allow-listed skill must be absent; err = %v", err) + } + // Kept skills still read from base, references included. + if c := readFile(t, got, "lark-a/references/x.md"); c != "base a ref" { + t.Errorf("kept skill content = %q, want base content", c) + } +} + +// Remove wins over Allow, mirroring Rule's Deny-over-Allow. +func TestResolve_RemoveWinsOverAllow(t *testing.T) { + got := mustResolve(t, baseTree(), &platform.SkillsOverlay{ + Allow: []string{"lark-a", "lark-b"}, + Remove: []string{"lark-b"}, + }) + gotNames := topLevel(t, got) + if want := []string{"lark-a"}; !slices.Equal(gotNames, want) { + t.Errorf("top level = %v, want %v", gotNames, want) + } +} + +// Overlay entries are exempt from Allow: content the integrator +// explicitly ships needs no allow-listing. +func TestResolve_OverlayExemptFromAllow(t *testing.T) { + overlay := skillFS(map[string]string{"acme-guide/SKILL.md": "mine"}) + got := mustResolve(t, baseTree(), &platform.SkillsOverlay{ + Allow: []string{"lark-a"}, + Overlay: overlay, + }) + gotNames := topLevel(t, got) + if want := []string{"acme-guide", "lark-a"}; !slices.Equal(gotNames, want) { + t.Errorf("top level = %v, want %v", gotNames, want) + } +} + +// An Allow name absent from the base aborts startup, same as Remove. +func TestResolve_AllowUnknown_Errors(t *testing.T) { + _, err := Resolve(baseTree(), []PluginSkill{{PluginName: "acme", SkillsOverlay: &platform.SkillsOverlay{Allow: []string{"lark-nope"}}}}) + if err == nil { + t.Fatal("expected error allow-listing a skill absent from base") + } +} + +func TestResolve_RemoveUnknown_Errors(t *testing.T) { + _, err := Resolve(baseTree(), []PluginSkill{{PluginName: "acme", SkillsOverlay: &platform.SkillsOverlay{Remove: []string{"lark-nope"}}}}) + if err == nil { + t.Fatal("expected error removing a skill absent from base") + } +} + +func TestResolve_RemoveInvalidName_Errors(t *testing.T) { + for _, bad := range []string{"", ".", "..", "a/b", `a\b`} { + if _, err := Resolve(baseTree(), []PluginSkill{{PluginName: "acme", SkillsOverlay: &platform.SkillsOverlay{Remove: []string{bad}}}}); err == nil { + t.Errorf("Remove %q: expected error, got nil", bad) + } + } +} + +func TestResolve_OverlayMissingSKILLmd_Errors(t *testing.T) { + overlay := skillFS(map[string]string{"lark-bad/other.md": "no skill.md here"}) + _, err := Resolve(baseTree(), []PluginSkill{{PluginName: "acme", SkillsOverlay: &platform.SkillsOverlay{Overlay: overlay}}}) + if err == nil { + t.Fatal("expected error for overlay entry missing SKILL.md") + } +} + +func TestResolve_OverlayNonDirEntry_Errors(t *testing.T) { + overlay := skillFS(map[string]string{"loose.md": "top-level file, not a skill dir"}) + _, err := Resolve(baseTree(), []PluginSkill{{PluginName: "acme", SkillsOverlay: &platform.SkillsOverlay{Overlay: overlay}}}) + if err == nil { + t.Fatal("expected error for non-directory overlay entry") + } +} + +func TestResolve_TwoOwners_Errors(t *testing.T) { + _, err := Resolve(baseTree(), []PluginSkill{ + {PluginName: "acme", SkillsOverlay: &platform.SkillsOverlay{Remove: []string{"lark-a"}}}, + {PluginName: "globex", SkillsOverlay: &platform.SkillsOverlay{Remove: []string{"lark-b"}}}, + }) + if !errors.Is(err, ErrMultipleSkillsOverlays) { + t.Fatalf("err = %v, want ErrMultipleSkillsOverlays", err) + } +} + +// TestResolve_ComposedConformsToFS runs the composed tree through the +// standard io/fs conformance checker to catch Open/ReadDir/Stat drift. +func TestResolve_ComposedConformsToFS(t *testing.T) { + overlay := skillFS(map[string]string{ + "lark-new/SKILL.md": "new", + "lark-new/references/y.md": "new ref", + }) + got := mustResolve(t, baseTree(), &platform.SkillsOverlay{ + Remove: []string{"lark-shared"}, + Overlay: overlay, + }) + if err := fstest.TestFS(got, + "lark-a/SKILL.md", + "lark-a/references/x.md", + "lark-b/SKILL.md", + "lark-new/SKILL.md", + "lark-new/references/y.md", + ); err != nil { + t.Fatalf("composed FS failed fs conformance: %v", err) + } +} + +// brokenFS fails every Open, standing in for an integrator Base that points at +// a missing or unreadable tree. +type brokenFS struct{} + +func (brokenFS) Open(string) (fs.File, error) { return nil, fs.ErrNotExist } + +// A custom Base whose root cannot be read must fail at Resolve (startup), not +// surface later as an empty `skills list`. +func TestResolve_unreadableBaseFailsFast(t *testing.T) { + _, err := Resolve(baseTree(), []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{Base: brokenFS{}}, + }}) + if err == nil || !strings.Contains(err.Error(), "Base") { + t.Fatalf("unreadable Base must fail fast with a named error, got %v", err) + } +} + +// Overlay top-level directories are skill names and must pass the same name +// rule as Allow/Remove; a name the reader can never serve (backslash and +// friends) fails composition instead of being listed but unreadable. +func TestResolve_overlayInvalidSkillNameFailsFast(t *testing.T) { + overlay := skillFS(map[string]string{`bad\name/SKILL.md`: "x"}) + _, err := Resolve(baseTree(), []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{Overlay: overlay}, + }}) + if err == nil || !strings.Contains(err.Error(), "not a valid skill name") { + t.Fatalf("invalid overlay skill name must fail fast, got %v", err) + } +} + +// The allow-list is applied to the resolve-time manifest: a skill added to +// the base FS after composition stays invisible to both list and read. +func TestResolve_allowGatesSkillsAddedAfterResolve(t *testing.T) { + base := baseTree() + got, err := Resolve(base, []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{Allow: []string{"lark-a"}}, + }}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if want := []string{"lark-a"}; !slices.Equal(topLevel(t, got), want) { + t.Fatalf("top level = %v, want %v", topLevel(t, got), want) + } + + // Mutate the base afterwards, as a CLI upgrade (or a misbehaving + // integrator FS) would. + base["lark-late/SKILL.md"] = &fstest.MapFile{Data: []byte("late")} + + if names := topLevel(t, got); !slices.Equal(names, []string{"lark-a"}) { + t.Errorf("late-added skill leaked into the listing: %v", names) + } + if _, err := fs.ReadFile(got, "lark-late/SKILL.md"); err == nil { + t.Error("late-added skill must not be readable through the allow gate") + } +} + +// The manifest makes list and read agree under a mutated FS: names route by +// the composition-time snapshot, so a top-level skill added to EITHER tree +// afterwards neither lists nor reads, and ownership never flips. +func TestResolve_manifestKeepsListAndReadConsistent(t *testing.T) { + base := baseTree() + overlay := skillFS(map[string]string{"lark-a/SKILL.md": "upper a"}) + got, err := Resolve(base, []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{Overlay: overlay}, + }}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + + // Late additions to both trees stay invisible to list AND read. + base["lark-late/SKILL.md"] = &fstest.MapFile{Data: []byte("late lower")} + overlay["lark-upper-late/SKILL.md"] = &fstest.MapFile{Data: []byte("late upper")} + names := topLevel(t, got) + for _, n := range names { + if n == "lark-late" || n == "lark-upper-late" { + t.Errorf("late-added skill %q leaked into listing", n) + } + } + if _, err := fs.ReadFile(got, "lark-late/SKILL.md"); err == nil { + t.Error("late-added lower skill must not be readable") + } + if _, err := fs.ReadFile(got, "lark-upper-late/SKILL.md"); err == nil { + t.Error("late-added upper skill must not be readable") + } + + // Ownership stays with the snapshot: lark-a keeps reading from upper. + if body := readFile(t, got, "lark-a/SKILL.md"); body != "upper a" { + t.Errorf("lark-a = %q, want the overlay copy", body) + } + + // A late upper entry cannot steal a name the lower snapshot already owns. + overlay["lark-b/SKILL.md"] = &fstest.MapFile{Data: []byte("late upper b")} + if body := readFile(t, got, "lark-b/SKILL.md"); body != "base b" { + t.Errorf("lark-b = %q, want the original lower owner", body) + } + + // If the snapshotted upper owner disappears, reads fail from that owner; + // they must not silently fall back to a same-named lower skill. + delete(overlay, "lark-a/SKILL.md") + if body, err := fs.ReadFile(got, "lark-a/SKILL.md"); err == nil { + t.Errorf("deleted upper owner fell back to lower content %q", body) + } +} + +type oneShotRootFS struct { + fs.FS + rootReads int +} + +func (f *oneShotRootFS) ReadDir(name string) ([]fs.DirEntry, error) { + if name == "." { + f.rootReads++ + if f.rootReads > 1 { + return nil, errors.New("root directory read more than once") + } + } + return fs.ReadDir(f.FS, name) +} + +// Validation and manifest construction must consume the same root snapshot. +// A mutable FS may change between reads, so Resolve reads each tree's root +// exactly once and routes all later list/read operations through that snapshot. +func TestResolve_scansEachSkillTreeRootOnce(t *testing.T) { + base := &oneShotRootFS{FS: baseTree()} + overlay := &oneShotRootFS{FS: skillFS(map[string]string{ + "lark-new/SKILL.md": "new", + })} + + got, err := Resolve(base, []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{ + Remove: []string{"lark-shared"}, + Overlay: overlay, + }, + }}) + if err != nil { + t.Fatalf("Resolve: %v", err) + } + if base.rootReads != 1 || overlay.rootReads != 1 { + t.Fatalf("root reads = base:%d overlay:%d, want one each", base.rootReads, overlay.rootReads) + } + names := topLevel(t, got) + if want := []string{"lark-a", "lark-b", "lark-new"}; !slices.Equal(names, want) { + t.Fatalf("top level = %v, want %v", names, want) + } + if body := readFile(t, got, "lark-new/SKILL.md"); body != "new" { + t.Fatalf("lark-new = %q, want overlay content", body) + } +} + +// A replacement Base is held to the same shape rules as Overlay: stray files, +// invalid names, or a directory without SKILL.md fail at Resolve. +func TestResolve_baseValidatedLikeOverlay(t *testing.T) { + cases := []struct { + name string + base fstest.MapFS + want string + }{ + {"stray file", skillFS(map[string]string{"README.md": "x"}), "not a directory"}, + {"missing SKILL.md", fstest.MapFS{"lark-a/notes.txt": &fstest.MapFile{Data: []byte("x")}}, "missing SKILL.md"}, + {"invalid name", skillFS(map[string]string{`bad\name/SKILL.md`: "x"}), "not a valid skill name"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + _, err := Resolve(baseTree(), []PluginSkill{{ + PluginName: "acme", + SkillsOverlay: &platform.SkillsOverlay{Base: tc.base}, + }}) + if err == nil || !strings.Contains(err.Error(), tc.want) { + t.Fatalf("want error containing %q, got %v", tc.want, err) + } + }) + } +}