Follow-up from #173: 1 item(s) (safety)

[mvillmow]

Consolidated follow-up from implementation of #173.

Each section below lists scope-checked follow-up items discovered during implementation. Items are restricted to core library defects, security, safety hazards, or critical functional bugs.

Safety

Dispatcher.call_tool raises bare KeyError on missing team_id argument

mcp_server.py:82 does str(arguments["team_id"]) with no guard. The MCP SDK passes the raw arguments dict without validating it against the inputSchema, so a client that omits team_id causes an unhandled KeyError that propagates through _call_tool in build_server() and crashes the stdio server process rather than returning a structured MCP error. Fix: catch KeyError (or use arguments.get) and raise a descriptive ValueError (or return a TextContent error response) so the server stays alive across bad requests.

---

The implementer also considered the items below and rejected them as out of scope; they are recorded in the PR body, not filed as separate issues.

[mvillmow]

Implementation Plan

I have everything needed. Now I'll determine the exact line numbers (the issue says line 82, but the actual arguments["team_id"] is line 81 in the current branch tip — let me note the branch may have diverged slightly; I'll cite by symbol). The fix follows the error-translation skill: catch KeyError, raise a descriptive ValueError. There's an existing UnknownToolError(ValueError) pattern to mirror.

Here is the plan.

---

Implementation Plan

Objective

Dispatcher.call_tool in the read-only MCP server does team_id = str(arguments["team_id"]) with no guard (src/telemachy/mcp_server.py:81 on branch 173-auto-impl). The MCP SDK forwards the raw arguments dict to _call_tool without validating it against the tool's inputSchema, so a client that invokes agamemnon_list_team_tasks while omitting team_id triggers an unhandled KeyError that propagates out of build_server()'s _call_tool and crashes the stdio server process. The fix translates the low-level KeyError at this boundary into a descriptive domain ValueError, so a malformed request fails with a structured, actionable message and the server stays alive across bad requests.

Approach

• Scope is branch 173-auto-impl, not main. git show --stat 07ea99a confirms src/telemachy/mcp_server.py and tests/test_mcp_server.py were added by commit 07ea99a ("feat: read-only MCP server for Agamemnon state queries") which lives only on 173-auto-impl/origin/173-auto-impl (git branch -a --contains 07ea99a). The file does not exist on main. This fix must be branched from / target 173-auto-impl (or be rebased onto whatever branch carries mcp_server.py at merge time); a branch off main would have no file to edit.
• Translate the error at the boundary; don't swallow it. This is the error-translation pattern from resilience-circuit-breaker-error-translation: catch the low-level KeyError (an implementation detail of dict access) and re-raise a domain-level ValueError with raise ... from exc so the cause chain is preserved for debugging. Returning None or a silent default would mask the malformed request (a rejected "Failed Attempt" in that skill).
• Mirror the module's existing exception idiom. The module already defines class UnknownToolError(ValueError) (mcp_server.py:58) for the sibling bad-request case (unknown tool name). I add a parallel class MissingArgumentError(ValueError) for the missing-required-argument case. Both subclass ValueError, so the SDK's _call_tool surfaces them as the same structured error class category, and existing tests that catch ValueError/UnknownToolError semantics stay consistent.
• Why a raised ValueError is sufficient (not a returned TextContent). The issue offers either. The unknown-tool path already raises UnknownToolError(ValueError) rather than returning an error TextContent, and the MCP SDK's @server.call_tool() wrapper catches handler exceptions and converts them to a structured MCP isError tool result — it does not crash the process for a raised exception, only for one that escapes the framework's own try/except. A bare KeyError is just as caught-and-structured by the SDK as a ValueError would be; the real defect the issue describes is the bare, non-descriptive KeyError (opaque 'team_id' message), not that an exception is raised at all. Raising a descriptive ValueError keeps this path symmetric with UnknownToolError and gives the client an actionable message. I keep the fix consistent with the file's established convention rather than introducing a second error-reporting mechanism.
• Dispatcher is the unit-testable seam. Per the module docstring and test_build_server_is_thin_adapter..., Dispatcher has no mcp SDK dependency and is tested directly with an AsyncMock client. The fix lives entirely in Dispatcher.call_tool, so it is fully testable on a clean checkout with no MCP SDK installed (the existing tests already import only Dispatcher/_TOOLS/UnknownToolError).

Files to Create

None.

Files to Modify

src/telemachy/mcp_server.py

Change 1 — add a domain exception next to the existing UnknownToolError (mcp_server.py:58-59):

class UnknownToolError(ValueError):
    """Raised when call_tool() receives a name not in _TOOLS."""


class MissingArgumentError(ValueError):
    """Raised when call_tool() is missing a required tool argument."""

Change 2 — guard the team_id lookup in Dispatcher.call_tool (replace the body of the agamemnon_list_team_tasks branch, currently mcp_server.py:80-82):

        if name == "agamemnon_list_team_tasks":
            try:
                team_id = str(arguments["team_id"])
            except KeyError as exc:
                raise MissingArgumentError(
                    "agamemnon_list_team_tasks requires a 'team_id' argument"
                ) from exc
            tasks = await self._client.get_tasks(team_id)
            return json.dumps(tasks, indent=2)
        raise UnknownToolError(f"unknown tool: {name}")

(Note: the issue cites line 82; on the current branch tip arguments["team_id"] is line 81 — cite by symbol Dispatcher.call_tool to stay robust to minor drift.)

tests/test_mcp_server.py

Add MissingArgumentError to the import and add one regression test next to test_call_tool_unknown_name_raises (tests/test_mcp_server.py:48):

from telemachy.mcp_server import (
    _TOOLS,
    Dispatcher,
    MissingArgumentError,
    UnknownToolError,
)

@pytest.mark.asyncio
async def test_call_tool_missing_team_id_raises_descriptive_error() -> None:
    """Omitting the required team_id must raise a descriptive ValueError,
    not a bare KeyError that would crash the stdio server."""
    client = AsyncMock()
    dispatcher = Dispatcher(client)
    with pytest.raises(MissingArgumentError, match="team_id"):
        await dispatcher.call_tool("agamemnon_list_team_tasks", {})
    # The client must never be touched when the request is malformed.
    client.get_tasks.assert_not_awaited()


@pytest.mark.asyncio
async def test_missing_argument_error_is_value_error() -> None:
    """MissingArgumentError stays a ValueError so the SDK/_call_tool surfaces
    it as a structured bad-request, consistent with UnknownToolError."""
    assert issubclass(MissingArgumentError, ValueError)

Implementation Order

1. Branch from the carrier branch, not main. git fetch origin 173-auto-impl then create a fix branch off origin/173-auto-impl (e.g. 283-mcp-missing-team-id). Confirm src/telemachy/mcp_server.py exists in the working tree before editing.
2. Add MissingArgumentError(ValueError) immediately after UnknownToolError in mcp_server.py.
3. Wrap the arguments["team_id"] access in Dispatcher.call_tool with try/except KeyError → raise MissingArgumentError(...) from exc (Change 2 above).
4. Update tests/test_mcp_server.py imports and add the two new tests.
5. Run the test suite (just test / pixi run pytest) and confirm the new tests pass and the existing six tests still pass.
6. Run just lint (ruff) to confirm no style regressions.
7. Open a PR targeting 173-auto-impl (per Agent Guardrails: never commit to main; squash-merge via GitHub UI). Reference issue Follow-up from #173: 1 item(s) (safety) #283 and note in the PR body that the fix targets the branch carrying the MCP server, since it is not yet on main.

Verification

Acceptance criterion (issue #283): missing team_id no longer raises a bare KeyError; a descriptive ValueError is raised instead and the client is not called.

# Criterion 1: missing team_id raises descriptive ValueError (not KeyError), client untouched
pixi run pytest tests/test_mcp_server.py -k missing_team_id -v

# Criterion 2: the new error type is a ValueError (structured bad-request, like UnknownToolError)
pixi run pytest tests/test_mcp_server.py -k missing_argument_error_is_value_error -v

# Regression: the happy path with a valid team_id still delegates correctly, plus full module suite
pixi run pytest tests/test_mcp_server.py -v

# Lint clean
pixi run ruff check src/telemachy/mcp_server.py tests/test_mcp_server.py

(If just is preferred per CLAUDE.md Common Commands: just test and just lint cover the same.)

Skills Used

• resilience-circuit-breaker-error-translation (team KB) — applied the boundary error-translation pattern: catch the low-level KeyError, re-raise a domain ValueError with raise ... from exc to preserve the cause chain; do not swallow/return None.
• silent-boundary-observability-exception-classification (team KB) — considered. Its fail-safe "return None, never block, log and classify" contract does not apply here: call_tool is a request handler that must report a malformed request back to the caller, not silently continue. Raising a descriptive, SDK-structured ValueError is the correct boundary behavior; swallowing to None would mask the bad request. Noted for why the broad-catch/return-None approach was rejected.
• architecture-executable-convention-guard-pattern (team KB) — informed the "validate required field before proceeding" framing: the inputSchema declares team_id required, but nothing enforced it at runtime; the guard makes that documented invariant executable, and the test asserts the client is never reached on a malformed request.

Changes from Review

N/A — initial plan.

---

Final review verdict (from strict review loop)

This is decisive. The installed mcp SDK's call_tool decorator (server.py:583-584) wraps the handler in try/except Exception as e: return self._make_error_result(str(e)). So a raised KeyError (or ValueError) is caught and converted into a structured isError result — it does NOT crash the stdio server process.

This means:

• The issue's stated failure mode is factually wrong: a bare KeyError does NOT crash the server; the SDK already catches it into a structured error.
• The plan's reinterpretation ("the real defect is the opaque message, not a crash") turns out to be correct and is now verified against the installed SDK — resolving the plan's own highest-risk assumption in the plan's favor.

The fix (descriptive ValueError) still has genuine value: _make_error_result(str(e)) surfaces the exception's str() to the client. A bare KeyError's str is "'team_id'" (opaque); the new MissingArgumentError's str is actionable. So the fix improves the client-facing error message — a real, if narrower, improvement than the issue claims.

Now let me evaluate the plan against the rubric.

Assessment:

Requirements alignment — The single acceptance criterion (missing team_id → descriptive ValueError not bare KeyError, client untouched) is named and addressed with a concrete step (Change 2). ✓ The plan correctly notes line drift (82 vs 81 — verified: actual is the line with team_id = str(arguments["team_id"])).

Plan completeness — Design rationale, exact code, two tests, implementation order, verification commands, lint. ✓

Concreteness — Names mcp_server.py:58-59, Dispatcher.call_tool, UnknownToolError, exact diff, test names. ✓ Excellent.

Risk surface — Correctly identifies the branch-scoping risk (file only on 173-auto-impl, not main) with git evidence, and Agent Guardrails (never target main). ✓

Verification plan — Three named pytest invocations + lint, mapped to criteria. ✓

Stage handoff — Implementer can act without re-deriving. ✓

Principles:

• P1/KISS, P2/YAGNI: Minimal — one exception class mirroring the existing one, one try/except, two tests. No scope creep. ✓
• P3/TDD: Two tests, one asserting behavior (descriptive raise + client untouched), one asserting the type contract. Proportional. ✓
• P4/DRY: Mirrors UnknownToolError idiom rather than inventing a new mechanism. ✓
• P7/POLA: Keeps the path symmetric with UnknownToolError (raise, don't return TextContent). Justified well. ✓

Minor findings:

1. The plan accepts the issue's "crashes the stdio server process" framing in its Objective without flagging it as unverified/incorrect — though it does hedge correctly later ("a bare KeyError is just as caught-and-structured by the SDK as a ValueError"). The plan reached the right design conclusion despite the issue's wrong premise. The risk it flagged ("if false, raising ValueError alone may not keep the server alive") is now resolved in its favor by SDK inspection — but the plan left it unverified. Minor: the implementer should verify the SDK behavior (which I've now done), but the plan's design is robust either way.
2. The two-test split is slightly redundant — test_call_tool_missing_team_id_raises_descriptive_error already implies a ValueError via MissingArgumentError; the separate issubclass test is borderline trivial. Not a real defect; acceptable belt-and-suspenders consistent with the file's existing style (test_module_has_no_write_method_references).

No critical or major findings. The plan is concrete, correctly scoped, guardrail-aware, and its design conclusion holds even though the issue's failure-mode description is technically inaccurate. The plan correctly chose the right fix regardless.

• The plan's design is correct and verified against the installed SDK (server.py:583): exceptions become structured isError results; the descriptive ValueError improves the client-facing message via _make_error_result(str(e)).
• Branch-scoping risk is well-handled with git evidence.
• Tests are proportional and mirror the existing idiom.
• Only minor findings (issue-premise inaccuracy unflagged in Objective; one borderline-trivial test).

Grade: A
Verdict: GO

---

Generated by Claude Code Planner (strict review loop)

[mvillmow]

🔍 Plan Review

This is decisive. The installed mcp SDK's call_tool decorator (server.py:583-584) wraps the handler in try/except Exception as e: return self._make_error_result(str(e)). So a raised KeyError (or ValueError) is caught and converted into a structured isError result — it does NOT crash the stdio server process.

This means:

• The issue's stated failure mode is factually wrong: a bare KeyError does NOT crash the server; the SDK already catches it into a structured error.
• The plan's reinterpretation ("the real defect is the opaque message, not a crash") turns out to be correct and is now verified against the installed SDK — resolving the plan's own highest-risk assumption in the plan's favor.

The fix (descriptive ValueError) still has genuine value: _make_error_result(str(e)) surfaces the exception's str() to the client. A bare KeyError's str is "'team_id'" (opaque); the new MissingArgumentError's str is actionable. So the fix improves the client-facing error message — a real, if narrower, improvement than the issue claims.

Now let me evaluate the plan against the rubric.

Assessment:

Requirements alignment — The single acceptance criterion (missing team_id → descriptive ValueError not bare KeyError, client untouched) is named and addressed with a concrete step (Change 2). ✓ The plan correctly notes line drift (82 vs 81 — verified: actual is the line with team_id = str(arguments["team_id"])).

Plan completeness — Design rationale, exact code, two tests, implementation order, verification commands, lint. ✓

Concreteness — Names mcp_server.py:58-59, Dispatcher.call_tool, UnknownToolError, exact diff, test names. ✓ Excellent.

Risk surface — Correctly identifies the branch-scoping risk (file only on 173-auto-impl, not main) with git evidence, and Agent Guardrails (never target main). ✓

Verification plan — Three named pytest invocations + lint, mapped to criteria. ✓

Stage handoff — Implementer can act without re-deriving. ✓

Principles:

• P1/KISS, P2/YAGNI: Minimal — one exception class mirroring the existing one, one try/except, two tests. No scope creep. ✓
• P3/TDD: Two tests, one asserting behavior (descriptive raise + client untouched), one asserting the type contract. Proportional. ✓
• P4/DRY: Mirrors UnknownToolError idiom rather than inventing a new mechanism. ✓
• P7/POLA: Keeps the path symmetric with UnknownToolError (raise, don't return TextContent). Justified well. ✓

Minor findings:

1. The plan accepts the issue's "crashes the stdio server process" framing in its Objective without flagging it as unverified/incorrect — though it does hedge correctly later ("a bare KeyError is just as caught-and-structured by the SDK as a ValueError"). The plan reached the right design conclusion despite the issue's wrong premise. The risk it flagged ("if false, raising ValueError alone may not keep the server alive") is now resolved in its favor by SDK inspection — but the plan left it unverified. Minor: the implementer should verify the SDK behavior (which I've now done), but the plan's design is robust either way.
2. The two-test split is slightly redundant — test_call_tool_missing_team_id_raises_descriptive_error already implies a ValueError via MissingArgumentError; the separate issubclass test is borderline trivial. Not a real defect; acceptable belt-and-suspenders consistent with the file's existing style (test_module_has_no_write_method_references).

No critical or major findings. The plan is concrete, correctly scoped, guardrail-aware, and its design conclusion holds even though the issue's failure-mode description is technically inaccurate. The plan correctly chose the right fix regardless.

• The plan's design is correct and verified against the installed SDK (server.py:583): exceptions become structured isError results; the descriptive ValueError improves the client-facing message via _make_error_result(str(e)).
• Branch-scoping risk is well-handled with git evidence.
• Tests are proportional and mirror the existing idiom.
• Only minor findings (issue-premise inaccuracy unflagged in Objective; one borderline-trivial test).

Grade: A
Verdict: GO