Add an indicator CLI and a shared indicator registry, reused by MCP - #416
Draft
cinar wants to merge 7 commits into
Draft
Add an indicator CLI and a shared indicator registry, reused by MCP#416cinar wants to merge 7 commits into
cinar wants to merge 7 commits into
Conversation
Introduces a name-based registry package that looks up an indicator, binds its parameters, and computes it over asset snapshots. Both the new indicator-cli command and the MCP server's new "indicator" tool are built on it, so the two front ends stay in sync as indicators are added instead of duplicating lookup and parameter-binding logic. Also points mcp/go.mod at the local module tree via a replace directive, so mcp/ builds against the current source instead of the last tagged release. Fixes #415 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #416 +/- ##
==========================================
- Coverage 92.32% 91.83% -0.49%
==========================================
Files 229 235 +6
Lines 7280 8185 +905
==========================================
+ Hits 6721 7517 +796
- Misses 472 575 +103
- Partials 87 93 +6 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
…CP goroutine leak - Format dates and floats the same way helper.Csv does (helper.DefaultDateTimeFormat, 'g' verb) instead of a hardcoded date layout and 'f', so indicator-cli's CSV output matches how the rest of the project formats the same OHLCV data. - writeCsv now takes io.Writer instead of *os.File. - mcp/indicator.go's runIndicator now derives a cancelable context and has its snapshot-producing goroutine select on ctx.Done(). Previously, an early return (e.g. a series ending before the dates did) left the rest of the computation pipeline's goroutines blocked forever with nothing draining them -- a leak per failed request in this long-lived server process. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Unanchored, it also matched cmd/indicator-cli/ (any path segment named indicator-cli, not just the build output at the repo root), which silently offered to skip the source directory on git add. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Registers nearly every trend, momentum, volatility, and volume
indicator (one Register(...) call each, following the original 8 as
templates), split into per-category files for readability.
Six types are deliberately left out, each with a comment explaining
why:
- MovingMax, MovingMin, MovingSum, MovingStd, trend.Stochastic,
momentum.Streak: internal building blocks other indicators
compose, not traded on their own.
- Envelope, Mlr, Mls: no natural default (a caller-supplied Ma
implementation, or an arbitrary (x, y) series with no fixed OHLCV
field to bind to).
- PivotPoint: returns a struct per value instead of a plain number,
which doesn't fit the Series shape.
Registry.Run also changed how it feeds an indicator's inputs: instead
of one synchronized multi-way fan-out of the snapshot channel (via
helper.DuplicateWithContext), it now materializes the snapshots once
and gives the dates and each Field its own independent producer. This
was required to register momentum.ConnorsRsi and momentum.Rvi, which
read their whole input before returning anything -- sharing a
lock-step fan-out with those deadlocks, since nothing drains the dates
channel until Run itself returns. The new approach also makes Result's
Dates and Series channels safe to drain in any order/pace, not just
interleaved.
Along the way, exercising every indicator against 400 synthetic
snapshots surfaced four pre-existing bugs in the underlying library,
unrelated to and reproducible without this registry, each worth its
own issue:
- trend.Stc: ComputeWithContext's output channel never closes when
fed from an independent producer.
- momentum.PringsSpecialK: has no IdlePeriod() method, unlike every
other indicator, so callers can't know how many leading dates to
skip.
- trend.T3 and momentum.Fisher: ComputeWithContext yields far fewer
values than IdlePeriod() promises (reproduced calling them
directly, with no registry code involved).
These four are excluded from the registry with a comment pointing at
this finding, rather than registered with known-wrong output.
Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Aug 21, 2026
cinar
added a commit
that referenced
this pull request
Aug 21, 2026
Closes #418. ## What was missing Every other indicator in this library implements `IdlePeriod() int` (the subject of the consistency sweep in #413) so a caller knows how many leading values to skip before the output channel starts yielding real data. `PringsSpecialK` didn't, even though `ComputeWithContext` already computes exactly this value internally: ```go maxIdle := p.Sma195Roc530.IdlePeriod() + p.Roc530.IdlePeriod() ``` used to align every other SMA(ROC(...)) branch to the slowest one. Without an exposed `IdlePeriod()`, a generic caller (e.g. the indicator registry in #416) has no way to know how many leading dates correspond to its warm-up — which, with the default periods, is 724. Its own test already worked around this by reconstructing an equivalent value from unrelated standalone `Sma`/`Roc` instances rather than calling a method on the type itself — a sign the type should expose it. ## Fix ```go // IdlePeriod is the initial period that Pring's Special K won't yield any results. func (p *PringsSpecialK[T]) IdlePeriod() int { return p.Sma195Roc530.IdlePeriod() + p.Roc530.IdlePeriod() } ``` `TestPringsSpecialKComputeBasicOutput` now calls `IdlePeriod()` on the actual instance instead of hand-reconstructing the number from a throwaway `Sma(530)`/`Roc(195)` pair (which happened to total the same value only because `(530-1)+195 == 194+530`) — it already passed before this change and still does, confirming the new method's value matches what that test independently expected all along. Added `TestPringsSpecialKIdlePeriod` as a direct check. ## Test plan - [x] `go test -race -cover ./momentum/...` — 92.4% coverage - [x] `go vet`, `staticcheck`, `gosec` clean on `momentum/` (an unrelated pre-existing `revive` finding in `rvi.go`, a file this PR doesn't touch, is untouched) - [x] `go build ./...` and the full `INDICATOR_BASE` test suite (`asset`, `backtest`, `cmd`, `helper`, `momentum`, `strategy`, `trend`, `volatility`, `volume`) pass 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
…rmula fixes Merges master (which fixed the channel/length bugs in #421-424) and registers momentum.Fisher and momentum.PringsSpecialK, both of which now check out against real and synthetic data. trend.Stc and trend.T3 stay excluded: their respective #421/#423 fixes only addressed the channel/length symptom, not underlying formula bugs (near-zero-denominator division in Stc, wrong weighted-sum coefficients in T3) filed separately as #425/#426. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #415.
Summary
registrypackage (root module): a name -> indicator lookup. Given an indicator name, string parameters (e.g.Period=14, or nested paths likeEma1.Periodfor MACD), and a channel ofasset.Snapshot, it computes the indicator and returns its named output series aligned with dates. 76 indicators registered across trend, momentum, volatility, and volume (-liston the CLI, orregistry.Names(), enumerates them) — split intodefinitions_trend.go/definitions_momentum.go/definitions_volatility.go/definitions_volume.gofor readability.cmd/indicator-cli, matching the shape ofindicator-sync(-source-name/-source-configfor the asset repository,-lastfor a lookback window). Looks up an indicator by name via the registry and writes the result as CSV to stdout (dates/floats formatted the same wayhelper.Csvformats them elsewhere in this project);-listprints the registered names.indicatortool on the MCP server (mcp/indicator.go,mcp/mcp_server.go), built on the same registry as the CLI, so both front ends stay in sync as indicators are added.mcp/go.modnow has areplace github.com/cinar/indicator/v2 => ../directive, somcp/builds against the local tree instead of trailing the last tagged release — otherwise the new registry package would be invisible to it../registry/...totaskfile.yml'sINDICATOR_BASEand anindicator-clibuild tobuild-tools; added/indicator-cliand/mcp/mcpto.gitignore(root-anchored — an earlier unanchoredindicator-clientry accidentally also matched thecmd/indicator-cli/source directory).master's subsequent fixes for trend.Stc: ComputeWithContext output channel never closes #417-420 (landed as Fix Stc.ComputeWithContext output channel never closing #421-424) and registered the two that turned out fully correct:momentum.Fisher(fisher) andmomentum.PringsSpecialK(prings_special_k).Not registered, on purpose
MovingMax,MovingMin,MovingSum,MovingStd,trend.Stochastic(the generic single-series one),momentum.Streak: internal building blocks other indicators compose, not traded on their own.Envelope,Mlr,Mls: no natural default to construct with (a caller-suppliedMaimplementation, or an arbitrary(x, y)series with no fixed OHLCV field to bind to).PivotPoint: returns a struct per value instead of a plain number, which doesn't fit theSeriesshape every other indicator here uses.trend.Stc,trend.T3: exercising every registered indicator against real and synthetic data surfaced that Fix Stc.ComputeWithContext output channel never closing #421/Fix T3 dropping most of its output #423 only fixed each one's channel-closing/output-length symptom, not its underlying formula.Stcdivides by a denominator that's frequently near zero, yielding-Infand swings like1435/-710for an indicator documented to oscillate 0-100.T3's weighted-sum coefficients don't sum to 1, so it doesn't track price at all (a flat 100-input returns-63). Filed as trend.Stc: STC formula divides by a near-zero denominator, producing -Inf and huge out-of-range swings #425/trend.T3: weighted-sum coefficients don't sum to 1, so T3 doesn't track price at all #426, with fixes up as PR Fix STC dividing by a near-zero denominator, producing -Inf/huge out-of-range swings #428/Fix T3's weighted-sum coefficients not summing to 1 #429 (not yet merged) — along with a third bug found in the process,trend.MovingSumpermanently going NaN after a single bad value instead of recovering once it leaves the window (trend.MovingSum: a single NaN/Inf permanently poisons the running sum, even after it leaves the window #427, PR Fix MovingSum's running sum staying NaN/Inf forever after one bad value #430).Design notes from review
registry.Runfeeds each indicator's input from its own independent producer (materializing the snapshots once, then onehelper.SliceToChanWithContextper consumer) rather than a single synchronized fan-out (helper.DuplicateWithContext) shared between the dates and every Field. This was required to registermomentum.ConnorsRsi/momentum.Rvi, which read their entire input before returning anything — sharing a lock-step fan-out with those deadlocks, since nothing drains the dates channel untilRunitself returns. As a side effect,Result'sDatesandSerieschannels are now safe to drain in any order/pace, not just interleaved.indicator-cli's CSV writer formats dates and floats the same wayhelper.Csvdoes (helper.DefaultDateTimeFormat,'g'verb) instead of a hardcoded layout and'f', so its output reads like any other CSV this project produces;writeCsvtakesio.Writerinstead of*os.File.mcp/indicator.go'srunIndicatorderives a cancelable context and its snapshot-producing goroutine selects onctx.Done(). Previously, an early return (e.g. a series ending before the dates did) left the rest of the computation pipeline's goroutines blocked forever in this long-lived server process — a leak per failed request.registry/registry_test.go'sTestRunEveryRegisteredIndicatorsynthetic fixture is now 800 points (was 400):prings_special_k'sIdlePeriod()is 724, the longest of any registered indicator, and needs the extra runway.Test plan
go build ./...(root module) andgo build ./...(mcp module)go vet,staticcheck,revive,gosecclean on the new/changed packagesgo test -race -cover ./registry/...— 95%+ coverage, including a smoke test that runs every registered indicator (all 76) end to endgo test -race -cover ./...inmcp/— includes an in-process MCP client test calling the newindicatortoolgo test -race ./...at the repo root — full suite, no regressions from the master mergeasset/testdata/repository/brk-b.csv: single-output (sma), nested-param (macd -param Ema1.Period=), multi-output (ichimoku_cloud5 series,kdj,aroon), the two newly-registered indicators (fisher,prings_special_k),-list, and the unknown-indicator error pathtask docswas not run — it produced degraded output (lost GitHub blob-link resolution across every package) in the sandbox this was built in; worth a separate docs pass before merging, consistent with how doc updates have landed as their own PRs recently (e.g. Document update. #414).🤖 Generated with Claude Code