feat(vulnerability-remediation): support upgrading transitive dependencies without adding as top-level#118
Conversation
…ncies without adding as top-level Port changes from OpenHands/vulnerability-fixer#48 When a CVE fix requires upgrading a transitive dependency (not directly declared in manifest files), the remediation agent now attempts to upgrade it without adding it as a top-level dependency in the manifest file. Changes: - Added Step 2: Determine if direct or transitive dependency - Added Step 2a: Upgrading TRANSITIVE Dependencies with package manager-specific commands - Updated UV lockfile section with transitive upgrade guidance - Updated README with Direct vs Transitive Dependencies documentation Co-authored-by: openhands <openhands@all-hands.dev>
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟡 Acceptable - Solves a real problem (avoiding unnecessary top-level deps) but the implementation via massive prompt with 7 package managers worth of special cases is a maintainability concern.
Verdict: ✅ Worth merging with reservations
Key Insight: You're embedding a reference manual into a prompt. When you need 7 different special cases for package managers, the solution isn't a bigger prompt—it's better abstraction.
| ### 2a. Upgrading TRANSITIVE Dependencies (if applicable) | ||
| If the package is a transitive dependency, try upgrading it without adding to manifest files. This is the preferred approach as it keeps dependencies minimal. | ||
|
|
||
| **General Principle**: Most modern package managers support upgrading specific transitive dependencies in the lockfile without adding them as top-level dependencies: | ||
|
|
||
| For EACH lockfile found, attempt the transitive upgrade: | ||
|
|
||
| - **uv (uv.lock)**: | ||
| ```bash | ||
| # Upgrade transitive dependency without modifying pyproject.toml | ||
| uv lock --upgrade-package {vuln.package_name} | ||
|
|
||
| # If the upgrade fails due to version constraints from another package, | ||
| # you may need to upgrade that parent package too: | ||
| uv lock --upgrade-package {vuln.package_name} --upgrade-package <parent_package_that_constrains_it> | ||
| ``` | ||
|
|
||
| - **Poetry (poetry.lock)**: | ||
| ```bash | ||
| # Try updating just the specific package in the lockfile | ||
| poetry update {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **npm (package-lock.json)**: | ||
| ```bash | ||
| # npm can update nested dependencies via overrides in package.json or: | ||
| npm update {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **yarn (yarn.lock)**: | ||
| ```bash | ||
| # yarn supports resolutions in package.json to override transitive deps | ||
| yarn upgrade {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **pnpm (pnpm-lock.yaml)**: | ||
| ```bash | ||
| # pnpm supports overrides in package.json | ||
| pnpm update {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **Cargo (Cargo.lock)**: | ||
| ```bash | ||
| # Cargo can update transitive deps | ||
| cargo update -p {vuln.package_name} | ||
| ``` | ||
|
|
||
| - **Go (go.sum)**: | ||
| ```bash | ||
| # Go modules can update transitive deps | ||
| go get {vuln.package_name}@v{vuln.fixed_version} | ||
| go mod tidy |
There was a problem hiding this comment.
🟠 Important - Prompt Complexity Explosion: You've hardcoded 7 package managers' worth of commands directly into the prompt. This is a maintenance nightmare.
Every time a package manager updates its CLI, you'll need to modify this prompt. Every new package manager means another block of text. This prompt is becoming a reference manual.
Better approach: Consider extracting package manager commands to a separate reference file or lookup table that the agent can consult. The prompt should describe what to do ("attempt transitive upgrade"), not how for every possible tool.
| # Go modules can update transitive deps | ||
| go get {vuln.package_name}@v{vuln.fixed_version} | ||
| go mod tidy | ||
| ``` |
There was a problem hiding this comment.
🟠 Important - Vague Verification: "Verify the upgrade succeeded by checking the lockfile for the new version" - but no concrete command is provided.
How should the agent verify? grep? Parse JSON/YAML/TOML? Different lockfile formats require different approaches.
Suggestion: Provide specific verification commands like:
# For uv.lock
grep '{ name = "{vuln.package_name}", version = "{vuln.fixed_version}"' uv.lock
# For package-lock.json
jq -r '.packages[] | select(.name == "{vuln.package_name}") | .version' package-lock.json| - **uv (uv.lock)**: | ||
| ```bash | ||
| # Upgrade transitive dependency without modifying pyproject.toml | ||
| uv lock --upgrade-package {vuln.package_name} | ||
|
|
||
| # If the upgrade fails due to version constraints from another package, |
There was a problem hiding this comment.
🟡 Suggestion - Hand-wavy Constraint Handling: You tell the agent to upgrade constraining packages if the upgrade fails due to version constraints:
uv lock --upgrade-package {vuln.package_name} --upgrade-package <parent_package_that_constrains_it>But how does the agent identify <parent_package_that_constrains_it>? There's no guidance on parsing the error message or inspecting the dependency tree.
Suggestion: Either provide concrete steps to identify the constraining package (e.g., "parse the uv lock error message for 'required by '") or remove this advanced case and stick with the fallback to direct dependency.
| # Check if the package is listed as a DIRECT dependency in manifest files | ||
| echo "=== Checking for direct dependency declarations ===" | ||
| find . \\( -name node_modules -o -name .venv -o -name venv -o -name vendor -o -name .git -o -name __pycache__ -o -name dist -o -name build \\) -prune -o -name "pyproject.toml" -print | xargs grep -l "{vuln.package_name}" 2>/dev/null | ||
| find . \\( -name node_modules -o -name .venv -o -name venv -o -name vendor -o -name .git \\) -prune -o -name "requirements*.txt" -print | xargs grep -l "{vuln.package_name}" 2>/dev/null | ||
| find . \\( -name node_modules -o -name .venv -o -name venv -o -name vendor -o -name .git \\) -prune -o -name "package.json" -print | xargs grep -l "{vuln.package_name}" 2>/dev/null |
There was a problem hiding this comment.
🟢 Acceptable - Minor Robustness Regression: You changed from -exec grep -l "{vuln.package_name}" {} + to -print | xargs grep -l.
The -exec version is more robust (handles filenames with spaces/special characters better without needing xargs), but the xargs version works fine for typical project structures.
This is a pragmatic trade-off—the xargs pattern is more common and readable for most developers. Just be aware it can fail on edge cases with weird filenames.
| @@ -219,7 +285,9 @@ def create_remediation_prompt(vuln: Vulnerability, repo_name: str) -> str: | |||
| 3. If a version is found, install it: `pipx install uv==$UV_VERSION --force` | |||
| 4. Verify installation: `uv --version | grep "$UV_VERSION"` (proceed only if successful) | |||
| 5. If version extraction fails or returns empty, proceed with the currently installed version and note this in your output | |||
| 6. Run: `uv lock --upgrade-package {vuln.package_name}` or `uv sync` | |||
| 6. For transitive deps: `uv lock --upgrade-package {vuln.package_name}` | |||
| If constraints prevent upgrade: `uv lock --upgrade-package {vuln.package_name} --upgrade-package <constraining_package>` | |||
| For direct deps after manifest update: `uv sync` | |||
|
|
|||
| - **npm (package.json + package-lock.json)**: `cd` to directory, run `npm install` or `npm update {vuln.package_name}` | |||
| - **yarn (package.json + yarn.lock)**: `cd` to directory, run `yarn install` or `yarn upgrade {vuln.package_name}` | |||
| @@ -230,11 +298,11 @@ def create_remediation_prompt(vuln: Vulnerability, repo_name: str) -> str: | |||
| - **Maven (pom.xml)**: Update the version directly in pom.xml | |||
There was a problem hiding this comment.
🟡 Suggestion - No Testing: This is a significant behavior change to the remediation workflow (adding transitive vs direct dependency detection, new fallback logic) with no corresponding tests.
How do you know:
- The agent will correctly parse this complex multi-step prompt?
- The transitive upgrade commands are correct for each package manager?
- The fallback logic actually triggers when transitive upgrade fails?
Recommendation: Add integration tests that verify the prompt generates correct remediation steps for both direct and transitive dependency scenarios across major package managers.
neubig
left a comment
neubig
left a comment
There was a problem hiding this comment.
Hi @aivong-openhands I kinda agree with the auto-reviewer that the prompt is becoming very complex. Do we need all of this info? If so maybe it's OK, but if not we could simplify.
Further to consider: maybe we could refactor this repo to rely on the skill in the OpenHands/extensions repo? Then OpenHands/extensions could be a single source of truth for these directions. This doesn't necessarily need to be addressed in this PR though.
True it is long now. These prompt updates are addressing issues I see in generated vulnerability fixer PRs so I think they are a nice to have. Maybe we could simplify the prompt. Also we already manage two prompts in the vulnerability-fixer as well. Having a single source of truth would be really nice @OpenHands create an issue to simplify to one prompt in https://github.com/OpenHands/vulnerability-fixer for the local and cloud clients. Just to clarify, did we want to refactor https://github.com/OpenHands/vulnerability-fixer to use the plugin/skill from here (extensions)? |
|
I'm on it! aivong-openhands can track my progress at all-hands.dev |
4 participants
Summary
Port changes from OpenHands/vulnerability-fixer#48
When a CVE fix requires upgrading a transitive dependency (not directly declared in manifest files), the remediation agent now attempts to upgrade it without adding it as a top-level dependency in the manifest file.
Changes
scan_and_remediate.pyREADME.mdPackage Manager Commands for Transitive Upgrades
uv lock --upgrade-package <package>poetry update <package>npm update <package>oroverridesyarn upgrade <package>orresolutionspnpm update <package>oroverridescargo update -p <package>go get <package>@v<version>+go mod tidyReferences