chore: linting a mix of problems new to this branch and preexisting in the last refactor

  - rename package output_workflow > outputworkflow
  - relax some linters to match GAF
  - wrap errors
  - add many missing comments
  - refactor RunTest() and others to reduce complexity
  - import ordering
  - check bounds before casting ints (on uniqueCount)
  - remove duplicate severity color function
  - change CAPS naming on consts to match Go styling
  - follow guidelines to tighten security on file writes -> 0x644 to 0x600
  - Go naming for Id -> ID and Json -> JSON
  - ignore ireturn warning since we *want* to return an interface in our imported template write function
  - reorder parameters
  - ... more I'm forgetting

Author: bsalomon-snyk

.golangci.yaml

@@ -36,10 +36,6 @@ linters-settings:
     enable-all: true
     disable:
       - fieldalignment
-  ireturn:
-    allow:
-      - error
-      - github.com/snyk/go-application-framework/pkg/workflow.Data
   lll:
     line-length: 160
   misspell:
@@ -122,7 +118,8 @@ linters:
     - goprintffuncname
     - gosec
     - interfacebloat
-    - ireturn
+    # TODO(ireturn): revisit in a followup; required for the output workflow
+    #- ireturn
     - lll
     - misspell
     - nakedret
@@ -152,6 +149,11 @@ linters:
 
 issues:
   exclude-rules:
+    - path: internal/legacy/definitions/oapi\.gen\.go
+      linters:
+        - godot
+        - tagliatelle
+        - wrapcheck
     - path: _test\.go
       linters:
         - bodyclose
@@ -163,4 +165,4 @@ issues:
         - testpackage
   include:
     - EXC0012
-    - EXC0014
+    - EXC0014

internal/bundlestore/client.go

@@ -62,13 +62,15 @@ func NewClient(httpClient *http.Client, csConfig CodeClientConfig, cScanner code
 	}
 }
 
+// host returns the appropriate host URL based on configuration.
 func (c *HTTPClient) host() string {
 	if c.CodeClientConfig.IsFedramp() {
 		return c.SnykApi() + "/hidden/orgs/" + c.CodeClientConfig.Organization() + "/code"
 	}
 	return c.SnykCodeApi()
 }
 
+// request sends an HTTP request to the bundle store.
 func (c *HTTPClient) request(
 	ctx context.Context,
 	method string,
@@ -117,6 +119,8 @@ func (c *HTTPClient) request(
 	return responseBody, nil
 }
 
+// createBundle creates a new bundle with the given file hashes.
+//
 //nolint:gocritic // Code copied verbatim from code-client-go
 func (c *HTTPClient) createBundle(ctx context.Context, fileHashes map[string]string) (string, []string, error) {
 	requestBody, err := json.Marshal(fileHashes)
@@ -137,6 +141,8 @@ func (c *HTTPClient) createBundle(ctx context.Context, fileHashes map[string]str
 	return bundle.BundleHash, bundle.MissingFiles, nil
 }
 
+// extendBundle extends an existing bundle with new or removed files.
+//
 //nolint:gocritic // Code copied verbatim from code-client-go
 func (c *HTTPClient) extendBundle(ctx context.Context, bundleHash string, files map[string]BundleFile, removedFiles []string) (string, []string, error) {
 	requestBody, err := json.Marshal(ExtendBundleRequest{

internal/commands/ostest/depgraph_flow.go

@@ -17,7 +17,7 @@ import (
 	"github.com/snyk/cli-extension-os-flows/internal/errors"
 	"github.com/snyk/cli-extension-os-flows/internal/flags"
 	"github.com/snyk/cli-extension-os-flows/internal/legacy/definitions"
-	"github.com/snyk/cli-extension-os-flows/internal/output_workflow"
+	"github.com/snyk/cli-extension-os-flows/internal/outputworkflow"
 )
 
 // RunUnifiedTestFlow handles the unified test API flow.
@@ -38,50 +38,63 @@ func RunUnifiedTestFlow(
 	if err != nil {
 		return nil, err
 	}
-	var allLegacyFindings []definitions.LegacyVulnerabilityResponse
-	var allOutputData []workflow.Data
 
 	localPolicy := createLocalPolicy(riskScoreThreshold, severityThreshold)
 
+	allLegacyFindings, allOutputData, err := testAllDepGraphs(
+		ctx,
+		ictx,
+		testClient,
+		orgID,
+		errFactory,
+		logger,
+		localPolicy,
+		depGraphs,
+		displayTargetFiles,
+	)
+	if err != nil {
+		return nil, err
+	}
+
+	//nolint:contextcheck // The handleOutput call chain is not context-aware
+	return handleOutput(ictx, allLegacyFindings, allOutputData, errFactory)
+}
+
+func testAllDepGraphs(
+	ctx context.Context,
+	ictx workflow.InvocationContext,
+	testClient testapi.TestClient,
+	orgID string,
+	errFactory *errors.ErrorFactory,
+	logger *zerolog.Logger,
+	localPolicy *testapi.LocalPolicy,
+	depGraphs []*testapi.IoSnykApiV1testdepgraphRequestDepGraph,
+	displayTargetFiles []string,
+) ([]definitions.LegacyVulnerabilityResponse, []workflow.Data, error) {
+	var allLegacyFindings []definitions.LegacyVulnerabilityResponse
+	var allOutputData []workflow.Data
+
 	for i, depGraph := range depGraphs {
 		displayTargetFile := ""
 		if i < len(displayTargetFiles) {
 			displayTargetFile = displayTargetFiles[i]
 		}
 
-		// Create depgraph subject
-		depGraphSubject := testapi.DepGraphSubjectCreate{
-			Type:     testapi.DepGraphSubjectCreateTypeDepGraph,
-			DepGraph: depGraph,
-			Locator: testapi.LocalPathLocator{
-				Paths: []string{displayTargetFile},
-				Type:  testapi.LocalPath,
-			},
-		}
-
-		// Create test subject with depgraph
-		var subject testapi.TestSubjectCreate
-		err = subject.FromDepGraphSubjectCreate(depGraphSubject)
+		subject, err := createTestSubject(depGraph, displayTargetFile)
 		if err != nil {
-			return nil, fmt.Errorf("failed to create test subject: %w", err)
-		}
-
-		// Project name assigned as follows: --project-name || config project name || scannedProject?.depTree?.name
-		// TODO: use project name from Config file
-		config := ictx.GetConfiguration()
-		projectName := config.GetString(flags.FlagProjectName)
-		if projectName == "" && len(depGraph.Pkgs) > 0 {
-			projectName = depGraph.Pkgs[0].Info.Name
+			return nil, nil, err
 		}
 
+		projectName := getProjectName(ictx, depGraph)
 		packageManager := depGraph.PkgManager.Name
 		depCount := max(0, len(depGraph.Pkgs)-1)
 
 		// Run the test with the depgraph subject
-		legacyFinding, outputData, err := RunTest(ctx, ictx, testClient, subject, projectName, packageManager, depCount, displayTargetFile, orgID, errFactory, logger, localPolicy)
-
+		legacyFinding, outputData, err := RunTest(
+			ctx, ictx, testClient, subject, projectName, packageManager, depCount,
+			displayTargetFile, orgID, errFactory, logger, localPolicy)
 		if err != nil {
-			return nil, err
+			return nil, nil, err
 		}
 
 		if legacyFinding != nil {
@@ -90,59 +103,92 @@ func RunUnifiedTestFlow(
 		allOutputData = append(allOutputData, outputData...)
 	}
 
+	return allLegacyFindings, allOutputData, nil
+}
+
+func createTestSubject(
+	depGraph *testapi.IoSnykApiV1testdepgraphRequestDepGraph,
+	displayTargetFile string,
+) (testapi.TestSubjectCreate, error) {
+	// Create depgraph subject
+	depGraphSubject := testapi.DepGraphSubjectCreate{
+		Type:     testapi.DepGraphSubjectCreateTypeDepGraph,
+		DepGraph: *depGraph,
+		Locator: testapi.LocalPathLocator{
+			Paths: []string{displayTargetFile},
+			Type:  testapi.LocalPath,
+		},
+	}
+
+	// Create test subject with depgraph
+	var subject testapi.TestSubjectCreate
+	err := subject.FromDepGraphSubjectCreate(depGraphSubject)
+	if err != nil {
+		return subject, fmt.Errorf("failed to create test subject: %w", err)
+	}
+	return subject, nil
+}
+
+func getProjectName(
+	ictx workflow.InvocationContext,
+	depGraph *testapi.IoSnykApiV1testdepgraphRequestDepGraph,
+) string {
+	// Project name assigned as follows: --project-name || config project name || scannedProject?.depTree?.name
+	// TODO: use project name from Config file
+	config := ictx.GetConfiguration()
+	projectName := config.GetString(flags.FlagProjectName)
+	if projectName == "" && len(depGraph.Pkgs) > 0 {
+		projectName = depGraph.Pkgs[0].Info.Name
+	}
+	return projectName
+}
+
+func handleOutput(
+	ictx workflow.InvocationContext,
+	allLegacyFindings []definitions.LegacyVulnerabilityResponse,
+	allOutputData []workflow.Data,
+	errFactory *errors.ErrorFactory,
+) ([]workflow.Data, error) {
 	config := ictx.GetConfiguration()
 	jsonOutput := config.GetBool("json")
-	jsonFileOutput := config.GetString(output_workflow.OUTPUT_CONFIG_KEY_JSON_FILE)
+	jsonFileOutput := config.GetString(outputworkflow.OutputConfigKeyJSONFile)
 
 	// Human-readable output is suppressed only when --json is specified.
 	wantsHumanReadable := !jsonOutput
+	wantsJSONFile := jsonFileOutput != ""
+	wantsJSONStdOut := jsonOutput
 
 	var finalOutput []workflow.Data
 	if wantsHumanReadable {
-		outputDestination := output_workflow.NewOutputDestination()
+		outputDestination := outputworkflow.NewOutputDestination()
 		// The output workflow returns data it did not handle, like test summaries for exit code calculation.
-		remainingData, err := output_workflow.OutputWorkflowEntryPoint(ictx, allOutputData, outputDestination)
+		remainingData, err := outputworkflow.EntryPoint(ictx, allOutputData, outputDestination)
 		if err != nil {
-			return nil, err
+			return nil, fmt.Errorf("failed to process output workflow: %w", err)
 		}
 		finalOutput = append(finalOutput, remainingData...)
 	}
 
 	// Handle JSON output to a file or stdout.
-	wantsJSONFile := jsonFileOutput != ""
-	wantsJSONStdOut := jsonOutput
-
-	if wantsJSONFile || wantsJSONStdOut {
-		if len(allLegacyFindings) > 0 {
-			var findingsData any
-			if len(allLegacyFindings) == 1 {
-				findingsData = allLegacyFindings[0]
-			} else {
-				findingsData = allLegacyFindings
-			}
-
-			var buffer bytes.Buffer
-			encoder := json.NewEncoder(&buffer)
-			encoder.SetEscapeHTML(false)
-			encoder.SetIndent("", "  ")
-			if err = encoder.Encode(findingsData); err != nil {
-				return nil, errFactory.NewLegacyJSONTransformerError(fmt.Errorf("marshaling to json: %w", err))
-			}
-			// encoder.Encode adds a newline, which we trim to match Marshal's behavior.
-			findingsBytes := bytes.TrimRight(buffer.Bytes(), "\n")
+	if !wantsJSONFile && !wantsJSONStdOut || len(allLegacyFindings) == 0 {
+		return finalOutput, nil
+	}
 
-			if wantsJSONFile {
-				if err := os.WriteFile(jsonFileOutput, findingsBytes, 0644); err != nil {
-					return nil, fmt.Errorf("failed to write JSON output to file: %w", err)
-				}
-			}
+	jsonBytes, err := prepareJSONOutput(allLegacyFindings, errFactory)
+	if err != nil {
+		return nil, err
+	}
 
-			if wantsJSONStdOut {
-				finalOutput = append(finalOutput, NewWorkflowData(ApplicationJSONContentType, findingsBytes))
-			}
+	if wantsJSONFile {
+		if err := os.WriteFile(jsonFileOutput, jsonBytes, 0o600); err != nil {
+			return nil, fmt.Errorf("failed to write JSON output to file: %w", err)
 		}
 	}
 
+	if wantsJSONStdOut {
+		finalOutput = append(finalOutput, NewWorkflowData(ApplicationJSONContentType, jsonBytes))
+	}
+
 	// If only JSON output to stdout was requested, we still need the summary for the exit code.
 	if wantsJSONStdOut && !wantsHumanReadable {
 		for _, d := range allOutputData {
@@ -155,24 +201,46 @@ func RunUnifiedTestFlow(
 	return finalOutput, nil
 }
 
+func prepareJSONOutput(
+	allLegacyFindings []definitions.LegacyVulnerabilityResponse,
+	errFactory *errors.ErrorFactory,
+) ([]byte, error) {
+	if len(allLegacyFindings) == 0 {
+		return nil, nil
+	}
+
+	var findingsData any
+	if len(allLegacyFindings) == 1 {
+		findingsData = allLegacyFindings[0]
+	} else {
+		findingsData = allLegacyFindings
+	}
+
+	var buffer bytes.Buffer
+	encoder := json.NewEncoder(&buffer)
+	encoder.SetEscapeHTML(false)
+	encoder.SetIndent("", "  ")
+	if err := encoder.Encode(findingsData); err != nil {
+		return nil, errFactory.NewLegacyJSONTransformerError(fmt.Errorf("marshaling to json: %w", err))
+	}
+	// encoder.Encode adds a newline, which we trim to match Marshal's behavior.
+	return bytes.TrimRight(buffer.Bytes(), "\n"), nil
+}
+
 // Create local policy only if risk score or severity threshold are specified.
 func createLocalPolicy(riskScoreThreshold *uint16, severityThreshold *testapi.Severity) *testapi.LocalPolicy {
 	if riskScoreThreshold == nil && severityThreshold == nil {
 		return nil
 	}
 
-	localPolicy := &testapi.LocalPolicy{}
-	if riskScoreThreshold != nil {
-		localPolicy.RiskScoreThreshold = riskScoreThreshold
-	}
-	if severityThreshold != nil {
-		localPolicy.SeverityThreshold = severityThreshold
+	return &testapi.LocalPolicy{
+		RiskScoreThreshold: riskScoreThreshold,
+		SeverityThreshold:  severityThreshold,
 	}
-	return localPolicy
 }
 
 // createDepGraphs creates depgraphs from the file parameter in the context.
-func createDepGraphs(ictx workflow.InvocationContext) ([]testapi.IoSnykApiV1testdepgraphRequestDepGraph, []string, error) {
+func createDepGraphs(ictx workflow.InvocationContext) ([]*testapi.IoSnykApiV1testdepgraphRequestDepGraph, []string, error) {
 	depGraphResult, err := service.GetDepGraph(ictx)
 	if err != nil {
 		return nil, nil, fmt.Errorf("failed to get dependency graph: %w", err)
@@ -182,15 +250,15 @@ func createDepGraphs(ictx workflow.InvocationContext) ([]testapi.IoSnykApiV1test
 		return nil, nil, fmt.Errorf("no dependency graphs found")
 	}
 
-	depGraphs := make([]testapi.IoSnykApiV1testdepgraphRequestDepGraph, len(depGraphResult.DepGraphBytes))
+	depGraphs := make([]*testapi.IoSnykApiV1testdepgraphRequestDepGraph, len(depGraphResult.DepGraphBytes))
 	for i, depGraphBytes := range depGraphResult.DepGraphBytes {
 		var depGraphStruct testapi.IoSnykApiV1testdepgraphRequestDepGraph
 		err = json.Unmarshal(depGraphBytes, &depGraphStruct)
 		if err != nil {
 			return nil, nil,
 				fmt.Errorf("unmarshaling depGraph from args failed: %w", err)
 		}
-		depGraphs[i] = depGraphStruct
+		depGraphs[i] = &depGraphStruct
 	}
 
 	return depGraphs, depGraphResult.DisplayTargetFiles, nil

internal/commands/ostest/sbom_reachability_flow.go

@@ -47,6 +47,7 @@ func RunSbomReachabilityFlow(
 	return nil, nil // TODO: return something meaningful once this function is complete
 }
 
+// validateDirectory checks if the given path exists and contains files.
 func validateDirectory(sourceCodePath string, logger *zerolog.Logger, errFactory *errors.ErrorFactory) error {
 	exists, err := dirExists(sourceCodePath)
 	if err != nil {
@@ -68,6 +69,7 @@ func validateDirectory(sourceCodePath string, logger *zerolog.Logger, errFactory
 	return nil
 }
 
+// dirExists checks if the given path exists as a directory.
 func dirExists(path string) (bool, error) {
 	_, err := os.Stat(path)
 	if err != nil {
@@ -80,6 +82,7 @@ func dirExists(path string) (bool, error) {
 	return true, nil
 }
 
+// dirContainsFiles checks if the given directory contains any files.
 func dirContainsFiles(path string) (bool, error) {
 	entries, err := os.ReadDir(path)
 	if err != nil {