fix(python): validate BEST_EFFORTS requires VIRTUAL_ENV - #570
Open
a-oren wants to merge 5 commits into
Open
Conversation
Add early validation in PythonControllerBase.getDependencies() that throws a RuntimeException when BEST_EFFORTS=true is set without VIRTUAL_ENV=true. Without this check, the best-efforts install logic is silently skipped because it is gated behind automaticallyInstallPackageOnEnvironment(), which returns false for PythonControllerRealEnv. Implements TC-5477 Assisted-by: Claude Code
Contributor
Reviewer's GuideAdds an early validation in PythonControllerBase.getDependencies to enforce that BEST_EFFORTS installs are only allowed when VIRTUAL_ENV is enabled, and introduces a unit test in PythonControllerRealEnvTest that reproduces and asserts the new behavior and error messaging. Sequence diagram for BEST_EFFORTS and VIRTUAL_ENV validation in getDependenciessequenceDiagram
participant Caller
participant PythonControllerBase
participant Environment
Caller->>PythonControllerBase: getDependencies(pathToPythonBin, requirementsPath)
PythonControllerBase->>PythonControllerBase: isVirtualEnv()
PythonControllerBase->>PythonControllerBase: isRealEnv()
alt isVirtualEnv or isRealEnv
PythonControllerBase->>PythonControllerBase: prepareEnvironment(pathToPythonBin)
end
PythonControllerBase->>Environment: getBoolean(PROP_TRUSTIFY_DA_PYTHON_INSTALL_BEST_EFFORTS, false)
Environment-->>PythonControllerBase: bestEfforts
alt bestEfforts and not automaticallyInstallPackageOnEnvironment()
PythonControllerBase->>Caller: throw RuntimeException
else
PythonControllerBase->>PythonControllerBase: automaticallyInstallPackageOnEnvironment()
alt automaticallyInstallPackageOnEnvironment
PythonControllerBase->>Environment: getBoolean(PROP_TRUSTIFY_DA_PYTHON_INSTALL_BEST_EFFORTS, false)
Environment-->>PythonControllerBase: installBestEfforts
PythonControllerBase->>PythonControllerBase: installAndResolveDependencies(installBestEfforts)
else
PythonControllerBase->>PythonControllerBase: resolveDependenciesWithoutInstall()
end
PythonControllerBase-->>Caller: dependencies
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Consider throwing a more specific exception type (e.g., IllegalStateException) instead of a generic RuntimeException for the conflicting BEST_EFFORTS/VIRTUAL_ENV configuration to make failure modes clearer and easier to catch.
- You call Environment.getBoolean(PROP_TRUSTIFY_DA_PYTHON_INSTALL_BEST_EFFORTS, false) twice in getDependencies; consider reading it once into a local variable to avoid duplication and ensure consistency.
- The BEST_EFFORTS/VIRTUAL_ENV configuration check currently runs after prepareEnvironment; if prepareEnvironment can have side effects, it may be cleaner to validate configuration before preparing the environment so invalid setups fail as early as possible.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider throwing a more specific exception type (e.g., IllegalStateException) instead of a generic RuntimeException for the conflicting BEST_EFFORTS/VIRTUAL_ENV configuration to make failure modes clearer and easier to catch.
- You call Environment.getBoolean(PROP_TRUSTIFY_DA_PYTHON_INSTALL_BEST_EFFORTS, false) twice in getDependencies; consider reading it once into a local variable to avoid duplication and ensure consistency.
- The BEST_EFFORTS/VIRTUAL_ENV configuration check currently runs after prepareEnvironment; if prepareEnvironment can have side effects, it may be cleaner to validate configuration before preparing the environment so invalid setups fail as early as possible.
## Individual Comments
### Comment 1
<location path="src/test/java/io/github/guacsec/trustifyda/utils/PythonControllerRealEnvTest.java" line_range="339-343" />
<code_context>
+ /** Verifies that BEST_EFFORTS=true without VIRTUAL_ENV=true throws a descriptive error. */
+ @Test
+ @RestoreSystemProperties
+ @SetSystemProperty(key = PROP_TRUSTIFY_DA_PYTHON_INSTALL_BEST_EFFORTS, value = "true")
+ void best_Efforts_Without_Virtual_Env_Should_Throw_Runtime_Exception() {
+ String requirementsTxt = getFileFromString("requirements.txt", "flask==9.9.9\n");
+ RuntimeException runtimeException =
+ assertThrows(
+ RuntimeException.class,
+ () -> pythonControllerRealEnv.getDependencies(requirementsTxt, true));
+ assertTrue(
+ runtimeException.getMessage().contains(PROP_TRUSTIFY_DA_PYTHON_INSTALL_BEST_EFFORTS));
+ assertTrue(runtimeException.getMessage().contains(PROP_TRUSTIFY_DA_PYTHON_VIRTUAL_ENV));
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Make the VIRTUAL_ENV setting explicit in the test to avoid reliance on global state
This test should cover the case where BEST_EFFORTS=true and VIRTUAL_ENV is explicitly disabled/absent, but it currently only sets BEST_EFFORTS and depends on whatever VIRTUAL_ENV state happens to exist. Please explicitly set or clear `PROP_TRUSTIFY_DA_PYTHON_VIRTUAL_ENV` (e.g., with another `@SetSystemProperty` or a helper) so the scenario is isolated from other tests and not affected by global configuration leakage.
```suggestion
/** Verifies that BEST_EFFORTS=true without VIRTUAL_ENV=true throws a descriptive error. */
@Test
@RestoreSystemProperties
@SetSystemProperty(key = PROP_TRUSTIFY_DA_PYTHON_INSTALL_BEST_EFFORTS, value = "true")
@SetSystemProperty(key = PROP_TRUSTIFY_DA_PYTHON_VIRTUAL_ENV, value = "false")
void best_Efforts_Without_Virtual_Env_Should_Throw_Runtime_Exception() {
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #570 +/- ##
=======================================
Coverage ? 68.74%
Complexity ? 1002
=======================================
Files ? 65
Lines ? 4255
Branches ? 746
=======================================
Hits ? 2925
Misses ? 990
Partials ? 340
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
Contributor
Author
|
[sdlc-workflow/verify-pr] Re: @sourcery-ai[bot] review — 1. IllegalStateException instead of RuntimeException: Classified as code change request (upgraded from suggestion) — this matches project convention: 16 instances of 2. Read Environment.getBoolean once into local variable: Classified as suggestion — no documented convention or consistent codebase pattern requiring local variable extraction for repeated method calls. No sub-task created. 3. Validate configuration before prepareEnvironment: Classified as suggestion — CONVENTIONS.md mentions "Environment validation at initialization time" but no codebase pattern mandates validation-before-setup ordering within method bodies. No sub-task created. |
Contributor
Author
Verification Report for TC-5477 (commit 21e42b8)
Overall: WARNTwo WARN checks:
This comment was AI-generated by sdlc-workflow/verify-pr v0.13.8. |
Change RuntimeException to IllegalStateException for conflicting configuration state validations in PythonControllerBase.getDependencies(). This aligns with the codebase convention where IllegalStateException is used for invalid/conflicting configuration states (16 existing instances across providers). Both conflicting-settings throw statements are updated: - BEST_EFFORTS=true without VIRTUAL_ENV=true (line 83) - BEST_EFFORTS=true with MATCH_MANIFEST_VERSIONS=true (line 102) The test assertion in PythonControllerRealEnvTest is updated to expect IllegalStateException. The VirtualEnvTest assertion still passes since IllegalStateException extends RuntimeException. Implements TC-5493 Assisted-by: Claude Code
…epareEnvironment Read Environment.getBoolean(PROP_TRUSTIFY_DA_PYTHON_INSTALL_BEST_EFFORTS) once into a local variable to avoid duplication and ensure consistency. Move the BEST_EFFORTS/VIRTUAL_ENV validation before prepareEnvironment() so invalid configurations fail before any side effects occur. Implements TC-5493 Assisted-by: Claude Code
…_EFFORTS test Set PROP_TRUSTIFY_DA_PYTHON_VIRTUAL_ENV to "false" explicitly instead of relying on the default state, improving test isolation. Implements TC-5492 Assisted-by: Claude Code
Update README to clarify that BEST_EFFORTS=true requires both VIRTUAL_ENV=true and MATCH_MANIFEST_VERSIONS=false. Implements TC-5477 Assisted-by: Claude Code
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
PythonControllerBase.getDependencies()that throws whenTRUSTIFY_DA_PYTHON_INSTALL_BEST_EFFORTS=trueis set withoutTRUSTIFY_DA_PYTHON_VIRTUAL_ENV=trueautomaticallyInstallPackageOnEnvironment(), which returnsfalseforPythonControllerRealEnvPythonControllerRealEnvTestverifying the error message names both environment variablesRuntimeExceptiontoIllegalStateExceptionfor both conflicting-settings validations ingetDependencies(), aligning with the codebase convention (16 instances ofIllegalStateExceptionfor configuration conflicts across providers)@SetSystemProperty(key = PROP_TRUSTIFY_DA_PYTHON_VIRTUAL_ENV, value = "false")to BEST_EFFORTS test for proper test isolationImplements TC-5477
Implements TC-5493
Implements TC-5492
Test plan
best_Efforts_Without_Virtual_Env_Should_Throw_Runtime_Exceptionpasses (now assertsIllegalStateException)PythonControllerRealEnvTesttests pass (no regression)PythonControllerVirtualEnvTesttests pass (BEST_EFFORTS+VIRTUAL_ENV flow unchanged)