# M6.3 — store.go query port to Postgres

| Field | Value |
|---|---|
| Phase | M6 — agent-manager migration |
| Size | L — 3+ days |
| Status | ⬜ Not started |
| Flags | — |
| Spec | inlined below |
| Blocks | M6.2 |

## Goal

Every query in `internal/store/store.go` rewritten against the M6.2 schema,
sqlite dropped entirely (decided: full port, not a dual sqlite/Postgres
backend — this is single-machine usage, no standalone-without-cluster
requirement to preserve).

## Facts (inlined — no spec read needed)

Read directly from `internal/store/store.go` (1397 lines) on the
`add-headless-spawn` branch:

- **65 call sites** (`db.Exec`, `db.QueryRow`, `db.Query`, `tx.Exec`,
  `tx.QueryRow`) use sqlite `?` positional placeholders. Postgres
  (`pgx` or `lib/pq`) needs `$1, $2, ...` — mechanical but must be done
  per-statement since arg count varies 1–16 across sites.
- **Upserts are already Postgres-compatible.** All 9 upsert sites
  (`SetReviewRepo`, `SetReviewBase`, `SetReviewScope`, `SetSetting`,
  `ensureGroup`, `CreateGroup`, `AddGroup`, `createSession`'s group
  insert, `PlaceSession`'s group insert) already use `ON
  CONFLICT(...) DO UPDATE SET ... = excluded....` / `ON CONFLICT(...) DO
  NOTHING`, valid as-is in Postgres. No rewrite beyond the placeholder
  swap — this corrects an earlier assumption that these were sqlite
  `INSERT OR REPLACE` and needed real rework.
- `boolToInt()`/`archived != 0`-style conversions at ~8 sites go away —
  scan `int archived`/`int acked`/`int pendingClaimed`/`int` locals in
  `ListSessions`, `Get` and replace with `bool` directly.
- `encodeTime()`/`decodeTime()` calls at every session-timestamp
  read/write go away — pass `time.Time` straight through; `pgx` handles
  `TIMESTAMPTZ` natively. Delete `timeenc_test.go` along with the
  functions it tests (M6.2 already noted its coverage folds into the
  schema tests instead).
- `db.SetMaxOpenConns(1)` + `PRAGMA journal_mode=WAL`
  (store.go:76-79) gave sqlite serialized writes for free. Some call
  sites lean on that implicit serialization — most notably
  `createSession`'s `sort_order = (SELECT COALESCE(MAX(sort_order)+1, 0)
  FROM sessions WHERE group_name = ? AND parent_id = ?)` subquery, which
  races under concurrent Postgres writers with a real connection pool.
  Wrap it in `SELECT ... FOR UPDATE` inside the existing transaction (the
  function already opens one) rather than trusting single-writer
  semantics that no longer exist. Same pattern applies to
  `ReorderSession`/`SwapSessionOrder`/`ReorderGroup`/`SwapGroupOrder`'s
  read-then-renumber-then-write sequences.
- FK violations are new failure modes M6.2 introduced (group/parent FKs).
  `createSession` and `PlaceSession` already validate `group_name`/
  `parent_id` through `validParent`/`ensureGroup` before writing, so those
  paths shouldn't hit a live constraint in practice — but wrap the actual
  `INSERT`/`UPDATE` error and translate a `23503` (foreign_key_violation)
  SQLSTATE into the same descriptive errors the pre-validation already
  produces, so a race between the check and the write degrades to a clear
  error instead of a raw driver error reaching the CLI.
- `driver: modernc.org/sqlite` import and the blank `_ "modernc.org/
  sqlite"` in `Open()` are deleted; replaced with `pgx` (`github.com/
  jackc/pgx/v5/stdlib` for `database/sql` compatibility, keeping the rest
  of the file's `*sql.DB`-based code unchanged) or a native `pgx.Pool` —
  pick `pgx/v5/stdlib` unless a later task needs pgx-native features
  (e.g. `COPY`), since it's the smaller diff against the existing
  `database/sql` code.

## Steps

1. Swap the driver import and `Open()`'s connection string handling
   (sqlite file path -> Postgres DSN, likely from an env var or flag the
   CLI already has a slot for — check `cmd/` for how `Open()` is called
   today).
2. Delete `db.SetMaxOpenConns(1)` / `PRAGMA journal_mode=WAL`; size the
   pool deliberately instead (`SetMaxOpenConns` to something sane for a
   single-machine client, e.g. 5–10).
3. Delete `init()`'s `CREATE TABLE IF NOT EXISTS` + 14-migration list —
   M6.2's migration owns schema creation now; `Open()` just connects and
   optionally runs a `schema_version` sanity check.
4. Mechanically convert every `?` to `$N` across the 65 call sites, in
   file order, verifying arg count against placeholder count each time
   (this is where an off-by-one is easiest to introduce silently).
5. Remove `boolToInt`/`archived != 0` conversions; scan struct fields
   directly as `bool`.
6. Remove `encodeTime`/`decodeTime`/`secondsCeiling`; pass `time.Time`
   directly. Delete `timeenc_test.go`.
7. Add `SELECT ... FOR UPDATE` (or equivalent explicit locking) to the
   sort-order read-then-write sequences named above.
8. Add FK-violation (`23503`) error translation at the write sites that
   can theoretically race past their own pre-validation.
9. Run agent-manager's full existing test suite against a real Postgres
   (M6.1's cluster, or local `postgres:16` for iteration) — every current
   test should still pass unmodified in intent, only in backing store.

## Acceptance

- Zero references to `modernc.org/sqlite`, `?` placeholders,
  `boolToInt`, `encodeTime`/`decodeTime` remain in `internal/store/`.
- Full existing store test suite passes against Postgres.
- Concurrent `CreateSession` calls (simulated) never produce duplicate
  `sort_order` values within the same `group_name`/`parent_id`.
- A `spawn` CLI round trip (create session, update status, delete) works
  end-to-end against the M6.1 cluster.

## Verify

**Harness:** `internal/store/*_test.go` running against the disposable
Postgres from M6.2's harness.

**Integration test** — extend/rename the existing store test files:
1. `a1_no_sqlite_references` — `grep -r "modernc.org/sqlite" internal/`
   returns nothing.
2. `a2_no_bare_placeholders` — `grep -rE '\?[,)]' internal/store/store.go`
   inside SQL string literals returns nothing (manual review of any
   `?` that's part of a non-SQL string, e.g. a Go format verb, to avoid a
   false positive).
3. `a3_existing_suite_passes` — `go test ./internal/store/...` green
   against Postgres.
4. `a4_concurrent_sort_order` — spawn N goroutines each calling
   `CreateSession` into the same group concurrently; assert the resulting
   `sort_order` values are a dense 0..N-1 permutation with no duplicates.
5. `a5_fk_violation_translated` — attempt `PlaceSession` with a
   `parentID` deleted between the read and the write (simulate via a
   second connection); assert the returned error is the existing
   descriptive one, not a raw pgx driver error.
6. `a6_spawn_roundtrip` — `spawn` CLI subcommand creates a session,
   `UpdateStatus`, then `Delete`; assert no error and the row is gone.

**Command:** `go test ./internal/... -run TestStore -v`

**False pass:**
- Running the suite against sqlite still (leftover build tag or import)
  while believing it validated Postgres. Assertion 1 is the guard —
  without it, the whole task could report green while the driver swap
  never actually happened.
- Skipping assertion 4. A `sort_order` race is invisible in every
  single-threaded test and only shows up as duplicate/out-of-order
  sessions under real concurrent use, which is exactly the class of bug
  removing `SetMaxOpenConns(1)` introduces.

## Traps

- Converting `?` to `$N` by simple find-and-replace in file order without
  re-checking each statement's actual arg list — a multi-arg statement
  reordered during earlier edits (e.g. `createSession`'s 15-arg INSERT)
  will silently bind the wrong value to the wrong column, and Go's
  `database/sql` won't catch a type-compatible mismatch (e.g. two `TEXT`
  columns swapped).
- Forgetting `pending_inputs` is now `JSONB` (M6.2), not `TEXT` — the
  existing `json.Marshal`/`Unmarshal` round-trip in
  `encodePendingInputs`/`pendingInputState` still works unchanged (pgx
  scans `JSONB` into `[]byte` the same as `TEXT`), but don't add an extra
  marshal layer thinking the column type changed the Go-side contract.

---

Background: [M6.2](M6.2-schema-port.md) · `internal/store/store.go`, `internal/spawn/spawn.go` (agent-manager, `add-headless-spawn` branch)