The import door reads an Excel workbook

[ashwin-agami]

Closes #261.

The import door refused an .xlsx and asked for a CSV export first — the one ingestion surface with a hard refusal, on the file format a question bank actually lives in. The earlier attempt (#267) tried to have the skill read the workbook, which can't work: the Read tool cannot open one, and nothing else on a clean machine can either. This is the route #261's correction proposed instead.

Change

plugins/agami/scripts/_xlsx.py reads one sheet of an .xlsx with zipfile and xml.etree.ElementTree — a workbook is a zip of XML, so no dependency is added to a plugin that installs with none. It yields the same rows a CSV does, and everything after that is the existing parse: still one parser, one place a column can be misread.

golden_author.py parse --file <path> [--sheet <name>]. --csv stays as a second spelling of --file, so every existing caller works unchanged.

What a real question-bank workbook needed

Checked against a real 53-sheet validation workbook, where "read the first sheet" would have been wrong four ways:

• The questions aren't on the first sheet. With several sheets and none named, the parse stops (exit 2) and lists them all; --sheet names one. Which tab holds the questions stays the user's call, and the skill is told never to pick one. A name is matched exactly, then once more ignoring case and surrounding space if that's unambiguous.
• A title block sits above the table. The header is now the first row within the top 20 that names a question column — the same exact alias match as before, never a guess. A CSV with no header still refuses exactly as it did.
• Row numbers must match Excel's. Rows keep Excel's own numbering, including rows the file omits entirely, so a skipped row is reported at the number the person sees.
• Excel pads the sheet to 1,000 rows. Trailing rows with no values are dropped instead of reported as hundreds of empty questions.

On that workbook: without a sheet name it stops and lists the sheets; with one, it finds the header on row 4 and reads all 50 questions with 0 skipped, in about 0.2s, writing nothing.

Also read as the sheet shows them: shared strings with rich-text runs (phonetic guides excluded), inline strings, booleans, numbers, sparse cells by column letter (past Z); an error value (#N/A) is blank rather than imported as a question. Refused with a sentence rather than a traceback: .xls (Excel's older binary format), a file that isn't a workbook, an XML part declaring a DTD (the entity-expansion vector), and an oversized part.

The payload gains header_row and, for a workbook, sheet, so the confirmation table can say what was read — a wrong tab parses cleanly and is only caught by someone seeing its name.

Not changed, deliberately

Column matching stays exact. A statement column under a header the contract doesn't know is still not picked up — a workbook makes that easier to hit, so the skill now says to tell the user which header held the statements and have them rename it, but widening the alias set is its own decision.

Docs

agami-save-golden/SKILL.md: Phase 2a hands a workbook to the parse instead of refusing it, and asks which sheet; 2b shows --file/--sheet and the new payload fields; the error table swaps the refusal row for the three workbook messages; the frontmatter mentions workbooks. shared/golden-dataset-shape.md and the CHANGELOG follow.

Tests

tests/test_golden_author_xlsx.py builds every workbook in the test from the format's own parts — synthetic, a few kilobytes: a workbook parses to the same rows as the CSV it would export; header below a title block with skips at Excel's row numbers across an omitted row; formatted-but-empty rows aren't skips; several sheets refuse and name them, and --sheet picks one; rich text, inline, boolean, number and error cells; sparse cells past Z; .xls and a non-workbook refused; a DTD-declaring part refused; parsing a workbook writes nothing; --sheet on a CSV is noted and the CSV still parses. Plus a skill-doc test that the refusal is gone and the sheet is left to the user. The existing parse, import and no-network tests pass unchanged.

🤖 Generated with Claude Code

[Copilot]

🟡 Changes recommended

Critical XLSX security and resource-exhaustion issues, plus compatibility and correctness gaps, remain unresolved.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Adds dependency-free .xlsx question-bank import while preserving the existing CSV parsing flow.

Changes:

• Adds workbook reading, sheet selection, header detection, and Excel row metadata.
• Updates CLI, skill documentation, dataset documentation, and changelog.
• Adds comprehensive workbook and documentation tests.

File summaries

File	Summary and review notes
tests/test_golden_author_xlsx.py	Adds workbook parsing, edge-case, and safety tests.
tests/test_ah109_save_golden_skill.py	Verifies updated skill guidance.
plugins/agami/skills/agami-save-golden/SKILL.md	Documents workbook import; needs payload-field and workbook-error guidance updates (nits).
plugins/agami/shared/golden-dataset-shape.md	Documents supported workbook input.
plugins/agami/scripts/golden_author.py	Integrates workbook parsing; header scanning can silently drop headerless CSV questions (moderate, 1 vote).
plugins/agami/scripts/_xlsx.py	Needs encoding-safe DTD protection (critical, 2 votes), bounded row validation (critical, 3 votes), Strict OOXML support or actionable rejection (moderate, 2 votes), and accurate ambiguous-sheet errors (moderate, 1 vote).
CHANGELOG.md	Qualify that only trailing formatted-empty rows are dropped (nit).

Review details

Suppressed comments (6)

CHANGELOG.md:24

• This changelog wording says every formatted-but-empty row is dropped, but the implementation intentionally keeps such rows between filled rows so they are reported as skips; only trailing padding is removed. Qualify this as trailing rows to keep the release note accurate.

  formats but never fills are dropped rather than reported as empty questions. An `.xls` file —

plugins/agami/scripts/_xlsx.py:179

• The column index is likewise an untrusted list size. A small cell with a very large column reference can make this loop allocate an enormous row before the workbook is parsed. Cap the index at Excel's 16,384-column limit (XFD) and refuse malformed coordinates before padding.

            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("")

plugins/agami/scripts/_xlsx.py:75

• When case/trim normalization matches more than one tab, len(matches) != 1 reports that there is “no sheet named” even though the requested name is ambiguous and the candidates exist. Distinguish zero matches from multiple matches and report the ambiguity so the user knows to choose an exact tab name.

            if len(matches) != 1:
                raise WorkbookError(
                    f"this workbook has no sheet named {sheet!r}. Sheets: {_listed(sheets)}"
                )

plugins/agami/scripts/golden_author.py:288

• Scanning arbitrary rows for an alias makes the promised headerless-CSV refusal unsafe: a one-column bank whose first question is literally prompt, question, or ask is accepted as the header and that question is silently dropped. Keep the existing first-row refusal for CSVs (or add an unambiguous header/data distinction) while allowing title blocks where intended.

    for index, row in enumerate(rows[:_HEADER_SCAN_ROWS]):
        if "query" in _columns(row):
            return index

plugins/agami/skills/agami-save-golden/SKILL.md:84

• The parser now adds header_row and, for workbooks, sheet, but the Phase 2b instructions still describe only columns, rows, skipped, and summary. That means the skill does not tell the confirmation step to show which tab and header were actually read, losing the context needed to catch a wrong-tab import. Update the payload instructions and confirmation table to include the new fields.

python3 "$AGAMI_PLUGIN_ROOT/scripts/golden_author.py" parse \
  --file <path-to-csv-or-xlsx> [--sheet "<sheet name>"] \
  > /tmp/agami-golden-parse-<ts>.json

plugins/agami/skills/agami-save-golden/SKILL.md:275

• The new reader has additional actionable refusal messages for DTD-bearing, oversized, missing, and malformed workbook parts, but this error table only documents the .xls and non-workbook cases. Those failures therefore have no skill-level recovery guidance. Add a generic workbook-read failure entry or document the added messages and their next steps.

| `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 "<name>"`. Never pick one yourself. |
| `agami-save-golden: this workbook has no sheet named '<name>'` | 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. |

• Files reviewed: 7/7 changed files
• Comments generated: 3
• Review effort level: Lite

---

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

[ashwin-agami]

Right — a UTF-16 part spells nothing an ASCII search can find. Fixed in ca351f0: the guard is now expat itself, with handlers on the doctype and entity declarations, run after expat has honoured the part's declared encoding. A parametrized test sends the same entity-expansion part in UTF-8 and UTF-16, and asserts the UTF-16 bytes don't contain <!DOCTYPE — the case the old check missed — before asserting both are refused.

[ashwin-agami]

Agreed, and the column had the same hole. Fixed in ca351f0: a row number is checked against Excel's 1,048,576 and a column against its 16,384 before either places a value, and a malformed row number or cell reference is refused. The sheet is held sparsely — filled cells only — and rows are built only up to the last filled one, capped at 100,000 rows and 2,000,000 built cells, so a styled cell on Excel's last row costs nothing and data that deep is refused. A parametrized test covers row 2,147,483,647, a non-numeric row, column XFE, a malformed reference and data on row 1,048,576, each asserting a refusal within a time bound.

[ashwin-agami]

Fixed in ca351f0: Strict is read rather than rejected. The spreadsheet namespace is taken from each part's own root element, the relationship-id attribute is matched by local name, and the workbook part and shared strings are found through relationships (the package's root .rels, then the workbook's) rather than fixed paths. A test builds the same workbook in both conformance classes and asserts they read identically.

[ashwin-agami]

Copilot's six suppressed comments, all addressed in ca351f0:

• Column index unbounded. Checked against Excel's 16,384 columns before it places a value; with the row fix, a crafted cell costs a refusal rather than memory (tested).
• A name matching two tabs loosely reported as missing. Now reported as ambiguous, naming the matching tabs (tested).
• Header scanning could drop a headerless CSV's question. Scanning is for workbooks only. A CSV's header is still its first non-empty row, so a question that reads "question" can't make the rows above it vanish (tested). In a workbook, every non-empty row above the header is reported in a new above_header field, with a warning — nothing is dropped unsaid.
• Skill didn't describe the new payload fields. 2b documents header_row, sheet and above_header, and 2c now says which sheet and header row were read, and quotes the rows above the header, before showing the rows.
• Skill had no recovery for the other workbook refusals. The error table covers the ambiguous name and every read failure (damaged, missing or oversized part, DTD, out-of-range coordinates, too large).
• CHANGELOG said every formatted-but-empty row is dropped. It says trailing rows now, which is what the reader does.

Re-checked against the real 53-sheet workbook: header on row 4, all 50 questions, 0 skipped, the two title rows above the header reported, nothing written.

[ashwin-agami]

Pushed 767b592: a column the import doesn't read, but whose header looks like a field the sheet hasn't supplied (e.g. Warehouse SQL when there is no sql column), is no longer dropped silently.

• parse lists it under unrecognized with the field it could be, and warns on stderr.
• The skill asks the person about each one before importing; on a yes it re-runs with --column sql="Warehouse SQL". It never maps a column on its own.
• A mapping that names an unknown field, maps a field twice, or names a header that isn't in the sheet is refused.
• Matching stays exact — the look-alike check only decides what to ask about, never what to read.