Merge pull request #152 from CodeGateSoftware/fix/orders-rule-id

fix(orders): populate orders.rule_id from originating rule (Phase-2 debt)

Author: eaitbrahim

keel/agent.py

@@ -140,7 +140,13 @@ def _build_rule(row: dict[str, Any]) -> Rule:
         if "signal_patterns" in kwargs:
             kwargs["signal_patterns"] = tuple(kwargs["signal_patterns"])
 
-    return rule_cls(**kwargs)
+    rule = rule_cls(**kwargs)
+    # Thread the real `rules.id` onto the instance (additive `Rule.rule_id`, default `None`) so
+    # it can flow onto emitted `Signal`s and, from there, into `orders.rule_id`/`signals.rule_id`
+    # for audit -- `.get` rather than `row["id"]` so a hand-built row with no "id" (some tests)
+    # still builds a rule, just with `rule_id` left at its `None` default.
+    rule.rule_id = row.get("id")
+    return rule
 
 
 # -- freshness -----------------------------------------------------------------------------
@@ -532,6 +538,7 @@ def _handle_exits(
         cts_score=0,
         entry_technique="market",
         ts=now_ts,
+        rule_id=owning_rule.rule_id,
     )
     result = executor.execute(
         exit_signal, broker, repo, config, mode, confirm_fn=confirm_fn, now_ts=now_ts

keel/execution/executor.py

@@ -346,6 +346,7 @@ def _build_intent(
             notional=sizing.spend(qty, setup.entry),
             is_dca=is_dca,
             rule_kind=signal.rule_name,
+            rule_id=signal.rule_id,
             available_quote=available_quote,
             withdrawals_enabled=withdrawals,
         )
@@ -365,6 +366,7 @@ def _build_intent(
         notional=sizing.spend(qty, entry),
         is_dca=False,
         rule_kind=signal.rule_name,
+        rule_id=signal.rule_id,
     )
 
 
@@ -590,7 +592,7 @@ def _order_row(intent: OrderIntent, mode: str, now_ts: int) -> dict[str, Any]:
         # called `'autonomous'`. Nothing reads this column back -- it is an audit trail only --
         # so the rows are deliberately left as-written rather than rewritten by a migration.
         confirmation=mode,
-        rule_id=None,
+        rule_id=intent.rule_id,
         created_at=now_ts,
         updated_at=now_ts,
     )

keel/execution/guards.py

@@ -119,6 +119,12 @@ class OrderIntent:
     notional: Decimal
     is_dca: bool
     rule_kind: str
+    # The originating `rules.id` DB row (`signal.rule_id`, threaded from `agent._build_rule`),
+    # carried through purely so `executor._order_row` can write it to `orders.rule_id` -- no rail
+    # reads this, and it plays no part in any guard decision. `None` (the default) for an intent
+    # built from a hand-constructed `Rule`/`Signal` (most tests) or a caller that doesn't thread
+    # one (`place_bracket`/`scale_out`/`_roll_stop`, which only have a `rule_name` string).
+    rule_id: int | None = None
     # Rail 13 (USDC-funding): the live available quote-currency (default USDC) balance, fetched
     # by the caller (the executor) from the broker -- guards has no broker access of its own.
     # `None` means "unknown/unavailable" and fails the BUY closed, same as a missing quote

keel/strategy/engine.py

@@ -136,6 +136,7 @@ def evaluate(
             cts_score=cts_result.total,
             entry_technique=technique,
             ts=setup.ts,
+            rule_id=rule.rule_id,
         )
         signals.append(signal)
         log_event(
@@ -321,10 +322,11 @@ def _persist_signal(repo: Repository, signal: Signal, cts_result: indicators_cts
     """Write an emitted `Signal` to the `signals` table via `Repository.insert_signal`
     (schema: `data/db.py`; P3 Task 1 added the typed method).
 
-    `rule_id` is left `NULL` (no DB-backed `rules` row lookup is wired here -- Task 9's
-    promotion lifecycle owns that linkage); `indicators` carries the full CTS breakdown +
-    setup for audit/explainability; `fired=1` marks this as an actionable, gate-cleared
-    signal -- rejected candidates are never persisted at all.
+    `rule_id` carries `signal.rule_id` -- the originating `rules.id`, threaded from
+    `agent._build_rule` through the `Rule` that emitted this `Signal` (`None` for a signal from a
+    hand-constructed rule, e.g. most tests); `indicators` carries the full CTS breakdown + setup
+    for audit/explainability; `fired=1` marks this as an actionable, gate-cleared signal --
+    rejected candidates are never persisted at all.
     """
     setup = signal.setup
     payload = {
@@ -347,7 +349,7 @@ def _persist_signal(repo: Repository, signal: Signal, cts_result: indicators_cts
     }
     repo.insert_signal(
         {
-            "rule_id": None,
+            "rule_id": signal.rule_id,
             "product_id": signal.product_id,
             "ts": signal.ts,
             "indicators": json.dumps(payload, default=str),

keel/strategy/paper.py

@@ -9,16 +9,18 @@
 `orders(mode='paper')` via `Repository`. No live order placement ever happens here
 (Phase 3 `execution/*` owns that).
 
-**Schema note:** `orders.rule_id` is an `INTEGER` foreign key into the `rules` table
-(populated by promotion/demotion, task 9); paper fills from a rule that hasn't been
-persisted there yet would violate that FK, so paper orders leave `rule_id` NULL and
-instead carry `rule_name` plus this module's own reconstruction fields (`role`,
-`entry_order_id`, pnl/mfe/mae/outcome/etc.) JSON-encoded in `orders.raw_response` --
-the column live trading reserves for a broker's raw response, unused for paper
-fills. `track_record()` reads that back out (via `Repository.get_orders(mode='paper')`,
-P3 Task 1) to reconstruct `Trade`s and aggregates them via the shared
-`strategy.stats.summarize` helper into the same `BacktestResult` shape `backtest.py`
-produces, so paper and historical stats are directly comparable.
+**Schema note:** `orders.rule_id` is an `INTEGER` foreign key into the `rules` table. A paper
+`Signal` carries `rule_id` only when it was emitted for a rule reconstructed from a persisted
+`rules` row (`agent._build_rule`) -- which is always true for a real paper cycle (paper mode
+loads its rules the same way live mode does, from `repo.get_rules("paper")`), so the FK is
+satisfied by construction; a hand-built `Signal` with no `rule_id` (most tests) still writes
+`NULL`, exactly as before. Either way, `rule_name` plus this module's own reconstruction fields
+(`role`, `entry_order_id`, pnl/mfe/mae/outcome/etc.) are still JSON-encoded in
+`orders.raw_response` -- the column live trading reserves for a broker's raw response, unused for
+paper fills -- since `rule_id` alone doesn't carry that bookkeeping. `track_record()` reads that
+back out (via `Repository.get_orders(mode='paper')`, P3 Task 1) to reconstruct `Trade`s and
+aggregates them via the shared `strategy.stats.summarize` helper into the same `BacktestResult`
+shape `backtest.py` produces, so paper and historical stats are directly comparable.
 """
 
 from __future__ import annotations
@@ -67,6 +69,10 @@ class _OpenPaperPosition:
     whether a debit actually happened keeps the close-side credit and the equity mark
     correct-by-construction regardless of seed timing.
     """
+    #: The entry order's `rule_id` (its DB row's real value, `None` if it had none) -- carried
+    #: so the paired exit order writes the SAME `rule_id`, mirroring how `rule_name` above is
+    #: taken from the position rather than re-read off the exit signal.
+    rule_id: int | None = None
 
 
 def _touches(candle: Candle, price: Decimal) -> bool:
@@ -165,6 +171,10 @@ def _load_open_positions(self) -> None:
                 # that id is only ever stamped by `seed_cash` -- so every rehydrated
                 # position was opened while cash was seeded, and was costed at the time.
                 costed=True,
+                # The entry order's own `rule_id` column -- a real DB value, not something the
+                # JSON payload needs to carry -- so a restart-rehydrated position still closes
+                # with the same `rule_id` a same-process exit would have written.
+                rule_id=order.get("rule_id"),
             )
 
     def has_open_position(self, product_id: str) -> bool:
@@ -319,7 +329,7 @@ def _enter(self, signal: Signal, qty: Decimal = _QTY) -> int | None:
                 "actual_fill": entry_fill,
                 "raw_response": json.dumps(payload),
                 "confirmation": "paper",
-                "rule_id": None,
+                "rule_id": signal.rule_id,
                 "created_at": signal.ts,
                 "updated_at": signal.ts,
             }
@@ -333,6 +343,7 @@ def _enter(self, signal: Signal, qty: Decimal = _QTY) -> int | None:
             entry_ts=setup.ts,
             qty=qty,
             costed=costed,
+            rule_id=signal.rule_id,
         )
         if costed:
             self._cash -= entry_fill * qty + fee
@@ -392,7 +403,7 @@ def _close(self, position: _OpenPaperPosition, exit_price: Decimal, exit_ts: int
                 "actual_fill": exit_fill,
                 "raw_response": json.dumps(payload),
                 "confirmation": "paper",
-                "rule_id": None,
+                "rule_id": position.rule_id,
                 "created_at": exit_ts,
                 "updated_at": exit_ts,
             }

keel/strategy/rules/base.py

@@ -65,6 +65,10 @@ class Signal:
     cts_score: int
     entry_technique: str
     ts: int
+    #: The originating `rules.id` DB row, when the emitting `Rule` was reconstructed via
+    #: `agent._build_rule` (which threads `Rule.rule_id` through). `None` for a hand-constructed
+    #: `Rule`/`Signal` (most tests) -- purely additive metadata, never read by any gate/guard.
+    rule_id: int | None = None
 
 
 @dataclass
@@ -92,11 +96,18 @@ class Rule(ABC):
     `promotion_class` selects the rule's promotion floor (`strategy.promotion.floor_for_class`):
     the default `"default"` uses the canonical 100/0.55 floor; trend-followers override it to
     `"trend_follow"` for a low-win/high-R:R floor (KB §25.5).
+
+    `rule_id` is the originating `rules.id` DB row, set by `agent._build_rule` on a rule loaded
+    from `repo.get_rules()`. It defaults to `None` -- same pattern as `promotion_class` -- so
+    every hand-constructed `Rule` in tests (or anywhere else) is unaffected; it exists purely so
+    the id can be threaded onto emitted `Signal`s and, from there, into `orders.rule_id` for
+    audit -- it plays no part in `detect`/`exit_signal`/any guard or gate.
     """
 
     name: str
     params: dict
     promotion_class: str = "default"
+    rule_id: int | None = None
 
     @abstractmethod
     def detect(self, candles_by_tf: dict[Granularity, list[Candle]]) -> Setup | None:

tests/data/test_repository.py

@@ -179,6 +179,26 @@ def test_insert_order_ids_increment(repo):
     assert id2 > id1
 
 
+def test_insert_order_round_trips_a_non_none_rule_id(repo):
+    """`orders.rule_id` is a real FK into `rules` -- once the row exists, an order naming it
+    must round-trip that id exactly, not just `None` (the `_order()` default)."""
+    rule_id = repo.insert_rule("pullback_continuation", {"lookback": 20})
+
+    order_id = repo.insert_order(_order(rule_id=rule_id))
+
+    stored = repo.get_order(order_id)
+    assert stored["rule_id"] == rule_id
+
+
+def test_get_orders_also_round_trips_rule_id(repo):
+    rule_id = repo.insert_rule("dca", {})
+    repo.insert_order(_order(rule_id=rule_id))
+    repo.insert_order(_order(rule_id=None))
+
+    rows = repo.get_orders()
+    assert {r["rule_id"] for r in rows} == {rule_id, None}
+
+
 def test_update_order_updates_fields_and_round_trips_decimal(repo):
     order_id = repo.insert_order(_order())
 
@@ -348,6 +368,15 @@ def test_insert_signal_returns_id_and_round_trips(repo):
     assert row["fired"] == 1
 
 
+def test_insert_signal_round_trips_a_non_none_rule_id(repo):
+    rule_id = repo.insert_rule("rsi_meanrev", {})
+
+    signal_id = repo.insert_signal(_signal(rule_id=rule_id))
+
+    row = repo._conn.execute("SELECT * FROM signals WHERE id = ?", (signal_id,)).fetchone()
+    assert row["rule_id"] == rule_id
+
+
 def test_insert_signal_ids_increment(repo):
     id1 = repo.insert_signal(_signal())
     id2 = repo.insert_signal(_signal())

tests/execution/test_executor.py

@@ -349,6 +349,98 @@ def test_confirm_mode_without_confirm_fn_defaults_to_not_placed(repo):
     assert len(broker.place_calls) == 0
 
 
+# -- rule_id metadata (Phase-2 debt: orders.rule_id was always written NULL) ----------------------
+
+
+def test_placed_order_carries_the_signals_rule_id(repo):
+    """The fix under test: `orders.rule_id` used to be hardcoded `None` in `_order_row`
+    regardless of the signal. It must now carry `signal.rule_id` end to end through
+    `_build_intent`'s `OrderIntent.rule_id`.
+    """
+    rule_id = repo.insert_rule("pullback_continuation", {}, status="live")
+    broker = FakeBroker()
+    signal = _enter_signal(rule_id=rule_id)
+
+    result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS)
+
+    assert result.placed is True
+    order = repo.get_order(result.order_id)
+    assert order["rule_id"] == rule_id
+
+
+def test_a_signal_with_no_rule_id_still_writes_none(repo):
+    """Backward-compat: a signal from a hand-constructed rule (no `rule_id` supplied, the
+    default) still writes `NULL`, exactly as before this fix."""
+    broker = FakeBroker()
+    signal = _enter_signal()  # no rule_id override -> defaults to None
+
+    result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS)
+
+    assert result.placed is True
+    order = repo.get_order(result.order_id)
+    assert order["rule_id"] is None
+
+
+def test_rule_id_is_purely_additive_metadata_placement_and_guards_are_unchanged(repo):
+    """The metadata-only guarantee: two otherwise-identical signals, differing only in
+    `rule_id`, must produce byte-for-byte identical guard/placement outcomes -- same veto
+    decisions, same `placed`, same broker calls/order-configuration, same sized qty/notional.
+    The ONLY difference in the resulting order rows is the `rule_id` column.
+    """
+    rule_id = repo.insert_rule("pullback_continuation", {}, status="live")
+    broker_a = FakeBroker()
+    broker_b = FakeBroker()
+    signal_no_id = _enter_signal(rule_id=None)
+    signal_with_id = _enter_signal(rule_id=rule_id)
+
+    result_a = execute(signal_no_id, broker_a, repo, _config(), mode="autonomous", now_ts=NOW_TS)
+    result_b = execute(
+        signal_with_id, broker_b, repo, _config(), mode="autonomous", now_ts=NOW_TS + 1
+    )
+
+    assert result_a.placed == result_b.placed is True
+    assert result_a.vetoed_by == result_b.vetoed_by == []
+    assert len(broker_a.preview_calls) == len(broker_b.preview_calls)
+    assert len(broker_a.place_calls) == len(broker_b.place_calls)
+    assert (
+        broker_a.place_calls[0]["order_configuration"]
+        == broker_b.place_calls[0]["order_configuration"]
+    )
+
+    order_a = repo.get_order(result_a.order_id)
+    order_b = repo.get_order(result_b.order_id)
+    # Every field EXCEPT rule_id/created_at/updated_at/id must match -- proving rule_id is the
+    # only thing that changed.
+    for field in (
+        "mode",
+        "product_id",
+        "side",
+        "order_type",
+        "qty",
+        "status",
+        "confirmation",
+    ):
+        assert order_a[field] == order_b[field], f"{field} differs -- not metadata-only"
+    assert order_a["rule_id"] is None
+    assert order_b["rule_id"] == rule_id
+
+
+def test_rail_violating_signal_with_a_rule_id_is_still_vetoed_the_same_way(repo):
+    """`rule_id` must not influence guard decisions -- a vetoed intent stays vetoed."""
+    rule_id = repo.insert_rule("pullback_continuation", {}, status="live")
+    broker = NoNetworkBroker()
+    signal = _enter_signal(
+        product_id="DOGE-USD", setup=_setup(product_id="DOGE-USD"), rule_id=rule_id
+    )
+
+    result = execute(signal, broker, repo, _config(), mode="autonomous", now_ts=NOW_TS)
+
+    assert result.placed is False
+    assert result.order_id is None
+    assert any(v.startswith("halal_allowlist") for v in result.vetoed_by)
+    assert repo.get_orders() == []
+
+
 # -- rail-violating signal -> vetoed, never previews/places --------------------------------------
 
 

tests/strategy/test_base.py

@@ -88,6 +88,36 @@ def test_constructs_with_expected_fields(self) -> None:
         assert signal.entry_technique == "signal_candle"
         assert signal.ts == 1_700_000_100
 
+    def test_rule_id_defaults_to_none(self) -> None:
+        """A hand-constructed `Signal` (most tests, and any caller that predates this field)
+        must still build with no `rule_id` kwarg -- additive, backward-compatible default."""
+        setup = _setup()
+        signal = Signal(
+            rule_name="pullback_continuation",
+            product_id="BTC-USD",
+            action=Action.ENTER,
+            side=Side.BUY,
+            setup=setup,
+            cts_score=7,
+            entry_technique="signal_candle",
+            ts=1_700_000_100,
+        )
+        assert signal.rule_id is None
+
+    def test_rule_id_round_trips_when_supplied(self) -> None:
+        signal = Signal(
+            rule_name="pullback_continuation",
+            product_id="BTC-USD",
+            action=Action.ENTER,
+            side=Side.BUY,
+            setup=None,
+            cts_score=7,
+            entry_technique="signal_candle",
+            ts=0,
+            rule_id=42,
+        )
+        assert signal.rule_id == 42
+
     def test_setup_may_be_none_for_none_action(self) -> None:
         signal = Signal(
             rule_name="rsi_meanrev",
@@ -187,3 +217,17 @@ def test_concrete_subclass_satisfies_the_abc(self) -> None:
         assert rule.describe() == {"name": "trivial", "params": {}}
         assert rule.name == "trivial"
         assert rule.params == {}
+
+    def test_rule_id_defaults_to_none(self) -> None:
+        """A hand-constructed `Rule` (most tests, and every rule not loaded via
+        `agent._build_rule`) must still build with no `rule_id` -- additive, backward-compatible
+        default, same pattern as `promotion_class`."""
+        rule = _TrivialRule()
+        assert rule.rule_id is None
+
+    def test_rule_id_is_settable_after_construction(self) -> None:
+        """`agent._build_rule` sets this as a plain attribute post-construction (not a
+        constructor kwarg) -- `Rule` is a plain mutable object, not frozen."""
+        rule = _TrivialRule()
+        rule.rule_id = 7
+        assert rule.rule_id == 7