The /agami-eval skill by vishalkalbi27 · Pull Request #241 · AgamiAI/agami-core

vishalkalbi27 · 2026-08-26T11:33:32Z

Spec: AH-106

Fifth in a stack, on #240 (AH-101-golden-run-executor), which it targets as its base. It
needs the reader, both comparators and the runner at once. Merge the stack in order and this
retargets onto main. Review only this branch's five commits.

Summary

The runner exists and nothing can call it. This is the developer surface: run a golden dataset from
the tool the model is edited in, and read the result with failures first. No admin, no server,
no browser.

Changes

plugins/agami/scripts/run_golden_eval.py (new) — the CLI the runner never had.
--profile · --dataset · --list · --timeout-s. --list answers "what is there" without a
database, a generator or credentials. Run mode reproduces the e2e test's wiring, prints a verdict
payload to stdout and writes a richer artifact to dashboard_dir("eval", profile).
plugins/agami/skills/agami-eval/SKILL.md (new) — cloned section for section from
agami-reconcile, which is the sanctioned shape. Preflight, dataset selection, run, then Phase 3
in the contract's fixed order.
tests/test_ah106_eval_skill.py (new) — 43 tests.
Seven documentation files — see Drive-by corrections below.

The script owns the schema string. ClaudeCliGenerator takes the tables and columns
pre-rendered, and nothing upstream produces them — the only construction site in the repo was a
constant in a unit test. Built in-process from list_subject_areas + get_subject_area_bundle.

Decisions

Four were taken at the grounding gate and are recorded in the spec's ## Decisions:

This slice ships the script, widening In scope by one file. The runner is a Python function
with five required keyword arguments and no CLI; the house rule forbids driving that from
python3 -c; five of the nine success criteria would have had no artifact to assert against; and
AH-110, which must "share the same presentation", cannot share one with a prose file.
Stdout carries no SQL; the artifact keeps it. Two formats, deliberately — the contract forbids
SQL on the scoring step's stdout, and the report must show the two statements side by side.
This slice writes the joined run artifact AH-107 renders. GoldenRunResult carries neither
the question nor expected.sql, so only the caller holding both the dataset and the result can
join them. It does not render HTML.
A dataset is named as an argument, asked for only when needed.

Findings from review

Two passes ran over the diff. Six must-fixes, all reproduced, all fixed and re-verified.

The answer key's column names were reaching stdout. The comparator builds two of its reasons from
the unmatched golden column names — the aliases the author wrote in expected.sql — and the skill
routes reason straight into the chat table. The script already dropped the structured
unmatched_golden_columns field, so the design had decided those names do not go to stdout, and
reason smuggled them back. The existing test could not see it: it planted its sentinel on a
passing case, whose reason is empty. Now replaced with a count before printing, with an audit of
every other reason the comparator can produce (recorded in the commit) and an end-to-end test that
reddens if the comparator rewords.

An unwritable artifact directory discarded the whole run. The write happened before the print, so
a read-only mount or a full disk threw away every verdict as a traceback — after spending a model
call and two warehouse queries per case. The payload now prints regardless.

loader.load_datasource sat outside the guarded block, so a missing or malformed
datasource.yaml produced a bare traceback that also printed the absolute artifacts path — which the
runner goes to some length to withhold. A sibling test already asserted "Traceback" not in err, so
the contract was explicit.

The schema string dropped the schema qualifier. On a Postgres or Snowflake profile whose schema
is not on the connection's search_path, every generated statement would be unqualified and every
item would error — an eval run of pure noise that reads as a model regression. The sqlite sample
cannot expose it, so no test caught it; now rendered schema.table per the repo's own convention.

A YAML syntax error leaked a fragment of the answer key. PyYAML quotes ~75 characters of the
offending source line back inside its message, and on a golden dataset that line is sql:. The
message is now cut to its first line; code and locator stay whole.

Three prose criteria survived mutations that deleted the behaviour they asserted — deleting
unscored from the summary template, moving the unconfirmed section above the failures, and deleting
the zero-confirmed line all left the suite green. All three now assert the thing itself: the template
line, the section ordering, and the sentence.

One defect was caught during the build, before review: a confirmed item scored unscored was
classified as a failure. It counts in neither failed nor gating_failures, so the summary would
have read failed: 0 above a failure list with a row in it — breaking the criterion that summary
counts match rendered rows. There is now a fifth unscored section, and summary["sections"] is
derived from the emitted rows so counts and rows cannot disagree.

Drive-by corrections the spec asked for

shared/invocation-conventions.md claimed "five skills" while seven shipped — agami-serve and
agami-deploy were missing from its table. Rather than add an eighth row to a wrong count, the two
missing rows are added and a test derives the count from the directory, so it cannot drift again.
docs/install/claude-code-cli.md's "7 skills available" and README.md's command table would both
have become false on landing. shared/file-layout.md gains the dataset path and the eval dashboard
kind; shared/plan-mode-check.md gains the per-skill refusal, verbatim-matched to the skill's own;
agami_paths.dashboard_dir's docstring gains eval (and its byte-mirrored vendored copy with it).

Residual risk

The shape-level reason is recognised by matching its wording, because it carries no structured
field to rebuild from. If the comparator rewords it, the match fails open — the identifier would
pass through. That is pinned by an end-to-end test that reddens on a reword, so CI catches it, but a
future change there needs re-auditing rather than re-wording.

Pre-existing, not fixed here: an injected third-party SqlGenerator that put SQL in its own
error string would reach stdout through the runner's error relay (the shipped generator only ever
returns four fixed, value-free sentences); and --profile accepts a path separator, which is
agami_paths behaviour shared by every plugin script.

Checklist

Spec referenced (Spec: AH-106)
Every spec success criterion implemented and covered by a test (9 of 9, plus the feature's E2E
items and REQ-006)
Full suite green — 5099 passed, 12 skipped, 0 failed
ruff check clean; gitleaks clean
No test weakened or deleted. Three were modified deliberately: one for the schema-qualifier fix,
two strengthened from substring presence to the assertion the criterion actually requires.
No real customer, organization, table, or question names — fixtures use the shipped sample store
No spec ids in source, comments, or docs
Adds no MCP tool and no sm subcommand; writes nothing into <profile>/
Reviewed via the panel (correctness/tests, structural rubric, security/disclosure,
documentation accuracy); all findings dispositioned

🤖 Generated with Claude Code

Copilot

Pull request overview

Adds the missing developer-facing entrypoint for running “golden evals” end-to-end (list datasets, pick one, run generation/execution/scoring, and present failures-first) and documents/validates the /agami-eval skill contract via a comprehensive test suite.

Changes:

Introduces run_golden_eval.py CLI to run/list golden datasets and emit a stdout-safe JSON verdict plus a richer on-disk artifact.
Adds the new /agami-eval skill spec (phases, guardrails, refusal wording) and updates shared/docs to include the new skill + paths.
Adds tests/test_ah106_eval_skill.py to lock down ordering, counters, no-SQL-on-stdout guarantees, and doc invariants.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
tests/test_ah106_eval_skill.py	End-to-end style contract tests for the script output + skill/doc invariants.
README.md	Adds `/agami-eval` to the public command table.
plugins/agami/skills/agami-eval/SKILL.md	Defines the eval skill phases, output presentation rules, and guardrails.
plugins/agami/shared/plan-mode-check.md	Adds plan-mode refusal wording for `agami-eval`.
plugins/agami/shared/invocation-conventions.md	Adds `agami-eval` row and updates shipped-skill count.
plugins/agami/shared/file-layout.md	Documents new golden dataset location and eval outputs under `local/`.
plugins/agami/scripts/run_golden_eval.py	New CLI wiring for running golden datasets with stdout-safe verdicts + artifact writing.
plugins/agami/scripts/README.md	Documents the new script and its dependency expectations.
plugins/agami/lib/agami_paths.py	Updates `dashboard_dir` docstring to include `eval`.
packages/agami-core/src/agami_paths.py	Mirrors the `dashboard_dir` docstring update in the core package copy.
docs/install/claude-code-cli.md	Updates “skills available” expected output to include `agami-eval`.

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+# A dev checkout keeps the package beside the plugin and does not install it; a marketplace install
+# ships the plugin alone, with the package already importable. Prepending the checkout's source when
+# it is actually there covers both without asking which install this is.
+_PKG_SRC = Path(__file__).resolve().parents[3] / "packages" / "agami-core" / "src"
+if _PKG_SRC.is_dir() and str(_PKG_SRC) not in sys.path:
+    sys.path.insert(0, str(_PKG_SRC))
+
+try:
+    import agami_paths


 | `charts/<profile>/<ts>.html` | Per-query HTML reports |
 | `exports/<profile>/<ts>.csv` | Per-query CSV exports |
-| `{review,model,examples-validation}/<profile>/<ts>.html` | Per-profile dashboards |
+| `{review,model,examples-validation,eval}/<profile>/<ts>.html` | Per-profile dashboards. The `eval` kind also holds a JSON run artifact per run — the answer key and the generated statement, side by side |


Copilot

Pull request overview

Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.

Suppressed comments (1)

plugins/agami/shared/file-layout.md:20

This table row documents eval artifacts as <ts>.html, but run_golden_eval.py writes JSON artifacts (<ts>.json) under the eval/ dashboard dir. As written, the doc implies an HTML artifact that doesn't exist and hides the real JSON path.

| `{review,model,examples-validation,eval}/<profile>/<ts>.html` | Per-profile dashboards. The `eval` kind also holds a JSON run artifact per run — the answer key and the generated statement, side by side |

+    elif not datasets:
+        _stop(
+            "this profile has no golden datasets to run. The first one goes in "
+            f"{datasets_dir}/<name>.yaml"
+        )


 def dashboard_dir(kind: str, profile: str, art: Path | None = None) -> Path:
-    """Rendered dashboards: kind in {model, review, examples-validation}."""
+    """Rendered dashboards: kind in {model, review, examples-validation, eval}."""
    return local_dir(art) / kind / profile


 def dashboard_dir(kind: str, profile: str, art: Path | None = None) -> Path:
-    """Rendered dashboards: kind in {model, review, examples-validation}."""
+    """Rendered dashboards: kind in {model, review, examples-validation, eval}."""
    return local_dir(art) / kind / profile


…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>

…d examples

… 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>

…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>

…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>

vishalkalbi27 requested review from ashwin-agami and sandeep-agami as code owners August 26, 2026 11:33

vishalkalbi27 mentioned this pull request Aug 26, 2026

Eval command and suites #243

Open

8 tasks

ashwin-agami requested a lite review from Copilot August 26, 2026 16:31

Copilot started reviewing on behalf of ashwin-agami August 26, 2026 16:32 View session

Copilot AI reviewed Aug 26, 2026

View reviewed changes

vishalkalbi27 force-pushed the AH-106-agami-eval-skill branch from b447cf3 to 9ee3bc2 Compare August 27, 2026 07:40

vishalkalbi27 force-pushed the AH-101-golden-run-executor branch from 515b085 to 0808838 Compare August 27, 2026 13:50

vishalkalbi27 force-pushed the AH-106-agami-eval-skill branch from 9ee3bc2 to 39578e9 Compare August 27, 2026 13:50

vishalkalbi27 force-pushed the AH-101-golden-run-executor branch from 0808838 to 27f92fb Compare August 27, 2026 15:22

vishalkalbi27 force-pushed the AH-106-agami-eval-skill branch from 39578e9 to 1b1fac9 Compare August 27, 2026 15:29

ashwin-agami requested a lite review from Copilot August 29, 2026 10:38

Copilot started reviewing on behalf of ashwin-agami August 29, 2026 10:38 View session

Copilot AI reviewed Aug 29, 2026

View reviewed changes

vishalkalbi27 force-pushed the AH-101-golden-run-executor branch from 27f92fb to a7f870f Compare August 29, 2026 14:38

Base automatically changed from AH-101-golden-run-executor to main August 29, 2026 14:45

vishalkalbi27 and others added 10 commits August 29, 2026 20:28

semantic model: the generator gets the approved metrics and the worke…

8061780

…d examples

vishalkalbi27 force-pushed the AH-106-agami-eval-skill branch from 6365b79 to fef4ac2 Compare August 29, 2026 15:07

Navigation Menu

Uh oh!

The /agami-eval skill - #241

The /agami-eval skill#241
vishalkalbi27 wants to merge 10 commits into
mainfrom
AH-106-agami-eval-skill

vishalkalbi27 commented Aug 26, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

vishalkalbi27 commented Aug 26, 2026

Summary

Changes

Decisions

Findings from review

Drive-by corrections the spec asked for

Residual risk

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants