Skip to content

fix(search): the documented way to enable local search built a worse index than none - #2187

Merged
Ikalus1988 merged 1 commit into
mainfrom
fix/sag-export-freshness
Sep 25, 2026
Merged

Ikalus1988 merged 1 commit into
mainfrom
fix/sag-export-freshness

Conversation

@Ikalus1988

@Ikalus1988 Ikalus1988 commented Sep 25, 2026 •

Copy link
Copy Markdown
Owner

User description

Fixes the half of #2185 that can be fixed without deciding where the export step belongs.

The trap, measured: data/sag.db is gitignored, so a fresh clone builds it from the tracked
data/okf/lessons.jsonl — last written 2026-07-07, naming 177 of 453 lesson files. The search
handler prefers SAG over the complete BM25 path, and its hint said only
Run: python3 scripts/build_sag_index.py. So following the documentation produced an index covering 39%
of the corpus, preferred over the one covering all of it:

probe query from a lesson the stale export cannot contain
  stale (194 lessons) -> 0 hits
  fresh (411 lessons) -> 1 hit

Two changes: the hint now names export_okf.py && build_sag_index.py in that order (the second reads
what the first writes), and build_sag_index.py measures the export against the corpus and warns when it
covers less than COVERAGE_FLOOR — 0.8, sitting in the measured gap between a fresh export (91%) and the
committed one (39%). It warns rather than refuses: an old checkout is legitimate, and a build script that
will not run is a worse trap than the one it closes.

Evidence: python3 -m pytest tests/ -q → 2241 passed, 15 skipped. Against the committed export
the builder prints the warning; against a fresh export it is silent.

tests/test_sag_export_freshness.py (12 tests) pins the measurement, the boundary, the silent case
outside a checkout, and both wirings. 4 mutations red: the call removed from build_index · threshold
below the stale population · denominator counting READMEs · the hint losing the export step.

One of those four was green at first — every test called the warning function directly, so deleting its
call from build_index passed — which is why there is now a test that goes through build_index itself.
Same hole as the --kv-only early return (#1822) and the unread exit output (#2183): the function was
tested, the wiring was not.

Still open in #2185: nothing regenerates that tracked file in CI, so it will drift again. That needs a
decision (give it a writer in the daily job like data/lessons.json, or stop tracking it) rather than a
change I make unilaterally.


PR Type

Bug fix, Tests


Description

  • Hint names export_okf.py && build_sag_index.py in order

  • build_sag_index.py warns when export covers < COVERAGE_FLOOR

  • 12 tests pin measurement, boundary, wiring, and hint order


Diagram Walkthrough

flowchart LR
  Old["Old hint: build_sag_index.py only"]
  Trap["Stale export → 39% index, preferred over BM25"]
  Hint["New hint: export_okf.py && build_sag_index.py"]
  Build["build_index measures export vs corpus"]
  Warn["Warn when coverage < 0.8"]
  Old --> Trap
  Hint --> Build
  Build --> Warn
Loading

File Walkthrough

Relevant files
Bug fix
search.py
hint names both export and build steps in order                   

misakanet/server/handlers/search.py

+9/-2     
build_sag_index.py
warn when OKF export coverage is below 0.8 floor                 

scripts/build_sag_index.py

  • Added LESSONS_DIR and COVERAGE_FLOOR = 0.8 constants
  • Added corpus_lesson_files() excluding READMEs
  • Added export_coverage() returning (covered, total)
  • Added warn_if_export_is_stale() printing cause + remediation
  • Wired the warning into build_index() after record load
+48/-0   
Tests
test_sag_export_freshness.py
pin freshness measurement, boundary, and hint wiring         

tests/test_sag_export_freshness.py

  • 12 new tests pinning corpus denominator, complete export, stale
    fixture
  • Parametrized boundary test across COVERAGE_FLOOR
  • No-corpus silent case and warning-content assertions
  • End-to-end tests through real build_index and handle_search
  • Hint-order assertion via _no_index_hint behavioral driver
+169/-0 

…index than none (#2185)

`data/sag.db` is gitignored, so every fresh clone builds it — from `data/okf/lessons.jsonl`, a **tracked**
file with **no writer in CI**. Measured 2026-09-25:

    data/okf/lessons.jsonl   last written 2026-07-07 (194 rows)
    a fresh export_okf.py    411 rows
    the committed export names 177 of 453 lesson files (61% missing)

and `misakanet/server/handlers/search.py` prefers SAG over the complete BM25 path
(`if HAS_SAG and not explain:`), while the hint it returns when no index exists says only:

    "Run: python3 scripts/build_sag_index.py to enable BM25/SAG search"

`build_sag_index.py` only complains about a missing export (`Run export_okf.py first` is in the
*file-not-found* branch), and the tracked file exists — so following the documented remedy produced an
index covering 39% of the corpus, preferred over the one that covers all of it. Demonstrated with a probe
query taken from a lesson the stale export cannot contain:

    stale (194 lessons) -> 0 hits
    fresh (411 lessons) -> 1 hit

Two changes, both halves of the trap:

* **the hint names both steps, in order** (`export_okf.py && build_sag_index.py`), because the second
  command reads what the first one writes;
* **`build_sag_index.py` measures the export against the corpus** and prints a warning naming the cause
  and the command when it covers less than `COVERAGE_FLOOR` of it. Threshold measured, not chosen: a
  fresh export covers 91%, the committed one covered 39%. It warns rather than refuses — an old
  checkout or a filtered export is legitimate, and a build script that will not run is a worse trap than
  the one it closes.

Evidence (2026-09-25): `python3 -m pytest tests/` → 2241 passed, 15 skipped. Against the committed
export the builder now prints the warning; against a fresh one it stays silent.

`tests/test_sag_export_freshness.py` (12 tests) pins the measurement, the boundary, the silent case
outside a checkout, and both wirings; 4 mutations go red: the call removed from `build_index` · the
threshold dropped below the stale population · the denominator counting READMEs · the hint losing the
export step.

The first of those four was green on the first attempt and is the reason the file now has
`test_building_from_a_stale_export_warns_at_the_point_of_use`: every other test called the warning
function directly, so deleting its call from `build_index` left the suite passing. The function was
tested; the wiring was not — the same shape of hole as the `--kv-only` early return in #1822 and the
`exit` output in #2183.

What this does NOT fix, and #2185 stays open for it: nothing regenerates the tracked export in CI, so it
will drift again. That wants a writer in the daily job (the same treatment `data/lessons.json` gets) or a
decision to stop tracking it — an owner call about where the export step belongs, not a code change I
should make on my own.

Signed-off-by: misakanet-bot <bot@misakanet.dev>
@github-actions

Copy link
Copy Markdown
Contributor

PR Genius Analysis

  • Risk Level: medium_risk
  • PR Size: +226/-2 (228 lines, medium)
  • Impact: 3 files changed (misakanet, scripts, tests)
  • Rules: 8 core + 5 repo-specific

Checklist

  • ci_passing (PENDING) — checks are still running
  • dco_signoff (PASS) — all commits signed
  • tests_updated (PASS) — 1 test file(s) changed
  • issue_reference (PASS) — linked issue found

Anti-Patterns Detected

  • None detected

Suggestions

  • No structural changes suggested; proceed with normal review.

@cloudflare-workers-and-pages

Copy link
Copy Markdown
Contributor

Deploying with  Cloudflare Workers  Cloudflare Workers

The latest updates on your project. Learn more about integrating Git with Workers.

Status Name Latest Commit Preview URL Updated (UTC)
✅ Deployment successful!
View logs
misakanet-web 15da730 Commit Preview URL

Branch Preview URL
Sep 25 2026, 03:26 AM

@github-actions

Copy link
Copy Markdown
Contributor

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ❌

2185 - Partially compliant

Compliant requirements:

  • Tighten the no-index hint so it instructs users to run export_okf.py before build_sag_index.py (in that order).

Non-compliant requirements:

Requires further human verification:

  • Whether the soft warning is the right design choice vs. a hard refusal — the PR justifies the softer stance as "a build script that refuses to run is a worse trap than the one it closes", which is a judgment call.

1822 - Not compliant

Non-compliant requirements:

  • Make the tool-count gate read its expected value from an independent source.
  • Have scripts/doctor.py's remote-reachability check actually run in CI and tighten the "reachable" criterion.
  • Make /api/health's top-level ok flag reflect sustained sub-system failures.
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Coverage denominator undercounts templates

The inline comment claims the export "skips…READMEs and templates" (47 files), but corpus_lesson_files() only excludes p.name == "README.md". Templates that the export filters out remain in the corpus set, so len(corpus & paths) is smaller than the export size. In a corpus with many templates (the exact threshold depends on LESSONS_DIR.rglob("*.md") content), a fresh export could still cover <0.8 and trigger the new warning. The fix is either to filter templates in corpus_lesson_files() (matching the export's actual behavior — verify against scripts/export_okf.py) or to compute coverage against the export's intended denominator. Worth confirming the actual template count before merging, since export_okf.py is not in this diff.

# The export is a *tracked* file with no writer in CI (issue #2185), so it can be arbitrarily old
# while looking perfectly healthy: it exists, it parses, and this script is happy to build an index
# from it. Measured 2026-09-25 — a fresh export covers 411 of the 458 `lessons/**/*.md` files (the
# 47 it skips are READMEs and templates), and the version committed on 2026-07-07 covered 179. So
# the threshold sits in a wide gap, and anything below it is a stale export rather than a corpus
# that legitimately shrank.
COVERAGE_FLOOR = 0.8


def corpus_lesson_files() -> set[str]:
    """The `lessons/**/*.md` files, excluding READMEs — what a fresh export is expected to cover."""
    if not LESSONS_DIR.is_dir():
        return set()
    return {p.relative_to(REPO_ROOT).as_posix() for p in LESSONS_DIR.rglob("*.md")
            if p.name != "README.md"}


def export_coverage(records: list[dict]) -> tuple[int, int]:
    """(covered, corpus size) — how much of the corpus the export actually names."""
    corpus = corpus_lesson_files()
    if not corpus:
        return (0, 0)
    paths = {str(r.get("path") or "") for r in records}
    return (len(corpus & paths), len(corpus))
Hardcoded staleness numbers in source comment

The new comment cites "measured 2026-09-25…last written 2026-07-07 and covered 179 of 458 lessons". These specific figures will drift (the PR description already gives a different number — 177 of 453) and the comment will quietly mislead anyone reading the file after the corpus grows or the export is regenerated. Either drop the counts (keep only the issue reference + explanation) or move them to a single source-of-truth doc the script could also consult.

# Two steps, in this order. `build_sag_index.py` reads the *tracked* OKF export
# (`data/okf/lessons.jsonl`), and that file has no writer in CI: measured
# 2026-09-25 it was last written 2026-07-07 and covered 179 of 458 lessons, so
# running only the second command built an index missing 61% of the corpus — and
# SAG is preferred over the complete BM25 path, which made recall *worse* than
# building nothing. See issue #2185.
"Run: python3 scripts/export_okf.py && python3 scripts/build_sag_index.py"
" to enable BM25/SAG search (the export refreshes data/okf/lessons.jsonl,"
" which build_sag_index.py reads)"

@Ikalus1988
Ikalus1988 merged commit b36806e into main Sep 25, 2026
32 of 37 checks passed
@github-actions

Copy link
Copy Markdown
Contributor

🎉 Merged — Thank you!

Your contribution has been merged into main.

PR: #2187 — fix(search): the documented way to enable local search built a worse index than none

What's next:

  • Your code is now part of MisakaNet's failure-lesson corpus (now 411 lessons)
  • Feel free to pick up another issue labeled good first issue or status: competition
  • Questions? Ask in this thread or open a Discussion

Welcome to the MisakaNet contributor community! 🧠

@github-actions

Copy link
Copy Markdown
Contributor

✅ Merged! Thanks again, @Ikalus1988.

fix(search): the documented way to enable local search built a worse index than none (+226 lines, 3 files)

Quick question — did any MisakaNet lesson help you this time?
→ Share feedback

No need to reply if nothing comes to mind. ⚡

@github-actions

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

No code suggestions found for the PR.

@Ikalus1988

Copy link
Copy Markdown
Owner Author

🧾 Audit Report — PR #2187 (15da730)

📊 Quality Score

⚠️ Quality score unavailable; continuing with hard gates.

🔏 DCO Audit

✅ All commits signed-off.

📏 PR Size

Metric Value
Files Changed 3
Lines Added 226

🔐 Secret Scan

✅ No hardcoded secrets detected.

📦 Dependency Audit

⏭️ Skipped; no Python/JS dependency files changed.

🧪 Test Suite

✅ PASS — 56% coverage

📋 Lesson Schema

✅ All lessons valid.

⚖️ Verdict

✅ All gates passed. Ready for merge.


Scope: full | Triggered by 15da730 | View run

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant