Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion SPECS/ARCHIVE/INDEX.md
Original file line number Diff line number Diff line change
@@ -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

Expand Down Expand Up @@ -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 |
Expand Down Expand Up @@ -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) |
Expand Down
Original file line number Diff line number Diff line change
@@ -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.

8 changes: 7 additions & 1 deletion SPECS/INPROGRESS/next.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.
87 changes: 87 additions & 0 deletions SPECS/Workplan.md
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down