Skip to content

Fix notional order rejection handling - #234

Merged
Pigbibi merged 2 commits into
mainfrom
codex/firstrade-ibit-notional-rejection-fix
Jul 28, 2026
Merged

Fix notional order rejection handling#234
Pigbibi merged 2 commits into
mainfrom
codex/firstrade-ibit-notional-rejection-fix

Conversation

@Pigbibi

@Pigbibi Pigbibi commented Jul 28, 2026

Copy link
Copy Markdown
Contributor

Summary

  • classify live Firstrade responses as submitted only when the broker accepted the request and returned an order ID
  • keep notional USD separate from share quantity and render dollar-based orders correctly
  • persist broker rejection 1219 as a retryable execution block with localized notification text
  • add regression coverage for adapter, execution, persistence, and Telegram output

Root cause

The runtime treated every live broker response as submitted and copied notional_usd into quantity. A Firstrade 400 response therefore appeared as a submitted order, while the dollar amount was displayed and projected as a share count.

Validation

  • python -m pytest -q — 192 passed
  • python -m ruff check --exclude external .
  • uv lock --check
  • git diff --check
  • python3 -m build --no-isolation

No live order endpoint was invoked and no persisted runtime state was changed.

Pigbibi and others added 2 commits July 28, 2026 13:49
Co-Authored-By: Codex <noreply@openai.com>
…notional-rejection-fix

Co-Authored-By: Codex <noreply@openai.com>
@Pigbibi Pigbibi closed this Jul 28, 2026
@Pigbibi Pigbibi reopened this Jul 28, 2026
@github-actions

Copy link
Copy Markdown

🤖 Codex PR Review

Merge allowed: 2 finding(s) reported but none are blocking

ℹ️ Other Findings

1. 🟡 [MEDIUM] Logic in application/rebalance_service.py

BROKER_EXECUTION_BLOCKING_SKIP_REASONS now includes the generic broker_rejected reason, so any live broker rejection that is not refCode 1219 will still be promoted by run_strategy_cycle() to EXECUTION_BLOCKED. This is reachable from the existing live-order path because _broker_rejection_reason() maps every non-accepted response to broker_rejected, which turns ordinary validation/transient rejects into a persisted strategy-wide block instead of a normal failed execution. (line 74)

Suggestion: Only treat the specific broker rejection codes that require manual intervention as blocking (for this PR, 1219), or inspect the broker refCode before adding a rejected order to the execution-blocking set.

2. 🟡 [MEDIUM] Logic in application/execution_service.py

pending_sell_release_symbols.append(symbol) still executes before _record_order_result() decides whether the sell was actually accepted. If a live sell order is rejected, the rest of execute_value_target_plan() continues with that symbol marked as having a pending sell even though the broker refused it, so same-cycle release/buy calculations can run on a false assumption for that position. (line 744)

Suggestion: Append to pending_sell_release_symbols only inside the _record_order_result(...) success branch, alongside submitted_sell_orders.append(order_result) and sell_submitted = True.


Review by Codex PR Review bot • PR

@Pigbibi

Pigbibi commented Jul 28, 2026

Copy link
Copy Markdown
Contributor Author

Review findings checked against the current runtime contract; no code change is needed:

  1. broker_rejected intentionally remains a retryable EXECUTION_BLOCKED outcome. Removing it would resolve a rejected live order to terminal NO_ACTION, preventing a safe retry.
  2. A rejected sell intentionally remains a sell-release dependency so buys that rely on unrealized sell proceeds stay blocked. Confirmed release value is computed only from accepted sell orders, so rejected sells never add buying power.

Both paths are fail-closed and covered by the current execution-stage semantics.

@Pigbibi
Pigbibi merged commit 2ad6039 into main Jul 28, 2026
5 checks passed
@Pigbibi
Pigbibi deleted the codex/firstrade-ibit-notional-rejection-fix branch July 28, 2026 06:08
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