diff --git a/CHANGELOG.md b/CHANGELOG.md index 3058a531..88a46322 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,26 @@ 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. 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. 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 - **A schema response says when a datasource has stored examples.** Clients often skipped diff --git a/plugins/agami/scripts/_xlsx.py b/plugins/agami/scripts/_xlsx.py new file mode 100644 index 00000000..604048b4 --- /dev/null +++ b/plugins/agami/scripts/_xlsx.py @@ -0,0 +1,331 @@ +"""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`, `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. 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 + +import posixpath +import re +import xml.etree.ElementTree as ET +import zipfile +from typing import Optional +from xml.parsers import expat + +# 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}" + +# 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]{1,3})[0-9]{1,7}") + +_UNPARSEABLE = "this workbook contains XML this reader will not parse" + + +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 _workbook(book)[0]] + + +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 `. 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, shared_strings_part = _workbook(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"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, shared_strings_part)) + + +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.""" + 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) + _refuse_a_dtd(data, part) + try: + return ET.fromstring(data) + except ET.ParseError as exc: + raise WorkbookError(f"this workbook has a damaged part ({part})") from exc + + +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 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"): + # 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, 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 _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 not part or part not in book.namelist(): + return [] + table = _xml(book, part) + main = _namespace(table) + return [_text(item, main) for item in table.iter(main + "si")] + + +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")] + return "".join(parts) + + +def _rows(sheet: ET.Element, strings: list[str]) -> list[list[str]]: + """Every row up to the last with anything in it, at Excel's row number, each cell at its column. + + 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]] = [] + 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 _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, main) 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..98eda46a 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: @@ -217,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. @@ -243,25 +284,74 @@ 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 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 + + +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]], *, 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 + 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. + """ + if workbook: + for index, row in enumerate(rows[:_HEADER_SCAN_ROWS]): + 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, mapping) else None + 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, + 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 @@ -269,11 +359,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"}) @@ -306,11 +396,14 @@ def _parse_rows(header: list[str], body: list[list[str]]) -> dict[str, Any]: "columns": header, "rows": rows, "skipped": skipped, + "unrecognized": _unrecognized(header, columns), "summary": {"parsed": len(rows), "skipped": len(skipped)}, } -def _parse(path: str) -> 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 @@ -319,17 +412,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, 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) if reconcile._looks_like_header(header): _stop( @@ -342,7 +448,43 @@ 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:]) + 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( + 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 + # seeing its name. + 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" + ) + 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. @@ -711,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", @@ -1061,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. @@ -1068,7 +1243,10 @@ def _dispatch(args: argparse.Namespace) -> int: so that nothing else can invent one. """ if args.cmd == "parse": - payload = _parse(args.csv) + 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)) @@ -1106,8 +1284,25 @@ 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", + ) + 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/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..e95fe3e8 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,33 +65,35 @@ 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 — 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 \ - --csv \ + --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), `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`. `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 @@ -272,13 +274,18 @@ 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: 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. | +| `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 `--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..894da530 100644 --- a/tests/test_ah109_save_golden_skill.py +++ b/tests/test_ah109_save_golden_skill.py @@ -36,6 +36,18 @@ 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 + # 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(): """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..6e6c459c --- /dev/null +++ b/tests/test_golden_author_xlsx.py @@ -0,0 +1,478 @@ +"""The import door reads an `.xlsx` workbook: one named sheet, a header wherever it starts, rows +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, 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, Union +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 + +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" +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, Union[str, bytes]]] = None, + strict: bool = False, +) -> 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. + """ + 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, Union[str, bytes]] = {} + 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) + ) + 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): + 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, 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)) + code = golden_author.main(["parse", *argv]) + captured = capsys.readouterr() + 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 +): + """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 + 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 — 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, + { + "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 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 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",)], 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): + """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_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, + {"Questions": {1: ["question"], 2: [QUERY]}, "questions ": {1: ["question"], 2: [QUERY]}}, + ) + + code, payload, err = _parse(tmp_path, monkeypatch, capsys, "--file", workbook, "--sheet", "QUESTIONS") + + 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 + + +# --- the header, and what is never silently dropped ----------------------------------------------- + + +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): + """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 + + +@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 — including in UTF-16, where no + byte sequence spells `' + ']>' + 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): + """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() 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