Feat/risk tiering#633
Open
sang-neo03 wants to merge 8 commits intomainfrom
Open
Conversation
github-actions
Bot
added
the
size/L
|
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 command-level risk annotations and a confirmation flow: commands can be annotated with a risk level, help shows a Changes
Sequence DiagramsequenceDiagram
participant Agent as Agent/User
participant CLI as CLI
participant Handler as Service Handler
participant Confirm as cmdutil.RequireConfirmation
participant Output as Error Output
Agent->>CLI: Run command (no --yes)
CLI->>Handler: Invoke method handler
Handler->>Handler: Inspect method risk
alt risk != "high-risk-write" or --yes present
Handler->>Handler: Proceed with API call
Handler->>Agent: Return result
else risk == "high-risk-write" and --yes missing
Handler->>Confirm: Call RequireConfirmation(action)
Confirm->>Output: Build ExitError (confirmation_required) with risk/action
Output->>Agent: Exit code 10 + JSON error envelope (stderr)
Agent->>Agent: Inspect envelope, decide to confirm
Agent->>CLI: Re-run command with --yes
CLI->>Handler: Handler proceeds and executes
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #633 +/- ##
==========================================
+ Coverage 62.89% 63.03% +0.13%
==========================================
Files 428 438 +10
Lines 37280 38632 +1352
==========================================
+ Hits 23448 24351 +903
- Misses 11721 12133 +412
- Partials 2111 2148 +37 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@5248b08bf1e93961ba9b98f35b6d249e4a416a82🧩 Skill updatenpx skills add larksuite/cli#feat/risk-tiering -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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
shortcuts/common/runner.go (1)
638-655:⚠️ Potential issue | 🟠 MajorGate unconfirmed high-risk shortcuts before resolving inputs.
resolveInputFlagscan consume stdin and read@filecontents before the confirmation error is returned. For high-risk writes without--yes, fail with exit 10 before local I/O; keep--dry-runbypassing the gate.🛡️ Proposed fix
if err := validateEnumFlags(rctx, s.Flags); err != nil { return err } + if s.Risk == "high-risk-write" && !rctx.Bool("dry-run") && !rctx.Bool("yes") { + return cmdutil.RequireConfirmation(s.Service + " " + s.Command) + } if err := resolveInputFlags(rctx, s.Flags); err != nil { return err } if err := output.ValidateJqFlags(rctx.JqExpr, "", rctx.Format); err != nil { @@ if rctx.Bool("dry-run") { return handleShortcutDryRun(f, rctx, s) } - if s.Risk == "high-risk-write" && !rctx.Bool("yes") { - return cmdutil.RequireConfirmation(s.Service + " " + s.Command) - } - if err := s.Execute(rctx.ctx, rctx); err != nil {Please add regression coverage for a high-risk shortcut using stdin or
@fileinput without--yes. As per coding guidelines, "Every behavior change must have an accompanying test".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@shortcuts/common/runner.go` around lines 638 - 655, Move the high-risk confirmation gate to run before any local I/O by checking s.Risk == "high-risk-write" and !rctx.Bool("yes") (but still allow rctx.Bool("dry-run") to bypass the gate) before calling resolveInputFlags; specifically, perform the s.Risk + !rctx.Bool("yes") check (and call/return cmdutil.RequireConfirmation(s.Service + " " + s.Command") to produce the exit-10 behavior) immediately after the existing dry-run check and before resolveInputFlags/rctx JQ validation, so resolveInputFlags (which may consume stdin or `@file`) is not invoked for unconfirmed high-risk shortcuts; also add a regression test that invokes the shortcut with stdin or `@file` input without --yes and verifies it fails early with the expected exit code.cmd/service/service.go (1)
245-261:⚠️ Potential issue | 🟠 MajorMove the confirmation gate before request construction.
buildServiceRequest(opts)can consume stdin and read upload files before theconfirmation_requirederror is returned. For an unconfirmed high-risk write, return exit 10 before local I/O while still allowing--dry-runto render the preview.🛡️ Proposed fix
- request, fileMeta, err := buildServiceRequest(opts) - if err != nil { - return err - } - - if opts.DryRun { - if fileMeta != nil { - return cmdutil.PrintDryRunWithFile(f.IOStreams.Out, request, config, opts.Format, fileMeta.FieldName, fileMeta.FilePath, fileMeta.FormFields) - } - return serviceDryRun(f, request, config, opts.Format) - } - - if registry.GetStrFromMap(opts.Method, "risk") == "high-risk-write" { + highRisk := registry.GetStrFromMap(opts.Method, "risk") == "high-risk-write" + if highRisk && !opts.DryRun { if yes, _ := opts.Cmd.Flags().GetBool("yes"); !yes { return cmdutil.RequireConfirmation(opts.SchemaPath) } } + request, fileMeta, err := buildServiceRequest(opts) + if err != nil { + return err + } + + if opts.DryRun { + if fileMeta != nil { + return cmdutil.PrintDryRunWithFile(f.IOStreams.Out, request, config, opts.Format, fileMeta.FieldName, fileMeta.FilePath, fileMeta.FormFields) + } + return serviceDryRun(f, request, config, opts.Format) + } +Please add a regression test for a high-risk service command with
--params -,--data -, or--fileconfirming it returnsconfirmation_requiredbefore consuming input. As per coding guidelines, "Every behavior change must have an accompanying test".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/service/service.go` around lines 245 - 261, Move the high-risk confirmation check to run before calling buildServiceRequest(opts): check registry.GetStrFromMap(opts.Method, "risk") == "high-risk-write" and, unless opts.DryRun is true, require confirmation via cmdutil.RequireConfirmation(opts.SchemaPath) (using the existing opts.Cmd.Flags().GetBool("yes") logic) so we return the confirmation_required exit before any local I/O or stdin consumption; preserve existing DryRun behavior so --dry-run still constructs and prints the request (using cmdutil.PrintDryRunWithFile/serviceDryRun) without prompting. Also add a regression test that invokes the high-risk service command with --params - / --data - / --file and asserts it returns confirmation_required before consuming input.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/service/service_risk_test.go`:
- Around line 87-96: The test currently asserts on the error string from
cmd.Execute() instead of the structured error; update the test to type-assert
the returned error to ExitConfirmationRequired (or unwrap and compare via
errors.Is against ExitConfirmationRequired) and then assert the embedded risk
details match the expected action "drive.files.delete" (e.g., inspect the
returned confirmation error's Risk or Detail fields) rather than relying on
substring checks so the service gate protocol remains stable; locate the
assertions around cmd.Execute() in service_risk_test.go and replace the string
checks with these structured assertions.
In `@internal/cmdutil/confirm_test.go`:
- Around line 16-51: The test TestRequireConfirmation_EnvelopeShape does not
assert the presence or value of the FixCommand on the confirmation envelope, so
regressions that omit FixCommand will go unnoticed; update the test (in
internal/cmdutil/confirm_test.go, within TestRequireConfirmation_EnvelopeShape)
to check that exitErr.Detail.FixCommand (or the corresponding field on d) is
non-nil/non-empty and equals the expected remediation command (e.g., the exact
string the user should run to fix the issue such as "your-expected-fix-command"
or "add --yes to confirm" style), failing the test if the FixCommand is missing
or incorrect so that RequireConfirmation must continue to populate that field.
- Around line 111-120: The test assumes buildFixCommand simply joins os.Args
with spaces; update both the implementation (buildFixCommand) to produce a
shell-safe serialization (escape or quote tokens containing spaces, quotes or
JSON characters so the resulting fix_command is replayable by shells/agents) and
the test (TestBuildFixCommand_AppendsYes) to cover complex tokens (e.g., a JSON
param or a path with spaces) and assert the returned string contains properly
quoted/escaped tokens plus the appended "--yes"; locate buildFixCommand and any
code that emits fix_command to implement quoting/escaping (use a standard
shell-quoting strategy) and change the test’s os.Args and expected string to
match the new safe serialization.
In `@internal/cmdutil/confirm.go`:
- Around line 49-52: The current construction of fix_command uses
strings.Join(os.Args, " ") which can produce unsafe/unambiguous shell input when
args contain spaces or shell metacharacters; modify the code that builds parts
(the parts slice and the strings.Join call in confirm.go) to shell-escape or
quote each argv element before joining (also quote the appended "--yes");
implement per-argument quoting (e.g., map each element through a shell-escape
helper or use strconv.Quote/robust single-quote-escape logic) and then join the
quoted elements with spaces so the resulting fix_command is safe for agent
execution.
---
Outside diff comments:
In `@cmd/service/service.go`:
- Around line 245-261: Move the high-risk confirmation check to run before
calling buildServiceRequest(opts): check registry.GetStrFromMap(opts.Method,
"risk") == "high-risk-write" and, unless opts.DryRun is true, require
confirmation via cmdutil.RequireConfirmation(opts.SchemaPath) (using the
existing opts.Cmd.Flags().GetBool("yes") logic) so we return the
confirmation_required exit before any local I/O or stdin consumption; preserve
existing DryRun behavior so --dry-run still constructs and prints the request
(using cmdutil.PrintDryRunWithFile/serviceDryRun) without prompting. Also add a
regression test that invokes the high-risk service command with --params - /
--data - / --file and asserts it returns confirmation_required before consuming
input.
In `@shortcuts/common/runner.go`:
- Around line 638-655: Move the high-risk confirmation gate to run before any
local I/O by checking s.Risk == "high-risk-write" and !rctx.Bool("yes") (but
still allow rctx.Bool("dry-run") to bypass the gate) before calling
resolveInputFlags; specifically, perform the s.Risk + !rctx.Bool("yes") check
(and call/return cmdutil.RequireConfirmation(s.Service + " " + s.Command") to
produce the exit-10 behavior) immediately after the existing dry-run check and
before resolveInputFlags/rctx JQ validation, so resolveInputFlags (which may
consume stdin or `@file`) is not invoked for unconfirmed high-risk shortcuts; also
add a regression test that invokes the shortcut with stdin or `@file` input
without --yes and verifies it fails early with the expected exit code.
🪄 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: 69f1a5f6-9945-4d41-8ed4-c3b23b4ef49c
📒 Files selected for processing (13)
cmd/root.gocmd/root_risk_help_test.gocmd/service/service.gocmd/service/service_risk_test.gointernal/cmdutil/confirm.gointernal/cmdutil/confirm_test.gointernal/cmdutil/risk.gointernal/cmdutil/risk_test.gointernal/output/envelope.gointernal/output/exitcode.goshortcuts/common/common.goshortcuts/common/runner.goskills/lark-shared/SKILL.md
💤 Files with no reviewable changes (1)
- shortcuts/common/common.go
Comment on lines
+87
to
+96
| err := cmd.Execute() | ||
| if err == nil { | ||
| t.Fatal("expected confirmation error, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "requires confirmation") { | ||
| t.Errorf("expected 'requires confirmation' in error, got: %v", err) | ||
| } | ||
| if !strings.Contains(err.Error(), "drive.files.delete") { | ||
| t.Errorf("expected schema path in error action, got: %v", err) | ||
| } |
There was a problem hiding this comment.
Assert the structured confirmation error, not only text.
This integration test would still pass if the service path returned a plain error containing the same words. Pin ExitConfirmationRequired and the risk detail so the service gate keeps the stable agent-facing protocol.
Proposed assertion
import (
+ "errors"
"strings"
"testing"
"github.com/larksuite/cli/internal/cmdutil"
+ "github.com/larksuite/cli/internal/output"
)
@@
if err == nil {
t.Fatal("expected confirmation error, got nil")
}
+ var exitErr *output.ExitError
+ if !errors.As(err, &exitErr) {
+ t.Fatalf("expected *output.ExitError, got %T: %v", err, err)
+ }
+ if exitErr.Code != output.ExitConfirmationRequired {
+ t.Errorf("exit code = %d, want %d", exitErr.Code, output.ExitConfirmationRequired)
+ }
+ if exitErr.Detail == nil || exitErr.Detail.Risk == nil {
+ t.Fatalf("expected structured risk detail, got %#v", exitErr.Detail)
+ }
+ if exitErr.Detail.Risk.Level != "high-risk-write" || exitErr.Detail.Risk.Action != "drive.files.delete" {
+ t.Errorf("risk detail = %#v", exitErr.Detail.Risk)
+ }
if !strings.Contains(err.Error(), "requires confirmation") {
t.Errorf("expected 'requires confirmation' in error, got: %v", err)
}As per coding guidelines, “Make error messages structured, actionable, and specific for AI agent parsing”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/service/service_risk_test.go` around lines 87 - 96, The test currently
asserts on the error string from cmd.Execute() instead of the structured error;
update the test to type-assert the returned error to ExitConfirmationRequired
(or unwrap and compare via errors.Is against ExitConfirmationRequired) and then
assert the embedded risk details match the expected action "drive.files.delete"
(e.g., inspect the returned confirmation error's Risk or Detail fields) rather
than relying on substring checks so the service gate protocol remains stable;
locate the assertions around cmd.Execute() in service_risk_test.go and replace
the string checks with these structured assertions.
Comment on lines
+16
to
+51
| func TestRequireConfirmation_EnvelopeShape(t *testing.T) { | ||
| err := RequireConfirmation("drive +delete") | ||
| if err == nil { | ||
| t.Fatal("expected non-nil error") | ||
| } | ||
|
|
||
| var exitErr *output.ExitError | ||
| if !errors.As(err, &exitErr) { | ||
| t.Fatalf("expected *output.ExitError, got %T", err) | ||
| } | ||
| if exitErr.Code != output.ExitConfirmationRequired { | ||
| t.Errorf("Code = %d, want %d", exitErr.Code, output.ExitConfirmationRequired) | ||
| } | ||
| if exitErr.Detail == nil { | ||
| t.Fatal("Detail is nil") | ||
| } | ||
| d := exitErr.Detail | ||
| if d.Type != "confirmation_required" { | ||
| t.Errorf("Type = %q, want confirmation_required", d.Type) | ||
| } | ||
| if !strings.Contains(d.Message, "drive +delete") || !strings.Contains(d.Message, "requires confirmation") { | ||
| t.Errorf("Message = %q, want it to mention action and 'requires confirmation'", d.Message) | ||
| } | ||
| if d.Hint != "add --yes to confirm" { | ||
| t.Errorf("Hint = %q, want 'add --yes to confirm'", d.Hint) | ||
| } | ||
| if d.Risk == nil { | ||
| t.Fatal("Risk is nil") | ||
| } | ||
| if d.Risk.Level != "high-risk-write" { | ||
| t.Errorf("Risk.Level = %q, want high-risk-write", d.Risk.Level) | ||
| } | ||
| if d.Risk.Action != "drive +delete" { | ||
| t.Errorf("Risk.Action = %q, want drive +delete", d.Risk.Action) | ||
| } | ||
| } |
There was a problem hiding this comment.
Assert fix_command on the confirmation envelope.
This test pins risk and the message, but not the required remediation command. If RequireConfirmation stops assigning FixCommand, these tests still pass even though the protocol regresses.
Proposed test tightening
func TestRequireConfirmation_EnvelopeShape(t *testing.T) {
+ orig := os.Args
+ t.Cleanup(func() { os.Args = orig })
+ os.Args = []string{"lark-cli", "drive", "+delete"}
+
err := RequireConfirmation("drive +delete")
if err == nil {
t.Fatal("expected non-nil error")
}
@@
if d.Hint != "add --yes to confirm" {
t.Errorf("Hint = %q, want 'add --yes to confirm'", d.Hint)
}
+ if d.FixCommand != "lark-cli drive +delete --yes" {
+ t.Errorf("FixCommand = %q, want retry command with --yes", d.FixCommand)
+ }
if d.Risk == nil {
t.Fatal("Risk is nil")
}As per coding guidelines, “Every behavior change must have an accompanying test” and “Make error messages structured, actionable, and specific for AI agent parsing”.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func TestRequireConfirmation_EnvelopeShape(t *testing.T) { | |
| err := RequireConfirmation("drive +delete") | |
| if err == nil { | |
| t.Fatal("expected non-nil error") | |
| } | |
| var exitErr *output.ExitError | |
| if !errors.As(err, &exitErr) { | |
| t.Fatalf("expected *output.ExitError, got %T", err) | |
| } | |
| if exitErr.Code != output.ExitConfirmationRequired { | |
| t.Errorf("Code = %d, want %d", exitErr.Code, output.ExitConfirmationRequired) | |
| } | |
| if exitErr.Detail == nil { | |
| t.Fatal("Detail is nil") | |
| } | |
| d := exitErr.Detail | |
| if d.Type != "confirmation_required" { | |
| t.Errorf("Type = %q, want confirmation_required", d.Type) | |
| } | |
| if !strings.Contains(d.Message, "drive +delete") || !strings.Contains(d.Message, "requires confirmation") { | |
| t.Errorf("Message = %q, want it to mention action and 'requires confirmation'", d.Message) | |
| } | |
| if d.Hint != "add --yes to confirm" { | |
| t.Errorf("Hint = %q, want 'add --yes to confirm'", d.Hint) | |
| } | |
| if d.Risk == nil { | |
| t.Fatal("Risk is nil") | |
| } | |
| if d.Risk.Level != "high-risk-write" { | |
| t.Errorf("Risk.Level = %q, want high-risk-write", d.Risk.Level) | |
| } | |
| if d.Risk.Action != "drive +delete" { | |
| t.Errorf("Risk.Action = %q, want drive +delete", d.Risk.Action) | |
| } | |
| } | |
| func TestRequireConfirmation_EnvelopeShape(t *testing.T) { | |
| orig := os.Args | |
| t.Cleanup(func() { os.Args = orig }) | |
| os.Args = []string{"lark-cli", "drive", "+delete"} | |
| err := RequireConfirmation("drive +delete") | |
| if err == nil { | |
| t.Fatal("expected non-nil error") | |
| } | |
| var exitErr *output.ExitError | |
| if !errors.As(err, &exitErr) { | |
| t.Fatalf("expected *output.ExitError, got %T", err) | |
| } | |
| if exitErr.Code != output.ExitConfirmationRequired { | |
| t.Errorf("Code = %d, want %d", exitErr.Code, output.ExitConfirmationRequired) | |
| } | |
| if exitErr.Detail == nil { | |
| t.Fatal("Detail is nil") | |
| } | |
| d := exitErr.Detail | |
| if d.Type != "confirmation_required" { | |
| t.Errorf("Type = %q, want confirmation_required", d.Type) | |
| } | |
| if !strings.Contains(d.Message, "drive +delete") || !strings.Contains(d.Message, "requires confirmation") { | |
| t.Errorf("Message = %q, want it to mention action and 'requires confirmation'", d.Message) | |
| } | |
| if d.Hint != "add --yes to confirm" { | |
| t.Errorf("Hint = %q, want 'add --yes to confirm'", d.Hint) | |
| } | |
| if d.FixCommand != "lark-cli drive +delete --yes" { | |
| t.Errorf("FixCommand = %q, want retry command with --yes", d.FixCommand) | |
| } | |
| if d.Risk == nil { | |
| t.Fatal("Risk is nil") | |
| } | |
| if d.Risk.Level != "high-risk-write" { | |
| t.Errorf("Risk.Level = %q, want high-risk-write", d.Risk.Level) | |
| } | |
| if d.Risk.Action != "drive +delete" { | |
| t.Errorf("Risk.Action = %q, want drive +delete", d.Risk.Action) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmdutil/confirm_test.go` around lines 16 - 51, The test
TestRequireConfirmation_EnvelopeShape does not assert the presence or value of
the FixCommand on the confirmation envelope, so regressions that omit FixCommand
will go unnoticed; update the test (in internal/cmdutil/confirm_test.go, within
TestRequireConfirmation_EnvelopeShape) to check that exitErr.Detail.FixCommand
(or the corresponding field on d) is non-nil/non-empty and equals the expected
remediation command (e.g., the exact string the user should run to fix the issue
such as "your-expected-fix-command" or "add --yes to confirm" style), failing
the test if the FixCommand is missing or incorrect so that RequireConfirmation
must continue to populate that field.
Comment on lines
+111
to
+120
| func TestBuildFixCommand_AppendsYes(t *testing.T) { | ||
| orig := os.Args | ||
| t.Cleanup(func() { os.Args = orig }) | ||
| os.Args = []string{"lark-cli", "drive", "+delete", "--file-token", "abc"} | ||
|
|
||
| got := buildFixCommand() | ||
| want := "lark-cli drive +delete --file-token abc --yes" | ||
| if got != want { | ||
| t.Errorf("buildFixCommand() = %q, want %q", got, want) | ||
| } |
There was a problem hiding this comment.
Cover shell-safe fix_command serialization.
The current expectation only covers trivial tokens and effectively locks in raw space-joining. A retry command containing JSON params or paths with spaces becomes ambiguous/non-replayable for agents.
Proposed regression test
func TestBuildFixCommand_AppendsYes(t *testing.T) {
orig := os.Args
t.Cleanup(func() { os.Args = orig })
os.Args = []string{"lark-cli", "drive", "+delete", "--file-token", "abc"}
@@
if got != want {
t.Errorf("buildFixCommand() = %q, want %q", got, want)
}
}
+
+func TestBuildFixCommand_QuotesArgsWithSpaces(t *testing.T) {
+ orig := os.Args
+ t.Cleanup(func() { os.Args = orig })
+ os.Args = []string{
+ "lark-cli",
+ "drive",
+ "+delete",
+ "--params",
+ `{"file_token":"tok abc"}`,
+ }
+
+ got := buildFixCommand()
+ want := `lark-cli drive +delete --params '{"file_token":"tok abc"}' --yes`
+ if got != want {
+ t.Errorf("buildFixCommand() = %q, want %q", got, want)
+ }
+}As per coding guidelines, “Design CLI flags, help text, and error messages with AI agent consumption in mind; every error message will be parsed by AI agents”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmdutil/confirm_test.go` around lines 111 - 120, The test assumes
buildFixCommand simply joins os.Args with spaces; update both the implementation
(buildFixCommand) to produce a shell-safe serialization (escape or quote tokens
containing spaces, quotes or JSON characters so the resulting fix_command is
replayable by shells/agents) and the test (TestBuildFixCommand_AppendsYes) to
cover complex tokens (e.g., a JSON param or a path with spaces) and assert the
returned string contains properly quoted/escaped tokens plus the appended
"--yes"; locate buildFixCommand and any code that emits fix_command to implement
quoting/escaping (use a standard shell-quoting strategy) and change the test’s
os.Args and expected string to match the new safe serialization.
Comment on lines
+49
to
+52
| parts := make([]string, 0, len(os.Args)+1) | ||
| parts = append(parts, os.Args...) | ||
| parts = append(parts, "--yes") | ||
| return strings.Join(parts, " ") |
There was a problem hiding this comment.
Shell-escape argv before emitting fix_command.
strings.Join(os.Args, " ") makes fix_command ambiguous and potentially unsafe for agent execution when flag values contain spaces, quotes, ;, &&, $(), or newlines. Since agents are instructed to retry this command, quote each argv element before joining.
🛡️ Proposed fix
parts := make([]string, 0, len(os.Args)+1)
- parts = append(parts, os.Args...)
+ for _, arg := range os.Args {
+ parts = append(parts, shellQuoteArg(arg))
+ }
parts = append(parts, "--yes")
return strings.Join(parts, " ")
}
+
+func shellQuoteArg(arg string) string {
+ if arg != "" && strings.IndexFunc(arg, func(r rune) bool {
+ return !((r >= 'a' && r <= 'z') ||
+ (r >= 'A' && r <= 'Z') ||
+ (r >= '0' && r <= '9') ||
+ strings.ContainsRune("@%_+=:,./-", r))
+ }) == -1 {
+ return arg
+ }
+ return "'" + strings.ReplaceAll(arg, "'", "'\"'\"'") + "'"
+}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmdutil/confirm.go` around lines 49 - 52, The current construction
of fix_command uses strings.Join(os.Args, " ") which can produce
unsafe/unambiguous shell input when args contain spaces or shell metacharacters;
modify the code that builds parts (the parts slice and the strings.Join call in
confirm.go) to shell-escape or quote each argv element before joining (also
quote the appended "--yes"); implement per-argument quoting (e.g., map each
element through a shell-escape helper or use strconv.Quote/robust
single-quote-escape logic) and then join the quoted elements with spaces so the
resulting fix_command is safe for agent execution.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/cmdutil/risk_test.go (1)
51-60: Optional: also assert the risk key was written, not just that the map was initialized.
TestSetRisk_InitializesNilAnnotationsonly verifiescmd.Annotations != nilafterSetRisk. A regression that initialized the map but failed to store the value (e.g., a future refactor swapping the order of the init and the assignment) would still pass this test. Consider strengthening the assertion.♻️ Proposed tightening
SetRisk(cmd, "write") if cmd.Annotations == nil { t.Fatal("SetRisk should lazily initialize Annotations") } + if level, ok := GetRisk(cmd); !ok || level != "write" { + t.Errorf("expected risk annotation to be stored: level=%q ok=%v", level, ok) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/risk_test.go` around lines 51 - 60, Update the test TestSetRisk_InitializesNilAnnotations to not only assert that cmd.Annotations is non-nil after calling SetRisk(cmd, "write") but also that the expected key/value was written into the map: check that cmd.Annotations["risk"] (or whatever key SetRisk uses) exists and equals "write". Locate SetRisk and use its exact annotation key name in the assertion so the test fails if the map is initialized but the value is not stored.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/cmdutil/risk_test.go`:
- Around line 51-60: Update the test TestSetRisk_InitializesNilAnnotations to
not only assert that cmd.Annotations is non-nil after calling SetRisk(cmd,
"write") but also that the expected key/value was written into the map: check
that cmd.Annotations["risk"] (or whatever key SetRisk uses) exists and equals
"write". Locate SetRisk and use its exact annotation key name in the assertion
so the test fails if the map is initialized but the value is not stored.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5188e54b-43eb-46d6-8c15-0bab72515005
📒 Files selected for processing (1)
internal/cmdutil/risk_test.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/cli_e2e/sheets/sheets_crud_workflow_test.go (1)
206-214: Optional: preferYes: trueover inline--yesfor consistency.This cleanup builds the
--yesarg inline inArgs, while the rest of the file (and the broader e2e suite in this PR) uses the newRequest.Yesfield. Switching keeps the opt-in mechanism uniform across cleanups and lets a future change to the flag name flow through the runner only.♻️ Proposed change
deleteResult, deleteErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{ Args: []string{ "drive", "+delete", "--file-token", spreadsheetToken, "--type", "sheet", - "--yes", }, DefaultAs: "bot", + Yes: true, })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/cli_e2e/sheets/sheets_crud_workflow_test.go` around lines 206 - 214, The test cleanup currently passes the confirmation flag inline in clie2e.RunCmd Args ("--yes"); update this to use the Request.Yes field on the clie2e.Request instead (remove "--yes" from Args and set Yes: true) so the call to RunCmd and the clie2e.Request usage in the delete block matches the rest of the suite and will pick up future flag-name changes centrally; locate the RunCmd invocation that builds clie2e.Request in the delete block (where deleteResult, deleteErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{...}) is created) and make the swap.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/cli_e2e/sheets/sheets_crud_workflow_test.go`:
- Around line 206-214: The test cleanup currently passes the confirmation flag
inline in clie2e.RunCmd Args ("--yes"); update this to use the Request.Yes field
on the clie2e.Request instead (remove "--yes" from Args and set Yes: true) so
the call to RunCmd and the clie2e.Request usage in the delete block matches the
rest of the suite and will pick up future flag-name changes centrally; locate
the RunCmd invocation that builds clie2e.Request in the delete block (where
deleteResult, deleteErr := clie2e.RunCmd(cleanupCtx, clie2e.Request{...}) is
created) and make the swap.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7870479e-2690-41d4-923b-4fe5ab562b90
📒 Files selected for processing (12)
tests/cli_e2e/calendar/calendar_create_event_test.gotests/cli_e2e/calendar/calendar_manage_calendar_test.gotests/cli_e2e/calendar/calendar_personal_event_workflow_test.gotests/cli_e2e/calendar/calendar_rsvp_workflow_test.gotests/cli_e2e/core.gotests/cli_e2e/demo/task_lifecycle_test.gotests/cli_e2e/im/chat_workflow_test.gotests/cli_e2e/sheets/sheets_crud_workflow_test.gotests/cli_e2e/sheets/sheets_filter_workflow_test.gotests/cli_e2e/task/helpers_test.gotests/cli_e2e/task/task_update_workflow_test.gotests/cli_e2e/task/tasklist_workflow_test.go
✅ Files skipped from review due to trivial changes (4)
- tests/cli_e2e/demo/task_lifecycle_test.go
- tests/cli_e2e/task/helpers_test.go
- tests/cli_e2e/task/task_update_workflow_test.go
- tests/cli_e2e/calendar/calendar_rsvp_workflow_test.go
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Give AI agents a stable protocol signal for high-risk write operations. Introduces exit code
10 = confirmation_requiredwith a structured error envelope (risk.level,risk.action,fix_command), unifying the confirmation path across shortcut and service commands.Changes
ExitConfirmationRequired = 10and extendErrDetailwithFixCommand+Riskfields (internal/output/{exitcode,envelope}.go)cmdutil.RequireConfirmation(action)+SetRisk/GetRiskhelpers, shared by shortcut runner and service command path (internal/cmdutil/{confirm,risk}.go)RequireConfirmation, wireSetRiskon mount (shortcuts/common/{runner.go,common.go})method["risk"]from meta, auto-register--yesforhigh-risk-write, insert gate after--dry-runcheck (cmd/service/service.go)Risk: <level>line in--helpoutput for any command with risk metadata (cmd/root.go)risk_test.go,confirm_test.go,root_risk_help_test.go,service_risk_test.goTest Plan
go test ./...— all framework packages green; e2e failures pre-existing on main)lark-cli drive +deleteandlark-cli drive files copycommands returnexit 10+confirmation_requiredenvelope when--yesisomitted
--helpshowsRisk: <level>for shortcut and service commands--dry-runbypasses gate for both pathsSummary by CodeRabbit
New Features
Documentation
Tests