feat: Add StrategyGroup with back-pair alignment

[Copilot]

Replaces the StrategyHub approach with StrategyGroup helpers that coordinate multiple streaming hubs by aligning both previous and current results across hubs.

• Changes

  • Added BackPair<T> and StrategyGroup overloads for 2, 3, and 4 hubs that only succeed when previous and current timestamps match across all hubs.
  • Updated the golden-cross example/test and simulation strategy to use StrategyGroup back pairs with explicit close-price sourcing.
  • Added XML documentation to the StrategyGroup helpers.

• Example

QuoteHub quotes = new();
StrategyGroup<SmaResult, SmaResult> group =
    new(quotes.ToSmaHub(50), quotes.ToSmaHub(200));

if (group.TryGetBackPair(out var fast, out var slow)
    && fast.Previous.Sma <= slow.Previous.Sma
    && fast.Current.Sma > slow.Current.Sma)
{
    // golden cross detected
}

Original prompt

For this library, I'd like to implement a container that will help external users compose a strategy that uses indicators from hubs. See if you can get this to work, something like this StrategyHub:

    private bool[] GoldenCross(
        IQuoteProvider<IQuote> quoteHub,
        int fastLookback,
        int slowLookback): StrategyHub<IReusable>
    {

        EmaHub emaFastHub = quoteHub.ToEmaHub(fastLookback);
        EmaHub emaSlowHub = quoteHub.ToEmaHub(slowLookback);

        if (emaFastHub.Results[^2].Ema < emaSlowHub.Results[^2].Ema  // or .Value
        && emaFastHub.Results[^1].Ema > emaSlowHub.Results[^1].Ema)
        {
            // cross over occurred
        }
    }

---

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

[coderabbitai]

Important

Review skipped

Bot user detected.

To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

📝 Walkthrough

Walkthrough

This pull request introduces a Server-Sent Events (SSE) infrastructure for quote streaming and adds multi-hub alignment utilities. It creates a new Test.SseServer ASP.NET Core project with two endpoints that stream quotes via SSE—one generating random quotes and another streaming predefined quotes. A new StrategyHub module provides alignment utilities for pairing results across multiple indicator hubs. The simulation tool is refactored to use HTTP-based quote streaming rather than in-process generation. Integration tests validate the alignment functionality with golden cross strategy examples. Existing simulation code is removed and replaced with server-based components.

Possibly related PRs

• \feat: Add SSE server and Golden Cross strategy simulation #1901: Makes the same code-level additions including Test.SseServer, GoldenCrossStrategy, ServerManager, StrategyHub, and related simulate/server infrastructure changes.

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[JGronholz]

One thing:

QuoteHub quotes = new();
StrategyHub<IQuote> strat_EmaCross = new(quotes);

EmaHub fast = strat_EmaCross.Use(quotes.ToEmaHub(10));
EmaHub slow = strat_EmaCross.Use(quotes.ToEmaHub(20));

DonchianHub donch = quotes.ToDonchianHub(...);
if (strat_EmaCross.TryGetLatest(donch, slow, out var f, out var s) //this is wrong, but not a compiler error
    && f.previous.Ema < s.previous.Ema //compiler error here, instead
    && f.current.Ema > s.current.Ema) //compiler error here, instead

Since this is brand new, I don't mind suggesting:

StrategyHub<IQuote, THub1, THub2>
StrategyHub.Use(THub1 hub1, THub2 hub2); //generic keeps references
if (strat_EmaCross.TryGetLatest(out var f, out var s))... (f will still be THub1Result, s will still be THub2Result)

StrategyHub`1:
This one instance is a reusable pattern that works against any (fast, slow) emas (i.e. 10/20, 50/100, 100/200).
If operating via interface only, must be careful to use correct observables (emahub).

StrategyHub<IQuote> strat_EmaCross = new(quotes);
if (strat_EmaCross.TryGetLatest(fast, slow, out var f, out var s)

StrategyHub`2:
Similar to my ReusableObserver, intended to detect conditions from a single indicator. (i.e. Rsi oversold)
Timestamp coordination not required.

StrategyHub<IQuote, RsiHub> strat_RsiExtremes = new(quotes);

StrategyHub`3:
This one instance watches two specific hubs.

StrategyHub<IQuote, EmaHub, EmaHub> strat_EmaCross = new(quotes);
strat_EmaCross.Use(emaHubFast, emaHubSlow); //strongly-typed, saves references
if (strat_EmaCross.TryGetLatest(out var f, out var s) //still becomes strongly-typed

StrategyHub`5:

StrategyHub<IQuote, EmaHub, EmaHub, RsiHub, StochRsiHub> strat = new(quotes);
strat.Use(..., ..., ..., ...); //strongly-typed, saves references
if (strat.TryGetLatest(out var f, out var s, out var rsi, out var stoch)

strat.TryGetLatest(out var f, out var s, out var rsi, out var stoch) //StrategyHub`5
calls
base.TryGetLatest(out var s, out var rsi, out var stoch) //StrategyHub`4
calls
base.TryGetLatest(out var rsi, out var stoch) //StrategyHub`3

StrategyHub`5 would retain access to StrategyHub`3.TryGetLatest(out var One, out var Two)
StrategyHub`5 should not expose/implement StrategyHub`1.TryGetLatest().
StrategyHub`1 abstract?

[DaveSkender]

@coderabbitai review overall, and for alignment to needs: containers for streaming strategies.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Find StrategyHub implementation
fd -e cs StrategyHub

Length of output: 86

---

🏁 Script executed:

#!/bin/bash
# Read the StrategyHub implementation
cat src/s-z/StrategyHub/StrategyHub.cs

Length of output: 66

---

🏁 Script executed:

#!/bin/bash
# Find and read tests related to StrategyHub
fd -e cs -p ".*Test.*StrategyHub.*" --exec cat {}

Length of output: 0

---

🏁 Script executed:

#!/bin/bash
# Check if there are any other files related to StrategyHub
rg -l "StrategyHub" --type cs -C3

Length of output: 124

---

🏁 Script executed:

#!/bin/bash
# Read the correct StrategyHub implementation
cat src/_common/Strategy/StrategyHub.cs

Length of output: 3148

---

🏁 Script executed:

#!/bin/bash
# Read the test file
cat tests/public-api/customizable/StrategyHub.Tests.cs

Length of output: 3431

---

🏁 Script executed:

#!/bin/bash
# Read the GoldenCrossStrategy usage example
cat tools/simulate/GoldenCrossStrategy.cs

Length of output: 8213

---

🏁 Script executed:

#!/bin/bash
# Look at StreamHub base classes to understand the architecture
fd -e cs StreamHub | head -5 | xargs cat

Length of output: 17870

---

@DaveSkender, I've reviewed the PR and JGronholz's type safety concerns. Here's my comprehensive analysis:

Type Safety Issue (Valid Concern)

JGronholz identified a real problem with the current design. In this code:

StrategyHub<IQuote> strat = new(quotes);
EmaHub fast = strat.Use(quotes.ToEmaHub(10));
EmaHub slow = strat.Use(quotes.ToEmaHub(20));
DonchianHub donch = quotes.ToDonchianHub(...);

// Compiles fine, but donch wasn't part of the strategy!
if (strat.TryGetLatest(donch, slow, out var f, out var s)
    && f.previous.Ema < s.previous.Ema)  // Runtime error: DonchianResult has no .Ema

The problem: Use() doesn't actually store the hubs—it just returns them. So TryGetLatest has no way to verify you're passing the correct hubs. This shifts error detection from compile-time to runtime.

Design Trade-offs

Current Design:

• ✅ Simple, minimal code
• ✅ Works with any number of hubs (2, 3, 4, etc.)
• ✅ Flexible - no pre-registration needed
• ❌ No compile-time safety
• ❌ Easy to make mistakes in complex strategies

JGronholz's Suggestion:

• ✅ Strong compile-time type safety
• ✅ Hubs registered once, can't be confused
• ✅ Clearer intent (hubs are "owned" by strategy)
• ❌ Requires multiple StrategyHub variants (StrategyHub<T, TH1>, StrategyHub<T, TH1, TH2>, etc.)
• ❌ More complex implementation and inheritance hierarchy
• ❌ Less flexible - locked to specific hubs at construction

Alternative Solutions

Option 1: Keep Current Design + Better Naming

The Use() method is misleading—it suggests registration but doesn't actually do anything. Consider:

// Remove Use() entirely
EmaHub fast = quotes.ToEmaHub(10);
EmaHub slow = quotes.ToEmaHub(20);

if (strat.TryGetLatestAligned(fast, slow, out var f, out var s))

This is honest about what's happening but doesn't solve type safety.

Option 2: Runtime Validation

Add hub tracking to detect misuse at runtime (earlier than property access):

public THub Use<THub, TResult>(THub hub) where ...
{
    _trackedHubs.Add(hub); // Store for validation
    return hub;
}

public bool TryGetLatest<THub1, THub2>(...) 
{
    if (!_trackedHubs.Contains(hub1) || !_trackedHubs.Contains(hub2))
        throw new InvalidOperationException("Hub not registered with Use()");
    // ...
}

Option 3: Implement JGronholz's Hierarchical Design

This provides compile-time safety but requires significant implementation:

// Base for single-hub strategies
public class StrategyHub<TIn, THub1> where TIn : IReusable { ... }

// Two-hub strategies
public class StrategyHub<TIn, THub1, THub2> : StrategyHub<TIn, THub1> 
{
    public void Use(THub1 hub1, THub2 hub2) { ... }
    public bool TryGetLatest(out var h1, out var h2) { ... }
}

// And so on for THub3, THub4...

Alignment with Streaming Strategy Needs

What's working well:

• ✅ Timestamp alignment logic is solid
• ✅ TryGetLatest correctly validates count ≥ 2 and timestamp matching
• ✅ Pattern detection (golden cross) works as intended
• ✅ Real-world simulation (GoldenCrossStrategy.cs) demonstrates practical usage
• ✅ Clean integration with existing streaming hub architecture

What could be improved:

• ⚠️ Type safety concerns limit confidence in complex multi-hub strategies
• ⚠️ Use() method provides no value in current form
• ⚠️ Documentation should warn about hub parameter discipline

Recommendation

Given that this is a brand new API (as JGronholz noted), now is the ideal time to get the design right. I recommend:

1. For v1 (minimal changes): Implement Option 2 (runtime validation) to catch mistakes early, and either remove Use() or make it meaningful by storing hubs for validation.

2. For v2 (if type safety is priority): Implement a simplified version of JGronholz's suggestion:

   • StrategyHub<TIn, THub1, THub2> for the common 2-hub case (covers most strategies)
   • Keep current StrategyHub<TIn> as fallback for 3+ hubs or dynamic scenarios
   • This provides safety for 80% of use cases without full complexity

The current implementation works correctly but relies heavily on developer discipline. The question is: Does this library prioritize compile-time safety (more complex API) or simplicity (runtime discipline)?

What's your preference for the direction here?

---

🧠 Learnings used

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T10:07:54.613Z
Learning: Applies to src/**/Stream*.cs : Consult open specs before modifying stateful pipelines in StreamHub indicators for streaming support

---

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-01T06:10:39.665Z
Learning: Applies to src/**/*.cs : Ensure stateless calculations are thread-safe; streaming hubs must isolate instance state (no static mutable fields)

[DaveSkender]

@coderabbitai per re: your recommendations, I don’t love any of these options. All we really need is a nice way to group and orchestrate hub results in a reactive manner, the StrategyHub doesn’t even need to be an actual hub. I do agree that Use is unneeded complexity and type and thread safety should be addressed. From a clean slate, what do you recommend, given our Golden Cross example?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@DaveSkender, great point about starting from a clean slate! Here's what I recommend:

Clean-Slate Design: AlignedPair<THub1, THub2>

Core concept: A lightweight, strongly-typed coordinator that holds references to hubs and provides aligned snapshots.

For the Golden Cross Example:

QuoteHub quotes = new();
EmaHub fast = quotes.ToEmaHub(10);
EmaHub slow = quotes.ToEmaHub(20);

// Create coordinator with strong typing at construction
var emaPair = new AlignedPair<EmaHub, EmaHub>(fast, slow);

// Type-safe access - f and s are strongly typed to their hub result types
if (emaPair.TryGetLatest(out var f, out var s)
    && f.previous.Ema < s.previous.Ema
    && f.current.Ema > s.current.Ema)
{
    // golden cross detected
}

Key Benefits:

✅ Type Safety: Hub types known at compile time - no way to pass wrong hub
✅ Thread Safety: TryGetLatest() captures immutable snapshots atomically
✅ Simple API: Construction + one method
✅ No Complexity: No inheritance, no Use(), no provider dependency
✅ Clear Intent: "Aligned pair" describes exactly what it does

Implementation Sketch:

public sealed class AlignedPair<THub1, THub2>
    where THub1 : IQuoteProvider<IReusableResult>
    where THub2 : IQuoteProvider<IReusableResult>
{
    private readonly THub1 hub1;
    private readonly THub2 hub2;
    private readonly object lockObj = new();

    public AlignedPair(THub1 hub1, THub2 hub2)
    {
        this.hub1 = hub1;
        this.hub2 = hub2;
    }

    public bool TryGetLatest<TResult1, TResult2>(
        out (TResult1 previous, TResult1 current) first,
        out (TResult2 previous, TResult2 current) second)
        where TResult1 : IReusableResult
        where TResult2 : IReusableResult
    {
        lock (lockObj)
        {
            var results1 = hub1.Results;
            var results2 = hub2.Results;

            if (results1.Count < 2 || results2.Count < 2)
            {
                first = default;
                second = default;
                return false;
            }

            var curr1 = (TResult1)results1[^1];
            var prev1 = (TResult1)results1[^2];
            var curr2 = (TResult2)results2[^1];
            var prev2 = (TResult2)results2[^2];

            // Alignment check
            if (curr1.Timestamp != curr2.Timestamp || 
                prev1.Timestamp != prev2.Timestamp)
            {
                first = default;
                second = default;
                return false;
            }

            first = (prev1, curr1);
            second = (prev2, curr2);
            return true;
        }
    }
}

For More Hubs:

// Two hubs (most common)
AlignedPair<EmaHub, EmaHub>(fast, slow)

// Three hubs
AlignedTriple<EmaHub, EmaHub, RsiHub>(fast, slow, rsi)

// Four hubs
AlignedQuad<EmaHub, EmaHub, RsiHub, StochRsiHub>(...)

Why This Works:

1. No inheritance complexity - each arity is independent
2. Compile-time safety - hub types are generic parameters
3. Thread-safe by design - lock in TryGetLatest() protects snapshot
4. Honest naming - "AlignedPair" tells you exactly what it does
5. Not a hub - it's a coordinator, not part of the streaming pipeline
6. Minimal API - constructor + one method per class

Alternative Simpler Option:

If you want even less code, static extension methods:

public static class HubAlignment
{
    public static bool TryGetAligned<THub1, THub2, TResult1, TResult2>(
        this THub1 hub1, THub2 hub2,
        out (TResult1 prev, TResult1 curr) first,
        out (TResult2 prev, TResult2 curr) second)
        where THub1 : IQuoteProvider<IReusableResult>
        where THub2 : IQuoteProvider<IReusableResult>
        where TResult1 : IReusableResult
        where TResult2 : IReusableResult
    { /* same logic */ }
}

// Usage
if (fast.TryGetAligned(slow, out var f, out var s)
    && f.prev.Ema < s.prev.Ema
    && f.curr.Ema > s.curr.Ema)
{
    // golden cross
}

This has no state and no objects, just pure functions. Even simpler, but loses the "grouping" aspect.

---

My recommendation: Go with AlignedPair<THub1, THub2> (and siblings for 3-4 hubs). It's the sweet spot of type safety, thread safety, simplicity, and clear semantics.

What do you think? Want me to draft a full implementation?

---

🧠 Learnings used

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-01T06:10:39.665Z
Learning: Applies to src/**/*.cs : Ensure stateless calculations are thread-safe; streaming hubs must isolate instance state (no static mutable fields)

---

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T10:07:54.613Z
Learning: Applies to src/**/Stream*.cs : Consult open specs before modifying stateful pipelines in StreamHub indicators for streaming support

[DaveSkender]

@copilot replace the StrategyHub implementation approach with the AlignedPairs recommendation in this thread with 2, 3, and 4 hub overloads, but let’s call it a StrategyGroup<T1,T2,..,Tn>(..). Reimplement the golden cross example.

[Copilot]

@copilot replace the StrategyHub implementation approach with the AlignedPairs recommendation in this thread with...

Replaced StrategyHub with aligned StrategyGroup overloads (2–4 hubs) and refreshed the golden-cross example to use aligned pairs; see commit c9a53f7.

[DaveSkender]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

Actionable comments posted: 8

🤖 Fix all issues with AI agents

In @src/_common/Strategy/StrategyHub.cs:
- Around line 16-42: Add XML documentation comments for the public constructor
StrategyGroup(IStreamObservable<T1>, IStreamObservable<T2>), the Hub1 and Hub2
properties, and the TryGetAligned method: include <summary> describing the
purpose of StrategyGroup and what Hub1/Hub2 expose, add <param name="hub1"> and
<param name="hub2"> for the constructor (and <exception
cref="ArgumentNullException"> when null), add <typeparam name="T1"> and
<typeparam name="T2"> describing the series types, and for TryGetAligned add
<summary>, <param name="pair1"> and <param name="pair2"> describing the out
aligned pairs, and a <returns> indicating the boolean alignment result; keep
comments concise and follow the existing XML doc style used in the project.
- Around line 68-72: Add XML documentation comments for the three public
properties Hub1, Hub2 and Hub3 in StrategyHub.cs: provide a concise summary for
each property describing what stream it exposes and its generic element type
(IStreamObservable<T1>, IStreamObservable<T2>, IStreamObservable<T3>), using the
project’s XML-doc style (summary tag and any existing tag conventions) so the
CS1591 warnings are resolved and the build succeeds.
- Around line 130-136: Add XML documentation comments for the four get-only
properties Hub1, Hub2, Hub3, and Hub4 (IStreamObservable<T1>,
IStreamObservable<T2>, IStreamObservable<T3>, IStreamObservable<T4>) to satisfy
CS1591; include a <summary> describing each hub's role in the strategy pipeline
and, if helpful, a <typeparam> reference to the corresponding generic type and a
<value> or <returns> describing what the observable yields. Ensure the comments
sit immediately above the property declarations for Hub1, Hub2, Hub3, and Hub4.

In @tests/public-api/customizable/StrategyHub.Tests.cs:
- Around line 47-108: The GoldenCross test lacks assertions beyond printing
balance; update the test (method GoldenCross) to assert expected outcomes after
computing crossover/crossunder and trade effects: ensure hasPairs is true
(already checked), then assert on trade state by verifying units and balance
reflect expected behavior (e.g., if crossover then units > 0 and balance equals
units * price, if crossunder then units == 0), and/or add a trades counter
incremented in the crossover/crossunder branches and assert trades meets
expected bounds; use the existing symbols TryGetAligned, fastPair, slowPair,
crossover, crossunder, units, balance and price to implement these assertions.

In @tools/server/DataLoader.cs:
- Around line 78-89: In QuoteFromCsv, add bounds checking before indexing into
values to avoid IndexOutOfRangeException: verify values.Length >= 6 and if not
either throw an informative exception (e.g., ArgumentException or
FormatException) that includes the offending csvLine and actual length or return
a handled result (null/optional) per project convention; additionally consider
replacing direct Parse calls with TryParse (DateTime.TryParse/decimal.TryParse
using EnglishCulture) to fail gracefully and provide clearer error messages;
update callers of QuoteFromCsv accordingly to handle the new failure mode.

In @tools/simulate/GoldenCrossStrategy.cs:
- Around line 13-16: Replace the removed StrategyHub API with the new
StrategyGroup API: change the field declaration from StrategyHub<IQuote>
_strategyHub to StrategyGroup<EmaResult, EmaResult> (rename to _strategyGroup if
helpful), update the constructor where StrategyHub.Use<T>() is called to instead
register the two EMA strategies via the StrategyGroup registration method (e.g.,
Add or equivalent on StrategyGroup) targeting EmaResult outputs, and update
ProcessQuote where TryGetLatest() is called to the StrategyGroup equivalent that
returns the latest EmaResult pair (use the StrategyGroup method that yields the
latest results for the two outputs). Ensure all references to _strategyHub and
StrategyHub methods are replaced with the new _strategyGroup and StrategyGroup
methods that accept/return EmaResult types.

In @tools/simulate/Program.cs:
- Around line 14-24: ServerManager.StartServer may return null but the code
proceeds regardless; check the returned serverProcess after calling
ServerManager.StartServer(5001) and handle the null case immediately (e.g., log
an error via the existing logger, throw or call Environment.Exit/Process.Kill to
fail fast) so you don't continue to await Task.Delay and run GoldenCrossStrategy
when the SSE server failed to start; update the startup sequence around the
serverProcess variable to validate non-null and abort with a clear error message
if startup failed.

In @tools/simulate/ServerManager.cs:
- Around line 19-32: The hardcoded path for serverExePath assumes "Debug" and
"net10.0" which breaks for Release or different TFMs; update the logic in
ServerManager to search for the built executable instead of a single hardcoded
path: either probe both "Debug" and "Release" and multiple known TFMs or,
better, use Directory.EnumerateFiles/EnumerateDirectories under
Path.Combine(serverProjectPath, "bin") to find "Test.SseServer.exe" (or any
*.exe matching the project name) and set serverExePath to the discovered file;
if nothing is found, keep falling back to
StartServerWithDotNetRun(serverProjectPath, port). Ensure the code references
serverExePath and StartServerWithDotNetRun so the existing fallback remains
intact.

🧹 Nitpick comments (5)

tools/server/DataLoader.cs (1)

42-68: Consider renaming seed parameter to basePrice for clarity.

The seed parameter represents the base price for geometric Brownian motion simulation, not an RNG seed (as the Random is initialized with a GUID hash on each call). This naming could be confusing.

♻️ Optional rename for clarity

-internal static Quote GenerateRandomQuote(DateTime timestamp, double seed)
+internal static Quote GenerateRandomQuote(DateTime timestamp, double basePrice)
 {
     Random random = new(Guid.NewGuid().GetHashCode());
     const double volatility = 0.01;
     const double drift = 0.001 * 0.01;

-    double open = Price(seed, volatility * volatility, drift, random);
+    double open = Price(basePrice, volatility * volatility, drift, random);
     double close = Price(open, volatility, drift, random);

     double ocMax = Math.Max(open, close);
-    double high = Price(seed, volatility * 0.5, 0, random);
+    double high = Price(basePrice, volatility * 0.5, 0, random);
     high = high < ocMax ? (2 * ocMax) - high : high;

     double ocMin = Math.Min(open, close);
-    double low = Price(seed, volatility * 0.5, 0, random);
+    double low = Price(basePrice, volatility * 0.5, 0, random);
     low = low > ocMin ? (2 * ocMin) - low : low;

-    double volume = Price(seed * 1000, volatility * 2, drift: 0, random);
+    double volume = Price(basePrice * 1000, volatility * 2, drift: 0, random);

tools/server/Program.cs (1)

83-83: Consider caching the longest quotes dataset.

DataLoader.GetLongest() is called on every request to /quotes/longest, which reads and parses the CSV file each time. Since this is static test data, consider loading it once at startup.

♻️ Suggested caching approach

+// Load static test data once at startup
+IReadOnlyList<Quote> longestQuotesCache = DataLoader.GetLongest();
+
 // SSE endpoint: Longest quotes (deterministic)
 app.MapGet(
     "/quotes/longest",
     async (HttpContext context, int interval = 100, int? batchSize = null) => {
         context.Response.ContentType = "text/event-stream";
         context.Response.Headers.CacheControl = "no-cache";

-        IReadOnlyList<Quote> longestQuotes = DataLoader.GetLongest();
+        IReadOnlyList<Quote> longestQuotes = longestQuotesCache;
         int totalQuotes = batchSize ?? longestQuotes.Count;

tools/simulate/ServerManager.cs (1)

118-137: Consider logging if process doesn't exit within timeout.

WaitForExit(3000) returns false if the timeout elapses before the process exits, but this is not checked or logged.

🔧 Optional improvement

 process.Kill(entireProcessTree: true);
-process.WaitForExit(3000);
-Console.WriteLine("[ServerManager] SSE server stopped");
+if (!process.WaitForExit(3000))
+{
+    Console.WriteLine("[ServerManager] SSE server did not exit within timeout");
+}
+else
+{
+    Console.WriteLine("[ServerManager] SSE server stopped");
+}

tools/simulate/GoldenCrossStrategy.cs (1)

22-22: Extract magic number for initial balance.

The initial balance 10000.0 is duplicated in multiple places (lines 22, 207, 212, 214). Consider extracting it as a constant for consistency.

♻️ Proposed refactor

 private const int FastPeriod = 50;
 private const int SlowPeriod = 200;
+private const double InitialBalance = 10000.0;

-private double _balance = 10000.0;
+private double _balance = InitialBalance;

 // In PrintSummary:
-double totalPnL = finalValue - 10000.0;
+double totalPnL = finalValue - InitialBalance;

-Console.WriteLine($"Initial Balance:   ${10000.0:N2}");
+Console.WriteLine($"Initial Balance:   ${InitialBalance:N2}");

-Console.WriteLine($"Total P&L:         ${totalPnL:N2} ({totalPnL / 10000.0:P2})");
+Console.WriteLine($"Total P&L:         ${totalPnL:N2} ({totalPnL / InitialBalance:P2})");

Also applies to: 207-214

tools/simulate/Program.cs (1)

1-5: Potentially unused imports.

System.Text.Json (line 3) does not appear to be used in this file. Consider removing if unused.

 using System.Diagnostics;
 using System.Globalization;
-using System.Text.Json;
 using Skender.Stock.Indicators;
 using Test.Simulation;

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 304802c and c9a53f7.

⛔ Files ignored due to path filters (1)

• tests/public-api/customizable/StrategyHub.Calc.xlsx is excluded by !**/*.xlsx and included by **

📒 Files selected for processing (14)

• Stock.Indicators.sln
• src/_common/Strategy/StrategyHub.cs
• tests/public-api/customizable/StrategyHub.Tests.cs
• tools/server/DataLoader.cs
• tools/server/Program.cs
• tools/server/Test.SseServer.csproj
• tools/server/packages.lock.json
• tools/simulate/GlobalSuppressions.cs
• tools/simulate/GoldenCrossStrategy.cs
• tools/simulate/Program.cs
• tools/simulate/ServerManager.cs
• tools/simulate/Simulator.cs
• tools/simulate/Test.Simulation.csproj
• tools/simulate/Utilities.cs

💤 Files with no reviewable changes (4)

• tools/simulate/GlobalSuppressions.cs
• tools/simulate/Test.Simulation.csproj
• tools/simulate/Utilities.cs
• tools/simulate/Simulator.cs

🧰 Additional context used

📓 Path-based instructions (4)

**/*.{cs,md}

📄 CodeRabbit inference engine (AGENTS.md)

Treat all warnings and errors as failures that must be fixed; do not suppress warnings or ignore them

Files:

• tools/simulate/ServerManager.cs
• tests/public-api/customizable/StrategyHub.Tests.cs
• tools/server/Program.cs
• tools/simulate/Program.cs
• tools/simulate/GoldenCrossStrategy.cs
• tools/server/DataLoader.cs
• src/_common/Strategy/StrategyHub.cs

tests/**/*.cs

📄 CodeRabbit inference engine (tests/AGENTS.md)

tests/**/*.cs: Write tests for all public methods
Cover edge cases: empty input, minimum/maximum values, boundary conditions in tests
Use descriptive test names that explain the scenario
Keep tests focused on single behaviors

Files:

• tests/public-api/customizable/StrategyHub.Tests.cs

---

⚙️ CodeRabbit configuration file

tests/**/*.cs: Review for:

• Test coverage: Ensure edge cases and boundary conditions are tested
• Mathematical validation: Verify tests use manually calculated results
• Performance tests: Check benchmark tests for regression detection
• Integration tests: Validate API compatibility and chaining scenarios
• Streaming tests: Ensure real-time scenarios are properly tested
• Error scenarios: Verify exception handling and invalid input tests

Files:

• tests/public-api/customizable/StrategyHub.Tests.cs

**/*.{csproj,props,targets,sln}

⚙️ CodeRabbit configuration file

**/*.{csproj,props,targets,sln}: Verify:

• Version consistency: Package versions, target frameworks alignment
• Security: No vulnerable packages, proper security settings
• Performance: Compiler optimizations enabled for release builds
• Analyzers: Code quality rules properly configured
• Packaging: NuGet metadata complete and accurate

Files:

• tools/server/Test.SseServer.csproj
• Stock.Indicators.sln

src/**/*.cs

📄 CodeRabbit inference engine (src/AGENTS.md)

src/**/*.cs: Implement single-pass O(n) algorithms unless mathematically impossible (justify exceptions in PR)
Provide deterministic WarmupPeriod helper or documented rule for each indicator
Use double for speed; escalate to decimal only when rounding affects financial correctness (>0.5 tick at 4-decimal pricing)
Allocate only result list and minimal working buffers; avoid temporary per-step Lists or LINQ in loops
Ensure stateless calculations are thread-safe; streaming hubs must isolate instance state (no static mutable fields)
Use ArgumentOutOfRangeException for invalid numeric parameter ranges
Use ArgumentException for semantic misuse (e.g., insufficient history)
Never swallow exceptions; wrap only to add context with parameter name and offending value when relevant
Use non-nullable double types internally with intentional NaN propagation; convert NaN to null only at the final result boundary via .NaN2Null()
Guard variable denominators with ternary checks (e.g., denom != 0 ? num / denom : double.NaN) to prevent division by zero
Never use epsilon values (e.g., 1e-8, 1e-9) for zero checks in division guards; use exact zero comparison (!= 0 or == 0)
Accept NaN inputs and allow natural propagation through calculations; never reject or filter NaN values
Only validate quotes for null/missing quotes, not for NaN values in quote properties (High/Low/Close/etc.)
Use double.NaN to represent uninitialized state instead of sentinel values like 0 or -1
Avoid off-by-one errors when calculating lookback or warmup periods
Favor double for performance, switching to decimal only when business accuracy demands it; avoid chained calculations that cause precision loss
Avoid performance regressions from unnecessary allocations or LINQ; prefer span-friendly loops and avoid boxing
All public methods must have XML documentation
Validate all user inputs in indicator implementations
Use consistent formatting following .editorconfig

Use Roslynator for ...

Files:

• src/_common/Strategy/StrategyHub.cs

---

⚙️ CodeRabbit configuration file

src/**/*.cs: Focus on:

• Mathematical accuracy: Verify financial calculations against reference implementations
• Performance: Flag unnecessary allocations, excessive LINQ, boxing operations
• Precision: Ensure appropriate use of double vs decimal for financial calculations
• Null safety: Validate handling of insufficient data scenarios
• Input validation: Check parameter validation and error handling
• Lookback periods: Verify off-by-one errors in window calculations
• Streaming support: Ensure stateful indicators handle edge cases properly
• Memory efficiency: Check for proper span usage and buffer management
• XML documentation: Ensure all public APIs have complete documentation

Files:

• src/_common/Strategy/StrategyHub.cs

🧠 Learnings (17)

📚 Learning: 2026-01-10T10:07:54.613Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T10:07:54.613Z
Learning: Applies to src/**/*.csproj : Multi-target .NET framework versions net10.0, net9.0, and net8.0 with analyzers enabled for strict code quality

Applied to files:

• tools/server/packages.lock.json
• tools/server/Test.SseServer.csproj

📚 Learning: 2026-01-10T10:07:54.613Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-01-10T10:07:54.613Z
Learning: Applies to src/**/*.cs : Use Roslynator for .NET linting with `roslynator fix --properties TargetFramework=net10.0 --severity-level info` for development and `dotnet format` before committing

Applied to files:

• tools/server/packages.lock.json
• tools/server/Test.SseServer.csproj

📚 Learning: 2025-09-02T02:01:48.007Z

Learnt from: DaveSkender
Repo: DaveSkender/Stock.Indicators PR: 1384
File: tests/indicators/s-z/Tsi/Tsi.Tests.cs:14-14
Timestamp: 2025-09-02T02:01:48.007Z
Learning: The Stock.Indicators repository uses centralized package management via tests\Directory.Packages.props to define MSTest package versions across all test projects, currently using MSTest.TestAdapter and MSTest.TestFramework version 3.10.3.

Applied to files:

• tools/server/packages.lock.json
• tools/server/Test.SseServer.csproj
• Stock.Indicators.sln

📚 Learning: 2026-01-01T06:10:39.665Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-01T06:10:39.665Z
Learning: Applies to src/**/*.csproj : Targets must include net10.0, net9.0, and net8.0 (all must build and pass tests)

Applied to files:

• tools/server/packages.lock.json
• tools/server/Test.SseServer.csproj
• Stock.Indicators.sln

📚 Learning: 2026-01-01T06:10:39.665Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-01T06:10:39.665Z
Learning: Applies to src/tests/**/*.Tests.cs : Provide comprehensive unit test coverage for all code paths in indicators

Applied to files:

• tests/public-api/customizable/StrategyHub.Tests.cs
• tools/server/Test.SseServer.csproj
• Stock.Indicators.sln

📚 Learning: 2026-01-01T06:10:39.665Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-01T06:10:39.665Z
Learning: Applies to src/tests/**/*.Regression.Tests.cs : Add regression tests to `tests/indicators/**/{IndicatorName}.Regression.Tests.cs`

Applied to files:

• tests/public-api/customizable/StrategyHub.Tests.cs
• tools/server/Test.SseServer.csproj
• Stock.Indicators.sln

📚 Learning: 2025-11-05T07:07:02.485Z

Learnt from: DaveSkender
Repo: DaveSkender/Stock.Indicators PR: 1723
File: tests/public-api/Convergence.BufferList.Tests.cs:13-102
Timestamp: 2025-11-05T07:07:02.485Z
Learning: Convergence tests in tests/public-api/Convergence.*.Tests.cs are designed to identify the minimum historical data periods required for indicators to stabilize to a certain precision threshold, not to perform exact equality validation against static series (which is done in regression tests). Future enhancements should validate that indicators converge after an expected number of periods within a defined precision tolerance.

Applied to files:

• tests/public-api/customizable/StrategyHub.Tests.cs
• tools/server/DataLoader.cs

📚 Learning: 2025-11-05T07:06:56.179Z

Learnt from: DaveSkender
Repo: DaveSkender/Stock.Indicators PR: 1723
File: tests/public-api/Convergence.StreamHub.Tests.cs:13-130
Timestamp: 2025-11-05T07:06:56.179Z
Learning: Applies to tests/public-api/Convergence.StreamHub.Tests.cs and tests/public-api/Convergence.BufferList.Tests.cs: Convergence tests should track the expected number of periods required for convergence to a certain precision threshold and should align with the same expected values used in Series style equivalent tests. Currently used for exploratory identification of convergence points but should be formalized with assertions.

Applied to files:

• tests/public-api/customizable/StrategyHub.Tests.cs

📚 Learning: 2025-10-21T03:24:07.782Z

Learnt from: DaveSkender
Repo: DaveSkender/Stock.Indicators PR: 1581
File: tests/indicators/e-k/ForceIndex/ForceIndex.Regression.Tests.cs:15-15
Timestamp: 2025-10-21T03:24:07.782Z
Learning: In regression tests (tests/indicators/**/*.Regression.Tests.cs), using default parameters (e.g., not specifying lookback periods) is intentional to validate that defaults remain consistent across Series, Buffer, and Stream implementations. Test failures due to default changes are desired behavior.

Applied to files:

• tests/public-api/customizable/StrategyHub.Tests.cs
• tools/server/DataLoader.cs

📚 Learning: 2025-12-23T09:47:28.113Z

Learnt from: DaveSkender
Repo: DaveSkender/Stock.Indicators PR: 1745
File: tests/indicators/s-z/StochRsi/StochRsi.BufferList.Tests.cs:51-57
Timestamp: 2025-12-23T09:47:28.113Z
Learning: For bounded indicators (e.g., Stoch, StochRsi, Williams %R), prefer algebraic stability with explicit boundary checks instead of post-calculation clamping or ToPrecision rounding. Implement checks such as if value >= high then set to 100, if value <= low then set to 0. This prevents floating-point precision errors at boundaries and ensures exact 0/100 values. Apply this guidance to test and implementation files under tests/ indicators where these calculations are validated to ensure consistency across edge cases.

Applied to files:

• tests/public-api/customizable/StrategyHub.Tests.cs

📚 Learning: 2026-01-01T06:10:39.665Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-01T06:10:39.665Z
Learning: Applies to src/tools/performance/**/*.cs : Include performance tests for computationally intensive indicators

Applied to files:

• tools/server/Test.SseServer.csproj
• tools/server/DataLoader.cs

📚 Learning: 2025-12-31T18:52:28.248Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: tests/AGENTS.md:0-0
Timestamp: 2025-12-31T18:52:28.248Z
Learning: Run unit tests with `dotnet test tests/indicators/ --settings tests/tests.unit.runsettings` to exclude Integration and Regression tests

Applied to files:

• tools/server/Test.SseServer.csproj

📚 Learning: 2025-12-31T18:52:28.248Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: tests/AGENTS.md:0-0
Timestamp: 2025-12-31T18:52:28.248Z
Learning: Run regression tests with `dotnet test tests/indicators/ --settings tests/tests.regression.runsettings` before releases for baseline validation

Applied to files:

• tools/server/Test.SseServer.csproj

📚 Learning: 2025-11-12T00:30:31.400Z

Learnt from: DaveSkender
Repo: DaveSkender/Stock.Indicators PR: 1734
File: .github/workflows/test-indicators-matrix.yml:38-51
Timestamp: 2025-11-12T00:30:31.400Z
Learning: In .github/workflows/test-indicators-matrix.yml, the workflow always requires .NET 10 to build and run test projects. The "legacy sut" .NET SDK installation (e.g., 6.0.x) is only for multi-targeting support, and the TARGET_FRAMEWORK environment variable controls which framework version tests actually run against using the `--property:TestFramework` parameter.

Applied to files:

• tools/server/Test.SseServer.csproj

📚 Learning: 2026-01-01T06:10:39.665Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: src/AGENTS.md:0-0
Timestamp: 2026-01-01T06:10:39.665Z
Learning: Applies to src/**/*.cs : Only validate quotes for null/missing quotes, not for NaN values in quote properties (High/Low/Close/etc.)

Applied to files:

• tools/server/DataLoader.cs

📚 Learning: 2025-10-01T07:32:55.702Z

Learnt from: CR
Repo: DaveSkender/Stock.Indicators PR: 0
File: .github/instructions/performance-testing.instructions.md:0-0
Timestamp: 2025-10-01T07:32:55.702Z
Learning: Optimize hot paths: minimize allocations, avoid excessive LINQ, use Span<T> where appropriate, cache calculations, and test with realistic data sizes (502 periods)

Applied to files:

• tools/server/DataLoader.cs

📚 Learning: 2025-10-03T17:49:24.747Z

Learnt from: DaveSkender
Repo: DaveSkender/Stock.Indicators PR: 1471
File: tests/indicators/s-z/Tsi/Tsi.StreamHub.Tests.cs:84-247
Timestamp: 2025-10-03T17:49:24.747Z
Learning: Static series implementations are the canonical reference for indicators, aligned to original reputable author formulas linked in documentation. Buffer lists and stream hubs must match static series exactly.

Applied to files:

• src/_common/Strategy/StrategyHub.cs

🧬 Code graph analysis (3)

tests/public-api/customizable/StrategyHub.Tests.cs (4)

tests/indicators/_base/TestBase.cs (1)

• TestBase (9-30)

src/_common/Strategy/StrategyHub.cs (9)

• StrategyGroup (16-42)
• StrategyGroup (20-24)
• StrategyGroup (47-100)
• StrategyGroup (58-66)
• StrategyGroup (105-169)
• StrategyGroup (118-128)
• TryGetAligned (30-41)
• TryGetAligned (81-99)
• TryGetAligned (146-168)

src/e-k/Ema/Ema.StreamHub.cs (1)

• EmaResult (27-52)

src/s-z/Sma/Sma.StreamHub.cs (1)

• SmaResult (24-42)

tools/server/Program.cs (2)

src/_common/Math/Numerical.cs (1)

• DateTime (105-110)

tools/server/DataLoader.cs (4)

• Quote (42-68)
• Quote (78-89)
• DataLoader (8-90)
• IReadOnlyList (12-40)

tools/simulate/GoldenCrossStrategy.cs (3)

src/_common/Quotes/Quote.StreamHub.cs (1)

• IQuote (50-59)

src/e-k/Ema/Ema.StreamHub.cs (1)

• EmaResult (27-52)

tools/server/DataLoader.cs (2)

• Quote (42-68)
• Quote (78-89)

🪛 GitHub Actions: Test Indicators

src/_common/Strategy/StrategyHub.cs

[warning] 118-118: IDE0290: Use primary constructor

---

[warning] 58-58: IDE0290: Use primary constructor

---

[warning] 20-20: IDE0290: Use primary constructor

---

[warning] 20-20: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2>.StrategyGroup(IStreamObservable, IStreamObservable)'

---

[warning] 26-26: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2>.Hub1'

---

[warning] 28-28: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2>.Hub2'

---

[warning] 30-30: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2>.TryGetAligned(out AlignedPair, out AlignedPair)'

---

[warning] 68-68: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2, T3>.Hub1'

---

[warning] 70-70: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2, T3>.Hub2'

---

[warning] 72-72: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2, T3>.Hub3'

---

[warning] 130-130: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2, T3, T4>.Hub1'

---

[warning] 132-132: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2, T3, T4>.Hub2'

---

[warning] 134-134: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2, T3, T4>.Hub3'

---

[warning] 136-136: CS1591: Missing XML comment for publicly visible type or member 'StrategyGroup<T1, T2, T3, T4>.Hub4'

🪛 GitHub Actions: Test Indicators (matrix)

tools/simulate/GoldenCrossStrategy.cs

[error] 14-14: dotnet build failed: The type or namespace name 'StrategyHub<>' could not be found (are you missing a using directive or an assembly reference?).

🪛 GitHub Check: matrix validation (macos-latest, 10.0.x)

tools/simulate/GoldenCrossStrategy.cs

[failure] 14-14:
The type or namespace name 'StrategyHub<>' could not be found (are you missing a using directive or an assembly reference?)

---

[failure] 14-14:
The type or namespace name 'StrategyHub<>' could not be found (are you missing a using directive or an assembly reference?)

🪛 GitHub Check: matrix validation (macos-latest, 8.0.x)

tools/simulate/GoldenCrossStrategy.cs

[failure] 14-14:
The type or namespace name 'StrategyHub<>' could not be found (are you missing a using directive or an assembly reference?)

---

[failure] 14-14:
The type or namespace name 'StrategyHub<>' could not be found (are you missing a using directive or an assembly reference?)

🪛 GitHub Check: matrix validation (ubuntu-latest, 10.0.x)

tools/simulate/GoldenCrossStrategy.cs

[failure] 14-14:
The type or namespace name 'StrategyHub<>' could not be found (are you missing a using directive or an assembly reference?)

---

[failure] 14-14:
The type or namespace name 'StrategyHub<>' could not be found (are you missing a using directive or an assembly reference?)

🪛 GitHub Check: matrix validation (ubuntu-latest, 8.0.x)

tools/simulate/GoldenCrossStrategy.cs

[failure] 14-14:
The type or namespace name 'StrategyHub<>' could not be found (are you missing a using directive or an assembly reference?)

---

[failure] 14-14:
The type or namespace name 'StrategyHub<>' could not be found (are you missing a using directive or an assembly reference?)

🔇 Additional comments (13)

tools/server/packages.lock.json (1)

1-16: LGTM!

The package lock file is correctly structured for version 2 format with proper dependency tracking. The single net10.0 target is appropriate for a tools/server project (as opposed to the main library which multi-targets). The Roslynator.Analyzers version constraint and content hash support reproducible builds.

Stock.Indicators.sln (1)

42-43: LGTM!

The new Test.SseServer project is correctly added to the solution with:

• Proper SDK-style project GUID format
• Complete configuration mappings for all Debug/Release and platform combinations
• Correct nesting under the Tools solution folder, consistent with other tool projects

Also applies to: 142-153, 166-166

tools/server/DataLoader.cs (1)

12-40: LGTM with a note on path fragility.

The multi-path search strategy is reasonable for test data discovery. The hardcoded net10.0 in line 16 may need updating when target frameworks change, but this is acceptable for internal test tooling.

tests/public-api/customizable/StrategyHub.Tests.cs (2)

67-70: Test only validates final quote pair, not historical crossover events.

The loop feeds all LongestQuotes but TryGetAligned is only called once after the loop completes (lines 72-74). This validates the alignment API works but doesn't test detecting multiple crossover events throughout the historical data. If comprehensive golden cross detection is intended, consider iterating alignment checks per quote.

If this is intentionally a simple integration test for the StrategyGroup alignment API (not a full strategy backtest), then the current design is acceptable. Please confirm the intended scope.

---

8-45: LGTM!

Well-structured integration test that:

• Creates fast/slow EMA hubs with distinct lookback periods
• Validates TryGetAligned returns correct previous/current pairs
• Confirms strategy-based cross detection matches manual calculation

Good coverage of the core StrategyGroup alignment functionality.

tools/server/Program.cs (2)

17-74: LGTM!

The /quotes/random endpoint correctly implements SSE streaming with:

• Proper content-type and cache-control headers
• Cancellation token propagation through context.RequestAborted
• Appropriate exception handling for OperationCanceledException and IOException
• ConfigureAwait(false) for async operations
• Progress logging and batch termination support

---

90-127: LGTM!

The streaming loop correctly:

• Bounds-checks both totalQuotes and longestQuotes.Count
• Handles client disconnection gracefully
• Maintains consistent SSE event format with the random endpoint

src/_common/Strategy/StrategyHub.cs (2)

30-41: Inconsistent out-parameter handling when returning false.

When TryGetLatestPair fails (lines 32-37), pairs are reset to default. However, when timestamps don't match (line 40), the method returns false but retains the populated pair values. This inconsistency could confuse callers who might assume out-params are invalid/default when the method returns false.

Consider resetting pairs before returning false for timestamp mismatch, or document that pairs may contain valid (but unaligned) data when returning false.

     return pair1.Current.Timestamp == pair2.Current.Timestamp;
+    // If timestamps don't match, consider:
+    // bool aligned = pair1.Current.Timestamp == pair2.Current.Timestamp;
+    // if (!aligned) { pair1 = default; pair2 = default; }
+    // return aligned;

---

171-188: Utility implementation looks correct.

The TryGetLatestPair helper correctly handles the minimum results check and uses efficient index-from-end syntax.

tools/simulate/ServerManager.cs (1)

96-116: Repository root detection is well-implemented.

The upward traversal with null-checking for the parent directory handles edge cases correctly.

tools/simulate/Program.cs (1)

7-12: Argument parsing looks reasonable.

The defaults are sensible. Minor consideration: no validation that interval and count are positive, but this is acceptable for a tool project.

tools/server/Test.SseServer.csproj (1)

1-34: Verify Roslynator.Analyzers version management configuration.

The project correctly enables analyzers, nullable reference types, and code style enforcement. However, the Roslynator.Analyzers PackageReference on line 20 has no explicit version specified. Confirm whether the package version is managed via central package management (e.g., Directory.Packages.props) or if an explicit version should be pinned here.

tools/simulate/GoldenCrossStrategy.cs (1)

110-113: The Dispose method is correctly implemented.

_quoteHub, _fastEma, and _slowEma do not implement IDisposable. These types extend StreamHub<TIn, TOut> through their respective base classes (QuoteProvider and ChainHub), which do not implement IDisposable. Only _httpClient requires disposal, which is already handled.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test lacks meaningful assertion on expected outcome.

The GoldenCross test only prints the balance without asserting any expected result. This means the test will always pass regardless of the actual behavior, defeating the purpose of regression detection. Per coding guidelines, tests should validate expected outcomes.

💡 Suggested improvement

Consider adding an assertion to validate expected behavior. At minimum, verify that alignment succeeded and optionally assert on expected balance range or trade count:

     Console.WriteLine("Balance is {0:N6}", balance);
+
+    // Validate that the test completed successfully
+    balance.Should().BeGreaterThan(0, "Balance should remain positive");
 }

Or track and assert on trade execution:

int trades = 0;
// ... in crossover/crossunder blocks, increment trades
trades.Should().BeGreaterThanOrEqualTo(0, "Should detect crossover events over LongestQuotes");

🤖 Prompt for AI Agents

In @tests/public-api/customizable/StrategyHub.Tests.cs around lines 47 - 108,
The GoldenCross test lacks assertions beyond printing balance; update the test
(method GoldenCross) to assert expected outcomes after computing
crossover/crossunder and trade effects: ensure hasPairs is true (already
checked), then assert on trade state by verifying units and balance reflect
expected behavior (e.g., if crossover then units > 0 and balance equals units *
price, if crossunder then units == 0), and/or add a trades counter incremented
in the crossover/crossunder branches and assert trades meets expected bounds;
use the existing symbols TryGetAligned, fastPair, slowPair, crossover,
crossunder, units, balance and price to implement these assertions.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Add bounds checking for CSV parsing to prevent runtime exceptions.

If the CSV line contains fewer than 6 comma-separated values, accessing values[5] will throw an IndexOutOfRangeException. Consider adding validation or using a pattern that fails gracefully.

🔧 Suggested fix with validation

 private static Quote QuoteFromCsv(string csvLine)
 {
     string[] values = csvLine.Split(',');
+
+    if (values.Length < 6)
+    {
+        throw new FormatException($"Invalid CSV line: expected 6 fields, got {values.Length}");
+    }

     return new Quote(
         Timestamp: DateTime.Parse(values[0], EnglishCulture),
         Open: decimal.Parse(values[1], EnglishCulture),
         High: decimal.Parse(values[2], EnglishCulture),
         Low: decimal.Parse(values[3], EnglishCulture),
         Close: decimal.Parse(values[4], EnglishCulture),
         Volume: decimal.Parse(values[5], EnglishCulture));
 }

📝 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

	private static Quote QuoteFromCsv(string csvLine)
	{
	string[] values = csvLine.Split(',');
	return new Quote(
	Timestamp: DateTime.Parse(values[0], EnglishCulture),
	Open: decimal.Parse(values[1], EnglishCulture),
	High: decimal.Parse(values[2], EnglishCulture),
	Low: decimal.Parse(values[3], EnglishCulture),
	Close: decimal.Parse(values[4], EnglishCulture),
	Volume: decimal.Parse(values[5], EnglishCulture));
	}
	private static Quote QuoteFromCsv(string csvLine)
	{
	string[] values = csvLine.Split(',');
	if (values.Length < 6)
	{
	throw new FormatException($"Invalid CSV line: expected 6 fields, got {values.Length}");
	}
	return new Quote(
	Timestamp: DateTime.Parse(values[0], EnglishCulture),
	Open: decimal.Parse(values[1], EnglishCulture),
	High: decimal.Parse(values[2], EnglishCulture),
	Low: decimal.Parse(values[3], EnglishCulture),
	Close: decimal.Parse(values[4], EnglishCulture),
	Volume: decimal.Parse(values[5], EnglishCulture));
	}

🤖 Prompt for AI Agents

In @tools/server/DataLoader.cs around lines 78 - 89, In QuoteFromCsv, add bounds
checking before indexing into values to avoid IndexOutOfRangeException: verify
values.Length >= 6 and if not either throw an informative exception (e.g.,
ArgumentException or FormatException) that includes the offending csvLine and
actual length or return a handled result (null/optional) per project convention;
additionally consider replacing direct Parse calls with TryParse
(DateTime.TryParse/decimal.TryParse using EnglishCulture) to fail gracefully and
provide clearer error messages; update callers of QuoteFromCsv accordingly to
handle the new failure mode.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

No validation that server started successfully.

If ServerManager.StartServer returns null, the code continues to wait and run the strategy, which will fail with an HTTP connection error. Consider checking the return value and failing early.

🔧 Proposed fix

 // Start SSE server
 Process? serverProcess = ServerManager.StartServer(5001);
+if (serverProcess is null)
+{
+    Console.WriteLine("Failed to start SSE server. Exiting.");
+    return;
+}

 try
 {
     // Wait for server to start
     await Task.Delay(2000).ConfigureAwait(false);

📝 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

	// Start SSE server
	Process? serverProcess = ServerManager.StartServer(5001);
	// initialize quote provider
	QuoteHub quoteHub = new();
	// subscribe indicator hubs (SMA, EMA, etc.)
	SmaHub smaHub = quoteHub.ToSmaHub(3);
	EmaHub emaHub = quoteHub.ToEmaHub(5);
	EmaHub useChain = quoteHub.ToQuotePartHub(CandlePart.HL2).ToEmaHub(7);
	EmaHub emaChain = quoteHub.ToSmaHub(4).ToEmaHub(4); // chainable
	/* normally, you'd plugin your WebSocket here
	* and use `quoteHub.Add(q);` to connect the streams */
	// simulate streaming quotes
	for (int i = 0; i < quantityToStream; i++)
	try
	{
	Quote quote = quotes[i];
	quoteHub.Add(quote); // on arrival from external WebSocket
	// govern simulation rate
	Thread.Sleep(60000 / quotesPerMinute);
	// Wait for server to start
	await Task.Delay(2000).ConfigureAwait(false);
	// send output to console
	Util.PrintData(quote, smaHub, emaHub, useChain, emaChain);
	// Run Golden Cross strategy
	using GoldenCrossStrategy strategy = new(endpoint, interval, count);
	await strategy.RunAsync().ConfigureAwait(false);
	// Start SSE server
	Process? serverProcess = ServerManager.StartServer(5001);
	if (serverProcess is null)
	{
	Console.WriteLine("Failed to start SSE server. Exiting.");
	return;
	}
	try
	{
	// Wait for server to start
	await Task.Delay(2000).ConfigureAwait(false);
	// Run Golden Cross strategy
	using GoldenCrossStrategy strategy = new(endpoint, interval, count);
	await strategy.RunAsync().ConfigureAwait(false);

🤖 Prompt for AI Agents

In @tools/simulate/Program.cs around lines 14 - 24, ServerManager.StartServer
may return null but the code proceeds regardless; check the returned
serverProcess after calling ServerManager.StartServer(5001) and handle the null
case immediately (e.g., log an error via the existing logger, throw or call
Environment.Exit/Process.Kill to fail fast) so you don't continue to await
Task.Delay and run GoldenCrossStrategy when the SSE server failed to start;
update the startup sequence around the serverProcess variable to validate
non-null and abort with a clear error message if startup failed.

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Hardcoded Debug configuration and framework version may cause runtime failures.

The path construction assumes Debug configuration and net10.0 framework:

"bin", "Debug", "net10.0", "Test.SseServer.exe"

This will fail when running from Release builds or if the target framework changes. Consider using runtime detection or always falling through to dotnet run for robustness.

🔧 Suggested improvement

-string serverExePath = Path.Combine(
-    serverProjectPath,
-    "bin",
-    "Debug",
-    "net10.0",
-    "Test.SseServer.exe");
-
-// If exe doesn't exist, try running with dotnet run
-if (!File.Exists(serverExePath))
-{
-    Console.WriteLine($"[ServerManager] Server executable not found at {serverExePath}. Attempting dotnet run...");
-    return StartServerWithDotNetRun(serverProjectPath, port);
-}
+// Use dotnet run for reliability across configurations
+return StartServerWithDotNetRun(serverProjectPath, port);

Alternatively, probe for both Debug and Release configurations.

🤖 Prompt for AI Agents

In @tools/simulate/ServerManager.cs around lines 19 - 32, The hardcoded path for
serverExePath assumes "Debug" and "net10.0" which breaks for Release or
different TFMs; update the logic in ServerManager to search for the built
executable instead of a single hardcoded path: either probe both "Debug" and
"Release" and multiple known TFMs or, better, use
Directory.EnumerateFiles/EnumerateDirectories under
Path.Combine(serverProjectPath, "bin") to find "Test.SseServer.exe" (or any
*.exe matching the project name) and set serverExePath to the discovered file;
if nothing is found, keep falling back to
StartServerWithDotNetRun(serverProjectPath, port). Ensure the code references
serverExePath and StartServerWithDotNetRun so the existing fallback remains
intact.

[DaveSkender]

@copilot after addressing these, run full code completion checklist.

[Copilot]

@copilot after addressing these, run full code completion checklist.

Ran full code completion checklist after the fixes: dotnet format, build, dotnet test, and markdownlint (commit d8a5e95).

[DaveSkender]

@coderabbitai re-review.

1. Cleanup all open prior reviews, feedback, and comments (not just the last one)
   • Close items if they are now resolved or obsolete
   • Refresh if outdated but still relevant
2. Add new feedback on changed code since your last review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Read the current StrategyHub implementation
cat src/_common/Strategy/StrategyHub.cs

echo "---TESTS---"
# Read the tests
cat tests/public-api/customizable/StrategyHub.Tests.cs

echo "---GOLDEN_CROSS---"
# Read the GoldenCrossStrategy implementation
cat tools/simulate/GoldenCrossStrategy.cs

Length of output: 18573

---

@DaveSkender, I've completed a comprehensive re-review. Here's my assessment:

✅ Previously Raised Issues - Status

All major concerns from the thread have been resolved:

1. Type safety concern (JGronholz) - ✅ RESOLVED: StrategyGroup<T1,T2> provides compile-time type safety; no way to pass wrong hub types
2. Use() confusion - ✅ RESOLVED: Removed entirely; hubs stored in constructor
3. Not a hub - ✅ RESOLVED: StrategyGroup is a coordinator, not derived from any hub base class
4. Fixed arity vs flexible - ✅ RESOLVED: Implemented 2, 3, 4 hub variants as discussed

🎯 Current Implementation Quality

Excellent alignment with the clean-slate design:

✅ Strong typing - Generic parameters enforce compile-time safety
✅ Simple API - Constructor + single method (TryGetBackPair)
✅ Clear ownership - Hubs stored as properties
✅ Proper validation - Null checks, count checks, timestamp alignment
✅ Good reuse - StrategyGroupUtilities shared across variants
✅ Real-world usage - Golden Cross strategy demonstrates practical application

📝 Documentation Gaps

Missing XML docs on the 2-hub variant:

/// <summary>
/// Aligns the most recent back pairs across two hubs.
/// </summary>
public sealed class StrategyGroup<T1, T2>
{
    // ❌ Missing constructor docs
    public StrategyGroup(IStreamObservable<T1> hub1, IStreamObservable<T2> hub2)
    
    // ❌ Missing property docs
    public IStreamObservable<T1> Hub1 { get; }
    public IStreamObservable<T2> Hub2 { get; }
    
    // ❌ Missing method docs
    public bool TryGetBackPair(out BackPair<T1> pair1, out BackPair<T2> pair2)

The 3-hub and 4-hub variants have complete XML documentation, but the 2-hub (most commonly used) variant lacks it.

⚠️ Minor Implementation Concerns

1. Naming: BackPair vs AlignedPair

The term "BackPair" works but is less discoverable than "AlignedPair":

• "Back" suggests looking backward, which is accurate
• "Aligned" emphasizes timestamp coordination, which is the key feature
• "AlignedPair" was the original recommendation and is more descriptive

Recommendation: Consider renaming to AlignedPair<T> and TryGetAligned(...) for better discoverability.

2. Thread Safety Reliance

internal static bool TryGetBackPair<TResult>(
    IStreamObservable<TResult> hub,
    out BackPair<TResult> pair)
{
    IReadOnlyList<TResult> cache = hub.Results;  // Assumes immutable snapshot
    // ...
}

The code assumes hub.Results returns an immutable snapshot. This is likely safe given the streaming hub architecture, but no explicit locking is present. If Results is a live collection, there's a race condition window between accessing cache[^2] and cache[^1].

Recommendation: Add a comment documenting the thread-safety assumption, or wrap in a lock if Results can change during enumeration.

3. Type Casting in GoldenCrossStrategy

_strategyGroup = new StrategyGroup<EmaResult, EmaResult>(
    _quoteHub.ToEmaHub(FastPeriod),
    _quoteHub.ToEmaHub(SlowPeriod));
_fastEma = (EmaHub)_strategyGroup.Hub1;  // ❌ Cast defeats type safety
_slowEma = (EmaHub)_strategyGroup.Hub2;  // ❌ Cast defeats type safety

The strategy casts Hub1/Hub2 back to EmaHub to access .Results.Count. This is safe but defeats some benefits of the strongly-typed coordinator.

Alternative approach:

// Store hubs separately, pass to StrategyGroup
_fastEma = _quoteHub.ToEmaHub(FastPeriod);
_slowEma = _quoteHub.ToEmaHub(SlowPeriod);
_strategyGroup = new StrategyGroup<EmaResult, EmaResult>(_fastEma, _slowEma);

🧪 Test Coverage

Tests are solid but focused on happy path:

• ✅ Validates alignment logic
• ✅ Tests 2-hub scenario (golden cross)
• ⚠️ Missing edge case coverage:
  • Less than 2 results in hubs
  • Misaligned timestamps
  • 3-hub and 4-hub variants

🎯 Overall Assessment

This is a high-quality implementation that successfully addresses all the major concerns raised in the review thread. The design is clean, type-safe, and pragmatic.

Recommended polish:

1. Add XML documentation to the 2-hub StrategyGroup<T1, T2> variant
2. Document thread-safety assumptions around Results access
3. Consider renaming BackPair → AlignedPair for discoverability
4. Store EmaHub references separately in GoldenCrossStrategy to avoid casting

Ship it? Yes, with documentation additions. The core implementation is production-ready.

---

Would you like me to open an issue to track the documentation additions, or would you prefer to handle them in this PR?

[DaveSkender]

emphasizes timestamp coordination

@coderabbitai Are you sure this is true?
What kind of coordination is it?

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@DaveSkender, yes, I'm confident this is accurate. Let me clarify what's happening:

The Coordination in StrategyGroup

Looking at the implementation:

public bool TryGetBackPair(out BackPair<T1> pair1, out BackPair<T2> pair2)
{
    bool success1 = StrategyGroupUtilities.TryGetBackPair(Hub1, out pair1);
    bool success2 = StrategyGroupUtilities.TryGetBackPair(Hub2, out pair2);
    
    if (!success1 || !success2) { /* ... */ }
    
    // This is the coordination:
    if (pair1.Current.Timestamp != pair2.Current.Timestamp)
    {
        pair1 = default;
        pair2 = default;
        return false;  // ❌ Timestamps don't match - not aligned
    }
    
    return true;  // ✅ Timestamps match - aligned
}

What Kind of Coordination?

Temporal alignment — the method ensures that the "Current" results from both hubs share the exact same timestamp before returning them to the strategy logic.

Why This Matters:

In streaming scenarios, different hubs may process quotes at slightly different rates or times:

• Fast EMA hub might have processed 100 quotes
• Slow EMA hub might have processed 99 quotes
• Or they're at the same count but the latest timestamps don't match

Without this check: Your golden cross detection could compare a fast EMA from 10:30:05 against a slow EMA from 10:30:03 — mixing data from different moments in time.

With this check: The method returns false until both hubs have results with matching timestamps, ensuring your strategy compares apples-to-apples.

More Accurate Term

If "timestamp coordination" sounds unclear, streaming literature calls this "temporal alignment" or "watermark alignment" — preventing streams from getting too far ahead of each other and ensuring joins operate on synchronized time points.

In your case, it's simpler (no watermarks), but the principle is the same: ensuring cross-hub comparisons happen on data from the same point in time.

Does that clarify the coordination type, or would you prefer different terminology in the docs?

[DaveSkender]

I’m not settled on naming here, “strategy” is not the right terminology.

[DaveSkender]

Undecided on whether this StrategyGroup util is even a good idea for the library, unsure. Pausing.