feat(extension): Plugin / Hook framework with command pruning#910
Open
sang-neo03 wants to merge 25 commits into
Open
feat(extension): Plugin / Hook framework with command pruning#910sang-neo03 wants to merge 25 commits into
sang-neo03 wants to merge 25 commits into
Conversation
Add a single public extension contract under extension/platform: integrators
implement the Plugin interface and register Observers, Wrappers, Lifecycle
handlers, and pruning Rules through the Registrar in one Install call.
Command pruning:
- Rule (Allow / Deny / MaxRisk / Identities) with doublestar globs
- 4-axis AND evaluation, parent-group aggregation, unknown-risk allow
- Sources: Plugin.Restrict (single-rule) and ~/.lark-cli/policy.yml
- Plugin path is fail-closed (envelope on rule error / multiple Restrict);
yaml path is fail-open (warning, CLI continues)
- strict-mode stubs now also write the denial annotation so the hook
layer's denial guard physically isolates Wrap chains on them
- HOME path never leaked through policy_source label
Hook framework:
- Observer (panic-safe, Before/After), Wrapper (middleware, may short-circuit
via AbortError), Lifecycle (Startup + Shutdown only)
- Recover guards every plugin entry point: Capabilities(), Install(),
Wrapper factory composition AND inner Handler, Lifecycle handlers
- namespacedWrap copies AbortError so a plugin's package-level sentinel
is never mutated across concurrent invocations
- Selector unknown-risk uniform: ByExactRisk / ByWrite / ByReadOnly never
match unannotated commands; safety-side hooks opt in via
ByWrite().Or(ByUnknownRisk())
Bootstrap orchestration (cmd/build.go + cmd/policy.go):
- InstallAll uses a staging Registrar + atomic commit
- FailClosed plugin install / Plugin.Restrict conflict / Startup handler
failure each install a structured envelope guard at every dispatch path
- walkGuard neutralises every cobra bypass we know of (PersistentPreRunE
first-wins, ValidateArgs, ParseFlags, legacyArgs, __complete /
__completeNoDesc, non-runnable groups, required-arg subcommands)
- cmd/root.go::Execute calls hook.Emit(Shutdown, runErr) after
rootCmd.Execute; isCompletionCommand skips both __complete and
__completeNoDesc so Tab completion never triggers Shutdown handlers
Capabilities consistency:
- Restricts=true must declare FailurePolicy=FailClosed
- RequiredCLIVersion (semver constraint) is validated against build.Version;
a malformed constraint is treated as untrusted-config and aborts
unconditionally, regardless of FailurePolicy (DEV builds included)
JSON envelope contract:
- error.type closed enum: pruning / strict_mode / hook / plugin_install /
plugin_conflict / plugin_lifecycle
- reason_code closed enums per type, all referenced by structured tests
Bootstrap surfaces (new user commands):
- lark-cli config policy show -- JSON view of the active Rule + source
- lark-cli config policy validate -- parse + schema + glob check, no apply
Coverage:
- extension/platform: every public type has a unit test
- internal/{pruning,hook,platformhost,policydecision,cmdmeta}: full coverage
of denial guard isolation, AbortError sentinel safety, observer panic
safety, lifecycle error/panic typing, staging atomic rollback
- cmd/plugin_integration_test.go: end-to-end through buildInternal with
synthetic and real command trees
- cmd/install_guard_test.go: walkGuard covers auth / config / __complete /
__completeNoDesc / non-runnable parents
The pruning denyStub and the strict-mode stub previously only swapped
RunE plus Hidden + DisableFlagParsing. Cobra's dispatch order means
several pre-RunE gates can fire BEFORE the stub's RunE ever runs:
1. Args validator: shortcut commands often declare cobra.NoArgs.
With DisableFlagParsing=true the user's `--doc xxx --mode append`
looks like positional args, so ValidateArgs surfaces a usage
error instead of the pruning / strict_mode envelope. Observer
hooks also miss the dispatch entirely.
2. Parent PersistentPreRunE: cmd/auth/auth.go declares a
PersistentPreRunE that returns external_provider when env
credentials are set. Cobra's "first PersistentPreRunE wins
walking up from the leaf" then short-circuits with
external_provider instead of the leaf's denial envelope.
Both stubs now also set:
- Args = cobra.ArbitraryArgs (bypass gate 1)
- PersistentPreRunE = no-op leaf hook (bypass gate 2)
- PreRunE / PreRun / PersistentPreRun = nil (defensive)
Effect: dispatch reaches the wrapped RunE, observers fire, the real
pruning / strict_mode envelope is emitted regardless of credential
provider or flag count.
Adds regression tests covering both gates on both stub paths.
cmd/config/config.go::NewCmdConfig declares a PersistentPreRunE that calls f.RequireBuiltinCredentialProvider; with env credentials set, it returns external_provider for every config subcommand. `config policy show` and `config policy validate` are READ-ONLY diagnostic commands -- they inspect or parse the user-layer rule without touching credentials. They MUST work regardless of which credential provider is active, otherwise users on env-credential deployments cannot debug their policy. Same shape as the codex C11/C13 fix: install a no-op leaf-level PersistentPreRunE on the `policy` group so cobra's "first walking up from leaf" rule picks ours over the config parent's. Regression caught by divergent e2e (F1-F6 all returned external_provider before this fix; all pass after). Adds a unit test pinning the PersistentPreRunE override.
RegisterShortcutsWithContext now calls cmdmeta.SetDomain on each
service-level cobra.Command (im, docs, drive, calendar, ...) so the
business-domain axis is actually populated on every shortcut leaf via
parent-chain inheritance.
Before this change, platform.ByDomain("docs") never matched any
command: the domain annotation was unset across the entire shortcut
tree, so the selector's d != "" guard always failed and risk-style
selectors silently degraded to no-op.
The SetDomain call is placed AFTER the create-or-reuse branch so it
fires whether the service command was freshly created here or had
already been added by cmd/service/service.go's OpenAPI auto-
registration (which runs first and creates im, drive, calendar, etc.).
Without this placement only pure-shortcut services like docs would
have been tagged.
Adds a regression test asserting:
- service-group cobra.Command carries the cmdmeta.domain annotation
- leaf shortcuts inherit the domain via parent-chain walk
…d their contributions
…lback When a user passed an unknown subcommand or shortcut (e.g. `lark-cli drive +bogus`), cobra returned `flag.ErrHelp` for the non-runnable group command, printed the parent help, and exited 0. AI agents couldn't distinguish a typo from an intentional help request. Install a tree-wide guard that attaches a RunE to every group command without its own Run/RunE. The RunE forwards no-args invocations to help (preserving prior behavior) and emits a structured unknown_subcommand ExitError (exit 2) listing available subcommands when args are present.
…denied
The envelope's `type` field was leaking implementation terms ("pruning",
"strict_mode") that describe enforcement mechanism rather than the user-
facing semantic. It also duplicated `detail.layer`, and forced consumers
to branch on two values for the same conceptual error ("a command was
denied by policy").
Collapse both into a single semantic type "command_denied". The
enforcement layer ("pruning" / "strict_mode") is preserved in
`detail.layer` so debugging and per-layer diagnostics still work.
…s active
The pruning engine used to treat any command without a risk annotation as
ALLOW even when a Rule with MaxRisk was set, and would silently skip the
MaxRisk comparison whenever the command's risk string was outside the
closed taxonomy. Both gaps let an unannotated or typo'd write command
slip past an "agent read-only" pruning rule.
Engine now denies before any other axis when a Rule is registered:
- reason_code "risk_not_annotated" for commands with no risk
- reason_code "risk_invalid" for commands whose risk is outside
the read | write | high-risk-write
taxonomy (e.g. typo "wrtie")
Main-flow is preserved: a nil Rule still returns Allowed=true
unconditionally, so a CLI with no pruning plugin behaves identically to
before. ByUnknownRisk() is removed from the public surface since the
Unknown state is no longer reachable through risk-based selectors when
any Rule is active; safety-side widening composition is no longer needed.
`config policy show`, `config policy validate`, and `config plugins show` are local-introspection-only commands kept behind the pruning diagnostic whitelist so operators can always inspect why a command was denied. They do not need to surface in `--help` for AI agents and were contributing to help noise. Hide the `policy` and `plugins` parent groups and both `show` / `validate` leaves. Commands remain callable by exact name and continue to bypass user-layer pruning via diagnosticPaths.
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an in-process plugin SDK, host installer, hook registry/installer, a policy resolver/engine that prunes commands with structured deny stubs, build-time guards for install/conflicts/lifecycle/unknown-subcommand/strict-mode, diagnostic commands, examples, and comprehensive tests. ChangesCLI plugin and policy system
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested labels
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests (beta)
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@1f9f75abd545dcef46611e29cecdf09009fcafc8🧩 Skill updatenpx skills add larksuite/cli#feat/extension-platform -y -g |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (4)
cmd/install_guard_test.go (1)
133-183: 💤 Low valueConsider removing unused
resultsarray or clarifying test intent.The
resultsarray is populated (line 166) but never inspected (line 182). The test comment at line 128 states "both observed errors must keep their own HookName," but the test only verifies the shared sentinel wasn't mutated (lines 179-180).If the test's scope is intentionally narrow (verifying sentinel immutability only), consider removing the
resultsarray and the loop capture logic (lines 156-168) to eliminate confusion. Otherwise, add assertions to verify each wrapper received a distinct namespaced HookName.♻️ Option 1: Remove unused results array (if narrow scope is intended)
- results := make([]string, 2) var wg sync.WaitGroup wg.Add(2) for i, m := range matched { - i, m := i, m + m := m go func() { defer wg.Done() - err := m.Fn(func(context.Context, *platform.Invocation) error { return nil })( + _ = m.Fn(func(context.Context, *platform.Invocation) error { return nil })( context.Background(), &platform.Invocation{}) - if ab, ok := err.(*platform.AbortError); ok { - results[i] = ab.HookName - } }() } wg.Wait() - // We are not using namespacedWrap directly here... + // The test verifies the sentinel AbortError is not mutated by concurrent wrappers. if shared.HookName != "plugin-shared-name" { t.Errorf("shared sentinel AbortError was mutated: HookName = %q", shared.HookName) } - _ = results🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/install_guard_test.go` around lines 133 - 183, TestNamespacedWrap_doesNotMutateSharedAbortError currently builds and writes into the results slice from each matched WrapperEntry.Fn but never inspects it, which is confusing; either remove the results slice and the goroutine capture/store logic (the results variable and its writes inside the goroutines) if you only intend to assert the shared AbortError sentinel wasn’t mutated, or keep results and add assertions after wg.Wait() to verify that each results[i] contains the expected namespaced HookName(s) (i.e., that matched wrappers returned distinct HookName values) to match the test comment and intent.extension/platform/abort_test.go (1)
14-28: 💤 Low valueConsider more explicit assertion for cause appending.
The test on lines 25-26 verifies that the Cause changes the message by checking inequality. While functionally correct, it would be clearer to assert that the message contains the cause's error text explicitly.
📝 Suggested improvement
- if got := withCause.Error(); got == bare.Error() { - t.Errorf("Cause should be appended to message, got %q", got) + got := withCause.Error() + if !strings.Contains(got, fs.ErrNotExist.Error()) { + t.Errorf("Error() should contain cause text, got %q", got) }You would also need to add
"strings"to the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@extension/platform/abort_test.go` around lines 14 - 28, Update TestAbortError_messageFormats to assert the cause text is explicitly appended instead of just checking inequality: replace the inequality check on withCause.Error() with an assertion that the returned string contains fs.ErrNotExist.Error() (or "file does not exist") and the expected base message from platform.AbortError for "audit.upload"; reference the test function TestAbortError_messageFormats and the withCause variable/struct, and add "strings" to the imports so you can use strings.Contains in the assertion.internal/platformhost/host.go (1)
59-71: ⚡ Quick winResolve plugin names once and reuse throughout install.
Plugin.Name()is invoked in bothdetectDuplicateNamesand the install loop. For stateful/buggy plugins, this can produce inconsistent names or second-call failures after precheck. Cache validated names from the prepass and reuse them for duplicate checks andinstallOne.Also applies to: 309-327
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platformhost/host.go` around lines 59 - 71, The code calls Plugin.Name() twice (in detectDuplicateNames and again in the install loop via safeCallName), which can fail or return inconsistent results for stateful plugins; change the prepass so detectDuplicateNames (or a new prepass function) validates and returns a cached mapping of plugin -> name (e.g., map[*Plugin]string or []string aligned with the plugins slice) and use that cached name map in the install loop passed into installOne instead of calling safeCallName again; update detectDuplicateNames/safeCallName to populate and return the cache and modify the install loop to look up the name from the cache when calling installOne and any other code paths that previously invoked Plugin.Name().cmd/config/policy_test.go (1)
18-25: 💤 Low valueConsider using
cmdutil.TestFactoryfor consistency.The manual factory construction works but deviates from the test guideline to use
cmdutil.TestFactory(t, config)for unit tests. While these lightweight diagnostic tests don't need the full factory setup, using the helper ensures consistency across the codebase.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/config/policy_test.go` around lines 18 - 25, The helper newPolicyTestFactory manually constructs a cmdutil.Factory and IOStreams; replace it with the standard test helper by using cmdutil.TestFactory in tests: update callers to call cmdutil.TestFactory(t, <config>) instead of newPolicyTestFactory and remove or replace newPolicyTestFactory with a wrapper that delegates to cmdutil.TestFactory(t, config) so tests use the consistent TestFactory helper rather than directly instantiating cmdutil.Factory/IOStreams.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/config/policy.go`:
- Around line 121-126: The function runConfigPolicyValidate reads the
user-supplied path directly with os.ReadFile; call validate.SafeInputPath on the
incoming path (handle its error and return an output.Errorf with ExitValidation
like the existing pattern), use the returned safePath when invoking os.ReadFile
instead of the raw path, and ensure any error messages include the original path
context; reference runConfigPolicyValidate, validate.SafeInputPath, and
os.ReadFile when making the change.
In `@extension/platform/register_test.go`:
- Around line 41-50: TestRegister_resetClears mutates package-global state but
doesn't register t.Cleanup, so if the test fails early the global registry
remains dirty; call t.Cleanup(platform.ResetForTesting) at the start of the test
(immediately after platform.ResetForTesting() or before any platform.Register
calls) to ensure platform.ResetForTesting() always runs on test teardown and
reference the existing symbols TestRegister_resetClears,
platform.ResetForTesting, platform.Register, and platform.RegisteredPlugins when
making the change.
In `@extension/platform/selector_test.go`:
- Around line 135-143: The test TestSelector_NilSafeWhenComposed currently never
executes the composed selector and thus doesn't verify behavior; update it to
actually call the composed selector returned by s.And(platform.All()) (and/or
s.Or(...)) and assert the documented nil-as-None semantics — i.e., ensure
composing a zero-value platform.Selector with platform.All() does not panic when
invoked and yields the expected boolean outcome; reference the platform.Selector
type and the And/Or and All methods to locate and update the test so it invokes
the resulting selector and asserts non-panic and correct result.
In `@extension/platform/selector.go`:
- Around line 93-110: The combinators And, Or, and Not call s(cmd) directly and
will panic if a nil Selector is used; update each to treat a nil Selector as
None() (i.e., a selector that always returns false): in And, if s==nil or
other==nil return a selector that always returns false; in Or, if s==nil return
other (which may be nil/None) and if other==nil return s; otherwise return the
composed OR; in Not, if s==nil return a selector that always returns true
(negation of None), else return the negating wrapper. Use the Selector type and
CommandView in the checks/wrappers so the behavior matches the documented
nil-as-None contract.
In `@internal/hook/install.go`:
- Around line 320-333: The comment incorrectly states the function is exported
for tests while the implementation is package-private: rename
populateInvocationDenial to PopulateInvocationDenial if you intend it to be
exported for tests (update all call sites and the package API), otherwise change
the comment to reflect it is internal-only and invoked indirectly (e.g., called
by the wrapper) so tests should use the wrapper; reference the function name
populateInvocationDenial when making the change so you update the correct
symbol.
In `@internal/hook/testing.go`:
- Around line 14-18: SetStderrForTesting currently mutates the package-level
stderr closure with no way to restore it; change its signature to return a
restore function (e.g., func() ) that captures the previous stderr value, set
stderr to wrap the provided io.Writer, and have the returned restore func reset
stderr back to the captured original; reference the SetStderrForTesting function
and the package-level stderr variable so callers/tests can call the returned
restore to avoid leaking global state.
In `@internal/plugininventory/inventory.go`:
- Around line 92-113: SetActive and GetActive currently only do a shallow copy
of Inventory (cp := *inv / *active) which leaves slices and nested structs
(e.g., Inventory.Plugins slice, each Plugin's Hooks slice, Rule internals)
sharing backing memory; update both functions to perform a deep copy: allocate
new slices for Inventory.Plugins and for each Plugin copy the struct value and
allocate/copy its Hooks slice and any pointer/complex fields, and similarly
deep-copy Rule or other nested fields so callers receive completely independent
copies; use the symbols SetActive, GetActive, Inventory, Plugins, Plugin (and
Hook/Rule types) to locate where to implement element-by-element copying and new
allocations.
In `@internal/pruning/yaml/schema.go`:
- Around line 42-48: Parse currently decodes only the first YAML document (in
function Parse, vars s and dec using gopkgyaml.NewDecoder and dec.Decode),
leaving trailing documents ignored; after the initial dec.Decode(&s) succeeds,
attempt to decode a second document into a dummy variable (e.g. var extra
interface{}) and treat any result other than io.EOF as an error—if the second
Decode returns nil (meaning another document exists) or an error other than
io.EOF, return a descriptive error (wrap with fmt.Errorf like the existing error
style) to reject inputs containing multiple YAML documents.
---
Nitpick comments:
In `@cmd/config/policy_test.go`:
- Around line 18-25: The helper newPolicyTestFactory manually constructs a
cmdutil.Factory and IOStreams; replace it with the standard test helper by using
cmdutil.TestFactory in tests: update callers to call cmdutil.TestFactory(t,
<config>) instead of newPolicyTestFactory and remove or replace
newPolicyTestFactory with a wrapper that delegates to cmdutil.TestFactory(t,
config) so tests use the consistent TestFactory helper rather than directly
instantiating cmdutil.Factory/IOStreams.
In `@cmd/install_guard_test.go`:
- Around line 133-183: TestNamespacedWrap_doesNotMutateSharedAbortError
currently builds and writes into the results slice from each matched
WrapperEntry.Fn but never inspects it, which is confusing; either remove the
results slice and the goroutine capture/store logic (the results variable and
its writes inside the goroutines) if you only intend to assert the shared
AbortError sentinel wasn’t mutated, or keep results and add assertions after
wg.Wait() to verify that each results[i] contains the expected namespaced
HookName(s) (i.e., that matched wrappers returned distinct HookName values) to
match the test comment and intent.
In `@extension/platform/abort_test.go`:
- Around line 14-28: Update TestAbortError_messageFormats to assert the cause
text is explicitly appended instead of just checking inequality: replace the
inequality check on withCause.Error() with an assertion that the returned string
contains fs.ErrNotExist.Error() (or "file does not exist") and the expected base
message from platform.AbortError for "audit.upload"; reference the test function
TestAbortError_messageFormats and the withCause variable/struct, and add
"strings" to the imports so you can use strings.Contains in the assertion.
In `@internal/platformhost/host.go`:
- Around line 59-71: The code calls Plugin.Name() twice (in detectDuplicateNames
and again in the install loop via safeCallName), which can fail or return
inconsistent results for stateful plugins; change the prepass so
detectDuplicateNames (or a new prepass function) validates and returns a cached
mapping of plugin -> name (e.g., map[*Plugin]string or []string aligned with the
plugins slice) and use that cached name map in the install loop passed into
installOne instead of calling safeCallName again; update
detectDuplicateNames/safeCallName to populate and return the cache and modify
the install loop to look up the name from the cache when calling installOne and
any other code paths that previously invoked Plugin.Name().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b28d09f-6813-420c-8dc2-37377303bc1c
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (79)
cmd/auth/login.gocmd/build.gocmd/config/config.gocmd/config/plugins.gocmd/config/policy.gocmd/config/policy_test.gocmd/global_flags_test.gocmd/install_guard_test.gocmd/plugin_integration_test.gocmd/policy.gocmd/policy_test.gocmd/prune.gocmd/prune_test.gocmd/root.gocmd/root_integration_test.gocmd/root_test.gocmd/unknown_subcommand_test.goextension/platform/abort.goextension/platform/abort_test.goextension/platform/capabilities.goextension/platform/doc.goextension/platform/errors.goextension/platform/handler.goextension/platform/invocation.goextension/platform/lifecycle.goextension/platform/plugin.goextension/platform/register.goextension/platform/register_test.goextension/platform/registrar.goextension/platform/rule.goextension/platform/selector.goextension/platform/selector_test.goextension/platform/types.goextension/platform/types_test.goextension/platform/view.gogo.modinternal/cmdmeta/meta.gointernal/cmdmeta/meta_test.gointernal/cmdutil/factory.gointernal/hook/doc.gointernal/hook/emit.gointernal/hook/emit_test.gointernal/hook/install.gointernal/hook/install_default.gointernal/hook/install_test.gointernal/hook/registry.gointernal/hook/testing.gointernal/hook/walk.gointernal/platformhost/doc.gointernal/platformhost/error.gointernal/platformhost/host.gointernal/platformhost/host_test.gointernal/platformhost/staging.gointernal/platformhost/version.gointernal/platformhost/version_test.gointernal/plugininventory/build.gointernal/plugininventory/build_test.gointernal/plugininventory/inventory.gointernal/policydecision/denial.gointernal/policydecision/denial_test.gointernal/pruning/active.gointernal/pruning/aggregation_test.gointernal/pruning/apply.gointernal/pruning/diagnostic.gointernal/pruning/diagnostic_test.gointernal/pruning/engine.gointernal/pruning/engine_test.gointernal/pruning/path.gointernal/pruning/resolver.gointernal/pruning/resolver_test.gointernal/pruning/source_label_test.gointernal/pruning/strict_mode_skip_test.gointernal/pruning/validate.gointernal/pruning/validate_test.gointernal/pruning/yaml/reader.gointernal/pruning/yaml/schema.gointernal/pruning/yaml/schema_test.goshortcuts/register.goshortcuts/register_test.go
…olicy yaml - selector.go: And/Or/Not now treat nil Selector as None() per godoc, preventing runtime panic when composed selectors are invoked. - schema.go: Parse rejects multi-document YAML input so a stray '---' separator can't silently drop trailing policy constraints.
Collaborator
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #910 +/- ##
==========================================
+ Coverage 65.92% 66.38% +0.46%
==========================================
Files 523 560 +37
Lines 49692 51543 +1851
==========================================
+ Hits 32758 34219 +1461
- Misses 14134 14457 +323
- Partials 2800 2867 +67 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (2)
cmd/config/plugins.go (1)
63-67: ⚡ Quick winKeep the JSON shape consistent when inventory is absent.
The success branch always emits
pluginsandtotal, but this branch dropstotal. Returning"total": 0here keeps stdout machine-friendly without forcing callers to special-case the bootstrap-not-finished path.♻️ Minimal change
if inv == nil { output.PrintJson(f.IOStreams.Out, map[string]any{ "plugins": []any{}, + "total": 0, "note": "no inventory recorded; bootstrap did not finish", }) return nil }As per coding guidelines, stdout is data (JSON envelopes), so the machine-readable payload should stay stable across branches.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/config/plugins.go` around lines 63 - 67, The nil-inventory branch in the function emitting plugin JSON uses output.PrintJson with "plugins" and "note" but omits "total", breaking the JSON shape; update that call (where inv == nil) to include "total": 0 so the output matches the success branch schema and keeps stdout machine-friendly (modify the map passed to output.PrintJson in cmd/config/plugins.go).internal/cmdpolicy/diagnostic_test.go (1)
17-31: ⚡ Quick winAdd
config/plugins/showto the EvaluateAll diagnostic-bypass test path.
TestIsDiagnosticPathcovers this path statically, but there’s no runtimeEvaluateAllassertion ensuring it still bypasses rule pruning like the other diagnostic commands.Suggested test addition
func configPolicyTree() *cobra.Command { root := &cobra.Command{Use: "lark-cli"} config := &cobra.Command{Use: "config"} root.AddCommand(config) policy := &cobra.Command{Use: "policy"} config.AddCommand(policy) policy.AddCommand(&cobra.Command{Use: "show", RunE: noop}) policy.AddCommand(&cobra.Command{Use: "validate", RunE: noop}) + plugins := &cobra.Command{Use: "plugins"} + config.AddCommand(plugins) + plugins.AddCommand(&cobra.Command{Use: "show", RunE: noop})if !got["config/policy/validate"].Allowed { t.Errorf("config/policy/validate must be unconditionally allowed; got Allowed=false reason=%q", got["config/policy/validate"].ReasonCode) } + if !got["config/plugins/show"].Allowed { + t.Errorf("config/plugins/show must be unconditionally allowed; got Allowed=false reason=%q", + got["config/plugins/show"].ReasonCode) + }Also applies to: 56-72
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmdpolicy/diagnostic_test.go` around lines 17 - 31, The EvaluateAll diagnostic-bypass test is missing the runtime path for "config/plugins/show" so it isn't asserted to bypass rule pruning; update the test that exercises EvaluateAll (and the configPolicyTree used by tests) to include a command node for "config plugins show" (mirror the static TestIsDiagnosticPath expectation) and add an EvaluateAll assertion that the path "config/plugins/show" is treated as a diagnostic path and therefore is not pruned by rules; locate configPolicyTree, TestIsDiagnosticPath and the EvaluateAll test helpers to add the new command and the corresponding runtime assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/build.go`:
- Around line 175-180: When applyUserPolicyPruning returns an error and we
choose to warn-and-continue (the path that calls warnPolicyError), clear the
globally cached active policy so a previous build's policy isn't reported as
active; update the error branch in cmd/build.go to call cmdpolicy.SetActive(nil)
(or the appropriate zero value via cmdpolicy.SetActive(...)) before calling
warnPolicyError. Ensure this change is applied in the same conditional that
handles the non-plugin (warn-and-continue) case so installation/return behavior
(installPluginConflictGuard + return) is unchanged.
In `@cmd/platform_bootstrap_test.go`:
- Around line 23-32: In the tmpHome helper, set the LARKSUITE_CLI_CONFIG_DIR
environment variable so tests don't pick up runner configs: inside the tmpHome(t
*testing.T) function (after creating dir or cfgDir) call
t.Setenv("LARKSUITE_CLI_CONFIG_DIR", <temp-dir>) — e.g. use the same temp dir or
a fresh t.TempDir() — so the config root is isolated for tests that build the
real command tree.
In `@cmd/platform_guards_test.go`:
- Around line 41-43: The tests call an undefined helper
platform.ResetForTesting(), so add an exported ResetForTesting function to the
platform package that clears/resets any global test state (e.g., plugin
registry) so subsequent platform.Register(failClosedAbortingPlugin{}) behaves
predictably; keep its signature as func ResetForTesting() so it can be called
directly and passed to t.Cleanup(platform.ResetForTesting) from the test. Ensure
ResetForTesting resets whatever platform.Register and the package-wide globals
rely on.
- Around line 79-90: The test selects a runnable leaf with walk/auth but always
calls leaf.RunE(...), which will panic if the command only defines Run (not
RunE); update the test to detect which handler exists for the selected command:
if leaf.RunE != nil call checkGuardError(t, leaf.RunE(leaf, nil)), otherwise if
leaf.Run != nil call checkGuardError(t, func(...) error { leaf.Run(leaf, nil);
return nil }(/* adapt call signature */)) — i.e. invoke Run when RunE is nil (or
wrap Run into an error-returning invocation) so the test never dereferences a
nil RunE. Ensure you reference the leaf variable and the Run/RunE methods when
making the change.
In `@cmd/plugin_integration_test.go`:
- Around line 104-105: Replace the undefined symbol platform.ResetForTesting
with the actual reset helper that exists in your test helpers: either import the
package that defines ResetForTesting and alias it to platform, or change the
call sites to the correct package name (for example testhelpers.ResetForTesting
or platformtest.ResetForTesting) so the symbol resolves; update all occurrences
(the direct calls and the t.Cleanup(...) registrations) consistently across the
file.
In `@internal/cmdpolicy/active.go`:
- Around line 40-53: The ActivePolicy copies currently only perform a shallow
copy so the embedded Rule remains aliased; update both places that do cp := *p
(the setter that assigns activePolicy) and cp := *activePolicy (GetActive) to
deep-copy the Rule field before assigning/returning. Concretely: after making
the shallow copy of ActivePolicy, create a new Rule value (or clone the Rule
slice/map if Rule is a collection, or instantiate a new Rule struct if Rule is a
pointer) and copy its contents into cp.Rule so cp.Rule is an independent copy;
do this in the setter (where activePolicy = &cp) and in GetActive() before
returning the cp pointer to prevent callers from mutating shared Rule state.
Ensure the deep-copy logic uses the actual Rule type semantics (allocate new
struct/slice/map and copy elements) so no references to the original Rule
remain.
In `@internal/cmdpolicy/apply.go`:
- Around line 63-65: The Apply function is over-counting because it
unconditionally increments count after calling installDenyStub even when that
helper returns early for strict-mode stubs; change the contract so
installDenyStub returns a boolean (or error) indicating whether a stub was
actually installed and only increment count when that return value is true
(update the two call sites in Apply that currently call installDenyStub and
increment count—also the equivalent loop around lines 135-148—to check the
returned flag before incrementing).
In `@internal/cmdpolicy/engine.go`:
- Around line 246-256: The early-return on no live children incorrectly ignores
a runnable node's own denied state; modify the block that currently checks
!liveChildSeen so that if cmdRunnable is true you consult denied[cmdPath] and
return that value (true if the command itself is denied) otherwise return false;
update the logic around the liveChildSeen check in the function (referencing
liveChildSeen, cmdRunnable, denied and cmdPath) so runnable nodes with only
placeholder branches correctly propagate their own denied status to parents.
In `@internal/cmdpolicy/resolver_test.go`:
- Around line 65-67: Replace the hardcoded absolute missing path in
TestResolve_missingYamlIsNoRule with a path guaranteed to not exist in the
test's temp directory: call t.TempDir(), then join a non-existent filename
(e.g., filepath.Join(t.TempDir(), "policy.yml")) and pass that to
cmdpolicy.Resolve; update references in the test function
TestResolve_missingYamlIsNoRule to use filepath.Join and ensure import of
"path/filepath" if missing.
In `@internal/hook/install_test.go`:
- Around line 197-199: The test currently calls
hook.SetStderrForTesting(&bytes.Buffer{}) and mutates global stderr without
restoring it; fix this by saving the previous stderr sink returned by
hook.SetStderrForTesting (e.g., prev := hook.SetStderrForTesting(...)) and
register a t.Cleanup(func() { hook.SetStderrForTesting(prev) }) so the original
sink is restored after the test, keeping the override scoped to the test.
In `@internal/hook/install.go`:
- Around line 45-58: Install currently guards root and reg but not snapshot;
because wrapRunE dereferences snapshot (CommandViewSource) you must check
snapshot for nil before wrapping commands — either return early from Install if
snapshot is nil or skip calling wrapRunE for each command when snapshot == nil;
update Install (which calls walkTree and wrapRunE) to perform this nil check and
avoid invoking wrapRunE with a nil snapshot.
- Around line 132-134: The wrapper-propagated context is being ignored when
calling the original handlers: update invokeOriginal (invoked from
finalHandler/composed) to set the command's context to the wrapper context
before calling originalRunE or originalRun (e.g., use c.SetContext(ctx) or
c.Context = ctx), then invoke originalRunE(c, args) / originalRun(c, args);
optionally restore the command's previous context after the call to avoid
side-effects.
In `@internal/platform/inventory.go`:
- Around line 212-225: SetActiveInventory and GetActiveInventory currently
perform a shallow copy of Inventory (cp := *inv / cp := *activeInventory),
leaving fields like Plugins and their nested slices/maps sharing backing memory;
change both to produce a deep copy of the Inventory struct by allocating a new
Inventory value, copying scalar fields, and for each slice/map field (notably
Inventory.Plugins and any nested slices/maps inside Plugin) allocate new
slices/maps and copy/clone each element/value so callers cannot mutate global
state; update uses of activeInventory/inventoryMu accordingly to set/get the
fully deep-copied value.
In `@internal/platform/staging.go`:
- Around line 128-139: The Restrict method on stagingRegistrar currently stores
the caller's *platform.Rule pointer directly (r.rule = rule), allowing external
mutation; instead, create a deep copy of the incoming rule (copy all scalar
fields and allocate+copy each slice/array field and any nested structs) and
assign that copy to r.rule; locate the stagingRegistrar.Restrict function and
replace the direct assignment with code that clones the Rule (including slice
fields like any []string, []RuleCondition, etc.) before storing it so the
registrar holds an independent snapshot.
In `@internal/platform/version.go`:
- Around line 124-127: The code currently truncates versions with more than
three numeric components (fields := strings.Split(s, ".") ... if len(fields) > 3
{ fields = fields[:3] }), which lets malformed RequiredCLIVersion values like
"1.2.3.4" pass; instead, detect len(fields) > 3 and return an error (or mark the
version invalid) indicating a malformed version string including the original s,
so parsing/validation fails closed; update the parsing function that uses
s/fields (the version parse/validate function handling RequiredCLIVersion) to
return that error and ensure callers propagate or handle it.
---
Nitpick comments:
In `@cmd/config/plugins.go`:
- Around line 63-67: The nil-inventory branch in the function emitting plugin
JSON uses output.PrintJson with "plugins" and "note" but omits "total", breaking
the JSON shape; update that call (where inv == nil) to include "total": 0 so the
output matches the success branch schema and keeps stdout machine-friendly
(modify the map passed to output.PrintJson in cmd/config/plugins.go).
In `@internal/cmdpolicy/diagnostic_test.go`:
- Around line 17-31: The EvaluateAll diagnostic-bypass test is missing the
runtime path for "config/plugins/show" so it isn't asserted to bypass rule
pruning; update the test that exercises EvaluateAll (and the configPolicyTree
used by tests) to include a command node for "config plugins show" (mirror the
static TestIsDiagnosticPath expectation) and add an EvaluateAll assertion that
the path "config/plugins/show" is treated as a diagnostic path and therefore is
not pruned by rules; locate configPolicyTree, TestIsDiagnosticPath and the
EvaluateAll test helpers to add the new command and the corresponding runtime
assertion.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d9983c38-0376-4d6c-8c3e-79f30bd08b04
📒 Files selected for processing (76)
.github/workflows/ci.ymlMakefilecmd/build.gocmd/config/plugins.gocmd/config/policy.gocmd/config/policy_test.gocmd/platform_bootstrap.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/platform_guards_test.gocmd/plugin_integration_test.gocmd/prune.gocmd/prune_test.gocmd/root_integration_test.goextension/platform/README.mdextension/platform/builder.goextension/platform/builder_test.goextension/platform/doc.goextension/platform/errors.goextension/platform/errors_test.goextension/platform/example_test.goextension/platform/examples/.gitignoreextension/platform/examples/README.mdextension/platform/examples/audit-observer/README.mdextension/platform/examples/audit-observer/main.goextension/platform/examples/readonly-policy/README.mdextension/platform/examples/readonly-policy/main.goextension/platform/handler.goextension/platform/identity.goextension/platform/invocation.goextension/platform/register.goextension/platform/register_testing.goextension/platform/risk.goextension/platform/risk_test.goextension/platform/rule.goextension/platform/selector.goextension/platform/selector_test.goextension/platform/view.gogo.modinternal/cmdmeta/meta.gointernal/cmdmeta/meta_test.gointernal/cmdpolicy/active.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/denial.gointernal/cmdpolicy/denial_test.gointernal/cmdpolicy/diagnostic.gointernal/cmdpolicy/diagnostic_test.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/engine_test.gointernal/cmdpolicy/path.gointernal/cmdpolicy/resolver.gointernal/cmdpolicy/resolver_test.gointernal/cmdpolicy/source_label_test.gointernal/cmdpolicy/strict_mode_skip_test.gointernal/cmdpolicy/suggest.gointernal/cmdpolicy/suggest_test.gointernal/cmdpolicy/validate.gointernal/cmdpolicy/validate_test.gointernal/cmdpolicy/yaml/reader.gointernal/cmdpolicy/yaml/schema.gointernal/cmdpolicy/yaml/schema_test.gointernal/hook/doc.gointernal/hook/install.gointernal/hook/install_test.gointernal/hook/invocation.gointernal/hook/registry.gointernal/platform/doc.gointernal/platform/error.gointernal/platform/host.gointernal/platform/host_test.gointernal/platform/inventory.gointernal/platform/inventory_test.gointernal/platform/staging.gointernal/platform/version.gointernal/platform/version_test.go
✅ Files skipped from review due to trivial changes (8)
- extension/platform/examples/audit-observer/README.md
- internal/cmdpolicy/yaml/reader.go
- extension/platform/examples/README.md
- extension/platform/README.md
- extension/platform/examples/readonly-policy/README.md
- internal/platform/doc.go
- extension/platform/doc.go
- go.mod
🚧 Files skipped from review as they are similar to previous changes (8)
- extension/platform/errors.go
- extension/platform/view.go
- extension/platform/handler.go
- extension/platform/selector_test.go
- cmd/config/policy_test.go
- extension/platform/selector.go
- internal/cmdmeta/meta_test.go
- internal/cmdmeta/meta.go
…uilder
Introduces extension/platform — the in-process plugin SDK external
Go forks of lark-cli use to extend or restrict the command surface.
Plugins compile in via blank import; there is no dynamic loading
and no RPC isolation.
Public SDK (extension/platform):
- Plugin interface (Name / Version / Capabilities / Install).
- Registrar verbs: Observe, Wrap, On, Restrict.
- Hook types: Observer (side-effect, panic-safe, fires Before/After
RunE), Wrapper (middleware, may short-circuit via AbortError),
LifecycleHandler (Startup / Shutdown), Selector with nil-safe
And/Or/Not composition.
- Risk / Identity are defined string types with closed taxonomies;
ParseRisk / ParseIdentity convert raw strings with the
absent-vs-invalid distinction the engine relies on.
- Builder ergonomic constructor (NewPlugin().Observer().Wrap()
...MustBuild()) that enforces name/hookName grammar, hookName
uniqueness, and the Restrict ↔ FailClosed pairing regardless of
call order.
- Invocation is a read-only interface; the framework's concrete
invocation type lives in internal/hook so plugins cannot
fabricate denial / strict-mode / identity state. Args() returns
a defensive copy on every call so hook mutation cannot leak
into the original RunE.
- CommandDeniedError + AbortError carry structured fields for the
closed `command_denied` / `hook` envelope contract.
- ResetForTesting gated behind //go:build testing.
- README + godoc examples (Observer / Wrapper / Restrict) + two
runnable example forks (audit-observer, readonly-policy).
Host (internal/platform, internal/hook, internal/cmdpolicy):
- InstallAll: staged plugin registration with atomic commit, panic
isolation, FailOpen / FailClosed semantics, RequiredCLIVersion
semver check, single-Restrict invariant, duplicate-plugin-name
detection.
- hook.Install wraps every runnable cmd.RunE with:
Before observers (panic-safe) → denial guard → composed Wrap
chain → original RunE → After observers (always fire, even on
err). Denied commands physically bypass the Wrap chain so a
plugin Wrapper cannot suppress or rewrite a denial; observers
still see the attempt for audit.
- Recover shim around plugin Wrappers converts panics (including
the factory call) into a structured `hook` envelope with
reason_code=panic; namespacing shim attributes AbortError to
the namespaced hook name.
- cmdpolicy (renamed from internal/pruning) is the user-layer
command policy engine: walks the cobra tree, evaluates each
runnable command against a Rule's four-axis filter (Allow /
Deny / MaxRisk / Identities), produces parent-group aggregate
denials, and installs denyStubs. Rule.AllowUnannotated opts out
of the unannotated-deny gate for gradual adoption; risk_invalid
typos always deny with an edit-distance "did you mean"
suggestion.
- Strict-mode stub in cmd/prune.go composes the shared
detail.* / wrapped CommandDeniedError shape via cmdpolicy
helpers (BuildDenialError / CommandDeniedFromDenial /
DenialDetailMap), so command_denied envelopes from strict-mode
and user-layer policy carry the same closed-enum fields
(detail.layer / reason_code / policy_source). The historical
short Message + independent Hint are preserved unchanged.
- cmdpolicy/yaml: structural parsing of ~/.lark-cli/policy.yml
with KnownFields strict mode, including allow_unannotated.
- `config policy show` / `config policy validate` and the plugin
inventory diagnostic surface the resolved Rule (allow,
deny, max_risk, identities, allow_unannotated) and the hook
contributions per plugin.
Envelope contract (docs/extension/reason-codes.md):
- error.type is a closed set: command_denied, hook, plugin_install,
plugin_conflict, plugin_lifecycle.
- reason_code is a closed enum per error.type, dispatched on by
external agents and CI integrations.
- detail.layer = "policy" | "strict_mode" attributes the rejection.
Build / CI:
- Makefile unit-test / vet / coverage and ci.yml fast-gate +
unit-test + coverage now pass -tags testing so register_testing.go
is visible; ./extension/... is in the package list so the SDK's
own tests actually run.
- fmt-check and examples-build Makefile targets.
- bmatcuk/doublestar/v4 added as a direct dependency for `**` glob
matching in Rule.Allow / Rule.Deny.
Author-facing material:
- docs/extension/ (quickstart, plugin-author-guide, reason-codes)
is provided in the working tree but kept out of git tracking
per repo convention (.gitignore covers docs/).
Change-Id: I3b8ecc2923bd54c2dff19e5dce8a0855a6f9e703
liangshuo-1
force-pushed
the
feat/extension-platform
branch
from
e72dcdd to
461e3c6
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cmd/config/policy.go (1)
100-100:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate untrusted policy paths and switch filesystem calls to
vfsLine 123 reads a CLI-supplied path directly, and Line 100/Line 123 use
os.*APIs. This bypasses required path validation and testable filesystem abstraction.Suggested fix
import ( "fmt" - "os" "github.com/spf13/cobra" "github.com/larksuite/cli/internal/cmdpolicy" pyaml "github.com/larksuite/cli/internal/cmdpolicy/yaml" "github.com/larksuite/cli/internal/cmdutil" "github.com/larksuite/cli/internal/output" + "github.com/larksuite/cli/internal/validate" + "github.com/larksuite/cli/internal/vfs" ) @@ if active.Source.Kind == cmdpolicy.SourcePlugin && active.YAMLPath != "" { - if _, err := os.Stat(active.YAMLPath); err == nil { + if _, err := vfs.Stat(active.YAMLPath); err == nil { out["yaml_shadowed"] = true fmt.Fprintln(f.IOStreams.ErrOut, "note: a plugin contributed Restrict(); yaml IGNORED") } } @@ func runConfigPolicyValidate(f *cmdutil.Factory, path string) error { - data, err := os.ReadFile(path) + safePath, err := validate.SafeInputPath(path) + if err != nil { + return output.Errorf(output.ExitValidation, "validation", + "invalid path %q: %v", path, err) + } + data, err := vfs.ReadFile(safePath) if err != nil { return output.Errorf(output.ExitValidation, "validation", "read policy yaml %q: %v", path, err) }As per coding guidelines: “Use
vfs.*instead ofos.*for all filesystem access” and “Validate paths before reading withvalidate.SafeInputPathbecause CLI arguments are untrusted.”Also applies to: 123-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/config/policy.go` at line 100, The code currently calls os.Stat and reads a CLI-supplied path via active.YAMLPath without validation; replace those os.* calls with the vfs equivalents (e.g., vfs.Stat, vfs.ReadFile) and validate the CLI path first using validate.SafeInputPath(active.YAMLPath) before any filesystem operation; update the conditional that checks the file existence (the os.Stat call) and the subsequent file read logic to use vfs and ensure you handle and return validation or vfs errors appropriately so the path is not used untrusted.internal/cmdpolicy/resolver_test.go (1)
65-67:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse a tempdir-based missing path for deterministic test behavior
Line 66 should avoid a hardcoded absolute path and use a guaranteed-missing file under
t.TempDir().Suggested fix
func TestResolve_missingYamlIsNoRule(t *testing.T) { - got, src, err := cmdpolicy.Resolve(nil, "/nonexistent/policy.yml") + yamlPath := filepath.Join(t.TempDir(), "missing-policy.yml") + got, src, err := cmdpolicy.Resolve(nil, yamlPath) if err != nil { t.Fatalf("missing yaml should not error, got %v", err) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cmdpolicy/resolver_test.go` around lines 65 - 67, TestResolve_missingYamlIsNoRule uses a hardcoded absolute path; change the test to build a guaranteed-missing path under t.TempDir() and pass that to cmdpolicy.Resolve. In the TestResolve_missingYamlIsNoRule function, call t.TempDir(), join a non-existing filename (e.g. filepath.Join(t.TempDir(), "policy.yml")) and use that path instead of "/nonexistent/policy.yml"; ensure you import "path/filepath" if needed.internal/platform/inventory.go (1)
205-225:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDeep-copy active inventory snapshots to prevent shared mutable state
Line 212 and Line 224 only clone the top-level struct. Nested slices/pointers remain shared, so callers can mutate global inventory outside lock protection.
Suggested fix
+func cloneInventory(in *Inventory) *Inventory { + if in == nil { + return nil + } + out := &Inventory{Plugins: make([]PluginEntry, len(in.Plugins))} + for i := range in.Plugins { + p := in.Plugins[i] + p.Observers = append([]HookEntry(nil), p.Observers...) + p.Wrappers = append([]HookEntry(nil), p.Wrappers...) + p.Lifecycles = append([]HookEntry(nil), p.Lifecycles...) + if p.Rule != nil { + r := *p.Rule + r.Allow = append([]string(nil), r.Allow...) + r.Deny = append([]string(nil), r.Deny...) + r.Identities = append([]string(nil), r.Identities...) + p.Rule = &r + } + out.Plugins[i] = p + } + return out +} + func SetActiveInventory(inv *Inventory) { inventoryMu.Lock() defer inventoryMu.Unlock() - if inv == nil { - activeInventory = nil - return - } - cp := *inv - activeInventory = &cp + activeInventory = cloneInventory(inv) } @@ func GetActiveInventory() *Inventory { inventoryMu.RLock() defer inventoryMu.RUnlock() - if activeInventory == nil { - return nil - } - cp := *activeInventory - return &cp + return cloneInventory(activeInventory) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/platform/inventory.go` around lines 205 - 225, SetActiveInventory and GetActiveInventory only make a shallow copy of Inventory, leaving nested slices/maps/pointers shared and mutable; update both functions (SetActiveInventory and GetActiveInventory) to perform a full deep copy of the Inventory contents (copy all nested slices, maps, and any pointer fields into new allocations) before assigning to activeInventory or returning it so callers never share internal mutable state with the global snapshot; ensure the deep-copy logic handles nils and preserves lengths/capacities and reuse or add a helper like deepCopyInventory(inv *Inventory) *Inventory to centralize the copy logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@cmd/config/policy.go`:
- Line 100: The code currently calls os.Stat and reads a CLI-supplied path via
active.YAMLPath without validation; replace those os.* calls with the vfs
equivalents (e.g., vfs.Stat, vfs.ReadFile) and validate the CLI path first using
validate.SafeInputPath(active.YAMLPath) before any filesystem operation; update
the conditional that checks the file existence (the os.Stat call) and the
subsequent file read logic to use vfs and ensure you handle and return
validation or vfs errors appropriately so the path is not used untrusted.
In `@internal/cmdpolicy/resolver_test.go`:
- Around line 65-67: TestResolve_missingYamlIsNoRule uses a hardcoded absolute
path; change the test to build a guaranteed-missing path under t.TempDir() and
pass that to cmdpolicy.Resolve. In the TestResolve_missingYamlIsNoRule function,
call t.TempDir(), join a non-existing filename (e.g. filepath.Join(t.TempDir(),
"policy.yml")) and use that path instead of "/nonexistent/policy.yml"; ensure
you import "path/filepath" if needed.
In `@internal/platform/inventory.go`:
- Around line 205-225: SetActiveInventory and GetActiveInventory only make a
shallow copy of Inventory, leaving nested slices/maps/pointers shared and
mutable; update both functions (SetActiveInventory and GetActiveInventory) to
perform a full deep copy of the Inventory contents (copy all nested slices,
maps, and any pointer fields into new allocations) before assigning to
activeInventory or returning it so callers never share internal mutable state
with the global snapshot; ensure the deep-copy logic handles nils and preserves
lengths/capacities and reuse or add a helper like deepCopyInventory(inv
*Inventory) *Inventory to centralize the copy logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fd45d8d-5cef-4924-86a0-c64cf77b801d
📒 Files selected for processing (75)
.github/workflows/ci.ymlMakefilecmd/build.gocmd/config/plugins.gocmd/config/policy.gocmd/config/policy_test.gocmd/platform_bootstrap.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/platform_guards_test.gocmd/plugin_integration_test.gocmd/prune.gocmd/prune_test.gocmd/root_integration_test.goextension/platform/README.mdextension/platform/builder.goextension/platform/builder_test.goextension/platform/doc.goextension/platform/errors.goextension/platform/errors_test.goextension/platform/example_test.goextension/platform/examples/.gitignoreextension/platform/examples/README.mdextension/platform/examples/audit-observer/README.mdextension/platform/examples/audit-observer/main.goextension/platform/examples/readonly-policy/README.mdextension/platform/examples/readonly-policy/main.goextension/platform/handler.goextension/platform/identity.goextension/platform/invocation.goextension/platform/register.goextension/platform/register_testing.goextension/platform/risk.goextension/platform/risk_test.goextension/platform/rule.goextension/platform/selector.goextension/platform/selector_test.goextension/platform/view.gointernal/cmdmeta/meta.gointernal/cmdmeta/meta_test.gointernal/cmdpolicy/active.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/denial.gointernal/cmdpolicy/denial_test.gointernal/cmdpolicy/diagnostic.gointernal/cmdpolicy/diagnostic_test.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/engine_test.gointernal/cmdpolicy/path.gointernal/cmdpolicy/resolver.gointernal/cmdpolicy/resolver_test.gointernal/cmdpolicy/source_label_test.gointernal/cmdpolicy/strict_mode_skip_test.gointernal/cmdpolicy/suggest.gointernal/cmdpolicy/suggest_test.gointernal/cmdpolicy/validate.gointernal/cmdpolicy/validate_test.gointernal/cmdpolicy/yaml/reader.gointernal/cmdpolicy/yaml/schema.gointernal/cmdpolicy/yaml/schema_test.gointernal/hook/doc.gointernal/hook/install.gointernal/hook/install_test.gointernal/hook/invocation.gointernal/hook/registry.gointernal/platform/doc.gointernal/platform/error.gointernal/platform/host.gointernal/platform/host_test.gointernal/platform/inventory.gointernal/platform/inventory_test.gointernal/platform/staging.gointernal/platform/version.gointernal/platform/version_test.go
✅ Files skipped from review due to trivial changes (7)
- extension/platform/examples/.gitignore
- extension/platform/README.md
- extension/platform/examples/README.md
- extension/platform/examples/audit-observer/README.md
- extension/platform/examples/readonly-policy/README.md
- extension/platform/doc.go
- internal/hook/doc.go
🚧 Files skipped from review as they are similar to previous changes (60)
- internal/platform/doc.go
- internal/cmdpolicy/yaml/reader.go
- extension/platform/errors_test.go
- internal/platform/error.go
- extension/platform/invocation.go
- extension/platform/view.go
- internal/cmdpolicy/diagnostic_test.go
- extension/platform/examples/audit-observer/main.go
- internal/cmdpolicy/suggest.go
- extension/platform/errors.go
- internal/cmdpolicy/path.go
- extension/platform/examples/readonly-policy/main.go
- internal/cmdmeta/meta.go
- internal/cmdpolicy/strict_mode_skip_test.go
- extension/platform/risk.go
- internal/platform/inventory_test.go
- internal/cmdpolicy/diagnostic.go
- extension/platform/example_test.go
- internal/hook/registry.go
- extension/platform/rule.go
- extension/platform/identity.go
- cmd/config/policy_test.go
- internal/cmdpolicy/validate.go
- extension/platform/register_testing.go
- cmd/prune_test.go
- internal/cmdpolicy/yaml/schema.go
- internal/cmdpolicy/aggregation_test.go
- internal/platform/staging.go
- extension/platform/builder_test.go
- cmd/root_integration_test.go
- internal/cmdpolicy/active.go
- cmd/config/plugins.go
- internal/cmdpolicy/suggest_test.go
- internal/cmdpolicy/source_label_test.go
- extension/platform/selector.go
- internal/cmdpolicy/validate_test.go
- extension/platform/selector_test.go
- cmd/plugin_integration_test.go
- internal/cmdpolicy/engine.go
- cmd/platform_guards_test.go
- internal/cmdpolicy/yaml/schema_test.go
- internal/cmdmeta/meta_test.go
- cmd/platform_bootstrap_test.go
- internal/platform/version_test.go
- extension/platform/handler.go
- internal/platform/version.go
- extension/platform/builder.go
- cmd/build.go
- internal/cmdpolicy/denial_test.go
- extension/platform/risk_test.go
- internal/cmdpolicy/apply.go
- internal/cmdpolicy/denial.go
- cmd/prune.go
- internal/platform/host.go
- extension/platform/register.go
- internal/hook/install.go
- internal/hook/install_test.go
- internal/cmdpolicy/engine_test.go
- internal/cmdpolicy/resolver.go
- internal/platform/host_test.go
…uilder
Introduces extension/platform — the in-process plugin SDK external
Go forks of lark-cli use to extend or restrict the command surface.
Plugins compile in via blank import; there is no dynamic loading
and no RPC isolation.
Public SDK (extension/platform):
- Plugin interface (Name / Version / Capabilities / Install).
- Registrar verbs: Observe, Wrap, On, Restrict.
- Hook types: Observer (side-effect, panic-safe, fires Before/After
RunE), Wrapper (middleware, may short-circuit via AbortError),
LifecycleHandler (Startup / Shutdown), Selector with nil-safe
And/Or/Not composition.
- Risk / Identity are defined string types with closed taxonomies;
ParseRisk / ParseIdentity convert raw strings with the
absent-vs-invalid distinction the engine relies on.
- Builder ergonomic constructor (NewPlugin().Observer().Wrap()
...MustBuild()) that enforces name/hookName grammar, hookName
uniqueness, and the Restrict ↔ FailClosed pairing regardless of
call order.
- Invocation is a read-only interface; the framework's concrete
invocation type lives in internal/hook so plugins cannot
fabricate denial / strict-mode / identity state. Args() returns
a defensive copy on every call so hook mutation cannot leak
into the original RunE.
- CommandDeniedError + AbortError carry structured fields for the
closed `command_denied` / `hook` envelope contract.
- ResetForTesting gated behind //go:build testing.
- README + godoc examples (Observer / Wrapper / Restrict) + two
runnable example forks (audit-observer, readonly-policy).
Host (internal/platform, internal/hook, internal/cmdpolicy):
- InstallAll: staged plugin registration with atomic commit, panic
isolation, FailOpen / FailClosed semantics, RequiredCLIVersion
semver check, single-Restrict invariant, duplicate-plugin-name
detection.
- hook.Install wraps every runnable cmd.RunE with:
Before observers (panic-safe) → denial guard → composed Wrap
chain → original RunE → After observers (always fire, even on
err). Denied commands physically bypass the Wrap chain so a
plugin Wrapper cannot suppress or rewrite a denial; observers
still see the attempt for audit.
- Recover shim around plugin Wrappers converts panics (including
the factory call) into a structured `hook` envelope with
reason_code=panic; namespacing shim attributes AbortError to
the namespaced hook name.
- cmdpolicy (renamed from internal/pruning) is the user-layer
command policy engine: walks the cobra tree, evaluates each
runnable command against a Rule's four-axis filter (Allow /
Deny / MaxRisk / Identities), produces parent-group aggregate
denials, and installs denyStubs. Rule.AllowUnannotated opts out
of the unannotated-deny gate for gradual adoption; risk_invalid
typos always deny with an edit-distance "did you mean"
suggestion.
- Strict-mode stub in cmd/prune.go composes the shared
detail.* / wrapped CommandDeniedError shape via cmdpolicy
helpers (BuildDenialError / CommandDeniedFromDenial /
DenialDetailMap), so command_denied envelopes from strict-mode
and user-layer policy carry the same closed-enum fields
(detail.layer / reason_code / policy_source). The historical
short Message + independent Hint are preserved unchanged.
- cmdpolicy/yaml: structural parsing of ~/.lark-cli/policy.yml
with KnownFields strict mode, including allow_unannotated.
- `config policy show` / `config policy validate` and the plugin
inventory diagnostic surface the resolved Rule (allow,
deny, max_risk, identities, allow_unannotated) and the hook
contributions per plugin.
Envelope contract (docs/extension/reason-codes.md):
- error.type is a closed set: command_denied, hook, plugin_install,
plugin_conflict, plugin_lifecycle.
- reason_code is a closed enum per error.type, dispatched on by
external agents and CI integrations.
- detail.layer = "policy" | "strict_mode" attributes the rejection.
Build / CI:
- Makefile unit-test / vet / coverage and ci.yml fast-gate +
unit-test + coverage now pass -tags testing so register_testing.go
is visible; ./extension/... is in the package list so the SDK's
own tests actually run.
- fmt-check and examples-build Makefile targets.
- bmatcuk/doublestar/v4 added as a direct dependency for `**` glob
matching in Rule.Allow / Rule.Deny.
Author-facing material:
- docs/extension/ (quickstart, plugin-author-guide, reason-codes)
is provided in the working tree but kept out of git tracking
per repo convention (.gitignore covers docs/).
Change-Id: I3b8ecc2923bd54c2dff19e5dce8a0855a6f9e703
liangshuo-1
force-pushed
the
feat/extension-platform
branch
from
461e3c6 to
4ff5ad2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/root_integration_test.go (1)
436-439: ⚡ Quick winUse the shared strict-mode constants in the remaining envelope assertions.
Line 437/438, Line 456/457, and Line 501/502 still hardcode strings even though canonical constants were introduced at Line 33-Line 35. Reusing them avoids fixture drift.
Proposed cleanup
- Message: `strict mode is "user", only user-identity commands are available`, - Hint: "if the user explicitly wants to switch policy, see `lark-cli config strict-mode --help` (confirm with the user before switching; switching does NOT require re-bind)", + Message: strictModeUserMessage, + Hint: strictModeHint,- Message: `strict mode is "bot", only bot-identity commands are available`, - Hint: "if the user explicitly wants to switch policy, see `lark-cli config strict-mode --help` (confirm with the user before switching; switching does NOT require re-bind)", + Message: strictModeBotMessage, + Hint: strictModeHint,- Message: `strict mode is "bot", only bot-identity commands are available`, - Hint: "if the user explicitly wants to switch policy, see `lark-cli config strict-mode --help` (confirm with the user before switching; switching does NOT require re-bind)", + Message: strictModeBotMessage, + Hint: strictModeHint,Also applies to: 455-458, 500-503
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmd/root_integration_test.go` around lines 436 - 439, Replace the hardcoded strict-mode strings in the envelope assertions (the Type/Message/Hint fields that currently use "command_denied" and the message `strict mode is "user", only user-identity commands are available` and its hint) with the canonical strict-mode constants introduced earlier in the file near the top (the shared strict-mode constants added around lines 33–35); update all three assertion blocks (the ones around the shown diff plus the blocks at the other two locations) to reference those package-level constants instead of literal strings so the fixtures stay in sync.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmd/config/plugins.go`:
- Around line 63-67: When inventory is nil, the JSON response omits the "total"
field causing clients to branch; update the nil branch that calls
output.PrintJson(f.IOStreams.Out, ...) so the map includes "total": 0 (matching
the normal branch schema) alongside "plugins" and "note" — locate the
conditional handling around inv == nil in plugins.go (the block that calls
output.PrintJson) and add the "total" key to the returned map.
---
Nitpick comments:
In `@cmd/root_integration_test.go`:
- Around line 436-439: Replace the hardcoded strict-mode strings in the envelope
assertions (the Type/Message/Hint fields that currently use "command_denied" and
the message `strict mode is "user", only user-identity commands are available`
and its hint) with the canonical strict-mode constants introduced earlier in the
file near the top (the shared strict-mode constants added around lines 33–35);
update all three assertion blocks (the ones around the shown diff plus the
blocks at the other two locations) to reference those package-level constants
instead of literal strings so the fixtures stay in sync.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72a35f0a-4393-4ed5-8898-fc95adfdfe46
📒 Files selected for processing (76)
.github/workflows/ci.ymlMakefilecmd/build.gocmd/config/plugins.gocmd/config/policy.gocmd/config/policy_test.gocmd/platform_bootstrap.gocmd/platform_bootstrap_test.gocmd/platform_guards.gocmd/platform_guards_test.gocmd/plugin_integration_test.gocmd/prune.gocmd/prune_test.gocmd/root_integration_test.goextension/platform/README.mdextension/platform/builder.goextension/platform/builder_test.goextension/platform/doc.goextension/platform/errors.goextension/platform/errors_test.goextension/platform/example_test.goextension/platform/examples/.gitignoreextension/platform/examples/README.mdextension/platform/examples/audit-observer/README.mdextension/platform/examples/audit-observer/main.goextension/platform/examples/readonly-policy/README.mdextension/platform/examples/readonly-policy/main.goextension/platform/handler.goextension/platform/identity.goextension/platform/invocation.goextension/platform/register.goextension/platform/register_testing.goextension/platform/risk.goextension/platform/risk_test.goextension/platform/rule.goextension/platform/selector.goextension/platform/selector_test.goextension/platform/view.gointernal/cmdmeta/meta.gointernal/cmdmeta/meta_test.gointernal/cmdpolicy/active.gointernal/cmdpolicy/aggregation_test.gointernal/cmdpolicy/apply.gointernal/cmdpolicy/denial.gointernal/cmdpolicy/denial_test.gointernal/cmdpolicy/diagnostic.gointernal/cmdpolicy/diagnostic_test.gointernal/cmdpolicy/engine.gointernal/cmdpolicy/engine_test.gointernal/cmdpolicy/path.gointernal/cmdpolicy/resolver.gointernal/cmdpolicy/resolver_test.gointernal/cmdpolicy/source_label_test.gointernal/cmdpolicy/strict_mode_skip_test.gointernal/cmdpolicy/suggest.gointernal/cmdpolicy/suggest_test.gointernal/cmdpolicy/validate.gointernal/cmdpolicy/validate_test.gointernal/cmdpolicy/yaml/reader.gointernal/cmdpolicy/yaml/schema.gointernal/cmdpolicy/yaml/schema_test.gointernal/hook/doc.gointernal/hook/install.gointernal/hook/install_default.gointernal/hook/install_test.gointernal/hook/invocation.gointernal/hook/registry.gointernal/platform/doc.gointernal/platform/error.gointernal/platform/host.gointernal/platform/host_test.gointernal/platform/inventory.gointernal/platform/inventory_test.gointernal/platform/staging.gointernal/platform/version.gointernal/platform/version_test.go
✅ Files skipped from review due to trivial changes (10)
- extension/platform/examples/readonly-policy/main.go
- extension/platform/examples/.gitignore
- internal/hook/install_default.go
- extension/platform/examples/README.md
- extension/platform/examples/readonly-policy/README.md
- extension/platform/examples/audit-observer/README.md
- internal/hook/doc.go
- extension/platform/README.md
- extension/platform/doc.go
- internal/platform/doc.go
🚧 Files skipped from review as they are similar to previous changes (63)
- extension/platform/invocation.go
- extension/platform/view.go
- extension/platform/register_testing.go
- extension/platform/errors.go
- extension/platform/handler.go
- extension/platform/identity.go
- internal/cmdpolicy/diagnostic_test.go
- internal/platform/inventory_test.go
- internal/cmdpolicy/suggest.go
- internal/cmdpolicy/suggest_test.go
- internal/cmdpolicy/validate.go
- cmd/prune.go
- internal/cmdpolicy/strict_mode_skip_test.go
- internal/cmdmeta/meta_test.go
- extension/platform/examples/audit-observer/main.go
- internal/cmdpolicy/validate_test.go
- .github/workflows/ci.yml
- internal/hook/registry.go
- cmd/prune_test.go
- extension/platform/errors_test.go
- internal/platform/error.go
- internal/cmdpolicy/active.go
- internal/cmdpolicy/aggregation_test.go
- cmd/config/policy.go
- internal/cmdpolicy/yaml/reader.go
- extension/platform/risk_test.go
- internal/platform/version_test.go
- internal/cmdpolicy/denial_test.go
- internal/cmdpolicy/denial.go
- internal/cmdpolicy/diagnostic.go
- extension/platform/builder_test.go
- internal/cmdpolicy/resolver_test.go
- extension/platform/rule.go
- extension/platform/register.go
- internal/platform/version.go
- cmd/config/policy_test.go
- extension/platform/selector_test.go
- internal/cmdpolicy/resolver.go
- internal/cmdpolicy/yaml/schema.go
- internal/hook/invocation.go
- extension/platform/example_test.go
- internal/platform/inventory.go
- extension/platform/selector.go
- internal/cmdpolicy/source_label_test.go
- extension/platform/risk.go
- cmd/platform_bootstrap.go
- internal/cmdpolicy/path.go
- internal/cmdpolicy/yaml/schema_test.go
- cmd/build.go
- internal/cmdpolicy/engine.go
- internal/platform/host.go
- cmd/platform_guards_test.go
- cmd/platform_guards.go
- internal/cmdpolicy/apply.go
- internal/cmdmeta/meta.go
- internal/platform/staging.go
- cmd/platform_bootstrap_test.go
- internal/hook/install_test.go
- internal/cmdpolicy/engine_test.go
- internal/hook/install.go
- cmd/plugin_integration_test.go
- internal/platform/host_test.go
- extension/platform/builder.go
…at/extension-platform # Conflicts: # cmd/config/plugins.go # cmd/platform_guards_test.go # cmd/plugin_integration_test.go # internal/platform/host_test.go
- cmdpolicy: skip AnnotationPureGroup commands in EvaluateAll, aggregateParents, and hasRunnableDescendant so user-layer policy no longer blocks `<group> --help` after the unknown-subcommand guard attaches RunE to every parent - cmd/root: tag guarded parent groups with AnnotationPureGroup - extension/platform: drop `//go:build testing` from register_testing.go so `go test ./...` works without an extra build tag - extension/platform/README: inline reason_code reference, fix plugin lifecycle diagram order (init/Register precede RegisteredPlugins) - cmd/platform_bootstrap: route userPolicyPath through core.GetBaseConfigDir so LARKSUITE_CLI_CONFIG_DIR is honoured - cmdpolicy: add RedactHomeDir helper, fold base config dir and $HOME prefixes for config policy show + resolver errors - internal/platform: reject unrecognised FailurePolicy values with invalid_capability instead of silently fail-open - cmd/config: surface diagnostic policy/plugins commands in `config --help` Long text - CHANGELOG: document command_denied error.type rename and unknown_subcommand exit-2 behavior change
- hook/install: propagate wrapper-injected ctx to invokeOriginal so RunE/Run see context values added by upstream Wrappers - hook/testing: SetStderrForTesting returns a restore func; tests now defer it via t.Cleanup to avoid cross-test sink leakage - cmdpolicy/active: deep-copy ActivePolicy.Rule on SetActive/GetActive so callers can't mutate the stored global through shared slices - platform/inventory: deep-copy Inventory + nested Plugins / HookEntry / RuleView slices on SetActiveInventory / GetActiveInventory - platform/staging: Restrict clones the plugin-supplied Rule before retaining it so the plugin can't mutate it after Install returns - platform/version: reject RequiredCLIVersion with more than three numeric components instead of silently truncating 1.2.3.4 to 1.2.3 - cmd/platform_bootstrap: clear cmdpolicy.SetActive on yaml resolver error so config policy show doesn't surface a stale rule - cmd/platform_bootstrap_test: tmpHome pins LARKSUITE_CLI_CONFIG_DIR so host env can't bleed into the policy test fixtures - cmdpolicy/apply: installDenyStub returns bool; Apply count no longer over-reports when strict-mode short-circuits the install - cmdpolicy/engine: aggregateParents now returns the runnable hybrid's own denial status when all children are placeholder branches - cmdpolicy/resolver_test: use t.TempDir()-rooted missing path instead of hardcoded /nonexistent for hermetic missing-file assertion - cmd/config/plugins: empty-inventory branch emits total: 0 so the JSON schema stays stable across populated/empty cases - cmd/platform_guards_test: select leaf by RunE != nil (not Runnable) so the test doesn't nil-deref on Run-only commands - gofmt run on previously committed cmdpolicy/path*.go (CI fast-gate)
The depguard / forbidigo rule blocks filepath.Abs in internal/ on the grounds that it accesses the filesystem (Getwd) directly. Switch RedactHomeDir + foldPrefix to operate on filepath.Clean strings; real callers pass already-absolute paths (resolver builds yamlPath via filepath.Join on the absolute config root), so the redaction outcome is unchanged for production inputs. Relative inputs fall through to the unchanged branch — filepath.Rel rejects the mixed-absoluteness case with an error, which the foldPrefix helper already treats as "not a hit".
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Introduce a single public extension contract under
extension/platformso integrators can register Observers, Wrappers, Lifecycle handlers, and pruning Rules through oneInstallcall. Adds a fail-closed command-pruning pipeline (plugin Rule +~/.lark-cli/policy.yml) layered on top of a panic-safe hook framework, with structured JSON envelopes for every denial / install / lifecycle failure so AI agents get a stable protocol signal instead of cobra's silent help fallback.Changes
extension/platform/(public contract)Plugininterface +Registrar(Observer / Wrapper / Lifecycle / Restrict)Rule(Allow / Deny / MaxRisk / Identities) with doublestar globs, 4-axis AND evaluationByDomain,ByExactRisk,ByWrite,ByReadOnly; unknown-risk is uniform — read-side selectors never match unannotated commandsAbortError+Capabilities{Restricts, FailurePolicy, RequiredCLIVersion}FailurePolicyenum is validated at install time — unrecognised values are rejected withinvalid_capability(always-abort) instead of silently fail-openResetForTestingexported as a plain helper (no//go:build testing) sogo test ./...works without an extra build tagPolicy engine (
internal/cmdpolicy/)Restrict); yaml path is fail-open (warning, CLI continues)riskannotation or carrying a risk outside the closed taxonomy (read | write | high-risk-write) are denied withreason_code = risk_not_annotated | risk_invalid— closes the gap where an unannotated or typo'd write command could slip past an agent read-only ruleAnnotationPureGroupmarker isolates the unknown-subcommand guard from the engine: parent groups that only carry the guard-injected RunE are skipped inEvaluateAll,aggregateParents, andhasRunnableDescendant, so a user policy withmax_risk: readno longer denies<group> --helpArgs(→ArbitraryArgs) and install a no-op leafPersistentPreRunE, so the wrapped RunE is reached regardless of cobraValidateArgs/ parent-PersistentPreRunE orderRedactHomeDirfolds both the base config dir (core.GetBaseConfigDir(), which honoursLARKSUITE_CLI_CONFIG_DIR) and$HOMEsoconfig policy showoutput and resolver error messages stop leaking absolute filesystem paths to AI agents / CI logsHook framework (
internal/hook/)AbortError), Lifecycle (Startup + Shutdown)recoverguards every plugin entry point:Capabilities(),Install(), Wrapper factory composition AND inner Handler, Lifecycle handlersnamespacedWrapcopiesAbortErrorso a plugin's package-level sentinel is never mutated across concurrent invocationsBootstrap (
cmd/build.go,cmd/platform_bootstrap.go,cmd/root.go)InstallAlluses a staging Registrar + atomic commitPlugin.Restrictconflict / Startup handler failure each install a structured envelope guard at every dispatch pathwalkGuardneutralises every cobra bypass:PersistentPreRunEfirst-wins,ValidateArgs,ParseFlags,legacyArgs,__complete/__completeNoDesc, non-runnable groups, required-arg subcommandsinstallUnknownSubcommandGuardtags every group it attaches RunE to withcmdpolicy.AnnotationPureGroup, preserving the parent-group invariant the policy engine relies onuserPolicyPathresolves viacore.GetBaseConfigDir()soLARKSUITE_CLI_CONFIG_DIRoverrides reach the policy file location consistentlyExecuteemitshook.Emit(Shutdown, runErr)afterrootCmd.Execute; completion paths skip ShutdownRegisterShortcutsWithContexttags each service group (im,docs,drive,calendar, …) withcmdmeta.SetDomain, soplatform.ByDomain("docs")actually matches via parent-chain inheritanceEnvelope contract (
internal/output/)error.typeclosed enum:command_denied/hook/plugin_install/plugin_conflict/plugin_lifecycle— collapses the previouspruning/strict_modesplit (enforcement mechanism is preserved indetail.layer)unknown_subcommandExitError (exit 2) listing available subcommands instead of silently falling back to help with exit 0Diagnostic surfaces
lark-cli config policy show— JSON view of the active Rule + sourcelark-cli config plugins show— installed plugins and their contributions--help, but aLonghint onlark-cli config --helpadvertises their existence.config policygroup installs a no-op leafPersistentPreRunEso its read-only diagnostics work under env-credential deployments (the parentconfiggroup'sRequireBuiltinCredentialProviderwould otherwise reject them withexternal_provider)Breaking changes
See
CHANGELOG.md[Unreleased]for the full migration notes.error.typerename — strict-mode + policy denials both emitcommand_denied.detail.layer(policy|strict_mode) preserves which layer rejected the call. Agents matchingerror.type == "strict_mode"must update.unknown_subcommandenvelope instead of exit 0 + help. Scripts relying on the exit-0 fallback must update;lark-cli <group> --helpcontinues to return exit 0 + help.Test Plan
go test ./...— passes from a fresh clone, no build tag neededcmd/plugin_integration_test.go— end-to-end throughbuildInternalwith synthetic and real command treescmd/install_guard_test.go—walkGuardcoversauth/config/__complete/__completeNoDesc/ non-runnable parents / required-arg subcommandscmd/unknown_subcommand_test.go— unknown subcommand returns structured envelope (exit 2), no-arg invocation still prints help (exit 0)internal/cmdpolicy/aggregation_test.go— pure-group regression:<group> --helpreturns exit 0 + help undermax_risk: read; aggregate denial still installed when every live child is deniedinternal/cmdpolicy/path_test.go—RedactHomeDirfoldsLARKSUITE_CLI_CONFIG_DIRoverride prefix, $HOME prefix, and leaves unrelated paths unchangedcobra.NoArgs+ parentPersistentPreRunE, observers fire, real envelope is emitted regardless of credential providerlark-cli drive +bogus→unknown_subcommandenvelope (exit 2);lark-cli drive --helpundermax_risk: read→ exit 0 + help;config policy showoutputs<config>/policy.ymlinstead of an absolute filesystem path; unannotated write command is denied when a Rule withMaxRisk=readis activeRelated Issues
Summary by CodeRabbit
New Features
Bug Fixes