Skip to content

fix: reject positional arguments in shortcuts#227

Merged
MaxHuang22 merged 2 commits intomainfrom
fix/reject-positional-args
Apr 8, 2026
Merged

fix: reject positional arguments in shortcuts#227
MaxHuang22 merged 2 commits intomainfrom
fix/reject-positional-args

Conversation

@MaxHuang22
Copy link
Copy Markdown
Collaborator

@MaxHuang22 MaxHuang22 commented Apr 2, 2026

Summary

  • Shortcuts 之前会静默忽略位置参数(如 lark-cli docs +search "hello"),导致空搜索结果
  • 在所有声明式 shortcut 的 cobra 命令上添加 Args 校验器,传入位置参数时打印 usage 并报错:positional arguments are not supported (got "hello"); pass values via flags
  • 错误使用普通 error 而非 ExitError,避免输出 JSON 信封,保持 CLI 原生体验

Test plan

  • go test ./shortcuts/common/... -run TestRejectPositionalArgs 通过
  • go test ./shortcuts/common/... 全量通过
  • go build ./... 编译通过
  • 手动验证 lark-cli docs +search "hello" 输出 usage + 错误提示
  • 手动验证 lark-cli docs +search --query "hello" 正常工作

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Bug Fixes
    • Shortcut commands now reject positional arguments; values must be supplied via flags. Error messages clearly state positional arguments are unsupported and display the offending values, and CLI usage is shown to guide correct input.
  • Tests
    • Added unit tests covering no args, single arg, and multiple args to ensure consistent rejection messaging and behavior.

@github-actions github-actions bot added the size/M Single-domain feat or fix with limited business impact label Apr 2, 2026
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 2, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: b65aa624-8a25-45e5-9f9a-d485f59a0b12

📥 Commits

Reviewing files that changed from the base of the PR and between 1b6d962 and 002d36f.

📒 Files selected for processing (2)
  • shortcuts/common/runner.go
  • shortcuts/common/runner_args_test.go
✅ Files skipped from review due to trivial changes (1)
  • shortcuts/common/runner.go
🚧 Files skipped from review as they are similar to previous changes (1)
  • shortcuts/common/runner_args_test.go

📝 Walkthrough

Walkthrough

Added a Cobra positional-argument validator rejectPositionalArgs() and configured commands to use it so any provided positional args are rejected; included unit tests covering single, multiple, and empty/nil argument cases.

Changes

Cohort / File(s) Summary
Positional Args Validation
shortcuts/common/runner.go
Added rejectPositionalArgs() returning a cobra.PositionalArgs that permits zero args and returns a fmt.Errorf when positional args are present. Wired into command construction via Args: rejectPositionalArgs(), changing CLI error path to Cobra’s standard error/usage output.
Validation Tests
shortcuts/common/runner_args_test.go
Added three tests (TestRejectPositionalArgs_WithArgs, TestRejectPositionalArgs_MultipleArgs, TestRejectPositionalArgs_NoArgs) verifying rejection messages include provided values and that nil/empty slices are accepted.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

I nibble code in morning light,
Positional berries—nope, not right.
Flags are tidy, neat, and sage,
Hop this change into the cage. 🐇✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: adding validation to reject positional arguments in shortcuts, which directly matches the primary objective of the changeset.
Description check ✅ Passed The PR description covers all required template sections: Summary explains the motivation, Changes lists main modifications, Test Plan documents verification steps with checkmarks, and Related Issues is addressed.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/reject-positional-args

Comment @coderabbitai help to get the list of available commands and usage tips.

@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 2, 2026

Greptile Summary

This PR fixes silent swallowing of positional arguments in declarative shortcut commands by wiring a rejectPositionalArgs() validator into mountDeclarative. Previously, passing a bare value like lark-cli docs +search "hello" would silently produce empty results; now cobra rejects it with a clear usage message.

Key changes:

  • shortcuts/common/runner.go: rejectPositionalArgs() is a plain func() cobra.PositionalArgs (no unused parameters) that formats the full args slice with %q, covering single and multiple positional arguments in the error message. Using a plain error (not ExitError) keeps cobra's default usage printing and avoids wrapping the message in a JSON envelope.
  • shortcuts/common/runner_args_test.go: Three table-style tests cover the single-arg, multi-arg, and no-arg paths; all previous review concerns (unused *Shortcut parameter, first-arg-only message, missing multi-arg test) have been resolved.

Confidence Score: 5/5

Safe to merge — the change is minimal, well-tested, and all prior review concerns have been addressed.

No P0 or P1 findings remain. All three issues flagged in the previous review round (unused parameter, first-arg-only error message, missing multi-arg test) are fully resolved in this revision. The implementation is correct, the test coverage is solid, and the change has a very small blast radius (only declarative shortcut commands).

No files require special attention.

Important Files Changed

Filename Overview
shortcuts/common/runner.go Adds rejectPositionalArgs() validator to mountDeclarative and defines the helper; all previous review concerns (unused parameter, first-arg-only message, missing multi-arg test) are fully addressed.
shortcuts/common/runner_args_test.go New test file covering single arg, multiple args, and no-args cases; TestRejectPositionalArgs_MultipleArgs was added to address the previous-round feedback.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["User runs: lark-cli docs +search hello"] --> B["cobra parses command"]
    B --> C{"Args validator\nrejectPositionalArgs()"}
    C -- "len(args) == 0" --> D["RunE: runShortcut()"]
    C -- "len(args) > 0" --> E["Return plain error\npositional arguments are not supported"]
    E --> F["cobra prints Usage + Error line\nno JSON envelope"]
    D --> G["API call succeeds"]
Loading

Reviews (2): Last reviewed commit: "fix: address PR review comments" | Re-trigger Greptile

@github-actions
Copy link
Copy Markdown

github-actions bot commented Apr 2, 2026

🚀 PR Preview Install Guide

🧰 CLI update

npm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@002d36f11355571413a0ba3aef3ba59065a339c5

🧩 Skill update

npx skills add larksuite/cli#fix/reject-positional-args -y -g

Shortcuts silently ignored positional arguments (e.g. `lark-cli docs
+search "hello"`), causing empty results. Add Args validator to all
declarative shortcuts so cobra prints usage and a clear error message
telling users to pass values via flags instead.

Change-Id: I7579f9c871138cf91dd5f5d8c1d51bda3f77a1db
- Remove unused *Shortcut parameter from rejectPositionalArgs
- Show all positional args in error message instead of only the first
- Add test case for multiple positional arguments

Change-Id: Ifea92d09ddabcd35fbf2db98d9888d18af59b894
@MaxHuang22 MaxHuang22 force-pushed the fix/reject-positional-args branch from 1b6d962 to 002d36f Compare April 8, 2026 06:30
@greptile-apps
Copy link
Copy Markdown

greptile-apps bot commented Apr 8, 2026

Greptile encountered an error while reviewing this PR. Please reach out to support@greptile.com for assistance.

@MaxHuang22 MaxHuang22 merged commit 7158dc2 into main Apr 8, 2026
15 checks passed
@MaxHuang22 MaxHuang22 deleted the fix/reject-positional-args branch April 8, 2026 07:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/M Single-domain feat or fix with limited business impact

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants