Commit Graph

13 Commits

  • fix: implement 'Allow this session' for apply_patch approvals (#8451)
    **Summary**
    This PR makes “ApprovalDecision::AcceptForSession / don’t ask again this
    session” actually work for `apply_patch` approvals by caching approvals
    based on absolute file paths in codex-core, properly wiring it through
    app-server v2, and exposing the choice in both TUI and TUI2.
    - This brings `apply_patch` calls to be at feature-parity with general
    shell commands, which also have a "Yes, and don't ask again" option.
    - This also fixes VSCE's "Allow this session" button to actually work.
    
    While we're at it, also split the app-server v2 protocol's
    `ApprovalDecision` enum so execpolicy amendments are only available for
    command execution approvals.
    
    **Key changes**
    - Core: per-session patch approval allowlist keyed by absolute file
    paths
    - Handles multi-file patches and renames/moves by recording both source
    and destination paths for `Update { move_path: Some(...) }`.
    - Extend the `Approvable` trait and `ApplyPatchRuntime` to work with
    multiple keys, because an `apply_patch` tool call can modify multiple
    files. For a request to be auto-approved, we will need to check that all
    file paths have been approved previously.
    - App-server v2: honor AcceptForSession for file changes
    - File-change approval responses now map AcceptForSession to
    ReviewDecision::ApprovedForSession (no longer downgraded to plain
    Approved).
    - Replace `ApprovalDecision` with two enums:
    `CommandExecutionApprovalDecision` and `FileChangeApprovalDecision`
    - TUI / TUI2: expose “don’t ask again for these files this session”
    - Patch approval overlays now include a third option (“Yes, and don’t
    ask again for these files this session (s)”).
        - Snapshot updates for the approval modal.
    
    **Tests added/updated**
    - Core:
    - Integration test that proves ApprovedForSession on a patch skips the
    next patch prompt for the same file
    - App-server:
    - v2 integration test verifying
    FileChangeApprovalDecision::AcceptForSession works properly
    
    **User-visible behavior**
    - When the user approves a patch “for session”, future patches touching
    only those previously approved file(s) will no longer prompt gain during
    that session (both via app-server v2 and TUI/TUI2).
    
    **Manual testing**
    Tested both TUI and TUI2 - see screenshots below.
    
    TUI:
    <img width="1082" height="355" alt="image"
    src="https://github.com/user-attachments/assets/adcf45ad-d428-498d-92fc-1a0a420878d9"
    />
    
    
    TUI2:
    <img width="1089" height="438" alt="image"
    src="https://github.com/user-attachments/assets/dd768b1a-2f5f-4bd6-98fd-e52c1d3abd9e"
    />
  • chore: unify conversation with thread name (#8830)
    Done and verified by Codex + refactor feature of RustRover
  • [app-server] fix config loading for conversations (#8765)
    Currently we don't load config properly for app server conversations.
    see:
    https://linear.app/openai/issue/CODEX-3956/config-flags-not-respected-in-codex-app-server.
    This PR fixes that by respecting the config passed in.
    
    Tested by running `cargo build -p codex-cli &&
    RUST_LOG=codex_app_server=debug CODEX_BIN=target/debug/codex cargo run
    -p codex-app-server-test-client -- \
    --config
    model_providers.mock_provider.base_url=\"http://localhost:4010/v2\" \
        --config model_provider=\"mock_provider\" \
        --config model_providers.mock_provider.name="hello" \
        send-message-v2 "hello"`
    and verified that the mock_provider is called instead of default
    provider.
    
    #closes
    https://linear.app/openai/issue/CODEX-3956/config-flags-not-respected-in-codex-app-server
    
    ---------
    
    Co-authored-by: Michael Bolin <mbolin@openai.com>
  • chore: add model/list call to app-server-test-client (#8331)
    Allows us to run `cargo run -p codex-app-server-test-client --
    model-list` to return the list of models over app-server.
  • Removed experimental "command risk assessment" feature (#7799)
    This experimental feature received lukewarm reception during internal
    testing. Removing from the code base.
  • updating app server types to support execpoilcy amendment (#7747)
    also includes minor refactor merging `ApprovalDecision` with
    `CommandExecutionRequestAcceptSettings`
  • fix: remove serde(flatten) annotation for TurnError (#7499)
    The problem with using `serde(flatten)` on Turn status is that it
    conditionally serializes the `error` field, which is not the pattern we
    want in API v2 where all fields on an object should always be returned.
    
    ```
    #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
    #[serde(rename_all = "camelCase")]
    #[ts(export_to = "v2/")]
    pub struct Turn {
        pub id: String,
        /// Only populated on a `thread/resume` response.
        /// For all other responses and notifications returning a Turn,
        /// the items field will be an empty list.
        pub items: Vec<ThreadItem>,
        #[serde(flatten)]
        pub status: TurnStatus,
    }
    
    #[derive(Serialize, Deserialize, Debug, Clone, PartialEq, JsonSchema, TS)]
    #[serde(tag = "status", rename_all = "camelCase")]
    #[ts(tag = "status", export_to = "v2/")]
    pub enum TurnStatus {
        Completed,
        Interrupted,
        Failed { error: TurnError },
        InProgress,
    }
    ```
    
    serializes to:
    ```
    {
      "id": "turn-123",
      "items": [],
      "status": "completed"
    }
    
    {
      "id": "turn-123",
      "items": [],
      "status": "failed",
      "error": {
        "message": "Tool timeout",
        "codexErrorInfo": null
      }
    }
    ```
    
    Instead we want:
    ```
    {
      "id": "turn-123",
      "items": [],
      "status": "completed",
      "error": null
    }
    
    {
      "id": "turn-123",
      "items": [],
      "status": "failed",
      "error": {
        "message": "Tool timeout",
        "codexErrorInfo": null
      }
    }
    ```
  • [app-server-test-client] add send-followup-v2 (#7271)
    Add a new endpoint that allows us to test multi-turn behavior.
    
    Tested with running:
    ```
    RUST_LOG=codex_app_server=debug CODEX_BIN=target/debug/codex \
          cargo run -p codex-app-server-test-client -- \
          send-follow-up-v2 "hello" "and now a follow-up question"
    ```
  • [app-server] feat: v2 apply_patch approval flow (#6760)
    This PR adds the API V2 version of the apply_patch approval flow, which
    centers around `ThreadItem::FileChange`.
    
    This PR wires the new RPC (`item/fileChange/requestApproval`, V2 only)
    and related events (`item/started`, `item/completed` for
    `ThreadItem::FileChange`, which are emitted in both V1 and V2) through
    the app-server
    protocol. The new approval RPC is only sent when the user initiates a
    turn with the new `turn/start` API so we don't break backwards
    compatibility with VSCE.
    
    Similar to https://github.com/openai/codex/pull/6758, the approach I
    took was to make as few changes to the Codex core as possible,
    leveraging existing `EventMsg` core events, and translating those in
    app-server. I did have to add a few additional fields to
    `EventMsg::PatchApplyBegin` and `EventMsg::PatchApplyEnd`, but those
    were fairly lightweight.
    
    However, the `EventMsg`s emitted by core are the following:
    ```
    1) Auto-approved (no request for approval)

    - EventMsg::PatchApplyBegin
    - EventMsg::PatchApplyEnd
    
    2) Approved by user
    - EventMsg::ApplyPatchApprovalRequest
    - EventMsg::PatchApplyBegin
    - EventMsg::PatchApplyEnd
    
    3) Declined by user
    - EventMsg::ApplyPatchApprovalRequest
    - EventMsg::PatchApplyBegin
    - EventMsg::PatchApplyEnd
    ```
    
    For a request triggering an approval, this would result in:
    ```
    item/fileChange/requestApproval
    item/started
    item/completed
    ```
    
    which is different from the `ThreadItem::CommandExecution` flow
    introduced in https://github.com/openai/codex/pull/6758, which does the
    below and is preferable:
    ```
    item/started
    item/commandExecution/requestApproval
    item/completed
    ```
    
    To fix this, we leverage `TurnSummaryStore` on codex_message_processor
    to store a little bit of state, allowing us to fire `item/started` and
    `item/fileChange/requestApproval` whenever we receive the underlying
    `EventMsg::ApplyPatchApprovalRequest`, and no-oping when we receive the
    `EventMsg::PatchApplyBegin` later.
    
    This is much less invasive than modifying the order of EventMsg within
    core (I tried).
    
    The resulting payloads:
    ```
    {
      "method": "item/started",
      "params": {
        "item": {
          "changes": [
            {
              "diff": "Hello from Codex!\n",
              "kind": "add",
              "path": "/Users/owen/repos/codex/codex-rs/APPROVAL_DEMO.txt"
            }
          ],
          "id": "call_Nxnwj7B3YXigfV6Mwh03d686",
          "status": "inProgress",
          "type": "fileChange"
        }
      }
    }
    ```
    
    ```
    {
      "id": 0,
      "method": "item/fileChange/requestApproval",
      "params": {
        "grantRoot": null,
        "itemId": "call_Nxnwj7B3YXigfV6Mwh03d686",
        "reason": null,
        "threadId": "019a9e11-8295-7883-a283-779e06502c6f",
        "turnId": "1"
      }
    }
    ```
    
    ```
    {
      "id": 0,
      "result": {
        "decision": "accept"
      }
    }
    ```
    
    ```
    {
      "method": "item/completed",
      "params": {
        "item": {
          "changes": [
            {
              "diff": "Hello from Codex!\n",
              "kind": "add",
              "path": "/Users/owen/repos/codex/codex-rs/APPROVAL_DEMO.txt"
            }
          ],
          "id": "call_Nxnwj7B3YXigfV6Mwh03d686",
          "status": "completed",
          "type": "fileChange"
        }
      }
    }
    ```
  • [app-server] introduce turn/completed v2 event (#6800)
    similar to logic in
    `codex/codex-rs/exec/src/event_processor_with_jsonl_output.rs`.
    translation of v1 -> v2 events:
    `codex/event/task_complete` -> `turn/completed`
    `codex/event/turn_aborted` -> `turn/completed` with `interrupted` status
    `codex/event/error` -> `turn/completed` with `error` status
    
    this PR also makes `items` field in `Turn` optional. For now, we only
    populate it when we resume a thread, and leave it as None for all other
    places until we properly rewrite core to keep track of items.
    
    tested using the codex app server client. example new event:
    ```
    < {
    <   "method": "turn/completed",
    <   "params": {
    <     "turn": {
    <       "id": "0",
    <       "items": [],
    <       "status": "interrupted"
    <     }
    <   }
    < }
    ```
  • feat: add app-server-test-client crate for internal use (#5391)
    For app-server development it's been helpful to be able to trigger some
    test flows end-to-end and print the JSON-RPC messages sent between
    client and server.