Validate MCP client flow and harden approval concurrency#15
Conversation
|
Review note: the repository source of truth is this host and this branch history. The Debian 13 VPS is only a validation lab for bootstrap and host evidence; it is not the primary repo or deployment target. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 27cc47368a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ORDER BY | ||
| CASE status | ||
| WHEN 'pending' THEN 0 | ||
| ELSE 1 | ||
| END, |
There was a problem hiding this comment.
Prefer executing match over pending duplicates
When a database is upgraded from older versions, it can already contain both pending and executing approvals for the same action digest; this query orders pending first, so prepare_matching_tool_approval_for_execution() will claim the pending row instead of reporting the in-flight execution. In that state, a confirmed retry can start a second execution of the same mutating action concurrently, which defeats the new duplicate-execution hardening.
Useful? React with 👍 / 👎.
| payload = self.runtime.approve_tool_approval(approval_id) | ||
| return self._build_standard_tool_result(payload, is_error=False) |
There was a problem hiding this comment.
Mark failed approval_approve calls as MCP tool errors
approval_approve is always returned with is_error=False, but runtime.approve_tool_approval() can legitimately return a failed execution payload (execution.ok == false, approval status failed). In that case, standard MCP clients receive a non-error tool result for a failed action and may treat it as success, which can hide failed writes or retries.
Useful? React with 👍 / 👎.
Summary
This change closes the next runtime/MCP hardening slice in three areas:
mc mcp-serveagainst a real standard MCP client through the official Inspector CLIWhat Changed
MCP interoperability
src/master_control/interfaces/mcp/server.pyto support standard JSON-RPC MCP requests over stdioapproval_list,approval_get,approval_approve,approval_rejectapprovals/*top-level methods for backwards compatibilityApproval concurrency
src/master_control/store/session_store.pyapproval_in_progressexplicitly from the runtime when a confirmed retry hits an in-flight approvalValidation and docs
scripts/validate_mcp_client.pydocs/mcp-client-validation.mdReview Guide
Review in this order:
src/master_control/interfaces/mcp/server.pyFocus on JSON-RPC compatibility, legacy compatibility retention, and how approval tools are exposed for standard MCP clients.
src/master_control/store/session_store.pyandsrc/master_control/core/runtime.pyFocus on approval deduplication, claim/finalize behavior, and the in-flight execution guard.
tests/test_mcp_server.py,tests/test_mcp_stdio_integration.py, andtests/test_session_store.pyFocus on whether the new tests prove the intended interoperability and concurrency guarantees.
scripts/validate_mcp_client.pyanddocs/mcp-client-validation.mdFocus on whether the Inspector-backed validation path is repeatable and documented clearly.
Confirm
README.md,docs/status.md,docs/roadmap.md, anddocs/beta-readiness-gate.mdmatch the new reality without overstating maturity.Validation
python3 -m ruff check .python3 -m mypy srcPYTHONPATH=src python3 -m pytest -qPYTHONPATH=src python3 -m unittest discover -s testspython3 -m compileall srcPYTHONPATH=src python3 -m master_control --json doctorpython3 -m pip wheel . --no-deps -w /tmp/mc-dist-phase2python3 scripts/validate_mcp_client.py --jsonpython3 scripts/validate_operator_bootstrap.py --output-dir /tmp/mc-bootstrap-validation-phase2 --provider heuristic --python python3python3 scripts/validate_operator_bootstrap.py ...on the Debian 13 VPS lab using the current branch snapshotResidual Risks
core.runtime,session_store, and CLI entrypoint remain major hotspots