feat(map-efficient): #303 Slice 6 — flip parallel defaults ON + MAP_EFFICIENT_SEQUENTIAL_ONLY kill-switch#309
Merged
Merged
Conversation
…ON + kill-switch Flips the two default-OFF gates so concurrent wave execution is on by default: - worktree.isolation: off -> auto - execution.concurrent_dispatch: false -> true (wave_mode already auto). 'auto' degrades to sequential when not-a-git-repo / dirty / worktree-unsupported. Off-ramps (the user-requested disable option): - MAP_EFFICIENT_SEQUENTIAL_ONLY=1 — global kill-switch, forces the legacy sequential walker (no wave-loop, no worktrees, no concurrent dispatch), checked FIRST in select_execution_strategy + compute_dispatch_gate before any config read; byte-identical to pre-5a legacy. - per-repo config: worktree.isolation: off / execution.concurrent_dispatch: false. The HC-1 behavior-neutrality / byte-identical-legacy invariant moves from 'default config' to 'kill-switch engaged' — the 5b leak guards now protect the off-ramp. Fixed _worktree_isolation_mode falsy-set so an explicit false/off/0/no still maps to off after the default became auto. Migration: the flip changes behavior only where the key is ABSENT; explicit opt-outs are preserved. make check 3224 passed; kill-switch + opt-out + default-on all proven by tests.
📝 WalkthroughWalkthroughSlice 6 flips ChangesSlice 6: Concurrent dispatch and worktree isolation ON by default + env kill-switch
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
tests/test_wave_eval_harness.py (1)
150-233: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winAssert the dispatch decision, not just its prerequisites.
These tests now describe “wave-loop engaged by default” and “kill-switch forces sequential,” but they only check reader defaults plus blueprint shape. If
select_execution_strategy()orcompute_dispatch_gate()regresses, both tests still pass. Add one orchestrator-level assertion here, or narrow the test names/docs to the reader-level invariant.🤖 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_eval_harness.py` around lines 150 - 233, The tests currently verify only config readers and blueprint structure, not the actual dispatch outcome, so a regression in select_execution_strategy() or compute_dispatch_gate() could still pass. Update test_default_config_has_parallel_defaults and test_kill_switch_forces_sequential to assert the orchestrator-level decision directly (for the relevant plan/state), or else rename/reword them to clearly scope them to _execution_wave_mode and _worktree_isolation_mode only.tests/test_map_orchestrator.py (1)
4861-4885: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winClear the kill-switch env in non-kill-switch tests.
These tests assert default-on / per-repo behavior, but they inherit
MAP_EFFICIENT_SEQUENTIAL_ONLYfrom the outside environment. Since the kill-switch short-circuits before config reads, a developer or CI shell with it set will either fail these assertions or exercise the wrong path.Suggested local fix pattern
monkeypatch.chdir(tmp_path) +monkeypatch.delenv("MAP_EFFICIENT_SEQUENTIAL_ONLY", raising=False)Also applies to: 5171-5178, 5325-5338
🤖 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_map_orchestrator.py` around lines 4861 - 4885, The strategy-selection tests in select_execution_strategy are accidentally depending on MAP_EFFICIENT_SEQUENTIAL_ONLY being unset, so they can hit the kill-switch path and bypass the intended config-driven behavior. Update the affected test cases around map_orchestrator.select_execution_strategy to explicitly clear or override MAP_EFFICIENT_SEQUENTIAL_ONLY before asserting default-on/per-repo behavior, and restore the environment afterward so the tests are isolated from the outer shell or CI environment.
🧹 Nitpick comments (1)
tests/test_project_config.py (1)
388-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLock down the generated config defaults too.
These tests cover
MapConfig()andload_map_config()defaults, but thegenerate_default_config()flip is still effectively unguarded:tests/test_worktree_isolation.pyonly checks for the substringworktree.isolation: off, which still appears in the new off-ramp prose. Please add an assertion for the actual example defaults (worktree.isolation: auto, and ideallyexecution.concurrent_dispatch: true) so this docs/template contract can’t silently drift.🤖 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 388 - 421, The default config generation path is not fully covered, so `generate_default_config()` can drift without failing tests. Update the relevant tests around `tests/test_worktree_isolation.py` to assert the actual example defaults emitted by `generate_default_config()`, specifically that the generated config includes `worktree.isolation: auto` and ideally `execution.concurrent_dispatch: true`, rather than only matching the old `off` substring. Use the existing `generate_default_config` and `worktree.isolation`/`execution.concurrent_dispatch` symbols to anchor the assertions.
🤖 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 `@docs/USAGE.md`:
- Around line 233-235: The global kill-switch wording is inconsistent across the
docs, with one place describing the fallback as pre-Slice-6 and another as
pre-Slice-5a legacy. Update the kill-switch description in the USAGE
documentation to use the same rollback baseline term as the rest of the repo,
and keep the wording consistent with the related `MAP_EFFICIENT_SEQUENTIAL_ONLY`
explanation and the matching references in `docs/ARCHITECTURE.md` and
`CHANGELOG.md`.
In `@src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja`:
- Around line 155-164: The step-runner guard in _concurrent_dispatch_enabled
currently only consults config, so run_concurrent_wave and related entry points
can bypass the global off-ramp. Add an early MAP_EFFICIENT_SEQUENTIAL_ONLY check
in _concurrent_dispatch_enabled before calling _map_config_str, and return False
immediately when it is set; keep the config lookup as the fallback for normal
behavior. Apply the same guard consistently anywhere this helper is used so
direct CLI/coordinator paths cannot re-enable concurrent dispatch.
In `@tests/test_map_orchestrator.py`:
- Around line 5282-5287: Clear the MAP_EFFICIENT_SEQUENTIAL_ONLY kill-switch
before this per-repo isolation test so the assertions exercise the
worktree.isolation opt-out path rather than the global disable path. Update the
test around the create_subtask_worktree setup to explicitly unset or isolate
that environment state before writing .map/config.yaml and invoking the relevant
worktree logic, using the existing test helpers and variables in
test_map_orchestrator.py to keep the scenario focused on worktree.isolation:
off.
- Around line 4918-4937: The kill-switch test currently only checks the final
sequential outcome, so it can still pass even if config is read. Update the fake
runner in test_map_orchestrator by changing the _wm and _iso hooks inside
_fake_runner_parallel to raise if they are invoked, so the test explicitly
verifies the no-config-read path before the map_step_runner logic is consulted.
---
Outside diff comments:
In `@tests/test_map_orchestrator.py`:
- Around line 4861-4885: The strategy-selection tests in
select_execution_strategy are accidentally depending on
MAP_EFFICIENT_SEQUENTIAL_ONLY being unset, so they can hit the kill-switch path
and bypass the intended config-driven behavior. Update the affected test cases
around map_orchestrator.select_execution_strategy to explicitly clear or
override MAP_EFFICIENT_SEQUENTIAL_ONLY before asserting default-on/per-repo
behavior, and restore the environment afterward so the tests are isolated from
the outer shell or CI environment.
In `@tests/test_wave_eval_harness.py`:
- Around line 150-233: The tests currently verify only config readers and
blueprint structure, not the actual dispatch outcome, so a regression in
select_execution_strategy() or compute_dispatch_gate() could still pass. Update
test_default_config_has_parallel_defaults and test_kill_switch_forces_sequential
to assert the orchestrator-level decision directly (for the relevant
plan/state), or else rename/reword them to clearly scope them to
_execution_wave_mode and _worktree_isolation_mode only.
---
Nitpick comments:
In `@tests/test_project_config.py`:
- Around line 388-421: The default config generation path is not fully covered,
so `generate_default_config()` can drift without failing tests. Update the
relevant tests around `tests/test_worktree_isolation.py` to assert the actual
example defaults emitted by `generate_default_config()`, specifically that the
generated config includes `worktree.isolation: auto` and ideally
`execution.concurrent_dispatch: true`, rather than only matching the old `off`
substring. Use the existing `generate_default_config` and
`worktree.isolation`/`execution.concurrent_dispatch` symbols to anchor the
assertions.
🪄 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: 19e42508-0307-4254-9e4d-d990c1342213
📒 Files selected for processing (16)
.map/scripts/map_orchestrator.py.map/scripts/map_step_runner.pyCHANGELOG.mddocs/ARCHITECTURE.mddocs/USAGE.mdsrc/mapify_cli/config/project_config.pysrc/mapify_cli/templates/map/scripts/map_orchestrator.pysrc/mapify_cli/templates/map/scripts/map_step_runner.pysrc/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinjasrc/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinjatests/test_map_orchestrator.pytests/test_map_step_runner.pytests/test_project_config.pytests/test_slice5b_leak_guards.pytests/test_wave_eval_harness.pytests/test_worktree_isolation.py
Comment on lines
+233
to
+235
| 1. **Global kill-switch** — set `MAP_EFFICIENT_SEQUENTIAL_ONLY=1` in your shell. | ||
| Forces the full legacy sequential path, byte-identical to pre-Slice-6, regardless | ||
| of any config. Unset to restore default parallel behavior. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Align the kill-switch baseline wording.
Line 234 says the env kill-switch is byte-identical to pre-Slice-6, but Lines 332-334 in this same file describe the same path as pre-Slice-5a legacy. docs/ARCHITECTURE.md and CHANGELOG.md use the pre-5a wording too, so this currently gives readers two different rollback baselines for one contract.
🤖 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 `@docs/USAGE.md` around lines 233 - 235, The global kill-switch wording is
inconsistent across the docs, with one place describing the fallback as
pre-Slice-6 and another as pre-Slice-5a legacy. Update the kill-switch
description in the USAGE documentation to use the same rollback baseline term as
the rest of the repo, and keep the wording consistent with the related
`MAP_EFFICIENT_SEQUENTIAL_ONLY` explanation and the matching references in
`docs/ARCHITECTURE.md` and `CHANGELOG.md`.
Comment on lines
155
to
164
| def _concurrent_dispatch_enabled(project_dir: Path) -> bool: | ||
| """Return True when execution.concurrent_dispatch is explicitly enabled. | ||
| """Return True when execution.concurrent_dispatch is enabled (default ON, Slice 6). | ||
|
|
||
| Mirrors the _wt_isolation_enabled pattern. Default is False (off) so the | ||
| sequential path stays byte-identical to Slice 5a by default (HC-1). Never | ||
| raises. | ||
| Default is True — Slice 6 flipped from False. Disable via | ||
| MAP_EFFICIENT_SEQUENTIAL_ONLY=1 (global kill-switch) or set | ||
| `execution.concurrent_dispatch: false` in .map/config.yaml. | ||
| Mirrors the canonical MapConfig default (config/project_config.py). Never raises. | ||
| """ | ||
| raw = _map_config_str(project_dir, "execution.concurrent_dispatch", "false") | ||
| raw = _map_config_str(project_dir, "execution.concurrent_dispatch", "true") | ||
| return raw.strip().lower() in _CONCURRENT_DISPATCH_TRUTHY |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Honor the kill-switch in the step-runner guard.
run_concurrent_wave() can still proceed under MAP_EFFICIENT_SEQUENTIAL_ONLY=1 because _concurrent_dispatch_enabled() only reads config. Add the env check before config reads so direct CLI/coordinator calls cannot bypass the global off-ramp.
Proposed fix
+_SEQUENTIAL_ONLY_TRUTHY = frozenset({"1", "true", "yes", "y", "on"})
+
+
+def _sequential_only_env() -> bool:
+ """Return True when MAP_EFFICIENT_SEQUENTIAL_ONLY is set to a truthy value."""
+ import os as _os
+
+ return (
+ _os.environ.get("MAP_EFFICIENT_SEQUENTIAL_ONLY", "").strip().lower()
+ in _SEQUENTIAL_ONLY_TRUTHY
+ )
+
+
def _concurrent_dispatch_enabled(project_dir: Path) -> bool:
"""Return True when execution.concurrent_dispatch is enabled (default ON, Slice 6).
@@
`execution.concurrent_dispatch: false` in .map/config.yaml.
Mirrors the canonical MapConfig default (config/project_config.py). Never raises.
"""
+ if _sequential_only_env():
+ return False
raw = _map_config_str(project_dir, "execution.concurrent_dispatch", "true")
return raw.strip().lower() in _CONCURRENT_DISPATCH_TRUTHYAlso applies to: 17477-17483, 17551-17562
🤖 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_step_runner.py.jinja` around
lines 155 - 164, The step-runner guard in _concurrent_dispatch_enabled currently
only consults config, so run_concurrent_wave and related entry points can bypass
the global off-ramp. Add an early MAP_EFFICIENT_SEQUENTIAL_ONLY check in
_concurrent_dispatch_enabled before calling _map_config_str, and return False
immediately when it is set; keep the config lookup as the fallback for normal
behavior. Apply the same guard consistently anywhere this helper is used so
direct CLI/coordinator paths cannot re-enable concurrent dispatch.
Comment on lines
+4918
to
+4937
| def _fake_runner_parallel(wave_mode: str, isolation: str) -> "types.ModuleType": | ||
| mod = types.ModuleType("map_step_runner") | ||
|
|
||
| def _wm(_project_dir: object) -> str: | ||
| del _project_dir | ||
| return wave_mode | ||
|
|
||
| def _iso(_project_dir: object) -> str: | ||
| del _project_dir | ||
| return isolation | ||
|
|
||
| mod._execution_wave_mode = _wm # type: ignore[attr-defined] | ||
| mod._worktree_isolation_mode = _iso # type: ignore[attr-defined] | ||
| return mod | ||
|
|
||
| _orig_msr = _sys.modules.get("map_step_runner") | ||
| try: | ||
| # Even with parallel-ready defaults (wave_mode=auto, isolation=auto), | ||
| # the kill-switch short-circuits to sequential before reading config. | ||
| _sys.modules["map_step_runner"] = _fake_runner_parallel("auto", "auto") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make the kill-switch short-circuit test fail if config is read.
The test comment says the kill-switch fires before reading config, but the fake runner returns normal values. Have those hooks raise if called so this test proves the no-config-read invariant, not just the final sequential result.
Suggested test hardening
- def _fake_runner_parallel(wave_mode: str, isolation: str) -> "types.ModuleType":
+ def _fake_runner_parallel() -> "types.ModuleType":
mod = types.ModuleType("map_step_runner")
- def _wm(_project_dir: object) -> str:
- del _project_dir
- return wave_mode
+ def _must_not_read_config(_project_dir: object) -> str:
+ raise AssertionError("kill-switch must short-circuit before map_step_runner config reads")
- def _iso(_project_dir: object) -> str:
- del _project_dir
- return isolation
-
- mod._execution_wave_mode = _wm # type: ignore[attr-defined]
- mod._worktree_isolation_mode = _iso # type: ignore[attr-defined]
+ mod._execution_wave_mode = _must_not_read_config # type: ignore[attr-defined]
+ mod._worktree_isolation_mode = _must_not_read_config # type: ignore[attr-defined]
return mod🤖 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_map_orchestrator.py` around lines 4918 - 4937, The kill-switch
test currently only checks the final sequential outcome, so it can still pass
even if config is read. Update the fake runner in test_map_orchestrator by
changing the _wm and _iso hooks inside _fake_runner_parallel to raise if they
are invoked, so the test explicitly verifies the no-config-read path before the
map_step_runner logic is consulted.
Comment on lines
+5282
to
+5287
| # create_subtask_worktree with per-repo isolation=off must still no-op. | ||
| # This covers the per-repo opt-out path (separate from kill-switch). | ||
| # Write a config with worktree.isolation: off explicitly. | ||
| map_dir = tmp_path / ".map" | ||
| map_dir.mkdir(parents=True, exist_ok=True) | ||
| (map_dir / "config.yaml").write_text("worktree.isolation: off\n", encoding="utf-8") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Clear the kill-switch before proving the per-repo isolation opt-out.
This block says it covers worktree.isolation: off separately, but MAP_EFFICIENT_SEQUENTIAL_ONLY is still set from Line 5258. If worktree creation is disabled by the global kill-switch, these assertions can pass without proving the config opt-out path.
Suggested fix
# create_subtask_worktree with per-repo isolation=off must still no-op.
# This covers the per-repo opt-out path (separate from kill-switch).
+ monkeypatch.delenv("MAP_EFFICIENT_SEQUENTIAL_ONLY", raising=False)
# Write a config with worktree.isolation: off explicitly.📝 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
| # create_subtask_worktree with per-repo isolation=off must still no-op. | |
| # This covers the per-repo opt-out path (separate from kill-switch). | |
| # Write a config with worktree.isolation: off explicitly. | |
| map_dir = tmp_path / ".map" | |
| map_dir.mkdir(parents=True, exist_ok=True) | |
| (map_dir / "config.yaml").write_text("worktree.isolation: off\n", encoding="utf-8") | |
| # create_subtask_worktree with per-repo isolation=off must still no-op. | |
| # This covers the per-repo opt-out path (separate from kill-switch). | |
| monkeypatch.delenv("MAP_EFFICIENT_SEQUENTIAL_ONLY", raising=False) | |
| # Write a config with worktree.isolation: off explicitly. | |
| map_dir = tmp_path / ".map" | |
| map_dir.mkdir(parents=True, exist_ok=True) | |
| (map_dir / "config.yaml").write_text("worktree.isolation: off\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_map_orchestrator.py` around lines 5282 - 5287, Clear the
MAP_EFFICIENT_SEQUENTIAL_ONLY kill-switch before this per-repo isolation test so
the assertions exercise the worktree.isolation opt-out path rather than the
global disable path. Update the test around the create_subtask_worktree setup to
explicitly unset or isolate that environment state before writing
.map/config.yaml and invoking the relevant worktree logic, using the existing
test helpers and variables in test_map_orchestrator.py to keep the scenario
focused on worktree.isolation: off.
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 6 — flip the parallel-execution defaults ON, with a global kill-switch. Concurrent wave execution is now the default for
/map-efficient; opt-out is one env var or a per-repo config key.Done at explicit user direction (the council-recommended 2–4 week soak was waived by the operator); the kill-switch is the required off-ramp.
Defaults flipped (2)
worktree.isolationoffautoexecution.concurrent_dispatchfalsetrue(
execution.wave_modewas alreadyauto.)autodegrades to sequential when not-a-git-repo / dirty / worktree-unsupported — never hard-fails.Off-ramps (the disable option)
MAP_EFFICIENT_SEQUENTIAL_ONLY=1— global kill-switch. Forces the legacy sequential walker (no wave-loop, no worktrees, no concurrent dispatch), checked first inselect_execution_strategy+compute_dispatch_gatebefore any config read. Byte-identical to pre-5a legacy.worktree.isolation: offand/orexecution.concurrent_dispatch: false.Why this is safe
auto, an explicitworktree.isolation: false/off/0/nostill maps tooff(regression guard added).Verification
make check→ 3224 passed, render parity clean, ruff + pyright 0/0/0.isolation=auto, concurrent=True; explicitfalse→off/False; kill-switch → sequential.Rollout note
This is the final flip of #303. Operators can revert instantly via the kill-switch or per-repo keys. No shadow-mode.
Part of #303 (builds on 5a #306, 5b #308).
Summary by CodeRabbit
New Features
Bug Fixes
Documentation