fix(sdk): give table-cell annotations semantic row and column names

[adelin-b]

Intent

Fix Lavish table cell annotations so filtered or sorted rows retain semantic row and column identity while preserving the exact clicked locator.

What Changed

• Added src/table-cell.js, which resolves the clicked cell's visible row and column names and attaches them as a type: "table-cell" target; the clicked element's own selector, tag, and text are unchanged, so the CSS locator still points at exactly what was highlighted. Naming is suppressed rather than guessed where a merge makes it unprovable: spans are read from the browser-parsed rowSpan/colSpan (falling back to HTML's integer-parsing rules on the raw attribute), a rowspan starting earlier in a row's own <thead>/<tbody>/<tfoot> group drops the row name unless a <th scope="row"> is declared, and a column name requires an unshifted header row whose colspans sum to the clicked row's with no straddling cell.
• Wired the target through the UI: context() in src/artifact-sdk.js takes an opt-in { table: true } so only the annotation card pays the table walk (snapshot() does not), and the card heading/placeholder now read "Annotate cell: Row → Column" for a cell click or "Annotate <tag> in Row → Column" for a nested element. src/chrome-client.js shows the semantic name as the queued pill's Target and the CSS selector as a separate Locator row when they differ.
• Generalized the SDK bundling in src/server.js: serializeModuleHelpers now serializes any shared module's exports as same-scope consts (throwing on a non-function export instead of shipping an empty {}), and it injects src/table-cell.js alongside src/mermaid-node.js. README and AGENTS.md document the new target contract and suppression rules, and .gitignore stops tracking local .omc/ runtime state apart from .omc/skills/.

Risk Assessment

✅ Low: The table-cell logic is spec-correct on row-group clipping and span parsing, fails closed by omitting unprovable labels rather than guessing, is covered by behavioral tests that boot the real served bundle, and has its contract documented on the owning surfaces; the open items are a scope question about the artifact-driven queuePrompt path and branch-history hygiene, neither of which can produce a wrong label.

Testing

• ⏭️ Test - skipped

Pipeline

Updates from git push no-mistakes

✅ **intent** - passed

✅ No issues found.

✅ **Rebase** - passed

✅ No issues found.

⚠️ **Review** - 3 issues (1 warning, 2 infos)

• ⚠️ src/table-cell.js:114 - tableHasRowSpan(table) scans the whole table, so shifted is true for a rowspan confined to <thead> — the standard grouped-header idiom <th rowspan="2">. That suppresses rowLabel (line 128) for every <tbody> row, even though a rowspan cannot cross a row-group boundary and therefore cannot shift any body row's cells, so the positional cells[0] heading is still provable there. Concrete path: <table><thead><tr><th rowspan="2">Feature</th><th colspan="2">Result</th></tr><tr><th>A</th><th>B</th></tr></thead><tbody><tr><td>Login</td><td>ok</td><td>ok</td></tr>…</tbody></table>; filter to a subset and click an ok cell — the target comes back {rowLabel: "", columnLabel: ""} and the agent is left with only tr:nth-of-type(n), which is exactly the misdirected-feedback failure this change targets. (columnLabel is correctly empty here on its own merits: the leaf header row is genuinely shifted, so its width no longer matches the body row's.) Narrowing the row-label guard to rowspans in the clicked cell's own row group at or above its row would keep the never-guess invariant and restore coverage for this shape. This tradeoff is explicitly documented in README.md:207 and AGENTS.md, so it reads as a deliberate conservative choice rather than an oversight — flagging it as the author's call, not a blocker.
• ℹ️ src/table-cell.js:50 - tableCellSpansRows parses the raw rowspan attribute string in preference to the browser-parsed cell.rowSpan. HTML's rules for parsing non-negative integers ignore trailing garbage, so rowspan="2x" renders as a real two-row span, but Number("2x") is NaN, Number.isFinite(raw) is false, and the helper reports no rowspan. shifted is then false at line 114 and tableCellTarget emits positional rowLabel/columnLabel for a grid that is actually shifted — the fail-open outcome the module's own comments say must never happen (every other malformed value, e.g. rowspan="1.5", fails closed instead). tableColumnSpan at line 41 has the same attribute-first shape, though there a mismatch usually trips the header/row width check and fails closed. Reading cell.rowSpan/cell.colSpan first when finite and falling back to the attribute parse is strictly more spec-correct (browsers already handle rowspan="" → 1 and rowspan="0" → 0) and leaves the existing DOM-stub unit tests, which define neither property, on the current fallback path.

🔧 Fix: scope table rowspan guard to row group; parse spans per HTML
3 issues (1 warning, 2 infos) still open:

• ⚠️ src/artifact-sdk.js:704 - A fix round gated the table lookup to annotation clicks: context() only resolves target under { table: true }, which showAnnotationCard passes (line 1684) but queuePrompt does not (line 704). The stated rationale — "snapshot() calls this for every element in the document" (line 307) — applies only to snapshot(); queuePrompt runs once per queued prompt, so the cost there is one bounded table walk. The author's original commit (f8b94c7) attached the target in context() for every caller, so this is a narrowing the pipeline introduced, and the new bundle test at test/artifact-sdk-bundle.test.js:246 locks it in. Reachable path: the input playbook (src/playbooks.js:196) tells artifact authors to build choices from native controls, and the table playbook (src/playbooks.js:62) tells them to make rows easy targets; a per-row radio plus a submit that calls lavish.queuePrompt in a client-filtered table now delivers selector: "...tr:nth-of-type(7) > td..." and no target — exactly the misdirected-row-number problem the intent names, just on the artifact-driven path instead of the click path. README.md:206 scopes the user-facing contract to "Clicking an element inside a table", so this is a scope decision rather than a broken contract: confirm whether artifact-queued prompts from inside a table should also carry the row/column names (one-line change: context(originElement, { table: true })).
• ℹ️ src/artifact-sdk.js:320 - The fix round replaced the author's cell-level identity with element-level identity plus a side-channel target. In f8b94c7 a click inside a cell set base.tag = "table-cell", overwrote base.selector with the cell's selector, and coarsened base.text to the cell text; now selector/tag/text keep describing the clicked element and only target carries the cell (lines 317-321), with the card heading branching on c.target?.type and a new "Annotate <code> in Row → Column" wording for nested clicks (lines 1699-1715). I judge the new contract the better one — it matches the intent's "preserving the exact clicked locator", keeps tag a real tag name in line with the target contract in AGENTS.md, and keeps the heading consistent with the on-screen highlight, which outlines the clicked element and not the cell. Flagging it only because it is a user-visible change to the author's deliberate design made by the pipeline rather than by the author: confirm the element-identity-plus-target shape is the wanted contract.
• ℹ️ .gitignore:10 - Commit 3531311 committed ~1.1k lines of local agent runtime state under .omc/ (project-memory.json with the absolute worktree path under /Users/adelinb, session tool-error state, session-end job tickets), and 8fa4344 deleted it again and added this ignore block. The working tree is therefore clean — the base..HEAD diff carries only these three .gitignore lines — but the blobs are still reachable in the branch's own history, so they land in main if this branch is merged with history preserved rather than squashed. Nothing credential-bearing is in them, but they are one contributor's tool state in a public, npm-published repo. Two calls for the author: squash (or drop those two commits) before merge, and decide whether an ignore rule for a contributor-local agent tool belongs in the shared .gitignore at all, since it is unrelated to the table-cell intent.

⏭️ **Test** - skipped

Step was skipped.

⏭️ **Document** - skipped

Step was skipped.

⏭️ **Lint** - skipped

Step was skipped.

✅ **Push** - passed

✅ No issues found.

[kunchenguid]

Speaking as Kun's firstmate: this is a good corrective — semantic row and column names, fail-closed, locator unchanged. It's conflicted with current main (createSdkJs now takes attachment options), and I can't push your fork, so I'm landing the same change onto main via a follow-up rather than leaving the conflict with you.

[kunchenguid]

Follow-up onto current main (conflict resolution, same contracts): #256

This does not close or rebase #242.

[kunchenguid]

Speaking as Kun's firstmate: the conflict follow-up is up as #256 (same table-cell contracts on current main; your fork stays as-is). It is not mergeable yet — this repo requires git push no-mistakes, and the follow-up still needs that signature. I'll keep #242 open.

[kunchenguid]

Speaking as Kun's firstmate: the conflict follow-up #256 is now merged (semantic table-cell names on main). Leaving this PR open so you can close it when you want.