Sitelet https://github.com/nextcloud/assistant/pull/674
Skip to content

Let the chat providers know about the conversation/session ID - #674

Open
julien-nc wants to merge 1 commit into
mainfrom
enh/noid/chat-conversation-id-to-provider
Open

julien-nc wants to merge 1 commit into
mainfrom
enh/noid/chat-conversation-id-to-provider

Conversation

@julien-nc

Copy link
Copy Markdown
Member

Add extra conversation_id input to scheduled chat tasks if the current provider has it in its optional input shape

🤖 AI (if applicable)

  • The content of this PR was partly or fully generated using AI (N/A)

…t provider has it in its optional input shape

Signed-off-by: Julien Veyssier <julien-nc@posteo.net>
@coderabbitai

coderabbitai Bot commented Sep 29, 2026

Copy link
Copy Markdown

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Advanced

Run ID: ec856a28-af13-4409-a00a-52d11c3cb30f

📥 Commits

Reviewing files that changed from the base of the PR and between 7466db4 and aaeacd9.

📒 Files selected for processing (1)
  • lib/Service/ChatService.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The text, multimodal, and audio chat task scheduling methods now add the session ID as a string conversation_id when the corresponding task type declares that optional input. Otherwise, the methods leave the task input unchanged.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to aaeac

No actionable merge-blocking risk is established for the conditional conversation ID inputs.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to aaeac

Existing checks still restrict scheduling to a user’s own session, but providers may now use the session ID to associate requests with a conversation. Whether they isolate that state by user and task purpose is not established.

Retained concerns

  • Medium · security · inferred: The new provider-facing conversation_id is only a session ID and is shared by message and title tasks. The available contract does not establish whether providers isolate conversation state by user, task purpose, or deployment; state mixing is possible if a provider treats the value as a shared state key.
Security review details

Security Blast Radius

  • inferred — An authenticated user able to schedule their own chat or title tasks can cause the identifier to reach any supporting text, multimodal, or audio provider. Exposure beyond that user’s conversation depends on provider identity and state scoping, which is not established.

Security Findings and Attack Paths

  • inferred — No provider-side data disclosure is verified. A provider that uses conversation_id alone as a retained state key could combine requests that the application treats as distinct, including title and message tasks; provider-side separation is the unresolved condition.

Trust Boundaries and Controls

  • observed — Request-controlled session IDs are checked against the authenticated user before these scheduling paths run. The task retains that user ID and the existing scheduling exception handling; optional-input gating checks shape compatibility, not provider conversation ownership.

Resilience and Maintainability Implications

  • inferred — Local session deletion removes session and message records but shows no provider-conversation cleanup. Whether deletion, cancellation, retries, or concurrent tasks can leave or reuse remote state depends on provider and Task Processing behavior not available here.

Hardening Proposals

  • proposed — Confirm the consuming providers’ user, deployment, and task-purpose namespacing and conversation-state lifecycle before relying on session ID as their conversation key; if they do not provide those guarantees, use a suitably scoped identifier and define cleanup behavior.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly summarizes the main change: passing the conversation or session ID to chat providers.
Description check ✅ Passed The description directly explains that scheduled chat tasks receive conversation_id when the provider declares it as an optional input.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 1 files.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

3. to review enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant