gabriel / musehub public
explore-search-500-fix.md markdown
219 lines 10.7 KB
Raw
sha256:34035d72cef530c1ab9d6a6f53be18d803dde705fc3157617d70352a96d0747b Merge branch 'fix/wire-push-external-parent-manifest' into dev Human 8 days ago

Fix GET /explore/search 500 — wrong TemplateResponse argument order

Background

The proximate trigger

While TDD-ing the mist-Explore-filter fix (MusehubRepo.domain_id != "mist" in musehub_discover.list_public_repos/search_repos_by_text), a new regression test called GET /explore/search?type=repos&q=findable against two seeded repos and got a 500, in complete isolation, with zero mist repos involved. /explore/search had no pre-existing test coverage at all — this is not a regression from that work, it's a pre-existing, previously-undiscovered bug that TDD happened to surface.

Root cause — fully diagnosed, not a guess

fastapi.templating.Jinja2Templates.TemplateResponse (Starlette 1.0, confirmed installed) requires the signature TemplateResponse(request, name, context, ...). This module's own docstring (musehub/api/routes/musehub/_templates.py) documents exactly that:

return templates.TemplateResponse(request, "musehub/pages/foo.html", ctx)

explore_search_fragment (musehub/api/routes/musehub/ui.py:251) has two calls that use the old, unsupported two-argument order instead:

# Line ~275 — type == "commits" branch
return templates.TemplateResponse(
    "musehub/fragments/search_commit_results.html", ctx
)

# Line ~283 — type == "repos" branch (the default)
return templates.TemplateResponse(
    "musehub/fragments/search_repo_results.html", ctx
)

Because these are positional arguments, request never gets passed at all — Starlette receives the template-name string in the request parameter slot and the context dict in the name parameter slot. When it then calls into Jinja to load "the template" (actually the context dict), Jinja's loader cache key computation — cache_key = (weakref.ref(self.loader), name) (jinja2/environment.py::_load_template) — builds a tuple containing that dict. Python then rejects using that tuple as a dict key in the LRU cache's backing dict:

TypeError: cannot use 'tuple' as a dict key (unhashable type: 'dict')

This is why the error surfaces deep inside jinja2/utils.py's LRUCache.__getitem__ and looks unrelated to the actual bug at the call site — it's three layers removed from the real mistake.

Scope of the damage — confirmed by a full codebase sweep

Grepped and manually classified every TemplateResponse call site in musehub/ (74 total). Exactly these 2 use the broken argument order — every other one (72/74, across ui_docs.py, ui_symbols.py, ui_proposals.py, ui_new_repo.py, ui_user_profile.py, ui_domains.py, ui_legal.py, htmx_helpers.py, and elsewhere) correctly passes request first. This is a narrow, fully-scoped, two-call-site bug — not a systemic pattern — but its blast radius is total: every non-empty search query on the Explore page has 500'd since whenever this file was last touched, silently, because nothing ever tested it.

Why this matters beyond "fix two lines"

Two lines fixes the crash. But the reason it shipped broken and stayed broken is that /explore/search had zero test coverage — the exact failure mode this workspace's TDD convention exists to prevent. A two-line fix with no regression test is how this recurs the next time someone edits this file. gabriel asked for comprehensive, load-bearing, multi-phase — so this plan also closes the systemic gap: a guard that would have caught this bug on day one, and real E2E coverage for a feature that currently has none.

Goal — definition of done

  1. GET /explore/search?type=repos&q=... returns 200 with correctly rendered results for any query that matches ≥1 public repo.
  2. GET /explore/search?type=commits&q=... returns 200 with correctly rendered results for any query that matches ≥1 commit.
  3. Both paths correctly exclude mist-backed repos (domain_id="mist"), closing the loose end from the mist-Explore-filter work — that fix's own test had to route around this bug by testing the service function directly instead of via HTTP; this issue lets that gap close for real.
  4. A repo-wide structural guard exists so this exact bug class (a TemplateResponse call missing request as its first argument) is caught automatically if it's ever reintroduced anywhere in musehub/api/routes/musehub/, not just in these two spots.
  5. Every deliverable is TDD'd: a red test proving the crash (or the missing behavior) written first, then made green.

Phases

Ordered by load-bearing dependency — the crash must be fixed before any positive-path behavior can be tested end-to-end, and the systemic guard is only meaningful once the two known instances are gone (otherwise it would immediately fail on the very bug it's meant to prevent).

Phase 1 — Fix the confirmed crash

  • [ ] ESB_01 — Red test: GET /explore/search?type=repos&q=<term matching a seeded public repo> currently returns 500. Fix: change ui.py's repos-branch TemplateResponse call to TemplateResponse(request, "musehub/fragments/search_repo_results.html", ctx). Green: same test now asserts 200 and the repo name appears in the response body.
  • [ ] ESB_02 — Red test: GET /explore/search?type=commits&q=<term matching a seeded commit> currently returns 500 (verify this independently — the commits branch shares the same bug pattern but has not been confirmed to have a second, different bug once the TemplateResponse call is fixed; if search_commit_results.html or musehub_repository.global_search has its own issue, it surfaces here and gets its own red-then-green cycle). Fix: same argument-order correction on the commits-branch call.

Exit gate: Both search types return 200 with real matching data in a fresh test run; python3 -m pytest tests/test_musehub_discover.py -k explore_search (new tests from this phase) green; zero regressions in the existing 300+ musehub test suite.

Phase 2 — Comprehensive regression guard against this bug class

  • [ ] ESB_03 — A repo-wide structural test (AST-based, not regex) that parses every musehub/api/routes/musehub/*.py file, finds every ast.Call node whose function is an attribute access named TemplateResponse, and asserts the first positional argument is always a Name node with id == "request" — never a string literal. This is the guard that would have caught ESB_01/ESB_02 on day one. Runs as a normal pytest test (tests/test_template_response_call_shape.py or similar), not a separate lint step, so it's part of the same muse code test gate every other regression is.
  • [ ] ESB_04 — Confirm ESB_03 actually catches the bug it's designed for: temporarily reproduce the exact broken call shape in a throwaway fixture file within the test (not in production code) and assert the guard flags it — proving the detector works, not just that it passes today by coincidence.

Exit gate: ESB_03 passes against the current (fixed) codebase; ESB_04 proves the guard is a real detector, not a tautology; re-run after Phase 1's fix confirms both known bad call sites are clean.

Phase 3 — Real end-to-end coverage for a previously-untested feature

  • [ ] ESB_05 — E2E test: seed several public repos with varying names/descriptions/tags, verify GET /explore/search?type=repos returns only genuinely matching repos, in the documented order (by commit count, per search_repos_by_text's existing order_by(desc(commit_count_col))), with mist-backed repos (domain_id="mist") correctly excluded even when their filename would otherwise match the query.
  • [ ] ESB_06 — E2E test: seed commits across multiple repos, verify GET /explore/search?type=commits groups matches by repo and truncates to 5 rows per repo per the template's match.matches[:5] slice, with correct repo links.
  • [ ] ESB_07 — Empty-state E2E test: a query matching nothing returns 200 with the "No repos found" / "No commits found" empty-state copy, not an error and not an empty 200 body.
  • [ ] ESB_08 — Short-query guard test: q under 2 characters (or empty) returns 200 with an empty body without touching the database at all — confirms the existing if not safe_q or len(safe_q) < 2: return Response(content="") early-return still works correctly once the rest of the function is exercised by real tests for the first time.

Exit gate: /explore/search has real, positive-path, negative-path, and edge-case test coverage for both search types — a feature that previously had none.

Phase 4 — Live verification on staging

  • [ ] ESB_09 — Deploy to staging. Using real repo data (gabriel-muse, Knowtation, scooling-lab, musehub, muse-zsh, muse), run real GET /explore/search?type=repos&q=... and ?type=commits&q=... queries against https://staging.musehub.ai and confirm 200 responses with correctly rendered, correctly filtered results — not just green tests.
  • [ ] ESB_10 — Manually exercise the search box in a real browser against staging: type a query, confirm the HTMX swap into #search-results renders without a flash of broken markup or a network-tab 500.

Exit gate: Live, in-browser confirmation on real staging data — not just the test suite.

Acceptance criteria (whole-issue gate)

  • Both /explore/search search types (repos, commits) work end-to-end against real data, verified live on staging, not just unit-tested.
  • Mist-backed repos are excluded from repo search results, verified via a real HTTP-level test (closing the workaround from the mist-Explore-filter fix, which could only test this at the service-function level because this bug blocked the HTTP path).
  • A structural regression guard exists and is proven to actually detect the bug class it targets, not just pass by coincidence.
  • Full TDD coverage: every deliverable above has a red-then-green test.
  • Zero regressions in the existing test suite.

Out of scope (explicit, for future issues)

  • Any change to the actual search ranking/matching logic in search_repos_by_text or global_search — this issue is about the broken response path, not search quality or relevance tuning.
  • The Recent Mists profile preview component (tracked separately in musehub#118) — unrelated feature, filed independently.
  • A general lint rule or pre-commit hook enforcing TemplateResponse argument order project-wide outside of musehub/api/routes/musehub/ESB_03's guard is scoped to the UI route directory where this bug class can actually occur; broader tooling (e.g. a custom flake8/ruff rule) is a separate, larger investment not needed to close this issue.
File History 3 commits
sha256:34035d72cef530c1ab9d6a6f53be18d803dde705fc3157617d70352a96d0747b Merge branch 'fix/wire-push-external-parent-manifest' into dev Human 8 days ago
sha256:fc04e4cae9e1774d6a21b65c45daeed0e6787eb581d13aa1b03bfe9384a34226 Merge branch 'fix/two-column-scroll-layout' into dev Human 8 days ago
sha256:408916fc5973ba59c6e4eebaa80ebdcc801c0a63205651e25009d11548f79454 chore: bump version to 0.2.0.dev2 — nightly.2, matching muse Sonnet 4.6 patch 11 days ago