(plan) system review and break down plans
This commit is contained in:
Story Crater Bot
@@ -0,0 +1,243 @@
|
||||
# M6.2 — Postgres schema for agent-manager sessions
|
||||
|
||||
| Field | Value |
|
||||
|---|---|
|
||||
| Phase | M6 — agent-manager migration |
|
||||
| Size | M — 1–3 days |
|
||||
| Status | ⬜ Not started |
|
||||
| Flags | — |
|
||||
| Spec | inlined below |
|
||||
| Blocks | M6.1 |
|
||||
|
||||
## Goal
|
||||
|
||||
The sqlite schema agent-manager's `internal/store/store.go` builds up
|
||||
through 14 `ALTER TABLE` migrations, collapsed into one Postgres-native
|
||||
schema — with the sqlite workarounds (integer booleans, dual-encoded
|
||||
timestamps) removed rather than carried over.
|
||||
|
||||
## Facts (inlined — no spec read needed)
|
||||
|
||||
Current schema, read directly from `internal/store/store.go` (6 tables, no
|
||||
FK constraints anywhere — every relationship is enforced in Go, not SQL):
|
||||
|
||||
```mermaid
|
||||
erDiagram
|
||||
GROUPS ||--o{ SESSIONS : "group_name (app-level, no FK)"
|
||||
SESSIONS ||--o{ SESSIONS : "parent_id (self-ref, app-level, no FK)"
|
||||
SESSIONS ||--o| REVIEW_TARGETS : "session_id (app-level, no FK)"
|
||||
SESSIONS ||--o{ REVIEW_BASES : "session_id (app-level, no FK)"
|
||||
SESSIONS ||--o| REVIEW_SCOPES : "session_id (app-level, no FK)"
|
||||
SETTINGS {
|
||||
text key PK
|
||||
text value
|
||||
}
|
||||
SESSIONS {
|
||||
text id PK
|
||||
text name
|
||||
text tool
|
||||
text cwd
|
||||
text group_name "app-level FK -> GROUPS.name"
|
||||
text status
|
||||
int archived "bool 0/1 in sqlite"
|
||||
int created_at "unix nanos, dual-encoded in sqlite"
|
||||
int last_status_at "unix nanos"
|
||||
text agent_session_id
|
||||
text pending_inputs "JSON array blob"
|
||||
int pending_claimed "bool 0/1"
|
||||
text launch_prompt
|
||||
int sort_order
|
||||
int acked "bool 0/1"
|
||||
text snapshot
|
||||
text worktree_repo
|
||||
text worktree_branch
|
||||
int agent_launched_at "unix nanos"
|
||||
text retired_agent_session_id
|
||||
text parent_id "app-level FK -> SESSIONS.id, self"
|
||||
}
|
||||
GROUPS {
|
||||
text name PK
|
||||
int sort_order
|
||||
text path
|
||||
int archived "bool 0/1"
|
||||
text worktree
|
||||
}
|
||||
REVIEW_TARGETS {
|
||||
text session_id PK
|
||||
text repo_root
|
||||
}
|
||||
REVIEW_BASES {
|
||||
text session_id PK
|
||||
text repo_root PK
|
||||
text base_ref
|
||||
}
|
||||
REVIEW_SCOPES {
|
||||
text session_id PK
|
||||
text scope
|
||||
}
|
||||
```
|
||||
|
||||
Target Postgres DDL:
|
||||
|
||||
```sql
|
||||
CREATE TABLE groups (
|
||||
name TEXT PRIMARY KEY,
|
||||
sort_order INTEGER NOT NULL DEFAULT 0,
|
||||
path TEXT NOT NULL DEFAULT '',
|
||||
archived BOOLEAN NOT NULL DEFAULT FALSE,
|
||||
worktree TEXT NOT NULL DEFAULT ''
|
||||
);
|
||||
|
||||
CREATE TABLE sessions (
|
||||
id TEXT PRIMARY KEY,
|
||||
name TEXT NOT NULL,
|
||||
tool TEXT NOT NULL,
|
||||
cwd TEXT NOT NULL,
|
||||
group_name TEXT NOT NULL REFERENCES groups(name),
|
||||
status TEXT NOT NULL,
|
||||
archived BOOLEAN NOT NULL DEFAULT FALSE,
|
||||
created_at TIMESTAMPTZ NOT NULL,
|
||||
last_status_at TIMESTAMPTZ NOT NULL,
|
||||
agent_session_id TEXT NOT NULL DEFAULT '',
|
||||
pending_inputs JSONB NOT NULL DEFAULT '[]',
|
||||
pending_claimed BOOLEAN NOT NULL DEFAULT FALSE,
|
||||
launch_prompt TEXT NOT NULL DEFAULT '',
|
||||
sort_order INTEGER NOT NULL DEFAULT 0,
|
||||
acked BOOLEAN NOT NULL DEFAULT FALSE,
|
||||
snapshot TEXT NOT NULL DEFAULT '',
|
||||
worktree_repo TEXT NOT NULL DEFAULT '',
|
||||
worktree_branch TEXT NOT NULL DEFAULT '',
|
||||
agent_launched_at TIMESTAMPTZ,
|
||||
retired_agent_session_id TEXT NOT NULL DEFAULT '',
|
||||
parent_id TEXT REFERENCES sessions(id)
|
||||
);
|
||||
|
||||
CREATE TABLE settings (
|
||||
key TEXT PRIMARY KEY,
|
||||
value TEXT NOT NULL
|
||||
);
|
||||
|
||||
CREATE TABLE review_targets (
|
||||
session_id TEXT PRIMARY KEY REFERENCES sessions(id) ON DELETE CASCADE,
|
||||
repo_root TEXT NOT NULL
|
||||
);
|
||||
|
||||
CREATE TABLE review_bases (
|
||||
session_id TEXT NOT NULL REFERENCES sessions(id) ON DELETE CASCADE,
|
||||
repo_root TEXT NOT NULL,
|
||||
base_ref TEXT NOT NULL,
|
||||
PRIMARY KEY (session_id, repo_root)
|
||||
);
|
||||
|
||||
CREATE TABLE review_scopes (
|
||||
session_id TEXT PRIMARY KEY REFERENCES sessions(id) ON DELETE CASCADE,
|
||||
scope TEXT NOT NULL
|
||||
);
|
||||
```
|
||||
|
||||
**Decision: add real FKs, not app-level-only.** Current Go code manually
|
||||
cascades `review_targets`/`review_bases`/`review_scopes` deletes before
|
||||
deleting a `sessions` row (`Delete`, `DeleteChild` in store.go) — `ON
|
||||
DELETE CASCADE` on those three removes that manual bookkeeping entirely.
|
||||
`group_name -> groups(name)` and `parent_id -> sessions(id)` get FKs too,
|
||||
since Postgres makes them free and they catch the exact class of bug
|
||||
`validParent`/`ensureGroup` exist in Go to prevent by hand. This is a
|
||||
behavior change from sqlite (constraint violation now possible on write
|
||||
paths that previously just wrote garbage) — M6.3 must add error handling
|
||||
for FK violations at the ~4 call sites that write `group_name`/
|
||||
`parent_id` without having already validated the reference through
|
||||
`validParent`/`ensureGroup`.
|
||||
|
||||
**Booleans** — `BOOLEAN`, not `INTEGER`. Removes `boolToInt()`/`!= 0` at
|
||||
every read/write site in store.go.
|
||||
|
||||
**Timestamps** — `TIMESTAMPTZ`, not the `encodeTime`/`decodeTime`
|
||||
nanosecond-or-legacy-seconds hack (store.go:1369-1396). That hack exists
|
||||
only because sqlite has no native timestamp type and old rows needed a
|
||||
seconds/nanos disambiguation heuristic (`secondsCeiling`). A fresh
|
||||
Postgres database has no legacy rows — the whole function pair is deleted,
|
||||
not ported. `agent_launched_at` becomes nullable (`NULL` = "never
|
||||
restarted") instead of the sqlite sentinel `0`.
|
||||
|
||||
**`pending_inputs`** — `JSONB`, not `TEXT` holding a JSON string. Same
|
||||
`json.Marshal`/`Unmarshal` round-trip in Go either way, but `JSONB` lets
|
||||
Postgres validate the shape on write instead of accepting malformed JSON
|
||||
that only fails on the next read.
|
||||
|
||||
## Steps
|
||||
|
||||
1. Write `agent-manager` migration files (this repo's `sqlx`-migration
|
||||
convention doesn't apply — agent-manager is a separate Go repo; use
|
||||
whatever migration tool its `add-headless-spawn` branch already has, or
|
||||
a plain `.sql` file run once at `Open()` if it has none — check before
|
||||
introducing a new dependency).
|
||||
2. `CREATE TABLE` in FK-dependency order: `groups`, then `sessions`
|
||||
(references `groups` and itself), then `settings`,
|
||||
`review_targets`/`review_bases`/`review_scopes`.
|
||||
3. No indexes beyond the primary keys are needed yet — `ListSessions`
|
||||
filters on `archived` and orders by `group_name, sort_order,
|
||||
created_at`; add a composite index only if M6.6's gate shows it's slow,
|
||||
not preemptively.
|
||||
4. Confirm `CREATE EXTENSION` is not needed anywhere (plain relational
|
||||
schema, no pgvector) — unlike this project's own `memory-db`.
|
||||
|
||||
## Acceptance
|
||||
|
||||
- Schema applies to a clean `agent-manager-db` database.
|
||||
- `archived`/`acked`/`pending_claimed` are `BOOLEAN`, not `INTEGER`.
|
||||
- `created_at`/`last_status_at`/`agent_launched_at` are `TIMESTAMPTZ`.
|
||||
- Deleting a `sessions` row cascades `review_targets`/`review_bases`/
|
||||
`review_scopes` without the Go code doing it manually.
|
||||
- Inserting a session with an unknown `group_name` is rejected by the FK,
|
||||
not silently written.
|
||||
|
||||
## Verify
|
||||
|
||||
**Harness:** disposable Postgres, same image as M6.1's `Cluster`
|
||||
(`ghcr.io/cloudnative-pg/postgresql:16.2`) — or a local `postgres:16`
|
||||
container for fast iteration, since agent-manager's own test suite doesn't
|
||||
need the real cluster.
|
||||
|
||||
**Integration test** — extend agent-manager's existing store tests
|
||||
(`internal/store/*_test.go` already has `timeenc_test.go`; that file is
|
||||
deleted in M6.3, its coverage folded into this schema's tests) with:
|
||||
1. `a1_migrate_clean` — apply to an empty database, assert all 6 tables
|
||||
exist.
|
||||
2. `a2_bool_columns_are_boolean` — `information_schema.columns` reports
|
||||
`boolean` for `archived`, `acked`, `pending_claimed`.
|
||||
3. `a3_timestamp_columns_are_timestamptz` — same, for `created_at`,
|
||||
`last_status_at`, `agent_launched_at`.
|
||||
4. `a4_review_cascade` — insert a session + a `review_targets` row for it,
|
||||
delete the session, assert `review_targets` is empty without a separate
|
||||
`DELETE FROM review_targets` call.
|
||||
5. `a5_group_fk_rejects_unknown` — insert a session with a `group_name`
|
||||
that has no matching `groups` row, assert it's rejected.
|
||||
6. `a6_parent_fk_self_ref` — insert two sessions where the second's
|
||||
`parent_id` points at the first, assert it succeeds; point it at a
|
||||
nonexistent id, assert it's rejected.
|
||||
|
||||
**Command:** `go test ./internal/store/... -run TestSchema`
|
||||
|
||||
**False pass:**
|
||||
- Testing the schema against sqlite (leftover `modernc.org/sqlite` import)
|
||||
instead of real Postgres. `BOOLEAN`/`TIMESTAMPTZ`/`JSONB`/FK-cascade
|
||||
behavior all differ or are silently accepted-but-ignored by sqlite —
|
||||
every assertion above becomes meaningless against the wrong engine.
|
||||
|
||||
## Traps
|
||||
|
||||
- Keeping `ON DELETE CASCADE` off `group_name`/`parent_id` FKs by
|
||||
accident (only adding it to the `review_*` ones). A group delete or
|
||||
session delete then hits an FK violation instead of the graceful
|
||||
"session has terminals of its own; move them out first" error
|
||||
`PlaceSession` already gives — check M6.3 preserves that message instead
|
||||
of leaking a raw constraint-violation error to the CLI.
|
||||
- Making `parent_id` `NOT NULL DEFAULT ''` (matching the sqlite default)
|
||||
instead of nullable. `'' REFERENCES sessions(id)` is never satisfiable
|
||||
except by a literal empty-string-id row, which doesn't exist — every
|
||||
top-level session's insert then fails the FK. Must be `NULL` for "no
|
||||
parent."
|
||||
|
||||
---
|
||||
|
||||
Background: [M6.1](M6.1-agent-manager-db-manifest.md) · `internal/store/store.go` (agent-manager, `add-headless-spawn` branch)
|
||||
Reference in New Issue
Block a user