feat(map-efficient): behavior-neutral parallel-wave foundation (#303 Slices 0–4)#304
Merged
Conversation
…off (part of #303 Slice 0)
…ol compat, absent->off (part of #303 Slice 0)
…enforcement, lint.auto_prune, observability.parallelism) (part of #303 Slice 0)
…ath behavior-neutrality (part of #303 Slice 0)
…plicate-edge, always on) (part of #303 Slice 1)
…alized/redundant, warn-only) (part of #303 Slice 1)
…en isolation off) (part of #303 Slice 2)
…l) + orphan cleanup (dormant when off) (part of #303 Slice 2)
…l (concurrency off) (part of #303 Slice 3) Scope correction: gated the EXISTING orchestrator wave-loop (get_wave_step/advance_wave already existed) rather than duplicating it in the runner. Added select_execution_strategy (wave_mode + color-group concurrency predicate) and WAVE_CONCURRENCY_ENABLED=False signal.
…stable reason codes, no-op) (part of #303 Slice 4)
…nti-signals, add gated concurrent example + decision table (part of #303 Slice 4)
…token leak; make check green (part of #303 Slices 0-4)
|
Warning Review limit reached
Next review available in: 23 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (17)
📝 WalkthroughWalkthroughAdds wave-mode-aware orchestration, worktree isolation flow, dependency linting, parallelism observability scaffolding, updated skill docs/templates, and matching fixtures/tests. ChangesWave Execution Strategy and Worktree Isolation
Sequence Diagram(s)sequenceDiagram
participant map_orchestrator
participant map_step_runner
participant git
participant step_state_json
map_orchestrator->>map_step_runner: _execution_wave_mode(project_dir)
map_orchestrator->>step_state_json: read execution_waves
map_orchestrator->>map_orchestrator: select_execution_strategy()
map_step_runner->>git: probe worktree support / status
map_step_runner->>map_step_runner: resolve_worktree_isolation()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
🚥 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}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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 128-146: The execution strategy table is inconsistent with
`select_execution_strategy()`: the `wave_mode="on"` with width-1 case should not
be labeled as wave-loop if the code still returns "sequential". Update the
`execution.wave_mode` decision row in `select_execution_strategy`’s documented
behavior so it matches the actual branch for `has_parallel_groups == False`, and
keep the guidance aligned with the `get_next_step` vs `get_wave_step` cursor
separation. Make the same wording correction in the mirrored
`efficient-reference.md` copies under `.agents/skills/map-efficient` and
`src/mapify_cli/templates/codex/skills/map-efficient`.
In `@src/mapify_cli/dependency_graph.py`:
- Around line 687-689: The guard in dependency_graph.py’s thin-edge check
contradicts the comment and still allows false positives when only one node has
io data. Update the conditional in the relevant dependency analysis logic so it
skips the edge when either side is missing io data, matching the intent
described near the `a_io`/`b_io` check and preventing `thin_edge` from being set
without full input/output evidence.
In
`@src/mapify_cli/templates_src/codex/skills/map-efficient/efficient-reference.md.jinja`:
- Around line 101-107: Update the execution-strategy table in
efficient-reference.md.jinja to match select_execution_strategy(): for
wave_mode="on" with only width-1 color groups, it should still resolve to the
legacy sequential walker unless the dispatcher explicitly switches to wave_loop.
Adjust the affected row(s) so the documented mapping for wave_mode="on" aligns
with get_next_step vs get_wave_step / validate_wave_step / advance_wave and
avoids claiming wave-loop behavior when no group has width >= 2.
In `@src/mapify_cli/templates_src/map/scripts/map_orchestrator.py.jinja`:
- Around line 2352-2354: `get_wave_step()` currently adds `concurrency_enabled`
only in the active-wave path, so the no-wave and already-complete early returns
return a different shape. Update the `get_wave_step` template in
`map_orchestrator.py.jinja` so every return branch includes
`concurrency_enabled`, using the same `WAVE_CONCURRENCY_ENABLED` value (or an
explicit false equivalent if appropriate) in the early exits as well as the main
branch, then re-render the templates.
In `@src/mapify_cli/templates_src/map/scripts/map_step_runner.py.jinja`:
- Around line 15508-15512: The worktree probe cache in the probe/result flow is
treating transient `git worktree add` failures as permanent unsupported state.
Update the logic around `_WORKTREE_PROBE_CACHE`, the `git worktree add`
handling, and the cached miss rewrite path so only successful probes are
memoized, or keep a separate create-failure status instead of mapping it to
`worktree_unsupported`. Ensure `auto` can recover after a transient failure and
`required` does not keep aborting once the repo is fixed.
- Around line 15747-15776: The managed-worktree filter in the worktree
enumeration logic is too loose because it uses a substring check on the path
string, which can misclassify unrelated worktrees as managed. Update the
candidate collection in the git worktree scan to use real resolved-path ancestry
against the storage root before adding entries to candidate_paths, and keep the
existing _wt_force_remove flow only for paths confirmed to live under the
managed storage directory.
In
`@src/mapify_cli/templates_src/skills/map-efficient/efficient-reference.md.jinja`:
- Around line 128-136: The `execution.wave_mode="on"` case for color groups of
size 1 is inconsistent with `select_execution_strategy()`. Update the table and
surrounding text in the `efficient-reference.md.jinja` template so the `on` +
all groups width-1 row maps to the sequential dispatcher, matching the logic in
`select_execution_strategy()` and the related `wave_loop`/`sequential` behavior.
In `@tests/test_map_orchestrator.py`:
- Around line 4647-4653: The select_execution_strategy test setup is overwriting
sys.modules["map_step_runner"] and then unconditionally removing it, which can
erase a preexisting module imported by other tests. Update the affected blocks
in test_map_orchestrator.py to save the previous sys.modules["map_step_runner"]
value before assigning fake_runner_off/fake_runner_on/etc., and restore that
original entry in the finally block instead of always popping it. Use the same
save/restore pattern around each temporary map_step_runner injection in the
select_execution_strategy cases.
In `@tests/test_wave_eval_harness.py`:
- Around line 18-29: The test module import setup leaves SCRIPTS_PATH inserted
on sys.path for the whole pytest session, so import order affects later imports
of map_step_runner and map_utils. Move the import logic in
tests/test_wave_eval_harness.py to a local helper or fixture using importlib,
and make sure sys.path is restored after loading the runner so the path mutation
is scoped to these tests only.
In `@tests/test_wave_fixtures.py`:
- Around line 107-117: The non_git_dir verification in
test_vc1_all_fixtures_loadable_non_git_dir unconditionally invokes git
rev-parse, which breaks the test when git is not installed. Add a
git-availability guard in this test (using the same skip behavior used elsewhere
in the fixture module) before running the subprocess, so the non_git_dir
assertion only executes when git is available.
🪄 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: 5a259894-24d5-4242-82ce-54f56c46ce95
📒 Files selected for processing (24)
.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/dependency_graph.pysrc/mapify_cli/parallelism_observability.pysrc/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/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/fixtures/wave_blueprints.pytests/test_dependency_graph.pytests/test_map_orchestrator.pytests/test_map_step_runner.pytests/test_parallelism_observability.pytests/test_wave_eval_harness.pytests/test_wave_fixtures.py
Comment on lines
+18
to
+29
| SCRIPTS_PATH = ( | ||
| Path(__file__).resolve().parents[1] | ||
| / "src" | ||
| / "mapify_cli" | ||
| / "templates" | ||
| / "map" | ||
| / "scripts" | ||
| ) | ||
|
|
||
| sys.path.insert(0, str(SCRIPTS_PATH)) | ||
|
|
||
| import map_step_runner # noqa: E402 # type: ignore[import-not-found] |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Avoid leaving the generated scripts path on sys.path.
This runs at module import time and persists for the rest of the pytest process, so later imports of map_step_runner/map_utils depend on collection order instead of test-local setup. Load the runner via a local importlib helper or a fixture that restores sys.path after these tests.
🤖 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 18 - 29, The test module import
setup leaves SCRIPTS_PATH inserted on sys.path for the whole pytest session, so
import order affects later imports of map_step_runner and map_utils. Move the
import logic in tests/test_wave_eval_harness.py to a local helper or fixture
using importlib, and make sure sys.path is restored after loading the runner so
the path mutation is scoped to these tests only.
… run - impl: behavior-neutral foundation (default-config proof test); shared .jinja must not leak provider tokens into codex variant - error: verify decomposer 'X does not exist' claims via grep before create-new subtasks; aggregate lint gate is not substitutable by targeted per-file checks - arch: reserve-parameter forward-compat to avoid caller-rewrite cascades across subtasks
- dependency_graph: thin_edge guard now requires BOTH nodes to have io data (was OR, contradicted comment) — fixes false positives - orchestrator: get_wave_step includes concurrency_enabled in all return branches (shape consistency) - runner: worktree probe no longer memoizes transient failures (auto/required can recover in-session) - runner: orphan cleanup uses resolved-path ancestry, not substring match (prevents misclassifying+deleting unrelated worktrees) - docs: execution-strategy table: wave_mode=on + all-width-1 maps to legacy sequential walker (matches select_execution_strategy) - tests: save/restore real map_step_runner in sys.modules; git-availability guard on non_git_dir test
Owner
Author
|
Addressed CodeRabbit review in Fixed (code):
Fixed (docs): execution-strategy table — Fixed (tests): save/restore the real Skipped —
|
…lice 0 with #305 #305 landed #303 Slice 0 (worktree_isolation enum + execution_wave_mode) in MapConfig while #304 was in flight. Reconcile per 'MapConfig = single source': - Adopt main's _wt_isolation_enabled (auto->False until Slice 5); keep _worktree_isolation_mode as the runner-side enum reader for probe/fallback. - Align _execution_wave_mode default off->auto to match MapConfig; behavior stays neutral via the isolation gate. - select_execution_strategy now gates wave-loop on worktree.isolation != off (mirrors #305): default (wave_mode=auto, isolation=off) -> legacy sequential. - Update decision tables (claude+codex) + neutrality tests for the new defaults. make check green (3019 passed); render parity clean.
Owner
Author
|
Reconciled with #305 (merged mid-flight). #305 landed #303 Slice 0 ( Merged
This PR remains the Slices 1–4 layer (dep-graph lint, worktree probe/fallback/cleanup, predicate-gated wave-loop, doc surgery, dormant observability) on top of #305's Slice 0.
|
This was referenced Jun 29, 2026
Merged
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.
What
Behavior-neutral foundation for activating parallel-wave execution in
/map-efficient— Slices 0–4 of the llm-council rollout in #303. Every new config key defaults to today's behavior; no concurrent dispatch is added and no default is flipped. The legacy sequential walker remains the working default path.Closes part of #303 (Slices 5–7 — concurrent dispatch, default flips, decomposer tuning — deferred).
Slices
execution.wave_mode(off|auto|on),worktree.isolationwidened to enumoff|auto|required(legacy bool compat: false→off, true→required; absent→off; no migration), lint/observability toggles (all no-op defaults). Canned wave/repo-shape fixtures + eval/regression test asserting wave+color computation and that defaults select the sequential path.warn, auto-prune opt-in false.git worktree add --detachprobe (try/finally, cached),require_clean_merge_target, fallback matrix (auto→sequential+warning / required→hard-fail) with stable reason codes, idempotent orphan cleanup. Dormant (zero git) when isolation off.select_execution_strategygates the existing orchestrator wave-loop onexecution.wave_mode ∈ {on,auto}AND a color group of ≥2 (predicate keys on color-group concurrency, not raw wave count).WAVE_CONCURRENCY_ENABLED=False→ sequential-inside even formode=="parallel"waves.concurrency_enabled/parallel_ready; dormantparallelism.jsonschema + stable reason codes (contract-first, parity-tested against the runner constants).Behavior-neutrality (adversarially verified by final-verifier)
Default config →
_execution_wave_mode="off",_worktree_isolation_mode="off",select_execution_strategy="sequential",concurrency_enabled=False; worktree probe/fallback/cleanup dormant with zero git;lint_dependency_graphis a pure function with no live caller yet.Notes
get_wave_step/validate_wave_step/advance_wave) already existed in the orchestrator — this PR gates them rather than duplicating into the runner.templates_src/**/*.jinjaviamake render-templates.Verification
make check— 3005 passed, 3 skipped; ruff / mypy / pyright 0/0/0make check-render— generated trees matchtemplates_srcSummary by CodeRabbit
New Features
execution.wave_modeand parallel-ready groups.Bug Fixes
Documentation