Skip to content

fix(improvement): enforce training provenance at held-out boundaries - #1302

Merged
drewstone merged 1 commit into
mainfrom
fix/training-evaluation-provenance
Sep 20, 2026
Merged

drewstone merged 1 commit into
mainfrom
fix/training-evaluation-provenance

Conversation

@drewstone

@drewstone drewstone commented Sep 20, 2026

Copy link
Copy Markdown
Contributor

Status

Open, non-draft; all four required final-head CI checks pass on 896a48cd9b7e1e0899f70c6210779f57b78642b1.

https://github.com/tangle-network/agent-runtime/actions/runs/35481127476

Passed: ci, agent-bench, official-optimizers, and packed-cohort. This is one product commit on merged main 3505d81e7f85683bc25bbd46a6bcbfdaf9503a71. No protection or test was weakened. Review has been requested from tangletools; its current provisional auto-approval is not represented as a completed independent audit. A separate source self-review records the further sealed-bundle compatibility boundary.

Purpose

Follow up on merged #1287/#1300 with two reproduced correctness fixes and a critical, outcome-driven review of the training work. One Runtime PR; no new dependency, exporter, optimizer, scheduler, or deployment client. The implementation changes add 32 net production TypeScript lines; the rest is regression coverage, the requested critique, and release metadata.

Correctness fixes

  • Preserve an explicit training executionRef in harness.train(...). The bound harness reference identifies evaluation, not the trainer/serving implementation. Previously it silently replaced the caller's training identity, and an existing test asserted that wrong behavior. Training now requires its own reference; profile and validator binding remain intact.
  • Enforce known checkpoint-training exposure at Runtime's profile evaluation boundaries. One internal helper checks the current receipt and all retained ancestors against canonical held-out content digests. It covers method baseline/candidate materialization and authored/optimized profile proposals, checking both comparison arms. A contaminated baseline is refused before optimizer construction or analysis. Training-visible validation rows count as exposure. Existing development-task reuse remains allowed.
  • Keep these checks in the existing profile validation/measurement paths. Diagnostic Eval APIs remain unchanged. Exact digest intersection detects declared exposure; it does not prove absence of undeclared pretraining or semantic duplicates. The separate sealed-bundle path currently refuses profile metadata, not silently strips the training receipt; this PR does not claim to add that integration.

Critical review and finish criteria

docs/research/training-purpose-audit-2026-09-19.md records 12 decisions: why each mechanism exists, its present contribution, concrete defects or evidence gaps, what to retain/delete/reuse, and how completion is judged. It separates engineering completion, measured user value, and continual-learning evidence. The architecture now explicitly identifies recursive execution as a substrate rather than the product's success criterion.

The review does not confuse a receipt or mock-serving fixture with live GPU/model quality. It calls out actual consumer integration, serving attestation, native checkpoint formats, cost accounting, remote reconciliation, bounded ancestry, dataset scalability, command diagnostics, and API contract clarity. The existing Blueprint P5 work remains the consumer integration owner; no duplicate campaign is introduced here.

Verification

  • Failure-first control on the merged implementation with the same dependencies: 11 failed, 84 passed across the same five focused files. These reproduce wrong identity and current/ancestral training exposure.
  • Repaired focused suite: 95 passed, including positive controls for legitimate development reuse and task-name reuse with different content.
  • Full local suite: 4,258 passed, six existing skips across 319 passing/two skipped files. Source typecheck and full lint pass.
  • Clean exact-main preparation: https://github.com/tangle-network/agent-runtime/actions/runs/35480972205 — frozen dependency install, source/example types, lint, focused regressions, release checks, packed exports/edge execution, and generated documentation passed. Its write-enabled delivery job handled only verified Git objects.
  • Downloaded preparation evidence and verified archive SHA-256 337fe6c548829b393bd8e0a4ef3f1277100ce10a703cf6908f7470ddf01faf4f, candidate/base SHAs, clean worktree, native logs, and exact source/test/script bytes against the local tested tree. They match. No temporary workflow or patch carrier enters this PR; the preparation workflow was retired after delivery.
  • Normal final-head PR CI above independently passes all four repository gates, including the full suite, types, lint, release checks, packed package exports, edge execution, generated docs, Bench integration, official optimizers, and exact first-party package cohort.

Compatibility and boundaries

Prepare Runtime 0.244.0 and Bench 0.13.6. harness.train(...) callers must explicitly supply the training/serving execution reference; omission no longer silently borrows evaluation identity. Previously accepted profile comparisons with declared held-out contamination now fail. No dependency or benchmark grading change.

This is a source audit with executable regression controls, not an independent third-party audit or a claim that every Tangle repository has been inspected. No merge, npm publication, deployment, or paid training/inference performed.

@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 — 896a48cd

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-09-20T01:20:17Z

@drewstone drewstone left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Source self-review and scope check, not an independent approval:

The two fixed defects are reproduced against the exact merged implementation with the same dependency cohort (11 failures in the new/updated controls; repaired suite 95/95). The prepared candidate's source/test/script bytes were checked against the locally tested files; artifact SHA-256, candidate/base SHAs, clean worktree, and native verification logs match.

A further boundary audit checked the separate sealed candidate-bundle path. candidateProfileFromGenericProfile in src/candidate-execution/profile.ts explicitly rejects metadata, rather than silently stripping a trained profile's receipt. This is an unsupported integration path, not a reason to bypass the current closed candidate contract or claim universal trained-profile support. This PR intentionally covers the existing complete-profile optimization/proposal paths. Any broader execution integration must preserve the portable receipt and demonstrate the real consumer path.

Please critically review: (1) baseline checks happen before optimizer construction/analysis; (2) selected candidates are checked before final measurement; (3) both current and ancestor inventories include validation exposure; (4) canonical scenario/content digests are the same identity carried by the real exporter, without pretending exact hashes detect semantic duplicates; (5) training executionRef is not confused with evaluation executionRef; (6) existing independent development reuse remains valid. Tests include negative controls and positive controls for these distinctions.

The next value-producing integration remains the existing Blueprint P5 work (#2473): real export -> real trainer -> serving evidence -> actual worker -> fresh paired measurement, with complete adaptation costs. A fake serving port, an omitted timeout, and a green unit suite do not establish deployment, crash recovery, or improvement. The committed 12-part audit defines finite completion gates and identifies the outstanding adapter obligations.

@tangletools tangletools left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Value Audit — sound

Verdict sound
Coverage 2 of 2 lenses (value, usefulness)
Concerns 0 (none)
Heuristic 0.0s
Duplication 0.0s
Interrogation 243.0s (2 bridge agents)
Total 243.0s

💰 Value — sound

Fixes two real provenance defects in the newly landed training path — receipts mislabeled with the evaluation executor's identity, and declared training datasets never checked against held-out eval content — by extending the codebase's existing admission-time assert pattern; coherent, in-grain, veri

  • What it does: Two behavior deltas. (1) createProfileImprovementHarness().train() no longer silently overwrites the training receipt's executionRef with the harness's evaluation identity: ProfileImprovementHarnessTrainOptions now requires a caller-supplied executionRef (profile-improvement-harness.ts:20,106-110) because ImproveTrainingOptions.executionRef 'pins trainer, serving adapter and their private dependen
  • Goals it achieves: Protects the integrity of the package's core promise (eval-gated self-improvement, per package.json description). Before this PR, a training receipt recorded the wrong execution identity — corrupting the provenance chain anything downstream (serving verification, audit) would trust — and a profile trained on held-out evaluation content sailed through every gate, inflating ship decisions (the audit
  • Assessment: Good on its merits. The exposure check is not a new invention: it extends an existing family of held-out hygiene asserts (assertReleaseSplitIsFresh/assertReleaseScenariosAreFresh in improvement-cycle.ts:600-640, assertDirectCandidateReleaseWorkIsFresh in authored-profile-improvement.ts:234-253) from optimizer scenario/split reuse to the new training-receipt provenance source, in the same assert-fu
  • Better / existing approach: none — this is the right approach. Searched rg for heldOut|held-out|decontam|contaminat|exposure across src/, docs/, candidate-execution/verify.ts, and runtime/promotion-gate.ts: the only existing held-out enforcement is the scenario/split freshness family cited above, which cannot see training receipts. Placing the check inside training.ts or schema validation would be wrong because training time
  • 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":1789867339889,"sessionID":"ses_f4397d0fdffeRJTDQCCIu6vAiH","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_dd391783"}}}

🎯 Usefulness — sound

A coherent provenance fix that corrects a wrong identity contract on the young train API and extends the codebase's established fail-closed release-freshness guards to training receipts at all three live measurement boundaries, with refusal provably preceding any spend.

  • Integration: Fully wired and reachable: assertProfileTrainingIsHeldOut runs inside method materialization (method-execution.ts:380, baseline at :394 before optimizer construction), and in both public intelligence proposal paths (improvement-cycle.ts:699,748; authored-profile-improvement.ts:174-175) exported via the ./intelligence subpath (package.json:37, api-surface.json:709-710). The harness.train executionR
  • Fit with existing patterns: Follows the established pattern exactly: assertReleaseScenariosAreFresh/assertProfileReleaseWorkIsFresh (improvement-cycle.ts:617-678) already refuse optimizer-visible reuse of release scenarios; this adds the analogous check for declared training lineage at the same boundaries, placed with the existing candidate admission helpers. intelligence→improvement imports already exist (improvement-cycle.
  • Real-world viability: Executed verification: 130 tests across the five touched suites pass, tsc --noEmit passes, build + verify-package-exports.mjs pass. Missing training identity returns a typed succeeded:false/stage:admission result rather than throwing (tests/profile-training.test.ts:356-371). Profiles without training metadata pass through unchanged (candidate-validation.ts:39), so existing profiles are unaffected.
  • Model: opencode/zai-coding-plan/glm-5.2
  • Bridge attempts: 1

No concerns — sound change, no better or existing approach found. ✅


What this audit checks

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

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

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

value-audit · 20260920T012621Z

@tangletools

Copy link
Copy Markdown
Contributor

✅ No Blockers — 896a48cd

Review health 100/100 · Reviewer score 65/100 · Confidence 95/100 · 10 findings (10 low)

glm: Correctness 65 · Security 65 · Testing 65 · Architecture 65

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 24 changed files. Global verifier still owns final merge decision.

🟡 LOW Held-out exposure check silently no-ops if training exporters use a different contentDigest domain — src/improvement/candidate-validation.ts

The guard is heldOutDigests.has(task.contentDigest) where heldOutDigests are campaignScenarioIdentity(scenario).scenarioDigest values (method-execution.ts:372) and contentDigest is a caller-supplied sha256 in the training receipt. Nothing at a schema or runtime boundary validates that a receipt's contentDigest was produced by the same canonical scenario serialization, so a dataset author who digests task content differently gets a silent pass on contaminated lineages. The repository's own fixtures and tests use the correct domain (tests/profile-training-exposure.test.ts:18,55), and docs/research/training-purpose-audit-2026-09-19.md explicitly discloses this boundary ('Exporters must preserve the same canonical content identity... This is not a general decontamination certificate'), so

🟡 LOW Error precedence change for doubly-invalid proposeAgentProfileImprovement inputs — src/intelligence/improvement-cycle.ts

Moving sealProfileImprovementBenchmark and the baseline training-exposure assert above the improvement.costCeiling equality check (lines 700-705) means callers that supply both an invalid benchmark/trained baseline and a mismatched costCeiling now get the benchmark or exposure error instead of the costCeiling error. All paths still fail closed before any spend (tests assert analysisCalls=0, methodCalls=0, measured=0), so impact is limited to callers matching on specific error text. No in-repo caller was found that depends on the old precedence; acceptable as-is.

🟡 LOW Held-out enforcement trusts host-declared digest consistency — src/intelligence/improvement-cycle.ts

heldOutDigests comes from benchmark.tasks[].scenario.digest and is intersected with profile.metadata.training receipts' dataset.tasks[].contentDigest. Both are host-declared Sha256Digest values; nothing at either call site (also authored-profile-improvement.ts:173) verifies the two digests were computed over the same canonical task bytes. A host that digests benchmark scenarios differently from the training pipeline silently passes overlapping content, defeating the guarantee this PR adds. The callee docstring acknowledges this ('exact provenance check, not fuzzy decontamination'), so it is a documented design limit, but it should be stated at the product boundary (e.g., host integration docs) because eval validity of every trained-profile release now rests on that discipline. Fix: documen

🟡 LOW Name-collision allowance control only covers the candidate arm — tests/authored-profile-improvement.test.ts

The negative control trains only options.candidateProfile with coincident benchmark/task names and fresh contentDigest. A trained baseline with the same coincident names is untested. Both arms pass through the same assertProfileTrainingIsHeldOut call (authored-profile-improvement.ts:174-175), so behavior is symmetric by construction; risk is negligible. Fix (optional): add the baseline arm to the loop or a mirrored case.

🟡 LOW New describe block omits the explicit timeout used by the sibling block — tests/authored-profile-improvement.test.ts

The pre-existing describe at line 135 uses { timeout: 30_000 } while the new 'trained profile release admission' block relies on the vitest.config.ts global testTimeout of 20_000. The allowance test runs the full paired measurement path, so its headroom is 20s vs the sibling's 30s for equivalent work. Tests currently finish in ~500ms, so this is cosmetic inconsistency, not a flake risk. Fix: pass { timeout: 30_000 } to the new describe for parity.

🟡 LOW Sort comparator never returns 0, so duplicate tasks fail with an indirect schema error — tests/helpers/trained-profile.ts

The comparator agentTrainingTaskKey(a) < agentTrainingTaskKey(b) ? -1 : 1 returns 1 for equal keys in both directions, an inconsistent comparator. If a future test passes duplicate tasks, Array.prototype.sort order is implementation-defined and snapshotAgentProfile then throws the zod issue 'training tasks must be sorted and unique' with no hint that the input contained duplicates. Production src/improvement/training.ts:364 uses the proper three-way form (a < b ? -1 : a > b ? 1 : 0) plus Map dedup. Impact today is nil (all current callers pass unique tasks and failure is still loud), but mirroring the three-way comparator (or deduplicating by key) would make failures self-explanatory. Fix: ` members = [...new Map(tasks.map((t) => [agentTrainingTaskKey(t), t])).values()].sort((a, b) =>

🟡 LOW Non-null assertion deviates from the file's task-guard convention — tests/improvement-cycle.test.ts

template.evaluation.experiment.benchmark.tasks[0]! uses a non-null assertion while every other test in this file guards with const task = ...[0]; if (!task) throw new Error('expected a profile improvement task') (e.g., lines 302-303, 489-490, 821-822). Purely stylistic; behavior is identical and biome accepts it. Fix: use the same if-guard for consistency.

🟡 LOW Baseline-exposure refusal only tested on default prompt surface — tests/profile-training-exposure.test.ts

The before-method-construction cases run with the default surface 'prompt'. The refusal is surface-independent (assertProfileTrainingIsHeldOut inspects profile.metadata.training inside materializeProfile), so behavior is covered, but a baseline trained profile run with surface 'agent-profile' is untested. Impact: minor; the check ordering before method construction is shared across surfaces. Fix: optionally add surface: 'agent-profile' to one inherited variant.

🟡 LOW No acceptance case for selection-partition training exposure — tests/profile-training-exposure.test.ts

The optimizer-selected acceptance case (exposedToFinal: false) only trains on a trainScenarios digest. A trained profile exposed to a selectionScenarios digest is also development-only content and should be accepted identically, but no test pins that. Impact: a future regression that mistakenly treats selection partitions as held-out would not be caught here. Fix: parameterize the accept case over trainScenarios[0] and selectionScenarios[0].

🟡 LOW Dead guard in the missing-identity test: the throwing agent callback can never be reached — tests/profile-training.test.ts

The agent callback throws 'must not execute an evaluation' (lines 361-363), but harness.train never invokes the bound agent under either the old or new implementation — under the removed fallback it would have substituted executionRef and succeeded at training, not called agent. The discriminating assertions are the admission failure and assertNoProfile; the agent guard documents intent but cannot fail. Harmless; could be dropped or the test name lean only on the admission assertions. No functional impact.


tangletools · 2026-09-20T02:08:07Z · 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 — 10 non-blocking findings — 896a48cd

Full multi-shot audit completed 8/8 planned shots over 24 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-09-20T02:08:07Z · immutable trace

@drewstone
drewstone merged commit ed008c7 into main Sep 20, 2026
4 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.

3 participants