Repository navigation
Reconcile additive select field option changes - #482
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
Critical issues remain: the rebuild path can overwrite concurrent select widening, and skipped searchable-flag changes can still drop required FTS indexes, causing search requests to fail.
2 open issues
What changed in this PR
This PR addresses #479 by reconciling safe select-option widening during startup provisioning, so existing databases accept newly declared values without requiring a table rebuild.
Changes:
- Adds metadata-only updates with atomic cache-generation bumps and stale-snapshot checks.
- Adds regressions for persisted upgrades, rollback, mixed changes, and conflicting schema edits.
- Documents widening limits and explicit-migration requirements for other field changes.
| File | Description |
|---|---|
src/scaffold/agents_md.zig |
Updates generated guidance on additive provisioning. |
src/provision.zig |
Classifies field changes, applies select widening, and adds regression tests. |
src/collections.zig |
Adds transactional, snapshot-checked field-option updates. |
docs/framework.md |
Explains widening rules, conflicts, and migration requirements. |
changelog.d/provision-field-options.md |
Records the provisioning fix and new warnings. |
Review effort: Balanced
66250f2 to
37c999b
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Adding the first searchable field with -Dfts5=false bypasses preflight validation, allowing schema changes and index/trigger removals to persist before startup fails with FtsDisabled.
1 open issue
2 resolved since last review
Review effort: Balanced
37c999b to
b5edfcc
Compare
valthon
left a comment
valthon
left a comment
There was a problem hiding this comment.
Automated review: 7 findings, 5 inline. The self-relation false-migration warning (src/provision.zig) is the one to fix before merging. These come from an automated pass. They weren't independently re-checked, and the code wasn't built or tested.
Not inline, because the file isn't in the diff: KNOWN_LIMITATIONS.md (~line 20) needs updating. It still describes auto-migration as purely additive, with only rename, drop and type changes skipped. It doesn't mention that select widening is now applied or that other option/flag changes are skipped. CLAUDE.md requires KNOWN_LIMITATIONS.md to stay in sync with the docs, and the published known-limitations page would now contradict the updated docs/framework.md and the scaffold text.
Generated by Claude Code
b5edfcc to
889055e
Compare
valthon
left a comment
valthon
left a comment
There was a problem hiding this comment.
Re-review of 889055e. Changes required: please resolve all 9 inline findings before merge, including the minor ones. No exceptions for "small" issues; this is the standard for the repo. (This is posted as a Comment only because GitHub doesn't allow "Request changes" on your own PR. Treat it as blocking.)
Earlier review: all 7 findings are addressed. The self-relation comparison, the full-definition snapshot guard, the single atomic provisionMetadata transaction, KNOWN_LIMITATIONS.md, the shared startup_log.refusal helper, and the widened cleanup all look resolved. Thanks.
New findings in this revision:
Correctness (these block the merge):
- Fields whose id starts with
_are still dropped before index validation, which can refuse startup (collections.zig:656, file-level comment because the line is unchanged). - Two servers booting the same release: the second gets
Conflictand fails to start, even though the stored definition already matches what it wanted. last_errorspoints into a freed arena.- A failure after commit (
PRAGMA foreign_keys=ON) reports a saved write as an error. - A reorder of select values is misreported as a widening, with an unnecessary generation bump.
Tests, efficiency, cleanup (also required):
updateFieldOptionsis only used by tests, so the stale-snapshot test doesn't cover the real provisioning path.- Every
update()takes the cross-process metadata lock before validating its input. - An extra full
collections.getfor every relation field on every boot. updateIndexesand thestandalonebranches are now dead code.
Please add a regression test for each correctness fix (1 to 5). These come from an automated review pass. They were not independently re-checked, and the code was not built or tested.
Generated by Claude Code
There was a problem hiding this comment.
Note
Copilot was unable to run its full agentic suite in this review.
Copilot review overview
3 open issues
read_bytes()[log_start:]),… · New
Review effort: Lite
c3b5d5e to
12e185f
Compare
Existing databases kept stale select options after a comptime
declaration grew. Changing { "a", "b" } to { "a", "b", "c" } left writes
of "c" rejected after a restart, because provisioning only added
columns and logged nothing about other field changes.
Startup now applies safe widening as a metadata-only write, preserving
records, live field ids, migration-owned indexes and triggers. A select
may gain values or raise its limit when every live value remains valid,
its flags are unchanged and its scalar or array storage stays the same.
Narrowing and any other option or flag change keep the live definition
and log that an explicit migration is required. Reordering the same
values leaves the stored order and cache generation alone.
Accepted fields, rules, declared indexes, tenant and search objects and
one generation bump now commit together. Provisioning checks the full
stored definition under the metadata lock, so a conflicting concurrent
edit fails with Conflict instead of overwriting newer fields, rules or
OAuth options, while identical concurrent plans converge without
another rebuild or generation bump. Every rebuild input, including
relation targets, is read after taking that lock.
Because the stored definition is now read under the lock, an update
validates caller fields and options before transaction setup and the
canonical stored name afterwards, covering reserved system names and
legacy FTS suffixes. A metadata-only plan applies that check only when
it will write, so an unchanged legacy collection keeps starting. An
incoming rename stays ignored. The owned result is allocated before
commit, so a failed reload rolls the rebuild back.
If foreign keys cannot be restored after commit, the connection is
logged and quarantined while the committed result is still returned.
Search follows the flags actually accepted, so a skipped searchable
change neither creates nor removes FTS objects, and disabled FTS
rejects an accepted searchable addition before any rebuild. Relation
names and ids compare as the same target, and user field ids are
treated as opaque even with an underscore prefix. Reserved field names
are engine-owned only on auth collections, so a stored one on another
collection type stays in the definition, the snapshot comparison and
index validation.
startup_log.refusal shares the warn-under-test spelling between
provisioning and the Postgres connection refusals. The framework guide,
scaffold guidance, known limitations and changelog describe these
boundaries.
Co-Authored-By: Claude Opus 5.5 <[email protected]>
Claude-Session: https://claude.ai/code/session_01HTAHrkfe1L2hfpLeP1CQAS
12e185f to
ba15832
Compare
There was a problem hiding this comment.
🔵 Needs a closer look
Cross-process locking and atomic reconciliation affect critical persistence paths, but validation lacks live concurrent PostgreSQL coverage, warranting human review of transaction and rollback boundaries.
No open issues 🎉
2 resolved since last review
Review effort: Balanced
Existing databases retain stale select options after a comptime declaration grows. Startup now accepts safe widening, so changing
{ "a", "b" }to{ "a", "b", "c" }lets writes usecafter restart without rebuilding records.Widening preserves live field IDs, records, migration-owned indexes and triggers, and scalar or array storage. Narrowing and other option or flag changes keep the live definition and log migration guidance. Reordering the same values preserves their stored order and cache generation.
Provisioning checks the complete stored definition under its metadata lock. Conflicting edits fail with
Conflictwithout overwriting newer fields, rules or OAuth options, and identical concurrent plans converge without another rebuild or generation bump. Accepted fields, rules, declared indexes, tenant and search objects and one generation bump commit together.Caller fields and options are validated before transaction setup. The canonical stored name is validated under the metadata lock, including reserved system names and legacy FTS suffixes, and incoming rename values remain ignored. A metadata-only plan applies that check only when it will write, so an unchanged legacy collection keeps starting. A failure to restore foreign keys after commit logs the failure and quarantines the connection while preserving the committed result.
Search follows the persisted flags that were accepted. Disabled FTS rejects accepted searchable additions before rebuilding. Index validation handles opaque custom field IDs and rejects indexes on encrypted fields.
Reserved field names are engine-owned only on auth collections. A stored reserved-named field on another collection type, which only a migration can create, stays in the definition through select widening and in the snapshot comparison. Adding a new field to such a collection fails validation before any write, rather than rebuilding the table without that column.
The framework guide, scaffold guidance and known limitations describe these boundaries.
Fixes #479.
Validation on Zig 0.16.0, against the tree of this head on
mainat8157414e:zig build test --summary allreturned 0.tests/adminwith-n 4: 366 passed, 154 skipped.check-gating, allocator contracts, disabled agent CLI, error-code ledger, doctor ledger and changelog assembly passed.The
zaptransport re-pin, its changelog fragment and thetests/adminfixture work that were previously on this branch are in #481. The prior head is preserved oncodex-does-too-much-482.🤖 Generated with Claude Code
https://claude.ai/code/session_01HTAHrkfe1L2hfpLeP1CQAS