skill_evolution: host integration hooks (analyst fn, incumbent score, richer trajectories)

[evekhm]

What

Three additive, backward-compatible seams in scripts/skill_evolution.py so a host system can adopt the engine as its single evolution implementation instead of maintaining a fork:

1. error_analyst_fn on collect_patches/evolve_skill — a host-supplied analyst for failure trajectories, called as fn(client, model, session, current_skill, tools) and returning patch text or None. Lets hosts plug in richer analysts (e.g. an agentic investigator with tool access, per Trace2Skill's finding that agentic analysis outperforms single-pass) while the fleet dispatch, quality gate, and consolidation stay in the engine. Success trajectories keep the built-in analyst.
2. incumbent_score on select_candidate/evolve_skill — hosts that already measured the base skill (the quality report the run consumes) pass the score instead of having score_fn(current_skill) re-measure the incumbent on fresh, noisy traffic.
3. Richer trajectory rendering in format_trajectory, rendered only when the session carries the keys (same backward-compatible pattern as tool_calls_detail): verification counts, correction_boundaries (wrong claim vs corrected fact), per-segment execution_sub_trajectories with traces, and a full-session execution_trace. _has_parroted_recovery also honors a parroted outcome in execution_sub_trajectories.

All new parameters default to prior behavior; sessions without the new keys render exactly as before.

Testing

• 7 new tests in tests/test_skill_evolution.py; 41 total pass.
• Adopted in production by the skill-evolution-lab host (its evolve.py is now a ~350-line adapter over this engine, replacing a 1,240-line fork; Evolution algorithm now lives in the SDK — evolve.py becomes a thin adapter evekhm/skill-evolution-lab#64 and fix: add ORDER BY before LIMIT for deterministic session ordering #56, both merged). Verified end-to-end on a fresh GCP project through the error_analyst_fn hook (agentic analysts, 100% of patches through the quality gate), the tools= parameter (live-derived agent toolbox), and the incumbent guard consuming the pre-measured baseline: local pipeline V0 30.8–46.2% → V1 100.0% meaningful on the evolve set, 38.2% → 100.0% on a 55-question held-out exam; deployed pipeline V0 23.1% → V1 100.0%.

[google-cla]

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

[caohy1988]

Review at b64cbc1 — approve pending CLA + one code decision

Reviewed the full diff against the engine's existing contracts (this is the follow-up seam extraction to the U6 work in #385). Ran the suite locally: 41/41 pass, including the 7 new tests. CI is green across Python 3.10–3.14.

What verifies well:

• All three seams are genuinely additive — defaults preserve prior behavior, and the existing tests are untouched.
• error_analyst_fn dispatch only replaces failure-trajectory analysts; host exceptions degrade to warnings through the shared fut.result() handler, so a host bug can't abort the fleet; the quality gate still applies to host patches — the right invariant.
• incumbent_score uses is not None (a legitimate 0.0 baseline works), participates in the margin gate, and the artifacts writer never re-scores, so the hook isn't defeated downstream.
• The prompt addition (correction-as-hypothesis) is consistent with the anti-parroting evidence from the U6 recording.
• _has_parroted_recovery honoring execution_sub_trajectories closes the reclassification gap for hosts that segment per correction.

[P1, merge blocker] cla/google is failing: commit b64cbc1 is authored by evekhm-odyssey-bot@users.noreply.github.com, which isn't covered by a CLA (the other commit is evekhm@google.com — that's why #385 passed and this one doesn't). Re-author the bot commit or get the bot account covered.

Two inline items.

[caohy1988]

[P2] This early return also skips the execution_trace block below — a host that supplies both execution_sub_trajectories and execution_trace silently loses the full-session trace. The comment above only justifies preferring segment traces over the sub_trajectories outcome list, the PR body presents the two renderings as independent, and no test covers both keys present (test_format_renders_full_session_execution_trace exercises execution_trace alone). Either narrow the early return to skip only the sub_trajectories fallback, or document the precedence here and add a both-keys test.

[evekhm]

Fixed in fa5271b — the precedence is now: per-segment traces suppress ONLY the redundant sub_trajectories outcome list (same labels, no evidence); the full-session execution_trace always renders. Locked with a dual-key test asserting both SEGMENT-TRACE and FULL-SESSION-TRACE appear and the brief list does not.

[caohy1988]

[P3 nit] incumbent_score has no effect when score_fn is None (the median-size path wins). Harmless, but worth one docstring line — otherwise a host can pass a baseline and believe the incumbent guard is active when it isn't.

[evekhm]

Fixed in fa5271b, and promoted beyond a docstring line: select_candidate now logs an explicit UNGATED warning when incumbent_score arrives without score_fn (test asserts it), raises ValueError on a non-finite incumbent_score (NaN/-inf/+inf regression tests), and the docstring scopes the guard to the score_fn-present case.

[caohy1988]

Code review: skill_evolution host integration hooks

Multi-perspective review of b64cbc1 against the main merge-base (740ae8e), run across 8 lenses (correctness, testing, maintainability, project standards, API contract, adversarial, host/agent discoverability, repo history). Every finding below was verified against the PR head checkout; the test suite was re-run locally (41 passed, matching the PR's claim).

Verdict: Not ready to merge — blocked on the failing cla/google check. The code itself is in good shape (genuinely additive, and it correctly preserves this module's established guard patterns); findings #3, #5, and #6 are worth fixing before hosts start relying on the new seams, the rest are hardening and documentation follow-ups.

What's solid

• All three seams degrade gracefully (or [], .get(..., ""), None defaults) — existing callers and old sessions render byte-identically. Verified, not just claimed.
• The PR preserves both of this module's historical invariants: sanitize-then-validate ordering (#301) and the incumbent-guard audit trail via selection.txt (#319).
• Docstrings for the three new parameters are clear and accurate.

Findings

P0 — blocking

#	Where	Finding
1	CONTRIBUTING.md:9 / PR checks	The cla/google check is failing. CONTRIBUTING.md requires contributions to be accompanied by a signed CLA, so this PR cannot be reviewed/merged by maintainers until the author signs at https://cla.developers.google.com/. All other checks (build, format, tests on 3.10-3.14) pass.

P1 — should fix

#	Where	Finding
2	scripts/skill_evolution.py:917	The four new session-dict keys (verifications, correction_boundaries, execution_sub_trajectories, execution_trace) are undocumented in the public contract. evolve_skill's Args docstring still says a report "must contain sessions with ... conversation or question/response" and never mentions the enrichment keys; scripts/README.md's Python-API knob list also omits error_analyst_fn and incumbent_score entirely. For a PR whose stated goal is host adoption, the discovery surface a host actually reads doesn't mention the seams. Fix: enumerate the optional keys (with shapes) in the evolve_skill/format_trajectory docstrings, citing quality_report.py as the reference producer, and add both new params to the README knob list + example.
3	scripts/skill_evolution.py:880	A non-finite incumbent_score silently disables the restraint guard. With incumbent = float('nan'), best_score < incumbent + min_improvement is always False, so the best-scoring candidate ships unconditionally — the exact "never ship a worse skill" property this function exists to enforce, defeated by one bad host value (-inf does the same trivially). Fix: validate at entry — if incumbent_score is not None and not math.isfinite(incumbent_score): raise ValueError(...) — and add a NaN/-inf regression test.

P2 — recommended

#	Where	Finding
4	scripts/skill_evolution.py:350,367	The outcome-icon mapping {"recovered": "+", "parroted": "~"} is now duplicated in both segment renderers. Extract a module-level _SEGMENT_OUTCOME_ICONS constant.
5	scripts/skill_evolution.py:357	The execution_sub_trajectories block ends in an early return result, so a session carrying both per-segment traces and a full-session execution_trace (or legacy sub_trajectories) never renders the latter — including the case where segments carry no trace text at all, which renders less evidence than before. Flagged independently by three review lenses. This is a design call: if segment-precedence is intended, document it at the branch and lock it with a dual-key test; if not, only early-return when segments actually carry traces (if exec_subtraj and any(seg.get("trace") for seg in exec_subtraj)), and let the execution_trace block still append.
6	scripts/skill_evolution.py:863	incumbent_score is silently ignored when score_fn is None — the median-size branch returns before the incumbent is ever read, so a host passing only incumbent_score gets ungated median selection with no signal. Flagged independently by three lenses. Fix: warn (matching the existing logger.warning style) or raise when incumbent_score is not None and score_fn is None, and scope the docstring promise to the score_fn-present case.
7	scripts/skill_evolution.py:752,909	error_analyst_fn=None carries no type hint while every neighboring callable param is typed (score_fn: Optional[Callable[[str], float]]). Add Optional[Callable[[Any, str, dict, str, Optional[str]], Optional[str]]] (or a named alias) in both signatures.
8	tests/test_skill_evolution.py	The early-return precedence of #5 is untested — no test constructs a session with execution_sub_trajectories together with sub_trajectories/execution_trace. This is exactly the backward-compat-sensitive path.
9	tests/test_skill_evolution.py	analyst_mode="both" + error_analyst_fn is untested — the docstring's "success trajectories always use the built-in analyst" contract is only exercised under analyst_mode="error-only".
10	scripts/skill_evolution.py:793-802	With client=None (now a legitimized pattern per the new test) and analyst_mode="both", every success-trajectory analyst raises inside its future and is swallowed by the blanket except Exception — all success patches silently vanish behind warning logs. Consider failing fast: client is required unless error_analyst_fn is set and analyst_mode="error-only".

Suggested resolution order

Theme	Findings	Type
CLA	#1	Author action — blocks everything
Incumbent-guard integrity	#3, #6	One validation block at the top of select_candidate resolves both; do #3 first
Rendering precedence	#5, #8	Decide the precedence semantics once, then lock with the dual-key test
Host-analyst contract hardening	#7, #9, #10	Mechanical; see also residual risks below
Host-facing docs	#2	Mechanical docs pass (docstrings + README + example)

Residual risks (informational, no action required to merge)

• A host analyst returning a truthy non-string (dict, response object) crashes collect_patches at the quality gate (patch.strip() -> AttributeError) after the entire fleet has run — outside the per-future exception handler, so a host type bug costs the full LLM spend. The documented "patch text or None" contract is undefended.
• No timeout on analyst futures: a blocking host error_analyst_fn hangs collect_patches indefinitely (as_completed has no timeout; the built-in path implicitly relies on client network timeouts).
• Run artifacts never record whether a patch came from the host analyst vs the built-in prompt — an auditor reading *_patches.json/selection.txt can't tell error_analyst_fn was even configured. A source field per patch record would close this.
• The new "user correction is a HYPOTHESIS" rule counterweights only the built-in ERROR_ANALYST_PROMPT; a host analyst receives the raw session — where correction_boundaries labels user assertions as correct_fact — and carries no such rule unless it reimplements it.
• A stale or unit-mismatched incumbent_score shifts the min_improvement gate by exactly the drift; the engine cannot sanity-check it — only host discipline and selection.txt cover it.
• evolve_skill is now 17 keyword params over a growing implicit session-dict schema, in a scripts/ module with no __all__/version signal, while being marketed as an adopt-don't-fork engine — no stability signal warns a second host before a future breaking change.

Additional test gaps

error_analyst_fn returning None / raising / returning gate-failing text (the gate is the stated defense but is only exercised with gate-passing fixtures); incumbent_score NaN/-inf; partition_trajectories reclassification via execution_sub_trajectories exercised only at the _has_parroted_recovery unit level.

---

_{Review method: 8 parallel specialized reviewers over the PR head checkout, findings cross-corroborated (items #5 and #6 were each flagged by 3 independent lenses), then verified by direct code inspection; test suite re-run locally. Generated with Claude Code ce-code-review, posted on behalf of the repository maintainer.}

[caohy1988]

Supplemental review at b64cbc1: one additional, independently reproduced trajectory-rendering defect that was not covered by the earlier review. Existing findings were deduplicated rather than reposted.

[caohy1988]

[P2] Render execution traces for single-turn sessions

This read is inside the if conversation branch, but quality_report.py can emit a supported session with question, response, and execution_trace and no conversation. format_trajectory() then uses the fallback at line 387 and silently omits the trace, so the analyst cannot see routing, tool-call, or tool-error evidence. Share the trace rendering across both session-shape branches and add a question/response + execution_trace regression test.

[evekhm]

Fixed in fa5271b: trace rendering is extracted into a shared _format_execution_trace helper used by both session shapes — a question/response session with execution_trace and no conversation now renders its routing/tool evidence. Regression test included.

[evekhm]

All review findings addressed:

• P0 CLA — the bot-authored commit is re-authored as evekhm@google.com (author-only rewrite, b64cbc1 → a71e5bf) and the branch is merged up to current main (post-Skill-evolution lab: remove the #358/#359 workarounds (U6, closes #360) #385). cla/google should now pass; the recurring bot-author trap is also being blocked mechanically via the repo pre-commit hook.
• P1 Revise README for clarity and updated link #3 + P2 Add Python 3.14 support and fix related test issues #6 (guard integrity) — fa5271b: entry validation in select_candidate (ValueError on non-finite incumbent_score; UNGATED warning when score_fn is absent), NaN/±inf tests, scoped docstring.
• P2 docs: add ontology and context graph learning guide #5/Move ontology reference documentation to docs/ontology/ #8 + supplemental (rendering) — fa5271b: segment traces suppress only the redundant outcome list, execution_trace always renders (dual-key test); single-turn sessions render their trace via a shared helper (regression test).
• P1 Revamp README, enhance documentation navigation, and fix CI #2 + P3 nit (docs) — fa5271b: the evolve_skill report docstring enumerates the optional enrichment keys with shapes; scripts/README.md knob list + example gain incumbent_score and error_analyst_fn with their scoping caveats.
• P2 Overhaul README, add documentation indexes, and fix CI issues #4/Set version to 0.1.0 and implement PyPI release pipeline #7/feat(ontology): add ontology models, loader, and tests #9/BigQuery Agent Analytics SDK Feature Wish List #10 (hardening) — fa5271b: _SEGMENT_OUTCOME_ICONS constant, ErrorAnalystFn type alias on both signatures, fail-fast when client=None outside hosted error-only mode, and the both-mode dispatch test (host analyst for failures, built-in for successes).
• Residual risks — filed as skill_evolution host hooks: contract hardening follow-ups (from PR #395 review) #397 rather than growing this diff (non-string patch crash, analyst timeouts, patch source provenance, host-side correction-as-hypothesis note, stability signal).

Suite at head: 54 passed (48 + 6 new). Ready for another look.

[caohy1988]

Fresh full review at fa5271b. The isolated PR-head test file passes (54 passed), git diff --check is clean, and all current CI checks are green. No P0 or P1 defect survived validation. I left three validated P2 findings inline.

Verdict: Ready with fixes.

Suggested order: preserve partial trajectory evidence, define the host-patch contract, then correct the verification-shape documentation.

[caohy1988]

[P2] Preserve unmatched correction outcomes

execution_sub_trajectories can be partial because _segment_trace_by_turns() skips individual segments that it cannot align to trace spans. This condition suppresses the entire sub_trajectories list as soon as any execution segment exists, so an unmatched parroted outcome can disappear even though the session is still classified as a failure.

Please render brief entries that have no matching execution segment, or retain the full brief list alongside the traced segments. Add a regression test with two brief outcomes but only one execution segment.

[evekhm]

Fixed in 201394e: brief entries with no traced counterpart (matched on start/end turns) render alongside the traced segments — only covered entries are suppressed. Regression test: two brief outcomes, one execution segment; the uncovered parroted outcome must appear.

[caohy1988]

[P2] Document the actual verifications shape

The reference producer emits verifications as an integer count (quality_report.py uses ctx.get("verifications", 0)), and format_trajectory() renders it as User verification requests: <count>. This docstring instead tells hosts to supply a list of records, which produces a raw list representation where the analyst prompt expects a count.

Please document this key as an integer count. If detailed verification records are also intended, give them a separate key and render that shape explicitly.

[evekhm]

Fixed in 201394e (also flagged as P1 in the second pass): the docstring now matches the producer/renderer/test — verifications is an int count.

[caohy1988]

[P2] Specify the required host-patch envelope

The callback contract promises patch text or None, but every returned value is later filtered by passes_quality_gate(), which also requires a minimum length, a recognized root-cause category, and exact markdown markers such as ## Root Cause plus ## Proposed Patch. A host can satisfy the documented return type and still have every patch silently discarded.

Please document the required envelope, expose or log specific gate-rejection reasons, and add tests for one valid and one malformed host callback result.

[evekhm]

Fixed in 201394e: the envelope is documented on error_analyst_fn (≥50 chars, a ROOT_CAUSE_CATEGORIES token, ## Root Cause/## Pattern + ## Proposed Patch/Content:), gate rejects now log their specific reason via _quality_gate_reason, and tests cover one valid + one malformed host result.

[caohy1988]

Full review (10 reviewer passes; every finding independently re-verified)

Verdict: ready with fixes. The PR delivers what it claims: all three seams were traced and confirmed additive and backward-compatible (every new format_trajectory branch is gated on key presence and falls through to the prior output; select_candidate's new parameter is appended with a default; no in-repo caller breaks on the new client=None ValueError). All five prior review rounds are verified addressed at fa5271b, and the deferred items are properly tracked in #397 (the issue body matches the deferral list).

9 findings are posted inline: 2 high (one is a one-line docstring fix), 6 moderate, 1 low. Three of them (the non-string-trace crash at :308, the NaN fallback at :919, the malformed-segment crash at :236) are new and not covered by #397 as filed; two others (:809 timeout, :807 non-string patch) overlap #397 items.

Suggested grouping

Theme	Comments	Resolution
Host-input hardening at the seam	:809, :807, :308, :236	Decide once: extend #397 to cover the two new items, or land the cheap isinstance/coercion guards in-PR and leave only the timeout deferred
Incumbent-guard consistency	:919, :892	One shared isfinite helper: validate the computed incumbent in select_candidate, call it early in evolve_skill
Standalone	:969 (docstring), :1019 (client-free path reachability), :347 (helper extraction)	:969 is the only one I'd consider merge-gating, and it's one line

Fix order suggestion: :969 docstring -> :919 + :892 shared helper -> decide the hardening group -> :1019 docs-or-wiring decision -> :347 extraction.

Non-blocking observations

• select_candidate logs the UNGATED warning before the empty-viable check (~:899): with incumbent_score set, score_fn=None, and no viable candidates, the log claims median-size selection while the base skill is actually returned.
• Exec sub-trajectory segments missing turn bounds render (turns None-None) (~:372); the sibling sub_trajectories renderer guards with is not None.
• A systematically failing host analyst degrades to a silent "No patches to consolidate" no-op visible only in logs (all per-future errors are warnings); same for oversized traces blowing up analyst prompts per-session.
• scripts/README.md's enrichment-key summary omits verifications and correction_boundaries; the CLI section doesn't mention that the score_fn/incumbent_score/error_analyst_fn family is Python-API-only.
• skill_evolution.py is now ~1,196 lines (1,047 at base); trajectory formatting would split out cleanly as a sibling module if enrichment keys keep accumulating.

Test coverage worth adding (mostly follow-up material)

• A byte-identical backward-compat test: a legacy session with none of the new keys should produce exactly the pre-change format_trajectory output (the PR's central claim; current tests assert feature presence only).
• Host-analyst failure paths: a raising error_analyst_fn (swallow-as-warning parity), a non-string return, and a host patch that fails passes_quality_gate (the README promises the gate applies).
• incumbent_score/error_analyst_fn driven through evolve_skill itself (only the inner functions are exercised today).
• The verifications test passes 2 (int) while the docstring says dict list — align with whichever shape :969 settles on.

[caohy1988]

P1 — docstring contradicts the actual verifications shape. This line documents verifications as a "list of {claim, verified} dicts", but the reference producer (quality_report.py), the renderer (f"User verification requests: {...}" — a bare interpolation), and this PR's own test (verifications=2) all treat it as an int count. A host following the docstring gets a raw Python repr dumped into the analyst prompt.

Suggested: describe it as an int count of user verification requests. (Flagged independently by two review passes; the mismatch is verifiable from the diff alone.)

[evekhm]

Fixed in 201394e — int count, per the producer, renderer, and test.

[caohy1988]

P1 — no timeout on analyst futures now that host callables run in the fleet. fut.result() in the as_completed loop below has no timeout. Before this PR the fleet ran only built-in genai calls; this hook admits arbitrary host callables into the same executor, so an error_analyst_fn that blocks forever hangs collect_patches indefinitely with no diagnostic.

I see #397 already tracks the analyst-future timeout — keeping it deferred is defensible; noting that the exposure grew with this seam. fut.result(timeout=...) with concurrent.futures.TimeoutError handled like any other analyst failure would close it.

[evekhm]

Implemented in 201394e rather than deferred — agreed the exposure grew with this seam. New analyst_timeout_s on collect_patches and evolve_skill (default None): a timed-out future degrades to a warning like any other analyst failure, and the executor shuts down without waiting when a timeout is set, so the hung worker cannot re-hang shutdown. Hung-analyst test included; #397 keeps the broader default-policy question.

[caohy1988]

P2 — the str-or-None contract is not enforced; a non-string patch crashes after the full fleet spend. Nothing checks the host analyst's return type: a truthy non-string (e.g. {"patch": ...}) passes the if result: append in the collection loop, then passes_quality_gate(p) calls .strip() on it and raises AttributeError outside the try/except that guards fut.result() — an uncaught crash at :842, after the entire fleet's LLM spend. (#397's wording implies a swallowed failure; it's actually uncaught.)

Suggested: append only when isinstance(result, str), logging and dropping anything else.

[evekhm]

Fixed in 201394e: truthy non-string returns are dropped with a warning at collection, before the gate — regression test asserts no post-spend AttributeError and the surviving patch is kept.

[caohy1988]

P2 — a structured execution_trace silently zeroes out the whole run. This concatenation assumes exec_trace is a str. format_trajectory runs inside the analyst futures, so a host supplying a list/dict trace (a plausible mistake — traces are often structured) raises TypeError per session, each swallowed as a warning by the broad except in the collection loop; the run degrades to "No patches to consolidate" and silently returns the unchanged skill.

Suggested: coerce non-strings (json.dumps(exec_trace, default=str)) — or reject loudly before dispatch, matching the spirit of the new client=None fail-fast.

[evekhm]

Fixed in 201394e: _format_execution_trace coerces non-str traces to a JSON dump (default=str, str() fallback) — a structured trace renders instead of zeroing the run. Test with a list-of-dicts trace.

[caohy1988]

P2 (structural) — consider extracting these blocks to match the helper convention this diff establishes. The Correction Evidence block here and the Execution sub-trajectories block below are ~12-15 inline lines each, while the same diff adds _format_execution_trace as an extracted helper. Both blocks depend only on session and would extract cleanly (_format_correction_evidence, _format_execution_subtrajectories), keeping format_trajectory from growing linearly as enrichment keys accumulate.

[evekhm]

Done in 201394e: _format_correction_evidence and _format_execution_subtrajectories extracted, matching the _format_execution_trace convention this diff established.

[caohy1988]

P2 — NaN from this fallback still silently disables the incumbent guard. The new finite-check guards the incumbent_score parameter, but this sibling branch is unguarded: a NaN returned by score_fn(current_skill) makes best_score < incumbent + min_improvement evaluate False, and the best candidate ships ungated — exactly the failure mode the ValueError's own message describes.

Suggested: validate the computed incumbent (covering both branches) with the same math.isfinite check.

[evekhm]

Fixed in 201394e: the computed score_fn(current_skill) fallback is finite-checked right after computation and raises with its own message. Regression test: a scorer returning NaN for the base skill.

[caohy1988]

P2 — this check fires only after the full fleet + consolidation spend. select_candidate runs last in evolve_skill, so a host passing NaN/inf incumbent_score burns the entire analyst fleet and best-of-N consolidation before getting this ValueError.

Suggested: hoist into a small shared helper called at the top of evolve_skill — pairs naturally with validating the computed incumbent (see the comment at :919), so one helper resolves both.

[evekhm]

Fixed in 201394e: validation is hoisted into _validate_incumbent_score, called at the very top of evolve_skill (before client creation, so a bad baseline fails before any fleet or consolidation spend) and still by select_candidate for standalone use.

[caohy1988]

P2 (design call) — the client-free path this PR validates is unreachable via evolve_skill. client = client or _make_client(project, location) at :1008 runs before this call, so the client=None + error_analyst_fn + analyst_mode='error-only' combination that collect_patches now explicitly supports can never occur through the public entry point — only by importing collect_patches directly. Meanwhile the README example attaches error_analyst_fn to evolve_skill, which is where hosts will look.

Either skip _make_client when the hosted-error-only combination holds, or state plainly in the README/docstrings that the client-free path is collect_patches-only.

[evekhm]

Documented in 201394e as the design answer: evolve_skill always requires a client because the consolidator uses it regardless of analyst mode — the client-free combination is a standalone-collect_patches surface, and both docstrings now say so explicitly.

[caohy1988]

P3 — one malformed segment entry kills the whole report. st.get("outcome") raises AttributeError if a host-supplied execution_sub_trajectories entry isn't a dict, and nothing catches it — partition_trajectories dies before any analysis runs. The unguarded pattern pre-existed for sub_trajectories, but this diff extends it to a new, host-supplied (less-trusted) key.

Suggested: if isinstance(st, dict) and st.get("outcome") == "parroted": so one bad session can't poison the run.

[evekhm]

Fixed in 201394e: _has_parroted_recovery and the renderer both filter non-dict segment entries; a malformed host entry can no longer kill partition_trajectories. Test included.

[caohy1988]

Second full review pass on fa5271b (10 reviewer lenses, every finding independently re-verified against the head tree). Two results worth recording:

1. Everything previously posted still stands. All 11 outstanding inline comments from the 2026-08-07 reviews re-validated against the current head — including the two on lines this review also converged on independently: the partial execution_sub_trajectories suppression at :386 (confirmed realistic: _segment_trace_by_turns in quality_report.py has multiple continue paths, so a proper-subset segment list occurs with the reference producer itself, and the suppressed brief list then drops parrot/recover evidence for the uncovered segments) and the undocumented passes_quality_gate envelope at :779 (confirmed: the gate requires length >= 50, a ROOT_CAUSE_CATEGORIES token, and ## Root Cause/## Pattern + ## Proposed Patch/Content: markers — none documented for error_analyst_fn, and rejected patches leave only an aggregate count in the logs). No duplicate comments posted for those; this note serves as corroboration on the existing threads.

2. Two new items with no existing thread are posted inline below: a missing failure-path test for a raising error_analyst_fn, and the all-host-futures-failed case degrading to a clean-looking no-op run.

Also re-verified this pass: format_trajectory output is byte-for-byte identical for legacy sessions (both conversation and single-turn shapes) when none of the new keys are present — the PR's backward-compatibility claim holds as stated. A small test locking that parity in would make it durable.

[caohy1988]

P2 (test gap, no existing thread) — no test covers a raising error_analyst_fn. The engine's tolerance for a failing host analyst rests entirely on the broad except Exception in the collection loop below, but every host-analyst test in tests/test_skill_evolution.py uses a well-behaved analyst — nothing exercises the swallow-and-continue path for this new branch. If that handling regresses (moved, narrowed), nothing catches it, and the failure mode is the engine's worst: a silent zero-patch run.

Suggested test: one collect_patches call where error_analyst_fn raises for one failure session and returns a valid patch for another; assert (via caplog) the warning fires, no exception propagates, and the surviving patch is collected.

[evekhm]

Added in 201394e: test_raising_host_analyst_partial_failure_tolerated — one raising host analyst, one valid; asserts the warning fires via caplog, nothing propagates, and the surviving patch is collected.

[caohy1988]

P2 (no existing thread) — all host futures failing degrades to a clean-looking no-op evolution. Every raising analyst future — including a systematically broken error_analyst_fn (wrong arity, import error, bad credentials in the host's closure) — hits the same per-future except Exception / logger.warning at :839-840. When ALL host futures fail, collect_patches returns an empty list and evolve_skill logs "No patches to consolidate; returning the current skill" — indistinguishable from a healthy run that genuinely found nothing to patch. The PR's own client=None ValueError exists precisely because this silent-swallow outcome was judged unacceptable for a predictable misconfiguration; a wholly-broken host analyst is the same failure class with no such guard.

Suggested: track per-kind failure counts in the collection loop; when error_analyst_fn is set and every host future raised, raise a RuntimeError naming the first exception (or at minimum logger.error with the aggregate). Partial failures stay tolerated as today.

[evekhm]

Implemented in 201394e: the collection loop tracks host failures, and when error_analyst_fn is set and EVERY host future raised, collect_patches raises RuntimeError naming the first exception — a systematically broken host can no longer masquerade as a healthy zero-patch run. Partial failures stay tolerated (both cases tested).

[evekhm]

All 13 findings from the 08-07 and 08-11 passes addressed in 201394e (replies on every thread):

• Evidence preservation: uncovered brief correction outcomes render alongside partial traced segments (turn-span matching); structured execution_trace values coerce to a JSON dump; malformed segment entries no longer kill partitioning.
• Host-analyst contract: envelope documented + gate rejects logged with their reason; non-string returns dropped pre-gate; all-host-failures raises RuntimeError; analyst_timeout_s bounds a hung analyst (executor shutdown does not re-hang); raising-host partial tolerance tested.
• Guard completeness: _validate_incumbent_score hoisted to the top of evolve_skill (fails before spend) and shared with select_candidate; the computed score_fn(current_skill) fallback is finite-checked too.
• Docs/structure: verifications documented as an int count; client-free path scoped to standalone collect_patches; correction-evidence and segment rendering extracted to helpers; legacy byte-parity locked by test (no new sections without new keys).

Suite at head: 64 passed (54 + 10 new). Branch is behind main again — say the word if you want a merge-up before the next pass, or I can leave the base stable for review.

[caohy1988]

Round-4 verification pass at 201394e — I re-ran everything rather than reading the summary, and all claims hold:

Verified against code + suite

• Suite count: 64 passed at this head (fetched into a clean worktree; exactly 10 new test functions, matching "54 + 10"). CI green on all checks at this head, cla/google included.
• Partial-segment evidence: covered_spans matching on (start_turn, end_turn) is implemented exactly as described; the new test pins both directions (uncovered parroted renders, covered brief entry stays suppressed — the "\n[+] covered" not in out assertion is the right one).
• Structured execution_trace: coerces via json.dumps(..., default=str) with a str() fallback; the comment documenting why (a per-session TypeError inside a future would zero the whole run) is accurate — that was the original failure mode.
• Malformed segments: non-dict entries are filtered in _has_parroted_recovery and both format paths; the partition-survival test passes.
• Host-analyst contract: envelope documented in the collect_patches docstring; gate rejects now log their reason via _quality_gate_reason; truthy non-strings are dropped pre-gate; all-host-failures raises RuntimeError (and correctly counts only host futures, so both-mode successes can't trip or shield it); analyst_timeout_s bounds the wait — the 0.3s-vs-release.wait(5) test genuinely proves the hang is bounded, and shutdown(wait=False, cancel_futures=True) (fine on requires-python >= 3.10) prevents the re-hang at executor teardown.
• Guard completeness: _validate_incumbent_score is hoisted above the report load and client construction in evolve_skill (so it fails before any spend, as claimed) and shared with select_candidate; the computed score_fn(current_skill) fallback is finite-checked too.
• verifications doc: now says int — this matches both the actual rendering (skill_evolution.py:377-378 prints the scalar) and the reference producer (quality_report.py:951,1067,1170 all write ints). The old docstring was the wrong one.
• Client-free scoping: the evolve_skill comment correctly states the consolidator always needs a client; standalone collect_patches keeps the client-free hosted error-only path.
• Legacy byte-parity: the no-new-sections test covers both session shapes.
• Threads: all 9 open review threads have your reply as the last comment; the 08-04 P3 nit is covered by the new select_candidate docstring sentence ("effect ONLY when score_fn is also provided").
• Housekeeping: the as_completed import is not orphaned (still used by the consolidation fleet at :1163).

Three small, non-blocking observations (no action required for merge, your call):

1. Duplicate UNGATED warning — _validate_incumbent_score now runs twice in the evolve_skill flow (top of evolve_skill, then inside select_candidate), so a host passing incumbent_score without score_fn gets the warning logged twice. Cosmetic; a local functools.lru_cache-style once-guard or just accepting it are both fine.
2. (None, None) span collision — covered_spans keys on seg.get(...); a traced segment missing both turn keys produces (None, None), which would suppress all spanless brief entries with the same key. The reference producer always emits turns, so this is theoretical, but a one-line if seg.get("start_turn") is not None guard when building covered_spans would make the matching total.
3. Worth one doc line for operators — with the switch from as_completed to sequential iteration, worst-case wait with a hung fleet is now N x analyst_timeout_s (per-future, in submission order), not analyst_timeout_s overall. That's the right trade for per-future bounding and the tests pin it, but hosts picking a timeout value should know it's per-analyst. The docstring says "per-analyst timeout" — arguably sufficient; flagging only because the sequential-iteration comment explains the mechanism without stating the aggregate bound.

Also noting a design question you may want to consider (not a defect vs. the stated contract): the all-host-failures RuntimeError counts exceptions/timeouts only. A systematically contract-violating host — one that returns a dict for every session — degrades to per-session warnings + a zero-patch run, the same clean-looking no-op the guard exists to prevent. Deterministic type bugs are usually caught in dev, unlike transient exceptions, so the current line is defensible — but if you want the guard to mean "zero usable host patches from a host that was called", counting pre-gate drops toward host_failed would close it.

Nothing here blocks. This round addresses all 13 findings; the residual-risk items tracked in #397 remain the right place for the rest. Branch is BEHIND main — per your offer, I'll leave the merge-up timing to you.

[evekhm]

Round-4 observations all taken, in 5cb50a2 (plus the merge-up to current main in 21bdd22, per your timing offer):

1. Duplicate UNGATED warning — now logs exactly once: evolve_skill's early call validates with warn_ungated=False (the pre-spend raise is unchanged) and select_candidate owns the warning. Test asserts exactly one occurrence through the combined flow.
2. (None, None) span collision — covered_spans skips traced segments with no turn keys, making the matching total; a spanless traced segment can no longer blanket-suppress spanless brief entries (test included).
3. Aggregate timeout bound — the docstring now states it explicitly: per analyst, in submission order, worst case N × analyst_timeout_s for a wholly hung fleet.
4. The design question, answered in the affirmative — pre-gate type drops now count toward the all-host-failures guard, so a host returning dicts for every session raises RuntimeError ("failed or returned an unusable result") instead of producing the clean-looking no-op the guard exists to prevent. Gate-rejected strings still don't count — that's a quality issue, not a broken host. Test: all-dict-returning host raises.

Suite at head: 67 passed; CI matrices green at 5cb50a2. From my side this is ready for the merge verdict.

[caohy1988]

Code review: round 5 (head 5cb50a2)

Verdict: approve. I re-verified the round-1–4 fixes at this head: the client-required guard, the all-host-failures RuntimeError, non-string patch handling, the spanless (None, None) suppression collision, the single-fire UNGATED warning, and the non-finite incumbent guard on both the incumbent_score parameter and the score_fn(current_skill) fallback all exist with tests.

I also stress-checked the per-future timeout design for a queue-delay flaw and it holds up: iteration is in submission order and the pool executes futures in the same order, so wall time spent waiting on earlier futures absorbs the queue delay of later ones — the documented N × analyst_timeout_s worst case is correct, and legitimately-slow-but-healthy backlogs don't spuriously time out.

Two residual observations, both minor and non-blocking:

1. Conflicting outcomes across span-matched segments get hidden (low). Suppression of brief sub_trajectories entries is keyed only on (start_turn, end_turn). If a brief entry says parroted for turns 1–2 while the traced execution_sub_trajectories segment for the same span says recovered, the brief entry — and its parroted evidence — vanishes from the rendered trajectory. Session classification is safe (_has_parroted_recovery checks both lists), but the analyst never sees the disagreement. Only matters if the two producers can disagree on the same span; if the reference producer guarantees agreement, fine to leave as is.

2. _validate_incumbent_score type errors (nit). math.isfinite("0.5") raises TypeError, not the documented ValueError. Harmless — it still fails before any model spend — but the docstring says ValueError.

CI is green across Python 3.10–3.14 at this head.

[caohy1988]

Review: Approve with nits

Verified locally: checked out the PR head and ran tests/test_skill_evolution.py — 67 tests pass. CI green across Python 3.10–3.14.

This is a well-constructed, genuinely additive PR: all three hooks default to prior behavior, the failure modes a host can induce (broken analyst, hung analyst, NaN baseline, malformed session keys) are each hardened and tested, and the docstrings are unusually honest about residual risks (e.g. the N×timeout worst case). Findings below are all minor — none block merge.

Findings

MINOR — silent behavior change for client=None (scripts/skill_evolution.py:847-855)
Previously collect_patches(report, skill, client=None, ...) returned [] (analyst futures failed with AttributeError, swallowed as warnings); it now raises ValueError. That's the right hardening — the old behavior was broken-by-design — but it contradicts the description's unqualified "all new parameters default to prior behavior." Worth a one-line note in the PR body or README changelog.

MINOR — _format_correction_evidence lacks the malformed-entry guard its siblings got (scripts/skill_evolution.py:302-315)
b.get('turn_index') raises AttributeError on a non-dict entry inside the analyst future, so the session is silently dropped as a warning. sub_trajectories, execution_sub_trajectories, and execution_trace were all hardened against exactly this (isinstance checks at :238, :402, :345); correction_boundaries was missed. Suggest filtering boundaries to dicts, same pattern.

MINOR — rendering asymmetry between session shapes; docstring overclaims (scripts/skill_evolution.py:442-454 vs :392-439)
The single-turn (question/response) path renders execution_trace but not execution_sub_trajectories, correction_boundaries, or verifications, while the evolve_skill docstring (:1098-1107) says the enrichment keys are rendered "when present" with no shape qualification. Either render in both shapes or qualify the docstring ("conversation-shaped sessions").

MINOR — unbounded trace injection into analyst prompts
execution_trace (:353-356) and per-segment trace (:336) are interpolated with no truncation. A host capturing verbose routing/LLM-request traces can multiply prompt size (and Vertex spend) per analyst call. The built-in path has the same exposure via tool_calls_detail, so pre-existing in spirit, but the new keys are the ones most likely to be huge. Suggest a documented cap or truncation note in the session-schema docs.

MINOR — stale PR description
Claims "7 new tests … 41 total pass." The diff actually adds 26 test functions; the file now collects 67 (ran them: 67 passed). The body also omits the two features the round 2–4 hardening commits added: analyst_timeout_s and the all-host-failures RuntimeError guard.

NIT — error_analyst_fn silently ignored with analyst_mode="success-only" (:860-873)
failures is emptied before dispatch, so a caller passing both gets no warning their analyst was never invoked. A one-line logger.warning would match the UNGATED-warning philosophy used for incumbent_score.

NIT — validation polish (:990-995)
_validate_incumbent_score raises for a non-finite score even when score_fn is None (score unused on that path — a warning would suffice); math.isfinite("0.9") raises the interpreter's generic TypeError, not the helpful message — a type check first would polish it.

NIT — cosmetic (turns None-None) (:332-334)
Traced-segment headers don't guard spanless segments the way the brief renderer does (span = "" at :431-435).

NIT — agent_recovered defaults to False (:313)
Prints "Agent recovered: False" for "unknown" when the key is absent; b.get('agent_recovered') rendering None would be less misleading.

Positives

• Exceptional failure-mode engineering: type-violating host returns counted toward the all-failures guard (:907-917), RuntimeError instead of a silent zero-patch run (:939-946), executor.shutdown(wait=…, cancel_futures=True) so a hung analyst can't re-hang shutdown (:934-937), non-finite incumbent rejected before any model spend (:1141).
• Real edge-case tests: the (None, None) span-collision suppression bug, partial traced-segment coverage, malformed segment entries, hung-analyst timeout, and a legacy-render parity test that directly substantiates the backward-compat claim.
• Filing #397 to track residual follow-ups rather than ballooning this diff — good hygiene.

Review by @caohy1988's assistant (Kimi Code CLI).

[caohy1988]

Code review: current head 5cb50a2

Verdict: Not ready to merge. CI is green and the local PR suite passes (67 passed), but the new host/selection seams still have three P1 failure paths. All findings below were reproduced against the current head.

P1 - should fix before merge

#	Where	Finding
1	scripts/skill_evolution.py:1054	Candidate scores are not checked with math.isfinite. A score_fn returning positive infinity for a candidate selects it and passes the improvement guard, even though non-finite incumbent scores are rejected to protect the same restraint property. Validate every candidate score and add NaN/infinity tests.
2	scripts/skill_evolution.py:901-937	Timed-out callbacks keep their executor slots. With max_workers=1, one blocking callback prevents a healthy queued callback from starting; both futures time out and the function raises a false all-host-failures error. In analyst_mode="both", a queued built-in success analyst can also be omitted. Use a cancellable/quarantined execution boundary with replacement capacity and add one-worker saturation tests.
3	scripts/skill_evolution.py:907	Falsy non-string callback results (False, 0, [], {}) skip the type check and look like valid no-patch results. Treat only None as the valid empty sentinel; count every other non-string as a host failure.

P2 - recommended

#	Where	Finding
4	scripts/skill_evolution.py:309,336	Optional malformed evidence can discard an otherwise valid trajectory. A dict/list segment trace raises TypeError; a non-dict correction boundary raises AttributeError. Coerce structured trace values and filter malformed nested entries instead of failing the analyst job.
5	scripts/skill_evolution.py:442-454	The single-turn question/response branch renders only execution_trace; it drops verifications, correction_boundaries, sub_trajectories, and execution_sub_trajectories. Use one shared enrichment renderer for both supported session shapes.

Validation and residual risk

• Reproduced Add streaming evaluation, Dashboard V2, design docs, and CI workflow #1 with score_fn(candidate) == float("inf").
• Reproduced Revamp README, enhance documentation navigation, and fix CI #2 with one blocked and one healthy callback at max_workers=1.
• Reproduced Revise README for clarity and updated link #3 with an all-False host callback.
• Reproduced Overhaul README, add documentation indexes, and fix CI issues #4 with a dict segment trace and a None correction boundary.
• Reproduced docs: add ontology and context graph learning guide #5 with an enriched single-turn session.
• Prior actionable review feedback remains addressed at this head.
• Timed-out worker threads can still outlive collect_patches and delay process exit. This is documented, but remains an operational risk.

Suggested order: timeout scheduling (#2) -> selection/result guard integrity (#1, #3) -> shared defensive evidence rendering (#4, #5).

[caohy1988]

Open-PR sweep recheck at unchanged head 5cb50a2.

No commits have landed since my full current-head review, and the current CI checks remain green. The existing Not ready verdict and findings still apply, especially the three P1 correctness issues around non-finite candidate scores, executor starvation after callback timeouts, and falsy non-string callback results.

Full current-head review: #395 (comment)

I am linking instead of duplicating the complete finding body on the same head.

[caohy1988]

Requesting changes at exact head 5cb50a2. The focused suite passes (67 tests) and current CI is green, but three P1 correctness/reliability defects remain:

1. scripts/skill_evolution.py:1054 validates only the incumbent score for finiteness. A candidate score of +inf can win and pass the margin gate, while NaN is silently skipped. Validate every candidate score and test all non-finite cases.
2. scripts/skill_evolution.py:907 checks if result and not isinstance(result, str), so falsy non-string host results (False, 0, {}, []) are treated as healthy no-patch results and can bypass the all-host-failures guard. Treat every non-None, non-string result as a host failure.
3. scripts/skill_evolution.py:867-937 uses a bounded executor and waits on futures in submission order. With max_workers=1, a first hung callback keeps the only worker after its wait times out; queued healthy callbacks never start and then time out too. That breaks the documented partial-failure tolerance. Add a hung-first/healthy-second regression test and use an execution boundary or scheduling strategy that preserves capacity after timeout.

These findings were independently revalidated on the current head.

[evekhm-odyssey-bot]

All five round-6 findings addressed in d2b23b3 (plus a merge-up to current main in c4af0db — no overlap with this PR's files):

P1-1 — non-finite candidate scores (select_candidate)
Every candidate score is now checked with math.isfinite; a non-finite score raises ValueError naming the value, symmetric with the existing incumbent-score guard. float("inf") can no longer win selection and pass the improvement gate. Tests cover inf, -inf, and NaN candidates.

P1-2 — executor starvation after callback timeouts (collect_patches)
Dispatch moved from ThreadPoolExecutor to a quarantine boundary (_AnalystCall): each call runs on its own daemon thread gated by a shared BoundedSemaphore(max_workers). On timeout the call is quarantined — a RUNNING call immediately hands its slot to the next queued analyst (replacement capacity), and a still-QUEUED call is cancelled outright, so each queued analyst always gets a fresh full timeout window. Two one-worker saturation tests: (a) max_workers=1, hung host + healthy queued host → the healthy patch is collected and no false all-host-failures RuntimeError; (b) analyst_mode="both" at max_workers=1 → the queued built-in success analyst still runs after the hung host is quarantined. Bonus on the residual risk you noted: quarantined threads are daemons now, so they can no longer delay process exit (the old executor's non-daemon workers could).

P1-3 — falsy non-string results (collect_patches)
None is now the ONLY valid no-patch sentinel; the check is result is None → skip, any other non-string (including False, 0, [], {}) → contract violation counting toward the all-host-failures guard. Tests: an all-falsy host (False/0/[]/{} across four sessions) trips the RuntimeError; an all-None host remains a healthy zero-patch run. Docstring updated to state the sentinel contract explicitly.

P2-4 — malformed optional evidence
New shared _coerce_trace_text helper (the same json.dumps-with-fallback the full-session trace already used) now also coerces per-segment trace values, and _format_correction_evidence filters non-dict boundary entries. Tests: a dict-valued segment trace renders as a readable dump; [None, "not-a-dict", {...valid...}] boundaries render the valid entry without AttributeError.

P2-5 — single-turn branch drops evidence
Both session shapes now share one enrichment renderer, _format_session_enrichments (corrections, verifications, correction evidence, per-segment traces, uncovered brief outcomes, full-session trace). Test: a question/response session with all enrichment keys renders every section, including a brief segment with no traced counterpart. The legacy-parity test (no new keys → no new headings) still passes for both shapes.

Verification at c4af0db: tests/test_skill_evolution.py → 75 passed (67 + 8 new); the two concurrency tests were run 5× consecutively with no flakes; pyink --check and isort clean; git diff --check clean.

[caohy1988]

Requesting changes at exact head c4af0db.

The prior round's candidate-score, falsy non-string, and queued-starvation defects are fixed, the focused suite passes (75 tests), git diff --check is clean, and current CI is green. The replacement timeout scheduler introduces one new P1 reliability defect and three related P2 defects:

1. P1 - quarantine no longer enforces max_workers (scripts/skill_evolution.py:891-893). On timeout, wait() releases the semaphore while the callback is still executing. Each replacement can therefore run concurrently with every quarantined callback. An exact-head reproduction with max_workers=1, 20 callbacks, and a short timeout observed 18 callbacks executing at once. This can turn slow analysts into an external-service fan-out rather than a bounded fleet.
2. P2 - one thread is allocated per trajectory (scripts/skill_evolution.py:863,966-984). Every _AnalystCall starts a daemon thread before the semaphore gate. A 300-session exact-head reproduction produced 302 process threads with max_workers=1; report size, not max_workers, bounds thread allocation.
3. P2 - max_workers=0 now hangs (scripts/skill_evolution.py:964). BoundedSemaphore(0) is accepted, every call blocks in acquire(), and the default wait(None) never returns. The previous ThreadPoolExecutor failed fast for this configuration. Validate a positive integral worker count before dispatch.
4. P2 - empty host strings bypass the sentinel contract (scripts/skill_evolution.py:1011-1037). The docs say None is the only healthy no-patch sentinel, but "" is silently skipped and whitespace-only strings are gate-rejected without counting as an unusable host result. Handle these explicitly and add all-empty/all-whitespace tests.

The independent validator confirmed all four findings. Please use a bounded dispatch/cancellation policy that measures actual active callbacks and live threads, not only semaphore tokens, then add scale/concurrency regressions.

[evekhm-odyssey-bot]

All four round-7 findings are addressed in a358b80 (which also merges the latest main).

1. P1 — quarantine now enforces a hard concurrency bound. Timeouts can no longer fan the fleet out one replacement per quarantined call. Slot hand-offs go through a donation budget in the new _FleetSlots: at most max_workers quarantined callables may run on donated slots at once; once the budget is exhausted, a timed-out call keeps its slot until the callable actually returns. Live analyst callables are therefore hard-capped at 2 × max_workers regardless of report size — the physics-imposed floor given that Python threads cannot be killed and the round-6 requirement that one hung analyst must not starve queued analysts at max_workers=1 (that behavior is preserved: the first timeout still donates its slot, and the round-6 tests still pass unchanged). Regression: test_quarantine_keeps_live_callables_bounded reproduces your scenario shape (max_workers=1, 12 slow callbacks, 0.05s timeout) measuring actual concurrent callable entries, and asserts peak ≤ 2 — deterministically, since the analysts block on an event until teardown.

2. P2 — lazy dispatch; threads bounded by workers, not report size. _AnalystCall no longer starts its thread in __init__. A dispatcher thread acquires a slot before creating each call's thread, and a call cancelled while still queued never gets a thread at all. Regression: test_thread_allocation_bounded_by_workers_not_report_size (60 sessions, max_workers=2) asserts threading.active_count() growth stays ≤ 6 over baseline; the old dispatch allocated all 60 up front.

3. P2 — max_workers validated before dispatch. New _validate_max_workers rejects zero/negative/non-int (incl. bool and floats) with ValueError, called at the top of collect_patches and hoisted to the top of evolve_skill before client creation (same pattern as _validate_incumbent_score). Tests cover 0, -3, 2.5, True in collect_patches and the pre-spend guarantee in evolve_skill (a _make_client that raises if reached).

4. P2 — blank strings no longer bypass the sentinel contract. "" and whitespace-only host returns are now explicit contract violations: warned, dropped before the quality gate, and counted toward the all-host-failures guard, so an all-blank host raises instead of reading as a healthy zero-patch run. Docstring updated (None remains the only no-patch sentinel). Tests: all-empty/all-whitespace trips the guard; one blank + one valid patch stays a tolerated partial failure.

Suite: 81 tests pass (75 prior + 6 new), git diff --check clean, autoformat applied.

[caohy1988]

Fresh review of 6378d7ba8a3ce33712dac90224b91b1d6dae86b1: no new actionable findings; approving.

I reviewed host callback dispatch and result validation, finite incumbent/candidate scoring, enriched trajectory rendering, and the lazy daemon-thread dispatcher with bounded quarantine donations. The prior reported candidate-score, sentinel, starvation, concurrency-budget, lazy-thread-allocation, and worker-count issues are addressed in this head. The focused suite passes: 81 tests, Python 3.13.5; current CI checks completed successfully or were skipped.

I also exercised the real #472 adapter against both engine versions. That exposed a caller-side scoring defect in #472: it supplies a production-report baseline while candidates are measured on a different eval set. The finding is posted on #472; it is not a defect in this PR's documented premeasured-score behavior.

Coverage limits: no live model or cloud calls. The timeout is deliberately applied per analyst in submission order, and timed-out daemon callbacks may continue until they return. Candidate-consolidation timeout policy remains outside this PR's changes.