fix(orders): populate orders.rule_id from originating rule (Phase-2 debt) - #152
Merged
Conversation
…ebt)
orders.rule_id and signals.rule_id were always written NULL: _build_rule
read row["kind"]/row["params"] but discarded row["id"], so the real DB
id never reached Signal, OrderIntent, or the order/signal insert.
Threads the rules-row id through as an additive optional field, default
None everywhere (fully backward-compatible with hand-constructed
Rule/Signal instances used throughout the test suite):
- Rule.rule_id / Signal.rule_id (keel/strategy/rules/base.py): new
optional fields, same default-None pattern as Rule.promotion_class.
- agent._build_rule: sets rule.rule_id = row.get("id") after
construction (plain attribute set, not a constructor kwarg -- Rule
is a mutable object).
- agent._handle_exits: threads owning_rule.rule_id onto the EXIT
Signal it constructs.
- engine.evaluate: passes rule_id=rule.rule_id into the emitted ENTER
Signal; engine._persist_signal now writes signal.rule_id instead of
a hardcoded None into signals.rule_id.
- guards.OrderIntent: new optional rule_id field, carried through
purely as audit metadata -- no rail reads it.
- executor._build_intent: passes rule_id=signal.rule_id into both the
ENTER and EXIT OrderIntent construction; executor._order_row writes
intent.rule_id instead of a hardcoded None.
- paper.py: _OpenPaperPosition gets an optional rule_id field so the
paired exit order writes the same rule_id as its entry (mirroring
how rule_name is already threaded); _load_open_positions reads
rule_id directly off the entry order row on rehydration; the
raw_response rule_name blob is unchanged, only additive.
Metadata-only: no sizing, guard, placement, or confirm/rail logic
changed. tests/execution/test_guards.py and test_reconcile.py are
untouched and pass unchanged. New tests prove rule_id is now populated
while guard decisions/broker calls/order shape are byte-for-byte
identical to a signal with no rule_id.
No historical backfill -- forward inserts only, SCHEMA_VERSION
unchanged. Backfilling pre-existing NULL rows is a separate,
best-effort follow-up (old rows can't always be mapped to a rule
unambiguously by kind/name alone).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two coverage gaps flagged by independent review of the metadata-only
rule_id fix:
- Extend test_held_position_whose_exit_fires_gets_an_exit_order to
assert the SELL/exit order row's rule_id matches the seeded owning
rule's DB id -- the LIVE exit path (agent._handle_exits) had no
assertion pinning it, only the paper exit path did.
- Add test_a_rehydrated_position_with_a_legacy_null_rule_id_still_closes_without_crashing:
an entry order with rule_id=NULL (a position opened before this
change, or from a signal with no rule_id) rehydrates via
_load_open_positions and produces an exit -- proving the legacy-NULL
case round-trips cleanly (order.get("rule_id"), no crash) and never
fabricates an id.
Test-only; no source changes.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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
1 participant
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.
What
Closes the long-standing Phase-2 debt where
orders.rule_id(andsignals.rule_id) were always written NULL. The column has always existed (FK → rules(id)), but the originating rule's DB id was dropped early (_build_rulediscardedrow["id"]), so it never reached the insert.This threads the real rule id through the chain — additively, defaulting
None:get_rulesrowid→Rule.rule_id(set in_build_rule) →Signal.rule_id(engine ENTER + agent EXIT) →OrderIntent.rule_id→ the order-row insert. Also wiressignals.rule_idinengine._persist_signal, and the paper path (enter/close, including restart-rehydrated positions).Metadata-only — nothing about order placement changes
The only behavioral difference is a previously-NULL column now carries the rule id. No sizing, guard, rail, veto, confirm, or placement logic changed. Every hunk is either a new
None-defaulted field, a hardcodedNoneswapped to the real id, or a docstring.tests/execution/test_guards.pyandtests/execution/test_reconcile.pyhave a zero-line source diff and pass unchanged.test_rule_id_is_purely_additive_metadata_placement_and_guards_are_unchangedruns two otherwise-identical signals (with/withoutrule_id) throughexecute()and asserts identicalplaced/vetoed_by/ broker preview+place calls /order_configuration— only therule_idcolumn differs.Scope
SCHEMA_VERSIONunchanged. Old NULL rows stay NULL (unrecoverable by kind/name) — noted as a follow-up.place_bracket/scale_out/_roll_stopstill writerule_id=NULL(they only receive arule_name, andrules.namehas no UNIQUE constraint so a name→id lookup would be ambiguous). FK-safe; left as an accepted scope limit.Tests
+20 tests (
1597 → 1617), ruff clean. Covers: repository non-NULL round-trip, live ENTER + live EXIT order rows carry the seeded rule's id, a full agent cycle, paper enter/close, restart-rehydration of a legacy-NULL position (no crash), and the additive-metadata placement-identical guarantee.Review
Built TDD (Sonnet), then an independent fresh-context Opus safety review of the order path → verdict: mergeable-as-is. It verified the metadata-only guarantee (no rail reads
rule_id), id correctness in every write path, FK-safety (PRAGMA foreign_keys = ON; all ids are real PKs or NULL), and backward-compat. The two MINOR test-coverage gaps it raised (live-exit rule_id, legacy-NULL rehydration) are included in this branch.🤖 Generated with Claude Code