Repository navigation
fix(chat): pass the chat user instructions to the Context Agent - #709
marcelklehr merged 2 commits into
Conversation
The "Chat User Instructions for Chat Completions" admin setting was only used for classic chat. With the Context Agent the instructions were never sent, so agentic chat ignored them. Pass the session's system message as the optional `user_instructions` input to the classic, multimodal and audio agency tasks, if the provider declares it (Context Agent appends it to its own system prompt). Older Context Agent versions keep working as before. Fixes nextcloud#338 Signed-off-by: Viacheslav Huliaiev <vyacheslavhulyaev@gmail.com>
53a2f7f to
fea8c03
Compare
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Severity of issue fixed: Medium Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 36.84% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 3 files. (1 skipped: … | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Description check The description clearly explains that the pull request forwards chat user instructions to supported Context Agent tasks and includes testing details. Title check The title clearly and concisely summarizes the main change: passing chat user instructions to the Context Agent. Linked Issues check Issue #338requires the configured Chat User Instructions to reach Context Agent requests.ChatServicepasses the session system message as optionaluser_instructionsfor supported chat, multimoda…Out of Scope Changes check The changed files support #338. They implement instruction propagation, preserve compatibility with older Context Agent versions, update the related admin hint, and add focused tests. No unrelated cha…Full details: Linked Issues check
-
Explanation
Issue
#338requires the configured Chat User Instructions to reach Context Agent requests.ChatServicepasses the session system message as optionaluser_instructionsfor supported chat, multimodal, and audio agency tasks. Assignment runs omit the input. The audio provider forwards supported instructions and requests speech output.ChatServiceAgencyTestcovers supported, unsupported, missing, unreadable, and assignment cases.
Full details: Out of Scope Changes check
-
Explanation
The changed files support
#338. They implement instruction propagation, preserve compatibility with older Context Agent versions, update the related admin hint, and add focused tests. No unrelated change is demonstrated.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 36.84% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 19 functions across 3 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Warning
Some tools did not complete. Review the errors below.
🔧 PHPStan (2.3.0)
-
PHPStan reported 1 non-file analysis error(s); analysis is incomplete
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.
Comment @coderabbitai help to get the list of available commands.
|
Hey @vyacheslavhulyaev-glitch |
| if ($userInstructions !== '') { | ||
| $taskInput['user_instructions'] = $userInstructions; | ||
| } | ||
| } |
There was a problem hiding this comment.
This is a nice unintrusive method 👍
| if (!isset($this->taskProcessingManager->getAvailableTaskTypes()[$taskTypeId]['optionalInputShape']['user_instructions'])) { | ||
| return; | ||
| } | ||
| $userInstructions = $this->getSessionSystemPrompt($sessionId); |
There was a problem hiding this comment.
We might want to catch InternalException here
|
@vyacheslavhulyaev-glitch Thanks a lot, very nice PR.
@marcelklehr I think it's fine to send the default system prompt. The previous behaviour was kind of unexpected, now it's consistent between classic and agentic chat. @vyacheslavhulyaev-glitch Wdyt?
Does it make sense to prevent injecting this admin-defined system prompt in Assignment runs? There's a sort of safeguard for that in context_agent: the admin-defined system prompt is prefixed with @marcelklehr @vyacheslavhulyaev-glitch wdyt?
Ouch, nice catch. Can we also try to prevent markdown formatting in the main system prompt in this case so the admin-defined part about that would be ignored? |
| if (isset($input['memories'], $this->taskProcessingManager->getAvailableTaskTypes()[ContextAgentAudioInteraction::ID]['optionalInputShape']['memories'])) { | ||
| $contextAgentTaskInput['memories'] = $input['memories']; | ||
| } | ||
| /** @psalm-suppress UndefinedClass */ | ||
| if (isset($input['user_instructions'], $this->taskProcessingManager->getAvailableTaskTypes()[ContextAgentInteraction::ID]['optionalInputShape']['user_instructions'])) { | ||
| $contextAgentTaskInput['user_instructions'] = $input['user_instructions']; | ||
| } |
There was a problem hiding this comment.
it might be efficient to get the task types only once from the taskprocessing manager, even though the task types are cached since they are fetched from the distributed cache.
https://github.com/nextcloud/server/blob/fecc0bd985996b31f760369075fec09ee73e4b69/lib/private/TaskProcessing/Manager.php#L950
| {{ t('assistant', '"{user}" is a placeholder for the user\'s display name.') }} | ||
| <template v-if="state.context_agent_available"> | ||
| <br> | ||
| {{ t('assistant', 'When chatting through the Context Agent, these instructions are added to the agent\'s own instructions (requires a Context Agent version that supports it).') }} |
There was a problem hiding this comment.
could be worth mentioning the context agent version since it would be available. It will be visible to the admin only so should be fine.
I think the next version would be v2.10.0
cc @marcelklehr
| @@ -983,6 +1012,8 @@ private function scheduleAgencyAudioTask( | |||
| $taskInput['memories'] = $this->sessionSummaryService->getMemories($userId); | |||
| } | |||
| /** @psalm-suppress UndefinedClass */ | |||
| $this->addAgencyUserInstructions($taskInput, \OCP\TaskProcessing\TaskTypes\ContextAgentAudioInteraction::ID, $sessionId); | |||
| /** @psalm-suppress UndefinedClass */ | |||
There was a problem hiding this comment.
nit: the same applies here and in scheduleAgencyTask too since addAgencyUserInstructions() calls gets the task types internally but it could be fine for code readability.
wdyt?
we could try to remove this line from the context agent's prompt: in addition, Assistant could append the user instructions with something specifying that it's an audio chat so output text accordingly in the just my two cents. |
- Do not pass the chat user instructions to assignment runs, they run unattended - Ask the Context Agent for plain text in audio interactions (output_mode=speech), the reply goes to text-to-speech - Run the agent without instructions if they cannot be read - Fetch the available task types only once - Mention the required Context Agent version in the admin hint Signed-off-by: Viacheslav Huliaiev <vyacheslavhulyaev@gmail.com>
|
Thanks for the reviews! I pushed the changes here and in nextcloud/context_agent#285. Default prompt: I agree with Julien, I think it's better if classic and agentic chat behave the same, so I didn't change that. Assignments: I don't think the "unless they conflict" part helps much here. The assignment prompt is sent as a normal user message and there is no assignment specific system prompt, so nothing would override "ask for clarification". So I just don't pass the user instructions for assignment runs now. If you'd rather keep them there, it's a one line change. Audio: I added an optional @marcelklehr InternalException is caught now, it logs a warning and the agent just runs without the instructions. @kyteinsky the task types are fetched only once now (ChatService and the audio provider), and the hint mentions Context Agent 2.10.0. About removing the "unless they conflict" line and adding an audio note in |
|
Instead of |
julien-nc
left a comment
julien-nc
left a comment
There was a problem hiding this comment.
About removing the "unless they conflict" line and adding an audio note in scheduleAgencyAudioTask()
It's fine like it is now.
Fixes #338
The "Chat User Instructions for Chat Completions" admin setting was only used for classic chat. With the Context Agent the instructions were never sent, so agentic chat ignored them.
Requires nextcloud/context_agent#285, which adds the optional
user_instructionsinput and appends it to the agent's own system prompt (it does not replace it, see @janepie's comment in #338).Changes
ChatService: the classic, multimodal and audio agency tasks now get the session's system message asuser_instructions, but only if the task type declares that optional input. That's the same check as formemories, so older Context Agent versions keep working unchanged. Reading the system message was moved into a helper shared with classic chat.ContextAgentAudioInteractionProvider: declares the input as well and forwards it to theContextAgentInteractionsubtask.ChatServiceAgencyTest: instructions are sent / not sent when the agent does not support them / not sent when the session has no system message.Testing
Local Nextcloud 35.0.2 with Context Agent from nextcloud/context_agent#285 and OpenAI
gpt-4.1-mini:ChatServiceAgencyTest: 3/3 pass, and the first test fails without the fixAgentSkillsServiceTest::testExtractFrontmatterValid, which also fails onmainlocally (nofiles_externaltables in my dev instance)cs:checkclean, Psalm no errors, frontend buildsNotes / questions
scheduleAgencyTask, so they get the instructions too. Is that wanted?