feat(mail): unknown-flag fuzzy-match for lark-cli mail domain#806
feat(mail): unknown-flag fuzzy-match for lark-cli mail domain#806xzcong0820 wants to merge 1 commit into
Conversation
github-actions
Bot
added
domain/mail
📝 WalkthroughWalkthroughAdds mail-specific unknown-flag error detection and suggestion generation. A new ChangesMail Flag Suggestion Error Handler
🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers:
🚥 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 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❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #806 +/- ##
==========================================
+ Coverage 65.46% 65.82% +0.36%
==========================================
Files 510 514 +4
Lines 47129 48020 +891
==========================================
+ Hits 30851 31608 +757
- Misses 13607 13689 +82
- Partials 2671 2723 +52 ☔ 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@d271bf39cab20d2b094e2c00dfa734d79c05bf3c🧩 Skill updatenpx skills add xzcong0820/larksuite-cli#harness/01kr5edze944jyrxcn6m99jcd1 -y -g |
| m = c | ||
| } | ||
| return m | ||
| } |
There was a problem hiding this comment.
Issue: Go 1.23 (per go.mod) supports the built-in variadic min(). The custom min3 helper can be replaced with min(a, b, c) — one fewer function to maintain.
Severity: 💡 Suggestion
Suggested Fix:
curr[j] = min(curr[j-1]+1, prev[j]+1, prev[j-1]+cost)Then remove min3.
| if err == nil { | ||
| return nil | ||
| } | ||
| token, isShorthand, ok := parseUnknownToken(err.Error()) |
There was a problem hiding this comment.
Issue: flagSuggestErrorFunc is set on the mail parent svc and cobra walks up the parent chain for the nearest non-nil hook — this works correctly. However, there's no integration-level test that exercises the full path: RegisterShortcuts → InstallOnMail → unknown flag on a real mail subcommand → structured ExitError. The 12 unit tests cover the internal logic well, but a single end-to-end test in register_test.go would guard against the wiring breaking silently (e.g., if the if service == "mail" branch is accidentally removed or the package import changes).
Severity: 💡 Suggestion
Suggested Fix: Add a test similar to TestRegisterShortcutsMountsBaseCommands that calls RegisterShortcuts, looks up a mail subcommand, sets FlagErrorFunc to fire via an invalid flag, and asserts the returned error is an *output.ExitError with Type: "unknown_flag".
Adds shortcuts/mail/flag_suggest.go (~120 LOC) implementing a cobra
FlagErrorFunc hook for the mail subcommand tree. On 'unknown flag: --X'
or 'unknown shorthand flag: "X" in -X', it collects flags from the
current command via cmd.Flags().VisitAll, runs bidirectional prefix
match + Levenshtein DP (threshold=max(1,len/3+1), cap 4), and returns
top-5 candidates inside the existing ErrorEnvelope JSON:
error.type = "unknown_flag"
error.detail.{unknown, command_path, candidates}
error.detail.candidates[*] = {flag, shorthand, distance, reason}
Exit code stays 1 (ExitAPI), not ExitValidation - no breaking change for
CI/agent scripts that check non-zero exit. stderr switches from plain
'Error: unknown flag: --X' to JSON envelope, aligning with the existing
'errors = JSON envelope on stderr' convention; mail unknown-flag was the
last gap.
Scope is strictly the mail subcommand tree: shortcuts/register.go gains
a single 'if service == "mail" { mail.InstallOnMail(svc) }' branch
after the existing Mount loop. Other domains (calendar / im / api /
auth / ...) keep cobra's default FlagErrorFunc and unchanged plain-text
stderr behavior.
Covers:
- shortcuts/mail/flag_suggest.go (new, ~120 LOC)
- shortcuts/mail/flag_suggest_test.go (new, 12 table-driven tests)
- shortcuts/register.go (+3 lines after mail Mount loop)
No changes to cmd/root.go or internal/output/* - ErrDetail.Detail is
already interface{}, handleRootError already routes *ExitError via
WriteErrorEnvelope.
xzcong0820
force-pushed
the
harness/01kr5edze944jyrxcn6m99jcd1
branch
from
5ae9c2d to
d271bf3
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@shortcuts/register_test.go`:
- Around line 357-363: The test currently only checks that returned error isn't
an *output.ExitError; additionally assert that the FlagErrorFunc is identity by
verifying the exact pointer is returned: call baseCmd.FlagErrorFunc()(baseCmd,
in) (already assigned to got) and assert got == in (pointer equality) to ensure
pass-through; update the test around the variables got and in (and the
FlagErrorFunc invocation) to include this equality check so any
wrapper/regression will fail.
🪄 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: d918f259-9a3a-4b2d-ab83-caca250b3df7
📒 Files selected for processing (4)
shortcuts/mail/flag_suggest.goshortcuts/mail/flag_suggest_test.goshortcuts/register.goshortcuts/register_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- shortcuts/register.go
- shortcuts/mail/flag_suggest.go
| got := baseCmd.FlagErrorFunc()(baseCmd, in) | ||
| // Default cobra hook is identity — anything else means the mail hook | ||
| // leaked across domains. | ||
| var exitErr *output.ExitError | ||
| if errors.As(got, &exitErr) { | ||
| t.Fatalf("base service unexpectedly produced *output.ExitError: %#v", exitErr) | ||
| } |
There was a problem hiding this comment.
Assert true pass-through identity for non-mail errors
This test currently proves only “not an *output.ExitError”. To lock the default Cobra contract, also assert got == in (same pointer), otherwise wrappers/regressions could slip through.
Suggested test hardening
in := errors.New("unknown flag: --bogus")
got := baseCmd.FlagErrorFunc()(baseCmd, in)
// Default cobra hook is identity — anything else means the mail hook
// leaked across domains.
var exitErr *output.ExitError
if errors.As(got, &exitErr) {
t.Fatalf("base service unexpectedly produced *output.ExitError: %#v", exitErr)
}
+ if got != in {
+ t.Fatalf("base service should pass through original error pointer, got %T (%v)", got, got)
+ }📝 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.
| got := baseCmd.FlagErrorFunc()(baseCmd, in) | |
| // Default cobra hook is identity — anything else means the mail hook | |
| // leaked across domains. | |
| var exitErr *output.ExitError | |
| if errors.As(got, &exitErr) { | |
| t.Fatalf("base service unexpectedly produced *output.ExitError: %#v", exitErr) | |
| } | |
| got := baseCmd.FlagErrorFunc()(baseCmd, in) | |
| // Default cobra hook is identity — anything else means the mail hook | |
| // leaked across domains. | |
| var exitErr *output.ExitError | |
| if errors.As(got, &exitErr) { | |
| t.Fatalf("base service unexpectedly produced *output.ExitError: %#v", exitErr) | |
| } | |
| if got != in { | |
| t.Fatalf("base service should pass through original error pointer, got %T (%v)", got, got) | |
| } |
🤖 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 `@shortcuts/register_test.go` around lines 357 - 363, The test currently only
checks that returned error isn't an *output.ExitError; additionally assert that
the FlagErrorFunc is identity by verifying the exact pointer is returned: call
baseCmd.FlagErrorFunc()(baseCmd, in) (already assigned to got) and assert got ==
in (pointer equality) to ensure pass-through; update the test around the
variables got and in (and the FlagErrorFunc invocation) to include this equality
check so any wrapper/regression will fail.
2 participants
Generated by the harness-coding skill.
Summary
Adds mail-domain unknown-flag fuzzy-match → JSON ErrorEnvelope with did-you-mean candidates for
lark-cli mail. Strictly scoped toshortcuts/mail/+ a 1-line branch inshortcuts/register.go; non-mail domains are untouched. Exit code is unchanged (still1viaExitAPI, notExitValidation) — no breaking change to callers parsing exit codes.3 files changed, 508 insertions:
shortcuts/mail/flag_suggest.go— implementation (InstallOnMail,parseUnknownToken,collectFlags,suggestw/ prefix + Levenshtein DP,suggestShorthand,buildHint).shortcuts/mail/flag_suggest_test.go— 12 table-driven tests covering prefix / Levenshtein / hidden-skipped / top-N stable sort / shorthand exact + prefix fallback / parse regex / errorFunc pass-through / exit-code regression.shortcuts/register.go— +3 lines:if service == "mail" { mail.InstallOnMail(svc) }after the mount loop.Sprints
Source specs
This MR was created autonomously. Quality gates were enforced by the repo's own pre-commit hooks.
Summary by CodeRabbit
New Features
Behavior
Tests