Consolidation: AI chat companion + 7 scholiq-deps features + 8 more clean/approved branches (PHPUnit green)

[rubenvdlinde]

What this is

A single consolidation branch off development (integration/scholiq-deps-and-companion, 58 commits ahead) folding together the ready / approved in-flight OpenRegister work — plus the test-suite cleanup that makes the whole thing green (PHPUnit was red on development itself with ~49 errors + 1 failure; fixed here).

Folded in

Stream	PR(s)	Status on its own branch
AI chat companion orchestrator (IMcpToolProvider, McpToolsService provider-aggregator refactor, BuiltIn/{Registers,Schemas,Objects}ToolProvider, POST /api/chat/stream SSE + /api/chat/health, Message.context migration + tests)	#1466	green
feat/scholiq-deps/* — appendOnly flag, per-tenant HMAC audit key API, calculatedChange notification trigger, idempotencyKey notification dedup, dateDiff calculation primitive, cross-schema aggregation joins, runtime.user.* manifest context	#1476–#1482	(already bundled in integration/scholiq-deps-all, the base of this branch)
Features & Roadmap menu — xwiki integration plan-to-issues	#1465	green
Collaborative-editing patterns doc	#1458	green
Platform capabilities catalog + declarative-annotation deltas	#1353	approved
mail-sidebar: body-mount + attachment drag-drop + @author email typo fix	#1285	approved
Specs: entity-relation-grondslagen	#1494	green
Specs: text-extraction-eml	#1495	green
i18n: wrap bare strings + Dutch→English keys (with an eslint --fix pass for the indentation it touched)	#1273	green

Conflict resolutions

• lib/Migration/Version1Date20260511100000.php (add/add) — kept the scholiq-deps openregister_tenant_keys migration at that name; renamed the AI-companion Message.context migration to Version1Date20260511130000.
• lib/Db/Schema.php — kept both the new ANNOTATION_VOCABULARY constant and the VALID_LINKED_TYPES @deprecated notice.
• src/modals/webhook/EditWebhook.vue — kept the headersPlaceholder computed (the inline t('…\n…') would reintroduce the documented \n-in-template SyntaxError).
• CHANGELOG.md — merged the two ### Breaking Changes lists.
• lib/Service/Object/SaveObject.php — added the @param docblock entry for the new $folderManagementHandler constructor parameter introduced upstream.

Test-suite cleanup (the green-making part)

development itself was red on PHPUnit (~49 errors + 1 failure). All fixed here:

Fix	Clears
ReferentialIntegrityService + RegisterService: get_class($db->getDatabasePlatform()) → get_debug_type(…) — null-safe (a mocked IDBConnection returns null), degrades to MySQL syntax instead of fatal-erroring	~42 errors (ReferentialIntegrityServiceTest ×3 dirs + RegisterServiceTest)
TransitionEngine: removed final so TransitionControllerTest can double it (ClassIsFinalException); note added re: extracting an interface if sealing is reintroduced	6 errors (TransitionControllerTest)
ManifestServiceTest: ObjectEntity::getUuid/getRegister/getSchema/getOwner/getCreated/getUpdated are @method magic via Entity::__call, so createMock() can't configure them — build the profile mock with getMockBuilder()->onlyMethods(['getObject'])->addMethods([the magic getters])	2 errors
AppendOnlyTest: dropped ->willReturn(null) on the mocked void SaveObject::clearAllCaches() (PHPUnit 10 IncompatibleReturnValueException); setHardValidation(false) in the schema helper so the mocked ValidateObject doesn't short-circuit saveObject() with an empty ValidationException; stubbed the RenderObject mock's renderEntity() since saveObject() returns renderEntity($savedObject, …) not the handler's return directly	4 errors + 2 failures
AnnotationNotificationDispatcherTest: the dispatcher resolves the local base URL (emitTalk/emitWebhook) and expression recipient resolvers (resolveExpressionRecipients) via IServerContainer::get(); the test mocked the container but never stubbed get(), so emitTalk fataled resolving IConfig and resolvers registered via \OC::$server->registerService weren't visible. Added a $serverServices map seeded with an IConfig mock; stubbed serverContainer->get() to read it; resolver tests register under their DI tag	1 error + 1 failure
McpToolsServiceTest rewritten as the provider-aggregator test + new tests/Unit/Mcp/BuiltIn/{Registers,Schemas,Objects}ToolProviderTest (the registers/schemas/objects CRUD logic moved into the BuiltIn providers)	63 errors
phpcs cleanups surfaced by the merges (migration block-comments, SaveObject docblock) + an eslint --fix pass for i18n's indentation	(phpcs/eslint)

CI

workflow_dispatch run on this branch (commit b29b6abe): all green — phpcs, eslint, psalm, phpstan, phpmd, phpmetrics, stylelint, license ×2, security ×2, Newman, PHPUnit on both PHP 8.3 and 8.4. (Push doesn't auto-trigger CI for integration/** branches — this PR's pull_request event does.)

Deliberately NOT folded in (need work before they can join)

• feature/1307/* integration-registry stack (feat(integration-registry): contract + registry + external router (umbrella PR 1/N) #1467→docs(integration-registry): developer guide + leaf scaffold + tracking sync (umbrella PR 10b/N) #1490) — merging onto current development surfaces 134 phpcs errors (the stack's positional-internal-call style predates a stricter "named parameters" sniff now active on development). Needs a phpcs-cleanup rebase first.
• feature/add-live-updates (feat: live updates via notify_push (REST transport for realtime-updates) #1453) — CONFLICTING with development; conflicts overlap the PermissionHandler/test fixes here. Needs its own rebase.
• fix/port-get-files-to-mapping-runtime (feat: getRegisters with expanded schemas, getFiles Twig function, and mass delete fixes #1445), feat/1428/extend-schemas-register-service (Implement extend-schemas-in-register-service: honour _extend on RegisterService end-to-end (#1428) #1430), feat/1342/validate-self-folder-access (Implement validate-self-folder-access: HTTP 403 + audit on cross-tenant @self.folder writes (#1342) #1431) — all CHANGES_REQUESTED; folding them in produced ~220 PHPUnit errors (their constructor/API changes break their own test mocks — that's the review feedback). They need their tests fixed first.
• Drafts feat(github-issues): backend skeleton for Features & Roadmap menu (WIP, 10/22 tasks) #1463 (roadmap-menu backend, 10/22 tasks) and feat(features-roadmap): OpenRegister pilot integration — route + nav link (section 5) #1491 (roadmap pilot frontend).

Note on risky tests

RelationHandlerTest, Tmlo*, ObjectReferenceProviderTest, CrossSchemaAggregationRunnerTest report PHPUnit risky ("executed code not listed as covered") — phpunit.xml has failOnRisky="false", so they don't gate. Tightening their @uses annotations is a separate low-priority cleanup.

[WilcoLouwerse]

Review

🔴 Blockers (7)

• resolveConversation loads conversation by UUID without ownership check (IDOR) (lib/Controller/ChatStreamController.php:549)
  resolveConversation() calls $this->conversationMapper->findByUuid(uuid: $conversationUuid) without verifying that the resolved conversation belongs to $userId. Any authenticated user can supply any conversationUuid and read or append to another user's conversation thread. After findByUuid, assert $conversation->getUserId() === $userId and throw/emit an error SSE event with code: forbidden if they differ.
• Exception message leaked verbatim to client in SSE error event (lib/Controller/ChatStreamController.php:507)
  $e->getMessage() is emitted directly in the SSE error event. For an LLM/DB exception this may contain connection strings, internal paths, or API key fragments. The logger already records the full error; the client only needs a generic message. Replace the SSE payload with a static 'An internal error occurred.' and keep $e->getMessage() only in the logger call.
• resolveUserProfile calls findAll with _rbac:false, _multitenancy:false — bypasses ALL access controls (lib/Service/ManifestService.php:3722)
  ManifestService::resolveUserProfile() calls $this->objectService->findAll() with _rbac: false and _multitenancy: false. The profile lookup ignores role-based access control and multi-tenancy boundaries. If the same Nextcloud UID exists in multiple tenants, this can return a profile belonging to a different tenant. Use the default RBAC and multitenancy flags, or at minimum add an explicit organisation filter when multitenancy is disabled.
• buildUserContext seeds runtime.user with raw profile->getObject() data with no allowlist (lib/Service/ManifestService.php:3755)
  buildUserContext() merges raw $profile->getObject() ?? [] into $context unconditionally. Every stored field from the profile — including any fields marked internal by the schema — is returned verbatim. There is no allowlist or field-level filtering. This leaks PII or sensitive schema fields to the caller's browser/apps consuming the manifest. Add an explicit field allowlist driven by a schema annotation (e.g. x-openregister-manifest-user-fields).
• RegistersToolProvider exposes all-tenant register list without user/org filtering (lib/Mcp/BuiltIn/RegistersToolProvider.php:1712)
  invokeTool() dispatches to listRegisters() which calls $this->registerService->findAll(limit, offset) with no user or organisation context. In a multi-tenant deployment this may return registers belonging to all organisations. The IMcpToolProvider interface docblock states implementations MUST check IDOR boundaries. Pass the current user context into the service calls or confirm the service already applies per-org scoping and document it here. Same issue likely in SchemasToolProvider and ObjectsToolProvider.
• Agent picker falls back to first agent in DB without filtering by user — cross-user data exposure (lib/Controller/ChatStreamController.php:441)
  When neither agentUuid nor conversationUuid is supplied, the controller calls $this->agentMapper->findAll(limit: 1) and uses the first returned agent. This ignores which user is making the request. In a multi-user deployment the first agent could belong to a different user. Either require a caller-supplied agentUuid, or filter the findAll() by userId: $userId.
• Unique index TOCTOU window allows duplicate dispatch under concurrency (lib/Migration/Version1Date20260511120000.php:2513)
  The migration creates a unique index on (notification_slug, idempotency_key), but NotificationDispatchLogMapper::record() uses INSERT without ON DUPLICATE KEY handling. isDuplicate() is called before record(), so two concurrent dispatches can both pass isDuplicate() and both emit notifications before either records the log row. Use INSERT IGNORE (MySQL) / INSERT ... ON CONFLICT DO NOTHING (Postgres), or insert-first then catch unique-constraint violation to abort the send.

🟡 Concerns (10)

• stream() uses exit; bypassing NC shutdown handlers — DB connection leak under PHP-FPM (lib/Controller/ChatStreamController.php:395)
  The stream() method calls exit; at multiple points to bypass NC's response handler. The design document acknowledges this but the DB connection leak risk under PHP-FPM is an open question that has not been resolved before merge. Before every exit;, explicitly close or commit transactions and release connections, or add a register_shutdown_function for cleanup.
• invokeTool() catches \Exception, not \Throwable — TypeError propagates uncaught (lib/Service/Mcp/McpToolsService.php:4382)
  McpToolsService::invokeTool() catches only \Exception. A \TypeError is an \Error, not an \Exception, so it would escape both this handler and the controller's catch (Exception $e), causing a fatal 500 without an SSE error event. Change to catch (\Throwable $e) in both McpToolsService::invokeTool() and ChatStreamController::stream().
• findRegisterForSchema uses loose in_array(...) — type coercion bugs possible — lib/Service/Aggregation/AggregationRunner.php:3203
• Message.context never persisted — spec requirement not met — lib/Controller/ChatStreamController.php:469
• schemaSlug not validated — uncontrolled schema resolution — lib/Service/ManifestService.php:3662
• generateKey() uses random_bytes directly; injected ISecureRandom is unused dead code — lib/Service/TenantKeyService.php:5268
• Health probe catches all \Throwable and returns 503 — config errors silently masked as 'no_provider' — lib/Controller/ChatHealthController.php:237
• isDuplicate cutoff formatted in local time — timezone-sensitive — lib/Db/NotificationDispatchLogMapper.php:1003
• Cross-schema RBAC gate uses objectOwner: null — lib/Service/Aggregation/AggregationRunner.php:2987
• Ollama guard leaves $response uninitialized — lib/Service/Chat/ResponseGenerationHandler.php:3484

🟢 Minor (1)

• stream() declares return type Response but always exits (lib/Controller/ChatStreamController.php:396)
  The method signature declares : Response but always calls exit; — never returns a value. Static analysers may warn. Either return new Response() on the unreachable last line, or use void if the framework allows it.

Reviewed by WilcoLouwerse via automated batch review.

[WilcoLouwerse]

Addressed every note from the security review in 864dd83. One commit, 15 files touched, +584 / −121.

🔴 Blockers (7)

#	Issue	Fix
1	resolveConversation IDOR	ChatStreamController::resolveConversation() now asserts $conversation->getUserId() === $userId and emits a forbidden SSE frame on mismatch.
2	Exception message leaked over SSE	stream()'s top-level catch logs the full exception but emits a static "An internal error occurred." over the wire.
3	resolveUserProfile bypasses RBAC / multitenancy	Dropped _rbac: false and _multitenancy: false; the (schema, ncUserId) filter is now narrowing, not a tenant-scope substitute.
4	buildUserContext leaked raw getObject()	Replaced the blanket merge with an allowlist (x-openregister-manifest-user-fields on the schema config + materialised calc names + {id, roles}).
5	MCP tool providers all-tenant exposure	RegistersToolProvider / SchemasToolProvider / ObjectsToolProvider now pass _rbac: true / _multitenancy: true explicitly and document the IMcpToolProvider IDOR boundary.
6	Fallback agent picker	pickFallbackAgentForUser() now iterates the first 20 agents and returns the first one canUserAccessAgent() clears — no more "first row of openregister_agents regardless of owner".
7	TOCTOU on (notification_slug, idempotency_key)	NotificationDispatchLogMapper::record() catches the unique-index violation and raises DuplicateDispatchException; dispatcher's claimIdempotencyKey() now claims first, sends after — the unique index is the authoritative serialisation point.

🟡 Concerns (10)

#	Issue	Fix
1	exit; leaks DB connection under PHP-FPM	New emitAndExit() helper commits any open transaction via IDBConnection::commit() before exit;. All termination paths funnel through it.
2	McpToolsService catches \Exception not \Throwable	Switched to \Throwable in both callTool() and invokeTool() so \TypeError/\Error subclasses don't escape the MCP envelope.
3	Loose in_array in findRegisterForSchema	Both sides cast to (int), strict mode flag set to true.
4	Message.context never persisted	Added an explicit TODO referencing the spec section — the storeMessage path needs a follow-up to thread context through.
5	schemaSlug not validated	New MAX_SLUG_LENGTH + regex (/^[A-Za-z0-9_\-]{1,128}$/) gate at the top of getEnrichedManifest(); fails closed to runtime.user = null.
6	ISecureRandom is dead code	Removed the parameter from TenantKeyService (constructor, DI registration, and test). Docblock now explains why random_bytes() is the deliberate choice.
7	Health probe masks config errors	ChatHealthController now logs the exception at warning level and returns {status: 'config_error'} (still HTTP 503) so operators can distinguish from a fresh no_provider instance.
8	Dedup cutoff in local time	All three of isDuplicate(), record(), pruneExpired() now anchor to new DateTimeZone('UTC').
9	Cross-schema RBAC objectOwner: null	Documented as the deliberate list-level form — added a comment explaining the PermissionHandler null-pair semantics.
10	Ollama branch leaves $response uninitialised	Clarified — $response = '' is now flagged in a comment as the deliberate seed before the provider branches, so future provider additions can't regress to undefined-variable.

🟢 Minor (1)

Issue	Fix
stream() declares Response return type but always exits	Added an unreachable return new Response(); after the top-level catch for static-analyser comfort.

Notes

• lib/Db/DuplicateDispatchException.php is a new typed exception class so callers can react specifically to the unique-index race.
• php -l clean and phpcs reports zero errors on touched lib files (the remaining warnings are line-length and pre-existing).
• Local PHPUnit can't run without NC bootstrap; CI on this branch was green prior to the change and the edits parallel existing patterns.

🤖 Generated with Claude Code

[WilcoLouwerse]

Re-review ✅

All 7 🔴 blockers, 10 🟡 concerns, and 1 🟢 minor from the prior review are addressed in 864dd83. Verified each fix against the diff:

• IDOR / RBAC / multi-tenancy fixes are correct and fail-closed (resolveConversation strict !==, resolveUserProfile flags removed, allowlist-driven buildUserContext, MCP providers documented + flagged)
• TOCTOU is genuinely claim-then-send via NotificationDispatchLogMapper::record() + unique-index catch + new DuplicateDispatchException
• SSE error path no longer leaks $e->getMessage(); emitAndExit() commits DB transactions before exit;
• All UTC/strict-type/validation concerns resolved; ISecureRandom dead code removed
• C4 (Message.context persistence) accepted as tracked TODO with spec link; C9/C10 are documentation-only as proposed and accurate

No new 🔴 or 🟡 issues introduced. Three documented trade-offs (claim-then-send leaves stale claim on send failure; claimIdempotencyKey fail-open on non-constraint DB errors; ObjectsToolProvider relies on ObjectService::findAll defaults rather than passing flags explicitly) are acceptable.

All 8 prior threads resolved.

Reviewed by WilcoLouwerse via /review-pr (Strict mode re-review).

[bbrands02]

Few comments, not sure if they should be blocking:

1. Blocker: /api/chat/stream is NoCSRFRequired while mutating state.
   It creates/updates conversations/messages via authenticated POST, so removing CSRF protection is risky unless this endpoint is strictly token/API-client only. Route is authenticated POST, but controller has NoCSRFRequired.
2. Blocker/spec mismatch: Message.context is added but not actually persisted.
   The controller comment says ChatService/HistoryHandler currently has no context parameter, so the new column is not populated yet. That means the PR claims context support, but runtime behavior only forwards it through a shim.
3. Risk: safeShutdown() commits unknown open transactions after errors.
   If ChatService::processMessage() throws mid-write and leaves a transaction open, the controller emits an error and then commit()s any open transaction. That can persist partial/failed state. Prefer rollback on error path, or only commit transactions explicitly started by this controller.
4. Concern: tenant key service returns plaintext old/new HMAC keys on rotation.
   rotateTenantKey() returns both old and new raw keys. That is okay only if no controller/API exposes this directly. If exposed to clients, it leaks audit signing material and should instead return metadata only.

[WilcoLouwerse]

Thanks @bbrands02 — three of the four are actionable and have been pushed as ffe43219; the fourth is already tracked as a deferred TODO.

1. CSRF on /api/chat/stream — agreed, fixed. Dropped #[NoCSRFRequired] (and the now-unused import). The "SSE / EventSource can't carry the requesttoken header" justification doesn't apply here — the endpoint is POST + fetch() with a JSON body, which can attach the requesttoken normally. The docblock now spells out why CSRF is required.

2. Message.context not persisted — confirmed; already tracked as a deferred TODO (discussion_r3228238293). The __cn_ai_context__ ragSettings shim is intentional v1; threading context through ChatService::processMessage() → HistoryHandler → MessageMapper is the follow-up. The TODO in ChatStreamController.php already references the spec section so the gap stays visible.

3. safeShutdown() commits on error path — agreed, fixed. safeShutdown() now takes a bool $rollback flag; emitAndExit() passes true when $eventType === 'error', so partial writes left behind by a failed ChatService::processMessage() are rolled back rather than persisted alongside the user-visible error frame. Success path still commits.

4. rotateTenantKey() plaintext old/new — agreed, fixed (no live exposure today, but tightened anyway). Verified via git grep: no controller, route, or CLI invokes it; only the unit tests do. The public return is now metadata-only — {tenant_id, rotated_at, retired_key_id}. Callers that legitimately need the new active key call getCurrentTenantKey() after rotation, so audit signing material never crosses the public API surface (matches the file-level docblock's "Keys are NEVER exposed through any REST endpoint."). Tests updated to assert the new contract.

phpcs/psalm clean on the touched lib/ files.