M4-01: Verify execution/risk architecture and extract ADR-005/006 - #190
Merged
Conversation
Extracts ADR-005 (strategy emits intents, not broker orders) and ADR-006 (execution and risk are separate stages) from the consolidated decision registry into standalone files, following the exact convention M3-01 (#144/PR #159) established for ADR-007/008/017/018/019 -- editorial relocation only, no Context/Decision/Consequences/ Alternatives Considered text altered from what was accepted inline. Reconciles the intended M4 pipeline against the actual M3 public contracts now on main: order.Proposal and order.Request (ADR-017) already carry the pre-risk/post-risk vocabulary ADR-006's own Decision describes, and broker.Account.Submit (ADR-007/008) is already the terminal step an approved request reaches. Confirms dependency direction holds: no strategy, execution, or risk package exists yet, and nothing in order/account/broker imports any of them. package-boundaries.org's own Preliminary Package Map was missing execution and risk entirely (rules 3-6 already named them, but the map itself did not), a real documentation gap relative to the canonical architecture document's own dependency graph, which does include them. Fixed by adding both as siblings of strategy/broker/portfolio, depending only on order (and risk additionally on account/portfolio), with neither depending on strategy or broker -- matching ADR-005's own "strategy never imports execution/risk/broker" rule. The one open architectural question M4-01's own scope calls out -- whether risk policies are strictly approve/reject or may modify a proposal -- is not resolved here. It is already tracked as its own issue, #180 (M4-05: define risk evaluation contracts and decision model), whose own scope explicitly asks for a new ADR (likely ADR-029) once that issue is worked -- the same "point to the issue that will actually resolve it" pattern M3-01 used for its own one open question (broker event ordering, resolved by #147/M3-04) rather than deciding it prematurely here. Co-Authored-By: Claude Sonnet 5 <[email protected]> Claude-Session: https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9
Contributor
There was a problem hiding this comment.
🟡 Changes recommended
The updated package-boundary documentation introduces terminology/dependency clarity issues that should be resolved to keep the architecture docs internally consistent.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR updates the project’s architecture baseline for Milestone 4 by extracting ADR-005 and ADR-006 into standalone Org files and updating the ADR registry and package-boundary documentation to reflect the execution/risk separation.
Changes:
- Extracted ADR-005 and ADR-006 from
docs/arch/adr-decisions.orginto standalone ADR files following the established ADR file convention. - Updated
docs/arch/adr-decisions.orgto point ADR-005/006 to their new files and added an explanatory “extracted ADR” note. - Updated
docs/arch/package-boundaries.org’s preliminary package map and added text describing whereexecutionandriskfit in the dependency structure.
File summaries
| File | Description |
|---|---|
| docs/arch/package-boundaries.org | Updates the preliminary package map to include execution and risk, and adds an explanatory paragraph about the M4 pipeline/dependencies. |
| docs/arch/adr-decisions.org | Converts ADR-005/006 index entries to point to standalone ADR files and replaces inline ADR bodies with pointer text. |
| docs/arch/adr-005-strategy-emits-intents.org | New standalone ADR file for ADR-005 with Status preamble and the extracted decision text. |
| docs/arch/adr-006-execution-risk-separate-stages.org | New standalone ADR file for ADR-006 with Status preamble and the extracted decision text. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+51
to
+60
| =execution= and =risk= (M4, ADR-005/ADR-006) sit alongside =strategy=, | ||
| =broker=, and =portfolio= at this tier: both depend on =order= (the | ||
| =order.Proposal=/=order.Request= vocabulary ADR-017 already | ||
| established), and =risk= additionally depends on =account=/=portfolio= | ||
| to evaluate a proposal against current exposure. Neither depends on | ||
| =strategy= or =broker= — =strategy= never imports either (ADR-005), and | ||
| the chain from a strategy's own =Intent= through =execution= and =risk= | ||
| to an approved =order.Request= reaching =broker.Account.Submit= is | ||
| wired by application services, not by a direct import between any of | ||
| these sibling packages. |
Comment on lines
+57
to
+60
| the chain from a strategy's own =Intent= through =execution= and =risk= | ||
| to an approved =order.Request= reaching =broker.Account.Submit= is | ||
| wired by application services, not by a direct import between any of | ||
| these sibling packages. |
This was referenced Aug 25, 2026
rustyeddy
added a commit
that referenced
this pull request
Aug 25, 2026
Adds order.Intent, the broker-ignorant value a strategy or operator uses to express what it wants to accomplish before execution planning translates it into a concrete order.Proposal (ADR-005/ADR-006). Per the design discussion on issue #177: the canonical type lives in order, not a new strategy package. execution and risk already depend on order for the neighboring Proposal->Request->Order->Fill vocabulary (ADR-017); putting Intent in strategy instead would have forced execution to import strategy, directly reversing the "execution does not depend on strategy" boundary #176/PR #190 just established. strategy itself remains uncreated, deferred to M6. v0 ships exactly the four intent kinds ADR-005 itself names -- Enter, Exit, AdjustStop, TargetExposure -- with per-kind field requirements enforced by NewIntent: Enter carries only Side (sizing is risk's job, ADR-006); Exit carries neither Side nor Quantity (it means "remove whatever exposure exists," not a specific delta); AdjustStop carries an absolute StopPrice; TargetExposure carries the explicit desired Side+Quantity, the one kind where quantity genuinely originates from the strategy rather than from risk sizing. No CancelEntry or NoAction kind is added yet, per the same discussion. Intent identifies instrument.ID (the canonical economic identity, ADR-003), not a venue-specific instrument.Listing -- keeping one intent meaningful across every broker adapter; execution planning selects the concrete Listing as planning context. Adds id.IntentID, reinstating one of the three identifier kinds id/doc.go's own "Deferred identifiers" section explicitly earmarked for exactly this moment: Intent is now a concrete, persisted domain object to justify a generated identity. Intent also carries id.Metadata so the intent -> proposal -> risk decision -> request -> fill correlation chain ADR-005's own Consequences section names is anchored from the start. Fixes package-boundaries.org's dependency map, which an earlier version (from #176) had drawn with the canonical intent type living in strategy -- exactly the reversed-dependency mistake this issue's own design discussion caught before implementation. Claude-Session: https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9 Co-authored-by: Claude Sonnet 5 <[email protected]>
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 changed
Extracted ADR-005 (strategy emits intents, not broker orders) and
ADR-006 (execution and risk are separate stages) from
docs/arch/adr-decisions.orginto standalone.orgfiles, followingthe exact convention M3-01 (#144, PR #159) established:
docs/arch/adr-005-strategy-emits-intents.orgdocs/arch/adr-006-execution-risk-separate-stages.orgThe registry now carries short pointer entries for each, plus an
explanatory note (matching the ADR-007/008/017/018/019 pattern) and an
updated Decision Index table.
docs/arch/package-boundaries.org's own Preliminary Package Map wasmissing
executionandriskentirely — rules 3-6 already named them,but the map itself did not, a real gap relative to the canonical
architecture document's own dependency graph. Fixed by adding both as
siblings of
strategy/broker/portfolio, depending only onorder(and
riskadditionally onaccount/portfolio), with neitherdepending on
strategyorbroker— matching ADR-005's own "strategynever imports execution/risk/broker" rule.
Why
Issue #176 (M4-01) asks for M4's architecture baseline to be prepared
before execution/risk implementation begins: extract the accepted
decisions, reconcile them against the M3 contracts now on
main, andconfirm dependency direction.
Verification performed (no redesign)
Consequences/Alternatives Considered text was diffed against the
original inline section and is byte-identical (only the
**→*heading level changed, plus a new Status preamble noting the
extraction).
Intent -> Proposal -> RiskDecision -> OrderRequestpipeline against the actual M3 types:order.Proposaland
order.Request(ADR-017) already carry the pre-risk/post-riskvocabulary ADR-006 describes, and
broker.Account.Submit(ADR-007/008) is already the terminal step an approved request
reaches.
strategy,execution, orriskpackage exists yet, and nothing inorder/account/brokerimports any of them (
grepacross the module).one historical citation in
docs/milestones/m1-completion-review.orgcites the ADR number only, not its old inline location, so it needed
no update.
The one open architectural question named in #176's own scope
Whether risk policies are strictly approve/reject or may modify a
proposal is not resolved here. It is already tracked as its own
issue, #180 (M4-05: define risk evaluation contracts and decision
model), whose own scope explicitly asks for a new ADR (likely
ADR-029) once that issue is worked — the same "point to the issue that
will actually resolve it" pattern M3-01 used for its own one open
question (broker event ordering, resolved by #147/M3-04) rather than
deciding it prematurely here.
Test plan
go build ./...andgo vet ./...pass (docs-only change; no Gocode was touched, so no new tests apply)
gofmt -l .clean,go test ./...passes (unaffected)original inline section to confirm no wording was altered
Closes #176
🤖 Generated with Claude Code
https://claude.ai/code/session_014JmhCNQ3Nh3veVXzZifMa9