feat(phase9 #100 D10.h): legacy search hard-cut + web_search canonicalization + retrieval handle exposure + D10 hurl coverage#1721
Merged
Conversation
…cy delete + web_search canonicalize + cursor narrative Per design pack §G D10.h + amendment-#2 + PM scope lock (msg=dc63c7e6): Block A — search_collection / search_chat_files hard-cut + caller sweep: - aperag/mcp/server.py: delete `search_collection` (line 295-465 ~170 LOC) + `search_chat_files` (468-583 ~116 LOC) bodies; rewrite the `aperag_usage_guide` resource docstring to document the canonical D10 split tools (vector_search / graph_search / fulltext_search / web_search) instead of the deprecated omnibus surface; replace the "deferred to D10.h cutover" NOTE comment with a closure-state note pointing at the split tools. - aperag/domains/agent_runtime/services.py: update `_KNOWLEDGE_SEARCH_TOOLS` from `{"list_collections", "search_collection"}` to the split-tool set `{list_collections, list_documents, vector_search, graph_search, fulltext_search}`; expand `_READING_TOOLS` to cover the 6 D10.c read primitives so user-activity routing tracks the post-cutover surface end-to-end. Block B — web_search §B.4 canonicalization: - aperag/mcp/tools/search_web.py: `query` is now positional + required (raises ValueError on empty); every other parameter is keyword-only; the result-count limit is named `top_k` (not `max_results`); `source` is `str | None` so a missing domain filter is null. The wrapper still passes `max_results` to the internal `/api/v2/web/search` payload — backend rename is intentionally out of this lane (PM constraint #2 msg=309b3ed3 "目标是 cutover contract 收口，不是重写 hurl 框架"). - tests/unit_test/mcp/test_search_split.py: replace the deferred-shape signature pin with `test_web_search_signature_matches_b4_canonical` — asserts query is required, the 4 other params are kw-only with the canonical names + defaults, and the legacy `max_results` parameter is gone. Block D — stale narrative cleanup: - aperag/mcp/cursor/invariants.py: drop the "(pending architect canonical lock per msg=441c5e56)" parenthetical now that #1710 has merged the canonical lock to main. Block C (retrieval `chunk_id` / `section_path` / `heading_anchor` propagation per Drift #1) is left for the architect to author on this same branch per msg=b9e7f91e cross-lane offer; block E (D10 hurl coverage suite) follows after C lands. #80 territory deliberately untouched (snapshot_assembler / RedisChatMessageHistory / StoredChatMessagePart / chat/history / utils/history) per PM msg=309b3ed3 #100/#80 disjoint lock.
4 new hurl files exercise the post-cutover MCP wire surface end-to- end; mirror 21_d10_capabilities's tools/list + substring-contains pattern (the wire is JSON-RPC framed inside SSE, so jsonpath would skip the envelope): - 22_d10_read_primitives.hurl — pin the §A.1-§A.8 input schema names for every paginated and metadata read primitive plus the ``read_document`` byte-range parameters and the ``read_document_chunk`` stable handle. - 23_d10_pagination.hurl — pin §C.5 PaginationParams (cursor + limit) inputs and PaginationResult (items + next_cursor + total_count) outputs on every paginated read primitive. - 24_d10_search_split.hurl — pin the §B.1-§B.4 split-search surface and the §B.4 canonical parameter set (top_k present, max_results intentionally absent). Includes assertions on chunk_id/section_path/heading_anchor that exercise the retrieval- propagation block authored by the architect on this same branch — these will fail until that commit lands, which is the intended cross-block gate. - 25_d10_cutover.hurl — cutover gate: every canonical D10 tool name is still on tools/list, the legacy search_collection / search_chat_files omnibus pair is gone. A future regression that re-registers either legacy tool fails CI loudly before merge. Per PM constraint #2 (msg=309b3ed3) the hurl coverage matches this hard-cut one-for-one — no infra refactor, no new transport, just substring-contains over the existing ``/mcp/`` mount.
…ding_anchor on SearchResultMetadata Per amendment-#2 Drift #1 (msg=ebfcdabe) + PM final scope lock (msg=dc63c7e6 / msg=5760999e) — D10.h cutover Block C: surface the 3 LOCKED §A.9 stable handle fields on the retrieval domain public allowlist so external Agents (Claude Code / Codex / Cursor) can navigate from search hits back to the canonical D10.c read primitives (``read_document_chunk(chunk_id)`` / ``read_document_section(section_path)``). ## Changes - ``aperag/domains/retrieval/schemas.py``: - ``SearchResultMetadata`` allowlist adds 3 fields (``chunk_id``, ``section_path``, ``heading_anchor``). The model keeps ``extra="forbid"`` — adding the fields does not relax the allowlist. - ``SearchResultMetadata.from_raw()`` extends extraction so the 3 fields propagate from upstream backends that already include them (e.g. ``aperag/domains/indexing/fulltext_index.py:541-553`` already surfaces ``chunk_id``). - ``tests/unit_test/domains/retrieval/test_search_result_metadata.py`` (NEW) — 7 tests pin the contract: 1. Each of the 3 fields is constructible. 2. Unknown keys still rejected (``extra="forbid"`` regression). 3. ``from_raw()`` extracts all 3 when present. 4. Missing fields surface as ``None`` (upstream propagation gap is not a schema break). 5. Non-string / empty values filtered so the public surface never carries a numeric chunk_id or empty string. ## Out of scope (per PM msg=5760999e #4 constraint) Indexing-layer attachment of ``section_path`` / ``heading_anchor`` to chunk metadata at index time is NOT included here. Per the constraint, multi-indexer expansion (vector / fulltext / graph / summary / vision each writing section context) would balloon the write-set; we keep D10.h Block C as a 1-2-touch retrieval-domain surface change. The 3 fields surface as ``None`` until the indexing-layer enhancement lands in a follow-up. ``chunk_id`` populates immediately via the existing fulltext index ``_source.chunk_id`` propagation (``fulltext_index.py:541-553``). ## §G hard gate compliance - #1 (3-root grep): no caller assertion drift on ``SearchResultMetadata`` shape outside the allowlist additions (allowlist additions are additive). - #5 (cross-stack): only ``aperag/domains/retrieval/schemas.py`` + ``tests/unit_test/domains/retrieval/`` touched on the architect side; Bryce's Block A/B/D commits cover the rest of D10.h scope. Block C complement to architect+Bryce co-own #100 (msg=a17a4017 execution split: architect commits Block C on shared branch).
… cutover Per §G hard gate #4 the cutover lane is the right place to update the test surface that previously pinned the legacy ``search_collection`` behaviour, so the assertions match the post-cutover reality: - tests/unit_test/mcp/test_search_split.py: drop the two ``[DEPRECATED]`` banner tests and the two body-still-targets-v2 tests; replace them with two cutover-removal tests (``test_search_collection_legacy_omnibus_removed_from_module`` / ``test_search_chat_files_legacy_omnibus_removed_from_module``) that pin both the runtime attribute absence and the absence of the ``async def`` in source. Inline the ``_async_def_source`` ast walk into the one remaining caller (``test_web_search_module_targets_v2_web_path``). - tests/unit_test/test_mcp_server.py: rename ``test_search_collection_docstring_explains_step_and_failure_meaning`` to ``test_search_collection_legacy_omnibus_no_longer_registered`` — the user-visible step language for the new split tools is covered in ``test_search_split.py`` so this file just pins the absence. - tests/unit_test/test_mcp_contract.py: rewrite the ``search_collection`` URL-and-import invariants as ``test_legacy_search_collection_omnibus_stays_removed`` + ``test_search_result_legacy_import_stays_gone``. The original tests guarded the Phase 2 retrieval hard-cut (URL must be v2, SearchResult must come from the retrieval domain); the post-cutover invariant is that neither symbol re-enters the server module at all. - tests/unit_test/agent_runtime/test_agent_runtime_v3.py: ``test_event_service_to_event_envelope_adds_user_activity_contract`` switches its sample tool name from the removed ``search_collection`` to ``vector_search`` so the user-activity inference contract is exercised against the canonical ``_KNOWLEDGE_SEARCH_TOOLS`` set updated in ``aperag/domains/agent_runtime/services.py``. 1001/1001 unit tests pass; ruff check + format clean.
…itive hurl gate + stale split-tool docstrings
Per Weston msg=8a691444:
1. tests/fixtures/mcp_agent.py
The ``searcher`` agent's instructions still pointed at the omnibus
``search_collection()`` call — a real instruction surface, not just
prose. After the cutover that tool is gone from MCP ``tools/list``
so the fixture would teach the agent to call a non-existent tool.
Migrate the instruction to the canonical D10 split-search flow:
``list_collections`` first, then compose ``vector_search`` /
``fulltext_search`` / ``graph_search`` per the question, and chain
into ``read_document_chunk`` / ``read_document_section`` via the
``chunk_id`` / ``section_path`` handles on each ``SearchResultItem``.
2. tests/e2e_http/hurl/full/24_d10_search_split.hurl
The three ``contains "\"chunk_id\""`` / ``"\"section_path\""`` /
``"\"heading_anchor\""`` assertions claimed to gate the
``SearchResultItem.metadata`` outputSchema, but the split-search
tools return ``Dict[str, Any]`` whose FastMCP outputSchema is
``additionalProperties: true`` — those substrings would actually
be matched by unrelated read-primitive input schemas (e.g.,
``read_document_chunk(chunk_id=...)``), making the hurl a
false-positive gate. Drop the three assertions and add a header
comment pointing at the proper Pydantic-level pin in
``tests/unit_test/domains/retrieval/test_search_result_metadata.py``
(which the architect's block C already authored).
3. aperag/mcp/tools/search_{vector,graph,fulltext}.py module docstrings
Each one still narrated the omnibus deprecation timeline ("the
alias remains until D10.h cutover") even though D10.h is the lane
that just deleted it. Rewrite the three module docstrings to
describe the post-cutover state: each split tool is the sole
public entry point for its recall mode, and the retrieval
``SearchResultMetadata`` allowlist now exposes the three §A.9
stable handle fields. Non-blocking per Weston, but cleaner to
land while we are touching this lane.
1001 unit tests pass; ruff check + format clean.
CI e2e-http-provider failed at ``22_d10_read_primitives.hurl:52`` with ``the HTTP method <body> is not valid``. The four new hurl files I added in block E left the body assertions hanging directly under ``HTTP 200`` without an ``[Asserts]`` block, so the hurl parser tried to read each ``body contains "..."`` line as the start of a new request (looking for an HTTP method like GET/POST/etc). The existing ``21_d10_capabilities.hurl`` (which my files were modeled on) does have ``[Asserts]`` after its final ``HTTP 200`` — I missed that line when copying the pattern. Add it to all four: - 22_d10_read_primitives.hurl - 23_d10_pagination.hurl - 24_d10_search_split.hurl - 25_d10_cutover.hurl Same hurl-only fix pattern as huangheng's #1719 follow-up (`379d2535`): no production code touched, only the hurl assertion framing.
…in 23_d10_pagination.hurl
CI run on head ``7d64991`` failed at
``23_d10_pagination.hurl:67`` and ``:68`` — same false-positive
pattern Weston already flagged for 24_d10_search_split.hurl
(msg=8a691444). The MCP tool functions in ``aperag/mcp/server.py``
are typed ``-> Dict[str, Any]``, so FastMCP exposes only
``"outputSchema": {"additionalProperties": true}`` on the
``tools/list`` wire — the ``items / next_cursor / total_count``
PaginationResult envelope field names never reach the body, and
``body contains "\\"next_cursor\\""`` / ``"\\"total_count\\""``
genuinely fail.
The PaginationParams input names (``cursor / limit``) DO surface
on the wire because they are inputSchema parameters; those four
substrings stay. Replaced the misleading "FastMCP emits the
Pydantic model JSON schema" comment with a header note pointing at
the cursor unit suite, which is where the envelope-shape pin
already lives (``tests/unit_test/mcp/test_cursor_contract.py`` +
``tests/unit_test/service/test_pagination_helper.py``).
Same hurl-only fix pattern as the previous ``[Asserts]`` push and
huangheng's #1719 ``379d2535`` follow-up — no production code
touched.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
D10.h cutover, single PR per design pack §G + amendment-#2 + PM final scope lock (msg=dc63c7e6 / msg=5760999e). The system is pre-launch with no users / no data per @earayu2 msg=9730bb6b, so the soak / data-migration gate that previously guarded
task #100is dropped — only the code-level gates (caller sweep, write-set boundary, CI green) apply.Write set
search_collection/search_chat_fileshard-cut + caller sweepaperag/mcp/server.py(delete two omnibus tools + rewrite theaperag_usage_guideresource),aperag/domains/agent_runtime/services.py(_KNOWLEDGE_SEARCH_TOOLS/_READING_TOOLS)web_search§B.4 canonicalizationaperag/mcp/tools/search_web.py(queryrequired, kw-only,top_k,source: str | None),tests/unit_test/mcp/test_search_split.py(signature pin)chunk_id/section_path/heading_anchorexposureaperag/domains/retrieval/schemas.py, newtests/unit_test/domains/retrieval/test_search_result_metadata.pybec7bde5)aperag/mcp/cursor/invariants.py(pending architect canonical lock per msg=441c5e56removed)tests/e2e_http/hurl/full/22_d10_read_primitives.hurl/23_d10_pagination.hurl/24_d10_search_split.hurl/25_d10_cutover.hurltests/unit_test/mcp/test_search_split.py,tests/unit_test/test_mcp_server.py,tests/unit_test/test_mcp_contract.py,tests/unit_test/agent_runtime/test_agent_runtime_v3.py§G hard gates
search_collection/search_chat_filesruntime references are gone everywhere except the agent-runtime translator string fixtures (which intentionally exercise routing on arbitrary tool-name strings) and asource_type: "search_collection"test fixture in chat history (D8.5-BE artifact, harmless string token). Both are non-functional and out of D10.h scope.mcp_server.search_collection/search_chat_filesas a runtime attribute is migrated in this same PR (services.py routing set, mcp_server / mcp_contract / agent_runtime_v3 tests).test_search_collection_legacy_omnibus_removed_from_module, etc).task #80territory deliberately untouched (snapshot_assembler,RedisChatMessageHistory,StoredChatMessagePart,chat/history,utils/history); architect-confirmed in inventory msg=b9e7f91e.Cross-block gate
24_d10_search_split.hurlassertschunk_id/section_path/heading_anchorappear on the wire. Those assertions were placed before block C landed so the gate would fail loudly if Drift #1 propagation didn't ship in the same PR — block C'sSearchResultMetadataallowlist update closes that gate.Out of scope (deferred per PM constraint)
aperag/service/search_service.py"legacy compatibility layer" — never created in the repo, so nothing to delete (PM msg=dc63c7e6 feat: api test #3).RedisChatMessageHistorycleanup —task #80territory, owned by @明书 on PR feat(phase8 #80 D8.6 chunk-1): hard-cut RedisChatMessageHistory + StoredChatMessage* legacy #1720+ (PM msg=309b3ed3 feat/frontend #1).section_path/heading_anchorattach across the 5 indexer modes (vector_index/graph_index/summary_index/vision_index/fulltext_index) — explicit out-of-scope per PM msg=5760999e fix: upload token #4 (multi-indexer expansion would balloon the write-set). The schema field is exposed; populated values for those handles arrive once each indexer is taught to attach section context, which is a future lane.aperag/domains/web_access/schemas.pymax_results→top_krename — the wrapper translates at the MCP boundary (PM constraint feat: auth bearer token support #2 msg=309b3ed3, "目标是 cutover contract 收口").Test plan
uv run pytest tests/unit_test/— 1001 passed, 29 skippeduv run ruff check+uv run ruff format --check— cleanReviewers per RR2
feedback_dataflow_review.mdstep 0+ enforcement + step 0 dataflow trace; this lane is co-owned so the architect's own block C is intentionally excluded from the architect's quick-check perimeter)🤖 Generated with Claude Code