WBS-6.2: property tests for sl-viewer corpus_paths, parquet source, settings - #425
Conversation
WBS-6.2 evidence: integration tests under crates/sl-viewer/tests/ that exercise the PR #419 surfaces with proptest. * corpus_paths: round-trip preserves all paths, is_empty() agrees with custom_paths.is_empty(). * parquet source: list() returns exactly the unique session ids written, load(id) rehydrates messages in write-order with a recognised role. Fixture writer lives inside the integration test because the unit-test helper is gated on cfg(test). * settings: every (Theme, DefaultTab) pair survives save -> load. proptest is added to sl-viewer's [dev-dependencies] (mirrors the workspace root) so the integration test can reach proptest::prelude.
🤖 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: 53 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 ignored due to path filters (1)
📒 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}
| for (i, (id, rows)) in by_id.iter().enumerate() { | ||
| let path = dir.path().join(format!("sessions-{i:04}.parquet")); | ||
| write_fixture(&path, rows); |
There was a problem hiding this comment.
Suggestion: The fixture generation creates one file for each unique session ID, so the property never exercises the loader's duplicate-session-across-files behavior. A regression in resolving or hydrating an ID claimed by multiple parquet files would pass this suite. Generate multiple files that intentionally share at least one session ID and assert the documented resolution behavior. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Duplicate parquet exports lack property coverage.
- ⚠️ Session hydration depends on untested file resolution.
- ❌ A resolution regression can display incomplete session data.
(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.rs
**Line:** 213:215
**Comment:**
*Incomplete Implementation: The fixture generation creates one file for each unique session ID, so the property never exercises the loader's duplicate-session-across-files behavior. A regression in resolving or hydrating an ID claimed by multiple parquet files would pass this suite. Generate multiple files that intentionally share at least one session ID and assert the documented resolution behavior.
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| prop_assert_eq!( | ||
| session.messages.len(), | ||
| expected_rows.len(), | ||
| "session {} message count mismatch", | ||
| id, | ||
| ); | ||
| for (actual, expected) in session.messages.iter().zip(expected_rows.iter()) { | ||
| prop_assert_eq!( | ||
| actual.content.clone(), | ||
| expected.content.clone(), | ||
| "session {} message body mismatch", | ||
| id, | ||
| ); |
There was a problem hiding this comment.
Suggestion: The generated rows include ts_ms, and the fixture writes that column, but the property never compares the loaded timestamps with the expected values. A timestamp extraction, conversion, or row-alignment regression can therefore pass despite the fixture exercising timestamp data. Assert each loaded message's ts_ms against the corresponding expected.ts_ms. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Viewer activity ordering can use missing timestamps.
- ⚠️ Corpus timeline metadata is not property-checked.
- ⚠️ Timestamp conversion 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.rs
**Line:** 244:256
**Comment:**
*Incomplete Implementation: The generated rows include `ts_ms`, and the fixture writes that column, but the property never compares the loaded timestamps with the expected values. A timestamp extraction, conversion, or row-alignment regression can therefore pass despite the fixture exercising timestamp data. Assert each loaded message's `ts_ms` against the corresponding `expected.ts_ms`.
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 recognised = matches!( | ||
| actual.role, | ||
| session_ledger::domain::session::Role::User | ||
| | session_ledger::domain::session::Role::Assistant | ||
| | session_ledger::domain::session::Role::System | ||
| | session_ledger::domain::session::Role::Tool | ||
| | session_ledger::domain::session::Role::Subagent | ||
| ); | ||
| prop_assert!( | ||
| recognised, | ||
| "session {} produced unrecognised role {:?}", | ||
| id, | ||
| actual.role, | ||
| ); | ||
| } |
There was a problem hiding this comment.
Suggestion: The test accepts any recognised role regardless of the generated input role. A loader regression that maps every row to Role::User would still pass, including for assistant, system, tool, and subagent inputs. Compare actual.role with the expected role derived from expected.role instead of only checking that it is one of the enum variants. [incomplete implementation]
Severity Level: Major ⚠️
- ❌ Viewer transcripts can misattribute message authors.
- ⚠️ Subagent and tool distinctions are not verified.
- ⚠️ Role-sensitive downstream processing can receive wrong data.
(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.rs
**Line:** 259:273
**Comment:**
*Incomplete Implementation: The test accepts any recognised role regardless of the generated input role. A loader regression that maps every row to `Role::User` would still pass, including for assistant, system, tool, and subagent inputs. Compare `actual.role` with the expected role derived from `expected.role` instead of only checking that it is one of the enum variants.
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 fixAppend crates/sl-viewer/tests/properties_viewer.rs to the WBS-6.2 evidence set so the audit gap matrix reflects PR #425. Status stays partial — fuzzing cadence, full loom/shuttle, and perf-budget gates remain.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| Err(session_ledger::ports::PortError::NotFound(missing)) => { | ||
| panic!("list() returned {} but load() reported NotFound({})", id, missing); | ||
| } | ||
| Err(err) => { |
There was a problem hiding this comment.
WARNING: Silent backend error swallowing weakens property coverage
The Err(err) branch at lines 278-282 silently accepts any error other than NotFound, printing only to stderr. A loader regression that returns PermissionDenied or another backend error for every session would still pass this property test because no assertions are executed for errored sessions.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| roles.push(Some(row.role.as_str())); | ||
| contents.push(Some(row.content.as_str())); | ||
| ts_values.push(row.ts_ms); | ||
| cwds.push(None); |
There was a problem hiding this comment.
SUGGESTION: cwd and title columns never populated with non-None values
cwds.push(None) and titles.push(None) at lines 156-157 mean the fixture always writes NULL for these schema columns. The property never exercises non-None values, so a regression in how the loader handles populated cwd or title fields would go undetected.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Reviewed by step-3.7-flash · Input: 53.2K · Output: 14.7K · Cached: 348.4K |
WBS-6.2 (follow-up to #425): extends the sl-viewer property surface to cover the theme and daemon_url modules that PR #419 also touched. Theme (crates/sl-viewer/src/theme.rs): * Theme JSON round-trip preserves the variant and the lowercase serialisation contract documented on the type. * Theme::default() is always System (regression guard for the #[default] attribute). * ThemeColors::for_theme is total: every variant maps to the same palette as the matching dark()/light() constructor; System falls back to dark (design contract). * ThemeColors gains PartialEq + Eq derives so the property test can compare palettes structurally. daemon_url (crates/sl-viewer/src/daemon_url.rs): * daemon_api_url(path) is exactly base + '/' + path-with-leading- slash-stripped, regardless of leading or trailing slashes on the path argument. * daemon_api_url is idempotent w.r.t. leading slash stripping (api/x and /api/x produce the same URL). * daemon_host_display never starts with http(s):// and never ends with '/'. WBS-6.2 evidence list and CHANGELOG Unreleased reflect the new property file (properties_viewer_theme_url.rs).
* Add property tests for sl-viewer theme + daemon_url WBS-6.2 (follow-up to #425): extends the sl-viewer property surface to cover the theme and daemon_url modules that PR #419 also touched. Theme (crates/sl-viewer/src/theme.rs): * Theme JSON round-trip preserves the variant and the lowercase serialisation contract documented on the type. * Theme::default() is always System (regression guard for the #[default] attribute). * ThemeColors::for_theme is total: every variant maps to the same palette as the matching dark()/light() constructor; System falls back to dark (design contract). * ThemeColors gains PartialEq + Eq derives so the property test can compare palettes structurally. daemon_url (crates/sl-viewer/src/daemon_url.rs): * daemon_api_url(path) is exactly base + '/' + path-with-leading- slash-stripped, regardless of leading or trailing slashes on the path argument. * daemon_api_url is idempotent w.r.t. leading slash stripping (api/x and /api/x produce the same URL). * daemon_host_display never starts with http(s):// and never ends with '/'. WBS-6.2 evidence list and CHANGELOG Unreleased reflect the new property file (properties_viewer_theme_url.rs). * Add IntentState JSON round-trip + terminal-invariant properties WBS-6.2 (follow-up to #427): tests/properties.rs gains two proptest properties for the IntentState FSM's serde contract: * intent_state_json_round_trip_preserves_variant — every IntentState variant serialises to its lowercase kebab-case Debug name and round-trips back to the same variant. Catches drift in the `#[serde(rename_all = "kebab-case")]` attribute. * intent_state_terminal_invariant_holds_across_serde — `is_terminal` agrees with the serde representation: serialising + deserialising Pruned must not silently turn it into something non-terminal. --------- Co-authored-by: SessionLedger Bot <team@sessionledger.local>
…rift fixes (#429) * Fix pre-existing CI drift (fuzz + rootless + clippy) + viewer unfinished_tab properties Four bounded cleanups observed while CI was running on #425/#427: 1) scripts/fuzz-cadence-check.ps1: the "PR smoke stays short" anchor was checking ci.yml for max_total_time=10, but the 10s fuzz-smoke job no longer lives there (consolidated into fuzz-blocking.yml at 30s). Re-pointed the check at the actual PR fuzz budget: fuzz-blocking.yml with max_total_time=30. 2) scripts/rootless-nonet-check.ps1 + .github/workflows/ci.yml: the script expected ci.yml to cross-reference rootless-nonet.yml, but that anchor had drifted out of ci.yml while security.yml retained it. Added a small rootless-nonet-policy smoke job to ci.yml (matches the docs/ops/sandbox-boundary.md C04 L40 contract that 'ci.yml cross-reference | done') and tightened the script's regex so continue-on-error detection can't bleed across jobs. 3) clippy -D warnings under --all-targets --all-features: * tests/alloc_profile.rs: panic-in-if-then — folded the Windows panic into an explicit else branch. * tests/replay_breadth.rs: 6 unnecessary trailing commas in assert! macros. 4) crates/sl-viewer/tests/properties_viewer_unfinished_tab.rs: 6 proptest properties covering reason_label (non-empty, injective) and unfinished_items (deterministic, descending by ts_ms, tiebreak by session_id asc, length-monotonic). Adds to WBS-6.2 viewer property surface alongside #425/#427. WBS-6.2 evidence list and CHANGELOG Unreleased reflect the new property file. * WBS-6.2: link new viewer unfinished_tab surface + CI drift fixes WBS-6.2 evidence list and TRACEABILITY.json gain the new properties_viewer_unfinished_tab.rs and reference #428 for the unfinished-tab properties + CI drift fixups. Status stays partial (fuzzing cadence, full loom/shuttle, perf-budget gates remain). --------- Co-authored-by: SessionLedger Bot <team@sessionledger.local>
User description
Summary
Adds integration-test property evidence for the three new modules introduced by PR #419:
corpus_paths::CorpusPathConfiground-trip and mergingparquet_source::ParquetCorpusSourcelist/load invariantssettings::Settingspersistence round-tripThe properties live under
crates/sl-viewer/tests/properties_viewer.rsand are gated via#[cfg(feature = "parquet")]where applicable so the parquet case only runs when the feature is enabled.Properties
corpus_paths_round_trip_preserves_all_paths— arbitrary 0..16 paths survive save -> load; also asserts the round-trip does not silently dedupe (user-picked paths are authoritative).corpus_paths_empty_predicate_agrees_with_field—is_empty()andcustom_paths.is_empty()agree for arbitrary inputs.parquet_list_matches_loaded_sessions— for any set of 1..12 rows bounded to ASCII printable bodies and a recognised role label, writing one parquet per unique session id and askinglist()must return exactly those ids, and eachload(id)must rehydrate every message body in write-order with a recognised role.settings_round_trip_preserves_all_fields— every(Theme, DefaultTab)pair (3 x 9 = 27 cases) survivessave_to_path->load_from_pathequality.Implementation notes
proptestis a workspace dev-dep declared in the rootCargo.toml; I re-declare it undersl-viewer/[dev-dependencies]so the integration test canuse proptest::prelude::*. Version kept in sync.parquet_source::test_fixturebecause that helper is gated on#[cfg(test)] mod test_fixtureand is not reachable from an integration test. The schema literal in the test mirrorsCLAUDE_SCHEMA(session_id, role, content, ts_ms, cwd, title) — the extracwd/titlecolumns are written asNonesince the property does not exercise metadata.Validation
cargo test -p sl-viewer --test properties_viewer --features "desktop parquet" --locked— 4 passed.cargo fmt -p sl-viewer— clean.cargo clippy -p sl-viewer --test properties_viewer --features "desktop parquet" --locked -- -D warnings— clean.cargo build --release -p sl-viewer --features desktop --locked— succeeds.Out of scope
corpus_cta::tests::folder_picker_returns_none_in_headless_buildstest fails in this headless test env becauserfdcannot spawn a native dialog off the main thread. That test is unrelated to this change and was already failing before my edits.WBS-6.2 row in
docs/ops/WBS.mdwill flip frompartial->donein a follow-up commit once this lands.CodeAnt-AI Description
Add property-based coverage for viewer path, parquet, and settings behavior
What Changed
Impact
✅ Fewer viewer configuration regressions✅ Reliable parquet session discovery and loading✅ Preserved viewer preferences across restarts💡 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.