carverauto/serviceradar

Fork 0

Srql/rust tests #2420

Merged

mfreeman451 merged 4 commits from refs/pull/2420/head into main

2025-11-19 04:08:49 +00:00

mfreeman451 commented

2025-11-18 16:26:22 +00:00

(Migrated from github.com)

Owner

Imported from GitHub pull request.

Original GitHub pull request: #1952
Original author: @mfreeman451
Original URL: https://github.com/carverauto/serviceradar/pull/1952
Original created: 2025-11-18T16:26:22Z
Original updated: 2025-11-19T04:09:21Z
Original head: carverauto/serviceradar:srql/rust_tests
Original base: main
Original merged: 2025-11-19T04:08:49Z by @mfreeman451

User description

IMPORTANT: Please sign the Developer Certificate of Origin

Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include
a DCO sign-off statement indicating the DCO acceptance in one commit message. Here
is an example DCO Signed-off-by line in a commit message:

Signed-off-by: J. Doe <j.doe@domain.com>

Describe your changes

Issue ticket number and link

Code checklist before requesting a review

I have signed the DCO?
The build completes without errors?
All tests are passing when running make test?

PR Type

Tests, Enhancement

Description

Add comprehensive unit tests for SRQL query modules
- cpu_metrics.rs: stats query validation with aggregation and grouping
- interfaces.rs: count aggregation query generation
- logs.rs: multi-field stats query with JSON payload shaping
Refactor query planning logic into standalone functions for testability
- Extract build_query_plan() and determine_limit() from QueryEngine impl
- Enable direct testing without instantiating the full engine
Add integration tests for SRQL DSL end-to-end scenarios
- Device availability filters, discovery sources, service types
- Pagination, ordering, time range resolution
- Validates canonical documentation examples
Create OpenSpec proposal for SRQL API test harness
- Defines requirements for deterministic fixture-based testing
- Outlines tasks for CI integration and contributor documentation

Diagram Walkthrough

flowchart LR
  A["Query Modules<br/>cpu_metrics, interfaces, logs"] -->|"Add unit tests"| B["Module Tests"]
  C["QueryEngine impl"] -->|"Extract functions"| D["build_query_plan<br/>determine_limit"]
  D -->|"Enable testing"| E["Integration Tests"]
  E -->|"Validate DSL"| F["Docs Examples"]
  G["OpenSpec Proposal"] -->|"Defines"| H["Test Harness<br/>Requirements"]

File Walkthrough

Relevant files

Tests

cpu_metrics.rs `Add CPU metrics stats query unit tests` rust/srql/src/query/cpu_metrics.rs Add `#[cfg(test)]` module with `stats_query_matches_cpu_language_reference()` test Test validates AVG aggregation, GROUP BY, and JSON payload structure Helper function `stats_plan()` constructs test `QueryPlan` with time range and filters Asserts generated SQL contains expected clauses and bind parameter count	+46/-0
interfaces.rs `Add interfaces count aggregation unit tests` rust/srql/src/query/interfaces.rs Add `#[cfg(test)]` module with `stats_count_interfaces_emits_count_query()` test Validates count aggregation query generation for interface statistics Helper `stats_plan()` creates test plan with device filter and time range Asserts generated SQL contains COUNT function	+44/-0
logs.rs `Add logs stats query aggregation unit tests` rust/srql/src/query/logs.rs Add `#[cfg(test)]` module with `stats_query_counts_logs_for_service()` test Validates multi-field aggregation (count + group_uniq_array) and JSON shaping Helper `stats_plan()` constructs plan with service name filter and 24-hour time range Asserts SQL contains COUNT, jsonb_agg, and proper JSON object construction	+48/-0

Tests, enhancement

mod.rs

Refactor query planning and add integration tests

rust/srql/src/query/mod.rs

Extract build_query_plan() and determine_limit() as standalone
functions from QueryEngine impl
Update execute_query() and translate_query() to call extracted
functions
Add comprehensive #[cfg(test)] module with 6 integration tests
Tests validate canonical DSL examples: device availability filters,
discovery sources, service types, interfaces IP filtering, pollers
health/status
Helper plan_for() and test_config() support deterministic test
execution
Validates query planning, entity selection, filtering, ordering, and
time range resolution

+223/-33

Documentation

proposal.md `OpenSpec proposal for SRQL API test harness` openspec/changes/add-srql-api-tests/proposal.md Defines motivation: SRQL translator lacks end-to-end regression tests Outlines 4-part solution: API fixture harness, DSL coverage tests, negative-path coverage, translator unit tests Specifies impact: seeded database fixture, new test targets, documentation updates required Emphasizes risk of silent regressions in filters, aggregations, error codes	+15/-0
spec.md `SRQL API test requirements and scenarios` openspec/changes/add-srql-api-tests/specs/srql/spec.md Defines 3 requirements: canonical DSL flow tests, error handling validation, DSL semantics unit tests Specifies 6 scenarios: device filter success, aggregation pagination, invalid DSL 400 response, missing auth 401 response, availability filter alignment, aggregation example queries Each scenario includes GIVEN/WHEN/THEN structure for clarity Ties tests to documentation examples in `docs/docs/srql-language-reference.md`	+40/-0
tasks.md `SRQL API test implementation tasks` openspec/changes/add-srql-api-tests/tasks.md Breaks down implementation into 3 sections: API fixture harness, DSL coverage tests, CI/docs updates Lists 9 concrete tasks with checkboxes for tracking progress Covers fixture dataset definition, test helper implementation, integration tests, error handling, auth validation, unit tests, CI wiring, and contributor documentation	+13/-0

Imported from GitHub pull request. Original GitHub pull request: #1952 Original author: @mfreeman451 Original URL: https://github.com/carverauto/serviceradar/pull/1952 Original created: 2025-11-18T16:26:22Z Original updated: 2025-11-19T04:09:21Z Original head: carverauto/serviceradar:srql/rust_tests Original base: main Original merged: 2025-11-19T04:08:49Z by @mfreeman451 --- ### **User description** ## IMPORTANT: Please sign the Developer Certificate of Origin Thank you for your contribution to ServiceRadar. Please note, when contributing, the developer must include a [DCO sign-off statement]( https://developercertificate.org/) indicating the DCO acceptance in one commit message. Here is an example DCO Signed-off-by line in a commit message: ``` Signed-off-by: J. Doe <j.doe@domain.com> ``` ## Describe your changes ## Issue ticket number and link ## Code checklist before requesting a review - [ ] I have signed the DCO? - [ ] The build completes without errors? - [ ] All tests are passing when running make test? ___ ### **PR Type** Tests, Enhancement ___ ### **Description** - Add comprehensive unit tests for SRQL query modules - `cpu_metrics.rs`: stats query validation with aggregation and grouping - `interfaces.rs`: count aggregation query generation - `logs.rs`: multi-field stats query with JSON payload shaping - Refactor query planning logic into standalone functions for testability - Extract `build_query_plan()` and `determine_limit()` from `QueryEngine` impl - Enable direct testing without instantiating the full engine - Add integration tests for SRQL DSL end-to-end scenarios - Device availability filters, discovery sources, service types - Pagination, ordering, time range resolution - Validates canonical documentation examples - Create OpenSpec proposal for SRQL API test harness - Defines requirements for deterministic fixture-based testing - Outlines tasks for CI integration and contributor documentation ___ ### Diagram Walkthrough ```mermaid flowchart LR A["Query Modules cpu_metrics, interfaces, logs"] -->|"Add unit tests"| B["Module Tests"] C["QueryEngine impl"] -->|"Extract functions"| D["build_query_plan determine_limit"] D -->|"Enable testing"| E["Integration Tests"] E -->|"Validate DSL"| F["Docs Examples"] G["OpenSpec Proposal"] -->|"Defines"| H["Test Harness Requirements"] ``` <details> <summary><h3> File Walkthrough</h3></summary> <table><thead><tr><th></th><th align="left">Relevant files</th></tr></thead><tbody><tr><td>Tests</td><td><table> <tr> <td> <details> <summary>cpu_metrics.rs<dd><code>Add CPU metrics stats query unit tests</code>                                      </dd></summary> <hr> rust/srql/src/query/cpu_metrics.rs <ul><li>Add <code>#[cfg(test)]</code> module with <code>stats_query_matches_cpu_language_reference()</code> test <li> Test validates AVG aggregation, GROUP BY, and JSON payload structure <li> Helper function <code>stats_plan()</code> constructs test <code>QueryPlan</code> with time range and filters <li> Asserts generated SQL contains expected clauses and bind parameter count</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1952/files#diff-400c860c805cd3e1a5e7f7c42cece63cce3e62c1f3fd8b49a5803e26b40fe31e">+46/-0</a>    </td> </tr> <tr> <td> <details> <summary>interfaces.rs<dd><code>Add interfaces count aggregation unit tests</code>                            </dd></summary> <hr> rust/srql/src/query/interfaces.rs <ul><li>Add <code>#[cfg(test)]</code> module with <code>stats_count_interfaces_emits_count_query()</code> test <li> Validates count aggregation query generation for interface statistics <li> Helper <code>stats_plan()</code> creates test plan with device filter and time range <li> Asserts generated SQL contains COUNT function</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1952/files#diff-1ec833d4525fb2806523888b8c57b76ca8dd2ab70539368ccecc4d262769c8c5">+44/-0</a>    </td> </tr> <tr> <td> <details> <summary>logs.rs<dd><code>Add logs stats query aggregation unit tests</code>                            </dd></summary> <hr> rust/srql/src/query/logs.rs <ul><li>Add <code>#[cfg(test)]</code> module with <code>stats_query_counts_logs_for_service()</code> test <li> Validates multi-field aggregation (count + group_uniq_array) and JSON shaping <li> Helper <code>stats_plan()</code> constructs plan with service name filter and 24-hour time range <li> Asserts SQL contains COUNT, jsonb_agg, and proper JSON object construction</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1952/files#diff-f4d3b33667f2e79a6ebe4cfff931f93c728d9a81c305ac13586e623850a504db">+48/-0</a>    </td> </tr> </table></td></tr><tr><td>Tests, enhancement</td><td><table> <tr> <td> <details> <summary>mod.rs<dd><code>Refactor query planning and add integration tests</code>                </dd></summary> <hr> rust/srql/src/query/mod.rs <ul><li>Extract <code>build_query_plan()</code> and <code>determine_limit()</code> as standalone functions from <code>QueryEngine</code> impl <li> Update <code>execute_query()</code> and <code>translate_query()</code> to call extracted functions <li> Add comprehensive <code>#[cfg(test)]</code> module with 6 integration tests <li> Tests validate canonical DSL examples: device availability filters, discovery sources, service types, interfaces IP filtering, pollers health/status <li> Helper <code>plan_for()</code> and <code>test_config()</code> support deterministic test execution <li> Validates query planning, entity selection, filtering, ordering, and time range resolution</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1952/files#diff-393e3fa3d0e41741834cd7cd398a06111ab7b141ae6caca7a5dcc0e036172491">+223/-33</a></td> </tr> </table></td></tr><tr><td>Documentation</td><td><table> <tr> <td> <details> <summary>proposal.md<dd><code>OpenSpec proposal for SRQL API test harness</code>                            </dd></summary> <hr> openspec/changes/add-srql-api-tests/proposal.md <ul><li>Defines motivation: SRQL translator lacks end-to-end regression tests <li> Outlines 4-part solution: API fixture harness, DSL coverage tests, negative-path coverage, translator unit tests <li> Specifies impact: seeded database fixture, new test targets, documentation updates required <li> Emphasizes risk of silent regressions in filters, aggregations, error codes</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1952/files#diff-8478335a646a61fa4a74ea2f9c784cd441632ec92dba1b3f8d5b5fed52c9db3d">+15/-0</a>    </td> </tr> <tr> <td> <details> <summary>spec.md<dd><code>SRQL API test requirements and scenarios</code>                                  </dd></summary> <hr> openspec/changes/add-srql-api-tests/specs/srql/spec.md <ul><li>Defines 3 requirements: canonical DSL flow tests, error handling validation, DSL semantics unit tests <li> Specifies 6 scenarios: device filter success, aggregation pagination, invalid DSL 400 response, missing auth 401 response, availability filter alignment, aggregation example queries <li> Each scenario includes GIVEN/WHEN/THEN structure for clarity <li> Ties tests to documentation examples in <code>docs/docs/srql-language-reference.md</code></ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1952/files#diff-e14559d7bc9392db00b7d08e31b875dd4fbe0eb15538a04ec4e9dbd8b342ad8c">+40/-0</a>    </td> </tr> <tr> <td> <details> <summary>tasks.md<dd><code>SRQL API test implementation tasks</code>                                              </dd></summary> <hr> openspec/changes/add-srql-api-tests/tasks.md <ul><li>Breaks down implementation into 3 sections: API fixture harness, DSL coverage tests, CI/docs updates <li> Lists 9 concrete tasks with checkboxes for tracking progress <li> Covers fixture dataset definition, test helper implementation, integration tests, error handling, auth validation, unit tests, CI wiring, and contributor documentation</ul> </details> </td> <td><a href="https://github.com/carverauto/serviceradar/pull/1952/files#diff-da75158b3b75bf8129cbec0f47cd86dcee1b97f8cccb1b1864c628e11fb83beb">+13/-0</a>    </td> </tr> </table></td></tr></tr></tbody></table> </details> ___

qodo-code-review[bot] commented

2025-11-18 16:27:05 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1952#issuecomment-3548485519
Original created: 2025-11-18T16:27:05Z

PR Compliance Guide 🔍

(Compliance updated until commit `github.com/carverauto/serviceradar@990449075c`)

Below is a summary of compliance checks for this PR:

Security Compliance
⚪	Insecure DB auth Description: The local Postgres bootstrap script disables authentication by setting pg_hba.conf to trust for all hosts (both IPv4 and IPv6), which is unsafe if the container port becomes reachable outside the test harness. harness.rs [37-56] Referred Code const BOOTSTRAP_SCRIPT: &str = r#"set -euo pipefail cat <<'EOS' >/tmp/bootstrap.sh #!/bin/bash set -euo pipefail export PATH=/usr/lib/postgresql/16/bin:$PATH initdb -D /tmp/pgdata >/tmp/initdb.log cat <<'CONF' >> /tmp/pgdata/postgresql.conf shared_preload_libraries = 'timescaledb,age' listen_addresses = '*' max_connections = 50 CONF cat <<'HBA' > /tmp/pgdata/pg_hba.conf host all all 0.0.0.0/0 trust host all all ::/0 trust HBA exec postgres -D /tmp/pgdata -c logging_collector=off EOS chmod +x /tmp/bootstrap.sh exec /tmp/bootstrap.sh "#;
	Unverified image load Description: The harness loads a container image from a local tar archive path (`/opt/cnpg_image.tar`) without verifying integrity or signature, allowing a tampered image to be used if the file is replaced. harness.rs [468-499] Referred Code `fn fetch_cnpg_image() -> anyhow::Result<()> { if load_cnpg_from_archive()? { return Ok(()); } let status = Command::new("docker") .args(["pull", CNPG_IMAGE_REF]) .status()?; if !status.success() { anyhow::bail!( "docker pull {CNPG_IMAGE_REF} failed with status {:?}", status.code() ); } Ok(()) } fn load_cnpg_from_archive() -> anyhow::Result<bool> { let archive = Path::new(CNPG_ARCHIVE); if !archive.exists() { return Ok(false); } ... (clipped 11 lines)`
	Exposed database service Description: The service exposes the CNPG primary via a public LoadBalancer with shared IP annotations and no NetworkPolicy, increasing risk of unauthorized external database access if credentials leak. services.yaml [1-21] Referred Code `apiVersion: v1 kind: Service metadata: name: srql-fixture-rw-ext namespace: srql-fixtures labels: app: srql-fixture annotations: metallb.universe.tf/address-pool: k3s-pool metallb.universe.tf/ip-addr-pool: k3s-pool metallb.universe.tf/allow-shared-ip: serviceradar-public external-dns.alpha.kubernetes.io/hostname: srql-fixture.serviceradar.cloud. spec: type: LoadBalancer selector: cnpg.io/cluster: srql-fixture cnpg.io/instanceRole: primary ports: - name: postgres port: 5432 targetPort: 5432`
	Hardcoded secret placeholders Description: Kubernetes Secret manifest contains placeholder plaintext passwords in version control, which may lead to accidental use or leakage if applied without replacement. cnpg-test-credentials.yaml [1-10] Referred Code `apiVersion: v1 kind: Secret metadata: name: srql-test-db-credentials namespace: srql-fixtures type: Opaque stringData: username: srql # Replace the password before applying. Do not commit real credentials. password: CHANGEME`
	Hardcoded admin secret Description: Admin Secret manifest includes a plaintext placeholder password; committing admin credential templates with defaults risks accidental deployment with weak/known values. cnpg-test-admin-credentials.yaml [1-10] Referred Code `apiVersion: v1 kind: Secret metadata: name: srql-test-admin-credentials namespace: srql-fixtures type: Opaque stringData: username: srql_hydra # Replace the password before applying. Do not commit real credentials. password: CHANGEME_ADMIN`
	Secret exposure via CI env Description: CI workflow exports database URLs (including credentials) via environment variables; if job logs or steps echo these values, secrets could be exposed—masking and least-privilege secrets recommended. main.yml [15-18] Referred Code `BUILDBUDDY_ORG_API_KEY: ${{ secrets.BUILDBUDDY_ORG_API_KEY }} SRQL_TEST_DATABASE_URL: ${{ secrets.SRQL_TEST_DATABASE_URL }} SRQL_TEST_ADMIN_URL: ${{ secrets.SRQL_TEST_ADMIN_URL }}`
	CI secrets handling Description: GitHub Actions workflow passes DB connection secrets as environment variables to jobs; ensure no steps print env, and prefer GitHub Actions secrets with explicit masking and least-privileged DB roles. tests-rust.yml [21-25] Referred Code `env: BUILDBUDDY_ORG_API_KEY: ${{ secrets.BUILDBUDDY_ORG_API_KEY }} SRQL_TEST_DATABASE_URL: ${{ secrets.SRQL_TEST_DATABASE_URL }} SRQL_TEST_ADMIN_URL: ${{ secrets.SRQL_TEST_ADMIN_URL }} strategy:`
	Public router exposure Description: The `router` method was made public, increasing the surface where a misconfigured caller could expose routes without required middleware (e.g., auth); verify all entrypoints enforce authentication. server.rs [48-52] Referred Code `pub fn router(&self) -> Router { Router::new() .route("/healthz", get(Self::health)) .route("/api/query", post(Self::query)) .route("/translate", post(Self::translate))`
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
🔴	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Secrets Exposure: The code logs connection details including database names and hosts and may read secrets from files; ensure no credentials are logged and avoid printing DSNs even in tests. Referred Code match url.parse::<PgConfig>() { Ok(cfg) => { let hosts: Vec<String> = cfg .get_hosts() .iter() .map(\|host\| match host { Host::Tcp(host) => host.to_string(), Host::Unix(path) => format!("unix:{}", path.display()), }) .collect(); let ports = cfg.get_ports(); let port = ports.first().copied().unwrap_or_default(); let user = cfg.get_user().unwrap_or("<unset>"); let dbname = cfg.get_dbname().unwrap_or("<unset>"); eprintln!( "[srql-test] {name}: user={user} db={dbname} hosts={:?} port={}", hosts, port ); } Err(err) => { eprintln!("[srql-test] {name}: failed to parse connection string ({err})"); ... (clipped 3 lines) Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: Action Logging: The new test harness performs critical actions (database resets, seeding, remote lock acquisition) without adding explicit audit logging of who/when/what for these operations. Referred Code `guard .reset_database(&config) .await .expect("failed to reset remote fixture database"); seed_fixture_database(&config.database_url) .await .expect("failed to seed fixture database"); let app_config = test_config(config.database_url.clone()); let server = Server::new(app_config) .await .expect("failed to boot SRQL server for remote harness"); let router = server.router(); let harness = SrqlTestHarness { router, api_key: API_KEY.to_string(), }; test(harness).await; ... (clipped 3 lines)` Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Error Handling: Database bootstrap and extension installation handle some errors but rely on retries and may surface generic errors without contextual logging for all edge cases (e.g., Docker failures, missing fixtures). Referred Code } async fn seed_fixture_database(database_url: &str) -> anyhow::Result<()> { let mut attempts = 0usize; let (client, connection) = loop { match tokio_postgres::connect(database_url, NoTls).await { Ok(parts) => break parts, Err(err) => { if attempts >= DB_CONNECT_RETRIES { return Err(err.into()); } attempts += 1; sleep(TokioDuration::from_millis(DB_CONNECT_DELAY_MS)).await; } } }; tokio::spawn(async move { if let Err(err) = connection.await { eprintln!("fixture database connection closed with error: {err}"); } }); ... (clipped 29 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Sensitive Details: Helper functions print connection parsing errors and detailed paths which could expose internal details when running in CI; confirm these are limited to test context and not user-facing. Referred Code `Err(err) => { eprintln!("[srql-test] {name}: failed to parse connection string ({err})"); } } }` Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Shell Usage: The harness invokes external commands (docker/skopeo) and constructs SQL statements; although inputs are controlled, review quoting and environment use to ensure no injection risk in CI contexts. Referred Code fn ensure_cnpg_image() -> anyhow::Result<()> { let state = CNPG_BUILD_STATE.get_or_init(\|\| Mutex::new(false)); let mut fetched = state .lock() .expect("CNPG image build mutex poisoned by previous panic"); if !fetched { if !cnpg_image_present()? { fetch_cnpg_image()?; } fetched = true; } Ok(()) } fn cnpg_image_present() -> anyhow::Result<bool> { let status = Command::new("docker") .args(["image", "inspect", CNPG_IMAGE_REF]) .stdout(Stdio::null()) .stderr(Stdio::null()) .status()?; Ok(status.success()) ... (clipped 34 lines) Learn more about managing compliance generic rules or creating your own custom rules

Compliance status legend

🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

Previous compliance checks

Compliance check up to commit 6f91b3a

Security Compliance
🟢	No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
⚪	🎫 No ticket provided Create ticket/issue
Codebase Duplication Compliance
⚪	Codebase context is not defined Follow the guide to enable codebase context checks.
Custom Compliance
🟢	Generic: Meaningful Naming and Self-Documenting Code Objective: Ensure all identifiers clearly express their purpose and intent, making code self-documenting Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Error Handling Objective: To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Secure Logging Practices Objective: To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. Status: Passed Learn more about managing compliance generic rules or creating your own custom rules
⚪	Generic: Comprehensive Audit Trails Objective: To create a detailed and reliable record of critical system actions for security analysis and compliance. Status: No audit logs: New functions and tests add query planning and SQL generation behavior without any audit logging of critical actions such as query execution or translation. Referred Code impl QueryEngine { pub fn new(pool: PgPool, config: Arc<AppConfig>) -> Self { Self { pool, config } } pub fn config(&self) -> &AppConfig { &self.config } pub async fn execute_query(&self, request: QueryRequest) -> Result<QueryResponse> { let ast = parser::parse(&request.query)?; let plan = build_query_plan(&self.config, &request, ast)?; let mut conn = self.pool.get().await.map_err(\|err\| { error!(error = ?err, "failed to acquire database connection"); ServiceError::Internal(anyhow::anyhow!("{err:?}")) })?; let results = match plan.entity { Entity::Devices => devices::execute(&mut conn, &plan).await?, Entity::Events => events::execute(&mut conn, &plan).await?, Entity::Interfaces => interfaces::execute(&mut conn, &plan).await?, ... (clipped 68 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Robust Error Handling and Edge Case Management Objective: Ensure comprehensive error handling that provides meaningful context and graceful degradation Status: Limited errors: Added tests assert on SQL strings but do not validate error branches or edge cases like empty stats, invalid fields, or time ranges, leaving robustness unverified in new code paths. Referred Code #[cfg(test)] mod tests { use super::*; use crate::parser::{Entity, Filter, FilterOp, FilterValue}; use chrono::{Duration as ChronoDuration, TimeZone, Utc}; #[test] fn stats_query_counts_logs_for_service() { let plan = stats_plan( r#"count() as total, group_uniq_array(service_name) as services"#, "serviceradar-core", ); let stats_sql = build_stats_query(&plan).expect("stats query should parse"); let stats_sql = stats_sql.expect("stats SQL expected"); let lower = stats_sql.sql.to_lowercase(); assert!( lower.contains("count(") && lower.contains("jsonb_agg"), "unexpected stats SQL: {}", stats_sql.sql ); ... (clipped 26 lines) Learn more about managing compliance generic rules or creating your own custom rules
	Generic: Security-First Input Validation and Data Handling Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities Status: Limit clamping: While limits are clamped, new planning code relies on AST parsing and cursor decoding without added validation in this diff; verify external inputs are fully validated upstream. Referred Code `fn build_query_plan( config: &AppConfig, request: &QueryRequest, ast: QueryAst, ) -> Result<QueryPlan> { let limit = determine_limit(config, request.limit.or(ast.limit)); let offset = request .cursor .as_deref() .map(decode_cursor) .transpose()? .unwrap_or(0) .max(0); let time_range = ast .time_filter .map(\|spec\| spec.resolve(Utc::now())) .transpose()?; Ok(QueryPlan { entity: ast.entity, filters: ast.filters, ... (clipped 14 lines)` Learn more about managing compliance generic rules or creating your own custom rules

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1952#issuecomment-3548485519 Original created: 2025-11-18T16:27:05Z --- ## PR Compliance Guide 🔍  #### (Compliance updated until commit https://github.com/carverauto/serviceradar/commit/990449075ce54ca51a2509419fb43ebe88ee5f7a) Below is a summary of compliance checks for this PR: <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td rowspan=8>⚪</td> <td><details><summary>Insecure DB auth </summary> Description: The local Postgres bootstrap script disables authentication by setting pg_hba.conf to trust for all hosts (both IPv4 and IPv6), which is unsafe if the container port becomes reachable outside the test harness. <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-f15ba86798901e9218b7310fbeae763be61ab6ccf3fe592a79d63eda02eea8b4R37-R56'>harness.rs [37-56]</a> <details open><summary>Referred Code</summary> ```rust const BOOTSTRAP_SCRIPT: &str = r#"set -euo pipefail cat <<'EOS' >/tmp/bootstrap.sh #!/bin/bash set -euo pipefail export PATH=/usr/lib/postgresql/16/bin:$PATH initdb -D /tmp/pgdata >/tmp/initdb.log cat <<'CONF' >> /tmp/pgdata/postgresql.conf shared_preload_libraries = 'timescaledb,age' listen_addresses = '*' max_connections = 50 CONF cat <<'HBA' > /tmp/pgdata/pg_hba.conf host all all 0.0.0.0/0 trust host all all ::/0 trust HBA exec postgres -D /tmp/pgdata -c logging_collector=off EOS chmod +x /tmp/bootstrap.sh exec /tmp/bootstrap.sh "#; ``` </details></details></td></tr> <tr><td><details><summary>Unverified image load </summary> Description: The harness loads a container image from a local tar archive path (<code>/opt/cnpg_image.tar</code>) without verifying integrity or signature, allowing a tampered image to be used if the file is replaced. <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-f15ba86798901e9218b7310fbeae763be61ab6ccf3fe592a79d63eda02eea8b4R468-R499'>harness.rs [468-499]</a> <details open><summary>Referred Code</summary> ```rust fn fetch_cnpg_image() -> anyhow::Result<()> { if load_cnpg_from_archive()? { return Ok(()); } let status = Command::new("docker") .args(["pull", CNPG_IMAGE_REF]) .status()?; if !status.success() { anyhow::bail!( "docker pull {CNPG_IMAGE_REF} failed with status {:?}", status.code() ); } Ok(()) } fn load_cnpg_from_archive() -> anyhow::Result<bool> { let archive = Path::new(CNPG_ARCHIVE); if !archive.exists() { return Ok(false); } ... (clipped 11 lines) ``` </details></details></td></tr> <tr><td><details><summary>Exposed database service </summary> Description: The service exposes the CNPG primary via a public LoadBalancer with shared IP annotations and no NetworkPolicy, increasing risk of unauthorized external database access if credentials leak. <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-13e6849acd10dec57a7dd7e2a55ab32e69ceb6459ea2623019433a9b7d5c50d9R1-R21'>services.yaml [1-21]</a> <details open><summary>Referred Code</summary> ```yaml apiVersion: v1 kind: Service metadata: name: srql-fixture-rw-ext namespace: srql-fixtures labels: app: srql-fixture annotations: metallb.universe.tf/address-pool: k3s-pool metallb.universe.tf/ip-addr-pool: k3s-pool metallb.universe.tf/allow-shared-ip: serviceradar-public external-dns.alpha.kubernetes.io/hostname: srql-fixture.serviceradar.cloud. spec: type: LoadBalancer selector: cnpg.io/cluster: srql-fixture cnpg.io/instanceRole: primary ports: - name: postgres port: 5432 targetPort: 5432 ``` </details></details></td></tr> <tr><td><details><summary>Hardcoded secret placeholders </summary> Description: Kubernetes Secret manifest contains placeholder plaintext passwords in version control, which may lead to accidental use or leakage if applied without replacement. <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-ee52f5c992bee483f8f78735b0c3e026be34b39d5644dd93e4b947c741b3bcebR1-R10'>cnpg-test-credentials.yaml [1-10]</a> <details open><summary>Referred Code</summary> ```yaml apiVersion: v1 kind: Secret metadata: name: srql-test-db-credentials namespace: srql-fixtures type: Opaque stringData: username: srql # Replace the password before applying. Do not commit real credentials. password: CHANGEME ``` </details></details></td></tr> <tr><td><details><summary>Hardcoded admin secret </summary> Description: Admin Secret manifest includes a plaintext placeholder password; committing admin credential templates with defaults risks accidental deployment with weak/known values. <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-17196f71225a4ea5edc7e57d45aec1070df0b69ff0fd7d38c511405ada95133fR1-R10'>cnpg-test-admin-credentials.yaml [1-10]</a> <details open><summary>Referred Code</summary> ```yaml apiVersion: v1 kind: Secret metadata: name: srql-test-admin-credentials namespace: srql-fixtures type: Opaque stringData: username: srql_hydra # Replace the password before applying. Do not commit real credentials. password: CHANGEME_ADMIN ``` </details></details></td></tr> <tr><td><details><summary>Secret exposure via CI env </summary> Description: CI workflow exports database URLs (including credentials) via environment variables; if job logs or steps echo these values, secrets could be exposed—masking and least-privilege secrets recommended. <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-7829468e86c1cc5d5133195b5cb48e1ff6c75e3e9203777f6b2e379d9e4882b3R15-R18'>main.yml [15-18]</a> <details open><summary>Referred Code</summary> ```yaml BUILDBUDDY_ORG_API_KEY: ${{ secrets.BUILDBUDDY_ORG_API_KEY }} SRQL_TEST_DATABASE_URL: ${{ secrets.SRQL_TEST_DATABASE_URL }} SRQL_TEST_ADMIN_URL: ${{ secrets.SRQL_TEST_ADMIN_URL }} ``` </details></details></td></tr> <tr><td><details><summary>CI secrets handling </summary> Description: GitHub Actions workflow passes DB connection secrets as environment variables to jobs; ensure no steps print env, and prefer GitHub Actions secrets with explicit masking and least-privileged DB roles. <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-fe45c58ef3e0ce2e65eb15e70842d134358ff957e727155bcfd21cc09ad38940R21-R25'>tests-rust.yml [21-25]</a> <details open><summary>Referred Code</summary> ```yaml env: BUILDBUDDY_ORG_API_KEY: ${{ secrets.BUILDBUDDY_ORG_API_KEY }} SRQL_TEST_DATABASE_URL: ${{ secrets.SRQL_TEST_DATABASE_URL }} SRQL_TEST_ADMIN_URL: ${{ secrets.SRQL_TEST_ADMIN_URL }} strategy: ``` </details></details></td></tr> <tr><td><details><summary>Public router exposure </summary> Description: The <code>router</code> method was made public, increasing the surface where a misconfigured caller could expose routes without required middleware (e.g., auth); verify all entrypoints enforce authentication. <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-2fc110913a9b9140116cb47742007161b4016b81cc0c325a2b635905c12a3811R48-R52'>server.rs [48-52]</a> <details open><summary>Referred Code</summary> ```rust pub fn router(&self) -> Router { Router::new() .route("/healthz", get(Self::health)) .route("/api/query", post(Self::query)) .route("/translate", post(Self::translate)) ``` </details></details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'>Custom Compliance</td></tr> <tr><td rowspan=1>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=1>🔴</td> <td><details> <summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-f15ba86798901e9218b7310fbeae763be61ab6ccf3fe592a79d63eda02eea8b4R420-R443'>Secrets Exposure</a>: The code logs connection details including database names and hosts and may read secrets from files; ensure no credentials are logged and avoid printing DSNs even in tests. <details open><summary>Referred Code</summary> ```rust match url.parse::<PgConfig>() { Ok(cfg) => { let hosts: Vec<String> = cfg .get_hosts() .iter() .map(|host| match host { Host::Tcp(host) => host.to_string(), Host::Unix(path) => format!("unix:{}", path.display()), }) .collect(); let ports = cfg.get_ports(); let port = ports.first().copied().unwrap_or_default(); let user = cfg.get_user().unwrap_or("<unset>"); let dbname = cfg.get_dbname().unwrap_or("<unset>"); eprintln!( "[srql-test] {name}: user={user} db={dbname} hosts={:?} port={}", hosts, port ); } Err(err) => { eprintln!("[srql-test] {name}: failed to parse connection string ({err})"); ... (clipped 3 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=4>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-f15ba86798901e9218b7310fbeae763be61ab6ccf3fe592a79d63eda02eea8b4R118-R141'>Action Logging</a>: The new test harness performs critical actions (database resets, seeding, remote lock acquisition) without adding explicit audit logging of who/when/what for these operations. <details open><summary>Referred Code</summary> ```rust guard .reset_database(&config) .await .expect("failed to reset remote fixture database"); seed_fixture_database(&config.database_url) .await .expect("failed to seed fixture database"); let app_config = test_config(config.database_url.clone()); let server = Server::new(app_config) .await .expect("failed to boot SRQL server for remote harness"); let router = server.router(); let harness = SrqlTestHarness { router, api_key: API_KEY.to_string(), }; test(harness).await; ... (clipped 3 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-f15ba86798901e9218b7310fbeae763be61ab6ccf3fe592a79d63eda02eea8b4R158-R207'>Error Handling</a>: Database bootstrap and extension installation handle some errors but rely on retries and may surface generic errors without contextual logging for all edge cases (e.g., Docker failures, missing fixtures). <details open><summary>Referred Code</summary> ```rust } async fn seed_fixture_database(database_url: &str) -> anyhow::Result<()> { let mut attempts = 0usize; let (client, connection) = loop { match tokio_postgres::connect(database_url, NoTls).await { Ok(parts) => break parts, Err(err) => { if attempts >= DB_CONNECT_RETRIES { return Err(err.into()); } attempts += 1; sleep(TokioDuration::from_millis(DB_CONNECT_DELAY_MS)).await; } } }; tokio::spawn(async move { if let Err(err) = connection.await { eprintln!("fixture database connection closed with error: {err}"); } }); ... (clipped 29 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-f15ba86798901e9218b7310fbeae763be61ab6ccf3fe592a79d63eda02eea8b4R439-R443'>Sensitive Details</a>: Helper functions print connection parsing errors and detailed paths which could expose internal details when running in CI; confirm these are limited to test context and not user-facing. <details open><summary>Referred Code</summary> ```rust Err(err) => { eprintln!("[srql-test] {name}: failed to parse connection string ({err})"); } } } ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-f15ba86798901e9218b7310fbeae763be61ab6ccf3fe592a79d63eda02eea8b4R445-R499'>Shell Usage</a>: The harness invokes external commands (docker/skopeo) and constructs SQL statements; although inputs are controlled, review quoting and environment use to ensure no injection risk in CI contexts. <details open><summary>Referred Code</summary> ```rust fn ensure_cnpg_image() -> anyhow::Result<()> { let state = CNPG_BUILD_STATE.get_or_init(|| Mutex::new(false)); let mut fetched = state .lock() .expect("CNPG image build mutex poisoned by previous panic"); if !*fetched { if !cnpg_image_present()? { fetch_cnpg_image()?; } *fetched = true; } Ok(()) } fn cnpg_image_present() -> anyhow::Result<bool> { let status = Command::new("docker") .args(["image", "inspect", CNPG_IMAGE_REF]) .stdout(Stdio::null()) .stderr(Stdio::null()) .status()?; Ok(status.success()) ... (clipped 34 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2">   </td></tr></tbody></table> <details><summary>Compliance status legend</summary> 🟢 - Fully Compliant 🟡 - Partial Compliant 🔴 - Not Compliant ⚪ - Requires Further Human Verification 🏷️ - Compliance label </details> ___ #### Previous compliance checks <details> <summary>Compliance check up to commit <a href='https://github.com/carverauto/serviceradar/commit/6f91b3ac92a398be67a5613a88d5565d25795f2d'>6f91b3a</a></summary> <table><tbody><tr><td colspan='2'>Security Compliance</td></tr> <tr><td>🟢</td><td><details><summary>No security concerns identified</summary> No security vulnerabilities detected by AI analysis. Human verification advised for critical code. </details></td></tr> <tr><td colspan='2'>Ticket Compliance</td></tr> <tr><td>⚪</td><td><details><summary>🎫 No ticket provided </summary> - [ ] Create ticket/issue  </details></td></tr> <tr><td colspan='2'>Codebase Duplication Compliance</td></tr> <tr><td>⚪</td><td><details><summary>Codebase context is not defined </summary> Follow the <a href='https://qodo-merge-docs.qodo.ai/core-abilities/rag_context_enrichment/'>guide</a> to enable codebase context checks. </details></td></tr> <tr><td colspan='2'>Custom Compliance</td></tr> <tr><td rowspan=3>🟢</td><td> <details><summary>Generic: Meaningful Naming and Self-Documenting Code</summary> **Objective:** Ensure all identifiers clearly express their purpose and intent, making code self-documenting **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Error Handling</summary> **Objective:** To prevent the leakage of sensitive system information through error messages while providing sufficient detail for internal debugging. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td> <details><summary>Generic: Secure Logging Practices</summary> **Objective:** To ensure logs are useful for debugging and auditing without exposing sensitive information like PII, PHI, or cardholder data. **Status:** Passed > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td rowspan=3>⚪</td> <td><details> <summary>Generic: Comprehensive Audit Trails</summary> **Objective:** To create a detailed and reliable record of critical system actions for security analysis and compliance. **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-393e3fa3d0e41741834cd7cd398a06111ab7b141ae6caca7a5dcc0e036172491R116-R204'>No audit logs</a>: New functions and tests add query planning and SQL generation behavior without any audit logging of critical actions such as query execution or translation. <details open><summary>Referred Code</summary> ```rust impl QueryEngine { pub fn new(pool: PgPool, config: Arc<AppConfig>) -> Self { Self { pool, config } } pub fn config(&self) -> &AppConfig { &self.config } pub async fn execute_query(&self, request: QueryRequest) -> Result<QueryResponse> { let ast = parser::parse(&request.query)?; let plan = build_query_plan(&self.config, &request, ast)?; let mut conn = self.pool.get().await.map_err(|err| { error!(error = ?err, "failed to acquire database connection"); ServiceError::Internal(anyhow::anyhow!("{err:?}")) })?; let results = match plan.entity { Entity::Devices => devices::execute(&mut conn, &plan).await?, Entity::Events => events::execute(&mut conn, &plan).await?, Entity::Interfaces => interfaces::execute(&mut conn, &plan).await?, ... (clipped 68 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Robust Error Handling and Edge Case Management</summary> **Objective:** Ensure comprehensive error handling that provides meaningful context and graceful degradation **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-f4d3b33667f2e79a6ebe4cfff931f93c728d9a81c305ac13586e623850a504dbR557-R603'>Limited errors</a>: Added tests assert on SQL strings but do not validate error branches or edge cases like empty stats, invalid fields, or time ranges, leaving robustness unverified in new code paths. <details open><summary>Referred Code</summary> ```rust #[cfg(test)] mod tests { use super::*; use crate::parser::{Entity, Filter, FilterOp, FilterValue}; use chrono::{Duration as ChronoDuration, TimeZone, Utc}; #[test] fn stats_query_counts_logs_for_service() { let plan = stats_plan( r#"count() as total, group_uniq_array(service_name) as services"#, "serviceradar-core", ); let stats_sql = build_stats_query(&plan).expect("stats query should parse"); let stats_sql = stats_sql.expect("stats SQL expected"); let lower = stats_sql.sql.to_lowercase(); assert!( lower.contains("count(") && lower.contains("jsonb_agg"), "unexpected stats SQL: {}", stats_sql.sql ); ... (clipped 26 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td><details> <summary>Generic: Security-First Input Validation and Data Handling</summary> **Objective:** Ensure all data inputs are validated, sanitized, and handled securely to prevent vulnerabilities **Status:** <a href='https://github.com/carverauto/serviceradar/pull/1952/files#diff-393e3fa3d0e41741834cd7cd398a06111ab7b141ae6caca7a5dcc0e036172491R212-R246'>Limit clamping</a>: While limits are clamped, new planning code relies on AST parsing and cursor decoding without added validation in this diff; verify external inputs are fully validated upstream. <details open><summary>Referred Code</summary> ```rust fn build_query_plan( config: &AppConfig, request: &QueryRequest, ast: QueryAst, ) -> Result<QueryPlan> { let limit = determine_limit(config, request.limit.or(ast.limit)); let offset = request .cursor .as_deref() .map(decode_cursor) .transpose()? .unwrap_or(0) .max(0); let time_range = ast .time_filter .map(|spec| spec.resolve(Utc::now())) .transpose()?; Ok(QueryPlan { entity: ast.entity, filters: ast.filters, ... (clipped 14 lines) ``` </details> > Learn more about managing compliance <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#configuration-options'>generic rules</a> or creating your own <a href='https://qodo-merge-docs.qodo.ai/tools/compliance/#custom-compliance'>custom rules</a> </details></td></tr> <tr><td align="center" colspan="2">  </td></tr></tbody></table> </details>

qodo-code-review[bot] commented

2025-11-18 16:32:37 +00:00

(Migrated from github.com)

Author

Owner

Imported GitHub PR comment.

Original author: @qodo-code-review[bot]
Original URL: https://github.com/carverauto/serviceradar/pull/1952#issuecomment-3548508030
Original created: 2025-11-18T16:32:37Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category Suggestion Impact

High-level

Category	Suggestion	Impact
High-level	Implement tests against a database Replace fragile SQL string-based tests with more robust tests that execute queries against a seeded database to validate actual results, aligning the implementation with the strategy proposed in the PR's own design documents. Examples: rust/srql/src/query/mod.rs [269-274] `let sql = devices::to_debug_sql(&plan).expect("should build SQL for docs query"); assert!( sql.to_lowercase() .contains("\"unified_devices\".\"is_available\" = $3"), "expected SQL to include availability predicate, got: {sql}" );` openspec/changes/add-srql-api-tests/proposal.md [7-10] 1. Build a deterministic SRQL API test harness around the Rust service that spins up against a seeded Postgres/CNPG schema so `/api/query` calls can be executed inside `cargo test` (or equivalent Bazel target). 2. Capture canonical DSL scenarios—inventory filters, aggregations, ordering/limits—and assert that `/api/query` responses (rows + metadata) match golden fixtures so any regression fails fast. 3. Add negative-path coverage for bad DSL, malformed JSON, and auth failures to confirm the API returns 400/401 responses instead of panicking, and publish run instructions so CI and local contributors run the suite. 4. Layer translator-level unit tests that exercise the SRQL language semantics described in `docs/docs/srql-language-reference.md` (filters, entity selectors, sorting, availability flags, etc.) so example queries like `in:devices time:last_7d sort:last_seen:desc limit:20 is_available:true` are locked down independent of the HTTP harness. Solution Walkthrough: Before: `// File: rust/srql/src/query/mod.rs #[test] fn devices_docs_example_available_true() { let query = "in:devices time:last_7d sort:last_seen:desc limit:20 is_available:true"; let plan = plan_for(query); // ... assertions on the plan ... let sql = devices::to_debug_sql(&plan).expect("should build SQL"); assert!( sql.to_lowercase() .contains("\"unified_devices\".\"is_available\" = $3"), "expected SQL to include availability predicate, got: {sql}" ); }` After: // Hypothetical test using a database harness #[tokio::test] async fn devices_docs_example_available_true_returns_correct_data() { // 1. Set up a test database with seeded data let test_db = TestDatabase::with_fixtures(&[ Device { id: "dev-1", is_available: true, ... }, Device { id: "dev-2", is_available: false, ... }, ]).await; // 2. Execute the query against the live service connected to the test DB let query = "in:devices is_available:true"; let response = test_db.execute_query(query).await; // 3. Assert on the actual results, not the SQL assert_eq!(response.results.len(), 1); assert_eq!(response.results[0]["id"], "dev-1"); } Suggestion importance[1-10]: 9 __ Why: The suggestion correctly identifies that the new tests rely on fragile SQL string assertions, and advocates for the more robust database-backed testing strategy already outlined in the PR's own `openspec` proposal.	High
Update

Implement tests against a database

Replace fragile SQL string-based tests with more robust tests that execute
queries against a seeded database to validate actual results, aligning the
implementation with the strategy proposed in the PR's own design documents.

Examples:

rust/srql/src/query/mod.rs [269-274]

        let sql = devices::to_debug_sql(&plan).expect("should build SQL for docs query");
        assert!(
            sql.to_lowercase()
                .contains("\"unified_devices\".\"is_available\" = $3"),
            "expected SQL to include availability predicate, got: {sql}"
        );

openspec/changes/add-srql-api-tests/proposal.md [7-10]

1. Build a deterministic SRQL API test harness around the Rust service that spins up against a seeded Postgres/CNPG schema so `/api/query` calls can be executed inside `cargo test` (or equivalent Bazel target).
2. Capture canonical DSL scenarios—inventory filters, aggregations, ordering/limits—and assert that `/api/query` responses (rows + metadata) match golden fixtures so any regression fails fast.
3. Add negative-path coverage for bad DSL, malformed JSON, and auth failures to confirm the API returns 400/401 responses instead of panicking, and publish run instructions so CI and local contributors run the suite.
4. Layer translator-level unit tests that exercise the SRQL language semantics described in `docs/docs/srql-language-reference.md` (filters, entity selectors, sorting, availability flags, etc.) so example queries like `in:devices time:last_7d sort:last_seen:desc limit:20 is_available:true` are locked down independent of the HTTP harness.

Solution Walkthrough:

Before:

// File: rust/srql/src/query/mod.rs

#[test]
fn devices_docs_example_available_true() {
    let query =
        "in:devices time:last_7d sort:last_seen:desc limit:20 is_available:true";
    let plan = plan_for(query);

    // ... assertions on the plan ...

    let sql = devices::to_debug_sql(&plan).expect("should build SQL");
    assert!(
        sql.to_lowercase()
            .contains("\"unified_devices\".\"is_available\" = $3"),
        "expected SQL to include availability predicate, got: {sql}"
    );
}

After:

// Hypothetical test using a database harness

#[tokio::test]
async fn devices_docs_example_available_true_returns_correct_data() {
    // 1. Set up a test database with seeded data
    let test_db = TestDatabase::with_fixtures(&[
        Device { id: "dev-1", is_available: true, ... },
        Device { id: "dev-2", is_available: false, ... },
    ]).await;

    // 2. Execute the query against the live service connected to the test DB
    let query = "in:devices is_available:true";
    let response = test_db.execute_query(query).await;

    // 3. Assert on the actual results, not the SQL
    assert_eq!(response.results.len(), 1);
    assert_eq!(response.results[0]["id"], "dev-1");
}

Suggestion importance[1-10]: 9

Why: The suggestion correctly identifies that the new tests rely on fragile SQL string assertions, and advocates for the more robust database-backed testing strategy already outlined in the PR's own openspec proposal.

High

Update

Imported GitHub PR comment. Original author: @qodo-code-review[bot] Original URL: https://github.com/carverauto/serviceradar/pull/1952#issuecomment-3548508030 Original created: 2025-11-18T16:32:37Z --- ## PR Code Suggestions ✨  Explore these optional code suggestions: <table><thead><tr><td>Category</td><td align=left>Suggestion                                                                                                                                    </td><td align=center>Impact</td></tr><tbody><tr><td rowspan=1>High-level</td> <td> <details><summary>Implement tests against a database</summary> ___ **Replace fragile SQL string-based tests with more robust tests that execute queries against a seeded database to validate actual results, aligning the implementation with the strategy proposed in the PR's own design documents.** ### Examples: <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1952/files#diff-393e3fa3d0e41741834cd7cd398a06111ab7b141ae6caca7a5dcc0e036172491R269-R274">rust/srql/src/query/mod.rs [269-274]</a> </summary> ```rust let sql = devices::to_debug_sql(&plan).expect("should build SQL for docs query"); assert!( sql.to_lowercase() .contains("\"unified_devices\".\"is_available\" = $3"), "expected SQL to include availability predicate, got: {sql}" ); ``` </details> <details> <summary> <a href="https://github.com/carverauto/serviceradar/pull/1952/files#diff-8478335a646a61fa4a74ea2f9c784cd441632ec92dba1b3f8d5b5fed52c9db3dR7-R10">openspec/changes/add-srql-api-tests/proposal.md [7-10]</a> </summary> ```markdown 1. Build a deterministic SRQL API test harness around the Rust service that spins up against a seeded Postgres/CNPG schema so `/api/query` calls can be executed inside `cargo test` (or equivalent Bazel target). 2. Capture canonical DSL scenarios—inventory filters, aggregations, ordering/limits—and assert that `/api/query` responses (rows + metadata) match golden fixtures so any regression fails fast. 3. Add negative-path coverage for bad DSL, malformed JSON, and auth failures to confirm the API returns 400/401 responses instead of panicking, and publish run instructions so CI and local contributors run the suite. 4. Layer translator-level unit tests that exercise the SRQL language semantics described in `docs/docs/srql-language-reference.md` (filters, entity selectors, sorting, availability flags, etc.) so example queries like `in:devices time:last_7d sort:last_seen:desc limit:20 is_available:true` are locked down independent of the HTTP harness. ``` </details> ### Solution Walkthrough: #### Before: ```markdown // File: rust/srql/src/query/mod.rs #[test] fn devices_docs_example_available_true() { let query = "in:devices time:last_7d sort:last_seen:desc limit:20 is_available:true"; let plan = plan_for(query); // ... assertions on the plan ... let sql = devices::to_debug_sql(&plan).expect("should build SQL"); assert!( sql.to_lowercase() .contains("\"unified_devices\".\"is_available\" = $3"), "expected SQL to include availability predicate, got: {sql}" ); } ``` #### After: ```markdown // Hypothetical test using a database harness #[tokio::test] async fn devices_docs_example_available_true_returns_correct_data() { // 1. Set up a test database with seeded data let test_db = TestDatabase::with_fixtures(&[ Device { id: "dev-1", is_available: true, ... }, Device { id: "dev-2", is_available: false, ... }, ]).await; // 2. Execute the query against the live service connected to the test DB let query = "in:devices is_available:true"; let response = test_db.execute_query(query).await; // 3. Assert on the actual results, not the SQL assert_eq!(response.results.len(), 1); assert_eq!(response.results[0]["id"], "dev-1"); } ``` <details><summary>Suggestion importance[1-10]: 9</summary> __ Why: The suggestion correctly identifies that the new tests rely on fragile SQL string assertions, and advocates for the more robust database-backed testing strategy already outlined in the PR's own `openspec` proposal. </details></details></td><td align=center>High </td></tr> <tr><td align="center" colspan="2"> - [ ] Update  </td><td></td></tr></tbody></table>