diff --git a/.tbd/workspaces/outbox/issues/is-01khx9xwpv03xfg2cc5aqtwn8g.md b/.tbd/workspaces/outbox/issues/is-01khx9xwpv03xfg2cc5aqtwn8g.md deleted file mode 100644 index fba8237b..00000000 --- a/.tbd/workspaces/outbox/issues/is-01khx9xwpv03xfg2cc5aqtwn8g.md +++ /dev/null @@ -1,22 +0,0 @@ ---- -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 deleted file mode 100644 index ddfc19e9..00000000 --- a/.tbd/workspaces/outbox/issues/is-01khx9y5kgsweghjsdnt8wze22.md +++ /dev/null @@ -1,21 +0,0 @@ ---- -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 deleted file mode 100644 index a0b13e86..00000000 --- a/.tbd/workspaces/outbox/issues/is-01khx9yd02anf0w14rkt37mcej.md +++ /dev/null @@ -1,19 +0,0 @@ ---- -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 deleted file mode 100644 index 09314fdb..00000000 --- a/.tbd/workspaces/outbox/issues/is-01khx9ym9e1d2dyb3nh3dqs7qe.md +++ /dev/null @@ -1,19 +0,0 @@ ---- -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 deleted file mode 100644 index 18a7d9ad..00000000 --- a/.tbd/workspaces/outbox/issues/is-01khx9yvwgs8af6m7q1yp8jb6s.md +++ /dev/null @@ -1,19 +0,0 @@ ---- -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 deleted file mode 100644 index e7596b4d..00000000 --- a/.tbd/workspaces/outbox/issues/is-01khx9z3qgwx2s3gfnwbsnv45j.md +++ /dev/null @@ -1,17 +0,0 @@ ---- -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 deleted file mode 100644 index cd18a794..00000000 --- a/.tbd/workspaces/outbox/issues/is-01khy5qjtzdbp2189eh4x1xkea.md +++ /dev/null @@ -1,15 +0,0 @@ ---- -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 deleted file mode 100644 index 7e1916dc..00000000 --- a/.tbd/workspaces/outbox/mappings/ids.yml +++ /dev/null @@ -1,7 +0,0 @@ -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 ec30f421..8d3431e9 100644 --- a/docs/markform-reference.md +++ b/docs/markform-reference.md @@ -450,8 +450,9 @@ A row with at least one non-empty cell is retained. | `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. +**Row sparseness warning:** When a non-empty row has more than half of its cells empty +(e.g., 1 of 3 filled, or 1 of 4 filled; even splits like 2 of 4 don’t trigger this), 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: diff --git a/docs/markform-spec.md b/docs/markform-spec.md index 06dff8d3..2d1f60fb 100644 --- a/docs/markform-spec.md +++ b/docs/markform-spec.md @@ -493,8 +493,9 @@ 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. +**Row sparseness warning:** When a non-empty row has more than half of its cells empty +(e.g., 1 of 3 filled, or 1 of 4 filled; even splits like 2 of 4 don’t trigger this), a +validation warning is emitted. This helps catch cases where an agent produced sparse or incomplete data. **Example with per-column constraints:** diff --git a/packages/markform/src/engine/table/parseTable.ts b/packages/markform/src/engine/table/parseTable.ts index 685fb9a1..e2ce5456 100644 --- a/packages/markform/src/engine/table/parseTable.ts +++ b/packages/markform/src/engine/table/parseTable.ts @@ -137,23 +137,35 @@ export function parseCellValue(rawValue: string, columnType: ColumnTypeName): Ce } // ============================================================================= -// Empty Row Detection +// Empty Cell Detection // ============================================================================= +/** + * Check if a single cell is empty. + * Used by both row normalization and validation to ensure consistent logic. + * + * - Skipped cells are empty + * - Aborted cells are NOT empty (they carry intentional signal) + * - Answered cells are empty only if value is undefined/null/empty string + * - Unknown states are treated conservatively as NOT empty + */ +export function isCellEmpty(cell: CellResponse | undefined): boolean { + if (!cell || cell.state === 'skipped') return true; + if (cell.state === 'aborted') return false; + // Only 'answered' cells can be empty based on value check + if (cell.state === 'answered') { + return cell.value === undefined || cell.value === null || cell.value === ''; + } + // Unknown state - treat conservatively as NOT empty + return false; +} + /** * 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 === ''; - }); + return Object.values(row).every(isCellEmpty); } // ============================================================================= diff --git a/packages/markform/src/engine/validate.ts b/packages/markform/src/engine/validate.ts index cc72690f..d8e8c2a4 100644 --- a/packages/markform/src/engine/validate.ts +++ b/packages/markform/src/engine/validate.ts @@ -42,6 +42,7 @@ import type { YearField, YearValue, } from './coreTypes.js'; +import { isCellEmpty } from './table/parseTable.js'; // ============================================================================= // Validation Options and Results @@ -975,15 +976,7 @@ function validateTableRow( // 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; + const filledCells = columns.filter((col) => !isCellEmpty(row[col.id])).length; if (filledCells > 0 && filledCells < totalCells && totalCells - filledCells > totalCells / 2) { issues.push({ diff --git a/packages/markform/tests/unit/engine/parseTable.test.ts b/packages/markform/tests/unit/engine/parseTable.test.ts index 7853189b..f7266318 100644 --- a/packages/markform/tests/unit/engine/parseTable.test.ts +++ b/packages/markform/tests/unit/engine/parseTable.test.ts @@ -11,6 +11,7 @@ import { extractColumnsFromTable, parseInlineTable, isRowFullyEmpty, + isCellEmpty, } from '../../../src/engine/table/parseTable.js'; import type { TableColumn, @@ -19,6 +20,35 @@ import type { } from '../../../src/engine/coreTypes.js'; describe('parseTable', () => { + describe('isCellEmpty', () => { + it('returns true for undefined cell', () => { + expect(isCellEmpty(undefined)).toBe(true); + }); + + it('returns true for skipped cell', () => { + expect(isCellEmpty({ state: 'skipped' })).toBe(true); + }); + + it('returns false for aborted cell', () => { + expect(isCellEmpty({ state: 'aborted' })).toBe(false); + }); + + it('returns true for answered cell with empty value', () => { + expect(isCellEmpty({ state: 'answered', value: '' })).toBe(true); + expect(isCellEmpty({ state: 'answered', value: undefined })).toBe(true); + expect(isCellEmpty({ state: 'answered', value: null as any })).toBe(true); + }); + + it('returns false for answered cell with meaningful value', () => { + expect(isCellEmpty({ state: 'answered', value: 'test' })).toBe(false); + expect(isCellEmpty({ state: 'answered', value: 0 })).toBe(false); + }); + + it('returns false for unknown state (conservative)', () => { + expect(isCellEmpty({ state: 'unknown' as any })).toBe(false); + }); + }); + describe('parseCellValue', () => { // [rawValue, columnType, expectedState, expectedValue] type CellCase = [ @@ -462,6 +492,23 @@ describe('parseTable', () => { false, ); }); + + it('treats malformed cell with unknown state conservatively (not empty)', () => { + // Simulate a malformed/corrupted cell with an unexpected state + // This should be treated as NOT empty (conservative approach) + const malformedRow = { + name: { state: 'unknown' as any, value: 'test' }, + }; + expect(isRowFullyEmpty(malformedRow)).toBe(false); + }); + + it('treats malformed cell with unknown state and no value conservatively (not empty)', () => { + // Even with no value, unknown state should be treated conservatively + const malformedRow = { + name: { state: 'unknown' as any, value: undefined }, + }; + expect(isRowFullyEmpty(malformedRow)).toBe(false); + }); }); describe('empty row filtering in parseMarkdownTable', () => { diff --git a/packages/markform/tests/unit/engine/validate.test.ts b/packages/markform/tests/unit/engine/validate.test.ts index e290cad9..7bee49d2 100644 --- a/packages/markform/tests/unit/engine/validate.test.ts +++ b/packages/markform/tests/unit/engine/validate.test.ts @@ -1445,8 +1445,9 @@ markform: expect(result.isValid).toBe(true); }); - it('warns on mostly-empty row (1 of 4 cells filled)', () => { - const markdown = `--- + describe('mostly-empty row warnings', () => { + it('warns on mostly-empty row (1 of 4 cells filled)', () => { + const markdown = `--- markform: spec: MF/0.1 --- @@ -1463,19 +1464,19 @@ markform: {% /form %} `; - const form = parseForm(markdown); - const result = validate(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]'); - }); + 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 = `--- + it('does not warn when half cells are filled (2 of 4)', () => { + const markdown = `--- markform: spec: MF/0.1 --- @@ -1492,15 +1493,15 @@ markform: {% /form %} `; - const form = parseForm(markdown); - const result = validate(form); + const form = parseForm(markdown); + const result = validate(form); - const warnings = result.issues.filter((i) => i.severity === 'warning'); - expect(warnings).toHaveLength(0); - }); + 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 = `--- + it('warns on mostly-empty row (1 of 3 cells filled)', () => { + const markdown = `--- markform: spec: MF/0.1 --- @@ -1517,16 +1518,16 @@ markform: {% /form %} `; - const form = parseForm(markdown); - const result = validate(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'); - }); + 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 = `--- + it('does not warn on 1 of 2 cells filled (even split)', () => { + const markdown = `--- markform: spec: MF/0.1 --- @@ -1543,11 +1544,12 @@ markform: {% /form %} `; - const form = parseForm(markdown); - const result = validate(form); + const form = parseForm(markdown); + const result = validate(form); - const warnings = result.issues.filter((i) => i.severity === 'warning'); - expect(warnings).toHaveLength(0); + const warnings = result.issues.filter((i) => i.severity === 'warning'); + expect(warnings).toHaveLength(0); + }); }); it('minRows fails when only empty rows are provided', () => {