test(qa): add property and fuzz smoke evidence - #123
Conversation
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds property-based tests for deduplication, merging, and OKF serialization, introduces an OKF fuzz target with CI smoke execution, and updates contributor, tracking, and line-ending configuration. ChangesValidation gates
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant CI as fuzz-smoke CI job
participant Cargo as cargo +nightly fuzz
participant Target as okf_roundtrip
participant OKF as OkfDocument and serde_json
CI->>Cargo: run okf_roundtrip with address sanitizer
Cargo->>Target: provide fuzz input
Target->>OKF: deserialize, serialize, and deserialize JSON
OKF-->>Target: reparsed document
Target-->>Cargo: assert document equality
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces property-based testing and fuzzing infrastructure to the session-ledger project. Specifically, it adds a suite of property tests in tests/properties.rs using the proptest crate to verify deduplication key stability, merge permutation invariance, merge idempotency, and OKF JSON roundtrip invariants. Additionally, it establishes a fuzzing harness under fuzz/ with an okf_roundtrip target, updates project documentation (including CONTRIBUTING.md, GAP_QA_MATRIX.md, and WBS.md) to reflect these new testing capabilities, and configures the corresponding dependency updates in Cargo.toml and Cargo.lock. There are no review comments, and I have no feedback to provide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![kilo-code-bot[bot]](https://avatars.githubusercontent.com/in/2193792?s=60&v=4){kind=small}
| session | ||
| } | ||
|
|
||
| fn role(index: u8) -> Role { |
There was a problem hiding this comment.
WARNING: role helper silently maps any future Role variant to System
The _ => Role::System catch-all absorbs values beyond the current 5 variants. If Role grows, property tests will silently stop exercising the new variant and may mask bugs.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| runs-on: ubuntu-latest | ||
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 | ||
| - uses: dtolnay/rust-toolchain@4be7066ada62dd38de10e7b70166bc74ed198c30 # stable |
There was a problem hiding this comment.
WARNING: fuzz-smoke job missing Swatinem/rust-cache
All other CI jobs use Swatinem/rust-cache, but fuzz-smoke rebuilds every dependency from scratch. For a nightly toolchain + libfuzzer-sys build, this wastes CI minutes and increases the chance of rate-limit timeouts.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
| @@ -0,0 +1,2 @@ | |||
| /.gitattributes text eol=lf | |||
| /fuzz/** text eol=lf | |||
There was a problem hiding this comment.
SUGGESTION: Consider extending .gitattributes LF normalization to tests/**
The current rule /fuzz/** text eol=lf only covers the fuzz scaffold. New test files like tests/properties.rs could still carry CRLF line endings on Windows.
Reply with @kilocode-bot fix it to have Kilo Code address this issue.
Code Review SummaryStatus: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Fix these issues in Kilo Cloud Previous Review Summary (commit 572eaf4)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 572eaf4)Status: 3 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (4 files)
Reviewed by hy3-20260706:free · Input: 95K · Output: 34.4K · Cached: 294.8K |
572eaf4 to
48a1faa
Compare
Signed-off-by: KooshaPari <kooshapari@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
Signed-off-by: KooshaPari <kooshapari@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
48a1faa to
9173bf9
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There 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 @.gitattributes:
- Around line 1-2: Extend the existing LF normalization rules in .gitattributes
to include the tests/** path, ensuring files such as tests/properties.rs
consistently use LF line endings.
In @.github/workflows/ci.yml:
- Around line 61-80: Add the pinned Swatinem/rust-cache action to the fuzz-smoke
job after the Rust toolchain setup and before cargo-fuzz installation,
configuring it to reuse the nightly toolchain and dependency build cache while
preserving the existing RUSTFLAGS and fuzz command.
- Around line 61-80: Add a finite timeout-minutes value to the fuzz-smoke job
definition, covering toolchain setup, cargo-fuzz installation, and the fuzz
execution rather than relying only on the fuzzer’s -max_total_time argument.
Keep the existing fuzz-smoke steps and runtime limit unchanged.
- Line 70: Update the workflow permissions and checkout configuration: add an
explicit least-privilege top-level permissions policy for GITHUB_TOKEN, and
update the actions/checkout step to disable credential persistence with
persist-credentials set to false. Apply these changes to the workflow block
covering the checkout step.
🪄 Autofix (Beta)
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
Run ID: d357fc9e-be17-453d-abd0-2af117880500
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockfuzz/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (12)
.gitattributes.github/workflows/ci.ymlCONTRIBUTING.mdCargo.tomldocs/ops/GAP_QA_MATRIX.mddocs/ops/TRACEABILITY.jsondocs/ops/WBS.mdfuzz/.gitignorefuzz/Cargo.tomlfuzz/corpus/okf_roundtrip/minimal.jsonfuzz/fuzz_targets/okf_roundtrip.rstests/properties.rs
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: semgrep-cloud-platform/scan
- GitHub Check: visual contract · WCAG AA
- GitHub Check: reproducible build · sl-daemon
- GitHub Check: cargo deny check
- GitHub Check: cargo audit
🧰 Additional context used
📓 Path-based instructions (2)
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Fix Clippy warnings rather than suppressing them with#[allow]; any necessary suppression must include a tracking-issue comment.
Run and maintain compatibility with the repository's Cargo build, test, and Clippy checks:cargo build --all-targets --locked,cargo test --all-features --locked, andcargo clippy --all-targets --all-features.
Format Rust code according tocargo fmt --all --check.
Files:
fuzz/fuzz_targets/okf_roundtrip.rstests/properties.rs
**/Cargo.toml
📄 CodeRabbit inference engine (AGENTS.md)
Respect the pinned MSRV and workspace Rust version: Rust 1.85 as specified by
rust-toolchain.tomlandrust-version = "1.85".
Files:
fuzz/Cargo.tomlCargo.toml
🪛 LanguageTool
docs/ops/WBS.md
[uncategorized] ~32-~32: The official name of this software platform is spelled with a capital “H”.
Context: ...; fuzz/fuzz_targets/okf_roundtrip.rs; .github/workflows/ci.yml; CONTRIBUTING.md; `...
(GITHUB)
🪛 zizmor (1.26.1)
.github/workflows/ci.yml
[warning] 70-70: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 61-80: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[info] 71-71: action functionality is already included by the runner (superfluous-actions): use rustup and/or cargo in a script step
(superfluous-actions)
🔇 Additional comments (13)
tests/properties.rs (2)
25-33: Role catch-all silently absorbs future variants.Already flagged: the
_ => Role::Systemarm maps any variant beyond the current 5 toSystem, so a futureRoleaddition would silently stop being exercised by these property tests rather than causing a compile error.
35-170: LGTM!Cargo.toml (1)
39-39: 🎯 Functional CorrectnessVerify
proptest = "1.11.0"exists on crates.io.the latest version is 1.9.0 per current package listings, and docs.rs likewise shows proptest 1.9.0 - Docs.rs as the latest published release. A pin of
1.11.0may not resolve, which would breakcargo build --all-targets --lockedmandated by the repo's Rust guidelines.
[dependency_check]
- Flagging: proptest crate version
- Action: confirm via
cargo search proptestor crates.io before merge.As per coding guidelines, "Run and maintain compatibility with the repository's Cargo build, test, and Clippy checks:
cargo build --all-targets --locked".fuzz/Cargo.toml (1)
1-22: 🩺 Stability & AvailabilityConfirm the nightly fuzz job is properly isolated from the pinned 1.85 toolchain.
This project requires the nightly compiler since it uses the -Z compiler flag to provide address sanitization. and This also needs a nightly compiler since it uses some unstable command-line flags. so cargo-fuzz's nightly requirement here is expected. However, the commit summary states the fuzz-smoke job runs nightly "despite
rust-toolchain.toml" — please confirm the nightly override is scoped only to the fuzz-smoke CI step (e.g., viaactions-rs/toolchainorrustup run nightly) and does not affectcargo build --all-targets --locked/ Clippy invocations that must stay on the pinned 1.85 toolchain per the repo's Rust guidelines..github/workflows/ci.ymlisn't in this review batch, so this can't be confirmed from the files provided.As per coding guidelines, "Respect the pinned MSRV and workspace Rust version: Rust 1.85 as specified by
rust-toolchain.tomlandrust-version = "1.85"."docs/ops/WBS.md (1)
32-32: 🗄️ Data Integrity & Integration | ⚡ Quick winEvidence list diverges from
TRACEABILITY.json.This row keeps
benches/pipeline.rsin the WBS-6.2 evidence paths, but the correspondingWBS-6.2entry indocs/ops/TRACEABILITY.json(lines 136-145) omits it. The file's own update contract requires both to be updated consistently "in the same change."docs/ops/TRACEABILITY.json (2)
136-145: 🗄️ Data Integrity & Integration | ⚡ Quick winAdd
benches/pipeline.rsto the WBS-6.2 evidence array.This mirrors the gap already flagged in
docs/ops/WBS.md(Line 32): the markdown table keepsbenches/pipeline.rsas evidence, but this JSON array drops it, breaking the machine/human mirror the repo's traceability contract requires.📝 Proposed fix
"evidence": [ "tests/properties.rs", "fuzz/fuzz_targets/okf_roundtrip.rs", ".github/workflows/ci.yml", - "CONTRIBUTING.md" + "CONTRIBUTING.md", + "benches/pipeline.rs" ]
175-196: LGTM!docs/ops/GAP_QA_MATRIX.md (1)
21-21: LGTM!Also applies to: 55-55
fuzz/corpus/okf_roundtrip/minimal.json (1)
1-1: LGTM!fuzz/fuzz_targets/okf_roundtrip.rs (1)
1-15: LGTM!fuzz/.gitignore (1)
1-3: LGTM!.github/workflows/ci.yml (1)
52-60: LGTM!CONTRIBUTING.md (1)
18-32: LGTM!
| /.gitattributes text eol=lf | ||
| /fuzz/** text eol=lf |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Extend LF normalization to tests/**.
tests/properties.rs (new in this cohort) isn't covered by any eol=lf rule, so it can still pick up CRLF line endings on Windows.
♻️ Proposed fix
/.gitattributes text eol=lf
/fuzz/** text eol=lf
+/tests/** text eol=lf🤖 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 @.gitattributes around lines 1 - 2, Extend the existing LF normalization
rules in .gitattributes to include the tests/** path, ensuring files such as
tests/properties.rs consistently use LF line endings.
| fuzz-smoke: | ||
| name: fuzz smoke (10 seconds) | ||
| runs-on: ubuntu-latest | ||
| # cargo-fuzz requires nightly; do not inherit workflow RUSTFLAGS (-D warnings) | ||
| # which breaks sanitizer flag composition under stable toolchains. | ||
| env: | ||
| RUSTFLAGS: "" | ||
| RUSTUP_TOOLCHAIN: nightly | ||
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 | ||
| - uses: dtolnay/rust-toolchain@4be7066ada62dd38de10e7b70166bc74ed198c30 # nightly for fuzz | ||
| with: | ||
| toolchain: nightly | ||
| components: rust-src | ||
| - name: install cargo-fuzz | ||
| uses: taiki-e/install-action@e28ac56891501ddb0600608470dbe94544964ed4 # cargo-fuzz | ||
| with: | ||
| tool: cargo-fuzz | ||
| - name: fuzz OKF parse and roundtrip | ||
| run: cargo +nightly fuzz run okf_roundtrip --sanitizer address -- -max_total_time=10 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
fuzz-smoke still lacks Swatinem/rust-cache.
Every dependency (including the nightly toolchain build) is rebuilt from scratch each run, wasting CI minutes for a job that should stay small.
♻️ Proposed fix
- uses: dtolnay/rust-toolchain@4be7066ada62dd38de10e7b70166bc74ed198c30 # nightly for fuzz
with:
toolchain: nightly
components: rust-src
+ - uses: Swatinem/rust-cache@e18b497796c12c097a38f9edb9d0641fb99eee32 # v2
- name: install cargo-fuzz🧰 Tools
🪛 zizmor (1.26.1)
[warning] 70-70: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 61-80: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[info] 71-71: action functionality is already included by the runner (superfluous-actions): use rustup and/or cargo in a script step
(superfluous-actions)
🤖 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 around lines 61 - 80, Add the pinned
Swatinem/rust-cache action to the fuzz-smoke job after the Rust toolchain setup
and before cargo-fuzz installation, configuring it to reuse the nightly
toolchain and dependency build cache while preserving the existing RUSTFLAGS and
fuzz command.
🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win
No timeout-minutes bound on fuzz-smoke.
The -max_total_time=10 flag only bounds the fuzzer's own run, not the toolchain install/build steps preceding it. A stalled install (e.g. network hiccup on nightly/cargo-fuzz download) leaves the job unbounded.
♻️ Proposed fix
fuzz-smoke:
name: fuzz smoke (10 seconds)
runs-on: ubuntu-latest
+ timeout-minutes: 10📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fuzz-smoke: | |
| name: fuzz smoke (10 seconds) | |
| runs-on: ubuntu-latest | |
| # cargo-fuzz requires nightly; do not inherit workflow RUSTFLAGS (-D warnings) | |
| # which breaks sanitizer flag composition under stable toolchains. | |
| env: | |
| RUSTFLAGS: "" | |
| RUSTUP_TOOLCHAIN: nightly | |
| steps: | |
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 | |
| - uses: dtolnay/rust-toolchain@4be7066ada62dd38de10e7b70166bc74ed198c30 # nightly for fuzz | |
| with: | |
| toolchain: nightly | |
| components: rust-src | |
| - name: install cargo-fuzz | |
| uses: taiki-e/install-action@e28ac56891501ddb0600608470dbe94544964ed4 # cargo-fuzz | |
| with: | |
| tool: cargo-fuzz | |
| - name: fuzz OKF parse and roundtrip | |
| run: cargo +nightly fuzz run okf_roundtrip --sanitizer address -- -max_total_time=10 | |
| fuzz-smoke: | |
| name: fuzz smoke (10 seconds) | |
| runs-on: ubuntu-latest | |
| timeout-minutes: 10 | |
| # cargo-fuzz requires nightly; do not inherit workflow RUSTFLAGS (-D warnings) | |
| # which breaks sanitizer flag composition under stable toolchains. | |
| env: | |
| RUSTFLAGS: "" | |
| RUSTUP_TOOLCHAIN: nightly | |
| steps: | |
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 | |
| - uses: dtolnay/rust-toolchain@4be7066ada62dd38de10e7b70166bc74ed198c30 # nightly for fuzz | |
| with: | |
| toolchain: nightly | |
| components: rust-src | |
| - name: install cargo-fuzz | |
| uses: taiki-e/install-action@e28ac56891501ddb0600608470dbe94544964ed4 # cargo-fuzz | |
| with: | |
| tool: cargo-fuzz | |
| - name: fuzz OKF parse and roundtrip | |
| run: cargo +nightly fuzz run okf_roundtrip --sanitizer address -- -max_total_time=10 |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 70-70: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
[warning] 61-80: overly broad permissions (excessive-permissions): default permissions used due to no permissions: block
(excessive-permissions)
[info] 71-71: action functionality is already included by the runner (superfluous-actions): use rustup and/or cargo in a script step
(superfluous-actions)
🤖 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 around lines 61 - 80, Add a finite timeout-minutes
value to the fuzz-smoke job definition, covering toolchain setup, cargo-fuzz
installation, and the fuzz execution rather than relying only on the fuzzer’s
-max_total_time argument. Keep the existing fuzz-smoke steps and runtime limit
unchanged.
| RUSTFLAGS: "" | ||
| RUSTUP_TOOLCHAIN: nightly | ||
| steps: | ||
| - uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win
Harden workflow permissions and checkout credential persistence.
Static analysis flags default (broad) GITHUB_TOKEN permissions and a checkout step that doesn't disable credential persistence.
🔒 Proposed fix
fuzz-smoke:
name: fuzz smoke (10 seconds)
runs-on: ubuntu-latest
+ permissions:
+ contents: read
env:
RUSTFLAGS: ""
RUSTUP_TOOLCHAIN: nightly
steps:
- uses: actions/checkout@9c091bb21b7c1c1d1991bb908d89e4e9dddfe3e0 # v7
+ with:
+ persist-credentials: falseAlso applies to: 61-80
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 70-70: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 70, Update the workflow permissions and
checkout configuration: add an explicit least-privilege top-level permissions
policy for GITHUB_TOKEN, and update the actions/checkout step to disable
credential persistence with persist-credentials set to false. Apply these
changes to the workflow block covering the checkout step.
Source: Linters/SAST tools
1 participant
Summary
Test plan
cargo test --all-features --lockedcargo clippy --all-targets --all-features -- -D warningscargo test --test properties --lockedthree times withPROPTEST_CASES=64cargo check --manifest-path fuzz/Cargo.tomlpwsh -NoProfile -File ./docs/ops/traceability_lint.ps1fuzz-smokeCI job completes its bounded 10-second run