From 97188205728fc7f2cd727176de05f88593b4b041 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Mon, 27 Jul 2026 22:09:01 +0200 Subject: [PATCH 1/2] fix(file-action): handle the source file disappearing mid-task Both file action listeners looked the source file up with getFirstNodeById() and used the result without checking it. That call returns null when the file was moved or deleted while the task ran, which is easy to hit now that tasks can take a while. In the successful listener this was worse than a plain fatal: the null deref happened inside the try block, and the catch handler then dereferenced the same node again to send a notification. That second throw escaped the handler and aborted the whole TaskSuccessfulEvent dispatch, so unrelated listeners never ran. Bail out with a warning in both listeners instead. Signed-off-by: Chris Coutinho --- lib/Listener/FileActionTaskFailedListener.php | 8 ++++++++ lib/Listener/FileActionTaskSuccessfulListener.php | 8 ++++++++ 2 files changed, 16 insertions(+) diff --git a/lib/Listener/FileActionTaskFailedListener.php b/lib/Listener/FileActionTaskFailedListener.php index 0e8bfa296..68303f010 100644 --- a/lib/Listener/FileActionTaskFailedListener.php +++ b/lib/Listener/FileActionTaskFailedListener.php @@ -14,6 +14,7 @@ use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\Files\IRootFolder; +use OCP\Files\Node; use OCP\TaskProcessing\Events\TaskFailedEvent; use Psr\Log\LoggerInterface; @@ -52,6 +53,13 @@ public function handle(Event $event): void { $this->logger->debug('FileActionTaskListener', ['source_file_id' => $sourceFileId]); $userFolder = $this->rootFolder->getUserFolder($task->getUserId()); $sourceFile = $userFolder->getFirstNodeById($sourceFileId); + if (!$sourceFile instanceof Node) { + // the source file was removed while the task was running, there is nothing to point the user at + $this->logger->warning('FileActionTaskListener task failed and the source file is gone', [ + 'source_file_id' => $sourceFileId, + ]); + return; + } $this->notificationService->sendFileActionNotification( $task->getUserId(), $taskTypeId, $task->getId(), $sourceFileId, $sourceFile->getName(), $userFolder->getRelativePath($sourceFile->getPath()), diff --git a/lib/Listener/FileActionTaskSuccessfulListener.php b/lib/Listener/FileActionTaskSuccessfulListener.php index 3ee5c3140..3482169bf 100644 --- a/lib/Listener/FileActionTaskSuccessfulListener.php +++ b/lib/Listener/FileActionTaskSuccessfulListener.php @@ -15,6 +15,7 @@ use OCP\EventDispatcher\Event; use OCP\EventDispatcher\IEventListener; use OCP\Files\IRootFolder; +use OCP\Files\Node; use OCP\SystemTag\TagNotFoundException; use OCP\TaskProcessing\Events\TaskSuccessfulEvent; use OCP\TaskProcessing\TaskTypes\TextToTextSummary; @@ -57,6 +58,13 @@ public function handle(Event $event): void { $userFolder = $this->rootFolder->getUserFolder($task->getUserId()); $sourceFile = $userFolder->getFirstNodeById($sourceFileId); $this->logger->debug('FileActionTaskListener', ['source file id' => $sourceFileId]); + if (!$sourceFile instanceof Node) { + // the source file was removed while the task was running, there is nowhere to write the result + $this->logger->warning('FileActionTaskListener task succeeded but the source file is gone', [ + 'source file id' => $sourceFileId, + ]); + return; + } try { $sourceFileParent = $sourceFile->getParent(); $this->logger->debug('FileActionTaskListener', ['source file PARENT id' => $sourceFileParent->getId()]); From cfb103dc6d382d574860322af7779707907a88b0 Mon Sep 17 00:00:00 2001 From: Chris Coutinho Date: Mon, 27 Jul 2026 22:09:14 +0200 Subject: [PATCH 2/2] feat(file-action): summarize a file by sending it to the AI provider The summarize file action always extracted the text itself and passed the result to core:text2text:summary as a Text input. That extraction is the weak part of the flow: the PDF parser returns nothing for scanned documents and garbage for complex layouts, and core caps Text inputs at 512 kB, so a large file fails outright before it ever reaches a model. Providers can now advertise an optional input_attachments slot, and when one does we hand it the file untouched instead. The mandatory text input stays empty in that case; the provider builds its own instructions around the attachment, and anything we put there would be untranslated and fight that prompt. Eligibility is deliberately narrow. PDFs always go to the provider. Text, markdown and csv keep using local extraction unless they are big enough that the extracted text would not fit in a Text input. Office formats are never attached, since providers reject them and only our own parsers can read them. The thresholds are readable from app config. Nothing changes when no provider advertises the slot, so this is inert until a provider supports it. Signed-off-by: Chris Coutinho --- .../LoadAdditionalScriptsListener.php | 8 + lib/Service/AttachmentService.php | 143 +++++++++++++++++ lib/Service/TaskProcessingService.php | 48 +++++- src/files/fileActions.js | 22 ++- tests/unit/Service/AttachmentServiceTest.php | 151 ++++++++++++++++++ 5 files changed, 361 insertions(+), 11 deletions(-) create mode 100644 lib/Service/AttachmentService.php create mode 100644 tests/unit/Service/AttachmentServiceTest.php diff --git a/lib/Listener/LoadAdditionalScriptsListener.php b/lib/Listener/LoadAdditionalScriptsListener.php index 5043d4831..2bf74e177 100644 --- a/lib/Listener/LoadAdditionalScriptsListener.php +++ b/lib/Listener/LoadAdditionalScriptsListener.php @@ -8,6 +8,7 @@ namespace OCA\Assistant\Listener; use OCA\Assistant\AppInfo\Application; +use OCA\Assistant\Service\AttachmentService; use OCA\Files\Event\LoadAdditionalScriptsEvent; use OCP\AppFramework\Services\IInitialState; use OCP\EventDispatcher\Event; @@ -28,6 +29,7 @@ public function __construct( private IAppConfig $appConfig, private IInitialState $initialStateService, private ITaskProcessingManager $taskProcessingManager, + private AttachmentService $attachmentService, ) { } @@ -46,6 +48,12 @@ public function handle(Event $event): void { $this->initialStateService->provideInitialState('stt-available', $sttAvailable); $this->initialStateService->provideInitialState('tts-available', $ttsAvailable); $this->initialStateService->provideInitialState('summarize-available', $summarizeAvailable); + // extra mime types the summarize action can handle by sending the file to the provider, + // on top of the ones we can extract text from ourselves + $this->initialStateService->provideInitialState( + 'summarize-attachment-mime-types', + $summarizeAvailable ? $this->attachmentService->getAttachableMimeTypes(TextToTextSummary::ID) : [], + ); Util::addInitScript(Application::APP_ID, Application::APP_ID . '-fileActions'); // New file menu to generate images diff --git a/lib/Service/AttachmentService.php b/lib/Service/AttachmentService.php new file mode 100644 index 000000000..13afa132a --- /dev/null +++ b/lib/Service/AttachmentService.php @@ -0,0 +1,143 @@ +taskProcessingManager->getAvailableTaskTypes(); + $shape = $taskTypes[$taskTypeId]['optionalInputShape']['input_attachments'] ?? null; + return $shape instanceof ShapeDescriptor + && $shape->getShapeType() === EShapeType::ListOfFiles; + } catch (\Throwable $e) { + // this is also called while rendering templates, never let it bubble up + $this->logger->debug('Could not determine attachment support for ' . $taskTypeId, ['exception' => $e]); + return false; + } + } + + /** + * Whether $file is one we would rather hand to the model directly. + */ + public function isAttachableFile(File $file): bool { + if ($file->getSize() > $this->getMaxAttachmentSize()) { + return false; + } + $mimeType = $file->getMimeType(); + if (in_array($mimeType, self::ATTACHABLE_MIME_TYPES, true)) { + return true; + } + return in_array($mimeType, self::EXTRACTABLE_TEXT_MIME_TYPES, true) + && $file->getSize() > $this->getTextInlineThreshold(); + } + + /** + * Whether the file behind $fileId should be attached to a $taskTypeId task + * rather than parsed into text first. + */ + public function shouldAttachFile(File $file, string $taskTypeId): bool { + return $this->providerSupportsInputAttachments($taskTypeId) + && $this->isAttachableFile($file); + } + + /** + * The mime types the summarize file action can handle over and above the + * ones we can extract text from. + * + * These end up in a file action `enabled()` check that compares mime types + * exactly, so only concrete types belong here - no wildcards. + * + * @return list + */ + public function getAttachableMimeTypes(string $taskTypeId): array { + if (!$this->providerSupportsInputAttachments($taskTypeId)) { + return []; + } + return self::ATTACHABLE_MIME_TYPES; + } + + private function getMaxAttachmentSize(): int { + return $this->appConfig->getValueInt( + Application::APP_ID, + 'max_attachment_size', + self::DEFAULT_MAX_ATTACHMENT_SIZE, + lazy: true, + ); + } + + private function getTextInlineThreshold(): int { + return $this->appConfig->getValueInt( + Application::APP_ID, + 'text_inline_threshold', + self::DEFAULT_TEXT_INLINE_THRESHOLD, + lazy: true, + ); + } +} diff --git a/lib/Service/TaskProcessingService.php b/lib/Service/TaskProcessingService.php index 0db8b2866..cf228a7a7 100644 --- a/lib/Service/TaskProcessingService.php +++ b/lib/Service/TaskProcessingService.php @@ -33,6 +33,7 @@ public function __construct( private IRootFolder $rootFolder, private LoggerInterface $logger, private AssistantService $assistantService, + private AttachmentService $attachmentService, ) { } @@ -88,6 +89,15 @@ public function getOutputFileContent(int $fileId): string { return $file->getContent(); } + /** + * Task types that take the input file itself rather than its text content. + */ + private function isAudioInputTaskType(string $taskTypeId): bool { + return $taskTypeId === AudioToText::ID + || (class_exists('OCP\\TaskProcessing\\TaskTypes\\AudioToTextSubtitles') + && $taskTypeId === \OCP\TaskProcessing\TaskTypes\AudioToTextSubtitles::ID); + } + public function isFileActionTaskTypeSupported(string $taskTypeId): bool { $authorizedTaskTypes = [AudioToText::ID, TextToTextSummary::ID]; if (class_exists('OCP\\TaskProcessing\\TaskTypes\\TextToSpeech')) { @@ -114,9 +124,7 @@ public function runFileAction(string $userId, int $fileId, string $taskTypeId): throw new Exception('Invalid task type for file action'); } try { - $input = ($taskTypeId === AudioToText::ID) || (class_exists('OCP\\TaskProcessing\\TaskTypes\\AudioToTextSubtitles') && $taskTypeId === \OCP\TaskProcessing\TaskTypes\AudioToTextSubtitles::ID) - ? ['input' => $fileId] - : ['input' => $this->assistantService->parseTextFromFile($userId, fileId: $fileId)]; + $input = $this->buildFileActionInput($userId, $fileId, $taskTypeId); } catch (NotPermittedException|GenericFileException|LockedException|\OCP\Files\NotFoundException|Exception $e) { $this->logger->warning('Assistant runFileAction, impossible to read the file action input file', ['exception' => $e]); throw new Exception('Impossible to read the file action input file'); @@ -140,4 +148,38 @@ public function runFileAction(string $userId, int $fileId, string $taskTypeId): } return $taskId; } + + /** + * Build the task input for a file action. + * + * Audio task types consume the file directly. For the others we normally + * extract the text ourselves, but when the provider can take files and the + * file is one our parsers handle badly, we hand it over untouched instead. + * + * The mandatory text input stays empty in that case: the provider builds + * its own instructions around the attachment, and anything we put there + * would be untranslated and fight that prompt. + * + * @return array + * @throws GenericFileException + * @throws LockedException + * @throws NotPermittedException + * @throws \OCP\Files\NotFoundException + */ + private function buildFileActionInput(string $userId, int $fileId, string $taskTypeId): array { + if ($this->isAudioInputTaskType($taskTypeId)) { + return ['input' => $fileId]; + } + + $file = $this->rootFolder->getUserFolder($userId)->getFirstNodeById($fileId); + if ($file instanceof File && $this->attachmentService->shouldAttachFile($file, $taskTypeId)) { + $this->logger->debug('Assistant file action: sending the file to the provider as an attachment', [ + 'fileId' => $fileId, + 'taskTypeId' => $taskTypeId, + ]); + return ['input' => '', 'input_attachments' => [$fileId]]; + } + + return ['input' => $this->assistantService->parseTextFromFile($userId, fileId: $fileId)]; + } } diff --git a/src/files/fileActions.js b/src/files/fileActions.js index f58b87d13..0f4369dc8 100644 --- a/src/files/fileActions.js +++ b/src/files/fileActions.js @@ -39,7 +39,7 @@ function registerGroupAction(mimeTypes) { registerFileAction(groupAction) } -function registerSummarizeAction() { +function registerSummarizeAction(mimeTypes) { const summarizeAction = { id: 'assistant-summarize', parent: 'assistant-group', @@ -51,7 +51,7 @@ function registerSummarizeAction() { && nodes.length === 1 && !nodes.some(({ permissions }) => (permissions & Permission.READ) === 0) && nodes.every(({ type }) => type === FileType.File) - && nodes.every(({ mime }) => VALID_TEXT_MIME_TYPES.includes(mime)) + && nodes.every(({ mime }) => mimeTypes.includes(mime)) }, iconSvgInline: () => SummarizeSymbol, order: 0, @@ -199,24 +199,30 @@ const assistantEnabled = loadState('assistant', 'assistant-enabled', false) const summarizeAvailable = loadState('assistant', 'summarize-available', false) const sttAvailable = loadState('assistant', 'stt-available', false) const ttsAvailable = loadState('assistant', 'tts-available', false) +// files the AI provider can read itself, so summarizing them does not depend on our text extraction +const summarizeAttachmentMimeTypes = loadState('assistant', 'summarize-attachment-mime-types', []) +const summarizeMimeTypes = [...new Set([...VALID_TEXT_MIME_TYPES, ...summarizeAttachmentMimeTypes])] if (assistantEnabled) { if (summarizeAvailable || sttAvailable || ttsAvailable) { - const groupMimeTypes = [] - if (summarizeAvailable || ttsAvailable) { - groupMimeTypes.push(...VALID_TEXT_MIME_TYPES) + const groupMimeTypes = new Set() + if (summarizeAvailable) { + summarizeMimeTypes.forEach((mime) => groupMimeTypes.add(mime)) + } + if (ttsAvailable) { + VALID_TEXT_MIME_TYPES.forEach((mime) => groupMimeTypes.add(mime)) } if (sttAvailable) { - groupMimeTypes.push(...VALID_AUDIO_MIME_TYPES) + VALID_AUDIO_MIME_TYPES.forEach((mime) => groupMimeTypes.add(mime)) } - registerGroupAction(groupMimeTypes) + registerGroupAction([...groupMimeTypes]) } if (sttAvailable) { registerSttAction() registerSttSubtitlesAction() } if (summarizeAvailable) { - registerSummarizeAction() + registerSummarizeAction(summarizeMimeTypes) } if (ttsAvailable) { registerTtsAction() diff --git a/tests/unit/Service/AttachmentServiceTest.php b/tests/unit/Service/AttachmentServiceTest.php new file mode 100644 index 000000000..274b97953 --- /dev/null +++ b/tests/unit/Service/AttachmentServiceTest.php @@ -0,0 +1,151 @@ +taskProcessingManager = $this->createMock(IManager::class); + + $appConfig = $this->createMock(IAppConfig::class); + // no admin overrides, always hand back the default + $appConfig->method('getValueInt')->willReturnCallback( + static fn (string $app, string $key, int $default = 0, bool $lazy = false): int => $default, + ); + + return new AttachmentService( + $this->taskProcessingManager, + $appConfig, + $this->createMock(LoggerInterface::class), + ); + } + + private function mockFile(string $mimeType, int $size): File&MockObject { + $file = $this->createMock(File::class); + $file->method('getMimeType')->willReturn($mimeType); + $file->method('getSize')->willReturn($size); + return $file; + } + + /** + * @param ?ShapeDescriptor $shape the input_attachments descriptor the provider advertises, if any + */ + private function mockAvailableTaskTypes(?ShapeDescriptor $shape): void { + $this->taskProcessingManager->method('getAvailableTaskTypes')->willReturn([ + self::TASK_TYPE_ID => [ + 'optionalInputShape' => $shape === null ? [] : ['input_attachments' => $shape], + ], + ]); + } + + public function testProviderWithoutAttachmentSlotIsNotSupported(): void { + $service = $this->buildService(); + $this->mockAvailableTaskTypes(null); + self::assertFalse($service->providerSupportsInputAttachments(self::TASK_TYPE_ID)); + } + + public function testProviderAdvertisingListOfFilesIsSupported(): void { + $service = $this->buildService(); + $this->mockAvailableTaskTypes(new ShapeDescriptor('Attachments', 'Files', EShapeType::ListOfFiles)); + self::assertTrue($service->providerSupportsInputAttachments(self::TASK_TYPE_ID)); + } + + public function testSlotWithTheWrongShapeTypeIsNotSupported(): void { + $service = $this->buildService(); + // a provider using the same key for something else must not switch the feature on + $this->mockAvailableTaskTypes(new ShapeDescriptor('Attachments', 'Files', EShapeType::Text)); + self::assertFalse($service->providerSupportsInputAttachments(self::TASK_TYPE_ID)); + } + + public function testUnknownTaskTypeIsNotSupported(): void { + $service = $this->buildService(); + $this->mockAvailableTaskTypes(new ShapeDescriptor('Attachments', 'Files', EShapeType::ListOfFiles)); + self::assertFalse($service->providerSupportsInputAttachments('core:text2text:chat')); + } + + public function testManagerFailureDoesNotBubbleUp(): void { + $service = $this->buildService(); + // this runs while rendering templates, a throw here would break the page + $this->taskProcessingManager->method('getAvailableTaskTypes') + ->willThrowException(new \RuntimeException('boom')); + self::assertFalse($service->providerSupportsInputAttachments(self::TASK_TYPE_ID)); + } + + /** + * @dataProvider attachableFileDataProvider + */ + public function testIsAttachableFile(string $mimeType, int $size, bool $expected): void { + $service = $this->buildService(); + self::assertSame($expected, $service->isAttachableFile($this->mockFile($mimeType, $size))); + } + + public function attachableFileDataProvider(): array { + return [ + // PDFs always go to the provider, our parser is the unreliable part + 'small pdf' => ['application/pdf', 12_000, true], + 'large pdf' => ['application/pdf', 20_000_000, true], + 'oversized pdf' => ['application/pdf', 80_000_000, false], + // text extracts fine and cheaply until it no longer fits in a Text input + 'small markdown' => ['text/markdown', 10_000, false], + 'large markdown' => ['text/markdown', 1_000_000, true], + 'small plain text' => ['text/plain', 1_000, false], + 'large plain text' => ['text/plain', 500_000, true], + 'large csv' => ['text/csv', 400_000, true], + // office formats are rejected provider side, they must keep using text extraction + 'docx' => ['application/vnd.openxmlformats-officedocument.wordprocessingml.document', 5_000_000, false], + 'odt' => ['application/vnd.oasis.opendocument.text', 900_000, false], + 'rtf' => ['text/rtf', 900_000, false], + 'image' => ['image/jpeg', 900_000, false], + ]; + } + + public function testShouldAttachFileRequiresBothProviderSupportAndAnEligibleFile(): void { + $service = $this->buildService(); + $this->mockAvailableTaskTypes(new ShapeDescriptor('Attachments', 'Files', EShapeType::ListOfFiles)); + + self::assertTrue($service->shouldAttachFile($this->mockFile('application/pdf', 12_000), self::TASK_TYPE_ID)); + self::assertFalse($service->shouldAttachFile($this->mockFile('text/markdown', 10_000), self::TASK_TYPE_ID)); + } + + public function testShouldNotAttachFileWhenTheProviderCannotTakeAttachments(): void { + $service = $this->buildService(); + $this->mockAvailableTaskTypes(null); + self::assertFalse($service->shouldAttachFile($this->mockFile('application/pdf', 12_000), self::TASK_TYPE_ID)); + } + + public function testAttachableMimeTypesAreOnlyAdvertisedWhenTheProviderSupportsThem(): void { + $service = $this->buildService(); + $this->mockAvailableTaskTypes(new ShapeDescriptor('Attachments', 'Files', EShapeType::ListOfFiles)); + $mimeTypes = $service->getAttachableMimeTypes(self::TASK_TYPE_ID); + + self::assertContains('application/pdf', $mimeTypes); + // the file action compares mime types exactly, wildcards would never match + foreach ($mimeTypes as $mimeType) { + self::assertStringNotContainsString('*', $mimeType); + } + } + + public function testNoAttachableMimeTypesWithoutProviderSupport(): void { + $service = $this->buildService(); + $this->mockAvailableTaskTypes(null); + self::assertSame([], $service->getAttachableMimeTypes(self::TASK_TYPE_ID)); + } +}