Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions cmd/gaze/main.go
Original file line number Diff line number Diff line change
Expand Up @@ -217,7 +217,7 @@ func runAnalyze(p analyzeParams) error {
return fmt.Errorf("getting working directory: %w", err)
}

pkgPaths, err := loader.ResolvePackagePaths(p.patterns, moduleDir)
pkgPaths, err := loader.ResolvePackagePaths(p.patterns, moduleDir, p.stderr)
if err != nil {
return fmt.Errorf("resolving package patterns: %w", err)
}
Expand Down Expand Up @@ -1066,7 +1066,7 @@ func runQuality(p qualityParams) error {
return fmt.Errorf("getting working directory: %w", err)
}

pkgPaths, err := loader.ResolvePackagePaths(p.patterns, moduleDir)
pkgPaths, err := loader.ResolvePackagePaths(p.patterns, moduleDir, p.stderr)
if err != nil {
return fmt.Errorf("resolving package patterns: %w", err)
}
Expand Down
9 changes: 8 additions & 1 deletion internal/aireport/runner_steps.go
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,14 @@ func resolveQualityDeps(deps []qualityPipelineDeps) qualityPipelineDeps {
d = deps[0]
}
if d.resolvePackagePaths == nil {
d.resolvePackagePaths = loader.ResolvePackagePaths
// Wrap loader.ResolvePackagePaths to match the DI function
// type (2-param). Warnings are discarded (nil stderr) to
// avoid cascading the io.Writer parameter through the DI
// type and all test fakes. Direct callers in cmd/gaze and
// goprovider pass stderr for user-facing warning output.
d.resolvePackagePaths = func(patterns []string, moduleDir string) ([]string, error) {
return loader.ResolvePackagePaths(patterns, moduleDir, nil)
Comment thread
trevor-vaughan marked this conversation as resolved.
}
}
if d.loadAndAnalyze == nil {
d.loadAndAnalyze = analysis.LoadAndAnalyze
Expand Down
19 changes: 18 additions & 1 deletion internal/loader/loader.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ package loader
import (
"fmt"
"go/token"
"io"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -151,7 +152,12 @@ func FindModuleRoot(startDir string) (string, error) {
// wildcards) to individual fully-qualified package paths. It uses a
// lightweight NeedName load mode, deduplicates results, and filters
// out test-variant packages (those with a "_test" suffix).
func ResolvePackagePaths(patterns []string, moduleDir string) ([]string, error) {
//
// Packages with load errors (e.g., nonexistent patterns that produce
// synthetic packages from go/packages) are skipped and a warning is
// written to stderr for each error. If stderr is nil, warnings are
// silently discarded.
func ResolvePackagePaths(patterns []string, moduleDir string, stderr io.Writer) ([]string, error) {
if len(patterns) == 0 {
return nil, nil
}
Expand All @@ -170,6 +176,17 @@ func ResolvePackagePaths(patterns []string, moduleDir string) ([]string, error)
if pkg.PkgPath == "" || seen[pkg.PkgPath] || strings.HasSuffix(pkg.PkgPath, "_test") {
continue
}
// Skip packages that failed to load (phantom paths from
// unresolvable patterns). go/packages returns err == nil
// at the call level but populates pkg.Errors.
if len(pkg.Errors) > 0 {
if stderr != nil {
for _, e := range pkg.Errors {
_, _ = fmt.Fprintf(stderr, "warning: skipping %s: %s\n", pkg.PkgPath, e)
}
}
continue
}
seen[pkg.PkgPath] = true
pkgPaths = append(pkgPaths, pkg.PkgPath)
}
Expand Down
34 changes: 31 additions & 3 deletions internal/loader/loader_test.go
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package loader_test

import (
"bytes"
"os"
"path/filepath"
"strings"
Expand Down Expand Up @@ -121,7 +122,7 @@ func TestResolvePackagePaths_Wildcard(t *testing.T) {
}

root := findModuleRoot(t)
paths, err := loader.ResolvePackagePaths([]string{"./..."}, root)
paths, err := loader.ResolvePackagePaths([]string{"./..."}, root, nil)
if err != nil {
t.Fatalf("ResolvePackagePaths failed: %v", err)
}
Expand Down Expand Up @@ -153,7 +154,7 @@ func TestResolvePackagePaths_SinglePackage(t *testing.T) {

root := findModuleRoot(t)
paths, err := loader.ResolvePackagePaths(
[]string{"github.com/unbound-force/gaze/internal/loader"}, root,
[]string{"github.com/unbound-force/gaze/internal/loader"}, root, nil,
)
if err != nil {
t.Fatalf("ResolvePackagePaths failed: %v", err)
Expand All @@ -168,7 +169,7 @@ func TestResolvePackagePaths_SinglePackage(t *testing.T) {

func TestResolvePackagePaths_EmptyPatterns(t *testing.T) {
root := findModuleRoot(t)
paths, err := loader.ResolvePackagePaths([]string{}, root)
paths, err := loader.ResolvePackagePaths([]string{}, root, nil)
if err != nil {
t.Fatalf("ResolvePackagePaths failed: %v", err)
}
Expand All @@ -177,6 +178,33 @@ func TestResolvePackagePaths_EmptyPatterns(t *testing.T) {
}
}

func TestResolvePackagePaths_InvalidPattern(t *testing.T) {
Comment thread
trevor-vaughan marked this conversation as resolved.
if testing.Short() {
t.Skip("skipping slow test: loads package via go/packages")
}

root := findModuleRoot(t)
var buf bytes.Buffer
paths, err := loader.ResolvePackagePaths(
[]string{"github.com/nonexistent/does/not/exist"}, root, &buf,
)
if err != nil {
t.Fatalf("ResolvePackagePaths returned unexpected error: %v", err)
}
if len(paths) != 0 {
t.Errorf("expected empty paths for nonexistent pattern, got %v", paths)
}

// Verify warning was written to stderr.
stderr := buf.String()
if !strings.Contains(stderr, "warning: skipping") {
t.Errorf("expected warning prefix in stderr, got %q", stderr)
}
if !strings.Contains(stderr, "github.com/nonexistent/does/not/exist") {
t.Errorf("expected package path in stderr warning, got %q", stderr)
}
}

func TestIsMainPkg_Library(t *testing.T) {
if testing.Short() {
t.Skip("skipping slow test: loads package via go/packages")
Expand Down
2 changes: 1 addition & 1 deletion internal/provider/goprovider/contract.go
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ func BuildContractCoverageFunc(
stderr io.Writer,
aiMapperFn ...quality.AIMapperFunc,
) (func(pkg, function string) (crap.ContractCoverageInfo, bool), []string) {
pkgPaths, err := loader.ResolvePackagePaths(patterns, moduleDir)
pkgPaths, err := loader.ResolvePackagePaths(patterns, moduleDir, stderr)
if err != nil {
_, _ = fmt.Fprintf(stderr, "quality pipeline: failed to resolve packages: %v\n", err)
return nil, nil
Expand Down
2 changes: 2 additions & 0 deletions openspec/changes/fix-phantom-package-paths/.openspec.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
schema: unbound-force
created: 2026-07-07
65 changes: 65 additions & 0 deletions openspec/changes/fix-phantom-package-paths/design.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
## Context

`loader.ResolvePackagePaths` (`internal/loader/loader.go:154-177`) resolves package patterns to fully-qualified paths using `go/packages.Load`. The `go/packages` API returns `err == nil` at the call level even when individual packages fail to load -- per-package errors are stored in `pkg.Errors`. The current implementation never checks `pkg.Errors`, so synthetic packages for nonexistent patterns pass through the filter and are returned as valid paths.

This was identified in issue #104 with a confirmed failing test on `main`. The issue comment (#104, council analysis) provides a thorough critique of the naive fix, identifying five gaps that this design addresses.

## Goals / Non-Goals

### Goals

- Filter out packages with load errors in `ResolvePackagePaths`, preventing phantom paths from entering downstream analysis
- Surface warnings for skipped packages so users understand why a pattern was dropped
- Add a test for the invalid-pattern case in `internal/loader/loader_test.go`
- Fix the currently-failing `TestBuildContractCoverageFunc_InvalidPattern` test
- Close #104

### Non-Goals

- Consolidating the duplicate `resolvePackagePaths` copies across packages (tracked in #139; both copies now delegate to `loader.ResolvePackagePaths`, so fixing the shared function fixes all callers)
- Differentiating error kinds (`ListError` vs `TypeError` vs `ParseError`) -- the council comment suggests this but it adds complexity for marginal benefit in the `ResolvePackagePaths` context, which uses `NeedName` mode only (type errors are not produced at this load level)
- Checking `pkg.Name == ""` as a complementary defense -- while noted in the critique, `pkg.Errors` is the canonical signal; `Name == ""` is an implementation detail of `go/packages` that could change

## Decisions

### D1: Add `io.Writer` parameter for warning output

`ResolvePackagePaths` currently has no way to surface diagnostics. Rather than returning a `[]error` or using a global logger, adding an `io.Writer` parameter follows the project's established pattern (see `BuildContractCoverageFunc`, `analyzePackageCoverage`, `quality.Options.Stderr`). Four of five callers already have an `io.Writer` available (`BuildContractCoverageFunc`, `runQualityStep`, `runAnalyze`, `runQuality`). The fifth (`runClassifyStep`) does not; it passes `nil` per D4 to avoid cascading signature changes.

**Signature change**: `ResolvePackagePaths(patterns []string, moduleDir string) ([]string, error)` becomes `ResolvePackagePaths(patterns []string, moduleDir string, stderr io.Writer) ([]string, error)`.

This aligns with Constitution Principle IV (Testability) -- tests can capture warnings via `bytes.Buffer`.

### D2: Skip all errored packages, warn per-error

For each package with `len(pkg.Errors) > 0`, emit one warning line per error to `stderr` and skip the package. This is consistent with `LoadModule` (lines 113-120) which also silently excludes errored packages, though `LoadModule` does not log warnings.

The council critique notes that silently dropping packages is "arguably worse" than the current behavior. The warning output addresses this -- the package is dropped from the result but the user is informed why.

### D3: Do not return an error for partial resolution

If some patterns resolve and others don't, return the valid paths with warnings for the invalid ones. Only the existing behavior of returning `(nil, nil)` for completely empty input is preserved. This matches the best-effort semantics already documented in `BuildContractCoverageFunc`: "if the quality pipeline fails for any package, those packages are silently skipped."

If ALL patterns fail to resolve (non-empty input producing zero valid paths), the function returns `(nil, nil)` -- not an error. The caller (`BuildContractCoverageFunc`) already handles this case correctly by returning `nil`. Callers cannot distinguish "no input" from "all input invalid" via return values alone; the warning output to `stderr` is the only differentiator.

### D4: Nil-safe stderr handling

If `stderr` is nil, warnings are silently discarded. This avoids forcing callers that don't care about diagnostics to pass a writer. The function checks `stderr != nil` before writing.

## Coverage Strategy

Unit test in `internal/loader/loader_test.go` (`TestResolvePackagePaths_InvalidPattern`) covers the new `pkg.Errors` filtering logic directly. Existing `TestBuildContractCoverageFunc_InvalidPattern` in `internal/crap/contract_test.go` serves as integration-level regression guard through the `BuildContractCoverageFunc` -> `ResolvePackagePaths` call chain. Both guarded by `testing.Short()` since they invoke `go/packages.Load`. No e2e test needed -- the fix is internal to the loader package.

## Risks / Trade-offs

### R1: Signature change is breaking for callers

Adding `io.Writer` changes the exported function signature. All known callers are within this module (`internal/crap/contract.go`, `internal/aireport/runner_steps.go`), so the blast radius is contained. External consumers cannot import `internal/` packages.

### R2: NeedName mode limits error kinds

`ResolvePackagePaths` uses `packages.NeedName` -- a lightweight mode. At this load level, errors are primarily `ListError` (package not found). The blanket `len(pkg.Errors) > 0` check is safe here because `TypeError` and `ParseError` are not produced without `NeedTypes`/`NeedSyntax`. If the load mode were ever expanded, this decision would need revisiting.

### R3: No error aggregation

The function does not aggregate errors into a returned `error`. This is intentional (D3) but means a caller passing `stderr=nil` would silently get fewer results than expected. Callers that need strict validation should check result count against input count.
63 changes: 63 additions & 0 deletions openspec/changes/fix-phantom-package-paths/proposal.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
## Why

`loader.ResolvePackagePaths` does not check `pkg.Errors` on packages returned by `go/packages.Load`. When a pattern does not resolve to a real package, `go/packages` returns a synthetic `*packages.Package` with `PkgPath` set to the input pattern and `Errors` populated. Because the filter only rejects empty/duplicate/`_test` paths, the phantom path is returned as valid.

This violates the function's contract ("resolves package patterns to individual fully-qualified package paths") and causes a failing unit test on `main`. For a test-quality tool, shipping with red tests is a credibility problem (Constitution Principle IV).

Ref: https://github.com/unbound-force/gaze/issues/104

## What Changes

Add `pkg.Errors` checking to `loader.ResolvePackagePaths` so that packages with load errors are skipped rather than returned as valid paths. Log warnings for skipped packages. Add a test for the invalid-pattern case directly in `internal/loader/loader_test.go`.

## Capabilities

### New Capabilities

- None

### Modified Capabilities

- `loader.ResolvePackagePaths`: Now filters out packages with load errors (phantom paths) and logs warnings via a new `io.Writer` parameter for the caller to observe diagnostics.

### Removed Capabilities

- None

## Impact

- **`internal/loader/loader.go`**: `ResolvePackagePaths` gains `pkg.Errors` filtering. Signature changes to accept `io.Writer` for warning output.
- **`internal/loader/loader_test.go`**: New test for invalid/nonexistent pattern.
- **`internal/crap/contract.go`**: Caller updated to pass `stderr` to `ResolvePackagePaths`.
- **`internal/aireport/runner_steps.go`**: Two call sites updated. `runQualityStep` (line 90) passes its existing `stderr` parameter. `runClassifyStep` (line 192) passes `nil` since it lacks an `io.Writer` parameter -- warnings are discarded for classify-step resolution, which is acceptable per D4.
- **`cmd/gaze/main.go`**: Two callers updated -- `runAnalyze` (line 220) and `runQuality` (line 1069), both pass their existing `p.stderr` parameter.
- Fixes failing `TestBuildContractCoverageFunc_InvalidPattern` test on `main`.
- Closes #104.

## Constitution Alignment

Assessed against the Gaze project constitution.

### I. Accuracy

**Assessment**: PASS

This fix improves accuracy by preventing phantom package paths from entering the analysis pipeline. Currently, nonexistent patterns produce paths that silently fail downstream, dropping GazeCRAP/quality contributions without a clear error. After this fix, invalid patterns are detected early and surfaced as warnings.

### II. Minimal Assumptions

**Assessment**: PASS

The fix uses `pkg.Errors` -- the standard `go/packages` mechanism for reporting per-package load failures. It does not introduce new assumptions about project structure or coding style. The `go/packages` API documents that `Load` returns `err == nil` even when individual packages fail; checking `pkg.Errors` is the expected usage pattern.

### III. Actionable Output

**Assessment**: PASS

Warning messages for skipped packages give users clear, actionable feedback: the specific pattern that failed and the specific error from `go/packages`. This replaces silent failure downstream with early, explicit diagnostics.

### IV. Testability

**Assessment**: PASS

The fix directly restores a currently-failing test. The `io.Writer` parameter enables test-time capture of warning output without global state. A new dedicated test in `loader_test.go` verifies the invalid-pattern filtering behavior in isolation.
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
## ADDED Requirements

### Requirement: Phantom Package Filtering

`loader.ResolvePackagePaths` MUST skip packages where `len(pkg.Errors) > 0` and MUST NOT include their `PkgPath` in the returned slice. This prevents synthetic packages returned by `go/packages.Load` for nonexistent patterns from being treated as valid package paths.

#### Scenario: Nonexistent package pattern

- **GIVEN** `ResolvePackagePaths` is called with pattern `"github.com/nonexistent/does/not/exist"`
- **WHEN** `go/packages.Load` returns a synthetic package with `PkgPath` set to the pattern and `Errors` populated
- **THEN** the returned slice MUST be empty and no error MUST be returned

#### Scenario: Mix of valid and invalid patterns

- **GIVEN** `ResolvePackagePaths` is called with patterns `["github.com/unbound-force/gaze/internal/loader", "github.com/nonexistent/pkg"]`
- **WHEN** `go/packages.Load` returns one valid package and one errored package
- **THEN** the returned slice MUST contain only the valid package path and a warning MUST be written to `stderr` for the invalid pattern

### Requirement: Warning Output for Skipped Packages

`loader.ResolvePackagePaths` MUST accept an `io.Writer` parameter for diagnostic output. When a package is skipped due to load errors, the function MUST write a warning line per error to the writer in the format: `warning: skipping <PkgPath>: <error message>`.

#### Scenario: Warning output for invalid pattern

- **GIVEN** `ResolvePackagePaths` is called with a nonexistent pattern and a non-nil `io.Writer`
- **WHEN** the package is skipped due to load errors
- **THEN** the writer MUST contain a warning line including the package path and error detail

#### Scenario: Nil stderr writer

- **GIVEN** `ResolvePackagePaths` is called with `stderr` as `nil`
- **WHEN** a package has load errors
- **THEN** the package MUST still be skipped and no panic MUST occur

## MODIFIED Requirements

### Requirement: ResolvePackagePaths Signature

`loader.ResolvePackagePaths` signature changes from:

```go
func ResolvePackagePaths(patterns []string, moduleDir string) ([]string, error)
```

to:

```go
func ResolvePackagePaths(patterns []string, moduleDir string, stderr io.Writer) ([]string, error)
```

Previously: The function accepted only patterns and moduleDir. The new `io.Writer` parameter enables diagnostic output without global logger dependency.

## REMOVED Requirements

None.
Loading
Loading