fix(scan): address 6 review minors from PR #993 (bookshelf-h7yv) #1066

Merged

zombor merged 1 commit from bd-bookshelf-h7yv into main 2026-07-09 13:18:32 +00:00

Conversation 1 Commits 1 Files changed 6 +70 -11

zombor commented 2026-07-09 05:58:38 +00:00

Owner

Copy link

Summary

Addresses the 6 actionable review minors deferred at PR #993 merge (2 security, 1 security-logging, 1 doc, 1 test-hygiene, 1 error-classification). CSS and JS test-rename minors were already on main.

• Security (handler:178): When BookID==0 (race window ~13ms after 202), return {"status":"pending"} rather than skipping the ownership check and exposing {"status":"running"} to a user whose ownership hasn't been verified yet.
• Security (handler:84): Use r.ContentLength > 0 instead of != 0; an empty chunked body (ContentLength==-1) no longer hits json EOF → 400.
• Security-logging (llm_scan_workflow.go): Log partial.Raw at Debug instead of Info to avoid storing potentially copyrighted OCR-extracted book text in Seq on every extraction failure.
• Doc (pdf_page_images.go): Fix stale comment that said maxRenderPages (8); the const is 5.
• Test hygiene (engine_integration_test.go): Fold two separate Expect calls in the LLMScanWorkflow integration It into one Expect(detail.Result, detailErr).
• Error classification (build_extended_deps.go): Propagate transient DB errors from getProviderCfg as real errors instead of mapping them to ErrLLMDisabled (permanent sentinel); ExtractMetadata itself returns ErrLLMDisabled when config is unconfigured.

Test plan

• New tests for BookID==0 → status:pending and ContentLength==-1 → 202
• make test green (3835 specs)
• make coverage green (100%)
• golangci-lint clean on changed packages

Closes bead bookshelf-h7yv on merge.

## Summary Addresses the 6 actionable review minors deferred at PR #993 merge (2 security, 1 security-logging, 1 doc, 1 test-hygiene, 1 error-classification). CSS and JS test-rename minors were already on main. - **Security (handler:178):** When `BookID==0` (race window ~13ms after 202), return `{"status":"pending"}` rather than skipping the ownership check and exposing `{"status":"running"}` to a user whose ownership hasn't been verified yet. - **Security (handler:84):** Use `r.ContentLength > 0` instead of `!= 0`; an empty chunked body (`ContentLength==-1`) no longer hits json EOF → 400. - **Security-logging (llm_scan_workflow.go):** Log `partial.Raw` at `Debug` instead of `Info` to avoid storing potentially copyrighted OCR-extracted book text in Seq on every extraction failure. - **Doc (pdf_page_images.go):** Fix stale comment that said `maxRenderPages (8)`; the const is `5`. - **Test hygiene (engine_integration_test.go):** Fold two separate `Expect` calls in the LLMScanWorkflow integration `It` into one `Expect(detail.Result, detailErr)`. - **Error classification (build_extended_deps.go):** Propagate transient DB errors from `getProviderCfg` as real errors instead of mapping them to `ErrLLMDisabled` (permanent sentinel); `ExtractMetadata` itself returns `ErrLLMDisabled` when config is unconfigured. ## Test plan - [x] New tests for `BookID==0` → `status:pending` and `ContentLength==-1` → 202 - [x] `make test` green (3835 specs) - [x] `make coverage` green (100%) - [x] `golangci-lint` clean on changed packages Closes bead bookshelf-h7yv on merge.

zombor added 1 commit 2026-07-09 05:58:38 +00:00

fix(scan): address 6 review minors from PR #993 ... ecce5bc9b0

Security minors (metadata_llm_scan_handler.go):
- Return {status:"pending"} when BookID==0 instead of skipping the
  ownership check and exposing {status:"running"} during the ~13ms race
  window after the 202 response.
- Use r.ContentLength > 0 instead of != 0 so empty chunked bodies
  (ContentLength==-1) are treated as absent rather than triggering a
  json EOF 400.

Security minor (llm_scan_workflow.go):
- Log partial.Raw at Debug instead of Info on extraction errors to avoid
  storing potentially copyrighted book text in Seq on every failure
  (log-retention PII concern).

Code review minors:
- pdf_page_images.go: fix stale doc comment that said maxRenderPages (8);
  const is 5.
- engine_integration_test.go: fold two separate Expect calls in the
  LLMScanWorkflow registration It into one Expect(result, err).
- build_extended_deps.go: propagate transient DB errors from getProviderCfg
  instead of masking them as ErrLLMDisabled (permanent); ExtractMetadata
  itself returns ErrLLMDisabled when the resolved config is unconfigured.

Tests added for the two handler behavior changes (BookID==0 path and
ContentLength=-1 path).

CSS minor (min-height token) and JS test rename were already on main.

Closes bead bookshelf-h7yv on merge.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

zombor commented 2026-07-09 06:10:02 +00:00

Author

Owner

Copy link

CODE REVIEW: APPROVED

Reviewed diff origin/main...origin/bd-bookshelf-h7yv. CI is the source of behavioral truth; tests were not re-run locally.

---

Finding #1 — metadata_llm_scan_handler.go:180 — BookID==0 returns {status:"pending"}

Logic split is correct: the old if s.BookID != 0 && s.BookID != id let BookID==0 fall through to the running-status path without ownership validation. The new pair:

if s.BookID == 0 { return json.NewEncoder(w).Encode({Status:"pending"}) }
if s.BookID != id { return ErrNotFound }

correctly short-circuits before any ownership assertion. The 200+{"status":"pending"} response is consistent with the other status values (all 200). The user already passed the book-access check (checkBookAccess) before reaching this code, which limits exposure of the pending state to users with at least one valid book access.

Test: inner BeforeEach overrides getLLMScanResult → BookID: 0; outer JustBeforeEach executes the handler; Expect(result.Status, err).To(Equal("pending")) folds the nil-error and value checks per project convention. Black-box (package books_test), one Expect per It. CORRECT.

---

Finding #2 — metadata_llm_scan_handler.go:84 — ContentLength > 0 guard

!= 0 → > 0 correctly prevents the json.EOF 400 on empty chunked bodies (ContentLength==-1, empty pipe). The original review offered > 0 as an explicit option, and the implementer chose it. Note: a chunked request with ContentLength==-1 that carries actual JSON would also skip decode and silently use the default provider. In practice HTTP clients sending a JSON body set Content-Length explicitly, making this case theoretical. No security impact; behaviour degrades gracefully to default-provider rather than an error. CORRECT for the stated intent.

Test: pipe body forces ContentLength==-1; writer closed immediately (empty); expects 202. //nolint:errcheck on pw.Close() in test is acceptable (test context). Black-box, one Expect per It. CORRECT.

---

Finding #3 — llm_scan_workflow.go:185 — wrapExtractErr log level Info → Debug

partial.Raw (the model's raw OCR/text output) may contain copyright title-page text. Downgrading to Debug avoids retaining potentially copyrighted book content in Seq on every extraction failure. Comment in the diff is accurate. No correctness impact. CORRECT.

---

Finding #4 — pdf_page_images.go:67 — doc comment (8) → (5)

Actual constant: const maxRenderPages = 5 (line 23). Comment was stale at (8). Now matches the code. CORRECT.

---

Finding #5 — engine_integration_test.go — fold two Expects

Expect(detail.Result, detailErr).NotTo(ContainSubstring("activity not found"))

Gomega multi-actual form: asserts detailErr == nil AND detail.Result does not contain the substring. Follows project convention ("fold the no-error check into the value assertion"). CORRECT.

---

Finding #6 — build_extended_deps.go:1510 — propagate real DB error

Old: return metalllm.ExtractResult{}, metalllm.ErrLLMDisabled — misclassified a transient DB error as a permanent sentinel, causing the activity to fail immediately with no-retry.

New: return metalllm.ExtractResult{}, fmt.Errorf("read llm provider config: %w", err) — the real error propagates. isPermanentLLMErr does not match it (it checks errors.Is(err, llm.ErrLLMDisabled) etc.), so the activity is correctly treated as transient and retried. The error message ("read llm provider config: connection refused") reaches ErrMsg in the status response only if all retry attempts exhaust, and is exposed only to the authenticated job owner — acceptable in a self-hosted context. CORRECT.

---

Skipped "already fixed on main" items: The bead completion comment states CSS and JS rename were already on main. No items from the bead's 6-point description are missing from the diff — all 6 are present and addressed.

---

REVIEW VERDICT: 0 blocker, 0 major, 0 minor

CODE REVIEW: APPROVED Reviewed diff `origin/main...origin/bd-bookshelf-h7yv`. CI is the source of behavioral truth; tests were not re-run locally. --- **Finding #1 — `metadata_llm_scan_handler.go:180` — BookID==0 returns `{status:"pending"}`** Logic split is correct: the old `if s.BookID != 0 && s.BookID != id` let BookID==0 fall through to the running-status path without ownership validation. The new pair: ```go if s.BookID == 0 { return json.NewEncoder(w).Encode({Status:"pending"}) } if s.BookID != id { return ErrNotFound } ``` correctly short-circuits before any ownership assertion. The 200+`{"status":"pending"}` response is consistent with the other status values (all 200). The user already passed the book-access check (`checkBookAccess`) before reaching this code, which limits exposure of the pending state to users with at least one valid book access. Test: inner `BeforeEach` overrides `getLLMScanResult` → `BookID: 0`; outer `JustBeforeEach` executes the handler; `Expect(result.Status, err).To(Equal("pending"))` folds the nil-error and value checks per project convention. Black-box (`package books_test`), one Expect per It. CORRECT. --- **Finding #2 — `metadata_llm_scan_handler.go:84` — `ContentLength > 0` guard** `!= 0` → `> 0` correctly prevents the `json.EOF` 400 on empty chunked bodies (ContentLength==-1, empty pipe). The original review offered `> 0` as an explicit option, and the implementer chose it. Note: a chunked request with ContentLength==-1 that carries actual JSON would also skip decode and silently use the default provider. In practice HTTP clients sending a JSON body set Content-Length explicitly, making this case theoretical. No security impact; behaviour degrades gracefully to default-provider rather than an error. CORRECT for the stated intent. Test: pipe body forces ContentLength==-1; writer closed immediately (empty); expects 202. `//nolint:errcheck` on `pw.Close()` in test is acceptable (test context). Black-box, one Expect per It. CORRECT. --- **Finding #3 — `llm_scan_workflow.go:185` — `wrapExtractErr` log level Info → Debug** `partial.Raw` (the model's raw OCR/text output) may contain copyright title-page text. Downgrading to Debug avoids retaining potentially copyrighted book content in Seq on every extraction failure. Comment in the diff is accurate. No correctness impact. CORRECT. --- **Finding #4 — `pdf_page_images.go:67` — doc comment `(8)` → `(5)`** Actual constant: `const maxRenderPages = 5` (line 23). Comment was stale at `(8)`. Now matches the code. CORRECT. --- **Finding #5 — `engine_integration_test.go` — fold two Expects** ```go Expect(detail.Result, detailErr).NotTo(ContainSubstring("activity not found")) ``` Gomega multi-actual form: asserts `detailErr == nil` AND `detail.Result` does not contain the substring. Follows project convention ("fold the no-error check into the value assertion"). CORRECT. --- **Finding #6 — `build_extended_deps.go:1510` — propagate real DB error** Old: `return metalllm.ExtractResult{}, metalllm.ErrLLMDisabled` — misclassified a transient DB error as a permanent sentinel, causing the activity to fail immediately with no-retry. New: `return metalllm.ExtractResult{}, fmt.Errorf("read llm provider config: %w", err)` — the real error propagates. `isPermanentLLMErr` does not match it (it checks `errors.Is(err, llm.ErrLLMDisabled)` etc.), so the activity is correctly treated as transient and retried. The error message (`"read llm provider config: connection refused"`) reaches `ErrMsg` in the status response only if all retry attempts exhaust, and is exposed only to the authenticated job owner — acceptable in a self-hosted context. CORRECT. --- **Skipped "already fixed on main" items:** The bead completion comment states CSS and JS rename were already on `main`. No items from the bead's 6-point description are missing from the diff — all 6 are present and addressed. --- REVIEW VERDICT: 0 blocker, 0 major, 0 minor

zombor force-pushed bd-bookshelf-h7yv from ecce5bc9b0 to 2c6ee60c5b 2026-07-09 13:12:20 +00:00 Compare

zombor merged commit 50d0a43dc9 into main 2026-07-09 13:18:32 +00:00

zombor referenced this pull request from a commit 2026-07-09 13:18:33 +00:00

Merge pull request 'fix(scan): address 6 review minors from PR #993 (bookshelf-h7yv)' (#1066) from bd-bookshelf-h7yv into main

Sign in to join this conversation.

Reviewers

No reviewers

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

1 participant

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

zombor/pergamum!1066

No description provided.