fix(search): the banner states the DEPTH, not the store (#526) - #542
Merged
Merged
Conversation
|
Warning Review limit reachedNext included review available in 8 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (9)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
jphein
added a commit
that referenced
this pull request
Sep 18, 2026
--next-seq said 162 after the final fetch, but #542 (the banner PR, still open) already carries 162 on its branch; 163 avoids colliding with my own open PR. commit: HEAD for the merge step to resolve. Four renderers (changelog, README, llms-full, python-api — result_ordering gained a public helper); check-docs clean 8/8. Part of #526 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
"no curated document matched" is a claim about the STORE. What was measured is the RETURNED SET, and in a wing with ~941K transcript drawers the curated layer begins at rank 14-27, so at --limit 3 the sentence was true of the result and false of the corpus — read by two sessions as "this project has nothing curated on that" (#526). Three cases now, on one result object: curated ranked inside the limit -> nothing said about curation curated promoted from rank K -> "1 curated hit promoted from rank K; the curated layer in this wing begins there - widen the pool with --limit 30" none within the deep fetch -> "no curated document in the top N", and NO rank clause, because no K exists The advice names the mechanism. The requested limit sizes the hybrid fusion candidate pool (#534), so --limit 30 is not "scroll further down the list", it is "widen the pool" — the document was never a candidate. JOINT PRODUCER/CONSUMER TEST, written before the producer shipped so the field's shape was fixed by the consumer's need: tests/ test_search_banner_contract.py drives one result object from curated_first_rank (#534) to the rendered banner across all three cases and asserts the printed K equals the K in --json, parametrised at 14 and 27. A banner whose number disagrees with the field is worse than no number: one reader acts on the prose, another parses the field. The old all-transcript / transcript-plus-diary split is dropped from the banner; the per-hit ⟨transcript⟩ / ⟨diary⟩ tags carry it, and at header level the useful statement is the depth. all_transcript() consequently has no production caller — kept as a tested predicate, with its "for the header line" docstring corrected so it does not describe behaviour that no longer exists. Three #477 banner tests asserted the old wording; updated to the new contract, not deleted, and one now also asserts the phrase "no curated document matched" is ABSENT. Part of #526 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
seq from --next-seq taken after the final fetch; commit: HEAD for the merge step to resolve. All four renderers (changelog, README, llms-full, python-api — the all_transcript docstring changed); check-docs clean. Part of #526 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jphein
force-pushed
the
feat/526-banner
branch
from
September 19, 2026 03:35
0ad6948 to
587c573
Compare
jphein
added a commit
that referenced
this pull request
Sep 19, 2026
--next-seq said 162 after the final fetch, but #542 (the banner PR, still open) already carries 162 on its branch; 163 avoids colliding with my own open PR. commit: HEAD for the merge step to resolve. Four renderers (changelog, README, llms-full, python-api — result_ordering gained a public helper); check-docs clean 8/8. Part of #526 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 19, 2026
… both markers #534's promotion tag and this PR's collapse tag were already composed in both renderers on this branch (_print_hit_table's header line; the _provenance_tag return). With #542 on main the composition is a named producer→consumer pair rather than an incidental one: one result object whose reserved hit also absorbed a duplicate, driven through table, full and compact, must show both tags on the SAME line, and --json must carry promoted/promoted_from_rank and duplicates_collapsed/duplicate_of on the same hit. Four tests (3 parametrised + JSON). Entry test count 13 → 17. Part of #526 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 19, 2026
jphein
added a commit
that referenced
this pull request
Sep 19, 2026
…tch and reserved slot (#526) (#543) ## feat(search): collapse identical-text hits before the curated deep fetch and reserved slot (#526, PR 3 of 4) Off `69fd30c1` (#534 squash). Part of #526. **Defect.** A document mined at two paths — `docs/foo.md` and its `docs/rescued-from-vartmp-20260905/foo.md` copy — comes back twice, so `--limit 3` spends two of three slots on the same words. Existing mechanisms checked first: `prefer_curated` reorders near-duplicates but never removes; `searcher._dedupe_rendered_hits` keys on the SAME path and closet hits only. Neither closes this. **Change.** `result_ordering.collapse_identical_text(results)` — in place, idempotent over an already-collapsed list, never raises, same contract as `prefer_curated`. | aspect | rule | |---|---| | identity | `text` with whitespace runs collapsed; nothing looser — a one-line edit stays two documents | | position | first occurrence keeps the ranker's slot | | payload | a later higher-trust copy (memory < file < transcript < diary via `_kind_rank`) takes the slot and inherits the bookkeeping — same words, the copy a reader can cite | | JSON channel | kept hit gets `duplicates_collapsed: N`, `duplicate_of: [paths…]` — per hit, never summed | | prose channel | `⟨N identical copies collapsed⟩` from one producer `_collapse_tag`, rendered by `table`/`full` (`_print_hit_table`) and `compact` (`_provenance_tag`) — one test per renderer | | where | fast, hybrid and MCP-fallback routes, after `annotate`, BEFORE `_deep_fetch_when_nothing_curated`; and on the deeper list | | depth trigger | also fires when a collapse left the shallow list shorter than the limit — one extra call, only when a duplicate was seen; `call_count == 1` on the common path (tested) | **Producer/consumer, one test.** `test_collapse_runs_before_the_curated_deep_fetch_and_reservation`: the curated copy at rank 14 says the same words as the transcript at rank 2; collapsed first, the curated copy takes rank 2 on its own merit — visible at `--limit 3` with NO `promoted` flag and `curated_first_rank == 2`. That is the "before reservation" claim, executed. **Positive control on the base 69fd30c.** 11/13 new tests FAIL there (5 helper tests by ImportError, T1/T4 and all three renderer tests by assertion); 2 pass and are marked in their docstrings as regression guards, not discriminators (`no_second_call_when_nothing_collapsed…`, `table_does_not_mark_unique_hits`). **Existing tests changed (contract, disclosed).** Three fixtures in two existing test files had two or more hits with identical text — exactly what this collapses — and were given distinct texts so each keeps testing what it names: `test_cli_daemon::test_widen_tolerates_a_failed_second_call` (two `UNRELATED` transcripts → 1), `test_cli_daemon::test_widen_that_finds_nothing_curated_still_reports_honestly` (4 identical → 1, silently weakening it), and `test_cli_search_output::test_one_line_per_hit` (two default `_hit()`s). **Composed marker (PART 50 go-time condition).** #534's `_promotion_tag` and this PR's `_collapse_tag` are composed in BOTH renderers — `_print_hit_table`'s `[N] wing / room` line (table + full) and `_provenance_tag`'s return (compact) — so a hit that is both reserved and a collapse anchor shows both. Named producer→consumer pair: `test_a_hit_that_is_both_promoted_and_a_collapse_anchor_shows_both` (parametrised table/full/compact, asserts both tags on the SAME line) and `test_json_carries_both_markers_on_the_same_hit`. **A fixture trap worth naming.** My first shallow/deep payloads shared dict objects, so collapsing the deeper list re-counted the kept dict (`duplicates_collapsed: 2`). The daemon returns fresh JSON per call; the fixtures now `deepcopy`, with a comment saying why. **Blast radius.** `result_ordering.py` (+1 public helper, `__all__`), `cli.py` (`_collapse_tag`, `_provenance_tag`, `_print_hit_table`, `_deep_fetch_when_nothing_curated` gains `short=`, `_daemon_search_fast`, `_daemon_search_hybrid`, MCP fallback in `cmd_search`), 2 existing test fixtures, 1 new test file (13 tests). **Verification.** Full suite 7565 passed / 82 skipped / 0 failed (rc from the same invocation); targeted 230/230 across 7 search/render files; `ruff check` + `ruff format --check` clean; check-docs (see entry commit). Landing order: after #542 in the cli.py queue (`_provenance_tag` is touched by both — expect one textual rebase at go). 🤖 Generated with [Claude Code](https://claude.com/claude-code)
jphein
added a commit
that referenced
this pull request
Sep 19, 2026
a5d8903 164 landed with #541, 165 with #542 and 166 with #543; --next-seq after the final fetch says 167. The rebase also merged #543's `short=` and this PR's `warnings=` on _deep_fetch_when_nothing_curated and its two call sites — both kept. All four renderers; check-docs clean. Part of #526 Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
jphein
added a commit
that referenced
this pull request
Sep 19, 2026
…its (#526) (#545) ## fix(search): a daemon error object is an ERROR with a code, never 0 hits (#526, PR 4 of 4) Consumer of #536's option-C error shape (`{error, code, source, status?, detail?}` — lucid-error-contract owns the shape; this PR adds one key to the documented set and emits it). **Part of #526** — deliberately not "Closes": #526 enumerates three fixes (banner → #542, retrieval → #534, dedup → #543), all merged before this PR; this fourth PR is a defect found while measuring those three (a busy daemon rendering as an empty corpus), adjacent to the umbrella rather than one of its parts. #526 can be closed by hand once this lands, citing the four. **Defect, measured on production.** A saturated daemon answered `/search/hybrid` with HTTP 200 and `{"error": {"code": -32003, "message": "daemon busy: 8 MCP tool call(s) in flight (PALACE_MCP_TOOL_MAX_INFLIGHT=8)"}}`. The REST transports returned it verbatim, the search helpers read `.get("results") or []`, and the CLI printed **0 hits, exit 1**. A busy daemon was indistinguishable from an empty corpus — and the depth banner would then have said no curated document was in the top N. That is #526's own error class inside #526's fix. **The base defect is wider than search, and one grade worse in one verb.** On base 8408188, `status --json` against a 200 + `-32003` error object **exited 0**, echoing the error object as if it were the payload — a SUCCESS exit carrying an error (Oracle PART 51). On this head every daemon-backed verb that reaches a transport — 7/7 driven by Oracle with a busy body — exits 2 with `code: "daemon_busy"`, because the classification lives in the three transports rather than in any one verb. **Change.** | piece | rule | |---|---| | classifier `_raise_if_daemon_error_object(body, route)` | keyed on the PAYLOAD: dict with `error` and neither `results` nor `result`. `-32003` or "busy" in the message → `DaemonBusyError`; anything else → `DaemonError("daemon error <code> on <route>: <msg>")` (the prefix `_fail_daemon` keys its reachable line on). A body with `results` beside an `error` passes through. | | producers | all three transports call it — `_call_daemon_rest`, `_post_daemon_rest`, `_call_daemon_tool` — so REST and MCP agree. In `_call_daemon_tool` the classifier raises for every error envelope, so the pre-existing `raise DaemonError("daemon error <code>: <msg>")` below it was dead and is deleted; the message now carries the route (`… on /mcp <tool>: …`), which the old one lacked — kept on purpose. Tests that build that string by hand as a side effect are unaffected. | | renderer | `cmd_search`'s `except DaemonError` → `_fail_daemon(e, want_json, route=…, query=…)` (the one search-shaped site #536 did not reach). New busy branch emits a **dict literal**: `{"error": prose, "code": "daemon_busy", "source": "daemon", "detail": <daemon words>, "route": …}`, exit 2; prose: `palace daemon at <url> is busy — <words>; retry shortly` on stderr | | documented set | `daemon_busy` appended to the header line AND to `BRANCHABLE_CODES` in `test_cli_daemon_error_contract.py` — both or neither | | optimisations degrade, never silently | `_deep_fetch_when_nothing_curated(..., warnings=)` appends `deeper fetch unavailable: <daemon words>`; auto-mode's swallowed hybrid fallback appends `hybrid fallback unavailable: …`. The header already prints `warnings`, so `curated_first_rank: null` is read as "the depth was not checked" | **Why a literal.** `test_every_code_we_emit_is_in_the_documented_set` walks dict literals only (emitted ⊆ documented). Adding `daemon_busy` to the header without a literal emitter would be unverified vocabulary reading as a contract. `test_daemon_busy_is_emitted_as_a_dict_literal` is the converse for this one key — safe here because the emitter is known to be a literal; the general converse would false-positive on `_fail_daemon`'s computed `code`. **Pre-registration amendment, stated.** I pre-registered "2nd response busy → exit 2, never `curated_first_rank: null`". That contradicts the tested contract that the deeper fetch is an optimisation (`test_widen_tolerates_a_failed_second_call` degrades to the shallow hits). Replacement: degrade AND carry the daemon's words in `warnings`; joint producer→consumer test (`test_busy_deeper_fetch_keeps_the_shallow_hits_and_warns` + `test_the_header_prints_that_warning`). **Positive control on the base (#536 @9441576b, unfixed).** 15/15 substantive tests FAIL there (4 exit/code, 3 degrade, 3 documented/literal, 4 classifier, 1 MCP). A filler test written to justify an import was deleted rather than kept. **Blast radius.** `cli.py`: `DaemonBusyError` (new class), `_raise_if_daemon_error_object` (new), `_call_daemon_tool`, `_call_daemon_rest`, `_post_daemon_rest`, `_fail_daemon` (busy branch), `cmd_search` except, `_deep_fetch_when_nothing_curated` (`warnings=` kwarg), `_daemon_search_fast` (adds `warnings` only when non-empty — return shape otherwise unchanged), `_daemon_search_hybrid`, `_daemon_search_auto`; header contract line; `BRANCHABLE_CODES`. `_window_daemon_get` is deliberately untouched — different producer, same payload rule would apply if it ever returned a 200 error object. **Verification.** Stacked on #536 @9441576b: full suite 7596 passed / 82 skipped / 0 failed (rc from the same invocation); 330/330 across 12 files including #536's own contract/4xx/one-message/exit-propagation/window-source/cypher/stats suites; `ruff check` + `ruff format --check` clean; check-docs (see entry commit). Lands after #536; expect one generated-only rebase at go. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
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
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.
fix(search): the banner states the DEPTH, not the store (#526, PR 2 of 4)
Stacked on #534 (curated_first_rank producer). Part of #526.
Defect.
! all N hits are session-transcript copies — no curated document matchedis a claim about the STORE; the measurement was of the RETURNED SET. In wing 2g the
curated layer begins at rank 14-27, so at
--limit 3the sentence was true of theresult and false of the corpus, and two sessions concluded the project had nothing.
Change. One banner, three cases, keyed on
curated_first_rankfrom #534:! 1 curated hit promoted from rank K; the curated layer in this wing begins there — widen the pool with --limit 30curated_first_rank: null)! no curated document in the top N— no rank clause, no K exists"Widen the pool" is the mechanism (#534: the limit sizes the hybrid fusion candidate
pool), not "scroll further".
Producer/consumer pair, one test.
tests/test_search_banner_contract.pydrives oneresult object from the field to the rendered line for all three cases and asserts
printed K ==
--jsonK at 14 and 27. Negative: the phrase "no curated documentmatched" is asserted ABSENT. The banner is only reached when
resultsis non-empty,so an error payload cannot print it (PR 4 hardens the error path itself).
Blast radius.
_print_search_header(cli.py),all_transcriptdocstring(provenance.py — no production caller now, kept + tested), 3 #477 banner tests updated
to the new wording. No JSON field changes.
Verification. Rebased onto 69fd30c (#534 squash): full suite 7559 passed / 82 skipped / 1 failed — the one failure was
test_quiet_suppresses_the_banner, whose substringpromoted from ranknow also matches the per-hit marker #534's PART 44 fix added to the table renderer; sharpened to assert the banner SENTENCE is absent under--quietand the per-hit marker survives (quiet strips chrome, not provenance). After: banner contract 8/8 + depth 16/16; ruff check + format clean; check-docs clean (see entry commit). Order note: lands after #536 and #541 in the cli.py queue — expect one generated-only rebase at go.🤖 Generated with Claude Code