Skip to content

feat(campaign)!: make code surfaces content-addressed#336

Open
drewstone wants to merge 11 commits into
mainfrom
feat/immutable-code-surface
Open

feat(campaign)!: make code surfaces content-addressed#336
drewstone wants to merge 11 commits into
mainfrom
feat/immutable-code-surface

Conversation

@drewstone

@drewstone drewstone commented Jul 9, 2026

Copy link
Copy Markdown
Contributor

What changed

  • Finalize every code candidate with exact base/candidate commits, final Git tree, and a SHA-256-bound binary patch.
  • Generate Git binary patches in isolated metadata with fixed config, attributes, locale, diff options, and compression so identical trees produce identical patch bytes despite ambient settings.
  • Provide a fail-closed check that binds the candidate checkout, refs, objects, patch, raw file bytes, executable modes, and ignored files immediately before evaluation.
  • Reject Git clean/smudge filter mismatches, replacement objects, symbolic locator paths, external or untracked symlink targets, symlink paths that leave and re-enter the checkout, and submodules whose executable bytes are not bound.
  • Use one location-independent identity for selection, provenance, hosted snapshots, proposer deduplication, and no-op detection; reject legacy path-only code surfaces.
  • Version durable provenance as tangle.loop-provenance.v2 so content-addressed code hashes cannot be confused with v1 path-based hashes.
  • Release the breaking pre-1.0 contract as 0.113.0 for npm and PyPI; 0.112.0 was published by feat(analyst): type every agent improvement surface #337 while this PR was under review.

Why

Repository-level optimization is not trustworthy if a recorded candidate can point at mutable paths, moved Git refs, config-dependent patch bytes, or a checkout whose raw bytes differ from the signed tree while Git reports it clean.
This gives evaluation consumers a byte-for-byte proof that the checkout they are about to run is the candidate selected for measurement and promotion.

Breaking change

resolveWorktreePath() no longer returns a best-effort unchecked locator; it verifies the finalized candidate and throws on any identity mismatch.
Path-only CodeSurface objects are invalid, and downstream callers must migrate to adapter-finalized candidates when adopting 0.113.0.
There is intentionally no lenient fallback on the evaluation path.

Verification

  • pnpm test: 270 files, 2,803 passed, 2 skipped
  • pnpm typecheck
  • pnpm build
  • pnpm verify:package
  • pnpm lint: 0 errors, 4 existing warnings
  • real-Git regressions for repository/global/system diff-config drift, clean filters, smudge filters, hidden executable-mode changes, SHA-1/SHA-256 object formats, symlink paths/targets/trampolines, submodules, moved refs, and inherited Git environment redirection
  • identity regressions for hosted snapshots, composite proposer deduplication, policy-edit no-op detection, provenance v2, and reordered patch descriptors
  • merged current origin/main after feat(analyst): type every agent improvement surface #337 and verified git merge-tree --write-tree origin/main HEAD: clean

@drewstone drewstone requested a review from tangletools July 9, 2026 23:34
@drewstone drewstone requested review from tangletools and removed request for tangletools July 9, 2026 23:44
tangletools
tangletools previously approved these changes Jul 10, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — c284c942

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-10T07:14:11Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 130.2s (2 bridge agents)
Total 130.2s

💰 Value — sound-with-nits

Makes code-tier candidates content-addressed (commits/trees/binary-patch) and verified before evaluation, extending the repo's existing promptHash content-identity philosophy to code — a coherent, in-grain change with no existing equivalent found.

  • What it does: CodeSurface (src/campaign/types.ts:132) now requires baseCommit/baseTree/candidateCommit/candidateTree plus a {format:'git-diff-binary', sha256, byteLength} patch descriptor, all readonly. A new src/campaign/surface-identity.ts owns assertCodeSurfaceIdentity, codeSurfaceIdentityMaterial, surfaceContentHash (full sha256) and surfaceHash (16-char loop key) derived from ONE identity.
  • Goals it achieves: Recorded optimization candidates can no longer point at mutable paths or moved refs: the identity used by selection/promotion (surfaceHash/surfaceContentHash/RunRecord.promptHash) is now bound to the exact evaluated bytes. Unifies the previously-divergent 16-char loop key and full provenance hash so they cannot disagree, and gives code cells the same real content hash prompt cells already ha
  • Assessment: Good change, in the grain. It mirrors the established promptHash content-identity pattern (run-record.ts:145, agent-profile-cell.ts) rather than inventing a new one, follows the repo's fail-loud/no-fallback doctrine (asserts + typed WorktreeAdapterError, legacy path-only surfaces rejected), and genuinely de-duplicates the two inline hashers in provenance.ts and run-optimization.ts into o
  • Better / existing approach: None materially better found. Searched for prior art — grep candidateHash|contentAddress|patchHash|candidateTree|surfaceContentHash|promptHash and CandidateBundle|base64|deployable: prompt-tier content identity already exists (promptHash) but no code-tier content identity or candidate bundle existed; code surfaces were path/ref-only, so this fills a real gap rather than reinventing. Considered
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound

Content-addresses code-candidate identity through the measurement/selection/promotion chain, consolidating two prior path-hashing helpers into one immutable-identity module with hardened verification primitives.

  • Integration: The identity functions are LIVE in the loop today: runOptimization calls surfaceHash on every candidate (run-optimization.ts:143,215) for the loop dedup key, and provenance uses surfaceContentHash for the byte-verifiable record (provenance.ts:40). assertCodeSurfaceIdentity is wired into policyEditProposer.coerceCandidateSurface (policy-edit.ts:112) to reject legacy path-only surfaces at the propos
  • Fit with existing patterns: Fits the established grain and IMPROVES on it. The codebase previously had TWO separate identity functions hashing the SAME mutable material (worktreeRef+baseRef) — surfaceHash in run-optimization.ts and surfaceContentHash in provenance.ts. This PR consolidates both into surface-identity.ts (run-optimization.ts:358 and provenance.ts:43 now re-export from it) and switches the hashed material from m
  • Real-world viability: Hardened well beyond the happy path. Verification rejects hidden index entries (worktree/index.ts:174-181), dirty/ignored files (183-202), moved refs (204-216), wrong trees (218-241), and patch-byte mismatches (281-287); rebuilds the index via read-tree --reset to defeat stat-cache spoofing (246); strips GIT_* repository-redirection env vars (85-90); and uses binary-safe (Uint8Array) diff handling
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 worktreeRef is required-but-not-identity [maintenance] ``

CodeSurface.worktreeRef is typed required and assertCodeSurfaceIdentity rejects empty values (surface-identity.ts:14), yet its own doc says it is a 'mutable locator deliberately excluded from content hashes' (types.ts). It is needed by resolveWorktreePath/verifyCodeSurface to find the checkout, so carrying it is justified — but a type that is mandatory yet non-identity-bearing is a small consistency wart a reviewer may want named (e.g. a separate locator vs identity split). Does no


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260710T071807Z

tangletools
tangletools previously approved these changes Jul 10, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — d70a91f2

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-10T07:25:04Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 2 (2 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 223.6s (2 bridge agents)
Total 223.6s

💰 Value — sound-with-nits

Replaces a mutable path+ref CodeSurface locator with a content-addressed identity (exact commits, tree, and a deterministic binary-patch digest) plus a verifier that re-binds that identity to the live checkout — a coherent, in-grain trust fix with no existing equivalent to reuse, carrying only minor

  • What it does: Pre-PR, gitWorktreeAdapter.finalize() returned {kind:'code', worktreeRef, baseRef, summary} and both surfaceHash (presets/run-optimization.ts) and surfaceContentHash (provenance.ts) hashed the locator strings worktreeRef+baseRef. The change: (1) makes CodeSurface carry baseCommit/baseTree/candidateCommit/candidateTree plus a patch{format:'git-diff-binary', sha256, byteLength} (src/
  • Goals it achieves: Make the bytes actually evaluated identical to the candidate identity used for selection/promotion/provenance. Closes the gap where a recorded candidate could silently point at moved refs, a filter-distorted-but-git clean checkout, replacement objects, or symlink/submodule content not bound to the signed tree. Also removes a real prior inconsistency: two separate hash functions over the same (wr
  • Assessment: Good on its merits. The threat model is concrete and each defense maps to a named bypass with a real-Git regression test (tests/campaign/worktree.test.ts covers clean/smudge filters, hidden mode changes, symlink paths/targets, submodules, moved refs, env redirection). The design is in the codebase's grain: fail-loud typed errors (WorktreeAdapterError), no silent fallback, identity-by-content, VC
  • Better / existing approach: none — this is the right approach, and no existing equivalent was found. Searched: git grep for execFileSync('git'/ls-tree/rev-parse/diff-index across src — the only other git use is a tiny read-only metadata helper in src/product-benchmark/export.ts:127 (export provenance, not verification, not reusable). verdict-cache.ts:90 contentHash is a generic canonical-JSON hasher; reusing it w
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound

Content-addressed code-surface identity replaces mutable path-based hashing across the full optimization loop, with comprehensive real-Git verification at finalization — well-wired, no dead surface, no competing pattern.

  • Integration: Fully wired into the hot path. surfaceHash/surfaceContentHash (now centralized in surface-identity.ts:57-64) are the identity functions consumed by run-optimization.ts:215 (candidate dedup keys), provenance.ts:175 (byte-identical provenance record), self-improve.ts:730 (RunRecord.promptHash), run-improvement-loop.ts:140 (winner detection), run-lineage-loop.ts:340, and `profile/inde
  • Fit with existing patterns: Consolidates two scattered, weaker implementations (the 16-char loop key in run-optimization.ts and the full content hash in provenance.ts) into one canonical module. The old code hashed {kind, worktreeRef, baseRef} — mutable path labels — so two identical candidates in different worktrees got different hashes and a renamed worktree looked like a new candidate. The new identity (`surface-ide
  • Real-world viability: Verification is thorough and adversarial: scrubs inherited Git env vars that redirect the repo (worktree/index.ts:36-46,96-102), detects clean/smudge filter entries via ls-files -v (line 447), catches untracked and ignored files (lines 456-466), verifies raw blob bytes against tree objects with TOCTOU-safe open→fstat→read→recheck (lines 273-313), checks executable modes (line 400), validates s
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 ~650 lines of hand-rolled git plumbing is a real maintenance surface [maintenance] ``

worktree/index.ts grew to 654 lines including a binary ls-tree -z parser (parseGitTreeEntries, line 174), streaming blob hashing with TOCTOU fd guards (hashGitBlobFile, line 273), and symlink/submodule binding. Justified by the goal (raw-byte verification necessarily bypasses git's own clean/smudge path, so it can't just git diff --exit-code) and covered by real-Git tests, but it is bespoke code that future git behavior could make brittle. No in-repo library exists to delegate to, so this is

🟡 Verification is opt-in; nothing in this package invokes it on the run path [against-grain] ``

verifyCodeSurface/resolveWorktreePath are exported (index.ts:342-343) and unit-tested, but git grep shows no in-src caller — the only references are docs (SKILL.md:979) telling consumers to call it before execution. The CodeSurface.patch docstring (types.ts:155) also references a 'deployable candidate bundle' with base64 content that has no in-repo implementation (git grep candidate-bundle|deployable in src → none). This is consistent with substrate layering (consumers wire it), but a


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260710T073031Z

tangletools
tangletools previously approved these changes Jul 10, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 95702ea0

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-10T07:35:46Z

tangletools
tangletools previously approved these changes Jul 10, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — ecde3882

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-10T07:40:23Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Concerns 2 (2 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 216.6s (2 bridge agents)
Total 216.6s

💰 Value — sound-with-nits

Replaces path/ref-based code-candidate identity with a fail-closed, byte-for-byte content address (git object ids + SHA-256 binary patch) verified against the real checkout before evaluation — coherent, in-grain, and it closes a real integrity hole; ship with one optional DRY note.

  • What it does: Before, CodeSurface was {kind, worktreeRef, baseRef?, summary?} and both surfaceHash (run-optimization.ts) and surfaceContentHash (provenance.ts) hashed JSON.stringify({kind, worktreeRef, baseRef}) — i.e. the mutable locator, never the bytes — with no checkout verification. After (src/campaign/types.ts:132-164, surface-identity.ts, worktree/index.ts), a finalized surface carries exact `b
  • Goals it achieves: Makes tier-4 (repository-level) optimization trustworthy: a recorded candidate is byte-for-byte the checkout that gets measured and promoted, immune to moved refs, mutable paths, post-finalization edits, clean/smudge-filter deception, hidden filemode flips, inherited GIT_* env redirection, replacement objects, symlink escapes/trampolines, and submodule ambiguity. It also consolidates two near-du
  • Assessment: Good on its merits and squarely in the codebase's grain. The repo is already pervasively content-addressed and fail-loud — canonicalJson/contentHash (src/verdict-cache.ts:84-92), attestation (src/attestation.ts:4-20,64), pre-registration (src/pre-registration.ts:46-49,133), dataset, agent-profile-cell, analyst/policy-edit all use sha256: content identity with strict no-coercion — and CLAUDE.
  • Better / existing approach: Checked for an existing equivalent to fold into: src/integrity/ has no git/worktree/tree/checkout logic (grep git|worktree|tree|commit|verifyCode|checkout → no matches), and there was no prior verifyCodeSurface — so nothing is duplicated. The only reuse candidate is the generic canonicalJson/contentHash in src/verdict-cache.ts:84-92 (already imported by src/campaign/run-campaign.ts:15):
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound-with-nits

Content-addressed CodeSurface identity replaces the old path-only identity, deeply wired into the loop machinery and provenance, with a fail-closed byte-level verifier — coherent, correct grain, no competing pattern.

  • Integration: The hash functions (surfaceHash/surfaceContentHash) are immediately and deeply consumed: run-optimization.ts:215 (loop dedup + Pareto), run-improvement-loop.ts:140 (winner==baseline detection), run-lineage-loop.ts:221 (dispatchRef + runDir), provenance.ts:193 (baseline/winner content hashes), self-improve.ts:730 (RunRecord.promptHash), and profile/index.ts:219. assertCodeSurfaceIdentity is called
  • Fit with existing patterns: Fits the established substrate pattern exactly. The WorktreeAdapter interface was already VCS-pluggable by design (worktree/index.ts:8-10); this strengthens the existing finalize→CodeSurface→measure pipeline rather than adding a parallel one. MutableSurface = string | CodeSurface (types.ts:173) is the existing union; the change deepens CodeSurface's fields rather than replacing the type. No compet
  • Real-world viability: Extremely robust verification at worktree/index.ts:426-555. Strips all GIT_* repository-redirection env vars (lines 36-46, 96-101) and sets GIT_NO_REPLACE_OBJECTS=1 to prevent object replacement bypass. Verifies raw file bytes against Git tree object IDs via streaming hash (lines 273-313), not git status which clean/smudge filters can deceive. Checks executable modes (line 406), rejects submodules
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟡 Snapshot surfaceHash in self-improve.ts still uses location-dependent JSON.stringify, diverging from the loop's content-addressed identity [integration] ``

self-improve.ts:647-650 computes the snapshot's surfaceHash via hashString(JSON.stringify(surface)), which for a CodeSurface includes the mutable worktreeRef field. The loop's surfaceHash (surface-identity.ts:67-68) deliberately excludes worktreeRef via codeSurfaceIdentityMaterial. So two identical candidates from different worktree paths produce different snapshot hashes but identical loop hashes — a semantic mismatch between what the contract output reports and how the loop deduplicates. The s

💰 Value Audit

🟡 surfaceContentHash hand-rolls sha256 instead of delegating to existing contentHash() [duplication] ``

src/campaign/surface-identity.ts:60-69 recomputes createHash('sha256').update(...) over a JSON string while src/verdict-cache.ts:88-92 already exports the repo's canonical strict contentHash(value) (used by src/campaign/run-campaign.ts:15 and src/attestation.ts:20). Delegating would remove one more ad-hoc hasher and keep all content addressing on one primitive. Mitigating: the identity object is fixed-key-order and built only from assertCodeSurfaceIdentity-validated primitives (surface-ide


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260710T074543Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 1c0312dd

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-10T07:57:51Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 375.5s (2 bridge agents)
Total 375.5s

💰 Value — sound

Makes tier-4 code candidates byte-identifiable and verifies the checkout's raw bytes against the signed tree before eval — a coherent, in-grain trust-boundary hardening that also removes prior duplication.

  • What it does: Before: gitWorktreeAdapter.finalize() returned a CodeSurface carrying only a mutable locator (worktreeRef + optional baseRef) — see old src/campaign/worktree/index.ts:98-106. Selection, provenance, dedup and no-op detection all keyed on JSON.stringify({kind, worktreeRef, baseRef}), so two byte-identical candidates at different paths looked different and a recorded candidate could silen
  • Goals it achieves: Evaluation consumers get a byte-for-byte guarantee that the checkout they run is the candidate selected for measurement/promotion — closing the gap where Git reports clean while raw bytes differ (filters, replacement objects, mode-only changes). It also fixes a real correctness bug: content-identical candidates at different worktree paths now collapse to one (regression at `proposers/composite.tes
  • Assessment: Good on its merits and squarely in the codebase's grain. It matches the documented substrate doctrine — src/attestation.ts:10-13 states content-addressing is the substrate's job (signing is the consumer's), and this change is exactly substrate-side content-addressing + verification. It honors the repo's fail-loud/no-fallback rule (every mismatch throws WorktreeAdapterError; legacy path-only su
  • Better / existing approach: None — this is the right approach and it removes duplication rather than creating it. I checked src/attestation.ts and src/verdict-cache.ts (contentHash over canonical JSON): those address serializable report objects, not Git checkout bytes — none of them re-hash raw tracked blobs against a signed tree, so there is no existing equivalent to reuse. The change actually consolidates two pre-e
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound

Content-addressed code surfaces replace the old mutable path/ref identity with byte-verifiable Git tree + binary-patch identity, fully wired into the optimization loop's hashing, dedup, provenance, and RunRecord paths.

  • Integration: Fully reachable and correctly wired. The sole production constructor — gitWorktreeAdapter().finalize() at worktree/index.ts:610-624 — populates every new field (baseCommit, baseTree, candidateCommit, candidateTree, patch) and self-verifies via verifyCodeSurfaceWithGit before returning (line 625). surfaceContentHash is consumed in 6 production modules: provenance records (`provenance.ts:175
  • Fit with existing patterns: Replaces and consolidates cleanly. Previously surfaceHash lived in run-optimization.ts:355 and surfaceContentHash in provenance.ts:46 — both hashed by mutable worktreeRef + baseRef JSON. This PR consolidates both into surface-identity.ts with content-based identity that excludes locator paths and commit metadata. Backward-compat re-exports from provenance.ts:43 and `run-optimization.
  • Real-world viability: Strong adversarial coverage: 23 real-git test cases in worktree.test.ts exercise clean/smudge filters, hidden executable-mode changes, symlink traversal/trampoline escapes, submodules, ignored dirty files, moved refs, inherited GIT_DIR/GIT_WORK_TREE env bypass, and forged patch digests — all against actual temp git repos via execFileSync('git'), not mocks. Content-stability (identical bytes at
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260710T080550Z

@tangletools

Copy link
Copy Markdown
Contributor

❌ Needs Work — 1c0312dd

Review health 100/100 · Reviewer score 51/100 · Confidence 95/100 · 10 findings (1 high, 2 medium, 7 low)

deepseek kimi-code aggregate
Readiness 51 73 51
Confidence 95 95 95
Correctness 51 73 51
Security 51 73 51
Testing 51 73 51
Architecture 51 73 51

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 8/8 planned shots over 26 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 26 changed files. Global verifier still owns final merge decision.

Blocking

🔴 HIGH resolveWorktreePath now throws on verification failure — breaking semantic change — src/campaign/worktree/index.ts

Previously resolveWorktreePath returned a path via isAbsolute/existsSync/join — a best-effort locator resolution. Now it calls verifyCodeSurface() which throws WorktreeAdapterError on ANY verification failure (dirty tree, hidden index entries, symlink traversal, wrong commit, etc.). Callers that previously used resolveWorktreePath for non-finalized code surfaces or surfaces whose worktree became dirty over time will now receive unexpected exceptions. This is a deliberate design change (verify-before-use) but is undocumented as a breaking API change. Recommend: add a non-verifying variant or document the break in changelog.

Other

🟠 MEDIUM CodeSurface type made fully required — all existing consumers must add 7 new fields — src/campaign/types.ts

baseRef went from optional to required. Six new required fields (baseCommit, baseTree, candidateCommit, candidateTree, patch.format, patch.sha256, patch.byteLength) added. All fields made readonly. The canonical producer gitWorktreeAdapter.finalize() was updated, but any test utility, integration, or external consumer manually constructing a CodeSurface (memory.test.ts:91, skill-opt.test.ts:220, analyst-surface.test.ts:65) must update. The as never casts in those test paths indicate awareness, but downstream packages (agent-runtime, agent-knowledge) importing CodeSurface will get compilation errors until updated.

🟠 MEDIUM Binary-patch identity is not reproducible across git environments, yet verification recomputes it — src/campaign/worktree/index.ts

The whole point of this PR is an immutable, location-independent candidate identity, but the patch digest is not actually reproducible for binary files. PATCH_ARGS (lines 131-143) pins color/ext-diff/textconv/renames/algorithm/prefixes but never pins compression, and defaultGit (lines 94-110) only scrubs GIT_CONFIG/GIT_CONFIG_* env overrides — git still reads the repo config and ~/.gitconfig. I reproduced it directly: for the same binary change, git diff --binary ... produced sha256 cdcc9ec65d6dc9440c040cb5118a2e4170950e77f

🟡 LOW One identity-less code candidate truncates a composite member's entire remaining slate — src/campaign/proposers/composite.ts

The dedup key changed from JSON.stringify(surface) (never throws) to surfaceContentHash(surface) (line 103), which calls assertCodeSurfaceIdentity and throws a TypeError for any code surface missing baseCommit/baseTree/candidateCommit/candidateTree/patch. That throw sits inside the per-member try (lines 93-110) that wraps the ENTIRE inner proposal loop, so a single malformed code candidate from a member aborts processing of that member's remaining proposals — valid later candidates are silently dropped and the memb

🟡 LOW gitBytes wraps defaultGit Buffer return in unnecessary Buffer.from() copy — src/campaign/worktree/index.ts

defaultGit returns a Buffer from execFileSync. gitBytes calls typeof output === 'string' ? Buffer.from(output, 'utf8') : Buffer.from(output). The Buffer.from(buffer) branch creates a copy of potentially up to 256MB (MAX_GIT_OUTPUT_BYTES) for binary diffs. The copy is wasted: patchBytes, parseGitTreeEntries, and parseGitPathList all subsequently call Buffer.from() again on the result. A zero-copy path using output directly or output.subarray() would be more efficient. Not a correctness issue.

🟡 LOW Hosted test only covers CodeSurface path, not string-surface hash path — src/contract/self-improve.test.ts

The new test verifies that two CodeSurfaces with different worktreeRef produce identical surfaceHash values. It does not exercise the string path of surfaceHash (where surface is a plain string) through the hosted ingest pipeline. The string path is covered implicitly by the existing power-analysis test (which uses 'be careful' as baselineSurface) but the hosted event's surfaceHash field isn't asserted there. Low risk since surfaceHash's string branch is trivial (sha256(string)).

🟡 LOW New identity test asserts only the baseline snapshot, not the proposed-candidate generation snapshot — src/contract/self-improve.test.ts

The test finds the ingested event whose baseline.surfaceHash === surfaceHash(baseline) and asserts event.generations[0].surfaceHash === expected plus a standalone surfaceHash(sameBytesElsewhere) === expected. generations[0] is the baseline (shipEvalRunToHosted pushes index 0 = baseline first), so the snapshot-level assertions only prove the BASELINE uses surfaceHash(); the proposed candidate (sameBytesElsewhere) would land in generations[1], which is never asserted. Because baseline and the candidate share content here the candidate would coincidentally also equal expected, but a regression that kept surfaceHash for the baseline while reverting a generation snapshot to a path-derived hash would not be caught. Fix: also assert the candidate generation snapshot (e.g. generations[1]?.surfaceH

🟡 LOW String-surface hosted snapshot hash silently changes format (no string-branch test or continuity note) — src/contract/self-improve.ts

The swap to surfaceHash(surface) applies to ALL MutableSurface, including the common string-prompt baseline. For strings the value changes from the local 8-hex FNV-1a hashString(surface) to a 16-hex sha256 prefix. The hosted wire doc (src/hosted/types.ts:79) only promises a 'stable hash' string, so this is not a contract violation, but it is an observable cross-version discontinuity of the orchestrator/dashboard pivot + dedup key for every existing prompt-surface run: historical and new runs for the identical prompt will no longer share a surfaceHash. There is no string-surface test of snapshotFromCampaign (the new test only constructs CodeSurface) and no changelog/note flagging the pivot-key change. Fix: either add a one-line note/changelog about the string-surface hash change and a minim

🟡 LOW hashString (FNV-1a) remains unused for surface identity but still exported in file — src/contract/self-improve.ts

The non-cryptographic hashString is still defined and used only as a seed mix-in for cellsToRunRecords (line 767). It is no longer the surface identity hash — that role moved to surfaceHash from surface-identity. Consider marking it local-only or inlining it into cellsToRunRecords since its original purpose (surface identity) is gone. Not a correctness issue — SHA-256 is now used for identity.

🟡 LOW Filter-based worktree tests invoke sed without a win32 guard — tests/campaign/worktree.test.ts

Lines 179-180 and 199-200 register git clean/smudge filters that shell out to sed 's/^evil$/safe/' / sed 's/^safe$/evil/', yet unlike the sibling executable-mode test (line 214: if (process.platform === 'win32') return) these tests are not platform-guarded. On a Windows runner sed is unavailable, so the filter program errors and the asserted bypass (git status clean while raw bytes differ) may not be reached for the intended reason. Impact is environmental only — project CI is Linux and sed is standard on Linux/macO


tangletools · 2026-07-10T08:47:46Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

❌ 1 Blocking Finding — 1c0312dd

Full multi-shot audit completed 8/8 planned shots over 26 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 26 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-07-10T08:47:46Z · immutable trace

tangletools
tangletools previously approved these changes Jul 10, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 14ac8d88

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-10T21:17:04Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 142.6s (2 bridge agents)
Total 142.6s

💰 Value — sound

Replaces path-only code-candidate locators with a fail-closed, content-addressed identity (exact base/candidate commits + trees + a compression-pinned SHA-256 binary patch) and verifies the live checkout byte-for-byte before evaluation — a coherent, in-grain integrity fix with no existing equivalent

  • What it does: Makes tier-4 code candidates content-addressed and verified. CodeSurface (src/campaign/types.ts:135) now requires exact baseCommit/baseTree/candidateCommit/candidateTree plus a {format:'git-diff-binary', sha256, byteLength} patch descriptor; worktreeRef is explicitly a mutable locator excluded from identity. A new single source of truth, src/campaign/surface-identity.ts:1, defines `a
  • Goals it achieves: Closes a real integrity hole: previously a recorded candidate was just {worktreeRef, baseRef} (old types.ts / old worktree/index.ts finalize returned only those), and both surfaceHash/surfaceContentHash hashed JSON.stringify({kind, worktreeRef, baseRef}) — so a candidate could silently point at moved refs, a dirty-but-"clean"-reporting checkout, config-dependent patch bytes, or swapped raw
  • Assessment: Good on its merits and squarely in the grain. The repo's stated doctrine (CLAUDE.md) is "No fallbacks. Fail loud" and typed external-boundary outcomes — this PR is that doctrine applied to the Git boundary, with intentionally no lenient fallback. The breaking type change is justified (path-only surfaces were the bug). Git history shows the feature (9460139) followed by four adversarial hardening f
  • Better / existing approach: none — this is the right approach. I checked for an existing equivalent to reuse: grepped surfaceContentHash|surfaceHash|content-address|immutable and verifyCodeSurface|resolveWorktreePath|assertCodeSurfaceIdentity across src, and inspected git log -- src/campaign/worktree/index.ts src/campaign/provenance.ts src/campaign/types.ts. Before this PR the worktree adapter did zero verification (ol
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound

Consolidates two previously-divergent code-surface hashers into one content-addressed identity and adds byte-for-byte Git verification at finalize + resolve time — coherent, fully wired, no dead surface.

  • Integration: Fully reachable. The producer path (gitWorktreeAdapter.finalize → verifyCodeSurfaceWithGit at worktree/index.ts:630) is wired end-to-end. All four identity primitives (surfaceHash, surfaceContentHash, assertCodeSurfaceIdentity, codeSurfaceIdentityMaterial) have real internal callers: composite proposer dedup (composite.ts:103), policy-edit equality (policy-edit.ts:121), provenance content hashes (
  • Fit with existing patterns: Matches the codebase grain exactly. Consolidates a real duplication: surfaceHash lived in run-optimization.ts:358 and surfaceContentHash lived in provenance.ts, with different material for code surfaces (one hashed worktreeRef, the other hashed kind+worktreeRef+baseRef) — now both derive from one location-independent identity in surface-identity.ts. No competing git-content-addressing exists; the
  • Real-world viability: Genuinely adversarial, tested against real Git (not mocks). The 422-line test suite at tests/campaign/worktree.test.ts covers: repo+global compression drift (line 141), clean-filter bypass (226), smudge-filter rewrite (246), assume-unchanged hidden mutations (216), executable-mode drift under core.filemode=false (261), symlink locators + symlink parents + external targets + leave-and-reenter tramp
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260710T212114Z

tangletools
tangletools previously approved these changes Jul 10, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 3f22a4c2

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-10T21:39:24Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 136.6s (2 bridge agents)
Total 136.6s

💰 Value — sound

Replaces a path-only CodeSurface with a content-addressed, fail-closed candidate identity (commits/trees/SHA-256 patch) verified byte-for-byte at finalize and at measurement — coherent, on-doctrine, and it actually removes a pre-existing duplicated hasher.

  • What it does: CodeSurface (src/campaign/types.ts:140) used to be {kind, worktreeRef, baseRef?, summary?} and resolveWorktreePath() (pre-9460139 worktree/index.ts) returned a path unchecked. Now the surface carries exact baseCommit/baseTree/candidateCommit/candidateTree plus a {format:'git-diff-binary', sha256, byteLength} patch descriptor (types.ts:147-161); gitWorktreeAdapter.finalize() freezes tho
  • Goals it achieves: Makes repository-level optimization trustworthy: a recorded candidate can no longer point at a mutable path, a moved ref, config-dependent patch bytes, or a checkout Git reports clean while its raw bytes differ. Consumers get a fail-loud proof that the checkout they run is the candidate selected for measurement/promotion, and the same bytes at a different path now dedupe/no-op correctly (composite
  • Assessment: Good, and squarely in the codebase's grain. It is the literal embodiment of this repo's CLAUDE.md doctrine ('No fallbacks. Fail loud', typed outcomes, assertRealBackend, fail-closed labelTrust) — assertCodeSurfaceIdentity mirrors assertRealBackend, and the change lifts a repeated concern INTO the substrate (agent-eval), matching the layering rule that already pulled DefaultVerdict/`RunRe
  • Better / existing approach: none — this is the right approach. Searched for an existing equivalent (surfaceContentHash|surfaceHash|codeSurfaceIdentityMaterial|assertCodeSurfaceIdentity|resolveWorktreePath|verifyCodeSurface|gitWorktreeAdapter|integrity|assertRealBackend): the only analogous primitive is assertRealBackend (src/integrity/), which guards LLM-backend provenance, a different domain — this PR mirrors its shape
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound

Replaces mutable-locator (worktreeRef) surface identity with content-addressed Git identity (commits + tree + SHA-256-bound binary patch), centralized in one module and re-exported from all original call sites — fixing a fundamental correctness flaw in how candidates are deduped, hashed, and verifie

  • Integration: Fully wired. The three identity functions (surfaceContentHash, surfaceHash, assertCodeSurfaceIdentity) are centralized in surface-identity.ts:1 and re-exported from their original homes (provenance.ts:43 re-exports surfaceContentHash; run-optimization.ts:358 re-exports surfaceHash), so every existing consumer — composite.ts:103 (dedup), policy-edit.ts:121 (sameSurface), `self-i
  • Fit with existing patterns: Eliminates a duplication. Previously surfaceContentHash lived in provenance.ts and surfaceHash lived in run-optimization.ts, both hashing the mutable {kind, worktreeRef, baseRef} tuple — two implementations of the same concept that disagreed on format (16-char vs full sha256). Now both derive from one codeSurfaceIdentityMaterial in surface-identity.ts:45. The breaking CodeSurface t
  • Real-world viability: Verification is adversarial-grade, not happy-path: strips inherited Git environment variables that could redirect object storage (worktree/index.ts:36-46,98-101), pins patch compression at command scope (PATCH_ARGS:134), detects clean/smudge filter mismatches via ls-files -v (:458-465), rejects ignored/untracked files post-finalization (:467-477), re-hashes every raw blob on disk against
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 Content-addressed CodeSurface type is git-shaped inside a 'VCS-pluggable' adapter [against-grain] ``

CodeSurface now exposes git commit/tree/patch fields (types.ts:147-161) even though WorktreeAdapter is documented as VCS-agnostic for a future jj adapter (worktree/index.ts:8-10). Not a blocker — only git ships today, and a jj adapter can populate the same baseTree/candidateTree + git-compatible binary-patch descriptor — but if a second VCS arrives, the identity descriptor (not the interface) is what would need generalizing. Note for the reviewer; does not gate shipping.


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260710T214326Z

tangletools
tangletools previously approved these changes Jul 10, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — 9db6ca29

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-10T21:54:39Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 148.7s (2 bridge agents)
Total 148.7s

💰 Value — sound

Replaces path-locator code candidates with a content-addressed, fail-closed identity plus a pre-eval byte-for-byte checkout proof — coherent, in-grain, and it removes two prior near-duplicate hashers that hashed the mutable path.

  • What it does: Finalizes each code candidate with exact baseCommit/baseTree/candidateCommit/candidateTree plus a SHA-256+length-bound binary patch, and derives a single location-independent identity (src/campaign/surface-identity.ts:48-69) used for selection, provenance, hosted snapshots, proposer dedup (composite.ts:103) and no-op detection (policy-edit.ts:121). resolveWorktreePath() now runs verifyCodeSurface
  • Goals it achieves: Makes repository-level optimization trustworthy: a recorded candidate can no longer silently point at a moved ref, a config-dependent patch, or a checkout whose raw bytes/modes diverge from the signed tree while git reports clean. Closes the concrete bypasses named in the PR — clean/smudge filter mismatch (raw blob re-hash vs tree, worktree/index.ts:436-484), replace-objects (GIT_NO_REPLACE_OBJECT
  • Assessment: Good on its merits and squarely in the codebase's grain. The repo already runs on content-addressing (verdict-cache contentHash/canonicalJson, attestation sha256/canonical-json, dataset contentHash, agent-profile-cell, pre-registration SignedManifest) and fail-loud doctrine; this PR brings the one tier still on mutable path locators up to that standard, and docs/design/loop-taxonomy.md:165-171 alr
  • Better / existing approach: none — this is the right approach. Checked verdict-cache.ts contentHash/canonicalJson (src/verdict-cache.ts:84-92): that's a whole-object canonical hash, which would be wrong here because code identity is a deliberate subset projection that excludes worktreeRef and candidateCommit so two paths/commits with the same base+tree+patch collide (surface-identity.ts:48-50). attestation.ts:10 explicitly d
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound

Content-addressed CodeSurface identity replaces mutable path-based hashing across the substrate; identity primitives are fully wired into dedup/provenance/hosted-snapshots, verification is self-tested with real Git and correctly placed at the consumer-dispatch boundary.

  • Integration: Fully wired. surface-identity.ts is now the single source for surfaceContentHash/surfaceHash, consolidating what was previously duplicated in provenance.ts:43 and run-optimization.ts:358. Five internal modules consume it: compositeProposer (composite.ts:34 — dedup key), policyEditProposer (policy-edit.ts:19 — coercion gate + same-surface check), self-improve.ts:44 (hosted snapshots + RunRecord.pro
  • Fit with existing patterns: Matches the established grain exactly. The prior design had surfaceHash (run-optimization.ts) and surfaceContentHash (provenance.ts) as separate functions with identical path-based logic for code surfaces — this PR unifies them into one module and upgrades code surfaces from path-identity to content-identity. The breaking contract change (path-only CodeSurface invalid, resolveWorktreePath fails lo
  • Real-world viability: Holds up under adversarial conditions. Patch generation is isolated in a scratch bare repo with pinned config/env/locale/diff-options/compression (worktree/index.ts:175-224) so identical trees produce identical bytes regardless of ambient settings — the real-world config-drift problem the PR targets. Verification hashes raw file bytes bypassing Git clean/smudge filters (hashGitBlobFile at line 345
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260710T215850Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Auto-approved drewstone PR — b1e24f67

This PR was opened by the trusted drewstone account.
The full PR reviewer audit still runs separately and will publish findings if it detects issues.

tangletools · auto-approval · reason: drewstone_author · 2026-07-10T22:11:56Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Value Audit — sound

Verdict sound
Concerns 1 (1 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 220.7s (2 bridge agents)
Total 220.7s

💰 Value — sound

Makes tier-4 code candidates byte-verifiable and collapses three divergent path-based identity hacks into one canonical content address; coherent, in-grain, no existing equivalent found.

  • What it does: Replaces the path-only CodeSurface ({kind, worktreeRef, baseRef?, summary?}, pre-PR src/campaign/types.ts:140-149) with a content-addressed one carrying exact baseCommit/baseTree/candidateCommit/candidateTree plus a SHA-256-bound binary patch descriptor (types.ts:140-164). Adds src/campaign/surface-identity.ts with assertCodeSurfaceIdentity, codeSurfaceIdentityMaterial, `surfaceContent
  • Goals it achieves: Make repository-level optimization trustworthy: a recorded candidate can no longer point at a mutable path, a moved/replaced ref, config-dependent patch bytes, or a checkout whose raw bytes diverge from the signed tree while Git reports clean. Eval consumers get a fail-closed, byte-for-byte proof that the checkout they are about to run is the candidate that was selected, measured, deduplicated, sn
  • Assessment: Good change on its merits, squarely in the codebase's grain. The repo's own CLAUDE.md mandates "No fallbacks. Fail loud" and fail-closed defaults; turning resolveWorktreePath from a best-effort unchecked locator into a verify-then-throw gate is that doctrine applied to the code tier. The change is substrate-contained (no upward deps), the breaking pre-1.0 bump to 0.113.0 is explicitly licensed,
  • Better / existing approach: I looked for an existing equivalent to reuse: verdict-cache.ts:90 contentHash, dataset.ts:56, pre-registration.ts / gates/sequential.ts:119 signManifest hashes are all for other domains (judge-verdict cache, scenario manifests, protocol pre-registration) and deliberately JSON-keyed — routing a code surface through them would recreate the exact path-only-JSON bug this fixes, so the dedi
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound

Content-addressed CodeSurface identity is consumed transparently by every optimization loop run via surfaceHash/surfaceContentHash; verification primitives are exported for the external agent-runtime dispatch consumer documented in SKILL.md.

  • Integration: The core identity functions (surfaceHash, surfaceContentHash) are called pervasively — runOptimization, provenance, compositeProposer, policyEditProposer, selfImprove all consume the new content-addressed hashes transparently. gitWorktreeAdapter.finalize() now self-verifies (worktree/index.ts:697) and produces immutable surfaces. The exported verifyCodeSurface/resolveWorktreePath have no in-repo c
  • Fit with existing patterns: Fits the established MutableSurface = string | CodeSurface contract perfectly. The optimization loop was already surfaceHash-driven; this PR replaces path-based hashes with content-addressed ones at the single identity seam (surface-identity.ts). The WorktreeAdapter interface was already VCS-pluggable; finalize() now returns content-addressed CodeSurface instead of a path locator. Follows the repo
  • Real-world viability: Verification is defense-in-depth: isolated bare repo with fixed config/attributes/locale/compression for deterministic patch bytes (worktree/index.ts:147-181,196-224); raw file hashing via direct fd read that bypasses Git clean/smudge filters (hashGitBlobFile, lines 345-385); TOCTOU mitigation (lstat before openSync, fstatSync after, size re-check during read); Git environment sanitization strippi
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 resolveWorktreePath re-derives the full binary patch on every call [proportion] ``

verifyCodeSurfaceWithGit (worktree/index.ts:613-619) recomputes the patch via patchBytes, which mkdtempSyncs a scratch dir, git init --bares a throwaway repo, and runs the full base..candidate binary diff (worktree/index.ts:186-224) — and resolveWorktreePath (:723-724), the hot pre-eval entry point, discards everything except .path. Per-cell resolve is therefore O(diff size) plus a bare-repo init each time, even though verifyCodeSurface already returns the verified patchBytes f


What this audit checks

It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.

Pass What it asks
Heuristic Vague title? Whitespace-only or cruft-bearing diff? (content signals only)
Duplication Do added function/class names already exist elsewhere in the repo?
Value Audit What does it do? What goal does it achieve? Is it good? Better architecture or already-exists?
Usefulness Audit Does it integrate and fit? Will it hold up in real use and actually get used?

Findings are concerns, not blocks — the human reviewer decides what to do with them.

value-audit · 20260710T221719Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — b1e24f67

Review health 100/100 · Reviewer score 56/100 · Confidence 95/100 · 10 findings (3 medium, 7 low)

deepseek kimi-code aggregate
Readiness 56 83 56
Confidence 95 95 95
Correctness 56 83 56
Security 56 83 56
Testing 56 83 56
Architecture 56 83 56

Reviewer score is advisory once the run is complete and the verdict has no blockers.

Full multi-shot audit completed 8/8 planned shots over 27 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 27 changed files. Global verifier still owns final merge decision.

🟠 MEDIUM provenance.ts fallback content hash uses 16-char ID with sha256: prefix — misleading — src/campaign/provenance.ts

When surfaceByHash.get(c.surfaceHash) returns undefined, contentHash is set to sha256:${c.surfaceHash}. But c.surfaceHash is only a 16-char hex string (the loop-identity key, not the full SHA-256). The resulting 23-char value has a sha256: prefix but only 16 hex chars, which fails the SHA256 = /^sha256:[a-f0-9]{64}$/ pattern used elsewhere in surface-identity.ts. Any consumer that parses or validates contentHash as a sha256 digest will fail on this fallback. Fix: either drop the sha256: prefix for the fallback case (e.g., unknown:), or use a distinguishable sentinel like missing:${c.surfaceHash}.

🟠 MEDIUM No tests for new surface-identity.ts module — src/campaign/surface-identity.ts

surface-identity.ts centralizes 4 exported functions (assertCodeSurfaceIdentity, codeSurfaceIdentityMaterial, surfaceContentHash, surfaceHash) that are the foundation of code-surface identity across the entire campaign module. None of these have dedicated tests. A bug in assertCodeSurfaceIdentity (e.g., a missing field in the validation loop, a regex that silently accepts invalid hex) would propagate through dedup (composite.ts), verification (worktree/index.ts), and provenance (provenance.ts). The module is testable without git or filesystem access. Add a surface-identity.test.ts covering: valid CodeSurface passes, each required field missing/invalid throws TypeError, surfaceContentHash for identical-code different-worktreeRef surfaces are equal, surfaceHash is a 16-char prefix of surface

🟠 MEDIUM No tests for worktree verification functions (~500 LoC of security-critical code) — src/campaign/worktree/index.ts

verifyCodeSurfaceWithGit, patchBytes, assertRawTreeMatchesWorktree, hashGitBlobFile, assertSymlinkTargetIsBound, and related helpers have zero tests. The git test seam (GitRunner injection) makes these testable — a test can supply a mock runner returning canned git output. Key paths to test: verifyCodeSurfaceWithGit rejects worktrees with skip-worktree index entries, rejects extra untracked files, rejects blob hash mismatches, rejects ancestry violations (candidate not descended from base), rejects symlinks pointing outside the worktree or to untracked content, and passes a clean checkout. The compromise risk is low since the code is defensive, but a regression here (e.g., a future change dropping the merge-base check) would silently accept invalid candidates.

🟡 LOW analyst-surface.test.ts uses incomplete (pre-PR) CodeSurface shape with as never cast — src/campaign/analyst-surface.test.ts

The test passes { kind: 'code', worktreeRef: 'wt/abc' } as never to test that buildAnalystSurfaceDispatch rejects code-tier surfaces. After this PR, this object no longer satisfies CodeSurface (missing baseCommit, baseTree, candidateCommit, candidateTree, patch). The test still passes because analyst-surface.ts's surfaceToText() only checks typeof surface !== 'string' and then accesses .kind/.worktreeRef — it does NOT call assertCodeSurfaceIdentity. The as never cast papers over the type error. If surfaceToText is later updated to call assertCodeSurfaceIdentity before rejecting, this test shape would throw a different error (TypeError about missing fields vs. the expected prompt-tier rejection). Not a bug today, but masks future refactor risk.

🟡 LOW One malformed code-surface proposal truncates a member's whole batch and mislabels it as a member failure — src/campaign/proposers/composite.ts

The dedup key is now surfaceContentHash(surface), which for a code surface invokes assertCodeSurfaceIdentity and can throw. That call sits inside the inner for-loop (lines 95-107) but the try/catch wraps the entire member processing (lines 93-110). A throw on proposal k therefore (a) stops processing that member's later proposals while leaving earlier pushes in pool, and (b) pushes a '<member.kind>: ...' error at [line 109](https://github.com/tangle-network/agent-eval/blob/b1e24f6702b562e8fbb4806a576689176044377d/s

🟡 LOW Malformed/legacy code surface now aborts the whole policy-edit proposer instead of degrading to text — src/campaign/proposers/policy-edit.ts

Previously any object with kind==='code' and a string worktreeRef was returned as-is. The new branch calls assertCodeSurfaceIdentity(surface) which throws TypeError if baseCommit/baseTree/candidateCommit/candidateTree/patch are missing, and that throw is not caught, so it escapes propose() and fails the entire policyEditProposer run rather than falling through to the return JSON.stringify(surface, null, 2) path at line 115. For prompt-tier policy edits this never triggers, but any partial/legacy code-shaped object reaching applyPolicyEditToSurface now hard-fails instead of being coerced. Fix: either catch the TypeError and fall back to the JSON.stringif

🟡 LOW Verify/finalize reject any ignored file, constraining how Node candidates are materialized — src/campaign/worktree/index.ts

extraPaths (lines 534-539) includes ls-files --others --ignored --exclude-standard -z, and any hit rejects at 540-543; tests/campaign/worktree.test.ts:427-436 confirms even a pre-existing ignored file makes finalize throw. This is intentional and CHANGELOG-documented (bytes must be fully git-bound), but it means ignored artifacts such as node_modules/dist/.turbo/coverage can never coexist with the worktree at finalize or at resolveWorktreePath time. For a JS candidate whose worker needs installed deps, the harness must install them outside the worktree or materialize the patch onto a fresh checkout with its own dependency resolution — worth the merger con

🟡 LOW Hosted surfaceHash identity algorithm/length changed for ALL surfaces, not just code — src/contract/self-improve.ts

The old branch typeof surface === 'string' ? hashString(surface) : hashString(JSON.stringify(surface ?? '')) produced an 8-char FNV-1a hex digest for every surface kind. The replacement surfaceHash(surface) (surface-identity.ts) returns the first 16 hex chars of a SHA-256 over (for strings) the raw text or (for code) the canonical identity material. So for string-surface selfImprove runs that set hostedTenant, the EvalRunGenerationSnapshot.surfaceHash value — and its length and algorithm — now differ from prior versions even though nothing about the prompt changed. This is the intended unification with provenance, and it is exactly what fixes the worktreeRef instability, but it is a wire-format change to the hosted ingest event: any hosted dashboard/ingest consumer that dedupes, joins,

🟡 LOW Schema version bump from v1 to v2 is untested for backward readers — tests/campaign/presets.test.ts

The provenance record schema changed from 'tangle.loop-provenance.v1' to 'tangle.loop-provenance.v2' (provenance.ts:88, 189). The test at line 502 asserts this new version. If consumers of the provenance format (hosted ingest, OTel exporters, audit tools) hardcode 'v1', they will silently fail to parse v2 records. No test covers v1 compatibility or v1→v2 migration path. This is expected (the PR is a breaking identity-model change) but the consumers should be notified.

🟡 LOW as never cast could silently accept a valid CodeSurface if types loosen — tests/campaign/skill-opt.test.ts

Line 220 casts { kind: 'code', worktreeRef: '/wt/a' } as never to bypass the typechecker. The test intent is to ensure skillOptProposer.propose() rejects CodeSurface inputs. If CodeSurface were to relax back to the old shape (kind + worktreeRef only), this test would silently accept a now-valid CodeSurface and fail because the rejection wouldn't occur. The same pattern appears in policy-edit-proposer.test.ts:232. Consider adding a comment noting that this cast is intentional because the object is deliberately incomplete to trigger runtime rejection.


tangletools · 2026-07-10T22:42:24Z · trace

@tangletools tangletools dismissed their stale review July 10, 2026 22:42

Superseded by re-review — no blocking findings on latest commit.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved — 10 non-blocking findings — b1e24f67

Full multi-shot audit completed 8/8 planned shots over 27 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 27 changed files. Global verifier still owns final merge decision.

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-07-10T22:42:24Z · immutable trace

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants