Repository navigation
Fix app and server lint findings - #716
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
Summary
The author reports that validation passed on commit The assembly timeout fixture comes from prerequisite PR WalkthroughThe pull request marks several constructors and accessors as ChangesApplication and server API
Concurrent store test updates
Priority: ⬇️ Low Change: Refactor Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Testing (Overall) |  |
The new invalid-frame test is substantive. It uses a real duplex stream and a decoder error, then checks the returned InvalidData error and the warning context. However, the pull request adds compil… |
Add a compile-time pass test, registered with the existing UI/trybuild tests, that calls the changed public app and envelope APIs from const contexts and checks their resulting values. Keep the invalid-frame regression test, and add a serve… |
| User-Facing Documentation |  |
Document the new public API capability. The PR changes public WireframeApp::codec, WireframeApp::fragmentation, WireframeApp::memory_budgets, Envelope::new, and PacketParts::new from `pub fn… |
Add a clear note to docs/users-guide.md that the affected public methods are const fn and can be used in constant evaluation where their arguments and surrounding value construction support it. Include a short example for the public con… |
| Developer Documentation | |
The pull request changes internal APIs and abstractions without updating docs/developers-guide.md. It adds the crate-private handle_unprepared_connection_result path and routes handle_connection… |
Update docs/developers-guide.md with the internal connection-handling path, its compatibility relationship to the deprecated public methods, and the direct AppFactory/Preamble server-boundary requirements. Update the relevant design d… |
| Testing (Compile-Time / Ui) | |
The pull request introduces compile-time API behaviour by changing public WireframeApp::codec, WireframeApp::fragmentation, WireframeApp::memory_budgets, Envelope::new, and PacketParts::new … |
Add a trybuild pass fixture such as tests/ui/app_const_api.rs, register it in tests/compile_error.rs, and exercise the changed public methods from const functions or const values. Cover the const constructors and the builder methods, pl… |
-
Check name Status Explanation Docstring Coverage Docstring coverage is 89.09% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 55 functions across 19 files. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Module-Level Documentation Pass the module-level documentation check. The authoritative diff changes 19 existing Rust files and adds no module declarations. Every changed file-level module has a leading //!docstring in both …Testing (Unit And Behavioural) Accept the testing coverage. The PR adds an async behavioural test that drives the full inbound stream path with a real Tokio duplex stream, verifies InvalidDatapropagation, and checks the warning …Testing (Property / Proof) Pass this check. The reviewed diff introduces no new invariant over a broad input range, state space, ordering, or transition set. It mainly changes function constness, local names, equivalent control… Unit Architecture Pass. The reviewed diff does not introduce a Unit Architecture violation. The changes are lint-oriented renames, const fnannotations, equivalent struct-update syntax, direct trait bounds, and test …Domain Architecture The pull request does not introduce a domain architecture violation. The authoritative diff contains lint-oriented refactors, naming changes, const fndeclarations, equivalent control-flow rewrites,…Observability Treat this check as passed. The diff is lint and naming cleanup plus const fnand equivalent struct-update changes. The connection handler routing changes from the deprecated public wrapper to `hand…Title check The title directly describes the main change: fixing selected lint findings in app and server code. No roadmap item or issue reference is required by the provided context. Description check The description clearly explains the lint fixes, behavioural constraints, regression tests, validation commands, and related pull requests. It is directly related to the changeset.
Full details: Testing (Overall)
-
Explanation
The new invalid-frame test is substantive. It uses a real duplex stream and a decoder error, then checks the returned
InvalidDataerror and the warning context. However, the pull request adds compile-time API functionality by changing public methods such asWireframeApp::codec,fragmentation,memory_budgets,Envelope::new, andPacketParts::newtoconst fn. No test covers these methods in a const context. The existingclient_const_apitest covers only client APIs, so it would still pass if these newconstqualifiers were removed.Resolution
Add a compile-time pass test, registered with the existing UI/trybuild tests, that calls the changed public app and envelope APIs from const contexts and checks their resulting values. Keep the invalid-frame regression test, and add a server-level error-path test if the direct
handle_unprepared_connection_resulttest is not intended to cover the changed server caller.
Full details: User-Facing Documentation
-
Explanation
Document the new public API capability. The PR changes public
WireframeApp::codec,WireframeApp::fragmentation,WireframeApp::memory_budgets,Envelope::new, andPacketParts::newfrompub fntopub const fn, which enables constant-context use. The authoritative diff contains no documentation changes.docs/users-guide.mddocuments these APIs but contains noconst fn, compile-time, or constant-context guidance.Resolution
Add a clear note to
docs/users-guide.mdthat the affected public methods areconst fnand can be used in constant evaluation where their arguments and surrounding value construction support it. Include a short example for the public constructors, and explain any limits forWireframeAppbuilder values.
Full details: Developer Documentation
-
Explanation
The pull request changes internal APIs and abstractions without updating
docs/developers-guide.md. It adds the crate-privatehandle_unprepared_connection_resultpath and routeshandle_connectionand the server connection spawner through it. It also removes the privateWireframeFactoryandWireframePreamblehelper abstractions and appliesAppFactoryandPreambledirectly. The changed-file inventory contains no developer-guide, design-document, ADR, roadmap, or execplan update. Existing guide text describes the broader prepared-application model, but it does not document the new internal handler path or the changed server binding abstraction.Resolution
Update
docs/developers-guide.mdwith the internal connection-handling path, its compatibility relationship to the deprecated public methods, and the directAppFactory/Preambleserver-boundary requirements. Update the relevant design document or add an ADR addendum if this routing or boundary change is intended as an architectural decision. Keep any affected roadmap or execplan status current.
Full details: Testing (Compile-Time / Ui)
-
Explanation
The pull request introduces compile-time API behaviour by changing public
WireframeApp::codec,WireframeApp::fragmentation,WireframeApp::memory_budgets,Envelope::new, andPacketParts::newtoconst fn. The existing trybuild suite covers only client const APIs;tests/compile_error.rsandtests/ui/client_const_api.rsare unchanged, and the only added test is a runtime inbound-handler test. No compile-time test verifies the new app APIs. The warning test uses focused semantic assertions, so a snapshot is not required for that output.Resolution
Add a trybuild pass fixture such as
tests/ui/app_const_api.rs, register it intests/compile_error.rs, and exercise the changed public methods from const functions or const values. Cover the const constructors and the builder methods, plus thecodecaccessor through a const function accepting a borrowedWireframeApp. Keep assertions focused on stable values and run the compile-time test target.
Mark constructors const; let values rest.
Name each binding to make its role clear.
Keep frame budgets on their same course.
Route unprepared streams through the new path.
Check each bad frame and warning.
Let the server workers start in step.
Comment @coderabbitai help to get the list of available commands.
Reviewer's GuideThis lint-remediation PR makes targeted constness, naming, type-expression, and visibility changes across app and server code, removes private trait aliases and lint suppressions, and preserves connection framing, timeout, compatibility, and error-reporting behavior with a new invalid-frame regression test. Sequence diagram for compatibility connection error handlingsequenceDiagram
participant Server as Server connection spawner
participant App as WireframeApp
participant Handler as Internal connection handler
participant Stream as Connection stream
Server->>App: build()
Server->>App: handle_unprepared_connection_result(stream)
App->>Handler: process connection
Handler->>Stream: decode and frame input
alt invalid frame
Stream-->>Handler: framing error
Handler-->>App: io::Result error
App-->>Server: error
Server->>Server: warn connection task error
else valid frames
Handler-->>App: Ok(())
App-->>Server: success
end
Sequence diagram for preamble timeout handlingsequenceDiagram
participant Server as Connection spawner
participant Timeout as Tokio timeout
participant Stream as TCP stream
participant Preamble as read_preamble
Server->>Timeout: timeout(limit, read_preamble(stream))
Timeout->>Preamble: read_preamble(stream)
alt preamble completes
Preamble-->>Timeout: Result preamble and leftover
Timeout-->>Server: preamble result
else timeout expires
Timeout-->>Server: elapsed error
Server->>Server: timeout_error()
Server-->>Server: return decode error
end
Flow diagram for per-connection codec and assembly stateflowchart TD
A[Accepted connection] --> B[clone frame codec]
B --> C[Create CombinedCodec]
C --> D[Create Framed stream]
D --> E[Reassemble fragments]
E --> F[Assemble message]
F --> G[Reset deserialization failure counter]
F --> H[Propagate assembly error]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-access[bot]](https://avatars.githubusercontent.com/in/3755875?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
@coderabbitai review |
|
Make app and server accessors const where supported, name reused bindings distinctly, and simplify optional and fallible paths without changing their results. Remove private trait aliases and deprecation suppression by routing internal callers through a documented private connection handler. Add a regression test that verifies invalid-frame errors and warning context through that handler. Preserve the assembly timeout fixture already stabilized in the prerequisite layer.
Add a trybuild pass case for the const app accessors and packet constructors. Document the const capability and the retained compatibility handler and server trait bounds so the public and internal contracts stay clear.
486b3e4 to
89063bf
Compare
There was a problem hiding this comment.
Gates Passed
See analysis details in CodeScene
Absence of Expected Change Pattern
- wireframe/src/server/config/binding.rs is usually changed with: wireframe/src/server/config/preamble.rs
Quality Gate Profile: Pay Down Tech Debt
Install CodeScene MCP: safeguard and uplift AI-generated code. Catch issues early with our IDE extension and CLI tool.
|
Please reconcile the pre-merge rows in CodeRabbit's walkthrough against PR #716's current candidate: base
The current candidate passed sequential Please confirm whether these rows are now satisfied on the stated head, or identify any remaining defect. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
base=3f144ddf0358891c36eeed80525ce1478d974028
head=89063bfc0d87583a31eb7aa0746867c045c72312
git rev-parse HEAD
git cat-file -t "$base"
git cat-file -t "$head"
gh api repos/leynos/wireframe/issues/comments/5808435728 --jq .body
git diff --stat "$base" "$head"
for file in tests/ui/app_const_api.rs tests/compile_error.rs src/app/inbound_handler/mod.rs src/app/inbound_handler/tests.rs src/server/connection_spawner.rs; do
ast-grep outline "$file" --items all || true
done
git diff "$base" "$head" -- tests/ui/app_const_api.rs tests/compile_error.rs docs/users-guide.md docs/developers-guide.md src/server/connection_spawner.rs src/app/inbound_handler/mod.rs src/app/inbound_handler/tests.rsLength of output: 39392 🏁 Script executed: #!/bin/bash
set -euo pipefail
for file in src/server/mod.rs src/server/config/binding.rs src/app/builder/codec.rs src/app/builder/config.rs src/app/envelope.rs; do
ast-grep outline "$file" --items all || true
done
printf '\n--- Const fixture and registration ---\n'
nl -ba tests/ui/app_const_api.rs
sed -n '1,65p' tests/compile_error.rs
printf '\n--- Guide context ---\n'
sed -n '125,180p' docs/users-guide.md
sed -n '260,325p' docs/developers-guide.md
printf '\n--- Prepared handler at base ---\n'
git show 3f144ddf0358891c36eeed80525ce1478d974028:src/server/connection_spawner.rs | sed -n '57,110p'
printf '\n--- Current handler and regression ---\n'
sed -n '138,205p' src/app/inbound_handler/mod.rs
sed -n '121,165p' src/app/inbound_handler/tests.rs
printf '\n--- Factory and preamble contracts ---\n'
rg -n -A50 -B5 'pub trait (AppFactory|Preamble)\b' src/server
sed -n '1,150p' src/server/config/binding.rs
printf '\n--- Public const API diff ---\n'
git diff 3f144ddf0358891c36eeed80525ce1478d974028 89063bfc0d87583a31eb7aa0746867c045c72312 -- src/app/builder/codec.rs src/app/builder/config.rs src/app/envelope.rsLength of output: 30234
Discard the optional prepared-server error-path test request for this candidate. The stated base already contains the prepared handoff and its error warning. This candidate only renames the local handoff binding at that call site. My earlier description that this PR routes the server through the unprepared handler was incorrect. No remaining defect was found in these rows. This conclusion comes from source and diff inspection. I did not rerun the validation commands or verify hosted checks. The walkthrough still displays the older assessment; this reply does not update those generated statuses. Thanks for clarifying the prepared-server baseline. ✏️ Learnings added
You are interacting with an AI system. |
|
@coderabbitai approve Published head: 89063bf; base: 3f144dd. The latest completed review round is a focused current-head reconciliation. CodeRabbit's reply #6194494808 marked all four pre-merge rows satisfied. It also confirmed the optional prepared-server error-path request is stale for this base: the prepared handoff and warning already exist at the base commit. Earlier substantive review findings from the old candidate were: compile-time API coverage, user-facing documentation, and developer documentation. The current head adds the registered compile-pass fixture and both guide updates. The invalid-frame regression checks error propagation and warning context. CodeRabbit reported no remaining defect in those rows. The old CHANGES_REQUESTED submission is bound to 486b3e4, not this published head. Pre-merge checks: required CI: run 38750600889 passed. CodeQL run 38750587961 passed across Actions, Python, and Rust. CodeScene result 7971147, Gecko Security, Dependabot configuration, and CodeQL status checks passed; remaining optional checks are skipped. Rebase/stack state: #716 targets Please approve if those findings are resolved. If approval remains blocked, identify the specific unresolved finding or missing required inspection. |
1 similar comment
|
@coderabbitai approve Published head: 89063bf; base: 3f144dd. The latest completed review round is a focused current-head reconciliation. CodeRabbit's reply #6194494808 marked all four pre-merge rows satisfied. It also confirmed the optional prepared-server error-path request is stale for this base: the prepared handoff and warning already exist at the base commit. Earlier substantive review findings from the old candidate were: compile-time API coverage, user-facing documentation, and developer documentation. The current head adds the registered compile-pass fixture and both guide updates. The invalid-frame regression checks error propagation and warning context. CodeRabbit reported no remaining defect in those rows. The old CHANGES_REQUESTED submission is bound to 486b3e4, not this published head. Pre-merge checks: required CI: run 38750600889 passed. CodeQL run 38750587961 passed across Actions, Python, and Rust. CodeScene result 7971147, Gecko Security, Dependabot configuration, and CodeQL status checks passed; remaining optional checks are skipped. Rebase/stack state: #716 targets Please approve if those findings are resolved. If approval remains blocked, identify the specific unresolved finding or missing required inspection. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
|
@pandalump The completed reconciliation identified no remaining defect in the four pre-merge rows at The approval workflow will check current permissions and review readiness before submitting approval. 🧠 Learnings used✅ Action performedChanges approved. Review threads were not changed. |
|
@wafflecat-df12 The completed reconciliation found no remaining defect in the four pre-merge rows at The comment chain reports that approval already succeeded. For this repeated request, the approval workflow will check current permissions and review readiness. 🧠 Learnings used✅ Action performedCodeRabbit has already approved this PR, so no new approval was submitted. Review threads were not changed. |
Summary
This branch fixes the selected app and server lint findings while preserving connection, framing and timeout behaviour. It makes supported accessors and constructors constant, names cloned state distinctly, removes private trait aliases and their visibility suppressions, and keeps the deprecated public connection entrypoint by routing internal callers through a documented private handler. Tests cover invalid-frame error propagation and warning context, plus compile-time use of the const app APIs.
This source-fix layer follows client test PR #712 at
a6f363c3dfdf6b756c4596f9abaf76893b565bbfand is the parent of connection/fragment PR #717. The assembly timeout fixture is inherited from prerequisite PR #714; this branch does not repeat its change.Review walkthrough
Validation
89063bfc0d87583a31eb7aa0746867c045c72312, sequentialmake check-fmt,make lint(rustdoc, Clippy and Whitaker),make typecheck,make test,make markdownlintandmake nixiepassed. The test run includes the new trybuild compile-pass case. Logs are under/tmp/*-38f85142-6fec-4b04-a025-f87573dec93b-harden-lint-app-server.out.486b3e4eb9a223ecd4b1b8599445b2416588d897, not this head. They found the pinned Concordatrust-makefile-baselineandmarkdown-formatting-baselineaudits compliant.rust-build-defaultsretained the recorded BD-001/BD-002 conflict between ambient defaults and the selected explicit dev-fast policy (Concordat #153);main-owned-codescene-coverageretained the old pinned CV-005 result addressed upstream by Concordat #203. The selected canon revision was8a2c99cc36532a8e6db6b1cc366966dc761c6444.Notes
Other Rust source findings remain assigned to later narrow waves. This PR does not enable the final lint configuration.
References