fix(orchestrator): atomic workflow completion + canonical reopen gate#106
Open
fix(orchestrator): atomic workflow completion + canonical reopen gate#106
Conversation
map-check used `jq` to set `current_state="WORKFLOW_COMPLETE"` directly on step_state.json. The mutation skipped `current_step_phase`, which stayed on "ACTOR" from the last subtask's edit. `reopen_for_fixes` then refused the post-/map-review transition because it gated on `current_step_phase != "COMPLETE"`. - Add `mark_workflow_complete(branch)` orchestrator API that atomically sets `workflow_status`, `current_step_id`, `current_step_phase`, and a new `completed_at` field; refuses while `pending_steps` is non-empty. - Replace the `jq` mutation in map-check/SKILL.md with the new API. - Make `reopen_for_fixes` accept the canonical `workflow_status == "WORKFLOW_COMPLETE"` signal (with fallback to legacy fields so existing state files marked via the old `jq` path can still be reopened). - Regression + unit tests cover the stale-phase scenario and the full mark → reopen lifecycle.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces an orchestrator-level, atomic “workflow complete” transition and updates /map-check to use it, while making reopen_for_fixes accept the canonical completion signal (workflow_status == "WORKFLOW_COMPLETE") with legacy fallbacks to recover from partially-mutated state.
Changes:
- Added
mark_workflow_complete(branch)to atomically set completion fields and refuse completion whenpending_stepsis non-empty. - Updated
reopen_for_fixesgating to rely on canonicalworkflow_status(with fallbacks for legacy/corrupted state files). - Updated
/map-checkskill docs to prohibit direct state mutation and call the new orchestrator command; added/updated tests for these behaviors.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
src/mapify_cli/templates/map/scripts/map_orchestrator.py |
Adds completed_at, mark_workflow_complete, and canonical completion detection used by reopen_for_fixes. |
.map/scripts/map_orchestrator.py |
Mirrors the same orchestrator changes in the source-of-truth script copy. |
tests/test_map_orchestrator.py |
Adds coverage for atomic completion, completed_at persistence, and reopen gating regression. |
src/mapify_cli/templates/skills/map-check/SKILL.md |
Switches from jq state mutation to calling mark_workflow_complete; adds explicit warning. |
.claude/skills/map-check/SKILL.md |
Mirrors the same /map-check documentation updates for the Claude skill source. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| state.workflow_status = "WORKFLOW_COMPLETE" | ||
| state.current_step_id = "COMPLETE" | ||
| state.current_step_phase = "COMPLETE" | ||
| state.completed_at = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") |
Comment on lines
+1310
to
1316
| if not _is_workflow_complete(state): | ||
| return { | ||
| "status": "error", | ||
| "message": ( | ||
| f"Workflow is in phase '{state.current_step_phase}', not COMPLETE. " | ||
| f"Workflow is in phase '{state.current_step_phase}' " | ||
| f"(workflow_status='{state.workflow_status}'), not COMPLETE. " | ||
| "Use monitor_failed for non-COMPLETE retry." |
| state.workflow_status = "WORKFLOW_COMPLETE" | ||
| state.current_step_id = "COMPLETE" | ||
| state.current_step_phase = "COMPLETE" | ||
| state.completed_at = datetime.now(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ") |
Comment on lines
+1310
to
1316
| if not _is_workflow_complete(state): | ||
| return { | ||
| "status": "error", | ||
| "message": ( | ||
| f"Workflow is in phase '{state.current_step_phase}', not COMPLETE. " | ||
| f"Workflow is in phase '{state.current_step_phase}' " | ||
| f"(workflow_status='{state.workflow_status}'), not COMPLETE. " | ||
| "Use monitor_failed for non-COMPLETE retry." |
- mark_workflow_complete: use shared _utc_timestamp() helper instead of hand-rolled strftime to keep RFC3339 formatting consistent with contract_ready_subtasks[*].ready_at and other timestamped fields. - reopen_for_fixes: reset workflow_status="IN_PROGRESS" and clear completed_at when reopening. Without this, post-reopen state still reads workflow_status="WORKFLOW_COMPLETE" while pending_steps and current_step_phase=ACTOR are reset — exactly the kind of partial transition this PR was opened to eliminate. Adds regression test.
Owner
Author
|
@copilot-pull-request-reviewer please re-review — addressed the two findings from your previous pass in afc5ec4 (use |
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
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.
Summary
mark_workflow_complete(branch)— new orchestrator API that atomically setsworkflow_status,current_step_id,current_step_phase, and a newcompleted_atfield; refuses whilepending_stepsis non-empty.map-check/SKILL.md: replaced the historicaljq '.current_state = "WORKFLOW_COMPLETE"'mutation with the new API + explicit "never edit state directly" warning.reopen_for_fixesnow gates on the canonical signalworkflow_status == "WORKFLOW_COMPLETE"(with fallback to legacycurrent_step_id/current_step_phaseso already-corrupted state files can still be reopened).Why
A user hit this on a real branch:
/map-checkfinalised the workflow viajq, which silently bypassedcurrent_step_phase. The field stayed on"ACTOR"from the last subtask's Actor cycle. When/map-reviewlater triedpython3 .map/scripts/map_orchestrator.py reopen_for_fixes, it was refused with"Workflow is in phase 'ACTOR', not COMPLETE"even though the workflow was clearly complete (workflow_status = "WORKFLOW_COMPLETE",current_step_id = "COMPLETE").Two architectural rules from
architecture-patterns.mdapply:Test plan
tests/test_map_orchestrator.py— newTestMarkWorkflowCompleteclass (5 tests): atomic transition, reject when pending, no state file,completed_atround-trip, end-to-endmark → reopen.test_accepts_canonical_workflow_status_with_stale_phasereproduces the STACKLAND-1591 case (stalecurrent_step_phase="ACTOR"+ canonicalworkflow_status="WORKFLOW_COMPLETE"→ reopen succeeds).test_rejects_non_complete_phase→test_rejects_in_progress_workflowto match the new contract.ruff check src/ tests/— clean.mypy src/— clean.pytest --ignore=tests/integration/test_e2e_claude_sdk.py— 1051 passed, 2 skipped (SDK e2e excluded; needs API key, unrelated).make sync-templates—.claude/andsrc/mapify_cli/templates/are byte-identical (verified viadiff).