Skip to content

feat(intelligence): ship reviewable improvement lifecycle#511

Merged
drewstone merged 5 commits into
mainfrom
feat/intelligence-main-clean
Jul 11, 2026
Merged

feat(intelligence): ship reviewable improvement lifecycle#511
drewstone merged 5 commits into
mainfrom
feat/intelligence-main-clean

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Problem

Intelligence could analyze and generate candidates, but the public runtime did not expose one durable proposal -> review -> approved execution lifecycle.

Solution

  • Expose proposeAgentImprovement, reviewAgentImprovementProposal, and executeApprovedAgentCandidate.
  • Bind proposal, profile, bundle, review, execution plan, materialization receipt, and final run receipt with canonical digests.
  • Keep live trace export best-effort while keeping candidate execution fail-closed.
  • Adopt agent-eval 0.113, agent-interface 0.25, and agent-profile-materialize 0.3.1.
  • Export candidate execution as a dedicated public subpath and refresh generated API docs.

Proof

  • pnpm typecheck
  • pnpm lint
  • pnpm test: 148 files, 1478 passed, 2 skipped
  • pnpm build
  • pnpm verify:package
  • pnpm docs:freshness

Release package version: 0.92.1.

tangletools
tangletools previously approved these changes Jul 11, 2026

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 41d68861

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

tangletools · auto-approval · reason: drewstone_author · 2026-07-11T07:31:15Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

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

💰 Value — sound-with-nits

Adds a content-addressed propose→review→execute lifecycle that composes existing analyst/improve/candidate-execution primitives into the missing human-approval front door — coherent and in-grain, with one hand-rolled profile projection that belongs in the substrate.

  • What it does: Exposes three public functions (+2 verifiers) in src/intelligence/improvement-cycle.ts: proposeAgentImprovement (improvement-cycle.ts:156) runs runAnalystLoop + held-out improve(), optionally seals an executable bundle via a caller buildCandidate adapter, and emits a canonical-digested AgentImprovementProposal; reviewAgentImprovementProposal (198) emits a digested approve/reject/requ
  • Goals it achieves: Close the gap where Intelligence could analyze and generate measured candidates but had no durable, human-reviewable, fail-closed path to execution. After merge: a promotion executes only the exact bundle a human approved, every stage is digest-bound and tamper-evident, the linkage lands on the run trace for audit, and live trace export stays best-effort while candidate execution stays fail-closed
  • Assessment: Sound. It is glue, not a reimplementation: it composes runAnalystLoop, improve(), and the existing candidate-execution verify/prepare/execute primitives (candidate-execution/verify.ts:37, prepare.ts:97, execute.ts:75) over the established content-addressed grain (canonicalCandidateDocument, Sha256Digest, immutableCandidateValue). The fail-closed ordering (verify proposal → verify review
  • Better / existing approach: Searched for an existing lifecycle: analyst-loop (proposeFromFindings/withheld_for_review), mcp/delegates CoderReview, and delivery ProposedProfileDiff/applyAgentProfileDiff — none provide a persisted, digest-bound propose→review→execute over sealed candidate bundles, so this is the right layer, not a duplicate. The one real design smell is candidateProfileAsAgentProfile+publicResource/`
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound

The reviewable improvement lifecycle is a clean, digest-bound propose→review→execute composition that sits naturally on top of the existing analyst, improve(), and candidate-execution primitives — no competing pattern, no dead surface.

  • Integration: The three new public verbs are exported from the public ./intelligence subpath (src/intelligence/index.ts:108-114) and are reachable as the package's published API. They compose exclusively existing primitives: runAnalystLoop (src/analyst-loop), improve() (src/improvement/improve.ts:465), sealAgentCandidateBundle (a 17-line wrapper over canonicalCandidateDigest + agentCandidateBundleSchema in src/
  • Fit with existing patterns: Fits the codebase's grain precisely. The prior PRs (#507 candidate execution, #509 protected model grants) built the immutable verify→prepare→execute→receipt pipeline specifically as the foundation for this lifecycle. The surfaces.ts expansion (skills, mcp, hooks, subagents, workflows, code, rolloutPolicy, agentProfile) mirrors the ImproveSurface union in src/improvement/improve.ts:75-86 one-to-on
  • Real-world viability: Holds up under stress. Every boundary is digest-verified: proposeAgentImprovement binds the candidate profile hash + optional sealed bundle (improvement-cycle.ts:185-190), reviewAgentImprovementProposal refuses approval without both a passing measured gate AND a sealed bundle (improvement-cycle.ts:205-211), and executeApprovedAgentCandidate checks review.decision, proposalDigest match, candidateBu
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟠 Hand-rolled sealed↔public AgentProfile projection will drift with the substrate [maintenance] ``

candidateProfileAsAgentProfile plus publicResource/publicValue/mapPublicValues/shellQuote (improvement-cycle.ts:324–439) manually re-project the sealed AgentCandidateProfile into a public AgentProfile (re-wrapping mcp args/env, hooks command strings, resource refs) just so assertCandidateProfileBinding can compare digests across agent-interface's two schemas. This enumerates profile fields by name, so any new field agent-interface adds is silently dropped from the binding check o

🟡 canonicalJsonValue pre-pass is largely redundant with the canonical digest [duplication] ``

canonicalJsonValue (improvement-cycle.ts:487–506) is applied to evaluation and findings (177–178) before embedding, but the proposal digest is then computed over the whole withoutDigest object via canonicalCandidateDocumentcanonicalCandidateBytescanonicalJson (digest.ts:12–18), which already canonicalizes those nested values. The only added behavior is stripping undefined and rejecting non-finite numbers/undefined array entries. Confirm whether canonicalJson already covers


What this audit checks

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

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

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

value-audit · 20260711T073725Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 40d53334

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

tangletools · auto-approval · reason: drewstone_author · 2026-07-11T07:39:49Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

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

💰 Value — sound

Composes the existing measured-improvement + sealed-candidate + fail-closed-execution primitives into a public, digest-bound propose→review→execute-approved lifecycle that the runtime genuinely lacked; in-grain, proportionate, no existing equivalent found.

  • What it does: Adds two new source files (src/candidate-execution/bundle.ts, src/intelligence/improvement-cycle.ts) and wires them into the public API. proposeAgentImprovement runs runAnalystLoop + improve(), optionally seals an AgentCandidateBundle via the new sealAgentCandidateBundle, and emits a content-addressed AgentImprovementProposal. reviewAgentImprovementProposal produces a digest-boun
  • Goals it achieves: Close the gap where Intelligence could analyze and generate candidates but the public runtime exposed no durable, human-reviewable, fail-closed path from measured recommendation to approved execution. After merge: a product gets one canonical, tamper-evident lifecycle (proposal→review→approved-execution→receipt) instead of each hand-wiring its own; live trace export stays best-effort while candida
  • Assessment: Good, coherent, in the codebase's grain. The lifecycle reuses already-shipped, already-tested substrate — runAnalystLoop, improve(), canonicalCandidateDigest/canonicalCandidateDocument/omitTopLevelDigest/immutableCandidateValue, verifyAgentCandidateBundle, prepareAgentCandidateExecution, executePreparedAgentCandidate (all from #507/#509, confirmed present pre-PR). The only net-ne
  • Better / existing approach: none — this is the right approach. I checked for an existing equivalent: loop-runner.ts 'review' mode is a code-worktree review loop, not an improvement approval gate; types.ts:387 proposal_created is an unrelated runtime stream event; analyst-loop review-then-apply (autoApply off, withheld_for_review) emits unstructured edits with no sealed bundle, digest-bound review, or execute-ap
  • Model: opencode/kimi-for-coding/k2p7
  • Bridge attempts: 1

🎯 Usefulness — sound

A coherent proposal→review→execute lifecycle that binds existing analyst/improvement/candidate-execution primitives with digest-linked accountability, fail-closed approval gates, and OTLP evidence export — fills a real gap with no dead surface or competing pattern.

  • Integration: All three verbs are exported from the ./intelligence subpath (intelligence/index.ts:108-114) and compose existing primitives: runAnalystLoop, improve, sealAgentCandidateBundle/verifyAgentCandidateBundle/prepareAgentCandidateExecution/executePreparedAgentCandidate. CandidateExecutionEvidence is threaded into RunRecord/RunReport (intelligence/index.ts:183-184,583-601) and forwarded by withIntelligen
  • Fit with existing patterns: Thin composition over established primitives — does not reimplement the analyst loop, improvement loop, or candidate-execution pipeline. Matches the codebase grain of composing lower-level primitives into a public surface. Complementary to the remote withIntelligence→pullCertified receive path (which delivers remotely-certified improvements); this lifecycle produces locally-measured improvements.
  • Real-world viability: Deliberately fail-closed: no skill write-back pre-approval (improvement-cycle.ts:159-161), approve refused without ship verdict + sealed bundle (:205-212), execute verifies proposal/review/bundle digests + bundle↔review binding + product-owned authorizeReview callback (:231-244), judge-derived findings rejected (:166-169), canonical digest re-verified on every verify (:280-294,456-457), candidate-
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

💰 Value Audit

🟡 candidateProfileAsAgentProfile hand-enumerates AgentProfile fields [maintenance] ``

improvement-cycle.ts:324-406 rebuilds an AgentProfile from a bundle's candidate profile by listing field names ('name','prompt',...,'resources'). If AgentProfile gains a sealable field this allowlist must be updated or new fields silently drop. It is bounded by assertCandidateProfileBinding (rejects connections/metadata/extensions and re-digests), so it cannot ship a skewed profile — but it is a maintenance coupling to watch. Not a ship-blocker; the boundary is deliberate.


What this audit checks

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

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

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

value-audit · 20260711T074530Z

@tangletools

Copy link
Copy Markdown
Contributor

❌ Needs Work — 40d53334

Review health 100/100 · Reviewer score 0/100 · Confidence 95/100 · 64 findings (3 high, 14 medium, 47 low)

opencode-kimi glm deepseek aggregate
Readiness 0 20 0 0
Confidence 95 95 95 95
Correctness 0 20 0 0
Security 0 20 0 0
Testing 0 20 0 0
Architecture 0 20 0 0

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

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

Blocking

🔴 HIGH Generated mcp.md union line permuted off source order — breaks enforced docs:check freshness gate — docs/api/mcp.md

Line 836 now renders recommendation: "ship" \| "reject" \| "approve-with-nits" \| "changes-requested" with Defined in: mcp/delegates.ts:102. The cited source src/mcp/delegates.ts:102 declares recommendation: 'ship' | 'approve-with-nits' | 'changes-requested' | 'reject' and is byte-identical base→head (PR does not touch this file). typedoc.json sort: ["source-order"] and the generator versions (typedoc 0.28.19, typedoc-plugin-markdown 4.12.0, typescript ^5.7.0) are all unchanged base→head, and the base doc already emitted source order — so a fresh pnpm docs:api must reproduce ship | approve-with-nits | changes-requested | reject, never the committed order. Ev

🔴 HIGH New surface kinds lack path traversal protection — src/agent/surfaces.ts

The skill (L154-159), mcp (L173-180), hook (L181-186), subagent (L187-193), and workflow (L194-200) cases construct paths via join(surfaces.*, subject.name, ...) without safeJoin wrapping. The subject.name, subject.server, and subject.tool fields originate from parseFindingSubject which parses LLM-generated finding strings — effectively untrusted input. The code case (L206-209) in the same PR does use safeJoin, and tests exist for traversal rejection on code/knowledge.raw/rag (tests/agent.test.ts L222-234). No traversal-rejection test exists for skill/mcp/hook/subagent/workflow. An LLM-generated subject like kind:mcp server:../secrets would produce mcp/../secrets/README.md and escape the mcp surface root.

🔴 HIGH New security-critical improvement-cycle module ships with no tests — src/intelligence/improvement-cycle.ts

improvement-cycle.ts is an Added file (506 lines) implementing the propose/review/execute/verify approval chain — exactly the code that decides whether a measured candidate may run. It contains the canonical-digest binding (verifyAgentImprovementProposal lines 270-296), the review↔proposal↔bundle linkage checks (executeApprovedAgentCandidate lines 228-267), the measured-vs-bundled profile normalization contract (assertCandidateProfileBinding / candidateProfileAsAgentProfile [lines 310-406](https://github.

Other

🟠 MEDIUM Path-traversal sanitization applied asymmetrically — new surface kinds join raw subject components — src/agent/surfaces.ts

The PR adds safeJoin() and routes code, knowledge.raw, and rag docId through it, and its new tests (tests/agent.test.ts:222-235) establish resolveSubjectPath as the sanitization boundary by constructing subjects directly and expecting null. But the new kinds added in the same function join untrusted components raw: join(surfaces.skills, subject.name, 'SKILL.md') (line 157), mcp server/tool (lines 176-180), hook/subagent/workflow names ([lines 184](https://github.com/tangle-network/agent-runtime/blob/40d53334b8fdf7ca5d91657075a128c5d8

🟠 MEDIUM rag case: subject.corpus bypasses safeJoin via join() normalization — src/agent/surfaces.ts

The rag case calls safeJoin(join(surfaces.rag, subject.corpus), ...). safeJoin only validates the second argument (child). The first argument (root) is join(surfaces.rag, subject.corpus) — if subject.corpus = '../../etc', Node's path.join normalizes this to 'etc' before safeJoin ever sees it. safeJoin('etc', 'doc.md') returns 'etc/doc.md', escaping the rag directory. The existing test (tests/agent.test.ts L232-234) only tests docId traversal, not corpus traversal. The corpus field should be validated separately, or the entire rag path construction should use a two-stage safeJoin: safeJoin(safeJoin(surfaces.rag, subject.corpus), docId).

🟠 MEDIUM Plan-declared grader identity is never enforced against the grader actually executed — src/candidate-execution/prepare.ts

This PR signs task.grader into the canonical execution plan (prepare.ts:292) and validates it (prepare.ts:571-577), and types.ts:154 adds the identity type — but nothing downstream compares it to the grader that actually runs. persistCandidateBenchmarkResult (outcome-evidence.ts:275-279, unchanged) writes grader into the benchmark-result material from graded.grader, i.e. the grader PORT's self-reported name/version/artifact snapshot, and runBoundCandidateBenchmarkGrader (benchmark-grader.ts:60-72) only binds implementation bytes ↔ task outcome ↔ output. Grep of the repo confirms no code reads executionPlan.material.grader after prepare. Impact: an evaluator can declare grader A in the signed plan yet run grader B at score time; the two content-addressed artifacts silently disag

🟠 MEDIUM The code-surface ship + winner-retention cleanup path is completely untested — src/improvement/improve.test.ts

The only code-surface test uses a flat judge (composite 0.5 for everything) and asserts result.shipped === false (line 449), so preparedCode.cleanup(retainedWinner) is only ever exercised with retainedWinner === undefined. The retention logic is the most data-loss-sensitive new code in this shot: isCodeSurface() discriminant check (surface.kind === 'code', improve.ts:311-313), the worktreeRef match against the driver's finalized map (improvement-driver.ts:127, 136-143), and skipping discard for the retained winner. If a code run ever ships and retention fails (e.g. the substrate's CodeSurface discriminant differs), the shipped winner worktree wou

🟠 MEDIUM Post-ship cleanup failure masks a successful run and hides the shipped winner — src/improvement/improve.ts

In the success path, await preparedCode?.cleanup(shipped ? raw.winner.surface : undefined) runs AFTER shipped is computed but BEFORE the result is built/returned (lines 551-565). The cleanup impl (lines 346-361) collects errors from BOTH managed.cleanup() (losing-candidate discard) and worktree.discard(baselineWorktree) and throws an AggregateError on any failure. Consequence: a transient git/FS failure discarding the ephemeral baseline worktree — or a locked losing candidate — turns a completed, paid-for ship verdic

🟠 MEDIUM Resource leak: baseline worktree not discarded when finalize fails after create succeeds — src/improvement/improve.ts

At line 325, worktree.create() creates a baseline worktree on disk. At line 326, worktree.finalize() commits it into a CodeSurface. If finalize throws (e.g., git commit failure on a broken repo), the baselineWorktree is leaked — it exists on disk but preparedCode was never assigned, so the caller's catch block at line 538 has no preparedCode to call cleanup() on. The orphaned worktree directory

🟠 MEDIUM Zero test coverage for 506-line improvement-cycle.ts with security-critical digest/approval logic — src/intelligence/improvement-cycle.ts

No test file exists (globbed src/**/improvement-cycle* — only the implementation file). The five exported functions enforce a content-addressed approval chain: proposal digest over canonical JSON, candidate bundle re-sealing, profile-bundle binding via candidateProfileAsAgentProfile normalization (including shellQuote for hooks), approval gate (gateDecision==='ship' + sealed bundle required), review↔proposal digest binding, and authorizeReview callback. None of these paths are exercised by any test in this PR. The with-intelligence.test.ts only passes opaque sha256 strings through candidateExecution. At minimum, a propose→verify round-trip, an approve-without-bundle rejection, a tampered-digest rejection, and a profile-bundle mismatch rejection are needed before shipping.

🟠 MEDIUM canonicalJsonValue silently drops undefined object properties, throws on undefined array entries — src/intelligence/improvement-cycle.ts

Line 493-496: [undefined] throws 'undefined array entry at $[0]'. Line 500-501: {a: undefined} silently becomes {}. This asymmetry means undefined properties in objects are silently dropped from the canonical JSON, while undefined array entries halt the computation. If a finding or evaluation field has an undefined property (e.g., an optional field), it's silently removed from the digest computation, potentially hiding missing data. Fix: either throw for undefined object values too, or handle both

🟠 MEDIUM Profile hash and byte count mismatch in exportRunRecord — src/intelligence/index.ts

addPayloadAttributes (line 604) sets tangle.agent.profile_hash from the REDACTED profile and tangle.agent.profile_bytes from the redacted profile's serialized length. Line 610 then overwrites tangle.agent.profile_hash with contentHash(record.profile) — the RAW profile hash. But tangle.agent.profile_bytes is NOT overwritten, so it remains the byte count of the redacted profile. Result: the hash and byte count refer to different underlying data. A consumer that validates the hash against the byte count would fail. Fix: either move [line 610

🟠 MEDIUM Profile hash overwritten with unredacted content, breaking hash-content consistency — src/intelligence/index.ts

Line 604-609 calls addPayloadAttributes(labels, 'tangle.agent.profile', redactor(record.profile), includeFullPayload), which sets tangle.agent.profile_hash = contentHash(serializeJson(redacted)) and tangle.agent.profile_bytes = byteLength(redacted). Line 610 then OVERWRITES: labels['tangle.agent.profile_hash'] = contentHash(record.profile) — the UNREDACTED profile hash. Result: when payloadAttributes='full', the exported tangle.agent.profile is the redacted profile but _hash is of unredacted — a consumer cannot verify the hash agains

🟠 MEDIUM serializeJson unbounded for full-payload exports — src/intelligence/index.ts

The old previewJson had a 4096-char truncation limit. The new serializeJson removes the limit entirely. When payloadAttributes='full', a multi-megabyte input/output could produce span attributes exceeding OTLP protocol limits (~128KB per span), causing export failures or excessive memory. The config comment on line 257-262 documents this as opt-in, but there's no safety valve in code (no size warning, no truncation fallback). Fix: add a configurable max-size guard or a warning log when serialized content exceeds a safe threshold.

🟠 MEDIUM tangle.agent.profile hash/bytes/content describe two different values — src/intelligence/index.ts

In the if (record.profile) block (lines 603-611), addPayloadAttributes(labels,'tangle.agent.profile', redactor(record.profile), includeFullPayload) sets tangle.agent.profile_hash = contentHash(serializeJson(redactor(...))), tangle.agent.profile_bytes = byteLength of the redacted serialization, and (when full) tangle.agent.profile = redacted content. The very next statement (line 610) then OVERWRITES labels['tangle.agent.profile_hash'] = contentHash(record.profile) — a hash of the RAW, un-redacted, un-serialized object. End state: _

🟠 MEDIUM No attribute value size cap — large tool payloads risk silent collector drop — src/otel-export.ts

serialized() produces unbounded strings. For tool_call/tool_result events with includeControlPayloads enabled, the payload is serialized TWICE: once inside tangle.runtime.event (full record JSON) and again as tool.input/tool.output (line 289). A 5MB tool result becomes ~10MB of span attributes. OTLP/JSON collectors enforce per-attribute limits (Jaeger default 1024 chars, Tempo configurable) and will silently truncate or drop these. The test at tests/otel-export.test.ts:35 explicitly asserts no truncation occurs, confirming this is by design — but there is no caller-side truncation either. For production tool outputs (file reads, search results), this will cause data loss

🟠 MEDIUM Non-finite latencyMs crashes the span builder (RangeError from BigInt) — src/otel-export.ts

endMs = startMs + event.latencyMs (lines 313-316) feeds flatOtelSpanmsToNsBigInt(Math.floor(ms)) (line 623). For a type-legal llm_call event with latencyMs: NaN or Infinity, this throws RangeError: The number NaN/Infinity cannot be converted to a BigInt — reproduced in a throwaway vitest probe. RuntimeStreamEvent is a public @stable type any custom backend can emit, and the sibling builder in this same file (buildLoopSpanNodes) guards every numeric with a Number.isFinite num() helper, so the hardening pattern already

🟡 LOW Generated docs reorder union members differently than source — docs/api/mcp.md

The generated docs show 'ship' | 'reject' | 'approve-with-nits' | 'changes-requested' but src/mcp/delegates.ts:102 (unchanged in both base and head) has 'ship' | 'approve-with-nits' | 'changes-requested' | 'reject'. Union order is semantically irrelevant but doc-source divergence erodes trust in generated docs. Root cause is likely non-deterministic ordering in typedoc or the docs generator. Not a code bug, but nits-worthy.

🟡 LOW mcp.md recommendation union reordered without source change (noise diff) — docs/api/mcp.md

The doc line changed from "ship" | "approve-with-nits" | "changes-requested" | "reject" to "ship" | "reject" | "approve-with-nits" | "changes-requested", but git diff base head -- src/mcp/delegates.ts is EMPTY — the source at delegates.ts:102 still has the original order. The value SET is identical (TS unions are unordered), so this is semantically a no-op, but it produces a misleading diff implying a source change. Likely a TypeDoc ordering side-effect from a transitive type rebuild. Not blocking; flag only because noise-diffs in generated docs reduce the signal value of future regens.

🟡 LOW No test for optional surface type mismatch — src/agent/surfaces.ts

The validateSurfaces function (L332-337) checks that optional surfaces are the right type (directory vs file), and a test covers this for required surfaces (tests/agent.test.ts L619-636). But no test covers the optional-surface path — e.g., declaring skills: 'not-a-dir' where not-a-dir is an existing file. A missing continue/fallthrough bug here would be undetected.

🟡 LOW Stale operator-facing docs omit the 8 new optional surfaces — src/agent/surfaces.ts

The error message rendered when validation fails (lines 355-359) and the AgentSurfaces doc comment (line 33) still say optional surfaces are only 'scaffolding / memory / rag / outputSchema'. Operators hitting a validation failure get guidance that doesn't mention skills/mcp/hooks/subagents/workflows/rolloutPolicy/agentProfile/code, the surfaces this PR adds. Cosmetic but user-facing in the exact error path this PR extends.

🟡 LOW Unchanged error message doesn't mention new surfaces — src/agent/surfaces.ts

The error message at L356-358 reads: 'Optional surfaces (scaffolding / memory / rag / outputSchema) may be omitted; the loop will reject...' — this lists only the original 4 optional surfaces, not the 8 new ones added in this PR (skills, mcp, hooks, subagents, workflows, rolloutPolicy, agentProfile, code). Operators who misconfigure new surfaces will see incomplete guidance in the error.

🟡 LOW memory documented as a file store but new isDirectory() enforcement rejects file-backed memory — src/agent/surfaces.ts

The interface doc comment (line 50) declares memory as 'memory store path (JSONL / SQLite / DB)' — a file. The PR's new type enforcement puts memory in optionalDirSurfaces and flags any existing file as 'not-directory', failing defineAgent at startup. Verified: validateSurfaces({...surfaces, memory: 'memory.jsonl'}, root) → [{ surface: 'memory', path: 'memory.jsonl', reason: 'not-directory' }]. Pre-PR this manifest validated cleanly, so this is a silent breaking change for consumers following the documented contract (the memory routing at [line 215](

* Surface declarations. Every path is repo-relative (or absolute) at

🟡 LOW existsSync + statSync TOCTOU can crash validation instead of reporting an issue — src/agent/surfaces.ts

The new checks call existsSync(abs) and then statSync(abs) on the next line (307, 320, 333/335). If the path is removed or permissions change between the two calls, statSync throws ENOENT/EACCES and validateSurfaces crashes at defineAgent time instead of returning a SurfaceValidationIssue — the opposite of the function's fail-loud-with-issues contract. Fix: replace existsSync+statSync with a single try/catch around statSync, mapping ENOENT/ENOTDIR to reason 'missing'.

🟡 LOW renderSurfaceIssues footer omits the newly-added optional surfaces — src/agent/surfaces.ts

The help footer still reads 'Optional surfaces (scaffolding / memory / rag / outputSchema)' but the validated set now also includes skills / mcp / hooks / subagents / workflows / code / rolloutPolicy / agentProfile. An operator who hits a 'not-directory' issue on e.g. skills won't see it acknowledged in the rendered guidance. The rendered (i.reason) value is correct; only the prose is stale. Fix: either drop the parenthetical list ('Optional surfaces may be omitted; ...') or regenerate it from the optionalDirSurfaces/optionalFileSurfaces arrays so it can't drift again.

🟡 LOW statSync after existsSync is TOCTOU-fragile (theoretical) — src/agent/surfaces.ts

Pattern if (!existsSync(abs)) {...} else if (!statSync(abs).isDirectory()) {...} appears 4x (lines 305-309, 318-322, 333-337). If the path is deleted between existsSync and statSync, statSync throws ENOENT uncaught, crashing defineAgent with a raw Node error instead of a clean SurfaceValidationIssue. Window is microseconds and this runs once at startup, so practical impact is near-zero; a try/catch around statSync (or replacing the existsSync+statSync pair with a single statSync in a try/catch) would make it robust. Not a merge blocker.

🟡 LOW No dedicated unit test for sealAgentCandidateBundle digest mismatch path — src/candidate-execution/bundle.ts

The function's core invariant (parsedDigest !== digest -> throw) is never directly tested. It's exercised indirectly through verifyAgentImprovementProposal in improvement-cycle.test.ts, but only on valid input. A direct test passing schema-valid-but-mutated input (or a mock schema that transforms) would prove the guard fires. Impact: a future schema change adding a transform could silently break content-addressing without test coverage catching it.

🟡 LOW AgentCandidateBundleInput not exported from candidate-execution/index.ts despite appearing in exported intelligence API — src/candidate-execution/index.ts

AgentCandidateBundleInput is the return type of ProposeAgentImprovementOptions.buildCandidate (improvement-cycle.ts:118), and ProposeAgentImprovementOptions is exported via the intelligence barrel. However, AgentCandidateBundleInput itself is only importable from './bundle', not from the candidate-execution barrel. Currently not a blocker because src/index.ts does not re-export the intelligence module, making this internal-only. But if intelligence is ever surfaced as a public entry point, consumers cannot import the named type. Fix: add type AgentCandidateBundleInput and sealAgentCandidateBundle to the index.ts export block alongside the existing types.

🟡 LOW sealAgentCandidateBundle / AgentCandidateBundleInput are not exported from the candidate-execution index — src/candidate-execution/index.ts

bundle.ts is absent from index.ts's export list, so the new ./candidate-execution package subpath (package.json, added by this PR) cannot reach sealAgentCandidateBundle or the AgentCandidateBundleInput type. Yet the PR's regenerated public docs (docs/api/intelligence.md:1335) name AgentCandidateBundleInput as the ProposeAgentImprovementOptions.buildCandidate return type, and src/index.ts re-exports only ./candidate-execution (index.ts). External consumers cannot import the type from any public entry point (deep imports are blocked by the exports map). Fix: add export { type AgentCandidateBundleInput, sealAgentCandidateBundle } from './bundle' to index.ts (or export the type from the intelligence entry).

🟡 LOW sealAgentCandidateBundle not exported from candidate-execution public subpath — src/candidate-execution/index.ts

package.json adds a new ./candidate-execution export subpath pointing to dist/candidate-execution/index.js, but sealAgentCandidateBundle and AgentCandidateBundleInput (from bundle.ts) are not re-exported in src/candidate-execution/index.ts. They are only accessible via direct file import from src/intelligence/improvement-cycle.ts. If external consumers are expected to use the candidate-execution subpath, these should be added to the barrel export.

🟡 LOW New grader validation and plan-stamping have no direct test coverage — src/candidate-execution/prepare.ts

The test diff only adds a valid grader to two fixtures (tests/candidate-execution-prepare.test.ts:269, tests/helpers/candidate-execution-fixture.ts:362). No test exercises the new rejection branches (empty grader name/version, artifact.byteLength <= 0, malformed artifact.sha256 at prepare.ts:571-577), and the main binding test asserts many plan fields but never plan.grader (grep confirms no grader assertion outside the fixture). A regression that dropped or corrupted the grader identity from the plan would pass the suite. Fix: add assertions that executionPlan.value.material.grader equals the declared identity and one rejection case per new validation branch.

🟡 LOW AgentCandidateBenchmarkGraderIdentity missing TSDoc comment — src/candidate-execution/types.ts

The new interface lacks a TSDoc comment, causing it to appear as 'Undocumented supporting type' in the API docs primitive catalog (confirmed at docs/api/primitive-catalog.md:208). All other public types in this file have documentation.

🟡 LOW Error-path worktree cleanup branch is untested — src/improvement/improve.test.ts

The catch block at improve.ts:538-549 (selfImprove throws → cleanup all worktrees → rethrow) is never exercised by any test. The happy-path cleanup (line 552) IS verified via the worktree list assertion in the code-surface test (line 450: toHaveLength(1)), but a regression that breaks error-path cleanup (e.g., a future change that swallows the cleanup error or skips the discard) would not be caught. Impact: resource leak regressions on error paths go undetected. Fix: add a test that injects a selfImprove failure (e.g., a stu

🟡 LOW Baseline worktree leaks if finalize() throws inside prepareCodeRun — src/improvement/improve.ts

prepareCodeRun is called at line 506 OUTSIDE the try/catch block (which starts at line 520). Inside prepareCodeRun (lines 325-326), worktree.create() succeeds, then worktree.finalize() is called. If finalize() throws (e.g., git commit failure, disk full), the baseline worktree created at [line 325](https://github.com/tangle-network/agent-runtime/blob/40d53334b8fdf7ca5d91657075a128c5d8f8bc5a/src/improvemen

🟡 LOW ImproveOptions type refactor silently inherits all SelfImproveOptions passthrough properties — src/improvement/improve.ts

The type change from interface to Omit<SelfImproveOptions, 'analyzeGeneration' | 'baselineSurface' | 'findings' | 'gate' | 'proposer'> & { ... } means callers can now pass any SelfImproveOptions property (onProgress, onProvenance, collectWorkerRecords, expectUsage, captureSource, autoOnPromote, etc.) that were not previously on the ImproveOptions interface. This is intentional and tested (see 'forwards evaluation controls' test), but it means the public API surface expanded without any deprecation or migration note. If a caller passes a SelfImproveOptions property that selfImprove rejects at runtime, the error surfaces from deep inside selfImprove rather than as a TypeScript type error. Callers who type-check against the published ImproveOptions type should be unaffected, but runtime

🟡 LOW New mandatory input-profile schema validation is a silent breaking change for existing callers — src/improvement/improve.ts

Lines 483-488 add agentProfileSchema.safeParse(profile) at entry, rejecting any profile the zod schema considers invalid — previously improve() accepted any profile and only touched the lever field. Tests prove minimal {name} fixtures pass and a type-invalid subagent (maxSteps: 'many') is rejected, so the schema is permissive-but-typed. Still, any existing caller passing a persisted profile with a shape the strict schema rejects (unknown to this shot since agent-interface's schema isn't in these files) now gets a ConfigError where it previously ran — including for surface: 'code' runs that never consume profile fields for the surface. This may be int

🟡 LOW isCodeSurface type-narrowing is structural, not branded — fragile to upstream CodeSurface shape changes — src/improvement/improve.ts

The isCodeSurface guard checks typeof surface === 'object' && surface !== null && surface.kind === 'code'. This correctly narrows MutableSurface to CodeSurface given the current upstream type definition, but if @tangle-network/agent-eval/campaign adds a new surface variant with kind: 'code' (or changes the discriminator field name), this guard silently starts matching the wrong type, potentially extracting a non-existent worktreeRef. A branded check (e.g., importing and using a CodeSurface type guard from the substrate) would be more robust. Not blocking — the current upstream shape is stable.

🟡 LOW skills option doc still promises a refs-array default that now throws — src/improvement/improve.ts

The doc comment (lines 130-134) and baselineSurfaceFor (lines 228-230) still describe 'Without this, surface: skills optimizes the profile's skills REFS array' as the default back-compat behavior. But the new guard at lines 489-493 throws ConfigError whenever surface==='skills' and neither generator nor skills is provided — so the refs-array baseline is now reachable ONLY when a caller sup

🟡 LOW candidateProfileAsAgentProfile is a tight, fragile equality contract that can falsely reject valid candidates — src/intelligence/improvement-cycle.ts

Approval requires canonicalCandidateDigest(measured) === canonicalCandidateDigest(candidateProfileAsAgentProfile(bundled)) (lines 318-321), and proposeAgentImprovement runs the same check at creation (line 175), so any drift fails closed. The normalization re-derives the public profile: hooks are re-rendered as a shell command via shellQuote joining executable+args ([lines 355-367](https://github.com/tangle-network/agent-runtime/blob/40d53334b8fdf7ca5d91657075a128c5d8f8bc5a/src/intelligence/improvement-cycle.t

🟡 LOW canonicalJsonValue doesn't handle Date/RegExp/Buffer/non-POJO objects — src/intelligence/improvement-cycle.ts

Non-plain objects like Date, RegExp, and class instances fall through to the typeof==='object' branch (line 499). Object.entries(new Date()) returns [], turning such objects into {}. Object.entries(/test/) also returns []. While none of the current improvementEvaluation or finding fields are these types, a future SelfImproveResult or AnalystFinding change adding such types would silently corrupt digest computations. Fix: add an explicit check for plain-object vs exotic-object (e.g., checking Object.prototype.toString).

🟡 LOW payloadAttributes:'full' ships unbounded payload size in a single span attribute — src/intelligence/index.ts

The old previewJson bounded payloads to 4096 chars with a [truncated] marker. serializeJson (lines 407-419) now returns the full JSON.stringify output and addPayloadAttributes (lines 421-431) writes it verbatim into labels[key] whenever includeFullPayload is true, with no size cap. The with-intelligence test exercises a 5000-char input to prove no truncation. Impact: when an operator opts into payloadAttributes:'full', a multi-MB input/output/profile is emitted as one OTLP attribute; many OTLP backends drop or reject oversized

🟡 LOW Test attrsOf ignores OTLP array/kvlist/bytes attribute types — src/intelligence/with-intelligence.test.ts

The test helper only extracts stringValue, intValue, doubleValue, and boolValue from span attributes. It silently drops arrayValue, kvlistValue, and bytesValue. If any future span attribute uses these OTLP types, tests would report them as undefined, potentially masking bugs. Low severity because no current attributes use these types, but the helper should either handle them or throw on unhandled types to prevent silent test gaps.

🟡 LOW Runtime-event token summarization drops cachedInput and reasoning tokens — src/intelligence/with-intelligence.ts

summarizeRuntimeEvents accumulates only tokensIn/tokensOut (lines 124-126) into summary.inputTokens/outputTokens, and exportCompleted builds the fallback tokens object as { input, output } only (lines 226-230). RunRecord.tokens (index.ts:176-181) supports cachedInput and reasoning, and index.ts:556-567 exports gen_ai.usage.cache_read_input_tokens / reasoning_tokens when present — but those fields are only ever populated from an explicit report.tokens, never from runtimeEvents. Impact: a caller that stream

🟡 LOW Unrecorded successful runs now export tangle.outcome.success = true by default — src/intelligence/with-intelligence.ts

Success is resolved as report.success ?? (caught !== undefined ? false : (eventSummary.success ?? error === undefined)). For a run that returns normally, records nothing, and emits no runtime events, all inputs are undefined and the expression yields error === undefined → true, so the run span carries tangle.outcome.success = true. The previous code only emitted the success attribute when the agent explicitly recorded it (...(report.success !== undefined ? { success: report.success } : {})), so an un-instrumented run exported no success value. Impact: dashboards/billing that count success by tangle.outcome.success === true will now treat every fire-and-forget run as a success even when the agent's output was never evaluated; this is an unannounced semantic shift. The complement (caug

🟡 LOW exportCompleted not wrapped in try/catch — telemetry-side throw can fail a successful agent turn or mask the original error — src/intelligence/with-intelligence.ts

exportCompleted (lines 222-273) runs summarizeRuntimeEvents and constructs the RunRecord BEFORE calling client.exportRunRecord (which has its own try/catch). The call sites are: try { output = await agent(...); exportCompleted(output); return output } catch (cause) { exportCompleted(undefined, cause); throw cause }. If exportCompleted throws in the success path (e.g., a malformed runtime event causes summarizeRuntimeEvents to throw), the catch block treats the telemetry error as 'cause', calls exportCompleted again (may throw again), and throws the telemetry error — the successful agent output is lost. If exportCompleted throws in the catch path,

🟡 LOW success always derived and defaults to true on no-signal runs — behavior change from optional to always-present — src/intelligence/with-intelligence.ts

The old code (diff -) used: ...(report.success !== undefined ? { success: report.success } : {}) — success was omitted from the RunRecord when the agent didn't report it. The new code (line 240-242) always sets success to a boolean via: report.success ?? (caught !== undefined ? false : (eventSummary.success ?? error === undefined)). When the agent completes without error, without explicit success, and without a final event, success defaults to true (because error === undefined). Downstream consumers that previously distinguished 'success unknown' (attribute absent) from 'success: true' now see success=true unconditionally. The exported span always

🟡 LOW Error span status not tested — backend_error and non-completed final paths unverified — src/otel-export.ts

The span.status override (code 2 for backend_error and non-completed final events) is the most security-relevant path — it's what makes failures visible in trace dashboards. No test covers it. A regression that silently drops the status override would hide all backend errors from operators. Add a test asserting span.status.code === 2 and span.status.message for both a backend_error event and a final event with status 'failed'.

🟡 LOW Hardcoded GenAI semconv strings instead of GEN_AI constant; missing gen_ai.provider.name — src/otel-export.ts

The llm_call branch hardcodes 'gen_ai.usage.input_tokens', 'gen_ai.usage.output_tokens', etc. as string literals instead of reusing the file-level GEN_AI constant (lines 73-79). The file's own comment (line 71) says 'We use provider.name + input/output_tokens' but neither the GEN_AI constant nor this function sets gen_ai.provider.name. Pre-existing gap inherited by the new code. Minor consistency nit; functionally the spans will render in any OTel-compatible viewer but may fail strict GenAI semconv validators.

🟡 LOW Missing test coverage for backend_error, final, and redact callback paths — src/otel-export.ts

Tests cover tool_call (with MCP identity) and llm_call, plus default-redact for tool payloads. The backend_error (lines 300-302), final (lines 303-309), error-status override (lines 318-323), and customer redact callback (line 269) paths have zero test coverage. The logic is straightforward and follo

🟡 LOW Non-finite numeric attrs serialize as invalid OTLP/JSON (doubleValue: null) — src/otel-export.ts

costUsd, tokensIn, tokensOut, latencyMs are copied into attrs without finite checks (lines 294-297); toAttributes (608-620) maps a non-integer number to { doubleValue: value }, and JSON.stringify turns NaN/Infinity into null — confirmed by probe: wire attr becomes {"key":"tangle.cost.usd","value":{"doubleValue":null}}, which violates the OTLP/JSON schema and can get the whole /v1/traces batch rejected by a strict collector (telemetry data loss). Same root cause as the RangeError finding; a Number.isFinite guard in toAttributes fixes it globally for every builder in this file.

🟡 LOW Recoverable backend_error events always produce ERROR span status — src/otel-export.ts

The condition event.type === 'backend_error' unconditionally sets status code 2, ignoring the event's recoverable: boolean field. OTel convention distinguishes transient/retried errors from terminal ones. Since each event maps to its own span and recovery is a separate final event, this is a defensible per-event semantic — but if the backend retries and succeeds, the trace will still show a red ERROR span for the transient failure. Consider gating on !event.recoverable or documenting the intent.

🟡 LOW Untimestamped events get export-time (Date.now) span timestamps, distorting trace timelines — src/otel-export.ts

Events with missing/unparseable timestamp (optional on tool_call/tool_result/llm_call/text_delta/reasoning_delta variants) fall back to Date.now() evaluated when spans are BUILT, not when events occurred. The intelligence flow (src/intelligence/index.ts:631) buffers runtimeEvents until run end and builds spans at export time, so every untimestamped event clusters at export wall-clock with zero duration (or start=now/end=now+latency for llm_call), interleaved falsely with correctly-timestamped events. Deterministic from the code; data-fidelity impact only. Consider falling back to monotonic interpolation between neighboring timestamped events or the run span's start, or document the behavior on RuntimeEventOtelOptions.

🟡 LOW serialized has no size bound on span attributes — src/otel-export.ts

tangle.runtime.event (line 276) and tool.input/tool.output (line 289) serialize the full sanitized record via serialized() with no size limit. When includeControlPayloads is true, a large tool result (e.g. 10MB file read) produces an oversized span attribute that some OTEL collectors reject (e.g. Datadog 25KB per-attribute limit). This is by-design 'lossless' per the function JSDoc, but callers should be aware that opting into payloads can silently drop spans if the collector enforces attribute size limits. Consider documenting this in the JSDoc or add

🟡 LOW Fixture grader artifact locators disagree on key — tests/helpers/candidate-execution-fixture.ts

task.grader.artifact.locator uses key 'grader/fixture', while createCandidateOutputFixture's grader port artifact (put at line 233) uses key grader/<sha256hex> for the same bytes. sha256 and byteLength match, and no src code or test currently compares the two refs (prepare.ts:292 signs task.grader into the plan; outcome-evidence.ts:278 records the port's artifact in the result), so there is no functional break today. But the fixture models an evaluator that pins one immutable grader artifact; the divergent keys weaken that invariant and would produce a confusing failure if a future test deep-equals plan vs result grader artifacts. Fix: derive

🟡 LOW Grader artifact locator key inconsistency between task.grader and output fixture — tests/helpers/candidate-execution-fixture.ts

task.grader.artifact.locator.key is hardcoded as 'grader/fixture' (line 366), while createCandidateOutputFixture's put() generates the key as grader/<sha256-hex> (line 225). Both refs share the same sha256 and byteLength, and reads are sha256-keyed so this has zero functional impact. However, task.grader is embedded into the signed execution plan material (prepare.ts:292), so the locator key 'grader/fixture' becomes part of canonicalized evidence. For consistency and to avoid confusing future reader

🟡 LOW Locator key inconsistent between task identity and output fixture — tests/helpers/candidate-execution-fixture.ts

The grader artifact locator key is hardcoded as 'grader/fixture' (line 366), while createCandidateOutputFixture's put function (line 225) computes keys as 'grader/<sha256_hex>'. Both reference the same sha256 (same fixtureGraderBytes), so correctness is unaffected — the read path resolves by sha256, not locator. This is a test-only fixture inconsistency. If a future test were to compare artifacts by locator instead of sha256, it would spuriously fail. Align the key format or document the divergence.

🟡 LOW Evidence assertion omits two of the four receipt digests — tests/improvement-cycle.test.ts

executed.evidence is asserted with toMatchObject over proposalDigest/reviewDigest/bundleDigest/executionId/succeeded/runReceiptDigest, but CandidateExecutionEvidence also carries executionPlanDigest and materializationReceiptDigest (improvement-cycle.ts:99-100) — the two digests that bind the receipt to the exact materialized plan. A regression that dropped or corrupted those fields would pass this test (toMatchObject ignores unlisted keys, and it is the file's own stated goal to link 'one exact receipt'). Fix: extend the matchObject with executionPlanDigest: expect.stringMatching(/^sha256:/) and materializationReceiptDigest: expect.stringMatching(/^sha256:/).

🟡 LOW Happy-path test asserts request presence but not its content — tests/improvement-cycle.test.ts

After execution the test only asserts expect(request).toBeDefined(). The sibling suite (tests/candidate-execution-execute.test.ts:108-121) asserts the same request carries trace tags, launch env isolation (no PATH leak), and hard/observed limits. Adding e.g. expect(request?.trace.tags).toMatchObject(...) or an env-isolation assertion would make this integration test catch a regression where the approved-candidate path drops trace linkage or leaks evaluator env, rather than only catching 'executor was called'. Fix: assert one or two request fields (trace tags + launch.env keys) already validated by the lower-level suite.

🟡 LOW No coverage for the 'request-changes' review decision branch — tests/improvement-cycle.test.ts

AgentImprovementReviewDecision = 'approve' | 'reject' | 'request-changes' (improvement-cycle.ts:79). Tests cover approve (line 162), reject (line 234), and unauthorized-approve (line 280), but never exercise 'request-changes'. Per improvement-cycle.ts:233, executeApprovedAgentCandidate treats any non-approve decision identically (throws 'not an approval'), so this is not a correctness hole — but

🟡 LOW No test coverage for 'request-changes' review decision — tests/improvement-cycle.test.ts

The AgentImprovementReviewDecision type includes 'request-changes' but no test exercises this path through reviewAgentImprovementProposal. Both 'approve' and 'reject' are tested, and executeApprovedAgentCandidate correctly rejects 'reject', but 'request-changes' is never validated to produce a structurally valid review without the approval guardrails (gateDecision + candidateBundle checks at improvement-cycle.ts:206-211).

🟡 LOW No test for improvement.shipped=false path (proposer returns no winner) — tests/improvement-cycle.test.ts

All test proposers return a winner with a score that passes the gate (gateDecision === 'ship'). The improvement.shipped = false path at improvement-cycle.ts:172-173 results in candidateBundle = undefined, which then causes reviewAgentImprovementProposal to reject approval with 'candidate cannot be approved until an executable bundle is sealed'. This path is untested — a regression in the improvement.shipped logic or the gateDecision filtering could go undetected.

🟡 LOW Optional chaining on candidateBundle?.digest weakens evidence assertion — tests/improvement-cycle.test.ts

Line 214: bundleDigest: proposed.proposal.candidateBundle?.digest — if candidateBundle were somehow undefined (impossible in this test flow since executeApprovedAgentCandidate would throw first), toMatchObject would silently match bundleDigest: undefined against the evidence. The optional chaining provides no benefit here and could mask a regression in a code path that changes how candidateBundle is populated. Replace with proposed.proposal.candidateBundle.digest since line 152 already asserts it exists.

🟡 LOW afterEach calls vi.restoreAllMocks() but no test installs a vi.spyOn — tests/improvement-cycle.test.ts

Lines 129-132: afterEach(() => { cleanupCandidateFixtures(); vi.restoreAllMocks() }). None of the four tests call vi.spyOn, vi.fn with mocked modules, or vi.doMock — the executors and registries are hand-rolled objects. vi.restoreAllMocks() is therefore dead. Harmless, but it implies a mock discipline that isn't actually in play and may mislead future contributors into thinking mocks are being cleaned up. Either remove the call or add a comment noting it's a guard for future spies.

🟡 LOW alignedBundle helper only propagates systemPrompt, silently dropping other profile changes — tests/improvement-cycle.test.ts

The helper at line 121 accepts profile: { prompt?: { systemPrompt?: string } } and only maps systemPrompt to the bundle. If the improvement modified model, tools, MCP, hooks, or other profile fields (which is possible when surface !== 'prompt'), those changes would be silently dropped in the sealed bundle. The assertCandidateProfileBinding call in proposeAgentImprovement (improvement-cycle.ts:175) would then fail for those surfaces, but the test would misleadingly pass for surface: 'prompt' alone. This helper should at minimum carry all profile fields or be explicitly scoped to the prompt surface.


tangletools · 2026-07-11T09:19:18Z · trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

❌ 3 Blocking Findings — 40d53334

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

Full immutable report for this review: trace

Summary comment for this run: full summary


tangletools · 2026-07-11T09:19:18Z · immutable trace

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

✅ Auto-approved drewstone PR — 2e8c889d

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

tangletools · auto-approval · reason: drewstone_author · 2026-07-11T15:33:33Z

@drewstone drewstone merged commit 5cd1b9b into main Jul 11, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants