More coverage - #86
Conversation
📝 WalkthroughWalkthroughThe changes add integration tests for agent registry and TLS behavior, expose the WebSocket server’s TLS module and helper APIs, and make the WebGPU WIT trimming helper public with fixture-based validation. ChangesAgent registry validation
WebSocket TLS exposure
WebGPU WIT trimming validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
|
Overall Grade |
Security Reliability Complexity Hygiene Coverage |
Code Review Summary
| Analyzer | Status | Updated (UTC) | Details |
|---|---|---|---|
| C# | Jul 14, 2026 9:19p.m. | Review ↗ | |
| C & C++ | Jul 14, 2026 9:19p.m. | Review ↗ | |
| Docker | Jul 14, 2026 9:19p.m. | Review ↗ | |
| Java | Jul 14, 2026 9:19p.m. | Review ↗ | |
| JavaScript | Jul 14, 2026 9:19p.m. | Review ↗ | |
| Python | Jul 14, 2026 9:19p.m. | Review ↗ | |
| Rust | Jul 14, 2026 9:19p.m. | Review ↗ | |
| Secrets | Jul 14, 2026 9:19p.m. | Review ↗ | |
| Code coverage | Jul 14, 2026 9:41p.m. | Review ↗ |
Code Coverage Summary
| Language | Line Coverage (New Code) | Line Coverage (Overall) |
|---|---|---|
| Aggregate | 100% |
54.8% [▲ up 1.3% from main] |
| Python | - | 54.2% |
| Rust | 100% |
54.8% [▲ up 1.3% 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.
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Pull Request Overview
This PR successfully increases test coverage for registry persistence, TLS management, and WIT file processing, with all primary acceptance criteria addressed through implemented test scenarios. Codacy reports that the changes are up to standards with no new quality issues.
There is a medium-severity concern regarding a misleading function signature in utilities/int-gen/src/wit/upstream.rs that should be addressed to reduce caller complexity. Additionally, while the tests are functional, the use of hardcoded relative paths in the WebGPU fixture tests may lead to instability in certain CI environments.
About this PR
- This PR lacks a description. Providing context on the coverage goals and the specific scenarios being addressed is necessary for long-term maintainability and tracking.
Test suggestions
- AgentRegistry save/load round-trip and session persistence check
- AgentRegistry reconnection and session lookup logic
- AgentRegistry loading from a missing file
- TLS certificate generation, disk storage, and rustls configuration build
- WIT trimming logic verification against a real fixture
Low confidence findings
- The test 'strips_webgpu_wit_and_reemits_the_package' uses a hardcoded path to a file in the 'generated/' directory. This may cause failures in restricted CI environments or if the execution context changes. Consider using a more robust method to locate test fixtures.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| reason = "inputs are trusted (upstream WIT or the committed fixture); a parse failure is a bug, so unwrap" | ||
| )] | ||
| fn strip_webgpu(raw: &str) -> Result<String, Error> { | ||
| pub fn strip_webgpu(raw: &str) -> Result<String, Error> { |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: The function signature is misleading because it never returns an Err. All parsing and encoding failures are handled by panicking, so wrapping the result in a Result adds unnecessary complexity for callers.
Try running the following prompt in your coding agent:
Simplify
strip_webgpuinutilities/int-gen/src/wit/upstream.rsto returnStringinstead ofResult<String, Error>, update the return statement at the end of the function to remove theOk()wrapper, and remove theclippy::unwrap_in_resultexpectation from the function's attribute block.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@libs/edge-toolkit/tests/registry.rs`:
- Around line 45-50: Update agent_record_with_pending_builder_replaces_the_map
to initialize a non-empty pending-message map, replace it with a distinct
non-empty map through with_pending_direct_messages, and assert
pending_direct_messages exactly matches the replacement map. Ensure the test
would fail if the builder ignores its argument or retains the original map.
- Around line 33-38: Update the persistence test around AgentRegistry::load so
it first verifies the reloaded registry contains the agent’s persisted record
using the available record lookup API, or serialized YAML when no lookup exists.
Keep the existing agent_session assertion separately to confirm sessions remain
unpersisted.
In `@utilities/int-gen/tests/webgpu_trim.rs`:
- Around line 17-25: Add a negative assertion in the strip_webgpu test after
producing out, checking that a known declaration or method from
WEBGPU_DROP_METHODS present in the fixture is absent from the trimmed output.
Keep the existing assertions for retained package and resource content
unchanged, and use the known dropped member rather than an arbitrary string.
🪄 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: b1412a61-a4e1-41f2-89ad-393216ac07b2
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
libs/edge-toolkit/tests/registry.rsservices/ws-server/Cargo.tomlservices/ws-server/src/lib.rsservices/ws-server/src/main.rsservices/ws-server/src/tls.rsservices/ws-server/tests/tls.rsutilities/int-gen/src/wit/upstream.rsutilities/int-gen/tests/webgpu_trim.rs
| // Persist and reload. Sessions are #[serde(skip)], so they return as None, but the agent survives. | ||
| let dir = tempdir().unwrap(); | ||
| let path = dir.path().join("registry.yaml"); | ||
| registry.save(&path).unwrap(); | ||
| let reloaded = AgentRegistry::<String>::load(&path).unwrap(); | ||
| assert_eq!(reloaded.agent_session(&agent_id), None, "sessions are not persisted"); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the reload assertion prove that the record was persisted.
reloaded.agent_session(&agent_id) == None is also true for an entirely empty registry, so this does not verify that the agent survived the save/load round trip. Assert a persisted field through the registry's record lookup API, or inspect the serialized YAML if no such API exists; keep the session assertion separately.
🤖 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 `@libs/edge-toolkit/tests/registry.rs` around lines 33 - 38, Update the
persistence test around AgentRegistry::load so it first verifies the reloaded
registry contains the agent’s persisted record using the available record lookup
API, or serialized YAML when no lookup exists. Keep the existing agent_session
assertion separately to confirm sessions remain unpersisted.
| #[test] | ||
| fn agent_record_with_pending_builder_replaces_the_map() { | ||
| let record = AgentRecord::<String>::new(AgentConnectionState::Disconnected, None, None) | ||
| .with_pending_direct_messages(BTreeMap::new()); | ||
| assert!(record.pending_direct_messages.is_empty()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Exercise replacement with a non-empty pending-message map.
The newly constructed record and the replacement map are both empty, so this test passes even if with_pending_direct_messages ignores its argument or fails to replace an existing map. Seed a non-empty map, then replace it with a distinct map and assert the exact result.
🤖 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 `@libs/edge-toolkit/tests/registry.rs` around lines 45 - 50, Update
agent_record_with_pending_builder_replaces_the_map to initialize a non-empty
pending-message map, replace it with a distinct non-empty map through
with_pending_direct_messages, and assert pending_direct_messages exactly matches
the replacement map. Ensure the test would fail if the builder ignores its
argument or retains the original map.
| let out = strip_webgpu(&raw).unwrap(); | ||
| assert!( | ||
| out.contains("package wasi:webgpu"), | ||
| "re-emitted WIT should still declare the wasi:webgpu package, got:\n{out}" | ||
| ); | ||
| // The trimmer keeps the compute resources (e.g. gpu-device) while dropping cross-package glue. | ||
| assert!( | ||
| out.contains("resource gpu-device"), | ||
| "expected the gpu-device resource to survive, got:\n{out}" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert that trimming removes an unwanted declaration.
These assertions only verify retained content, so a no-op implementation could still pass. Add a negative assertion for a known member from WEBGPU_DROP_METHODS that exists in the fixture but must be absent from out.
🤖 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 `@utilities/int-gen/tests/webgpu_trim.rs` around lines 17 - 25, Add a negative
assertion in the strip_webgpu test after producing out, checking that a known
declaration or method from WEBGPU_DROP_METHODS present in the fixture is absent
from the trimmed output. Keep the existing assertions for retained package and
resource content unchanged, and use the known dropped member rather than an
arbitrary string.
Summary by CodeRabbit
New Features
Tests