Repository navigation
feat(registry): record whether a research result can still be trusted - #22
Merged
Merged
Conversation
Four P0 fixes just landed, each of which changes numerical output: BUG-22 (fundamentals visible before publication), BUG-23 (ML CV split on rows rather than dates), BUG-21 (walk-forward OOS window covered the training period), and BUG-03 (every transaction cost charged at half). `NO_LOOKAHEAD_CONTRACT.md` already prescribes what to do about that -- its closing section says, in order: file the bug, fix it, update the list of implemented locations, and **mark the affected versions of prior results**. Step four was not possible: the Registry recorded runs but had no field for whether a run's output is still trustworthy, so "which results does this fix invalidate?" could only be answered by reading commit messages. Adds to `research_runs`: validity valid | affected | invalidated | superseded (default 'valid') affected_by JSON list of causes, e.g. ["BUG-03", "BUG-22"] and a migration, because CREATE TABLE IF NOT EXISTS silently leaves an existing table alone -- the live data/trading.db has 41 runs that predate these columns. Store API: mark_validity(run_id, validity, reason) -> bool list_affected(reason=None) -> runs that are no longer 'valid' Reasons accumulate and are de-duplicated, so a run can be affected by several findings and re-marking is idempotent. Unknown validity states raise rather than being stored. CLI, since the project is CLI-first and an API alone would not be reachable: python main.py runs list [--limit N] python main.py runs affected [--reason BUG-03] python main.py runs mark <run_id> <state> [--reason BUG-03] The marking of existing runs is deliberately NOT done here -- that requires deciding which of the 41 historical runs each fix actually affects, which is a research judgement about those results, not a mechanical update. Tests cover the migration path from a pre-existing table (the case most likely to break silently), the accumulate-and-dedupe behaviour, filtering by cause, and rejection of unknown states. All 11 use tmp_path, so they never touch the real Registry. Noted while writing them, and NOT fixed here: `begin_run` derives run_id from `int(time.time())` plus the input hash, so two runs with identical inputs inside the same second collide on the primary key and raise IntegrityError. 1329 passed, 1 skipped. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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
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.
The four P0 fixes that just landed (#18 BUG-22, #19 BUG-23, #20 BUG-21, #21 BUG-03) each change numerical output. That makes "which existing results are now wrong?" load-bearing — and it had no machine-readable answer.
The project already decided this question, once
NO_LOOKAHEAD_CONTRACT.md's closing section prescribes the procedure:Step 3 was not possible:
research_runshad no field for whether a run's output is still trustworthy, so answering it meant reading commit messages — the failure mode Truth First exists to prevent.What this adds
validityvalid|affected|invalidated|superseded(defaultvalid)affected_by["BUG-03", "BUG-22"]Plus a migration —
CREATE TABLE IF NOT EXISTSsilently leaves an existing table alone, and the livedata/trading.dbholds 41 runs predating these columns. Verified against a hand-built pre-migration table, including that existing rows survive with a sensible default.Reasons accumulate and de-duplicate, so re-marking is idempotent and a run can carry several causes. Unknown states raise rather than being stored.
Deliberately not done here
Marking the 41 existing runs. That needs a decision about which historical results each fix actually affects — BUG-03 touches every backtest, BUG-22 only fundamental factors, BUG-23 only ML signals. That is a research judgement about those results, not a mechanical update, and doing it silently would repeat the class of mistake these bugs represent.
A separate defect found while writing the tests
begin_runderivesrun_idfromint(time.time())plus the input hash, so two runs with identical inputs inside the same second collide on the primary key and raiseIntegrityError. Not fixed here; the fixture sidesteps it with a comment saying why.Tests
11 tests on
tmp_path, never touching the real Registry: the migration path (most likely to break silently), accumulate-and-dedupe, filtering by cause, and rejection of unknown states.1329 passed, 1 skipped.🤖 Generated with Claude Code