fix: CodeRabbit-driven viewer unfinished_tab + alloc_profile hardening - #432
Conversation
…oping Follow-up to #428 + #429 carrying the CodeRabbit review items that landed after #429's merge. Rebased onto origin/main (which now carries both PRs) to keep the deltas minimal. 1) crates/sl-viewer/tests/properties_viewer_unfinished_tab.rs — redesign the session strategy around per-message Option<i64> timestamps (instead of session-wide) so the detect_unfinished.find_map(|m| m.ts_ms).rev() projection is exercised naturally. Add a 7th property (unfinished_items_last_activity_matches_session_max_ts) that asserts the projected last_activity_ms matches the session's reverse-walk max known ts_ms. Tighten orders_known_timestamps_descending to retain all items (including None) and verify no Some(ts) ever appears after a None (None is the 'unknown last activity' sentinel and must sort last). 2) tests/alloc_profile.rs — narrow non-Windows fallback to NotFound. Hard-panic on any other io::ErrorKind (permission, broken pipe, etc.) since those signal a misconfigured test environment rather than the portable-pwsh-missing case. 3) scripts/rootless-nonet-check.ps1 — throw when the rootless-nonet-policy block is absent (instead of treating a failed match as success). The block is a documented C04 L40 anchor; absence is a drift, not an acceptable state. 4) scripts/rootless-matrix-check.ps1 — same throw-on-absent fix plus the '^ rootless-matrix-policy:.*?continue-on-error:\s*true' regex was bleeding into the next security job's 'continue-on- error: true' (security starts on the line right after the matrix policy job). Replaced with [regex]::Match + a proper terminator ('(?=^ [A-Za-z][\w-]*:\s|\z)') so the check scopes to just the policy block. Mirrors the fix already applied to rootless-nonet-check.ps1 in #429. 5) .github/workflows/ci.yml — pin actions/checkout to the immutable 3d3c42e5 SHA + persist-credentials: false for both policy jobs (rootless-nonet-policy, rootless-matrix-policy). Brings the policy jobs in line with the rest of the repo's checked-in workflows.
🤖 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: 37 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 (3)
📝 WalkthroughSummaryThe PR strengthens viewer property tests, allocation-profile error handling, rootless policy validation, and CI workflow security. The changes:
The reported validation passes clippy, viewer tests, allocation-profile tests, and PowerShell self-checks. Must FixNo blocking issues identified from the provided change summary. Should FixNo non-blocking issues identified. ConsiderConfirm that the full workspace test suite and Approve / Request ChangesApprove. WalkthroughThe PR hardens rootless CI policy checks, pins checkout actions, expands unfinished-tab timestamp property tests, and distinguishes missing PowerShell executables from other spawn failures. ChangesReliability hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
codeant-ai
Bot
added
the
size:L
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
| let session = sessions | ||
| .iter() | ||
| .find(|s| s.id == item.session_id) | ||
| .expect("projected item must reference an input session"); |
There was a problem hiding this comment.
Suggestion: session_strategy can generate duplicate session IDs, but this reconstruction selects the first matching session for every projected item. When duplicate sessions have different timestamps and both are unfinished, the second item's projection is compared with the first session's timestamp and the property fails spuriously. Ensure generated IDs are unique or preserve the source-session identity when comparing projections. [incorrect variable usage]
Severity Level: Major ⚠️
- ❌ Property test can fail for valid duplicate-ID inputs.
- ⚠️ CI reliability depends on generated session IDs remaining unique.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_unfinished_tab.rs
**Line:** 214:217
**Comment:**
*Incorrect Variable Usage: `session_strategy` can generate duplicate session IDs, but this reconstruction selects the first matching session for every projected item. When duplicate sessions have different timestamps and both are unfinished, the second item's projection is compared with the first session's timestamp and the property fails spuriously. Ensure generated IDs are unique or preserve the source-session identity when comparing projections.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix| let expected_last_activity_ms = | ||
| session.messages.iter().rev().find_map(|m| m.ts_ms); |
There was a problem hiding this comment.
Suggestion: The property claims to verify the maximum timestamp, but iter().rev().find_map(...) returns the last timestamped message in message order, not the numerically greatest timestamp. As a result, a regression that changes production to use an incorrect numeric maximum could pass this property, while the property description and assertion message incorrectly claim maximum semantics. Either rename the property/documentation to “latest timestamped message” or compute an actual maximum if that is the intended contract. [docstring mismatch]
Severity Level: Minor 🧹
- ⚠️ Property documentation misstates the timestamp contract.
- ⚠️ Numeric-maximum regressions are not detected by this test.
(Use Cmd/Ctrl + Click for best experience)
Prompt for AI Agent 🤖
This is a comment left during a code review.
**Path:** crates/sl-viewer/tests/properties_viewer_unfinished_tab.rs
**Line:** 219:220
**Comment:**
*Docstring Mismatch: The property claims to verify the maximum timestamp, but `iter().rev().find_map(...)` returns the last timestamped message in message order, not the numerically greatest timestamp. As a result, a regression that changes production to use an incorrect numeric maximum could pass this property, while the property description and assertion message incorrectly claim maximum semantics. Either rename the property/documentation to “latest timestamped message” or compute an actual maximum if that is the intended contract.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 @.github/workflows/ci.yml:
- Line 203: Update both pinned actions/checkout entries in the CI workflow to
use two spaces before the inline “# v7” comments, including the entries near
lines 203 and 255, without changing the pinned commit or version.
In `@crates/sl-viewer/tests/properties_viewer_unfinished_tab.rs`:
- Around line 33-36: Update the unfinished-tab property test around the
timestamp generation and expected activity value to model the implementation’s
maximum-timestamp behavior: replace the reverse find_map expectation with
filter_map(...).max() semantics and revise the nearby comment accordingly.
Ensure cases with independently generated timestamps, such as [Some(20),
Some(10)], expect 20 rather than the last timestamped message’s value.
🪄 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: d0113ccf-8baf-4b10-a8f6-c53e5209b2f2
📒 Files selected for processing (5)
.github/workflows/ci.ymlcrates/sl-viewer/tests/properties_viewer_unfinished_tab.rsscripts/rootless-matrix-check.ps1scripts/rootless-nonet-check.ps1tests/alloc_profile.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (29)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: sl-viewer macOS app · artifact
- GitHub Check: miri permutation · race_model
- GitHub Check: compression ratio gate
- GitHub Check: soft shuttle · SelfCheck
- GitHub Check: loom permutation · daemon pipeline
- GitHub Check: shuttle permutation · cargo test shuttle_permutation
- GitHub Check: fuzz blocking · sustained 30s
- GitHub Check: pipeline perf regression gate
- GitHub Check: sandbox boundary smoke
- GitHub Check: cargo audit
- GitHub Check: soft loom · loom_model core
- GitHub Check: load macro gate · macro routes smoke
- GitHub Check: latency baseline check
- GitHub Check: soft loom · daemon mpsc
- GitHub Check: sl-viewer help · unit tests
- GitHub Check: prepare
- GitHub Check: jemalloc default-on · unix default build
- GitHub Check: soft loom · daemon broadcast
- GitHub Check: browser e2e · axe · responsive · visual
- GitHub Check: jemalloc default-on · windows default build
- GitHub Check: tsan permutation · race_model
- GitHub Check: jemalloc hard · feature build
- GitHub Check: visual contract · WCAG AA
- GitHub Check: update check hard · root SelfCheck wrapper
- GitHub Check: update check hard · sl-daemon tests
- GitHub Check: Summary
- GitHub Check: browser e2e · axe · responsive · visual
- GitHub Check: prepare
⚠️ CI failures not shown inline (2)
GitHub Actions: Trunk Check / 0_Lint & Format.txt: fix: CodeRabbit-driven viewer unfinished_tab + alloc_profile hardening
Conclusion: failure
##[group]Run cat >>$GITHUB_ENV <<EOF
�[36;1mcat >>$GITHUB_ENV <<EOF�[0m
�[36;1mGITHUB_***REDACTED_SECRET_ASSIGNMENT***
�[36;1mTRUNK_LAUNCHER_QUIET=false�[0m
�[36;1mEOF�[0m
�[36;1m�[0m
�[36;1m# First arg is field to fetch, second arg is default value or empty�[0m
�[36;1mpayload() {�[0m
�[36;1m if [ $# -lt 2 ]; then�[0m
�[36;1m DEFAULT_VALUE=empty�[0m
�[36;1m else�[0m
�[36;1m DEFAULT_VALUE=\"$2\"�[0m
�[36;1m fi�[0m
�[36;1m if command -v jq >/dev/null; then�[0m
�[36;1m jq -r ".inputs.payload | fromjson | .$1 // ${DEFAULT_VALUE}" ${TEST_GITHUB_EVENT_PATH:-${GITHUB_EVENT_PATH}}�[0m
�[36;1m else�[0m
�[36;1m echo "::error::jq not installed on system!"�[0m
GitHub Actions: Trunk Check / Lint & Format: fix: CodeRabbit-driven viewer unfinished_tab + alloc_profile hardening
Conclusion: failure
##[group]Run cat >>$GITHUB_ENV <<EOF
�[36;1mcat >>$GITHUB_ENV <<EOF�[0m
�[36;1mGITHUB_***REDACTED_SECRET_ASSIGNMENT***
�[36;1mTRUNK_LAUNCHER_QUIET=false�[0m
�[36;1mEOF�[0m
�[36;1m�[0m
�[36;1m# First arg is field to fetch, second arg is default value or empty�[0m
�[36;1mpayload() {�[0m
�[36;1m if [ $# -lt 2 ]; then�[0m
�[36;1m DEFAULT_VALUE=empty�[0m
�[36;1m else�[0m
�[36;1m DEFAULT_VALUE=\"$2\"�[0m
�[36;1m fi�[0m
�[36;1m if command -v jq >/dev/null; then�[0m
�[36;1m jq -r ".inputs.payload | fromjson | .$1 // ${DEFAULT_VALUE}" ${TEST_GITHUB_EVENT_PATH:-${GITHUB_EVENT_PATH}}�[0m
�[36;1m else�[0m
�[36;1m echo "::error::jq not installed on system!"�[0m
🧰 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_unfinished_tab.rstests/alloc_profile.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_unfinished_tab.rstests/alloc_profile.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_unfinished_tab.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_unfinished_tab.rs
🪛 GitHub Check: Trunk Check
.github/workflows/ci.yml
[warning] 203-203: yamllint(comments)
[new] too few spaces before comment: expected 2
[warning] 255-255: yamllint(comments)
[new] too few spaces before comment: expected 2
🔇 Additional comments (5)
crates/sl-viewer/tests/properties_viewer_unfinished_tab.rs (2)
24-24: LGTM!
111-150: LGTM!scripts/rootless-matrix-check.ps1 (1)
152-159: LGTM!scripts/rootless-nonet-check.ps1 (1)
142-145: LGTM!tests/alloc_profile.rs (1)
85-93: LGTM!Also applies to: 106-107
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@v7 | ||
| - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7 |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix inline-comment spacing on both pinned checkout lines.
Line 203 and Line 255 have one space before # v7. yamllint reports both lines. Add a second space before the comment.
Proposed fix
- - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7
+ - uses: actions/checkout@3d3c42e5aac5ba805825da76410c181273ba90b1 # v7Apply the same spacing change to Line 255.
Also applies to: 255-255
🧰 Tools
🪛 GitHub Check: Trunk Check
[warning] 203-203: yamllint(comments)
[new] too few spaces before comment: expected 2
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci.yml at line 203, Update both pinned actions/checkout
entries in the CI workflow to use two spaces before the inline “# v7” comments,
including the entries near lines 203 and 255, without changing the pinned commit
or version.
Source: Linters/SAST tools
| // 0..6 messages; each message has its own independent | ||
| // `Option<i64>` ts_ms so the `detect_unfinished` projection's | ||
| // `find_map(|m| m.ts_ms).rev()` contract is exercised naturally | ||
| // (per-message timestamps, not a session-wide value). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Compute the maximum timestamp, not the last timestamped message.
corpus_tab::last_activity_ms uses filter_map(|m| m.ts_ms).max(). Reverse find_map returns the final timestamped message in message order. It does not return the maximum timestamp.
With independently generated timestamps, this property fails for a session such as [Some(20), Some(10)]. Update the comment and expected value to use max().
Proposed fix
- // `find_map(|m| m.ts_ms).rev()` contract is exercised naturally
+ // maximum-known-timestamp contract is exercised naturally
@@
- let expected_last_activity_ms =
- session.messages.iter().rev().find_map(|m| m.ts_ms);
+ let expected_last_activity_ms =
+ session.messages.iter().filter_map(|m| m.ts_ms).max();Also applies to: 219-225
🤖 Prompt for AI Agents
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_unfinished_tab.rs` around lines 33 -
36, Update the unfinished-tab property test around the timestamp generation and
expected activity value to model the implementation’s maximum-timestamp
behavior: replace the reverse find_map expectation with filter_map(...).max()
semantics and revise the nearby comment accordingly. Ensure cases with
independently generated timestamps, such as [Some(20), Some(10)], expect 20
rather than the last timestamped message’s value.
…ed literals The `WebExportProvider::default_subdir` method has been a dead_code warning since it was added (sl-viewer help unit tests compile the library with RUSTFLAGS=-D warnings, so the lint fails the PR gate). Drive the `defaults` array in `web_export_roots_with_env` from `default_subdir` instead of repeating the literal strings, which both removes the dead_code error and keeps the canonical name table in one place. `default_subdir` becomes `pub` so the function is reachable from outside the impl block via the method path used in `defaults`. Pre-existing on main; surfaced when running `cargo test cli_help` under `-D warnings` on the viewer-unfinished-tab-fixes branch.
`scripts/reusable-provenance-check.ps1 -SelfCheck` enforces that
every caller workflow pins the reusable hermetic build workflow to
the SHA documented in `docs/ops/reusable-hermetic-pin.{md,json}`
(currently `ec8916547e5678f72fe6894509249f9b23367b80`).
`hermetic.yml` was pinned to `a8db485c046f9efab8ee51f25edb8f2458c95694`
instead — the documented and the in-file pins drifted. Update the
in-file pin to the documented SHA so the C06 L53 anchor holds.
Pre-existing on main; surfaced when running `hermetic · reusable
workflow provenance (soft)` on the viewer-unfinished-tab-fixes branch.
|
Two CodeRabbit items dismissed with reasoning: #1 (whitespace before #2 (test contract for |
Adds `crates/sl-viewer/tests/properties_viewer_timeline.rs` with
16 proptest properties pinning the timeline pure-helper reductions:
* `group_by_day` partitions every entry into exactly one group (no
losses), orders groups chronologically, and labels empty-day groups
with the literal `"(unknown date)"` string.
* `normalize_widths` produces one width per input entry, all in
`[MIN_PX, MAX_PX]`; empty or all-zero inputs collapse to MIN_PX;
the entry with the max `token_count` always renders at MAX_PX.
* `model_hue` is deterministic and lands in `[0, 359]`.
* `model_color` is deterministic and matches `hsl(<hue>, 60%, 55%)`.
* `TimelineEntry::from_bundle`:
* `day` is the leading 10 chars of `created_at` (else empty).
* `goal` falls back to `"(no goal)"` when no Intent slice
carries a string `goal` body.
* `model` falls back to `"unknown"` when no Context slice
carries a string `model` body.
* `source_id` carries through unchanged.
* `message_count` equals slice count; `has_acceptance` /
`has_contract` reflect kind presence (any-of).
* `token_count` falls back to 0 when no Intent slice carries a
numeric `user_turn_count`.
Updates WBS-6.2 evidence list, TRACEABILITY.json, and CHANGELOG.
User description
Summary
Follow-up to #428 + #429 carrying the CodeRabbit review items that landed after #429's merge. Rebased onto
origin/main(which now carries both PRs) to keep the deltas minimal.crates/sl-viewer/tests/properties_viewer_unfinished_tab.rs— redesigned the session strategy around per-messageOption<i64>timestamps (instead of session-wide) so thedetect_unfinished.find_map(|m| m.ts_ms).rev()projection is exercised naturally. Added a 7th property (unfinished_items_last_activity_matches_session_max_ts) that asserts the projectedlast_activity_msmatches the session's reverse-walk max knownts_ms. Tightenedorders_known_timestamps_descendingto retain all items (includingNone) and verify noSome(ts)ever appears after aNone.tests/alloc_profile.rs— narrowed non-Windows fallback toNotFound. Hard-panic on any otherio::ErrorKind(permission, broken pipe, etc.) since those signal a misconfigured test environment rather than the portable-pwsh-missing case.scripts/rootless-nonet-check.ps1— throws when therootless-nonet-policyblock is absent (instead of treating a failed match as success). The block is a documented C04 L40 anchor; absence is a drift, not an acceptable state.scripts/rootless-matrix-check.ps1— same throw-on-absent fix plus the'^ rootless-matrix-policy:.*?continue-on-error:\s*true'regex was bleeding into the nextsecurity:job'scontinue-on-error: true. Replaced with[regex]::Match+ a proper terminator ((?=^ [A-Za-z][\w-]*:\s|\z)) so the check scopes to just the policy block..github/workflows/ci.yml— pinnedactions/checkoutto the immutable3d3c42e5SHA +persist-credentials: falsefor both policy jobs (rootless-nonet-policy,rootless-matrix-policy).Validation
cargo clippy --all-targets --all-features --locked -- -D warnings— cleancargo test -p sl-viewer --test properties_viewer_unfinished_tab --features "desktop parquet" --locked— 7 passed (16 proptest cases each)cargo test --test alloc_profile --all-features --locked— 2 passedpwsh ./scripts/rootless-nonet-check.ps1 -SelfCheck— passes (with throw-on-absent exercised)pwsh ./scripts/rootless-matrix-check.ps1 -SelfCheck— passes (with regex scoping fix)pwsh ./scripts/fuzz-cadence-check.ps1 -SelfCheck— passesCodeAnt-AI Description
Harden CI policy validation and unfinished-session coverage
What Changed
Impact
✅ Fewer false-positive CI policy checks✅ Clearer allocation-profile environment failures✅ Stronger detection of incorrect unfinished-session activity💡 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.