fix: Show error messages on fatal failures #32 (#34)

* fix: Show error messages on fatal failures #32

* test: Add E2E test

* test: fix E2E scripts

* test: fix E2E scripts

* fix: Race condition in concurrent analyzer

* fix: update formatting to ensure docs URL is clickable

Signed-off-by: Sahil Bansal <bansalsahil315@gmail.com>

* fix: E2E test

---------

Signed-off-by: Sahil Bansal <bansalsahil315@gmail.com>
Co-authored-by: Sahil Bansal <bansalsahil315@gmail.com>

Author: abhisek

.github/workflows/ci.yml

@@ -36,6 +36,37 @@ jobs:
         with:
           token: ${{ secrets.CODECOV_TOKEN }}
 
+  e2e-test:
+    runs-on: ubuntu-latest
+    timeout-minutes: 15
+    steps:
+      - name: Checkout Source
+        uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 # v4
+
+      - name: Setup Go
+        uses: actions/setup-go@0aaccfd150d50ccaeb58ebd88d36e91967a5f35b # v5
+        with:
+          go-version: 1.24
+          check-latest: true
+
+      - name: Setup Node
+        uses: actions/setup-node@49933ea5288caeca8642d1e84afbd3f7d6820020 # v4
+        with:
+          node-version: 20
+          check-latest: true
+
+      - name: Setup PNPM
+        uses: pnpm/action-setup@a7487c7e89a18df4991f7f222e4898a00d66ddda # v4
+        with:
+          version: 10
+
+      - name: Build Binary
+        run: make
+
+      - name: Run E2E Tests
+        run: chmod +x test/e2e.sh && ./test/e2e.sh
+        shell: bash
+
   goreleaser-test:
     runs-on: ubuntu-latest
     timeout-minutes: 15

analyzer/malysis_active_scan.go

@@ -39,7 +39,7 @@ var _ Analyzer = &malysisActiveScanAnalyzer{}
 
 func NewMalysisActiveScanAnalyzer(config MalysisActiveScanAnalyzerConfig) (*malysisActiveScanAnalyzer, error) {
 	if config.TenantId == "" || config.ApiKey == "" {
-		return nil, fmt.Errorf("active scanning requires SafeDep Cloud authentication credentials")
+		return nil, fmt.Errorf("active scanning requires SafeDep Cloud credentials. See: https://docs.safedep.io/cloud/malware-analysis")
 	}
 
 	headers := http.Header{}

cmd/npm/npm.go

@@ -1,11 +1,13 @@
 package npm
 
 import (
+	"context"
 	_ "embed"
 
 	"github.com/safedep/dry/log"
-	"github.com/safedep/pmg/config"
+	"github.com/safedep/pmg/internal/flows"
 	"github.com/safedep/pmg/internal/ui"
+	"github.com/safedep/pmg/packagemanager"
 	"github.com/spf13/cobra"
 )
 
@@ -15,12 +17,7 @@ func NewNpmCommand() *cobra.Command {
 		Short:              "Guard npm package manager",
 		DisableFlagParsing: true,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			config, err := config.FromContext(cmd.Context())
-			if err != nil {
-				ui.Fatalf("Failed to get config: %s", err)
-			}
-
-			err = executeNpmFlow(cmd.Context(), config, args)
+			err := executeNpmFlow(cmd.Context(), args)
 			if err != nil {
 				log.Errorf("Failed to execute npm flow: %s", err)
 			}
@@ -29,3 +26,12 @@ func NewNpmCommand() *cobra.Command {
 		},
 	}
 }
+
+func executeNpmFlow(ctx context.Context, args []string) error {
+	packageManager, err := packagemanager.NewNpmPackageManager(packagemanager.DefaultNpmPackageManagerConfig())
+	if err != nil {
+		ui.Fatalf("Failed to create npm package manager proxy: %s", err)
+	}
+
+	return flows.Common(packageManager).Run(ctx, args)
+}

cmd/npm/pnpm.go

@@ -1,11 +1,13 @@
 package npm
 
 import (
+	"context"
 	_ "embed"
 
 	"github.com/safedep/dry/log"
-	"github.com/safedep/pmg/config"
+	"github.com/safedep/pmg/internal/flows"
 	"github.com/safedep/pmg/internal/ui"
+	"github.com/safedep/pmg/packagemanager"
 	"github.com/spf13/cobra"
 )
 
@@ -15,12 +17,7 @@ func NewPnpmCommand() *cobra.Command {
 		Short:              "Guard pnpm package manager",
 		DisableFlagParsing: true,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			config, err := config.FromContext(cmd.Context())
-			if err != nil {
-				ui.Fatalf("Failed to get config: %s", err)
-			}
-
-			err = executePnpmFlow(cmd.Context(), config, args)
+			err := executePnpmFlow(cmd.Context(), args)
 			if err != nil {
 				log.Errorf("Failed to execute pnpm flow: %s", err)
 			}
@@ -29,3 +26,12 @@ func NewPnpmCommand() *cobra.Command {
 		},
 	}
 }
+
+func executePnpmFlow(ctx context.Context, args []string) error {
+	packageManager, err := packagemanager.NewNpmPackageManager(packagemanager.DefaultPnpmPackageManagerConfig())
+	if err != nil {
+		ui.Fatalf("Failed to create pnpm package manager proxy: %s", err)
+	}
+
+	return flows.Common(packageManager).Run(ctx, args)
+}

docs/development.md

@@ -0,0 +1,3 @@
+# Development
+
+- [User Interface](./ui.md)

docs/ui.md

@@ -0,0 +1,29 @@
+# User Interface
+
+PMG is an interactive tool. We support multiple interactivity modes such as `silent`, `verbose` etc. to meet different developer experience needs. As such, we need to standardize the UI, UX and interactive messaging guidance for developers.
+
+## Messaging
+
+Two types of messages to users are supported:
+
+1. UI messages
+2. Logs
+
+### UI messages
+
+UI messages are displayed in the user interface, currently in the terminal. Following types of messages are supported:
+
+1. **Status updates** - Meant for showing the stage or status of the workflow.
+2. **Error messages** - Meant for showing fatal error messages
+
+| Type   | Mode    | Show? |
+| ------ | ------- | ----- |
+| Status | Silent  | No    |
+| Status | Verbose | Yes   |
+| Error  | Silent  | Yes   |
+| Error  | Verbose | Yes   |
+
+### Logs
+
+Logs are by default for inspection and debugging purposes. They are not shown by default but can be configured through verbosity levels or logging to files. Consider logs as something meant for use only when there is an unexpected behavior.
+

guard/guard.go

@@ -172,6 +172,8 @@ func (g *packageManagerGuard) continueExecution(ctx context.Context, pc *package
 	cmd.Stdout = os.Stdout
 	cmd.Stderr = os.Stderr
 
+	// We will fail based on executed command's exit code. This is important
+	// because other tools (scripts, CI etc.) may depend on this exit code.
 	return cmd.Run()
 }
 
@@ -204,13 +206,19 @@ func (g *packageManagerGuard) concurrentAnalyzePackages(ctx context.Context,
 		}()
 	}
 
+	// Queue all packages for analysis
 	for _, pkg := range packages {
 		jobs <- pkg
 	}
 	close(jobs)
 
 	analysisResults := []*analyzer.PackageVersionAnalysisResult{}
+
+	// We must wait for the results go routine to collect all results
+	rwg := sync.WaitGroup{}
+	rwg.Add(1)
 	go func() {
+		defer rwg.Done()
 		for result := range results {
 			analysisResults = append(analysisResults, result)
 		}
@@ -219,8 +227,10 @@ func (g *packageManagerGuard) concurrentAnalyzePackages(ctx context.Context,
 	waiter := make(chan struct{})
 	go func() {
 		wg.Wait()
-		close(waiter)
 		close(results)
+
+		rwg.Wait()
+		close(waiter)
 	}()
 
 	select {

guard/guard_test.go

@@ -0,0 +1,47 @@
+package guard
+
+import (
+	"context"
+	"testing"
+
+	packagev1 "buf.build/gen/go/safedep/api/protocolbuffers/go/safedep/messages/package/v1"
+	"github.com/safedep/pmg/analyzer"
+	"github.com/stretchr/testify/assert"
+)
+
+func TestGuardConcurrentlyAnalyzePackagesMalwareQueryService(t *testing.T) {
+	mq, err := analyzer.NewMalysisQueryAnalyzer(analyzer.MalysisQueryAnalyzerConfig{})
+	if err != nil {
+		t.Fatalf("failed to create mq: %v", err)
+	}
+
+	pg, err := NewPackageManagerGuard(DefaultPackageManagerGuardConfig(), nil, nil,
+		[]analyzer.PackageVersionAnalyzer{mq}, PackageManagerGuardInteraction{})
+	if err != nil {
+		t.Fatalf("failed to create pg: %v", err)
+	}
+
+	t.Run("should resolve a single known malicious package version", func(t *testing.T) {
+		r, err := pg.concurrentAnalyzePackages(context.Background(), []*packagev1.PackageVersion{
+			{
+				Package: &packagev1.Package{
+					Name:      "nyc-config",
+					Ecosystem: packagev1.Ecosystem_ECOSYSTEM_NPM,
+				},
+				Version: "10.0.0",
+			},
+		})
+		if err != nil {
+			t.Fatalf("failed to analyze packages: %v", err)
+		}
+
+		assert.Equal(t, 1, len(r))
+		assert.Equal(t, "nyc-config", r[0].PackageVersion.GetPackage().GetName())
+		assert.Equal(t, "10.0.0", r[0].PackageVersion.GetVersion())
+		assert.Equal(t, packagev1.Ecosystem_ECOSYSTEM_NPM, r[0].PackageVersion.GetPackage().GetEcosystem())
+		assert.NotEmpty(t, r[0].ReferenceURL)
+		assert.NotEmpty(t, r[0].Summary)
+		assert.NotNil(t, r[0].Data)
+		assert.Equal(t, analyzer.ActionBlock, r[0].Action)
+	})
+}

cmd/npm/common.go internal/flows/common.gocmd/npm/common.go renamed to internal/flows/common.go

@@ -1,8 +1,7 @@
-package npm
+package flows
 
 import (
 	"context"
-	"fmt"
 
 	"github.com/safedep/pmg/analyzer"
 	"github.com/safedep/pmg/config"
@@ -11,30 +10,48 @@ import (
 	"github.com/safedep/pmg/packagemanager"
 )
 
-func executeCommonFlow(ctx context.Context, config config.Config, pm packagemanager.PackageManager, args []string) error {
+type commonFlow struct {
+	pm packagemanager.PackageManager
+}
+
+// Creates a common flow of execution for all package managers. This should work for most
+// of the cases unless a package manager has its own unique requirements. Configuration
+// should be passed through the context (Global Config)
+func Common(pm packagemanager.PackageManager) *commonFlow {
+	return &commonFlow{
+		pm: pm,
+	}
+}
+
+func (f *commonFlow) Run(ctx context.Context, args []string) error {
+	config, err := config.FromContext(ctx)
+	if err != nil {
+		ui.Fatalf("Failed to get config: %s", err)
+	}
+
 	packageResolverConfig := packagemanager.NewDefaultNpmDependencyResolverConfig()
 	packageResolverConfig.IncludeTransitiveDependencies = config.Transitive
 	packageResolverConfig.TransitiveDepth = config.TransitiveDepth
 	packageResolverConfig.IncludeDevDependencies = config.IncludeDevDependencies
 
 	packageResolver, err := packagemanager.NewNpmDependencyResolver(packageResolverConfig)
 	if err != nil {
-		return fmt.Errorf("failed to create npm dependency resolver: %w", err)
+		ui.Fatalf("Failed to create dependency resolver: %s", err)
 	}
 
 	var analyzers []analyzer.PackageVersionAnalyzer
 
 	if config.Paranoid {
 		malysisActiveScanAnalyzer, err := analyzer.NewMalysisActiveScanAnalyzer(analyzer.DefaultMalysisActiveScanAnalyzerConfig())
 		if err != nil {
-			return fmt.Errorf("failed to create malysis active scan analyzer: %w", err)
+			ui.Fatalf("Failed to create malware analyzer: %s", err)
 		}
 
 		analyzers = append(analyzers, malysisActiveScanAnalyzer)
 	} else {
 		malysisQueryAnalyzer, err := analyzer.NewMalysisQueryAnalyzer(analyzer.MalysisQueryAnalyzerConfig{})
 		if err != nil {
-			return fmt.Errorf("failed to create malysis query analyzer: %w", err)
+			ui.Fatalf("Failed to create malware analyzer: %s", err)
 		}
 
 		analyzers = append(analyzers, malysisQueryAnalyzer)
@@ -50,28 +67,15 @@ func executeCommonFlow(ctx context.Context, config config.Config, pm packagemana
 	guardConfig := guard.DefaultPackageManagerGuardConfig()
 	guardConfig.DryRun = config.DryRun
 
-	proxy, err := guard.NewPackageManagerGuard(guardConfig, pm, packageResolver, analyzers, interaction)
+	proxy, err := guard.NewPackageManagerGuard(guardConfig, f.pm, packageResolver, analyzers, interaction)
 	if err != nil {
-		return fmt.Errorf("failed to create package manager guard: %w", err)
+		ui.Fatalf("Failed to create package manager guard: %s", err)
 	}
 
-	return proxy.Run(ctx, args)
-}
-
-func executeNpmFlow(ctx context.Context, config config.Config, args []string) error {
-	packageManager, err := packagemanager.NewNpmPackageManager(packagemanager.DefaultNpmPackageManagerConfig())
-	if err != nil {
-		return fmt.Errorf("failed to create npm package manager: %w", err)
-	}
-
-	return executeCommonFlow(ctx, config, packageManager, args)
-}
-
-func executePnpmFlow(ctx context.Context, config config.Config, args []string) error {
-	packageManager, err := packagemanager.NewNpmPackageManager(packagemanager.DefaultPnpmPackageManagerConfig())
+	err = proxy.Run(ctx, args)
 	if err != nil {
-		return fmt.Errorf("failed to create pnpm package manager: %w", err)
+		ui.Fatalf("pmg: failed to execute command: %s", err)
 	}
 
-	return executeCommonFlow(ctx, config, packageManager, args)
+	return err
 }