diff --git a/roboco/api/schemas/v2/do.py b/roboco/api/schemas/v2/do.py index 67627731..e242fbee 100644 --- a/roboco/api/schemas/v2/do.py +++ b/roboco/api/schemas/v2/do.py @@ -31,16 +31,21 @@ class NoteRequest(BaseModel): scope: str = "note" task_id: UUID | None = None title: str | None = None - # decision scope (all required at gateway when scope='decision') - context: str | None = None + # decision scope (all required at gateway when scope='decision'). + # Typed as non-nullable str (default "") so the MCP tool schema declares + # the field as `string` not `anyOf[string, null]` — smoke-6 showed + # minimax-m2.7 passing literal `null` for these and the server-side + # gate looping forever on `incomplete_input`. Empty string still counts + # as missing at the gate. + context: str = "" options: list[dict[str, str]] | None = None # [{name, pros, cons}, ...] - chosen: str | None = None - rationale: str | None = None + chosen: str = "" + rationale: str = "" consequences: list[str] | None = None # reflect scope (what_done/learned/struggled required when scope='reflect') - what_done: str | None = None - what_learned: str | None = None - what_struggled: str | None = None + what_done: str = "" + what_learned: str = "" + what_struggled: str = "" next_steps: list[str] | None = None diff --git a/roboco/mcp/do_server.py b/roboco/mcp/do_server.py index c5f05601..eb2516c4 100644 --- a/roboco/mcp/do_server.py +++ b/roboco/mcp/do_server.py @@ -179,14 +179,14 @@ def note( scope: str = "note", task_id: str | None = None, title: str | None = None, - context: str | None = None, + context: str = "", options: list[dict[str, str]] | None = None, - chosen: str | None = None, - rationale: str | None = None, + chosen: str = "", + rationale: str = "", consequences: list[str] | None = None, - what_done: str | None = None, - what_learned: str | None = None, - what_struggled: str | None = None, + what_done: str = "", + what_learned: str = "", + what_struggled: str = "", next_steps: list[str] | None = None, ) -> dict[str, Any]: """Write a journal entry. scope in note|decision|reflect|learning|struggle. diff --git a/roboco/services/gateway/content_actions.py b/roboco/services/gateway/content_actions.py index 801611f2..3e8f2833 100644 --- a/roboco/services/gateway/content_actions.py +++ b/roboco/services/gateway/content_actions.py @@ -375,32 +375,43 @@ class ContentActions: if missing: if scope == "decision": remediate = ( + "DO NOT pass null. context / chosen / rationale must be " + "non-empty strings — do not omit them or pass null. If you " + "genuinely have no context, write context='(none — direct " + "request)' or describe the task brief itself.\n\n" "re-issue note(scope='decision', ...) with these fields filled: " - f"{', '.join(missing)}.\n\nExample:\n" + f"{', '.join(missing)}.\n\nExample (replace the angle-bracket " + "placeholders with real strings — do NOT send the placeholders " + "as-is and do NOT send null):\n" "note(\n" " scope='decision',\n" - " text='',\n" - " context='',\n" + " text='Going with redis for the queue',\n" + " context='Need a queue for background work',\n" " options=[\n" - " {'name': 'optionA', 'pros': '', 'cons': ''},\n" - " {'name': 'optionB', 'pros': '', 'cons': ''},\n" + " {'name': 'redis', 'pros': 'fast', 'cons': 'ephemeral'},\n" + " {'name': 'postgres', 'pros': 'durable', 'cons': 'slower'},\n" " ],\n" - " chosen='',\n" - " rationale='',\n" + " chosen='redis',\n" + " rationale='speed beats durability for this experiment',\n" ")\n\n" "Pre-gateway parity — these populate the panel's Decisions view." ) else: remediate = ( + "DO NOT pass null. what_done / what_learned / what_struggled " + "must be non-empty strings — do not omit them or pass null. " + "If a field genuinely doesn't apply, write 'n/a' or 'nothing " + "notable'.\n\n" "re-issue note(scope='reflect', ...) with these fields filled: " - f"{', '.join(missing)}.\n\nExample:\n" + f"{', '.join(missing)}.\n\nExample (replace the placeholders " + "with real strings — do NOT send null):\n" "note(\n" " scope='reflect',\n" - " text='',\n" - " what_done='',\n" - " what_learned='',\n" - " what_struggled='',\n" - " next_steps=['', ''],\n" + " text='Smoke test pass on PR #17',\n" + " what_done='Edited README.md L42 and committed [abc12345]',\n" + " what_learned='The submit_for_qa verb requires reflect',\n" + " what_struggled='Initial commit message under 20 chars',\n" + " next_steps=['Wait for QA review'],\n" ")\n\n" "Pre-gateway parity — these populate the panel's Reflections view." ) diff --git a/tests/unit/api/schemas/v2/test_note_request_no_null.py b/tests/unit/api/schemas/v2/test_note_request_no_null.py new file mode 100644 index 00000000..7dad6372 --- /dev/null +++ b/tests/unit/api/schemas/v2/test_note_request_no_null.py @@ -0,0 +1,93 @@ +"""Smoke-6: NoteRequest rejects null for decision/reflect string fields. + +Original bug: minimax-m2.7 read the MCP tool schema, saw +`context: anyOf[string, null]`, and decided null was valid. Pydantic +on the route accepted it (because the field WAS `str | None`), passed +it through, and the server-side gate looped on `incomplete_input`. + +Fix tightens the schema: those fields are now `str = ""`. The MCP +schema generator emits `string` (not `string | null`), and Pydantic +on the route rejects literal null with 422. Empty string is still +treated as missing at the gateway gate. +""" + +from __future__ import annotations + +import pytest +from pydantic import ValidationError +from roboco.api.schemas.v2.do import NoteRequest + + +def test_note_request_accepts_omitted_decision_fields() -> None: + """A bare note (scope='note') with no structured fields builds cleanly.""" + req = NoteRequest(text="just an observation") + assert req.context == "" + assert req.chosen == "" + assert req.rationale == "" + assert req.what_done == "" + assert req.what_learned == "" + assert req.what_struggled == "" + + +def test_note_request_rejects_null_context() -> None: + """Passing literal null for context fails Pydantic validation.""" + with pytest.raises(ValidationError) as exc_info: + NoteRequest.model_validate({"text": "x", "scope": "decision", "context": None}) + msg = str(exc_info.value) + assert "context" in msg.lower() + + +def test_note_request_rejects_null_chosen() -> None: + with pytest.raises(ValidationError) as exc_info: + NoteRequest.model_validate({"text": "x", "scope": "decision", "chosen": None}) + assert "chosen" in str(exc_info.value).lower() + + +def test_note_request_rejects_null_rationale() -> None: + with pytest.raises(ValidationError) as exc_info: + NoteRequest.model_validate( + {"text": "x", "scope": "decision", "rationale": None} + ) + assert "rationale" in str(exc_info.value).lower() + + +def test_note_request_rejects_null_what_done() -> None: + with pytest.raises(ValidationError) as exc_info: + NoteRequest.model_validate({"text": "x", "scope": "reflect", "what_done": None}) + assert "what_done" in str(exc_info.value).lower() + + +def test_note_request_rejects_null_what_learned() -> None: + with pytest.raises(ValidationError) as exc_info: + NoteRequest.model_validate( + {"text": "x", "scope": "reflect", "what_learned": None} + ) + assert "what_learned" in str(exc_info.value).lower() + + +def test_note_request_rejects_null_what_struggled() -> None: + with pytest.raises(ValidationError) as exc_info: + NoteRequest.model_validate( + {"text": "x", "scope": "reflect", "what_struggled": None} + ) + assert "what_struggled" in str(exc_info.value).lower() + + +def test_note_request_accepts_non_empty_strings() -> None: + """A fully-filled decision request validates.""" + req = NoteRequest.model_validate( + { + "text": "Going with X", + "scope": "decision", + "context": "We have a choice between A and B", + "options": [ + {"name": "A", "pros": "fast", "cons": "fragile"}, + {"name": "B", "pros": "robust", "cons": "slow"}, + ], + "chosen": "A", + "rationale": "speed beats robustness for this experiment", + } + ) + assert req.context == "We have a choice between A and B" + assert req.chosen == "A" + assert req.rationale.startswith("speed")