Design protocol-agnostic wireframe_testing harness lifecycle - #614
Design protocol-agnostic wireframe_testing harness lifecycle#614leynos wants to merge 5 commits into
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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:
WalkthroughDocument RFC 0001, proposing a protocol-agnostic server lifecycle API, connector helper, compatibility wrappers, verification plan, and roadmap entries for ChangesTest harness lifecycle RFC
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 4❌ Failed checks (4 warnings)
✅ Passed checks (16 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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.
55ec0d9 to
a3174c0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a3174c00e1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub fn local_addr(&self) -> SocketAddr; | ||
| pub async fn shutdown(&mut self) -> TestResult<()>; |
There was a problem hiding this comment.
When a reader or future doctest compiles this rust,no_run block, Rust rejects these bodyless methods and the bodyless free functions below because declarations ending in ; are not permitted in inherent impl blocks or at module scope; no_run prevents execution but still requests compilation. Add placeholder bodies such as unimplemented!() or mark the deliberately schematic block as ignored/plaintext so the RFC does not advertise invalid Rust as a compilable example.
AGENTS.md reference: AGENTS.md:L27-L30
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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 `@docs/contents.md`:
- Around line 89-90: Update the “Testing helpers” entry in docs/contents.md to
describe the implemented in-process server and client pair helpers provided by
the wireframe_testing companion crate, replacing the proposal-oriented “Design
and public API” wording.
In `@docs/rfcs/0001-protocol-agnostic-test-harness-lifecycle.md`:
- Around line 14-15: Update the [pair-plan] reference in the RFC to target
12-3-2-in-process-server-and-client-pair-test-harness.md instead of the
incorrect 17-3-2 document.
- Around line 1-466: Reduce RFC 0001 below the repository’s 400-line Markdown
limit by moving detailed verification and release-sequencing content from
sections “7. Verification plan” and “8. Migration and release sequencing” into
cohesive supporting documentation, while retaining only the essential
requirements and recommendation in the RFC. Update any affected references so
the shortened RFC remains coherent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 85b5b7dd-5f78-42d7-85f7-5eca469f1f4c
📒 Files selected for processing (3)
docs/contents.mddocs/rfcs/0001-protocol-agnostic-test-harness-lifecycle.mddocs/roadmap.md
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
leynos/rust-prover-tools(auto-detected)leynos/shared-actions(auto-detected)leynos/whitaker(auto-detected)
| - [Testing helpers](wireframe-testing-crate.md) Design and public API for the | ||
| `wireframe_testing` companion crate. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Describe wireframe_testing as an existing companion crate.
Replace “Design and public API” with a description of the implemented in-process testing helpers. The current wording makes the entry sound like a proposal rather than a usable companion crate.
Proposed wording
-- [Testing helpers](wireframe-testing-crate.md) Design and public API for the
- `wireframe_testing` companion crate.
+- [Testing helpers](wireframe-testing-crate.md) In-process server and client
+ pair helpers provided by the `wireframe_testing` companion crate.🤖 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 `@docs/contents.md` around lines 89 - 90, Update the “Testing helpers” entry in
docs/contents.md to describe the implemented in-process server and client pair
helpers provided by the wireframe_testing companion crate, replacing the
proposal-oriented “Design and public API” wording.
Source: MCP tools
| <!-- markdownlint-disable-next-line MD013 --> | ||
| [pair-plan]: ../execplans/17-3-2-in-process-server-and-client-pair-test-harness.md |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Correct the execution-plan reference.
Point [pair-plan] at the existing 12-3-2-in-process-server-and-client-pair-test-harness.md document; the current 17-3-2 target does not match the repository’s execution-plan path.
Proposed fix
-[pair-plan]: ../execplans/17-3-2-in-process-server-and-client-pair-test-harness.md
+[pair-plan]: ../execplans/12-3-2-in-process-server-and-client-pair-test-harness.md🤖 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 `@docs/rfcs/0001-protocol-agnostic-test-harness-lifecycle.md` around lines 14 -
15, Update the [pair-plan] reference in the RFC to target
12-3-2-in-process-server-and-client-pair-test-harness.md instead of the
incorrect 17-3-2 document.
Source: MCP tools
Mark the schematic Rust blocks in RFC 0001 as `ignore` so the RFC no longer advertises bodyless declarations as compilable `no_run` examples. Strengthen the verification plan: require an explicit `trybuild` compile-time UI test for the retained pair helpers and misconfigured lifecycle calls, and add `proptest`-based coverage for the shutdown-idempotence, readiness, and concurrent-handle invariants, reserving exhaustive proof for genuine lemmas. Rewrite the `docs/contents.md` "Testing helpers" entry to describe the implemented in-process server and client pair helpers rather than a proposal. Reflect the new property and compile-time test requirements in roadmap items 17.3.3 and 17.3.4. The suggested [pair-plan] change to `12-3-2-...` is skipped: only `17-3-2-in-process-server-and-client-pair-test-harness.md` exists, so the current reference is correct and the proposed target would break the link. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
1 participant
Summary
wireframe_testingserver lifecycle17.3.3through17.3.5Why
The existing
WireframePairis a useful convenience for Wireframe's default client, but it fixes the client codec, preamble typestate, and builder return type. It also constructs the server internally. Downstream protocols therefore cannot combine the shared listener, readiness, task ownership, and shutdown machinery with their own codec, typed preamble, server hooks, or client.The mxd behavioural-harness design in leynos/mxd#401 supplies the first concrete downstream case. It needs Hotline framing and a Hotline preamble while retaining Wireframe's real loopback server lifecycle.
The review also found a separate release-quality gap: the workspace's default member and current Make targets do not execute the companion crate's tests and doctests explicitly. Issue #578 records resulting doctest drift.
Proposed design
The RFC introduces three layers:
spawn_wireframe_serverandspawn_wireframe_server_onaccept a fully configured, unboundWireframeServerand return a non-genericRunningWireframeServerafter listener binding and readiness.spawn_wireframe_server_and_connectcomposes that handle with any caller-supplied asynchronous client connector and cleans up the server when connection fails.WireframePair,spawn_wireframe_pair, andspawn_wireframe_pair_defaultAPIs remain source-compatible convenience wrappers for the default client.The running handle owns bounded, idempotent shutdown and task joining. The connector returns the caller-owned protocol client separately because an arbitrary client may require asynchronous logout, ordinary drop, or no explicit close operation.
The RFC also requires package-specific all-feature tests and doctests in local and Continuous Integration quality gates before publishing the extension.
Roadmap
17.3.3: extract the protocol-neutral running-server lifecycle17.3.4: add the custom connector and prove it with a non-default codec and typed preamble17.3.5: makewireframe_testingan explicit quality-gate target and repair wireframe_testing doctests fail to compile; crate's own tests never run in CI #578Compatibility
This is a design-only, additive proposal. It changes no Rust code and does not deprecate the existing pair helpers.
Validation
The three changed Markdown files pass:
Result: 0 errors.
The review-feedback commit was additionally gated in a local checkout.
make check-fmt,make typecheck,make test,make markdownlint, andmake nixieall pass.make lintreports pre-existingno_expect_outside_testsfindings in test-support helpers (for examplesrc/client/tests/helpers.rs,src/client/tests/request_hooks.rs) that are untouched by this branch and identical onmain;main's tip passed CI with the same pinned whitaker0.2.6, so these are a local whitaker toolchain/staging drift rather than a regression introduced here. Fixing them belongs to the whitaker test-wave rollout, not this documentation-only change.Review feedback addressed
ignoreso the RFC no longer advertises bodyless declarations as compilableno_runexamples.docs/contents.md"Testing helpers" entry to describe the implemented in-process server and client pair helpers.trybuildcompile-time UI test andproptest-based coverage of the shutdown, readiness, and concurrent-handle invariants; reflected both in roadmap items17.3.3/17.3.4.[pair-plan]change to12-3-2-…: only17-3-2-in-process-server-and-client-pair-test-harness.mdexists, so the current reference is correct and the proposed target would break the link.Open implementation questions
TestErrorvariant rather than a contextual messageReferences