fix: code review fixes — security, robustness, tests, and CI hardening

- Restore markitdown[all] extras for docx/pptx/xlsx support
- Sanitize concept names to prevent path traversal in compiler
- Add path traversal guard in copy_relative_images
- Fix _write_concept duplicate append when frontmatter lacks sources key
- Remove dead write_wiki_files function
- Fix watcher thread race in _schedule_flush
- Warn when unimplemented --fix flag is used in lint command
- Harden CI publish workflow with environment gate and SHA-pinned actions
- Fix test_indexer to actually assert IndexConfig flag values
- Fix test_converter to test correct PDF code path (pymupdf, not markitdown)
- Use str.find() instead of str.index() in frontmatter parsing to avoid ValueError

Author: KylinMountain

.github/workflows/publish.yml

@@ -8,12 +8,13 @@ on:
 jobs:
   publish:
     runs-on: ubuntu-latest
+    environment: pypi
     permissions:
       id-token: write
     steps:
-      - uses: actions/checkout@v4
+      - uses: actions/checkout@692973e3d937129bcbf40652eb9f2f61becf3332  # v4.2.2
 
-      - uses: actions/setup-python@v5
+      - uses: actions/setup-python@f677139bbe7f9c59b41e40162b753c062f5d49a3  # v5.2.0
         with:
           python-version: "3.12"
 
@@ -24,4 +25,4 @@ jobs:
         run: python -m build
 
       - name: Publish to PyPI
-        uses: pypa/gh-action-pypi-publish@release/v1
+        uses: pypa/gh-action-pypi-publish@fb13cb306901256ace3dab689990e13a5550ffaa  # release/v1.11.0

openkb/agent/compiler.py

@@ -302,14 +302,15 @@ def _write_concept(wiki_dir: Path, name: str, content: str, source_file: str, is
         existing = path.read_text(encoding="utf-8")
         if source_file not in existing:
             if existing.startswith("---"):
-                end = existing.index("---", 3)
-                fm = existing[:end + 3]
-                body = existing[end + 3:]
-                if "sources:" in fm:
-                    fm = fm.replace("sources: [", f"sources: [{source_file}, ")
-                else:
-                    fm = fm.replace("---\n", f"---\nsources: [{source_file}]\n", 1)
-                existing = fm + body
+                end = existing.find("---", 3)
+                if end != -1:
+                    fm = existing[:end + 3]
+                    body = existing[end + 3:]
+                    if "sources:" in fm:
+                        fm = fm.replace("sources: [", f"sources: [{source_file}, ")
+                    else:
+                        fm = fm.replace("---\n", f"---\nsources: [{source_file}]\n", 1)
+                    existing = fm + body
             else:
                 existing = f"---\nsources: [{source_file}]\n---\n\n" + existing
             existing += f"\n\n{content}"
@@ -334,14 +335,15 @@ def _add_related_link(wiki_dir: Path, concept_slug: str, doc_name: str, source_f
     # Update sources in frontmatter
     if source_file not in text:
         if text.startswith("---"):
-            end = text.index("---", 3)
-            fm = text[:end + 3]
-            body = text[end + 3:]
-            if "sources:" in fm:
-                fm = fm.replace("sources: [", f"sources: [{source_file}, ")
-            else:
-                fm = fm.replace("---\n", f"---\nsources: [{source_file}]\n", 1)
-            text = fm + body
+            end = text.find("---", 3)
+            if end != -1:
+                fm = text[:end + 3]
+                body = text[end + 3:]
+                if "sources:" in fm:
+                    fm = fm.replace("sources: [", f"sources: [{source_file}, ")
+                else:
+                    fm = fm.replace("---\n", f"---\nsources: [{source_file}]\n", 1)
+                text = fm + body
         else:
             text = f"---\nsources: [{source_file}]\n---\n\n" + text
 

openkb/agent/tools.py

@@ -73,41 +73,3 @@ def write_wiki_file(path: str, content: str, wiki_root: str) -> str:
     full_path.write_text(content, encoding="utf-8")
     return f"Written: {path}"
 
-
-def write_wiki_files(files_json: str, wiki_root: str) -> str:
-    """Write multiple Markdown files to the wiki in one call.
-
-    Args:
-        files_json: JSON array of objects, each with ``"path"`` and ``"content"`` keys.
-            Example: ``[{"path": "concepts/foo.md", "content": "# Foo\\n..."}]``
-        wiki_root: Absolute path to the wiki root directory.
-
-    Returns:
-        Summary of written files, or error message on failure.
-    """
-    import json
-
-    try:
-        files = json.loads(files_json)
-    except json.JSONDecodeError as exc:
-        return f"Invalid JSON: {exc}"
-
-    if not isinstance(files, list):
-        return "Expected a JSON array of {path, content} objects."
-
-    root = Path(wiki_root).resolve()
-    written: list[str] = []
-    for entry in files:
-        path = entry.get("path", "")
-        content = entry.get("content", "")
-        if not path:
-            continue
-        full_path = (root / path).resolve()
-        if not full_path.is_relative_to(root):
-            written.append(f"Skipped (path escape): {path}")
-            continue
-        full_path.parent.mkdir(parents=True, exist_ok=True)
-        full_path.write_text(content, encoding="utf-8")
-        written.append(path)
-
-    return f"Written {len(written)} files: {', '.join(written)}"

openkb/cli.py

@@ -334,9 +334,11 @@ def on_new_files(paths):
 
 
 @cli.command()
-@click.option("--fix", is_flag=True, default=False, help="Automatically fix lint issues.")  # TODO: --fix not yet implemented
+@click.option("--fix", is_flag=True, default=False, help="Automatically fix lint issues (not yet implemented).")
 def lint(fix):
     """Lint the knowledge base for structural and semantic inconsistencies."""
+    if fix:
+        click.echo("Warning: --fix is not yet implemented. Running lint in report-only mode.")
     kb_dir = _find_kb_dir()
     if kb_dir is None:
         click.echo("No knowledge base found. Run `openkb init` first.")

openkb/images.py

@@ -171,7 +171,10 @@ def copy_relative_images(
 
     for match in _RELATIVE_RE.finditer(markdown):
         alt, rel_path = match.group(1), match.group(2)
-        src = source_dir / rel_path
+        src = (source_dir / rel_path).resolve()
+        if not src.is_relative_to(source_dir.resolve()):
+            logger.warning("Image path escapes source dir: %s; skipping.", rel_path)
+            continue
         if not src.exists():
             logger.warning(
                 "Relative image not found: %s; leaving original link.", src

openkb/watcher.py

@@ -37,11 +37,12 @@ def __init__(self, callback: Callable[[list[str]], None], debounce_seconds: floa
 
     def _schedule_flush(self) -> None:
         """Cancel any existing timer and start a fresh debounce timer."""
-        if self._timer is not None:
-            self._timer.cancel()
-        self._timer = threading.Timer(self._debounce_seconds, self._flush)
-        self._timer.daemon = True
-        self._timer.start()
+        with self._lock:
+            if self._timer is not None:
+                self._timer.cancel()
+            self._timer = threading.Timer(self._debounce_seconds, self._flush)
+            self._timer.daemon = True
+            self._timer.start()
 
     def _flush(self) -> None:
         """Call the callback with all collected pending paths, then clear."""

pyproject.toml

@@ -27,7 +27,7 @@ classifiers = [
 keywords = ["ai", "rag", "retrieval", "knowledge-base", "llm", "pageindex", "agents", "document"]
 dependencies = [
     "pageindex==0.3.0.dev0",
-    "markitdown",
+    "markitdown[all]",
     "click>=8.0",
     "watchdog>=3.0",
     "litellm",

tests/test_converter.py

@@ -81,27 +81,24 @@ def test_md_raw_file_copied(self, kb_dir):
 
 
 class TestConvertDocumentPdfShort:
-    def test_short_pdf_converted_via_markitdown(self, kb_dir, tmp_path):
-        """PDF under threshold is converted with markitdown."""
+    def test_short_pdf_converted_via_pymupdf(self, kb_dir, tmp_path):
+        """PDF under threshold is converted with pymupdf (convert_pdf_with_images)."""
         src = tmp_path / "short.pdf"
         src.write_bytes(b"%PDF-1.4 fake content")
 
-        fake_result = MagicMock()
-        fake_result.text_content = "# Short PDF\n\nConverted content."
-
         with (
             patch("openkb.converter.pymupdf.open") as mock_mu,
-            patch("openkb.converter.MarkItDown") as mock_mid_cls,
+            patch("openkb.converter.convert_pdf_with_images", return_value="# Short PDF\n\nConverted.") as mock_cpwi,
         ):
             fake_doc = MagicMock()
             fake_doc.page_count = 5  # below default threshold of 20
             fake_doc.__enter__ = MagicMock(return_value=fake_doc)
             fake_doc.__exit__ = MagicMock(return_value=False)
             mock_mu.return_value = fake_doc
-            mock_mid_cls.return_value.convert.return_value = fake_result
 
             result = convert_document(src, kb_dir)
 
+        mock_cpwi.assert_called_once()
         assert result.skipped is False
         assert result.is_long_doc is False
         assert result.source_path is not None

tests/test_indexer.py

@@ -95,10 +95,11 @@ def test_localclient_called_with_index_config(self, kb_dir, sample_tree, tmp_pat
         with patch("openkb.indexer.PageIndexClient", return_value=fake_client) as mock_cls:
             index_long_document(pdf_path, kb_dir)
 
-        # Verify PageIndexClient was instantiated
+        # Verify PageIndexClient was instantiated with correct IndexConfig
         mock_cls.assert_called_once()
-        # Check that index_config with correct flags was passed
         _, kwargs = mock_cls.call_args
-        ic = kwargs.get("index_config") or mock_cls.call_args[0][0] if mock_cls.call_args[0] else None
-        # Either as positional or keyword — either way PageIndexClient was called
-        assert mock_cls.called
+        ic = kwargs.get("index_config")
+        assert ic is not None, "index_config must be passed to PageIndexClient"
+        assert ic.if_add_node_text is True
+        assert ic.if_add_node_summary is True
+        assert ic.if_add_doc_description is True