Skip to content

Fix ChatHistory.load failure atomicity - #292

Merged
Eigenwise merged 2 commits into
Eigenwise:mainfrom
Afloat16:fix/chat-history-load-atomic
Oct 9, 2026
Merged

Eigenwise merged 2 commits into
Eigenwise:mainfrom
Afloat16:fix/chat-history-load-atomic

Conversation

@Afloat16

@Afloat16 Afloat16 commented Oct 2, 2026

Copy link
Copy Markdown
Contributor

Fixes #290.

ChatHistory.load currently clears the existing messages and assigns replacement metadata before deserialization completes. A failed message can therefore erase the old state or leave a partial replacement.

This change builds the replacement messages and reads metadata in local variables, then updates the instance only after every message has been reconstructed and its multimodal paths processed. Existing exception handling and successful-load behavior stay unchanged. The load docstring now states the failure-atomicity guarantee.

Added regression tests cover malformed JSON, missing metadata, missing or invalid message data, class lookup failure, invalid schema content, failure in a later message, and Message validation failures. They assert that the original message list, message identity, serialized history, max_messages, and current_turn_id survive a failed load. Successful nonempty and empty replacements are covered too.

Validation on Python 3.12.14 with the repository's locked dependencies:

  • Clean main: 355 passed, 3 skipped.
  • The 15 new cases on the original load implementation: 11 failed, 4 passed.
  • The same 15 cases after this fix: 15 passed.
  • Full core suite with coverage: 370 passed, 3 skipped; 93% overall coverage and 100% chat_history.py coverage.
  • Repository-wide Black check: 203 files unchanged.
  • Repository-wide Flake8: passed.
  • git diff --check: passed.

The three skipped tests are existing live MiniMax integration tests; they were not run without a provider key. The local test environment also needed socksio for its SOCKS proxy. No project dependency or lockfile was changed.

@Eigenwise

Eigenwise commented Oct 4, 2026 •

Copy link
Copy Markdown
Owner

Historical draft-stage check: the staged load looked correct. I checked 20 selected tests plus late schema, Message, module and path-normalization failures; prior list/message identities and metadata survived.

The later check of 93b29ef7 ran 25 load tests and five additional identity/value probes, including late failures and successful multimodal replacement. The PR is now marked ready. No further test reshaping is requested; I still owe the final merge check.

@Afloat16

Afloat16 commented Oct 4, 2026

Copy link
Copy Markdown
Contributor Author

Pushed 93b29ef with test-only changes. Fault construction now uses a path/value parameter table, and the replacement setup and preservation assertions are shared. Changed function complexities are 1–3; an isolated C901 check passes the limit of 10 without the file exemptions.

The late normalization regression uses two real image messages: the first path is normalized before the second path fails. It checks exact exception classes/messages for wrapped TypeError and unchanged RuntimeError, alongside late schema, Message and missing-module failures. Prior list/message identities, serialized contents and both metadata fields remain checked.

Validation:

  • All 20 load regression/replacement cases pass; against the original load, 16 fail and 4 pass.
  • Full branch core: 375 passed, 3 skipped on Python 3.12.14 and 3.13.5; chat_history.py coverage is 100%.
  • Repository-wide Black/Flake8 and the isolated complexity check pass.
  • Exact-head quality CI passed Black, Flake8 and 383 tests with 3 skips on its merge snapshot, which includes eight newer-main MCP tests.

The three skips are existing live MiniMax tests without a provider key. Local environments only needed socksio for the configured SOCKS proxy; no project dependency or lockfile changed. Draft status is unchanged.

@Eigenwise

Eigenwise commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

Historical recheck of 93b29ef7: 25 load tests and five additional identity/value probes passed, including late failures and successful multimodal replacement. The production load method measured complexity 3 with all 17 statements and both branches covered. I found no introduced correctness defect.

This comment was written while the PR was a draft. It is now marked ready, and I still owe the final merge check. Keep the meaningful assertions; no further test reshaping is requested.

@Afloat16
Afloat16 marked this pull request as ready for review October 5, 2026 11:21
@Eigenwise

Eigenwise commented Oct 5, 2026 •

Copy link
Copy Markdown
Owner

I see this is marked ready at 93b29ef7, the same head we checked. The earlier 25 load tests and five additional identity/value probes passed. No further test reshaping is requested. I still owe the final merge check.

@Eigenwise Eigenwise left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked 93b29ef7b851596c8d22ddb106122157279d9e05: 25 focused history-load tests passed, no skips. The failure cases preserve the existing history and message identity; the replacement cases load typed content and the expected metadata. All nine touched functions have measured CRAP scores below 6 (1 to 5), with full coverage of each body. Thanks for the fix.

@Eigenwise
Eigenwise merged commit 7fc2c7e into Eigenwise:main Oct 9, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ChatHistory.load destroys the existing history before validating the replacement

2 participants