feat(file-action): summarize a whole file by sending it to the AI provider - #616
feat(file-action): summarize a whole file by sending it to the AI provider#616cbcoutinho wants to merge 2 commits into
Conversation
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 <chris@coutinho.io>
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 <chris@coutinho.io>
|
One caveat surfaced while verifying the provider side (nextcloud/integration_openai#419) against live endpoints, because it affects what this action produces rather than whether it works. Some backends drop a document attachment without erroring, and the model then answers from nothing. Tested with a 659-byte PDF containing one invented fact ("annual rainfall in Zenda is 812 millimetres"), so a wrong answer is unambiguous:
Unpinned, the same OpenRouter request alternated between routes and answers. This is not caused by either PR — it is how those upstreams handle the (unchanged) OpenAI It does not change this PR's design. The attachment path is only taken when a provider advertises This comment was generated with the help of AI, and reviewed by a Human |
|
Thank you for your contribution! |
|
Hi @janepie sure thing 👍 I'll reply once I've confirmed via testing a real server with my feedback. Btw it looks like current developments on this app is targeting NC 35. Is there a way to backport this feature so that it works on NC 33? That's my current target instance version. |
|
The stable32 branch is the last one that supports NC33, but as we normally do not backport features and Assistant is changing fast these days this might not get released for NC33. |
|
You can use the bot for a backport though with commenting |
2 participants
Summary
Today the Summarize file action never gives the LLM the file.
TaskProcessingService::runFileActionextracts the text withAssistantService::parseTextFromFileand sends that string tocore:text2text:summary, whoseinputslot isEShapeType::Text— capped at 512 kB by core.That extraction is the weak link:
smalot/pdfparserreturns nothing for scanned PDFs and garbage for complex layouts (cf. AI summarization doesn't work for the PDFs shipping with Nextcloud ("Nextcloud Manual.pdf") #499, where the reporter's own summary was "the PDF parser is not reliable enough for in-the-wild PDFs")default:branch returns raw bytes as "text" for unsupported binariesWhen the configured provider can accept files, Summarize now hands over the document untouched and lets the model read it.
How it works
A provider advertises an optional
input_attachments(ListOfFiles) slot. The newAttachmentServicefeature-detects it viagetAvailableTaskTypes()[…]['optionalInputShape'], exactly like the existingmemoriespattern, andrunFileActionthen sends:The mandatory text input stays empty on purpose: the provider builds its own instructions around the attachment, and anything we put there would be untranslated and fight that prompt. Core accepts
''for a mandatory Text slot, and this matches the convention already used byMultimodalChatWithToolsProvider.Nothing changes until a provider advertises the slot, so this is inert on its own.
Eligibility is deliberately narrow
application/pdftext/plain,text/markdown,text/csvAll three thresholds are readable from app config.
Also included
A drive-by fix in the two file-action listeners, which both used
getFirstNodeById()without a null check. That returnsnullwhen the source file is moved or deleted while the task runs — increasingly likely as tasks get longer.In
FileActionTaskSuccessfulListenerthis was worse than a plain fatal: the null deref happened inside thetry, and thecatchhandler then dereferenced the same node again to send a notification. That second throw escaped the handler and aborted the wholeTaskSuccessfulEventdispatch, so unrelated listeners never ran.Depends on
nextcloud/integration_openai#419, which adds the
input_attachmentsslot toSummaryProvider(and fixes document attachments on Mistral). This PR is safe to merge first — without a provider that advertises the slot, behaviour is byte-identical to today.Relationship to #602
No overlap. #602 covers chat attachments via
MultimodalChatWithTools; this covers the Files → Summarize action, which #602 does not touch (noTaskProcessingService, nofileActions.js). The only shared surface is the initial-state listener.Testing
tests/unit/Service/AttachmentServiceTest.php— 21 tests covering the capability-detection matrix (slot absent / correctListOfFiles/ wrong shape type / manager throwing) and the full eligibility matrix. Passing locally.Gates run locally:
lint,cs:check(0/98),psalm(no errors),composer run openapi(openapi.jsonbyte-identical — no API surface changed),eslint,npm run build.Manual E2E still to do against a live provider: scanned PDF,
Nextcloud Manual.pdf(#499's reproducer), a 2 MB.md,.docxstill falling back to extraction, and a backend without document support falling back cleanly.This PR was generated with the help of AI, and reviewed by a Human