feat: container signature status/progress + mageflow.astatus#135
Merged
Conversation
Add ContainerStatus (per-container breakdown with terminal-state percentage) and ContainersStatus (aggregate wrapper) to the signature status module, plus a NotAContainerError for the status lookups. Refs #134 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add an abstract container_status() to ContainerTaskSignature. Swarm derives counts from its bookkeeping lists; chain classifies children by their task_status. Refs #134 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add astatus(*ids) that loads signatures, verifies each is a container (raising MissingSignatureError / NotAContainerError otherwise) and returns their ContainersStatus. Exposed as HatchetMageflow.astatus and re-exported from the mageflow package. Refs #134 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Add unit tests for swarm/chain container_status (terminal percentage, done, empty) and for astatus (single, aggregate, and the missing / non-container raise paths). Refs #134 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds structured progress reporting for swarm and chain container signatures, concurrent multi-container status aggregation, new error handling, and Mageflow/Hatchet API exports with unit coverage. ChangesContainer status feature
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Mageflow
participant thirdmagic.status
participant Rapyer
participant ContainerTaskSignature
Mageflow->>thirdmagic.status: astatus(*signature_ids)
thirdmagic.status->>Rapyer: Load signatures
Rapyer-->>thirdmagic.status: Return signatures
thirdmagic.status->>ContainerTaskSignature: container_status()
ContainerTaskSignature-->>thirdmagic.status: Return ContainerStatus
thirdmagic.status-->>Mageflow: Return ContainersStatus
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@libs/mageflow/tests/unit/workflows/test_astatus.py`:
- Around line 11-33: Extend test_astatus_returns_status_for_single_container to
assert the running count is 1 and the completion flag is false, matching the
fixture’s current_running=1 and unfinished task state. Keep the existing status
assertions unchanged.
In `@libs/third-magic/thirdmagic/swarm/model.py`:
- Around line 190-201: Update the swarm task-management logic around add_tasks()
and is_swarm_done() to reject duplicate signature keys for new additions and
compare task-key multiplicities so persisted duplicates cannot report completion
early; preserve accurate total, finished, and percentage status values. In
libs/third-magic/thirdmagic/swarm/model.py lines 190-201, apply the completion
fix through the existing container_status() flow; in
libs/third-magic/tests/unit/test_container_status.py lines 35-52, add regression
coverage verifying duplicate additions are rejected or completion remains false
until every duplicate instance is completed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 231431a9-d7b1-4432-9706-09e223294d8b
📒 Files selected for processing (11)
libs/mageflow/mageflow/__init__.pylibs/mageflow/mageflow/clients/hatchet/mageflow.pylibs/mageflow/tests/unit/workflows/test_astatus.pylibs/third-magic/tests/unit/test_container_status.pylibs/third-magic/thirdmagic/chain/model.pylibs/third-magic/thirdmagic/container.pylibs/third-magic/thirdmagic/errors.pylibs/third-magic/thirdmagic/signature/__init__.pylibs/third-magic/thirdmagic/signature/status.pylibs/third-magic/thirdmagic/status.pylibs/third-magic/thirdmagic/swarm/model.py
Comment on lines
+11
to
+33
| async def test_astatus_returns_status_for_single_container(mock_adapter): | ||
| # Arrange | ||
| setup = await create_swarm_item_test_setup( | ||
| num_tasks=4, | ||
| stop_after_n_failures=None, | ||
| current_running=1, | ||
| tasks_left_indices=[3], | ||
| finished_indices=[0], | ||
| failed_indices=[1], | ||
| ) | ||
|
|
||
| # Act | ||
| result = await mageflow.astatus(setup.swarm_task.key) | ||
|
|
||
| # Assert | ||
| assert isinstance(result, ContainersStatus) | ||
| status = result.containers[0] | ||
| assert status.signature_id == setup.swarm_task.key | ||
| assert status.total == 4 | ||
| assert status.finished == 1 | ||
| assert status.failed == 1 | ||
| assert status.pending == 1 | ||
| assert status.percentage == 50.0 |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert the running and completion fields too.
This fixture deliberately creates one running task, but the test never verifies status.running or status.is_done. A regression that drops either field could still pass the current assertions.
Proposed test additions
assert status.failed == 1
+ assert status.running == 1
assert status.pending == 1
assert status.percentage == 50.0
+ assert status.is_done is FalseAs per path instructions, tests should cover contract completeness and edge cases.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| async def test_astatus_returns_status_for_single_container(mock_adapter): | |
| # Arrange | |
| setup = await create_swarm_item_test_setup( | |
| num_tasks=4, | |
| stop_after_n_failures=None, | |
| current_running=1, | |
| tasks_left_indices=[3], | |
| finished_indices=[0], | |
| failed_indices=[1], | |
| ) | |
| # Act | |
| result = await mageflow.astatus(setup.swarm_task.key) | |
| # Assert | |
| assert isinstance(result, ContainersStatus) | |
| status = result.containers[0] | |
| assert status.signature_id == setup.swarm_task.key | |
| assert status.total == 4 | |
| assert status.finished == 1 | |
| assert status.failed == 1 | |
| assert status.pending == 1 | |
| assert status.percentage == 50.0 | |
| async def test_astatus_returns_status_for_single_container(mock_adapter): | |
| # Arrange | |
| setup = await create_swarm_item_test_setup( | |
| num_tasks=4, | |
| stop_after_n_failures=None, | |
| current_running=1, | |
| tasks_left_indices=[3], | |
| finished_indices=[0], | |
| failed_indices=[1], | |
| ) | |
| # Act | |
| result = await mageflow.astatus(setup.swarm_task.key) | |
| # Assert | |
| assert isinstance(result, ContainersStatus) | |
| status = result.containers[0] | |
| assert status.signature_id == setup.swarm_task.key | |
| assert status.total == 4 | |
| assert status.finished == 1 | |
| assert status.failed == 1 | |
| assert status.running == 1 | |
| assert status.pending == 1 | |
| assert status.percentage == 50.0 | |
| assert status.is_done is False |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/mageflow/tests/unit/workflows/test_astatus.py` around lines 11 - 33,
Extend test_astatus_returns_status_for_single_container to assert the running
count is 1 and the completion flag is false, matching the fixture’s
current_running=1 and unfinished task state. Keep the existing status assertions
unchanged.
Source: Path instructions
Comment on lines
+190
to
+201
| async def container_status(self) -> ContainerStatus: | ||
| return ContainerStatus.from_counts( | ||
| signature_id=self.key, | ||
| task_name=self.task_name, | ||
| status=self.task_status.status, | ||
| total=len(self.tasks), | ||
| finished=len(self.finished_tasks), | ||
| failed=len(self.failed_tasks), | ||
| running=self.current_running_tasks, | ||
| pending=len(self.tasks_left_to_run), | ||
| is_done=await self.is_swarm_done(), | ||
| ) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Prevent duplicate task keys from reporting completion early.
add_tasks() permits the same signature key more than once, while is_swarm_done() compares sets. A closed swarm containing [A, A] reports is_done=True after one completion, even though this status returns total=2, finished=1, and percentage=50.0.
libs/third-magic/thirdmagic/swarm/model.py#L190-L201: preserve task-key uniqueness when adding tasks and make completion multiplicity-aware for existing persisted duplicates.libs/third-magic/tests/unit/test_container_status.py#L35-L52: add a duplicate-key regression case that verifies duplicate additions are rejected or cannot report completion early.
As per path instructions, **/tests/**: “Focus on test coverage completeness and edge cases.”
📍 Affects 2 files
libs/third-magic/thirdmagic/swarm/model.py#L190-L201(this comment)libs/third-magic/tests/unit/test_container_status.py#L35-L52
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@libs/third-magic/thirdmagic/swarm/model.py` around lines 190 - 201, Update
the swarm task-management logic around add_tasks() and is_swarm_done() to reject
duplicate signature keys for new additions and compare task-key multiplicities
so persisted duplicates cannot report completion early; preserve accurate total,
finished, and percentage status values. In
libs/third-magic/thirdmagic/swarm/model.py lines 190-201, apply the completion
fix through the existing container_status() flow; in
libs/third-magic/tests/unit/test_container_status.py lines 35-52, add regression
coverage verifying duplicate additions are rejected or completion remains false
until every duplicate instance is completed.
Source: Path instructions
Collaborator
Author
There was a problem hiding this comment.
the bug is with the add_task, not here, and will be fixed in the short future
Contributor
There was a problem hiding this comment.
@yedidyakfir Understood—the root cause is duplicate admission in add_task(). The current is_swarm_done() set comparison still makes persisted duplicates complete early, so I’ll leave this finding open until that follow-up lands. Would you like me to create a GitHub issue to track it?
Replace the per-id asyncio.gather of afind_one with one batch rapyer.afind, and short-circuit on empty input so an empty id list never triggers a full-DB extraction. Missing keys are detected via a length check and still raise MissingSignatureError. Refs #134 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The per-container progress method is now astatus() on ContainerTaskSignature (and swarm/chain), matching the client-facing mageflow.astatus naming. Refs #134 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ContainerStatus and ContainersStatus are now imported from thirdmagic (top-level and thirdmagic.signature). The mageflow package only re-exports the astatus function, not the result types. Refs #134 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Build the entire expected ContainerStatus / ContainersStatus in the arrange step and compare with equality, instead of asserting individual fields. Refs #134 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Closes #134
Summary
Adds the ability to inspect how far along a container signature (Swarm / Chain) is, plus a client function to query several containers at once.
ContainerStatus— structured per-container breakdown:total,finished,failed,running,pending,percentage,status,is_done. Percentage is terminal-state:(finished + failed) / total, guarded fortotal == 0.ContainersStatus— wrapslist[ContainerStatus]with an aggregateoverall_percentage.container_status()— abstract onContainerTaskSignature; swarm derives from its bookkeeping lists, chain classifies children bytask_status.mageflow.astatus(*ids)— loads signatures, raisesMissingSignatureError/NotAContainerErrorwhen appropriate, returns aContainersStatus. Exposed asHatchetMageflow.astatusand re-exported from the package.Commits
NotAContainerErrorcontainer_status()for swarm and chainastatusaggregator + client method + exportsTesting
New unit tests cover swarm/chain breakdowns (terminal percentage, done, empty), multi-container aggregation, and the missing / non-container raise paths.
third-magic: 134 passedmageflow: 261 passed🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests