Add conflict resolution test and check semantic release - #82
Merged
Conversation
- Extract unit tests from tests/llm/test_conflict_resolution.py into tests/unit/test_conflict_resolution.py - Register 'llm' pytest marker in pyproject.toml for LLM integration tests
📝 WalkthroughWalkthroughUpdated developer workflow docs and tooling (pre-commit/commit message target, pytest marker); refined conflict-resolution prompt logic to preserve IDs and tighten update/delete/no-change rules; added extensive unit and LLM-driven tests covering parsing, retry, and many scenario cases. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Resolver as ConflictResolver
participant LLM
participant Parser
participant Store as Datastore
Client->>Resolver: resolve_conflicts(old_entities, new_entities, prompt_template)
Resolver->>LLM: send compiled prompt (default_conflict_resolution / conflict_resolution)
LLM-->>Resolver: return classification JSON (may be wrapped in markdown)
Resolver->>Parser: parse & validate JSON (handle markdown, retry on transient errors)
Parser-->>Resolver: structured events per-entity (ADD/UPDATE/DELETE/NONE)
Resolver->>Store: apply events or return decisions (preserve IDs, attach old_entity where needed)
Resolver-->>Client: return result_by_id
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
pyproject.toml (1)
63-70:⚠️ Potential issue | 🟠 MajorExclude
llmtests from the default pytest run.Line 64 still uses
-m 'not phoenix', so@pytest.mark.llmtests run by default despite the test module documentation explicitly stating they are "excluded from the default run." This will cause failures in default test runs when no LLM backend is configured.💡 Suggested fix
[tool.pytest.ini_options] -addopts = "--ignore=explorations -m 'not phoenix'" +addopts = "--ignore=explorations -m 'not phoenix and not llm'" markers = [ "e2e", "unit", "phoenix", "llm" ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pyproject.toml` around lines 63 - 70, The pytest default options currently exclude only phoenix tests; update the addopts under [tool.pytest.ini_options] so that marker llm is also excluded by default (e.g., change the -m expression to exclude both phoenix and llm). Locate the addopts setting in pyproject.toml and modify its marker expression (the value assigned to addopts) to include "not llm" alongside "not phoenix" so `@pytest.mark.llm` tests are skipped when running the default test suite.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/unit/test_conflict_resolution.py`:
- Line 1: Two test files share the same module name 'test_conflict_resolution',
causing mypy duplicate-module errors; rename one of them (for example change the
unit test module named test_conflict_resolution to test_conflict_resolution_unit
or test_conflict_resolution_unit_tests) and update any references/imports or
test discovery config so the module name no longer collides (ensure the test
runner and any imports that referenced test_conflict_resolution now point to the
new module name).
---
Outside diff comments:
In `@pyproject.toml`:
- Around line 63-70: The pytest default options currently exclude only phoenix
tests; update the addopts under [tool.pytest.ini_options] so that marker llm is
also excluded by default (e.g., change the -m expression to exclude both phoenix
and llm). Locate the addopts setting in pyproject.toml and modify its marker
expression (the value assigned to addopts) to include "not llm" alongside "not
phoenix" so `@pytest.mark.llm` tests are skipped when running the default test
suite.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
AGENTS.mdjustfilepyproject.tomltests/llm/test_conflict_resolution.pytests/unit/test_conflict_resolution.py
test(llm)!: improve conflict resolution prompt clarity and add comprehensive test suite - Enhanced UPDATE operation to only trigger on concrete new information, not mere rewordings - Clarified DELETE operation to explicitly handle negation phrases like "no longer", "switched away from", "quit" - Improved NONE operation to properly handle paraphrases and synonyms - Fixed ADD operation to use retrieved entity ID instead of generating new ID - Added extensive examples for each operation type with realistic scenarios - Replaced old test file with comprehensive parametrized test suite covering 60+ scenarios
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
kaizen/llm/conflict_resolution/prompts/conflict_resolution.jinja2 (1)
22-36:⚠️ Potential issue | 🟠 MajorResolve conflicting ADD ID instructions.
Line 22 says additions can use a new ID, while Line 36 says additions must use the retrieved entity ID. This contradiction can cause inconsistent ADD outputs.
Suggested fix
- "id" : "<ID of the entity>", # Use existing ID for updates/deletes, or new ID for additions + "id" : "<ID of the entity>", # Use existing ID for updates/deletes; for additions, use the retrieved entity ID🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@kaizen/llm/conflict_resolution/prompts/conflict_resolution.jinja2` around lines 22 - 36, The prompt has a contradiction between the "id" field comment that allows a "new ID for additions" and the later rule "If there is an addition, use the ID from the retrieved entity"; update the template so the "id" comment and any related bullet points consistently require using the retrieved entity's ID for ADD events (remove or replace the "new ID for additions" wording), and ensure the "event" rules and the sentence "If there is an addition, use the ID from the retrieved entity." remain aligned with the ADD requirement; search for the "id" field, the "event" field, and the sentence "If there is an addition, use the ID from the retrieved entity." to make the edits.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@kaizen/llm/conflict_resolution/prompts/default_conflict_resolution.jinja2`:
- Around line 119-123: The example for the entity with "id": "2" (type "fact")
is inconsistent: it has "event": "NONE" but the "content" changes from "Likes
racing" to "Likes cheese pizza"; update the example so that a NONE event
preserves the original content (restore "content" to "Likes racing") or, if a
content change was intended, change "event" to a modifying value (e.g.,
"MODIFIED") to match the new content.
In `@tests/llm/test_long_conflict_resolution.py`:
- Around line 114-121: The test currently treats expect_any_of as requiring all
listed per-ID events simultaneously which rejects valid mutually-exclusive
outcomes; update the assertion logic in test_conflict_resolution_scenarios so
that expect_any_of is evaluated as a disjunction (any(...) matches) rather than
a conjunction, i.e., for each candidate in expect_any_of accept the entire
candidate map as an alternative valid result and pass if any candidate map fully
matches the actual per-ID results; locate and change the code that
iterates/compares expect_any_of (references: expect_any_of, expect, and the
test_conflict_resolution_scenarios function) so it checks any(candidate_matches)
instead of requiring all expected IDs/events at once.
- Around line 606-607: The test file uses a non-approved pytest marker
"@pytest.mark.llm"; replace that marker with one of the approved markers
("unit", "e2e", or "phoenix") in the test declaration (see the decorator above
the parametrized test in tests/llm/test_long_conflict_resolution.py) so the test
uses the permitted marker naming convention; choose the marker that best
describes the test scope (e.g., "unit" for fast isolated tests, "e2e" for
integration-like flows, or "phoenix" for special suite) and update the decorator
accordingly.
---
Outside diff comments:
In `@kaizen/llm/conflict_resolution/prompts/conflict_resolution.jinja2`:
- Around line 22-36: The prompt has a contradiction between the "id" field
comment that allows a "new ID for additions" and the later rule "If there is an
addition, use the ID from the retrieved entity"; update the template so the "id"
comment and any related bullet points consistently require using the retrieved
entity's ID for ADD events (remove or replace the "new ID for additions"
wording), and ensure the "event" rules and the sentence "If there is an
addition, use the ID from the retrieved entity." remain aligned with the ADD
requirement; search for the "id" field, the "event" field, and the sentence "If
there is an addition, use the ID from the retrieved entity." to make the edits.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
kaizen/llm/conflict_resolution/prompts/conflict_resolution.jinja2kaizen/llm/conflict_resolution/prompts/default_conflict_resolution.jinja2tests/llm/test_long_conflict_resolution.py
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/llm/test_long_conflict_resolution.py (2)
606-607:⚠️ Potential issue | 🟠 MajorReplace
@pytest.mark.llmwith an approved test marker.This suite is currently categorized with a non-approved marker. Please use one of
unit,e2e, orphoenix(and update the module docstring run command accordingly).#!/bin/bash set -euo pipefail echo "=== Declared markers in pyproject.toml ===" fd '^pyproject\.toml$' --exec rg -n '^\s*markers\s*=|^\s*"(unit|e2e|phoenix|llm)\b' {} echo echo "=== Marker usage in this file ===" rg -n --type=py '@pytest\.mark\.(unit|e2e|phoenix|llm)\b' tests/llm/test_long_conflict_resolution.pyAs per coding guidelines,
tests/**/*.py: Use pytest markersunit,e2e, andphoenixto categorize tests appropriately.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/test_long_conflict_resolution.py` around lines 606 - 607, Replace the non-approved pytest marker `@pytest.mark.llm` with one of the approved markers (unit, e2e, or phoenix) throughout this module (look for the decorator symbol "@pytest.mark.llm") and update the module docstring's example/test run command to reflect the chosen marker; ensure any other occurrences in this file (e.g., top-level decorators on tests or class definitions) are updated to the same approved marker so marker usage and the docstring remain consistent.
623-630:⚠️ Potential issue | 🟠 MajorPer-ID assertion logic still can reject valid mutually-exclusive outcomes.
The current loop validates each expected ID independently, which can force incompatible combinations in ambiguous scenarios. Model alternatives should be validated as whole-result disjunctions (
any(candidate_map_matches)), not merged per-ID constraints.Suggested direction
-class ConflictScenario(TypedDict): +class ConflictScenario(TypedDict, total=False): label: str old: list[RecordedEntity] new: list[RecordedEntity] expect: dict[str, str | list[str]] + expect_any_of: list[dict[str, str | list[str]]] +def _matches_expected( + result_by_id: dict[str, object], expected: dict[str, str | list[str]] +) -> bool: + for entity_id, expected_event in expected.items(): + if entity_id not in result_by_id: + return False + actual_event = result_by_id[entity_id].event + if isinstance(expected_event, list): + if actual_event not in expected_event: + return False + elif actual_event != expected_event: + return False + return True + def test_conflict_resolution_scenarios(scenario: ConflictScenario) -> None: result_by_id = {update.id: update for update in resolve_conflicts(scenario["old"], scenario["new"])} - for entity_id, expected_event in scenario["expect"].items(): - ... + if "expect_any_of" in scenario: + assert any(_matches_expected(result_by_id, candidate) for candidate in scenario["expect_any_of"]) + else: + assert _matches_expected(result_by_id, scenario["expect"])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/test_long_conflict_resolution.py` around lines 623 - 630, The per-ID assertions in the test loop force incompatible combinations; instead, build the full actual mapping from resolve_conflicts (result_by_id -> map of id to event) and assert that this entire mapping matches at least one allowed expected combination for the scenario: replace the per-entity assertions in the loop over scenario["expect"] with logic that constructs actual_map = {id: update.event for update in resolve_conflicts(scenario["old"], scenario["new"])} and then check any(expected_combination_matches(actual_map)) where expected_combination_matches verifies for every id in the expected combination that actual_map[id] equals the expected value or is contained in the expected list; reference resolve_conflicts, result_by_id/actual_map, scenario["expect"], expected_event and actual_event to locate and implement this change.
🧹 Nitpick comments (1)
tests/llm/test_long_conflict_resolution.py (1)
23-25: Use a fixed timestamp in test entities to reduce run-to-run variance.
datetime.now()makes fixture payloads non-deterministic across runs. For real-LLM tests, a constantcreated_atimproves reproducibility.Proposed change
from datetime import datetime @@ +FIXED_CREATED_AT = datetime(2025, 1, 1, 0, 0, 0) + def _entity(entity_id: str, content: str, entity_type: str = "fact") -> RecordedEntity: """Shorthand factory for a RecordedEntity.""" - return RecordedEntity(id=entity_id, type=entity_type, content=content, metadata={}, created_at=datetime.now()) + return RecordedEntity( + id=entity_id, + type=entity_type, + content=content, + metadata={}, + created_at=FIXED_CREATED_AT, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/llm/test_long_conflict_resolution.py` around lines 23 - 25, The _entity factory uses datetime.now() making test payloads non-deterministic; replace the dynamic timestamp with a fixed constant (e.g., a module-level TEST_TIMESTAMP = datetime(2020, 1, 1, 0, 0, 0)) and have _entity pass that constant as created_at when constructing the RecordedEntity so all test entities use the same stable timestamp.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/llm/test_long_conflict_resolution.py`:
- Around line 606-607: Replace the non-approved pytest marker `@pytest.mark.llm`
with one of the approved markers (unit, e2e, or phoenix) throughout this module
(look for the decorator symbol "@pytest.mark.llm") and update the module
docstring's example/test run command to reflect the chosen marker; ensure any
other occurrences in this file (e.g., top-level decorators on tests or class
definitions) are updated to the same approved marker so marker usage and the
docstring remain consistent.
- Around line 623-630: The per-ID assertions in the test loop force incompatible
combinations; instead, build the full actual mapping from resolve_conflicts
(result_by_id -> map of id to event) and assert that this entire mapping matches
at least one allowed expected combination for the scenario: replace the
per-entity assertions in the loop over scenario["expect"] with logic that
constructs actual_map = {id: update.event for update in
resolve_conflicts(scenario["old"], scenario["new"])} and then check
any(expected_combination_matches(actual_map)) where expected_combination_matches
verifies for every id in the expected combination that actual_map[id] equals the
expected value or is contained in the expected list; reference
resolve_conflicts, result_by_id/actual_map, scenario["expect"], expected_event
and actual_event to locate and implement this change.
---
Nitpick comments:
In `@tests/llm/test_long_conflict_resolution.py`:
- Around line 23-25: The _entity factory uses datetime.now() making test
payloads non-deterministic; replace the dynamic timestamp with a fixed constant
(e.g., a module-level TEST_TIMESTAMP = datetime(2020, 1, 1, 0, 0, 0)) and have
_entity pass that constant as created_at when constructing the RecordedEntity so
all test entities use the same stable timestamp.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (2)
pyproject.tomltests/llm/test_long_conflict_resolution.py
🚧 Files skipped from review as they are similar to previous changes (1)
- pyproject.toml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A starting point for testing LLM-based conflict resolution
Summary by CodeRabbit
Documentation
Tests
Chores
Behavior