docs: FS↔M anchors+mapping, release workflow, Rust WASM policy demo, tip link #87
Conversation
…tor; CI job to fail on divergence; add deterministic WASM policy ABI doc; add spec linter (id/signing/schema)
…cal bytes; versioning policy + CHANGELOG; wire-format CBOR profile
- Add explicit HTML anchors for FS‑1..FS‑14 and M‑1..M‑9 - Insert FS↔M mapping table for cross‑references - Keeps ignoreDeadLinks in place; links build clean locally
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Summary by CodeRabbitRelease Notes
WalkthroughAdds CI/release workflows, multi-language canonicalization tooling and vectors, extensive spec/model normative content (FS‑1..FS‑14 + mappings), a deterministic WASM policy demo and ABI doc, a compliance harness with many feedback tests and runner, and miscellaneous docs/config and repo hygiene updates. Changes
Sequence Diagram(s)sequenceDiagram
actor Developer
participant GitHub
participant VectorsWF as Vectors Workflows
participant ReleaseWF as Release Workflow
participant Tools
participant GHRelease as softprops/action-gh-release
Developer->>GitHub: push tag refs/tags/vX.Y.Z
GitHub->>ReleaseWF: trigger on tag push
ReleaseWF->>ReleaseWF: checkout (fetch-depth:0), check directories
ReleaseWF->>Tools: zip schemas -> dist/schemas.zip
ReleaseWF->>Tools: zip tests/vectors + scripts/vectors -> dist/vectors.zip
ReleaseWF->>GHRelease: publish release with artifacts
Developer->>GitHub: push branch / PR
GitHub->>VectorsWF: trigger on push/PR
VectorsWF->>Tools: run Python/Go/Rust/C tools -> compute IDs
VectorsWF->>VectorsWF: compare IDs, fail on mismatch
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
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 |
![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.
ℹ️ 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".
| #![no_std] | ||
|
|
||
| // Minimal, deterministic policy demo ABI. | ||
| // | ||
| // Exported symbol: | ||
| // validate(entry_ptr, entry_len, state_ptr, state_len, out_ptr, out_len_ptr) -> u32 | ||
| // | ||
| // Semantics: | ||
| // - Returns 0 to indicate acceptance; non-zero for rejection. | ||
| // - Writes a short UTF-8 message into [out_ptr, out_ptr+*out_len_ptr) if non-null. | ||
| // - This demo accepts iff entry_len is even AND state_len is even; otherwise rejects. | ||
| // - Purely deterministic: no clock, randomness, or host I/O. | ||
|
|
||
| #[no_mangle] | ||
| pub extern "C" fn validate( | ||
| _entry_ptr: *const u8, | ||
| entry_len: usize, | ||
| _state_ptr: *const u8, | ||
| state_len: usize, | ||
| out_ptr: *mut u8, | ||
| out_len_ptr: *mut usize, | ||
| ) -> u32 { | ||
| let accept = (entry_len & 1) == 0 && (state_len & 1) == 0; | ||
| let msg = if accept { b"ACCEPT\n" } else { b"REJECT\n" }; | ||
|
|
||
| unsafe { | ||
| if !out_ptr.is_null() && !out_len_ptr.is_null() { | ||
| let mut n = core::ptr::read(out_len_ptr); | ||
| if n > msg.len() { n = msg.len(); } | ||
| core::ptr::copy_nonoverlapping(msg.as_ptr(), out_ptr, n); | ||
| core::ptr::write(out_len_ptr, n); | ||
| } | ||
| } | ||
| if accept { 0 } else { 1 } | ||
| } |
There was a problem hiding this comment.
The new WASM policy demo is compiled as a cdylib with #![no_std] but does not define a #[panic_handler] (nor depend on a crate that supplies one). no_std crates require a panic handler; without it, cargo build --target wasm32-unknown-unknown fails with lang item required, but not found: panic_impl, so the example cannot be built as advertised.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/spec/formal-spec.md (1)
46-56:⚠️ Critical: Add blank lines before section headings to comply with Markdown standards.Static analysis flags 16 MD022 violations: all section headings (FS-4 through FS-13) lack required blank lines above and below. Per markdownlint, headings must be surrounded by blank lines. This breaks the document's visual hierarchy and violates standard Markdown structure.
Apply this diff to fix all heading spacing:
If any step fails, the implementation MUST NOT advance the ref and MUST return an error. + -### FS‑5 replay(R, L) +### FS‑5 replay(R, L) Replay SHALL fold the deterministic transition function over the ordered Entries (M‑2) from the genesis to head, producing `state(L)`. Replay MUST NOT consult external state. + -### FS‑6 verify(R, L) +### FS‑6 verify(R, L) Verification SHALL check the entire history of `L` for conformity: ordering, fast‑forward ref evolution, canonical hashes, required attestations, deterministic policy acceptance, and deterministic replay equivalence. Verification MUST be possible using repository‑local data only (no network). + ## 3. Determinism & Constraints + -### FS‑7 Append‑Only & Total Order +### FS‑7 Append‑Only & Total OrderRepeat this pattern for all remaining headings through FS-14.
Also applies to: 58-64, 68-74, 76-78, 80-82, 84-86, 88-90, 94-100
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (11)
.github/workflows/release.yml(1 hunks).gitignore(1 hunks)docs/compliance/index.md(3 hunks)docs/implementation/errors.md(1 hunks)docs/spec/formal-spec.md(6 hunks)docs/spec/model-source.md(1 hunks)docs/spec/model.md(6 hunks)tests/policy/wasm/demo/Cargo.toml(1 hunks)tests/policy/wasm/demo/Makefile(1 hunks)tests/policy/wasm/demo/README.md(1 hunks)tests/policy/wasm/demo/src/lib.rs(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
tests/policy/wasm/demo/Makefile
[warning] 5-5: Missing required phony target "test"
(minphony)
🪛 markdownlint-cli2 (0.18.1)
docs/compliance/index.md
126-126: Hard tabs
Column: 13
(MD010, no-hard-tabs)
126-126: Hard tabs
Column: 24
(MD010, no-hard-tabs)
128-128: Hard tabs
Column: 22
(MD010, no-hard-tabs)
128-128: Table column count
Expected: 3; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
129-129: Hard tabs
Column: 18
(MD010, no-hard-tabs)
129-129: Hard tabs
Column: 50
(MD010, no-hard-tabs)
129-129: Table column count
Expected: 3; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
130-130: Hard tabs
Column: 18
(MD010, no-hard-tabs)
130-130: Hard tabs
Column: 42
(MD010, no-hard-tabs)
130-130: Table column count
Expected: 3; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
131-131: Hard tabs
Column: 22
(MD010, no-hard-tabs)
131-131: Table column count
Expected: 3; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
132-132: Trailing spaces
Expected: 0 or 2; Actual: 1
(MD009, no-trailing-spaces)
132-132: Hard tabs
Column: 21
(MD010, no-hard-tabs)
132-132: Hard tabs
Column: 44
(MD010, no-hard-tabs)
132-132: Table column count
Expected: 3; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
133-133: Hard tabs
Column: 16
(MD010, no-hard-tabs)
133-133: Table column count
Expected: 3; Actual: 1; Too few cells, row will be missing data
(MD056, table-column-count)
204-204: Ordered list item prefix
Expected: 1; Actual: 12; Style: 1/1/1
(MD029, ol-prefix)
docs/spec/model-source.md
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/spec/formal-spec.md
59-59: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
59-59: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
63-63: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
63-63: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
69-69: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
69-69: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
73-73: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
73-73: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
77-77: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
77-77: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
81-81: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
81-81: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
85-85: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
85-85: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
89-89: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
89-89: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
95-95: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
95-95: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
tests/policy/wasm/demo/README.md
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🪛 YAMLlint (1.37.1)
.github/workflows/release.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 34-34: too many blank lines (1 > 0)
(empty-lines)
🔇 Additional comments (12)
docs/implementation/errors.md (2)
9-11: VitePress container syntax is correct.The
::: tip ... :::admonition block follows standard VitePress conventions and integrates cleanly with the document structure.
9-11: Link target confirmed—no issues found.The reference to
/cli/harnessis valid.docs/cli/harness.mdexists in the codebase. The code change requires no modifications..gitignore (1)
70-72: Good organization—tmp/ directory pattern is properly sectioned.The addition follows existing conventions in the file with a clear section header. The
tmp/pattern correctly matches the temporary artifacts directory referenced in the release workflow.docs/spec/model.md (1)
13-16: ✅ Anchor placement and cross-referencing are well-structured.All M-1 through M-9 anchors are correctly positioned (lowercase IDs:
m-1,m-2, …,m-9) immediately preceding their respective info blocks. Cross-references to FS sections are present and use consistent notation. The content expansions clarify state transition semantics, admission predicates, invariants, and deterministic replay properties—all critical to formal compliance.Also applies to: 30-33, 43-49, 55-58, 60-63, 65-68, 74-79, 91-94, 98-101
docs/spec/formal-spec.md (2)
110-125: ✅ FS↔M Mapping table is semantically sound and properly cross-referenced.The mapping correctly links all 9 Model clauses to their substantiating Formal Spec clauses:
- M-1 (Ledger structure) → FS-1, FS-2, FS-3 ✓
- M-2 (State and transition) → FS-5, FS-10 ✓
- M-3 (Admission) → FS-3, FS-4 ✓
- M-4..M-6 (Invariants) → FS-7, FS-8 ✓
- M-7 (Deterministic replay) → FS-5, FS-9, FS-10 ✓
- M-8 (Offline verify) → FS-6 ✓
- M-9 (Policy determinism) → FS-3, FS-9 ✓
All anchor ID references use lowercase (e.g.,
#fs-1,#fs-5) matching the anchor definitions. Markdown table syntax is correct; no structural defects.
11-21: ✅ FS-1 and FS-2 data structure definitions are precise.Entry (FS-1) and Attestation (FS-2) sections clearly specify immutability, content-addressability, and deterministic serialization—critical prerequisites for later operations. The cross-reference to M-1 via "See FS-1..FS-3" in model.md is correctly grounded here.
docs/spec/model-source.md (1)
1-8: ✅ Metadata is complete and professionally formatted.Frontmatter now includes author attribution, creation date (2025-10-27), license (MIT), scope, status (Draft), summary, and semantic version (0.1.0). This aligns with formal specification practices and enables version tracking for compliance.
tests/policy/wasm/demo/Cargo.toml (1)
1-7: LGTM: Package and library configuration is correct for WASM.The
cdylibcrate-type is the appropriate choice for producing a C-compatible dynamic library for WASM targets.tests/policy/wasm/demo/Makefile (1)
1-7: LGTM: Variable definitions and phony targets are appropriate.The Makefile structure is clean and straightforward for a build-only workflow.
tests/policy/wasm/demo/src/lib.rs (3)
14-22: LGTM: Function signature correctly implements the C ABI contract.The
#[no_mangle]andextern "C"attributes are properly applied. The underscore-prefixed parameters correctly signal that the pointers are intentionally unused in this demo implementation.
23-24: LGTM: Acceptance logic is correct and efficient.The bitwise AND to check for even numbers (
& 1 == 0) is idiomatic and efficient. The logic correctly implements the documented behavior.
26-35: Unsafe block requires documented caller guarantees (already flagged above).The unsafe operations are structurally sound for FFI but make strong assumptions:
What's done correctly:
- Null pointer checks before dereferencing
- Length capping to prevent writing beyond message bounds
- Returns actual bytes written via
out_len_ptrCritical assumptions (must be documented):
out_len_ptrpoints to valid, aligned memoryout_ptris valid for writes of*out_len_ptrbytes- The output buffer has sufficient capacity
- No overlap between buffers (guaranteed by FFI contract)
This is standard FFI practice where the caller bears responsibility for providing valid pointers, but these preconditions must be explicitly documented (addressed in previous comment).
One edge case to verify: what happens if the caller passes
out_len_ptrpointing to a value larger than the actual buffer capacity? The code caps tomsg.len()but doesn't cap to the actual available buffer size.Example: Caller passes
out_ptrpointing to a 4-byte buffer butout_len_ptrpointing to value1000. The code will cap tomsg.len()(7 bytes for "ACCEPT\n"), but this exceeds the 4-byte buffer, causing undefined behavior.This is the caller's responsibility per FFI contracts, but should be made explicit in documentation:
// # Safety Requirements (Caller Contract): // ... // - If out_ptr is non-null, the buffer [out_ptr, out_ptr + *out_len_ptr) must be valid for writes // - The caller MUST ensure *out_len_ptr does not exceed the actual buffer capacity
.github/workflows/release.yml
Outdated
| name: Release Artifacts | ||
|
|
||
| on: | ||
| push: | ||
| tags: | ||
| - 'v*' | ||
|
|
||
| permissions: | ||
| contents: write | ||
|
|
||
| jobs: | ||
| release: | ||
| runs-on: ubuntu-latest | ||
| steps: | ||
| - name: Checkout | ||
| uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 | ||
|
|
||
| - name: Prepare assets | ||
| run: | | ||
| mkdir -p dist | ||
| zip -r dist/schemas.zip schemas | ||
| zip -r dist/vectors.zip tests/vectors scripts/vectors | ||
| - name: Create GitHub Release | ||
| uses: softprops/action-gh-release@v2 | ||
| with: | ||
| files: | | ||
| dist/schemas.zip | ||
| dist/vectors.zip | ||
| env: | ||
| GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Tag-triggered release workflows need safeguards to prevent accidental/invalid releases.
The workflow triggers on any tag matching v*. This is permissive and could inadvertently release from development/experiment tags (e.g., v-wip, v.test). Consider:
- Stricter tag pattern:
v[0-9]+.[0-9]+.[0-9]+*for semantic versioning. - Branch guard: Add a condition to only release from
main(or similar). - Draft releases: Use
draft: trueinsoftprops/action-gh-release@v2to require manual promotion.
Apply this diff to tighten the trigger pattern and add a branch guard:
on:
push:
tags:
- - 'v*'
+ - 'v[0-9]+.[0-9]+.[0-9]+*'
permissions:
contents: write
jobs:
release:
runs-on: ubuntu-latest
+ if: github.ref_type == 'tag' && startsWith(github.ref, 'refs/tags/v')
steps:Or, use softprops/action-gh-release draft mode for manual promotion:
- name: Create GitHub Release
uses: softprops/action-gh-release@v2
with:
+ draft: true
files: |
dist/schemas.zip
dist/vectors.zip🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 34-34: too many blank lines (1 > 0)
(empty-lines)
docs/compliance/index.md
Outdated
| ## 1. Purpose | ||
|
|
||
| The compliance suite ensures that any implementation of the **Ledger-Kernel** (e.g., `libgitledger`, `ledger-core-rust`, `ledger-js`) adheres to the invariants, semantics, and deterministic behavior defined in [`SPEC`](../spec/) and [`MODEL`](../model/). | ||
| The compliance suite ensures that any implementation of the **Ledger-Kernel** (e.g., `libgitledger`, `ledger-core-rust`, `ledger-js`) adheres to the invariants, semantics, and deterministic behavior defined in [`SPEC.md`](./SPEC.md) and [`MODEL.md`](./MODEL.md). |
There was a problem hiding this comment.
🧩 Analysis chain
Verify link targets: SPEC.md and MODEL.md.
Line 14 updates references to ./SPEC.md and ./MODEL.md. Confirm these files exist at the compliance directory level or adjust relative paths accordingly. Broken links will harm navigation and compliance tracking.
🏁 Script executed:
#!/bin/bash
# Verify SPEC.md and MODEL.md exist relative to compliance/index.md
cd docs/compliance || exit 1
for file in SPEC.md MODEL.md; do
if [ -f "$file" ]; then
echo "✓ Found: $file"
else
# Check parent directories
if [ -f "../spec/formal-spec.md" ] && [ "$file" = "SPEC.md" ]; then
echo "⚠ $file not found locally; check if reference should be ../spec/formal-spec.md"
elif [ -f "../spec/model.md" ] && [ "$file" = "MODEL.md" ]; then
echo "⚠ $file not found locally; check if reference should be ../spec/model.md"
else
echo "✗ $file NOT FOUND; relative link is broken"
fi
fi
doneLength of output: 65
🏁 Script executed:
#!/bin/bash
# Explore repository structure and locate relevant files
echo "=== Searching for index.md files ==="
fd -t f "index.md" . | head -20
echo ""
echo "=== Searching for SPEC.md files ==="
fd -t f "SPEC.md" . | head -20
echo ""
echo "=== Searching for MODEL.md files ==="
fd -t f "MODEL.md" . | head -20
echo ""
echo "=== Checking docs/ directory structure ==="
fd -t d "docs" . | head -10
fd -t f . docs/ 2>/dev/null | head -30Length of output: 1164
🏁 Script executed:
#!/bin/bash
# Extract line 14 from docs/compliance/index.md to verify the exact links
echo "=== Content around line 14 in docs/compliance/index.md ==="
sed -n '10,18p' docs/compliance/index.mdLength of output: 492
Fix broken documentation links: use correct relative paths to SPEC.md and MODEL.md.
The links ./SPEC.md and ./MODEL.md in line 14 are broken. These files reside at the repository root, not in docs/compliance/. Update to:
../../SPEC.md../../MODEL.md
Broken links will render as dead ends in the compliance documentation and break navigation flow.
🤖 Prompt for AI Agents
In docs/compliance/index.md around line 14, the relative links ./SPEC.md and
./MODEL.md point to files in the repo root but resolve incorrectly from
docs/compliance/; update the two links to use the correct relative paths
../../SPEC.md and ../../MODEL.md so they point to the repository root files.
| Author: J. Kirby Ross <[email protected]> (https://github.com/flyingrobots) | ||
| Created: 2025-10-27 | ||
| License: MIT | ||
| Scope: This document defines the minimal, requisite semantics and invariants that a Git-native ledger implementation must satisfy to be considered compliant. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Add language specifiers to all fenced code blocks (MD040).
Fenced blocks at lines 5, 20, 26, 32 lack language identifiers (e.g., ```math or ```latex). While semantically neutral for content display, explicitly declaring the block type improves tooling and accessibility.
-```
+```math
\mathcal{R} = (\mathcal{O}, \mathcal{R_{efs}})
-```
+```Repeat for all remaining code fences.
Also applies to: 20-20, 26-26, 32-32
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In docs/spec/model-source.md around lines 5, 20, 26, and 32, several fenced code
blocks are missing language specifiers; update each opening fence to include an
appropriate language identifier (e.g., ```math or ```latex) that reflects the
block content, leaving the closing fences as-is so the blocks remain properly
delimited; ensure the chosen identifier matches the block semantics for tooling
and accessibility.
| build: | ||
| rustup target add $(TARGET) >/dev/null 2>&1 || true | ||
| cargo build --release --target $(TARGET) |
There was a problem hiding this comment.
The silent error suppression is unacceptable for a build system.
Line 10 uses || true to suppress all errors from rustup target add, which will hide legitimate failures (rustup not installed, network issues, permission errors) and cause confusing downstream failures in cargo.
Additionally, there's no verification that the build actually succeeded or produced the expected output.
Apply this diff to improve error handling and verification:
build:
- rustup target add $(TARGET) >/dev/null 2>&1 || true
+ @rustup target add $(TARGET) 2>&1 | grep -q "already installed\|has been installed" || \
+ (rustup target add $(TARGET) && echo "✓ Added target $(TARGET)")
cargo build --release --target $(TARGET)
+ @test -f $(OUT) || (echo "ERROR: Build failed to produce $(OUT)" && exit 1)
+ @echo "✓ Built $(OUT) ($$(wc -c < $(OUT)) bytes)"Or, for a simpler approach that only checks if rustup exists:
build:
- rustup target add $(TARGET) >/dev/null 2>&1 || true
+ @command -v rustup >/dev/null || (echo "ERROR: rustup not found" && exit 1)
+ @rustup target add $(TARGET) 2>/dev/null || true
cargo build --release --target $(TARGET)
+ @test -f $(OUT) && echo "✓ Built $(OUT)" || echo "ERROR: Build failed"Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In tests/policy/wasm/demo/Makefile around lines 9 to 11, remove the silent
suppression "|| true" and add explicit checks and verification: first ensure
rustup is available (exit with a clear error if not), run `rustup target add
$(TARGET)` without swallowing errors so failures are visible, run `cargo build
--release --target $(TARGET)` and ensure it fails on non‑zero exit, then after
build verify the expected output artifact (the .wasm file under
target/$(TARGET)/release/) exists and exit with an error message if it does not.
Ensure all commands propagate failures so the make target fails loudly on
problems.
tests/policy/wasm/demo/README.md
Outdated
| ``` | ||
| // extern "C" ABI | ||
| u32 validate(entry_ptr, entry_len, state_ptr, state_len, out_ptr, out_len_ptr) | ||
| ``` | ||
|
|
||
| - Returns `0` on acceptance; non-zero on rejection. | ||
| - Writes a short message (`ACCEPT` or `REJECT`) into the caller-provided buffer if non-null. | ||
| - Deterministic: no clock, RNG, network, filesystem, or host I/O. | ||
|
|
||
| Acceptance rule (demo): accept iff both `entry_len` and `state_len` are even. | ||
|
|
||
| ## Build | ||
|
|
||
| - Install the target once: | ||
|
|
||
| ``` | ||
| rustup target add wasm32-unknown-unknown | ||
| ``` | ||
|
|
||
| - Build release artifact: | ||
|
|
||
| ``` | ||
| cargo build --release --target wasm32-unknown-unknown | ||
| ``` | ||
|
|
||
| - The resulting module will be at: | ||
|
|
||
| ``` | ||
| ./target/wasm32-unknown-unknown/release/policy_wasm_demo.wasm | ||
| ``` |
There was a problem hiding this comment.
Fix markdown code block language specifiers for proper syntax highlighting.
Four code blocks are missing language specifiers, which prevents proper syntax highlighting and violates documentation standards.
Apply this diff:
-```
+```c
// extern "C" ABI
u32 validate(entry_ptr, entry_len, state_ptr, state_len, out_ptr, out_len_ptr)
```
Acceptance rule (demo): accept iff both `entry_len` and `state_len` are even.
## Build
- Install the target once:
-```
+```bash
rustup target add wasm32-unknown-unknown
```
- Build release artifact:
-```
+```bash
cargo build --release --target wasm32-unknown-unknown
```
- The resulting module will be at:
-```
+```text
./target/wasm32-unknown-unknown/release/policy_wasm_demo.wasm
```🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
5-5: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
20-20: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
26-26: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
32-32: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for AI Agents
In tests/policy/wasm/demo/README.md around lines 5 to 34, several fenced code
blocks lack language specifiers; add them to enable syntax highlighting and
match the suggested diff: change the first code block to use ```c, the two
command blocks to use ```bash, and the final path block to use ```text; ensure
you only modify the opening fence lines to include the language identifiers and
keep the existing code and closing fences unchanged.
…rs (resolve .gitignore)
… CI vectors C step, implementers guide edits, config tweaks)
There was a problem hiding this comment.
Actionable comments posted: 29
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (2)
CHANGELOG.mdis excluded by!*.mdREADME.mdis excluded by!*.md
📒 Files selected for processing (22)
.github/workflows/vectors-cbor.yml(1 hunks).github/workflows/vectors-matrix.yml(1 hunks).gitignore(1 hunks)docs/.vitepress/config.ts(1 hunks)docs/implementation/implementers.md(5 hunks)docs/spec/policy-wasm.md(1 hunks)docs/spec/versioning.md(1 hunks)docs/spec/wire-format.md(1 hunks)scripts/harness/README.md(1 hunks)scripts/harness/config.sample.toml(1 hunks)scripts/harness/run.sh(1 hunks)scripts/lint/spec_lint.py(1 hunks)scripts/vectors/c/blake3_id.c(1 hunks)scripts/vectors/go-cbor/go.mod(1 hunks)scripts/vectors/go-cbor/main.go(1 hunks)scripts/vectors/go/go.mod(1 hunks)scripts/vectors/go/main.go(1 hunks)scripts/vectors/python/canon.py(1 hunks)scripts/vectors/python/cbor_canon.py(1 hunks)scripts/vectors/rust/Cargo.toml(1 hunks)scripts/vectors/rust/src/main.rs(1 hunks)tests/vectors/core/entry_canonical.json(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (5)
scripts/vectors/rust/src/main.rs (2)
scripts/lint/spec_lint.py (3)
canonical(11-27)preimage(29-32)main(52-70)scripts/vectors/python/canon.py (3)
canonical(12-30)preimage(32-36)main(38-49)
scripts/lint/spec_lint.py (1)
scripts/vectors/python/canon.py (3)
canonical(12-30)preimage(32-36)main(38-49)
scripts/vectors/python/cbor_canon.py (1)
scripts/vectors/c/blake3_id.c (1)
main(7-20)
scripts/vectors/c/blake3_id.c (1)
scripts/vectors/python/cbor_canon.py (1)
main(14-25)
scripts/vectors/python/canon.py (2)
scripts/lint/spec_lint.py (3)
canonical(11-27)preimage(29-32)main(52-70)scripts/vectors/rust/src/main.rs (3)
canonical(5-27)preimage(29-33)main(35-42)
🪛 actionlint (1.7.8)
.github/workflows/vectors-matrix.yml
22-22: shellcheck reported issue in this script: SC2086:info:2:31: Double quote to prevent globbing and word splitting
(shellcheck)
34-34: shellcheck reported issue in this script: SC2086:info:2:31: Double quote to prevent globbing and word splitting
(shellcheck)
46-46: shellcheck reported issue in this script: SC2086:info:2:31: Double quote to prevent globbing and word splitting
(shellcheck)
.github/workflows/vectors-cbor.yml
16-16: shellcheck reported issue in this script: SC2086:info:3:36: Double quote to prevent globbing and word splitting
(shellcheck)
25-25: shellcheck reported issue in this script: SC2086:info:3:36: Double quote to prevent globbing and word splitting
(shellcheck)
31-31: property "id" is not defined in object type {cache-hit: string; go-version: string}
(expression)
🪛 Clang (14.0.6)
scripts/vectors/c/blake3_id.c
[error] 1-1: 'stdio.h' file not found
(clang-diagnostic-error)
🪛 GitHub Actions: Canonicalization Vectors (Python/Rust/Go)
scripts/vectors/go/main.go
[error] 10-10: missing go.sum entry for module providing package lukechampine.com/blake3 (imported by lk_canon_go); to add: go mod tidy or run 'go get lukechampine.com/blake3@latest'.
scripts/vectors/go-cbor/main.go
[error] 10-10: missing go.sum entry for module providing package lukechampine.com/blake3 (imported by lk_canon_go); to add: go mod tidy or run 'go get lukechampine.com/blake3@latest'.
🪛 GitHub Actions: CBOR Vectors (Python/Go)
scripts/vectors/go/main.go
[error] 9-9: Command failed: 'cd scripts/vectors/go-cbor && go build -o ../../bin_lk_cbor_go .' - missing go.sum entry for module providing package github.com/fxamacker/cbor/v2 (imported by lk_cbor_go). To fix, run 'go mod tidy' or add the module with the correct version.
scripts/vectors/go-cbor/main.go
[error] 9-9: Command failed: 'cd scripts/vectors/go-cbor && go build -o ../../bin_lk_cbor_go .' - missing go.sum entry for module providing package github.com/fxamacker/cbor/v2 (imported by lk_cbor_go). To fix, run 'go mod tidy' or add the module with the correct version.
🪛 LanguageTool
docs/spec/policy-wasm.md
[style] ~17-~17: Three successive sentences begin with the same word. Consider rewording the sentence or use a thesaurus to find a synonym.
Context: ...rts or entropy sources are forbidden. - No I/O: filesystem, network, environment v...
(ENGLISH_WORD_REPEAT_BEGINNING_RULE)
🪛 markdownlint-cli2 (0.18.1)
docs/spec/wire-format.md
158-158: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Above
(MD022, blanks-around-headings)
scripts/harness/README.md
11-11: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
20-20: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
docs/implementation/implementers.md
34-34: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
🪛 OSV Scanner (2.2.4)
scripts/vectors/go/go.mod
[LOW] 1-1: stdlib 1.21.99: Stack exhaustion in all Parse functions in go/parser
(GO-2024-3105)
[LOW] 1-1: stdlib 1.21.99: Stack exhaustion in Decoder.Decode in encoding/gob
(GO-2024-3106)
[LOW] 1-1: stdlib 1.21.99: Stack exhaustion in Parse in go/build/constraint
(GO-2024-3107)
[LOW] 1-1: stdlib 1.21.99: Usage of IPv6 zone IDs can bypass URI name constraints in crypto/x509
(GO-2025-3373)
[LOW] 1-1: stdlib 1.21.99: Sensitive headers incorrectly sent after cross-domain redirect in net/http
(GO-2025-3420)
[LOW] 1-1: stdlib 1.21.99: Timing sidechannel for P-256 on ppc64le in crypto/internal/nistec
(GO-2025-3447)
[LOW] 1-1: stdlib 1.21.99: Request smuggling due to acceptance of invalid chunked data in net/http
(GO-2025-3563)
[LOW] 1-1: stdlib 1.21.99: Inconsistent handling of O_CREATE|O_EXCL on Unix and Windows in os in syscall
(GO-2025-3750)
[LOW] 1-1: stdlib 1.21.99: Sensitive headers not cleared on cross-origin redirect in net/http
(GO-2025-3751)
[LOW] 1-1: stdlib 1.21.99: Incorrect results returned from Rows.Scan in database/sql
(GO-2025-3849)
[LOW] 1-1: stdlib 1.21.99: Unexpected paths returned from LookPath in os/exec
(GO-2025-3956)
[LOW] 1-1: stdlib 1.21.99: Excessive CPU consumption in ParseAddress in net/mail
(GO-2025-4006)
[LOW] 1-1: stdlib 1.21.99: Quadratic complexity when checking name constraints in crypto/x509
(GO-2025-4007)
[LOW] 1-1: stdlib 1.21.99: ALPN negotiation error contains attacker controlled information in crypto/tls
(GO-2025-4008)
[LOW] 1-1: stdlib 1.21.99: Quadratic complexity when parsing some invalid inputs in encoding/pem
(GO-2025-4009)
[LOW] 1-1: stdlib 1.21.99: Insufficient validation of bracketed IPv6 hostnames in net/url
(GO-2025-4010)
[LOW] 1-1: stdlib 1.21.99: Parsing DER payload can cause memory exhaustion in encoding/asn1
(GO-2025-4011)
[LOW] 1-1: stdlib 1.21.99: Lack of limit when parsing cookies can cause memory exhaustion in net/http
(GO-2025-4012)
[LOW] 1-1: stdlib 1.21.99: Panic when validating certificates with DSA public keys in crypto/x509
(GO-2025-4013)
[LOW] 1-1: stdlib 1.21.99: Unbounded allocation when parsing GNU sparse map in archive/tar
(GO-2025-4014)
[LOW] 1-1: stdlib 1.21.99: Excessive CPU consumption in Reader.ReadResponse in net/textproto
(GO-2025-4015)
scripts/vectors/go-cbor/go.mod
[LOW] 1-1: stdlib 1.21.99: Stack exhaustion in all Parse functions in go/parser
(GO-2024-3105)
[LOW] 1-1: stdlib 1.21.99: Stack exhaustion in Decoder.Decode in encoding/gob
(GO-2024-3106)
[LOW] 1-1: stdlib 1.21.99: Stack exhaustion in Parse in go/build/constraint
(GO-2024-3107)
[LOW] 1-1: stdlib 1.21.99: Usage of IPv6 zone IDs can bypass URI name constraints in crypto/x509
(GO-2025-3373)
[LOW] 1-1: stdlib 1.21.99: Sensitive headers incorrectly sent after cross-domain redirect in net/http
(GO-2025-3420)
[LOW] 1-1: stdlib 1.21.99: Timing sidechannel for P-256 on ppc64le in crypto/internal/nistec
(GO-2025-3447)
[LOW] 1-1: stdlib 1.21.99: Request smuggling due to acceptance of invalid chunked data in net/http
(GO-2025-3563)
[LOW] 1-1: stdlib 1.21.99: Inconsistent handling of O_CREATE|O_EXCL on Unix and Windows in os in syscall
(GO-2025-3750)
[LOW] 1-1: stdlib 1.21.99: Sensitive headers not cleared on cross-origin redirect in net/http
(GO-2025-3751)
[LOW] 1-1: stdlib 1.21.99: Incorrect results returned from Rows.Scan in database/sql
(GO-2025-3849)
[LOW] 1-1: stdlib 1.21.99: Unexpected paths returned from LookPath in os/exec
(GO-2025-3956)
[LOW] 1-1: stdlib 1.21.99: Excessive CPU consumption in ParseAddress in net/mail
(GO-2025-4006)
[LOW] 1-1: stdlib 1.21.99: Quadratic complexity when checking name constraints in crypto/x509
(GO-2025-4007)
[LOW] 1-1: stdlib 1.21.99: ALPN negotiation error contains attacker controlled information in crypto/tls
(GO-2025-4008)
[LOW] 1-1: stdlib 1.21.99: Quadratic complexity when parsing some invalid inputs in encoding/pem
(GO-2025-4009)
[LOW] 1-1: stdlib 1.21.99: Insufficient validation of bracketed IPv6 hostnames in net/url
(GO-2025-4010)
[LOW] 1-1: stdlib 1.21.99: Parsing DER payload can cause memory exhaustion in encoding/asn1
(GO-2025-4011)
[LOW] 1-1: stdlib 1.21.99: Lack of limit when parsing cookies can cause memory exhaustion in net/http
(GO-2025-4012)
[LOW] 1-1: stdlib 1.21.99: Panic when validating certificates with DSA public keys in crypto/x509
(GO-2025-4013)
[LOW] 1-1: stdlib 1.21.99: Unbounded allocation when parsing GNU sparse map in archive/tar
(GO-2025-4014)
[LOW] 1-1: stdlib 1.21.99: Excessive CPU consumption in Reader.ReadResponse in net/textproto
(GO-2025-4015)
🪛 Ruff (0.14.3)
scripts/lint/spec_lint.py
1-1: Shebang is present but file is not executable
(EXE001)
24-24: Avoid specifying long messages outside the exception class
(TRY003)
27-27: Avoid specifying long messages outside the exception class
(TRY003)
37-37: Do not catch blind exception: Exception
(BLE001)
38-38: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
38-38: Avoid specifying long messages outside the exception class
(TRY003)
44-44: Do not catch blind exception: Exception
(BLE001)
47-47: Multiple statements on one line (colon)
(E701)
48-48: Multiple statements on one line (colon)
(E701)
scripts/vectors/python/cbor_canon.py
1-1: Shebang is present but file is not executable
(EXE001)
5-5: Do not catch blind exception: Exception
(BLE001)
10-10: Do not catch blind exception: Exception
(BLE001)
18-18: Unnecessary mode argument
Remove mode argument
(UP015)
scripts/vectors/python/canon.py
1-1: Shebang is present but file is not executable
(EXE001)
5-5: Do not catch blind exception: Exception
(BLE001)
42-42: Unnecessary mode argument
Remove mode argument
(UP015)
🪛 Shellcheck (0.11.0)
scripts/harness/run.sh
[info] 241-241: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🪛 YAMLlint (1.37.1)
.github/workflows/vectors-matrix.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 17-17: too many spaces inside braces
(braces)
[error] 17-17: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
.github/workflows/vectors-cbor.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 24-24: too many spaces inside braces
(braces)
[error] 24-24: too many spaces inside braces
(braces)
[error] 35-35: too many blank lines (1 > 0)
(empty-lines)
🔇 Additional comments (7)
.gitignore (1)
70-71: LGTM!Clean addition of a temp artifacts section. The
tmp/pattern appropriately excludes temporary build/test artifacts as noted in the PR objectives.scripts/vectors/rust/Cargo.toml (1)
1-10: LGTM!Well-structured Cargo manifest. Dependencies (serde, serde_json, blake3) are stable, widely-used crates with appropriate version constraints. Edition 2021 is current and correct.
docs/spec/versioning.md (1)
1-43: LGTM!Comprehensive and well-structured versioning policy. The SemVer definitions are clear, v0.1.0 scope is appropriate, and migration guidance provides actionable advice for implementers. The document aligns perfectly with the release workflow additions in this PR.
docs/.vitepress/config.ts (1)
11-14: LGTM with acknowledged limitation.Footer addition is clean. The static version reference ("see GitHub tags") is acknowledged in the PR objectives as a temporary placeholder, with dynamic
SPEC_VERSIONinjection planned for a follow-up PR.Recommendation: Ensure the follow-up PR is tracked (open an issue if not already done) to inject the version from tags at build time.
Would you like me to open an issue to track the follow-up PR for dynamic version injection?
tests/vectors/core/entry_canonical.json (1)
1-8: Test vector structure is sound.The Entry structure is well-formed with appropriate fields for canonicalization testing. The timestamp being in the past (Oct 31) relative to the PR date (Nov 4) is appropriate for deterministic test vectors.
docs/spec/policy-wasm.md (1)
59-65: Compliance verification approach is sound.The double-invocation test with identical inputs/outputs correctly validates determinism. Requiring hosts to document limits and imports ensures transparency for implementers.
.github/workflows/vectors-cbor.yml (1)
21-28: Critical: Go cbor id step structure is broken.Lines 25-28 are incorrectly indented as part of the
actions/setup-go@v5action config, not as a separate run step. This workflow will fail because the Go build and execution commands won't run.Apply this diff to create a proper separate run step:
- name: Go cbor id id: go uses: actions/setup-go@v5 with: { go-version: '1.21.x' } - - run: | + + - name: Build and run Go cbor + id: go-run + run: | cd scripts/vectors/go-cbor && go build -o ../../bin_lk_cbor_go . scripts/bin_lk_cbor_go tests/vectors/core/entry_canonical.json > go_cbor_id.txt echo "id=$(cat go_cbor_id.txt)" >> $GITHUB_OUTPUTThen update line 33 to reference the correct step:
- test "${{ steps.go.outputs.id }}" = "${{ steps.py.outputs.id }}" + test "${{ steps.go-run.outputs.id }}" = "${{ steps.py.outputs.id }}"Likely an incorrect or invalid review comment.
| on: | ||
| push: | ||
| branches: [ main ] | ||
| pull_request: | ||
|
|
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fix YAML formatting for consistency.
Static analysis flags several style issues: bracket/brace spacing, truthy values, extra blank lines.
Apply these formatting fixes:
on:
push:
- branches: [ main ]
+ branches: [main]
pull_request:And at line 24:
- with: { go-version: '1.21.x' }
+ with: {go-version: '1.21.x'}And remove the trailing blank line at the end of the file.
Based on static analysis.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 5-5: too many spaces inside brackets
(brackets)
🤖 Prompt for AI Agents
In .github/workflows/vectors-cbor.yml around lines 3-7 and at line 24, the YAML
has inconsistent formatting (bracket/brace spacing), improper truthy values,
extra blank lines and a trailing blank line at EOF; fix by normalizing lists to
block style (e.g. replace inline brackets with a dash list for branches), ensure
boolean-like values are proper lowercase true/false or omitted per GitHub
Actions schema, remove unintended extra blank lines (including the trailing
blank line at end of file), and correct spacing around braces/brackets to match
repo style so the file passes static analysis.
| on: | ||
| push: | ||
| branches: [ main ] | ||
| pull_request: |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Fix YAML formatting for consistency.
Static analysis flags style issues with brackets and truthy values (same as vectors-cbor.yml).
Apply these formatting fixes:
on:
push:
- branches: [ main ]
+ branches: [main]
pull_request:And at lines 17, 40 for braces spacing.
Based on static analysis.
📝 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.
| on: | |
| push: | |
| branches: [ main ] | |
| pull_request: | |
| on: | |
| push: | |
| branches: [main] | |
| pull_request: |
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 5-5: too many spaces inside brackets
(brackets)
🤖 Prompt for AI Agents
In .github/workflows/vectors-matrix.yml around lines 3 to 6, change the inline
array and empty key style to the repository convention: replace "branches: [
main ]" with a block style "branches:" followed by "- main", and replace the
bare "pull_request:" with an explicit empty mapping "pull_request: {}". Also at
lines 17 and 40 fix brace spacing to match project style by adding a single
space after '{' and before '}' (e.g. "{ value }" not "{value}").
| func main() { | ||
| if len(os.Args) < 2 { log.Fatalf("usage: lk_canon_go entry.json") } | ||
| b, err := ioutil.ReadFile(os.Args[1]) | ||
| if err != nil { log.Fatal(err) } | ||
| var v map[string]interface{} | ||
| if err := json.Unmarshal(b, &v); err != nil { log.Fatal(err) } | ||
| can := preimage(v) | ||
| h := b3.Sum256([]byte(can)) | ||
| fmt.Printf("%x\n", h[:]) |
There was a problem hiding this comment.
Float64 decoding destroys canonical integers.
json.Unmarshal shoves every number into float64. Anything above 2^53 loses precision before you even hit the int check, so distinct ledger IDs collapse to the same canonical string. Use a decoder with UseNumber() and parse json.Number strings as integers; only then can you reject real floats and keep the integer domain intact.
| elif isinstance(obj, (int,)): | ||
| return str(obj) | ||
| elif isinstance(obj, float): | ||
| die("floats are forbidden in canonical positions; encode as string") | ||
| elif obj is None: | ||
| return 'null' | ||
| elif isinstance(obj, bool): | ||
| return 'true' if obj else 'false' |
There was a problem hiding this comment.
Booleans are being serialized as integers.
bool inherits from int, so Line 19 grabs booleans before Line 25 ever sees them. As written, True canonicalizes to "1" and False to "0", completely violating the JSON canonical form and breaking hash parity with the Rust/Go implementations. Move the boolean branch above the integer branch (or check type(obj) is bool) so that booleans emit true/false. Fix it.
🤖 Prompt for AI Agents
In scripts/vectors/python/canon.py around lines 19 to 26, booleans are being
caught by the integer branch because bool subclasses int, causing True/False to
serialize as "1"/"0"; fix it by matching booleans before integers (i.e., move
the isinstance(obj, bool) branch above the int branch) or by using a strict type
check like type(obj) is bool so booleans produce 'true'/'false' as required by
the canonical JSON rules.
| @@ -0,0 +1,29 @@ | |||
| #!/usr/bin/env python3 | |||
There was a problem hiding this comment.
Add execute permission to script.
The shebang is present but the file is not executable. Add execute permission.
Run:
chmod +x scripts/vectors/python/cbor_canon.pyBased on static analysis.
🧰 Tools
🪛 Ruff (0.14.3)
1-1: Shebang is present but file is not executable
(EXE001)
🤖 Prompt for AI Agents
In scripts/vectors/python/cbor_canon.py at lines 1-1 the file has a valid
shebang but lacks execute permission; change the file mode to make it executable
(e.g., run chmod +x scripts/vectors/python/cbor_canon.py or update the
repository file mode via git add/chmod and commit) so the script can be run
directly.
| try: | ||
| import cbor2 # pip install cbor2 | ||
| except Exception: | ||
| print("ERROR: python module 'cbor2' is required (pip install cbor2)", file=sys.stderr) | ||
| sys.exit(2) | ||
| try: | ||
| import blake3 | ||
| except Exception: | ||
| print("ERROR: python module 'blake3' is required (pip install blake3)", file=sys.stderr) | ||
| sys.exit(2) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Narrow exception handling to ImportError.
Catching bare Exception is overly broad. Use ImportError to specifically catch module import failures.
Apply this diff:
try:
import cbor2 # pip install cbor2
-except Exception:
+except ImportError:
print("ERROR: python module 'cbor2' is required (pip install cbor2)", file=sys.stderr)
sys.exit(2)
try:
import blake3
-except Exception:
+except ImportError:
print("ERROR: python module 'blake3' is required (pip install blake3)", file=sys.stderr)
sys.exit(2)Based on static analysis.
📝 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.
| try: | |
| import cbor2 # pip install cbor2 | |
| except Exception: | |
| print("ERROR: python module 'cbor2' is required (pip install cbor2)", file=sys.stderr) | |
| sys.exit(2) | |
| try: | |
| import blake3 | |
| except Exception: | |
| print("ERROR: python module 'blake3' is required (pip install blake3)", file=sys.stderr) | |
| sys.exit(2) | |
| try: | |
| import cbor2 # pip install cbor2 | |
| except ImportError: | |
| print("ERROR: python module 'cbor2' is required (pip install cbor2)", file=sys.stderr) | |
| sys.exit(2) | |
| try: | |
| import blake3 | |
| except ImportError: | |
| print("ERROR: python module 'blake3' is required (pip install blake3)", file=sys.stderr) | |
| sys.exit(2) |
🧰 Tools
🪛 Ruff (0.14.3)
5-5: Do not catch blind exception: Exception
(BLE001)
10-10: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
In scripts/vectors/python/cbor_canon.py around lines 3 to 12, the import error
handlers currently use a bare "except Exception" which is too broad; change both
exception handlers to "except ImportError" so only import failures are caught
and other exceptions propagate, leaving the rest of the error messages and
sys.exit(2) behavior unchanged.
| Value::Null => "null".to_string(), | ||
| Value::Bool(b) => if *b { "true" } else { "false" }.to_string(), | ||
| Value::Number(n) => { | ||
| if let Some(i) = n.as_i64() { i.to_string() } else { panic!("floats forbidden in canonical positions") } | ||
| } | ||
| Value::String(s) => serde_json::to_string(s).unwrap(), | ||
| } |
There was a problem hiding this comment.
You just outlawed legitimate integers.
serde_json::Number::as_i64() returns None for anything outside the signed 64-bit range, even if the source JSON held an integer that Python’s canonicalizer happily preserves. Hitting that branch now panics, so any ledger entry containing a wider integer bricks the tool. Handle u64 and i64 explicitly and reject only the true floats.
🤖 Prompt for AI Agents
In scripts/vectors/rust/src/main.rs around lines 20 to 26, the current code
panics for JSON integers outside the signed 64-bit range because it only checks
Number::as_i64(); update the branch to handle Number::as_i64() first, then
Number::as_u64() (formatting the unsigned value with to_string), and only reject
(panic) if the number is a true float (Number::as_f64() is Some) or otherwise
cannot be represented as i64/u64; this preserves large unsigned integers while
still forbidding floats in canonical positions.
|
Branch consolidation report:
Action proposed: Merge this PR (#87). Skip merging compliance/verify-proofs-skeleton to avoid destructive rollbacks. |
…, release guard/dirs/eof, compliance links/table/heading, model code fences, cargo opts, Makefile errors) [expected to fail]
…ler test to build demo
…rectory validation, enforce no trailing newline; update tests accordingly
…axonomy table with pipes, and promote '12. Future Work' to heading; tests now pass for docs checks
…ve '|| true', add explicit checks incl. artifact verification; update test
…back tests now pass
…e; link to added tests
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (2)
tests/policy/wasm/demo/Makefile (1)
9-13: Artifact verification check lacks error messaging and fails silently.Line 13's
@test -f $(OUT)will cause the target to fail if the artifact doesn't exist, but provides no user feedback. The@suppresses echoing the command, so the Make error is opaque.Improve diagnostics by adding an explicit message:
build: command -v rustup >/dev/null rustup target add $(TARGET) cargo build --release --target $(TARGET) - @test -f $(OUT) + @test -f $(OUT) || (echo "ERROR: WASM build failed to produce $(OUT)" >&2; exit 1)Alternatively, remove the
@and add messaging:- @test -f $(OUT) + test -f $(OUT) || { echo "ERROR: Build artifact missing: $(OUT)" >&2; exit 1; }tests/policy/wasm/demo/src/lib.rs (1)
3-12: Document the unsafe caller contract inline.You’re still exporting an FFI that dereferences raw pointers without spelling out the caller’s obligations. That’s unacceptable for a
no_stddemo we expect integrators to copy-paste—without the invariants written right here, they will get UB. Add the full# Safetysection describing pointer validity, alignment, buffer capacity, and non-overlap just like we already asked in the previous review.// - This demo accepts iff entry_len is even AND state_len is even; otherwise rejects. // - Purely deterministic: no clock, randomness, or host I/O. +// +// # Safety +// - entry_ptr must be null if entry_len == 0, otherwise valid for reads of entry_len bytes. +// - state_ptr must be null if state_len == 0, otherwise valid for reads of state_len bytes. +// - out_len_ptr must be either null (and then out_ptr must also be null) or valid for read/write of a usize. +// - If out_ptr is non-null, it must be valid for writes of *out_len_ptr bytes and properly aligned for u8. +// - The slice [out_ptr, out_ptr + *out_len_ptr) must not alias the input slices in a way that violates Rust aliasing rules. +// - The caller must ensure the pointed-to memory stays allocated and accessible for the duration of the call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
FEEDBACK.mdis excluded by!*.md
📒 Files selected for processing (17)
.github/workflows/release.yml(1 hunks).gitignore(1 hunks)docs/compliance/index.md(3 hunks)scripts/tests/feedback/01_wasm_panic_handler.sh(1 hunks)scripts/tests/feedback/02_release_tag_guard.sh(1 hunks)scripts/tests/feedback/03_release_dirs_validation.sh(1 hunks)scripts/tests/feedback/04_release_eof.sh(1 hunks)scripts/tests/feedback/05_docs_compliance_links.sh(1 hunks)scripts/tests/feedback/06_docs_error_table_tabs.sh(1 hunks)scripts/tests/feedback/07_docs_heading_future.sh(1 hunks)scripts/tests/feedback/08_model_source_codefence_lang.sh(1 hunks)scripts/tests/feedback/09_wasm_cargo_optimizations.sh(1 hunks)scripts/tests/feedback/10_wasm_makefile_errors.sh(1 hunks)scripts/tests/run_feedback_tests.sh(1 hunks)tests/policy/wasm/demo/Cargo.toml(1 hunks)tests/policy/wasm/demo/Makefile(1 hunks)tests/policy/wasm/demo/src/lib.rs(1 hunks)
🧰 Additional context used
🪛 checkmake (0.2.2)
tests/policy/wasm/demo/Makefile
[warning] 5-5: Missing required phony target "test"
(minphony)
🔇 Additional comments (19)
scripts/tests/feedback/08_model_source_codefence_lang.sh (1)
1-8: ✓ Correct enforcement of fenced code block language specification.The use of
grep -xF '```'correctly identifies lines containing only backticks (no language specifier). The strict exit on discovery prevents code-fence drift in the model source.scripts/tests/feedback/10_wasm_makefile_errors.sh (1)
1-9: ✓ Makefile error-suppression and artifact-check detection is sound.Line 4 correctly identifies silent error suppression (
|| true), and line 9 validates the presence of artifact verification (test -f $(OUT)). Both escape sequences are correct.docs/compliance/index.md (5)
14-14: ✓ Link references updated to internal paths.Links now correctly use
/spec/and/spec/modelinstead of broken relative paths./SPEC.mdand./MODEL.md. This resolves the prior navigation breakage.
99-99: ✓ Proof artifacts table extended appropriately.Addition of
policy.proof.jsonentry aligns with expanded policy testing scope and maintains table structure consistency.
126-133: ✓ Error taxonomy table properly formatted.All rows now use correct pipe-delimited columns with spaces (no tabs), fixing prior MD010 and MD056 violations. Three columns as expected: Code | Name | Meaning.
141-146: ✓ Compliance Scoring table properly formatted.Uniform pipe-delimited structure with consistent spacing across Level and Requirements columns.
204-204: ✓ "Future Work" section now a proper heading.Changed from bare list item
12. Future Workto markdown heading## 12. Future Work, resolving prior MD029 violation (ordered list prefix mismatch).scripts/tests/feedback/06_docs_error_table_tabs.sh (1)
1-7: ✓ Tab detection enforces markdown linting compliance.The script correctly uses
rg -n "\t"to catch literal tabs in compliance documentation, preventing regression of prior MD010 violations.tests/policy/wasm/demo/Cargo.toml (1)
1-15: ✓ Cargo.toml structure and WASM profile settings are sound.Proper TOML formatting with cdylib crate-type and release profile optimizations (opt-level, lto, strip, panic) match intended WASM size optimization goals. Edition 2021 is current.
However, note that the test script
scripts/tests/feedback/09_wasm_cargo_optimizations.shuses line-anchored regex patterns (^stripand^panic) that will not match these indented settings. Please verify the regex patterns in that script are updated to allow leading whitespace, or this Cargo.toml will fail that test.scripts/tests/feedback/05_docs_compliance_links.sh (1)
1-7: ✓ Broken link detection prevents regression of fixed documentation links.The regex
\./SPEC\.md|\./MODEL\.mdcorrectly identifies the old relative paths that were migrated to internal navigation. Proper escape sequences for literal dots and pipe alternation.scripts/tests/feedback/07_docs_heading_future.sh (1)
1-7: Well-constructed validation script. Pattern anchoring (^and$) is correct, and error messaging flows to stderr as expected. ✓scripts/tests/feedback/01_wasm_panic_handler.sh (1)
1-10: Panic handler validation and build check are sound. The pattern is properly anchored (^), and build output suppression preserves error propagation (failures still exit non-zero). ✓scripts/tests/feedback/04_release_eof.sh (1)
1-11: EOF validation is bulletproof. File existence check (line 5) prevents spurious errors. Theodoutput format comparison with leading space (line 8) correctly matches newline character code (10). Logic is sound: succeed if no trailing newline, fail otherwise. ✓scripts/tests/feedback/03_release_dirs_validation.sh (1)
1-5: Straightforward directory validation checks. Both rg patterns are simple and correct: line 4 ensures the directory-iteration loop is present, line 5 ensures the ERROR message pattern exists. ✓.github/workflows/release.yml (2)
21-32: Directory validation implementation is solid (improvements from prior feedback).The defensive loop (lines 24–28) now explicitly validates that required directories exist before attempting zip operations. Error messages are clear and redirect to stderr. The pattern incorporates all prior review feedback on this section. ✓
13-13: Guard condition is appropriately restrictive.The
if:condition properly gates the job to only run when the ref is a tag starting withv, preventing accidental releases from stray branches or non-tag pushes. ✓scripts/tests/run_feedback_tests.sh (3)
1-10: Test harness pattern is sound; generation via heredocs is valid.The design accumulates test results so all tests run even if one fails, then exits non-zero if any failed—this is the correct behavior for comprehensive feedback. The
run()function on line 4 is terse but syntactically valid bash. ✓
7-131: Spot-check: Generated test scripts appear consistent with standalone files.Sampled verification of generated scripts against provided files:
- T1 (lines 7–18) vs. 01_wasm_panic_handler.sh: Content matches. ✓
- T4 (lines 45–57) vs. 04_release_eof.sh: Content matches. ✓
- T7 (lines 85–93) vs. 07_docs_heading_future.sh: Content matches. ✓
All generated scripts are made executable (
chmod +x) immediately after creation. Harness execution model (lines 134–143) runs each test viabash -e, capturing non-zero exits as failures. Pattern is consistent and correct.Please confirm that all 10 generated scripts (T1–T10) are functionally identical to their standalone counterparts. If you use this harness in CI, ensure it's invoked so that no discrepancies can creep in between the generated and standalone versions.
134-148: Final result accumulation and exit logic is correct.The
|| trueon each test invocation (lines 134–143) is intentional and correct—it allows all tests to execute, then line 145–147 checks if any failed and exits with status 1. This ensures comprehensive feedback rather than short-circuiting on the first failure. ✓
| on: | ||
| push: | ||
| tags: | ||
| - 'v[0-9]+.[0-9]+.[0-9]+' |
There was a problem hiding this comment.
🧩 Analysis chain
Verify GitHub Actions glob pattern for semantic version tags.
Line 6's pattern - 'v[0-9]+.[0-9]+.[0-9]+' uses glob syntax (not regex). In GitHub Actions glob patterns, [0-9] is a character class (matches single digit), but + is literal (not a quantifier as in regex). This means the pattern matches v0.0.+ literally, not v1.2.3.
For semantic versioning (v1.2.3), the correct glob pattern should be:
- 'v[0-9]*.[0-9]*.[0-9]*'(wildcard for any digit sequence), or- 'v[0-9][0-9]*.[0-9][0-9]*.[0-9][0-9]*'(if stricter), or- Use regex syntax if GitHub Actions supports it in this context.
Verify the tag pattern behavior:
Or test locally by creating a tag matching your pattern and verifying the workflow triggers.
🌐 Web query:
GitHub Actions workflow tag filtering glob patterns semantic version
💡 Result:
Short answer: GitHub Actions uses glob patterns (not regular expressions) to match tag names in on.push.tags / tags-ignore, so you cannot strictly express full semver with regex quantifiers (like +). Use globs to approximate semver and, if you need exact semver validation, combine a permissive tag-glob with an in-workflow regex check.
Key points and examples
- Docs: tags use shell-style globbing (*, **, ?, [chars], ! for negation). Exact doc reference. [1]
- Globs you can use:
- Match any v-prefixed semver-ish tag: tags: [ 'v*' ] — matches v1, v1.2.3, v1.2.3-alpha, etc. [1]
- Match major only or major.minor: tags: [ 'v1.' ] or [ 'v1..*' ] — approximates numeric components but does not enforce digits. [1]
- Use character classes to require digits at a specific position (single digit only): tags: [ 'v[0-9].[0-9].[0-9]' ] — but this only matches single digits and cannot express "one or more" digits. [1][2]
- What you cannot do: express regex-like quantifiers (e.g., + for one-or-more digits) or full semver rules inside the tags glob — globs lack that expressiveness. Many people mistakenly try regex (e.g., ^v[0-9]+.[0-9]+.[0-9]+) and it won't work. [2][3]
Recommended pattern (practical)
- Trigger on a permissive glob, then validate inside the job:
- on:
push:
tags: [ 'v*' ] - In the first job step, check github.ref (or github.ref_name) with a proper regex (bash, node, or an action). If it doesn't match your semver regex (for example ^v[0-9]+.[0-9]+.[0-9]+(-[0-9A-Za-z.-]+)?$), exit/skip the rest. This gives exact semver enforcement while keeping the trigger reliable. [1][2][3]
- on:
References
- GitHub Actions workflow syntax (tags/globs). [1]
- Stack Overflow examples & explanation that tags use globs (not regex) and common pitfalls. [2][3]
Fix the tag glob pattern—it will not trigger on semantic version releases.
GitHub Actions uses shell-style globbing (not regex) for tags, so quantifiers like + are treated as literal characters, not as "one or more" operators. The pattern 'v[0-9]+.[0-9]+.[0-9]+' is fundamentally broken: it won't match v1.2.3 at all; it will only match tags containing literal + characters.
Use a permissive glob like v* and validate the exact semver format with a regex check inside the workflow. Alternatively, if a stricter glob is preferred: 'v[0-9]*.[0-9]*.[0-9]*' (though this still lacks strict digit-only enforcement).
Current line 6:
- 'v[0-9]+.[0-9]+.[0-9]+'Change to:
- 'v*'Then add a validation step early in your job to enforce exact semver matching.
🤖 Prompt for AI Agents
.github/workflows/release.yml lines 3-6: the tag pattern uses regex-like
quantifiers so it won't match semantic version tags (e.g., v1.2.3); replace the
trigger with a permissive glob such as 'v*' and add an early workflow step in
the job to validate the tag against a proper semver regex, failing the job if it
does not match (alternatively use a stricter glob like 'v[0-9]*.[0-9]*.[0-9]*'
if you want some bounding but still perform exact validation in the workflow).
| # Temp artifacts | ||
| tmp/ | ||
|
|
||
| FEEDBACK.md |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Trailing blank line after new section.
Lines 72-73 contain a blank line after FEEDBACK.md. While not technically harmful, this introduces a trailing blank line that diverges from the structure of prior sections (e.g., lines 59-60 after fastlane section). For consistency and minimal git diffs, remove the extra blank line.
~# Temp artifacts
~tmp/
~
~FEEDBACK.md
+# Temp artifacts
+tmp/
+
+FEEDBACK.mdCommittable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In .gitignore around lines 70 to 73, there's an extra trailing blank line after
the new "FEEDBACK.md" entry; remove the blank line so the section ends
immediately after "FEEDBACK.md" to match the formatting of prior sections and
minimize diffs.
| rg -n "^\s*tags:\s*$" "$f" >/dev/null | ||
| grep -nF -- "- 'v[0-9]+.[0-9]+.[0-9]+'" "$f" >/dev/null | ||
| # Check we guard by ref_type/startsWith | ||
| rg -n "^\s*if:\s*github.ref_type == 'tag'.*startsWith\(github.ref, 'refs/tags/v'\)" "$f" >/dev/null |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
Test pattern assumptions are fragile; verify exact match against release.yml.
Line 6 uses grep -nF (literal grep) to match the exact string - 'v[0-9]+.[0-9]+.[0-9]+' including bracket syntax. This works only if the YAML has exactly this format; any variation (spacing, quoting, escape differences) will cause a false negative.
Line 8's rg pattern assumes the if: condition is on a single line; if YAML wraps or reformats, the pattern may fail.
Verify these patterns match your actual release.yml using:
If either fails, adjust the test patterns to use more flexible matchers (e.g., looser whitespace handling, or split checks).
🏁 Script executed:
#!/bin/bash
# Verify line 6 pattern matches
grep -F -- "- 'v[0-9]+.[0-9]+.[0-9]+'" .github/workflows/release.yml
echo "---"
# Verify line 8 pattern matches
rg "^\s*if:\s*github.ref_type == 'tag'.*startsWith\(github.ref, 'refs/tags/v'\)" .github/workflows/release.ymlLength of output: 109
Patterns currently work; test remains unnecessarily brittle.
Both patterns match the actual release.yml file, so the specific concerns about mismatches are unfounded for the current state. However, the underlying fragility is real:
- Line 6's
grep -Ftreats[0-9]as literal characters, meaning it's actually validating the exact marker string (not a regex), which works but offers zero flexibility. - Line 8's rg pattern assumes the
if:condition stays on a single line; any legitimate YAML reformatting breaks the test.
These patterns work now, but they're brittle dependencies on exact formatting. A more maintainable approach would use a YAML parser or significantly more flexible matchers (whitespace tolerance, multi-line support).
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
| f=tests/policy/wasm/demo/Cargo.toml | ||
| rg -n "^strip\s*=\s*true" "$f" >/dev/null | ||
| rg -n "^panic\s*=\s*\"abort\"" "$f" >/dev/null |
There was a problem hiding this comment.
🔴 CRITICAL: Regex anchors will fail to match indented TOML keys.
The patterns ^strip\s*=\s*true and ^panic\s*=\s*\"abort\" anchor to column 0 (start of line). However, in TOML files under [profile.release], these keys are indented with whitespace. An indented strip = true will NOT match ^strip.
This test will incorrectly fail even when the Cargo.toml contains valid settings.
Replace the line-anchor ^ with a pattern that permits leading whitespace:
-rg -n "^strip\s*=\s*true" "$f" >/dev/null
-rg -n "^panic\s*=\s*\"abort\"" "$f" >/dev/null
+rg -n "strip\s*=\s*true" "$f" >/dev/null
+rg -n "panic\s*=\s*\"abort\"" "$f" >/dev/nullAlternatively, to be more strict and avoid false positives in comments, use:
-rg -n "^strip\s*=\s*true" "$f" >/dev/null
-rg -n "^panic\s*=\s*\"abort\"" "$f" >/dev/null
+rg -n "^\s*strip\s*=\s*true" "$f" >/dev/null
+rg -n "^\s*panic\s*=\s*\"abort\"" "$f" >/dev/nullThe latter preserves the anchor and explicitly allows optional leading whitespace (\s*).
🤖 Prompt for AI Agents
In scripts/tests/feedback/09_wasm_cargo_optimizations.sh around lines 1 to 5,
the two rg patterns anchor to column 0 and will miss keys indented under TOML
sections; change the regexes to allow optional leading whitespace so indented
keys (e.g. " strip = true") match — for example use patterns that start with
optional whitespace before the key (and keep the rest of the spacing checks),
and ensure you preserve quoting for the panic value; update both rg invocations
accordingly so the test passes for indented TOML keys while still avoiding
matching commented lines.
| NAME=policy_wasm_demo | ||
| OUT=target/$(TARGET)/release/$(NAME).wasm | ||
|
|
||
| .PHONY: all build clean |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Add missing .PHONY: test target (static analysis).
The static analysis tool checkmake(1) flags that the Makefile is missing a test target in .PHONY. While not required for this Makefile's current functionality, it's a convention to declare it even if unused, or to add a dummy target:
-.PHONY: all build clean
+.PHONY: all build clean test
+
+test:
+ @echo "No tests defined for WASM demo" >&2Alternatively, if test should actually run the built artifact, implement that instead.
📝 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.
| .PHONY: all build clean | |
| .PHONY: all build clean test | |
| test: | |
| @echo "No tests defined for WASM demo" >&2 |
🧰 Tools
🪛 checkmake (0.2.2)
[warning] 5-5: Missing required phony target "test"
(minphony)
🤖 Prompt for AI Agents
In tests/policy/wasm/demo/Makefile around line 5, add a `.PHONY: test`
declaration (or implement a real `test` target) to satisfy checkmake static
analysis: update the `.PHONY` line to include `test` (or add a separate `.PHONY:
test` line) and either leave `test` as a no-op/stub target that returns success
or implement it to run the built artifact (e.g., execute the binary or run a
selected test command).
…wasm README/code fences & Safety docs; vectors C compile step; YAML style; implementers schema link; wire-format blank line; config per-check timeout; harness order & clauses
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
docs/spec/wire-format.md (4)
62-62: Add a blank line for better spacing.The existing text is dense, and the paragraph should be easier to read with an additional line break.
- Strings MUST NOT contain unpaired surrogates; escaping MUST be minimal (e.g., `"`, `\\`, `\uXXXX` where necessary). + Canonicalization Function (pseudocode)
31-32: Add explanation why BLAKE3 was chosen.The existing text is missing context about BLAKE3, which would be beneficial to readers.
-Primary content hash: **BLAKE3‑256** over the canonical Entry preimage (see below). The digest SHALL be encoded as lowercase hex (64 hex chars). +Primary content hash: **BLAKE3‑256**[^BLAKE3] over the canonical Entry preimage (see below). The digest SHALL be encoded as lowercase hex (64 hex chars). + +[^BLAKE3]: Link to BLAKE3 docs.
62-62: Add a blank line for better spacing.The existing text is dense, and the paragraph should be easier to read with an additional line break.
- Strings MUST NOT contain unpaired surrogates; escaping MUST be minimal (e.g., `"`, `\\`, `\uXXXX` where necessary). + Canonicalization Function (pseudocode)
31-32: Add explanation why BLAKE3 was chosen.The existing text is missing context about BLAKE3, which would be beneficial to readers.
-Primary content hash: **BLAKE3‑256** over the canonical Entry preimage (see below). The digest SHALL be encoded as lowercase hex (64 hex chars). +Primary content hash: **BLAKE3‑256**[^BLAKE3] over the canonical Entry preimage (see below). The digest SHALL be encoded as lowercase hex (64 hex chars). + +[^BLAKE3]: Link to BLAKE3 docs.
♻️ Duplicate comments (1)
.github/workflows/vectors-matrix.yml (1)
3-6: ** YAML formatting still unaddressed from past review.**YAMLlint flags continue to report spacing violations in brackets (line 5) and braces (lines 17, 40). Per the previous review, apply:
on: push: - branches: [ main ] + branches: [main] pull_request:And:
- name: Set up Python uses: actions/setup-python@v5 - with: { python-version: '3.x' } + with: { python-version: '3.x' }(similar for line 40 with Go version)
These style fixes should be applied to pass linting.
Also applies to: 17-17, 40-40
📜 Review details
Configuration used: CodeRabbit UI
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (20)
.github/workflows/vectors-matrix.yml(1 hunks)docs/implementation/implementers.md(5 hunks)docs/spec/policy-wasm.md(1 hunks)docs/spec/wire-format.md(1 hunks)scripts/harness/README.md(1 hunks)scripts/harness/config.sample.toml(1 hunks)scripts/harness/run.sh(1 hunks)scripts/tests/feedback/11_wasm_readme_codefences.sh(1 hunks)scripts/tests/feedback/12_wasm_safety_doc.sh(1 hunks)scripts/tests/feedback/13_ci_yaml_style.sh(1 hunks)scripts/tests/feedback/14_ci_c_compile_fix.sh(1 hunks)scripts/tests/feedback/15_implementers_schema_link.sh(1 hunks)scripts/tests/feedback/16_policy_wasm_bullets.sh(1 hunks)scripts/tests/feedback/17_wire_format_blankline.sh(1 hunks)scripts/tests/feedback/18_config_timeout_override.sh(1 hunks)scripts/tests/feedback/19_harness_readme_fences.sh(1 hunks)scripts/tests/feedback/20_harness_order_iteration.sh(1 hunks)scripts/tests/feedback/21_harness_clause_default.sh(1 hunks)tests/policy/wasm/demo/README.md(1 hunks)tests/policy/wasm/demo/src/lib.rs(1 hunks)
🧰 Additional context used
🪛 actionlint (1.7.8)
.github/workflows/vectors-matrix.yml
22-22: shellcheck reported issue in this script: SC2086:info:2:31: Double quote to prevent globbing and word splitting
(shellcheck)
34-34: shellcheck reported issue in this script: SC2086:info:2:31: Double quote to prevent globbing and word splitting
(shellcheck)
46-46: shellcheck reported issue in this script: SC2086:info:2:31: Double quote to prevent globbing and word splitting
(shellcheck)
🪛 markdownlint-cli2 (0.18.1)
scripts/harness/README.md
39-39: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
docs/implementation/implementers.md
34-34: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
🪛 Shellcheck (0.11.0)
scripts/harness/run.sh
[info] 249-249: Note that A && B || C is not if-then-else. C may run when A is true.
(SC2015)
🪛 YAMLlint (1.37.1)
.github/workflows/vectors-matrix.yml
[warning] 3-3: truthy value should be one of [false, true]
(truthy)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 5-5: too many spaces inside brackets
(brackets)
[error] 17-17: too many spaces inside braces
(braces)
[error] 17-17: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
[error] 40-40: too many spaces inside braces
(braces)
🔇 Additional comments (17)
scripts/tests/feedback/17_wire_format_blankline.sh (1)
1-9: LGTM!The script is clear and concise. It looks good.
scripts/tests/feedback/15_implementers_schema_link.sh (1)
1-4: Regex pattern is permissive; verify intentionality.The pattern
https://github.com/.*/ledger-kernel/.*/schemas/compliance_report.schema.jsonuses.*for both the GitHub account and the path component (between/ledger-kernel/and/schemas). This will match:
- Any GitHub org/user with a repo named
ledger-kernel(not justflyingrobots/ledger-kernel)- Any path after
/ledger-kernel/(e.g.,blob/main,blob/develop,tree/v1.0/)If this test is meant to enforce a link to the official ledger-kernel repo, the pattern should be more specific:
rg -F "https://github.com/flyingrobots/ledger-kernel/blob/main/schemas/compliance_report.schema.json" "$f" >/dev/null(Note:
-Ftreats the string as literal, avoiding regex escaping headaches.)Clarify whether the breadth is intentional for flexibility or an oversight, and update if stricter verification is desired.
docs/implementation/implementers.md (2)
19-21: Schema link fix resolves previous 404 issue—approved.The previous review flagged a broken relative link (
./schemas/compliance_report.schema.json) that resolved to a non-existent path. This change correctly replaces it with an absolute GitHub URL:https://github.com/flyingrobots/ledger-kernel/blob/main/schemas/compliance_report.schema.json.This link now points to the correct schema and is reachable. ✓
1-124: No issues found—schema links comply with test expectations.Both lines 21 and 66 contain matching full GitHub URLs that pass the test pattern validation. The
scripts/tests/feedback/15_implementers_schema_link.shtest confirms the pattern is present in the file and will pass.scripts/tests/feedback/14_ci_c_compile_fix.sh (1)
1-5: ✓ LGTM — Solid verification for C compilation.The script correctly validates the gcc compile command in the workflow, addressing the critical compilation issue flagged in the past review. Pattern matching is precise and uses strict bash mode appropriately.
.github/workflows/vectors-matrix.yml (1)
58-68: ✓ Critical C compilation issue from past review is now fixed.The C verification step now correctly:
- Compiles the C tool first (line 65):
gcc -O2 -lblake3 -o c_b3sum scripts/vectors/c/blake3_id.c- Pipes canonical bytes into the compiled binary (line 66):
./c_b3sum < can.txt > c_id.txt- Compares the hash against Python output (line 68)
This aligns with the validation in
scripts/tests/feedback/14_ci_c_compile_fix.sh, which verifies the exact compile pattern. The fix resolves the stdin-compilation flaw from the previous review.scripts/tests/feedback/11_wasm_readme_codefences.sh (1)
1-6: LGTM!Simple, effective validation for WASM demo README code fence requirements. Pattern checks are correct; both fences will cause script to exit 1 if missing.
scripts/tests/feedback/19_harness_readme_fences.sh (1)
1-14: LGTM!Correctly validates that the harness README has at least one fenced code block with a language specifier. Logic is sound, though you could simplify to a single
rg -qcall instead of the while loop (optional).scripts/tests/feedback/21_harness_clause_default.sh (1)
1-10: LGTM!Perfectly validates the fixes from prior review comments. Confirms run.sh uses empty default for clauses and includes the empty-guard check. Escaping with
--is correct.scripts/tests/feedback/18_config_timeout_override.sh (1)
1-5: LGTM!Validates the sample config includes per-check timeout override at [checks.C-4], addressing the prior review comment requesting this demonstration. Patterns are correct.
scripts/harness/config.sample.toml (1)
23-27: Excellent. Per‑check timeout override demonstrated.Line 26 adds
timeout_sec = 10to C-4, properly addressing the prior review comment. The value is distinct from the globaltimeout_sec = 20, clearly demonstrating override semantics.scripts/harness/README.md (1)
1-48: LGTM!Clear, well‑structured documentation. Blank lines around code fences are properly placed. Usage and config examples are concrete and practical. The static analysis flag about missing language specifier on line 40 is a false positive (that section is a list, not a code fence).
scripts/tests/feedback/20_harness_order_iteration.sh (1)
1-11: LGTM!Correctly validates that run.sh uses ordered iteration via
check_order[@]array rather than unordered associative array keys. The guard at line 5 will fail the build if the old pattern is reintroduced. Addresses the prior major review about deterministic ordering.scripts/harness/run.sh (3)
65-109: Config parsing: Order preservation works correctly, but duplicate-section defense is unnecessary.Lines 65–109 implement TOML parsing with order tracking via the
check_orderarray. The logic at lines 71–73 checks for duplicates:if [[ " ${check_order[*]} " != *" $cid "* ]]; then check_order+=("$cid") fiThis is defensive, but in a single-pass parse of a well-formed config file, duplicates are theoretically impossible unless the TOML has duplicate section headers (e.g., two
[checks.C-1]sections). If that's a concern, validate the config schema beforehand. Current approach is safe but slightly redundant.Optional improvement: Add explicit config validation or document the expected behavior if duplicate sections are present.
Does the project expect to validate TOML schema format separately, or should this script detect and reject malformed configs (e.g., duplicate sections)?
176-181: LGTM: Ordered iteration and correct default handling.Lines 176–181 correctly iterate checks in declared order and use the empty-string default for clauses:
for key in "${check_order[@]}"; do lvl="${check_level[$key]:-core}" clauses="${check_clause[$key]:-}" to="${check_timeout[$key]:-}" run_one "$key" "$lvl" "${check_cmd[$key]}" "$clauses" "$to" doneThis addresses prior review feedback: uses
check_orderfor deterministic ordering and empty default (:-}with no value) for clauses, ensuringappend_resultreceives an empty string (which produces[]JSON, not["-"]). Default for level ("core") and timeout ("") are sensible.
130-148: LGTM: Result aggregation logic is sound.The
append_resultfunction correctly handles JSON construction:
- Converts CSV clauses to a JSON array (lines 137–141)
- Handles empty clauses by producing
[](line 138)- Omits the
notesfield if empty (line 146)- Uses jq with proper argument binding to avoid injection
scripts/tests/feedback/12_wasm_safety_doc.sh (1)
1-4: Flawless validation script. Pattern matching is precise and failure mode is correct.The script enforces the presence of a
/// Safetydoc block in the FFI export. The pattern^/// Safetyanchors to the start of the line, avoiding false positives from inline mentions. Strict bash flags (-euo pipefail) ensure any failure propagates immediately. Output suppression via/dev/nullis appropriate for a pass/fail test.
| run: python -m pip install --upgrade pip blake3 | ||
| - name: Python compute id | ||
| id: py | ||
| run: | |
There was a problem hiding this comment.
Unquoted variable expansions: Quote $GITHUB_OUTPUT to satisfy ShellCheck SC2086.
Lines 22, 34, 46 expand $GITHUB_OUTPUT without quotes. Although unlikely in GitHub Actions (where the path is controlled and safe), ShellCheck SC2086 flags this as a best practice violation. Apply quotes defensively:
- echo "id=$(cat py_id.txt)" >> $GITHUB_OUTPUT
+ echo "id=$(cat py_id.txt)" >> "$GITHUB_OUTPUT"Apply the same fix on lines 34 and 46.
Also applies to: 34-34, 46-46
🧰 Tools
🪛 actionlint (1.7.8)
22-22: shellcheck reported issue in this script: SC2086:info:2:31: Double quote to prevent globbing and word splitting
(shellcheck)
🤖 Prompt for AI Agents
.github/workflows/vectors-matrix.yml lines 22, 34, 46: ShellCheck SC2086 flags
unquoted expansions of $GITHUB_OUTPUT; update each occurrence to quote the
variable (use "$GITHUB_OUTPUT" wherever currently using $GITHUB_OUTPUT) to
prevent word-splitting/expansion issues, and apply the same change to all three
lines.
| - name: C (blake3) check over canonical bytes | ||
| run: | | ||
| sudo apt-get update && sudo apt-get install -y libblake3-dev | ||
| echo -n "${{ steps.py.outputs.id }}" > py_id_hex | ||
| # Compute canonical bytes via Python tool | ||
| python scripts/vectors/python/canon.py tests/vectors/core/entry_canonical.json > can.txt | ||
| # Compile the C tool and pipe canonical bytes into it | ||
| gcc -O2 -lblake3 -o c_b3sum scripts/vectors/c/blake3_id.c | ||
| ./c_b3sum < can.txt > c_id.txt | ||
| echo "C: $(cat c_id.txt)" | ||
| test "$(cat c_id.txt)" = "${{ steps.py.outputs.id }}" |
There was a problem hiding this comment.
Dead variable: Remove unused py_id_hex assignment (line 61).
Line 61 writes the Python ID hexadecimal output to a file but never references py_id_hex afterward:
echo -n "${{ steps.py.outputs.id }}" > py_id_hexThe file is created and immediately discarded. Remove this line or justify its use if it serves a debugging purpose.
🤖 Prompt for AI Agents
In .github/workflows/vectors-matrix.yml around lines 58 to 68 the workflow
writes the step output to a temporary file with `echo -n "${{
steps.py.outputs.id }}" > py_id_hex` but that file is never used; remove that
line to eliminate the dead variable, or if it was intended for debugging,
document its purpose and reference the file later (or gate it behind a
conditional/step that only runs in debug mode).
| git submodule update --init --recursive | ||
| ``` | ||
|
|
||
| 2) In CI and locally, reference schemas and docs from `external/ledger-kernel/`. |
There was a problem hiding this comment.
Fix MD029 list numbering violation: use 1) instead of 2).
The markdown linter (markdownlint-cli2) reports a violation: ordered list items must use prefix 1 (auto-numbered style 1/1/1). When a code block interrupts an ordered list (lines 27–32), the list context resets. Line 34 must restart numbering as 1), not continue as 2).
Apply this diff:
-2) In CI and locally, reference schemas and docs from `external/ledger-kernel/`.
+1) In CI and locally, reference schemas and docs from `external/ledger-kernel/`.(Markdown will auto-render both items numerically regardless of the source numbering.)
📝 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.
| 2) In CI and locally, reference schemas and docs from `external/ledger-kernel/`. | |
| 1) In CI and locally, reference schemas and docs from `external/ledger-kernel/`. |
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
34-34: Ordered list item prefix
Expected: 1; Actual: 2; Style: 1/1/1
(MD029, ol-prefix)
🤖 Prompt for AI Agents
In docs/implementation/implementers.md around line 34, the ordered list item
incorrectly uses "2)" after a code block broke the list context; change the list
item prefix to "1)" so the file conforms to MD029 (use auto-numbered "1)" style)
and re-run the linter to confirm the violation is resolved.
|
|
||
| # Deterministic WASM Policy ABI (FS‑9) | ||
|
|
||
| This document specifies a minimal, deterministic host ABI for policy evaluation. It complements FS‑9 and the Compliance Policy/WASM level. |
There was a problem hiding this comment.
Self-referential wording: the document IS FS‑9, it doesn't "complement" itself.
Line 7 states "It complements FS‑9 and the Compliance Policy/WASM level", but the document title (line 5) declares this document to be "FS‑9". A document cannot complement itself.
Choose one:
-This document specifies a minimal, deterministic host ABI for policy evaluation. It complements FS‑9 and the Compliance Policy/WASM level.
+This document specifies a minimal, deterministic host ABI for policy evaluation, defining FS‑9 and the Compliance Policy/WASM level.or
-This document specifies a minimal, deterministic host ABI for policy evaluation. It complements FS‑9 and the Compliance Policy/WASM level.
+This document specifies the FS‑9 deterministic host ABI for policy evaluation at the Compliance Policy/WASM level.📝 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.
| This document specifies a minimal, deterministic host ABI for policy evaluation. It complements FS‑9 and the Compliance Policy/WASM level. | |
| This document specifies a minimal, deterministic host ABI for policy evaluation, defining FS‑9 and the Compliance Policy/WASM level. |
| This document specifies a minimal, deterministic host ABI for policy evaluation. It complements FS‑9 and the Compliance Policy/WASM level. | |
| This document specifies the FS‑9 deterministic host ABI for policy evaluation at the Compliance Policy/WASM level. |
🤖 Prompt for AI Agents
In docs/spec/policy-wasm.md around line 7, the sentence "It complements FS‑9 and
the Compliance Policy/WASM level" is self‑referential because this document is
titled FS‑9; fix by making the statement consistent with the title: either
remove the "complements FS‑9" fragment and say it "complements the Compliance
Policy/WASM level" or change the title/reference so the document is no longer
called FS‑9 and then keep the original wording; ensure the title and this line
agree and update whichever one you choose so the document does not claim to
complement itself.
|
|
||
| Implementations MAY opt into a CBOR Canonical Encoding profile (RFC 8949 §4.2) for the id preimage. In this profile: |
There was a problem hiding this comment.
Add explanation why CBOR offers benefits.
The existing text should mention the advantages of CBOR.
- Implementations MAY opt into a CBOR Canonical Encoding profile (RFC 8949 §4.2) for the id preimage. In this profile:
+ Implementations MAY opt into a CBOR Canonical Encoding profile (RFC 8949 §4.2) for the id preimage. This improves performance when processing binary data, so it is recommended when performance is paramount. However, this does create differences to JSON and so it must not be used for JSON data.Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/spec/wire-format.md around lines 160–161, the note about opting into
CBOR Canonical Encoding should be expanded to explain why CBOR is beneficial:
add a short sentence or two after the existing line describing CBOR's advantages
(compact binary representation, efficient encoding for nested/heterogeneous
data, and widespread library support), and call out that the canonical profile
gives deterministic, reproducible byte sequences (important for id preimage
hashing/signature stability) and refer to RFC 8949 for details; keep the wording
concise and include these concrete benefits so implementers understand the
motivation.
Add explanation why CBOR offers benefits.
The existing text should mention the advantages of CBOR.
- Implementations MAY opt into a CBOR Canonical Encoding profile (RFC 8949 §4.2) for the id preimage. In this profile:
+ Implementations MAY opt into a CBOR Canonical Encoding profile (RFC 8949 §4.2) for the id preimage. This improves performance when processing binary data, so it is recommended when performance is paramount. However, this does create differences to JSON and so it must not be used for JSON data.Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In docs/spec/wire-format.md around lines 160 to 161, the sentence mentioning
that implementations may opt into a CBOR Canonical Encoding profile should be
expanded to briefly state why CBOR is beneficial: add a short explanation that
CBOR offers compact binary representation (smaller payloads), deterministic
canonical encoding (stable id preimages across implementations), efficient
parsing for constrained environments, and wide existing tooling/standards
support; insert this as one or two concise sentences immediately after the
existing line so the rationale is clear to readers.
| if command -v python3 >/dev/null 2>&1; then | ||
| python3 - <<'PY' "$SCHEMA" "$OUT_JSON" && exit 0 || exit 1 | ||
| import json, sys | ||
| try: | ||
| import jsonschema | ||
| except Exception: | ||
| sys.exit(1) | ||
| with open(sys.argv[1]) as s: schema=json.load(s) | ||
| with open(sys.argv[2]) as f: data=json.load(f) | ||
| jsonschema.validate(data, schema) | ||
| print("Schema validation: OK") | ||
| PY | ||
| validate_ok=$? |
There was a problem hiding this comment.
CRITICAL: Validation logic causes script to exit prematurely, skipping level‑based verdict check.
Lines 233–245 have a fatal control flow bug:
if command -v python3 >/dev/null 2>&1; then
python3 - <<'PY' "$SCHEMA" "$OUT_JSON" && exit 0 || exit 1
validate_ok=$?
fiWhen python3 exists, the && exit 0 || exit 1 construct causes the script to exit immediately (line 234), preventing line 235 and the subsequent fallback/level-check logic (lines 247–256) from ever running. This means:
- The
--levelflag verdict check (lines 253–256) is unreachable - The script always exits with success/failure from the Python validation, not from the requested level outcome
- Sample invocation:
run.sh --level core && echo "passed"will pass even if core checks failed
Fix: Restructure to capture the exit code without calling exit in the Python subprocess. Remove the && exit 0 || exit 1 tail and instead let the script continue:
- python3 - <<'PY' "$SCHEMA" "$OUT_JSON" && exit 0 || exit 1
- validate_ok=$?
+ python3 - <<'PY' "$SCHEMA" "$OUT_JSON"
+ validate_ok=$?🤖 Prompt for AI Agents
In scripts/harness/run.sh around lines 233 to 245, the inline python invocation
currently uses "&& exit 0 || exit 1" which causes the whole script to terminate
immediately and skips the later level-based verdict logic; remove the "&& exit 0
|| exit 1" tail, run the python heredoc without forcing an exit, capture its
exit code into validate_ok=$? immediately after the heredoc, and let the script
continue to the existing fallback/level-check logic so the --level verdict is
computed from validate_ok rather than the python process forcing an exit.
| # Allow either with: { go-version: '1.21.x' } OR no Go step | ||
| if rg -n "with: \{ *go-version: '1\.[0-9]+\.[0-9]+' *\}" "$f" -U -N >/dev/null 2>&1; then :; else :; fi | ||
| done |
There was a problem hiding this comment.
Dead code: conditional performs no-op in either branch.
Line 7 evaluates a Go version pattern but produces no effect regardless of match or miss (both branches execute :). Either the validation is unnecessary and the lines should be removed, or the logic is incomplete and should fail on an unsatisfied condition.
If the intent is to allow either presence or absence of a Go version (per the comment), remove lines 6-8 entirely. If you meant to enforce Go version presence, apply:
- # Allow either with: { go-version: '1.21.x' } OR no Go step
- if rg -n "with: \{ *go-version: '1\.[0-9]+\.[0-9]+' *\}" "$f" -U -N >/dev/null 2>&1; then :; else :; fi
+ # Require explicit Go version specification
+ rg -n "with: \{ *go-version: '1\.[0-9]+\.[0-9]+' *\}" "$f" -U -N >/dev/nullClarify intent and clean up.
📝 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.
| # Allow either with: { go-version: '1.21.x' } OR no Go step | |
| if rg -n "with: \{ *go-version: '1\.[0-9]+\.[0-9]+' *\}" "$f" -U -N >/dev/null 2>&1; then :; else :; fi | |
| done | |
| # Require explicit Go version specification | |
| rg -n "with: \{ *go-version: '1\.[0-9]+\.[0-9]+' *\}" "$f" -U -N >/dev/null | |
| done |
🤖 Prompt for AI Agents
In scripts/tests/feedback/13_ci_yaml_style.sh around lines 6 to 8, the if/else
conditional runs a regex check but both branches are no-ops (":"), making it
dead code; if the intent is to allow either presence or absence of the Go
version as the comment states, remove the entire conditional (lines 6–8) so only
the comment remains, otherwise if you intended to enforce presence change the
else branch to fail the script (e.g., print an error and exit non-zero) — pick
one and implement accordingly.
| set -euo pipefail | ||
| f=docs/spec/policy-wasm.md | ||
| # Count bullets that start with '- No ' | ||
| count=$(rg -n "^- No " "$f" || true | wc -l | tr -d ' ') |
There was a problem hiding this comment.
CRITICAL: Pipeline bug renders this test completely nonfunctional.
The operator precedence in bash parses your pipeline as rg ... || (true | wc | tr), NOT as (rg ... || true) | wc | tr.
What actually happens:
-
When rg finds matches (exit 0): rg outputs the matched lines with line numbers (e.g.,
"15:- No time\n16:- No RNG\n"), the|| true | wc -l | tr -d ' 'alternative is skipped, and$countis assigned the raw match text, not a count. -
Line 6 then attempts integer comparison
[ "$count" -ge 3 ]on a multi-line string → bash runtime error: "integer expression expected". -
When rg finds no matches (exit 1): the alternative executes,
$countbecomes"0", and the test passes.
Impact: The test fails with a bash error precisely when it should detect violations. Zero coverage.
Apply this fix:
-count=$(rg -n "^- No " "$f" || true | wc -l | tr -d ' ')
+count=$(rg -c "^- No " "$f" 2>/dev/null || echo 0)or if you prefer the explicit pipeline grouping:
-count=$(rg -n "^- No " "$f" || true | wc -l | tr -d ' ')
+count=$({ rg -n "^- No " "$f" || true; } | wc -l | tr -d ' ')The first option (using -c) is cleaner and avoids the intermediate line-number formatting.
📝 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.
| count=$(rg -n "^- No " "$f" || true | wc -l | tr -d ' ') | |
| count=$(rg -c "^- No " "$f" 2>/dev/null || echo 0) |
🤖 Prompt for AI Agents
In scripts/tests/feedback/16_policy_wasm_bullets.sh around line 5, the pipeline
uses `rg -n "^- No " "$f" || true | wc -l | tr -d ' '` which due to operator
precedence assigns raw match text to count when rg succeeds; change the command
to correctly produce a numeric count by either using rg's count flag (e.g., use
`rg -c "^- No " "$f" || true | tr -d ' '`) or explicitly group the alternative
so the fallback is applied before wc (e.g., wrap the rg/true in parentheses:
`(rg -n "^- No " "$f" || true) | wc -l | tr -d ' '`). Ensure $count receives
only a numeric string so the subsequent integer comparison works.
| - Returns `0` on acceptance; non-zero on rejection. | ||
| - Writes a short message (`ACCEPT` or `REJECT`) into the caller-provided buffer if non-null. | ||
| - Deterministic: no clock, RNG, network, filesystem, or host I/O. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Document the newline in the diagnostic output format.
Line 11 states the function "Writes a short message (ACCEPT or REJECT)", but the implementation (tests/policy/wasm/demo/src/lib.rs line 33) writes b"ACCEPT\n" and b"REJECT\n" with a trailing newline. Callers parsing the output buffer need to know this.
-- Writes a short message (`ACCEPT` or `REJECT`) into the caller-provided buffer if non-null.
+- Writes a short message (`ACCEPT\n` or `REJECT\n`) into the caller-provided buffer if non-null.or
-- Writes a short message (`ACCEPT` or `REJECT`) into the caller-provided buffer if non-null.
+- Writes a short newline-terminated message (`ACCEPT` or `REJECT`) into the caller-provided buffer if non-null.📝 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.
| - Returns `0` on acceptance; non-zero on rejection. | |
| - Writes a short message (`ACCEPT` or `REJECT`) into the caller-provided buffer if non-null. | |
| - Deterministic: no clock, RNG, network, filesystem, or host I/O. | |
| - Returns `0` on acceptance; non-zero on rejection. | |
| - Writes a short message (`ACCEPT\n` or `REJECT\n`) into the caller-provided buffer if non-null. | |
| - Deterministic: no clock, RNG, network, filesystem, or host I/O. |
| - Returns `0` on acceptance; non-zero on rejection. | |
| - Writes a short message (`ACCEPT` or `REJECT`) into the caller-provided buffer if non-null. | |
| - Deterministic: no clock, RNG, network, filesystem, or host I/O. | |
| - Returns `0` on acceptance; non-zero on rejection. | |
| - Writes a short newline-terminated message (`ACCEPT` or `REJECT`) into the caller-provided buffer if non-null. | |
| - Deterministic: no clock, RNG, network, filesystem, or host I/O. |
🤖 Prompt for AI Agents
tests/policy/wasm/demo/README.md lines 10-12: the docs say the function writes
"ACCEPT" or "REJECT" but the implementation appends a trailing newline
("ACCEPT\n" / "REJECT\n"); update the README to state that the diagnostic output
includes a trailing newline so callers know to trim or parse accordingly (e.g.,
"Writes a short message (`ACCEPT\n` or `REJECT\n`) into the caller-provided
buffer if non-null" or equivalent).
| #![no_std] | ||
|
|
||
| // Minimal, deterministic policy demo ABI. | ||
| // | ||
| // Exported symbol: | ||
| // validate(entry_ptr, entry_len, state_ptr, state_len, out_ptr, out_len_ptr) -> u32 | ||
| // | ||
| // Semantics: | ||
| // - Returns 0 to indicate acceptance; non-zero for rejection. | ||
| // - Writes a short UTF-8 message into [out_ptr, out_ptr+*out_len_ptr) if non-null. | ||
| // - This demo accepts iff entry_len is even AND state_len is even; otherwise rejects. | ||
| // - Purely deterministic: no clock, randomness, or host I/O. | ||
|
|
||
| /// Safety | ||
| /// | ||
| /// Caller must uphold the following preconditions: | ||
| /// - `entry_ptr` is valid for reads of `entry_len` bytes when `entry_len>0`. | ||
| /// - `state_ptr` is valid for reads of `state_len` bytes when `state_len>0`. | ||
| /// - If `out_ptr` is non-null, it is valid for writes of `*out_len_ptr` bytes. | ||
| /// - If `out_len_ptr` is non-null, it is valid for reading and writing a `usize`. | ||
| /// - All non-null pointers are properly aligned for their types. | ||
| /// - The output buffer `[out_ptr, out_ptr + *out_len_ptr)` does not overlap inputs. | ||
| #[no_mangle] | ||
| pub extern "C" fn validate( | ||
| _entry_ptr: *const u8, | ||
| entry_len: usize, | ||
| _state_ptr: *const u8, | ||
| state_len: usize, | ||
| out_ptr: *mut u8, | ||
| out_len_ptr: *mut usize, | ||
| ) -> u32 { | ||
| let accept = (entry_len & 1) == 0 && (state_len & 1) == 0; | ||
| let msg = if accept { b"ACCEPT\n" } else { b"REJECT\n" }; | ||
|
|
||
| unsafe { | ||
| if !out_ptr.is_null() && !out_len_ptr.is_null() { | ||
| let mut n = core::ptr::read(out_len_ptr); | ||
| if n > msg.len() { n = msg.len(); } | ||
| core::ptr::copy_nonoverlapping(msg.as_ptr(), out_ptr, n); | ||
| core::ptr::write(out_len_ptr, n); | ||
| } | ||
| } | ||
| if accept { 0 } else { 1 } | ||
| } | ||
|
|
||
| use core::panic::PanicInfo; | ||
|
|
||
| #[panic_handler] | ||
| fn panic(_info: &PanicInfo) -> ! { | ||
| loop {} | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Impeccable FFI hygiene and panic-handler compliance. No mercy found; implementation is solid.
The safety contract (lines 14-22) is explicit and comprehensive. The unsafe operations (lines 35-42) correctly enforce preconditions via null checks before dereferencing, clamp buffer writes to prevent overruns, and use copy_nonoverlapping from a local literal (no aliasing risk). The panic handler (lines 48-51) is appropriate for a sandboxed WASM environment with host-enforced fuel limits.
Minor observation (no action required): The demo writes "ACCEPT\n" or "REJECT\n" (with trailing newlines), which is consistent with "short UTF-8 message" in the spec (docs/spec/policy-wasm.md line 39). However, if the caller provides a buffer smaller than 7 bytes, the output will be truncated mid-word (e.g., "ACC"), which may be unexpected. This is acceptable for a demo, and the safety contract already places the burden on the caller to provide adequate buffer space.
Verify that the diagnostic output format (newline-terminated strings, potential truncation) is explicitly documented in the spec or in tests/policy/wasm/demo/README.md:
🏁 Script executed:
#!/bin/bash
# Description: Check if the demo README or spec documents the diagnostic format.
# Search for documentation of ACCEPT/REJECT format in the README
echo "=== README.md ==="
rg -C3 'ACCEPT|REJECT' tests/policy/wasm/demo/README.md
# Search for diagnostic format documentation in the spec
echo "=== policy-wasm.md ==="
rg -C3 'diagnostic|message' docs/spec/policy-wasm.mdLength of output: 1250
Documentation does not match implementation; newline characters are undocumented.
The README states the output is ACCEPT or REJECT with no mention of the trailing newline character. However, the code at lines 32–33 writes b"ACCEPT\n" and b"REJECT\n", respectively. This undocumented discrepancy creates a contract violation: callers expecting 6–7 byte messages will receive 7–8 bytes with an appended newline, causing truncation or buffer overrun if they allocate a buffer based on the README.
- Update
tests/policy/wasm/demo/README.mdto explicitly document that output includes a trailing newline:"ACCEPT\n"(8 bytes) or"REJECT\n"(7 bytes). - Document the minimum required buffer size (at least 8 bytes) to prevent truncation.
🤖 Prompt for AI Agents
In tests/policy/wasm/demo/src/lib.rs lines 1–51 the implementation writes a
trailing newline (b"ACCEPT\n" / b"REJECT\n") but the README does not document
that; update tests/policy/wasm/demo/README.md to state the output strings
include a trailing newline (i.e., "ACCEPT\n" and "REJECT\n") and specify the
minimum output buffer size required (at least 7 bytes) so callers allocate
enough space to avoid truncation or overruns.
…es as enforcement)
…d Cargo.lock for demo crate
| target/ | ||
| Cargo.lock |
There was a problem hiding this comment.
🧩 Analysis chain
Verify crate type before committing Cargo.lock; add .wasm artifact ignore.
The .gitignore needs clarification on two fronts:
-
Cargo.lock(line 2): Per Rust/Cargo best practices, library crates should NOT commitCargo.lock(it's generated locally). Only binaries should commit it. Since the enriched summary describes this as a "C ABI validate function" (indicating a library), this entry may be incorrect unless the crate is explicitly a binary. -
Missing
*.wasmentry: Compiled WASM binaries should be ignored as build artifacts. Add*.wasmto prevent accidental commits of compiled output.
Please verify the crate type by inspecting the Cargo.toml:
Then apply the appropriate fix:
If it's a library (most likely for WASM modules):
-target/
-Cargo.lock
+target/
+*.wasmIf it's a binary (less likely, but possible):
target/
Cargo.lock
+*.wasm🏁 Script executed:
#!/bin/bash
# Verify crate type (library vs. binary) for the WASM demo
cat tests/policy/wasm/demo/Cargo.toml | grep -A 5 '^\[lib\]\|^\[\[bin\]\]' || echo "No explicit crate type; defaults to lib if src/lib.rs exists"
[ -f tests/policy/wasm/demo/src/lib.rs ] && echo "✓ src/lib.rs exists (library crate)" || echo "✗ src/lib.rs not found"
[ -f tests/policy/wasm/demo/src/main.rs ] && echo "✓ src/main.rs exists (binary crate)" || echo "✗ src/main.rs not found"Length of output: 142
Remove Cargo.lock from library crate .gitignore; add *.wasm artifact ignore.
This .gitignore violates Rust best practices for library crates:
-
Cargo.lock(line 2)—REMOVE THIS. Per Cargo specifications, library crates must NOT commitCargo.lock; it's generated locally during development. YourCargo.tomldeclares[lib]withcrate-type = ["cdylib"], confirming this is a library. CommittingCargo.lockfor libraries is incorrect and will cause version conflicts for consumers. -
Missing
*.wasmentry—ADD THIS. Compiled WASM artifacts are build outputs and must be ignored to prevent accidental commits.
Corrected .gitignore:
target/
*.wasm
🤖 Prompt for AI Agents
In tests/policy/wasm/demo/.gitignore around lines 1 to 2, remove the committed
Cargo.lock entry and add an ignore for compiled wasm artifacts; specifically
delete the second line "Cargo.lock" and add a new line "*.wasm" so the file
contains the build target dir and wasm artifacts only.
This PR completes the remaining spec/docs/DX tasks:
v*and attach to GitHub Releasetests/policy/wasm/demo(builds to.wasmfor Policy level demos)Notes:
docs/.vitepress/config.tsstill hasignoreDeadLinks: true. We can flip it tofalseafter the remaining link migrations are finished; the site builds clean locally now.tmp/junit.xmland ignorestmp/.Follow‑ups (separate PRs OK):
SPEC_VERSIONinto docs footer at build time (e.g., read latest tag)