Merge pull request #84 from SoundBlaster/codex/fu-p13-review-followup-backlog

P13: Add follow-up backlog from implementation gap audit

Author: SoundBlaster

SPECS/ARCHIVE/INDEX.md

@@ -1,6 +1,6 @@
 # mcpbridge-wrapper Tasks Archive
 
-**Last Updated:** 2026-02-19 (REVIEW_fu_p12_t3_1_error_message_docstring_archived)
+**Last Updated:** 2026-02-19 (REVIEW_Phase13_Implementation_Gap_Audit_archived)
 
 ## Archived Tasks
 
@@ -196,6 +196,7 @@
 | [REVIEW_P13-T4_stdio_proxy_mode.md](_Historical/REVIEW_P13-T4_stdio_proxy_mode.md) | Review report for P13-T4 |
 | [REVIEW_P13-T5_prompt_reduction_multi_client_stability.md](_Historical/REVIEW_P13-T5_prompt_reduction_multi_client_stability.md) | Review report for P13-T5 |
 | [REVIEW_P13-T6_broker_mode_configuration_migration.md](_Historical/REVIEW_P13-T6_broker_mode_configuration_migration.md) | Review report for P13-T6 |
+| [REVIEW_Phase13_Implementation_Gap_Audit.md](_Historical/REVIEW_Phase13_Implementation_Gap_Audit.md) | Consolidated Phase 13 implementation gap audit report |
 | [REVIEW_FU-P13-T4-1_broker_proxy_loop.md](_Historical/REVIEW_FU-P13-T4-1_broker_proxy_loop.md) | Review report for FU-P13-T4-1 |
 | [REVIEW_FU-P13-T4-2_broker_proxy_reconnect.md](_Historical/REVIEW_FU-P13-T4-2_broker_proxy_reconnect.md) | Review report for FU-P13-T4-2 |
 | [REVIEW_FU-P13-T2-1_event_wait_shutdown.md](_Historical/REVIEW_FU-P13-T2-1_event_wait_shutdown.md) | Review report for FU-P13-T2-1 |
@@ -368,6 +369,7 @@
 | 2026-02-19 | FU-P12-T1-5 | Archived REVIEW_FU-P12-T1-5_client_identity_retention report |
 | 2026-02-19 | FU-P12-T1-6 | Archived Uniform_HTML_escaping_in_renderClientWidgets (PASS) |
 | 2026-02-19 | FU-P12-T1-6 | Archived REVIEW_FU-P12-T1-6_uniform_client_widget_escaping report |
+| 2026-02-19 | P13 | Archived REVIEW_Phase13_Implementation_Gap_Audit report |
 | 2026-02-19 | FU-P12-T3-1 | Archived Document_unused_error_message_parameter_in_MetricsCollector_record_response (PASS) |
 | 2026-02-19 | FU-P12-T3-1 | Archived REVIEW_FU-P12-T3-1_error_message_docstring report |
 | 2026-02-19 | FU-P12-T3-2 | Archived Add_error_code_column_to_audit_CSV_export (PASS) |

SPECS/ARCHIVE/_Historical/REVIEW_Phase13_Implementation_Gap_Audit.md

@@ -0,0 +1,69 @@
+## REVIEW REPORT — Phase 13 Implementation Gap Audit
+
+**Scope:** Final implementation phase review (`Phase 13: Persistent Broker & Shared Xcode Session`, including P13-T1..P13-T6 and linked follow-ups)  
+**Primary Sources:** `SPECS/Workplan.md`, `SPECS/ARCHIVE/P13-T*_*/`, `src/mcpbridge_wrapper/broker/*`, `src/mcpbridge_wrapper/__main__.py`, `docs/broker-mode.md`
+
+### Summary Verdict
+- [ ] Approve
+- [ ] Approve with comments
+- [x] Request changes
+- [ ] Block
+
+### Critical / High Findings
+
+1. **[High] `--broker-spawn` auto-start path is not functionally complete (`--broker-daemon` entrypoint missing).**
+   - `BrokerProxy` spawns `python -m mcpbridge_wrapper --broker-daemon`: `src/mcpbridge_wrapper/broker/proxy.py:135`.
+   - `main()` only parses `--broker-connect` / `--broker-spawn`; no daemon-mode branch exists: `src/mcpbridge_wrapper/__main__.py:192`, `src/mcpbridge_wrapper/__main__.py:271`, `src/mcpbridge_wrapper/__main__.py:273`.
+   - Unknown flags are forwarded to upstream bridge command in direct path: `src/mcpbridge_wrapper/bridge.py:38`.
+   - Result: spawned process does not become a broker daemon and auto-spawn can timeout instead of creating the socket.
+   - Gap type: missed/lost feature versus expected runtime behavior in P13-T4/P13-T6 broker adoption flow.
+
+2. **[High] JSON-RPC numeric request IDs are lossy/collision-prone due 20-bit masking.**
+   - Integer IDs are truncated: `int_id = original_id & _ID_MASK`: `src/mcpbridge_wrapper/broker/transport.py:306`.
+   - Response restoration returns lower 20 bits unless mapped as string: `src/mcpbridge_wrapper/broker/transport.py:397`.
+   - Impacts:
+     - IDs larger than `0xFFFFF` are mutated in responses.
+     - Negative integer IDs are transformed.
+     - Distinct large IDs can collide within one session.
+   - Gap type: functional protocol correctness risk under valid JSON-RPC ID patterns.
+
+### Secondary Findings
+
+3. **[Medium] Planned local security boundary is documented but not enforced in transport.**
+   - Architecture/ADR specifies same-UID verification via `getpeereid` and owner-only socket permissions:  
+     `SPECS/ARCHIVE/P13-T1_Design_persistent_broker_architecture_and_protocol_contract/P13-T1_Design_persistent_broker_architecture_and_protocol_contract.md:55`,  
+     `SPECS/ARCHIVE/P13-T1_Design_persistent_broker_architecture_and_protocol_contract/P13-T1_Design_persistent_broker_architecture_and_protocol_contract.md:201`.
+   - Runtime currently accepts clients without UID validation; `peer_uid` is derived from `peername` and not enforced: `src/mcpbridge_wrapper/broker/transport.py:189`, `src/mcpbridge_wrapper/broker/transport.py:201`.
+   - Socket mode is not explicitly set to `0600` after bind: `src/mcpbridge_wrapper/broker/transport.py:77`.
+   - Gap type: security hardening feature not closed.
+
+4. **[Medium] Broker startup is not atomic if transport startup fails after upstream launch.**
+   - In `start()`, upstream launch + PID file write occur before transport bind/start: `src/mcpbridge_wrapper/broker/daemon.py:109`, `src/mcpbridge_wrapper/broker/daemon.py:112`, `src/mcpbridge_wrapper/broker/daemon.py:127`.
+   - If `transport.start()` raises, `start()` exits without cleanup of upstream process or PID/socket state.
+   - Gap type: reliability/lifecycle issue under bind/permission/race failures.
+
+5. **[Medium] Core Phase 13 objective remains partially unverified (prompt reduction acceptance not closed).**
+   - Workplan still marks P13-T5 as partial and leaves manual prompt criterion unchecked: `SPECS/Workplan.md:2102`, `SPECS/Workplan.md:2114`.
+   - Validation report confirms unresolved interactive verification: `SPECS/ARCHIVE/P13-T5_Validate_prompt_reduction_and_multi_client_stability/P13-T5_Validation_Report.md:26`.
+   - Gap type: incomplete closure of the main UX outcome this phase targets (reduced repeated Xcode permission prompts).
+
+6. **[Low] Operational “broker status/start/stop” is documented as shell/python one-liners, not first-class CLI mode.**
+   - Workplan output expected health/status command: `SPECS/Workplan.md:2001`.
+   - Current guide relies on long inline commands including private attribute mutation (`d._transport=t`): `docs/broker-mode.md:28`, `docs/broker-mode.md:42`, `docs/broker-mode.md:54`.
+   - Gap type: maintainability/operability mismatch vs intended productized flow.
+
+### Tests and Coverage Gaps
+
+- Missing end-to-end runtime test that `--broker-spawn` actually creates a live broker socket (not only mocked spawn paths).
+- Missing transport tests for large/negative integer request IDs to ensure identity preservation.
+- Missing tests for UID rejection behavior (if security boundary is intended to be enforced).
+- Missing failure-path test for `BrokerDaemon.start()` where `transport.start()` fails after upstream launch.
+
+### Recommended Follow-Up Tasks
+
+1. Implement explicit daemon CLI mode (`--broker-daemon` or `broker host`) in `main()` and add E2E spawn validation.
+2. Replace lossy int-bitmask remap with reversible per-session int-ID mapping (parallel to string ID map), then add regression tests.
+3. Enforce local auth boundary (`getpeereid` same-UID check) and explicit socket permission hardening (`chmod 0600`) with tests.
+4. Make `BrokerDaemon.start()` transactional (cleanup on partial startup failure).
+5. Complete P13-T5 interactive desktop validation and update verdict from PARTIAL to PASS/FAIL with evidence.
+

SPECS/INPROGRESS/next.md

@@ -11,4 +11,10 @@
 
 ## Suggested Next Tasks
 
-- No pending tasks remain (`126/126` complete).
+- FU-P13-T10: Implement explicit broker daemon entrypoint and operational CLI flows (P0)
+- FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport (P1)
+- FU-P13-T12: Enforce local Unix-socket security boundary for broker clients (P1)
+- FU-P13-T13: Make broker startup transactional when transport bind/start fails (P1)
+- FU-P13-T14: Complete interactive Xcode prompt verification and close P13-T5 (P1)
+
+Pending follow-up backlog from review: `5` open tasks.

SPECS/Workplan.md

@@ -2187,6 +2187,93 @@ Phase 9 Follow-up Backlog
 
 ---
 
+#### ⬜️ FU-P13-T10: Implement explicit broker daemon entrypoint and operational CLI flows
+- **Description:** Make broker host mode first-class by implementing a real daemon entrypoint (`--broker-daemon` or equivalent broker subcommand) in `__main__.py`, ensuring `--broker-spawn` can reliably auto-start and connect. Replace doc-only one-liner operational flows with supported CLI commands for start/status/stop.
+- **Priority:** P0
+- **Dependencies:** P13-T2, P13-T4
+- **Parallelizable:** no
+- **Outputs/Artifacts:**
+  - Updated `src/mcpbridge_wrapper/__main__.py` broker daemon branch and command parsing
+  - Updated `src/mcpbridge_wrapper/broker/proxy.py` spawn target (if needed)
+  - Integration test covering `--broker-spawn` end-to-end readiness
+  - Updated `docs/broker-mode.md` and setup docs with first-class broker host commands
+- **Acceptance Criteria:**
+  - [ ] Running `mcpbridge-wrapper --broker-daemon` starts broker host mode and creates live PID/socket state
+  - [ ] `--broker-spawn` successfully auto-starts broker and connects without manual bootstrap
+  - [ ] No broker-only flags are accidentally forwarded to `xcrun mcpbridge`
+  - [ ] Start/status/stop commands are documented as supported CLI flows (not private inline Python snippets)
+
+---
+
+#### ⬜️ FU-P13-T11: Preserve JSON-RPC numeric request ID fidelity in broker transport
+- **Description:** Remove lossy 20-bit integer ID masking in broker request remapping and implement a reversible per-session ID mapping for numeric IDs so all valid JSON-RPC IDs round-trip exactly.
+- **Priority:** P1
+- **Dependencies:** P13-T3
+- **Parallelizable:** yes
+- **Outputs/Artifacts:**
+  - Updated `src/mcpbridge_wrapper/broker/transport.py` ID remap/restore strategy
+  - Updated `src/mcpbridge_wrapper/broker/types.py` session mapping fields (if required)
+  - New/updated unit tests for large, negative, and concurrent numeric IDs
+- **Acceptance Criteria:**
+  - [ ] Integer IDs (including negative and > 20-bit) are returned unchanged to clients
+  - [ ] Distinct concurrent numeric IDs cannot collide within a session
+  - [ ] Existing string-ID routing behavior remains backward compatible
+  - [ ] Broker transport tests cover ID round-trip fidelity for int and string IDs
+
+---
+
+#### ⬜️ FU-P13-T12: Enforce local Unix-socket security boundary for broker clients
+- **Description:** Implement same-UID peer credential verification for broker socket clients and enforce owner-only socket permissions, aligning runtime behavior with P13-T1 ADR security decisions.
+- **Priority:** P1
+- **Dependencies:** P13-T1, P13-T3
+- **Parallelizable:** yes
+- **Outputs/Artifacts:**
+  - Updated `src/mcpbridge_wrapper/broker/transport.py` peer credential checks and rejection path
+  - Updated broker socket creation flow to enforce `0600` permissions
+  - Unit tests for accepted/rejected client credential cases
+  - Documentation update in `docs/broker-mode.md` and/or `docs/troubleshooting.md`
+- **Acceptance Criteria:**
+  - [ ] Broker accepts only same-UID local clients
+  - [ ] Connections failing UID verification are rejected without affecting active sessions
+  - [ ] Broker socket file is owner-readable/writable only (`0600`)
+  - [ ] Security-boundary behavior is documented and test-covered
+
+---
+
+#### ⬜️ FU-P13-T13: Make broker startup transactional when transport bind/start fails
+- **Description:** Harden `BrokerDaemon.start()` so partial startup failures (for example socket bind errors after upstream launch) perform full rollback, leaving no orphaned upstream process or stale PID/socket files.
+- **Priority:** P1
+- **Dependencies:** P13-T2, P13-T3
+- **Parallelizable:** yes
+- **Outputs/Artifacts:**
+  - Updated `src/mcpbridge_wrapper/broker/daemon.py` startup failure rollback path
+  - Regression tests for transport-start failure after upstream launch
+  - Troubleshooting note for deterministic failure behavior
+- **Acceptance Criteria:**
+  - [ ] If transport startup fails, upstream subprocess is terminated and waited
+  - [ ] PID/socket files are cleaned up on startup failure
+  - [ ] Broker state returns to a safe non-ready state after rollback
+  - [ ] Unit tests cover rollback behavior and prevent regression
+
+---
+
+#### ⬜️ FU-P13-T14: Complete interactive Xcode prompt verification and close P13-T5
+- **Description:** Execute and document the remaining human-run interactive validation for Xcode permission prompts in direct mode vs broker mode, then update P13-T5 verdict and linked acceptance states.
+- **Priority:** P1
+- **Dependencies:** P13-T5, P13-T6
+- **Parallelizable:** no
+- **Outputs/Artifacts:**
+  - Updated `SPECS/ARCHIVE/P13-T5_Validate_prompt_reduction_and_multi_client_stability/P13-T5_manual_prompt_validation.md`
+  - Updated `SPECS/ARCHIVE/P13-T5_Validate_prompt_reduction_and_multi_client_stability/P13-T5_Validation_Report.md`
+  - Workplan status update for P13-T5 acceptance line items
+- **Acceptance Criteria:**
+  - [ ] Interactive desktop run confirms observed prompt behavior for repeated short-lived sessions
+  - [ ] P13-T5 manual prompt criterion is resolved to PASS or FAIL with concrete evidence
+  - [ ] Any discovered deviations are captured in troubleshooting and/or follow-up bug tasks
+  - [ ] BUG-T4 related resolution path is reconciled with the final validation outcome
+
+---
+
 
 #### ✅ FU-BUG-T7-1: Cap `pending_methods` map to guard against unbounded growth
 - **Status:** ✅ Completed (2026-02-18)