Golden dataset reader

[vishalkalbi27]

Spec: AH-100

Summary

A golden dataset had nowhere to live and no shape to be written in. This adds the on-disk location,
the YAML shape, the typed records, and the validation posture — the read half only. Nothing here
writes a dataset, touches a database, or runs anything; a later slice owns each of those.

A golden dataset is a file of questions whose answer is already agreed: the author writes the
question, the SQL they accept as the answer key, and how strictly a run has to match it. The runner
that consumes these decides pass/fail, so this module's whole job is to hand it records it can trust
— and to say out loud which files and cases it had to drop getting there.

Two properties are load-bearing and both are pinned by tests:

• Fault isolation. One malformed file, or one malformed case inside an otherwise good file, must
  not cost the run every other case. Each fault becomes a finding and the read continues.
• No path escapes. Nothing returned carries a filesystem path — not on a record, and not in a
  finding message — so a downstream runner cannot forward a dataset location into a subprocess.

Changes

• packages/agami-core/src/semantic_model/golden.py (new) — GoldenDataset / GoldenItem and
  their nested models, all deriving models.py::_Base so extra="forbid" makes a typo'd field a
  named refusal rather than a silent no-op. One entry point,
  load_golden_datasets(profile, art=None) -> tuple[list[GoldenDataset], ValidationResult],
  reusing validator.py's Finding / ValidationResult rather than inventing a second reporting
  shape. Includes the relativity lint: an item whose question reads as relative while its answer key
  is pinned to fixed dates is reported as a dataset error and left in place, because the case is
  broken, not the model.
• plugins/agami/shared/golden-dataset-shape.md (new) — the canonical authoring reference,
  following metric-entity-shape.md and inheriting its hard rule against reading another profile to
  learn a shape. That rule binds harder here: a golden dataset is the business definitions and the
  answer key in one file, so globbing a sibling profile reads another customer's questions together
  with the SQL that answers them. A test parses the doc's example through the real reader, so the
  reference cannot drift from the parser that has to accept it.
• docs/format-spec.md — one row in the sharable table, owner User-authored.
• tests/test_golden_dataset_reader.py (new) — 73 tests.

Not touched: semantic_model/__init__.py, which is mirrored byte-for-byte into plugins/agami/lib/
and pinned by test_plugin_lib_resolution.py::VENDORED. golden.py is deliberately absent from that
list — the mirror is stdlib-only and this needs PyYAML + pydantic. No pyproject.toml change is
needed; semantic_model already ships as a whole package.

Decisions made during the build

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

1. id and query are required, and sql_confirmed: true with no expected.sql is refused.
   Read literally, "everything but sql_confirmed is optional" yields an item with no item_key and
   nothing for the runner to generate against; and a confirmed item with no answer key is the one
   kind that can gate a run and can never fail it.
2. recorded is typed columns / rows / optional at, not a free-form map — a receipt
   nothing can assert against cannot support the drift detection it exists for.
3. confirmed_by is a free-text method + optional ISO at. Narrowing to a Literal later is
   additive; widening a closed set is not.
4. The module lands in semantic_model/, which already owns the pydantic models, the YAML
   loading and the validator.

Findings from the build and review

• The field count moved twice, deliberately. The spec said "six fields" while naming five —
  F22's list only reads as more because it carries name (the filename stem) and item_key
  (derived from id), neither of which an author writes. That was a miscount and is corrected in
  the spec. Then a sixth field was genuinely added: bounds (see below), so the count is now
  six for a real reason rather than an arithmetic one.
• bounds was added after the first review round, because the comparator slice found that
  match: bounded was writable while its band was not — the model is extra="forbid" and the
  docs bound the added fields with "and nothing else", so one of the contract's five match levels
  could never do anything. GoldenBounds carries min_rows / max_rows / min_value /
  max_value, and two pairing refusals close the inert combination in both directions: bounded
  without a band, and a band under any other level. Landed here while this PR was open; after
  merge the same change would have cost a new spec.
• The shape doc taught must_filter as predicates (status = 'paid') while F22 defines it as
  column names. Caught in review, fixed to columns. The doc is the only authority authors have
  until the statement comparator lands, so every dataset authored from the draft would have failed
  the gate — and no test caught it, because the reader takes list[str] either way.
• The frozen-date regex originally matched any bare integer 1900–2099, so LIMIT 2000 or
  total_amount > 1999 raised an error-severity false positive on a correct dataset. The bare-year
  branch is gone; the quoted-ISO-date branch still catches the canonical frozen shape.
• Two crash paths broke fault isolation: a non-list test_cases, and a non-string YAML root key
  (no: and on: resolve to booleans under YAML 1.1) both raised TypeError past the
  except ValidationError, losing every other file in the directory. Both now produce a named
  finding.
• Two leak paths were closed in finding messages: a raw OSError carried the absolute path, and
  the full pydantic ValidationError echoed input_value= — which can hold answer-key SQL,
  must_filter values, and recorded row data. Messages now name the field and the reason without
  the value. There is no caller yet, so the contract was settled while it was still free.
• Pre-existing, not fixed here: agami_paths.profile_dir joins profile with no containment
  check, so profile="../.." escapes the artifacts dir. It sits on an untouched line and is shared
  by every loader in the package; fixing it here would leave the others inconsistent.

Checklist

• Spec referenced (Spec: AH-100)
• Every spec success criterion implemented and covered by a test (13 of 13, plus the feature's
  E2E items and REQ-006)
• Full suite green — 4719 passed, 12 skipped, 0 failed
• ruff check clean; gitleaks clean
• No test weakened or deleted. Three fixtures changed to column-form must_filter and a
  sample-store column, because the doc no longer teaches predicate form; assertions unchanged.
• No real customer, organization, table, or question names — fixtures are questions over the
  shipped sample store
• No spec ids in source, comments, docs, or commit messages
• Reviewed via the panel (correctness, structural rubric, security/disclosure); all findings
  dispositioned

🤖 Generated with Claude Code

[vishalkalbi27]

Pushed b449bed after the first review round — worth re-reading if you had already started.

What changed: a sixth authored item field, bounds, plus its refusals, tests and docs.

Why now: building the result-set comparator surfaced that match: bounded was writable while its band was not. The model is extra="forbid" and the docs bound the added fields with "and nothing else", so an author could select one of the contract's five match levels and have it do nothing, silently. Two validators close the inert combination in both directions — bounded without a band, and a band under any other level — so neither half can be written on its own.

Doing it here costs one commit; after this merges the same change costs a new spec, which is why it landed mid-review rather than as a follow-up.

53 → 73 tests, no existing test modified. Full suite 4719 passed, 12 skipped, 0 failed.

[vishalkalbi27]

Heads-up: #239 (the result-set comparator) is stacked on this branch and targets it as its base, because it imports MatchLevel and GoldenBounds from golden.py.

Merging this one first lets #239 retarget cleanly onto main. The bounds field added here in b449bed is what #239's bounded match level scores against.

[Copilot]

Pull request overview

Adds the “golden dataset” read-path to semantic_model/: a YAML reader plus typed (Pydantic v2) records and validation/findings reporting, designed to (1) isolate faults to a single file/case and (2) avoid leaking filesystem paths in returned records or findings.

Changes:

• Introduces semantic_model/golden.py with GoldenDataset / GoldenItem models, YAML loading, per-file/per-case validation findings, and the relativity lint + bounded-band pairing rules.
• Adds the canonical authoring reference for the YAML shape (plugins/agami/shared/golden-dataset-shape.md) and links the new artifact type in docs/format-spec.md.
• Adds a large unit test suite covering happy paths, fault isolation, and non-leakage assertions.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

File	Description
packages/agami-core/src/semantic_model/golden.py	New golden dataset reader + Pydantic models, validation/findings, and relativity lint logic.
plugins/agami/shared/golden-dataset-shape.md	New canonical documentation for authoring golden dataset YAML files.
tests/test_golden_dataset_reader.py	New unit tests validating parsing, refusal behavior, fault isolation, and “no path escapes” guarantees.
docs/format-spec.md	Documents the new <profile>/golden_datasets/<name>.yaml artifact in the format spec table.

---

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