experiment(conoir): coNoir × apertrue double-dip feasibility + locked architecture#6
Open
newtsjamie wants to merge 9 commits into
Open
experiment(conoir): coNoir × apertrue double-dip feasibility + locked architecture#6newtsjamie wants to merge 9 commits into
newtsjamie wants to merge 9 commits into
Conversation
Proof-of-life for the apertrue x coNoir primitive: a minimal nullifier membership circuit (Poseidon2 + field equality) run through the full 3-party REP3 MPC pipeline. Both no-collision (public bit 0) and collision (public bit 1) cases generate and verify; only the bit is revealed, the candidate and set stay secret-shared. Reference artifacts only (circuit + run script + README) — the co-noir binary/CRS are reproducible from the co-snarks repo and not committed. Not for main; lineage for the upcoming IMT-non-membership version. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
imt_nm_mini isolates the two bit-decomposition ops Colofon's check_non_membership needs (Field::lt range check + to_le_bits Merkle path) plus Poseidon2. Ran under 3-party REP3 MPC -> proof verified, so coNoir's co-brillig/co-acvm handle them and the full Colofon IMT will port. Depth-8 single check ~4s (build-proving-key ~2.0s dominant). README notes the beta.20 migrations needed for the real port (u1 removed -> bool; to_le_bits returns [bool; N]). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
imt_nm_scale mirrors Colofon's per-component non-membership shape (leaf-preimage hash + low-leaf lt + 32-level Merkle path), batched over N checks; bench_imt_scale.sh sweeps N and times the MPC pipeline. Full Colofon scale (N=50, depth-32, 153,843 gates) under 3-party REP3 MPC: ~160s total (proving-key 107s dominant, proof 30s, witness 23s), verify constant 29ms. ~3k gates/check. Nodes co-located (no network latency). Feasibility + perf envelope established. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
bound_nm demonstrates the apertrue <-> coNoir binding: a public hiding
commitment Commit(nullifier, blind) is the C2PA authenticity anchor; the
party secret-shares (nullifier, blind); a lightweight in-MPC opening
check binds them; non-membership runs on the secret-shared nullifier;
only the collision bit is revealed. No recursive verification in MPC.
Verified under 3-party MPC: no-collision -> 0, collision -> 1, and
binding-failure (substituted nullifier) -> rejected ("commitment opening
failed"). README adds the threat-model notes (membership-oracle defense
via authenticated queries; coNoir is semi-honest only).
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
imt_real calls the REAL colofon_imt lib (check_non_membership + CveLeafPreimage), compiled under nargo beta.20 and run under 3-party coNoir MPC: non_existence proven, verified, ~3.5s at depth-8. Confirms faithfulness, not just a reimplementation. Two migrations needed against beta.20: root.nr u1 -> bool, and bump the poseidon dep v0.1.1 -> v0.3.0 (v0.1.1 fails: comptime RATE + hash arity). README documents these. Networked-latency item left open (needs real infra). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
binding_wrapper is the client-side, off-MPC step: recursively verifies an apertrue proof (real bb_proof_verification lib, beta.18) and emits C = Commit(public_values[2], blind) -- a commitment to the VERIFIED nullifier, not a free witness. Gate count 773,025: smaller than image_aggregator (~1.5M, 2x verify) which apertrue already proves client-side in production, and within initSRSChonk(2^21) capacity. So the recursion overhead is in-budget, not a new cost category. README records the locked architecture: containerised downstream sidecar, composition (Option A) not duplication (B) nor production mutation (C now). N stays unblinded; proof_a/proof_b untouched. Remaining risk is product (lighthouse + design partner + node governance), not engineering. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds coNoir spike circuits, Quorum commitment and proof circuits, adapter and provenance tooling, and supporting scripts and documentation for the MPC flows. ChangescoNoir Spike and Quorum Pipelines
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90+ minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@experiments/conoir-spike/bench_imt_scale.sh`:
- Around line 8-11: The TV variable pointing to test_vectors/imt_nm_scale does
not match the actual location of the circuit files in the current package
layout. Update the TV path constant to reference the correct location at
experiments/conoir-spike/imt_nm_scale so that when the benchmark script executes
in that target path (as performed on line 35), it can properly locate the
necessary circuit files and prevent the benchmark from failing before proving.
In `@experiments/conoir-spike/binding_wrapper/Nargo.toml`:
- Around line 5-10: Update the compiler_version field in the Nargo.toml file
from the permissive constraint ">=1.0.0" to the specific pinned version
"1.0.0-beta.18" to match the documented and tested configuration for the
binding_wrapper module. This ensures consistency with the bb_proof_verification
dependency and the README documentation which explicitly targets the beta.18
toolchain.
In `@experiments/conoir-spike/binding_wrapper/src/main.nr`:
- Around line 23-27: The verify_honk_proof call does not validate that vk_hash
corresponds to an authentic or allowlisted verification key before extracting
the nullifier from public_values. Add a constraint check before calling
verify_honk_proof that compares the provided vk_hash against a stored or
allowlisted authentic vk_hash value (similar to the pattern used in
aztec_verifier/src/main.nr lines 202-203). This constraint must be enforced
before the verify_honk_proof invocation to ensure only trusted circuits can have
their public outputs used, preventing an attacker from providing a valid proof
from an alternative circuit while the wrapper extracts values from the wrong
circuit's output.
In `@experiments/conoir-spike/bound_nm/src/main.nr`:
- Around line 18-20: The commit function in bound_nm and the commitment
computation in binding_wrapper use different Poseidon2 APIs that may not produce
identical results, risking silent failures if they diverge. Unify the commitment
primitive by ensuring both modules use the same implementation and API for
computing commitments from a value and blind parameter. Either update the commit
function to match the sponge-based Poseidon2::hash approach used in
binding_wrapper or update binding_wrapper to use the same raw permutation
approach, ensuring both produce identical commitments for the same inputs.
In `@experiments/conoir-spike/run_imt_nm_mini.sh`:
- Around line 38-39: The echo statement describing public_input incorrectly
labels the output as "the revealed collision bit" when the circuit actually
publishes the computed IMT root. Update the echo string in the section that
displays the public_input file content to correctly describe the output as the
IMT root instead of referencing a collision bit.
In `@experiments/conoir-spike/run_nullifier_check.sh`:
- Around line 16-19: The script uses `jobs -p` to wait for background processes
in three stages (generate-witness, proving-key generation, and proof
generation), but `jobs -p` only returns currently running jobs and may miss
already-finished jobs, causing the script to exit successfully even if
background processes failed. Replace the `jobs -p` pattern in all three stages
by capturing each background process's PID immediately after launching it using
the $! variable, storing these PIDs (for the three "CO" generate-witness
commands in the first stage, the three proving-key commands in the second stage,
and the three proof commands in the third stage), and then explicitly waiting
for each PID individually with error checking to ensure any failures are
properly detected and propagated.
- Line 7: The CO variable assignment on line 7 uses an incorrect relative path
with too many directory traversals. The path ../../../target/release/co-noir
goes up one directory too many from the experiments/conoir-spike/ location.
Change the CO variable to use ../../target/release/co-noir instead, which
correctly navigates up two levels to reach the project root where the co-noir
binary is located.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3071c6d5-dc7e-439b-9c16-8b4f7bd74b89
📒 Files selected for processing (21)
experiments/conoir-spike/README.mdexperiments/conoir-spike/bench_imt_scale.shexperiments/conoir-spike/binding_wrapper/.gitignoreexperiments/conoir-spike/binding_wrapper/Nargo.tomlexperiments/conoir-spike/binding_wrapper/src/main.nrexperiments/conoir-spike/bound_nm/Nargo.tomlexperiments/conoir-spike/bound_nm/Prover.tomlexperiments/conoir-spike/bound_nm/src/main.nrexperiments/conoir-spike/imt_nm_mini/Nargo.tomlexperiments/conoir-spike/imt_nm_mini/Prover.tomlexperiments/conoir-spike/imt_nm_mini/src/main.nrexperiments/conoir-spike/imt_nm_scale/Nargo.tomlexperiments/conoir-spike/imt_nm_scale/src/main.nrexperiments/conoir-spike/imt_real/Nargo.tomlexperiments/conoir-spike/imt_real/Prover.tomlexperiments/conoir-spike/imt_real/src/main.nrexperiments/conoir-spike/nullifier_check/Nargo.tomlexperiments/conoir-spike/nullifier_check/Prover.tomlexperiments/conoir-spike/nullifier_check/src/main.nrexperiments/conoir-spike/run_imt_nm_mini.shexperiments/conoir-spike/run_nullifier_check.sh
Comment on lines
+8
to
+11
| CO=../../../target/release/co-noir | ||
| TV=test_vectors/imt_nm_scale | ||
| CRS1=../../co-noir-common/src/crs/bn254_g1.dat | ||
| CRS2=../../co-noir-common/src/crs/bn254_g2.dat |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix benchmark path constants to match this package layout.
Line 9 targets test_vectors/imt_nm_scale, but this PR adds the circuit at experiments/conoir-spike/imt_nm_scale; Line 35 then executes in that target path. As written, the benchmark can fail before proving.
💡 Suggested patch
-NARGO=~/.nargo/bin/nargo
-CO=../../../target/release/co-noir
-TV=test_vectors/imt_nm_scale
+NARGO=~/.nargo/bin/nargo
+CO=../../target/release/co-noir
+TV=imt_nm_scaleAlso applies to: 17-17, 35-35
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/bench_imt_scale.sh` around lines 8 - 11, The TV
variable pointing to test_vectors/imt_nm_scale does not match the actual
location of the circuit files in the current package layout. Update the TV path
constant to reference the correct location at
experiments/conoir-spike/imt_nm_scale so that when the benchmark script executes
in that target path (as performed on line 35), it can properly locate the
necessary circuit files and prevent the benchmark from failing before proving.
Comment on lines
+5
to
+10
| compiler_version = ">=1.0.0" | ||
| description = "Option A: recursively verify an apertrue proof, emit C = Commit(nullifier, blind). Client-side, off-MPC. Production proof_a/proof_b untouched." | ||
|
|
||
| [dependencies] | ||
| bb_proof_verification = { git = "https://github.com/AztecProtocol/aztec-packages/", tag = "v4.0.0-devnet.2-patch.3", directory = "barretenberg/noir/bb_proof_verification" } | ||
| poseidon = { tag = "v0.1.1", git = "https://github.com/noir-lang/poseidon" } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Cross-check declared versions vs documented compatibility notes.
rg -n 'compiler_version|poseidon|beta\.18|beta\.20|v0\.1\.1|v0\.3\.0' \
experiments/conoir-spike/binding_wrapper/Nargo.toml \
experiments/conoir-spike/bound_nm/Nargo.toml \
experiments/conoir-spike/README.mdRepository: Apertrue/circuits
Length of output: 1465
🏁 Script executed:
cat -n experiments/conoir-spike/README.md | sed -n '20,100p'Repository: Apertrue/circuits
Length of output: 5786
🏁 Script executed:
cat -n experiments/conoir-spike/README.md | sed -n '99,120p'Repository: Apertrue/circuits
Length of output: 1908
🏁 Script executed:
cat experiments/conoir-spike/binding_wrapper/Nargo.tomlRepository: Apertrue/circuits
Length of output: 680
🏁 Script executed:
cat -n experiments/conoir-spike/README.md | sed -n '85,105p'Repository: Apertrue/circuits
Length of output: 1681
Pin compiler_version to the documented beta.18 toolchain.
The manifest declares compiler_version = ">=1.0.0" but README line 102 explicitly documents that binding_wrapper targets the beta.18 toolchain (using bb_proof_verification's beta.18 version). Pin to "1.0.0-beta.18" to match the tested configuration. The current poseidon v0.1.1 is compatible with beta.18 (the v0.1.1→v0.3.0 migration mentioned in the README applies to the beta.20 port of imt_real, not this module).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/binding_wrapper/Nargo.toml` around lines 5 - 10,
Update the compiler_version field in the Nargo.toml file from the permissive
constraint ">=1.0.0" to the specific pinned version "1.0.0-beta.18" to match the
documented and tested configuration for the binding_wrapper module. This ensures
consistency with the bb_proof_verification dependency and the README
documentation which explicitly targets the beta.18 toolchain.
Comment on lines
+23
to
+27
| verify_honk_proof(vk, proof, public_values, vk_hash); | ||
|
|
||
| // 2. The nullifier is the VERIFIED public output at index 2. | ||
| let nullifier = public_values[2]; | ||
|
|
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare wrapper vs upstream verifier pattern for VK hash binding.
rg -n 'verify_honk_proof|vk_hash|VK hash mismatch|assert\(vk_hash' \
experiments/conoir-spike/binding_wrapper/src/main.nr \
aztec_verifier/src/main.nrRepository: Apertrue/circuits
Length of output: 2307
🏁 Script executed:
# Read the full main.nr file from binding_wrapper to understand current implementation
cat -n experiments/conoir-spike/binding_wrapper/src/main.nrRepository: Apertrue/circuits
Length of output: 1901
🏁 Script executed:
# Check the aztec_verifier function signature to understand the allowlisted vk_hash pattern
sed -n '155,210p' aztec_verifier/src/main.nrRepository: Apertrue/circuits
Length of output: 3377
Constrain vk_hash to an allowlisted key before extracting the nullifier.
The verify_honk_proof call at line 23 verifies internal consistency of (vk, proof, public_values, vk_hash) but does not validate that vk_hash corresponds to the authentic apertrue circuit. An attacker can provide a valid proof and verification key for a different circuit, and verify_honk_proof will succeed with that alternative VK while the wrapper extracts public_values[2] from the wrong circuit's output.
Compare this to aztec_verifier/src/main.nr lines 202-203, which validates vk_hash == stored_vk_hash before calling verify_honk_proof. The binding_wrapper should follow the same pattern:
Proposed hardening
fn main(
proof: UltraHonkZKProof,
vk: UltraHonkVerificationKey,
public_values: [Field; 12],
vk_hash: Field,
+ expected_vk_hash: pub Field,
blind: Field,
) -> pub Field {
+ assert(vk_hash == expected_vk_hash, "VK hash mismatch");
verify_honk_proof(vk, proof, public_values, vk_hash);
let nullifier = public_values[2];
Poseidon2::hash([nullifier, blind], 2)
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/binding_wrapper/src/main.nr` around lines 23 - 27,
The verify_honk_proof call does not validate that vk_hash corresponds to an
authentic or allowlisted verification key before extracting the nullifier from
public_values. Add a constraint check before calling verify_honk_proof that
compares the provided vk_hash against a stored or allowlisted authentic vk_hash
value (similar to the pattern used in aztec_verifier/src/main.nr lines 202-203).
This constraint must be enforced before the verify_honk_proof invocation to
ensure only trusted circuits can have their public outputs used, preventing an
attacker from providing a valid proof from an alternative circuit while the
wrapper extracts values from the wrong circuit's output.
Comment on lines
+18
to
+20
| fn commit(value: Field, blind: Field) -> Field { | ||
| std::hash::poseidon2_permutation::<4>([value, blind, 0, 0])[0] | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the two commitment constructions used by the seam.
rg -n 'poseidon2_permutation|Poseidon2::hash|fn commit' \
experiments/conoir-spike/bound_nm/src/main.nr \
experiments/conoir-spike/binding_wrapper/src/main.nr \
tree_aggregator/src/main.nr
# Expected: one canonical construction everywhere, or explicit documented/tested equivalence.Repository: Apertrue/circuits
Length of output: 1047
🏁 Script executed:
# Check imports and understand what Poseidon2::hash and poseidon2_permutation are
cat -n experiments/conoir-spike/bound_nm/src/main.nr | head -30Repository: Apertrue/circuits
Length of output: 1852
🏁 Script executed:
# Check binding_wrapper to understand line 29 in full context
cat -n experiments/conoir-spike/binding_wrapper/src/main.nr | head -35Repository: Apertrue/circuits
Length of output: 1901
🏁 Script executed:
# Search for any tests or documentation about Poseidon2 commitment equivalence
fd -t f '.*test.*\|.*spec.*' experiments/conoir-spike/ | head -20Repository: Apertrue/circuits
Length of output: 155
🏁 Script executed:
# Look for Poseidon2 imports and their origins
rg -n 'use.*Poseidon2|use.*poseidon2_permutation' experiments/conoir-spike/Repository: Apertrue/circuits
Length of output: 371
🏁 Script executed:
# Search for any test files that test commitments
fd -t f '\.(test|spec)\.' --type f . | xargs grep -l 'commit\|Poseidon2' 2>/dev/null | head -20Repository: Apertrue/circuits
Length of output: 155
🏁 Script executed:
# Check if there are any comments or documentation about the equivalence
rg -n 'poseidon2_permutation|equivalence|commitment.*hash' experiments/conoir-spike/ --type nr -B 2 -A 2Repository: Apertrue/circuits
Length of output: 182
🏁 Script executed:
# Check the README for any notes about this issue
cat experiments/conoir-spike/README.mdRepository: Apertrue/circuits
Length of output: 7911
🏁 Script executed:
# Search for any tests in the entire repository that might test this
find . -name '*.nr' -type f | xargs grep -l 'fn.*test\|#\[test\]' 2>/dev/null | head -10Repository: Apertrue/circuits
Length of output: 577
Unify the commitment primitive across bound_nm and binding_wrapper.
bound_nm computes commitments with std::hash::poseidon2_permutation::<4>([value, blind, 0, 0])[0], while binding_wrapper emits Poseidon2::hash([nullifier, blind], 2). These are different APIs (raw permutation vs. sponge-based hash) and must produce identical commitments for the binding to remain secure. The architecture depends on this equivalence, but uses two distinct implementations without explicit cross-module verification. If these diverge—whether immediately or across future Noir/coNoir versions—valid openings will fail silently.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/bound_nm/src/main.nr` around lines 18 - 20, The
commit function in bound_nm and the commitment computation in binding_wrapper
use different Poseidon2 APIs that may not produce identical results, risking
silent failures if they diverge. Unify the commitment primitive by ensuring both
modules use the same implementation and API for computing commitments from a
value and blind parameter. Either update the commit function to match the
sponge-based Poseidon2::hash approach used in binding_wrapper or update
binding_wrapper to use the same raw permutation approach, ensuring both produce
identical commitments for the same inputs.
Comment on lines
+38
to
+39
| echo "--- public_input (the revealed collision bit) ---" | ||
| cat "$TV/public_input" 2>/dev/null || echo "(no public_input file)" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fix mislabeled public output description.
Line 38 says the output is a “collision bit,” but this circuit publishes the computed IMT root (pub Field). Please relabel to avoid incorrect interpretation of experiment results.
Proposed fix
-echo "--- public_input (the revealed collision bit) ---"
+echo "--- public_input (the revealed IMT root) ---"📝 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.
Suggested change
| echo "--- public_input (the revealed collision bit) ---" | |
| cat "$TV/public_input" 2>/dev/null || echo "(no public_input file)" | |
| echo "--- public_input (the revealed IMT root) ---" | |
| cat "$TV/public_input" 2>/dev/null || echo "(no public_input file)" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/run_imt_nm_mini.sh` around lines 38 - 39, The echo
statement describing public_input incorrectly labels the output as "the revealed
collision bit" when the circuit actually publishes the computed IMT root. Update
the echo string in the section that displays the public_input file content to
correctly describe the output as the IMT root instead of referencing a collision
bit.
| set -e | ||
| cd "$(dirname "$(realpath "$0")")" | ||
|
|
||
| CO=../../../target/release/co-noir # built binary (faster than cargo run) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fix the co-noir binary path resolution.
Line 7 likely points one directory too high, which can make the script fail before pipeline start.
Proposed fix
-CO=../../../target/release/co-noir # built binary (faster than cargo run)
+CO=../../target/release/co-noir # built binary (faster than cargo run)📝 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.
Suggested change
| CO=../../../target/release/co-noir # built binary (faster than cargo run) | |
| CO=../../target/release/co-noir # built binary (faster than cargo run) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/run_nullifier_check.sh` at line 7, The CO variable
assignment on line 7 uses an incorrect relative path with too many directory
traversals. The path ../../../target/release/co-noir goes up one directory too
many from the experiments/conoir-spike/ location. Change the CO variable to use
../../target/release/co-noir instead, which correctly navigates up two levels to
reach the project root where the co-noir binary is located.
Comment on lines
+16
to
+19
| "$CO" generate-witness --input "$TV/Prover.toml.0.shared" --circuit "$TV/target/nullifier_check.json" --protocol REP3 --config configs/party1.toml --out "$TV/witness.0.shared" & | ||
| "$CO" generate-witness --input "$TV/Prover.toml.1.shared" --circuit "$TV/target/nullifier_check.json" --protocol REP3 --config configs/party2.toml --out "$TV/witness.1.shared" & | ||
| "$CO" generate-witness --input "$TV/Prover.toml.2.shared" --circuit "$TV/target/nullifier_check.json" --protocol REP3 --config configs/party3.toml --out "$TV/witness.2.shared" | ||
| wait $(jobs -p) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Don’t rely on jobs -p for failure propagation in background stages.
Lines 19, 25, and 34 can miss failed background processes (already-finished jobs are not guaranteed to appear in jobs -p), causing false-success runs.
Proposed fix
echo "### 2. generate-witness in MPC (3 parties) ###"
-"$CO" generate-witness --input "$TV/Prover.toml.0.shared" --circuit "$TV/target/nullifier_check.json" --protocol REP3 --config configs/party1.toml --out "$TV/witness.0.shared" &
-"$CO" generate-witness --input "$TV/Prover.toml.1.shared" --circuit "$TV/target/nullifier_check.json" --protocol REP3 --config configs/party2.toml --out "$TV/witness.1.shared" &
-"$CO" generate-witness --input "$TV/Prover.toml.2.shared" --circuit "$TV/target/nullifier_check.json" --protocol REP3 --config configs/party3.toml --out "$TV/witness.2.shared"
-wait $(jobs -p)
+"$CO" generate-witness --input "$TV/Prover.toml.0.shared" --circuit "$TV/target/nullifier_check.json" --protocol REP3 --config configs/party1.toml --out "$TV/witness.0.shared" & p1=$!
+"$CO" generate-witness --input "$TV/Prover.toml.1.shared" --circuit "$TV/target/nullifier_check.json" --protocol REP3 --config configs/party2.toml --out "$TV/witness.1.shared" & p2=$!
+"$CO" generate-witness --input "$TV/Prover.toml.2.shared" --circuit "$TV/target/nullifier_check.json" --protocol REP3 --config configs/party3.toml --out "$TV/witness.2.shared" & p3=$!
+wait "$p1"; wait "$p2"; wait "$p3"Apply the same pattern to proving-key and proof stages.
Also applies to: 22-25, 31-34
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 19-19: Quote this to prevent word splitting.
(SC2046)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/run_nullifier_check.sh` around lines 16 - 19, The
script uses `jobs -p` to wait for background processes in three stages
(generate-witness, proving-key generation, and proof generation), but `jobs -p`
only returns currently running jobs and may miss already-finished jobs, causing
the script to exit successfully even if background processes failed. Replace the
`jobs -p` pattern in all three stages by capturing each background process's PID
immediately after launching it using the $! variable, storing these PIDs (for
the three "CO" generate-witness commands in the first stage, the three
proving-key commands in the second stage, and the three proof commands in the
third stage), and then explicitly waiting for each PID individually with error
checking to ensure any failures are properly detected and propagated.
Source: Linters/SAST tools
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@experiments/conoir-spike/ONE-PAGER.md`:
- Line 2: The heading "Let distrusting organizations check each other's data for
fraud — without sharing it, and without anyone being able to lie about it." is
currently marked as h3 with three hash symbols (###) but should be marked as h2
with two hash symbols (##) to follow proper markdown heading hierarchy and
comply with the markdownlint MD001 rule that prohibits skipping heading levels.
Change the three hash symbols to two hash symbols at the beginning of this line.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2bab0aae-c8af-4cc2-b5a8-f0370a157a5e
📒 Files selected for processing (1)
experiments/conoir-spike/ONE-PAGER.md
| @@ -0,0 +1,34 @@ | |||
| # Apertrue — Authenticated Private Computation | |||
| ### *Let distrusting organizations check each other's data for fraud — without sharing it, and without anyone being able to lie about it.* | |||
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix markdown heading hierarchy violation.
Line 2 is marked as h3 (###) but should be h2 (##) to comply with markdown heading-level incrementing rules (no skipping levels). This is flagged by markdownlint MD001.
🔧 Proposed fix
- ### *Let distrusting organizations check each other's data for fraud — without sharing it, and without anyone being able to lie about it.*
+ ## *Let distrusting organizations check each other's data for fraud — without sharing it, and without anyone being able to lie about it.*📝 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.
Suggested change
| ### *Let distrusting organizations check each other's data for fraud — without sharing it, and without anyone being able to lie about it.* | |
| ## *Let distrusting organizations check each other's data for fraud — without sharing it, and without anyone being able to lie about it.* |
🧰 Tools
🪛 markdownlint-cli2 (0.22.1)
[warning] 2-2: Heading levels should only increment by one level at a time
Expected: h2; Actual: h3
(MD001, heading-increment)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/ONE-PAGER.md` at line 2, The heading "Let
distrusting organizations check each other's data for fraud — without sharing
it, and without anyone being able to lie about it." is currently marked as h3
with three hash symbols (###) but should be marked as h2 with two hash symbols
(##) to follow proper markdown heading hierarchy and comply with the
markdownlint MD001 rule that prohibits skipping heading levels. Change the three
hash symbols to two hash symbols at the beginning of this line.
Source: Linters/SAST tools
…esign Captures the Quorum authenticated-private-computation spike, with this session's additions: - ARCHITECTURE.md: envelope-agnostic core + pluggable input adapters, two-signer model, open decisions, honest status + build sequence. - proof_a_receivable: in-circuit "Proof A" -- verifies the obligor/issuer ECDSA-P256 signature over canonical_id and the role-bound trust-list leaf, emits the role-stamped anchor. Proven end-to-end with negative tests (bad sig, unauthorised role); run_proof_a.sh. - adapters/: formal adapter interface (ADAPTER.md) + FatturaPA reference adapter (fattura_adapter.py) proving the full pipeline on the real invoice (real FatturaPA -> canonical_id -> Proof A -> bound_receivables); run_fattura_pipeline.sh. - _mkroot: Poseidon2 Merkle-root helper. Also commits the prior untracked spike: commit/bound receivable + aggregate engines and the p1_provenance FatturaPA->C2PA demo. Stand-in obligor key and in-circuit X.509-chain verification are documented follow-ups (real signer = partner crux). Private keys and build artifacts are gitignored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 18
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🟡 Minor comments (5)
experiments/conoir-spike/quorum/p1_provenance/README.md-46-49 (1)
46-49: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the reproduction input file.
cupsfilter invoice.txt > invoice.pdfno longer matches the rest of this section, which is about the real FatturaPA XML sample. As written, the command will generate the wrong artifact (or fail), so the later C2PA and MPC steps are not actually reproducible.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/conoir-spike/quorum/p1_provenance/README.md` around lines 46 - 49, The reproduction steps in the README are using the wrong input file, so update the command in the C2PA/PDF workflow to use the existing FatturaPA XML sample instead of invoice.txt. Make sure the command sequence around the render/sign flow still points to the same sample artifact used by the rest of this section, so the later C2PA and MPC steps remain reproducible.experiments/conoir-spike/quorum/adapters/run_fattura_pipeline.sh-18-18 (1)
18-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDon’t compare custom XML runs against the fixed fixture CID.
XMLis user-selectable, but Line 45 always expects the default invoice’sobligor_true.cid. Passing any other valid XML will fail before the proof pipeline.Guard the fixture-only assertion
-XML="${1:-$QUORUM/p1_provenance/IT01234567890_FPR01.xml}" +DEFAULT_XML="$QUORUM/p1_provenance/IT01234567890_FPR01.xml" +XML="${1:-$DEFAULT_XML}" @@ -# confirm canonical_id matches the committed obligor_true.cid -EXPECT_CID="0x$(xxd -p "$QUORUM/p1_provenance/admission/certs/obligor_true.cid" | tr -d '\n')" -if [ "$CANONICAL_ID" = "$EXPECT_CID" ]; then - green "PASS: canonical_id == obligor_true.cid ($EXPECT_CID)" -else - red "FAIL: canonical_id $CANONICAL_ID != obligor_true.cid $EXPECT_CID"; exit 1 +if [ "$XML" = "$DEFAULT_XML" ] && [ -f "$QUORUM/p1_provenance/admission/certs/obligor_true.cid" ]; then + EXPECT_CID="0x$(xxd -p "$QUORUM/p1_provenance/admission/certs/obligor_true.cid" | tr -d '\n')" + if [ "$CANONICAL_ID" = "$EXPECT_CID" ]; then + green "PASS: canonical_id == obligor_true.cid ($EXPECT_CID)" + else + red "FAIL: canonical_id $CANONICAL_ID != obligor_true.cid $EXPECT_CID"; exit 1 + fi fiAlso applies to: 44-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/conoir-spike/quorum/adapters/run_fattura_pipeline.sh` at line 18, The pipeline in run_fattura_pipeline.sh is asserting the fixture-specific CID even when XML is passed as a user-selected input, so custom XML runs fail unnecessarily. Update the CID check around the proof setup logic to only enforce obligor_true.cid when XML is the default fixture path, or otherwise skip/branch the assertion based on the selected XML; use the XML variable and the proof/fixture comparison block near the existing CID check to locate the fix.experiments/conoir-spike/quorum/_mkroot/src/main.nr-14-18 (1)
14-18: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winConstrain
merkle_indicesto bits.indices[i] == 0treats any nonzero field element as the right branch, so a malformed witness like2still passes. Add an explicit0/1constraint before branching.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/conoir-spike/quorum/_mkroot/src/main.nr` around lines 14 - 18, The Merkle path logic in the main proof routine branches on `indices[i] == 0`, which allows any nonzero witness value to be treated as the right branch. Update the `merkle_indices` handling in `main.nr` to explicitly constrain each `indices[i]` to be binary before it is used in the loop, so only 0 or 1 are accepted in the `is_left` check and the `Poseidon2::hash` path selection.experiments/conoir-spike/quorum/p1_provenance/p1_next.py-55-58 (1)
55-58: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winSplit compound statements so Ruff passes.
Lines 55, 58, and 108 currently violate Ruff E701/E702.
Proposed fix
- xml_bytes = open(xml_path, "rb").read() - r = PdfReader(invoice_pdf); w = PdfWriter(); w.append(r) + with open(xml_path, "rb") as f: + xml_bytes = f.read() + r = PdfReader(invoice_pdf) + w = PdfWriter() + w.append(r) @@ - with open(tmp, "wb") as f: w.write(f) - raw = open(tmp, "rb").read() + with open(tmp, "wb") as f: + w.write(f) + with open(tmp, "rb") as f: + raw = f.read() @@ - with open(out_path, "wb") as f: f.write(signed) + with open(out_path, "wb") as f: + f.write(signed)Also applies to: 108-108
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/conoir-spike/quorum/p1_provenance/p1_next.py` around lines 55 - 58, Split the compound statements in the invoice PDF workflow so Ruff no longer flags E701/E702: in the block around PdfReader, PdfWriter, append, add_attachment, and the write call, break each semicolon-separated statement into separate lines and avoid putting the with-open and write logic on the same line; also apply the same cleanup to the other flagged statement in the module near the second occurrence around the write/export path. Keep the behavior unchanged while making the control flow and assignments explicit in the affected functions.Source: Linters/SAST tools
experiments/conoir-spike/quorum/proof_a_receivable/src/main.nr-35-41 (1)
35-41: 🔒 Security & Privacy | 🟡 Minor | ⚡ Quick winCompare the canonical bytes directly.
be_bytes_to_field(canonical_id_bytes) == cidstill accepts any 32-byte BN254 representative ofcid, so the proof binds to a residue class rather than the exactbe32(canonical_id)message. Comparecid.to_be_bytes()tocanonical_id_bytes, or range-check the bytes before reducing.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/conoir-spike/quorum/proof_a_receivable/src/main.nr` around lines 35 - 41, The check in be_bytes_to_field and the proof logic still accepts any 32-byte BN254 representative of the same Field value instead of binding the proof to the exact canonical bytes. Update the verification path around canonical_id_bytes and cid so it compares cid.to_be_bytes() directly against canonical_id_bytes, or add a byte-level range/canonicality check before any reduction; keep the fix localized to the proof_a_receivable flow that currently uses be_bytes_to_field.
🧹 Nitpick comments (1)
experiments/conoir-spike/quorum/proof_a_receivable/run_proof_a.sh (1)
129-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the unused
SIGBADassignment.The negative test mutates the first byte via
FIRST/NEWFIRST;SIGBADis never consumed.Proposed cleanup
-SIGBAD=$(echo "$SIG" | sed -E 's/^\[([0-9]+),/["bad",/' ; true) # bump first signature byte by 1 (mod 256) to keep it a valid u8🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@experiments/conoir-spike/quorum/proof_a_receivable/run_proof_a.sh` at line 129, Remove the unused SIGBAD assignment from run_proof_a.sh; the negative test already mutates the signature through FIRST and NEWFIRST, so this extra shell variable is dead code. Delete the SIGBAD line and keep the existing SIG/SIG2 path intact, checking the surrounding mutation logic in the proof_a_receivable script to ensure no references to SIGBAD remain.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@experiments/conoir-spike/quorum/_mkroot/src/main.nr`:
- Around line 4-10: The current be_bytes_to_field helper collapses all 32 bytes
into a single Field value, which can make the trust-list leaf non-injective.
Update the membership leaf construction to keep P-256 coordinates separate by
splitting each coordinate into fixed-width limbs below the field modulus before
hashing, and apply this in the code path that builds the (x, y, role) leaf so
distinct signer keys cannot map to the same leaf.
In `@experiments/conoir-spike/quorum/adapters/fattura_adapter_object.json`:
- Around line 4-8: The `canonical_fields.debtor_id` value is being normalized as
a number, which strips leading zeros and can collapse distinct debtor
identifiers into the same `canonical_id`. Update the `fattura_adapter_object`
artifact so `debtor_id` is preserved in its fixed-format string or byte
representation, matching `_notes.field_derivation.debtor_cf`, and regenerate the
sample from that encoding. Also apply the same correction to the other affected
`canonical_fields` entries referenced in the comment.
In `@experiments/conoir-spike/quorum/adapters/fattura_adapter.py`:
- Around line 91-95: The `nargo execute` handling in `fattura_adapter.py` is
parsing stdout without checking whether the subprocess succeeded, so failures
can surface as a misleading `StopIteration` and hide stderr. Update the
`subprocess.run` call in the `nargo execute` flow to keep the full result
object, check its return code before searching for “Circuit output”, and if it
failed, surface/report stderr directly. Keep the fix localized around the code
that assigns `out` and extracts `line`/`inner` so the failure path is handled
before parsing.
- Around line 67-70: The canonical amount calculation in the invoice
normalization logic uses float conversion, which can produce unstable cents and
change the resulting canonical_id. Update the amount handling in
fattura_adapter.py where invoice_uuid, debtor_id, amount, and issue_date are
derived so that the amount is computed with Decimal instead of float, then
converted to cents deterministically before building the canonical fields. Keep
the existing normalization flow intact and adjust the invoice amount path
specifically to avoid binary floating-point rounding issues.
- Around line 30-56: parse_canonical_fields() currently parses untrusted
FatturaPA XML with xml.etree.ElementTree, so update it to use a hardened parser
from defusedxml.ElementTree instead. Keep the existing XML-walking logic in
_find() and _local() unchanged, but ensure all parsing in this module goes
through the defusedxml API. Also add the new defusedxml dependency in whatever
package management file this project uses so the import is available at runtime.
- Around line 60-69: The FatturaPA identifier parsing in fattura_adapter.py is
too strict because `numero` and `debtor_cf` may be alphanumeric, so the current
`int(numero)` and `int(debtor_cf)` conversions will reject valid invoices.
Update the logic around `invoice_uuid` and `debtor_id` to either validate and
fail fast with a clear error when only numeric fixtures are supported, or
replace the integer casts with a deterministic string-based encoding that
preserves alphanumeric values.
In `@experiments/conoir-spike/quorum/ARCHITECTURE.md`:
- Around line 55-60: Resolve the Proof A status mismatch by making the “Today
(stand-in)” note in ARCHITECTURE.md explicitly historical if the off-circuit
obligor check has been superseded, or update the later status table to match the
current implementation state. Align the wording around the Proof A / obligor
verification path so the description of admission, anchor consumption, and
signer_role checks is consistent with the table and clearly reflects the actual
trust boundary.
In `@experiments/conoir-spike/quorum/bound_aggregate/src/main.nr`:
- Around line 29-30: The circuit currently exposes accepted_line_role and
accepted_value_role as public inputs, which lets a prover supply arbitrary
matching roles unless they are constrained internally. Update the main circuit
logic to pin these values to the expected constants by enforcing insurer lines
use role 3 and valuers use role 4, using the existing accepted_line_role and
accepted_value_role symbols where they are declared or checked. Make sure the
constraint is applied inside the circuit itself rather than relying on external
verifier configuration.
- Around line 40-55: The aggregate check in bound_aggregate::main currently
accumulates the line values in a Field, which can wrap before the final
over-insured comparison. Change the running sum to a bounded integer type in the
loop over N_LINES, and add explicit overflow/range checks as values are added
before comparing the total against agreed_value. Keep the anchor and role
validation logic in place, and update the final comparison to use the bounded
total rather than casting from Field.
In `@experiments/conoir-spike/quorum/bound_receivables/src/main.nr`:
- Line 26: The proof currently trusts caller-supplied accepted_roles, so it only
shows the role is in whatever list the caller provides. Update the circuit
around the public inputs and checks in main.nr to pin the allowed roles inside
the proof logic itself, using the existing accepted_roles/ALLOW_LIST flow only
as a fixed constraint rather than a user-controlled policy. Make the
verification in the role-checking path reject any role outside the hardcoded
acceptable set so a permissive caller list cannot admit seller self-signatures.
In `@experiments/conoir-spike/quorum/p1_provenance/admission/admission.sh`:
- Around line 21-22: The admission script is only emitting the role/guarantee
and never uses COMMIT or NARGO, so it is not producing the role-bound anchor
artifact that downstream consumers expect. Update the admit flow in admission.sh
(especially the admit logic and any related output formatting) to invoke the
Nargo circuit using COMMIT and emit the resulting anchor, not just the role,
while preserving the existing guarantee output if needed. Make sure the script’s
final artifact matches the Layer-2 admission contract and that the role-bound
anchor is what gets printed or written for consumers.
In `@experiments/conoir-spike/quorum/proof_a_receivable/src/main.nr`:
- Around line 35-41: The trust-list leaf encoding in be_bytes_to_field and the
matching root builder logic is lossy because it reduces P-256 coordinates into
BN254 Field values before hashing. Update both proof_a_receivable/src/main.nr
and _mkroot/src/main.nr to hash a lossless byte/limb representation of
signer_pubkey_x/y instead of Field-reduced residues, then regenerate the trust
root so the root binds the exact 32-byte coordinates.
In `@experiments/conoir-spike/quorum/README.md`:
- Around line 15-17: The anchor definition in the quorum README is inconsistent
with the role-bound behavior used by commit_receivable and bound_receivables, so
update the Layer 1 schema text to explicitly include role in the anchor
alongside the existing anchor fields. Make the wording match the later paragraph
and ADAPTER.md so both derivations use the same role-bound anchor for a
receivable, and apply the same correction wherever the schema is repeated in the
referenced section.
In `@experiments/conoir-spike/quorum/run_bound_aggregate.sh`:
- Around line 18-22: The background job handling in run_bound_aggregate.sh is
using bare wait calls after launching generate-witness, build-proving-key, and
generate-proof, which can hide failures. Update the script to track each spawned
PID explicitly for those loops and wait on each PID individually so any failed
witness, proving-key, or proof job causes the script to fail. Keep the changes
localized around the generate-witness, build-proving-key, and generate-proof
command blocks.
In `@experiments/conoir-spike/quorum/run_bound_receivables.sh`:
- Around line 24-31: The background job handling in run_bound_receivables.sh is
too loose because bare wait can hide failures and let later steps proceed with
missing outputs. Update the witness, build-proving-key, and generate-proof
sections to capture each spawned PID in the existing loops and wait on those
specific PIDs after launch, so failures from generate-witness,
build-proving-key, and generate-proof are surfaced before verify runs.
In `@experiments/conoir-spike/quorum/run_quorum_scenarios.sh`:
- Around line 15-29: The prover helper in run_quorum_scenarios.sh writes to a
predictable /tmp/q.toml, which can be hijacked or corrupted. Update prover() to
create and use a unique temporary TOML file instead of a fixed path, and make
run() consume that generated path; keep the change localized to the prover() and
run() helpers so the temp file is safely created and cleaned up per invocation.
- Around line 29-38: The scenario helper in run_quorum_scenarios.sh currently
masks failures because run() always returns success after piping output through
grep with a fallback true. Update run() so it preserves the exit status of
./run_bound_receivables.sh and the scenario checks fail when the proof or runner
fails, while still filtering for the expected markers. Keep the change localized
to run() and the scenario invocations in the script so the expected outcomes are
asserted rather than hidden.
- Around line 4-5: The quorum scenario script is hard-coding a
developer-specific path and can silently continue from the wrong directory if cd
fails. Update the setup in run_quorum_scenarios.sh to resolve the script’s own
directory dynamically (using the script location rather than
$HOME/apertrue/...), and make the directory change fail fast so later commands
do not run from the caller’s directory. Use the existing script entrypoint
context around the Q/cd block to keep the fix localized.
---
Minor comments:
In `@experiments/conoir-spike/quorum/_mkroot/src/main.nr`:
- Around line 14-18: The Merkle path logic in the main proof routine branches on
`indices[i] == 0`, which allows any nonzero witness value to be treated as the
right branch. Update the `merkle_indices` handling in `main.nr` to explicitly
constrain each `indices[i]` to be binary before it is used in the loop, so only
0 or 1 are accepted in the `is_left` check and the `Poseidon2::hash` path
selection.
In `@experiments/conoir-spike/quorum/adapters/run_fattura_pipeline.sh`:
- Line 18: The pipeline in run_fattura_pipeline.sh is asserting the
fixture-specific CID even when XML is passed as a user-selected input, so custom
XML runs fail unnecessarily. Update the CID check around the proof setup logic
to only enforce obligor_true.cid when XML is the default fixture path, or
otherwise skip/branch the assertion based on the selected XML; use the XML
variable and the proof/fixture comparison block near the existing CID check to
locate the fix.
In `@experiments/conoir-spike/quorum/p1_provenance/p1_next.py`:
- Around line 55-58: Split the compound statements in the invoice PDF workflow
so Ruff no longer flags E701/E702: in the block around PdfReader, PdfWriter,
append, add_attachment, and the write call, break each semicolon-separated
statement into separate lines and avoid putting the with-open and write logic on
the same line; also apply the same cleanup to the other flagged statement in the
module near the second occurrence around the write/export path. Keep the
behavior unchanged while making the control flow and assignments explicit in the
affected functions.
In `@experiments/conoir-spike/quorum/p1_provenance/README.md`:
- Around line 46-49: The reproduction steps in the README are using the wrong
input file, so update the command in the C2PA/PDF workflow to use the existing
FatturaPA XML sample instead of invoice.txt. Make sure the command sequence
around the render/sign flow still points to the same sample artifact used by the
rest of this section, so the later C2PA and MPC steps remain reproducible.
In `@experiments/conoir-spike/quorum/proof_a_receivable/src/main.nr`:
- Around line 35-41: The check in be_bytes_to_field and the proof logic still
accepts any 32-byte BN254 representative of the same Field value instead of
binding the proof to the exact canonical bytes. Update the verification path
around canonical_id_bytes and cid so it compares cid.to_be_bytes() directly
against canonical_id_bytes, or add a byte-level range/canonicality check before
any reduction; keep the fix localized to the proof_a_receivable flow that
currently uses be_bytes_to_field.
---
Nitpick comments:
In `@experiments/conoir-spike/quorum/proof_a_receivable/run_proof_a.sh`:
- Line 129: Remove the unused SIGBAD assignment from run_proof_a.sh; the
negative test already mutates the signature through FIRST and NEWFIRST, so this
extra shell variable is dead code. Delete the SIGBAD line and keep the existing
SIG/SIG2 path intact, checking the surrounding mutation logic in the
proof_a_receivable script to ensure no references to SIGBAD remain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c5467eda-b076-4186-93e5-8efd72ac226f
⛔ Files ignored due to path filters (14)
experiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_leaf.pemis excluded by!**/*.pemexperiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_leaf.pubis excluded by!**/*.pubexperiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_root.pemis excluded by!**/*.pemexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_leaf.pemis excluded by!**/*.pemexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_leaf.pubis excluded by!**/*.pubexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_root.pemis excluded by!**/*.pemexperiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_leaf.pemis excluded by!**/*.pemexperiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_leaf.pubis excluded by!**/*.pubexperiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_root.pemis excluded by!**/*.pemexperiments/conoir-spike/quorum/p1_provenance/admission/certs/trust_store.pemis excluded by!**/*.pemexperiments/conoir-spike/quorum/p1_provenance/invoice.pdfis excluded by!**/*.pdfexperiments/conoir-spike/quorum/p1_provenance/invoice_c2pa.pdfis excluded by!**/*.pdfexperiments/conoir-spike/quorum/p1_provenance/invoice_quorum.pdfis excluded by!**/*.pdfexperiments/conoir-spike/quorum/p1_provenance/invoice_with_xml.pdfis excluded by!**/*.pdf
📒 Files selected for processing (57)
experiments/conoir-spike/quorum/.gitignoreexperiments/conoir-spike/quorum/ARCHITECTURE.mdexperiments/conoir-spike/quorum/README.mdexperiments/conoir-spike/quorum/_mkroot/Nargo.tomlexperiments/conoir-spike/quorum/_mkroot/Prover.tomlexperiments/conoir-spike/quorum/_mkroot/src/main.nrexperiments/conoir-spike/quorum/adapters/ADAPTER.mdexperiments/conoir-spike/quorum/adapters/fattura_adapter.pyexperiments/conoir-spike/quorum/adapters/fattura_adapter_object.jsonexperiments/conoir-spike/quorum/adapters/run_fattura_pipeline.shexperiments/conoir-spike/quorum/aggregate-README.mdexperiments/conoir-spike/quorum/bound_aggregate/Nargo.tomlexperiments/conoir-spike/quorum/bound_aggregate/Prover.tomlexperiments/conoir-spike/quorum/bound_aggregate/src/main.nrexperiments/conoir-spike/quorum/bound_receivables/Nargo.tomlexperiments/conoir-spike/quorum/bound_receivables/Prover.tomlexperiments/conoir-spike/quorum/bound_receivables/Prover_sdi.tomlexperiments/conoir-spike/quorum/bound_receivables/src/main.nrexperiments/conoir-spike/quorum/commit_aggregate/Nargo.tomlexperiments/conoir-spike/quorum/commit_aggregate/Prover.tomlexperiments/conoir-spike/quorum/commit_aggregate/src/main.nrexperiments/conoir-spike/quorum/commit_receivable/Nargo.tomlexperiments/conoir-spike/quorum/commit_receivable/Prover.tomlexperiments/conoir-spike/quorum/commit_receivable/Prover_a.tomlexperiments/conoir-spike/quorum/commit_receivable/Prover_sdi.tomlexperiments/conoir-spike/quorum/commit_receivable/src/main.nrexperiments/conoir-spike/quorum/p1_provenance/IT01234567890_FPR01.xmlexperiments/conoir-spike/quorum/p1_provenance/README.mdexperiments/conoir-spike/quorum/p1_provenance/admission/admission.shexperiments/conoir-spike/quorum/p1_provenance/admission/certs/_chk.cidexperiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_leaf.csrexperiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_leaf.extexperiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_root.srlexperiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_true.cidexperiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_true.sigexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_infl.cidexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_infl.sigexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_leaf.csrexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_leaf.extexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_root.srlexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_true.cidexperiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_true.sigexperiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_leaf.csrexperiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_leaf.extexperiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_root.srlexperiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_true.cidexperiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_true.sigexperiments/conoir-spike/quorum/p1_provenance/c2pie_schema.jsonexperiments/conoir-spike/quorum/p1_provenance/manifest_receivable.c2patool.jsonexperiments/conoir-spike/quorum/p1_provenance/p1_next.pyexperiments/conoir-spike/quorum/proof_a_receivable/Nargo.tomlexperiments/conoir-spike/quorum/proof_a_receivable/Prover.tomlexperiments/conoir-spike/quorum/proof_a_receivable/run_proof_a.shexperiments/conoir-spike/quorum/proof_a_receivable/src/main.nrexperiments/conoir-spike/quorum/run_bound_aggregate.shexperiments/conoir-spike/quorum/run_bound_receivables.shexperiments/conoir-spike/quorum/run_quorum_scenarios.sh
✅ Files skipped from review due to trivial changes (14)
- experiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_leaf.ext
- experiments/conoir-spike/quorum/.gitignore
- experiments/conoir-spike/quorum/_mkroot/Nargo.toml
- experiments/conoir-spike/quorum/commit_receivable/Prover_sdi.toml
- experiments/conoir-spike/quorum/proof_a_receivable/Nargo.toml
- experiments/conoir-spike/quorum/commit_receivable/Prover_a.toml
- experiments/conoir-spike/quorum/commit_receivable/Prover.toml
- experiments/conoir-spike/quorum/commit_aggregate/Nargo.toml
- experiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_root.srl
- experiments/conoir-spike/quorum/commit_aggregate/Prover.toml
- experiments/conoir-spike/quorum/p1_provenance/admission/certs/sdi_infl.cid
- experiments/conoir-spike/quorum/p1_provenance/admission/certs/obligor_true.cid
- experiments/conoir-spike/quorum/commit_receivable/Nargo.toml
- experiments/conoir-spike/quorum/p1_provenance/admission/certs/seller_true.sig
Comment on lines
+4
to
+10
| fn be_bytes_to_field(b: [u8; 32]) -> Field { | ||
| let mut acc: Field = 0; | ||
| for i in 0..32 { | ||
| acc = acc * 256 + b[i] as Field; | ||
| } | ||
| acc | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🌐 Web query:
What field modulus does Noir's Field use on BN254, and can an arbitrary 32-byte secp256r1/P-256 coordinate be represented without modulo reduction?
💡 Result:
The field modulus for Noir's native Field type when using the BN254 backend is 21888242871839275222246405745257275088548364400416034343698204186575808495617 [1]. This prime is a 254-bit integer [2]. Regarding the representation of an arbitrary 32-byte secp256r1 (P-256) coordinate: It cannot be represented as a single Noir Field element without modulo reduction if the coordinate value exceeds the BN254 field modulus [2]. In Noir, the Field type represents elements of the underlying curve's finite field; if an input (such as a 32-byte array) is converted to a Field element using standard conversion methods (like from_be_bytes or from_le_bytes), any value greater than or equal to the field modulus will silently wrap around via modulo reduction [2]. To represent a 32-byte value without losing information to modulo reduction, you must avoid using the Field type for storage or arithmetic [2]. Instead, you should maintain such data as a byte array (e.g., [u8; 32]) or decompose it into smaller integer types (e.g., u64 or u128) that fit within the native range without overflow [2]. Note that when working with cryptographic primitives like secp256r1, Noir's standard library functions (such as std::ecdsa_secp256r1::verify_signature) typically accept these coordinates as [u8; 32] arrays to ensure proper handling of the 256-bit data [3][4].
Citations:
- 1: noir-lang/noir@5bbce79
- 2: https://noir-lang.org/docs/noir/concepts/data_types/fields
- 3: https://github.com/noir-lang/noir/blob/master/noir_stdlib/src/ecdsa_secp256r1.nr
- 4: https://noir-lang.org/docs/noir/standard_library/cryptographic_primitives/ecdsa_sig_verification
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files experiments/conoir-spike/quorum/_mkroot/src/main.nr
wc -l experiments/conoir-spike/quorum/_mkroot/src/main.nr
cat -n experiments/conoir-spike/quorum/_mkroot/src/main.nrRepository: Apertrue/circuits
Length of output: 1536
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'be_bytes_to_field|signer_pubkey_x|signer_pubkey_y|compute_merkle_root|Poseidon2::hash\(\[x_f, y_f, signer_role\]' \
.
printf '\n---\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' \
'P-256|secp256r1|merkle_root|merkle path|signer_role|pubkey' \
experiments/conoir-spike/quorum/_mkroot/srcRepository: Apertrue/circuits
Length of output: 9170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
wc -l experiments/conoir-spike/quorum/proof_a_receivable/src/main.nr experiments/conoir-spike/quorum/adapters/ADAPTER.md
printf '\n--- proof_a_receivable/src/main.nr ---\n'
cat -n experiments/conoir-spike/quorum/proof_a_receivable/src/main.nr | sed -n '1,130p'
printf '\n--- ADAPTER.md ---\n'
cat -n experiments/conoir-spike/quorum/adapters/ADAPTER.md | sed -n '1,120p'Repository: Apertrue/circuits
Length of output: 12493
Avoid collapsing P-256 coordinates into one Field. The BN254 reduction makes the (x, y, role) trust-list leaf non-injective, so distinct signer keys can map to the same membership leaf. Split each coordinate into fixed-width limbs below the field modulus before hashing.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/_mkroot/src/main.nr` around lines 4 - 10, The
current be_bytes_to_field helper collapses all 32 bytes into a single Field
value, which can make the trust-list leaf non-injective. Update the membership
leaf construction to keep P-256 coordinates separate by splitting each
coordinate into fixed-width limbs below the field modulus before hashing, and
apply this in the code path that builds the (x, y, role) leaf so distinct signer
keys cannot map to the same leaf.
Comment on lines
+4
to
+8
| "canonical_fields": { | ||
| "invoice_uuid": 1234567890000123, | ||
| "debtor_id": 9876543210, | ||
| "amount": 500, | ||
| "issue_date": 20141218 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve debtor identifiers as strings.
This sample already drops a leading zero: _notes.field_derivation.debtor_cf is "09876543210" but canonical_fields.debtor_id is 9876543210. Numeric normalization will make distinct identifiers hash to the same canonical_id, which can create false duplicate/non-duplicate decisions across parties.
Keep fixed-format IDs in their canonical string/byte form and regenerate this artifact from that encoding.
Also applies to: 189-193
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/adapters/fattura_adapter_object.json` around
lines 4 - 8, The `canonical_fields.debtor_id` value is being normalized as a
number, which strips leading zeros and can collapse distinct debtor identifiers
into the same `canonical_id`. Update the `fattura_adapter_object` artifact so
`debtor_id` is preserved in its fixed-format string or byte representation,
matching `_notes.field_derivation.debtor_cf`, and regenerate the sample from
that encoding. Also apply the same correction to the other affected
`canonical_fields` entries referenced in the comment.
Comment on lines
+30
to
+56
| import argparse, binascii, json, os, subprocess, sys, xml.etree.ElementTree as ET | ||
|
|
||
| HERE = os.path.dirname(os.path.abspath(__file__)) | ||
| QUORUM = os.path.dirname(HERE) | ||
| NARGO = os.environ.get("NARGO", os.path.expanduser("~/.nargo/bin/nargo")) | ||
| KEYDIR = os.path.join(HERE, "keys") | ||
| N_P256 = 0xFFFFFFFF00000000FFFFFFFFFFFFFFFFBCE6FAADA7179E84F3B9CAC2FC632551 | ||
|
|
||
| def _local(tag): | ||
| return tag.split('}', 1)[-1] # strip XML namespace | ||
|
|
||
| def _find(root, *path): | ||
| """Walk by LOCAL element names (namespace-agnostic), return first text match.""" | ||
| nodes = [root] | ||
| for name in path: | ||
| nxt = [] | ||
| for n in nodes: | ||
| for c in n: | ||
| if _local(c.tag) == name: | ||
| nxt.append(c) | ||
| nodes = nxt | ||
| if not nodes: | ||
| return None | ||
| return nodes[0].text.strip() if nodes[0].text else None | ||
|
|
||
| def parse_canonical_fields(xml_path): | ||
| root = ET.parse(xml_path).getroot() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- fattura_adapter.py outline ---'
ast-grep outline experiments/conoir-spike/quorum/adapters/fattura_adapter.py --view expanded || true
echo '--- search for defusedxml ---'
rg -n "defusedxml|ElementTree as ET|xml\.etree\.ElementTree" -S . || true
echo '--- dependency/manifests ---'
git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|Pipfile|poetry\.lock|setup\.py|setup\.cfg|uv\.lock|flake\.lock)$' || trueRepository: Apertrue/circuits
Length of output: 1012
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the adapter implementation around the XML parse and any downstream assumptions.
file='experiments/conoir-spike/quorum/adapters/fattura_adapter.py'
echo "--- $file (selected lines) ---"
nl -ba "$file" | sed -n '1,180p'
echo '--- dependency manifests mentioning defusedxml ---'
for f in $(git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|Pipfile|poetry\.lock|setup\.py|setup\.cfg|uv\.lock|flake\.lock)$'); do
echo "### $f"
rg -n "defusedxml" "$f" || true
doneRepository: Apertrue/circuits
Length of output: 280
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='experiments/conoir-spike/quorum/adapters/fattura_adapter.py'
echo '--- adapter excerpt ---'
sed -n '55,130p' "$file"
echo '--- repository Python dependency files ---'
git ls-files | rg '(^|/)(pyproject\.toml|requirements.*\.txt|Pipfile|poetry\.lock|setup\.py|setup\.cfg|uv\.lock|flake\.lock)$' || trueRepository: Apertrue/circuits
Length of output: 4279
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file='experiments/conoir-spike/quorum/adapters/fattura_adapter.py'
sed -n '130,220p' "$file"Repository: Apertrue/circuits
Length of output: 2780
Use a hardened XML parser for invoice input. parse_canonical_fields() reads user-supplied FatturaPA XML directly with xml.etree.ElementTree; switch to defusedxml.ElementTree to harden parsing of untrusted XML, and add the dependency where this project manages Python packages.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 56-56: Using xml to parse untrusted data is known to be vulnerable to XML attacks; use defusedxml equivalents
(S314)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/adapters/fattura_adapter.py` around lines 30
- 56, parse_canonical_fields() currently parses untrusted FatturaPA XML with
xml.etree.ElementTree, so update it to use a hardened parser from
defusedxml.ElementTree instead. Keep the existing XML-walking logic in _find()
and _local() unchanged, but ensure all parsing in this module goes through the
defusedxml API. Also add the new defusedxml dependency in whatever package
management file this project uses so the import is available at runtime.
Source: Linters/SAST tools
Comment on lines
+60
to
+69
| supplier_vat = _find(hdr, "CedentePrestatore", "DatiAnagrafici", "IdFiscaleIVA", "IdCodice") | ||
| debtor_cf = _find(hdr, "CessionarioCommittente", "DatiAnagrafici", "CodiceFiscale") | ||
| numero = _find(body, "DatiGenerali", "DatiGeneraliDocumento", "Numero") | ||
| data = _find(body, "DatiGenerali", "DatiGeneraliDocumento", "Data") | ||
| imponibile = _find(body, "DatiBeniServizi", "DatiRiepilogo", "ImponibileImporto") | ||
| divisa = _find(body, "DatiGenerali", "DatiGeneraliDocumento", "Divisa") | ||
|
|
||
| invoice_uuid = int(supplier_vat) * 1_000_000 + int(numero) | ||
| debtor_id = int(debtor_cf) # int() drops leading zeros | ||
| amount = round(float(imponibile) * 100) # EUR -> cents |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Italian FatturaPA XML NumeroandCodiceFiscale field formats: can they contain non-numeric characters?
💡 Result:
In the Italian FatturaPA XML format, the Numero (invoice number) and CodiceFiscale fields have distinct requirements regarding character types: Numero (Field 2.1.1.4): This field allows for alphanumeric characters [1][2]. While it is generally used for invoice numbering, it must include at least one numeric character to be valid according to technical specifications [3][4]. The field has a maximum length of 20 characters [3][5][4]. CodiceFiscale (Various fields, e.g., 1.2.1.2, 1.3.1.2, 1.4.1.2): This field is strictly alphanumeric and must contain between 11 and 16 characters [6][7]. It is used to hold the Italian tax code (Codice Fiscale) for various entities (e.g., sender, recipient, representative) [8][7][9]. In summary, both fields can (and often do) contain non-numeric characters, but they are subject to specific validation rules—the Numero field requires at least one digit [3][4], and the CodiceFiscale field must adhere to the specific structure and length constraints of a valid Italian tax code [6][7].
Citations:
- 1: https://www.money.it/IMG/pdf/suggerimenti_compilazione_fatturapa_v1.1.pdf
- 2: https://www.eu-einvoicing.com/italy/formats/fatturaPA-invoice-format-italy
- 3: https://www.agenziaentrate.gov.it/portale/documents/20143/2931841/Table+view+B2B+Ordinary+invoice.xls/53388c61-951e-089a-1771-7ebd0ed662f4
- 4: https://www.fatturapa.gov.it/export/documenti/Technical-Rules-for-European-Invoicing-v2.4.pdf
- 5: https://www.fatturapa.gov.it/export/documenti/Specifiche_tecniche_del_formato_FatturaPA_v1.2.1_EN.pdf
- 6: https://www.agenziaentrate.gov.it/portale/documents/20143/296761/Provvedimento%2005022018%20dati%20fattura_SpecTecRegoleCompilazioneDATIFATTURA%20v1.2.pdf/95645a32-3c27-d303-0a41-d3d62142dc60
- 7: https://www.masaf.gov.it/flex/files/d/a/2/D.a5f4639cbb69f9d711bf/7_Formato_FatturaPA_tabellare_1.0.pdf
- 8: https://www.agenziaentrate.gov.it/portale/documents/20143/4838631/AllegatoC.pdf/c6687369-3a8d-8ed9-14ac-b522fcad661c
- 9: https://www.agenziaentrate.gov.it/portale/documents/20143/296471/Provvedimento%2021%20dicembre%202018_ALLEGATO-B-DATI-FISCALI-FATTURA-XML-21.12.18-def.pdf/c774d5a9-3459-ae1b-f671-7be363acea87
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '=== file outline ===\n'
ast-grep outline experiments/conoir-spike/quorum/adapters/fattura_adapter.py --view expanded || true
printf '\n=== relevant lines ===\n'
sed -n '1,140p' experiments/conoir-spike/quorum/adapters/fattura_adapter.py | cat -n
printf '\n=== search for related parsing/tests ===\n'
rg -n "supplier_vat|debtor_cf|invoice_uuid|CodiceFiscale|Numero|fattura_adapter" experiments/conoir-spike -g '!**/__pycache__/**' || trueRepository: Apertrue/circuits
Length of output: 16260
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('experiments/conoir-spike/quorum/adapters/fattura_adapter.py')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 50 <= i <= 90:
print(f"{i:4}: {line}")
PYRepository: Apertrue/circuits
Length of output: 2455
Reject alphanumeric FatturaPA identifiers up front. Numero and CodiceFiscale can legally contain letters, so int(numero) / int(debtor_cf) will reject valid XML. If this adapter only supports numeric fixtures, fail fast with a clear validation error; otherwise switch to a deterministic string encoding.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/adapters/fattura_adapter.py` around lines 60
- 69, The FatturaPA identifier parsing in fattura_adapter.py is too strict
because `numero` and `debtor_cf` may be alphanumeric, so the current
`int(numero)` and `int(debtor_cf)` conversions will reject valid invoices.
Update the logic around `invoice_uuid` and `debtor_id` to either validate and
fail fast with a clear error when only numeric fixtures are supported, or
replace the integer casts with a deterministic string-based encoding that
preserves alphanumeric values.
Comment on lines
+67
to
+70
| invoice_uuid = int(supplier_vat) * 1_000_000 + int(numero) | ||
| debtor_id = int(debtor_cf) # int() drops leading zeros | ||
| amount = round(float(imponibile) * 100) # EUR -> cents | ||
| issue_date = int(data.replace("-", "")) # 2014-12-18 -> 20141218 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Avoid binary floats for canonical monetary fields.
round(float(imponibile) * 100) can produce unstable cents and therefore a different canonical_id. Use Decimal for invoice amounts.
Proposed fix
-import argparse, binascii, json, os, subprocess, sys
+import argparse, binascii, json, os, subprocess, sys
+from decimal import Decimal, ROUND_HALF_UP
@@
- amount = round(float(imponibile) * 100) # EUR -> cents
+ amount = int((Decimal(imponibile) * 100).quantize(Decimal("1"), rounding=ROUND_HALF_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.
Suggested change
| invoice_uuid = int(supplier_vat) * 1_000_000 + int(numero) | |
| debtor_id = int(debtor_cf) # int() drops leading zeros | |
| amount = round(float(imponibile) * 100) # EUR -> cents | |
| issue_date = int(data.replace("-", "")) # 2014-12-18 -> 20141218 | |
| invoice_uuid = int(supplier_vat) * 1_000_000 + int(numero) | |
| debtor_id = int(debtor_cf) # int() drops leading zeros | |
| amount = int((Decimal(imponibile) * 100).quantize(Decimal("1"), rounding=ROUND_HALF_UP)) # EUR -> cents | |
| issue_date = int(data.replace("-", "")) # 2014-12-18 -> 20141218 |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/adapters/fattura_adapter.py` around lines 67
- 70, The canonical amount calculation in the invoice normalization logic uses
float conversion, which can produce unstable cents and change the resulting
canonical_id. Update the amount handling in fattura_adapter.py where
invoice_uuid, debtor_id, amount, and issue_date are derived so that the amount
is computed with Decimal instead of float, then converted to cents
deterministically before building the canonical fields. Keep the existing
normalization flow intact and adjust the invoice amount path specifically to
avoid binary floating-point rounding issues.
Comment on lines
+18
to
+22
| for i in 0 1 2; do "$CO" generate-witness --input "$TV/Prover.toml.$i.shared" --circuit "$J" --protocol REP3 --config "$CFG/party$((i+1)).toml" --out "$TV/witness.$i.shared" & done; wait | ||
| for i in 0 1 2; do "$CO" build-proving-key --witness "$TV/witness.$i.shared" --circuit "$J" --protocol REP3 --config "$CFG/party$((i+1)).toml" --out "$TV/pk.$i.shared" --crs "$CRS1" & done; wait | ||
| "$CO" create-vk --circuit "$J" --crs "$CRS1" --hasher keccak --vk "$TV/vk" | ||
| "$CO" generate-proof --proving-key "$TV/pk.0.shared" --protocol REP3 --hasher keccak --config "$CFG/party1.toml" --crs "$CRS1" --out "$TV/proof.0.proof" --vk "$TV/vk" --public-input "$TV/public_input" & | ||
| for i in 1 2; do "$CO" generate-proof --proving-key "$TV/pk.$i.shared" --protocol REP3 --hasher keccak --config "$CFG/party$((i+1)).toml" --crs "$CRS1" --out "$TV/proof.$i.proof" --vk "$TV/vk" & done; wait |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the relevant script region and surrounding settings
echo "== File: experiments/conoir-spike/quorum/run_bound_aggregate.sh =="
wc -l experiments/conoir-spike/quorum/run_bound_aggregate.sh
cat -n experiments/conoir-spike/quorum/run_bound_aggregate.sh | sed -n '1,120p'
echo
echo "== Search for job-handling patterns in nearby scripts =="
rg -n --no-heading 'wait|pids=|\$\!' experiments/conoir-spike/quorum -g '*.sh'
echo
echo "== Bash wait / set -e probe =="
bash -lc 'set -e; (sleep 0.1; exit 7) & pid=$!; wait $pid; echo "after-wait status=$?"' || echo "probe-1 exited with $?"
bash -lc 'set -e; (sleep 0.1; exit 7) & (sleep 0.1; exit 0) & wait; echo "after-bare-wait status=$?"' || echo "probe-2 exited with $?"
bash -lc 'set -e; (sleep 0.1; exit 7) & (sleep 0.1; exit 0) & wait; echo "this should not print"; exit 0' || echo "probe-3 exited with $?"Repository: Apertrue/circuits
Length of output: 2628
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== File: experiments/conoir-spike/quorum/run_bound_aggregate.sh =="
wc -l experiments/conoir-spike/quorum/run_bound_aggregate.sh
cat -n experiments/conoir-spike/quorum/run_bound_aggregate.sh | sed -n '1,140p'
echo
echo "== Search for job-handling patterns in nearby scripts =="
rg -n --no-heading 'wait|pids=|\$\!' experiments/conoir-spike/quorum -g '*.sh'
echo
echo "== Bash wait / set -e probe =="
bash -lc 'set -e; (sleep 0.1; exit 7) & pid=$!; wait $pid; echo "after-wait status=$?"' || echo "probe-1 exited with $?"
bash -lc 'set -e; (sleep 0.1; exit 7) & (sleep 0.1; exit 0) & wait; echo "after-bare-wait status=$?"' || echo "probe-2 exited with $?"
bash -lc 'set -e; (sleep 0.1; exit 7) & (sleep 0.1; exit 0) & wait; echo "this should not print"; exit 0' || echo "probe-3 exited with $?"Repository: Apertrue/circuits
Length of output: 2628
🏁 Script executed:
bash -lc '
set -e
echo "case 1: one fail, one succeed"
(bash -lc "sleep 0.1; exit 7") &
p1=$!
(bash -lc "sleep 0.2; exit 0") &
p2=$!
wait
echo "survived bare wait"
' ; echo "rc=$?"
bash -lc '
set -e
echo "case 2: explicit PID wait on a failing child"
(bash -lc "sleep 0.1; exit 7") &
p1=$!
wait "$p1"
echo "survived pid wait"
' ; echo "rc=$?"Repository: Apertrue/circuits
Length of output: 754
Wait on each background PID explicitly. A bare wait can return success even if one of the witness/proving-key/proof jobs failed, so the script can continue with incomplete artifacts and report a false success.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/run_bound_aggregate.sh` around lines 18 - 22,
The background job handling in run_bound_aggregate.sh is using bare wait calls
after launching generate-witness, build-proving-key, and generate-proof, which
can hide failures. Update the script to track each spawned PID explicitly for
those loops and wait on each PID individually so any failed witness,
proving-key, or proof job causes the script to fail. Keep the changes localized
around the generate-witness, build-proving-key, and generate-proof command
blocks.
Comment on lines
+24
to
+31
| for i in 0 1 2; do "$CO" generate-witness --input "$TV/Prover.toml.$i.shared" --circuit "$J" --protocol REP3 --config "$CFG/party$((i+1)).toml" --out "$TV/witness.$i.shared" & done; wait | ||
| echo "### 3. build-proving-key (3 parties) ###" | ||
| for i in 0 1 2; do "$CO" build-proving-key --witness "$TV/witness.$i.shared" --circuit "$J" --protocol REP3 --config "$CFG/party$((i+1)).toml" --out "$TV/pk.$i.shared" --crs "$CRS1" & done; wait | ||
| echo "### 4. create-vk ###" | ||
| "$CO" create-vk --circuit "$J" --crs "$CRS1" --hasher keccak --vk "$TV/vk" | ||
| echo "### 5. generate-proof (3 parties) ###" | ||
| "$CO" generate-proof --proving-key "$TV/pk.0.shared" --protocol REP3 --hasher keccak --config "$CFG/party1.toml" --crs "$CRS1" --out "$TV/proof.0.proof" --vk "$TV/vk" --public-input "$TV/public_input" & | ||
| for i in 1 2; do "$CO" generate-proof --proving-key "$TV/pk.$i.shared" --protocol REP3 --hasher keccak --config "$CFG/party$((i+1)).toml" --crs "$CRS1" --out "$TV/proof.$i.proof" --vk "$TV/vk" & done; wait |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the script around the reported lines.
nl -ba experiments/conoir-spike/quorum/run_bound_receivables.sh | sed -n '1,120p'
# Check bash's built-in documentation for wait semantics.
bash -lc 'help wait' 2>/dev/null || true
# Probe bare `wait` behavior with successful and failing background jobs.
python3 - <<'PY'
import subprocess, textwrap, tempfile, os, json, sys
script = r'''
set -e
(false) &
(true) &
wait
echo "after wait"
'''
p = subprocess.run(["bash", "-lc", script], capture_output=True, text=True)
print("exit:", p.returncode)
print("stdout:", p.stdout)
print("stderr:", p.stderr)
script2 = r'''
set -e
(false) &
pid1=$!
(true) &
pid2=$!
wait "$pid1"
echo "after wait pid1"
'''
p2 = subprocess.run(["bash", "-lc", script2], capture_output=True, text=True)
print("exit2:", p2.returncode)
print("stdout2:", p2.stdout)
print("stderr2:", p2.stderr)
PYRepository: Apertrue/circuits
Length of output: 195
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the target lines with line numbers using cat instead of nl.
cat -n experiments/conoir-spike/quorum/run_bound_receivables.sh | sed -n '1,120p'
# Check bash wait semantics with a failing background job and bare wait.
python3 - <<'PY'
import subprocess
cases = {
"bare_wait_with_one_failure": r'''
set -e
(false) &
(true) &
wait
echo "after wait"
''',
"pid_wait_with_one_failure": r'''
set -e
(false) &
pid1=$!
(true) &
pid2=$!
wait "$pid1"
echo "after wait pid1"
''',
}
for name, script in cases.items():
p = subprocess.run(["bash", "-lc", script], capture_output=True, text=True)
print(f"== {name} ==")
print("exit:", p.returncode)
print("stdout:", p.stdout.strip())
print("stderr:", p.stderr.strip())
PYRepository: Apertrue/circuits
Length of output: 3939
Track each background PID instead of a bare wait.
A plain wait can still return success here even if one of the witness/proving-key/proof jobs failed, so the script can reach verify with incomplete outputs. Use per-job wait "$pid" in all three loops.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/run_bound_receivables.sh` around lines 24 -
31, The background job handling in run_bound_receivables.sh is too loose because
bare wait can hide failures and let later steps proceed with missing outputs.
Update the witness, build-proving-key, and generate-proof sections to capture
each spawned PID in the existing loops and wait on those specific PIDs after
launch, so failures from generate-witness, build-proving-key, and generate-proof
are surfaced before verify runs.
Comment on lines
+4
to
+5
| Q="$HOME/apertrue/circuits/experiments/conoir-spike/quorum" | ||
| cd "$Q" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Resolve the script directory instead of hard-coding a developer path.
If cd "$Q" fails, the script continues from the caller’s directory and the later || true can hide the failure.
Proposed fix
#!/usr/bin/env bash
+set -euo pipefail
# Apertrue Quorum -- the two-signer thesis, end to end under MPC.
# Anchors come from Layer-2 admission (p1_provenance/admission/admission.sh).
-Q="$HOME/apertrue/circuits/experiments/conoir-spike/quorum"
-cd "$Q"
+Q="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)"
+cd "$Q" || exit 1📝 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.
Suggested change
| Q="$HOME/apertrue/circuits/experiments/conoir-spike/quorum" | |
| cd "$Q" | |
| set -euo pipefail | |
| Q="$(cd -- "$(dirname -- "${BASH_SOURCE[0]}")" && pwd)" | |
| cd "$Q" || exit 1 |
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 5-5: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/run_quorum_scenarios.sh` around lines 4 - 5,
The quorum scenario script is hard-coding a developer-specific path and can
silently continue from the wrong directory if cd fails. Update the setup in
run_quorum_scenarios.sh to resolve the script’s own directory dynamically (using
the script location rather than $HOME/apertrue/...), and make the directory
change fail fast so later commands do not run from the caller’s directory. Use
the existing script entrypoint context around the Q/cd block to keep the fix
localized.
Source: Linters/SAST tools
Comment on lines
+15
to
+29
| prover() { # anchor role amount financed_csv -> /tmp/q.toml | ||
| cat > /tmp/q.toml <<TOML | ||
| candidate_anchor = "$1" | ||
| signer_role = "$2" | ||
| accepted_roles = ["1","2"] | ||
| invoice_uuid = "1234567890000123" | ||
| debtor_id = "9876543210" | ||
| amount = "$3" | ||
| issue_date = "20141218" | ||
| salt = "0x7777777777777777" | ||
| financed = [ $4 ] | ||
| TOML | ||
| } | ||
|
|
||
| run() { ./run_bound_receivables.sh /tmp/q.toml 2>&1 | grep -iE "verified|verification failed|=> already_financed|allow-list|opening failed" || true; } |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Use a unique temp file for the generated prover input.
/tmp/q.toml is predictable; a local symlink can hijack the write or cause unintended file corruption.
Proposed fix
+TMP_Q="$(mktemp "${TMPDIR:-/tmp}/quorum.XXXXXX.toml")"
+trap 'rm -f "$TMP_Q"' EXIT
+
prover() { # anchor role amount financed_csv -> /tmp/q.toml
- cat > /tmp/q.toml <<TOML
+ cat > "$TMP_Q" <<TOML
candidate_anchor = "$1"
signer_role = "$2"
accepted_roles = ["1","2"]
@@
TOML
}
-run() { ./run_bound_receivables.sh /tmp/q.toml 2>&1 | grep -iE "verified|verification failed|=> already_financed|allow-list|opening failed" || true; }
+run() { ./run_bound_receivables.sh "$TMP_Q" 2>&1 | grep -iE "verified|verification failed|=> already_financed|allow-list|opening failed" || true; }
@@
-rm -f /tmp/q.tomlAlso applies to: 39-39
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 15-15: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/q.toml
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
[warning] 28-28: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/q.toml
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/run_quorum_scenarios.sh` around lines 15 -
29, The prover helper in run_quorum_scenarios.sh writes to a predictable
/tmp/q.toml, which can be hijacked or corrupted. Update prover() to create and
use a unique temporary TOML file instead of a fixed path, and make run() consume
that generated path; keep the change localized to the prover() and run() helpers
so the temp file is safely created and cleaned up per invocation.
Source: Linters/SAST tools
Comment on lines
+29
to
+38
| run() { ./run_bound_receivables.sh /tmp/q.toml 2>&1 | grep -iE "verified|verification failed|=> already_financed|allow-list|opening failed" || true; } | ||
|
|
||
| echo "########## 1) OBLIGOR, true receivable, not financed (expect 0, owed) ##########" | ||
| prover "$OBL" 2 500 "$CLEAN"; run | ||
| echo "########## 2) OBLIGOR, true receivable, DOUBLE-FINANCED (expect 1, owed) ##########" | ||
| prover "$OBL" 2 500 "$CLEAN5,$CID_TRUE"; run | ||
| echo "########## 3) SdI, INFLATED invoice seller cleared, not financed (expect 0, cleared-NOT-owed = THE GAP) ##########" | ||
| prover "$SDI_INFL" 1 5000 "$CLEAN"; run | ||
| echo "########## 4) SELLER self-signed (role 3) -> MPC allow-list rejects (defense in depth) ##########" | ||
| prover "$SELLER" 3 500 "$CLEAN"; run |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the expected scenario outcomes instead of masking failures.
run() always exits successfully, so a failed proof, missing output marker, or broken runner can still make the scenario script appear green.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 38-38: Writing to or reading from a hardcoded, predictable path under /tmp is vulnerable to symlink and TOCTOU attacks: a local attacker can pre-create the file (or a symlink pointing elsewhere) and hijack or corrupt the contents. Generate a unique, unpredictable temporary file with mktemp instead, e.g. tmpfile="$(mktemp)" (or mktemp -d for directories) and reference "$tmpfile".
Context: /tmp/q.toml
Note: [CWE-377] Insecure Temporary File.
(predictable-tmp-file-bash)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/run_quorum_scenarios.sh` around lines 29 -
38, The scenario helper in run_quorum_scenarios.sh currently masks failures
because run() always returns success after piping output through grep with a
fallback true. Update run() so it preserves the exit status of
./run_bound_receivables.sh and the scenario checks fail when the proof or runner
fails, while still filtering for the expected markers. Keep the change localized
to run() and the scenario invocations in the script so the expected outcomes are
asserted rather than hidden.
Parallel to the FatturaPA native-rail adapter, this proves the C2PA path where C2PA is the load-bearing carrier (an unstructured warehouse receipt with no clearance rail): - warehouse_adapter.py: wrap (c2pie embeds an org.apertrue.quorum.collateral custom assertion carrying canonical fields + operator pubkey + raw-ECDSA- over-canonical_id signature, hash-bound via c2pa.hash.data) and read (c2patool extracts the assertion; canonical_id re-derived from recovered fields, matches). - proves the SAME pipeline: C2PA doc -> Proof A (in-circuit ECDSA verify + role-bound trust-list membership) -> anchor == expected -> bound_receivables false->true. run_warehouse_pipeline.sh (idempotent). - ADAPTER.md: new C2PA-document adapter section; C2PA = carrier, trust = the in-circuit-verified signature. Now both adapter legs are demonstrated end-to-end: FatturaPA (native rail, no C2PA) and warehouse receipt (C2PA carrier). Stand-in keys gitignored. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@experiments/conoir-spike/quorum/adapters/run_warehouse_pipeline.sh`:
- Around line 39-42: Remove the hardcoded machine-specific Python candidate from
the interpreter discovery loop in run_warehouse_pipeline.sh. Update the cand
list in the startup logic to rely only on portable locations such as C2PIE_PY,
$HERE/.c2pie_venv/bin/python3, and $QUORUM/p1_provenance/venv/bin/python3, so
the script no longer references a local scratchpad path tied to one machine.
- Around line 157-158: The “double” fixture is using a different identifier in
financed than the anchor passed into bound_receivables, so the membership-hit
case is not actually exercising the intended path. Update the positive br_run
fixture so the financed entry uses the same proven anchor value as
candidate_anchor="$PROVEN_ANCHOR" instead of $CANONICAL_ID, keeping the rest of
the test data unchanged. Verify the “clean” case still uses a non-matching ID
and that the “double” case now matches the anchor used by bound_receivables.
In `@experiments/conoir-spike/quorum/adapters/warehouse_adapter.py`:
- Around line 76-80: Handle `nargo execute` failures in `warehouse_adapter.py`
before parsing stdout. In the code path that runs `subprocess.run` and then
searches for “Circuit output” in `out.splitlines()`, capture the full result
object instead of only `.stdout`, check the return code, and surface stderr or a
descriptive exception from the failed `nargo execute` call before attempting to
parse output. Update the parsing logic around the `inner`/`cid, anchor`
extraction so it only runs after a successful subprocess result and emits a
clearer error if the expected circuit output line is missing.
- Around line 180-182: Resolve the hard-coded cupsfilter path in the
warehouse_adapter subprocess call by making the binary lookup configurable
instead of always using /usr/sbin/cupsfilter. Update the logic around the
subprocess.run invocation in the adapter to resolve cupsfilter from $PATH by
default, and allow an environment override so deployments with the binary in a
different location still work. Keep the change localized to the existing adapter
flow that builds raw and writes out_pdf.
- Around line 263-272: read_manifest currently accepts the active_manifest and
collateral assertion without confirming the C2PA validation result, so tampered
PDFs can still be parsed. Update read_manifest to inspect the c2patool output
for a validated/successful manifest state before returning COLLATERAL_LABEL
data, and reject the PDF by raising an error or SystemExit if validation failed
or is absent. Use the existing read_manifest flow, rep parsing, and
COLLATERAL_LABEL lookup to keep the guard close to where the manifest is read.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5ddced44-ff89-4c7e-8fe1-609cea68ba50
⛔ Files ignored due to path filters (2)
experiments/conoir-spike/quorum/adapters/warehouse_receipt.pdfis excluded by!**/*.pdfexperiments/conoir-spike/quorum/adapters/warehouse_receipt_c2pa.pdfis excluded by!**/*.pdf
📒 Files selected for processing (9)
experiments/conoir-spike/quorum/_mkroot/Prover.tomlexperiments/conoir-spike/quorum/adapters/ADAPTER.mdexperiments/conoir-spike/quorum/adapters/run_warehouse_pipeline.shexperiments/conoir-spike/quorum/adapters/warehouse_adapter.pyexperiments/conoir-spike/quorum/adapters/warehouse_adapter_object.jsonexperiments/conoir-spike/quorum/adapters/warehouse_receipt.txtexperiments/conoir-spike/quorum/bound_receivables/Prover.tomlexperiments/conoir-spike/quorum/commit_receivable/Prover.tomlexperiments/conoir-spike/quorum/proof_a_receivable/Prover.toml
✅ Files skipped from review due to trivial changes (5)
- experiments/conoir-spike/quorum/_mkroot/Prover.toml
- experiments/conoir-spike/quorum/proof_a_receivable/Prover.toml
- experiments/conoir-spike/quorum/commit_receivable/Prover.toml
- experiments/conoir-spike/quorum/adapters/warehouse_receipt.txt
- experiments/conoir-spike/quorum/adapters/ADAPTER.md
🚧 Files skipped from review as they are similar to previous changes (1)
- experiments/conoir-spike/quorum/bound_receivables/Prover.toml
Comment on lines
+39
to
+42
| for cand in "${C2PIE_PY:-}" \ | ||
| "/private/tmp/claude-501/-Users-jamienewton/1c6e4848-1e0b-4678-af70-eb7bad88f949/scratchpad/c2pie_venv/bin/python3" \ | ||
| "$HERE/.c2pie_venv/bin/python3" \ | ||
| "$QUORUM/p1_provenance/venv/bin/python3" \ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Remove the machine-specific Python path from discovery.
The /private/tmp/.../scratchpad/c2pie_venv/bin/python3 entry is a local artifact, leaks a username/path into the repo, and is dead everywhere except the author’s machine.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/adapters/run_warehouse_pipeline.sh` around
lines 39 - 42, Remove the hardcoded machine-specific Python candidate from the
interpreter discovery loop in run_warehouse_pipeline.sh. Update the cand list in
the startup logic to rely only on portable locations such as C2PIE_PY,
$HERE/.c2pie_venv/bin/python3, and $QUORUM/p1_provenance/venv/bin/python3, so
the script no longer references a local scratchpad path tied to one machine.
Comment on lines
+157
to
+158
| br_run '"0x01","0x02","0x03","0x04","0x05","0x06"' "clean (cid NOT in financed book)" "false" | ||
| br_run "\"0x01\",\"0x02\",\"$CANONICAL_ID\",\"0x04\",\"0x05\",\"0x06\"" "double (cid IS in financed book)" "true" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the proven anchor in the “double” fixture.
bound_receivables is fed candidate_anchor="$PROVEN_ANCHOR", but the positive case inserts $CANONICAL_ID into financed. Those are different values, so this branch cannot exercise the intended membership hit.
Suggested fix
-br_run "\"0x01\",\"0x02\",\"$CANONICAL_ID\",\"0x04\",\"0x05\",\"0x06\"" "double (cid IS in financed book)" "true"
+br_run "\"0x01\",\"0x02\",\"$PROVEN_ANCHOR\",\"0x04\",\"0x05\",\"0x06\"" "double (anchor IS in financed book)" "true"📝 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.
Suggested change
| br_run '"0x01","0x02","0x03","0x04","0x05","0x06"' "clean (cid NOT in financed book)" "false" | |
| br_run "\"0x01\",\"0x02\",\"$CANONICAL_ID\",\"0x04\",\"0x05\",\"0x06\"" "double (cid IS in financed book)" "true" | |
| br_run '"0x01","0x02","0x03","0x04","0x05","0x06"' "clean (cid NOT in financed book)" "false" | |
| br_run "\"0x01\",\"0x02\",\"$PROVEN_ANCHOR\",\"0x04\",\"0x05\",\"0x06\"" "double (anchor IS in financed book)" "true" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/adapters/run_warehouse_pipeline.sh` around
lines 157 - 158, The “double” fixture is using a different identifier in
financed than the anchor passed into bound_receivables, so the membership-hit
case is not actually exercising the intended path. Update the positive br_run
fixture so the financed entry uses the same proven anchor value as
candidate_anchor="$PROVEN_ANCHOR" instead of $CANONICAL_ID, keeping the rest of
the test data unchanged. Verify the “clean” case still uses a non-matching ID
and that the “double” case now matches the anchor used by bound_receivables.
Comment on lines
+76
to
+80
| out = subprocess.run([NARGO, "execute", "--program-dir", cr], | ||
| capture_output=True, text=True).stdout | ||
| line = next(l for l in out.splitlines() if "Circuit output" in l) | ||
| inner = line.split("Circuit output:", 1)[1].split("[", 1)[1].rsplit("]", 1)[0] | ||
| cid, anchor = [t.strip() for t in inner.split(",")] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the target file and nearby symbols first.
ast-grep outline experiments/conoir-spike/quorum/adapters/warehouse_adapter.py --view expanded || true
printf '\n--- file excerpt ---\n'
sed -n '1,220p' experiments/conoir-spike/quorum/adapters/warehouse_adapter.py | cat -n
printf '\n--- search for compute_canonical_id usage and tests ---\n'
rg -n "compute_canonical_id|Circuit output|nargo execute|warehouse_adapter" experiments/conoir-spike -S || trueRepository: Apertrue/circuits
Length of output: 20213
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '250,310p' experiments/conoir-spike/quorum/adapters/warehouse_adapter.py | cat -n
printf '\n--- fattura comparison ---\n'
sed -n '80,110p' experiments/conoir-spike/quorum/adapters/fattura_adapter.py | cat -n
printf '\n--- warehouse pipeline wrapper ---\n'
sed -n '1,120p' experiments/conoir-spike/quorum/adapters/run_warehouse_pipeline.sh | cat -nRepository: Apertrue/circuits
Length of output: 12495
Handle nargo execute failures before parsing stdout.
subprocess.run(...).stdout drops the return code and stderr, so a failed nargo execute (or any run that doesn’t print Circuit output) ends up as a StopIteration here instead of surfacing the actual circuit error. Raise a clearer error from the subprocess result first.
🧰 Tools
🪛 Ruff (0.15.20)
[error] 76-76: subprocess call: check for execution of untrusted input
(S603)
[error] 78-78: Ambiguous variable name: l
(E741)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/adapters/warehouse_adapter.py` around lines
76 - 80, Handle `nargo execute` failures in `warehouse_adapter.py` before
parsing stdout. In the code path that runs `subprocess.run` and then searches
for “Circuit output” in `out.splitlines()`, capture the full result object
instead of only `.stdout`, check the return code, and surface stderr or a
descriptive exception from the failed `nargo execute` call before attempting to
parse output. Update the parsing logic around the `inner`/`cid, anchor`
extraction so it only runs after a successful subprocess result and emits a
clearer error if the expected circuit output line is missing.
Comment on lines
+180
to
+182
| raw = subprocess.run(["/usr/sbin/cupsfilter", txt_path], | ||
| check=True, capture_output=True).stdout | ||
| open(out_pdf, "wb").write(raw) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('experiments/conoir-spike/quorum/adapters/warehouse_adapter.py')
lines = path.read_text().splitlines()
for i in range(150, 205):
if i <= len(lines):
print(f"{i:4d}: {lines[i-1]}")
PY
printf '\n--- cupsfilter refs ---\n'
rg -n "cupsfilter|CUPSFILTER|usr/sbin/cupsfilter|PATH" experiments/conoir-spike/quorum -S
printf '\n--- dependency mentions ---\n'
rg -n "cupsfilter|CUPS|printer|pdf" -S README* experiments/conoir-spike/quorum -g '!**/__pycache__/**'Repository: Apertrue/circuits
Length of output: 10526
Resolve cupsfilter from $PATH instead of hard-coding /usr/sbin/cupsfilter. The adapter already treats it as an external dependency, so pinning the absolute path will break installs where the binary lives elsewhere; an env override would keep it configurable.
🧰 Tools
🪛 ast-grep (0.44.0)
[warning] 181-181: File path is request-/variable-derived; validate and normalize to prevent path traversal.
Context: open(out_pdf, "wb")
Note: [CWE-22] Improper Limitation of a Pathname to a Restricted Directory ('Path Traversal').
(open-filename-from-request)
🪛 Ruff (0.15.20)
[error] 180-180: subprocess call: check for execution of untrusted input
(S603)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/adapters/warehouse_adapter.py` around lines
180 - 182, Resolve the hard-coded cupsfilter path in the warehouse_adapter
subprocess call by making the binary lookup configurable instead of always using
/usr/sbin/cupsfilter. Update the logic around the subprocess.run invocation in
the adapter to resolve cupsfilter from $PATH by default, and allow an
environment override so deployments with the binary in a different location
still work. Keep the change localized to the existing adapter flow that builds
raw and writes out_pdf.
Comment on lines
+263
to
+272
| def read_manifest(pdf_path): | ||
| """Parse the C2PA manifest via c2patool, return (collateral_assertion_dict, manifest_label).""" | ||
| out = subprocess.run(["c2patool", pdf_path], check=True, capture_output=True, text=True).stdout | ||
| rep = json.loads(out) | ||
| label = rep["active_manifest"] | ||
| man = rep["manifests"][label] | ||
| for a in man["assertions"]: | ||
| if a["label"] == COLLATERAL_LABEL: | ||
| return a["data"], label | ||
| raise SystemExit(f"FAIL: {COLLATERAL_LABEL} assertion not found in {pdf_path}") |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== file outline ==\n'
ast-grep outline experiments/conoir-spike/quorum/adapters/warehouse_adapter.py --view expanded || true
printf '\n== relevant lines ==\n'
nl -ba experiments/conoir-spike/quorum/adapters/warehouse_adapter.py | sed -n '220,330p'
printf '\n== search usages ==\n'
rg -n "read_manifest|active_manifest|c2patool|COLLATERAL_LABEL|canonical_id|Proof A|proof a|canonical_id" experiments/conoir-spike/quorum -SRepository: Apertrue/circuits
Length of output: 1190
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
path = Path('experiments/conoir-spike/quorum/adapters/warehouse_adapter.py')
lines = path.read_text().splitlines()
for start, end, title in [
(240, 305, 'warehouse_adapter.py lines 240-305'),
(305, 360, 'warehouse_adapter.py lines 305-360'),
]:
print(f"\n== {title} ==")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PY
printf '\n== usages of read_manifest and manifest validation-related symbols ==\n'
rg -n "read_manifest|active_manifest|c2patool|COLLATERAL_LABEL|validate|validation|signature|manifest" experiments/conoir-spike/quorum -SRepository: Apertrue/circuits
Length of output: 26790
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== validation-state references ==\n'
rg -n "validation_state|validation state|validated|c2patool.*validation|active_manifest" experiments/conoir-spike/quorum -S
printf '\n== adapter docs around read path ==\n'
python3 - <<'PY'
from pathlib import Path
path = Path('experiments/conoir-spike/quorum/adapters/ADAPTER.md')
lines = path.read_text().splitlines()
for start, end, title in [
(100, 150, 'ADAPTER.md lines 100-150'),
]:
print(f"\n== {title} ==")
for i in range(start, min(end, len(lines)) + 1):
print(f"{i:4d}: {lines[i-1]}")
PYRepository: Apertrue/circuits
Length of output: 4918
Fail closed on invalid C2PA manifests.
read_manifest() only parses c2patool output and pulls active_manifest; it never checks that the manifest validated before accepting org.apertrue.quorum.collateral. Gate extraction on a validated manifest, or reject the PDF when validation fails, so a tampered carrier cannot feed stale fields/key material into Proof A.
🧰 Tools
🪛 ast-grep (0.44.0)
[error] 264-264: Command coming from incoming request
Context: subprocess.run(["c2patool", pdf_path], check=True, capture_output=True, text=True)
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(subprocess-from-request)
🪛 Ruff (0.15.20)
[error] 265-265: subprocess call: check for execution of untrusted input
(S603)
[error] 265-265: Starting a process with a partial executable path
(S607)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@experiments/conoir-spike/quorum/adapters/warehouse_adapter.py` around lines
263 - 272, read_manifest currently accepts the active_manifest and collateral
assertion without confirming the C2PA validation result, so tampered PDFs can
still be parsed. Update read_manifest to inspect the c2patool output for a
validated/successful manifest state before returning COLLATERAL_LABEL data, and
reject the PDF by raising an error or SystemExit if validation failed or is
absent. Use the existing read_manifest flow, rep parsing, and COLLATERAL_LABEL
lookup to keep the guard close to where the manifest is read.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Experiment branch — review for the design, not for merge. R&D spike proving out coNoir (TACEO coSNARKs / MPC) × apertrue for privacy-preserving double-dip detection (insurance / trade-finance / supply-chain). No production circuits are touched;
proof_a/proof_band the deterministic nullifierNare untouched.What's verified end-to-end (run under coNoir 3-party REP3 MPC, proofs verified)
nullifier_check— double-dip check under MPC, only the collision bit revealed.imt_nm_mini— the bit-decomposition ops the IMT needs (Field::lt,to_le_bits) work under MPC.imt_nm_scale— full Colofon scale (depth-32, 50 checks, 153,843 gates): ~160s prove, 29 ms verify.bound_nm— the authenticated-MPC binding: public hidingCommit(nullifier, blind)anchor + in-MPC opening check + non-membership. Verified no-collision (0), collision (1), and binding-failure (substituted nullifier → rejected).imt_real— the realcolofon_imtlib (check_non_membership+CveLeafPreimage) compiled under beta.20 and run under MPC. Two migrations:u1→bool, poseidonv0.1.1→v0.3.0.What's compile-verified but NOT run end-to-end (deliberate)
binding_wrapper(Option A, client-side, off-MPC): recursively verifies an apertrue proof and emitsC = Commit(public_values[2], blind)— a commitment to the verified nullifier. Compiles, 773,025 gates. Recursion uses the samebb_proof_verification/verify_honk_proofpath apertrue'simage_aggregator(~1.5M gates, 2× verify) already runs end-to-end in production, so it's in-budget; the only new logic is an index read + one Poseidon commit. Full end-to-end prove deferred to production integration.Locked architecture (CTO decision)
Containerised downstream sidecar that consumes apertrue's existing proof + deterministic
N; derivesCviabinding_wrapper(composition, not duplicating the C2PA circuit); secret-sharesNinto the MPC; runs the bound non-membership.Nis never blinded (load-bearing for dedup/SD/verification/on-chain). Rejected: duplicating proof_a (anti-pattern). Deferred: foldingCinto proof_a (post-lighthouse, behind a flag — breaking format change).Threat-model notes
mpc-core/src/lib.rs): secure vs passive nodes, not active. Needs reputable/independent node operators (governance) or malicious-secure REP3 (not yet implemented).Not done (out of scope here)
Cinto proof_a (deferred to post-lighthouse).Remaining risk is product (lighthouse + design partner + node governance), not engineering. See
experiments/conoir-spike/README.mdfor full details and reproduce steps.🤖 Generated with Claude Code
Summary by CodeRabbit