fix: Phase 1 HIGH review remediation (5 of 6)
Backend (data-integrity / audit-trail): - mark_evidence_status: make the read-modify-write + audit atomic under _write_lock + _transaction (was 4 unlocked connections, last-writer-wins, audit could diverge from DB state). - promote_collection_candidates: acquire _write_lock, use a deterministic _stable_id, wrap writes in _transaction, and guard already-promoted batches so a retry/double-click cannot mint duplicate knowledge entries. - risk_scoring: bucket web-detection entities by name so two distinct entities in one image both count (was collapsed to one 'entity' bucket, under-scoring high->medium). Identical entities still dedup. Frontend (silent loss of operator actions): - runManualSearch: in-flight guard; rerunHistoricalQuery voids the call, preventing duplicate/racing search POSTs. - deactivateCorrectionEntry: persist via the real knowledge-deactivate endpoint instead of a local-only mutation + fake audit event. - provider-retry button: report honestly (no backend endpoint) instead of writing a fake 'Provider called' audit event. Tests: +4 regression tests (mark_evidence_status rollback, batch-promote idempotency, two distinct entities -> high, identical entity dedup). Remaining Phase 1 item #6 (_auto_naver/_auto_google refactor) deferred. Backend suite: 361 passed. app.js: node --check OK, 41 static tests pass.
This commit is contained in:
changukyu
parent
d008e35c2e
commit
89c1fe4788
6 changed files with 250 additions and 127 deletions
|
|
@ -93,10 +93,15 @@ def _web_detection_points(item: Evidence) -> int:
|
|||
|
||||
def _web_detection_bucket(item: Evidence) -> str:
|
||||
match = str(item.data.get("match", ""))
|
||||
if match in {"full", "partial", "page", "visual", "entity"}:
|
||||
if match in {"full", "partial", "page", "visual"}:
|
||||
return match
|
||||
if item.data.get("entity"):
|
||||
return "entity"
|
||||
# Distinct named entities are independent risk signals (e.g. two different
|
||||
# celebrities/characters in one image), so they must not share one "entity"
|
||||
# bucket and dedup each other away. Key the bucket by the entity name; only
|
||||
# genuinely identical entities collapse.
|
||||
entity = str(item.data.get("entity", "")).strip()
|
||||
if match == "entity" or entity:
|
||||
return f"entity:{entity.casefold()}" if entity else "entity"
|
||||
if item.data.get("url"):
|
||||
return "url"
|
||||
return ""
|
||||
|
|
|
|||
|
|
@ -593,10 +593,16 @@ class CopyrighterStore(
|
|||
) -> dict[str, Any]:
|
||||
if status not in EVIDENCE_OPERATOR_STATUSES:
|
||||
raise ValueError("unsupported evidence status")
|
||||
payload = self._get("evidence", evidence_id)
|
||||
# Serialize the read-modify-write so two operators marking the same item
|
||||
# cannot lose each other's update, and persist the evidence change + its
|
||||
# audit event atomically (mirrors record_decision). The rescore is
|
||||
# derived state: it runs after the commit but still under the write lock,
|
||||
# so no other writer interleaves between the change and the rescore.
|
||||
with self._write_lock:
|
||||
with self._transaction() as conn:
|
||||
payload = self._get("evidence", evidence_id, conn=conn)
|
||||
if str(payload.get("submission_id", "")) != submission_id:
|
||||
raise ValueError("evidence does not belong to submission")
|
||||
|
||||
payload["operatorStatus"] = status
|
||||
payload["operatorStatusLabel"] = EVIDENCE_OPERATOR_STATUSES[status]
|
||||
payload["operatorStatusNote"] = note.strip()
|
||||
|
|
@ -606,14 +612,15 @@ class CopyrighterStore(
|
|||
payload["contributed"] = False
|
||||
else:
|
||||
payload["contributed"] = _default_evidence_contribution(payload)
|
||||
self._put("evidence", evidence_id, payload)
|
||||
self._rescore_submission(submission_id)
|
||||
self._put("evidence", evidence_id, payload, conn=conn)
|
||||
self.add_audit_event(
|
||||
actor="rights.ops",
|
||||
event="Evidence status changed",
|
||||
object_id=evidence_id,
|
||||
change=f"{EVIDENCE_OPERATOR_STATUSES[status]} · {note or 'note optional'}",
|
||||
conn=conn,
|
||||
)
|
||||
self._rescore_submission(submission_id)
|
||||
return self.review(submission_id)
|
||||
|
||||
def providers(self) -> list[dict[str, Any]]:
|
||||
|
|
|
|||
|
|
@ -696,6 +696,7 @@ class StoreOperationsMixin:
|
|||
if not candidate_ids:
|
||||
raise ValueError("candidate_ids required")
|
||||
|
||||
with self._write_lock:
|
||||
candidates = [self._get("collection_candidates", candidate_id) for candidate_id in candidate_ids]
|
||||
sample_fingerprints = _unique_texts(
|
||||
fingerprint
|
||||
|
|
@ -705,7 +706,17 @@ class StoreOperationsMixin:
|
|||
if not sample_fingerprints:
|
||||
raise ValueError("selected candidates have no sample fingerprints")
|
||||
|
||||
entry_id = f"kb-candidate-{_timestamp_id()}"
|
||||
# Deterministic id so a racing retry / double-click upserts the same
|
||||
# knowledge entry instead of minting a duplicate timestamped row.
|
||||
entry_id = _stable_id("kb-candidate-batch", *sorted(candidate_ids))
|
||||
# Idempotent: if this exact batch was already promoted, do nothing.
|
||||
if all(
|
||||
candidate.get("status") == "promoted"
|
||||
and candidate.get("promotedKnowledgeId") == entry_id
|
||||
for candidate in candidates
|
||||
):
|
||||
return self.bootstrap()
|
||||
|
||||
queries = _unique_texts(str(candidate.get("query", "")) for candidate in candidates)
|
||||
image_assets = _unique_texts(str(candidate.get("imageAsset", "")) for candidate in candidates)
|
||||
image_facts = [
|
||||
|
|
@ -745,16 +756,18 @@ class StoreOperationsMixin:
|
|||
"items": image_facts,
|
||||
},
|
||||
}
|
||||
self._put("knowledge_entries", entry_id, entry)
|
||||
with self._transaction() as conn:
|
||||
self._put("knowledge_entries", entry_id, entry, conn=conn)
|
||||
for candidate in candidates:
|
||||
candidate["status"] = "promoted"
|
||||
candidate["promotedKnowledgeId"] = entry_id
|
||||
self._put("collection_candidates", str(candidate["id"]), candidate)
|
||||
self._put("collection_candidates", str(candidate["id"]), candidate, conn=conn)
|
||||
self.add_audit_event(
|
||||
"rights.ops",
|
||||
"Knowledge entry manually created",
|
||||
name,
|
||||
f"promoted {len(candidates)} collected candidates",
|
||||
conn=conn,
|
||||
)
|
||||
return self.bootstrap()
|
||||
|
||||
|
|
|
|||
|
|
@ -229,3 +229,54 @@ def test_score_is_clamped_to_100():
|
|||
)
|
||||
assert result.score == 100
|
||||
assert result.band == "high"
|
||||
|
||||
|
||||
def test_two_distinct_web_entities_stack_into_high_risk():
|
||||
# Two different named entities in one image are independent signals and must
|
||||
# not dedup each other away. Regression: both collapsed into one "entity"
|
||||
# bucket, dropping the second +45 and under-scoring a high image to medium.
|
||||
result = RiskScorer().score(
|
||||
[
|
||||
Evidence(
|
||||
source=EvidenceSource.WEB_DETECTION,
|
||||
reason="Web entity matched character A",
|
||||
confidence=0.94,
|
||||
data={"entity": "캐릭터 A", "category": "character"},
|
||||
),
|
||||
Evidence(
|
||||
source=EvidenceSource.WEB_DETECTION,
|
||||
reason="Web entity matched celebrity B",
|
||||
confidence=0.94,
|
||||
data={"entity": "유명인 B", "category": "celebrity"},
|
||||
),
|
||||
]
|
||||
)
|
||||
|
||||
assert result.score >= 70
|
||||
assert result.band == "high"
|
||||
assert "Web entity matched character A" in result.reasons
|
||||
assert "Web entity matched celebrity B" in result.reasons
|
||||
|
||||
|
||||
def test_same_web_entity_repeated_does_not_double_count():
|
||||
# The same entity reported twice IS redundant and must still dedup to one
|
||||
# signal — guards the fix from over-counting genuinely identical entities.
|
||||
result = RiskScorer().score(
|
||||
[
|
||||
Evidence(
|
||||
source=EvidenceSource.WEB_DETECTION,
|
||||
reason="Web entity matched character A",
|
||||
confidence=0.94,
|
||||
data={"entity": "캐릭터 A", "category": "character"},
|
||||
),
|
||||
Evidence(
|
||||
source=EvidenceSource.WEB_DETECTION,
|
||||
reason="Web entity matched character A (again)",
|
||||
confidence=0.94,
|
||||
data={"entity": "캐릭터 A", "category": "character"},
|
||||
),
|
||||
]
|
||||
)
|
||||
|
||||
assert result.score == 45
|
||||
assert result.band == "medium"
|
||||
|
|
|
|||
|
|
@ -8044,6 +8044,56 @@ def test_record_decision_rolls_back_when_audit_fails(tmp_path: Path, monkeypatch
|
|||
store._get("knowledge_entries", store_serialization_module._stable_id("kb-watchlist", "SUB-1"))
|
||||
|
||||
|
||||
def test_mark_evidence_status_rolls_back_when_audit_fails(tmp_path: Path, monkeypatch):
|
||||
image_store = _image_store(tmp_path / "submissions")
|
||||
store = CopyrighterStore(tmp_path / "copyrighter.sqlite3")
|
||||
store.initialize()
|
||||
store.seed_from_image_store(image_store)
|
||||
face_evidence = next(item for item in store.review("SUB-DB1")["evidence"] if item["source"] == "face")
|
||||
|
||||
def boom(*args, **kwargs):
|
||||
raise RuntimeError("audit down")
|
||||
|
||||
monkeypatch.setattr(store, "add_audit_event", boom)
|
||||
with pytest.raises(RuntimeError):
|
||||
store.mark_evidence_status("SUB-DB1", face_evidence["id"], "false_positive", "오탐")
|
||||
|
||||
# The evidence status change must roll back atomically with the failed audit.
|
||||
after = next(item for item in store.review("SUB-DB1")["evidence"] if item["id"] == face_evidence["id"])
|
||||
assert after.get("operatorStatus") != "false_positive"
|
||||
|
||||
|
||||
def test_batch_promote_collection_candidates_is_idempotent_under_retry(tmp_path: Path):
|
||||
store = CopyrighterStore(tmp_path / "copyrighter.sqlite3")
|
||||
store.initialize()
|
||||
for index in (1, 2):
|
||||
store._put(
|
||||
"collection_candidates",
|
||||
f"CAND-{index}",
|
||||
{
|
||||
"id": f"CAND-{index}",
|
||||
"provider": "naver",
|
||||
"query": f"query-{index}",
|
||||
"title": f"Candidate {index}",
|
||||
"status": "candidate",
|
||||
"sourceUrl": f"https://example.test/{index}",
|
||||
"collectedEpoch": index,
|
||||
"sampleFingerprints": [f"fp-{index}"],
|
||||
},
|
||||
)
|
||||
request = {"candidate_ids": ["CAND-1", "CAND-2"], "name": "Batch entry"}
|
||||
|
||||
store.promote_collection_candidates(request)
|
||||
# A double-click / retry must NOT mint a second knowledge entry.
|
||||
store.promote_collection_candidates(request)
|
||||
|
||||
entries = store.bootstrap()["knowledgeEntries"]
|
||||
assert len(entries) == 1
|
||||
candidate = store._get("collection_candidates", "CAND-1")
|
||||
assert candidate["status"] == "promoted"
|
||||
assert candidate["promotedKnowledgeId"] == entries[0]["id"]
|
||||
|
||||
|
||||
@pytest.mark.parametrize(
|
||||
"url",
|
||||
[
|
||||
|
|
|
|||
|
|
@ -2381,10 +2381,14 @@ async function setDecision(decision, requiresMemo = false) {
|
|||
|
||||
|
||||
|
||||
let isManualSearchActive = false;
|
||||
|
||||
async function runManualSearch(event) {
|
||||
|
||||
event.preventDefault();
|
||||
|
||||
if (isManualSearchActive) return;
|
||||
|
||||
const submission = getSelectedCase();
|
||||
|
||||
const provider = document.getElementById("manual-query-provider").value;
|
||||
|
|
@ -2429,6 +2433,8 @@ async function runManualSearch(event) {
|
|||
|
||||
|
||||
|
||||
isManualSearchActive = true;
|
||||
|
||||
try {
|
||||
|
||||
await apiJson("/api/search/manual", {
|
||||
|
|
@ -2449,6 +2455,10 @@ async function runManualSearch(event) {
|
|||
|
||||
status.textContent = errorValue.message;
|
||||
|
||||
} finally {
|
||||
|
||||
isManualSearchActive = false;
|
||||
|
||||
}
|
||||
|
||||
}
|
||||
|
|
@ -2475,7 +2485,7 @@ function rerunHistoricalQuery(button) {
|
|||
|
||||
providerSelect.value = provider;
|
||||
|
||||
runManualSearch(new Event("submit"));
|
||||
void runManualSearch(new Event("submit"));
|
||||
|
||||
}
|
||||
|
||||
|
|
@ -3218,34 +3228,35 @@ function addKnowledgeEntryFromCase() {
|
|||
|
||||
|
||||
|
||||
function deactivateCorrectionEntry(correctionId) {
|
||||
|
||||
async function deactivateCorrectionEntry(correctionId) {
|
||||
const correction = corrections.find((item) => item.id === correctionId);
|
||||
|
||||
if (!correction) return;
|
||||
|
||||
correction.derivedEntries.forEach((entry) => {
|
||||
|
||||
entry.active = false;
|
||||
// Deactivate the derived knowledge entries through the real backend endpoint
|
||||
// so the change is persisted and audited server-side. The previous version
|
||||
// only mutated local state and appended a fake audit event, so the action was
|
||||
// silently discarded on the next refreshFromApi().
|
||||
const entryIds = (correction.derivedEntries || [])
|
||||
.map((entry) => entry && entry.id)
|
||||
.filter(Boolean);
|
||||
if (!entryIds.length) {
|
||||
showApiError("이 보정 항목에는 서버에 반영할 수 있는 파생 지식 항목 ID가 없습니다.");
|
||||
return;
|
||||
}
|
||||
|
||||
try {
|
||||
let payload;
|
||||
for (const entryId of entryIds) {
|
||||
payload = await apiJson(`/api/knowledge/${encodeURIComponent(entryId)}/deactivate`, {
|
||||
method: "POST",
|
||||
body: JSON.stringify({ reason: "보정 파생 항목 일괄 비활성화" }),
|
||||
});
|
||||
|
||||
auditEvents.unshift({
|
||||
|
||||
timestamp: "방금",
|
||||
|
||||
actor: "rights.ops",
|
||||
|
||||
event: "Knowledge entry deactivated",
|
||||
|
||||
object: correction.decisionId,
|
||||
|
||||
change: "all derived entries inactive",
|
||||
|
||||
});
|
||||
|
||||
renderAll();
|
||||
|
||||
}
|
||||
if (payload) applyBootstrap(payload);
|
||||
await refreshFromApi();
|
||||
} catch (errorValue) {
|
||||
showApiError(errorValue.message);
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
|
|
@ -3665,25 +3676,11 @@ function bindEvents() {
|
|||
const retryButton = event.target.closest("[data-provider-retry]");
|
||||
|
||||
if (retryButton) {
|
||||
|
||||
auditEvents.unshift({
|
||||
|
||||
timestamp: "방금",
|
||||
|
||||
actor: "admin.ops",
|
||||
|
||||
event: "Provider called",
|
||||
|
||||
object: retryButton.dataset.providerRetry,
|
||||
|
||||
change: "retry failed enrichments",
|
||||
|
||||
});
|
||||
|
||||
renderAll();
|
||||
|
||||
// No provider-wide retry endpoint exists. Tell the operator honestly
|
||||
// instead of writing a fake "Provider called" audit event that never
|
||||
// re-ran anything (which falsely implied the failures were retried).
|
||||
showApiError("제공자 단위 '실패 재시도'는 아직 서버에 구현되지 않았습니다. 개별 케이스의 '재분석'을 사용하세요.");
|
||||
return;
|
||||
|
||||
}
|
||||
|
||||
|
||||
|
|
@ -3692,7 +3689,7 @@ function bindEvents() {
|
|||
|
||||
if (correctionButton) {
|
||||
|
||||
deactivateCorrectionEntry(correctionButton.dataset.deactivateCorrection);
|
||||
void deactivateCorrectionEntry(correctionButton.dataset.deactivateCorrection);
|
||||
|
||||
return;
|
||||
|
||||
|
|
|
|||
Loading…
Reference in a new issue