From 779c5d6b3686499cd72d3d6693748966414189b8 Mon Sep 17 00:00:00 2001 From: Egor Merkushev Date: Thu, 19 Feb 2026 11:46:43 +0300 Subject: [PATCH 1/2] Follow-up P13: phase13 implementation gap audit --- SPECS/INPROGRESS/next.md | 8 +++- SPECS/Workplan.md | 87 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 94 insertions(+), 1 deletion(-) diff --git a/SPECS/INPROGRESS/next.md b/SPECS/INPROGRESS/next.md index b3cabfcf..e6cc1c72 100644 --- a/SPECS/INPROGRESS/next.md +++ b/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. diff --git a/SPECS/Workplan.md b/SPECS/Workplan.md index 4941f694..cccb63b0 100644 --- a/SPECS/Workplan.md +++ b/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) From d5302495a17afcf2b3bb0e53351dab75d70e3533 Mon Sep 17 00:00:00 2001 From: Egor Merkushev Date: Thu, 19 Feb 2026 11:46:46 +0300 Subject: [PATCH 2/2] Archive REVIEW_Phase13_Implementation_Gap_Audit report --- SPECS/ARCHIVE/INDEX.md | 4 +- ...REVIEW_Phase13_Implementation_Gap_Audit.md | 69 +++++++++++++++++++ 2 files changed, 72 insertions(+), 1 deletion(-) create mode 100644 SPECS/ARCHIVE/_Historical/REVIEW_Phase13_Implementation_Gap_Audit.md diff --git a/SPECS/ARCHIVE/INDEX.md b/SPECS/ARCHIVE/INDEX.md index 2d685717..efdf0c97 100644 --- a/SPECS/ARCHIVE/INDEX.md +++ b/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) | diff --git a/SPECS/ARCHIVE/_Historical/REVIEW_Phase13_Implementation_Gap_Audit.md b/SPECS/ARCHIVE/_Historical/REVIEW_Phase13_Implementation_Gap_Audit.md new file mode 100644 index 00000000..88c3174f --- /dev/null +++ b/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. +