Skip to content

fix: stabilize public CLI bridge#19

Merged
hudsonaikins merged 4 commits into
mainfrom
codex/sdk-review-prep
Mar 8, 2026
Merged

fix: stabilize public CLI bridge#19
hudsonaikins merged 4 commits into
mainfrom
codex/sdk-review-prep

Conversation

@hudsonaikins
Copy link
Copy Markdown
Contributor

@hudsonaikins hudsonaikins commented Mar 7, 2026

Greptile Summary

This PR stabilizes the public CLI bridge used by the TypeScript TUI, moving neural/cli.py from 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 to uv.

Key changes:

  • CLI bridge hardening (neural/cli.py): exposes CLI_COMMANDS, moves deployment imports to lazy-loading functions (_safe_list_providers, _create_provider), and fixes the --json flag so it emits a machine-readable error envelope when no sub-command is provided (previously it printed human-readable argparse help to stdout, breaking the bridge contract).
  • Deployment sub-commands: deployments list, deployments status, deployments logs, and deployments stop are fully implemented and connected through _build_provider / _resolve_provider_name with sensible auto-discovery priority (daytona > docker > single-provider).
  • Test coverage (tests/test_cli.py): new tests for the JSON envelope contract, provider resolution, missing-subcommand error, and an integration subprocess test validating clean stderr.
  • Polymarket US data collection (neural/data_collection/polymarket_us.py): adds a polling DataSource with candle history, trade replay, and market events pagination.
  • Strategy refactor (neural/analysis/strategies/__init__.py, base.py): _strategy_presets() now uses direct local imports instead of calling __getattr__ explicitly; BaseStrategy adds StrategyConfig-based initialization.
  • CI migration: replaces pip/actions/setup-python with astral-sh/setup-uv@v7 and uv run throughout.

Confidence Score: 3/5

  • Mostly safe to merge; one concrete runtime crash risk in the new Polymarket data source and a minor display bug in doctor output should be addressed first.
  • The CLI bridge core is well-tested and the logic is sound. The _safe_list_providers lazy-import fix, the missing-subcommand JSON error path, and the deployment sub-commands all look correct. The score is held back by (1) a definite AttributeError crash in PolymarketUSMarketsSource.collect() when NormalizedMarket.metadata is None, and (2) a silent display gap in _format_doctor where docker module status is collected but never shown to users running neural doctor without --json.
  • neural/data_collection/polymarket_us.py (line 63 — None metadata dereference) and neural/cli.py (line 448 — docker missing from _format_doctor text output).

Important Files Changed

Filename Overview
neural/cli.py Central CLI bridge module: adds CLI_COMMANDS list, lazy-loads deployment imports, fixes --json flag for missing sub-commands, and adds deployment sub-commands (list, status, logs, stop). Minor: _format_doctor omits docker from human-readable output.
tests/test_cli.py New CLI bridge tests covering doctor, capabilities, paper order (mocked and real client), deployments list/stop, and the new missing-subcommand JSON error path. The real-client paper order test writes to .tmp-paper-cli without cleanup, which can cause cross-run state pollution.
neural/data_collection/polymarket_us.py New Polymarket US data source with polling, candle history, trade replay, and market events. The collect() async generator calls m.metadata.get(...) without guarding against metadata is None, which will raise AttributeError for any market with no metadata.
neural/analysis/strategies/base.py Adds Signal, Position, Strategy, BaseStrategy, and StrategyConfig with backward-compat properties. StrategyConfig is missing initial_capital, so BaseStrategy(config=...) always uses the hardcoded default of 1000.0.
neural/analysis/strategies/init.py Lazy-loading __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 in sys.modules keeps this inexpensive.
neural/trading/client.py Extends TradingClient with Polymarket US exchange support, async paper-order path, and a _run_coro_sync guard that raises a clear error when called inside a running event loop instead of silently deadlocking.
.github/workflows/ci.yml Migrates CI from pip/actions/setup-python to uv/astral-sh/setup-uv@v7 with built-in caching. All tool invocations now use uv 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
    end
Loading

Comments Outside Diff (3)

  1. neural/analysis/strategies/__init__.py, line 359-369 (link)

    create_strategy return type weakened from Strategy to object

    The 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 the Strategy protocol.

    Since all strategy classes in _strategy_presets() are concrete subclasses of Strategy, the return type can be tightened:

    (Import Strategy lazily inside the function body if needed to avoid a circular dependency.)

    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: neural/analysis/strategies/__init__.py
    Line: 359-369
    
    Comment:
    **`create_strategy` return type weakened from `Strategy` to `object`**
    
    The 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 the `Strategy` protocol.
    
    Since all strategy classes in `_strategy_presets()` are concrete subclasses of `Strategy`, the return type can be tightened:
    
    
    
    (Import `Strategy` lazily inside the function body if needed to avoid a circular dependency.)
    
    How can I resolve this? If you propose a fix, please make it concise.
  2. neural/data_collection/polymarket_us.py, line 63 (link)

    m.metadata None dereference in collect

    NormalizedMarket.metadata is typed as dict[str, Any] | None, so calling .get("raw", {}) directly will raise AttributeError: '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
    This is a comment left during a code review.
    Path: neural/data_collection/polymarket_us.py
    Line: 63
    
    Comment:
    **`m.metadata` None dereference in `collect`**
    
    `NormalizedMarket.metadata` is typed as `dict[str, Any] | None`, so calling `.get("raw", {})` directly will raise `AttributeError: 'NoneType' object has no attribute 'get'` whenever an adapter returns a market with no metadata attached. This silently breaks the async generator mid-stream.
    
    
    
    How can I resolve this? If you propose a fix, please make it concise.
  3. neural/analysis/strategies/base.py, line 443-460 (link)

    StrategyConfig cannot set initial_capital

    StrategyConfig covers all risk parameters except initial_capital. When a caller constructs BaseStrategy(config=my_config), the capital is always silently pinned to the Strategy default of 1000.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:

    @dataclass
    class StrategyConfig:
        initial_capital: float = 1000.0   # add this
        max_position_size: float = 0.1
        ...

    and propagating it in BaseStrategy.__init__:

    super().__init__(
        name=name,
        initial_capital=config.initial_capital,
        ...
    )
    Prompt To Fix With AI
    This is a comment left during a code review.
    Path: neural/analysis/strategies/base.py
    Line: 443-460
    
    Comment:
    **`StrategyConfig` cannot set `initial_capital`**
    
    `StrategyConfig` covers all risk parameters except `initial_capital`. When a caller constructs `BaseStrategy(config=my_config)`, the capital is always silently pinned to the `Strategy` default of `1000.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`:
    
    ```python
    @dataclass
    class StrategyConfig:
        initial_capital: float = 1000.0   # add this
        max_position_size: float = 0.1
        ...
    ```
    
    and propagating it in `BaseStrategy.__init__`:
    
    ```python
    super().__init__(
        name=name,
        initial_capital=config.initial_capital,
        ...
    )
    ```
    
    How can I resolve this? If you propose a fix, please make it concise.

Last reviewed commit: b7fff51

Greptile also left 1 inline comment on this PR.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented Mar 7, 2026

📚 Documentation Status

✅ Code changes detected

  • Docstring coverage checked
  • API documentation validation completed
    ✅ README changes detected

This comment is automatically generated by the documentation workflow.

Comment thread neural/cli.py
Comment thread README.md
Comment thread neural/cli.py
Comment thread neural/analysis/strategies/__init__.py
Comment thread neural/cli.py Outdated
Comment thread neural/cli.py
Comment thread tests/test_cli.py Outdated
Comment thread neural/cli.py
"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']}",
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@hudsonaikins hudsonaikins merged commit 3c4621c into main Mar 8, 2026
21 checks passed
@hudsonaikins hudsonaikins deleted the codex/sdk-review-prep branch March 8, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant