fix: stabilize public CLI bridge#19
Merged
Merged
Conversation
📚 Documentation Status✅ Code changes detected
This comment is automatically generated by the documentation workflow. |
| "Tooling:", | ||
| f" uv={payload['tooling']['uv_available']} bun={payload['tooling']['bun_available']} daytona_cli={payload['tooling']['daytona_cli_available']}", | ||
| "Optional modules:", | ||
| f" kalshi_python={installed['kalshi_python']} textblob={installed['textblob']} vaderSentiment={installed['vaderSentiment']} daytona={installed['daytona']}", |
There was a problem hiding this comment.
docker module omitted from human-readable doctor output
_handle_doctor adds "docker": _module_available("docker") to installed_modules, but _format_doctor never renders it. Users running neural doctor (without --json) have no visibility into whether docker is available, even though it's a deployment provider dependency. This creates a gap between the machine-readable and human-readable outputs.
Suggested change
| f" kalshi_python={installed['kalshi_python']} textblob={installed['textblob']} vaderSentiment={installed['vaderSentiment']} daytona={installed['daytona']}", | |
| f" kalshi_python={installed['kalshi_python']} textblob={installed['textblob']} vaderSentiment={installed['vaderSentiment']} daytona={installed['daytona']} docker={installed['docker']}", |
Prompt To Fix With AI
This is a comment left during a code review.
Path: neural/cli.py
Line: 448
Comment:
**`docker` module omitted from human-readable doctor output**
`_handle_doctor` adds `"docker": _module_available("docker")` to `installed_modules`, but `_format_doctor` never renders it. Users running `neural doctor` (without `--json`) have no visibility into whether `docker` is available, even though it's a deployment provider dependency. This creates a gap between the machine-readable and human-readable outputs.
```suggestion
f" kalshi_python={installed['kalshi_python']} textblob={installed['textblob']} vaderSentiment={installed['vaderSentiment']} daytona={installed['daytona']} docker={installed['docker']}",
```
How can I resolve this? If you propose a fix, please make it concise.
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.
Greptile Summary
This PR stabilizes the public CLI bridge used by the TypeScript TUI, moving
neural/cli.pyfrom an unstable experimental state to a fully tested, machine-readable JSON surface. It also extends the trading and data-collection stack with Polymarket US support, refactors the analysis strategies module to use proper lazy imports, and migrates CI/CD touv.Key changes:
neural/cli.py): exposesCLI_COMMANDS, moves deployment imports to lazy-loading functions (_safe_list_providers,_create_provider), and fixes the--jsonflag so it emits a machine-readable error envelope when no sub-command is provided (previously it printed human-readableargparsehelp to stdout, breaking the bridge contract).deployments list,deployments status,deployments logs, anddeployments stopare fully implemented and connected through_build_provider/_resolve_provider_namewith sensible auto-discovery priority (daytona>docker> single-provider).tests/test_cli.py): new tests for the JSON envelope contract, provider resolution, missing-subcommand error, and an integration subprocess test validating clean stderr.neural/data_collection/polymarket_us.py): adds a pollingDataSourcewith candle history, trade replay, and market events pagination.neural/analysis/strategies/__init__.py,base.py):_strategy_presets()now uses direct local imports instead of calling__getattr__explicitly;BaseStrategyaddsStrategyConfig-based initialization.pip/actions/setup-pythonwithastral-sh/setup-uv@v7anduv runthroughout.Confidence Score: 3/5
_safe_list_providerslazy-import fix, the missing-subcommand JSON error path, and the deployment sub-commands all look correct. The score is held back by (1) a definiteAttributeErrorcrash inPolymarketUSMarketsSource.collect()whenNormalizedMarket.metadataisNone, and (2) a silent display gap in_format_doctorwheredockermodule status is collected but never shown to users runningneural doctorwithout--json.neural/data_collection/polymarket_us.py(line 63 —Nonemetadata dereference) andneural/cli.py(line 448 —dockermissing from_format_doctortext output).Important Files Changed
CLI_COMMANDSlist, lazy-loads deployment imports, fixes--jsonflag for missing sub-commands, and adds deployment sub-commands (list, status, logs, stop). Minor:_format_doctoromitsdockerfrom human-readable output..tmp-paper-cliwithout cleanup, which can cause cross-run state pollution.collect()async generator callsm.metadata.get(...)without guarding againstmetadata is None, which will raiseAttributeErrorfor any market with no metadata.Signal,Position,Strategy,BaseStrategy, andStrategyConfigwith backward-compat properties.StrategyConfigis missinginitial_capital, soBaseStrategy(config=...)always uses the hardcoded default of 1000.0.__getattr__pattern for all strategy classes._strategy_presets()now uses direct local imports (fixing the previous__getattr__call anti-pattern).create_strategy()calls_strategy_presets()on every invocation, but module-level caching insys.moduleskeeps this inexpensive.TradingClientwith Polymarket US exchange support, async paper-order path, and a_run_coro_syncguard that raises a clear error when called inside a running event loop instead of silently deadlocking.pip/actions/setup-pythontouv/astral-sh/setup-uv@v7with built-in caching. All tool invocations now useuv run. Clean migration.Sequence Diagram
sequenceDiagram participant TUI as TypeScript TUI participant CLI as neural CLI (cli.py) participant Providers as _safe_list_providers participant Deploy as DeploymentProvider participant Paper as PaperTradingClient participant Kalshi as KalshiHTTPClient TUI->>CLI: python -m neural --json <command> CLI->>CLI: parse_args() alt No sub-command handler CLI-->>TUI: {"ok": false, "error": {"code": "ValueError", ...}} else handler found alt deployments list/status/logs/stop CLI->>Providers: _safe_list_providers() Providers-->>CLI: ["daytona"] or [] alt No providers CLI-->>TUI: {"ok": false, "error": {"code": "RuntimeError", ...}} else Provider resolved CLI->>Deploy: asyncio.run(provider.list_deployments()) Deploy-->>CLI: [DeploymentInfo, ...] CLI-->>TUI: {"ok": true, "data": {"provider": "...", "deployments": [...]}} end else paper order CLI->>Paper: PaperTradingClient(...) CLI->>Paper: asyncio.run(place_order(...)) Paper-->>CLI: {success, order_id, ...} CLI-->>TUI: {"ok": true, "data": {"order": {...}, "portfolio": {...}}} else markets/quote/positions CLI->>Kalshi: KalshiHTTPClient() Kalshi-->>CLI: market data CLI-->>TUI: {"ok": true, "data": {...}} else doctor/capabilities/providers list CLI->>Providers: _safe_list_providers() Providers-->>CLI: [...] CLI-->>TUI: {"ok": true, "data": {...}} end endComments Outside Diff (3)
neural/analysis/strategies/__init__.py, line 359-369 (link)create_strategyreturn type weakened fromStrategytoobjectThe original signature was
def create_strategy(preset: str, **override_params) -> Strategy:. The new signature uses-> object, which is a public API type annotation regression. Any downstream caller relying on the return type for type-checking (e.g.,mypy, IDE autocompletion) will lose the guarantee that the returned value satisfies theStrategyprotocol.Since all strategy classes in
_strategy_presets()are concrete subclasses ofStrategy, the return type can be tightened:(Import
Strategylazily inside the function body if needed to avoid a circular dependency.)Prompt To Fix With AI
neural/data_collection/polymarket_us.py, line 63 (link)m.metadataNone dereference incollectNormalizedMarket.metadatais typed asdict[str, Any] | None, so calling.get("raw", {})directly will raiseAttributeError: 'NoneType' object has no attribute 'get'whenever an adapter returns a market with no metadata attached. This silently breaks the async generator mid-stream.Prompt To Fix With AI
neural/analysis/strategies/base.py, line 443-460 (link)StrategyConfigcannot setinitial_capitalStrategyConfigcovers all risk parameters exceptinitial_capital. When a caller constructsBaseStrategy(config=my_config), the capital is always silently pinned to theStrategydefault of1000.0. Users who build configs programmatically (e.g. from a YAML file) will never be able to control capital through the config object.Consider adding the field to
StrategyConfig:and propagating it in
BaseStrategy.__init__:Prompt To Fix With AI
Last reviewed commit: b7fff51