fix(url-ingest): preserve raw file on pipeline failure, fix HTML echo

Round-2 review on commit 47e5ec2 surfaced two issues:

1. add_single_file returned bool, so the URL branch couldn't tell
   "dedup skip" from "mid-pipeline failure". A transient LLM error
   during compile_long_doc would delete the just-downloaded PDF and
   leave a dangling PageIndex entry (hash registration only happens
   on full success, so `openkb remove` couldn't recover it). Now the
   function returns Literal["added","skipped","failed"]; the URL
   branch only unlinks on "skipped" — failures keep the raw file so
   the user can retry without re-downloading.

2. _extract_html echoed the pre-collision filename instead of
   target.name, so on a title collision the user was told the file
   was saved to a path occupied by the previous document. PDF branch
   already used target.name correctly; HTML branch now matches.

Author: KylinMountain

openkb/cli.py

@@ -14,6 +14,7 @@
 import sys
 import time
 from pathlib import Path
+from typing import Literal
 
 import os
 
@@ -130,7 +131,7 @@ def _find_kb_dir(override: Path | None = None) -> Path | None:
     return None
 
 
-def add_single_file(file_path: Path, kb_dir: Path) -> bool:
+def add_single_file(file_path: Path, kb_dir: Path) -> Literal["added", "skipped", "failed"]:
     """Convert, index, and compile a single document into the knowledge base.
 
     Steps:
@@ -140,11 +141,12 @@ def add_single_file(file_path: Path, kb_dir: Path) -> bool:
     4. Else: compile_short_doc.
 
     Returns:
-        True if the file was newly indexed and compiled. False when the
-        file was skipped as a duplicate (hash already registered) or
-        when any pipeline stage failed — callers that need to clean up
-        the source file (URL-ingest path, for example) can act on a
-        ``False`` return.
+        ``"added"`` on full success, ``"skipped"`` when the file's hash
+        is already in the registry (dedup), or ``"failed"`` when any
+        pipeline stage raised. URL-ingest distinguishes these so it can
+        unlink the just-downloaded raw file on dedup (it would otherwise
+        be an orphan) while preserving it on failure so the user can
+        retry without re-downloading.
     """
     from openkb.agent.compiler import compile_long_doc, compile_short_doc
     from openkb.state import HashRegistry
@@ -163,11 +165,11 @@ def add_single_file(file_path: Path, kb_dir: Path) -> bool:
     except Exception as exc:
         click.echo(f"  [ERROR] Conversion failed: {exc}")
         logger.debug("Conversion traceback:", exc_info=True)
-        return False
+        return "failed"
 
     if result.skipped:
         click.echo(f"  [SKIP] Already in knowledge base: {file_path.name}")
-        return False
+        return "skipped"
 
     doc_name = file_path.stem
     index_result = None  # populated only on the long-doc branch
@@ -181,7 +183,7 @@ def add_single_file(file_path: Path, kb_dir: Path) -> bool:
         except Exception as exc:
             click.echo(f"  [ERROR] Indexing failed: {exc}")
             logger.debug("Indexing traceback:", exc_info=True)
-            return False
+            return "failed"
 
         summary_path = kb_dir / "wiki" / "summaries" / f"{doc_name}.md"
         click.echo(f"  Compiling long doc (doc_id={index_result.doc_id})...")
@@ -199,7 +201,7 @@ def add_single_file(file_path: Path, kb_dir: Path) -> bool:
                 else:
                     click.echo(f"  [ERROR] Compilation failed: {exc}")
                     logger.debug("Compilation traceback:", exc_info=True)
-                    return False
+                    return "failed"
     else:
         click.echo(f"  Compiling short doc...")
         for attempt in range(2):
@@ -213,7 +215,7 @@ def add_single_file(file_path: Path, kb_dir: Path) -> bool:
                 else:
                     click.echo(f"  [ERROR] Compilation failed: {exc}")
                     logger.debug("Compilation traceback:", exc_info=True)
-                    return False
+                    return "failed"
 
     # Register hash only after successful compilation
     if result.file_hash:
@@ -232,7 +234,7 @@ def add_single_file(file_path: Path, kb_dir: Path) -> bool:
 
     append_log(kb_dir / "wiki", "ingest", file_path.name)
     click.echo(f"  [OK] {file_path.name} added to knowledge base.")
-    return True
+    return "added"
 
 
 # ---------------------------------------------------------------------------
@@ -426,10 +428,12 @@ def add(ctx, path):
         fetched = fetch_url_to_raw(path, kb_dir)
         if fetched is None:
             return
-        added = add_single_file(fetched, kb_dir)
-        if not added:
-            # Duplicate (or failed mid-pipeline) — drop the just-fetched
-            # file so raw/ doesn't accumulate orphans across retries.
+        outcome = add_single_file(fetched, kb_dir)
+        # Only clean up on dedup-skip. On "failed" we keep the file so
+        # the user can retry (e.g. transient LLM error during compile)
+        # without re-downloading — and so they don't lose data when
+        # indexing has already succeeded but compilation didn't.
+        if outcome == "skipped":
             fetched.unlink(missing_ok=True)
         return
 

openkb/url_ingest.py

@@ -208,7 +208,7 @@ def _extract_html(url: str, raw_dir: Path) -> Path | None:
     target.write_text(markdown, encoding="utf-8")
     click.echo(
         f"  Extracted: {title!r}\n"
-        f"  Saved: raw/{filename} ({len(markdown) // 1024 or 1} KB clean markdown)"
+        f"  Saved: raw/{target.name} ({len(markdown) // 1024 or 1} KB clean markdown)"
     )
     return target
 

tests/test_url_ingest.py

@@ -414,8 +414,10 @@ def test_fetch_pdf_picks_unique_name_when_target_exists(tmp_path):
     assert result.read_bytes() == body
 
 
-def test_fetch_html_picks_unique_name_when_target_exists(tmp_path):
-    """Two blog posts both titled 'Introduction' must NOT collide."""
+def test_fetch_html_picks_unique_name_when_target_exists(tmp_path, capsys):
+    """Two blog posts both titled 'Introduction' must NOT collide. The
+    user-facing 'Saved: ...' echo must also reflect the renamed path —
+    otherwise the message lies about where the file actually went."""
     raw_dir = tmp_path / "raw"
     raw_dir.mkdir()
     (raw_dir / "Introduction.md").write_text("first blog post body")
@@ -435,6 +437,8 @@ def test_fetch_html_picks_unique_name_when_target_exists(tmp_path):
     assert (raw_dir / "Introduction.md").read_text() == "first blog post body"
     assert result == raw_dir / "Introduction_2.md"
     assert result.read_text() == second_md
+    out = capsys.readouterr().out
+    assert "Saved: raw/Introduction_2.md" in out
 
 
 def test_fetch_pdf_uses_post_redirect_url_for_filename(tmp_path):
@@ -458,10 +462,10 @@ def test_fetch_pdf_uses_post_redirect_url_for_filename(tmp_path):
     assert result.name == "great-paper.pdf"
 
 
-def test_add_single_file_returns_true_on_success(tmp_path):
-    """The new bool return contract: True when the file was actually
-    indexed. Used by the URL-ingest cleanup path to decide whether the
-    just-downloaded file in raw/ should be unlinked."""
+def test_add_single_file_returns_added_on_success(tmp_path):
+    """Tri-state return contract: ``"added"`` when the file was newly
+    indexed. URL-ingest uses this to decide whether to keep / unlink
+    the just-downloaded file."""
     from openkb.cli import add_single_file
     from openkb.converter import ConvertResult
 
@@ -487,12 +491,12 @@ def test_add_single_file_returns_true_on_success(tmp_path):
 
     with patch("openkb.cli.convert_document", return_value=mock_result), \
          patch("openkb.cli.asyncio.run"):
-        added = add_single_file(doc, tmp_path)
+        outcome = add_single_file(doc, tmp_path)
 
-    assert added is True
+    assert outcome == "added"
 
 
-def test_add_single_file_returns_false_on_skip(tmp_path):
+def test_add_single_file_returns_skipped_on_dedup(tmp_path):
     from openkb.cli import add_single_file
     from openkb.converter import ConvertResult
 
@@ -505,14 +509,48 @@ def test_add_single_file_returns_false_on_skip(tmp_path):
 
     skipped = ConvertResult(skipped=True)
     with patch("openkb.cli.convert_document", return_value=skipped):
-        added = add_single_file(doc, tmp_path)
+        outcome = add_single_file(doc, tmp_path)
 
-    assert added is False
+    assert outcome == "skipped"
+
+
+def test_add_single_file_returns_failed_on_pipeline_error(tmp_path):
+    """A pipeline failure (e.g. transient LLM error during compilation)
+    must be distinguishable from dedup-skip, so URL-ingest can preserve
+    the raw file for retry instead of deleting it."""
+    from openkb.cli import add_single_file
+    from openkb.converter import ConvertResult
+
+    (tmp_path / ".openkb").mkdir()
+    (tmp_path / ".openkb" / "config.yaml").write_text("model: gpt-4o-mini\n")
+    (tmp_path / ".openkb" / "hashes.json").write_text("{}")
+    (tmp_path / "raw").mkdir()
+    (tmp_path / "wiki" / "summaries").mkdir(parents=True)
+    (tmp_path / "wiki" / "sources").mkdir(parents=True)
+    (tmp_path / "wiki" / "log.md").write_text("")
+
+    doc = tmp_path / "raw" / "x.md"
+    doc.write_text("# Hello")
+    source_path = tmp_path / "wiki" / "sources" / "x.md"
+    source_path.write_text("# Hello")
+
+    mock_result = ConvertResult(
+        raw_path=doc, source_path=source_path,
+        is_long_doc=False, file_hash="cafe" * 16,
+    )
+
+    # Make both compile attempts raise to drive the failure path.
+    with patch("openkb.cli.convert_document", return_value=mock_result), \
+         patch("openkb.cli.asyncio.run", side_effect=RuntimeError("LLM 503")), \
+         patch("openkb.cli.time.sleep"):
+        outcome = add_single_file(doc, tmp_path)
+
+    assert outcome == "failed"
 
 
 def test_url_ingest_cleans_up_orphan_on_dedup_skip(tmp_path, monkeypatch):
     """End-to-end: when the URL-fetched file is already in the registry,
-    add_single_file returns False and the CLI must unlink it from raw/
+    add_single_file returns "skipped" and the CLI unlinks it from raw/
     so the user doesn't accumulate untracked duplicates."""
     from click.testing import CliRunner
     from openkb.cli import cli
@@ -541,3 +579,44 @@ def test_url_ingest_cleans_up_orphan_on_dedup_skip(tmp_path, monkeypatch):
     assert "[SKIP]" in result.output
     # Orphan cleanup: the URL-fetched file must be gone from raw/.
     assert not fetched_path.exists()
+
+
+def test_url_ingest_keeps_raw_file_on_pipeline_failure(tmp_path):
+    """The point of the tri-state return: a pipeline failure (e.g. LLM
+    timeout during compilation) must NOT delete the downloaded file —
+    the user can retry without re-downloading, and we don't lose data
+    when indexing has already succeeded but compilation hasn't."""
+    from click.testing import CliRunner
+    from openkb.cli import cli
+    from openkb.converter import ConvertResult
+
+    (tmp_path / ".openkb").mkdir()
+    (tmp_path / ".openkb" / "config.yaml").write_text("model: gpt-4o-mini\n")
+    (tmp_path / ".openkb" / "hashes.json").write_text("{}")
+    (tmp_path / "raw").mkdir()
+    (tmp_path / "wiki" / "summaries").mkdir(parents=True)
+    (tmp_path / "wiki" / "sources").mkdir(parents=True)
+    (tmp_path / "wiki" / "log.md").write_text("")
+
+    fetched_path = tmp_path / "raw" / "paper.pdf"
+    fetched_path.write_bytes(b"%PDF-fake")
+    source_path = tmp_path / "wiki" / "sources" / "paper.md"
+    source_path.write_text("# fake")
+
+    mock_result = ConvertResult(
+        raw_path=fetched_path, source_path=source_path,
+        is_long_doc=False, file_hash="cafe" * 16,
+    )
+
+    runner = CliRunner()
+    with patch("openkb.cli._find_kb_dir", return_value=tmp_path), \
+         patch("openkb.url_ingest.fetch_url_to_raw", return_value=fetched_path), \
+         patch("openkb.cli.convert_document", return_value=mock_result), \
+         patch("openkb.cli.asyncio.run", side_effect=RuntimeError("LLM 503")), \
+         patch("openkb.cli.time.sleep"):
+        result = runner.invoke(cli, ["add", "https://example.com/paper.pdf"])
+
+    assert result.exit_code == 0, result.output
+    assert "[ERROR] Compilation failed" in result.output
+    # The raw file must be preserved so the user can retry.
+    assert fetched_path.exists()