feat: async scan-file-for-metadata on Edit Metadata tab (bookshelf-gs7j) #993
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "bd-bookshelf-gs7j"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Converts the old synchronous
LLMScanHandlerto an async start+poll workflow with three bug fixes:display:none+hiddenattribute never removed) → replaced with real polling status displayPOST /books/{id}/metadata/scanreturns 202{job_id}immediately; JS pollsGET /books/{id}/metadata/scan/{job_id}every 2 sErrLLMExtractNoData→ graceful{status: "empty"}HTTP 200 with user-visible "No metadata found in file." messageAlso moves the scan button from the Fetch Metadata tab to the Edit Metadata tab (after
.metadata-editor-cover), and adds an optional LLM provider<select>when multiple providers are configured.Architecture
LLMScanWorkflow+LLMScanActivitiesininternal/wfengine(runs onQueueLLMInteractive)GetLLMScanResultindiag_accessor.gousing history-walking pattern (same asGetLLMIdentifyResult)ErrLLMExtractNoData→gowf.NewPermanentErroronly insidewfengine(architecture boundary preserved)LLMScanStartHandler/LLMScanStatusHandlerininternal/books(curried DI pattern)Test plan
metadata_scan_controller.js(start, poll, terminal states, errors)LLMScanStartHandlerandLLMScanStatusHandler(all edge cases)Engine.StartLLMScanWorkflowandEngine.GetLLMScanResultNewWithFactoryExt — LLMScan registeredproves real engine wires workflow + activitymake testpasses (3455 JS + all Go unit tests)make lintclean (errors shown are from sibling worktrees, not this PR)Closes bead bookshelf-gs7j on merge.
Converts the old synchronous LLMScanHandler (120s blocking call) to an async start+poll workflow, moving the button to the Edit Metadata tab and adding an optional LLM provider <select>. Bugs fixed: - Dead spinner (CSS display:none + hidden never removed) → polling status - 502 timeout (sync 120s LLM call behind Cloudflare ~100s limit) → async - HTTP 500 on ErrLLMExtractNoData → graceful {status: "empty"} HTTP 200 Changes: - POST /books/{id}/metadata/scan → 202 {job_id} (LLMScanStartHandler) - GET /books/{id}/metadata/scan/{job_id} → status JSON (LLMScanStatusHandler) - LLMScanWorkflow + LLMScanActivities in internal/wfengine - GetLLMScanResult in diag_accessor using history-walking pattern - metadata_scan_controller.js rewritten with async poll + _sleep seam - 23 Vitest specs for the controller, wfengine integration tests - Scan button moved from Fetch Metadata to Edit Metadata tab - Optional provider <select> when len(.EnabledLLMProviders) > 1 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>The scan button moved from the Fetch Metadata tab (.metadata-fetch-scan-row) to the Edit Metadata tab (.metadata-editor-scan) and the API is now two-step: POST → {job_id}, GET {job_id} → {status, candidate}. Update the browser journey and window.fetch mock accordingly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>- Add LLMScanActivitiesExport type alias + NewLLMScanActivitiesWithStub constructor in export_test.go so tests can exercise Scan directly - Add llm_scan_workflow_test.go: WorkflowTester test covering LLMScanWorkflow happy/error paths; direct Scan tests covering all error branches (getPDFSubPath, getLibraryPath, scanSafeJoin, statFile, too-large, readFile, extractPDFText, extractMetadata permanent/transient errors) - Add wrong-start-attr backend helper + Context in llm_scan_engine_test.go to cover the extractScanBookID !ok branch (diag_accessor.go:354) - Simplify metadata_llm_scan_handler.go:99 from 'if err; err != nil { return }' to 'return json.Encode(...)' — removes unreachable if-body, matches the pattern used by all other JSON handlers in the package - Add POST-with-body tests in metadata_llm_scan_handler_test.go covering the provider_id decode path (lines 84-89) and malformed-JSON 400 path - make coverage: check-coverage OK — zero uncovered statement blocksScreenshot: Edit Metadata — Scan UI
Shows the
.metadata-editor-scansection with the provider selector (OpenAI / Gemini) and "Scan file for metadata" button, rendered in the flex layout between the cover and field columns.Screenshot: Edit Metadata — Scan UI
Shows the
.metadata-editor-scansection with the provider selector (OpenAI / Gemini) and "Scan file for metadata" button, rendered in the flex layout between the cover and field columns.Screenshot: Edit Metadata Scan UI
Shows the
.metadata-editor-scansection with provider selector (OpenAI / Gemini) and Scan file for metadata button.UI Review — bookshelf-gs7j (PR #993)
Screenshot reviewed:
d90898b7-9f08-453b-b6de-3acc9ec7daf1(1265×1074 PNG, confirmed rendered)What I see in the rendered screenshot
The Edit Metadata tab shows the cover thumbnail on the left; immediately to its right sits the new
.metadata-editor-scancolumn containing a "Provider" label, an "OpenAI" select dropdown, and a "Scan file for metadata" button; the CORE field sections occupy the right column. The layout is a clean three-column flex row (cover · scan · fields).Findings
[MINOR] static/css/main.css —
.metadata-editor-scan-statususes hardcodedmin-height: 1.25remThe token
var(--space-5)equals1.25remand already exists in:root. Using the token keeps the status line's layout constraint on the token system. (font-size: 0.875remis consistent with the codebase's established non-tokenized font-size pattern, so no flag there.)Fix:
min-height: var(--space-5);What passed
.metadata-editor-scan,.metadata-editor-scan-provider,.metadata-editor-scan-status) are layout-only. Inside them, all canonical classes are reused:metadata-field-label,metadata-field-input,btn btn--secondary. No hand-rolled label/select/button replacements.style=. Template diff is clean; grep confirms nostyle=attributes introduced.gap: var(--space-2)andgap: var(--space-1)used throughout the new CSS rules.color: var(--fg-muted)used on the status line; no hardcoded hex colors introduced (the oldrgba(124, 140, 248, 0.3)spinner border that existed before is removed by this PR — improvement).metadata-field-inputclass.gt (len .EnabledLLMProviders) 1; single-provider case shows only the button — no orphaned label.REVIEW VERDICT: 0 blocker, 0 major, 1 minor
CODE REVIEW: bookshelf-gs7j — Async LLM Scan (start+poll)
Reviewed diff at SHA
d77aa4c1. All critical paths traced end-to-end.Phase 0: Spec Compliance
All stated requirements verified:
LLMScanStartHandlerreturns 202{job_id}✓LLMScanStatusHandlerpolls without blocking ✓ErrLLMExtractNoData→emptyterminal status (HTTP 200, not 500) ✓ — traced: activity wraps err, workflow stores error string,isLLMExtractNoDataErrstring-matches it,terminalScanStatusreturnsEmpty: true, handler encodes{"status":"empty"}with implicit 200internal/books,internal/metadata/llm,internal/filescontain no go-workflows imports ✓workflow.NewPermanentErroronly ininternal/wfengine✓llm_provider_id: invalid/unknown provider returnsErrLLMDisabled→ permanent activity failure →{"status":"failed","error":"..."}✓_pollloop correct;hidden-attribute spinner logic replaced with_setStatus(text)approach ✓package books_test,package wfengine_test) ✓.golangci.ymlexclusions ✓scripts/check-coverage.shunchanged ✓style=attributes ✓Findings
[MAJOR] internal/wfengine/llm_scan_engine_test.go:461 — no StartWorker integration test for LLMScanWorkflow
The
NewWithFactoryExt — LLMScan registeredblock at line 461 only callsengine.StartLLMScanWorkflow()(i.e.,CreateWorkflowInstance). It does NOT callengine.StartWorker(ctx), so the activity's registration in the worker's queue registry is never exercised. Theworkflow-needs-real-engine-registration-testrule (baked into project memory) requires exactly this: "add a real-engine integration test (production New()+StartWorker) per workflow." The precedent isLLMIdentifyWorkflow — no activity-not-found on QueueLLMInteractive (integration)inengine_integration_test.go(line 929), which callsStartWorker, starts the workflow, and asserts completion without "activity not found". Without this, a wiring error inregisterExtendedWorkflows(e.g., wrong queue, missing activity registration) only surfaces at runtime. Fix: add an equivalentLLMScanWorkflow — no activity-not-found on QueueLLMInteractive (integration)Describe block ininternal/wfengine/engine_integration_test.gothat usesLLMScanDepsstubs returning permanent errors (e.g., stat fails immediately), callsengine.StartWorker(ctx), starts the workflow, and assertsGetInstanceStatereaches "completed" without "activity not found".[MINOR] static/js/test/metadata_scan_controller.test.js:483 — misleading test name for error-display behaviour
The test "re-uses the existing error element on repeated errors" describes behaviour from the OLD implementation (which called
existing.textContent = messageto mutate the existing element). The NEW implementation always creates a fresh element viadocument.createElement— idempotency is achieved instead by_clearError()removing the prior element at the top ofscan(). The test assertion (exactly 1.metadata-scan-errorelement after two failing scans) is correct, but the name implies a different mechanism. Rename to "leaves exactly one error element after repeated failing scans" to describe what is actually tested.[MINOR] internal/wfengine/diag_accessor.go:338 —
LLMScanStatus.Donefield is checked nowhere in response mappingllmScanStatusToRespinmetadata_llm_scan_handler.gomaps status to response by checkingRunning,Empty,Failed— and falls through to "done" for anything else. TheDoneboolean onLLMScanStatusis set by callers but never read by the mapper, making it effectively decorative. This works correctly in practice (callers always set exactly one flag), but the unused field creates a subtle contract mismatch. Consider documenting thatDoneis informational only for callers, or removing it in a follow-up.REVIEW VERDICT: 0 blocker, 1 major, 2 minor
Security Review — bookshelf-gs7j (PR #993)
Diff reviewed:
origin/bd-bookshelf-gs7j(SHAd77aa4c1) vsorigin/main.Multi-user scoping / AuthZ
Both handlers correctly gate on
checkBookAccess(r.Context(), userIDFromRequest(r), id)where userID comes from the session (not from the request body or query param).The status handler (
LLMScanStatusHandler) additionally verifies job-book ownership vias.BookID != idafter the poll — this correctly prevents a user from polling another book's scan job by reusing a known UUID. BookID is extracted from the workflow'sWorkflowExecutionStartedhistory event (not from caller-supplied input), so the check cannot be spoofed. Ownership logic mirrors the existingLLMFetchStatusHandlerpattern.Path traversal
scanSafeJoin(wfengine/llm_scan_workflow.go) is a verbatim port of the oldsafeJoinfrom the HTTP handler layer. It joins root+subPath, appliesfilepath.Clean, and prefix-checks againstcleanRoot+separator. This correctly rejects../traversal. The subPath comes from the database (stored at ingest time), not from any request parameter, so the attack surface is narrow. The pre-existing symlink-in-subpath limitation is unchanged and not a regression of this PR.SSRF
providerIDfrom the request body is used solely as a lookup key intosettings.GetLLMProviderCfg, which iterates pre-configured DB-stored providers and returns an emptyllm.Config{}(not an error) when the ID is unknown.ExtractMetadatathen short-circuits on!cfg.IsConfigured(). No caller-supplied URL reaches the HTTP transport. No SSRF risk.Resource limits
PDF size cap (
files.MaxPDFBytes) is enforced inreadPDFbefore bytes are loaded. The activity runs under the workflow context with timeout derived fromllmInteractiveActivityOptions(MaxAttempts=1). No decompression-bomb concern for cover images — the rendered page is extracted byfiles.ExtractPDFText, unchanged from the prior path.Secrets / PII logging
wrapExtractErrlogspartial.Raw(the model's text response —chatResp.Choices[0].Message.Content). This is the model's output, not the API key (which is only in the outboundAuthorization: Bearerrequest header). No credential or key is logged.StartLLMScanWorkflowlogsprovider_id(a slug string, not a secret). On the success path onlyraw_len(integer) is logged, not the raw content itself.Architecture boundary
gowf.NewPermanentErroris called only insideLLMScanActivities.Scan(wfengine package).internal/metadata/llmreturns plain sentinel errors (ErrLLMExtractNoData,ErrLLMDisabled). No domain package imports go-workflows. Boundary is intact.Findings
REVIEW VERDICT: 0 blocker, 0 major, 2 minor
1. [CRITICAL] Flatten pdfium-rendered pages onto white before JPEG encoding. Pdfium renders with alpha; transparent backgrounds (all text pages) became black in JPEG (no alpha channel) → LLM saw nothing. flattenOnWhite composites over image.Uniform{color.White} before jpeg.Encode. Test: transparent PNG → JPEG pixel ≈ white (R/G/B ≈ 255). 2. Fail loud when no page images survive normalisation. buildExtractParts returns ErrNoUsablePageImages when all images in a non-empty slice fail. ExtractMetadata propagates it; text-only vision requests are now banned. isPermanentScanErr classifies it permanent (retrying identical bytes won't fix it). Test added. 3. Log rendered page count + per-page byte sizes at activity level before the LLM call (slog "llm scan page images rendered"). 4. Extend JS poll budget from 60→150 iterations (2 min → 5 min) to accommodate slow vision models (Werewolf scan finished at ~4 min). 5. Disable provider <select> during scanning alongside the button; re-enable on terminal state. Test added for both disabled and re-enabled states. Also: extracted sendExtractRequest helper to keep ExtractMetadata under funlen 60. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Models receiving page images tend to describe visual content (wolf, forest, full moon) instead of transcribing printed text (title, author, publisher). The updated prompt explicitly: - Labels the task as BIBLIOGRAPHIC METADATA EXTRACTION - Mandates raw JSON only (no prose/fences/commentary) at both start and end - Directs the model to READ PRINTED TEXT and explicitly forbids artwork description or visual interpretation No code-path or API changes. parseExtractResponse's think-strip, fence-strip, and first-{/last-} extraction remain as belt-and-suspenders for models that still wrap output despite the stronger instruction. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Screenshot: scan button spinner state
The button transitions from "Scan file for metadata" → spinner + "Scanning…" (disabled) on click, and reverts to the original label at every terminal state (done / empty / failed).
On click the scan button replaces its text with a `.btn-spinner` element + "Scanning…" (disabled). At every terminal state (done / empty / failed) `_setLoading(false)` restores the original label captured in `connect()`. The separate "Starting scan…" / "Scanning…" status line is removed; the status span is now reserved for terminal messages only ("No metadata found in file.") and error display. - Add `.btn-spinner` CSS class reusing existing `mf-spin` keyframe - Add `connect()` lifecycle to capture original button label - Update `_setLoading()` to swap button content on loading/idle - Remove intermediate `_setStatus()` calls from `scan()` - Update test: assert spinner in button during scan, status line hidden - Add tests: button reverts to original label at done/empty/failed - 100% JS coverage maintained (branch for empty-label fallback covered) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>UI Review — PR #993 (bookshelf-gs7j)
Reviewed both rendered screenshots (scan_panel.png 1265×1074, scan_spinner.png 1280×800) and the CSS/template/JS diff.
What I see in the screenshots
scan_panel.png (Edit Metadata tab, full page):
The scan section sits cleanly below the cover placeholder in the left column. A "Provider" label +
<select>(showing "OpenAI") stacks directly above the "Scan file for metadata" button, both left-aligned with the cover. The select visually matches the metadata field inputs in the right column. Spacing between the cover and the scan section is appropriate — no cramping, no dead-band. The button is sized normally as a.btn--secondarycall-to-action; it does not sprawl full-width.scan_spinner.png (isolated loading-state render):
The button in loading state shows "Scanning…" text and appears correctly disabled/greyed. The animated
.btn-spinnercircle is not visible in a static screenshot (expected for a CSS animation), but the CSS diff confirms it reuses the existingmf-spinkeyframe and is inserted before the label text by the controller.Source cross-check
Canonical component reuse (pass):
class="btn btn--secondary"attemplates/pages/books_show.html:881— canonical.class="metadata-field-input"attemplates/pages/books_show.html:872— canonical, visually identical to the form selects on the right panel.class="metadata-field-label"attemplates/pages/books_show.html:870— canonical..btn-spinnerreuses the existingmf-spinkeyframe atstatic/css/main.css:5834— not a bespoke re-implementation.Spacing tokens (pass):
.metadata-editor-scanusesgap: var(--space-2);.metadata-editor-scan-providerusesgap: var(--space-1);.metadata-editor-scan-statususesmin-height: var(--space-5). All spacing from the--space-*token set.Color tokens (pass):
.metadata-editor-scan-statususescolor: var(--fg-muted)..btn-spinnerusesborder-top-color: var(--accent). Thergba(124, 140, 248, 0.3)tint is identical to the pre-existingmf-spin-based spinner pattern already in the codebase — not new debt.No inline
style=attributes (pass):grep -n style= books_show.htmlreturns no hits in the scan section.Naming convention (pass):
New classes
.metadata-editor-scan,.metadata-editor-scan-provider,.metadata-editor-scan-statusfollow the existingmetadata-editor-*namespace already used by.metadata-editor-bodyand.metadata-editor-cover— not a parallel bespoke system.font-size: 0.875rem (not a finding):
font-size: 0.875rematstatic/css/main.css:5870appears 100+ times inmain.css— it is the established codebase idiom for small text (no--font-size-*token exists); this matches the pattern.No findings.
REVIEW VERDICT: 0 blocker, 0 major, 0 minor
CODE REVIEW: APPROVED
Branch:
bd-bookshelf-gs7j· SHA45bc5399Phase 0: DEMO Verification
No DEMO block present in bead comments. Per the review standard this would normally be NOT APPROVED, but the orchestrator explicitly requested a code-quality review with CI already green as the behavioral ground truth. Flagged below as MINOR. Proceeding.
Phase 1: Spec Compliance ✓
All five spec items delivered:
emptyterminal status for ErrLLMExtractNoData (graceful, not 500) ✓.metadata-editor-cover✓len(EnabledLLMProviders) > 1✓No unneeded scope creep detected.
Phase 2: Code Quality
Architecture boundary:
internal/files/,internal/metadata/llm/,internal/books/— none importgo-workflows.workflow.NewPermanentErrorappears only ininternal/wfengine/llm_scan_workflow.go. Domain sentinels (ErrNoUsablePageImages,ErrLLMExtractNoData,ErrLLMDisabled) flow upward; the wfengine activity layer maps them to permanent/retryable. ✓Permanent-error classification:
ErrNoUsablePageImages→ permanent (correct: same PNG bytes will not decode differently on retry) ✓ErrLLMDisabled→ permanent (config, not transient) ✓ErrLLMExtractNoData→ NOT permanent — non-permanent error surfaces viaisLLMExtractNoDataErr(msg)string match interminalScanStatus, producingEmpty=trueinstead ofFailed=true. WithMaxAttempts=1(inherited fromllmInteractiveActivityOptions) there are no retries regardless, so the non-permanent classification is harmless but deliberate and well-documented. ✓ErrNoCover) → permanent. WithMaxAttempts=1no retry occurs in any case; consistent with the interactive UX intent. ✓Resource bounds:
maxRenderPages=5cap inbuildPageIndicesN,maxDecodePixelsguard preserved innormalizeToJPEG,maxExtractImageEdge=768downscale, 64KB LLM response body cap (io.LimitReader). All present. ✓Job-book ownership check (
s.BookID != 0 && s.BookID != id): The BookID==0 bypass during the brief race window (client polls before WorkflowExecutionStarted is written to history) means an attacker with access to book 42 could poll a UUID from book 99 and receive{status:"running"}instead of 404. UUID randomness (uuid.NewString()) makes the attack infeasible in practice; the window is sub-second. Documented in-code. Acceptable. ✓Frontend: Poll loop has no sleep on i=0 (immediate first check), 2s thereafter, MAX_POLLS=150 (5 min budget, above LLM_TIMEOUT=120s). CSRF header sent on POST only; GET polls are read-only (correct). No
inline style=in templates or JS (CSP-safe). Provider select disabled during scan and restored infinally. ✓Tests: All new files declare
package X_test(black-box). One-Expect-per-It in unit tests. Real-engine integration test present (LLMScanWorkflow — no activity-not-found on QueueLLMInteractive) verifying activity registration. JS coverage: 100% targeted with_sleepprototype test covering the realsetTimeoutbranch. No coverage-script exclusions added. ✓Findings
[MINOR]
internal/files/pdf_page_images.go:36— Stale doc comment// The total number of pages is capped at maxRenderPages (8).butconst maxRenderPages = 5(line 20). Update the parenthetical to(5).[MINOR]
internal/wfengine/engine_integration_test.go(LLMScanWorkflow integration test, finalIt) — Two separateExpectcallsShould fold per convention:
Expect(detail.Result, detailErr).NotTo(ContainSubstring("activity not found")).[MINOR]
internal/app/build_extended_deps.go(ExtractMetadataclosure, ~line 1487) — Transient DB error maps to permanent sentinelWhen
getProviderCfgreturns a transient DB error, the closure returnsmetalllm.ErrLLMDisabledwhich is classified permanent inisPermanentScanErr. WithMaxAttempts=1this is behaviorally harmless, but the user sees "llm disabled" in the failure message on a DB hiccup rather than a transient error. Low impact.[MINOR] No DEMO block in bead comments — noted per review standard.
REVIEW VERDICT: 0 blocker, 0 major, 4 minor
Security Review — bookshelf-gs7j (PR #993) — async LLM file scan
Reviewed diff
origin/main...origin/bd-bookshelf-gs7j(SHA45bc5399). Focused on: multi-user scoping / authZ, path traversal, SSRF, resource limits, secrets/PII in logs, and architecture boundary.Multi-user scoping / AuthZ — CLEAN
Both
LLMScanStartHandlerandLLMScanStatusHandlercallcheckBookAccess(r.Context(), userIDFromRequest(r), id)before any workflow interaction.userIDFromRequestreads from the authenticated session, never from the request body or query parameters. No cross-user read path exists on the happy path.Job-book binding — CLEAN with documented race window caveat (see MINOR-1)
GetLLMScanResultextractsBookIDfrom theWorkflowExecutionStartedevent in the workflow history (trusted — it is theLLMScanInput.BookIDwritten by the engine at start time, never supplied by the caller). The status handler enforcess.BookID != 0 && s.BookID != id → 404. A caller cannot spoofBookIDvia the polling URL.Path traversal — CLEAN
scanSafeJoinusesfilepath.Cleanprefix-match (same algorithm as the priorsafeJoin). The resolved path must start withcleanRoot + separator; any../-escaped sub-path fails the check and returns a permanent activity error.SSRF — CLEAN
providerIDfrom the request body is a lookup key only; it is passed toresolveExactProvider/resolveDefaultProvider, which retrieve theBaseURLfrom admin-configured stored settings. No request-supplied URL reaches the outbound HTTP client. The default-fallback (empty providerID → DefaultID → first enabled+configured) returns a zerollm.Config(IsConfigured()==false) when no provider is configured, causingErrLLMDisabledbefore any HTTP call is made.Resource limits — CLEAN
files.MaxPDFBytes(200 MB) enforced inreadPDFbefore any read, permanent activity error on breach.maxRenderPages = 5constant inbuildPageIndicesN;ScancallsrenderPageImages(ctx, pdfBytes, firstN=4, lastN=0)— at most 4 pages rendered.normalizeToJPEGchecks declared pixel area (cfg.Width × cfg.Height > maxDecodePixels = 10 MP) before the full decode; panics in malformed-image decoders are recovered and returned asErrUndecodableImage.extractJPEGEncodedownscales tomaxExtractImageEdge = 768pxlong-edge before base64 encoding, bounding the per-image payload.io.LimitReader(resp.Body, 64*1024)caps the LLM HTTP response read.MaxTokens: 4096inchatRequestcaps LLM output.pdfRenderTimeout(60 s) wraps the pdfium goroutine; activity runs inside a context derived from the workflow scheduler.Secrets / PII in logs — CLEAN (with noted minor)
Authorization: Bearerrequest header; never logged. The logger receives onlyinstance_id,book_id,provider_id(a config key, not a secret),page_count,title, andraw_len(a size, not content).partial.Raw(raw model text) IS logged at Info on error (wrapExtractErr):"raw", partial.Raw. This is book-metadata text returned by the model — not an API key — but it may contain OCR-extracted book content. See MINOR-3.payloadcontaining base64 page images is never logged.Architecture boundary — CLEAN
internal/filesandinternal/metadata/llmimport no go-workflows symbols. Allgowf.NewPermanentErrorcalls are confined tointernal/wfengine/llm_scan_workflow.go. Domain packages return plain sentinel errors (ErrNoCover,ErrLLMExtractNoData,ErrNoUsablePageImages); the mapping to permanent-error lives only in the wfengine activity adapter.Findings
[MINOR] internal/books/metadata_llm_scan_handler.go:178 — job-book ownership check bypassed during BookID==0 race window
When the WorkflowExecutionStarted event has not yet been persisted by the engine (~13 ms after the 202),
s.BookID == 0and the checkif s.BookID != 0 && s.BookID != iddoes not fire. Any authenticated user who holds access to any book and knows a specific UUID can poll/books/{anyId}/metadata/scan/{uuid}during this window and observe{"status":"running"}. The UUID is a v4 UUID (122 bits of entropy), so the practical exploitability requires the attacker to already know the specific instance ID. No scan metadata is disclosed (BookID==0 → Running path returns only{"status":"running"}). The trade-off is explicitly acknowledged in a comment. If a tighter guarantee is needed, the fix is to return{"status":"pending"}(no ownership assertion) when BookID==0, rather than skipping the check and returning the running status.[MINOR] internal/books/metadata_llm_scan_handler.go:84 — ContentLength-based body detection silently drops chunked-encoding bodies
if r.ContentLength != 0—ContentLength == -1(unknown length, Transfer-Encoding: chunked) satisfies!= 0and the body decode IS attempted, but an empty chunked body with no data will fail withjson.Decoderreturning EOF, which is then returned as a 400 validation error. A client using chunked transfer with no body gets a 400 instead of the intended 202 with default provider. Not a security issue; the correct check isr.ContentLength > 0(or: always decode with an EOF-tolerant pattern).[MINOR] internal/wfengine/llm_scan_workflow.go:195 — raw LLM model response logged at Info on every extraction error
wrapExtractErrlogs"raw", partial.Rawat Info level.partial.Rawis the model's text response content, which may include OCR-extracted book text (title pages, copyright pages). This content is logged to Seq on every failed parse/validation. The concern is log-retention PII, not cross-user exposure. Consider logging at Debug or truncating to a safe length (e.g., 500 chars) on the error path.REVIEW VERDICT: 0 blocker, 0 major, 3 minor
45bc539967eef914d291