mirror of
https://github.com/rennf93/roboco.git
synced 2026-08-03 07:23:24 +02:00
fix(gateway): reject null + DO-NOT-PASS-NULL remediate for decision/reflect
Smoke-6 found the agent calling note(scope='decision', context=null, chosen=null, rationale=null) eight times in a row. Root cause split across two surfaces: 1. The MCP tool schema declared these fields as `str | None = None`, producing a JSON schema of `anyOf [string, null]`. minimax-m2.7 read that and decided null was a valid value — passed it on every retry. 2. The remediate text used `<placeholder>` syntax for the example without telling the agent "don't pass null" explicitly. Fixes: - roboco/api/schemas/v2/do.py NoteRequest: context, chosen, rationale, what_done, what_learned, what_struggled now typed `str = ""` (no None). Pydantic on the route rejects literal null with 422 BEFORE the gateway sees it. Empty string still counts as missing at the gate. - roboco/mcp/do_server.py note(): matching signature changes so the MCP tool schema declares the fields as `string` not `anyOf[string,null]`. - roboco/services/gateway/content_actions.py: remediate text now opens with "DO NOT pass null" and the example uses concrete values (redis vs postgres) instead of <angle bracket> placeholders. Reflect remediate also gets the don't-pass-null intro and concrete values. 8 new tests pin "schema rejects null for each of the 6 string fields" plus "empty defaults work for unscoped notes".
This commit is contained in:
@@ -31,16 +31,21 @@ class NoteRequest(BaseModel):
|
|||||||
scope: str = "note"
|
scope: str = "note"
|
||||||
task_id: UUID | None = None
|
task_id: UUID | None = None
|
||||||
title: str | None = None
|
title: str | None = None
|
||||||
# decision scope (all required at gateway when scope='decision')
|
# decision scope (all required at gateway when scope='decision').
|
||||||
context: str | None = None
|
# 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}, ...]
|
options: list[dict[str, str]] | None = None # [{name, pros, cons}, ...]
|
||||||
chosen: str | None = None
|
chosen: str = ""
|
||||||
rationale: str | None = None
|
rationale: str = ""
|
||||||
consequences: list[str] | None = None
|
consequences: list[str] | None = None
|
||||||
# reflect scope (what_done/learned/struggled required when scope='reflect')
|
# reflect scope (what_done/learned/struggled required when scope='reflect')
|
||||||
what_done: str | None = None
|
what_done: str = ""
|
||||||
what_learned: str | None = None
|
what_learned: str = ""
|
||||||
what_struggled: str | None = None
|
what_struggled: str = ""
|
||||||
next_steps: list[str] | None = None
|
next_steps: list[str] | None = None
|
||||||
|
|
||||||
|
|
||||||
|
|||||||
@@ -179,14 +179,14 @@ def note(
|
|||||||
scope: str = "note",
|
scope: str = "note",
|
||||||
task_id: str | None = None,
|
task_id: str | None = None,
|
||||||
title: str | None = None,
|
title: str | None = None,
|
||||||
context: str | None = None,
|
context: str = "",
|
||||||
options: list[dict[str, str]] | None = None,
|
options: list[dict[str, str]] | None = None,
|
||||||
chosen: str | None = None,
|
chosen: str = "",
|
||||||
rationale: str | None = None,
|
rationale: str = "",
|
||||||
consequences: list[str] | None = None,
|
consequences: list[str] | None = None,
|
||||||
what_done: str | None = None,
|
what_done: str = "",
|
||||||
what_learned: str | None = None,
|
what_learned: str = "",
|
||||||
what_struggled: str | None = None,
|
what_struggled: str = "",
|
||||||
next_steps: list[str] | None = None,
|
next_steps: list[str] | None = None,
|
||||||
) -> dict[str, Any]:
|
) -> dict[str, Any]:
|
||||||
"""Write a journal entry. scope in note|decision|reflect|learning|struggle.
|
"""Write a journal entry. scope in note|decision|reflect|learning|struggle.
|
||||||
|
|||||||
@@ -375,32 +375,43 @@ class ContentActions:
|
|||||||
if missing:
|
if missing:
|
||||||
if scope == "decision":
|
if scope == "decision":
|
||||||
remediate = (
|
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: "
|
"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"
|
"note(\n"
|
||||||
" scope='decision',\n"
|
" scope='decision',\n"
|
||||||
" text='<one-line summary of the decision>',\n"
|
" text='Going with redis for the queue',\n"
|
||||||
" context='<the situation that led to it>',\n"
|
" context='Need a queue for background work',\n"
|
||||||
" options=[\n"
|
" options=[\n"
|
||||||
" {'name': 'optionA', 'pros': '<pros>', 'cons': '<cons>'},\n"
|
" {'name': 'redis', 'pros': 'fast', 'cons': 'ephemeral'},\n"
|
||||||
" {'name': 'optionB', 'pros': '<pros>', 'cons': '<cons>'},\n"
|
" {'name': 'postgres', 'pros': 'durable', 'cons': 'slower'},\n"
|
||||||
" ],\n"
|
" ],\n"
|
||||||
" chosen='<which option>',\n"
|
" chosen='redis',\n"
|
||||||
" rationale='<why this option — cite trade-offs>',\n"
|
" rationale='speed beats durability for this experiment',\n"
|
||||||
")\n\n"
|
")\n\n"
|
||||||
"Pre-gateway parity — these populate the panel's Decisions view."
|
"Pre-gateway parity — these populate the panel's Decisions view."
|
||||||
)
|
)
|
||||||
else:
|
else:
|
||||||
remediate = (
|
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: "
|
"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"
|
"note(\n"
|
||||||
" scope='reflect',\n"
|
" scope='reflect',\n"
|
||||||
" text='<one-line summary of the reflection>',\n"
|
" text='Smoke test pass on PR #17',\n"
|
||||||
" what_done='<what shipped, where (file:line / commit)>',\n"
|
" what_done='Edited README.md L42 and committed [abc12345]',\n"
|
||||||
" what_learned='<new info you didn't have before>',\n"
|
" what_learned='The submit_for_qa verb requires reflect',\n"
|
||||||
" what_struggled='<where you got stuck — even briefly>',\n"
|
" what_struggled='Initial commit message under 20 chars',\n"
|
||||||
" next_steps=['<follow-up #1>', '<follow-up #2>'],\n"
|
" next_steps=['Wait for QA review'],\n"
|
||||||
")\n\n"
|
")\n\n"
|
||||||
"Pre-gateway parity — these populate the panel's Reflections view."
|
"Pre-gateway parity — these populate the panel's Reflections view."
|
||||||
)
|
)
|
||||||
|
|||||||
@@ -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")
|
||||||
Reference in New Issue
Block a user