[fix] Refuse a malformed harness kind with a structured error at commit and invoke (F4) - #6409
Conversation
A harness.kind the runtime cannot read (a number, an unknown string) was accepted by the commit API with a 200, which persisted a config that could never run. Invoking it then died on the enum's bare ValueError as an unhandled HTTP 500 whose body was the Python repr, naming neither the field nor the values a caller could have sent. Both boundaries now refuse it with a shape. HarnessKind.coerce raises a coded 400 that names the field, the value, and every harness that exists, so the invoke path fails closed with an error a client can act on; it stays a ValueError so existing guards keep working. The workflow commit path checks the candidate revision and refuses to store an unrunnable agent config, answering 422 with the agent-actionable envelope on the route and the same envelope inside the agent's own commit tool. An absent, null, or blank kind still means "use the default", unchanged. Claude-Session: https://claude.ai/code/session_0165tsjmvf3qvTPFcb9EV44g
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (6)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe SDK now rejects unreadable ChangesHarness kind validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: 🟡 Moderate · up to Malformed nonblank harness values now receive structured errors before persistence or invocation, but whitespace-only values can still be committed and later fail during execution, leaving the workflow unavailable. Merge should wait for this validation contract mismatch to be fixed or explicitly accepted. Sequence Diagram(s)sequenceDiagram
participant AgentTemplate
participant HarnessKind_coerce
participant commit_workflow_revision_checked
participant commit_revision
participant FastAPI
AgentTemplate->>HarnessKind_coerce: validate explicit harness.kind
HarnessKind_coerce-->>AgentTemplate: InvalidHarnessKindError for invalid values
commit_workflow_revision_checked->>HarnessKind_coerce: validate candidate harness.kind
HarnessKind_coerce-->>commit_workflow_revision_checked: validation result
commit_workflow_revision_checked->>commit_revision: persist valid candidate
commit_workflow_revision_checked-->>FastAPI: InvalidAgentHarnessError
FastAPI-->>FastAPI: return structured HTTP 422
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Team
Run ID: 1417e198-e5b5-4c4b-bcfd-250b073202db
📒 Files selected for processing (10)
api/oss/src/apis/fastapi/workflows/router.pyapi/oss/src/core/tools/platform_handlers.pyapi/oss/src/core/workflows/service.pyapi/oss/tests/pytest/unit/workflows/test_commit_endpoint.pyapi/oss/tests/pytest/unit/workflows/test_commit_harness_validation.pysdks/python/agenta/sdk/agents/__init__.pysdks/python/agenta/sdk/agents/dtos.pysdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.pysdks/python/oss/tests/pytest/unit/agents/test_dtos_harness_kind_refusal.pyservices/oss/tests/pytest/unit/agent/test_template_shape_validation.py
Included review availability: Your plan provides up to 8 included reviews per hour; 2 remain after this review.
| } | ||
|
|
||
|
|
||
| class InvalidAgentHarnessError(Exception): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move InvalidAgentHarnessError to the workflow type module.
InvalidAgentHarnessError is a workflow-domain exception that FastAPI and platform layers import. Define it in api/oss/src/core/workflows/types.py or api/oss/src/core/workflows/dtos.py, then import it into the service and boundaries.
As per coding guidelines, “Define domain exceptions in the core layer (core/{domain}/types.py or core/{domain}/dtos.py).”
Source: Coding guidelines
…kind a 422 Two review findings, both reproduced before fixing. Parsing a template stringified the caller's value after coercing it, so the SDK's own HarnessKind member became "harnesskind.claude" (str() on a str Enum gives the member repr, not the value) and was then refused by make_harness. A member now resolves to its wire value, while a string still keeps its stored spelling so a legacy pi_agenta revision normalizes exactly where it always did. The commit refusal echoed the offending value verbatim, and Python's json parser accepts the non-standard NaN and Infinity literals in a request body while Starlette serializes with allow_nan=False. A caller sending one was refused correctly and then got a 500 raised inside the response, which is the failure this boundary exists to replace. Non-finite floats are now echoed as text. Claude-Session: https://claude.ai/code/session_0165tsjmvf3qvTPFcb9EV44g
|
Both P2s reproduced against the source, and both are fixed in 1. The coerced member was discarded — real, and worse than the reportConfirmed exactly as described. So the SDK's own enum was not accepted by the SDK's own parser. The mangling predates this PR (the old line stringified the same way), but this PR made it worse in one specific sense: it used to die at Fixed, but not by using Tests: every 2. Non-finite floats broke the response — realConfirmed in both halves: And end to end through the real code, before the fix: Fixed with a Tests: VerificationBoth fixes were reverted in place and the suites re-run: 5 failures, exactly the new cases (3 enum/SDK, 2 non-finite router), and nothing else. Restored, all green. Full suites: API 2880 passed, SDK 2517 passed, services 162 passed. |
1 participant
The defect
Found live by the gate's new bad-harness cell (H1). A malformed
harness.kind(12345,"not_a_real_harness") was ACCEPTED by the commit API with a 200, which persisted an agent config that can never run. Invoking that revision then died on an unhandled HTTP 500 whose body was the bare Python repr:'12345' is not a valid HarnessKind. The caller learned neither which field was wrong nor which values exist, and got no code to branch on.The trace:
HarnessKind.coerce(sdks/python/agenta/sdk/agents/dtos.py) didstr(value).lower()and thencls(normalized), so a non-member raised the enum's plainValueError. Nothing abovemake_harness(sdks/python/agenta/sdk/agents/adapters/harnesses.py:158) caught it, andhandle_invoke_failuremaps an unclassified exception to 500 with a stack trace. Nothing at the commit boundary looked at the field at all.The core invariant did hold throughout: no turn ran, no rows were stored, and the harness was never silently defaulted to Pi. This changes the refusal SHAPE and stops an unrunnable config from being stored.
The fix, at both boundaries
Commit —
api/oss/src/core/workflows/service.py. A new_reject_unreadable_harness_kindruns on the CANDIDATE revision insidecommit_workflow_revision_checked, which is the one point both commit forms pass through: a delta has already been merged onto the head by then, and a full-data commit is the data as sent. It raises the domain exceptionInvalidAgentHarnessError, which carries the agent-actionable envelope (code,message,retryable: false,next_step,detailswith the field, the value, and the allowed harnesses). The route maps it to 422, the same status every other "your change is not committable" answer on that route uses, and the agent's own commit tool returns the identical envelope as anAgentErrorrather than raising.The check is deliberately narrow. It reads one field, only when the commit carries it, so a workflow that is not an agent takes exactly its old path.
Invoke —
sdks/python/agenta/sdk/agents/dtos.py.HarnessKind.coercenow raisesInvalidHarnessKindError, anErrorStatuswith code 400 and type...#v0:agent:invalid-harness-kind, whose message names the field, the value with its type, and every harness that exists.AgentTemplate.from_paramsvalidates a present kind where the template is read, which is before the handler selects a backend or resolves anything, so a config persisted before this fix now fails closed with a code instead of a 500.Two deliberate choices worth a reviewer's eye:
ValueError. The enum has always raised one there and callers (including an existing SDK test) guard on that type, so inheriting both keeps those guards working while the middleware renders the coded status. A dedicatedErrorStatuswas chosen over theagent_run_failedcode the brief suggested: this is a malformed request, not a run that failed, and it matches the neighbouringAgentTemplateShapeErrorin the same file, which is also a coded 400. Either satisfies the H1 cell, which accepts any 4xx or coded frame whose text names the harness.make_harnesscall site. Withcoercetyped, the only remaining bareValueErrorthere is the unreachable branch for an enum member with no adapter class, which is our bug, not the caller's. Relabelling it as a 400 would blame the caller for a server defect, so it stays a 500.An absent, null, or blank kind still means "use the default" at both boundaries, unchanged, because that is what every config that never set a harness relies on. A legacy
pi_agentarevision still reads as plain Pi.Tests
Four files, at each boundary:
sdks/python/oss/tests/pytest/unit/agents/test_dtos_harness_kind_refusal.py(new, 26 cases): coerce refuses12345, an unknown string,0,[],{}; the message names the value and every harness; it is still aValueError; readable kinds andpi_agentaare unchanged; absent/null/blank still default; the invoke remap answers 400 with the field, and the bareValueErrorit replaced is pinned as the 500 it used to be.sdks/python/oss/tests/pytest/unit/agents/test_agent_composition_seam.py: a pre-persisted bad config driven through the real handler refuses with the coded error, and no session is created, so no turn can be stored.services/oss/tests/pytest/unit/agent/test_template_shape_validation.py: the same refusal through the deployed agent service handler.api/oss/tests/pytest/unit/workflows/test_commit_harness_validation.py(new, 24 cases) andtest_commit_endpoint.py: the values refused, the far larger set of commits left alone (no agent, no harness, no kind, prompt-only), the envelope's contents, the 422 mapping, and the DAO never being awaited, which is the "nothing is persisted" proof.Results: API unit suite 2872 passed, SDK unit suite 2512 passed, services unit suite 162 passed.
ruff formatandruff checkclean withruff@0.15.12, the version CI pins. One pre-existing failure,sdks .../agents/test_streaming.py::test_cli_stream_terminal_only_on_empty_request, fails identically with every change in this branch stashed, so it is not from this work.Both new suites were verified to fail without the fix: removing the commit-path call makes the "nothing is persisted" case fail, and the SDK cases cannot even import without the new error type.
https://claude.ai/code/session_0165tsjmvf3qvTPFcb9EV44g