Repository navigation
feat(report): add --output-format kyaml - #248
Merged
Merged
Conversation
`crate::kyaml` writes reports in KYAML, the estate YAML dialect, following the KYAML encoder of mikefarah yq v4. Output is a fixed point of `yq -p yaml -o kyaml`, so it passes standards' kyaml-format.sh --check, and a checked-in golden pins it byte-for-byte to yq's output for a real report. Keys and numbers follow JCS (UTF-16 key order, ECMAScript numbers), so the KYAML and JSON reports of a scan hold the same data. YAML non-printables (U+007F..U+009F, U+FFFE, U+FFFF) are escaped. yq writes them raw, and a raw U+0085 reads back as a line break. README listed `markdown` and `text` as --output-format values; it now lists the real ones. 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 20 minutes. View limit details
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 |
hyperpolymath
marked this pull request as ready for review
October 26, 2026 19:52
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 501b7dd072
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+94
to
+95
| .is_some_and(|c| c.is_ascii_alphabetic() || c == '_') | ||
| && chars.all(|c| c.is_ascii_alphanumeric() || c == '_' || c == '-') |
There was a problem hiding this comment.
When to_string receives an object with a key such as "null", "Null", or "NULL", this predicate emits it bare, so YAML resolves the key as null rather than preserving the JSON string key. The configured serde-saphyr loader explicitly treats case-insensitive plain null scalars as null (deserialization contract), causing a round trip into HashMap<String, _> to fail or changing the key's type for untyped consumers. Exclude null-like spellings from bare keys so the advertised JSON/KYAML data equivalence holds.
Useful? React with 👍 / 👎.
Comment on lines
+15
to
+16
| /// KYAML, the estate YAML dialect (D280/D307). | ||
| Kyaml, |
There was a problem hiding this comment.
Adding this ValueEnum variant without regenerating the tracked shell completions leaves every checked-in Bash, Zsh, Fish, and Nushell completion list at json yaml nickel sarif. Users who install the repository-provided completion files therefore cannot discover or complete the new kyaml value; rerun the Justfile completion-generation target and commit its outputs.
Useful? React with 👍 / 👎.
hyperpolymath
added a commit
that referenced
this pull request
Oct 26, 2026
…erify (#247) Stacked PR D of the estate-format series: - A: `.kyaml` loaders - B: example profiles - C: JCS reports - E: KYAML output (#248) A, B, C and E are merged. This PR builds on `crate::json` from the JCS-reports PR. ## What changes - **JCS hashing.** `assail --attest` hashes the intent and evidence records as their JCS (RFC 8785) bytes via the new `crate::json::to_canonical_bytes`. It previously hashed `serde_json::to_vec` struct order. The `.attestation.json` sidecar itself is now written as I-JSON/JCS (J-1). - **Content binding on verify (a fix).** `attest verify` used to recompute only `chain_hash` from the three *stored* digests, so the embedded intent and evidence could be edited after sealing without detection. It now rehashes both from the file and fails on a mismatch. It hashes the parsed JSON tree rather than the typed structs, so fields this build doesn't know about stay inside the hash. - **`hash_scheme` on the seal** (`jcs-sha256`). The scheme is prefixed into the chain hash: `SHA-256(hash_scheme || intent_hash || evidence_hash || report_hash)`. Any Ed25519 signature therefore covers it too. - **Fixed-width digests (Codex P1, fixed in d88ccd9).** `attest verify` rejects any digest field that isn't exactly 64 lowercase hex characters. Without that check, an attacker could move the scheme name into `intent_hash` and drop `hash_scheme`. That reproduced the signed bytes on the legacy path, which skips the content rehash. With fixed widths, the legacy chain input (192 bytes) and the scheme-prefixed one (202 bytes) can't coincide. - **Legacy sidecars.** Sidecars with no `hash_scheme` deserialise as `serde-json-sha256`, use the old chain-hash formula, and verify as before, reported as `Content: not rehashed (legacy hash scheme)`. An unknown scheme fails. ## Validation - `cargo fmt --check`, plus `cargo clippy --all-targets -D warnings` both with `--all-features` and without: clean. - `cargo test --all-features`: all suites green after merging main (lib: 509 passed). - New tests in `src/attestation/mod.rs`: - A fresh sidecar verifies with content bound. - `intent_hash` equals SHA-256 of a **hand-written** JCS string. - Pretty-printing the sidecar keeps it valid. - Editing intent or evidence is detected. - A genuine legacy seal verifies but is unbound. - Relabelling a seal as legacy breaks the chain hash. - The scheme-shift attack is rejected. That test reported *no* failures before d88ccd9. - An unknown scheme fails. - End-to-end with the built binary, cross-checked with tools that share no code with the crate: - `jq -cjS .attestation.intent | sha256sum` and the same for evidence match the seal. - `sha256sum report.json` matches `report_hash`. - `printf 'jcs-sha256%s%s%s' I E R | sha256sum` matches `chain_hash`. - Editing the evidence fails verification. - Downgrade plus edit fails with a chain mismatch. - On a signed sidecar, the signature verifies, a downgrade fails, and the scheme-shift attack fails (`intent_hash is not a SHA-256 digest`). ## Proof angle `src/abi/AttestationUnforgeability.idr` is parametric in `chainHash` under the `chainCR` (injectivity) hypothesis. Its doc now states the scope: the model covers **one scheme at a time**, and `integrity`, `authenticity` and `nonRepudiation` hold per scheme. Scheme binding (no downgrade) is not modelled. In the code it rests on the fixed-width digest check. The Codex P1 was a failure of exactly the injectivity `chainCR` assumes: the bug was in the encoding, not in SHA-256. The model can be extended with a fourth `chainHash` input and a no-downgrade lemma. That needs an Idris2 type-check, which this environment couldn't run, and no workflow here type-checks `src/abi/`. The open review thread on `seal.rs` holds that decision for the owner. ## Follow-ups (outside this PR) - `attest verify --report <file>` to recompute `report_hash` from the report itself; today it is only cross-checked against `decision_hash`. - A `--require-bound` mode (or a policy default) that rejects legacy, unbound seals. - An Idris2 type-check job for `src/abi/` in CI. - `evidence.evidence_hash` hashes `usize::to_le_bytes`, whose width depends on the platform. That is harmless for the seal, which uses JCS, but not portable. - Consumers that parse the sidecar (DYADT / K9 contractiles) should read `hash_scheme`. That work is in other repositories. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k --------- Signed-off-by: Jonathan D.A. Jewell <6759885+hyperpolymath@users.noreply.github.com> Co-authored-by: Claude <noreply@anthropic.com>
hyperpolymath
added a commit
that referenced
this pull request
Oct 26, 2026
Follow-up to #248 (KYAML report output), found while drafting the upstream yq issue about its KYAML encoder. ## Problem go-yaml, the parser behind `yq` and standards' `scripts/kyaml-format.sh`, treats a raw U+2028 (LS) or U+2029 (PS) as a line break, which is YAML 1.1 behaviour. Our emitter already escaped NEL (U+0085) but wrote LS and PS raw. A report key holding either one therefore produced KYAML that `yq` could not read at all: ``` $ yq -p yaml -o json old.kyaml # key "x<LS>y" written raw Error: ... go-yaml load error in parser (while parsing a flow mapping) ... did not find expected ',' or '}' ``` Values were unaffected, and `crate::yaml` (serde-saphyr, YAML 1.2) read these files correctly before and after. In #248 I deliberately left U+2028 raw because YAML 1.2 calls it printable. Testing keys showed that was the wrong call for interop. ## Fix `write_quoted` also escapes U+2028 and U+2029 as ` ` and ` `. Those escapes are valid in YAML 1.1 and 1.2, so nothing that parsed the old output loses anything. ## Validation - `cargo fmt --check`, plus `cargo clippy --all-targets -D warnings` with and without `--all-features`: clean. - `cargo test --all-features`: all suites green (lib: 510 passed). - **New test `line_breaks_in_keys_are_escaped`** covers NEL, LS and PS in keys: exact output, and a round trip through `crate::yaml`. Run against the previous escape set, it fails with the raw separator in the output (`"x\u{2028}y"`). - `escapes_non_printables_that_yq_writes_raw` is updated: it asserted LS stays raw, and now asserts it is escaped. - **End to end with real `yq` v4.54.1:** KYAML from this branch with NEL, LS or PS in a key reads back intact (`yq -p yaml -o json`). ## One thing this PR can't fix `kyaml-format.sh --check` still **refuses** these files: `REFUSED — the KYAML rendering does not parse to the same data`. The gate re-renders through `yq -o kyaml`, and yq's encoder writes the characters raw again. That is the upstream yq bug: - With a `yq` built from my proposed upstream patch, the same three files pass (`all 3 file(s) are KYAML`). - The upstream issue is drafted but **not filed yet**; filing it is the owner's call. Until yq is fixed, a report key containing NEL, LS or PS can't pass the gate. Such keys are rare in practice, since report keys are field names and file paths. --- _Generated by [Claude Code](https://claude.ai/code/session_01PFaqv6iBXrt3W5Akd1Fq5k)_ Co-authored-by: Claude <noreply@anthropic.com>
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.
Stacked PR E of the estate-format series:
.kyamlloadersThis PR is independent of #245 and #247 and branches from
main.What changes
src/kyaml.rs. A KYAML emitter that follows mikefarahyqv4'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.ReportOutputFormat::Kyamladds--output-format kyaml, extension.kyaml, parsed from"kyaml". Loaders have read.kyamlsince PR A.README.adoclisted--output-formatvaluesmarkdownandtext, which never existed. It now lists the real ones.Contract, checked against the real tools
yq -p yaml -o kyaml, the testscripts/kyaml-format.sh --checkrunsscripts/kyaml-format.sh --check, run for realall 4 file(s) are KYAMLyq -p json -o kyamlof the JCS reportreports/*.yamlassault reports, plus assail ofsrc/(52 KB) andtests/crate::yaml(serde-saphyr)The test
real_report_is_byte_identical_to_yqpins this in CI without yq.tests/fixtures/kyaml/assail-report.kyamlis yq's output for the checked-in JCS report next to it.Two documented differences from yq
\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.shreportsREFUSED … 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.escapeDoubleQuotedStringupstream.yq -p jsonreformats some floats (1e-7becomes1e-07, Go style). The KYAML report keeps the JCS form (1e-7), whichyq -p yamlpreserves, 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.kyaml::tests:--output-format bogusnow listsjson, yaml, kyaml, nickel, sarif.Follow-ups (outside this PR)
assailignores--output-formatand always writes JCS JSON. Honouring it means deciding which bytes the--attestseal hashes.kyaml-format.shcould explain its refusal for this case.Generated by Claude Code