feat(chat-ai): SSE streaming + IMcpToolProvider + Message.context

[rubenvdlinde]

Summary

Implements the OR-side of the ai-chat-companion 3-spec chain (hydra ADR-034).

• NEW OCA\OpenRegister\Mcp\IMcpToolProvider PHP interface — per-app MCP tool registration; tool ids namespaced as {appId}.{toolName}, enforced mechanically by McpToolsService.
• NEW built-in providers (lib/Mcp/BuiltIn/) for registers, schemas, objects — the existing static tools migrated onto the new contract.
• NEW POST /api/chat/stream SSE controller emitting the six-event envelope (token, tool_call, tool_result, heartbeat, final, error). v1 emits a single `final` after the synchronous LLM call; token-by-token streaming via LLPhant hooks is a follow-up.
• NEW GET /api/chat/health — lightweight public probe for the nextcloud-vue widget at mount.
• NEW migration adds Message.context JSON column (default {}).
• REFACTOR McpToolsService now enumerates IMcpToolProvider implementations in-process per turn (126 added / 493 removed LOC).
• FIX pre-existing OR bug: ResponseGenerationHandler unconditionally instantiated OpenAIChat($config) which type-errored when $config was OllamaConfig. Wrapped in if ($chatProvider !== 'ollama') — Ollama chat was completely broken before this.

Spec change

Delta MODIFIES the existing chat-ai capability. New requirements:

1. IMcpToolProvider PHP interface
2. McpToolsService provider-discovery refactor
3. SSE streaming endpoint POST /api/chat/stream
4. Health probe GET /api/chat/health
5. Message.context JSON column
6. MCP tool authorization flowthrough (ADR-005 Rule 3)

Browser-verified

Both endpoints proven live: /api/chat/health returns 200 + {status:\"ok\",capabilities:[\"chat\",\"stream\"]}. /api/chat/stream emits the heartbeat correctly, conversation gets created cleanly. End-to-end Ollama LLM round-trip works in principle but Ollama's /api/chat endpoint specifically hangs on this dev env (Ollama-side issue, not OR-side).

Dependencies

Depends on the hydra ai-chat-companion spec landing first (it defines the contracts this PR implements).

Test plan

• openspec validate ai-chat-companion-orchestrator --strict passes
• /api/chat/health returns 200 with the configured provider, 503 without
• /api/chat/stream opens SSE, emits heartbeat, creates a Conversation row
• Fireworks streaming spike (task 1, blocker for token-by-token streaming)
• composer check:strict clean (PHPCS / PHPMD / Psalm / PHPStan / PHPUnit)
• Unit tests for McpToolsService namespace enforcement
• Unit tests for ChatStreamController envelope shape

[github-actions]

Quality Report — ConductionNL/openregister @ f85808a

Check	PHP	Vue	Security	License	Tests
lint	✅
phpcs	❌
phpmd	✅
psalm	✅
phpstan	✅
phpmetrics	✅
eslint		❌
stylelint		✅
composer			✅	❌ 1/153 denied
npm			✅	✅ 598/598
PHPUnit					⏭️
Newman					⏭️
Playwright					⏭️

❌ Denied composer licenses

Package	Version	License
dompdf/dompdf	v3.1.5	LGPL-2.1

---

Quality workflow — 2026-05-11 11:20 UTC

Download the full PDF report from the workflow artifacts.

[WilcoLouwerse]

[BLOCKER] IDOR: resolveConversation loads conversation by UUID without owner check

When conversationUuid is non-empty, conversationMapper->findByUuid(uuid: $conversationUuid) is called without verifying that the returned conversation's userId matches the authenticated $userId. Any authenticated user can supply another user's conversation UUID and hijack that conversation's context, inject messages, or read its history via the SSE pipeline. Fix: after findByUuid(), assert $conversation->getUserId() === $userId and throw (or emit an SSE error) if they differ.

[WilcoLouwerse]

[BLOCKER] MCP tool provider performs no user-permission check

listObjects() calls $this->objectService->findAll(config: $config) and getObject() calls $this->objectService->find($id) without passing a user context or calling any ACL/permission service. The IMcpToolProvider docblock explicitly states: 'Implementations MUST check Nextcloud auth and per-object IDOR boundaries before returning data', yet none of the three built-in providers do this. An LLM invoked on behalf of user A can retrieve objects from registers user A has no read permission on. Same issue in RegistersToolProvider::listRegisters() and SchemasToolProvider::listSchemas().

[WilcoLouwerse]

[BLOCKER] Internal exception message leaked verbatim into SSE error event

$e->getMessage() is written directly into the message field of the error SSE event sent to the client. If an internal exception carries a DB error, stack trace fragment, API key, or LLM provider URL, it will be exposed to the browser. Replace with a generic user-facing message and log the raw exception; only expose a sanitised string in the SSE payload.

[WilcoLouwerse]

[BLOCKER] Outer catch only catches Exception, not \Throwable

catch (Exception $e) will not catch \Error, \TypeError, \ParseError, or other \Throwable subclasses. If json_decode returns an unexpected type and a subsequent PHP 8 typed property assignment throws a TypeError, the exception propagates past the catch, no error SSE event is emitted, the client connection drops silently, and Nextcloud's error handler may emit non-SSE output that corrupts the stream. Change to catch (\Throwable $e) to guarantee the error event is always sent before exit.

[WilcoLouwerse]

[BLOCKER] RegistersToolProvider list/create/update/delete exposed with no permission check

listRegisters(), createRegister(), updateRegister(), deleteRegister() call RegisterService methods without passing the current user or consulting ACL. A non-admin user prompted via the AI can delete or overwrite any register in the system. Inject IUserSession and validate against the register's ownership/ACL before mutating state.

[WilcoLouwerse]

[BLOCKER] Migration filename collision with PRs #1478, #1482, #1496

Four concurrent PRs each introduce a migration named Version1Date20260511100000.php. Nextcloud's migration runner identifies migrations by class name; merging more than one of these PRs will cause a fatal Class already declared error or silently skip the second migration depending on the autoloader. Rename this migration to an unused timestamp (e.g. Version1Date20260511110000) before merge and coordinate with the authors of the colliding PRs.

[WilcoLouwerse]

[BLOCKER] SPDX license header missing from new lib/ files (Gate 1)

All five new files under lib/ (IMcpToolProvider.php, ObjectsToolProvider.php, RegistersToolProvider.php, SchemasToolProvider.php, ChatStreamController.php, ChatHealthController.php) use a @license EUPL-1.2 docblock tag but omit the machine-readable SPDX-License-Identifier: EUPL-1.2 and SPDX-FileCopyrightText comment lines required by ADR-014 and the SPDX gate. CI will reject these files.

[WilcoLouwerse]

[CONCERN] agentMapper->findAll(limit:1) auto-selects first agent regardless of user permissions

When neither conversationUuid nor agentUuid is provided, the controller falls back to $this->agentMapper->findAll(limit: 1) and picks the first agent in the database. This exposes agents that may be private, admin-only, or tenant-scoped to any authenticated user. The fallback should only return agents visible to $userId.

[WilcoLouwerse]

[CONCERN] Client-supplied context echoed verbatim in the final SSE event

$context is read from the request body ($body['context'] ?? null) and then emitted unchanged in the final event payload. If a malicious or buggy client injects a context object containing credentials, session tokens, or PII, those values are reflected back in the SSE stream. Either drop context from the final payload entirely or strip to the known-safe fields (appId, pageKind, objectUuid, registerSlug, schemaSlug, capturedAt).

[WilcoLouwerse]

[CONCERN] exit; bypasses Nextcloud shutdown handlers before DB cleanup

The PR's own design.md (Open Question 2) acknowledges that exit; may suppress Nextcloud's register_shutdown_function handlers responsible for DB connection cleanup. The implementation does not address this: no explicit getDatabaseConnection()->close() is called before exit. Under high concurrency this can exhaust the DB connection pool. Add a test and a comment confirming the risk was validated.

[WilcoLouwerse]

[CONCERN] getTools() called on every listTools() invocation — no caching

listTools() iterates $this->providers and calls getTools() on each provider on every request turn. If an external IMcpToolProvider performs I/O inside getTools(), this multiplies cost per turn. Tool descriptors are static metadata and should be cached at registration time.

[WilcoLouwerse]

[CONCERN] No rate limiting on POST /api/chat/stream

The SSE streaming endpoint performs an LLM call (external cost) per request with no throttle. An authenticated user can open unbounded parallel SSE connections to drive up LLM costs or cause resource exhaustion. Add UserRateThrottle attribute or document operator-level mitigation (nginx limit_req).

[WilcoLouwerse]

[CONCERN] No heartbeat during synchronous LLM call — 15s heartbeat design not implemented

The design (D3) calls for a heartbeat event every 15 seconds during blocking operations. The v1 implementation emits one heartbeat before the LLM call then blocks synchronously. Reverse proxies with idle-timeout < LLM latency will silently drop the connection. Document this prominently in the code as a known proxy-timeout risk, not just in design.md.

[WilcoLouwerse]

[CONCERN] Message.context not validated before persistence — arbitrary JSON stored

$context = $body['context'] ?? null is passed as-is into $ragSettings['__cn_ai_context__']. tasks.md task 8.2 requires validation that context is a JSON object, returning HTTP 400 if not. No validation exists. A malicious client can store arbitrarily large or structurally unexpected JSON in oc_openregister_messages.context.

[WilcoLouwerse]

Review

🔴 Blockers (7)

• IDOR: resolveConversation loads conversation by UUID without owner check — lib/Controller/ChatStreamController.php:511
• MCP tool provider performs no user-permission check — lib/Mcp/BuiltIn/ObjectsToolProvider.php:706
• Internal exception message leaked verbatim into SSE error event — lib/Controller/ChatStreamController.php:469
• Outer catch only catches Exception, not \Throwable — lib/Controller/ChatStreamController.php:457
• RegistersToolProvider list/create/update/delete exposed with no permission check — lib/Mcp/BuiltIn/RegistersToolProvider.php:940
• Migration filename collision with PRs #1478, #1482, #1496 — lib/Migration/Version1Date20260511100000.php:1417
• SPDX license header missing from new lib/ files (Gate 1) — lib/Mcp/IMcpToolProvider.php:1283

🟡 Concerns (8)

• agentMapper->findAll(limit:1) auto-selects first agent regardless of user permissions — lib/Controller/ChatStreamController.php:395
• Client-supplied context echoed verbatim in the final SSE event — lib/Controller/ChatStreamController.php:450
• exit; bypasses Nextcloud shutdown handlers before DB cleanup — lib/Controller/ChatStreamController.php:456
• getTools() called on every listTools() invocation — no caching — lib/Service/Mcp/McpToolsService.php:1700
• No rate limiting on POST /api/chat/stream — lib/Controller/ChatStreamController.php:371
• No heartbeat during synchronous LLM call — 15s heartbeat design not implemented — lib/Controller/ChatStreamController.php:435
• No unit test for ChatStreamController (tests/Unit/Controller:1)
  tasks.md task 9.2 requires ChatStreamControllerTest.php covering: (a) 6-event envelope, (b) non-streaming degradation, (c) heartbeat, (d) unauthenticated rejection. None of these tests appear in the diff. The PHPUnit CI failure may partly be caused by untested controller paths. Prerequisite for merge given the IDOR and exception-leak issues.
• Message.context not validated before persistence — arbitrary JSON stored — lib/Controller/ChatStreamController.php:432

🟢 Minor (3)

• Comment says authenticated but auth is enforced only by framework default (appinfo/routes.php:14)
  // Chat - SSE streaming endpoint (authenticated). — accurate at runtime, but the absence of an explicit #[NoAdminRequired] attribute reference in the comment means a future refactor adding #[PublicPage] would silently remove the auth requirement without the comment becoming obviously wrong.
• catch (\Exception $e) in invokeTool() — does not catch \Throwable (lib/Service/Mcp/McpToolsService.php:2129)
  invokeTool() catches only \Exception. PHP \Error subclasses (e.g. \TypeError) will propagate uncaught. Change to catch (\Throwable $e) for consistency with the spec requirement that invokeTool always returns a result envelope.
• Raw php://input read without size limit — potential DoS via large bodies (lib/Controller/ChatStreamController.php:379)
  file_get_contents('php://input') has no size cap. A client can submit a multi-megabyte body (e.g. a large context blob) loaded entirely into memory before validation. Add an explicit size check via $_SERVER['CONTENT_LENGTH'].

Reviewed by WilcoLouwerse via automated batch review.