Skip to content

feat(release): adopt Agent Interface 2 cohort - #178

Merged
drewstone merged 4 commits into
mainfrom
fix/interface-2-peer-20260831
Aug 31, 2026
Merged

feat(release): adopt Agent Interface 2 cohort#178
drewstone merged 4 commits into
mainfrom
fix/interface-2-peer-20260831

Conversation

@drewstone

Copy link
Copy Markdown
Contributor

Summary

  • Release @tangle-network/agent-knowledge as 11.0.0 for the Agent Interface 2.0.0 and Agent Eval 0.171.0 cohort.
  • Pin Zod 4.5.4 to match both cohort packages and keep one schema implementation.
  • Fix the exported knowledge activation schema so TypeScript 7 emits portable declarations.
  • Add activation contract regressions and package verification for private declaration paths and Zod alignment.
  • Refresh direct dependencies and the lockfile after Eval 0.171.0 became public.

Verification

  • pnpm exec biome check src tests scripts/verify-package.mjs
  • pnpm typecheck
  • pnpm exec vitest run tests/kb-improvement/contracts.test.ts tests/kb-improvement/activation.test.ts (13 passed)
  • pnpm build (35 files)
  • pnpm run verify:package (public Eval 0.171.0, Interface 2.0.0, Core 0.9.6, and Zod 4.5.4; portable declarations and five package imports passed)
  • pnpm outdated --recursive (no output; exit 0)

Release prerequisite

Eval 0.171.0 is public on npm and PyPI.
Its npm tarball shasum is c82e612719fe793b6b37f73c720789b3205c0eae.
Its npm integrity is sha512-SBbzZIDdoqeA8QmbpnGAKpOO5hDBgCIMNnmt2/vDpvTqCKpOA+tqQw8vROx183DrMPC0rmmnVR8qWBq508dF4w==.

tangletools
tangletools previously approved these changes Aug 31, 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 — c63e8465

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-31T16:30:11Z

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟡 Value Audit — sound-with-nits

Verdict sound-with-nits
Coverage 2 of 2 lenses (value, usefulness)
Concerns 4 (4 weak-concern)
Heuristic 0.0s
Duplication 0.0s
Interrogation 274.6s (2 bridge agents)
Total 274.6s

💰 Value — sound-with-nits

A disciplined major release that moves the dependency cohort to Interface 2 / Eval 0.171.0 with one shared Zod copy, plus a type-first schema annotation and a real declaration-portability guard — everything it claims verified in this checkout, only minor nits.

  • What it does: Releases @tangle-network/agent-knowledge 11.0.0 on the Agent Interface 2.0.0 / Agent Eval 0.171.0 cohort: peer ranges move to >=0.171.0 <0.172.0 and ^2.0.0 (package.json:87-88), dev pins follow (94-95), and Zod is exact-pinned 4.5.4 (84) with a minimumReleaseAgeExclude entry (122). It rewrites the persisted activation-record contract as an explicit interface with the schema annotated z.ZodType<Kno
  • Goals it achieves: Keep exactly one copy of every cohort package (eval, core, interface, zod) in a consumer install, so schema types are structurally identical at compile time and runtime — the pin is provably correct, since installed agent-interface 2.0.0 and agent-eval 0.171.0 both depend on zod exactly 4.5.4 (node_modules/.../agent-interface/package.json:88, agent-eval:207). Make published declarations reference
  • Assessment: Good on its merits and squarely in the codebase's grain. The verify-package additions copy the script's established idiom exactly (exactVersion mirrors exactDevelopmentPin at 285-298; declarationFiles mirrors the javascriptFiles walker; the zod single-copy check extends the existing cohort assertion at 199-203). The type-first annotation matches how the cohort sibling itself ships — agent-interfac
  • Better / existing approach: none — this is the right approach. I searched for alternatives: publint and attw (already in the verify:package chain) check export resolution and manifest lint, not .pnpm/absolute-path content inside declaration text; scripts/check-api-surface.mjs records shape digests, not path portability; no shared d.ts-portability or schema-annotation helper exists in src or scripts. The one empirical wrinkle
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 2
  • Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error event without a message: {"type":"error","timestamp":1788193939485,"sessionID":"ses_fa755e252ffede3eTowciizJmq","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_6fca5b2e"}}}

🎯 Usefulness — sound

A clean cohort-release PR: the schema rewrite is on the live activation-persistence path, the zod pin and portability checks extend the existing release gate rather than inventing a new one, and I reproduced the build, tests, and api-surface checks locally.

  • Integration: Everything changed is reachable and used. knowledgeImprovementActivationRecordSchema is not new surface but a rewrite of live runtime code: persistKnowledgeActivationResult parses it before durable write (src/kb-improvement/activation.ts:152) and loadKnowledgeActivationRecord parses it on every read-back (src/kb-improvement/activation.ts:207), both driven by the exported promote/restore transition
  • Fit with existing patterns: Follows established patterns rather than competing. Peer ranges keep the house style (exact minor window for agent-eval >=0.171.0 <0.172.0, caret for agent-interface ^2.0.0, exact dev pins) and the verify script extends the existing single-installed-copy proof to zod instead of adding a second mechanism. The explicit-interface-plus-z.ZodType-annotation pattern is new to this repo (only occu
  • Real-world viability: Holds up beyond the happy path. I ran the new contract tests: 4/4 passed (round-trip alignment plus three traversal/control-char candidateId rejections matching the repo's path-containment doctrine). The schema's only consumers use .parse (no .shape/narrowing that the ZodType widening would break, verified by grep across src). The verify additions fail loud with typed errors (exactVersion rejec
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

🎯 Usefulness Audit

🟡 Interface↔schema drift guard has an optional-field blind spot [robustness] ``

The z.ZodType annotation (src/kb-improvement/contracts.ts:185) makes TypeScript catch drift on required fields, and the new round-trip test (tests/kb-improvement/contracts.test.ts:35) catches unknown-key and constraint drift. But an optional field added to the interface without a matching schema entry would pass both (output lacking an optional is still assignable; the test fixture wouldn't set it). The standard mitigation would be a satisfies-style two-wa

🟡 Exact-string coupling to agent-interface's zod dependency format [problem-fit] ``

verify-package.mjs now asserts installedAgentInterface.dependencies.zod === '4.5.4' exactly. If a future agent-interface 2.0.x declares '^4.5.4' or moves to 4.5.6 within this package's caret peer range, verify:package fails even though the peer range admits it. This is a conscious cohort lock, not a bug, and it fails loud at release time — but a human should know the gate now pins the dependency format of a peer package, slightly tighter than the existing version-admission checks (assertCaretA

💰 Value Audit

🟡 Type-first annotation widens the static type beyond the runtime contract [maintenance] ``

The explicit interface declares candidateId: string while the schema enforces safePathSegmentSchema (1-128 chars, [A-Za-z0-9._-]) at contracts.ts:161-165; a caller can now statically construct a record parse would reject. This is the inherent tradeoff of the z.ZodType pattern (Input defaults to unknown) and the new test covers the runtime side, so it does not gate — but if more schemas adopt the pattern, a SafePathSegment branded type would restore the lost precision.

🟡 declarationFiles duplicates the javascriptFiles walker [duplication] ``

scripts/verify-package.mjs:430-448 now has two structurally identical recursive walkers differing only in the extension test. A shared walk(directory, filter) would remove the copy, but each checker owning its own walker matches the script's existing per-assertion style; trivial.


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 · 20260831T163652Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — c63e8465

Review health 100/100 · Reviewer score 46/100 · Confidence 95/100 · 32 findings (2 medium, 30 low)

opencode GLM 5.2 opencode DeepSeek v4 Pro opencode DeepSeek v4 Flash aggregate
Readiness 52 77 46 46
Confidence 95 95 95 95
Correctness 52 77 46 46
Security 52 77 46 46
Testing 52 77 46 46
Architecture 52 77 46 46

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

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

🟠 MEDIUM Single-zod-copy guarantee claimed but not proven; agent-eval zod drift undetected unless npm hoists it — scripts/verify-package.mjs

Lines 158-160 read only top-level node_modules/zod/package.json; line 174 checks only agent-interface's DECLARED zod. installedAgentEval.dependencies?.zod is never read, and no npm ls zod --all --parseable single-copy proof exists (contrast assertSingleInstalledAgentStack at line 355). Experiment: with fake-knowledge/interface pinning zod 4.5.4 and fake-eval pinning 4.5.3, npm install of the three t

🟠 MEDIUM zod single-copy invariant is asserted for agent-interface only; a nested duplicate zod under agent-eval passes verification — scripts/verify-package.mjs

Lines 171-178 check installedZod.version and installedAgentInterface.dependencies.zod, but agent-eval's (and agent-core's) zod dependency is never asserted, and zod is not added to assertSingleInstalledAgentStack (line 199), which is the only check that proves 'one installed copy' via npm ls. If agent-eval later pins a different exact zod, npm installs a nested node_modules/@tangle-network/agent-eval/node_modules/zod, every existing check still passes, and [line 263](https://github.com/tangle-network/agent-knowledge/blob/c63e84

🟡 LOW 11.0.0 entry omits the consumer-migration note that every prior peer-range bump carried — CHANGELOG.md

Interface jumps from ^1.6.0 to ^2.0.0 (a peer major), and Eval moves to 0.171.x. Prior entries that moved the peer cohort (10.7.1, 10.7.0, 7.2.0) explicitly warned consumers: 'A consumer must move Eval and Interface with this package.' — installing 11.0.0 beside Interface 1.x or Eval 0.170.x is an unmet peer that blocks the install. The new entry says only 'Consumers now resolve one canonical profile, candidate, and tool-definition type' and lists no 'Breaking Changes' heading or 'What to do' action, so an upgrading consumer has no stated migration step. Evidence: diff at CHANGELOG.md:7-10 vs the 10.7.1 entry at CHANGELOG.md:39-42 and the 10.0.0 major at CHANGELOG.md:123-127. Fix: add a bolded consumer-action line naming the two peer moves, and consider a '### Breaking Changes' section

🟡 LOW Changelog omits activation-record schema annotation and portable-declaration verification work — CHANGELOG.md

Commit 411beae ('fix(package): annotate activation record schema') re-typed knowledgeImprovementActivationRecordSchema as an explicit interface with a z.ZodType<> annotation, added assertPortableDeclarations plus zod-cohort checks to scripts/verify-package.mjs, and added 49 lines of tests (tests/kb-improvement/contracts.test.ts). None of this appears under 11.0.0. The exported type shape is unchanged, so consumer impact is nil and the omission does not mislead; still, the repo's changelog convention (see 10.7.1's exhaustive entries) would record it. Fix: add a short bullet under Changed or Fixed noting the activation-record schema annotation and the portable-declaration gate.

🟡 LOW Install command pins exact versions; agent-knowledge@11.0.0 not yet on registry — README.md

The command installs exact pins: pnpm add @tangle-network/agent-knowledge@11.0.0 @tangle-network/agent-eval@0.171.0 @tangle-network/agent-interface@2.0.0. The two peers exist on npm (verified 0.171.0 and 2.0.0), but agent-knowledge@11.0.0 currently 404s (latest published is 10.8.0). This is normal for a release PR whose publish is tag-triggered post-merge, so not a defect in this change; if publish fails or is delayed, the documented command breaks. Exact-pin convention also means the README goes stale each release cycle (it previously advertised 8.0.8 while 10.8.0 was latest). No action required beyond confirming the 11.0.0 publish lands.

🟡 LOW Exact zod pin guarantees one schema copy only within the cohort, not across consumers — package.json

zod moves from ^4.4.3 to the exact pin 4.5.4 to 'keep one schema implementation' (CHANGELOG 11.0.0). zod is a direct dependency, not a peer, so a consumer whose own tree resolves zod 4.4.x exactly (a range 4.5.4 does not satisfy) installs a second zod copy, and the two zod schemas remain distinguishable by instanceof. Acceptable for the stated cohort goal, and partially self-healing because agent-interface@2.0.0 also pins 4.5.4 (verify-package.mjs asserts it), so dedupe works whenever the consumer range admits 4.5.4. Nit only; no fix required unless cross-copy schema identity must hold for all consumers.

🟡 LOW Runtime dependency added to pnpm minimumReleaseAgeExclude supply-chain exemption — package.json

zod@4.5.4 is added to pnpm.minimumReleaseAgeExclude while pnpm.minimumReleaseAge stays 4320. Every prior entry is an internal @tangle-network package or a dev-only tool (esbuild, vite); zod is the first runtime dependency shipped to consumers to be exempted from the 3-day release-age guard, so a freshly published zod@4.5.4 installs without the age check. Impact is narrow because the dep is exact-pinned and the repo installs from a frozen lockfile, but the guard is deliberately relaxed for a runtime dep. Fix: leave the entry only if zod 4.5.4's age requires it; otherwise drop it, or document why a runtime dep needs the exemption.

🟡 LOW minimumReleaseAgeExclude admits zod published inside the repo's own 72h supply-chain soak window — package.json

minimumReleaseAge is 4320 minutes (3 days), and zod@4.5.4 was published 2026-08-29 (verified via npm view) while this PR is dated 2026-08-31, so install only resolves because 'zod@4.5.4' was added to minimumReleaseAgeExclude. This is deliberate (CHANGELOG: 'Pin Zod 4.5.4 to match Interface 2') and scoped to the exact version so a future publish cannot ride the exclusion, but it means the package ships depending on a release younger than the window the repo set to defend against freshly-compromised publishes. Acceptable given the upstream cohort forces 4.5.4; consider removing the exclude once the version ages past 3 days so the gate's default posture is restored for any later bump. No action required to merge.

🟡 LOW Dev toolchain engine gap for node 21.x — pnpm-lock.yaml

New transitive dev-toolchain deps declare engines '^20.19.0 || >=22.12.0' (oxc-parser@0.147.0, rolldown@1.2.6, vite@8.2.2), which excludes node 21.x. Repo package.json engines is '>=20.19.0', nominally permitting node 21.x. On node 21.0-21.6 a fresh install would emit engine warnings or fail under engine-strict. Dev-only (build/lint/test tooling), node 21 is EOL, and the main runtime deps (proper-lockfile, zod 4.5.4) carry no such constraint, so impact is nil in practice. Informational only; consistent with the previous 0.144.0/1.1.5 entries which already used the same range.

🟡 LOW Major transitive bump: @noble/hashes 1.8.0 -> 2.4.0 via agent-interface 2.0.0 — pnpm-lock.yaml

agent-interface jumped 1.6.0 -> 2.0.0 and its resolved snapshot now depends on @noble/hashes@2.4.0 (packages entry line 367) instead of 1.8.0, a major-version API change. This is the deliberate release cohort from commit 'feat(release): adopt Agent Interface 2 cohort', and the lockfile correctly reflects it (validated: all refs resolve, engines >=20.19.0 match repo engines). Impact is on the code-review shots: agent-knowledge runtime code that re-exports or uses agent-interface types/hashing must be confirmed compatible with the 2.x API. No lockfile action needed; verified the old 1.8.0 entry is fully removed (grep count 0).

🟡 LOW Transitive major bump of @noble/hashes 1.8.0 -> 2.4.0 via agent-interface 2.0.0 — pnpm-lock.yaml

agent-interface@2.0.0 now depends on @noble/hashes@2.4.0 (was 1.8.0); pnpm removed the 1.8.0 entry entirely, so no other package in the tree requires 1.x. This is a major-version jump of a cryptographic hashing library consumed transitively through the public agent-interface dependency. No known CVE and no resolution conflict, but the verifier should confirm the interface-2.0 API-surface/contract checks (other shots) account for any @noble/hashes 2.x behavioral change (e.g. return-type/byte-encoding differences).

🟡 LOW zod pinned to 4.5.4, a release exempted from the minimumReleaseAge supply-chain guard — pnpm-lock.yaml

The lockfile pins zod to exactly 4.5.4 (specifier '4.5.4', resolution at line 2542 with integrity sha512-X5XaVWZ...). package.json (outside this shot) adds 'zod@4.5.4' to minimumReleaseAgeExclude, bypassing the 4320-minute (3-day) release-age guard for this fresh version. zod is a runtime dependency of the published package, so a compromised-or-broken fresh release would flow straight to consumers; the exemption also converts the prior caret range (^4.4.3) to an exact pin, freezing every consumer install at the unaged version. Impact is mitigated by the integrity hash and the scoped (single-version) exclusion. Fix: once 4.5.4 is >3 days old, drop the minimumReleaseAgeExcl

🟡 LOW .pnpm substring match can false-positive on comment text — scripts/verify-package.mjs

/.pnpm[/\]/ scans raw file text, so a JSDoc comment or string literal mentioning '.pnpm/' (e.g. documentation about pnpm behavior carried into dist) fails verification even when all specifiers are portable. No src file currently contains '.pnpm' (grep'd src/), so this cannot fire today; a comment-position guard or anchoring to import syntax would remove the spurious-failure mode.

🟡 LOW Final message claims 'one installed copy ... and zod' but single-copy is never verified for zod — scripts/verify-package.mjs

The summary prints 'one installed copy each of ... and zod', but zod is not passed to assertSingleInstalledAgentStack (line 199 only covers agent-eval, agent-core, agent-interface). The zod check only reads the top-level node_modules/zod/package.json version. pnpm-lock.yaml contains a separate zod@3.25.76 entry (a transitive dependency still pulls zod v3), so npm's install used by this script can legitimately install a nested zod@3 copy alongside zod@4.5.4 without this script noticing. The claim overstates the guarantee. If single-zod-copy is the real intent, add zod to the assertSingleInstalledAgentStack call (or an npm ls zod assertion).

🟡 LOW Portable-declarations regexes false-positive on comments/JSDoc and miss file:// and triple-slash absolute refs — scripts/verify-package.mjs

The check at lines 304-307 applies both regexes to raw file text, so any comment/JSDoc containing 'from "/abs"', 'import "/abs"', or 'from "C:..."' flags the file and fails the release gate. Verified by direct execution: '// from "/usr/lib/foo"' and '/** import { a } from "C:\foo" */' both match the absolute-path regex. Generated .d.ts routinely preserves JSDoc. Conversely the regex misses 'from "file:///..."' and '/// ' (no from/import token), so coverage is incomplete. Impact: spurious release blocker on an unrelated comment; absolute file:// refs can slip through. Fix: strip comments before matching (or restrict to lin

🟡 LOW Published agent-knowledge manifest's own zod dependency is never asserted — scripts/verify-package.mjs

assertPublishedRequiredPeer (lines 179-180) validates only the two agent peers in the packed manifest. Nothing checks installedPackage.dependencies?.zod === zodVersion, so a packaging regression that drops or retypes zod in the shipped manifest goes undetected: zod still installs transitively via agent-eval/agent-interface, the top-level read at line 158 passes, and the gate is green while the published package silently loses its declared zod dependency. Symmetric to the peer assertions. Fix: add an assertPublishedRequiredDepen

🟡 LOW absolute-path regex misses import("/abs") type-form, UNC paths, and triple-slash references — scripts/verify-package.mjs

/(?:from|import)\s*"'/ requires the quote directly after from/import, so declare const a: import("/abs/path").X (a very common tsc type-position emission), \server\share UNC paths, and /// are all unflagged (verified synthetically). The .pnpm textual scan independently covers the main pnpm-store leak, so this is a robustness gap, not a hole for the known failure mode. Tightening to also match import(\s*["'] with a leading / or drive letter would close most of it.

🟡 LOW assertPortableDeclarations regexes lack the self-test the other matcher has — scripts/verify-package.mjs

hasStaticImport has a dedicated self-test (assertEdgeUnsafeStaticImportMatcher, lines 413-428) that validates positive/negative cases. The two new regexes in assertPortableDeclarations (\.pnpm[/\\]|node_modules[/\\]\.pnpm and (?:from|import)\s*["'](?:\/|[A-Za-z]:[/\\])) have no such self-test, so a regex regression (e.g. an unanchored 'from'/'import' matching inside a JSDoc comment) would only surface as a spurious CI failure against real dist output. Minor: add a small matcher self-test mirroring the existing pattern.

🟡 LOW exactVersion duplicates exactDevelopmentPin logic — scripts/verify-package.mjs

exactVersion (line 293) is the same /^\d+.\d+.\d+$/ + throw shape as exactDevelopmentPin (line 285); only the error message differs via the description param. exactDevelopmentPin could call exactVersion (or a shared helper) with the package name as the description, removing the duplicated regex and control flow. Nit only; no behavior change.

🟡 LOW new portability matcher has no self-test despite the established pattern in this script — scripts/verify-package.mjs

The sibling regex matcher hasStaticImport is guarded by assertEdgeUnsafeStaticImportMatcher (lines 413-428) with labeled positive/negative cases run on every invocation; assertPortableDeclarations and exactVersion get no equivalent, so a regex regression (e.g., the import()-form gap in the previous finding) ships silently. declarationFiles (line 440) also duplicates the javascriptFiles walker shape for a one-suffix difference. Adding a labeled case table like the sibling's would match the file's own convention.

🟡 LOW zod manifest check demands exact pin, contradicting the file's own caret-range rationale — scripts/verify-package.mjs

if (installedAgentInterface.dependencies?.zod !== zodVersion) requires agent-interface to declare an EXACT zod version. This is the opposite of the principle documented at lines 181-183, where assertCaretAdmits is used because 'the cohort is proven by the single installed copy, not by the specifier's shape' and exact pins duplicate packages. If agent-interface later declares zod: ^4.5.0 (a caret range that npm would still dedupe to a single 4.5.4 copy), installedZod.version === zodVersion passes but this check throws 'agent-interface must use zod 4.5.4, received ^4.5.0', failing CI with no actual dual-zod problem. It also couples agent-knowledge CI t

🟡 LOW Explicit record interface can drift from the schema; the ZodType annotation guards only one direction — src/kb-improvement/contracts.ts

KnowledgeImprovementActivationRecord is now a hand-maintained interface mirrored against knowledgeImprovementActivationRecordSchema, annotated as z.ZodType. TS only enforces that the schema's inferred output is assignable to the interface. It does not enforce the reverse: if a future edit adds a required field to the schema but not the interface, the schema output remains assignable to the narrower interface and tsc passes while .parse() silently returns objects carrying extra data. It also cannot catch a schema that weakens a constraint (interface declares candidateId: string; the schema enforces safePathSegmentSchema — a value constructed without .parse() would only fail at runtime). Current state is fully in sync (verified by round-trip parse, tsc,

🟡 LOW Interface loosens candidateId from safePathSegmentSchema to plain string — src/kb-improvement/contracts.ts

The hand-written interface types candidateId as string while the schema constrains it via safePathSegmentSchema (min 1, max 128, [A-Za-z0-9][A-Za-z0-9._-]*). A caller could construct a type-valid record with candidateId '../escape' that parse then rejects. Every persistence path (persistKnowledgeActivationResult at activation.ts:152 and loadKnowledgeActivationRecord at activation.ts:207) funnels through parse, so no unvalidated value reaches disk; the widening matches the file's existing convention (KnowledgeImprovementCandidateRecord.candidateId is also plain string). Informational only.

🟡 LOW ZodType annotation widens schema type and drops compile-time excess-field drift check — src/kb-improvement/contracts.ts

Annotating as z.ZodType<KnowledgeImprovementActivationRecord> instead of the inferred ZodObject<...> means (a) future consumers cannot .extend/.pick/.shape without a cast, and (b) assignability is one-directional: a schema output field added later but absent from the interface would compile silently until .strict() rejects a valid-looking record at parse time. Current mitigation is adequate (tests/kb-improvement/contracts.test.ts:33-36 round-trips an interface-typed fixture through parse with toEqual, and strict() throws on any field the interface declares but the schema drops), so this is a nit, not a defect. Optionally add a type-level exactness assertion (e.g. a satisfies/Equal check in the test) if the schema grows.

🟡 LOW Alignment test covers only one outcome variant — tests/kb-improvement/contracts.test.ts

The z.ZodType<KnowledgeImprovementActivationRecord> annotation declares that parse() returns the hand-written interface AgentImprovementActivationResult, while runtime validation uses agentImprovementActivationResultSchema. These are two independent type sources that can drift. The new guard test (tests/kb-improvement/contracts.test.ts:35) round-trips a single representative record whose outcome is only { status: 'expired' }, but the schema's outcome is a 6-variant discriminated union (applied, already-applied, conflict, expired, unsupported, failed, indeterminate). Drift in any of the other five variants would not be caught. Impact is type-only (no runtime change); fix by round-tripping one record per outcome variant or adding a compile-time satisfies check

🟡 LOW Fixture mutation asserts changed=false with differing hashes — tests/kb-improvement/contracts.test.ts

The fixture sets changed: false while beforeHash (64xa) differs from afterHash (64xb). The receipt schema has no cross-field refinement so this is schema-valid (the passing test proves it), but it documents a semantically inconsistent receipt as the canonical example. Fix: either set changed: true, or make beforeHash === afterHash with changed: false.

🟡 LOW Mutation receipt transactionId only tested as null — tests/kb-improvement/contracts.test.ts

knowledgeImprovementMutationReceiptSchema constrains transactionId to z.string().uuid().nullable(), but the fixture only ever sets null, so the .uuid() branch is never exercised. A uuid rejection bug would pass this suite. Minor gap; the positive uuid path is left to activation.test.ts.

🟡 LOW No accept-boundary tests for safePathSegmentSchema — tests/kb-improvement/contracts.test.ts

Only the fixture 'candidate-1' exercises the accept path of safePathSegmentSchema (/^[A-Za-z0-9][A-Za-z0-9.-]*$/). Portable edge ids that the production writer (src/kb-improvement/workspace.ts:287, state.ts:249) will happily store — 'candidate.sub', 'candidate-child_1', and the 128-char max-length boundary — are unpinned. A future tightening of the regex (e.g. banning '') would silently reject ids the filesystem layer accepts and this test would not catch it. Low because the negative cases are the security-relevant ones and they are covered.

🟡 LOW No negative tests for record-level strictness or mutation receipt fields — tests/kb-improvement/contracts.test.ts

The it.each block only rejects bad candidateId values. The schema guards the durable read path (loadKnowledgeActivationRecord parses untrusted disk JSON at src/kb-improvement/activation.ts:207), and nothing verifies rejection of an unknown extra key, a wrong kind literal, a non-uuid transactionId, a non-hex beforeHash, or an out-of-enum target. Impact: a future weakening of .strict() or the receipt schema would not fail this test. Fix: add an it.each of invalid records covering kind, mutation.target, mutation.transactionId, and mutation.beforeHash.

🟡 LOW Only the 'expired' outcome variant is exercised — tests/kb-improvement/contracts.test.ts

The fixture always uses outcome: { status: 'expired' }, the only outcome variant with no targets array. The production shapes persisted by persistKnowledgeActivationResult are 'applied' (transactionId + targets tuple) and 'already-applied'/'conflict' (targets state tuple). Impact: the round-trip type-alignment claim is proven only for the degenerate variant. Fix: parameterize the fixture over at least the 'applied' outcome with a single target transition. Mitigated by the result schema being owned and tested by @tangle-network/agent-interface.

🟡 LOW Path-rejection coverage is narrow — tests/kb-improvement/contracts.test.ts

The it.each covers three non-portable paths (../, backslash, NUL) but not the other safePathSegmentSchema constraints: empty string (min(1)), >128 chars (max(128)), a leading non-alphanumeric such as '-foo' or '.foo', or a leading digit. These are the boundary conditions most likely to regress if the schema is loosened later, and the test's stated purpose is to pin the runtime contract. Add cases for '', 'x'.repeat(129), and '-leading' to lock the full regex. Non-blocking; the tested cases are all correct.

🟡 LOW it.each asserts bare toThrow without message or field — tests/kb-improvement/contracts.test.ts

Rejection assertions use expect(...).toThrow() with no message match, so they cannot distinguish a candidateId rejection from an unrelated schema failure. Sound today because the fixture is proven valid by the round-trip test and only candidateId differs, but asserting the zod path (e.g. /candidateId/) would make the intent explicit and the failure diagnosis faster.


tangletools · 2026-08-31T16:53:36Z · 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.

✅ Approved — 32 non-blocking findings — c63e8465

Full multi-shot audit completed 7/7 planned shots over 7 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 7/7 planned shots over 7 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 7/7 planned shots over 7 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-31T16:53:36Z · 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 — 31d74c18

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.

This approval is provisional. It rests on the audit running. If the audit cannot run — for example the CLI bridge rejects it — this approval is dismissed rather than left standing, so an unrun check never reads as a passing one.

tangletools · auto-approval · reason: drewstone_author · 2026-08-31T16:54:34Z

@drewstone
drewstone merged commit 898c92e into main Aug 31, 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