From fcd335f2189b43e28d423ed382dacc040de9f22c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 22:48:27 +0000 Subject: [PATCH 1/5] fix(engine): reject non-positive tick size at CSV parse A window whose ticks all had size 0 produced zero total volume, making OFI (`(buy-sell)/(buy+sell)`) and VWAP (`price_size_sum/total_volume`) evaluate to NaN, which was then written to the metrics table. The synthetic generator never emits 0, so this was latent but reachable with any hand-crafted tape. Reject `size == 0` at parse time with a serde field validator, so the error carries row context via the existing csv error path and no metrics code has to guard the denominator. `MetricsBucket::ofi`'s "denominator is never zero" doc claim is now actually guaranteed. Negative inputs were already rejected by `u32` parsing; both cases are covered by new tests. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012nrxeE8qNcKXkYovFWkxGH --- engine/src/metrics.rs | 5 +++-- engine/src/tick.rs | 39 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 2 deletions(-) diff --git a/engine/src/metrics.rs b/engine/src/metrics.rs index 636f7da..5ee6427 100644 --- a/engine/src/metrics.rs +++ b/engine/src/metrics.rs @@ -24,8 +24,9 @@ pub struct MetricsBucket { impl MetricsBucket { /// Order-flow imbalance: `(buy − sell) / (buy + sell)`, in `[-1, 1]`. /// - /// Buckets only exist when at least one tick landed in them, so the - /// denominator is never zero. + /// Buckets only exist when at least one tick landed in them, and tick sizes + /// are validated positive when the tape is parsed (`tick::read_csv`), so the + /// total volume — and thus the denominator — is never zero. #[must_use] pub fn ofi(&self) -> f64 { let buy = self.buy_volume as f64; diff --git a/engine/src/tick.rs b/engine/src/tick.rs index ca344f4..92fdbc6 100644 --- a/engine/src/tick.rs +++ b/engine/src/tick.rs @@ -8,6 +8,10 @@ use serde::Deserialize; pub struct Tick { pub timestamp_ns: i64, pub price: f64, + /// Contracts traded. Validated positive when the tape is parsed so a + /// window's total volume is never zero (which would make OFI and VWAP + /// divide by zero). + #[serde(deserialize_with = "positive_size")] pub size: u32, pub aggressor: Aggressor, } @@ -21,6 +25,23 @@ pub enum Aggressor { Sell, } +/// Deserialize a tick `size`, rejecting zero. +/// +/// A window whose ticks all have zero size would have zero total volume, so +/// order-flow imbalance and VWAP would divide by zero. Rejecting it at parse +/// time keeps those metrics well-defined. Negative inputs are already rejected +/// by `u32` parsing, before this runs. +fn positive_size<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + let size = u32::deserialize(deserializer)?; + if size == 0 { + return Err(serde::de::Error::custom("size must be positive, got 0")); + } + Ok(size) +} + /// Read a tick tape from a CSV file with the header /// `timestamp_ns,price,size,aggressor`. pub fn read_csv(path: &Path) -> Result, csv::Error> { @@ -67,4 +88,22 @@ mod tests { let csv = "timestamp_ns,price,size,aggressor\n1000,21400.00,3,X\n"; assert!(read_from(csv.as_bytes()).is_err()); } + + #[test] + fn rejects_zero_size() { + let csv = "timestamp_ns,price,size,aggressor\n\ + 1000,21400.00,3,B\n\ + 2000,21400.25,0,S\n"; + let err = read_from(csv.as_bytes()).unwrap_err(); + assert!( + err.to_string().contains("size must be positive"), + "unexpected error: {err}" + ); + } + + #[test] + fn rejects_negative_size() { + let csv = "timestamp_ns,price,size,aggressor\n1000,21400.00,-1,B\n"; + assert!(read_from(csv.as_bytes()).is_err()); + } } From 726f398000d90ed5a6c0daf5a7ad08f0e14ada52 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 22:48:38 +0000 Subject: [PATCH 2/5] fix(backtest): reject exit-before-entry on partial journal updates The PATCH body validator only enforced `exited_at_ns >= entered_at_ns` when both fields arrived together, so a partial update setting just one timestamp could persist an exit before its entry (the stored value the Pydantic validator can't see). `journal.update_entry` now merges the update with the stored row before writing and raises `InvalidEntryUpdate` when the result would be out of order; the endpoint maps it to 422, the same status the rule already returns on POST. Nothing is written when the check fails. Regression tests cover both single-timestamp directions at the journal and API layers. Also folds in two form cleanups to the same module: `insert_entry` and `import_csv` now build the INSERT from one shared `_INSERT_SQL` constant, and the bulk import stamps `created_at_ns` per row (matching the single-insert path) instead of sharing one batch timestamp. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012nrxeE8qNcKXkYovFWkxGH --- backtest/src/backtest/api.py | 11 +++++- backtest/src/backtest/journal.py | 66 +++++++++++++++++++++++--------- backtest/tests/test_api.py | 14 +++++++ backtest/tests/test_journal.py | 28 ++++++++++++++ 4 files changed, 98 insertions(+), 21 deletions(-) diff --git a/backtest/src/backtest/api.py b/backtest/src/backtest/api.py index e6d7469..7473926 100644 --- a/backtest/src/backtest/api.py +++ b/backtest/src/backtest/api.py @@ -179,8 +179,15 @@ def get_journal_entry(entry_id: int) -> dict[str, Any]: @app.patch("/journal/{entry_id}") def patch_journal_entry(entry_id: int, patch: JournalEntryPatch) -> dict[str, Any]: - """Apply a partial update to a journal entry, returning the updated row.""" - updated = journal.update_entry(_db_path(), entry_id, patch.model_dump(exclude_unset=True)) + """Apply a partial update to a journal entry, returning the updated row. + + Returns 422 when the update would put the exit before the entry once applied + to the stored row (checked even if only one timestamp is in the body). + """ + try: + updated = journal.update_entry(_db_path(), entry_id, patch.model_dump(exclude_unset=True)) + except journal.InvalidEntryUpdate as exc: + raise HTTPException(status_code=422, detail=str(exc)) from exc if updated is None: raise HTTPException(status_code=404, detail=f"journal entry {entry_id} not found") return updated diff --git a/backtest/src/backtest/journal.py b/backtest/src/backtest/journal.py index 1b14b16..24e938e 100644 --- a/backtest/src/backtest/journal.py +++ b/backtest/src/backtest/journal.py @@ -74,6 +74,21 @@ ) """ +# INSERT for one row: the caller-supplied columns plus the managed created_at_ns. +_INSERT_SQL = ( + f"INSERT INTO journal_entries ({', '.join(_INPUT_COLUMNS)}, created_at_ns) " + f"VALUES ({', '.join(['?'] * (len(_INPUT_COLUMNS) + 1))})" +) + + +class InvalidEntryUpdate(ValueError): + """Raised when a partial update would leave the stored entry inconsistent. + + Specifically, when the resulting ``exited_at_ns`` would fall before + ``entered_at_ns`` once the update is applied to the stored row. The API maps + this to a 422. + """ + def init_db(conn: sqlite3.Connection) -> None: """Create the ``journal_entries`` table if it does not already exist.""" @@ -104,16 +119,10 @@ def insert_entry(path: str | Path, entry: dict[str, Any]) -> dict[str, Any]: must be present or SQLite raises ``IntegrityError``. """ values = tuple(entry.get(col) for col in _INPUT_COLUMNS) - created_at_ns = time.time_ns() - placeholders = ", ".join(["?"] * (len(_INPUT_COLUMNS) + 1)) with sqlite3.connect(path) as conn: conn.row_factory = sqlite3.Row init_db(conn) - cursor = conn.execute( - f"INSERT INTO journal_entries ({', '.join(_INPUT_COLUMNS)}, created_at_ns) " - f"VALUES ({placeholders})", - (*values, created_at_ns), - ) + cursor = conn.execute(_INSERT_SQL, (*values, time.time_ns())) stored = conn.execute( f"SELECT {', '.join(_ROW_COLUMNS)} FROM journal_entries WHERE id = ?", (cursor.lastrowid,), @@ -145,6 +154,12 @@ def update_entry(path: str | Path, entry_id: int, fields: dict[str, Any]) -> dic Only recognised, mutable columns in ``fields`` are written; ``id`` and ``created_at_ns`` are never touched. Returns ``None`` if the entry does not exist. An empty update is a no-op that returns the current row. + + The time-ordering rule (``exited_at_ns`` >= ``entered_at_ns``) is enforced + against the *stored* row: a partial update touching only one endpoint is + checked against the other as it already stands, raising + :class:`InvalidEntryUpdate` if the result would put the exit before the + entry. Nothing is written in that case. """ updates = {col: value for col, value in fields.items() if col in _UPDATABLE_COLUMNS} if not updates: @@ -154,15 +169,21 @@ def update_entry(path: str | Path, entry_id: int, fields: dict[str, Any]) -> dic return None assignments = ", ".join(f"{col} = ?" for col in updates) with sqlite3.connect(path) as conn: + conn.row_factory = sqlite3.Row try: - cursor = conn.execute( - f"UPDATE journal_entries SET {assignments} WHERE id = ?", - (*updates.values(), entry_id), - ) + current = conn.execute( + "SELECT entered_at_ns, exited_at_ns FROM journal_entries WHERE id = ?", + (entry_id,), + ).fetchone() except sqlite3.OperationalError: return None - if cursor.rowcount == 0: + if current is None: return None + _check_times_ordered(updates, current) + conn.execute( + f"UPDATE journal_entries SET {assignments} WHERE id = ?", + (*updates.values(), entry_id), + ) return get_entry(path, entry_id) @@ -236,17 +257,11 @@ def import_csv(csv_path: str | Path, path: str | Path) -> int: """ with open(csv_path, newline="") as handle: rows = list(csv.DictReader(handle)) - created_at_ns = time.time_ns() - placeholders = ", ".join(["?"] * (len(_INPUT_COLUMNS) + 1)) - statement = ( - f"INSERT INTO journal_entries ({', '.join(_INPUT_COLUMNS)}, created_at_ns) " - f"VALUES ({placeholders})" - ) with sqlite3.connect(path) as conn: init_db(conn) for row in rows: values = tuple(_coerce(col, row.get(col)) for col in _INPUT_COLUMNS) - conn.execute(statement, (*values, created_at_ns)) + conn.execute(_INSERT_SQL, (*values, time.time_ns())) return len(rows) @@ -265,6 +280,19 @@ def _build_list_query( return f"{select}{where} ORDER BY {time_column} DESC LIMIT ?", params +def _check_times_ordered(updates: dict[str, Any], current: sqlite3.Row) -> None: + """Raise :class:`InvalidEntryUpdate` if applying ``updates`` to ``current`` + would put ``exited_at_ns`` before ``entered_at_ns``. + + Each endpoint takes its updated value if present, else the stored one; the + check is skipped when the effective exit is ``None`` (an open position). + """ + entered = updates.get("entered_at_ns", current["entered_at_ns"]) + exited = updates.get("exited_at_ns", current["exited_at_ns"]) + if entered is not None and exited is not None and exited < entered: + raise InvalidEntryUpdate("exited_at_ns must be >= entered_at_ns") + + def _read(path: str | Path, query: str, params: Sequence[Any]) -> list[dict[str, Any]]: path = Path(path) if not path.exists(): diff --git a/backtest/tests/test_api.py b/backtest/tests/test_api.py index 34a52e1..73657bb 100644 --- a/backtest/tests/test_api.py +++ b/backtest/tests/test_api.py @@ -229,6 +229,20 @@ def test_patch_journal_entry_invalid_returns_422(client): assert client.patch(f"/journal/{created['id']}", json={"side": "up"}).status_code == 422 +def test_patch_exit_before_stored_entry_returns_422(client): + created = _post_entry(client, entered_at_ns=2_000_000_000).json() # no exit stored + response = client.patch(f"/journal/{created['id']}", json={"exited_at_ns": 1_000_000_000}) + assert response.status_code == 422 + # Nothing persisted: the entry still has no exit. + assert client.get(f"/journal/{created['id']}").json()["exited_at_ns"] is None + + +def test_patch_entry_after_stored_exit_returns_422(client): + created = _post_entry(client, entered_at_ns=1_500_000_000, exited_at_ns=1_600_000_000).json() + response = client.patch(f"/journal/{created['id']}", json={"entered_at_ns": 1_700_000_000}) + assert response.status_code == 422 + + def test_delete_journal_entry(client): created = _post_entry(client).json() assert client.delete(f"/journal/{created['id']}").status_code == 204 diff --git a/backtest/tests/test_journal.py b/backtest/tests/test_journal.py index 9af553a..43b9d0a 100644 --- a/backtest/tests/test_journal.py +++ b/backtest/tests/test_journal.py @@ -179,6 +179,34 @@ def test_update_entry_empty_is_noop(db): assert same["size"] == stored["size"] +def test_update_entry_rejects_exit_before_stored_entry(db): + stored = journal.insert_entry(db, _entry(2000)) # entered 2000, exited 3000 + with pytest.raises(journal.InvalidEntryUpdate): + journal.update_entry(db, stored["id"], {"exited_at_ns": 1500}) + # Nothing persisted: the stored exit is unchanged. + assert journal.get_entry(db, stored["id"])["exited_at_ns"] == 3000 + + +def test_update_entry_rejects_entry_after_stored_exit(db): + stored = journal.insert_entry(db, _entry(1000)) # entered 1000, exited 2000 + with pytest.raises(journal.InvalidEntryUpdate): + journal.update_entry(db, stored["id"], {"entered_at_ns": 2500}) + assert journal.get_entry(db, stored["id"])["entered_at_ns"] == 1000 + + +def test_update_entry_moving_both_times_is_allowed(db): + stored = journal.insert_entry(db, _entry(1000)) # entered 1000, exited 2000 + # 5000 is after the old exit (2000), but the merged pair is consistent. + updated = journal.update_entry(db, stored["id"], {"entered_at_ns": 5000, "exited_at_ns": 6000}) + assert (updated["entered_at_ns"], updated["exited_at_ns"]) == (5000, 6000) + + +def test_update_entry_clearing_exit_is_allowed(db): + stored = journal.insert_entry(db, _entry(1000)) + updated = journal.update_entry(db, stored["id"], {"exited_at_ns": None}) + assert updated["exited_at_ns"] is None + + def test_delete_entry(db): stored = journal.insert_entry(db, _entry(1000)) assert journal.delete_entry(db, stored["id"]) is True From ada893f7e749e75c47974cac09d3fec88fb07cad Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 22:48:52 +0000 Subject: [PATCH 3/5] docs(backtest): document the coach CLI entrypoint `main()` was the only public function in the module without a docstring. Describe the CLI (optional --load seeding, then analyze and print). Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012nrxeE8qNcKXkYovFWkxGH --- backtest/src/backtest/coach.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/backtest/src/backtest/coach.py b/backtest/src/backtest/coach.py index 7a7df64..b6e739d 100644 --- a/backtest/src/backtest/coach.py +++ b/backtest/src/backtest/coach.py @@ -175,6 +175,8 @@ def _render(analysis: BehavioralAnalysis) -> str: def main(argv: list[str] | None = None) -> None: + """CLI entry point: optionally seed the journal from a CSV (``--load``), then + analyze it with Claude and print the rendered observations.""" parser = argparse.ArgumentParser(description="Analyze a trade journal with Claude.") parser.add_argument("--db", default=os.environ.get(DB_PATH_ENV, "metrics.db")) parser.add_argument("--limit", type=int, default=100) From 243b74fd6b42130eac0fdcdb2b61e89c304dd88c Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 22:48:53 +0000 Subject: [PATCH 4/5] chore(backtest): cap anthropic SDK major and drop stale uv.lock gitattribute `coach.py` rides recent Anthropic SDK surface (messages.parse with output_format, parsed_output, adaptive thinking), so a 1.0 major could break it silently. Cap it `>=0.117,<1`; leave fastapi/uvicorn lower-bounded so CI keeps exercising the latest of the stable deps. Also remove the `uv.lock linguist-vendored` line: the project builds with pip, not uv, and the file has never existed. A full uv.lock plus CI migration was considered and deferred. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012nrxeE8qNcKXkYovFWkxGH --- .gitattributes | 1 - backtest/pyproject.toml | 4 +++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/.gitattributes b/.gitattributes index 5855b59..6b9d4e2 100644 --- a/.gitattributes +++ b/.gitattributes @@ -3,4 +3,3 @@ Cargo.lock linguist-vendored package-lock.json linguist-vendored pnpm-lock.yaml linguist-vendored yarn.lock linguist-vendored -uv.lock linguist-vendored diff --git a/backtest/pyproject.toml b/backtest/pyproject.toml index 08af5de..95ed492 100644 --- a/backtest/pyproject.toml +++ b/backtest/pyproject.toml @@ -10,7 +10,9 @@ requires-python = ">=3.11" dependencies = [ "fastapi>=0.110", "uvicorn>=0.29", - "anthropic>=0.117", + # Capped below the next major: coach.py rides recent SDK surface + # (messages.parse(output_format=...), parsed_output, adaptive thinking). + "anthropic>=0.117,<1", ] [project.optional-dependencies] From 6cb2020301daa7dee2bdef80c0e2e67fa7ac814d Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 22 Jul 2026 22:48:53 +0000 Subject: [PATCH 5/5] docs: record the Phase 0-3 review decisions Note the PATCH stored-state validation in the journal API contract, and add a Design decisions entry logging the Phase 0-3 hardening review: the zero-size parse guard, the partial-update ordering check, the INSERT/ created_at cleanups, and the anthropic version cap. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_012nrxeE8qNcKXkYovFWkxGH --- README.md | 32 ++++++++++++++++++++++++++++++-- 1 file changed, 30 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 3e1268b..e071570 100644 --- a/README.md +++ b/README.md @@ -91,8 +91,10 @@ curl -X POST 'localhost:8000/journal/analyze' # Claude behavioral analys ``` `side` must be `long`/`short`, `size`/`entry_price`/`entered_at_ns` must be positive, and -`exited_at_ns` (if given) must be ≥ `entered_at_ns` — invalid bodies return 422. If Claude -declines or returns no structured analysis, `/journal/analyze` returns 502. +`exited_at_ns` (if given) must be ≥ `entered_at_ns` — on `PATCH` that ordering is checked +against the stored entry too, so updating just one timestamp can't leave the exit before the +entry. Invalid bodies return 422. If Claude declines or returns no structured analysis, +`/journal/analyze` returns 502. Run the checks the same way CI does: @@ -177,3 +179,29 @@ calls were made by me — as the project progresses. JavaScript's safe-integer range (2^53 ≈ 9e15). Rather than change the Phase 1–2 metrics contract now, the Phase 4 dashboard will read them as strings/BigInt; documented in the `backtest.api` module docstring so it is not forgotten. +- **Phase 0–3 hardening review before starting Phase 4** (mine): rather than build the + dashboard on top of whatever was already merged, I had Claude review the finished Phases 0–3 + for correctness and form. The baseline was green (Rust + Python suites, lint, end-to-end + pipeline); the fixes it proposed and I accepted were: + - **Zero-size ticks rejected at parse** (Claude's proposal, accepted): a window whose ticks + all had `size == 0` gave a zero denominator, making OFI and VWAP `NaN` and poisoning the + SQLite write. A serde field validator now rejects `size == 0` when the tape is parsed + (with row context), so `MetricsBucket::ofi`'s "denominator is never zero" is actually true. + The synthetic generator never emits 0, so this only ever bit hand-crafted tapes — a latent + hole, closed at the boundary rather than checked per metric. + - **`PATCH` time-ordering checked against the stored row** (Claude's proposal, accepted): + the API-boundary validator only caught `exited_at_ns < entered_at_ns` when both fields + arrived together, so a partial update of just one timestamp could persist an exit before its + entry. `journal.update_entry` now merges the update with the stored row before writing and + raises `InvalidEntryUpdate`; the endpoint maps it to **422** — one status code for one + domain rule (409 was considered and rejected for that reason) — keeping the pydantic + both-fields check as a fast path. + - **Journal INSERT shared, `created_at_ns` stamped per row** (Claude's proposal, accepted): + `insert_entry` and `import_csv` now build the INSERT from one `_INSERT_SQL` constant, and the + bulk import stamps each row at its own insertion time instead of sharing one batch timestamp, + matching the single-insert path. Form only; behavior otherwise unchanged. + - **`anthropic` capped below the next major** (mine): `>=0.117,<1`, because the coach rides + recent SDK surface (`messages.parse(output_format=...)`, `parsed_output`, adaptive thinking) + that a 1.0 could break; `fastapi`/`uvicorn` stay lower-bounded so CI keeps exercising latest. + A full `uv.lock` was considered and deferred (it would mean migrating CI off pip); the stale + `uv.lock` entry in `.gitattributes`, for a tool the project doesn't use, was removed.