feat(map-efficient): #303 Slice 5a — auto-worktree wave-loop, sequential dispatch (flag-gated)#306
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThis PR implements the Slice 5a wave concurrency infrastructure: adds a read-only ChangesSlice 5a Wave Dispatch Signals, concurrency_ready Gate, and Dormant Config Fields
Sequence Diagram(s)sequenceDiagram
participant Coordinator as Coordinator (Orchestrator)
participant get_wave_step
participant concurrency_ready
participant WorktreeRegistry
participant GitWorktreeList
Coordinator->>get_wave_step: get_wave_step(branch)
get_wave_step-->>Coordinator: {dispatch_mode, isolation_active, reason, concurrency_enabled}
Note over Coordinator: isolation_active=True → Slice 5a sequential loop
loop per wave member (one per turn)
Coordinator->>Coordinator: dispatch Actor subagent pinned to $WT_PATH
end
Coordinator->>concurrency_ready: concurrency_ready(subtask_ids, branch)
concurrency_ready->>WorktreeRegistry: lookup sidecar record per subtask
concurrency_ready->>GitWorktreeList: git worktree list --porcelain
concurrency_ready-->>Coordinator: {ready, per_subtask, reason}
alt ready=True
Coordinator->>Coordinator: merge_wave_worktrees (atomic)
else ready=False
Coordinator->>Coordinator: rollback / retry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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: 7
🧹 Nitpick comments (2)
tests/test_wave_parallel_harness.py (1)
57-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the new isolation enum in the harness.
This helper still enables isolation with legacy
true, so these tests don't exercise the newworktree.isolation in {auto,required}contract directly. Usingrequiredhere would make the harness validate the Slice 5a path instead of its backward-compat shim.Suggested change
(repo / ".map" / "config.yaml").write_text( - "worktree.isolation: true\nworktree.max_deletions: 50\n", + "worktree.isolation: required\nworktree.max_deletions: 50\n", encoding="utf-8", )🤖 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 `@tests/test_wave_parallel_harness.py` around lines 57 - 62, Update the test harness helper _enable so it uses the new worktree.isolation enum value instead of the legacy boolean true. Adjust the config written by _enable in tests/test_wave_parallel_harness.py to set worktree.isolation to required, so the harness exercises the new isolation contract directly and validates the Slice 5a path rather than the backward-compatibility shim.tests/test_project_config.py (1)
154-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMirror this dormant-consumer guard for
retry_degraded_once.This subprocess assertion only scans for
max_actors, so a new runner/orchestrator read ofretry_degraded_oncein Python sources would still pass VC3 even though the Slice 5a dormant contract applies to both fields.Suggested change
def test_vc3_grep_subprocess_confirms_no_runner_orchestrator_consumer(self): """Subprocess grep across runner/orchestrator/step_runner Python sources. @@ """ src_root = Path(__file__).parent.parent / "src" / "mapify_cli" - result = subprocess.run( - ["grep", "-rl", "max_actors", str(src_root)], - capture_output=True, - text=True, - ) - files_with_max_actors = [ - line for line in result.stdout.splitlines() if line.strip() - ] - # Runner/orchestrator Python source files are forbidden in Slice 5a. - # Documentation (.md, .jinja) and observability modules are allowed. - _FORBIDDEN_STEMS = ("runner", "orchestrator", "step_runner", "wave_coordinator") - forbidden = [ - f for f in files_with_max_actors - if any(stem in Path(f).stem for stem in _FORBIDDEN_STEMS) - and f.endswith(".py") - ] - assert forbidden == [], ( - "max_actors found in runner/orchestrator Python sources in Slice 5a " - "(DORMANT violation — field must not be consumed until Slice 5b):\n" - + "\n".join(forbidden) - ) + for field_name in ("max_actors", "retry_degraded_once"): + result = subprocess.run( + ["grep", "-rl", field_name, str(src_root)], + capture_output=True, + text=True, + ) + files_with_field = [ + line for line in result.stdout.splitlines() if line.strip() + ] + # Runner/orchestrator Python source files are forbidden in Slice 5a. + # Documentation (.md, .jinja) and observability modules are allowed. + _FORBIDDEN_STEMS = ( + "runner", + "orchestrator", + "step_runner", + "wave_coordinator", + ) + forbidden = [ + f for f in files_with_field + if any(stem in Path(f).stem for stem in _FORBIDDEN_STEMS) + and f.endswith(".py") + ] + assert forbidden == [], ( + f"{field_name} found in runner/orchestrator Python sources in Slice 5a " + "(DORMANT violation — field must not be consumed until Slice 5b):\n" + + "\n".join(forbidden) + )🤖 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 `@tests/test_project_config.py` around lines 154 - 183, Extend the dormant-consumer subprocess guard in test_vc3_grep_subprocess_confirms_no_runner_orchestrator_consumer to also scan for retry_degraded_once, not just max_actors. Update the grep/filter logic so any Python source under runner/orchestrator/step_runner/wave_coordinator that mentions retry_degraded_once is collected into the forbidden list, while keeping docs and observability modules allowed. Use the existing test structure and symbols like _FORBIDDEN_STEMS and files_with_max_actors as the place to mirror the check.
🤖 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 @.claude/skills/map-efficient/efficient-reference.md:
- Around line 665-672: Remove the Evaluator requirement from the Slice 5a merge
recipe so it matches the skill’s phase model. Update the wording around the
merge instructions in the affected section to require only Monitor before
calling merge_wave_worktrees, and keep the guidance consistent with
concurrency_ready, merge_subtask_worktree, and merge_wave_worktrees without
mentioning Evaluator in the normal path.
In @.map/scripts/map_step_runner.py:
- Around line 15345-15350: `concurrency_ready()` is treating a worktree as clean
even when `_wt_git(["status", "--porcelain"], cwd=wt_path)` fails, because an
empty stdout still falls through to the clean path. Update the worktree-status
handling to fail closed: in the status-check logic that sets `ok`/reason codes
(near `_WT_REASON_DIRTY` and related concurrency_ready worktree checks), require
a successful git exit before marking clean, and when the status command returns
non-zero mark the member not ready with the appropriate failure reason instead
of assuming cleanliness. Apply the same fix to the duplicated status-handling
block referenced by the second range.
In `@src/mapify_cli/templates_src/codex/skills/map-efficient/SKILL.md.jinja`:
- Around line 16-20: Add the missing `concurrency_ready` entry to the Script
Routing section so the workflow matches the earlier `isolation_active`
description. Update the routing table in `SKILL.md.jinja` to list
`concurrency_ready` alongside the other subcommands, keeping the wording
consistent with the existing routing entries and the wave-loop verification
flow.
In `@src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja`:
- Around line 2308-2314: The import handling around _worktree_isolation_mode in
map_orchestrator.py.jinja is too broad because the except ImportError also masks
failures raised inside map_step_runner itself. Narrow the fallback so only a
missing import of map_step_runner is treated as isolation_active=False, and let
nested ImportError exceptions propagate or otherwise be distinguished from the
import attempt. Keep the behavior localized to the try/except that wraps the
map_step_runner import and _worktree_isolation_mode lookup.
In
`@src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja`:
- Around line 151-160: Remove the mention of parallel_ready from the Slice 5b
gate text in the concurrent actor dispatch example, and align the wording with
the actual fields returned by get_wave_step; use only concurrency_enabled,
dispatch_mode, isolation_active, and reason so the guidance matches the real API
shape.
In `@src/mapify_cli/templates_src/skills/map-efficient/SKILL.md.jinja`:
- Line 210: Add the missing isolation condition to the execution-strategy
summary in SKILL.md.jinja: update the `select_execution_strategy` description so
the wave-loop (`get_wave_step` / `validate_wave_step` / `advance_wave`) only
activates when `execution.wave_mode ∈ {on, auto}`, a color group has at least
two members, and `worktree.isolation != off`. Keep the sequential fallback
(`get_next_step`) described as the default whenever that isolation gate is not
satisfied, so the summary matches the Slice 5a contract and decision table.
In `@src/mapify_cli/templates/map/scripts/map_step_runner.py`:
- Around line 16881-16886: The clean-tree check around `_wt_git(["status",
"--porcelain"], cwd=wt_path)` should fail closed instead of treating a failed
`git status` as clean. In the worktree readiness logic, check `st.returncode`
before building `dirty_lines`; if the status command fails for a registered
worktree, treat it as not clean and block the subtask rather than returning
ready. Keep the existing filtering via `_wt_porcelain_path` and
`_wt_is_runtime_state_path`, but make the readiness decision depend on a
successful `git status` result.
---
Nitpick comments:
In `@tests/test_project_config.py`:
- Around line 154-183: Extend the dormant-consumer subprocess guard in
test_vc3_grep_subprocess_confirms_no_runner_orchestrator_consumer to also scan
for retry_degraded_once, not just max_actors. Update the grep/filter logic so
any Python source under runner/orchestrator/step_runner/wave_coordinator that
mentions retry_degraded_once is collected into the forbidden list, while keeping
docs and observability modules allowed. Use the existing test structure and
symbols like _FORBIDDEN_STEMS and files_with_max_actors as the place to mirror
the check.
In `@tests/test_wave_parallel_harness.py`:
- Around line 57-62: Update the test harness helper _enable so it uses the new
worktree.isolation enum value instead of the legacy boolean true. Adjust the
config written by _enable in tests/test_wave_parallel_harness.py to set
worktree.isolation to required, so the harness exercises the new isolation
contract directly and validates the Slice 5a path rather than the
backward-compatibility shim.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c834d347-a828-4de4-adbb-154005758393
📒 Files selected for processing (23)
.agents/skills/map-efficient/SKILL.md.agents/skills/map-efficient/efficient-reference.md.claude/skills/map-efficient/SKILL.md.claude/skills/map-efficient/efficient-reference.md.map/scripts/map_orchestrator.py.map/scripts/map_step_runner.pysrc/mapify_cli/config/project_config.pysrc/mapify_cli/templates/codex/skills/map-efficient/SKILL.mdsrc/mapify_cli/templates/codex/skills/map-efficient/efficient-reference.mdsrc/mapify_cli/templates/map/scripts/map_orchestrator.pysrc/mapify_cli/templates/map/scripts/map_step_runner.pysrc/mapify_cli/templates/skills/map-efficient/SKILL.mdsrc/mapify_cli/templates/skills/map-efficient/efficient-reference.mdsrc/mapify_cli/templates_src/codex/skills/map-efficient/SKILL.md.jinjasrc/mapify_cli/templates_src/codex/skills/map-efficient/efficient-reference.md.jinjasrc/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinjasrc/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinjasrc/mapify_cli/templates_src/skills/map-efficient/SKILL.md.jinjasrc/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinjatests/test_map_orchestrator.pytests/test_project_config.pytests/test_wave_parallel_harness.pytests/test_worktree_isolation.py
Comment on lines
+665
to
+672
| 3. **Verify** all member worktrees with `concurrency_ready` (ST-003) before merge. | ||
| 4. **Accept atomically** via `merge_wave_worktrees` after every subtask passes | ||
| Monitor (+ Evaluator) — never merge one at a time. | ||
|
|
||
| Do NOT merge them one at a time: every worktree was cut off the same HEAD, so | ||
| the first `merge_subtask_worktree` advances the working branch and the next trips | ||
| `BASE_DIVERGED`. Accept the whole wave atomically instead — only after EVERY | ||
| subtask in the wave has passed Monitor (+ Evaluator): |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Drop the Evaluator gate from this Slice 5a merge recipe.
map-efficient documents Monitor as the terminal validator in normal execution, so requiring Monitor (+ Evaluator) here contradicts the skill’s own phase model and blocks the wave-accept path on a phase operators never run.
🤖 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 @.claude/skills/map-efficient/efficient-reference.md around lines 665 - 672,
Remove the Evaluator requirement from the Slice 5a merge recipe so it matches
the skill’s phase model. Update the wording around the merge instructions in the
affected section to require only Monitor before calling merge_wave_worktrees,
and keep the guidance consistent with concurrency_ready, merge_subtask_worktree,
and merge_wave_worktrees without mentioning Evaluator in the normal path.
Comment on lines
+15345
to
+15350
| # concurrency_ready reason codes | ||
| _WT_REASON_NO_RECORD: str = "no_record" | ||
| _WT_REASON_PATH_MISSING: str = "path_missing" | ||
| _WT_REASON_NOT_REGISTERED: str = "not_registered" | ||
| _WT_REASON_HEAD_MISMATCH: str = "head_mismatch" | ||
| _WT_REASON_DIRTY: str = "dirty" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when git status cannot inspect a worktree.
If _wt_git(["status", "--porcelain"], cwd=wt_path) returns non-zero, st.stdout can still be empty, so this member is marked clean and ok=True. That makes concurrency_ready() report a broken worktree as merge-ready.
Proposed fix
# concurrency_ready reason codes
_WT_REASON_NO_RECORD: str = "no_record"
_WT_REASON_PATH_MISSING: str = "path_missing"
_WT_REASON_NOT_REGISTERED: str = "not_registered"
_WT_REASON_HEAD_MISMATCH: str = "head_mismatch"
_WT_REASON_DIRTY: str = "dirty"
+_WT_REASON_STATUS_ERROR: str = "status_error"
...
# (4) Clean tree (ignoring MAP runtime-state paths)
st = _wt_git(["status", "--porcelain"], cwd=wt_path)
+ if st.returncode != 0:
+ per_subtask[sid] = {
+ "ok": False,
+ "reason": _WT_REASON_STATUS_ERROR,
+ "path": raw_path,
+ }
+ if first_failure is None:
+ first_failure = _WT_REASON_STATUS_ERROR
+ continue
dirty_lines = [
ln
for ln in st.stdout.splitlines()
if ln.strip() and not _wt_is_runtime_state_path(_wt_porcelain_path(ln))
]Also applies to: 16881-16896
🤖 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 @.map/scripts/map_step_runner.py around lines 15345 - 15350,
`concurrency_ready()` is treating a worktree as clean even when
`_wt_git(["status", "--porcelain"], cwd=wt_path)` fails, because an empty stdout
still falls through to the clean path. Update the worktree-status handling to
fail closed: in the status-check logic that sets `ok`/reason codes (near
`_WT_REASON_DIRTY` and related concurrency_ready worktree checks), require a
successful git exit before marking clean, and when the status command returns
non-zero mark the member not ready with the appropriate failure reason instead
of assuming cleanliness. Apply the same fix to the duplicated status-handling
block referenced by the second range.
Comment on lines
+16
to
+20
| section when the workflow below points to it. Under `isolation_active` (Slice 5a), | ||
| the wave-loop creates per-member worktrees, dispatches actor subagents | ||
| **sequentially** (one per turn), verifies via `concurrency_ready`, then accepts | ||
| atomically via `merge_wave_worktrees`; concurrent fan-out is Slice 5b | ||
| (`dispatch_mode==concurrent`). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document concurrency_ready in the routing table too.
This paragraph now tells operators to verify via concurrency_ready, but the later Script Routing section still omits that subcommand. Adding it there will keep the workflow docs self-consistent.
🤖 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 `@src/mapify_cli/templates_src/codex/skills/map-efficient/SKILL.md.jinja`
around lines 16 - 20, Add the missing `concurrency_ready` entry to the Script
Routing section so the workflow matches the earlier `isolation_active`
description. Update the routing table in `SKILL.md.jinja` to list
`concurrency_ready` alongside the other subcommands, keeping the wording
consistent with the existing routing entries and the wave-loop verification
flow.
Comment on lines
+2308
to
+2314
| try: | ||
| from map_step_runner import ( # pyright: ignore[reportMissingImports] | ||
| _worktree_isolation_mode, | ||
| ) | ||
| isolation_active = _worktree_isolation_mode(Path(".")) != "off" | ||
| except ImportError: | ||
| isolation_active = False |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Don't swallow nested import failures here.
This except ImportError also catches failures inside map_step_runner, then reports isolation_active=False. That turns a broken render/package state into misleading sequential metadata instead of surfacing the real error.
Suggested change
try:
from map_step_runner import ( # pyright: ignore[reportMissingImports]
_worktree_isolation_mode,
)
- isolation_active = _worktree_isolation_mode(Path(".")) != "off"
- except ImportError:
+ except ModuleNotFoundError as exc:
+ if exc.name != "map_step_runner":
+ raise
isolation_active = False
+ else:
+ isolation_active = _worktree_isolation_mode(Path(".")) != "off"📝 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
| try: | |
| from map_step_runner import ( # pyright: ignore[reportMissingImports] | |
| _worktree_isolation_mode, | |
| ) | |
| isolation_active = _worktree_isolation_mode(Path(".")) != "off" | |
| except ImportError: | |
| isolation_active = False | |
| try: | |
| from map_step_runner import ( # pyright: ignore[reportMissingImports] | |
| _worktree_isolation_mode, | |
| ) | |
| except ModuleNotFoundError as exc: | |
| if exc.name != "map_step_runner": | |
| raise | |
| isolation_active = False | |
| else: | |
| isolation_active = _worktree_isolation_mode(Path(".")) != "off" |
🤖 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 `@src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja` around
lines 2308 - 2314, The import handling around _worktree_isolation_mode in
map_orchestrator.py.jinja is too broad because the except ImportError also masks
failures raised inside map_step_runner itself. Narrow the fallback so only a
missing import of map_step_runner is treated as isolation_active=False, and let
nested ImportError exceptions propagate or otherwise be distinguished from the
import attempt. Keep the behavior localized to the try/except that wraps the
map_step_runner import and _worktree_isolation_mode lookup.
Comment on lines
+151
to
+160
| ### Concurrent Actor dispatch — **Slice 5b only** (`dispatch_mode == 'concurrent'`) — GATED EXAMPLE | ||
|
|
||
| > **IMPORTANT — read before using this example.** | ||
| > Concurrent fan-out (emitting multiple `Task(actor)` calls in a single message) is | ||
| > enabled **only when concurrency is shipped: Slice 5+ / `concurrency_enabled: true` / | ||
| > `parallel_ready` flag set**. In the **current framework** `concurrency_enabled` is | ||
| > **False**, so dispatch stays **SEQUENTIAL even when a wave has `mode=="parallel"`**. | ||
| > The example below is reference material for when that capability ships; do NOT | ||
| > treat it as an active instruction now. | ||
| > **Slice 5b** and is enabled **only when `concurrency_enabled: true` / | ||
| > `parallel_ready` flag set / `dispatch_mode == 'concurrent'`**. In the **current | ||
| > framework** (`concurrency_enabled=False`, Slice 5a), dispatch stays **SEQUENTIAL | ||
| > even when a wave has `mode=="parallel"`** — one Actor per turn, each pinned to | ||
| > its own worktree. The example below is reference material for when Slice 5b ships; | ||
| > do NOT treat it as an active instruction now. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
for f in \
src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja \
src/mapify_cli/templates/map/scripts/map_orchestrator.py \
.map/scripts/map_orchestrator.py
do
if [ -f "$f" ]; then
echo "=== $f ==="
rg -n -C2 'select_execution_strategy|get_wave_step|dispatch_mode|isolation_active|reason|concurrency_allowed|concurrency_enabled|parallel_ready' "$f"
echo
fi
doneRepository: azalio/map-framework
Length of output: 32859
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '=== template excerpt ===\n'
sed -n '140,175p' src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja
printf '\n=== parallel_ready references ===\n'
rg -n 'parallel_ready|concurrency_enabled|dispatch_mode|isolation_active|concurrency_allowed' src/mapify_cli/templates_src src/mapify_cli/templates .claude .codex .agents 2>/dev/null || trueRepository: azalio/map-framework
Length of output: 22749
Drop parallel_ready from the Slice 5b gate text. get_wave_step exposes concurrency_enabled, dispatch_mode, isolation_active, and reason; parallel_ready is not part of the returned shape and will mislead readers.
🤖 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
`@src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja`
around lines 151 - 160, Remove the mention of parallel_ready from the Slice 5b
gate text in the concurrent actor dispatch example, and align the wording with
the actual fields returned by get_wave_step; use only concurrency_enabled,
dispatch_mode, isolation_active, and reason so the guidance matches the real API
shape.
| ``` | ||
|
|
||
| **Execution strategy:** `select_execution_strategy` chooses between the legacy sequential walker and the wave-loop. The wave-loop (`get_wave_step` / `validate_wave_step` / `advance_wave`) engages only when `execution.wave_mode ∈ {on, auto}` AND a color group has ≥2 members; otherwise `get_next_step` (sequential walker) runs. See [efficient-reference.md](efficient-reference.md#wave-execution) for the decision table and full wave loop. | ||
| **Execution strategy:** `select_execution_strategy` chooses between the legacy sequential walker and the wave-loop. The wave-loop (`get_wave_step` / `validate_wave_step` / `advance_wave`) engages only when `execution.wave_mode ∈ {on, auto}` AND a color group has ≥2 members; otherwise `get_next_step` (sequential walker) runs. Under `isolation_active` (Slice 5a), the wave-loop creates per-member worktrees, dispatches Actors **sequentially** (one per turn, `HC-3`), verifies via `concurrency_ready`, then accepts atomically via `merge_wave_worktrees`; concurrent fan-out is Slice 5b (`dispatch_mode==concurrent`). See [efficient-reference.md](efficient-reference.md#wave-execution) for the decision table and full wave loop. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Add the missing isolation gate to the execution-strategy summary.
Line 210 currently says the wave-loop engages on wave_mode + color-group size alone, but the Slice 5a contract and the reference table also require worktree.isolation != off. As written, this overstates when wave APIs are active and misdocuments the default path.
🤖 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 `@src/mapify_cli/templates_src/skills/map-efficient/SKILL.md.jinja` at line
210, Add the missing isolation condition to the execution-strategy summary in
SKILL.md.jinja: update the `select_execution_strategy` description so the
wave-loop (`get_wave_step` / `validate_wave_step` / `advance_wave`) only
activates when `execution.wave_mode ∈ {on, auto}`, a color group has at least
two members, and `worktree.isolation != off`. Keep the sequential fallback
(`get_next_step`) described as the default whenever that isolation gate is not
satisfied, so the summary matches the Slice 5a contract and decision table.
Comment on lines
+16881
to
+16886
| st = _wt_git(["status", "--porcelain"], cwd=wt_path) | ||
| dirty_lines = [ | ||
| ln | ||
| for ln in st.stdout.splitlines() | ||
| if ln.strip() and not _wt_is_runtime_state_path(_wt_porcelain_path(ln)) | ||
| ] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail closed when git status errors.
This clean-tree check ignores st.returncode. If git status --porcelain fails for a registered worktree, dirty_lines stays empty and the subtask can be reported ready even though cleanliness was never verified. As per coding guidelines, "All shipped templates must be edited in src/mapify_cli/templates_src/**/*.jinja and re-rendered with make render-templates; never edit the generated trees directly (src/mapify_cli/templates/**, .claude/**, .codex/**, .agents/skills/**)."
Suggested fix
+# add alongside the other _WT_REASON_* constants
+_WT_REASON_STATUS_FAILED: str = "status_failed"
+
st = _wt_git(["status", "--porcelain"], cwd=wt_path)
+ if st.returncode != 0:
+ per_subtask[sid] = {
+ "ok": False,
+ "reason": _WT_REASON_STATUS_FAILED,
+ }
+ if first_failure is None:
+ first_failure = _WT_REASON_STATUS_FAILED
+ continue
dirty_lines = [
ln
for ln in st.stdout.splitlines()
if ln.strip() and not _wt_is_runtime_state_path(_wt_porcelain_path(ln))
]📝 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
| st = _wt_git(["status", "--porcelain"], cwd=wt_path) | |
| dirty_lines = [ | |
| ln | |
| for ln in st.stdout.splitlines() | |
| if ln.strip() and not _wt_is_runtime_state_path(_wt_porcelain_path(ln)) | |
| ] | |
| st = _wt_git(["status", "--porcelain"], cwd=wt_path) | |
| if st.returncode != 0: | |
| per_subtask[sid] = { | |
| "ok": False, | |
| "reason": _WT_REASON_STATUS_FAILED, | |
| } | |
| if first_failure is None: | |
| first_failure = _WT_REASON_STATUS_FAILED | |
| continue | |
| dirty_lines = [ | |
| ln | |
| for ln in st.stdout.splitlines() | |
| if ln.strip() and not _wt_is_runtime_state_path(_wt_porcelain_path(ln)) | |
| ] |
🤖 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 `@src/mapify_cli/templates/map/scripts/map_step_runner.py` around lines 16881 -
16886, The clean-tree check around `_wt_git(["status", "--porcelain"],
cwd=wt_path)` should fail closed instead of treating a failed `git status` as
clean. In the worktree readiness logic, check `st.returncode` before building
`dirty_lines`; if the status command fails for a registered worktree, treat it
as not clean and block the subtask rather than returning ready. Keep the
existing filtering via `_wt_porcelain_path` and `_wt_is_runtime_state_path`, but
make the readiness decision depend on a successful `git status` result.
Source: Coding guidelines
This was referenced Jun 29, 2026
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.
Summary
#303 Slice 5a — wire
worktree.isolation in {auto,required}so the existing wave-loop creates per-color-group git worktrees and accepts each wave atomically viamerge_wave_worktrees, while dispatching Actors sequentially. This exercises real worktree creation + atomic whole-wave merge + whole-wave rollback at full fidelity without introducing concurrency.Behavior is byte-identical to today under the default
worktree.isolation=off(the legacy sequential walker runs). Concurrent N-Task fanout (Slice 5b) and the default flips (Slice 6) are explicitly out of scope and not present.Implemented per the llm-council deep verdict (conv
d5ff5181, tracked on #303), which recommended splitting the original Slice 5 into 5a (this PR) / 5b.What changed
concurrency_allowedderived field (pure function ofselect_execution_strategy) +select_execution_strategyCLI handler. Council Q1: the coordinator owns the "is concurrency permitted" signal.get_wave_stepreturns structureddispatch_mode/isolation_active/ stablereasoncode, replacing the bareWAVE_CONCURRENCY_ENABLEDecho.concurrency_enabledkept as a back-compat alias.dispatch_modeis always"sequential"in 5a (HC-3).concurrency_ready— read-only coordinator-owned wave-worktree readiness check (path registered +HEAD==base_sha+ clean), reusing existing_wt_*helpers. No skill-reported flag crosses the LLM trust boundary (council Q1).max_actors(default 4, clamp[1,8], bool-excluded) +retry_degraded_once(default false) config +clamp_max_actorshelper. Read by no 5a execution path.tests/test_wave_parallel_harness.pyfor the council Q6 merge landmines: orphan-cleanup-on-failure, generated-file conflict → whole-wave rollback (attribution names both subtasks), post-wave-gate failure → rollback inside the transaction, no-op classification, deterministic sorted-id merge order.Hard constraints (all proven by tests)
make check-renderclean.dispatch_modestays"sequential";WAVE_CONCURRENCY_ENABLEDstaysFalse.make checkgreen.Verification
make check→ 3057 passed, 3 skipped, render parity clean, ruff + pyright 0 errors.merge_wave_worktrees— the GSD-style fresh-context-per-task: worktree isolation for autonomous execution #284 atomic rollback holds against real git worktrees.Follow-ups (not in this PR)
dispatch_mode == 'concurrent'),record_dispatch_actualself-audit,max_actorsbatch-split wiring — after this slice's matrix is green.wave_mode/worktree.isolationdefaults after a soak. No shadow-mode rollout.Part of #303.
Summary by CodeRabbit
New Features
Documentation
Tests