Convert Root Manifest to Hybrid Workspace (10.1.1 & 15.1.1 complete)#520
Merged
Convert Root Manifest to Hybrid Workspace (10.1.1 & 15.1.1 complete)#520
Conversation
Contributor
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughDescribe the root Cargo.toml conversion into a hybrid workspace, add dev-deps for test helpers, extend developer docs and an ExecPlan, add BDD + rstest workspace-manifest tests and helpers, and refactor internal helpers and test utilities (Default impls, error construction, logging recovery). Changes
Poem
🚥 Pre-merge checks | ✅ 7✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
Reviewer's GuideConverts the repo into a hybrid Cargo workspace for roadmap item 10.1.1, adds regression and BDD tests plus fixtures around workspace semantics, and refactors various helpers (message sequencing, testing harness, logging, observability, frame conversion, and memory budget checks) to support the staged rollout while updating developer and verification docs and marking the roadmap milestone complete. Sequence diagram for message sequence validation in MessageSeriessequenceDiagram
actor Test
participant Series as MessageSeries
Test->>Series: validate_and_advance_sequence(incoming, is_last)
alt sequence_tracking is Untracked
Series->>Series: start_sequence_tracking(incoming, is_last)
Series->>Series: advance_sequence_or_overflow(incoming, is_last)
alt overflow and not is_last
Series-->>Test: Err(SequenceOverflow)
else ok
Series-->>Test: Ok(())
end
else sequence_tracking is Tracked
Series->>Series: advance_tracked_sequence(incoming, is_last)
Series->>Series: compute expected from next_sequence
alt incoming < expected
Series-->>Test: Err(DuplicateFrame)
else incoming != expected
Series-->>Test: Err(SequenceMismatch)
else incoming == expected
Series->>Series: advance_sequence_or_overflow(incoming, is_last)
alt overflow and not is_last
Series-->>Test: Err(SequenceOverflow)
else ok
Series-->>Test: Ok(())
end
end
end
Updated class diagram for MessageSeries sequence trackingclassDiagram
class MessageSeries {
FrameSequence next_sequence
SequenceTracking sequence_tracking
MessageKey message_key
+validate_and_advance_sequence(incoming: FrameSequence, is_last: bool) Result~(), MessageSeriesError~
-advance_sequence_or_overflow(incoming: FrameSequence, is_last: bool) Result~(), MessageSeriesError~
-start_sequence_tracking(incoming: FrameSequence, is_last: bool) Result~(), MessageSeriesError~
-advance_tracked_sequence(incoming: FrameSequence, is_last: bool) Result~(), MessageSeriesError~
}
class SequenceTracking {
<<enum>>
Untracked
Tracked
}
class FrameSequence {
u64 value
+checked_increment() Option~FrameSequence~
}
class MessageSeriesError {
<<enum>>
SequenceOverflow
DuplicateFrame
SequenceMismatch
}
class MessageKey
MessageSeries --> SequenceTracking
MessageSeries --> FrameSequence
MessageSeries --> MessageSeriesError
MessageSeries --> MessageKey
MessageSeriesError ..> FrameSequence
Updated class diagram for testing utilities and loggingclassDiagram
class Logger {
+start() Logger
+pop() Option~String~
+clear()
}
class LoggerHandle {
-MutexGuard~Logger~ guard
+new() LoggerHandle
+clear()
+pop() Option~String~
+default() LoggerHandle
}
class ObservabilityHandle {
+new() ObservabilityHandle
+codec_error_counter(error_type: &str, recovery_policy: &str) u64
+default() ObservabilityHandle
}
class PendingServer {
-Option~ServerShutdownHandle~ inner
+take() Option~ServerShutdownHandle~
}
class ServerShutdownHandle {
<<typealias>>
oneshot_Sender_unit
JoinHandle_Result_unit_ServerError
}
class ServerError
LoggerHandle --> Logger
ObservabilityHandle --> Logger : uses_for_metrics
PendingServer --> ServerShutdownHandle
ServerShutdownHandle --> ServerError
Updated class diagram for frame conversion utilitiesclassDiagram
class Endianness {
<<enum>>
Big
Little
}
class FrameConversion {
+checked_prefix_cast~T~(len: usize): Result~T, io_Error~
+bytes_to_u64(bytes: &[u8], size: usize, endianness: Endianness): Result~u64, io_Error~
-fill_buf_with_prefix(buf: &mut [u8; 8], prefix: &[u8], endianness: Endianness)
}
class io_Error
FrameConversion --> Endianness
FrameConversion --> io_Error
Flow diagram for workspace manifest regression testsflowchart TD
A[Start workspace_manifest tests] --> B[Load workspace_manifest fixtures]
B --> C[Spawn hybrid workspace test world]
C --> D[Invoke cargo metadata via helper]
D --> E{Metadata matches<br>staged contract?}
E -->|Yes| F[Assert root package is default member only]
E -->|No| G[Fail tests with diagnostic]
F --> H[Run additional rstest-bdd steps]
H --> I{All behavioural<br>assertions pass?}
I -->|Yes| J[Workspace manifest tests pass]
I -->|No| K[Workspace manifest tests fail]
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![codescene-delta-analysis[bot]](https://avatars.githubusercontent.com/in/53921?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for the following test failure and provide an AI coding agent prompt for the fix: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for the following lint violations and provide an AI coding agent prompt for the fix: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for the following concern and provide an AI coding agent prompt for the fix: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for the following lint violations and provide an AI coding agent prompt for the fix: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please suggest a fix for the following test failures and provide an AI coding agent prompt for the fix: Please also suggest a fix for these not being caught by the agent. |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai Please investigate and identify a fix for the following test failure and provide an AI coding agent prompt for the fix: |
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. ❌ Failed checks (3 warnings)
|
This comment was marked as resolved.
This comment was marked as resolved.
Introduced a three-helper split in message series sequence tracking: - `start_sequence_tracking()` handles untracked-to-tracked transition, - `advance_tracked_sequence()` validates duplicates, gaps, and order, - `advance_sequence_or_overflow()` manages sequence increment and overflow. Added comprehensive property-based and unit tests for sequence logic. Also added `fill_buf_with_prefix` helper with tests in frame conversion and a type alias for server shutdown handle in wireframe testing crate. Documentation updated accordingly to explain new helpers and internals. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…ts for sequence handling Replaced proptest-based property tests with explicit unit tests covering advance_sequence_or_overflow, start_sequence_tracking, and advance_tracked_sequence behaviors. This change improves test clarity and maintainability by targeting key sequence handling cases such as normal increments, overflow conditions, duplicate detection, and sequence mismatches. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
leynos
changed the title
…ement and sequence tracking Add unit tests for prefix-buffer placement across endianness cases in frame/conversion.rs and for sequence tracking, overflow, and ordering helpers in message_assembler/series.rs. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Owner
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Owner
Author
|
@coderabbitai Have the following failed checks now been resolved? If further work is required, please provide an AI agent prompt for the remaining work to be done to address these failures. ❌ Failed checks (3 warnings)
|
This comment was marked as resolved.
This comment was marked as resolved.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
tests/fixtures/workspace_manifest.rs (1)
166-173:⚠️ Potential issue | 🟡 MinorDrop the remaining substring match on Cargo package IDs.
Remove
member.contains(VERIFICATION_PACKAGE_NAME). Thepackagesarray already gives exact package names, so this extra check depends on Cargo's opaque pkgid formatting and can fail for the wrong reason.♻️ Proposed simplification
pub fn verify_verification_crate_is_absent(&self) -> TestResult { let metadata = self.metadata_json()?; - let workspace_members = Self::metadata_string_array(&metadata, "workspace_members")?; - if workspace_members.iter().any(|member| { - member - .as_str() - .is_some_and(|member| member.contains(VERIFICATION_PACKAGE_NAME)) - }) || Self::packages_include_name(&metadata, VERIFICATION_PACKAGE_NAME)? + if Self::packages_include_name(&metadata, VERIFICATION_PACKAGE_NAME)? { return Err( "verification crate should not join the workspace until roadmap item 10.1.2".into(), ); }🔍 Verification script
#!/bin/bash set -euo pipefail cargo metadata --no-deps --format-version 1 > /tmp/workspace-metadata.json python - <<'PY' import json with open('/tmp/workspace-metadata.json', 'r', encoding='utf-8') as fh: metadata = json.load(fh) print("workspace_members:") for member in metadata["workspace_members"]: print(f" - {member}") print("\npackage names:") for name in sorted(pkg["name"] for pkg in metadata["packages"]): print(f" - {name}") PY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/fixtures/workspace_manifest.rs` around lines 166 - 173, The code in verify_verification_crate_is_absent currently does a substring match against workspace_members using member.contains(VERIFICATION_PACKAGE_NAME); remove that substring check — either drop the workspace_members.any branch entirely and rely on Self::packages_include_name(&metadata, VERIFICATION_PACKAGE_NAME)? for exact package-name matching, or if you need to keep the workspace_members check, change the closure to test exact equality (member.as_str().is_some_and(|m| m == VERIFICATION_PACKAGE_NAME)). Update the condition so it no longer depends on Cargo pkgid substring matching (referencing verify_verification_crate_is_absent, metadata_string_array, packages_include_name, and VERIFICATION_PACKAGE_NAME).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/execplans/10-1-1-convert-root-manifest-into-hybrid-workspace.md`:
- Line 7: The document's top "Status: IMPLEMENTED" is inconsistent with the open
validation in Stage E; update the file
docs/execplans/10-1-1-convert-root-manifest-into-hybrid-workspace.md so the
ExecPlan is internally consistent by either marking "Stage E" as complete and
adding the recorded environment exception details to that stage, or by changing
the status line from "Status: IMPLEMENTED" to a qualifier (e.g., "Status:
IMPLEMENTED (validation pending)" or "Status: PARTIALLY IMPLEMENTED") to reflect
the open validation; apply the same reconciliation to the related section around
lines referenced (134-138) to ensure both the top-level Status and Stage E
agree.
In `@docs/wireframe-testing-crate.md`:
- Around line 442-446: Rewrite the paragraph to remove second-person phrasing
and switch to Oxford spelling for "initialization": explain that
Default::default() (or deriving #[derive(Default)] on a fixture struct) should
be used when the handle is a field of a larger fixture world so the standard
initialization machinery constructs it automatically, and that new() should be
used when an explicit construction point with a visible call site is required
(for example, when recovering from a poisoned mutex during test teardown);
ensure no use of "you" or other 1st/2nd-person pronouns and keep the examples
referencing Default::default(), #[derive(Default)], and new().
In `@src/frame/conversion.rs`:
- Around line 193-240: Replace the seven near-duplicate unit tests for
fill_buf_with_prefix with a single parameterised rstest that takes (input:
&[u8], endianness: Endianness, expected: [u8;8]) and asserts the buffer equals
expected; include cases for Big and Little for 1, 2, 4 and 8-byte inputs (add
the missing little-endian 2-byte case #[case(&[0x01,0x02], Endianness::Little,
[0x01,0x02,0,0,0,0,0,0])] so both endiannesses are symmetric), reuse the same
buffer initialization (let mut buf = [0u8;8]; fill_buf_with_prefix(&mut buf,
input, endianness); assert_eq!(buf, expected)) and remove the individual test
functions (keep references to fill_buf_with_prefix and Endianness to locate the
code).
- Around line 36-46: The function fill_buf_with_prefix currently uses direct
slicing (buf[8 - size..], buf[..size]) under a clippy suppression; change it to
validate and compute ranges safely by using checked_sub to compute the start
index for Big endian and get_mut or get to obtain subslices (returning an Err if
size is not one of 1,2,4,8 or otherwise out-of-bounds), change
fill_buf_with_prefix signature to return Result<(), YourErrorType> and copy into
the safely-obtained subslice, and update the call site that currently does
fill_buf_with_prefix(&mut buf, prefix, endianness); to propagate the Result with
? as indicated; also replace the seven duplicated tests with a parameterised
#[rstest] covering widths [1,2,4,8] for both Endianness::Big and
Endianness::Little (including the previously-missing little-endian 2-byte case)
to assert the buffer contents for each combination.
In `@src/message_assembler/series.rs`:
- Around line 313-322: Add a regression test that calls
start_sequence_tracking(FrameSequence(u32::MAX), false) and asserts it returns
Err(MessageSeriesError::SequenceOverflow { .. }) and that the Series instance's
sequence_tracking and next_sequence fields remain unchanged; mirror the existing
advance_sequence_or_overflow test pattern but explicitly check
Series.sequence_tracking and Series.next_sequence before and after the call to
ensure no partial mutation occurred, referencing the Series struct and its
start_sequence_tracking and advance_sequence_or_overflow methods.
In `@tests/fixtures/workspace_manifest.rs`:
- Around line 68-80: Rename the helper metadata_string_array to a neutral name
like metadata_array and change its return type from &'a Vec<Value> to a slice
&'a [Value]; update its signature (metadata_array<'a>(metadata: &'a Value,
field: &str) -> Result<&'a [Value], String>) and adjust the packages function to
call Self::metadata_array(metadata, "packages") and to return Result<&[Value],
String> so callers no longer depend on the Vec backing type.
---
Duplicate comments:
In `@tests/fixtures/workspace_manifest.rs`:
- Around line 166-173: The code in verify_verification_crate_is_absent currently
does a substring match against workspace_members using
member.contains(VERIFICATION_PACKAGE_NAME); remove that substring check — either
drop the workspace_members.any branch entirely and rely on
Self::packages_include_name(&metadata, VERIFICATION_PACKAGE_NAME)? for exact
package-name matching, or if you need to keep the workspace_members check,
change the closure to test exact equality (member.as_str().is_some_and(|m| m ==
VERIFICATION_PACKAGE_NAME)). Update the condition so it no longer depends on
Cargo pkgid substring matching (referencing verify_verification_crate_is_absent,
metadata_string_array, packages_include_name, and VERIFICATION_PACKAGE_NAME).
🪄 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
Run ID: 6ec79d6e-a455-49c8-95f7-299ff3a224ef
📒 Files selected for processing (6)
docs/developers-guide.mddocs/execplans/10-1-1-convert-root-manifest-into-hybrid-workspace.mddocs/wireframe-testing-crate.mdsrc/frame/conversion.rssrc/message_assembler/series.rstests/fixtures/workspace_manifest.rs
This change updates the private helper fill_buf_with_prefix in src/frame/conversion.rs to return a Result type, adding explicit validation of prefix length and bounds checking. It improves error handling by returning io::Error when the prefix length is invalid rather than panicking or assuming correctness. Additional changes include: - Updating callers to handle the Result return value. - Adding comprehensive tests using rstest to verify correct buffer placement for all supported prefix sizes and endianness. - Enhancing documentation in docs/developers-guide.md for fill_buf_with_prefix and ServerShutdownHandle. - Various minor fixes and improvements in message_assembler series tests and workspace manifest test fixture. This refactor improves robustness and correctness in frame conversion logic. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/developers-guide.md`:
- Around line 209-211: Edit the sentence in the docs that describes
LoggerHandle::new(): add a comma before "so" in "drains any buffered log records
so the next test starts from a clean state" to read "drains any buffered log
records, so the next test starts from a clean state" to improve readability.
🪄 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
Run ID: 7ed481a1-15c1-4874-8bd7-66dedcf0c5fb
📒 Files selected for processing (6)
docs/developers-guide.mddocs/execplans/10-1-1-convert-root-manifest-into-hybrid-workspace.mddocs/wireframe-testing-crate.mdsrc/frame/conversion.rssrc/message_assembler/series.rstests/fixtures/workspace_manifest.rs
Owner
Author
|
@coderabbitai pause |
Contributor
✅ Actions performedReviews paused. |
…or explanation Corrected comma placement in the description of LoggerHandle::new() to improve clarity. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
…ated module Refactor sequence tracking unit and property tests from inline module in `series.rs` into a separate `tests.rs` file to improve code organization and readability. Co-authored-by: devboxerhub[bot] <devboxerhub[bot]@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Changes
Rationale
Plan of work (highlights)
Concrete file plan (high level)
Risks, constraints, and tolerances
Progress (as of this PR)
Decision Log
Outcomes & Retrospective
Context and orientation
Task
Summary by Sourcery