ENG-816 - AgentCall.get() returns a sentinel None; every caller re-derives the same 404 guard - #189
ENG-816 - AgentCall.get() returns a sentinel None; every caller re-derives the same 404 guard#189druks-operator[bot] wants to merge 1 commit into
Conversation
There was a problem hiding this comment.
Verdict: approve
Every acceptance criterion is satisfied by the diff, and the backend suite is green locally.
AC1 — druks/durable/exceptions.py:18 defines AgentCallNotFound(Exception) with the No agent call <id>. message; AgentCall.get() (druks/durable/models.py:493) is now typed -> "AgentCall", reads the row once, and raises. test_agent_call_get_raises_for_missing_call covers the raise and pins the message.
AC2 — rg 'AgentCall\.get\(' returns exactly the seven production sites named in the plan (gateway services x2, extensions/base x2, durable/reads x2, api/artifacts). None branches on the return value; every one either propagates or catches. Test call sites (test_agents.py:247, test_cost_capture.py:88/101) pass real IDs and are unaffected.
AC3 — get_agent_call_files returns None on catch (reads.py:38), stream_transcript returns out of the async iterator inside the session scope (reads.py:201, keeping the original comment), and _artifact_content returns None (services.py:93). test_transcript_stream_unknown_call_closes still asserts the HTTP 200 empty-body contract and passes.
AC4 — mcp/gateway/routes.py:53 translates the durable error into the existing wire AgentCallNotFound, so REST and MCP serialization are unchanged; the durable model stays free of AgentApiError. test_missing_agent_call_returns_wire_error pins the exact body, test_mcp_endpoint.py:294 still pins the MCP code, and the new test_api_runs.py cases pin Run not found. / Agent call not found..
AC5 — model and service tests now expect druks.durable.exceptions.AgentCallNotFound; route and MCP tests still assert the AGENT_CALL_NOT_FOUND wire shape.
The approach matches what the plan specified: durable-package-internal exception, no export from druks.durable.__init__, no AgentApiError inheritance, and no scope creep into other models' nullable get() methods. The diff touches no dependencies, lockfiles, generated files, or config.
Checks
uv run ruff check backend— pass ("All checks passed!").uv run pytest backend/— pass, 1121 passed (needed local Postgres + Redis containers, per AGENTS.md).uv pip install -e backend/tests/druks-field_notes— pass.uv run pytest backend/tests/test_proof_extension*.py— pass, 5 passed.npm --prefix frontend run lint,npm --prefix frontend run build,npm --prefix frontend test— not run: no Node/npm in this sandbox. No CI check exists onf59b4a9to substitute. The diff changes zero frontend files, so this is a verification gap, not evidence of a problem.
Open findings
backend/druks/api/artifacts.py:20— the newexcept AgentCallNotFoundbranch has no focused test;test_get_artifact_404_when_content_goneseeds a real call and hits the missing-file branch instead. See the inline comment.backend/druks/mcp/gateway/services.py:93— same gap for the artifact-projection handler. Both guard a state theondelete="CASCADE"FK makes unreachable through the DB, so a monkeypatchedAgentCall.getis the only practical way to cover them.
Push back on any of this if you read it differently.
| path = call.get_file_path(artifact.path) if call else None | ||
| try: | ||
| call = AgentCall.get(artifact.agent_call_id) | ||
| except AgentCallNotFound as error: |
There was a problem hiding this comment.
Non-blocking: this except AgentCallNotFound branch has no focused test. test_get_artifact_404_when_content_gone seeds a real AgentCall, so it exercises the missing-file branch below, not this one. The same applies to the except in druks/mcp/gateway/services.py:_artifact_content.
Both branches are hard to reach through the DB — Artifact.agent_call_id is a ForeignKey(..., ondelete="CASCADE"), so an artifact never outlives its call — which is why I am not blocking. If you want the coverage, the cheapest route is monkeypatching AgentCall.get to raise, in a sibling of test_get_artifact_404_when_content_gone (and one in test_agent_services.py for the gateway projection). Leaving both as untested defensive guards is also defensible — push back if you disagree.
|
Code review: Replaces AgentCall.get()'s None-sentinel with a typed AgentCallNotFound, updates all seven call sites and adds coverage for each — clean, scoped exactly to ENG-816, and the wire-level 404 shape is preserved end to end. |
Linear ticket: ENG-816
Plan
Approach
Replace the nullable model lookup with a durable-domain exception, then handle that exception only where absence is an intentional read result or where an API boundary must translate it. The existing
druks.mcp.gateway.exceptions.AgentCallNotFoundremains the agent API's wire error; the durable model must not depend on the HTTP/API layer.Implementation
backend/druks/durable/exceptions.py, addAgentCallNotFound(Exception)with an initializer acceptingagent_call_id: strand producingNo agent call <id>.. Keep it internal to the durable package rather than expandingdruks.durable.__init__'s author-facing exports.backend/druks/durable/models.py, changeAgentCall.get()from-> "AgentCall | None"to-> "AgentCall". Read the row once, raise the durableAgentCallNotFound(agent_call_id)when absent, and otherwise return the row.backend/druks/mcp/gateway/services.py:get_agent_call: remove the nullable guard and allow the durable exception to reach the route boundary.backend/druks/mcp/gateway/services.py:_artifact_content: catch the durable exception and returnNone, preserving the existing omitted-artifact behavior.backend/druks/extensions/base.py:get_transcript: catch the durable exception at the endpoint and preserve HTTP 404 detailRun not found..backend/druks/extensions/base.py:get_file: catch it at the endpoint and preserve HTTP 404 detailAgent call not found..backend/druks/durable/reads.py:get_agent_call_files: catch it explicitly and returnNone, which the existing files endpoint translates to its 404.backend/druks/durable/reads.py:stream_transcript: catch it inside the polling session and end the async iterator, preserving the current HTTP 200 empty SSE response for an unknown/not-yet-present call.backend/druks/api/artifacts.py:get_artifact: catch it at the endpoint and raise the existing HTTP 404artifact content missing; after a successful lookup, access the call directly and retain the separate missing-file check.backend/druks/mcp/gateway/routes.py:get_agent_call, catchdruks.durable.exceptions.AgentCallNotFoundand raise the existingdruks.mcp.gateway.exceptions.AgentCallNotFound(call_id)from it. This preserves both REST and MCP serialization without coupling the durable model toAgentApiError.backend/tests/test_agent_services.py, add direct coverage that a missingAgentCall.get()raises the durable exception, and update the service-level missing-call expectation to that domain type.backend/tests/test_agent_routes.py, pin the missing agent-call REST response to HTTP 404 and the exactAGENT_CALL_NOT_FOUNDbody.backend/tests/test_api_runs.py, retain the existing unknown-call SSE test and add focused missing-call assertions for the transcript range, file inventory, and file-download endpoints so their current 404 behavior is protected.backend/tests/test_mcp_endpoint.pyand artifact-content 404 coverage inbackend/tests/test_artifacts.pyas wire-contract regression coverage.Preserved wire examples
{"code":"AGENT_CALL_NOT_FOUND","message":"No agent call no-such-call.","retryable":false}{"error":"HTTP_404","detail":"artifact content missing"}An unknown transcript stream remains HTTP 200 with an empty response body.
Out of scope
get()methods; this PR is limited toAgentCall.get()and its current callers.Ruled out
AgentCallNotFoundinheritAgentApiErrorwas rejected because it would couple persistence/domain code to the HTTP agent surface and would force read-side polling paths into wire-error semantics.get_agent_call_files, artifact projection, and especially the transcript SSE iterator intentionally treat absence as an empty/closed read; changing the stream to a 404 would break its existing HTTP 200 empty-body contract.get()was rejected because those methods have separate caller contracts and would broaden this focused change into an independently reviewable repository-wide behavior migration.Noneand centralizing the repeated truthiness check in a helper was rejected becauseAgentCall.get()would still expose an unchecked failure sentinel, allowing future callers to omit not-found handling and preserving the underlying contract defect.Acceptance Criteria
backend/druks/durable/exceptions.pydefinesAgentCallNotFound, andAgentCall.get(agent_call_id: str) -> AgentCallraises it with the missing call identifier instead of returningNone.AgentCall.get()with an absent ID and assertsAgentCallNotFound.AgentCall.get()call sites are converted from return-value truthiness guards to exception-based control flow; no call site performsif not call:or an equivalent nullable check against its result.AgentCall.get(and inspect the seven resulting sites in gateway services, extension routes, durable reads, and the artifact endpoint.get_agent_call_files()returnsNone,stream_transcript()closes without events, and gateway artifact projection returnsNonewhenAgentCallNotFoundis caught.GET /api/agent-calls/no-such-callreturns HTTP 404 with{"code":"AGENT_CALL_NOT_FOUND","message":"No agent call no-such-call.","retryable":false}; extension transcript/file routes retain their current 404 details; an artifact whose content cannot be resolved retains{"error":"HTTP_404","detail":"artifact content missing"}.druks.durable.exceptions.AgentCallNotFound, while route and MCP tests continue asserting theAGENT_CALL_NOT_FOUNDwire response.backend/tests/test_agent_services.py,backend/tests/test_agent_routes.py,backend/tests/test_mcp_endpoint.py, andbackend/tests/test_api_runs.py.