Skip to content

feat(postgres): implement PG serve session curation and insight persistence#898

Open
rodboev wants to merge 11 commits into
kenn-io:mainfrom
rodboev:pr/pg-serve-writes
Open

feat(postgres): implement PG serve session curation and insight persistence#898
rodboev wants to merge 11 commits into
kenn-io:mainfrom
rodboev:pr/pg-serve-writes

Conversation

@rodboev

@rodboev rodboev commented Jun 27, 2026

Copy link
Copy Markdown
Contributor

PG serve already supports shared stars and pins, but the rest of the dashboard curation surface still behaves as read-only. Session rename, trash, restore, empty trash, insight delete, and generated-insight persistence all route through the existing db.Store seam, yet the PostgreSQL adapter still returns db.ErrReadOnly for those methods, and the insight-generation route stops on a coarse read-only check before it can save anything.

This change fills in the remaining PG dashboard-write slice without widening scope to local-only features or other read-only backends. PostgreSQL gets the missing insight storage plus Store implementations for insight CRUD and session-management methods, and the server now treats generated-insight writes as an explicit capability: pg serve advertises it, the frontend enables the Generate controls in that mode, and plain read-only stores such as duckdb still fail fast before any generator work starts. Settings remain blocked in pg serve through the existing read-only contract, while local serve --no-sync stays writable because it still uses a local Store.

The tests cover the PG round trips for insight CRUD and session management, the read-only capability split on the insight route, the pg serve and local no-sync settings behavior, the pg serve version metadata that enables the frontend, and the two frontend insight entry points. Stars and pins are left untouched, and ingestion-oriented methods such as WriteSessionBatchAtomic remain read-only.

Closes #183

@rodboev rodboev force-pushed the pr/pg-serve-writes branch from 796db71 to 9117d03 Compare June 27, 2026 20:28
@roborev-ci

roborev-ci Bot commented Jun 27, 2026

Copy link
Copy Markdown

roborev: Combined Review (9117d03)

Summary verdict: The PR has two medium correctness risks around PostgreSQL write capability reporting and PG-side session curation persistence.

Medium

  • internal/postgres/store.go:61
    PG stores report insight generation as available unconditionally, but pg serve supports roles where schema migration is skipped for read-only or insufficient privileges. With a schema-compatible read-only role, the UI enables generation and the backend runs the agent, then fails on InsertInsight.
    Fix: Make insight-write capability depend on a startup permission probe or store configuration, and only advertise/allow generation when the PG role can actually insert/delete insight rows.

  • internal/postgres/store.go:281
    PG serve curation writes display_name and deleted_at directly into sessions, but pg push still owns and overwrites those columns from local SQLite on later session pushes. A rename, trash, restore, or delete done in PG serve can be silently undone when the source session changes or a full push runs.
    Fix: Store PG-local session curation separately, or update the push/upsert path to preserve/merge PG-side curation fields intentionally.


Panel: ci_default_security | Synthesis: codex, 10s | Members: codex_default (codex/default, done, 5m32s), codex_security (codex/security, done, 3m42s) | Total: 9m24s

@roborev-ci

roborev-ci Bot commented Jun 27, 2026

Copy link
Copy Markdown

roborev: Combined Review (649f1c3)

Medium findings remain; no Critical or High issues were reported.

Medium

  • internal/postgres/store.go:366 - Permanent deletes in PG serve are not durable across a later pg push --full or any source-side change. DeleteSessionIfTrashed and EmptyTrash physically delete rows, but the push path has no PG-side tombstone/exclusion check, so the next full push can reinsert the same local SQLite sessions.

    • Fix: Add a PG tombstone/excluded-sessions table and have push skip those IDs, or keep a non-visible tombstone row instead of deleting the session row.
  • internal/postgres/store.go:61 - PG serve always advertises insight generation as available, even when the PostgreSQL role is read-only or lacks INSERT privilege on insights. runPGServe already tolerates read-only migration failures, so such deployments can start and let users run an agent only to fail when saving the insight.

    • Fix: Probe/write-check the insights capability at startup or in Store, return false when inserts are not permitted, and set the version capability from that result.

Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 5m5s), codex_security (codex/security, done, 2m23s) | Total: 7m36s

@roborev-ci

roborev-ci Bot commented Jun 27, 2026

Copy link
Copy Markdown

roborev: Combined Review (479d126)

Summary verdict: Two medium-severity correctness issues remain; no high or critical findings were reported.

Medium

  • Location: internal/postgres/push.go:1354

    • Problem: COALESCE(sessions.display_name, EXCLUDED.display_name) and the matching deleted_at logic cannot distinguish PG-serve curation from previously pushed source values. PG-side clears/restores get overwritten by the next source push, while source-side renames/restores can be silently ignored and then fingerprinted as pushed.
    • Fix: Track PG-local curation separately, or compare against the last pushed values so only true PG-serve edits are preserved and deliberate null changes are durable.
  • Location: internal/postgres/store.go:102

    • Problem: The insight write capability probe inserts an explicit id = -1, so it can report generation as available even when the role lacks usage on insights_id_seq, which the real InsertInsight path needs for BIGSERIAL.
    • Fix: Probe with the real insertion shape, such as INSERT INTO insights DEFAULT VALUES RETURNING id, inside the rolled-back transaction.

Panel: ci_default_security | Synthesis: codex, 9s | Members: codex_default (codex/default, done, 5m54s), codex_security (codex/security, done, 3m27s) | Total: 9m30s

@roborev-ci

roborev-ci Bot commented Jun 27, 2026

Copy link
Copy Markdown

roborev: Combined Review (b819522)

Summary verdict: Two medium-severity correctness issues need attention; no security regressions were reported.

Medium

  • Location: internal/postgres/store.go:340
    Problem: The new PG write methods wrap permission/read-only database errors as generic errors, so a pg serve role without UPDATE/DELETE/INSERT privileges now returns 500s instead of the existing db.ErrReadOnly/501 behavior.
    Fix: Add a helper that maps IsReadOnlyError(err) to db.ErrReadOnly and use it for insight and session curation writes.

  • Location: internal/postgres/push.go:1066
    Problem: readPGExcludedSessionIDs builds one PostgreSQL parameter per modified session before push batching happens. Large full pushes can exceed PostgreSQL/pgx parameter limits and fail before any sessions are pushed.
    Fix: Batch the exclusion lookup, similar to UsageEventFingerprints, or use a single array parameter safely supported by the driver.


Panel: ci_default_security | Synthesis: codex, 8s | Members: codex_default (codex/default, done, 8m50s), codex_security (codex/security, done, 3m17s) | Total: 12m15s

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

feat: write support in pg serve

1 participant