fix: address review findings — backward compat, ACP safety, result timing, security

- Fix 1: tasks_md_sync only sets MACP fields when columns exist in table headers
- Fix 2: ACP dispatch now escalates instead of falsely completing
- Fix 3: Removed premature collect_result() from dispatch_task()
- Fix 4: Yolo brief staged via temp file (0600) instead of process args
- Fix 5: cleanup_worktree validates path against configured worktree base

docs/DEVELOPER-GUIDE/orchestrator-matrix/macp-phase1.md

@@ -12,8 +12,8 @@ MACP Phase 1 extends `tools/orchestrator-matrix/` without replacing the existing
 ## Dispatch Modes
 
 1. `exec`: runs the task's `command` directly inside the task worktree.
-2. `yolo`: launches `mosaic yolo <runtime>` with the task brief content via a PTY wrapper.
-3. `acp`: emits the config payload a caller can hand to an ACP/OpenClaw session spawner.
+2. `yolo`: launches `mosaic yolo <runtime>` via a PTY wrapper and stages the brief in a temporary file so the brief body is not exposed in process arguments.
+3. `acp`: escalates immediately with `ACP dispatch requires OpenClaw integration (Phase 2)` until real ACP/OpenClaw spawning exists.
 
 ## Result Contract
 
@@ -28,4 +28,4 @@ MACP writes task result JSON under `.mosaic/orchestrator/results/` by default. R
 
 ## Compatibility
 
-Legacy tasks that omit `dispatch` still behave like the original matrix controller. This keeps existing `tasks.json` workflows functional while allowing orchestrators to opt into MACP incrementally.
+Legacy tasks that omit `dispatch` still behave like the original matrix controller. `tasks_md_sync.py` only injects MACP fields when the corresponding markdown headers exist, which keeps existing `tasks.json` workflows functional while allowing orchestrators to opt into MACP incrementally.

docs/PRD.md

@@ -48,7 +48,7 @@ The current orchestrator-matrix rail can queue shell-based worker tasks, but it
 2. Event schema must recognize `task.gated`, `task.escalated`, and `task.retry.scheduled`, plus a `dispatcher` source.
 3. Dispatcher functions must set up worktrees, build commands, execute tasks, collect results, and clean up worktrees.
 4. Controller `run_single_task()` must route MACP-aware tasks through the dispatcher and emit the correct lifecycle events/status transitions.
-5. `tasks_md_sync.py` must map optional MACP table columns when present and otherwise apply config defaults.
+5. `tasks_md_sync.py` must map optional MACP table columns only when those headers are present in `docs/TASKS.md`; absent MACP headers must not inject MACP fields into legacy tasks.
 6. `bin/mosaic` must route `mosaic macp ...` to a new `bin/mosaic-macp` script.
 
 ## Non-Functional Requirements
@@ -94,5 +94,5 @@ The current orchestrator-matrix rail can queue shell-based worker tasks, but it
 ## Assumptions
 
 1. ASSUMPTION: A single issue can track the full Phase 1 implementation because the user requested one bounded feature delivery rather than separate independent tickets.
-2. ASSUMPTION: For `acp` dispatch, generating the config/payload and returning it as dispatcher output is sufficient for Phase 1 because the brief explicitly says the caller will use it with OpenClaw.
+2. ASSUMPTION: For `acp` dispatch in Phase 1, the controller must escalate the task immediately with a clear reason instead of pretending work ran before OpenClaw integration exists.
 3. ASSUMPTION: `task.gated` should be emitted by the controller as the transition into quality-gate execution, which keeps gate-state ownership in one place alongside the existing gate loop.

docs/TASKS.md

@@ -14,4 +14,4 @@ Canonical tracking for active work. Keep this file current.
 
 | id | status | description | issue | repo | branch | depends_on | blocks | agent | started_at | completed_at | estimate | used | notes |
 |---|---|---|---|---|---|---|---|---|---|---|---|---|---|
-| MACP-PHASE1 | blocked | Implement MACP Phase 1 across orchestrator schemas, dispatcher, controller, CLI, config, and task sync while preserving legacy queue behavior. | #8 | bootstrap | feat/macp-phase1 | | | Jarvis | 2026-03-27T23:00:00Z | 2026-03-27T23:45:00Z | medium | completed | Implementation, verification, review, commit, and push completed. Blocked on PR creation: `~/.config/mosaic/tools/git/pr-create.sh -t 'feat: implement MACP phase 1 core protocol' -b ... -B main -H feat/macp-phase1` failed with `Remote repository required: Specify ID via --repo or execute from a local git repo.` |
+| MACP-PHASE1 | in-progress | Implement MACP Phase 1 across orchestrator schemas, dispatcher, controller, CLI, config, and task sync while preserving legacy queue behavior. | #8 | bootstrap | feat/macp-phase1 | | | Jarvis | 2026-03-27T23:00:00Z |  | medium | in-progress | Review-fix pass started 2026-03-28T00:38:01Z to address backward-compatibility, ACP safety, result timing, and worktree/brief security findings on top of the blocked PR-create state. Prior blocker remains: `~/.config/mosaic/tools/git/pr-create.sh -t 'feat: implement MACP phase 1 core protocol' -b ... -B main -H feat/macp-phase1` failed with `Remote repository required: Specify ID via --repo or execute from a local git repo.` |

docs/scratchpads/macp-phase1.md

@@ -56,6 +56,32 @@ Implement MACP Phase 1 in `mosaic-bootstrap` by extending the orchestrator-matri
 - 2026-03-27: Added explicit worker escalation handling via the `MACP_ESCALATE:` stdout marker.
 - 2026-03-27: Committed and pushed branch `feat/macp-phase1` (`7ef49a3`, `fd6274f`).
 - 2026-03-27: Blocked in PR workflow when `~/.config/mosaic/tools/git/pr-create.sh` failed to resolve the remote repository from this worktree.
+- 2026-03-28: Resumed from blocked state for a review-fix pass covering 5 findings in `docs/tasks/MACP-PHASE1-fixes.md`.
+
+## Review Fix Pass
+
+### Scope
+
+1. Restore legacy `tasks_md_sync.py` behavior so rows without MACP headers do not become MACP tasks.
+2. Make ACP dispatch fail-safe via escalation instead of a no-op success path.
+3. Move MACP result writes to the controller after quality gates determine the final task status.
+4. Remove brief text from yolo command arguments by switching to file-based brief handoff.
+5. Restrict worktree cleanup to validated paths under the configured worktree base.
+
+### TDD / Test-First Decision
+
+1. This is a bug-fix and security-hardening pass, so targeted reproducer verification is required.
+2. Repo appears to use focused script-level verification rather than a Python test suite for this surface, so reproducer checks will be command-driven and recorded as evidence.
+
+### Planned Verification Additions
+
+| Finding | Verification |
+|---|---|
+| Legacy task reclassification | Sync `docs/TASKS.md` without MACP headers into `tasks.json` and confirm `dispatch` is absent so controller stays on `run_shell()` |
+| ACP no-op success | Run controller/dispatcher with `dispatch=acp` and confirm `status=escalated`, exit path is non-zero, and `task.escalated` is emitted |
+| Premature result write | Inspect result JSON after final controller state only; confirm gate results are present and no dispatcher pre-write remains |
+| Brief exposure | Build yolo command and confirm the brief body is absent from the command text |
+| Unsafe cleanup | Call cleanup against a path outside configured base and confirm it is refused |
 
 ## Tests Run
 

docs/tasks/MACP-PHASE1-fixes.md

@@ -0,0 +1,64 @@
+# MACP Phase 1 — Review Fixes
+
+**Branch:** `feat/macp-phase1` (amend/fix commits on top of existing)
+**Repo worktree:** `~/src/mosaic-bootstrap-worktrees/macp-phase1`
+
+These are 5 findings from code review and security review. Fix all of them.
+
+---
+
+## Fix 1 (BLOCKER): Legacy task reclassification in tasks_md_sync.py
+
+**File:** `tools/orchestrator-matrix/controller/tasks_md_sync.py`
+**Problem:** `build_task()` now always sets `task["dispatch"]` from `macp_defaults` even when the `docs/TASKS.md` table has no `dispatch` column. The controller's `is_macp_task()` check is simply `"dispatch" in task`, so ALL synced tasks get reclassified as MACP tasks and routed through the dispatcher instead of the legacy `run_shell()` path. This breaks backward compatibility.
+
+**Fix:** Only populate `dispatch`, `type`, `runtime`, and `branch` from MACP defaults when the markdown table row **explicitly contains that column**. If the column is absent from the table header, do NOT inject MACP fields. Check whether the parsed headers include these column names before adding them.
+
+---
+
+## Fix 2 (BLOCKER): ACP dispatch is a no-op that falsely reports success
+
+**File:** `tools/orchestrator-matrix/dispatcher/macp_dispatcher.py`
+**Problem:** For `dispatch == "acp"`, `build_dispatch_command()` returns a `python3 -c ...` command that only prints JSON but doesn't actually spawn an ACP session. The controller then marks the task completed even though no work ran.
+
+**Fix:** For `acp` dispatch, do NOT generate a command that runs and exits 0. Instead, set the task status to `"escalated"` with `escalation_reason = "ACP dispatch requires OpenClaw integration (Phase 2)"` and return exit code 1. This makes it fail-safe — ACP tasks won't silently succeed. The controller should emit a `task.escalated` event for these.
+
+---
+
+## Fix 3 (SHOULD-FIX): Premature result write before quality gates
+
+**File:** `tools/orchestrator-matrix/dispatcher/macp_dispatcher.py`
+**Problem:** `dispatch_task()` calls `collect_result(task, exit_code, [], orch_dir)` immediately after the worker exits, before quality gates run. If the controller crashes between this write and the final overwrite, a false "completed" result with empty gate_results sits on disk.
+
+**Fix:** Remove the `collect_result()` call from `dispatch_task()`. Instead, have `dispatch_task()` return `(exit_code, output)` only. The controller in `mosaic_orchestrator.py` should call `collect_result()` AFTER quality gates have run, when the final status is known. Make sure the controller passes gate_results to collect_result.
+
+---
+
+## Fix 4 (SECURITY MEDIUM): Brief contents exposed in process arguments
+
+**File:** `tools/orchestrator-matrix/dispatcher/macp_dispatcher.py`
+**Problem:** For `yolo` dispatch, `build_dispatch_command()` puts the full task brief text into the shell command as an argument: `mosaic yolo codex "<entire brief>"`. This is visible in `ps` output, shell logs, and crash reports.
+
+**Fix:** Write the brief contents to a temporary file with restrictive permissions (0600), and pass the file path to the worker instead. The command should read: `mosaic yolo <runtime> "$(cat /path/to/brief-TASKID.tmp)"` or better, restructure so the worker script reads from a file path argument. Use the existing `orchestrator-worker.sh` pattern which already reads from a task file. Clean up the temp file after dispatch completes.
+
+---
+
+## Fix 5 (SECURITY MEDIUM): Unrestricted worktree cleanup path
+
+**File:** `tools/orchestrator-matrix/dispatcher/macp_dispatcher.py`  
+**Problem:** `cleanup_worktree()` trusts `task['worktree']` without validating it belongs to the expected worktree base directory. A tampered task could cause deletion of unrelated worktrees.
+
+**Fix:** Before running `git worktree remove`, validate that the resolved worktree path starts with the configured `worktree_base` (from `config.macp.worktree_base`, with `{repo}` expanded). If the path doesn't match the expected base, log a warning and refuse to clean up. Add a helper `_is_safe_worktree_path(worktree_path, config)` for this validation.
+
+---
+
+## Verification
+
+After fixes:
+1. `python3 -c "from tools.orchestrator_matrix.dispatcher import macp_dispatcher"` should not error (fix import path if needed, or just verify `python3 tools/orchestrator-matrix/dispatcher/macp_dispatcher.py` has no syntax errors)
+2. Legacy tasks.json with no `dispatch` field must still work with the controller's `run_shell()` path
+3. ACP dispatch should NOT mark tasks completed — should escalate
+4. No brief text should appear in generated shell commands for yolo dispatch
+5. `cleanup_worktree()` should refuse paths outside the configured base
+
+Commit with: `fix: address review findings — backward compat, ACP safety, result timing, security`