feat(brokers): finish the port flip -- the default venue resolves through the registry (#524) - #582
Merged
Merged
Conversation
…Preview only (#524) The dual-shape probes the issue named are deleted, not dormant: - ConfirmFn takes Preview, not Preview | Mapping -- every broker the live path can construct answers preview_order in the port's type - _preview_book's dict arm is gone; the spread gate (#350) and the entry-override warning (#332) read Preview.detail alone, and garbage falls into the same fail-closed arm as an absent key - _read_preview/_interactive_confirm (keel/commands/confirm.py) lose the legacy Coinbase dict arm and the 'Coinbase order preview' header that existed only to name the shape's one producer - ExecutionResult.preview is Preview | None Tests: a dict preview now renders UNREADABLE and demands the typed phrase; _preview_book reads no book out of a dict; the agent gate tests drive Previews; the direct-dict warning fixtures build Previews.
_build_broker no longer constructs CoinbaseClient for the coinbase branch: every name, the default included, resolves through the keel.brokers entry points, and the CLI's per-venue knowledge is the TRANSPORT it hands the resolved adapter (CDP secrets into a RESTClient for coinbase; endpoint/ feed/keys for alpaca). An adapter that resolves but has no wiring is still refused by name; an unknown name still fails through the registry's LookupError listing what is installed. The flip surfaced and closed the last pre-port shape on the order path: get_order's consumers probed a dict, and a registry-resolved adapter would have crashed _upgrade_to_observed_economics AFTER placement. The executor and reconcile now read the port's OrderStatus, and the legacy client answers in it too. Consumers moved with it: - keel balance reads get_balances(); gather_holdings takes list[Balance] - list_products moved onto CoinbaseAdapter as a documented Coinbase-extra (the port's catalogue surface stays the per-product get_instrument); the legacy client's copy is deleted, not left beside it - market_feed annotates the port's Broker, not the legacy client Tests: the default-venue pin now asserts the CoinbaseAdapter and its RESTClient wiring; the registry-resolved coinbase adapter serves execute() end-to-end (balances -> instrument -> preview -> place -> bracket) against venue fixtures; the fake venue serves the executor's port reads and refuses preview as the port's honest exception.
… client (#524) The documented exception -- 'the only broker the live path constructs is CoinbaseClient, which has no capabilities() at all' -- is no longer true: every broker _build_broker can now construct is a registry-resolved adapter that answers capabilities(). The clause is deleted from BrokerCapabilities' field note, rail 19's comment in guards.py, agent _venue_schedule's docstring (whose fallback now serves paper's None broker and contract violators, not a pre-port client), and assets.py's VENUE constant. What each note still records, honestly: asset_classes remains unread by engine code (guards.check is broker-less by design and paper passes broker=None, so a capabilities gate cannot live there); the venue-session fallback stays for the paper cycle; VENUE stays a constant because the screen is repo-driven -- the capabilities().venue replacement needs a broker handle threaded into it, which is #202/#233 follow-on work. The default-venue pin now asserts the resolved adapter declares itself (venue=coinbase, session_bound=False), so the clause cannot quietly return.
…er (#524) The old comment justified dev-only with 'nothing constructs it -- _common.py still builds CoinbaseClient directly'. That reason died with the flip: the adapter is reachable, and selecting it is refused explicitly at arm time for lacking CLI credential wiring. The comment now states the decision: dev-only until credential wiring AND #233's capability-based venue visibility exist, because runtime availability without wiring ships pynacl to installs that still cannot select the venue. Nothing about what ships changes.
…ood refusal names why (#524)
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
1 participant
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.
Audit of #524's acceptance checkboxes, against
origin/mainbefore this PR_build_brokerresolves the venue throughload_broker/discover_brokerscoinbasebranch constructedCoinbaseClientdirectly; the registry path existed only for non-default nameskeel/commands/_common.py:185-195(direct construction),:199-201(registry, other names only)get_instrument(packages/keel-broker-api/keel_broker_api/port.py:132),get_balances -> list[Balance](port.py:130); executor reads them atkeel/execution/executor.py:393and:441OrderSpecvaluesMarketIOCByQuote/ByBase(executor.py:1271-1315), the bracket is already aBracketGTCOrderSpec (executor.py:1489-1513), placement viabroker.place_order(spec)(executor.py:694)ConfirmFndual type (executor.py:161),_preview_bookdict arm (executor.py:945-973),ExecutionResult.previewdual type (executor.py:144), confirm gate dict arm (keel/commands/confirm.py:96-122);place_result.get("success")was already gone (executor.py:704). Grandfather:packages/keel-broker-api/keel_broker_api/capabilities.py:30-40("nocapabilities()at all"),keel/execution/guards.py:843-845,keel/agent.py:913-915,keel/commands/assets.py:63-74_common.py:211-236; un-wired venue refused by name (_common.py:203-209); unknown name fails through the registry's LookupError listing what is installed (packages/keel-broker-api/keel_broker_api/registry.py:26-28); pinned bytests/test_paper_equities_profile.py(test_build_broker_refuses_a_venue_without_cli_wiring,test_build_broker_unknown_name_surfaces_the_entry_point_list)What this PR finishes
keel_broker_api.registry.load_broker; the CLI's per-venue knowledge is the transport it hands the resolved adapter (CDP secrets ->RESTClientfor coinbase; endpoint/feed/keys for alpaca). The directCoinbaseClientconstruction is deleted.ConfirmFntakesPreview;_preview_bookreadsPreview.detailonly; the confirm gate's legacy Coinbase dict arm (and its "Coinbase order preview" header) is gone, and a dict now renders UNREADABLE (fail-closed, typed phrase);ExecutionResult.previewisPreview | None. Thecapabilities()grandfather clause retired fromBrokerCapabilities, rail 19's comment,_venue_schedule, andassets.VENUE.get_order's consumers (_upgrade_to_observed_economics,execution/reconcile) still probed a dict, while every registry-resolved adapter answers the port'sOrderStatus— a registry-built coinbase broker would have raisedAttributeErrorafter placement. All consumers now readOrderStatus; the legacy client answers in it too.keel balancereadsget_balances()(gather_holdingstakeslist[Balance]);list_productsmoved ontoCoinbaseAdapteras a documented Coinbase-extra (the port's catalogue surface stays the per-productget_instrument— a ~900-row sweep is a discovery concern, not an order-path one), and the legacy client's copy is deleted rather than left beside it;market_feedannotates the port'sBroker.Robinhood / runtime availability — decision, stated
Robinhood stays dev-only; nothing about what ships changes. The flip makes the adapter reachable (installing it +
broker.name: robinhoodnow reaches_build_broker, which refuses explicitly for lacking CLI credential wiring — instead of the old world where nothing could even construct it). Runtime availability remains a real decision, not a side effect of the flip: it needs (a) credential wiring for its Ed25519 keys and (b) #233's capability-based venue visibility, so an install cannot silently widen what a deployment can select. Until then pynacl in every install buys nothing.pyproject.toml's dev-group comment now states this reasoning.What stays for #502 (not duplicated here)
The bracket is already an OrderSpec (
BracketGTC, since #569) — this PR builds nothing bracket-shaped. What remains #502's is the management policy: live stop wiring needs a cancel-and-replace decision for the native bracket, and a partial entry leaves the bracket oversized for what is held (detect-and-surface only, as the docstrings record). The roll-protocol hardening (#519) and stop-management behaviour are untouched — the full suite pins them and stays green.Test delta
12 test functions added, 11 removed (the five legacy-dict confirm-gate tests and superseded variants are gone; new: registry end-to-end proofs, dict-unreadable proof,
_preview_bookdeletion proof, adapterlist_productspin, capabilities-bearing default venue). Totals: 4301 passed, 3 skipped — identical to the origin/main baseline count.Gates:
uv run pytest -qgreen;uv run ruff check keel tests packagesclean;uv run ruff format --checkclean on all 23 touched files;uv run mypyclean (347 files).keel/commands/test_service_isolation.pyand the capabilities scanner tests are green.Closes #524