Skip to content

fix(agentsview): skip schema DDL for compatible pg push#888

Merged
wesm merged 5 commits into
kenn-io:mainfrom
flexiondotorg:fix/pg-push-skip-current-schema-ddl
Jun 27, 2026
Merged

fix(agentsview): skip schema DDL for compatible pg push#888
wesm merged 5 commits into
kenn-io:mainfrom
flexiondotorg:fix/pg-push-skip-current-schema-ddl

Conversation

@flexiondotorg

@flexiondotorg flexiondotorg commented Jun 26, 2026

Copy link
Copy Markdown
Contributor
  • Use read-only schema checks before DDL.
  • Skip schema and index maintenance for compatible PostgreSQL schemas.
  • Fall back to EnsureSchema() when the schema is missing or incompatible.
  • Add unit coverage for the new path.

Closes #887

@flexiondotorg
fix(agentsview): skip schema DDL for compatible pg push
a234f20 
  - Use read-only schema checks before DDL.
  - Skip schema and index maintenance for compatible PostgreSQL schemas.
  - Fall back to EnsureSchema() when the schema is missing or incompatible.
  - Add unit coverage for the new path.

Closes kenn-io#887

Signed-off-by: Martin Wimpress <code@wimpress.io>
@roborev-ci

roborev-ci Bot commented Jun 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (a234f20)

The PR has one medium correctness issue; no high or critical findings were reported.

Medium

  • internal/postgres/sync.go:283 - The compatible-schema fast path returns before running EnsureSchema, but EnsureSchema also performs correctness repairs such as backfillIsAutomatedPG and the token coverage repair. Existing PG schemas with all required columns but stale classifier results or missing token_coverage_repair_v1 metadata will now skip those repairs indefinitely.

    Suggested fix: Split non-DDL data repairs from DDL/index migration and run the required DML repair path after a successful compatibility probe, or make the compatibility probe account for repair metadata and invoke a DML-only migration when needed.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 3m42s), codex_security (codex/security, done, 1m25s) | Total: 5m13s

@leejuhanKr leejuhanKr mentioned this pull request Jun 26, 2026
Closed
@flexiondotorg
fix(agentsview): run data repairs on compatible pg push path
d84b26b 
  The compatible-schema fast path returned before EnsureSchema, so it
  skipped the non-DDL data repairs that keep is_automated and token
  coverage flags correct on existing rows. Run those repairs on the fast
  path while still skipping the index and column DDL that blocks pg serve
  reads (kenn-io#887). The repairs issue only row-level writes and stay gated by
  schemaDone, so they run at most once per push process.

Signed-off-by: Martin Wimpress <code@wimpress.io>
@roborev-ci

roborev-ci Bot commented Jun 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (d84b26b)

Summary verdict: One medium issue needs attention before merge.

Medium

  • internal/postgres/sync.go:283: The compatible-schema fast path can skip EnsureSchema even when push-required tables are missing. CheckSchemaCompat does not probe model_pricing and treats cursor_usage_events as optional, but Push immediately calls syncModelPricing, which queries model_pricing unconditionally. An older PostgreSQL schema that otherwise passes the compatibility probe now skips the DDL that used to create model_pricing, causing push to fail instead of migrating.

    Suggested fix: Use a push-specific compatibility probe that includes push-written tables and columns, at least model_pricing and the cursor usage table, or fall back to full EnsureSchema when those tables are absent.

Panel: ci_default_security | Synthesis: codex, 7s | Members: codex_default (codex/default, done, 3m56s), codex_security (codex/security, done, 48s) | Total: 4m51s

@flexiondotorg
fix(agentsview): require push tables before compatible pg fast path
4724e71 
  The compatible-schema fast path skipped EnsureSchema based on
  CheckSchemaCompat, which never probes model_pricing and treats
  cursor_usage_events as optional. Push queries model_pricing and writes
  cursor_usage_events, so an older schema missing those tables passed the
  probe and then failed the push instead of migrating. Gate the fast path
  on pushSchemaCurrent, which also requires both push-written tables, and
  fall back to EnsureSchema when either is absent.

Signed-off-by: Martin Wimpress <code@wimpress.io>
@roborev-ci

roborev-ci Bot commented Jun 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (4724e71)

Schema fast path has one medium correctness issue; no security findings.

Medium

  • internal/postgres/schema.go:1628 - The compatible-schema fast path only checks that cursor_usage_events exists, but does not verify the unique idx_cursor_usage_events_dedup index that bulkInsertCursorUsageEvents relies on for ON CONFLICT DO NOTHING. If a schema is otherwise compatible but missing that index, repeated pushes can duplicate cursor usage rows and inflate usage/cost analytics.

    Fix: Treat correctness-critical indexes as part of pushSchemaCurrent, at least idx_cursor_usage_events_dedup, or make cursor usage insertion deduplicate explicitly without relying on skipped DDL.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 7m11s), codex_security (codex/security, done, 1m9s) | Total: 8m26s

@flexiondotorg
fix(agentsview): require cursor dedup index for compatible pg fast path
ef5ebe3 
  bulkInsertCursorUsageEvents dedups via a targetless ON CONFLICT DO
  NOTHING, which only suppresses duplicates when the partial unique index
  idx_cursor_usage_events_dedup exists. The compatible-schema fast path
  checked tables but not that index, so a schema missing it would silently
  duplicate cursor usage rows on repeated pushes. Gate pushSchemaCurrent on
  the index too and fall back to EnsureSchema when it is absent.

Signed-off-by: Martin Wimpress <code@wimpress.io>
@roborev-ci

roborev-ci Bot commented Jun 26, 2026

Copy link
Copy Markdown

roborev: Combined Review (ef5ebe3)

Summary verdict: one Medium issue should be addressed before merge.

Medium

  • internal/postgres/schema.go:1485 - CheckSchemaCompat is shared by pg serve read-only startup, but now requires push-only metadata/schema (sync_metadata and owner_marker). A read-only serve role or legacy schema that is otherwise sufficient for read paths can now fail compatibility even though the UI could still serve sessions.

    Suggested fix: Keep CheckSchemaCompat limited to columns/tables required by read paths, and move push-only probes like sync_metadata / owner_marker into pushSchemaCurrent or a separate push compatibility check.

Panel: ci_default_security | Synthesis: codex, 6s | Members: codex_default (codex/default, done, 4m38s), codex_security (codex/security, done, 1m27s) | Total: 6m11s

@flexiondotorg
fix(agentsview): keep push-only schema probes out of read compat check
f89cc66 
  CheckSchemaCompat gates both pg serve read-only startup and the push fast
  path, but it probed sync_metadata and sessions.owner_marker, which only
  push uses. A read-only serve role or legacy schema that was fine for reads
  then failed compatibility at startup. Move those two probes into a new
  checkPushSchemaCompat that pushSchemaCurrent runs, leaving CheckSchemaCompat
  limited to read-path schema

Signed-off-by: Martin Wimpress <code@wimpress.io>
@roborev-ci

roborev-ci Bot commented Jun 27, 2026

Copy link
Copy Markdown

roborev: Combined Review (f89cc66)

No issues found.

Panel: ci_default_security | Synthesis: codex | Members: codex_default (codex/default, done, 3m30s), codex_security (codex/security, done, 2m19s) | Total: 5m49s

@wesm wesm merged commit 746242d into kenn-io:main Jun 27, 2026
12 checks passed
@wesm

wesm commented Jun 27, 2026

Copy link
Copy Markdown
Member

thank you!

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

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

pg push schema maintenance can block pg serve dashboards

2 participants

@flexiondotorg @wesm