Harden Phase 0–3: validate tick sizes, enforce time ordering on PATCH

.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

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.

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]

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

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)

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():

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

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

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;

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<u32, D::Error>
+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<Vec<Tick>, 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());
+    }
 }