diff --git a/docs/reviews/2026-06-22-project-review.md b/docs/reviews/2026-06-22-project-review.md new file mode 100644 index 0000000..4d5d8ab --- /dev/null +++ b/docs/reviews/2026-06-22-project-review.md @@ -0,0 +1,562 @@ +# Copyrighter 프로젝트 코드 리뷰 — 2026-06-22 + +> 다중 에이전트 리뷰(12개 유닛, 도메인 전문 리뷰어) · 모든 CRITICAL/HIGH 발견은 적대적 검증(실제 코드 기반 반증 시도)을 거침. +> 검증 통과 발견 **66건** (적대적 검증으로 반증 제거 3건). +> CRITICAL 0 · HIGH 6 · MEDIUM 43 · LOW 17. +> +> ⚠️ `arch-concurrency`(아키텍처/동시성) 유닛은 일시적 레이트리밋으로 실패했습니다. 해당 영역은 `sec-sql`/`bug-store` 유닛이 부분적으로 커버했으며, 재실행을 권장합니다. + +## 1. 요약 (Executive Summary) + +## Overall Health: Solid foundation, no CRITICAL issues, a handful of real HIGH-severity correctness bugs + +The **copyrighter** project is in good shape post-remediation. Across 66 verified findings (3 refuted and dropped) there are **0 CRITICAL** and **6 HIGH** issues — the remaining 43 MEDIUM / 17 LOW are mostly defence-in-depth, maintainability, and test-coverage gaps rather than live exploits. The prior review's hardening held up: wildcard CORS is gone, auth covers all data routes, the request body cap + SQLite WAL/`busy_timeout` are in place, SSRF is guarded with redirect re-validation, path traversal is blocked, and the Python god-file was split into mixins. + +### What is genuinely solid +- **Security perimeter (HTTP/SSRF/SQL/XSS units all confirm this):** parameterized queries everywhere with a `STORE_TABLES` allowlist, `escapeHtml()` applied consistently to innerHTML data, `safeUrl()` allowlisting external URLs, SSRF guard with per-redirect-hop re-validation, untrusted media served with strict CSP+`nosniff`+`no-store`. No hardcoded secrets, no `eval`/`new Function`, no unsafe deserialization (all payloads are plain JSON). +- **Air-gap posture (bld-airgap):** runtime deps pinned and transitively complete in a single `pip download`; GUI loads only local assets (bundled font, no CDN); no telemetry/auto-update anywhere. +- **Test breadth (tst-coverage):** 390 test functions / 35 files; Playwright E2E for decision-flow + upload; `record_decision` atomicity already pinned with a rollback test. + +### Biggest risks (all confirmed HIGH) +1. **Data-integrity / audit-trail bugs in the store.** `mark_evidence_status` performs a non-atomic read-modify-write-audit across four separate connections with no `_write_lock` and no `_transaction()` — concurrent operator edits silently last-writer-win and the audit log diverges from DB state. `promote_collection_candidates` likewise lacks the write lock, uses a non-deterministic `_timestamp_id()`, and has no transaction/idempotency guard, so concurrent batch-promotes mint **duplicate, non-collapsible knowledge entries**. (Note: `mark_evidence_status` is the single most-reported issue — it surfaced 6 times across units; it is the top fix.) +2. **Silent loss of operator actions in the GUI.** `deactivateCorrectionEntry` and the provider-retry handler both mutate module-level arrays and append a synthetic audit event with **no API call** — the action evaporates on the next `refreshFromApi()`. Operators believe work happened when nothing was persisted. +3. **Risk under-scoring (mis-classification).** `_web_detection_bucket` collapses every named entity into one `"entity"` bucket, so a second distinct entity (e.g. two different celebrities/characters in one image) is silently dropped — suppressing ~40 points and potentially flipping a genuinely **high**-risk image to **medium**, reducing operator scrutiny. +4. **Frontend concurrency:** `rerunHistoricalQuery` fires async `runManualSearch` with no await and no in-flight guard, enabling duplicate/racing search POSTs and interleaved bootstrap refreshes. +5. **Maintainability hotspots (acknowledged debt):** `_auto_naver_search` (234 lines) / `_auto_google_custom_search` (165 lines) are near-identical copy-paste cascades that must be kept in sync; `app.js` is 3,926 lines (5x ceiling) and `styles.css` is 2,819. + +The unifying risk is **audit/state integrity** — multiple independent findings show operator actions that either silently fail to persist or persist without a matching audit record. For a rights-review compliance tool, that is the theme most worth fixing first. + +## 2. 통계 + +| 심각도 | 건수 | +|---|---| +| CRITICAL | 0 | +| HIGH | 6 | +| MEDIUM | 43 | +| LOW | 17 | + +| 분류 | 건수 | +|---|---| +| security | 23 | +| bug | 19 | +| maintainability | 8 | +| error-handling | 8 | +| testing | 4 | +| build | 3 | +| performance | 1 | + +## 3. 교차 테마 + +### Audit-trail & state-persistence integrity + +The single most important cross-cutting risk: multiple independent findings show operator actions that either persist without a matching audit record, or appear to succeed but never reach the server. mark_evidence_status writes evidence+audit+rescore non-atomically across four connections (no _write_lock, no _transaction), so concurrent edits last-writer-win and the audit log diverges. promote_collection_candidates has the same transaction gap plus a non-deterministic ID, producing duplicate knowledge entries. On the frontend, deactivateCorrectionEntry and the provider-retry button mutate local arrays and push synthetic audit events with no API call, so the action silently evaporates on the next refreshFromApi(). For a rights-review compliance tool this is the theme to fix first. + +_관련 발견: mark_evidence_status: non-atomic read-modify-write without _write_lock or _transaction, promote_collection_candidates missing write lock and non-idempotent ID, deactivateCorrectionEntry mutates shared module-level arrays without an API call, provider-retry button writes a local-only synthetic audit entry with no API call, rerun_enrichment and run_auto_search perform unguarded concurrent read-modify-write on evidence and submissions, mark_evidence_status atomicity is not tested_ + +### Unguarded concurrency / fire-and-forget async + +ThreadingHTTPServer means store read-modify-write sequences need _write_lock + _transaction, and the GUI needs in-flight guards on async actions. Several store methods skip the lock; several frontend handlers fire async calls without await, void, or a disabled-button guard, enabling duplicate/racing POSTs and interleaved bootstrap refreshes. + +_관련 발견: rerunHistoricalQuery calls async runManualSearch without await and without an in-flight guard, No in-flight guard on setDecision, rerun_enrichment and run_auto_search perform unguarded concurrent read-modify-write on evidence and submissions, promote_collection_candidates missing write lock and non-idempotent ID, mark_evidence_status: non-atomic read-modify-write without _write_lock or _transaction_ + +### Risk-scoring correctness (under-escalation) + +Bugs in RiskScorer cause genuinely high-risk images to score lower than they should or to show misleading justifications. The entity-bucket collapse drops all but the first named entity; the category/entity ordering asymmetry makes scoring depend on iteration order; and zero-point skip reasons pollute the operator-facing reasons list. All reduce the fidelity of the signal operators rely on to escalate. + +_관련 발견: Multiple distinct web-detection entity signals collapse into one scoring bucket, Entity and category-matched WEB_DETECTION items share the same dedup bucket causing priority-dependent under-scoring, SEARCH_SKIPPED and EXTERNAL_SKIPPED reasons pollute ScoreResult.reasons despite contributing zero points, ScoreResult.reasons is a mutable list inside a frozen dataclass, allowing inadvertent mutation_ + +### Silent error swallowing without logging + +A recurring pattern of bare except blocks and hardcoded fallbacks that hide infrastructure failures (SSRF rejections, network/disk errors, malformed Ollama responses, corrupt DB rows) at the same level as expected skips, making real failures invisible in logs and, for one case, silently leaking EXIF/GPS to external APIs. + +_관련 발견: _sync_face_crops swallows image load failure without any log or audit trail, _store_candidate_image silently discards network and fingerprint errors with no log, _response_text falls back to 'Evidence summary unavailable.' without logging or raising, json.loads() on DB rows with no error handling, _search_result_page_image_candidates and _search_result_stylesheet_image_urls swallow all page-fetch exceptions silently, Silent EXIF/GPS metadata leakage when PIL re-encoding fails in build_external_derivative, Provider lastFailure attribution reads evidence[0] rather than the first failed item, Unhandled exception types in request handlers log stack traces to stderr_ + +### Defence-in-depth security headers, cookie & XSS hardening + +No live exploit on the air-gapped localhost deployment, but the security posture relies too heavily on single layers (SameSite cookie, DB CHECK constraints) with no browser-level backstop. The app shell ships no CSP/X-Frame-Options, the session cookie is the raw master token, several innerHTML/src/href sinks skip escapeHtml/safeUrl, and there is no auth rate-limiting or token-entropy floor. Cheap to fix; important if the deployment ever sits behind a proxy. + +_관련 발견: No security headers on main application responses (missing CSP, X-Frame-Options, X-Content-Type-Options), No Content-Security-Policy header on the app shell or API responses, Cookie stores raw auth token, missing Secure flag, No minimum-entropy check on COPYRIGHTER_AUTH_TOKEN, No rate limiting on authentication or any API endpoint, CSRF protection relies solely on SameSite=Strict with no Origin validation fallback, innerHTML templates inject server-derived label lookups without escapeHtml, riskClass() injects unvalidated server string into class attribute without escaping, submission.asset assigned directly to img.src without safeUrl, similar-strip and face-crop-strip image URLs not passed through safeUrl, faceCrops href and img src use escapeHtml but not safeUrl, fetch() calls use no explicit credentials mode_ + +### Outbound HTTP / SSRF consistency + +The candidate-image/page fetch path is well-guarded (SSRF check re-validated per redirect, size-capped), but the JSON API transport diverges: no response-size cap, no redirect SSRF re-validation, and Google API keys embedded in URLs that would appear in tracebacks. Aligning UrllibJsonTransport with store_remote_fetch's hardened opener closes the gap. + +_관련 발견: UrllibJsonTransport reads API responses without a size limit, UrllibJsonTransport follows HTTP redirects without SSRF re-validation, Google and Custom Search API keys embedded in outbound request URLs expose them in Python tracebacks_ + +### Maintainability hotspots & copy-paste duplication + +Large files and duplicated logic raise the cost and risk of every future change. The two auto-search mega-functions are near-identical cascades that must be kept in sync (a fix to one can silently miss the other); the adapter methods and keyword-candidate fallback repeat the same boilerplate; app.js (3926) and styles.css (2819) far exceed the 800-line ceiling. Most of this is documented, consciously deferred debt. + +_관련 발견: _auto_naver_search() and _auto_google_custom_search() each exceed 165 lines, NaverSearchAdapter has three search() / search_pages() / search_web_pages() methods with nearly identical structure, collect_keyword_candidates() is 85 lines with a repeated 3-phase naver fallback pattern, app.js is 3926 lines, styles.css at 2819 lines, formatReason() uses a cascade of string replacement fallbacks, window.prompt() used synchronously inside async event handlers_ + +### Test-coverage gaps for shipped fixes & security invariants + +Several recent fixes and security guarantees lack pinning tests, so a future refactor could silently regress them: the Naver blog/web dedup fix, mark_evidence_status atomicity, media-route auth enforcement, untrusted-media security headers, safeUrl/escapeHtml behaviour, and the redirect-SSRF handler. + +_관련 발견: No regression test for the Naver blog/web dedup fix (commit 71a6da1), mark_evidence_status atomicity is not tested; only record_decision rollback is pinned, Media route authentication is not verified under auth_token enforcement, Security headers on untrusted media (_file with untrusted=True) are not asserted in any test, safeUrl / escapeHtml XSS defence in app.js is structural-only; no behavioural assertions, _fetch_page_url_bytes and _fetch_stylesheet_url_bytes SSRF guard not independently tested, import-folder with empty/missing path body is not validated by the test suite, E2E decision flow covers only 4 scenarios; no test for hold/reject after prior approve_ + +### Air-gapped deployment completeness + +Core air-gap posture is correct (pinned deps, local assets, no telemetry), but three transfer/integrity gaps remain in documentation and supply chain: Ollama model weights and Playwright Chromium binaries are not documented for offline transfer, and the planned hash-verified lock file was never committed. + +_관련 발견: offline-install.md omits Ollama model bundling procedure, offline-install.md omits Playwright Chromium binary transfer procedure, No full transitive dependency lock file with hashes, LLM assistant is unconditionally constructed even when OLLAMA_BASE_URL is not set_ + +## 4. 리메디에이션 로드맵 + +## Prioritized Remediation Roadmap + +### Phase 1 — HIGH-severity correctness & integrity (fix before merge) +These are confirmed bugs with operator-visible or data-loss impact. Small, surgical fixes. + +1. **Make `mark_evidence_status` atomic** *(finding: "mark_evidence_status: non-atomic read-modify-write without _write_lock or _transaction")* — wrap the read/mutate/write/rescore/audit body in `with self._write_lock: with self._transaction() as conn:` and thread `conn=` through `_put`, `_rescore_submission`, and `add_audit_event`, mirroring `record_decision` (sqlite_store.py:569). **This is the highest priority — it was independently reported 6 times.** Effort: ~1 hr. Add a rollback test mirroring `test_record_decision_rolls_back_when_audit_fails` (closes "mark_evidence_status atomicity is not tested"). + +2. **Fix `promote_collection_candidates` concurrency + idempotency** *(finding: "promote_collection_candidates missing write lock and non-idempotent ID")* — acquire `_write_lock`, replace `_timestamp_id()` with `_stable_id('kb-candidate-batch', *sorted(candidate_ids))`, wrap all writes in `_transaction()`, and add the same already-promoted guard the single-candidate path has. Effort: ~1.5 hr. + +3. **Persist `deactivateCorrectionEntry` and provider-retry** *(findings: "deactivateCorrectionEntry mutates shared module-level arrays without an API call"; "provider-retry button writes a local-only synthetic audit entry with no API call")* — replace the local mutation + synthetic audit with a real `await apiJson(...)` followed by `await refreshFromApi()`, following the `toggleProvider` pattern. If no backend endpoint exists yet, disable the button / show "not yet available" rather than faking success. Effort: ~2 hr each (depends on whether endpoints exist). + +4. **Fix entity dedup under-scoring** *(finding: "Multiple distinct web-detection entity signals collapse into one scoring bucket")* — make the bucket entity-name-specific: `f"entity:{str(item.data.get('entity','')).casefold()}"`. While here, also fix the related LOW ordering asymmetry ("Entity and category-matched WEB_DETECTION items share the same dedup bucket") by mirroring `_web_detection_points` priority in `_web_detection_bucket`. Add a regression test for two distinct entities flipping a score to `high`. Effort: ~1.5 hr. + +5. **Add in-flight guard + await to `rerunHistoricalQuery`/`runManualSearch`** *(finding: "rerunHistoricalQuery calls async runManualSearch without await and without an in-flight guard")* — disable the submit button before `await` and re-enable in `finally`; `void` or `await` the call. Effort: ~1 hr. + +### Phase 2 — MEDIUM security hardening (defence-in-depth; batch into one security PR) +None are exploitable in the current air-gapped localhost model, but they are cheap and the deployment may move behind a proxy. + +6. **Security headers on the app shell** *(findings: "No security headers on main application responses"; "No Content-Security-Policy header on the app shell or API responses")* — in `_file()` when `untrusted=False` and content is HTML/JS, add `Content-Security-Policy: default-src 'self'; ... frame-ancestors 'none'`, `X-Frame-Options: DENY`, `X-Content-Type-Options: nosniff`. The app uses no inline scripts/CDN so a strict CSP is compatible. Effort: ~1 hr. + +7. **Decouple session cookie from the raw token** *(finding: "Cookie stores raw auth token, missing Secure flag")* — store an HMAC-derived session tag instead of the verbatim `COPYRIGHTER_AUTH_TOKEN`, and add the `Secure` attribute. Effort: ~1.5 hr. + +8. **Auth-token entropy check + auth rate limiting** *(findings: "No minimum-entropy check on COPYRIGHTER_AUTH_TOKEN"; "No rate limiting on authentication or any API endpoint")* — reject tokens < 32 chars at startup; add a per-IP failed-attempt counter with short lockout + `time.sleep` on failure. Effort: ~2 hr together. + +9. **Cap outbound response size** *(finding: "UrllibJsonTransport reads API responses without a size limit")* — apply `response.read(MAX + 1)` and raise `HttpJsonError` past the cap, mirroring `store_remote_fetch`. Prevents OOM from a runaway Ollama/API response. Effort: ~45 min. + +10. **Origin/Referer CSRF fallback** *(finding: "CSRF protection relies solely on SameSite=Strict")* and **keep API keys out of tracebacks** *(finding: "Google and Custom Search API keys embedded in outbound request URLs")* — add an Origin check for cookie-authed non-GET requests; move Google keys to a header/POST body or strip them from re-raised exceptions. Effort: ~2 hr together. + +11. **Escape remaining innerHTML / class sinks** *(findings: "innerHTML templates inject server-derived label lookups without escapeHtml"; "riskClass() injects unvalidated server string"; "submission.asset assigned directly to img.src without safeUrl"; "similar-strip and face-crop-strip image URLs not passed through safeUrl"; "faceCrops href and img src use escapeHtml but not safeUrl")** — wrap all server-derived values in `escapeHtml()`, allowlist `riskClass`, and route every `src`/`href` through `safeUrl()`. These are one-liners; batch them. Effort: ~1.5 hr total. + +### Phase 3 — MEDIUM correctness, error-handling & robustness +12. **Add `_write_lock` to operator rerun paths** *(finding: "rerun_enrichment and run_auto_search perform unguarded concurrent read-modify-write")*. Effort: ~30 min. +13. **Fix `ensure_queue` UPDATE WHERE-key bug** *(finding: "ensure_queue UPDATE uses computed queue_id instead of existing row id on folder_path match")* — use the existing row's id. Effort: ~45 min. +14. **Stop swallowing errors silently** *(findings: "_sync_face_crops swallows image load failure"; "_store_candidate_image silently discards network and fingerprint errors"; "_response_text falls back ... without logging"; "json.loads() on DB rows with no error handling"; "_search_result_page_image_candidates ... swallow all page-fetch exceptions")* — add `logging.warning`/`debug` with URL/submission-id context; wrap `json.loads` in try/except raising a domain error mapped to a safe 500. Effort: ~2 hr. +15. **Fix provider `lastFailure` attribution** *(finding: "Provider lastFailure attribution reads evidence[0] rather than the first failed item")* — use `next(... if item.source in FAILURE_SOURCES)`. Effort: ~30 min. +16. **Drop zero-point skip reasons from `ScoreResult.reasons`** *(finding: "SEARCH_SKIPPED and EXTERNAL_SKIPPED reasons pollute ScoreResult.reasons")*. Effort: ~30 min. +17. **GUI null-guards + double-submit guards** *(findings: "rerunSelectedEnrichment accesses button.disabled without null guard"; "applyCoverageFilter accesses sourceSelect.value without null guard"; "No in-flight guard on setDecision")*. Effort: ~1 hr. +18. **Targeted evidence query in dedup hot path** *(finding: "Auto-search dedup loads entire evidence table")* — swap `_evidence_by_submission().get(id)` for `_evidence_for_submission(id)`. Effort: ~20 min (the only performance finding). +19. **EXIF/GPS leak on PIL fallback** *(finding: "Silent EXIF/GPS metadata leakage when PIL re-encoding fails")* — log a warning (or hard-fail) instead of silently sending original bytes; important since an air-gapped target without Pillow hits this every submission. Effort: ~45 min. +20. **Make `ScoreResult.reasons` immutable** *(finding: "ScoreResult.reasons is a mutable list inside a frozen dataclass")* — convert to `tuple` in `__post_init__`. Effort: ~30 min. + +### Phase 4 — MEDIUM maintainability (schedule deliberately; larger refactors) +21. **Extract shared search-plan helper** *(finding: "_auto_naver_search() and _auto_google_custom_search() each exceed 165 lines")* — highest-value refactor; collapses two (partly three) copy-paste cascades into one `_execute_search_query_plan(...)`. Effort: ~1 day. +22. **De-duplicate adapter boilerplate** *(finding: "NaverSearchAdapter has three search()/search_pages()/search_web_pages() methods")* and **collapse the naver fallback cascade** *(finding: "collect_keyword_candidates() is 85 lines with a repeated 3-phase naver fallback")*. Effort: ~0.5 day. +23. **Split `app.js` and `styles.css`** *(findings: "app.js is 3926 lines"; "styles.css at 2819 lines")* — already documented as accepted/deferred debt (downgraded to MEDIUM). Split into the proposed ES-module / CSS-partial layout; concatenate at build for air-gap. Effort: ~1-2 days. Lower urgency — no runtime risk. +24. **Replace `window.prompt()` with inline forms** *(finding: "window.prompt() used synchronously inside async event handlers")* — also unblocks reliable Playwright testing. Effort: ~0.5 day. + +### Phase 5 — Test coverage & docs (close regression gaps) +25. **Add the missing regression tests** *(findings: "No regression test for the Naver blog/web dedup fix"; "mark_evidence_status atomicity is not tested" — covered by item 1; "Media route authentication is not verified"; "Security headers on untrusted media ... not asserted"; "safeUrl/escapeHtml XSS defence ... no behavioural assertions"; "_fetch_page/stylesheet SSRF guard not independently tested"; "import-folder with empty/missing path body is not validated"; "E2E decision flow ... no hold/reject after prior approve")*. Effort: ~1 day total; do alongside the fixes they protect. +26. **Document offline extras** *(findings: "offline-install.md omits Ollama model bundling procedure"; "offline-install.md omits Playwright Chromium binary transfer"; "No full transitive dependency lock file with hashes")* — add Ollama model + Playwright Chromium transfer sections and a `requirements-lock.txt` with `--hash` (the plan's Task 7). Effort: ~0.5 day. + +### Phase 6 — LOW defence-in-depth (opportunistic cleanup) +Batch when touching the relevant files: SSRF re-validation on `UrllibJsonTransport` redirects, explicit `credentials: 'same-origin'` on `fetch()`, `_put()` f-string fallback → explicit raise, `_drop_stale_rebuild_temp_tables` regex re-validation, sanitized `handle_error` override, and gating `_llm_assistant` construction on `OLLAMA_BASE_URL` (avoids per-submission connect-timeout stalls). Effort: ~0.5 day combined. + +## 5. 발견 상세 + +### 🟠 HIGH + +#### 1. mark_evidence_status: non-atomic read-modify-write without _write_lock or _transaction +- 분류 `bug` · 파일 `src/rights_filter/server/sqlite_store.py:587-617` · 신뢰도 high · 검증 **confirmed** · 유닛 `sec-sql` +- **설명:** mark_evidence_status performs a read (_get at line 596), mutates the dict in-memory (lines 600-608), writes (_put at line 609), rescores (line 610), then writes an audit event (lines 611-616) — all across four separate database connections with no coordinating lock and no _transaction() wrapper. This is the exact non-atomic pattern that record_decision (line 557-585) already fixed by using _transaction(). Under ThreadingHTTPServer, two concurrent operator requests marking the same evidence item will open interleaved connections: the second _get reads stale pre-first-write state, the second _put overwrites the first silently (last-writer-wins), and the audit entry for the losing write is recorded against the wrong final state. The rescore then fires on a different connection, potentially racing against the _put that just committed. The prior-context digest (§3) explicitly lists this as still PENDING: 'mark_evidence_status atomic state+audit transaction — same _transaction pattern as record_decision, not yet applied.' +- **영향:** Evidence status changes can be silently lost when two operators act concurrently on the same evidence item. The audit log records events that do not match the final DB state, undermining the audit trail integrity that is a product invariant ('filter never auto-changes review status'). +- **권장:** Wrap the read-mutate-write-audit sequence in a single _transaction() context and acquire self._write_lock, mirroring the pattern in record_decision (lines 569-584). The corrected body should be: with self._write_lock: with self._transaction() as conn: payload = self._get('evidence', evidence_id, conn=conn); ... self._put('evidence', evidence_id, payload, conn=conn); self._rescore_submission(submission_id); self.add_audit_event(..., conn=conn). + +#### 2. promote_collection_candidates missing write lock and non-idempotent ID +- 분류 `bug` · 파일 `src/rights_filter/server/store_operations.py:694-759` · 신뢰도 high · 검증 **confirmed** · 유닛 `bug-store` +- **설명:** promote_collection_candidates uses _timestamp_id() (non-deterministic) for entry_id and does not acquire _write_lock. Two concurrent batch-promote requests create two separate knowledge entries with different IDs and mark candidates with different promotedKnowledgeId values. The writes also use separate auto-commit calls with no _transaction() wrapper, so a server crash mid-loop leaves the knowledge entry written but some candidates not marked promoted. The single-candidate variant promote_collection_candidate (line 654) correctly acquires _write_lock, uses _stable_id for idempotency, and wraps its writes in _transaction(). +- **영향:** Concurrent double-clicks or parallel API requests create duplicate knowledge entries for the same candidate batch, polluting the reference DB with duplicates that cannot be idempotently collapsed. +- **권장:** Wrap the method body with 'with self._write_lock:' and replace _timestamp_id() with _stable_id('kb-candidate-batch', *sorted(candidate_ids)). Wrap all _put and add_audit_event calls in 'with self._transaction() as conn:' passing conn to each call, mirroring the pattern in promote_collection_candidate. + +#### 3. Multiple distinct web-detection entity signals collapse into one scoring bucket +- 분류 `bug` · 파일 `src/rights_filter/analysis/risk_scoring.py:29-37` · 신뢰도 high · 검증 **confirmed** · 유닛 `bug-analysis` +- **설명:** In `RiskScorer.score`, after computing `bucket = _web_detection_bucket(item)` for a `WEB_DETECTION` item, the code skips any item whose bucket is already in `counted_web_buckets`. `_web_detection_bucket` returns `"entity"` for ANY item that has a non-empty `data['entity']` field — regardless of which entity it identifies. As a result, if Google Vision returns two independent entity annotations for different known subjects (e.g. `entity='BTS'` then `entity='BLACKPINK'`, both with `category='celebrity'`), the second item is unconditionally skipped once the `'entity'` bucket is claimed by the first. Only the first entity ever contributes points. This can suppress 45+ points and shift a genuinely high-risk image to `medium`, preventing operator escalation. +- **영향:** Under-scoring of images that trigger multiple distinct entity recognitions. A composition image with two identifiable characters/celebrities would lose up to 45 points from the second detection, potentially staying at 'medium' rather than 'high', reducing operator scrutiny. +- **권장:** Make the dedup bucket specific enough to represent truly redundant signals. Options: (a) use `entity:` as the bucket key (e.g. `f"entity:{str(item.data.get('entity','')).casefold()}"`) so two different named entities get distinct buckets; (b) remove the entity bucket entirely and instead cap the WEB_DETECTION contribution in a post-loop clamp rather than per-category early exit. The intent of the dedup (preventing 10 identical 'visual similar' items from stacking) is correct — it just needs to be scoped to exact-duplicate signals, not all entity detections. + +#### 4. rerunHistoricalQuery calls async runManualSearch without await and without an in-flight guard +- 분류 `bug` · 파일 `web/operator-gui/app.js:2466-2479` · 신뢰도 high · 검증 **confirmed** · 유닛 `bug-frontend` +- **설명:** `rerunHistoricalQuery` (line 2466) populates the query input and provider select, then calls `runManualSearch(new Event('submit'))` synchronously — but `runManualSearch` is async. Because the call is not awaited and there is no `void`, the returned Promise is discarded. More critically, `runManualSearch` has no in-flight guard (no disabled state, no flag): if the user clicks two different 'Query Re-run' buttons in quick succession, or if the suggestion runner and the re-run fire close together, two concurrent POST /api/search/manual requests can race, both calling `refreshFromApi()` concurrently, which causes two parallel `/api/bootstrap` fetches and two `applyBootstrap` + `renderAll` calls that interleave non-atomically. The `runManualSearch` path itself also clears `queryInput.value` mid-flight, meaning the second call may POST an empty query string. +- **영향:** Concurrent POSTs can produce duplicate evidence entries or an empty-query search; interleaved bootstrap refreshes can leave the UI in an inconsistent state. +- **권장:** Add an in-flight guard to `runManualSearch` (analogous to `isSuggestedQueryRunActive` in `executeSuggestedQueries`): disable the submit button before `await apiJson(...)` and re-enable in a `finally`. In `rerunHistoricalQuery`, either `void` the call explicitly or convert the function to async and `await runManualSearch(...)`. The submit event propagation pattern (passing `new Event('submit')`) bypasses the form's own `submit` listener flow; it works here but is fragile — consider extracting the search logic into a shared helper instead. + +#### 5. deactivateCorrectionEntry mutates shared module-level arrays without an API call — state is never persisted and will be overwritten +- 분류 `bug` · 파일 `web/operator-gui/app.js:3221-3248` · 신뢰도 high · 검증 **confirmed** · 유닛 `bug-frontend` +- **설명:** `deactivateCorrectionEntry` (line 3221) directly mutates items inside the `corrections` array (`entry.active = false`) and pushes a synthetic entry onto `auditEvents`. There is no API call — no POST/PATCH to the server. The function then calls `renderAll()`. The apparent effect is shown in the UI, but the next call to `refreshFromApi()` (triggered by any other action, e.g. approving a case) will call `applyBootstrap` which calls `replaceCollection(corrections, payload.corrections)` and `replaceCollection(auditEvents, payload.auditEvents)`, overwriting the local mutations entirely. The 'deactivate derived entries' action silently becomes a no-op on the server, and the synthetic audit log entry disappears. This violates the invariant that every operator action must be persisted. +- **영향:** Operator believes derived knowledge entries are deactivated. On next data refresh the UI silently reverts. Audit trail entry is also lost. Data integrity is compromised. +- **권장:** Replace the local mutation with a real API call (e.g. POST /api/corrections/{id}/deactivate or PATCH /api/knowledge/{derived-entry-id}). Until a server endpoint exists, at minimum add a visible warning in the UI that this action is not yet persisted, or disable the button. The `void` cast on `deactivateKnowledgeEntry` at line 3634 shows the correct pattern for similar operations. + +#### 6. _auto_naver_search() and _auto_google_custom_search() each exceed 165 lines — highest-risk large functions in the Python layer +- 분류 `maintainability` · 파일 `src/rights_filter/server/store_enrichment.py:391-791` · 신뢰도 high · 검증 **confirmed** · 유닛 `mnt-hotspots` +- **설명:** _auto_naver_search() spans lines 557–791 (234 lines) and _auto_google_custom_search() spans lines 391–556 (165 lines). Both functions contain an identical structural skeleton: (a) plan queries, (b) check existing dedup signatures, (c) for each planned query call the image/primary search, (d) annotate evidence with strategy metadata, (e) write evidence rows, (f) call similarity matching, (g) append a history entry, (h) call the page/web secondary search with its own dedup check, (i) repeat (d)–(g) for the secondary results, (j) flush queryHistory to the submission, (k) update provider lastSuccess/lastFailure/usage. The only meaningful differences are: naver has three search types (image, blog, web) vs Google's two (image, web); naver uses blog_query_count / web_query_count rate limiters; the provider keys differ. This structural duplication means any fix to the copy-annotate-write-history pattern must be applied in two places. Additionally, _manual_search_evidence() at lines 426–534 (109 lines) replicates a subset of the same cascade (naver image → blog → web fallback; google image → web fallback) as a third copy. +- **영향:** Any fix to the dedup logic, the `query_strategy` annotation, or the history-flush pattern must be applied in both mega-functions. A bug introduced in one copy (e.g., missing `existing_signatures.add` for one search type) will not be caught without running both providers end-to-end. +- **권장:** Extract a shared `_execute_search_query_plan(submission_id, provider_id, planned_queries, image_store, *, call_one_query_fn)` helper that owns steps (b)–(k): it accepts a callable `call_one_query_fn(query) -> tuple[list[Evidence], str]` (returns evidence + history provider label) and handles dedup, evidence write, similarity, history flush, and usage update. _auto_naver_search and _auto_google_custom_search become thin callers that build the per-provider callable and pass it in. This also makes _manual_search_evidence naturally align with the same helper. + +### 🟡 MEDIUM + +#### 7. mark_evidence_status state update and audit write are non-atomic +- 분류 `bug` · 파일 `src/rights_filter/server/sqlite_store.py:587-617` · 신뢰도 high · 검증 **unverified** · 유닛 `sec-http` +- **설명:** This is the deferred item explicitly tracked in the prior-review plan. `_put(evidence, ...)` (line 609), `_rescore_submission` (line 610), and `add_audit_event` (line 611) execute as three separate auto-committed statements. A crash or concurrent write between the `_put` and `add_audit_event` calls leaves evidence with a new `operatorStatus` but no corresponding audit record, producing a silent audit divergence. `record_decision` (line 569) already uses `self._transaction()` for the same pattern — `mark_evidence_status` should too. +- **영향:** Audit trail can silently diverge from actual evidence state, undermining the integrity of the rights-review record. +- **권장:** Wrap the entire function body in `with self._transaction() as conn:` and thread `conn=conn` through `_put`, `_rescore_submission`, and `add_audit_event`, matching the pattern already used by `record_decision`. + +#### 8. ensure_queue UPDATE uses computed queue_id instead of existing row id on folder_path match +- 분류 `bug` · 파일 `src/rights_filter/server/store_queue.py:99-130` · 신뢰도 high · 검증 **unverified** · 유닛 `bug-store` +- **설명:** ensure_queue selects existing rows by id OR folder_path (lines 99-102). When a row is found via the folder_path match but that row has a different id than the newly computed queue_id, the UPDATE on lines 113-115 uses queue_id as the WHERE key and silently matches 0 rows. Line 117 then deactivates the actual existing row (id != queue_id), and the final SELECT finds nothing because no row was inserted or correctly updated. _queue_row_by_id(queue_id) also returns None, so the method returns None instead of a queue dict. +- **영향:** If a folder path normalization ever changes (e.g., symlink resolution), the next ensure_queue call silently returns None, causing seed_from_image_store to crash on str(queue['id']) with a TypeError. +- **권장:** After fetching existing, extract its id: existing_id = existing['id'] if existing else None. In the else branch use WHERE id = ? with existing_id. Adjust the deactivation on line 117 and the final SELECT on line 120 to use the effective active id. + +#### 9. rerun_enrichment and run_auto_search perform unguarded concurrent read-modify-write on evidence and submissions +- 분류 `bug` · 파일 `src/rights_filter/server/store_operations.py:202-281, 283-312` · 신뢰도 medium · 검증 **unverified** · 유닛 `bug-store` +- **설명:** Both rerun_enrichment and run_auto_search perform long chains of _get and _put calls on submissions and evidence tables without holding _write_lock. Two concurrent requests for the same submission can interleave: one reads pre-rerun evidence, the other finishes and writes its results, and the first then overwrites with stale data. seed_from_image_store correctly wraps its entire path with _write_lock (sqlite_store.py:229-234); the operator-triggered rerun paths do not. The _write_lock is a reentrant RLock so inner callers like _apply_provider_usage_delta that also acquire it will not deadlock. +- **영향:** Under concurrent operator-triggered reruns for the same submission, evidence writes are silently overwritten and per-provider usage counters can lose increments. +- **권장:** Acquire self._write_lock at the top of both rerun_enrichment and run_auto_search, mirroring the pattern used in seed_from_image_store and purge_expired_face_crops. + +#### 10. SEARCH_SKIPPED and EXTERNAL_SKIPPED reasons pollute ScoreResult.reasons despite contributing zero points +- 분류 `bug` · 파일 `src/rights_filter/analysis/risk_scoring.py:57-63` · 신뢰도 high · 검증 **unverified** · 유닛 `bug-analysis` +- **설명:** In the `FAILURE / EXTERNAL_SKIPPED / SEARCH_SKIPPED / ENRICHMENT_FAILURE` branch (lines 45-63), the code correctly adds 0 to `score` for `SEARCH_SKIPPED` and `EXTERNAL_SKIPPED` sources, but then unconditionally calls `reasons.append(item.reason)` on line 63 for all non-LLM paths including these zero-point sources. The `ScoreResult.reasons` list is shown to operators as justification for the risk score. Reasons like 'Search API skipped (quota)' or 'External provider disabled' are operational status messages, not risk signals, so including them in the reasons list misrepresents what drove the score — and they can appear even in a score=0 / band='low' result. The bug is that the `reasons.append` is outside the `if score += 30` block, coupling reason emission to all non-LLM sources regardless of point contribution. +- **영향:** Operators see misleading reason text in the review UI for skip/disabled states. A score=0 'low' result could display 'External provider disabled' as a reason, implying a risk signal when none exists. Separately, a legitimate high-risk item that also had search skipped will show the skip reason alongside the true risk reasons, diluting the signal clarity. +- **권장:** Move `reasons.append(item.reason)` inside the block that awards non-zero points, or introduce a separate `operational_notes` list in `ScoreResult` for zero-contribution status messages. Minimal fix: `if item.source in {EvidenceSource.FAILURE, EvidenceSource.ENRICHMENT_FAILURE}: score += 30; reasons.append(item.reason)` (handle the skipped sources separately without appending their reasons). + +#### 11. rerunSelectedEnrichment accesses button.disabled without null guard +- 분류 `bug` · 파일 `web/operator-gui/app.js:2540, 2551, 2580` · 신뢰도 high · 검증 **unverified** · 유닛 `bug-frontend` +- **설명:** `rerunSelectedEnrichment` fetches `document.getElementById('bulk-rerun')` (line 2540). If `ids.length` is 0, the early return at line 2545 fires before any button access. But if `ids.length > 0`, the code reaches `button.disabled = true` at line 2551 and `button.disabled = false` in `finally` at line 2580. If the element is ever absent from the DOM (e.g. test environment, or if index.html is modified), both accesses throw a TypeError that propagates out of the async function as an unhandled rejection, bypassing the `setImportStatus` error path. The analogous pattern in `rerunEnrichment` (line 2498) correctly does `if (button) button.disabled = true`. +- **영향:** Crashes the function in environments where the DOM element is missing (tests, future HTML refactors), producing an uncaught TypeError. +- **권장:** Apply the same null guard as `rerunEnrichment`: replace `button.disabled = true` with `if (button) button.disabled = true` and in the finally block `if (button) button.disabled = false`. + +#### 12. provider-retry button writes a local-only synthetic audit entry with no API call +- 분류 `bug` · 파일 `web/operator-gui/app.js:3665-3687` · 신뢰도 high · 검증 **unverified** · 유닛 `bug-frontend` +- **설명:** The `[data-provider-retry]` click handler (line 3665) pushes a hardcoded synthetic event into `auditEvents` and calls `renderAll()`, but makes no API call to actually retry failed enrichments. The audit entry says 'retry failed enrichments' but nothing is retried. On the next `refreshFromApi()` call the synthetic entry is overwritten. This is similar to the `deactivateCorrectionEntry` issue — the operator believes they triggered a retry but no server-side action occurred. +- **영향:** Operator action has no server-side effect; operator is misled by the synthetic audit entry which disappears on next refresh. +- **권장:** Implement the API call (e.g. POST /api/providers/{id}/retry-failures) and await it before updating the audit log. Until the endpoint is available, either hide the button or show a 'not yet implemented' message instead of creating a misleading synthetic audit entry. + +#### 13. applyCoverageFilter accesses sourceSelect.value without null guard +- 분류 `bug` · 파일 `web/operator-gui/app.js:498-510` · 신뢰도 medium · 검증 **unverified** · 유닛 `bug-frontend` +- **설명:** `applyCoverageFilter` calls `const sourceSelect = document.getElementById('source-filter')` then immediately accesses `sourceSelect.value` and `sourceSelect.options` at lines 504-505 without checking that `sourceSelect` is non-null. The function is called from the delegated `coverage-tabs` click handler which can fire at any time. If the DOM element is absent (test environment, or the coverage-tabs listener fires before the DOM is fully ready on a slow machine), this throws `TypeError: Cannot read properties of null`. +- **영향:** Uncaught TypeError if the DOM element is absent; potential crash of the coverage filter flow. +- **권장:** Add an early return guard: `if (!sourceSelect) return;` immediately after the `getElementById` call, consistent with the null check pattern used in `renderImportStatus` (line 1262) and `renderCollectionCandidates` (line 1947). + +#### 14. No in-flight guard on setDecision — rapid decision button clicks can POST multiple decisions +- 분류 `bug` · 파일 `web/operator-gui/app.js:2328-2380` · 신뢰도 high · 검증 **unverified** · 유닛 `bug-frontend` +- **설명:** `setDecision` (line 2328) does not disable the approve/hold/reject buttons before `await apiJson(...)` at line 2362. An operator who double-clicks a decision button, or clicks two different decision buttons quickly (e.g. 'approve' then immediately 'reject'), will fire two concurrent POST /api/submissions/{id}/decision requests. The second one may overwrite the first or produce a conflict depending on server logic. The server may guard against this, but the client has no protection. Compare with `rerunEnrichment` (line 2498) which correctly disables its button before the await. +- **영향:** Duplicate or conflicting decision POST requests can be sent; server state may diverge from operator intent. +- **권장:** Disable all three decision buttons (approve/hold/reject) before the await and re-enable them in a finally block: query `document.querySelectorAll('[data-decision]')` and toggle `.disabled` around the API call. + +#### 15. Provider lastFailure attribution reads evidence[0] rather than the first failed item +- 분류 `bug` · 파일 `src/rights_filter/server/store_enrichment.py:311, 545, 780` · 신뢰도 high · 검증 **unverified** · 유닛 `err-silent` +- **설명:** In `_rerun_google_image_search` (line 311), `_auto_google_custom_search` (line 545), and `_auto_naver_search` (line 780), when the failure condition is triggered (`any(item.source in {FAILURE, SEARCH_SKIPPED, ENRICHMENT_FAILURE} ...)`), the code sets `lastFailure` to `all_domain_evidence[0].reason` — the first item in the list, not the first failing item. If any successful evidence items come before the failure item in the list, the `lastFailure` field is set to the reason of a successful result (e.g. 'Naver search result found'), making the provider dashboard actively misleading. +- **영향:** The operator's provider health panel shows a spurious, plausible-sounding success message as the failure reason, obscuring the actual API error. This makes diagnosing quota exhaustion or API key rotation failures harder. +- **권장:** Replace `all_domain_evidence[0].reason` with `next((item.reason for item in all_domain_evidence if item.source in {EvidenceSource.FAILURE, EvidenceSource.SEARCH_SKIPPED, EvidenceSource.ENRICHMENT_FAILURE}), ' failed')` to ensure the reported reason is always from an actual failure item. + +#### 16. mark_evidence_status state+audit write is not wrapped in a transaction +- 분류 `bug` · 파일 `src/rights_filter/server/sqlite_store.py:587-617` · 신뢰도 high · 검증 **unverified** · 유닛 `err-silent` +- **설명:** `mark_evidence_status` (lines 587–617) performs three separate writes without an enclosing `_transaction()`: `self._put("evidence", ...)` at line 609, `self._rescore_submission` at line 610 (which itself does `_get` + `_put`), and `self.add_audit_event` at line 611. Each opens and commits its own connection. If the process is interrupted between the evidence `_put` and the `add_audit_event`, the evidence status changes are applied but unaudited. This is the same pattern that was fixed for `record_decision` (H6 in the prior review) but the fix was not extended to `mark_evidence_status`, which is noted in the prior digest's 'still PENDING' section. +- **영향:** An interrupted operator evidence-status change produces no audit trail, creating a divergence between the evidence state and the audit log. This is a governance integrity issue for the rights-review workflow. +- **권장:** Wrap the entire `mark_evidence_status` body (after validation) in `with self._transaction() as conn:`, passing `conn` to `_put`, `_rescore_submission`, and `add_audit_event`, following the same pattern as `record_decision` at line 569. + +#### 17. deactivateCorrectionEntry() mutates shared array in-place without a server round-trip — client state can desync +- 분류 `bug` · 파일 `web/operator-gui/app.js:3221-3248` · 신뢰도 high · 검증 **unverified** · 유닛 `mnt-hotspots` +- **설명:** Lines 3227–3229: `correction.derivedEntries.forEach((entry) => { entry.active = false; });` mutates the in-memory corrections array directly without calling any API endpoint. Lines 3233–3245 then push a fake audit event into the local `auditEvents` array. No HTTP call is made, so the deactivation is never persisted. After the next `refreshFromApi()` (which happens after the next unrelated action), the correction entries will reappear as active and the synthetic audit event will vanish. This violates the 'no mutation' coding standard and the 'errors handled explicitly' standard. Compare with deactivateKnowledgeEntry() (line 3346) which correctly calls the API. +- **영향:** Operators who click 'Deactivate derived entries' will see a brief success in the UI, but the state will silently revert to active on the next API refresh. This is a silent functional regression: the action appears to work but persists nothing. +- **권장:** Add a backend endpoint (e.g. `POST /api/corrections/{id}/deactivate-derived`) and replace the local mutation with an async call to that endpoint followed by `await refreshFromApi()`. If the endpoint does not exist yet, at minimum add a `console.warn` and document the stub, but the current silent-success UX is misleading to operators. + +#### 18. Provider retry button appends a fake audit event client-side without calling any API +- 분류 `bug` · 파일 `web/operator-gui/app.js:3665-3687` · 신뢰도 high · 검증 **unverified** · 유닛 `mnt-hotspots` +- **설명:** The `data-provider-retry` click handler (lines 3665–3687) pushes a synthetic audit event into the local `auditEvents` array and calls `renderAll()` — but makes no HTTP request. The retry does not actually trigger any server-side re-enrichment. The audit event disappears after the next `refreshFromApi()`. This is a stub that was never wired to a backend, but there is no visual indication to the operator that the button is non-functional beyond a transient log entry. +- **영향:** Operators who click 'Retry failed' believe a re-enrichment has been triggered. No work actually occurs, and the next refresh silently removes the fake log entry. This can cause operators to miss genuine failures. +- **권장:** Either wire the button to `POST /api/submissions/{id}/rerun-enrichment` or `POST /api/submissions/{id}/search-auto`, or disable and mark the button as 'not yet available' so operators are not misled. The existing `rerunEnrichment()` function (line 2484) already implements the correct API call pattern. + +#### 19. offline-install.md omits Ollama model bundling procedure +- 분류 `build` · 파일 `docs/operations/offline-install.md:1-60` · 신뢰도 high · 검증 **unverified** · 유닛 `bld-airgap` +- **설명:** The offline-install.md covers only Python wheels (numpy/opencv/pillow). It does not mention Ollama or the `qwen2.5:0.5b-instruct` model weights, which must also cross the air gap. `_llm_assistant` in `src/rights_filter/integrations/env_clients.py:239–245` always constructs an `OllamaGenerateLlmClient` pointing to `http://127.0.0.1:11434` (defaulting to the local Ollama daemon). If Ollama is installed on the target but the model was never pulled on the build machine and transferred, every `rerun-enrichment` call will fail with an `ENRICHMENT_FAILURE` evidence record (2-second connect timeout, graceful degradation via `InternalLlmAssistant.summarize` catch-all at `llm_assistance.py:71`). The runtime failure is non-crashing, but the operator will see silent LLM failures on every submission in a fully air-gapped deploy without documentation telling them why. +- **영향:** LLM-based evidence summaries silently fail on air-gapped targets where Ollama is installed but the model cache was not transferred. Operators may not understand why the feature is broken since the runbook does not mention this requirement. +- **권장:** Add an 'Optional: Ollama LLM' section to `docs/operations/offline-install.md` covering: (1) On the build machine: `ollama pull qwen2.5:0.5b-instruct` then locate and copy `~/.ollama/models/` (or the path in `OLLAMA_MODELS`). (2) On the target: install Ollama binary offline, point `OLLAMA_MODELS` at the transferred model cache. (3) Note that LLM is optional — if Ollama is absent, analysis proceeds without LLM summaries, which is acceptable. `copyrighter-operation-worklist.md:196` already documents `ollama pull` for online setups; cross-reference that and add the offline variant. + +#### 20. json.loads() on DB rows with no error handling — malformed payload propagates as uncontrolled 500 +- 분류 `error-handling` · 파일 `src/rights_filter/server/store_persistence.py:229, 257, 294, 303` · 신뢰도 medium · 검증 **unverified** · 유닛 `sec-sql` +- **설명:** All four json.loads() calls that deserialize stored payload rows (_get at line 229, _all at line 257, _evidence_for_submission at line 294, _evidence_by_submission at line 303) are bare — no try/except. The json_valid(payload) CHECK constraint guards INSERT/UPDATE paths and will catch newly-written bad rows, but it does not protect reads: legacy rows inserted before the constraint migration, rows written directly to the DB file outside this application, or any future code path that bypasses _put() would produce a JSONDecodeError. That exception propagates through the request handler without being caught, causing an unhandled 500. Depending on how the HTTP layer formats that error, a Python traceback including the file path, DB path, and partial payload value could be sent to the client. +- **영향:** A single corrupt row can render an entire API response (bootstrap(), review(), audit_events()) unavailable and may leak internal path or data structure information if the traceback reaches the operator GUI. +- **권장:** Add a try/except json.JSONDecodeError around each json.loads() in the persistence layer. On failure, log the error with the table and row id (but not the raw payload content) and either skip the row with a sentinel value or raise a domain-level StorageCorruptionError that the HTTP handler maps to a 500 with a safe static message. Example: try: return json.loads(row['payload']) except json.JSONDecodeError as exc: raise StorageCorruptionError(f'{table}/{id_value} has corrupt JSON payload') from exc. + +#### 21. _response_text falls back to 'Evidence summary unavailable.' without logging or raising +- 분류 `error-handling` · 파일 `src/rights_filter/analysis/llm_assistance.py:138-149` · 신뢰도 high · 검증 **unverified** · 유닛 `err-silent` +- **설명:** `_response_text` (line 138) attempts three different response-key patterns from the Ollama API response. If none match — including if Ollama silently returns a non-empty but structurally unexpected response body — the function silently returns the hardcoded fallback `"Evidence summary unavailable."` (line 149) with no log, no metric, and no signal to the caller. The caller `OllamaGenerateLlmClient.summarize_evidence` (line 50–56) will propagate this as if it were a normal summary string. `InternalLlmAssistant.summarize` then persists it as `EvidenceSource.LLM_SUMMARY` with `verified=True` when source URLs exist. +- **영향:** Silent API contract breakage: if Ollama's response format changes, every LLM evidence record becomes a fabricated 'unavailable' stub stored with full `verified=True` status, invisible to operators. +- **권장:** When all key patterns miss on a non-empty response body, log at WARNING level with the actual response keys (sanitized to avoid PII) and return the fallback only after logging. Alternatively, raise `HttpJsonError` so `InternalLlmAssistant.summarize` stores an `ENRICHMENT_FAILURE` record that is auditable. + +#### 22. _sync_face_crops swallows image load failure without any log or audit trail +- 분류 `error-handling` · 파일 `src/rights_filter/server/store_enrichment.py:150-153` · 신뢰도 high · 검증 **unverified** · 유닛 `err-silent` +- **설명:** `_sync_face_crops` (line 143) wraps `image_store.image_payload(submission_id)` in a bare `except Exception: return` (lines 150–153). The method is called from both the main seed loop (`sqlite_store.py:248`) and `_rerun_internal_analysis` (`store_enrichment.py:270`). When the image file is missing or corrupted, the face-crop sync silently returns: no log entry, no audit event, and the submission's `faceCrops` field is never set (left as absent or stale from a prior run). Downstream calls such as `_google_face_crop_web_detection` then read `submission.get("faceCrops")` and may use stale bounding boxes from a previous iteration. +- **영향:** Stale `faceCrops` from a previous analysis survive in the submission record, causing `_google_face_crop_web_detection` to attempt Google Vision calls on face boxes from an image that may no longer exist, wasting quota and potentially generating misleading evidence. The failure is entirely invisible in the audit log. +- **권장:** Replace `except Exception: return` with `except Exception as exc: logging.warning("face crop sync skipped for %s: %s", submission_id, exc); return`. Optionally clear `faceCrops` on the submission before returning so stale boxes are not left in place. + +#### 23. _store_candidate_image silently discards network and fingerprint errors with no log +- 분류 `error-handling` · 파일 `src/rights_filter/server/store_search_candidates.py:664-679` · 신뢰도 high · 검증 **unverified** · 유닛 `err-silent` +- **설명:** `_store_candidate_image` has two bare `except Exception` blocks: one at line 672 that discards errors reading a cached image from disk (returning `record = None`), and another at line 678 that discards all fetch errors (returning `None` to the caller). Neither block logs the error type, URL, or submission ID. This method is called inside tight loops in `_search_result_image_similarity_evidence`, `_face_crop_search_result_similarity_evidence`, `_collection_candidates_from_evidence`, and `_candidate_payload_from_evidence`. A systematic SSRF guard rejection, a 4xx/5xx HTTP error, or a disk-full `OSError` all produce the same silent `None` return, making it impossible to distinguish expected skip conditions (non-image URL) from real infrastructure failures. +- **영향:** SSRF guard rejections, rate-limit 429 responses, and disk-full errors are all silently swallowed. An operator running keyword collection or manual search sees 0 candidates returned with no explanation. There is no way to detect a systematic provider failure from logs alone. +- **권장:** Add at minimum a `logging.debug` (for expected non-image URLs) and `logging.warning` (for network or OS errors) inside each `except` block, including the URL and a truncated exception message. The SSRF `ValueError` in particular should be logged at WARNING to make attacker-induced SSRF attempts visible. + +#### 24. mark_evidence_status() writes evidence and audit in separate un-transacted calls +- 분류 `error-handling` · 파일 `src/rights_filter/server/sqlite_store.py:587-617` · 신뢰도 high · 검증 **unverified** · 유닛 `mnt-hotspots` +- **설명:** mark_evidence_status() (lines 596–617) performs: (a) self._put('evidence', ...) at line 609, (b) self._rescore_submission() at line 610, (c) self.add_audit_event() at line 611 — three separate DB writes without a wrapping _transaction(). If the process dies between (a) and (c), the evidence status is updated but no audit trail is recorded. This is explicitly listed as a remaining deferred item in the Prior Review Digest ('mark_evidence_status atomic state+audit transaction — same _transaction() pattern as record_decision, not yet applied'). +- **영향:** Evidence status changes can be recorded without an audit trail if the server crashes between the evidence write and the audit write. For a rights-review tool this is a compliance risk. +- **권장:** Wrap the body of mark_evidence_status() in `with self._transaction() as conn:` and pass `conn=conn` to both `self._put` and `self.add_audit_event`, matching the pattern used in `record_decision` and `promote_collection_candidate`. + +#### 25. ScoreResult.reasons is a mutable list inside a frozen dataclass, allowing inadvertent mutation +- 분류 `maintainability` · 파일 `src/rights_filter/domain/records.py:71-75` · 신뢰도 high · 검증 **unverified** · 유닛 `bug-analysis` +- **설명:** `ScoreResult` is declared `@dataclass(frozen=True)` to signal immutability, but `reasons: list[str] = field(default_factory=list)` makes the `reasons` attribute a mutable list. Python's frozen dataclass prevents attribute *reassignment* (`result.reasons = [...]` raises `FrozenInstanceError`) but does NOT prevent mutation of the list object itself (`result.reasons.append(...)` silently succeeds). Any code path that holds a reference to the `ScoreResult` can mutate the reasons list after the fact, violating the immutability guarantee the frozen annotation implies. This is particularly risky because `run.score` (an `AnalysisRun` field) holds the `ScoreResult`, and `run` is shared across multiple code paths in `EvidenceEnricher.enrich_latest`. +- **영향:** Any caller that obtains a `ScoreResult` and mutates `.reasons` will silently corrupt shared state. In the current `AnalysisRun`, where the same `score` object is read by the operator-facing review presenter after enrichment, a concurrent mutation (in a multi-threaded `ThreadingHTTPServer` context) would cause non-deterministic reason lists to appear in the UI. +- **권장:** Change `reasons` to `tuple[str, ...]` (immutable) and update all callers to pass `tuple(_unique(reasons))` at construction time. Alternatively, use `reasons: list[str] = field(default_factory=list)` with `__post_init__` converting to a tuple: `object.__setattr__(self, 'reasons', tuple(self.reasons))`. Update `ScoreResult` usages in tests and `RiskScorer.score` accordingly. + +#### 26. window.prompt() used synchronously inside async event handlers — blocks event loop +- 분류 `maintainability` · 파일 `web/operator-gui/app.js:3347, 3363` · 신뢰도 high · 검증 **unverified** · 유닛 `bug-frontend` +- **설명:** `deactivateKnowledgeEntry` (line 3347) and `reactivateKnowledgeEntry` (line 3363) call `window.prompt()` synchronously. This freezes the browser tab while the native dialog is open, blocking all scheduled microtasks and macro-tasks. It also fails silently in some automated and sandboxed environments (e.g. Playwright tests, iframes with `allow-modals` removed) where `window.prompt()` returns `null` immediately. For `deactivateKnowledgeEntry` the null return means the function exits early (correct); for `reactivateKnowledgeEntry` an empty string is indistinguishable from the user pressing Cancel vs. OK with no text. +- **영향:** Blocks the event loop during dialog display; breaks in sandboxed iframes and some test harnesses. +- **권장:** Replace `window.prompt()` with an inline form rendered into the DOM (same pattern as the `knowledge-edit-form` shown at lines 1883-1903) or a modal dialog. This is consistent with the rest of the UI which uses inline form patterns. If `window.prompt` must remain, document the Playwright test expectation for `page.on('dialog', ...)` so CI does not hang. + +#### 27. app.js is 3926 lines — far exceeds 800-line file limit +- 분류 `maintainability` · 파일 `web/operator-gui/app.js:1-3926` · 신뢰도 high · 검증 **confirmed** · 유닛 `mnt-hotspots` +- **설명:** app.js is nearly 4000 lines. The project standard is 800 lines maximum. The file contains at least six distinct responsibility clusters that are already partially separated for Python code: state management and bootstrap (lines 1–230), formatting/labeling utilities (lines 526–830), DOM rendering functions for each view (lines 1113–2183), event orchestration functions (lines 2328–3430), the event dispatcher (lines 3458–3824), and the init/boot sequence (lines 3828–3906). The file was deliberately left as a monolith for the frontend while the Python layer was split into mixins, but at 4k lines it violates the project standard and makes targeted review, testing, and parallel editing impractical. +- **영향:** No runtime bug today, but the file size makes diff review, targeted hot-fixes, and parallel module extraction significantly harder. Any PR touching this file touches 4k lines. +- **권장:** Split into at least six ES modules or IIFE namespaces loaded via `