M3-15: Perform M3 broker simulation and account-state completion review - #175
Merged
Conversation
…w (M3-15) Verification-first review, following the M1/M2/M2.5/M2.6 completion reviews' own precedent: exercises the public broker port, the deterministic simulator, service/broker, and the cmd/trader broker CLI directly against this milestone's own review checklist rather than trusting prior PR descriptions. Confirms: all sixteen M3 issues closed with merged PRs; the public broker port stays narrow and broker-neutral; broker.Account/ account.Snapshot keep their operational-handle/immutable-state split; account/order remain free of any broker dependency (mechanically enforced); no hidden wall clock, randomness, global logger, or global config exists in any deterministic M3 code path; service/CLI boundaries follow ADR-022 with formatting kept transport-side; logging follows ADR-023 with a mechanically enforced attribute allowlist; no risk/strategy/execution/portfolio concern has leaked into M3; every M3-related ADR is Accepted; and make check passes. Found and fixed one concrete gap during the exercise: three broker CLI response paths (JSON accounts, table snapshot, table submit) and the noPriceSource/cliPriceSource value types had zero test coverage, leaving cmd/trader at 82.3%, below the project's 85% target. Closed with four new CLI subtests (brokervertical_test.go) and a new unit test file (brokerservice_test.go) rather than deferred; cmd/trader now measures 86.3%. Recommendation: M3 is ready to close; M4 (execution and risk) may begin. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9
Contributor
There was a problem hiding this comment.
🟢 Approval recommended
The changes are documentation and additional tests only, and the new tests align with existing CLI/service boundaries while improving coverage without altering production behavior.
Pull request overview
Adds a verification-first Milestone 3 (broker simulation + account state) completion review document and closes a concrete test coverage gap found during that exercise by expanding cmd/trader broker CLI tests.
Changes:
- Add
docs/milestones/m3-completion-review.orgdocumenting a checklist-driven M3 closeout verification against broker/service/CLI/simulator behavior and ADR constraints. - Extend
cmd/traderbroker CLI vertical-slice tests to cover previously untested formatter paths (JSON accounts, table snapshot, table submit). - Add focused unit tests for
noPriceSource,cliPriceSource, andresolveSubmitPriceSourcein the broker CLI composition layer.
File summaries
| File | Description |
|---|---|
| docs/milestones/m3-completion-review.org | New M3 closeout review doc capturing verification steps, findings, deferrals, and recommendation. |
| cmd/trader/brokervertical_test.go | Adds broker CLI subtests to exercise JSON/table response formatting paths previously unexecuted in tests. |
| cmd/trader/brokerservice_test.go | New unit tests for CLI price-source value types and submit price-source resolution logic. |
Review details
- Files reviewed: 3/3 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
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.
Summary
docs/milestones/m3-completion-review.org), following the M1/M2/M2.5/M2.6 completion reviews' own precedent: exercises the public broker port, the deterministic simulator,service/broker, and thecmd/traderbroker CLI directly against this milestone's own review checklist, rather than trusting prior PR descriptions.broker.Account/account.Snapshotkeep their operational-handle/immutable-state split;account/orderremain free of any broker dependency (mechanically enforced); no hidden wall clock/randomness/global logger/global config in any deterministic M3 path; service/CLI boundaries follow ADR-022 with formatting kept transport-side; logging follows ADR-023 with a mechanically enforced attribute allowlist; no risk/strategy/execution/portfolio concern has leaked into M3; every M3-related ADR is Accepted;make checkpasses.noPriceSource/cliPriceSourcevalue types had zero test coverage, leavingcmd/traderat 82.3% — below the project's 85% target. Closed with four new CLI subtests (brokervertical_test.go) and a new unit test file (brokerservice_test.go);cmd/tradernow measures 86.3%.Test plan
go build ./...,go vet ./...,gofmt -l .all cleanmake check(fmt-check, vet, lint, test, race) passes across the whole repositorycmd/tradercoverage confirmed at 86.3% (was 82.3%), all other M3 packages re-measured above the 85% target with no zero-coverage functionsCloses #158
🤖 Generated with Claude Code
https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9