feat: chain classification after OCR with AUTO_OCR_THEN_CLASSIFY - #926
Open
adamflagg wants to merge 9 commits into
Open
feat: chain classification after OCR with AUTO_OCR_THEN_CLASSIFY#926adamflagg wants to merge 9 commits into
adamflagg wants to merge 9 commits into
Conversation
Red phase: tests for SetPrompt/GetPrompt on LLMProvider and renderOCRPrompt function that renders OCR templates per-document with existing document content. Ref: icereed#882
Moves OCR template rendering from startup-only to per-document,
following the same pattern used by title, tag, correspondent, and
all other prompt templates. Enables the OCR prompt to reference the
document's existing text via {{.Content}}.
Changes:
- Add renderOCRPrompt() for per-document template rendering
- Add SetPrompt/GetPrompt on LLMProvider for per-document overrides
- Add ExistingContent to OCROptions, passed from background processor
- Update default ocr_prompt.tmpl with conditional {{.Content}} block
- Startup rendering passes Content="" as fallback (backward compatible)
Templates without {{.Content}} work identically to before. Users with
custom prompts opt in by adding {{if .Content}}...{{end}} to their
template.
Ref: icereed#882
Red phase: tests for classifyDocument extraction and AUTO_OCR_THEN_CLASSIFY chaining behavior. Covers: - classifyDocument function exists and is callable - Chaining disabled: OCR-only, no classification fields - Chaining enabled + classify fails: OCR content still written Ref: icereed#165, icereed#449
When AUTO_OCR_THEN_CLASSIFY=true, documents processed for OCR are immediately classified (title, tags, correspondent, document type, date, custom fields) using the freshly OCR'd text in memory — no re-fetch from paperless-ngx needed. Changes: - Add AUTO_OCR_THEN_CLASSIFY env var (default: false) - Extract classifyDocument() from processAutoTagDocuments for reuse - After OCR completes, optionally chain into classification - Merge classification results into the OCR update (single API call) - Remove both AUTO_OCR_TAG and AUTO_TAG when chaining Error handling preserves OCR work: if classification fails, the OCR text is still written back and the OCR tag removed. Users can retry classification by applying the AUTO_TAG. Closes icereed#165 Closes icereed#449 Ref: icereed#497
- Replace SetPrompt with WithPrompt to avoid mutating the shared ocrProvider singleton (concurrency safety) - Cap existing content to 8000 chars before injecting into OCR prompt to avoid blowing vision model context on long documents - Tighten language test assertion with t.Setenv and assert.Equal Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…m debug log - Remove empty-string guard so intentionally-empty templates take effect - Log prompt length instead of full content in debug output Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds per-document OCR prompting, a new exported App.classifyDocument method, OCROptions.ExistingContent, and OCR→classification chaining in background processing; updates OCR provider to support per-document prompts and adds tests for chaining and prompt behavior. Changes
Sequence DiagramsequenceDiagram
participant App as App
participant OCR as OCR Engine
participant Classifier as Classifier
participant DB as Database
App->>OCR: renderOCRPrompt(existingContent) → produce per-doc prompt
Note over OCR: prompt truncated to 8000 chars\nprovider cloned with prompt
App->>OCR: ProcessDocument / ProcessImage using scoped provider
OCR-->>App: OCR content
alt AUTO_OCR_THEN_CLASSIFY enabled
App->>Classifier: classifyDocument(document + OCR content)
Classifier-->>App: DocumentSuggestion
App->>App: Merge OCR content + suggestions\nremove AUTO_TAG on success
else
App->>App: Persist OCR content only (no classification)
end
App->>DB: UpdateDocuments(merged suggestion & content)
DB-->>App: Update complete
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
ocr_prompt.go (1)
16-19: Consider Unicode-safe truncation.The current truncation
existingContent[:maxOCRExistingContentLen]may split a multi-byte UTF-8 rune if the content contains non-ASCII characters (e.g., German, Chinese, Arabic text). This could produce invalid UTF-8 at the truncation point.♻️ Proposed Unicode-safe truncation
func renderOCRPrompt(existingContent string) (string, error) { if len(existingContent) > maxOCRExistingContentLen { - existingContent = existingContent[:maxOCRExistingContentLen] + // Truncate to maxOCRExistingContentLen bytes, then trim to last valid rune + existingContent = strings.ToValidUTF8(existingContent[:maxOCRExistingContentLen], "") }Alternatively, use rune-based truncation if exact character count matters more than byte count.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ocr_prompt.go` around lines 16 - 19, The truncation in renderOCRPrompt currently slices bytes (existingContent[:maxOCRExistingContentLen]) which can cut multibyte UTF‑8 runes; change it to perform Unicode-safe truncation by operating on runes (e.g., convert existingContent to []rune and slice to maxOCRExistingContentLen runes or use utf8 utilities) so existingContent is never cut in the middle of a rune; update the code around renderOCRPrompt and the maxOCRExistingContentLen usage accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@background_chaining_test.go`:
- Around line 141-158: The test disables all generators (autoGenerateTitle,
autoGenerateTags, autoGenerateCorrespondents, autoGenerateCreatedDate,
autoGenerateDocumentType) but only asserts RemoveTags changed; update the
success-path assertions in background_chaining_test.go (and the similar block at
lines ~199-205) to also assert that the merged classification returned by the
background processing contains the copied fields from the suggestion object—e.g.
SuggestedTitle, SuggestedCorrespondents, SuggestedCreatedDate,
SuggestedDocumentType/custom fields—not just tags, so verify those specific
fields are present and equal to the expected suggested values after
classifyDocument/background processing.
- Around line 27-30: The test mutates package globals (autoTag, autoOcrTag,
pdfOCRCompleteTag, ocrTemplate) and does not restore them, causing
order-dependent failures; update TestClassifyDocument_Exists (and the other
affected tests around lines noted) to save the original values at start and
register a cleanup to restore them (e.g., origAutoTag := autoTag;
t.Cleanup(func(){ autoTag = origAutoTag })) so each test resets autoTag,
autoOcrTag, pdfOCRCompleteTag, and ocrTemplate after running.
In `@background.go`:
- Around line 283-295: The chained OCR->classification path reuses the
lightweight document from GetDocumentsByTag and may miss custom fields; before
calling app.classifyDocument you should reload the full document (use the
existing GetDocument method on app to fetch the document by ID into classifyDoc)
then set classifyDoc.Content = processedDoc.Text,
refreshCustomFieldsCache(app.Client) as before, and then call
app.classifyDocument(ctx, classifyDoc, docLogger) so classification runs against
the complete document state.
- Around line 300-311: The merge currently only appends autoTag to
documentSuggestion.RemoveTags and discards any tags classifySuggestion intended
to remove; update the merge to preserve all classification removals by appending
classifySuggestion.RemoveTags (and autoTag if needed) into
documentSuggestion.RemoveTags, ensuring you combine (and optionally deduplicate)
the slices so documentSuggestion.RemoveTags contains all tags from
classifySuggestion.RemoveTags plus autoTag; modify the code around
documentSuggestion.RemoveTags and autoTag references to perform this merge
instead of only appending autoTag.
- Around line 87-95: classifyDocument reads settings.CustomFieldsEnable without
synchronization causing a race when run from background goroutines; wrap the
access in the same settingsMutex used elsewhere (e.g., use
settingsMutex.RLock()/RUnlock() or acquire the mutex and copy the needed
settings fields into local variables) before constructing
GenerateSuggestionsRequest so all reads from settings are protected; ensure you
reference the same settings and settingsMutex symbols to avoid introducing a new
lock and avoid holding the lock longer than necessary by copying values out
first.
---
Nitpick comments:
In `@ocr_prompt.go`:
- Around line 16-19: The truncation in renderOCRPrompt currently slices bytes
(existingContent[:maxOCRExistingContentLen]) which can cut multibyte UTF‑8
runes; change it to perform Unicode-safe truncation by operating on runes (e.g.,
convert existingContent to []rune and slice to maxOCRExistingContentLen runes or
use utf8 utilities) so existingContent is never cut in the middle of a rune;
update the code around renderOCRPrompt and the maxOCRExistingContentLen usage
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ce2ea496-0527-4c02-8a0f-7e6fae221432
📒 Files selected for processing (10)
background.gobackground_chaining_test.godefault_prompts/ocr_prompt.tmplmain.goocr.goocr/llm_provider.goocr/llm_provider_prompt_test.goocr_prompt.goocr_prompt_test.gotypes.go
- Restore all mutated package globals in tests via t.Cleanup - Fetch full document (GetDocument) before chained classification to match the auto-tag path and include custom fields - Merge classification RemoveTags with dedup instead of only appending autoTag Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
background.go (1)
86-95:⚠️ Potential issue | 🔴 CriticalGuard
settings.CustomFieldsEnablewithsettingsMutex.The direct read on Line 94 can still race with runtime settings updates now that this helper runs on background goroutines.
Suggested fix
func (app *App) classifyDocument(ctx context.Context, document Document, logger *logrus.Entry) (*DocumentSuggestion, error) { + settingsMutex.RLock() + generateCustomFields := settings.CustomFieldsEnable + settingsMutex.RUnlock() + suggestionRequest := GenerateSuggestionsRequest{ Documents: []Document{document}, GenerateTitles: strings.ToLower(autoGenerateTitle) != "false", GenerateTags: strings.ToLower(autoGenerateTags) != "false", GenerateCorrespondents: strings.ToLower(autoGenerateCorrespondents) != "false", GenerateDocumentTypes: strings.ToLower(autoGenerateDocumentType) != "false", GenerateCreatedDate: strings.ToLower(autoGenerateCreatedDate) != "false", - GenerateCustomFields: settings.CustomFieldsEnable, + GenerateCustomFields: generateCustomFields, }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@background.go` around lines 86 - 95, The read of settings.CustomFieldsEnable in classifyDocument races with runtime updates; wrap that read with the settingsMutex RLock/RUnlock: in classifyDocument acquire settingsMutex.RLock(), copy settings.CustomFieldsEnable into a local bool (e.g. customFieldsEnabled), defer settingsMutex.RUnlock(), and then use that local variable when constructing GenerateSuggestionsRequest so background goroutines no longer read settings directly without the lock.background_chaining_test.go (1)
168-220:⚠️ Potential issue | 🟡 MinorAssert at least one merged classification field in the success-path test.
Because all generators are disabled here, this test currently only proves that
RemoveTagswas merged. It would still pass if the OCR→classification path stopped copying fields like title, correspondent, created date, document type, or custom fields.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@background_chaining_test.go` around lines 168 - 220, The test currently only asserts RemoveTags and OCR content; update the success-path assertions after calling app.processAutoOcrTagDocuments to also verify that at least one merged classification field was applied (e.g., check client.lastSuggestions[0].SuggestedTitle, SuggestedCorrespondents, SuggestedCreatedDate, SuggestedDocumentType, or SuggestedCustomFields is non-empty/non-nil) so the OCR→classification merge behavior in processAutoOcrTagDocuments is actually validated; use mockClientWithCapture.lastSuggestions[0] to inspect these fields and assert that at least one of them contains the expected merged value.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@background.go`:
- Around line 287-299: If GetDocument(ctx, document.ID) returns an error, do not
fall back to the lightweight document or perform chained classification; instead
log the error and skip calling app.classifyDocument so AUTO_TAG can retry later.
Concretely, in the block around GetDocument / classifyDoc / processedDoc.Text /
refreshCustomFieldsCache, remove the fallback assignment classifyDoc = document
when fetchErr != nil and gate the call to app.classifyDocument on fetchErr ==
nil (keeping the OCR update processedDoc.Text in place); ensure you still
refresh any caches only when you will classify, and do not call
app.classifyDocument if the full fetch failed.
---
Duplicate comments:
In `@background_chaining_test.go`:
- Around line 168-220: The test currently only asserts RemoveTags and OCR
content; update the success-path assertions after calling
app.processAutoOcrTagDocuments to also verify that at least one merged
classification field was applied (e.g., check
client.lastSuggestions[0].SuggestedTitle, SuggestedCorrespondents,
SuggestedCreatedDate, SuggestedDocumentType, or SuggestedCustomFields is
non-empty/non-nil) so the OCR→classification merge behavior in
processAutoOcrTagDocuments is actually validated; use
mockClientWithCapture.lastSuggestions[0] to inspect these fields and assert that
at least one of them contains the expected merged value.
In `@background.go`:
- Around line 86-95: The read of settings.CustomFieldsEnable in classifyDocument
races with runtime updates; wrap that read with the settingsMutex RLock/RUnlock:
in classifyDocument acquire settingsMutex.RLock(), copy
settings.CustomFieldsEnable into a local bool (e.g. customFieldsEnabled), defer
settingsMutex.RUnlock(), and then use that local variable when constructing
GenerateSuggestionsRequest so background goroutines no longer read settings
directly without the lock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 613e04af-6bc5-4949-b19e-20352a3076c2
📒 Files selected for processing (2)
background.gobackground_chaining_test.go
Don't fall back to the lightweight GetDocumentsByTag document when GetDocument fails — classification on incomplete state (missing custom fields) could produce wrong results. Instead skip classification and let the user retry via AUTO_TAG. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
background.go (1)
262-265:⚠️ Potential issue | 🔴 CriticalUpdate DocumentSuggestion.OriginalDocument to use the fully loaded document after classification.
Line 264 sets
OriginalDocument: documentfrom GetDocumentsByTag, which omits CustomFields and DocumentTypeName. When UpdateDocuments merges custom field suggestions (line 587 in paperless.go), it operates on empty CustomFields.After the classification fetch at line 294, use
classifyDoc(or the original document if classification is skipped) for both classification and the DocumentSuggestion's OriginalDocument field to ensure UpdateDocuments has complete state for merging.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@background.go` around lines 262 - 265, The DocumentSuggestion being constructed uses the lightweight document returned by GetDocumentsByTag (OriginalDocument: document) which lacks CustomFields and DocumentTypeName; change the code so OriginalDocument is set to the fully loaded/classified document (the result of classifyDoc or the original document when classification is skipped) before creating DocumentSuggestion. Locate the creation of DocumentSuggestion and replace OriginalDocument: document with OriginalDocument: classifiedDoc (or the variable you use to hold the post-classification/full document), ensuring classifyDoc is invoked where applicable and that UpdateDocuments will receive the complete document state for merging custom field suggestions (the same object used in the classification branch).
🧹 Nitpick comments (1)
background.go (1)
297-298: Refresh the custom-fields cache once per batch, not per document.
refreshCustomFieldsCachedoes a fresh Paperless fetch each time, so calling it here adds an extra network round-trip for every OCR’d document, even when custom-field generation is off. Consider moving this refresh outside the loop or gating it on custom-field generation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@background.go` around lines 297 - 298, refreshCustomFieldsCache(app.Client) is being called inside the per-document classification loop causing a network fetch for every OCR'd document; move the refresh out of that loop so it runs once per batch or only when custom-field generation is enabled. Update the code around the classification loop to call refreshCustomFieldsCache(app.Client) once before iterating documents (or wrap the call with the feature flag/generation check used for custom-field creation) so you avoid per-document network round-trips.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@background.go`:
- Around line 262-265: The DocumentSuggestion being constructed uses the
lightweight document returned by GetDocumentsByTag (OriginalDocument: document)
which lacks CustomFields and DocumentTypeName; change the code so
OriginalDocument is set to the fully loaded/classified document (the result of
classifyDoc or the original document when classification is skipped) before
creating DocumentSuggestion. Locate the creation of DocumentSuggestion and
replace OriginalDocument: document with OriginalDocument: classifiedDoc (or the
variable you use to hold the post-classification/full document), ensuring
classifyDoc is invoked where applicable and that UpdateDocuments will receive
the complete document state for merging custom field suggestions (the same
object used in the classification branch).
---
Nitpick comments:
In `@background.go`:
- Around line 297-298: refreshCustomFieldsCache(app.Client) is being called
inside the per-document classification loop causing a network fetch for every
OCR'd document; move the refresh out of that loop so it runs once per batch or
only when custom-field generation is enabled. Update the code around the
classification loop to call refreshCustomFieldsCache(app.Client) once before
iterating documents (or wrap the call with the feature flag/generation check
used for custom-field creation) so you avoid per-document network round-trips.
…ngsMutex Matches the pattern in app_llm.go and the fix in PR icereed#928. Prevents a data race when settings are updated at runtime. See icereed#927 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds
AUTO_OCR_THEN_CLASSIFY=trueto feed vision-model OCR output directly into classification in a single pass.Depends on: #925
Problem
The OCR and classification pipelines are completely decoupled. Vision-model OCR produces better text than what paperless-ngx stores, but classification never sees it — it runs in a separate cycle using whatever text the document already has. There's no way to classify against the vision-model output without waiting for a second polling cycle and configuring both tags independently.
Related: #165, #449, #497, #882
Solution
When enabled, the OCR path hands its extracted text directly to
classifyDocument()before saving — so the LLM classifies against the vision-model output, not the document's existing text. Everything is applied in a singleUpdateDocumentscall.If classification fails, OCR content is still saved. The user can retry via
AUTO_TAG.User modes (defaults unchanged)
AUTO_TAGAUTO_OCR_TAGAUTO_OCR_TAG+AUTO_OCR_THEN_CLASSIFY=trueChanges
classifyDocument()fromprocessAutoTagDocuments— shared by both auto-tag and chaining paths (pure refactor)processAutoOcrTagDocuments— passes vision-model text to classification when enabledAUTO_OCR_THEN_CLASSIFY(defaultfalse) — opt-in, no impact on existing usersTest plan
TestClassifyDocument_Exists— extracted method worksTestProcessAutoOcrTagDocuments_ChainingDisabled— OCR-only, no classification fieldsTestProcessAutoOcrTagDocuments_ChainingEnabled_ClassifySucceeds— OCR + classification merged, both tags removedTestProcessAutoOcrTagDocumentssubtests still passSummary by CodeRabbit
New Features
Quality & Observability
Tests