Skip to content

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

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

feat(campaign)!: make code surfaces content-addressed#336
drewstone wants to merge 7 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.
  • Pin Git binary-patch compression at command scope so identical trees produce identical patch bytes despite repository or global config.
  • 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.
  • Release the breaking pre-1.0 contract as 0.112.0 for npm and PyPI.

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.112.0.
There is intentionally no lenient fallback on the evaluation path.

Verification

  • pnpm test: 270 files, 2,774 passed, 2 skipped
  • pnpm typecheck
  • pnpm build
  • pnpm verify:package
  • pnpm lint: 0 errors, 4 existing warnings
  • real-Git regressions for repository/global compression drift, clean filters, smudge filters, hidden executable-mode changes, 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, and reordered patch descriptors
  • 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 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

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