test(viewer): bundle_diff proptest surface (WBS-6.2 #434) - #434
test(viewer): bundle_diff proptest surface (WBS-6.2 #434)#434KooshaPari wants to merge 1 commit into
Conversation
Adds `crates/sl-viewer/tests/properties_viewer_bundle_diff.rs` with
10 proptest properties pinning the `bundle_diff::diff_fields` and
`OkfBundle::from_bundle` reductions:
* `diff_fields` returns the documented 9-field set in stable order
(guards against UI row-count drift when fields are added).
* `diff_fields(a, a)` is reflexive: no fields differ on equal inputs.
* `diff_fields(a, a.clone())` is idempotent: clone-mirror produces no
differences.
* `diff_fields(a, b)` is value-flipped symmetric:
`diff_fields(b, a)` swaps `value_a`/`value_b` per field but the
`differs` set is identical.
* `FieldDiff::differs` matches `value_a != value_b` per field
(catches drift where the boolean is computed independently of values).
* `Option<String>` fields (model, created_at, goal) render the em-dash
fallback (`—`) when both sides are `None`, and the resulting diff
is not a difference.
* `OkfBundle::from_bundle`:
* `message_count` equals the input slice count.
* `has_acceptance`/`has_contract` reflect presence of those kinds
(any-of) in the input continuation.
* `token_count` falls back to 0 when no Intent slice carries a
numeric `user_turn_count` (3-variant: missing slice / missing field
/ non-numeric field).
* `source_id` carries through from the continuation unchanged.
Updates WBS-6.2 evidence list, TRACEABILITY.json, and CHANGELOG.
🤖 CodeAnt AI — Review Status
|
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
|
Warning Review limit reached
Next review available in: 49 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| let diffs = diff_fields(&a, &b); | ||
| for d in &diffs { | ||
| prop_assert_eq!( | ||
| d.differs, | ||
| d.value_a != d.value_b, | ||
| "{}.differs ({}) must match value_a != value_b ({} != {})", | ||
| d.name, d.differs, d.value_a, d.value_b, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Suggestion: The diff properties compare diff_fields outputs only against each other, so they do not independently verify that each named row reads the corresponding OkfBundle member. A consistently wrong mapping, such as emitting token_count under another field's name while preserving the same values on both sides, can satisfy reflexivity, symmetry, and differs == value_a != value_b. Add fixtures that assign distinct values to every field and assert each row's expected values. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Diff UI rows could display the wrong bundle fields.
- ⚠️ Duration, timestamps, goals, and flags lack mapping coverage.
- ⚠️ Future refactors could pass relational properties incorrectly.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_bundle_diff.rs
**Line:** 160:168
**Comment:**
*Incomplete Implementation: The diff properties compare `diff_fields` outputs only against each other, so they do not independently verify that each named row reads the corresponding `OkfBundle` member. A consistently wrong mapping, such as emitting `token_count` under another field's name while preserving the same values on both sides, can satisfy reflexivity, symmetry, and `differs == value_a != value_b`. Add fixtures that assign distinct values to every field and assert each row's expected values.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let bundles: Vec<Bundle> = (0..slice_count) | ||
| .map(|i| Bundle::new(BundleKind::Intent, serde_json::json!({"i": i}))) | ||
| .collect(); |
There was a problem hiding this comment.
Suggestion: The from_bundle properties never construct Context or populated Intent bodies, so regressions in duration_ms, model, created_at, or goal extraction will pass all these tests despite those fields being part of the reduced OkfBundle consumed by the diff UI. Add cases with populated and missing values for each reducer field. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Bundle comparison can lose model and timestamp metadata.
- ⚠️ Goal values may disappear from the diff UI.
- ⚠️ Duration extraction regressions remain undetected.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_bundle_diff.rs
**Line:** 209:211
**Comment:**
*Incomplete Implementation: The `from_bundle` properties never construct `Context` or populated `Intent` bodies, so regressions in `duration_ms`, `model`, `created_at`, or `goal` extraction will pass all these tests despite those fields being part of the reduced `OkfBundle` consumed by the diff UI. Add cases with populated and missing values for each reducer field.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let bundles: Vec<Bundle> = match variant { | ||
| 0 => Vec::new(), | ||
| 1 => vec![Bundle::new(BundleKind::Intent, serde_json::json!({"goal": "x"}))], | ||
| _ => vec![Bundle::new( | ||
| BundleKind::Intent, | ||
| serde_json::json!({"user_turn_count": "not-a-number"}), | ||
| )], | ||
| }; | ||
| let cb = ContinuationBundle { | ||
| source_id: "test".into(), | ||
| bundles, | ||
| }; | ||
| let okf = OkfBundle::from_bundle(&cb); | ||
| prop_assert_eq!(okf.token_count, 0, "token_count must default to 0 when missing/non-numeric"); |
There was a problem hiding this comment.
Suggestion: The property does not verify the documented aggregation contract for token_count: every generated case contains zero or one Intent bundle, so an implementation that uses only the first Intent bundle instead of summing user_turn_count across all Intent bundles will pass. Generate multiple Intent bundles with numeric counts and assert their sum. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Bundle comparison can display an undercounted token total.
- ⚠️ Multi-message viewer sessions can expose incorrect token counts.
- ⚠️ The documented aggregation contract remains untested.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_bundle_diff.rs
**Line:** 253:266
**Comment:**
*Incomplete Implementation: The property does not verify the documented aggregation contract for `token_count`: every generated case contains zero or one `Intent` bundle, so an implementation that uses only the first `Intent` bundle instead of summing `user_turn_count` across all `Intent` bundles will pass. Generate multiple `Intent` bundles with numeric counts and assert their sum.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| /// Compile-time guarantee that the FieldDiff-derived constants stay in sync. | ||
| /// If the impl adds a field, this test fails to compile until EXPECTED_FIELD_NAMES | ||
| /// is updated, prompting the reviewer to confirm the UI row count. | ||
| #[allow(dead_code)] | ||
| const fn _assert_field_count_fits_diff(diff: &[FieldDiff], expected_len: usize) -> bool { | ||
| diff.len() == expected_len | ||
| } |
There was a problem hiding this comment.
Suggestion: This is not a compile-time guarantee because _assert_field_count_fits_diff is never invoked; defining a const fn does not evaluate it or enforce its result. The documented synchronization protection therefore does nothing, and only the runtime property test checks the count. Replace this unused helper with an actually evaluated compile-time assertion or remove the misleading comment. [comment mismatch]
Severity Level: Minor 🧹
- ⚠️ Claimed compile-time synchronization protection is absent.
- ⚠️ Field-count drift is detected only when tests execute.
- ⚠️ The helper can mislead maintainers reviewing coverage.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_bundle_diff.rs
**Line:** 284:290
**Comment:**
*Comment Mismatch: This is not a compile-time guarantee because `_assert_field_count_fits_diff` is never invoked; defining a `const fn` does not evaluate it or enforce its result. The documented synchronization protection therefore does nothing, and only the runtime property test checks the count. Replace this unused helper with an actually evaluated compile-time assertion or remove the misleading comment.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
|
Closing due to unresolved conflicts. Cannot auto-rebase. |
Adds `crates/sl-viewer/tests/properties_viewer_bundle_diff.rs` with
10 proptest properties pinning the `bundle_diff::diff_fields` and
`OkfBundle::from_bundle` reductions:
* `diff_fields` returns the documented 9-field set in stable order
(guards against UI row-count drift when fields are added).
* `diff_fields(a, a)` is reflexive: no fields differ on equal inputs.
* `diff_fields(a, a.clone())` is idempotent: clone-mirror produces no
differences.
* `diff_fields(a, b)` is value-flipped symmetric:
`diff_fields(b, a)` swaps `value_a`/`value_b` per field but the
`differs` set is identical.
* `FieldDiff::differs` matches `value_a != value_b` per field
(catches drift where the boolean is computed independently of values).
* `Option<String>` fields (model, created_at, goal) render the em-dash
fallback (`—`) when both sides are `None`, and the resulting diff
is not a difference.
* `OkfBundle::from_bundle`:
* `message_count` equals the input slice count.
* `has_acceptance`/`has_contract` reflect presence of those kinds
(any-of) in the input continuation.
* `token_count` falls back to 0 when no Intent slice carries a
numeric `user_turn_count` (3-variant: missing slice / missing field
/ non-numeric field).
* `source_id` carries through from the continuation unchanged.
Updates WBS-6.2 evidence list, TRACEABILITY.json, and CHANGELOG.
Co-authored-by: SessionLedger Bot <team@sessionledger.local>
User description
Summary
Adds
crates/sl-viewer/tests/properties_viewer_bundle_diff.rswith 10 proptest properties pinning thebundle_diff::diff_fieldsandOkfBundle::from_bundlereductions (WBS-6.2 #434).diff_fields(6 properties)diff_fields(a, a)is reflexive: no fields differ on equal inputsdiff_fields(a, a.clone())is idempotent: clone-mirror produces no differencesdiff_fields(a, b)is value-flipped symmetric:diff_fields(b, a)swapsvalue_a/value_bper field but thediffersset is identicalFieldDiff::differsmatchesvalue_a != value_bper field (catches drift where the boolean is computed independently of values)Option<String>fields (model, created_at, goal) render the em-dash fallback (—) when both sides areNone, and the resulting diff is not a differenceOkfBundle::from_bundle(4 properties)message_countequals the input slice counthas_acceptance/has_contractreflect presence of those kinds (any-of) in the input continuationtoken_countfalls back to 0 when no Intent slice carries a numericuser_turn_count(3-variant: missing slice / missing field / non-numeric field)source_idcarries through from the continuation unchangedValidation
cargo test -p sl-viewer --test properties_viewer_bundle_diff --features "desktop parquet" --locked— 10 passedcargo clippy -p sl-viewer --test properties_viewer_bundle_diff --features "desktop parquet" --locked -- -D warnings— cleancargo fmt --all --check— cleanWBS / TRACEABILITY
WBS-6.2 evidence list and
TRACEABILITY.jsongaincrates/sl-viewer/tests/properties_viewer_bundle_diff.rs. Status stayspartial(fuzzing cadence, full loom/shuttle, perf-budget gates remain). CHANGELOG Unreleased documents the new surface.CodeAnt-AI Description
Add property coverage for viewer bundle comparisons and bundle summaries
What Changed
Impact
✅ Stable bundle comparison rows✅ Clear em-dash display for missing values✅ Reliable bundle summary counts and flags💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.