fix(skills): add NULL-safe SQL scan guidance to Phase 3 store-query starter

7396d66fe58eaec3c5b557748a07124ef12f066b · 2026-05-16 13:35:41 -0700 · Trevin Chow

Novel-feature commands authored against generated typed FTS/upsert
tables or against json_extract on the resources blob silently drop
every row whose scanned column is NULL. `database/sql`'s `rows.Scan`
into a bare `string`/`int64`/`float64` returns a non-nil error on NULL
("converting NULL to string is unsupported"), and the conventional
`for rows.Next()` loop continues past scan errors — so the query
returns zero records, no error reaches the caller, and the feature
looks healthy because the upstream API call succeeded.

The Phase 3 store-query RunE skeleton in skills/printing-press/SKILL.md
documents the resources-table-keyed query pattern but says nothing
about NULL handling. Add an inline scan-comment in the skeleton and a
parallel-shape "NULL-safe SQL scans MUST use sql.Null* targets (or
COALESCE) for any column that can be NULL" paragraph after the
streaming-frame normalizers callout, with right/wrong code snippets.

The generator does not emit per-resource typed query helpers
(writeThroughCache stores opaque JSON via UpsertBatch; resolveLocal
reads opaque JSON), so the issue's `comp:generator` framing was off —
the fix surface is the SKILL prompt that drives Phase 3 novel-feature
authoring.

Closes #1438
Refs #1469 (NULL-safe SQL sub-bullet addressed here; the
helper-enumeration sub-bullet remains open under that issue)

Files touched

M skills/printing-press/SKILL.md

Diff

commit 7396d66fe58eaec3c5b557748a07124ef12f066b
Author: Trevin Chow <trevin@trevinchow.com>
Date:   Sat May 16 13:35:41 2026 -0700

    fix(skills): add NULL-safe SQL scan guidance to Phase 3 store-query starter
    
    Novel-feature commands authored against generated typed FTS/upsert
    tables or against json_extract on the resources blob silently drop
    every row whose scanned column is NULL. `database/sql`'s `rows.Scan`
    into a bare `string`/`int64`/`float64` returns a non-nil error on NULL
    ("converting NULL to string is unsupported"), and the conventional
    `for rows.Next()` loop continues past scan errors — so the query
    returns zero records, no error reaches the caller, and the feature
    looks healthy because the upstream API call succeeded.
    
    The Phase 3 store-query RunE skeleton in skills/printing-press/SKILL.md
    documents the resources-table-keyed query pattern but says nothing
    about NULL handling. Add an inline scan-comment in the skeleton and a
    parallel-shape "NULL-safe SQL scans MUST use sql.Null* targets (or
    COALESCE) for any column that can be NULL" paragraph after the
    streaming-frame normalizers callout, with right/wrong code snippets.
    
    The generator does not emit per-resource typed query helpers
    (writeThroughCache stores opaque JSON via UpsertBatch; resolveLocal
    reads opaque JSON), so the issue's `comp:generator` framing was off —
    the fix surface is the SKILL prompt that drives Phase 3 novel-feature
    authoring.
    
    Closes #1438
    Refs #1469 (NULL-safe SQL sub-bullet addressed here; the
    helper-enumeration sub-bullet remains open under that issue)
---
 skills/printing-press/SKILL.md | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/skills/printing-press/SKILL.md b/skills/printing-press/SKILL.md
index f4bc614b..c4198fbf 100644
--- a/skills/printing-press/SKILL.md
+++ b/skills/printing-press/SKILL.md
@@ -2556,6 +2556,11 @@ RunE: func(cmd *cobra.Command, args []string) error {
 		return fmt.Errorf("query: %w", err)
 	}
 	defer rows.Close()
+	// Scan each row. id/data on the resources table are NOT NULL so bare
+	// strings are safe; ANY optional field selected via json_extract or
+	// pulled from a typed FTS/upsert table can be NULL — use sql.Null*
+	// scan targets (or COALESCE in the SQL) for those, see the NULL-safe
+	// scans paragraph below.
 	var results []yourRowType // scan rows into the slice
 	if flags.asJSON || (!isTerminal(cmd.OutOrStdout()) && !humanFriendly) {
 		enc := json.NewEncoder(cmd.OutOrStdout())
@@ -2575,6 +2580,22 @@ For features that combine both (cache an API response in the store, or fall thro
 
 **Streaming frame normalizers MUST use `cliutil.ExtractNumber` / `cliutil.ExtractInt` rather than raw `float64`/`int64` struct fields.** Real-world WebSocket and streaming JSON feeds (Binance, Coinbase, Kraken, Stripe `*_decimal`, vendor-specific market-data feeds) commonly encode numeric values as JSON-encoded strings (`"price":"1.91"`). `json.Unmarshal` of a JSON string into a `float64` field returns no error and silently leaves the field at 0; combined with NULL-on-zero patterns this discards the entire numeric feed with no error signal anywhere in the pipeline. The helpers accept both shapes (JSON number or JSON-encoded string), report `ok=false` on missing/null/unparseable, and are the canonical extraction path for `map[string]json.RawMessage` decoders. Re-implementing this inline as a `float64` struct field is the silent-aggregation-failure bug class.
 
+**NULL-safe SQL scans MUST use `sql.Null*` scan targets (or `COALESCE(<col>, <zero>)` in the query) for any column that can be NULL.** SQLite returns NULL for any absent JSON field selected via `json_extract(data, '$.optional_field')`, for any nullable column in a typed FTS/upsert table the generator emits, and for any field the API omits from a particular response. `database/sql`'s `rows.Scan` into a bare `string`/`int64`/`float64` returns a non-nil error on NULL (`Scan error on column index N: converting NULL to string is unsupported`) — and the surrounding `for rows.Next()` loop typically `continue`s on scan error, silently dropping every row. The result: queries return zero records, no error reaches the caller, the feature looks healthy because the API call succeeded. Use `var v sql.NullString` (or `NullInt64` / `NullFloat64` / `NullTime`) as the scan target and copy `.String` / `.Int64` / `.Float64` / `.Time` into your row struct, accepting the zero value as the missing-field representation. Re-implementing this inline as bare-string scans is the silent-row-drop bug class.
+
+```go
+// Wrong — every NULL column kills the row.
+var name string
+if err := rows.Scan(&id, &name); err != nil { continue }
+
+// Right — NULL becomes the zero value, no row is lost.
+var name sql.NullString
+if err := rows.Scan(&id, &name); err != nil { continue }
+result.Name = name.String
+
+// Also right — push the default into the query so the scan target stays bare.
+SELECT id, COALESCE(json_extract(data, '$.name'), '') FROM resources WHERE ...
+```
+
 **Typed exit-code verification:** If a novel command intentionally returns a non-zero code for a non-error control-flow result, add `cmd.Annotations["pp:typed-exit-codes"] = "0,<code>"` (or the equivalent `Annotations: map[string]string{...}` literal) and document the same command-specific codes in its help. Do not list the global failure palette in command help unless those exits should count as a verify pass for that command; keep general exit-code troubleshooting in README/SKILL prose.
 
 **MCP exposure:** The generator emits `internal/mcp/cobratree/`, and the MCP binary mirrors the Cobra tree at startup. When you add, rename, or remove a user-facing Cobra command, the MCP surface follows automatically. Two annotations control how each command appears as an MCP tool:

← c31632b3 feat(skills): add /printing-press-amend skill — dogfood + di · back to Cli Printing Press · fix(cli): scope-aware sync --param dispatch with --global-pa 4e304df5 →