fix: remap chunk results after skipped messages

[Kevin Turcios (KRRT7)]

Summary

• Remap source-stream chunk locations to consecutive storage message ordinals when failed messages are skipped.
• Preserve correct chunk embedding and semantic-reference locations for messages committed after a skipped message.
• Add end-to-end coverage for a failed message followed by a successful one.
• Retain regression coverage for graceful shutdown with a full chunk queue.

Validation

• make format check test
• GitHub CI and CodeQL

[Kevin Turcios (KRRT7)]

Bernhard Merkle (@bmerkle) robgruen if you wouldn't mind reviewing, thanks

[robgruen]

I would maybe add one more test that includes a repair & reimport to make sure there are no duplicates.

something like this: fail -> repair -> replay -> replay

[Kevin Turcios (KRRT7)]

I've pushed new tests for that, hopefully they're good.

[Bernhard Merkle (bmerkle)]

Review summary

Ran a multi-pass review (correctness, test coverage, comment accuracy, error handling) against the diff. The core fix is sound, but the specific bug this PR claims to fix is not actually covered by a test, and there's one error-handling inconsistency worth closing before merge.

Critical Issues

None. The positional remap in _commit_batch_from_chunk_results was traced against the reassembler's invariants (contiguous per-message result grouping in chunk_results, monotonic ordinals, zero-chunk-message handling) and is correct given how add_messages.py's producer/reassembler currently stage results.

Important Issues

1. The remap this PR fixes has no test verifying it end-to-end. Every extractor in the test suite returns an empty KnowledgeResponse, so add_knowledge_batch_to_semantic_ref_index never actually produces a SemanticRef, and no test reads transcript.semantic_refs/semantic_ref_index after a skip. A regression that reverted just the knowledge_items remap (lines 344-351) would pass every test currently in this PR. Suggest adding a test with a non-empty KnowledgeResponse (e.g. one entity per chunk, named after the chunk text) across a skip, then asserting the resulting semref's message_ordinal points at the correct storage message, not the source-stream one.
2. Narrow scenario coverage. All skip tests cover exactly one skip of a single-chunk message. Worth adding cases for: two consecutive skips, a skip followed by a multi-chunk message, a zero-chunk message adjacent to a skip, and a replay that crosses a batch boundary — these are the shapes most likely to expose an off-by-one in result_group_index.
3. Unguarded dict lookup breaks this function's own error-handling convention — see inline suggestion below.
4. Positional pairing is inferred from group counts, not verified by identity. ChunkProcessingResult already carries a message: TMessage field; asserting result.message is message while pairing would catch a future reassembler regression that silently swapped two same-sized messages (a scenario the current count-only checks wouldn't catch).

Suggestions

• Line 341-342: "Chunk results exceed staged messages with chunks" carries no numbers (consumed vs. expected group count) — worth adding for debuggability, consistent with the other guards in this function.
• Line 303 comment ("Store embedding by source message/chunk location until commit remaps it") is a little misleading — the map itself is never remapped; it's read with the source key throughout, while a separate list is built in storage order.

Strengths

• Transaction semantics are clean: every raise in this function happens before any DB-mutating call, inside a BEGIN IMMEDIATE/rollback-on-exception transaction — no path leaves partial state.
• Exception propagation to the caller is fail-fast, no swallowing (asyncio.TaskGroup).
• fail_on_text on ControlledExtractor is a well-justified addition for deterministic failure injection under concurrent extraction.
• _replay() realistically mirrors the production importer pattern in tools/ingest_email.py.

(Note: add_messages.py's ChunkProcessingResult docstring still references a nonexistent success property — confirmed that file is unchanged by this PR/identical to main, so that's pre-existing and out of scope here, but worth a follow-up cleanup.)

[Bernhard Merkle (bmerkle)]

This PR conflicts with main after #300/#301 (the import-style cleanup landed on main in the meantime). I resolved it locally on a mirror of this branch:

bmerkle/typeagent-py@7802f0b — bmerkle@7802f0b
(branch: review-changes-pr277)

Only one real conflict, in tests/test_add_messages_streaming.py: both sides added an independent import on the same line (this branch added AddMessagesResult from interfaces_core; main's #300 replaced the kplib alias with a direct KnowledgeResponse import from knowledge_schema). Both symbols are used in the file, so the fix keeps both imports — no logic changes elsewhere; the rest of the overlap (add_messages.py, conversation_base.py, test_add_messages_pipeline.py) auto-merged cleanly.

Verified after merging: make format (no changes), make check (pyright, 0 errors on 3.12 and 3.15), and uv run pytest (760 passed).

Kevin Turcios (@KRRT7) feel free to cherry-pick/merge that commit into this branch, or I'm happy to open it as a fresh PR against main if that's easier.

[Kevin Turcios (KRRT7)]

Bernhard Merkle (@bmerkle) why don't you open the PR against this branch so that it resolves it here and attribution is kept properly?