fix(osv): drop the expired REMOVE BY field; unblock Rust commits on the ghost gate - #434
Merged
Merged
Conversation
…he ghost gate Two pre-existing blockers, both surfaced by trying to wire a gate that had never been wired. **scripts/check-remove-by.cjs was connected to nothing.** Not package.json, not husky, not CI. It shipped in #421 — mine — to enforce the repo's own "REMOVE BY" contract, and has sat inert since. Running it by hand found one deadline that fell due TODAY: osv::Affected::versions, REMOVE BY 2026-08-15. Removed the field. Verified inert first: no `.versions` read anywhere in src-tauri/src/osv/, and matching runs off `affected_ranges` through check_version_affected, which falls back to "assume affected" when it cannot decide — so an advisory carrying only `versions` still alerts and dropping the field costs no coverage. Serde ignores unknown fields, so the wire format is unchanged. Five constructors set it to None (one production merge path in sync.rs, four fixtures); all removed. **The ghost gate blocked every Rust-touching commit.** #425 broke open a closed gate loop and seeded a 119-entry backlog on 2026-08-14, but the seed predates the post-#421 tree, so 13 commands were left unallowlisted and the gate failed on main for anyone staging Rust. Those 13 are not a #421 regression, and the entries say so. Each was orphaned when its only caller was deleted — but that caller was a component nothing mounted (ConvergenceTab was referenced solely by itself and its own test), so the feature was already unreachable before #421 removed the dead code. The detector simply could not see it until #425 fixed it. Added them to the documented backlog so the gate blocks NEW regressions again, with a reason recording what they actually are — because allowlisting is not the fix here: several back Signal-tier features that /signal still markets (Standing Queries, Cross-Project Intelligence). Those need UI built or the marketing pulled. Tracked, not buried. No always-on invariants workflow: #429 already landed a "Repo guards" job covering file sizes, window spawns and release channel, citing the same 2026-08-14 outage. Duplicating it would be noise. check-remove-by still needs one line in that job — validate.yml is claimed by that lane, so it is theirs. Verified: check-remove-by exits 0; ghost gate 13 new -> 0; cargo test --lib 4300 passed / 0 failed; cargo fmt --check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WJ6BP3GX5HYrnjW1nGtrvC
runyourempire
added a commit
that referenced
this pull request
Aug 15, 2026
… its own maintenance (#462) ## The defect `source_items_fts` is an **external content** FTS5 table (`content='source_items'`). SQLite stores only the inverted index and reads the text back out of `source_items`, so it maintains **nothing** automatically — every write has to be mirrored in by hand. Three paths never were: | Path | What it did | |---|---| | `batch_upsert_pending_source_items` | Wrote `title`/`content` with **no FTS statement at all** — any item whose embedding failed was unsearchable. | | `cleanup_old_items`, `run_maintenance`, `prune_noise`, `trg_source_items_cascade_delete` | Removed rows from `source_items` and left their postings behind. **No FTS `'delete'` existed anywhere in the repository.** | | `upsert_source_item`, `batch_upsert_source_items` | Used `INSERT OR REPLACE` **after** updating `source_items`. On an external-content table the implicit REPLACE-delete reads the old values back from the content table — which already held the NEW text — so it removed the new postings and stranded the old. | The third one was not in the original report and is the dominant cause on the live corpus: it runs on every re-fetch of an item whose text changed. ### Measured on a read-only snapshot of the live 247 MB corpus ``` BEFORE integrity-check rank0=PASS rank1=FAIL(database disk image is malformed) 2,631 divergent terms | 2,148 stale + 2,630 missing vocab rows | 38 items affected AFTER integrity-check rank0=PASS rank1=PASS ``` Rank matters, and it is why this went unnoticed. On an **external-content** table `('integrity-check', 0)` checks only that the index's own b-trees are well formed — it **passes** on the corrupted corpus. Only `('integrity-check', 1)` recomputes the index checksum from `source_items`. `PRAGMA quick_check`, which `Database::new` runs at every startup, also passes. The user-visible symptom, reproduced on real rows: searching a word that had been *edited out* of an item still returns that item. ## The fix **Schema 104** replaces all hand-maintenance with `trg_source_items_fts_{insert,update,delete}` and rebuilds the index once. The three manual FTS statements are deleted. Measured end-to-end on a copy of the live database, the whole `Database::new` path — pre-migration backup copy of 247 MB included — took **5.9 s**. A trigger is the right home for two reasons: no future call site can forget it, and the `'delete'` command needs the pre-update values that only `OLD.*` still has. Two deliberate narrowings on the UPDATE trigger make this **strictly less** write amplification than what it replaces: - `OF title, content` — the scoring drain stamps thousands of `relevance_score` updates per run and now touches the index not at all. - `WHEN OLD.title IS NOT NEW.title OR OLD.content IS NOT NEW.content` — a re-fetch that rewrites identical text (the common case) does no index work, where the unconditional `INSERT OR REPLACE` always did. **Migration, not startup repair.** A guarded startup repair would need `integrity-check` rank 1 — a full tokenizing scan of the content table — on every launch of both the GUI and the engine, and it would grow linearly with the corpus. The migration runs exactly once, inside the existing transactional + backed-up + `migration_history`-recorded framework. `fts_integrity_check()` and `rebuild_fts_index()` are exposed for diagnostics and tests. ## Also in this PR **Defect 2 — headless engine had no DB maintenance.** `run_scheduled_maintenance` now runs at the end of every headless cycle, at the end of a drain, and every 10 drain cycles. Every previous caller lived in the GUI monitoring loop, so a headless-only day did all of the writing and none of the upkeep — `scheduler_state` froze at 2026-08-12 13:54 while `fourda-engine` wrote until 08-13 14:05 with zero checkpoints. The TRUNCATE gate drops 50 MB → 16 MB, a size the engine can actually reach: `wal_autocheckpoint = 1000` at a 4 KiB page churns around 4 MB, so an un-truncated WAL sat between the two numbers indefinitely (25.9 MB when reported, 47.7 MB a day later). **Defect 3 — backup pruning.** The pruner understood only `<db>.backup.vN`, so two unbounded families grew forever: hand-made `.bak-pre-*` snapshots and `.db.corrupt-<unix>` quarantine copies (the corruption-recovery path writes one per incident). It now keeps the newest 2 of **each family independently**, where the retention unit for a hand-made snapshot is the snapshot *plus its `-wal`/`-shm` siblings* — a database restored without its WAL, or a WAL without its database, is worse than neither. (The first cut of this counted the sibling as its own slot; my own `families_are_pruned_independently` test caught it splitting pairs, and there is now a test pinning the pairing rule.) Conservative by construction: matched strictly against this database's own file name, an unparseable suffix is left alone, a file whose mtime cannot be read sorts newest rather than oldest, `4da.db.bakery` is not mistaken for a backup, and the just-written backup is protected. The retention rule is extracted as a pure function with tests — the previous prune bug survived precisely because the rule was only reachable through real `read_dir` output. **Nothing is deleted by this PR**; the pruner is merely made capable. **Defect 4 — partial.** `prune_orphaned_project_dependencies` is restructured into read / stat / write phases so `std::fs::metadata` no longer runs with a transaction open (one dead mapped network drive pinned a WAL snapshot for a full SMB timeout, stalling checkpointing), and its delete phase opens `BEGIN IMMEDIATE` so contention becomes a bounded wait instead of an un-retryable `SQLITE_BUSY_SNAPSHOT` on lock upgrade. The broader `retry_on_busy` + `BEGIN IMMEDIATE` sweep across the codebase is **not** done — see "Left untouched" below. ## Verification **The new tests are real.** Restored the pre-fix `sources.rs` and `migrations.rs`, kept the final set of tests, and re-ran: **8 of 9 fail**, each with the symptom it names. ``` pending_embedding_items_are_indexed_for_search left: 0, right: 1 reupserting_retires_the_terms_... a title term that was edited out must stop matching retention_delete_removes_the_items_postings a deleted item must leave no postings behind batch_upsert_indexes_inserts_and_updates the batch update path must retire replaced terms too every_delete_path_keeps_the_index_consistent prune_noise must not strand postings hybrid_search_bm25_leg_tracks_the_current_text the keyword leg must not still match text the item no longer contains rebuild_repairs_an_index_that_diverged_... FAILED fresh_database_installs_the_fts_triggers left: [], right: [3 triggers] scoring_updates_do_not_disturb_the_index ok (asserts a property that already held) ``` **The real migration was run over the real corpus.** A throwaway in-crate test (not committed) pointed `Database::new` at a copy of the live snapshot, so the whole Rust path ran — sqlite-vec registration, pre-migration backup, schema 103 → 104, the new pruner: ``` MIGRATED 12273 items to schema 104 in 5.87s; source_vec rows=12273 -> integrity-check rank1 PASS RETENTION deleted 12273 real rows -> integrity-check rank1 PASS BACKUPS v101=pruned v102=kept v103=kept bak-pre-v7=kept bak-pre-v8=kept corrupt=kept settings.json.bak-pre-keepme=kept ``` The Python-level verification could not cover this: stock `sqlite3` cannot load `vec0`, so nothing touching `source_vec` was exercised there. Gates: `cargo fmt --check` clean · `cargo clippy -- -D warnings` clean for **both** default and `--features experimental` · `check-file-sizes`, `check-doc-location`, `private-asset-guard`, `ghost-commands`, `compound-quality-check` and `validate-translations` clean. `cargo test --tests` — **4,640 passed / 0 failed / 13 ignored** across all 9 binaries: | binary | passed | |---|---| | `fourda_lib` (lib) | 4,316 (baseline 4,300; +16 new) | | `stack_simulation` | 124 | | `victauri_dogfood` | 157 | | `pipeline_integration` | 13 | | `cli` | 13 | | `migration_tests` | 12 | | `source_resilience` | 5 | | `fourda`, `fourda-engine` | 0 (no tests) | **The operator's live database was never opened for writing.** All measurement was done on snapshots taken with SQLite's online backup API (a plain file copy of a live WAL database tears). ### Two things worth knowing about the machine, not this PR⚠️ **D: hit 0.03 GB free mid-run and killed a link** (`os error 112`) — Defect 3's exact failure mode, arriving on its own. 14 worktrees hold ~230 GB of `src-tauri/target` between them, and `cleanup-orphaned-worktrees.cjs --execute` reclaims none of it (0 dirs removable; it only has 8 merged *branches* to delete). Space was freed and every suite re-run.⚠️ **A stale worktree base looks exactly like a code regression.** This branch was cut from `c1fd348c`; by commit time `origin/main` was 6 commits ahead, and the pre-commit ghost gate failed with *13 new ghost commands in files this PR never touches*. The cause was that `scripts/ghost-command-backlog.json` had gained 13 entries upstream (#434 — "unblock Rust commits on the ghost gate"). Rebasing onto current `main` cleared it. Worth a note in the worktree docs: **re-fetch before committing**, because that failure reads as your fault. ## Left untouched **The `retry_on_busy` helper + `BEGIN IMMEDIATE` sweep (rest of Defect 4).** Applying `BEGIN IMMEDIATE` and a retry wrapper to the codebase's `unchecked_transaction()` call sites is a large, uniform change to locking behavior, and doing it to *some* write paths is worse than doing it to none — mixed deferred/immediate writers can deadlock in ways neither does alone. It wants its own PR with a contention benchmark. The one site fixed here was fixed because it had a specific, independently-diagnosed bug (blocking I/O inside a transaction) rather than as a partial sweep. **`busy_timeout` left at 5000 ms.** Raising it is a one-line change that would reduce `SQLITE_BUSY`, but it trades errors for UI stalls of up to the new timeout, and I have no contention measurement to size it from. Deliberate non-change. **The 262 ad-hoc `Connection::open` call sites** are out of scope as instructed. --- **Nothing on the operator's machine was deleted or mutated.** The migration will run on first launch after merge; it writes `4da.db.backup.v103` first (existing behavior) and the whole step measured 5.9 s on a copy of the current corpus. Note for activation: both binaries need rebuilding before the migration takes effect — `fourda.exe` and `fourda-engine.exe` each run `Database::new`, and whichever starts first performs the migration. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
runyourempire
added a commit
that referenced
this pull request
Aug 15, 2026
…destroys the corpus) (#464) ## Opening a newer database with an older 4DA build destroys the user's corpus Found while rehearsing the schema-104 activation from #462 on a **copy** of the founder's live database. Measured, not inferred. The 2026-08-14 build opened a schema-104 database. The migration guard in `migrations.rs` correctly refused it. `get_database()`'s last-resort fallback then read that refusal as corruption: ``` WARN 4da::db: Database open failed after preemptive recovery — last-resort fallback error=Database schema version 104 is newer than this version of 4DA supports (max 103). INFO 4da::db: Corrupt database preserved, creating fresh database corrupt="…\4da.db.corrupt" INFO 4da::db: Running Phase 1: multi-format files (schema version 1 -> 2) ``` Before / after, same directory: | file | size | schema | source_items | |---|---|---|---| | `4da.db` (what the app now uses) | 1.3 MB | 103 | **0** | | `4da.db.corrupt` (the real corpus) | 283 MB | 104 | **15,659** | The app comes up empty and starts re-fetching from zero. One log line is the only trace. **Every rollback to a previous release does this** — and on this fleet it needs no rollback at all, because the scheduled background refresh runs whatever was last compiled into `target/debug/fourda.exe`. The guard itself has been correct since 2026-03-29. The bug is entirely in how the caller classifies its error. ## The fix **1. A schema-too-new error is routed away from the corrupt-db fallback.** `state.rs` now bails out and returns the error, mirroring the `is_database_lock_contention` bail-out directly above it — that precedent already existed for exactly this shape of problem. The detector keys on **both** `SQLITE_MISMATCH` and a phrase shared with the producer via `SCHEMA_TOO_NEW_PHRASE`, so: - an unrelated `SQLITE_MISMATCH` cannot suppress genuine corruption recovery, and - producer and detector cannot drift apart. Getting this wrong in either direction is expensive: too narrow and the corpus is destroyed; too broad and a genuinely corrupt database never heals. Both directions are tested. **2. Quarantine copies are no longer auto-pruned.** #462 added `*.db.corrupt` / `*.db.corrupt-<unix>` to the backup pruner to reclaim disk. That was my change and it was unsafe: a quarantined database is the user's only copy of that data, and — per the bug above — can be their entire live corpus. Reclaiming 338 MB is not worth a chance of deleting 15,659 items. They stay *classified* so the pruner can report the disk they hold; only `*.db.backup.vN` and hand-made `*.bak-*` snapshots are collected. **3. The guard gets tests.** It had none in ~5 months. A future schema is refused with an error that says why; a database at the current schema still reopens cleanly with a consistent FTS index (so the guard cannot pass by being indiscriminate); and one test asserts **end-to-end that the error the guard actually produces is the one the detector recognises** — testing them apart would let them drift and silently re-arm the corpus-destroying path. ## Also Documents the two skew traps in CLAUDE.md's gotchas. Both cost real time this week and both present as your own bug: - **Old binary vs. newer database** (above) — migrate and rebuild together. - **Stale worktree base** — `main` moved 6 commits during one agent session, after which the pre-commit ghost gate failed citing 13 "NEW" ghost commands in files the branch never touched. They had simply been allowlisted upstream in #434. Re-fetch and rebase before committing; if a gate blames code you did not write, check your base before you touch an allowlist. ## Verification `cargo fmt --check` clean · `cargo clippy -- -D warnings` clean for **both** default and `--features experimental` · `cargo test --lib` **4,378 passed / 0 failed / 8 ignored**. The founder's live database was **not** migrated and **not** written to. All of the above was measured on copies taken with SQLite's online backup API. That decision is the point of this PR: activating #462 before the binaries are rebuilt would have destroyed the corpus on the next scheduled refresh. ## Activation, in the right order With this merged, activation is safe and is a single ordered operation: 1. `git pull` in `D:\4DA` 2. `cd src-tauri && cargo build --bin fourda --bin fourda-engine` 3. launch — the migration runs, rebuilding the FTS index (447 ms on the 15,659-item corpus) Step 2 before step 3 is the whole rule. Doing 3 with a stale step 2 is what this PR makes survivable. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
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.
Two pre-existing blockers, both surfaced by trying to wire a gate that had never been wired. Plus one finding that needs a product decision, not a code fix.
1.
check-remove-by.cjswas connected to nothingIt shipped in #421 — mine — to enforce the repo's own "REMOVE BY" contract. It is absent from
package.json,.husky/, and every workflow. It has been inert since the day it was written.Running it by hand found one deadline that fell due that same day:
osv::Affected::versions,REMOVE BY 2026-08-15.Removed the field, after verifying it is genuinely inert:
.versionsread anywhere insrc-tauri/src/osv/affected_rangesviacheck_version_affected, which falls back to "assume affected" when it cannot decide — so an advisory carrying onlyversionsstill alerts, and dropping the field costs no coverageFive constructors set it to
None(one production merge path insync.rs, four fixtures); all removed.2. The ghost gate blocked every Rust-touching commit
#425 broke open a closed gate loop and seeded a 119-entry backlog on 2026-08-14, but that seed predates the post-#421 tree — so 13 commands were left unallowlisted and the gate failed on
mainfor anyone staging Rust.These are not a #421 regression, and the backlog entries say so. Each was orphaned when its only caller was deleted — but that caller was a component nothing mounted. Verified at
8a170c4b^:ConvergenceTab.tsxwas referenced solely by itself and its own test. The features were already unreachable; #421 removed dead code and the then-broken detector hid the consequence.Added to
scripts/ghost-command-backlog.jsonso the gate blocks NEW regressions again.3.⚠️ Needs a decision: Signal-tier features with no UI
Allowlisting is triage, not the fix. 13 of the dead commands are Signal-tier, and two cards currently on
/signalhave no user interface at all:create_standing_query,list_standing_queries,delete_standing_query,get_standing_query_suggestionsget_cross_project_dependencies,get_tech_convergence,get_project_health_comparisonBriefingView.tsxonly listens for astanding-query-matchesevent to render a toast; nothing can create, list or delete a standing query. The paid page markets features a customer cannot use. Either the UI gets built or the marketing gets pulled — that is a product call, so it is flagged here rather than papered over.Related correction: #421 reported "IPC health 100%, 0 ghosts." That was measured before #425 fixed the detector ("119 dead IPC commands were invisible"). The real figure on
mainis 69.6%. Do not quote the 100%.What this deliberately does NOT do
No always-on invariants workflow. I built one, then found #429 had already landed a
Repo guardsjob covering file sizes, window spawns and release channel — citing the same 2026-08-14 outage. Duplicating it would be noise, so mine was deleted.check-remove-bystill needs one line in thatRepo guardsjob..github/workflows/validate.ymlis claimed by the lane that shipped #429 and they are still ahead ofmain, so it is theirs to add rather than mine to conflict with.Verification
check-remove-byexits 0cargo test --lib: 4300 passed, 0 failedcargo fmt --check: cleantailreturns tail's status and will report a failed build as success.🤖 Generated with Claude Code
https://claude.ai/code/session_01WJ6BP3GX5HYrnjW1nGtrvC