refactor: split Execute into Build + Execute with explicit IO and keychain injection#371
refactor: split Execute into Build + Execute with explicit IO and keychain injection#371
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:
📝 WalkthroughWalkthroughExtracts CLI construction into a reusable Build(ctx, inv, opts...) entry point; refactors Factory to accept injected IOStreams and lazy keychain factory; adds SystemIO() and SetDefaultFS(fs); updates callers/tests and wires strict-mode filtering into schema command behavior. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Build as Build (cmd/build.go)
participant SystemIO as SystemIO()
participant NewDefault as NewDefault(streams, inv)
participant Factory as Factory
participant Keychain as KeychainFactory
participant Cobra as Cobra Root Cmd
Caller->>Build: Build(ctx, inv, opts...)
alt WithIO provided
Build->>Build: apply WithIO streams
else
Build->>SystemIO: SystemIO()
SystemIO-->>Build: *IOStreams
end
Build->>NewDefault: NewDefault(streams, inv)
NewDefault-->>Factory: returns *Factory (with IOStreams)
Build->>Factory: apply WithKeychain? (replace kc factory)
Factory->>Keychain: keychain() (lazy resolve when needed)
Build->>Cobra: construct root cmd, wire ctx & IO, register subcommands
Build-->>Caller: return *cobra.Command
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Greptile SummaryThis PR splits The refactor is architecturally sound. The one noteworthy concern is that the centralized Confidence Score: 5/5Safe to merge; no correctness or security issues introduced — all findings are P2 style/performance concerns. The refactor correctly migrates IO injection and keychain late-binding. The keychain closure pattern is applied consistently through the credential chain. The only new issues are O(N) config reads during command registration (performance, not correctness) and the non-thread-safe global FS setter. All remaining findings are P2. internal/cmdutil/identity_flag.go — ResolveStrictMode called once per command registration, causing repeated disk reads. Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller
participant build.go
participant cmdutil
participant credential
participant fs as core/vfs
Caller->>build.go: Build(ctx, inv, WithIO(...), WithKeychain(kc))
build.go->>build.go: apply BuildOptions → buildConfig
build.go->>cmdutil: NewDefault(streams, inv)
cmdutil->>cmdutil: f.Keychain = keychain.Default()
cmdutil->>cmdutil: buildCredentialProvider(Keychain closure)
build.go->>build.go: f.Keychain = cfg.keychain (late bind)
loop For each service method & shortcut
build.go->>cmdutil: AddAPIIdentityFlag(ctx, cmd, f, ...)
cmdutil->>credential: f.ResolveStrictMode(ctx) → ResolveAccount
credential->>fs: LoadMultiAppConfig() [disk read]
fs-->>credential: MultiAppConfig
credential-->>cmdutil: StrictMode
cmdutil->>cmdutil: register --as flag (hidden or visible)
end
build.go->>credential: f.ResolveStrictMode(ctx) [disk read again]
build.go->>build.go: pruneForStrictMode(rootCmd, mode)
build.go-->>Caller: *cobra.Command
Caller->>build.go: rootCmd.Execute()
build.go->>credential: ResolveAccount (first real credential use)
credential->>fs: LoadMultiAppConfig() [disk read again]
Reviews (5): Last reviewed commit: "fix: align strict-mode completion and bu..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
🚀 PR Preview Install Guide🧰 CLI updatenpm i -g https://pkg.pr.new/larksuite/cli/@larksuite/cli@719073aed5ff7f3bcccc3ff39d401dfc35faf8c0🧩 Skill updatenpx skills add larksuite/cli#refactor/build-execute-split -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: 3
🧹 Nitpick comments (2)
internal/cmdutil/factory_http_test.go (1)
12-40: UseTestFactoryinstead of hand-rolledFactoryvalues in these tests.These three tests now build minimal
Factoryliterals inline. That works today, but it bypasses the repo’s standard test wiring and makes the tests easier to drift asFactorydefaults evolve. As per coding guidelines**/*_test.go: Usecmdutil.TestFactory(t, config)for creating test factories in Go tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/factory_http_test.go` around lines 12 - 40, Replace the inline Factory literals in the tests that call cachedHttpClientFunc (used in TestCachedHttpClientFunc, TestCachedHttpClientFunc_HasTimeout, and TestCachedHttpClientFunc_HasRedirectPolicy) with the repository-standard test factory: call cmdutil.TestFactory(t, /* config or nil */) to obtain a *Factory and use that for creating fn; this removes the hand-rolled &Factory{IOStreams: &IOStreams{ErrOut: io.Discard}} instances and ensures test wiring and defaults are consistent with future Factory changes.cmd/build.go (1)
39-43: Verify terminal detection implementation with actual IOStreams usage context.The current approach at line 39 infers
IsTerminalonly fromin(stdin). Go best practices recommend checking each stream independently:stdinTTY status matters for interactive input, whilestdout/stderrTTY status matters for rendering decisions (colors, progress spinners). Depending on howcmdutil.IOStreams.IsTerminalis actually used downstream, this single-stream check may cause incorrect behavior when streams have mixed TTY states (e.g., stdin is a TTY but output is redirected).Before refactoring, confirm:
- How is
IOStreams.IsTerminalused throughout the codebase?- Is it for interactive prompts (use
inTTY) or output rendering (useerrOutTTY)?- If the flag controls output behavior, consider deriving it from
errOutinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/build.go` around lines 39 - 43, The terminal-detection currently sets cmdutil.IOStreams.IsTerminal based only on `in`, which can be wrong for output-rendering decisions; inspect how `IOStreams.IsTerminal` is used (interactive prompts vs. output rendering) and then fix by checking TTY status per stream (`in`, `out`, `errOut`) and either (A) derive `IsTerminal` from the stream used for rendering (likely `errOut`) or (B) extend `cmdutil.IOStreams` to expose separate flags such as `IsInTerminal`, `IsOutTerminal`, `IsErrTerminal` and populate them using term.IsTerminal on each file descriptor (`in.(*os.File).Fd()`, `out.(*os.File).Fd()`, `errOut.(*os.File).Fd()`) so downstream code can make correct decisions.
🤖 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/build.go`:
- Around line 67-69: The loop over functional options can panic if a nil option
is present; update the code that iterates over opts (the for _, o := range opts
loop) to guard before invoking o(cfg) by checking that o != nil and either skip
nil entries or filter them out beforehand, ensuring cfg is only passed to
non-nil option functions (references: opts, o, cfg).
In `@cmd/root_integration_test.go`:
- Around line 138-141: The test currently assigns f.IOStreams after calling
cmdutil.NewDefault which lets factory construction (e.g., DefaultTokenProvider
wiring to f.IOStreams.ErrOut) write to real stderr; instead, create the test
buffers and an ioStreams value first and pass it into cmdutil.NewDefault via the
InvocationContext (so the call becomes cmdutil.NewDefault(nil,
cmdutil.InvocationContext{Profile: profile, IOStreams: ioStreams})), and remove
the subsequent f.IOStreams reassignment so all factory-internal logging uses the
test buffers.
In `@internal/cmdutil/factory_default.go`:
- Around line 35-43: NewDefault currently only replaces a nil streams pointer
with SystemIO(), so partially populated IOStreams (e.g., &IOStreams{Out: buf})
leave ErrOut or In nil and can panic; change NewDefault to normalize any non-nil
partial IOStreams by first obtaining a full default via SystemIO() and then
copy/override only the non-nil fields from the incoming streams into that
default before assigning to Factory.IOStreams (refer to NewDefault, IOStreams,
Factory, and f.IOStreams.ErrOut) so the Factory always holds a fully-initialized
IOStreams struct.
---
Nitpick comments:
In `@cmd/build.go`:
- Around line 39-43: The terminal-detection currently sets
cmdutil.IOStreams.IsTerminal based only on `in`, which can be wrong for
output-rendering decisions; inspect how `IOStreams.IsTerminal` is used
(interactive prompts vs. output rendering) and then fix by checking TTY status
per stream (`in`, `out`, `errOut`) and either (A) derive `IsTerminal` from the
stream used for rendering (likely `errOut`) or (B) extend `cmdutil.IOStreams` to
expose separate flags such as `IsInTerminal`, `IsOutTerminal`, `IsErrTerminal`
and populate them using term.IsTerminal on each file descriptor
(`in.(*os.File).Fd()`, `out.(*os.File).Fd()`, `errOut.(*os.File).Fd()`) so
downstream code can make correct decisions.
In `@internal/cmdutil/factory_http_test.go`:
- Around line 12-40: Replace the inline Factory literals in the tests that call
cachedHttpClientFunc (used in TestCachedHttpClientFunc,
TestCachedHttpClientFunc_HasTimeout, and
TestCachedHttpClientFunc_HasRedirectPolicy) with the repository-standard test
factory: call cmdutil.TestFactory(t, /* config or nil */) to obtain a *Factory
and use that for creating fn; this removes the hand-rolled &Factory{IOStreams:
&IOStreams{ErrOut: io.Discard}} instances and ensures test wiring and defaults
are consistent with future Factory changes.
🪄 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: c1353809-ae72-4972-a783-e77b28aa3964
📒 Files selected for processing (9)
cmd/build.gocmd/root.gocmd/root_integration_test.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/factory_http_test.gointernal/cmdutil/iostreams.gointernal/credential/default_provider.gointernal/credential/integration_test.go
| for _, o := range opts { | ||
| o(cfg) | ||
| } |
There was a problem hiding this comment.
Guard against nil functional options to avoid panic.
At Line 68, calling o(cfg) without a nil check can panic if a nil option is passed in opts.
Proposed fix
for _, o := range opts {
- o(cfg)
+ if o != nil {
+ o(cfg)
+ }
}📝 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.
| for _, o := range opts { | |
| o(cfg) | |
| } | |
| for _, o := range opts { | |
| if o != nil { | |
| o(cfg) | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/build.go` around lines 67 - 69, The loop over functional options can
panic if a nil option is present; update the code that iterates over opts (the
for _, o := range opts loop) to guard before invoking o(cfg) by checking that o
!= nil and either skip nil entries or filter them out beforehand, ensuring cfg
is only passed to non-nil option functions (references: opts, o, cfg).
| f := cmdutil.NewDefault(nil, cmdutil.InvocationContext{Profile: profile}) | ||
| stdout := &bytes.Buffer{} | ||
| stderr := &bytes.Buffer{} | ||
| f.IOStreams = &cmdutil.IOStreams{In: nil, Out: stdout, ErrOut: stderr} |
There was a problem hiding this comment.
Pass the test buffers into NewDefault directly.
Swapping f.IOStreams after Line 138 leaves anything captured during factory construction still pointing at the original system streams. In this PR, DefaultTokenProvider is wired with f.IOStreams.ErrOut inside NewDefault, so this helper can silently leak warnings/errors to the real stderr instead of the test buffer.
Suggested fix
- f := cmdutil.NewDefault(nil, cmdutil.InvocationContext{Profile: profile})
stdout := &bytes.Buffer{}
stderr := &bytes.Buffer{}
- f.IOStreams = &cmdutil.IOStreams{In: nil, Out: stdout, ErrOut: stderr}
+ f := cmdutil.NewDefault(&cmdutil.IOStreams{
+ In: nil,
+ Out: stdout,
+ ErrOut: stderr,
+ }, cmdutil.InvocationContext{Profile: profile})
return f, stdout, stderr📝 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.
| f := cmdutil.NewDefault(nil, cmdutil.InvocationContext{Profile: profile}) | |
| stdout := &bytes.Buffer{} | |
| stderr := &bytes.Buffer{} | |
| f.IOStreams = &cmdutil.IOStreams{In: nil, Out: stdout, ErrOut: stderr} | |
| stdout := &bytes.Buffer{} | |
| stderr := &bytes.Buffer{} | |
| f := cmdutil.NewDefault(&cmdutil.IOStreams{ | |
| In: nil, | |
| Out: stdout, | |
| ErrOut: stderr, | |
| }, cmdutil.InvocationContext{Profile: profile}) | |
| return f, stdout, stderr |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/root_integration_test.go` around lines 138 - 141, The test currently
assigns f.IOStreams after calling cmdutil.NewDefault which lets factory
construction (e.g., DefaultTokenProvider wiring to f.IOStreams.ErrOut) write to
real stderr; instead, create the test buffers and an ioStreams value first and
pass it into cmdutil.NewDefault via the InvocationContext (so the call becomes
cmdutil.NewDefault(nil, cmdutil.InvocationContext{Profile: profile, IOStreams:
ioStreams})), and remove the subsequent f.IOStreams reassignment so all
factory-internal logging uses the test buffers.
| func NewDefault(streams *IOStreams, inv InvocationContext) *Factory { | ||
| if streams == nil { | ||
| streams = SystemIO() | ||
| } | ||
| f := &Factory{ | ||
| Keychain: keychain.Default(), | ||
| Invocation: inv, | ||
| } | ||
| f.IOStreams = &IOStreams{ | ||
| In: os.Stdin, | ||
| Out: os.Stdout, | ||
| ErrOut: os.Stderr, | ||
| IsTerminal: term.IsTerminal(int(os.Stdin.Fd())), | ||
| IOStreams: streams, | ||
| } |
There was a problem hiding this comment.
Normalize partial IOStreams inputs before storing them on the factory.
NewDefault only substitutes SystemIO() when streams == nil. With the new public IO injection path, callers can now legally pass a partially filled struct like &IOStreams{Out: buf}. The first warning/error write through f.IOStreams.ErrOut then panics on a nil writer, and Cobra can also inherit nil stdio handles.
Suggested fix
func NewDefault(streams *IOStreams, inv InvocationContext) *Factory {
- if streams == nil {
- streams = SystemIO()
- }
+ sys := SystemIO()
+ if streams == nil {
+ streams = sys
+ } else {
+ if streams.In == nil {
+ streams.In = sys.In
+ }
+ if streams.Out == nil {
+ streams.Out = sys.Out
+ }
+ if streams.ErrOut == nil {
+ streams.ErrOut = sys.ErrOut
+ }
+ }
f := &Factory{
Keychain: keychain.Default(),
Invocation: inv,
IOStreams: streams,
}📝 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.
| func NewDefault(streams *IOStreams, inv InvocationContext) *Factory { | |
| if streams == nil { | |
| streams = SystemIO() | |
| } | |
| f := &Factory{ | |
| Keychain: keychain.Default(), | |
| Invocation: inv, | |
| } | |
| f.IOStreams = &IOStreams{ | |
| In: os.Stdin, | |
| Out: os.Stdout, | |
| ErrOut: os.Stderr, | |
| IsTerminal: term.IsTerminal(int(os.Stdin.Fd())), | |
| IOStreams: streams, | |
| } | |
| func NewDefault(streams *IOStreams, inv InvocationContext) *Factory { | |
| sys := SystemIO() | |
| if streams == nil { | |
| streams = sys | |
| } else { | |
| if streams.In == nil { | |
| streams.In = sys.In | |
| } | |
| if streams.Out == nil { | |
| streams.Out = sys.Out | |
| } | |
| if streams.ErrOut == nil { | |
| streams.ErrOut = sys.ErrOut | |
| } | |
| } | |
| f := &Factory{ | |
| Keychain: keychain.Default(), | |
| Invocation: inv, | |
| IOStreams: streams, | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/cmdutil/factory_default.go` around lines 35 - 43, NewDefault
currently only replaces a nil streams pointer with SystemIO(), so partially
populated IOStreams (e.g., &IOStreams{Out: buf}) leave ErrOut or In nil and can
panic; change NewDefault to normalize any non-nil partial IOStreams by first
obtaining a full default via SystemIO() and then copy/override only the non-nil
fields from the incoming streams into that default before assigning to
Factory.IOStreams (refer to NewDefault, IOStreams, Factory, and
f.IOStreams.ErrOut) so the Factory always holds a fully-initialized IOStreams
struct.
tuxedomm
force-pushed
the
refactor/build-execute-split
branch
from
0bfc2e4 to
dd05477
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
cmd/build.go (2)
67-69:⚠️ Potential issue | 🟡 MinorGuard nil functional options before invocation.
Line 68 can panic if a nil option is present in
opts.Proposed fix
for _, o := range opts { - o(cfg) + if o != nil { + o(cfg) + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/build.go` around lines 67 - 69, The loop invoking functional options in cmd/build.go should guard against nil entries to avoid a panic; in the block that iterates over opts (the for _, o := range opts loop), add a nil-check and only call o(cfg) when o != nil (i.e., skip nil option functions), ensuring that any nil option in the slice won't cause a runtime panic.
37-44:⚠️ Potential issue | 🟠 MajorBackfill nil
WithIOarguments to keep streams always writable.At Line 43,
WithIOstoresout/errOutas-is; passing nil values can break warning/progress/error output routing and runtime writes.Proposed fix
func WithIO(in io.Reader, out, errOut io.Writer) BuildOption { return func(c *buildConfig) { + sys := cmdutil.SystemIO() + if in == nil { + in = sys.In + } + if out == nil { + out = sys.Out + } + if errOut == nil { + errOut = sys.ErrOut + } isTerminal := false if f, ok := in.(*os.File); ok { isTerminal = term.IsTerminal(int(f.Fd())) } c.streams = &cmdutil.IOStreams{In: in, Out: out, ErrOut: errOut, IsTerminal: isTerminal} } }As per coding guidelines "
**/*.go: Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/build.go` around lines 37 - 44, The WithIO function should defensively backfill nil I/O args so streams are always writable: in the WithIO closure (function WithIO) ensure any nil in/out/errOut are replaced with sensible defaults (os.Stdin, os.Stdout, os.Stderr) before computing isTerminal and creating the cmdutil.IOStreams instance on buildConfig; update the IsTerminal check to use the resolved input reader (not the original possibly-nil value) and then assign c.streams = &cmdutil.IOStreams{In: resolvedIn, Out: resolvedOut, ErrOut: resolvedErrOut, IsTerminal: isTerminal} so progress/warnings go to stderr and program output can go to stdout reliably.internal/cmdutil/factory_default.go (1)
35-43:⚠️ Potential issue | 🟠 MajorNormalize partial
IOStreamsinputs before storing onFactory.At Line 36, only a nil pointer is handled. A non-nil
IOStreamswith nilErrOut/Out/Incan still flow through and break warning/error writes later.Proposed fix
func NewDefault(streams *IOStreams, inv InvocationContext) *Factory { - if streams == nil { - streams = SystemIO() - } + sys := SystemIO() + if streams == nil { + streams = sys + } else { + if streams.In == nil { + streams.In = sys.In + } + if streams.Out == nil { + streams.Out = sys.Out + } + if streams.ErrOut == nil { + streams.ErrOut = sys.ErrOut + } + } f := &Factory{ Keychain: keychain.Default(), Invocation: inv, IOStreams: streams, }As per coding guidelines "
**/*.go: Program output (JSON envelopes) must go to stdout; progress, warnings, and hints must go to stderr".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/cmdutil/factory_default.go` around lines 35 - 43, NewDefault currently only replaces a nil *IOStreams; if callers pass a non-nil IOStreams with some nil fields (In/Out/ErrOut) those nils can break later writes. In NewDefault, after the nil-check, obtain defaults := SystemIO() and for any nil field on the incoming streams (streams.In, streams.Out, streams.ErrOut) assign the corresponding defaults field so the Factory stores a fully-populated IOStreams; keep using keychain.Default(), Invocation inv and the normalized streams when constructing the Factory.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/build.go`:
- Around line 67-69: The loop invoking functional options in cmd/build.go should
guard against nil entries to avoid a panic; in the block that iterates over opts
(the for _, o := range opts loop), add a nil-check and only call o(cfg) when o
!= nil (i.e., skip nil option functions), ensuring that any nil option in the
slice won't cause a runtime panic.
- Around line 37-44: The WithIO function should defensively backfill nil I/O
args so streams are always writable: in the WithIO closure (function WithIO)
ensure any nil in/out/errOut are replaced with sensible defaults (os.Stdin,
os.Stdout, os.Stderr) before computing isTerminal and creating the
cmdutil.IOStreams instance on buildConfig; update the IsTerminal check to use
the resolved input reader (not the original possibly-nil value) and then assign
c.streams = &cmdutil.IOStreams{In: resolvedIn, Out: resolvedOut, ErrOut:
resolvedErrOut, IsTerminal: isTerminal} so progress/warnings go to stderr and
program output can go to stdout reliably.
In `@internal/cmdutil/factory_default.go`:
- Around line 35-43: NewDefault currently only replaces a nil *IOStreams; if
callers pass a non-nil IOStreams with some nil fields (In/Out/ErrOut) those nils
can break later writes. In NewDefault, after the nil-check, obtain defaults :=
SystemIO() and for any nil field on the incoming streams (streams.In,
streams.Out, streams.ErrOut) assign the corresponding defaults field so the
Factory stores a fully-populated IOStreams; keep using keychain.Default(),
Invocation inv and the normalized streams when constructing the Factory.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fa2aed7a-e6aa-4953-9965-85347239d904
📒 Files selected for processing (10)
cmd/build.gocmd/init.gocmd/root.gocmd/root_integration_test.gointernal/cmdutil/factory_default.gointernal/cmdutil/factory_default_test.gointernal/cmdutil/factory_http_test.gointernal/cmdutil/iostreams.gointernal/credential/default_provider.gointernal/credential/integration_test.go
✅ Files skipped from review due to trivial changes (2)
- cmd/root_integration_test.go
- internal/cmdutil/factory_default_test.go
🚧 Files skipped from review as they are similar to previous changes (4)
- internal/credential/integration_test.go
- internal/cmdutil/factory_http_test.go
- internal/cmdutil/iostreams.go
- cmd/root.go
tuxedomm
force-pushed
the
refactor/build-execute-split
branch
from
9412170 to
212fbae
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/schema/schema.go (1)
448-453:⚠️ Potential issue | 🟠 MajorApply strict-mode filtering to
schema <service>outputs too.
modeis resolved above, but this branch still renders the raw service spec. In strict mode,schema <service> --format prettygoes throughprintResourceList, and--format jsonreturnsspecunchanged, so methods hidden at the resource/method level are still discoverable one level up.[suggested fix]
♻️ Minimal direction
if len(parts) == 1 { + filteredSpec := filterSpecByStrictMode(spec, mode) if opts.Format == "pretty" { - printResourceList(out, spec) + printResourceList(out, filteredSpec) } else { - output.PrintJson(out, spec) + output.PrintJson(out, filteredSpec) } return nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/schema/schema.go` around lines 448 - 453, The branch handling the single-part service output currently prints the raw spec (using printResourceList and output.PrintJson) and ignores the previously-resolved mode; modify this branch to first compute a filtered spec according to mode (e.g., filteredSpec := filterSpecByMode(spec, mode) or call the existing helper that applies strict-mode filtering) and then pass filteredSpec to printResourceList(out, filteredSpec) for opts.Format == "pretty" and to output.PrintJson(out, filteredSpec) for JSON, ensuring hidden resources/methods are removed before rendering.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/schema/schema.go`:
- Around line 448-453: The branch handling the single-part service output
currently prints the raw spec (using printResourceList and output.PrintJson) and
ignores the previously-resolved mode; modify this branch to first compute a
filtered spec according to mode (e.g., filteredSpec := filterSpecByMode(spec,
mode) or call the existing helper that applies strict-mode filtering) and then
pass filteredSpec to printResourceList(out, filteredSpec) for opts.Format ==
"pretty" and to output.PrintJson(out, filteredSpec) for JSON, ensuring hidden
resources/methods are removed before rendering.
0b3180b to
57db84e
Compare
github-actions
bot
added
size/XL
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/schema/schema.go (1)
448-453:⚠️ Potential issue | 🟠 MajorStrict-mode filtering is skipped for
schema <service>output.When
len(parts) == 1, both pretty and JSON branches return unfiltered service data. This is inconsistent with Line 473/Line 501 behavior and can surface methods that are later hidden/invalid under strict mode.🛠️ Suggested direction
if len(parts) == 1 { + view := spec + if mode.IsActive() { + view = filterSpecMethodsByStrictMode(spec, mode) + } if opts.Format == "pretty" { - printResourceList(out, spec) + printResourceList(out, view) } else { - output.PrintJson(out, spec) + output.PrintJson(out, view) } return nil }// helper (outside this hunk) func filterSpecMethodsByStrictMode(spec map[string]interface{}, mode core.StrictMode) map[string]interface{} { if !mode.IsActive() || spec == nil { return spec } out := make(map[string]interface{}, len(spec)) for k, v := range spec { out[k] = v } resources, _ := spec["resources"].(map[string]interface{}) if resources == nil { return out } resCopy := make(map[string]interface{}, len(resources)) for rn, rv := range resources { rm, _ := rv.(map[string]interface{}) if rm == nil { resCopy[rn] = rv continue } rmCopy := make(map[string]interface{}, len(rm)) for k, v := range rm { rmCopy[k] = v } if methods, ok := rm["methods"].(map[string]interface{}); ok { rmCopy["methods"] = filterMethodsByStrictMode(methods, mode) } resCopy[rn] = rmCopy } out["resources"] = resCopy return out }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/schema/schema.go` around lines 448 - 453, The branch handling len(parts) == 1 returns the unfiltered spec for both pretty and JSON outputs; update it to apply strict-mode filtering before printing by calling a new helper like filterSpecMethodsByStrictMode(spec, opts.StrictMode) (or the existing filterMethodsByStrictMode inside that helper) and then pass the filtered spec to printResourceList(out, filtered) or output.PrintJson(out, filtered) respectively; ensure you add/import core.StrictMode if needed and place the helper (filterSpecMethodsByStrictMode) alongside other helpers so resources->methods are filtered consistently with the other branches.
🧹 Nitpick comments (1)
cmd/schema/schema.go (1)
429-435: Resolve strict mode only after the empty-path early return.On Line 431,
ResolveStrictModeruns even forlark-cli schema(no path), where strict filtering is not used. This adds avoidable account/credential work to a lightweight listing path.♻️ Suggested change
func schemaRun(opts *SchemaOptions) error { out := opts.Factory.IOStreams.Out - mode := opts.Factory.ResolveStrictMode(opts.Ctx) if opts.Path == "" { printServices(out) return nil } + mode := opts.Factory.ResolveStrictMode(opts.Ctx)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/schema/schema.go` around lines 429 - 435, In schemaRun, avoid calling ResolveStrictMode when opts.Path is empty; move the call to opts.Factory.ResolveStrictMode(opts.Ctx) so it occurs after the early-return branch that checks opts.Path == "" and calls printServices(out), ensuring ResolveStrictMode is only executed for non-empty-path flows that need strict filtering rather than for the lightweight listing path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/schema/schema.go`:
- Around line 448-453: The branch handling len(parts) == 1 returns the
unfiltered spec for both pretty and JSON outputs; update it to apply strict-mode
filtering before printing by calling a new helper like
filterSpecMethodsByStrictMode(spec, opts.StrictMode) (or the existing
filterMethodsByStrictMode inside that helper) and then pass the filtered spec to
printResourceList(out, filtered) or output.PrintJson(out, filtered)
respectively; ensure you add/import core.StrictMode if needed and place the
helper (filterSpecMethodsByStrictMode) alongside other helpers so
resources->methods are filtered consistently with the other branches.
---
Nitpick comments:
In `@cmd/schema/schema.go`:
- Around line 429-435: In schemaRun, avoid calling ResolveStrictMode when
opts.Path is empty; move the call to opts.Factory.ResolveStrictMode(opts.Ctx) so
it occurs after the early-return branch that checks opts.Path == "" and calls
printServices(out), ensuring ResolveStrictMode is only executed for
non-empty-path flows that need strict filtering rather than for the lightweight
listing path.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cmd/schema/schema.go (2)
331-355:⚠️ Potential issue | 🟡 MinorKeep tab completion in sync with strict-mode filtering.
schemaRunnow hides identity-incompatible resources/methods, butcompleteSchemaPathstill completes against the unfiltered registry. In strict mode that means shell completion can suggest a path this same command later rejects as unknown. Please thread the factory/context intoValidArgsFunctionand apply the same filtering there.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/schema/schema.go` around lines 331 - 355, ValidArgsFunction currently calls completeSchemaPath against the unfiltered registry, causing suggestions that schemaRun later rejects in strict mode; update NewCmdSchema to pass the Factory/Context into the completion function by wrapping completeSchemaPath with a closure that sets up the same SchemaOptions/ctx/filtering used by schemaRun (reuse SchemaOptions and opts.Ctx or f.Context), so completion applies the same identity-compatible resource/method filtering before returning completions; reference NewCmdSchema, ValidArgsFunction, completeSchemaPath, SchemaOptions, schemaRun and the provided Factory to locate where to thread the context and apply the filter.
473-499:⚠️ Potential issue | 🟠 MajorReturn “unknown resource” when strict mode prunes every method.
After Lines 477/493 filter the methods, this branch still succeeds for a resource whose visible method set is empty. That diverges from
filterSpecByStrictMode, which drops those resources entirely, soschema <service>hides the resource whileschema <service>.<resource>still exposes an empty shell. Please short-circuit here and reuse the existing unknown-resource path once the filtered method set is empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/schema/schema.go` around lines 473 - 499, After filtering methods with filterMethodsByStrictMode in the branch handling opts.Format == "pretty" and the JSON branch, check whether the resulting methods map is empty and, if so, take the same "unknown resource" code path used elsewhere (i.e., short-circuit and print the unknown-resource message/exit) instead of continuing to render an empty resource; update the pretty branch (after methods = filterMethodsByStrictMode(...)) and the JSON branch (after assigning filtered["methods"] = filterMethodsByStrictMode(...)) to detect len(methods)==0 (or len(filtered["methods"].(map[string]interface{}))==0) and reuse the existing unknown-resource handling for consistency with filterSpecByStrictMode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/schema/schema.go`:
- Around line 331-355: ValidArgsFunction currently calls completeSchemaPath
against the unfiltered registry, causing suggestions that schemaRun later
rejects in strict mode; update NewCmdSchema to pass the Factory/Context into the
completion function by wrapping completeSchemaPath with a closure that sets up
the same SchemaOptions/ctx/filtering used by schemaRun (reuse SchemaOptions and
opts.Ctx or f.Context), so completion applies the same identity-compatible
resource/method filtering before returning completions; reference NewCmdSchema,
ValidArgsFunction, completeSchemaPath, SchemaOptions, schemaRun and the provided
Factory to locate where to thread the context and apply the filter.
- Around line 473-499: After filtering methods with filterMethodsByStrictMode in
the branch handling opts.Format == "pretty" and the JSON branch, check whether
the resulting methods map is empty and, if so, take the same "unknown resource"
code path used elsewhere (i.e., short-circuit and print the unknown-resource
message/exit) instead of continuing to render an empty resource; update the
pretty branch (after methods = filterMethodsByStrictMode(...)) and the JSON
branch (after assigning filtered["methods"] = filterMethodsByStrictMode(...)) to
detect len(methods)==0 (or len(filtered["methods"].(map[string]interface{}))==0)
and reuse the existing unknown-resource handling for consistency with
filterSpecByStrictMode.
…mentation Change-Id: If5c3e50e84859f9ac4ffceeb0ac3dc7b7330b274
When strict mode is active, schema output now excludes methods that are incompatible with the forced identity. This applies to both pretty and JSON output formats at the resource and method levels. Change-Id: I39647d5578466c3e23dc545bfb917ae075203ad7
Change-Id: Iec11151c5002c2f58a8aa067d08747db2e4d2d8c
Change-Id: I1d8da0dd4636384203cb748605682677ad1c4d92
tuxedomm
force-pushed
the
refactor/build-execute-split
branch
from
57db84e to
719073a
Compare
1 participant
Summary
Execute()intoBuild()(public, returns*cobra.Command) +buildInternal()(private, also returns*Factory)WithIOfor IO streams,WithKeychainfor secret storage backendSystemIO()constructor instead of hardcodingos.Stdin/Stdout/StderrinsideNewDefaultfunc()closure in credential provider, allowing callers to replacef.Keychainafter Factory constructionWarnIfProxiedto usef.IOStreams.ErrOutinstead ofos.StderrMotivation
Prepare the public API for external consumers (e.g. server mode) that need to:
Test plan
go build ./...passesgo test ./internal/cmdutil/ ./internal/credential/ ./cmd/ -racepassesgolangci-lint run— 0 issues on changed packagesos.Stdin/Stdout/Stderrleaks introducedSummary by CodeRabbit
New Features
Refactor
Tests