You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Root cause: The Build 954 fix added return; only to the auto-RETEST path (line 189). The RETEST_MANUAL path had no return; after its null check, causing execution to fall through to:
entryOrders[entryName] = entryOrder -- assigns null to the dictionary
ExecuteSmartDispatchEntry(...) -- SIMA dispatch launched with a null master order
Additionally, activePositions[entryName] was pre-registered before submit in both retest paths but not cleaned up on null return -- leaving orphaned PositionInfo in the dict.
Fix: Added activePositions.TryRemove(entryName, out _) + return; to RETEST_MANUAL path. Added activePositions.TryRemove to auto-RETEST path (existing return; already present). State is now fully rolled back on null submit, mirroring the OR baseline pattern.
FINDING 2 -- IPC dead code still wired live (src/V12_001.cs)
Root cause: Build 955 emptied ConnectToStrategy() body but left the wiring active:
Line 369: Task.Run(() => ConnectToStrategy()) still fired on Realtime -- spinning a thread for a no-op
SendCommand(): if (!isConnected) ConnectToStrategy() called unconditionally (isConnected is permanently false)
ReceiveLoop() -- fully implemented, never called -- flagged as dead code by DeepSource
ScheduleReconnect() -- called only from ReceiveLoop, itself calls the now-empty ConnectToStrategy
Fix: Removed Task.Run wiring from OnStateChange Realtime. Removed if (!isConnected) ConnectToStrategy() from SendCommand(). Deleted ConnectToStrategy() empty method, ReceiveLoop(), and ScheduleReconnect(). SendCommand() safely no-ops via existing tcpStream != null guard. DisconnectFromStrategy() retained (called from Terminated state).
Changed Files
File
Change
src/V12_002.Entries.Retest.cs
Add TryRemove + return to RETEST_MANUAL null path; add TryRemove to auto-RETEST null path
src/V12_001.cs
Remove Task.Run IPC wiring, delete ConnectToStrategy/ReceiveLoop/ScheduleReconnect dead code
Fixed OCO/bracket order loss during stop cancellations; safer stop replacements with atomic quantity checks, cascade restoration, and aligned follower handling
Prevented continued processing of failed RETEST entries by cleaning up state on null submissions
Documentation
Added agent operation protocol guidance
Documented Build 950 OCO cascade fix and verification steps
Chores
Deprecated IPC connection system (AutoConnect is now no-op)
Updated build tag to 956
Added pre-triage PR audit workflow for automated audits
- Add TargetSnapshot class for bracket state capture
- Extend PendingStopReplacement with CapturedTargets
- Fix follower stop-cancel black hole in HandleMatchedFollowerOrder
- Add RestoreCascadedTargets via TriggerCustomEvent
- Fix CreateNewStopOrder to re-link OcoGroupId
- Add bracket restore to V8.30 timeout fallback path
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…e fallback
Fix 1 (V12_001.cs): Stub ConnectToStrategy() with early return -- strategy no
longer hosts IPC server after Phase 6 pruning. Panel runs in standalone UI mode;
SendCommand calls safely no-op via tcpStream null guard.
Fix 2 (V12_002.Entries.Retest.cs): Insert return; inside null-order guard after
SubmitOrderUnmanaged fails. Prevents retestFiredThisSession latch and SIMA
dispatch from arming on a failed order submission.
Fix 3 (V12_001.cs): Add else branch in LoadConfig() to normalize both activeMode
and selectedConfigMode to RMA when a deprecated/pruned mode is found in saved
config. Prevents GetSettings() from loading garbage data from a deleted slot.
Build tag incremented to 954.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
FINDING 1 (CRITICAL) -- V12_002.Trailing.cs:
- Snapshot CapturedTargets/BracketRestorationNeeded BEFORE pendingStopReplacements.TryAdd
in both TryAdd paths (CancelPending/Submitted and Working/Accepted state handlers).
- Eliminated race window where stop-cancel callback could read a partially-constructed
PendingStopReplacement with BracketRestorationNeeded=false and skip RestoreCascadedTargets.
- Removed now-redundant post-TryAdd snapshot blocks (were dead/duplicate after fix).
FINDING 2 (HIGH) -- V12_002.Orders.Callbacks.cs:
- HandleOrderCancelled (master path): snapshot RemainingContracts under stateLock once;
use same snapshot for guard check and CreateNewStopOrder call. Eliminates 0-qty stop risk.
- OnAccountOrderUpdate (follower path): move RemainingContracts guard inside stateLock so
check and use are atomic. Eliminates TOCTOU on follower stop replacement qty.
FINDING 3 (LOW) -- V12_001.cs:
- Remove unreachable try/catch body after intentional return stub in ConnectToStrategy().
SendCommand() safely no-ops via tcpStream null guard -- no behavior change.
Bump BUILD_TAG -> 955. ASCII gate: PASS.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…dead code removal
FINDING 1 -- RETEST_MANUAL null-submit fall-through (Retest.cs, lines 326-331)
The Build 954 return guard was applied only to the auto-RETEST path. The RETEST_MANUAL
path had no return after the null check, allowing execution to fall through to:
- entryOrders[entryName] = entryOrder (assigns null)
- ExecuteSmartDispatchEntry(...) (SIMA dispatch with null order)
Fix: add activePositions.TryRemove + return to RETEST_MANUAL path to mirror auto-RETEST.
FINDING 2 -- auto-RETEST orphaned activePositions on null submit (Retest.cs, lines 185-189)
activePositions[entryName] was pre-registered before submit but not cleaned up on null
return. Fix: add activePositions.TryRemove(entryName, out _) before the existing return.
FINDING 3 -- IPC dead code (V12_001.cs)
ConnectToStrategy() body was emptied in Build 955 but still wired:
- Task.Run(() => ConnectToStrategy()) still fired on Realtime (line 369)
- if (!isConnected) ConnectToStrategy() still called from SendCommand()
- ReceiveLoop() and ScheduleReconnect() still defined but never called
Fix: remove wiring calls, delete empty ConnectToStrategy(), delete dead methods.
Verification: ASCII gate PASS (deploy-sync.ps1). Pending F5 compile in NT8.
Supersedes PR #25 (build/955-audit-remediation) per Fresh PR Rule.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, you can upgrade your account or add credits to your account and enable them for code reviews in your settings.
No actionable comments were generated in the recent review. 🎉
ℹ️ Recent review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a4182258-f7a3-4ed1-adce-4dcf7b9aaa25
📥 Commits
Reviewing files that changed from the base of the PR and between c8037b4 and b24e1c8.
📒 Files selected for processing (2)
.github/workflows/codex-audit.yml
src/V12_001.cs
🚧 Files skipped from review as they are similar to previous changes (2)
.github/workflows/codex-audit.yml
src/V12_001.cs
📝 Walkthrough
Walkthrough
Adds OCO/bracket replacement and cascade-restoration logic (target snapshots, OCO ID capture, restore flow), removes IPC lifecycle implementation, adds a GitHub pre-triage audit workflow, updates docs/standards, and includes small retest cleanup and build-tag updates. (≤50 words)
Inserted Claude Agent Operation Protocol guidance and added Build 950/956 OCO Cascade Fix documentation with problem, three-part solution, and verification steps.
CI / PR Audit Workflow .github/workflows/codex-audit.yml
New GitHub Actions workflow to run an automated PR audit (aider / headless model) and post results to PR comments using secrets for API keys.
IPC Deprecation Cleanup src/V12_001.cs
Removed private IPC lifecycle fields/methods and auto-connect behavior; LoadConfig normalizes unknown modes to "RMA"; SendCommand no longer attempts IPC auto-connect.
Stop Replacement State Tracking src/V12_002.Trailing.cs, src/V12_002.cs
Added CapturedTargets[] and BracketRestorationNeeded to PendingStopReplacement and introduced TargetSnapshot type; snapshot up to five targets when queuing replacements.
Order Callback & Follower Handling src/V12_002.Orders.Callbacks.cs
Read RemainingContracts under lock to form atomic snapshots before recreating stops; added follower-path replacement branch that mirrors master behavior and schedules cascade restoration when needed.
Order Management & Restoration src/V12_002.Orders.Management.cs
Capture OCO group ID under lock and pass as bracketOcoId when recreating stops; added RestoreCascadedTargets() to re-submit profit targets for follower vs local contexts and update internal state.
On null SubmitOrderUnmanaged return, remove pre-registered activePositions entry and short-circuit to avoid further processing.
Build Tag src/V12_002.cs
Bumped BUILD_TAG from "948" to "956".
Sequence Diagram
sequenceDiagram
participant PM as PositionManager
participant SC as StopCancellationHandler
participant OSR as PendingReplacementStore
participant OMS as OrderManagementSystem
participant BRK as Broker/OMS API
participant OCO as CascadeRestorationEngine
PM->>SC: Notify stop cancelled / UpdateStopOrder
SC->>SC: Snapshot up to 5 TargetOrders -> TargetSnapshot[]
SC->>OSR: Enqueue PendingStopReplacement(CapturedTargets, BracketRestorationNeeded)
OSR->>OMS: Request CreateNewStopOrder(entryName, bracketOcoId)
OMS->>OMS: Lock & read RemainingContracts (atomic snapshot)
OMS->>BRK: Submit replacement stop (include captured OcoGroupId)
BRK-->>OMS: Confirm new stop
OMS-->>OSR: Confirm replacement created / remove pending entry
OSR->>OCO: If BracketRestorationNeeded && CapturedTargets -> RestoreCascadedTargets
OCO->>OMS: Recreate target limit orders (follower vs local)
OMS->>PM: Update internal order dictionaries & logs
Loading
Estimated code review effort
🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
🐰 I nudged the brackets, saved each thread,
Snapped back targets when the stop was fled,
OCO IDs found and stitched in place,
Build tag shining — nine-five-six, ace!
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 57.14% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The pull request title accurately summarizes the main changes: DeepSource remediation addressing RETEST_MANUAL null cleanup and IPC dead code removal, with BUILD_TAG updated to 956.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches
📝 Generate docstrings (stacked PR)
📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
Create PR with unit tests
Post copyable unit tests in a comment
Commit unit tests in branch build/956-deepsource-remediation
Comment @coderabbitai help to get the list of available commands and usage tips.
We reviewed changes in 24ec680...b24e1c8 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
Some issues found as part of this review are outside of the diff in this pull request and aren't shown in the inline review comments due to GitHub's API limitations. You can see those issues on the DeepSource dashboard.
PR Report Card
Overall Grade
Focus Area: Hygiene
Security
Reliability
Complexity
Hygiene
Feedback
No centralized time abstraction
Multiple uses of DateTime.Now and time-sensitive branching increase timezone bugs and make logic hard to test; introduce a single UTC time provider (IClock) so all code uses DateTime.UtcNow through one, testable abstraction.
Monolithic files and methods
Multiple namespaces in one file and a function with cyclomatic complexity of 51 indicate fused responsibilities and sprawling state, which also causes mutable fields that should be readonly; split files, extract smaller methods/classes, and favor immutable fields.
Inconsistent code hygiene with missing analyzers
Repeated nullable-check verbosity and missed readonly suggestions show style rules aren't enforced; enable Roslyn analyzers and autofixable code-style rules so these patterns are detected and corrected automatically.
The reason will be displayed to describe this comment to others. Learn more.
Nullable check expression can be simplified
While it is common to explicitly check an object's nullability before accessing its value, you can further simplify this expression by
either using the conditional access operator ? or pattern matching instead. These proposed solutions are succinct and reduce your
keystrokes.
The reason will be displayed to describe this comment to others. Learn more.
Consider using `DateTime.UtcNow` over `DateTime.Now`
DateTime.Now refers to the date and time that is local and specific to the computer on which the program is running.
Because you may either misinterpret this information or miss out on any additional information, it is recommended that you use DateTime.UtcNow.
This ensures that your program, irrespective of which computer it runs on, utilizes information that is kept consistent.
The reason will be displayed to describe this comment to others. Learn more.
Nullable check expression can be simplified
While it is common to explicitly check an object's nullability before accessing its value, you can further simplify this expression by
either using the conditional access operator ? or pattern matching instead. These proposed solutions are succinct and reduce your
keystrokes.
The reason will be displayed to describe this comment to others. Learn more.
Nullable check expression can be simplified
While it is common to explicitly check an object's nullability before accessing its value, you can further simplify this expression by
either using the conditional access operator ? or pattern matching instead. These proposed solutions are succinct and reduce your
keystrokes.
The reason will be displayed to describe this comment to others. Learn more.
Consider using `DateTime.UtcNow` over `DateTime.Now`
DateTime.Now refers to the date and time that is local and specific to the computer on which the program is running.
Because you may either misinterpret this information or miss out on any additional information, it is recommended that you use DateTime.UtcNow.
This ensures that your program, irrespective of which computer it runs on, utilizes information that is kept consistent.
The reason will be displayed to describe this comment to others. Learn more.
Null check can be simplified
The ?. operator allows you to access a property or a member only if it is non-null. Consider simplifying the highlighted expression, i.e. the object and its member's null checks via the usage of this ?. operator.
This PR addresses two DeepSource-flagged C# blocking findings from the PR #25 diff: a null fall-through bug in the RETEST_MANUAL entry path and live-wired IPC dead code in the panel indicator. It also bundles the Build 950 OCO cascade bracket-restoration FSM (follower stop replacement + target re-submission after broker OCO auto-cancel) that was the main body of the superseded PR #25.
Key changes:
V12_002.Entries.Retest.cs: Adds activePositions.TryRemove(entryName, out _) + return to both the auto-RETEST and RETEST_MANUAL null-submit paths, preventing a null value from being latched into entryOrders and a phantom SIMA dispatch from being fired.
V12_001.cs: Removes Task.Run(() => ConnectToStrategy()) from Realtime state and removes if (!isConnected) ConnectToStrategy() from SendCommand(). Deletes ConnectToStrategy(), ReceiveLoop(), and ScheduleReconnect() bodies. Several orphaned fields (receiveThread, reconnectTimer, _ipcRetryCount, _lastRetryLogTime) and the responseQueue dequeue loop in OnBarUpdate remain and will likely be flagged by the next DeepSource scan; the AutoConnect / IpcPort[NinjaScriptProperty] attributes are also still live in the NT8 UI despite being permanent no-ops.
V12_002.Orders.Callbacks.cs: Adds a follower stop-replacement handler (matching pendingStopReplacements on OnAccountOrderUpdate), mirroring the master-path in HandleOrderCancelled. The early return in the matched-stop path skips RemoveGhostOrderRef — whether this is safe depends on whether follower stop orders are tracked in ghost refs.
V12_002.Trailing.cs / V12_002.Orders.Management.cs: Adds CapturedTargets[] snapshots to PendingStopReplacement and a RestoreCascadedTargets() method that re-submits OCO-cascade-cancelled targets after any stop replacement path (normal callback, follower callback, V8.30 timeout). A non-atomic two-field write (CapturedTargets then BracketRestorationNeeded) on a live concurrent record creates a narrow race where the callback guard short-circuits and restoration is silently skipped.
Confidence Score: 3/5
The primary null-fix is correct and safe, but incomplete IPC field cleanup and a real threading race in the OCO bracket restore path should be addressed before merge.
The RETEST/RETEST_MANUAL null-submit fix is sound and straightforward. The IPC dead-code removal is mostly correct but leaves several orphaned fields (receiveThread, reconnectTimer, _ipcRetryCount, _lastRetryLogTime, responseQueue dequeue loop) that will trigger new DeepSource findings—partially defeating the stated goal. More critically, the non-atomic two-field write on PendingStopReplacement (CapturedTargets then BracketRestorationNeeded) in the trailing path creates a genuine race condition where a callback thread could skip bracket restoration silently on a live account. The RemoveGhostOrderRef skip also needs clarification. NT8 compile verification is still pending (human checkbox unchecked).
Pay close attention to src/V12_002.Trailing.cs (non-atomic concurrent write), src/V12_002.Orders.Callbacks.cs (ghost-ref skip), and src/V12_001.cs (remaining orphaned IPC fields).
Important Files Changed
Filename
Overview
src/V12_002.Entries.Retest.cs
Core fix: adds activePositions.TryRemove + return to both the auto-RETEST and RETEST_MANUAL null-order paths, correctly preventing null assignment to entryOrders and phantom SIMA dispatch.
src/V12_001.cs
IPC dead-code removal is correct but incomplete: ConnectToStrategy, ReceiveLoop, ScheduleReconnect are deleted, yet receiveThread, reconnectTimer, _ipcRetryCount, _lastRetryLogTime, and the responseQueue dequeue loop remain as orphaned dead code; AutoConnect and IpcPort NinjaScript properties are still visible in the NT8 UI despite being no-ops.
src/V12_002.Orders.Callbacks.cs
Adds follower stop-replacement handler and OCO cascade restoration trigger; early return after matching a pending stop replacement bypasses RemoveGhostOrderRef, which may leave a stale ghost-order entry for the replaced (cancelled) follower stop.
src/V12_002.Trailing.cs
Adds target snapshots for OCO cascade detection and RestoreCascadedTargets trigger on V8.30 timeout path; non-atomic write of CapturedTargets/BracketRestorationNeeded on a live concurrent pending record creates a narrow race window where bracket restoration could be silently skipped.
src/V12_002.Orders.Management.cs
Adds RestoreCascadedTargets() for OCO cascade recovery and re-links replacement stops to the broker OCO bracket via OcoGroupId; logic is well-structured with proper null guards and state-lock snapshots.
src/V12_002.cs
BUILD_TAG bumped to "956"; adds TargetSnapshot inner class and CapturedTargets/BracketRestorationNeeded fields to PendingStopReplacement — clean, minimal, self-contained changes.
Sequence Diagram
sequenceDiagram
participant ST as Strategy Thread
participant CB as OnAccountOrderUpdate<br/>(Callback Thread)
participant PSR as pendingStopReplacements
participant AP as activePositions
ST->>AP: activePositions[entryName] = pos (pre-register)
ST->>ST: SubmitOrderUnmanaged(...)
alt entryOrder == null (RETEST / RETEST_MANUAL fix)
ST->>AP: TryRemove(entryName) [Build 956]
ST->>ST: return [Build 956]
else entryOrder != null
ST->>ST: entryOrders[entryName] = entryOrder
ST->>ST: ExecuteSmartDispatchEntry(...)
end
Note over ST,CB: Stop Replacement Flow (Build 950/955)
ST->>ST: UpdateStopOrder(entryName, ...)
ST->>ST: Snapshot Working targets → CapturedTargets[]
ST->>PSR: TryAdd(entryName, PendingStopReplacement{CapturedTargets, BracketRestorationNeeded})
ST->>ST: CancelOrder(currentStop)
CB->>PSR: Match OldOrder == cancelledStop
CB->>AP: TryGetValue(key, out pos)
CB->>CB: lock(stateLock) → _rQty
CB->>ST: CreateNewStopOrder(key, _rQty, ...)
alt BracketRestorationNeeded && CapturedTargets != null
CB->>ST: TriggerCustomEvent → RestoreCascadedTargets(key, snap)
ST->>ST: Re-submit each Cancelled target with OcoGroupId
end
CB->>PSR: TryRemove(key)
After removing ConnectToStrategy(), ReceiveLoop(), and ScheduleReconnect(), five class-level fields are now dead code that DeepSource will flag in the next audit sweep:
receiveThread (line 166) — declared, never assigned after ConnectToStrategy() removal.
reconnectTimer (line 169) — assigned only inside removed ScheduleReconnect(). DisconnectFromStrategy() still calls reconnectTimer?.Dispose(), which will always be a null-safe no-op.
responseQueue (line 170) — still dequeued in OnBarUpdate (while (responseQueue.TryDequeue(...))) but never enqueued now that ReceiveLoop() is removed. The dequeue loop is permanently dead code and ProcessStrategyResponse can never fire.
_ipcRetryCount (line 172) and _lastRetryLogTime (line 173) — only written inside ConnectToStrategy().
Since the PR's stated goal is full dead code removal, these fields should also be deleted. The responseQueue dequeue loop in OnBarUpdate should be removed too, as it will never produce output.
AutoConnect and IpcPort now no-ops but still visible in NT8 UI
Both AutoConnect and IpcPort remain decorated with [NinjaScriptProperty], so they still appear in the NinjaTrader indicator configuration dialog. Since ConnectToStrategy() has been removed and the Realtime state handler is now a comment-only block, toggling AutoConnect or changing IpcPort has no effect at runtime.
A user who loads this indicator, sees Auto Connect = True, and wonders why the IPC LED never turns green would be reasonably confused. Consider either:
Removing both [NinjaScriptProperty] attributes (and the properties themselves) so they disappear from the UI, or
Adding [Browsable(false)] so they stay in code for potential future use but don't surface to users.
The reason will be displayed to describe this comment to others. Learn more.
RemoveGhostOrderRef skipped for cancelled follower stop orders
When a follower stop matches a pendingStopReplacements entry, the code calls TryRemove and then returns on line 622, bypassing the RemoveGhostOrderRef(order, reason) call on line 627.
For any follower stop that was being tracked as a ghost order ref, the old (now-cancelled) stop will remain in the ghost order collection indefinitely. This could cause stale state in follower account management—specifically, any logic that iterates ghost refs to check for open orders may count the replaced (cancelled) stop as still-active.
The callback comment explicitly says this block "mirrors HandleOrderCancelled master path," but the master path (lines 354–374 earlier in the same method) removes from pendingStopReplacements and does NOT call RemoveGhostOrderRef either—so the symmetry holds. If stop orders are definitively not tracked in ghost refs, this is fine and a brief code comment confirming that would silence the concern. If they are, calling RemoveGhostOrderRef before the return is necessary.
The reason will be displayed to describe this comment to others. Learn more.
Non-atomic write of CapturedTargets / BracketRestorationNeeded on a live concurrent record
PendingStopReplacement is stored in the ConcurrentDictionarypendingStopReplacements, but its fields are plain (non-volatile) C# fields. When updating an already-existing pending record here, CapturedTargets and BracketRestorationNeeded are written in two separate statements:
A callback thread running between these two writes (e.g., the OnAccountOrderUpdate follower-stop handler or the V8.30 timeout path in Trailing.cs) will see BracketRestorationNeeded == false while CapturedTargets != null. Because the callback guard is:
...it will short-circuit on false and silently skip target restoration, leaving the position without its bracket targets. The window is narrow but non-zero on a multi-core system running live order callbacks.
Consider writing BracketRestorationNeededfirst, or consolidating both flags into a single atomic object swap (e.g., assign a new PendingStopReplacement clone with both fields set and use TryUpdate).
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR targets build hygiene and safety fixes in the NinjaTrader 8 V12 strategy/panel codebase, primarily addressing DeepSource findings and tightening order-state rollback paths.
Changes:
Fix RETEST/RETEST_MANUAL null-order rollback by removing pre-registered activePositions state and preventing fall-through into SIMA dispatch.
The reason will be displayed to describe this comment to others. Learn more.
The Build 950 walkthrough section lists "src/V12_002.cs ... BUILD_TAG = "950"", but this PR sets BUILD_TAG to "956". Either update this bullet to match the current BUILD_TAG or clarify that the section is describing a historical Build 950 snapshot rather than the current code state.
The reason will be displayed to describe this comment to others. Learn more.
PR description/title focuses on DeepSource remediation (RETEST_MANUAL null rollback + IPC dead code removal), but this PR also introduces/extends bracket-restoration functionality (TargetSnapshot/CapturedTargets, RestoreCascadedTargets, OcoGroupId relinking) across multiple strategy files. Please reflect these additional behavioral changes in the PR description and ensure they get explicit validation coverage, since they materially affect live order-management behavior.
The reason will be displayed to describe this comment to others. Learn more.
CreateNewStopOrder(): in the follower Account-API path, CreateOrder() can return null, but the code calls pos.ExecutingAccount.Submit(new[] { newStop }) unconditionally. This can throw before the later newStop==null guard and bypass the emergency-flatten path. Add a null-guard before Submit (and handle Submit exceptions consistently with other follower submit paths).
Suggested change
pos.ExecutingAccount.Submit(new[]{newStop});
// Guard against null CreateOrder result and handle Submit exceptions so that
// the later newStop == null safety logic can trigger emergency handling.
if(newStop!=null)
{
try
{
pos.ExecutingAccount.Submit(new[]{newStop});
}
catch(ExceptionsubmitEx)
{
// Log and force failure path by nulling newStop so downstream safety logic engages.
Print("Error submitting follower stop order for "+entryName+": "+submitEx);
The reason will be displayed to describe this comment to others. Learn more.
RestoreCascadedTargets(): executingAccount.Submit(...) is not wrapped in try/catch. If Submit throws (e.g., disconnected account / rejected order submission), the exception can bubble out of the TriggerCustomEvent handler and potentially disable the strategy while the position remains unprotected. Catch Submit exceptions here and log + fail safe (e.g., skip restoration for that target and/or trigger the same emergency-protection path used elsewhere).
Suggested change
executingAccount.Submit(new[]{tOrd});
newTarget=tOrd;
try
{
executingAccount.Submit(new[]{tOrd});
newTarget=tOrd;
}
catch(Exceptionex)
{
Print(string.Format("[B951] ERROR: Target T{0} restore submit FAILED for {1}: {2}",
snap.TargetNum, entryName, ex.Message));
// Fail-safe: leave newTarget as null so downstream logic treats this as an un-restored target.
The reason will be displayed to describe this comment to others. Learn more.
UpdateStopOrder(): the "Refresh CapturedTargets" block mutates pending.CapturedTargets and pending.BracketRestorationNeeded after the record is already visible in pendingStopReplacements. This reintroduces the same cross-thread visibility/TOCTOU risk the earlier "snapshot BEFORE TryAdd" change was meant to remove (callbacks can observe BracketRestorationNeeded/CapturedTargets in an inconsistent or stale state and skip restoration). Prefer keeping these fields immutable after insertion (or replace the pending record atomically with a fully-initialized new instance).
Suggested change
// Build 950: Refresh CapturedTargets on the live pending record if not yet populated.
The reason will be displayed to describe this comment to others. Learn more.
HandleMatchedFollowerOrder(): in the follower stop-replacement match path, the method returns immediately after removing pendingStopReplacements. This bypasses the usual RemoveGhostOrderRef(order, reason) cleanup that other terminal follower updates perform, which can leave stale stop references behind if CreateNewStopOrder fails (and also prevents the ghost/purge audit from running). Call RemoveGhostOrderRef(order, reason) before returning from this handled stop-cancel path.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/V12_002.Trailing.cs (1)
501-509: ⚠️ Potential issue | 🟠 Major
Refresh CapturedTargets on the normal pending-update path too.
The new refresh logic only runs in the TryAdd race fallback. Once pendingStopReplacements already contains entryName, the fast path above returns with the original snapshot, so target changes during the cancel window can still be replayed from stale CapturedTargets by RestoreCascadedTargets() — for example after MoveSpecificTarget() cancels and recreates a target at a different price.
Proposed fix
if (pendingStopReplacements.TryGetValue(entryName, out var existingPending))
{
// Update the pending replacement atomically (pending is a reference type)
existingPending.StopPrice = validatedStopPrice;
existingPending.Quantity = pos.RemainingContracts;
+ var capturedTargets = CaptureWorkingTargets(entryName);+ existingPending.CapturedTargets = capturedTargets;+ existingPending.BracketRestorationNeeded = capturedTargets != null;
pos.CurrentStopPrice = validatedStopPrice;
pos.CurrentTrailLevel = newTrailLevel;
return;
}
You can back CaptureWorkingTargets(entryName) with the same 1..5 snapshot loop already duplicated in the later branches.
Also applies to: 556-570
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.Trailing.cs` around lines 501 - 509, The fast path that updates
pendingStopReplacements (when pendingStopReplacements.TryGetValue(entryName, out
var existingPending) succeeds) leaves CapturedTargets stale; modify that branch
to refresh CapturedTargets by calling CaptureWorkingTargets(entryName) inside
the same 1..5 snapshot retry loop used elsewhere before updating
existingPending.StopPrice, existingPending.Quantity and
pos.CurrentStopPrice/CurrentTrailLevel so RestoreCascadedTargets() will see an
up-to-date snapshot (same pattern used in the TryAdd race fallback); ensure you
reference entryName and use the identical retry semantics to avoid races with
MoveSpecificTarget and other cancel/recreate flows.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/V12_001.cs`:
- Around line 369-370: The IPC deprecation removed ConnectToStrategy() and made
AutoConnect a no-op, leaving tcpClient/tcpStream unassigned so SendCommand()
never writes; fix by either reinstating a valid connection bootstrap that
assigns tcpClient and tcpStream before SendCommand() is used (restore or replace
ConnectToStrategy() logic and ensure AutoConnect or an explicit Connect method
performs tcpClient = ... and tcpStream = tcpClient.GetStream()) or, if IPC is
intentionally removed, remove/disable the command surface: change
SendCommand()/execution button handlers to no-op with visible disabled UI state
and add clear logging/error messages so the buttons are disabled and do not
appear live. Target symbols: ConnectToStrategy, AutoConnect, tcpClient,
tcpStream, SendCommand and the execution button handlers referenced in the
class.
- Around line 787-800: GetModeButton currently falls back to modeOrbButton so
the null branch is never hit; change the recovery here to detect that fallback
and normalize stale modes: after calling GetModeButton(activeMode) check if
modeBtn is null OR (modeBtn == modeOrbButton && activeMode != "RMA") and if so
run the deprecation recovery (Print warning, set activeMode and
selectedConfigMode to "RMA", call GetModeButton("RMA") and HighlightModeButton).
This preserves existing GetModeButton behavior but ensures unsupported saved
modes in activeMode/selectedConfigMode/fullConfig.ActiveMode are normalized.
In `@src/V12_002.Orders.Callbacks.cs`:
- Around line 377-383: The restoration is being triggered immediately after
CreateNewStopOrder(kvp.Key, ...) even though that helper may have failed or
created a terminal/no-op stop; change the logic to only enqueue
RestoreCascadedTargets when there is a confirmed, live replacement stop: after
calling CreateNewStopOrder use the stopOrders collection (e.g.
stopOrders[kvp.Key] or stopOrders[entryName]) to verify the stop exists and is
not in a terminal state (cancelled/filled/invalid) before calling
TriggerCustomEvent(o => RestoreCascadedTargets(...), null); alternatively,
adjust CreateNewStopOrder to return a success flag or a reference to the created
stop and gate the RestoreCascadedTargets call on that result; apply the same fix
to the analogous block at the other location (lines 611-616).
In `@src/V12_002.Orders.Management.cs`:
- Around line 522-530: The follower replacement stop is created and submitted
without guarding for a null return or Submit() exceptions; update the block that
calls pos.ExecutingAccount.CreateOrder(...) and
pos.ExecutingAccount.Submit(new[] { newStop }) to mirror SubmitBracketOrders():
first capture the result of pos.ExecutingAccount.CreateOrder into newStop and
immediately null-check it (do not call Submit if null), then wrap the
Submit(...) call in a local try/catch that logs the error and performs the
emergency flattening/cleanup for the follower position (same emergency handling
used in SubmitBracketOrders()), ensuring the old stop isn’t left cancelled
without protection; reference pos.ExecutingAccount.CreateOrder,
pos.ExecutingAccount.Submit, newStop, and SubmitBracketOrders() when making the
changes.
- Around line 623-642: The restore logic currently uses TargetSnapshot.Qty
(snap.Qty) which is the original order size; change the restore to compute
remaining = Math.Max(0, snap.Qty - snap.CapturedOrder.Filled) and use remaining
instead of snap.Qty when creating or submitting orders (both the
executingAccount.CreateOrder call and the SubmitOrderUnmanaged calls), and skip
creating/submitting if remaining == 0; reference TargetSnapshot (snap), its Qty
and CapturedOrder.Filled, and update the tOrd/newTarget branches to use this
compensated remaining quantity.
---
Outside diff comments:
In `@src/V12_002.Trailing.cs`:
- Around line 501-509: The fast path that updates pendingStopReplacements (when
pendingStopReplacements.TryGetValue(entryName, out var existingPending)
succeeds) leaves CapturedTargets stale; modify that branch to refresh
CapturedTargets by calling CaptureWorkingTargets(entryName) inside the same 1..5
snapshot retry loop used elsewhere before updating existingPending.StopPrice,
existingPending.Quantity and pos.CurrentStopPrice/CurrentTrailLevel so
RestoreCascadedTargets() will see an up-to-date snapshot (same pattern used in
the TryAdd race fallback); ensure you reference entryName and use the identical
retry semantics to avoid races with MoveSpecificTarget and other cancel/recreate
flows.
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bcbc3e15-3447-465e-ba6b-661bf6e1cdd6
📥 Commits
Reviewing files that changed from the base of the PR and between 24ec680 and ad60a17.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
This deprecation leaves the sidebar unable to send commands.
ConnectToStrategy() is gone, AutoConnect is explicitly a no-op, and there is no remaining assignment to tcpClient or tcpStream anywhere else in this class. Every SendCommand() now just falls through the tcpStream != null && tcpStream.CanWrite guard, so the execution buttons still look live while silently doing nothing. Either keep one real connection/bootstrap path or disable/remove the command surface with the IPC deprecation.
Also applies to: 3013-3039
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_001.cs` around lines 369 - 370, The IPC deprecation removed
ConnectToStrategy() and made AutoConnect a no-op, leaving tcpClient/tcpStream
unassigned so SendCommand() never writes; fix by either reinstating a valid
connection bootstrap that assigns tcpClient and tcpStream before SendCommand()
is used (restore or replace ConnectToStrategy() logic and ensure AutoConnect or
an explicit Connect method performs tcpClient = ... and tcpStream =
tcpClient.GetStream()) or, if IPC is intentionally removed, remove/disable the
command surface: change SendCommand()/execution button handlers to no-op with
visible disabled UI state and add clear logging/error messages so the buttons
are disabled and do not appear live. Target symbols: ConnectToStrategy,
AutoConnect, tcpClient, tcpStream, SendCommand and the execution button handlers
referenced in the class.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor
The deprecated-mode recovery path is currently unreachable.
GetModeButton() falls back to modeOrbButton, so modeBtn is never null here once the panel exists. That means this branch never normalizes stale config, and unsupported saved modes can survive in selectedConfigMode and fullConfig.ActiveMode.
Proposed fix
private Button GetModeButton(string mode)
{
- switch(mode.ToUpper())+ switch ((mode ?? string.Empty).ToUpperInvariant())
{
case "ORB": return modeOrbButton;
case "RMA": return modeRmaButton;
case "RETEST": return modeRetestButton;
case "MOMO": return modeMomoButton;
case "FFMA": return modeFfmaButton;
case "TREND": return modeTrendButton;
- default: return modeOrbButton;+ default: return null;
}
}
else
{
// [Build 954]: Saved mode is deprecated/unrecognized -- normalize both vars to RMA baseline.
Print("[WARN][954] Unrecognized saved mode '" + activeMode + "' -- falling back to RMA.");
activeMode = "RMA";
selectedConfigMode = "RMA";
+ fullConfig.ActiveMode = "RMA";+ activeCount = fullConfig.LastUsedCountPerMode.TryGetValue("RMA", out int rmaCount)+ ? rmaCount+ : (fullConfig.ActiveCount > 0 ? fullConfig.ActiveCount : 3);
modeBtn = GetModeButton("RMA");
if (modeBtn != null) HighlightModeButton(modeBtn);
}
📝 Committable suggestion
‼️IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
ButtonmodeBtn=GetModeButton(activeMode);
if(modeBtn!=null)HighlightModeButton(modeBtn);
if(modeBtn!=null)
{
HighlightModeButton(modeBtn);
}
else
{
// [Build 954]: Saved mode is deprecated/unrecognized -- normalize both vars to RMA baseline.
Print("[WARN][954] Unrecognized saved mode '"+activeMode+"' -- falling back to RMA.");
activeMode="RMA";
selectedConfigMode="RMA";
modeBtn=GetModeButton("RMA");
if(modeBtn!=null)HighlightModeButton(modeBtn);
}
ButtonmodeBtn=GetModeButton(activeMode);
if(modeBtn!=null)
{
HighlightModeButton(modeBtn);
}
else
{
// [Build 954]: Saved mode is deprecated/unrecognized -- normalize both vars to RMA baseline.
Print("[WARN][954] Unrecognized saved mode '"+activeMode+"' -- falling back to RMA.");
Verify each finding against the current code and only fix it if needed.
In `@src/V12_001.cs` around lines 787 - 800, GetModeButton currently falls back to
modeOrbButton so the null branch is never hit; change the recovery here to
detect that fallback and normalize stale modes: after calling
GetModeButton(activeMode) check if modeBtn is null OR (modeBtn == modeOrbButton
&& activeMode != "RMA") and if so run the deprecation recovery (Print warning,
set activeMode and selectedConfigMode to "RMA", call GetModeButton("RMA") and
HighlightModeButton). This preserves existing GetModeButton behavior but ensures
unsupported saved modes in activeMode/selectedConfigMode/fullConfig.ActiveMode
are normalized.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical
Restore targets only after a live replacement stop exists.
Lines 377-383 and 611-616 enqueue RestoreCascadedTargets() immediately after CreateNewStopOrder(). That helper can early-return or flatten on failure, so this can recreate profit targets while no working replacement stop exists. Please gate restoration on stopOrders[entryName] holding a non-terminal stop, or have CreateNewStopOrder() report that state explicitly.
Also applies to: 611-616
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.Orders.Callbacks.cs` around lines 377 - 383, The restoration is
being triggered immediately after CreateNewStopOrder(kvp.Key, ...) even though
that helper may have failed or created a terminal/no-op stop; change the logic
to only enqueue RestoreCascadedTargets when there is a confirmed, live
replacement stop: after calling CreateNewStopOrder use the stopOrders collection
(e.g. stopOrders[kvp.Key] or stopOrders[entryName]) to verify the stop exists
and is not in a terminal state (cancelled/filled/invalid) before calling
TriggerCustomEvent(o => RestoreCascadedTargets(...), null); alternatively,
adjust CreateNewStopOrder to return a success flag or a reference to the created
stop and gate the RestoreCascadedTargets call on that result; apply the same fix
to the analogous block at the other location (lines 611-616).
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
sed -n '60,85p' src/V12_002.Orders.Management.cs
printf'\n---\n'
sed -n '522,544p' src/V12_002.Orders.Management.cs
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 3301
Guard the follower replacement stop before submitting it.
Line 530 submits newStop without null-checking the CreateOrder() result or wrapping Submit() in exception handling. The later null guard at line 544 runs after the submit has already occurred. If CreateOrder() returns null or Submit() throws, the outer catch only logs while the old stop is already cancelled, leaving the follower position unprotected. Mirror the pattern from SubmitBracketOrders() (lines 67–83): null-check the created order and wrap Submit() in a local try/catch with emergency flattening.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.Orders.Management.cs` around lines 522 - 530, The follower
replacement stop is created and submitted without guarding for a null return or
Submit() exceptions; update the block that calls
pos.ExecutingAccount.CreateOrder(...) and pos.ExecutingAccount.Submit(new[] {
newStop }) to mirror SubmitBracketOrders(): first capture the result of
pos.ExecutingAccount.CreateOrder into newStop and immediately null-check it (do
not call Submit if null), then wrap the Submit(...) call in a local try/catch
that logs the error and performs the emergency flattening/cleanup for the
follower position (same emergency handling used in SubmitBracketOrders()),
ensuring the old stop isn’t left cancelled without protection; reference
pos.ExecutingAccount.CreateOrder, pos.ExecutingAccount.Submit, newStop, and
SubmitBracketOrders() when making the changes.
sed -n '610,650p' src/V12_002.Orders.Management.cs
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 2064
🏁 Script executed:
rg -n 'Order\.Filled|Quantity.*Filled|Filled.*Quantity' src/V12_002.Orders.Management.cs | head -20
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 63
🏁 Script executed:
sed -n '283,295p' src/V12_002.cs
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 517
Restore targets using only remaining (unfilled) quantity to prevent duplicating already-executed contracts.
The issue is confirmed. TargetSnapshot.Qty stores the original Order.Quantity (line 525, 566, 589 in Trailing.cs: Qty = _tOA.Quantity), not the remaining open quantity. When a target order is 60% filled before OCO-cascade cancellation, restoring snap.Qty resubmits the full original size, recreating the already-filled contracts.
Fix: Either snapshot the remaining quantity at capture time—Qty = _tOA.Quantity - _tOA.Filled—or compensate during restore—snap.Qty - snap.CapturedOrder.Filled (lines 628 and 640-642). The CapturedOrder reference is available and contains the Filled data needed for compensation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.Orders.Management.cs` around lines 623 - 642, The restore logic
currently uses TargetSnapshot.Qty (snap.Qty) which is the original order size;
change the restore to compute remaining = Math.Max(0, snap.Qty -
snap.CapturedOrder.Filled) and use remaining instead of snap.Qty when creating
or submitting orders (both the executingAccount.CreateOrder call and the
SubmitOrderUnmanaged calls), and skip creating/submitting if remaining == 0;
reference TargetSnapshot (snap), its Qty and CapturedOrder.Filled, and update
the tOrd/newTarget branches to use this compensated remaining quantity.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 3
♻️ Duplicate comments (2)
src/V12_001.cs (2)
787-800: ⚠️ Potential issue | 🟡 Minor
This saved-mode recovery branch is still unreachable.
GetModeButton() still falls back to modeOrbButton for unknown values, so modeBtn is never null here. Unsupported saved modes will keep surviving in selectedConfigMode/fullConfig.ActiveMode while the UI highlights ORB. Make GetModeButton() return null for unknown modes, or validate activeMode explicitly before this null check.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_001.cs` around lines 787 - 800, The recovery branch never runs
because GetModeButton(...) currently falls back to modeOrbButton for unknown
modes; change the behavior so unknown activeMode values are detected and
normalized: either update GetModeButton to return null when the input doesn't
match any valid mode (so the existing null-check branch in the block that calls
GetModeButton(activeMode) can execute), or add an explicit validation before
calling GetModeButton that checks activeMode against the set of known modes
(e.g., "RMA", "ORB", etc.) and, on mismatch, set activeMode and
selectedConfigMode to "RMA" and then call GetModeButton("RMA"); ensure
references to GetModeButton, HighlightModeButton, activeMode, and
selectedConfigMode are adjusted accordingly.
366-370: ⚠️ Potential issue | 🟠 Major
Deprecated IPC leaves the execution surface live but permanently disconnected.
ConnectToStrategy() and the receive path are gone, and this class no longer assigns tcpClient or tcpStream anywhere. Every execution button still calls SendCommand(), so the UI looks operational while every command silently falls through the tcpStream != null guard. Either keep a real bootstrap path, or disable/remove the command surface and show clear deprecated-state feedback.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_001.cs` around lines 366 - 370, The Realtime branch leaves the
execution surface live but never initializes tcpClient/tcpStream
(ConnectToStrategy() and the receive path were removed), so SendCommand()
silently does nothing; fix by either restoring a bootstrap path that assigns
tcpClient and tcpStream (re-implement the minimal ConnectToStrategy/receive
setup used when State == State.Realtime and ensure activeSymbol is set from
Instrument.MasterInstrument.Name) or explicitly disable the command surface:
make SendCommand() return a clear deprecated-state error, set the UI execution
buttons to a disabled/deprecated state when State == State.Realtime, and surface
a descriptive deprecation message so users know commands will not be sent.
Ensure references to ConnectToStrategy, tcpClient, tcpStream, SendCommand,
State.Realtime, and activeSymbol are updated accordingly.
🧹 Nitpick comments (2)
.github/workflows/codex-audit.yml (2)
33-43: Consider adding set -e and error handling for the gh command.
If the gh pr comment command fails (e.g., rate limiting, permission issues), the workflow will report success. Adding error handling ensures failures are surfaced properly.
🛡️ Proposed fix
- name: Post Audit Results to PR
env:
GH_TOKEN: ${{ secrets.GITHUB_TOKEN }}
run: |
+ set -e
if [ -f audit_output.md ]; then
gh pr comment ${{ github.event.pull_request.number }} --body-file audit_output.md
else
echo "Audit output file not found."
# Fallback if Aider stdout'd it instead
gh pr comment ${{ github.event.pull_request.number }} --body "The OpenAI audit finished, but the markdown file was not saved correctly. Please check action logs."
fi
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/codex-audit.yml around lines 33 - 43, Update the "Post
Audit Results to PR" step to fail the job on errors by enabling strict shell
mode (e.g., add set -e at top of the run script) and add explicit error handling
around the gh pr comment calls: check the exit status of the gh pr comment
commands (or chain with || { echo "gh failed"; exit 1; }) after both the
audit_output.md and fallback branches so any failure (rate limit, auth, etc.)
causes the workflow step to fail and surfaces the error; ensure you reference
the same gh pr comment invocation used in the step.
23-24: Pin aider-chat to a specific version for reproducible builds.
Installing without a version constraint (pip install aider-chat) can cause unexpected breakage when the package releases breaking changes. The current stable version is 0.86.2.
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/codex-audit.yml around lines 23 - 24, The workflow step
"Install Aider (OpenAI CLI)" currently installs "aider-chat" without a version
constraint; update that pip install invocation to pin the package to the stable
version "0.86.2" (e.g., change the pip install command installing "aider-chat"
to install "aider-chat==0.86.2") so builds are reproducible and protected from
upstream breaking changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/codex-audit.yml:
- Around line 29-31: The workflow is hardcoding
".agent/brain/codex_pr25_audit.md" which appears to reference the wrong PR;
update the aider run command message to point to a correct, stable audit
template or make it dynamic: either replace ".agent/brain/codex_pr25_audit.md"
with a generic template like ".agent/brain/audit_template.md" or interpolate the
current PR identifier (e.g. use an env var such as $PR_NUMBER or GITHUB_REF) so
the message passed to aider (the string in the run block invoking aider
--message) references the intended/current audit file rather than
codex_pr25_audit.md.
- Around line 26-31: Add a pre-check and timeout around the aider invocation:
before running the aider command referenced in the workflow step, verify the
mission file ".agent/brain/codex_pr25_audit.md" exists and fail fast with an
error if it does not, and run the aider process with a command-level timeout
(e.g., using timeout or a GitHub Actions timeout mechanism) so the aider
invocation cannot hang indefinitely; specifically update the step that runs the
"aider --model gpt-4o --no-auto-commits --yes-always --message ..." command to
first test for the mission file's existence and then invoke aider under a
bounded timeout, ensuring any non-zero exit or timeout is surfaced and fails the
job so audit_output.md is not silently missing or stale.
- Line 31: The workflow command uses the aider flag "--yes-always" which will
auto-confirm any interactive prompt (not just commits) and conflicts with the
instruction "without modifying any source files"; remove the "--yes-always" flag
from the aider invocation (the line invoking "aider --model gpt-4o
--no-auto-commits --yes-always --message ...") so aider cannot auto-confirm
non-commit actions, leaving "--no-auto-commits" in place to prevent automated
commits.
---
Duplicate comments:
In `@src/V12_001.cs`:
- Around line 787-800: The recovery branch never runs because GetModeButton(...)
currently falls back to modeOrbButton for unknown modes; change the behavior so
unknown activeMode values are detected and normalized: either update
GetModeButton to return null when the input doesn't match any valid mode (so the
existing null-check branch in the block that calls GetModeButton(activeMode) can
execute), or add an explicit validation before calling GetModeButton that checks
activeMode against the set of known modes (e.g., "RMA", "ORB", etc.) and, on
mismatch, set activeMode and selectedConfigMode to "RMA" and then call
GetModeButton("RMA"); ensure references to GetModeButton, HighlightModeButton,
activeMode, and selectedConfigMode are adjusted accordingly.
- Around line 366-370: The Realtime branch leaves the execution surface live but
never initializes tcpClient/tcpStream (ConnectToStrategy() and the receive path
were removed), so SendCommand() silently does nothing; fix by either restoring a
bootstrap path that assigns tcpClient and tcpStream (re-implement the minimal
ConnectToStrategy/receive setup used when State == State.Realtime and ensure
activeSymbol is set from Instrument.MasterInstrument.Name) or explicitly disable
the command surface: make SendCommand() return a clear deprecated-state error,
set the UI execution buttons to a disabled/deprecated state when State ==
State.Realtime, and surface a descriptive deprecation message so users know
commands will not be sent. Ensure references to ConnectToStrategy, tcpClient,
tcpStream, SendCommand, State.Realtime, and activeSymbol are updated
accordingly.
---
Nitpick comments:
In @.github/workflows/codex-audit.yml:
- Around line 33-43: Update the "Post Audit Results to PR" step to fail the job
on errors by enabling strict shell mode (e.g., add set -e at top of the run
script) and add explicit error handling around the gh pr comment calls: check
the exit status of the gh pr comment commands (or chain with || { echo "gh
failed"; exit 1; }) after both the audit_output.md and fallback branches so any
failure (rate limit, auth, etc.) causes the workflow step to fail and surfaces
the error; ensure you reference the same gh pr comment invocation used in the
step.
- Around line 23-24: The workflow step "Install Aider (OpenAI CLI)" currently
installs "aider-chat" without a version constraint; update that pip install
invocation to pin the package to the stable version "0.86.2" (e.g., change the
pip install command installing "aider-chat" to install "aider-chat==0.86.2") so
builds are reproducible and protected from upstream breaking changes.
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 08cf683f-ff4a-4e92-8efa-27a0dbf15809
📥 Commits
Reviewing files that changed from the base of the PR and between ad60a17 and 2a5781f.
# Use Aider in headless mode to execute the audit using GPT-4o
aider --model gpt-4o --no-auto-commits --yes-always --message "Execute audit mission: .agent/brain/codex_pr25_audit.md. Read the workflows and write the final P1/P2/P3 severity report safely to audit_output.md without modifying any source files."
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Add a timeout and file existence check to prevent workflow hangs and silent failures.
The aider command lacks a timeout, which could cause the workflow to hang indefinitely if the OpenAI API is unresponsive. Additionally, there's no validation that the audit mission file exists before invoking aider.
🛡️ Proposed fix
- name: Run Codex Audit via OpenAI
env:
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
+ timeout-minutes: 10
run: |
+ if [ ! -f ".agent/brain/codex_pr25_audit.md" ]; then+ echo "::error::Audit mission file not found: .agent/brain/codex_pr25_audit.md"+ exit 1+ fi
# Use Aider in headless mode to execute the audit using GPT-4o
aider --model gpt-4o --no-auto-commits --yes-always --message "Execute audit mission: .agent/brain/codex_pr25_audit.md. Read the workflows and write the final P1/P2/P3 severity report safely to audit_output.md without modifying any source files."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/codex-audit.yml around lines 26 - 31, Add a pre-check and
timeout around the aider invocation: before running the aider command referenced
in the workflow step, verify the mission file ".agent/brain/codex_pr25_audit.md"
exists and fail fast with an error if it does not, and run the aider process
with a command-level timeout (e.g., using timeout or a GitHub Actions timeout
mechanism) so the aider invocation cannot hang indefinitely; specifically update
the step that runs the "aider --model gpt-4o --no-auto-commits --yes-always
--message ..." command to first test for the mission file's existence and then
invoke aider under a bounded timeout, ensuring any non-zero exit or timeout is
surfaced and fails the job so audit_output.md is not silently missing or stale.
# Use Aider in headless mode to execute the audit using GPT-4o
aider --model gpt-4o --no-auto-commits --yes-always --message "Execute audit mission: .agent/brain/codex_pr25_audit.md. Read the workflows and write the final P1/P2/P3 severity report safely to audit_output.md without modifying any source files."
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor
Hardcoded audit file reference may be stale.
The audit message references codex_pr25_audit.md but this is PR #26. If this is meant to be a static audit template, the naming is confusing. If it should reference the current PR, this appears to be a copy-paste oversight.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/codex-audit.yml around lines 29 - 31, The workflow is
hardcoding ".agent/brain/codex_pr25_audit.md" which appears to reference the
wrong PR; update the aider run command message to point to a correct, stable
audit template or make it dynamic: either replace
".agent/brain/codex_pr25_audit.md" with a generic template like
".agent/brain/audit_template.md" or interpolate the current PR identifier (e.g.
use an env var such as $PR_NUMBER or GITHUB_REF) so the message passed to aider
(the string in the run block invoking aider --message) references the
intended/current audit file rather than codex_pr25_audit.md.
# Use Aider in headless mode to execute the audit using GPT-4o
aider --model gpt-4o --no-auto-commits --yes-always --message "Execute audit mission: .agent/brain/codex_pr25_audit.md. Read the workflows and write the final P1/P2/P3 severity report safely to audit_output.md without modifying any source files."
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor
🧩 Analysis chain
🌐 Web query:
aider-chat --yes-always flag behavior
💡 Result:
aider-chat --yes-always puts aider into “auto-confirm” mode: it will automatically answer Yes to interactive confirmation prompts (eg, confirmations about actions it’s about to take). The same setting can be provided via the AIDER_YES_ALWAYS environment variable. [1]
Notes/quirks seen in practice:
It was previously named --yes (and can still be abbreviated on the CLI), but was renamed to --yes-always; config/env support was also updated (AIDER_YES_ALWAYS, yes-always: in YAML). [2]
Despite the name, it does not necessarily auto-run shell commands. There’s an open report that with --yes-always aider may neither prompt to run a suggested shell command nor execute it, meaning you still need to explicitly run commands yourself (eg via /run). [3][4]
--yes-always auto-confirms prompts about all actions aider suggests, not just commits.
While --no-auto-commits prevents git commits, --yes-always will auto-confirm any interactive prompt aider raises about modifications or other actions. This creates tension with the prompt instruction "without modifying any source files"—if aider misinterprets the directive and suggests code changes, --yes-always will auto-confirm them. Consider whether this automatic confirmation aligns with the audit mission's safety requirements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/codex-audit.yml at line 31, The workflow command uses the
aider flag "--yes-always" which will auto-confirm any interactive prompt (not
just commits) and conflicts with the instruction "without modifying any source
files"; remove the "--yes-always" flag from the aider invocation (the line
invoking "aider --model gpt-4o --no-auto-commits --yes-always --message ...") so
aider cannot auto-confirm non-commit actions, leaving "--no-auto-commits" in
place to prevent automated commits.
────────────────────────────────────────────────────────────────────────────────
Update git name with: git config user.name "Your Name"
Update git email with: git config user.email "you@example.com"
You can skip this check with --no-gitignore
Added .aider* to .gitignore
Aider v0.86.2
Main model: gpt-4o with diff edit format
Weak model: gpt-4o-mini
Git repo: .git with 215 files
Repo-map: using 4096 tokens, auto refresh
Initial repo scan can be slow in larger repos, but only happens once.
Repo-map can't include
/home/runner/work/universal-or-strategy/universal-or-strategy/AntigravityMobile
Has it been deleted from the file system but not from git?
litellm.RateLimitError: RateLimitError: OpenAIException - You exceeded your
current quota, please check your plan and billing details. For more information
on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.
The API provider has rate limited you. Try again later or check your quotas.
Retrying in 0.2 seconds...
litellm.RateLimitError: RateLimitError: OpenAIException - You exceeded your
current quota, please check your plan and billing details. For more information
on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.
The API provider has rate limited you. Try again later or check your quotas.
Retrying in 0.5 seconds...
litellm.RateLimitError: RateLimitError: OpenAIException - You exceeded your
current quota, please check your plan and billing details. For more information
on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.
The API provider has rate limited you. Try again later or check your quotas.
Retrying in 1.0 seconds...
litellm.RateLimitError: RateLimitError: OpenAIException - You exceeded your
current quota, please check your plan and billing details. For more information
on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.
The API provider has rate limited you. Try again later or check your quotas.
Retrying in 2.0 seconds...
litellm.RateLimitError: RateLimitError: OpenAIException - You exceeded your
current quota, please check your plan and billing details. For more information
on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.
The API provider has rate limited you. Try again later or check your quotas.
Retrying in 4.0 seconds...
litellm.RateLimitError: RateLimitError: OpenAIException - You exceeded your
current quota, please check your plan and billing details. For more information
on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.
The API provider has rate limited you. Try again later or check your quotas.
Retrying in 8.0 seconds...
litellm.RateLimitError: RateLimitError: OpenAIException - You exceeded your
current quota, please check your plan and billing details. For more information
on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.
The API provider has rate limited you. Try again later or check your quotas.
Retrying in 16.0 seconds...
litellm.RateLimitError: RateLimitError: OpenAIException - You exceeded your
current quota, please check your plan and billing details. For more information
on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.
The API provider has rate limited you. Try again later or check your quotas.
Retrying in 32.0 seconds...
litellm.RateLimitError: RateLimitError: OpenAIException - You exceeded your
current quota, please check your plan and billing details. For more information
on this error, read the docs: https://platform.openai.com/docs/guides/error-codes/api-errors.
The API provider has rate limited you. Try again later or check your quotas.
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
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.
![deepsource-io[bot]](https://avatars.githubusercontent.com/in/16372?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Summary
Supersedes PR #25 (
build/955-audit-remediation). Addresses all DeepSource C# blocking findings on the PR #25 diff.FINDING 1 -- RETEST_MANUAL null-submit fall-through (
src/V12_002.Entries.Retest.cs, lines 326-331)Root cause: The Build 954 fix added
return;only to the auto-RETEST path (line 189). The RETEST_MANUAL path had noreturn;after its null check, causing execution to fall through to:entryOrders[entryName] = entryOrder-- assignsnullto the dictionaryExecuteSmartDispatchEntry(...)-- SIMA dispatch launched with a null master orderAdditionally,
activePositions[entryName]was pre-registered before submit in both retest paths but not cleaned up on null return -- leaving orphaned PositionInfo in the dict.Fix: Added
activePositions.TryRemove(entryName, out _)+return;to RETEST_MANUAL path. AddedactivePositions.TryRemoveto auto-RETEST path (existingreturn;already present). State is now fully rolled back on null submit, mirroring the OR baseline pattern.FINDING 2 -- IPC dead code still wired live (
src/V12_001.cs)Root cause: Build 955 emptied
ConnectToStrategy()body but left the wiring active:Task.Run(() => ConnectToStrategy())still fired on Realtime -- spinning a thread for a no-opSendCommand():if (!isConnected) ConnectToStrategy()called unconditionally (isConnected is permanentlyfalse)ReceiveLoop()-- fully implemented, never called -- flagged as dead code by DeepSourceScheduleReconnect()-- called only from ReceiveLoop, itself calls the now-empty ConnectToStrategyFix: Removed
Task.Runwiring from OnStateChange Realtime. Removedif (!isConnected) ConnectToStrategy()fromSendCommand(). DeletedConnectToStrategy()empty method,ReceiveLoop(), andScheduleReconnect().SendCommand()safely no-ops via existingtcpStream != nullguard.DisconnectFromStrategy()retained (called from Terminated state).Changed Files
src/V12_002.Entries.Retest.csTryRemove+returnto RETEST_MANUAL null path; addTryRemoveto auto-RETEST null pathsrc/V12_001.cssrc/V12_002.csVerification
deploy-sync.ps1)Related
Closes (supersedes) #25
Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Documentation
Chores