diff --git a/AGENTS.md b/AGENTS.md index a838f5b..bcabdf2 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -2,7 +2,9 @@ - mission: on-demand JavaScript dependency PR risk summary - JS package manager scope: support `package.json` with `package-lock.json` or - `pnpm-lock.yaml`, plus `pnpm-workspace.yaml` for pnpm workspace discovery + `pnpm-lock.yaml`, plus `pnpm-workspace.yaml` for pnpm workspace discovery, + and support classic `yarn.lock` fallback for root, workspace, and nested + standalone Yarn targets - keep GitHub I/O in `internal/github` - keep orchestration in `internal/app` - keep deterministic logic in `internal/analysis` and `internal/npm` diff --git a/CHANGELOG.md b/CHANGELOG.md index 53772e4..f2e4280 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,15 @@ All notable changes to `gh-dep-risk` will be documented in this file. ## Unreleased +- Fixed local fallback against large repository lockfiles fetched from the + GitHub contents API by following blob-backed `encoding: none` responses for + files such as large `yarn.lock` and `pnpm-lock.yaml`. +- Marked local npm and Yarn file/workspace packages consistently during local + fallback so local targets are not treated like registry packages for target + traversal or publish-age lookups. +- Tightened the public scope wording to distinguish GitHub dependency-review + ecosystem coverage from the narrower local fallback support matrix. + ## v0.1.7 - Expanded the dependency-review path to surface mixed-ecosystem pull requests diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d50f59e..d935495 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -22,11 +22,12 @@ go build -o gh-dep-risk.exe . .\gh-dep-risk.exe version ``` -The supported JS package manager scope is: +The supported local JS fallback scope is: - npm: `package.json`, `package-lock.json` - pnpm: `package.json`, `pnpm-lock.yaml` - pnpm workspace discovery: `pnpm-workspace.yaml` +- yarn: `package.json`, `yarn.lock` (Yarn Classic only) Build with explicit metadata: diff --git a/README.md b/README.md index 620d113..d53f82e 100644 --- a/README.md +++ b/README.md @@ -11,7 +11,7 @@ webhook, queue, database, or dashboard infrastructure. ## Scope -- API-first multi-ecosystem dependency review +- on-demand pull request dependency review through GitHub Dependency Review - local fallback support matrix: - npm: `package.json`, `package-lock.json` - pnpm: `package.json`, `pnpm-lock.yaml` @@ -23,7 +23,8 @@ webhook, queue, database, or dashboard infrastructure. - no server, webhook receiver, GitHub App, DB, queue, dashboard, bun, or broad non-JS local fallback in this release -Dependency-review path ecosystems in this release: +Dependency Review ecosystems surfaced in this release when GitHub provides +them: - Cargo - Composer @@ -298,6 +299,8 @@ Notes: - Yarn local fallback supports classic `yarn.lock` installs only - likely Yarn Berry / Plug'n'Play lockfiles are detected and reported as an unsupported local-fallback case instead of being analyzed inaccurately +- large lockfiles served by the GitHub contents API without inline content are + still fetched through the corresponding blob object instead of failing early - if a lockfile-only workspace change cannot be mapped exactly, the report calls out that attribution is approximate instead of failing - if both `package-lock.json` and `pnpm-lock.yaml` exist for the same target diff --git a/RELEASING.md b/RELEASING.md index 77e3e71..9535b5e 100644 --- a/RELEASING.md +++ b/RELEASING.md @@ -10,10 +10,12 @@ git status --short --branch The tree should be clean before cutting a release tag. -## 2. Run tests +## 2. Run local verification ```bash go test ./... +go build ./... +git diff --check ``` ## 3. Build a release-quality binary locally @@ -40,7 +42,11 @@ go build -ldflags "-s -w -X gh-dep-risk/cmd.version=$version -X gh-dep-risk/cmd. .\gh-dep-risk.exe version --json ``` -## 4. Optional manual workflow smoke test +## 4. Run live CLI checks and optional manual workflow smoke test + +Before tagging, run the local binary against real pull requests if you have +GitHub auth available. Prefer at least one smaller PR and one larger PR so the +final release is not validated only against fixtures. If you have GitHub auth and repository access available, run: diff --git a/docs/smoke-test.md b/docs/smoke-test.md index 531c924..579c8c3 100644 --- a/docs/smoke-test.md +++ b/docs/smoke-test.md @@ -20,6 +20,10 @@ Verify: - the command exits `0`, `2`, `3`, or `4` as documented - the report includes repo, PR, score, blast radius, and recommended actions +- before release, run at least one smaller PR and one larger PR instead of + relying only on fixture-backed tests +- if the target repository has a very large lockfile, verify the run does not + fail on a GitHub contents API `encoding: none` response ## 2. Workflow dispatch run diff --git a/internal/analysis/types_test.go b/internal/analysis/types_test.go new file mode 100644 index 0000000..1efe18e --- /dev/null +++ b/internal/analysis/types_test.go @@ -0,0 +1,48 @@ +package analysis + +import ( + "testing" + + "gh-dep-risk/internal/npm" +) + +func TestCollectRegistryTargetsSkipsLocalWorkspacePackages(t *testing.T) { + input := Input{ + Target: AnalysisTarget{ + DisplayName: "apps/web", + ManifestPath: "apps/web/package.json", + LockfilePath: "yarn.lock", + Kind: TargetKindWorkspace, + PackageManager: "yarn", + OwningDirectory: "apps/web", + }, + BaseManifest: manifest(nil, nil, nil), + HeadManifest: manifest(map[string]string{ + "local-lib": "file:../local-lib", + }, nil, nil), + BaseLockfile: &npm.Lockfile{ + Manager: "yarn", + Packages: map[string]npm.LockPackage{}, + Importers: map[string]npm.LockImporter{}, + }, + HeadLockfile: &npm.Lockfile{ + Manager: "yarn", + Packages: map[string]npm.LockPackage{ + "node_modules/local-lib": { + Path: "node_modules/local-lib", + Name: "local-lib", + Version: "1.0.0", + Resolved: "file:../local-lib", + WorkspaceLocal: true, + Dependencies: map[string]string{}, + }, + }, + Importers: map[string]npm.LockImporter{}, + }, + } + + targets := CollectRegistryTargets(input) + if len(targets) != 0 { + t.Fatalf("expected no registry targets for local workspace package, got %#v", targets) + } +} diff --git a/internal/github/client.go b/internal/github/client.go index 3007c7d..9501019 100644 --- a/internal/github/client.go +++ b/internal/github/client.go @@ -355,6 +355,7 @@ func (c *APIClient) GetRepositoryFile(ctx context.Context, repo Repo, path, ref var resp struct { Content string `json:"content"` Encoding string `json:"encoding"` + SHA string `json:"sha"` } contentPath := fmt.Sprintf("repos/%s/%s/contents/%s?ref=%s", repo.Owner, repo.Name, escapeContentPath(path), url.QueryEscape(ref)) @@ -364,14 +365,20 @@ func (c *APIClient) GetRepositoryFile(ctx context.Context, repo Repo, path, ref } return nil, classifyAuthError(err) } - if resp.Encoding != "base64" { - return nil, fmt.Errorf("unsupported content encoding %q for %s", resp.Encoding, path) - } - decoded, err := base64.StdEncoding.DecodeString(strings.ReplaceAll(resp.Content, "\n", "")) - if err != nil { - return nil, fmt.Errorf("decode %s: %w", path, err) - } - return decoded, nil + return decodeRepositoryContent(path, resp.Content, resp.Encoding, resp.SHA, func(sha string) (string, string, error) { + var blob struct { + Content string `json:"content"` + Encoding string `json:"encoding"` + } + blobPath := fmt.Sprintf("repos/%s/%s/git/blobs/%s", repo.Owner, repo.Name, url.PathEscape(sha)) + if err := client.DoWithContext(ctx, "GET", blobPath, nil, &blob); err != nil { + if IsHTTPStatus(err, 404) { + return "", "", ErrNotFound + } + return "", "", classifyAuthError(err) + } + return blob.Content, blob.Encoding, nil + }) } func (c *APIClient) ListIssueComments(ctx context.Context, repo Repo, issueNumber int) ([]IssueComment, error) { @@ -596,6 +603,38 @@ func escapeContentPath(path string) string { return strings.Join(parts, "/") } +func decodeRepositoryContent(path, content, encoding, sha string, fetchBlob func(sha string) (string, string, error)) ([]byte, error) { + switch encoding { + case "base64": + return decodeBase64RepositoryContent(path, content) + case "none": + if strings.TrimSpace(sha) == "" { + return nil, fmt.Errorf("unsupported content encoding %q for %s", encoding, path) + } + if fetchBlob == nil { + return nil, fmt.Errorf("unsupported content encoding %q for %s", encoding, path) + } + blobContent, blobEncoding, err := fetchBlob(sha) + if err != nil { + return nil, err + } + if blobEncoding != "base64" { + return nil, fmt.Errorf("unsupported blob encoding %q for %s", blobEncoding, path) + } + return decodeBase64RepositoryContent(path, blobContent) + default: + return nil, fmt.Errorf("unsupported content encoding %q for %s", encoding, path) + } +} + +func decodeBase64RepositoryContent(path, content string) ([]byte, error) { + decoded, err := base64.StdEncoding.DecodeString(strings.ReplaceAll(content, "\n", "")) + if err != nil { + return nil, fmt.Errorf("decode %s: %w", path, err) + } + return decoded, nil +} + type AuthError struct { Op string Err error diff --git a/internal/github/client_test.go b/internal/github/client_test.go index e1a0522..01dc26b 100644 --- a/internal/github/client_test.go +++ b/internal/github/client_test.go @@ -3,6 +3,7 @@ package github import ( "bytes" "context" + "encoding/base64" "errors" "reflect" "strings" @@ -189,6 +190,32 @@ func TestClassifyAuthErrorWrapsHTTP401AsAuthError(t *testing.T) { } } +func TestDecodeRepositoryContentSupportsLargeBlobFallback(t *testing.T) { + want := []byte("hello from blob fallback") + content, err := decodeRepositoryContent("yarn.lock", "", "none", "blob-sha", func(sha string) (string, string, error) { + if sha != "blob-sha" { + t.Fatalf("unexpected blob sha %q", sha) + } + return base64.StdEncoding.EncodeToString(want), "base64", nil + }) + if err != nil { + t.Fatal(err) + } + if !bytes.Equal(content, want) { + t.Fatalf("unexpected decoded content: got %q want %q", string(content), string(want)) + } +} + +func TestDecodeRepositoryContentRejectsUnsupportedEncodingWithoutBlobSHA(t *testing.T) { + _, err := decodeRepositoryContent("pnpm-lock.yaml", "", "none", "", nil) + if err == nil { + t.Fatal("expected error") + } + if !strings.Contains(err.Error(), `unsupported content encoding "none"`) { + t.Fatalf("unexpected error: %v", err) + } +} + func strconvFormatBool(value bool) string { if value { return "true" diff --git a/internal/npm/jsgraph.go b/internal/npm/jsgraph.go index e288d41..149f675 100644 --- a/internal/npm/jsgraph.go +++ b/internal/npm/jsgraph.go @@ -148,6 +148,9 @@ func (l *Lockfile) CollectTargetPackages(targetDir string, directNames []string) continue } result.All[pkg.Path] = pkg + if pkg.WorkspaceLocal { + continue + } queue = append(queue, pkg) } @@ -166,6 +169,9 @@ func (l *Lockfile) CollectTargetPackages(targetDir string, directNames []string) continue } result.All[dep.Path] = dep + if dep.WorkspaceLocal { + continue + } queue = append(queue, dep) } } diff --git a/internal/npm/lockfile.go b/internal/npm/lockfile.go index d02ac96..072581a 100644 --- a/internal/npm/lockfile.go +++ b/internal/npm/lockfile.go @@ -145,6 +145,7 @@ func parseNPMLockfile(data []byte) (*Lockfile, error) { OS: append([]string(nil), entry.OS...), CPU: append([]string(nil), entry.CPU...), Dependencies: cloneMap(entry.Dependencies), + WorkspaceLocal: isLocalPackageReference(entry.Resolved), } } return lockfile, nil @@ -290,6 +291,7 @@ func parseYarnLockfile(data []byte) (*Lockfile, error) { if entry.Version == "" { continue } + entry.WorkspaceLocal = isLocalPackageReference(entry.Resolved) key := entry.Name + "@" + entry.Version if existingPath, ok := seen[key]; ok { existing := lockfile.Packages[existingPath] @@ -302,6 +304,7 @@ func parseYarnLockfile(data []byte) (*Lockfile, error) { if existing.Integrity == "" { existing.Integrity = entry.Integrity } + existing.WorkspaceLocal = existing.WorkspaceLocal || entry.WorkspaceLocal lockfile.Packages[existingPath] = existing continue } @@ -350,6 +353,7 @@ func flattenLegacyDependencies(target map[string]LockPackage, parent string, dep OS: append([]string(nil), dep.OS...), CPU: append([]string(nil), dep.CPU...), Dependencies: cloneMap(dep.Requires), + WorkspaceLocal: isLocalPackageReference(dep.Resolved), } flattenLegacyDependencies(target, path, dep.Dependencies) } @@ -678,6 +682,24 @@ func isPNPMWorkspaceLink(version string) bool { return strings.HasPrefix(lower, "link:") || strings.HasPrefix(lower, "workspace:") } +func isLocalPackageReference(value string) bool { + lower := strings.ToLower(strings.TrimSpace(value)) + switch { + case strings.HasPrefix(lower, "file:"): + return true + case strings.HasPrefix(lower, "link:"): + return true + case strings.HasPrefix(lower, "workspace:"): + return true + case strings.HasPrefix(lower, "portal:"): + return true + case lower == "workspace-local": + return true + default: + return false + } +} + func pnpmResolved(tarball, repo, resolutionType string) string { switch { case strings.TrimSpace(tarball) != "": diff --git a/internal/npm/lockfile_test.go b/internal/npm/lockfile_test.go index 383b70c..e67f50a 100644 --- a/internal/npm/lockfile_test.go +++ b/internal/npm/lockfile_test.go @@ -141,12 +141,22 @@ func TestCollectTargetPackagesForWorkspaceUsesSharedRootLockfile(t *testing.T) { } view := lockfile.CollectTargetPackages("apps/web", []string{"@acme/ui", "axios", "react"}) + ui, ok := view.Direct["@acme/ui"] + if !ok { + t.Fatalf("expected @acme/ui to resolve as a direct workspace dependency") + } + if !ui.WorkspaceLocal { + t.Fatalf("expected @acme/ui to be marked workspace-local") + } if _, ok := view.Direct["axios"]; !ok { t.Fatalf("expected axios to resolve as a direct workspace dependency") } if _, ok := view.Transitive["node_modules/form-data"]; !ok { t.Fatalf("expected shared root transitive dependency to be included") } + if _, ok := view.Transitive["node_modules/clsx"]; ok { + t.Fatalf("did not expect local workspace package dependencies to be attributed as external transitive packages") + } if view.Approximate { t.Fatalf("expected exact workspace attribution for shared root lockfile fixture") } @@ -268,6 +278,36 @@ func TestParseYarnClassicLockfileCollectsStandalonePackages(t *testing.T) { } } +func TestParseYarnClassicLockfileMarksLocalFileReferences(t *testing.T) { + data := []byte(`# yarn lockfile v1 + +local-lib@file:../local-lib: + version "1.0.0" + resolved "file:../local-lib" + +left-pad@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/left-pad/-/left-pad-1.0.0.tgz" +`) + + lockfile, err := ParseLockfile(data) + if err != nil { + t.Fatal(err) + } + + view := lockfile.CollectTargetPackages("", []string{"local-lib", "left-pad"}) + localLib, ok := view.Direct["local-lib"] + if !ok { + t.Fatalf("expected local-lib to resolve as a direct yarn dependency") + } + if !localLib.WorkspaceLocal { + t.Fatalf("expected local-lib to be marked workspace-local") + } + if len(view.Transitive) != 0 { + t.Fatalf("did not expect transitive traversal through local file dependency, got %#v", view.Transitive) + } +} + func TestParseYarnBerryLockfileReturnsUnsupportedFallbackError(t *testing.T) { data, err := os.ReadFile(filepath.Join("..", "..", "testdata", "yarn.unsupported.lock")) if err != nil {