fix: operator approval workflow — add approval toggle, lockout CRUD, and PBT tests
- Add GET/PUT /api/admin/trading/approval-config endpoints - Add POST/DELETE /api/admin/trading/lockouts endpoints - Add useApprovalConfig, useUpdateApprovalConfig, useCreateLockout, useDeleteLockout hooks - Add Paper Order Approval toggle card with confirmation dialog - Add lockout creation form and delete button to Active Lockouts card - Add MSW handlers for all new endpoints - Add property-based tests for bug condition exploration and preservation
This commit is contained in:
Celes Renata
@@ -0,0 +1,211 @@
|
||||
# Operator Approval Workflow Bugfix Design
|
||||
|
||||
## Overview
|
||||
|
||||
The operator approval system is fully implemented in the backend (`services/risk/approval.py`, `services/adapters/broker_service.py`) but is effectively disconnected from the operator's control. The `requires_approval()` function always returns `False` in paper mode because `OperatorApproval.auto_approve_paper` defaults to `True` and the `config` JSONB column in `risk_configs` is initialized as `{}`, which means `PortfolioRiskConfig.from_db_json({})` always produces default values that auto-approve paper orders. Additionally, the lockout system lacks a creation endpoint and UI controls.
|
||||
|
||||
The fix connects the existing approval pipeline to the UI by: (1) adding a toggle to control `auto_approve_paper` in the risk config JSON, (2) adding a POST endpoint and UI form for manual symbol lockouts, and (3) adding a DELETE endpoint and UI control for early lockout removal.
|
||||
|
||||
## Glossary
|
||||
|
||||
- **Bug_Condition (C)**: The condition where `requires_approval()` returns `False` in paper mode because `auto_approve_paper` defaults to `True` and the risk config JSON never overrides it — causing all paper orders to bypass the approval gate silently
|
||||
- **Property (P)**: When the operator has disabled `auto_approve_paper` via the UI, `requires_approval()` returns `True` for paper mode, causing orders to be held for approval; when a manual lockout is created via the new endpoint, it appears in the Active Lockouts section
|
||||
- **Preservation**: Existing live-mode approval behavior, disabled-mode bypass, approval review/expiry workflows, risk engine rejection pipeline, and system-generated lockouts must remain unchanged
|
||||
- **`requires_approval()`**: Function in `services/risk/approval.py` that determines whether an order needs operator approval based on trading mode and config
|
||||
- **`process_order_job()`**: Function in `services/adapters/broker_service.py` that processes broker queue jobs, running risk evaluation and the approval gate
|
||||
- **`load_risk_config()`**: Function in `services/adapters/broker_service.py` that loads `PortfolioRiskConfig` from the `risk_configs.config` JSONB column
|
||||
- **`OperatorApproval`**: Pydantic model in `services/risk/engine.py` with `auto_approve_paper` (default `True`) and `require_approval_for_live` (default `True`)
|
||||
- **`risk_configs`**: PostgreSQL table (migration 010) storing the active risk configuration as a JSONB `config` column
|
||||
|
||||
## Bug Details
|
||||
|
||||
### Bug Condition
|
||||
|
||||
The bug manifests when the system is in paper trading mode (the default). The `requires_approval()` function returns `False` because `OperatorApproval.auto_approve_paper` defaults to `True`, and the `risk_configs.config` JSONB column is initialized as `{}` — meaning `PortfolioRiskConfig.from_db_json({})` always produces a config with `auto_approve_paper=True`. There is no UI control to set this field to `False`, so the approval gate in `process_order_job()` is never reached for paper orders.
|
||||
|
||||
A secondary condition is that the `symbol_lockouts` table has no POST endpoint, so manual lockouts cannot be created, and the Active Lockouts section on the Trading page is permanently empty unless internal processes create lockouts.
|
||||
|
||||
**Formal Specification:**
|
||||
```
|
||||
FUNCTION isBugCondition(input)
|
||||
INPUT: input of type { trading_mode: TradingMode, risk_config_json: dict }
|
||||
OUTPUT: boolean
|
||||
|
||||
config := PortfolioRiskConfig.from_db_json(risk_config_json)
|
||||
|
||||
RETURN (
|
||||
trading_mode == PAPER
|
||||
AND config.operator_approval.auto_approve_paper == True
|
||||
AND "operator_approval" NOT IN risk_config_json
|
||||
)
|
||||
OR (
|
||||
input.action == "create_manual_lockout"
|
||||
AND NOT exists(POST_endpoint_for_symbol_lockouts)
|
||||
)
|
||||
END FUNCTION
|
||||
```
|
||||
|
||||
### Examples
|
||||
|
||||
- **Paper mode, empty config JSON**: `risk_configs.config = {}`, `trading_mode = 'paper'` → `requires_approval()` returns `False` → orders bypass approval silently. **Expected**: operator should be able to toggle `auto_approve_paper` to `False` so `requires_approval()` returns `True`.
|
||||
- **Paper mode, config with operator_approval**: `risk_configs.config = {"operator_approval": {"auto_approve_paper": false}}` → `requires_approval()` returns `True` → orders held for approval. **Expected**: this is the correct behavior, but currently unreachable because no UI sets this field.
|
||||
- **Manual lockout attempt**: Operator wants to lock out AAPL for 60 minutes → no POST endpoint exists → lockout cannot be created. **Expected**: `POST /api/admin/trading/lockouts` creates a row in `symbol_lockouts`.
|
||||
- **Lockout removal**: Operator wants to cancel an active lockout early → no DELETE endpoint exists → lockout persists until expiry. **Expected**: `DELETE /api/admin/trading/lockouts/{id}` removes the lockout.
|
||||
|
||||
## Expected Behavior
|
||||
|
||||
### Preservation Requirements
|
||||
|
||||
**Unchanged Behaviors:**
|
||||
- Live mode with `require_approval_for_live=True` must continue to require operator approval for live orders
|
||||
- Disabled mode must continue to return `False` from `requires_approval()` (orders blocked upstream)
|
||||
- The existing `PUT /api/admin/trading/approvals/{id}` endpoint must continue to approve/reject pending approvals
|
||||
- The `expire_stale_approvals()` function must continue to mark expired approvals
|
||||
- Orders that fail risk checks must continue to be rejected before reaching the approval gate
|
||||
- System-generated lockouts (news-shock, cooldown) must continue to function unaffected by the new manual lockout feature
|
||||
- Mouse clicks, trading mode switches, risk tier changes, and all other Trading page controls must continue to work
|
||||
|
||||
**Scope:**
|
||||
All inputs that do NOT involve the new `auto_approve_paper` toggle, manual lockout creation, or lockout deletion should be completely unaffected by this fix. This includes:
|
||||
- Live mode approval behavior (already working)
|
||||
- Risk evaluation pipeline
|
||||
- Order idempotency and duplicate prevention
|
||||
- Position sync and broker account registration
|
||||
- All other Trading page controls (mode, risk tier, macro toggle, competitive toggle, paper reset)
|
||||
|
||||
## Hypothesized Root Cause
|
||||
|
||||
Based on the bug description and code analysis, the root causes are:
|
||||
|
||||
1. **Missing UI control for `auto_approve_paper`**: The `OperatorApproval.auto_approve_paper` field defaults to `True` in the Pydantic model. The `risk_configs.config` JSONB column is initialized as `{}` by the `set_trading_mode` endpoint. Since `PortfolioRiskConfig.from_db_json({})` fills all defaults, `auto_approve_paper` is always `True`. The Trading page has no toggle to update this field in the config JSON.
|
||||
|
||||
2. **`update_trading_config` does full JSON replace**: The existing `PUT /api/admin/trading/config` endpoint replaces the entire `config` JSONB column. A new UI control needs to read the current config, merge the `operator_approval` settings, and write back the full config — or a dedicated endpoint should handle the toggle.
|
||||
|
||||
3. **Missing POST endpoint for `symbol_lockouts`**: The `GET /api/admin/trading/lockouts` endpoint exists for reading active lockouts, but there is no `POST` endpoint to create a manual lockout entry. The table schema supports it (no constraints preventing manual entries), but the API route is missing.
|
||||
|
||||
4. **Missing DELETE endpoint for `symbol_lockouts`**: There is no endpoint to remove a lockout early. The only way a lockout expires is by time (`expires_at > NOW()`).
|
||||
|
||||
5. **Missing frontend form and controls**: The Trading page renders lockout data read-only and has no form for creating lockouts or toggling approval settings.
|
||||
|
||||
## Correctness Properties
|
||||
|
||||
Property 1: Bug Condition - Paper Mode Approval Toggle
|
||||
|
||||
_For any_ `PortfolioRiskConfig` where `trading_mode` is `PAPER` and `operator_approval.auto_approve_paper` is `False`, the `requires_approval()` function SHALL return `True`, causing orders to be held for operator approval.
|
||||
|
||||
**Validates: Requirements 2.1, 2.2**
|
||||
|
||||
Property 2: Preservation - Non-Paper-Toggle Approval Behavior
|
||||
|
||||
_For any_ `PortfolioRiskConfig` where `trading_mode` is `LIVE` or `DISABLED`, the `requires_approval()` function SHALL produce the same result as the original function: `True` when `trading_mode` is `LIVE` and `require_approval_for_live` is `True`, and `False` when `trading_mode` is `DISABLED`. This preserves all existing approval behavior for non-paper modes.
|
||||
|
||||
**Validates: Requirements 3.1, 3.2, 3.3, 3.4, 3.5**
|
||||
|
||||
## Fix Implementation
|
||||
|
||||
### Changes Required
|
||||
|
||||
Assuming our root cause analysis is correct:
|
||||
|
||||
**File**: `services/api/app.py`
|
||||
|
||||
**New Endpoints**:
|
||||
1. **`PUT /api/admin/trading/approval-config`**: Dedicated endpoint to update `operator_approval` settings in the risk config JSON. Reads the current config, merges the `operator_approval` sub-object, and writes back. Accepts `{ auto_approve_paper: bool }`.
|
||||
2. **`GET /api/admin/trading/approval-config`**: Returns the current `operator_approval` settings from the active risk config.
|
||||
3. **`POST /api/admin/trading/lockouts`**: Creates a manual lockout entry in `symbol_lockouts`. Accepts `{ ticker: string, reason: string, duration_minutes: int, lockout_type?: string }`. Computes `expires_at` from `NOW() + duration_minutes`.
|
||||
4. **`DELETE /api/admin/trading/lockouts/{lockout_id}`**: Deletes a lockout entry by ID, allowing early removal.
|
||||
|
||||
**File**: `frontend/src/api/hooks.ts`
|
||||
|
||||
**New Hooks**:
|
||||
1. **`useApprovalConfig()`**: Fetches current approval config via `GET /api/admin/trading/approval-config`.
|
||||
2. **`useUpdateApprovalConfig()`**: Mutation to update approval config via `PUT /api/admin/trading/approval-config`. Invalidates `['approval-config']` and `['trading-config']` query keys.
|
||||
3. **`useCreateLockout()`**: Mutation to create a manual lockout via `POST /api/admin/trading/lockouts`. Invalidates `['lockouts']` query key.
|
||||
4. **`useDeleteLockout()`**: Mutation to delete a lockout via `DELETE /api/admin/trading/lockouts/{id}`. Invalidates `['lockouts']` query key.
|
||||
|
||||
**File**: `frontend/src/pages/Trading.tsx`
|
||||
|
||||
**UI Changes**:
|
||||
1. **Approval Toggle Card**: New card between the existing controls and Pending Approvals section. Contains a toggle switch for `auto_approve_paper` with a confirmation dialog (since enabling approval requirements changes order flow). Shows current state and last-changed timestamp.
|
||||
2. **Lockout Creation Form**: Add a form to the Active Lockouts card with fields for ticker (text input), reason (text input), and duration in minutes (number input). Submit button calls `useCreateLockout()`.
|
||||
3. **Lockout Delete Button**: Add a "Remove" button to each lockout row that calls `useDeleteLockout()` with a confirmation step.
|
||||
|
||||
## Testing Strategy
|
||||
|
||||
### Validation Approach
|
||||
|
||||
The testing strategy follows a two-phase approach: first, surface counterexamples that demonstrate the bug on unfixed code, then verify the fix works correctly and preserves existing behavior.
|
||||
|
||||
### Exploratory Bug Condition Checking
|
||||
|
||||
**Goal**: Surface counterexamples that demonstrate the bug BEFORE implementing the fix. Confirm or refute the root cause analysis. If we refute, we will need to re-hypothesize.
|
||||
|
||||
**Test Plan**: Write tests that call `requires_approval()` with various `PortfolioRiskConfig` instances and trading modes. Run these tests on the UNFIXED code to observe that paper mode always returns `False` regardless of intent.
|
||||
|
||||
**Test Cases**:
|
||||
1. **Paper Mode Default Config**: Call `requires_approval(PortfolioRiskConfig())` with `PAPER` mode — will return `False` on unfixed code (demonstrates the bug)
|
||||
2. **Paper Mode Empty JSON**: Call `requires_approval(PortfolioRiskConfig.from_db_json({}))` with `PAPER` mode — will return `False` on unfixed code (demonstrates the root cause)
|
||||
3. **Missing Lockout POST**: Attempt `POST /api/admin/trading/lockouts` — will return 404/405 on unfixed code
|
||||
4. **Missing Lockout DELETE**: Attempt `DELETE /api/admin/trading/lockouts/{id}` — will return 404/405 on unfixed code
|
||||
|
||||
**Expected Counterexamples**:
|
||||
- `requires_approval()` returns `False` for paper mode even when the operator intends to require approval
|
||||
- Possible causes: `auto_approve_paper` defaults to `True` and config JSON never overrides it
|
||||
|
||||
### Fix Checking
|
||||
|
||||
**Goal**: Verify that for all inputs where the bug condition holds, the fixed function produces the expected behavior.
|
||||
|
||||
**Pseudocode:**
|
||||
```
|
||||
FOR ALL input WHERE isBugCondition(input) DO
|
||||
config := PortfolioRiskConfig with auto_approve_paper = False
|
||||
result := requires_approval(config, PAPER)
|
||||
ASSERT result == True
|
||||
END FOR
|
||||
```
|
||||
|
||||
### Preservation Checking
|
||||
|
||||
**Goal**: Verify that for all inputs where the bug condition does NOT hold, the fixed function produces the same result as the original function.
|
||||
|
||||
**Pseudocode:**
|
||||
```
|
||||
FOR ALL input WHERE NOT isBugCondition(input) DO
|
||||
ASSERT requires_approval_original(input) == requires_approval_fixed(input)
|
||||
END FOR
|
||||
```
|
||||
|
||||
**Testing Approach**: Property-based testing is recommended for preservation checking because:
|
||||
- It generates many test cases automatically across the input domain of (trading_mode, auto_approve_paper, require_approval_for_live) combinations
|
||||
- It catches edge cases that manual unit tests might miss (e.g., unusual config combinations)
|
||||
- It provides strong guarantees that behavior is unchanged for all non-buggy inputs
|
||||
|
||||
**Test Plan**: Observe behavior on UNFIXED code first for live mode and disabled mode, then write property-based tests capturing that behavior.
|
||||
|
||||
**Test Cases**:
|
||||
1. **Live Mode Preservation**: Verify `requires_approval()` returns `True` for live mode with `require_approval_for_live=True` — must be identical before and after fix
|
||||
2. **Disabled Mode Preservation**: Verify `requires_approval()` returns `False` for disabled mode — must be identical before and after fix
|
||||
3. **Approval Review Preservation**: Verify `review_approval()` continues to update approval status correctly
|
||||
4. **Expiry Preservation**: Verify `expire_stale_approvals()` continues to mark expired approvals
|
||||
5. **Risk Rejection Preservation**: Verify orders failing risk checks are still rejected before the approval gate
|
||||
|
||||
### Unit Tests
|
||||
|
||||
- Test `requires_approval()` with all combinations of trading mode and approval settings
|
||||
- Test new `POST /api/admin/trading/lockouts` endpoint creates valid lockout rows
|
||||
- Test new `DELETE /api/admin/trading/lockouts/{id}` endpoint removes lockouts
|
||||
- Test new `PUT /api/admin/trading/approval-config` endpoint updates config JSON correctly
|
||||
- Test edge cases: expired lockouts not returned, invalid ticker, zero/negative duration
|
||||
|
||||
### Property-Based Tests
|
||||
|
||||
- Generate random `PortfolioRiskConfig` instances with varying `auto_approve_paper` and `require_approval_for_live` values across all three trading modes, and verify `requires_approval()` returns the correct boolean for each combination
|
||||
- Generate random lockout parameters (ticker, duration, reason) and verify the POST endpoint creates valid entries that appear in GET results and respect expiry
|
||||
- Test preservation: for all non-paper-mode configs, verify `requires_approval()` behavior is identical to the original implementation
|
||||
|
||||
### Integration Tests
|
||||
|
||||
- Test full order flow: set `auto_approve_paper=False` via API → submit order job → verify approval request created → approve → verify order submitted to broker
|
||||
- Test lockout flow: create manual lockout via API → verify it appears in GET → delete it → verify it's gone
|
||||
- Test UI rendering: approval toggle card renders with correct state, lockout form submits correctly, delete button removes lockout
|
||||
Reference in New Issue
Block a user