Skip to content

Address code-base audit findings - #534

Merged
pandalump merged 14 commits into
mainfrom
code-base-audit-2026-06-05
Jun 17, 2026
Merged

Address code-base audit findings#534
pandalump merged 14 commits into
mainfrom
code-base-audit-2026-06-05

Conversation

@lodyai

@lodyai lodyai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Summary

Implements the 2026-06-05 code-base audit refactor plan across the client send path, app outbound encoding, builder transitions, inbound decode/pipeline handling, client pool internals, workspace membership, example/benchmark support, and fragment test helpers.

ExecPlan: docs/execplans/code-base-audit-2026-06-05.md

Review walkthrough

Validation

  • make check-fmt: passed on the final implementation gate.
  • make lint: passed on the final implementation gate.
  • make test: passed on the final implementation gate.
  • make markdownlint: passed after the final ExecPlan close-out update.
  • coderabbit review --agent: completed with zero findings on the final Milestone 7 rerun.

Notes

This pull request remains draft for reviewer triage, but the requested implementation work in the ExecPlan is complete and pushed.

@coderabbitai

coderabbitai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

An error occurred during the review process. Please try again later.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch code-base-audit-2026-06-05

Comment @coderabbitai help to get the list of available commands and usage tips.

@sourcery-ai

sourcery-ai Bot commented Jun 4, 2026

Copy link
Copy Markdown
Contributor

Reviewer's Guide

Introduces a detailed ExecPlan Markdown document that defines the non-code execution plan, constraints, risks, milestones, validation gates, and follow-up issue handling for addressing the 2026-06-05 code-base audit findings, without implementing any of the refactors yet.

Flow diagram for ExecPlan approval gate and milestone loop

flowchart TD
  A[Draft ExecPlan created] --> B[Plan review]
  B --> C{ExecPlan approved?}
  C -- No --> B
  C -- Yes --> D[Select next milestone]

  D --> E[Implement milestone changes]
  E --> F[Run make check-fmt]
  F --> G[Run make lint]
  G --> H[Run make test]
  H --> I[Run coderabbit review --agent]

  I --> J{All gates and CodeRabbit clear?}
  J -- No --> E
  J -- Yes --> K[git commit -F ...]
  K --> L{More milestones?}
  L -- Yes --> D
  L -- No --> M[Run final gates and update draft PR]
Loading

File-Level Changes

Change Details Files
Add a comprehensive ExecPlan document describing how future refactors will address 2026-06-05 code-base audit findings.
  • Document constraints, tolerances, risks, and decision log governing how the refactor work must be executed and validated.
  • Define seven concrete implementation milestones covering client send pipeline, app encoding/inbound policy, app builder transitions, pool internals, workspace membership, example/benchmark helpers, and fragment test helpers.
  • Specify validation workflow for each milestone, including make targets, CodeRabbit review usage, and logging expectations.
  • Outline progress tracking, surprises/discoveries, follow-up GitHub issues, and retrospective/outcome recording requirements.
docs/execplans/code-base-audit-2026-06-05.md

Tips and commands

Interacting with Sourcery

  • Trigger a new review: Comment @sourcery-ai review on the pull request.
  • Continue discussions: Reply directly to Sourcery's review comments.
  • Generate a GitHub issue from a review comment: Ask Sourcery to create an
    issue from a review comment by replying to it. You can also reply to a
    review comment with @sourcery-ai issue to create an issue from it.
  • Generate a pull request title: Write @sourcery-ai anywhere in the pull
    request title to generate a title at any time. You can also comment
    @sourcery-ai title on the pull request to (re-)generate the title at any time.
  • Generate a pull request summary: Write @sourcery-ai summary anywhere in
    the pull request body to generate a PR summary at any time exactly where you
    want it. You can also comment @sourcery-ai summary on the pull request to
    (re-)generate the summary at any time.
  • Generate reviewer's guide: Comment @sourcery-ai guide on the pull
    request to (re-)generate the reviewer's guide at any time.
  • Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
    pull request to resolve all Sourcery comments. Useful if you've already
    addressed all the comments and don't want to see them anymore.
  • Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
    request to dismiss all existing Sourcery reviews. Especially useful if you
    want to start fresh with a new review - don't forget to comment
    @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

  • Enable or disable review features such as the Sourcery-generated pull request
    summary, the reviewer's guide, and others.
  • Change the review language.
  • Add, remove or edit custom review instructions.
  • Adjust other review settings.

Getting Help

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

leynos added 10 commits June 12, 2026 01:10
Describe the gated implementation path for the audit refactors,
including validation, CodeRabbit review, follow-up issue handling,
and the explicit approval gate before code changes.
Extract the shared serialize, request-hook, framed-send, timing, and
error-hook flow into a private client helper. Keep correlation handling in
`send_envelope` and `call_streaming` while routing the repeated transport
pipeline through one path.

Record the milestone validation and review result in the ExecPlan.
Extract shared app outbound encoding and move response send helpers out
of the inbound handler. Route codec-driver sends through the shared
helper while preserving the raw length-delimited response path.

Delegate inbound deserialization failure policy to
`DeserFailureTracker` and split dispatchable envelope construction out
of `handle_frame`, keeping the counter reset after the full inbound
pipeline succeeds.

Add focused response and outbound encoding tests with clearer assertion
helpers, document the new helper boundaries, and record the zero-copy
serializer deferral in ADR 005 and the ExecPlan.
Add a dedicated `WireframeApp` connection-state rebuild helper and use it
from `on_connection_setup` instead of manually reconstructing every app
field.

Document and test the ordering rule: setup changes the connection state
type, so teardown registered for the prior state is cleared and teardown
must be registered after setup when both hooks are required.
Centralize pool lock poison recovery behind a documented helper, route client builder connect paths through into_parts, and replace the pooled lease dispatch macro with an async helper.

The pool recovery path now emits a warning and metric, with tests covering normal and poisoned lock recovery plus observability side effects. The developer guide and ExecPlan document the reusable pool patterns and validation history.
Add wireframe_testing to the explicit workspace members while keeping the root package as the only default member.

Update the workspace manifest integration and BDD tests to assert the helper crate membership, and refresh the developer guide command guidance for root, workspace, verification, and testing-crate workflows.
Move common TCP example runtime setup into examples/support/runtime_bootstrap.rs so ping_pong and packet_enum share tracing, listener, serve-loop, and runtime startup code.

Move codec benchmark helper modules into wireframe_testing::codec_benchmarks and update benches, unit tests, and BDD fixtures to import them through the helper crate instead of path-coupling to tests/common.
Split the shared fragment transport test helper into focused modules for app setup, assertions, configuration, envelope construction, errors, and framed transport while preserving the existing facade import surface.

Document the helper layout in the developer guide and keep the execplan updated with validation and CodeRabbit review outcomes.
Mark the code-base audit execution plan complete after all implementation milestones, validation gates, CodeRabbit reviews, and branch pushes have succeeded.
Reapply the helper crate package-id import and test annotation after
rebasing onto origin/main.

This keeps the direct workspace manifest regression aligned with the BDD fixture and preserves the existing lint policy for assertion-heavy integration tests.
@lodyai
lodyai Bot force-pushed the code-base-audit-2026-06-05 branch from 60ef8e7 to 49ba698 Compare June 11, 2026 23:17
codescene-delta-analysis[bot]

This comment was marked as outdated.

Remove duplicate blank lines so the developer guide passes markdownlint after the rebase.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos

leynos commented Jun 11, 2026

Copy link
Copy Markdown
Owner

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response.

Code Duplication

tests/response.rs: contains 5 functions with similar structure: send_response_framed_propagates_io_error,send_response_framed_propagates_serialization_error,send_response_framed_with_codec_propagates_frame_encoding_error,send_response_framed_with_codec_propagates_serialization_error and 1 more functions

tests/lifecycle.rs: contains 2 functions with similar structure: setup_without_teardown_runs,teardown_without_setup_does_not_run

tests/fixtures/workspace_manifest.rs: contains 2 functions with similar structure: WorkspaceManifestWorld.verify_helper_crate_is_workspace_member,WorkspaceManifestWorld.verify_verification_crate_is_workspace_member

Why does this problem occur?

Duplicated code often leads to code that's harder to change since the same logical change has to be done in multiple functions. More duplication gives lower code health.

How to fix it?

A certain degree of duplicated code might be acceptable. The problems start when it is the same behavior that is duplicated across the functions in the module, ie. a violation of the Don't Repeat Yourself (DRY) principle. DRY violations lead to code that is changed together in predictable patterns, which is both expensive and risky. DRY violations can be identified using CodeScene's X-Ray analysis to detect clusters of change coupled functions with high code similarity. Read More
Once you have identified the similarities across functions, look to extract and encapsulate the concept that varies into its own function(s). These shared abstractions can then be re-used, which minimizes the amount of duplication and simplifies change.

@coderabbitai

This comment was marked as resolved.

@coderabbitai

coderabbitai Bot commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
cat tests/response.rs
echo "=== LIFECYCLE ==="
cat tests/lifecycle.rs

Length of output: 23990



Files and symbols:

  • tests/response.rs: send_response_framed_propagates_serialization_error, send_response_framed_with_codec_propagates_frame_encoding_error, send_response_framed_with_codec_propagates_serialization_error
  • tests/lifecycle.rs: setup_without_teardown_runs, teardown_without_setup_does_not_run

After reading the current code, further extraction is deleterious for both findings. Use the suppression messages below.


Suppression messages

tests/response.rs — three error-path test functions

The structural similarity in these three tests is an irreducible artefact of error-path testing in Rust, not a DRY violation. The shared patterns (construct basic_app(), open a duplex channel, create a Framed, call the method under test, assert the error variant) have already been factored into the module-level helpers basic_app(), assert_io_error(), and assert_serialize_error(). The only remaining repetition is the inescapable test scaffolding that every async error-path test must contain. The two functions that differ solely in which send method they call (send_response_framed vs send_response_framed_with_codec) cannot be further unified without resorting to trait-object async closures or a match-arm dispatch table — both of which obscure which method is under test, degrading diagnostic clarity when a test fails. Collapsing them would trade a cosmetic similarity metric improvement for a real reduction in test legibility and debuggability.

tests/lifecycle.rssetup_without_teardown_runs / teardown_without_setup_does_not_run

These two tests are intentionally parallel in structure because they test complementary guarantees of the same subsystem: that a setup callback fires exactly once on connect, and that a teardown callback does not fire when no connection completes. Their structural similarity is load-bearing: a reader comparing them side-by-side immediately sees the single dimension along which they differ (which builder method is registered and what the expected count is). Extracting a shared run_single_callback_test helper would hide that dimension inside a parameter, forcing the reader to reason about the helper's internals to understand either test. Variable naming and assertion messages are already aligned; there is no actionable duplication remaining.

codescene-delta-analysis[bot]

This comment was marked as outdated.

codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos
leynos marked this pull request as ready for review June 14, 2026 02:23

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry @leynos, your pull request is larger than the review limit of 150000 diff characters

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@leynos

leynos commented Jun 15, 2026

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 15, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@leynos

leynos commented Jun 16, 2026

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@leynos

leynos commented Jun 16, 2026

Copy link
Copy Markdown
Owner

@coderabbitai resume

@coderabbitai

coderabbitai Bot commented Jun 16, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Reviews resumed.

Move response send error-path fixtures and cases into a dedicated module so the main response integration test file stays below the 400-line project constraint.

Record the follow-up review decision in the audit execplan.
codescene-delta-analysis[bot]

This comment was marked as outdated.

@leynos

leynos commented Jun 16, 2026

Copy link
Copy Markdown
Owner

@coderabbitai Please suggest a fix for this issue and supply a prompt for an AI coding agent to enable it to apply the fix. Include the file and symbol names indicated in the issue at the head of your response.

tests/response/response_errors.rs

Comment on lines +214 to +225

async fn send_response_framed_with_codec_propagates_serialization_error() {
    let app = basic_app().expect("failed to create app");
    let (client, _server) = tokio::io::duplex(1024);
    let mut framed = Framed::new(client, app.length_codec());

    let err = app
        .send_response_framed_with_codec(&mut framed, &FailingResp)
        .await
        .expect_err("framed send should fail before transport");

    assert_serialize_error(&err);
}

❌ New issue: Code Duplication
The module contains 3 functions with similar structure: send_response_framed_propagates_serialization_error,send_response_framed_with_codec_propagates_frame_encoding_error,send_response_framed_with_codec_propagates_serialization_error

@coderabbitai

This comment was marked as resolved.

@leynos

leynos commented Jun 17, 2026

Copy link
Copy Markdown
Owner

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@pandalump

Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@pandalump
pandalump merged commit 9f666a6 into main Jun 17, 2026
7 checks passed
@pandalump
pandalump deleted the code-base-audit-2026-06-05 branch June 17, 2026 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants