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
This PR configures custom providers in opencode.json and updates the workflows to use the correct model identifiers. This should resolve the premature exits and hangs in the CI/CD pipeline.
Summary by cubic
Removes unstable OpenCode-based Qwen/GLM review workflows and rebuilds the Jules PR audit to prevent CI hangs and missed comments. Also tightens order-callback logic to reduce follower desyncs and stop/target mismatches.
Dependencies
Deleted qwen-review.yml and glm-review.yml; standardized on jules-pr-review.yml.
Rebuilt Jules audit: resolves branch via gh, polls v1alpha/.../activities with a 60‑minute window, and posts a concise summary back to the PR.
Pinned release-drafter/release-drafter and actions/checkout; switched link checks to JustinBeckwith/linkinator-action; added whitespace/diff guardrails in AGENTS.md, CODEX.md, GEMINI.md, JULES.md; trimmed oversized docs.
Bug Fixes
Executions: de‑dup execution events, reduce stop quantity on target/trim fills, and run the shadow check immediately after executions.
Followers: anchor cascade matching to avoid cross-signal cascades, honor cancel/replace FSM for targets/stops, reassert expectedPositions before replacements, and always clear _propagationActive.
Account updates: track expected positions for both master and fleet accounts and enqueue terminal updates to reduce ghost refs and false flattens.
Written for commit 60c7b81. Summary will update on new commits.
CodeAnt-AI Description
Fix follower order state handling and remove failing AI review workflows
What Changed
Follower and master order updates now keep expected position counts in sync after fills, cancels, replacements, and stop/trim events, which reduces false emergency flattens and stale desync warnings
Master-to-follower price moves and cancel cascades now use clearer matching rules so unrelated follower orders are less likely to be affected
Jules PR audits now wait longer and poll less often, so long-running reviews can finish and post their summary back to the PR
Removed the Qwen and GLM review workflows that were failing in CI, and cleaned up related workflow references
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
@codeant-ai ask: Your question here
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
@codeant-ai ask: Can you suggest a safer alternative to storing this secret?
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
@codeant-ai: Your feedback here
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
@codeant-ai: Do not flag unused imports.
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
@codeant-ai: review
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Improvements
Refined order and execution handling for more reliable fills, cancellations, follower propagation, and cleanup.
Faster/more deterministic orphan reconciliation and cascade cleanup paths.
Chores
Updated CI workflows: swapped review/link-check actions, pinned checkout/release steps, and tightened workflow permissions.
Removed optional wait step in one review flow.
Documentation
Added stricter diff-size and whitespace rules and updated mission/implementation plans.
… complete
- master_roadmap.md: Phase 4 closed, Build-984 Source Hardening opened
- nexus_a2a.json: Phase set to B984_P3_ARCHITECT, 12 deferred findings catalogued
- docs/brain/build984_architect_intake.md: P1->P3 intake brief for Claude (F-01 to F-04 evidenced)
Phase 4 declaration: ProcessOnStateChange extraction verified live in src/V12_002.Lifecycle.cs
at handlers: SetDefaults(93) Configure(220) DataLoaded(302) Realtime(404) Terminated(451).
12 Arena findings triaged as pre-existing source defects, deferred to this mission.
We reviewed changes in 7da6989...60c7b81 on this pull request. Below is the summary for the review, and you can see the individual issues we found as inline review comments.
AI Review is run only on demand for your team. We're only showing results of static analysis review right now. To trigger AI Review, comment @deepsourcebot review on this thread.
The reason will be displayed to describe this comment to others. Learn more.
PropagateMaster_ResolveFollowers has a cyclomatic complexity of 36 with "very-high" risk
A function with high cyclomatic complexity can be hard to understand and
maintain. Cyclomatic complexity is a software metric that measures the number of
independent paths through a function. A higher cyclomatic complexity indicates
that the function has more decision points and is more complex.
This PR pins and migrates CI workflow actions, rewrites the Jules audit orchestration and polling, adds agent diff/whitespace constraints to docs, rewrites Phase‑5 planning pages, and refactors V12_002 order callbacks/propagation/execution into staged helpers with explicit FSM and defensive re-syncs.
Adds rules banning whitespace/line-ending/indentation mutations across files and enforcing a 150,000-character maximum PR diff size.
Implementation Plan & Mission Dashboard docs/brain/implementation_plan.md, docs/brain/task.md
Rewrites implementation_plan.md to a Phase 5 forensic repair plan and condenses task.md mission dashboard and execution log; updates objectives and metadata for Phase 5.
V12_002 Order Callback Refactoring
Layer / File(s)
Summary
Account Order Dispatch src/V12_002.Orders.Callbacks.AccountOrders.cs
OnAccountOrderUpdate delegates expected-position updates to ProcessAccountOrder_UpdateMasterExpected or ProcessAccountOrder_UpdateFleetExpected, then enqueues terminal update.
HandleMatchedFollowerOrder is restructured into FSM-aware helpers (HandleMatchedFollower_PendingCancelReplace, HandleMatchedFollower_TargetReplaceCancel, HandleMatchedFollower_StopReplacement, HandleMatchedFollower_PendingCleanupPurge) that return booleans to indicate handling.
ExecuteFollowerCascadeCleanup now delegates to ExecuteFollowerCascade_SuppressMasterReplace, ExecuteFollowerCascade_ResolveFollowers (signal-based TRADETYPE parsing), ExecuteFollowerCascade_CleanupUnfilled, and ExecuteFollowerCascade_EmergencyFlattenFilled.
Flat Position Sync & Orphan Reconciliation src/V12_002.Orders.Callbacks.Execution.cs
HandleFlatPositionUpdate delegates to HandleFlatPosition_SyncExpected, then conditionally to HandleFlatPosition_ReconcileOrphans (short-circuits when activePositions.Count == 0), and finally HandleFlatPosition_CleanupActivePositions.
Execution Dedup & Fill Dispatch src/V12_002.Orders.Callbacks.Execution.cs
ProcessOnExecutionUpdate performs dedup early (ProcessOnExecution_Dedup with early returns), then dispatches to ProcessOnExecution_HandleStopFill, ProcessOnExecution_HandleTargetFill, ProcessOnExecution_HandleTrimFill, followed by ProcessOnExecution_RunShadowCheck.
"🐰 I hopped through workflows and docs tonight,
Jules now finds branches by CLI light,
Followers dance in FSM-ordered rows,
Phase five maps where the mission goes."
🚥 Pre-merge checks | ✅ 3 | ❌ 2
❌ Failed checks (2 warnings)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 9.52% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check
⚠️ Warning
The PR description lacks the required template structure, mandatory pre-flight checklist gates, and test results section needed for a Phase 5 engineering change.
Restructure the description to follow the template: add Build Tag/Mission Context, list Files Changed with rationales, complete all Mandatory Gates (ASCII, Lock-Free Audit, Lint, Build Readiness, Deploy Sync, BUILD_TAG verification), include Architecture Review items, and provide Test Results from LogicAudit or AMAL harness output.
✅ Passed checks (3 passed)
Check name
Status
Explanation
Title check
✅ Passed
The title 'Configure OpenCode Custom Providers (Qwen & GLM)' directly describes a key workflow change (adopting custom providers for two AI review systems), but misses the major equally-important fix: rebuilding Jules polling and removing broken OpenCode workflows.
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
✏️ Tip: You can configure your own custom pre-merge checks in the settings.
✨ Finishing Touches📝 Generate docstrings
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch fix/opencode-config-v2
Tip
💬 Introducing Slack Agent: The best way for teams to turn conversations into code.
Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Generate code and open pull requests
Plan features and break down work
Investigate incidents and troubleshoot customer tickets together
Automate recurring tasks and respond to alerts with triggers
Summarize progress and report instantly
Built for teams:
Shared memory across your entire org—no repeating context
Per-thread sandboxes to safely plan and execute work
Governance built-in—scoped access, auditability, and budget controls
One agent for your entire SDLC. Right inside Slack.
This PR migrates GLM and Qwen workflows from vendor-specific actions to the unified anomalyco/opencode/github@latest provider. The migration is correctly implemented for GLM but contains two critical defects in the Qwen workflow that will prevent it from functioning.
Critical Issues Found
qwen-review.yml:
Logic Error (Line 21): The || operator fallback syntax is invalid in GitHub Actions env: context and will fail at runtime
Security/Permissions (Lines 12-13): Missing permissions: block will prevent the workflow from posting review comments (silently fails)
These must be fixed before merge to ensure the Qwen workflow operates correctly.
Changes Reviewed
✅ glm-review.yml: Properly migrated with permissions and standardized environment variables
✅ Documentation files: Formatting updates only, no logic changes
Recommendation: Fix the two critical defects in qwen-review.yml before merging.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
The reason will be displayed to describe this comment to others. Learn more.
🛑 Logic Error: GitHub Actions doesn't support || operator in env: context. This will fail at runtime if QWEN_TOKEN is undefined. The fallback to DASHSCOPE_API_KEY won't work as intended.
The reason will be displayed to describe this comment to others. Learn more.
🛑 Security Vulnerability: Missing permissions block prevents workflow from commenting on PRs. The glm-review.yml workflow includes this block but it's absent here, causing silent failure when OpenCode attempts to post review comments.1
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces new repository-wide constraints regarding whitespace mutations and strict diff character limits across several documentation files. A significant portion of the changes involves architectural refactoring within the order management system, specifically decomposing complex, monolithic methods in the src/V12_002.Orders.Callbacks namespace into smaller, modular private helper functions to improve maintainability. Additionally, the mission dashboard and implementation plan have been reset to align with the upcoming 'Phase 5 Distributed Pipeline' objectives. I have no feedback to provide as no review comments were included in the request.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR is currently not up to standards due to a significant mismatch between the stated intent (OpenCode provider configuration) and the implementation, which includes a massive refactoring of core C# order logic and a project transition to Phase 5. Several critical issues prevent merging: the missing 'opencode.json' file mentioned in the description, high-severity security risks in CI/CD workflows, and the lack of unit tests for the significantly refactored order handling logic.
While the refactoring introduces a necessary fix for REAPER desyncs via fleet account tracking, burying such a critical logic change within an undocumented refactor violates the repository's 'Surgical Changes' protocol. Furthermore, the complete overwrite of documentation files has resulted in the loss of Phase 4 history without archival. Codacy analysis indicates 58 new issues, many related to security and code complexity, that must be addressed.
About this PR
The PR exhibits significant scope misalignment; the description focuses on custom provider configuration, but the diff contains a massive refactor of core order logic and a phase shift. Additionally, 'implementation_plan.md' and 'task.md' have been overwritten with Phase 5 boilerplate, resulting in the loss of all Phase 4/Build-984 history without archival.
1 comment outside of the diffopencode.json
line 1 🟡 MEDIUM RISK
The file 'opencode.json' mentioned in the description is missing from this PR. Please verify if it was omitted accidentally.
Test suggestions
Verify GLM workflow correctly resolves GITHUB_TOKEN and GLM_API_KEY with the new model identifier.
Verify Qwen workflow correctly resolves DashScope API keys and compatible base URL.
Verify jules_audit.js successfully resolves PR branch names using 'gh pr view' when triggered by a comment.
Test ProcessAccountOrder_UpdateFleetExpected correctly updates expected positions on fleet stop/target fills.
Verify HandleFlatPosition_ReconcileOrphans handles strategy restarts where activePositions is empty.
Unit test PropagateMaster_IdentifyMove correctly categorizes entry, stop, and target moves based on object identity.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Verify jules_audit.js successfully resolves PR branch names using 'gh pr view' when triggered by a comment.
2. Test ProcessAccountOrder_UpdateFleetExpected correctly updates expected positions on fleet stop/target fills.
3. Verify HandleFlatPosition_ReconcileOrphans handles strategy restarts where activePositions is empty.
4. Unit test PropagateMaster_IdentifyMove correctly categorizes entry, stop, and target moves based on object identity.
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
This PR introduces a massive refactoring of core C# order handling logic and a critical fleet account tracking fix that were not described in the PR title, violating the 'Surgical Changes' and 'Mission Brief' protocols. While the logic for 'ProcessAccountOrder_UpdateFleetExpected' correctly addresses the 'stale expectedPositions' defect that caused REAPER false flattens, this must be explicitly called out in the PR summary. Additionally, ensure all control structures use curly braces for consistency.
private bool PropagateMaster_IdentifyMove(Order masterOrder, out string masterEntryName, out bool isEntryMove, out bool isStopMove, out bool isTargetMove, out int masterTargetNum)
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: The cyclomatic complexity of this method is high (15), exceeding the threshold of 8. Consider breaking it down into smaller, specialized identification methods (e.g., TryIdentifyEntryMove, TryIdentifyStopMove).
The reason will be displayed to describe this comment to others. Learn more.
🟡 MEDIUM RISK
Suggestion: This logic for searching activePositions to match an entry order is duplicated. Consider refactoring it into a shared private helper method like 'TryFindPositionForOrder'.
Try running the following prompt in your IDE agent:
Refactor the duplicated position lookup logic in src/V12_002.Orders.Callbacks.cs (lines 437-444 and 456-463) into a single reusable method 'private bool TryFindPositionForOrder(Order order, out string entryName, out PositionInfo pos)'.
Polling endpoint changed to /activities for better status detection
Added isFailed flag and completion detection logic
docs/brain/implementation_plan.md - Updated with forensic repair plan
Verification
Jules workflow polling logic: The changes correctly extend timeout from ~20min to 60min and use less aggressive polling (60s vs 30s intervals), reducing API spam while allowing time for deep audits.
Completion detection: The new logic properly checks for sessionCompleted and sessionFailed activity markers instead of just polling session state.
ASCII Compliance: All string literals in modified files are ASCII-safe.
No lock() statements introduced in JavaScript code.
Files Reviewed (3 files changed)
Reviewed by laguna-m.1-20260312:free · 451,202 tokens
The method HandleOrderCancelled_RollbackUnfilledEntry unconditionally returns false (line 453), yet the caller at line 377 checks if (!handled && HandleOrderCancelled_RollbackUnfilledEntry(order)) return true;. This conditional can never be true, making it dead code. The original inline code did perform the entry rollback and then fell through to RemoveGhostOrderRef (which still happens), so the behavior is preserved. However, the method signature (returning bool) and the caller's check are misleading — they imply there's a scenario where ghost-ref removal should be skipped after rollback, which isn't the case.
Suggested fix:
Change the method to `void` and remove the `if` wrapper:
// In HandleOrderCancelled:
if (!handled)
HandleOrderCancelled_RollbackUnfilledEntry(order);
RemoveGhostOrderRef(order, "CANCELLED");
return true;
// Change signature:
private void HandleOrderCancelled_RollbackUnfilledEntry(Order order)
Was this helpful? React with 👍 / 👎 | Reply gitar fix to apply this suggestion
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Target-replace cancel handling is invoked from the unfilled-entry cancel branch, so normal follower target cancel confirmations do not execute the target FSM resubmit path. This can leave a position without its replacement target after cancel/replace. Invoke target-replace cancel handling in the terminal follower-order path where target cancels are processed. [logic error]
Severity Level: Critical 🚨
- ❌ Follower target replacements never resubmit after cancel confirmation.
- ❌ Follower positions can be left without protective targets.
- ⚠️ Trailing/breakeven workflows misconfigure follower exit orders.
Steps of Reproduction ✅
1. Trigger a follower target move using the breakeven / trailing logic in
`src/V12_002.Trailing.Breakeven.cs:220-45`. For follower positions (`pos.IsFollower &&
pos.ExecutingAccount != null`), `MoveSpecificTarget` builds a `FollowerTargetReplaceSpec`,
stores it in `_followerTargetReplaceSpecs[targetOrderName]`, stamps REAPER grace, and
calls `pos.ExecutingAccount.Cancel(new[] { targetOrder });` (lines 23-42).
2. The broker confirms the cancel for that follower target order and emits an
account-level event; the strategy receives this in `OnAccountOrderUpdate`
(`src/V12_002.Orders.Callbacks.AccountOrders.cs:37-74`), which enqueues the event and
eventually calls `ProcessQueuedAccountOrder` (lines 673-705).
3. Inside `ProcessQueuedAccountOrder`, for a follower account it iterates
`activePositions` and uses `TryFindOrderInPosition` (lines 195-212) to find a matching
position key, then calls `HandleMatchedFollowerOrder(matchedEntry, matchedPos, order,
acctName, reason)` at line 702.
4. For a target cancel, `TryFindOrderInPosition` matches the order via one of the
`targetNOrders` dictionaries, but in `HandleMatchedFollowerOrder` (lines 301-355) the
first branch gate is `entryOrders.TryGetValue(matchedEntry, out var entryOrder) &&
(entryOrder == order || ...) && !matchedPos.EntryFilled`. Since `entryOrders` contains the
entry order, not the target order, this condition fails; the code takes the `else` branch
at line 341, which only handles stop replacements
(`HandleMatchedFollower_StopReplacement(order)`), pending cleanup
(`HandleMatchedFollower_PendingCleanupPurge(order)`) and terminal logging /
`RemoveGhostOrderRef(order, reason)` — it **never** calls
`HandleMatchedFollower_TargetReplaceCancel(order)` there.
5. The new target-FSM handler `HandleMatchedFollower_TargetReplaceCancel(order)` (defined
at lines 431–463) is only invoked from the unfilled-entry cancel branch via the snippet at
lines 331–335. Because follower target cancel confirmations never satisfy the entry-order
branch predicate, the intended FSM path that scans `_followerTargetReplaceSpecs`, matches
`CancellingOrderId`, and calls `TriggerCustomEvent(o =>
SubmitFollowerTargetReplacement(capturedKey, captured), null);` (lines 447–455) is not
executed, leaving the follower position without its replacement target even though Phase 1
recorded the FSM spec as documented in `src/V12_002.Symmetry.Replace.cs:19-23`.
This is a comment left during a code review.
**Path:** src/V12_002.Orders.Callbacks.AccountOrders.cs
**Line:** 331:335
**Comment:***Logic Error: Target-replace cancel handling is invoked from the unfilled-entry cancel branch, so normal follower target cancel confirmations do not execute the target FSM resubmit path. This can leave a position without its replacement target after cancel/replace. Invoke target-replace cancel handling in the terminal follower-order path where target cancels are processed.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Expected-position rollback on replacement submit failure is gated by !zeroStartReasserted, but the zero-start path already adds an expected-position delta earlier. If submit throws in that path, the pre-added expected delta is never reverted, leaving stale expected state and sync-pending behavior without a live order. [logic error]
Severity Level: Major ⚠️
- ⚠️ expectedPositions can remain inflated after replacement submit failure.
- ⚠️ REAPER may chase ghost exposure without live follower entries.
- ⚠️ Dispatch-sync flags remain set, delaying future dispatch cycles.
Steps of Reproduction ✅
1. Initiate a follower entry replacement via a master entry move: `ProcessOnOrderUpdate`
in `src/V12_002.Orders.Callbacks.cs:20-32` calls `PropagateMasterPriceMove(order,
limitPrice, stopPrice, quantity)` at line 29, which for entry moves eventually calls
`PropagateFollowerEntryReplace` and sets up a `FollowerReplaceSpec` (see
`PropagateMasterEntryMove` and `PropagateFollowerEntryReplace` in
`src/V12_002.Orders.Callbacks.Propagation.cs:324-395`).
2. The follower replace FSM records the spec in `_followerReplaceSpecs` and sends a
cancel; when the broker confirms the cancel, `OnAccountOrderUpdate` ->
`ProcessQueuedAccountOrder` -> `HandleMatchedFollowerOrder` route the event to
`HandleMatchedFollower_PendingCancelReplace`
(`src/V12_002.Orders.Callbacks.AccountOrders.cs:357-429`), which, on a matching FSM, calls
`SubmitFollowerReplacement(sigName, acctNameCapture, fsmCapture.PendingPrice,
fsmCapture.PendingQty, fsmCapture)` at line 414.
3. In `SubmitFollowerReplacement` (`src/V12_002.Orders.Callbacks.Propagation.cs:448-473`),
the first step is `SubmitFollowerReplacement_ReassertExpected(...)` (lines 475-505). If a
prior cascade cleanup zeroed `expectedPositions[ExpKey(accountName)]` but the FSM is now
resubmitting a replacement, `expectedPositions.TryGetValue(_b948ExpKey, out
_b948CurrentExp);` plus `zeroStartReasserted = _b948CurrentExp == 0 && qty != 0;` at lines
482-485 cause the guard path to run: `AddExpectedPositionDeltaLocked(_b948ExpKey,
_b948Delta);` and `MarkDispatchSyncPending(_b948ExpKey);` (lines 487-491) bump
expectedPositions before the new order is submitted.
4.`SubmitFollowerReplacement` then creates an order and calls
`SubmitFollowerReplacement_SubmitEntry(acct, newEntry, fleetSignalName, expectedKey,
expectedDelta, zeroStartReasserted)`
(`src/V12_002.Orders.Callbacks.Propagation.cs:521-543`). In
`SubmitFollowerReplacement_SubmitEntry`, the catch block at lines 534-541 only rolls back
`expectedDelta` if `!zeroStartReasserted && expectedDelta != 0`, but in the zero-start
reassert path the earlier delta was added directly in
`SubmitFollowerReplacement_ReassertExpected` and is **not** captured in `expectedDelta`.
If `acct.Submit(new[] { newEntry })` throws in this zeroStartReasserted path (e.g., broker
submission failure), `expectedPositions[expectedKey]` remains inflated because the catch
block does not revert the guard delta, leaving non-zero expectedPositions for an account
that has no live replacement entry, and leaving `MarkDispatchSyncPending` in effect.
5. Subsequent REAPER / cleanup logic that relies on expectedPositions (e.g., meta-guard
checks in `src/V12_002.Orders.Management.Cleanup.cs:123-140` and the REAPER desync
comments at `src/V12_002.Orders.Callbacks.Propagation.cs:477-481`) now sees a phantom
expected position with no corresponding `activePositions` entry, which is treated as a
desync and can trigger unnecessary repair/flatten behavior for that follower account.
This is a comment left during a code review.
**Path:** src/V12_002.Orders.Callbacks.Propagation.cs
**Line:** 534:540
**Comment:***Logic Error: Expected-position rollback on replacement submit failure is gated by `!zeroStartReasserted`, but the zero-start path already adds an expected-position delta earlier. If submit throws in that path, the pre-added expected delta is never reverted, leaving stale expected state and sync-pending behavior without a live order.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: The fleet expected-position sync treats PartFilled stop updates as a full close and immediately forces expected size to zero. On partial stop fills this makes expected and actual diverge while contracts are still open, which can trigger false desync/flatten logic. Only zero expected size on fully filled stop, and apply a delta for partial fills. [logic error]
Severity Level: Critical 🚨
- ❌ Fleet follower stops can trigger false emergency flattens.
- ⚠️ expectedPositions zeroed while follower contracts still open.
- ⚠️ REAPER desync logic sees spurious Expected != Actual state.
Steps of Reproduction ✅
1. Run the V12_002 strategy (partial class in `src/V12_002.cs`) on a master account with
at least one fleet follower account configured (fleet-account logic referenced in
`src/V12_002.UI.IPC.Commands.Fleet.cs:189` which notes state is driven by
`OnAccountOrderUpdate`).
2. Open a fleet follower position so that `activePositions` holds a follower
`PositionInfo` with a live stop order whose `order.Name` starts with `"Stop_"` (this is
the naming convention expected by `ProcessAccountOrder_UpdateFleetExpected` at
`src/V12_002.Orders.Callbacks.AccountOrders.cs:109-120`).
3. Cause a partial stop fill on the follower (e.g., multi-contract stop where only part of
the quantity fills). NinjaTrader raises an account-level callback which is handled by
`OnAccountOrderUpdate(object sender, OrderEventArgs e)` at
`src/V12_002.Orders.Callbacks.AccountOrders.cs:37-74`.
4. In `OnAccountOrderUpdate`, when `IsFleetAccount(acct)` is true and `order.OrderState ==
OrderState.PartFilled`, the code takes the fleet branch and calls
`ProcessAccountOrder_UpdateFleetExpected(order, acct)` (lines 64–71 of the same file),
which executes the snippet at lines 111–118: because `order.Name.StartsWith("Stop_")`, it
unconditionally enqueues `ctx.SetExpectedPositionLocked(fExpKey, 0)` even though the
underlying `PositionInfo` still has `RemainingContracts > 0` (see
`HandleMatchedFollower_StopReplacement` using `RemainingContracts` at
`src/V12_002.Orders.Callbacks.AccountOrders.cs:491-510`).
5. Subsequent desync / REAPER logic (e.g., the cascade comments at
`src/V12_002.Orders.Callbacks.AccountOrders.cs:582-585` explicitly describing the critical
scenario `actualQty != 0, expectedQty == 0 -> Emergency Flatten`) now sees a fleet account
whose `expectedPositions[ExpKey(acct.Name)]` is 0 while its `activePositions` entry is
still partially open, enabling false REAPER desync detection and premature emergency
flattening during normal partial stop fill scenarios.
This is a comment left during a code review.
**Path:** src/V12_002.Orders.Callbacks.AccountOrders.cs
**Line:** 111:118
**Comment:***Logic Error: The fleet expected-position sync treats `PartFilled` stop updates as a full close and immediately forces expected size to zero. On partial stop fills this makes expected and actual diverge while contracts are still open, which can trigger false desync/flatten logic. Only zero expected size on fully filled stop, and apply a delta for partial fills.
Validate the correctness of the flagged issue. If correct, How can I resolve this? If you propose a fix, implement it and please make it concise.
Once fix is implemented, also check other comments on the same PR, and ask user if the user wants to fix the rest of the comments as well. if said yes, then fetch all the comments validate the correctness and implement a minimal fix
Polling https.get hangs on network error; JSON.parse can crash the script.
Two problems in the same block:
https.get has no req.on('error', ...) handler. A network error emits on the request object, which is neither caught by the Promise constructor nor by any other handler, leaving the Promise permanently unresolved and the loop iteration stuck.
JSON.parse(body) executes synchronously inside res.on('end', ...). A malformed or empty response (e.g., a rate-limit HTML page) throws, which is not caught by the Promise and becomes an unhandled exception, killing the script instead of failing gracefully.
🔧 Proposed fix
-sessionData = await new Promise((resolve) => {- https.get(pollOptions, (res) => {- let body = '';- res.on('data', (chunk) => body += chunk);- res.on('end', () => resolve(JSON.parse(body)));- });-});+sessionData = await new Promise((resolve, reject) => {+ const req = https.get(pollOptions, (res) => {+ let body = '';+ res.on('data', (chunk) => body += chunk);+ res.on('end', () => {+ try { resolve(JSON.parse(body)); }+ catch (e) { reject(new Error(`Poll response parse error: ${e.message} — body: ${body.slice(0, 200)}`)); }+ });+ });+ req.on('error', reject);+});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/jules-pr-review.yml around lines 153 - 159, The Promise
wrapping the https.get call that assigns sessionData can hang on network errors
and crash on malformed JSON; modify the new Promise block around
https.get(pollOptions, ...) to attach an error handler on the request
(req.on('error', ...) or the callback's returned request) that rejects the
Promise on network errors and also wrap the JSON.parse(body) inside a try/catch
in the res.on('end', ...) handler so you reject the Promise on parse errors (and
optionally resolve with null/throw a descriptive error) to ensure the awaiting
loop can handle failures instead of hanging or crashing.
53-56: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win
< and > not filtered — prompt injection via XML tag breakout.
The current sanitization replaces newlines and quotes but leaves <, >, and / intact. A comment body containing </comment_body_untrusted>INJECTED_INSTRUCTION will close the trusted-data tag early, and any text that follows is processed by Jules as a direct instruction rather than untrusted data. Since issue_comment events can be created by any GitHub user with read access to the repo, this is an exploitable prompt injection path.
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/jules-pr-review.yml around lines 53 - 56, The sanitization
for commentBody (assigned to safeCommentBody) currently strips newlines and
quotes but leaves angle brackets and slashes, allowing XML/HTML tag injection;
update the replace chain that constructs safeCommentBody (the .replace/.slice
sequence) to also remove or escape '<', '>', and '/' characters (e.g., add a
.replace call targeting /[<>\/]/g to strip or convert them to safe entities) so
untrusted comments cannot close or inject into XML/HTML-like tags.
src/V12_002.Orders.Callbacks.AccountOrders.cs (1)
78-104: ⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
Use incremental fill deltas here, not cumulative order.Filled.
These helpers currently treat PartFilled as if the whole stop is flat, and they subtract the cumulative order.Filled value on every target/trim partial. On multi-update fills that double-counts earlier executions and can push expectedPositions away from broker state, which is exactly what the downstream desync logic keys off. This needs to apply only the newly filled delta per update, or be moved onto the incremental execution path instead.
Also applies to: 111-138
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/V12_002.Orders.Callbacks.AccountOrders.cs` around lines 78 - 104, This
logic is using the cumulative order.Filled on every update, causing
double-counting; change it to apply only the incremental fill delta by tracking
the previous filled amount for the order (e.g. a per-order map keyed by order id
or order.Name) and computing int delta = order.Filled - previousFilled (skip if
delta == 0), then use delta instead of filledQty when adjusting
ctx.expectedPositions in the Enqueue lambda and update the saved previousFilled
to order.Filled; apply the same change to the similar block referenced at lines
111-138; adjust handling for sign (subtract delta when currentExp > 0, add delta
when currentExp < 0) and keep use of ExpKey(Account.Name),
SetExpectedPositionLocked and _nakedPositionFirstSeen as-is.
Undo the zero-start reassertion when replacement submit fails.
SubmitFollowerReplacement_ReassertExpected adds to expectedPositions and marks dispatch sync pending when the account was at zero, but the submit failure path only rolls back expectedDelta. If acct.Submit(...) throws after the zero-start path, the follower is left with a synthetic expected position and no replacement order.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/glm-review.yml:
- Line 21: Replace the mutable tag "anomalyco/opencode/github@latest" with a
specific commit SHA to pin the action; update the uses entry (the string
"anomalyco/opencode/github@latest") to
"anomalyco/opencode/github@<FULL_COMMIT_SHA>" where <FULL_COMMIT_SHA> is the
exact 40-character commit hash from the anomalyco/opencode/github repository you
want to pin to, commit the change, and verify the workflow still runs as
expected.
In @.github/workflows/qwen-review.yml:
- Line 18: Replace the mutable action reference anomalyco/opencode/github@latest
with the specific commit SHA recommended
(anomalyco/opencode/github@2c14fc5586fe0b88e5c04732d2e846769cc35671) to pin the
action; update the uses entry in the workflow so it matches the pinned pattern
already used for actions/checkout.
- Line 24: Update the model identifier to include the provider prefix so
OpenCode can resolve it (change "model: qwen-plus" to the provider-qualified
form like "model: openai/qwen-plus" wherever this YAML is used); also prefer
configuring the provider via OpenCode's native configuration (opencode.json)
instead of relying solely on OPENAI_BASE_URL because the
`@ai-sdk/openai-compatible` options are dropped during the provider transform—move
provider-specific settings into opencode.json with proper namespace routing if
routing issues persist.
In `@src/V12_002.Orders.Callbacks.Execution.cs`:
- Around line 224-225: The check in the execution callback currently only treats
orders starting with "Stop_" as stops, so change the condition in the method
that calls ProcessOnExecution_HandleStopFill (the if using
orderName.StartsWith("Stop_")) to also match the "S_" prefix (e.g.,
orderName.StartsWith("Stop_") || orderName.StartsWith("S_")); do the same fix
for the other occurrence that calls ProcessOnExecution_HandleStopFill (the
second if block around the other StartWith check mentioned in the review) so
both "Stop_" and "S_" executions are routed to
ProcessOnExecution_HandleStopFill.
In `@src/V12_002.Orders.Callbacks.Propagation.cs`:
- Around line 143-149: The early return in PropagateMaster_ApplyFollowerMove
assumes ctx and ctx.Followers are non-null; change the conditional around
symmetryMasterEntryToDispatch.TryGetValue(masterEntryName, out string
dispatchId) && symmetryDispatchById.TryGetValue(dispatchId, out var ctx) to also
verify ctx != null and ctx.Followers != null before returning ctx.Followers, and
if either is null fall back to the existing scan path so you don't iterate a
null snapshot; update the check around symmetryDispatchById/ctx and the return
site accordingly.
---
Outside diff comments:
In @.github/workflows/jules-pr-review.yml:
- Around line 153-159: The Promise wrapping the https.get call that assigns
sessionData can hang on network errors and crash on malformed JSON; modify the
new Promise block around https.get(pollOptions, ...) to attach an error handler
on the request (req.on('error', ...) or the callback's returned request) that
rejects the Promise on network errors and also wrap the JSON.parse(body) inside
a try/catch in the res.on('end', ...) handler so you reject the Promise on parse
errors (and optionally resolve with null/throw a descriptive error) to ensure
the awaiting loop can handle failures instead of hanging or crashing.
- Around line 53-56: The sanitization for commentBody (assigned to
safeCommentBody) currently strips newlines and quotes but leaves angle brackets
and slashes, allowing XML/HTML tag injection; update the replace chain that
constructs safeCommentBody (the .replace/.slice sequence) to also remove or
escape '<', '>', and '/' characters (e.g., add a .replace call targeting
/[<>\/]/g to strip or convert them to safe entities) so untrusted comments
cannot close or inject into XML/HTML-like tags.
In `@src/V12_002.Orders.Callbacks.AccountOrders.cs`:
- Around line 78-104: This logic is using the cumulative order.Filled on every
update, causing double-counting; change it to apply only the incremental fill
delta by tracking the previous filled amount for the order (e.g. a per-order map
keyed by order id or order.Name) and computing int delta = order.Filled -
previousFilled (skip if delta == 0), then use delta instead of filledQty when
adjusting ctx.expectedPositions in the Enqueue lambda and update the saved
previousFilled to order.Filled; apply the same change to the similar block
referenced at lines 111-138; adjust handling for sign (subtract delta when
currentExp > 0, add delta when currentExp < 0) and keep use of
ExpKey(Account.Name), SetExpectedPositionLocked and _nakedPositionFirstSeen
as-is.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdc0a891-7cd6-4e25-a8e0-33617844c199
📥 Commits
Reviewing files that changed from the base of the PR and between 7da6989 and 62173cc.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Handle S_ stop executions the same way as Stop_.
This refactor now routes stop fills only through StartsWith("Stop_"), but the rest of the strategy still treats both Stop_ and S_ as valid stop prefixes. An S_... execution will skip the stop-fill path here, so remaining targets stay live and pending cleanup never runs.
Possible fix
- if (orderName.StartsWith("Stop_"))+ if (orderName.StartsWith("Stop_") || orderName.StartsWith("S_"))
ProcessOnExecution_HandleStopFill(orderName, price, quantity);
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/V12_002.Orders.Callbacks.Execution.cs` around lines 224 - 225, The check
in the execution callback currently only treats orders starting with "Stop_" as
stops, so change the condition in the method that calls
ProcessOnExecution_HandleStopFill (the if using orderName.StartsWith("Stop_"))
to also match the "S_" prefix (e.g., orderName.StartsWith("Stop_") ||
orderName.StartsWith("S_")); do the same fix for the other occurrence that calls
ProcessOnExecution_HandleStopFill (the second if block around the other
StartWith check mentioned in the review) so both "Stop_" and "S_" executions are
routed to ProcessOnExecution_HandleStopFill.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Guard null dispatch snapshots before returning followers.
TryGetValue(..., out var ctx) only proves the key exists. If ctx or ctx.Followers is null during teardown or before publication, PropagateMaster_ApplyFollowerMove will blow up on the foreach and skip the propagation entirely. Fall back to the scan unless you have a non-null follower snapshot.
Possible fix
- if (symmetryMasterEntryToDispatch.TryGetValue(masterEntryName, out string dispatchId) &&- symmetryDispatchById.TryGetValue(dispatchId, out var ctx))+ if (symmetryMasterEntryToDispatch.TryGetValue(masterEntryName, out string dispatchId) &&+ symmetryDispatchById.TryGetValue(dispatchId, out var ctx) &&+ ctx != null &&+ ctx.Followers != null &&+ ctx.Followers.Length > 0)
{
// ADR-019: ctx.Followers is an immutable snapshot published via Interlocked.CompareExchange.
// Zero-alloc, lock-free, point-in-time consistent. Hot path on every master price move.
return ctx.Followers;
}
📝 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.
// ADR-019: ctx.Followers is an immutable snapshot published via Interlocked.CompareExchange.
// Zero-alloc, lock-free, point-in-time consistent. Hot path on every master price move.
returnctx.Followers;
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/V12_002.Orders.Callbacks.Propagation.cs` around lines 143 - 149, The
early return in PropagateMaster_ApplyFollowerMove assumes ctx and ctx.Followers
are non-null; change the conditional around
symmetryMasterEntryToDispatch.TryGetValue(masterEntryName, out string
dispatchId) && symmetryDispatchById.TryGetValue(dispatchId, out var ctx) to also
verify ctx != null and ctx.Followers != null before returning ctx.Followers, and
if either is null fall back to the existing scan path so you don't iterate a
null snapshot; update the check around symmetryDispatchById/ctx and the return
site accordingly.
The reason will be displayed to describe this comment to others. Learn more.
5 issues found across 16 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/qwen-review.yml">
<violation number="1" location=".github/workflows/qwen-review.yml:18">
P1: Pin `anomalyco/opencode/github` to a specific commit SHA instead of the mutable `@latest` tag. This action receives `GITHUB_TOKEN` and API keys, so an unpinned reference creates a supply-chain attack vector.</violation>
<violation number="2" location=".github/workflows/qwen-review.yml:24">
P1: OpenCode requires `provider_id/model_id` format for model identifiers. The bare `qwen-plus` will fail to resolve. Since this is routed through an OpenAI-compatible endpoint via `OPENAI_BASE_URL`, use `openai/qwen-plus`.</violation>
</file>
<file name=".github/workflows/glm-review.yml">
<violation number="1" location=".github/workflows/glm-review.yml:21">
P1: Pin `anomalyco/opencode/github` to a commit SHA instead of the mutable `@latest` tag. This action has access to secrets and write permissions, making it a supply-chain risk—the same risk the checkout pin was intended to mitigate.</violation>
</file>
<file name="src/V12_002.Orders.Callbacks.Execution.cs">
<violation number="1" location="src/V12_002.Orders.Callbacks.Execution.cs:224">
P1: Missing `S_` prefix check. The rest of the strategy (e.g., `HandleOrderCancelled`, `HandleMatchedFollower_PendingCleanupPurge`) treats both `Stop_` and `S_` as valid stop prefixes. An `S_`-prefixed stop execution will skip this path entirely, leaving targets live and cleanup unrun.</violation>
<violation number="2" location="src/V12_002.Orders.Callbacks.Execution.cs:249">
P1: Behavioral regression: `ProcessOnExecution_RunShadowCheck()` now runs unconditionally, but in the original code the `return;` inside the target-fill `alreadyProcessed` guard exited the entire `ProcessOnExecutionUpdate` method, skipping `ShadowEngineCheck()`. After extracting to a sub-method, the `return;` only exits `HandleTargetFill`, so the shadow check fires on duplicate fills that should be no-ops. In a live trading system this could trigger unintended trailing-stop adjustments on already-processed executions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
The reason will be displayed to describe this comment to others. Learn more.
P1: Behavioral regression: ProcessOnExecution_RunShadowCheck() now runs unconditionally, but in the original code the return; inside the target-fill alreadyProcessed guard exited the entire ProcessOnExecutionUpdate method, skipping ShadowEngineCheck(). After extracting to a sub-method, the return; only exits HandleTargetFill, so the shadow check fires on duplicate fills that should be no-ops. In a live trading system this could trigger unintended trailing-stop adjustments on already-processed executions.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/V12_002.Orders.Callbacks.Execution.cs, line 249:
<comment>Behavioral regression: `ProcessOnExecution_RunShadowCheck()` now runs unconditionally, but in the original code the `return;` inside the target-fill `alreadyProcessed` guard exited the entire `ProcessOnExecutionUpdate` method, skipping `ShadowEngineCheck()`. After extracting to a sub-method, the `return;` only exits `HandleTargetFill`, so the shadow check fires on duplicate fills that should be no-ops. In a live trading system this could trigger unintended trailing-stop adjustments on already-processed executions.</comment>
<file context>
@@ -197,6 +213,49 @@ private void ProcessOnExecutionUpdate(
+ // Build 1105: Shadow callback injection -- closes 100-500ms leader flatten gap.
+ // ManageTrailingStops covers steady-state trailing. This covers immediate
+ // execution events (stop fill, target fill) where next trailing cycle is too late.
+ ProcessOnExecution_RunShadowCheck();
+ }
+ catch (Exception ex)
</file context>
The reason will be displayed to describe this comment to others. Learn more.
P1: Missing S_ prefix check. The rest of the strategy (e.g., HandleOrderCancelled, HandleMatchedFollower_PendingCleanupPurge) treats both Stop_ and S_ as valid stop prefixes. An S_-prefixed stop execution will skip this path entirely, leaving targets live and cleanup unrun.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At src/V12_002.Orders.Callbacks.Execution.cs, line 224:
<comment>Missing `S_` prefix check. The rest of the strategy (e.g., `HandleOrderCancelled`, `HandleMatchedFollower_PendingCleanupPurge`) treats both `Stop_` and `S_` as valid stop prefixes. An `S_`-prefixed stop execution will skip this path entirely, leaving targets live and cleanup unrun.</comment>
<file context>
@@ -197,6 +213,49 @@ private void ProcessOnExecutionUpdate(
+ // ============================================================
+ // 1. STOP LOSS FILL - Manual OCO: Cancel all remaining targets
+ // ============================================================
+ if (orderName.StartsWith("Stop_"))
+ ProcessOnExecution_HandleStopFill(orderName, price, quantity);
+
</file context>
The reason will be displayed to describe this comment to others. Learn more.
⚠️Bug: Fragile failure detection via substring match on title
Line 172 detects session failure by checking if a progress update's title contains the substring 'failed' (case-insensitive). This is overly broad — a progress title like "0 tests failed", "No checks failed", or "Previously failed checks now pass" would incorrectly mark the session as failed, causing the workflow to exit with code 1.
Consider relying solely on the sessionFailed activity marker, which is the structured signal for failure, and removing the heuristic title check.
CI failed: The Jules AI forensic audit process timed out after 60 minutes, causing the CI build to fail. Additionally, an unrelated git submodule error was triggered during job cleanup.
Overview
One primary failure was identified: the jules_audit.js script timed out after 60 minutes of polling, indicating the audit session did not complete within the expected window. An additional non-fatal git submodule configuration error was noted during the post-job cleanup phase.
Failures
Jules Audit Timeout (confidence: high)
Type: test
Affected jobs: 74913063952
Related to change: yes
Root cause: The AI audit session exceeded the 60-minute execution threshold defined in the polling logic (maxAttempts = 60 at 1 minute per poll).
Suggested fix: Increase the maxAttempts constant in jules_audit.js to allow for longer processing times, or investigate if the recent configuration changes to OpenCode providers have negatively impacted the audit performance.
Git Submodule Error (confidence: high)
Type: configuration
Affected jobs: 74913063952
Related to change: no
Root cause: The repository configuration contains an entry for 'AntigravityMobile' in the Git metadata that is missing from .gitmodules.
Suggested fix: This is a cleanup-related warning/error that does not block the build logic, but it should be addressed by removing the orphan submodule reference from the git configuration to clean up log output.
Summary
Change-related failures: 1 (Jules audit timeout likely linked to new provider configurations).
Recommended action: Extend the timeout in jules_audit.js to verify if the audit eventually completes. If it continues to fail, review the performance impact of the new OpenCode provider settings on the audit service.
Rebuilds Jules AI polling logic and streamlines order execution workflows to improve CI stability. Addressing findings requires correcting the fragile substring-based failure detection, resolving the unreachable return logic in HandleOrderCancelled_RollbackUnfilledEntry, and reducing deep nesting in newly extracted methods.
⚠️Bug: Fragile failure detection via substring match on title
Line 172 detects session failure by checking if a progress update's title contains the substring 'failed' (case-insensitive). This is overly broad — a progress title like "0 tests failed", "No checks failed", or "Previously failed checks now pass" would incorrectly mark the session as failed, causing the workflow to exit with code 1.
Consider relying solely on the sessionFailed activity marker, which is the structured signal for failure, and removing the heuristic title check.
Multiple extracted methods (HandleMatchedFollower_StopReplacement, ExecuteFollowerCascade_CleanupUnfilled, ExecuteFollowerCascade_EmergencyFlattenFilled, HandleMatchedFollower_PendingCleanupPurge, HandleFlatPosition_ReconcileOrphans) have method bodies indented 16-32 spaces deep, carried over from their original inline context. While this compiles correctly (C# is brace-matched, not indentation-sensitive), the severe mismatch between method declaration indentation (8 spaces) and body indentation (24+ spaces) makes the code extremely difficult to read and maintain. For example, HandleMatchedFollower_StopReplacement (line 491) opens its brace at 12 spaces but has return true at 24 spaces and the closing } for the if-block at 16 spaces.
Suggested fix
Re-indent the bodies of all extracted methods to match their new nesting level (typically 8 or 12 spaces for a class method body). An IDE auto-format (Ctrl+K, Ctrl+D in Visual Studio) on these files would fix all instances.
The method HandleOrderCancelled_RollbackUnfilledEntry unconditionally returns false (line 453), yet the caller at line 377 checks if (!handled && HandleOrderCancelled_RollbackUnfilledEntry(order)) return true;. This conditional can never be true, making it dead code. The original inline code did perform the entry rollback and then fell through to RemoveGhostOrderRef (which still happens), so the behavior is preserved. However, the method signature (returning bool) and the caller's check are misleading — they imply there's a scenario where ghost-ref removal should be skipped after rollback, which isn't the case.
Suggested fix
Change the method to `void` and remove the `if` wrapper:
// In HandleOrderCancelled:
if (!handled)
HandleOrderCancelled_RollbackUnfilledEntry(order);
RemoveGhostOrderRef(order, "CANCELLED");
return true;
// Change signature:
private void HandleOrderCancelled_RollbackUnfilledEntry(Order order)
✅ 1 resolved✅ Edge Case: No guard against missing/empty activities array on API error
📄 .github/workflows/jules-pr-review.yml:162-176
If the activities endpoint returns an HTTP error (e.g., 429 rate-limit or 5xx), the response body may not contain an activities array. The current code silently skips via the if (activitiesData && activitiesData.activities) check, which is fine, but it means the loop will silently poll for 60 minutes without ever detecting completion if the API is persistently erroring. Consider logging when activitiesData is unexpectedly null/missing activities so operators can diagnose stuck workflows.
🤖 Prompt for agents
Code Review: Rebuilds Jules AI polling logic and streamlines order execution workflows to improve CI stability. Addressing findings requires correcting the fragile substring-based failure detection, resolving the unreachable return logic in `HandleOrderCancelled_RollbackUnfilledEntry`, and reducing deep nesting in newly extracted methods.
1. 💡 Quality: Extracted methods retain deeply nested indentation from inline code
Files: src/V12_002.Orders.Callbacks.AccountOrders.cs:491-505, src/V12_002.Orders.Callbacks.AccountOrders.cs:633-647, src/V12_002.Orders.Callbacks.AccountOrders.cs:660-671, src/V12_002.Orders.Callbacks.Execution.cs:119-130
Multiple extracted methods (`HandleMatchedFollower_StopReplacement`, `ExecuteFollowerCascade_CleanupUnfilled`, `ExecuteFollowerCascade_EmergencyFlattenFilled`, `HandleMatchedFollower_PendingCleanupPurge`, `HandleFlatPosition_ReconcileOrphans`) have method bodies indented 16-32 spaces deep, carried over from their original inline context. While this compiles correctly (C# is brace-matched, not indentation-sensitive), the severe mismatch between method declaration indentation (8 spaces) and body indentation (24+ spaces) makes the code extremely difficult to read and maintain. For example, `HandleMatchedFollower_StopReplacement` (line 491) opens its brace at 12 spaces but has `return true` at 24 spaces and the closing `}` for the if-block at 16 spaces.
Suggested fix:
Re-indent the bodies of all extracted methods to match their new nesting level (typically 8 or 12 spaces for a class method body). An IDE auto-format (Ctrl+K, Ctrl+D in Visual Studio) on these files would fix all instances.
2. 💡 Quality: `HandleOrderCancelled_RollbackUnfilledEntry` always returns false
Files: src/V12_002.Orders.Callbacks.cs:377, src/V12_002.Orders.Callbacks.cs:437-451
The method `HandleOrderCancelled_RollbackUnfilledEntry` unconditionally returns `false` (line 453), yet the caller at line 377 checks `if (!handled && HandleOrderCancelled_RollbackUnfilledEntry(order)) return true;`. This conditional can never be true, making it dead code. The original inline code did perform the entry rollback and then fell through to `RemoveGhostOrderRef` (which still happens), so the behavior is preserved. However, the method signature (returning `bool`) and the caller's check are misleading — they imply there's a scenario where ghost-ref removal should be skipped after rollback, which isn't the case.
Suggested fix:
Change the method to `void` and remove the `if` wrapper:
// In HandleOrderCancelled:
if (!handled)
HandleOrderCancelled_RollbackUnfilledEntry(order);
RemoveGhostOrderRef(order, "CANCELLED");
return true;
// Change signature:
private void HandleOrderCancelled_RollbackUnfilledEntry(Order order)
3. ⚠️ Bug: Fragile failure detection via substring match on title
Files: .github/workflows/jules-pr-review.yml:172
Line 172 detects session failure by checking if a progress update's title contains the substring 'failed' (case-insensitive). This is overly broad — a progress title like "0 tests failed", "No checks failed", or "Previously failed checks now pass" would incorrectly mark the session as failed, causing the workflow to exit with code 1.
Consider relying solely on the `sessionFailed` activity marker, which is the structured signal for failure, and removing the heuristic title check.
Suggested fix:
const failedAct = activitiesData.activities.find(a => a.sessionFailed);
Tip
Comment Gitar fix CI or enable auto-apply: gitar auto-apply:on
Options
Auto-apply is off → Gitar will not commit updates to this branch. Display: compact → Showing less information.
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/jules-pr-review.yml:
- Around line 154-160: The polling Promise around https.get should be hardened:
add an 'error' listener on the request and on the response, handle non-2xx HTTP
status codes by rejecting or (better) resolving null, guard JSON.parse with
try/catch and resolve null on parse failures, and add a timeout to abort the
request; update the block that creates activitiesData (the Promise using
https.get, pollOptions, body, and JSON.parse) so that request.on('error'/
'timeout') and res.on('error') resolve null (or reject if you prefer consistent
handling), check res.statusCode before accumulating body and resolve null on bad
status, and wrap JSON.parse(body) in try/catch to avoid throwing.
In `@docs/brain/implementation_plan.md`:
- Line 43: Update the instruction that currently tells engineers to run
"powershell -File .\deploy-sync.ps1" so it references the documented Build &
Sync verification script "powershell -File .\scripts\build_readiness.ps1"
instead; locate the text in implementation_plan.md where the deploy-sync.ps1
command appears and replace it with the build_readiness.ps1 command so the Build
Pillar/ASCII gate verification uses the correct script name.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
Push a commit to this branch (recommended)
Create a new PR with the fixes
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 903f0ef4-bafe-4821-9cce-0867ed196ddc
📥 Commits
Reviewing files that changed from the base of the PR and between 62173cc and 60c7b81.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟠 Major | ⚡ Quick win
Polling loop has no error handling — one transient failure aborts the entire 60-minute audit.
https.get is called with no 'error' listener and JSON.parse(body) is unguarded. Across 60 polls over an hour, a single DNS hiccup, ECONNRESET, TLS retry, or non-JSON gateway response (e.g., 502/503 HTML page) will throw an unhandled exception and crash node jules_audit.js, which is exactly the failure mode the increased polling window was meant to avoid. The promise also never rejects on HTTP error status codes, so a non-2xx body will be silently fed to JSON.parse.
🛡️ Suggested hardening of the poll request
- const activitiesData = await new Promise((resolve) => {- https.get(pollOptions, (res) => {- let body = '';- res.on('data', (chunk) => body += chunk);- res.on('end', () => resolve(JSON.parse(body)));- });- });+ const activitiesData = await new Promise((resolve) => {+ const req = https.get(pollOptions, (res) => {+ let body = '';+ res.on('data', (chunk) => body += chunk);+ res.on('end', () => {+ if (res.statusCode < 200 || res.statusCode >= 300) {+ console.warn(`\nPoll non-2xx (${res.statusCode}); will retry.`);+ return resolve(null);+ }+ try {+ resolve(JSON.parse(body));+ } catch (e) {+ console.warn(`\nPoll parse error: ${e.message}; will retry.`);+ resolve(null);+ }+ });+ });+ req.on('error', (e) => {+ console.warn(`\nPoll network error: ${e.message}; will retry.`);+ resolve(null);+ });+ });
The existing if (activitiesData && activitiesData.activities) guard already short-circuits cleanly when null is returned, so transient failures simply consume an attempt instead of killing the run.
📝 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
const activitiesData = await new Promise((resolve) => {
https.get(pollOptions, (res) => {
let body = '';
res.on('data', (chunk) => body += chunk);
res.on('end', () => resolve(JSON.parse(body)));
});
});
const activitiesData = await new Promise((resolve) => {
const req = https.get(pollOptions, (res) => {
let body = '';
res.on('data', (chunk) => body += chunk);
res.on('end', () => {
if (res.statusCode < 200 || res.statusCode >= 300) {
console.warn(`\nPoll non-2xx (${res.statusCode}); will retry.`);
return resolve(null);
}
try {
resolve(JSON.parse(body));
} catch (e) {
console.warn(`\nPoll parse error: ${e.message}; will retry.`);
resolve(null);
}
});
});
req.on('error', (e) => {
console.warn(`\nPoll network error: ${e.message}; will retry.`);
resolve(null);
});
});
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/jules-pr-review.yml around lines 154 - 160, The polling
Promise around https.get should be hardened: add an 'error' listener on the
request and on the response, handle non-2xx HTTP status codes by rejecting or
(better) resolving null, guard JSON.parse with try/catch and resolve null on
parse failures, and add a timeout to abort the request; update the block that
creates activitiesData (the Promise using https.get, pollOptions, body, and
JSON.parse) so that request.on('error'/ 'timeout') and res.on('error') resolve
null (or reject if you prefer consistent handling), check res.statusCode before
accumulating body and resolve null on bad status, and wrap JSON.parse(body) in
try/catch to avoid throwing.
2. In `.github/workflows/jules-pr-review.yml`, update the `maxAttempts` variable in the polling loop to `60` and change the `setTimeout` interval from `30000` to `60000` (60 seconds) to allow for a 60-minute timeout window with less aggressive polling.
3. Once edits are complete, run `powershell -File .\deploy-sync.ps1` and verify the ASCII gate passes.
The reason will be displayed to describe this comment to others. Learn more.
⚠️ Potential issue | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash# Confirm whether deploy-sync.ps1 exists and how it relates to the documented build_readiness.ps1
fd -HI -t f 'deploy-sync.ps1'
fd -HI -t f 'build_readiness.ps1'
fd -HI -t f -e ps1 . scripts 2>/dev/null || fd -HI -t f -e ps1
Repository: mkalhitti-cloud/universal-or-strategy
Length of output: 484
Update line 43 to reference the documented Build & Sync script.
Line 43 instructs the engineer to run powershell -File .\deploy-sync.ps1, but the documented Build & Sync command for verifying the Build Pillar gate is powershell -File .\scripts\build_readiness.ps1. While deploy-sync.ps1 exists in the repo, this change ensures the handoff executes the correct verification step as intended by the implementation plan.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@docs/brain/implementation_plan.md` at line 43, Update the instruction that
currently tells engineers to run "powershell -File .\deploy-sync.ps1" so it
references the documented Build & Sync verification script "powershell -File
.\scripts\build_readiness.ps1" instead; locate the text in
implementation_plan.md where the deploy-sync.ps1 command appears and replace it
with the build_readiness.ps1 command so the Build Pillar/ASCII gate verification
uses the correct script name.
The reason will be displayed to describe this comment to others. Learn more.
2 issues found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name=".github/workflows/jules-pr-review.yml">
<violation number="1" location=".github/workflows/jules-pr-review.yml:140">
P1: Polling only the first activities page can miss completion/failure events and cause CI timeouts.</violation>
<violation number="2" location=".github/workflows/jules-pr-review.yml:172">
P2: Do not infer failure from progress title text; rely on `sessionFailed` activity only.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review, or fix all with cubic.
The reason will be displayed to describe this comment to others. Learn more.
P1: Polling only the first activities page can miss completion/failure events and cause CI timeouts.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/jules-pr-review.yml, line 140:
<comment>Polling only the first activities page can miss completion/failure events and cause CI timeouts.</comment>
<file context>
@@ -134,35 +134,55 @@ jobs:
const pollOptions = {
hostname: 'jules.googleapis.com',
- path: `/v1alpha/${sessionName}`,
+ path: `/v1alpha/${sessionName}/activities?pageSize=100`,
method: 'GET',
headers: { 'x-goog-api-key': apiKey }
</file context>
The reason will be displayed to describe this comment to others. Learn more.
P2: Do not infer failure from progress title text; rely on sessionFailed activity only.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At .github/workflows/jules-pr-review.yml, line 172:
<comment>Do not infer failure from progress title text; rely on `sessionFailed` activity only.</comment>
<file context>
@@ -134,35 +134,55 @@ jobs:
+
+ // Check for completion markers
+ const completedAct = activitiesData.activities.find(a => a.sessionCompleted);
+ const failedAct = activitiesData.activities.find(a => a.sessionFailed || (a.progressUpdated && a.progressUpdated.title && a.progressUpdated.title.toLowerCase().includes('failed')));
+
+ if (completedAct) {
</file context>
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}
![amazon-q-developer[bot]](https://avatars.githubusercontent.com/in/1220912?s=60&v=4){kind=small}
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
codeant-ai
Bot
added
the
size:L
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![codeant-ai[bot]](https://avatars.githubusercontent.com/in/646884?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
User description
This PR configures custom providers in opencode.json and updates the workflows to use the correct model identifiers. This should resolve the premature exits and hangs in the CI/CD pipeline.
Summary by cubic
Removes unstable OpenCode-based Qwen/GLM review workflows and rebuilds the Jules PR audit to prevent CI hangs and missed comments. Also tightens order-callback logic to reduce follower desyncs and stop/target mismatches.
Dependencies
qwen-review.ymlandglm-review.yml; standardized onjules-pr-review.yml.gh, pollsv1alpha/.../activitieswith a 60‑minute window, and posts a concise summary back to the PR.release-drafter/release-drafterandactions/checkout; switched link checks toJustinBeckwith/linkinator-action; added whitespace/diff guardrails inAGENTS.md,CODEX.md,GEMINI.md,JULES.md; trimmed oversized docs.Bug Fixes
expectedPositionsbefore replacements, and always clear_propagationActive.Written for commit 60c7b81. Summary will update on new commits.
CodeAnt-AI Description
Fix follower order state handling and remove failing AI review workflows
What Changed
Impact
✅ Fewer false emergency flattens✅ Fewer follower desync alerts✅ More reliable PR review runs🔄 Retrigger CodeAnt AI Review
Details
💡 Usage Guide
Checking Your Pull Request
Every time you make a pull request, our system automatically looks through it. We check for security issues, mistakes in how you're setting up your infrastructure, and common code problems. We do this to make sure your changes are solid and won't cause any trouble later.
Talking to CodeAnt AI
Got a question or need a hand with something in your pull request? You can easily get in touch with CodeAnt AI right here. Just type the following in a comment on your pull request, and replace "Your question here" with whatever you want to ask:
This lets you have a chat with CodeAnt AI about your pull request, making it easier to understand and improve your code.
Example
Preserve Org Learnings with CodeAnt
You can record team preferences so CodeAnt AI applies them in future reviews. Reply directly to the specific CodeAnt AI suggestion (in the same thread) and replace "Your feedback here" with your input:
This helps CodeAnt AI learn and adapt to your team's coding style and standards.
Example
Retrigger review
Ask CodeAnt AI to review the PR again, by typing:
Check Your Repository Health
To analyze the health of your code repository, visit our dashboard at https://app.codeant.ai. This tool helps you identify potential issues and areas for improvement in your codebase, ensuring your repository maintains high standards of code health.
Summary by CodeRabbit
Improvements
Chores
Documentation