From 637869fc24ce2e609f51ed4993b688308d6dfd6b Mon Sep 17 00:00:00 2001 From: Jeppe Fredsgaard Blaabjerg Date: Wed, 30 Sep 2026 13:35:32 +0200 Subject: [PATCH 1/2] fix(fix): commit every file a fix changes in PR mode PR mode kept only the git-changed paths that coana listed in modifiedFiles, so anything coana wrote without reporting (e.g. package.json override bumps) was left out of the PR. Now every tracked file the fix changes is committed; untracked files still need coana or a manifest name to vouch for them, and files already dirty before the fix are left out unless coana reports them. git diff printed paths relative to the repo root while git ls-files, git add and coana use cwd-relative paths, so below the repo root the fix was skipped as "no changes". Use --relative, and -z for both lists. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 + .../coana-fix-dynamic-sbom-inference.test.mts | 2 +- src/commands/fix/coana-fix-pr-files.test.mts | 241 ++++++++++++++++++ src/commands/fix/coana-fix.mts | 69 +++-- src/utils/git.mts | 15 +- src/utils/git.test.mts | 68 ++++- 6 files changed, 373 insertions(+), 27 deletions(-) create mode 100644 src/commands/fix/coana-fix-pr-files.test.mts 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..ab5d670d2 100644 --- a/src/commands/fix/coana-fix.mts +++ b/src/commands/fix/coana-fix.mts @@ -198,6 +198,47 @@ 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, so every tracked file the fix +// changes is committed. Untracked files also need coana or a manifest name +// to vouch for them, keeping build output in repos without a .gitignore 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 +663,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 +732,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', + ]) + }) +}) From 985148407bb3a2c1a8b9a81848da43f996c29e5d Mon Sep 17 00:00:00 2001 From: Jeppe Fredsgaard Blaabjerg Date: Wed, 30 Sep 2026 13:38:27 +0200 Subject: [PATCH 2/2] chore(fix): trim selectFixedFiles comment Co-Authored-By: Claude Opus 5.5 --- src/commands/fix/coana-fix.mts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/commands/fix/coana-fix.mts b/src/commands/fix/coana-fix.mts index ab5d670d2..e76a91e33 100644 --- a/src/commands/fix/coana-fix.mts +++ b/src/commands/fix/coana-fix.mts @@ -216,9 +216,8 @@ async function gitWorkingTreeChanges( } } -// Coana's modifiedFiles may under-report, so every tracked file the fix -// changes is committed. Untracked files also need coana or a manifest name -// to vouch for them, keeping build output in repos without a .gitignore out. +// 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,