Fix test hygiene gaps for issue #289#304
Conversation
… note merge, batch dedup)
…replacements (qbtrix#287) 1. Route internal supersession to _internal_supersede_log (blocker 1) 2. Stop storing raw user_input as semantic replacement (blocker 2) 3. Wire ContradictionDetector in note() (item 3) 4. Rewrite tests to exercise observe() paths (item 4) 5. note-merge: set superseded=True, use UTC, add tier field
78b580e to
6adf592
Compare
prakashUXtech
left a comment
prakashUXtech
left a comment
There was a problem hiding this comment.
The core #289 work is genuinely good. The vacuous if approved: guard in test_chain_integration is now an unconditional assert approved is True; the new test_client_seams.py invokes all eight previously-uncovered MCP tools through the real Client(mcp).call_tool seam with non-trivial assertions (the soul_reload one even mutates the .soul on disk between two client calls, which is exactly right); and the evolution negative-path tests are proven fix-dependent (I ran them against dev, 3 fail there). The production fix in evolution/manager.py is sound too, including catching the silent-attribute-creation bug in _set_nested_attr.
The blocker is structural: this PR is stacked on the still-open #302. Only the last commit (6adf592) is #289 work; the other four are #302's memory-correctness commits, which aren't on dev and haven't been reviewed on their own PR. Merging #304 as-is would pull in #302's behavior changes unreviewed and cause duplicate-content conflicts later. Please rebase onto dev dropping the #302 commits (git rebase --onto origin/dev 95a5af1~1 fix/test-hygiene-289 leaves just 6adf592), and let #302 land through #302.
Two more:
- The PR body says
birthnow rejects unknown kwargs and fails fast, but the code only logs a warning and continues (the docs correctly say "ignored with a warning"). Pick one: implement real rejection (raiseTypeError) to match the body, or reword the body to "warn". Either way the new behavior needs acaplogtest, since right now a regression that drops the warning ships green. - Item 5 (the 84 brittle Rich substring assertions) isn't addressed or mentioned. A one-line deferral note in the body is fine, but it should be acknowledged.
Smaller: the new memory_settings param on birth is unrelated to #289 and untested (0 references), so either test it or split it out; the evolution/manager.py and soul.py file headers need their Updated: lines; and the title should be Conventional Commits (test: harden suite hygiene gaps (#289)).
The three real #289 items are solid, so once it's unbundled from #302 this is close.
2 participants
Hardened the test suite around issue #289 by closing weak assertions and adding missing seam coverage.
Changes:
Reject unknown Soul.birth() kwargs so typoed config keys fail fast.
Remove swallowed soul_dir= usage from contradiction tests.
Make the evolution approval trust-chain test assert approval before checking chain entries.
Add negative-path tests for evolution apply/propose behavior.
Add FastMCP client seam coverage for MCP tool registration, soul_audit, soul_dream, soul_reload, and soul_context_*.
Update API docs for the stricter Soul.birth() behavior.
Validation:
152 passed
ruff check passed