A golden run gets a report: both statements side by side, no rows, no network - #242
Open
vishalkalbi27 wants to merge 50 commits into
Open
A golden run gets a report: both statements side by side, no rows, no network#242vishalkalbi27 wants to merge 50 commits into
vishalkalbi27 wants to merge 50 commits into
Conversation
vishalkalbi27
requested review from
ashwin-agami and
sandeep-agami
as code owners
August 26, 2026 15:17
There was a problem hiding this comment.
Pull request overview
Adds a new “golden run” HTML report surface to the agami eval flow, so failures can be debugged by reading the confirmed (golden) statement and generated statement side-by-side in a self-contained, no-network artifact.
Changes:
- Introduces a stdlib-only renderer (
render_golden_run.py) + new HTML template to render a golden run report without embedding result rows. - Extends
run_golden_eval.pyto write both the JSON artifact and the HTML report (same timestamped stem) and to return both paths in the printed payload. - Adds targeted unit/e2e tests to pin the report’s invariants (no network, both statements present, no rows; plus markup pinning for JS-driven DOM).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_render_golden_run.py | New test suite pinning report payload/projection rules, placeholder substitution order, and template markup callsites. |
| tests/test_plugin_lib_resolution.py | Ensures the new renderer imports in marketplace layout. |
| tests/test_golden_eval_e2e.py | E2E assertion that a real run writes a report containing both statements and omits row-related fields. |
| tests/test_ah106_eval_skill.py | Updates eval-skill expectations to include report and asserts artifact/report write-failure behavior. |
| plugins/agami/skills/agami-eval/SKILL.md | Updates skill instructions to point users at the HTML report (and JSON beside it). |
| plugins/agami/shared/golden-run-template.html | New standalone, JS-rendered report template with no subresource loads. |
| plugins/agami/scripts/run_golden_eval.py | Writes report alongside artifact, adds joined per-item fields needed by the report, and emits both paths. |
| plugins/agami/scripts/render_golden_run.py | New stdlib-only renderer projecting run JSON into a safe embedded payload for the template. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+387
to
+389
| def test_the_table_set_delta_is_rendered_above_the_statements(): | ||
| """"Generated read `orders`, the answer key read `customers`" is usually the whole finding, so | ||
| it is one line above the two statements rather than something to be spotted in them.""" |
Comment on lines
+159
to
+165
| summary = run.get("summary") or {} | ||
| return { | ||
| **{name: summary.get(name) for name in _SUMMARY_COUNTS}, | ||
| "completed": bool(summary.get("completed")), | ||
| "sections": summary.get("sections") or {}, | ||
| "verified": any(item.get("confirmed") for item in items), | ||
| } |
Comment on lines
+128
to
+145
| score = item.get("score") or {} | ||
| accuracy = score.get("accuracy") | ||
| return { | ||
| "item_key": item.get("item_key", ""), | ||
| "question": item.get("question", ""), | ||
| "section": item.get("section", ""), | ||
| "confirmed": bool(item.get("confirmed")), | ||
| "passed": bool(item.get("passed")), | ||
| "gated": bool(item.get("gated")), | ||
| "status": score.get("status", ""), | ||
| # None means nothing was scored and 0.0 is a score an item earned, so the two are kept | ||
| # apart here as carefully as they are where they were decided. | ||
| "accuracy": None if accuracy is None else _shown(accuracy), | ||
| "reason": score.get("reason", ""), | ||
| "expected_sql": item.get("expected_sql", ""), | ||
| "generated_sql": item.get("generated_sql", ""), | ||
| "tables": _tables_claim(item), | ||
| } |
Comment on lines
+105
to
+109
| claims = (item.get("claims") or {}).get("claims") or [] | ||
| if not claims: | ||
| return None | ||
| claim = claims[0] | ||
| return { |
vishalkalbi27
force-pushed
the
AH-106-agami-eval-skill
branch
from
August 27, 2026 07:40
b447cf3 to
9ee3bc2
Compare
vishalkalbi27
force-pushed
the
AH-107-golden-run-report
branch
from
August 27, 2026 07:40
21b9295 to
8f6b47e
Compare
A golden dataset is a file of questions whose answer is already agreed — the question, the SQL the author accepts as the answer key, and how strictly a run has to match it. This adds the reader for them, at <artifacts_dir>/<profile>/golden_datasets/*.yaml. Two properties the reader is built around: * Fault isolation. One unparseable file, or one malformed case inside an otherwise good file, costs that file or that case and nothing else. Every drop comes back as a finding, so a typo is loud rather than a case that quietly stopped running. * No path escapes. Nothing returned carries a filesystem path, so a downstream runner cannot forward a dataset location into a subprocess — the records are self-sufficient by construction rather than by the runner's good manners. The filename is the dataset's identity: the reader injects it and a file that declares its own `name:` is refused, because two places to say it would disagree with nothing on disk to settle which won. `sql_confirmed` is the one field with no default, and a confirmed case with no SQL is refused: it is the one kind of case that can gate a run, so it is the one kind that must be able to fail one. Findings reuse validator.py's Finding/ValidationResult, and the models derive models.py::_Base, so `extra="forbid"` is what names a near-miss field rather than dropping it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… does not
A question asked relative to today ("last quarter", "past 30 days") over an
answer key pinned to fixed dates is a rotted case: the window the question names
slides forward and the SQL it is scored against stays where the author left it.
Today that shows up as a model failure, which blames the wrong side — the item
is broken, not the answer.
So the reader now lints for it and reports
`golden_relative_question_frozen_sql` naming the file and the case. Unlike the
refusals around it this does NOT drop the item: dropping would hide a dataset
fault behind a shrinking suite, so the case still reads and still comes back,
and the fault travels as a finding.
The rule is "relative iff anchored to now": the question matches a relative
phrase, the statement carries a date literal, and no CURRENT_DATE / NOW /
SYSDATE / GETDATE / 'now' anchor appears. The anchor set is the "what is now"
functions only — INTERVAL, DATEADD, DATE_SUB and DATE_TRUNC are arithmetic and
are relative only when their own anchor is, which already matches on its own.
DATE_TRUNC in particular would suppress the lint on exactly the frozen shape it
exists to catch.
Regex, not a parser: the question side is not SQL at all, and sql_guard.py
already sets the house rule that this kind of textual check does not bring a
second parser with it. An item with no expected.sql is skipped in silence —
nothing to inspect, and an unconfirmed case with no answer key is legal.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Documents the shape the reader accepts, so an author never has to glob a sibling profile to find out — which here would read another tenant's questions together with the SQL that answers them. The doc's example is parsed by the real reader in a test, so the reference cannot drift from the parser that has to accept it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The reader promised fault isolation and path-free records; three ways in it kept neither. A non-list `test_cases` and a non-string YAML root key each raised out of `load_golden_datasets`, so one bad file cost every other file in the directory — both are now refused as `golden_invalid_dataset` with the file skipped. `OSError` was interpolated whole, carrying the absolute path (and in a hosted deployment the tenant) into a finding; only the error class and its `strerror` survive now. Pydantic's default rendering carried `input_value=` — the author's SQL, filters and recorded rows — into a finding that any caller may forward to a log; findings now name the field, the reason and the rule and nothing else. The relativity lint treated any bare 1900-2099 integer as a frozen date, so `LIMIT 2000` earned a correct file an error-severity finding. That branch is gone; the quoted-ISO-date branch already catches the shape the reference teaches. The other direction was wrong too: bare `NOW`/`TODAY`/`GETDATE` matched a comment or a CTE and *suppressed* the lint, so the function-style anchors now need their call paren. The authoring reference said `must_filter` takes predicates where the contract takes column names — every file authored from it would have failed the gate. It also promised that a typo costs one case, which holds for a case but not for a key at the top of the file, where the whole file is lost; and it never mentioned that a repeated id silently drops a case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`match: bounded` was writable and inert: the item model forbids unknown keys, so there was nowhere to author the band the level compares against, and a case that asked for it silently got nothing. `bounds` is that band — `min_rows`/`max_rows` on the row count and `min_value`/`max_value` on a single-cell answer, every edge optional so a case that only cares the answer is not zero can say so without inventing a ceiling it never checked. The two halves are refused apart. `bounded` with no band has nothing to compare against, and a band under any other level is read by nothing; both keep passing, so both are the silent hole `extra="forbid"` exists to close. A band that names no edge, a negative row count, and a floor above its own ceiling are refused for the same reason — each reads as a model fault to whoever reads the run rather than as the authoring mistake it is. No refusal names a value: the band sits on a case that also carries the answer key, and a finding travels wherever its result does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The relativity lint reads a quoted ISO date as the pinned day it is, but the pattern stopped at an optional time — and the closing quote is what ends the match. So `'2024-01-01T00:00:00Z'` and `'2024-01-01 00:00:00+00'` matched nothing at all, and the finding went missing on an answer key pinned to a moment as hard as any the lint does catch. Silence in the one direction that costs something: the case rots, the run blames the model, and nobody is told the file is the fault. The zone joins the pattern rather than being trimmed off the end of it — a UTC designator or an offset, in the two-digit and four-digit spellings a warehouse actually emits. The discipline that keeps this lint at error severity is untouched: still no bare four-digit integer, so `LIMIT 2000` and `total_amount > 1999` stay silent, and the existing tests for both still hold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…an be compared
Raw cells cannot be compared. True == 1 == 1.0 == Decimal(1) and all four hash
alike, so a Counter reads a boolean column as an integer column of zeros and
ones; float('nan') != itself yet dict's identity fast-path collapses the same
NaN object anyway, making a row count depend on driver object reuse; and
Decimal('0.1') != 0.1, so one number read through two drivers disagrees.
Every cell becomes a hashable (type_tag, value) key instead. Numbers normalise
through an explicit decimal context (the ambient one is process-global and any
caller can narrow it), a float goes through Decimal(repr(x)) rather than
Decimal(x), dates canonicalise to ISO text with an aware value read as an
instant in UTC, and text is left exactly as it came — folding it would hide the
difference a comparator exists to find.
Only the canonicalisation layer; comparison and scoring sit on top of it later.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… they share Three steps on top of the canonical cell keys, kept separate because each is wrong in its own way. Is the result ordered? Read off the top-level node's own `order` argument and never with a search for an Order anywhere in the tree: a subquery's ORDER BY, a CTE's, an OVER (ORDER BY ...) and an array_agg(x ORDER BY x) all order something other than the result, and a search finds every one of them. The union cases come out right in both directions for free — an ORDER BY after a UNION hangs off the Union node and is a total order, one inside a single arm is not. The dialect is threaded through because a generic parse of a backtick-quoting statement raises rather than merely losing detail. A statement that is missing, empty or unparseable is read as ORDERED: the permissive reading would silently stop checking an ordering the author asked for, and a visible false failure is recoverable where a silent weakening is not. Which column answers which? By VALUES only — never by name, never by position. A generated statement that aliases the total and selects it second still answered the question, and one that reused the golden name for a different value did not. Pairing is an augmenting-path matching so that columns carrying identical values cannot collapse onto one partner. How far do the rows agree? As a multiset unless the author ordered them: a row returned twice where the answer key has it once is a different answer, usually a join that fanned out, and a set comparison is exactly the one that hides it. A row narrower than its column list — which ExecResult never validates — is raised as this module's own RaggedRow, so the caller can report the case rather than field an IndexError from a projection. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`compare_result_sets` is the way in: it reads whether the answer key ordered its rows, dispatches to the level the author asked for, and hands back a frozen `ItemScore`. It is total — a ragged row, a malformed result, or a band that cannot be applied come back with an error status rather than as an exception, so one bad item costs that item and not the run. The score carries verdicts, row counts and column names. Never a cell and never the answer key's SQL: the payload being judged here is result data, and a score travels further than the run that produced it. Two empty results are UNSCORED rather than a pass — they agree about nothing, and calling that a full match is how a statement returning nothing gates like a right one. And accuracy is capped just short of the 1.0 pass mark unless the overlap is complete: rounding to three places alone would hand a pass to 4002 of 4004 rows. Also replaces the augmenting-path column matching with a greedy scan. Columns pair on EQUALITY of their value vectors, and equality is transitive, so the candidates are equivalence classes and partners inside one are interchangeable — there is no augmenting path to find. The matching tests are unchanged and still pass.
A review of the scoring call found six places where it reported a verdict weaker, or plainer wrong, than the one it had actually reached: * A top-level ORDER BY was read off `args['order']` of whatever node parsed out. A trailing comment, a leading SET, a parenthesised statement or an EXPLAIN all parse to nodes that carry no such argument, so the answer key's ordering was silently dropped and the item scored a full 1.0 on reversed rows with no note. A parenthesised statement is now unwrapped, and a root that cannot carry an order takes the assumed-ordered path the module's own comment demands. * An unknown dialect raises ValueError and a deeply nested statement a RecursionError, neither of them a SqlglotError, and that read happens outside the totality net — so a call documented as never raising did. * The both-empty guard ran before the level dispatch, which dropped exactly the failure `nonempty` and `bounded` exist to catch: those levels have no answer key by design, so their golden side is legitimately empty. * `values` rounded whole numbers to nine significant digits, so any id or count above ~1e9 compared equal to its neighbours. Whole numbers carry no floating-point tail to forgive and are no longer bucketed; the comment and docstring now say bucket rather than tolerance, which is what it is. * A row-count difference was laundered through the unmatched-column branch and reported as "no generated column carries the values of: id" — the string a person reads for the most common regression there is. It is checked before any column is paired now, and names rows. * A band with both a row half and a value half could never reach the row half: a result the value band could not be applied to short-circuited as an error, leaving the item permanently unjudgeable. It falls through to the row band, and an empty result scores 0.0 rather than erroring. Also: the accuracy is the raw share rather than a rounded-then-capped one (three decimals belong to the report renderer), `__all__` names only the two symbols that are the interface, and the value band's inclusive edges are pinned by tests that a `<`/`<=` mutation now fails. The tests' zero-padded identifier fixture was a real deployment's code carried over from the comparator this ports; it is synthetic now. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vishalkalbi27
force-pushed
the
AH-107-golden-run-report
branch
2 times, most recently
from
August 27, 2026 13:30
ee7e4fc to
8045a86
Compare
vishalkalbi27
force-pushed
the
AH-106-agami-eval-skill
branch
from
August 27, 2026 13:50
9ee3bc2 to
39578e9
Compare
vishalkalbi27
force-pushed
the
AH-107-golden-run-report
branch
from
August 27, 2026 13:50
8045a86 to
6cf5d14
Compare
… context is named Three reason strings dropped the verb between the two numbers they compare, so a report read "the answer key has 3 rows and the generated result 2" and a fourth said "out of 3 the generated statement returned" — which is the harder half of a diagnostic to parse in exactly the moment someone is parsing it under pressure. The decimal change is smaller than it looks and is recorded as such. `to_integral_value` is exempt from the Inexact and Rounded traps and no rounding mode changes whether a non-integral value differs from its integral form, so nothing observable turned on the ambient context here. It is named anyway because every other decimal operation in this module names one, and the module's whole promise is independence from how a number arrived. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ot false-positive
…laim, not a crash A statement writing LIMIT 1.5, LIMIT 1e3 or EXTRACT(YEAR FROM d) = 2025.5 parses, and reading it raised ValueError straight out of read_claims — whose whole contract is that it never does, because the caller is an eval run that must survive whatever a generator emits. A non-integral literal is now a shape the module declines to compare, which is the None every other unmodelled shape already reads as. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…changes A dataset on disk, the shipped sample store database, real rows through the one chokepoint, and the comparators reading what SQLite actually returned. Only the model is stubbed. The unit tests assert each seam alone and would still pass if two of them disagreed about the shape they exchange; this one fails instead. And the deliberate half: the privacy test's docstring promised that no shipped script makes a network call, and the privacy doc promised there was no network call anywhere. Starting the operator's own client as a child process makes both false in spirit. Nothing went red — the forbidden list has no subprocess entry and should not gain one — which is why the wording is corrected here on purpose. Both now state the real posture: no in-process network client, and one out-of-process invocation of the user's own client, on an explicit eval command. The assertion and the forbidden list are untouched. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Ids get reallocated; the comment says what the run has to survive instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…by flag The eval's child was started as `claude -p` and nothing else, and the module said that gave it "no tools, no MCP configuration". In this client the absence of a flag is the permissive default, not deny-all: omitting `--tools` hands the child every built-in tool, omitting `--strict-mcp-config` loads the operator's own MCP servers, and the default setting sources read the launch directory's `CLAUDE.md`, `.claude/settings.json` and `.mcp.json`. `HOME` is on the environment allowlist, so that was the whole route to the answer key: the artifacts pointer under `~/.config`, the artifacts directory it names, the dataset carrying `expected.sql` and its recorded rows, and the credentials file beside them. The allowlist withheld the NAME of the path while shipping the root it is computed from. Each default now gets a flag that closes it, and the child starts in a temporary empty directory rather than wherever the eval was launched from. The test that asserted the argument list carried no `--` argument asserted the bug; it now asserts the three flags and the pinned working directory instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A `bounded` or `nonempty` item judges the generated result on its own terms and never reads the golden rows, but the scorer branched on the answer key first. An item at one of those levels that also carried a confirmed `expected.sql` spent a warehouse query producing rows nobody looked at — and worse, a guard refusal of that statement returned `unscored`, so the item inherited a refusal from a statement its level never needed. Also: an injected generator's returned error string was relayed into the persisted reason at whatever length it chose, while a raised exception's text is dropped as somebody else's. Same threat model, so the returned one is now bounded. And one test holds the runner's self-judging levels and the comparator's keyed ones to a partition of `MatchLevel`, which nothing did before — a sixth level would have landed in neither and been reported as having no answer key. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The run's `org` reached the generator's prompt and stopped there. Both executions went through the chokepoint without it, so credentials resolved for the single-tenant `local` org — and the org-less `DATASOURCE_URL` names are offered to `local` and to no other org precisely so that a named tenant with a forgotten variable fails closed instead of being pointed at the shared warehouse. A named tenant's golden dataset was therefore scored against whatever warehouse the host happened to have. Passing the org at both call sites is the fix. The end-to-end fixture moves with it: it named a tenant while relying on an org-less variable, which is the conflation this change undoes, so it now sets the org-scoped one. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…y why a run stopped Four things a golden run got wrong or left unsaid. A declared `must_filter` was never checked on an item with no answer key. The statement diff was guarded on there being two statements, but that gate reads the generated statement alone — `must_filter` is the dataset's requirement rather than a property of the golden statement — so a keyless case could declare a required column, filter nothing, and be reported as passed. The diff now runs on every item that produced a statement; a keyless item's claims read `unknown` throughout instead of being absent, which the two tests asserting the old shape move with. A generator that raised left `completed=False` as the only trace, so a forty-item run that broke on the first item rendered as an empty run with no reason. The exception's type — never its message — now goes into the run's findings, next to what the reader could not read. `gating_failures` counts scored items, so a run whose every generation was unreadable reports zero of them and is not green. The counter's semantics are deliberately unchanged: it and `completed` and `errored` are now documented as the three a caller reads together. And the shipped generator's timeout is its own — an injected generator that hangs hangs the run, because the seam has no parameter that could carry a bound. Two tests fill gaps rather than fixing anything: the pass mark is now pinned by the comparator's own near miss (4002 of 4004 rows rounds up at three decimals and must still fail), and a run that stops on a later item is asserted to keep the outcomes it already had. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nswer The model context now carries the approved metrics and the worked examples, and both are written for a person: `billings` says "always break the result out by currency, never return one bare number", and the examples answer richly because a reader benefits from it. A generator given that guidance applies it everywhere - a question counting invoices per status came back split by status AND currency with a summed `billings` column nobody asked for, and a question asking how many customers exist came back with active and inactive alongside the total. Every one of those is a good answer and none of them is the agreed one. Scoring compares against a key a person already signed off, so a wider statement is a different answer rather than a better one - and the item fails on rows that are not wrong, only unasked for. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A date window keyed on the BARE column name, so two tables in one join carrying the same date column had their bounds merged into one interval neither statement wrote. This is one of the two claims allowed to gate, so an invented window is a correct statement failed — the worst direction for the check the design was most careful about. Keyed on the qualified column now. The bare name is kept alongside because `runtime._predicate_columns` is bare by its own design, and the partial-reduction test reads against it. A self-judging level never runs the answer key, yet the answer key's SQL was still handed to the comparator, so `order_sensitive` could be set from a statement that took no part in the score. A diagnostic naming evidence nobody consulted is worse than one naming none. `capture_output=True` held the child's stderr in memory and nothing ever read it. A client can echo the whole prompt there, and the prompt carries the model's vocabulary, so it is discarded by the OS instead. And the truncated generator error overshot the length it is named for: the marker now sits inside `_MAX_RELAYED_ERROR` rather than past it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…erdicts The runner is a function with five required keyword arguments and no way to reach it. This is the wiring: it picks the dataset, renders the tables and columns the generator is given, and decides what a verdict looks like on a terminal. Stdout carries verdicts and never SQL — neither the answer key nor the generated statement is in the printed payload, so a pasted terminal carries no statement with it. Both go to a JSON artifact under the gitignored local dir instead, joined to the question, which is what a report can render side by side. Items are printed failures first, then errors, then the unconfirmed cases that can never gate, then the passes. The summary carries all three values a verdict rests on: a generator that raises truncates the run, and the counts alone would read as clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A confirmed item whose result sets are both empty scores `unscored`: nothing was compared, so it did not pass — and it was landing under failures, where it counted in neither `failed` nor `gating_failures`. The summary would have said `failed: 0` above a list with a row in it, and the substance is wrong either way: an unscored item is its own thing to look at, not a miss. So it gets its own section, ordered after errors and before the unconfirmed. The summary now also carries per-section counts, derived from the emitted rows rather than recomputed, so what is claimed and what is rendered cannot drift apart. The runner's own counters are untouched: they are what a verdict rests on, which is a different question from what is on screen. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…e verdicts The harness had a command and no caller. This is the caller: preflight, pick the dataset, run it, and present what came back — failures first, with the errored, the unscored and the unconfirmed each under their own heading, because a reader scanning for what broke must not pick up an item that can never gate. Cloned from the reconcile skill section for section, so there is one shape for a skill of this kind rather than two. Three things it says that nothing in code can enforce: a verdict is not "zero failures" (a run where every generation errored has none and is not green), a statement is never pasted into chat even though the artifact holds one, and authoring a dataset belongs to the shared reference rather than to a glob across somebody else's profile. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ually ship The invocation doc opened on "agami ships five skills" and listed five while the directory held seven — agami-serve and agami-deploy shipped without a row, and nothing caught it because the count was prose and the truth was a directory listing. It is now eight, with the two missing rows restored, and a test derives both sides so the next skill cannot drift the same way. The rest is registration: the golden-dataset path and the eval dashboard kind in the layout doc, the refusal text in the shared plan-mode doc (quoted verbatim from the skill, so the two cannot disagree), the eval kind in dashboard_dir's docstring — the third place a kind is registered, and the one that had gone stale twice — plus the helper's row in the scripts README and the skill's row in the install doc and the README's command table. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The printed payload already dropped `unmatched_golden_columns`, and then `reason` put the same names back — the comparator builds two of its reasons out of the aliases the author wrote in `expected.sql`, and the skill routes `reason` straight into a chat table. Both are now replaced on the way out: a count for the unmatched columns, an unnamed sentence for the type mismatch. The artifact keeps the score whole, which is where a drill-down reads them from. Four more things the run path got wrong: - A completed run was discarded when its artifact could not be written. The write is guarded now and the verdicts print either way — the run had already spent a model call and two warehouse queries per case. - The model load sat above the guarded block, so a profile that was never connected, or one whose model does not parse, raised a bare traceback with the absolute artifacts path in it. Both are preflight refusals now. - The schema handed to the generator dropped the schema qualifier, so a profile whose schema is not on the connection's search path generated unrunnable SQL and read as a model regression. It also collapsed two same-named tables in two schemas into one. - A YAML syntax error relayed PyYAML's quoted source line — a fragment of the answer key — onto a surface that promises no SQL. Only the first line travels. Three prose criteria survived deleting what they assert, so they now assert the template line and the section ordering rather than the presence of a word. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hat it is REQ-010 sends "the question and the model context" to the generator. The rendering sent names and types alone, which is a narrower reading than that: it dropped the description a curator wrote so a reader would filter a column correctly, and it dropped `key_terminology` entirely. The failure it causes is not subtle. `netsuite.transactions.type` is a string whose values are NetSuite codes; the glossary defines `CustInvc` as "the billings/revenue transaction type — filter transactions.type = 'CustInvc' for revenue questions". Given the column name alone the generator guesses 'Invoice', matches no rows, and the run reports a model regression that is really a missing vocabulary. Both sections are appended to the same flattened string rather than given prompt fields of their own: `SqlGenerator.generate` takes one schema argument, and the argument list is the isolation boundary, so widening it is a contract change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… script does The path block resolved `packages/agami-core/src` and nothing else, which covers a dev checkout and neither of the other two layouts. A marketplace install ships the plugin with `<version>/lib` and no `packages/` at all — so this script would tell that user to pip-install a library their plugin already ships, which is the one instruction that cannot help them. `_agami_lib.ensure_importable()` is the helper that knows all three layouts, and the eight other runtime scripts in this directory already call it. The friendly ImportError below stays: it is still the right message once the library genuinely is absent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vishalkalbi27
force-pushed
the
AH-106-agami-eval-skill
branch
from
August 27, 2026 15:29
39578e9 to
1b1fac9
Compare
vishalkalbi27
force-pushed
the
AH-107-golden-run-report
branch
from
August 27, 2026 15:29
6cf5d14 to
aeda4e9
Compare
…ry skill does The generator's context was assembled here, in Python, from the loader — a second description of what `agami-query/SKILL.md` assembles in prose. Two descriptions of one thing drift, and the eval's copy would drift into scoring a generator the product does not ship. It now runs the same four commands the skill runs: `sm areas`, `sm bundle` per area, `sm org-context`, and `sm examples --query --top-k` per question. The first three do not depend on the question and are fetched once for the whole run, because each call starts an interpreter and a per-item fetch would spend about a second an item to receive the same bytes back. Three sections the eval never sent now reach the generator: the datasource narrative and glossary that `org-context` composes, the entities that map a reader's words onto columns, and the relationships that carry the join cardinality a fan-trap turns on. Ranking is per question and spans EVERY area. `sm examples` reads one area's library at a time, and an eval is not told which area a question belongs to; ranking only the first handed an incident question the asset library on a profile whose areas begin `asset, change, cmdb…`. `agami-query` picks the area by reading descriptions, which is a judgement this has no model to make, so the merged top few is the approximation. `ClaudeCliGenerator` takes a callable as well as a string, because the ranking depends on the question and the generator is built once. `SqlGenerator.generate` is untouched: that argument list is the isolation boundary. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vishalkalbi27
force-pushed
the
AH-107-golden-run-report
branch
from
August 29, 2026 10:13
aeda4e9 to
0f3a3f4
Compare
…he tenant The empty-profile refusal quoted the resolved `golden_datasets` path in full, which is the absolute artifacts directory — and on a hosted deployment that path encodes the tenant. Every other refusal in this script already withholds it, including the two directly above this one, so the rule was the file's own and this line was the exception. It says the same thing in relative terms now, and `--list` still carries the resolved path in its payload for a caller who asks for it rather than has it printed at them. `_pick` loses the argument it only used for that message, and a test pins the rule so the next message written here inherits it. `dashboard_dir`'s docstring said "Rendered dashboards" while `eval` writes a JSON run artifact beside its HTML. Corrected in both copies together, byte for byte: the vendored mirror is drift-checked and a docstring counts. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ements side by side
Three decimals is enough precision to round 4002/4004 up to 1.000, and an item passes at exactly 1.0. So the number beside "did not reproduce the answer key" could look identical to the number beside a pass — the one confusion a report that exists to explain a failure must not introduce. Only a real 1.0 is shown as 1.000 now; anything short of the mark is held short of it.
The golden-run renderer substituted the run's JSON third, before the profile,
the logos and the stylesheet. A question reading "how many {{THEME_CSS}} orders?"
therefore had a stylesheet spliced into the object literal by a later replace:
the JSON stopped parsing, the script threw at load, and because the whole body
is built by that script the report rendered blank with nothing on it saying why.
The payload goes in last now, after every other placeholder, and a comment says
why this renderer's order is deliberate where its siblings' is not — its payload
is author-written SQL and free text.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tuted last Review found the page untested: every assertion read the embedded JSON, so a report that drew nothing at all passed the whole file. The drawing code is now pinned by its markup — the ceiling of what this suite can assert without a browser — and a mutation of it fails a test. The payload is also substituted after every other placeholder now. It was third, so a question carrying a literal placeholder spliced raw CSS into the object the page builds itself from: the JSON stopped parsing, the script threw at load, and the report rendered empty with nothing anywhere saying why. Two projected fields nothing on the page read are gone, so the whitelist that keeps result rows off the file is one every name earns a place in.
… write `--items-file` is a handoff and the shape on the far side of one is promised by a docstring, not enforced here. Four fields took the promise at its word, and each turned a malformed item into a blank page: `claims` assumed a dict of a list of dicts, `score` assumed a dict, and `accuracy` reached `_shown`'s comparison against 1.0, where a string raises. That failure has no recourse. The report is what a person opens when a run has ALREADY gone wrong, so a report that cannot render is the second failure in a row with nothing behind it. Each of these now costs the field it broke and nothing else. Two further defaults were the wrong way round. A run that omits `completed` has not said it stopped partway, and banner-ing a finished run as incomplete is the worse of the two errors, so absent now reads as complete. And `verified` accepted any truthy `confirmed`, so the string "False" in a hand-edited artifact would render a run nobody signed off as verified. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
vishalkalbi27
force-pushed
the
AH-107-golden-run-report
branch
from
August 29, 2026 12:08
0f3a3f4 to
185b5ef
Compare
vishalkalbi27
force-pushed
the
AH-106-agami-eval-skill
branch
from
August 29, 2026 15:07
6365b79 to
fef4ac2
Compare
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 join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
When a golden question stops matching, the useful artifact is the confirmed statement and the
generated one read against each other. That does not fit in a chat message, which is why the eval
skill's in-chat summary is deliberately compact.
agami already answers this shape four times — a chart, the model explorer, the examples-validation
queue and the prune view are each a self-contained HTML file written by a stdlib-only script that
substitutes into a template in
shared/. This is the fifth, and it is meant to be recognisablythe same thing.
Spec: AH-107
Changes
render_golden_run.py+golden-run-template.html—render(*, title, profile, run) -> strbehind the same four flags the sibling renderers take. It takes the run whole rather than an
items list, because the header reads
summaryandcompleted, which are not per-item.python3a user has, with no packageinstalled. That constraint is why the claim difference is carried in the artifact rather than
computed at render time — the renderer cannot parse a statement.
claims,confirmed,passed,gated,section). Without them the table-set delta above each item — usually the whole finding — has noinput. A change that drops any of them empties part of the report without failing anything.
reportsits besideartifact, sharing onefilename stamp so the pair is findable by name; each write degrades on its own, because the run is
already paid for and the verdicts print either way.
its own
_SECTION_ORDER. A second ordering would be a second opinion about what a run looks like.The two rules the report keeps
The rule that keeps a key off a terminal is about stdout and about what a model reads; a
gitignored file a person opens afterwards is neither.
embedded whole, so a field added upstream later cannot arrive on the page by accident. Asserted by
planting row-shaped values on the item, on its score, and on the run.
Review
Three defects worth naming, all found in review and fixed here:
nothing passed all 20 tests — proven by gutting the drawing code and watching the suite stay
green. The drawing is now pinned by its markup, which is this suite's ceiling without a browser,
and a mutation of it fails a test.
so a question containing one of those literals spliced raw CSS into the object the page builds
itself from: JSON stopped parsing, the script threw at load, and the report rendered empty with
nothing saying why. The payload is substituted last now.
1.000, and an itempasses at exactly
1.0— so a failure could print the same number as a pass. Only a real1.0reaches
1.000now.One correction to the spec, recorded in its
## Decisions: self-containment could not be inheritedfrom "the other four templates" as written — the chart template loads its plotting library from a
CDN, so only three of the four hold the property. It is asserted here instead.
Checklist
uv run dev.py checkgreen — ruff, full suite, gitleaks, lib-drift.loosened.
🤖 Generated with Claude Code