feat: sim snapshot per-position marks and margin-aware buying power

[rustyeddy]

What changed

internal/account

• New PositionMark {Listing ListingKey, Price, AsOf} and SnapshotParams.Marks. Validation: each mark must name an open position, appear at most once, have a positive price, and have a set AsOf that is not after the snapshot's AsOf.
• Marks are optional per position, as ADR-066 requires: consumers treat a missing mark as an error, never as zero or AvgPrice.
• Accessors: Snapshot.Marks() (a copy, in Positions order) and Snapshot.Mark(key).

internal/adapters/broker/sim

• Marks record the simulator clock time they were set (fill, Advance, or ObserveMark) and are reported on every snapshot.
• The private positionKey is replaced by account.ListingKey, per the feat: shared gross-notional / initial-margin calculation #421 follow-up.
• New optional AccountConfig.InitialMarginRatio (positive), stored as a margin.Ratio.

When the ratio is set, the snapshot's margin fields come from the shared internal/account/margin calculation:

Field	Value
MarginUsed	required margin on gross notional, each position at its mark
MarginAvailable	equity − MarginUsed; negative when over the limit, with no forced action (v1)
BuyingPower	MarginAvailable ÷ ratio, floored at 0, since an over-limit account has no funds to open new positions

When it's nil (every existing caller), the fields are unchanged: BuyingPower = MarginAvailable = cash, and MarginUsed = 0.

• New AccountConfig.MarginModelInfo(): returns {Name: "none"} when unset, or {initial-margin-ratio, v1, "ratio=<r>"}. feat: backtest initial_margin_ratio config and risk-engine composition #414 will record it in the manifest.

Why

#412, step 3 of the Account Margin Admission milestone (#409). The risk rule (#413) needs a current price for every open position, and the simulator needs real margin fields.

One observation, now pinned by a test: the simulator's cash moves only by realized P&L and fees, not by notional. So in legacy mode BuyingPower stays at the full starting cash no matter how much is open (a 4.4× position still reports $10,000 of buying power). That is the #409 gap. With a ratio configured, the fields are computed from equity instead.

How it was tested

go test -race passes: sim at 88.5% coverage, account at 100%. New tests:

• Config: invalid ratio (0, negative) rejected; MarginModelInfo returns none, or ratio-specific values that are distinguishable.
• Margin fields:
  • legacy (no ratio), unchanged even at 4.4× exposure
  • flat at 1.0 and 0.5
  • long at 1.0 and 0.5
  • short at 1.0
  • multi-instrument long + short at 1.0 and 0.5
  • over the limit at entry: MarginAvailable −34000, BuyingPower 0
• Adverse move: ratio 0.5 with an adverse close gives equity 2500, MarginAvailable −2000, BuyingPower 0. The position is untouched and there are no orders.
• Marks:
  • a fill sets price and time
  • Positions order, deterministic across calls
  • Advance updates only the observed listing, so the other listing's mark is older
  • ObserveMark updates price and time
  • snapshot validation (each rejection case) and optionality
• Full-notional sizer (ADR-061): run against real simulator snapshots it respects the ratio. 1.0 gives 9090 units; 0.5 still caps at equity with 9090; 2.0 halves it to 4545.

All existing sim tests pass unchanged, because no ratio keeps the legacy behavior. No golden fixtures changed. make fmt-check vet lint is clean. The only failure in go test ./... is TestPublicSurface from the stray clock/ package on main, which #423 fixes.

Documentation changed

Go doc comments: PositionMark (including the staleness semantics), SnapshotParams.Marks, AccountConfig.InitialMarginRatio, MarginModelInfo, and marginFieldsLocked.

Closes #412

🤖 Generated with Claude Code

[Copilot]

Copilot review overview

🟡 Changes recommended

Zero-price inputs can poison simulator snapshots, and JSONL round trips currently discard the newly added marks.

Review effort: Balanced
Findings: 2 · 2

Open (4)

• Reject zero fill prices before committing state · New
• Reject zero observation marks before account mutation · New
• Preserve marks in JSONL account snapshot round trips · New
• Build margin snapshots from current positions only · New

What changed in this PR

Adds per-position snapshot marks and optional margin-aware simulator accounting for #412.

Changes:

• Adds validated, ordered position marks to account snapshots.
• Adds configurable initial-margin calculations and model metadata.
• Adds tests for marks, margin fields, and sizing behavior.

File	Description
internal/​account/​mark.go	Defines and validates position marks.
internal/​account/​mark_test.go	Tests mark validation and ordering.
internal/​account/​snapshot.go	Exposes marks through snapshots.
internal/​adapters/​broker/​sim/​config.go	Adds margin-ratio configuration.
internal/​adapters/​broker/​sim/​broker.go	Initializes mark and margin state.
internal/​adapters/​broker/​sim/​advance.go	Timestamps observed marks.
internal/​adapters/​broker/​sim/​account.go	Computes margin-aware snapshot fields.
internal/​adapters/​broker/​sim/​margin_test.go	Tests simulator margin behavior.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[rustyeddy]

Fixed in c67b0c1: buildFill rejects a zero final price (after tick rounding and slippage, before commission or anything else is committed) with ErrInvalidFill. TestZeroFillPriceLeavesAccountUnchanged checks that there are no positions, marks, or open orders, that AsOf is unchanged, and that the order was never stored, and that the same request fills once a valid price exists.

[rustyeddy]

Fixed in c67b0c1: ObserveMark rejects a zero price with ErrInvalidObservation before mutating, and Observation.validate now requires a positive Low. That covers Advance and AdvanceBar, the line-197 path. TestZeroObservationPricesRejectedBeforeMutation covers all three and checks that marks and AsOf are unchanged.

[rustyeddy]

Fixed in c67b0c1: accountWire gains marks (markWire: instrument/provider/venue, price, as_of, with omitempty so older entries still decode). On read, each mark's listing is resolved against the entry's own decoded positions; a mark naming no position is ErrCorruptEntry. TestWriterReaderRoundTripsAccountMarks round-trips two listings, one with a venue, with different AsOf values.

[rustyeddy]

Fixed in c67b0c1: marginFieldsLocked builds margin.Marks while iterating the current positions, so historical marks for closed listings are no longer copied.

[rustyeddy]

Reviewed PR #424 at head db3fea2accdb4b5480412eb7197f784b2b0dcea0.

The overall architecture looks right: Snapshot gains explicit per-listing marks, the simulator preserves legacy behavior when no margin model is configured, and the configured path derives margin fields from the shared #421 calculation.

I agree with the four existing Copilot findings. Two are correctness blockers, one is persistence/audit correctness, and one is an avoidable scaling problem.

1. Reject zero fill prices before they can become marks/state

A fill price that is zero must be rejected before commitFill mutates the position/mark state.

The new account.PositionMark validation correctly rejects a zero price, so allowing a zero fill to commit means the simulator can enter a state that can no longer produce a valid Snapshot.

That is particularly dangerous because mutation has already happened by the time snapshot construction reports the problem.

Please validate the fill price on the fill path before committing any position, mark, cash/PnL, order, or event state, and add a regression test proving a rejected zero-price fill leaves the account unchanged.

2. Reject invalid ObserveMark prices before mutation

ObserveMark currently calls observeMark directly and observeMark writes the supplied price into s.marks without validating it.

Again, a zero mark can poison an otherwise valid open position so the next Snapshot fails validation.

AdvanceBar goes through Observation.validate(), but the narrower ObserveMark path needs the same basic positive-price guarantee.

This should return an error and leave marks / asOf untouched.

3. JSONL journal round-trip must preserve marks

This is a real audit/reproducibility issue, not just serialization polish.

account.Snapshot now contains semantically meaningful marks and mark timestamps, but internal/adapters/journal/jsonl/accountWire still serializes only positions/open orders and the numeric account fields.

On read-back, fromAccountWire reconstructs the Snapshot with no Marks, silently losing information.

Given the JSONL adapter's own stated contract—explicit wire shapes so audit-relevant state is not silently discarded—marks need a wire representation and round-trip coverage.

I would add something like:

type positionMarkWire struct {
    Listing ... // listing identity or a stable key representation
    Price   num.Price
    AsOf    time.Time
}

and verify a snapshot with multiple listings/mark times survives write/read exactly.

4. Build the margin mark map only from current positions

I agree with Copilot's performance finding.

s.marks intentionally retains historical marks after positions close. Copying the entire map into margin.Marks on every snapshot makes margin-aware snapshot work grow with every listing ever traded rather than current exposure.

The shared margin calculation only needs marks for the current positions, so build the input map while iterating those positions.

That also has a nice correctness property: it makes explicit that historical/closed marks are display/history state, not inputs to current margin.

5. Minor semantic point: prefer the supplied observation timestamp for mark AsOf

Both ObserveMark(..., at time.Time) and AdvanceBar(..., at time.Time) receive the market observation timestamp, but the implementation stamps marks with deps.Clock.Now().

If the scheduler always advances the simulated clock to exactly at, these happen to match today. But the interface already carries the authoritative observation time, and PositionMark.AsOf is documented as when the price was observed.

I would prefer storing at for bar/observation-derived marks and the fill timestamp (or corresponding execution time) for fill-derived marks. That avoids coupling mark provenance to an implicit clock synchronization invariant.

I would treat this as a small design cleanup rather than a blocker if the scheduler invariant is explicitly guaranteed and tested.

What looks good

The core direction is solid:

• PositionMark is listing-scoped and preserves independent AsOf times;
• marks are optional in the generic account Snapshot but required by consumers that need valuation;
• MarginAvailable is allowed to go negative while BuyingPower floors at zero;
• no maintenance-margin liquidation is introduced;
• no configured margin model preserves the old simulator behavior;
• the configured calculation reuses internal/account/margin;
• account.ListingKey replaces the simulator's duplicate private identity type;
• model metadata makes the margin policy distinguishable for reproducibility.

One operational note: GitHub currently reports this PR as not mergeable at the current base/head, independently of the code-review findings.

Current state: changes requested before merge. The zero-price mutation paths and JSONL mark loss are the important correctness blockers; I would fix the historical-mark copy as part of the same pass.

[rustyeddy]

Addressed the review in c67b0c1 (replies are on each Copilot thread):

1. Zero fill price: rejected on the final execution price, before anything is committed; there's a regression test that the account is unchanged.
2. Zero ObserveMark / observation prices: rejected with ErrInvalidObservation before mutation, across ObserveMark, Advance, and AdvanceBar (positive Low).
3. JSONL marks: the journal now round-trips marks, with a test using two listings and different AsOf values. A mark naming no position is a corrupt entry.
4. Current-only marks: the margin map is built from open positions only.
5. Mark AsOf: I kept clock-derived times. sim.Deps.Clock's doc already records the rule that every timestamp the simulator produces comes from its clock, not from Observation.Time. Stamping marks from at would also let a mark's AsOf run ahead of the snapshot's clock-derived AsOf, which the snapshot rejects; several existing Advance tests pass a bar time ahead of the clock. Instead, as you suggested, the Scheduler invariant is now pinned: TestScheduler_ClockEqualsBarTimeWhenObservingMarks wraps the observer and advancer and asserts clock == bar time on every call. It's also documented on the simulator's mark type.

Coverage: sim 89.4%, account 100%. jsonl went from 80.8% on main to 81.4%; it was already below 85% before this PR. go test -race ./... passes and lint is clean. The mergeability warning was stale: GitHub shows MERGEABLE CLEAN.

[rustyeddy]

Re-review of PR #424 at head c67b0c1b318f8d38179fe7298c4ee5a950b22846.

The prior review blockers are addressed cleanly.

Verified fixes

1. Zero fill prices

   • the fill path now rejects a zero final execution price before state is committed;
   • the regression test verifies position/mark/order/AsOf state remains unchanged.

2. Zero observation marks

   • ObserveMark rejects zero before mutation;
   • the observation validation path also rejects invalid zero-price bar inputs before Advance / AdvanceBar can write marks.

3. JSONL mark preservation

   • accountWire now includes explicit markWire entries;
   • decode resolves each mark against the snapshot's decoded positions and treats an unknown mark/listing as a corrupt journal entry;
   • account.NewSnapshot receives the reconstructed Marks, so round-trip fidelity is preserved.

4. Margin snapshot scaling

   • marginFieldsLocked now builds the margin mark map from current open positions only, rather than copying the simulator's historical mark cache.

5. Mark timestamp semantics

   • the simulator deliberately keeps all produced timestamps clock-derived;
   • the scheduler invariant that clock time equals bar time when calling mark/intrabar hooks is now explicitly pinned by a test.
   • Given the simulator's existing timestamp ownership rule, I think this is a coherent choice.

Architecture check

The resulting structure remains consistent with ADR-066 and the milestone plan:

• snapshot marks are per listing and independently timestamped;
• missing marks remain explicit rather than falling back to AvgPrice;
• configured margin fields reuse the shared #421 calculation;
• MarginAvailable may be negative after drift while BuyingPower floors at zero;
• no maintenance-margin liquidation is introduced;
• unset margin config preserves legacy simulator behavior;
• account.ListingKey is reused consistently across snapshot/simulator/margin code;
• JSONL now preserves the new account state rather than silently dropping it.

GitHub now reports the PR as mergeable.

I do not see another substantive blocker in the latest diff.

PR #424 is merge-ready from my review.

GitHub currently shows no combined-status entries for this head, so this assessment is based on the latest code, tests represented in the PR, and review state rather than an external CI status signal.