feat: zero-knowledge run attestation + static no-recursion gate#474
feat: zero-knowledge run attestation + static no-recursion gate#474Solizardking wants to merge 1 commit into
Conversation
Add internal/attest — a stdlib-only package that makes "Zero" literal: - Hash-chained transcripts: every run event folds into a SHA-256 chain whose head is a 32-byte payload commitment; JSONL export re-verifies offline with VerifyJSONL, and any tampering is detected. - Nullifiers bit-compatible with clawd-zk (@clawd/zk-client computeNullifier): SHA-256(secret ‖ context ‖ nonce_u64le), giving one-shot replay protection when published on Solana via publish_attestation (~$0.005/run via compressed PDAs). - Attestation artifact carrying exactly the four public inputs the clawd-zk program verifies: attester, modelHash, payloadCommitment, nullifier. The chain learns that a run happened, which model set produced it, and that it happened exactly once — never prompts, tool calls, or outputs. - Static no-recursion gate: norecursion_test.go builds the package's intra-package call graph on every go test and fails on any direct or mutual recursion. Announce the capability at the top of the README and link docs/ATTESTATION.md from the documentation index. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
WalkthroughThis PR adds a new ChangesZK Run Attestation
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant Transcript
participant Nullifier
participant Attestation
Caller->>Transcript: Append(kind, taskID, payload)
Transcript-->>Transcript: update SHA-256 hash chain
Caller->>Transcript: Attest(secret, context, modelID)
Transcript->>Nullifier: Nullifier(secret, context)
Nullifier-->>Transcript: nullifier hash
Transcript->>Attestation: build Attestation struct
Attestation-->>Caller: Attestation (commitment, nullifier, modelHash)
Suggested reviewers:
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Warning |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
internal/attest/norecursion_test.go (1)
28-86: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCall-graph nodes are keyed by bare function name, ignoring receivers.
Methods and funcs are merged into the same namespace by
fd.Name.Namealone (acknowledged in the comment as "conservative"). If the package ever grows two distinct types with same-named methods (e.g., twoString()methods, one calling the other type's helper of the same name), this could produce false-positive cycles unrelated to actual recursion. Not an issue with the current single-type package, but worth keeping in mind as the package grows.🤖 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 `@internal/attest/norecursion_test.go` around lines 28 - 86, The call graph in norecursion_test.go is collapsing methods and functions into the same bucket by using only fd.Name.Name, which can create false recursion matches when different receivers share a method name. Update the graph-building logic around the fn struct and the fd.Name.Name/declared lookup to use a receiver-qualified symbol name for methods while keeping plain names for funcs, and apply the same symbol scheme when collecting callees so graph keys remain unique.internal/attest/attest.go (1)
60-76: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueNo concurrency guard on
Transcript.
Appendmutatest.head/t.recordswithout synchronization. If multiple goroutines ever append concurrently (e.g., tool calls run in parallel and each appends its own event), this races. Given "run" semantics probably imply single-goroutine sequential appends today, this is speculative, but worth a doc comment noting the type is not safe for concurrent use.🤖 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 `@internal/attest/attest.go` around lines 60 - 76, Transcript.Append mutates Transcript.head and Transcript.records without any synchronization, so the Transcript type should be documented as not safe for concurrent use unless you plan to add locking. Add a clear doc comment on Transcript and/or Append stating it must only be used sequentially by one goroutine, and if concurrent appends are intended, introduce a mutex around the Append mutation path in Transcript.Append.
🤖 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 `@internal/attest/attest.go`:
- Around line 187-231: The Attestation schema and Transcript.Attest()
implementation are missing the attester field that docs/ATTESTATION.md describes
as a public input. Update Attestation and Attest() to either include and
populate attester explicitly, or make the serialization/docs consistently state
that attester is provided elsewhere. Use the existing Attestation struct,
ModelSetID, and Transcript.Attest symbols to locate the change, and keep the
JSON/public-input shape aligned with publish_attestation.
---
Nitpick comments:
In `@internal/attest/attest.go`:
- Around line 60-76: Transcript.Append mutates Transcript.head and
Transcript.records without any synchronization, so the Transcript type should be
documented as not safe for concurrent use unless you plan to add locking. Add a
clear doc comment on Transcript and/or Append stating it must only be used
sequentially by one goroutine, and if concurrent appends are intended, introduce
a mutex around the Append mutation path in Transcript.Append.
In `@internal/attest/norecursion_test.go`:
- Around line 28-86: The call graph in norecursion_test.go is collapsing methods
and functions into the same bucket by using only fd.Name.Name, which can create
false recursion matches when different receivers share a method name. Update the
graph-building logic around the fn struct and the fd.Name.Name/declared lookup
to use a receiver-qualified symbol name for methods while keeping plain names
for funcs, and apply the same symbol scheme when collecting callees so graph
keys remain unique.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9fa7981b-8834-4083-b7e5-4cc0ee41f220
📒 Files selected for processing (5)
README.mddocs/ATTESTATION.mdinternal/attest/attest.gointernal/attest/attest_test.gointernal/attest/norecursion_test.go
| type Attestation struct { | ||
| Schema string `json:"schema"` | ||
| Context string `json:"context"` | ||
| ModelHash string `json:"modelHash"` | ||
| PayloadCommitment string `json:"payloadCommitment"` | ||
| Nullifier string `json:"nullifier"` | ||
| Events int `json:"events"` | ||
| CreatedAt string `json:"createdAt"` | ||
| } | ||
|
|
||
| // ModelSetID canonicalizes a set of model IDs (dedupe, sort, join) so | ||
| // the same model set always hashes to the same modelHash regardless of | ||
| // which model won which turn. | ||
| func ModelSetID(models []string) string { | ||
| seen := make(map[string]bool, len(models)) | ||
| uniq := make([]string, 0, len(models)) | ||
| for _, m := range models { | ||
| if m != "" && !seen[m] { | ||
| seen[m] = true | ||
| uniq = append(uniq, m) | ||
| } | ||
| } | ||
| sort.Strings(uniq) | ||
| return strings.Join(uniq, ",") | ||
| } | ||
|
|
||
| // Attest builds the attestation for a finished transcript. | ||
| // modelID is hashed (SHA-256) so the chain learns *which* model class ran | ||
| // without the transcript; secret must be >=16 bytes of private material. | ||
| func (t *Transcript) Attest(secret []byte, context, modelID string) (*Attestation, error) { | ||
| null, err := Nullifier(secret, context) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
| modelHash := sha256.Sum256([]byte(modelID)) | ||
| return &Attestation{ | ||
| Schema: attestationSchema, | ||
| Context: context, | ||
| ModelHash: "0x" + hex.EncodeToString(modelHash[:]), | ||
| PayloadCommitment: t.CommitmentHex(), | ||
| Nullifier: "0x" + hex.EncodeToString(null[:]), | ||
| Events: len(t.records), | ||
| CreatedAt: time.Now().UTC().Format(time.RFC3339), | ||
| }, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== internal/attest/attest.go ==\n'
wc -l internal/attest/attest.go
sed -n '1,280p' internal/attest/attest.go
printf '\n== docs/ATTESTATION.md ==\n'
wc -l docs/ATTESTATION.md
sed -n '1,260p' docs/ATTESTATION.md
printf '\n== search attester / publish_attestation ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'attester|publish_attestation|Attestation struct|Attest\(' .Repository: Gitlawb/zero
Length of output: 10429
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== attestation usages ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'ModelHash|PayloadCommitment|Nullifier|CreatedAt|Schema|Context|Attestation\{' internal docs README.md
printf '\n== internal/daemon/remote/auth.go ==\n'
wc -l internal/daemon/remote/auth.go
sed -n '1,220p' internal/daemon/remote/auth.go
printf '\n== internal/attest/attest_test.go ==\n'
wc -l internal/attest/attest_test.go
sed -n '1,220p' internal/attest/attest_test.goRepository: Gitlawb/zero
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== attester references ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' '\battester\b|\bAttester\b' internal docs README.md
printf '\n== attestation tests ==\n'
sed -n '1,220p' internal/attest/attest_test.go
printf '\n== any publish_attestation binding code ==\n'
rg -n --hidden --glob '!**/node_modules/**' --glob '!**/dist/**' --glob '!**/build/**' 'publish_attestation|payloadCommitment|nullifier|modelHash|attestation' internal docs README.md | head -n 200Repository: Gitlawb/zero
Length of output: 6786
Align the attestation schema with the documented public inputs Attestation here omits attester, but docs/ATTESTATION.md says publish_attestation consumes it as a public input. Either add it to Attestation/Attest() or update the docs and downstream serialization to make it explicit that attester is supplied elsewhere.
🤖 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 `@internal/attest/attest.go` around lines 187 - 231, The Attestation schema and
Transcript.Attest() implementation are missing the attester field that
docs/ATTESTATION.md describes as a public input. Update Attestation and Attest()
to either include and populate attester explicitly, or make the
serialization/docs consistently state that attester is provided elsewhere. Use
the existing Attestation struct, ModelSetID, and Transcript.Attest symbols to
locate the change, and keep the JSON/public-input shape aligned with
publish_attestation.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Link an approved parent issue before continuing this PR
PR description
The PR still has the template placeholderFixes #and the approved-issue checklist item is unchecked, butCONTRIBUTING.mdsays community PRs must be tied to an existing issue with theissue-approvedlabel and that PRs opened before that approval may be closed without review. Please link the approved parent issue, or move this proposal back to an issue for maintainer approval before continuing implementation review. -
[P2] Do not announce live attestation support before it is wired into Zero
README.md:17
The README now says "Zero can now prove an agent run happened" and describes per-run Solana replay protection, but the only references tointernal/attestoutside the new package are the README and the new doc. No agent runtime, transcript/session, CLI, or publish path actually records events into this transcript or produces/publishes an attestation during a Zero run. This turns an unused internal helper into a top-of-README product claim, so users will expect a capability the application does not provide. Please either wire the attestation flow into the actual run path, or keep the docs scoped to an experimental/internal package instead of announcing it as supported Zero behavior. -
[P2] Complete CodeRabbit's request to include the attester public input
internal/attest/attest.go:187
CodeRabbit's schema request is still valid:docs/ATTESTATION.mdsayspublish_attestationhas four public inputs, includingattester, but the exportedAttestationstruct andTranscript.Attestreturn onlymodelHash,payloadCommitment, andnullifierplus metadata. A caller following the documented artifact cannot construct the documented public-input set from this API. Please either include and populateattesterin the artifact/API, or change the docs and serialization contract to make it explicit thatattesteris supplied somewhere else. -
[P2] Make
Attestgenerate a per-run nullifier
internal/attest/attest.go:216
Transcript.Attestalways callsNullifier(secret, context)without a nonce or any run-specific value, while the docs example passes the constant context"zero/run/v1". That means two different runs using the same secret and documented context produce the same nullifier, so the first published attestation would make every later run look like a replay instead of providing one-shot replay protection per run. Please thread a nonce/run identifier intoAttestor derive the nullifier from a unique run-specific value, and update the docs/tests so the replay-protection claim matches the API.
|
Perfect!! |
2 participants
Add internal/attest — a stdlib-only package that makes "Zero" literal:
Announce the capability at the top of the README and link docs/ATTESTATION.md from the documentation index.
Summary
Linked issue
Fixes #
Checklist
issue-approvedlabel.go build ./...,go vet ./..., andgo test ./...pass locally.gofmtclean.-racewhere relevant).Summary by CodeRabbit
New Features
Documentation
Tests