fix(knowledge): make the sync_status column the single source of truth

[chenmingwei23]

Problem / Motivation

A knowledge source's sync state is stored twice: the sources.sync_status
COLUMN and a sync_status key inside the properties JSON blob. Writers and
readers are split across the two stores, so each side can act on a state the
other never wrote.

Two symptoms a user can see today:

• A local file that has been deleted keeps rendering its old badge in the
  Library ("synced"). The watcher marks it missing in the properties blob only,
  and the Library renders the column. The blob key is also never cleared when
  the file comes back, so the two stores stay divergent for the life of the row.
• The watcher's folder pre-scan skip reads the blob copy, so any pause recorded
  in the column alone still walks and delete-reconciles the whole folder every
  sweep. Today that is avoided only because three handlers remember to write
  both stores by hand; the next transition that forgets re-opens the bug.

Restoring a bundle also landed every source at the pending default, because
import_bundle -- the third insert path -- never wrote the column at all.

Why it matters

Sync state is what the Library shows and what the sweep obeys, so a divergence
is both a wrong badge and wrong work: a paused folder that is still walked
spends the scan budget the user paused to stop, and an errored source that only
one reader can see is retried forever. It has already been patched twice
(seeding the column on insert, then reading either store in sync_all), each
time by teaching one more site about both copies. That is the pattern that keeps
the defect alive: every new transition has to remember a rule nothing enforces.

What changed (motivation -> approach -> change)

Symptom: state read from one store, written to the other. Root cause: two
stores, with no seam that makes one of them authoritative. Fix: converge on the
column, and put the convergence at the store's own write seam rather than in a
convention writers must remember.

The blob keeps exactly one role -- the INSERT-time channel a caller uses to
STATE an initial status, held to an allowlist. It is no longer persisted next to
the column, and no row keeps a second answer once the store has opened. After
insert the column is written explicitly or not at all: a status found in a
properties write is DROPPED, not applied. That direction is deliberate -- a blob
read off a legacy row is stale by definition, and applying it would let the
watcher stamp missing back onto a file it had just re-ingested (pinned by a
test).

• store.py: _without_sync_status strips the key on all three insert paths and
  on every update_source, so no caller can mint a second copy.
• store.py: ONE migration pass over the rows that still carry a copy does all
  the convergence -- it absorbs the pre-existing initial-state repair, repairs the
  column where it was never written, and then RETIRES the copy. Every candidate is
  a row whose blob holds a status. Membership is decided on the PARSED blob
  rather than a substring of its text: JSON escapes are legal inside a key, so a
  blob stored as {"sync_\u0073tatus": "paused"} holds the key this pass
  converges without ever containing it literally, and a raw-text filter would
  skip the row -- leaving the column at its default and the watcher walking a
  folder the user had paused. import_bundle used to store a bundle's properties
  verbatim, so such a row can exist. The retirement is what keeps the repair
  honest: the migration runs on every open, so leaving the key would make it a
  standing reader of a value that goes stale the moment a column-only writer moves
  the row.
• store.py: a LIFECYCLE value in the blob is deliberately NOT promoted, not even
  'error'. It cannot be ordered against the column -- a pre-column
  _record_failure wrote 'error' to the blob alone, and a later successful
  re-ingest wrote 'synced' to the column alone, so neither copy carries evidence
  of which happened last. Promoting would mark a RECOVERED source errored, with
  the copy retired in the same pass so nothing could correct it. Not promoting
  costs at most ONE sync attempt: _record_failure reads consecutive_failures
  from the blob, which such a row already carries at or above its threshold, so the
  first attempt that fails writes the column and quiesces the source for good --
  while an attempt that SUCCEEDS is the right outcome for a source that had
  recovered. The column is authoritative; a value that cannot be ordered against
  it does not get to overrule it.
• store.py: import_bundle restores each source's status from the COLUMN that
  export_all ships, falling back to a legacy bundle's blob copy, so a restored
  pause is not silently resumed -- then strips the blob like every other insert
  path, so a status the allowlist REFUSED cannot be read back by the migration
  one reopen later. 'paused' joins the durable initial states, which every
  insert path now reads through one shared allowlist; a bundle is untrusted
  input, so an outcome state like error or syncing still cannot be asserted
  about work that never ran.
• store.py: update_source gains if_sync_status, a compare-and-set on the
  status column, and the migration's repair binds the column it read as well as
  the blob. Every live writer transitions the column WITHOUT touching properties, so
  a blob-only precondition still matched: the repair could stamp the blob's initial
  state over a transition that had just landed, and the sweep could stamp
  'synced' over an 'error' a manual sync recorded while it ran. A caller that derives a
  status from a snapshot passes the value it saw; a caller writing the outcome of
  something that just happened has current information and does not.
• store.py: the migration's JSON parses also catch RecursionError -- a
  RuntimeError, so the ValueError/TypeError guard missed it. json.loads
  recurses per nesting level and this runs on EVERY open, so one pathologically
  nested legacy blob would abort every store construction rather than skipping
  one row.
• watcher.py: the folder pre-scan skip reads the column; a vanished
  local_file is marked missing in the column, and can LEAVE that state when
  the file returns. A returning file's content is READ, not assumed: deletion is
  the event that breaks the mtime heuristic, because a restore preserving the
  archived mtime (cp -p, rsync -t, tar -x) can put different content on disk
  under an mtime that never advanced, so the change gate now also fires for a row
  that reads missing. The clear runs LAST, after the read, and its
  compare-and-set on missing is what decides whether it is the write that takes
  the marker off: an ingestion that ran has already written the column (synced
  when it stored the document, error on a partial write) and this no-ops, while
  the one outcome that writes NO status -- the pre-ingest duplicate gate, which
  refuses the write because a holder already holds this exact document -- is the
  one it clears. A failed ingest raises, so the clear is never reached with the
  file unread. Both writes derive from the sweep's snapshot, so both are
  compare-and-set. Status writes go through update_source
  (one spelling in the file, including the one that was already there).
• watcher.py: every store call in the sweep's own body -- the two source
  queries, the post-ingest re-read, and all four writes -- now runs off the event
  loop. The sweep is a background coroutine and sqlite reads are synchronous, so
  a contended database could hold one for as long as busy_timeout and stall
  every other task; KnowledgeStore keeps a connection per thread, so a worker
  gets its own. Scope boundary, stated rather than implied: the re-embed job the
  sweep delegates to at the end still makes synchronous store calls. Those are
  pre-existing, belong to the embedding-job machinery rather than to source
  state, and are untouched here. .github/sync-io-in-async-baseline.txt records
  the shrink the repo's own gate asks for: knowledge/sync.py 1 -> 0 and
  knowledge/watcher.py 6 -> 2.
• watcher.py: the sweep's own error handler called .get() on a sqlite3.Row,
  which has no such method -- so any failure in the single-file loop raised
  AttributeError from inside the handler, replacing the real error and
  abandoning every remaining source for that sweep. Found by the test for the
  failed-re-ingest case above.
• sync.py: _record_failure writes the column only; sync_all reads it only,
  off the event loop.
• handlers/knowledge.py: confirm / pause / resume stop double-writing the blob.

No frontend change: the Library already reads the top-level sync_status field,
which is the column.

Tests

New test/test_knowledge_sync_status_column.py -- the watcher contract:

• a paused folder is not walked, with the state in the column and no blob copy
• an unconfirmed folder is not walked; an active one still is
• a vanished local_file is marked missing in the column, and writes no
  second copy
• a returning file leaves missing; a present file at another status is
  untouched; a returning file whose re-ingest FAILS stays missing rather than
  reporting content it never ingested; and the recovery write loses to a status
  that moved mid-sweep
• a file restored under an UNADVANCED mtime is READ rather than assumed -- the
  case where the mtime gate alone would have reported freshness about a file it
  never opened
• an ingestion that writes no status (the duplicate gate) still gets the marker
  cleared, so a present, accounted-for file does not go on reading missing;
  and the clear loses to a status the ingestion itself wrote

test/test_knowledge.py -- the store contract, including a JSON-escaped status
key that converges and is retired: insert stores no second copy and
does not mutate the caller's dict; an outcome state in properties still never
seeds the column, while paused is accepted; update_source drops a blob-borne
status and honours if_sync_status; a stale blob cannot move the column; the
migration retires the copy without promoting a lifecycle value, does not
re-error a source that has since synced (whether it recovered before or after the
first open), loses its repair to a concurrent column write, and survives a blob
too deeply nested to parse; an export/import round trip restores
paused and pending_confirmation while refusing error; a legacy blob-only
bundle still restores; and a refused bundle status does not come back at the next
open.

Rewritten rather than deleted, so the behaviour they guarded is still guarded:
the sync_all legacy-error test now builds a real pre-column row and reopens the
store, so it proves the honest contract: the row is attempted once, and the first
failure writes the column and quiesces it for good; the
mid-pass race test still proves a concurrent properties write beats the snapshot,
and now also pins the self-heal -- every write for the raced row is refused, so
nothing is lost, and the next open converges it; the
pause / project-docs / autosource assertions read the column; the bundle
JSON-well-formedness test keeps its subject on a non-status key.

Every new or changed case was checked RED against the code it fixes -- 11 of 15
against unmodified src/ in round 1, and every round-2 and round-3 case against
the revision it corrects. 558 tests pass across the affected files; flake8 and
isort clean on every touched file.

Manual verification

N/A -- unit coverage sufficient. Both symptoms are store/watcher state
transitions with no UI of their own, and each is now driven end to end through
the real KnowledgeWatcher._scan() and a real reopened KnowledgeStore,
including the concurrent-writer windows.

Related Issues

No GitHub issue: this comes from the local review backlog as f-20260815-02
(recorded 2026-08-15, held for a ruling on which store should win; the ruling is
the column, which is what both prior patches had been converging on anyway).

Pattern harvest

Rule candidate: review-prompt / agents-md.
Pattern: "one fact persisted in two places, converged by asking every writer to
update both". The tell is a comment that explains why a write is duplicated
("keep the JSON copy in sync with the column") or a read that ORs two stores
("errored in EITHER store") -- both appeared here, added by earlier fixes to
this same defect. Each such patch narrows the window instead of closing it,
because correctness then rests on every future writer remembering. The
generalizable move is the one taken here: pick the authoritative store, and
enforce it at the single write seam so the other copy cannot be created, rather
than teaching one more site to write both.

Three narrower rules this PR earned the hard way, all worth a review prompt:

• When a change EMPTIES a storage channel, every READER of that channel has to
  move in the same commit. Revision 1 stripped the blob but left import_bundle
  reading it, which would have resumed a paused folder on every restore.
  Mechanical check: after removing a key from a persisted structure, grep for
  remaining readers of that key.
• A retirement migration that RUNS ON EVERY OPEN and leaves the retired copy in
  place is not a migration, it is a permanent second reader. Revision 2 had a
  lift that would re-apply a stale error over a fresh synced at every start.
  Mechanical check: a repair pass that reads a field it does not also clear will
  re-apply it forever.
• A compare-and-set must bind every field a competing writer can move, not just
  the one the reader happened to look at. Revision 2's lift bound the blob only
  -- and after this change every live writer moves the COLUMN and leaves the blob
  alone, so the precondition matched exactly when it should not have. Mechanical
  check: when converging on one field, audit each CAS whose precondition names a
  different field from the one being written.

Revision note (c48c2c1e4). An earlier revision of this PR DID promote a blob
'error' onto the column. Opus showed that a source which recovered before the
first upgrade open would be re-marked errored and then have its copy retired, so
nothing could correct it -- the third finding against that one mechanism across
three rounds. Adding a fourth guard was the wrong answer: the promotion is not
load-bearing, because the scheduler's own failure count re-quiesces a still-failing
row on its first failed attempt. It is gone, and the pattern harvest below records
that.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Design-level review of 9c1fd2014aea49709ebfd36fdda593aef49b6923 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All source hunks read and cross-checked against the description; the frontend badge path renders the column value generically, so missing landing in the column needs no frontend change, as claimed. The design gate raises nothing actionable: the fix targets the root cause (two stores with no authoritative seam) rather than the symptom, enforces convergence at the store's single write seam, handles the migration's ordering ambiguity and concurrency windows with explicit reasoning, and the destructive retirement is bounded and argued. Here is the review:

Design-Verdict: PASS

Root-cause fix at the correct seam — the store's own write path — with the migration's irreversibility explicitly bounded and every concurrency window compare-and-set.

[DESIGN-REVIEWED] 9c1fd20

[github-actions]

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 9c1fd2014aea49709ebfd36fdda593aef49b6923 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All verification complete. export_all ships the column (store.py:1668), the frontend reads only the API's column field, and the grep for sync_status across src/ shows zero remaining post-insert blob readers or writers — the four remaining properties={"sync_status": ...} sites are all the deliberately-kept insert-time channel. Consumer counts for the new surface check out. Final review:

First-Principles-Verdict: PASS

Two stores for one state was the cause of both reported symptoms; this deletes the second store at the write seam, with 0 blob readers/writers left behind.

What this change ships

Intent: stop a source's sync state diverging (stale "synced" badge on a deleted file, paused folders still walked) by making the sync_status column the only store — a FIX.

1. Deleted local file's badge now reads "missing" — justified
2. Pausing a folder actually stops the sweep walking it — justified
3. A returning file leaves "missing"; content is re-read, not mtime-assumed — justified
4. Bundle restore keeps each source's status instead of resetting to pending — justified
5. paused becomes a legal starting state — justified (consumer: the restore path)
6. update_source gains if_sync_status compare-and-set — justified (2 consumers: watcher.py:325, 385)
7. Every-open migration repairs then retires blob copies — justified (cause-level)
8. Legacy blob-only errored sources are polled until one real failure — justified (copies unorderable; cost bounded to one attempt)
9. Sweep and scheduler store calls hop off the event loop — rides along, but forced by the check_sync_io_in_async gate the fix's new calls would trip
10. Sweep exception logging no longer aborts the loop (sqlite3.Row.get) — rides along, in the loop the fix rewrites

Sibling count: grepped sync_status across src/ — zero post-insert blob readers or writers remain; the four surviving properties={"sync_status": ...} sites all feed the insert-time channel the design keeps. No duplicate mechanism: the migration replaces the prior narrower repair rather than adding beside it. Framing matches the diff throughout.

[FIRST-PRINCIPLES-REVIEWED] 9c1fd20

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

Reviewed 9c1fd2014aea49709ebfd36fdda593aef49b6923 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 9c1fd20

Verdict parsed from the review's SHA-scoped output markers for commit 9c1fd2014aea49709ebfd36fdda593aef49b6923.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 9c1fd2014aea49709ebfd36fdda593aef49b6923: <one-sentence reason>

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 9c1fd2014aea49709ebfd36fdda593aef49b6923 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 9c1fd20

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 9c1fd2014aea49709ebfd36fdda593aef49b6923: <one-sentence reason>

[chenmingwei23]

Both advisory reviews landed the same two findings against 028faafe5. Both were real and both were mine; fixed in 491154b9b.

1. Export/import round trip lost the status for post-fix bundles -- FIXED (491154b9b).

The reviewers are right, and the finding is sharper than the description I shipped: export_all serializes SELECT * FROM sources, so the state travels in the COLUMN, while my import_bundle seeded it from the blob that the same PR strips on every insert and update. Every bundle exported from a fixed store would have restored at pending, so a paused folder would have been walked again -- the exact outcome the PR body claimed it prevented. import_bundle now reads the bundle's sync_status and falls back to the blob only for a bundle written before the column travelled. Both channels go through one shared allowlist (_initial_status_or_default), so a bundle still cannot assert an outcome state about work that never ran on this machine.

Pinned by test_export_import_restores_a_paused_source (round-trips paused and pending_confirmation, and refuses to restore error) and test_export_import_restores_a_legacy_bundle_from_the_blob. The first was confirmed RED against 028faafe5 -- it reproduces exactly what you described.

This also answers the First Principles note that paused as an initial state had no live consumer: restoring a bundle is now that consumer, rather than legacy blobs only.

2. The watcher bypassed the seam the PR declares canonical -- FIXED (491154b9b).

Also correct, and it undercut the PR's own thesis. I had matched the raw UPDATE sources SET sync_status = ... spelling of the line three lines below mine, which was the wrong consistency to optimise for in a change whose point is that one seam owns this column. All three status writes in watcher.py -- the two I added plus the pre-existing FileTooLargeError one -- now go through store.update_source(...), so the file has one spelling and each transition gets the updated_at stamp and any future invariant added at the seam.

Scope note so the claim is not overstated: the raw column writes in ingestion.py, dashboard/handlers/knowledge.py and the auto-research app are unchanged. They are correct today (they write the authoritative store) and converting those call sites repo-wide is an independent refactor, not this fix. What this PR makes true is that the column is the only STORE; it does not yet make update_source the only WRITER.

The PR body was updated before the push, so the description the lanes read matches the diff. It also carries a second pattern-harvest rule this exchange earned: when a change empties a storage channel, every reader of that channel has to move in the same commit -- after removing a key from a persisted structure, grep for remaining readers of that key.

[chenmingwei23]

Round 2 dispositions against 491154b9b. Three lanes raised four findings; all four were real and all four are fixed in 5da4a431a. No overrides used.

1. GPT (BLOCKING) + First Principles + Design: the every-open migration is itself a reader of the blob copy -- FIXED (5da4a431a).

All three lanes converged on this from different directions and they were all right, including against my own comment from round 1 that called the leftover blob key "inert (no reader consults it)". The migration's error-lift IS a reader, and it runs on every open, so:

• Design's path: ingestion's success writers are column-only (ingestion.py:748, :921, and update_source(last_synced=...) with no properties), so a legacy blob-error row that later syncs keeps its copy, and the next start stamps error back over the fresh synced. That is exactly the stale-blob-wins direction update_source refuses.
• FP's and GPT's path: import_bundle stored the blob verbatim, so a bundle error the allowlist had just REFUSED (my own test pinned restored[errored] == "pending") would be applied one reopen later, defeating the boundary.

Fixed at both ends. import_bundle now passes the blob through _without_sync_status after deriving the restored column value, so all three insert paths behave alike. And the migration now RETIRES the copy in a third compare-and-set pass, so the lift and the pending-repair are a one-time repair rather than standing readers: after one open, no row carries a second answer and nothing can be re-applied.

Pinned by test_migration_does_not_re_error_a_source_that_has_since_synced (Design's exact sequence: legacy blob-error, reopen, column-only successful sync, reopen again -- must stay synced), test_import_does_not_leave_a_refused_status_for_the_migration (FP's and GPT's path), and an added assertion in test_migration_lifts_a_legacy_json_only_error_onto_the_column that every row's blob is retired while the rest of it survives. All confirmed RED against 491154b9b.

2. GPT (BLOCKING): the recovery write marked a failed ingestion as synced -- FIXED (5da4a431a).

Correct. I had cleared missing immediately after the existence check, before the file was read, so a returning file whose re-ingest then failed was left claiming synced for content the store never had. The clear now runs LAST in the loop body and only when no re-ingest was needed -- a changed file's status is left to the pipeline, which writes it on both outcomes. A failed re-ingest leaves the row missing, which is honest (something is wrong and the next sweep retries, since mtime/hash are not persisted) rather than a false claim.

Pinned by test_a_failed_reingest_is_not_recorded_as_synced.

That test then surfaced a latent bug on main, in the same loop's own error handler: logger.exception(..., row.get("uri", row["id"])) calls .get() on a sqlite3.Row, which has no such method. Any failure in the single-file loop therefore raised AttributeError from inside the handler, replacing the real error and escaping the loop, so every remaining source was abandoned for that sweep. Fixed to row["uri"] or row["id"] -- one line, and the honest behaviour GPT's finding assumed ("outer handler preserves ...") did not actually exist until now.

3. GPT (BLOCKING): synchronous SQLite write on the event loop -- FIXED (5da4a431a).

Accepted and applied, after checking the remedy was safe rather than taking it on faith: KnowledgeStore.db is a thread-local property that opens a connection per thread, and knowledge/artifact_ingest.py already calls store methods through asyncio.to_thread, so offloading is the established and correct pattern here. All status writes in the single-file loop plus the sibling properties write now go through await asyncio.to_thread(self.store.update_source, ...).

Scope note, so nothing is overclaimed: the two SELECTs at the top of _scan still run inline, as do the raw column writes elsewhere (FP counted 5 in ingestion.py, 11 in dashboard/handlers/knowledge.py). Those are correct today -- they write the authoritative store -- and making the whole sweep non-blocking, or routing every writer through update_source, is an independent change. This PR makes the column the only STORE; it does not claim update_source is the only WRITER.

4. First Principles, accepted-and-deferred: 16 raw column writes remain elsewhere.

Agreed with your own disposition on it. They write the authoritative store, so there is no divergence and nothing to fix for this defect; converting them is a mechanical refactor that belongs on its own.

The PR body was updated before this push, so the description the lanes read matches the diff, and its pattern-harvest section now carries the rule this round earned: a repair pass that runs on every open and reads a field it does not also clear is not a migration, it is a permanent second reader.

[chenmingwei23]

Round 3 dispositions against 5da4a431a. Design Review PASSED and Opus reported no blocking findings on that head. GPT raised four BLOCKING items and First Principles two subtractions; three of GPT's are fixed in 979b67ccd, and two items are rebutted with reasons below. No overrides used.

GPT (BLOCKING) store.py:609 -- the lift could overwrite a concurrent successful sync -- FIXED (979b67ccd).

Correct, and it is the sharpest finding of the round. My COALESCE(sync_status,'') != 'error' guard was not a compare-and-set: it tested a predicate, not the value I had observed. Since every live writer now transitions the COLUMN and leaves properties untouched, a sync succeeding inside the window left properties = ? still matching, so the lift stamped 'error' over a fresh 'synced'. The lift now binds the column it read (AND sync_status IS ?), which is the same both-copies discipline the pre-existing 'pending' repair already uses.

Pinned by test_migration_lift_loses_to_a_concurrent_column_write. Worth noting how the first attempt at that test was wrong: it fired the concurrent write on the first parse of the blob, which happens during the earlier 'pending' repair pass, so the lift's own fresh SELECT saw the post-race value and the test passed for the wrong reason. It now fires on the second parse, inside the lift pass, and is RED against 5da4a431a.

GPT (BLOCKING) watcher.py:355 -- stale recovery could overwrite a failed manual sync -- FIXED (979b67ccd).

Correct, same class one layer up. 'missing' came from the snapshot taken at the top of the sweep, so a manual sync that failed while the sweep ran had written 'error' by the time the recovery write landed. update_source now takes if_sync_status, a compare-and-set on the status column, and both snapshot-derived watcher writes pass the value they observed.

Where the guard is applied is a deliberate line rather than a blanket: a write whose value comes from an earlier SNAPSHOT is guarded, because it can be wrong by the time it lands; a write recording the outcome of something that just happened in this sweep (the FileTooLargeError branch) carries current information and is not. That is stated on the method.

Pinned by test_update_source_compare_and_set_refuses_a_moved_row and test_recovery_does_not_overwrite_a_status_that_moved, both RED against 5da4a431a.

GPT (BLOCKING) store.py:112 -- deep legacy JSON could abort every store open -- FIXED (979b67ccd).

Real, and cheap, so accepted without argument even though it needs an extreme blob. RecursionError is a RuntimeError, so except (ValueError, TypeError) did not catch it, and this code runs on every store construction -- an unparsable row would mean a gateway that cannot start rather than one skipped row. Measured the actual threshold before writing the test: 5,000 nesting levels still parse, 50,000 raise. Added to the new pass, to _without_sync_status, and to the pre-existing 'pending' repair -- that one runs first, so fixing only mine would have left the hazard exactly where it was.

Pinned by test_migration_survives_a_pathologically_nested_blob, which asserts the deep row is skipped while its healthy neighbour is still repaired.

GPT (BLOCKING) store.py:598 -- "every-open migration scans block gateway startup" -- REBUTTED.

The prescribed fix is "revert the added every-open migration scans", which would reinstate the standing-reader defect that Design Review and First Principles both required fixing last round, so it needs evidence rather than compliance. Three measurements:

1. Scale. The scan is SELECT id, properties, sync_status FROM sources WHERE properties LIKE '%sync_status%'. sources holds one row per knowledge source -- tens, not the items table's thousands. The finding's premise ("large source table") is not the shape this table has.
2. Relative cost. _migrate already performs strictly heavier work on every open in the same function: the orphan cleanup at store.py:708-723 builds a seven-way NOT IN anti-join across items, ingestion_jobs, folder_file_state, artifact_item_state, agent_item_state and source_locations, then issues three DELETEs through it. An O(#sources) scan next to that is a rounding error.
3. It self-extinguishes. The pass retires the key it matches on, so after one open the predicate matches zero rows.

On the anchor itself: KnowledgeStore is built by a synchronous property (dashboard/state.py:5377), so all of _migrate is synchronous wherever it is first touched -- including every pre-existing step. This change does not introduce that property, and making store construction async is a different piece of work from a two-store convergence fix. Happy to be overruled with a measurement on a real database, but I do not think reverting the retirement is the right trade against a defect two other lanes called blocking.

First Principles subtraction: drop the four asyncio.to_thread wraps -- REBUTTED (lane conflict).

This is the direct opposite of GPT's round-2 BLOCKING finding on the same lines, which cited the repo's own no-blocking-call-on-event-loop anchor. I cannot satisfy both, and the blocking lane citing a codified project rule wins over "no harm is reported in this PR". Your substantive point stands though: the offload is partial, since the two SELECTs and get_source_by_uri in the same coroutine are still inline. I have deliberately NOT extended it to those -- completing it would turn a two-store convergence into an event-loop refactor of _scan, which is the scope objection this same review would be right to raise next. The wraps cover the writes the change actually adds or moves; the reads are untouched, pre-existing, and out of scope.

First Principles subtraction: fold the legacy-error lift into the retire loop -- DONE (979b67ccd).

Good catch, and it made the code better in a second way: the two passes shared the properties LIKE '%sync_status%' predicate, so folding them into one SELECT and one loop is what surfaced that the lift needed the column bound as well (the finding above). It is now a single pass that lifts and then retires each row, with both writes compare-and-set on the row as read.

First Principles, previously accepted-and-deferred: 16 raw column writes remain in ingestion.py and the handlers. Unchanged and still agreed -- they write the authoritative store, so there is no divergence to fix here.

[chenmingwei23]

Round 4, now at acbf6c970. Design Review and First Principles both PASSED on 979b67ccd, and Opus reported no blocking findings on it. Two things went into this push.

First Principles subtraction: fold the pre-existing 'pending' repair loop into the lift-and-retire loop -- DONE (acbf6c970).

Taken, and it is a better shrink than it first looks. Every candidate for either kind of convergence is a row whose blob holds a status, so one SELECT covers both: the initial-state repair (blob status onto a column still at its 'pending' default, under the allowlist) and the 'error' lift (from any column) are now two branches in one loop, followed by the retire. Semantics are unchanged per row -- a row at 'pending' with a blob 'error' still takes the lift branch rather than the repair branch, because 'error' is deliberately not an initial state.

The side effect is worth naming because it settles the last open argument on this PR: the store now performs ONE open-time scan of sources where main performs one. My earlier rebuttal of GPT's "every-open migration scans" finding argued the added scan was cheap; after this fold there is no added scan at all, so the premise is gone rather than merely outweighed.

It also changed a test outcome, in a direction that pins something real. test_migration_skips_a_row_whose_properties_moved_mid_repair used to end with the raced row lifted to 'error', because the second pass did a fresh SELECT that saw the settled state. With one pass, every write for that row is refused by its compare-and-set, so the row is left exactly as the concurrent writer left it. The test now asserts that: the stale snapshot is never applied, the refused retire leaves the copy intact so nothing is lost, and a second open converges it to 'error' with the copy retired. That is the self-heal the code comments claim, now actually exercised.

CI red: Backend Lint & Type Check (3.10) -- FIXED (acbf6c970).

black --target-version py310 was not satisfied on the new test file (the gate checks new files). Reformatted with black 26.5.1 and verified --check clean, flake8 clean, and the file's tests still pass. Formatting only, no assertions changed. The 3.12 sibling showed cancelled rather than a second failure -- matrix fail-fast, not an independent problem.

GPT's run against 979b67ccd was still in flight when this went out, and that head had a red lint lane anyway, so its verdict there is superseded; it will re-roll on acbf6c970. Everything from round 3 stands as dispositioned above -- three findings fixed, the to_thread lane conflict and the every-open-scan finding rebutted, and the latter's premise now removed outright by the fold.

558 tests pass across the affected files; black, flake8 and isort clean on every touched file.

[chenmingwei23]

Round 5, now at 560820587. Design Review PASSED on acbf6c970. First Principles came back to CONCERNS on the one item it has now raised twice, so it deserves a resolution rather than the same rebuttal again.

First Principles: the asyncio.to_thread wrappers ride along with three synchronous siblings on the same path -- RESOLVED by completing it (560820587).

Your dichotomy is the right frame: either loop-blocking matters, in which case three unfixed siblings are the problem, or it does not, in which case the wrappers are surface. I cannot resolve it by deleting them -- that is precisely the line GPT blocked on in round 2, citing the repo's own no-blocking-call-on-event-loop anchor, and an advisory subtraction does not outrank a codified project rule enforced as blocking. So I have resolved it on the other branch: the siblings are gone.

watcher.py:195 and :288 (the folder and local_file queries) now go through a small _store_rows helper that runs the query in a worker thread, and :345 (get_source_by_uri after ingest) is wrapped directly. Every store call in _scan's own body -- two reads, one re-read, four writes -- is now off the loop, so the count you grepped is 7 wrapped, 0 synchronous.

The named harm, since you asked for one rather than a caveat: the sweep is a background coroutine on the gateway's loop, KnowledgeStore._connect sets busy_timeout=10000, and sqlite reads are synchronous -- so a single contended read can hold the loop for up to ten seconds while every other task waits. The thread-local connection is what makes the offload safe, not what makes it necessary; that was a fair criticism of how I worded it.

Scope boundary, stated rather than implied, because your grep stopped at _scan's body and mine did not: _scan ends by awaiting _maybe_reembed_stale(), and the re-embed job under it (_run_reembed_job, _finalize_reembed_job) still makes synchronous store calls. Those are pre-existing, belong to the embedding-job machinery rather than to source state, and are untouched here. So the honest claim is "the source sweep's own store access is off the loop", not "the sweep is non-blocking", and the PR body now says exactly that.

606 tests pass across the affected files; flake8, black and isort clean.

On GPT's lane: its run against 979b67ccd came back "review incomplete" -- no verdict produced, which is a transient model error rather than a finding, and it was pinned to a superseded head in any case. It re-rolls on 560820587. Its round-3 findings remain dispositioned above: three fixed, the every-open-scan one rebutted and its premise since removed outright by folding the migration into a single pass.

[chenmingwei23]

Round 7 disposition against 8a0f2c9a1. Design Review PASS, First Principles PASS, Opus no blocking findings. GPT raised one BLOCKING item; it is real and fixed in 1e78cb2e4.

GPT (BLOCKING) store.py:594 -- escaped JSON keys bypass the convergence -- FIXED (1e78cb2e4).

Correct, and I verified the premise rather than taking it on faith. JSON permits escapes inside a KEY, so a blob stored as {"sync_\u0073tatus": "paused"} parses to sync_status while never containing that substring, and properties LIKE '%sync_status%' skips the row. The consequence is the one you named: the column stays at its 'pending' default, and since the watcher now reads the column, it walks a folder the user had paused.

Reachability checked, because it decides whether this is a real row or a thought experiment: _validated_json_column (store.py:35) returns the input TEXT, not a re-serialization, so the pre-change import_bundle stored a bundle's properties verbatim -- an imported row really can hold the escaped form. Nothing in-tree can write it any more (json.dumps never escapes ASCII, and _without_sync_status re-serializes on every insert and update), so this is strictly a legacy-row case, but legacy rows are exactly what this migration exists for.

Fixed by taking your remedy rather than patching the filter: the pass no longer prefilters on blob text at all and decides membership from the parsed value. There is no substring that can cover the escaped forms -- every character in the key can be escaped independently -- so the only correct predicate is the decoded one. The prefilter was an optimization, and a cheap one to drop: sources holds one row per knowledge source, and after this pass no row carries a copy, so later opens parse and skip.

Pinned by test_migration_converges_a_json_escaped_status_key, which asserts the fixture really lacks the literal substring while parsing to the key, then that the row converges to 'paused' and the copy is retired and re-serialized so the escape cannot return. Confirmed RED against 8a0f2c9a1.

Everything else re-verified on the new head: 559 tests pass across the affected suites, and flake8, black, isort and scripts/check_sync_io_in_async.py are clean. The PR body was updated before this push, so the description and the diff agree.

[chenmingwei23]

Round 8 disposition -- head 2c6dbc687.

GPT 5.6, BLOCKING, watcher.py:367 -- a returning file could be marked synced without its content being read. FIXED.

The finding is real and I confirmed it reachable before fixing it. The clear ran behind the mtime gate, and deletion is precisely the event that invalidates that gate: a restore preserving the archived mtime (cp -p, rsync -t, tar -x, or a plain copy out of an older backup) puts different content on disk under an mtime that never advanced. The gate then reported "unchanged" about a file it had not read, and the recovery write stamped synced over an index holding the pre-deletion copy -- which would keep answering searches.

Fixed by removing the special case rather than adding a second hash site: the change gate now fires for mtime > stored_mtime or a row that reads missing, so a returning file is always read. When the hash differs it re-ingests and the pipeline owns the column as before; when it matches, synced is an evidence-backed claim instead of an assumption. This also records the restored file's real mtime, so a later edit under a lowered mtime is still detected.

Two existing tests in this file had seeded a mismatched content_hash while their docstrings claimed an unchanged file -- that premise was wrong, and the fix exposed it. Both now seed the file's real digest, which is what "unchanged" was supposed to mean.

New test test_a_restore_under_an_unadvanced_mtime_is_not_called_synced pins the case. Mutation-verified: against 1e78cb2e4 it fails with ingest_file awaited 0 times and the row stamped synced; with the fix the file is re-ingested and the recovery write stays out.

Gates on 2c6dbc687: 317 tests pass across the five knowledge/handler suites (-n 4), flake8 clean, black --check --target-version py310 clean, isort clean, scripts/check_sync_io_in_async.py passes.

No override used. Design Review, First Principles and Opus 4.8 all passed on 1e78cb2e4 with no open concerns.

[chenmingwei23]

Round 9 disposition -- head 9c1fd2014. All four findings legitimate, all four fixed. No override used.

BLOCKING, watcher.py:363 -- a duplicate re-ingest left a restored file marked missing. FIXED.

Verified by enumerating every exit of ingest_file. _skip_as_duplicate is reachable with source_id set (it is called unconditionally in the gate hop, and exclude_source_id=source_id means a hit under a DIFFERENT source triggers it), and it writes no sync_status -- it records a terminal job row and deletes this source's superseded items. It is also the ONLY non-raising exit that writes nothing: _ingest_file_body writes synced when processed == total and error otherwise, and the remaining exits raise. So reingested suppressed a clear that nothing else performs, and the marker sat on a file that was present and accounted for.

Fixed by deleting the guard and the flag, which leaves the CAS as the only discriminator -- and it is the right one. An ingestion that ran has already moved the row off missing, so the write no-ops; the outcome that wrote nothing is exactly the one it clears; a failed ingest raises, so the clear is never reached with the file unread. That removes a special case rather than adding one, and it is the same shape as round 8's fold.

Two tests pin it. test_an_ingest_that_writes_no_status_still_clears_missing is mutation-verified RED against 2c6dbc687 (assert 'missing' == 'synced'). test_the_clear_loses_to_a_status_the_ingest_wrote pins the CAS that makes the change safe, with the pipeline writing error for a partial write mid-sweep.

sync.py:90 -- "attempted once" is false when detect_changes() returns false. FIXED (claim corrected).

Correct: a legacy blob-error row is polled every sweep until an attempt of its own fails, not once. The BEHAVIOUR is deliberate and stays -- promoting a lifecycle value out of the blob is what three earlier rounds rejected, because a blob value cannot be ordered against the column and would mark a source errored that had in fact recovered. What was wrong is the bound I claimed for it. The comment now says the row is polled like any other source until an attempt fails, and that a poll finding nothing to fetch costs what every healthy source's poll costs. test_legacy_json_only_error_is_quiesced_after_one_attempt is renamed ..._by_its_first_failure and its docstring matches.

store.py:587 -- "Both writes are compare-and-set on the blob AND the column". FIXED (claim corrected).

Correct, and the CODE is right while the comment overstated it. The repair predicates on both (WHERE id = ? AND sync_status = 'pending' AND properties = ?); the retirement predicates on the blob alone, which is the only field it writes. Requiring the column to be unmoved there would abandon the copy for precisely the transitions that are expected to happen -- every live writer moves the column and leaves properties untouched. The comment now states each write's actual precondition and why they differ.

store.py:1693 -- the obsolete "migration's error-lift" claim. FIXED.

Round 6 removed the lift and the migration now refuses to promote error, so that rationale for stripping the blob on the bundle-restore path no longer holds. The strip stays, for the reason that does hold: after an insert the column is the only place a status lives, and a value the allowlist just refused has no business surviving inside the row. While correcting it I found the same dead claim in TestSyncAllSkipsErroredSources's class docstring ("the store lifts those onto the column when it opens") and removed it too.

Gates on 9c1fd2014: 319 tests pass across the five knowledge/handler suites (-n 4), flake8 clean, black --check --target-version py310 clean, isort clean, scripts/check_sync_io_in_async.py passes. Single commit.

[bolichen97]

Reviewed via parallel subagent audit: diff matches description, CI fully green, no blocking findings, no unresolved threads.