Repository navigation
Conversation
adamjohnwright
left a comment
There was a problem hiding this comment.
Thanks, this is careful work.
- I wrote this with the help of Claude, looking at the PR as well. You can address these issues or not. Hopefully they help
I spot-checked about a dozen claims against the code at the PR head and all were accurate. The gaps you found are the most useful part: FR-003's weighting was never built, FR-005 ended up a constructor argument nobody sets, the endpoint lives at /chat/guest/api/answer on beta, a refusal sends only done, and the first token takes ~5.6 s rather than 2 s. Moving the "alternatives rejected" reasoning out of the deleted plans and into the specs is right, and the FR/SC/T IDs the code cites still mean what they meant.
A few things before merging:
- Line-number references. The PR adds about 390 path:line references (e.g. csv_chroma.py:610-611). They're correct today but will drift on the next refactor, and then the specs will point at the wrong code, which is the steering problem this PR is trying to fix. Could the "Today" columns name symbols instead (reciprocal_rank_fusion, limiter_from_env), or carry one "checked at " line per table? The endpoint contracts are the exception; precise detail belongs there.
- Open items need a home. Several things are now filed as historical but were never decided or built: 002 D1–D3 (the default model), 007 D1, 006 D2 (where reactome-mcp runs), 012 Story 1 (public datasets) and 013 Story 3 (handoff to the logged-in chat). Can each one link to an issue, so they don't quietly drop?
- Deleted edge cases. The edge-case and acceptance-scenario lists were removed rather than carried over, e.g. 001's no bundle, missing st_id, zero results and budget 0. If tests cover them, fine; if not, I'd keep the lists.
- Constitution. It says retrofitting specs onto existing code "is archaeology and is not done". This PR is maintenance rather than new specs, but a sentence in the constitution saying how finished specs are kept would settle it for next time.
Minor: tasks.md is kept for 009–011 but deleted for 003/004; if that's because of the measurements in them, a line in the description would help. Also, src/util/rate_limit.py's docstring still says an answer takes 20–40 s.
Preserve feature decisions, evidence, and current contracts while removing obsolete implementation plans and redundant checklists. Clarify historical requirements and implementation outcomes so completed specs do not steer future work.
Documentation only; no application code changes.