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
- 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.
Added autonomous PR handover protocol (fresh branch and new PR requirement) and Claude agent operation guidelines; established GitHub Actions workflow for automated Codex audits on PR events with GPT-4o analysis and comment posting.
Documentation docs/brain/walkthrough.md
Added Build 950 section documenting OCO Cascade Fix with two-part resilient bracket replacement FSM, including problem statement, three targeted fixes, affected files, and verification steps.
IPC Deprecation src/V12_001.cs
Removed IPC connection lifecycle management: eliminated ConnectToStrategy(), ReceiveLoop(), ScheduleReconnect() methods and related state variables (isConnected, isShuttingDown); removed Task.Run wiring from OnStateChange; adjusted UI handling to mark AutoConnect as no-op and IPC as deprecated.
RETEST Null Cleanup src/V12_002.Entries.Retest.cs
Added activePositions.TryRemove() and early return; on null SubmitOrderUnmanaged in both ExecuteRetestEntry and ExecuteRetestManualEntry to prevent orphaned state and null entry assignment.
Implemented comprehensive bracket target restoration: added TargetSnapshot class and augmented PendingStopReplacement with CapturedTargets array and BracketRestorationNeeded flag; introduced thread-safe target snapshots in callbacks and trailing logic; added RestoreCascadedTargets() method to re-submit cancelled/rejected targets; updated OCO group ID capture and passing in stop order creation; incremented BUILD_TAG to "956".
Sequence Diagram
sequenceDiagram
actor Strategy as Strategy Thread
participant OrdMgmt as Orders.Management
participant Trailing as Trailing.cs
participant Callbacks as Orders.Callbacks
participant Broker as Broker/OCO
participant LimitTargets as Cascaded Limit Orders
Strategy->>Trailing: UpdateStopOrder() called<br/>(stop cancellation/update)
Trailing->>Trailing: Build TargetSnapshot array<br/>(capture up-to 5 targets)
Trailing->>Trailing: Create PendingStopReplacement<br/>with CapturedTargets & flag
Trailing->>OrdMgmt: CleanupStalePendingReplacements()<br/>if BracketRestorationNeeded
OrdMgmt->>Broker: Create new stop order<br/>(with OCO group ID)
Broker-->>OrdMgmt: Stop confirmed
OrdMgmt->>OrdMgmt: RestoreCascadedTargets()<br/>(iterate captured targets)
loop For each Cancelled/Rejected Target
OrdMgmt->>LimitTargets: Re-submit broker limit order<br/>(follower path)
OrdMgmt->>LimitTargets: Re-submit local limit order<br/>(local path)
end
Callbacks->>Broker: HandleMatchedFollowerOrder()<br/>on stop match
Callbacks->>Callbacks: Snapshot RemainingContracts<br/>(thread-safe)
alt BracketRestorationNeeded
Callbacks->>OrdMgmt: RestoreCascadedTargets()<br/>for follower
OrdMgmt->>LimitTargets: Re-establish bracket targets
end
Loading
Estimated Code Review Effort
🎯 4 (Complex) | ⏱️ ~50 minutes
Poem
🐰 Hops with glee through bracket lands, Cascades restored by careful hands, IPC flies away at last, RETEST nulls consigned to past, Build 956 hops so grand! ✨
🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
Check name
Status
Explanation
Resolution
Out of Scope Changes check
⚠️ Warning
Several changes appear out-of-scope relative to the stated #26 objectives: additions to standards_manifesto.md (Fresh PR Rule and Claude Agent Protocol), new codex-audit.yml workflow, and docs/brain/walkthrough.md (Build 950 documentation) are not mentioned in issue #26 requirements.
Clarify whether documentation and workflow additions should be part of the 957 PR or deferred to separate PRs; confirm these align with the PR objectives if intentional.
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 (3 passed)
Check name
Status
Explanation
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title 'fix(957): Build 956 DeepSource remediation -- IPC pruning & TOCTOU fixes' directly and clearly summarizes the main changes: removal of deprecated IPC code and fixes for race conditions (TOCTOU), which align with the file changes shown in the raw summary.
Linked Issues check
✅ Passed
The PR fully addresses the coding requirements from linked issue #26: IPC dead code removal from V12_001.cs [#26], RETEST_MANUAL null cleanup with state rollback in V12_002.Entries.Retest.cs [#26], and TOCTOU race fixes via snapshots in Orders.Callbacks.cs and Trailing.cs [#26]. Build tag is updated to 956 [#26].
✏️ 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/957-956-clean-handover
Tip
Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs).
Share your feedback on Discord.
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
Mutable state plus sloppy null handling
Fields left mutable and repetitive null checks propagate through modules; marking ephemeral counters readonly where possible and using concise nullable operators (??, is null patterns) will reduce accidental mutation and redundant checks.
Time handling entwined with complex logic
Scattered DateTime.Now calls inside a monster UpdateStopOrder tie ambiguous local-time semantics to deeply branched logic; introduce a single UTC time provider and split the method into focused helpers to make time deterministic and branches testable.
Multiple namespaces in one file and oversized functions indicate poor modular boundaries; adopt one-namespace-per-file and extract responsibilities into smaller classes/methods to prevent god-methods and cascading hygiene issues.
The reason will be displayed to describe this comment to others. Learn more.
Consider marking `receiveThread` as `readonly`
The readonly modifier can be applied to fields that are not initialized anywhere in a class and if initialized, it is either initialized inline or in the constructor.
This modifier prevents you from rewriting/overwriting its values and may even allow the runtime to perform additional optimizations.
Consider using this modifier when and where possible.
The reason will be displayed to describe this comment to others. Learn more.
Consider marking `reconnectTimer` as `readonly`
The readonly modifier can be applied to fields that are not initialized anywhere in a class and if initialized, it is either initialized inline or in the constructor.
This modifier prevents you from rewriting/overwriting its values and may even allow the runtime to perform additional optimizations.
Consider using this modifier when and where possible.
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.
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.
────────────────────────────────────────────────────────────────────────────────
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 PR delivers two DeepSource remediations — IPC dead-code pruning from V12_001.cs and TOCTOU race fixes for RemainingContracts reads — together with the Build 950 OCO cascade recovery FSM (RestoreCascadedTargets, follower stop handler, OcoGroupId re-linking) and defensive null-order guards in the RETEST/RETEST_MANUAL paths.
Key changes:
src/V12_001.cs: ConnectToStrategy(), ReceiveLoop(), ScheduleReconnect(), isConnected, and isShuttingDown removed as deprecated IPC dead code; however DisconnectFromStrategy() now calls .Close() without try-catch, which is a regression from the original (see inline comment).
src/V12_002.Trailing.cs: Target snapshots are correctly captured before TryAdd in both stop-state branches (Build 955 TOCTOU fix). However, the "update existing pending record" path mutates CapturedTargets and BracketRestorationNeeded without stateLock, creating the same race it attempts to fix (see inline comment).
src/V12_002.Orders.Callbacks.cs: Master stop TOCTOU fix is clean; new follower stop handler correctly mirrors the master path.
src/V12_002.Orders.Management.cs: RestoreCascadedTargets() is well-structured with proper stateLock snapshots and follower/local account routing.
src/V12_002.Entries.Retest.cs: Clean null-order early-return guards preventing downstream state corruption.
Safe to merge with caution — two correctness issues need resolution before production deployment.
The OCO bracket restoration FSM and follower stop handler are well-designed, but the PR contains a concrete regression (missing try-catch in DisconnectFromStrategy), a new unsynchronized mutation that contradicts the PR's own TOCTOU-fix goal (pending record update in Trailing.cs), and a broken CI workflow. The source logic issues are lower-risk for live trading since the IPC path is deprecated and the race window is narrow, but they should be resolved before merging.
Pay close attention to src/V12_002.Trailing.cs (unsynchronized pending record mutation at lines 568-569), src/V12_001.cs (DisconnectFromStrategy try-catch removal at lines 3002-3003), and .github/workflows/codex-audit.yml (hardcoded PR-25 audit path at line 35).
Important Files Changed
Filename
Overview
src/V12_001.cs
IPC dead code pruned (ConnectToStrategy, ReceiveLoop, ScheduleReconnect, isConnected, isShuttingDown removed); DisconnectFromStrategy() regresses by dropping try-catch around Close() calls, exposing ObjectDisposedException/IOException at shutdown.
src/V12_002.Trailing.cs
Target snapshots correctly captured before TryAdd in both CancelPending and Working paths; however the "refresh existing pending record" path mutates CapturedTargets and BracketRestorationNeeded without holding stateLock, reintroducing the TOCTOU race this PR claims to fix.
.github/workflows/codex-audit.yml
New Codex audit workflow hardcodes a PR-25-specific brief path, making the workflow non-functional for all other PRs; --yes-always flag also allows Aider to modify workspace files despite the "no source file modification" instruction.
src/V12_002.Orders.Callbacks.cs
TOCTOU fix for master stop path (snapshot RemainingContracts under stateLock) is correct; new follower stop handler mirrors master logic cleanly with matching stateLock snapshot and proper pendingStopReplacements cleanup.
src/V12_002.Orders.Management.cs
CreateNewStopOrder now re-links replacement stop to the broker OCO bracket via snapshotted OcoGroupId; new RestoreCascadedTargets() method is well-guarded with stateLock snapshots and correctly distinguishes follower vs local account paths.
src/V12_002.Entries.Retest.cs
Correct defensive fix: TryRemove pre-registered activePositions entry and early return on null order submission prevents downstream null-assignment and false SIMA dispatch in both RETEST and RETEST_MANUAL paths.
src/V12_002.cs
BUILD_TAG bumped to 956; new TargetSnapshot inner class and PendingStopReplacement fields (CapturedTargets, BracketRestorationNeeded) added cleanly with clear comments.
.agent/standards_manifesto.md
Documentation-only: adds Fresh PR Rule and Claude Agent Operation Protocol sections; no functional impact.
docs/brain/walkthrough.md
Documentation-only: adds Build 950 OCO Cascade Fix walkthrough section describing the two-part FSM design and verification steps.
Sequence Diagram
sequenceDiagram
participant ST as Strategy Thread
participant CB as OnOrderUpdate / OnAccountOrderUpdate
participant PSR as pendingStopReplacements
participant NT as NinjaTrader Broker
Note over ST: UpdateStopOrder() called (BE/trail)
ST->>ST: Snapshot targets (Build 955 TOCTOU fix)
ST->>PSR: TryAdd(PendingStopReplacement { CapturedTargets, BracketRestorationNeeded })
ST->>NT: CancelOrder(currentStop)
Note over NT: Broker cancels stop → OCO cascade kills T1/T2/T3
NT-->>CB: Order Cancelled (stop)
CB->>PSR: Lookup OldOrder match
alt Master account (HandleOrderCancelled)
CB->>CB: lock(stateLock) → snapshot RemainingContracts
CB->>NT: CreateNewStopOrder(qty, stopPrice, OcoGroupId)
CB->>CB: TriggerCustomEvent → RestoreCascadedTargets()
CB->>PSR: TryRemove
else Follower account (HandleMatchedFollowerOrder — Build 950)
CB->>CB: lock(stateLock) → snapshot RemainingContracts
CB->>NT: CreateNewStopOrder(qty, stopPrice, OcoGroupId)
CB->>CB: TriggerCustomEvent → RestoreCascadedTargets()
CB->>PSR: TryRemove + return
end
Note over ST: RestoreCascadedTargets() on strategy thread
ST->>ST: lock(stateLock) → snapshot pos fields
loop Each TargetSnapshot
ST->>ST: Check CapturedOrder.OrderState == Cancelled/Rejected
ST->>NT: Re-submit limit order with OcoGroupId
ST->>ST: Update tDict[entryName] = newTarget
end
Note over NT: Full OCO bracket restored (Stop + T1/T2/T3)
The reason will be displayed to describe this comment to others. Learn more.
Removed try-catch on Close() is a regression
The original DisconnectFromStrategy() wrapped both Close() calls in individual try { } catch { } blocks to suppress errors if the stream/client was already in a faulted or disposed state. The new code calls them bare, which means an ObjectDisposedException or IOException (e.g., if the socket errored before shutdown) will propagate uncaught up through the lock, which could crash the strategy at termination.
Since IPC is being deprecated and Connect() is never called, the client/stream will almost always be null — but the ?. null-conditional operator doesn't guard against a non-null but already-disposed object. Restoring the silent-catch is the safer choice:
The reason will be displayed to describe this comment to others. Learn more.
Unsynchronized mutation of shared pending record reintroduces TOCTOU
This PR's stated goal is to fix TOCTOU races by snapshotting values under stateLock, yet these two field writes to the PendingStopReplacement object happen without holding any lock:
HandleOrderCancelled (and the new follower-stop handler in Orders.Callbacks.cs) reads kvp.Value.BracketRestorationNeeded and kvp.Value.CapturedTargets from a different thread without holding a lock. A callback thread could observe BracketRestorationNeeded == false while CapturedTargets has already been written with a non-null array (or the opposite), producing a torn read that silently skips bracket restoration.
The two field writes must be atomic relative to readers. Options:
Write them inside a lock (stateLock) block — consistent with the rest of this PR's approach.
Assign the fully-initialized PendingStopReplacement as a new atomic reference (replace the whole entry in the ConcurrentDictionary) rather than mutating fields in place.
# 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." > audit_output.md
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded PR-25 audit brief will fail for all future PRs
The audit mission path codex_pr25_audit.md is hardcoded. This workflow triggers on every PR (opened, synchronize, reopened), but the referenced brief is PR-specific. For PR #26, #27, and any future PR, Aider will attempt to read a file that does not exist and will either fail silently, hallucinate the brief, or produce a meaningless report.
The brief path should either be:
Dynamic based on the PR number (e.g., .agent/brain/codex_pr${{ github.event.pull_request.number }}_audit.md), or
A stable, PR-agnostic audit mission document.
Additionally, --yes-always combined with --no-auto-commits does not prevent Aider from writing to files in the workspace — it only suppresses the commit step. If the model's response includes file edits (which Aider applies eagerly), source files could be modified within the CI run. Consider using --dry-run or restricting Aider to a read-only subset of the repo to enforce the "without modifying any source files" instruction.
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR targets Build 956 remediation work across the NinjaTrader 8 strategy/panel codebase, focusing on IPC deprecation cleanup, TOCTOU hardening in order-management paths, and documenting/automating audit workflows.
Changes:
Remove deprecated IPC connection/receive/reconnect code from V12_001 (panel), leaving IPC as a no-op.
Harden stop-replacement / bracket logic by snapshotting state and capturing/restoring OCO targets during stop replacement flows.
Add a GitHub Action workflow to run an automated “Codex pre-triage” audit and post results to PRs; update internal standards/docs.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 4 comments.
Show a summary per file
File
Description
src/V12_002.cs
Bumps build tag; introduces TargetSnapshot and extends pending stop replacement state to support bracket restoration.
src/V12_002.Trailing.cs
Captures target snapshots before enqueuing pending stop replacements; schedules restoration during timeout/emergency stop replacement.
src/V12_002.Orders.Management.cs
Re-links replacement stops to OcoGroupId; adds RestoreCascadedTargets() to re-submit cascade-cancelled targets.
src/V12_002.Orders.Callbacks.cs
Snapshots remaining qty under stateLock; adds follower stop-cancel handling to mirror master stop replacement path.
src/V12_002.Entries.Retest.cs
Fixes null-submit fall-through by rolling back state and returning after null order creation.
src/V12_001.cs
Prunes deprecated IPC connection logic and normalizes deprecated/unrecognized saved mode handling.
docs/brain/walkthrough.md
Documents the OCO cascade fix and verification steps.
.github/workflows/codex-audit.yml
Adds PR audit workflow that runs Aider/OpenAI and comments results back to the PR.
.agent/standards_manifesto.md
Adds “Fresh PR Rule” and agent operation protocol guidance.
The reason will be displayed to describe this comment to others. Learn more.
In the follower-path stop replacement, CreateOrder(...) can return null; calling pos.ExecutingAccount.Submit(new[] { newStop }) unconditionally can throw and bypass the existing newStop == null emergency-flatten logic. Add a null-guard before Submit and treat Submit() exceptions as bracket-fatal (flatten / rollback) consistent with other follower CreateOrder call sites in this file (e.g., the CIT nudge path).
Suggested change
pos.ExecutingAccount.Submit(new[]{newStop});
if(newStop!=null)
{
try
{
pos.ExecutingAccount.Submit(new[]{newStop});
}
catch(ExceptionsubmitEx)
{
// Treat submit failures as bracket-fatal: log and trigger emergency-flatten path.
Print("V12 ERROR: Failed to submit follower replacement stop for "+entryName+" - "+submitEx);
The reason will be displayed to describe this comment to others. Learn more.
This stop-replacement handler returns after creating the replacement stop, which skips the RemoveGhostOrderRef(order, reason) cleanup that normally runs for matched follower terminal events. That can leave stale stop/target references and interfere with later adoption/cleanup logic. Instead of returning immediately, ensure the old stop order is still passed through the same ghost-ref cleanup path (and keep the pendingStopReplacements removal semantics).
The reason will be displayed to describe this comment to others. Learn more.
This workflow installs aider-chat from PyPI without a pinned version/hash. For a security-sensitive repo, pin the exact version (and ideally use hashes) to reduce supply-chain risk and keep audit output stable over time.
# 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." > audit_output.md
The reason will be displayed to describe this comment to others. Learn more.
This job always attempts to run with OPENAI_API_KEY, but if the secret isn't configured (or if the PR comes from a fork where secrets aren't exposed), the step will fail noisily. Add a guard (e.g., job/step if:) to skip the audit when the key isn't available, and consider restricting execution to non-fork PRs to avoid running untrusted code paths with repository write permissions.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 7
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)
552-570: ⚠️ Potential issue | 🟠 Major
The failed TryAdd() path still loses the latest replacement state.
You now snapshot targets before TryAdd(), but when another thread wins the add race, the live PendingStopReplacement is not fully updated: the first fallback only changes StopPrice, and the second TryAdd() block has no merge fallback at all. That leaves stale Quantity/CapturedTargets on the record that eventually recreates the stop, which can restore the wrong bracket or submit an oversize stop.
Possible fix
else if (pendingStopReplacements.TryGetValue(entryName, out var pending))
{
// Just update the pending price
pending.StopPrice = validatedStopPrice;
+ pending.Quantity = pos.RemainingContracts;+ pending.CapturedTargets = _b950Refresh.Count > 0 ? _b950Refresh.ToArray() : null;+ pending.BracketRestorationNeeded = _b950Refresh.Count > 0;
}
Apply the same merge fallback after the later TryAdd(entryName, newPending) block as well.
Also applies to: 591-604
🤖 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 552 - 570, The code snapshots target
state into newPending but when ConcurrentDictionary.TryAdd(entryName,
newPending) loses the race the existing live PendingStopReplacement isn't
merged, so we must apply the same merge fallback used earlier: after any failed
TryAdd(entryName, newPending) detect the existing instance (e.g., pending) via
TryGetValue and merge the latest fields into it (update StopPrice, Quantity,
CapturedTargets, and BracketRestorationNeeded as appropriate) instead of only
setting StopPrice; apply this exact merge fix both where pending is first
updated and in the later TryAdd(entryName, newPending) block so the live record
never retains stale Quantity/CapturedTargets.
🤖 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 33-35: The current step runs the aider command string ("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.\"") and redirects stdout to audit_output.md, which can clobber or capture
the wrong content; remove the shell redirection ("> audit_output.md") so Aider
itself writes audit_output.md, then immediately check the command's exit status
and fail the job if non-zero (e.g., by testing $?) to ensure the workflow
surfaces errors rather than silently producing an empty or progress-filled file.
In `@docs/brain/walkthrough.md`:
- Line 79: The documentation's BUILD_TAG value is out of sync with the
code—update the documentation entry that lists `BUILD_TAG = "950"` to match the
code's current value `BUILD_TAG = "956"` (or, if the intent is to change the
code, update `src/V12_002.cs` where `BUILD_TAG` is defined) and ensure the
related mentions of `TargetSnapshot` and `PendingStopReplacement` in the
walkthrough are consistent with that tag so docs and the `BUILD_TAG` constant in
`V12_002.cs` align.
In `@src/V12_001.cs`:
- Around line 364-368: The Realtime branch removed the IPC connect path but left
SendCommand dependent on tcpStream and removed the receive loop, so the panel
appears live but cannot send/receive; either (A) wire the panel to the
replacement transport by initializing the new transport where tcpStream was set
(replace assignments in the State == State.Realtime block and update SendCommand
to call the new transport API and reintroduce a receive/dispatch loop), or (B)
disable the interactive surface by short-circuiting the Realtime branch (when
State == State.Realtime) to set the UI to read-only and make SendCommand return
early when tcpStream/new-transport is null; update references to
ConnectToStrategy/AutoConnect to use the replacement transport API and ensure
telemetry handlers are restored or removed accordingly.
- Around line 785-798: GetModeButton currently returns a non-null fallback
(modeOrbButton) for unknown strings, so the else branch never normalizes corrupt
modes; fix by explicitly validating activeMode before relying on GetModeButton:
check activeMode against the set of known mode identifiers (e.g., "RMA", "ORB",
etc.) or use a switch/enum in the code around the block using
GetModeButton/HighlightModeButton to detect unknown values, and when invalid set
activeMode = selectedConfigMode = "RMA" before calling GetModeButton("RMA");
alternatively, change GetModeButton to return null for unknown modes instead of
falling back to modeOrbButton so the existing else branch can run as intended.
In `@src/V12_002.Orders.Management.cs`:
- Around line 587-608: The code captures remainingContracts but never uses it
when re-submitting targets; change the restore loop that re-creates snapshot
targets (the code using snap.Qty) to treat remainingContracts as a budget:
before creating each limit order cap the restore quantity to Math.Min(snap.Qty,
remainingContracts), skip creating orders when remainingContracts <= 0, and
after successfully queuing/submitting a restored order decrement
remainingContracts by the restored amount; use the already-captured
remainingContracts variable (captured under stateLock) and ensure you stop
restoring once it reaches zero so you never recreate more outstanding quantity
than the position actually has.
- Around line 522-530: The code calls pos.ExecutingAccount.CreateOrder(...) and
immediately Submits newStop without guarding for null; add an immediate
null-check after CreateOrder (check newStop == null) and handle it (log error on
pos/entryName and skip Submit or take fallback protection) and wrap the call to
pos.ExecutingAccount.Submit(new[] { newStop }) in a try/catch to handle
submission failures (log detailed error and ensure position state remains
protected), referencing CreateOrder, Submit, newStop, pos.ExecutingAccount,
entryName and the stateLock usage to locate the surrounding logic.
In `@src/V12_002.Trailing.cs`:
- Around line 474-480: The restore is being scheduled unconditionally; change
the logic so RestoreCascadedTargets is only called after confirming the
protective stop was actually created and is active. After calling
CreateNewStopOrder (the stop-creation call that precedes this block), check the
returned stop reference/result for non-null and non-terminal/active state, and
only then, if pending.BracketRestorationNeeded and pending.CapturedTargets !=
null, call TriggerCustomEvent(o => RestoreCascadedTargets(_tKey, _tSnap), null).
In short: gate the existing pending.BracketRestorationNeeded flow on the
successful/active stop reference returned by CreateNewStopOrder instead of
running it immediately.
---
Outside diff comments:
In `@src/V12_002.Trailing.cs`:
- Around line 552-570: The code snapshots target state into newPending but when
ConcurrentDictionary.TryAdd(entryName, newPending) loses the race the existing
live PendingStopReplacement isn't merged, so we must apply the same merge
fallback used earlier: after any failed TryAdd(entryName, newPending) detect the
existing instance (e.g., pending) via TryGetValue and merge the latest fields
into it (update StopPrice, Quantity, CapturedTargets, and
BracketRestorationNeeded as appropriate) instead of only setting StopPrice;
apply this exact merge fix both where pending is first updated and in the later
TryAdd(entryName, newPending) block so the live record never retains stale
Quantity/CapturedTargets.
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 91e83c08-9376-420e-aabc-339b02959ec2
📥 Commits
Reviewing files that changed from the base of the PR and between 24ec680 and b24e1c8.
# 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." > audit_output.md
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor
Conflicting output mechanisms may cause audit results to be lost.
The command redirects Aider's stdout to audit_output.md via >, but the --message also instructs Aider to write to audit_output.md. This creates two potential issues:
If Aider writes to the file directly, the > redirect will overwrite it with stdout (which may be empty or just progress output)
If Aider only writes to stdout, the file will contain raw Aider output including progress messages, not a clean report
🔧 Proposed fix: Let Aider write the file, capture exit status
- name: Run Codex Audit via OpenAI
env:
OPENAI_API_KEY: ${{ secrets.OPENAI_API_KEY }}
run: |
# 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." > audit_output.md+ 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." || true
🤖 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 - 35, The current step
runs the aider command string ("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.\"") and redirects stdout to audit_output.md, which can clobber or capture
the wrong content; remove the shell redirection ("> audit_output.md") so Aider
itself writes audit_output.md, then immediately check the command's exit status
and fail the job if non-zero (e.g., by testing $?) to ensure the workflow
surfaces errors rather than silently producing an empty or progress-filled file.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor
BUILD_TAG mismatch between documentation and code.
The documentation states BUILD_TAG = "950" but src/V12_002.cs defines BUILD_TAG = "956". This inconsistency could cause confusion during debugging or audits.
‼️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.
Verify each finding against the current code and only fix it if needed.
In `@docs/brain/walkthrough.md` at line 79, The documentation's BUILD_TAG value is
out of sync with the code—update the documentation entry that lists `BUILD_TAG =
"950"` to match the code's current value `BUILD_TAG = "956"` (or, if the intent
is to change the code, update `src/V12_002.cs` where `BUILD_TAG` is defined) and
ensure the related mentions of `TargetSnapshot` and `PendingStopReplacement` in
the walkthrough are consistent with that tag so docs and the `BUILD_TAG`
constant in `V12_002.cs` align.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical
Removing the connect path makes the panel silently non-functional.
SendCommand() still depends on tcpStream, but this class no longer initializes tcpStream anywhere and the receive loop is gone as well. After this change, the order/config controls still look live but cannot dispatch commands or receive telemetry. Either disable/remove this surface or wire it to the replacement transport before release.
Also applies to: 3037-3037
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_001.cs` around lines 364 - 368, The Realtime branch removed the IPC
connect path but left SendCommand dependent on tcpStream and removed the receive
loop, so the panel appears live but cannot send/receive; either (A) wire the
panel to the replacement transport by initializing the new transport where
tcpStream was set (replace assignments in the State == State.Realtime block and
update SendCommand to call the new transport API and reintroduce a
receive/dispatch loop), or (B) disable the interactive surface by
short-circuiting the Realtime branch (when State == State.Realtime) to set the
UI to read-only and make SendCommand return early when tcpStream/new-transport
is null; update references to ConnectToStrategy/AutoConnect to use the
replacement transport API and ensure telemetry handlers are restored or removed
accordingly.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
This saved-mode fallback never actually normalizes unknown modes.
GetModeButton() falls back to modeOrbButton for unknown strings, so modeBtn is non-null here once the panel exists. That means corrupt/deprecated modes stay in selectedConfigMode, ORB gets highlighted, and the bad mode is written back on the next save. Validate activeMode explicitly here, or let GetModeButton() return null for unknown modes.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/V12_001.cs` around lines 785 - 798, GetModeButton currently returns a
non-null fallback (modeOrbButton) for unknown strings, so the else branch never
normalizes corrupt modes; fix by explicitly validating activeMode before relying
on GetModeButton: check activeMode against the set of known mode identifiers
(e.g., "RMA", "ORB", etc.) or use a switch/enum in the code around the block
using GetModeButton/HighlightModeButton to detect unknown values, and when
invalid set activeMode = selectedConfigMode = "RMA" before calling
GetModeButton("RMA"); alternatively, change GetModeButton to return null for
unknown modes instead of falling back to modeOrbButton so the existing else
branch can run as intended.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
# First, check the file size and read the relevant section with context
wc -l src/V12_002.Orders.Management.cs
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 118
🏁 Script executed:
# Read the section around lines 522-530 with broader context to see if there's a later null-check
sed -n '510,560p' src/V12_002.Orders.Management.cs
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 3244
🏁 Script executed:
# Search for the SubmitBracketOrders method to see the pattern mentioned
rg -n "SubmitBracketOrders" src/V12_002.Orders.Management.cs -A 20
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 2221
🏁 Script executed:
# Search for other CreateOrder and Submit patterns in the file
rg -n "CreateOrder.*Submit" src/V12_002.Orders.Management.cs -B 2 -A 5
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 63
🏁 Script executed:
# Look for any null checks on newStop variable
rg -n "newStop.*null|null.*newStop" src/V12_002.Orders.Management.cs -B 2 -A 2
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 563
Guard CreateOrder() before submitting the replacement stop.
If CreateOrder() returns null, Submit(new[] { newStop }) will throw an exception before the newStop == null guard at line 544 executes, leaving the position unprotected. The follower path needs an immediate null-check and submit error handler.
‼️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
// Build 950: Re-link replacement stop to broker OCO bracket.
Print(string.Format("[STOP_FATAL] CreateOrder returned null for {0}. Emergency flattening.",entryName));
FlattenPositionByName(entryName);
return;
}
try
{
pos.ExecutingAccount.Submit(new[]{newStop});
}
catch(ExceptionsubmitEx)
{
Print(string.Format("[STOP_FATAL] Submit threw for {0}: {1}. Emergency flattening.",entryName,submitEx.Message));
FlattenPositionByName(entryName);
return;
}
🤖 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 code calls
pos.ExecutingAccount.CreateOrder(...) and immediately Submits newStop without
guarding for null; add an immediate null-check after CreateOrder (check newStop
== null) and handle it (log error on pos/entryName and skip Submit or take
fallback protection) and wrap the call to pos.ExecutingAccount.Submit(new[] {
newStop }) in a try/catch to handle submission failures (log detailed error and
ensure position state remains protected), referencing CreateOrder, Submit,
newStop, pos.ExecutingAccount, entryName and the stateLock usage to locate the
surrounding logic.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Restore only the live outstanding target quantity.
remainingContracts is captured here but never used; the loop always re-submits snap.Qty. If a target partially fills or another exit reduces size after the snapshot, this can restore more limit quantity than the position still has open. Use the current outstanding size as a budget and stop restoring once that budget reaches zero.
Also applies to: 620-643
🤖 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 587 - 608, The code captures
remainingContracts but never uses it when re-submitting targets; change the
restore loop that re-creates snapshot targets (the code using snap.Qty) to treat
remainingContracts as a budget: before creating each limit order cap the restore
quantity to Math.Min(snap.Qty, remainingContracts), skip creating orders when
remainingContracts <= 0, and after successfully queuing/submitting a restored
order decrement remainingContracts by the restored amount; use the
already-captured remainingContracts variable (captured under stateLock) and
ensure you stop restoring once it reaches zero so you never recreate more
outstanding quantity than the position actually has.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major
Only restore targets after a protective stop is confirmed live.
This schedules RestoreCascadedTargets() immediately after CreateNewStopOrder(), even if that call bailed out or flattened on failure. In those cases you can recreate targets against an unprotected or flattening position. Gate the restore on a non-terminal stop reference after the stop-creation call returns.
Verify each finding against the current code and only fix it if needed.
In `@src/V12_002.Trailing.cs` around lines 474 - 480, The restore is being
scheduled unconditionally; change the logic so RestoreCascadedTargets is only
called after confirming the protective stop was actually created and is active.
After calling CreateNewStopOrder (the stop-creation call that precedes this
block), check the returned stop reference/result for non-null and
non-terminal/active state, and only then, if pending.BracketRestorationNeeded
and pending.CapturedTargets != null, call TriggerCustomEvent(o =>
RestoreCascadedTargets(_tKey, _tSnap), null). In short: gate the existing
pending.BracketRestorationNeeded flow on the successful/active stop reference
returned by CreateNewStopOrder instead of running it immediately.
Closing in favor of PR #29 (build/959-958-lock-remediation).
The Codex audit identified missing lock(stateLock) guards on activePositions/entryOrders mutations in Retest.cs. Per the handover protocol, a fresh branch was created to trigger the Gemini Bot Audit Gauntlet on main: #29
Codex PR #27 audit flagged all activePositions / entryOrders writes in
ExecuteRetestEntry and ExecuteRetestManualEntry as missing the StateLock
Rule (Manifesto S2). Six writes wrapped in lock(stateLock) inline blocks:
- activePositions[entryName] = pos (both methods)
- activePositions.TryRemove in null-submit rollback (both methods)
- entryOrders[entryName] = entryOrder (both methods)
No logic change -- TryRemove + early return already present from Build 956.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
github-actions
Bot
added
Orders / Callbacks
Core Strategy
Workflows / CI
Agent / Manifesto
labels
![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
Audit Status
Notes
Fresh PR per GEMINI.md Section 6 Fresh PR Rule. Supersedes PR #26.
Closes #26
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores