test(viewer): session-ledger heuristic acceptance extractor proptest surface (WBS-6.2 #466) - #486
Merged
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 · |
📝 WalkthroughSummaryThe PR adds property-based tests for SessionLedger context, contract, and acceptance extractors. The coverage includes pattern matching, deduplication, determinism, role-sensitive behavior, API parity, and Must Fix
Should Fix
Consider
Request ChangesRequest changes until the acceptance property-test file is decomposed below the 500-line limit and the required Rust checks pass. WalkthroughAdded property-based tests for the context, contract, and acceptance extractors. The tests cover pattern detection, normalization, deduplication, determinism, trait parity, scoring, role-sensitive behavior, and empty-state semantics. Documentation now records the new WBS-6.2 evidence paths. ChangesExtractor property-test coverage
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 2⚔️ Resolve merge conflicts 💡
🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
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:XXL
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
Comment on lines
+427
to
+430
| // stronger statements below pin the value. Here we pin the contract | ||
| // that with no findings, the score is 0. | ||
| if a.evidence.is_empty() && !a.user_confirmed && a.testing_evidence.is_empty() { | ||
| prop_assert_eq!(a.satisfaction_score, 0); |
There was a problem hiding this comment.
Suggestion: This property is vacuous for generated messages containing any acceptance signal: it only checks the score inside a conditional after extraction, without asserting that the generated input is signal-free. A score regression for genuinely signal-free text can therefore pass without executing the assertion. Generate text that excludes all known patterns and assert the score is unconditionally zero. [incorrect condition logic]
Severity Level: Major ⚠️
- ⚠️ Zero-score behavior is conditionally tested.
- ⚠️ Signal-containing cases bypass the assertion.
- ❌ Score regressions can evade this property.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_session_ledger_acceptance.rs
**Line:** 427:430
**Comment:**
*Incorrect Condition Logic: This property is vacuous for generated messages containing any acceptance signal: it only checks the score inside a conditional after extraction, without asserting that the generated input is signal-free. A score regression for genuinely signal-free text can therefore pass without executing the assertion. Generate text that excludes all known patterns and assert the score is unconditionally zero.
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
Comment on lines
+472
to
+498
| // Re-derive the count: unique evidence + user messages with a confirmation. | ||
| let mut expected_unique_evidence: Vec<String> = Vec::new(); | ||
| let mut user_messages_with_confirm = 0_usize; | ||
| for msg in &session.messages { | ||
| let lower = msg.content.to_lowercase(); | ||
| for pat in EVIDENCE_PATTERNS { | ||
| if lower.contains(pat) { | ||
| let ev = format!("Evidence: '{pat}'"); | ||
| if !expected_unique_evidence.contains(&ev) { | ||
| expected_unique_evidence.push(ev); | ||
| } | ||
| } | ||
| } | ||
| if msg.role == Role::User { | ||
| let mut found = false; | ||
| for pat in USER_CONFIRMATION_PATTERNS { | ||
| if lower.contains(pat) { | ||
| found = true; | ||
| break; | ||
| } | ||
| } | ||
| if found { | ||
| user_messages_with_confirm += 1; | ||
| } | ||
| } | ||
| } | ||
| let expected_count = expected_unique_evidence.len() + user_messages_with_confirm; |
There was a problem hiding this comment.
Suggestion: The expected score is re-derived with the same pattern lists and counting rules as the implementation, so a shared regression in signal selection or duplicate-confirmation handling can make both sides agree. It also omits testing_evidence entirely. Derive the expected result from an independent domain-level contract or fixed examples that exercise testing evidence, duplicate messages, and multiple confirmation patterns. [logic error]
Severity Level: Major ⚠️
- ❌ Score regressions can pass this property.
- ⚠️ Testing-evidence score behavior remains unverified.
- ⚠️ Acceptance score coverage is coupled to implementation lists.
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_session_ledger_acceptance.rs
**Line:** 472:498
**Comment:**
*Logic Error: The expected score is re-derived with the same pattern lists and counting rules as the implementation, so a shared regression in signal selection or duplicate-confirmation handling can make both sides agree. It also omits `testing_evidence` entirely. Derive the expected result from an independent domain-level contract or fixed examples that exercise testing evidence, duplicate messages, and multiple confirmation patterns.
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![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/sl-viewer/tests/properties_session_ledger_acceptance.rs`:
- Around line 587-603: Rename satisfaction_score_alone_does_not_make_non_empty
and its documentation to describe the actual user-confirmation behavior being
tested. Update the test wording to state that the confirmation phrase sets
user_confirmed and increases satisfaction_score, while retaining the existing
non-empty assertion; do not label this as score-only behavior.
- Around line 290-299: Update the `user_confirmed_false_when_no_role_confirms`
property input strategy to exclude documented confirmation tokens, such as by
generating digits-only text, while preserving the existing session construction
and assertion.
In `@crates/sl-viewer/tests/properties_session_ledger_context.rs`:
- Around line 398-405: Update both decision comparisons in
crates/sl-viewer/tests/properties_session_ledger_context.rs at lines 398-405 and
422-430 to compare the complete key_decisions collections, using Decision’s
PartialEq, rather than only comparing their lengths; preserve the existing
comparisons for the other context fields.
- Around line 451-462: Replace the two one-way assertions with a direct equality
assertion between the context’s is_empty() result and all_empty in
crates/sl-viewer/tests/properties_session_ledger_context.rs:451-462. Apply the
same change to the corresponding assertions for c in
crates/sl-viewer/tests/properties_session_ledger_contract.rs:360-369 and a in
crates/sl-viewer/tests/properties_session_ledger_acceptance.rs:576-584,
preserving each test’s existing all_empty calculation.
In `@crates/sl-viewer/tests/properties_session_ledger_contract.rs`:
- Around line 195-208: Extend all_patterns_case_insensitive to cover
TEST_PATTERNS, CONSTRAINT_PATTERNS, and DO_NOT_TOUCH_PATTERNS in addition to
CRITERIA_PATTERNS. Add uppercase property cases for each pattern set, preserving
the existing session extraction and non-empty assertion for the corresponding
contract section.
🪄 Autofix
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 Plus
Run ID: 46f2de3b-14ca-43c1-9d91-aa520c6bbe39
📒 Files selected for processing (6)
CHANGELOG.mdcrates/sl-viewer/tests/properties_session_ledger_acceptance.rscrates/sl-viewer/tests/properties_session_ledger_context.rscrates/sl-viewer/tests/properties_session_ledger_contract.rsdocs/ops/TRACEABILITY.jsondocs/ops/WBS.md
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Summary
- GitHub Check: prepare
- GitHub Check: browser e2e · axe · responsive · visual
⚠️ CI failures not shown inline (2)
GitHub Check: Summary: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
GitHub Check: Mergify Merge Queue: The current Mergify configuration is invalid
Conclusion: failure
* Invalid condition 'author=dependabot[bot] | renovate[bot]' @ root → pull_request_rules → item 1 → conditions → item 0 → author=dependabot[bot] | renovate[bot]
```
Invalid GitHub login
```
* Invalid condition 'author=trunk-io[bot] | mergify[bot] | github-actions[bot]' @ root → pull_request_rules → item 2 → conditions → item 0 → author=trunk-io[bot] | mergify[bot] | github-actions[bot]
```
Invalid GitHub login
```
* Invalid condition 'age>=30d' @ root → pull_request_rules → item 8 → conditions → item 2 → age>=30d
```
Invalid attribute
```
* Extra inputs are not permitted @ root → pull_request_rules → item 0 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 1 → actions → post_merge
* Extra inputs are not permitted @ root → pull_request_rules → item 3 → actions → request_reviews → github_accounts
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Use the Rust toolchain pinned inrust-toolchain.toml; the workspace MSRV is Rust 1.85.
Validate Rust workspace changes with the prescribed locked build, all-features test suite, Clippy, and rustfmt checks where applicable.
Files:
crates/sl-viewer/tests/properties_session_ledger_contract.rscrates/sl-viewer/tests/properties_session_ledger_context.rscrates/sl-viewer/tests/properties_session_ledger_acceptance.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Fix Clippy warnings; do not add
#[allow]unless it includes a tracking-issue comment.
Files:
crates/sl-viewer/tests/properties_session_ledger_contract.rscrates/sl-viewer/tests/properties_session_ledger_context.rscrates/sl-viewer/tests/properties_session_ledger_acceptance.rs
crates/sl-viewer/**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
crates/sl-viewer/**/*.{rs,toml}: Thesl-viewercrate uses Dioxus 0.6; use the Dioxus CLI/toolchain for desktop development and bundling.
Usecargo check -p sl-vieweras the fast inner-loop check for viewer changes.
Files:
crates/sl-viewer/tests/properties_session_ledger_contract.rscrates/sl-viewer/tests/properties_session_ledger_context.rscrates/sl-viewer/tests/properties_session_ledger_acceptance.rs
crates/sl-viewer/**/*
📄 CodeRabbit inference engine (AGENTS.md)
When packaging the macOS viewer, account for the documented Electrobun/Dioxus code-signing requirements.
Files:
crates/sl-viewer/tests/properties_session_ledger_contract.rscrates/sl-viewer/tests/properties_session_ledger_context.rscrates/sl-viewer/tests/properties_session_ledger_acceptance.rs
*
📄 CodeRabbit inference engine (AGENTS.md)
*: Perform feature work in a git worktree under.claude/worktrees/, created fromorigin/mainon a branch named<type>/<topic>, rather than working directly onmain.
Do not make direct commits to protectedmain; use a pull request.
Do not usegit reset --hard,git stash, orgit cleanin worktrees.
Do not use--no-verifyor bypass hooks without operator approval.
Do not work on a branch or worktree another actor is using.
Files:
CHANGELOG.md
🪛 LanguageTool
docs/ops/WBS.md
[uncategorized] ~32-~32: The official name of this software platform is spelled with a capital “H”.
Context: ...; fuzz/fuzz_targets/jsonl_ingest.rs; .github/workflows/ci.yml; .github/workflows/b...
(GITHUB)
[uncategorized] ~32-~32: The official name of this software platform is spelled with a capital “H”.
Context: ...ingest.rs; .github/workflows/ci.yml; .github/workflows/bench-gate.yml; docs/ops/pe...
(GITHUB)
🔇 Additional comments (4)
CHANGELOG.md (1)
71-73: LGTM!docs/ops/TRACEABILITY.json (1)
334-336: LGTM!docs/ops/WBS.md (1)
32-32: LGTM!crates/sl-viewer/tests/properties_session_ledger_context.rs (1)
12-15: 📐 Maintainability & Code QualityProvide prescribed Rust validation results.
Validate these new property tests with the pinned
rust-toolchain.tomltoolchain. Provide results for the prescribed locked build, all-features test suite, Clippy, rustfmt, andcargo check -p sl-viewer.
crates/sl-viewer/tests/properties_session_ledger_context.rs#L12-L15: include this test file in the prescribed validation.crates/sl-viewer/tests/properties_session_ledger_contract.rs#L11-L14: include this test file in the prescribed validation.crates/sl-viewer/tests/properties_session_ledger_acceptance.rs#L12-L15: include this test file in the prescribed validation.As per coding guidelines: “Use the Rust toolchain pinned in
rust-toolchain.toml” and validate Rust workspace changes with locked build, tests, Clippy, and rustfmt checks.Source: Coding guidelines
Comment on lines
+290
to
+299
| fn user_confirmed_false_when_no_role_confirms( | ||
| body in "[a-zA-Z0-9 .]{1,40}", | ||
| ) { | ||
| let owned = body.clone(); | ||
| let s = owned.as_str(); | ||
| let messages = vec![(Role::User, s), (Role::Assistant, s)]; | ||
| let session = make_session("confirm-none", &messages); | ||
| let a = HeuristicAcceptanceExtractor::extract_acceptance(&session); | ||
| prop_assert!(!a.user_confirmed, "user_confirmed must be false when no confirmation is present"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file size =="
wc -l crates/sl-viewer/tests/properties_session_ledger_acceptance.rs
echo "== outline =="
ast-grep outline crates/sl-viewer/tests/properties_session_ledger_acceptance.rs --view compact || true
echo "== relevant sections =="
sed -n '1,40p;240,320p;540,620p' crates/sl-viewer/tests/properties_session_ledger_acceptance.rs
echo "== search for confirmation detection references =="
rg -n "user_confirmed|confirm|confirmations|thanks|approved|working|HeuristicAcceptanceExtractor|extract_acceptance" crates/sl-viewer crates -g '*.rs' | head -200Repository: KooshaPari/SessionLedger
Length of output: 35266
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== acceptance extractor references in implementation =="
rg -n "struct HeuristicAcceptanceExtractor|impl.*AcceptanceExtractor|USER_CONFIRMATION|user_confirmed|confirmation_patterns|confirm" crates/sl-viewer crates -g '*.rs'
echo "== source excerpts =="
fd -i 'acceptance_extractor' crates -t f -x sh -c 'echo "--- $1"; sed -n "1,220p" "$1"' sh {}Repository: KooshaPari/SessionLedger
Length of output: 17808
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
fd -i 'acceptance_extractor' . -t f
echo "== search implementation names =="
rg -n "HeuristicAcceptanceExtractor|AcceptanceExtractor|user_confirmed|USER_CONFIRMATION_PATTERNS|confirmation" crates -g '*.rs' | head -250
echo "== Rust path search for crate deps =="
rg -n "crates/session-ledger|crates/sl-viewer|session_ledger::distill::acceptance_extractor" crates -g '*.rs' | head -80Repository: KooshaPari/SessionLedger
Length of output: 12380
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== workspace manifest =="
sed -n '1,180p' Cargo.toml
echo "== session_ledger implementation excerpt =="
sed -n '1,260p' src/distill/acceptance_extractor.rs
echo "== properties acceptance top constant excerpt =="
sed -n '45,225p' crates/sl-viewer/tests/properties_session_ledger_acceptance.rs
echo "== rust toolchain =="
cat rust-toolchain.tomlRepository: KooshaPari/SessionLedger
Length of output: 18956
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
acceptance_path = Path("src/distill/acceptance_extractor.rs")
impl_text = acceptance_path.read_text()
patterns = re.search(r'const USER_CONFIRMATION_PATTERNS: &[&str] = &\[.*?\];', impl_text, re.S).group(0)
raw = patterns.split("USER_CONFIRMATION_PATTERNS")[1]
user_patterns = [s.strip().strip('"\'').strip(',').strip() for s in re.findall(r'"([^"]*)"', raw) + re.findall(r"'([^']*)'", raw.replace('"', "'")) if s.strip() and re.search(r'const USER_CONFIRMATION_PATTERNS', patterns)]
# Extract exact string literals from the actual const after USER_CONFIRMATION_PATTERNS
user_patterns = [s.strip().strip('"\'\t\n ') for s in patterns.split('USER_CONFIRMATION_PATTERNS')[1].split('];')[0].splitlines() if isinstance(s, str)]
# use exact values from const directly
text = patterns.split("USER_CONFIRMATION_PATTERNS")[1]
exact = [line.strip().strip(',').strip('".') for line in text.splitlines() if line.strip().startswith('"')]
print("user_patterns:", exact)
sample = "th3_8f029a12x7k5"
generated = re.compile(r'^[A-Za-z0-9 ._]{1,40}$').fullmatch(sample)
print("sample matches generator:", bool(generated))
print("sample contains any literal:", any(p.lower() in sample.lower() for p in exact))
print("literals that are single words matching generator:", [p for p in exact if p.replace(' ', '').replace("-", '').replace("'", "").isalnum() and re.compile(r'^[A-Za-z0-9 ._]{1,40}$').fullmatch(p)])
PYRepository: KooshaPari/SessionLedger
Length of output: 294
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
p = Path("src/distill/acceptance_extractor.rs")
text = p.read_text()
start = text.index("USER_CONFIRMATION_PATTERNS")
end = text.find("];", start) + 2
block = text[start:end]
print("block")
print(block[:800])
lines = block.splitlines()
literals = []
for line in lines:
for m in re.findall(r'"([^"]*)"', line):
s = m.strip().strip(",")
if s:
literals.append(s)
print("literals:", literals)
gen = re.compile(r"^ [a-zA-Z0-9 .]{1,40}$")
examples = ["approved", "thanks", "working", "38g1j5", ".a.b.c."]
for example in examples:
body = f" {example}"
print(f"example {example!r}: matches={bool(gen.fullmatch(body))}; detected={any(p.lower() in body.lower() for p in literals)}")
PYRepository: KooshaPari/SessionLedger
Length of output: 1067
Exclude confirmation tokens from this property input.
[a-zA-Z0-9 .]{1,40} can generate values such as approved, thanks, and working. HeuristicAcceptanceExtractor::extract_acceptance() matches these in user messages and sets user_confirmed = true, so this property can fail for valid input. Use input that cannot produce a documented user-confirmation substring, for example digits-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sl-viewer/tests/properties_session_ledger_acceptance.rs` around lines
290 - 299, Update the `user_confirmed_false_when_no_role_confirms` property
input strategy to exclude documented confirmation tokens, such as by generating
digits-only text, while preserving the existing session construction and
assertion.
Comment on lines
+587
to
+603
| /// `satisfaction_score` alone does NOT make `Acceptance` non-empty. | ||
| #[test] | ||
| fn satisfaction_score_alone_does_not_make_non_empty(_dummy in 0_u8..1) { | ||
| // Build a session that produces satisfaction_score > 0 with no | ||
| // collection findings. With the documented formula, evidence | ||
| // adds 1 to the count, so any evidence pattern will also populate | ||
| // the evidence vector. Instead, drive the score purely via the | ||
| // user-confirmation path -- a user message with a confirmation | ||
| // phrase will bump user_confirmed yet the score rises -- so | ||
| // user_confirmed alone is what makes acceptance non-empty. | ||
| let mut session = Session::new("score-only", Corpus::Forge); | ||
| session.messages.push(Message::new(Role::User, "looks good")); | ||
| let a = HeuristicAcceptanceExtractor::extract_acceptance(&session); | ||
| prop_assert!(a.user_confirmed); | ||
| prop_assert!(a.satisfaction_score > 0); | ||
| prop_assert!(!a.is_empty()); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | grep 'crates/sl-viewer/tests/properties_session_ledger_acceptance.rs'
echo "== file size =="
wc -l crates/sl-viewer/tests/properties_session_ledger_acceptance.rs
echo "== relevant sections =="
sed -n '540,620p' crates/sl-viewer/tests/properties_session_ledger_acceptance.rs
echo "== searches: HeuristicAcceptanceExtractor, is_empty, satisfaction_score, user_confirmed =="
rg -n "HeuristicAcceptanceExtractor|satisfaction_score|user_confirmed|is_empty|looks good|confirmation" crates/sl-viewer/tests/properties_session_ledger_acceptance.rs crates -g '*.rs' | head -200Repository: KooshaPari/SessionLedger
Length of output: 29584
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate files mentioning HeuristicAcceptanceExtractor ==="
rg -l "HeuristicAcceptanceExtractor|satisfaction_score|user_confirmed" crates/sl-viewer crates -g '*.rs' || true
echo "== git diff stat/name-status for context =========="
git diff --stat
git diff -- crates/sl-viewer/tests/properties_session_ledger_acceptance.rs | sed -n '1,220p' || trueRepository: KooshaPari/SessionLedger
Length of output: 470
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== all matching occurrences of HeuristicAcceptanceExtractor =="
rg -n --hidden --glob '!target/**' 'HeuristicAcceptanceExtractor|struct Acceptance|impl Acceptance|fn is_empty|user_confirmed|satisfaction_score|evidence' . | sed -n '1,240p'
echo "== source files with matching occurrences =="
rg -l --hidden --glob '!target/**' 'HeuristicAcceptanceExtractor|struct Acceptance|impl Acceptance|user_confirmed|satisfaction_score|evidence' . || trueRepository: KooshaPari/SessionLedger
Length of output: 41077
Rename or replace this property.
This setup sets user_confirmed and satisfaction_score to positive values. It therefore tests that user confirmation plus scoring makes Acceptance non-empty, not score-only behavior. Rename the property to describe that condition, or remove it because the score does not increase without evidence or user confirmation.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sl-viewer/tests/properties_session_ledger_acceptance.rs` around lines
587 - 603, Rename satisfaction_score_alone_does_not_make_non_empty and its
documentation to describe the actual user-confirmation behavior being tested.
Update the test wording to state that the confirmation phrase sets
user_confirmed and increases satisfaction_score, while retaining the existing
non-empty assertion; do not label this as score-only behavior.
Comment on lines
+398
to
+405
| let a = HeuristicContextExtractor::extract_context(&session); | ||
| let b = HeuristicContextExtractor::extract_context(&session); | ||
| prop_assert_eq!(a.cwd, b.cwd); | ||
| prop_assert_eq!(a.title, b.title); | ||
| prop_assert_eq!(a.files_mentioned, b.files_mentioned); | ||
| prop_assert_eq!(a.key_symbols, b.key_symbols); | ||
| prop_assert_eq!(a.environment_notes, b.environment_notes); | ||
| prop_assert_eq!(a.key_decisions.len(), b.key_decisions.len()); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== candidate files =="
git ls-files | rg '(^|/)properties_session_ledger_context\.rs$|HeuristicContextExtractor|key_decisions' || true
echo
echo "== target file outline =="
ast-grep outline crates/sl-viewer/tests/properties_session_ledger_context.rs --view compact 2>/dev/null || true
echo
echo "== relevant sections =="
sed -n '360,445p' crates/sl-viewer/tests/properties_session_ledger_context.rs 2>/dev/null || true
echo
echo "== HeuristicContextExtractor definitions/usages =="
rg -n "struct HeuristicContextExtractor|impl[^{]*HeuristicContextExtractor|fn extract_context|fn extract\\(" crates/sl-viewer -S || trueRepository: KooshaPari/SessionLedger
Length of output: 4447
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target imports and make_session =="
sed -n '1,120p' crates/sl-viewer/tests/properties_session_ledger_context.rs || true
echo
echo "== local tests around extract methods =="
rg -n "HeuristicContextExtractor::|trait_path_matches_associated_function|extract_context_is_deterministic|struct HeuristicContextExtractor|impl HeuristicContextExtractor|key_decisions" crates/sl-viewer/tests/properties_session_ledger_context.rs crates -S -g'*.rs' || true
echo
echo "== repository-wide key_decisions occurrences =="
rg -n "key_decisions|struct Context|enum KeyDecision|Decisions|decision" crates --glob '*.rs' || trueRepository: KooshaPari/SessionLedger
Length of output: 19446
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== git status =="
git status --short
echo
echo "== search context_extractor in entire repo =="
rg -n "context_extractor|ContextExtractor|KeyDecision|key_decisions|struct Context" . -S -g'*.rs' || true
echo
echo "== all context extractor test files =="
fd -e rs 'properties_session_ledger_context|context_extractor|ContextMatcher|Context' . | sortRepository: KooshaPari/SessionLedger
Length of output: 14664
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== core context and decision types =="
sed -n '1,130p' src/domain/context.rs
echo
echo "== context extractor decision construction =="
sed -n '70,175p' src/distill/context_extractor.rs
echo
echo "== ports ContextExtractor =="
sed -n '90,115p' src/ports/mod.rsRepository: KooshaPari/SessionLedger
Length of output: 9220
Compare full Decision values.
Decision derives PartialEq, so comparing a.key_decisions == b.key_decisions and via_fn.key_decisions == via_trait.key_decisions catches changed summaries, changed rationales, and ordering drift.
📍 Affects 1 file
crates/sl-viewer/tests/properties_session_ledger_context.rs#L398-L405(this comment)crates/sl-viewer/tests/properties_session_ledger_context.rs#L422-L430
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sl-viewer/tests/properties_session_ledger_context.rs` around lines 398
- 405, Update both decision comparisons in
crates/sl-viewer/tests/properties_session_ledger_context.rs at lines 398-405 and
422-430 to compare the complete key_decisions collections, using Decision’s
PartialEq, rather than only comparing their lengths; preserve the existing
comparisons for the other context fields.
Comment on lines
+451
to
+462
| let all_empty = ctx.cwd.is_none() | ||
| && ctx.title.is_none() | ||
| && ctx.files_mentioned.is_empty() | ||
| && ctx.key_decisions.is_empty() | ||
| && ctx.key_symbols.is_empty() | ||
| && ctx.environment_notes.is_empty(); | ||
| if all_empty { | ||
| prop_assert!(ctx.is_empty()); | ||
| } | ||
| if !ctx.is_empty() { | ||
| prop_assert!(!all_empty); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching relevant properties:"
fd -a 'properties_session_ledger_(context|contract|acceptance)\.rs$' .
for f in \
crates/sl-viewer/tests/properties_session_ledger_context.rs \
crates/sl-viewer/tests/properties_session_ledger_contract.rs \
crates/sl-viewer/tests/properties_session_ledger_acceptance.rs
do
if [ -f "$f" ]; then
echo
echo "===== $f ====="
wc -l "$f"
sed -n '340,475p' "$f" | cat -n
else
echo "missing: $f"
fi
doneRepository: KooshaPari/SessionLedger
Length of output: 15628
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching relevant properties:"
fd -a 'properties_session_ledger_(context|contract|acceptance)\.rs$' .
for f in \
crates/sl-viewer/tests/properties_session_ledger_context.rs \
crates/sl-viewer/tests/properties_session_ledger_contract.rs \
crates/sl-viewer/tests/properties_session_ledger_acceptance.rs
do
if [ -f "$f" ]; then
echo
echo "===== $f ====="
wc -l "$f"
sed -n '340,-p' "$f" | sed -n '340,475p' "$f" | cat -n
else
echo "missing: $f"
fi
doneRepository: KooshaPari/SessionLedger
Length of output: 6911
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Acceptance is_empty/finding block after success-score tests:"
sed -n '560,595p' crates/sl-viewer/tests/properties_session_ledger_acceptance.rs | cat -n
echo
echo "Contract full block:"
sed -n '356,369p' crates/sl-viewer/tests/properties_session_ledger_contract.rs | cat -n
echo
echo "Find is_empty implementations by name:"
rg -n "impl .*Is.*Empty|fn is_empty\\(&self\\) -> bool|pub fn is_empty\\(&self\\) -> bool" crates/sl-viewer/src crates --glob '*.rs'
echo
echo "Struct definitions for is_empty candidates:"
rg -n -C 4 "pub struct.*(Context|Contract|Acceptance)|impl.*(Context|Contract|Acceptance)|fn is_empty" crates/sl-viewer/src --glob '*.rs'Repository: KooshaPari/SessionLedger
Length of output: 5119
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Heuristic extractor locations:"
rg -n "Heuristic(Context|Contract|Acceptance)Extractor|extract_context|extract_contract|extract_acceptance|Context|ContractExtractor|AcceptanceExtractor" crates/sl-viewer/src crates/sl-viewer/tests --glob '*.rs'
echo
echo "Candidate files with relevant names:"
fd -i 'context|contract|acceptance|extractor' crates/sl-viewer/src crates/sl-viewer/tests -e rsRepository: KooshaPari/SessionLedger
Length of output: 18784
🏁 Script executed:
#!/bin/bash
set -euo_pipefail
echo "Extract distill source files:"
fd -a 'context_extractor.rs|contract_extractor.rs|acceptance_extractor.rs|context.rs|contract.rs|acceptance.rs' crates/sl-viewer/src crates/sl-viewer/tests --exclude '*.rs.lock' | sed -n '1,200p'
echo
echo "Search is_empty in distill/port domain files only:"
rg -n "impl .*for (Context|Contract|Acceptance)|impl.*Empty|pub fn is_empty|fn is_empty|MUST BE|contract found|context found|acceptance found" crates/sl-viewer/src crates --glob '*.rs'Repository: KooshaPari/SessionLedger
Length of output: 815
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Extract distill source files:"
fd -a 'context_extractor.rs|contract_extractor.rs|acceptance_extractor.rs|context.rs|contract.rs|acceptance.rs' crates/sl-viewer/src crates/sl-viewer/tests --exclude '*.rs.lock' | sed -n '1,200p'
echo
echo "Search is_empty in distill/port/domain files only:"
rg -n "impl .*for (Context|Contract|Acceptance)|impl.*Empty|pub fn is_empty|fn is_empty|MUST BE|contract found|context found|acceptance found" crates/sl-viewer/src crates --glob '*.rs'Repository: KooshaPari/SessionLedger
Length of output: 1210
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
# Read-only behavioral probe: prove the second assertion is the contrapositive of the first
# and can pass when is_empty() is wrong on non-empty findings.
cases = {
"finding_without_is_empty": (False, True),
"empty_with_is_empty": (True, False),
"perfect": (True, True),
"no_findings_no_is_empty": (False, False),
"is_empty_fallback_true": (True, True),
}
for name, (all_empty, is_empty) in cases.items():
property_assumed = (not all_empty) or is_empty
second_assert_case = "ok" if (not is_empty) or (not all_empty) else "fails"
print(f"{name:30s}: all_empty={all_empty!s:5} is_empty={is_empty!s:5} property_ok={property_assumed!s:5} second_assert={second_assert_case}")
PYRepository: KooshaPari/SessionLedger
Length of output: 658
Assert is_empty() equivalence directly.
The second assertion is only the contrapositive of the first; it still passes when is_empty() incorrectly stays true while findings are present. Replace both assertions with a single prop_assert_eq!(value.is_empty(), all_empty).
crates/sl-viewer/tests/properties_session_ledger_context.rs#L100-L124: assert equality betweenctx.is_empty()andall_empty.crates/sl-viewer/tests/properties_session_ledger_contract.rs#L359-L370: assert equality betweenc.is_empty()andall_empty.crates/sl-viewer/tests/properties_session_ledger_acceptance.rs#L564-L589: assert equality betweena.is_empty()andall_empty.
📍 Affects 3 files
crates/sl-viewer/tests/properties_session_ledger_context.rs#L451-L462(this comment)crates/sl-viewer/tests/properties_session_ledger_contract.rs#L360-L369crates/sl-viewer/tests/properties_session_ledger_acceptance.rs#L576-L584
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sl-viewer/tests/properties_session_ledger_context.rs` around lines 451
- 462, Replace the two one-way assertions with a direct equality assertion
between the context’s is_empty() result and all_empty in
crates/sl-viewer/tests/properties_session_ledger_context.rs:451-462. Apply the
same change to the corresponding assertions for c in
crates/sl-viewer/tests/properties_session_ledger_contract.rs:360-369 and a in
crates/sl-viewer/tests/properties_session_ledger_acceptance.rs:576-584,
preserving each test’s existing all_empty calculation.
Comment on lines
+195
to
+208
| proptest! { | ||
| /// All patterns are detected case-insensitively. | ||
| #[test] | ||
| 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.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Repository files matching target:"
fd -a 'properties_session_ledger_contract\.rs$' . || true
file="$(fd 'properties_session_ledger_contract\.rs$' . | head -n 1 || true)"
if [ -n "${file:-}" ]; then
echo "Target file: $file"
echo "Line count:"
wc -l "$file"
echo
echo "Relevant section around property:"
sed -n '170,225p' "$file" | nl -ba -v170
echo
echo "Pattern identifiers and tests nearby:"
rg -n "CRITERIA_PATTERNS|TEST_PATTERNS|CONSTRAINT_PATTERNS|DO_NOT_TOUCH_PATTERNS|case_insensitive|is_empty|contract" "$file"
fi
echo
echo "All usages of those identifiers in sl-viewer:"
rg -n "CRITERIA_PATTERNS|TEST_PATTERNS|CONSTRAINT_PATTERNS|DO_NOT_TOUCH_PATTERNS|HeuristicContractExtractor::extract_contract|success_criteria|test_patterns|constraint_patterns|do_not_touch_patterns" crates/sl-viewer || trueRepository: KooshaPari/SessionLedger
Length of output: 507
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/sl-viewer/tests/properties_session_ledger_contract.rs"
echo "Target file: $file"
printf '\n--- Lines 180-215 (property) ---\n'
sed -n '180,215p' "$file" | awk '{printf "%7i %s\n", NR+179, $0}'
printf '\n--- Pattern/relevant identifier references ---\n'
rg -n "CRITERIA_PATTERNS|TEST_PATTERNS|CONSTRAINT_PATTERNS|DO_NOT_TOUCH_PATTERNS|case_insensitive|is_empty|HeuristicContractExtractor::extract_contract|make_session|Result" "$file"
printf '\n--- Extractor tests mentioning pattern sections ---\n'
rg -n "patterns|Criteria|Test|Constraint|DoNotTouch|do not touch|no touch" "$file"Repository: KooshaPari/SessionLedger
Length of output: 7213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="crates/sl-viewer/tests/properties_session_ledger_contract.rs"
echo "--- Lines 1-50 (pattern constants and empty session shape) ---"
sed -n '1,50p' "$file" | awk '{printf "%7i %s\n", NR, $0}'
echo "--- Lines 80-190 (pattern coverage tests) ---"
sed -n '80,190p' "$file" | awk '{printf "%7i %s\n", NR+79, $0}'
echo "--- Locate extractor tests source ---"
fd -i 'contract.*extractor|heuristic.*extractor|extractor.*contract' crates/sl-viewer -t f
rg -n "struct HeuristicContractExtractor|impl HeuristicContractExtractor|fn extract_contract|success_criteria|tests_or_verifications|constraints|do_not_touch" crates/sl-viewer -SRepository: KooshaPari/SessionLedger
Length of output: 18275
Cover uppercase detection for every contract pattern set.
all_patterns_case_insensitive only exercises CRITERIA_PATTERNS, so TEST_PATTERNS, CONSTRAINT_PATTERNS, and DO_NOT_TOUCH_PATTERNS can regress to case-sensitive matching and still pass. Add uppercase cases for all four sections or use a single property over all documented patterns.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/sl-viewer/tests/properties_session_ledger_contract.rs` around lines
195 - 208, Extend all_patterns_case_insensitive to cover TEST_PATTERNS,
CONSTRAINT_PATTERNS, and DO_NOT_TOUCH_PATTERNS in addition to CRITERIA_PATTERNS.
Add uppercase property cases for each pattern set, preserving the existing
session extraction and non-empty assertion for the corresponding contract
section.
Shards the unique acceptance_extractor proptest from PR #486 onto main without the obsolete pre-rename daemon source churn.
KooshaPari
force-pushed
the
fix/viewer-acceptance-properties-20260809
branch
from
August 12, 2026 08:10
df12dec to
6995da2
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:XL
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
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.
User description
WBS-6.2 surface 6: session-ledger distill pipeline.
Branch off fix/viewer-mock-data-properties-20260809 rebased main. Test file compiles and passes locally.
CodeAnt-AI Description
Add property coverage for session acceptance evidence extraction
What Changed
Impact
✅ Fewer regressions in continuation acceptance signals✅ Accurate user-confirmation detection✅ Consistent acceptance scores and evidence output💡 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.