fix(url-ingest): address PR #55 self-review findings

- _unique_path() prevents silent overwrites in raw/ when two URLs
  sanitize to the same filename (applied at both PDF and HTML write
  sites)
- PDF filename now derives from response.geturl() (the post-redirect
  URL), so DOI/shortlink resolvers produce sensible names
- add_single_file() returns bool; URL branch unlinks the fetched
  raw/* file when the downstream add is skipped (dedup), preventing
  orphan files that re-resurrect on every retry

Author: KylinMountain

openkb/cli.py

@@ -130,14 +130,21 @@ def _find_kb_dir(override: Path | None = None) -> Path | None:
     return None
 
 
-def add_single_file(file_path: Path, kb_dir: Path) -> None:
+def add_single_file(file_path: Path, kb_dir: Path) -> bool:
     """Convert, index, and compile a single document into the knowledge base.
 
     Steps:
     1. Load config to get the model name.
     2. Convert the document (hash-check; skip if already known).
     3. If long doc: run PageIndex then compile_long_doc.
     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.
     """
     from openkb.agent.compiler import compile_long_doc, compile_short_doc
     from openkb.state import HashRegistry
@@ -156,11 +163,11 @@ def add_single_file(file_path: Path, kb_dir: Path) -> None:
     except Exception as exc:
         click.echo(f"  [ERROR] Conversion failed: {exc}")
         logger.debug("Conversion traceback:", exc_info=True)
-        return
+        return False
 
     if result.skipped:
         click.echo(f"  [SKIP] Already in knowledge base: {file_path.name}")
-        return
+        return False
 
     doc_name = file_path.stem
     index_result = None  # populated only on the long-doc branch
@@ -174,7 +181,7 @@ def add_single_file(file_path: Path, kb_dir: Path) -> None:
         except Exception as exc:
             click.echo(f"  [ERROR] Indexing failed: {exc}")
             logger.debug("Indexing traceback:", exc_info=True)
-            return
+            return False
 
         summary_path = kb_dir / "wiki" / "summaries" / f"{doc_name}.md"
         click.echo(f"  Compiling long doc (doc_id={index_result.doc_id})...")
@@ -192,7 +199,7 @@ def add_single_file(file_path: Path, kb_dir: Path) -> None:
                 else:
                     click.echo(f"  [ERROR] Compilation failed: {exc}")
                     logger.debug("Compilation traceback:", exc_info=True)
-                    return
+                    return False
     else:
         click.echo(f"  Compiling short doc...")
         for attempt in range(2):
@@ -206,7 +213,7 @@ def add_single_file(file_path: Path, kb_dir: Path) -> None:
                 else:
                     click.echo(f"  [ERROR] Compilation failed: {exc}")
                     logger.debug("Compilation traceback:", exc_info=True)
-                    return
+                    return False
 
     # Register hash only after successful compilation
     if result.file_hash:
@@ -225,6 +232,7 @@ def add_single_file(file_path: Path, kb_dir: Path) -> None:
 
     append_log(kb_dir / "wiki", "ingest", file_path.name)
     click.echo(f"  [OK] {file_path.name} added to knowledge base.")
+    return True
 
 
 # ---------------------------------------------------------------------------
@@ -408,15 +416,22 @@ def add(ctx, path):
         click.echo("No knowledge base found. Run `openkb init` first.")
         return
 
-    # URL ingest: download into raw/ first, then fall through to the
-    # normal file-path branch below. fetch_url_to_raw returns the
-    # local path it wrote, or None on failure.
+    # URL ingest: download into raw/ first, then call add_single_file
+    # explicitly so we can clean up the just-downloaded file if it
+    # turns out to be a duplicate (registry already has its hash).
+    # Without this, re-adding the same URL leaves an orphan in raw/
+    # that the registry can't reach via openkb remove.
     from openkb.url_ingest import looks_like_url, fetch_url_to_raw
     if looks_like_url(path):
         fetched = fetch_url_to_raw(path, kb_dir)
         if fetched is None:
             return
-        path = str(fetched)
+        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.
+            fetched.unlink(missing_ok=True)
+        return
 
     target = Path(path)
     if not target.exists():

openkb/url_ingest.py

@@ -126,6 +126,28 @@ def _pdf_filename(url: str, content_disposition: str | None) -> str:
     return _sanitize_filename(last or "document", ".pdf")
 
 
+def _unique_path(target: Path) -> Path:
+    """Return ``target`` if it doesn't exist yet, otherwise append ``_2``,
+    ``_3``, … to the stem until an unused name is found.
+
+    Prevents silent overwrites in ``raw/`` when two different URLs
+    sanitize to the same filename (e.g. two blog posts both titled
+    "Introduction" → both ``Introduction.md``).
+    """
+    if not target.exists():
+        return target
+    stem = target.stem
+    suffix = target.suffix
+    parent = target.parent
+    for i in range(2, 10_000):
+        candidate = parent / f"{stem}_{i}{suffix}"
+        if not candidate.exists():
+            return candidate
+    raise RuntimeError(
+        f"Could not find a free filename for {target} after 10k attempts"
+    )
+
+
 def _download_pdf_chunked(response, head_bytes: bytes, target: Path) -> None:
     """Write the already-read ``head_bytes`` plus the remaining streamed
     body to ``target``. Chunked so very large PDFs (50+ MB) don't sit in
@@ -179,7 +201,10 @@ def _extract_html(url: str, raw_dir: Path) -> Path | None:
     metadata = trafilatura.extract_metadata(raw_html)
     title = (metadata.title if metadata else None) or url
     filename = _sanitize_filename(title, ".md")
-    target = raw_dir / filename
+    # Pick a non-colliding name — two blog posts titled "Introduction"
+    # would otherwise overwrite each other in raw/ and leave the hash
+    # registry pointing at stale bytes.
+    target = _unique_path(raw_dir / filename)
     target.write_text(markdown, encoding="utf-8")
     click.echo(
         f"  Extracted: {title!r}\n"
@@ -228,13 +253,20 @@ def fetch_url_to_raw(url: str, kb_dir: Path) -> Path | None:
         actual = _sniff_content_type(head_bytes, declared)
 
         if actual == "pdf":
+            # Derive the filename from the *post-redirect* URL — urllib
+            # follows redirects by default, so the user-typed URL may
+            # not be the URL that actually served the bytes (DOI / short
+            # link resolvers, mirror redirects, etc.). Falls back to the
+            # original input when the response doesn't expose a final
+            # URL.
+            final_url = response.geturl() or url
             filename = _pdf_filename(
-                url, response.headers.get("Content-Disposition"),
+                final_url, response.headers.get("Content-Disposition"),
             )
-            target = raw_dir / filename
+            target = _unique_path(raw_dir / filename)
             _download_pdf_chunked(response, head_bytes, target)
             size_mb = target.stat().st_size / (1024 * 1024)
-            click.echo(f"  Saved: raw/{filename} ({size_mb:.1f} MB PDF)")
+            click.echo(f"  Saved: raw/{target.name} ({size_mb:.1f} MB PDF)")
             return target
 
     if actual == "html":

tests/test_url_ingest.py

@@ -10,6 +10,7 @@
     _pdf_filename,
     _sanitize_filename,
     _sniff_content_type,
+    _unique_path,
     fetch_url_to_raw,
     looks_like_url,
 )
@@ -182,6 +183,9 @@ def get(self, key, default=None):
     # read(N) returns chunks; read() with no arg returns rest
     stream = io.BytesIO(body)
     resp.read = stream.read
+    # urllib's HTTPResponse exposes geturl(); default to empty string so
+    # callers using `response.geturl() or url` fall through to the input URL.
+    resp.geturl = lambda: ""
     resp.__enter__ = lambda self: resp
     resp.__exit__ = lambda *a: None
     return resp
@@ -359,3 +363,181 @@ def test_fetch_network_error_returns_none(tmp_path, capsys):
     assert result is None
     err = capsys.readouterr().err
     assert "Network error" in err
+
+
+# ---------------------------------------------------------------------------
+# Self-review fixes from PR #55 review pass
+# ---------------------------------------------------------------------------
+
+
+def test_unique_path_returns_target_when_free(tmp_path):
+    p = tmp_path / "foo.pdf"
+    assert _unique_path(p) == p
+
+
+def test_unique_path_finds_next_free_slot(tmp_path):
+    """_2 / _3 / … must be appended to the stem (not the suffix) and
+    must keep probing until a free name is found."""
+    (tmp_path / "foo.pdf").write_bytes(b"a")
+    (tmp_path / "foo_2.pdf").write_bytes(b"b")
+
+    result = _unique_path(tmp_path / "foo.pdf")
+    assert result == tmp_path / "foo_3.pdf"
+
+
+def test_unique_path_handles_no_suffix(tmp_path):
+    """Files without an extension still get a usable suffix."""
+    (tmp_path / "README").write_text("x")
+    result = _unique_path(tmp_path / "README")
+    assert result.name == "README_2"
+
+
+def test_fetch_pdf_picks_unique_name_when_target_exists(tmp_path):
+    """Two URLs that sanitize to the same filename must NOT silently
+    overwrite — the second one gets `_2` appended."""
+    raw_dir = tmp_path / "raw"
+    raw_dir.mkdir()
+    (raw_dir / "paper.pdf").write_bytes(b"%PDF-existing\nfirst")
+
+    body = b"%PDF-1.7\nsecond URL content"
+    resp = _fake_response(body=body, headers={"Content-Type": "application/pdf"})
+    # Make response.geturl mimic real urllib (returns the input URL when
+    # there's no redirect). The fake response builder doesn't set this.
+    resp.geturl = lambda: "https://mirror.example.com/paper.pdf"
+
+    with patch("urllib.request.urlopen", return_value=resp):
+        result = fetch_url_to_raw("https://mirror.example.com/paper.pdf", tmp_path)
+
+    # First file is untouched, second went to paper_2.pdf
+    assert (raw_dir / "paper.pdf").read_bytes() == b"%PDF-existing\nfirst"
+    assert result == raw_dir / "paper_2.pdf"
+    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."""
+    raw_dir = tmp_path / "raw"
+    raw_dir.mkdir()
+    (raw_dir / "Introduction.md").write_text("first blog post body")
+
+    resp = _fake_response(body=b"<html>", headers={"Content-Type": "text/html"})
+
+    second_md = "# Introduction\n\nA completely different second blog post body. " * 10
+    fake_meta = MagicMock()
+    fake_meta.title = "Introduction"
+
+    with patch("urllib.request.urlopen", return_value=resp), \
+         patch("trafilatura.fetch_url", return_value="<html>...</html>"), \
+         patch("trafilatura.extract", return_value=second_md), \
+         patch("trafilatura.extract_metadata", return_value=fake_meta):
+        result = fetch_url_to_raw("https://blog2.example.com/post", 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
+
+
+def test_fetch_pdf_uses_post_redirect_url_for_filename(tmp_path):
+    """When urllib follows a redirect (DOI → publisher CDN, short URLs,
+    etc.), the filename must be derived from the final URL — not the
+    user's original input — when the response has no Content-Disposition
+    to override either."""
+    body = b"%PDF-1.7\n..."
+    resp = _fake_response(
+        body=body,
+        headers={"Content-Type": "application/pdf"},  # NO Content-Disposition
+    )
+    # urllib's HTTPResponse.geturl() returns the post-redirect URL
+    resp.geturl = lambda: "https://publisher.example.com/articles/2024/great-paper.pdf"
+
+    with patch("urllib.request.urlopen", return_value=resp):
+        result = fetch_url_to_raw("https://doi.org/10.1234/abc", tmp_path)
+
+    # Filename comes from the redirected URL's basename, not "abc"
+    assert result is not None
+    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."""
+    from openkb.cli import add_single_file
+    from openkb.converter import ConvertResult
+
+    # Build a minimal KB scaffold
+    (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" / "concepts").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 converted")
+
+    mock_result = ConvertResult(
+        raw_path=doc, source_path=source_path,
+        is_long_doc=False, file_hash="cafe" * 16,
+    )
+
+    with patch("openkb.cli.convert_document", return_value=mock_result), \
+         patch("openkb.cli.asyncio.run"):
+        added = add_single_file(doc, tmp_path)
+
+    assert added is True
+
+
+def test_add_single_file_returns_false_on_skip(tmp_path):
+    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()
+    doc = tmp_path / "raw" / "x.md"
+    doc.write_text("# Hello")
+
+    skipped = ConvertResult(skipped=True)
+    with patch("openkb.cli.convert_document", return_value=skipped):
+        added = add_single_file(doc, tmp_path)
+
+    assert added is False
+
+
+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/
+    so the user doesn't accumulate untracked duplicates."""
+    from click.testing import CliRunner
+    from openkb.cli import cli
+    from openkb.converter import ConvertResult
+
+    # Minimal KB
+    (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()
+
+    # Fake the URL fetch — write directly to where url_ingest would
+    fetched_path = tmp_path / "raw" / "paper.pdf"
+    fetched_path.write_bytes(b"%PDF-fake")
+
+    runner = CliRunner()
+    # fetch_url_to_raw is lazy-imported inside `add`, so patch it at the
+    # source module — that's where the `from ... import` resolves.
+    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=ConvertResult(skipped=True)):
+        result = runner.invoke(cli, ["add", "https://example.com/paper.pdf"])
+
+    assert result.exit_code == 0, result.output
+    assert "[SKIP]" in result.output
+    # Orphan cleanup: the URL-fetched file must be gone from raw/.
+    assert not fetched_path.exists()