feat: add E2E test suite with nightly CI, fix dashboard Dockerfile #52

Merged
sharang merged 3 commits from feat/e2e-tests into main 2026-03-30 10:04:07 +00:00
18 changed files with 1124 additions and 37 deletions

View File

@@ -70,7 +70,7 @@ jobs:
# Tests (reuses compilation artifacts from clippy)
- name: Tests (core + agent)
run: cargo test -p compliance-core -p compliance-agent
run: cargo test -p compliance-core -p compliance-agent --lib
Review

[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-agent to cargo 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 | *

**[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-agent` to `cargo 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 | * <!-- compliance-fp:0318699a9f87360b01475eb9fc5c879e57a3d39cc3c95a39b027f587a481c7cd -->
Review

[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 | *

**[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 | * <!-- compliance-fp:7c6667a7ba5d7d102c83c987e9e0130f4537c7325d05f0f869a4296f75e3b05e -->
- name: Tests (dashboard server)
run: cargo test -p compliance-dashboard --features server --no-default-features
- name: Tests (dashboard web)

View File

@@ -0,0 +1,52 @@
name: Nightly E2E Tests
on:
schedule:
- cron: '0 3 * * *' # 3 AM UTC daily
workflow_dispatch: # Allow manual trigger
env:
CARGO_TERM_COLOR: always
RUSTFLAGS: "-D warnings"
RUSTC_WRAPPER: /usr/local/bin/sccache
SCCACHE_DIR: /tmp/sccache
TEST_MONGODB_URI: "mongodb://root:example@mongo:27017/?authSource=admin"
concurrency:
group: nightly-e2e
cancel-in-progress: true
Review

[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

**[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* <!-- compliance-fp:b89ca0314ccd87c2ebae0f837d53434ab52e1ea699b7aa1f44da43e353815f65 -->
Review

[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

**[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* <!-- compliance-fp:b89ca0314ccd87c2ebae0f837d53434ab52e1ea699b7aa1f44da43e353815f65 -->
jobs:
e2e:
name: E2E Tests
runs-on: docker
container:
Review

[high] Incorrect Git Checkout in Nightly Workflow

The nightly workflow uses git checkout FETCH_HEAD which may not correctly handle shallow clones or specific commit references. When GITHUB_SHA is 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 of FETCH_HEAD to ensure consistent checkout behavior regardless of how the workflow is triggered.

*Scanner: code-review/logic | *

**[high] Incorrect Git Checkout in Nightly Workflow** The nightly workflow uses `git checkout FETCH_HEAD` which may not correctly handle shallow clones or specific commit references. When `GITHUB_SHA` is 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 of `FETCH_HEAD` to ensure consistent checkout behavior regardless of how the workflow is triggered. *Scanner: code-review/logic | * <!-- compliance-fp:c5b52d0d054cb365b5c2c5d637138fe7994386e85cc3966734e5f92be127653d -->
image: rust:1.94-bookworm
Review

[high] Incorrect Git Checkout in Nightly Workflow

The nightly workflow uses git checkout FETCH_HEAD which 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_HEAD or ensure proper handling of the fetched commit with explicit reference resolution.

*Scanner: code-review/logic | *

**[high] Incorrect Git Checkout in Nightly Workflow** The nightly workflow uses `git checkout FETCH_HEAD` which 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_HEAD` or ensure proper handling of the fetched commit with explicit reference resolution. *Scanner: code-review/logic | * <!-- compliance-fp:8514398dee99f9b2c2ce5751bcd8aa59184e21da957b2c6286756caf8fc91d0a -->
services:
Review

[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 | *

**[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 | * <!-- compliance-fp:ee8d33daa31a5770c9a9fac970ddabaf6c59f91f0f5723a5a2e15971a9e6da55 -->
mongo:
image: mongo:7
env:
MONGO_INITDB_ROOT_USERNAME: root
MONGO_INITDB_ROOT_PASSWORD: example
steps:
- name: Checkout
run: |
git init
git remote add origin "${GITHUB_SERVER_URL}/${GITHUB_REPOSITORY}.git"
git fetch --depth=1 origin "${GITHUB_SHA:-refs/heads/main}"
git checkout FETCH_HEAD
- name: Install sccache
run: |
curl -fsSL https://github.com/mozilla/sccache/releases/download/v0.9.1/sccache-v0.9.1-x86_64-unknown-linux-musl.tar.gz \
| tar xz --strip-components=1 -C /usr/local/bin/ sccache-v0.9.1-x86_64-unknown-linux-musl/sccache
chmod +x /usr/local/bin/sccache
env:
RUSTC_WRAPPER: ""
Review

[high] Missing Error Handling in E2E Test Step

The E2E test step does not explicitly check for failure of the cargo test command. If the tests fail, the workflow will continue without indicating failure, potentially masking test failures.

Suggested fix: Add explicit error checking such as set -e before the cargo test command or use || exit 1 to ensure workflow fails on test failure.

*Scanner: code-review/logic | *

**[high] Missing Error Handling in E2E Test Step** The E2E test step does not explicitly check for failure of the `cargo test` command. If the tests fail, the workflow will continue without indicating failure, potentially masking test failures. Suggested fix: Add explicit error checking such as `set -e` before the cargo test command or use `|| exit 1` to ensure workflow fails on test failure. *Scanner: code-review/logic | * <!-- compliance-fp:c1caa3bb910cc6ebd28a7a8138a1dca0cf8daffd56aa3965dcb5de590f3b1166 -->
Review

[high] Missing Error Handling in E2E Test Step

The E2E test step does not explicitly check for failure of the cargo test command. If the tests fail, the workflow will continue without indicating failure, potentially masking test failures.

Suggested fix: Add explicit error handling or use set -e to ensure the workflow fails when tests do not pass.

*Scanner: code-review/logic | *

**[high] Missing Error Handling in E2E Test Step** The E2E test step does not explicitly check for failure of the `cargo test` command. If the tests fail, the workflow will continue without indicating failure, potentially masking test failures. Suggested fix: Add explicit error handling or use `set -e` to ensure the workflow fails when tests do not pass. *Scanner: code-review/logic | * <!-- compliance-fp:c1caa3bb910cc6ebd28a7a8138a1dca0cf8daffd56aa3965dcb5de590f3b1166 -->
- name: Run E2E tests
run: cargo test -p compliance-agent --test e2e -- --test-threads=4
Review

[medium] Inconsistent error handling in E2E workflow

The nightly E2E workflow uses cargo test without the --quiet flag 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 -e or checking exit codes explicitly.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling in E2E workflow** The nightly E2E workflow uses `cargo test` without the `--quiet` flag 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 -e` or checking exit codes explicitly. *Scanner: code-review/convention | * <!-- compliance-fp:0e3143be5f7b05063028617a8644cbace0a40ed2a07e97f7164bd0cf55524da6 -->
Review

[medium] Inconsistent error handling in E2E workflow

The nightly E2E workflow uses cargo test without the --quiet flag 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 | *

**[medium] Inconsistent error handling in E2E workflow** The nightly E2E workflow uses `cargo test` without the `--quiet` flag 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 | * <!-- compliance-fp:0e3143be5f7b05063028617a8644cbace0a40ed2a07e97f7164bd0cf55524da6 -->
- name: Show sccache stats
run: sccache --show-stats
if: always()
Review

[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 | *

**[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 | * <!-- compliance-fp:faf02a438de27ddef1487637a3be46588628d484be0009002f05bfcffad9c281 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:faf02a438de27ddef1487637a3be46588628d484be0009002f05bfcffad9c281 -->

View File

@@ -1,6 +1,6 @@
FROM rust:1.94-bookworm AS builder
RUN cargo install dioxus-cli --version 0.7.3
RUN cargo install dioxus-cli --version 0.7.4
Review

[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 | *

**[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 | * <!-- compliance-fp:a1c9b817ce2d3144852b328a8baa62e3a6b99e0b11d5dde6616c674eac3176e1 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:a1c9b817ce2d3144852b328a8baa62e3a6b99e0b11d5dde6616c674eac3176e1 -->
ARG DOCS_URL=/docs

View File

@@ -42,3 +42,14 @@ tokio-tungstenite = { version = "0.26", features = ["rustls-tls-webpki-roots"] }
futures-core = "0.3"
dashmap = { workspace = true }
tokio-stream = { workspace = true }
[dev-dependencies]
Review

[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 | *

**[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 | * <!-- compliance-fp:b192cfa323eab5d28a5a4ff14e0a715e13e33f6c534225e4c81f81c2fedc4a7d -->
Review

[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 | *

**[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 | * <!-- compliance-fp:b192cfa323eab5d28a5a4ff14e0a715e13e33f6c534225e4c81f81c2fedc4a7d -->
Review

[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 | *

**[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 | * <!-- compliance-fp:d2c8d75ebbc20c642be01b08b9ba94dee38a8d620dff7c740ec7c88b41fbc582 -->
compliance-core = { workspace = true, features = ["mongodb"] }
reqwest = { workspace = true }
serde_json = { workspace = true }
tokio = { workspace = true }
mongodb = { workspace = true }
uuid = { workspace = true }
secrecy = { workspace = true }
axum = "0.8"
tower-http = { version = "0.6", features = ["cors"] }

View File

@@ -0,0 +1,16 @@
// Library entrypoint — re-exports for integration tests and the binary.
pub mod agent;
pub mod api;
pub mod config;
pub mod database;
pub mod error;
pub mod llm;
pub mod pentest;
pub mod pipeline;
pub mod rag;
pub mod scheduler;
pub mod ssh;
#[allow(dead_code)]
pub mod trackers;
Review

[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 | *

**[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 | * <!-- compliance-fp:5bdd3605df83bebae2ab51f87fce135a82a11f45f9a2da88cf484c02fa291201 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:5bdd3605df83bebae2ab51f87fce135a82a11f45f9a2da88cf484c02fa291201 -->
Review

[low] Unnecessary allow(dead_code) for public module

The trackers module 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 | *

**[low] Unnecessary allow(dead_code) for public module** The `trackers` module 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 | * <!-- compliance-fp:675899286d2d36214519f014d8b16e0b45179e1ea16e55c63a84e9e3f8ad5433 -->
pub mod webhooks;

View File

@@ -1,17 +1,4 @@
mod agent;
mod api;
pub(crate) mod config;
mod database;
mod error;
mod llm;
mod pentest;
mod pipeline;
mod rag;
mod scheduler;
mod ssh;
#[allow(dead_code)]
mod trackers;
mod webhooks;
use compliance_agent::{agent, api, config, database, scheduler, ssh, webhooks};
Review

[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] 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 | * <!-- compliance-fp:41f88a7df4fd8b57780764c05bbf3c04d375edaa0c709869cedec9141321d696 -->
Review

[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] 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 | * <!-- compliance-fp:41f88a7df4fd8b57780764c05bbf3c04d375edaa0c709869cedec9141321d696 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:8e30c60ca264041404baad6c4dc86e6e9463b7a62da4a11246fe008fef4a7a94 -->
#[tokio::main]
async fn main() -> Result<(), Box<dyn std::error::Error>> {

View File

@@ -33,6 +33,7 @@ struct PatternRule {
file_extensions: Vec<String>,
}
#[allow(clippy::new_without_default)]
impl GdprPatternScanner {
pub fn new() -> Self {
let patterns = vec![
@@ -98,6 +99,7 @@ impl Scanner for GdprPatternScanner {
}
}
#[allow(clippy::new_without_default)]
impl OAuthPatternScanner {
pub fn new() -> Self {
let patterns = vec![

View File

@@ -1,3 +1,165 @@
// Shared test helpers for compliance-agent integration tests.
// Shared test harness for E2E / integration tests.
//
// Add database mocks, fixtures, and test utilities here.
// Spins up the agent API server on a random port with an isolated test
// database. Each test gets a fresh database that is dropped on cleanup.
use std::sync::Arc;
use compliance_agent::agent::ComplianceAgent;
use compliance_agent::api;
use compliance_agent::database::Database;
use compliance_core::AgentConfig;
use secrecy::SecretString;
Review

[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 | *

**[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 | * <!-- compliance-fp:9aff8d47c0bb2816345efafcb1b1cb15c2ce584c7f78e8fa2b883542706f3030 -->
/// A running test server with a unique database.
pub struct TestServer {
pub base_url: String,
Review

[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 | *

**[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 | * <!-- compliance-fp:7f2eda4eaf506407e7f067c8a66cab6bf7f6c8f98f68978664613963d67637d4 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:7f2eda4eaf506407e7f067c8a66cab6bf7f6c8f98f68978664613963d67637d4 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:7f2eda4eaf506407e7f067c8a66cab6bf7f6c8f98f68978664613963d67637d4 -->
pub client: reqwest::Client,
db_name: String,
Review

[medium] Inconsistent error handling with unwrap() in test setup

The test helper uses unwrap_or_else() with expect() inside the start() method, which can mask underlying configuration issues. The unwrap() 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 | *

**[medium] Inconsistent error handling with unwrap() in test setup** The test helper uses `unwrap_or_else()` with `expect()` inside the `start()` method, which can mask underlying configuration issues. The `unwrap()` 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 | * <!-- compliance-fp:fda6f59952019f2f02a88604a80615c33d2146e6bbdef1f65f431b8012b4b3bf -->
mongodb_uri: String,
Review

[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 | *

**[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 | * <!-- compliance-fp:885abd06eddecef12d09e88ad71da4ecc887fd059b99f2a44eb69abfbec24323 -->
}
Review

[medium] Inconsistent error handling with unwrap() in test code

The test code uses unwrap_or_else() followed by expect() in multiple places, which creates inconsistent error handling patterns. Specifically, line 21 uses unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into()) followed by expect("Failed to connect to MongoDB — is it running?") on line 28. This pattern should be consistent - either use expect() directly or handle errors more gracefully.

Suggested fix: Use consistent error handling by either removing the unwrap_or_else and using expect directly on the environment variable, or refactor to use proper error propagation with ? operator.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling with unwrap() in test code** The test code uses `unwrap_or_else()` followed by `expect()` in multiple places, which creates inconsistent error handling patterns. Specifically, line 21 uses `unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into())` followed by `expect("Failed to connect to MongoDB — is it running?")` on line 28. This pattern should be consistent - either use `expect()` directly or handle errors more gracefully. Suggested fix: Use consistent error handling by either removing the `unwrap_or_else` and using `expect` directly on the environment variable, or refactor to use proper error propagation with `?` operator. *Scanner: code-review/convention | * <!-- compliance-fp:469d9c0a9c5c001577c353a26de6f37b6dd58630952a89d41e578b4d7b7c5e24 -->
Review

[medium] Inconsistent error handling with unwrap() in test code

The test code uses unwrap_or_else() followed by expect() in multiple places, which creates inconsistent error handling patterns. Specifically, line 21 uses unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into()) followed by expect("Failed to connect to MongoDB — is it running?") on line 28. This pattern should be consistent - either use expect() directly on the result or handle errors more gracefully.

Suggested fix: Use consistent error handling by either removing the unwrap_or_else and using expect() directly on the environment variable read, or refactor to use proper error propagation with ? operator.

*Scanner: code-review/convention | *

**[medium] Inconsistent error handling with unwrap() in test code** The test code uses `unwrap_or_else()` followed by `expect()` in multiple places, which creates inconsistent error handling patterns. Specifically, line 21 uses `unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into())` followed by `expect("Failed to connect to MongoDB — is it running?")` on line 28. This pattern should be consistent - either use `expect()` directly on the result or handle errors more gracefully. Suggested fix: Use consistent error handling by either removing the `unwrap_or_else` and using `expect()` directly on the environment variable read, or refactor to use proper error propagation with `?` operator. *Scanner: code-review/convention | * <!-- compliance-fp:469d9c0a9c5c001577c353a26de6f37b6dd58630952a89d41e578b4d7b7c5e24 -->
impl TestServer {
/// Start an agent API server on a random port with an isolated database.
pub async fn start() -> Self {
let mongodb_uri = std::env::var("TEST_MONGODB_URI")
.unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into());
// Unique database name per test run to avoid collisions
let db_name = format!("test_{}", uuid::Uuid::new_v4().simple());
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");
Review

[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 | *

**[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 | * <!-- compliance-fp:4d85f24a45e6921e3f0d750bcd89db21c8c9890bfcc02ccecebdbb9f6039d0d1 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:e9e6fefe855d78d58f4616ebc55d03a86a68c1166fff2dcb4f9b592358e726c9 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:e9e6fefe855d78d58f4616ebc55d03a86a68c1166fff2dcb4f9b592358e726c9 -->
let config = AgentConfig {
mongodb_uri: mongodb_uri.clone(),
mongodb_database: db_name.clone(),
litellm_url: std::env::var("TEST_LITELLM_URL")
.unwrap_or_else(|_| "http://localhost:4000".into()),
litellm_api_key: SecretString::from(String::new()),
litellm_model: "gpt-4o".into(),
litellm_embed_model: "text-embedding-3-small".into(),
agent_port: 0, // not used — we bind ourselves
scan_schedule: String::new(),
cve_monitor_schedule: String::new(),
git_clone_base_path: "/tmp/compliance-scanner-tests/repos".into(),
ssh_key_path: "/tmp/compliance-scanner-tests/ssh/id_ed25519".into(),
github_token: None,
github_webhook_secret: None,
gitlab_url: None,
gitlab_token: None,
gitlab_webhook_secret: None,
jira_url: None,
jira_email: None,
jira_api_token: None,
jira_project_key: None,
searxng_url: None,
nvd_api_key: None,
keycloak_url: None,
keycloak_realm: None,
keycloak_admin_username: None,
keycloak_admin_password: None,
pentest_verification_email: None,
pentest_imap_host: None,
pentest_imap_port: None,
pentest_imap_tls: false,
pentest_imap_username: None,
pentest_imap_password: None,
};
let agent = ComplianceAgent::new(config, db);
// Build the router with the agent extension
let app = api::routes::build_router()
.layer(axum::extract::Extension(Arc::new(agent)))
.layer(tower_http::cors::CorsLayer::permissive());
Review

[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 | *

**[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 | * <!-- compliance-fp:e10cc2c8ba2703e6eb21ba7415358a7664b937f3eb7c2775f63e20823a13ff66 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:53aee5eb0a41128b2ae0e2d3877dff01c717048a63461cb0018cc295ae4cb787 -->
// Bind to port 0 to get a random available port
let listener = tokio::net::TcpListener::bind("127.0.0.1:0")
.await
.expect("Failed to bind test server");
let port = listener.local_addr().expect("no local addr").port();
Review

[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 | *

**[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 | * <!-- compliance-fp:827b6208efdd88d9826777a08d6f112fc7625b8dee3d3355e9b1dc195976147c -->
tokio::spawn(async move {
axum::serve(listener, app).await.ok();
});
let base_url = format!("http://127.0.0.1:{port}");
let client = reqwest::Client::builder()
.timeout(std::time::Duration::from_secs(30))
.build()
.expect("Failed to build HTTP client");
// Wait for server to be ready
for _ in 0..50 {
if client
.get(format!("{base_url}/api/v1/health"))
.send()
.await
.is_ok()
{
break;
}
tokio::time::sleep(std::time::Duration::from_millis(50)).await;
}
Self {
base_url,
client,
db_name,
mongodb_uri,
}
}
/// GET helper
pub async fn get(&self, path: &str) -> reqwest::Response {
self.client
.get(format!("{}{path}", self.base_url))
.send()
.await
.expect("GET request failed")
}
/// POST helper with JSON body
pub async fn post(&self, path: &str, body: &serde_json::Value) -> reqwest::Response {
self.client
.post(format!("{}{path}", self.base_url))
.json(body)
.send()
.await
Review

[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 | *

**[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 | * <!-- compliance-fp:6bf1cee307ab60c1a3a4a42bf4fb3db372df72373d2dbec007d26b2ddd6cc8aa -->
.expect("POST request failed")
}
/// PATCH helper with JSON body
pub async fn patch(&self, path: &str, body: &serde_json::Value) -> reqwest::Response {
self.client
.patch(format!("{}{path}", self.base_url))
.json(body)
.send()
.await
.expect("PATCH request failed")
}
/// DELETE helper
pub async fn delete(&self, path: &str) -> reqwest::Response {
self.client
.delete(format!("{}{path}", self.base_url))
.send()
.await
.expect("DELETE request failed")
}
/// Get the unique database name for direct MongoDB access in tests.
Review

[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 | *

**[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 | * <!-- compliance-fp:9741533d729a93f99f64ad0b1e37a1056303311f3e205249e0f06b392fa20ecc -->
Review

[medium] Potential panic in test cleanup when MongoDB connection fails

In the cleanup method (lines 154-157), if mongodb::Client::with_uri_str() fails, the test will panic due to .ok() being called on a Result. The ok() method converts Result<T, E> to Option<T>, but since we're calling ok() on the result of drop(), 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 | *

**[medium] Potential panic in test cleanup when MongoDB connection fails** In the `cleanup` method (lines 154-157), if `mongodb::Client::with_uri_str()` fails, the test will panic due to `.ok()` being called on a `Result`. The `ok()` method converts `Result<T, E>` to `Option<T>`, but since we're calling `ok()` on the result of `drop()`, 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 | * <!-- compliance-fp:19abece4b8645e793765181f7b4af4e48c6bab6c4712a36413e71c5a5bd31cd1 -->
pub fn db_name(&self) -> &str {
&self.db_name
}
Review

[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 | *

**[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 | * <!-- compliance-fp:d19397e2892a5b583d7eeb7ab41ca336640683bf0a139bc6f720fbd539a4d6d6 -->
/// Drop the test database on cleanup
pub async fn cleanup(&self) {
if let Ok(client) = mongodb::Client::with_uri_str(&self.mongodb_uri).await {
client.database(&self.db_name).drop().await.ok();
}
}
}

View File

@@ -0,0 +1,7 @@
// E2E test entry point.
//
// Run with: cargo test -p compliance-agent --test e2e
// Requires: MongoDB running (set TEST_MONGODB_URI if not default)
mod common;
mod integration;

View File

@@ -0,0 +1,221 @@
use crate::common::TestServer;
use serde_json::json;
/// Insert a DAST target directly into MongoDB linked to a repo.
async fn insert_dast_target(server: &TestServer, repo_id: &str, name: &str) -> String {
let 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();
let db = client.database(&server.db_name());
Review

[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] 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 | * <!-- compliance-fp:ae6e223faef673f642a0ce214f5ed44b8e24155b374bc75b5b52ce4bbd091eb6 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:fc1aa5c7b5158ab4641b468aca6c47d2df8936030f891932ab7c0070c7c21ae9 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:e3f15772ab5ee78fdf6a1284f88de60a02cd8cca0bb39252f91be89e588618de -->
Review

[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 | *

**[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 | * <!-- compliance-fp:aa407af431fa19ba934a9558be1b7b662573001db8106455c8b9dbd574b7f87d -->
let result = db
.collection::<mongodb::bson::Document>("dast_targets")
Review

[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

**[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* <!-- compliance-fp:666a556b613798e8d21034f2afb84e6c3827c40785f03d42534652b0236ba507 -->
Review

[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

**[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* <!-- compliance-fp:666a556b613798e8d21034f2afb84e6c3827c40785f03d42534652b0236ba507 -->
Review

[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

**[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* <!-- compliance-fp:666a556b613798e8d21034f2afb84e6c3827c40785f03d42534652b0236ba507 -->
.insert_one(mongodb::bson::doc! {
"name": name,
Review

[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] 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 | * <!-- compliance-fp:bdfaf8bbadabecc8883732b4d0471b8ea372966117f50ad6b7bd1168ac4aa620 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:bacc9b89684ad14ff1299abb1a7cf92272bcce7f000b1d8e39b4c81927b44de0 -->
Review

[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] 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 | * <!-- compliance-fp:a29306b4a28b344cb9becff8f2e30ca7f8f76c7071f319f797812caf9cba2ce2 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:bacc9b89684ad14ff1299abb1a7cf92272bcce7f000b1d8e39b4c81927b44de0 -->
Review

[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] 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 | * <!-- compliance-fp:821299b13807e9a19b9e1ed216ef41dff72060f444deb0f9bf3d5fbaa6c5d363 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:bacc9b89684ad14ff1299abb1a7cf92272bcce7f000b1d8e39b4c81927b44de0 -->
"base_url": format!("https://{name}.example.com"),
Review

[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 | *

**[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 | * <!-- compliance-fp:97476c58e5efd4eb5fe7e4cfe5cf8b0bb09f358119112031033fa369f083ff45 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:584716b22b11921b441c0a69c9a1076825707c003df1be233474c1f556672999 -->
Review

[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 of unwrap() 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 | *

**[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 of `unwrap()` 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 | * <!-- compliance-fp:97476c58e5efd4eb5fe7e4cfe5cf8b0bb09f358119112031033fa369f083ff45 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:e478898f300ac54e083c30ad71c2fd8e312c74055956685f7b56b1471d8e93c1 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:0fcdf5def87f7cdb7a69aa0f87361d79e5d5822e2a24ead39f6800f3ed805abc -->
Review

[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 | *

**[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 | * <!-- compliance-fp:97476c58e5efd4eb5fe7e4cfe5cf8b0bb09f358119112031033fa369f083ff45 -->
"target_type": "webapp",
"repo_id": repo_id,
"rate_limit": 10,
"allow_destructive": false,
"created_at": mongodb::bson::DateTime::now(),
})
.await
.unwrap();
result.inserted_id.as_object_id().unwrap().to_hex()
}
/// Insert a pentest session linked to a target.
async fn insert_pentest_session(server: &TestServer, target_id: &str, repo_id: &str) -> String {
let 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();
let db = client.database(&server.db_name());
let result = db
.collection::<mongodb::bson::Document>("pentest_sessions")
.insert_one(mongodb::bson::doc! {
"target_id": target_id,
"repo_id": repo_id,
"strategy": "comprehensive",
"status": "completed",
"findings_count": 1_i32,
"exploitable_count": 0_i32,
"created_at": mongodb::bson::DateTime::now(),
})
.await
.unwrap();
result.inserted_id.as_object_id().unwrap().to_hex()
}
/// Insert an attack chain node linked to a session.
async fn insert_attack_node(server: &TestServer, session_id: &str) {
let 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();
let db = client.database(&server.db_name());
db.collection::<mongodb::bson::Document>("attack_chain_nodes")
.insert_one(mongodb::bson::doc! {
"session_id": session_id,
"node_id": "node-1",
"tool_name": "recon",
"status": "completed",
"created_at": mongodb::bson::DateTime::now(),
})
.await
.unwrap();
}
/// Insert a DAST finding linked to a target.
async fn insert_dast_finding(server: &TestServer, target_id: &str, session_id: &str) {
let 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();
let db = client.database(&server.db_name());
db.collection::<mongodb::bson::Document>("dast_findings")
.insert_one(mongodb::bson::doc! {
"scan_run_id": "run-1",
"target_id": target_id,
"vuln_type": "xss",
"title": "Reflected XSS",
"description": "XSS in search param",
"severity": "high",
"endpoint": "https://example.com/search",
"method": "GET",
"exploitable": true,
"evidence": [],
"session_id": session_id,
"created_at": mongodb::bson::DateTime::now(),
})
.await
.unwrap();
}
/// Helper to count documents in a collection
async fn count_docs(server: &TestServer, collection: &str) -> u64 {
let 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();
let db = client.database(&server.db_name());
db.collection::<mongodb::bson::Document>(collection)
.count_documents(mongodb::bson::doc! {})
.await
.unwrap()
}
#[tokio::test]
async fn delete_repo_cascades_to_dast_and_pentest_data() {
let server = TestServer::start().await;
// Create a repo
let resp = server
.post(
"/api/v1/repositories",
&json!({
"name": "cascade-test",
"git_url": "https://github.com/example/cascade-test.git",
}),
)
.await;
let body: serde_json::Value = resp.json().await.unwrap();
let repo_id = body["data"]["id"].as_str().unwrap().to_string();
// Insert DAST target linked to repo
let target_id = insert_dast_target(&server, &repo_id, "cascade-target").await;
// Insert pentest session linked to target
let session_id = insert_pentest_session(&server, &target_id, &repo_id).await;
// Insert downstream data
insert_attack_node(&server, &session_id).await;
insert_dast_finding(&server, &target_id, &session_id).await;
// Verify data exists
assert_eq!(count_docs(&server, "dast_targets").await, 1);
assert_eq!(count_docs(&server, "pentest_sessions").await, 1);
assert_eq!(count_docs(&server, "attack_chain_nodes").await, 1);
assert_eq!(count_docs(&server, "dast_findings").await, 1);
// Delete the repo
let resp = server
.delete(&format!("/api/v1/repositories/{repo_id}"))
.await;
assert_eq!(resp.status(), 200);
// All downstream data should be gone
assert_eq!(count_docs(&server, "dast_targets").await, 0);
assert_eq!(count_docs(&server, "pentest_sessions").await, 0);
Review

[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

**[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* <!-- compliance-fp:62d4c103e1bf533d1c71157851db26a26ed1701bd7ac76653eb8ac18220d4b7f -->
Review

[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

**[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* <!-- compliance-fp:62d4c103e1bf533d1c71157851db26a26ed1701bd7ac76653eb8ac18220d4b7f -->
Review

[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

**[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* <!-- compliance-fp:bd27914f737daa52c5daeb876ce6deaf0b04b6513adf37d2e20bcf8c6241fc73 -->
assert_eq!(count_docs(&server, "attack_chain_nodes").await, 0);
assert_eq!(count_docs(&server, "dast_findings").await, 0);
server.cleanup().await;
}
#[tokio::test]
async fn delete_repo_cascades_sast_findings_and_sbom() {
let server = TestServer::start().await;
// Create a repo
let resp = server
.post(
"/api/v1/repositories",
&json!({
"name": "sast-cascade",
"git_url": "https://github.com/example/sast-cascade.git",
}),
)
.await;
let body: serde_json::Value = resp.json().await.unwrap();
let repo_id = body["data"]["id"].as_str().unwrap().to_string();
// Insert SAST finding and SBOM entry
let 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();
let db = client.database(&server.db_name());
let now = mongodb::bson::DateTime::now();
db.collection::<mongodb::bson::Document>("findings")
.insert_one(mongodb::bson::doc! {
"repo_id": &repo_id,
"fingerprint": "fp-test-1",
"scanner": "semgrep",
"scan_type": "sast",
"title": "SQL Injection",
"description": "desc",
"severity": "critical",
"status": "open",
"created_at": now,
"updated_at": now,
})
.await
.unwrap();
db.collection::<mongodb::bson::Document>("sbom_entries")
.insert_one(mongodb::bson::doc! {
"repo_id": &repo_id,
"name": "lodash",
"version": "4.17.20",
"package_manager": "npm",
"known_vulnerabilities": [],
})
.await
.unwrap();
assert_eq!(count_docs(&server, "findings").await, 1);
assert_eq!(count_docs(&server, "sbom_entries").await, 1);
// Delete repo
server
.delete(&format!("/api/v1/repositories/{repo_id}"))
.await;
// Both should be gone
assert_eq!(count_docs(&server, "findings").await, 0);
assert_eq!(count_docs(&server, "sbom_entries").await, 0);
server.cleanup().await;
}

View File

@@ -0,0 +1,48 @@
use crate::common::TestServer;
use serde_json::json;
#[tokio::test]
async fn add_and_list_dast_targets() {
let server = TestServer::start().await;
// Initially empty
let resp = server.get("/api/v1/dast/targets").await;
assert_eq!(resp.status(), 200);
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["data"].as_array().unwrap().len(), 0);
// Add a target
let resp = server
Review

[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 | *

**[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 | * <!-- compliance-fp:0abbac6de0a7badf47cceef05c9468bf716c13565eab29234691a7e5a9ab40f7 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:0abbac6de0a7badf47cceef05c9468bf716c13565eab29234691a7e5a9ab40f7 -->
.post(
"/api/v1/dast/targets",
&json!({
"name": "test-app",
"base_url": "https://test-app.example.com",
"target_type": "webapp",
}),
)
.await;
Review

[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 | *

**[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 | * <!-- compliance-fp:f29872ce84570cf392dd7ca7f9d7d0dfa2dcdc76e65b3e817e772eec34396f8a -->
assert_eq!(resp.status(), 200);
// List should return 1
let resp = server.get("/api/v1/dast/targets").await;
let body: serde_json::Value = resp.json().await.unwrap();
let targets = body["data"].as_array().unwrap();
assert_eq!(targets.len(), 1);
assert_eq!(targets[0]["name"], "test-app");
assert_eq!(targets[0]["base_url"], "https://test-app.example.com");
server.cleanup().await;
}
#[tokio::test]
async fn list_dast_findings_empty() {
let server = TestServer::start().await;
let resp = server.get("/api/v1/dast/findings").await;
assert_eq!(resp.status(), 200);
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["data"].as_array().unwrap().len(), 0);
server.cleanup().await;
}

View File

@@ -0,0 +1,144 @@
use crate::common::TestServer;
use serde_json::json;
/// Helper: insert a finding directly via MongoDB for testing query endpoints.
async fn insert_finding(server: &TestServer, repo_id: &str, title: &str, severity: &str) {
// We insert via the agent's DB by posting to the internal test path.
// Since there's no direct "create finding" API, we use MongoDB directly.
let mongodb_uri = std::env::var("TEST_MONGODB_URI")
.unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into());
// Extract the database name from the server's unique DB
// We'll use the agent's internal DB through the stats endpoint to verify
let client = mongodb::Client::with_uri_str(&mongodb_uri).await.unwrap();
// Get the DB name from the test server by parsing the health response
Review

[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] 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 | * <!-- compliance-fp:bca9002ef111c226d89c6f58b68277e9b43933ef911f0ddfd5ce47c1bc01a7a8 -->
Review

[medium] Deeply nested control flow in test helper

The insert_finding helper 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 | *

**[medium] Deeply nested control flow in test helper** The `insert_finding` helper 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 | * <!-- compliance-fp:e660d47598d6efa43b2d329695dd5d5293be2715b79489acc968aef65c907b4c -->
// For now, we use a direct insert approach
let db = client.database(&server.db_name());
Review

[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 | *

**[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 | * <!-- compliance-fp:ba0d34b58d2ca8f4da93986e03c31e44b80faf3a2d04fd76ea56ecef2bf80698 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:014343c6aa85460a2615f864956c7f565eb7e15323d440cb398268f0fd08a0a0 -->
let now = mongodb::bson::DateTime::now();
Review

[medium] Missing error handling in MongoDB connection setup

In the insert_finding function, 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 | *

**[medium] Missing error handling in MongoDB connection setup** In the `insert_finding` function, 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 | * <!-- compliance-fp:9d7bed2c5f134e6c91558a457de1336a4e35f1baf8a573efa3618d171020efd4 -->
db.collection::<mongodb::bson::Document>("findings")
.insert_one(mongodb::bson::doc! {
"repo_id": repo_id,
Review

[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] 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 | * <!-- compliance-fp:4a87437d6459e3ade255fbcaa1a77c5469fa52f44744310eebaa9a078fa0c7f5 -->
Review

[medium] Complex boolean expression in test helper

The insert_finding function 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 | *

**[medium] Complex boolean expression in test helper** The `insert_finding` function 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 | * <!-- compliance-fp:89829ae8491c5df12de66c1b9657cb500904fe3d9eeeb8557bd84d0160149b97 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:a0a7e9a644c313f3313be25eb4d19764058bb9ec92d1ce5654f5b311e731748e -->
"fingerprint": format!("fp-{title}-{severity}"),
Review

[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 | *

**[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 | * <!-- compliance-fp:98334682a9125870f93f577c05362281bdfbf6b4418399e232470db156cd67a5 -->
"scanner": "test-scanner",
"scan_type": "sast",
Review

[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 | *

**[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 | * <!-- compliance-fp:eba98ec82187ce4d4298917435965fa4e0ac4dc759e4ca6c857ef81a06a710da -->
Review

[high] Incorrect MongoDB document insertion in test helper

The insert_finding helper function attempts to insert a document into the 'findings' collection but uses mongodb::bson::Document type incorrectly. It should use mongodb::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()); to let db = client.database(&server.db_name()); and remove the redundant mongodb::bson::Document construction. The correct approach is to use the mongodb::bson::doc! macro directly in the insert_one call.

*Scanner: code-review/logic | *

**[high] Incorrect MongoDB document insertion in test helper** The `insert_finding` helper function attempts to insert a document into the 'findings' collection but uses `mongodb::bson::Document` type incorrectly. It should use `mongodb::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());` to `let db = client.database(&server.db_name());` and remove the redundant `mongodb::bson::Document` construction. The correct approach is to use the `mongodb::bson::doc!` macro directly in the insert_one call. *Scanner: code-review/logic | * <!-- compliance-fp:245827395f1d6d4d65375e5ac3d3461501f4fb35f964f130f8f90c958d9d99f1 -->
"title": title,
"description": format!("Test finding: {title}"),
"severity": severity,
"status": "open",
"created_at": now,
Review

[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 | *

**[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 | * <!-- compliance-fp:505ae941e1edb16b3969886a562d6579f70675ed5fb7f9f5b160398cffa1dca1 -->
"updated_at": now,
})
.await
.unwrap();
}
Review

[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 | *

**[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 | * <!-- compliance-fp:0d6aab1592447d60b1015685b9b163add511d9e5b223d2f61686e49515f8b353 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:0d6aab1592447d60b1015685b9b163add511d9e5b223d2f61686e49515f8b353 -->
#[tokio::test]
async fn list_findings_empty() {
let server = TestServer::start().await;
let resp = server.get("/api/v1/findings").await;
assert_eq!(resp.status(), 200);
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["data"].as_array().unwrap().len(), 0);
assert_eq!(body["total"], 0);
server.cleanup().await;
}
#[tokio::test]
async fn list_findings_with_data() {
let server = TestServer::start().await;
insert_finding(&server, "repo1", "SQL Injection", "critical").await;
insert_finding(&server, "repo1", "XSS", "high").await;
insert_finding(&server, "repo2", "Info Leak", "low").await;
let resp = server.get("/api/v1/findings").await;
assert_eq!(resp.status(), 200);
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["total"], 3);
// Filter by severity
let resp = server.get("/api/v1/findings?severity=critical").await;
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["total"], 1);
assert_eq!(body["data"][0]["title"], "SQL Injection");
// Filter by repo
Review

[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 | *

**[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 | * <!-- compliance-fp:e242b2a49b771f920983d1e4a0bb596935e36571296564b71e7e97fb78c74101 -->
let resp = server.get("/api/v1/findings?repo_id=repo1").await;
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["total"], 2);
server.cleanup().await;
}
#[tokio::test]
async fn update_finding_status() {
let server = TestServer::start().await;
insert_finding(&server, "repo1", "Test Bug", "medium").await;
// Get the finding ID
let resp = server.get("/api/v1/findings").await;
Review

[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 | *

**[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 | * <!-- compliance-fp:20e7acd977bbff1b52d0281fd9ff2974287b2eae7d1d4fb4ba4ec4c0624a494b -->
let body: serde_json::Value = resp.json().await.unwrap();
let finding_id = body["data"][0]["_id"]["$oid"].as_str().unwrap();
// Update status to resolved
let resp = server
.patch(
&format!("/api/v1/findings/{finding_id}/status"),
&json!({ "status": "resolved" }),
)
Review

[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 | *

**[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 | * <!-- compliance-fp:8c1b459e365bc84cb231066c8cf5dab4aa32b83bc856808b0395ea065a39d3a5 -->
.await;
assert_eq!(resp.status(), 200);
// Verify it's updated
let resp = server.get(&format!("/api/v1/findings/{finding_id}")).await;
assert_eq!(resp.status(), 200);
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["data"]["status"], "resolved");
server.cleanup().await;
}
Review

[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 | *

**[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 | * <!-- compliance-fp:f3d519193519dadafd88ce85d5799799919e4fddb0532f83b8addd19bcd3a920 -->
#[tokio::test]
async fn bulk_update_finding_status() {
let server = TestServer::start().await;
insert_finding(&server, "repo1", "Bug A", "high").await;
insert_finding(&server, "repo1", "Bug B", "high").await;
// Get both finding IDs
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()
Review

[medium] Potential race condition in bulk update test

In the bulk_update_finding_status test, 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 | *

**[medium] Potential race condition in bulk update test** In the `bulk_update_finding_status` test, 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 | * <!-- compliance-fp:4bb0442449a77535cd5fec0a5bb198ad6403b4153804a8fbddab1604cad4828b -->
.unwrap()
.iter()
.map(|f| f["_id"]["$oid"].as_str().unwrap().to_string())
.collect();
// Bulk update
let resp = server
.patch(
"/api/v1/findings/bulk-status",
&json!({
"ids": ids,
"status": "false_positive"
}),
)
.await;
assert_eq!(resp.status(), 200);
// Verify both are updated
for id in &ids {
let resp = server.get(&format!("/api/v1/findings/{id}")).await;
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["data"]["status"], "false_positive");
}
server.cleanup().await;
}

View File

@@ -0,0 +1,29 @@
use crate::common::TestServer;
#[tokio::test]
async fn health_endpoint_returns_ok() {
let server = TestServer::start().await;
let resp = server.get("/api/v1/health").await;
assert_eq!(resp.status(), 200);
let body: serde_json::Value = resp.json().await.unwrap();
Review

[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 | *

**[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 | * <!-- compliance-fp:7d42c98eb9f00cdcdb7317e853138058613ec12004afd61be60f1ecd4ddefb43 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:7d42c98eb9f00cdcdb7317e853138058613ec12004afd61be60f1ecd4ddefb43 -->
assert_eq!(body["status"], "ok");
server.cleanup().await;
}
#[tokio::test]
async fn stats_overview_returns_zeroes_on_empty_db() {
let server = TestServer::start().await;
let resp = server.get("/api/v1/stats/overview").await;
assert_eq!(resp.status(), 200);
let body: serde_json::Value = resp.json().await.unwrap();
let data = &body["data"];
assert_eq!(data["repositories"], 0);
assert_eq!(data["total_findings"], 0);
server.cleanup().await;
}

View File

@@ -0,0 +1,6 @@
mod cascade_delete;
mod dast;
mod findings;
mod health;
mod repositories;
mod stats;

View File

@@ -0,0 +1,110 @@
use crate::common::TestServer;
use serde_json::json;
#[tokio::test]
async fn add_and_list_repository() {
let server = TestServer::start().await;
// Initially empty
let resp = server.get("/api/v1/repositories").await;
assert_eq!(resp.status(), 200);
Review

[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 using assert!(result.is_ok()) or similar assertions to make test failures more informative.

*Scanner: code-review/convention | *

**[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 using `assert!(result.is_ok())` or similar assertions to make test failures more informative. *Scanner: code-review/convention | * <!-- compliance-fp:0d7cc17501177dcd3ebbebd6cc55f317b7fdc2228a42b54b1df42321388f4882 -->
Review

[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 using assert! or matches! macros to ensure tests fail gracefully with meaningful error messages rather than panicking.

*Scanner: code-review/convention | *

**[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 using `assert!` or `matches!` macros to ensure tests fail gracefully with meaningful error messages rather than panicking. *Scanner: code-review/convention | * <!-- compliance-fp:0d7cc17501177dcd3ebbebd6cc55f317b7fdc2228a42b54b1df42321388f4882 -->
Review

[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 using assert! or expect() to provide meaningful failure messages. For example: resp.json().await.expect("Failed to parse JSON response")

*Scanner: code-review/convention | *

**[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 using `assert!` or `expect()` to provide meaningful failure messages. For example: `resp.json().await.expect("Failed to parse JSON response")` *Scanner: code-review/convention | * <!-- compliance-fp:0d7cc17501177dcd3ebbebd6cc55f317b7fdc2228a42b54b1df42321388f4882 -->
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["data"].as_array().unwrap().len(), 0);
// Add a repository
let resp = server
.post(
"/api/v1/repositories",
Review

[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 | *

**[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 | * <!-- compliance-fp:224bb81514bf2d5ddd2badfa0d12f2e20407cd8b8b9b99586aff6ac5ba716984 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:224bb81514bf2d5ddd2badfa0d12f2e20407cd8b8b9b99586aff6ac5ba716984 -->
&json!({
"name": "test-repo",
"git_url": "https://github.com/example/test-repo.git",
}),
)
.await;
assert_eq!(resp.status(), 200);
let body: serde_json::Value = resp.json().await.unwrap();
let repo_id = body["data"]["id"].as_str().unwrap().to_string();
assert!(!repo_id.is_empty());
// List should now return 1
let resp = server.get("/api/v1/repositories").await;
let body: serde_json::Value = resp.json().await.unwrap();
let repos = body["data"].as_array().unwrap();
Review

[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 | *

**[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 | * <!-- compliance-fp:a2fb8026d06e5c35159e084fd434444313bad98fe9f5acd762142ba80aebda04 -->
assert_eq!(repos.len(), 1);
assert_eq!(repos[0]["name"], "test-repo");
server.cleanup().await;
}
#[tokio::test]
async fn add_duplicate_repository_fails() {
let server = TestServer::start().await;
let payload = json!({
"name": "dup-repo",
Review

[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 | *

**[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 | * <!-- compliance-fp:79404cdc0af0daf79084f8dbc5e22f551dbf6228313b03e1777c21e138df09f7 -->
"git_url": "https://github.com/example/dup-repo.git",
Review

[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 | *

**[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 | * <!-- compliance-fp:9b1045fc19cc41f2cb98ef86e9f014af89c8facaf7713549d12c7ba539c28ccf -->
});
// First add succeeds
let resp = server.post("/api/v1/repositories", &payload).await;
assert_eq!(resp.status(), 200);
// Second add with same git_url should fail (unique index)
let resp = server.post("/api/v1/repositories", &payload).await;
assert_ne!(resp.status(), 200);
server.cleanup().await;
}
#[tokio::test]
async fn delete_repository() {
let server = TestServer::start().await;
// Add a repo
let resp = server
.post(
"/api/v1/repositories",
&json!({
"name": "to-delete",
"git_url": "https://github.com/example/to-delete.git",
}),
)
.await;
let body: serde_json::Value = resp.json().await.unwrap();
let repo_id = body["data"]["id"].as_str().unwrap();
// Delete it
let resp = server
.delete(&format!("/api/v1/repositories/{repo_id}"))
.await;
assert_eq!(resp.status(), 200);
// List should be empty again
let resp = server.get("/api/v1/repositories").await;
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["data"].as_array().unwrap().len(), 0);
server.cleanup().await;
}
#[tokio::test]
async fn delete_nonexistent_repository_returns_404() {
let server = TestServer::start().await;
let resp = server
.delete("/api/v1/repositories/000000000000000000000000")
.await;
assert_eq!(resp.status(), 404);
server.cleanup().await;
}
#[tokio::test]
async fn delete_invalid_id_returns_400() {
let server = TestServer::start().await;
let resp = server.delete("/api/v1/repositories/not-a-valid-id").await;
assert_eq!(resp.status(), 400);
server.cleanup().await;
}

View File

@@ -0,0 +1,111 @@
use crate::common::TestServer;
use serde_json::json;
#[tokio::test]
async fn stats_overview_reflects_inserted_data() {
let server = TestServer::start().await;
// Add a repo
server
.post(
"/api/v1/repositories",
&json!({
"name": "stats-repo",
"git_url": "https://github.com/example/stats-repo.git",
}),
Review

[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 | *

**[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 | * <!-- compliance-fp:21682da07278fb7ae664138e200dd6c7a37beb857124375238def2e8c9056e88 -->
Review

[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] 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 | * <!-- compliance-fp:21682da07278fb7ae664138e200dd6c7a37beb857124375238def2e8c9056e88 -->
Review

[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

**[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* <!-- compliance-fp:63b5369117f23b5d3eb4660e872d0f6e4ad3f19a56842e1412aa1adca30368e8 -->
)
.await;
// Insert findings directly
let mongodb_uri = std::env::var("TEST_MONGODB_URI")
Review

[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 | *

**[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 | * <!-- compliance-fp:aba496fc642f2bb45c202be95dccc4ede8e6ed348653e4238a75f57c5fd18a81 -->
Review

[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] 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* <!-- compliance-fp:b3f0c02251fb2f33c6b0e791505cfdd929da3636ff2c76593e2b1bb09653cc9e -->
Review

[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 | *

**[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 | * <!-- compliance-fp:b62cc126460ba06026d79b5e96396c139d16c8e222d8202291a286524c2e23b7 -->
.unwrap_or_else(|_| "mongodb://root:example@localhost:27017/?authSource=admin".into());
let client = mongodb::Client::with_uri_str(&mongodb_uri).await.unwrap();
Review

[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 | *

**[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 | * <!-- compliance-fp:03cc3a5bd2f00e9db656d0b2b85fe47ae4f6b231c155a87d7ce1a63815b8b93e -->
let db = client.database(&server.db_name());
let now = mongodb::bson::DateTime::now();
Review

[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

**[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* <!-- compliance-fp:d53d897b076d41d5ac92c60fa84f3f8d1b4618cfd68f2e1763f7edbd28f0a4c3 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:a1a9a866622cbd9a3511b0d894a3be2363e618210df0b874a11bdcb5614722ae -->
for (title, severity) in [
("Critical Bug", "critical"),
("High Bug", "high"),
("Medium Bug", "medium"),
("Low Bug", "low"),
] {
db.collection::<mongodb::bson::Document>("findings")
.insert_one(mongodb::bson::doc! {
"repo_id": "test-repo-id",
"fingerprint": format!("fp-{title}"),
Review

[high] Incorrect repository ID used in stats test

In the stats_overview_reflects_inserted_data test, the MongoDB insert operation uses hardcoded "test-repo-id" as the repo_id instead 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 | *

**[high] Incorrect repository ID used in stats test** In the `stats_overview_reflects_inserted_data` test, the MongoDB insert operation uses hardcoded `"test-repo-id"` as the `repo_id` instead 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 | * <!-- compliance-fp:58e393684ec38fb142d980eb7174be895282d0e0fa27cfed101b2a7a7fff61bb -->
Review

[high] Incorrect repository ID used in stats test

In the stats_overview_reflects_inserted_data test, 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, 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 | *

**[high] Incorrect repository ID used in stats test** In the `stats_overview_reflects_inserted_data` test, 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, 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 | * <!-- compliance-fp:58e393684ec38fb142d980eb7174be895282d0e0fa27cfed101b2a7a7fff61bb -->
Review

[high] Incorrect repository ID used in stats test

In the stats_overview_reflects_inserted_data test, 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 | *

**[high] Incorrect repository ID used in stats test** In the `stats_overview_reflects_inserted_data` test, 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 | * <!-- compliance-fp:58e393684ec38fb142d980eb7174be895282d0e0fa27cfed101b2a7a7fff61bb -->
"scanner": "test",
"scan_type": "sast",
"title": title,
"description": "desc",
"severity": severity,
"status": "open",
"created_at": now,
"updated_at": now,
})
.await
.unwrap();
}
let resp = server.get("/api/v1/stats/overview").await;
assert_eq!(resp.status(), 200);
Review

[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 | *

**[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 | * <!-- compliance-fp:81d1a1532274fcd407b66ed10bf64791d1aa8227acdcd6ebfcd680771d7458e5 -->
let body: serde_json::Value = resp.json().await.unwrap();
let data = &body["data"];
assert_eq!(data["repositories"], 1);
assert_eq!(data["total_findings"], 4);
assert_eq!(data["critical"], 1);
assert_eq!(data["high"], 1);
server.cleanup().await;
}
#[tokio::test]
async fn stats_update_after_finding_status_change() {
let server = TestServer::start().await;
// Insert a finding
let 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();
let db = client.database(&server.db_name());
let now = mongodb::bson::DateTime::now();
let result = db
.collection::<mongodb::bson::Document>("findings")
.insert_one(mongodb::bson::doc! {
"repo_id": "repo-1",
"fingerprint": "fp-stats-test",
"scanner": "test",
"scan_type": "sast",
"title": "Stats Test Finding",
"description": "desc",
"severity": "high",
"status": "open",
"created_at": now,
"updated_at": now,
})
.await
.unwrap();
let finding_id = result.inserted_id.as_object_id().unwrap().to_hex();
// Stats should show 1 finding
let resp = server.get("/api/v1/stats/overview").await;
let body: serde_json::Value = resp.json().await.unwrap();
assert_eq!(body["data"]["total_findings"], 1);
// Mark it as resolved
server
.patch(
&format!("/api/v1/findings/{finding_id}/status"),
&json!({ "status": "resolved" }),
)
.await;
// The finding still exists (status changed, not deleted)
let resp = server.get("/api/v1/stats/overview").await;
let body: serde_json::Value = resp.json().await.unwrap();
// total_findings counts all findings regardless of status
assert_eq!(body["data"]["total_findings"], 1);
server.cleanup().await;
}

View File

@@ -1,4 +1,9 @@
// Integration tests for the compliance-agent crate.
// E2E / Integration tests for the compliance-agent API.
Review

[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 | *

**[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-fp:d3092dba2fa76350d44f9b590a28468b619c34ea80a7523ad71ef78d7949a777 -->
Review

[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 | *

**[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-fp:d3092dba2fa76350d44f9b590a28468b619c34ea80a7523ad71ef78d7949a777 -->
Review

[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 | *

**[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 | * <!-- compliance-fp:d3092dba2fa76350d44f9b590a28468b619c34ea80a7523ad71ef78d7949a777 -->
//
// Add tests that exercise the full pipeline, API handlers,
// and cross-module interactions here.
// These tests require a running MongoDB instance. Set TEST_MONGODB_URI
// if it's not at the default `mongodb://root:example@localhost:27017`.
//
// Run with: cargo test -p compliance-agent --test e2e
// Or nightly: (via CI with MongoDB service container)
mod api;

View File

@@ -15,6 +15,30 @@ use crate::parsers::registry::ParserRegistry;
use super::community::detect_communities;
use super::impact::ImpactAnalyzer;
/// Walk up the qualified-name hierarchy to find the closest ancestor
/// that exists in the node map.
///
/// For `"src/main.rs::config::load"` this tries:
/// 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.
Review

[high] Off-by-one error in find_parent_qname when handling qualified names without '::'

The find_parent_qname function incorrectly handles qualified names that do not contain any '::' separator. When current.rfind("::") returns None, it should immediately return None because there's no parent. However, the current implementation enters an infinite loop because current is never updated to an empty string, and current.rfind("::") will always return None on subsequent iterations.

Suggested fix: Fix the loop condition to properly handle the case where there are no '::' separators. The function should return None immediately when rfind("::") returns None.

*Scanner: code-review/logic | *

**[high] Off-by-one error in `find_parent_qname` when handling qualified names without '::'** The `find_parent_qname` function incorrectly handles qualified names that do not contain any '::' separator. When `current.rfind("::")` returns `None`, it should immediately return `None` because there's no parent. However, the current implementation enters an infinite loop because `current` is never updated to an empty string, and `current.rfind("::")` will always return `None` on subsequent iterations. Suggested fix: Fix the loop condition to properly handle the case where there are no '::' separators. The function should return `None` immediately when `rfind("::")` returns `None`. *Scanner: code-review/logic | * <!-- compliance-fp:4f0c8fed46fedb2cd694d79018ff461ca30882bddb2a67889b803bf9400395b4 -->
fn find_parent_qname(qname: &str, node_map: &HashMap<String, NodeIndex>) -> Option<String> {
let mut current = qname.to_string();
Review

[high] Off-by-one error in find_parent_qname when handling empty segments

The find_parent_qname function 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 '::'), calling find_parent_qname("src/main.rs::config::", &node_map) will incorrectly return Some("src/main.rs::") instead of None. This happens because rfind("::") 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 that current is not empty before checking node_map.contains_key(&current). If current becomes empty, immediately return None since there's no valid parent.

*Scanner: code-review/logic | *

**[high] Off-by-one error in `find_parent_qname` when handling empty segments** The `find_parent_qname` function 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 '::'), calling `find_parent_qname("src/main.rs::config::", &node_map)` will incorrectly return `Some("src/main.rs::")` instead of `None`. This happens because `rfind("::")` 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 that `current` is not empty before checking `node_map.contains_key(&current)`. If `current` becomes empty, immediately return `None` since there's no valid parent. *Scanner: code-review/logic | * <!-- compliance-fp:2eb26751c2b51b0992ca797d2171add9abeef5e3b7ec00641b9438058c779075 -->
loop {
// Try stripping the last "::" segment
if let Some(pos) = current.rfind("::") {
current.truncate(pos);
if node_map.contains_key(&current) {
return Some(current);
}
continue;
}
// No more "::" — this is a top-level node (file), no parent
return None;
}
}
/// The main graph engine that builds and manages code knowledge graphs
pub struct GraphEngine {
parser_registry: ParserRegistry,
@@ -89,7 +113,12 @@ impl GraphEngine {
Ok((code_graph, build_run))
}
/// Build petgraph from parsed output, resolving edges to node indices
/// Build petgraph from parsed output, resolving edges to node indices.
///
/// After resolving the explicit edges from parsers, we synthesise
/// `Contains` edges so that every node is reachable from its parent
/// file or module. This eliminates disconnected "islands" that
/// otherwise appear when files share no direct call/import edges.
fn build_petgraph(&self, parse_output: ParseOutput) -> Result<CodeGraph, CoreError> {
let mut graph = DiGraph::new();
let mut node_map: HashMap<String, NodeIndex> = HashMap::new();
@@ -102,15 +131,13 @@ impl GraphEngine {
node_map.insert(node.qualified_name.clone(), idx);
}
// Resolve and add edges — rewrite target to the resolved qualified name
// so the persisted edge references match node qualified_names.
// Resolve and add explicit edges from parsers
let mut resolved_edges = Vec::new();
for mut edge in parse_output.edges {
let source_idx = node_map.get(&edge.source);
let resolved = self.resolve_edge_target(&edge.target, &node_map);
if let (Some(&src), Some(tgt)) = (source_idx, resolved) {
// Update target to the resolved qualified name
let resolved_name = node_map
.iter()
.find(|(_, &idx)| idx == tgt)
@@ -121,7 +148,48 @@ impl GraphEngine {
graph.add_edge(src, tgt, edge.kind.clone());
Review

[medium] Deeply nested control flow in Contains edge synthesis

The Contains edge synthesis logic in build_petgraph has 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 | *

**[medium] Deeply nested control flow in Contains edge synthesis** The Contains edge synthesis logic in `build_petgraph` has 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 | * <!-- compliance-fp:ebb388b8c9a39790be19560562289778152605b8e0efa7c4437eb42fe14eb898 -->
Review

[medium] Deeply nested control flow in Contains edge synthesis

The Contains edge synthesis logic in build_petgraph has 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 | *

**[medium] Deeply nested control flow in Contains edge synthesis** The Contains edge synthesis logic in `build_petgraph` has 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 | * <!-- compliance-fp:ebb388b8c9a39790be19560562289778152605b8e0efa7c4437eb42fe14eb898 -->
resolved_edges.push(edge);
}
// Skip unresolved edges (cross-file, external deps) — conservative approach
}
Review

[medium] Potential panic in Contains edge synthesis due to unwrapping

In the build_petgraph function, 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 accessing nodes.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 | *

**[medium] Potential panic in Contains edge synthesis due to unwrapping** In the `build_petgraph` function, 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 accessing `nodes.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 | * <!-- compliance-fp:8fef57ee461770d6e5a6f05027d6682a43b24a46e96ec186f1217b0521ac7b2c -->
// Synthesise Contains edges: connect each node to its closest
// parent in the qualified-name hierarchy.
//
// For "src/main.rs::config::load", the parent chain is:
// "src/main.rs::config" → "src/main.rs"
//
// We walk up the qualified name (splitting on "::") and link to
// the first ancestor that exists in the node map.
let repo_id = nodes.first().map(|n| n.repo_id.as_str()).unwrap_or("");
let build_id = nodes
.first()
.map(|n| n.graph_build_id.as_str())
.unwrap_or("");
Review

[medium] Potential panic in Contains edge synthesis

In the build_petgraph function, when synthesizing Contains edges, the code directly indexes into node_map with node_map[qname] and node_map[&parent_qname] without checking if keys exist. If find_parent_qname returns 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 that find_parent_qname always returns valid keys that exist in the node_map.

*Scanner: code-review/convention | *

**[medium] Potential panic in Contains edge synthesis** In the `build_petgraph` function, when synthesizing Contains edges, the code directly indexes into `node_map` with `node_map[qname]` and `node_map[&parent_qname]` without checking if keys exist. If `find_parent_qname` returns 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 that `find_parent_qname` always returns valid keys that exist in the node_map. *Scanner: code-review/convention | * <!-- compliance-fp:8dd7e0d4858858437bc1d8ee1aab9a0dc4707d18346582b2b885e696a5588737 -->
let qualified_names: Vec<String> = nodes.iter().map(|n| n.qualified_name.clone()).collect();
let file_paths: HashMap<String, String> = nodes
.iter()
.map(|n| (n.qualified_name.clone(), n.file_path.clone()))
.collect();
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];
Review

[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 | *

**[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 | * <!-- compliance-fp:0a07b5c5e60542e191d085eb18dc095e44d9bb6e67d6ba8198ed436e707b610d -->
Review

[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 via node_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 | *

**[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 via `node_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 | * <!-- compliance-fp:7b8b7aeab65def86d8a4928c52add29e13f3dc20b8300ef4ba84c3239eabda40 -->
// Avoid duplicate edges
if !graph.contains_edge(parent_idx, child_idx) {
graph.add_edge(parent_idx, child_idx, CodeEdgeKind::Contains);
resolved_edges.push(CodeEdge {
id: None,
repo_id: repo_id.to_string(),
graph_build_id: build_id.to_string(),
source: parent_qname,
target: qname.clone(),
kind: CodeEdgeKind::Contains,
file_path: file_paths.get(qname).cloned().unwrap_or_default(),
line_number: None,
});
}
}
}
Ok(CodeGraph {
@@ -132,33 +200,62 @@ impl GraphEngine {
})
Review

[medium] Complex boolean expression in edge resolution

The resolve_edge_target function 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 | *

**[medium] Complex boolean expression in edge resolution** The `resolve_edge_target` function 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 | * <!-- compliance-fp:1df9276a3941f76ed4bfb6e0944d204c3e52011243f491b1b22c00b4e0e75c90 -->
Review

[medium] Complex boolean expression in edge resolution

The resolve_edge_target function 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 | *

**[medium] Complex boolean expression in edge resolution** The `resolve_edge_target` function 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 | * <!-- compliance-fp:1df9276a3941f76ed4bfb6e0944d204c3e52011243f491b1b22c00b4e0e75c90 -->
}
/// Try to resolve an edge target to a known node
/// Try to resolve an edge target to a known node.
///
/// Resolution strategies (in order):
/// 1. Direct qualified-name match
/// 2. Suffix match: "foo" matches "src/main.rs::mod::foo"
/// 3. Module-path match: "config::load" matches "src/config.rs::load"
/// 4. Self-method: "self.method" matches "::method"
fn resolve_edge_target(
&self,
target: &str,
node_map: &HashMap<String, NodeIndex>,
) -> Option<NodeIndex> {
// Direct match
// 1. Direct match
if let Some(idx) = node_map.get(target) {
return Some(*idx);
}
// Try matching just the function/type name (intra-file resolution)
// 2. Suffix match: "foo" → "path/file.rs::foo"
Review

[medium] Inconsistent error handling in edge resolution

The resolve_edge_target function uses multiple resolution strategies but doesn't consistently handle cases where resolution fails. Specifically, the module-path matching strategy has a fallback that uses unwrap_or(target) which could silently ignore resolution failures. Additionally, the function returns None for failed resolutions but the logic around stripped and segments processing 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 | *

**[medium] Inconsistent error handling in edge resolution** The `resolve_edge_target` function uses multiple resolution strategies but doesn't consistently handle cases where resolution fails. Specifically, the module-path matching strategy has a fallback that uses `unwrap_or(target)` which could silently ignore resolution failures. Additionally, the function returns `None` for failed resolutions but the logic around `stripped` and `segments` processing 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 | * <!-- compliance-fp:db291b9f59cd056cf731606233f9adf47c61a007c50358bf7c59a62b24d6ab03 -->
let suffix_pattern = format!("::{target}");
let dot_pattern = format!(".{target}");
for (qualified, idx) in node_map {
// Match "foo" to "path/file.rs::foo" or "path/file.rs::Type::foo"
if qualified.ends_with(&format!("::{target}"))
|| qualified.ends_with(&format!(".{target}"))
{
if qualified.ends_with(&suffix_pattern) || qualified.ends_with(&dot_pattern) {
return Some(*idx);
Review

[medium] Inconsistent error handling in edge resolution

The resolve_edge_target function uses multiple resolution strategies but doesn't consistently handle cases where resolution fails. Specifically, the module-path matching strategy has a fallback that uses unwrap_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 | *

**[medium] Inconsistent error handling in edge resolution** The `resolve_edge_target` function uses multiple resolution strategies but doesn't consistently handle cases where resolution fails. Specifically, the module-path matching strategy has a fallback that uses `unwrap_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-fp:fe0ea8db4d5285f77a7d0255e0c2c805e208c7328ec273c297250ab58f56868c -->
}
}
// Try matching method calls like "self.method" -> look for "::method"
// 3. Module-path match: "config::load" → try matching the last N
// segments of the target against node qualified names.
// This handles cross-file calls like `crate::config::load` or
// `super::handlers::process` where the prefix differs.
if target.contains("::") {
// Strip common Rust path prefixes
let stripped = target
.strip_prefix("crate::")
.or_else(|| target.strip_prefix("super::"))
.or_else(|| target.strip_prefix("self::"))
.unwrap_or(target);
let segments: Vec<&str> = stripped.split("::").collect();
// Try matching progressively shorter suffixes
for start in 0..segments.len() {
let suffix = segments[start..].join("::");
let pattern = format!("::{suffix}");
for (qualified, idx) in node_map {
if qualified.ends_with(&pattern) {
return Some(*idx);
}
}
}
}
// 4. Self-method: "self.method" → "::method"
if let Some(method_name) = target.strip_prefix("self.") {
let pattern = format!("::{method_name}");
for (qualified, idx) in node_map {
if qualified.ends_with(&format!("::{method_name}"))
|| qualified.ends_with(&format!(".{method_name}"))
{
if qualified.ends_with(&pattern) {
return Some(*idx);
}
}
@@ -353,4 +450,83 @@ mod tests {
assert!(code_graph.node_map.contains_key("a::c"));
assert!(code_graph.node_map.contains_key("a::d"));
}
#[test]
fn test_contains_edges_synthesised() {
let engine = GraphEngine::new(1000);
let mut output = ParseOutput::default();
// File → Module → Function hierarchy
output.nodes.push(make_node("src/main.rs"));
output.nodes.push(make_node("src/main.rs::config"));
output.nodes.push(make_node("src/main.rs::config::load"));
let code_graph = engine.build_petgraph(output).unwrap();
Review

[low] Test assertion relies on specific ordering of edges

The test test_contains_edges_synthesised checks 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 | *

**[low] Test assertion relies on specific ordering of edges** The test `test_contains_edges_synthesised` checks 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 | * <!-- compliance-fp:dc29a0cbe654ab4e6cda772de23ffc1bc46b39fd440b495dd484d974570e2030 -->
// Should have 2 Contains edges:
// src/main.rs → src/main.rs::config
// src/main.rs::config → src/main.rs::config::load
let contains_edges: Vec<_> = code_graph
.edges
.iter()
.filter(|e| matches!(e.kind, CodeEdgeKind::Contains))
.collect();
assert_eq!(contains_edges.len(), 2, "expected 2 Contains edges");
let sources: Vec<&str> = contains_edges.iter().map(|e| e.source.as_str()).collect();
assert!(sources.contains(&"src/main.rs"));
assert!(sources.contains(&"src/main.rs::config"));
}
#[test]
fn test_contains_edges_no_duplicates_with_existing_edges() {
let engine = GraphEngine::new(1000);
let mut output = ParseOutput::default();
output.nodes.push(make_node("src/main.rs"));
output.nodes.push(make_node("src/main.rs::foo"));
// Explicit Calls edge (foo calls itself? just for testing)
output.edges.push(CodeEdge {
id: None,
repo_id: "test".to_string(),
graph_build_id: "build1".to_string(),
source: "src/main.rs::foo".to_string(),
target: "src/main.rs::foo".to_string(),
kind: CodeEdgeKind::Calls,
file_path: "src/main.rs".to_string(),
line_number: Some(1),
});
let code_graph = engine.build_petgraph(output).unwrap();
// 1 Calls + 1 Contains = 2 edges total
assert_eq!(code_graph.edges.len(), 2);
}
#[test]
fn test_cross_file_resolution_with_module_path() {
let engine = GraphEngine::new(1000);
let node_map = build_test_node_map(&["src/config.rs::load_config", "src/main.rs::main"]);
// "crate::config::load_config" should resolve to "src/config.rs::load_config"
let result = engine.resolve_edge_target("crate::config::load_config", &node_map);
assert!(result.is_some(), "cross-file crate:: path should resolve");
}
#[test]
fn test_find_parent_qname() {
let node_map = build_test_node_map(&[
"src/main.rs",
"src/main.rs::config",
"src/main.rs::config::load",
]);
assert_eq!(
find_parent_qname("src/main.rs::config::load", &node_map),
Some("src/main.rs::config".to_string())
);
assert_eq!(
find_parent_qname("src/main.rs::config", &node_map),
Some("src/main.rs".to_string())
);
assert_eq!(find_parent_qname("src/main.rs", &node_map), None);
}
}