feat: add doctor preflight for shortcut readiness#837
Conversation
|
|
github-actions
Bot
added
the
size/L
📝 WalkthroughWalkthroughAdds a new ChangesDoctor Preflight Feature
Sequence DiagramsequenceDiagram
participant CLI as lark-cli
participant PreflightCmd as PreflightCommand
participant Factory as Factory/Config
participant CredProv as CredentialProvider
participant TokenStore as TokenStore
CLI->>PreflightCmd: invoke preflight <service> <shortcut> (--as, --format)
PreflightCmd->>Factory: load effective CLI config / profile
PreflightCmd->>PreflightCmd: resolve target shortcut & required scopes
PreflightCmd->>CredProv: resolve identity/token (if user)
CredProv->>TokenStore: fetch stored token / metadata
PreflightCmd->>PreflightCmd: evaluate checks (config, strict-mode, token, scopes, risk)
PreflightCmd->>CLI: render JSON or pretty result + next-action commands
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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 #837 +/- ##
==========================================
- Coverage 65.77% 65.68% -0.09%
==========================================
Files 516 517 +1
Lines 48625 49043 +418
==========================================
+ Hits 31985 32216 +231
- Misses 13881 14046 +165
- Partials 2759 2781 +22 ☔ 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@685dd307631980d8772dff3b32e5221e2bb7618b🧩 Skill updatenpx skills add lizixionglzx/cli#feat/doctor-preflight -y -g |
evandance
added
domain/core
![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
🧹 Nitpick comments (2)
cmd/doctor/preflight_test.go (1)
526-555: ⚡ Quick winConsider whether
cmdutil.TestFactorycan replacecmdutil.NewDefaulthere.Per coding guidelines, test factories should use
cmdutil.TestFactory(t, config). However, Line 553 injects a customcredential.ProviderwhichTestFactorymay not support. IfTestFactorycan accommodate custom credential providers, refactor to use it; otherwise the current approach is justified for controlling token resolution in tests.As per coding guidelines: "Use
cmdutil.TestFactory(t, config)for test factories in unit tests"🤖 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/doctor/preflight_test.go` around lines 526 - 555, The test helper newPreflightFactory currently constructs a Factory via cmdutil.NewDefault so it can set f.Credential to credential.NewCredentialProvider(... with preflightAccountResolver and preflightTokenResolver); replace this with cmdutil.TestFactory(t, config) if TestFactory supports injecting a custom credential provider, otherwise add support: update cmdutil.TestFactory signature (or add cmdutil.TestFactoryWithCredential) to accept an optional credential.Provider parameter and use it to set f.Credential internally, then switch newPreflightFactory to call the TestFactory variant and remove the manual f.Credential assignment (keep using preflightAccountResolver and preflightTokenResolver when constructing the provider).cmd/doctor/preflight.go (1)
110-123: ⚡ Quick winValidate
--asupfront alongside--format.
--formatis validated immediately inRunE, but--asvalidation is deferred toresolvePreflightIdentity, which only runs afterresolveTargetShortcutandresolvePreflightConfig(config load + keychain resolution). A user / agent that passes--as=wrongpays the cost of config resolution before getting the validation error, and the two flag failures behave differently for callers. Moving the--asallowlist check next to the--formatcheck makes the validation order predictable and keeps both flag-validation errors close tocobra.ExactArgs(2).♻️ Proposed change
RunE: func(cmd *cobra.Command, args []string) error { opts.Ctx = cmd.Context() opts.Service = args[0] opts.Shortcut = args[1] if !slices.Contains([]string{"json", "pretty"}, opts.Format) { return output.ErrValidation("invalid --format %q: must be json or pretty", opts.Format) } + normalizedAs := strings.TrimSpace(opts.RequestedAs) + if normalizedAs == "" { + normalizedAs = string(core.AsAuto) + } + if !slices.Contains([]string{string(core.AsAuto), string(core.AsUser), string(core.AsBot)}, normalizedAs) { + return output.ErrValidation("invalid --as %q: must be auto, user, or bot", opts.RequestedAs) + } + opts.RequestedAs = normalizedAs return doctorPreflightRun(opts) },🤖 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/doctor/preflight.go` around lines 110 - 123, The RunE should validate the --as flag immediately (same place as --format) so callers get a fast, consistent flag error; update the RunE block that currently sets opts.RequestedAs and opts.Format to check that opts.RequestedAs is one of "auto","user","bot" (returning output.ErrValidation(...) on invalid values) before calling doctorPreflightRun, leaving resolvePreflightIdentity as-is for later runtime resolution.
🤖 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/doctor/preflight.go`:
- Around line 290-303: The early-return on identity_supported currently returns
raw checks/actions and skips appending the risk hint; in runPreflightChecks
update the identity failure path (the block that handles
opts.Factory.CheckIdentity(...) error) to call appendRiskHints(checks, actions)
(or appendRiskHints(checks, actions, opts) if that signature is used) before
returning so the "risk" preflightCheck is included and the JSON/report shape
matches other failure paths; preserve the existing
preflightCheck/preflightAction values (Name "identity_supported", Blocking true,
etc.) and the return of checks, actions after appendRiskHints.
---
Nitpick comments:
In `@cmd/doctor/preflight_test.go`:
- Around line 526-555: The test helper newPreflightFactory currently constructs
a Factory via cmdutil.NewDefault so it can set f.Credential to
credential.NewCredentialProvider(... with preflightAccountResolver and
preflightTokenResolver); replace this with cmdutil.TestFactory(t, config) if
TestFactory supports injecting a custom credential provider, otherwise add
support: update cmdutil.TestFactory signature (or add
cmdutil.TestFactoryWithCredential) to accept an optional credential.Provider
parameter and use it to set f.Credential internally, then switch
newPreflightFactory to call the TestFactory variant and remove the manual
f.Credential assignment (keep using preflightAccountResolver and
preflightTokenResolver when constructing the provider).
In `@cmd/doctor/preflight.go`:
- Around line 110-123: The RunE should validate the --as flag immediately (same
place as --format) so callers get a fast, consistent flag error; update the RunE
block that currently sets opts.RequestedAs and opts.Format to check that
opts.RequestedAs is one of "auto","user","bot" (returning
output.ErrValidation(...) on invalid values) before calling doctorPreflightRun,
leaving resolvePreflightIdentity as-is for later runtime resolution.
🪄 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: 6c01efba-0854-4d97-9323-3f9fdfdce327
📒 Files selected for processing (4)
README.mdREADME.zh.mdcmd/doctor/preflight.gocmd/doctor/preflight_test.go
🚧 Files skipped from review as they are similar to previous changes (2)
- README.md
- README.zh.md
| if err := opts.Factory.CheckIdentity(resolvedAs, shortcutAuthTypes(shortcut)); err != nil { | ||
| checks = append(checks, preflightCheck{ | ||
| Name: "identity_supported", | ||
| Status: "fail", | ||
| Blocking: true, | ||
| Message: err.Error(), | ||
| }) | ||
| actions = append(actions, preflightAction{ | ||
| Type: "switch_identity", | ||
| Blocking: true, | ||
| Reason: "choose an identity supported by the target shortcut", | ||
| }) | ||
| return checks, actions | ||
| } |
There was a problem hiding this comment.
Inconsistent risk-hint emission when identity_supported fails.
Every other early-return path in runPreflightChecks finishes through appendRiskHints (strict-mode fail at line 272, user-token fail at line 318, and the normal end at line 351), but the identity_supported failure path returns the bare checks, actions instead. As a result, when the requested identity is unsupported by the shortcut, the risk check is silently dropped from the report while it appears in every other not-ready case, which makes the rendered output and JSON shape inconsistent for consumers.
🐛 Proposed fix
- actions = append(actions, preflightAction{
- Type: "switch_identity",
- Blocking: true,
- Reason: "choose an identity supported by the target shortcut",
- })
- return checks, actions
+ actions = append(actions, preflightAction{
+ Type: "switch_identity",
+ Blocking: true,
+ Reason: "choose an identity supported by the target shortcut",
+ })
+ return appendRiskHints(checks, actions, shortcut, opts.Service, opts.Shortcut)📝 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.
| if err := opts.Factory.CheckIdentity(resolvedAs, shortcutAuthTypes(shortcut)); err != nil { | |
| checks = append(checks, preflightCheck{ | |
| Name: "identity_supported", | |
| Status: "fail", | |
| Blocking: true, | |
| Message: err.Error(), | |
| }) | |
| actions = append(actions, preflightAction{ | |
| Type: "switch_identity", | |
| Blocking: true, | |
| Reason: "choose an identity supported by the target shortcut", | |
| }) | |
| return checks, actions | |
| } | |
| if err := opts.Factory.CheckIdentity(resolvedAs, shortcutAuthTypes(shortcut)); err != nil { | |
| checks = append(checks, preflightCheck{ | |
| Name: "identity_supported", | |
| Status: "fail", | |
| Blocking: true, | |
| Message: err.Error(), | |
| }) | |
| actions = append(actions, preflightAction{ | |
| Type: "switch_identity", | |
| Blocking: true, | |
| Reason: "choose an identity supported by the target shortcut", | |
| }) | |
| return appendRiskHints(checks, actions, shortcut, opts.Service, opts.Shortcut) | |
| } |
🤖 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/doctor/preflight.go` around lines 290 - 303, The early-return on
identity_supported currently returns raw checks/actions and skips appending the
risk hint; in runPreflightChecks update the identity failure path (the block
that handles opts.Factory.CheckIdentity(...) error) to call
appendRiskHints(checks, actions) (or appendRiskHints(checks, actions, opts) if
that signature is used) before returning so the "risk" preflightCheck is
included and the JSON/report shape matches other failure paths; preserve the
existing preflightCheck/preflightAction values (Name "identity_supported",
Blocking true, etc.) and the return of checks, actions after appendRiskHints.
evandance
left a comment
evandance
left a comment
There was a problem hiding this comment.
Thanks for the contribution — the implementation itself is clean. Before reviewing line-by-line, though, I'd like to step back: I couldn't find a motivating issue, and a lot of this overlaps with existing lark-cli doctor and the structured runtime error envelope (errType + hint + suggested command). Could we open an issue first to align on the concrete agent scenario this targets, and how the new subcommand fits next to those existing surfaces? Once the product direction is settled, the implementation here is in good shape to build on.
3 participants
Summary
Add
lark-cli doctor preflightto check whether a shortcut is ready before execution.The CLI already performs strong runtime checks for config, identity, login, scopes, and strict mode, but callers currently discover those prerequisites only after attempting the real shortcut. This command surfaces the same readiness information earlier and returns structured next actions for both humans and AI agents.
Changes
doctor preflight <service> <shortcut>--as auto|user|bot--format json|prettyScope
This PR is intentionally limited to shortcuts and does not change the real execution pipeline.
Testing
Risk
Low risk, additive only.
Summary by CodeRabbit
New Features
doctor preflightcommand to verify whether a shortcut is ready to run (checks config, identity, login/token, scopes, and risk hints); supports JSON and human-readable output.Documentation
Tests