alishahryar1--free-claude-code
2bcaf3ac74
## Problem Messaging tree state, queue coordination, task ownership, and persistence were jointly mutated across several classes. Raw message IDs were treated as globally unique, allowing cross-chat collisions and unsafe cancellation or clear ordering. ## Changes | Before | After | | --- | --- | | Managers and processors coordinated partial tree mutations through exposed locks and mutable nodes. | MessageTree owns atomic transitions and returns detached effects to task, UI, and persistence owners. | | Raw message IDs and task identities could collide across chats or detached generations. | Scoped tree identities and opaque claim IDs isolate chats and reject stale task writes. | | Session snapshots duplicated graph links and retained ingress payloads. | Lean scoped snapshots rebuild validated indexes and continue reading existing session files. | | Stop, clear, and runner persistence could interleave across commit boundaries. | Admission epochs, cancellation-safe detach, and authoritative writes make committed cleanup durable. | | Tree implementation classes leaked through messaging package exports. | Adapter-facing values and ports remain supported while tree internals stay internal. | <!-- greptile_comment --> <details open><summary><h3>Greptile Summary</h3></summary> This PR makes messaging trees own their state transitions and scoped identities. The main changes are: - Scoped tree and voice ownership by platform and chat. - Opaque claim IDs for queued task execution. - Manager-owned atomic cancellation, clear, and successor task launch paths. - Lean scoped snapshots with legacy session restore support. - Messaging tree internals removed from the public package surface. </details> <h3>Confidence Score: 5/5</h3> This looks safe to merge. No blocking issues found in the changed code. <details><summary><h3><a href="https://www.greptile.com/trex"><img alt="T-Rex" src="https://greptile-static-assets.s3.amazonaws.com/trex/trex_green.svg" height="20" align="absmiddle"></a> T-Rex Logs</h3></summary> **What T-Rex did** - Executed the messaging and contract-boundary test suite with pytest across all specified test modules. - Observed the run completed with 96 tests passing in 4.10 seconds and an exit code of 0. - The exact pytest command used for the run is documented in the proof to enable reproducibility of the test scope. - The run log is available as an artifact for reviewers to inspect test output and details. <a href="https://app.greptile.com/trex/runs/14083891/artifacts"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewAllArtifactsDark.svg?v=4"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewAllArtifacts.svg?v=4"><img alt="View all artifacts" src="https://greptile-static-assets.s3.amazonaws.com/badges/ViewAllArtifacts.svg?v=4"></picture></a> <sub><a href="https://www.greptile.com/trex"><img alt="T-Rex" src="https://greptile-static-assets.s3.amazonaws.com/trex/trex_green.svg" height="14" align="absmiddle"></a> Ran code and verified through T-Rex</sub> </details> <details open><summary><h3>Important Files Changed</h3></summary> | Filename | Overview | |----------|----------| | src/free_claude_code/messaging/trees/runtime.py | Adds claim-scoped state transitions and cancellation tombstones for late runner writes. | | src/free_claude_code/messaging/trees/manager.py | Centralizes aggregate publication, branch removal, global clear, and successor task launch decisions. | | src/free_claude_code/messaging/trees/processor.py | Runs tasks by opaque claim ID and routes cleanup through manager-owned finish handling. | | src/free_claude_code/messaging/trees/snapshot.py | Serializes scoped tree snapshots and restores supported legacy tree shapes. | | src/free_claude_code/messaging/voice.py | Scopes pending voice registrations by message scope and message ID. | </details> <!-- greptile_failed_comments --> <h3>Comments Outside Diff (1)</h3> 1. `src/free_claude_code/messaging/commands.py`, line 161-164 ([link](https://github.com/alishahryar1/free-claude-code/blob/a4e49f082a9d3ffc1f6cdf3ab6a065b0f22a759d/src/free_claude_code/messaging/commands.py#L161-L164)) <a href="#"><img alt="P1" src="https://greptile-static-assets.s3.amazonaws.com/badges/p1.svg?v=9" align="top"></a> **Voice Cancellation Is Unscoped** The tree reply path is now scoped by `incoming.scope`, but the voice fallback still cancels by only `chat_id` and `reply_id`. If Discord and Telegram both have the same raw chat/message IDs, a reply `/clear` from one platform can cancel a pending voice note from the other platform, crossing the ownership boundary this PR adds for message trees. <details><summary><strong>Artifacts</strong></summary><br /> **[Repro: focused pytest harness that models cross-platform raw ID collision for voice cancellation](https://app.greptile.com/trex/artifacts/0ad480a9-4e20-4b4e-970e-63aebc4577b3)** - Contains supporting evidence from the run (text/x-python; charset=utf-8). **[Repro: verbose pytest output showing unscoped cancel\_pending\_voice call and Discord-owned voice cancellation from Telegram /clear](https://app.greptile.com/trex/artifacts/ead824f3-2f41-4028-a8e3-8359c5f6d3a9)** - Keeps the command output available without making the summary code-heavy. <a href="https://app.greptile.com/trex/runs/14082218/artifacts?artifact=0ad480a9-4e20-4b4e-970e-63aebc4577b3"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewArtifactsDark.svg?v=4"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/ViewArtifacts.svg?v=4"><img alt="View artifacts" src="https://greptile-static-assets.s3.amazonaws.com/badges/ViewArtifacts.svg?v=4"></picture></a> </details> <sub><a href="https://www.greptile.com/trex"><img alt="T-Rex" src="https://greptile-static-assets.s3.amazonaws.com/trex/trex_green.svg" height="14" align="absmiddle"></a> Ran code and verified through T-Rex</sub> <a href="https://app.greptile.com/api/ide/codex?prompt=IMPORTANT%3A%20Work%20in%20the%20repository%20%22alishahryar1%2Ffree-claude-code%22%20on%20the%20existing%20branch%20%22architecture%2Fmessage-tree-ownership%22.%20Checkout%20that%20branch%20%E2%80%94%20do%20NOT%20create%20a%20new%20branch%20or%20open%20a%20new%20PR.%20Push%20your%20changes%20to%20%22architecture%2Fmessage-tree-ownership%22.%0A%0AThis%20is%20a%20comment%20left%20during%20a%20code%20review.%0APath%3A%20src%2Ffree_claude_code%2Fmessaging%2Fcommands.py%0ALine%3A%20161-164%0A%0AComment%3A%0A**Voice%20Cancellation%20Is%20Unscoped**%0A%0AThe%20tree%20reply%20path%20is%20now%20scoped%20by%20%60incoming.scope%60%2C%20but%20the%20voice%20fallback%20still%20cancels%20by%20only%20%60chat_id%60%20and%20%60reply_id%60.%20If%20Discord%20and%20Telegram%20both%20have%20the%20same%20raw%20chat%2Fmessage%20IDs%2C%20a%20reply%20%60%2Fclear%60%20from%20one%20platform%20can%20cancel%20a%20pending%20voice%20note%20from%20the%20other%20platform%2C%20crossing%20the%20ownership%20boundary%20this%20PR%20adds%20for%20message%20trees.%0A%0AHow%20can%20I%20resolve%20this%3F%20If%20you%20propose%20a%20fix%2C%20please%20make%20it%20concise.&repo=alishahryar1%2Ffree-claude-code&pr=1048&platform=github"><picture><source media="(prefers-color-scheme: dark)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixInCodexDark.svg?v=6"><source media="(prefers-color-scheme: light)" srcset="https://greptile-static-assets.s3.amazonaws.com/badges/FixInCodex.svg?v=6"><img alt="Fix in Codex" src="https://greptile-static-assets.s3.amazonaws.com/badges/FixInCodex.svg?v=6"></picture></a> <!-- /greptile_failed_comments --> <sub>Reviews (4): Last reviewed commit: ["Serialize successor task publication wit..."](https://github.com/alishahryar1/free-claude-code/commit/ec8744a8882c1ebc38d5dc7e87aaf2e31b08f653) | [Re-trigger Greptile](https://app.greptile.com/api/retrigger?id=43492190)</sub> <!-- /greptile_comment -->
164 行
5.5 KiB
Python
164 行
5.5 KiB
Python
import asyncio
|
|
from unittest.mock import AsyncMock, MagicMock
|
|
|
|
import pytest
|
|
|
|
from free_claude_code.messaging.models import MessageScope
|
|
from free_claude_code.messaging.trees import MessageState
|
|
from free_claude_code.messaging.workflow import MessagingWorkflow
|
|
|
|
_SCOPE = MessageScope(platform="telegram", chat_id="chat_1")
|
|
|
|
|
|
@pytest.fixture
|
|
def handler_integration(mock_platform, mock_cli_manager, mock_session_store):
|
|
return MessagingWorkflow(mock_platform, mock_cli_manager, mock_session_store)
|
|
|
|
|
|
async def _events(events):
|
|
for event in events:
|
|
yield event
|
|
|
|
|
|
async def _wait_for_idle(handler: MessagingWorkflow) -> None:
|
|
for _ in range(100):
|
|
if handler.tree_queue.task_count() == 0:
|
|
return
|
|
await asyncio.sleep(0)
|
|
raise AssertionError("messaging claims did not finish")
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_full_conversation_flow_single_user(
|
|
handler_integration,
|
|
mock_platform,
|
|
mock_cli_manager,
|
|
incoming_message_factory,
|
|
) -> None:
|
|
mock_platform.queue_send_message = AsyncMock(side_effect=["s1", "s2"])
|
|
root_session = MagicMock()
|
|
root_session.start_task.return_value = _events(
|
|
[
|
|
{"type": "session_info", "session_id": "sess1"},
|
|
{
|
|
"type": "assistant",
|
|
"message": {"content": [{"type": "text", "text": "Reply 1"}]},
|
|
},
|
|
{"type": "exit", "code": 0, "stderr": None},
|
|
]
|
|
)
|
|
reply_session = MagicMock()
|
|
reply_session.start_task.return_value = _events(
|
|
[
|
|
{"type": "session_info", "session_id": "sess2"},
|
|
{
|
|
"type": "assistant",
|
|
"message": {"content": [{"type": "text", "text": "Reply 2"}]},
|
|
},
|
|
{"type": "exit", "code": 0, "stderr": None},
|
|
]
|
|
)
|
|
mock_cli_manager.get_or_create_session.side_effect = [
|
|
(root_session, "pending_1", True),
|
|
(reply_session, "pending_2", True),
|
|
]
|
|
|
|
await handler_integration.handle_message(
|
|
incoming_message_factory(text="message 1", message_id="m1")
|
|
)
|
|
await _wait_for_idle(handler_integration)
|
|
root = await handler_integration.tree_queue.get_node(_SCOPE, "m1")
|
|
assert root is not None
|
|
assert root.state is MessageState.COMPLETED
|
|
assert root.session_id == "sess1"
|
|
|
|
await handler_integration.handle_message(
|
|
incoming_message_factory(
|
|
text="message 2",
|
|
message_id="m2",
|
|
reply_to_message_id="m1",
|
|
)
|
|
)
|
|
await _wait_for_idle(handler_integration)
|
|
reply = await handler_integration.tree_queue.get_node(_SCOPE, "m2")
|
|
assert reply is not None
|
|
assert reply.state is MessageState.COMPLETED
|
|
assert reply.parent_id == "m1"
|
|
mock_cli_manager.get_or_create_session.assert_called_with(session_id="sess1")
|
|
reply_session.start_task.assert_called_with(
|
|
"message 2", session_id="sess1", fork_session=True
|
|
)
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_error_propagation_chain(
|
|
handler_integration,
|
|
mock_platform,
|
|
mock_cli_manager,
|
|
incoming_message_factory,
|
|
) -> None:
|
|
started = asyncio.Event()
|
|
release_error = asyncio.Event()
|
|
|
|
async def failing_events():
|
|
started.set()
|
|
await release_error.wait()
|
|
yield {"type": "error", "error": {"message": "failed"}}
|
|
|
|
session = MagicMock()
|
|
session.start_task.return_value = failing_events()
|
|
mock_cli_manager.get_or_create_session.return_value = (session, "sess1", False)
|
|
mock_platform.queue_send_message = AsyncMock(side_effect=["s1", "s2"])
|
|
|
|
await handler_integration.handle_message(
|
|
incoming_message_factory(text="m1", message_id="m1")
|
|
)
|
|
await started.wait()
|
|
await handler_integration.handle_message(
|
|
incoming_message_factory(text="m2", message_id="m2", reply_to_message_id="m1")
|
|
)
|
|
release_error.set()
|
|
await _wait_for_idle(handler_integration)
|
|
|
|
root = await handler_integration.tree_queue.get_node(_SCOPE, "m1")
|
|
child = await handler_integration.tree_queue.get_node(_SCOPE, "m2")
|
|
assert root is not None and root.state is MessageState.ERROR
|
|
assert child is not None and child.state is MessageState.ERROR
|
|
rendered = "\n".join(
|
|
call.args[2] for call in mock_platform.queue_edit_message.call_args_list
|
|
)
|
|
assert "Parent task failed" in rendered
|
|
|
|
|
|
@pytest.mark.asyncio
|
|
async def test_different_trees_process_independently(
|
|
handler_integration,
|
|
mock_platform,
|
|
mock_cli_manager,
|
|
incoming_message_factory,
|
|
) -> None:
|
|
session_one = MagicMock()
|
|
session_one.start_task.return_value = _events([{"type": "exit", "code": 0}])
|
|
session_two = MagicMock()
|
|
session_two.start_task.return_value = _events([{"type": "exit", "code": 0}])
|
|
mock_cli_manager.get_or_create_session.side_effect = [
|
|
(session_one, "s1", False),
|
|
(session_two, "s2", False),
|
|
]
|
|
mock_platform.queue_send_message = AsyncMock(side_effect=["status-t1", "status-t2"])
|
|
|
|
await asyncio.gather(
|
|
handler_integration.handle_message(
|
|
incoming_message_factory(text="t1", message_id="t1")
|
|
),
|
|
handler_integration.handle_message(
|
|
incoming_message_factory(text="t2", message_id="t2")
|
|
),
|
|
)
|
|
await _wait_for_idle(handler_integration)
|
|
|
|
node_one = await handler_integration.tree_queue.get_node(_SCOPE, "t1")
|
|
node_two = await handler_integration.tree_queue.get_node(_SCOPE, "t2")
|
|
assert node_one is not None and node_one.state is MessageState.COMPLETED
|
|
assert node_two is not None and node_two.state is MessageState.COMPLETED
|