Plan: agent eval suite (Gap #3), with ADR-01 on architecture - #47
Merged
Conversation
docs/37-AGENT-EVAL-PLAN.md. Three of gap #3's four claims do not survive contact with the code, and the fourth misses the thing that matters most. Not running in CI: zero references to `eval` under .github/. The only Python job runs `pytest tests/ -v --tb=short || true`, and tests/ does not exist — its own comment says so. Not guarding four toy cases: it guards nothing, and the cases are tautologies. echo_runner is the identity function and the case asserts output equals input; the obligations runner is a substring matcher and the case asserts the type it hardcodes. They cannot fail. EVAL_USE_HTTP covers /extract_obligations only; nothing in evals/ touches the agent chat path. And the audit missed scripts/persona-tests/ — an SSE parser that reassembles token deltas and joins tool_call_start to tool_call_result, multi-turn conversations on one sessionId, expectedTools/notHallucinated/ maxLatencyMs, 66 committed conversations plus ~86 persona asks against an 800-contract seeded fixture. The Python package has the schema, baseline and regression check but no agent loop; the JS one has the agent loop and the coverage but no schema, baseline or CI. The work is consolidation. Scouting also turned up two live defects. The persona suite passes provider/modelId to askAgent, which does not accept them and never puts them in the request body — so 66 conversations have run on org defaults while the report attributes their cost and latency to gpt-4.1-mini. Verified against lib.mjs:40-48. And nothing records which model answered: the done frame carries only session_id, while _platform_resolve picks the first provider with an env key — so adding a secret to the repo silently changes every eval result. A baseline without that is uninterpretable. Cost is the other trap. An eval run has no identity at any layer; the daily cap defaults to $50/day BLOCK and now fails closed, so a breach kills the run mid-suite and every later case misreports as "runner raised"; and running under a real customer org spends their BYOK key while still incrementing the platform counter. Revised estimate 3-4 weeks against the audit's 1-2, sequenced so the gate can fail before any case is written — fifty cases in a harness nothing runs is fifty more dead controls, which is what docs/36 just spent four waves removing.
Taking the architectural call rather than presenting options. E7 asked "which harness survives", which was the wrong question. The right one is what seam we test at, and how agent behaviour becomes deterministic enough to gate a PR on. Seam: the public HTTP API, not in-process Python. The argument is the defect record, not taste. Of everything docs/36 found -- the cross-tenant write, the VIEWER RBAC gap, the write-tool permission map, the cost cap failing open, the nine dead controls -- almost all live at or above the Node boundary. An in-process Python harness structurally cannot see RBAC, tenancy or the cost cap. A suite that cannot test the security-critical layer is not the suite this product needs. The HTTP API is also the seam the user experiences, so the eval measures the product rather than an implementation detail. Language: JavaScript, on grain rather than preference. The repo has 23 JS check scripts and zero Python tests; the fixture is TypeScript. Choosing JS costs one reimplementation of cli.py's 150-line baseline diff. Choosing Python costs the SSE parser, the multi-turn engine, the tool-call assertions and the 800-contract fixture -- all of which work today -- plus a Python test culture with no existing examples. Python eval libraries consume traces and datasets, not runners, and Langfuse is already wired, so that door stays open. The load-bearing decision is record/replay. Nondeterminism is why agent evals cannot gate a PR, and the usual answers are both bad: temperature=0 does not make tool-calling deterministic and is not how production runs, and loosening assertions until they stop discriminating defeats the point. Separate the two questions instead. "Does my code do the right thing given what the model said" -- tool dispatch, the confirm gate, RBAC, error surfacing, memory replay -- is deterministic, is most of the agent, and is where every bug in docs/36 actually lived. "Is what the model said any good" is the expensive noisy one. Record once, replay free on every PR, reserve real calls for nightly. This is cheap here because build_llm has exactly one caller, router.py:345 -- a single injection seam covering every LLM call in the system. That is the same seam E11 wanted for sampling, so E11 collapses into it. Three tiers: invariants and contract tests block every PR at zero cost and no API key; behavioural evals run nightly on main. E9's constraint -- no model key, public repo, no secrets on fork PRs -- stops being an obstacle and becomes the reason fork PRs get the same gate maintainers do. Risks accepted and written down: fixture staleness (T3 nightly is the tripwire), recording a bug as expected behaviour (the docs/36 rule applies -- a fixture that cannot produce a red is not evidence), and T2 being blind to prompt regressions by construction, which must be stated in the suite README so nobody reads a green T2 as "the prompt is fine".
The draft asserted "T1 exists already — it is the 16 agent-loops checks." Too clean. Those checks are a mix: some are pure static file analysis and genuinely free, while l1/l2/l9/l10/l12 drive real chat turns against a live stack and a real model, which makes them T3 by this plan's own definition. Classifying them is a Wave C task, not a given -- and a grep for login()/API} is not sufficient to do it, since it misfiles the Playwright check as static and l7-prompt-truth as stack-dependent. Also recorded, verified: ci.yml already stands up pgvector/pgvector:pg16 and redis:7-alpine as services for test-api, with DATABASE_URL and REDIS_URL wired. Stack-dependent checks in CI are not net-new infrastructure. What is missing is the agents service and a model key -- exactly the T2/T3 boundary, since replayed fixtures need neither.
docs/37 E1/E4/E5. The step this replaces was, verbatim, `pytest tests/ -v --tb=short || true` with apps/agents/tests/ not existing: three independent reasons it could never fail, and its own comment said it reported nothing. scripts/evals/run.mjs runs the checks named in manifest.mjs by tier and produces an exit code CI can gate on. Tiers are cost and determinism, not subject: t1 is static analysis with no services, no database and no model key; t2 needs the stack but makes no model call; t3 makes real calls and runs nightly. The 17 existing checks are classified by reading their imports and call sites -- a grep for login()/API} misfiles the Playwright check as static and l7-prompt-truth as stack-dependent. Two defects of the harness this supersedes are fixed by construction rather than patched later. A check that asserts NOTHING is a failure, not a pass -- the Python harness returned True with a warning that reached neither the failure count nor the exit code. And the baseline records each check's name AND assertion count, so a check that disappears or quietly asserts less is a regression; in the old harness, deleting a case produced "no regressions" and exit 0. Preconditions are probed, and an unmet one SKIPS loudly. A skip is never a pass: "could not check" and "checked and fine" must not share an exit code. e1-gate-bites.mjs watches the gate fail four ways against a fixture tree rather than by mutating live checks. The fourth is the one pass/fail alone cannot see: a check silently dropping from 2 assertions to 1 reports 0 failures and still exits 3. CI gains an `agent-evals` job -- its own name in the checks list and a node runtime, running t1 blocking on every PR. t1 needs no secret, which is what makes it safe to block fork PRs on; the public repo cannot give them secrets, so a key-gated suite would be green-and-meaningless there. The nightly t3 workflow is added but deliberately manual-only, with a guard that refuses to run without EVAL_ORG_ID. Enabling the schedule before E8 lands is how you bill a customer for your test suite: BYOK is returned before the cost cap is checked. Tier 1: 5 checks, 54 assertions, under a second.
docs/37 E2/E3, plus a new E13 recorded rather than guessed at. E2 — the done frame carried only session_id, so a flipped eval case could not be attributed to a prompt regression, a model swap or a key rotation. All four values already existed on ResolvedLlm and were thrown away; the frame now carries provider, model, tier and source. Found while fixing it: chat.py stamped req.provider and req.model_id -- the REQUESTED values -- over every frame, and spread them LAST, so they would have clobbered anything authoritative the orchestrator set. Pinning a provider with no key made the stream report that provider anyway. The decoration now fills in only where the event did not already say, and the done frame carries the genuinely resolved pair. Verified: pinning anthropic on a box with no anthropic key now reports google/gemini-2.5- flash rather than claiming anthropic answered. E3 — askAgent never destructured provider or modelId and never put them in the request body, so lib-multi.mjs's pin was silently discarded and 66 committed conversations ran on org defaults while persona-test-report.md attributed their cost and latency to gpt-4.1-mini. Both are now accepted and forwarded. E13 is recorded, NOT fixed. Probing four turns -- opus-reasoning, gpt-5-turbo, haiku, and no pin -- all resolve identically to tier=fast/gemini-2.5-flash, and the echoed request model_id is the service default in every case, so a client pin does not reach model selection at all. That may be correct: org AI settings arguably should win over a caller-supplied model. But the current behaviour is the worst of both -- accepted, echoed inaccurately, silently ignored. I am not asserting an expectation I have not established is intended; that is how a check comes to encode a wrong belief. It needs a product decision, and it blocks the audit's own stated motivation for this gap, since a suite that cannot pin a model cannot compare two. e2-model-observability.mjs 2/12 -> 13/13, registered as t3.
chat.py's provider auto-fallback overwrote req.model_id unconditionally
whenever it swapped provider:
if resolved_provider != req.provider:
req.model_id = model_for(resolved_provider, tier="smart")
DEFAULT_PROVIDER is anthropic. On a deployment holding a single provider
key -- the common case, and this workspace, which has a Google key only --
EVERY request takes that branch, so every model pin in the product was
discarded, including pins perfectly valid for the provider actually in
use. Because the model id is also what the orchestrator sniffs to choose a
tier, this destroyed the caller's tier signal too, not just the model.
Now it substitutes only when the requested model does not belong to the
resolved provider, which is the case the original comment was actually
about (claude-sonnet-4-6 against openai). Verified: gemini-2.5-flash used
to echo back as gemini-2.5-pro; it now echoes correctly.
The second layer is recorded, not changed. With the pin arriving intact,
both gemini-2.5-pro and gemini-2.5-flash still resolve to
gemini-2.5-flash at tier fast -- on an org with no OrgAiSettings row, so
that is the platform tier default rather than org config. Whether a caller
should be able to override it is a real product decision; org cost control
is a legitimate reason to say no. I am not asserting an expectation I
cannot establish is intended. It does block model-comparison evals, which
is the audit's stated motivation for this gap.
Also points the nightly t3 workflow at GOOGLE_API_KEY/GEMINI_API_KEY
rather than a secret that does not exist. Listed explicitly rather than
passing every provider, because _platform_resolve picks the first provider
with a key -- quietly adding a second would change every eval result, and
that should be a deliberate edit.
e2-model-observability.mjs 16/16. Tier 1 still 5 checks, 54 assertions.
docs/37 E12, the load-bearing choice in ADR-01. apps/agents/app/replay.py records real model responses and serves them back deterministically. Verified: a recorded turn replays identically three times in 6-8ms with EVERY model API key unset, and a missing fixture fails loudly naming the expected path rather than quietly falling back to a live call -- the one thing this module must never do, since it would turn a free deterministic PR gate into an unpredictable bill. Keyed on (session_id, call_index), NOT a hash of the messages. Hashing was the obvious design and is wrong: the system prompt is in every message list, so editing one line of a 240-line prompt would invalidate every fixture and force a full re-record, making replay annoying enough that people stop using it. Call-order keying means a prompt edit invalidates nothing -- consistent with ADR-01, where tier 2 is deliberately blind to prompt regressions -- while still catching the code making a different number or order of model calls, which is a real behavioural change. Callers key by choosing a stable sessionId; no new plumbing, because session_id already reaches the router as thread_id. A design error worth recording. The seam was first placed at build_llm -- the single chokepoint, which looked obviously right. Too deep: with no API key the service raises "No LLM API key found" before the router is consulted, so replay still required a key, defeating the point of a tier that runs free and keyless on fork PRs. Caught by blanking every key and watching a replay run 500. It now short-circuits in resolve_llm above provider and key resolution, and chat.py skips provider validation under replay. NOT working and recorded as such: recording a TOOL-CALLING turn captures empty content and no tool calls. ReplayChatModel is correct in isolation -- it loads the fixture, streams O/K, and raises on a missing one -- so the fault is in RecordingChatModel._astream, most likely that bind_tools on the inner model returns a RunnableBinding whose streamed tool-call chunks are not merged the way _capture expects. Replay therefore covers prose-only turns today, which is a small fraction of what tier 2 needs, since tool dispatch is the main thing worth testing deterministically. The seam is inert unless AGENT_REPLAY_MODE is set; production is untouched. Tier 1 still 5 checks, 54 assertions, no regressions.
The previous commit reported tool-call recording as broken and blamed RecordingChatModel._astream. That was wrong, and diagnosed from a single sample. Probing the recording chain directly showed it captures tool calls correctly, and re-recording end to end produced a clean two-call fixture: call 0 with two contract_search invocations, call 1 with the prose answer. The original empty fixture was the model declining to call a tool for that prompt -- a model response, not a defect. Diagnosing from one observation is how a plan acquires a false claim of its own, which is the same failure this whole suite exists to prevent. Tool-call replay verified: a replayed turn dispatches its recorded tools in 34ms with no API key, and the tools GENUINELY EXECUTE -- contract_search hits the real database and returns real rows. Only the model is replaced. That is the seam that makes tool dispatch, the confirm gate and RBAC testable without a model, which is the point of tier 2. /health now advertises replayMode and the runner treats it as a precondition, so e12-replay SKIPS when replay is off rather than silently running against a live model -- which would burn quota and vary run to run while reporting as a free deterministic gate. Verified both ways. The tier gate immediately earned its keep: l2-redline-propose was filed t2 because it never calls /agent/chat, but it reaches a model indirectly through the redline_propose tool and produced a 502 under replay. Moved to t3. Indirect model dependencies are the classification trap here. e12-replay.mjs 15/15. Tier 1: 5 checks, 54 assertions, sub-second, keyless. Tier 2: 5 checks, 75 assertions, ~35s, keyless.
Wave C. Python evals/ is deleted -- runner.py, cli.py, the four tautological cases, baseline.json, graders -- along with p75-5-verify.mjs (which proved regression detection by a tautology and overwrote the tracked baseline as a side effect), scripts/eval.py and smoke_d09.py, a meta-test of the harness now retired. Both persona suites are registered in the manifest and run under the one runner, which needed only a `dir`/`entry` field because a suite and a check report the same summary line. Cases deliberately stayed as executable checks rather than becoming YAML. The runner needs a summary line, not a schema, and rewriting 17 working revert-verified checks into a case format would have been churn for no assertion gained. ADR-01 asked for consolidation, not uniformity. The persona suites need their own seeded users, so without them they failed with "Invalid email or password" -- an environment gap that reads exactly like a product defect. Added a `personas` precondition; they now SKIP loudly. Wave D. seed-eval-org.mjs creates a dedicated eval org with an explicit OrgAiSettings row, because absent means the $50/day BLOCK default applies -- a fresh org does not opt out of the cap, it opts in. Policy is `warn` not `block`: since docs/36 L11 the cap fails closed, so a breach would kill the suite mid-run and every case after it would misreport as "runner raised" rather than as a model regression, blaming the wrong thing. And the invariant that actually matters -- no OrgAiKey, ever -- because BYOK is returned before the cost cap is checked, so an eval under a customer org would spend their provider account with nothing able to stop it. Also adds scripts/evals/README.md, which states plainly what a green tier 2 does NOT mean: it replays the model, so it is blind to prompt regressions by construction. Nobody should read it as "the prompt is fine". Tier 2 CI wiring is left as a TODO in ci.yml rather than silently omitted -- an eval tier nobody runs is the exact failure this suite was built to end. t1 5 checks / 54 assertions. t2 6 checks / 84 assertions. t3 9 checks running, 2 suites skipping on fixtures. e8 9/9, e12 15/15, e1 13/13.
I asserted tier 1 needs "no services, no database, no model key, $0". CI disagreed on the first run: all five tier-1 checks died with a module resolution error on a clean checkout, 0/5. Cause: every check imports the shared harness, and harness.mjs statically imported PrismaClient from apps/api/node_modules. So merely importing the harness pulled in a generated Prisma client, and the tier that was supposed to run on nothing in fact required `pnpm install` plus `prisma generate`. The claim was in a comment; nothing enforced it. Fixed at the root rather than by adding an install step to the job, which would have made the claim false-but-passing. Prisma is now required lazily inside db() via createRequire -- createRequire rather than a dynamic import because db() is synchronous and many callers depend on that. Verified the way this session has learned to verify: physically moved apps/api/node_modules/@prisma/client/index.js aside and ran tier 1. All 57 assertions passed with no Prisma on disk, then restored. Asserting it in a comment is what produced the bug in the first place. e1-gate-bites gains a section pinning the property: the harness must not import Prisma at module load, it must require it lazily inside db(), and every tier-1 entry must declare zero preconditions. 16/16. This is the gate doing exactly what it was built for, on its first encounter with real infrastructure -- catching a claim I had verified only on a machine where the claim happened to be true.
Second CI failure, two more false claims of mine, both invisible locally. THIRTEEN checks hardcoded /Users/temp/Documents/Code/draft-legal -- the author's machine. They passed here and died everywhere else. Now derived from import.meta.url. l5-redline-reach shells out to apps/agents/.venv/bin/python, which no clean checkout has, so it was never a tier-1 check however static its assertions looked. Reclassified to t2 behind a new `venv` precondition. A build artifact is as absent as a service on a fresh clone. The runner was also hiding the cause. It kept the LAST 300 chars of failed output -- stack frames -- so three different root causes all reported "...tracePromise.__proto__ (node:internal/modules/esm/loader:681:26)", which diagnosed nothing. It now extracts the error message. Stopped guessing and built the missing feedback loop: `git archive HEAD` into a temp dir and run the suite there. No node_modules, different absolute path -- what CI actually does, and what a local run structurally cannot tell you. That reproduced both failures in seconds instead of a push-and-wait cycle each. e1-gate-bites gains two assertions pinning both lessons, and both were WRONG on the first attempt in the same way everything else has been this session: the absolute-path scan flagged this file because the comment above it quotes the offending path verbatim, and the venv scan flagged it because a regex must contain the pattern it searches for. Fixed by stripping comments and by one named self-exclusion, rather than by loosening the assertion until it stopped discriminating. Clean-checkout simulation: 4 checks, 46 assertions, no regressions, no node_modules, no Prisma, no venv, no keys.
The tool that should have existed before the gate did. This suite's first two CI runs were red for three reasons a local run structurally cannot surface -- a static Prisma import in the shared harness, thirteen checks hardcoding /Users/<someone>/..., and a "tier 1" check shelling out to the Python venv -- and each cost a push-and-wait cycle to find. git archive HEAD into a temp dir reproduces all three in seconds: tracked files only, no node_modules, no .venv, a different absolute path. Exactly what a fresh clone gets. --worktree overlays uncommitted edits so a fix can be checked before it is committed, and the exit code is the suite's own, so it works as a pre-push hook. A local pass does not predict CI, and now nobody has to learn that the expensive way twice.
The highest-value remaining work is not new cases. Replay makes it possible to move the checks that already guard real, once-live defects from t3 to t2 -- the VIEWER RBAC gate, approval forgery, thread poisoning, the cost cap -- so they gate every pull request instead of running nightly. A tenancy regression merged at 2pm and caught at 3am is a bad trade when the alternative is free and deterministic. The conversion is mechanical: stable sessionId, record once, move tier. Step four is the one that will be skipped and the only one that matters: re-break what the check guards and watch it go red under replay. Replay makes a check cheap to run, not automatically correct -- a fixture recorded from a turn where the model never exercised the guarded path will pass forever while asserting nothing, which is the assertion-free case E4 closed, one level up.
Wrote that giving a check a stable sessionId "is usually a one-line change", then checked l4-draft-gate before starting the conversion: it has THREE model-touching paths, not one -- the chat turn at :60 plus sections 3-6 driving POST /agent/draft, which reaches a model by a different route with its own session handling. Half-converting it would have left a security check in a worse state than nightly. Names l1-thread-poisoning and l11-cost-cap as the genuinely single-path candidates to convert first, and tells the reader to count the paths before estimating. An estimate written from one example is how this plan's source document came to be wrong in three of four claims.
E13 — pins are honoured. resolve_llm has accepted provider_override and model_override since it was written, and orchestrator.py's own docstring says "Provider + model are passed per-request so the user can switch live". The call site simply never passed them, so the product's stated model-switching feature did nothing at all. Now wired. Verified: gemini-2.5-flash resolves to flash, gemini-2.5-pro to pro, no pin to the config default. The objection is cost -- any caller could pick the most expensive model. But the daily cap is already the control for that and, since docs/36 L11, fails closed. Locking the model would be a second, weaker control over the same risk, priced at killing model comparison, which is this gap's stated motivation. If per-role model limits are ever wanted they belong in the permission layer, not in a silent override that also lies about which model answered. e2-model-observability now asserts it rather than deliberately declining to: two different pins must produce two different models, and an unpinned turn must still resolve from config. 16/16 -> 18/18. Tier-2 CI — yes, and it is the highest-value infrastructure item left, but it is a job to build, not a line to add. Tier 2 holds l4-draft-tenancy, which guards the cross-tenant write that was LIVE in production; it should gate every PR. It needs Postgres and Redis (proven in test-api), plus the API booted and healthy, plus the agents service in replay mode, plus fixtures. Sequenced after the first t3->t2 promotion, because that is what makes the fixtures load-bearing -- building the job first means debugging service orchestration against checks that do not need it yet. A flaky gate is worse than an honest TODO, because people learn to re-run it rather than read it.
Pre-merge near-miss. SideAgentRail.tsx:494 and AgentHomePage.tsx:493 both send provider:'openai', modelId:'gpt-4.1-mini'. Before E13 that pin was ignored, so a deployment holding only a Google key worked by accident. Honouring pins could therefore have broken every agent turn in the web UI on any single-provider deployment -- including this one. It does not: chat.py's auto-fallback normalises BOTH provider and model to something valid before the orchestrator is reached, so the override only ever forwards already-valid values. Verified by replaying the exact web-client request: 200, three token frames, resolved google/gemini-2.5-pro, no error. That ordering is now load-bearing and nothing asserted it. Three assertions added, sending byte-for-byte what the shipped clients send: a pin for an unconfigured provider must not error, must fall back to a configured provider, and must still produce an answer. I nearly merged a behaviour change whose blast radius I had not checked against the actual callers. The check is the cheap part; looking was the part that mattered. e2-model-observability 18/18 -> 21/21.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Planning only — no product code. docs/37-AGENT-EVAL-PLAN.md plus a correction to
docs/33.Three of gap #3's four claims don't survive contact with the code
evalreferences under.github/. The only Python job runspytest tests/ … || true, andtests/doesn't exist — its own comment says so.echo_runneris the identity function and the case asserts output equals input. The obligations runner is a substring matcher asserting the type it hardcodes. They cannot fail.EVAL_USE_HTTPplumbing exists"/extract_obligations. Nothing inevals/touches the agent chat path.scripts/persona-tests/— SSE parser, multi-turn, tool assertions, 66 committed conversations against an 800-contract fixture.Two live defects found while scouting
The persona suite has never run on the model it reports.
lib-multi.mjspassesprovider/modelIdtoaskAgent, which doesn't accept them and never puts them in the body. 66 conversations ran on org defaults while the report attributes their cost and latency togpt-4.1-mini.Nothing records which model answered. The
doneframe carries onlysession_id, and_platform_resolvepicks the first provider with an env key — so adding a secret to the repo silently changes every eval result.ADR-01 — the architectural call
One suite, three tiers, JavaScript, with record/replay.
docs/36bug lived) from "is what the model said good" (expensive, noisy). Record once, replay free on every PR, real calls nightly. Cheap becausebuild_llmhas exactly one caller.Risks accepted and written down, including that T2 is blind to prompt regressions by construction.
Revised estimate: 3–4 weeks, not 1–2
Sequenced so the gate can fail before any case is written — fifty cases in a harness nothing runs is fifty more dead controls.