Conversation
Contributor
|
🧙 Sourcery has finished reviewing your pull request! 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 3 issues, and left some high level feedback:
- In
discoverUvWorkspaceMembers,manifestsis anArrayListbut lateraddFirst(rootPyproject)is called, which will not compile; consider usingmanifests.add(0, rootPyproject)or switchingmanifeststo aLinkedList. - The
Files.walk(workspaceDir)indiscoverUvWorkspaceMembersdoes not use the ignore patterns to prune traversal (they are only applied after discovery viaWorkspaceUtils.filterByIgnorePatterns), which could lead to unnecessary scanning of large directories likenode_modulesor.git; consider incorporating ignore patterns into the walk itself.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `discoverUvWorkspaceMembers`, `manifests` is an `ArrayList` but later `addFirst(rootPyproject)` is called, which will not compile; consider using `manifests.add(0, rootPyproject)` or switching `manifests` to a `LinkedList`.
- The `Files.walk(workspaceDir)` in `discoverUvWorkspaceMembers` does not use the ignore patterns to prune traversal (they are only applied after discovery via `WorkspaceUtils.filterByIgnorePatterns`), which could lead to unnecessary scanning of large directories like `node_modules` or `.git`; consider incorporating ignore patterns into the walk itself.
## Individual Comments
### Comment 1
<location path="src/main/java/io/github/guacsec/trustifyda/impl/ExhortApi.java" line_range="1080-1081" />
<code_context>
+ });
+ }
+
+ if (PyprojectTomlUtils.getProjectName(toml) != null) {
+ manifests.addFirst(rootPyproject);
+ }
+
</code_context>
<issue_to_address>
**issue (bug_risk):** ArrayList does not support addFirst; this line will not compile or will behave incorrectly if the type changes later.
To fix this, either change `manifests` to a `LinkedList`/`Deque` implementation, or keep it as an `ArrayList` and use `manifests.add(0, rootPyproject);` instead. If maintaining discovery order is important, consider documenting that intent in the surrounding code or comments.
</issue_to_address>
### Comment 2
<location path="src/main/java/io/github/guacsec/trustifyda/impl/ExhortApi.java" line_range="1085-1086" />
<code_context>
+ }
+
+ return WorkspaceUtils.filterByIgnorePatterns(workspaceDir, manifests, ignorePatterns);
+ } catch (Exception e) {
+ LOG.warning("Failed to discover uv workspace members: " + e.getMessage());
+ return Collections.emptyList();
+ }
</code_context>
<issue_to_address>
**suggestion:** Logging only the exception message may make diagnosing discovery failures difficult.
Using only `e.getMessage()` drops the stack trace and root cause, which makes diagnosing workspace discovery failures much harder. Prefer logging the full exception (e.g., `LOG.log(Level.WARNING, "Failed to discover uv workspace members", e)`) so production issues remain debuggable while still returning an empty list.
Suggested implementation:
```java
return WorkspaceUtils.filterByIgnorePatterns(workspaceDir, manifests, ignorePatterns);
} catch (Exception e) {
LOG.warning("Failed to discover uv workspace members: " + e.getMessage());
return Collections.emptyList();
}
```
```java
return WorkspaceUtils.filterByIgnorePatterns(workspaceDir, manifests, ignorePatterns);
} catch (Exception e) {
LOG.log(Level.WARNING, "Failed to discover uv workspace members", e);
return Collections.emptyList();
}
private static final Set<String> DEFAULT_WORKSPACE_DISCOVERY_IGNORE =
```
1. Ensure that `java.util.logging.Level` is imported at the top of `ExhortApi.java`, e.g.:
`import java.util.logging.Level;`
2. If the logger is not a `java.util.logging.Logger` but another logging framework (e.g., SLF4J), adjust the logging call accordingly (for SLF4J it would be `LOG.warn("Failed to discover uv workspace members", e);` instead of using `Level`).
</issue_to_address>
### Comment 3
<location path="src/test/java/io/github/guacsec/trustifyda/impl/UvWorkspaceDiscoveryTest.java" line_range="33-42" />
<code_context>
+ "libs" + File.separator + "core" + File.separator + "pyproject.toml"));
+ }
+
+ @Test
+ void discoverWorkspaceManifests_uvNoLockFile() throws IOException {
+ Path workspaceDir = UV_FIXTURES.resolve("uv_workspace_no_lock").toAbsolutePath().normalize();
+
+ ExhortApi api = new ExhortApi(Mockito.mock(java.net.http.HttpClient.class));
+ List<Path> manifests = api.discoverWorkspaceManifests(workspaceDir, Set.of());
+
+ assertThat(manifests).isEmpty();
+ }
+
+ @Test
+ void discoverWorkspaceManifests_uvNoWorkspaceConfig() throws IOException {
+ Path workspaceDir = UV_FIXTURES.resolve("uv_workspace_no_config").toAbsolutePath().normalize();
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add a test for uv workspace configs with empty or whitespace-only `members`/`exclude` arrays
`discoverUvWorkspaceMembers` explicitly ignores null/blank entries in `members` and `exclude`, and returns an empty result when `memberPatterns` is empty. Existing tests cover no workspace config and normal members, but not the case where `[tool.uv.workspace]` exists and these arrays contain only empty or whitespace-only strings. Please add a fixture and test for that scenario to verify the filtering logic and avoid treating such configs as valid members in future.
Suggested implementation:
```java
@Test
void discoverWorkspaceManifests_uvWhitespaceOnlyMembersAndExclude() throws IOException {
Path workspaceDir =
UV_FIXTURES.resolve("uv_workspace_whitespace_only").toAbsolutePath().normalize();
ExhortApi api = new ExhortApi(Mockito.mock(java.net.http.HttpClient.class));
List<Path> manifests = api.discoverWorkspaceManifests(workspaceDir, Set.of());
assertThat(manifests).isEmpty();
}
}
```
1. Add a new test fixture directory at `src/test/resources/tst_manifests/workspace/uv/uv_workspace_whitespace_only`.
2. Inside that directory, create a `pyproject.toml` (or equivalent workspace config used by `discoverUvWorkspaceMembers`) containing a `[tool.uv.workspace]` section where both `members` and `exclude` are arrays with only empty and/or whitespace-only strings, for example:
- `members = ["", " ", " "]`
- `exclude = ["", " "]`
3. Ensure that this fixture does not contain any other valid member patterns that would cause `discoverUvWorkspaceMembers` to return non-empty `memberPatterns`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Member
Author
Verification Report — TC-4266PR: #446 | Branch: Review Feedback
All 3 threads classified in prior run; no new threads. CI Status46/46 checks PASS Scope ContainmentSingle clean commit Previously: PR had wrong base branch (TC-4264) and included Go workspace commits. Fixed by rebasing onto Sensitive PatternsNo secrets, credentials, or API keys detected. Acceptance Criteria
Test Quality7 tests in Verification Commands
This comment was AI-generated by sdlc-workflow/verify-pr v0.5.11. Updated after branch cleanup (rebase onto main, single clean commit). |
Parse [tool.uv.workspace] member and exclude globs from pyproject.toml to discover all workspace member manifests. Virtual workspaces (no [project] section) correctly exclude the root pyproject.toml. Adds __pycache__ and .venv to default ignore patterns. Also fix Java PathMatcher quirk where **/X/** doesn't match root-level paths in WorkspaceUtils.filterByIgnorePatterns. TC-4266 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
Test Results460 tests 460 ✅ 1m 41s ⏱️ Results for commit 8602496. |
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
1 participant
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
[tool.uv.workspace]member and exclude globs frompyproject.tomlto discover workspace member manifests[project]section) correctly exclude rootpyproject.tomlfrom results__pycache__and.venvto default ignore patternsPyprojectTomlUtilsandWorkspaceUtilsfor TOML parsing and ignore filteringTest plan
TC-4266
🤖 Generated with Claude Code
Summary by Sourcery
Add support for discovering Python uv workspaces during workspace manifest resolution and extend default ignore patterns.
New Features:
Enhancements:
Tests: