Refactor NPM package updater #1024

eranturgeman · 2026-01-07T09:55:05Z

All tests passed. If this feature is not already covered by the tests, I added new tests.
This pull request is on the dev branch.
I used gofmt for formatting the code before submitting the pull request.
Update documentation about new features / new supported technologies

This PR changes the NPM package handler to test-based fixes instead of cli command fixes.
As past of the change we ease the installation after a fix is performed to only regenerate the lock file, hence reducing the strict build process we used to have and make the process less error prone

Missing testcase:
We need to add a new integration test test case to TestScanRepositoryCmd_Run that verifies we do not regenerate a lock file if it doesnt exists in remote. since the TestScanRepositoryCmd_Run is not passing right now and we need to fix it I added it to the overall test plan for future addition

… regeneration

… location extraction for a given VulnerabilityDetails

…r-npm-package-updater

eyalk007 · 2026-01-07T15:15:07Z

please provide a link to a fix pr

eyalk007

reviewed most of files
please tell me you are done so i can rereveiw

packagehandlers/npmpackageupdater.go

packagehandlers/commonpackageupdater.go

packagehandlers/npmpackageupdater.go

packagehandlers/npmpackageupdater_test.go

packagehandlers/commonpackageupdater.go

packagehandlers/commonpackageupdater_test.go

packagehandlers/npmpackageupdater.go

eranturgeman · 2026-01-19T11:13:07Z

please provide a link to a fix pr

https://github.com/eranturgeman/npm-small/pull/14

packagehandlers/npmpackageupdater.go

utils/git.go

…r-npm-package-updater

github-actions · 2026-01-25T14:23:07Z

🐸 JFrog Frogbot

packagehandlers/commonpackageupdater.go

eyalk007 · 2026-01-26T08:05:18Z

packagehandlers/npmpackageupdater.go

+func (npm *NpmPackageUpdater) fixVulnerabilityAndRegenerateLockIfNeeded(vulnDetails *utils.VulnerabilityDetails, descriptorPath string, originalWd string, vulnRegexp *regexp.Regexp) (err error) {
+	descriptorContent, err := os.ReadFile(descriptorPath)
+	if err != nil {
+		return fmt.Errorf("failed to read file '%s': %w", descriptorPath, err)
+	}
+
+	if !vulnRegexp.MatchString(strings.ToLower(string(descriptorContent))) {
+		return fmt.Errorf("dependency '%s' not found in descriptor '%s' despite lock file evidence", vulnDetails.ImpactedDependencyName, descriptorPath)
+	}
+
+	backupContent := make([]byte, len(descriptorContent))
+	copy(backupContent, descriptorContent)
+	updatedContent, err := npm.updateVersionInDescriptor(descriptorContent, vulnDetails.ImpactedDependencyName, vulnDetails.SuggestedFixedVersion, descriptorPath)
+	if err != nil {
+		return fmt.Errorf("failed to update version in descriptor: %w", err)
+	}
+
+	if err = os.WriteFile(descriptorPath, updatedContent, 0644); err != nil {
+		return fmt.Errorf("failed to write updated descriptor '%s': %w", descriptorPath, err)
+	}
+
+	descriptorDir := filepath.Dir(descriptorPath)
+	if err = os.Chdir(descriptorDir); err != nil {
+		return fmt.Errorf("failed to change directory to '%s': %w", descriptorDir, err)
+	}
+	defer func() {
+		if chErr := os.Chdir(originalWd); chErr != nil {
+			err = errors.Join(err, fmt.Errorf("failed to return to original directory: %w", chErr))
+		}
+	}()
+
+	lockFileTracked, checkErr := utils.IsFileTrackedByGit(npmLockFileName, originalWd)
+	if checkErr != nil {
+		log.Debug(fmt.Sprintf("Failed to check if lock file is tracked in git: %s. Proceeding with lock file regeneration.", checkErr.Error()))
+		lockFileTracked = true
+	}
+
+	if !lockFileTracked {
+		log.Debug(fmt.Sprintf("Lock file '%s' does not exist in remote, skipping lock file regeneration", npmLockFileName))
+		log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s' without regenerating lock file", vulnDetails.ImpactedDependencyName, vulnDetails.ImpactedDependencyVersion, vulnDetails.SuggestedFixedVersion, descriptorPath))
+		return
+	}
+
+	if err = npm.regenerateLockFileWithRetry(); err != nil {
+		log.Warn(fmt.Sprintf("Failed to regenerate lock file after updating '%s' to version '%s': %s. Rolling back...", vulnDetails.ImpactedDependencyName, vulnDetails.SuggestedFixedVersion, err.Error()))
+		if rollbackErr := os.WriteFile(descriptorPath, backupContent, 0644); rollbackErr != nil {
+			return fmt.Errorf("failed to rollback descriptor after lock file regeneration failure: %w (original error: %v)", rollbackErr, err)
+		}
+		return err
+	}
+	log.Debug(fmt.Sprintf("Successfully updated '%s' from version '%s' to '%s' in descriptor '%s'", vulnDetails.ImpactedDependencyName, vulnDetails.ImpactedDependencyVersion, vulnDetails.SuggestedFixedVersion, descriptorPath))
+	return nil


as i said before this fucntion does multiple things at once, and is 60 lines long and that is making it hard to read, also this is seperation of concerns
separate them

something like this

func (npm *NpmPackageUpdater) updateDirectDependency(vulnDetails *utils.VulnerabilityDetails) error { descriptorPaths, err := npm.getDescriptorsToFixFromVulnerability(vulnDetails) if err != nil { return err } initialWd, err := os.Getwd() if err != nil { return fmt.Errorf("failed to get current working directory: %w", err) } vulnRegexp := BuildPackageRegex(vulnDetails.ImpactedDependencyName, npmDependencyRegexpPattern) for _, descriptorPath := range descriptorPaths { // 1. Fix the descriptor backup, err := npm.fixDescriptor(vulnDetails, descriptorPath, vulnRegexp) if err != nil { log.Warn(...) continue } // 2. Check if lockfile is tracked descriptorDir := filepath.Dir(descriptorPath) if isTracked, _ := npm.isLockfileTracked(descriptorDir, initialWd); !isTracked { log.Debug("Lockfile not tracked, skipping regeneration") continue } // 3. Regenerate lockfile (with rollback on failure) if err := npm.regenerateLockFile(descriptorPath, descriptorDir, backup); err != nil { log.Warn(...) continue } } return nil }

this is just pseudo code but there should be a concept of separation not just one big function

eyalk007 · 2026-01-26T08:13:53Z

packagehandlers/npmpackageupdater.go

+	return nil
+}
+
+func (npm *NpmPackageUpdater) updateVersionInDescriptor(content []byte, packageName, newVersion, descriptorPath string) ([]byte, error) {


Consider replacing manual JSON parsing with tidwall/sjson library:
will remove all the edge cases handling and around 70 lines from file

eyalk007 · 2026-01-26T08:15:54Z

packagehandlers/npmpackageupdater.go

+
+	escapedName := regexp.QuoteMeta(packageName)
+	replacePattern := fmt.Sprintf(npmDependencyReplacePattern, escapedName)
+	replaceRegex := regexp.MustCompile("(?i)" + replacePattern)


npm is case sensitive not sure this is correct

eyalk007 · 2026-01-26T08:22:11Z

packagehandlers/npmpackageupdater.go

+}
+
+// Creates an environment slice with npm isolation variables that override user's .npmrc settings for specific options while allowing registry configuration to pass through
+func (npm *NpmPackageUpdater) buildIsolatedEnv() []string {


please check this

packagehandlers/npmpackageupdater.go

eyalk007

looks a lot better please look at my comments

eranturgeman added 4 commits January 7, 2026 11:44

refactoring npm package handler to use text based fixes and lock file…

aa9c313

… regeneration

improving GetVulnerabilityRegexCompiler and adding extractor for file…

1b56a43

… location extraction for a given VulnerabilityDetails

renamed file to better adjust new package refactoring and naming

23f38c8

adding tests for new functions

5c3e233

eranturgeman requested a deployment to frogbot January 7, 2026 09:55 — with GitHub Actions Waiting

eranturgeman requested review from eyalk007 and orto17 January 7, 2026 09:55

fix UpdateDependency test for NPM

0d44549

eranturgeman requested a deployment to frogbot January 7, 2026 13:48 — with GitHub Actions Waiting

remove comments

5e027cc

eranturgeman had a problem deploying to frogbot January 7, 2026 13:55 — with GitHub Actions Failure

eranturgeman added safe to test Approve running integration tests on a pull request improvement Automatically generated release notes labels Jan 7, 2026

github-actions bot removed the safe to test Approve running integration tests on a pull request label Jan 7, 2026

Merge branch 'v3_er' of https://github.com/jfrog/frogbot into refacto…

42ba784

…r-npm-package-updater

eranturgeman requested a deployment to frogbot January 7, 2026 14:05 — with GitHub Actions Waiting

improve tests

28504c1

eranturgeman requested a deployment to frogbot January 8, 2026 07:57 — with GitHub Actions Waiting

/

888f5a9

eranturgeman requested a deployment to frogbot January 8, 2026 07:58 — with GitHub Actions Waiting

eyalk007 requested changes Jan 8, 2026

View reviewed changes

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/commonpackageupdater.go Outdated Show resolved Hide resolved

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/commonpackageupdater.go Show resolved Hide resolved

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/commonpackageupdater_test.go Show resolved Hide resolved

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/npmpackageupdater.go Show resolved Hide resolved

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/npmpackageupdater.go Outdated Show resolved Hide resolved

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/npmpackageupdater.go Outdated Show resolved Hide resolved

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/npmpackageupdater.go Show resolved Hide resolved

orto17 reviewed Jan 12, 2026

View reviewed changes

packagehandlers/npmpackageupdater.go Outdated Show resolved Hide resolved

.

5367593

eranturgeman requested a deployment to frogbot January 19, 2026 10:36 — with GitHub Actions Waiting

fix and add tests and linter issues

154f0da

eranturgeman requested a deployment to frogbot January 19, 2026 10:58 — with GitHub Actions Waiting

linter

e9fd374

eranturgeman temporarily deployed to frogbot January 19, 2026 11:12 — with GitHub Actions Inactive

eranturgeman requested review from eyalk007 and orto17 January 19, 2026 11:13

eranturgeman added the safe to test Approve running integration tests on a pull request label Jan 19, 2026

github-actions bot removed the safe to test Approve running integration tests on a pull request label Jan 19, 2026

orto17 reviewed Jan 21, 2026

View reviewed changes

packagehandlers/npmpackageupdater.go Show resolved Hide resolved

eyalk007 reviewed Jan 21, 2026

View reviewed changes

utils/git.go Outdated Show resolved Hide resolved

change IsFileExistsInRemote to IsFileTrackedByGit for simplicity

5d1bc14

eranturgeman requested a deployment to frogbot January 25, 2026 12:29 — with GitHub Actions Waiting

eranturgeman added the safe to test Approve running integration tests on a pull request label Jan 25, 2026

github-actions bot removed the safe to test Approve running integration tests on a pull request label Jan 25, 2026

Merge branch 'v3_er' of https://github.com/jfrog/frogbot into refacto…

b8af6f3

…r-npm-package-updater

eranturgeman deployed to frogbot January 25, 2026 14:00 — with GitHub Actions Active

eranturgeman added the safe to test Approve running integration tests on a pull request label Jan 25, 2026

github-actions bot removed the safe to test Approve running integration tests on a pull request label Jan 25, 2026

eyalk007 reviewed Jan 26, 2026

View reviewed changes

packagehandlers/commonpackageupdater.go Show resolved Hide resolved

eyalk007 reviewed Jan 26, 2026

View reviewed changes

packagehandlers/commonpackageupdater.go Show resolved Hide resolved

eyalk007 reviewed Jan 26, 2026

View reviewed changes

packagehandlers/npmpackageupdater.go Show resolved Hide resolved

eyalk007 requested changes Jan 26, 2026

View reviewed changes

Refactor NPM package updater #1024

Are you sure you want to change the base?

Refactor NPM package updater #1024

Conversation

eranturgeman commented Jan 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

eyalk007 commented Jan 7, 2026

Uh oh!

eyalk007 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

eranturgeman commented Jan 19, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Jan 25, 2026

Uh oh!

Uh oh!

Uh oh!

eyalk007 Jan 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

eyalk007 Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

eyalk007 Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

eyalk007 Jan 26, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

eyalk007 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

eranturgeman commented Jan 7, 2026 •

edited

Loading

eranturgeman commented Jan 19, 2026 •

edited

Loading

eyalk007 Jan 26, 2026 •

edited

Loading