fix(contracts): params validation rejects only unknown keys; accepted params + results are checked after the handler
Handlers own their documented domain codes (4006 missing session_id, 4015 bad
url, 4009 orphan claim); the contract's job on the way in is the one check no
handler performs — an unknown key (4000 with the key path). Missing/mistyped
fields are re-checked AFTER a successful handler answer under the strict
test policy, so a contract narrower than the wire still fails the suite.
Two models widened from the suite: SeedMessage (clients forward stored rows
verbatim), tool.complete.args (mirrored child rows omit it). Tests that
drove session.activate with prompt params (and vice versa) or stubbed
_live_session_payload with a bare {session_id} now send the real shapes.
This commit is contained in:
@@ -2415,12 +2415,13 @@ export interface SessionCreateParams {
|
||||
room_plumbing?: boolean
|
||||
follow_profile_config?: boolean
|
||||
}
|
||||
/** One create-time transcript row (``session_history._coerce_seed_history``); ``text`` is the legacy alias of ``content``; only ``display_kind: "hidden"`` is accepted from the wire. */
|
||||
/** One create-time transcript row (``session_history._coerce_seed_history``); ``text`` is the legacy alias of ``content``; only ``display_kind: "hidden"`` is accepted from the wire. Clients forward stored rows verbatim (``_row_id``, ``timestamp``, …) and the coercer drops what it does not use, so the row stays open. */
|
||||
export interface SeedMessage {
|
||||
role: string
|
||||
content?: string | null
|
||||
text?: string | null
|
||||
display_kind?: string | null
|
||||
[key: string]: unknown
|
||||
}
|
||||
export interface SessionCreateResult {
|
||||
session_id: string
|
||||
@@ -3866,7 +3867,7 @@ export interface ToolStartPayload {
|
||||
export interface ToolCompletePayload {
|
||||
tool_id: string
|
||||
name: string
|
||||
args: Record<string, unknown>
|
||||
args?: Record<string, unknown> | null
|
||||
duration_s?: number | null
|
||||
result?: unknown
|
||||
summary?: string | null
|
||||
|
||||
@@ -23016,8 +23016,8 @@
|
||||
"type": "object"
|
||||
},
|
||||
"SeedMessage": {
|
||||
"additionalProperties": false,
|
||||
"description": "One create-time transcript row (``session_history._coerce_seed_history``); ``text`` is the\nlegacy alias of ``content``; only ``display_kind: \"hidden\"`` is accepted from the wire.",
|
||||
"additionalProperties": true,
|
||||
"description": "One create-time transcript row (``session_history._coerce_seed_history``); ``text`` is the\nlegacy alias of ``content``; only ``display_kind: \"hidden\"`` is accepted from the wire. Clients\nforward stored rows verbatim (``_row_id``, ``timestamp``, \u2026) and the coercer drops what it does\nnot use, so the row stays open.",
|
||||
"properties": {
|
||||
"role": {
|
||||
"title": "Role",
|
||||
@@ -29610,9 +29610,17 @@
|
||||
"type": "string"
|
||||
},
|
||||
"args": {
|
||||
"additionalProperties": true,
|
||||
"title": "Args",
|
||||
"type": "object"
|
||||
"anyOf": [
|
||||
{
|
||||
"additionalProperties": true,
|
||||
"type": "object"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
],
|
||||
"default": null,
|
||||
"title": "Args"
|
||||
},
|
||||
"duration_s": {
|
||||
"anyOf": [
|
||||
@@ -29694,8 +29702,7 @@
|
||||
},
|
||||
"required": [
|
||||
"tool_id",
|
||||
"name",
|
||||
"args"
|
||||
"name"
|
||||
],
|
||||
"title": "ToolCompletePayload",
|
||||
"type": "object"
|
||||
|
||||
@@ -112,7 +112,7 @@ def test_membership_preserves_terminal_delivery_and_revokes_departed_peers(monke
|
||||
newcomer = b if attachment == "direct" else FanoutTransport(a, b)
|
||||
assert server._attach_session_transport(session, newcomer)
|
||||
assert server._attach_session_transport(session, b)
|
||||
for kind in ("message.start", "message.delta", "message.complete"):
|
||||
for kind in ("message.delta", "reasoning.delta", "message.complete"):
|
||||
server._emit(kind, "fanout-invariant", {"text": "α"})
|
||||
first, second = a.receive(), b.receive()
|
||||
assert first == second
|
||||
|
||||
@@ -66,7 +66,7 @@ def homes(monkeypatch, tmp_path):
|
||||
monkeypatch.setattr(server, "_default_session_cwd", lambda *a, **k: str(tmp_path))
|
||||
monkeypatch.setattr(server, "_child_run_active", lambda _key: False)
|
||||
monkeypatch.setattr(
|
||||
server, "_live_session_payload", lambda sid, session, **_k: {"session_id": sid}
|
||||
server, "_live_session_payload", lambda sid, session, **_k: {"session_id": sid, "message_count": 0, "messages": [], "info": {}}
|
||||
)
|
||||
known = set(server._sessions)
|
||||
yield homes
|
||||
|
||||
@@ -201,7 +201,7 @@ def test_resume_closes_profile_db_on_live_session_fast_path(profile_dbs, monkeyp
|
||||
monkeypatch.setattr(
|
||||
server,
|
||||
"_live_session_payload",
|
||||
lambda sid, session, **_kwargs: {"session_id": sid},
|
||||
lambda sid, session, **_k: {"session_id": sid, "message_count": 0, "messages": [], "info": {}},
|
||||
)
|
||||
monkeypatch.setattr(server, "_child_run_active", lambda _key: False)
|
||||
|
||||
|
||||
@@ -199,9 +199,10 @@ def test_reconnect_cannot_cross_orphan_interrupt_claim(monkeypatch, path, claim)
|
||||
elif path == "reuse":
|
||||
response = server._resume_reuse_live(ctx, sid, session)
|
||||
else:
|
||||
name = {"activate": "session.activate", "prompt": "prompt.submit"}[path]
|
||||
name, extra = {"activate": ("session.activate", {"omit_messages": True}),
|
||||
"prompt": ("prompt.submit", {"text": "continue"})}[path]
|
||||
response = server.handle_request({"jsonrpc": "2.0", "id": 1, "method": name,
|
||||
"params": {"session_id": sid, "text": "continue", "omit_messages": True}})
|
||||
"params": {"session_id": sid, **extra}})
|
||||
assert response.get("error", {}).get("code") == (4007 if claim == "retired" else 4009)
|
||||
assert session["transport"] is server._detached_ws_transport
|
||||
assert sid in server._pending_ws_reaps
|
||||
|
||||
@@ -253,7 +253,7 @@ class ToolCompletePayload(Payload):
|
||||
|
||||
tool_id: str
|
||||
name: str
|
||||
args: dict[str, JsonValue]
|
||||
args: dict[str, JsonValue] | None = None # mirrored child rows / room relays omit it
|
||||
duration_s: float | None = None
|
||||
result: JsonValue = None
|
||||
summary: str | None = None
|
||||
|
||||
@@ -98,17 +98,31 @@ def _report(kind: str, name: str, exc: ValidationError) -> None:
|
||||
|
||||
|
||||
def validate_params(contract: MethodContract | ServerRequestContract, params: dict) -> tuple[dict | None, str | None]:
|
||||
"""``(params, None)`` when valid — the ORIGINAL dict, so handlers keep reading what the client
|
||||
sent; ``(None, message)`` for the ``4000`` error."""
|
||||
"""Reject UNKNOWN keys (``4000`` with the key path) — the one check no handler performs, and the
|
||||
one that catches a renamed or misspelled field on either side. Required / type errors are left to
|
||||
the handler, which owns its documented domain codes (``4006`` missing session_id, ``4015`` bad
|
||||
url, …) and which clients already branch on; ``check_params_accepted`` closes the loop by
|
||||
flagging a handler that SUCCEEDS on params the contract calls invalid."""
|
||||
try:
|
||||
contract.params.model_validate(params)
|
||||
except ValidationError as exc:
|
||||
first = exc.errors()[0]
|
||||
loc = ".".join(str(p) for p in first.get("loc", ())) or "params"
|
||||
return None, f"invalid params for {contract.name}: {loc}: {first.get('msg')}"
|
||||
for err in exc.errors():
|
||||
if err.get("type") == "extra_forbidden":
|
||||
loc = ".".join(str(p) for p in err.get("loc", ())) or "params"
|
||||
return None, f"invalid params for {contract.name}: {loc}: {err.get('msg')}"
|
||||
return params, None
|
||||
|
||||
|
||||
def check_params_accepted(contract: MethodContract | ServerRequestContract, params: dict) -> None:
|
||||
"""The handler answered with a result: the params it accepted must be valid under the
|
||||
contract, else the contract is narrower than the wire (a required field that is optional in
|
||||
practice, a type the handler coerces). Same strict/log policy as results."""
|
||||
try:
|
||||
contract.params.model_validate(params)
|
||||
except ValidationError as exc:
|
||||
_report("params accepted by", contract.name, exc)
|
||||
|
||||
|
||||
def check_result(contract: MethodContract | ServerRequestContract, result: dict) -> None:
|
||||
try:
|
||||
contract.result.model_validate(result)
|
||||
|
||||
@@ -109,7 +109,11 @@ class LiveSessionSnapshot(Result):
|
||||
|
||||
class SeedMessage(Params):
|
||||
"""One create-time transcript row (``session_history._coerce_seed_history``); ``text`` is the
|
||||
legacy alias of ``content``; only ``display_kind: "hidden"`` is accepted from the wire."""
|
||||
legacy alias of ``content``; only ``display_kind: "hidden"`` is accepted from the wire. Clients
|
||||
forward stored rows verbatim (``_row_id``, ``timestamp``, …) and the coercer drops what it does
|
||||
not use, so the row stays open."""
|
||||
|
||||
model_config = Params.model_config | {"extra": "allow"}
|
||||
|
||||
role: str
|
||||
content: str | None = None
|
||||
|
||||
@@ -816,6 +816,7 @@ def handle_request(req: dict) -> dict | None:
|
||||
finally:
|
||||
_current_rpc_method.reset(token)
|
||||
if contract is not None and isinstance(response, dict) and isinstance(response.get("result"), dict):
|
||||
_contracts.check_params_accepted(contract, params)
|
||||
_contracts.check_result(contract, response["result"])
|
||||
return response
|
||||
|
||||
|
||||
@@ -92,7 +92,7 @@ def _register(req: ServerRequest) -> None:
|
||||
raise RuntimeError(f"server request {req.method!r} has no contract in tui_gateway/contracts")
|
||||
_, problem = contracts.validate_params(contract, {"session_id": req.sid, **req.params})
|
||||
if problem is not None:
|
||||
raise ValueError(problem)
|
||||
raise ValueError(problem) # a key the renderer's typed handler would never read: our bug
|
||||
with _lock:
|
||||
_open[req.id] = req
|
||||
_write(req.frame())
|
||||
|
||||
Reference in New Issue
Block a user