Vendetta: single-file and multi-location deps#1680
Conversation
WalkthroughThis pull request refactors vendored dependency handling in the Ficus analyzer. The 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/App/Fossa/Ficus/Analyze.hs`:
- Around line 233-242: classifyLocation currently treats any non-file as
"directory" which misclassifies non-existent paths; update classifyLocation to
check existence explicitly by calling Directory.doesDirectoryExist fullPath (or
a combined exists check) in addition to Directory.doesFileExist, and set locType
to one of "file", "directory", or "not_found" (or similar) based on those
checks, keeping the same JSON shape but returning the explicit type for fullPath
and preserving the original path value loc.
In `@test/Ficus/FicusSpec.hs`:
- Around line 78-95: The test currently uses a fixed tmpDir name
"ficus-vendored-test" which can collide between parallel runs; replace the
tmpDir binding to create a unique temp directory via
Directory.createTempDirectory (use systemTmpDir and a prefix like
"ficus-vendored-test") and use the returned path for creating the vendored
subdir, writing files, calling vendoredDepsToSourceUnit, and for
Directory.removeDirectoryRecursive cleanup; update references to tmpDir in this
test block accordingly so the test uses and cleans up the unique directory.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0f7345ce-bc46-4ef3-b9db-a3e43e749393
📒 Files selected for processing (4)
Changelog.mdsrc/App/Fossa/Ficus/Analyze.hssrc/App/Fossa/Ficus/Types.hstest/Ficus/FicusSpec.hs
| describe "vendoredDepsToSourceUnit" $ do | ||
| it "classifies directories and files correctly in vendored metadata" $ do | ||
| systemTmpDir <- Directory.getTemporaryDirectory | ||
| let tmpDir = systemTmpDir FP.</> "ficus-vendored-test" | ||
| Directory.createDirectoryIfMissing True (tmpDir FP.</> "vendored") | ||
| writeFile (tmpDir FP.</> "sqlite3.c") "" | ||
|
|
||
| let dep = | ||
| FicusVendoredDependency | ||
| { ficusVendoredDependencyName = "github.com/test/dep" | ||
| , ficusVendoredDependencyEcosystem = "git" | ||
| , ficusVendoredDependencyVersion = Nothing | ||
| , ficusVendoredDependencyLocations = ["vendored", "sqlite3.c"] | ||
| } | ||
|
|
||
| result <- vendoredDepsToSourceUnit tmpDir [dep] | ||
|
|
||
| Directory.removeDirectoryRecursive tmpDir |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider using a unique temporary directory name.
Using a fixed temp directory name "ficus-vendored-test" could cause issues if tests run in parallel or if a previous test run left stale files. Consider using createTempDirectory to generate a unique directory name.
♻️ Suggested improvement
describe "vendoredDepsToSourceUnit" $ do
it "classifies directories and files correctly in vendored metadata" $ do
- systemTmpDir <- Directory.getTemporaryDirectory
- let tmpDir = systemTmpDir FP.</> "ficus-vendored-test"
- Directory.createDirectoryIfMissing True (tmpDir FP.</> "vendored")
+ tmpDir <- Directory.createTempDirectory =<< Directory.getTemporaryDirectory $ "ficus-vendored-test"
+ Directory.createDirectoryIfMissing True (tmpDir FP.</> "vendored")
writeFile (tmpDir FP.</> "sqlite3.c") ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/Ficus/FicusSpec.hs` around lines 78 - 95, The test currently uses a
fixed tmpDir name "ficus-vendored-test" which can collide between parallel runs;
replace the tmpDir binding to create a unique temp directory via
Directory.createTempDirectory (use systemTmpDir and a prefix like
"ficus-vendored-test") and use the returned path for creating the vendored
subdir, writing files, calling vendoredDepsToSourceUnit, and for
Directory.removeDirectoryRecursive cleanup; update references to tmpDir in this
test block accordingly so the test uses and cleans up the unique directory.
1 participant
Overview
Important
This is a breaking change. The ficus binary update (PR #163) and this CLI change must be released together.
This PR makes the following changes:
FicusVendoredDependencynow parses"locations"(a JSON array) instead of"path"(a string).vendoredDepsToSourceUnitis now effectful (IO) so it canstateach location to classify it as"file"or"directory"in the vendored metadata sent to Core. This is needed because single-file lib locations are files, not directories.sourceUnitOriginPathsand each gets its own entry in the"vendored"metadata array.Acceptance criteria
"locations"array from ficus vendetta findingssourceUnitOriginPathsTesting plan
This will be our target directory for scanning. It decompress to this:
cd /path/to/ficus cargo build --releasecd /path/to/sparkle cargo run -p sparkle servecd /path/to/sparkle sudo caddy runYou should see the following:
[ { "imports": [], "locator": "git+github.com/jaysmito101/cgl$411c131", "metadata": { "vendored": [ { "path": "cgl.c", "type": "file" }, { "path": "cgl.h", "type": "file" } ] } }, { "imports": [], "locator": "git+github.com/glennrp/libpng$v1.6.54", "metadata": { "vendored": [ { "path": "libpng", "type": "directory" } ] } } ]Risks
Must be released in lockstep with https://github.com/fossas/ficus/pull/163 since the ficus binary is embedded in the CLI and the output format is changing.
Metrics
None
References
Checklist
docs/.docs/README.msand gave consideration to how discoverable or not my documentation is.Changelog.md. If this PR did not mark a release, I added my changes into an## Unreleasedsection at the top..fossa.ymlorfossa-deps.{json.yml}, I updateddocs/references/files/*.schema.jsonAND I have updated example files used byfossa initcommand. You may also need to update these if you have added/removed new dependency type (e.g.pip) or analysis target type (e.g.poetry).docs/references/subcommands/<subcommand>.md.