refactor(server): decompose semantic.py and cookbook.py

[cbcoutinho]

Part 3 of 3, splitting the SonarCloud S3776 work that was #1437. Stacked on
#1441.

This is the part that isn't mechanical. Parts 1 and 2 only change where
functions are defined; this one changes code shape, which is why it is separated
— it deserves the closest reading of the three.

semantic.py — 80

nc_semantic_search was an 850-line function, so lifting it to module level
alone would have relocated the finding onto the tool rather than fixing it.
Five cohesive pieces it was carrying are now their own functions:

helper	what it was
_retrieve_candidates	the doc_types is None / per-type dispatch + merge
_to_semantic_results	row → response mapping (the file_url ternary sat 5 levels deep)
_expand_results_with_context	the include_context fan-out and its task group
_parse_modified_bounds	ADR-027 date parsing + the cross-field ordering guard
_rerank_pool	the rerank pass and its metric label
_fetch_limit / _log_top_results	the overfetch ternary; the post-verification debug log

Each is documented with why it moved (nesting, not reuse), so the next reader
doesn't mistake them for shared utilities.

cookbook.py — 153, the worst of the five

The lift alone was not enough here either: nc_cookbook_create_recipe (16)
and nc_cookbook_update_recipe (20) were over threshold on their own, so
stopping at extraction would have traded one finding for two new ones.

Ten sequential if field: assignments became one field-mapping table, and the
HTTP-status if/elif chains a status→message dict.

The one thing worth reviewing closely: the create/update guard difference is
preserved and now stated explicitly rather than left implicit in two
near-identical blocks —

create: keep=bool                        # drops falsy: an omitted optional isn't sent
update: keep=lambda v: v is not None     # keeps "": an empty string CLEARS the field

That asymmetry was the easiest thing to lose in this refactor, and losing it
would silently change what an empty-string update does.

Result

Max cognitive complexity across both files: 13, from 153 and 80.

Verification

ruff, ruff format, ty clean. Tests run in CI per the repo owner's
instruction. The three parts together are byte-identical to the #1437 tree that
passed the full 28-check matrix and on which SonarCloud reported "No issues
found".

Refs Deck card 1203.

---

This PR was generated with the help of AI, and reviewed by a Human

🤖 Generated with Claude Code

https://claude.ai/code/session_019iCzZAuCMsZS7UJYfy3htd

[claude]

Claude finished @cbcoutinho's task in 3m 22s —— View job

---

Review of PR #1442 — decompose semantic.py and cookbook.py

Todo list:

• Gather context (PR description, comments, CLAUDE.md conventions)
• Diff semantic.py against base branch (origin/refactor/lift-deck-registration)
• Diff cookbook.py against base branch
• Check the create/update field-mapping asymmetry called out in the PR description
• Check annotation fix from round-1 review (443a42a) and the restored TODO from round-2 (1a5c6f3)
• Attempt to run ruff/ty (blocked — see note below)
• Post final review

Summary

I diffed both files line-by-line against origin/refactor/lift-deck-registration and traced each extracted helper back to the block it replaced. This is a careful, well-documented refactor — I didn't find any behavioral regressions.

semantic.py

• _retrieve_candidates: the doc_types is None vs. per-type-loop-then-sort-then-cap-to-fetch_limit logic is byte-for-byte equivalent to the original, including the TODO(ADR-019) comment that was correctly restored in 1a5c6f3 after round-2 review caught the drop.
• _fetch_limit / _rerank_pool: verified the verification_budget=limit * 2 trim and the effective_pool_size gating match the original fetch_limit/rerank block exactly, including the SKIPPED→"false" metric-label mapping.
• _to_semantic_results / _expand_results_with_context: unchanged logic, just lifted. The client/accessible_owners annotation gap flagged in round-1 was fixed correctly (client: "NextcloudClient" behind TYPE_CHECKING, matching the webdav.py/deck.py forward-ref pattern; accessible_owners: list[str] matches AccessibleScope.owners).
• Round-2 also tightened list[Any] → list[SearchResult] on four signatures and search_algo: Any → BM25HybridSearchAlgorithm, checked against how rerank_results/filter_by_relevance/verify_search_results already type the same values rather than taken on faith. settings: Any is left untyped with a documented reason (dynaconf object, consistent with the rest of the file) — reasonable.

cookbook.py

• The asymmetry the PR calls out as "the easiest thing to lose" is preserved correctly: nc_cookbook_create_recipe calls _recipe_payload(keep=bool, ...) (drops falsy, matching the original ten if field: blocks, including that recipe_yield=0 was — and still is — dropped on create, a preexisting quirk not introduced here), while nc_cookbook_update_recipe calls _recipe_payload(keep=lambda v: v is not None, ...) (matching the original if field is not None: blocks, so "" still clears a field). Confirmed against the pre-refactor if-chains field-by-field.
• _raise_recipe_error's status→message dicts for create/update/the update's get_recipe fetch failure match the original if/elif chains exactly, status code for status code, including the fallback messages.
• The other eight tools (list_recipes, get_recipe, delete_recipe, search_recipes, list_categories, get_recipes_in_category, list_keywords, get_recipes_with_keywords, set_config, reindex) were correctly left as plain if/elif chains — they weren't over threshold, so collapsing them into the dict pattern too would have been unnecessary abstraction.
• Module-level registration in configure_cookbook_tools follows the same lift pattern established in part 1 (859bc45, deck.py): decorators moved from the nested @mcp.tool(...) stack onto the module-level def, then registered via mcp.tool(...)( fn ).

Minor, non-blocking observations

• None of the newly-extracted pure functions (_parse_modified_bounds, _recipe_payload, _fetch_limit, _to_semantic_results) have direct unit tests targeting them by name — coverage is still only indirect, through nc_semantic_search/nc_cookbook_create_recipe/nc_cookbook_update_recipe end-to-end tests (which do exist: tests/unit/server/test_semantic_*, tests/server/test_cookbook_mcp.py, etc.). Now that these are module-level and pure, they'd be cheap to test in isolation — worth considering as a follow-up, not a blocker for this refactor.
• SonarCloud's Quality Gate passed with 0 new issues, consistent with the stated goal (max cognitive complexity 13 across both files).

Tooling note

I could not run ruff, ruff format, ty, or pytest in this environment — the same allowlist gap the author flagged in their round-1 reply (Glob, Grep, LS, Read plus a few git/gh commands only) also blocks Bash(uv run ...) here. My review is based on manual line-by-line diffing against the base branch rather than tool output; the author's own local runs (clean ruff/ruff format/ty) and the passing SonarCloud gate are the available corroboration.

No blocking findings. This looks ready to merge on its own merits, contingent on CI (Tests) coming back green per the author's note in the previous comment.

[cbcoutinho]

Round-1 finding addressed in 443a42a7:

• Missing type annotations on _expand_results_with_context: fixed. client: "NextcloudClient" behind TYPE_CHECKING, and accessible_owners: list[str].

You were right about both the gap and the types, and I verified rather than took them: server/webdav.py:167 and server/deck.py:370 do use the cycle-guarded forward-ref pattern, and AccessibleScope.owners is declared list[str] in search/access_filter.py:100.

Worth noting why it happened, since it's a general hazard of this kind of refactor: as a nested closure those two were captured from the enclosing scope and carried no annotation of their own. Lifting them to parameters is precisely the point where that becomes a signature gap — and nothing flags it, because ty was already clean before the change.

On your tooling note — ruff, ruff format and ty are clean locally (ty reports no diagnostics with the annotations added). CI could not run on this PR when you reviewed it: it was opened against a feature branch before stack #1443 existed, so Tests was filtered out by branches: [master] and only the 3 always-on checks ran. Re-triggered; the full matrix is running now, and I won't call this done until it is green.

Also: your inability to run ruff/ty/pytest here is a real gap rather than a quirk of this session — the workflow's allowlist grants Glob, Grep, LS, Read plus a few git/gh commands and nothing else. #1439 recently added Task to it (that is why this review completed at all — the un-split PR failed three times trying to fan out). Adding the test/lint commands is a reasonable follow-up, and I've flagged it for the repo owner rather than widening CI permissions unilaterally.

[cbcoutinho]

Round-2 findings addressed in 1a5c6f3a:

• Dropped TODO(ADR-019): expose VERIFICATION_OVERFETCH: restored in _retrieve_candidates, together with the rationale it was attached to (why 2×, what high ghost density costs, that the index self-heals via lazy eviction). Good catch — I confirmed with grep that after the extraction it existed nowhere else in the tree, so it wasn't tracked elsewhere. This is the failure mode worth naming for the whole refactor: a moved block keeps its code but can silently shed the comments explaining it, and nothing notices — not ty, not the tests, not Sonar. Only a line-by-line reader.
• list[Any] → list[SearchResult] on _retrieve_candidates, _rerank_pool, _to_semantic_results, _log_top_results, and search_algo: Any → BM25HybridSearchAlgorithm. Verified against the convention rather than taken on your word: rerank_results (search/rerank.py:222), filter_by_relevance (relevance.py:197) and verify_search_results all use list[SearchResult], and this file already imported the algorithm class.

Left as-is: settings: Any — it's a dynaconf object with no declared type, and the rest of the file passes it around untyped for that reason. Narrowing it here would be a false precision.

ruff, ruff format, ty clean.

On your verification note: SonarCloud's gate passing with 0 new issues is the right thing to have leaned on, and the Tests matrix genuinely had not run when you looked — this PR was opened against a feature branch before stack #1443 existed, so branches: [master] filtered it out and only the 3 always-on checks ran. That's now re-triggered and running. I won't call this done until it's green.

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
2.2% Duplication on New Code

See analysis details on SonarQube Cloud