refactor(driver)!: remove the persona-driver surface - #628
Conversation
decideNextUserTurn, DecideNextUserTurnOpts and buildDriverSystemPrompt leave the package with src/driver.ts and the user-simulation-driver example. A role written as a code function cannot be optimized, and a persona driver is an AgentProfile on a graph edge rather than a packaged function. The one caller these had, gtm-agent, now owns the role in its own repository, where the prompt is product data the product can change and measure. Every primitive that call needed stays public: CostLedger.runPaidCall prices and attributes it, maximumChargeForLlmRequest caps it, costReceiptFromLlm records it, and assertServedModel holds the transport to the model id it was asked for. PersonaConfig and DriverState stay too. ConvergenceTracker goes with them. Its only caller was AgentDriver, and it never reached the barrel.
|
@tangletools review now |
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 8738f4ba
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-16T19:38:54Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 8738f4ba
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-16T19:38:59Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 1 (1 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 227.0s (2 bridge agents) |
| Total | 227.0s |
💰 Value — sound
Deletes the deprecated code-authored persona-driver surface (decideNextUserTurn, buildDriverSystemPrompt, orphaned ConvergenceTracker) after migrating its one caller — a clean, correctly-sequenced removal in the grain of the repo's prompts-as-data direction.
- What it does: Breaking removal (0.145.22 → 0.146.0): deletes src/driver.ts (decideNextUserTurn, buildDriverSystemPrompt) and its 329-line test, src/convergence.ts (ConvergenceTracker) and its 105-line test, the user-simulation-driver example + its README row, and the barrel exports of decideNextUserTurn/DecideNextUserTurnOpts (src/index.ts). Also bumps package.json + python client versions, updates the analyst-
- Goals it achieves: Finish the #618 deprecation that 0.145.22 (#626, commit 452ae6c) started by deleting AgentDriver; remove the orphaned ConvergenceTracker (verified at HEAD~1: never in the barrel, only self-references plus its own test — dead code since AgentDriver died); and complete the architectural move that a simulated-user role is product data a consumer can change and measure, not a code function baked into
- Assessment: Good on its merits. The removal is coherent and complete: no dangling references at HEAD (remaining buildDriverSystemPrompt hits are the unrelated MultishotShape callback field, which the PR correctly excludes); warnDeprecatedOnce retains a live user (ProductClient, src/client.ts:22); analyzeSeries is untouched apart from a comment; typecheck passes and the affected suites pass (deprecation + mult
- Better / existing approach: none — this is the right approach. Searched for an existing equivalent: the codebase's other persona simulation, the multishot path (runMultishotMatrix + src/multishot/shape-defaults.ts), already implements the better data-driven variant with caller-overridable prompts and is retained — this PR removes the inferior code-authored duplicate rather than reinventing anything. ConvergenceTracker had no
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
A clean, verified removal of a fully-migrated persona-driver surface that deletes dead code without breaking any live caller or leaving stale references.
- Integration: Nothing calls the deleted surface anymore, and the removal is complete: src/driver.ts, src/convergence.ts, their tests, examples/user-simulation-driver, and the barrel exports (src/index.ts:292-293 removed) are all gone; repo-wide grep finds zero remaining references to decideNextUserTurn/ConvergenceTracker outside CHANGELOG history, and the only buildDriverSystemPrompt hits are the distinct Multi
- Fit with existing patterns: Fits the repo's declared layering exactly: CLAUDE.md positions agent-eval as substrate with no product coupling, and the established in-repo pattern for persona-driven conversations is already the AgentProfile-derived MultishotShape (optional callbacks via defaultShapeFromProfile, src/multishot/shape-defaults.ts:72-73, consumed at src/multishot/multishot.ts:289) — this deletion removes the competi
- Real-world viability: Verified: pnpm typecheck clean; deprecation and benchmark digest tests pass; the version bump 0.145.22 → 0.146.0 (correct 0.x breaking signal) propagates to the python client fallback version and to the regenerated benchmark lock SHA, which the self-recomputing test 'recomputes the pinned digest from the repository sources' passes against. The only test failures observed were Go-subprocess spawns
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
💰 Value Audit
🟡 PersonaConfig/DriverState now have zero in-repo consumers yet stay in the barrel [maintenance] ``
After this change, git grep shows the only in-repo reference to PersonaConfig/DriverState outside src/types.ts is a doc comment (src/rl/sim-fidelity.ts:5) and the barrel re-export (src/index.ts:58,61). The CHANGELOG rationale — a harness writing its own driver still needs the vocabulary — is defensible under the substrate doctrine, but if the migrated gtm-agent persona-driver defines its own local types, these exports have no consumers anywhere. Candidate for the next dead-surface sweep, not a b
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.
feedbackPatterns told AgentDriver which product approvals to reject and driverModel picked its driver model. With the class gone nothing reads either, here or in any repository that depends on this package. A field that advertises behaviour the package no longer has is worse than no field.
|
@tangletools review now |
|
On the The dead fields that concern names in spirit are gone in 91152ee: |
|
@tangletools review now |
|
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 91152ee9
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-16T20:08:48Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 2 (2 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 1201.5s (2 bridge agents) |
| Total | 1201.5s |
💰 Value — sound-with-nits
Deletes the deprecated persona-driver surface (driver.ts, its test, example, and the orphaned ConvergenceTracker) whose sole caller migrated to gtm-agent, and the multishot subsystem already supplies the replacement — a clean, in-grain removal.
- What it does: Removes the deprecated persona-driver surface: src/driver.ts (buildDriverSystemPrompt + decideNextUserTurn + DecideNextUserTurnOpts, 183 lines), src/driver.test.ts (329 lines), examples/user-simulation-driver/ (index.ts + README + examples/README.md row), plus the orphaned ConvergenceTracker in src/convergence.ts and tests/convergence.test.ts. Strips the barrel exports of decideNextUserTurn/Decide
- Goals it achieves: Finishes the staged deprecation of a product-coupled role that no longer belongs in the substrate. The one caller (gtm-agent) already migrated the role into its own repo where the prompt is mutable product data, and the multishot subsystem (MultishotShape.buildDriverSystemPrompt + defaultMultishotDriverSystemPrompt over an AgentProfile, src/multishot/shape-defaults.ts:40-52, src/multishot/types.ts
- Assessment: Coherent and in-grain. It is part two of a plan already begun by #626 (which deleted AgentDriver in 0.145.22), a pure deletion with no behavior delta, so risk is minimal. The version bump to 0.146.0 is correct for a breaking removal pre-1.0, and the benchmark lock-digest update is a mechanical consequence of the version change (those files are in ANALYST_BENCHMARK_DEPENDENCY_LOCK_FILES). typecheck
- Better / existing approach: None materially better — the replacement already exists (src/multishot: MultishotShape.buildDriverSystemPrompt callback + defaultMultishotDriverSystemPrompt), and the change correctly deletes the old surface instead of reinventing it. I grepped for any remaining consumer of the deleted symbols: the only surviving 'buildDriverSystemPrompt' references are the unrelated multishot callback (src/multis
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 4
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error; opencode/zai-coding-plan/glm-5.2: bridge stream ended without value-audit content
🎯 Usefulness — sound
A measured, complete deletion of a zero-caller product-coupled surface whose sole caller migrated first, with the shared persona types deliberately retained because the migrated caller still imports them.
- Integration: Clean and reachable. No dangling references remain: grep for
decideNextUserTurn/buildDriverSystemPrompt/ConvergenceTrackerreturns only the unrelatedMultishotShape.buildDriverSystemPromptcallback (caller-supplied, correctly distinguished) and doc-comment mentions;pnpm typecheckpasses. The one real caller (decideNextUserTurnin gtm-agenteval/kernel.ts) migrated first to gtm-agent - Fit with existing patterns: Follows the repo's own layering doctrine (CLAUDE.md): the prompt is product data that moves OUT to the consumer (gtm-agent can now change/measure it), while the persona/state types are substrate primitives that stay IN. The CHANGELOG documents exactly this split —
PersonaConfigandDriverStatestay 'a harness that writes its own driver still describes a persona and a produced state with them.' - Real-world viability: Deletion PR with no new behavior, so no happy-path/edge-input risk. The only realistic failure mode is merge-ordering, not runtime: if this lands before gtm-agent#901, a published 0.146.0 removes a symbol gtm-agent's master still calls. Both PRs cross-reference (#618/#901) and are in flight, so this is a coordination checkpoint, not a code defect.
analyzeSeries(series-convergence.ts) is untouch - Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 4
- Bridge warning: opencode/zai-coding-plan/glm-5.2: opencode exited 134: timeout: the monitored command dumped core
/home/drew/.local/bin/opencode: line 87: 3358196 Aborted timeout --signal=TERM --kill-after=30s "${RUN_TIMEOUT_SECONDS}s" "$REAL_BIN" "$@"
; opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness Audit
🟡 Deletion is gated on the gtm-agent#901 migration merging first [integration] ``
gtm-agent#901 (the migrated caller) was still
open/unmerged at audit time, and itseval/kernel.tsno longer importsdecideNextUserTurnonly once that PR merges. Merge #901 before/with #628, or gtm-agent's master picks up the breaking 0.146.0 and fails to build. Not a code problem — a release-ordering checkpoint the reviewer should confirm.
💰 Value Audit
🟡 PersonaConfig is left half-cleaned: four fields no in-package code reads, plus a stale 'Agent Driver' header [maintenance] ``
The PR removes PersonaConfig.feedbackPatterns and driverModel because 'a field that advertises behaviour the package no longer has is worse than no field' (CHANGELOG). But rigor, expertise, pressurePoints, and curveballs (src/types.ts:172,179,186,192) were consumed only by the now-deleted buildDriverSystemPrompt (git show main:src/driver.ts), so nothing in the package reads them anymore either — grep across src/ finds them only at their definition. The section header '// ── Agent Driver ──' (src
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
| opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|
| Readiness | 86 | 83 | 83 |
| Confidence | 95 | 95 | 95 |
| Correctness | 86 | 83 | 83 |
| Security | 86 | 83 | 83 |
| Testing | 86 | 83 | 83 |
| Architecture | 86 | 83 | 83 |
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 11 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 11 changed files. Global verifier still owns final merge decision.
🟡 LOW Version duplicated in three files with no sync guard — clients/python/src/agent_eval_rpc/__init__.py
agent-eval-rpc version lives in pyproject.toml:7, init.py:56 (PackageNotFoundError fallback), and uv.lock:37. All three are correctly 0.146.0 in this PR and uv lock --check passes, but nothing automated asserts the fallback string tracks the pyproject version. A future release that bumps pyproject only (or edits init only) silently desyncs installed-vs-source-reported version and the lock. This is a known-friction maintenance smell, not a current defect. Fix option: a tiny test that imports the built wheel/editable and asserts version == the pyproject version, or read the fallback from a single constant imported from a _version.py.
🟡 LOW pnpm build runs --source-only and skips the dependency-lock digest check — src/analyst/benchmark-implementation.ts
package.json 'build' invokes the checker with --source-only, which by design (check-analyst-benchmark-implementation.mjs:54-63) skips the dependency-lock pin. Since package.json's version string is part of the lock manifest, every release rotates this constant; a build stays green even if the rotation is forgotten, and only 'verify:package' / the test suite catch it. Pre-existing design, not introduced by this PR — the rotation here is correct. No action required for this change; noting the only latent risk surface around this constant.
🟡 LOW Docstring says 'surface' singular but 'per surface' later — src/deprecation.test.ts
Header now names only
ProductClientbut retains plural phrasing 'exactly once per process per surface'. Purely cosmetic; does not affect behavior or the tests. Fix: drop 'per surface' or keep generic wording.
🟡 LOW Root export removal ships with no runtime deprecation warning for legacy consumers — src/index.ts
The two removed root exports (
decideNextUserTurn,DecideNextUserTurnOpts) disappear in one step with no transitional shim, so any external consumer importing them from the package root fails at module resolution with a generic error rather than a guided message. Evidence: exports at old lines 291-293 removed; src/driver.ts deleted in 8738f4b; CHANGELOG 0.146.0 documents the migration but no runtime deprecation notice exists. Impact: low — the only known caller (gtm-agent) migrated to its own repo and package version is 0.x where breaking removals are in-policy; the CHANGELOG entry is the migration guide. Fix (optional): a temporary root export that throws a descriptive
🟡 LOW Root-specifier consumers of decideNextUserTurn break with no in-package replacement — src/index.ts
Removing
decideNextUserTurn/DecideNextUserTurnOptsfrom the root barrel is a breaking change for any consumer that imported them from the package root (@tangle-network/agent-eval), and there is no subpath left to import them from since src/driver.ts is deleted. This is intentional and documented (CHANGELOG: gtm-agent was the only caller and now owns the role; the primitives it needs stay public), so it is not a defect in this shot — flagged only so the global verifier confirms this lands in a breaking-change release (package.json is bumped to 0.146.0) rather than a patch.
🟡 LOW Breaking type removal verified by grep, not compiler — src/types.ts
feedbackPatterns, FeedbackPattern, and PersonaConfig.driverModel are removed as a breaking (^0.146.0) change. Static grep confirms no in-package reader remains, but
pnpm typecheckcould not be executed in this worktree (node_modules missing), so the absence of dangling references is unverified by tsc. Downstream consumers that still assign these optional fields would fail strict-literal excess-property checks after upgrade. CHANGELOG documents the removal and the migration path, which is adequate. Fix: none required for merge; recommend the CI typecheck on the real artifact as the authoritative check.
🟡 LOW Build not re-verified in sandbox; external consumers not greppable — src/types.ts
Removal of PersonaConfig.driverModel is a breaking change for any external consumer that still sets the field. Verified safe in-repo (tsc --noEmit clean; grep finds no in-repo setter), but the CHANGELOG's claim 'nothing reads either, in any repository that depends on it' is not verifiable from this tree. The commit is correctly marked breaking (!) with a migration note, so downstream type errors will be explicit. No fix required.
tangletools · 2026-08-16T21:23:42Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 7 non-blocking findings — 91152ee9
Full multi-shot audit completed 8/8 planned shots over 11 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 11 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-16T21:23:42Z · immutable trace
Blocked: the caller table misses three consumersDo not merge yet. Method:
In each of the three the symbol sits inside an import block that closes with Why the table read 1The stated method was a grep over "nine sibling repositories". These three were not in that set. The other exclusions in the table are correct and I reproduced them: Why it is not yet an outageThe three pin older releases, so nothing breaks today:
Each one breaks at the first bump to the release that carries this deletion. That is the failure this PR's own sequencing rule exists to prevent, and it is why gtm-agent#901 landed first. What unblocks itThree migrations on the gtm-agent#901 pattern — vendor the role into the consuming repository, where the prompt is product data that repository can change and measure — each with its own PR and its own verdict, merged before this one:
The zero-caller findings for
No repository imports any of the four as a value. Splitting the deletion — those four now, Version
|
0.146.0 is already published, so the persona-driver removal takes the next minor. Version locked across package.json, clients/python/pyproject.toml, clients/python/src/agent_eval_rpc/__init__.py and clients/python/uv.lock. The analyst benchmark dependency-lock digest covers package.json and uv.lock, so it moves with the version. The CHANGELOG now records all five first-party callers that owned this role, not one, and tells a caller pinned below 0.145.22 to check whether its pinned version called assertServedModel before porting.
…driver-20260816 # Conflicts: # CHANGELOG.md # clients/python/pyproject.toml # clients/python/src/agent_eval_rpc/__init__.py # clients/python/uv.lock # package.json # src/analyst/benchmark-implementation.ts
This removal was planned against a caller table that named one caller and missed four. The list of repositories was written by hand, and a repository absent from a hand-written list returns zero matches — indistinguishable from a repository that has no callers. The maintainer skill now carries the mechanical procedure: select repositories by structure, grep the default branch ref, resolve that ref per repository, match on word boundaries, union the registry search with repo-derived package names, grep published tarballs, and report what the sweep could not cover.
|
@tangletools review now |
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 2bc04826
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-16T22:28:28Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 2bc04826
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-16T22:28:30Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Auto-approved drewstone PR — 2bc04826
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-16T22:29:08Z
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 2 (2 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 801.0s (2 bridge agents) |
| Total | 801.0s |
💰 Value — sound
Completes the staged removal of the deprecated persona-driver surface (decideNextUserTurn, buildDriverSystemPrompt, ConvergenceTracker, dead PersonaConfig fields) whose replacement — the multishot engine with an injectable driver-prompt seam — already exists in this repo; a clean, proportionate dele
- What it does: Deletes src/driver.ts (decideNextUserTurn, buildDriverSystemPrompt, DecideNextUserTurnOpts), src/convergence.ts (ConvergenceTracker), the examples/user-simulation-driver example, and the PersonaConfig.feedbackPatterns/driverModel fields plus the FeedbackPattern type (src/types.ts), with their tests (src/driver.test.ts, tests/convergence.test.ts); trims deprecation.test.ts to the still-live Product
- Goals it achieves: Retires a surface that its own JSDoc had already condemned ('A role expressed as a code function can never be optimized' — deleted src/driver.ts, tracked by #618): a packaged LLM persona-prompt is product data that belongs in each caller's repository where it can be changed and measured, not a substrate export. It also removes code that was already dead in-package — ConvergenceTracker's only calle
- Assessment: Good on its merits. The removal is complete: pnpm typecheck passes clean after install, and git grep at HEAD shows zero remaining references to the six removed symbols — the surviving driverModel hits are the multishot engine's own independent option (src/multishot/multishot.ts:48). The CHANGELOG documents a real migration path naming still-reachable primitives (CostLedger.runPaidCall, maximumChar
- Better / existing approach: none — this is the right approach. The better architecture already exists in-repo and this change is what retires the redundant old one rather than reinventing anything: src/multishot/multishot.ts:289 calls opts.shape.buildDriverSystemPrompt, an injectable prompt seam typed at src/multishot/types.ts:105, with driverModel/driverFallbackModels/driverTransport config and golden-record regression free
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
A clean, verified removal of the deprecated persona-driver surface whose callers demonstrably migrated, with the replacement (multishot shape seam) established and green in-repo.
- Integration: Removal is complete and self-consistent: no file imports './driver' or './convergence' anymore (rg over src/tests/examples returns nothing), the barrel drops decideNextUserTurn/DecideNextUserTurnOpts (src/index.ts diff), and tests/consumer-contract.test.ts passes pinning the new export surface. The in-repo successor — multishot's pluggable driver loop with its own buildDriverSystemPrompt seam (src
- Fit with existing patterns: Fits the repo's layering doctrine exactly: product-coupled surface moves up to consumers, substrate primitives stay. The deprecation runway already shipped in 0.146.x (warn-once notices pinned by src/deprecation.test.ts, now narrowed to ProductClient), issue #618 tracked removal, and the 0.147.0 minor bump is correct semver for a 0.x breaking change. The caller census in the PR body is mechanicall
- Real-world viability: Verified: pnpm typecheck clean; the repinned dependency-lock digest (5b3a34ca…, src/analyst/benchmark-implementation.ts:14) matches real sources — the prebuild script printed 'public analyst benchmark digests valid … dependency lock 5b3a34ca… (4 files)' and benchmark-implementation.test.ts passed when the sandbox allowed process spawn. Full-suite test/build failures observed here (Go 'newosproc' i
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 PersonaConfig/DriverState are retained with zero in-package readers [maintenance] ``
After this PR the only in-src mention of PersonaConfig outside its definition is a comment (src/rl/sim-fidelity.ts:5); nothing in the package constructs or consumes either type (src/types.ts:165,195, exported at src/index.ts:58,61). The CHANGELOG consciously keeps them as vocabulary for harnesses that write their own driver, which is defensible, but they are now consumer-only types living in the substrate — a candidate for the same mechanical caller-set derivation and removal once external drive
🎯 Usefulness Audit
🟡 Retained persona fields carry docstrings that narrate the deleted driver [problem-fit] ``
PersonaConfig.rigor/expertise/pressurePoints/curveballs and PersonaRigor (src/types.ts:163-193) now have zero in-package readers — src/driver.ts was the only one — yet their docstrings still describe the removed behavior ('The driver LLM scales its tone and follow-up aggression', 'quoted into the driver prompt', section header '── Agent Driver ──'). This is the exact 'field that advertises behaviour the package no longer has' the CHANGELOG cites for deleting feedbackPatterns/driverModel; keeping
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 76 | 77 | 64 | 64 |
| Confidence | 95 | 95 | 95 | 95 |
| Correctness | 76 | 77 | 64 | 64 |
| Security | 76 | 77 | 64 | 64 |
| Testing | 76 | 77 | 64 | 64 |
| Architecture | 76 | 77 | 64 | 64 |
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 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 12 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Migration note misstates when decideNextUserTurn gained the served-model guard — CHANGELOG.md
The note says 'decideNextUserTurn did not call the guard before the 0.145.x line, so a port that omits it there matches the pinned behaviour exactly.' Wrong boundary: commit 5f0b09c added the assertServedModel call to src/driver.ts and release 797224e ('chore(release): agent-eval 0.144.8 (#572)') first shipped it; at 0.145.21 (452ae6c~1) the call is inside decideNextUserTurn (function at line 323, call at line 376). Impact: every caller pinned in [0.144.8, 0.145.21] (~24 releases) that follows this note omits assertServedModel in its port and silently drops the serve
🟠 MEDIUM Migration note misstates when decideNextUserTurn started calling the served-model guard — CHANGELOG.md
The note reads: 'decideNextUserTurn did not call the guard before the 0.145.x line, so a port that omits it there matches the pinned behaviour exactly.' This is factually wrong. Git history shows the call assertServedModel(model, paid.value.servedModel, { allowUnreported: true, context: 'decideNextUserTurn' }) was added inside decideNextUserTurn in commit 8cdd67f (feat(integrity), 2026-08-09) and was present in the released 0.144.6 and 0.144.7 trees (0624e26, line 376) — both BEFORE the 0.145.x line. A consumer pinned at 0.144.6 or 0.144.7 who follows this note and ports without the guard would silently accept a substituted served model that their pinned version rejected (it th
🟡 LOW Empty $ref from symbolic-ref surfaces as a mid-sweep fatal instead of an explicit skip-and-report — .claude/skills/agent-eval/SKILL.md
ref=$(git -C "$repo" symbolic-ref -q --short refs/remotes/origin/HEAD)uses -q, so a repo without a remote default branch sets ref to empty with no message. I tested the downstream command with an empty ref: git grep fails with 'fatal: unable to resolve revision' — loud, not a silent working-tree fallback, so the fail-loud doctrine holds (this disproves the worse hypothesis). But the prose requirement 'Report every repository the sweep could not cover' has no mechanical step; the snippet relies on an agent noticing a fatal mid-loop, and the fatal does not identify which repo failed. Fix: add a guard in the snippet, e.g.[ -n "$ref" ] || { echo "UNCOVERED: $repo (no origin/HEAD)"; continue; }, so the gap report the section demands is produced by the command rather than by post-hoc inter
🟡 LOW Hand-written single root (~/code) contradicts the section's own 'never hand-write the list' rule — .claude/skills/agent-eval/SKILL.md
The enumeration snippet runs
cd ~/codeonly. Verified on this machine: ~/company also contains 4 git repos with.gitdirectories (devops, gtm, tools, webb) that the sweep never reaches. The section's opening rule is 'Derive the caller set mechanically. Never grep a hand-written list of repositories' because 'a repository missing from a list returns zero matches, which reads exactly like a repository that has no callers' — but the root list is itself hand-written, so a consumer repo living outside ~/code is invisible in exactly the way the section warns against. Impact today is nil (verified: no ~/company repo references @tangle-network/agent-eval in package.json), so low severity. Fix: enumerate every known workspace root (e.g. for root in ~/code ~/company; do ...) or state in the pros
🟡 LOW Registry sweep greps only the latest tarball, contradicting the pinned-version rule — .claude/skills/agent-eval/SKILL.md
Line 110 says to download each package's
latesttarball and grepdist/, but lines 118-119 just above order recording each caller's pinned version becausemainis not always what a consumer resolves. A consumer pinned belowlatestresolves an olderdist/that thelatestgrep never sees. Fix: enumerate the pinned versions a consumer can resolve (e.g., registryversionslist) and grep each, or state that thelatestsweep covers only current consumers.
🟡 LOW Repo-selection loop silently skips worktree-only checkouts — .claude/skills/agent-eval/SKILL.md
[ -d "$d/.git" ]deliberately excludes worktree clones (.gitis a FILE, per line 93). If a consumer repo's only local checkout is a worktree, it never enters the sweep and returns zero matches — indistinguishable from 'no callers', the failure mode the section names on line 88. The dedupe intent is sound, but the doc should note that worktree-only checkouts must be resolved via their primary checkout orgit worktree listto avoid silent zero-match gaps.
🟡 LOW Sweep does not refresh refs before grepping the default branch — .claude/skills/agent-eval/SKILL.md
Lines 98-104 instruct grepping
refs/remotes/origin/HEADbut never fetch. A stale localorigin/mainreturns a silently incomplete caller set — the exact failure this section exists to prevent. The 'report uncovered repos' rule only fires when the ref is absent, not when it is stale. Fix: addgit -C "$repo" fetch origin(orfetch --refs) beforesymbolic-ref, or state that refs must be refreshed first.
🟡 LOW grep treats SYMBOL as regex, not fixed string — .claude/skills/agent-eval/SKILL.md
git grep ... -- "$SYMBOL"interprets $SYMBOL as a basic regex. A TypeScript identifier containing$(e.g.foo$, common in generated code) or.will not match literally, and[could error. The section already stresses mechanical accuracy ('a lookalike identifier cannot match'), so recommend adding-F(fixed string) alongside-w -Iso the symbol matches literally. Minor: doc-only, not executed by the package itself.
🟡 LOW node_modules exclusion pathspec misses top-level and nested directories — .claude/skills/agent-eval/SKILL.md
The example
git -C "$repo" grep -n -w -I -- "$SYMBOL" "$ref" -- ':(exclude)*/node_modules/*'uses pathspec*/node_modules/*. Git pathspecs match with wildmatch WM_PATHNAME, so*does not cross/; this pattern only matchesoneDir/node_modules/oneFileand misses top-levelnode_modules/...and anya/b/node_modules/.... The procedure's whole purpose is eliminating false positives/negatives, so an incomplete exclusion can reintroduce false 'callers' from vendoreddist/copies inside a dependent repo's node_modules. Fix: use two excludes or a depth-agnostic form, e.g.:(exclude,glob)**/node_modules/**is still depth-limited; the robust form is:(exclude)node_modulesplus:(exclude)*/node_modulesrepeated, or prefergit grepwith--exclude-standardplus an explicit `node
🟡 LOW Migration note misstates when decideNextUserTurn gained the served-model guard — CHANGELOG.md
Line 24 asserts 'decideNextUserTurn did not call the guard before the 0.145.x line'. Evidence contradicts this: git show v0.144.7:src/driver.ts:376 contains
assertServedModel(model, paid.value.servedModel, {...})inside decideNextUserTurn (import at line 6, call at line 376), while v0.144.6 has none — the guard was added in 0.144.7 (2026-08-09), before 0.145.0 (2026-08-11). A caller pinned to 0.144.7–0.145.21 who follows the advice 'a port that omits it there m
🟡 LOW Missing '---' separator between 0.147.0 and 0.146.0 sections — CHANGELOG.md
Every other version boundary in this file is separated by a '---' line (e.g. line 9, 43, 60, 71), but the new 0.147.0 section ends at line 24 and '## [0.146.0]' begins at line 25 with no separator. The insertion consumed the '---' that previously sat between Unreleased and 0.146.0. Cosmetic, but breaks the file's own Keep-a-Changelog separator convention. Fix: add '---' (with blank lines) between the 0.147.0 migration block and the 0.146.0 header.
🟡 LOW Missing blank line before the 0.146.0 heading — CHANGELOG.md
The new migration paragraph ends at line 24 and '## [0.146.0]' follows immediately on line 25 with no blank line, unlike every other section boundary in the file (which use a blank separator). Harmless to rendering but inconsistent with the file's established format.
🟡 LOW Missing entry separator before the 0.146.0 heading — CHANGELOG.md
The new 0.147.0 entry runs directly into '## [0.146.0]' with no blank line and no '---' separator, while every other entry boundary in the file is blank line + '---' + blank line (e.g. lines 43-45, 59-61). Renders fine but breaks the file's own convention. Fix: insert '\n---\n' between line 24 and the 0.146.0 heading.
🟡 LOW Duplicated version string can drift from pyproject.toml — clients/python/src/agent_eval_rpc/__init__.py
The PackageNotFoundError fallback hardcodes "0.147.0" as a second source of truth alongside pyproject.toml's version field. The publish workflow (.github/workflows/publish.yml:54) and uv.lock both derive from pyproject.toml, so this string only affects source-tree imports (agent_eval_rpc.version). It is consistent this bump, but nothing in CI verifies it matches pyproject.toml, so a future bump that updates only pyproject.toml will silently ship a stale version in source checkouts. Fix: read version from importlib.metadata only and let PackageNotFoundError raise, or add a CI check that greps the fallback against pyproject.toml. Pre-existing pattern, not introduced as a regression here.
🟡 LOW Version literal duplicated across pyproject and init fallback — clients/python/src/agent_eval_rpc/__init__.py
The PackageNotFoundError fallback hardcodes version = "0.147.0", duplicating the single source of truth in pyproject.toml (line 7). When installed as a wheel this branch is never hit (importlib.metadata resolves), so the hardcoded value only serves source-checkout installs and silently drifts if a future release bumps pyproject.toml but forgets this line. Pre-existing pattern, correctly synchronized in this PR. Optional improvement: derive the fallback from a build-time constant or drop it in favor of raising when metadata is absent.
🟡 LOW External consumers of removed root exports cannot be proven migrated — src/index.ts
Removing
decideNextUserTurnandDecideNextUserTurnOptsfrom the root barrel is a breaking public-API change for any consumer importing them from '@tangle-network/agent-eval' at the root specifier. Head commit 2bc0482 states the pre-removal caller table 'named one caller and missed four,' i.e. the initial sweep was wrong by 4 of 5, so the asserted first-party migration set (gtm-agent, legal-agent, insurance-agent, workcomp-agent, creative-agent, per CHANGELOG.md) has a documented history of being incomplete. Mitigations present: both symbols were @deprecated with removal tracked by #618 and warnDeprecatedOnce on call, the 0.147.0 bump is the correct 0.x breaking bump, and the changelog names replacement primitives (CostLedger.runPaidCall, maximumChargeForLlmRequest, costReceiptFromLlm,
🟡 LOW Root re-export removal is a breaking API change — src/index.ts
Removing
export { decideNextUserTurn }andexport type { DecideNextUserTurnOpts }from the root barrel is a breaking change for any consumer importing them from '@tangle-network/agent-eval'. This is intended and coordinated: the module src/driver.ts is deleted in the same PR, the version bumps 0.146.0 -> 0.147.0, and CHANGELOG.md documents the removal plus a migration path (CostLedger.runPaidCall, maximumChargeForLlmRequest, assertServedModel). No action required for this shot; flagged only so the global verifier confirms the version bump and changelog are part of this PR (they are).
🟡 LOW External-consumer claim for removed fields is unverified inside this repo — src/types.ts
CHANGELOG states 'nothing reads either, in this package or in any repository that depends on it.' The in-package half is proven (grep + clean tsc). The external half cannot be proven from this repo — a downstream consumer still setting PersonaConfig.feedbackPatterns would fail to compile only at its own build, after this breaking 0.147.0 lands. Fix: mention the removal in the PR description so consumer repos (gtm-agent per commit 8738f4b) are notified; the breaking-change marker and release bump are otherwise already correct.
tangletools · 2026-08-16T23:56:47Z · trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
✅ Approved — 18 non-blocking findings — 2bc04826
Full multi-shot audit completed 8/8 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 12 changed files. Global verifier still owns final merge decision.
Full immutable report for this review: trace
Summary comment for this run: full summary
tangletools · 2026-08-16T23:56:47Z · immutable trace
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟡 Value Audit — sound-with-nits
| Verdict | sound-with-nits |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 2 (2 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 265.1s (2 bridge agents) |
| Total | 265.1s |
💰 Value — sound
Executes an already-announced deprecation (#618) by deleting the persona-driver-as-code surface, its dead ConvergenceTracker companion, and readerless type fields — coherent with the package's own doctrine and with the multishot engine already providing the retained way to drive simulated users; shi
- What it does: Deletes src/driver.ts (decideNextUserTurn, buildDriverSystemPrompt — both already @deprecated on main, removal tracked by #618, verified via
git show origin/main:src/driver.ts), src/convergence.ts (ConvergenceTracker), the user-simulation-driver example, their tests, and the PersonaConfig.feedbackPatterns/driverModel fields whose only reader was AgentDriver (deleted in 0.145.22). Mechanical comp - Goals it achieves: (1) Stop shipping a persona expressed as a packaged code function — the package's own doctrine (src/worker-driver-seed.ts:3-8: 'A role expressed as a code function can never improve; a role expressed as versionable prompt data can') makes the old surface a living contradiction; (2) delete dead code — ConvergenceTracker had zero in-package callers after AgentDriver's deletion and never reached the
- Assessment: Good on its merits and with the codebase's grain. The capability the removed driver provided — an LLM persona interrogating an agent over turns — is retained in-package by runMultishot (src/multishot/multishot.ts:1,244-256), which already embodies the target architecture: the driver system prompt is injected data via MultishotShape.buildDriverSystemPrompt (src/multishot/types.ts:105), not a packag
- Better / existing approach: none — this is the right approach. Alternatives examined and rejected on evidence: (a) also deleting PersonaConfig/DriverState — wrong, the CHANGELOG documents they stay for harness-owned drivers and the audit found downstream users; (b) keeping driver.ts deprecated longer — pointless, callers have migrated and the surface contradicts worker-driver-seed.ts doctrine; (c) packaging a new persona-dri
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound-with-nits
A verified dead-surface removal: the persona-driver function exports, their only orphaned helper (ConvergenceTracker), and two now-inert PersonaConfig fields are deleted after all first-party callers moved to exact-pinned older versions, with in-flight inline branches covering the three repos that s
- Integration: Fully integrated as a removal. No dangling references remain: repo-wide grep for 'from ./driver' / 'from ./convergence' / user-simulation-driver returns only CHANGELOG history lines; pnpm typecheck is clean. The multishot hits on buildDriverSystemPrompt/driverModel (src/multishot/types.ts:105, src/multishot/multishot.ts:122) are that engine's own local options, not the deleted exports. Retained ty
- Fit with existing patterns: Fits the codebase's grain precisely. The substrate doctrine (CLAUDE.md layering rule) says product-shaped behavior belongs in consumers; the CHANGELOG's rationale (a persona driver is an AgentProfile on a graph edge, not a packaged function) matches the established pattern already used by the multishot engine, which takes buildDriverSystemPrompt as a caller-supplied shape callback (src/multishot/t
- Real-world viability: Holds up. No at-rest consumer breaks: every dependent repo pins an exact pre-removal version (0.145.3 for legal/insurance/creative, 0.145.22 peer for gtm), so publishing 0.147.0 changes nothing until a consumer deliberately lifts the pin. The lock-digest repin is correct — the repo's own validator prints 'public analyst benchmark digests valid ... dependency lock 5b3a34ca... (4 files)' matching sr
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 1
💰 Value Audit
🟡 Retained persona types still narrate the deleted in-package driver [maintenance] ``
src/types.ts:147 keeps the section header '// ── Agent Driver ──' naming the class deleted two releases ago, and the PersonaRigor/PersonaConfig doc comments (src/types.ts:155-193) still say 'the driver LLM scales its tone' — describing machinery this package no longer ships rather than the harness-owned driver that now reads these fields. Per the repo's comment discipline (comments describe what the code does, not history), these comments should be re-anchored to 'the harness's driver LLM' in a
🎯 Usefulness Audit
🟡 Deletion publishes ahead of three consumer migrations; CHANGELOG states them as done [integration] ``
CHANGELOG 0.147.0 says 'All five first-party callers now own the role in their own repositories', but on their default branches legal-agent (tests/eval/canonical.ts:120, import block resolving to '@tangle-network/agent-eval' at line 136), insurance-agent (tests/eval/canonical.ts:112, block ends line 134), and creative-agent (eval/e2e/creative-product-harness.ts:5,611) still import decideNextUserTurn from this package. The migrations sit on unmerged 'origin/chore/inline-persona-driver' branches (
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.
tangletools
left a comment
tangletools
left a comment
There was a problem hiding this comment.
🟢 Value Audit — sound
| Verdict | sound |
| Coverage | 2 of 2 lenses (value, usefulness) |
| Concerns | 2 (2 weak-concern) |
| Heuristic | 0.0s |
| Duplication | 0.0s |
| Interrogation | 1479.1s (2 bridge agents) |
| Total | 1479.1s |
💰 Value — sound
Retires the deprecated persona-driver surface (decideNextUserTurn, buildDriverSystemPrompt, ConvergenceTracker, dead PersonaConfig fields) after migrating all mechanically-derived callers, leaving the in-repo multishot engine and product-owned prompts as the architecture — a clean, in-grain completi
- What it does: Deletes src/driver.ts (decideNextUserTurn, DecideNextUserTurnOpts, buildDriverSystemPrompt — already @deprecated on main citing #618), src/convergence.ts (ConvergenceTracker, never barrel-exported, sole caller AgentDriver deleted in 0.145.22), the user-simulation-driver example, and the PersonaConfig.feedbackPatterns/driverModel/FeedbackPattern fields nothing reads anymore (~878 lines). Bumps to 0
- Goals it achieves: Moves the simulated-user role out of the substrate into the products that use it, where the prompt is measurable product data ('a role written as a code function cannot be optimized'). This is the substrate-layering doctrine applied: agent-eval keeps only the primitives a caller needs (CostLedger.runPaidCall at src/cost-ledger.ts:332, maximumChargeForLlmRequest and costReceiptFromLlm at src/index.
- Assessment: Good on its merits. (1) The removal cycle is proper: symbols were deprecated with loud once-per-process warnings on main (src/driver.ts @deprecated citing #618; CHANGELOG documents it), callers migrated first, then deletion — verified complete: rg finds zero references to the removed symbols outside historical CHANGELOG entries, and tsc --noEmit passes clean. (2) It is the grain of this codebase:
- Better / existing approach: none — this is the right approach. Searched for an existing equivalent before answering: the codebase already contains the better architecture this change defers to — src/multishot/multishot.ts:289 drives multi-turn simulated-user conversations with a caller-supplied shape.buildDriverSystemPrompt callback (src/multishot/types.ts:105) and per-shot pricing, and golden records freeze that loop's beha
- Model: opencode/zai-coding-plan/glm-5.2
- Bridge attempts: 2
- Bridge warning: opencode/kimi-for-coding/k2p7: opencode: opencode error
🎯 Usefulness — sound
A clean, mechanically-verified deletion of the dead persona-driver surface whose replacement (multishot engine + registry-backed prompt data) is already in place and actively developed.
- Integration: This is a removal, not an addition. Every deleted symbol — decideNextUserTurn, DecideNextUserTurnOpts, buildDriverSystemPrompt (src/driver.ts), ConvergenceTracker (src/convergence.ts), the user-simulation-driver example — has zero remaining references in the repo (rg across src/examples/clients returns only the new, unrelated driverModel option inside src/multishot/multishot.ts:48). Typecheck pass
- Fit with existing patterns: The removal is exactly the grain this repo already committed to. The deleted functions carried their own deprecation messages pointing at the same destination — 'roles belong in registry-backed prompt data, not code' and 'a role expressed as a code function can never be optimized' (src/driver.ts, before deletion). The multishot engine (runMultishot, MultishotShape, shape-defaults.ts) is that repla
- Real-world viability: Deletion carries no runtime behavior, so concurrency/error-path concerns do not apply to removed code. The one real-world edge — external dependents pinned below 0.145.22 that cannot import assertServedModel — is handled: the CHANGELOG's Migration section names the exact building blocks (CostLedger.runPaidCall, maximumChargeForLlmRequest, costReceiptFromLlm) and warns that a port below 0.145.22 ma
- Model: opencode/deepseek/deepseek-v4-pro
- Bridge attempts: 3
- Bridge warning: opencode/zai-coding-plan/glm-5.2: bridge stream ended without value-audit content; opencode/kimi-for-coding/k2p7: opencode: opencode error
💰 Value Audit
🟡 Same-named buildDriverSystemPrompt/driverModel survive in the multishot namespace with a different contract [maintenance] ``
src/multishot/types.ts:105 (MultishotShape.buildDriverSystemPrompt — a caller-supplied callback with a permissive default at src/multishot/shape-defaults.ts:72) and src/multishot/multishot.ts:48 (driverModel option) keep the exact identifiers just removed from the barrel. A consumer porting off the removed API could grep, find these, and assume the packaged adversarial prompt still exists somewhere. Not a duplicate capability — it is the intended caller-owned replacement — but a one-line CHANGEL
🎯 Usefulness Audit
🟡 PersonaConfig/DriverState/CompletionCriterion/PersonaRigor remain exported with zero in-package consumers [problem-fit] ``
src/types.ts:147-202 still defines CompletionCriterion, PersonaRigor, PersonaConfig, and DriverState under a now-stale '── Agent Driver ──' header, and src/index.ts:57-61 exports them. rg confirms the only remaining reference outside definition/export is a prose comment in src/rl/sim-fidelity.ts:5. The CHANGELOG justifies this ('a harness that writes its own driver still describes a persona and a produced state with them'), which is defensible public-substrate API — but the caller-set table in t
What this audit checks
It judges the change on its merits — not whether it was tasked out in an issue. Unticketed, fast-moving work is fine; the question is whether the change is good and whether a better or existing approach should be used instead.
| Pass | What it asks |
|---|---|
| Heuristic | Vague title? Whitespace-only or cruft-bearing diff? (content signals only) |
| Duplication | Do added function/class names already exist elsewhere in the repo? |
| Value Audit | What does it do? What goal does it achieve? Is it good? Better architecture or already-exists? |
| Usefulness Audit | Does it integrate and fit? Will it hold up in real use and actually get used? |
Findings are concerns, not blocks — the human reviewer decides what to do with them.
✅ No Blockers —
|
| opencode GLM 5.2 | opencode DeepSeek v4 Pro | opencode DeepSeek v4 Flash | aggregate | |
|---|---|---|---|---|
| Readiness | 51 | 89 | 60 | 51 |
| Confidence | 95 | 95 | 95 | 95 |
| Correctness | 51 | 89 | 60 | 51 |
| Security | 51 | 89 | 60 | 51 |
| Testing | 51 | 89 | 60 | 51 |
| Architecture | 51 | 89 | 60 | 51 |
Reviewer score is advisory once the run is complete and the verdict has no blockers.
Full multi-shot audit completed 8/8 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 12 changed files. Global verifier still owns final merge decision. | Full multi-shot audit completed 8/8 planned shots over 12 changed files. Global verifier still owns final merge decision.
🟠 MEDIUM Default-branch resolver fails on 47% of the sweep set, including agent-eval and agent-runtime — .claude/skills/agent-eval/SKILL.md
The prescribed resolver 'ref=$(git -C "$repo" symbolic-ref -q --short refs/remotes/origin/HEAD)' fails (empty output, exit 1) on 50 of 106 canonical repos under ~/code. In ~/code/agent-eval — the repo this skill maintains — refs/remotes/origin/HEAD does not exist (rev-parse: unknown revision); ~/code/agent-runtime fails the same way. These are the primary caller repos for a symbol-removal sweep. Line 106 forbids assuming main ('Do not assume the default branch is main; resolve it per repository'), so an agent hits a contradiction: the only sanctioned resolver returns nothing on the most important repos, degrading the mechanical sweep to reported-gap status e
🟠 MEDIUM Empty default-branch ref aborts the whole sweep instead of being reported — .claude/skills/agent-eval/SKILL.md
ref=$(git -C "$repo" symbolic-ref -q --short refs/remotes/origin/HEAD)produces an empty string when the remote has no origin/HEAD (verified: ~/code/agent-eval has no refs/remotes/origin/HEAD, andgit grep ... "$SYMBOL" "" -- ...then exits withfatal: unable to resolve revision:). In a multi-repo sweep the unresolved"$ref"kills the grep and, underset -eor a stop-on-error loop, silently ends coverage of every repo after it. The section's own prose ('Report every repository the sweep could not cover... Silence about a gap is the defect') demands the opposite, but the snippet has no guard. Fix: guard the ref before grepping, e.g.[ -n "$ref" ] || { echo "no remote default branch: $repo"; continue; }, and accumulate uncovered repos into the report.
🟠 MEDIUM Migration note's served-model-guard history is factually wrong — CHANGELOG.md
Line 24 states: 'A caller pinned below 0.145.22 cannot import assertServedModel' and 'decideNextUserTurn did not call the guard before the 0.145.x line, so a port that omits it there matches the pinned behaviour exactly.' Both sub-claims are contradicted by the released tags: (1) src/index.ts exported assertServedModel/assertServedModels from the barrel in v0.144.6 through v0.144.12 (removed only at v0.144.13 by commit 94c3ced, re-added at v0.145.22), so pinned callers in that window COULD import it; (2) the guard call
assertServedModel(model, paid.value.servedModel, { allowUnreported: true, context: 'decideNextUserTurn' })was already inside decideNextUserTurn from v0.144.7 on
🟠 MEDIUM Migration section falsely claims decideNextUserTurn never called the served-model guard before 0.145.x — CHANGELOG.md
Line 24 states: 'decideNextUserTurn did not call the guard before the 0.145.x line, so a port that omits it there matches the pinned behaviour exactly.' Git history contradicts this:
git log -S assertServedModel -- src/driver.tsshows the call was added in commit 5f0b09c (PR #571), which shipped in 0.144.8 (release commit 797224e; the CHANGELOG's own '0.144.8 - Duplicate candidate admission' entry describes that PR). The base driver.ts (lines 177-180) calls `assertServedModel(model, paid.value.servedModel, { allowUnreported: true, context: 'decideNextUserTurn' }
🟡 LOW $SYMBOL is interpolated as a basic regex, not a fixed string — .claude/skills/agent-eval/SKILL.md
git grepinterprets the pattern as a BRE, so a public symbol containing regex metacharacters (e.g.Foo.bar,Driver$,type_*) matches lookalikes (.=any char,*=repetition).-wnarrows boundaries but does not fix this; a.inside the symbol still over-matches. Since the doc then tells the reader to read every hit'sfromclause, the over-match is filtered by a human, but the sweep is not exact. Fix: add-F/--fixed-strings(git grep -F -n -w -I) so the symbol is matched literally; verified-Fworks on a tree ref.
🟡 LOW Empty-ref capture has no guard between resolution and grep — .claude/skills/agent-eval/SKILL.md
'ref=$(...)' discards the exit status, and the grep snippet at line 103 is shown per-repo with no '[ -n "$ref" ]' check. Given the 47% resolver failure rate above, an agent looping over ~106 repos sees one 'fatal: unable to resolve revision' line per failing repo mixed into otherwise-clean output; the failure is loud per-command but easy to miss in aggregate. The prose at line 115 ('Report every repository the sweep could not cover') is the only mitigation and is behavioral, not mechanical. Fix: add '[ -n "$ref" ] || { echo "U
🟡 LOW Structure-select loop silently drops worktree-only checkouts — .claude/skills/agent-eval/SKILL.md
[ -d "$d/.git" ]admits only canonical checkouts; a worktree's.gitis a file (as in this very review worktree), so a repository whose only local checkout is a worktree disappears from the sweep with no gap reported (the selection loop cannot enumerate what it excludes). Impact is bounded — the same repo's canonical checkout elsewhere is still picked up, and published dependents are covered by the registry-tarball union — but a maintainer could conclude a caller set is empty for a repo that exists only as a worktree. Fix: also accept[ -f "$d/.git" ]and resolve the worktree's main worktree viagit -C "$d" rev-parse --git-path common-dir, or explicitly document the exclusion as a reported gap.
🟡 LOW git grep -w is still regex, not a literal match — .claude/skills/agent-eval/SKILL.md
Line 107 states 'Use
git grep -wso a lookalike identifier cannot match', but the snippet at line 103 (git -C "$repo" grep -n -w -I -- "$SYMBOL" "$ref" ...) passes$SYMBOLas a basic regex.-wonly adds word-boundary anchoring; it does not escape metacharacters. A symbol containing.,[,*, or((e.g. a namespacedfoo.baror a method with a bracket pattern) would match unintended text, contradicting the stated goal. Fix: use-F -w(fixed-string + word-boundary) so$SYMBOLis matched literally. Non-blockin
🟡 LOW 0.147.0 entry omits the analyst dependency-lock repin shipped in this release — CHANGELOG.md
This release includes commit 6cdae0c, which changes ANALYST_BENCHMARK_DEPENDENCY_LOCK_SHA256 in src/analyst/benchmark-implementation.ts, yet the 0.147.0 entry has no Changed bullet for it. Precedent in the same file counts this as notable: the 0.145.20 entry (line 96) says 'Refreshed the analyst benchmark dependency-lock hash for this version.' A reader auditing benchmark-evidence provenance cannot tell from the changelog that the lock digest moved in 0.147.0. Fix: add a '### Changed' bullet noting the refreshed analyst benchmark dependency-lock digest.
🟡 LOW Missing '---' separator and blank line between the 0.147.0 entry and the 0.146.0 heading — CHANGELOG.md
The 0.147.0 Migration paragraph (line 24) runs directly into '## [0.146.0] — 2026-08-16' (line 25) with no blank line and no '---'. Every other entry boundary in this file uses blank-line + '---' + blank-line (lines 43, 60, 71, 98, 121, ...). CommonMark still parses the ATX heading, so rendering is unaffected, but plain-text readers and the next entry insertion will treat the boundary inconsistently with the file's structure. Fix: insert '\n\n---\n' between [line
🟡 LOW Missing --- separator before [0.146.0] heading — CHANGELOG.md
Every version section in this file is delimited by a
---horizontal rule (see lines 9, 43, 60, 71, ...). The new 0.147.0 Migration section ends at line 24 and## [0.146.0] — 2026-08-16(line 25) follows immediately with no blank line and no---. This breaks the file's own Keep-a-Changelog-style boundary convention and can flatten the heading in strict parsers. Fix: insert a blank line plus---between [line 24](https://github.com/tangle-network/agent-eval/
🟡 LOW Hardcoded version fallback duplicates pyproject version — clients/python/src/agent_eval_rpc/__init__.py
version falls back to a hardcoded '0.147.0' when importlib.metadata can't find the installed dist (running from source). It must match pyproject.toml:7 by hand; a future bump that edits only one site silently reports the wrong version. Both are consistent here and this is the repo's standing release pattern, so informational only. Fix (optional): read a single source of truth, e.g.
importlib.metadata.versiononly, or generate from package metadata at build time.
🟡 LOW Release version is hand-synced in three places — clients/python/src/agent_eval_rpc/__init__.py
The fallback
__version__ = "0.147.0"duplicates the pyproject.toml version and must be edited in lockstep every release (this PR edits 3 files for one number). It is consistent in this release, so no action required here, but a future release that bumps pyproject without the fallback will report the wrong version for non-installed (source-tree) runs. Consider deriving the fallback from package metadata at build time or adding a release-check assertion.
🟡 LOW Digest checker test flakes under thread-constrained environments — src/analyst/benchmark-implementation.ts
Not a defect in the changed line itself: the test that guards this constant (benchmark-implementation.test.ts 'fails when any bound source changes without a new digest') intermittently fails when run alongside other files because each spawnSync of the checker launches an esbuild service that can exceed the OS thread limit (observed 'runtime: failed to create new OS thread (have 16 already; errno=11)'). Pre-existing and environmental; the digest-validating tests themselves pass deterministically. No action required for this PR; a pool/serial option for the checker subprocess would harden CI on constrained runners.
🟡 LOW Digest repinned twice: first pin was stale against merged sources — src/analyst/benchmark-implementation.ts
Commit cef6e3f pinned 3678c017... at version-bump time; commit 6cdae0c then re-pinned to 5b3a34ca... because merging origin/main added 5 lines to package.json. The head value is correct (checker and tests pass), but the sequence shows the 'repin, then merge' order can ship a digest that momentarily misdescribes the lock files in the intermediate commit. Since the checker runs in CI/verify:package, the stale pin would have been caught on release; this is a process note, not a shipped defect.
🟡 LOW Dead persona types remain exported from the public root — src/index.ts
index.ts:55-65 still exports DriverState (line 58), PersonaConfig (line 61), and CompletionCriterion from './types', but the PR deleted their only in-repo consumer chain: src/driver.ts. discover-personas defines its own DiscoveredPersona shape and does not import PersonaConfig; the only other mention is a comment in src/rl/sim-fidelity.ts:5. These are now public API with no in-repo user. If keeping them is deliberate (to bound the 0.147.0 breaking surface for external consumers that read personas), say so in the CHANGELOG; otherwise prune them in the next breaking pass so
🟡 LOW External-caller sweep for the removed root exports is unverifiable from this repo — src/index.ts
Removing
decideNextUserTurn/DecideNextUserTurnOptsfrom the root barrel breaks any consumer importing them from the package root. In-repo evidence is clean (typecheck passes, no internal references, no./driversubpath export). The residual risk is external: commit 2bc0482's message states the removal 'was planned against a caller table that named one caller and missed four', and the CHANGELOG entry (this PR) claims all five first-party callers (gtm-agent, legal-agent, insurance-agent, workcomp-agent, creative-agent) migrated into their own repos with gtm-agent'seval/lib/persona-driver.tsas the worked example. That claim cannot be confirmed from within this repository — a downstream still pinned todecideNextUserTurnfrom the root will fail to resolve at import/link time after
🟡 LOW No regression test pins the removed public API — src/index.ts
The repo has an established pattern for pinning symbol removals: src/worker-driver-seed.test.ts:66-67 asserts
expect(api).not.toHaveProperty('buildWorkerDriverSystemPrompt')for the analogous earlier removal. Nothing equivalent guards this removal — e.g. a test asserting the root index no longer hasdecideNextUserTurnand thatimport('./driver')rejects. Without it, a future re-export or partial revert of src/driver.ts would silently re-publish the deprecated surface. Fix: add a two-line guard test next to deprecation.test.ts mirroring the worker-driver-seed pattern.
🟡 LOW Docblock header has pre-existing inaccuracies left untouched by a doc-hygiene diff — src/series-convergence.ts
The header the diff edited still says 'Three signals' and describes stabilized as 'low variance (< epsilon)', but the code (lines 22, 36, 48-49) uses a coefficient-of-variation threshold named stableCv (default 0.05), the state union has 5 members (drifting splits into drifting-up/drifting-down, plus insufficient-data at line 40), and 'drifting: ... beyond noise' (line 8) is not implemented — drift is a strict-monotone tail r
tangletools · 2026-08-17T00:33:15Z · trace
Post-migration census — zero first-party callers of
|
| repo | default branch | migration | value refs | doc mentions |
|---|---|---|---|---|
| legal-agent | main |
#366 merged b31cd2cb |
0 | 0 |
| insurance-agent | main |
#97 merged ad381e4b |
0 | 0 |
| workcomp-agent | master |
#28 merged 4cee60bc |
0 | 0 |
| creative-agent | master |
#496 merged 895ff9c5 |
0 | 0 |
creative-agent was the fourth caller — it held a live call at eval/e2e/creative-product-harness.ts:611 when I started, and its migration landed independently at 05:06Z. Every repo that had a hit before now has none.
Uncoverable, unchanged from this PR's sweep: persona-labs-sdk, tinder-for-anything (no remote default branch).
Published packages. @tangle-network/agent-eval@0.147.0 carries decideNextUserTurn in CHANGELOG.md only — dist/index.js and dist/index.d.ts have zero occurrences, so the deletion is already live on npm. Swept the rest of the fleet at current latest by tarball: agent-runtime@0.140.0, agent-knowledge@8.0.8, agent-app@0.45.62, agent-interface@1.0.1, sandbox@0.27.1, sandbox-ui@0.104.1 — 0 occurrences each.
The migrated repos survive this deletion, checked symbol by symbol
Each local driver imports only surface that still exists on main after this PR: PersonaConfig, DriverState, CompletionCriterion, CostLedger, CostLedgerHandle, costReceiptFromLlm, costReceiptFromLlmError, maximumChargeForLlmRequest, and — in legal and insurance — assertServedModel.
PersonaRigor is the one that would have bitten: it is no longer re-exported from the barrel. All three derive it structurally as NonNullable<PersonaConfig['rigor']> rather than importing it, and PersonaConfig.rigor survives (src/types.ts:172), so the barrel change is not a break.
Version state today: legal and insurance pin 0.145.22, workcomp pins 0.143.0. All three still resolve a version that ships the function, so nothing was running on borrowed time — the migrations are what make the next bump past 0.147.0 safe.
Prompt parity, measured
The load-bearing claim in all three PRs is that an eval run does not move. Verified rather than asserted: a harness called decideNextUserTurn (at the version each repo pins) and the repo's new decidePersonaTurn over 18 combinations of persona × history × product-context, and compared the dispatched request. System prompt, user message, model, temperature and maxTokens were identical in all 18 cases per repo.
workcomp's local copy deliberately omits the assertServedModel guard. That is correct at its pin: 0.143.0 does not export it and its decideNextUserTurn did not call it. legal and insurance keep the guard because 0.145.22 has it.
One inherited gap worth naming
Every reviewer found it independently, and it is real: no caller passes a costLedger, so each driver turn allocates a throwaway new CostLedger() and the receipt is discarded. That is byte-identical to the substrate function's own default, so the migrations did not introduce it — but the actor rename (decideNextUserTurn → <product>-persona-driver) only pays off once a caller threads a shared ledger through. Cheap follow-up in each product, now that the seam is theirs.
2 participants
Closes #618. The callers have migrated, so the surface goes.
The caller set, re-derived
An earlier revision of this PR claimed
decideNextUserTurnhad one first-party caller. It had five. The method was the defect: it grepped a hand-listed set of nine sibling repositories, and three shipped products were not on that list. A fourth, creative-agent, was missing from every list used during the correction as well.The set below is derived mechanically instead, so a repository cannot be absent by omission:
~/code— 106 of them, selected by.gitbeing a directory, which excludes worktree clones whose.gitis a file. Each is grepped on its own default branch ref (refs/remotes/origin/HEAD, falling back toorigin/main/origin/master), not on whatever branch happens to be checked out: 89 resolved toorigin/main, 12 toorigin/master, 3 toorigin/develop. Two repositories (persona-labs-sdk,tinder-for-anything) have no remote default branch and are reported as uncoverable rather than silently skipped. Matching is word-boundary (git grep -w), which is what keepsHostAgentDriverandRedTeamAgentDriverfrom ever matchingAgentDriver.@tangle-networkpackage at its currentlatest, by tarball — 78 candidate names (registry scope search unioned with every non-privatepackage.jsonname in those 106 repositories, because the registry search index returned only 13 of them). 39 are published; each tarball was downloaded and word-grepped. 11 declare a dependency on this package.Total: 155 raw matches across all six symbols.
Result — all six symbols
AgentDriverAgentDriverConfigbuildWorkerDriverSystemPromptbuildDriverSystemPromptdecideNextUserTurnDecideNextUserTurnOptsThe five callers of
decideNextUserTurnmastereval/kernel.tsmaintests/eval/canonical.ts:120:1387maintests/eval/canonical.ts:112:1093mastereval/driver.ts:24:73mastereval/e2e/creative-product-harness.ts:5:611Every one resolves to
@tangle-network/agent-eval— verified by reading thefromclause of the import block, not by proximity.Excluded, with the reason
projects/playproof/runner.mtsAgentDriverinterface AgentDriver. Only importscreateChatClient/ChatClientfrom this package.src/lib/eval-adversary.tsbuildDriverSystemPromptfunction buildDriverSystemPrompt.buildDriverSystemPromptMultishotShape.buildDriverSystemPromptproperty key — a caller-supplied field that shares the name.dist/eval/index.d.tsAgentDriverdocs/research/, redteam.evolve/progress.mdHostAgentDriver,RedTeamAgentDriver11 published dependents declare this package; none contains any of the six symbols in shipped code.
braid@0.1.3andtraces@0.11.6are dependents the earlier ten-package list did not include.What is deleted
src/driver.tsand its test,examples/user-simulation-driver, the barrel exports ofdecideNextUserTurnandDecideNextUserTurnOpts, and the row that taught the example inexamples/README.md.ConvergenceTrackerandsrc/convergence.tsgo with them — orphaned by 0.145.22, its only caller wasAgentDriver, and it never reached the barrel.analyzeSeriesinsrc/series-convergence.tsis a different thing and is untouched: it reads drift across runs, not progress within one.PersonaConfig.feedbackPatterns,FeedbackPatternandPersonaConfig.driverModelgo too — onlyAgentDriverread them.What stays
PersonaConfigandDriverState. A harness that writes its own driver still describes a persona and a produced state with them. Every primitive the deleted call used is still public:CostLedger.runPaidCall,maximumChargeForLlmRequest,costReceiptFromLlm,costReceiptFromLlmError, andassertServedModel.Version
0.147.0, not 0.146.0. The earlier revision declared 0.146.0, which
mainhas since published — a release at that number would have collided. No lane is cutting 1.0.0 (no branch, no open PR, no issue proposes it), so the removal takes the next minor, which is the breaking slot while this package is below 1.0.One migration note earned by the work: a caller pinned below 0.145.22 cannot import
assertServedModel. Check what the pinned version actually did before porting —decideNextUserTurndid not call the guard before the 0.145.x line, so a port that omits it there matches the pinned behaviour exactly, and a port that adds it silently changes behaviour. workcomp-agent is that case.Verification
pnpm typecheck,pnpm typecheck:examples,pnpm lint— clean.pnpm test— 380 files passed / 2 skipped, 5342 passed / 3 skipped, one clean run, no reruns.pnpm buildandpnpm run verify:package— exit 0.package.json,clients/python/pyproject.toml,clients/python/src/agent_eval_rpc/__init__.py,clients/python/uv.lock; analyst dependency-lock digest repinned against the post-merge sources.origin/mainmerged in (it published 0.146.0 and moved the digest); conflicts in the CHANGELOG, all four version files and the digest resolved, and the full suite rerun after.