refactor(improvement): consolidate training validation and execution lifecycle

[drewstone]

Status

Non-draft; all four required final-head CI checks pass on 39a546fd87e32186774e4e514c3a70ec8ec4ca23.

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

Passed: ci, agent-bench, official-optimizers, and packed-cohort. One product commit on main 6a419eac84bbf3ec80760d61385cd316395ea93b. No required check or protection was bypassed. tangletools currently shows its explicitly provisional auto-approval; that is not presented as a completed independent audit. The separate Codex reviewer reports exhausted review quota.

Purpose

Follow up on merged #1287 by consolidating execution infrastructure and removing training-only restrictions. One Runtime PR; no new dependency or parallel optimizer, dataset exporter, scheduler, provider, or deployment client.

Changes

• Use one candidate-validator implementation for training, optimization, composed method leaves, and bound harnesses. Preserve original callbacks in execution identity. Invalid overrides and ignored asynchronous results can no longer silently admit candidates.
• Extract the existing confirmed process-group teardown from the local coding harness and reuse it for command training. Preserve the old import location and existing grace/escalation policy. A child can flush its checkpoint on graceful shutdown; command success waits for the owned process group to disappear.
• Make training deadlines optional and use the existing chunked deadline timer and abort linking. Remove the seven-day cap, command-output 16 MiB cap, and streamed declared-input 1 GiB cap. Explicit caller output/checkpoint budgets, dataset snapshot bounds, nonempty checkpoints, and input identity checks remain enforced. Empty pinned configuration files are valid.
• Accept frozen profile results directly in improve and bound harnesses. Retraining updates auxiliary references to the old receipted checkpoint while preserving unrelated model choices. Interface's existing schema owns ancestry bounds; no duplicate literal limit.
• Snapshot direct command requests before asynchronous work and snapshot serving evidence before the next filesystem await. Callers cannot redirect an in-flight command or change the serving proof after returning it.

The change adds 72 net production TypeScript lines, including the extracted shared helper. Most additions are regression tests; the remaining changes document behavior and regenerate release metadata. Dependency versions, frozen lockfile, and exact cohort are unchanged.

Compatibility

Validators accept synchronously by returning undefined, or reject by throwing. Optimization previously ignored promises and other return values; it now rejects them, matching training. Use block bodies for side effects and perform asynchronous preparation outside this synchronous validation contract. This behavior change and the widened profile/deadline API are documented in Runtime 0.243.0. Bench 0.13.5 follows the Runtime dependency; benchmark APIs and grading do not change.

Verification

• Ten failure-first regressions fail against the merged feature's implementation and pass with these repairs.
• Twenty new regression cases cover long deadlines, omitted deadlines, cancellation, validator admission, retraining aliases, mutation boundaries, caller output budgets, empty pinned inputs, and real descendant checkpoint flushing.
• Full local suite and clean exact-main preparation each pass 4,246 tests, with six existing skips, across 318 passing/two skipped files. Source types and lint pass.
• Clean preparation: https://github.com/tangle-network/agent-runtime/actions/runs/35420693192 . The unchanged frozen dependency cohort installs; release/API metadata regenerate; full tests, lint, source/example types, release gates, packed exports, edge execution, and documentation freshness all pass. Shell pipelines propagate failures. The separate write-enabled job handled only verified Git objects and executed no package scripts.
• Downloaded and checked the preparation artifact digest, candidate/base SHAs, clean worktree, native logs, and every changed source/test/script Git blob against the locally tested bytes. They match exactly. No preparation workflow or patch carrier enters the product commit; the temporary workflow has been retired.
• Normal final-head PR CI above independently passes all four repository gates, including official optimizer and Bench integration.

Boundaries

The 35-day deadline test uses virtual time; it is not a claim of observed month-long uptime. Process-group cleanup covers the owned POSIX group, not separately detached sessions or an OS sandbox. Managed adapters still own remote cancellation/reconciliation. Byte checks, serving identity, receipt ancestry, disjoint task partitions, and receipt-before-profile publication are retained. Training does not itself establish model improvement or authorize promotion. No merge, publication, deployment, or paid training/inference performed.

[chatgpt-codex-connector]

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

[tangletools]

✅ Auto-approved drewstone PR — 39a546fd

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-19T04:19:59Z}

[tangletools]

⚠️ Review Interrupted — 39a546fd

The review runner stopped before publishing a final verdict: webhook_restarted.

State	Detail
Interrupted	webhook restarted

No review verdict was produced for this run. Trigger a fresh review on the current PR head if the PR is still open.

_{tangletools · #1300 · model: kimi-for-coding · updated 2026-09-19T05:33:33Z}

[tangletools]

✅ Auto-approved drewstone PR — 39a546fd

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-19T05:47:15Z}

[tangletools]

🟡 Value Audit — sound-with-nits

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

💰 Value — sound-with-nits

A follow-up consolidation to #1287 that fixes a real admission hole (async validators silently ignored), de-duplicates three divergent validator implementations into one, and replaces hand-rolled process teardown, deadline timers, and abort plumbing in the trainer with the runtime's existing primiti

• What it does: Four concrete deltas. (1) New src/improvement/candidate-validation.ts:25 exports assertCandidateValidator/validateProfileCandidate and is now the single validator admission path for method execution (src/improvement/method-execution.ts:391,420), training (src/improvement/training.ts:425), and the bound harness (src/improvement/profile-improvement-harness.ts:91,107,116); a validator that returns a
• Goals it achieves: Correctness and composability of the training path landed one commit earlier (6a419ea, #1287): every candidate admission goes through the same synchronous-void-or-throw contract regardless of entry point; a training child gets graceful shutdown and confirmed teardown identical to the coding harnesses; long-running training jobs are supported without a timer-overflow workaround cap; and an ImproveT
• Assessment: Good on its merits. It is precisely the kind of second pass a codebase wants after a large feature lands: the feature commit shipped three divergent copies of validator logic (one of which — method execution — lacked the async guard training had), a raw setTimeout with an arbitrary cap, manual AbortController listener management, and kill-only teardown. This PR converges all of them onto primitive
• Better / existing approach: none — this is the right approach. Searches run: rg 'process\.kill\(' src/ shows the only remaining hand-rolled group kill is src/runtime/isolated-checker.ts:177, which is an intentionally immediate-SIGKILL policy for a throwaway bwrap sandbox check (no grace desired, status via fd 3) — different semantics, not a missed consolidation target for this change; tangle-sandbox-exact-process-provider.
• 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":1789798077186,"sessionID":"ses_f47b8ae95ffekj0I990X5qwMu3","error":{"name":"UnknownError","data":{"message":"Unexpected server error. Check server logs for details.","ref":"err_d65bdc6b"}}}

🎯 Usefulness — sound

A coherent consolidation that replaces three bespoke validator copies and a raw-setTimeout/SIGKILL trainer lifecycle with the codebase's established deadline, abort, and process-group-teardown primitives, removing arbitrary ceilings that blocked realistic training runs — all reachable through the pu

• Integration: Fully wired. improve(profile, {mode:'training'}) and createCommandProfileTrainer are exported from src/index.ts:107 and documented as canonical in docs/canonical-api.md:196; the capability landed in the immediately preceding commit 6a419ea (#1287) and this PR is its consolidation follow-up, so callers exist now. scripts/verify-package-exports.mjs:307-320 (CI-gated) exercises the new frozen-p
• Fit with existing patterns: Fits the grain precisely — this PR is deletion of competing patterns in favor of established ones. Old trainer used a raw setTimeout with a 7-day ceiling (main:training.ts:419,378); now uses armDeadlineTimer from src/runtime/supervise/deadline.ts:12, the chunked-timer utility already used across ~12 supervise modules, whose header comment (deadline.ts:4) documents exactly the native-timer clam
• Real-world viability: Holds up beyond the happy path. I ran the four touched test files: 104/104 pass, including new boundary tests for no-implicit-deadline when timeoutMs is omitted, deadlines beyond native timer range, cancellation during input persistence and checkpoint fsync, adapters that ignore cancellation, committed profiles not retracted by late cancellation, and retrained-checkpoint model-reference rebinding
• Model: opencode/zai-coding-plan/glm-5.2
• Bridge attempts: 1

🔎 Heuristic Signals

🟡 Cruft: magic number added tests/profile-training-boundaries.test.ts

• const day = 24 * 60 * 60 * 1000

💰 Value Audit

🟡 Process-group teardown extraction stops one caller short [duplication] ``

src/runtime/isolated-checker.ts:173-181 still hand-rolls process.kill(-child.pid, 'SIGKILL') with fallback to child.kill, the same shape the extraction removed from training.ts. It is defensible to leave it: the checker wants immediate kill with no grace and has no group-exit-confirmation requirement, so forcing it through terminateProcessTreeAndConfirm would change its policy. A future pass could still give it the shared exit-confirmation loop if evidence shows sandbox checks leaking descenda

🟡 Shared helper retains a caller-specific default context label [maintenance] ``

terminateProcessTreeAndConfirm in its new generic home src/runtime/process-tree.ts:27 still defaults context = 'runLocalHarness'. All three current call sites either pass an explicit context (training.ts:269 passes 'createCommandProfileTrainer', worktree-harness.ts:826 passes 'defaultRunCommand') or rely on the default (local-harness.ts:742,1128). Preserving it was deliberate per the PR's 'preserve existing policy' stance, and error messages are unchanged for existing callers; a later cleanup

---

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 · 20260919T061029Z}

[tangletools]

✅ No Blockers — 39a546fd

Review health 100/100 · Reviewer score 59/100 · Confidence 95/100 · 12 findings (12 low)

glm: Correctness 59 · Security 59 · Testing 59 · Architecture 59

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

🟡 LOW improve() and harness docs lose the AgentProfile type identity — docs/api/index.md

The source change AgentProfile -> ReadonlyAgentProfile (improve.ts:87-99, profile-improvement-harness.ts:27) makes typedoc render 'profile: object' (index.md:3531) and leaves the profile parameter with no type line at all in both improve() overloads (index.md:9009-9011 and 9037-9039); previously both showed 'AgentProfile'. Readers of the generated API reference for the primary entrypoint can no longer see what the profile parameter is. The render is mechanically correct for DeepReadonly; fix at the source by making ReadonlyAgentProfile typedoc-renderable (e.g., an interface or preserved alias) and regenerate, or accept as a nit.

🟡 LOW Double blank line before the new section heading — docs/canonical-api.md

Lines 316-317 leave two consecutive blank lines before '### Candidate validation has one acceptance rule'; the rest of the file uses a single blank line between sections. Purely cosmetic; no content impact. Fix: collapse to one blank line.

🟡 LOW Aborted trainers now get SIGTERM-first teardown instead of immediate SIGKILL — src/improvement/training.ts

Base abort path sent SIGKILL to the process group immediately; head routes through terminateProcessTreeAndConfirm (SIGTERM, 250ms grace, SIGKILL, 1s confirm, then rejects 'process group N survived SIGKILL'). This is the runtime's standard ladder and adds bounded worst-case ~1.25s cancellation latency plus a hard failure signal when a descendant survives SIGKILL — a net improvement — but a trainer descendant that ignores SIGTERM now delays cancellation by up to 250ms per abort, and on the survived-SIGKILL path the execute promise settles with the teardown error rather than the original 'trainer cancelled' failure (failure ??= ordering). Both are acceptable; noting for verifier awareness only. No change requested.

🟡 LOW Admission-time resource ceilings removed for trainer output and training duration — src/improvement/training.ts

positiveLimit lost its maximum parameter: trainer maxOutputBytes had a hard 16 MiB admission cap (now any positive safe integer, tested at 18 MiB) and timeoutMs had a 7-day cap (now unbounded, chunk-armed via armDeadlineTimer), and timeoutMs is now optional so a training run can have no deadline at all. These are deliberate, documented ('Omit to rely on caller cancellation'), and enforcement remains at runtime (output drained with kill at limit; caller signal; managed-job trainingMayExist reporting). But a caller typo like maxOutputBytes: 1e15 or a forgotten timeoutMs is no longer rejected up front, so a hung trainer with no signal runs forever inside the library. Consider keeping an advisory upper bound or requiring an explicit timeoutMs: Infinity opt-out at the harness laye

🟡 LOW Ancestry-limit failure now surfaces a raw ZodError JSON blob as result.reason — src/improvement/training.ts

Base threw new Error('training receipt ancestry limit exceeded') when the chain exceeded 8 receipts. Head delegates to agentProfileTrainingSchema.shape.ancestors.parse(...) (z.array(...).max(8)), which throws a ZodError whose .message is a stringified issues array; the catch at line 602 puts that JSON blob into result.reason. The refusal still triggers at the same depth (verified: a parent with 8 ancestors is schema-valid, the 9-element array fails max(8)), and per-element receipt validation is strictly stronger than before, but operators reading the failure result lose the actionable one-line message. Fix: catch the ZodError and rethrow `new Error('traini

🟡 LOW Re-export shim duplicates the import of terminateProcessTreeAndConfirm — src/mcp/local-harness.ts

Line 41 imports terminateProcessTreeAndConfirm for internal use and line 51 re-exports the same symbol from the same module. The re-export is intentionally load-bearing for src/mcp/worktree-harness.ts (imports it from './local-harness'), so this is correct as-is; a future cleanup could point worktree-harness.ts at runtime/process-tree directly and drop the shim. No action required for this PR.

🟡 LOW Process-group confirm loop is exposed to a pgid-reuse race — src/runtime/process-tree.ts

waitForProcessGroupExit polls process.kill(-pgid, 0) and terminateProcessTreeAndConfirm escalates to SIGKILL on the polled pgid. If the group exits and the OS reuses the pid as a new process-group leader inside the 250ms grace + 1s kill window, the poll misreads the group as alive and signalProcessTree(child, 'SIGKILL') (line 16) targets -child.pid, potentially signaling an unrelated process group or throwing a false 'survived SIGKILL'. This logic is moved verbatim from the base revision (old local-harness.ts:1727-1737), so the PR introduces no regression, and the ~1.25s window makes exploitation or false failure improbable on Linux pid allocation. If hardening l

🟡 LOW processGroupExists treats EPERM as group-alive with no distinction — src/runtime/process-tree.ts

Non-ESRCH errors (e.g. EPERM when a group member is owned by another uid) return true, so terminateProcessTreeAndConfirm waits the full 1s kill-confirm window and then throws 'survived SIGKILL' even though SIGKILL was delivered to killable members. Verbatim carry-over from base; conservative direction (never falsely reports success), and callers convert the throw into a failed-run result rather than a hang. Acceptable as-is; a debug-grade detail worth knowing when diagnosing the rare 'survived SIGKILL' error.

🟡 LOW Fake timers cannot detect the native timer-clamp regression the test name promises — tests/profile-training-boundaries.test.ts

The test arms a 35-day deadline under vi.useFakeTimers and asserts it does not fire early. Sinon fake timers schedule the full delay with no 32-bit clamp, so the pre-fix naive setTimeout(onDeadline, remaining) passes this test (verified by mutation: replacing armDeadlineTimer's Math.min(remainingMs, MAX_TIMER_DELAY_MS) chunking with a single full-delay setTimeout still passes, while real Node would clamp >2^31-1ms to 1ms and abort training near-instantly). The test does validly pin exact firing time, stage='training', trainingMayExist=true, /deadline/ reason, and zero leaked timers. Fix: add an assertion that the first armed delay is chunked (e.g. vi.spyOn(globalThis, 'setTimeout') and assert the first scheduled delay <= 2_147_483_647), or unit-test armDeadlineTimer chunking with a schedul

🟡 LOW No-implicit-deadline test cannot detect an implicit default deadline — tests/profile-training-boundaries.test.ts

The test only asserts runProfileTraining succeeds without timeoutMs. A regression that arms an implicit default deadline (e.g. 10 minutes) would still pass, and the sibling 'still cancels a managed job when no deadline is configured' test also passes because the caller abort fires first. Strengthen cheaply: with vi.useFakeTimers({ toFake: ['setTimeout'] }) around the call, assert vi.getTimerCount() === 0 once training starts, directly pinning that omission arms no timer.

🟡 LOW Descendant-flush test depends on grandchild finishing inside the fixed 250ms SIGTERM grace window — tests/profile-training.test.ts

The grandchild only writes 'final child checkpoint' from its SIGTERM handler; terminateProcessTreeAndConfirm (src/runtime/process-tree.ts, processKillGraceMs = 250) escalates to SIGKILL if the process group survives 250ms after SIGTERM. Under loaded CI, signal delivery plus one event-loop turn plus writeFileSync could exceed 250ms, killing the grandchild mid-write and failing this test intermittently (missing or partial checkpoint). Impact: low but real flake risk. Fix: none required to merge; options include making the grace window configurable for tests or asserting success-with-retry in CI. Not a correctness bug in the shot's code.

🟡 LOW Test title claims a nonempty-checkpoint assertion it does not make — tests/profile-training.test.ts

The test only exercises the allowed side (empty pinned input with digest sha256Utf8('') still succeeds through improve). It never asserts the 'requiring a nonempty checkpoint' half of its title; that requirement is covered separately by the pre-existing 'empty checkpoint' refusal case at line 204. Impact: misleading coverage label; a future reader may believe this test guards checkpoint nonemptiness. Fix: drop 'while still requiring a nonempty checkpoint' from the title, or add an explicit companion assertion (e.g. a trainer writing an empty checkpoint with an empty input present must fail at stage 'checkpoint').

---

_{tangletools · 2026-09-19T06:37:39Z · trace}

[tangletools]

✅ Approved — 12 non-blocking findings — 39a546fd

Full multi-shot audit completed 8/8 planned shots over 23 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-19T06:37:39Z · immutable trace}