General tidy up - #101
Conversation
📝 WalkthroughWalkthroughThe PR adds an architecture-specific macOS deployment target, expands documentation-rule coverage, revises storage comments, and introduces regression tests for modules, WebSocket routes, health responses, TLS helpers, and registry persistence. ChangesmacOS build configuration
Documentation rule scope
Service regression tests
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Checkov (3.3.8)config/ast-grep/rules/doc-summary-ends-with-period.yamlTraceback (most recent call last): Comment |
 |
|
Overall Grade |
Security Reliability Complexity Hygiene Coverage  |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# |  |
Jul 22, 2026 11:23a.m. | Review ↗ |
| C & C++ | |
Jul 22, 2026 11:23a.m. | Review ↗ |
| Docker | |
Jul 22, 2026 11:23a.m. | Review ↗ |
| Java | |
Jul 22, 2026 11:23a.m. | Review ↗ |
| JavaScript | |
Jul 22, 2026 11:23a.m. | Review ↗ |
| Python | |
Jul 22, 2026 11:23a.m. | Review ↗ |
| Rust | |
Jul 22, 2026 11:23a.m. | Review ↗ |
| Secrets | |
Jul 22, 2026 11:23a.m. | Review ↗ |
| Code coverage | |
Jul 22, 2026 11:49a.m. | Review ↗ |
Code Coverage Summary
| Language | Line Coverage (Overall) |
|---|---|
| Aggregate | 58.8% [▲ up 0.4% from main] |
| Python | 84.3% |
| Rust | 57% [▲ up 0.4% from main] |
➟ Additional coverage metrics may have been reported. See full coverage report ↗
Important
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
Not up to standards ⛔🔴 Issues
|
| Category | Results |
|---|---|
| Complexity | 1 medium |
🟢 Metrics 4 complexity · 0 duplication
Metric Results Complexity 4 Duplication 0
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@services/modules/tests/configure_missing_root.rs`:
- Around line 1-2: The Rust doc summaries wrap before their terminating
punctuation. Rewrap the first-line summaries in
services/modules/tests/configure_missing_root.rs (lines 1-2),
services/modules/tests/get_module_file.rs (lines 1-4),
services/ws-server/tests/configure_app.rs (lines 1-2), and
services/ws/tests/load_registry.rs (lines 1-3) so each summary’s first line ends
with punctuation.
In `@services/modules/tests/get_module_file.rs`:
- Around line 5-6: Remove the crate-level cfg(feature = "openapi-spec") gate
from the integration test target in get_module_file.rs so it is never silently
disabled. Configure the test target or CI invocation explicitly to enable the
openapi-spec feature and ensure this regression test runs during normal
validation.
In `@services/ws-server/tests/health.rs`:
- Line 16: Update the health response assertions in
services/ws-server/tests/health.rs lines 16-16 and
services/ws-server/tests/configure_app.rs lines 31-33 to compare the response
status with StatusCode::OK instead of using is_success(), ensuring both tests
require exactly HTTP 200.
In `@services/ws-server/tests/tls.rs`:
- Around line 45-48: Update the test around load_tls_certs to create a valid
certificate/key pair in the temporary directory, remove only the certificate
file, and then invoke load_tls_certs with both paths. Keep the key present so
the assertion specifically exercises the missing-certificate read path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f2e91242-a81b-4677-868e-ef4656176c2e
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.mise/config.macos.tomlconfig/ast-grep/rules/doc-summary-ends-with-period.yamlservices/modules/tests/configure_missing_root.rsservices/modules/tests/get_module_file.rsservices/storage/src/routes.rsservices/storage/tests/put.rsservices/ws-server/tests/configure_app.rsservices/ws-server/tests/health.rsservices/ws-server/tests/tls.rsservices/ws/Cargo.tomlservices/ws/tests/load_registry.rs
| //! Covers `configure`'s fail-fast panic when `config.root` names a module that isn't among the | ||
| //! scanned `config.paths` -- a misconfiguration that must surface at startup, not as a silent 404. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the first-line Rust doc summaries across the new test files.
Each file wraps its summary before punctuation, violating the same documentation rule.
services/modules/tests/configure_missing_root.rs#L1-L2: rewrap the summary so Line 1 ends with punctuation.services/modules/tests/get_module_file.rs#L1-L4: rewrap the summary so Line 1 ends with punctuation.services/ws-server/tests/configure_app.rs#L1-L2: rewrap the summary so Line 1 ends with punctuation.services/ws/tests/load_registry.rs#L1-L3: rewrap the summary so Line 1 ends with punctuation.
📍 Affects 4 files
services/modules/tests/configure_missing_root.rs#L1-L2(this comment)services/modules/tests/get_module_file.rs#L1-L4services/ws-server/tests/configure_app.rs#L1-L2services/ws/tests/load_registry.rs#L1-L3
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/modules/tests/configure_missing_root.rs` around lines 1 - 2, The
Rust doc summaries wrap before their terminating punctuation. Rewrap the
first-line summaries in services/modules/tests/configure_missing_root.rs (lines
1-2), services/modules/tests/get_module_file.rs (lines 1-4),
services/ws-server/tests/configure_app.rs (lines 1-2), and
services/ws/tests/load_registry.rs (lines 1-3) so each summary’s first line ends
with punctuation.
Source: Coding guidelines
| #![cfg(test)] | ||
| #![cfg(feature = "openapi-spec")] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not silently disable this integration test with a feature gate.
#![cfg(feature = "openapi-spec")] removes the entire test target when the feature is absent, allowing normal test runs to pass without exercising this regression. Move feature selection to explicit test-target/CI configuration and run the target with openapi-spec enabled.
As per coding guidelines, tests under tests/ must never be conditionally disabled without explicit approval.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/modules/tests/get_module_file.rs` around lines 5 - 6, Remove the
crate-level cfg(feature = "openapi-spec") gate from the integration test target
in get_module_file.rs so it is never silently disabled. Configure the test
target or CI invocation explicitly to enable the openapi-spec feature and ensure
this regression test runs during normal validation.
Source: Coding guidelines
| let req = test::TestRequest::get().uri("/health").to_request(); | ||
| let resp = test::call_service(&app, req).await; | ||
|
|
||
| assert!(resp.status().is_success()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Pin both health tests to HTTP 200.
Both assertions accept any 2xx response, so they would miss a regression from 200 to another successful status.
services/ws-server/tests/health.rs#L16-L16: assertStatusCode::OKinstead ofis_success().services/ws-server/tests/configure_app.rs#L31-L33: assertStatusCode::OKinstead ofis_success().
📍 Affects 2 files
services/ws-server/tests/health.rs#L16-L16(this comment)services/ws-server/tests/configure_app.rs#L31-L33
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/ws-server/tests/health.rs` at line 16, Update the health response
assertions in services/ws-server/tests/health.rs lines 16-16 and
services/ws-server/tests/configure_app.rs lines 31-33 to compare the response
status with StatusCode::OK instead of using is_success(), ensuring both tests
require exactly HTTP 200.
| let dir = tempdir().unwrap(); | ||
| let cert = dir.path().join("missing-cert.pem"); | ||
| let key = dir.path().join("missing-key.pem"); | ||
| drop(load_tls_certs(&cert, &key)); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Isolate the missing-certificate case.
Both cert and key are absent, so this test only proves that some missing file causes failure. Generate a valid pair, delete only cert, and then call load_tls_certs so the certificate-read path is specifically covered.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@services/ws-server/tests/tls.rs` around lines 45 - 48, Update the test around
load_tls_certs to create a valid certificate/key pair in the temporary
directory, remove only the certificate file, and then invoke load_tls_certs with
both paths. Keep the key present so the assertion specifically exercises the
missing-certificate read path.
2 participants
Summary by CodeRabbit
Build Improvements
Bug Fixes
Tests
Documentation