test(sl-viewer): command_palette + daemon_url properties (28 props) - #507
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 · |
|
Warning Review limit reached
Next review available in: 42 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 (2)
📝 WalkthroughSummaryThe PR adds property-based tests across The PR should not merge because Must Fix
Should Fix
Consider
Request ChangesResolve the invalid Rust source and provide passing format, clippy, and workspace test results. WalkthroughThe pull request adds bounded daemon bundle search, property-based tests for viewer and envelope contracts, an MIT license, session handoff records, and hosted verification logs. It also leaves invalid merge-conflict syntax in ChangesSessionLedger changes
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SearchRequest
participant search_bundles
participant read_matching_bundle_metas
participant BundleFiles
SearchRequest->>search_bundles: submit query and limit
search_bundles->>read_matching_bundle_metas: pass filter specification
read_matching_bundle_metas->>BundleFiles: scan .okf.json files
BundleFiles-->>read_matching_bundle_metas: parseable or unreadable file
read_matching_bundle_metas-->>search_bundles: bounded matching metadata
search_bundles-->>SearchRequest: return search results
🚥 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 |
| let Ok(contents) = std::fs::read_to_string(&path) else { | ||
| continue; | ||
| }; | ||
| let Ok(value) = serde_json::from_str::<Value>(&contents) else { | ||
| continue; | ||
| }; | ||
| let meta = BundleMeta::from_value(&value); |
There was a problem hiding this comment.
Suggestion: The new search path still reads each candidate file into a full String and parses the entire document into a serde_json::Value before extracting metadata. A single large OKF payload therefore remains fully allocated during the request, so the advertised memory reduction and early-limit behavior are not achieved for large entity or message arrays; extract only the metadata fields from a streaming or selective deserializer. [performance]
Severity Level: Major ⚠️
- ⚠️ `/api/search` allocates entire candidate payloads.
- ⚠️ Large bundles increase daemon search memory pressure.
- ⚠️ Result limits do not limit per-file parsing.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-daemon/src/http.rs
**Line:** 1136:1142
**Comment:**
*Performance: The new search path still reads each candidate file into a full `String` and parses the entire document into a `serde_json::Value` before extracting metadata. A single large OKF payload therefore remains fully allocated during the request, so the advertised memory reduction and early-limit behavior are not achieved for large entity or message arrays; extract only the metadata fields from a streaming or selective deserializer.
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| <<<<<<< Updated upstream | ||
| <<<<<<< Updated upstream | ||
| <<<<<<< Updated upstream | ||
| <<<<<<< Updated upstream | ||
| <<<<<<< Updated upstream | ||
| use crate::mock_data::sample_sessions; | ||
| ======= | ||
| use crate::web_exports::*; | ||
| >>>>>>> Stashed changes | ||
| ======= | ||
| use crate::web_exports::*; | ||
| >>>>>>> Stashed changes | ||
| ======= | ||
| use crate::web_exports::*; | ||
| >>>>>>> Stashed changes | ||
| ======= | ||
| use crate::web_exports::*; | ||
| >>>>>>> Stashed changes | ||
| ======= | ||
| use crate::web_exports::*; | ||
| >>>>>>> Stashed changes |
There was a problem hiding this comment.
Suggestion: The import section contains unresolved merge markers and leaves the required sample_sessions import inside an unmerged branch. This prevents the viewer module from resolving its mock-data dependency and must be replaced with one clean, intentional import block before merging. [import error]
Severity Level: Critical 🚨
- ❌ `sl-viewer` compilation fails immediately.
- ❌ Viewer tests cannot build or execute.
- ❌ Mock corpus loading remains unavailable.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/src/corpus_loader.rs
**Line:** 17:37
**Comment:**
*Import Error: The import section contains unresolved merge markers and leaves the required `sample_sessions` import inside an unmerged branch. This prevents the viewer module from resolving its mock-data dependency and must be replaced with one clean, intentional import block before merging.
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 daemon_base_url_has_no_trailing_slash(_unused in 0u8..1u8) { | ||
| let base = daemon_base_url(); | ||
| prop_assert!( | ||
| !base.ends_with('/'), | ||
| "daemon base URL must not end with / (got {:?})", | ||
| base, | ||
| ); |
There was a problem hiding this comment.
Suggestion: This property rejects any compile-time SL_DAEMON_URL ending in /, even though both production helpers explicitly normalize trailing slashes with trim_end_matches('/'). A deployment configured with a valid base such as http://daemon.example/ will therefore fail the test despite producing the same correct API URLs and host display; assert the helper's normalized behavior instead of imposing a stricter raw-environment constraint. [logic error]
Severity Level: Major ⚠️
- ❌ Configured viewer builds fail property tests.
- ⚠️ `/api/search` URL normalization still works.
- ⚠️ Host display normalization still works.Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_daemon_url.rs
**Line:** 201:207
**Comment:**
*Logic Error: This property rejects any compile-time `SL_DAEMON_URL` ending in `/`, even though both production helpers explicitly normalize trailing slashes with `trim_end_matches('/')`. A deployment configured with a valid base such as `http://daemon.example/` will therefore fail the test despite producing the same correct API URLs and host display; assert the helper's normalized behavior instead of imposing a stricter raw-environment constraint.
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 fixThere was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_viewer_command_palette.rs`:
- Around line 116-150: Update palette_command_equality_is_fieldwise to construct
both PaletteCommand values from the generated id, label, hint, and action
inputs, then assert that equivalent commands compare equal; retain a separate
fieldwise inequality check only if it meaningfully uses differing generated
fields, otherwise replace the property with a focused unit test for the fixed
inequality case.
- Around line 152-174: Update the property tests
palette_command_equality_is_fieldwise and palette_command_is_copy_and_clone to
exercise generated inputs by constructing commands from their strategies, or
remove strategies that are not used. Add a generic T: Clone helper for clone
assertions, while retaining direct assignment for Copy checks and avoiding
direct .clone() calls in the test bodies.
In `@crates/sl-viewer/tests/properties_viewer_daemon_url.rs`:
- Around line 31-34: Update the property-test path strategy and its adjacent
comment so they agree: either replace the ASCII-only pattern with a
Unicode-capable generator to cover arbitrary Unicode paths, or revise the
comment to explicitly state that only ASCII paths are tested.
In `@HANDOFF-session-2026-08-05.md`:
- Line 85: Add a single blank line after each affected Markdown heading in the
handoff document, including between the “### Brand asset suite” heading and its
table, covering all referenced sections. Preserve the existing heading text and
table content.
🪄 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: b73df716-6b83-41f8-a52e-b41e0003c2b6
📒 Files selected for processing (7)
HANDOFF-session-2026-08-05.mdLICENSEcrates/sl-daemon/src/http.rscrates/sl-viewer/src/corpus_loader.rscrates/sl-viewer/tests/properties_viewer_command_palette.rscrates/sl-viewer/tests/properties_viewer_daemon_url.rsdocs/sessions/20260808-hosted-recheck/01_HOSTED_RECHECK.md
📜 Review details
⏰ Context from checks skipped due to timeout. (36)
- GitHub Check: Trunk Check
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: race smoke + channel/cancel model · ubuntu-latest
- GitHub Check: race smoke + channel/cancel model · windows-latest
- GitHub Check: update check hard · sl-daemon tests
- GitHub Check: race smoke + channel/cancel model · macos-latest
- GitHub Check: browser e2e · axe · responsive · visual
- GitHub Check: prepare
- GitHub Check: jemalloc default-on · windows default build
- GitHub Check: loom permutation · daemon shutdown
- GitHub Check: envelope-crypto · SelfCheck
- GitHub Check: loom permutation · daemon mpsc
- GitHub Check: loom permutation · daemon broadcast
- GitHub Check: sl-daemon · repository builder image offline build / sl-daemon · repository builder image offline build
- GitHub Check: hermetic · reusable workflow provenance (soft)
- GitHub Check: Lint & Format
- GitHub Check: load macro gate · macro routes smoke
- GitHub Check: pipeline perf regression gate
- GitHub Check: sl-viewer help · unit tests
- GitHub Check: miri permutation · SelfCheck
- GitHub Check: soft loom · daemon mpsc
- GitHub Check: alloc profile hard · dhat smoke
- GitHub Check: soft loom · daemon broadcast
- GitHub Check: soft loom · loom_model core
- GitHub Check: tsan permutation · race_model
- GitHub Check: sl-daemon build · macos-latest
- GitHub Check: session-ledger build · windows-latest
- GitHub Check: exotic check · x86_64-unknown-linux-musl
- GitHub Check: visual contract · WCAG AA
- GitHub Check: compression ratio gate
- GitHub Check: sl-daemon build · windows-latest
- GitHub Check: exotic check · aarch64-unknown-linux-gnu
- GitHub Check: sl-viewer macOS app · artifact
- GitHub Check: Kilo Code Review
- GitHub Check: Summary
- GitHub Check: prepare
⚠️ CI failures not shown inline (3)
GitHub Check: Trunk Check: Trunk Check
Conclusion: failure
Checked 6 modified files
✖ 2 unformatted files
To reproduce and test locally, run:
`trunk check`
For help resolving these issues, see our docs on [running on PRs](https://docs.trunk.io/check/github-integration#checking-pull-requests) or [debugging Trunk Check](https://docs.trunk.io/check/debugging)
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 (6)
*
📄 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:
LICENSEHANDOFF-session-2026-08-05.md
**/*.{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-daemon/src/http.rscrates/sl-viewer/src/corpus_loader.rscrates/sl-viewer/tests/properties_viewer_daemon_url.rscrates/sl-viewer/tests/properties_viewer_command_palette.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
Fix Clippy warnings; do not add
#[allow]unless it includes a tracking-issue comment.
Files:
crates/sl-daemon/src/http.rscrates/sl-viewer/src/corpus_loader.rscrates/sl-viewer/tests/properties_viewer_daemon_url.rscrates/sl-viewer/tests/properties_viewer_command_palette.rs
crates/sl-daemon/**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
Use
cargo test --manifest-path crates/sl-daemon/Cargo.tomlas the fast inner-loop test command forsl-daemonchanges.
Files:
crates/sl-daemon/src/http.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/src/corpus_loader.rscrates/sl-viewer/tests/properties_viewer_daemon_url.rscrates/sl-viewer/tests/properties_viewer_command_palette.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/src/corpus_loader.rscrates/sl-viewer/tests/properties_viewer_daemon_url.rscrates/sl-viewer/tests/properties_viewer_command_palette.rs
🪛 LanguageTool
docs/sessions/20260808-hosted-recheck/01_HOSTED_RECHECK.md
[grammar] ~22-~22: Ensure spelling is correct
Context: ...becauseprepare` failed; this is not qgate pass evidence. - No pull request curre...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[uncategorized] ~68-~68: The official name of this software platform is spelled with a capital “H”.
Context: ...re; quality gate · qgate: skipped | | .github/workflows/scorecard.yml | [31251719814...
(GITHUB)
[uncategorized] ~71-~71: The official name of this software platform is spelled with a capital “H”.
Context: ..._self_check_validates_anchorsrequires.github/workflows/ci.ymlto contain therootl...
(GITHUB)
HANDOFF-session-2026-08-05.md
[style] ~137-~137: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...&str) -> &'static strlookup helper. - Addeddangerous_inner_html: "{icon_svg(tab.i...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
[style] ~182-~182: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...b-pages / per-page additional panels. - No "feed data" affordance — user cannot po...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.23.2)
HANDOFF-session-2026-08-05.md
[warning] 1-1: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 85-85: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 91-91: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 95-95: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 99-99: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 109-109: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 117-117: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 118-118: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
[warning] 128-128: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 133-133: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 140-140: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 160-160: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 164-164: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 170-170: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 173-173: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 178-178: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 184-184: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 189-189: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
[warning] 192-192: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🔇 Additional comments (3)
LICENSE (1)
1-21: LGTM!docs/sessions/20260808-hosted-recheck/01_HOSTED_RECHECK.md (1)
1-215: LGTM!crates/sl-daemon/src/http.rs (1)
939-953: 📐 Maintainability & Code QualityRun the required Rust validation with the pinned toolchain.
The supplied context does not include verification output.
crates/sl-daemon/src/http.rs#L939-L953: Runcargo test --manifest-path crates/sl-daemon/Cargo.toml.crates/sl-viewer/src/corpus_loader.rs#L17-L37: Runcargo check -p sl-viewerafter resolving the conflict.crates/sl-viewer/tests/properties_viewer_command_palette.rs#L24-L276: Run the locked all-features test suite and Clippy.crates/sl-viewer/tests/properties_viewer_daemon_url.rs#L25-L237: Run the locked all-features test suite and rustfmt check.Source: Coding guidelines
| fn palette_command_equality_is_fieldwise( | ||
| id in "[a-z-]{3,12}", | ||
| label in "[A-Za-z ]{3,20}", | ||
| hint in "[A-Za-z ]{3,30}", | ||
| action_idx in 0usize..7, | ||
| ) { | ||
| let action = match action_idx { | ||
| 0 => PaletteAction::FocusSearch, | ||
| 1 => PaletteAction::ToggleTheme, | ||
| 2 => PaletteAction::OpenHelp, | ||
| 3 => PaletteAction::OpenSettings, | ||
| 4 => PaletteAction::NextTab, | ||
| 5 => PaletteAction::PrevTab, | ||
| _ => PaletteAction::ClearSearch, | ||
| }; | ||
| let a = PaletteCommand { | ||
| id: "left", | ||
| label: "left label", | ||
| hint: "left hint", | ||
| action: PaletteAction::FocusSearch, | ||
| }; | ||
| let b = PaletteCommand { | ||
| id: "right", | ||
| label: "right label", | ||
| hint: "right hint", | ||
| action: PaletteAction::ToggleTheme, | ||
| }; | ||
| // Sanity: two constructed PaletteCommands with different fields differ. | ||
| prop_assert_ne!(a, b); | ||
|
|
||
| // The id/label/hint/action combinations drawn above aren't used | ||
| // to construct two commands; we just need the proptest harness to | ||
| // see diverse inputs. | ||
| let _ = (id, label, hint, action); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Test the generated command values.
Lines 117-120 generate id, label, hint, and action. Line 149 discards all four values. The test only checks that two fixed, different commands are unequal.
Construct equivalent commands from the test inputs and assert equality. Otherwise, replace this property with a focused unit test for the fixed inequality case.
🤖 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_viewer_command_palette.rs` around lines 116
- 150, Update palette_command_equality_is_fieldwise to construct both
PaletteCommand values from the generated id, label, hint, and action inputs,
then assert that equivalent commands compare equal; retain a separate fieldwise
inequality check only if it meaningfully uses differing generated fields,
otherwise replace the property with a focused unit test for the fixed inequality
case.
| /// Property: Copy + Clone of PaletteCommand produce an equal value. | ||
| /// (Required for the `for (i, cmd) in COMMANDS.iter().enumerate()` | ||
| /// pattern to keep working without `.clone()` noise.) | ||
| #[test] | ||
| fn palette_command_is_copy_and_clone(_unused in 0u8..1u8) { | ||
| let cmd = COMMANDS[0]; | ||
| let copied = cmd; // Copy | ||
| let cloned = cmd.clone(); // Clone | ||
| prop_assert_eq!(copied, cmd); | ||
| prop_assert_eq!(cloned, cmd); | ||
| prop_assert_eq!(copied, cloned); | ||
| } | ||
|
|
||
| /// Property: PaletteAction equality holds across Copy/Clone. | ||
| #[test] | ||
| fn palette_action_is_copy_eq(_unused in 0u8..1u8) { | ||
| let original = PaletteAction::ToggleTheme; | ||
| let copied = original; | ||
| let cloned = original.clone(); | ||
| prop_assert_eq!(original, copied); | ||
| prop_assert_eq!(original, cloned); | ||
| prop_assert_eq!(copied, cloned); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
file="crates/sl-viewer/tests/properties_viewer_command_palette.rs"
printf '%s\n' '--- file outline ---'
ast-grep outline "$file" 2>/dev/null || true
printf '%s\n' '--- relevant source ---'
sed -n '1,210p' "$file"
printf '%s\n' '--- type definitions and clone assertions ---'
rg -n -C 3 'enum Palette(Command|Action)|struct Palette(Command|Action)|derive|clone\(\)|palette_command_equality_is_fieldwise|palette_command_is_copy_and_clone|palette_action_is_copy_eq' crates/sl-viewer
printf '%s\n' '--- workspace toolchain and clippy configuration ---'
cat rust-toolchain.toml 2>/dev/null || true
rg -n -C 2 'clone_on_copy|clippy|workspace.lints|PaletteCommand|PaletteAction' Cargo.toml crates/sl-viewer/Cargo.toml crates/sl-viewer/src crates/sl-viewer/testsRepository: KooshaPari/SessionLedger
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
test = Path("crates/sl-viewer/tests/properties_viewer_command_palette.rs").read_text()
source = Path("crates/sl-viewer/src/command_palette.rs").read_text()
print("--- derive evidence ---")
for name in ("PaletteAction", "PaletteCommand"):
match = re.search(
rf"#\[derive\(([^)]*)\)\]\s*pub (?:enum|struct) {name}\b",
source,
re.S,
)
print(name, match.group(1).replace("\n", " ").strip() if match else "not found")
print("--- direct clone evidence ---")
for line_no, line in enumerate(test.splitlines(), 1):
if ".clone()" in line and line_no <= 180:
print(f"{line_no}: {line.strip()}")
print("--- generated-input usage in palette_command_equality_is_fieldwise ---")
start = test.index("fn palette_command_equality_is_fieldwise")
end = test.index(" /// Property: Copy + Clone", start)
body = test[start:end]
for name in ("id", "label", "hint", "action"):
print(f"{name}: {len(re.findall(rf'\\b{name}\\b', body))} occurrence(s)")
print(body)
print("--- toolchain availability ---")
for command in ("rustc", "clippy-driver"):
import shutil
path = shutil.which(command)
print(command, path or "not found")
PY
if command -v clippy-driver >/dev/null 2>&1; then
direct="$(mktemp --suffix=.rs)"
generic="$(mktemp --suffix=.rs)"
trap 'rm -f "$direct" "$generic"' EXIT
cat >"$direct" <<'RS'
#[derive(Clone, Copy)]
struct Value;
fn main() {
let value = Value;
let _cloned = value.clone();
}
RS
cat >"$generic" <<'RS'
#[derive(Clone, Copy)]
struct Value;
fn clone_value<T: Clone>(value: &T) -> T {
value.clone()
}
fn main() {
let value = Value;
let _cloned = clone_value(&value);
}
RS
printf '%s\n' '--- clippy direct clone_on_copy probe ---'
clippy-driver --edition=2021 -W clippy::clone_on_copy "$direct" 2>&1 || true
printf '%s\n' '--- clippy generic Clone helper probe ---'
clippy-driver --edition=2021 -W clippy::clone_on_copy "$generic" 2>&1 || true
fiRepository: KooshaPari/SessionLedger
Length of output: 409
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
import shutil
test = Path("crates/sl-viewer/tests/properties_viewer_command_palette.rs").read_text()
source = Path("crates/sl-viewer/src/command_palette.rs").read_text()
print("--- derive evidence ---")
for name in ("PaletteAction", "PaletteCommand"):
match = re.search(
rf"#\[derive\(([^)]*)\)\]\s*pub (?:enum|struct) {name}\b",
source,
re.S,
)
print(name, match.group(1).replace("\n", " ").strip() if match else "not found")
print("--- direct clone evidence ---")
for line_no, line in enumerate(test.splitlines(), 1):
if ".clone()" in line and line_no <= 180:
print(f"{line_no}: {line.strip()}")
print("--- generated-input usage in palette_command_equality_is_fieldwise ---")
start = test.index("fn palette_command_equality_is_fieldwise")
end = test.index(" /// Property: Copy + Clone", start)
body = test[start:end]
for name in ("id", "label", "hint", "action"):
count = len(re.findall(rf"\b{name}\b", body))
print(f"{name}: {count} occurrence(s)")
print(body)
print("--- toolchain availability ---")
for command in ("rustc", "clippy-driver"):
print(command, shutil.which(command) or "not found")
PY
if command -v clippy-driver >/dev/null 2>&1; then
direct="$(mktemp --suffix=.rs)"
generic="$(mktemp --suffix=.rs)"
trap 'rm -f "$direct" "$generic"' EXIT
cat >"$direct" <<'RS'
#[derive(Clone, Copy)]
struct Value;
fn main() {
let value = Value;
let _cloned = value.clone();
}
RS
cat >"$generic" <<'RS'
#[derive(Clone, Copy)]
struct Value;
fn clone_value<T: Clone>(value: &T) -> T {
value.clone()
}
fn main() {
let value = Value;
let _cloned = clone_value(&value);
}
RS
printf '%s\n' '--- clippy direct clone_on_copy probe ---'
clippy-driver --edition=2021 -W clippy::clone_on_copy "$direct" 2>&1 || true
printf '%s\n' '--- clippy generic Clone helper probe ---'
clippy-driver --edition=2021 -W clippy::clone_on_copy "$generic" 2>&1 || true
fiRepository: KooshaPari/SessionLedger
Length of output: 17263
🏁 Script executed:
#!/bin/bash
set -e
tmpdir="$(mktemp -d)"
trap 'rm -rf "$tmpdir"' EXIT
cat >"$tmpdir/direct.rs" <<'RS'
#[derive(Clone, Copy)]
struct Value;
fn main() {
let value = Value;
let _cloned = value.clone();
}
RS
cat >"$tmpdir/generic.rs" <<'RS'
#[derive(Clone, Copy)]
struct Value;
fn clone_value<T: Clone>(value: &T) -> T {
value.clone()
}
fn main() {
let value = Value;
let _cloned = clone_value(&value);
}
RS
cd "$tmpdir"
printf '%s\n' '--- clippy direct clone_on_copy probe ---'
clippy-driver --edition=2021 -W clippy::clone_on_copy direct.rs 2>&1 || true
printf '%s\n' '--- clippy generic Clone helper probe ---'
clippy-driver --edition=2021 -W clippy::clone_on_copy generic.rs 2>&1 || trueRepository: KooshaPari/SessionLedger
Length of output: 756
Exercise generated inputs and avoid direct .clone() calls
palette_command_equality_is_fieldwisediscards its generated values. Build the commands from generated data, or remove the unused strategies.- Use a generic
T: Clonehelper for theCloneassertions. Keep direct assignment for theCopyassertions.
🤖 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_viewer_command_palette.rs` around lines 152
- 174, Update the property tests palette_command_equality_is_fieldwise and
palette_command_is_copy_and_clone to exercise generated inputs by constructing
commands from their strategies, or remove strategies that are not used. Add a
generic T: Clone helper for clone assertions, while retaining direct assignment
for Copy checks and avoiding direct .clone() calls in the test bodies.
Source: Coding guidelines
| // Accept both ASCII paths and arbitrary Unicode to ensure the | ||
| // join is byte-faithful (no URL-encoding happens here — the | ||
| // server is expected to encode). | ||
| path in "[a-zA-Z0-9/_.\\-?&=]{0,40}", |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="crates/sl-viewer/tests/properties_viewer_daemon_url.rs"
printf '%s\n' '--- target file ---'
cat -n "$file"
printf '%s\n' '--- related strategies and URL assertions ---'
rg -n -C 4 'proptest|path in|Unicode|url|join|percent|encode' "$file" crates/sl-viewer/Cargo.toml Cargo.toml
printf '%s\n' '--- proptest dependency declarations ---'
rg -n -C 2 'proptest' --glob 'Cargo.toml' --glob '*.rs'Repository: KooshaPari/SessionLedger
Length of output: 31173
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
text = Path("crates/sl-viewer/tests/properties_viewer_daemon_url.rs").read_text()
pattern = re.search(r'path in "([^"]+)"', text).group(1)
print("strategy:", pattern)
print("contains explicit Unicode range:", any(ord(c) > 127 for c in pattern))
# The character class is [a-zA-Z0-9/_.\-?&=].
allowed = set("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789/_.-?&=")
samples = ["abc/def?x=1", "é", "λ", "路径"]
for sample in samples:
print(repr(sample), all(ch in allowed for ch in sample))
PY
printf '%s\n' '--- daemon_url implementation ---'
fd -i 'daemon_url.rs' crates/sl-viewer --exec sh -c 'cat -n "$1"' sh {}Repository: KooshaPari/SessionLedger
Length of output: 13299
Test Unicode paths or correct the comment.
The strategy at lines 31-34 matches only ASCII characters. It does not generate arbitrary Unicode paths. If Unicode behavior is required, use a Unicode-capable strategy. Otherwise, describe the coverage as ASCII-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_viewer_daemon_url.rs` around lines 31 - 34,
Update the property-test path strategy and its adjacent comment so they agree:
either replace the ASCII-only pattern with a Unicode-capable generator to cover
arbitrary Unicode paths, or revise the comment to explicitly state that only
ASCII paths are tested.
|
|
||
| ## 3. What the previous session actually did (compressed timeline) | ||
|
|
||
| ### Phase 1 — Initial merge-close + install |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add blank lines around headings and the table.
Add one blank line after each affected heading. Add a blank line between the ### Brand asset suite heading and the table at Line 118. This resolves the reported MD022 and MD058 violations.
Also applies to: 91-91, 95-95, 99-99, 109-109, 117-118, 128-128, 133-133, 140-140, 160-160, 164-164, 170-170, 173-173, 178-178, 184-184, 189-189, 192-192
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 85-85: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 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 `@HANDOFF-session-2026-08-05.md` at line 85, Add a single blank line after each
affected Markdown heading in the handoff document, including between the “###
Brand asset suite” heading and its table, covering all referenced sections.
Preserve the existing heading text and table content.
Source: Linters/SAST tools
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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_viewer_cli_help.rs`:
- Around line 60-71: Update help_text_mentions_keyboard_shortcuts to assert a
complete command-palette shortcut, accepting only documented forms such as
Cmd+K, Ctrl+K, or Cmd/Ctrl+K; remove the separate independent Cmd/Ctrl and K
checks while preserving the existing ? help-toggle assertion.
In `@crates/sl-viewer/tests/properties_viewer_help_overlay.rs`:
- Around line 131-168: Extend the property tests around
escape_shortcut_closes_help_overlay to validate an Escape entry in SHORTCUTS for
each documented scope: command palette, search view, replay view, and bundle
comparison, in addition to help. Match scopes case-insensitively as the existing
help check does, and assert each required scope independently.
In `@crates/sl-viewer/tests/properties_viewer_tokens.rs`:
- Around line 105-107: Extend the property tests around
every_lab_coat_constant_appears_in_tokens_css to explicitly validate
BORDER_DARK, TEXT_MUTED_DARK, and DANGER_DARK in the :root[data-theme="dark"]
CSS declaration, since lab_coat_indexed_pairs() omits them. Assert each
dark-theme value appears in the generated tokens CSS while preserving the
existing indexed-pair coverage.
🪄 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: 1920886d-83e6-4497-aa6b-b16cc8dc60bb
📒 Files selected for processing (4)
crates/sl-viewer/tests/properties_viewer_cli_help.rscrates/sl-viewer/tests/properties_viewer_help_overlay.rscrates/sl-viewer/tests/properties_viewer_menu.rscrates/sl-viewer/tests/properties_viewer_tokens.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: prepare
- GitHub Check: browser e2e · axe · responsive · visual
- GitHub Check: Summary
⚠️ 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 (4)
**/*.{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_viewer_help_overlay.rscrates/sl-viewer/tests/properties_viewer_tokens.rscrates/sl-viewer/tests/properties_viewer_cli_help.rscrates/sl-viewer/tests/properties_viewer_menu.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_viewer_help_overlay.rscrates/sl-viewer/tests/properties_viewer_tokens.rscrates/sl-viewer/tests/properties_viewer_cli_help.rscrates/sl-viewer/tests/properties_viewer_menu.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_viewer_help_overlay.rscrates/sl-viewer/tests/properties_viewer_tokens.rscrates/sl-viewer/tests/properties_viewer_cli_help.rscrates/sl-viewer/tests/properties_viewer_menu.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_viewer_help_overlay.rscrates/sl-viewer/tests/properties_viewer_tokens.rscrates/sl-viewer/tests/properties_viewer_cli_help.rscrates/sl-viewer/tests/properties_viewer_menu.rs
🔇 Additional comments (2)
crates/sl-viewer/tests/properties_viewer_menu.rs (1)
16-17: 🎯 Functional CorrectnessCorrect the documented menu-ID count to 9.
menu.rsexports 9 IDs and defines no hidden ID.ALL_IDScorrectly contains all 9 IDs.> Likely an incorrect or invalid review comment.crates/sl-viewer/tests/properties_viewer_cli_help.rs (1)
23-24: 📐 Maintainability & Code QualityRun the Rust validation suite with a working C compiler.
cargo check -p sl-viewerstops while buildingzstd-sysbecausecccannot executecc1. The remaining checks did not run. Provide the required compiler toolchain, then rerun the locked checks, all-features tests, Clippy, and rustfmt with the pinned Rust toolchain.
| /// Property: the help text mentions the documentation toggle | ||
| /// (`?` for help overlay) and the command-palette shortcut (`Cmd+K`). | ||
| #[test] | ||
| fn help_text_mentions_keyboard_shortcuts(_unused in 0u8..1u8) { | ||
| let help = help_text(); | ||
| prop_assert!(help.contains("?"), "help text must mention ? help-toggle shortcut"); | ||
| prop_assert!( | ||
| help.contains("Cmd") || help.contains("Ctrl"), | ||
| "help text must mention Cmd/Ctrl keyboard shortcut", | ||
| ); | ||
| prop_assert!(help.contains("K"), "help text must mention K palette key"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the complete command-palette shortcut.
The separate Cmd/Ctrl and K checks accept unrelated text. A missing command-palette shortcut can pass this test. Assert a complete documented spelling such as Cmd+K, Ctrl+K, or Cmd/Ctrl+K.
🤖 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_viewer_cli_help.rs` around lines 60 - 71,
Update help_text_mentions_keyboard_shortcuts to assert a complete
command-palette shortcut, accepting only documented forms such as Cmd+K, Ctrl+K,
or Cmd/Ctrl+K; remove the separate independent Cmd/Ctrl and K checks while
preserving the existing ? help-toggle assertion.
| /// Property: Escape is documented for every relevant scope where | ||
| /// the bridge in app.rs closes an overlay. We require at least the | ||
| /// 4 documented escape scopes (help overlay, command palette, | ||
| /// search view, replay view, comparison panel). | ||
| #[test] | ||
| fn shortcut_table_covers_help_shortcut(_unused in 0u8..1u8) { | ||
| prop_assert!( | ||
| SHORTCUTS.iter().any(|s| s.keys == "?"), | ||
| "SHORTCUTS missing the ? help-toggle entry", | ||
| ); | ||
| } | ||
|
|
||
| /// Property: the Cmd+K / Ctrl+K command-palette shortcut is documented. | ||
| #[test] | ||
| fn shortcut_table_covers_command_palette(_unused in 0u8..1u8) { | ||
| let covers_cmd_k = SHORTCUTS.iter().any(|s| { | ||
| (s.keys.contains("Cmd+K") || s.keys.contains("Ctrl+K")) | ||
| && (s.keys.contains("/") || s.keys.contains("or")) | ||
| }); | ||
| prop_assert!( | ||
| covers_cmd_k, | ||
| "SHORTCUTS missing the Cmd+K / Ctrl+K command-palette entry", | ||
| ); | ||
| } | ||
|
|
||
| /// Property: SHORTCUTS always includes at least one Escape row for | ||
| /// the help overlay itself. | ||
| #[test] | ||
| fn escape_shortcut_closes_help_overlay(_unused in 0u8..1u8) { | ||
| let covers_help_escape = SHORTCUTS.iter().any(|s| { | ||
| s.keys == "Escape" | ||
| && s.scope.to_lowercase().contains("help") | ||
| }); | ||
| prop_assert!( | ||
| covers_help_escape, | ||
| "SHORTCUTS must include an Escape row for the help overlay", | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Test every documented Escape scope.
The property description lists command palette, search view, replay view, and bundle comparison in addition to the help overlay. The implementation only checks the help overlay. Add one Escape assertion for each listed scope.
🤖 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_viewer_help_overlay.rs` around lines 131 -
168, Extend the property tests around escape_shortcut_closes_help_overlay to
validate an Escape entry in SHORTCUTS for each documented scope: command
palette, search view, replay view, and bundle comparison, in addition to help.
Match scopes case-insensitively as the existing help check does, and assert each
required scope independently.
| fn every_lab_coat_constant_appears_in_tokens_css( | ||
| idx in 0usize..lab_coat_indexed_pairs().len(), | ||
| ) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the omitted dark-theme values.
BORDER_DARK, TEXT_MUTED_DARK, and DANGER_DARK are excluded from lab_coat_indexed_pairs(). A changed dark CSS override can therefore pass these tests. Add assertions that each value occurs in its :root[data-theme="dark"] declaration.
Also applies to: 187-192
🤖 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_viewer_tokens.rs` around lines 105 - 107,
Extend the property tests around every_lab_coat_constant_appears_in_tokens_css
to explicitly validate BORDER_DARK, TEXT_MUTED_DARK, and DANGER_DARK in the
:root[data-theme="dark"] CSS declaration, since lab_coat_indexed_pairs() omits
them. Assert each dark-theme value appears in the generated tokens CSS while
preserving the existing indexed-pair coverage.
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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_viewer_corpus_cta.rs`:
- Around line 113-119: Update trigger_open_corpus_is_callable so the property
test does not execute sl_viewer::corpus_cta::trigger_open_corpus or spawn
platform launchers. Replace the invocation with a compile-time function-pointer
type check, or route the launcher through an injectable test dependency while
preserving coverage that the action is callable across build configurations.
In `@crates/sl-viewer/tests/properties_viewer_corpus_paths.rs`:
- Around line 183-195: Update default_config_path_resolves_on_this_platform to
handle both default_config_path() outcomes: accept the fallback path under the
current working directory when config_dir() is unavailable, and require the path
to contain “SessionLedger” only when the platform configuration directory branch
is used.
- Around line 50-63: In the tests empty_is_empty and
config_with_paths_is_not_empty, replace both custom_paths length comparisons
with cfg.custom_paths.is_empty() and its negation, preserving the existing
assertions.
In `@crates/sl-viewer/tests/properties_viewer_detail_pane.rs`:
- Around line 60-75: Add a second property-test case alongside the existing
empty-bundle case, constructing populated Intent, Context, and Contract bundles
with nonzero token estimates and passing them to extract_detail. Assert the
expected values for every extracted field—intent_goal, acceptance_signals,
constraints, context_cwd, context_title, contract_criteria, and
total_token_estimate—while preserving the current empty case and its default
assertions.
In `@crates/sl-viewer/tests/properties_viewer_fixture.rs`:
- Around line 91-100: Update the empty-name property test function
empty_name_never_matches to assert that query_fixture_active("") returns false
using prop_assert!, rather than discarding the result; preserve the existing
visual_fixture_active and query_fixture_name checks.
In `@crates/sl-viewer/tests/properties_viewer_theme.rs`:
- Around line 73-76: Update is_lab_coat_hex to validate that every character
after the leading '#' is an ASCII digit or lowercase letter a through f, while
preserving the existing seven-character and prefix checks.
- Around line 57-68: The direct clone calls in theme_derives_hold and the
corresponding Settings property test trigger clone_on_copy; replace each
sample.clone() with std::clone::Clone::clone(&sample) while retaining both
equality assertions. Apply this in
crates/sl-viewer/tests/properties_viewer_theme.rs lines 57-68 and
crates/sl-viewer/tests/properties_viewer_settings.rs lines 192-207.
In `@crates/sl-viewer/tests/properties_viewer_web_exports.rs`:
- Around line 120-145: Update roots_with_no_explicit_only_returns_existing and
roots_with_no_explicit_under_downloads to create a temporary home directory
containing Downloads/ChatGPT, Downloads/Claude, and Downloads/Gemini before
calling web_export_roots_with_env. Assert the exact expected provider-to-path
pairs and retain the existing path validity and Downloads-root expectations.
In `@tests/properties_envelope.rs`:
- Around line 8-10: The envelope currently permits unauthenticated decryption
and treats wrong-key ciphertext as successful. Update the envelope
implementation around open to use authenticated encryption, requiring
authentication failure from modified ciphertext or an incorrect key to return
Err; revise the properties in the test module to assert rejection rather than
non-original plaintext, and add coverage for both tampering and wrong-key cases.
- Around line 28-34: Update the property tests around envelope behavior,
including envelope_key_env_is_documented and the existing envelope
round-trip/rejection tests, so generated plaintext vectors drive successful
encode/decode assertions and generated malformed strings drive rejection
assertions. Remove the unused constant-range input and preserve the documented
environment-key assertion as a separate deterministic check.
- Around line 69-72: Update the character assertion in the blob validation loop
to accept only ':' or lowercase hexadecimal characters, using matches! with
'0'..='9' and 'a'..='f' instead of is_ascii_lowercase(). Preserve the existing
error message and iteration behavior.
- Around line 36-40: Update the plain environment-mutating tests and their
helper around the SL_ENVELOPE_KEY setup to use an actual serialization mechanism
such as the project’s serial-test attribute or guard, rather than relying on the
module name. Add an RAII environment guard that captures the prior
SL_ENVELOPE_KEY value, restores it on drop including during panics, and ensure
the short-key test does not leave “deadbeef” set when no value previously
existed.
🪄 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: c98f855e-38a5-4a78-a8bd-b0157ba113b0
📒 Files selected for processing (9)
crates/sl-viewer/tests/properties_viewer_bundle_diff.rscrates/sl-viewer/tests/properties_viewer_corpus_cta.rscrates/sl-viewer/tests/properties_viewer_corpus_paths.rscrates/sl-viewer/tests/properties_viewer_detail_pane.rscrates/sl-viewer/tests/properties_viewer_fixture.rscrates/sl-viewer/tests/properties_viewer_settings.rscrates/sl-viewer/tests/properties_viewer_theme.rscrates/sl-viewer/tests/properties_viewer_web_exports.rstests/properties_envelope.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (3)
- GitHub Check: Kilo Code Review
- GitHub Check: Summary
- GitHub Check: prepare
⚠️ 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 (4)
**/*.{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_viewer_fixture.rstests/properties_envelope.rscrates/sl-viewer/tests/properties_viewer_detail_pane.rscrates/sl-viewer/tests/properties_viewer_corpus_paths.rscrates/sl-viewer/tests/properties_viewer_bundle_diff.rscrates/sl-viewer/tests/properties_viewer_theme.rscrates/sl-viewer/tests/properties_viewer_corpus_cta.rscrates/sl-viewer/tests/properties_viewer_web_exports.rscrates/sl-viewer/tests/properties_viewer_settings.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_viewer_fixture.rstests/properties_envelope.rscrates/sl-viewer/tests/properties_viewer_detail_pane.rscrates/sl-viewer/tests/properties_viewer_corpus_paths.rscrates/sl-viewer/tests/properties_viewer_bundle_diff.rscrates/sl-viewer/tests/properties_viewer_theme.rscrates/sl-viewer/tests/properties_viewer_corpus_cta.rscrates/sl-viewer/tests/properties_viewer_web_exports.rscrates/sl-viewer/tests/properties_viewer_settings.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_viewer_fixture.rscrates/sl-viewer/tests/properties_viewer_detail_pane.rscrates/sl-viewer/tests/properties_viewer_corpus_paths.rscrates/sl-viewer/tests/properties_viewer_bundle_diff.rscrates/sl-viewer/tests/properties_viewer_theme.rscrates/sl-viewer/tests/properties_viewer_corpus_cta.rscrates/sl-viewer/tests/properties_viewer_web_exports.rscrates/sl-viewer/tests/properties_viewer_settings.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_viewer_fixture.rscrates/sl-viewer/tests/properties_viewer_detail_pane.rscrates/sl-viewer/tests/properties_viewer_corpus_paths.rscrates/sl-viewer/tests/properties_viewer_bundle_diff.rscrates/sl-viewer/tests/properties_viewer_theme.rscrates/sl-viewer/tests/properties_viewer_corpus_cta.rscrates/sl-viewer/tests/properties_viewer_web_exports.rscrates/sl-viewer/tests/properties_viewer_settings.rs
🔇 Additional comments (2)
tests/properties_envelope.rs (1)
160-176: 📐 Maintainability & Code QualityRemove the Rust 2024 compatibility requirement.
The workspace uses edition 2021, so Rust 1.85 does not require
unsafeblocks for these environment calls. The validation command failed while compilingzstd-sysbecause the sandbox could not executecc1; it did not expose a Rust compatibility failure.> Likely an incorrect or invalid review comment.crates/sl-viewer/tests/properties_viewer_bundle_diff.rs (1)
17-275: LGTM!
| /// Property: `trigger_open_corpus()` is callable from any build | ||
| /// configuration and never panics. (Web builds mount a file picker, | ||
| /// desktop builds open the quick-start, headless is no-op.) | ||
| #[test] | ||
| fn trigger_open_corpus_is_callable(_unused in 0u8..1u8) { | ||
| sl_viewer::corpus_cta::trigger_open_corpus(); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- test file ---'
cat -n crates/sl-viewer/tests/properties_viewer_corpus_cta.rs | sed -n '1,180p'
printf '%s\n' '--- corpus_cta references and implementation ---'
rg -n -C 5 'trigger_open_corpus|quick.?start|file.?picker' crates/sl-viewer
printf '%s\n' '--- relevant Cargo configuration ---'
rg -n -C 3 'proptest|properties_viewer_corpus_cta|crate-type|dioxus' crates/sl-viewer Cargo.tomlRepository: KooshaPari/SessionLedger
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
source = Path("crates/sl-viewer/src/corpus_cta.rs").read_text()
manifest = Path("crates/sl-viewer/Cargo.toml").read_text()
test = Path("crates/sl-viewer/tests/properties_viewer_corpus_cta.rs").read_text()
# Extract each cfg-gated trigger_open_corpus definition and its body.
pattern = re.compile(
r'(?P<cfg>#\[cfg\([^\]]+\)\])\s*'
r'pub fn trigger_open_corpus\(\)\s*\{(?P<body>.*?)\n\}',
re.S,
)
definitions = pattern.findall(source)
print(f"trigger_open_corpus definitions: {len(definitions)}")
for cfg, body in definitions:
print(f"{cfg}: body={body.strip()!r}")
print(f"test invokes trigger_open_corpus: "
f"{test.count('sl_viewer::corpus_cta::trigger_open_corpus();')}")
print(f"default features: "
f"{re.search(r'^default\\s*=\\s*\\[(.*?)\\]', manifest, re.M).group(1)}")
desktop_body = next(
body for cfg, body in definitions if 'feature = "desktop"' in cfg
)
print(f"desktop implementation delegates to open_quickstart_desktop: "
f"{'open_quickstart_desktop();' in desktop_body}")
print(f"desktop launcher calls present: "
f"{all(x in source for x in ['Command::new(\"cmd\")', 'Command::new(\"open\")', 'Command::new(\"xdg-open\")'])}")
# Check whether the proposed replacement is a type-only reference.
replacement = "let _: fn() = sl_viewer::corpus_cta::trigger_open_corpus;"
print(f"proposed replacement calls function: "
f"{'trigger_open_corpus();' in replacement}")
PY
printf '%s\n' '--- exact implementation and feature declarations ---'
cat -n crates/sl-viewer/src/corpus_cta.rs | sed -n '20,122p'
cat -n crates/sl-viewer/Cargo.toml | sed -n '42,56p'Repository: KooshaPari/SessionLedger
Length of output: 409
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
import re
source = Path("crates/sl-viewer/src/corpus_cta.rs").read_text()
manifest = Path("crates/sl-viewer/Cargo.toml").read_text()
test = Path("crates/sl-viewer/tests/properties_viewer_corpus_cta.rs").read_text()
pattern = re.compile(
r'(?P<cfg>#\[cfg\([^\]]+\)\])\s*'
r'pub fn trigger_open_corpus\(\)\s*\{(?P<body>.*?)\n\}',
re.S,
)
definitions = pattern.findall(source)
print(f"trigger_open_corpus definitions: {len(definitions)}")
for cfg, body in definitions:
print(f"{cfg}: body={body.strip()!r}")
print("test invokes trigger_open_corpus:",
test.count("sl_viewer::corpus_cta::trigger_open_corpus();"))
default_match = re.search(r"^default\s*=\s*\[(.*?)\]", manifest, re.M)
print("default features:", default_match.group(1) if default_match else "<not found>")
desktop_body = next(
body for cfg, body in definitions if 'feature = "desktop"' in cfg
)
print("desktop implementation delegates to open_quickstart_desktop:",
"open_quickstart_desktop();" in desktop_body)
print("desktop launcher calls present:",
all(x in source for x in [
'Command::new("cmd")',
'Command::new("open")',
'Command::new("xdg-open")',
]))
replacement = "let _: fn() = sl_viewer::corpus_cta::trigger_open_corpus;"
print("proposed replacement calls function:",
"trigger_open_corpus();" in replacement)
PY
printf '%s\n' '--- exact implementation and feature declarations ---'
cat -n crates/sl-viewer/src/corpus_cta.rs | sed -n '20,122p'
cat -n crates/sl-viewer/Cargo.toml | sed -n '42,56p'Repository: KooshaPari/SessionLedger
Length of output: 8800
Do not invoke the production open action from this property test.
The default desktop feature makes trigger_open_corpus() spawn cmd, open, or xdg-open. Replace the call with a function-pointer type check, or test the launcher through injection.
🤖 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_viewer_corpus_cta.rs` around lines 113 -
119, Update trigger_open_corpus_is_callable so the property test does not
execute sl_viewer::corpus_cta::trigger_open_corpus or spawn platform launchers.
Replace the invocation with a compile-time function-pointer type check, or route
the launcher through an injectable test dependency while preserving coverage
that the action is callable across build configurations.
| fn empty_is_empty(_unused in 0u8..1u8) { | ||
| let cfg = CorpusPathConfig::empty(); | ||
| prop_assert!(cfg.is_empty()); | ||
| prop_assert_eq!(cfg.custom_paths.len(), 0); | ||
| } | ||
|
|
||
| /// Property: a config with paths is `is_empty() == false`. | ||
| #[test] | ||
| fn config_with_paths_is_not_empty( | ||
| paths in prop::collection::vec(".*", 1..5).prop_map(|v| v.into_iter().map(PathBuf::from).collect()), | ||
| ) { | ||
| let cfg = CorpusPathConfig { custom_paths: paths }; | ||
| prop_assert!(!cfg.is_empty()); | ||
| prop_assert!(cfg.custom_paths.len() >= 1); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
file="crates/sl-viewer/tests/properties_viewer_corpus_paths.rs"
printf '%s\n' "--- target file ---"
cat -n "$file" | sed -n '1,100p'
printf '%s\n' "--- workspace/toolchain files ---"
for f in rust-toolchain.toml Cargo.toml; do
if [ -f "$f" ]; then
echo "### $f"
sed -n '1,160p' "$f"
fi
done
printf '%s\n' "--- relevant lint configuration ---"
rg -n --hidden -g '!target' -g '!node_modules' 'len_zero|clippy|properties_viewer_corpus_paths|CorpusPathConfig' .Repository: KooshaPari/SessionLedger
Length of output: 20480
🏁 Script executed:
#!/bin/bash
set -eu
file="crates/sl-viewer/tests/properties_viewer_corpus_paths.rs"
cat -n "$file" | sed -n '1,100p'
rg -n --hidden -g '!target' -g '!node_modules' 'len_zero|clippy|CorpusPathConfig' .Repository: KooshaPari/SessionLedger
Length of output: 16693
🏁 Script executed:
#!/bin/bash
set -eu
if ! command -v clippy-driver >/dev/null 2>&1; then
echo "clippy-driver unavailable"
exit 0
fi
tmp="$(mktemp --suffix=.rs)"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'RS'
fn main() {
let values: Vec<u8> = Vec::new();
assert_eq!(values.len(), 0);
assert!(values.len() >= 1);
}
RS
clippy-driver --edition 2021 -W clippy::len_zero "$tmp" 2>&1 || trueRepository: KooshaPari/SessionLedger
Length of output: 7687
🏁 Script executed:
#!/bin/bash
set -u
tmp="/tmp/probe_len_zero.rs"
trap 'rm -f "$tmp"' EXIT
cat >"$tmp" <<'RS'
fn main() {
let values: Vec<u8> = Vec::new();
assert_eq!(values.len(), 0);
assert!(values.len() >= 1);
}
RS
cd /tmp
clippy-driver --edition 2021 -W clippy::len_zero "$tmp" 2>&1
status=$?
rm -f "$tmp"
exit "$status"Repository: KooshaPari/SessionLedger
Length of output: 417
🌐 Web query:
Rust Clippy len_zero lint assert_eq!(len(), 0) len() >= 1 official documentation
💡 Result:
The Clippy lint clippy::len_zero is designed to detect instances where code checks the length of an object (using .len()) specifically to determine if it is empty, and suggests using the more idiomatic .is_empty() method instead [1][2]. When using .len() == 0, .len() != 0, or similar comparisons (such as .len() >= 1 or .len() < 1), Clippy suggests that .is_empty() is often more efficient—as some types can calculate emptiness faster than an exact length—and generally results in cleaner, more readable code [1][2]. Regarding your specific examples: - Assertions like assert_eq!(len, 0): While len_zero primarily targets if-statements and expressions, using is_empty() is the preferred, idiomatic way to check for emptiness in Rust [1]. For assertions, the idiomatic approach is to use assert!(collection.is_empty) or assert!(!collection.is_empty) rather than comparing length to zero [3]. - Comparisons like len >= 1: The len_zero lint explicitly covers these patterns [1][2]. For example, x.len() >= 1 is flagged and suggested to be replaced with !x.is_empty() [1][2]. The lint only triggers if the type has an is_empty() method; if a type lacks this method, Clippy will not suggest the replacement [4][2]. This lint is part of the style category and is enabled by default [1][5].
Citations:
- 1: https://github.com/rust-lang/rust-clippy/blob/master/clippy_lints/src/len_zero.rs
- 2: https://github.com/rust-lang/rust-clippy/blob/fedd496a/clippy_lints/src/len_zero.rs
- 3: https://rust-lang.github.io/rust-clippy/stable/index.html
- 4: https://github.com/rust-lang/rust-clippy/blob/fedd496a/tests/ui/len_zero.rs
- 5: https://doc.rust-lang.org/stable/clippy/lints.html
Replace both length comparisons with is_empty() checks. Use cfg.custom_paths.is_empty() and !cfg.custom_paths.is_empty() to avoid clippy::len_zero.
🤖 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_viewer_corpus_paths.rs` around lines 50 -
63, In the tests empty_is_empty and config_with_paths_is_not_empty, replace both
custom_paths length comparisons with cfg.custom_paths.is_empty() and its
negation, preserving the existing assertions.
Source: Coding guidelines
| /// Property: `default_config_path()` returns `Some` on this | ||
| /// platform (every CI host has a config dir), and that path | ||
| /// includes "SessionLedger". | ||
| #[test] | ||
| fn default_config_path_resolves_on_this_platform(_unused in 0u8..1u8) { | ||
| let path = default_config_path(); | ||
| prop_assert!(path.is_some(), | ||
| "default_config_path should resolve to Some() on this platform"); | ||
| let p = path.unwrap(); | ||
| let path_str = p.to_string_lossy().to_string(); | ||
| prop_assert!(path_str.contains("SessionLedger"), | ||
| "default_config_path {:?} should include 'SessionLedger'", p); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target file outline ---'
ast-grep outline crates/sl-viewer/tests/properties_viewer_corpus_paths.rs
printf '%s\n' '--- target file relevant ranges ---'
cat -n crates/sl-viewer/tests/properties_viewer_corpus_paths.rs | sed -n '1,220p'
printf '%s\n' '--- default_config_path definitions and uses ---'
rg -n -C 5 'default_config_path|settings::dir|SessionLedger' crates/sl-viewer
printf '%s\n' '--- workspace/toolchain/test configuration ---'
fd -a -t f 'rust-toolchain.toml|Cargo.toml|Cargo.lock' .
printf '%s\n' '--- target test declarations and property macros ---'
rg -n -C 3 '#\[test\]|proptest|proptest!' crates/sl-viewer/tests/properties_viewer_corpus_paths.rsRepository: KooshaPari/SessionLedger
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- corpus_paths implementation ---'
cat -n crates/sl-viewer/src/corpus_paths.rs | sed -n '1,155p'
printf '%s\n' '--- dependency and toolchain metadata ---'
cat -n crates/sl-viewer/Cargo.toml | sed -n '1,80p'
cat -n rust-toolchain.toml
printf '%s\n' '--- exact relevant assertions and clippy-sensitive expressions ---'
rg -n -C 2 'custom_paths\.len|default_config_path_resolves|default_config_path\(' \
crates/sl-viewer/tests/properties_viewer_corpus_paths.rs \
crates/sl-viewer/src/corpus_paths.rs
printf '%s\n' '--- standalone fallback-branch verifier ---'
rustc --version
rustc - <<'RS'
use std::env;
use std::path::PathBuf;
fn model_default_config_path(config_dir: Option<PathBuf>, cwd: Result<PathBuf, ()>) -> Option<PathBuf> {
if let Some(base) = config_dir {
return Some(base.join("SessionLedger").join("corpus_paths.json"));
}
cwd.ok().map(|path| path.join("corpus_paths.json"))
}
fn main() {
let fallback = model_default_config_path(None, Ok(PathBuf::from("/tmp/worktree")));
assert_eq!(fallback, Some(PathBuf::from("/tmp/worktree/corpus_paths.json")));
assert!(!fallback.as_ref().unwrap().to_string_lossy().contains("SessionLedger"));
let unresolved = model_default_config_path(None, Err(()));
assert_eq!(unresolved, None);
let configured = model_default_config_path(Some(PathBuf::from("/home/user/.config")), Ok(PathBuf::from("/tmp/worktree")));
assert_eq!(
configured,
Some(PathBuf::from("/home/user/.config/SessionLedger/corpus_paths.json"))
);
println!("fallback Some without SessionLedger; both unresolved None; configured path contains SessionLedger");
}
RSRepository: KooshaPari/SessionLedger
Length of output: 18321
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- corpus_paths implementation ---'
cat -n crates/sl-viewer/src/corpus_paths.rs | sed -n '1,155p'
printf '%s\n' '--- dependency and toolchain metadata ---'
cat -n crates/sl-viewer/Cargo.toml | sed -n '1,80p'
cat -n rust-toolchain.toml
printf '%s\n' '--- exact relevant assertions and clippy-sensitive expressions ---'
rg -n -C 2 'custom_paths\.len|default_config_path_resolves|default_config_path\(' \
crates/sl-viewer/tests/properties_viewer_corpus_paths.rs \
crates/sl-viewer/src/corpus_paths.rs
printf '%s\n' '--- standalone fallback-branch verifier ---'
rustc --version
rustc - <<'RS'
use std::path::PathBuf;
fn model_default_config_path(config_dir: Option<PathBuf>, cwd: Result<PathBuf, ()>) -> Option<PathBuf> {
if let Some(base) = config_dir {
return Some(base.join("SessionLedger").join("corpus_paths.json"));
}
cwd.ok().map(|path| path.join("corpus_paths.json"))
}
fn main() {
let fallback = model_default_config_path(None, Ok(PathBuf::from("/tmp/worktree")));
assert_eq!(fallback, Some(PathBuf::from("/tmp/worktree/corpus_paths.json")));
assert!(!fallback.as_ref().unwrap().to_string_lossy().contains("SessionLedger"));
let unresolved = model_default_config_path(None, Err(()));
assert_eq!(unresolved, None);
let configured = model_default_config_path(
Some(PathBuf::from("/home/user/.config")),
Ok(PathBuf::from("/tmp/worktree")),
);
assert_eq!(
configured,
Some(PathBuf::from("/home/user/.config/SessionLedger/corpus_paths.json"))
);
println!("fallback Some without SessionLedger; both unresolved None; configured path contains SessionLedger");
}
RSRepository: KooshaPari/SessionLedger
Length of output: 17030
Handle both default_config_path() branches.
When dirs::config_dir() returns None, default_config_path() can return <cwd>/corpus_paths.json, which does not contain SessionLedger. Accept both None and this valid fallback. Assert SessionLedger only for the platform-directory branch.
🤖 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_viewer_corpus_paths.rs` around lines 183 -
195, Update default_config_path_resolves_on_this_platform to handle both
default_config_path() outcomes: accept the fallback path under the current
working directory when config_dir() is unavailable, and require the path to
contain “SessionLedger” only when the platform configuration directory branch is
used.
| // Construct an empty bundle and verify the function is callable. | ||
| use session_ledger::domain::bundle::{Bundle, BundleKind, ContinuationBundle}; | ||
| let cb = ContinuationBundle { | ||
| source_id: "test-source-id".into(), | ||
| bundles: vec![Bundle::new(BundleKind::Context, serde_json::json!({}))], | ||
| }; | ||
| let detail = extract_detail(&cb); | ||
| prop_assert_eq!(detail.source_id, "test-source-id"); | ||
| prop_assert_eq!(detail.intent_state, session_ledger::domain::intent::IntentState::Extracted); | ||
| prop_assert!(detail.intent_goal.is_none()); | ||
| prop_assert!(detail.context_cwd.is_none()); | ||
| prop_assert!(detail.context_title.is_none()); | ||
| prop_assert!(detail.acceptance_signals.is_empty()); | ||
| prop_assert!(detail.constraints.is_empty()); | ||
| prop_assert!(detail.contract_criteria.is_empty()); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- matching files ---'
git ls-files | rg '(^|/)(properties_viewer_detail_pane|.*detail.*|.*viewer.*)\.(rs|toml)$' | head -80
printf '%s\n' '--- extract_detail references ---'
rg -n -C 4 'extract_detail|total_token_estimate|acceptance_signals|contract_criteria|intent_goal|context_cwd|context_title' crates/sl-viewer crates 2>/dev/null | head -260
printf '%s\n' '--- test file outline ---'
if command -v ast-grep >/dev/null 2>&1; then
ast-grep outline crates/sl-viewer/tests/properties_viewer_detail_pane.rs
else
wc -l crates/sl-viewer/tests/properties_viewer_detail_pane.rs
fi
printf '%s\n' '--- test file ---'
cat -n crates/sl-viewer/tests/properties_viewer_detail_pane.rsRepository: KooshaPari/SessionLedger
Length of output: 26919
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- detail extraction implementation ---'
cat -n crates/sl-viewer/src/detail_pane.rs
printf '%s\n' '--- domain bundle definitions and token calculation ---'
rg -n -C 8 'struct Bundle|struct ContinuationBundle|impl Bundle|impl ContinuationBundle|total_token_estimate|BundleKind' src crates -g '*.rs' | head -320
printf '%s\n' '--- representative bundle fixtures ---'
cat -n crates/sl-viewer/src/mock_data.rs | sed -n '1,150p'
printf '%s\n' '--- detail-pane tests and related test assertions ---'
rg -n -C 5 'extract_detail|BundleDetail|total_token_estimate|skipped_by|watch_files|constraints|cwd|title' crates/sl-viewer/tests src -g '*.rs' | head -320
printf '%s\n' '--- manifest test configuration ---'
cat -n crates/sl-viewer/Cargo.toml | sed -n '1,180p'Repository: KooshaPari/SessionLedger
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- contract schema and producers ---'
rg -n -C 6 'struct Contract|criteria|skipped_by|watch_files|BundleKind::Contract|json!\(' src crates/sl-viewer/src -g '*.rs' \
| rg -A8 -B8 'Contract|criteria|skipped_by|watch_files|BundleKind::Contract' | head -260
printf '%s\n' '--- detail-pane consumers ---'
rg -n -C 8 'BundleDetail|contract_criteria|total_token_estimate|acceptance_signals|constraints' crates/sl-viewer/src -g '*.rs' | head -260
printf '%s\n' '--- precise test assertions ---'
python3 - <<'PY'
from pathlib import Path
p = Path("crates/sl-viewer/tests/properties_viewer_detail_pane.rs")
text = p.read_text()
fields = [
"source_id", "intent_goal", "intent_state", "acceptance_signals",
"constraints", "context_cwd", "context_title", "contract_criteria",
"total_token_estimate",
]
for field in fields:
lines = [i + 1 for i, line in enumerate(text.splitlines()) if field in line]
print(f"{field}: {lines}")
PY
printf '%s\n' '--- toolchain availability (without building or running repository code) ---'
if [ -f rust-toolchain.toml ]; then cat -n rust-toolchain.toml; else echo 'no rust-toolchain.toml'; fi
command -v rustc || true
command -v cargo || trueRepository: KooshaPari/SessionLedger
Length of output: 36116
Add a non-empty extraction case.
Construct populated Intent, Context, and Contract bundles with nonzero token estimates. Assert every extracted field, including intent_goal, acceptance_signals, constraints, context_cwd, context_title, contract_criteria, and total_token_estimate. Keep the empty case for default behavior.
🤖 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_viewer_detail_pane.rs` around lines 60 -
75, Add a second property-test case alongside the existing empty-bundle case,
constructing populated Intent, Context, and Contract bundles with nonzero token
estimates and passing them to extract_detail. Assert the expected values for
every extracted field—intent_goal, acceptance_signals, constraints, context_cwd,
context_title, contract_criteria, and total_token_estimate—while preserving the
current empty case and its default assertions.
| fn empty_name_never_matches(_unused in 0u8..1u8) { | ||
| // An empty-name call: should not panic and should return false | ||
| // (since query_fixture_name filters out empty values). | ||
| let _ = query_fixture_active(""); | ||
| // Documented behavior: fixture helper filters empty values, so | ||
| // visual_fixture_active never reports true for empty fixtures. | ||
| if !visual_fixture_active() { | ||
| prop_assert!(query_fixture_name().is_none(), | ||
| "visual_fixture_active false but query_fixture_name returned a value"); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate files ---'
git ls-files | rg '(^|/)(properties_viewer_fixture\.rs|.*fixture.*\.rs)$'
printf '%s\n' '--- relevant symbols ---'
rg -n -C 8 \
'fn empty_name_never_matches|query_fixture_active|query_fixture_name|visual_fixture_active' \
crates/sl-viewerRepository: KooshaPari/SessionLedger
Length of output: 30433
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
source = Path("crates/sl-viewer/src/fixture.rs").read_text()
test = Path("crates/sl-viewer/tests/properties_viewer_fixture.rs").read_text()
assert 'filter(|value| !value.trim().is_empty())' in source
assert 'query_fixture_name().as_deref() == Some(name)' in source
assert 'let _ = query_fixture_active("");' in test
assert 'prop_assert!(!query_fixture_active(""));' not in test
# Model the source-level invariant for representative query values.
def query_fixture_name(raw):
return None if raw is None or not raw.strip() else raw
def query_fixture_active(raw, name):
return query_fixture_name(raw) == name
for raw in (None, "", " ", "\t", "\n", "launch-splash", "other"):
assert not query_fixture_active(raw, ""), (raw, query_fixture_name(raw))
print("empty-name result is false for absent, empty, whitespace-only, and non-empty fixture values")
print("test currently discards the result and lacks the proposed assertion")
PYRepository: KooshaPari/SessionLedger
Length of output: 322
Assert the empty-name result.
query_fixture_active("") returns false, but the property discards this result. Replace the discarded call with prop_assert!(!query_fixture_active(""));.
🤖 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_viewer_fixture.rs` around lines 91 - 100,
Update the empty-name property test function empty_name_never_matches to assert
that query_fixture_active("") returns false using prop_assert!, rather than
discarding the result; preserve the existing visual_fixture_active and
query_fixture_name checks.
| fn roots_with_no_explicit_only_returns_existing( | ||
| home_str in "/[a-zA-Z0-9_./-]{3,40}", | ||
| ) { | ||
| let home = std::path::PathBuf::from(&home_str); | ||
| let roots = web_export_roots_with_env(&home, None); | ||
| for (_, path) in &roots { | ||
| prop_assert!(path.exists(), | ||
| "returned path {:?} doesn't exist", path); | ||
| } | ||
| } | ||
|
|
||
| /// Property: when no explicit list is provided, the returned paths | ||
| /// are rooted under `<home>/Downloads/<provider>` (the documented | ||
| /// fallback location). | ||
| #[test] | ||
| fn roots_with_no_explicit_under_downloads( | ||
| home_str in "/[a-zA-Z0-9_./-]{3,40}", | ||
| ) { | ||
| let home = std::path::PathBuf::from(&home_str); | ||
| let roots = web_export_roots_with_env(&home, None); | ||
| for (_, path) in &roots { | ||
| // Path should contain "Downloads" segment. | ||
| let lossy = path.to_string_lossy(); | ||
| prop_assert!(lossy.contains("Downloads"), | ||
| "root path {:?} not under Downloads", path); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- candidate test file ---'
wc -l crates/sl-viewer/tests/properties_viewer_web_exports.rs
sed -n '1,220p' crates/sl-viewer/tests/properties_viewer_web_exports.rs
printf '%s\n' '--- related symbols ---'
rg -n -C 5 'web_export_roots_with_env|Downloads|explicit' crates/sl-viewerRepository: KooshaPari/SessionLedger
Length of output: 33335
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- implementation ---'
sed -n '57,115p' crates/sl-viewer/src/web_exports.rs
printf '%s\n' '--- test dependency and temporary-directory patterns ---'
rg -n -C 3 'tempfile|tempdir|create_dir_all' crates/sl-viewer/Cargo.toml Cargo.toml crates/sl-viewer/tests crates/sl-viewer/src | head -160
printf '%s\n' '--- generated path shape check ---'
python3 - <<'PY'
import re
# Check whether the property generator can yield the current repository's likely
# existing path, and summarize the generated path grammar rather than executing
# repository code.
pattern = re.compile(r'^/[A-Za-z0-9_./-]{3,40}$')
for value in ['/tmp', '/tmp/foo', '/root', '/home', '/workspace', '/']:
print(value, bool(pattern.fullmatch(value)))
PYRepository: KooshaPari/SessionLedger
Length of output: 14053
Create fallback directories before asserting fallback roots.
The implementation filters nonexistent fallback paths, so these properties can pass with an empty result. Use a temporary home directory with Downloads/ChatGPT, Downloads/Claude, and Downloads/Gemini, then assert the exact provider-path pairs.
🤖 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_viewer_web_exports.rs` around lines 120 -
145, Update roots_with_no_explicit_only_returns_existing and
roots_with_no_explicit_under_downloads to create a temporary home directory
containing Downloads/ChatGPT, Downloads/Claude, and Downloads/Gemini before
calling web_export_roots_with_env. Assert the exact expected provider-to-path
pairs and retain the existing path validity and Downloads-root expectations.
| //! * `open` on a malformed blob returns Err rather than panicking | ||
| //! * `open` on a wrong-key blob returns a non-original plaintext | ||
| //! * `ENVELOPE_KEY_ENV` constant equals `"SL_ENVELOPE_KEY"` |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔴 Critical | 🏗️ Heavy lift
Replace unauthenticated envelope behavior.
src/envelope.rs:57-71 accepts any valid-format ciphertext and XOR-decrypts it without an authenticity check. An attacker who can modify a blob can change recovered plaintext without causing open to return an error. Line 9 codifies wrong-key decryption as successful behavior.
Use authenticated encryption and require open to reject modified ciphertext and wrong keys. Add these rejection tests before relying on this envelope for protected data.
🤖 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 `@tests/properties_envelope.rs` around lines 8 - 10, The envelope currently
permits unauthenticated decryption and treats wrong-key ciphertext as
successful. Update the envelope implementation around open to use authenticated
encryption, requiring authentication failure from modified ciphertext or an
incorrect key to return Err; revise the properties in the test module to assert
rejection rather than non-original plaintext, and add coverage for both
tampering and wrong-key cases.
| proptest! { | ||
| /// Property: `ENVELOPE_KEY_ENV` matches the documented env var name. | ||
| #[test] | ||
| fn envelope_key_env_is_documented(_unused in 0u8..1u8) { | ||
| prop_assert_eq!(ENVELOPE_KEY_ENV, "SL_ENVELOPE_KEY"); | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Make generated data drive envelope behavior.
_unused does not affect the assertion. The behavioral cases use fixed fixtures, so the suite does not property-test arbitrary plaintext or malformed blobs as documented.
Generate plaintext vectors and malformed strings with proptest, then assert round trips and rejection behavior for those generated values.
🤖 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 `@tests/properties_envelope.rs` around lines 28 - 34, Update the property tests
around envelope behavior, including envelope_key_env_is_documented and the
existing envelope round-trip/rejection tests, so generated plaintext vectors
drive successful encode/decode assertions and generated malformed strings drive
rejection assertions. Remove the unused constant-range input and preserve the
documented environment-key assertion as a separate deterministic check.
| // ── Plain `#[test]` block (env-mutating tests are serialized by cargo test, | ||
| // avoiding the proptest worker-thread race against std::env::var. | ||
| // ──────────────────────────────────────────────────────────────────────────── | ||
|
|
||
| /// Plain `#[test]` for env-mutating invariants (serialized by cargo test). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- toolchain ---'
if [ -f rust-toolchain.toml ]; then
cat rust-toolchain.toml
else
printf '%s\n' 'rust-toolchain.toml not found'
fi
printf '%s\n' '--- target file outline and lines ---'
ast-grep outline tests/properties_envelope.rs || true
sed -n '1,240p' tests/properties_envelope.rs
printf '%s\n' '--- environment key references ---'
rg -n -C 3 'SL_ENVELOPE_KEY|set_var|remove_var|env::var|Mutex|OnceLock|serial' tests src Cargo.toml rust-toolchain.toml 2>/dev/null || true
printf '%s\n' '--- test configuration ---'
rg -n -C 4 'edition|resolver|proptest|serial_test|dev-dependencies' Cargo.toml Cargo.lock 2>/dev/null || trueRepository: KooshaPari/SessionLedger
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workspace manifests ---'
find . -maxdepth 3 -name Cargo.toml -o -name Cargo.lock -o -name .cargo
for f in $(find . -maxdepth 3 -name Cargo.toml | sort); do
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
done
printf '%s\n' '--- focused environment mutations ---'
rg -n -C 5 'std::env::(set_var|remove_var)|env::(set_var|remove_var)|SL_ENVELOPE_KEY' \
tests/properties_envelope.rs src/envelope.rs tests 2>/dev/null
printf '%s\n' '--- repository test-runner configuration ---'
find . -maxdepth 3 -type f \( -name '.cargo' -o -name 'config' -o -name 'config.toml' -o -name 'Makefile' -o -name 'justfile' \) -print
rg -n -C 3 'test-threads|cargo test|nextest|serial_test|RUST_TEST_THREADS' . \
-g '!Cargo.lock' -g '!target/**' 2>/dev/null || true
printf '%s\n' '--- relevant envelope implementation ---'
sed -n '1,115p' src/envelope.rsRepository: KooshaPari/SessionLedger
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
p = Path("tests/properties_envelope.rs")
lines = p.read_text().splitlines()
for start, end in [(36, 42), (157, 207)]:
print(f"--- {p}:{start}-{end} ---")
for n in range(start, end + 1):
print(f"{n:4}: {lines[n-1]}")
PYRepository: KooshaPari/SessionLedger
Length of output: 2800
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workspace manifests ---'
find . -maxdepth 3 \( -name Cargo.toml -o -name Cargo.lock \) -print
for f in $(find . -maxdepth 3 -name Cargo.toml | sort); do
printf '\n--- %s ---\n' "$f"
sed -n '1,220p' "$f"
done
printf '%s\n' '--- focused environment mutations ---'
rg -n -C 5 'std::env::(set_var|remove_var)|env::(set_var|remove_var)|SL_ENVELOPE_KEY' \
tests/properties_envelope.rs src/envelope.rs tests 2>/dev/null
printf '%s\n' '--- repository test-runner configuration ---'
find . -maxdepth 3 -type f \( -name 'config' -o -name 'config.toml' -o -name 'Makefile' -o -name 'justfile' \) -print
rg -n -C 3 'test-threads|cargo test|nextest|serial_test|RUST_TEST_THREADS' . \
-g '!Cargo.lock' -g '!target/**' 2>/dev/null || true
printf '%s\n' '--- relevant envelope implementation ---'
sed -n '1,115p' src/envelope.rsRepository: KooshaPari/SessionLedger
Length of output: 50383
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
path = Path("tests/properties_envelope.rs")
lines = path.read_text().splitlines()
# Identify test functions and inspect only their bodies using brace balance.
tests = []
for i, line in enumerate(lines):
m = re.match(r"\s*fn\s+([A-Za-z0-9_]+)\s*\(", line)
if not m:
continue
name = m.group(1)
depth = 0
started = False
end = i
for j in range(i, len(lines)):
depth += lines[j].count("{") - lines[j].count("}")
if "{" in lines[j]:
started = True
if started and depth == 0:
end = j
break
body = "\n".join(lines[i:end + 1])
if "set_var" in body or "remove_var" in body:
tests.append((name, i + 1, end + 1, body))
print("environment-mutating tests in tests/properties_envelope.rs:")
for name, start, end, body in tests:
print(f"- {name}: lines {start}-{end}")
print(f" set_var={body.count('set_var')}, remove_var={body.count('remove_var')}")
print(f" explicit_lock={'Mutex' in body or 'lock(' in body}")
print(f" unwind_guard={'catch_unwind' in body or 'Drop' in body}")
print("\nrestoration-path checks:")
short = next(body for name, _, _, body in tests if name == "seal_returns_err_on_short_key")
helper = next(body for name, _, _, body in tests if name == "with_key_result")
print(f"- short-key test removes key when previous value is absent: "
f"{'remove_var' in short and 'if let Some' in short}")
print(f"- helper restores after normal closure return: "
f"{'let result = f();' in helper and 'result' in helper}")
print(f"- helper has an unwind guard: "
f"{'catch_unwind' in helper or 'Drop' in helper}")
# A minimal deterministic interleaving model for two tests that save, mutate,
# call seal, and restore the process-global key.
states = {"A": "A", "B": "B"}
trace = [
("A", "save", states["A"]),
("A", "set", "A"),
("B", "save", states["A"]),
("B", "set", "B"),
("A", "seal", states["B"]),
]
print("\ninterleaving model:")
for actor, action, value in trace:
print(f"- {actor} {action}: SL_ENVELOPE_KEY={value}")
print("result: A's seal observes B's key before either test restores it")
PYRepository: KooshaPari/SessionLedger
Length of output: 570
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
path = Path("tests/properties_envelope.rs")
text = path.read_text()
def function_body(name):
match = re.search(rf"\bfn\s+{re.escape(name)}\s*<*[^{{]*\{{", text)
if not match:
raise SystemExit(f"function not found: {name}")
start = match.start()
brace = text.find("{", match.start())
depth = 0
for pos in range(brace, len(text)):
if text[pos] == "{":
depth += 1
elif text[pos] == "}":
depth -= 1
if depth == 0:
return text[start:pos + 1]
raise SystemExit(f"unterminated function: {name}")
for name in ["seal_returns_err_on_missing_key",
"seal_returns_err_on_short_key",
"with_key_result"]:
body = function_body(name)
print(f"--- {name} ---")
print(f"set_var={body.count('set_var')}")
print(f"remove_var={body.count('remove_var')}")
print(f"has_unwind_guard={'catch_unwind' in body or 'Drop' in body}")
print(f"restores_absent_state={'else' in body and 'remove_var' in body}")
print("--- mutating tests ---")
for name in ["seal_returns_err_on_missing_key", "seal_returns_err_on_short_key"]:
body = function_body(name)
print(f"{name}: direct_lock={'Mutex' in body or '.lock()' in body}")
print("--- deterministic interleaving ---")
events = [
("A", "save", "original"),
("A", "set", "A"),
("B", "save", "A"),
("B", "set", "B"),
("A", "seal", "B"),
]
for actor, action, value in events:
print(f"{actor} {action}: SL_ENVELOPE_KEY={value}")
print("race_reachable=True")
PYRepository: KooshaPari/SessionLedger
Length of output: 798
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
from pathlib import Path
import re
path = Path("tests/properties_envelope.rs")
text = path.read_text()
def function_body(name):
match = re.search(rf"\bfn\s+{re.escape(name)}\s*(?:<[^{{]*>)?\s*\([^)]*\)[^{]*\{{", text)
if not match:
raise SystemExit(f"function not found: {name}")
brace = text.find("{", match.start())
depth = 0
for pos in range(brace, len(text)):
if text[pos] == "{":
depth += 1
elif text[pos] == "}":
depth -= 1
if depth == 0:
return text[match.start():pos + 1]
raise SystemExit(f"unterminated function: {name}")
for name in ["seal_returns_err_on_missing_key",
"seal_returns_err_on_short_key",
"with_key_result"]:
body = function_body(name)
print(f"--- {name} ---")
print(f"set_var={body.count('set_var')}")
print(f"remove_var={body.count('remove_var')}")
print(f"has_unwind_guard={'catch_unwind' in body or 'Drop' in body}")
print(f"restores_absent_state={'else' in body and 'remove_var' in body}")
print("--- mutating tests ---")
for name in ["seal_returns_err_on_missing_key", "seal_returns_err_on_short_key"]:
body = function_body(name)
print(f"{name}: direct_lock={'Mutex' in body or '.lock()' in body}")
print("--- deterministic interleaving ---")
events = [
("A", "save", "original"),
("A", "set", "A"),
("B", "save", "A"),
("B", "set", "B"),
("A", "seal", "B"),
]
for actor, action, value in events:
print(f"{actor} {action}: SL_ENVELOPE_KEY={value}")
print("race_reachable=True")
PYRepository: KooshaPari/SessionLedger
Length of output: 406
Serialize SL_ENVELOPE_KEY mutations and restore them with an RAII guard.
Cargo runs these #[test] functions in parallel; serial_tests is only a module name. The helper does not restore the environment during unwinding, and the short-key test leaves "deadbeef" set when no prior key exists.
🤖 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 `@tests/properties_envelope.rs` around lines 36 - 40, Update the plain
environment-mutating tests and their helper around the SL_ENVELOPE_KEY setup to
use an actual serialization mechanism such as the project’s serial-test
attribute or guard, rather than relying on the module name. Add an RAII
environment guard that captures the prior SL_ENVELOPE_KEY value, restores it on
drop including during panics, and ensure the short-key test does not leave
“deadbeef” set when no value previously existed.
| for ch in blob.chars() { | ||
| assert!(ch == ':' || ch.is_ascii_digit() || ch.is_ascii_lowercase(), | ||
| "blob char {:?} must be ':' or lowercase hex", ch); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- target test ---'
cat -n tests/properties_envelope.rs | sed -n '1,130p'
printf '%s\n' '--- envelope implementation ---'
if [ -f src/envelope.rs ]; then
cat -n src/envelope.rs | sed -n '1,130p'
fi
printf '%s\n' '--- related envelope references ---'
rg -n -S 'SL_ENVELOPE_KEY|envelope|split|hex|from_str_radix' --glob '*.rs' --glob '*.toml' --glob '*.md' . | head -300
printf '%s\n' '--- pinned toolchain and workspace metadata ---'
for f in rust-toolchain.toml Cargo.toml; do
if [ -f "$f" ]; then
printf '%s\n' "[$f]"
cat -n "$f" | sed -n '1,180p'
fi
done
printf '%s\n' '--- character predicate probe ---'
python3 - <<'PY'
for ch in ['0', '9', 'a', 'f', 'g', 'z', ':', 'A']:
print(repr(ch), ch.isascii() and ch.isdigit(), ch.isascii() and ch.islower(),
ch in ':0123456789abcdef')
PYRepository: KooshaPari/SessionLedger
Length of output: 50380
🏁 Script executed:
#!/bin/bash
set -eu
python3 - <<'PY'
def current(blob):
return all(ch == ':' or ch.isascii() and ch.isdigit()
or ch.isascii() and ch.islower()
for ch in blob)
def proposed(blob):
return all(
all(ch in '0123456789abcdef' for ch in part)
for part in blob.split(':')[1:]
)
cases = {
'v1:0123456789abcdef0123456789abcdef:deadbeef': True,
'v1:0123456789abcdef0123456789abcdef:gg': False,
'v1:0123456789abcdef0123456789abcdef:z': False,
'v1:0123456789abcdef0123456789abcdef:DE': False,
'v1:0123456789abcdef0123456789abcdef:': True,
}
for blob, expected in cases.items():
old_result = current(blob)
new_result = proposed(blob)
print(f'{blob!r}: current={old_result}, proposed={new_result}, '
f'expected_proposed={expected}')
assert new_result == expected
if 'gg' in blob or blob.endswith(':z'):
assert old_result is True
assert new_result is False
PYRepository: KooshaPari/SessionLedger
Length of output: 642
Restrict lowercase hexadecimal validation to 0–9 and a–f. is_ascii_lowercase() also accepts g–z, so the assertion accepts non-hex characters. Validate each encoded field with matches!(ch, '0'..='9' | 'a'..='f').
🤖 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 `@tests/properties_envelope.rs` around lines 69 - 72, Update the character
assertion in the blob validation loop to accept only ':' or lowercase
hexadecimal characters, using matches! with '0'..='9' and 'a'..='f' instead of
is_ascii_lowercase(). Preserve the existing error message and iteration
behavior.
…507) Shards the cp-durl command_palette + daemon_url property surfaces onto main without obsolete pre-rename daemon source churn and proptest-regressions.
88ad32a to
6f0646d
Compare
User description
Summary
Add property-test coverage for two sl-viewer surfaces not yet exercised by proptest:
command_palette(14 props): palette wire-format, action coverage, id taxonomydaemon_url(14 props): base/path join semantics, scheme strippingBoth modules are pure-Rust and have no Dioxus runtime dependency, so the tests compile without the heavy
desktopfeature tree (reqwest, image, brotli, etc.) — they run in 0.17s + 0.08s respectively on this branch's cargo cache.Properties
properties_viewer_command_palette.rs (14 props)
commands_array_is_nonempty: palette never emptycommand_ids_are_stable_id_strings: kebab-case, no--, no leading/trailing-command_labels_and_hints_are_nonempty: listbox options need textcommand_ids_are_unique: aria-activedescendant wiringevery_palette_action_is_reachable: all 7 PaletteAction variants dispatchedpalette_actions_are_unique_per_command: no duplicatespalette_command_equality_is_fieldwise: PartialEq across all fieldspalette_command_is_copy_and_clone: Copy + Clone produce equal valuespalette_action_is_copy_eq: PaletteAction Copy/Clone equalitypalette_action_distinct_variants_compare_unequal: 7 distinct variantscommands_order_matches_documented_taxonomy: public id ordercommand_labels_fit_in_palette_grid: label<=40, hint<=80 layoutcommands_action_set_is_seven_variants: exactly 7 unique actionscommands_dedup_preserves_count: dedup invariant independentproperties_viewer_daemon_url.rs (14 props)
daemon_api_url_joins_with_single_slash_separator: no double-slashdaemon_api_url_handles_leading_slash_or_not:/api/fooandapi/fooidenticaldaemon_api_url_empty_path_yields_base_with_trailing_slash: probe-friendlydaemon_api_url_preserves_base_prefix: no scheme swapdaemon_api_url_always_has_slash_between_base_and_path: separator invariantdaemon_api_url_is_idempotent: no hidden state mutationdaemon_api_url_preserves_trailing_slash_on_path:/api/foo/distinctdaemon_host_display_strips_http_scheme: never start with http(s)://daemon_host_display_is_nonempty: error-message safetydaemon_host_display_has_no_trailing_slash: hostname formdaemon_host_display_is_idempotent: deterministicdaemon_host_display_appears_in_base_url: literal transformationdaemon_base_url_has_no_trailing_slash: DEFAULT_DAEMON_BASE shapedaemon_api_url_stripped_equals_base_url: round-trip safeVerification
Both: 14 / 14 passed, 0 failed, 0 ignored. Total runtime <0.3s.
Closes WBS-6.2 evidence list item #42 (command_palette) + #43 (daemon_url).
CodeAnt-AI Description
Add property coverage for viewer contracts and limit daemon search work
What Changed
Impact
✅ Fewer large bundle payloads processed during search✅ Stable command palette and daemon URL behavior✅ Earlier detection of viewer regressions💡 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.