Add standalone MCP docs source discovery

[rmusser01]

Summary

• Adds disabled-by-default standalone MCP docs source discovery settings, status, request models, and docs.discover_source provider/host exposure.
• Implements bounded sitemap and page-link discovery with dry-run, register, ingest, and register_and_ingest flows, including policy enforcement, redaction, dedupe, and metadata propagation.
• Adds url_sitemap sync_source refresh support and hardens sitemap XML parsing with defusedxml.

Test Plan

• /Users/macbook-dev/Documents/GitHub/tldw_server2/.venv/bin/python -m pytest tldw_Server_API/tests/MCP_unified/docs -q -> 295 passed, 4 warnings
• /Users/macbook-dev/Documents/GitHub/tldw_server2/.venv/bin/python -m bandit -r apps/mcp-unified/src/mcp_unified/docs tldw_Server_API/tests/MCP_unified/docs -f json -o /tmp/bandit_mcp_docs_stage4b_source_discovery.json -> 0 findings
• /Users/macbook-dev/Documents/GitHub/tldw_server2/.venv/bin/python -m pytest tldw_Server_API/tests/MCP_unified/docs/test_docs_import_boundaries.py -q --tb=short -> 6 passed
• git diff --check -> clean

Change summary

This PR adds a local-docs/source-discovery layer for the standalone MCP docs corpus without coupling it to tldw_server runtime services. Discovery is opt-in and bounded by config so locked-down deployments remain safe by default. The implementation reuses the existing URL policy/fetch/acquisition seams, keeps BeautifulSoup/trafilatura optional via lazy imports/fallbacks, and uses SQLite-backed docs/source records so agents can discover, register, ingest, and refresh general document collections for FTS/RAG workflows.

---

Summary by cubic

Adds standalone, opt-in MCP docs source discovery via docs.discover_source. Enables bounded sitemap and page-link discovery with dry-run/apply, integrates url_sitemap sync refresh and apply-time ingestion, and hardens XML parsing.

• New Features

  • Added discovery settings, request/response models, service, and docs.discover_source provider/host exposure.
  • Supports auto/sitemap/page_links; dry_run/apply; apply actions register/ingest/register_and_ingest; bounded by max_discovery_pages, max_discovery_depth, max_discovery_sitemaps.
  • Enforces URL policy (allowed prefixes, same-origin), redaction, and dedupe; propagates metadata (collections, keywords, title).
  • Integrates with docs.sync_source for url_sitemap refresh and apply-time ingestion; keeps beautifulsoup4/trafilatura optional via lazy imports; SQLite-backed records.
  • Safe defaults: discovery disabled; low limits; discovery_apply_default is register.

• Bug Fixes

  • Hardened sitemap XML parsing with defusedxml; blocks DOCTYPE/unsafe constructs and applies strict size bounds.

^{Written for commit 5dc8ac6. Summary will update on new commits.}

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Repository UI

Review profile: ASSERTIVE

Plan: Pro

Run ID: 09cd5d53-99cc-4724-aa36-e10be0d9e555

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch codex/mcp-docs-source-discovery-design

---

_{Comment @coderabbitai help to get the list of available commands.}

[qodo-code-review]

PR Summary by Qodo

Add standalone MCP docs source discovery (bounded sitemap/page-link)

✨ Enhancement 🧪 Tests 📝 Documentation ⚙️ Configuration changes 🕐 40+ Minutes

AI Description

• Add disabled-by-default docs source discovery settings and request model.
• Implement bounded sitemap/page-link discovery with dry-run and apply flows.
• Enable url_sitemap source sync refresh and harden sitemap XML parsing.

Diagram

graph TD
  A["MCP Tool Provider"] --> B["DocsSourceDiscoveryService"] --> C["URLFetcher"] --> D{{"Web Source Policy"}}
  B --> E["DocsAcquisitionService"] --> F[("DocsCatalogStore")]
  A --> G["DocsSourceSyncService"] --> F
  G --> B
  subgraph Legend
    direction LR
    _svc(["Service"]) ~~~ _db[("SQLite Store")] ~~~ _ext{{"Policy/Guard"}}
  end

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Reuse existing crawler/sitemap library

• ➕ Less bespoke parsing/link-extraction code to maintain
• ➕ Potentially broader sitemap compatibility (indexes, gzip, etc.)
• ➖ Adds heavier dependencies to the standalone MCP footprint
• ➖ Harder to enforce the project’s strict policy/redaction/limits consistently
• ➖ May undermine the 'bounded, not a crawler' goal

2. Register-only + rely on sync_source for ingestion

• ➕ Simpler discover_source tool (no ingestion side effects)
• ➕ Keeps ingestion behavior centralized in sync pipeline
• ➖ More steps for agents/users (discover → register → sync)
• ➖ Harder to support one-shot 'register_and_ingest' workflows for small collections

3. Allow sitemapindex traversal (bounded)

• ➕ Supports common production sitemaps split across multiple files
• ➕ Better coverage for larger documentation sites
• ➖ Increases complexity and attack surface (more fetches)
• ➖ Makes bounding and safe defaults harder to reason about

Recommendation: The PR’s approach (disabled-by-default, bounded discovery that reuses existing fetch/policy seams and optionally registers/ingests) is appropriate for standalone MCP safety goals. The main alternative worth revisiting later is bounded sitemapindex traversal, but only if real sites require it and strict fetch-count limits are enforced.

Files changed (16) +4036 / -3

Enhancement (5) +1135 / -2

__init__.pyExport DiscoverSourceRequest from docs package +2/-0

Export DiscoverSourceRequest from docs package

• Adds DiscoverSourceRequest to the public module exports so callers can import it from mcp_unified.docs.

apps/mcp-unified/src/mcp_unified/docs/init.py

discovery.pyImplement bounded sitemap/page-link discovery service +656/-0

Implement bounded sitemap/page-link discovery service

• Adds DocsSourceDiscoveryService with dry-run/apply modes, candidate filtering (policy, same-origin, query redaction, dedupe, bounds), sitemap parsing via defusedxml, and optional BeautifulSoup link extraction with a safe fallback HTML parser. Supports apply actions to register url_sitemap sources and/or ingest accepted candidates while linking them back to the source.

apps/mcp-unified/src/mcp_unified/docs/discovery.py

mcp_module.pyExpose docs.discover_source tool and status in MCP provider +203/-2

Expose docs.discover_source tool and status in MCP provider

• Wires DocsSourceDiscoveryService into DocsMCPToolProvider when enabled, adds docs.discover_source to tool definitions, validates/normalizes arguments into DiscoverSourceRequest, and extends docs.status with a source_discovery section describing availability and limits.

apps/mcp-unified/src/mcp_unified/docs/mcp_module.py

models.pyAdd DiscoverSourceRequest and discovery type aliases +18/-0

Add DiscoverSourceRequest and discovery type aliases

• Introduces discovery-related Literal types and a DiscoverSourceRequest dataclass capturing kind/mode/apply action and bounding/metadata fields.

apps/mcp-unified/src/mcp_unified/docs/models.py

sync.pyAdd url_sitemap sync support via discovery candidates +256/-0

Add url_sitemap sync support via discovery candidates

• Extends source sync to handle url_sitemap sources by discovering current sitemap candidates, ingesting/linking accepted pages, and tombstoning missing links only when safe (avoids tombstoning when discovery is truncated). Adds helper functions for sitemap item formatting and page limit calculation.

apps/mcp-unified/src/mcp_unified/docs/sync.py

Tests (5) +787 / -1

test_docs_mcp_provider.pyTest discover_source tool exposure and provider delegation +114/-1

Test discover_source tool exposure and provider delegation

• Adds tests ensuring docs.discover_source is advertised only when enabled, denies calls when disabled, delegates correctly to the discovery service, and reports discovery status in docs.status.

tldw_Server_API/tests/MCP_unified/docs/test_docs_mcp_provider.py

test_docs_module_shim.pyTest host/module shim exposes discover_source when enabled +23/-0

Test host/module shim exposes discover_source when enabled

• Adds an async shim test verifying the DocsModule exposes docs.discover_source based on module settings; also asserts repo config keeps discovery disabled by default.

tldw_Server_API/tests/MCP_unified/docs/test_docs_module_shim.py

test_docs_settings.pyTest source discovery settings defaults and validation +37/-0

Test source discovery settings defaults and validation

• Adds tests for safe default discovery settings, parsing of configured discovery values, and rejection of invalid discovery_apply_default values.

tldw_Server_API/tests/MCP_unified/docs/test_docs_settings.py

test_docs_source_discovery.pyAdd unit tests for discovery service and parsers +334/-0

Add unit tests for discovery service and parsers

• Introduces comprehensive tests for sitemap parsing hardening (doctype/entity), candidate bounding, link extraction behavior, redaction, policy/scope filtering, dry-run vs apply behavior, and register/register_and_ingest flows.

tldw_Server_API/tests/MCP_unified/docs/test_docs_source_discovery.py

test_docs_source_sync.pyTest url_sitemap sync refresh and tombstone behavior +279/-0

Test url_sitemap sync refresh and tombstone behavior

• Adds tests for syncing url_sitemap sources in apply/dry-run modes, ingestion/linking behavior, tombstoning missing links only in apply, and avoiding tombstones when sitemap results are truncated by limits.

tldw_Server_API/tests/MCP_unified/docs/test_docs_source_sync.py

Documentation (5) +2076 / -0

2026-07-03-standalone-mcp-docs-stage4b-source-discovery-implementation-plan.mdAdd implementation plan for bounded docs source discovery +1361/-0

Add implementation plan for bounded docs source discovery

• Introduces a detailed Stage 4B implementation plan covering settings, tool surface, discovery flows, bounds, and testing strategy.

Docs/superpowers/plans/2026-07-03-standalone-mcp-docs-stage4b-source-discovery-implementation-plan.md

2026-07-03-standalone-mcp-docs-stage4b-source-discovery-design.mdAdd design spec for bounded source discovery +503/-0

Add design spec for bounded source discovery

• Adds the design for a non-crawling, bounded discovery feature (sitemap/seed page) including goals, non-goals, and safety constraints.

Docs/superpowers/specs/2026-07-03-standalone-mcp-docs-stage4b-source-discovery-design.md

task-12121 - Design-standalone-MCP-docs-Stage-4B-bounded-source-discovery.mdAdd backlog task for discovery design +71/-0

Add backlog task for discovery design

• Adds tracking task documenting the Stage 4B bounded source discovery design work.

backlog/tasks/task-12121 - Design-standalone-MCP-docs-Stage-4B-bounded-source-discovery.md

task-12122 - Plan-standalone-MCP-docs-Stage-4B-source-discovery-implementation.mdAdd backlog task for discovery implementation plan +66/-0

Add backlog task for discovery implementation plan

• Adds tracking task for the Stage 4B implementation planning phase and deliverables.

backlog/tasks/task-12122 - Plan-standalone-MCP-docs-Stage-4B-source-discovery-implementation.md

task-12124 - Implement-standalone-MCP-docs-Stage-4B-source-discovery.mdAdd backlog task for discovery implementation +75/-0

Add backlog task for discovery implementation

• Adds tracking task for implementing the Stage 4B discovery feature and associated test coverage.

backlog/tasks/task-12124 - Implement-standalone-MCP-docs-Stage-4B-source-discovery.md

Other (1) +38 / -0

settings.pyAdd disabled-by-default source discovery settings and coercion +38/-0

Add disabled-by-default source discovery settings and coercion

• Extends DocsSettings with enable_source_discovery and discovery bounds/default apply behavior; includes validation/coercion for discovery_apply_default and related numeric/bool fields.

apps/mcp-unified/src/mcp_unified/docs/settings.py

[gemini-code-assist]

Code Review

This pull request implements Stage 4B bounded source discovery for the standalone MCP docs corpus, introducing the DocsSourceDiscoveryService to support sitemap and page-link discovery, adding settings and models, wiring up the docs.discover_source MCP tool, and extending docs.sync_source to refresh url_sitemap sources. The review feedback highlights several opportunities to improve robustness and error handling, including safely handling non-string XML tags, catching ValueError on malformed URL ports, checking for None on retrieved documents to prevent AttributeError crashes, and properly propagating sitemap parsing failures rather than returning a successful status with empty candidates.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (0) 📜 Skill insights (0)

Context used

✅ Compliance rules (platform): 74 rules

1. _source_discovery_status missing docstring ✓ Resolved 📘 Rule violation ✧ Quality

Description

The newly added _source_discovery_status() function has no docstring. This violates the
requirement that new functions include docstrings.

Code

apps/mcp-unified/src/mcp_unified/docs/mcp_module.py[R64-80]

+def _source_discovery_status(settings: DocsSettings) -> dict[str, Any]:
+    enabled = settings.enable_source_discovery
+    available = enabled and settings.enable_web_acquisition
+    disabled_reason = None if available else (
+        "web_acquisition_disabled" if enabled else "source_discovery_disabled"
+    )
+    return {
+        "enabled": enabled,
+        "available": available,
+        "disabled_reason": disabled_reason,
+        "supported_kinds": ["sitemap", "page_links"],
+        "max_discovery_pages": settings.max_discovery_pages,
+        "max_discovery_depth": settings.max_discovery_depth,
+        "max_discovery_sitemaps": settings.max_discovery_sitemaps,
+        "discovery_apply_default": settings.discovery_apply_default,
+        "discovery_same_origin_only": settings.discovery_same_origin_only,
+    }

Evidence

The docstring rule requires each function to have a docstring as its first statement.
_source_discovery_status() is newly added and its body starts with code (enabled = ...) rather
than a docstring.

Rule 224214: Require docstrings for all modules, classes, and functions
apps/mcp-unified/src/mcp_unified/docs/mcp_module.py[64-80]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_source_discovery_status()` was introduced without a docstring.

## Issue Context
Compliance requires docstrings for all new/modified functions.

## Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/mcp_module.py[64-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. discovery.py missing docstrings ✓ Resolved 📘 Rule violation ✧ Quality

Description

The new mcp_unified.docs.discovery module defines multiple public classes/functions but provides
no module, class, or function docstrings. This reduces maintainability and violates the docstring
requirement for new code.

Code

apps/mcp-unified/src/mcp_unified/docs/discovery.py[R1-75]

+from __future__ import annotations
+
+import importlib
+from collections.abc import Mapping
+from dataclasses import dataclass, replace
+from html.parser import HTMLParser
+from typing import Any, Literal
+from urllib.parse import urljoin, urlsplit, urlunsplit
+
+from defusedxml import ElementTree
+from .acquisition.fetcher import URLFetcher
+from .acquisition.service import DocsAcquisitionService
+from .acquisition.policy import safe_argument_hash
+from .acquisition.policy import SourcePolicy
+from .models import AccessScope, DiscoverSourceRequest
+from .settings import DocsSettings
+from .source_utils import redacted_url_for_display, source_defaults_metadata, url_has_query
+from .store.sqlite import DocsCatalogStore
+
+CandidateStatus = Literal["accepted", "duplicate", "denied", "skipped", "ingested", "failed"]
+_SITEMAP_CONTENT_TYPES = ("application/xml", "text/xml", "application/xhtml+xml")
+
+
+@dataclass(frozen=True)
+class DiscoveredURLCandidate:
+    url: str
+    display_url: str
+    status: CandidateStatus
+    reason_code: str
+    source_kind: str
+    parent_url: str
+    parent_display_url: str
+    safe_argument_hash: str
+    document_id: int | None = None
+
+
+@dataclass(frozen=True)
+class SitemapParseResult:
+    status: str
+    reason_code: str
+    candidates: list[DiscoveredURLCandidate]
+    skipped: int = 0
+
+
+class DocsSourceDiscoveryService:
+    def __init__(
+        self,
+        *,
+        settings: DocsSettings,
+        store: DocsCatalogStore,
+        resolver: object | None = None,
+        transport: object | None = None,
+    ) -> None:
+        self.settings = settings
+        self.store = store
+        self.policy = SourcePolicy(
+            web_source_profile=settings.web_source_profile,
+            preapproved_domains=settings.preapproved_domains,
+            allowed_url_prefixes=settings.allowed_url_prefixes,
+            denied_domains=settings.denied_domains,
+            allow_arbitrary_public_domains=settings.allow_arbitrary_public_domains,
+        )
+        fetch_settings = replace(
+            settings,
+            allowed_content_types=_merged_content_types(settings.allowed_content_types, _SITEMAP_CONTENT_TYPES),
+        )
+        self.fetcher = URLFetcher(settings=fetch_settings, policy=self.policy, resolver=resolver, transport=transport)
+        self.acquisition = DocsAcquisitionService(settings=settings, store=store, resolver=resolver, transport=transport)
+
+    def discover_source(self, *, scope: AccessScope, request: DiscoverSourceRequest) -> dict[str, Any]:
+        if not self.settings.enable_source_discovery:
+            return _discovery_response(
+                status="denied",
+                reason_code="source_discovery_disabled",
+                request=request,

Evidence

The docstring rule requires a module docstring as the first statement and docstrings for
classes/functions. In discovery.py, the file begins with imports (no module docstring) and new
classes like DiscoveredURLCandidate are defined without docstrings.

Rule 224214: Require docstrings for all modules, classes, and functions
apps/mcp-unified/src/mcp_unified/docs/discovery.py[1-75]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`apps/mcp-unified/src/mcp_unified/docs/discovery.py` was added without required docstrings (module docstring at top, plus docstrings for classes and functions/methods).

## Issue Context
Compliance requires docstrings for all modules, classes, and functions in new/changed Python code.

## Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/discovery.py[1-120]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Sitemap sources unsyncable 🐞 Bug ≡ Correctness

Description

_register_sitemap_source stores source_url=None when the sitemap URL has a query string and
persist_url_query_strings is false, but _sync_url_sitemap requires source_url and fails with
source_url_missing, making the registered sitemap source impossible to refresh.

Code

apps/mcp-unified/src/mcp_unified/docs/discovery.py[R231-252]

+    def _register_sitemap_source(self, *, scope: AccessScope, request: DiscoverSourceRequest) -> dict[str, Any]:
+        can_persist_query_uri = self.settings.persist_url_query_strings or not url_has_query(request.url)
+        canonical_uri = request.url if can_persist_query_uri else redacted_url_for_display(request.url)
+        redacted_source_url = redacted_url_for_display(request.url)
+        metadata: dict[str, Any] = source_defaults_metadata(
+            keywords=request.keywords,
+            collection_names=request.collections,
+        )
+        metadata.update(
+            {
+                "discovery_kind": "sitemap",
+                "same_origin_only": self.settings.discovery_same_origin_only,
+            }
+        )
+        source_id = self.store.upsert_source(
+            scope=scope,
+            source_type="url_sitemap",
+            canonical_uri=canonical_uri,
+            display_name=request.title or redacted_source_url,
+            source_path=None,
+            source_url=request.url if can_persist_query_uri else None,
+            redacted_source_url=redacted_source_url,

Evidence

Discovery registration explicitly sets source_url to None for non-persisted query URLs, and the
sitemap sync code immediately fails when source_url is empty, proving this registered source type
cannot be synced.

apps/mcp-unified/src/mcp_unified/docs/discovery.py[231-252]
apps/mcp-unified/src/mcp_unified/docs/sync.py[522-544]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Sitemap sources can be created without a persisted `source_url` (when the seed URL contains a query string and query persistence is disabled). The sync path for `url_sitemap` requires `source_url` and hard-fails, so the newly created source cannot be refreshed.

### Issue Context
This is a lifecycle mismatch between discovery registration and sync execution.

### Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/discovery.py[231-256]
- apps/mcp-unified/src/mcp_unified/docs/sync.py[522-544]

### Implementation notes
Pick one consistent behavior:
- **Preferred:** If `url_has_query(request.url)` and `persist_url_query_strings` is false, deny the `register` / `register_and_ingest` action up-front with a clear reason_code (so no unusable source is created).
- Alternative: persist a canonicalized sync URL (without query) **only if** that is acceptable semantically for sitemaps; otherwise it will fetch the wrong resource.
- Optionally harden sync: if `source_url` is missing, attempt a safe fallback to `canonical_uri` only when it is a valid fetchable URL and does not violate persistence policy.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View more (5)

4. Test module lacks docstring ✗ Dismissed 📘 Rule violation ✧ Quality

Description

The new test module test_docs_source_discovery.py starts with imports and provides no module
docstring (and tests have no function docstrings). This violates the requirement for docstrings on
modules and functions in new files.

Code

tldw_Server_API/tests/MCP_unified/docs/test_docs_source_discovery.py[R1-35]

+from __future__ import annotations
+
+import importlib
+import json
+from pathlib import Path
+
+import pytest
+
+from mcp_unified.docs.acquisition.models import FetchResponse
+from mcp_unified.docs.discovery import (
+    DiscoveredURLCandidate,
+    DocsSourceDiscoveryService,
+    extract_page_links,
+    parse_sitemap_urlset,
+    public_candidate,
+)
+from mcp_unified.docs.models import AccessScope, DiscoverSourceRequest
+from mcp_unified.docs.settings import DocsSettings
+from mcp_unified.docs.store.sqlite import DocsCatalogStore
+from tldw_Server_API.tests.MCP_unified.docs.helpers import FakeResolver, FakeTransport
+
+pytestmark = pytest.mark.unit
+
+
+def test_parse_sitemap_urlset_rejects_doctype() -> None:
+    body = b'<!DOCTYPE foo [<!ENTITY xxe SYSTEM "file:///etc/passwd">]><urlset />'
+
+    result = parse_sitemap_urlset(body, max_pages=10)
+
+    assert result.reason_code == "sitemap_xml_forbidden_doctype"  # nosec B101
+    assert result.candidates == []  # nosec B101
+
+
+def test_parse_sitemap_urlset_enforces_page_limit_before_candidates() -> None:
+    body = b"""

Evidence

The docstring requirement applies to new Python modules and functions. This new test file begins
with imports (no module docstring) and introduces new test functions whose bodies begin with code
rather than docstrings.

Rule 224214: Require docstrings for all modules, classes, and functions
tldw_Server_API/tests/MCP_unified/docs/test_docs_source_discovery.py[1-35]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new test module and its test functions were added without required docstrings.

## Issue Context
Compliance requires docstrings for modules and functions in new/changed Python files.

## Fix Focus Areas
- tldw_Server_API/tests/MCP_unified/docs/test_docs_source_discovery.py[1-35]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. _coerce_discovery_apply_default missing docstring ✓ Resolved 📘 Rule violation ✧ Quality

Description

The newly added _coerce_discovery_apply_default() function has no docstring. This violates the
requirement that new functions include docstrings.

Code

apps/mcp-unified/src/mcp_unified/docs/settings.py[R126-131]

+def _coerce_discovery_apply_default(value: object, field_name: str) -> DiscoveryApplyDefault:
+    text = "register" if value is None else str(value).strip().lower()
+    if text not in {"register", "ingest", "register_and_ingest"}:
+        raise ValueError(f"{field_name} must be register, ingest, or register_and_ingest")
+    return cast(DiscoveryApplyDefault, text)
+

Evidence

The checklist requires that every function definition has a docstring as the first statement.
_coerce_discovery_apply_default() starts immediately with text = ... and has no docstring.

Rule 224214: Require docstrings for all modules, classes, and functions
apps/mcp-unified/src/mcp_unified/docs/settings.py[126-131]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`_coerce_discovery_apply_default()` was added without a function docstring.

## Issue Context
Compliance requires docstrings for new functions.

## Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/settings.py[126-131]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

6. Uncaught defusedxml exceptions ✓ Resolved 🐞 Bug ☼ Reliability

Description

parse_sitemap_urlset only catches ElementTree.ParseError, but defusedxml can raise
DefusedXmlException subclasses (e.g., DTD/Entities forbidden), which will bubble up and crash
discovery/sync instead of returning a controlled failure response.

Code

apps/mcp-unified/src/mcp_unified/docs/discovery.py[R339-348]

+def parse_sitemap_urlset(body: bytes, *, max_pages: int, parent_url: str = "") -> SitemapParseResult:
+    upper = body[:4096].upper()
+    if b"<!DOCTYPE" in upper:
+        return SitemapParseResult(status="denied", reason_code="sitemap_xml_forbidden_doctype", candidates=[])
+    if b"<!ENTITY" in upper:
+        return SitemapParseResult(status="denied", reason_code="sitemap_xml_forbidden_entity", candidates=[])
+    try:
+        root = ElementTree.fromstring(body)
+    except ElementTree.ParseError:
+        return SitemapParseResult(status="failed", reason_code="sitemap_parse_failed", candidates=[])

Evidence

The new sitemap parser only catches ParseError around defusedxml parsing, while other parts of the
repo explicitly catch DefusedXmlException as well; this indicates defusedxml exceptions are expected
and should be handled to avoid crashes.

apps/mcp-unified/src/mcp_unified/docs/discovery.py[339-348]
tldw_Server_API/app/services/xml_processing_service.py[14-56]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`parse_sitemap_urlset()` uses `defusedxml.ElementTree.fromstring()` but only handles `ParseError`. Defusedxml can also raise `DefusedXmlException` (and related subclasses), which currently will propagate and fail the entire tool call.

### Issue Context
The repo already uses the standard pattern `except (ParseError, DefusedXmlException)` elsewhere, but the new discovery sitemap parser does not.

### Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/discovery.py[339-355]

### Implementation notes
- Import `DefusedXmlException` from `defusedxml.common` and catch it alongside `ElementTree.ParseError`.
- Map forbidden/defused cases to a stable `status`/`reason_code` (e.g., `denied` with a specific reason) rather than throwing.
- Consider keeping (or removing) the manual `DOCTYPE`/`ENTITY` scan, but do not rely on it for safety.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

7. DiscoverSourceRequest lacks docstring ✓ Resolved 📘 Rule violation ✧ Quality

Description

The newly added DiscoverSourceRequest dataclass is missing a class docstring. This violates the
requirement that new classes include docstrings.

Code

apps/mcp-unified/src/mcp_unified/docs/models.py[R98-110]

+@dataclass(frozen=True)
+class DiscoverSourceRequest:
+    url: str
+    kind: DiscoveryKind = "auto"
+    mode: DiscoveryMode = "dry_run"
+    apply_action: DiscoveryApplyAction | None = None
+    max_pages: int | None = None
+    max_depth: int | None = None
+    collections: tuple[str, ...] = ()
+    keywords: tuple[str, ...] = ()
+    title: str | None = None
+    include_seed: bool = False
+

Evidence

The checklist requires every class definition to have an immediately following string literal
docstring. The added DiscoverSourceRequest class body begins directly with field definitions and
contains no docstring.

Rule 224214: Require docstrings for all modules, classes, and functions
apps/mcp-unified/src/mcp_unified/docs/models.py[98-110]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The new `DiscoverSourceRequest` dataclass has no class docstring.

## Issue Context
Compliance requires docstrings for all new classes.

## Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/models.py[98-110]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

8. fake_import missing return type ✓ Resolved 📘 Rule violation ✧ Quality

Description

The new nested helper fake_import lacks an explicit return type annotation. This violates the
requirement that all function parameters and return values be type hinted.

Code

tldw_Server_API/tests/MCP_unified/docs/test_docs_source_discovery.py[R57-61]

+    def fake_import(name: str, package: str | None = None):
+        if name == "bs4":
+            seen.append(name)
+        return original_import(name, package)
+

Evidence

The type-hints rule requires every function to declare an explicit return type. The newly added
fake_import(...) function has annotated parameters but no -> ... return annotation.

Rule 224215: Require type hints on all function parameters and return values
tldw_Server_API/tests/MCP_unified/docs/test_docs_source_discovery.py[53-61]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The nested function `fake_import` is missing a return type annotation.

## Issue Context
Compliance requires explicit type hints on all function parameters and return values in new/changed code.

## Fix Focus Areas
- tldw_Server_API/tests/MCP_unified/docs/test_docs_source_discovery.py[57-61]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

9. Sitemap sync reason lost ✓ Resolved 🐞 Bug ◔ Observability

Description

When sitemap candidates are not accepted (denied/duplicate/skipped), _sync_url_sitemap emits items
with status=skipped and the default reason_code="ok", losing the underlying candidate.reason_code
and making failures hard to diagnose.

Code

apps/mcp-unified/src/mcp_unified/docs/sync.py[R593-597]

+        for candidate in candidates:
+            if candidate.status != "accepted":
+                counts["skipped"] += 1
+                items.append(_sitemap_candidate_item(candidate, status="skipped"))
+                continue

Evidence

The loop appends non-accepted candidates using _sitemap_candidate_item without passing
reason_code, and the helper defaults reason_code to "ok", proving the real skip/deny reasons are
dropped.

apps/mcp-unified/src/mcp_unified/docs/sync.py[593-597]
apps/mcp-unified/src/mcp_unified/docs/sync.py[971-986]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
For non-accepted sitemap candidates, sync output always uses `_sitemap_candidate_item(..., reason_code='ok')` via default, masking why an entry was denied/duplicate/skipped.

### Issue Context
Discovery computes precise `candidate.reason_code` values (e.g., out-of-scope, query-not-persisted, limit exceeded) that should be preserved in sync output.

### Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/sync.py[593-623]
- apps/mcp-unified/src/mcp_unified/docs/sync.py[971-989]

### Implementation notes
- Pass through `reason_code=candidate.reason_code` when emitting skipped items.
- Optionally consider emitting the original candidate status (duplicate/denied) instead of collapsing everything to `skipped`, if the API contract allows it.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

10. Long import line in mcp_module.py ✓ Resolved 📘 Rule violation ✧ Quality

Description

The modified from .models import ... statement is overly long and violates PEP 8 formatting
expectations for imports/line length. This can cause formatter/linter warnings in the project
configuration.

Code

apps/mcp-unified/src/mcp_unified/docs/mcp_module.py[11]

+from .models import AccessScope, ContextRequest, DiscoverSourceRequest, SearchFilters, SearchRequest, SyncSourceRequest

Evidence

The PEP 8 rule requires imports and line length to conform to the configured limit. The changed
import line aggregates many names on a single line, which is a common formatter/linter violation.

Rule 224211: Enforce PEP 8 formatting for Python code
apps/mcp-unified/src/mcp_unified/docs/mcp_module.py[9-13]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
A modified import statement appears to exceed typical PEP 8 line-length limits and should be wrapped.

## Issue Context
Compliance requires no new PEP 8 violations in changed code.

## Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/mcp_module.py[11-11]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

11. Default-port origin mismatch ✓ Resolved 🐞 Bug ≡ Correctness

Description

Same-origin enforcement compares (scheme, hostname, port) from urlsplit, so https://example.com and
https://example.com:443 are treated as different origins and valid same-site candidates may be
incorrectly denied as candidate_out_of_scope.

Code

apps/mcp-unified/src/mcp_unified/docs/discovery.py[R483-484]

+        if settings.discovery_same_origin_only and _origin(candidate_url) != seed_origin:
+            filtered.append(_replace_candidate(candidate, candidate_url, display_url, "denied", "candidate_out_of_scope"))

Evidence

Discovery uses raw urlsplit().port in _origin() for same-origin checks, while URL normalization
elsewhere explicitly normalizes default ports to None; these differing behaviors can cause
same-origin URLs to be denied.

apps/mcp-unified/src/mcp_unified/docs/discovery.py[457-498]
apps/mcp-unified/src/mcp_unified/docs/discovery.py[595-597]
apps/mcp-unified/src/mcp_unified/docs/acquisition/policy.py[87-114]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
Origin comparison for `discovery_same_origin_only` uses `urlsplit().port` directly. This makes default-port URLs mismatch when one URL omits the port and the other specifies `:80`/`:443`.

### Issue Context
Other parts of the system normalize default ports away (port becomes `None`), but discovery origin checking does not.

### Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/discovery.py[457-498]
- apps/mcp-unified/src/mcp_unified/docs/discovery.py[595-597]

### Implementation notes
- Normalize ports before comparing: treat `None` and the scheme default port as equivalent.
- Consider reusing the existing URL normalization logic (`normalize_url`) to avoid subtle URL parsing inconsistencies.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

12. Discovery options non-functional ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

include_seed is parsed/exposed on docs.discover_source but never affects discovery output, and
max_depth is hard-rejected unless it equals settings.max_discovery_depth even though discovery does
not implement multi-depth traversal, making these knobs misleading to callers.

Code

apps/mcp-unified/src/mcp_unified/docs/mcp_module.py[R482-538]

+def _discover_source_request_from_args(
+    args: dict[str, Any],
+) -> tuple[DiscoverSourceRequest, dict[str, str] | None]:
+    url = _optional_str(args.get("url"))
+    if url is None:
+        return DiscoverSourceRequest(url=""), _invalid_discovery_request("url")
+    kind, error = _optional_discovery_choice(
+        args.get("kind"),
+        "kind",
+        default="auto",
+        allowed={"auto", "sitemap", "page_links"},
+    )
+    if error is not None:
+        return DiscoverSourceRequest(url=url), error
+    mode, error = _optional_discovery_choice(
+        args.get("mode"),
+        "mode",
+        default="dry_run",
+        allowed={"dry_run", "apply"},
+    )
+    if error is not None:
+        return DiscoverSourceRequest(url=url), error
+    apply_action, error = _optional_discovery_choice_or_none(
+        args.get("apply_action"),
+        "apply_action",
+        allowed={"register", "ingest", "register_and_ingest"},
+    )
+    if error is not None:
+        return DiscoverSourceRequest(url=url), error
+    max_pages, error = _optional_discovery_positive_int(args.get("max_pages"), "max_pages")
+    if error is not None:
+        return DiscoverSourceRequest(url=url), error
+    max_depth, error = _optional_discovery_positive_int(args.get("max_depth"), "max_depth")
+    if error is not None:
+        return DiscoverSourceRequest(url=url), error
+    collections, error = _optional_string_tuple(args.get("collections"), "collections")
+    if error is not None:
+        return DiscoverSourceRequest(url=url), error
+    keywords, error = _optional_string_tuple(args.get("keywords"), "keywords")
+    if error is not None:
+        return DiscoverSourceRequest(url=url), error
+    include_seed, error = _optional_discovery_bool(args.get("include_seed"), "include_seed", default=False)
+    if error is not None:
+        return DiscoverSourceRequest(url=url), error
+
+    return DiscoverSourceRequest(
+        url=url,
+        kind=kind,
+        mode=mode,
+        apply_action=apply_action,
+        max_pages=max_pages,
+        max_depth=max_depth,
+        collections=collections,
+        keywords=keywords,
+        title=_optional_str(args.get("title")),
+        include_seed=include_seed,
+    ), None

Evidence

The provider parses and forwards include_seed/max_depth into DiscoverSourceRequest, while discovery
validates max_depth in a way that prevents caller control and does not reference include_seed in its
execution path.

apps/mcp-unified/src/mcp_unified/docs/mcp_module.py[482-538]
apps/mcp-unified/src/mcp_unified/docs/models.py[98-110]
apps/mcp-unified/src/mcp_unified/docs/discovery.py[410-454]
apps/mcp-unified/src/mcp_unified/docs/discovery.py[70-133]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The `docs.discover_source` API surface includes `include_seed` and `max_depth`, but discovery behavior does not implement seed inclusion or depth traversal, and also rejects most `max_depth` values via validation.

### Issue Context
This creates an API contract mismatch: clients can set options that do nothing or unexpectedly error.

### Fix Focus Areas
- apps/mcp-unified/src/mcp_unified/docs/mcp_module.py[482-538]
- apps/mcp-unified/src/mcp_unified/docs/models.py[98-110]
- apps/mcp-unified/src/mcp_unified/docs/discovery.py[410-454]

### Implementation notes
- Either implement `include_seed` (prepend the seed URL as a candidate when true, with proper dedupe) or remove it from the request/tool schema.
- Change `max_depth` validation to match actual capabilities (e.g., allow `<= settings.max_discovery_depth` and deny only `>`, or remove until supported).
- If depth is intentionally unsupported for now, return a clearer reason_code (and/or document it in status) instead of requiring exact equality.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

[rmusser01]

Addressed the review feedback in e5a2c1b after rebasing on latest dev:

• Propagated sitemap parse failures from docs.discover_source instead of returning completed/ok.
• Caught defusedxml exceptions and hardened sitemap local-name handling.
• Normalized default ports in same-origin checks and kept malformed ports from crashing origin parsing.
• Denied register/register_and_ingest for query-bearing sitemap seeds when URL query persistence is disabled, avoiding unsyncable url_sitemap sources.
• Preserved sitemap skipped candidate reason codes in sync output.
• Added safe missing-document hash handling in discovery and sync linking.
• Made include_seed functional for page-link discovery and reports the actual one-hop depth cap.
• Wrapped the long import and added targeted docstrings/type annotations.

Verification:

• python -m pytest tldw_Server_API/tests/MCP_unified/docs -q -> 302 passed, 4 warnings
• python -m pytest tldw_Server_API/tests/MCP_unified/docs/test_docs_import_boundaries.py -q --tb=short -> 6 passed, 3 warnings
• Bandit on touched docs paths -> 0 findings
• git diff --check -> clean

I did not add docstrings to every pytest function; that is inconsistent with the existing test style and adds no useful safety.