fix(cascade): fuse heading-nav and TF-IDF signals instead of skipping TF-IDF on a zero-hit - #137
Conversation
|
Warning Review limit reachedNext included review available in 43 minutes. View limit detailsLimit details: You’ve used the included review currently available. This review ran on the open-source allowance, not this organization's plan, because the pull request author doesn't have an assigned seat. Waiting won't change this — ask an organization admin to assign them a seat, or add seats in Billing if every seat is already assigned, then retry. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
Comment |
code-rankerBuilt on a fork. View full report ↗ python
|
… TF-IDF on a zero-hit Problem 2 of constructorfabric#134: route_tier1 escalated immediately whenever heading-nav found zero hits, without ever running TF-IDF -- discarding a genuinely different signal. Heading-nav requires the query's exact literal substring somewhere in a section's raw text; TF-IDF tokenizes on individual words, so a query differing from the source only in punctuation/spacing/hyphenation can score unambiguously on TF-IDF even when heading-nav's exact-phrase match fails outright. Both methods now always run. Two new Tier 1 rows cover heading-nav's zero-hit case: TF-IDF alone resolves at Tier 1 when unambiguous, and otherwise escalates with TF-IDF's own top pick as the Tier-2 candidate (rather than none at all) unless TF-IDF also has no positive score anywhere. Existing rows (agree/disagree/diffuse-margin when heading-nav does hit) are unchanged. Deliberately still out of scope: folding OKF summaries into Tier 1 as a fourth signal -- Tier 1's whole value is being free and deterministic without ever touching the OKF bundle. Signed-off-by: TECK KEAT WILSON <yeow.teck.keat@constructor.tech>
7b9a838 to
af084c5
Compare
…signal Review findings on PR constructorfabric#137 (ainetx): - Major: route_tier1 unconditionally compared tfidf_ranked[0] against heading-nav's pick once heading-nav found a hit, even when every section scored exactly 0 (no real TF-IDF signal at all). tfidf_ranked[0] in that case is just an arbitrary document-order tie-break, not a genuine top pick -- comparing it anyway fabricated a "resolved_multi" / "heading_nav_tfidf_disagree" verdict (or, by coincidence of order, a fabricated agreement) out of a signal that was never there. Added an explicit row 4 (heading-nav has a hit, TF-IDF has no signal anywhere) that escalates on heading-nav's single, unconfirmed signal instead, symmetric to how rows 2/3 already handle TF-IDF's own no-signal case. Splits route_tier1 into two functions along the table's own two-part structure (heading-nav miss vs. hit) rather than disabling pylint's too-many-return-statements once the new row pushed past it. - Minor: five hand-built TestRouteTier2 fixtures still used the retired "heading_nav_no_hits" reason string (route_tier1 was renamed to "no_signal_from_either_method" for that case in the prior commit, without these being updated). Consolidated into one named constant, since route_tier2 never reads the reason field -- these fixtures only ever needed the row-1 *shape*, not a specific string -- so a future rename can't silently drift out of sync with these tests again. - Renamed the TestRouteTier1 test methods to match the routing table's real row numbers post-insertion (row 4 new; disagree/agree/diffuse-margin shifted from 3/2/4 to 5/6/7) and added a regression test reproducing the review's exact repro case for the new row 4. Signed-off-by: TECK KEAT WILSON <yeow.teck.keat@constructor.tech>
|
|
|
||
| def test_row3_tfidf_only_diffuse_escalates_with_tfidf_pick_as_candidate(self, tmp_path: Path, monkeypatch): | ||
| """Heading-nav still has nothing, and TF-IDF has a real but diffuse | ||
| signal (both sections score positively) -- escalates, but hands |
There was a problem hiding this comment.
route_tier2 lacks a test for the new TF-IDF-sourced row-3 escalate candidate
Severity: Minor
Problem
route_tier2's docstring was updated to say it's now called for rows 1, 3, and 6, with rows 3/6 narrowing to a single candidate section. Row 6's nav-sourced candidate already has coverage (pre-existing tests), but row 3's candidate is a brand-new provenance (a TF-IDF ranked entry via _as_candidate(tfidf_only_pick), not a heading-nav match) and no test in the diff calls route_tier2 with a Tier-1 result whose reason is heading_nav_no_hits_diffuse_tfidf.
How to reproduce
- Construct a route_tier1-style result with reason='heading_nav_no_hits_diffuse_tfidf' and a TF-IDF-derived candidate. 2. Call route_tier2 with it. 3. Observe there is no existing assertion in the diff verifying correct OKF-staleness narrowing for this input shape.
Expected behavior
A test asserting route_tier2, given a row-3 Tier-1 output, narrows OKF-staleness checking to exactly the TF-IDF-derived candidate section, mirroring the row-6 nav-sourced candidate test.
Actual behavior
No such test is added or modified in this diff; only route_tier1's three new branches (rows 1-3) are tested.
route_tier1 (row3, new) --candidate(tfidf)--> route_tier2 [UNTESTED for this candidate shape]
route_tier1 (row6, existing) --candidate(nav)--> route_tier2 [tested]
Impact
A candidate-shape or narrowing-logic bug specific to TF-IDF-sourced candidates (e.g. key mismatch between _as_candidate output and what route_tier2 expects) could ship undetected.
Suggested correction
Add a TestRouteTier2 test that feeds a reason='heading_nav_no_hits_diffuse_tfidf' Tier-1 result (with a TF-IDF-derived candidate) into route_tier2 and asserts correct single-section OKF narrowing.
How to verify
Run the new test and confirm it fails against a route_tier2 that mishandles TF-IDF-sourced candidates, then passes with the current implementation.
| match. Every Tier 1 call now runs both methods unconditionally; only their | ||
| *outcome* determines whether the query needed one signal, two, or must | ||
| escalate. Row 4 exists because "TF-IDF found nothing" (every section scores | ||
| exactly 0) is a distinct outcome from "TF-IDF agrees" or "TF-IDF disagrees" |
There was a problem hiding this comment.
OKF-independence guarantee for Tier 1 is documented but not enforced
Severity: Minor
Problem
The new module docstring paragraph states a newly-articulated architectural invariant: route_tier1 must never touch the OKF bundle, so that Tier 1 stays free and deterministic. Nothing in the diff backs this with an automated check -- there is no import-boundary lint rule scoped to cascade.py's route_tier1 vs. the OKF module, and no test that asserts route_tier1 never imports or calls into OKF-related code.
How to reproduce
- A future contributor edits route_tier1 to add an OKF lookup as a tiebreaker signal (e.g. to resolve ambiguous TF-IDF cases). 2. No lint rule or test fails. 3. The 'free and deterministic Tier 1' guarantee silently breaks, and callers relying on Tier 1 never touching OKF are unaware.
Expected behavior
Either an import-boundary/lint rule blocking cascade.py (or specifically route_tier1) from importing the OKF module, or a test that inspects route_tier1's dependencies/behavior to assert OKF is never consulted.
Actual behavior
The guarantee exists only as a comment in the module docstring; nothing in the codebase enforces it mechanically.
future edit -> route_tier1 imports/calls OKF module -> no lint/test catches it -> documented invariant silently violated
Impact
Low immediate risk, but the documented architectural boundary can erode over time without any signal, undermining the stated design guarantee and potentially reintroducing non-determinism or hidden LLM calls into what's advertised as a purely mechanical tier.
Suggested correction
Add a lightweight test (e.g. static check that cascade.py's route_tier1 function/module doesn't reference the OKF module) or wire an import-boundary lint rule analogous to the existing studio_proxy<->studio one, scoped to cascade.py vs. the OKF module.
How to verify
Introduce a stub OKF import/call inside route_tier1 and confirm the new test or lint rule fails.
| # positively on both -- a real signal, just not an unambiguous one. | ||
| _TFIDF_ONLY_DIFFUSE_SAMPLE = ( | ||
| "## SectionA\n\nA gizmo-thing sits here alone.\n\n" | ||
| "## SectionB\n\nThe gizmo hums. " + ("filler word text here. " * 30) + "A thing rattles.\n" |
There was a problem hiding this comment.
No test covers tfidf-only resolution with a single-section document
Severity: Minor
Problem
The new row-2 (tfidf_only_unambiguous) and row-3 (tfidf_only_diffuse) tests both use two-section fixtures. score_sections's unambiguous flag is generally computed from the margin between the top and second-ranked section; with only one section in the document there is no second candidate to compare against, which is a distinct code path from the two added fixtures.
How to reproduce
- Create a one-section markdown doc where the section scores positively on TF-IDF for some query but heading-nav has zero hits (e.g. a hyphenation gap as in the existing KAPING example, but with only one heading). 2. Call route_tier1. 3. Observe whether
unambiguousis trivially true/false and whether tier/reason match expectations -- this is currently unverified by any test.
Expected behavior
A test asserting route_tier1's exact return dict for a one-section document under the new heading-nav-zero-hits/TF-IDF-positive branch.
Actual behavior
No such test exists in the diff; the single-candidate boundary of the new tfidf-only paths is unexercised.
score_sections(one section) -> ranked=[X] -> unambiguous=? (untested) -> route_tier1 branch untested
Impact
A latent bug in how unambiguous behaves for a single-candidate ranking (e.g. vacuously True) could silently resolve queries at Tier 1 that should escalate, without any test catching a regression.
Suggested correction
Add a test with a one-section fixture, a query giving heading-nav zero hits and a positive TF-IDF score, asserting route_tier1's tier/reason/candidates.
How to verify
New test passes and documents the intended behavior for the single-section case.
| def test_row1_neither_method_has_signal_escalates(self, tmp_path: Path, monkeypatch): | ||
| """"making up" is tfidf.py's own documented adversarial case: "up" is | ||
| filtered by the 3-char minimum, and "making" never appears anywhere | ||
| in this fixture, so TF-IDF has nothing either -- both methods |
There was a problem hiding this comment.
No regression test guarding that the TF-IDF-only unambiguous branch never calls route_tier2/OKF
Severity: Minor
Problem
route_tier1's new row-2 branch (tfidf_only_unambiguous) resolves entirely within Tier 1 and, per the docstring intent, should never reach Tier 2/OKF. The only test covering this branch, test_row2_tfidf_only_unambiguous_resolves_at_tier1, checks the returned dict shape but does not mock or spy on route_tier2 (or an OKF lookup) to assert it was never called.
How to reproduce
- Inspect tests/test_cascade.py's test_row2_tfidf_only_unambiguous_resolves_at_tier1.
- Note it calls route_tier1 directly and asserts equality on the returned dict.
- Note no mock.patch/spy on studio.utils.cascade.route_tier2 (or the OKF lookup it uses) is set up to assert zero calls.
Expected behavior
A test that patches route_tier2 (or the underlying OKF status function) and asserts it is never called when route_tier1 takes the tfidf_only_unambiguous path, so a future refactor that accidentally threads Tier 2/OKF into this branch fails CI.
Actual behavior
Only the return-value shape is tested; nothing would fail if a future change caused this branch to also consult route_tier2/OKF, since route_tier1 doesn't currently call route_tier2 in any branch and no test enforces that invariant going forward.
route_tier1(tfidf_only_unambiguous) --returns--> {tier: resolved, ...} [tested] ; route_tier1 --calls?--> route_tier2/OKF [NOT tested to be absent]
Impact
Low likelihood but real coverage gap: a future maintainer merging Tier 1 and Tier 2 logic (e.g. while adding OKF awareness to Tier 1, which the docstring explicitly says is out of scope) would get no test failure signal for this specific branch.
Suggested correction
Add a test that monkeypatches/mocks route_tier2 (and/or the OKF status function it calls) and asserts it is not invoked when route_tier1 returns tier='resolved' with reason='tfidf_only_unambiguous'.
How to verify
Run the new test; it should fail if route_tier2 is called from this branch and pass with the current implementation.
| @@ -97,7 +178,7 @@ class TestRouteTier2: | |||
| def test_no_bundle_at_all_recommends_baseline(self, tmp_path: Path, monkeypatch): | |||
There was a problem hiding this comment.
route_tier2 has no test for the new row-3 (TF-IDF-only diffuse) escalation path
Severity: Minor
Problem
route_tier2's docstring was updated to say it's now called for rows 1, 3, 4, and 7 (previously 1 and 4), with row 3 narrowing to a TF-IDF-sourced candidate (a new provenance never exercised by route_tier2 before). The diff's test additions only cover route_tier1's three heading-nav-miss branches and reuse pre-existing route_tier2 tests that all pass the row-1 (no-candidate) shape via the new _NO_CANDIDATE_ESCALATION constant, or pre-existing nav-derived candidates.
How to reproduce
- Inspect tests/test_cascade.py's TestRouteTier2 class in the diff. 2. Note every tier1 fixture passed to route_tier2 uses reason='no_signal_from_either_method' (row 1) or a heading-nav-derived candidate (rows 4/7, pre-existing tests). 3. No fixture uses reason='heading_nav_no_hits_diffuse_tfidf' (row 3) with a TF-IDF-derived candidate.
Expected behavior
A test calling route_tier2 with a tier1 result shaped like row 3 (TF-IDF-only candidate) to confirm OKF staleness narrowing works correctly against a candidate whose provenance is score_sections rather than find_sections.
Actual behavior
Only route_tier1-level tests exercise the row-3 candidate shape; route_tier2 is never tested against it, so a candidate-shape or narrowing bug specific to TF-IDF-sourced candidates would not be caught by this test suite.
route_tier1 (row 3) --escalate+TFIDF candidate--> route_tier2
|
v
[untested narrowing logic]
Impact
A latent bug in route_tier2's handling of TF-IDF-derived candidates (e.g. a key mismatch from _as_candidate, or incorrect staleness narrowing) could ship undetected.
Suggested correction
Add a TestRouteTier2 test that builds a tier1 dict with reason='heading_nav_no_hits_diffuse_tfidf' and a TF-IDF-sourced candidate, then asserts route_tier2 narrows correctly to that section's OKF staleness.
How to verify
Run the new test and confirm it fails against a route_tier2 that mishandles TF-IDF-provenance candidates, then passes with the current implementation.
| match. Every Tier 1 call now runs both methods unconditionally; only their | ||
| *outcome* determines whether the query needed one signal, two, or must | ||
| escalate. Row 4 exists because "TF-IDF found nothing" (every section scores | ||
| exactly 0) is a distinct outcome from "TF-IDF agrees" or "TF-IDF disagrees" |
There was a problem hiding this comment.
Tier 1 / OKF independence is documented but not mechanically enforced
Severity: Minor
Problem
The module docstring newly asserts that route_tier1 must never query OKF status so Tier 1 stays free/deterministic, but this is prose only -- no lint rule, import-boundary check, or test in the diff verifies route_tier1 doesn't import or call into the OKF module.
How to reproduce
- A future PR adds an OKF status check inside route_tier1 (e.g. to 'optimize' escalation). 2. No test or lint fails. 3. The documented invariant silently breaks.
Expected behavior
Either an automated import-boundary rule/test asserting route_tier1 never references the OKF module, or acceptance that this is a soft, review-enforced convention.
Actual behavior
Only a comment states the invariant; nothing prevents violating it.
route_tier1 (docstring: 'never touches OKF') --(no enforcement)--> future edit could import OKF module --> invariant silently broken
Impact
Tier 1's determinism/no-LLM guarantee could regress unnoticed, undermining a documented architectural property.
Suggested correction
Add a small test (e.g. asserting 'okf' not in the cascade module's imports used by route_tier1, or a mock/spy ensuring OKF functions are never called during route_tier1) or an import-linter rule covering this pair.
How to verify
Introduce a stub OKF call inside route_tier1 and confirm the new test/lint rule fails.
| "## SectionB\n\ngadget gadget gadget.\n" | ||
| ) | ||
|
|
||
| # constructorfabric/studio#134, Oleg67's suggestion #4: a real single-signal |
There was a problem hiding this comment.
No test for tfidf_only_unambiguous with a single-section document
Severity: Minor
Problem
_TFIDF_ONLY_UNAMBIGUOUS_SAMPLE and _TFIDF_ONLY_DIFFUSE_SAMPLE both use two sections, so the new row-2 code path is never exercised against a document with exactly one section, where 'unambiguous' semantics (no second candidate to compare margin against) are a distinct boundary.
How to reproduce
- Construct a one-section doc where the query has 0 heading-nav hits but a positive TF-IDF score. 2. Call route_tier1. 3. No existing test asserts what tier/reason/candidate is returned in this case.
Expected behavior
A test asserting route_tier1's behavior for a one-section document under the row-2 conditions.
Actual behavior
This boundary is untested; behavior is only implied by code, not verified.
route_tier1 -> heading-nav 0 hits -> TF-IDF (1 section, score>0) -> unambiguous? -> [untested]
Impact
A regression or edge-case bug specific to single-section documents (a realistic short-document scenario) would not be caught by CI.
Suggested correction
Add a test fixture with one section and a query that yields 0 heading-nav hits and a positive TF-IDF score, asserting the expected route_tier1 result.
How to verify
Add the test, confirm it passes against current code and would fail if the single-candidate 'unambiguous' handling regressed.
| """Rows 1-3: heading-nav found nothing at all to anchor a guess to, so | ||
| TF-IDF alone decides whether there's anything else to go on. | ||
|
|
||
| Split out of :func:`route_tier1` (which was hitting pylint's |
There was a problem hiding this comment.
route_tier1 docstring's one-candidate row list omits row 3
Severity: Minor
Problem
The updated docstring says candidates has 'one for rows 2/4/6/7' but then parenthetically clarifies '(rows 3/4/7 despite escalating...)', which references row 3 as one of the escalating-but-one-candidate rows even though row 3 is missing from the main enumeration list. Since rows 2,3,4,6,7 all return exactly one candidate (row 1 returns none, row 5 returns two), the correct main list should include row 3.
How to reproduce
- Read the route_tier1 docstring in cascade.py after this diff. 2. Note the sentence 'one for rows 2/4/6/7 (rows 3/4/7 despite escalating...)'. 3. Compare to _route_tier1_heading_nav_miss's row-3 branch and test_row3_tfidf_only_diffuse_escalates_with_tfidf_pick_as_candidate, both of which show row 3 returns exactly one candidate.
Expected behavior
The main enumeration should read 'rows 2/3/4/6/7' so it's internally consistent with its own parenthetical and with the actual code/tests.
Actual behavior
Row 3 is dropped from the main list, creating a self-contradiction within the same docstring sentence.
docstring main list [2,4,6,7] --(inconsistent with)--> docstring parenthetical [3,4,7] --(consistent with)--> code/tests (row 3 = 1 candidate)
Impact
A reader relying on the docstring's main enumeration alone could wrongly conclude row 3 returns zero or an ambiguous number of candidates.
Suggested correction
Update the enumeration to 'one for rows 2/3/4/6/7 (rows 3/4/7 despite escalating, ...)'.
How to verify
Re-read the docstring and confirm the main list, the parenthetical, and the actual return statements/tests for rows 1-7 are mutually consistent.
| result = route_tier1(f, "making up") | ||
| assert result == {"tier": "escalate", "reason": "heading_nav_no_hits", "candidates": []} | ||
| assert result == {"tier": "escalate", "reason": "no_signal_from_either_method", "candidates": []} | ||
|
|
There was a problem hiding this comment.
No regression guard proving tfidf_only_unambiguous skips OKF/route_tier2
Severity: Minor
Problem
test_row2_tfidf_only_unambiguous_resolves_at_tier1 only asserts the returned dict equals the expected resolved result; it does not assert that route_tier2 or any OKF lookup was never called, so the documented 'Tier 1 stays free of OKF' guarantee for this specific new branch has no automated regression guard beyond the return-value check.
How to reproduce
- Hypothetically add a stray OKF/route_tier2 call inside the row-2 branch that doesn't affect the returned dict (e.g. a side-effecting lookup ignored downstream). 2. Run the existing test suite. 3. It still passes because only the return value is checked.
Expected behavior
A test that mocks/spies on route_tier2 (or the OKF module) and asserts it is not called when route_tier1 takes the tfidf_only_unambiguous path.
Actual behavior
Only the output shape is verified; no call-count/spy assertion exists.
route_tier1 (row 2) -> returns resolved dict (asserted) -> [OKF/route_tier2 call, if added, would go undetected]
Impact
A future regression that adds an unnecessary OKF touch to the fast, deterministic Tier-1-only path would go unnoticed by CI, silently degrading Tier 1's performance/determinism guarantee.
Suggested correction
Add a monkeypatch/spy on route_tier2 (or the OKF concept-file lookup function) in the row-2 test and assert it was never called.
How to verify
Add the spy-based assertion, confirm it passes now, and confirm it would fail if route_tier2 were called from that branch.
3 participants
Summary
route_tier1escalated immediately whenever heading-nav found zero hits, without ever running TF-IDF — discarding a genuinely different signal. Heading-nav (find_sections) requires the query's exact literal substring to appear somewhere in a section's raw text; TF-IDF (score_sections) tokenizes on individual alphanumeric words, so a query differing from the source only in punctuation/spacing/hyphenation can still score unambiguously on TF-IDF even when heading-nav's exact-phrase match fails outright.tfidf_only_unambiguous).heading_nav_no_hits_diffuse_tfidf) rather than none at all — unless TF-IDF also has no positive score anywhere, in which case it escalates with nothing (no_signal_from_either_method), same as the old behavior for a genuinely signal-less query.Update: review round (ainetx) — 1 Major, 1 Minor, both fixed
route_tier1still compared TF-IDF's document-order tie-break pick against heading-nav's real pick as if it were a genuine competing answer — fabricating aresolved_multi/heading_nav_tfidf_disagreeverdict (or, by coincidence of order, a fabricated agreement) out of a signal that was never there. Fix: added an explicit new row (heading-nav has a hit, TF-IDF has no signal anywhere) that escalates on heading-nav's single, unconfirmed signal instead — symmetric to how the zero-hit rows already handle TF-IDF's own no-signal case.route_tier1was split into two functions along the routing table's own two-part structure (heading-nav miss vs. hit) rather than disabling pylint's too-many-return-statements once the new row pushed past it. Added a regression test reproducing the review's exact repro case.TestRouteTier2fixtures still used the retired"heading_nav_no_hits"reason string (renamed to"no_signal_from_either_method"earlier in this same PR, without these being updated) —route_tier2never reads the reason field, so nothing failed, but the drift was real and misleading. Fix: consolidated into one named constant instead of five separate literal copies, so a future rename can't silently skip these fixtures again. Test method names were also renumbered to match the routing table's real rows post-insertion (new row 4; disagree/agree/diffuse-margin shifted from 3/2/4 to 5/6/7).Test plan
_TFIDF_ONLY_UNAMBIGUOUS_SAMPLE,_TFIDF_ONLY_DIFFUSE_SAMPLE) use a real hyphenation gap (source:"KAPING-framework"/ query:"KAPING framework") to reproduce heading-nav's exact-phrase miss while TF-IDF's tokenizer still resolves it — hand-verified againsttokenize()'s and_confidence()'s actual behavior, not just asserted against.pytest tests/test_cascade.py -q— 30 passed (the only test file this PR's diff touches — an earlier version of this description incorrectly claimed a 5-file, 182-passed command pulled in from a broader sanity check, not this PR's own scope)pylintoncascade.py— cleancfs validate --artifact architecture/features/traceability-validation.md— passes (pre-existing, unrelatedtoc-missing-descriptionwarning only)cfs spec-coverage --system studio --min-coverage 90 --min-file-coverage 60 --min-granularity 0.46— all thresholds metmain, not stacked on that PR