fix(security): bind writes to live membership - #133
Merged
KIDA-MNESIA merged 1 commit intoAug 30, 2026
Conversation
KIDA-MNESIA
marked this pull request as ready for review
August 30, 2026 22:41
yetone
added a commit
that referenced
this pull request
Aug 31, 2026
…nd-kept list Production served a v0.11.0 build whose migration had silently not applied. Every BYOA daemon's GET /inbox 500'd on `column delivered.delivery_recipient_id does not exist`; ~900 daemons retried, the database saturated, /api/health 503'd on "health db check timed out", and agent runs fell from ~6000 per 10 minutes to near zero. The migration was in the image and correct. `ALTER TABLE messages ADD COLUMN IF NOT EXISTS delivery_recipient_id` needs a brief AccessExclusiveLock on the hottest table in the schema; under live agent traffic it lost the race and tripped the 5s lock_timeout (55P03). ensureSchema then asked schemaAlreadyCurrent() whether it could skip, that function said yes, and migrate-bin logged "schema ensured" and exited 0 — so the main container started against a schema missing a column the app queries on every request. It said yes because it checked a hand-maintained list of sentinels, and #133 added the column without adding it there. A whitelist cannot catch what nobody remembered to add: the absent column was simply never asked about. The doc comment claimed the opposite — that forgetting was benign and the shortcut "can never skip a migration that hasn't actually been applied" — and that claim is what made the omission look harmless. The same shape had already been caught twice (llm_calls, llm_calls_rollup), each time answered by appending one more entry to the list. So stop keeping the list. `ddlExpectations()` reads every `CREATE TABLE IF NOT EXISTS` and `ALTER TABLE … ADD COLUMN IF NOT EXISTS` out of the DDL text (comments stripped, so a commented-out example cannot become a phantom expectation that disables the shortcut forever), and the check verifies all of them in one query. The few correctness indexes stay explicit because partial/concurrent index DDL is not derivable the same way. A negative result now names what is missing, rather than exiting 0 in silence. Applied to production separately: the migration ran clean in 17s once traffic was off, which is the whole shape of the bug — it only fails under the load that makes the shortcut fire.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
This follows up #115: that PR fixed whole-array lost updates; this PR closes the remaining authorization TOCTOU paths around those membership changes.
Security invariants
Validation
npm run lint(passes; one pre-existing informational finding inserver/src/agents/cli.ts)npm run guard:big-brainnpm run guard:llm-trackednpm run guard:engine-registrynpm run typechecknpm run server:typechecknpm run build