Repository navigation
Play a real-model scripted conversation nightly and assert a scored debrief - #507
charneykaye wants to merge 64 commits into
Conversation
…model smoke The nightly real-model harness previously proved one thing: a single NPC turn came back inside a latency budget. That leaves the two most valuable signals untested — whether a real model can sustain a multi-turn conversation, and whether the pipeline turns one into a *scored* debrief — and when it did fail, the output did not say whether the model failed to download, the server crashed, or the product rejected the model's output. The harness now runs the full journey and classifies every failure: - Scripted multi-turn conversation (the same three player turns as the fake-runtime playthrough in tests/e2e, so both cover the same shape), followed by session end and debrief generation. - Asserts a scored debrief: at least one rubric dimension scored, a numeric overall_score in [0, 100], a non-empty summary. Per-turn parse flags from the debug endpoint catch a conversation that only *looks* real — if every generated turn fell back to the canned safe utterance, the run fails. - Every failure gets one of six classes, each with its own exit code, banner, remedy and report field: budget (1), download (2), checksum (3), runtime (4), pipeline (5), timeout (6). A crashed child process outranks the client-side symptom it caused, so a dead core reads as `runtime`, not `pipeline`. - Checksum drift fails loudly: the on-disk file is re-verified before the weights are loaded, so a drifted or truncated *cache hit* is caught instead of being fed to the smoke. A mismatch deletes the bad local file. - A wall-clock deadline (default 25 min) trips before the CI job timeout so the failure names the phase that ran long, and caps each request timeout to the budget that is left. - Registry resolution moves into the script (--print-registry-model), so the cache key, download URL and expected checksum all come from one lookup. - The run writes a GitHub step summary (verdict, class, remedy, measured latencies, phase durations) alongside the JSON report artifact. The headline full_response_ms is now the median of the scripted turns, so one unlucky turn cannot flap the nightly. session_start_ms and debrief_ms are measured but not budget-checked: the NPC opening is authored scenario text rather than a generation (the old script's comments claimed otherwise), and debrief latency has no documented SLO. tests/scripts/test_nightly_model_smoke.py covers the harness's decision logic with no model, network or llama-server: checksum verification, the end-to-end assertions, budget arithmetic, failure classification, reporting, and the full run orchestration against faked-out servers. Writing it caught a real bug — Deadline.finish() checked the budget, so closing the books on an already exhausted deadline raised out of run_smoke's finally block and threw the classified verdict away, turning a reportable timeout into a bare traceback. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… min budget The nightly job now drives the extended harness and documents what it costs: - timeout-minutes drops from 60 to 30, the budget issue #457 asks for, with the per-phase breakdown recorded in the workflow header (~16 min warm cache, ~18 min cold). The harness's own 25 min deadline trips first so a slow run names its phase instead of surfacing as "The operation was canceled". - actions/cache splits into restore + save, with the save gated on success(). actions/cache's post step saves even when the job failed, which would have written a GGUF that failed verification into the cache — under a key derived from the expected hash, so every later run would restore the same bad bytes. - The run re-verifies the model against the registry checksum, so a drifted cache hit fails with exit 3 rather than being loaded. - MODEL_CACHE_PREFIX gives the one lever that actually clears a poisoned cache entry, since Actions cache entries are immutable. - The inline Python that read registry.yaml is replaced by the script's --print-registry-model, so the cache key, URL and checksum share one lookup. The harness's own unit tests run per-PR in the acceptance job. They need no model and take under a second, and without them the harness is only ever exercised at 04:00 UTC — so a typo in it shows up as a mysteriously red nightly rather than a red PR. Verified with actionlint across every workflow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…local repro docs/real-model-smoke.md (mirrored to the docs site under Development) is the page a red nightly sends you to. It covers what the job proves, the failure class table with the first thing to do for each, the runtime budget with its measured per-phase breakdown and why the ceiling is 30 min, how the CI hardware factor is calibrated, what to do about checksum drift (and why never to "fix" it by updating the expected hash), and a copy-pasteable local repro. Also: ROADMAP marks the Milestone-1 polish item done, and both copies of the performance doc now link the nightly that actually enforces the latency budgets they state — the claim "nightly CI smoke tests flag any regression greater than 20 %" previously had no reachable referent. The offline-smoke-tests pages point at it too, since "real model verification" there means the network promise, not the end-to-end one. Closes #457 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Self-review of #507 found three paths where the smoke reports a class that is not what actually happened — the one thing #457 asks it to get right. Ending the session twice. The turn pipeline sets flow_state to 'Ended' as soon as a turn carries an ending_type (turn_pipeline.py), so the explicit POST /end that follows answers 409, and _request_json files 4xx as `pipeline`. A scenario that ended on its own terms would have failed the nightly with exit 5 and "an end-to-end assertion failed". Skip /end when the scenario already ended; the existing early-ending test faked /end as 200 and so could not see this, and now asserts the call is not made. A slow debrief read as a crashed server. CONVSIM_LLAMA_CPP_TIMEOUT was sized for one NPC turn (300 s against a measured ~116 s), but the debrief is the slowest single generation at ~240 s — 25 % headroom, against the ~2x this job holds everything else to. Past that the adapter times out, convsim-core answers 500, and slowness is reported as `runtime` with a remedy about runner OOM. Size the adapter ceiling for the debrief instead (DEBRIEF_SLOWDOWN_FACTOR x the full-response CI ceiling) and keep our own POST timeout above it, so the component that gives up first is the one whose error names the knob. A lost deadline. cap() shrinks each request timeout to the budget that remains, so the last call before the deadline dies client-side and raises RUNTIME — exit 4 for what is exactly the condition exit 6 exists to report. Reclassify as `timeout` when the clock is exhausted and no child crashed; a crashed child still outranks both. Also: --wall-clock-budget-s 1500 (25 min) could not reliably trip before the 30 min job timeout it is supposed to pre-empt, because timeout-minutes covers the whole job while the harness clock starts at its own step — checkout, three pip installs, the cache restore and a cold 2.5 GB download burn up to ~8 min the harness never sees. On exactly the cold-cache nights where an attributed verdict matters most, GitHub would cancel the job first. Drop it to 1200 s (20 min) and add a test that parses the workflow so the two budgets cannot drift back together. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
Three ways the harness could report the wrong thing, all found by reading the classification contract back against the code. A cut-off HTTP transfer raises http.client.IncompleteRead, which is not an OSError, so the OSError-only handler in _download_with_progress let a truncated 2.5 GB download escape as a bare traceback: exit 1, which EXIT_CODES reads as `budget` — the one class that means "the product is fine, it is just slow" — and left the partial file on disk for the next run to checksum-fail on. Catch anything the transfer raises and classify it as `download`, which is what it is. The pre-run modes (--print-registry-model, --download-only, --verify-only) had no equivalent of run_smoke's catch-all, so an unparseable registry.yaml also exited 1 with no banner and no step summary. Classify unanticipated errors there the way run_smoke classifies its own, so exit 1 keeps meaning only what the table claims. Per-turn used_fallback flags come from the best-effort debug endpoint. When they were unavailable, every generated turn read as "not a fallback" and the run passed claiming real NPC turns — a canned safe utterance is a non-empty utterance too, so nothing else distinguishes them. Losing a diagnostic should not turn a healthy nightly red, so warn rather than fail, but say in the report that the proof is missing instead of implying it held. Tests for all three, plus the CLI entry point, which had none. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
run_smoke held the throwaway data directory in a `with tempfile.TemporaryDirectory(...)` nested inside the try block, but the children are stopped in the enclosing `finally`. A `with` block exits before the `finally` of the try it sits in, so the ordering was: delete the data directory, *then* terminate convsim-core — the server was still live, holding its SQLite database, WAL and log files open in a tree being rmtree'd. Unlinking open files is harmless on Linux, but any file the server creates during that window makes rmtree raise, and run_smoke's catch-all reports that as an unexpected harness error: exit 5, class `pipeline`, on a run whose conversation and debrief had both already passed. That is precisely the kind of unattributable red nightly this harness exists to prevent. Create the directory with mkdtemp and remove it in the same `finally`, after both children are stopped, with errors ignored — a stray file left behind is a tidiness problem, not a reason to overturn a decided verdict (the same reason Deadline.finish() does not raise). Covered by a new test asserting the directory still exists when convsim-core is terminated and is gone once run_smoke returns. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
argparse exits 2 on a usage error, and 2 is the `download` code — whose remedy is "nothing about the app changed, re-run the job". A triager following that advice re-runs a job that cannot succeed, because the problem is the invocation. The module docstring and docs/real-model-smoke.md both promise that every exit code names a class and that nothing escapes unclassified; exit 2 with no banner contradicted both. Route usage errors through the `pipeline` class, like every other surprise the harness did not anticipate. That exposed a second, smaller dishonesty: three failures now borrow `pipeline`'s exit code without sharing its cause (a usage error, a malformed registry.yaml, a harness bug mid-run), and all three printed the class remedy, "inspect the per-turn used_fallback flags" — pointing triage at model output when the thing to read is the traceback or the usage line. _print_banner and the step summary now take a per-failure remedy override, and each of the three supplies the advice that actually applies. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
Deadline.enter() switched phases and then checked the budget, so an overrun noticed at a phase boundary was attributed to the phase about to begin. A conversation that ate the whole budget reported "Wall-clock budget exhausted during phase 'debrief'" before the debrief had executed an instruction, and failed_phase in the report said the same — pointing triage at the wrong code in the one failure class whose entire purpose is naming where the time went. (Overruns noticed mid-phase, via clock.check() and cap(), were always attributed correctly; only the boundary case was wrong.) Check first, while self._phase is still the phase that consumed the budget, then switch. finish() in run_smoke's finally still closes out that phase, so phase_durations_s now also credits the elapsed time to the phase that spent it rather than splitting it across the boundary. Also record the class's remedy in the JSON report. The failure table in docs/real-model-smoke.md sends a triager to the report artifact for several classes; it carried failure_class but not the advice that goes with it, so the mapping had to be looked up elsewhere. The step summary already printed it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ons measurements Two accuracy problems in the runtime-budget accounting. The pre-smoke tally — the steps that burn timeout-minutes before the harness deadline starts ticking — listed checkout, pip install, cache restore and the cold 2.5 GB download, but not the `actions/cache/save` step this PR introduced by splitting restore from save. Uploading 2.5 GB of incompressible GGUF is ~1 min, so the cold-path figure is ~9 min, not ~8, leaving ~1 min of margin under the 30 min job timeout rather than ~2. The guard test asserted the understated number, so it would have let a future budget increase through. And the per-phase table was presented as "Measured on ubuntu-latest" when only the download, verification, cache and model-load rows are. The two inference rows are projected from the single latency this job has ever measured (~116 s for one behavioral-interview turn); the multi-turn conversation and the debrief have never run on a runner. Marked as projections, with a note to replace them with phase_durations_s from the first green nightly — a budget table nobody can tell the measured rows from is a table nobody will correct. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
run_smoke() classifies everything inside its own try/finally, but it was called from outside main()'s classifying handler — and its own prologue (the throwaway convsim-core data directory) and epilogue (the report and step-summary writes that follow the verdict) sit outside that block. An OSError there escaped as a bare traceback, so Python exited 1, which EXIT_CODES reads as `budget`: the one class whose remedy says the product is fine and just slow, and sends the triager looking for a prompt-size or sampling regression that does not exist. A full disk is the realistic cause, and the nightly reaches that step right after writing a 2.5 GB download and saving a 2.5 GB cache entry. Move the run_smoke() call inside the handler that already classifies the pre-run modes' surprises, so a banner and a step summary are still written and exit 1 keeps meaning only what it claims to mean. Its message and remedy now cover an escape from either side of run_smoke rather than only "before the smoke started", and the docs name a mid-run disk fill among the unanticipated failures that land on exit 5. Two tests cover it: mkdtemp failing in the prologue, and the report write failing after a verdict was already decided. Both exited 1 before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
`resolve_registry_model` classified a registry that names no `starter`
model — or one missing `download.url`/`download.sha256` — as `download`,
exit 2. That class's remedy ends "Nothing about the app changed — re-run
the job", which is advice that loops forever here: no number of re-runs
makes a missing registry entry appear, and nothing was ever fetched for
the network to be blamed for. It is the same trap the argparse exit-2
fix closed, reached by a different route.
These failures now take `pipeline`, the harness's catch-all for work it
could not carry out, and carry their own remedy — fix the registry file
and merge — rather than `pipeline`'s default "inspect the per-turn
used_fallback flags", which describes a conversation that never ran.
docs/real-model-smoke.md already promised exactly this ("a malformed
registry.yaml ... is also reported as exit 5"); now the code agrees.
- `SmokeFailure` takes an optional per-instance `remedy` override.
- `run_smoke` and `main` print and record that remedy instead of the
class default. `run_smoke` falls back to the class remedy when a
crashed child or an exhausted clock has overruled the class, since the
original failure's advice then describes a different failure.
- Tests: the override, the new classification for a missing role, an
ambiguous role, a missing checksum, and the step summary a registry
failure actually writes.
Also make the runtime-budget breakdown add up: the rows summed to ~2 min
less than the stated totals because checkout and setup-python were never
listed. Adds that row and corrects the warm-cache total to ~15 min, in
the workflow header and both copies of the doc.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
Issue #457 asks the nightly's failure output to distinguish a runtime crash from a pipeline assertion. Two functions make that call — `_request_json` (5xx ⇒ `runtime`, 4xx ⇒ `pipeline`, transport error ⇒ `runtime`) and `_wait_for_http` (an exited child is named immediately instead of polled for) — and every orchestration test monkeypatched both of them away, so neither ran under test. A mutation that collapses the 5xx/4xx split, or that drops the crashed-child short-circuit, passed the whole suite. Cover both directly. The readiness tests use timeouts far below the 300 s the real model-load wait allows: with the short-circuit removed, the crashed-child case would otherwise stall the per-PR suite for five minutes instead of going red. Verified both mutations now fail in under a second. Also report the last connection error with `!r`, as everything else in the harness does. `str()` on an exception raised with no arguments is the empty string, so a readiness failure could render as "llama-server did not become ready within 300 s ()" — leaving the `runtime` verdict's one diagnostic blank. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
…gression The new "scored debrief" assertion is the #457 acceptance criterion and stays as it is, but the thing it asserts on is thinner than the assertion implies: the debrief's dimension scores are accumulated purely from rubric_observations the *model* volunteers. Nothing asks it for them. The built-in behavioral_interview scenario defines no rubric, no prompt layer names any rubric dimensions, and the model's only hint is the bare rubric_observations array in the embedded output schema — whose empty list the schema accepts. The fake runtime returns [] always, which is why the per-PR playthrough asserts only that `scores` is a dict and never that it is non-empty. So the single most likely first failure of this job is an unscored debrief that is not a regression at all — and `pipeline`'s stock remedy, "inspect the per-turn used_fallback flags", would send a triager hunting a parse failure that never happened. The no-scores failure now carries its own note naming the cause and how to tell the two apart (did any earlier nightly ever score this scenario?), and docs/real-model-smoke.md — both copies — gets an "Unscored debrief" section the failure-class table links to from the exit-5 row. It says plainly that the fix for an array the model was never asked to fill belongs in the product, not in relaxing this check. No behaviour change: the assertion, its exit code and its class are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
_debug_flags_by_turn is documented as best-effort — "the debug drawer is a diagnostic, so a failure here must not turn a healthy smoke red" — and it honoured that for the transport (a 404 or a dead connection degrades to "the fallback check did not run") but not for the shape of the answer. The dict comprehension indexed t["turn_number"] unguarded, so a debug response whose turns key is absent-but-null, holds an entry without a turn number, or holds anything that is not an object raised KeyError/TypeError out of the helper, escaped to run_smoke's catch-all and reported an otherwise-green nightly as a harness bug with exit 5. That is the one verdict the issue's acceptance criterion cannot tolerate from a diagnostic: the model ran, the debrief scored, and the job goes red because a dev-only endpoint answered in a shape we did not expect. Treat an unreadable shape the way an unreachable endpoint is already treated: log what was skipped, return no flags, and let evaluate_turns warn that the check did not run — so an absent proof still never reads as a passed one. Four parametrised cases cover the shapes that used to raise (plus the empty body, which did not), asserting the run still exits 0 with the warning. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
"Debrief has no rubric dimension scores" is the failure this nightly is most likely to hit, and exit 5 alone does not say whether the model volunteered nothing to score or the debrief engine dropped what it was given. The remedies are opposites — one is a product gap to file, the other a bug to fix now — and the harness was sending the reader off to diff the previous nightly's artifact to tell them apart. The answer is already in the data the run collects: each turn response carries the validated rubric_observations the turn pipeline accepted, while the debrief engine re-derives its scores by re-parsing each NPC turn's *stored raw model output* with a plain json.loads. So observations present on the turns but absent from the debrief mean they were lost in between — output that needed repair, or that arrived pre-parsed from the runtime adapter, never reaches _parse_rubric_observations as valid JSON. Record rubric_observation_count per turn and the total as rubric_observations_seen, and let evaluate_debrief name the cause: zero gets the "nothing asks the model for them" note, above zero gets a regression verdict pointing at the debrief engine. Both still fail the run — a scored debrief is the #457 acceptance criterion — but the verdict no longer excuses a real bug or chases an imagined one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…per-PR The new doc justifies running this job nightly rather than per-PR by pointing at `tests/e2e/test_scripted_playthrough.py` as "the per-PR equivalent". No workflow runs tests/e2e — only scripts/release-smoke.sh does, at release time. So the sentence told a reader that the scripted conversation is covered on every pull request when nothing covers it until a release, which is the opposite of the reassurance it was offering. Name where that playthrough actually runs, and say what does run per-PR: this harness's own unit tests. Same correction in the script comment beside SCRIPTED_PLAYER_TURNS and in the test that guards the two scripts against drift. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The pre-run modes — the registry lookup, --download-only, --verify-only — write a step summary through the same renderer as a full run, but they keep no clock. Defaulting the missing timings to zero put "Wall clock: 0 s (budget 0 s)" at the top of every download and checksum verdict: a measurement that was never taken, in the first line a triager reads, on the two classes whose whole point is that they say nothing about how long anything took. Omit the line when there is no measurement to report. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
The latency table lists cold start, TTFT, TTS first-audio and STT round-trip, and the sentence above it now claims the nightly real-model smoke "flags any regression greater than 20 % against these values". It enforces none of them: cold start, TTS and STT are never exercised by that job, and TTFT is explicitly excluded by the harness because it needs the token-streaming gateway the smoke does not stand up. The one budget it does enforce — FULL_RESPONSE_MS, 10 s — was absent from the table entirely, even though the closing line promises the table mirrors LATENCY_BUDGETS. So linking the job made a false claim more specific and more credible instead of giving it a referent. Add the missing full-NPC-response row (the table now matches all five LATENCY_BUDGETS entries) and scope the sentence to it, saying plainly which budgets are hand-verified and why. Two tests keep it from drifting again: BUDGETS_MS is a hand-maintained copy of the TypeScript constant, so one asserts they still agree — a nightly failing against a stale budget is worse than no nightly — and the other asserts both copies of the performance doc still publish the figure the job scales, so the CI-coverage claim keeps a referent a reader can check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
The NPC can close the conversation itself — session_control.continue_session — and the harness correctly treats that as a pass: the product handled an early ending the way it is designed to, and the turns that did run were still real model output. But the job exists to play a *multi-turn* conversation, and a run that played one turn proves less than one that played three. Nothing said so: the step summary showed a green verdict, and only the per-turn array in the JSON artifact (or the log line nobody reads on a green run) revealed the conversation had been cut short. That is the one place this harness implied a proof it did not have, having been careful everywhere else — a missing parse flag warns, an unverified checksum warns. Record scripted_turns_played and warn with the ending_type when it is short of the script, so a green run on one turn cannot be mistaken for a green run on three. Documented in both copies of the real-model-smoke doc, and both the short and the full playthrough are now covered by orchestration tests. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
1 similar comment
|
Reviewed code and pushed fixes. |
The budget breakdown said its non-inference rows were measured. They were not: this job's own logs for the ten nightlies still in GitHub's retention disagree with every one of them by five to ten times. Measured, from those logs — the cold-cache rows from the one of the ten that missed the cache: set up job + checkout + setup-python ~10 s (doc said ~1 min) pip install x3 ~16 s (doc said ~2 min) model cache restore (2.3 GiB) ~25 s (no row at all) download + SHA-256 verify (2.3 GiB) ~9 s (doc said ~2 min + ~1 min) cache save (2.3 GiB) ~10 s (doc said ~1 min) model load + core start + session ~8 s (doc said ~0.5 min) So the claim that the steps before the smoke cost "up to ~9 min that the harness's own clock never sees" — the whole stated reason the harness budget is 20 min inside a 30 min job — is really ~1 min, and the ~15/~19 min totals carried about 5 min of phantom overhead on top of the two legitimately projected inference rows. The 20 min and 30 min ceilings are unchanged and still right; the reasoning printed next to them was false, and it is the reasoning the next person to tune them would start from. Two of the real numbers are counter-intuitive enough to state outright: a cache MISS (~9 s to fetch and hash the GGUF from Hugging Face) is cheaper than a cache HIT (~25 s to restore the same bytes from the Actions cache), so the cache saves the network and not the clock; and everything before the conversation together is ~1 min of a 30 min job, so the budget is inference and nothing else. The table now also carries the cache-restore row it was missing, and marks the one row that genuinely has no measurement — the harness's new re-verify before load — as bounded by the cold download row rather than measured. The inference rows stay projected, but from ten samples instead of one: 66/93/98/103/104/111/112/114/115/115 s for a single behavioral-interview turn. CI_HARDWARE_FACTOR's calibration prose quoted the top of that range (~116 s) as if it were typical; it now quotes the range and says the factor is set from the slow end deliberately. The 66 s run against the 115 s one is also the night-to-night spread a two-sample calibration could not have seen, which is the better argument for the 20x factor than any single measurement. Also attributes the replayed-opening evidence to its actual cause. All ten of those nightlies logged an NPC reply reciting the authored opening verbatim, which is what `replayed_opening` exists to catch — but the player turn they submitted was "Reply with exactly one sentence: Hello, I am ready.", an instruction to the model rather than anything an interview candidate says, and this harness replaces it with the three in-character SCRIPTED_PLAYER_TURNS the real starter model answered properly in local verification. Ten-for-ten with no cause attached reads as "the starter model always recites", which would have the first recital warning scrolled past as the documented status quo instead of read as new information. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
1 similar comment
|
Reviewed code and pushed fixes. |
The progress line is written with a leading "\r" and no newline, which a terminal collapses in place for free. CI has no terminal: it captures stdout through a pipe, so every explicit flush becomes its own log line. The one nightly still in retention that missed the model cache logged 2381 of them for a 2.3 GiB GGUF — and a `download` or `checksum` verdict is printed directly underneath that, which is the output a triager has to scroll past to reach the thing this job exists to tell them. Throttle to one line per PROGRESS_LOG_STEP_PCT (10 %). Eleven lines still show where a stalled transfer stopped; 2381 hide the banner they precede. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…onger exists The runtime-budget breakdown still labels the cache save a "post-step", which it was under `actions/cache`. This PR split restore and save precisely so the save could be gated on `success()`, and that made it an ordinary step that runs between the download and the smoke — two paragraphs below the table the doc explains the split and the gating. A reader checking the ~10 s row against a job timeline would go looking for a post step that is not there, and the ordering the gating depends on is the opposite of what "post-step" implies. The prose above the table has the same gap: it enumerates what runs before the harness as "checkout, three pip install steps and the model cache restore", which is the warm-cache job. A cold run also pays the download and the save before the harness starts, and the ~1 min total it claims is only true because both of those are counted. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
1 similar comment
|
Reviewed code and pushed fixes. |
…rral list ROADMAP.md marks #457 done, but the two documents that mirror it were left saying the opposite: * docs-site/.../project/roadmap.md is a line-for-line mirror of ROADMAP.md and is the copy the public docs site renders. It still showed the item unticked, so the published roadmap contradicted the repository's own. The #455 commit (6248a4a) updated both copies in one change; this one had updated only the repository copy. * post-alpha-issues.md item 6 is the deferral record for this very issue, and still presented it as deferred to Milestone 2 with the rationale "the fake runtime provides full structural coverage; real-model CI is a quality-of-life improvement" and no outcome. A reader of that page would conclude the nightly does not exist. It now carries a Status line in the same form items 2 and 3 use for shipped work, saying what the job does and pointing at the new doc — with the Milestone and Why-deferred lines left alone, since those are the historical record of the deferral rather than a claim about today. Docs only; no behaviour change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…erdict The download runs in its own step before the smoke, so it spends the job clock where the harness's Deadline cannot see it — and `urlopen`'s timeout is per socket read, not per transfer. A connection that went quiet repeatedly, or merely crawled, could therefore spend the whole ~9 min of margin between the harness's 20 min budget and the job's 30 min timeout and then hand the smoke a job clock with no room left in it. GitHub would cancel the job mid-conversation with no class, no phase and no remedy: the one unattributable outcome every other budget in this harness exists to replace, and the only route to it left open. The transfer now carries a 5 min budget (~33x the measured 9 s for 2.3 GiB, leaving ~4 min of the margin untouched) checked per chunk, with any single quiet stretch capped at 2 min. Exceeding either is exit 2 — `download`'s "nothing about the app changed, re-run the job" is the right advice for a stalled transfer — with its own remedy saying why the bound exists, and the partial file is still removed so the next run does not checksum-fail on it. Tests cover a crawling transfer (classified, own remedy, no partial file left), the read timeout sitting inside the overall budget, and the budget fitting inside the pre-smoke margin the workflow reserves. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
1 similar comment
|
Reviewed code and pushed fixes. |
|
Reviewed code, no issues found. |
verify_model_checksum normalises the expected digest's case and surrounding whitespace precisely so that a hand-pasted hash is not mistaken for drift — its docstring says why: a mismatch deletes a 2.5 GB file and reports that the pinned upstream file was replaced and that the check must not be relaxed, which is an expensive and thoroughly misleading verdict to hand someone who typed the argument wrong. A digest that is the wrong length, or not hex at all, is the same mistake and was getting exactly that treatment. Copying 64 hex characters out of a terminal by hand and losing one is the ordinary way to produce it, and the local repro in docs/real-model-smoke.md asks the reader to do precisely that copy. No file's hash can ever match a 63-character string, so the comparison is not a check; it just destroys the download and blames upstream. So the digest now has to be a SHA-256 digest before it is compared to one: 64 hex characters, checked after normalisation and before the file is hashed. A malformed one is `pipeline` (the class this harness uses for inputs it cannot act on), carries a remedy naming both places the value can come from, leaves the file where it was, and does not spend minutes hashing gigabytes against something that cannot match. Real drift is untouched: still exit 3, still deletes the bad bytes. Both copies of the doc say so, in the Checksum-drift section and next to `--verify-only`, which is the flag most likely to be pointed at a model the reader wants to keep. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
1 similar comment
|
Reviewed code and pushed fixes. |
|
Reviewed code, no issues found. |
verify_model_checksum rejects a malformed --model-sha256 without hashing
anything, but download_model reached it only after the transfer: a digest
copied one character short out of a terminal bought a 2.5 GB fetch and then
exit 5. The realistic source of that mistake is the local repro's hand-pasted
--model-sha256, which runs --download-only, so the reader waited out the whole
download to be told the argument was wrong -- and docs/real-model-smoke.md
already promises the opposite ("Nothing no file's hash could ever match is
allowed to cost a 2.5 GB download").
No bytes could have changed the verdict, so the shape guard moves into
_require_sha256_digest and now runs before the transfer as well as before the
hash. The failure text says so too: "Nothing was fetched" leads the message
that already said nothing was verified and nothing was deleted.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
…ayload `_debug_flags_by_turn` drops any /debug entry it cannot key by turn number, so the parse flags can cover some of the generated turns and not others. `evaluate_turns` only warned when *every* generated turn lacked them, which left the partial case silent — and not harmlessly: an absent `used_fallback` is falsy, so an unflagged turn counts towards "at least one generated turn was a real reply" with no evidence at all. With a payload that keyed turns 2 and 3 as fallbacks and dropped turn 1, the run went green reporting only "2 of 3 model-generated NPC turns fell back", with nothing anywhere in the report or step summary to say that the third turn — the one keeping the all-fell-back failure from firing — was never checked. The only trace was a print buried in the job log. The rubric path already refuses to round an unreadable turn down to zero (`_total_rubric_observations` returns None rather than 0 for exactly this situation); this is the same refusal applied to the fallback flags. All of them missing still reads as the whole check not running; some of them missing now names the count and the turns, and says the partial payload is itself worth a look at the turn response contract. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
…ll holds The runtime-budget table, the CI hardware factor and the recited-opening evidence were all sampled from ten nightlies described as "still in GitHub's log retention". Twelve are: the two oldest, 2026-09-22 and 2026-09-23, were left out, so a claim presented as the complete retained record was a subset of it. Re-read from the job API and the run logs for all twelve: - turn latency gains two samples (71 s and 94 s). The range 66-115 s is unchanged, so CI_HARDWARE_FACTOR 20 and the 240 s ceiling stand, but the median moves 108 -> 104 s and the slow end is ~11.5x the documented budget, not ~11.6x; - the recital check keeps its evidence: both added runs also logged an NPC reply whose leading characters were byte-identical to `opening_npc_says`, so the observation is twelve for twelve rather than ten for ten; - the per-step rows were measured against the step timings rather than estimated: set-up is ~5 s (not ~10 s), a warm pip install ~14 s, a cache restore ~24 s with a 19-28 s spread, and the one cold run's download ~10 s. The cache bullet also compared unlike things. "A cache miss (~9 s to fetch and hash) is cheaper than a cache hit (~25 s to restore)" dropped the ~10 s cache save that only a miss pays, inflating a real ~10 s gap into an apparent 16 s one; a reader tuning this would conclude the cache actively costs clock. It measures 31 s cold against 38-51 s warm, so the conclusion — the cache saves the network, not the clock, and abandoning a poisoned entry costs nothing — survives with the save counted. That measured 31-51 s pre-smoke total now replaces the rounded "~1 min" wherever the 9 min margin is justified against it. No behaviour change: docs, the workflow header and one harness docstring only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The orchestration tests fake out both children, so `_start_llama_server` and `_start_core` — the code that decides *what the nightly actually runs* — were the one part of the harness no test reached. Two of the env overrides they build fail silently when misspelled, because pydantic-settings ignores an unknown name and keeps the product default: - CONVSIM_RUNTIME_ID selects the real runtime over the deterministic fake. This one would at least be caught: the run checks /health afterwards. - CONVSIM_LLAMA_CPP_CHAT_TIMEOUT would not. Without it the adapter keeps its 180 s default, so the first turn slower than that is killed adapter-side and convsim-core answers 500 — class `runtime`, "the server crashed" — which is exactly the misclassification the override exists to prevent, on a night when the only thing wrong was slow inference. So the new tests assert the argv and the env, and then check every CONVSIM_* override the harness sets against the field names declared on ServiceConfig and LlamaCppConfig in the product source, which also catches a future override added under a name nothing reads. The data-directory assertion pins the privacy claim in docs/real-model-smoke.md: no database, log or pack outside the tree the finally block deletes. Also covered, all previously exercised only by hand: - `--download-only` succeeding — the workflow's own download step and step 3 of the documented local repro; - `--verify-only` accepting a matching file, and deleting a drifted one with exit 3, which is the behaviour the doc warns about; - a missing --model-id and a `--verify-only` without a digest exiting 5 rather than argparse's 2, which EXIT_CODES reads as `download`; - warnings reaching the step summary on a pass, since every warning the harness raises says what an otherwise-green verdict does not prove. Harness coverage 97% -> 99%; no production or harness behaviour changed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
Every transport-level failure is a `runtime` failure, so a *live* convsim-core that answered 5xx lands in the same class as a dead one — and `runtime`'s class remedy opens by telling the reader to read a child stderr tail, then names runner OOM and an incompatible llama-cpp-python wheel as the likely causes. When no child exited, that tail shows a healthy server and none of those causes applies. The reachable case is the one `_start_core` already half-mitigates. convsim-core answers 504 TURN_TIMEOUT when the adapter's generation budget runs out mid-reply, and the harness deliberately sets that budget above the latency ceiling so a slow turn is measured and reported as `budget` rather than killed and misreported as a crash. But that moves the cliff rather than removing it: at --ci-hardware-factor 20 the budget is 540 s against a 1200 s wall clock, so a turn past it is still cut off with the clock unspent, and neither `timeout` nor `budget` takes the class over. Exit 4 was then honest about the transport and wrong about the cause — the same misclassification the raised adapter timeout exists to prevent, one cliff further out. So when a `runtime` verdict finds both children still alive, print a remedy that says so: nothing crashed, a 504 means inference roughly 5× slower than any nightly has measured, start from measured_ms and phase_durations_s rather than an OOM — and otherwise read the tails for a server that is up but wedged. Only when the failure was using the class default, so the port-conflict failure keeps its own advice, which is right about a run where no child was ever started. The doc's failure table sent the reader to a stderr tail unconditionally; it now distinguishes the three cases and links a new section carrying the reasoning, with tests pinning that the link resolves and that the adapter budget quoted in both the doc and the remedy is the one the harness computes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
`_debug_flags_by_turn` drops any /debug entry it cannot key by turn
number, and `evaluate_turns` warns per turn that the fallback check did
not run for the turns left without flags — the whole point being that an
absent `used_fallback` is falsy, so an unflagged turn would otherwise
count towards "at least one generated turn was a real reply" on no
evidence.
An entry that *is* keyed but carries no `used_fallback` slipped straight
through that: the copy used `flag.get(...)`, which wrote False, and
`evaluate_turns` decides whether the check ran from the key's presence,
so the turn was both credited as a real reply and stripped of the warning
that says nothing checked it. Verified against the faked servers: a
payload of bare `{"turn_number": n}` entries produced `used_fallback:
false` on all three turns, zero warnings, exit 0 — a run reporting a
proof it never obtained. `used_native_structured_output` recorded the
same invented False into the report artifact.
Copy each flag only when the payload carries it, so a turn the endpoint
did not describe stays unflagged and the existing warning fires. Two
tests cover it: all flags missing, and an entry carrying `used_fallback`
alone, which must still prove the turn.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Reviewed code and pushed fixes. |
CI validates everything against the fake and scripted runtimes, so a regression that only appears under real inference — a prompt the model cannot satisfy, a schema the adapter mis-serialises, a debrief that generates prose but never a score — was invisible until someone played the app by hand. The existing
model-smoke-nightly.ymlscaffolding downloaded a real model but proved only that one NPC turn came back inside a latency budget, and when it failed, the output did not say whether the download broke, a server crashed, or the product rejected the model's output.What the nightly now does
tests/e2e/test_scripted_playthrough.py, so both cover the same shape), then session end and debrief generation. The run asserts a scored debrief: at least one rubric dimension scored, a numericoverall_scorein[0, 100], a non-empty summary.mainwhose logs GitHub still held on 2026-10-03 logged an NPC reply to the first player turn whose leading characters were byte-identical to the scenario's authoredopening_npc_says— the model reciting the question back.used_fallbackstays false for those (the utterance is neither empty nor the canned one), so the job went green on a conversation that never happened. Each turn now recordsreplayed_opening, under the same policy as the fallback check: one such turn warns, every generated turn doing it fails. Worth reading that twelve-for-twelve with its cause attached, which the doc now does: the player turn those runs submitted was"Reply with exactly one sentence: Hello, I am ready."— an instruction to the model, not something an interview candidate says — and this PR replaces it with in-character turns the real starter model answered properly. So the expected rate under the new script is zero, and the first recital warning is new information rather than the documented status quo.budget(1),download(2),checksum(3),runtime(4),pipeline(5),timeout(6). A crashed child process outranks the client-side symptom it caused, so a deadconvsim-corereads asruntime, notpipeline.--print-registry-model starterreplaces the inline Python that parsedregistry.yaml, so the cache key, download URL and expected checksum cannot drift apart.$GITHUB_STEP_SUMMARY, alongside the existing JSON report artifact.Runtime budget
timeout-minutesdrops 60 → 30, the target in the issue, with the per-phase breakdown recorded in the workflow header and in the new doc: ~11 min, warm cache or cold. The non-inference rows are measured from this job's own logs for the twelve nightlies whose logs GitHub still held on 2026-10-03 — and they are much smaller than they look: everything before the conversation measured 31–51 s of a 30 min job, and a cache miss costs no more than a cache hit (~10 s to fetch and hash the GGUF plus ~10 s to save it, against 19–28 s to restore the same bytes), so the cache saves the network, not the clock. The budget is inference and nothing else. The two inference rows are projected from the twelve single-turn latencies that job has measured — 66/71/93/94/98/103/104/111/112/114/115/115 s (median ~104 s) — rounded up to ~120 s per turn, and the doc says to replace them with realphase_durations_sfrom the first green nightly.actions/cacheis split intorestore+savewith the save gated onsuccess()— its post step saves even when the job failed, which would have written a GGUF that failed verification into the cache under a key derived from the expected hash, so every later run would restore the same bad bytes.MODEL_CACHE_PREFIXis the lever for abandoning a poisoned entry.The headline
full_response_msis now the median of the scripted turns, so one unlucky turn cannot flap the nightly. TheCI_HARDWARE_FACTORof 20 and its empirical calibration are unchanged.Two corrections to the previous harness
/startgenerates the NPC opening with a real model call. It does not — the opening is authored scenario text. Sosession_start_msis measured and reported but no longer budget-checked (the CI hardware factor does not apply to something that is not inference), and the fallback policy counts only model-generated turns; otherwise the "every turn fell back" check was unreachable.Deadline.finish()checked the wall-clock budget, so closing the books on an already-exhausted deadline raised out ofrun_smoke'sfinallyblock and discarded the classified verdict — a reportable timeout became a bare traceback. Found by the new tests; nowfinish()never raises.Docs
New
docs/real-model-smoke.md, mirrored to the docs site under Development: what the job proves, the failure-class table with the first thing to do for each, the runtime budget and its breakdown, how the CI hardware factor is calibrated, what checksum drift means (and why never to "fix" it by updating the expected hash), a copy-pasteable local repro, and the privacy posture. Both copies of the performance doc now link the job that actually enforces the budgets they state — the claim "nightly CI smoke tests flag any regression greater than 20 %" previously had no reachable referent. ROADMAP marks the Milestone-1 polish item done.Testing
tests/scripts/test_nightly_model_smoke.py(236 tests, ~3 s, no model/network/llama-server, 99 % harness coverage) runs per-PR in the acceptance job. Without it the harness is only ever exercised at 04:00 UTC, so a typo in it shows up as a mysteriously red nightly rather than a red PR. Coverage:downloadnotchecksum, a cache hit with drifted bytes still fails, and an expected digest that is not 64 hex characters is rejected aspipelinebefore the file is hashed — a digest mistyped into the local repro can never match any file, so treating it as drift deleted a 2.5 GB download under the "the pinned upstream file was replaced" remedy;starteris still the smallest registry model;run_smokeorchestration against faked-out servers: healthy pass, unscored debrief → exit 5, latency regression → exit 1, crashed child → exit 4, fake runtime refused, drift stops the run before any server starts, exhausted deadline → exit 6, early scenario ending, and an unexpected harness bug reported rather than raised;llama_cpp.serverargv, and everyCONVSIM_*override checked against the field namesServiceConfigandLlamaCppConfigactually declare, since pydantic silently ignores a misspelled one and keeps the product default (a mistypedCONVSIM_LLAMA_CPP_CHAT_TIMEOUTrestores the 180 s default, which turns a slow turn into the HTTP 500 the override exists to prevent);--download-onlyand--verify-onlysuccess paths, and the usage errors that must exit 5 rather than argparse's 2.Also verified:
actionlintclean across every workflow,--print-registry-model starteragainst the real registry, and all three checksum CLI paths (match / drift / missing) exercised by hand with their exit codes.The real-model path has now been exercised end-to-end against the real starter model (Qwen3 4B Q4_K_M, the registry pin) on a local llama.cpp
llama-server:convsim-coreon thellama_cppruntime played all three scripted turns at 6.6 / 7.4 / 8.3 s withused_fallback=falseandused_native_structured_output=trueon every turn, and returned a scored debrief (3–4 rubric dimensions,overall_score~49–50, 269-char summary, no fallback narrative) —run_smokeexit 0 with a correct report and step summary.--print-registry-model starterand the--verify-onlydrift path (exit 3, bad file deleted) were run by hand too. What that cannot cover is the runner itself — the 2.5 GB download, the cache restore/save, the CPU-only latencies behind--ci-hardware-factor 20and the 30 min job budget — so still worth triggering Run workflow on this branch before merge.Three acceptance tests fail in my local environment (
test_fake_runtime_is_used_by_default, two pack-validation tests), but they fail identically on a pristine checkout ofmain, so they are pre-existing and unrelated to this change. No product code is touched by this PR.Closes #457
🤖 Generated with Claude Code