fix(indexer): harden OpenCode ingestion against corrupt rows and cursor drift (#21) - #48
Conversation
…or drift (#21) Four independent robustness bugs in the OpenCode (SQLite) ingestion path, all in indexer.py: - _load_opencode_raw_entries: json.loads(row["data"]) and _epoch_ms_to_iso(row["time_created"]) ran unguarded per row. One corrupt row (bad JSON, NULL time_created) raised and aborted ingestion of the entire DB (all projects/sessions). Extracted _parse_opencode_row(), which try/excepts (TypeError, ValueError, OverflowError, json.JSONDecodeError) per row and returns None for a malformed row; the row is skipped instead of poisoning the batch. - index_opencode_db: os.path.realpath(row["worktree"]) crashed with TypeError when a project's worktree column was NULL. Now skips rows with worktree is None before calling realpath. - index_opencode_db: the "already ingested" filter compared ex.ply_start (an index into the (time_created, id)-sorted message/part list, rebuilt fresh every run) against a stored last_ply_end. A new message arriving with an earlier time_created than already-processed messages shifts every later index, which (a) changed source_turn_id for old exchanges, re-emitting them under a new exchange_id, while (b) genuinely new exchanges could collide with a stale exchange_id and get silently dropped. Replaced the position-based source_turn_id (str(ply_start)) with the exchange's own stable id (sha256 of conversation_id + the OpenCode user message id, already computed by parse_opencode_exchanges and independent of list position), and replaced the last_ply_end lookup with a direct query of already-persisted source_turn_ids for the session. Position drift can no longer duplicate or drop turns. - index_opencode_db: sqlite3.connect(f"file:{path}?mode=ro", uri=True) misparses a path containing '?', '#', or spaces (SQLite URI syntax treats them as query/fragment delimiters), silently opening the wrong file or failing to open it. Percent-encode the path (urllib.parse.quote, safe="/") before building the URI. Verification (RED -> GREEN): added tests/test_opencode_ingest_robustness.py with one test per bug. Confirmed each fails against the pre-fix code (git stash of indexer.py) with the exact reported failure mode -- JSONDecodeError aborting the whole DB, TypeError on NULL worktree, dropped/duplicated exchange content after an out-of-order message insert (content-level assertion, not just row count, since the row count coincidentally matches under the bug), and OperationalError: no such table for a '#' in the DB path -- then confirmed all four pass after restoring the fix. make check (run directly, not via the pre-commit hook): ruff 0 errors, pyright 0 errors, 665 pytest tests pass (4 new). Note: --no-verify used deliberately, per issue #46 (this worktree's git hooks run tests that spawn nested git subprocesses which can corrupt the outer repo's .git/config and index when run from inside a hook). make check was run directly beforehand and passes clean. Closes #21
| "timestamp": _epoch_ms_to_iso(row["time_created"]), | ||
| "data": json.loads(row["data"]), | ||
| } | ||
| if kind == "part": |
There was a problem hiding this comment.
Priority-1: the new guard only catches JSON decode/conversion errors. A row whose data is syntactically valid JSON but not an object (e.g. null, [], "text") is still accepted into raw_entries. parse_opencode_exchanges then calls .get() on that value while building message_role/boundaries, raising `AttributeError" and aborting the whole database's ingestion again — exactly the failure mode #21 asked to eliminate, just via a different input shape. Please also reject/skip non-dict JSON values here, not just JSON decode failures.
There was a problem hiding this comment.
Fixed in 828564c. _parse_opencode_row now checks isinstance(data, dict) after json.loads and returns None (row skipped) for syntactically-valid-but-non-object JSON (null, [], "text"). Added test_non_dict_json_row_is_skipped_not_crashed, which inserts message rows with those three payload shapes plus one such part row; confirmed RED against the pre-fix guard (AttributeError: 'list' object has no attribute 'get') and GREEN after the fix. make check run directly: ruff 0 errors, pyright 0 errors, 667 tests pass.
| "WHERE harness = 'opencode' AND source_session_id = ?", | ||
| (session_id,), | ||
| ) | ||
| } |
There was a problem hiding this comment.
Priority-1: backward-compat break on upgrade. Databases indexed before this patch store OpenCode exchanges with source_turn_id=str(ply_start) (a numeric string). After switching to the message-id-based cursor, every newly parsed exchange gets a hashed exchange.id instead, so known_exchange_ids (built from the old numeric turn ids) never recognizes any prior exchange as known. The first post-upgrade ingestion run will write a second copy of every previously-indexed OpenCode exchange under new canonical ids. The new regression test only starts from an empty target DB, so it doesn't cover this upgrade path. Please add a migration/compat path (e.g. a one-time backfill that recomputes existing rows' turn ids to the new scheme, or a fallback lookup that also matches the legacy positional id) and a regression test that ingests into a DB already populated under the OLD cursor scheme, then re-ingests under the new code and asserts no duplicates are created.
There was a problem hiding this comment.
Fixed in 828564c. Went with the fallback-lookup option: new_exchanges now also excludes an exchange when str(ex.ply_start) (the legacy numeric turn id) is already present in known_exchange_ids, alongside the existing exchange.id (hash) check. Safe because post-fix source_turn_id is always a 64-char sha256 hex string, so a numeric-string match can only ever be a pre-upgrade legacy row, never a false positive against a genuinely new exchange. Chose this over an in-place backfill migration to avoid rewriting exchanges.id/canonical_exchange_id (primary key referenced by code_touches/exchange_files), which would need cascading updates across those tables. Added test_upgrade_from_legacy_position_based_cursor_does_not_duplicate, which seeds the target DB via ingest_parse_result with a source_turn_id=str(ply_start) exchange (i.e. exactly what pre-patch index_opencode_db persisted), then re-runs the current index_opencode_db against the same OpenCode DB and asserts the exchange count stays at 1. Confirmed RED against the pre-fix filter (assert 1 == 0) and GREEN after. make check run directly: ruff 0 errors, pyright 0 errors, 667 tests pass.
…plication Two priority-1 issues raised on PR #48 for #21: - _parse_opencode_row only caught JSON decode/conversion errors. A row whose data is syntactically valid JSON but not an object (null, [], "text") passed the guard, entered raw_entries, and crashed parse_opencode_exchanges's `entry["data"].get(...)` calls with AttributeError -- the same "one bad row aborts the whole DB" failure #21 asked to eliminate, just via a different input shape. Now checks `isinstance(data, dict)` after json.loads and skips the row (returns None) if it isn't. - index_opencode_db's new id-based dedup broke upgrading from pre-patch data: exchanges indexed before this fix have source_turn_id = str(ply_start) (a numeric string); after the cursor change every freshly-parsed exchange computes a hashed exchange.id instead, so known_exchange_ids (built from the stored source_turn_id column) never matched a prior row and the first post-upgrade run would insert a second copy of every previously-indexed OpenCode exchange. Added a fallback check: an exchange is also treated as already-known if str(ex.ply_start) is present in known_exchange_ids. New-scheme ids are always 64-char sha256 hex, so this can never false-positive against a genuinely new exchange -- it only matches legacy numeric turn ids left over from before the upgrade. Tests added to tests/test_opencode_ingest_robustness.py: - test_non_dict_json_row_is_skipped_not_crashed - test_upgrade_from_legacy_position_based_cursor_does_not_duplicate (seeds the target DB via ingest_parse_result with a source_turn_id=str(ply_start) exchange -- i.e. exactly what the pre-patch index_opencode_db would have persisted -- then re-runs the current index_opencode_db and asserts the count stays at 1) Verification (RED -> GREEN): stashed the two indexer.py fixes (keeping the new tests), confirmed both fail with the exact reported failure modes (AttributeError: 'list' object has no attribute 'get'; assert 1 == 0 on the duplicate-count check), then restored the fixes and confirmed both pass. All 6 tests in the file pass. make check (run directly, not via the pre-commit hook, per #46): ruff 0 errors, pyright 0 errors, 667 pytest tests pass (2 new). Addresses review comments on #48.
| for ex in exchanges | ||
| if ex.id not in known_exchange_ids | ||
| and str(ex.ply_start) not in known_exchange_ids | ||
| ] |
There was a problem hiding this comment.
Priority-1, still incomplete: the legacy-position fallback matches by position (str(ex.ply_start)), but a legacy source_turn_id identifies a position, not a stable message identity. If an existing legacy exchange has ply_start==0 and a newly-arrived (out-of-order) message now sorts ahead of it, the new message shifts to position 0 and gets wrongly suppressed as 'known' (since position 0 matches the legacy id), while the old exchange shifts to position 2 and gets re-inserted as a duplicate under its new hash id. So the exact scenario this PR is supposed to fix (new out-of-order messages arriving after upgrade) still loses the real new exchange AND duplicates the old one. Please match legacy rows to their actual original identity instead of relying on position (e.g. a one-time migration that rewrites legacy exchanges' source_turn_id/id to the new hashed scheme using their original message id, so no position-based fallback is needed at all), and add a regression test where a new out-of-order message arrives ahead of an existing position-0 legacy exchange after upgrade.
There was a problem hiding this comment.
Agreed, and confirmed with a test — fixed in e0dc313. Dropped the position-based fallback entirely.
Legacy rows (source_turn_id is a short all-digit string — _is_legacy_opencode_turn_id distinguishes these from the new scheme's 64-char sha256 hex) are now matched to the current parse by content — the (user_content, agent_content) pair — instead of position. Content is invariant to how much the (time_created, id)-sorted list has shifted, since it comes from the message's actual text, not its index. On a match, the existing row's source_turn_id is rewritten in place to the new exchange.id.
One deliberate deviation from "rewrite source_turn_id/id": I only rewrite source_turn_id, not id/canonical_exchange_id. Reason: those are the FK target for code_touches/exchange_files/palace_objects/vec_exchanges, and palace_objects.id is itself sha256(f"palace:{exchange_id}") — rewriting exchanges.id would need to cascade into palace_objects.id and then its own downstream rooms/symbols/vec_palace. That cascade buys nothing here: index_opencode_db never falls through to ingest_parse_result's id-based dedup for an OpenCode exchange once this content-based pre-filter has already recognized it, so leaving the primary key untouched gives the identical deduplication guarantee with zero cascade risk. Happy to add the full cascade if you'd rather have id stay in sync with source_turn_id on principle, but functionally it's a no-op.
Added test_upgrade_with_out_of_order_message_migrates_legacy_and_captures_new, reproducing your exact scenario: seed one legacy exchange at ply_start==0 via a new _seed_legacy_opencode_exchange helper, then insert a new message with an earlier time_created (so the legacy exchange shifts to position 2), then reindex. Confirmed RED against the previous position-fallback commit — the new message's content was dropped while the old exchange showed up as if it were the only row (silent loss, exactly as you described) — then GREEN after this fix: reindexed count is 1, and both the legacy and new exchange content are present with no duplicates. All 7 tests in the file pass. make check run directly (ruff 0 errors, pyright 0 errors, 668 tests pass).
…ed migration Follow-up to PR #48 review: the previous fix (str(ex.ply_start) fallback in known_exchange_ids) was still broken, because a legacy source_turn_id identifies a *position*, not a stable identity. Exact failure the reviewer traced: a legacy exchange at ply_start==0 plus a newly-arrived out-of-order message that now sorts ahead of it shifts the new message into position 0 (wrongly matched as "known" against the legacy id) while the old exchange shifts to position 2 (re-inserted as a duplicate under its new hash id) -- the exact upgrade scenario #21 needs to survive, still failing. Fix: drop the position-based fallback entirely. Legacy rows (source_turn_id is a short all-digit string, distinguishable from the new scheme's 64-char sha256 hex by _is_legacy_opencode_turn_id) are now matched to the current parse by *content* -- the (user_content, agent_content) pair, which is invariant to how much the underlying (time_created, id)-sorted list has shifted, since it's derived from the message's actual text, not its list position. On a match, the existing DB row's source_turn_id is rewritten in place to the new exchange.id (a stable hash of conversation_id + the OpenCode message id); intentionally left untouched: `id`/`canonical_exchange_id` (the FK target for code_touches/exchange_files/palace_objects/ vec_exchanges) — rewriting those would cascade into palace_objects.id (itself sha256(f"palace:{exchange_id}")) and its own downstream rooms/symbols/vec_palace, for no functional gain: index_opencode_db never relies on ingest_parse_result's id-based dedup for OpenCode once this content-based pre-filter runs, so leaving the primary key stable is strictly safer with an identical deduplication guarantee. Tests (tests/test_opencode_ingest_robustness.py): - extracted _seed_legacy_opencode_exchange() to build a pre-patch persisted state without duplicating ~50 lines per test - test_upgrade_with_out_of_order_message_migrates_legacy_and_captures_new reproduces the reviewer's exact scenario: seed one legacy exchange at ply_start==0, then insert a new message with an earlier time_created, then reindex; asserts exactly one new row is inserted and both the legacy and new exchange content are present (not duplicated/dropped) Verification (RED -> GREEN): stashed the indexer.py fix (keeping the new test), confirmed it fails exactly as predicted -- the new message's content ("new ...") is dropped and the legacy content ("legacy ...") appears as if unique when it should coexist with the new one, i.e. the new exchange is suppressed and nothing gets duplicated in this particular assertion shape, matching the reviewer's "loses the new exchange" half of the report. Restored the fix: all 7 tests in the file pass, including the pre-existing test_upgrade_from_legacy_ position_based_cursor_does_not_duplicate. make check (run directly, not via the pre-commit hook, per #46): ruff 0 errors, pyright 0 errors, 668 pytest tests pass (1 new). Addresses further review comment on #48.
| legacy_by_content = { | ||
| (row["user_content"], row["agent_content"]): row["source_turn_id"] | ||
| for row in existing_rows | ||
| if _is_legacy_opencode_turn_id(row["source_turn_id"]) |
There was a problem hiding this comment.
Priority-2 (round 3), two remaining gaps in the content-based legacy matching:
-
legacy_by_contentis a dict keyed only by(user_content, agent_content). If two persisted legacy exchanges in the same session happen to share the same content pair, the second overwrites the first in the dict before matching runs. After both positions shift, only one gets migrated to its new identity; the other legacy row is never matched and stays behind as a permanent duplicate. Fix: key by a list/multiset of candidates per content pair (matched in original order, e.g. FIFO), not a single dict slot — so N legacy rows with identical content pair correctly consume N incoming matches. -
When a legacy exchange gets migrated (source_turn_id rewritten to the new hash-based id), its
ply_start/ply_end/session_refare left at the pre-upgrade position. In the 0\u21922 shift scenario this leaves the migrated row's ply/session_ref still pointing at ply 0 (the position now occupied by a different, new exchange), so context ordering andverbatim_refresolution point at the wrong source position instead of the migrated exchange's actual new location. Fix: when migrating a legacy exchange's identity, also update its ply_start/ply_end/session_ref to match its actual current position.
Please add regression tests for both: (a) two legacy exchanges in one session with identical (user_content, agent_content) pairs, both surviving an upgrade+shift without duplication; (b) after a position shift, the migrated exchange's ply_start/session_ref correctly reflects its new position (not the stale pre-upgrade one).
There was a problem hiding this comment.
Both fixed in 62a87e3.
(1) Duplicate-content collision: legacy_by_content is now dict[(user_content, agent_content), list[Row]], sorted per key by original ply_start and consumed FIFO. Correctness argument: two real messages' relative order is preserved across any number of later out-of-order insertions, because time_created on an existing row never changes — only new rows get added around it. So N legacy candidates for a content pair, sorted by their original ply_start, line up 1:1 in order with the N matching entries encountered while walking the current parse in ply_start order. Test: test_upgrade_with_duplicate_content_legacy_exchanges_does_not_leave_duplicate — two legacy exchanges with byte-identical content, plus a third genuinely-new message that shifts both; asserts exactly 3 rows survive.
(2) Stale ply_start/ply_end/session_ref: the migration UPDATE now also sets these three columns to the exchange's actual current position, not just source_turn_id. Test: test_migrated_legacy_exchange_reflects_new_position_not_stale_one — asserts a migrated row moves from the stale 0-1/#ply=0-1 to the real 2-3/#ply=2-3 after a shift.
Verified RED (stashed the fix, both new tests failed exactly as predicted — dict collision dropped one duplicate down to 2/3 rows; stale-position assertion saw (0, 1) instead of (2, 3)) then GREEN. All 9 tests in the file pass. make check run directly: ruff 0 errors, pyright 0 errors, 670 tests pass.
On the deeper structural question you asked me to address directly, rather than paper over: yes, there is one residual limitation, and I want to be explicit about it rather than imply this is now fully exact. Content-based matching is a heuristic reconciliation, not a true identity recovery — because pre-migration rows never persisted the OpenCode message's real id, (user_content, agent_content) is the only durable signal available to reconcile them after the fact. My FIFO-by-original-position fix correctly handles any number of legacy rows sharing identical content among themselves. It does not correctly disambiguate if a genuinely new message with byte-identical content to an existing legacy group also arrives in the very same reindex run and happens to sort earlier than one of the legacy siblings — in that case a legacy row could get migrated to the position of the wrong (but content-identical) sibling. This never causes duplication or data loss — the exchange count and the set of user_content/agent_content values are always correct — it only risks ply_start/session_ref pointing at a different member of an already content-indistinguishable group, which is unobservable in practice since those rows display identically anyway. Closing this fully would require a schema change (persisting the real message id at ingestion time, which none of the pre-patch data has) rather than another matching refinement, so I'm disclosing it instead of chasing it with more heuristics. Happy to scope that as a separate follow-up issue if you'd like it tracked.
…ref on legacy migration Follow-up to PR #48 review round 3, two priority-2 gaps in the content-based legacy migration: 1. legacy_by_content was a plain dict keyed by (user_content, agent_content). Two legacy exchanges sharing that exact pair (a repeated user message in the same session) collided: the second overwrote the first before matching ran, so after a position shift only one got migrated and the other stayed behind as a permanent duplicate on the next re-index. Fixed by keying to a list of candidate rows per content pair, sorted by their original ply_start and consumed FIFO. Old messages' relative order is preserved across any number of new out-of-order insertions (time_created never changes once a message exists), so N legacy rows with identical content correctly line up 1:1 with the N corresponding entries encountered while walking the current parse in ply order. 2. Migrating a legacy row only rewrote source_turn_id, leaving ply_start/ply_end/session_ref at their pre-upgrade values. After a 0->2 shift this left the migrated row pointing at ply 0 -- now occupied by a different, newly-arrived exchange -- corrupting ply-adjacent context ordering and verbatim_ref resolution. The UPDATE now also sets ply_start, ply_end, and session_ref to the exchange's actual current position. Tests added to tests/test_opencode_ingest_robustness.py: - generalized _seed_legacy_opencode_exchange -> _seed_legacy_opencode_exchanges to seed N pre-patch exchanges in one call (was hardcoded to exactly 1) - test_upgrade_with_duplicate_content_legacy_exchanges_does_not_leave_duplicate: two legacy exchanges with byte-identical (user_content, agent_content), a third genuinely-new message shifts both; asserts exactly 3 rows survive with no duplicate - test_migrated_legacy_exchange_reflects_new_position_not_stale_one: asserts a migrated exchange's ply_start/ply_end/session_ref move from the stale 0-1 to the actual current 2-3 after a shift Verification (RED -> GREEN): stashed the two fixes (keeping the new tests) -- confirmed both fail exactly as predicted (3-row assertion saw 2 due to the dict-collision drop; ply_start/ply_end assertion saw the stale (0, 1) instead of (2, 3)). Restored the fixes: all 9 tests in the file pass. make check (run directly, not via the pre-commit hook, per #46): ruff 0 errors, pyright 0 errors, 670 pytest tests pass (2 new). Addresses round-3 review comment on #48. Disclosed in the PR reply: a residual limitation remains for byte-identical-content exchanges that collide with a *newly arriving* same-content message in the same reindex run -- ply_start/session_ref among that specific identical- content group can be assigned to a different sibling than its original one. This never causes duplication or data loss (the count and content set are always correct), only a traceability nit confined to rows whose displayed content is indistinguishable from each other anyway. A fully exact fix would require having persisted each legacy row's real OpenCode message id at original ingestion time, which pre-patch code never recorded -- content is the only durable signal available for reconciling rows created before this migration existed.
|
Final review pass (round 4) found one more edge case in the legacy-content matching: a genuinely new message arriving with byte-identical The author's own round-3 reply already disclosed this exact limitation proactively before this round's review ran. Accepting this as a known, documented residual limitation rather than requesting further changes: it requires an exact content collision between pre-existing and brand-new data at the specific moment of the first post-upgrade reindex (self-limiting — only that first pass touches un-migrated legacy rows), and fully closing it would require persisting an identity signal the legacy schema never captured, which is out of scope for #21. Documented in the PR description under "Known, accepted residual limitation". Approving for merge. |
1 participant
Closes #21
Root cause
Four independent robustness gaps in
index_opencode_db/_load_opencode_raw_entries(src/codeatrium/indexer.py):json.loads(row["data"])/_epoch_ms_to_iso(row["time_created"])ran with no try/except. One corrupt row (bad JSON, NULLtime_created) raised and aborted ingestion of the entire DB (every project/session).worktree:os.path.realpath(row["worktree"])raisedTypeErrorwhen a project'sworktreecolumn wasNULL.ex.ply_start— an index into the(time_created, id)-sorted message/part list, rebuilt fresh every call — against a storedlast_ply_end. A new message arriving with an earliertime_createdthan already-processed messages shifts every later index. This both re-emits old turns under a newexchange_id(duplicate rows) and can silently drop genuinely-new turns if their shifted index collides with a stale hash.file:URI:sqlite3.connect(f"file:{path}?mode=ro", uri=True)misparses a path containing?,#, or spaces (SQLite URI syntax treats them as query/fragment delimiters), opening the wrong file or failing outright.Fix
_parse_opencode_row(): try/excepts(TypeError, ValueError, OverflowError, json.JSONDecodeError)per message/part row, returnsNonefor a malformed row so it's skipped instead of poisoning the whole batch.worktree is Nonebefore callingrealpath.source_turn_id(str(ply_start)) with the exchange's own stable id (sha256 ofconversation_id+ the OpenCode user message id — already computed byparse_opencode_exchanges, independent of list position). Replaced thelast_ply_endposition lookup with a direct query of already-persistedsource_turn_ids for the session. Position drift can no longer duplicate or drop turns.urllib.parse.quote(..., safe="/")) before building thefile:URI.Verification
Added
tests/test_opencode_ingest_robustness.py, one test per bug:test_corrupt_row_does_not_abort_whole_db_ingestiontest_worktree_none_is_skipped_not_raisedtest_out_of_order_message_does_not_reemit_existing_exchange(asserts actualuser_contentvalues, not just row count — the pre-fix code coincidentally produces the same row count via a hash collision while silently dropping the new turn and duplicating the old one)test_db_path_with_special_uri_characters_is_opened_correctlyRED: stashed the
indexer.pyfix, reran the 4 tests — 3 failed with the exact reported errors (json.JSONDecodeError,TypeError: expected str, bytes or os.PathLike,sqlite3.OperationalError: no such table: project); the 4th (reorder) initially passed against a row-count-only assertion, so I strengthened it to check content and confirmed it then fails against the pre-fix code with the predicted duplicate/drop.GREEN: restored the fix, all 4 pass.
make check(run directly, not via the pre-commit hook — see note below): ruff 0 errors, pyright 0 errors, 665 pytest tests pass (4 new).Note on
--no-verifyPer issue #46: this worktree's git hooks run
make checkvia pytest, and several existing tests spawn nestedgitsubprocess calls that can corrupt the outer repo's.git/config/index when run from inside a hook. Committed/pushed with--no-verifyafter runningmake checkdirectly beforehand (passes clean, see above).Scope note
Issue #19 (parallel worktree) touches
parse_opencode_exchanges(~line 636-693) in the same file to addgit_branchextraction — this PR only touches_load_opencode_raw_entries/index_opencode_db(non-adjacent), no overlap.Update: legacy-upgrade compatibility (review rounds 2-4)
The original fix above only covered fresh/never-migrated OpenCode DBs. Follow-up rounds fixed the upgrade path for DBs that were already indexed under the old position-based
source_turn_id=str(ply_start)scheme:(user_content, agent_content)content, using adict[(user_content, agent_content), list[Row]]sorted by originalply_startand consumed FIFO — correctly handling any number of legacy rows sharing identical content among themselves.ply_start/ply_end/session_refto its actual current position (previously left pointing at the pre-upgrade position).test_upgrade_with_duplicate_content_legacy_exchanges_does_not_leave_duplicate,test_migrated_legacy_exchange_reflects_new_position_not_stale_one.Known, accepted residual limitation: content-based matching is a heuristic reconciliation, not true identity recovery, because pre-migration rows never persisted OpenCode's real message id —
(user_content, agent_content)is the only durable signal available after the fact. It correctly disambiguates legacy rows from each other, but cannot distinguish a genuinely new message that arrives with byte-identical content to an existing legacy group in the same reindex pass from one of those legacy rows itself. This requires an exact content collision between old and new data at upgrade time — narrow enough (and self-correcting on the next normal reindex, since only the first post-upgrade pass touches un-migrated legacy rows) that a full fix would need persisting additional signal the legacy schema never captured, which is out of scope for this issue. Flagging explicitly rather than silently accepting it.