diff --git a/CHANGELOG.md b/CHANGELOG.md index 351a3cf58..980579a20 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,11 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). +## [Unreleased] + +### Fixed +- `socket fix` pull requests now include every file a fix changes, such as `package.json` override bumps, and work when run from a subdirectory of the repository. + ## [1.4.0](https://github.com/SocketDev/socket-cli/releases/tag/v1.4.0) - 2026-09-29 ### Changed diff --git a/src/commands/fix/coana-fix-dynamic-sbom-inference.test.mts b/src/commands/fix/coana-fix-dynamic-sbom-inference.test.mts index 09fe55e02..7a5fddbae 100644 --- a/src/commands/fix/coana-fix-dynamic-sbom-inference.test.mts +++ b/src/commands/fix/coana-fix-dynamic-sbom-inference.test.mts @@ -232,7 +232,7 @@ describe('socket fix --dynamic-sbom-inference', () => { }) mockGitUnstagedModifiedFiles.mockResolvedValue({ ok: true, - data: ['app/build.gradle', 'gradle/versions.gradle', 'README.md'], + data: ['app/build.gradle', 'gradle/versions.gradle'], }) await coanaFix({ ...baseConfig, ghsas: ['GHSA-1111-1111-1111'] }) diff --git a/src/commands/fix/coana-fix-pr-files.test.mts b/src/commands/fix/coana-fix-pr-files.test.mts new file mode 100644 index 000000000..633610469 --- /dev/null +++ b/src/commands/fix/coana-fix-pr-files.test.mts @@ -0,0 +1,241 @@ +import { promises as fs } from 'node:fs' + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +import { coanaFix } from './coana-fix.mts' + +import type { FixConfig } from './types.mts' + +const mockSpawnCoanaDlx = vi.hoisted(() => vi.fn()) +const mockSetupSdk = vi.hoisted(() => vi.fn()) +const mockFetchSupportedScanFileNames = vi.hoisted(() => vi.fn()) +const mockGetPackageFilesForScan = vi.hoisted(() => vi.fn()) +const mockHandleApiCall = vi.hoisted(() => vi.fn()) +const mockGetFixEnv = vi.hoisted(() => vi.fn()) +const mockGetSocketFixPrs = vi.hoisted(() => vi.fn()) +const mockFetchGhsaDetails = vi.hoisted(() => vi.fn()) +const mockGitUnstagedModifiedFiles = vi.hoisted(() => vi.fn()) +const mockGitUntrackedFiles = vi.hoisted(() => vi.fn()) +const mockGitCommit = vi.hoisted(() => vi.fn()) + +vi.mock('../../utils/dlx.mts', () => ({ + spawnCoanaDlx: mockSpawnCoanaDlx, +})) + +vi.mock('../../utils/sdk.mts', () => ({ + setupSdk: mockSetupSdk, +})) + +vi.mock('../scan/fetch-supported-scan-file-names.mts', () => ({ + fetchSupportedScanFileNames: mockFetchSupportedScanFileNames, +})) + +vi.mock('../../utils/path-resolve.mts', () => ({ + getPackageFilesForScan: mockGetPackageFilesForScan, +})) + +vi.mock('../../utils/api.mts', () => ({ + handleApiCall: mockHandleApiCall, +})) + +vi.mock('./env-helpers.mts', () => ({ + checkCiEnvVars: vi.fn(() => ({ missing: [], present: [] })), + getCiEnvInstructions: vi.fn(() => 'Set CI env vars'), + getFixEnv: mockGetFixEnv, +})) + +vi.mock('./pull-request.mts', () => ({ + getSocketFixPrs: mockGetSocketFixPrs, + openSocketFixPr: vi.fn(async () => ({ ok: false, error: new Error('') })), +})) + +vi.mock('../../utils/github.mts', () => ({ + enablePrAutoMerge: vi.fn(), + fetchGhsaDetails: mockFetchGhsaDetails, + setGitRemoteGithubRepoUrl: vi.fn(), +})) + +vi.mock('../../utils/git.mts', () => ({ + gitCheckoutBranch: vi.fn(() => Promise.resolve(true)), + gitCommit: mockGitCommit, + gitCreateBranch: vi.fn(() => Promise.resolve(true)), + gitDeleteBranch: vi.fn(() => Promise.resolve(true)), + gitPushBranch: vi.fn(() => Promise.resolve(true)), + gitRemoteBranchExists: vi.fn(() => Promise.resolve(false)), + gitResetAndClean: vi.fn(() => Promise.resolve(true)), + gitUnstagedModifiedFiles: mockGitUnstagedModifiedFiles, + gitUntrackedFiles: mockGitUntrackedFiles, +})) + +vi.mock('./branch-cleanup.mts', () => ({ + cleanupErrorBranches: vi.fn(), + cleanupFailedPrBranches: vi.fn(), + cleanupStaleBranch: vi.fn(() => Promise.resolve(true)), + cleanupSuccessfulPrLocalBranch: vi.fn(), +})) + +type Worktree = { modified: string[]; untracked: string[] } + +function committedFiles(): string[] { + expect(mockGitCommit).toHaveBeenCalledTimes(1) + return mockGitCommit.mock.calls[0]![1] as string[] +} + +describe('socket fix PR mode commits', () => { + const baseConfig: FixConfig = { + all: false, + applyFixes: true, + autopilot: false, + coanaVersion: undefined, + cwd: '/test/cwd', + debug: false, + disableExternalToolChecks: false, + disableMajorUpdates: false, + ecosystems: [], + exclude: [], + excludePaths: [], + ghsas: ['GHSA-f8q6-p94x-37v3'], + include: [], + minSatisfying: false, + minimumReleaseAge: '', + orgSlug: 'test-org', + outputFile: '', + packageManagers: [], + prCheck: true, + prLimit: 10, + rangeStyle: 'preserve', + showAffectedDirectDependencies: false, + silence: true, + spinner: undefined, + unknownFlags: [], + } + + let worktree: Worktree + + function applyFix( + changes: Worktree, + modifiedFiles: string[] | undefined, + ): void { + mockSpawnCoanaDlx.mockImplementation(async (args: string[]) => { + worktree = { + modified: [...new Set([...worktree.modified, ...changes.modified])], + untracked: [...new Set([...worktree.untracked, ...changes.untracked])], + } + await fs.writeFile( + args[args.indexOf('--output-file') + 1]!, + JSON.stringify({ type: 'applied-fixes', fixes: {}, modifiedFiles }), + ) + return { ok: true, data: '' } + }) + } + + beforeEach(() => { + vi.clearAllMocks() + worktree = { modified: [], untracked: [] } + mockSetupSdk.mockResolvedValue({ + ok: true, + data: { uploadManifestFiles: vi.fn() }, + }) + mockFetchSupportedScanFileNames.mockResolvedValue({ ok: true, data: {} }) + mockGetPackageFilesForScan.mockResolvedValue([ + '/test/cwd/package.json', + '/test/cwd/package-lock.json', + '/test/cwd/pnpm-lock.yaml', + ]) + mockHandleApiCall.mockResolvedValue({ ok: true, data: { tarHash: 'hash' } }) + mockGetFixEnv.mockResolvedValue({ + baseBranch: 'main', + githubToken: 'test-token', + gitEmail: 'test@example.com', + gitUser: 'test-user', + isCi: true, + repoInfo: { defaultBranch: 'main', owner: 'o', repo: 'r' }, + }) + mockGetSocketFixPrs.mockResolvedValue([]) + mockFetchGhsaDetails.mockResolvedValue(new Map()) + mockGitUnstagedModifiedFiles.mockImplementation(async () => ({ + ok: true, + data: worktree.modified, + })) + mockGitUntrackedFiles.mockImplementation(async () => ({ + ok: true, + data: worktree.untracked, + })) + mockGitCommit.mockResolvedValue(true) + }) + + it.each(['package-lock.json', 'pnpm-lock.yaml'])( + 'commits a transitive override bump coana leaves out of modifiedFiles (%s)', + async lockfile => { + applyFix({ modified: ['package.json', lockfile], untracked: [] }, [ + lockfile, + ]) + + await coanaFix(baseConfig) + + expect(committedFiles()).toEqual(['package.json', lockfile]) + }, + ) + + it('commits changed tracked files when coana reports no modifiedFiles', async () => { + applyFix( + { + modified: ['app/build.gradle', 'gradle/versions.gradle'], + untracked: [], + }, + undefined, + ) + + await coanaFix(baseConfig) + + expect(committedFiles()).toEqual([ + 'app/build.gradle', + 'gradle/versions.gradle', + ]) + }) + + it('leaves out files that were dirty before the fix unless coana reports them', async () => { + worktree = { + modified: ['README.md', 'package-lock.json'], + untracked: ['notes.txt', 'app/.socket.facts.json'], + } + applyFix({ modified: ['package.json'], untracked: [] }, [ + 'package-lock.json', + ]) + + await coanaFix(baseConfig) + + expect(committedFiles()).toEqual(['package-lock.json', 'package.json']) + }) + + it('commits new files only when coana reports them or they are manifests', async () => { + applyFix( + { + modified: ['build.sbt'], + untracked: [ + 'project/SocketDependencyOverrides.scala', + 'sub/package-lock.json', + 'target/classes/App.class', + ], + }, + ['build.sbt', 'project/SocketDependencyOverrides.scala'], + ) + + await coanaFix(baseConfig) + + expect(committedFiles()).toEqual([ + 'build.sbt', + 'project/SocketDependencyOverrides.scala', + 'sub/package-lock.json', + ]) + }) + + it('skips the fix when it changes nothing', async () => { + worktree = { modified: ['package.json'], untracked: [] } + applyFix({ modified: [], untracked: [] }, []) + + await coanaFix(baseConfig) + + expect(mockGitCommit).not.toHaveBeenCalled() + }) +}) diff --git a/src/commands/fix/coana-fix.mts b/src/commands/fix/coana-fix.mts index 6a7d17c3c..e76a91e33 100644 --- a/src/commands/fix/coana-fix.mts +++ b/src/commands/fix/coana-fix.mts @@ -198,6 +198,46 @@ function isFactsFile(filepath: string): boolean { return path.basename(filepath).toLowerCase() === DOT_SOCKET_DOT_FACTS_JSON } +type GitWorkingTreeChanges = { + modified: string[] + untracked: string[] +} + +async function gitWorkingTreeChanges( + cwd: string, +): Promise { + const { 0: modifiedCResult, 1: untrackedCResult } = await Promise.all([ + gitUnstagedModifiedFiles(cwd), + gitUntrackedFiles(cwd), + ]) + return { + modified: modifiedCResult.ok ? modifiedCResult.data : [], + untracked: untrackedCResult.ok ? untrackedCResult.data : [], + } +} + +// Coana's modifiedFiles may under-report; untracked files still need it or a +// manifest name, so build output in repos without a .gitignore stays out. +function selectFixedFiles( + before: GitWorkingTreeChanges, + after: GitWorkingTreeChanges, + writtenFiles: Set | undefined, + scanBaseNames: Set, +): string[] { + const dirtyBefore = new Set([...before.modified, ...before.untracked]) + const isFromFix = (relPath: string) => + !!writtenFiles?.has(relPath) || !dirtyBefore.has(relPath) + return [ + ...after.modified.filter(isFromFix), + ...after.untracked.filter( + relPath => + isFromFix(relPath) && + (!!writtenFiles?.has(relPath) || + scanBaseNames.has(path.basename(relPath))), + ), + ] +} + function readWrittenFiles(outputFile: string): Set | undefined { const result = readJsonSync(outputFile, { throws: false }) as | { modifiedFiles?: unknown } @@ -622,6 +662,9 @@ async function coanaFixWithFacts( // eslint-disable-next-line no-await-in-loop await factsSlot?.generated?.restore() + // eslint-disable-next-line no-await-in-loop + const changesBefore = await gitWorkingTreeChanges(cwd) + // Create a temporary file for Coana output. const tmpDir = os.tmpdir() const tmpFile = path.join(tmpDir, `socket-fix-${ghsaId}-${Date.now()}.json`) @@ -688,24 +731,13 @@ async function coanaFixWithFacts( continue ghsaLoop } - // Check for modified files after applying the fix. - // eslint-disable-next-line no-await-in-loop - const unstagedCResult = await gitUnstagedModifiedFiles(cwd) - // Build scripts the fix edits need not be manifests the scan uploads, - // and files it creates are untracked. - const writtenFiles = readWrittenFiles(tmpFile) - // eslint-disable-next-line no-await-in-loop - const untrackedCResult = await gitUntrackedFiles(cwd) - const modifiedFiles = writtenFiles - ? [ - ...(unstagedCResult.ok ? unstagedCResult.data : []), - ...(untrackedCResult.ok ? untrackedCResult.data : []), - ].filter(relPath => writtenFiles.has(relPath)) - : unstagedCResult.ok - ? unstagedCResult.data.filter(relPath => - scanBaseNames.has(path.basename(relPath)), - ) - : [] + const modifiedFiles = selectFixedFiles( + changesBefore, + // eslint-disable-next-line no-await-in-loop + await gitWorkingTreeChanges(cwd), + readWrittenFiles(tmpFile), + scanBaseNames, + ) if (!modifiedFiles.length) { debugFn('notice', `skip: no changes for ${ghsaId}`) diff --git a/src/utils/git.mts b/src/utils/git.mts index 4d804170d..83d50552d 100644 --- a/src/utils/git.mts +++ b/src/utils/git.mts @@ -511,16 +511,19 @@ export async function gitUnstagedModifiedFiles( ): Promise> { const stdioPipeOptions: SpawnOptions = { cwd } try { + // --relative matches the cwd-relative paths of `git ls-files` and + // `git add`; plain `git diff` prints them relative to the repo root. const gitDiffResult = await spawn( 'git', - ['diff', '--name-only'], + ['diff', '--name-only', '--relative', '-z'], stdioPipeOptions, ) - const changedFilesDetails = gitDiffResult.stdout - const relPaths = changedFilesDetails.split('\n') return { ok: true, - data: relPaths.map(p => normalizePath(p)), + data: gitDiffResult.stdout + .split('\0') + .filter(Boolean) + .map(p => normalizePath(p)), } } catch (e) { debugFn('error', 'Failed to get unstaged modified files') @@ -539,13 +542,13 @@ export async function gitUntrackedFiles( try { const result = await spawn( 'git', - ['ls-files', '--others', '--exclude-standard'], + ['ls-files', '--others', '--exclude-standard', '-z'], { cwd }, ) return { ok: true, data: result.stdout - .split('\n') + .split('\0') .filter(Boolean) .map(p => normalizePath(p)), } diff --git a/src/utils/git.test.mts b/src/utils/git.test.mts index 829475023..d4b1f83b2 100644 --- a/src/utils/git.test.mts +++ b/src/utils/git.test.mts @@ -1,4 +1,4 @@ -import { mkdtempSync, rmSync, writeFileSync } from 'node:fs' +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from 'node:fs' import { tmpdir } from 'node:os' import path from 'node:path' @@ -6,7 +6,13 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest' import { spawn } from '@socketsecurity/registry/lib/spawn' -import { getCiBranch, gitBranch } from './git.mts' +import { + getCiBranch, + gitBranch, + gitCommit, + gitUnstagedModifiedFiles, + gitUntrackedFiles, +} from './git.mts' // GitHub Actions sets these in its own runs, so they have to be cleared for // the tests to exercise anything other than the CI job they run inside. @@ -138,3 +144,61 @@ describe('gitBranch', () => { expect(await gitBranch(repoPath)).toBe('feature-branch') }) }) + +describe('working tree changes below the repo root', () => { + let repoPath = '' + let subPath = '' + + beforeEach(async () => { + repoPath = await createTempRepo() + subPath = path.join(repoPath, 'sub') + mkdirSync(subPath) + writeFileSync(path.join(subPath, 'package.json'), '{}\n') + writeFileSync(path.join(repoPath, 'root.json'), '{}\n') + await spawn('git', ['add', '.'], { cwd: repoPath }) + await spawn('git', ['commit', '-m', 'Add manifests'], { cwd: repoPath }) + writeFileSync(path.join(subPath, 'package.json'), '{"a":1}\n') + writeFileSync(path.join(repoPath, 'root.json'), '{"a":1}\n') + writeFileSync(path.join(subPath, 'new file.txt'), '\n') + }) + + afterEach(() => { + rmSync(repoPath, { force: true, recursive: true }) + }) + + it('lists modified and untracked files relative to cwd', async () => { + expect(await gitUnstagedModifiedFiles(subPath)).toEqual({ + ok: true, + data: ['package.json'], + }) + expect(await gitUntrackedFiles(subPath)).toEqual({ + ok: true, + data: ['new file.txt'], + }) + }) + + it('commits the listed paths from cwd', async () => { + const modified = await gitUnstagedModifiedFiles(subPath) + const untracked = await gitUntrackedFiles(subPath) + const filepaths = [ + ...(modified.ok ? modified.data : []), + ...(untracked.ok ? untracked.data : []), + ] + expect( + await gitCommit('Fix', filepaths, { + cwd: subPath, + email: 'test@socket.dev', + user: 'Socket Test', + }), + ).toBe(true) + const committed = ( + await spawn('git', ['show', '--name-only', '--format=', 'HEAD'], { + cwd: repoPath, + }) + ).stdout + expect(committed.split('\n')).toEqual([ + 'sub/new file.txt', + 'sub/package.json', + ]) + }) +})