Repository navigation
fix(assail): make assail reports deterministic - #245
Merged
hyperpolymath merged 4 commits intoOct 26, 2026
Merged
Conversation
Two runs of `assail` on the same input produced different reports:
frameworks, recommended_attacks, the taint-matrix rows and the
dependency-graph edges (and the test-only-function scan) were built
from HashSet/HashMap iteration order, which is randomised per run. JCS
sorts object keys but by design not arrays, so the canonical JSON, and
any hash over it such as the --attest seal, differed for identical
input. Measured on main: six runs of examples/vulnerable_program.rs
gave six distinct reports.
Use BTreeSet/BTreeMap at those five sites. AttackAxis, Framework and
WeakPointCategory derive PartialOrd and Ord (declaration order); the
derives are additive.
Six runs on the example file, three on examples/ and three on the
whole src/ tree are now byte-identical. A new integration test builds
a multi-directory fixture that detects several frameworks and
recommended attacks, and asserts nine analyses give identical canonical
JSON. With the old HashMap code restored the test fails ("assail report
differs between runs").
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k
Contributor
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 7 minutes. View limit details
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Add ordering traits src/types.rs |
Framework, AttackAxis and WeakPointCategory now derive PartialOrd and Ord. |
Order report data src/assail/analyzer.rs, tests/analyzer_tests.rs, CHANGELOG.adoc |
The analyzer uses ordered collections for framework detection, attack recommendations, dependency-directory groups and taint-matrix rows. A regression test compares canonical JSON across repeated analyses. The changelog records the determinism changes. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Suggested reviewers: metadatastician
Merge Risk: 
b8f9e
-
No merge-blocking issue was identified; the determinism change is ready for normal merge checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (1 skipped: 1… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly and concisely describes the main change: making assail reports deterministic. Description check The description directly explains the determinism problem, the implemented fix, the regression test, and the verification results. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request.
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 8 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches
-
📝 Generate docstrings
-
- 🤖 Coding Agent task started for docstring generation.
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
|
Add Carrot credits or activate Agent usage billing to use Autofix |
Contributor
|
🤖 Completed: Generate docstrings for PR #245 — View commit |
hyperpolymath
enabled auto-merge (squash)
October 26, 2026 19:32
hyperpolymath
added a commit
that referenced
this pull request
Oct 26, 2026
Stacked PR E of the estate-format series: - A: `.kyaml` loaders - B: example profiles - C: JCS reports - C2: deterministic reports (#245) - D: JCS attestation (#247) This PR is independent of #245 and #247 and branches from `main`. ## What changes - **New module `src/kyaml.rs`.** A KYAML emitter that follows mikefarah `yq` v4's KYAML encoder (`pkg/yqlib/encoder_kyaml.go`, v4.54.1): flow style, two-space indent, a trailing comma on every entry, bare keys only for `^[A-Za-z_][A-Za-z0-9_-]*$`, and every string double-quoted. - **Same data as the JCS report.** Keys follow JCS order (UTF-16 code units) and numbers are in JCS form, so the KYAML and JSON reports of a scan differ only in syntax. - **New format value.** `ReportOutputFormat::Kyaml` adds `--output-format kyaml`, extension `.kyaml`, parsed from `"kyaml"`. Loaders have read `.kyaml` since PR A. - **README fix.** `README.adoc` listed `--output-format` values `markdown` and `text`, which never existed. It now lists the real ones. ## Contract, checked against the real tools | Check | Result | |---|---| | Fixed point of `yq -p yaml -o kyaml`, the test `scripts/kyaml-format.sh --check` runs | ✅ on all reports below | | standards `scripts/kyaml-format.sh --check`, run for real | ✅ `all 4 file(s) are KYAML` | | Byte-identical to `yq -p json -o kyaml` of the JCS report | ✅ two historical `reports/*.yaml` assault reports, plus assail of `src/` (52 KB) and `tests/` | | Read back through `crate::yaml` (serde-saphyr) | ✅ same data as the JCS report | The test `real_report_is_byte_identical_to_yq` pins this in CI without yq. `tests/fixtures/kyaml/assail-report.kyaml` is yq's output for the checked-in JCS report next to it. ## Two documented differences from yq 1. **Non-printables.** YAML's non-printables (U+007F–U+009F, U+FFFE, U+FFFF) are escaped as `\uXXXX`. yq writes them raw, which YAML 1.2 does not allow. A raw **U+0085 (NEL)** reads back as a line break, so yq turns `"x\u0085y"` into `"x y"`. When a file contains one, `kyaml-format.sh` reports `REFUSED … the KYAML rendering does not parse to the same data` (exit 2). The gate therefore already treats yq's output there as untrustworthy. I chose data integrity over gate cleanliness for this rare case. - **Alternative:** write them raw like yq. The gate stays quiet, but NEL is silently corrupted. - **Root-cause fix (beyond this repo):** escape these in yq's `escapeDoubleQuotedString` upstream. 2. **Floats read from JSON.** `yq -p json` reformats some floats (`1e-7` becomes `1e-07`, Go style). The KYAML report keeps the JCS form (`1e-7`), which `yq -p yaml` preserves, so the fixed point holds. Byte identity with yq's JSON→KYAML path holds whenever yq doesn't reformat, which covers every report checked. ## Validation - `cargo fmt --check`: clean. - `cargo clippy --all-targets -D warnings`, with and without `--all-features`: clean. - `cargo test --all-features`: all 20 suites green. - 8 new unit tests in `kyaml::tests`: - yq layout (expected text from yq); - scalars and lists at the top level; - C0 escapes; - non-printable escapes, including NEL read back intact; - JCS key order with a supplementary-plane key; - JCS and I-JSON numbers; - the real-report golden; - a report round trip. - `--output-format bogus` now lists `json, yaml, kyaml, nickel, sarif`. ## Follow-ups (outside this PR) - `assail` ignores `--output-format` and always writes JCS JSON. Honouring it means deciding which bytes the `--attest` seal hashes. - **Upstream (mikefarah/yq):** escape C1, DEL and NEL in the KYAML encoder. - **standards:** `kyaml-format.sh` could explain its refusal for this case. --- _Generated by [Claude Code](https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k)_ Co-authored-by: Claude <noreply@anthropic.com>
…tput) Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k
hyperpolymath
disabled auto-merge
October 26, 2026 20:04
hyperpolymath
enabled auto-merge (squash)
October 26, 2026 20:04
hyperpolymath
disabled auto-merge
October 26, 2026 21:22
hyperpolymath
deleted the
claude/gallant-bell-4bo4oy-c2-deterministic-assail
branch
October 26, 2026 21:22
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #237 (JCS reports), as noted in that PR.
Problem
Running
assailtwice on the same input gave different reports. Onmain, six runs ofexamples/vulnerable_program.rsproduced six distinct reports. The cause is that these fields were built fromHashSet/HashMapiteration order, which Rust randomises per run:frameworksrecommended_attacksJCS sorts object keys but, by design, leaves array order alone. So since #237, the canonical JSON, and any hash over it such as the
--attestseal, differed for identical input.Fix
BTreeSet/BTreeMap.AttackAxis,FrameworkandWeakPointCategorynow derivePartialOrdandOrd(declaration order). The derives are additive.Evidence
examples/;src/tree.test_assail_report_is_deterministic. It builds a multi-directory fixture whoseCargo.tomltriggers several frameworks and asserts that nine analyses give identical canonical JSON. EveryHashMapin a process gets its own random seed, so this catches regressions within a single test run.HashMapcode restored, it fails with "assail report differs between runs".Verification
cargo fmt --check: clean.cargo build --locked: no warnings.cargo clippy --locked --all-features --all-targets -- -D warnings: clean.cargo test --locked --all-features --no-fail-fast: 693 passed, 0 failed.Generated by Claude Code