feat(exchange): add agent-friendly broker CLI - #1317
Open
bennycode wants to merge 13 commits into
Open
Conversation
…mates - Market orders retry once without extended hours when the venue rejects the flag (LSE instruments returned 400 extended-hours-trading-not-allowed). - Limit orders no longer send extendedHours at all: the limit endpoint rejects any request carrying the field with a generic 400 Invalid payload, on demo and live, US and LSE alike. - estimateFee is now denominated in the pair's counter currency. It previously multiplied a counter-currency notional by the fee rate and labeled the result with the account currency, misstating a ~10 GBP Rolls-Royce trade's FX fee as EUR 1.50 instead of 1.5 GBX. The now unused getFeeAsset hook is removed. - Alpaca AccountSchema tolerates missing daytrade_count and pattern_day_trader, which live cash accounts omit.
One entrypoint for account inspection and the full order lifecycle on any supported broker, built to be driven by coding agents as much as by humans: - verify, balances, instruments, quote, rules, orders, fills, time - buy/sell (market or --limit), with --dry-run validating against trading rules and estimating fees without placing - wait: poll an order to its terminal state (--timeout, --poll) - cancel (single or --all) - candles (one-shot) and watch-candles/watch-orders streaming NDJSON, with --take to exit after n events - JSON on stdout, broker problem types verbatim on stderr, exit code 1 on failure - paper by default, --live for the real account - a per-Alpaca-key process lock: Alpaca serves one market-data WebSocket per key, and a second connection makes the server drop the first, so a concurrent stream on the same machine now fails fast naming the holder PID instead of silently starving one side - --idle fails a subscribed-but-silent stream with a diagnostic instead of hanging - the bin exits explicitly after flushing output, because broker WebSockets intentionally stay open for long-running sessions and kept finished one-shot commands alive forever Replaces the trading212 demo scripts and their npm aliases.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
There are correctness issues in the CLI streaming/locking implementation that can leak the lock and break the documented NDJSON output, plus a race in stale lock takeover that can allow concurrent streams.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds a broker-generic exchange-cli entrypoint to the packages/exchange package, exposing account inspection, quoting, order placement (incl. --dry-run), order lifecycle management, and streaming (watch-*) over the real getBrokerClient composition root, alongside several broker fixes discovered via live API usage.
Changes:
- Introduce
exchange-clicommand runner (runCli) with JSON/NDJSON output, trading-rule validation, order polling, and streaming guard rails (--idle, per-key stream lock). - Fix Trading212 order placement edge cases (extended-hours fallback for market orders; remove
extendedHoursfrom limit order schema/requests) and correct fee estimate denomination semantics in the baseBroker. - Relax Alpaca account schema for live accounts where day-trade fields may be omitted; add CLI docs to README/CLAUDE and wire up the package
bin.
File summaries
| File | Description |
|---|---|
| packages/exchange/src/cli/runCli.ts | New CLI command router, streaming/watch implementation, order wait + dry-run validation. |
| packages/exchange/src/cli/runCli.test.ts | New unit tests for CLI parsing, counter resolution, dry-run, wait, and streaming behaviors. |
| packages/exchange/src/cli/processLock.ts | New per-machine, per-key lock to prevent Alpaca stream slot contention. |
| packages/exchange/src/cli/processLock.test.ts | Tests for lock acquisition, release, stale lock takeover, and key hashing. |
| packages/exchange/src/cli/exchange-cli.ts | New executable wrapper that runs the CLI and exits after flushing output. |
| packages/exchange/src/broker/trading212/Trading212Broker.ts | Trading212 market-order extended-hours fallback; remove extended-hours from limit flow; remove now-unused fee-asset hook. |
| packages/exchange/src/broker/trading212/Trading212Broker.test.ts | New tests for fee estimate denomination and extended-hours order placement behavior. |
| packages/exchange/src/broker/trading212/demo/listBalances.ts | Removed demo script (superseded by CLI). |
| packages/exchange/src/broker/trading212/demo/buy.ts | Removed demo script (superseded by CLI). |
| packages/exchange/src/broker/trading212/api/schema/OrderSchema.ts | Remove extendedHours from limit-order request schema and document the API behavior. |
| packages/exchange/src/broker/Broker.ts | Change estimateFee() denomination semantics to always be in pair.counter; remove getFeeAsset hook. |
| packages/exchange/src/broker/alpaca/api/schema/AccountSchema.ts | Allow daytrade_count / pattern_day_trader to be nullish for some live accounts. |
| packages/exchange/README.md | Document the new CLI and its agent-friendly/streaming behavior. |
| packages/exchange/package.json | Publish exchange-cli as a package binary; add npm run cli script; remove Trading212 demo scripts. |
| packages/exchange/CLAUDE.md | Add guidance to use/extend the CLI instead of adding demo scripts. |
Review details
- Files reviewed: 15/15 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+184
to
+186
| const releaseLock = options.acquireLock?.(); | ||
| const idleMillis = options.idle ? parseInterval(options.idle) : undefined; | ||
| let idleTimer: NodeJS.Timeout | undefined; |
|
|
||
| function parseInterval(value: string): number { | ||
| if (!isIntervalString(value)) { | ||
| throw new Error(`Invalid --interval "${value}". Use a duration like 1m, 5m, or 1h.`); |
Comment on lines
+97
to
+99
| Credentials come from <BROKER>_PAPER_API_KEY/_SECRET or <BROKER>_LIVE_API_KEY/_SECRET | ||
| environment variables (e.g. TRADING212_PAPER_API_KEY). Output is JSON on stdout; | ||
| watch-* commands emit one JSON object per line as events arrive. |
|
|
||
| try { | ||
| const result = await runCli(process.argv.slice(2)); | ||
| flushAndExit(process.stdout, result.text ?? JSON.stringify(result.json, null, 2), 0); |
Comment on lines
+24
to
+38
| } catch (error) { | ||
| if (!isErrnoException(error) || error.code !== 'EEXIST') { | ||
| throw error; | ||
| } | ||
| const holderPid = Number(readFileSync(lockFile, 'utf8')); | ||
| if (isProcessAlive(holderPid)) { | ||
| throw new Error( | ||
| `Another stream (PID ${holderPid}) is already connected with this Alpaca API key. ` + | ||
| `Alpaca serves one market-data connection per key — a second connection makes the server drop the first. ` + | ||
| `Stop that process or wait for it to finish.` | ||
| ); | ||
| } | ||
| // Stale lock from a dead process — take it over. | ||
| writeFileSync(lockFile, `${process.pid}`); | ||
| } |
- Atomic stale-lock takeover: remove the dead holder's file and re-race the wx create so two contenders can't both proceed. - Validate --idle before acquiring the stream lock so a parse error cannot leak the lock file. - Duration parse errors name the flag that failed (--idle, --timeout, --poll) instead of always saying --interval. - The closing watch-* summary prints compact so stdout stays one JSON object per line. - Usage text spells out both credential env var names instead of the ambiguous _API_KEY/_SECRET shorthand.
Owner
Author
|
Addressed the review in 78fc270:
The CodeQL finding is dismissed as a false positive: the SHA-256 fingerprints the API key id (not the secret) purely to name a lock file without leaking the credential. It's never stored or compared as an authentication verifier, so password-KDF guidance doesn't apply. |
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
Dry-run validation, stream locking, and NDJSON output contain correctness issues that should be resolved before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (6)
packages/exchange/src/cli/runCli.ts:185
- The lock is acquired before
--idleis parsed, but parsing occurs outside thetry/finally. An invalid idle value therefore throws without releasing the lock; in a long-lived caller ofrunCli, every later stream using this key is rejected as already held. Parse first or move this initialization inside the protected region.
writeEvent: (line: string) => void;
}): Promise<number> {
packages/exchange/src/cli/exchange-cli.ts:16
- After a
watch-* --takecommand, streamed events are single-line JSON but this final{events}result is pretty-printed across several lines. That makes stdout invalid NDJSON despite the documented one-object-per-line contract. Emit the streaming summary compactly (or suppress it).
// `compact` keeps the closing summary of watch-* commands on one line, preserving NDJSON output.
packages/exchange/src/cli/runCli.test.ts:317
- This test does not prove that the lock remains held for the whole watch: moving
releaseStreamLock()immediately after acquisition would preserve both call-count assertions. Hold the mocked subscription open and assert that release has not occurred before emitting the terminating event.
await runCli(['watch-candles', 'RRl_EQ', '--broker', 'trading212', '--counter', 'GBX', '--take', '1'], deps);
expect(
deps.acquireStreamLock,
'Trading212 candle streams ride the Alpaca socket, so they must hold the Alpaca-key lock'
).toHaveBeenCalledWith('alpaca-paper-key');
expect(deps.releaseStreamLock).toHaveBeenCalledTimes(1);
packages/exchange/src/cli/processLock.test.ts:59
- This assertion exercises the test-local
lockFileForcopy rather thanacquireStreamLock; changing production to put the raw key in its filename would leave the test green. Observe the file created by production (or expose and test one shared path builder) so that mutation is detected.
it('does not leak the credential into the lock file name', () => {
const key = 'super-secret-api-key';
expect(lockFileFor(key)).not.toContain(key);
packages/exchange/src/cli/processLock.ts:37
- Stale-lock takeover is not atomic: after observing a dead PID, this unconditional write lets two contenders both overwrite the file and both believe they acquired the lock. The read also races a holder releasing the file. Use an ownership-safe locking primitive or an atomic retry protocol instead of overwriting the existing pathname.
}
const holderPid = readHolderPid(lockFile);
packages/exchange/src/cli/runCli.ts:143
- This parser is also used for
--timeout,--poll, and--idle, but every malformed value is reported as an invalid--interval. That sends users and agents to the wrong option; pass the originating flag into the parser and name it in the diagnostic.
function parseInterval(value: string, flag: string): number {
if (!isIntervalString(value)) {
- Files reviewed: 15/15 changed files
- Comments generated: 6
- Review effort level: Balanced
| if (!isIntervalString(value)) { | ||
| throw new Error(`Invalid --interval "${value}". Use a duration like 1m, 5m, or 1h.`); | ||
| } | ||
| return ms(value); |
Comment on lines
+300
to
+302
| if (new Big(rules.base_increment).gt(0) && !quantity.mod(rules.base_increment).eq(0)) { | ||
| violations.push(`Size ${size} is not a multiple of the increment ${rules.base_increment}.`); | ||
| } |
Comment on lines
+303
to
+307
| if (violations.length > 0) { | ||
| throw new Error(`Dry run failed:\n- ${violations.join('\n- ')}`); | ||
| } | ||
|
|
||
| const notional = price.times(quantity); |
Comment on lines
+527
to
+529
| const events = await watchStream({ | ||
| acquireLock: needsLock && alpacaStreamKey ? () => deps.acquireStreamLock(alpacaStreamKey) : undefined, | ||
| broker, |
Comment on lines
+37
to
+38
| placeLimitOrder: vi.fn(), | ||
| placeMarketOrder: vi.fn(), |
Comment on lines
+43
to
+45
| const release = acquireStreamLock(key); | ||
| expect(release).toBeTypeOf('function'); | ||
| release(); |
The usage text was a hand-aligned template string that had to be kept in sync with the parseArgs config by eye. Commands and option help are now data (COMMANDS, OPTION_HELP keyed by keyof CLI_OPTIONS, so an option without help fails to compile) and the usage is rendered from them with automatic column alignment and wrapping. Unknown commands are rejected against the same list.
…ion environment Live execution should run on live-grade data: Alpaca's sandbox stream host is a test environment with no real-time guarantee. The CLI now prefers the Alpaca credential pair matching --live and falls back to the other pair when the preferred one is not configured, since market data is read-only and borrowing keys beats having no candles. This replaces the ALPACA_USE_PAPER toggle, which picked the data environment independently of the execution environment. Also empties the .env.defaults placeholders: the literal value 'secret' is truthy, so it masked every missing-credential check and would be sent to the broker as a real key.
The bin loads .env.live when --live is passed and .env.sandbox otherwise (process.loadEnvFile, both optional), so live credentials can live in their own file: a machine without .env.live simply cannot trade the real account. Variables already present in the environment win, matching --env-file semantics. Also comments out the .env.defaults credential placeholders: they were defined as empty strings, which loadEnvFile treats as existing variables and refuses to override, shadowing the mode files.
…ency The CLI selects .env.sandbox / .env.live via --live, so the defaults file had shrunk to comments plus two USE_PAPER flags nothing reads anymore. Demo scripts now load env files through tsx's native --env-file-if-exists flags instead of importing dotenv-defaults, which also removes the package's last @ts-ignore. Key-creation links already live in the README's Supported Brokers section.
Dropping the dotenv-defaults import cost the demo scripts their self-containment: a direct 'npx tsx <script>' loaded no environment. A shared loadEnvFiles util (process.loadEnvFile with skip-on-missing and never-override semantics) restores that for the demos and the CLI bin alike, so the npm scripts no longer need --env-file flags at all.
…ronment file The mode was encoded twice: in the file name (.env.live) and again in every variable name (TRADING212_LIVE_API_KEY). Now the file is the environment: both files carry the same unprefixed names (TRADING212_API_KEY, ALPACA_API_KEY, ...) and --live only picks the file. This deletes the LIVE/PAPER infix logic, the Alpaca credential fallback, and the TRADING212_USE_PAPER flag. Market data always uses Alpaca's production hosts: the sandbox data hosts are Broker-API-partner infrastructure and reject regular account keys with a 401 (verified live). The data-host switching inside AlpacaAPI remains untouched but is suspect for regular paper accounts for the same reason.
Market data is account-entitled, not environment-specific: paper and live keys both authenticate against data.alpaca.markets and stream.data.alpaca.markets (verified live with both key types). The sandbox data hosts are Broker-API-partner infrastructure and reject regular account keys with a 401, so pointing paper accounts at them broke every market-data call for real paper users. usePaperTrading now only selects trading-API hosts (REST and the trading stream), and AlpacaStreamCredentials drops the flag it no longer uses. The CLI and demo client pass the flag matching the key type their environment file provides.
exchange-cli can fetch candles but could not compute anything over them — analyzing volatility or momentum meant writing a throwaway script. The new command runs trading-signals indicators over recent candles: atr, ema, rsi, sma with --period (per-indicator defaults) and the usual --interval/--count. ATR additionally reports valuePct (ATR / last close), the normalized number that makes volatility comparable across instruments. Adds trading-signals as a dependency; the layering stays acyclic since it is the leaf package (trading-strategies already depends on both).
This reverts commit 1cce9a4.
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.
What
A broker-generic
exchange-clibinary covering account inspection and the full order lifecycle on any supported broker, plus four real bugs found by driving every command against live broker APIs while building it.CLI
verify,balances,instruments,quote,rules,orders,fills,buy/sell(market or--limit, with--dry-runvalidating against trading rules and estimating fees without placing),wait(poll an order to its terminal state),cancel,candles,watch-candles/watch-orders(NDJSON streaming,--taketo exit after n events),time.--liveopt-in.getBrokerClient), so every invocation exercises the production path. Credentials via<BROKER>_API_KEY/_API_SECRETenv vars, loaded from.env.livewhen--liveis passed and.env.sandboxotherwise — the file is the environment, so a machine without.env.livecannot trade the real account. Trading212 candle commands source market data from Alpaca (ALPACA_API_KEYfrom the same file; always the production data hosts, since the sandbox data hosts are Broker-API-partner-only and 401 regular keys).--idleturns a subscribed-but-silent stream into a quick diagnostic failure instead of an indefinite hang. The bin exits explicitly after flushing output because the broker WebSockets deliberately stay open for long-running sessions.trading212:buy/trading212:listBalancesdemo scripts.Fixes (each found live)
extendedHours: true, which LSE instruments reject with 400. Market orders now retry once on regular hours when the venue rejects the flag.extendedHourswith a generic 400 "Invalid payload" (verified on demo and live, US and LSE). The field is removed from the request schema.estimateFeemisstated cross-currency fees ~100x: it multiplied a counter-currency notional by the fee rate and labeled the result with the account currency, turning a ~10 GBP trade's FX fee of 1.5 GBX into "EUR 1.50". Estimates are now denominated inpair.counter; the realised debit currency still comes back onFill.feeAsset. The now unusedgetFeeAssethook is removed (no consumers).daytrade_countandpattern_day_traderare omitted on some live accounts and are nownullish(nothing consumes them).Verification
142 tests (37 new), typecheck and lint clean. Every command was additionally exercised against the live APIs: Trading212 paper full order lifecycle including
waittimeout and cancel, Alpaca live quote/candles/streaming, lock contention between two concurrent streams (loser refused instantly naming the holder PID, winner streamed undisturbed), and--idlefiring after 5s of silence.