Skip to content

fix(data): allow full strategy export pulls - #516

Open
mattamundson wants to merge 3 commits into
tradesdontlie:mainfrom
mattamundson:fix/full-strategy-export-caps
Open

mattamundson wants to merge 3 commits into
tradesdontlie:mainfrom
mattamundson:fix/full-strategy-export-caps

Conversation

@mattamundson

Copy link
Copy Markdown

Summary

  • raises OHLCV extraction cap from 500 to 10,000 bars
  • raises Strategy Tester filled-order cap from 20 to 1,000 orders
  • keeps existing command/API surface unchanged

Why

Full daily Strategy Tester parity runs need more history and filled orders than the previous hard caps allowed. A verified SPY daily Strategy Lab run required 8,460 daily bars and 247 filled orders; the prior caps silently truncated both surfaces.

Verification

  • node --check src/core/data.js
  • npm run test:unit
  • npm run lint
  • node --test tests/chart_history.test.js tests/cli.test.js
  • local full-order extraction check returned 247/247 Strategy Tester orders

@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 10, 2026

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review Completed 2026-09-10T23:50:51.778181Z a9074bc New commits
🔒 Security Review Completed 2026-09-10T17:03:20.617612Z 7a98e7e PR opened
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 7a98e7e963

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread src/core/data.js
Comment on lines +7 to +8
const MAX_OHLCV_BARS = 10000;
const MAX_TRADES = 1000;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Synchronize the exposed limits with the new caps

When users access these functions through the supported MCP, CLI, or strategy-report workflow, the public guidance still advertises the old limits: src/tools/data.js:7 and src/cli/commands/data.js:12 say OHLCV is capped at 500, while skills/strategy-report/SKILL.md:14 and CLAUDE.md:120 say trades are capped at 20. As a result, clients and agents following those contracts will not request the newly supported ranges, so the intended full-export behavior remains undiscoverable and strategy-report runs can continue returning only 20 orders; update these surfaces alongside the constants.

Useful? React with 👍 / 👎.

Comment thread src/core/data.js

const MAX_OHLCV_BARS = 500;
const MAX_TRADES = 20;
const MAX_OHLCV_BARS = 10000;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Apply the larger OHLCV cap to batch retrievals

When OHLCV is requested through the supported batch_run path with ohlcv_count above 500, src/core/batch.js:48 still computes Math.min(ohlcv_count || 100, 500) and slices the export to that value. Because this path implements extraction independently rather than calling getOhlcv, the new 10,000-bar cap has no effect and batch parity/export runs remain silently truncated to 500 bars; share the new cap with this path or update its limit as well.

Useful? React with 👍 / 👎.

@mattamundson

Copy link
Copy Markdown
Author

Pushed follow-up hardening commit a9074bc to this PR branch.

Local verification on Windows after the update:

  • npm ci -> pass
  • npm run test:unit -> 159 passed / 0 failed
  • npm run lint -> exit 0, 4 existing warnings
  • npm audit --omit=dev --json -> 0 vulnerabilities

Follow-up contents:

  • Update CLAUDE.md cap docs to match the new 10,000 OHLCV / 1,000 trades limits.
  • Add license: MIT package metadata.
  • Refresh lockfile transitive deps to clear production npm advisories.

Still intentionally scoped to TradingView Desktop/CDP research workflow; no execution/trading integration implied.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant