# 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