fix(trading): include fees in cash risk checks

[ECD5A]

Summary

• quote a conservative exchange-fee ceiling before submission and include it in the deterministic cash check
• require an explicit fee on buy risk requests and canonical fills, rejecting invalid quantities, prices, estimates, and returned fill fees
• run cheap local risk checks before requesting a fee quote and return a normal blocked outcome when quoting fails
• re-check the canonical fill against cash and exposure limits before portfolio accounting
• share the basis-point fee calculation across paper adapters and pin estimate/fill parity with a contract test

Receipt

Before this change, a portfolio with $100.00 cash could buy 0.001 BTC @ $100,000 with a 10 bps fee. The risk check compared only the $100.00 notional to cash and allowed the order; the fill then debited $100.10, leaving cash at -$0.10.

The regression tests reproduced that failure before the implementation change: the direct risk decision was allowed: true, and the engine outcome was filled. Both now block the order before it reaches the exchange.

Design alignment

This advances Conviction 5: the deterministic pre-trade guard is the last line of defense. It also protects the cash/NAV and canonical-fill invariants described in ADR 0005.

The ExchangeClient/Fill contract tightening is recorded under CHANGELOG.md Unreleased as a breaking change for the next minor release; this PR intentionally does not bump the package version because release commits own version changes.

Tests

• npx tsc --noEmit
• npm run build
• node dist/index.js --help
• Targeted RiskEngine / TradingEngine / exchange-adapter suite: 21 passed
• npm run test:strategies: 3 passed
• git diff --check

A full local npm test run passes the complete modified trading block but does not finish green on Windows because unrelated HOME/temp/session-permission and CLI-timeout tests fail in this environment.

[VickyXAI]

Thanks for this. The receipt in the description is exactly the right way to prove a risk regression, and the core change is correct: the fee belongs in the deterministic cash check, not in the LLM's judgment (Conviction 5). The interface propagation is complete — both implementations and the test stub all gained estimateFee.

Requesting changes on four points that harden the guard you just built. The rest are suggestions, take or leave.

Blocking

1. src/trading/risk.ts:51 — guard the notional inputs the same way you guard the fee.
const notional = order.qty * order.priceUsd; has no finiteness/sign check, while feeUsd two lines down now does. NaN > cashUsd is false, so a NaN notional passes the cash check and both cap checks and comes out allowed; a negative qty makes cashRequired negative and turns a "buy" into free money at applyFill. The tool layer (trading-execute.ts:164) validates today, but RiskEngine.check is the documented last line of defense and is a public export. Mirroring your fee guard for qty/priceUsd is ~5 lines.

2. src/trading/risk.ts:53 — the new Invalid estimated fee branch has no test.
The PR adds tests for the fee-inclusion path but never exercises NaN/Infinity/negative fees, so the guard could be loosened later without anything failing. One test iterating [NaN, Infinity, -0.01] and asserting allowed === false closes it.

3. src/trading/engine.ts:62 — the actual fill fee is applied unvalidated after risk approval.
placeOrder's returned fill.feeUsd goes straight into applyFill, which debits notional + fee with no finiteness/sign check and no post-fill cash invariant. The estimate/fill agreement is only a comment on the interface. An adapter whose real fee exceeds its estimate (min-fee floors, tier changes) silently drives cashUsd negative, and it persists. Cheapest fix: validate fill.feeUsd (finite, >= 0) before applyFill and reject or flag fills whose fee materially exceeds the estimate the risk check approved.

4. src/trading/risk.ts:52 — feeUsd?: number with ?? 0 makes the check fail-open.
Any caller that forgets to thread the fee silently gets the old fee-blind check back, while the rejection message still says "including $0.00 estimated fee". Making feeUsd required on buy orders (tests pass 0 explicitly) turns that regression lane into a compile error. All four review passes flagged this one independently.

Suggestions (non-blocking)

• src/trading/engine.ts:52 — estimateFee now runs before any risk check, so orders that used to be rejected locally will hit a future adapter's fee endpoint (possibly paid or rate-limited). Note the existing "does NOT touch the exchange" test only counts placeOrder, so it doesn't see this. Worth either reordering (cheap local checks first) or counting estimateFee in that test.
• src/trading/engine.ts:52 — a rejecting estimateFee (the interface allows Promise<number>) escapes openPosition as a raw exception instead of the {status:'blocked'} outcome every other failure path returns.
• No contract test asserts estimateFee(order) === (await placeOrder(order)).feeUsd for the two implementations — that's the exact divergence class this PR exists to prevent.
• Exact-fit boundary is untested: the check is strict >, so notional + fee exactly equal to cash should be allowed; nothing pins that.
• The bps formula (qty * priceUsd * feeBps) / 10_000 is now duplicated in mock-exchange.ts:53 and live-exchange.ts:56. A shared bpsFee() helper would enforce the "same fee model" comment structurally.
• Adding a required method to ExchangeClient is breaking for out-of-tree implementers of the published package; suggest this lands as a minor bump with a changelog note rather than a patch.

[ECD5A]

Thanks for the thorough review. I addressed all four blocking points and the actionable suggestions:

• added finite/positive quantity and price guards at the RiskEngine boundary
• added table-driven coverage for NaN, Infinity, and negative fee estimates, plus invalid notional inputs
• made buy-side feeUsd and canonical fill fees required instead of defaulting missing values to zero
• validate returned fill identity and fee against the approved estimate, then re-run deterministic risk checks on the canonical fill before accounting
• run a zero-fee local risk pass before fee quoting, and map quote failures to a normal blocked outcome
• moved bps math to one helper and added estimate/fill parity coverage for both exchange implementations
• pinned the exact-cash boundary and documented the breaking contract change for the next minor release

The branch is rebased onto the current main. Type-check, build, CLI smoke check, 21 targeted tests, and 3 strategy tests pass. The full Windows run also passes the modified trading block; its remaining failures are unrelated HOME/temp/session-permission and CLI-timeout cases, noted in the updated PR description.

[ECD5A]

The four blocking review points are addressed in the current head (2c2b2f0): boundary validation for quantity/price, invalid fee coverage, canonical fill-fee validation with a post-fill risk check, and a required buy-side fee instead of a fail-open default.

I also covered the non-blocking hardening items that fit this patch: local checks before fee quoting, blocked handling for quote failures, estimate/fill parity coverage for both exchange implementations, the exact-cash boundary, shared bps math, and the breaking-contract changelog note.

The focused type-check/build/CLI smoke check, 21 targeted tests, and 3 strategy tests pass. Please re-review the updated branch when convenient.