feat(mcp): expose Extra as an MCP server over stdio

[Karn2898]

Adds agentctl mcp serve --config ./agents.yml, which starts an MCP server (stdio transport) that exposes the full Extra agent graph as a single extra_chat tool.

What changed

• src/agentctl/mcp_serve.py — new ExtraMCPServer class that:

  • builds the LangGraphEngine once at startup and reuses it for all requests,
  • registers a single extra_chat FastMCP tool that captures self directly (no module-level mutable global),
  • assigns the engine to the cleanup slot before build() so a build failure still triggers close(),
  • on each call: resolves a Principal for the caller, creates a session when session_id is empty, and delegates to the existing ConversationService.send,
  • returns the RunResult (answer, visited, used_tools, session_id) as an MCP tool response.

• src/agentctl/main.py — adds the mcp Click group and mcp serve subcommand (wired before if __name__ == "__main__").

• tests/cli/test_mcp_serve_command.py — 9 tests covering:

  • CLI validation rejects invalid specs before starting stdio,
  • CLI starts stdio mode with a valid spec,
  • extra_chat creates a new session when session_id is empty,
  • extra_chat reuses the supplied session_id,
  • default user_id falls back to the stable ext:<digest> principal,
  • used_tools are propagated from the RunResult,
  • the response dict is JSON-serialisable,
  • the engine and DB are closed even if build() fails,
  • a real subprocess + stdio + MCP client round-trip proves end-to-end wiring and session isolation.

• examples/mcp-server/ — minimal single-agent spec with a tiny echo tool and a client.py that drives the server (also serves as living documentation of the wire format).

Done when (all checked)

• agentctl mcp serve --config ./agents.yml starts an MCP server
• an MCP client can discover extra_chat
• the client can send a message and receive an answer
• a new session is created when no session ID is provided
• the same session can continue a previous conversation
• different sessions do not share history
• the engine is built once and reused
• the existing Extra behavior is not duplicated or changed

[Asaf-prog]

Thanks — this is a significant improvement over #126. The Principal API is now used correctly, the example is valid, cleanup is better, the stdio E2E coverage is useful, and CI is green.

I still see three blockers before approving.

1. The MCP runtime composition still diverges from agentctl run

mcp serve currently builds the engine with only:

session_approval_repository=self._repositories.session_approvals

But the normal CLI runtime also injects:

tool_usage_repository=repositories.tool_usage
run_repository=repositories.runs

and passes:

run_repository=repositories.runs

into ConversationService.

So MCP still has a separate composition path where part of the runtime state may silently fall back to process-local implementations.

I would strongly prefer reusing the same application/runtime composition used by agentctl run rather than maintaining two slightly different wiring paths.

2. Database migrations are still missing from mcp serve

agentctl run calls:

upgrade_database()

before starting.

mcp serve currently does not.

The integration test actually performs upgrade_database() manually in the bootstrap script before launching the MCP CLI, which means the test hides this problem instead of proving that agentctl mcp serve works correctly against a fresh persistent database.

Please make the command responsible for the same database initialization contract as the existing runtime and remove the test-side workaround.

3. HITL is still not represented over the MCP boundary

ConversationService.send() may return a RunResult with:

status = PENDING_APPROVAL
pending_approval != None
answer = ""

But extra_chat currently returns only:

session_id
answer
visited
used_tools

So a suspended run can look like an empty successful response to the MCP caller, and there is no way to understand or resume that pending approval.

For the initial version, I think we need an explicit contract here.

Either:

• expose enough approval state and provide a supported resume/decision flow over MCP,

or:

• explicitly restrict MCP mode to auto-execution and reject/configure out approval-requiring flows.

What I don't want is a pending approval being silently represented as an empty answer.

One additional design/security concern:

user_id is currently accepted as an extra_chat tool argument and then converted directly into:

Principal.external(user_id)

But Principal.external() represents an identity that has already been verified by the host boundary.

Here the value is caller-controlled MCP input, so we should not describe or treat it as host-verified unless stdio mode explicitly defines the whole caller process as trusted.

Please make that trust model explicit and avoid crossing the Principal boundary with an arbitrary unverified identifier.

Once the runtime composition, migrations, HITL contract, and caller identity boundary are aligned with the rest of Extra, I think this will be much closer to approval.

[Karn2898]

Thanks — this is a significant improvement over #126. The Principal API is now used correctly, the example is valid, cleanup is better, the stdio E2E coverage is useful, and CI is green.

I still see three blockers before approving.

1. The MCP runtime composition still diverges from agentctl run

mcp serve currently builds the engine with only:

session_approval_repository=self._repositories.session_approvals

But the normal CLI runtime also injects:

tool_usage_repository=repositories.tool_usage
run_repository=repositories.runs

and passes:

run_repository=repositories.runs

into ConversationService.

So MCP still has a separate composition path where part of the runtime state may silently fall back to process-local implementations.

I would strongly prefer reusing the same application/runtime composition used by agentctl run rather than maintaining two slightly different wiring paths.

2. Database migrations are still missing from mcp serve

agentctl run calls:

upgrade_database()

before starting.

mcp serve currently does not.

The integration test actually performs upgrade_database() manually in the bootstrap script before launching the MCP CLI, which means the test hides this problem instead of proving that agentctl mcp serve works correctly against a fresh persistent database.

Please make the command responsible for the same database initialization contract as the existing runtime and remove the test-side workaround.

3. HITL is still not represented over the MCP boundary

ConversationService.send() may return a RunResult with:

status = PENDING_APPROVAL
pending_approval != None
answer = ""

But extra_chat currently returns only:

session_id
answer
visited
used_tools

So a suspended run can look like an empty successful response to the MCP caller, and there is no way to understand or resume that pending approval.

For the initial version, I think we need an explicit contract here.

Either:

• expose enough approval state and provide a supported resume/decision flow over MCP,

or:

• explicitly restrict MCP mode to auto-execution and reject/configure out approval-requiring flows.

What I don't want is a pending approval being silently represented as an empty answer.

One additional design/security concern:

user_id is currently accepted as an extra_chat tool argument and then converted directly into:

Principal.external(user_id)

But Principal.external() represents an identity that has already been verified by the host boundary.

Here the value is caller-controlled MCP input, so we should not describe or treat it as host-verified unless stdio mode explicitly defines the whole caller process as trusted.

Please make that trust model explicit and avoid crossing the Principal boundary with an arbitrary unverified identifier.

Once the runtime composition, migrations, HITL contract, and caller identity boundary are aligned with the rest of Extra, I think this will be much closer to approval.

Thanks for the detailed review — I really appreciate the thorough feedback, especially the callouts about the trust boundary for caller identity and the silent fallback on the HITL path. Those were both gaps that would have bitten us in production, and I'm glad they surfaced now rather than later.

I've pushed a follow-up commit that closes those gaps: the MCP path now wires up the same runtime pieces as the normal CLI flow, runs database migrations automatically before the server starts, and exposes the approval state explicitly over the MCP boundary with a supported resume flow. I also made sure the caller identity is treated as an anonymous local label rather than something host-verified, since stdio callers are external processes. The test suite is green and the widget TypeScript issue is fixed too — ready for another look when you have time.

[Asaf-prog]

Thanks for the follow-up — the previous runtime composition, migration, identity-boundary, and initial HITL gaps are much better now.

I still see a few blockers before approving.

1. The approval identity is inconsistent when user_id is used

extra_chat builds the principal from the caller-supplied user_id:

effective_user_id = user_id or DEFAULT_USER_ID
principal = _principal_for(effective_user_id)

but decide_approval always uses:

effective_user_id = DEFAULT_USER_ID
principal = _principal_for(effective_user_id)

So this flow will fail:

extra_chat(user_id="alice")
→ session owned by anon:alice
→ pending approval

decide_approval(...)
→ principal becomes anon:local-user
→ authorization fails

Please keep the caller identity consistent across the whole conversation / approval lifecycle.

One option is to include user_id in decide_approval as well.

Another, and probably cleaner for stdio v1, would be to define one stable identity for the MCP server connection/session instead of allowing identity to vary independently on each tool call.

2. A second approval after resume is still not represented

extra_chat correctly includes:

pending_approval

when the result is suspended.

But _handle_decide_approval() returns only:

session_id
status
answer
visited
used_tools

So this case is still lossy:

approval A
→ approve
→ run resumes
→ another tool requires approval B
→ status = pending_approval
→ approval B details are lost

The result serialization should be the same regardless of whether the RunResult came from send() or decide_approval().

I would strongly suggest extracting one shared RunResult -> MCP payload mapper and using it in both paths. That avoids the two response contracts drifting apart.

3. Invalid approval input silently becomes DENY

The current code does:

parse_decision(
    decision,
    default=ApprovalDecision.DENY,
)

That means a typo such as:

"aproove"

is silently converted into a denial.

At an external MCP boundary I would prefer invalid input to fail explicitly rather than turning malformed input into a real user decision.

Please use the strict parsing behavior here and return a proper MCP/tool input error for unsupported decision values.

4. The PR currently contains unrelated feedback/widget work from feat(feedback): add thumbs up/down buttons and persistence #138

This is now a scope blocker.

#137 currently includes commits and files related to the thumbs up/down feedback feature, including feedback persistence, widget changes, streamed assistant message IDs, and the widget bundle.

Those changes belong to #138 and should not be part of the MCP server PR.

Please clean the branch so #137 contains only the MCP server work and its directly required changes.

The branch is also behind current main, so after removing the unrelated commits, please update it against the latest main and rerun CI.

The MCP implementation itself is much closer now. Once the identity lifecycle is consistent, chained approvals preserve the full pending state, invalid decisions are rejected explicitly, and the PR is cleaned back to its intended scope, I think this will be ready for a final review.

[Karn2898]

Thanks for the follow-up — the previous runtime composition, migration, identity-boundary, and initial HITL gaps are much better now.

I still see a few blockers before approving.

1. The approval identity is inconsistent when user_id is used

extra_chat builds the principal from the caller-supplied user_id:

effective_user_id = user_id or DEFAULT_USER_ID
principal = _principal_for(effective_user_id)

but decide_approval always uses:

effective_user_id = DEFAULT_USER_ID
principal = _principal_for(effective_user_id)

So this flow will fail:

extra_chat(user_id="alice")
→ session owned by anon:alice
→ pending approval

decide_approval(...)
→ principal becomes anon:local-user
→ authorization fails

Please keep the caller identity consistent across the whole conversation / approval lifecycle.

One option is to include user_id in decide_approval as well.

Another, and probably cleaner for stdio v1, would be to define one stable identity for the MCP server connection/session instead of allowing identity to vary independently on each tool call.

2. A second approval after resume is still not represented

extra_chat correctly includes:

pending_approval

when the result is suspended.

But _handle_decide_approval() returns only:

session_id
status
answer
visited
used_tools

So this case is still lossy:

approval A
→ approve
→ run resumes
→ another tool requires approval B
→ status = pending_approval
→ approval B details are lost

The result serialization should be the same regardless of whether the RunResult came from send() or decide_approval().

I would strongly suggest extracting one shared RunResult -> MCP payload mapper and using it in both paths. That avoids the two response contracts drifting apart.

3. Invalid approval input silently becomes DENY

The current code does:

parse_decision(
    decision,
    default=ApprovalDecision.DENY,
)

That means a typo such as:

"aproove"

is silently converted into a denial.

At an external MCP boundary I would prefer invalid input to fail explicitly rather than turning malformed input into a real user decision.

Please use the strict parsing behavior here and return a proper MCP/tool input error for unsupported decision values.

4. The PR currently contains unrelated feedback/widget work from feat(feedback): add thumbs up/down buttons and persistence #138

This is now a scope blocker.

#137 currently includes commits and files related to the thumbs up/down feedback feature, including feedback persistence, widget changes, streamed assistant message IDs, and the widget bundle.

Those changes belong to #138 and should not be part of the MCP server PR.

Please clean the branch so #137 contains only the MCP server work and its directly required changes.

The branch is also behind current main, so after removing the unrelated commits, please update it against the latest main and rerun CI.

The MCP implementation itself is much closer now. Once the identity lifecycle is consistent, chained approvals preserve the full pending state, invalid decisions are rejected explicitly, and the PR is cleaned back to its intended scope, I think this will be ready for a final review.

Pushed fixes for all four blockers from the review:

Identity consistency — decide_approval now takes user_id and builds the same Principal as extra_chat, so identity stays consistent across the approval lifecycle within a conversation.
Chained approvals — pulled the shared response-mapping logic into _serialize_run_result(), used by both _handle_chat and _handle_decide_approval. So if a resumed run hits another approval gate, the full pending_approval block comes through instead of getting dropped.
Strict decision parsing — an invalid decision value now raises a ValueError instead of silently defaulting to DENY.
Branch scope — trimmed down to just the three MCP commits, feedback/widget stuff pulled out. Rebased on latest main, CI's green.

Ready for another look whenever you get a chance.

[Asaf-prog]

Thanks — this is much closer now. The previous blockers around identity consistency, chained approvals, branch scope, and shared result serialization are addressed.

I still see two things to fix before I can approve.

1. Please reuse the existing approval parsing boundary

The MCP path now does its own mapping:

mapping = {
    "approve": ApprovalDecision.ALLOW_ONCE,
    "reject": ApprovalDecision.DENY,
    "allow_for_session": ApprovalDecision.ALLOW_FOR_SESSION,
}

But agent_engine.approvals.decision.parse_decision() already exists specifically as the single external parsing boundary for approval decisions.

That module explicitly documents that free-text approval values from UI/API/CLI should be normalized in one place.

For MCP, the strict behavior we want is already available by calling:

parse_decision(decision)

without a default.

Invalid input will then raise InvalidDecision instead of silently becoming DENY.

Please reuse that existing contract rather than introducing a second parser/mapping in mcp_serve.py.

2. Please add regression tests for the fixes from the previous review

The implementation now addresses the bugs, but I don't see dedicated tests proving those regressions stay fixed.

Please add coverage for at least:

• identity consistency across approval:

  extra_chat(user_id="alice")
  → pending approval
  → decide_approval(user_id="alice")
  → succeeds with the same principal
  

• chained approvals:

  approval A
  → resume
  → approval B
  → response still includes the new pending_approval block
  

• invalid approval input:

  decision="aproove"
  → explicit input error
  → no approval decision is submitted
  

These were real correctness issues, so I would like them captured directly in the test suite rather than relying only on the happy path.

One final repo-state check: GitHub still shows this branch behind current main, so please update it against the latest main and rerun CI before the final review.

After that, I think this should be ready for approval.

[Karn2898]

Thanks for the follow-up review — both points are addressed:

Reuse the existing approval parsing boundary — replaced the ad-hoc mapping in mcp_serve.py with parse_decision(decision) (no default), so invalid input raises InvalidDecision and is surfaced as an explicit tool error. The MCP path now uses the single parsing contract from agent_engine.approvals.decision.
Regression tests added — added three focused tests in tests/cli/test_mcp_serve_command.py:
test_decide_approval_preserves_user_id_identity — proves user_id="alice" flows consistently from extra_chat through decide_approval
test_decide_approval_preserves_chained_pending_approval — proves a resumed run that hits a second approval still returns the new pending_approval block
test_decide_approval_rejects_invalid_decision — proves decision="aproove" raises an explicit ValueError
I still need to rebase onto the latest main (the branch is currently behind by a few commits) and rerun CI — I'll push that update shortly.