From 13bb204b284012251197892ed276ef0b4e6e8ebf Mon Sep 17 00:00:00 2001 From: Ashwin Ramachandran Date: Sun, 13 Sep 2026 17:13:35 -0700 Subject: [PATCH 1/3] The import door reads an Excel workbook MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A question bank lives in Excel, and the import door refused an .xlsx outright, asking for a CSV export first. The Read tool can't open a workbook, so the earlier attempt to have the skill read one (#267) could never work. A workbook is a zip of XML, so `_xlsx.py` reads one sheet of it with zipfile and ElementTree alone — no dependency added to a plugin that installs with none — into the same rows a CSV yields. Everything after that is one parse, so there is still one place a column can be misread. What a real question-bank workbook needed, beyond "read the first sheet": - Several sheets, the questions not on the first. With none named, the parse stops and lists them; `--sheet` names one (case and surrounding space are forgiven when unambiguous). Which tab holds the questions stays the user's call. - A title block above the table. The header is the first row within the top 20 that names a question column, by the same exact alias match as before. - Rows numbered as Excel numbers them, including rows the file omits, so a skipped row is reported at the number the person sees. - Rows Excel formats down to 1000 but never fills are dropped, not reported as a thousand empty questions. Shared strings with rich-text runs (phonetic guides left out), inline strings, booleans, numbers and sparse cells read as the sheet shows them; an error value is blank. An .xls file is refused with how to get past it, as is a file that isn't a workbook, a part declaring a DTD, or an oversized part. `--csv` keeps working as a second spelling of `--file`. Closes #261. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 13 + plugins/agami/scripts/_xlsx.py | 221 +++++++++++ plugins/agami/scripts/golden_author.py | 116 ++++-- plugins/agami/shared/golden-dataset-shape.md | 3 +- .../agami/skills/agami-save-golden/SKILL.md | 22 +- tests/test_ah109_save_golden_skill.py | 9 + tests/test_golden_author_xlsx.py | 346 ++++++++++++++++++ 7 files changed, 693 insertions(+), 37 deletions(-) create mode 100644 plugins/agami/scripts/_xlsx.py create mode 100644 tests/test_golden_author_xlsx.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 5964208d..b1ab7a0d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,19 @@ below corresponds to one such version. ## [Unreleased] +### Added + +- **The import door reads an Excel workbook.** `golden_author.py parse --file bank.xlsx` reads one + sheet of an `.xlsx` into the same parse a CSV goes through, with the standard library alone — a + workbook is a zip of XML, so no dependency is added to a plugin that installs with none. A workbook + with several sheets stops and names them, because which tab holds the questions is the person's + call; `--sheet` names it. The header no longer has to be the first row: the first row within the + top 20 that names a question column is taken, so a title block above the table is fine, and rows + keep Excel's own numbering, so a skipped row is reported at the number the person sees. Rows Excel + formats but never fills are dropped rather than reported as empty questions. An `.xls` file — + Excel's older binary format — is still refused, with how to get past it, and `--csv` keeps + working. (#261) + ### Changed - **A golden run pays for the model's description once, not once per question.** Every question diff --git a/plugins/agami/scripts/_xlsx.py b/plugins/agami/scripts/_xlsx.py new file mode 100644 index 00000000..133399c3 --- /dev/null +++ b/plugins/agami/scripts/_xlsx.py @@ -0,0 +1,221 @@ +"""Read one sheet of an `.xlsx` workbook as rows of text — the standard library and nothing else. + +A question bank lives in Excel, and the import door is the one surface that has to open such a file +itself: the Read tool cannot open a workbook, and a model reconstructing one would be a second parser +with no way to check it. So this reads the workbook the way the format is defined — a zip of XML +parts — with `zipfile` and `ElementTree`, and adds no dependency to a plugin that installs with none. + +It reads what a person SEES, not what Excel computes: a cell's stored value, a formula's cached +result, a shared string with its rich-text runs joined. Nothing is recalculated, no style is applied, +and a date comes through as the serial number Excel stores for it. That is enough for a column of +questions, ids, statements and tags, which is all the import door asks of a sheet. + +Rows keep Excel's own numbering — a row the file omits is an empty row, not a missing one — because +the parse reports skipped rows by number, and a person finds them by that number in their workbook. +""" + +from __future__ import annotations + +import posixpath +import re +import xml.etree.ElementTree as ET +import zipfile +from typing import Optional + +_MAIN = "{http://schemas.openxmlformats.org/spreadsheetml/2006/main}" +_REL = "{http://schemas.openxmlformats.org/officeDocument/2006/relationships}" +_PACKAGE_REL = "{http://schemas.openxmlformats.org/package/2006/relationships}" + +# The largest single part this will decompress. A sheet of questions is kilobytes; a part expanding +# past this is not a question bank, and reading it whole is how a crafted file exhausts memory. The +# images and embedded objects a workbook carries are never read, so a workbook full of screenshots +# does not come near it. +_MAX_PART_BYTES = 64 * 1024 * 1024 + +_CELL_REFERENCE = re.compile(r"([A-Z]+)(\d+)") + + +class WorkbookError(ValueError): + """A workbook this reader cannot use, carrying a sentence a person can act on.""" + + +def sheet_names(path: str) -> list[str]: + """Every sheet in the workbook, in the order Excel shows its tabs.""" + with _open(path) as book: + return [name for name, _ in _sheets(book)] + + +def read_sheet(path: str, sheet: Optional[str]) -> tuple[str, list[list[str]]]: + """One sheet's name and its rows, numbered as Excel numbers them. + + `sheet` may be None only for a workbook with exactly one sheet. Choosing among several is a + judgement — which tab holds the questions — and the first tab of a real workbook is as likely to + be a cover page, so the caller is told to ask rather than handed a guess. + + A name is matched exactly, then once more ignoring case and surrounding space, so `questions` + finds a tab called `Questions ` — but only when that looser match is unambiguous. + """ + with _open(path) as book: + sheets = _sheets(book) + if sheet is None: + if len(sheets) != 1: + raise WorkbookError( + f"this workbook has {len(sheets)} sheets, so which one holds the questions has " + f"to be named with --sheet. Sheets: {_listed(sheets)}" + ) + name, part = sheets[0] + else: + matches = [entry for entry in sheets if entry[0] == sheet] + if not matches: + wanted = sheet.strip().casefold() + matches = [entry for entry in sheets if entry[0].strip().casefold() == wanted] + if len(matches) != 1: + raise WorkbookError( + f"this workbook has no sheet named {sheet!r}. Sheets: {_listed(sheets)}" + ) + name, part = matches[0] + return name, _rows(_xml(book, part), _shared_strings(book)) + + +def _open(path: str) -> zipfile.ZipFile: + """The workbook as the zip it is, or a sentence saying it is not one. + + `FileNotFoundError` and `IsADirectoryError` are left to propagate: the caller already reports + both in its own words, and a second phrasing of "that path is wrong" would only drift. + """ + try: + return zipfile.ZipFile(path) + except zipfile.BadZipFile as exc: + raise WorkbookError( + "this file is not a readable .xlsx workbook — if it is an older Excel file renamed to " + ".xlsx, save it as .xlsx (or CSV) from Excel and re-invoke" + ) from exc + + +def _xml(book: zipfile.ZipFile, part: str) -> ET.Element: + """One XML part, parsed — refused if it is oversized, missing, malformed or declares a DTD. + + A spreadsheet part never declares a DTD. One that does is either not a workbook or is built to + make an XML parser expand entities, and refusing it outright is cheaper than reasoning about + which. + """ + try: + info = book.getinfo(part) + except KeyError as exc: + raise WorkbookError(f"this workbook is missing a part it refers to ({part})") from exc + if info.file_size > _MAX_PART_BYTES: + raise WorkbookError("a part of this workbook is too large to read as a question bank") + data = book.read(info) + if b" list[tuple[str, str]]: + """Each sheet's name and the zip part holding it, resolved through the workbook's relationships. + + A sheet's part is not guaranteed to be `sheet.xml` for the Nth tab — reordering tabs in Excel + reorders the names and leaves the files — so the relationship is followed rather than assumed. + """ + workbook = _xml(book, "xl/workbook.xml") + relationships = _xml(book, "xl/_rels/workbook.xml.rels") + targets = { + relationship.get("Id"): relationship.get("Target") or "" + for relationship in relationships.iter(_PACKAGE_REL + "Relationship") + } + found: list[tuple[str, str]] = [] + for sheet in workbook.iter(_MAIN + "sheet"): + target = targets.get(sheet.get(_REL + "id"), "") + # Relative to `xl/`, unless written as an absolute path inside the package. + if target.startswith("/"): + part = target.lstrip("/") + else: + part = posixpath.normpath(posixpath.join("xl", target)) + found.append((sheet.get("name") or "", part)) + if not found: + raise WorkbookError("this workbook has no sheets") + return found + + +def _shared_strings(book: zipfile.ZipFile) -> list[str]: + """The workbook's shared string table. Most text cells are an index into it.""" + if "xl/sharedStrings.xml" not in book.namelist(): + return [] + return [_text(item) for item in _xml(book, "xl/sharedStrings.xml").iter(_MAIN + "si")] + + +def _text(item: ET.Element) -> str: + """A string item's text: its own ``, then every rich-text run's, in order. + + Only direct children are read. A phonetic guide (``) also carries a ``, and a + whole-subtree search would splice its reading into the middle of the cell. + """ + parts = [item.findtext(_MAIN + "t") or ""] + parts += [run.findtext(_MAIN + "t") or "" for run in item.findall(_MAIN + "r")] + return "".join(parts) + + +def _rows(sheet: ET.Element, strings: list[str]) -> list[list[str]]: + """Every row of the sheet as text, placed at Excel's row number and each cell at its column. + + Trailing rows with nothing in them are dropped: a sheet formatted down to row 1000 has not got + 950 empty questions in it, and reporting each as a skip would bury the skips that matter. Empty + rows BETWEEN filled ones are kept, because they hold the numbering of everything after them. + """ + rows: list[list[str]] = [] + for row in sheet.iter(_MAIN + "row"): + number = int(row.get("r") or len(rows) + 1) + # Excel omits a row it has nothing to say about; that row is still one a person counts. + while len(rows) < number - 1: + rows.append([]) + cells: list[str] = [] + for cell in row.findall(_MAIN + "c"): + reference = _CELL_REFERENCE.fullmatch(cell.get("r") or "") + column = _column_index(reference.group(1)) if reference else len(cells) + while len(cells) < column: + cells.append("") + value = _value(cell, strings) + if column < len(cells): + cells[column] = value + else: + cells.append(value) + rows.append(cells) + while rows and not any(cell.strip() for cell in rows[-1]): + rows.pop() + return rows + + +def _column_index(letters: str) -> int: + """`A` is 0, `Z` is 25, `AA` is 26 — Excel's column letters as a list index.""" + index = 0 + for letter in letters: + index = index * 26 + (ord(letter) - ord("A") + 1) + return index - 1 + + +def _value(cell: ET.Element, strings: list[str]) -> str: + """A cell as text, by the type Excel recorded for it.""" + kind = cell.get("t") + if kind == "inlineStr": + inline = cell.find(_MAIN + "is") + return _text(inline) if inline is not None else "" + raw = cell.findtext(_MAIN + "v") or "" + if kind == "s": + try: + return strings[int(raw)] + except (ValueError, IndexError): + return "" + if kind == "b": + return {"1": "TRUE", "0": "FALSE"}.get(raw, raw) + if kind == "e": + # An error value (#N/A, #REF!) is not text anybody wrote. Blank, it is treated as the empty + # cell it effectively is, rather than imported as a question that reads "#N/A". + return "" + return raw + + +def _listed(sheets: list[tuple[str, str]]) -> str: + return ", ".join(repr(name) for name, _ in sheets) diff --git a/plugins/agami/scripts/golden_author.py b/plugins/agami/scripts/golden_author.py index d30377ce..41edc082 100644 --- a/plugins/agami/scripts/golden_author.py +++ b/plugins/agami/scripts/golden_author.py @@ -24,14 +24,16 @@ Usage: - python3 golden_author.py parse --csv /path/to/question-bank.csv + python3 golden_author.py parse --file /path/to/question-bank.csv + python3 golden_author.py parse --file /path/to/question-bank.xlsx --sheet Questions python3 golden_author.py import --profile main --dataset orders --rows /path/to/parsed.json python3 golden_author.py save --profile main --dataset orders --item /path/to/item.json Stdout is always one JSON document; every refusal and every warning goes to stderr with the prefix -below, so a caller can parse the one and strip the other. The parse door is stdlib only, plus -`reconcile.parse_value` for the expected-value column; the write doors go through AH-100's models, -which is what the guarded import below is for. +below, so a caller can parse the one and strip the other. The parse door is stdlib only — `_xlsx` +reads a workbook with `zipfile` and `ElementTree` — plus `reconcile.parse_value` for the +expected-value column; the write doors go through AH-100's models, which is what the guarded import +below is for. """ from __future__ import annotations @@ -58,8 +60,10 @@ _agami_lib.ensure_importable() -# A sibling script and stdlib-only, so it is imported plainly: it has none of the dependencies the -# guard below exists for. +# Sibling scripts and stdlib-only, so they are imported plainly: they have none of the dependencies +# the guard below exists for. `_xlsx` reads a workbook for the parse door; `reconcile` normalizes +# the expected-value column. +import _xlsx import reconcile try: @@ -243,18 +247,49 @@ def _cell(row: list[str], index: Optional[int]) -> str: return row[index].strip() -def _read_rows(path: str) -> list[list[str]]: - """The CSV as rows, blank lines included. +# How far down a sheet the header is looked for. A workbook often opens with a title, a date or a note +# above the table, and a CSV exported from one keeps them. Twenty rows covers a title block without +# reading so far into the data that a question which happens to read "question" could be taken for +# the header. +_HEADER_SCAN_ROWS = 20 + + +def _read_rows(path: str, sheet: Optional[str] = None) -> tuple[Optional[str], list[list[str]]]: + """The file as rows, blank lines included, and the sheet they came from (None for a CSV). Blank rows are kept because `skipped` reports a row number a person uses to find the row in their own sheet, and dropping anything ahead of the numbering makes every number after it point at the wrong line. + + A workbook is read by `_xlsx`, one named sheet at a time, into the same rows a CSV yields. + Everything after this function is one parse for both, so there is still exactly one place a + column can be misread. """ + suffix = Path(path).suffix.lower() + if suffix == ".xlsx": + return _xlsx.read_sheet(str(Path(path).expanduser()), sheet) + if suffix == ".xls": + raise _xlsx.WorkbookError( + "this is Excel's older binary .xls format, which cannot be read here — save it as " + ".xlsx (or CSV) from Excel and re-invoke" + ) with Path(path).expanduser().open(newline="", encoding="utf-8-sig") as handle: - return list(csv.reader(handle)) + return None, list(csv.reader(handle)) + + +def _header_index(rows: list[list[str]]) -> Optional[int]: + """Which row is the header: the first, within `_HEADER_SCAN_ROWS`, that names a question column. + + An exact alias match, never a guess at the first non-empty row: a title line above the table + names no column, and the row that does is the header by the same rule `_columns` applies to it. + """ + for index, row in enumerate(rows[:_HEADER_SCAN_ROWS]): + if "query" in _columns(row): + return index + return None -def _parse_rows(header: list[str], body: list[list[str]]) -> dict[str, Any]: +def _parse_rows(header: list[str], body: list[list[str]], first_row: int = 2) -> dict[str, Any]: """The rows and the skips, in sheet order. Every row is accounted for in exactly one of the two lists. A sheet that comes back shorter @@ -269,11 +304,11 @@ def _parse_rows(header: list[str], body: list[list[str]]) -> dict[str, Any]: # here would turn a clash they need to see into two rows that both look fine. derived: dict[str, int] = {} - # From 2, not 1: `body` is everything after the header, and a header is mandatory, so the - # first data row is the sheet's second line. Numbering from 1 here would report a number one - # short of the row a person opens, which is worse than no number at all — they look at a line - # that holds a perfectly good question and cannot see what was wrong with it. - for number, row in enumerate(body, start=2): + # `first_row` is the sheet's own number for the row after the header — 2 when the header is the + # first line, later when a title sits above it. Numbering from anything else would report a + # number that is not the row a person opens, which is worse than no number at all: they look at + # a line holding a perfectly good question and cannot see what was wrong with it. + for number, row in enumerate(body, start=first_row): query = _cell(row, columns.get("query")) if not query: skipped.append({"row": number, "reason": "empty question"}) @@ -310,7 +345,7 @@ def _parse_rows(header: list[str], body: list[list[str]]) -> dict[str, Any]: } -def _parse(path: str) -> Optional[dict[str, Any]]: +def _parse(path: str, sheet: Optional[str] = None) -> Optional[dict[str, Any]]: """The whole parse, or None having said on stderr why there is not one. Both refusals are the same event: no column can be identified as the question. Never a fallback @@ -319,17 +354,30 @@ def _parse(path: str) -> Optional[dict[str, Any]]: the cells it actually read, because that list is the whole of what the person needs to rename a column and re-invoke. - An alias match decides whether row 0 is the header, and `_looks_like_header` only chooses which + An alias match decides which row is the header, and `_looks_like_header` only chooses which sentence to refuse with. That ordering is deliberate: the alias set is exact, so a cell folding to `question` is a header and nothing else, whereas reconcile's heuristic reads the SECOND cell and answers `False` for a one-column sheet — which is the shape a question bank most often has. + The header may sit below a title block, within `_HEADER_SCAN_ROWS`; a refusal still quotes the + first row with anything in it, because that is the line the person sees at the top of the sheet. + + A workbook that cannot be used — several sheets and none named, a name that is no sheet, a file + that is not a workbook — is refused with `_xlsx`'s own sentence, which lists the sheets. """ - all_rows = _read_rows(path) - if not all_rows: + try: + sheet_name, all_rows = _read_rows(path, sheet) + except _xlsx.WorkbookError as exc: + _stop(str(exc)) + return None + if sheet is not None and sheet_name is None: + _warn("--sheet only applies to a workbook; this CSV holds one table and it was read") + filled = [row for row in all_rows if any(cell.strip() for cell in row)] + if not filled: _stop("this file is empty — the sheet needs a header row naming its question column") return None - header = all_rows[0] - if "query" not in _columns(header): + header_index = _header_index(all_rows) + if header_index is None: + header = filled[0] cells = ", ".join(repr(cell.strip()) for cell in header) if reconcile._looks_like_header(header): _stop( @@ -342,7 +390,15 @@ def _parse(path: str) -> Optional[dict[str, Any]]: f"first row reads: {cells}. Add a header naming one column 'question'" ) return None - payload = _parse_rows(header, all_rows[1:]) + payload = _parse_rows( + all_rows[header_index], all_rows[header_index + 1 :], first_row=header_index + 2 + ) + # Which row the header was found on, and which sheet, so the confirmation table can say what was + # read — a workbook with the wrong tab named parses cleanly and is only caught by a person + # seeing its name. + payload["header_row"] = header_index + 1 + if sheet_name is not None: + payload["sheet"] = sheet_name if payload["skipped"]: # The counts are in the payload, but a person reading a terminal sees the summary line, and # a skip they never notice is a question missing from their dataset. @@ -1068,7 +1124,7 @@ def _dispatch(args: argparse.Namespace) -> int: so that nothing else can invent one. """ if args.cmd == "parse": - payload = _parse(args.csv) + payload = _parse(args.source, args.sheet) if payload is None: return _CANNOT_START print(json.dumps(payload, indent=2)) @@ -1106,8 +1162,18 @@ def main(argv: Optional[list[str]] = None) -> int: parser = argparse.ArgumentParser(description="Author golden-dataset items from a spreadsheet.") sub = parser.add_subparsers(dest="cmd", required=True) - parse_cmd = sub.add_parser("parse", help="Read a question-bank CSV and print what it holds.") - parse_cmd.add_argument("--csv", required=True, help="the question bank to read") + parse_cmd = sub.add_parser( + "parse", help="Read a question bank — a CSV or an .xlsx sheet — and print what it holds." + ) + # `--csv` stays as a second spelling of the same flag: every caller written before a workbook + # could be read passes it, and a CSV it names is still read exactly as before. + parse_cmd.add_argument( + "--file", "--csv", dest="source", required=True, help="the question bank to read" + ) + parse_cmd.add_argument( + "--sheet", + help="for a workbook, the sheet holding the questions; required when it has more than one", + ) import_cmd = sub.add_parser("import", help="Write confirmed parse rows as unverified items.") import_cmd.add_argument("--rows", required=True, help="the confirmed `parse` payload") diff --git a/plugins/agami/shared/golden-dataset-shape.md b/plugins/agami/shared/golden-dataset-shape.md index 068bafe1..91c7b3b9 100644 --- a/plugins/agami/shared/golden-dataset-shape.md +++ b/plugins/agami/shared/golden-dataset-shape.md @@ -166,7 +166,8 @@ wrong thing. Two supported ways in, and both land the same shape. **`/agami-save-golden` is the skill that writes these.** It has two doors: a -question bank (a CSV, or a table pasted into chat) imports as items, after the +question bank (a CSV, one sheet of an Excel `.xlsx` workbook, or a table pasted +into chat) imports as items, after the parsed rows have been shown and agreed to — a row with no statement lands `sql_confirmed: false` (a question with nothing yet to check), and a row that already carries a statement lands `sql_confirmed: true` with `confirmed_by` diff --git a/plugins/agami/skills/agami-save-golden/SKILL.md b/plugins/agami/skills/agami-save-golden/SKILL.md index 733fd99d..d74dd9f8 100644 --- a/plugins/agami/skills/agami-save-golden/SKILL.md +++ b/plugins/agami/skills/agami-save-golden/SKILL.md @@ -1,6 +1,6 @@ --- name: agami-save-golden -description: "Writes golden-dataset items for a profile through two doors. The import door turns a question bank — a CSV, or a table pasted into chat — into items after the parsed rows have been shown and agreed to: a row that already carries a statement is written confirmed, a bare question is written unconfirmed. The save door writes one question, the statement that answered it and the result the person accepted, as a confirmed item. The curation door applies the changes queued on the golden-dataset explorer page, which may weaken a claim and may never grant one. Every write goes through agami-core's writer, is re-read by the runner's own reader before it is kept, and is append-only: a write that would change an item that already exists stops and shows the before and the after. This skill writes only; it never runs or scores a dataset." +description: "Writes golden-dataset items for a profile through two doors. The import door turns a question bank — a CSV, one sheet of an Excel .xlsx workbook, or a table pasted into chat — into items after the parsed rows have been shown and agreed to: a row that already carries a statement is written confirmed, a bare question is written unconfirmed. The save door writes one question, the statement that answered it and the result the person accepted, as a confirmed item. The curation door applies the changes queued on the golden-dataset explorer page, which may weaken a claim and may never grant one. Every write goes through agami-core's writer, is re-read by the runner's own reader before it is kept, and is append-only: a write that would change an item that already exists stops and shows the before and the after. This skill writes only; it never runs or scores a dataset." when_to_use: "Use when the user says 'save this as a golden question', 'add this to the golden dataset', 'import my question bank', 'turn this spreadsheet into a golden dataset', 'this answer is correct — remember it as ground truth', 'show me the golden datasets', 'what does this dataset not test', 'apply my queued changes', or '/agami-save-golden ' — any ask to record a question, or a bank of questions, that the model should be scored against later. Also use when the user replies with a back-channel block from a previously-rendered golden-dataset explorer page (first line `profile: `, then `golden-ops:` and a JSON array, ending `done`) — paste it and nothing else is needed. Requires agami-connect to have been run first (needs a profile with a semantic model). To RUN a dataset and see the verdicts, use `/agami-eval` instead: that skill reads and scores, this one writes and never runs or scores." argument-hint: "[dataset-name]" --- @@ -65,24 +65,22 @@ Route on what the user brought, and say which door you are opening: ## Phase 2: The import door -### 2a — Get a CSV +### 2a — Get the file -The parser reads **one format**, and that is deliberate: one parser is one place where a column can be misread. +The parse reads the question bank straight from the file, and there is still **one parser**: a workbook is read into rows and then parsed exactly as a CSV is, so there is one place a column can be misread. - **A `.csv` path** — use it as given. -- **A `.xlsx` / `.xls` path** — refuse and say how to get past it. Do not try to read it, and do not guess at its contents: - - > I can't read `.xlsx` directly. Open it in Excel (or Numbers / Google Sheets) and **Save As → CSV UTF-8**, then re-invoke me with the `.csv` path. - +- **A `.xlsx` path** — use it as given. Do not open it with the Read tool (it cannot read a workbook) and do not guess at its contents; the parse reads it. A workbook often holds several sheets — a cover page, a schema reference, the questions — and **which sheet holds the questions is the user's call, not yours**. Run the parse without `--sheet` first: a workbook with more than one sheet stops with exit `2` and lists every sheet. Show the list, suggest the one whose name reads like the question bank, and re-run with `--sheet ""` once they confirm. +- **A `.xls` path** (Excel's older binary format) — it cannot be read. Ask the user to save it as `.xlsx` (or CSV) from Excel and re-invoke. - **A table pasted into chat** — write it out as a CSV with the **Write tool** and then parse that file. One parser, one code path, and the file is also the thing the user can fix and re-run. Per [`shared/invocation-conventions.md`](../../shared/invocation-conventions.md): **never a heredoc, never `python3 -c`, never a shell variable** — quoting mangles the commas and quotes that are the whole point of a CSV. Write it to `/tmp/agami-golden-pasted-.csv` and tell the user where it went. -The sheet needs a header row with a **question column** — `question`, `query`, `nl question`, `prompt` or `ask` (case, underscores and hyphens all fold). Optional columns: `id`, `expected` / `expected value` / `answer`, `sql` / `statement`, `tags`. A header the contract does not know is left alone — matching is exact, never fuzzy, so an analyst's note column costs nothing. +The table needs a header row with a **question column** — `question`, `query`, `nl question`, `prompt` or `ask` (case, underscores and hyphens all fold). It does not have to be the first row: a title or a note above the table is fine, and the parse takes the first row within the top 20 that names a question column. Optional columns: `id`, `expected` / `expected value` / `answer`, `sql` / `statement`, `tags`. A header the contract does not know is left alone — matching is exact, never fuzzy, so an analyst's note column costs nothing. The same exactness means a column is only picked up under one of those names: a statement column headed `Warehouse SQL` is not `sql`, so if the rows come back without the statements the user expected, say which header held them and have them rename it. ### 2b — Parse (this writes nothing) ```bash python3 "$AGAMI_PLUGIN_ROOT/scripts/golden_author.py" parse \ - --csv \ + --file [--sheet ""] \ > /tmp/agami-golden-parse-.json ``` @@ -272,13 +270,15 @@ python3 "$AGAMI_PLUGIN_ROOT/scripts/golden_author.py" save \ | Exit `2` | Cannot start. Read the `agami-save-golden:` line on stderr — it names the cause. Nothing was written; a rolled-back write left the previous bytes exactly as they were. | | `agami-save-golden: this file is empty` / `no column here holds the question` / `this file has no header row` | The sheet's question column can't be identified. The refusal lists every header it read — quote it back, ask which column holds the question, and have them rename it (or add a header row) and re-invoke. Never guess at column 0: a bank of ids imported as questions fails every future run in a way that looks exactly like a model regression. | | `agami-save-golden: N row(s) were skipped` on a successful parse | A warning, not a stop. List every entry in `skipped` with its row number and reason before asking for the import — a question silently missing from a dataset is the failure this line exists to prevent. | -| A `.xlsx` / `.xls` path | Refuse with the Save As → CSV UTF-8 instruction (Phase 2a). Do not attempt to read it and do not reconstruct its contents from memory. | +| `agami-save-golden: this workbook has N sheets, so which one holds the questions has to be named with --sheet` | Not a fault — the workbook has more than one tab. List the sheets the message names, suggest the one that reads like the question bank, and on the user's word re-run with `--sheet ""`. Never pick one yourself. | +| `agami-save-golden: this workbook has no sheet named ''` | The name matched no tab (case and surrounding spaces are already forgiven). Show the sheets the message lists and re-run with the one the user means. | +| `agami-save-golden: this is Excel's older binary .xls format` / `this file is not a readable .xlsx workbook` | The file can't be read as a workbook. Ask the user to save it as `.xlsx` (or CSV) from Excel and re-invoke. Do not open it with the Read tool, and do not reconstruct its contents from memory. | | `agami-save-golden: this item does not say how its answer was confirmed` | `confirmed_by.method` was blank. Ask how the result was checked and re-write the item JSON — provenance is most of what a receipt is for. | | `agami-save-golden: '' is not a usable dataset name` / `profile name` | The stem or the profile was a path, not a name. Ask for the plain name (`orders`, not `orders/2024` or `../orders`) and re-invoke. Nothing was read and nothing was written. | | `agami-save-golden: dataset '' names the file rather than the dataset` | The extension was typed too. The stem *is* the dataset's name, so pass `orders`, not `orders.yaml`. Re-invoke; nothing was written. | | `agami-save-golden: this batch carries the id '' twice` | The sheet's own `id` column repeats a key, so two questions would land under one. Nothing was written. Show the user the two rows and ask which keeps the id. | | `agami-save-golden: this does not fit a golden case — …` | The item JSON is a shape the dataset reader refuses — most often `match: bounded` with no `bounds` block, or a `sql: null` on a save. The sentence names the field and the reason (never the value). Fix the item JSON and re-run. | -| `agami-save-golden: does not exist` / `this file is not readable JSON` | The `--csv` / `--rows` / `--item` path is wrong or the file you wrote is truncated. Re-write it with the Write tool and re-run; nothing was written. | +| `agami-save-golden: does not exist` / `this file is not readable JSON` | The `--file` / `--rows` / `--item` path is wrong or the file you wrote is truncated. Re-write it with the Write tool and re-run; nothing was written. | | `agami-save-golden: .yaml cannot be read as it stands` | The existing dataset has a fault that costs it a case, so nothing may be merged into it — a merge into a file the reader can't fully read would drop whatever it couldn't parse. Report the finding, point at [`shared/golden-dataset-shape.md`](../../shared/golden-dataset-shape.md), and let the user fix the named case first. (A dataset that merely *reports* a relative question over a frozen answer key is not this: that finding drops nothing, and writing to the dataset still works.) | | A relative question refused at save time | The window slides and the SQL doesn't. Anchor the statement to the current date, or rewrite the question to name its window, then re-invoke. Don't save it "for now". | | `golden_author's write doors need agami-core and its model extra` | The plugin's interpreter is missing `agami-core[model]`. Route to `/agami-connect`, which sets the environment up; nothing was written. | diff --git a/tests/test_ah109_save_golden_skill.py b/tests/test_ah109_save_golden_skill.py index 620dadd3..595205b9 100644 --- a/tests/test_ah109_save_golden_skill.py +++ b/tests/test_ah109_save_golden_skill.py @@ -36,6 +36,15 @@ def test_the_skill_carries_the_four_frontmatter_keys(): assert 'argument-hint: "[dataset-name]"' in FRONTMATTER +def test_a_workbook_is_parsed_rather_than_refused_and_the_sheet_is_the_users_call(): + """#261. The import door used to refuse `.xlsx` and ask for a CSV export. The parse reads a + workbook now, so the skill hands one over instead of refusing it — and never picks the sheet + itself, because a real workbook's first tab is as likely to be a cover page as the questions.""" + assert "Save As → CSV UTF-8" not in SKILL + assert "--file" in SKILL and "--sheet" in SKILL + assert "which sheet holds the questions is the user's call" in SKILL + + def test_the_skill_refuses_in_plan_mode(): """SC-9. Every door here writes to the model tree, so none of them can proceed read-only. diff --git a/tests/test_golden_author_xlsx.py b/tests/test_golden_author_xlsx.py new file mode 100644 index 00000000..2bd01795 --- /dev/null +++ b/tests/test_golden_author_xlsx.py @@ -0,0 +1,346 @@ +"""The import door reads an `.xlsx` workbook: one named sheet, a header wherever it starts, rows +numbered as Excel numbers them. + +Every workbook here is built in the test from the format's own parts — a zip of XML — so each +fixture is synthetic and a few kilobytes, and the reader is exercised on the structures a real +workbook actually has: shared strings with rich-text runs and phonetic guides, inline strings, typed +cells, sparse cells, a title block above the header, rows Excel formats but never fills, and more +than one sheet. Every question is over the shipped sample store database. +""" + +from __future__ import annotations + +import json +import sys +import zipfile +from pathlib import Path +from typing import Any, Optional +from xml.sax.saxutils import escape + +import pytest + +pytest.importorskip("pydantic") + +REPO_ROOT = Path(__file__).resolve().parent.parent +PKG_SRC = REPO_ROOT / "packages" / "agami-core" / "src" +if str(PKG_SRC) not in sys.path: + sys.path.insert(0, str(PKG_SRC)) +sys.path.insert(0, str(REPO_ROOT / "plugins" / "agami" / "scripts")) + +import _xlsx # noqa: E402 +import golden_author # noqa: E402 + +MAIN = "http://schemas.openxmlformats.org/spreadsheetml/2006/main" +REL = "http://schemas.openxmlformats.org/officeDocument/2006/relationships" +PACKAGE = "http://schemas.openxmlformats.org/package/2006/relationships" + +QUERY = "How many orders have been placed?" +SQL = "SELECT COUNT(*) AS order_count FROM orders" +PROFILE = "demo" + + +def _column(index: int) -> str: + letters, index = "", index + 1 + while index: + index, remainder = divmod(index - 1, 26) + letters = chr(65 + remainder) + letters + return letters + + +def _workbook( + tmp_path: Path, + sheets: dict[str, dict[int, list]], + filename: str = "bank.xlsx", + replace_parts: Optional[dict[str, str]] = None, +) -> str: + """A workbook with one worksheet per entry, each a map of Excel row number to cells. + + A cell is a str (a shared string), an int (a number), None (no cell element at all), or a tuple: + ("inline", text), ("rich", [run, ...]) — a shared string in runs, with a phonetic guide attached — + ("bool", value), ("error", code), or ("styled",) — a cell Excel formats but never fills. + """ + shared: list[Any] = [] + + def _shared(item: Any) -> int: + shared.append(item) + return len(shared) - 1 + + parts: dict[str, str] = {} + for number, rows in enumerate(sheets.values(), start=1): + body = [] + for row_number, cells in rows.items(): + xml_cells = [] + for index, cell in enumerate(cells): + ref = f"{_column(index)}{row_number}" + if cell is None: + continue + if isinstance(cell, tuple): + kind = cell[0] + if kind == "inline": + xml_cells.append(f'{escape(cell[1])}') + elif kind == "rich": + xml_cells.append(f'{_shared(cell)}') + elif kind == "bool": + xml_cells.append(f'{1 if cell[1] else 0}') + elif kind == "error": + xml_cells.append(f'{escape(cell[1])}') + elif kind == "styled": + xml_cells.append(f'') + elif isinstance(cell, int): + xml_cells.append(f'{cell}') + else: + xml_cells.append(f'{_shared(cell)}') + body.append(f'{"".join(xml_cells)}') + parts[f"xl/worksheets/sheet{number}.xml"] = ( + f'' + f'{"".join(body)}' + ) + + entries = "".join( + f'' + for n, name in enumerate(sheets, start=1) + ) + parts["xl/workbook.xml"] = ( + f'' + f"{entries}" + ) + relationships = "".join( + f'' + for n in range(1, len(sheets) + 1) + ) + parts["xl/_rels/workbook.xml.rels"] = ( + f'' + f"{relationships}" + ) + items = [] + for item in shared: + if isinstance(item, tuple): + runs = "".join(f"{escape(run)}" for run in item[1]) + items.append(f'{runs}PHONETIC') + else: + items.append(f"{escape(item)}") + parts["xl/sharedStrings.xml"] = ( + f'{"".join(items)}' + ) + parts.update(replace_parts or {}) + + path = tmp_path / filename + with zipfile.ZipFile(path, "w") as book: + for name, text in parts.items(): + book.writestr(name, text) + return str(path) + + +def _parse(tmp_path, monkeypatch, capsys, *argv: str): + """Run the parse verb and return (exit code, stdout payload, stderr).""" + monkeypatch.setenv("AGAMI_ARTIFACTS_DIR", str(tmp_path)) + code = golden_author.main(["parse", *argv]) + captured = capsys.readouterr() + return code, (json.loads(captured.out) if captured.out.strip() else None), captured.err + + +def test_a_workbook_parses_to_the_same_rows_as_the_csv_it_would_export( + tmp_path, monkeypatch, capsys +): + """One parser. A workbook and the CSV Excel would export from it produce the same rows, the same + ids and the same skips — the reader only turns the sheet into rows, and nothing downstream knows + which kind of file it came from.""" + workbook = _workbook( + tmp_path, + { + "Bank": { + 1: ["question", "sql", "tags"], + 2: [QUERY, SQL, "orders, smoke"], + 3: ["How many customers are on file?", None, "customers"], + } + }, + ) + sheet = tmp_path / "bank.csv" + sheet.write_text( + f'question,sql,tags\n{QUERY},{SQL},"orders, smoke"\nHow many customers are on file?,,customers\n', + encoding="utf-8", + ) + + code, from_workbook, _ = _parse(tmp_path, monkeypatch, capsys, "--file", workbook) + _, from_csv, _ = _parse(tmp_path, monkeypatch, capsys, "--csv", str(sheet)) + + assert code == 0 + assert from_workbook["rows"] == from_csv["rows"] + assert from_workbook["skipped"] == from_csv["skipped"] == [] + assert from_workbook["sheet"] == "Bank" and from_workbook["header_row"] == 1 + + +def test_the_header_is_found_below_a_title_block_and_skips_keep_excels_row_numbers( + tmp_path, monkeypatch, capsys +): + """A real question bank opens with a title, and the rows below it are numbered by Excel. A skip + is reported at the number a person sees in their own workbook — including across a row the file + omits entirely.""" + workbook = _workbook( + tmp_path, + { + "Questions": { + 1: ["Order questions for the quarterly review"], + # Row 2 is omitted from the file altogether. + 3: ["Q#", "Question", "Owner"], + 4: [1, QUERY, "analyst"], + 5: [2, ("styled",), "analyst"], + 6: [3, "How many customers are on file?", "analyst"], + } + }, + ) + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", workbook) + + assert code == 0 + assert payload["header_row"] == 3 + assert [row["query"] for row in payload["rows"]] == [QUERY, "How many customers are on file?"] + assert payload["skipped"] == [{"row": 5, "reason": "empty question"}] + assert "1 row(s) were skipped" in err + + +def test_rows_excel_formats_but_never_fills_are_not_reported_as_skips( + tmp_path, monkeypatch, capsys +): + """A sheet formatted down to row 1000 has not got a thousand questions in it. Reporting every + formatted-but-empty row as a skipped question would bury the one skip that matters.""" + workbook = _workbook( + tmp_path, + {"Bank": {1: ["question"], 2: [QUERY], 3: [("styled",)], 500: [("styled",)], 1000: []}}, + ) + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", workbook) + + assert code == 0 + assert payload["summary"] == {"parsed": 1, "skipped": 0} + assert "skipped" not in err + + +def test_a_workbook_with_several_sheets_asks_which_one_and_names_them(tmp_path, monkeypatch, capsys): + """Which tab holds the questions is the person's call — a workbook's first tab is as often a + cover page — so the parse stops and lists every sheet rather than guessing.""" + workbook = _workbook( + tmp_path, + { + "Overview": {1: ["About this workbook"]}, + "Questions": {1: ["question"], 2: [QUERY]}, + }, + ) + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", workbook) + assert code == 2 and payload is None + assert "'Overview'" in err and "'Questions'" in err and "--sheet" in err + + code, payload, _ = _parse(tmp_path, monkeypatch, capsys, "--file", workbook, "--sheet", "Questions") + assert code == 0 and payload["sheet"] == "Questions" and payload["summary"]["parsed"] == 1 + + # A person types the tab's name the way they remember it. + code, payload, _ = _parse(tmp_path, monkeypatch, capsys, "--file", workbook, "--sheet", " questions") + assert code == 0 and payload["sheet"] == "Questions" + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", workbook, "--sheet", "Answers") + assert code == 2 and payload is None + assert "'Answers'" in err and "'Overview'" in err and "'Questions'" in err + + +def test_cells_read_as_what_the_sheet_shows(tmp_path): + """Rich text joined, a phonetic guide left out, an inline string, a boolean, a number — and an + error value blank rather than imported as a question that reads '#N/A'.""" + workbook = _workbook( + tmp_path, + { + "Only": { + 1: [ + ("rich", ["How many ", "orders", "?"]), + ("inline", "typed straight into the cell"), + ("bool", True), + 42, + ("error", "#N/A"), + ] + } + }, + ) + + name, rows = _xlsx.read_sheet(workbook, None) + + assert name == "Only" + assert rows == [["How many orders?", "typed straight into the cell", "TRUE", "42", ""]] + assert "PHONETIC" not in json.dumps(rows) + + +def test_sparse_cells_land_in_their_own_columns(tmp_path): + """Excel writes no element for an empty cell, so position in the row is not the column. A value + in column C with nothing in B has to land in C — and past Z, in AB.""" + cells: list = ["question", None, "tags"] + [None] * 24 + ["far column"] + workbook = _workbook(tmp_path, {"Only": {1: cells}}) + + _, rows = _xlsx.read_sheet(workbook, None) + + assert rows[0][0] == "question" and rows[0][1] == "" and rows[0][2] == "tags" + assert rows[0][27] == "far column" + + +def test_an_old_xls_file_is_refused_with_a_way_past_it(tmp_path, monkeypatch, capsys): + """The older binary format is not a zip of XML and cannot be read without a dependency. The + refusal says how to get past it rather than failing to open a zip.""" + old = tmp_path / "bank.xls" + old.write_bytes(b"\xd0\xcf\x11\xe0\xa1\xb1\x1a\xe1") + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", str(old)) + + assert code == 2 and payload is None + assert ".xlsx" in err and "save" in err.lower() and "Traceback" not in err + + +def test_a_file_named_xlsx_that_is_not_a_workbook_is_refused_rather_than_raising( + tmp_path, monkeypatch, capsys +): + fake = tmp_path / "bank.xlsx" + fake.write_text("question\nHow many orders have been placed?\n", encoding="utf-8") + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", str(fake)) + + assert code == 2 and payload is None + assert "not a readable .xlsx workbook" in err and "Traceback" not in err + + +def test_a_workbook_part_declaring_a_doctype_is_refused(tmp_path, monkeypatch, capsys): + """A spreadsheet part never declares a DTD, and one that does is how an XML parser is made to + expand entities without bound. Refused before it is parsed.""" + bomb = ( + '' + ']>' + f'&b;' + "" + ) + workbook = _workbook( + tmp_path, + {"Only": {1: ["question"]}}, + replace_parts={"xl/worksheets/sheet1.xml": bomb}, + ) + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", workbook) + + assert code == 2 and payload is None + assert "will not parse" in err + + +def test_parsing_a_workbook_writes_nothing(tmp_path, monkeypatch, capsys): + """SC-3 holds for a workbook exactly as for a CSV: the parse has no write in it.""" + workbook = _workbook(tmp_path, {"Bank": {1: ["question"], 2: [QUERY]}}) + + code, _, _ = _parse(tmp_path, monkeypatch, capsys, "--file", workbook) + + assert code == 0 + assert not (tmp_path / PROFILE / "golden_datasets").exists() + + +def test_a_sheet_named_for_a_csv_is_noted_and_the_csv_still_parses(tmp_path, monkeypatch, capsys): + """A CSV has one table, so `--sheet` has nothing to choose. Said, and not treated as a failure.""" + sheet = tmp_path / "bank.csv" + sheet.write_text(f"question\n{QUERY}\n", encoding="utf-8") + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", str(sheet), "--sheet", "Bank") + + assert code == 0 and payload["summary"]["parsed"] == 1 + assert "only applies to a workbook" in err From ca351f0764dd09db7805493143e6de0aa9d39e7c Mon Sep 17 00:00:00 2001 From: Ashwin Ramachandran Date: Sun, 13 Sep 2026 17:27:22 -0700 Subject: [PATCH 2/3] Harden the workbook reader against crafted files, and never drop a row unsaid Copilot's review on #312, all nine findings: - The DTD guard searched for ASCII bytes, which a UTF-16 part never contains. It is now expat's own check, after the part's encoding is honoured. - A row number or column letter was used as a list length unchecked, so a tiny cell could allocate billions of entries. Both are checked against Excel's limits before they place anything, the sheet is held sparsely, and only rows up to the last filled one are built, under a cell budget. - Strict OOXML workbooks are read: namespaces come from each part's own root, and the workbook and shared strings are found by relationship, not path. - A name matching two tabs loosely is reported as ambiguous, not missing. - Header scanning is for workbooks only. A CSV's header is still its first non-empty row, so a question reading "question" cannot make the rows above it vanish; in a workbook, every row above the header is reported. - The skill names the sheet and header row before the rows, documents the new payload fields, and has recovery steps for every workbook refusal. - The CHANGELOG says trailing rows, which is what is dropped. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 14 +- plugins/agami/scripts/_xlsx.py | 274 ++++++++++++------ plugins/agami/scripts/golden_author.py | 47 ++- .../agami/skills/agami-save-golden/SKILL.md | 6 +- tests/test_golden_author_xlsx.py | 260 +++++++++++++---- 5 files changed, 437 insertions(+), 164 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b1ab7a0d..471e3106 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,11 +18,15 @@ below corresponds to one such version. sheet of an `.xlsx` into the same parse a CSV goes through, with the standard library alone — a workbook is a zip of XML, so no dependency is added to a plugin that installs with none. A workbook with several sheets stops and names them, because which tab holds the questions is the person's - call; `--sheet` names it. The header no longer has to be the first row: the first row within the - top 20 that names a question column is taken, so a title block above the table is fine, and rows - keep Excel's own numbering, so a skipped row is reported at the number the person sees. Rows Excel - formats but never fills are dropped rather than reported as empty questions. An `.xls` file — - Excel's older binary format — is still refused, with how to get past it, and `--csv` keeps + call; `--sheet` names it. In a workbook the header no longer has to be the first row: the first row + within the top 20 that names a question column is taken, so a title block above the table is fine, + and every row above the header is reported rather than silently dropped. A CSV's header is still + its first row. Rows keep Excel's own numbering, so a skipped row is reported at the number the + person sees, and trailing rows Excel formats but never fills are dropped rather than reported as + empty questions. Both the Transitional and Strict flavours of `.xlsx` are read. Every coordinate + in the file is checked against Excel's own limits before it is used, and a part declaring a DTD is + refused in any encoding, so a crafted workbook costs a refusal rather than memory. An `.xls` file + — Excel's older binary format — is still refused, with how to get past it, and `--csv` keeps working. (#261) ### Changed diff --git a/plugins/agami/scripts/_xlsx.py b/plugins/agami/scripts/_xlsx.py index 133399c3..604048b4 100644 --- a/plugins/agami/scripts/_xlsx.py +++ b/plugins/agami/scripts/_xlsx.py @@ -3,15 +3,22 @@ A question bank lives in Excel, and the import door is the one surface that has to open such a file itself: the Read tool cannot open a workbook, and a model reconstructing one would be a second parser with no way to check it. So this reads the workbook the way the format is defined — a zip of XML -parts — with `zipfile` and `ElementTree`, and adds no dependency to a plugin that installs with none. +parts — with `zipfile`, `expat` and `ElementTree`, and adds no dependency to a plugin that installs +with none. It reads what a person SEES, not what Excel computes: a cell's stored value, a formula's cached result, a shared string with its rich-text runs joined. Nothing is recalculated, no style is applied, and a date comes through as the serial number Excel stores for it. That is enough for a column of -questions, ids, statements and tags, which is all the import door asks of a sheet. +questions, ids, statements and tags, which is all the import door asks of a sheet. Both conformance +classes are read — Transitional, which Excel saves by default, and Strict — because they differ only +in the namespaces their parts declare. Rows keep Excel's own numbering — a row the file omits is an empty row, not a missing one — because the parse reports skipped rows by number, and a person finds them by that number in their workbook. + +The file is untrusted input in every coordinate it carries, not only in its size: a row number or a +column letter is used to place a value, so each is checked against Excel's own limits before it is +used, and the sheet is held sparsely until it is known how much of it actually has anything in it. """ from __future__ import annotations @@ -21,18 +28,32 @@ import xml.etree.ElementTree as ET import zipfile from typing import Optional +from xml.parsers import expat -_MAIN = "{http://schemas.openxmlformats.org/spreadsheetml/2006/main}" -_REL = "{http://schemas.openxmlformats.org/officeDocument/2006/relationships}" +# The package-level relationships namespace, which both conformance classes share. The spreadsheet +# vocabulary and the relationship-id attribute are NOT shared — Strict moves them under +# `purl.oclc.org` — so those are read off each part's own root element rather than hard-coded. _PACKAGE_REL = "{http://schemas.openxmlformats.org/package/2006/relationships}" -# The largest single part this will decompress. A sheet of questions is kilobytes; a part expanding -# past this is not a question bank, and reading it whole is how a crafted file exhausts memory. The -# images and embedded objects a workbook carries are never read, so a workbook full of screenshots -# does not come near it. +# Excel's own limits. A coordinate past either is not something Excel wrote, and refusing it before +# it is used to place a value is what keeps a few-byte cell from allocating a billion-entry row. +_MAX_EXCEL_ROWS = 1_048_576 +_MAX_EXCEL_COLUMNS = 16_384 + +# How far down a sheet this reads, and how much it builds. A question bank is hundreds of rows by +# tens of columns. Data further down than this, or a sheet that expands past this many cells once +# empties are trimmed — one filled cell far to the right on every row, say — is not a question +# bank, and building it would be how a small file exhausts memory. +_MAX_READ_ROWS = 100_000 +_MAX_CELLS = 2_000_000 + +# The largest single part this will decompress. The images and embedded objects a workbook carries +# are never read, so a workbook full of screenshots does not come near it. _MAX_PART_BYTES = 64 * 1024 * 1024 -_CELL_REFERENCE = re.compile(r"([A-Z]+)(\d+)") +_CELL_REFERENCE = re.compile(r"([A-Z]{1,3})[0-9]{1,7}") + +_UNPARSEABLE = "this workbook contains XML this reader will not parse" class WorkbookError(ValueError): @@ -42,7 +63,7 @@ class WorkbookError(ValueError): def sheet_names(path: str) -> list[str]: """Every sheet in the workbook, in the order Excel shows its tabs.""" with _open(path) as book: - return [name for name, _ in _sheets(book)] + return [name for name, _ in _workbook(book)[0]] def read_sheet(path: str, sheet: Optional[str]) -> tuple[str, list[list[str]]]: @@ -53,10 +74,11 @@ def read_sheet(path: str, sheet: Optional[str]) -> tuple[str, list[list[str]]]: be a cover page, so the caller is told to ask rather than handed a guess. A name is matched exactly, then once more ignoring case and surrounding space, so `questions` - finds a tab called `Questions ` — but only when that looser match is unambiguous. + finds a tab called `Questions `. If that looser match finds more than one tab, the name is + ambiguous and says so, rather than claiming no tab matched. """ with _open(path) as book: - sheets = _sheets(book) + sheets, shared_strings_part = _workbook(book) if sheet is None: if len(sheets) != 1: raise WorkbookError( @@ -69,12 +91,17 @@ def read_sheet(path: str, sheet: Optional[str]) -> tuple[str, list[list[str]]]: if not matches: wanted = sheet.strip().casefold() matches = [entry for entry in sheets if entry[0].strip().casefold() == wanted] - if len(matches) != 1: + if len(matches) > 1: + raise WorkbookError( + f"more than one sheet matches {sheet!r} once case and spaces are ignored — " + f"name one exactly. Matching sheets: {_listed(matches)}" + ) + if not matches: raise WorkbookError( f"this workbook has no sheet named {sheet!r}. Sheets: {_listed(sheets)}" ) name, part = matches[0] - return name, _rows(_xml(book, part), _shared_strings(book)) + return name, _rows(_xml(book, part), _shared_strings(book, shared_strings_part)) def _open(path: str) -> zipfile.ZipFile: @@ -93,12 +120,7 @@ def _open(path: str) -> zipfile.ZipFile: def _xml(book: zipfile.ZipFile, part: str) -> ET.Element: - """One XML part, parsed — refused if it is oversized, missing, malformed or declares a DTD. - - A spreadsheet part never declares a DTD. One that does is either not a workbook or is built to - make an XML parser expand entities, and refusing it outright is cheaper than reasoning about - which. - """ + """One XML part, parsed — refused if it is oversized, missing, malformed or declares a DTD.""" try: info = book.getinfo(part) except KeyError as exc: @@ -106,103 +128,191 @@ def _xml(book: zipfile.ZipFile, part: str) -> ET.Element: if info.file_size > _MAX_PART_BYTES: raise WorkbookError("a part of this workbook is too large to read as a question bank") data = book.read(info) - if b" list[tuple[str, str]]: - """Each sheet's name and the zip part holding it, resolved through the workbook's relationships. +def _refuse_a_dtd(data: bytes, part: str) -> None: + """Refuse a part that declares a DTD or an entity, however the part is encoded. - A sheet's part is not guaranteed to be `sheet.xml` for the Nth tab — reordering tabs in Excel - reorders the names and leaves the files — so the relationship is followed rather than assumed. + A spreadsheet part never declares one. One that does is either not a workbook or built to make + an XML parser expand entities without bound, so it is refused outright. The check is expat's own + — it has already honoured the part's declared encoding, UTF-16 included — rather than a search + for bytes spelling ` None: + raise WorkbookError(_UNPARSEABLE) + + detector.StartDoctypeDeclHandler = _declared + detector.EntityDeclHandler = _declared + try: + detector.Parse(data, True) + except expat.ExpatError as exc: + raise WorkbookError(f"this workbook has a damaged part ({part})") from exc + + +def _namespace(element: ET.Element) -> str: + """The `{uri}` prefix an element's own tag carries — which conformance class wrote this part.""" + return element.tag[: element.tag.index("}") + 1] if element.tag.startswith("{") else "" + + +def _workbook(book: zipfile.ZipFile) -> tuple[list[tuple[str, str]], Optional[str]]: + """Each sheet's name and part, and where the shared strings live — all by relationship. + + Nothing is assumed about file names. The workbook part is found from the package's root + relationships; a sheet's part is not guaranteed to be `sheet.xml` for the Nth tab (reordering + tabs in Excel reorders the names and leaves the files); and the shared strings are wherever the + workbook says they are. + """ + workbook_part = _workbook_part(book) + base = posixpath.dirname(workbook_part) + workbook = _xml(book, workbook_part) + main = _namespace(workbook) + relationships = _xml( + book, posixpath.join(base, "_rels", posixpath.basename(workbook_part) + ".rels") + ) + + targets: dict[str, str] = {} + shared_strings: Optional[str] = None + for relationship in relationships.iter(_PACKAGE_REL + "Relationship"): + part = _resolve(base, relationship.get("Target") or "") + targets[relationship.get("Id") or ""] = part + if (relationship.get("Type") or "").endswith("/sharedStrings"): + shared_strings = part + found: list[tuple[str, str]] = [] - for sheet in workbook.iter(_MAIN + "sheet"): - target = targets.get(sheet.get(_REL + "id"), "") - # Relative to `xl/`, unless written as an absolute path inside the package. - if target.startswith("/"): - part = target.lstrip("/") - else: - part = posixpath.normpath(posixpath.join("xl", target)) - found.append((sheet.get("name") or "", part)) + for sheet in workbook.iter(main + "sheet"): + # The `r:id` attribute's namespace also differs between the two conformance classes, so it + # is found by its local name. + relationship_id = next( + (value for key, value in sheet.attrib.items() if key.endswith("}id")), "" + ) + found.append((sheet.get("name") or "", targets.get(relationship_id, ""))) if not found: raise WorkbookError("this workbook has no sheets") - return found + return found, shared_strings + + +def _workbook_part(book: zipfile.ZipFile) -> str: + """Where the workbook part is, from the package's root relationships, else where Excel puts it.""" + if "_rels/.rels" in book.namelist(): + for relationship in _xml(book, "_rels/.rels").iter(_PACKAGE_REL + "Relationship"): + if (relationship.get("Type") or "").endswith("/officeDocument"): + return _resolve("", relationship.get("Target") or "") + return "xl/workbook.xml" -def _shared_strings(book: zipfile.ZipFile) -> list[str]: +def _resolve(base: str, target: str) -> str: + """A relationship target as a zip part name: relative to `base` unless written as absolute.""" + if target.startswith("/"): + return target.lstrip("/") + return posixpath.normpath(posixpath.join(base, target)) + + +def _shared_strings(book: zipfile.ZipFile, part: Optional[str]) -> list[str]: """The workbook's shared string table. Most text cells are an index into it.""" - if "xl/sharedStrings.xml" not in book.namelist(): + if not part or part not in book.namelist(): return [] - return [_text(item) for item in _xml(book, "xl/sharedStrings.xml").iter(_MAIN + "si")] + table = _xml(book, part) + main = _namespace(table) + return [_text(item, main) for item in table.iter(main + "si")] -def _text(item: ET.Element) -> str: +def _text(item: ET.Element, main: str) -> str: """A string item's text: its own ``, then every rich-text run's, in order. Only direct children are read. A phonetic guide (``) also carries a ``, and a whole-subtree search would splice its reading into the middle of the cell. """ - parts = [item.findtext(_MAIN + "t") or ""] - parts += [run.findtext(_MAIN + "t") or "" for run in item.findall(_MAIN + "r")] + parts = [item.findtext(main + "t") or ""] + parts += [run.findtext(main + "t") or "" for run in item.findall(main + "r")] return "".join(parts) def _rows(sheet: ET.Element, strings: list[str]) -> list[list[str]]: - """Every row of the sheet as text, placed at Excel's row number and each cell at its column. + """Every row up to the last with anything in it, at Excel's row number, each cell at its column. - Trailing rows with nothing in them are dropped: a sheet formatted down to row 1000 has not got - 950 empty questions in it, and reporting each as a skip would bury the skips that matter. Empty - rows BETWEEN filled ones are kept, because they hold the numbering of everything after them. + Held sparsely first — row number to column to value, filled cells only — so that a coordinate is + checked before it places anything, and nothing is built for a row or a column that holds nothing. + Trailing rows with nothing in them are never built: a sheet formatted down to row 1000 has not + got 950 empty questions in it. Empty rows BETWEEN filled ones are built, because they hold the + numbering of everything after them. """ + main = _namespace(sheet) + filled: dict[int, dict[int, str]] = {} + previous_row = 0 + for row in sheet.iter(main + "row"): + number = _row_number(row.get("r"), previous_row + 1) + previous_row = number + previous_column = -1 + for cell in row.findall(main + "c"): + column = _column(cell.get("r"), previous_column + 1) + previous_column = column + value = _value(cell, strings, main) + if value.strip(): + filled.setdefault(number, {})[column] = value + if not filled: + return [] + last = max(filled) + if last > _MAX_READ_ROWS: + raise WorkbookError( + f"this sheet has data on row {last:,}, further down than a question bank runs — " + "if the questions are near the top, delete the rows below them and re-invoke" + ) rows: list[list[str]] = [] - for row in sheet.iter(_MAIN + "row"): - number = int(row.get("r") or len(rows) + 1) - # Excel omits a row it has nothing to say about; that row is still one a person counts. - while len(rows) < number - 1: - rows.append([]) - cells: list[str] = [] - for cell in row.findall(_MAIN + "c"): - reference = _CELL_REFERENCE.fullmatch(cell.get("r") or "") - column = _column_index(reference.group(1)) if reference else len(cells) - while len(cells) < column: - cells.append("") - value = _value(cell, strings) - if column < len(cells): - cells[column] = value - else: - cells.append(value) - rows.append(cells) - while rows and not any(cell.strip() for cell in rows[-1]): - rows.pop() + built = 0 + for number in range(1, last + 1): + cells = filled.get(number, {}) + width = max(cells) + 1 if cells else 0 + built += width + 1 + if built > _MAX_CELLS: + raise WorkbookError("this sheet is too large to read as a question bank") + rows.append([cells.get(column, "") for column in range(width)]) return rows -def _column_index(letters: str) -> int: - """`A` is 0, `Z` is 25, `AA` is 26 — Excel's column letters as a list index.""" - index = 0 - for letter in letters: - index = index * 26 + (ord(letter) - ord("A") + 1) - return index - 1 - - -def _value(cell: ET.Element, strings: list[str]) -> str: +def _row_number(raw: Optional[str], default: int) -> int: + """A row's number, checked against Excel's limit before it is used to place anything.""" + if raw is None: + number = default + elif raw.isdigit(): + number = int(raw) + else: + raise WorkbookError(f"this workbook has a row with an unreadable number ({raw!r})") + if not 1 <= number <= _MAX_EXCEL_ROWS: + raise WorkbookError(f"this workbook has a row numbered {number}, outside anything Excel writes") + return number + + +def _column(reference: Optional[str], default: int) -> int: + """A cell's column index, from its reference (`C7` is 2), checked against Excel's last column.""" + if reference is None: + index = default + else: + match = _CELL_REFERENCE.fullmatch(reference) + if not match: + raise WorkbookError(f"this workbook has a cell with an unreadable reference ({reference!r})") + index = 0 + for letter in match.group(1): + index = index * 26 + (ord(letter) - ord("A") + 1) + index -= 1 + if not 0 <= index < _MAX_EXCEL_COLUMNS: + raise WorkbookError("this workbook has a cell past Excel's last column") + return index + + +def _value(cell: ET.Element, strings: list[str], main: str) -> str: """A cell as text, by the type Excel recorded for it.""" kind = cell.get("t") if kind == "inlineStr": - inline = cell.find(_MAIN + "is") - return _text(inline) if inline is not None else "" - raw = cell.findtext(_MAIN + "v") or "" + inline = cell.find(main + "is") + return _text(inline, main) if inline is not None else "" + raw = cell.findtext(main + "v") or "" if kind == "s": try: return strings[int(raw)] diff --git a/plugins/agami/scripts/golden_author.py b/plugins/agami/scripts/golden_author.py index 41edc082..7117d71d 100644 --- a/plugins/agami/scripts/golden_author.py +++ b/plugins/agami/scripts/golden_author.py @@ -247,10 +247,10 @@ def _cell(row: list[str], index: Optional[int]) -> str: return row[index].strip() -# How far down a sheet the header is looked for. A workbook often opens with a title, a date or a note -# above the table, and a CSV exported from one keeps them. Twenty rows covers a title block without -# reading so far into the data that a question which happens to read "question" could be taken for -# the header. +# How far down a WORKBOOK the header is looked for. A workbook often opens with a title, a date or a +# note above the table. Twenty rows covers a title block without reading so far into the data that a +# question which happens to read "question" could be taken for the header — and the rows above the +# header are reported, never silently dropped. _HEADER_SCAN_ROWS = 20 @@ -277,15 +277,26 @@ def _read_rows(path: str, sheet: Optional[str] = None) -> tuple[Optional[str], l return None, list(csv.reader(handle)) -def _header_index(rows: list[list[str]]) -> Optional[int]: - """Which row is the header: the first, within `_HEADER_SCAN_ROWS`, that names a question column. +def _header_index(rows: list[list[str]], *, workbook: bool) -> Optional[int]: + """Which row is the header, or None when no row can be. - An exact alias match, never a guess at the first non-empty row: a title line above the table - names no column, and the row that does is the header by the same rule `_columns` applies to it. + In a workbook it is the first row, within `_HEADER_SCAN_ROWS`, that names a question column — by + the same exact alias match `_columns` applies to any header, never a guess. A workbook often + opens with a title block, and `_parse` reports every row above the header rather than dropping + it unsaid. + + In a CSV it is the first row with anything in it, as it always was. A CSV has no title block to + scan past, and scanning one would turn a headerless bank whose second question happens to read + "question" into a bank whose first question silently vanished. """ - for index, row in enumerate(rows[:_HEADER_SCAN_ROWS]): - if "query" in _columns(row): - return index + if workbook: + for index, row in enumerate(rows[:_HEADER_SCAN_ROWS]): + if "query" in _columns(row): + return index + return None + for index, row in enumerate(rows): + if any(cell.strip() for cell in row): + return index if "query" in _columns(row) else None return None @@ -375,7 +386,7 @@ def _parse(path: str, sheet: Optional[str] = None) -> Optional[dict[str, Any]]: if not filled: _stop("this file is empty — the sheet needs a header row naming its question column") return None - header_index = _header_index(all_rows) + header_index = _header_index(all_rows, workbook=sheet_name is not None) if header_index is None: header = filled[0] cells = ", ".join(repr(cell.strip()) for cell in header) @@ -399,6 +410,18 @@ def _parse(path: str, sheet: Optional[str] = None) -> Optional[dict[str, Any]]: payload["header_row"] = header_index + 1 if sheet_name is not None: payload["sheet"] = sheet_name + # Every row above the header that had anything in it — usually a title. Not read, and said, + # because a row the parse passes over without a word is a question somebody can lose. + payload["above_header"] = [ + {"row": index + 1, "text": next(cell.strip() for cell in row if cell.strip())[:120]} + for index, row in enumerate(all_rows[:header_index]) + if any(cell.strip() for cell in row) + ] + if payload["above_header"]: + _warn( + f"{len(payload['above_header'])} row(s) above the header on row " + f"{header_index + 1} were not read — see `above_header` in the payload" + ) if payload["skipped"]: # The counts are in the payload, but a person reading a terminal sees the summary line, and # a skip they never notice is a question missing from their dataset. diff --git a/plugins/agami/skills/agami-save-golden/SKILL.md b/plugins/agami/skills/agami-save-golden/SKILL.md index d74dd9f8..5748829d 100644 --- a/plugins/agami/skills/agami-save-golden/SKILL.md +++ b/plugins/agami/skills/agami-save-golden/SKILL.md @@ -86,10 +86,12 @@ python3 "$AGAMI_PLUGIN_ROOT/scripts/golden_author.py" parse \ Then `Read` the file. `parse` is inert **by construction** — it has no write in it at all, which is what makes "the rows are confirmed before anything is written" a fact about the software rather than a promise about how you behave. -The payload carries `columns` (the header as found), `rows`, `skipped` (each with its row number in the user's own sheet and why) and `summary`. Exit `2` means no column could be identified as the question; the stderr line names every header it read, which is the whole of what the user needs to rename one and re-invoke. +The payload carries `columns` (the header as found), `header_row` (the sheet's own row number for that header), `rows`, `skipped` (each with its row number in the user's own sheet and why) and `summary`. A workbook adds `sheet` (the tab that was read) and `above_header` (every row above the header that had anything in it — usually a title — none of which was read). Exit `2` means the parse could not start: no column could be identified as the question — the stderr line names every header it read, which is the whole of what the user needs to rename one and re-invoke — or, for a workbook, the sheet has to be named or the file can't be read, and the stderr line says which. ### 2c — Render the rows and get an explicit yes +**Say what was read, before the rows.** For a workbook, name the sheet and the header row (*"Read the **Questions** sheet, header on row 4."*) and quote every `above_header` row, so the user can see nothing they meant as a question sat above the header. A right-looking table from the wrong tab is the one mistake the rows alone won't show. + **Show the rows as a markdown table.** Not a count, not a summary — the rows: ```markdown @@ -273,6 +275,8 @@ python3 "$AGAMI_PLUGIN_ROOT/scripts/golden_author.py" save \ | `agami-save-golden: this workbook has N sheets, so which one holds the questions has to be named with --sheet` | Not a fault — the workbook has more than one tab. List the sheets the message names, suggest the one that reads like the question bank, and on the user's word re-run with `--sheet ""`. Never pick one yourself. | | `agami-save-golden: this workbook has no sheet named ''` | The name matched no tab (case and surrounding spaces are already forgiven). Show the sheets the message lists and re-run with the one the user means. | | `agami-save-golden: this is Excel's older binary .xls format` / `this file is not a readable .xlsx workbook` | The file can't be read as a workbook. Ask the user to save it as `.xlsx` (or CSV) from Excel and re-invoke. Do not open it with the Read tool, and do not reconstruct its contents from memory. | +| `agami-save-golden: more than one sheet matches '' once case and spaces are ignored` | Two tabs differ only in case or a trailing space. Show the matching sheets the message lists and re-run with `--sheet` spelled exactly as the tab the user means. | +| `agami-save-golden: this workbook contains XML this reader will not parse` / `… has a damaged part` / `… is missing a part` / `a part of this workbook is too large` / `… has a row …` / `… has a cell …` / `this sheet has data on row …` / `this sheet is too large` | The workbook can't be read as a question bank as it stands — damaged, not written by Excel, or far larger than a question bank runs. Nothing was read. Ask the user to re-save it from Excel as `.xlsx`, or to copy just the question table into a fresh workbook or a CSV, and re-invoke. Never try to open it another way. | | `agami-save-golden: this item does not say how its answer was confirmed` | `confirmed_by.method` was blank. Ask how the result was checked and re-write the item JSON — provenance is most of what a receipt is for. | | `agami-save-golden: '' is not a usable dataset name` / `profile name` | The stem or the profile was a path, not a name. Ask for the plain name (`orders`, not `orders/2024` or `../orders`) and re-invoke. Nothing was read and nothing was written. | | `agami-save-golden: dataset '' names the file rather than the dataset` | The extension was typed too. The stem *is* the dataset's name, so pass `orders`, not `orders.yaml`. Re-invoke; nothing was written. | diff --git a/tests/test_golden_author_xlsx.py b/tests/test_golden_author_xlsx.py index 2bd01795..6e6c459c 100644 --- a/tests/test_golden_author_xlsx.py +++ b/tests/test_golden_author_xlsx.py @@ -1,20 +1,22 @@ """The import door reads an `.xlsx` workbook: one named sheet, a header wherever it starts, rows -numbered as Excel numbers them. +numbered as Excel numbers them — and a crafted workbook costs it a refusal, not memory. Every workbook here is built in the test from the format's own parts — a zip of XML — so each fixture is synthetic and a few kilobytes, and the reader is exercised on the structures a real workbook actually has: shared strings with rich-text runs and phonetic guides, inline strings, typed -cells, sparse cells, a title block above the header, rows Excel formats but never fills, and more -than one sheet. Every question is over the shipped sample store database. +cells, sparse cells, a title block above the header, rows Excel formats but never fills, more than +one sheet, and the Strict conformance class alongside the Transitional one. Every question is over +the shipped sample store database. """ from __future__ import annotations import json import sys +import time import zipfile from pathlib import Path -from typing import Any, Optional +from typing import Any, Optional, Union from xml.sax.saxutils import escape import pytest @@ -30,9 +32,16 @@ import _xlsx # noqa: E402 import golden_author # noqa: E402 -MAIN = "http://schemas.openxmlformats.org/spreadsheetml/2006/main" -REL = "http://schemas.openxmlformats.org/officeDocument/2006/relationships" +TRANSITIONAL = ( + "http://schemas.openxmlformats.org/spreadsheetml/2006/main", + "http://schemas.openxmlformats.org/officeDocument/2006/relationships", +) +STRICT = ( + "http://purl.oclc.org/ooxml/spreadsheetml/main", + "http://purl.oclc.org/ooxml/officeDocument/relationships", +) PACKAGE = "http://schemas.openxmlformats.org/package/2006/relationships" +MAIN = TRANSITIONAL[0] QUERY = "How many orders have been placed?" SQL = "SELECT COUNT(*) AS order_count FROM orders" @@ -51,7 +60,8 @@ def _workbook( tmp_path: Path, sheets: dict[str, dict[int, list]], filename: str = "bank.xlsx", - replace_parts: Optional[dict[str, str]] = None, + replace_parts: Optional[dict[str, Union[str, bytes]]] = None, + strict: bool = False, ) -> str: """A workbook with one worksheet per entry, each a map of Excel row number to cells. @@ -59,13 +69,14 @@ def _workbook( ("inline", text), ("rich", [run, ...]) — a shared string in runs, with a phonetic guide attached — ("bool", value), ("error", code), or ("styled",) — a cell Excel formats but never fills. """ + main, rel = STRICT if strict else TRANSITIONAL shared: list[Any] = [] def _shared(item: Any) -> int: shared.append(item) return len(shared) - 1 - parts: dict[str, str] = {} + parts: dict[str, Union[str, bytes]] = {} for number, rows in enumerate(sheets.values(), start=1): body = [] for row_number, cells in rows.items(): @@ -77,7 +88,9 @@ def _shared(item: Any) -> int: if isinstance(cell, tuple): kind = cell[0] if kind == "inline": - xml_cells.append(f'{escape(cell[1])}') + xml_cells.append( + f'{escape(cell[1])}' + ) elif kind == "rich": xml_cells.append(f'{_shared(cell)}') elif kind == "bool": @@ -92,7 +105,7 @@ def _shared(item: Any) -> int: xml_cells.append(f'{_shared(cell)}') body.append(f'{"".join(xml_cells)}') parts[f"xl/worksheets/sheet{number}.xml"] = ( - f'' + f'' f'{"".join(body)}' ) @@ -101,17 +114,25 @@ def _shared(item: Any) -> int: for n, name in enumerate(sheets, start=1) ) parts["xl/workbook.xml"] = ( - f'' + f'' f"{entries}" ) relationships = "".join( - f'' + f'' for n in range(1, len(sheets) + 1) ) + relationships += ( + f'' + ) parts["xl/_rels/workbook.xml.rels"] = ( f'' f"{relationships}" ) + parts["_rels/.rels"] = ( + f'' + f'' + "" + ) items = [] for item in shared: if isinstance(item, tuple): @@ -120,17 +141,25 @@ def _shared(item: Any) -> int: else: items.append(f"{escape(item)}") parts["xl/sharedStrings.xml"] = ( - f'{"".join(items)}' + f'{"".join(items)}' ) parts.update(replace_parts or {}) path = tmp_path / filename with zipfile.ZipFile(path, "w") as book: - for name, text in parts.items(): - book.writestr(name, text) + for name, content in parts.items(): + book.writestr(name, content) return str(path) +def _sheet_part(rows_xml: str) -> str: + """A raw worksheet part, for coordinates the builder would never write.""" + return ( + f'{rows_xml}' + "" + ) + + def _parse(tmp_path, monkeypatch, capsys, *argv: str): """Run the parse verb and return (exit code, stdout payload, stderr).""" monkeypatch.setenv("AGAMI_ARTIFACTS_DIR", str(tmp_path)) @@ -139,6 +168,9 @@ def _parse(tmp_path, monkeypatch, capsys, *argv: str): return code, (json.loads(captured.out) if captured.out.strip() else None), captured.err +# --- reading what the sheet shows --------------------------------------------------------------- + + def test_a_workbook_parses_to_the_same_rows_as_the_csv_it_would_export( tmp_path, monkeypatch, capsys ): @@ -168,14 +200,26 @@ def test_a_workbook_parses_to_the_same_rows_as_the_csv_it_would_export( assert from_workbook["rows"] == from_csv["rows"] assert from_workbook["skipped"] == from_csv["skipped"] == [] assert from_workbook["sheet"] == "Bank" and from_workbook["header_row"] == 1 + assert from_workbook["above_header"] == [] + + +def test_a_strict_open_xml_workbook_reads_like_a_transitional_one(tmp_path): + """Strict is a valid `.xlsx` too — it only declares different namespaces. A reader that knew one + family would find no sheets in the other and call a good workbook broken.""" + sheets = {"Bank": {1: ["question", "tags"], 2: [QUERY, "orders"]}} + transitional = _workbook(tmp_path, sheets, filename="transitional.xlsx") + strict = _workbook(tmp_path, sheets, filename="strict.xlsx", strict=True) + + assert _xlsx.read_sheet(strict, None) == _xlsx.read_sheet(transitional, None) + assert _xlsx.read_sheet(strict, None)[1] == [["question", "tags"], [QUERY, "orders"]] def test_the_header_is_found_below_a_title_block_and_skips_keep_excels_row_numbers( tmp_path, monkeypatch, capsys ): """A real question bank opens with a title, and the rows below it are numbered by Excel. A skip - is reported at the number a person sees in their own workbook — including across a row the file - omits entirely.""" + is reported at the number a person sees in their own workbook — across a row the file omits + entirely — and the title above the header is reported as not read, never silently dropped.""" workbook = _workbook( tmp_path, { @@ -196,24 +240,70 @@ def test_the_header_is_found_below_a_title_block_and_skips_keep_excels_row_numbe assert payload["header_row"] == 3 assert [row["query"] for row in payload["rows"]] == [QUERY, "How many customers are on file?"] assert payload["skipped"] == [{"row": 5, "reason": "empty question"}] - assert "1 row(s) were skipped" in err + assert payload["above_header"] == [ + {"row": 1, "text": "Order questions for the quarterly review"} + ] + assert "1 row(s) were skipped" in err and "above the header" in err def test_rows_excel_formats_but_never_fills_are_not_reported_as_skips( tmp_path, monkeypatch, capsys ): - """A sheet formatted down to row 1000 has not got a thousand questions in it. Reporting every - formatted-but-empty row as a skipped question would bury the one skip that matters.""" + """A sheet formatted down to its last row has not got a million questions in it — and building + a million empty rows to find that out would be its own failure. The trailing rows are never + built, however far down the formatting goes.""" workbook = _workbook( tmp_path, - {"Bank": {1: ["question"], 2: [QUERY], 3: [("styled",)], 500: [("styled",)], 1000: []}}, + {"Bank": {1: ["question"], 2: [QUERY], 3: [("styled",)], 1_048_576: [("styled",)]}}, ) + started = time.monotonic() code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", workbook) assert code == 0 assert payload["summary"] == {"parsed": 1, "skipped": 0} assert "skipped" not in err + assert time.monotonic() - started < 5 + + +def test_cells_read_as_what_the_sheet_shows(tmp_path): + """Rich text joined, a phonetic guide left out, an inline string, a boolean, a number — and an + error value blank rather than imported as a question that reads '#N/A'.""" + workbook = _workbook( + tmp_path, + { + "Only": { + 1: [ + ("rich", ["How many ", "orders", "?"]), + ("inline", "typed straight into the cell"), + ("bool", True), + 42, + ("error", "#N/A"), + ] + } + }, + ) + + name, rows = _xlsx.read_sheet(workbook, None) + + assert name == "Only" + assert rows == [["How many orders?", "typed straight into the cell", "TRUE", "42"]] + assert "PHONETIC" not in json.dumps(rows) + + +def test_sparse_cells_land_in_their_own_columns(tmp_path): + """Excel writes no element for an empty cell, so position in the row is not the column. A value + in column C with nothing in B has to land in C — and past Z, in AB.""" + cells: list = ["question", None, "tags"] + [None] * 24 + ["far column"] + workbook = _workbook(tmp_path, {"Only": {1: cells}}) + + _, rows = _xlsx.read_sheet(workbook, None) + + assert rows[0][0] == "question" and rows[0][1] == "" and rows[0][2] == "tags" + assert rows[0][27] == "far column" + + +# --- choosing the sheet --------------------------------------------------------------------------- def test_a_workbook_with_several_sheets_asks_which_one_and_names_them(tmp_path, monkeypatch, capsys): @@ -243,41 +333,54 @@ def test_a_workbook_with_several_sheets_asks_which_one_and_names_them(tmp_path, assert "'Answers'" in err and "'Overview'" in err and "'Questions'" in err -def test_cells_read_as_what_the_sheet_shows(tmp_path): - """Rich text joined, a phonetic guide left out, an inline string, a boolean, a number — and an - error value blank rather than imported as a question that reads '#N/A'.""" +def test_a_name_matching_two_tabs_loosely_is_called_ambiguous_not_missing( + tmp_path, monkeypatch, capsys +): + """Two tabs that differ only in case or a trailing space both match a loosely typed name. That + is an ambiguity to resolve, and "no sheet named" would send the person looking for a tab that + is sitting right there.""" workbook = _workbook( tmp_path, - { - "Only": { - 1: [ - ("rich", ["How many ", "orders", "?"]), - ("inline", "typed straight into the cell"), - ("bool", True), - 42, - ("error", "#N/A"), - ] - } - }, + {"Questions": {1: ["question"], 2: [QUERY]}, "questions ": {1: ["question"], 2: [QUERY]}}, ) - name, rows = _xlsx.read_sheet(workbook, None) + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", workbook, "--sheet", "QUESTIONS") - assert name == "Only" - assert rows == [["How many orders?", "typed straight into the cell", "TRUE", "42", ""]] - assert "PHONETIC" not in json.dumps(rows) + assert code == 2 and payload is None + assert "more than one sheet matches" in err and "'Questions'" in err and "'questions '" in err + assert "no sheet named" not in err -def test_sparse_cells_land_in_their_own_columns(tmp_path): - """Excel writes no element for an empty cell, so position in the row is not the column. A value - in column C with nothing in B has to land in C — and past Z, in AB.""" - cells: list = ["question", None, "tags"] + [None] * 24 + ["far column"] - workbook = _workbook(tmp_path, {"Only": {1: cells}}) +# --- the header, and what is never silently dropped ----------------------------------------------- - _, rows = _xlsx.read_sheet(workbook, None) - assert rows[0][0] == "question" and rows[0][1] == "" and rows[0][2] == "tags" - assert rows[0][27] == "far column" +def test_a_csv_header_is_still_its_first_row_so_no_question_vanishes_above_one( + tmp_path, monkeypatch, capsys +): + """A CSV has no title block to scan past. Scanning one would turn a headerless bank whose second + question happens to read "question" into a bank whose first question silently vanished — so a + CSV is refused unless its first row with anything in it is the header, as it always was.""" + sheet = tmp_path / "bank.csv" + sheet.write_text(f"{QUERY}\nquestion\nHow many customers are on file?\n", encoding="utf-8") + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", str(sheet)) + + assert code == 2 and payload is None + assert "no header row" in err and QUERY in err + + +def test_a_sheet_named_for_a_csv_is_noted_and_the_csv_still_parses(tmp_path, monkeypatch, capsys): + """A CSV has one table, so `--sheet` has nothing to choose. Said, and not treated as a failure.""" + sheet = tmp_path / "bank.csv" + sheet.write_text(f"question\n{QUERY}\n", encoding="utf-8") + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", str(sheet), "--sheet", "Bank") + + assert code == 0 and payload["summary"]["parsed"] == 1 + assert "only applies to a workbook" in err + + +# --- files that are not workbooks, and workbooks built to hurt ------------------------------------ def test_an_old_xls_file_is_refused_with_a_way_past_it(tmp_path, monkeypatch, capsys): @@ -304,15 +407,21 @@ def test_a_file_named_xlsx_that_is_not_a_workbook_is_refused_rather_than_raising assert "not a readable .xlsx workbook" in err and "Traceback" not in err -def test_a_workbook_part_declaring_a_doctype_is_refused(tmp_path, monkeypatch, capsys): +@pytest.mark.parametrize("encoding", ["utf-8", "utf-16"]) +def test_a_part_declaring_a_doctype_is_refused_in_any_encoding( + tmp_path, monkeypatch, capsys, encoding +): """A spreadsheet part never declares a DTD, and one that does is how an XML parser is made to - expand entities without bound. Refused before it is parsed.""" + expand entities without bound. Refused before it is parsed — including in UTF-16, where no + byte sequence spells `' - ']>' + f'' + ']>' f'&b;' "" - ) + ).encode(encoding) + if encoding == "utf-16": + assert b"x', + # A row number that is not a number. + 'x', + # A column past Excel's last one (XFD), used as a list length it would be a wide row. + 'x', + # A cell reference that is not a reference. + 'x', + # A real Excel coordinate, but data further down than any question bank runs. + 'x', + ], +) +def test_a_coordinate_outside_what_a_question_bank_holds_is_refused_before_it_allocates( + tmp_path, monkeypatch, capsys, rows_xml +): + """A few bytes of XML can name any row and any column. Each coordinate is checked before it + places a value, so a crafted cell costs a refusal rather than the memory to build its row.""" + workbook = _workbook( + tmp_path, + {"Only": {1: ["question"]}}, + replace_parts={"xl/worksheets/sheet1.xml": _sheet_part(rows_xml)}, + ) + + started = time.monotonic() + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", workbook) + + assert code == 2 and payload is None + assert "Traceback" not in err + assert time.monotonic() - started < 5 def test_parsing_a_workbook_writes_nothing(tmp_path, monkeypatch, capsys): @@ -333,14 +476,3 @@ def test_parsing_a_workbook_writes_nothing(tmp_path, monkeypatch, capsys): assert code == 0 assert not (tmp_path / PROFILE / "golden_datasets").exists() - - -def test_a_sheet_named_for_a_csv_is_noted_and_the_csv_still_parses(tmp_path, monkeypatch, capsys): - """A CSV has one table, so `--sheet` has nothing to choose. Said, and not treated as a failure.""" - sheet = tmp_path / "bank.csv" - sheet.write_text(f"question\n{QUERY}\n", encoding="utf-8") - - code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", str(sheet), "--sheet", "Bank") - - assert code == 0 and payload["summary"]["parsed"] == 1 - assert "only applies to a workbook" in err From 767b59267ff5819642c73904f2b2848e98ae7245 Mon Sep 17 00:00:00 2001 From: Ashwin Ramachandran Date: Sun, 13 Sep 2026 17:37:15 -0700 Subject: [PATCH 3/3] Ask about an unread column that looks like a field, and read it with --column A header like 'Warehouse SQL' is not an alias, so its statements were dropped without a word. The parse now lists such columns under 'unrecognized', the skill asks the person about each one, and --column sql="
" reads it on a yes. Matching stays exact; a mapping is never inferred. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 5 +- plugins/agami/scripts/golden_author.py | 136 ++++++++++++++++-- .../agami/skills/agami-save-golden/SKILL.md | 9 +- tests/test_ah109_save_golden_skill.py | 3 + tests/test_golden_authoring.py | 110 ++++++++++++-- 5 files changed, 233 insertions(+), 30 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 471e3106..c337571e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -27,7 +27,10 @@ below corresponds to one such version. in the file is checked against Excel's own limits before it is used, and a part declaring a DTD is refused in any encoding, so a crafted workbook costs a refusal rather than memory. An `.xls` file — Excel's older binary format — is still refused, with how to get past it, and `--csv` keeps - working. (#261) + working. A column the import doesn't read, but that looks like a field it does — a header with + `sql` or `question` in it, for a field the sheet hasn't supplied — is reported rather than + silently ignored, the skill asks the person about it, and `--column sql="
"` reads it + without renaming anything. (#261) ### Changed diff --git a/plugins/agami/scripts/golden_author.py b/plugins/agami/scripts/golden_author.py index 7117d71d..98eda46a 100644 --- a/plugins/agami/scripts/golden_author.py +++ b/plugins/agami/scripts/golden_author.py @@ -221,21 +221,58 @@ def _slug(query: str) -> str: return head.strip("-") -def _columns(header: list[str]) -> dict[str, int]: +def _columns(header: list[str], mapping: Optional[dict[str, str]] = None) -> dict[str, int]: """Which column holds which field, by index. - First match wins: a sheet with two columns folding to the same alias is one column and one - duplicate, and the person put the real one first. + A field the person mapped with `--column` is read from that column and from no alias, because + the mapping is their answer to which column they meant. Otherwise first match wins: a sheet with + two columns folding to the same alias is one column and one duplicate, and the person put the + real one first. """ + mapping = mapping or {} found: dict[str, int] = {} for index, cell in enumerate(header): folded = _fold(cell) + for field, named in mapping.items(): + if folded == _fold(named) and field not in found: + found[field] = index for field, aliases in _ALIASES.items(): - if folded in aliases and field not in found: + if field not in mapping and folded in aliases and field not in found: found[field] = index return found +# The words a header has to contain to be ASKED about — never to be read. Exact matching stays the +# only way a column is read, so this cannot misread one; what it prevents is the opposite loss, a +# `Warehouse SQL` column carrying every statement in the sheet that the import drops without a word. +_LOOK_ALIKE_WORDS: dict[str, frozenset[str]] = { + "query": frozenset({"question", "questions", "prompt", "ask"}), + "id": frozenset({"id", "key"}), + "expected_value": frozenset({"expected", "answer"}), + "sql": frozenset({"sql", "statement"}), + "tags": frozenset({"tag", "tags", "label", "labels"}), +} + + +def _unrecognized(header: list[str], columns: dict[str, int]) -> list[dict[str, str]]: + """Every unread column whose header names a field the sheet has not supplied, in sheet order. + + Only fields still missing: once `sql` is read from a column, another header saying SQL is a note + about it, and asking about it would train the person to wave the question through. + """ + used = set(columns.values()) + found = [] + for index, cell in enumerate(header): + if index in used or not cell.strip(): + continue + words = set(re.findall(r"[a-z0-9]+", _fold(cell))) + for field, candidates in _LOOK_ALIKE_WORDS.items(): + if field not in columns and words & candidates: + found.append({"column": cell.strip(), "could_be": field}) + break + return found + + def _cell(row: list[str], index: Optional[int]) -> str: """One cell, for a column the sheet may not have and a row that may stop short of it. @@ -277,7 +314,9 @@ def _read_rows(path: str, sheet: Optional[str] = None) -> tuple[Optional[str], l return None, list(csv.reader(handle)) -def _header_index(rows: list[list[str]], *, workbook: bool) -> Optional[int]: +def _header_index( + rows: list[list[str]], *, workbook: bool, mapping: Optional[dict[str, str]] = None +) -> Optional[int]: """Which row is the header, or None when no row can be. In a workbook it is the first row, within `_HEADER_SCAN_ROWS`, that names a question column — by @@ -291,23 +330,28 @@ def _header_index(rows: list[list[str]], *, workbook: bool) -> Optional[int]: """ if workbook: for index, row in enumerate(rows[:_HEADER_SCAN_ROWS]): - if "query" in _columns(row): + if "query" in _columns(row, mapping): return index return None for index, row in enumerate(rows): if any(cell.strip() for cell in row): - return index if "query" in _columns(row) else None + return index if "query" in _columns(row, mapping) else None return None -def _parse_rows(header: list[str], body: list[list[str]], first_row: int = 2) -> dict[str, Any]: +def _parse_rows( + header: list[str], + body: list[list[str]], + first_row: int = 2, + mapping: Optional[dict[str, str]] = None, +) -> dict[str, Any]: """The rows and the skips, in sheet order. Every row is accounted for in exactly one of the two lists. A sheet that comes back shorter than it went in, with nothing said about the difference, is how an import quietly loses a question nobody notices is missing. """ - columns = _columns(header) + columns = _columns(header, mapping) rows: list[dict[str, Any]] = [] skipped: list[dict[str, Any]] = [] # Derived ids only. An explicit id that repeats is a real duplicate in the person's own sheet @@ -352,11 +396,14 @@ def _parse_rows(header: list[str], body: list[list[str]], first_row: int = 2) -> "columns": header, "rows": rows, "skipped": skipped, + "unrecognized": _unrecognized(header, columns), "summary": {"parsed": len(rows), "skipped": len(skipped)}, } -def _parse(path: str, sheet: Optional[str] = None) -> Optional[dict[str, Any]]: +def _parse( + path: str, sheet: Optional[str] = None, mapping: Optional[dict[str, str]] = None +) -> Optional[dict[str, Any]]: """The whole parse, or None having said on stderr why there is not one. Both refusals are the same event: no column can be identified as the question. Never a fallback @@ -386,7 +433,7 @@ def _parse(path: str, sheet: Optional[str] = None) -> Optional[dict[str, Any]]: if not filled: _stop("this file is empty — the sheet needs a header row naming its question column") return None - header_index = _header_index(all_rows, workbook=sheet_name is not None) + header_index = _header_index(all_rows, workbook=sheet_name is not None, mapping=mapping) if header_index is None: header = filled[0] cells = ", ".join(repr(cell.strip()) for cell in header) @@ -401,8 +448,19 @@ def _parse(path: str, sheet: Optional[str] = None) -> Optional[dict[str, Any]]: f"first row reads: {cells}. Add a header naming one column 'question'" ) return None + header = all_rows[header_index] + folded = {_fold(cell) for cell in header} + for field, named in (mapping or {}).items(): + # A mapping onto a column that isn't there would parse as if the field were simply absent — + # the statements the person just said were in the sheet, dropped after they said so. + if _fold(named) not in folded: + cells = ", ".join(repr(cell.strip()) for cell in header if cell.strip()) + _stop( + f"--column maps {field!r} to {named!r}, which is not a column here. Columns found: {cells}" + ) + return None payload = _parse_rows( - all_rows[header_index], all_rows[header_index + 1 :], first_row=header_index + 2 + header, all_rows[header_index + 1 :], first_row=header_index + 2, mapping=mapping ) # Which row the header was found on, and which sheet, so the confirmation table can say what was # read — a workbook with the wrong tab named parses cleanly and is only caught by a person @@ -422,6 +480,11 @@ def _parse(path: str, sheet: Optional[str] = None) -> Optional[dict[str, Any]]: f"{len(payload['above_header'])} row(s) above the header on row " f"{header_index + 1} were not read — see `above_header` in the payload" ) + for column in payload["unrecognized"]: + _warn( + f"the column {column['column']!r} was not read but looks like `{column['could_be']}` — " + f'if it is, re-run with --column {column["could_be"]}="{column["column"]}"' + ) if payload["skipped"]: # The counts are in the payload, but a person reading a terminal sees the summary line, and # a skip they never notice is a question missing from their dataset. @@ -790,8 +853,14 @@ def _nearest_example(profile: str, question: str) -> Optional[dict]: # partial ranking can only under-report a departure, never invent one. break ranked = _sm_json( - "examples", str(root), "--area", str(name), - "--query", question, "--top-k", str(_CONVENTION_TOP_K), + "examples", + str(root), + "--area", + str(name), + "--query", + question, + "--top-k", + str(_CONVENTION_TOP_K), timeout_s=remaining, ) # `high_confidence` is the CLI's own answer to "does this library cover this question", @@ -1140,6 +1209,33 @@ def _add_write_args(cmd: argparse.ArgumentParser) -> None: cmd.add_argument("--description", help="the dataset's description") +def _mapping(values: Sequence[str]) -> Optional[dict[str, str]]: + """`--column FIELD=HEADER` values as a field-to-header map, or None having said why not. + + The map is the person's own answer to "is this the column you meant?" — it is never inferred — + so a value that is not plainly one known field and one header is refused rather than applied in + part. + """ + mapping: dict[str, str] = {} + for value in values: + field, separator, header = value.partition("=") + field, header = field.strip().lower(), header.strip() + if not separator or not field or not header: + _stop(f'--column takes FIELD=HEADER, like --column sql="Warehouse SQL" — got {value!r}') + return None + if field not in _ALIASES: + _stop( + f"--column names {field!r}, which is not a field this import reads — one of: " + f"{', '.join(_ALIASES)}" + ) + return None + if field in mapping: + _stop(f"--column maps {field!r} twice — name one column for it") + return None + mapping[field] = header + return mapping + + def _dispatch(args: argparse.Namespace) -> int: """Run the verb the arguments name. @@ -1147,7 +1243,10 @@ def _dispatch(args: argparse.Namespace) -> int: so that nothing else can invent one. """ if args.cmd == "parse": - payload = _parse(args.source, args.sheet) + mapping = _mapping(args.column) + if mapping is None: + return _CANNOT_START + payload = _parse(args.source, args.sheet, mapping) if payload is None: return _CANNOT_START print(json.dumps(payload, indent=2)) @@ -1197,6 +1296,13 @@ def main(argv: Optional[list[str]] = None) -> int: "--sheet", help="for a workbook, the sheet holding the questions; required when it has more than one", ) + parse_cmd.add_argument( + "--column", + action="append", + default=[], + metavar="FIELD=HEADER", + help="read HEADER as FIELD (query, id, expected_value, sql, tags) — only on the person's say-so", + ) import_cmd = sub.add_parser("import", help="Write confirmed parse rows as unverified items.") import_cmd.add_argument("--rows", required=True, help="the confirmed `parse` payload") diff --git a/plugins/agami/skills/agami-save-golden/SKILL.md b/plugins/agami/skills/agami-save-golden/SKILL.md index 5748829d..e95fe3e8 100644 --- a/plugins/agami/skills/agami-save-golden/SKILL.md +++ b/plugins/agami/skills/agami-save-golden/SKILL.md @@ -74,24 +74,26 @@ The parse reads the question bank straight from the file, and there is still **o - **A `.xls` path** (Excel's older binary format) — it cannot be read. Ask the user to save it as `.xlsx` (or CSV) from Excel and re-invoke. - **A table pasted into chat** — write it out as a CSV with the **Write tool** and then parse that file. One parser, one code path, and the file is also the thing the user can fix and re-run. Per [`shared/invocation-conventions.md`](../../shared/invocation-conventions.md): **never a heredoc, never `python3 -c`, never a shell variable** — quoting mangles the commas and quotes that are the whole point of a CSV. Write it to `/tmp/agami-golden-pasted-.csv` and tell the user where it went. -The table needs a header row with a **question column** — `question`, `query`, `nl question`, `prompt` or `ask` (case, underscores and hyphens all fold). It does not have to be the first row: a title or a note above the table is fine, and the parse takes the first row within the top 20 that names a question column. Optional columns: `id`, `expected` / `expected value` / `answer`, `sql` / `statement`, `tags`. A header the contract does not know is left alone — matching is exact, never fuzzy, so an analyst's note column costs nothing. The same exactness means a column is only picked up under one of those names: a statement column headed `Warehouse SQL` is not `sql`, so if the rows come back without the statements the user expected, say which header held them and have them rename it. +The table needs a header row with a **question column** — `question`, `query`, `nl question`, `prompt` or `ask` (case, underscores and hyphens all fold). It does not have to be the first row: a title or a note above the table is fine, and the parse takes the first row within the top 20 that names a question column. Optional columns: `id`, `expected` / `expected value` / `answer`, `sql` / `statement`, `tags`. A header the contract does not know is left alone — matching is exact, never fuzzy, so an analyst's note column costs nothing. The same exactness means a column is only picked up under one of those names — but a column it can't read that *looks* like a field (a header with `sql`, `statement`, `question`, `prompt`, `id`, `answer` or `tag` in it, for a field the sheet hasn't already supplied) is never dropped silently. The parse lists it under `unrecognized`, and Phase 2c asks about it. Nobody has to rename anything: `--column sql="Warehouse SQL"` reads that column as the statement. ### 2b — Parse (this writes nothing) ```bash python3 "$AGAMI_PLUGIN_ROOT/scripts/golden_author.py" parse \ - --file [--sheet ""] \ + --file [--sheet ""] [--column ="
"] \ > /tmp/agami-golden-parse-.json ``` Then `Read` the file. `parse` is inert **by construction** — it has no write in it at all, which is what makes "the rows are confirmed before anything is written" a fact about the software rather than a promise about how you behave. -The payload carries `columns` (the header as found), `header_row` (the sheet's own row number for that header), `rows`, `skipped` (each with its row number in the user's own sheet and why) and `summary`. A workbook adds `sheet` (the tab that was read) and `above_header` (every row above the header that had anything in it — usually a title — none of which was read). Exit `2` means the parse could not start: no column could be identified as the question — the stderr line names every header it read, which is the whole of what the user needs to rename one and re-invoke — or, for a workbook, the sheet has to be named or the file can't be read, and the stderr line says which. +The payload carries `columns` (the header as found), `header_row` (the sheet's own row number for that header), `rows`, `skipped` (each with its row number in the user's own sheet and why) and `summary`. `unrecognized` lists each unread column that looks like a missing field, with the field it could be. A workbook adds `sheet` (the tab that was read) and `above_header` (every row above the header that had anything in it — usually a title — none of which was read). Exit `2` means the parse could not start: no column could be identified as the question — the stderr line names every header it read, which is the whole of what the user needs to rename one and re-invoke — or, for a workbook, the sheet has to be named or the file can't be read, and the stderr line says which. ### 2c — Render the rows and get an explicit yes **Say what was read, before the rows.** For a workbook, name the sheet and the header row (*"Read the **Questions** sheet, header on row 4."*) and quote every `above_header` row, so the user can see nothing they meant as a question sat above the header. A right-looking table from the wrong tab is the one mistake the rows alone won't show. +**Ask about every `unrecognized` column, before anything else.** Each is a column the parse did not read whose header looks like a field the sheet hasn't supplied — `{"column": "Warehouse SQL", "could_be": "sql"}`. Ask about each by its header: *"The sheet has a column **Warehouse SQL** that wasn't read. Is that the SQL for each question? If so, rows that have it will import as confirmed."* On a yes, re-run the parse with `--column sql="Warehouse SQL"` (one `--column` per field) and show the new payload instead; on a no, carry on without it. Wait for the answer and never map a column on your own, even when the header plainly says SQL, because a statement column the person didn't mean becomes an answer key every future run is scored against. + **Show the rows as a markdown table.** Not a count, not a summary — the rows: ```markdown @@ -280,6 +282,7 @@ python3 "$AGAMI_PLUGIN_ROOT/scripts/golden_author.py" save \ | `agami-save-golden: this item does not say how its answer was confirmed` | `confirmed_by.method` was blank. Ask how the result was checked and re-write the item JSON — provenance is most of what a receipt is for. | | `agami-save-golden: '' is not a usable dataset name` / `profile name` | The stem or the profile was a path, not a name. Ask for the plain name (`orders`, not `orders/2024` or `../orders`) and re-invoke. Nothing was read and nothing was written. | | `agami-save-golden: dataset '' names the file rather than the dataset` | The extension was typed too. The stem *is* the dataset's name, so pass `orders`, not `orders.yaml`. Re-invoke; nothing was written. | +| `agami-save-golden: --column takes FIELD=HEADER` / `--column names '', which is not a field` / `--column maps '' twice` / `--column maps '' to '
', which is not a column` | The mapping you passed can't be applied, so nothing was parsed. Fix the flag — the refusal names the fields or the columns it read — and re-run; don't change which column you map without asking again. | | `agami-save-golden: this batch carries the id '' twice` | The sheet's own `id` column repeats a key, so two questions would land under one. Nothing was written. Show the user the two rows and ask which keeps the id. | | `agami-save-golden: this does not fit a golden case — …` | The item JSON is a shape the dataset reader refuses — most often `match: bounded` with no `bounds` block, or a `sql: null` on a save. The sentence names the field and the reason (never the value). Fix the item JSON and re-run. | | `agami-save-golden: does not exist` / `this file is not readable JSON` | The `--file` / `--rows` / `--item` path is wrong or the file you wrote is truncated. Re-write it with the Write tool and re-run; nothing was written. | diff --git a/tests/test_ah109_save_golden_skill.py b/tests/test_ah109_save_golden_skill.py index 595205b9..894da530 100644 --- a/tests/test_ah109_save_golden_skill.py +++ b/tests/test_ah109_save_golden_skill.py @@ -43,6 +43,9 @@ def test_a_workbook_is_parsed_rather_than_refused_and_the_sheet_is_the_users_cal assert "Save As → CSV UTF-8" not in SKILL assert "--file" in SKILL and "--sheet" in SKILL assert "which sheet holds the questions is the user's call" in SKILL + # A look-alike column is asked about, and only mapped on the person's word. + assert "`unrecognized`" in SKILL and '--column sql="Warehouse SQL"' in SKILL + assert "never map a column on your own" in SKILL def test_the_skill_refuses_in_plan_mode(): diff --git a/tests/test_golden_authoring.py b/tests/test_golden_authoring.py index f625cf10..e5f98fea 100644 --- a/tests/test_golden_authoring.py +++ b/tests/test_golden_authoring.py @@ -187,6 +187,85 @@ def test_header_aliases_fold_case_and_underscores(tmp_path, monkeypatch, capsys) assert payload["rows"][0]["expected_value"] == 7.0 +def _parse_with(tmp_path, monkeypatch, capsys, body: str, *extra: str): + """The parse verb over `body`, with extra arguments — (exit code, stdout payload, stderr).""" + monkeypatch.setenv("AGAMI_ARTIFACTS_DIR", str(tmp_path)) + code = golden_author.main(["parse", "--csv", _csv(tmp_path, body), *extra]) + captured = capsys.readouterr() + return code, (json.loads(captured.out) if captured.out.strip() else None), captured.err + + +def test_an_unread_column_that_looks_like_the_statement_is_reported_not_silently_ignored( + tmp_path, monkeypatch, capsys +): + """Matching stays exact, so `Warehouse SQL` is not read as `sql`. But a sheet with no statement + column and two headers saying SQL has almost certainly got its statements in one of them — and + importing without a word would drop every one. So each is reported, and the skill asks.""" + body = f"question,Warehouse SQL,Result: SQL,Notes\n{QUERY},{SQL},matched,checked\n" + + code, payload, err = _parse_with(tmp_path, monkeypatch, capsys, body) + + assert code == 0 + assert payload["rows"][0]["sql"] is None + assert payload["unrecognized"] == [ + {"column": "Warehouse SQL", "could_be": "sql"}, + {"column": "Result: SQL", "could_be": "sql"}, + ] + assert "'Warehouse SQL'" in err and "--column" in err + + +def test_a_mapped_column_is_read_as_the_field_it_is_mapped_to(tmp_path, monkeypatch, capsys): + """The person's answer, applied: once `sql` is mapped, that column is the statement — folded the + way any header is, so the spacing and case they typed don't matter — and the other look-alike + is no longer asked about, because the field is supplied.""" + body = f"question,Warehouse SQL,Result: SQL\n{QUERY},{SQL},matched\n" + + code, payload, _ = _parse_with( + tmp_path, monkeypatch, capsys, body, "--column", "sql=warehouse sql" + ) + + assert code == 0 + assert payload["rows"][0]["sql"] == SQL + assert payload["unrecognized"] == [] + + +def test_a_question_column_named_differently_can_be_mapped(tmp_path, monkeypatch, capsys): + """A question column the contract doesn't know by name is still refused on its own — never a + guess — and reads once the person says which column it is.""" + body = f"Prompt (Natural Language),sql\n{QUERY},{SQL}\n" + + code, payload, _ = _parse_with(tmp_path, monkeypatch, capsys, body) + assert code == 2 and payload is None + + code, payload, _ = _parse_with( + tmp_path, monkeypatch, capsys, body, "--column", "query=Prompt (Natural Language)" + ) + assert code == 0 + assert payload["rows"][0]["query"] == QUERY and payload["rows"][0]["sql"] == SQL + + +@pytest.mark.parametrize( + "arguments, message", + [ + (["--column", "sql"], "FIELD=HEADER"), + (["--column", "statement=Warehouse SQL"], "not a field this import reads"), + (["--column", "sql=Warehouse SQL", "--column", "sql=Result: SQL"], "twice"), + (["--column", "tags=Labels here"], "'Labels here'"), + ], +) +def test_a_column_mapping_that_cannot_apply_is_refused_rather_than_half_applied( + tmp_path, monkeypatch, capsys, arguments, message +): + """A mapping is the person's answer to "is this the column you meant?", so one that names no + known field, names a field twice, or names a header the sheet doesn't have is refused outright.""" + body = f"question,Warehouse SQL,Result: SQL\n{QUERY},{SQL},matched\n" + + code, payload, err = _parse_with(tmp_path, monkeypatch, capsys, body, *arguments) + + assert code == 2 and payload is None + assert message in err + + def test_expected_values_are_normalized_and_an_unparseable_one_is_null( tmp_path, monkeypatch, capsys ): @@ -317,7 +396,9 @@ def test_an_import_confirms_exactly_the_rows_that_carried_an_answer(tmp_path, mo assert [item.expected.sql_confirmed for item in items] == [False, True] assert items[1].expected.sql == REVENUE_SQL assert items[0].confirmed_by is None - assert items[1].confirmed_by.method == "provided as a pre-validated answer in the imported sheet" + assert ( + items[1].confirmed_by.method == "provided as a pre-validated answer in the imported sheet" + ) assert items[1].confirmed_by.at @@ -1208,7 +1289,9 @@ def test_the_departure_is_written_once_somebody_says_it_is_deliberate( sql="SELECT COUNT(*) AS order_count FROM orders WHERE placed_at >= '2024-01-01'", ) - code, _, _ = _run(tmp_path, monkeypatch, capsys, _save_argv(item, "orders", "--confirm-convention")) + code, _, _ = _run( + tmp_path, monkeypatch, capsys, _save_argv(item, "orders", "--confirm-convention") + ) assert code == 0 assert _items(tmp_path)[0].expected.sql.count("placed_at") @@ -1219,9 +1302,7 @@ def test_a_key_that_matches_the_convention_is_written_without_a_question( ): """The check has to be silent when there is nothing to say, or it becomes a prompt people learn to click through.""" - monkeypatch.setattr( - golden_author, "_nearest_example", lambda profile, question: _example(SQL) - ) + monkeypatch.setattr(golden_author, "_nearest_example", lambda profile, question: _example(SQL)) code, _, _ = _run(tmp_path, monkeypatch, capsys, _save_argv(_item_file(tmp_path))) @@ -1305,8 +1386,15 @@ def _ranked(*args, **kwargs): # judgement that the match is not close enough to be one. return { "high_confidence": False, - "matches": [{"score": 0.606, "example": {"question": "Anything else?", - "sql": "SELECT COUNT(*) FROM suppliers"}}], + "matches": [ + { + "score": 0.606, + "example": { + "question": "Anything else?", + "sql": "SELECT COUNT(*) FROM suppliers", + }, + } + ], } monkeypatch.setattr(golden_author, "_sm_json", _ranked) @@ -1374,9 +1462,7 @@ def test_a_departure_that_is_also_a_replacement_asks_both_questions_at_once( that each re-arm the other. The exit-code table already promised a payload could carry more than one key; this is the code keeping that promise. """ - monkeypatch.setattr( - golden_author, "_nearest_example", lambda profile, question: _example(SQL) - ) + monkeypatch.setattr(golden_author, "_nearest_example", lambda profile, question: _example(SQL)) # An item on disk first, so the second save is a replacement. code, _, _ = _run(tmp_path, monkeypatch, capsys, _save_argv(_item_file(tmp_path))) assert code == 0 @@ -1405,7 +1491,9 @@ def test_a_departure_that_is_also_a_replacement_asks_both_questions_at_once( # Both flags together, and it writes. code, _, _ = _run( - tmp_path, monkeypatch, capsys, + tmp_path, + monkeypatch, + capsys, _save_argv(item, "orders", "--confirm-convention", "--confirm-replace"), ) assert code == 0 and "placed_at" in _items(tmp_path)[0].expected.sql