test(viewer): session-ledger jsonl parser proptest surface (WBS-6.2 #472) - #493
test(viewer): session-ledger jsonl parser proptest surface (WBS-6.2 #472)#493KooshaPari wants to merge 0 commit into
Conversation
🤖 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 · |
codeant-ai
Bot
added
the
size:XXL
|
Warning Review limit reached
Next review available in: 12 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]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| let ctx = HeuristicContextExtractor::extract_context(&session); | ||
| for f in &ctx.files_mentioned { | ||
| prop_assert!(!f.is_empty(), "file path must be non-empty"); | ||
| prop_assert!(f.len() >= 3, "file path must be at least 3 chars"); | ||
| let has_slash = f.contains('/'); | ||
| let ends_with_ext = FILE_EXTENSIONS.iter().any(|e| f.to_lowercase().ends_with(e)); | ||
| prop_assert!( | ||
| has_slash || ends_with_ext, | ||
| "file path must contain / or end with documented extension: {f}" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: This property checks only the shape of returned file paths and never requires any generated file-like input to be detected. A regression in file extraction therefore passes with an empty result, and the see ... generator case provides no positive detection expectation. Assert the expected path for each generated body where the production contract says it is a file path. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ File extraction regressions can pass CI.
- ⚠️ Viewer memory context omits mentioned files.
- ⚠️ Continuation bundles lose file references.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_session_ledger_context.rs
**Line:** 114:125
**Comment:**
*Incomplete Implementation: This property checks only the shape of returned file paths and never requires any generated file-like input to be detected. A regression in file extraction therefore passes with an empty result, and the `see ...` generator case provides no positive detection expectation. Assert the expected path for each generated body where the production contract says it is a file path.
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| for sym in &ctx.key_symbols { | ||
| // Either the symbol itself contains `::` (kept through trimming), | ||
| // or it was derived from a token whose raw form contained `()`. | ||
| let from_double_colon = sym.contains("::"); | ||
| let from_func_call = symbol_sources.iter().any(|s| { | ||
| s.contains("()") && s.matches(|c: char| c.is_alphanumeric() || c == '_' || c == ':') | ||
| .collect::<String>() | ||
| .contains(sym.as_str()) | ||
| }); | ||
| prop_assert!( | ||
| from_double_colon || from_func_call, | ||
| "symbol must have come from a `::` or `()` token, got {sym}" | ||
| ); | ||
| } |
There was a problem hiding this comment.
Suggestion: The property only validates returned symbols, so an extractor regression that drops every generated symbol still passes because the loop is empty. Assert that each generated :: or () source token produces the expected cleaned symbol, including punctuation-trimming cases. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Symbol extraction regressions can pass viewer property tests.
- ⚠️ Memory tab context can omit referenced symbols.
- ⚠️ Distilled continuation context loses technical references.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_session_ledger_context.rs
**Line:** 280:293
**Comment:**
*Incomplete Implementation: The property only validates returned symbols, so an extractor regression that drops every generated symbol still passes because the loop is empty. Assert that each generated `::` or `()` source token produces the expected cleaned symbol, including punctuation-trimming cases.
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 via_fn = HeuristicContextExtractor::extract_context(&session); | ||
| let extractor = HeuristicContextExtractor::new(); | ||
| let via_trait = extractor.extract(&session).expect("extract must succeed"); | ||
| prop_assert_eq!(via_fn.cwd, via_trait.cwd); | ||
| prop_assert_eq!(via_fn.title, via_trait.title); | ||
| prop_assert_eq!(via_fn.files_mentioned, via_trait.files_mentioned); | ||
| prop_assert_eq!(via_fn.key_symbols, via_trait.key_symbols); | ||
| prop_assert_eq!(via_fn.environment_notes, via_trait.environment_notes); | ||
| prop_assert_eq!(via_fn.key_decisions.len(), via_trait.key_decisions.len()); |
There was a problem hiding this comment.
Suggestion: The trait-path property compares only the number of decisions, not their values. The trait implementation could return different summaries or rationales while this test still passes, even though Decision derives value equality and downstream consumers use both fields. Compare the complete key_decisions collection. [api mismatch]
Severity Level: Major ⚠️
- ⚠️ Trait and associated extraction can diverge silently.
- ⚠️ Viewer may display altered decision rationale.
- ⚠️ Continuation context can contain inconsistent decisions.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_session_ledger_context.rs
**Line:** 422:430
**Comment:**
*Api Mismatch: The trait-path property compares only the number of decisions, not their values. The trait implementation could return different summaries or rationales while this test still passes, even though `Decision` derives value equality and downstream consumers use both fields. Compare the complete `key_decisions` collection.
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 c = HeuristicContractExtractor::extract_contract(&session); | ||
| for crit in &c.success_criteria { | ||
| prop_assert!(crit.starts_with("Goal/requirement: '")); | ||
| prop_assert!(crit.ends_with("'")); | ||
| } | ||
| } |
There was a problem hiding this comment.
Suggestion: The section-shape checks iterate over output without asserting that the trigger produced any output. If recognition of the goal phrase regresses, this test passes with an empty collection. Require at least one matching entry and verify its content, and apply the same presence requirement to the verification, constraint, and do-not-touch properties. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Criteria recognition regressions pass formatting tests.
- ⚠️ Verification and constraint omissions can go undetected.
- ⚠️ Viewer contract data may be incomplete.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_session_ledger_contract.rs
**Line:** 93:98
**Comment:**
*Incomplete Implementation: The section-shape checks iterate over output without asserting that the trigger produced any output. If recognition of the goal phrase regresses, this test passes with an empty collection. Require at least one matching entry and verify its content, and apply the same presence requirement to the verification, constraint, and do-not-touch properties.
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| fn all_patterns_case_insensitive( | ||
| pat in proptest::sample::select(CRITERIA_PATTERNS), | ||
| ) { | ||
| let upper = pat.to_uppercase(); | ||
| let body = format!("we have a {upper} now"); | ||
| let messages = vec![(Role::User, body.as_str())]; | ||
| let session = make_session("criteria-case", &messages); | ||
| let c = HeuristicContractExtractor::extract_contract(&session); | ||
| prop_assert!(!c.success_criteria.is_empty(), "criteria pattern {pat} must match case-insensitively"); | ||
| } |
There was a problem hiding this comment.
Suggestion: This case-insensitivity property samples only CRITERIA_PATTERNS and checks only success_criteria. Case-sensitive regressions in test, constraint, or do-not-touch extraction remain undetected despite those pattern classes being part of the contract. Generate and assert all four pattern categories independently. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Uppercase verification phrases may be missed.
- ⚠️ Uppercase constraints may disappear.
- ⚠️ Do-not-touch rules may be omitted.
- ⚠️ Viewer acceptance context becomes incomplete.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_session_ledger_contract.rs
**Line:** 198:207
**Comment:**
*Incomplete Implementation: This case-insensitivity property samples only `CRITERIA_PATTERNS` and checks only `success_criteria`. Case-sensitive regressions in test, constraint, or do-not-touch extraction remain undetected despite those pattern classes being part of the contract. Generate and assert all four pattern categories independently.
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| fn make_session(id: &str, corpus: Corpus, n_msgs: usize) -> Session { | ||
| let mut session = Session::new(id, corpus); | ||
| for i in 0..n_msgs { | ||
| session | ||
| .messages | ||
| .push(Message::new(Role::User, format!("msg {i}"))); | ||
| } | ||
| session | ||
| } |
There was a problem hiding this comment.
Suggestion: The round-trip properties exercise only short lowercase IDs, Forge or Codex corpora, one user message, and generated msg i content. They never cover optional session metadata, timestamps, other roles, empty or multiple messages, or JSON-sensitive content, so serde field regressions can pass all of these tests. Generate complete sessions with varied fields and message contents. [incomplete implementation]
Severity Level: Major ⚠️
- ⚠️ Metadata serialization regressions can pass tests.
- ⚠️ Non-user roles remain unverified.
- ⚠️ ETL ingestion may lose session fields.
- ⚠️ Distilled output can differ from source sessions.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_session_ledger_jsonl_parser.rs
**Line:** 15:23
**Comment:**
*Incomplete Implementation: The round-trip properties exercise only short lowercase IDs, Forge or Codex corpora, one user message, and generated `msg i` content. They never cover optional session metadata, timestamps, other roles, empty or multiple messages, or JSON-sensitive content, so serde field regressions can pass all of these tests. Generate complete sessions with varied fields and message contents.
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
) Shards the unique viewer proptest from #493 onto main without the obsolete pre-rename daemon source churn.
1a66da6 to
62dc09d
Compare
Thanks for using CodeAnt! 🎉We're free for open-source projects. if you're enjoying it, help us grow by sharing. Share on X · |
codeant-ai
Bot
added
size:L
62dc09d to
f445859
Compare
User description
WBS-6.2 surface 12: session-ledger ingestion.
Branch off fix/viewer-mock-data-properties-20260809 rebased main. Test file compiles and passes locally.
CodeAnt-AI Description
Add property coverage for session-ledger JSONL parsing
What Changed
Impact
✅ Clearer JSONL parse errors✅ Reliable session ordering during import✅ Safer session-ledger ingestion💡 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.