feat: add E2E test suite with nightly CI, fix dashboard Dockerfile #52
Reference in New Issue
Block a user
Delete Branch "feat/e2e-tests"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
17 E2E tests covering repos, findings, cascade delete, DAST, and stats. Nightly CI with MongoDB. Also fixes dioxus-cli version in Dockerfile.
Compliance scan found 40 issue(s) in this PR:
@@ -0,0 +14,4 @@concurrency:group: nightly-e2ecancel-in-progress: true[high] Hardcoded MongoDB Credentials in Workflow Configuration
The nightly CI workflow (.gitea/workflows/nightly.yml) contains hardcoded MongoDB credentials in the TEST_MONGODB_URI environment variable. These credentials are exposed in the workflow configuration and could be accessed by anyone with read access to the repository, potentially allowing unauthorized access to the test database.
Suggested fix: Remove hardcoded credentials from the workflow file and use GitHub Actions secrets or a secure vault service to inject the database credentials at runtime.
Scanner: code-review/security | CWE: CWE-798
@@ -0,0 +21,4 @@name: E2E Testsruns-on: dockercontainer:image: rust:1.94-bookworm[high] Incorrect Git Checkout in Nightly Workflow
The nightly workflow uses
git checkout FETCH_HEADwhich may not correctly handle shallow clones or specific commit references. When using--depth=1, FETCH_HEAD might point to an unexpected commit or fail entirely if the repository state isn't properly initialized.Suggested fix: Use
git checkout -b temp_branch FETCH_HEADor ensure proper handling of the fetched commit with explicit reference resolution.*Scanner: code-review/logic | *
@@ -0,0 +42,4 @@| tar xz --strip-components=1 -C /usr/local/bin/ sccache-v0.9.1-x86_64-unknown-linux-musl/sccachechmod +x /usr/local/bin/sccacheenv:RUSTC_WRAPPER: ""[high] Missing Error Handling in E2E Test Step
The E2E test step does not explicitly check for failure of the
cargo testcommand. If the tests fail, the workflow will continue without indicating failure, potentially masking test failures.Suggested fix: Add explicit error checking such as
set -ebefore the cargo test command or use|| exit 1to ensure workflow fails on test failure.*Scanner: code-review/logic | *
@@ -0,0 +45,4 @@RUSTC_WRAPPER: ""- name: Run E2E testsrun: cargo test -p compliance-agent --test e2e -- --test-threads=4[medium] Inconsistent error handling in E2E workflow
The nightly E2E workflow uses
cargo testwithout the--quietflag and doesn't capture or log test failures explicitly. If tests fail, the workflow may not properly report the failure to the CI system, potentially masking test failures.Suggested fix: Add explicit error handling and logging around the test execution step, such as using
set -eor checking exit codes explicitly.*Scanner: code-review/convention | *
@@ -0,0 +49,4 @@- name: Show sccache statsrun: sccache --show-statsif: always()[medium] Complex boolean expression in GitHub Actions workflow
The workflow uses an 'if: always()' condition on the final step which could mask failures in previous steps. This makes it harder to detect when tests actually fail since the step will always run regardless of previous step outcomes.
Suggested fix: Remove the 'if: always()' condition and rely on standard workflow failure handling instead. If you need to show stats regardless, consider using a separate job or conditional logic that explicitly checks for success/failure states.
*Scanner: code-review/complexity | *
@@ -1,6 +1,6 @@FROM rust:1.94-bookworm AS builderRUN cargo install dioxus-cli --version 0.7.3RUN cargo install dioxus-cli --version 0.7.4[medium] Incorrect Dependency Version Bump
The dioxus-cli version was bumped from 0.7.3 to 0.7.4. While this seems like a minor update, it could introduce breaking changes or compatibility issues with existing code that depends on the previous version.
Suggested fix: Verify that all dependencies and integrations work correctly with dioxus-cli 0.7.4 before merging this change.
*Scanner: code-review/logic | *
@@ -43,2 +43,4 @@dashmap = { workspace = true }tokio-stream = { workspace = true }[dev-dependencies][medium] Unnecessary Dev Dependencies Added
Several dev-dependencies were added to the compliance-agent Cargo.toml including axum and tower-http. These are typically only needed for integration tests or server-side code but are now part of the main crate's dev-deps, which can increase build times and complexity.
Suggested fix: Move these dependencies to a dedicated integration test profile or remove them if they're not actually required for development/testing purposes.
*Scanner: code-review/logic | *
@@ -0,0 +12,4 @@pub mod scheduler;pub mod ssh;#[allow(dead_code)]pub mod trackers;[low] Unnecessary allow(dead_code) in lib.rs
The
#[allow(dead_code)]attribute is applied to the trackers module in lib.rs, which suggests that the module is intentionally unused. However, this can mask potential dead code issues and makes it harder to maintain the codebase by hiding unused code.Suggested fix: Remove the
#[allow(dead_code)]attribute and either implement functionality for the trackers module or remove it entirely if it's truly unused.*Scanner: code-review/convention | *
@@ -12,3 +1,1 @@#[allow(dead_code)]mod trackers;mod webhooks;use compliance_agent::{agent, api, config, database, scheduler, ssh, webhooks};[medium] Potential Dead Code Removal
The removal of individual module declarations in main.rs and the introduction of a single use statement suggests refactoring. However, there's no clear indication that dead code has been removed or that the new structure is fully validated. This could lead to subtle runtime issues if modules are not properly exposed.
Suggested fix: Ensure that all necessary modules are correctly re-exported through the lib.rs and that the new import structure doesn't break any expected functionality.
*Scanner: code-review/logic | *
[medium] Missing error propagation in main function
The main function in main.rs returns
Result<(), Box<dyn std::error::Error>>but doesn't handle the error case explicitly. While it will panic on error, this is less robust than explicitly handling and logging the error before exiting.Suggested fix: Explicitly handle the error case by logging it before exiting, e.g.,
if let Err(e) = main() { eprintln!("Error: {}", e); std::process::exit(1); }*Scanner: code-review/convention | *
@@ -4,0 +13,4 @@/// A running test server with a unique database.pub struct TestServer {pub base_url: String,[medium] Missing database cleanup on test server drop
The TestServer struct does not implement Drop, which means the cleanup() method is never automatically called when a test server goes out of scope. This can lead to leftover test databases in MongoDB, causing resource leaks and potential test interference.
Suggested fix: Implement the Drop trait for TestServer to ensure cleanup() is called when the server instance is dropped. Add
impl Drop for TestServer { fn drop(&mut self) { tokio::runtime::Handle::current().block_on(self.cleanup()); } }*Scanner: code-review/logic | *
@@ -4,0 +18,4 @@db_name: String,mongodb_uri: String,}[medium] Inconsistent error handling with unwrap() in test code
The test code uses
unwrap_or_else()followed byexpect()in multiple places, which creates inconsistent error handling patterns. Specifically, line 21 usesunwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into())followed byexpect("Failed to connect to MongoDB — is it running?")on line 28. This pattern should be consistent - either useexpect()directly or handle errors more gracefully.Suggested fix: Use consistent error handling by either removing the
unwrap_or_elseand usingexpectdirectly on the environment variable, or refactor to use proper error propagation with?operator.*Scanner: code-review/convention | *
@@ -4,0 +32,4 @@.await.expect("Failed to connect to MongoDB — is it running?");db.ensure_indexes().await.expect("Failed to create indexes");[medium] Complex boolean expression in test server startup
The TestServer::start() function contains a complex boolean expression in the AgentConfig construction that mixes environment variable fallbacks with hardcoded defaults, making it difficult to track which values are being used under what conditions.
Suggested fix: Extract the environment variable loading into separate variables with clear names, and use a more structured approach like a builder pattern or helper functions to make the configuration loading easier to follow and less error-prone.
*Scanner: code-review/complexity | *
@@ -4,0 +74,4 @@// Build the router with the agent extensionlet app = api::routes::build_router().layer(axum::extract::Extension(Arc::new(agent))).layer(tower_http::cors::CorsLayer::permissive());[medium] Deeply nested control flow in server readiness check
The server readiness check in TestServer::start() uses a nested loop with repeated conditional checks that could be flattened to improve readability and reduce the chance of logic errors when modifying the retry mechanism.
Suggested fix: Replace the for loop with a while let Some(result) = client.get(...).send().await.ok() { ... } pattern or extract the health check logic into a separate function to flatten the nesting.
*Scanner: code-review/complexity | *
@@ -4,0 +128,4 @@.post(format!("{}{path}", self.base_url)).json(body).send().await[medium] Hardcoded timeout in health check loop
The health check loop on lines 131-137 has a hardcoded timeout of 50ms between retries and a fixed number of attempts (50). This makes the test brittle and dependent on timing. If the server takes longer to start, the test may fail even though the server is functioning correctly.
Suggested fix: Make the retry interval and maximum attempts configurable or use a more adaptive approach to waiting for the server to become ready.
*Scanner: code-review/convention | *
@@ -4,0 +154,4 @@/// Get the unique database name for direct MongoDB access in tests.pub fn db_name(&self) -> &str {&self.db_name}[medium] Potential panic in test cleanup when dropping database
The cleanup method on line 157 uses
ok()to ignore errors when dropping the database. While this prevents panics, it hides potential failures in database cleanup that could leave test artifacts behind. The test suite should ensure cleanup is reliable or explicitly log failures.Suggested fix: Consider logging cleanup failures or using a more robust cleanup strategy that ensures databases are properly dropped, rather than silently ignoring errors.
*Scanner: code-review/convention | *
@@ -0,0 +7,4 @@.unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into());let client = mongodb::Client::with_uri_str(&mongodb_uri).await.unwrap();let db = client.database(&server.db_name());[medium] Hardcoded MongoDB URI in test code
The test code hardcodes the MongoDB connection string in multiple places instead of using environment variables or configuration. This makes tests less flexible and potentially insecure, as it exposes connection details directly in the source code rather than allowing them to be configured externally.
Suggested fix: Use a centralized configuration approach for database connections in tests, possibly through a shared test utility that handles environment variable loading and connection management consistently.
*Scanner: code-review/convention | *
[medium] Duplicated MongoDB connection setup in test helpers
Multiple test helper functions (
insert_dast_target,insert_pentest_session,insert_attack_node,insert_dast_finding,count_docs) each create their own MongoDB client connection using the same hardcoded URI logic. This creates unnecessary connection overhead and violates DRY principle, making it harder to maintain connection configuration.Suggested fix: Extract MongoDB client creation into a shared helper function that can be reused across all test helpers to reduce duplication and improve maintainability.
*Scanner: code-review/convention | *
@@ -0,0 +9,4 @@let db = client.database(&server.db_name());let result = db.collection::<mongodb::bson::Document>("dast_targets")[medium] Hardcoded MongoDB Credentials in Test Code
The test code contains hardcoded MongoDB credentials ('root:example') in multiple helper functions. While this is only in test files, it represents poor security hygiene and could lead to accidental exposure if the test code were ever committed to a public repository or if the credentials were reused in production code.
Suggested fix: Use environment variables or a secure configuration management system for database credentials even in test environments. Consider using a dedicated test database with unique credentials.
Scanner: code-review/security | CWE: CWE-798
@@ -0,0 +11,4 @@let result = db.collection::<mongodb::bson::Document>("dast_targets").insert_one(mongodb::bson::doc! {"name": name,[medium] Inconsistent MongoDB connection handling across test helper functions
Each helper function creates its own MongoDB connection instead of reusing a shared connection. This leads to unnecessary resource consumption and potential connection pool exhaustion during test execution, especially since all functions connect to the same database.
Suggested fix: Create a single MongoDB client instance and share it among all helper functions, or refactor to use a common connection utility to avoid repeated connection establishment.
*Scanner: code-review/logic | *
[medium] Duplicated MongoDB connection setup
Multiple helper functions repeatedly establish MongoDB connections using the same pattern with environment variable fallback. This creates redundant code and potential inconsistency if the connection logic needs to change.
Suggested fix: Extract MongoDB connection setup into a shared helper function to reduce duplication and improve maintainability.
*Scanner: code-review/complexity | *
@@ -0,0 +12,4 @@.collection::<mongodb::bson::Document>("dast_targets").insert_one(mongodb::bson::doc! {"name": name,"base_url": format!("https://{name}.example.com"),[medium] Inconsistent error handling with unwrap() in test code
The test code uses
unwrap()extensively when interacting with MongoDB connections and database operations. This violates the consistent error handling pattern established in the rest of the codebase where errors should be properly handled or propagated instead of panicking. This makes tests brittle and hides potential connection or database issues.Suggested fix: Replace all
unwrap()calls with proper error handling using?operator or explicit error assertions to make tests more robust and maintainable.*Scanner: code-review/convention | *
[high] Missing error handling for MongoDB operations
The test functions use
.unwrap()on MongoDB operations which will panic if the database operations fail. This could cause tests to crash instead of properly reporting failures, making it difficult to diagnose issues with the cascade delete functionality.Suggested fix: Replace
.unwrap()with proper error handling using?operator or explicit error checking to ensure tests fail gracefully when database operations don't succeed.*Scanner: code-review/logic | *
@@ -0,0 +147,4 @@// All downstream data should be goneassert_eq!(count_docs(&server, "dast_targets").await, 0);assert_eq!(count_docs(&server, "pentest_sessions").await, 0);[high] Insecure Direct Object Reference (IDOR) Potential in Repository Deletion
The repository deletion endpoint (/api/v1/repositories/{id}) may be vulnerable to IDOR if proper authorization checks are missing. An attacker could potentially delete repositories they don't own by guessing or enumerating repository IDs. This requires further investigation of the actual implementation but is flagged due to the nature of cascading deletions which can expose data access patterns.
Suggested fix: Ensure that repository deletion endpoints perform proper ownership verification before allowing deletion. Implement RBAC checks to ensure users can only delete repositories they own or have explicit permission to delete.
Scanner: code-review/security | CWE: CWE-639
@@ -0,0 +12,4 @@assert_eq!(body["data"].as_array().unwrap().len(), 0);// Add a targetlet resp = server[medium] Inconsistent error handling with unwrap() in test code
The test code uses
.unwrap()extensively on deserialization and database operations, which can cause tests to panic instead of failing gracefully when unexpected data structures are returned. This makes test failures harder to debug and can mask underlying issues.Suggested fix: Replace unwrap() calls with proper error handling using match expressions or assert! macros that provide better failure messages.
*Scanner: code-review/convention | *
@@ -0,0 +14,4 @@// Get the DB name from the test server by parsing the health response// For now, we use a direct insert approachlet db = client.database(&server.db_name());[medium] Inconsistent error handling with unwrap() in test code
Similar to dast.rs, the findings.rs test file uses unwrap() on JSON parsing and database operations, which can lead to panics instead of controlled test failures when data doesn't match expectations.
Suggested fix: Replace unwrap() calls with proper error handling using match expressions or assert! macros that provide better failure messages.
*Scanner: code-review/convention | *
@@ -0,0 +19,4 @@let now = mongodb::bson::DateTime::now();db.collection::<mongodb::bson::Document>("findings").insert_one(mongodb::bson::doc! {"repo_id": repo_id,[medium] Missing error handling for MongoDB connection in findings test
The MongoDB client creation and database access in the insert_finding helper function lacks proper error handling. If the MongoDB connection fails or the database name cannot be determined, the test will panic rather than fail gracefully, potentially masking underlying infrastructure issues.
Suggested fix: Add proper error handling around the MongoDB client creation and database access operations using ? operator or explicit error checking to ensure tests fail gracefully when database connectivity issues occur.
*Scanner: code-review/logic | *
[medium] Complex boolean expression in test helper
The
insert_findingfunction contains a complex boolean expression involving multiple nested conditions and error handling that makes it difficult to follow the control flow and increases risk of logic errors during maintenance.Suggested fix: Extract the MongoDB connection setup and database selection into separate functions to simplify the main logic and improve readability.
*Scanner: code-review/complexity | *
@@ -0,0 +32,4 @@}).await.unwrap();}[high] Incorrect MongoDB collection name in findings test
The test code attempts to insert findings into a MongoDB collection named 'findings', but based on typical application patterns and the context of the code, it should likely be inserting into a collection named 'vulnerabilities' or similar. This could lead to test data not being properly inserted or retrieved, causing test failures.
Suggested fix: Verify the correct MongoDB collection name for findings in your application schema and update the collection reference accordingly. Check if the application uses 'findings', 'vulnerabilities', or another collection name.
*Scanner: code-review/logic | *
@@ -0,0 +67,4 @@assert_eq!(body["total"], 1);assert_eq!(body["data"][0]["title"], "SQL Injection");// Filter by repo[medium] Deeply nested control flow in test assertions
Multiple test functions contain deeply nested assertion chains that make it hard to track which assertions apply to which conditions, increasing the chance of incorrect test behavior when modifying or extending tests.
Suggested fix: Flatten the nested assertions by extracting them into separate assertion functions or using more structured test approaches like table-driven tests.
*Scanner: code-review/complexity | *
@@ -0,0 +82,4 @@insert_finding(&server, "repo1", "Test Bug", "medium").await;// Get the finding IDlet resp = server.get("/api/v1/findings").await;[medium] Potential race condition in bulk update test due to direct DB access
The bulk update test uses direct MongoDB access to insert findings before performing the API test. This creates a potential race condition where the API might not see the newly inserted data immediately, especially if there are caching layers or background processes involved. The test assumes immediate consistency between direct DB writes and API reads.
Suggested fix: Consider adding a small delay or retry mechanism after inserting findings via MongoDB before proceeding with API tests, or ensure that the API layer properly handles eventual consistency scenarios.
*Scanner: code-review/logic | *
@@ -0,0 +7,4 @@let resp = server.get("/api/v1/health").await;assert_eq!(resp.status(), 200);let body: serde_json::Value = resp.json().await.unwrap();[medium] Inconsistent error handling with unwrap() in test code
The health.rs test file also uses unwrap() on JSON parsing, which can cause tests to crash instead of failing gracefully when the API returns unexpected responses.
Suggested fix: Replace unwrap() calls with proper error handling using match expressions or assert! macros that provide better failure messages.
*Scanner: code-review/convention | *
@@ -0,0 +7,4 @@// Initially emptylet resp = server.get("/api/v1/repositories").await;assert_eq!(resp.status(), 200);[medium] Inconsistent error handling with unwrap() in test code
The test code uses
.unwrap()extensively on deserialization and database operations, which can cause tests to panic instead of properly reporting failures when the expected data structure doesn't match. This makes test failures harder to debug and can mask underlying issues.Suggested fix: Replace
.unwrap()calls with proper error handling usingassert!(result.is_ok())or similar assertions to make test failures more informative.*Scanner: code-review/convention | *
@@ -0,0 +14,4 @@// Add a repositorylet resp = server.post("/api/v1/repositories",[medium] Inconsistent error handling with unwrap() in test code
Multiple
.unwrap()calls in test code that could panic during test execution instead of providing clear failure information. This violates the principle of graceful error handling even in test code.Suggested fix: Use explicit error checking with assertions like
assert!(resp.json().await.is_ok())instead of unwrapping.*Scanner: code-review/convention | *
@@ -0,0 +42,4 @@let payload = json!({"name": "dup-repo","git_url": "https://github.com/example/dup-repo.git",[medium] Complex boolean expression in test assertion
The test function 'add_duplicate_repository_fails' uses assert_ne!(resp.status(), 200) which could pass for any non-200 status code including 5xx errors, potentially masking other issues. A more specific check would improve reliability.
Suggested fix: Replace assert_ne!(resp.status(), 200) with a more specific assertion like assert_eq!(resp.status(), 409) or assert!(resp.status() >= 400 && resp.status() < 500) to ensure we're specifically checking for conflict error codes.
*Scanner: code-review/complexity | *
@@ -0,0 +12,4 @@&json!({"name": "stats-repo","git_url": "https://github.com/example/stats-repo.git",}),[medium] Deeply nested control flow in test setup
The 'stats_overview_reflects_inserted_data' test contains multiple nested operations involving MongoDB client setup, database access, and document insertion that make the test flow harder to follow. The test mixes integration concerns with data manipulation logic.
Suggested fix: Extract the MongoDB setup and data insertion into separate helper functions to flatten the control flow and improve readability. Consider using a fixture pattern to manage test data setup.
*Scanner: code-review/complexity | *
@@ -0,0 +19,4 @@// Insert findings directlylet mongodb_uri = std::env::var("TEST_MONGODB_URI").unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into());let client = mongodb::Client::with_uri_str(&mongodb_uri).await.unwrap();[medium] Hardcoded MongoDB URI in test code
The test code hardcodes a MongoDB connection string with default credentials, which could expose sensitive information or cause issues if the test environment differs from expectations. This also makes tests less portable and harder to configure securely.
Suggested fix: Use environment variables consistently for all database connections in tests, and consider adding validation that the required environment variables are set before proceeding with the test.
*Scanner: code-review/convention | *
@@ -0,0 +22,4 @@let client = mongodb::Client::with_uri_str(&mongodb_uri).await.unwrap();let db = client.database(&server.db_name());let now = mongodb::bson::DateTime::now();[medium] Hardcoded MongoDB Credentials in Test Code
The test code contains hardcoded MongoDB credentials in the form of a connection string with username 'root' and password 'example'. While this is in test code, it represents a potential security risk if these credentials were to be accidentally committed to version control or used in a non-test environment.
Suggested fix: Use environment variables or a proper test configuration mechanism to manage database credentials instead of hardcoding them. Even in test environments, sensitive information should not be hardcoded.
Scanner: code-review/security | CWE: CWE-798
@@ -0,0 +32,4 @@db.collection::<mongodb::bson::Document>("findings").insert_one(mongodb::bson::doc! {"repo_id": "test-repo-id","fingerprint": format!("fp-{title}"),[high] Incorrect repository ID used in stats test
In the
stats_overview_reflects_inserted_datatest, the MongoDB insert operation uses hardcoded"test-repo-id"as therepo_idinstead of using the actual repository ID that was created. This means the findings won't be associated with any real repository, causing the stats to not reflect the expected data.Suggested fix: Use the actual repository ID from the previous POST request instead of hardcoding "test-repo-id". Store the repo_id from the first POST response and use it in the MongoDB insert.
*Scanner: code-review/logic | *
@@ -1,4 +1,9 @@// Integration tests for the compliance-agent crate.// E2E / Integration tests for the compliance-agent API.[medium] Inconsistent test module naming
The test file was renamed from 'integration' to 'e2e' but the module name still references 'integration'. This creates confusion about the test scope and may lead to incorrect test discovery or execution patterns.
Suggested fix: Rename the module to 'e2e' to match the file name and test runner command, or update the comment to reflect the actual module name being used.
*Scanner: code-review/convention | *
Compliance scan found 46 issue(s) in this PR:
find_parent_qnamewhen handling empty segments@@ -0,0 +14,4 @@concurrency:group: nightly-e2ecancel-in-progress: true[high] Hardcoded MongoDB Credentials in Workflow Configuration
The nightly CI workflow (.gitea/workflows/nightly.yml) contains hardcoded MongoDB credentials in the TEST_MONGODB_URI environment variable. These credentials are exposed in the workflow configuration and could be accessed by anyone with read access to the repository, potentially allowing unauthorized access to the test database.
Suggested fix: Use GitHub Actions secrets or a secure vault to store database credentials instead of hardcoding them in the workflow file. Reference the secret using ${{ secrets.MONGODB_PASSWORD }} in the URI.
Scanner: code-review/security | CWE: CWE-798
@@ -0,0 +20,4 @@e2e:name: E2E Testsruns-on: dockercontainer:[high] Incorrect Git Checkout in Nightly Workflow
The nightly workflow uses
git checkout FETCH_HEADwhich may not correctly handle shallow clones or specific commit references. WhenGITHUB_SHAis not set (e.g., during manual triggers), this could result in checking out an unexpected commit or failing to checkout properly.Suggested fix: Use
git checkout ${{ github.sha }}instead ofFETCH_HEADto ensure consistent checkout behavior regardless of how the workflow is triggered.*Scanner: code-review/logic | *
@@ -0,0 +42,4 @@| tar xz --strip-components=1 -C /usr/local/bin/ sccache-v0.9.1-x86_64-unknown-linux-musl/sccachechmod +x /usr/local/bin/sccacheenv:RUSTC_WRAPPER: ""[high] Missing Error Handling in E2E Test Step
The E2E test step does not explicitly check for failure of the
cargo testcommand. If the tests fail, the workflow will continue without indicating failure, potentially masking test failures.Suggested fix: Add explicit error handling or use
set -eto ensure the workflow fails when tests do not pass.*Scanner: code-review/logic | *
@@ -0,0 +45,4 @@RUSTC_WRAPPER: ""- name: Run E2E testsrun: cargo test -p compliance-agent --test e2e -- --test-threads=4[medium] Inconsistent error handling in E2E workflow
The nightly E2E workflow uses
cargo testwithout the--quietflag and doesn't capture or log test failures explicitly. If tests fail, the workflow may not properly report the failure to the CI system, potentially masking test failures.Suggested fix: Add explicit error handling and logging around the test execution step, ensuring that any test failures are properly reported by the workflow.
*Scanner: code-review/convention | *
@@ -0,0 +49,4 @@- name: Show sccache statsrun: sccache --show-statsif: always()[medium] Complex boolean expression in GitHub Actions workflow
The workflow uses an 'if: always()' condition on the final step which could mask failures in previous steps. This makes it harder to detect when tests actually fail since the step will always run regardless of previous step outcomes.
Suggested fix: Remove the 'if: always()' condition and rely on standard workflow failure handling instead. If you need to show stats regardless, consider using a separate job or conditional logic that explicitly checks for success/failure states.
*Scanner: code-review/complexity | *
@@ -1,6 +1,6 @@FROM rust:1.94-bookworm AS builderRUN cargo install dioxus-cli --version 0.7.3RUN cargo install dioxus-cli --version 0.7.4[medium] Incorrect Dependency Version Bump
The dioxus-cli version was bumped from 0.7.3 to 0.7.4 in the Dockerfile.dashboard. This change might introduce breaking changes or compatibility issues with existing code that depends on the previous version.
Suggested fix: Verify that all dependencies and integrations work correctly with dioxus-cli 0.7.4 before merging this change.
*Scanner: code-review/logic | *
@@ -43,2 +43,4 @@dashmap = { workspace = true }tokio-stream = { workspace = true }[dev-dependencies][medium] Unnecessary Dev Dependencies Added
Several dev-dependencies were added to Cargo.toml including compliance-core, reqwest, serde_json, tokio, mongodb, uuid, secrecy, axum, and tower-http. These are likely meant for testing but should be reviewed to ensure they're actually needed for development/testing purposes.
Suggested fix: Ensure these dependencies are only included in dev profiles or remove them if not necessary for the project's build process.
*Scanner: code-review/logic | *
@@ -0,0 +12,4 @@pub mod scheduler;pub mod ssh;#[allow(dead_code)]pub mod trackers;[low] Unnecessary allow(dead_code) in lib.rs
The
#[allow(dead_code)]attribute on the trackers module in lib.rs suggests that dead code is intentionally allowed. However, this can mask potential issues with unused code and makes it harder to maintain the codebase.Suggested fix: Remove the
#[allow(dead_code)]attribute and address any actual dead code issues, or provide a clear justification for why the code is intentionally unused.*Scanner: code-review/convention | *
@@ -12,3 +1,1 @@#[allow(dead_code)]mod trackers;mod webhooks;use compliance_agent::{agent, api, config, database, scheduler, ssh, webhooks};[medium] Potential Dead Code Removal
The removal of individual module declarations in main.rs and addition of a single use statement suggests refactoring. However, it's unclear whether all modules are properly exposed through the library interface. If any module is accessed directly elsewhere, this change could break compilation.
Suggested fix: Verify that all modules referenced in other parts of the codebase are still accessible via the new import structure.
*Scanner: code-review/logic | *
[medium] Missing error propagation in main function
The main function in main.rs returns
Result<(), Box<dyn std::error::Error>>but doesn't handle or propagate specific error types from the application components. This could lead to unhandled errors being silently ignored.Suggested fix: Ensure that errors from the application components are properly handled and propagated rather than being boxed into a generic error type.
*Scanner: code-review/convention | *
@@ -4,0 +10,4 @@use compliance_agent::database::Database;use compliance_core::AgentConfig;use secrecy::SecretString;[low] Hardcoded test database name prefix
The test database name uses a hardcoded prefix 'test_' which could collide with existing databases or make it harder to identify test-specific collections in MongoDB. Using a more distinctive prefix or namespace would improve test isolation and debugging.
Suggested fix: Consider using a more distinctive prefix like 'compliance_test_' or include a timestamp to reduce collision risks.
*Scanner: code-review/convention | *
@@ -4,0 +13,4 @@/// A running test server with a unique database.pub struct TestServer {pub base_url: String,[medium] Missing database cleanup on test server drop
The TestServer struct does not implement Drop, which means the cleanup() method is never automatically called when a test server goes out of scope. This can lead to lingering test databases in MongoDB, especially if tests panic or are interrupted before cleanup completes.
Suggested fix: Implement the Drop trait for TestServer to ensure cleanup() is always called. Add
impl Drop for TestServer { fn drop(&mut self) { tokio::runtime::Handle::current().block_on(self.cleanup()); } }*Scanner: code-review/logic | *
@@ -4,0 +15,4 @@pub struct TestServer {pub base_url: String,pub client: reqwest::Client,db_name: String,[medium] Inconsistent error handling with unwrap() in test setup
The test helper uses
unwrap_or_else()withexpect()inside thestart()method, which can mask underlying configuration issues. Theunwrap()calls on environment variables and database connections should be replaced with proper error propagation or more graceful fallback mechanisms to ensure tests don't silently fail due to misconfiguration.Suggested fix: Replace
unwrap_or_else(|_| ...)with explicit error handling that propagates failures or uses a more robust fallback strategy instead of panicking.*Scanner: code-review/convention | *
@@ -4,0 +31,4 @@let db = Database::connect(&mongodb_uri, &db_name).await.expect("Failed to connect to MongoDB — is it running?");db.ensure_indexes().await.expect("Failed to create indexes");[medium] Complex boolean expression in test server startup
The TestServer::start() function contains a complex boolean expression in the AgentConfig construction that mixes environment variable fallbacks with hardcoded defaults, making it difficult to trace which values are being used under what conditions.
Suggested fix: Extract the environment variable loading into separate variables with clear names, and use a more structured approach like a builder pattern or helper function to reduce the cognitive load when reading the config initialization.
*Scanner: code-review/complexity | *
@@ -4,0 +75,4 @@let app = api::routes::build_router().layer(axum::extract::Extension(Arc::new(agent))).layer(tower_http::cors::CorsLayer::permissive());[medium] Deeply nested control flow in server readiness check
The server readiness check loop in TestServer::start() uses a nested structure with a for loop containing a conditional that breaks early, which increases cognitive complexity and makes it harder to follow the control flow.
Suggested fix: Flatten the control flow by extracting the health check logic into a separate function and using early returns instead of nesting, or restructure to avoid the explicit loop with break condition.
*Scanner: code-review/complexity | *
@@ -4,0 +151,4 @@.expect("DELETE request failed")}/// Get the unique database name for direct MongoDB access in tests.[medium] Test server cleanup may silently ignore database drop errors
The
cleanup()method uses.ok()when dropping the database, which suppresses any potential errors during cleanup. This can hide issues like connection problems or permission errors that might affect subsequent tests or leave stale data in the test database.Suggested fix: Log or propagate errors from the database drop operation instead of silently ignoring them to ensure cleanup failures are detected.
*Scanner: code-review/convention | *
@@ -0,0 +7,4 @@.unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into());let client = mongodb::Client::with_uri_str(&mongodb_uri).await.unwrap();let db = client.database(&server.db_name());[low] Hardcoded MongoDB URI in test helpers
Multiple test helper functions repeatedly fetch the MongoDB URI from environment variables with a hardcoded default value. This creates redundancy and makes it harder to maintain consistent database connections across test helpers. The repeated code also increases the chance of inconsistencies if the default URI needs to change.
Suggested fix: Extract the MongoDB connection setup into a shared helper function that handles both environment variable retrieval and connection creation to reduce duplication.
*Scanner: code-review/convention | *
@@ -0,0 +9,4 @@let db = client.database(&server.db_name());let result = db.collection::<mongodb::bson::Document>("dast_targets")[medium] Hardcoded MongoDB Credentials in Test Code
The test code contains hardcoded MongoDB credentials ('root:example') in multiple helper functions. While this is only in test files, it represents poor security hygiene and could lead to accidental exposure if the test code were ever committed to a public repository or if the credentials were reused in production code.
Suggested fix: Use environment variables or a secure configuration management system for database credentials even in test environments. Consider using a dedicated test database with unique credentials.
Scanner: code-review/security | CWE: CWE-798
@@ -0,0 +11,4 @@let result = db.collection::<mongodb::bson::Document>("dast_targets").insert_one(mongodb::bson::doc! {"name": name,[medium] Database connection reuse in test helpers
Each helper function creates its own MongoDB client connection instead of reusing the connection from the test server. This leads to unnecessary resource consumption and potential connection pool exhaustion during test execution.
Suggested fix: Modify the test helpers to accept a shared database connection or use a connection pool managed by the TestServer to avoid creating multiple connections per test helper call.
*Scanner: code-review/logic | *
[medium] Duplicated MongoDB connection setup
Multiple helper functions repeatedly establish MongoDB connections using the same pattern, creating redundant code and potential inconsistency if connection parameters change. This increases maintenance burden and risk of bugs due to duplicated logic.
Suggested fix: Extract MongoDB connection setup into a shared helper function or use a test fixture pattern to avoid repeated connection logic across multiple functions.
*Scanner: code-review/complexity | *
@@ -0,0 +12,4 @@.collection::<mongodb::bson::Document>("dast_targets").insert_one(mongodb::bson::doc! {"name": name,"base_url": format!("https://{name}.example.com"),[medium] Inconsistent error handling with unwrap() in test code
The test code uses
unwrap()extensively when interacting with MongoDB connections and database operations. This violates the consistent error handling pattern established in the rest of the codebase where errors should be properly handled or propagated instead of panicking. The use ofunwrap()in tests can mask underlying connection or database issues and make debugging harder.Suggested fix: Replace
unwrap()calls with proper error handling using?operator or explicit error assertions to maintain consistency with the project's error handling patterns.*Scanner: code-review/convention | *
[high] Missing error handling in database operations
The test functions use
.unwrap()on database operations which will panic if the database operations fail. This could cause tests to crash instead of properly reporting test failures when database connectivity issues occur.Suggested fix: Replace
.unwrap()with proper error handling using?operator or explicit error checking to ensure tests fail gracefully when database operations don't succeed.*Scanner: code-review/logic | *
@@ -0,0 +147,4 @@// All downstream data should be goneassert_eq!(count_docs(&server, "dast_targets").await, 0);assert_eq!(count_docs(&server, "pentest_sessions").await, 0);[high] Insecure Direct Object Reference (IDOR) Potential in Repository Deletion
The test verifies cascade deletion functionality but doesn't validate authorization controls. If the actual implementation lacks proper access control checks, an attacker could potentially delete repositories they don't own by crafting malicious requests with arbitrary repo IDs. This would allow unauthorized data destruction.
Suggested fix: Ensure that repository deletion endpoints verify the requesting user's ownership or authorization before performing any cascade operations. Add explicit authorization checks in the delete endpoint handler.
Scanner: code-review/security | CWE: CWE-285
@@ -0,0 +12,4 @@// We'll use the agent's internal DB through the stats endpoint to verifylet client = mongodb::Client::with_uri_str(&mongodb_uri).await.unwrap();// Get the DB name from the test server by parsing the health response[medium] Hardcoded MongoDB URI in test helper
The insert_finding helper function contains a hardcoded MongoDB URI fallback that bypasses the normal test setup process. This creates an implicit dependency on environment variables and makes the test less portable and harder to maintain.
Suggested fix: Use a consistent test database setup mechanism that provides the MongoDB connection string through the TestServer rather than hardcoding it.
*Scanner: code-review/convention | *
[medium] Deeply nested control flow in test helper
The
insert_findinghelper function contains multiple nested operations including environment variable access, MongoDB client initialization, and document insertion that are tightly coupled. This makes it difficult to reason about failure modes and increases risk of bugs when modifying any part of the chain.Suggested fix: Break down the helper into smaller, focused functions that handle one responsibility each (e.g., get DB name, connect to MongoDB, insert document) to improve maintainability and reduce coupling.
*Scanner: code-review/complexity | *
@@ -0,0 +14,4 @@// Get the DB name from the test server by parsing the health response// For now, we use a direct insert approachlet db = client.database(&server.db_name());[medium] Hardcoded MongoDB URI in test environment
The MongoDB URI is hardcoded in the test file with a default value that may not match the actual test environment configuration. This can cause tests to fail when run in different environments or when the default credentials are changed.
Suggested fix: Use environment variables consistently for all database connection details and provide clear documentation about required environment variables for running integration tests.
*Scanner: code-review/logic | *
@@ -0,0 +20,4 @@db.collection::<mongodb::bson::Document>("findings").insert_one(mongodb::bson::doc! {"repo_id": repo_id,"fingerprint": format!("fp-{title}-{severity}"),[medium] Missing error handling for MongoDB connection in findings test
The MongoDB client creation and database access in the insert_finding helper function lacks proper error handling. If the MongoDB connection fails or the database name cannot be determined, the test will panic rather than fail gracefully, potentially masking underlying infrastructure issues.
Suggested fix: Add proper error handling around the MongoDB client initialization and database access operations using ? operator or explicit error checking to ensure tests fail gracefully when database connectivity issues occur.
*Scanner: code-review/logic | *
@@ -0,0 +22,4 @@"repo_id": repo_id,"fingerprint": format!("fp-{title}-{severity}"),"scanner": "test-scanner","scan_type": "sast",[medium] Inconsistent error handling with unwrap() in test code
The test code uses
.unwrap()extensively on deserialization and database operations, which can cause tests to panic instead of failing gracefully when the test environment is misconfigured or when the API returns unexpected responses. This makes test failures harder to debug and can mask underlying issues.Suggested fix: Replace unwrap() calls with proper error handling using ? operator or explicit assertions to make test failures more informative and prevent panics.
*Scanner: code-review/convention | *
@@ -0,0 +27,4 @@"description": format!("Test finding: {title}"),"severity": severity,"status": "open","created_at": now,[medium] Direct MongoDB manipulation in integration tests
Integration tests directly manipulate MongoDB collections instead of using the public API, which violates the principle of testing through the public interface. This makes tests brittle and doesn't verify that the actual API endpoints work correctly.
Suggested fix: Refactor tests to use the public API for creating test data instead of direct database manipulation, ensuring tests validate the actual API behavior.
*Scanner: code-review/convention | *
@@ -0,0 +32,4 @@}).await.unwrap();}[high] Incorrect MongoDB collection name in findings test
The test code attempts to insert findings into a MongoDB collection named 'findings', but based on the context and typical application structure, it should likely be inserting into a collection like 'vulnerabilities' or similar. This could lead to test data not being properly stored or retrieved during integration tests.
Suggested fix: Verify the correct MongoDB collection name for findings in your application schema and update the collection reference accordingly. Check if the actual application stores findings in a different collection than 'findings'.
*Scanner: code-review/logic | *
@@ -0,0 +102,4 @@assert_eq!(body["data"]["status"], "resolved");server.cleanup().await;}[medium] Complex boolean expression in test assertions
The test functions use deeply nested JSON path access patterns like
body["data"][0]["name"]which are hard to read and prone to runtime errors if the structure changes. The complex boolean logic in assertions increases risk of false positives/negatives during maintenance.Suggested fix: Extract JSON path access into helper functions with proper error handling to make assertions more readable and less error-prone.
*Scanner: code-review/complexity | *
@@ -0,0 +7,4 @@// Initially emptylet resp = server.get("/api/v1/repositories").await;assert_eq!(resp.status(), 200);[medium] Inconsistent error handling with unwrap() in test code
The test code uses
.unwrap()extensively on deserialization and database operations, which can cause tests to panic instead of properly reporting test failures when these operations fail. This makes it harder to debug test failures and can mask underlying issues.Suggested fix: Replace
.unwrap()calls with proper error handling usingassert!ormatches!macros to ensure tests fail gracefully with meaningful error messages rather than panicking.*Scanner: code-review/convention | *
@@ -0,0 +41,4 @@let server = TestServer::start().await;let payload = json!({"name": "dup-repo",[medium] Complex boolean expression in test assertion
The test function 'add_duplicate_repository_fails' uses assert_ne!(resp.status(), 200) which could pass for any non-200 status code, including 500 errors or other unexpected failures. This makes the test less precise and potentially masking real issues.
Suggested fix: Replace assert_ne!(resp.status(), 200) with a more specific assertion like assert_eq!(resp.status(), 409) or assert!(resp.status() >= 400 && resp.status() < 500) to ensure we're specifically testing for the duplicate repository error case.
*Scanner: code-review/complexity | *
@@ -0,0 +12,4 @@&json!({"name": "stats-repo","git_url": "https://github.com/example/stats-repo.git",}),[medium] Deeply nested control flow in test setup
The 'stats_overview_reflects_inserted_data' test contains multiple nested operations involving MongoDB client setup, database access, and document insertion that make the test flow harder to follow. The test mixes integration test logic with direct database manipulation which increases complexity.
Suggested fix: Extract the MongoDB setup and document insertion into separate helper functions to flatten the control flow and improve readability. Consider using a fixture pattern to manage the test database state.
*Scanner: code-review/complexity | *
[medium] Hardcoded MongoDB Credentials in Test Code
The test code contains hardcoded MongoDB credentials in the form of a connection string with username 'root' and password 'example'. While this is in test files, it represents a potential security risk if these credentials were accidentally committed to version control or used in production environments.
Suggested fix: Use environment variables or a secure configuration management system for database credentials even in test environments. Consider using a dedicated test database with unique credentials.
Scanner: code-review/security | CWE: CWE-798
@@ -0,0 +17,4 @@.await;// Insert findings directlylet mongodb_uri = std::env::var("TEST_MONGODB_URI")[medium] Hardcoded MongoDB URI in test code
The test code hardcodes the MongoDB connection string with default values, making tests brittle and dependent on specific environment configurations. This violates test isolation principles and makes tests less portable.
Suggested fix: Use environment variables consistently for all database connections in tests, and provide better error handling when environment variables are missing rather than falling back to hardcoded defaults.
*Scanner: code-review/convention | *
@@ -0,0 +22,4 @@let client = mongodb::Client::with_uri_str(&mongodb_uri).await.unwrap();let db = client.database(&server.db_name());let now = mongodb::bson::DateTime::now();[medium] Inconsistent error handling with unwrap() in test code
Multiple test files use
.unwrap()on database operations and JSON parsing, which can cause tests to crash instead of failing gracefully when operations don't succeed. This violates the principle of robust test design where failures should be reported clearly.Suggested fix: Replace
.unwrap()calls with explicit error checking that converts failures into assertion failures rather than panicking.*Scanner: code-review/convention | *
@@ -0,0 +32,4 @@db.collection::<mongodb::bson::Document>("findings").insert_one(mongodb::bson::doc! {"repo_id": "test-repo-id","fingerprint": format!("fp-{title}"),[high] Incorrect repository ID used in stats test
In the
stats_overview_reflects_inserted_datatest, the MongoDB insert operation uses hardcoded"test-repo-id"as therepo_idinstead of using the actual repository ID that was created in the test. This means the findings won't be associated with any real repository, causing the stats to not reflect the expected data.Suggested fix: Store the repository ID from the successful POST request and use it in the MongoDB insert operation instead of the hardcoded value.
*Scanner: code-review/logic | *
@@ -1,4 +1,9 @@// Integration tests for the compliance-agent crate.// E2E / Integration tests for the compliance-agent API.[medium] Inconsistent test module naming
The test file was renamed from 'integration' to 'e2e' but the module name still references 'integration'. This creates confusion about the test scope and may lead to incorrect test discovery or execution patterns.
Suggested fix: Rename the module to 'e2e' to match the file name and test runner command, or update the comment to reflect the actual module name being used.
*Scanner: code-review/convention | *
@@ -18,0 +24,4 @@////// Returns the first match found, or `None` if the node is a root.fn find_parent_qname(qname: &str, node_map: &HashMap<String, NodeIndex>) -> Option<String> {let mut current = qname.to_string();[high] Off-by-one error in
find_parent_qnamewhen handling empty segmentsThe
find_parent_qnamefunction can produce incorrect results when dealing with qualified names that end with '::'. For example, given a node map containing "src/main.rs::config::" (with trailing '::'), callingfind_parent_qname("src/main.rs::config::", &node_map)will incorrectly returnSome("src/main.rs::")instead ofNone. This happens becauserfind("::")finds the last '::' but truncating it leaves an empty string which may match a node if one exists with an empty string key.Suggested fix: Add a check after
current.truncate(pos)to ensure thatcurrentis not empty before checkingnode_map.contains_key(¤t). Ifcurrentbecomes empty, immediately returnNonesince there's no valid parent.*Scanner: code-review/logic | *
@@ -121,7 +148,48 @@ impl GraphEngine {graph.add_edge(src, tgt, edge.kind.clone());[medium] Deeply nested control flow in Contains edge synthesis
The Contains edge synthesis logic in
build_petgraphhas deeply nested control flow with multiple nested conditions and loops. The code walks up the qualified name hierarchy while checking for existing edges, which creates a complex flow that's hard to reason about and maintain.Suggested fix: Flatten the nested structure by extracting the parent lookup logic into a separate function and using early returns instead of deep nesting. This would simplify the main control flow and make it easier to verify correctness.
*Scanner: code-review/complexity | *
@@ -125,0 +163,4 @@.first().map(|n| n.graph_build_id.as_str()).unwrap_or("");[medium] Potential panic in Contains edge synthesis
In the
build_petgraphfunction, when synthesizing Contains edges, the code directly indexes intonode_mapwithnode_map[qname]andnode_map[&parent_qname]without checking if keys exist. Iffind_parent_qnamereturns a parent that doesn't exist in the node_map, this will panic during runtime.Suggested fix: Use
.get()instead of indexing with[]to safely access values from node_map, or ensure thatfind_parent_qnamealways returns valid keys that exist in the node_map.*Scanner: code-review/convention | *
@@ -125,0 +173,4 @@for qname in &qualified_names {if let Some(parent_qname) = find_parent_qname(qname, &node_map) {let child_idx = node_map[qname];let parent_idx = node_map[&parent_qname];[medium] Incorrect edge duplication prevention in Contains edge synthesis
In the Contains edge synthesis logic, the check
!graph.contains_edge(parent_idx, child_idx)only prevents adding duplicate edges based on the indices. However, it doesn't prevent adding multiple edges with different metadata (like file paths) between the same pair of nodes. This could lead to inconsistent graph state where the same logical relationship appears multiple times with different metadata.Suggested fix: Consider using a more robust deduplication mechanism that also compares edge metadata, or ensure that all synthesized edges have consistent metadata across the graph.
*Scanner: code-review/logic | *
@@ -125,0 +174,4 @@if let Some(parent_qname) = find_parent_qname(qname, &node_map) {let child_idx = node_map[qname];let parent_idx = node_map[&parent_qname];[low] Unnecessary unwrap_or_default in Contains edge creation
In the Contains edge synthesis logic,
file_paths.get(qname).cloned().unwrap_or_default()can silently provide empty strings when file paths aren't found. This might mask missing data or incorrect assumptions about node structure, especially since the code already checks for existence of nodes vianode_map.Suggested fix: Consider adding a debug assertion or logging when a file path is missing, rather than silently defaulting to an empty string.
*Scanner: code-review/convention | *
@@ -132,33 +200,62 @@ impl GraphEngine {})[medium] Complex boolean expression in edge resolution
The
resolve_edge_targetfunction contains a complex boolean expression that checks multiple conditions for edge resolution. The logic involves multiple string pattern matching operations with various prefixes and suffixes, making it difficult to follow and prone to subtle bugs when modifying the resolution strategies.Suggested fix: Break down the complex conditionals into separate helper functions with clear names that describe their specific resolution strategy. This would make the intent clearer and reduce the chance of logical errors when adding new resolution types.
*Scanner: code-review/complexity | *
@@ -150,3 +224,2 @@|| qualified.ends_with(&format!(".{target}")){if qualified.ends_with(&suffix_pattern) || qualified.ends_with(&dot_pattern) {return Some(*idx);[medium] Inconsistent error handling in edge resolution
The
resolve_edge_targetfunction uses multiple resolution strategies but doesn't consistently handle cases where resolution fails. Specifically, the module-path matching strategy has a fallback that usesunwrap_or(target)which could lead to silent failures or incorrect behavior when the target string contains '::' but doesn't match any pattern.Suggested fix: Replace
unwrap_or(target)with explicit error handling or logging to make it clear when resolution fails. Consider returning an error or using a more robust fallback mechanism.*Scanner: code-review/convention | *
Compliance scan found 42 issue(s) in this PR:
find_parent_qnamewhen handling qualified names without '::'@@ -71,3 +71,3 @@# Tests (reuses compilation artifacts from clippy)- name: Tests (core + agent)run: cargo test -p compliance-core -p compliance-agentrun: cargo test -p compliance-core -p compliance-agent --lib[medium] Inconsistent test command in CI workflow
The test command for core and agent packages was changed from
cargo test -p compliance-core -p compliance-agenttocargo test -p compliance-core -p compliance-agent --lib. This change may exclude integration tests from being run, potentially hiding failures in the agent's integration behavior.Suggested fix: Revert to the previous command or explicitly include integration tests in the test suite.
*Scanner: code-review/convention | *
[low] Complex boolean expression in CI workflow
The test command in the CI workflow uses a complex boolean expression with multiple package specifications and feature flags that could be simplified for better readability and maintainability.
Suggested fix: Consider breaking down the cargo test command into separate steps or using a more structured approach to specify test targets and features.
*Scanner: code-review/complexity | *
@@ -0,0 +22,4 @@runs-on: dockercontainer:image: rust:1.94-bookwormservices:[medium] Deeply nested control flow in E2E workflow
The E2E workflow contains nested control structures with multiple conditional steps that make the workflow logic harder to follow and increase the risk of errors when modifying individual steps.
Suggested fix: Flatten the nested control flow by extracting conditional logic into separate jobs or using workflow conditions to simplify the overall structure.
*Scanner: code-review/complexity | *
@@ -43,2 +43,4 @@dashmap = { workspace = true }tokio-stream = { workspace = true }[dev-dependencies][high] Missing dev-dependencies in compliance-agent Cargo.toml
The compliance-agent/Cargo.toml file adds dev-dependencies but does not include all necessary dependencies for the E2E tests to run properly. Specifically, it's missing the 'tokio' dependency which is required for async runtime in tests.
Suggested fix: Add 'tokio = { workspace = true }' to the dev-dependencies section to ensure proper async runtime support during E2E testing.
*Scanner: code-review/logic | *
@@ -0,0 +12,4 @@pub mod scheduler;pub mod ssh;#[allow(dead_code)]pub mod trackers;[low] Unnecessary allow(dead_code) for public module
The
trackersmodule is marked with#[allow(dead_code)]but it's publicly exported via the lib.rs re-export. This suppresses dead code warnings for a public interface, which could hide unused code that should be removed or properly used.Suggested fix: Either remove the
#[allow(dead_code)]attribute if the module is actually used, or ensure the module is properly integrated into the public API.*Scanner: code-review/convention | *
@@ -12,3 +1,1 @@#[allow(dead_code)]mod trackers;mod webhooks;use compliance_agent::{agent, api, config, database, scheduler, ssh, webhooks};[high] Incorrect module imports in main.rs
In compliance-agent/src/main.rs, the use statements import modules from the compliance_agent crate, but the actual modules being imported are not defined in the lib.rs file. This will lead to compilation errors because the modules like 'agent', 'api', etc., are not properly exposed through the library interface.
Suggested fix: Ensure that all modules referenced in the use statement are properly exported in lib.rs or adjust the import paths to match the actual module structure.
*Scanner: code-review/logic | *
[medium] Missing explicit error handling in main function
The main function in main.rs uses
Box<dyn std::error::Error>which can mask specific error types and make debugging harder. The code should handle specific error cases more explicitly rather than relying on dynamic dispatch.Suggested fix: Replace
Box<dyn std::error::Error>with explicit error handling for known error types, or use a more specific error type that captures the expected failure modes.*Scanner: code-review/convention | *
@@ -4,0 +13,4 @@/// A running test server with a unique database.pub struct TestServer {pub base_url: String,[medium] Missing database cleanup on test server drop
The TestServer struct does not implement Drop, which means the cleanup() method is never called automatically when a test server goes out of scope. This can lead to lingering test databases in MongoDB, causing resource leaks and potential test interference.
Suggested fix: Implement the Drop trait for TestServer to ensure cleanup() is called when the server instance is dropped. Add
impl Drop for TestServer { fn drop(&mut self) { tokio::runtime::Handle::current().block_on(self.cleanup()); } }*Scanner: code-review/logic | *
@@ -4,0 +16,4 @@pub base_url: String,pub client: reqwest::Client,db_name: String,mongodb_uri: String,[low] Hardcoded test database name prefix
The test database name is constructed with a hardcoded prefix 'test_' (line 19). While this helps identify test databases, it could potentially conflict with existing databases named similarly. Additionally, the UUID generation approach may not be suitable for all environments where uniqueness is critical.
Suggested fix: Consider making the database name prefix configurable via an environment variable or using a more robust naming scheme that ensures uniqueness across different test runs and environments.
*Scanner: code-review/convention | *
@@ -4,0 +18,4 @@db_name: String,mongodb_uri: String,}[medium] Inconsistent error handling with unwrap() in test code
The test code uses
unwrap_or_else()followed byexpect()in multiple places, which creates inconsistent error handling patterns. Specifically, line 21 usesunwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into())followed byexpect("Failed to connect to MongoDB — is it running?")on line 28. This pattern should be consistent - either useexpect()directly on the result or handle errors more gracefully.Suggested fix: Use consistent error handling by either removing the
unwrap_or_elseand usingexpect()directly on the environment variable read, or refactor to use proper error propagation with?operator.*Scanner: code-review/convention | *
@@ -4,0 +32,4 @@.await.expect("Failed to connect to MongoDB — is it running?");db.ensure_indexes().await.expect("Failed to create indexes");[medium] Complex boolean expression in test server startup
The TestServer::start() function contains a complex boolean expression in the AgentConfig construction that mixes environment variable fallbacks with hardcoded defaults, making it difficult to track which values are being used under what conditions.
Suggested fix: Extract the environment variable loading into separate variables with clear names, and use a more structured approach like a builder pattern or helper functions to reduce the cognitive load when reading the config initialization.
*Scanner: code-review/complexity | *
@@ -4,0 +81,4 @@.await.expect("Failed to bind test server");let port = listener.local_addr().expect("no local addr").port();[medium] Deeply nested control flow in server readiness check
The server readiness check loop in TestServer::start() uses a nested structure with a for loop containing a conditional check and sleep, which could be flattened to improve readability and reduce potential logic errors during maintenance.
Suggested fix: Replace the for loop with a while let or tokio::time::sleep_until pattern to flatten the control flow and make the retry logic clearer.
*Scanner: code-review/complexity | *
@@ -4,0 +151,4 @@.expect("DELETE request failed")}/// Get the unique database name for direct MongoDB access in tests.[medium] Potential panic in test cleanup when MongoDB connection fails
In the
cleanupmethod (lines 154-157), ifmongodb::Client::with_uri_str()fails, the test will panic due to.ok()being called on aResult. Theok()method convertsResult<T, E>toOption<T>, but since we're callingok()on the result ofdrop(), any error during drop will be silently ignored. This can mask actual database connection issues during cleanup.Suggested fix: Remove the
.ok()call and allow potential errors to propagate, or explicitly log the error before ignoring it to maintain visibility into cleanup failures.*Scanner: code-review/convention | *
@@ -0,0 +7,4 @@.unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into());let client = mongodb::Client::with_uri_str(&mongodb_uri).await.unwrap();let db = client.database(&server.db_name());[medium] Duplicated MongoDB connection setup across multiple test helper functions
Multiple helper functions (
insert_dast_target,insert_pentest_session,insert_attack_node,insert_dast_finding,count_docs) all duplicate the MongoDB client initialization logic. This creates redundancy and potential inconsistencies if the connection parameters change. The connection setup should be centralized to avoid duplication and improve maintainability.Suggested fix: Create a shared helper function for MongoDB connection setup that can be reused across all test helper functions to reduce duplication and ensure consistent connection handling.
*Scanner: code-review/convention | *
@@ -0,0 +9,4 @@let db = client.database(&server.db_name());let result = db.collection::<mongodb::bson::Document>("dast_targets")[medium] Hardcoded MongoDB Credentials in Test Code
The test code contains hardcoded MongoDB credentials ('root:example') in multiple helper functions. While this is only in test files, it represents poor security hygiene and could lead to unauthorized database access if the test environment is compromised or if these credentials are accidentally used in production code paths.
Suggested fix: Use environment variables or a secure configuration management system for database credentials even in test environments. Consider using a dedicated test database with restricted permissions.
Scanner: code-review/security | CWE: CWE-798
@@ -0,0 +11,4 @@let result = db.collection::<mongodb::bson::Document>("dast_targets").insert_one(mongodb::bson::doc! {"name": name,[medium] Database connection reused across tests
Each helper function creates its own MongoDB client connection instead of reusing the one from the test server. This leads to unnecessary resource consumption and potential connection pool exhaustion during test runs.
Suggested fix: Pass a shared database connection or client instance to the helper functions rather than creating new connections in each function.
*Scanner: code-review/logic | *
[medium] Duplicated MongoDB connection setup
Multiple helper functions repeatedly establish MongoDB connections using the same URI retrieval and client creation pattern. This creates redundant code that's hard to maintain and increases risk of inconsistencies if the connection logic needs to change.
Suggested fix: Extract MongoDB connection setup into a shared helper function to reduce duplication and improve maintainability.
*Scanner: code-review/complexity | *
@@ -0,0 +12,4 @@.collection::<mongodb::bson::Document>("dast_targets").insert_one(mongodb::bson::doc! {"name": name,"base_url": format!("https://{name}.example.com"),[high] Missing error handling for database operations
The test helper functions use
.unwrap()on database operations which will panic if the database connection fails or operations fail. This could cause tests to crash instead of failing gracefully with a proper assertion.Suggested fix: Replace
.unwrap()calls with proper error handling using?operator or explicit error assertions to make tests more robust.*Scanner: code-review/logic | *
[medium] Inconsistent error handling with unwrap() in test code
The test code uses
unwrap()extensively when interacting with MongoDB connections and database operations. This violates the project's convention of proper error handling, which could hide failures during testing and make debugging difficult. The code should handle potential errors more gracefully instead of panicking.Suggested fix: Replace
unwrap()calls with proper error handling using?operator or explicit error checking to maintain consistency with the project's error handling patterns.*Scanner: code-review/convention | *
@@ -0,0 +147,4 @@// All downstream data should be goneassert_eq!(count_docs(&server, "dast_targets").await, 0);assert_eq!(count_docs(&server, "pentest_sessions").await, 0);[high] Potential Insecure Direct Object Reference (IDOR) in Repository Deletion
The repository deletion endpoint (/api/v1/repositories/{repo_id}) may be vulnerable to IDOR if proper authorization checks are missing. An attacker could potentially delete repositories they don't own by guessing or enumerating valid repository IDs, especially if there's no ownership verification before deletion.
Suggested fix: Implement proper authentication and authorization checks before allowing repository deletion. Ensure that users can only delete repositories they own or have explicit permission to delete.
Scanner: code-review/security | CWE: CWE-285
@@ -0,0 +12,4 @@assert_eq!(body["data"].as_array().unwrap().len(), 0);// Add a targetlet resp = server[medium] Inconsistent error handling with unwrap() in test code
The test code uses
.unwrap()extensively on deserialization and JSON parsing operations. While this is acceptable in test code, it creates inconsistency with the production code's error handling patterns which likely use proper error propagation. This could mask potential issues during testing if the JSON structure changes unexpectedly.Suggested fix: Consider using more robust error handling patterns like
expect()with descriptive messages or proper error propagation to maintain consistency with how errors should be handled in production code.*Scanner: code-review/convention | *
@@ -0,0 +21,4 @@"target_type": "webapp",}),).await;[medium] Complex boolean expression in test assertions
The test functions use deeply nested JSON path access patterns like
body["data"][0]["name"]which are hard to read and prone to runtime errors if the structure changes. This makes the tests brittle and harder to maintain.Suggested fix: Extract JSON path access into helper functions or use more structured deserialization to make assertions clearer and safer.
*Scanner: code-review/complexity | *
@@ -0,0 +16,4 @@// For now, we use a direct insert approachlet db = client.database(&server.db_name());let now = mongodb::bson::DateTime::now();[medium] Missing error handling in MongoDB connection setup
In the
insert_findingfunction, the MongoDB client creation and database access are wrapped in.unwrap()calls which will panic if the connection fails. This could cause tests to crash instead of failing gracefully when the test environment isn't properly configured.Suggested fix: Replace
.unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into())with proper error handling using?operator or logging and returning an error.*Scanner: code-review/logic | *
@@ -0,0 +19,4 @@let now = mongodb::bson::DateTime::now();db.collection::<mongodb::bson::Document>("findings").insert_one(mongodb::bson::doc! {"repo_id": repo_id,[medium] Inconsistent error handling with unwrap() in test code
Similar to dast.rs, this test file also uses
.unwrap()on JSON parsing and MongoDB operations. The MongoDB connection setup and document insertion both use unwrap(), which could hide issues in test environments where the database might not be properly configured.Suggested fix: Replace unwraps with proper error handling or use expect() with descriptive messages to improve test reliability and consistency with production error handling patterns.
*Scanner: code-review/convention | *
@@ -0,0 +22,4 @@"repo_id": repo_id,"fingerprint": format!("fp-{title}-{severity}"),"scanner": "test-scanner","scan_type": "sast",[high] Incorrect MongoDB document insertion in test helper
The
insert_findinghelper function attempts to insert a document into the 'findings' collection but usesmongodb::bson::Documenttype incorrectly. It should usemongodb::bson::doc!macro directly instead of constructing a Document and then inserting it. This will likely cause a compilation error or runtime panic due to incorrect BSON structure.Suggested fix: Change line 25 from
let db = client.database(&server.db_name());tolet db = client.database(&server.db_name());and remove the redundantmongodb::bson::Documentconstruction. The correct approach is to use themongodb::bson::doc!macro directly in the insert_one call.*Scanner: code-review/logic | *
@@ -0,0 +91,4 @@.patch(&format!("/api/v1/findings/{finding_id}/status"),&json!({ "status": "resolved" }),)[medium] Complex boolean expression in test assertions
Similar to dast.rs, the findings.rs tests use complex nested JSON access patterns like
body["data"][0]["_id"]["$oid"]which increase cognitive load and risk of bugs when the API response structure changes.Suggested fix: Use structured deserialization or helper methods to simplify JSON path access and improve test robustness.
*Scanner: code-review/complexity | *
@@ -0,0 +115,4 @@let resp = server.get("/api/v1/findings").await;let body: serde_json::Value = resp.json().await.unwrap();let ids: Vec<String> = body["data"].as_array()[medium] Potential race condition in bulk update test
In the
bulk_update_finding_statustest, the order of operations between getting finding IDs and updating them may lead to inconsistent state if other processes modify findings during this window. While this is a test scenario, it could mask real concurrency issues in production code.Suggested fix: Consider adding explicit synchronization or ensuring test isolation to prevent race conditions in multi-threaded test execution.
*Scanner: code-review/logic | *
@@ -0,0 +7,4 @@let resp = server.get("/api/v1/health").await;assert_eq!(resp.status(), 200);let body: serde_json::Value = resp.json().await.unwrap();[medium] Inconsistent error handling with unwrap() in test code
The health.rs test file also contains multiple uses of
.unwrap()on JSON parsing operations. While these tests are simple, the pattern of using unwrap() instead of more explicit error handling creates inconsistency with how the application should handle potential JSON parsing failures in production.Suggested fix: Use more explicit error handling patterns such as
expect()or proper error propagation to maintain consistency with production code error handling practices.*Scanner: code-review/convention | *
@@ -0,0 +7,4 @@// Initially emptylet resp = server.get("/api/v1/repositories").await;assert_eq!(resp.status(), 200);[medium] Inconsistent error handling with unwrap() in test code
The test code uses
.unwrap()extensively on deserialization and database operations, which can cause tests to panic instead of properly reporting test failures when unexpected conditions occur. This makes test failures harder to debug and can mask underlying issues.Suggested fix: Replace
.unwrap()calls with proper error handling usingassert!orexpect()to provide meaningful failure messages. For example:resp.json().await.expect("Failed to parse JSON response")*Scanner: code-review/convention | *
@@ -0,0 +14,4 @@// Add a repositorylet resp = server.post("/api/v1/repositories",[medium] Inconsistent error handling with unwrap() in test code
The test code uses
.unwrap()extensively on deserialization and database operations, which can cause tests to panic instead of properly reporting test failures when unexpected conditions occur. This makes test failures harder to debug and can mask underlying issues.*Scanner: code-review/convention | *
@@ -0,0 +29,4 @@// List should now return 1let resp = server.get("/api/v1/repositories").await;let body: serde_json::Value = resp.json().await.unwrap();let repos = body["data"].as_array().unwrap();[medium] Inconsistent error handling with unwrap() in test code
The test code uses
.unwrap()extensively on deserialization and database operations, which can cause tests to panic instead of properly reporting test failures when unexpected conditions occur. This makes test failures harder to debug and can mask underlying issues.*Scanner: code-review/convention | *
@@ -0,0 +17,4 @@.await;// Insert findings directlylet mongodb_uri = std::env::var("TEST_MONGODB_URI")[medium] Hardcoded MongoDB Credentials in Test Code
The test code contains hardcoded MongoDB credentials in the form of a connection string with username 'root' and password 'example'. While this is only in test files, it represents a potential security risk if these tests were to be run in an environment where such credentials could be exposed or misused.
Suggested fix: Use environment variables or a secure configuration management system for database credentials even in test environments. Consider using a dedicated test database with unique credentials.
Scanner: code-review/security | CWE: CWE-798
[medium] Deeply nested control flow in test setup
The test function 'stats_overview_reflects_inserted_data' has deeply nested control flow with multiple async operations and database interactions that are not clearly separated. This increases the cognitive load when trying to understand the test flow and could lead to errors if any part of the setup is modified.
Suggested fix: Extract the MongoDB setup and data insertion logic into separate helper functions to flatten the control flow and improve readability.
*Scanner: code-review/complexity | *
@@ -0,0 +32,4 @@db.collection::<mongodb::bson::Document>("findings").insert_one(mongodb::bson::doc! {"repo_id": "test-repo-id","fingerprint": format!("fp-{title}"),[high] Incorrect repository ID used in stats test
In the
stats_overview_reflects_inserted_datatest, the MongoDB insert operation uses hardcoded 'test-repo-id' as the repo_id instead of using the actual repository ID that was created in the test. This means the findings won't be associated with any real repository and will cause incorrect stats aggregation.Suggested fix: Store the repository ID from the successful POST request and use it in the MongoDB insert operation instead of the hardcoded value.
*Scanner: code-review/logic | *
@@ -0,0 +47,4 @@}let resp = server.get("/api/v1/stats/overview").await;assert_eq!(resp.status(), 200);[medium] Complex boolean expression in test assertion
The test function 'stats_overview_reflects_inserted_data' contains a complex boolean expression when checking the response status and asserting multiple fields in the response body. This makes it harder to reason about what exactly is being validated and could lead to missed assertions if one part fails.
Suggested fix: Break down the complex assertions into separate test steps or extract the response validation logic into helper functions to make each assertion's purpose clearer.
*Scanner: code-review/complexity | *
@@ -1,4 +1,9 @@// Integration tests for the compliance-agent crate.// E2E / Integration tests for the compliance-agent API.[medium] Inconsistent test module naming
The test file was renamed from 'mod.rs' to 'e2e.rs' but the module declaration still uses 'mod.rs' pattern. The test module name should match the file name for consistency.
Suggested fix: Rename the file to match the module declaration or update the module declaration to match the file name. Since this is a test file, consider renaming to 'e2e.rs' and updating the module declaration accordingly.
*Scanner: code-review/convention | *
@@ -18,0 +22,4 @@/// 1. `"src/main.rs::config"` (trim last `::` segment)/// 2. `"src/main.rs"` (trim again)////// Returns the first match found, or `None` if the node is a root.[high] Off-by-one error in
find_parent_qnamewhen handling qualified names without '::'The
find_parent_qnamefunction incorrectly handles qualified names that do not contain any '::' separator. Whencurrent.rfind("::")returnsNone, it should immediately returnNonebecause there's no parent. However, the current implementation enters an infinite loop becausecurrentis never updated to an empty string, andcurrent.rfind("::")will always returnNoneon subsequent iterations.Suggested fix: Fix the loop condition to properly handle the case where there are no '::' separators. The function should return
Noneimmediately whenrfind("::")returnsNone.*Scanner: code-review/logic | *
@@ -121,7 +148,48 @@ impl GraphEngine {graph.add_edge(src, tgt, edge.kind.clone());[medium] Deeply nested control flow in Contains edge synthesis
The Contains edge synthesis logic in
build_petgraphhas deeply nested control flow with multiple nested conditions and loops. The code walks up the qualified name hierarchy while checking for existing edges, which creates a complex flow that's hard to reason about and could introduce bugs during maintenance.Suggested fix: Flatten the nested structure by extracting the parent lookup logic into a separate function and using early returns to avoid deep nesting. Consider using a more functional approach with iterators to simplify the edge creation logic.
*Scanner: code-review/complexity | *
@@ -122,3 +149,3 @@resolved_edges.push(edge);}// Skip unresolved edges (cross-file, external deps) — conservative approach}[medium] Potential panic in Contains edge synthesis due to unwrapping
In the
build_petgraphfunction, there are several instances of.unwrap_or(""))and.cloned().unwrap_or_default()which can lead to panics or incorrect behavior if the nodes collection is empty or if expected fields are missing. Specifically, when accessingnodes.first()and then calling.as_str()on potentially missing fields, this could cause runtime issues if the parsing output structure isn't consistent.Suggested fix: Use proper error propagation instead of unwrapping. Replace
unwrap_or("")with explicit error handling or default values that don't risk panicking. Also consider adding assertions or early returns to validate that required fields exist before proceeding.*Scanner: code-review/convention | *
@@ -132,33 +200,62 @@ impl GraphEngine {})[medium] Complex boolean expression in edge resolution
The
resolve_edge_targetfunction contains a complex boolean expression that checks multiple conditions for edge resolution. The logic involves multiple string pattern matching operations with various prefixes and suffixes, making it difficult to follow and prone to subtle bugs when modifying the resolution strategies.Suggested fix: Break down the complex conditionals into separate helper functions or early returns to improve readability and reduce the chance of logical errors when adding new resolution strategies.
*Scanner: code-review/complexity | *
@@ -144,3 +218,3 @@}// Try matching just the function/type name (intra-file resolution)// 2. Suffix match: "foo" → "path/file.rs::foo"[medium] Inconsistent error handling in edge resolution
The
resolve_edge_targetfunction uses multiple resolution strategies but doesn't consistently handle cases where resolution fails. Specifically, the module-path matching strategy has a fallback that usesunwrap_or(target)which could silently ignore resolution failures. Additionally, the function returnsNonefor failed resolutions but the logic aroundstrippedandsegmentsprocessing could lead to missed matches without clear indication.Suggested fix: Replace
unwrap_or(target)with explicit error handling or logging to ensure resolution failures are properly tracked. Consider using a more robust approach for handling the module-path matching that doesn't silently fall back to the original target.*Scanner: code-review/convention | *
@@ -356,0 +461,4 @@output.nodes.push(make_node("src/main.rs::config::load"));let code_graph = engine.build_petgraph(output).unwrap();[low] Test assertion relies on specific ordering of edges
The test
test_contains_edges_synthesisedchecks for the presence of Contains edges by collecting all edges and asserting their count, but it doesn't verify that the specific parent-child relationships are correctly formed. The assertion only checks that the sources contain the expected values, but doesn't ensure that the edges actually connect the correct nodes in the graph structure.Suggested fix: Enhance the test to also verify that the actual graph connections match the expected parent-child relationships by checking both source and target of the edges, not just the sources.
*Scanner: code-review/convention | *