Build 925: P1 Fix Follower Stop Lifecycle Breach (ExecutingAccount.Cancel routing)#3
Conversation
…llucinated REAPER repairs, and Master/Fleet state synchronization
Patch RequestStopCancelLifecycleSafe() to route fleet follower stop cancellations through pos.ExecutingAccount.Cancel() instead of the master-local CancelOrder() API. ROOT CAUSE: CancelOrder() is a NinjaScript-managed API that exclusively targets orders submitted via SubmitOrderUnmanaged() on the master account. Fleet follower stops are submitted via acct.Submit() (Account API), so CancelOrder() silently fails to cancel them leaving follower stops live at the broker after FlattenAll/flatten events. FIX PATTERN: Mirrors the proven fix already in CleanupPosition() [BUG-2a], which correctly branches on pos.IsFollower && pos.ExecutingAccount != null before choosing between Account.Cancel() vs CancelOrder(). AUDIT TRAIL: - Vulnerability identified: PR #2 Codex review (P1 Safety Critical) - Reproducer: Price-move on master follower stop cancel fires via wrong account context stop remains live position unprotected FILES MODIFIED: - src/UniversalORStrategyV12_002_Dev.Orders.Callbacks.cs (line ~91) - src/UniversalORStrategyV12_002_Dev.cs (BUILD_TAG 924 925) - INFRASTRUCTURE_PROTOCOL.md (build reference updated)
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e2f9a5bdb9
ℹ️ 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".
| || (masterTradeType == "TREND" && kvp.Value.IsTRENDTrade) | ||
| || (masterTradeType == "RMA" && kvp.Value.IsRMATrade) | ||
| || (masterTradeType == "MOMO" && kvp.Value.IsMOMOTrade) | ||
| || (masterTradeType == "FFMA" && kvp.Value.IsFFMATrade) | ||
| || (masterTradeType == "RETEST" && kvp.Value.IsRetestTrade) |
There was a problem hiding this comment.
In PropagateMasterPriceMove, the new fallback classifier uses IsRMATrade to identify RMA followers, but followers created by ExecuteSmartDispatchEntry are stamped with IsRMATrade = true even for non-RMA trades, so this filter misroutes updates whenever the symmetry dispatch mapping is unavailable (the exact path handled by this fallback). In that scenario, OR followers fail the OR predicate and can be incorrectly included in RMA propagation, causing missed OR price-sync updates and cross-strategy order modifications.
Useful? React with 👍 / 👎.
| if (currentPrice <= 0) | ||
| { | ||
| Print("[IPC] ABORT RMA dispatch: currentPrice=0 — lastKnownPrice and Close[0] both invalid. Wait for first bar before sending RMA commands."); | ||
| break; |
There was a problem hiding this comment.
When currentPrice <= 0, this branch uses break, which exits the while (ipcCommandQueue.TryDequeue(...)) loop in ProcessIpcCommands instead of just skipping the current LONG/SHORT command. If this guard triggers during startup/pre-session, any later queued IPC commands are left unprocessed until another trigger/tick, which can delay follow-up control actions queued behind it.
Useful? React with 👍 / 👎.
… NinjaTrader partial-class compiler)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91ef689c26
ℹ️ 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".
| if (sig.Contains("_TREND_")) followerType = "TREND"; | ||
| else if (sig.Contains("_RETEST_")) followerType = "RETEST"; | ||
| else if (sig.Contains("_MOMO_")) followerType = "MOMO"; | ||
| else if (sig.Contains("_FFMA_")) followerType = "FFMA"; | ||
| else if (sig.Contains("_RMA_")) followerType = "RMA"; |
There was a problem hiding this comment.
In the fallback branch, trade type detection uses Contains("_RMA_")/Contains("_OR_") against the full signal, but follower signals are formatted with the account name embedded before the trade type (Fleet_<AccountName>_<TRADETYPE>_<Index> in SIMA.cs). If an account name itself contains one of these tokens (for example _RMA_), the first substring match will classify the follower as the wrong strategy type when dispatch mapping is missing, causing price-move propagation to be routed to the wrong followers and skipped for the intended ones.
Useful? React with 👍 / 👎.
…ame substring safety)
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 10af039d4d
ℹ️ 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".
| if (activePositions.TryGetValue(masterEntryName, out var masterPosForType)) | ||
| { | ||
| if (masterPosForType.IsTRENDTrade) masterTradeType = "TREND"; | ||
| else if (masterPosForType.IsRMATrade) masterTradeType = "RMA"; |
There was a problem hiding this comment.
In PropagateMasterPriceMove, masterTradeType is derived by checking IsRMATrade before IsRetestTrade, which misclassifies RETEST trades running in RMA mode as RMA. RETEST positions are created with both flags true in Entries.Retest.cs (IsRMATrade can be true while IsRetestTrade is always true), so when symmetry dispatch context is missing, fallback propagation will target RMA followers and skip RETEST followers, causing missed price-sync updates and cross-strategy order moves in that recovery path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 493fc36b89
ℹ️ 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".
| pos.ExecutingAccount.Submit(new[] { sOrd }); | ||
| stopOrder = sOrd; |
There was a problem hiding this comment.
The new follower path handles CreateOrder returning null, but it does not handle pos.ExecutingAccount.Submit(new[] { sOrd }) throwing; that exception is caught by the outer catch which only logs and returns. In a broker-reject/disconnect scenario, this leaves a filled follower position without a protective stop, and there is no guaranteed retry path because SubmitBracketOrders is only invoked on the initial entry-fill transition in OnOrderUpdate/OnExecutionUpdate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Summary
- Added a local
try/catcharound follower stop submission (pos.ExecutingAccount.Submit(new[] { sOrd })) inSubmitBracketOrdersso exceptions are handled at the critical call site instead of falling through to the outer catch. src/UniversalORStrategyV12_002_Dev.Orders.Management.csL67-L72 - On submit exception, the strategy now logs a
[BRACKET_FATAL]message, immediately flattens byentryName, and returns to enforce fail-closed behavior for unprotected follower positions. src/UniversalORStrategyV12_002_Dev.Orders.Management.csL73-L78 - Committed the fix as
b734e7cand created a follow-up PR entry via themake_prtool describing the change on top of the original PR.
Testing
⚠️ curl -sLA 'Mozilla/5.0' "https://duckduckgo.com/html/?q=NinjaTrader%20Account.Submit%20exception%20handling%20stop%20order" | head -n 20(no useful output returned in this environment)- ✅
git status --short - ✅
git show --stat --oneline --no-patch HEAD
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5f370e9cf4
ℹ️ 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".
| // Zero expected position — prevents Reaper from re-queuing a repair | ||
| string acctKey = pos.ExecutingAccount != null ? pos.ExecutingAccount.Name : Account.Name; | ||
| SetExpectedPositionLocked(ExpKey(acctKey), 0); |
There was a problem hiding this comment.
SymmetryGuardCascadeFollowerCleanup hard-resets expectedPositions to zero for each follower it cancels, but ExpKey(...) is an account+instrument aggregate rather than a per-entry key. If one pending dispatch is canceled while another entry on the same follower account is still active (the strategy allows multiple concurrent entries), this wipes the remaining expected quantity and creates ledger drift that can trigger incorrect REAPER actions. Use a per-entry delta rollback (or wait for broker-confirmed flat) instead of SetExpectedPositionLocked(..., 0) here.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex provide a fix for this. Ensure we only subtract the specific quantity of the cancelled entry from the account's expected total, rather than wiping it to zero.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
There was a problem hiding this comment.
Summary
- Fixed the cascade follower cleanup ledger bug by replacing aggregate hard-reset (
expected = 0) with a per-entry rollback delta, so only the canceled follower’s quantity is removed from the account+instrument expected total. This prevents wiping expected exposure for other still-active entries on the same follower account. src/UniversalORStrategyV12_002_Dev.Symmetry.csL681-L693 - Bumped build signature from
929to930to reflect this forensic/accounting fix. src/UniversalORStrategyV12_002_Dev.csL44 - Updated infrastructure protocol “Current Build” and startup audit text to
930. INFRASTRUCTURE_PROTOCOL.mdL19-L22 - Committed on the current branch as
541bc88and created a follow-up PR entry via themake_prtool (title: Build 930: Preserve follower account expected quantity during cascade cancel).
Testing
⚠️ curl -sL 'https://duckduckgo.com/html/?q=NinjaTrader+Account+Cancel+order+expected+position+delta' | head -n 20(required web search attempt returned no useful output in this environment)- ✅
git diff --check && git status --short - ✅
git add src/UniversalORStrategyV12_002_Dev.Symmetry.cs src/UniversalORStrategyV12_002_Dev.cs INFRASTRUCTURE_PROTOCOL.md && git commit -m "Build 930: rollback cascade expected position by canceled entry delta" - ✅
nl -ba src/UniversalORStrategyV12_002_Dev.Symmetry.cs | sed -n '668,708p' && nl -ba src/UniversalORStrategyV12_002_Dev.cs | sed -n '38,50p' && nl -ba INFRASTRUCTURE_PROTOCOL.md | sed -n '16,28p'
| if (extracted == "OR" || extracted == "RMA" || | ||
| extracted == "TREND" || extracted == "RETEST" || | ||
| extracted == "MOMO" || extracted == "FFMA") | ||
| followerType = extracted; |
There was a problem hiding this comment.
The fallback matcher in PropagateMasterPriceMove only accepts extracted trade tokens in a fixed set, so follower signals with trade types containing underscores (e.g. FFMA_MNL / FFMA_MNL_MKT) fail validation and drop into boolean fallback. Those booleans are not stamped for FFMA/MOMO followers in SIMA dispatch, so these followers default to "RMA" and are filtered out when symmetryMasterEntryToDispatch is unavailable. In that fallback path, master FFMA move-sync events can silently skip their followers.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
@codex provide a fix for this. Use a more permissive check (like .Contains or StartsWith) so that trade types with underscores like FFM_MNL are recognized.
There was a problem hiding this comment.
To use Codex here, create an environment for this repo.
There was a problem hiding this comment.
Summary
- Updated fallback follower trade-type classification in
PropagateMasterPriceMoveto use a suffix-marker parser instead of strict second-to-last segment extraction, so entries likeFFMA_MNL/FFMA_MNL_MKTare correctly recognized by base strategy type. This keeps matching resilient to underscore trade subtypes. src/UniversalORStrategyV12_002_Dev.Orders.Callbacks.csL1331-L1386 - Implemented “last valid marker wins” logic against known base types (
TREND,RETEST,MOMO,FFMA,RMA,OR) so account names containing tokens like_RMA_/_OR_don’t hijack classification. src/UniversalORStrategyV12_002_Dev.Orders.Callbacks.csL1353-L1383 - Preserved existing boolean fallback behavior when parsing cannot resolve a follower type, minimizing risk in the recovery path. src/UniversalORStrategyV12_002_Dev.Orders.Callbacks.csL1388-L1399
- Committed on the current branch as
a347a8dand created a follow-up PR entry via themake_prtool.
Testing
⚠️ curl -s 'https://duckduckgo.com/html/?q=c%23+string+suffix+match+startswith+underscore+token' | head -n 5(web query returned no useful output in this environment)- ✅
git status --short - ✅
git show --stat --oneline --no-patch HEAD - ✅
nl -ba src/UniversalORStrategyV12_002_Dev.Orders.Callbacks.cs | sed -n '1320,1410p'
…score trade type fix
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: dd670b6409
ℹ️ 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".
| // entries on the same account, causing REAPER to incorrectly flatten them. | ||
| // Instead subtract only this entry's quantity from the running total. | ||
| string acctKey = pos.ExecutingAccount != null ? pos.ExecutingAccount.Name : Account.Name; | ||
| DeltaExpectedPositionLocked(ExpKey(acctKey), -pos.TotalContracts); |
There was a problem hiding this comment.
SymmetryGuardCascadeFollowerCleanup always applies -pos.TotalContracts when canceling a follower entry, but expectedPositions is a signed account-level ledger (short reservations are negative). Canceling a pending short entry should add contracts back toward zero, not subtract again; with multiple active entries on the same account this corrupts expected quantity and can make REAPER treat healthy followers as desynced (repair/flatten actions on valid positions).
Useful? React with 👍 / 👎.
…-fix Build 925: P1 Fix Follower Stop Lifecycle Breach (ExecutingAccount.Cancel routing)
1 participant
Summary
Surgical P1 safety fix identified in PR #2 Codex review.
Root Cause
\RequestStopCancelLifecycleSafe()\ was calling \CancelOrder(stopOrder)\ unconditionally a NinjaScript master-local API that silently fails for fleet follower stops, because those stops are submitted via \�cct.Submit()\ (Account API), not \SubmitOrderUnmanaged().
Result: After a FlattenAll or price-move event, follower stops could remain live at the broker, leaving positions unprotected or creating reversal risk.
Fix
Added follower branch check inside \RequestStopCancelLifecycleSafe():
This mirrors the identical pattern already proven correct in \CleanupPosition()\ [BUG-2a].
Files Modified
Safety Classification