Repository navigation
refactor: replace the package-level run state with a struct [CLI-1986] - #7412
Conversation
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
This comment has been minimized.
This comment has been minimized.
7f9026a to
3f22a70
Compare
This comment has been minimized.
This comment has been minimized.
…[CLI-1986] The context of a run was a field of the run, which the containedctx linter rejects. The commands now take it from cobra, which carries it from ExecuteContext, and the teardown and the error output are given it as a parameter. The teardown context derives from it, so the requests of the teardown carry the interaction id of the run like all other requests. refactor: replace the package-level run state with a struct [CLI-1986] The engine, the configuration, the logger, the context and the help flag of a run were package-level variables, reachable from everywhere, and tests had to set and reset them. They now live in a cliRun that execute creates once per run and hands to the commands, to the teardown and to the helpers that used the variables. Everything that needs them is a method of cliRun, so every function says what it uses. execute returns the run together with the exit code, so tests inspect the configuration of the run they started instead of a global. No behavior changes. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
3f22a70 to
30ee340
Compare
PR Reviewer Guide 🔍
|
|
| config := r.config | ||
| networkAccess := r.engine.GetNetworkAccess() |
There was a problem hiding this comment.
nit: you could just change usage in the method instead of assigning the old names.
basti-snyk
left a comment
basti-snyk
left a comment
There was a problem hiding this comment.
Review panel (AI-generated, 11 reviewer lenses)
Scope: 6 files in cliv2/pkg/core, +332/-289. Tests touched: yes. Public API touched: no. Run and WithAdditionalExtensions keep their signatures, and the only callers outside the package (cliv2/cmd, cliv2-private/cmd) use nothing else.
Size: Large
Verdict: COMMENT. The refactor is mechanical, and every lens found the same runtime behaviour as main. No package-level engine, config, logger, context, help flag or interaction id is left. Only internalOS and buildType remain, both set at build time. The PR is large, but the tests are thorough and nothing outside the package changes, so nothing here blocks. One question to answer before merge: the teardown context (inline at main.go:554). Eight of eleven lenses raised it independently.
Raised by several lenses (non-blocking, so not posted inline):
- The interaction id goes through
AssembleUrnFromUUIDagain atmain.go:622,690and729, even thoughmain.go:611already stores a URN. GAF returns the input unchanged when it already has the prefix, so this is dead code, and the field comment atmain.go:101reads as if the value were a raw UUID. Raised by 9 lenses. - Locals that only rename fields:
debugLogger := r.logger(instrumentation.go:93),eng, logger := r.engine, r.logger(instrumentation.go:126),config := r.config(logheaderfooter.go:34,:102).sendInstrumentationnow reads botheng.GetConfiguration()andr.config, and the test feeds it two different configs, so it can't catch them drifting apart. Raised by 8 lenses. - The
cliRuncomment (main.go:93) says onlyinternalOSstays outside the struct, butbuildType(debug.go:16) is injected with-Xtoo. Raised by 7 lenses. setupTearDownRunbuilds a second discard logger (main_test.go:1043-1044) right afternewTestRun()made one. Raised by 6 lenses.logheaderfooter.go:74declaresr, _ := regexp.Compile(...)inside what is now a(r *cliRun)method, which shadows the receiver. Rename it tore.- Some helpers became methods while others (
runWorkflowAndProcessData,handleRetryNotification,populateRedactionTerms,tearDown'sstartTime/ua/cliAnalytics) still take pieces of the run as arguments. Pick one rule, here or in PR 2. - Follow-up:
errorListanderrorListMutexare still locals inexecutethat closures mutate, andos.Argsis still read directly, so the tests can't got.Parallel()yet.
PR description: the only test evidence is "62 pass / 50 fail on both". Please name the six specs so a reviewer can rerun them. A concrete manual check would help too, e.g. diff the snyk whoami -d header, footer and exit code before and after. The description should also mention that context now flows through ExecuteContext and cmd.Context(). That's equivalent today, but it's more than a rename.
FYI, unrelated to this PR: Test_mainWithErrorCode/outputs_an_error_list (main_test.go:113) fails locally on both this branch and the merge base (expected 2, actual 0). execute reads the developer's stored credentials, which win over SNYK_TOKEN=invalidToken. We couldn't run -race here because there is no cgo toolchain.
🤖 Generated with Claude Code
| func (r *cliRun) tearDown(ctx context.Context, err error, errorList []error, startTime time.Time, ua networking.UserAgentInfo, cliAnalytics analytics.Analytics) int { | ||
| // Create a context with timeout for teardown operations to ensure we don't hang indefinitely | ||
| teardownCtx, cancel := context.WithTimeout(context.Background(), teardownTimeout) | ||
| teardownCtx, cancel := context.WithTimeout(ctx, teardownTimeout) |
There was a problem hiding this comment.
[scenario-walker, architect, rigor-reviewer, error-path-reviewer, diff-auditor, security-escalator, necessity-hawk, consumer-advocate] should-fix: teardownCtx used to come from context.Background(). Now it comes from the run's ctx. Today that changes nothing, because ctx is Background plus a value and nothing cancels it. But the doc comment two lines up says teardown also runs on signal-triggered exit. Once anyone makes ctx cancellable (signal.NotifyContext, a run-level timeout), teardown starts out cancelled. Analytics, instrumentation and GLOBAL_CLEANUP would then fail quietly on exactly the exits where we need them. tearDown only needs ctx to pass the interaction id to displayError. Suggestion:
teardownCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), teardownTimeout)Or keep context.Background() here and pass ctx only to displayError. Either way, a PR that claims no behaviour change shouldn't move this as a side effect. No test pins it, because the teardown tests pass t.Context().
|
|
||
| // run the extensible cli | ||
| err = rootCommand.Execute() | ||
| err = rootCommand.ExecuteContext(ctx) |
There was a problem hiding this comment.
[scenario-walker] should-fix, needs a test: Execute() → ExecuteContext(ctx) together with globalContext → cmd.Context() (line 225) is the one piece of new wiring here, and nothing tests it. setupMainWorkflowTestEnv sets cmd.SetContext(t.Context()) itself. Test_CreateCommandsForWorkflowWithSubcommands calls RunE directly with a nil context, which only passes because GAF's WithContext ignores nil. If someone reverts this to Execute(), every workflow loses InteractionIdKey from its context and every test stays green. One test that runs the root command through ExecuteContext and checks that the invoked workflow's invocation.Context() carries the interaction id would pin it.
| return out.String() | ||
| } | ||
|
|
||
| func Test_writeLogHeader_showsInteractionIdOfTheRun(t *testing.T) { |
There was a problem hiding this comment.
[duplication-hunter, fixture-reviewer] should-fix: this new test repeats the setup in writeLogHeaderWithMachineID just above it almost line by line: config, engine, SetRuntimeInfo, buffer logger, newTestRun, wiring. It even wires the run differently, with one tuple assignment here and three separate assignments in the helper. Why do we do the same thing two ways? Could the helper take the interaction id, or return the run before writeLogHeader(), so both tests reuse it? Small one: the assertion on line 49 hardcodes the %-22s padding. assert.Regexp(t, Interaction:\s+run-1234, ...) checks the same thing and won't break when the column width changes.
|
|
||
| userInterface := mocks.NewMockUserInterface(gomock.NewController(t)) | ||
| userInterface.EXPECT().Output(gomock.Any()).DoAndReturn(func(out string) error { | ||
| assert.Contains(t, out, `"path": "/some/dir"`) |
There was a problem hiding this comment.
[assertion-sharpener, architect] should-fix: could we have a more definitive assertion here? assert.Contains(out, "path": "/some/dir") depends on MarshalIndent formatting and only proves the text shows up somewhere. Unmarshal out into JsonErrorStruct and assert Ok == false, Path == "/some/dir" and the exact ErrorMsg. Then a wrong field or a broken shape fails the test.
| @@ -91,20 +92,18 @@ func Test_isHtmlOutput(t *testing.T) { | |||
| } | |||
|
|
|||
| func Test_mainWithErrorCode(t *testing.T) { | |||
There was a problem hiding this comment.
[naming-reviewer, rigor-reviewer] should-fix: Test_mainWithErrorCode now calls execute(nil) in its main body (line 99). Only the subtest at line 113 still goes through mainWithErrorCode. The name no longer matches what most of the test exercises. Rename it to Test_execute, or split it so each test is named after the function it calls. Alternatively, Run could call execute directly and mainWithErrorCode could go away, which leaves one entry point.
| globalConfiguration = configuration.NewWithOpts() | ||
| globalConfiguration.Set(configuration.ANALYTICS_DISABLED, true) | ||
| globalEngine = workflow.NewWorkFlowEngine(globalConfiguration) | ||
| r := newTestRun() |
There was a problem hiding this comment.
[duplication-hunter] should-fix: newTestRun(), then a new config, a Set, then NewWorkFlowEngine(r.config). This pattern now appears here and, nearly verbatim, at lines 321, 385, 623 and 711. The duplication predates this PR, but the PR rewrites every one of those lines and adds newTestRun, so this is the cheap moment to fold it in. Can we reduce it? For example a newTestRunWithEngine(config) that sets both config and engine.
| authorization, _, userAgent := logHeaderAuthorizationInfo(config, networkAccess) | ||
| func (r *cliRun) writeLogHeader() { | ||
| config := r.config | ||
| authorization, _, userAgent := r.logHeaderAuthorizationInfo() |
There was a problem hiding this comment.
I wonder, if we can get rid of the oAuthEnabled return param.
| // cliRun is what one run of the CLI works on. It is created once per run and handed to the commands and to the | ||
| // teardown, so every function says what it uses. Only what is injected at build time (internalOS) is kept | ||
| // outside of it. | ||
| type cliRun struct { |
| initDebugBuild() | ||
|
|
||
| noopLogger := zerolog.New(io.Discard) | ||
| r := &cliRun{logger: &noopLogger, interactionId: instrumentation.AssembleUrnFromUUID(uuid.NewString())} |
There was a problem hiding this comment.
Why a noopLogger instead of a real one?
| type cliRun struct { | ||
| engine workflow.Engine | ||
| config configuration.Configuration | ||
| logger *zerolog.Logger |
There was a problem hiding this comment.
why not just have an accessor Method Logger() that takes the engine logger?
| }) | ||
| networkAccess.AddHeaderField("x-snyk-cli-version", cliv2.GetFullVersion()) | ||
| networkAccess.AddHeaderField("snyk-interaction-id", instrumentation.AssembleUrnFromUUID(interactionId)) | ||
| networkAccess.AddHeaderField("snyk-interaction-id", instrumentation.AssembleUrnFromUUID(r.interactionId)) |
There was a problem hiding this comment.
we probably need to think about how this can be set by top-level workflow indication rather than CLI main method invocation, e.g. through a pre-invoke hook.
| // newTestRun returns what a run of the CLI works on, with a quiet logger. Tests set the configuration and the | ||
| // engine they need. | ||
| func newTestRun() *cliRun { | ||
| logger := zerolog.New(io.Discard) |
There was a problem hiding this comment.
why not nooplogger here?
basti-snyk
left a comment
basti-snyk
left a comment
There was a problem hiding this comment.
Nice one! Some questions and nits, but nothing blocking.
CLI-1986
Pull Request Submission Checklist
What does this PR do?
The engine, configuration, logger, context and help flag of a run were package-level variables that tests had to set and reset. They now live in a
cliRun, created once per run byexecuteand handed to the commands, the teardown and the helpers, which became its methods.executereturns the run with the exit code, so tests inspect the run they started.No behavior changes. First of a stack of 2; independent of the other.
Where should the reviewer start?
cliRunandexecuteincliv2/pkg/core/main.go. The rest moves functions to methods and tests tonewTestRun.How should this be manually tested?
Not necessary. As a regression check, six existing acceptance specs (112 tests) give identical results with the
mainbinary and this branch: 62 pass and 50 fail on both, the latter because the legacy CLI binary is missing locally.What's the product update that needs to be communicated to CLI users?
None.
Risk assessment (Low | Medium | High)?
Low. A mechanical refactor covered by the unit tests.
🤖 Generated with Claude Code
Note
Low Risk
Mechanical state encapsulation with broad test updates; intended behavior parity with no new auth or network paths.
Overview
Replaces package-level run state (
globalEngine,globalConfiguration,globalLogger,globalContext,helpProvided,interactionId) with a per-invocationcliRunstruct created in newexecute, which returns the run plus exit code for tests.Command wiring, teardown, analytics/instrumentation, debug log header/footer, and error display are
cliRunmethods that readr.config,r.engine, andr.loggerinstead of globals.interactionIdlives on the run (debug header showsr.interactionId). Teardown and Cobra execution thread a singlecontextthroughExecuteContext,tearDown, anddisplayError; teardown’s timeout is derived from that context rather than alwayscontext.Background().Tests drop global
cleanup()in favor ofnewTestRun()and assert against thecliRunreturned fromexecute; a few tests lock in per-run interaction id and JSON errorpathbehavior.Reviewed by Cursor Bugbot for commit 30ee340. Bugbot is set up for automated code reviews on this repo. Configure here.