feat(kernel): surface-diff harvest + Prime Agent harness integration contract - #787
Conversation
…integration contract harvestSurfaceDiffs is the read-back dual of the mount manifest: re-hash the mounted profile surfaces (instructions/skills/memory) at settle and report what the agent itself changed — the harness-agnostic observation behind any self-refinement surface (claude-code memory edits, opencode AGENTS.md, Prime /refine). Observed session-scoped edits record; reuse-scoped edits go through the existing measured proposal pipeline, never auto-promoted. docs/design/prime-agent-harness-integration.md is the decision record for hosting Prime Agent as a sandbox-materialized harness: the boundary (harness owns its inner loop; Runtime owns identity/budget/lifecycle; Eval owns verdicts; Intelligence owns promotion), the anti-reinvention map of what the integration reuses, what is deliberately not built (TopologyAuthority, HarnessExecutionReceipt, an optional-method backend zoo), the numbered substrate wish-list, and the first gated /refine-transfer experiment. harness-compat.md gains a pending prime-agent column marker (measure, never inherit pi's row).
…ect checkpoint/branch claims boxSurfaceReader joins harvestSurfaceDiffs to the real box.fs.read seam (the one openSandboxRun reads deliverables through), with the SDK not-found error detected structurally. watch entries cover surfaces the agent CREATED at never-mounted paths — the common shape of a harness-authored memory/skill file — while the harvest stays layout-agnostic (the caller enumerates). Integration doc corrections: box.snapshot/branch, the CRIU capability probe, and the fork lineage already exist, so wish-list item 9 narrows to verifying snapshot coverage of the Prime daemon + kernel process tree; and a daemon-backed harness maps naturally onto the AgentEnvironmentProvider contract, so the adapter shape is per-tier (box backend vs provider), not forced.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 569f02e2
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-08-11T20:18:58Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 102.7s (2 bridge agents) |
| Total | 102.7s |
💰 Value — sound-with-nits
Lands a minimal, in-grain kernel primitive (read-back mounted-surface diff) plus a boundary ADR that explicitly rejects over-engineered alternatives; it is coherent and nothing existing does this work — the only nit is it ships with zero production callers.
- What it does: Adds
harvestSurfaceDiffs({ mounts, read, watch? })to/kernel: at settle, it re-reads the surfaces recorded inRunProvenance.mounts, SHA-256-hashes their settled bytes, and reportsmodified/removed/created/unreadablediffs against the mount manifest (unchanged → no entry; failed reads reported, never dropped). Two reader adapters ship:boxSurfaceReader(over thebox.fs.readseam, - Goals it achieves: (1) Close the symmetry gap:
recordMountrecords what the substrate placed INTO a run, but nothing recorded what the agent CHANGED in its own mounted surfaces before box teardown — the harness-agnostic generalization of Prime's/refine. (2) Make that evidence OBSERVED and session-scoped so it can later feed the existingproposeAgentImprovement→ review → gated-activation pipeline, never auto- - Assessment: Good change, squarely in the codebase's grain. It reuses the existing
MountManifestEntrytype (src/runtime/types.ts:134), reuses the exactbox.fs.readseam thatopenSandboxRunalready reads deliverables through (src/runtime/sandbox-run.ts:283), structurally typesSurfaceReadBoxto avoid an SDK import (the established pattern in src/runtime/in-process-sandbox-client.ts), and is tagged `@exp - Better / existing approach: Searched the three places that could plausibly overlap. (a)
recordMount/RunProvenance.mounts(src/runtime/types.ts:134-194) is the WRITE side only — it records what was placed in; nothing compares against settled bytes. The harvest is its read-back dual, not a duplicate. (b)harvestCorpus(src/runtime/harvest-corpus.ts:63) is a TRACE-derived harvest — it runs theobserve()analyst over jou - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
A coherent, well-fitting kernel primitive — the explicit read-back dual of the existing mount manifest — landing ahead of its first documented caller.
- Integration: Exported from
/kernel(src/runtime/index.ts:944-955); no in-tree production caller yet, only the test. This is the expected 'capability ahead of caller' case, not dead surface: the input it consumes (RunProvenance.mounts, populated viarecordMountinprepareBox) is a stable primitive (src/runtime/types.ts:180-194,src/runtime/run-loop.ts:178-185), and the design doc (`docs/design/pri - Fit with existing patterns: Matches the established
recordMount/RunProvenance.mountslaw precisely: the kernel never reads workspace contents — the caller supplies the read seam (SurfaceReader), mirroring howrecordMountkeeps hashing with the byte owner.boxSurfaceReaderrides the SAMEbox.fs.readseamopenSandboxRunuses to read deliverables (src/runtime/sandbox-run.ts:283), structurally typed (`SurfaceRead - Real-world viability: Error paths are the strongest part: missing→
removed, any other failure→unreadablecarrying the diagnostic, never silently dropped (lines 104-121, 137-146). Edge cases handled — duplicate paths collapse to last mount via Map (line 100), a watched path that was also mounted compares against its mount not ascreated(line 135), uppercase manifest hash normalized via.toLowerCase()(line 124), - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Primitive ships with zero production callers — infra-on-spec [proportion] ``
harvestSurfaceDiffs/boxSurfaceReader/fsSurfaceReaderare referenced only by their own test file and the kernel barrel (rg over src/runtime/run-loop.ts, sandbox-lineage.ts, sandbox-run.ts returns no callers). The doc says 'caller-invoked at settle by the same caller that recorded the mounts' but no settle path invokes it yet. This is defensible — the repo routinely lands kernel primitives ahead of wiring (git log shows many feat(...) primitives followed by later wiring PRs), the ADR's §6 na
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.
…th-form drift Review findings applied: boxSurfaceReader retries transient first-attempt 404s (the same write-flush blip openSandboxRun's deliverable read retries for) so a fresh self-edit is not reported missing; a NotFoundError whose resourceType names a non-file resource (the box or session being gone) is a transport failure, never 'every surface was removed'; lossy-decoded content (U+FFFD) is reported unreadable instead of hashed as a false modification. harvestSurfaceDiffs normalizes './'-prefixed paths for mount/watch collision (a mount-recorder form vs a file-tree form is one surface), dedupes watch entries by path, and runs all reads concurrently while preserving record order.
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 60 | 86 | 71 | 60 |
| Confidence | 90 | 90 | 90 | 90 |
| Correctness | 60 | 86 | 71 | 60 |
| Security | 60 | 86 | 71 | 60 |
| Testing | 60 | 86 | 71 | 60 |
| Architecture | 60 | 86 | 71 | 60 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 6/6 planned shots over 9 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 6/6 planned shots over 9 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 6/6 planned shots over 9 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Version regression 0.132.1 → 0.130.0 reflected on the version pin line — docs/canonical-api.md
The diff shows
**Version 0.132.1.**→**Version 0.130.0.**. The docs change itself is CORRECT: it matches package.json (which is also 0.130.0 at HEAD) and satisfies the freshness gate CLASS 1 version-pin check (scripts/check-docs-freshness.mjs:146). The concern is what it reflects: 0.132.1 is a published tag (git tag v0.132.1 exists; v0.130.x has NO tag, never released), so HEAD downgrades the package below the last release. This is visible in this file's diff but rooted in package.json (another shot). Impact: a published version regression breaks semver consumer expectations and npm install resolution for anyone pinned to 0.132.x. Fix: confirm with the release owner that the 0.130.0 reset is intentional (a revert of the 0.131.x–0.132.x line); if not, the package.json version and this
🟠 MEDIUM boxSurfaceReader skips the transient-404 retry the same seam's other consumer uses, so a flush delay misclassifies as 'removed' — src/runtime/surface-diff.ts
sandbox-run.ts:278-291 wraps box.fs.read in a 4-attempt backoff retry specifically because the box data plane transiently 404s just-written artifacts ('write not yet flushed, or an edge-read blip'). The SDK maps HTTP 404 -> NotFoundError (verified in sandbox SDK source:
case 404: return new NotFoundError(...)), and boxSurfaceReader maps NotFoundError -> missing:true -> SurfaceDiff status 'removed'. So at settle, a still-flushing box file produces a spurious 'removed' diff for a surface the agent actually left intact — and the doc's 'Reported rather than dropped' mitigation does not save it, because 'removed' looks like a legitimate agent deletion, not a transport hiccup. Impact: false self-edit evidence fed to the improvement pipeline. Fix: either retry NotFoundError N times with backoff
🟡 LOW HarvestSurfaceDiffsOptions missing type-level description — docs/api/runtime.md
The ### HarvestSurfaceDiffsOptions section (line 16104) goes directly to #### Properties with no summary paragraph. All sibling new types (SurfaceDiff, WatchedSurface, SurfaceReadBox, SurfaceReadOutcome, SurfaceReader) have descriptions. The primitive-catalog also classifies this as an undocumented supporting type. Root cause: the source interface declaration lacks a TSDoc comment. Add one at the declaration site and regenerate.
🟡 LOW Version header regresses to 0.130.0 and dependency floors are lowered; doc mirrors package.json so the risk sits in publish sequencing, not doc accuracy — docs/canonical-api.md
The doc's version (0.130.0), agent-eval floor (>=0.144.4), sandbox floor (>=0.19.1), and agent-interface floor (>=0.43.1) all exactly match package.json at head (verified line-by-line), so the doc is internally consistent and correct as a declaration. The changes are accurate consequences of this PR's large revert of post-0.130.0 work (0.132.1 -> 0.130.0; superviseDispatch, candidatePopulation, retained-run, driver-retry removed). Flag for the global verifier only: publishing 0.130.0 after 0.132.1 has been tagged, and code at head still importing AgentProfileDiff/applyAgentProfileDiff from agent-interface (src/candidate-execution/profile.ts, src/intelligence/delivery.ts, et al.), are package.json/CI concerns outside this shot's file; if those hold, no doc edit is needed.
🟡 LOW Incomplete path attribution for runProfileMatrix — docs/design/prime-agent-harness-integration.md
The 'Harness × model × task matrix' row lists both
defineLeaderboard/runProfileMatrixbut the 'Where' column cites onlysrc/runtime/define-leaderboard.ts.defineLeaderboardis there, butrunProfileMatrixlives in@tangle-network/agent-eval/campaign(confirmed via examples/coding-benchmark/benchmark.ts:27 and examples/webcode-matrix/webcode-matrix.ts:24). Impact: cosmetic — a reader following the pointer to find the matrix runner would miss it. Fix: add, @tangle-network/agent-eval/campaignto that cell.
🟡 LOW Surface-diff 'built' label could mislead out-of-context readers — docs/design/prime-agent-harness-integration.md
Section 3.1 labels harvestSurfaceDiffs as '(built — harvestSurfaceDiffs, /kernel)'. The referenced src/runtime/surface-diff.ts is introduced in this same PR (commit 09486c8). A reader encountering this doc after merge will find the code exists, so the claim is accurate. However, a reviewer auditing only this file without the broader PR diff could misinterpret 'built' as referring to pre-existing infrastructure rather than co-delivered code. No functional impact — the code exists and is correct.
🟡 LOW §2 table points holdout-gate symbols at supervise/budget.ts where they do not live — docs/design/prime-agent-harness-integration.md
Row 'Equal-compute topology comparison + holdout gates' lists
pairedLift,promotionGate,heldOutGatewith Where =src/runtime/supervise/budget.ts, bench/eval. Verified: none of the three are in budget.ts —promotionGateissrc/runtime/promotion-gate.ts:64,pairedLiftisbench/src/stats.mts:66,heldOutGateis@tangle-network/agent-eval/contract(external, see docs/api/primitive-catalog.md:1632). Only 'conserved budget pool' (createBudgetPool) actually lives in budget.ts. A reader navigating to budget.ts looking for the gate/lift symbols finds nothing. Fix: point the row atsrc/runtime/promotion-gate.ts+bench/src/stats.mts+agent-eval/campaign, or keep budget.ts only for the pool. All symbol names themselves are real (verified repo-wide), so no behavior is affect
🟡 LOW Line-wrap style diverges from the rest of the file — docs/research/harness-compat.md
The new 'Pending columns' section wraps at ~80 cols, while the existing file (matrix, warnings, Files) uses hard long-line paragraphs. Cosmetic only; the repo STE/Markdown guidance asks for one sentence per physical line, which the new block also does not follow. Recommend aligning wraps with the file's existing style on next touch. No content impact.
🟡 LOW Plural section heading with singular content — docs/research/harness-compat.md
Heading '## Pending columns' is plural but the section currently lists only prime-agent. Defensible as a forward-looking section header, but if no second column is anticipated soon, 'Pending column' would be marginally more accurate. No merge blocker.
🟡 LOW index.ts removes public exports without a semver-major bump — src/runtime/index.ts
This shot's index.ts deletes the public export surface for
superviseDispatch/SuperviseDispatchOptions(loop-dispatch), the entire retained-run family (startRetainedRun,reconnectRetainedRun,RetainedRunHandle, etc.), and the driver-retry family (classifyDriverFailure,DriverRetryPolicy,DriverAttemptsExhaustedError). The source files are deleted in the same PR and grep confirms zero internal references, so the change is internally consistent and typecheck-clean. It is nonetheless a breaking change for published-package consumers of those names, and package.json resets the version 0.132.1 -> 0.130.0 rather than a 1.x major. This is the PR's deliberate stability-contract graduation, so I flag it as informational: confirm the removal (and the backward version reset) is the int
🟡 LOW No test covers the transient-404-as-removed hazard or path-traversal escape — src/runtime/surface-diff.test.ts
Coverage of the typed-outcome branches is otherwise complete (modified/removed/created/unreadable, dup collapse, uppercase hash, mounted+watched overlap, NotFoundError mapping). Missing: (1) a case asserting the production behavior when box.fs.read rejects NotFoundError on first call then succeeds — would surface the no-retry hazard above as a failing or characterizing test; (2) a case asserting fsSurfaceReader behavior for '../'-escape and absolute paths, so the documented contract (or the fix) is locked in.
🟡 LOW 'created' status overstates when the file pre-existed in the box image — src/runtime/surface-diff.ts
A watched path that was never mounted but exists at settle is labeled
created— the interface doc and module doc define this as 'the shape a harness's new memory/skill file takes', and this signal is documented as improvement-relevant evidence that may later feed proposeAgentImprovement (docs/design/prime-agent-harness-integration.md:65-77). But 'created' actually means 'not in the mount manifest and now readable'; a file that pre-existed in the box base image (never mounted, never agent-authored) also reportscreated. The function has no mount-time baseline for watch paths, so the label is an inference, not a fact. Caller mitigation (only watch paths known absent at start) is documented, but a caller enumerating the box tree at settle cannot know what the image shipped. Consider namin
🟡 LOW A synchronously-throwing read aborts the entire harvest instead of reporting unreadable — src/runtime/surface-diff.ts
const outcome = await options.read(entry.path)is not wrapped in try/catch. The SurfaceReader contract is to return a typed outcome (per the type and module docs), and both shipped readers (boxSurfaceReader, fsSurfaceReader) comply. But a third-party reader that throws (sync or rejected promise) rejects the whole harvest, dropping every other diff — violating the module's stated law 'a failed read is reported as anunreadablediff carrying the error, never silently dropped' (line 14). A rejected promise already propagates identically. Cheap hardening: wrap the read call and convert thrown errors to theunreadablebranch.
🟡 LOW HarvestSurfaceDiffsOptions missing declaration summary, relegated to 'undocumented' in catalog — src/runtime/surface-diff.ts
The exported interface HarvestSurfaceDiffsOptions has property-level TSDoc (mounts/read/watch) but no summary line on the declaration itself. Per scripts/gen-primitive-catalog.mjs row policy ('add a TSDoc line at the declaration to earn a table row'), it lands in the kernel section's 'Undocumented supporting types' paragraph in docs/api/primitive-catalog.md:1081 while its sibling interfaces SurfaceDiff/WatchedSurface/SurfaceReadBox all earn table rows. The docs are technically consistent (runtime.md:16104 renders its properties), but a public, fully property-documented interface being catalogued as undocumented is a doc-completeness gap. Fix: add a one-line summary above [line 77](https://github.com/tangle-network/agent-runtime/blob/569f02e28f7a63515b48735f1826e02722543935/src/runtime/surf
🟡 LOW No cancellation signal on harvestSurfaceDiffs; a hung box read blocks settle indefinitely — src/runtime/surface-diff.ts
The loop awaits options.read(entry.path) sequentially with no AbortSignal. SurfaceReader itself takes no signal. If a box.fs.read hangs (transport stall), the settle harvest never completes and the caller has no way to abort. The sibling openSandboxRun path threads options.signal into every box read. Not blocking — settle-time callers can race the whole promise — but adding an optional signal threaded into the reader would match the kernel's established cancellation discipline.
🟡 LOW fsSurfaceReader allows absolute mount paths to bypass root directory — src/runtime/surface-diff.ts
Line 193:
const target = isAbsolute(path) ? path : resolve(root, path)— whenpathis absolute (e.g./etc/passwd), root is bypassed entirely. The JSDoc at lines 186-189 promises 'Relative mount paths resolve against root' but the implementation also accepts absolute paths. Mount paths come from the harness (not end-user input), so the blast radius is low, but the API contract and implementation are misaligned. Fix: either reject absolute paths with an error, or update the JSDoc to document that absolute paths are passed thr
🟡 LOW fsSurfaceReader does not contain mount paths under root — src/runtime/surface-diff.ts
const target = isAbsolute(path) ? path : resolve(root, path)— absolute mount paths are read verbatim and relative paths allow../traversal out of root. MountManifestEntry.path is caller-authored provenance that may be persisted/replayed (RunProvenance is surfaced on LoopResult). If a persisted or otherwise untrusted manifest is later harvested with fsSurfaceReader over a local root, the reader becomes a file-size + sha256-confirmation oracle outside the worktree (settled content is not returned, so no direct content exfiltration — hash and byteLength only). In the documented trusted-caller model there is no attack; the fix is cheap: reject or contain paths escaping root (resolve then check prefix), or document the containment contract explicitly.
🟡 LOW fsSurfaceReader resolves ../ and absolute paths outside root; undocumented escape — src/runtime/surface-diff.ts
const target = isAbsolute(path) ? path : resolve(root, path)— an absolute mount path bypasses root entirely, and a relative path containing '..' escapes it. Only the hash and byte size are exposed in the SurfaceDiff (never contents), and mount paths come from the caller's own RunProvenance (not untrusted input), so this is acceptable for the stated threat model. But the doc claims 'Relative mount paths resolve against root' without acknowledging the escape. If a harness ever forwards externally-influenced paths, this becomes a local-FS existence/size/hash oracle. Fix: either clamp with a resolved-path prefix check, or add one line to the doc noting paths are trusted.
tangletools · 2026-08-11T20:40:28Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 18 non-blocking findings — 569f02e2
Full multi-shot audit completed 6/6 planned shots over 9 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 6/6 planned shots over 9 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 6/6 planned shots over 9 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-08-11T20:40:28Z · immutable trace
…ntegration-uhdi3w # Conflicts: # docs/api/primitive-catalog.md
…ings agent-interface 0.46.x shipped the id as 'prime' (a distinct harness from pi — the fork's daemon rejects pi-line clients) and the sandbox backend enum accepts it, so 'prime' joins harnessBackends and the docs drop the provisional 'prime-agent' name. The decoder entry stays gated on validated wire output; the compat column stays gated on measurement. Review findings applied: a reader that throws is contained to an unreadable diff for that path instead of rejecting the whole harvest; fsSurfaceReader refuses paths that resolve outside its root (an escaping manifest path was an existence/hash oracle over the host filesystem); HarvestSurfaceDiffsOptions gains its declaration summary; 'created' is documented as a mount-manifest inference, not proof of authorship; the anti-reinvention table's holdout-gate and matrix rows point at the files that actually hold the symbols; the canonical-api version pin matches package.json at 0.132.2.
|
Review triage — the run audited Both MEDIUMs are resolved:
LOWs applied in
Deliberately not taken, with reasons:
Also picked up in the merge: Generated by Claude Code |
The release bump left the stored fixtures at runtimeVersion 0.132.1, so the fixture round-trip test fails against package.json on any branch carrying current main. Regenerated via generate:testing-fixture (the canonical path, so embedded digests stay valid).
…ntegration-uhdi3w # Conflicts: # src/testing/fixtures/agent-improvement-proposal.json # src/testing/fixtures/agent-profile-improvement-proposal.json
…ntegration-uhdi3w # Conflicts: # docs/api/primitive-catalog.md # docs/canonical-api.md
…ad and the surface harvest The box data plane can transiently 404 a just-written file. `openSandboxRun` already retried its deliverable read for that reason with an inline loop, and `boxSurfaceReader` added a second backoff of its own. Two copies of one policy drift apart. Extract `readBoxPathWithRetry` and call it from both sites. `beforeAttempt` keeps the caller's authority to abandon the read with an error only it can build, so `openSandboxRun` still throws `SandboxRunAbortError` with its drained events. Thread an `AbortSignal` into `boxSurfaceReader`. The harvest itself waits on nothing, so the reader owns every cancellable moment; `harvestSurfaceDiffs` documents why it takes no signal.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Concerns | 1 (1 low) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 108.0s (2 bridge agents) |
| Total | 108.0s |
💰 Value — sound
Adds a kernel surface-diff harvest (the read-back dual of RunProvenance.mounts), wires the shipped prime harness id, and extracts the pre-existing box-read retry into one shared primitive — coherent, in-grain, anti-reinvention-conscious; ship.
- What it does: Three concrete deltas. (1) New
/kernelprimitiveharvestSurfaceDiffs({mounts, read, watch?})(src/runtime/surface-diff.ts:131) re-hashes every mounted surface at settle and reports modified/removed/created/unreadable diffs; two readers join it to real substrates —boxSurfaceReaderover the sandboxbox.fs.readseam (surface-diff.ts:226) andfsSurfaceReaderover a worktree root with path-e - Goals it achieves: Capture the agent's self-surface-mutation evidence (claude-code editing CLAUDE.md, opencode editing AGENTS.md, Prime's
/refine) that currently evaporates at box teardown —RunProvenance.mounts(types.ts:192) records what went IN but nothing recorded what the agent CHANGED. That evidence is the input-side observation the existing improvement pipeline (proposeAgentImprovement, intelligence/imp - Assessment: Strong on its merits. The harvest mirrors an existing kernel law exactly — 'the kernel never reads workspace contents; the caller supplies the read seam' (surface-diff.ts:18-21), the read-back dual of how
recordMountkeeps mount hashing with the byte owner (run-loop.ts:182). It is harness-agnostic by construction (compares content hashes; needs no harness event format) and the four-status outcom - Better / existing approach: none — this is the right approach. I searched for an existing equivalent (rg refine|self-edit|surface.?diff|surfaceDiff across src/) and every
refinehit is the unrelated sample/refine strategy family (src/runtime/strategy.ts:828, run-loop.ts:6), not surface-edit evidence capture — so the harvest is genuinely new, not reinvention. The only existing surface-read path isopenSandboxRun's deliver - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
A coherent kernel primitive (surface-diff harvest) plus the shipped prime backend id and a faithful retry refactor — all in the codebase's grain, filling a real documented gap (agent self-mutation evidence that currently evaporates at teardown).
- Integration: Three pieces, three different reachability states, all sound. (1)
readBoxPathWithRetryis fully integrated NOW:openSandboxRun's settle calls it for every deliverable read (src/runtime/sandbox-run.ts:277), faithfully replacing the prior inline 4-attempt loop. (2)'prime'inharnessBackends(src/runtime/sandbox-backend.ts:33) is reachable NOW — the double-`satisfies readonly HarnessType[] s - Fit with existing patterns: Excellent fit, no competition. The harvest is the read-back dual of
recordMount— it consumes the existingMountManifestEntryverbatim (src/runtime/surface-diff.ts:135 vs src/runtime/types.ts:146) and obeys the same kernel law ('the kernel never reads workspace contents itself' — src/runtime/types.ts:78-80) by taking a caller-suppliedSurfaceReaderseam.boxSurfaceReaderrides the same `bo - Real-world viability: Built well past the happy path. Concurrency via
Promise.allwith record order preserved (surface-diff.ts:141,172); a throwing reader is contained to oneunreadablediff rather than rejecting the whole harvest (readOutcome, line 101); missing-vs-unreadable distinguished; box-gone-vs-file-gone distinguished via NotFoundErrorresourceType(line 256); lossy UTF-8 decode (U+FFFD) reports `unreada - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🔎 Heuristic Signals
🟡 Cruft: magic number added src/runtime/sandbox-run.ts
delayMs: options.readRetryDelayMs ?? 1000,
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.
…eadable one The SDK's HTTP mapper builds `new NotFoundError(data.resourceType || 'Resource', ...)`, so a file 404 carries `'Resource'` whenever the server does not name a type. Matching only file-ish names therefore classified an ordinary deletion as `unreadable`, and the harvest could never report `removed` — the one status that says the agent deleted a mounted surface. Invert the test: a NotFoundError is the file unless its `resourceType` names the box or the session, which stays `unreadable` because the transport is gone. Also stop the retry loop once the run is abandoned, and give `BoxSurfaceReaderOptions` the declaration summary the catalog asks for.
…ntegration-uhdi3w # Conflicts: # docs/api/primitive-catalog.md
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 5 (1 medium-concern, 1 low, 3 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.6s |
| Interrogation | 373.3s (2 bridge agents) |
| Total | 373.9s |
💰 Value — sound-with-nits
Bundles three changes: a well-built but caller-less surface-diff harvest (the titular feature), a one-line prime harness id + decision doc, and an unmentioned but coherent whole-forest provider-served-model-evidence refactor that is the actual bulk of the diff — all in-grain, with proportion and m
- What it does: Three logical changes land together. (1)
harvestSurfaceDiffs+boxSurfaceReader/fsSurfaceReader(src/runtime/surface-diff.ts) re-hash mounted surfaces at settle to report modified/removed/created/unreadable diffs; a sharedbox-read-retry(src/runtime/box-read-retry.ts) is extracted and also retrofitted intoopenSandboxRun's deliverable read (src/runtime/sandbox-run.ts:273). (2) The - Goals it achieves: (1) Capture what surfaces an agent mutated during a run as improvement evidence — the read-back dual of
RunProvenance.mounts, generalizing Prime's/refineto any harness. (2) Register the shipped Prime harness identity. (3) Let eval bind its single pre-admitted paid call to a proven provider-SERVED identity across recursively owned supervision trees; the priorsupervisedTreeModel(loop-dispa - Assessment: The provider-evidence refactor is the strongest part: it is a real precision improvement, correctly avoids double-counting summary nodes whose owned trees are authoritative (
spawn-journal.tscomments at the twocontinueguards), moves profile-matching to the observation point (assertObservedRouterModel/observedModelMatchesDeclaredalready inruntime.ts), and is in the grain of the journa - Better / existing approach: No existing equivalent to reuse — grep for
settledSha|surfaceDiff|mountDiff|rehashreturns nothing outside the new module, so the harvest is genuinely new, not a reinvention; and the prior provider-evidence path (RootProviderModelEvidence+ per-nodematerialization.model) is correctly being superseded, not duplicated. Thebox-read-retryshare is the right dedup. Two redirections worth a hu - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
Lands three coherent pieces in the codebase's grain — a shared box-read retry, provider-model evidence aggregation across owned journal forests (fully wired), and a surface-diff harvest primitive that is the read-back dual of the mount manifest (exported ahead of its external substrate caller, by de
- Integration: Two of the three new capabilities are already wired and called:
readBoxPathWithRetryis shared betweenopenSandboxRun's deliverable read (src/runtime/sandbox-run.ts:277) andboxSurfaceReader(src/runtime/surface-diff.ts:232) — this PR refactored the existing retry into box-read-retry.ts;aggregateProviderModelEvidenceis invoked byterminalAccountingat run settle (src/runtime/supervise/ - Fit with existing patterns: Fits the codebase's grain precisely. The harvest obeys the kernel law stated at types.ts:78-80 and surface-diff.ts:18-21 ('the kernel never reads workspace contents itself — the caller supplies the read seam'), mirroring how
recordMountkeeps mount hashing with the byte owner.boxSurfaceReaderrides the SAMEbox.fs.readseam that deliverable reads use (sandbox-run.ts:277), and the retry was - Real-world viability: Robust beyond the happy path, with strong test coverage (56/56 pass). Concurrent reads via Promise.all with record-order preservation (surface-diff.ts:141,172). A reader that THROWS is contained per-path into an
unreadablediff rather than rejecting the whole harvest (surface-diff.ts:101-111). The box reader distinguishes a gone-BOX (resourceType matching /sandbox|session/i) from a gone-FILE, so - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🔎 Heuristic Signals
🟡 Cruft: magic number added src/runtime/sandbox-run.ts
delayMs: options.readRetryDelayMs ?? 1000,
💰 Value Audit
🟠 Titular harvestSurfaceDiffs ships with zero in-tree callers [proportion] ``
rg "harvestSurfaceDiffs"across non-testsrc/matches only the definition (surface-diff.ts:131) and re-exports (index.ts,runtime/index.ts) — no settle path, supervise path, or mount recorder invokes it. That is ~300 lines of impl + 360 lines of tests + two readers landing as dead code in-repo, marked@experimental. The design doc frames it as 'built' for substrate/external adoption, which is plausible staging, but a human should decide: wire it to the settle caller that records moun
🟡 providerAttemptEvidence duplicated in two files [duplication] ``
Identical
function providerAttemptEvidence(model: string | undefined): ProviderModelExecutionEvidenceis defined atsrc/runtime/supervise/coordination-driver.ts:287andsrc/runtime/supervise/supervise.ts:684(byte-for-byte same body). Extract one shared helper (e.g. intomaterialization.tsalongside the other provider-evidence builders) and import from both.
🟡 PR bundles an unmentioned durable-journal refactor as the bulk of the diff [proportion] ``
Title and body describe surface-diff harvest + the prime id, but the provider-served-model-evidence refactor (spawn-journal
aggregateProviderModelEvidence, scope/materialization/runtime/loop-dispatch rewrites — roughly half of the +3003 lines) is not mentioned at all. This is not a terse-description complaint: bundling a change to durable journal semantics with an unused primitive under an unrelated title means a reviewer scanning the title will miss the highest-risk component. The refactor it
🎯 Usefulness Audit
🟡 Duplicate model-string parser across spawn-journal and model-identity [problem-fit] ``
servedIdentityParts(src/durable/spawn-journal.ts:642-652) parsesbase@snapshotand slices the last/segment, duplicating the logic in the privatemodelIdentityParts(src/runtime/model-identity.ts:30-38) plus the slicing in the siblingcanonicalObservedModel(model-identity.ts:23-28). The two diverge deliberately — the journal version skips theSNAPSHOT_TOKENregex (/^[A-Za-z0-9._-]+$/u) and accepts any non-empty snapshot — but that divergence is not documented at either site, so
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.
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | verifier:glm51 | aggregate | |
|---|---|---|---|---|---|
| Readiness | 77 | 73 | 47 | 80 | 47 |
| Confidence | 85 | 85 | 85 | 85 | 85 |
| Correctness | 77 | 73 | 47 | 82 | 47 |
| Security | 77 | 73 | 47 | 80 | 47 |
| Testing | 77 | 73 | 47 | 78 | 47 |
| Architecture | 77 | 73 | 47 | 88 | 47 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Verified all 20 reviewer findings against the actual code; 16 unique findings hold (2 medium, 14 low), 4 are duplicates. The sandbox-run.ts retry refactoring into box-read-retry.ts is behavior-preserving (same 4-attempt count, s
🟠 MEDIUM fsSurfaceReader doc overstates containment: symlink escape reads outside root — docs/api/runtime.md
runtime.md:26338 claims 'a persisted or mistyped manifest path cannot turn the harvest into an existence/hash oracle over the host filesystem.' The guarantee is lexical-only: fsSurfaceReader checks resolve(boundary,path) with startsWith(boundary+sep), but readFile() follows symlinks. A symlink placed inside root pointing at a host file passes the check and its bytes are read + SHA-256-hashed.
🟠 MEDIUM NotFoundError with default resourceType 'Resource' misclassifies a deleted surface as unreadable, not removed — src/runtime/surface-diff.ts
fileNotFound = notFound && (resourceType === undefined || /file|path/i.test(resourceType)). The SDK's NotFoundError constructor defaults resourceType to 'Resource' when the server omits it, so a real NotFoundError from box.fs.read is never undefined — it is at minimum 'Resource', which does not match /file|path/i. A file the agent genuinely deleted is classified as unreadable rather than removed. The unit test at surface-diff.test.ts:164-166 uses a bare Error with no resourceType, never exercising the 'Resource' default.
🟠 MEDIUM fsSurfaceReader containment is string-based and follows symlinks, defeating the documented escape guarantee — src/runtime/surface-diff.ts
The guard checks
target !== boundary && !target.startsWith(boundary + sep)on the resolved path string only, thenreadFile(target)follows symlinks. A symlink placed inside root (root/link -> /etc/passwd) passes the string check but reads outside the worktree. The doc comment at lines 265-272 claims this is prevented.
🟡 LOW BoxSurfaceReaderOptions listed as undocumented despite being a public option type — docs/api/primitive-catalog.md
The catalog adds
BoxSurfaceReaderOptionsto the 'Undocumented supporting types' list because its declaration in src/runtime/surface-diff.ts:203 carries only property-level TSDoc, no declaration-level TSDoc. It is a public option object threaded through boxSurfaceReader(box, options?).
🟡 LOW 'shipped and wired' / 'DONE' labels slightly outrun their own caveats — docs/design/prime-agent-harness-integration.md
§3.2 header 'The prime id — shipped and wired' and §5 item 1 'Harness identity — DONE' both immediately caveat that capability facts (model lock, selector honoring, measured compat column) are NOT done. The labels alone, if scanned, overstate.
🟡 LOW Conserved-budget argument overstates 'structurally impossible' — it bounds spend, not fan-out concurrency — docs/design/prime-agent-harness-integration.md
Lines 100-102 claim unbounded native fan-out is 'structurally impossible here' because native subagents spend inside the worker's conserved-budget reservation. The conserved pool bounds total cost (tokens/usd), not the number of concurrent native children — a harness can still fan out an unbounded child count that spends trivially per child.
🟡 LOW Imprecise 'three enums' phrasing for the compile-time pin — docs/design/prime-agent-harness-integration.md
The doc says the double-satisfies pin 'keeps the three enums aligned at compile time'. In src/runtime/sandbox-backend.ts:26-40 the pin is
as const satisfies readonly HarnessType[] satisfies readonly BackendType[]— two type enums plus the const array literal, not three enums.
🟡 LOW heldOutGate cited in the design doc but not present in the two local source files cited — docs/design/prime-agent-harness-integration.md
The anti-reinvention table names
heldOutGatenext topromotionGate/pairedLift, with the 'Where' column citingsrc/runtime/promotion-gate.ts,bench/src/stats.mts, and@tangle-network/agent-eval. Grep confirms heldOutGate is not in promotion-gate.ts or stats.mts; it exists in the external agent-eval package (primitive-catalog.md:1674). The table structure makes the source ambiguous.
🟡 LOW §6 prerequisite list omits items 6 and 8 without saying why — docs/design/prime-agent-harness-integration.md
§6 header reads 'when 1–5, 7, 9 land', excluding items 6 (Kernel-cell and effect visibility) and 8 (Fan-out caps as config). §5 groups 6–9 as 'unlock the experiment tier', so a reader must infer why 6/8 are skippable while 7/9 are hard blockers.
🟡 LOW 'pi's row' is a forward reference — no pi column exists in this matrix — docs/research/harness-compat.md
Line 37 says 'Do not inherit pi's row', but the Matrix section (lines 11-20) only has claude-code/codex/opencode columns; there is no pi row to inherit. The warning is coherent (pi is a distinct HarnessType per sandbox-backend.ts:32) but a reader must infer the row materializes when pi/prime columns are added.
🟡 LOW Retries deterministic failures — 404s and EACCES retried 3x with no error-class distinction — src/runtime/box-read-retry.ts
readBoxPathWithRetry retries EVERY failure up to the attempt cap with linear backoff, including deterministic ones (stable 404, EACCES). A harvest over N deleted surfaces at 3 attempts × 250ms base incurs N×750ms of dead backoff. The retry design intent (transient 404 on just-written files) is sound, but non-404 errors could be excluded.
🟡 LOW Unbounded concurrent read fan-out at settle — src/runtime/surface-diff.ts
Both Promise.all batches (line 141 mounts, line 158 watches) issue one read per path with no concurrency bound; a caller registering a large watch list fires that many concurrent box reads, each with up to 3 retries, at settle time.
🟡 LOW boxSurfaceReader retries after abort without a beforeAttempt guard — wastes a read on an abandoned run — src/runtime/surface-diff.ts
boxSurfaceReader passes
signalto readBoxPathWithRetry but NObeforeAttempt— so on abort, sleep() resolves early but the loop still issues the next read() call rather than short-circuiting. sandbox-run.ts:281 DOES supply beforeAttempt (throws SandboxRunAbortError); the harvest path does not.
🟡 LOW boxSurfaceReader's U+FFFD heuristic false-positives legitimate replacement characters — src/runtime/surface-diff.ts
The binary-detection guard
if (result.text.includes('\ufffd'))treats ANY U+FFFD in the decoded text as proof of lossy UTF-8 decoding and returnsunreadable. A legitimately-mounted UTF-8 file that contains an intentional U+FFFD will be reported unreadable at settle, dropping the diff signal.
🟡 LOW harvestSurfaceDiffs and boxSurfaceReader land without any production caller; integration unverified — src/runtime/surface-diff.ts
The only callers of harvestSurfaceDiffs / boxSurfaceReader / fsSurfaceReader are surface-diff.test.ts and the index.ts re-export — nothing in run-loop.ts, supervise-surface, or the bench worker invokes them yet. The SDK's NotFoundError shape it pattern-matches is unverified against the live SDK.
🟡 LOW pathKey only strips a leading './' — mid-path '.'/'..' and trailing slash are not canonicalized — src/runtime/surface-diff.ts
pathKey = (p) => p.replace(/^.//, '') reconciles './AGENTS.md' with 'AGENTS.md', but a mount recorded as 'a/b.md' and a watch/file-tree entry as 'a/./b.md' or 'a/x/../b.md' get distinct keys and produce duplicate/contradictory diffs.
tangletools · 2026-08-13T00:31:10Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 16 non-blocking findings — 1f5016f9
Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Verified all 20 reviewer fin
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-08-13T00:31:10Z · immutable trace
…t boundary Lexical containment let a symlink planted inside the root read host bytes through a contained-looking name: `readFile` follows the link, so the harvest could hash a file outside the worktree — the existence/hash oracle the contract says it prevents. Check containment twice: once lexically, then again on the link-resolved path. The root is link-resolved too, so a worktree or temp dir reached through a link still reads normally.
|
Review triage for the MEDIUM — MEDIUM ×2 — LOW — LOW — retries after abort waste a read. Fixed. The retry loop stops once the signal aborts. Refuted — Refuted — retrying deterministic failures. Retrying a 404 is the point: the box data plane 404s a file that exists but has not flushed, which is why Not taken, with reasons.
|
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 2 (1 low, 1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 262.1s (2 bridge agents) |
| Total | 262.1s |
💰 Value — sound
Ships a harness-agnostic surface-diff harvest (the read-back dual of the mount manifest) plus a shared box-read-retry, and registers the prime backend id — coherent, in-grain, deduplicates an existing inline retry, and the doc explicitly rejects the spec's over-engineered alternatives.
- What it does: Three things, against current main tip (5bcc1a7, 12 files / +1353): (1) Extracts the inline bounded-retry read loop from the deliverable read in sandbox-run.ts into a reusable
readBoxPathWithRetry(src/runtime/box-read-retry.ts), and refactors sandbox-run.ts:272-288 to use it. (2) AddsharvestSurfaceDiffs({mounts, read, watch?})+boxSurfaceReader+fsSurfaceReader(src/runtime/surface-dif - Goals it achieves: Capture agent self-edits to its own mounted profile surfaces (claude-code editing CLAUDE.md, opencode editing AGENTS.md, Prime's
/refine) that previously evaporated at box teardown — as session-scoped observations feeding the existing proposal pipeline, never auto-promoted. Secondarily: deduplicate the transient-404 retry pattern (now shared by the deliverable read and the surface reader) and re - Assessment: Clean and disciplined. The box-read-retry extraction is the right dedup — the deliverable read already had this retry inline, and the surface reader reuses it verbatim rather than reinventing. The harvest anchors on the existing
MountManifestEntry/RunProvenance.mountsseam (its stated 'read-back dual'), respecting the kernel-never-reads-workspace law: the caller supplies the read seam, mirrori - Better / existing approach: none — this is the right approach. Searched for existing surface/workspace-file diffing: none exists (grep for refine/selfEdit/surfaceDiff across src/ — the codebase's
refineis an unrelated retry STRATEGY in strategy.ts:828; improvement-cycle.ts:1002 hashes code patches, not mounted-surface self-edits). The mount manifest is the correct anchor and the change extends it as its dual rather than i - Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
Provider-identity evidence is fully wired end-to-end on the real billing path; the surface-diff harvest is a coherent, in-grain primitive landed ahead of its first documented caller; one now-write-only public field is the only wrinkle.
- Integration: Three pieces, all reachable. (1) Provider-identity rework: per-attempt evidence is recorded inside routerInlineExecutor/routerToolsInlineExecutor/bridgeExecutor via recordRuntimeOwnedProviderAttemptStart/Model/IdentityConflict (src/runtime/supervise/runtime.ts:614, 958, 1948), aggregated per-executor (materialization.ts runtimeOwnedExecutorProviderEvidence), projected onto the root drive (recordRu
- Fit with existing patterns: Fits established patterns, does not compete with them. harvestSurfaceDiffs mirrors the recordMount law verbatim — 'the kernel never reads workspace contents itself; the caller supplies the read seam' (surface-diff.ts:18-21, types.ts:77-84) — so it is the read-back dual of the existing mount manifest, not a parallel scheme. boxSurfaceReader rides the same box.fs.read seam openSandboxRun reads deliv
- Real-world viability: Hardened beyond the happy path. The harvest contains a reader that throws (surface-diff.ts:101 readOutcome), distinguishes a box-gone transport failure from a file-gone deletion via NotFoundError.resourceType so that 'the agent deleted every surface' cannot be recorded for a dead box (surface-diff.ts:250-263), reports lossy UTF-8 decodes as unreadable rather than hashing mojibake (surface-diff.ts:
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🔎 Heuristic Signals
🟡 Cruft: magic number added src/runtime/sandbox-run.ts
delayMs: options.readRetryDelayMs ?? 1000,
🎯 Usefulness Audit
🟡 SupervisedResult.rootProviderModel is now write-only in this repo [ergonomics] ``
After this PR, supervise.ts:1878-1890 still computes and stamps rootProviderModel and the field remains on all three SupervisedResult arms (types.ts:1187,1218,1246), but the dispatch identity path switched to providerModel (loop-dispatch.ts supervisedTreeModelForDispatch reads result.providerModel, not rootProviderModel). grep confirms no production reader of .rootProviderModel remains outside the test fixtures (tests/kernel/loop-dispatch.test.ts:480-516) and generated docs. The two fields can n
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.
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 74 | 71 | 64 | 64 |
| Confidence | 85 | 85 | 85 | 85 |
| Correctness | 74 | 71 | 64 | 64 |
| Security | 74 | 71 | 64 | 64 |
| Testing | 74 | 71 | 64 | 64 |
| Architecture | 74 | 71 | 64 | 64 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM fsSurfaceReader: a missing or deleted worktree root reports every mount as 'removed' — src/runtime/surface-diff.ts
resolvedRoot ??= await realpath(lexicalRoot)runs inside the try; if the root itself does not exist (worktree deleted, cleanup ran, wrong root passed), realpath throws ENOENT and the catch mapscode === 'ENOENT'tomissing: true. harvestSurfaceDiffs then reports EVERY mounted surface asremoved— 'the agent deleted everything' — instead of anunreadabletransport/config failure. This is the exact false-signal the module's own box reader guards against (transport-gone → unreadable) and it feeds the improvement-evidence pipeline. Empirically reproduced: fsSurfaceReader on a nonexistent root returned {missing:true} for a single read and the harvest returned [['a.md','removed'],['b.md','removed']]. Fix: distinguish root-absence from target-absence (e.g. resolve the root once, map roo
🟡 LOW Experimental API documented without an Experimental badge — docs/api/runtime.md
The whole surface-diff module is tagged @experimental (src/runtime/surface-diff.ts:23), but none of the 10 newly documented symbols (SurfaceDiff, WatchedSurface, HarvestSurfaceDiffsOptions, SurfaceReadBox, BoxSurfaceReaderOptions, SurfaceReader, SurfaceReadOutcome, harvestSurfaceDiffs, boxSurfaceReader, fsSurfaceReader) render the
**Experimental**badge that per-symbol-tagged APIs carry in the same file (e.g. SandboxRunAbortError at runtime.md:486, whose JSDoc has @experimental at sandbox-run.ts:76). Generator-consistent (module-level tags do not cascade), but the committed public API doc presents a new, explicitly-unstable API as stable. Fix: add @experimental to each exported symbol's JSDoc in src/runtime/surface-diff.ts and regenerate.
🟡 LOW SurfaceDiff.mountedSha256 'absent' list is incomplete — docs/api/runtime.md
Doc reads 'Absent for
created', implying present for the other three statuses. Source (src/runtime/surface-diff.ts:178-183) also omits mountedSha256 on a watch-originunreadablediff (a watched path whose read failed carries no mount). Fix: change to 'Absent forcreatedand for watch-origin diffs; present only on mount-origin diffs.'
🟡 LOW boxSurfaceReader doc overstates which NotFoundError resourceTypes count as transport failure — docs/api/runtime.md
Doc: 'maps to
missing: true— unless itsresourceTypenames something other than a file/path (the BOX or session being gone)'. Implementation (src/runtime/surface-diff.ts:250-265) only treats resourceType matching /sandbox|session/i as transport-gone; every other resourceType — including the SDK's default 'Resource' when the server names nothing — is classifiedmissing: true(a deletion). The doc's literal reading implies resourceType 'Resource' is a transport failure, contradicting the implemented and tested behavior (surface-diff.test.ts:238 'treats the SDK default Resource resourceType as a missing FILE'). Fix: reword to 'unless itsresourceTypenames the sandbox or session'.
🟡 LOW boxSurfaceReader doc says 'BOX' but regex matches 'sandbox' literally — docs/api/runtime.md
The prose describes transport-gone detection as 'its resourceType names something other than a file/path (the BOX or session being gone)'. The implementation at surface-diff.ts:260 uses /sandbox|session/i -- if the SDK ever returned resourceType:'Box' (not 'Sandbox'), it would NOT be classified as transportGone, contradicting the doc's implication. In practice the SDK uses 'Sandbox' (test at surface-diff.test.ts:283) and 'box'/'sandbox' are codebase synonyms, so this is terminology precision only. No behavioral risk. Fix: say 'sandbox or session' in the parenthetical to match the regex literally.
🟡 LOW 'three enums' overcounts the compile-time pin — docs/design/prime-agent-harness-integration.md
Text reads 'the double-
satisfiespin keeps the three enums aligned at compile time'. The actual pin in src/runtime/sandbox-backend.ts:30-45 isconst harnessBackends = [...] as const satisfies readonly HarnessType[] satisfies readonly BackendType[]— two type-level enums (HarnessType and BackendType), with the const array as the pinning literal. No third enum. Fix: 'keeps the two enums aligned' (or name the literal array explicitly).
🟡 LOW Awkward line wrap in status block — docs/design/prime-agent-harness-integration.md
Line 7 ends 'the harness id shipped' and line 8 resumes 'as
primeinagent-interface...' mid-sentence across the blockquote line break. Cosmetic only; reflow so the sentence reads cleanly within the status callout.
🟡 LOW Harvest attribution slightly overstated vs. code's own caveat — docs/design/prime-agent-harness-integration.md
Doc says harvestSurfaceDiffs 'reports what the agent itself changed'. The module's own docstring (src/runtime/surface-diff.ts:72-74) states
created'is an inference from the mount manifest, not a proof of authorship: a file the box IMAGE shipped at a never-mounted path also reportscreated'. The doc later describescreatedsemantics accurately, but the opening sentence over-attributes. Fix: soften to 'reports which surfaces differ at settle' or add the image-shipped caveat to §3.1.
🟡 LOW §4 conservation claim overstates the current invariant — docs/design/prime-agent-harness-integration.md
§4 argues TopologyAuthority is structurally unnecessary because 'a worker's native subagents spend inside its conserved-budget reservation (supervise/budget.ts), so no arm can buy more compute regardless of what the harness does internally.' The conserved pool only meters what flows through the runtime's UsageEvent stream (budget.ts: reserve/reconcile/observe); spend that never surfaces as a usage event is invisible to it. The doc itself concedes the precondition is unbuilt: wish-list item 4 (line ~157) says without native-session-tree exposure 'a Prime worker is one opaque blob and per-child cost attribution is lost.' So the no-TopologyAuthority rationale rests on conservation that is not yet guaranteed for Prime's native rlm children. Suggest softening to 'conservation holds once native-
🟡 LOW 'pi' referenced without definition in this doc — docs/research/harness-compat.md
The sentence 'Do not inherit
pi's row' assumes the reader knowspiis a separate harness column. The current matrix (lines 11-20) has nopicolumn andpiis never defined elsewhere in this file. A reader landing here cold cannot tell whetherpiis Prime Intellect's upstream, a prior internal name, or a typo forprime. Impact: minor readability regression in a research note; no behavioral risk since the instruction is conservative (forbid inheritance, require re-measurement). Fix: add a one-clause gloss on first use, e.g. 'Do not inherit thepi(upstream Prime Intellect CLI) row — ...'.
🟡 LOW First read attempt is still spent on an already-aborted run — src/runtime/box-read-retry.ts
The module doc states an abandoned run spends "no further attempt", but the abort check lives only in the catch branch (after a failed read). With no
beforeAttempt(the boxSurfaceReader case), a signal already aborted before the first iteration still triggers the firstread(path)call. ThebeforeAttemptcallback is invoked first and can throw (which is how sandbox-run.ts avoids the wasted read), but boxSurfaceReader passes no such callback, so its first attempt runs despite the run being abandoned. Not a correctness bug (a typed outcome is still returned as documented), but the doc overstates the guarantee. Cheap fix: checksignal?.abortedbefore the first read attempt as well.
🟡 LOW readBoxPathWithRetry invokes beforeAttempt twice for the same failed attempt on abort — src/runtime/box-read-retry.ts
On abort after a failed attempt, beforeAttempt runs once at the top of the attempt and again in the post-sleep abort guard (lines 51 and 61) before break. Empirically verified: with an aborting reader, invocations were [undefined, 'transient'] for one read. Harmless for the two current callers (sandbox-run's callback throws on abort so the second call is what aborts; boxSurfaceReader passes none), but a non-throwing beforeAttempt would observe the same failure twice. Document or dedupe.
🟡 LOW No test pins the deliverable-read refactor's abort/readError equivalence — src/runtime/sandbox-run.ts
The inline 4-attempt retry loop in openSandboxRun was replaced by readBoxPathWithRetry. The new unit tests cover readBoxPathWithRetry's success/failure/beforeAttempt-throw/attempt-clamp in isolation, and boxSurfaceReader's abort-returns-outcome path, but no test exercises the deliverable path's abort-during-backoff behavior — i.e. that a signal aborting mid-sleep raises SandboxRunAbortError(collected, lastReadError) with the correct
readError. The equivalence (backoff 1000/2000/3000ms, readError of the last attempt, abort throws with the last error) was verified by reading both implementations, but is not regression-pinned. A single test covering abort-during-ret
🟡 LOW sandbox-run: abort landing during the final read attempt now throws instead of returning a TurnResult — src/runtime/sandbox-run.ts
The old loop only checked
signal.abortedat the top of each attempt, so an abort that fired during the 4th (final) read attempt — and that read rejected — returned a TurnResult with readError set. The new readBoxPathWithRetry post-failure abort guard throws SandboxRunAbortError in that same window. This is a tightening (abort now always wins) and all in-repo callers (bench gate/worker) already handle SandboxRunAbortError, but it is a semantic change to the deliverable path worth confirming against external openSandboxRun consumers.
🟡 LOW 'removed' classification depends on unverified SDK error-shape contract — src/runtime/surface-diff.ts
missing: true(and therefore theremovedharvest status) is derived solely fromerr.name === 'NotFoundError'combined with the comment's claim that the SDK's HTTP mapper defaultsresourceTypeto'Resource'for a file 404. If the real sandbox SDK surfaces file-not-found with a different error name, or namesresourceTypewith a file-ish value that the/sandbox|session/iregex doesn't exclude, a genuine agent deletion is reportedunreadable(a transport/failure signal) instead ofremoved. This is a load-bearing assumption about an external package that is mocked in tests but not asserted against the real SDK. Recommend an integration test against the actual sandbox client, or widen the not-found detection (status code / message) to reduce contract fragility.
🟡 LOW New surface-diff module has no production caller in this PR — src/runtime/surface-diff.ts
harvestSurfaceDiffs,boxSurfaceReader, andfsSurfaceReaderare exported from src/runtime/index.ts but nothing outside the test file imports them. Confirmed via ripgrep across src/, tests/, examples/, bench/. This is the foundational primitive landing before its integration PR — the docstring ties it to a futureproposeAgentImprovementpipeline — but it's effectively dead library surface until that lands. Acceptable pattern for staged delivery; flag so reviewers know the integration shakeout (real mount manifests from run-loop.ts, real box.fs.read behavior under load, real path-form skew between mount recorder and file-tree enumeration) is still ahead, not behind.
🟡 LOW TOCTOU window between realpath containment check and readFile — src/runtime/surface-diff.ts
realpath(target)validates the resolved path is insiderealpath(lexicalRoot), thenreadFile(resolvedTarget)follows symlinks again. An attacker with write access to root (which the agent has in the worktree model) can swapresolvedTargetfor a symlink to a host file between the two calls, reading host bytes through a contained-looking name. Impact is bounded — the harvest only emits a SHA-256 hash and byte count, not raw contents, so the leak is a partial existence/size/hash oracle rather than content exfiltration. Hardening:open(resolvedTarget, O_NOFOLLOW)+fstatregular-file check, orreadFilewith{ flag: 'r' }after re-validating viafstaton the fd. The current defense is reasonable for the threat model but worth a follow-up.
🟡 LOW U+FFFD heuristic rejects legitimate text containing the replacement char — src/runtime/surface-diff.ts
result.text.includes('�')flags any U+FFFD as a lossy decode. A valid UTF-8 file that legitimately contains U+FFFD (e.g., a doc discussing encoding, a file that already had mojibake written by the agent) is reportedunreadablewithmissing:falseinstead of hashed. The comment acknowledges the tradeoff but the false-positive path is real and silent. Tighter check: validate viaBuffer.from(text).includes(Buffer.from([0xef,0xbf,0xbd]))AND cross-check that TextDecoder fatal-mode rejects the original bytes — or expose an opt-out for callers who know their surface is text-with-replacement-chars. Low impact: surfaces fall back tounreadable, harvest still reports the diff status, just without a hash.
🟡 LOW boxSurfaceReader: bare 404 on a dead sandbox can still classify as missing → bulk 'removed' — src/runtime/surface-diff.ts
The transport-gone guard is
/sandbox|session/i.test(resourceType). Verified in @tangle-network/sandbox@0.21.1 errors-D2aWy99l.js parseErrorResponse:new NotFoundError(data.resourceType || 'Resource', ...)— the type comes from the SERVER body and defaults to 'Resource'. If /files/read returns a bare 404 when the sandbox is gone (server omits resourceType), transportGone is false, notFound is true, and every surface reportsremoved. The 'Sandbox'-named case is guarded (and pinned by a test) but depends on the server naming the type. Consider also treating 'Resource'/'unknown' resourceId with a box-level error message (e.g. message containing the sandbox id) as transport-gone, or documenting this as a known false-positive bound.
🟡 LOW fsSurfaceReader: TOCTOU window between symlink-resolved realpath and readFile — src/runtime/surface-diff.ts
realpath(target)resolves the full link chain, thenreadFile(resolvedTarget)runs on the resolved path. A concurrent swap of the file at resolvedTarget for a symlink to a host path between the two calls is read by readFile, leaking host bytes into the settled hash/size reported by the harvest. Impact is bounded (read-only, caller-owned worktree, diff evidence only) and the planted-symlink-at-rest case is correctly rejected and tested; a dirfd/openat approach would close the race. Low severity; not a merge blocker.
🟡 LOW mojibake sentinel ('�') is a brittle binary detector — src/runtime/surface-diff.ts
Binary surfaces are rejected by testing
result.text.includes('�'). Two failure modes: (1) a legitimate text surface that genuinely contains a U+FFFD replacement character (rare but legal UTF-8) is misreportedunreadable; (2) if the box SDK's text wire decodes with something other than U+FFFD substitution (e.g. latin1/String.fromCharCodeor?replacement), lossy binary content will NOT contain '�' and instead be silently re-encoded and hashed as mojibake — producing a falsemodified/created. The code comments acknowledge the lossy-decode risk but the detection heuristic's correctness depends on an unverified decoder contract in @tangle-network/sandbox. Consider reading via the SDK's byte path (download()) when available, or treating this as a documented best-effort with a stro
🟡 LOW mountedSha256 casing is not normalized in diff output — src/runtime/surface-diff.ts
The equality check lowercases the manifest hash (line 161
entry.sha256.toLowerCase()), but the diff objects emitmountedSha256: entry.sha256verbatim (lines 149/155/165) whilesettledSha256is always lowercase hex. A manifest stored with an uppercase hash (explicitly supported — the test at surface-diff.test.ts 'treats an uppercase manifest hash as equal') yields amodifieddiff whosemountedSha256is uppercase andsettledSha256lowercase. Any downstream consumer that string-compares the two fields (rather than recomputing
🟡 LOW transportGone regex coupled to current SDK resourceType vocabulary — src/runtime/surface-diff.ts
/sandbox|session/imatches the SDK's currentNotFoundErrorresourceType values (Sandbox,Session,Interactive session). Verified against node_modules/@tangle-network/sandbox/dist — these are the only transport-level names emitted today. A future SDK version emitting a synonym (e.g.,Box,Workspace,Container,Instance) would misclassify a dead-box 404 as a file deletion, and the harvest would report every mounted surface asremoved. The inverse allowlist (treat anything-not-file-ish as transport) would be more robust to vocabulary drift; the current denylist is correct-for-now. Comment at lines 255-259 documents the reasoning, so this is
tangletools · 2026-08-13T00:52:40Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 23 non-blocking findings — 0327f7f7
Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 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-08-13T00:52:40Z · immutable trace
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 41 | 80 | 62 | 41 |
| Confidence | 85 | 85 | 85 | 85 |
| Correctness | 41 | 80 | 62 | 41 |
| Security | 41 | 80 | 62 | 41 |
| Testing | 41 | 80 | 62 | 41 |
| Architecture | 41 | 80 | 62 | 41 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM New cross-reference #signal-23 points to wrong property — docs/api/runtime.md
The new harvestSurfaceDiffs prose says '(BoxSurfaceReaderOptions.signal, or close over one...)'. BoxSurfaceReaderOptions.signal? is the 22nd signal heading -> anchor #signal-21, not #signal-23. The value #signal-23 resolves to occurrence 24 (an unrelated interface's signal at line 18277). The correct anchor is #signal-21. This is consistent with the TypeDoc-not-regenerated root cause: the anchor number was computed as if BoxSurfaceReaderOptions were appended at the end of the document rather than inserted mid-document. Fix: change #signal-23 to #signal-21, or regenerate TypeDoc.
🟠 MEDIUM Stale anchor #signal-21 broken by new BoxSurfaceReaderOptions.signal? heading — docs/api/runtime.md
The diff inserts BoxSurfaceReaderOptions.signal? at line 17050 as the 22nd 'signal'/'signal?' heading (both normalize to GitHub anchor 'signal'). Before this PR, RootHandle.signal at line 17199 was occurrence 22 -> #signal-21, and the cross-reference at line 16171 (
RootHandle.signal) correctly resolved to it. After this PR, occurrence 22 is now BoxSurfaceReaderOptions.signal?, so #signal-21
🟠 MEDIUM Stale anchor #status-12 broken by new SurfaceDiff.status heading — docs/api/runtime.md
The diff inserts SurfaceDiff.status at line 16912 as the 13th 'status' heading. Before this PR, NodeSnapshot.status (now at line 18392) was occurrence 13 -> #status-12, and the cross-reference at line 879 (
NodeSnapshot.status) correctly resolved to it. After this PR, occurrence 13 is SurfaceDiff.status, so #status-12 resolves to the wrong property. NodeSnapshot.status shifted to occurrence 14
🟡 LOW boxSurfaceReader doc overstates which resourceTypes count as a transport failure — docs/api/runtime.md
Doc says NotFoundError maps to missing 'unless its resourceType names something other than a file/path'. Implementation (src/runtime/surface-diff.ts:260) only treats resourceTypes matching /sandbox|session/i as transport-gone; any other resourceType (e.g. 'Blob', 'Memory', 'Agent') still yields missing: true and a
removeddiff. The parenthetical '(the BOX or session being gone)' is accurate; the 'something other than a file/path' clause is broader than the code. In practice the SDK only produces 'Resource' | sandbox/session types, so impact is limited to custom readers/doubles. Fix: reword the doc to name the sandbox/session exception exactly as the code does.
🟡 LOW fsSurfaceReader containment guarantee is lexical; symlinks inside root are followed — docs/api/runtime.md
Doc claims a manifest path 'cannot turn the harvest into an existence/hash oracle over the host filesystem'. The check (src/runtime/surface-diff.ts:280-287) is a lexical resolve() comparison only, and readFile() follows symlinks; no realpath() guard exists. A symlink planted inside the worktree (agent- or image-authored) pointing at a host file passes containment and its bytes are hashed into settledSha256/settledBytes. The stated threat (persisted/mistyped relative-or-absolute path strings) is indeed blocked, so this is a wording overstatement rather than a live bug. Fix: qualify the guarantee ('lexical escapes') or add a realpath containment check if host-file exposure is unacceptable for worktree workers.
🟡 LOW Doc references nonexistent symbol heldOutGate — docs/design/prime-agent-harness-integration.md
The anti-reinvention map row 'Equal-compute topology comparison + holdout gates' lists
heldOutGateas an existing primitive.grep -rn heldOutGateacross the repo returns nothing. The real holdout gate isheldoutSignificance(imported from @tangle-network/agent-eval/campaign and invoked insidepromotionGate, promotion-gate.ts:99) pluspromotionGate/pairedLift. Impact: a reader grepping for the named symbol will not find it. Fix: rename toheldoutSignificance(and note it lives in agent-eval, not the runtime).
🟡 LOW Doc softens the code's own 'created is an inference, not authorship proof' caveat — docs/design/prime-agent-harness-integration.md
Doc says watch entries let 'never-mounted paths that now exist report as created' and instructs the caller to enumerate paths 'via the box file tree'. The implementation's JSDoc (src/runtime/surface-diff.ts:72-75) is more careful: 'created is an inference from the mount manifest, not a proof of authorship: a file the box IMAGE shipped at a never-mounted path also reports created. Watch paths known absent at run start (or enumerate the tree at start AND settle and watch the difference) to make the label mean what it says.' The doc omits the known-absent-at-run-start requirement; a reader following 'enumerate via the box file tree' will get false created labels for image-shipped files. Fix: carry the code's caveat into §3.1.
🟡 LOW Markdown sentences packed onto shared physical lines (violates repo one-sentence-per-line convention) — docs/design/prime-agent-harness-integration.md
Shared AGENTS.md mandates: 'When writing or substantially editing long Markdown files, put each full sentence on its own physical line while preserving normal Markdown structure.' Violated in (a) the status blockquote lines 3-10 — e.g. line 7 runs '...the adapter; the harness id shipped as prime in agent-interface and the sandbox backend enum. This doc owns the Runtime side: ...' (three sentences on one physical line); (b) wish-list item 9 at [lines 175-184](https://github.com/tangle-network/agent
🟡 LOW §4 'structurally impossible' overstates the conservation guarantee, contradicting §5 item 4 — docs/design/prime-agent-harness-integration.md
§4 claims native subagents 'spend inside its conserved-budget reservation ... so no arm can buy more compute regardless of what the harness does internally'. This holds only for spend that reaches the budget pool's usage stream. Wish-list item 4 (line 156-157) admits 'without it a Prime worker is one opaque blob and per-child cost attribution is lost' — i.e. native-child compute is NOT currently metered, so an arm can in principle consume more real compute than its ledger shows. The doc's own conclusion (needs observability + fan-out caps, not a new authority type) is fine; only the 'structurally impossible' framing is overstated. Suggest
🟡 LOW Cross-reference numbering mismatch: 'runaway surface #3' vs 'second runaway surface' — docs/research/harness-compat.md
Warning item #3 (line 26) explicitly calls sub-agent fan-out 'the second runaway surface', but the new prime text refers to the same topic as 'runaway surface #3'. The #3 is the warning-list position, not the runaway-surface ordinal, so the two labels disagree. Fix: say 'runaway surface #3 in the warnings list' or renumber consistently.
🟡 LOW Dangling reference: no pi row exists in this matrix — docs/research/harness-compat.md
The text says 'Do not inherit
pi's row', but the capability matrix in this file (lines 11-20) has only claude-code/codex/opencode columns — there is nopirow/column anywhere in the doc. The warning mirrors the design doc's 'do not inherit pi's capability row by assumption' (prime-agent-harness-integration.md:95), but in this file a reader has nothing to find the row in. Fix: add thepicolumn to the matrix, or reword to 'do not inheritpi's capability claims'.
🟡 LOW Load-bearing divergence claim stated without provenance — docs/research/harness-compat.md
'its daemon rejects pi-line clients' is the justification for 'do not inherit pi's row' but carries no citation or measurement. The surrounding sentence ('every cell must be measured against the real CLI') hedges downstream cells but not this specific assertion. Consider tagging it as unverified hypothesis or attaching a source/experiment ID for traceability.
🟡 LOW Long single-bullet packs three distinct claims — docs/research/harness-compat.md
The 'prime' bullet interleaves (a) what Prime is, (b) where the id is wired, and (c) the 'do not inherit pi's row' divergence warning plus the three deltas to verify. Splitting into sub-bullets (id/links, non-inheritance caveat, deltas-to-verify) would match the matrix's scannable style. Readability nit only.
🟡 LOW Sentence-fragment grammar: 'The id shipped in' — docs/research/harness-compat.md
'The id shipped in
agent-interfaceHarnessType+ the sandbox backend enum' reads as a fragment; 'The id ships in' or 'The id is shipped in' would parse cleanly. Minor STE/clarity nit; the meaning is recoverable from context.
🟡 LOW An already-aborted signal still spends one read per surface in the harvest — src/runtime/box-read-retry.ts
With no beforeAttempt supplied (boxSurfaceReader passes none), an already-aborted signal does not prevent the first read(path) call; abort is only honored after the first failure (the post-catch check at line 60). harvestSurfaceDiffs takes no AbortSignal by design and relies on the reader, so an aborted harvest still performs one box read per surface and honors no signal during in-flight reads. Documented contract ('stops retrying' not 'never starts') and consistent with the abort-during-read test (expects 1 call), but worth one line in BoxSurfaceReaderOptions docs so callers pass an un-aborted signal or accept the single spent read.
🟡 LOW beforeAttempt is invoked twice on the abort path — src/runtime/box-read-retry.ts
On an aborted run
beforeAttemptruns once at the top of the attempt (line 51, with the prior error) and again inside the abort branch (line 61, with the current error), so a non-idempotent callback fires twice for one attempt. The only production caller (sandbox-run.ts) passes an idempotent abort-check, so there is no live bug today, but the documented contract ('runs before every attempt, carrying the previous attempt's error') is violated in the abort path and would bite a future caller with side effects.
🟡 LOW Abort during the final read attempt now throws SandboxRunAbortError where the old loop returned an empty deliverable — src/runtime/sandbox-run.ts
Old inline loop only re-checked options.signal.aborted at the TOP of each iteration; an abort firing during the last (4th) read attempt exited the loop and returned a normal TurnResult with raw='' and readError. The shared helper re-checks after the final catch (box-read-retry.ts:60 'if (options.signal?.aborted)') and calls beforeAttempt, so settle() now throws SandboxRunAbortError(collected, readError) for that window: failed=true, agent.turn/agent.run error events, exception propagated. This matches the helper's documented intent ('an abandoned run earns no further attempts... keeps abort authority') and is arguably a fix, but it is an observable contract change for callers that previously got a clean result on late-cancel. No test covers the abort-during-final-attempt path in either mod
🟡 LOW NotFoundError/resourceType contract unverified against the real @tangle-network/sandbox SDK — src/runtime/surface-diff.ts
The entire
removedstatus depends onerr.name === 'NotFoundError'(line 250) plus theresourceTypeheuristic (lines 251-260), justified by the comment 'the SDK's HTTP mapper builds new NotFoundError(data.resourceType || 'Resource', ...)'. Every test mocks this shape (surface-diff.test.ts creates plain Errors and sets.name/.resourceType), so the tests only prove the code against the author's assumed shape, not the real SDK. The SDK is an external catalog dep (@tangle-network/sandbox@0.21.1) and cannot be inspected in th
🟡 LOW U+FFFD check makes legitimately-FFFD content report a permanent unreadable diff — src/runtime/surface-diff.ts
result.text.includes('\uFFFD') flags any surface whose content legitimately contains a U+FFFD replacement character. If the mounted file's ORIGINAL bytes contained U+FFFD (e.g. a memory file that inherited one), the settled read reports { status: 'unreadable', error: 'not valid UTF-8' } at every settle even when the agent never touched the file — a durable spurious diff and a false-negative on the mount-vs-settled comparison. The guard correctly prevents hashing lossy-decoded mojibake as a false 'modified', but it cannot distinguish original-FFFD from lossy-decode-FFFD. Acceptable documented tradeoff; severity low because profile surfaces are normally clean UTF-8.
🟡 LOW U+FFFD sentinel misclassifies legitimate text containing the replacement char — src/runtime/surface-diff.ts
result.text.includes('�')treats any file whose content legitimately contains U+FFFD (a valid Unicode scalar a user/agent may actually write) as 'not valid UTF-8' and reports itunreadable, hashing nothing. Conversely, a binary file that happens to lossy-decode without a replacement char would slip through and be hashed as mojibake. It is a documented, deterministic heuristic and retry-cannot-recover logic is sound; flagged as a false-positive edge, not a blocker.
🟡 LOW U+FFFD substring check is a false-positive heuristic for binary-ish text surfaces — src/runtime/surface-diff.ts
if (result.text.includes('\uFFFD'))cannot distinguish 'the box wire lossy-decoded non-UTF-8 bytes' from 'the file legitimately contains a U+FFFD character' (replacement char as intentional content, or a normal-form text that happens to include it). A profile/memory surface containing a literal U+FFFD would be reportedunreadableinstead ofmodified/unchanged, silently dropping it from the diff. The doc-comment frames this as an intentional tradeoff (prefer false-unreadable over false-modified-mojibake), but the contract offers no override for callers who know their content is text-with-FFFD. Low impact for typical ASCII/UTF-8 profile surfaces; flag for awareness on corpus mounts that may contain replacement chars.
🟡 LOW fsSurfaceReader containment check is lexical; readFile follows symlinks, defeating the stated oracle guarantee — src/runtime/surface-diff.ts
The containment check at line 281 (target = resolve(...), then target.startsWith(boundary + sep)) only constrains the resolved string. readFile(target) follows symlinks by default, so a symlink an evaluated agent created at a mounted/watched path (root/CLAUDE.md -> /etc/passwd) passes the lexical check and the harvest reads, hashes, and reports the host file's content — exactly the 'existence/hash oracle over the host filesystem' the docstring at line 273 claims to prevent. This only affects the local/worktree reader (the sandbox path
🟡 LOW fsSurfaceReader containment is lexical, not realpath — symlink escapes root — src/runtime/surface-diff.ts
The documented guarantee is that 'a persisted or mistyped manifest path cannot turn the harvest into an existence/hash oracle over the host filesystem' (doc block lines ~283-292). But containment uses path.resolve() only (line 280-281), which is purely lexical, and readFile() (line 289) follows symlinks. A symlink inside
root(e.g. root/link -> /etc) combined with a manifest path oflink/passwdresolves lexically toroot/link/passwd, passes thestartsWith(boundary + sep)check, then readFile follows the link and hashes `/
🟡 LOW fsSurfaceReader containment uses resolve() — symlinks inside root escape, no realpath check — src/runtime/surface-diff.ts
The boundary check
target !== boundary && !target.startsWith(boundary + sep)usesresolve()which does NOT resolve symlinks. A symlink placed atroot/evil -> /etcwould pass the containment check (the lexical path is inside root) andreadFilewould follow it outside. Practical risk is low because this reader targets worktree-local workers where the agent already has direct FS access to the worktree (so symlink traversal is not an escalation), but if this reader is ever reused for a more confined context (e.g. untrusted manifest paths from a remote source), the containment should switch tofs.realpath(). No fix needed for current callers.
🟡 LOW harvestSurfaceDiffs runs mount reads and watch reads as two sequential Promise.all groups, not one concurrent fan-out — src/runtime/surface-diff.ts
mountDiffs = await Promise.all([...])is awaited beforewatchDiffs = await Promise.all([...])is even started. The JSDoc says 'reads run concurrently' which is true within each group but the two groups are serialized, so wall-time is max(mounts) + max(watches) rather than max(all). For a harvest over many surfaces through a retrying box reader this roughly doubles tail latency. Ordering is preserved by concatenation, but the same order could be achieved with a single Promise.all plus a sort by a (group,index) key. Minor efficiency gap; not a correctness issue.
🟡 LOW pathKey only strips a single leading './' — interior normalization gaps can split one surface into two entries — src/runtime/surface-diff.ts
p.replace(/^\.\//, '')does not handlefoo/./bar,foo//bar,foo/../bar, trailing slashes, or././AGENTS.md(strips only one./). A manifest mixing forms (e.g. one mount recordsa/./b.md, another recordsa/b.md) would create two byPath entries and the second would not dedupe the first. The doc explicitly defers absolute-vs-relative reconciliation to the caller, but interior-segment normalization is not called out. Low impact today because mount recorders emit consistent forms; worth a one-line note in the WatchedSurface/mounts doc or a stronger normalize (path.normalize minus leading './') if manifests ever bridge recorders.
🟡 LOW resourceType substring test /sandbox|session/i can misclassify a deleted file as unreadable — src/runtime/surface-diff.ts
transportGone is true for ANY resourceType containing 'sandbox' or 'session' (case-insensitive substring). The SDK 404 mapper defaults unknown types to 'Resource' (verified: errors-Bf-A-4SA.js:294 builds NotFoundError(data.resourceType || 'Resource', ...)), so ordinary deletions are correctly reported 'removed'. But if the server ever names a FILE resource type containing those words (e.g. 'SandboxFile' or 'SandboxWorkspacePath'), a genuine deletion is classified as a transport failure and the harvest can never report 'removed' for it — the same symptom the inverted heuristic at line 255-259 exists to avoid. The regex is a guess at the server's taxonomy; pr
tangletools · 2026-08-13T00:53:05Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 27 non-blocking findings — 15a1f64a
Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 5/5 planned shots over 12 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-08-13T00:53:05Z · immutable trace
…removed evidence Two ways the harvest could invent agent deletions: A missing or unreadable reader root made every mount report `removed` — the filesystem twin of a dead box answering for each path. The root is now classified once: gone means `unreadable`. The retry guard also sat after the read, so an already-cancelled run still spent an attempt and `beforeAttempt` ran twice for the same failed attempt. Move the guard between `beforeAttempt` and the read: one call per attempt, no read on an abandoned run, and an abort landing during the final attempt again returns the turn with its `readError` instead of throwing.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Concerns | 2 (1 low, 1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 130.2s (2 bridge agents) |
| Total | 130.2s |
💰 Value — sound-with-nits
Lands a coherent read-back dual to the mount manifest (surface-diff harvest), extracts the inline box-read retry into a shared primitive, and registers the prime harness id; the harvest ships as an untested-by-production primitive awaiting its first caller.
- What it does: Three things. (1) Extracts the transient-404 retry loop that was inline in openSandboxRun's artifact read into a reusable
readBoxPathWithRetry(src/runtime/box-read-retry.ts) with a typed outcome, abort handling, and a beforeAttempt hook — then reuses it in BOTH the deliverable read (src/runtime/sandbox-run.ts:277) and the new box surface reader. (2) AddsharvestSurfaceDiffs(src/runtime/surfa - Goals it achieves: (1) Capture evidence that currently evaporates: which profile surfaces (instructions, memory, skills) a harness self-edited during a run — the harness-agnostic generalization of Prime's
/refine, claude-code editing CLAUDE.md, opencode editing AGENTS.md. The mount manifest records what went IN; nothing recorded what came BACK. (2) Eliminate the duplicated retry logic by extracting one primitive b - Assessment: Coherent and in the grain. The harvest is the correct read-back dual to
RunProvenance.mounts(src/runtime/types.ts:192) — same shape (path + sha256 + bytes + source), same law (the kernel never reads workspace contents; the caller supplies the read seam, mirroring howrecordMountkeeps hashing with the byte owner). ThereadBoxPathWithRetryextraction is a clean deduplication: the surface rea - Better / existing approach: none — this is the right approach. Searched for: (a) an existing read-back/diff primitive over mounted surfaces — grep for harvestSurfaceDiffs/surface-diff/settled/workspace bytes found only this module and its tests; (b) overlap with candidate-execution's captureAgentCandidateWorkspace/buildAgentCandidateBundle — that system (src/candidate-execution/workspace-archive.ts) is a heavy full-workspace
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
A coherent harness-agnostic surface-diff harvest (the read-back dual of the existing mount manifest) plus a shared retry primitive immediately wired into the deliverable read path, with the 'prime' harness id backed by real upstream types.
- Integration: The retry extraction (box-read-retry.ts) is immediately consumed by sandbox-run.ts:277 (replacing the prior inline retry loop) and by boxSurfaceReader in surface-diff.ts:232 — two callers with the same transient-404 posture over the same box.fs.read seam. harvestSurfaceDiffs is exported from index.ts:989-996 but has no production caller in this PR; its input (RunProvenance.mounts) is populated by
- Fit with existing patterns: harvestSurfaceDiffs is explicitly the read-back dual of RunProvenance.mounts — it consumes the exact same MountManifestEntry type (surface-diff.ts:87, types.ts:192-194) and follows the kernel's established 'never reads workspace contents' law by taking a caller-supplied reader seam (surface-diff.ts:18-21), mirroring how recordMount keeps hashing with the byte owner (run-loop.ts:178-185). grep for
- Real-world viability: Reads run concurrently via Promise.all (surface-diff.ts:141,172). A reader that throws is contained by readOutcome (surface-diff.ts:101-111) so one bad path reports unreadable without rejecting the whole harvest. Abort is handled correctly: boxSurfaceReader threads the signal into readBoxPathWithRetry (surface-diff.ts:235), and sandbox-run.ts uses beforeAttempt to raise its own SandboxRunAbortErro
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
🔎 Heuristic Signals
🟡 Cruft: magic number added src/runtime/sandbox-run.ts
delayMs: options.readRetryDelayMs ?? 1000,
💰 Value Audit
🟡 harvestSurfaceDiffs + both readers ship with zero production callers [proportion] ``
grep for harvestSurfaceDiffs(/boxSurfaceReader(/fsSurfaceReader( across src/ returns ONLY src/runtime/surface-diff.ts and src/runtime/surface-diff.test.ts — nothing in run-loop.ts, the improvement pipeline, or any profile invokes the harvest at settle. ~318 lines of production code (surface-diff.ts) + ~395 lines of tests land as an exported /kernel primitive with no wiring. The design doc (section 3.1) states the intent — 'caller-invoked at settle by the same caller that recorded the mounts' — 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.
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 80 | 83 | 65 | 65 |
| Confidence | 90 | 90 | 90 | 90 |
| Correctness | 80 | 83 | 65 | 65 |
| Security | 80 | 83 | 65 | 65 |
| Testing | 80 | 83 | 65 | 65 |
| Architecture | 80 | 83 | 65 | 65 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 6/6 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 6/6 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 6/6 planned shots over 13 changed files. Global verifier still owns final merge decision.
🟡 LOW 'never reports created' invariant omits the absolute-vs-relative path-form caveat — docs/api/runtime.md
Doc states unconditionally that 'a watched path that was also mounted compares against its mount (never reports
created)'. The mount-vs-watch dedup uses pathKey (surface-diff.ts:116), which strips only a leading './' — an absolute mount path ('/home/x/AGENTS.md') and a relative watch path ('AGENTS.md') do NOT dedup, so the same file can yield BOTH a mount diff and acreatedwatch diff. The code itself warns 'Absolute vs relative cannot be reconciled here — keep those forms consistent' (surface-diff.ts:114-115); the doc drops that caveat while stating the invariant absolutely. Add the path-form caveat to the WatchedSurface/harvestSurfaceDiffs prose so a caller mixing forms isn't surprised by double-reported evidence.
🟡 LOW boxSurfaceReader doc overstates the resourceType-to-transport-failure rule — docs/api/runtime.md
Doc: a NotFoundError maps to
missing: true'unless itsresourceTypenames something other than a file/path (the BOX or session being gone), which is a transport failure'. Code (src/runtime/surface-diff.ts:260) treats ONLYresourceTypematching /sandbox|session/i as transport-gone; every other value — including the SDK's defaultresourceType: 'Resource', which names neither file/path nor box/session — maps tomissing: true(asserted by test surface-diff.test.ts:238, 'treats the SDK's default Resource resourceType as a missing FILE'). So for an ordinary deletion (the common case), the doc's rule would make a reader classify it as a transport failure when the code deliberately reportsremoved. The parenthetical is right; the generalization 'names something other than a file/path'
🟡 LOW Phantom symbol 'heldOutGate' referenced as if it exists — docs/design/prime-agent-harness-integration.md
The doc lists 'promotionGate, pairedLift, heldOutGate' as three parallel symbols in the 'Equal-compute topology comparison + holdout gates' row. grep for heldOutGate across all .ts/.mts returns zero matches. promotionGate (src/runtime/promotion-gate.ts:64) IS the holdout gate — it makes the statistical promotion decision over a holdout benchmark — so the concept is real but the symbol name is invented. A reader chasing the reference searches and finds nothing. Fix: drop 'heldOutGate' from the list, or rename to reference the holdout slice machinery in strategy-evolution.ts (holdoutN/holdoutOffset) that feeds promotionGate. Does not affect the doc's decision or any executable code.
🟡 LOW Shipped prime capability rows are pi-inherited, not measured, despite doc's 'measured, not inherited' rule — docs/design/prime-agent-harness-integration.md
The doc claims the id shipped 'in agent-interface (HarnessType; capability rows for its reasoning ladder and system-prompt semantics)' and wish-list item 1 (line 139) restates it, while follow-up 2 (line 95) and wish-list item 1 (line 142) declare that prime capability facts 'still land measured, not inherited' and 'do not inherit pi's capability row b
🟡 LOW 'Do not inherit pi's row' references a row absent from the current matrix — docs/research/harness-compat.md
The matrix (lines 11-20) has only claude-code/codex/opencode columns; there is no 'pi' row in this file. The warning 'Do not inherit pi's row' presumes a future pi column. Not incorrect — agent-interface treats pi and prime as distinct harnesses and docs/design/prime-agent-harness-integration.md:94-96 uses identical phrasing — but a reader of the matrix alone cannot resolve it. Optional: add a parenthetical that pi is the Pi-line lineage harness (sandbox-backend.ts:32) so the forward reference is resolvable without the design doc.
🟡 LOW Plural header for a single entry — docs/research/harness-compat.md
Section header reads '## Pending columns' (plural) but the list contains only the 'prime' entry. Either rename to 'Pending column' or add a one-line note that more pending columns are anticipated. Cosmetic only; no functional impact.
🟡 LOW Dangling symlink at a mounted path misreported as removed — src/runtime/surface-diff.ts
realpath(target) throws ENOENT for a broken link, and the catch at line 310-314 maps ENOENT to missing: true. If an agent replaces a mounted file with a dangling symlink (or the mount recorder writes one), the harvest reports 'removed' even though the path exists as a broken link. Absence-vs-broken-link is distinguishable via lstat; currently the only false-removed path in an otherwise conservative module.
🟡 LOW New surface-diff module has no production callers — integration is untested — src/runtime/surface-diff.ts
grep across src/ confirms harvestSurfaceDiffs/boxSurfaceReader/fsSurfaceReader are only referenced in surface-diff.ts, surface-diff.test.ts, and the index.ts barrel export. No production settle path invokes the harvest yet. The unit tests (30 passing) are thorough for the module's own contract, but the real-data-plane integration (actual box.fs.read wire format, actual SDK NotFoundError shape with resourceType, actual mount-manifest sha256 computed by callers over original bytes vs. settled sha256 over UTF-8 re-encoded text) is unverified. This is acceptable as new public API infrastructure, but a caller that computes mount sha256 over non-UTF-8 bytes (e.g. Buffer with CRLF) would see false 'modified' diffs because the read path hashes UTF-8-encoded text. Document or gate this when the fir
🟡 LOW TOCTOU window between realpath containment check and readFile — src/runtime/surface-diff.ts
realpath(target) checks containment, then readFile(resolvedTarget) is a separate syscall on a later path; an actor able to mutate the tree in between (swap a parent directory for a symlink after resolution) could redirect the read outside root. The window is microseconds and the threat model is a non-adversarial worktree, and cached resolvedRoot makes the check conservative — but an open(O_NOFOLLOW)-style read or re-verifying after read would close it if this reader is ever pointed at untrusted trees.
🟡 LOW U+FFFD detection heuristic can false-positive on legitimate content — src/runtime/surface-diff.ts
The lossy-UTF-8 check
if (result.text.includes('�'))treats ANY occurrence of U+FFFD as proof the text wire corrupted the bytes. U+FFFD is a valid Unicode codepoint that can legitimately appear in text files (some editors insert it for deliberately-invalid byte sequences, and it appears in certain test fixtures). A profile/memory surface containing a literal U+FFFD would be reportedunreadableinstead ofmodified/created, dropping a real diff. The author acknowledges this is a heuristic (comment: 'content the wire had to lossy-decode'), and for intended profile-surface use the likelihood is low. Consider additionally checking for U+FFFD CLUSTERING or using TextDecoder({fatal:true}) on the raw wire bytes if the SDK ever exposes them, to reduce the false-positive surface. Not blockin
🟡 LOW U+FFFD heuristic can misclassify legitimate text and clean-decode binary — src/runtime/surface-diff.ts
if (result.text.includes('\ufffd'))treats ANY replacement character as 'not valid UTF-8' and reports unreadable. A genuine text surface that legitimately contains a U+FFFD (e.g. a test fixture) is a false unreadable; conversely, ASCII-safe binary (all bytes < 0x80) decodes without U+FFFD and is hashed as text, producing a falsemodifiedinstead ofunreadable. The module doc already acknowledges binary needs a byte-faithful reader, so this is a known heuristic limit, not a blocker. Fix if it matters: compare the round-tripnew TextDecoder('utf-8', {fatal:true}).decode(bytes)againstresult.textinstead of sniffing one codepoint, or leave as documented heuristic.
🟡 LOW U+FFFD heuristic flags genuine content as unreadable and never reports binary-surface edits — src/runtime/surface-diff.ts
result.text.includes('�') cannot distinguish a lossy decode from a file that legitimately contains U+FFFD (valid UTF-8 for the replacement codepoint); such a surface is reported unreadable instead of modified. Related: any mounted BINARY surface (e.g. a .db the agent modified) is always unreadable through the text wire, so boxSurfaceReader can never report modified for it. Both are documented trade-offs, but worth a WAT file or a byte-faithful reader contract so a future caller does not treat 'unreadable' as 'agent never touched it'.
🟡 LOW Unbounded concurrent read fan-out over mounts + watches — src/runtime/surface-diff.ts
Promise.allover[...byPath.values()](and a second over watches) fires every mount read concurrently with no concurrency cap. Profile mounts can be numerous (skill/corpus trees); a large manifest spikes the box data plane at settle. Impact is proportional to mount count and currently unmeasured. Consider a small bounded pool (e.g. p-limit) if mount sets can grow large.
🟡 LOW fsSurfaceReader maps a directory target to generic unreadable, not a distinct diagnostic — src/runtime/surface-diff.ts
A mount/watch path that resolves to a directory (or the root itself, e.g. path '' or '.') passes both containment checks, then
readFile(resolvedTarget)throws EISDIR, which is neither ENOENT nor ENOTDIR, so it is reported asmissing:falseunreadable with a raw EISDIR message. Harmless (manifest records files), but the diagnostic could be clearer. Minor.
🟡 LOW harvestSurfaceDiffs fires every mount/watch read concurrently with no bound — src/runtime/surface-diff.ts
Promise.all over all mount paths (line 141) and all watch paths (line 172) starts every read at once. Each boxSurfaceReader read can spend attempts=3 with 250/500ms backoff (surface-diff.ts:229-233), so at settle a large manifest (many skills/memory files, plus enumerated watch paths) issues up to 3x N simultaneous box HTTP reads and up to ~750ms of sleeping per genuinely-missing path. The repo already owns mapWithConcurrency (util.ts:206) for bounding bursts; this module could expose a concurrency option. Latency/load concern only —
🟡 LOW transport-gone detection relies on /sandbox|session/i over a server-controlled string — src/runtime/surface-diff.ts
missing is computed as notFound && !transportGone where transportGone = /sandbox|session/i.test(resourceType). The SDK defaults resourceType to 'Resource' (verified in errors-D2aWy99l.js:298), so ordinary file 404s classify correctly; but if the settle-time 404 names the resource anything not matching the regex (e.g. 'Workspace', 'Box', 'Instance'), a vanished box reads as every mount removed — fabricating 'agent deleted every surface' into improvement evidence, exactly the failure the code exists to prevent. Recommend enumerating the server's actual resourceType values or treating any resourceType !== 'Resource' as unreadable.
🟡 LOW Near-duplicate coverage of the persistent-read-failure path — tests/runtime/sandbox-run.test.ts
This new test overlaps the existing 'surfaces a persistently failed read in readError WITHOUT throwing' test at tests/runtime/sandbox-run.test.ts:283-301: both use readRetryDelayMs:0, an always-throwing fsRead, assert exactly 4 read attempts, and assert readError matches. The sibling additionally asserts turn.out maps over the empty read ({raw:'',n:1}); this test only asserts reads and readError with a different error string. The added behavioral value is marginal (different error message + prose comments). Consider folding the turn.out assertion here and dropping one of the pair, or distinguishing the cases (e.g. a 404-vs-other classification) to justify the duplication.
🟡 LOW New test duplicates an existing case with weaker assertions — tests/runtime/sandbox-run.test.ts
The added 'returns the turn with its readError when every read attempt fails without an abort' test exercises the identical code path as 'surfaces a persistently failed read in readError WITHOUT throwing' at lines 283-301: same createFakeClient setup, same readRetryDelayMs:0, same 4-attempt exhaustion, same readError regex assertion. It is strictly weaker — it omits expect(turn.out).toEqual({ raw: '', n: 1 }), which is the load-bearing invariant (a fault is not recorded as an empty deliverable). If a regression made the read-fault path throw instead of return, both tests would fail, but the new one would do so with a less specific signal. Recommend ei
🟡 LOW New test near-duplicates existing persistent-failure coverage — tests/runtime/sandbox-run.test.ts
The added test (lines 578-597) repeats the contract already asserted by the test at lines 283-301: a persistently failing fs.read with readRetryDelayMs:0 yields 4 attempts and a
turn.readErrormatching the last error instead of throwing. The only material delta is the localreadscounter vs the fake client'sreadPathsside-channel, and the new test drops theturn.outassertion the old one has. Not a bug; consider folding this into the existing test or assertingturn.outtoo, otherwise the duplication is
tangletools · 2026-08-13T01:24:24Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 19 non-blocking findings — 57644246
Full multi-shot audit completed 6/6 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 6/6 planned shots over 13 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 6/6 planned shots over 13 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-08-13T01:24:24Z · immutable trace
|
Disposition for the Refuted — Accepted as designed, with reasons.
|
2 participants
What
Evaluates the "Prime Agent vs Tangle Agent Runtime" spec and lands the pieces that survive the critique — one harness-agnostic kernel primitive, the shipped
primeharness id wired in, and one decision record — while explicitly rejecting the spec's over-engineered proposals.1.
harvestSurfaceDiffs(/kernel) — the read-back dual of the mount manifestRunProvenance.mountsrecords what the substrate placed into a run (instructions, skills, memory). Nothing recorded what the agent changed in its own surfaces while working — the harness-agnostic generalization of Prime's/refine(claude-code editing CLAUDE.md/memory, opencode editing AGENTS.md, Prime editing its continual harness). That evidence evaporated at box teardown.harvestSurfaceDiffs({ mounts, read, watch? })re-hashes mounted surfaces at settle →modified/removed/created/unreadablediffs; unchanged surfaces produce no entry; failed reads (including a reader that throws) are contained and reported, never silently dropped; reads run concurrently with record order preserved.boxSurfaceReaderjoins it to the realbox.fs.readseam (the oneopenSandboxRunreads deliverables through), with the same transient-404 retry posture, a box-gone-vs-file-gone distinction (aNotFoundErrorwhoseresourceTypenames the sandbox is a transport failure, never "every surface was removed"), and a lossy-decode guard (U+FFFD content reportsunreadableinstead of hashing mojibake).fsSurfaceReadercovers worktree/local workers and refuses paths that resolve outside its root.watchentries cover never-mounted paths (a new memory/skill file — the most common/refineoutcome), documented as a mount-manifest inference with caller guidance; watch entries dedupe by path, and./-prefixed vs bare path forms resolve to one surface.proposeAgentImprovement→ review → gated activation pipeline, never auto-promoted.17 tests: box-double and worktree joins, retry, box-gone, lossy-decode, throw containment, root containment, path-form collision.
2. The
primeharness id, wiredagent-interface0.46.x shipped the id asprime(a distinct harness frompi— the fork's daemon rejects pi-line clients) and the sandbox backend enum accepts it, so'prime'joinsharnessBackendsinsrc/runtime/sandbox-backend.ts(the double-satisfiespin keeps the three enums compile-aligned). Two follow-ups stay gated on evidence, not releases: thetoolPartDecoders['prime']entry lands only once validated against Prime's real session output (unregistered harnesses correctly fall through to the try-all composite), and the harness-compat column lands only measured.3.
docs/design/prime-agent-harness-integration.md— the decision recordCertifiedCapability≈ its §8.6 verbatim;AgentEnvironmentCapabilities+box.snapshot/branch+ the CRIU lineage seam cover its checkpoint/fork ports; receipts, improvement governance, the harness×model matrix, the Prime Intellect trace bridge).TopologyAuthority(the conserved budget pool makes recursion "accounting incidents" structurally impossible), aHarnessExecutionReceiptenvelope (fields exist across current receipts; gaps are substrate reporting facts), an optional-methodAdvancedHarnessBackend(re-creates the adapter zoo the oneExecutorport replaced), and any Runtime-side persistent-kernel abstraction.ToolPartnormalization (zero new decode logic here), nativerlmchild-tree exposure, environment-identity digests, kernel-cell/effect visibility,/refineevent surfacing with scope policy, fan-out caps as config, and CRIU snapshot coverage verification for the Prime process tree.AgentEnvironmentProvider(sessions, capabilities, checkpoint/fork), so the sandbox side picks per tier — box backend for basic runs, provider for instrumented/experiment tiers./refine-derived edit transfer to held-out tasks at equal compute — run throughimprove()'s partitions and the existing gates, no new rig.Also: decision-table row in
canonical-api.md(version pin aligned to 0.132.2), a pending (measure-only)primecolumn marker inresearch/harness-compat.md, docs index entry, regenerateddocs/api/.Review
Two review rounds ran over the diff (a high-effort local review and the tangletools multi-shot audit). All actionable findings are applied — transient-404 retry, box-gone vs file-gone, lossy-decode guard, watch dedup + path-form normalization, concurrent reads, reader-throw containment, fs root containment, doc pointer fixes. The audit's two MEDIUMs (version regression, removed exports) were stale-base artifacts resolved by merging
main. Consciously deferred with reasons in the triage comment:readBatchbyte-faithful reads (SDK error-code vocabulary undocumented),AbortSignalthreading (no caller needs mid-harvest cancellation yet).Verification
pnpm run lint,pnpm run typecheck,pnpm run docs:check(freshness gate) — green on the merged tree.vitest src/runtime/surface-diff.test.ts— 17/17.tests/testing-fixture,tests/kernel/coordination-mcp,tests/mcp/worktree-harness,tests/mcp/local-harnessreproduce identically on cleanmainat3000211(pre-existing, environment-sensitive; unrelated to this diff).