refactor: rename map-planning to map-state, fix orchestrator cwd + sanitize, tighten Actor/Monitor#105
Merged
Conversation
…dination Five related fixes that all surfaced from one symptom: typing /map-plan in a project shipped with the previous skill bundle resolved to map-planning (branch-scoped state infra) instead of map-plan (ARCHITECT decomposition). 1. Skill collision: rename .claude/skills/map-planning -> .claude/skills/map-state and remove disable-model-invocation: true from map-plan/SKILL.md so the model sees both skills as distinct surfaces. Update skill-rules.json, monitor agents, docs (USAGE, INSTALL, roadmap, improvement-plan), and in-tree script comments. 2. map_orchestrator.py is now cwd-independent: anchors itself to the project root via Path(__file__).resolve().parents[2] before any state lookup. Previously, invoking the orchestrator via an absolute path from a different cwd silently read .map/<branch>/ from the caller's directory and returned misleading "step mismatch" errors. 3. Hardened map_step_runner._sanitize_for_json: previous regex preserved \t \n \r and relied on json.dumps to escape them, but bash command substitution does not preserve byte-perfect roundtrip in all locales — jq aborted with "Invalid string: control characters from U+0000 through U+001F must be escaped". The function now flattens newline variants to spaces and strips the entire \x00-\x1f\x7f range, so build_handoff_bundle output is robust through bash pipelines. 4. Block "pre-existing, unrelated" excuse for surfaced quality-gate failures: Monitor scope distinguishes pre-existing DORMANT tech debt (still OUT OF SCOPE) from pre-existing SURFACED failures (lint/type/test failing in the current run, regardless of whether the failing code predates the diff — must fix). Actor's QUICK REFERENCE and Subtask Intent now ban one-line "pre-existing, unrelated" dismissals; deferral requires explicit user approval. 5. Captured #3 and #4 as learned rules in .claude/rules/learned/error-patterns.md with WRONG/CORRECT examples so future agents do not reintroduce either regression. Templates synced via make sync-templates. CHANGELOG updated.
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a slash-skill naming collision in MAP, hardens workflow runtime behavior, and syncs the resulting skill/agent/documentation changes across the shipped .claude, .map, .codex, and template copies.
Changes:
- Renames the branch-scoped planning skill from
map-planningtomap-stateand makesmap-planmodel-invocable again. - Fixes workflow runtime behavior in the orchestrator and handoff bundle sanitization.
- Tightens Actor/Monitor guidance around surfaced quality-gate failures and updates docs/changelog/learned rules.
Reviewed changes
Copilot reviewed 24 out of 38 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
src/mapify_cli/templates/skills/skill-rules.json |
Renames template skill rule entry to map-state. |
src/mapify_cli/templates/skills/map-state/templates/task_plan.md |
Adds template task-plan scaffold for renamed skill. |
src/mapify_cli/templates/skills/map-state/templates/progress.md |
Adds progress-log template for renamed skill. |
src/mapify_cli/templates/skills/map-state/templates/iteration_history.md |
Adds iteration-history template for renamed skill. |
src/mapify_cli/templates/skills/map-state/templates/findings.md |
Adds findings template for renamed skill. |
src/mapify_cli/templates/skills/map-state/SKILL.md |
Renames skill frontmatter to map-state. |
src/mapify_cli/templates/skills/map-state/scripts/show-focus.sh |
Adds focus-display helper for renamed skill. |
src/mapify_cli/templates/skills/map-state/scripts/init-session.sh |
Adds session initialization helper for renamed skill. |
src/mapify_cli/templates/skills/map-state/scripts/get-plan-path.sh |
Updates usage/examples for renamed skill path. |
src/mapify_cli/templates/skills/map-state/scripts/check-complete.sh |
Adds stop-hook completion checker for renamed skill. |
src/mapify_cli/templates/skills/map-plan/SKILL.md |
Removes disable-model-invocation from map-plan. |
src/mapify_cli/templates/map/scripts/map_step_runner.py |
Hardens JSON sanitization for handoff bundles. |
src/mapify_cli/templates/map/scripts/map_orchestrator.py |
Anchors orchestrator cwd to project root in CLI mode. |
src/mapify_cli/templates/codex/agents/monitor.toml |
Updates Codex monitor guidance for surfaced gate failures and rename. |
src/mapify_cli/templates/agents/monitor.md |
Updates Claude monitor guidance for surfaced gate failures and rename. |
src/mapify_cli/templates/agents/actor.md |
Updates Actor guidance to require addressing surfaced gate failures. |
docs/USAGE.md |
Renames user-facing skill docs to map-state. |
docs/roadmap.md |
Updates roadmap reference to renamed skill path. |
docs/INSTALL.md |
Updates install tree to renamed skill directory. |
docs/improvement-plan.md |
Replaces planning-skill references with map-state. |
CHANGELOG.md |
Records rename and runtime hardening changes. |
.map/scripts/map_step_runner.py |
Source-of-truth step-runner sanitization change. |
.map/scripts/map_orchestrator.py |
Source-of-truth orchestrator cwd fix. |
.codex/agents/monitor.toml |
Source-of-truth Codex monitor guidance update. |
.claude/skills/skill-rules.json |
Source-of-truth skill rule rename to map-state. |
.claude/skills/map-state/templates/task_plan.md |
Source-of-truth task-plan template for renamed skill. |
.claude/skills/map-state/templates/progress.md |
Source-of-truth progress template for renamed skill. |
.claude/skills/map-state/templates/iteration_history.md |
Source-of-truth iteration-history template for renamed skill. |
.claude/skills/map-state/templates/findings.md |
Source-of-truth findings template for renamed skill. |
.claude/skills/map-state/SKILL.md |
Source-of-truth skill rename to map-state. |
.claude/skills/map-state/scripts/show-focus.sh |
Source-of-truth focus hook script. |
.claude/skills/map-state/scripts/init-session.sh |
Source-of-truth session init script. |
.claude/skills/map-state/scripts/get-plan-path.sh |
Source-of-truth renamed path helper docs. |
.claude/skills/map-state/scripts/check-complete.sh |
Source-of-truth stop-hook completion script. |
.claude/skills/map-plan/SKILL.md |
Source-of-truth map-plan visibility change. |
.claude/rules/learned/error-patterns.md |
Adds learned rules for sanitization and surfaced-failure handling. |
.claude/agents/monitor.md |
Source-of-truth Claude monitor guidance update. |
.claude/agents/actor.md |
Source-of-truth Actor guidance update. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1882
to
+1886
| # Anchor cwd to the project root before any state lookup or branch | ||
| # detection. The script lives at ``<project>/.map/scripts/map_orchestrator.py``, | ||
| # so ``parents[2]`` is the project root regardless of how the script was | ||
| # invoked. Without this chdir, an absolute-path call from a different cwd | ||
| # silently reads ``.map/<branch>/`` from the caller's directory and |
Comment on lines
+1375
to
+1377
| text = text.replace("\r\n", "\n").replace("\r", "\n") | ||
| text = text.replace("\n", " ").replace("\t", " ") | ||
| return re.sub(r"[\x00-\x1f\x7f]", "", text) |
…-anchor Addresses Copilot review feedback on PR #105: the two runtime fixes were landed without automated tests, so a regression could return without any CI failure. - TestSanitizeForJson (tests/test_map_step_runner.py): parametrised over the C0 control range and DEL, asserts whitespace flattening to spaces, preserves printable ASCII + Unicode, and verifies a full round-trip through json.dumps + json.loads with no raw control bytes leaking back. Closes the original "Sanitize Control Characters" rule's gap. - TestCwdIndependence (tests/test_map_orchestrator.py): subprocess-based test that copies the orchestrator + sibling .py modules into a tmp project_a, runs the script via absolute path from a foreign cwd project_b, and asserts state is read from project_a (not project_b). Covers both get_next_step and validate_step. The fix's reliance on Path(__file__).resolve().parents[2] is now guaranteed by an automated case — in-process tests cannot exercise it because they bypass main().
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 40 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+244
to
+245
| │ │ ├── map-state/ | ||
| │ │ │ └── SKILL.md # Branch-scoped planning state skill |
Comment on lines
+1882
to
+1888
| # Anchor cwd to the project root before any state lookup or branch | ||
| # detection. The script lives at ``<project>/.map/scripts/map_orchestrator.py``, | ||
| # so ``parents[2]`` is the project root regardless of how the script was | ||
| # invoked. Without this chdir, an absolute-path call from a different cwd | ||
| # silently reads ``.map/<branch>/`` from the caller's directory and | ||
| # returns misleading "step mismatch" errors. | ||
| os.chdir(Path(__file__).resolve().parents[2]) |
Two more findings from the second Copilot pass: 1. docs/INSTALL.md: the install-tree diagram still claimed a `.claude/commands/` directory with ten `map-*.md` files. That layout was removed long ago — every MAP slash surface now lives under `.claude/skills/<name>/SKILL.md`, and `tests/test_template_sync.py::test_no_map_command_files_remain` enforces the absence of `commands/map-*.md`. Replaced the stale tree with the actual shipped skill list (12 entries) and added a note pointing at the enforcing test. 2. tests/test_map_orchestrator.py::TestCwdIndependence: previous coverage exercised only `get_next_step` and `validate_step`. Added `test_set_waves_resolves_relative_blueprint_in_script_project` — subprocess test that seeds two competing blueprints (3-subtask vs 1-subtask), invokes `set_waves --blueprint .map/<branch>/blueprint.json` from a foreign cwd, and asserts the relative argument is rebased against the script's project. Confirms the cwd anchor protects every path-bearing CLI subcommand, not just the two we originally tested.
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 26 out of 40 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
.claude/skills/map-state/scripts/get-plan-path.sh:19
- These examples still document
.map/main/...as the non-git fallback, but the rest of the MAP runtime falls back to.map/default/...when git metadata is unavailable. That mismatch makes this renamed script's help text point users at the wrong path for--no-git/nongit workflows.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
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
Five fixes that all surfaced from one symptom: typing
/map-planresolved to themap-planningskill (branch-scoped state infra) instead of the realmap-plan(ARCHITECT decomposition). Investigating that collision exposed three more latent issues, all rolled in here.disable-model-invocation: trueonmap-planhid it from the model, so/map-planfuzzy-matched tomap-planning. Renamedmap-planning→map-state(kebab-case, no shared prefix) and unhidmap-plan.python3 /abs/path/.map/scripts/map_orchestrator.py validate_step "2.3"from a foreign cwd silently read.map/<branch>/from the caller's directory and returnedStep mismatch: expected 1.0, got 2.3. Now anchors viaPath(__file__).resolve().parents[2]._sanitize_for_jsontoo lax — previous regex preserved\t \n \rand relied onjson.dumpsto escape them, butBUNDLE=$(... build_handoff_bundle)followed byecho "$BUNDLE" | jqaborted withInvalid string: control characters from U+0000 through U+001F must be escaped. Hardened: flatten newline variants to spaces, strip entire\x00-\x1f\x7frange."Pre-existing failure unrelated to ST-XXX. Skipping."dismissals; deferral requires explicit user approval..claude/rules/learned/error-patterns.mdupdated with WRONG/CORRECT examples for docs: Remove non-functional Claude Marketplace installation documentation #3 and docs: Add comprehensive PATH setup instructions for mapify CLI installation #4 so future agents do not reintroduce either regression.Templates synced via
make sync-templates. CHANGELOG entry under[Unreleased].Test plan
make sync-templates— clean diff between.claude/+.codex/+.map/scripts/andsrc/mapify_cli/templates/pytest tests/test_skills.py tests/test_template_sync.py— 64 passedpytest tests/integration/test_e2e_artifact_contracts.py— 29 passed (orchestrator path resolution still correct under tmp_path)pytest tests/test_map_step_runner.py— 88 passed (sanitize hardening doesn't break callers)_sanitize_for_json: covers\r\n, NUL,\x07,\x0b,\x1f,\x7f, plain text, Unicode — all stripped to JSON-safe form, round-trip viajson.dumps/json.loadssucceedsmake check(full lint + mypy + 1029-test pytest, ~70 min including e2e Claude SDK) — 5 e2e failures, all pre-existing (test_plan_*ассертитstep_state.jsonafter/map-plan, but SKILL.md Step 7 explicitly says do NOT create it; remaining 3 are claude CLI timeouts / socket errors). No regressions from this PR.Migration
Downstream projects on the previous skill bundle need
mapify upgrade(or a manual copy of the renamed skill directory) to pick upmap-stateand the unhiddenmap-plan. Existing.map/<branch>/artifacts remain compatible — only the skill directory and itsskill-rules.jsonkey changed.