-
Notifications
You must be signed in to change notification settings - Fork 0
fix: CodeRabbit-driven viewer unfinished_tab + alloc_profile hardening #432
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 1 commit
aa4787d
6abd039
fa243fa
f726e1c
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -21,7 +21,7 @@ | |
|
|
||
| use proptest::prelude::*; | ||
| use session_ledger::domain::session::{Corpus, Message, Role, Session}; | ||
| use session_ledger::domain::worklog::{UnfinishedReason, UnfinishedWorkItem}; | ||
| use session_ledger::domain::worklog::UnfinishedReason; | ||
| use sl_viewer::unfinished_tab::{reason_label, unfinished_items}; | ||
|
|
||
| // ── strategies ───────────────────────────────────────────────────────────── | ||
|
|
@@ -30,19 +30,18 @@ fn session_strategy() -> impl Strategy<Value = Session> { | |
| ( | ||
| // session_id — non-empty, identifier-shaped. | ||
| "[a-zA-Z0-9_-]{1,16}", | ||
| // 0..6 messages; mix of roles + content. Bounded so the | ||
| // `detect_unfinished` projection runs cheaply. | ||
| // 0..6 messages; each message has its own independent | ||
| // `Option<i64>` ts_ms so the `detect_unfinished` projection's | ||
| // `find_map(|m| m.ts_ms).rev()` contract is exercised naturally | ||
| // (per-message timestamps, not a session-wide value). | ||
|
Comment on lines
+33
to
+36
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win Compute the maximum timestamp, not the last timestamped message.
With independently generated timestamps, this property fails for a session such as Proposed fix- // `find_map(|m| m.ts_ms).rev()` contract is exercised naturally
+ // maximum-known-timestamp contract is exercised naturally
@@
- let expected_last_activity_ms =
- session.messages.iter().rev().find_map(|m| m.ts_ms);
+ let expected_last_activity_ms =
+ session.messages.iter().filter_map(|m| m.ts_ms).max();Also applies to: 219-225 🤖 Prompt for AI Agents |
||
| prop::collection::vec( | ||
| (0u8..5, "[ -~]{1,40}"), | ||
| (0u8..5, "[ -~]{1,40}", prop::option::of(0i64..1_000_000_000_000)), | ||
| 0..6, | ||
| ), | ||
| // last_activity_ms — Some(i64) or None. None is the "unknown | ||
| // last activity" sentinel the worklog projector uses. | ||
| prop::option::of(0i64..1_000_000_000_000), | ||
| ) | ||
| .prop_map(|(session_id, messages, ts_ms)| { | ||
| .prop_map(|(session_id, messages)| { | ||
| let mut session = Session::new(format!("sess-{session_id}"), Corpus::Forge); | ||
| for (role_idx, content) in messages { | ||
| for (role_idx, content, ts_ms) in messages { | ||
| let role = match role_idx % 5 { | ||
| 0 => Role::User, | ||
| 1 => Role::Assistant, | ||
|
|
@@ -109,26 +108,46 @@ proptest! { | |
| } | ||
|
|
||
| /// Property: `unfinished_items` orders known timestamps descending. | ||
| /// Two items with the same `last_activity_ms` may appear in any order | ||
| /// (we don't constrain the tiebreak here; see next property). | ||
| /// Two invariants: | ||
| /// (a) within the sliding window of items with a known timestamp, | ||
| /// timestamps are non-increasing; | ||
| /// (b) once an item with `last_activity_ms == None` appears, no | ||
| /// later item may carry a known timestamp (None is the | ||
| /// "unknown last activity" sentinel and always sorts last). | ||
| #[test] | ||
| fn unfinished_items_orders_known_timestamps_descending( | ||
| sessions in prop::collection::vec(session_strategy(), 1..10), | ||
| ) { | ||
| let items = unfinished_items(&sessions); | ||
|
|
||
| // Filter to items with a known timestamp so the descending | ||
| // invariant applies cleanly. | ||
| let with_ts: Vec<&UnfinishedWorkItem> = | ||
| items.iter().filter(|i| i.last_activity_ms.is_some()).collect(); | ||
|
|
||
| for window in with_ts.windows(2) { | ||
| let prev = window[0].last_activity_ms.expect("filtered Some"); | ||
| let next = window[1].last_activity_ms.expect("filtered Some"); | ||
| prop_assert!( | ||
| prev >= next, | ||
| "known timestamps must be descending: {prev} came before {next}", | ||
| ); | ||
| // (a) descending among items with a known timestamp. | ||
| for window in items.windows(2) { | ||
| let prev = &window[0]; | ||
| let next = &window[1]; | ||
| match (prev.last_activity_ms, next.last_activity_ms) { | ||
| (Some(a), Some(b)) => { | ||
| prop_assert!( | ||
| a >= b, | ||
| "known timestamps must be non-increasing: {a} came before {b}", | ||
| ); | ||
| } | ||
| _ => {} | ||
| } | ||
| } | ||
|
|
||
| // (b) no Some(ts) appears after a None. | ||
| let mut seen_none = false; | ||
| for item in &items { | ||
| if seen_none { | ||
| prop_assert!( | ||
| item.last_activity_ms.is_none(), | ||
| "Some(ts) found after None: {:?} appeared after a None item", | ||
| item.last_activity_ms, | ||
| ); | ||
| } | ||
| if item.last_activity_ms.is_none() { | ||
| seen_none = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -177,4 +196,35 @@ proptest! { | |
| "appending sessions must not lose items: base={base_items}, combined={combined_items}", | ||
| ); | ||
| } | ||
|
|
||
| /// Property: for each projected item, `last_activity_ms` equals the | ||
| /// maximum known `ts_ms` over the *session's* messages, or `None` | ||
| /// if none of the session's messages carried a timestamp. This pins | ||
| /// the per-message → projected-Item reduction explicitly (the unit | ||
| /// tests in `domain/worklog.rs` cover specific values; this property | ||
| /// pins the projection over many shapes). | ||
| #[test] | ||
| fn unfinished_items_last_activity_matches_session_max_ts( | ||
| sessions in prop::collection::vec(session_strategy(), 0..8), | ||
| ) { | ||
| let items = unfinished_items(&sessions); | ||
|
|
||
| for item in &items { | ||
| // Reconstruct the source session by id. | ||
| let session = sessions | ||
| .iter() | ||
| .find(|s| s.id == item.session_id) | ||
| .expect("projected item must reference an input session"); | ||
|
Comment on lines
+214
to
+217
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: Severity Level: Major
|
||
|
|
||
| let expected_last_activity_ms = | ||
| session.messages.iter().rev().find_map(|m| m.ts_ms); | ||
|
Comment on lines
+219
to
+220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggestion: The property claims to verify the maximum timestamp, but Severity Level: Minor 🧹- ⚠️ Property documentation misstates the timestamp contract.
- ⚠️ Numeric-maximum regressions are not detected by this test.(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_unfinished_tab.rs
**Line:** 219:220
**Comment:**
*Docstring Mismatch: The property claims to verify the maximum timestamp, but `iter().rev().find_map(...)` returns the last timestamped message in message order, not the numerically greatest timestamp. As a result, a regression that changes production to use an incorrect numeric maximum could pass this property, while the property description and assertion message incorrectly claim maximum semantics. Either rename the property/documentation to “latest timestamped message” or compute an actual maximum if that is the intended contract.
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!( | ||
| item.last_activity_ms, | ||
| expected_last_activity_ms, | ||
| "session {}: projected last_activity_ms must equal session max known ts_ms", | ||
| session.id, | ||
| ); | ||
| } | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix inline-comment spacing on both pinned checkout lines.
Line 203 and Line 255 have one space before
# v7.yamllintreports both lines. Add a second space before the comment.Proposed fix
Apply the same spacing change to Line 255.
Also applies to: 255-255
🧰 Tools
🪛 GitHub Check: Trunk Check
[warning] 203-203: yamllint(comments)
[new] too few spaces before comment: expected 2
🤖 Prompt for AI Agents
Source: Linters/SAST tools