Repository navigation
mcpx: Phase 2 of #1834 — sampling message + result validation at the broker (pairing, era shape, no silent fill-in) #1836
Description
Phase 2 of #1834 — sampling message + result validation at the broker
Design: #1834 (mcpx sampling trust boundary). Design doc: files/designs/code-mcpx-sampling-trust-boundary.md in the keep-the-four-repos-healthy goal workspace. Origin: dezren39/darwin-nix#256.
Problem
Sampling messages and results are relayed without validation, in either direction. sampleViaBroker (pkgs/mcpx/internal/daemon/hooks.go) unmarshals only maxTokens/systemPrompt from a request — the messages array is never inspected, so tool-result messages holding non-tool_result content, or a tool_result whose id no earlier tool_use issued, pass through untouched (compare register SMP-06). On return the only checks are JSON-object / not-null, after which a missing role/model is silently filled in (result["role"] = "assistant", result["model"] = "unknown") and the result is never checked against its request: stopReason: "toolUse" or tool_use content against a tools-less request, and era-wrong content shapes (2025-11-25 made message content single-or-array; earlier eras single only — SMP-05), all relayed as-is. Gaps: sampling-both-validate-message-content (client + server rows) in pkgs/mcpx/internal/conformance/coverage_clientfeatures.go.
Scope — one validator at the broker choke point
One validation helper used by sampleViaBroker (the point legacy wire requests and modern input_required answers both converge on); do not copy checks into mcpclient/mcpserver separately.
- Request side, on open (before a broker question is opened):
messagesdecode as sampling messages for the upstream's era — content single block before 2025-11-25, single-or-array from 2025-11-25; tool-result messages hold onlytool_resultblocks; everytool_resultid pairs with atool_usein the preceding messages. Violations raise the typed-32602from Phase 1 back to the upstream immediately — a malformed request must not consume an answerer's attention. - Result side, on answer: the result must carry
roleandmodeland era-legal content;stopReason: "toolUse"/tool_usecontent blocks are valid only when the request carriedtools— against a tools-less request,-32602. Where the answer arrived through the broker, refuse it at the answer step with the question left open (the pattern the existing null-answer refusal ininternal/elicit/elicit.goestablished); where discovered after acceptance, refuse upstream with-32602. - Remove the silent
role/modelfill-in. It manufactures fields the answerer never supplied and hides exactly the malformation validation exists to catch. This behaviour change is deliberate and must be visible in the PR description. - Model-hint mismatch is NOT a failure: model preferences are expressly ignorable, so a result naming a different model than the hint asked for passes.
Out of scope: declaring any new sampling sub-capability (that is #1753's ground), rate limits (nix#257), CLI rendering (Phase 3), a genuine pre-delivery staging hold (open question on #1834).
Open seam for triage (recorded on the design)
Validation needs the upstream's negotiated era at the broker, which today receives params, not era. Settle where era comes from (the client connection that raised the question knows it). If it cannot be threaded without new plumbing, the recorded fallback is the permissive union (single-or-array accepted in all eras) with the era-strict half stated on this issue — do not silently shrink the scope.
Equally: this phase is independent of Phase 1 in code, but raises Phase 1's typed -32602; if Phase 1 has not landed, carry the error type with this phase (triage settles the seam — not a hard gate).
Tests (defining)
- Well-formed exchange in each era relays byte-identical (control case, both eras).
- Tool-result message mixed with text content →
-32602, no broker question opened. tool_resultwith an unpaired id →-32602, no question opened.- Result missing
roleormodel→ refused (question left open on the broker path); no fill-in occurs. - Result with
tool_usecontent /stopReason: "toolUse"against a tools-less request →-32602; the same result against a tools-carrying request passes. - Result naming a model different from the request's hint → passes.
Acceptance criteria
- The validator lives in one place and is exercised from
sampleViaBrokerin both directions; no duplicated checks in mcpclient/mcpserver. - Request violations are refused with
-32602before a broker question opens; result violations never reach the upstream; therole/modelfill-in is removed and the removal is called out in the PR. - Gaps
sampling-both-validate-message-content(both rows) are retired. - The era-source question is settled on this issue (threaded era, or the recorded union fallback with reasons).
Activity
developing-today commentedon Oct 28, 2026
CODE-765
dezren39 commentedon Oct 28, 2026
Phase 2 of design #1834, filed by the design architect (2026-10-28). Triage packet enqueued to repo-health-code-pilot as task 2719 (verified: queue=repo-health-code-pilot, status=queued, last_error=null). Independent of Phase 1 in code; raises Phase 1's typed -32602 — if #1835 has not landed, this phase carries the error type (triage settles the seam).
42 remaining items
dezren39 commentedon Oct 30, 2026
🤖 Agent note · done — PR #2443 merged, issue closed; Codex finding dispositioned in follow-up PR #2487
Lane B resumed task 3052: revalidated PR #2443 (CLEAN at
c9c82660, checks green, earlier Codex P2 race finding fixed + thread resolved), resolved its merge conflicts against the advanced main (mergeccf85113: modern.go kept Phase 2'srpcErrorFor, which Phase 1'sCodedError.RPCCode()was designed for; conformance matrix regenerated; mcpclient/daemon/elicit/conformance suites pass on the laptop, incl.-racesampling tests). The PR was then re-enqueued by another lane and merged 2026-10-30T01:08:19Z as390e8cad; #1836 auto-closed.One Codex P2 round landed on
ccf85113minutes before that merge (block-field validation). I verified it valid and fixed it — follow-up PR #2487 (c882c9b3: typed field validation indecodeSampleBlock+ regression tests; thread on #2443 replied and resolved). Task 3052 completes as fixed_merged; #2487's landing is tracked in the backlog.Merge pull request #2487 from developing-today/queue/3052-1836-codex-…
dezren39 commentedon Nov 3, 2026
agent-note:v1
state: triaged (coverage pass)
Coverage pass 2026-11-03: reviewed in the fleet-wide per-issue pass (drift / overlap / already-handled / stale / blockers / ordering). Verdict: still live; queued in repo-health-code-pilot as triage at priority 40 (priority/normal). If this is already handled at current main, the triage closes it with evidence; if it needs splitting, the triage files the split.
Found via: coverage pass 2026-11-03
dezren39 commentedon Nov 3, 2026
✅ Triage verdict — already done. Closing with evidence; no implementation needed.
repo-worker-code, OpenChamber scheduled lane (queue task 7747,repo-health-code-pilot)maintoday. The only reason this issue is open is that it was auto-closed by the merge, then reopened by the bot at 2026-10-30T01:15:51Z; the residual it was reopened for was tracked separately as mcpx(sampling): broker validator accepts degenerate empties — zero messages, empty content arrays, and a result with role "" / "user" pass ValidateSampleRequest/Result (#1836 residual) #2511 and closed COMPLETED 2026-11-02T15:37:09Z.Landing evidence
390e8cad.1b0ac3cd.As-built at current
maind3476bce7(2026-11-03)elicit.ValidateSampleRequest/elicit.ValidateSampleResult(pkgs/mcpx/internal/elicit/sampling.go:160,:218), used only from the broker choke point:sampleViaBroker(internal/daemon/hooks.go:315) validates the request at L329 — beforebroker.OpenRequest— and the answer at L394, before it is returned upstream. No duplicated checks in mcpclient/mcpserver.tool_use/tool_resultid pairing; violations returnelicit.SamplingInvalidParams(typed-32602) with no broker question opened.role/modelrequired, era-legal content,toolUseonly when the request carriedtools; the silentrole/modelfill-in is gone — the code comment athooks.go:384-388records the deliberate behaviour change. Model-hint mismatch still passes (hints are ignorable).mcpclient.SamplingEraFrom(ctx)athooks.go:325-327; the permissive union applies only when no era reaches the broker (direct handler calls in tests). No signature change toElicitHandler/pool.Hooks.conformance.TestSamplingValidation(internal/conformance/sampling_validation_test.go:13) coverssampling-both-validate-message-contenton both rows —coverage_clientfeatures.go:57-62now maps the gap throughSReq/CReqrequired-coverage forms pointing at that test, so it is no longer an open gap.Verification run for this triage
cd pkgs/mcpx && go test ./internal/conformance -run TestSamplingValidation -count=1→ ok (0.010s); the tree tested has zero diff ininternal/elicit,internal/daemon/hooks.goandinternal/conformanceagainstorigin/maind3476bce7.held/user/owned/do-not-mergelabels; no open PR orqueue/*1836*branch remains.Labels: cleared the stale in-flight markers (
queue/queued,status/needs-triage,status:in-review); close follows. Queue task 7747 completesalready_done.