Repository navigation
Spec 009: plan, research, contracts and tasks — the Spec Kit stages that had never run - #228
Merged
Merged
Conversation
HybridRetriever implements retrieval twice -- retrieve_documents and aretrieve_documents, the second gathering coroutines so collections are queried concurrently. Nothing exercised the async one, and that is the one the application serves through. bin/retrieval_baseline, the tool the constitution names for measuring retrieval changes, drives the sync one. If they drift, every measurement is of a path no user takes. They do not drift today: same documents, same order, checked across three query shapes. Two things this test had to be argued out of claiming. It first reported the paths returning different document sets -- that was FakeEmbeddings returning a fresh random vector per call, so the same query embedded differently on the second run. DeterministicFakeEmbedding fixes it, and the near-miss is recorded in the docstring because the false result is more instructive than the true one. And it does not cover the per-collection cap. Deterministic fake vectors carry no relation to the text, so every query retrieves much the same set and fusion lands on exactly the cap however many queries are given -- changing the cap on one path alone stays invisible. Tried at 12 and 60 documents per collection with one, three and seven queries; ten per collection every time. That limit is written into the docstring rather than left as apparent coverage, and covering it needs real embeddings, which is retrieval_baseline's job. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The new equivalence test failed on GitHub and passed here. BM25 tokenises with word_tokenize(..., language="english"), which needs punkt_tab; the Dockerfile installs it at build time and the CI test job never did. So the suite passed on any machine where a developer had downloaded it once and failed on a clean one -- the classic shape, and it was my test that exposed it rather than caused it. Any future test that drives retrieval would have hit the same wall. CI now fetches the same resource the Dockerfile does, so the async path is genuinely covered there rather than skipped. The test also skips with a message naming the download command when the resource is absent, so a developer without it gets that instead of a LookupError raised from inside nltk. Checked by patching nltk.data.find to raise. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Every spec here had a spec.md and nothing else, so /speckit-plan, /speckit-tasks and /speckit-analyze had never run -- and /speckit-analyze cannot run at all without plan.md and tasks.md. 008 shipped with no cross-artifact check. 009 now has the full set: plan.md with the constitution gates written out, research.md, data-model.md, contracts/, quickstart.md and 27 tasks. Two things came out of doing it properly rather than by hand. The first is a design constraint found by reading the code instead of assuming: the RAG chain is built once at profile construction and closes over a single HybridRetriever, so a per-question selection cannot be a constructor argument. Rebuilding per question would re-tokenise 17,004 documents for `reactions` alone. It travels through RunnableConfig["configurable"] instead -- which base.py already uses for enable_postprocess, so the mechanism is established here rather than invented. The second is what the analysis caught: D1 and D2 were still phrased as "Recommendation" while plan.md and data-model.md had already built on them as decided. They are recorded as decisions now, with a note saying that is what happened, rather than the spec being quietly rewritten to match its own downstream documents. The plan also writes down the gate that is not met: four of five collections have no sweep question that fails if routing stops searching them. That is Phase 2 and it blocks the feature, because until it closes the acceptance criterion cannot detect the failure this feature can cause. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Closing the process gap before building anything. Every spec in this repo had a
spec.mdand nothing else, so/speckit-plan,/speckit-tasksand/speckit-analyzehad never run — and analyze cannot run at all withoutplan.mdandtasks.md. Spec 008 shipped with no cross-artifact check.009 now has the full set:
plan.md,research.md,data-model.md,contracts/intent_classifier.md,quickstart.md, and 27 tasks.What doing it properly turned up
A design constraint, found by reading the code rather than assuming. The RAG chain is built once at profile construction and closes over a single
HybridRetriever, so a per-question collection selection cannot be a constructor argument. Rebuilding per question would re-tokenise 17,004 documents forreactionsalone — that's startup work, not per-message work.It travels through
RunnableConfig["configurable"]instead. Not invented for this:base.pyalready readsconfig["configurable"].get("enable_postprocess"), so the mechanism is established here, and its failure mode is a missing key → "all collections" → today's behaviour.An inconsistency the analysis caught. D1 and D2 were still phrased as "Recommendation", while
plan.mdanddata-model.mdhad already built on them as decided. They're recorded as decisions now — with a note saying that's what happened, rather than the spec being quietly rewritten to match its own downstream documents.The gate that is not met, written down
Four of the five collections have no sweep question that fails if routing stops searching them. That's Phase 2, and it blocks the feature: until it closes, the acceptance criterion cannot detect the failure this feature can cause.
Phase 2 is worth landing on its own regardless — it closes a hole that exists in the gate today, whether or not routing is ever built. If routing later proves to cost more recall than it saves, that PR still stands.
Constitution gates, evaluated rather than asserted
aretrieve_documents; acceptance runsanswer-sweepthrough the whole graph, and T017 asserts both paths filter alikelist_chroma_subdirectories()on the live bundle, not a literalNo code changes. Docs only.
🤖 Generated with Claude Code