Skip to content

Add fill-aware order status helpers - #216

Merged
Pigbibi merged 4 commits into
mainfrom
fix/fill-aware-order-status-contract
Jul 10, 2026
Merged

Add fill-aware order status helpers#216
Pigbibi merged 4 commits into
mainfrom
fix/fill-aware-order-status-contract

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • add a shared order-status normalization helper for fill-aware execution flows
  • add Schwab order-status fetching on top of get_order
  • cover the new helpers with targeted tests

Testing

  • python3 -m pytest /Users/lisiyi/Projects/QuantPlatformKit/tests/test_order_status.py /Users/lisiyi/Projects/QuantPlatformKit/tests/test_schwab_execution.py

Co-Authored-By: Codex <noreply@openai.com>
@github-actions

github-actions Bot commented Jul 10, 2026

Copy link
Copy Markdown

🤖 Codex PR Review

🚫 Merge blocked: 2 serious issue(s) found in high-risk files

⚖️ Codex Review Arbitration

🚫 block: At least one blocking finding remains valid, in fact both do. First, src/quant_platform_kit/common/order_status.py still hard-codes only two call shapes in _call_fetch_order_status(): (order_status_context, broker_order_id) and then (broker_order_id). The exported Schwab fetcher added in src/quant_platform_kit/schwab/execution.py is still defined as fetch_order_status(api_client, account_hash, order_id), so passing quant_platform_kit.schwab.fetch_order_status directly into compute_confirmed_sell_release_value() is not composable through the shared helper. The new tests only cover 1-arg and 2-arg fetchers, not the Schwab 3-arg integration. Second, _resolve_order_status() still returns the fetched payload immediately whenever normalize_order_status_payload(fetched) is non-None, before consulting local raw_payload or direct order fields. That means a fetched stale/unfilled status with executed_qty == 0 and executed_price == 0 still overrides a fresher local fill report, exactly as described. No test in the diff proves the helper preserves local fill evidence when broker lookup lags.

🚫 Blocking Issues

These issues must be fixed before this PR can be merged:

1. 🟠 [HIGH] Logic in src/quant_platform_kit/common/order_status.py

compute_confirmed_sell_release_value() only knows how to call status fetchers as (order_id) or (context, order_id), but the newly exported Schwab fetcher is defined as (api_client, account_hash, order_id). Passing quant_platform_kit.schwab.fetch_order_status directly into the shared helper will therefore raise at runtime or always fail the lookup, so the advertised Schwab integration is not actually composable with the new common API. (line 217)

Suggestion: Standardize broker status fetchers on a single callable shape accepted by the helper, or make order_status_context support structured unpacking (for example a tuple/dict that is splatted into the fetcher) before shipping this as a shared interface.

2. 🟠 [HIGH] Logic in src/quant_platform_kit/common/order_status.py

_resolve_order_status() always prefers the freshly fetched broker payload whenever normalization succeeds, even if the fetched order only contains an unfilled/stale status and zero execution fields. In a real fill-aware workflow, broker get_order calls can lag behind a fresher local ExecutionReport or raw_payload; this code would discard the known fill and return zero released value, blocking dependent buys despite an already executed sell. (line 231)

Suggestion: Only override the local order data when the fetched payload contains stronger evidence (non-zero executed quantity/price or a confirmed fill status), otherwise merge with or fall back to the local report/raw payload.

ℹ️ Other Findings

1. 🟡 [MEDIUM] Bug in src/quant_platform_kit/common/order_status.py

The generic quantity extractor treats a field named filled as a numeric fill quantity. Many broker/order payloads use filled as a boolean flag, and _float_or_none(True) becomes 1.0, which would release 1 * executed_price instead of the actual filled notional whenever such a payload also carries an average fill price. (line 156)

Suggestion: Remove the ambiguous filled alias from the numeric quantity candidates, or explicitly reject booleans/non-numeric sentinel fields before converting them to floats.


Review by Codex PR Review bot • PR

Pigbibi and others added 2 commits July 10, 2026 20:20
Co-Authored-By: Codex <noreply@openai.com>
Co-Authored-By: Codex <noreply@openai.com>

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: a10f4c76e7

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

"filledShares",
"executedShares",
"quantityFilled",
"quantity",

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Do not treat submitted quantities as fills

When a status lookup is unavailable or a broker payload includes submitted quantity before/without true fill fields, this fallback normalizes the order size as executed_qty (and the price fallbacks below can similarly use submitted/limit price). Since compute_confirmed_sell_release_value accepts any positive executed quantity even for non-fill statuses, an accepted/working sell order dict such as quantity=8, limit_price=100 releases $800 before any fill is confirmed, which can make buy-after-sell flows overspend.

Useful? React with 👍 / 👎.

return None
if order_status_context is not None:
try:
return fetch_order_status(order_status_context, broker_order_id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Support Schwab's two-part status context

The shared helper only calls contextual fetchers as fetch_order_status(order_status_context, broker_order_id), but the Schwab fetcher added in this commit is fetch_order_status(api_client, account_hash, order_id) in src/quant_platform_kit/schwab/execution.py. Passing (api_client, account_hash) as the context therefore raises TypeError and then falls through to the one-arg call, so compute_confirmed_sell_release_value cannot use the new Schwab lookup without every caller writing a custom adapter.

Useful? React with 👍 / 👎.

if isinstance(order, ExecutionReport):
return str(order.broker_order_id or "").strip()
if isinstance(order, Mapping):
direct = str(order.get("broker_order_id") or "").strip()

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Resolve raw broker order IDs before fetching

If callers pass a raw broker order mapping that uses the supported orderId/orderNo fields instead of the normalized broker_order_id, this resolver returns an empty ID and the supplied status fetcher is never called. In that scenario a sell order submitted as {"orderId": "OID-1", "status": "accepted"} will keep using the stale local payload and miss later fills even though normalize_order_status_payload already knows how to extract the broker order ID from those fields.

Useful? React with 👍 / 👎.

Co-Authored-By: Codex <noreply@openai.com>
@Pigbibi
Pigbibi merged commit 7fb3e1c into main Jul 10, 2026
2 of 3 checks passed
@Pigbibi
Pigbibi deleted the fix/fill-aware-order-status-contract branch July 10, 2026 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant