diff --git a/.tbd/workspaces/outbox/issues/is-01khx9xwpv03xfg2cc5aqtwn8g.md b/.tbd/workspaces/outbox/issues/is-01khx9xwpv03xfg2cc5aqtwn8g.md new file mode 100644 index 00000000..fba8237b --- /dev/null +++ b/.tbd/workspaces/outbox/issues/is-01khx9xwpv03xfg2cc5aqtwn8g.md @@ -0,0 +1,22 @@ +--- +type: is +id: is-01khx9xwpv03xfg2cc5aqtwn8g +title: "Spec: Table row validation — empty row dropping, cell-level warnings, minRows/maxRows enforcement" +kind: epic +status: closed +priority: 2 +version: 7 +spec_path: docs/project/specs/active/plan-2026-02-20-validate-form-rows.md +labels: [] +dependencies: [] +child_order_hints: + - is-01khx9y5kgsweghjsdnt8wze22 + - is-01khx9yd02anf0w14rkt37mcej + - is-01khx9ym9e1d2dyb3nh3dqs7qe + - is-01khx9yvwgs8af6m7q1yp8jb6s + - is-01khx9z3qgwx2s3gfnwbsnv45j +created_at: 2026-02-20T10:36:03.162Z +updated_at: 2026-02-20T17:28:14.954Z +closed_at: 2026-02-20T17:28:14.954Z +close_reason: "All phases complete: doc updates, empty row dropping in parse+apply, mostly-empty warnings, minRows enforcement, full test coverage" +--- diff --git a/.tbd/workspaces/outbox/issues/is-01khx9y5kgsweghjsdnt8wze22.md b/.tbd/workspaces/outbox/issues/is-01khx9y5kgsweghjsdnt8wze22.md new file mode 100644 index 00000000..ddfc19e9 --- /dev/null +++ b/.tbd/workspaces/outbox/issues/is-01khx9y5kgsweghjsdnt8wze22.md @@ -0,0 +1,21 @@ +--- +type: is +id: is-01khx9y5kgsweghjsdnt8wze22 +title: "Phase 1: Update markform-spec.md and markform-reference.md with empty row handling and sparseness warning docs" +kind: task +status: closed +priority: 1 +version: 5 +spec_path: docs/project/specs/active/plan-2026-02-20-validate-form-rows.md +labels: [] +dependencies: + - type: blocks + target: is-01khx9yd02anf0w14rkt37mcej + - type: blocks + target: is-01khx9ym9e1d2dyb3nh3dqs7qe +parent_id: is-01khx9xwpv03xfg2cc5aqtwn8g +created_at: 2026-02-20T10:36:12.271Z +updated_at: 2026-02-20T17:17:04.687Z +closed_at: 2026-02-20T17:17:04.686Z +close_reason: Updated markform-spec.md and markform-reference.md with empty row handling, minRows/maxRows semantics, and sparseness warning docs +--- diff --git a/.tbd/workspaces/outbox/issues/is-01khx9yd02anf0w14rkt37mcej.md b/.tbd/workspaces/outbox/issues/is-01khx9yd02anf0w14rkt37mcej.md new file mode 100644 index 00000000..a0b13e86 --- /dev/null +++ b/.tbd/workspaces/outbox/issues/is-01khx9yd02anf0w14rkt37mcej.md @@ -0,0 +1,19 @@ +--- +type: is +id: is-01khx9yd02anf0w14rkt37mcej +title: "Phase 2a: Add isRowFullyEmpty() helper and empty row filtering in parseMarkdownTable()" +kind: task +status: closed +priority: 1 +version: 4 +spec_path: docs/project/specs/active/plan-2026-02-20-validate-form-rows.md +labels: [] +dependencies: + - type: blocks + target: is-01khx9yvwgs8af6m7q1yp8jb6s +parent_id: is-01khx9xwpv03xfg2cc5aqtwn8g +created_at: 2026-02-20T10:36:19.841Z +updated_at: 2026-02-20T17:20:25.083Z +closed_at: 2026-02-20T17:20:25.081Z +close_reason: Added isRowFullyEmpty() helper and empty row filtering in parseMarkdownTable() and parseInlineTable() with full TDD coverage +--- diff --git a/.tbd/workspaces/outbox/issues/is-01khx9ym9e1d2dyb3nh3dqs7qe.md b/.tbd/workspaces/outbox/issues/is-01khx9ym9e1d2dyb3nh3dqs7qe.md new file mode 100644 index 00000000..09314fdb --- /dev/null +++ b/.tbd/workspaces/outbox/issues/is-01khx9ym9e1d2dyb3nh3dqs7qe.md @@ -0,0 +1,19 @@ +--- +type: is +id: is-01khx9ym9e1d2dyb3nh3dqs7qe +title: "Phase 2b: Filter empty rows in set_table and append_table patch handlers in apply.ts" +kind: task +status: closed +priority: 1 +version: 4 +spec_path: docs/project/specs/active/plan-2026-02-20-validate-form-rows.md +labels: [] +dependencies: + - type: blocks + target: is-01khx9yvwgs8af6m7q1yp8jb6s +parent_id: is-01khx9xwpv03xfg2cc5aqtwn8g +created_at: 2026-02-20T10:36:27.310Z +updated_at: 2026-02-20T17:22:27.541Z +closed_at: 2026-02-20T17:22:27.540Z +close_reason: Filtered empty rows in set_table and append_table patch handlers with unanswered edge case, full TDD coverage +--- diff --git a/.tbd/workspaces/outbox/issues/is-01khx9yvwgs8af6m7q1yp8jb6s.md b/.tbd/workspaces/outbox/issues/is-01khx9yvwgs8af6m7q1yp8jb6s.md new file mode 100644 index 00000000..18a7d9ad --- /dev/null +++ b/.tbd/workspaces/outbox/issues/is-01khx9yvwgs8af6m7q1yp8jb6s.md @@ -0,0 +1,19 @@ +--- +type: is +id: is-01khx9yvwgs8af6m7q1yp8jb6s +title: "Phase 3: Add mostly-empty row warning in validateTableRow() with strict-majority threshold" +kind: task +status: closed +priority: 1 +version: 4 +spec_path: docs/project/specs/active/plan-2026-02-20-validate-form-rows.md +labels: [] +dependencies: + - type: blocks + target: is-01khx9z3qgwx2s3gfnwbsnv45j +parent_id: is-01khx9xwpv03xfg2cc5aqtwn8g +created_at: 2026-02-20T10:36:35.087Z +updated_at: 2026-02-20T17:25:14.221Z +closed_at: 2026-02-20T17:25:14.221Z +close_reason: Added mostly-empty row warning in validateTableRow() with strict-majority threshold, moved minRows check before isEmpty early return, full TDD coverage +--- diff --git a/.tbd/workspaces/outbox/issues/is-01khx9z3qgwx2s3gfnwbsnv45j.md b/.tbd/workspaces/outbox/issues/is-01khx9z3qgwx2s3gfnwbsnv45j.md new file mode 100644 index 00000000..e7596b4d --- /dev/null +++ b/.tbd/workspaces/outbox/issues/is-01khx9z3qgwx2s3gfnwbsnv45j.md @@ -0,0 +1,17 @@ +--- +type: is +id: is-01khx9z3qgwx2s3gfnwbsnv45j +title: "Phase 4: Verify example forms and run full test suite for regressions" +kind: task +status: closed +priority: 2 +version: 3 +spec_path: docs/project/specs/active/plan-2026-02-20-validate-form-rows.md +labels: [] +dependencies: [] +parent_id: is-01khx9xwpv03xfg2cc5aqtwn8g +created_at: 2026-02-20T10:36:43.119Z +updated_at: 2026-02-20T17:28:07.041Z +closed_at: 2026-02-20T17:28:07.040Z +close_reason: "All quality gates pass: typecheck, lint, build, publint, 2256 unit tests, 44 golden tests" +--- diff --git a/.tbd/workspaces/outbox/issues/is-01khy5qjtzdbp2189eh4x1xkea.md b/.tbd/workspaces/outbox/issues/is-01khy5qjtzdbp2189eh4x1xkea.md new file mode 100644 index 00000000..cd18a794 --- /dev/null +++ b/.tbd/workspaces/outbox/issues/is-01khy5qjtzdbp2189eh4x1xkea.md @@ -0,0 +1,15 @@ +--- +type: is +id: is-01khy5qjtzdbp2189eh4x1xkea +title: Code review fixes for table row validation feature +kind: task +status: closed +priority: 1 +version: 3 +labels: [] +dependencies: [] +created_at: 2026-02-20T18:41:56.574Z +updated_at: 2026-02-20T18:48:08.151Z +closed_at: 2026-02-20T18:48:08.150Z +close_reason: Simplified isRowFullyEmpty logic, consolidated redundant tests, removed obvious comments +--- diff --git a/.tbd/workspaces/outbox/mappings/ids.yml b/.tbd/workspaces/outbox/mappings/ids.yml new file mode 100644 index 00000000..7e1916dc --- /dev/null +++ b/.tbd/workspaces/outbox/mappings/ids.yml @@ -0,0 +1,7 @@ +2h7f: 01khx9yd02anf0w14rkt37mcej +ds98: 01khx9xwpv03xfg2cc5aqtwn8g +iyg4: 01khx9yvwgs8af6m7q1yp8jb6s +osok: 01khx9y5kgsweghjsdnt8wze22 +s6wk: 01khx9ym9e1d2dyb3nh3dqs7qe +tq4r: 01khy5qjtzdbp2189eh4x1xkea +xg2p: 01khx9z3qgwx2s3gfnwbsnv45j diff --git a/docs/markform-reference.md b/docs/markform-reference.md index 5434f3ec..ec30f421 100644 --- a/docs/markform-reference.md +++ b/docs/markform-reference.md @@ -415,8 +415,13 @@ Structured tabular data with typed columns. Uses standard markdown table syntax. | `columnIds` | string[] | Yes | Array of snake_case column identifiers | | `columnLabels` | string[] | No | Display labels (defaults to header row) | | `columnTypes` | (string \| object)[] | No | Column types with optional constraints (defaults to all `string`) | -| `minRows` | number | No | Minimum row count (default: 0) | -| `maxRows` | number | No | Maximum row count (default: unlimited) | +| `minRows` | number | No | Minimum number of non-empty rows (default: 0) | +| `maxRows` | number | No | Maximum number of non-empty rows (default: unlimited) | + +**Empty row handling:** Fully-empty rows (where every cell is empty or skipped) are +silently dropped during normalization. +They are never counted toward `minRows`/`maxRows` and do not appear in parsed output. +A row with at least one non-empty cell is retained. **Column types:** @@ -445,6 +450,10 @@ Structured tabular data with typed columns. Uses standard markdown table syntax. | `min` / `max` | `number`, `year`, `date` | Value bounds | | `integer` | `number` | Must be integer | +**Row sparseness warning:** When a non-empty row has the majority of its cells empty +(strictly more than half), a validation warning is emitted. +This helps catch cases where an agent produced sparse or incomplete data. + **Sentinel values in cells:** Use `%SKIP%` or `%ABORT%` with optional reasons: ```markdown diff --git a/docs/markform-spec.md b/docs/markform-spec.md index 4ba0cb9a..06dff8d3 100644 --- a/docs/markform-spec.md +++ b/docs/markform-spec.md @@ -456,8 +456,13 @@ type. | `columnIds` | string[] | Yes | Array of snake_case column identifiers | | `columnLabels` | string[] | No | Array of display labels (backfilled from table header row if omitted) | | `columnTypes` | (string \| object)[] | No | Array of column types with optional constraints (defaults to all `'string'`) | -| `minRows` | number | No | Minimum row count (default: 0) | -| `maxRows` | number | No | Maximum row count (default: unlimited) | +| `minRows` | number | No | Minimum number of non-empty rows (default: 0) | +| `maxRows` | number | No | Maximum number of non-empty rows (default: unlimited) | + +**Empty row handling:** Fully-empty rows (where every cell is empty or skipped) are +silently dropped during normalization. +They are never counted toward `minRows`/`maxRows` and do not appear in parsed output. +A row with at least one non-empty cell is retained. **Column types and validation:** @@ -488,6 +493,10 @@ columnTypes=[{type: "string", required: true}, "number", "url"] | `max` | `number`, `year`, `date` | Maximum value | | `integer` | `number` | Value must be an integer | +**Row sparseness warning:** When a non-empty row has the majority of its cells empty +(strictly more than half), a validation warning is emitted. +This helps catch cases where an agent produced sparse or incomplete data. + **Example with per-column constraints:** ```md {% field kind="table" id="priority_table" label="Ranked Insights" diff --git a/docs/project/specs/active/plan-2026-02-20-validate-form-rows.md b/docs/project/specs/active/plan-2026-02-20-validate-form-rows.md new file mode 100644 index 00000000..c1bbe741 --- /dev/null +++ b/docs/project/specs/active/plan-2026-02-20-validate-form-rows.md @@ -0,0 +1,554 @@ +--- +title: Table Row Validation — Empty Row Dropping, Cell-Level Warnings, and minRows/maxRows Enforcement +description: > + Strengthen table field validation by dropping fully-empty rows on normalization, + adding warnings for mostly-empty rows, and ensuring minRows/maxRows count only + substantive rows. +author: Joshua Levy (github.com/jlevy) with LLM assistance +--- +# Feature: Table Row Validation — Empty Row Dropping, Cell-Level Warnings, and minRows/maxRows Enforcement + +**Date:** 2026-02-20 (last updated 2026-02-21) + +**Author:** Claude (with Joshua Levy) + +**Status:** In Progress + +## Overview + +Table fields currently have no distinction between substantive rows and fully-empty +rows. A row where every cell is `skipped` (empty) counts toward `minRows`, appears in +progress summaries as “submitted” data, and persists through serialization. +This creates several problems: + +1. **Empty rows satisfy `minRows`:** A table with `minRows=5` that contains 5 all-empty + rows passes validation, even though no data was actually provided. + +2. **No warning for mostly-empty rows:** A row with one cell filled and four cells empty + silently passes, even when this likely indicates the agent skipped required work. + +3. **Template/placeholder rows are indistinguishable from real data:** Forms that + include sample rows have no way to signal that those rows should be replaced. + +This spec addresses all three problems through three complementary changes: + +- **A: Drop fully-empty rows on normalization** — rows where every cell is + `skipped`/empty are silently dropped, as if they were never there. + +- **B: Warn on mostly-empty rows** — rows with data in some cells but many empty cells + produce a validation warning. + +- **C: Strengthen minRows/maxRows validation** — ensure these constraints count only + substantive (non-empty) rows, and that all required validation paths exist. + +## Goals + +- Fully-empty table rows are dropped during normalization (parse and patch apply), + making them invisible to validation, progress, and serialization +- Validation produces a warning when a non-empty row has the majority of its cells empty + (i.e., strictly more than half of cells are empty in a row that has some data) +- `minRows` and `maxRows` constraints count only substantive rows (rows with at least + one answered cell) +- `isFieldSubmitted()` in progress computation correctly treats tables with only empty + rows as not submitted +- All changes are backward compatible — existing forms continue to work; empty rows that + were previously serialized as `%SKIP%` rows are simply dropped on re-parse +- Representative example forms exercise all validation paths +- Full TDD coverage for all behavior changes + +## Non-Goals + +- Adding a `placeholderRows` attribute to the spec (deferred; the empty-row dropping + plus `minRows` covers the common case without new syntax) +- Changing the `placeholder` attribute to work on table fields (placeholder is a UI hint + for text-entry fields; tables are structurally different) +- Per-row metadata or annotations (e.g., “this row is an example”) +- Changes to the table editing UI or patch API +- Changing how `%SKIP%` sentinels work at the cell level + +## Background + +### Current Behavior + +**Empty cell parsing** (`src/engine/table/parseTable.ts:82-87`): An empty/whitespace +cell returns `{ state: 'skipped' }`. + +**Row parsing** (`src/engine/table/parseTable.ts:299-314`): All rows are parsed, +including rows where every cell is empty. +These produce rows where every cell has `state: 'skipped'`. + +**Table validation** (`src/engine/validate.ts:981-1028`): +- `isEmpty` check (line 986): `!value || rows.length === 0` — counts all rows, including + all-empty ones. +- `minRows` (line 1003): `rows.length < field.minRows` — counts all rows. +- `maxRows` (line 1013): `rows.length > field.maxRows` — counts all rows. +- Per-row validation (line 1023): validates each row including all-empty ones. + +**Progress computation** (`src/engine/summaries.ts:196-199`): +```typescript +case 'table': { + const v = value as TableValue; + return (v.rows?.length ?? 0) > 0; +} +``` +A table with any rows (including all-empty ones) is considered “submitted.” + +**Patch application** (`src/engine/apply.ts:635-682`): +- `set_table` (lines 635-650) and `append_table` (lines 653-670) create rows from patch + data without filtering empties. +- `delete_table` (lines 673-682) correctly handles the last-row case (sets state to + `unanswered` at line 678). + +### The Problem in Practice + +Consider a form with `minRows=5`: + +```md +| Name | Score | Notes | +|------|-------|-------| +| | | | +| | | | +| | | | +| | | | +| | | | +``` + +This passes `minRows=5` validation today, but contains zero useful data. +An agent that outputs this has satisfied the letter of the constraint but not the +intent. + +Similarly, a partially-filled table: + +```md +| Name | Score | Notes | +|------|-------|-------| +| Alice | 95 | | +| | | | +| | | | +``` + +Has 3 rows by count, but only 1 row with any data. +The 2 empty rows inflate the count and mask the fact that the agent stopped filling +after the first row. + +## Design + +### A: Drop Fully-Empty Rows on Normalization + +**Definition:** A “fully-empty row” is one where every cell has `state: 'skipped'` (or +equivalently, every cell value is empty/undefined after parsing). + +**Where to drop:** + +1. **At parse time** — in `parseMarkdownTable()` (`src/engine/table/parseTable.ts`), + after parsing all rows, filter out any row where every cell is `skipped`. This means + the parsed `TableValue` never contains fully-empty rows. + +2. **At patch apply time** — in the `set_table` and `append_table` handlers in + `apply.ts`, filter out fully-empty rows from the resulting row array. + This handles the case where an agent sends a patch with empty rows. + +**Helper function:** + +```typescript +/** + * Check if a table row is fully empty (all cells skipped/empty). + * Used during normalization to drop rows that carry no data. + * + * Aborted cells are NOT considered empty — they carry intentional signal + * (the agent explicitly declined to fill them, possibly with a reason). + */ +export function isRowFullyEmpty(row: TableRowResponse): boolean { + return Object.values(row).every((cell) => { + if (!cell || cell.state === 'skipped') return true; + if (cell.state === 'aborted') return false; + return cell.value === undefined || cell.value === null || cell.value === ''; + }); +} +``` + +This checks all cell entries in the row object rather than requiring a `columns` +parameter. This is simpler and sufficient because the row object only contains entries +for known column IDs (set during parsing or patch application). + +**Important:** Aborted cells (`%ABORT%`) are preserved — they represent an explicit +agent decision, not absence of data. + +**Behavioral impact:** +- A template form with header + separator + 3 empty rows parses as a table with 0 rows +- Serialization of that table produces header + separator only (no empty data rows) +- `minRows` checks see the true count of substantive rows +- Progress computation sees the table as empty (not submitted) + +### B: Warn on Mostly-Empty Rows + +**Definition:** A “mostly-empty row” is one that has at least one answered cell, but +strictly more than half of its cells are empty/skipped. + +**Where to warn:** In `validateTableRow()` (`src/engine/validate.ts`), after validating +individual cells, check whether the row is mostly empty. +If so, emit a `warning`-severity issue. + +**Warning message:** + +``` +Row N of "TABLE_LABEL" has most cells empty (M of K filled). +``` + +**Severity:** `warning` (not `error`). This is advisory — a form author may +intentionally have sparse rows (e.g., an “optional notes” column). +The warning helps catch cases where an agent is producing low-quality output. + +**Threshold:** Strictly more than half of cells are empty in a row that has at least one +filled cell. Formally: `emptyCells > totalCells / 2`. This means an even split (e.g., 2 +of 4 filled) does NOT warn. +This avoids warning on fully-empty rows (those are dropped by normalization) and on rows +that are half-filled or more. + +### C: Strengthen minRows/maxRows Validation + +Since fully-empty rows are dropped during normalization (Part A), the `minRows` and +`maxRows` checks in `validateTableField()` will automatically count only substantive +rows. +No additional filtering is needed in the validator itself — the normalization layer +guarantees that `rows.length` reflects substantive rows. + +**Changes needed in `validateTableField()`:** +- None for the `minRows`/`maxRows` logic itself (it already uses `rows.length`). +- The `isEmpty` check (`!value || rows.length === 0`) continues to work correctly + because normalization ensures that a table with only empty rows has `rows: []`. + +**Changes needed in `isFieldSubmitted()`** (`src/engine/summaries.ts:196-199`): +- No change needed — after normalization, `rows.length > 0` correctly means the table + has substantive data. + +**Documentation:** The spec and reference docs should clarify that `minRows` and +`maxRows` count only rows with at least one non-empty cell. + +## Components + +**Files to modify:** + +| File | Change | +| --- | --- | +| `src/engine/table/parseTable.ts` | Add `isRowFullyEmpty()` helper; filter empty rows in `parseMarkdownTable()` | +| `src/engine/apply.ts` | Filter empty rows in `set_table` and `append_table` patch handlers | +| `src/engine/validate.ts` | Add mostly-empty row warning in `validateTableRow()` | +| `docs/markform-spec.md` | Clarify empty row dropping, minRows/maxRows semantics | +| `docs/markform-reference.md` | Update table field docs | +| `tests/unit/engine/parseTable.test.ts` | Tests for empty row dropping at parse time | +| `tests/unit/engine/apply.test.ts` | Tests for empty row dropping at patch apply time | +| `tests/unit/engine/validate.test.ts` | Tests for mostly-empty row warnings, minRows with empty rows | +| `tests/unit/engine/summaries.test.ts` | Tests for progress computation with empty rows | + +## Implementation Plan + +### Phase 1: Documentation Updates + +Update the Markform spec and reference docs to define the new behavior before +implementing it. This establishes the contract that code and tests will be written +against. + +- [ ] **`docs/markform-spec.md`** — Table Fields section (lines 446-501): + - After the `minRows`/`maxRows` attribute rows (line 460), add a new paragraph: + > **Empty row handling:** Fully-empty rows (where every cell is empty or skipped) + > are silently dropped during normalization. + > They are never counted toward `minRows`/`maxRows` and do not appear in parsed + > output. A row with at least one non-empty cell is retained. + - Update the `minRows` description (line 459) from “Minimum row count” to “Minimum + number of non-empty rows” + - Update the `maxRows` description (line 460) from “Maximum row count” to “Maximum + number of non-empty rows” + - Add a new “Validation warnings” note after the per-column constraints table (after + line 489): + > **Row sparseness warning:** When a non-empty row has the majority of its cells + > empty (strictly more than half), a validation warning is emitted. + > This helps catch cases where an agent produced sparse or incomplete data. + +- [ ] **`docs/markform-reference.md`** — Table field section (lines 413-456): + - Same updates as above: + - Update `minRows` description (line 418) to “Minimum number of non-empty rows” + - Update `maxRows` description (line 419) to “Maximum number of non-empty rows” + - Add empty row handling note after the attributes table (after line 419) + - Add row sparseness warning note after the per-column constraints table (after line + 446\) + +### Phase 2: Empty Row Dropping (TDD) + +Tests first, then implementation. + +**Tests to write:** +- [ ] `parseTable.test.ts`: table with all-empty rows → parsed as 0 rows +- [ ] `parseTable.test.ts`: table with mix of empty and filled rows → only filled rows + retained +- [ ] `parseTable.test.ts`: table with rows where all cells are `%SKIP%` → dropped +- [ ] `parseTable.test.ts`: table with one cell filled in a row → row retained +- [ ] `apply.test.ts`: `set_table` patch with empty rows → empty rows filtered out +- [ ] `apply.test.ts`: `append_table` patch with empty rows → empty rows filtered out +- [ ] `apply.test.ts`: `set_table` with all-empty rows → table state becomes + `unanswered` +- [ ] Round-trip: parse form with empty rows → serialize → re-parse → same result (no + empty rows) + +**Implementation — `src/engine/table/parseTable.ts`:** + +- [ ] Add exported `isRowFullyEmpty()` helper (after line 137, before + `parseMarkdownTable`): + ```typescript + /** + * Check if a table row is fully empty (all cells skipped/empty). + * Aborted cells are NOT considered empty. + */ + export function isRowFullyEmpty(row: TableRowResponse): boolean { + return Object.values(row).every((cell) => { + if (!cell || cell.state === 'skipped') return true; + if (cell.state === 'aborted') return false; + return cell.value === undefined || cell.value === null || cell.value === ''; + }); + } + ``` + Note: This checks all cell entries in the row object rather than requiring a `columns` + parameter. Aborted cells are preserved since they carry intentional signal. + +- [ ] In `parseMarkdownTable()`, **positional path** (lines 298-314): after the for-loop + builds `rows` (line 312), filter before returning: + ```typescript + // Current (line 311-314): + rows.push(row); + } + return { ok: true, value: { kind: 'table', rows } }; + + // New: + rows.push(row); + } + const substantiveRows = rows.filter((r) => !isRowFullyEmpty(r)); + return { ok: true, value: { kind: 'table', rows: substantiveRows } }; + ``` + +- [ ] In `parseMarkdownTable()`, **header-based path** (lines 330-347): same filter + before returning: + ```typescript + // Current (line 344-347): + rows.push(row); + } + return { ok: true, value: { kind: 'table', rows } }; + + // New: + rows.push(row); + } + const substantiveRows = rows.filter((r) => !isRowFullyEmpty(r)); + return { ok: true, value: { kind: 'table', rows: substantiveRows } }; + ``` + +**Implementation — `src/engine/apply.ts`:** + +- [ ] Add import of `isRowFullyEmpty` from `./table/parseTable.js` (line 29, after + existing imports): + ```typescript + import { isRowFullyEmpty } from './table/parseTable.js'; + ``` + +- [ ] In `set_table` handler (lines 635-650): after building `rows` (line 644), filter + empty rows and handle the all-empty edge case: + ```typescript + // Current (lines 644-649): + return row; + }); + responses[patch.fieldId] = { + state: 'answered', + value: { kind: 'table', rows } as TableValue, + }; + + // New: + return row; + }); + const substantiveRows = rows.filter((r) => !isRowFullyEmpty(r)); + responses[patch.fieldId] = { + state: substantiveRows.length > 0 ? 'answered' : 'unanswered', + ...(substantiveRows.length > 0 && { + value: { kind: 'table', rows: substantiveRows } as TableValue, + }), + }; + ``` + Note: the `unanswered` edge case mirrors the existing pattern in `delete_table` (line + 678). + +- [ ] In `append_table` handler (lines 653-670): after building combined rows (line + 667), filter empty rows: + ```typescript + // Current (lines 664-669): + return row; + }); + responses[patch.fieldId] = { + state: 'answered', + value: { kind: 'table', rows: [...currentRows, ...newRows] } as TableValue, + }; + + // New: + return row; + }); + const allRows = [...currentRows, ...newRows].filter((r) => !isRowFullyEmpty(r)); + responses[patch.fieldId] = { + state: allRows.length > 0 ? 'answered' : 'unanswered', + ...(allRows.length > 0 && { + value: { kind: 'table', rows: allRows } as TableValue, + }), + }; + ``` + +### Phase 3: Mostly-Empty Row Warnings (TDD) + +Tests first, then implementation. + +**Tests to write:** +- [ ] `validate.test.ts`: row with 1 of 4 cells filled → warning (3/4 empty > half) +- [ ] `validate.test.ts`: row with 2 of 4 cells filled → no warning (even split, not + strict majority) +- [ ] `validate.test.ts`: row with 3 of 4 cells filled → no warning +- [ ] `validate.test.ts`: row with all cells filled → no warning +- [ ] `validate.test.ts`: row with 1 of 3 cells filled → warning (2/3 empty > half) +- [ ] `validate.test.ts`: row with 1 of 2 cells filled → no warning (1/2 is not strictly + more than half) +- [ ] `validate.test.ts`: warning message includes row number and field label +- [ ] `validate.test.ts`: warning severity is `warning`, not `error` + +**Implementation — `src/engine/validate.ts`:** + +- [ ] In `validateTableRow()` (lines 962-976), after the per-cell validation loop (line + 973), add the sparseness check: + ```typescript + function validateTableRow( + row: TableRowResponse, + columns: TableColumn[], + fieldId: string, + rowIndex: number, + fieldLabel: string, // NEW parameter + ): ValidationIssue[] { + const issues: ValidationIssue[] = []; + + for (const column of columns) { + const cell = row[column.id] ?? { state: 'skipped' }; + issues.push(...validateCellValue(cell, column, fieldId, rowIndex)); + } + + // NEW: Check for mostly-empty rows + const totalCells = columns.length; + const filledCells = columns.filter((col) => { + const cell = row[col.id]; + return cell && cell.state === 'answered' + && cell.value !== undefined && cell.value !== null && cell.value !== ''; + }).length; + + if (filledCells > 0 && filledCells < totalCells && (totalCells - filledCells) > totalCells / 2) { + issues.push({ + severity: 'warning', + message: `Row ${rowIndex + 1} of "${fieldLabel}" has most cells empty ` + + `(${filledCells} of ${totalCells} filled).`, + ref: `${fieldId}[${rowIndex}]`, + source: 'builtin', + }); + } + + return issues; + } + ``` + +- [ ] Update the call site in `validateTableField()` (line 1024) to pass `field.label`: + ```typescript + // Current: + issues.push(...validateTableRow(rows[i]!, field.columns, field.id, i)); + // New: + issues.push(...validateTableRow(rows[i]!, field.columns, field.id, i, field.label)); + ``` + +### Phase 4: Verify Example Forms and Run Full Suite + +- [ ] Run full test suite to confirm no regressions +- [ ] Run `markform inspect` on twitter-thread and rejection-test example forms +- [ ] Verify that a form with `minRows=3` and only empty rows fails validation +- [ ] Verify that a form with `minRows=3` and 3 filled rows + 2 empty rows passes (empty + rows dropped, 3 substantive rows remain) + +## Testing Strategy + +**Unit tests** (TDD — write tests before implementation): + +- **Parse tests** (`tests/unit/engine/parseTable.test.ts`): verify empty row dropping at + parse time across various combinations (all empty, mixed, single-cell filled, `%SKIP%` + rows) +- **Apply tests** (`tests/unit/engine/apply.test.ts`): verify empty row filtering on + `set_table` and `append_table` patches, including the all-empty → `unanswered` edge + case +- **Validation tests** (`tests/unit/engine/validate.test.ts`): verify mostly-empty row + warnings with various fill ratios, minRows enforcement with empty rows dropped, + maxRows with empty rows dropped +- **Progress tests** (`tests/unit/engine/summaries.test.ts`): verify + `isFieldSubmitted()` returns false for tables with only empty rows after normalization + (this should already pass once parse-time filtering is implemented, since the + `TableValue` will have `rows: []`) + +**Integration tests:** + +- Parse a form with a table containing mixed empty/filled rows → inspect → verify row + count and warnings +- Apply patches that include empty rows → verify they are dropped +- Serialize → re-parse round trip preserves only substantive rows + +**Example form testing:** + +- Run `markform inspect` on the twitter-thread form and rejection-test form to verify no + regressions +- Test with a form that has `minRows=3` and only empty rows → should fail validation + +## Rollout Plan + +Single PR. All changes are backward compatible: +- Forms with no empty rows: zero behavior change. +- Forms with empty rows: rows are silently dropped. + This is a behavioral improvement, not a breaking change, since empty rows carried no + useful information. +- The `warning` for mostly-empty rows is advisory and does not affect form completion + status. + +## Open Questions + +None — all design decisions are resolved in this spec. + +## References + +**Source code (all paths relative to `packages/markform/`):** + +- `src/engine/table/parseTable.ts:82-137` — `parseCellValue()`: empty cells → + `{ state: 'skipped' }` +- `src/engine/table/parseTable.ts:266-348` — `parseMarkdownTable()`: two parsing paths + (positional line 298, header-based line 330) +- `src/engine/validate.ts:745-957` — `validateCellValue()`: per-cell type+constraint + checks +- `src/engine/validate.ts:962-976` — `validateTableRow()`: iterates columns, delegates + to `validateCellValue()` +- `src/engine/validate.ts:981-1028` — `validateTableField()`: isEmpty (line 986), + minRows (line 1003), maxRows (line 1013), row loop (line 1023) +- `src/engine/apply.ts:635-650` — `set_table` patch handler +- `src/engine/apply.ts:653-670` — `append_table` patch handler +- `src/engine/apply.ts:673-682` — `delete_table` patch handler (reference for + `unanswered` pattern) +- `src/engine/summaries.ts:196-199` — `isFieldSubmitted()` table case: + `(v.rows?.length ?? 0) > 0` +- `src/engine/coreTypes.ts:324-329` — `TableField` interface (has `columns`, `minRows`, + `maxRows`) +- `src/engine/coreTypes.ts:478-491` — `CellResponse`, `TableRowResponse`, `TableValue` + types + +**Documentation:** + +- `docs/markform-spec.md:446-501` — Table Fields section (attributes table lines + 454-460, column constraints lines 478-489) +- `docs/markform-reference.md:413-456` — Table field reference (attributes table lines + 413-419, constraints lines 439-446) + +**Related specs and examples:** + +- `docs/project/specs/active/plan-2026-02-14-table-column-constraints.md` — Prior spec + for per-column constraints +- `packages/markform/examples/twitter-thread/twitter-thread.form.md` +- `packages/markform/examples/rejection-test/rejection-test.form.md` diff --git a/packages/markform/src/engine/apply.ts b/packages/markform/src/engine/apply.ts index 333e7bd5..02f44884 100644 --- a/packages/markform/src/engine/apply.ts +++ b/packages/markform/src/engine/apply.ts @@ -28,6 +28,7 @@ import type { } from './coreTypes.js'; import { detectSentinel } from './parseSentinels.js'; import { computeAllSummaries, computeFormState, isFormComplete } from './summaries.js'; +import { isRowFullyEmpty } from './table/parseTable.js'; import { validate } from './validate.js'; // ============================================================================= @@ -642,9 +643,12 @@ function applyPatch(form: ParsedForm, responses: Record, patc } return row; }); + const substantiveRows = rows.filter((r) => !isRowFullyEmpty(r)); responses[patch.fieldId] = { - state: 'answered', - value: { kind: 'table', rows } as TableValue, + state: substantiveRows.length > 0 ? 'answered' : 'unanswered', + ...(substantiveRows.length > 0 && { + value: { kind: 'table', rows: substantiveRows } as TableValue, + }), }; break; } @@ -662,9 +666,12 @@ function applyPatch(form: ParsedForm, responses: Record, patc } return row; }); + const allRows = [...currentRows, ...newRows].filter((r) => !isRowFullyEmpty(r)); responses[patch.fieldId] = { - state: 'answered', - value: { kind: 'table', rows: [...currentRows, ...newRows] } as TableValue, + state: allRows.length > 0 ? 'answered' : 'unanswered', + ...(allRows.length > 0 && { + value: { kind: 'table', rows: allRows } as TableValue, + }), }; break; } diff --git a/packages/markform/src/engine/table/parseTable.ts b/packages/markform/src/engine/table/parseTable.ts index a38cf2a4..685fb9a1 100644 --- a/packages/markform/src/engine/table/parseTable.ts +++ b/packages/markform/src/engine/table/parseTable.ts @@ -136,6 +136,26 @@ export function parseCellValue(rawValue: string, columnType: ColumnTypeName): Ce } } +// ============================================================================= +// Empty Row Detection +// ============================================================================= + +/** + * Check if a table row is fully empty (all cells skipped/empty). + * Used during normalization to drop rows that carry no data. + * + * Aborted cells are NOT considered empty — they carry intentional signal + * (the agent explicitly declined to fill them, possibly with a reason). + */ +export function isRowFullyEmpty(row: TableRowResponse): boolean { + return Object.values(row).every((cell) => { + if (!cell || cell.state === 'skipped') return true; + if (cell.state === 'aborted') return false; + // 'answered' cell with no meaningful value + return cell.value === undefined || cell.value === null || cell.value === ''; + }); +} + // ============================================================================= // Raw Table Parsing // ============================================================================= @@ -311,7 +331,8 @@ export function parseMarkdownTable( rows.push(row); } - return { ok: true, value: { kind: 'table', rows } }; + const substantiveRows = rows.filter((r) => !isRowFullyEmpty(r)); + return { ok: true, value: { kind: 'table', rows: substantiveRows } }; } // Build column ID to index mapping from headers (for inline column parsing) @@ -344,7 +365,8 @@ export function parseMarkdownTable( rows.push(row); } - return { ok: true, value: { kind: 'table', rows } }; + const substantiveRows = rows.filter((r) => !isRowFullyEmpty(r)); + return { ok: true, value: { kind: 'table', rows: substantiveRows } }; } /** @@ -556,5 +578,6 @@ export function parseInlineTable(content: string): ParseTableResult { rows.push(row); } - return { ok: true, value: { kind: 'table', rows } }; + const substantiveRows = rows.filter((r) => !isRowFullyEmpty(r)); + return { ok: true, value: { kind: 'table', rows: substantiveRows } }; } diff --git a/packages/markform/src/engine/validate.ts b/packages/markform/src/engine/validate.ts index 17777560..cc72690f 100644 --- a/packages/markform/src/engine/validate.ts +++ b/packages/markform/src/engine/validate.ts @@ -964,6 +964,7 @@ function validateTableRow( columns: TableColumn[], fieldId: string, rowIndex: number, + fieldLabel: string, ): ValidationIssue[] { const issues: ValidationIssue[] = []; @@ -972,6 +973,29 @@ function validateTableRow( issues.push(...validateCellValue(cell, column, fieldId, rowIndex)); } + // Check for mostly-empty rows (more than half of cells empty in a non-empty row) + const totalCells = columns.length; + const filledCells = columns.filter((col) => { + const cell = row[col.id]; + return ( + cell?.state === 'answered' && + cell.value !== undefined && + cell.value !== null && + cell.value !== '' + ); + }).length; + + if (filledCells > 0 && filledCells < totalCells && totalCells - filledCells > totalCells / 2) { + issues.push({ + severity: 'warning', + message: + `Row ${rowIndex + 1} of "${fieldLabel}" has most cells empty ` + + `(${filledCells} of ${totalCells} filled).`, + ref: `${fieldId}[${rowIndex}]`, + source: 'builtin', + }); + } + return issues; } @@ -994,13 +1018,8 @@ function validateTableField(field: TableField, value: TableValue | undefined): V return issues; } - // Skip other checks if no value - if (isEmpty) { - return issues; - } - - // Check minRows - if (field.minRows !== undefined && rows.length < field.minRows) { + // Check minRows (even when empty, since minRows > 0 implies data is needed) + if (field.minRows !== undefined && field.minRows > 0 && rows.length < field.minRows) { issues.push({ severity: 'error', message: `"${field.label}" must have at least ${field.minRows} row(s) (has ${rows.length})`, @@ -1009,6 +1028,11 @@ function validateTableField(field: TableField, value: TableValue | undefined): V }); } + // Skip other checks if no value + if (isEmpty) { + return issues; + } + // Check maxRows if (field.maxRows !== undefined && rows.length > field.maxRows) { issues.push({ @@ -1021,7 +1045,7 @@ function validateTableField(field: TableField, value: TableValue | undefined): V // Validate each row for (let i = 0; i < rows.length; i++) { - issues.push(...validateTableRow(rows[i]!, field.columns, field.id, i)); + issues.push(...validateTableRow(rows[i]!, field.columns, field.id, i, field.label)); } return issues; diff --git a/packages/markform/tests/unit/engine/apply.test.ts b/packages/markform/tests/unit/engine/apply.test.ts index 65a7a838..cf056f42 100644 --- a/packages/markform/tests/unit/engine/apply.test.ts +++ b/packages/markform/tests/unit/engine/apply.test.ts @@ -2230,6 +2230,123 @@ markform: }); }); + describe('empty row filtering in set_table', () => { + const tableForm = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="data" label="Data" columnIds=["name", "role"] %} +| Name | Role | +|------|------| +{% /field %} +{% /group %} + +{% /form %} +`; + + it('filters out fully-empty rows from set_table', () => { + const form = parseForm(tableForm); + const patches: Patch[] = [ + { + op: 'set_table', + fieldId: 'data', + value: [ + { name: 'Alice', role: 'Eng' }, + { name: null, role: null }, + { name: 'Bob', role: 'PM' }, + ], + }, + ]; + const result = applyPatches(form, patches); + expect(result.applyStatus).toBe('applied'); + const value = form.responsesByFieldId.data?.value; + if (value?.kind === 'table') { + expect(value.rows).toHaveLength(2); + expect(value.rows[0]?.name).toEqual({ state: 'answered', value: 'Alice' }); + expect(value.rows[1]?.name).toEqual({ state: 'answered', value: 'Bob' }); + } + }); + + it('sets state to unanswered when all rows are empty', () => { + const form = parseForm(tableForm); + const patches: Patch[] = [ + { + op: 'set_table', + fieldId: 'data', + value: [ + { name: null, role: null }, + { name: null, role: null }, + ], + }, + ]; + const result = applyPatches(form, patches); + expect(result.applyStatus).toBe('applied'); + expect(form.responsesByFieldId.data?.state).toBe('unanswered'); + }); + }); + + describe('empty row filtering in append_table', () => { + const tableForm = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="data" label="Data" columnIds=["name", "role"] %} +| Name | Role | +|------|------| +{% /field %} +{% /group %} + +{% /form %} +`; + + it('filters out empty rows from appended rows', () => { + const form = parseForm(tableForm); + applyPatches(form, [ + { op: 'set_table', fieldId: 'data', value: [{ name: 'Alice', role: 'Eng' }] }, + ]); + const patches: Patch[] = [ + { + op: 'append_table', + fieldId: 'data', + value: [ + { name: null, role: null }, + { name: 'Bob', role: 'PM' }, + ], + }, + ]; + const result = applyPatches(form, patches); + expect(result.applyStatus).toBe('applied'); + const value = form.responsesByFieldId.data?.value; + if (value?.kind === 'table') { + expect(value.rows).toHaveLength(2); + expect(value.rows[0]?.name).toEqual({ state: 'answered', value: 'Alice' }); + expect(value.rows[1]?.name).toEqual({ state: 'answered', value: 'Bob' }); + } + }); + + it('sets state to unanswered when appending only empty rows to empty table', () => { + const form = parseForm(tableForm); + const patches: Patch[] = [ + { + op: 'append_table', + fieldId: 'data', + value: [{ name: null, role: null }], + }, + ]; + const result = applyPatches(form, patches); + expect(result.applyStatus).toBe('applied'); + expect(form.responsesByFieldId.data?.state).toBe('unanswered'); + }); + }); + describe('delete_table patch', () => { const tableForm = `--- markform: diff --git a/packages/markform/tests/unit/engine/parseTable.test.ts b/packages/markform/tests/unit/engine/parseTable.test.ts index 8df9ae7d..7853189b 100644 --- a/packages/markform/tests/unit/engine/parseTable.test.ts +++ b/packages/markform/tests/unit/engine/parseTable.test.ts @@ -10,8 +10,13 @@ import { parseMarkdownTable, extractColumnsFromTable, parseInlineTable, + isRowFullyEmpty, } from '../../../src/engine/table/parseTable.js'; -import type { TableColumn, ColumnTypeName } from '../../../src/engine/coreTypes.js'; +import type { + TableColumn, + ColumnTypeName, + TableRowResponse, +} from '../../../src/engine/coreTypes.js'; describe('parseTable', () => { describe('parseCellValue', () => { @@ -419,4 +424,123 @@ describe('parseTable', () => { } }); }); + + describe('isRowFullyEmpty', () => { + const EMPTY_ROWS: [string, TableRowResponse][] = [ + ['all skipped', { name: { state: 'skipped' }, age: { state: 'skipped' } }], + ['empty object', {}], + [ + 'answered with empty values', + { name: { state: 'answered', value: '' }, age: { state: 'answered', value: undefined } }, + ], + ]; + + for (const [label, row] of EMPTY_ROWS) { + it(`returns true: ${label}`, () => { + expect(isRowFullyEmpty(row)).toBe(true); + }); + } + + it('returns false when any cell has a value', () => { + expect( + isRowFullyEmpty({ name: { state: 'answered', value: 'Alice' }, age: { state: 'skipped' } }), + ).toBe(false); + }); + + it('returns false for numeric zero (falsy but meaningful)', () => { + expect(isRowFullyEmpty({ score: { state: 'answered', value: 0 } })).toBe(false); + }); + + it('returns false for all-aborted row (aborted carries intentional signal)', () => { + expect(isRowFullyEmpty({ name: { state: 'aborted' }, age: { state: 'aborted' } })).toBe( + false, + ); + }); + + it('returns false for mixed skipped/aborted row', () => { + expect(isRowFullyEmpty({ name: { state: 'skipped' }, age: { state: 'aborted' } })).toBe( + false, + ); + }); + }); + + describe('empty row filtering in parseMarkdownTable', () => { + const COLUMNS: TableColumn[] = [ + { id: 'name', label: 'Name', type: 'string', required: false }, + { id: 'age', label: 'Age', type: 'number', required: false }, + ]; + + it('drops all-empty rows', () => { + const content = ` +| Name | Age | +|------|-----| +| | | +| | |`; + const result = parseMarkdownTable(content, COLUMNS); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.value.rows).toHaveLength(0); + } + }); + + it('retains filled rows and drops empty ones', () => { + const content = ` +| Name | Age | +|------|-----| +| Alice | 30 | +| | | +| Bob | 25 |`; + const result = parseMarkdownTable(content, COLUMNS); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.value.rows).toHaveLength(2); + expect(result.value.rows[0]?.name?.value).toBe('Alice'); + expect(result.value.rows[1]?.name?.value).toBe('Bob'); + } + }); + + it('drops rows with all %SKIP% sentinels', () => { + const content = ` +| Name | Age | +|------|-----| +| %SKIP% | %SKIP% | +| Alice | 30 |`; + const result = parseMarkdownTable(content, COLUMNS); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.value.rows).toHaveLength(1); + expect(result.value.rows[0]?.name?.value).toBe('Alice'); + } + }); + + it('retains row with at least one filled cell', () => { + const content = ` +| Name | Age | +|------|-----| +| Alice | |`; + const result = parseMarkdownTable(content, COLUMNS); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.value.rows).toHaveLength(1); + expect(result.value.rows[0]?.name?.value).toBe('Alice'); + } + }); + }); + + describe('empty row filtering in parseInlineTable', () => { + it('drops empty rows from inline tables', () => { + const content = ` +| Name | Age | +|------|-----| +| | | +| Alice | 30 | +| | |`; + const result = parseInlineTable(content); + expect(result.ok).toBe(true); + if (result.ok) { + expect(result.value.rows).toHaveLength(1); + expect(result.value.rows[0]?.name?.value).toBe('Alice'); + } + }); + }); }); diff --git a/packages/markform/tests/unit/engine/serialize.test.ts b/packages/markform/tests/unit/engine/serialize.test.ts index f181238d..4a37a004 100644 --- a/packages/markform/tests/unit/engine/serialize.test.ts +++ b/packages/markform/tests/unit/engine/serialize.test.ts @@ -1780,6 +1780,41 @@ markform: expect(output).toContain('Data unavailable'); }); + it('round-trips table with empty rows dropped', () => { + const markdown = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} +{% group id="g1" %} +{% field kind="table" id="data" label="Data" columnIds=["name", "role"] columnTypes=["string", "string"] %} +| Name | Role | +| --- | --- | +| Alice | Eng | +| | | +| Bob | PM | +| | | +{% /field %} +{% /group %} +{% /form %} +`; + const parsed = parseForm(markdown); + const tableVal = parsed.responsesByFieldId.data?.value; + if (tableVal?.kind === 'table') { + expect(tableVal.rows).toHaveLength(2); + } + + const output = serializeForm(parsed); + const reparsed = parseForm(output); + const reVal = reparsed.responsesByFieldId.data?.value; + if (reVal?.kind === 'table') { + expect(reVal.rows).toHaveLength(2); + expect(reVal.rows[0]?.name?.value).toBe('Alice'); + expect(reVal.rows[1]?.name?.value).toBe('Bob'); + } + }); + it('round-trips specVersion override', () => { const markdown = `--- markform: diff --git a/packages/markform/tests/unit/engine/summaries.test.ts b/packages/markform/tests/unit/engine/summaries.test.ts index fa5e0cae..b9f4ad33 100644 --- a/packages/markform/tests/unit/engine/summaries.test.ts +++ b/packages/markform/tests/unit/engine/summaries.test.ts @@ -769,6 +769,70 @@ John }); }); + describe('table with only empty rows treated as not submitted', () => { + it('table with only empty rows has empty=true after normalization', () => { + const markdown = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="data" label="Data" columnIds=["name", "age"] columnTypes=["string", "number"] %} +| Name | Age | +| --- | --- | +| | | +| | | +{% /field %} +{% /group %} + +{% /form %} +`; + const parsed = parseForm(markdown); + const progress = computeProgressSummary( + parsed.schema, + parsed.responsesByFieldId, + parsed.notes, + [], + ); + + expect(progress.fields.data?.empty).toBe(true); + expect(progress.counts.filledFields).toBe(0); + }); + + it('table with mixed empty/filled rows has empty=false', () => { + const markdown = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="data" label="Data" columnIds=["name", "age"] columnTypes=["string", "number"] %} +| Name | Age | +| --- | --- | +| Alice | 30 | +| | | +{% /field %} +{% /group %} + +{% /form %} +`; + const parsed = parseForm(markdown); + const progress = computeProgressSummary( + parsed.schema, + parsed.responsesByFieldId, + parsed.notes, + [], + ); + + expect(progress.fields.data?.empty).toBe(false); + expect(progress.counts.filledFields).toBe(1); + }); + }); + describe('empty vs answerState - orthogonal dimensions (markform-480)', () => { it('multi_select answered with no selections: empty=true, answerState=answered', () => { const markdown = `--- diff --git a/packages/markform/tests/unit/engine/validate.test.ts b/packages/markform/tests/unit/engine/validate.test.ts index 7e96caab..e290cad9 100644 --- a/packages/markform/tests/unit/engine/validate.test.ts +++ b/packages/markform/tests/unit/engine/validate.test.ts @@ -1437,6 +1437,165 @@ markform: {% /field %} {% /group %} +{% /form %} +`; + const form = parseForm(markdown); + const result = validate(form); + + expect(result.isValid).toBe(true); + }); + + it('warns on mostly-empty row (1 of 4 cells filled)', () => { + const markdown = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="items" label="Items" columnIds=["a", "b", "c", "d"] %} +| a | b | c | d | +|---|---|---|---| +| hello | | | | +{% /field %} +{% /group %} + +{% /form %} +`; + const form = parseForm(markdown); + const result = validate(form); + + const warnings = result.issues.filter((i) => i.severity === 'warning'); + expect(warnings).toHaveLength(1); + expect(warnings[0]?.message).toContain('most cells empty'); + expect(warnings[0]?.message).toContain('1 of 4 filled'); + expect(warnings[0]?.message).toContain('Items'); + expect(warnings[0]?.ref).toBe('items[0]'); + }); + + it('does not warn when half cells are filled (2 of 4)', () => { + const markdown = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="items" label="Items" columnIds=["a", "b", "c", "d"] %} +| a | b | c | d | +|---|---|---|---| +| hello | world | | | +{% /field %} +{% /group %} + +{% /form %} +`; + const form = parseForm(markdown); + const result = validate(form); + + const warnings = result.issues.filter((i) => i.severity === 'warning'); + expect(warnings).toHaveLength(0); + }); + + it('warns on mostly-empty row (1 of 3 cells filled)', () => { + const markdown = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="items" label="Items" columnIds=["a", "b", "c"] %} +| a | b | c | +|---|---|---| +| hello | | | +{% /field %} +{% /group %} + +{% /form %} +`; + const form = parseForm(markdown); + const result = validate(form); + + const warnings = result.issues.filter((i) => i.severity === 'warning'); + expect(warnings).toHaveLength(1); + expect(warnings[0]?.message).toContain('1 of 3 filled'); + }); + + it('does not warn on 1 of 2 cells filled (even split)', () => { + const markdown = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="items" label="Items" columnIds=["a", "b"] %} +| a | b | +|---|---| +| hello | | +{% /field %} +{% /group %} + +{% /form %} +`; + const form = parseForm(markdown); + const result = validate(form); + + const warnings = result.issues.filter((i) => i.severity === 'warning'); + expect(warnings).toHaveLength(0); + }); + + it('minRows fails when only empty rows are provided', () => { + const markdown = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="contacts" label="Contacts" columnIds=["name", "email"] minRows=2 %} +| Name | Email | +|------|-------| +| | | +| | | +| | | +{% /field %} +{% /group %} + +{% /form %} +`; + const form = parseForm(markdown); + const result = validate(form); + + expect(result.isValid).toBe(false); + expect(result.issues.some((i) => i.message.includes('at least 2 row'))).toBe(true); + }); + + it('minRows passes when enough filled rows after empty row dropping', () => { + const markdown = `--- +markform: + spec: MF/0.1 +--- + +{% form id="test" %} + +{% group id="g1" %} +{% field kind="table" id="contacts" label="Contacts" columnIds=["name", "email"] minRows=2 %} +| Name | Email | +|------|-------| +| Alice | alice@test.com | +| | | +| Bob | bob@test.com | +| | | +{% /field %} +{% /group %} + {% /form %} `; const form = parseForm(markdown);