umbragit/UmbraAudio
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix 3 Phase 2 bugs found by infra review

Browse Source 
1. merge_speakers never committed its utterance re-normalization. The CLI
   action path also returned without closing the connection, so Python rolled
   the transaction back on exit — merges silently lost every utterance rename.
   Now commits explicitly, and the CLI closes via try/finally.
   Verified: merging Guard 1 -> Guard 2 moved 6 utterances; a fresh connection
   sees 0/6 instead of the old 6/0.
2. assign_token_to_speaker wrote a dict into alias_evidence, but the discovery
   schema is list[str] and CharacterDiscoverer._alias_has_evidence() calls
   .lower() on each item — the next character-discovery run would have raised
   AttributeError. Now writes a descriptive sentence that also contains the
   alias, so the evidence check returns True for it.
3. dismiss_token overwrote evidence_json with the reason, discarding the scan
   evidence. Now preserves the original list and appends 'dismissed: <reason>'.

Added regression tests: cross-connection merge commit (file-backed DB),
alias-evidence string-list schema + discovery compatibility, dismiss evidence
preservation. 46 resolution/scanner tests green; 85 across Phase 1+2+discovery.
This commit is contained in:
B.D. Murphy
2026-09-24 11:31:33 -07:00
parent 14ee391722
commit 4d900d77a6
3 changed files with 108 additions and 6 deletions
Download Patch File Download Diff File Expand all files Collapse all files

7
scripts/scan_name_tokens.py
View File

@@ -131,7 +131,12 @@ def main() -> int:
if (args.assign or args.merge) and not args.speaker:
print("--speaker is required for --assign/--merge")
return 1
return _run_action(args, cfg, db)
try:
return _run_action(args, cfg, db)
finally:
# Close explicitly so any committed action is flushed and no
# dangling transaction is left open.
db.close()
if args.chapters:
ids = [int(x.strip()) for x in args.chapters.split(",")]

77
tests/test_orphan_resolution.py
View File

@@ -104,6 +104,29 @@ class TestAssign:
).fetchone()
assert row["status"] == "assigned" and row["target_speaker"] == "Guard 1"
def test_assign_alias_evidence_is_string_list(self, env) -> None:
# Regression guard: discovery's _alias_has_evidence() calls .lower() on
# each alias_evidence item, so it must stay list[str] (not dicts).
db, data_dir = env
_add_utt(db, "The Kid")
assign_token_to_speaker(db, data_dir, 1, "The Kid", "Guard 1")
guard = next(s for s in _load(data_dir) if s["primary_name"] == "Guard 1")
for item in guard["alias_evidence"]:
assert isinstance(item, str), f"non-string evidence: {item!r}"
item.lower() # must not raise
def test_assign_evidence_survives_discovery_alias_check(self, env) -> None:
# The evidence string must mention the alias so _alias_has_evidence()
# returns True for it on a later discovery pass.
from umbraaudio.analysis.character_discovery import CharacterDiscoverer
db, data_dir = env
_add_utt(db, "The Kid")
assign_token_to_speaker(db, data_dir, 1, "The Kid", "Guard 1")
guard = next(s for s in _load(data_dir) if s["primary_name"] == "Guard 1")
discoverer = CharacterDiscoverer.__new__(CharacterDiscoverer)
assert discoverer._alias_has_evidence(guard, "The Kid") is True
class TestCreate:
def test_create_from_token(self, env) -> None:
@@ -138,6 +161,26 @@ class TestDismiss:
# speakers.json and utterances must be untouched.
assert all("The Voice" not in s["primary_name"] for s in _load(data_dir))
def test_dismiss_preserves_scan_evidence(self, env) -> None:
from umbraaudio.analysis.name_tokens import (
NameToken,
ensure_name_tokens_table,
persist_name_tokens,
)
db, data_dir = env
ensure_name_tokens_table(db)
tok = NameToken(token="The Voice", kind="name_tag", occurrences=2, sample_texts=["a voice says,"])
persist_name_tokens(db, 1, [tok])
dismiss_token(db, 1, "The Voice", reason="not a character")
row = db.execute(
"SELECT evidence_json FROM name_tokens WHERE token='The Voice'"
).fetchone()
evidence = json.loads(row["evidence_json"])
# Original sample evidence survives; the reason is appended.
assert "a voice says," in evidence
assert any("dismissed: not a character" in e for e in evidence)
class TestMerge:
def test_merge_folds_aliases_and_renormalizes(self, env) -> None:
@@ -173,6 +216,40 @@ class TestMerge:
result = merge_speakers(db, data_dir, "Guard 1", "Kainda")
assert result["merged"] is False
def test_merge_commits_for_a_fresh_connection(self, tmp_path) -> None:
# Regression guard: merge_speakers must COMMIT its utterance rename, so a
# later, independent connection sees the re-normalized speaker. Uses a
# file-backed DB because in-memory DBs are per-connection.
db_path = tmp_path / "p.db"
db = sqlite3.connect(db_path)
db.row_factory = sqlite3.Row
db.execute("CREATE TABLE project (id INTEGER PRIMARY KEY)")
db.execute("INSERT INTO project (id) VALUES (1)")
db.execute(
"CREATE TABLE utterances (id INTEGER PRIMARY KEY, speaker_name TEXT, "
"type TEXT, text TEXT)"
)
db.execute("INSERT INTO utterances (speaker_name, type, text) VALUES ('Guard 1','dialogue','x')")
db.commit()
data_dir = tmp_path / ".umbra"
data_dir.mkdir()
(data_dir / "speakers.json").write_text(
json.dumps({"speakers": [
{"primary_name": "Kainda", "aliases": [], "chapters_appeared": []},
{"primary_name": "Guard 1", "aliases": [], "chapters_appeared": []},
]})
)
merge_speakers(db, data_dir, "Guard 1", "Kainda")
db.close()
# A completely separate connection must see the committed rename.
verify = sqlite3.connect(db_path)
verify.row_factory = sqlite3.Row
name = verify.execute("SELECT speaker_name FROM utterances").fetchone()["speaker_name"]
assert name == "Kainda"
verify.close()
def test_merge_into_self_raises(self, env) -> None:
db, data_dir = env
with pytest.raises(ResolutionError, match="itself"):

30
umbraaudio/analysis/orphan_resolution.py
View File

@@ -132,13 +132,16 @@ def assign_token_to_speaker(
already_alias = token.lower() in {a.lower() for a in aliases}
if token.lower() != primary.lower() and not already_alias:
aliases.append(token)
# alias_evidence is list[str] in the discovery schema
# (CharacterDiscoverer._alias_has_evidence calls .lower() on each item),
# so record a descriptive sentence, not a dict.
evidence = speaker.setdefault("alias_evidence", [])
if not isinstance(evidence, list):
evidence = []
speaker["alias_evidence"] = evidence
evidence.append(
{"alias": token, "source": "name_token_scan", "chapters": []}
)
entry = f"'{token}' confirmed as an alias of {primary} via name-token scan"
if entry not in evidence:
evidence.append(entry)
# Capture any utterances currently labelled with the token before renaming.
affected = db.execute(
@@ -231,18 +234,32 @@ def dismiss_token(
if not token:
raise ResolutionError("token required")
ensure_name_tokens_table(db)
# Preserve the original scan evidence and append the dismissal reason
# rather than overwriting it.
row = db.execute(
"SELECT evidence_json FROM name_tokens WHERE project_id=? AND token=?",
(project_id, token),
).fetchone()
try:
evidence = json.loads(row["evidence_json"]) if row and row["evidence_json"] else []
except (json.JSONDecodeError, TypeError):
evidence = []
if not isinstance(evidence, list):
evidence = [str(evidence)]
if reason:
evidence.append(f"dismissed: {reason}")
cur = db.execute(
"UPDATE name_tokens SET status=?, target_speaker=NULL, "
"evidence_json=?, updated_at=datetime('now') "
"WHERE project_id=? AND token=?",
(STATUS_DISMISSED, json.dumps([reason] if reason else []), project_id, token),
(STATUS_DISMISSED, json.dumps(evidence), project_id, token),
)
if (cur.rowcount or 0) == 0:
# Row not present: create it so the dismissal is recorded.
db.execute(
"INSERT INTO name_tokens (project_id, token, status, evidence_json) "
"VALUES (?, ?, ?, ?)",
(project_id, token, STATUS_DISMISSED, json.dumps([reason] if reason else [])),
(project_id, token, STATUS_DISMISSED, json.dumps(evidence)),
)
db.commit()
return {"ok": True, "token": token, "status": STATUS_DISMISSED}
@@ -302,6 +319,9 @@ def merge_speakers(
_save_speaker_doc(data_dir, doc, speakers)
changed = _renormalize_utterances(db, old_names, target_name)
# merge_speakers does not call _set_token_status, so it must commit its own
# utterance re-normalization here or the transaction is lost on close.
db.commit()
return {
"ok": True,
"merged": True,