From f842fdf6184c1a0f7568bb13fed3ffdaa92a8959 Mon Sep 17 00:00:00 2001 From: PJ Doland Date: Mon, 22 Jun 2026 15:37:05 -0400 Subject: [PATCH 1/4] feat(codex): experimental Codex agent mode via the Agent Client Protocol (#378) Add an opt-in Codex (OpenAI) agent mode that drives the chat panel over the Agent Client Protocol (ACP), the way Claude mode drives it today: streaming replies, tool-call cards with diffs, and per-tool approval. An NBI MCP tool is invoked end to end to prove the path. - ACP client backend (acp_agent.py) on a persistent worker thread, mapping ACP events onto NBI's chat surfaces, with single-flight turns and a hardened subprocess lifecycle. A small stdio MCP server (acp_mcp_server.py) exposes an NBI tool to the agent. - codex_settings + a codex_mode admin policy that defaults to force-off, plus OPENAI_API_KEY / OPENAI_BASE_URL / NBI_CODEX_CHAT_MODEL overrides, following the existing seven-place policy and settings-override conventions. - Frontend: a Codex settings tab (enable, model, key, base URL) mirroring the Claude tab, and chat-panel awareness so the input and routing follow Codex. - An in-chat agent picker (shown when more than one agent mode is enabled) that resolves a single active agent from the enabled modes and a persisted preference; with no preference the historical Claude-wins default holds. The picker is a custom icon dropdown, and the agent footer is unified (Ask/Agent and tools stay on the native model path; agent modes show only their badge). - Consistent branding: the OpenAI mark for the Codex participant and tab. - Admin gating parity with Claude: a codex_full_access policy (default force-off) pins Codex to approval_policy=untrusted so it asks before risky actions, clamped server-side; user-choice exposes a "Full access" toggle that flips the launch to approval_policy=never. README and admin-guide document the controls, the CODEX_HOME isolation, and the residual precedence caveat. Unit tests cover the ACP-to-NBI mapping, approval, the policy clamps, the active-agent resolution, the approval-arg mapping, and the credential scrub. The live path was verified in a running JupyterLab. --- README.md | 40 +- docs/admin-guide.md | 119 ++-- notebook_intelligence/acp_agent.py | 623 +++++++++++++++++++ notebook_intelligence/acp_mcp_server.py | 89 +++ notebook_intelligence/ai_service_manager.py | 86 ++- notebook_intelligence/config.py | 11 + notebook_intelligence/extension.py | 115 +++- notebook_intelligence/feature_flags.py | 27 + pyproject.toml | 5 + src/api.ts | 37 +- src/chat-sidebar.tsx | 153 +++-- src/components/agent-select.tsx | 162 +++++ src/components/checkbox.tsx | 1 + src/components/settings-panel.tsx | 201 +++++- src/index.ts | 1 + style/base.css | 102 +++ tests/test_acp_agent.py | 284 +++++++++ tests/test_ai_service_manager_integration.py | 29 + tests/test_cell_output_features_response.py | 45 ++ 19 files changed, 1982 insertions(+), 148 deletions(-) create mode 100644 notebook_intelligence/acp_agent.py create mode 100644 notebook_intelligence/acp_mcp_server.py create mode 100644 src/components/agent-select.tsx create mode 100644 tests/test_acp_agent.py diff --git a/README.md b/README.md index e2c69dbb..faf6b923 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ Bypass Permissions never persists: starting a **New chat session** (or `/clear`) Choosing **Bypass Permissions** does not arm it immediately. It opens a confirmation step; only after you confirm does bypass take effect, and while it is active the shield turns into a red warning icon as a persistent indicator. Bypass must be re-armed each session: starting a new chat or restarting the Claude client drops back to Default. And because the server re-checks the requested mode on every message, an armed bypass can never outlive a policy that an administrator has since turned off. -**Admin gating.** Bypass Permissions is **off by default** and hidden from the selector unless an administrator enables it: it is governed by the `claude_bypass_permissions` policy (`NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY`), the only admin policy whose default is `force-off` rather than `user-choice`. The requested mode is also clamped on the server for every message, so the gate can't be bypassed by a hand-crafted request. Independently, NBI honors Claude Code's enterprise [managed settings](https://code.claude.com/docs/en/settings): `permissions.disableBypassPermissionsMode` removes the option regardless of the NBI policy, and `permissions.defaultMode` sets the selector's starting mode (Bypass excepted, since it never auto-arms). See [Allowing Bypass Permissions](docs/admin-guide.md#allowing-bypass-permissions-in-the-claude-permission-mode-selector) in the admin guide. +**Admin gating.** Bypass Permissions is **off by default** and hidden from the selector unless an administrator enables it: it is governed by the `claude_bypass_permissions` policy (`NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY`), one of three admin policies that default to `force-off` rather than `user-choice` (the others gate the experimental Codex mode and Codex full access). The requested mode is also clamped on the server for every message, so the gate can't be bypassed by a hand-crafted request. Independently, NBI honors Claude Code's enterprise [managed settings](https://code.claude.com/docs/en/settings): `permissions.disableBypassPermissionsMode` removes the option regardless of the NBI policy, and `permissions.defaultMode` sets the selector's starting mode (Bypass excepted, since it never auto-arms). See [Allowing Bypass Permissions](docs/admin-guide.md#allowing-bypass-permissions-in-the-claude-permission-mode-selector) in the admin guide. The `/enter-plan-mode` and `/exit-plan-mode` slash commands still work if typed but are no longer offered in autocomplete; the selector replaces them and will retire the commands in a future release. @@ -196,24 +196,26 @@ Most settings panel toggles can be locked by org administrators. Two shapes: **Boolean policies** use the `*_POLICY` suffix and accept three values: `user-choice` (default — user toggles freely), `force-on` (locked enabled), `force-off` (locked disabled). When forced, the panel control is disabled with a "Locked by your administrator" tooltip and any client-side write is ignored. -| Env var | Locks the Settings panel control for | -| ---------------------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `NBI_EXPLAIN_ERROR_POLICY` | "Explain cell errors" | -| `NBI_OUTPUT_FOLLOWUP_POLICY` | "Ask about cell outputs" | -| `NBI_OUTPUT_TOOLBAR_POLICY` | "Show output toolbar" | -| `NBI_CLAUDE_MODE_POLICY` | "Enable Claude mode" | -| `NBI_CLAUDE_CONTINUE_CONVERSATION_POLICY` | "Remember conversation history" | -| `NBI_CLAUDE_CODE_TOOLS_POLICY` | "Claude Code tools" | -| `NBI_CLAUDE_JUPYTER_UI_TOOLS_POLICY` | "Jupyter UI tools" | -| `NBI_CLAUDE_SETTING_SOURCE_USER_POLICY` | Setting source: User | -| `NBI_CLAUDE_SETTING_SOURCE_PROJECT_POLICY` | Setting source: Project | -| `NBI_STORE_GITHUB_ACCESS_TOKEN_POLICY` | "Remember my GitHub Copilot access token" | -| `NBI_SKILLS_MANAGEMENT_POLICY` | The Skills tab (force-off hides it and 403s the API; also disables the managed-skills reconciler) | -| `NBI_CLAUDE_MCP_MANAGEMENT_POLICY` | The Claude-mode MCP Servers tab (force-off hides it and 403s `/claude-mcp/*`; independent of the non-Claude MCP Servers tab) | -| `NBI_CLAUDE_PLUGINS_MANAGEMENT_POLICY` | The Claude-mode Plugins tab (force-off hides it and 403s `/plugins/*`) | -| `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY` | "Bypass Permissions" in the Claude permission-mode selector (defaults to `force-off`, the only policy that does; `user-choice` exposes the option, which the user still arms per session) | -| `NBI_TERMINAL_DRAG_DROP_POLICY` | Terminal drag-drop file attach feature | -| `NBI_REFRESH_OPEN_FILES_ON_DISK_CHANGE_POLICY` | "Refresh open files when changed on disk" | +| Env var | Locks the Settings panel control for | +| ---------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `NBI_EXPLAIN_ERROR_POLICY` | "Explain cell errors" | +| `NBI_OUTPUT_FOLLOWUP_POLICY` | "Ask about cell outputs" | +| `NBI_OUTPUT_TOOLBAR_POLICY` | "Show output toolbar" | +| `NBI_CLAUDE_MODE_POLICY` | "Enable Claude mode" | +| `NBI_CODEX_MODE_POLICY` | "Enable Codex mode" (experimental ACP agent mode; defaults to `force-off`, set `user-choice` to allow it) | +| `NBI_CODEX_FULL_ACCESS_POLICY` | "Full access" for Codex: run tools without asking (defaults to `force-off`, so Codex asks before anything beyond trusted read-only commands; `user-choice` lets users opt into unattended runs) | +| `NBI_CLAUDE_CONTINUE_CONVERSATION_POLICY` | "Remember conversation history" | +| `NBI_CLAUDE_CODE_TOOLS_POLICY` | "Claude Code tools" | +| `NBI_CLAUDE_JUPYTER_UI_TOOLS_POLICY` | "Jupyter UI tools" | +| `NBI_CLAUDE_SETTING_SOURCE_USER_POLICY` | Setting source: User | +| `NBI_CLAUDE_SETTING_SOURCE_PROJECT_POLICY` | Setting source: Project | +| `NBI_STORE_GITHUB_ACCESS_TOKEN_POLICY` | "Remember my GitHub Copilot access token" | +| `NBI_SKILLS_MANAGEMENT_POLICY` | The Skills tab (force-off hides it and 403s the API; also disables the managed-skills reconciler) | +| `NBI_CLAUDE_MCP_MANAGEMENT_POLICY` | The Claude-mode MCP Servers tab (force-off hides it and 403s `/claude-mcp/*`; independent of the non-Claude MCP Servers tab) | +| `NBI_CLAUDE_PLUGINS_MANAGEMENT_POLICY` | The Claude-mode Plugins tab (force-off hides it and 403s `/plugins/*`) | +| `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY` | "Bypass Permissions" in the Claude permission-mode selector (defaults to `force-off`, as do `NBI_CODEX_MODE_POLICY` and `NBI_CODEX_FULL_ACCESS_POLICY`; `user-choice` exposes the option, which the user still arms per session) | +| `NBI_TERMINAL_DRAG_DROP_POLICY` | Terminal drag-drop file attach feature | +| `NBI_REFRESH_OPEN_FILES_ON_DISK_CHANGE_POLICY` | "Refresh open files when changed on disk" | The first three also have matching traitlets on `NotebookIntelligence` (`explain_error_policy`, `output_followup_policy`, `output_toolbar_policy`); add the others as needed in the same shape: diff --git a/docs/admin-guide.md b/docs/admin-guide.md index d8ef6e16..b744ea1f 100644 --- a/docs/admin-guide.md +++ b/docs/admin-guide.md @@ -75,57 +75,57 @@ If users share a home directory across nodes (NFS-backed shared HPC, classroom l The full surface, in one table. -| Name | Type | Default | Source | Purpose | -| ------------------------------------------------ | ---- | --------------------------- | ---------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `disabled_providers` | List | `[]` | traitlet on `NotebookIntelligence` | Hide providers from the user dropdown. Values: `github-copilot`, `ollama`, `litellm-compatible`, `openai-compatible`. | -| `allow_enabling_providers_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_PROVIDERS` re-enables hidden providers per pod. | -| `NBI_ENABLED_PROVIDERS` | csv | unset | env | Comma-separated provider IDs to re-enable. Effective only when `allow_enabling_providers_with_env=True`. | -| `disabled_tools` | List | `[]` | traitlet | Hide built-in tools from agent mode. Values listed in [Restricting features](#restricting-features-for-managed-deployments). | -| `allow_enabling_tools_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_BUILTIN_TOOLS` re-enables hidden tools per pod. | -| `NBI_ENABLED_BUILTIN_TOOLS` | csv | unset | env | Comma-separated tool IDs to re-enable. Effective only when `allow_enabling_tools_with_env=True`. | -| `disabled_coding_agent_launchers` | List | `[]` | traitlet | Hide JupyterLab launcher tiles for coding-agent CLIs even when the CLI is on `PATH`. Valid IDs: `claude-code`, `opencode`, `pi`, `github-copilot-cli`, `codex`. See [Disabling coding-agent launcher tiles](#disabling-coding-agent-launcher-tiles). | -| `allow_enabling_coding_agent_launchers_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_CODING_AGENT_LAUNCHERS` re-enables hidden tiles per pod. | -| `NBI_ENABLED_CODING_AGENT_LAUNCHERS` | csv | unset | env | Comma-separated launcher IDs to re-enable. Effective only when `allow_enabling_coding_agent_launchers_with_env=True`. | -| `enable_chat_feedback` | Bool | `False` | traitlet | Enables thumbs-up/down UI in chat and emits in-process `telemetry` events. | -| `enable_chat_feedback_always_visible` | Bool | `False` | traitlet | Renders thumbs-up/down buttons at full opacity on every assistant reply instead of revealing them only on hover. Requires `enable_chat_feedback=True`. | -| `additional_skipped_workspace_directories` | List | `[]` | traitlet | Extra directory names to skip in the chat-sidebar @-mention workspace file picker. Merged with the built-in skips (`__pycache__`, `node_modules`). Match is by directory name only, case-sensitive. | -| `NBI_ADDITIONAL_SKIPPED_WORKSPACE_DIRECTORIES` | csv | unset | env (appends to traitlet) | Comma-separated extra directory names. Resolved at server startup and concatenated with the traitlet value, so a spawn profile can add to (rather than replace) the org-wide list. | -| `allow_github_skill_import` | Bool | `True` | traitlet | When `False`, hides the **Import from GitHub** button in the Skills panel and rejects `/skills/import` POSTs with 403. Does not affect the managed-skills reconciler. | -| `NBI_ALLOW_GITHUB_SKILL_IMPORT` | bool | unset | env (overrides traitlet) | Per-pod override for `allow_github_skill_import`. Accepts `true`/`false`/`1`/`0`/`yes`/`no`/`on`/`off` (case-insensitive). Useful for varying the policy across spawn profiles. | -| `skills_manifest` | str | `""` | traitlet | URL or filesystem path to a managed-skills manifest, or a comma-separated list of either. Manifests are unioned with first-wins URL dedupe; name collisions surface as per-entry errors. See [`docs/skills.md`](skills.md#managed-skills-via-an-org-manifest). | -| `NBI_SKILLS_MANIFEST` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `skills_manifest_interval` | int | `86400` | traitlet | Seconds between reconciles. | -| `NBI_SKILLS_MANIFEST_INTERVAL` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `managed_skills_token` | str | `""` | traitlet | Bearer token for managed-skills GitHub fetches. | -| `NBI_MANAGED_SKILLS_TOKEN` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `allow_github_plugin_import` | Bool | `True` | traitlet | When `False`, hides the "From GitHub" affordance in the Plugins panel and rejects `claude plugin marketplace add` requests whose source resolves as a GitHub URL or `owner/repo` shorthand. Local-path and arbitrary-URL sources remain available. | -| `NBI_ALLOW_GITHUB_PLUGIN_IMPORT` | bool | unset | env (overrides traitlet) | Per-pod override for `allow_github_plugin_import`. Accepts `true`/`false`/`1`/`0`/`yes`/`no`/`on`/`off` (case-insensitive). | -| `skill_max_archive_mb` | Int | `100` | traitlet | Per-archive on-wire size cap (megabytes) for skill bundles fetched from GitHub. Applies to both user imports and managed-skills tarballs. `0` disables the cap. | -| `NBI_SKILL_MAX_ARCHIVE_MB` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `upload_max_mb` | Int | `50` | traitlet | Per-file size cap (megabytes) for the shared upload endpoint used by chat-sidebar attachments and terminal drag-drop. Requests over the cap return HTTP 413. `0` disables the cap. | -| `NBI_UPLOAD_MAX_MB` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `upload_retention_hours` | Int | `24` | traitlet | How long staged uploads survive in the temp directory before the next upload sweeps them. `0` keeps only the atexit purge (uploads survive the session). | -| `NBI_UPLOAD_RETENTION_HOURS` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `mcp_stdio_command_allowlist` | List | `[]` | traitlet | Regex allowlist for the stdio MCP server `command` field. Empty list (default) means no enforcement; non-empty list rejects any stdio MCP server whose command does not match. Matched with `re.search`; anchor (`^...$`) for literal equality. Applies at both Claude `mcp add` and `mcp.json` load. See [Restricting MCP stdio commands](#restricting-mcp-stdio-commands). | -| `NBI_MCP_STDIO_COMMAND_ALLOWLIST` | csv | unset | env (appends to traitlet) | Comma-separated regex patterns added to the traitlet at startup. Per-pod additions on an org baseline. | -| `tour_config_path` | str | `""` | traitlet | Filesystem path to a YAML/JSON file with admin overrides for the first-run sidebar tour copy. See [`docs/admin-tour-config.md`](admin-tour-config.md). | -| `NBI_TOUR_CONFIG_PATH` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `NBI_GH_ACCESS_TOKEN_PASSWORD` | str | `nbi-access-token-password` | env | Password used to encrypt the stored Copilot token in `user-data.json`. **Change in multi-tenant deployments.** | -| `NBI_REFUSE_DEFAULT_TOKEN_PASSWORD_ON_SHARED_FS` | bool | unset | env | When set, refuse to write `user-data.json` if the default `NBI_GH_ACCESS_TOKEN_PASSWORD` is still in use AND `~/.jupyter/nbi/` is readable by group or other. Opt-in to preserve backwards compatibility on single-user deployments where the directory mode is incidental. | -| `NBI_ALLOW_DEFAULT_TOKEN_PASSWORD` | bool | unset | env | Per-pod opt-out that disengages the refuse-on-shared-fs guard above. Admins who knowingly accept the risk (e.g., during a transition before rolling out a per-user password) set this so writes continue. | -| `NBI_RULES_AUTO_RELOAD` | bool | `true` | env | When `false`, ruleset edits require a JupyterLab restart to take effect. | -| `NBI_CLAUDE_CLI_PATH` | str | unset | env | Absolute path to the Claude Code CLI binary. When unset, NBI looks up `claude` on `PATH`. | -| `NBI_OPENCODE_CLI_PATH` | str | unset | env | Absolute path to the opencode CLI. When unset, NBI looks up `opencode` on `PATH`. Gates the opencode launcher tile. | -| `NBI_PI_CLI_PATH` | str | unset | env | Absolute path to the Pi CLI. When unset, NBI looks up `pi` on `PATH`. Gates the Pi launcher tile. | -| `NBI_GITHUB_COPILOT_CLI_PATH` | str | unset | env | Absolute path to the GitHub Copilot CLI. When unset, NBI looks up `copilot` on `PATH`. Gates the GitHub Copilot launcher tile. | -| `NBI_CODEX_CLI_PATH` | str | unset | env | Absolute path to the OpenAI Codex CLI. When unset, NBI looks up `codex` on `PATH`. Gates the Codex launcher tile. | -| `NBI_GHE_SUBDOMAIN` | str | `""` | env | GitHub Enterprise subdomain for GitHub Copilot users on a GHE tenant. Empty selects github.com. | -| `NBI_GITHUB_ENTERPRISE_HOSTS` | csv | `""` | env | Comma-separated hostnames the plugin marketplace detector treats as GitHub. Cookie-domain shape: bare token (`github.acme.com`) matches exactly; leading-dot token (`.acme.com`) matches any subdomain of `acme.com`. Independent of `NBI_GHE_SUBDOMAIN`, which only configures the Copilot OAuth tenant. Required so `allow_github_plugin_import = False` actually gates GHE marketplace adds and so the `GITHUB_TOKEN` / `gh auth token` chain injects on GHE sources. | -| `NBI_LOG_LEVEL` | str | `INFO` | env | Python logging level for the `notebook_intelligence` logger. | -| `LITELLM_LOCAL_MODEL_COST_MAP` | bool | `true` (NBI default) | env | litellm setting that NBI defaults to `true` when it loads litellm, so litellm reads the model-cost map bundled with the installed package instead of fetching it over HTTP (litellm's own default), which stalls on proxied networks. Set to `false` before starting JupyterLab to restore litellm's remote fetch. | -| `NBI_DISABLE_OUTPUT_SCRUB` | bool | unset | env | When set (`1` / `true` / `yes` / `on`), disables the shell-tool output scrubber so raw stdout/stderr (including any env-var values that leak) is sent through to chat. Default off; the scrubber redacts values for sensitive-named env vars (`TOKEN`, `SECRET`, `API_KEY`, ...) plus tokens with well-known credential prefixes (`ghp_`, `sk-ant-`, `AKIA`, ...). Opt out only when debugging credential helpers where the redaction interferes. | -| `GITHUB_TOKEN`, `GH_TOKEN` | str | unset | env | Used (in that order) by user-initiated skill imports and GitHub-sourced plugin marketplace adds for GitHub auth. Falls back to `gh` CLI auth. | -| `NBI_*_POLICY` | str | `user-choice` | env | Lock individual Settings panel toggles. See [README → Admin policies](../README.md#admin-policies) for the full list of `*_POLICY` env vars and matching traitlets, including `NBI_SKILLS_MANAGEMENT_POLICY`, `NBI_CLAUDE_MCP_MANAGEMENT_POLICY`, `NBI_CLAUDE_PLUGINS_MANAGEMENT_POLICY`, `NBI_TERMINAL_DRAG_DROP_POLICY`, and `NBI_REFRESH_OPEN_FILES_ON_DISK_CHANGE_POLICY`. One exception to the `user-choice` default: `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY` defaults to `force-off`; see [Allowing Bypass Permissions](#allowing-bypass-permissions-in-the-claude-permission-mode-selector). | +| Name | Type | Default | Source | Purpose | +| ------------------------------------------------ | ---- | --------------------------- | ---------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `disabled_providers` | List | `[]` | traitlet on `NotebookIntelligence` | Hide providers from the user dropdown. Values: `github-copilot`, `ollama`, `litellm-compatible`, `openai-compatible`. | +| `allow_enabling_providers_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_PROVIDERS` re-enables hidden providers per pod. | +| `NBI_ENABLED_PROVIDERS` | csv | unset | env | Comma-separated provider IDs to re-enable. Effective only when `allow_enabling_providers_with_env=True`. | +| `disabled_tools` | List | `[]` | traitlet | Hide built-in tools from agent mode. Values listed in [Restricting features](#restricting-features-for-managed-deployments). | +| `allow_enabling_tools_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_BUILTIN_TOOLS` re-enables hidden tools per pod. | +| `NBI_ENABLED_BUILTIN_TOOLS` | csv | unset | env | Comma-separated tool IDs to re-enable. Effective only when `allow_enabling_tools_with_env=True`. | +| `disabled_coding_agent_launchers` | List | `[]` | traitlet | Hide JupyterLab launcher tiles for coding-agent CLIs even when the CLI is on `PATH`. Valid IDs: `claude-code`, `opencode`, `pi`, `github-copilot-cli`, `codex`. See [Disabling coding-agent launcher tiles](#disabling-coding-agent-launcher-tiles). | +| `allow_enabling_coding_agent_launchers_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_CODING_AGENT_LAUNCHERS` re-enables hidden tiles per pod. | +| `NBI_ENABLED_CODING_AGENT_LAUNCHERS` | csv | unset | env | Comma-separated launcher IDs to re-enable. Effective only when `allow_enabling_coding_agent_launchers_with_env=True`. | +| `enable_chat_feedback` | Bool | `False` | traitlet | Enables thumbs-up/down UI in chat and emits in-process `telemetry` events. | +| `enable_chat_feedback_always_visible` | Bool | `False` | traitlet | Renders thumbs-up/down buttons at full opacity on every assistant reply instead of revealing them only on hover. Requires `enable_chat_feedback=True`. | +| `additional_skipped_workspace_directories` | List | `[]` | traitlet | Extra directory names to skip in the chat-sidebar @-mention workspace file picker. Merged with the built-in skips (`__pycache__`, `node_modules`). Match is by directory name only, case-sensitive. | +| `NBI_ADDITIONAL_SKIPPED_WORKSPACE_DIRECTORIES` | csv | unset | env (appends to traitlet) | Comma-separated extra directory names. Resolved at server startup and concatenated with the traitlet value, so a spawn profile can add to (rather than replace) the org-wide list. | +| `allow_github_skill_import` | Bool | `True` | traitlet | When `False`, hides the **Import from GitHub** button in the Skills panel and rejects `/skills/import` POSTs with 403. Does not affect the managed-skills reconciler. | +| `NBI_ALLOW_GITHUB_SKILL_IMPORT` | bool | unset | env (overrides traitlet) | Per-pod override for `allow_github_skill_import`. Accepts `true`/`false`/`1`/`0`/`yes`/`no`/`on`/`off` (case-insensitive). Useful for varying the policy across spawn profiles. | +| `skills_manifest` | str | `""` | traitlet | URL or filesystem path to a managed-skills manifest, or a comma-separated list of either. Manifests are unioned with first-wins URL dedupe; name collisions surface as per-entry errors. See [`docs/skills.md`](skills.md#managed-skills-via-an-org-manifest). | +| `NBI_SKILLS_MANIFEST` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `skills_manifest_interval` | int | `86400` | traitlet | Seconds between reconciles. | +| `NBI_SKILLS_MANIFEST_INTERVAL` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `managed_skills_token` | str | `""` | traitlet | Bearer token for managed-skills GitHub fetches. | +| `NBI_MANAGED_SKILLS_TOKEN` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `allow_github_plugin_import` | Bool | `True` | traitlet | When `False`, hides the "From GitHub" affordance in the Plugins panel and rejects `claude plugin marketplace add` requests whose source resolves as a GitHub URL or `owner/repo` shorthand. Local-path and arbitrary-URL sources remain available. | +| `NBI_ALLOW_GITHUB_PLUGIN_IMPORT` | bool | unset | env (overrides traitlet) | Per-pod override for `allow_github_plugin_import`. Accepts `true`/`false`/`1`/`0`/`yes`/`no`/`on`/`off` (case-insensitive). | +| `skill_max_archive_mb` | Int | `100` | traitlet | Per-archive on-wire size cap (megabytes) for skill bundles fetched from GitHub. Applies to both user imports and managed-skills tarballs. `0` disables the cap. | +| `NBI_SKILL_MAX_ARCHIVE_MB` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `upload_max_mb` | Int | `50` | traitlet | Per-file size cap (megabytes) for the shared upload endpoint used by chat-sidebar attachments and terminal drag-drop. Requests over the cap return HTTP 413. `0` disables the cap. | +| `NBI_UPLOAD_MAX_MB` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `upload_retention_hours` | Int | `24` | traitlet | How long staged uploads survive in the temp directory before the next upload sweeps them. `0` keeps only the atexit purge (uploads survive the session). | +| `NBI_UPLOAD_RETENTION_HOURS` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `mcp_stdio_command_allowlist` | List | `[]` | traitlet | Regex allowlist for the stdio MCP server `command` field. Empty list (default) means no enforcement; non-empty list rejects any stdio MCP server whose command does not match. Matched with `re.search`; anchor (`^...$`) for literal equality. Applies at both Claude `mcp add` and `mcp.json` load. See [Restricting MCP stdio commands](#restricting-mcp-stdio-commands). | +| `NBI_MCP_STDIO_COMMAND_ALLOWLIST` | csv | unset | env (appends to traitlet) | Comma-separated regex patterns added to the traitlet at startup. Per-pod additions on an org baseline. | +| `tour_config_path` | str | `""` | traitlet | Filesystem path to a YAML/JSON file with admin overrides for the first-run sidebar tour copy. See [`docs/admin-tour-config.md`](admin-tour-config.md). | +| `NBI_TOUR_CONFIG_PATH` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `NBI_GH_ACCESS_TOKEN_PASSWORD` | str | `nbi-access-token-password` | env | Password used to encrypt the stored Copilot token in `user-data.json`. **Change in multi-tenant deployments.** | +| `NBI_REFUSE_DEFAULT_TOKEN_PASSWORD_ON_SHARED_FS` | bool | unset | env | When set, refuse to write `user-data.json` if the default `NBI_GH_ACCESS_TOKEN_PASSWORD` is still in use AND `~/.jupyter/nbi/` is readable by group or other. Opt-in to preserve backwards compatibility on single-user deployments where the directory mode is incidental. | +| `NBI_ALLOW_DEFAULT_TOKEN_PASSWORD` | bool | unset | env | Per-pod opt-out that disengages the refuse-on-shared-fs guard above. Admins who knowingly accept the risk (e.g., during a transition before rolling out a per-user password) set this so writes continue. | +| `NBI_RULES_AUTO_RELOAD` | bool | `true` | env | When `false`, ruleset edits require a JupyterLab restart to take effect. | +| `NBI_CLAUDE_CLI_PATH` | str | unset | env | Absolute path to the Claude Code CLI binary. When unset, NBI looks up `claude` on `PATH`. | +| `NBI_OPENCODE_CLI_PATH` | str | unset | env | Absolute path to the opencode CLI. When unset, NBI looks up `opencode` on `PATH`. Gates the opencode launcher tile. | +| `NBI_PI_CLI_PATH` | str | unset | env | Absolute path to the Pi CLI. When unset, NBI looks up `pi` on `PATH`. Gates the Pi launcher tile. | +| `NBI_GITHUB_COPILOT_CLI_PATH` | str | unset | env | Absolute path to the GitHub Copilot CLI. When unset, NBI looks up `copilot` on `PATH`. Gates the GitHub Copilot launcher tile. | +| `NBI_CODEX_CLI_PATH` | str | unset | env | Absolute path to the OpenAI Codex CLI. When unset, NBI looks up `codex` on `PATH`. Gates the Codex launcher tile. | +| `NBI_GHE_SUBDOMAIN` | str | `""` | env | GitHub Enterprise subdomain for GitHub Copilot users on a GHE tenant. Empty selects github.com. | +| `NBI_GITHUB_ENTERPRISE_HOSTS` | csv | `""` | env | Comma-separated hostnames the plugin marketplace detector treats as GitHub. Cookie-domain shape: bare token (`github.acme.com`) matches exactly; leading-dot token (`.acme.com`) matches any subdomain of `acme.com`. Independent of `NBI_GHE_SUBDOMAIN`, which only configures the Copilot OAuth tenant. Required so `allow_github_plugin_import = False` actually gates GHE marketplace adds and so the `GITHUB_TOKEN` / `gh auth token` chain injects on GHE sources. | +| `NBI_LOG_LEVEL` | str | `INFO` | env | Python logging level for the `notebook_intelligence` logger. | +| `LITELLM_LOCAL_MODEL_COST_MAP` | bool | `true` (NBI default) | env | litellm setting that NBI defaults to `true` when it loads litellm, so litellm reads the model-cost map bundled with the installed package instead of fetching it over HTTP (litellm's own default), which stalls on proxied networks. Set to `false` before starting JupyterLab to restore litellm's remote fetch. | +| `NBI_DISABLE_OUTPUT_SCRUB` | bool | unset | env | When set (`1` / `true` / `yes` / `on`), disables the shell-tool output scrubber so raw stdout/stderr (including any env-var values that leak) is sent through to chat. Default off; the scrubber redacts values for sensitive-named env vars (`TOKEN`, `SECRET`, `API_KEY`, ...) plus tokens with well-known credential prefixes (`ghp_`, `sk-ant-`, `AKIA`, ...). Opt out only when debugging credential helpers where the redaction interferes. | +| `GITHUB_TOKEN`, `GH_TOKEN` | str | unset | env | Used (in that order) by user-initiated skill imports and GitHub-sourced plugin marketplace adds for GitHub auth. Falls back to `gh` CLI auth. | +| `NBI_*_POLICY` | str | `user-choice` | env | Lock individual Settings panel toggles. See [README → Admin policies](../README.md#admin-policies) for the full list of `*_POLICY` env vars and matching traitlets, including `NBI_SKILLS_MANAGEMENT_POLICY`, `NBI_CLAUDE_MCP_MANAGEMENT_POLICY`, `NBI_CLAUDE_PLUGINS_MANAGEMENT_POLICY`, `NBI_TERMINAL_DRAG_DROP_POLICY`, and `NBI_REFRESH_OPEN_FILES_ON_DISK_CHANGE_POLICY`. Three exceptions to the `user-choice` default all default to `force-off`: `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY` (see [Allowing Bypass Permissions](#allowing-bypass-permissions-in-the-claude-permission-mode-selector)), `NBI_CODEX_MODE_POLICY` (the experimental Codex agent mode), and `NBI_CODEX_FULL_ACCESS_POLICY` (Codex running tools without asking). | Configure traitlets in `jupyter_server_config.py`: @@ -569,10 +569,25 @@ c.NotebookIntelligence.claude_bypass_permissions_policy = "user-choice" Or via env: `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY=user-choice`. -The Claude permission-mode selector (Default / Accept Edits / Plan) always offers its three normal modes; all three keep NBI's tool-call confirmation flow. "Bypass Permissions" removes that confirmation entirely and runs every tool call with the Jupyter user's full access, so it is **disabled by default**: this is the only policy whose default is `force-off` rather than `user-choice`. Setting `user-choice` exposes the option, which the user must still arm explicitly per session through a confirm step; the armed state never persists across sessions or reconnects. `force-on` grants the same availability as `user-choice` and never auto-arms bypass. +The Claude permission-mode selector (Default / Accept Edits / Plan) always offers its three normal modes; all three keep NBI's tool-call confirmation flow. "Bypass Permissions" removes that confirmation entirely and runs every tool call with the Jupyter user's full access, so it is **disabled by default**: this is one of three policies whose default is `force-off` rather than `user-choice` (the others gate the experimental Codex agent mode and Codex full access, #378). Setting `user-choice` exposes the option, which the user must still arm explicitly per session through a confirm step; the armed state never persists across sessions or reconnects. `force-on` grants the same availability as `user-choice` and never auto-arms bypass. Independent of this policy, NBI refuses bypass whenever Claude Code's enterprise [managed settings](https://docs.anthropic.com/en/docs/claude-code/settings) set `permissions.disableBypassPermissionsMode`, and honors a managed `permissions.defaultMode` as the selector's starting mode (except bypass, which never starts armed). The clamp is enforced server-side at the request boundary, not just in the UI. +### Gating the experimental Codex agent (#378) + +Codex mode runs an external agent (the codex-acp adapter) over the Agent Client Protocol. The admin controls that apply: + +- **`NBI_CODEX_MODE_POLICY`** (default `force-off`): whether Codex mode can be enabled at all. While `force-off`, the Codex settings tab is hidden and `enabled` is clamped to false on every config write, so it is the nuclear off switch. +- **`NBI_CODEX_FULL_ACCESS_POLICY`** (default `force-off`): whether Codex may run tools without asking. While `force-off`, NBI pins the agent to `approval_policy = untrusted`, so it asks before anything beyond trusted read-only commands and each request surfaces through NBI's per-tool confirmation. `user-choice` exposes the "Full access" toggle in the Codex tab; `force-on` requires unattended runs. Like bypass, the value is clamped server-side, so a hand-crafted config write can't relax it. +- **Credentials and model** follow the same env-override locks as Claude: `OPENAI_API_KEY`, `OPENAI_BASE_URL`, and `NBI_CODEX_CHAT_MODEL` pin and hide the corresponding fields. + +How the pin is applied: NBI passes `approval_policy` to codex via the codex-acp binary's `-c key=value` command-line override, which takes precedence over the codex config file (`$CODEX_HOME/config.toml`). When an API key is configured, NBI also runs codex with an isolated `CODEX_HOME` under the NBI user directory, so the codex configuration base is NBI-controlled and neither the workspace's nor the user's `~/.codex` config is read (this isolates configuration only; the agent still operates on workspace files, subject to the approval pin). With ChatGPT (OAuth) auth, codex uses the user's own `~/.codex`; the `-c` pin still overrides its top-level `approval_policy`, but a user who controls that home directory is also the account the agent runs as. On shared or locked-down deployments, prefer API-key auth (so `CODEX_HOME` is isolated) and keep `NBI_CODEX_MODE_POLICY=force-off` unless Codex is explicitly needed. + +Known limitations: + +- NBI gates Codex through the approval posture above and through its per-tool confirmation, but it does not enforce a tool allowlist inside the agent the way Claude's tool policies do. The agent owns its own tool loop and NBI only mediates the approvals the agent chooses to request. +- The pin relies on codex honoring the command-line `-c approval_policy` override above any configuration it loads. NBI does not parse or strip codex's own config; if a future codex version applies a higher-precedence per-project setting, that assumption would need revisiting. Keep `NBI_CODEX_MODE_POLICY=force-off` where these constraints are not acceptable. + ### Disabling terminal drag-drop file attach ```python diff --git a/notebook_intelligence/acp_agent.py b/notebook_intelligence/acp_agent.py new file mode 100644 index 00000000..1b833797 --- /dev/null +++ b/notebook_intelligence/acp_agent.py @@ -0,0 +1,623 @@ +# Copyright (c) Mehmet Bektas + +"""Codex agent mode over the Agent Client Protocol (issue #378, Phase 1). + +A second, off-by-default agent mode that drives the chat panel like Claude mode +for the core loop (streaming, tool-call cards with diffs, per-tool approval), +backed by ``codex-acp`` over ACP instead of the Claude SDK. + +Design notes (validated by the Phase 0 spike under ``spikes/acp-codex/``): + +- The ACP client runs on its own worker thread with an asyncio loop, mirroring + ``ClaudeCodeClient``. Per-request ``query`` blocks on that loop via + ``run_coroutine_threadsafe`` while tool-call cards stream and ``request_permission`` + is answered cross-thread through ``wait_for_chat_user_input`` (the same poll a + signal pattern Claude uses). +- ``agent-client-protocol`` is imported here, and this module is only imported + when Codex mode is enabled, so the dependency loads lazily (matching #370). +- Phase 1 is deliberately allowed to duplicate a little of Claude's card/diff + wiring; Phase 2 extracts the shared layer. +""" + +import asyncio +import base64 +import concurrent.futures +import difflib +import logging +import os +import sys +import threading +import time +from typing import Any, Optional + +import acp +from acp import schema + +from notebook_intelligence.api import ( + ChatCommand, + ChatRequest, + ChatResponse, + Host, + MarkdownData, + ProgressData, + ToolCallData, + ConfirmationData, +) +from notebook_intelligence.base_chat_participant import BaseChatParticipant +from notebook_intelligence.util import ThreadSafeWebSocketConnector, get_jupyter_root_dir + +log = logging.getLogger(__name__) + +CODEX_AGENT_CHAT_PARTICIPANT_ID = "codex" +# Pinned in Phase 1 (the version the spike validated); revisit per release. +CODEX_ACP_PACKAGE = "@zed-industries/codex-acp@0.16.0" +_START_TIMEOUT = 60 # seconds to bring up codex-acp + a session +_RESPONSE_TIMEOUT = 30 * 60 # a turn can be long; cap so the UI doesn't hang forever +_POLL = 0.2 + +# The OpenAI mark, matching style/icons/openai.svg. Rendered as an in the +# chat (the response avatar), so the fill is pinned to OpenAI blue rather than +# currentColor, mirroring how the Claude participant pins Anthropic orange. +_CODEX_ICON_SVG = ( + '' + '' +) +CODEX_AGENT_ICON_URL = ( + "data:image/svg+xml;base64," + + base64.b64encode(_CODEX_ICON_SVG.encode("utf-8")).decode("utf-8") +) + +_MAX_DIFF_LINES = 60 + + +def _diff_lines(old: str, new: str, max_lines: int = _MAX_DIFF_LINES) -> tuple[list[dict], bool]: + """Line-level diff as typed lines for a tool-call card. + + Duplicated (minimally) from claude.py to keep this module independent of + the Claude SDK import; the shared version is a Phase 2 extraction. + """ + lines: list[dict] = [] + for raw in difflib.unified_diff( + (old or "").splitlines(), (new or "").splitlines(), lineterm="", n=1 + ): + if raw.startswith(("---", "+++", "@@")): + continue + if raw.startswith("+"): + lines.append({"type": "add", "content": raw[1:]}) + elif raw.startswith("-"): + lines.append({"type": "remove", "content": raw[1:]}) + else: + lines.append({"type": "context", "content": raw[1:] if raw[:1] == " " else raw}) + truncated = len(lines) > max(0, max_lines) + return lines[: max(0, max_lines)], truncated + + +# ACP tool kinds are finer than NBI's card-icon categories; collapse them. +_ACP_KIND_TO_NBI = { + "read": "read", "search": "read", "fetch": "read", + "edit": "edit", "delete": "edit", "move": "edit", + "execute": "execute", +} + + +def _nbi_kind(acp_kind: Optional[str]) -> str: + return _ACP_KIND_TO_NBI.get(acp_kind or "", "other") + + +def _nbi_status(acp_status: Optional[str]) -> str: + if acp_status in ("completed", "failed"): + return acp_status + return "in_progress" # pending / in_progress / None + + +def resolve_codex_acp_command() -> list[str]: + """The command that launches codex-acp. + + ``NBI_CODEX_ACP_COMMAND`` overrides (shell-split); otherwise run the pinned + package via ``npx``. Kept separate from the Claude CLI resolver because + codex-acp is an npm package, not a binary on PATH. + """ + override = os.environ.get("NBI_CODEX_ACP_COMMAND", "").strip() + if override: + import shlex + return shlex.split(override) + return ["npx", "-y", CODEX_ACP_PACKAGE] + + +def codex_approval_args(full_access: bool) -> list[str]: + """Codex config overrides that pin its approval posture. + + Default (``full_access`` off) forces ``approval_policy = untrusted`` so + Codex asks before anything beyond trusted read-only commands, surfacing the + request through NBI's per-tool confirmation. ``full_access`` (gated by the + force-off ``codex_full_access`` admin policy) lets it run unattended. The + flag is honored by the codex-acp binary's ``-c key=value`` override, so it + works for both API-key and ChatGPT-auth sessions. + + The override takes precedence over the codex config file. In the API-key + path NBI also isolates ``CODEX_HOME`` (see ``_child_env``), so the config + base is NBI-controlled and neither the workspace nor the user's ~/.codex is + read. With ChatGPT auth, codex uses the user's own ~/.codex; the ``-c`` + pin still overrides its top-level approval_policy. See the admin guide for + the residual caveat on shared deployments. + """ + policy = "never" if full_access else "untrusted" + return ["-c", f'approval_policy="{policy}"'] + + +class _NbiAcpClient(acp.Client): + """The editor side of ACP. Maps agent events onto NBI's chat surfaces.""" + + def __init__(self, owner: "AcpAgentClient"): + self._owner = owner + self._tool_state: dict[str, dict] = {} + + @property + def _response(self) -> Optional[ChatResponse]: + return self._owner.current_response + + async def session_update(self, session_id, update, **kw): + resp = self._response + if resp is None: + return + su = getattr(update, "session_update", None) + if su in ("tool_call", "tool_call_update"): + self._emit_tool_call(resp, update) + elif su == "agent_message_chunk": + text = _block_text(getattr(update, "content", None)) + if text: + resp.stream(MarkdownData(content=text)) + elif su == "agent_thought_chunk": + text = _block_text(getattr(update, "content", None)) + if text: + resp.stream(MarkdownData(reasoning_content=text)) + elif su == "available_commands_update": + self._owner.available_commands = [ + getattr(c, "name", "") for c in (update.available_commands or []) + ] + + def _emit_tool_call(self, resp: ChatResponse, update): + tid = update.tool_call_id + state = self._tool_state.setdefault( + tid, {"kind": "other", "title": "", "status": "in_progress", "diffs": []} + ) + if getattr(update, "kind", None) is not None: + state["kind"] = _nbi_kind(update.kind) + if getattr(update, "title", None): + state["title"] = update.title + if getattr(update, "status", None) is not None: + state["status"] = _nbi_status(update.status) + diffs = _diffs_from_content(getattr(update, "content", None)) + if diffs: + state["diffs"] = diffs + resp.stream(ToolCallData( + id=tid, + title=state["title"] or "Tool call", + kind=state["kind"], + status=state["status"], + diffs=state["diffs"] or None, + )) + + async def request_permission(self, options, session_id, tool_call, **kw): + resp = self._response + allow = next((o for o in options if o.kind == "allow_once"), None) \ + or next((o for o in options if str(o.kind).startswith("allow")), None) + if resp is None or allow is None: + # No UI to ask, or no allow option offered: fail closed (reject). + return acp.RequestPermissionResponse( + outcome=schema.DeniedOutcome(outcome="cancelled") + ) + callback_id = f"acp-perm-{tool_call.tool_call_id}" + title = getattr(tool_call, "title", None) or "Run this tool?" + resp.stream(ConfirmationData( + title="Codex tool call", + message=( + f"Approve: {title}?\n\n" + "Codex decides which tools to ask about, so some actions may run " + "without a prompt." + ), + confirmArgs={"id": resp.message_id, "data": { + "callback_id": callback_id, "data": {"confirmed": True}}}, + cancelArgs={"id": resp.message_id, "data": { + "callback_id": callback_id, "data": {"confirmed": False}}}, + confirmLabel="Approve", + cancelLabel="Reject", + )) + user_input = await ChatResponse.wait_for_chat_user_input(resp, callback_id) + if user_input.get("confirmed"): + return acp.RequestPermissionResponse( + outcome=schema.AllowedOutcome(outcome="selected", option_id=allow.option_id) + ) + reject = next((o for o in options if o.kind == "reject_once"), None) \ + or next((o for o in options if str(o.kind).startswith("reject")), None) + if reject is not None: + return acp.RequestPermissionResponse( + outcome=schema.AllowedOutcome(outcome="selected", option_id=reject.option_id) + ) + return acp.RequestPermissionResponse(outcome=schema.DeniedOutcome(outcome="cancelled")) + + # fs/*: implemented so an agent that delegates file ops (e.g. claude-acp) + # routes through NBI. codex-acp self-applies, so these may not fire for it. + async def read_text_file(self, path, session_id, limit=None, line=None, **kw): + try: + with open(path, encoding="utf-8") as f: + return acp.ReadTextFileResponse(content=f.read()) + except Exception as e: + raise acp.RequestError.internal_error(str(e)) + + async def write_text_file(self, content, path, session_id, **kw): + try: + os.makedirs(os.path.dirname(path) or ".", exist_ok=True) + with open(path, "w", encoding="utf-8") as f: + f.write(content) + return acp.WriteTextFileResponse() + except Exception as e: + raise acp.RequestError.internal_error(str(e)) + + # terminal/*: not emulated in Phase 1 (Codex runs shell internally). + async def create_terminal(self, command, session_id, **kw): + raise acp.RequestError.method_not_found("terminal not supported") + + async def terminal_output(self, session_id, terminal_id, **kw): + raise acp.RequestError.method_not_found("terminal not supported") + + async def wait_for_terminal_exit(self, session_id, terminal_id, **kw): + raise acp.RequestError.method_not_found("terminal not supported") + + async def kill_terminal(self, session_id, terminal_id, **kw): + return None + + async def release_terminal(self, session_id, terminal_id, **kw): + return None + + +def _block_text(block) -> str: + if block is None: + return "" + return getattr(block, "text", "") or "" + + +def _diffs_from_content(content) -> list[dict]: + out: list[dict] = [] + remaining = _MAX_DIFF_LINES + for c in content or []: + if getattr(c, "type", None) != "diff": + continue + if remaining <= 0: + break + lines, truncated = _diff_lines( + getattr(c, "old_text", "") or "", getattr(c, "new_text", "") or "", + max_lines=remaining, + ) + if lines: + out.append({"path": getattr(c, "path", ""), "lines": lines, "truncated": truncated}) + remaining -= len(lines) + return out + + +class AcpAgentClient: + """Persistent ACP client: one codex-acp subprocess + session on a worker + thread, prompted once per chat request.""" + + def __init__(self, host: Host): + self._host = host + self.websocket_connector: Optional[ThreadSafeWebSocketConnector] = ( + host.websocket_connector + ) + self.current_response: Optional[ChatResponse] = None + self.available_commands: list[str] = [] + self._thread: Optional[threading.Thread] = None + self._loop: Optional[asyncio.AbstractEventLoop] = None + self._conn = None + self._session_id: Optional[str] = None + self._proc = None + self._stderr_task = None + self._started = threading.Event() + self._start_error: Optional[str] = None + self._shutdown = None # asyncio.Event, created on the loop + self._shutting_down = False + self._client: Optional[_NbiAcpClient] = None + self._lock = threading.Lock() + # Serializes turns: the ACP session runs one prompt at a time, and + # current_response is shared, so a second concurrent turn must not + # interleave with the first. + self._turn_lock = threading.Lock() + + @property + def codex_settings(self) -> dict: + return self._host.nbi_config.codex_settings + + def _ensure_started(self) -> bool: + with self._lock: + if ( + self._thread is not None + and self._thread.is_alive() + and not self._shutting_down + ): + return self._start_error is None + # A thread that is shutting down (or already dead) must not be + # reused: its loop is closing, so scheduling a prompt on it would + # hang. Wait for it to exit, then start a fresh one. + if self._thread is not None and self._thread.is_alive(): + self._thread.join(timeout=_START_TIMEOUT) + self._started.clear() + self._start_error = None + self._shutting_down = False + self._thread = threading.Thread( + target=self._thread_main, name="nbi-codex-acp", daemon=True + ) + self._thread.start() + if not self._started.wait(timeout=_START_TIMEOUT): + self._start_error = self._start_error or "Codex agent did not start in time" + return False + return self._start_error is None + + def _thread_main(self): + try: + asyncio.run(self._serve()) + except Exception as e: + self._start_error = f"Codex agent failed to start: {e}" + log.error(self._start_error, exc_info=True) + self._started.set() + + async def _serve(self): + self._loop = asyncio.get_running_loop() + self._shutdown = asyncio.Event() + workdir = get_jupyter_root_dir() + env = self._child_env() + cmd = resolve_codex_acp_command() + codex_approval_args( + bool(self.codex_settings.get("full_access", False)) + ) + log.info("Starting codex-acp: %s", " ".join(cmd)) + try: + self._proc = await asyncio.create_subprocess_exec( + *cmd, stdin=asyncio.subprocess.PIPE, stdout=asyncio.subprocess.PIPE, + stderr=asyncio.subprocess.PIPE, cwd=workdir, env=env, + ) + self._stderr_task = asyncio.create_task(self._drain_stderr()) + self._client = _NbiAcpClient(self) + self._conn = acp.connect_to_agent( + self._client, self._proc.stdin, self._proc.stdout + ) + init = await self._conn.initialize( + protocol_version=acp.PROTOCOL_VERSION, + client_capabilities=schema.ClientCapabilities( + fs=schema.FileSystemCapabilities(read_text_file=True, write_text_file=True), + ), + client_info=schema.Implementation(name="notebook-intelligence", version="1.0.0"), + ) + await self._authenticate(init) + mcp = schema.McpServerStdio( + name="nbi", command=sys.executable, + args=["-m", "notebook_intelligence.acp_mcp_server"], env=[], + ) + sess = await self._conn.new_session(cwd=workdir, mcp_servers=[mcp]) + self._session_id = sess.session_id + log.info("codex-acp session ready: %s", self._session_id) + except Exception as e: + self._start_error = f"Codex agent session failed: {e}" + log.error(self._start_error, exc_info=True) + self._started.set() + await self._teardown() + return + self._started.set() + try: + await self._shutdown.wait() + finally: + await self._teardown() + + def _child_env(self) -> dict: + env = {k: v for k, v in os.environ.items() + if k != "CLAUDECODE" and not k.startswith("CLAUDE_CODE_")} + api_key = (self.codex_settings.get("api_key") or "").strip() \ + or os.environ.get("OPENAI_API_KEY", "").strip() + if api_key: + env["OPENAI_API_KEY"] = api_key + # Use a clean config dir so a stale ChatGPT OAuth token doesn't + # shadow API-key auth (the spike hit refresh_token_reused). + env["CODEX_HOME"] = os.path.join( + self._host.nbi_config.nbi_user_dir, "codex-home" + ) + os.makedirs(env["CODEX_HOME"], exist_ok=True) + return env + + async def _authenticate(self, init): + methods = [m.id for m in (init.auth_methods or [])] + if not methods: + return + api_key = (self.codex_settings.get("api_key") or "").strip() \ + or os.environ.get("OPENAI_API_KEY", "").strip() + method = "openai-api-key" if (api_key and "openai-api-key" in methods) else methods[0] + try: + await self._conn.authenticate(method_id=method) + except Exception as e: + log.warning("codex-acp authenticate(%s) failed: %s", method, e) + + async def _drain_stderr(self): + while self._proc and self._proc.stderr: + line = await self._proc.stderr.readline() + if not line: + break + s = line.decode(errors="replace").rstrip() + if s: + log.debug("codex-acp: %s", s[:200]) + + async def _teardown(self): + if self._proc and self._proc.returncode is None: + try: + self._proc.terminate() + await asyncio.wait_for(self._proc.wait(), timeout=5) + except Exception: + try: + self._proc.kill() + await self._proc.wait() + except Exception: + pass + task = getattr(self, "_stderr_task", None) + if task is not None: + task.cancel() + # Drop references to the dying loop/session so _ensure_started can't + # hand a closing loop to the next turn (see the restart race). + self._conn = None + self._session_id = None + self._client = None + self._proc = None + self._loop = None + + async def _run_prompt(self, text: str): + await self._conn.prompt( + prompt=[schema.TextContentBlock(type="text", text=text)], + session_id=self._session_id, + ) + + async def _cancel(self): + try: + if self._conn and self._session_id: + await self._conn.cancel(session_id=self._session_id) + except Exception as e: + log.debug("codex-acp cancel failed: %s", e) + + def query(self, request: ChatRequest, response: ChatResponse) -> Optional[str]: + """Run one turn. Returns an error string on failure, else None. + + Mirrors ClaudeCodeClient.query: blocks the per-request thread while the + turn streams on the ACP loop thread. + """ + if not self._turn_lock.acquire(blocking=False): + return "Codex agent is busy with another request" + try: + if not self._ensure_started(): + return self._start_error or "Codex agent is not available" + loop = self._loop + if loop is None: + return "Codex agent is not available" + # Fresh turn: drop the prior turn's accumulated tool-call cards so + # the per-id merge cache does not grow without bound. + if self._client is not None: + self._client._tool_state.clear() + self.current_response = response + try: + fut = asyncio.run_coroutine_threadsafe( + self._run_prompt(request.prompt), loop + ) + except RuntimeError as e: + return f"Codex agent is not available: {e}" + start = time.time() + try: + while True: + if request.cancel_token is not None and request.cancel_token.is_cancel_requested: + self._schedule(self._cancel(), loop) + return None + try: + fut.result(timeout=_POLL) + return None + except concurrent.futures.TimeoutError: + pass + except Exception as e: + log.error("codex-acp turn failed: %s", e, exc_info=True) + return f"Codex agent error: {e}" + if time.time() - start > _RESPONSE_TIMEOUT: + self._schedule(self._cancel(), loop) + return "Codex agent response timeout" + finally: + self.current_response = None + finally: + self._turn_lock.release() + + def _schedule(self, coro, loop): + try: + asyncio.run_coroutine_threadsafe(coro, loop) + except RuntimeError: + coro.close() + + def shutdown(self): + # Mark shutting-down first so a concurrent _ensure_started won't reuse + # the loop that is about to close. + self._shutting_down = True + if self._loop is not None and self._shutdown is not None: + try: + self._loop.call_soon_threadsafe(self._shutdown.set) + except Exception: + pass + + +class CodexAgentChatParticipant(BaseChatParticipant): + def __init__(self, host: Host): + super().__init__() + self._host = host + self._client = AcpAgentClient(host) + + @property + def id(self) -> str: + return CODEX_AGENT_CHAT_PARTICIPANT_ID + + @property + def name(self) -> str: + return "Codex" + + @property + def description(self) -> str: + return "OpenAI Codex (via the Agent Client Protocol)" + + @property + def icon_path(self) -> str: + return CODEX_AGENT_ICON_URL + + @property + def commands(self) -> list[ChatCommand]: + return [ChatCommand(name=c, description="") for c in self._client.available_commands] + + @property + def websocket_connector(self) -> ThreadSafeWebSocketConnector: + return self._client.websocket_connector + + @websocket_connector.setter + def websocket_connector(self, connector: ThreadSafeWebSocketConnector): + self._client.websocket_connector = connector + + def restart_client(self) -> None: + """Stop the running ACP client so the next request restarts it. + + Codex reads its credentials and model from codex_settings at launch; a + settings change only takes effect on a fresh subprocess. + """ + self._client.shutdown() + + def chat_prompt(self, model_provider: str, model_name: str) -> str: + return "" + + async def handle_chat_request( + self, request: ChatRequest, response: ChatResponse, options: dict = {} + ) -> None: + self._current_chat_request = request + try: + response.stream(ProgressData("Thinking…")) + result = self._client.query(request, response) + if isinstance(result, str) and result: + response.stream(MarkdownData(content=f"**Codex agent error:** {result}")) + except Exception as e: + log.error("Error handling Codex chat request: %s", e, exc_info=True) + try: + response.stream(MarkdownData(content=f"**Error:** {e}")) + except Exception: + pass + finally: + try: + response.finish() + except Exception: + pass diff --git a/notebook_intelligence/acp_mcp_server.py b/notebook_intelligence/acp_mcp_server.py new file mode 100644 index 00000000..fbd5ca83 --- /dev/null +++ b/notebook_intelligence/acp_mcp_server.py @@ -0,0 +1,89 @@ +# Copyright (c) Mehmet Bektas + +"""A small stdio MCP server that exposes NBI tools to an ACP agent. + +ACP's ``session/new`` takes stdio/socket MCP servers, not the in-process SDK +MCP server Claude mode uses (``create_sdk_mcp_server``), so NBI's tools must run +as a real subprocess for the agent-mode framework (issue #378, Phase 1). This +module is that subprocess: newline-delimited JSON-RPC over stdio, launched as +``python -m notebook_intelligence.acp_mcp_server`` with its cwd set to the +JupyterLab working directory by the participant. + +Phase 1 ships one safe, dependency-free tool (``nbi_workspace_root``) so the +end-to-end MCP path is exercised; richer Jupyter UI tools that need the live +websocket are a follow-up. +""" + +import json +import os +import sys + +PROTOCOL_VERSION = "2025-06-18" + +TOOLS = [ + { + "name": "nbi_workspace_root", + "description": ( + "Return the absolute path of the JupyterLab workspace root that " + "Notebook Intelligence is serving. Use it to resolve relative paths." + ), + "inputSchema": {"type": "object", "properties": {}}, + }, +] + + +def _send(obj): + sys.stdout.write(json.dumps(obj) + "\n") + sys.stdout.flush() + + +def _result(mid, result): + _send({"jsonrpc": "2.0", "id": mid, "result": result}) + + +def _error(mid, code, message): + _send({"jsonrpc": "2.0", "id": mid, "error": {"code": code, "message": message}}) + + +def _call_tool(name, arguments): + if name == "nbi_workspace_root": + return {"content": [{"type": "text", "text": os.getcwd()}]} + raise KeyError(name) + + +def main(): + for line in sys.stdin: + line = line.strip() + if not line: + continue + try: + msg = json.loads(line) + except json.JSONDecodeError: + continue + method = msg.get("method") + mid = msg.get("id") + if method == "initialize": + params = msg.get("params") or {} + _result(mid, { + "protocolVersion": params.get("protocolVersion", PROTOCOL_VERSION), + "capabilities": {"tools": {}}, + "serverInfo": {"name": "nbi", "version": "1.0.0"}, + }) + elif method == "notifications/initialized": + continue # notification: no response + elif method == "tools/list": + _result(mid, {"tools": TOOLS}) + elif method == "tools/call": + params = msg.get("params") or {} + try: + _result(mid, _call_tool(params.get("name"), params.get("arguments") or {})) + except KeyError as e: + _error(mid, -32602, f"unknown tool: {e}") + except Exception as e: # surface tool failures as MCP errors + _error(mid, -32603, f"tool error: {e}") + elif mid is not None: + _error(mid, -32601, f"method not found: {method}") + + +if __name__ == "__main__": + main() diff --git a/notebook_intelligence/ai_service_manager.py b/notebook_intelligence/ai_service_manager.py index f303a8a6..395dce4d 100644 --- a/notebook_intelligence/ai_service_manager.py +++ b/notebook_intelligence/ai_service_manager.py @@ -132,10 +132,15 @@ def websocket_connector(self, _websocket_connector: ThreadSafeWebSocketConnector self._websocket_connector = _websocket_connector self._mcp_manager.websocket_connector = _websocket_connector self._claude_code_chat_participant.websocket_connector = _websocket_connector + if self._codex_chat_participant is not None: + self._codex_chat_participant.websocket_connector = _websocket_connector def initialize(self): self.chat_participants = {} self._claude_code_chat_participant = ClaudeCodeChatParticipant(self) + # Created lazily the first time Codex mode is active so the ACP SDK + # import stays off the startup path (#378). + self._codex_chat_participant = None self.register_llm_provider(GitHubCopilotLLMProvider()) self.register_llm_provider(self._openai_compatible_llm_provider) self.register_llm_provider(self._litellm_compatible_llm_provider) @@ -221,8 +226,33 @@ def update_models_from_config(self): else: self.unregister_chat_participant(self._claude_code_chat_participant) + # Codex agent mode (#378). is_codex_mode already reflects the resolved + # active agent, so it is exclusive with is_claude_code_mode. + if self.is_codex_mode: + self._default_chat_participant = self._ensure_codex_participant() + elif self._codex_chat_participant is not None: + self.unregister_chat_participant(self._codex_chat_participant) + self.chat_participants[DEFAULT_CHAT_PARTICIPANT_ID] = self._default_chat_participant + def _ensure_codex_participant(self): + """Create (lazily, to defer the ACP import) and register the Codex + participant, returning it (#378).""" + if self._codex_chat_participant is None: + from notebook_intelligence.acp_agent import CodexAgentChatParticipant + self._codex_chat_participant = CodexAgentChatParticipant(self) + self._codex_chat_participant.websocket_connector = self._websocket_connector + if self._codex_chat_participant.id not in self.chat_participants: + self.register_chat_participant(self._codex_chat_participant) + return self._codex_chat_participant + + def restart_codex_client(self): + """Drop the running ACP subprocess (if any) so the next Codex request + relaunches it with the current codex_settings (#378). No-op when the + participant has never been created.""" + if self._codex_chat_participant is not None: + self._codex_chat_participant.restart_client() + def update_mcp_servers(self): self._mcp_manager.update_mcp_servers(self.nbi_config.mcp) @@ -317,9 +347,53 @@ def inline_completion_model(self) -> InlineCompletionModel: def embedding_model(self) -> EmbeddingModel: return self._embedding_model + # Agent modes that take over chat, in the priority order used to pick a + # default when more than one is enabled. Extends as more ACP agents land + # (e.g. gemini, opencode). Claude is first so the historical "Claude wins + # when both are on" behavior is preserved when the user has no preference. + AGENT_MODE_PRIORITY = ("claude", "codex") + + def _agent_mode_enabled(self, mode: str) -> bool: + if mode == "claude": + return bool(self.nbi_config.claude_settings.get("enabled", False)) + if mode == "codex": + return bool(self.nbi_config.codex_settings.get("enabled", False)) + return False + + @property + def enabled_agent_modes(self) -> list[str]: + return [m for m in self.AGENT_MODE_PRIORITY if self._agent_mode_enabled(m)] + + @staticmethod + def resolve_active_agent(enabled: list[str], preferred: Optional[str]) -> Optional[str]: + """Pick the active agent: the preference when it is enabled, else the + highest-priority enabled mode, else None.""" + if not enabled: + return None + if preferred in enabled: + return preferred + return enabled[0] + + @property + def active_agent_mode(self) -> Optional[str]: + """The single agent mode currently handling chat. + + When several agent modes are enabled the user's stored preference + (``active_chat_agent``) wins; otherwise the highest-priority enabled + mode is used. Returns None when no agent mode is enabled (the native + provider path handles chat). + """ + return self.resolve_active_agent( + self.enabled_agent_modes, self.nbi_config.get("active_chat_agent") + ) + @property def is_claude_code_mode(self) -> bool: - return self.nbi_config.claude_settings.get('enabled', False) + return self.active_agent_mode == "claude" + + @property + def is_codex_mode(self) -> bool: + return self.active_agent_mode == "codex" @property def claude_models(self) -> list[dict]: @@ -450,13 +524,19 @@ def get_chat_participant(self, prompt: str) -> ChatParticipant: async def handle_chat_request(self, request: ChatRequest, response: ChatResponse, options: dict = {}) -> None: is_claude_code_mode = self.is_claude_code_mode - if not is_claude_code_mode and self.chat_model is None: + is_codex_mode = self.is_codex_mode + if not is_claude_code_mode and not is_codex_mode and self.chat_model is None: response.stream(MarkdownData("Chat model is not set!")) response.stream(ButtonData("Configure", "notebook-intelligence:open-configuration-dialog")) response.finish() return request.host = self - prompt_parts = PromptParts(input=request.prompt, participant=CLAUDE_CODE_CHAT_PARTICIPANT_ID) if is_claude_code_mode else AIServiceManager.parse_prompt(request.prompt) + if is_claude_code_mode: + prompt_parts = PromptParts(input=request.prompt, participant=CLAUDE_CODE_CHAT_PARTICIPANT_ID) + elif is_codex_mode: + prompt_parts = PromptParts(input=request.prompt, participant=self._ensure_codex_participant().id) + else: + prompt_parts = AIServiceManager.parse_prompt(request.prompt) # add MCP server prompt messages to chat history if prompt_parts.mcp_prompt_name != "": diff --git a/notebook_intelligence/config.py b/notebook_intelligence/config.py index 60f1e58b..f3e6c0d4 100644 --- a/notebook_intelligence/config.py +++ b/notebook_intelligence/config.py @@ -12,8 +12,10 @@ from notebook_intelligence.feature_flags import ( CHAT_MODEL_OVERRIDES, CLAUDE_SETTINGS_OVERRIDES, + CODEX_SETTINGS_OVERRIDES, INLINE_COMPLETION_MODEL_OVERRIDES, apply_claude_policies, + apply_codex_policies, apply_string_overrides, ) @@ -172,6 +174,15 @@ def claude_settings(self): resolved, self._string_overrides, CLAUDE_SETTINGS_OVERRIDES ) + @property + def codex_settings(self): + resolved = apply_codex_policies( + self.get('codex_settings', {}), self._feature_policies + ) + return apply_string_overrides( + resolved, self._string_overrides, CODEX_SETTINGS_OVERRIDES + ) + @property def chat_model(self): raw = self.get('chat_model', {'provider': 'github-copilot', 'model': 'gpt-4.1'}) diff --git a/notebook_intelligence/extension.py b/notebook_intelligence/extension.py index d733ebee..69fb912b 100644 --- a/notebook_intelligence/extension.py +++ b/notebook_intelligence/extension.py @@ -32,6 +32,7 @@ CHAT_MODEL_OVERRIDES, CLAUDE_CODE_TOOLS_ID, CLAUDE_SETTINGS_OVERRIDES, + CODEX_SETTINGS_OVERRIDES, INLINE_COMPLETION_MODEL_OVERRIDES, JUPYTER_UI_TOOLS_ID, POLICY_FORCE_OFF, @@ -39,6 +40,7 @@ POLICY_USER_CHOICE, VALID_POLICIES, apply_claude_policies, + apply_codex_policies, apply_string_overrides, is_force_off, is_locked, @@ -291,6 +293,12 @@ def _resolve_positive_int_with_env(env_var_name: str, traitlet_value: int) -> in ("output_followup", "NBI_OUTPUT_FOLLOWUP_POLICY", "output_followup_policy"), ("output_toolbar", "NBI_OUTPUT_TOOLBAR_POLICY", "output_toolbar_policy"), ("claude_mode", "NBI_CLAUDE_MODE_POLICY", "claude_mode_policy"), + ("codex_mode", "NBI_CODEX_MODE_POLICY", "codex_mode_policy"), + ( + "codex_full_access", + "NBI_CODEX_FULL_ACCESS_POLICY", + "codex_full_access_policy", + ), ( "claude_continue_conversation", "NBI_CLAUDE_CONTINUE_CONVERSATION_POLICY", @@ -356,13 +364,17 @@ def _resolve_positive_int_with_env(env_var_name: str, traitlet_value: int) -> in # Fallback used when a policies dict hasn't been populated (handler classes # before _setup_handlers, direct construction in tests). Everything defaults -# to user-choice except bypass, which must fail closed. +# to user-choice except bypass, the experimental Codex mode, and Codex full +# access, which must fail closed. FEATURE_POLICY_DEFAULTS = {name: POLICY_USER_CHOICE for name in FEATURE_POLICY_NAMES} FEATURE_POLICY_DEFAULTS["claude_bypass_permissions"] = POLICY_FORCE_OFF +FEATURE_POLICY_DEFAULTS["codex_mode"] = POLICY_FORCE_OFF +FEATURE_POLICY_DEFAULTS["codex_full_access"] = POLICY_FORCE_OFF # ``(setting_lock_name, env_var)`` pairs for the value-presence-locks. The # claude_api_key entry maps to ANTHROPIC_API_KEY (the SDK's native convention) -# rather than an NBI-prefixed env var. Same for claude_base_url. +# rather than an NBI-prefixed env var. Same for claude_base_url. The codex_* +# entries follow the same idea against OpenAI's native env vars. STRING_OVERRIDE_SPEC = ( ("chat_model_provider", "NBI_CHAT_MODEL_PROVIDER"), ("chat_model_id", "NBI_CHAT_MODEL_ID"), @@ -372,6 +384,9 @@ def _resolve_positive_int_with_env(env_var_name: str, traitlet_value: int) -> in ("claude_inline_completion_model", "NBI_CLAUDE_INLINE_COMPLETION_MODEL"), ("claude_api_key", "ANTHROPIC_API_KEY"), ("claude_base_url", "ANTHROPIC_BASE_URL"), + ("codex_chat_model", "NBI_CODEX_CHAT_MODEL"), + ("codex_api_key", "OPENAI_API_KEY"), + ("codex_base_url", "OPENAI_BASE_URL"), ) SETTING_LOCK_NAMES = tuple(name for name, _ in STRING_OVERRIDE_SPEC) @@ -384,6 +399,7 @@ def _build_feature_policies_response(policies: dict, nbi_config) -> dict: feature only needs an entry here plus a matching env-var resolution. """ claude_settings = nbi_config.claude_settings or {} + codex_settings = nbi_config.codex_settings or {} tools = claude_settings.get("tools") or [] sources = claude_settings.get("setting_sources") or [] @@ -392,6 +408,11 @@ def _build_feature_policies_response(policies: dict, nbi_config) -> dict: "output_followup": nbi_config.enable_output_followup, "output_toolbar": nbi_config.enable_output_toolbar, "claude_mode": bool(claude_settings.get("enabled", False)), + # Experimental Codex (ACP) agent mode; defaults to force-off (#378). + "codex_mode": bool(codex_settings.get("enabled", False)), + # Codex autonomous "full access" posture; defaults to force-off so the + # agent asks before risky actions unless an admin opts in (#378). + "codex_full_access": bool(codex_settings.get("full_access", False)), "claude_continue_conversation": bool( claude_settings.get("continue_conversation", False) ), @@ -460,15 +481,18 @@ def _build_setting_locks_response(string_overrides: dict) -> dict: } -def _scrub_credentials_for_wire(claude_settings: dict, string_overrides: dict) -> dict: +def _scrub_credentials_for_wire( + settings: dict, string_overrides: dict, api_key_lock: str = "claude_api_key" +) -> dict: """Strip the api_key from the capabilities response when locked by env. - The Anthropic SDK reads ANTHROPIC_API_KEY directly; surfacing the value - through the frontend would leak the credential. + The SDKs read their key env var directly (ANTHROPIC_API_KEY for Claude, + OPENAI_API_KEY for Codex); surfacing the value through the frontend would + leak the credential. """ - if not string_overrides.get("claude_api_key"): - return claude_settings - result = dict(claude_settings or {}) + if not string_overrides.get(api_key_lock): + return settings + result = dict(settings or {}) result["api_key"] = "" return result @@ -594,6 +618,14 @@ def is_provider_enabled(provider_id: str) -> bool: "claude_settings": _scrub_credentials_for_wire( nbi_config.claude_settings, self.string_overrides ), + "codex_settings": _scrub_credentials_for_wire( + nbi_config.codex_settings, self.string_overrides, "codex_api_key" + ), + # Which agent modes are enabled and which one is currently active. + # The frontend shows an agent picker when more than one is enabled + # and gates the active agent's UI on active_agent_mode (#378). + "enabled_agent_modes": ai_service_manager.enabled_agent_modes, + "active_agent_mode": ai_service_manager.active_agent_mode, "spinner_verbs": _read_claude_spinner_verbs(), "claude_models": ai_service_manager.claude_models, # Drive launcher-tile visibility (issues #183, #260). Each flag @@ -668,6 +700,8 @@ def post(self): "inline_completion_debouncer_delay", "mcp_server_settings", "claude_settings", + "codex_settings", + "active_chat_agent", "enable_explain_error", "enable_output_followup", "enable_output_toolbar", @@ -699,6 +733,8 @@ def post(self): has_model_change = False has_claude_settings_change = False + has_codex_settings_change = False + has_active_agent_change = False for key in data: if key in locked_keys: continue @@ -735,6 +771,34 @@ def post(self): if self.string_overrides.get("claude_api_key"): value = dict(value) value["api_key"] = "" + elif key == "codex_settings": + value = apply_codex_policies(value, self.feature_policies) + value = apply_string_overrides( + value, self.string_overrides, CODEX_SETTINGS_OVERRIDES + ) + # OPENAI_API_KEY is a credential; same handling as Claude above. + if self.string_overrides.get("codex_api_key"): + value = dict(value) + value["api_key"] = "" + # Only act when something actually changed. The settings tab + # re-POSTs on mount, and an unconditional restart would bounce + # the live ACP subprocess every time the tab is opened. Compare + # against the raw stored value (not the codex_settings property, + # which re-injects env overrides such as OPENAI_API_KEY and would + # never match the scrubbed value we persist). + has_codex_settings_change = ( + value != (ai_service_manager.nbi_config.get("codex_settings") or {}) + ) + elif key == "active_chat_agent": + # The preferred agent when several agent modes are enabled. + # Reject unknown ids ("" clears the preference); an unknown + # value would just be ignored by the resolver, but persisting + # junk helps nobody. + if value not in ai_service_manager.AGENT_MODE_PRIORITY and value != "": + continue + has_active_agent_change = ( + value != (ai_service_manager.nbi_config.get("active_chat_agent") or "") + ) ai_service_manager.nbi_config.set(key, value) if key == "store_github_access_token": if value: @@ -756,13 +820,22 @@ def post(self): default_chat_participant.update_client_debounced() ai_service_manager.nbi_config.save() - if has_model_change or has_claude_settings_change: + if ( + has_model_change + or has_claude_settings_change + or has_codex_settings_change + or has_active_agent_change + ): ai_service_manager.update_models_from_config() if has_claude_settings_change: default_chat_participant = ai_service_manager.default_chat_participant if isinstance(default_chat_participant, ClaudeCodeChatParticipant): # needed to reconnect / update default_chat_participant.update_client_debounced() + if has_codex_settings_change: + # Codex reads its key/model from codex_settings at launch, so the + # running ACP subprocess must restart to pick up the new values. + ai_service_manager.restart_codex_client() self.finish(json.dumps({})) @@ -2867,6 +2940,30 @@ class NotebookIntelligence(ExtensionApp): config=True, ) + codex_mode_policy = TraitletEnum( + list(VALID_POLICIES), + default_value=POLICY_FORCE_OFF, + help=""" + Org-wide policy for whether the experimental Codex (ACP) agent mode is + available (#378). Defaults to force-off; set to user-choice to let users + enable it. Overridden by the NBI_CODEX_MODE_POLICY env var. + """, + config=True, + ) + + codex_full_access_policy = TraitletEnum( + list(VALID_POLICIES), + default_value=POLICY_FORCE_OFF, + help=""" + Org-wide policy for Codex "full access" (#378): running tools + autonomously without asking. Defaults to force-off, so Codex is pinned + to ask before anything beyond trusted read-only commands. Set to + user-choice to let users opt into unattended runs, or force-on to + require it. Overridden by the NBI_CODEX_FULL_ACCESS_POLICY env var. + """, + config=True, + ) + claude_continue_conversation_policy = TraitletEnum( list(VALID_POLICIES), default_value=POLICY_USER_CHOICE, diff --git a/notebook_intelligence/feature_flags.py b/notebook_intelligence/feature_flags.py index 05e1ca27..531d2fbc 100644 --- a/notebook_intelligence/feature_flags.py +++ b/notebook_intelligence/feature_flags.py @@ -26,6 +26,11 @@ ("claude_api_key", "api_key"), ("claude_base_url", "base_url"), ) +CODEX_SETTINGS_OVERRIDES = ( + ("codex_chat_model", "chat_model"), + ("codex_api_key", "api_key"), + ("codex_base_url", "base_url"), +) def resolve_feature_flag(policy: str, user_setting: bool) -> Tuple[bool, bool]: @@ -87,6 +92,28 @@ def apply_member_policy(members: list, item: str, policy: str) -> list: return list(members) +def apply_codex_policies(codex_settings: dict, policies: dict) -> dict: + """Apply admin policies to a ``codex_settings`` dict (issue #378). + + Two gates: ``codex_mode`` clamps ``enabled``, and ``codex_full_access`` + clamps ``full_access`` (the autonomous, run-without-asking posture, which + defaults to force-off like Claude's bypass-permissions). Used on both the + read path and the write-filter path, like ``apply_claude_policies``. + """ + result = dict(codex_settings or {}) + mode_policy = policies.get("codex_mode", POLICY_USER_CHOICE) + if mode_policy == POLICY_FORCE_ON: + result["enabled"] = True + elif mode_policy == POLICY_FORCE_OFF: + result["enabled"] = False + full_access_policy = policies.get("codex_full_access", POLICY_USER_CHOICE) + if full_access_policy == POLICY_FORCE_ON: + result["full_access"] = True + elif full_access_policy == POLICY_FORCE_OFF: + result["full_access"] = False + return result + + def apply_claude_policies(claude_settings: dict, policies: dict) -> dict: """Apply admin policies to a ``claude_settings`` dict. diff --git a/pyproject.toml b/pyproject.toml index a1f1bde2..52023548 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,6 +49,11 @@ dependencies = [ # BerriAI/litellm#25231 for the upstream fix. "mcp>=1.27.0", "claude-agent-sdk", + # Agent Client Protocol client SDK for Codex (and future ACP) agent mode + # (#378). Pinned because the protocol is pre-1.0 and the Python SDK still + # ships occasional breaking changes; imported lazily (only when Codex mode + # is used) per the #370 lazy-provider-import policy. + "agent-client-protocol==0.10.1", "anthropic>=0.22.1", # Used to tear down an agent subprocess and its descendants (shells, MCP # servers, dev servers) on cancel; see util.terminate_process_tree. Already diff --git a/src/api.ts b/src/api.ts index 203dd3a0..5be24b8a 100644 --- a/src/api.ts +++ b/src/api.ts @@ -267,6 +267,8 @@ export type FeaturePolicyName = | 'output_followup' | 'output_toolbar' | 'claude_mode' + | 'codex_mode' + | 'codex_full_access' | 'claude_continue_conversation' | 'claude_code_tools' | 'claude_jupyter_ui_tools' @@ -296,7 +298,10 @@ export type SettingLockName = | 'claude_chat_model' | 'claude_inline_completion_model' | 'claude_api_key' - | 'claude_base_url'; + | 'claude_base_url' + | 'codex_chat_model' + | 'codex_api_key' + | 'codex_base_url'; export type ISettingLocks = Record; @@ -389,6 +394,26 @@ export class NBIConfig { return this.capabilities.claude_settings; } + get codexSettings(): any { + return this.capabilities.codex_settings ?? {}; + } + + // The agent modes that are enabled, in priority order; the frontend shows an + // agent picker when more than one is on. + get enabledAgentModes(): string[] { + return this.capabilities.enabled_agent_modes ?? []; + } + + // The single agent mode currently handling chat (the user's pick when + // several are enabled), or null when the native provider path is in use. + get activeAgentMode(): string | null { + return this.capabilities.active_agent_mode ?? null; + } + + get isInCodexMode(): boolean { + return this.activeAgentMode === 'codex'; + } + get spinnerVerbs(): { mode: string; verbs: string[] } | null { return this.capabilities.spinner_verbs ?? null; } @@ -398,7 +423,7 @@ export class NBIConfig { } get isInClaudeCodeMode(): boolean { - return this.claudeSettings.enabled === true; + return this.activeAgentMode === 'claude'; } get isClaudeCliAvailable(): boolean { @@ -501,6 +526,8 @@ export class NBIConfig { 'output_followup', 'output_toolbar', 'claude_mode', + 'codex_mode', + 'codex_full_access', 'claude_continue_conversation', 'claude_code_tools', 'claude_jupyter_ui_tools', @@ -559,7 +586,10 @@ export class NBIConfig { 'claude_chat_model', 'claude_inline_completion_model', 'claude_api_key', - 'claude_base_url' + 'claude_base_url', + 'codex_chat_model', + 'codex_api_key', + 'codex_base_url' ]; const result = {} as ISettingLocks; for (const name of names) { @@ -677,6 +707,7 @@ export class NBIAPI { static getChatEnabled() { return ( this.config.isInClaudeCodeMode || + this.config.isInCodexMode || (this.config.chatModel.provider === GITHUB_COPILOT_PROVIDER_ID ? !this.getGHLoginRequired() : this.config.llmProviders.find( diff --git a/src/chat-sidebar.tsx b/src/chat-sidebar.tsx index 7e766434..c53b9cbd 100644 --- a/src/chat-sidebar.tsx +++ b/src/chat-sidebar.tsx @@ -78,6 +78,8 @@ import { CheckBoxItem } from './components/checkbox'; import { SafeAnchor } from './components/safe-anchor'; import { mcpServerSettingsToEnabledState } from './components/mcp-util'; import claudeSvgStr from '../style/icons/claude.svg'; +import openaiSvgStr from '../style/icons/openai.svg'; +import { AgentSelect } from './components/agent-select'; import { AskUserQuestion } from './components/ask-user-question'; import { ClaudeSessionPicker } from './components/claude-session-picker'; import { @@ -1240,6 +1242,9 @@ function getActiveChatModel(): { provider: string; model: string } { } function SidebarComponent(props: any) { + const [activeAgent, setActiveAgent] = useState( + NBIAPI.config.activeAgentMode ?? '' + ); const [chatMessages, setChatMessages] = useState([]); const [prompt, setPrompt] = useState(''); const [draftPrompt, setDraftPrompt] = useState(''); @@ -2303,6 +2308,7 @@ function SidebarComponent(props: any) { mcpServerSettingsRef.current ); setMCPServerEnabledState(newMcpServerEnabledState); + setActiveAgent(NBIAPI.config.activeAgentMode ?? ''); setRenderCount(renderCount => renderCount + 1); }; NBIAPI.configChanged.connect(handler); @@ -4041,6 +4047,19 @@ function SidebarComponent(props: any) { + {NBIAPI.config.enabledAgentModes.length > 1 && ( +
+ Agent + { + setActiveAgent(mode); + NBIAPI.setConfig({ active_chat_agent: mode }); + }} + /> +
+ )}
{skillsReloadedVisible && (
@@ -4070,31 +4089,33 @@ function SidebarComponent(props: any) {
)} - {!NBIAPI.config.isInClaudeCodeMode && ghLoginRequired && ( -
-
- You are not logged in to GitHub Copilot. Please login now to - activate chat. -
-
- + {!NBIAPI.config.isInClaudeCodeMode && + !NBIAPI.config.isInCodexMode && + ghLoginRequired && ( +
+
+ You are not logged in to GitHub Copilot. Please login now to + activate chat. +
+
+ - + +
-
- )} + )} {chatEnabled && (chatMessages.length === 0 ? ( @@ -4355,45 +4376,48 @@ function SidebarComponent(props: any) { />
- {!NBIAPI.config.isInClaudeCodeMode && ( -
- { + if (event.target.value === 'ask') { + setToolSelections(toolSelectionsEmpty); + } + setShowModeTools(false); + setChatMode(event.target.value); + }} + > + + + +
+ )} + {chatMode !== 'ask' && + !NBIAPI.config.isInClaudeCodeMode && + !NBIAPI.config.isInCodexMode && ( +
- )} - {chatMode !== 'ask' && !NBIAPI.config.isInClaudeCodeMode && ( - - )} + + {selectedToolCount > 0 && <>{selectedToolCount}} + + )} {NBIAPI.config.isInClaudeCodeMode && ( )} + {NBIAPI.config.isInCodexMode && ( + + )}
+
+ + + + +
+
Codex account
+
+
+
+
+
+ API Key (optional) +
+ setApiKey(event.target.value)} + /> +
+
+
+ Base URL (optional) +
+ setBaseUrl(event.target.value)} + /> +
+
+
+
+
+ + + ); +} diff --git a/src/index.ts b/src/index.ts index 37760b33..c4e1e09c 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1589,6 +1589,7 @@ const plugin: JupyterFrontEndPlugin = { const isChatEnabled = (): boolean => { return ( NBIAPI.config.isInClaudeCodeMode || + NBIAPI.config.isInCodexMode || (NBIAPI.config.chatModel.provider === GITHUB_COPILOT_PROVIDER_ID ? !githubLoginRequired() : NBIAPI.config.chatModel.provider !== 'none') diff --git a/style/base.css b/style/base.css index 9a61ea06..2d9fc088 100644 --- a/style/base.css +++ b/style/base.css @@ -200,6 +200,91 @@ flex-grow: 1; } +.sidebar-agent-select { + display: flex; + align-items: center; + gap: 6px; + padding: 4px 10px 6px; +} + +.sidebar-agent-label { + font-size: 12px; + color: var(--jp-ui-font-color2); +} + +.agent-select-container { + position: relative; + display: flex; + flex-grow: 1; +} + +.agent-select-button { + display: flex; + align-items: center; + gap: 6px; + width: 100%; + box-sizing: border-box; + padding: 2px 6px; + cursor: pointer; + text-align: left; +} + +.agent-select-button-label { + flex-grow: 1; +} + +.agent-select-icon { + display: inline-flex; + flex-shrink: 0; +} + +.agent-select-icon svg { + width: 14px; + height: 14px; +} + +.agent-select-menu { + position: absolute; + top: 100%; + left: 0; + margin-top: 4px; + z-index: 100; + min-width: 160px; + padding: 4px; + background-color: var(--jp-layout-color1); + border: 1px solid var(--jp-border-color1); + border-radius: 4px; + box-shadow: 0 2px 8px rgb(0 0 0 / 30%); +} + +.agent-select-menu-item { + display: flex; + align-items: center; + gap: 6px; + width: 100%; + box-sizing: border-box; + padding: 4px 8px; + border: none; + background: none; + text-align: left; + cursor: pointer; + color: var(--jp-ui-font-color1); + font-size: var(--jp-ui-font-size1); + border-radius: 2px; +} + +.agent-select-menu-item:hover, +.agent-select-menu-item:focus-visible { + background-color: var(--jp-layout-color2); + outline: none; +} + +.agent-select-menu-check { + display: inline-flex; + width: 16px; + flex-shrink: 0; +} + .sidebar-messages { flex-grow: 1; overflow-y: auto; @@ -1461,6 +1546,12 @@ pre:has(.code-block-header) { color: #d97757; } +.codex-icon svg { + width: 16px; + height: 16px; + color: #0a84ff; +} + .send-button { display: flex; align-items: center; @@ -1659,6 +1750,10 @@ button.send-button:disabled:active:not(.send-button-stop) { font-size: 15px; } +.config-warning { + color: var(--jp-warn-color1); +} + .model-config-section-body { padding-left: 10px; display: flex; @@ -2944,6 +3039,13 @@ svg.access-token-warning { height: 18px; } +/* The Codex footer badge matches the Claude badge size. */ +.chat-mode-widgets-container .codex-icon svg { + margin-top: 4px; + width: 18px; + height: 18px; +} + .chat-confirmation-form.ask-user-question { display: flex; flex-direction: column; diff --git a/tests/test_acp_agent.py b/tests/test_acp_agent.py new file mode 100644 index 00000000..cf06647c --- /dev/null +++ b/tests/test_acp_agent.py @@ -0,0 +1,284 @@ +# Copyright (c) Mehmet Bektas + +"""Unit tests for the Codex/ACP backend mapping (issue #378, Phase 1). + +These exercise the editor-side translation (ACP events -> NBI cards/approval) +without launching codex-acp; the live end-to-end path is covered by the +Phase 0 spike and the JupyterLab Playwright check. +""" + +import asyncio +import concurrent.futures +from types import SimpleNamespace + +import pytest + +from acp import schema + +from notebook_intelligence.acp_agent import ( + _NbiAcpClient, + _diffs_from_content, + _nbi_kind, + _nbi_status, +) +from notebook_intelligence.api import ChatResponse, ResponseStreamDataType + + +class FakeResponse(ChatResponse): + """Captures streamed data and supports the user-input signal round trip.""" + + def __init__(self): + super().__init__() + self.streamed = [] + + @property + def message_id(self) -> str: + return "msg-1" + + def stream(self, data, finish: bool = False) -> None: + self.streamed.append(data) + + def finish(self) -> None: + pass + + +def _client_with_response(resp): + owner = SimpleNamespace(current_response=resp) + return _NbiAcpClient(owner) + + +class TestKindStatusMapping: + @pytest.mark.parametrize("acp_kind,expected", [ + ("read", "read"), ("search", "read"), ("fetch", "read"), + ("edit", "edit"), ("delete", "edit"), ("move", "edit"), + ("execute", "execute"), ("think", "other"), (None, "other"), + ]) + def test_kind(self, acp_kind, expected): + assert _nbi_kind(acp_kind) == expected + + @pytest.mark.parametrize("acp_status,expected", [ + ("pending", "in_progress"), ("in_progress", "in_progress"), + (None, "in_progress"), ("completed", "completed"), ("failed", "failed"), + ]) + def test_status(self, acp_status, expected): + assert _nbi_status(acp_status) == expected + + +class TestDiffMapping: + def test_file_edit_content_becomes_typed_diff_lines(self): + content = [SimpleNamespace(type="diff", path="/x.py", old_text="a\n", new_text="a\nb\n")] + diffs = _diffs_from_content(content) + assert len(diffs) == 1 + assert diffs[0]["path"] == "/x.py" + assert {"type": "add", "content": "b"} in diffs[0]["lines"] + + def test_non_diff_content_ignored(self): + content = [SimpleNamespace(type="content", text="hello")] + assert _diffs_from_content(content) == [] + + +class TestToolCallStreaming: + def test_tool_call_emits_card_with_kind_and_diff(self): + resp = FakeResponse() + client = _client_with_response(resp) + update = SimpleNamespace( + session_update="tool_call", tool_call_id="t1", kind="edit", + status="in_progress", title="Edit /x.py", + content=[SimpleNamespace(type="diff", path="/x.py", old_text="", new_text="hi\n")], + ) + asyncio.run(client.session_update("s", update)) + cards = [d for d in resp.streamed if d.data_type == ResponseStreamDataType.ToolCall] + assert len(cards) == 1 + assert cards[0].id == "t1" and cards[0].kind == "edit" + assert cards[0].status == "in_progress" and cards[0].diffs + + def test_partial_update_merges_cached_kind(self): + resp = FakeResponse() + client = _client_with_response(resp) + asyncio.run(client.session_update("s", SimpleNamespace( + session_update="tool_call", tool_call_id="t1", kind="execute", + status="in_progress", title="Run", content=None))) + # A later update carries only the new status; kind must survive. + asyncio.run(client.session_update("s", SimpleNamespace( + session_update="tool_call_update", tool_call_id="t1", kind=None, + status="completed", title=None, content=None))) + last = [d for d in resp.streamed if d.data_type == ResponseStreamDataType.ToolCall][-1] + assert last.kind == "execute" and last.status == "completed" + + def test_agent_message_chunk_streams_markdown(self): + resp = FakeResponse() + client = _client_with_response(resp) + asyncio.run(client.session_update("s", SimpleNamespace( + session_update="agent_message_chunk", + content=SimpleNamespace(text="hello world")))) + md = [d for d in resp.streamed if d.data_type == ResponseStreamDataType.Markdown] + assert md and md[0].content == "hello world" + + +class TestPermission: + def _opts(self): + return [ + schema.PermissionOption(kind="allow_once", name="Allow", option_id="a1"), + schema.PermissionOption(kind="reject_once", name="Reject", option_id="r1"), + ] + + def _tool_call(self): + return SimpleNamespace(tool_call_id="t1", title="Run echo") + + def _run_with_answer(self, confirmed): + resp = FakeResponse() + client = _client_with_response(resp) + + async def drive(): + task = asyncio.create_task( + client.request_permission(self._opts(), "s", self._tool_call()) + ) + # Let request_permission stream the card and start awaiting input. + await asyncio.sleep(0.05) + assert any( + d.data_type == ResponseStreamDataType.Confirmation for d in resp.streamed + ) + resp.on_user_input({ + "callback_id": "acp-perm-t1", "data": {"confirmed": confirmed} + }) + return await task + + return asyncio.run(drive()) + + def test_approve_selects_allow_option(self): + result = self._run_with_answer(True) + assert isinstance(result.outcome, schema.AllowedOutcome) + assert result.outcome.option_id == "a1" + + def test_reject_selects_reject_option(self): + result = self._run_with_answer(False) + assert isinstance(result.outcome, schema.AllowedOutcome) + assert result.outcome.option_id == "r1" + + def test_no_response_fails_closed(self): + client = _client_with_response(None) + result = asyncio.run( + client.request_permission(self._opts(), "s", self._tool_call()) + ) + assert isinstance(result.outcome, schema.DeniedOutcome) + + +class TestPolicyClamp: + def test_force_off_clamps_enabled(self): + from notebook_intelligence.feature_flags import apply_codex_policies + assert apply_codex_policies({"enabled": True}, {"codex_mode": "force-off"}) == {"enabled": False} + + def test_user_choice_keeps_user_value(self): + from notebook_intelligence.feature_flags import apply_codex_policies + assert apply_codex_policies({"enabled": True}, {"codex_mode": "user-choice"}) == {"enabled": True} + + def test_full_access_force_off_clamps(self): + from notebook_intelligence.feature_flags import apply_codex_policies + out = apply_codex_policies( + {"full_access": True}, {"codex_full_access": "force-off"} + ) + assert out["full_access"] is False + + def test_full_access_user_choice_keeps_value(self): + from notebook_intelligence.feature_flags import apply_codex_policies + assert ( + apply_codex_policies( + {"full_access": True}, {"codex_full_access": "user-choice"} + )["full_access"] + is True + ) + + def test_full_access_force_on(self): + from notebook_intelligence.feature_flags import apply_codex_policies + assert ( + apply_codex_policies( + {"full_access": False}, {"codex_full_access": "force-on"} + )["full_access"] + is True + ) + + +class TestApprovalArgs: + """The approval posture pinned onto the codex-acp command line.""" + + def test_default_pins_untrusted(self): + from notebook_intelligence.acp_agent import codex_approval_args + args = codex_approval_args(False) + assert args == ["-c", 'approval_policy="untrusted"'] + + def test_full_access_runs_unattended(self): + from notebook_intelligence.acp_agent import codex_approval_args + assert codex_approval_args(True) == ["-c", 'approval_policy="never"'] + + +class TestSingleFlight: + """The ACP session runs one prompt at a time; a second concurrent turn + must be rejected rather than interleave with the first.""" + + def _client(self): + from notebook_intelligence.acp_agent import AcpAgentClient + host = SimpleNamespace( + websocket_connector=None, + nbi_config=SimpleNamespace(codex_settings={"enabled": True}), + ) + return AcpAgentClient(host) + + def test_second_concurrent_turn_is_rejected(self): + client = self._client() + # Simulate a turn already in flight by holding the turn lock. + assert client._turn_lock.acquire(blocking=False) + try: + req = SimpleNamespace(prompt="hi", cancel_token=None) + result = client.query(req, FakeResponse()) + assert result is not None and "busy" in result.lower() + finally: + client._turn_lock.release() + + def test_unavailable_agent_releases_the_lock(self): + client = self._client() + # When the agent can't start, query returns the error and still frees + # the lock (the outer finally). + client._ensure_started = lambda: False + client._start_error = "boom" + result = client.query(SimpleNamespace(prompt="x", cancel_token=None), FakeResponse()) + assert result == "boom" + assert client._turn_lock.acquire(blocking=False) + client._turn_lock.release() + + def test_completed_turn_releases_lock_and_resets_tool_state(self): + client = self._client() + # Drive query through the inner run/poll body to a clean finish so the + # nested try/finally (lock release + current_response reset) is covered, + # not just the early-return path. + client._ensure_started = lambda: True + client._loop = object() # only used as an opaque handle below + client._client = SimpleNamespace(_tool_state={"stale": {}}) + + done = concurrent.futures.Future() + done.set_result(None) + + async def _noop(): + return None + + client._run_prompt = lambda prompt: _noop() + + def fake_schedule(coro, loop): + coro.close() # we never run the real prompt coroutine + return done + + import notebook_intelligence.acp_agent as mod + orig = mod.asyncio.run_coroutine_threadsafe + mod.asyncio.run_coroutine_threadsafe = fake_schedule + try: + result = client.query( + SimpleNamespace(prompt="hi", cancel_token=None), FakeResponse() + ) + finally: + mod.asyncio.run_coroutine_threadsafe = orig + + assert result is None + # Prior turn's tool-call cache was cleared, lock released, response reset. + assert client._client._tool_state == {} + assert client.current_response is None + assert client._turn_lock.acquire(blocking=False) + client._turn_lock.release() diff --git a/tests/test_ai_service_manager_integration.py b/tests/test_ai_service_manager_integration.py index 20882cfe..0c5a374f 100644 --- a/tests/test_ai_service_manager_integration.py +++ b/tests/test_ai_service_manager_integration.py @@ -217,3 +217,32 @@ def test_no_fetch_when_claude_mode_disabled(self): ): manager.update_models_from_config() assert mock_fetch.call_count == 0 + + +class TestResolveActiveAgent: + """The active-agent resolution that backs the in-chat agent picker (#378). + + Pure static logic, so it needs no AIServiceManager instance. + """ + + def test_none_enabled_returns_none(self): + assert AIServiceManager.resolve_active_agent([], "claude") is None + assert AIServiceManager.resolve_active_agent([], None) is None + + def test_single_enabled_ignores_preference(self): + assert AIServiceManager.resolve_active_agent(["codex"], None) == "codex" + # A stale preference for a disabled mode is ignored. + assert AIServiceManager.resolve_active_agent(["codex"], "claude") == "codex" + + def test_preference_wins_when_enabled(self): + assert AIServiceManager.resolve_active_agent(["claude", "codex"], "codex") == "codex" + assert AIServiceManager.resolve_active_agent(["claude", "codex"], "claude") == "claude" + + def test_no_preference_falls_back_to_priority_order(self): + # Both enabled, no preference -> first (Claude), preserving the + # historical "Claude wins" default. + assert AIServiceManager.resolve_active_agent(["claude", "codex"], None) == "claude" + assert AIServiceManager.resolve_active_agent(["claude", "codex"], "") == "claude" + + def test_unknown_preference_falls_back(self): + assert AIServiceManager.resolve_active_agent(["claude", "codex"], "gemini") == "claude" diff --git a/tests/test_cell_output_features_response.py b/tests/test_cell_output_features_response.py index 725fea88..d27898ad 100644 --- a/tests/test_cell_output_features_response.py +++ b/tests/test_cell_output_features_response.py @@ -10,6 +10,7 @@ _build_cell_output_features_response, _build_feature_policies_response, _build_setting_locks_response, + _scrub_credentials_for_wire, ) from notebook_intelligence.feature_flags import ( CLAUDE_CODE_TOOLS_ID, @@ -28,6 +29,7 @@ def _config( store_token=False, refresh_open_files=True, claude_settings=None, + codex_settings=None, ): return SimpleNamespace( enable_explain_error=explain, @@ -36,6 +38,7 @@ def _config( store_github_access_token=store_token, refresh_open_files_on_disk_change=refresh_open_files, claude_settings=claude_settings if claude_settings is not None else {}, + codex_settings=codex_settings if codex_settings is not None else {}, ) @@ -223,6 +226,15 @@ def test_presence_of_value_locks_the_field(self): # Unrelated keys remain unlocked. assert response["claude_chat_model"] == {"locked": False} + def test_codex_locks_track_their_env_overrides(self): + response = _build_setting_locks_response( + {"codex_api_key": "sk-openai", "codex_base_url": ""} + ) + assert response["codex_api_key"] == {"locked": True} + # Empty string is the user-choice signal, not a lock. + assert response["codex_base_url"] == {"locked": False} + assert response["codex_chat_model"] == {"locked": False} + def test_response_includes_every_known_lock_name(self): response = _build_setting_locks_response({}) assert set(response.keys()) == { @@ -234,4 +246,37 @@ def test_response_includes_every_known_lock_name(self): "claude_inline_completion_model", "claude_api_key", "claude_base_url", + "codex_chat_model", + "codex_api_key", + "codex_base_url", } + + +class TestScrubCredentialsForWire: + def test_unlocked_settings_pass_through_unchanged(self): + settings = {"api_key": "sk-secret", "base_url": "https://x"} + assert _scrub_credentials_for_wire(settings, {}) is settings + + def test_claude_key_scrubbed_when_env_locks_it(self): + settings = {"api_key": "sk-secret", "enabled": True} + scrubbed = _scrub_credentials_for_wire( + settings, {"claude_api_key": "sk-env"} + ) + assert scrubbed["api_key"] == "" + assert scrubbed["enabled"] is True + # The source dict is not mutated in place. + assert settings["api_key"] == "sk-secret" + + def test_codex_key_scrubbed_with_its_own_lock_name(self): + settings = {"api_key": "sk-openai"} + # The Claude lock must not scrub the Codex key. + assert ( + _scrub_credentials_for_wire( + settings, {"claude_api_key": "sk-env"}, "codex_api_key" + ) + is settings + ) + scrubbed = _scrub_credentials_for_wire( + settings, {"codex_api_key": "sk-env"}, "codex_api_key" + ) + assert scrubbed["api_key"] == "" From cc4ac80d83233f93bd743cc7130de779f9f34a0a Mon Sep 17 00:00:00 2001 From: PJ Doland Date: Fri, 10 Jul 2026 19:58:49 -0400 Subject: [PATCH 2/4] feat(acp): rename to ACP mode, fix streaming and context, add sessions (#378) Response to the PR #380 review feedback. Rename Codex mode to ACP mode with selectable agent types. The mode is named after the protocol, not the first agent: acp_settings gains an `agent` field backed by a registry (acp_registry.py, Codex today; Pi and OpenCode slot in once their adapters are validated), and the chat header and message avatars keep showing the selected agent's name and icon. All policy plumbing follows: acp_mode / acp_full_access policies, NBI_ACP_* env vars, and the acp_* setting locks. Claude mode and ACP mode are now mutually exclusive. Enabling one turns the other off at the settings boundary (most-recent selection wins, Claude wins ties per AGENT_MODE_PRIORITY), so the chat-header agent dropdown and the active_chat_agent preference are gone. Auto-disabling Claude also disconnects its live SDK client; the isinstance-gated disconnect at the end of the handler would otherwise skip because the default participant has already switched to ACP. Fix streamed replies rendering one word per line. ACP delivers token-sized deltas; each was streamed as MarkdownData, which the sidebar renders as its own paragraph. MarkdownPartData concatenates consecutive parts into one block. Fix attached files never reaching the agent. The websocket handler appends the turn's context lines (attachments, current-file pointer, output context) to the chat history exactly like Claude mode, but the ACP client sent only request.prompt. assemble_query now joins the turn's user-role lines, and extension.py gives ACP mode the same context framing and per-turn history slicing as Claude mode (is_agent_session_mode). Add new-session and session history. The header's new-chat button now shows in ACP mode and starts a fresh session on the running agent (ClearChatHistory), serialized against turns via the turn lock. The history button lists the agent's own stored sessions through ACP session/list and resumes via session/load (capability-gated), with NBI context preambles stripped from previews. A timed-out load forces a client restart so a delayed session swap cannot leak a replayed conversation into a later turn. Verified live in JupyterLab against codex-acp 0.16.0 with a real OpenAI key: single-paragraph rendering, attachment read via the agent's own tool, fresh-session isolation, resume recall, and the settings-panel exclusivity flip. --- README.md | 45 +- docs/admin-guide.md | 122 +++--- notebook_intelligence/acp_agent.py | 418 +++++++++++++------ notebook_intelligence/acp_registry.py | 116 +++++ notebook_intelligence/ai_service_manager.py | 98 ++--- notebook_intelligence/config.py | 12 +- notebook_intelligence/extension.py | 302 ++++++++++---- notebook_intelligence/feature_flags.py | 20 +- pyproject.toml | 9 +- src/api.ts | 67 ++- src/chat-sidebar.tsx | 91 ++-- src/components/agent-select.tsx | 162 ------- src/components/claude-session-picker.tsx | 56 ++- src/components/settings-panel.tsx | 157 ++++--- src/index.ts | 2 +- src/tour/tour-steps.ts | 6 +- style/base.css | 91 +--- tests/test_acp_agent.py | 122 +++++- tests/test_ai_service_manager_integration.py | 57 ++- tests/test_cell_output_features_response.py | 26 +- tests/test_image_context.py | 12 + tests/test_websocket_handler_integration.py | 10 + 22 files changed, 1194 insertions(+), 807 deletions(-) create mode 100644 notebook_intelligence/acp_registry.py delete mode 100644 src/components/agent-select.tsx diff --git a/README.md b/README.md index faf6b923..ee3be4e4 100644 --- a/README.md +++ b/README.md @@ -105,7 +105,7 @@ Bypass Permissions never persists: starting a **New chat session** (or `/clear`) Choosing **Bypass Permissions** does not arm it immediately. It opens a confirmation step; only after you confirm does bypass take effect, and while it is active the shield turns into a red warning icon as a persistent indicator. Bypass must be re-armed each session: starting a new chat or restarting the Claude client drops back to Default. And because the server re-checks the requested mode on every message, an armed bypass can never outlive a policy that an administrator has since turned off. -**Admin gating.** Bypass Permissions is **off by default** and hidden from the selector unless an administrator enables it: it is governed by the `claude_bypass_permissions` policy (`NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY`), one of three admin policies that default to `force-off` rather than `user-choice` (the others gate the experimental Codex mode and Codex full access). The requested mode is also clamped on the server for every message, so the gate can't be bypassed by a hand-crafted request. Independently, NBI honors Claude Code's enterprise [managed settings](https://code.claude.com/docs/en/settings): `permissions.disableBypassPermissionsMode` removes the option regardless of the NBI policy, and `permissions.defaultMode` sets the selector's starting mode (Bypass excepted, since it never auto-arms). See [Allowing Bypass Permissions](docs/admin-guide.md#allowing-bypass-permissions-in-the-claude-permission-mode-selector) in the admin guide. +**Admin gating.** Bypass Permissions is **off by default** and hidden from the selector unless an administrator enables it: it is governed by the `claude_bypass_permissions` policy (`NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY`), one of three admin policies that default to `force-off` rather than `user-choice` (the others gate the experimental ACP agent mode and ACP full access). The requested mode is also clamped on the server for every message, so the gate can't be bypassed by a hand-crafted request. Independently, NBI honors Claude Code's enterprise [managed settings](https://code.claude.com/docs/en/settings): `permissions.disableBypassPermissionsMode` removes the option regardless of the NBI policy, and `permissions.defaultMode` sets the selector's starting mode (Bypass excepted, since it never auto-arms). See [Allowing Bypass Permissions](docs/admin-guide.md#allowing-bypass-permissions-in-the-claude-permission-mode-selector) in the admin guide. The `/enter-plan-mode` and `/exit-plan-mode` slash commands still work if typed but are no longer offered in autocomplete; the selector replaces them and will retire the commands in a future release. @@ -196,26 +196,26 @@ Most settings panel toggles can be locked by org administrators. Two shapes: **Boolean policies** use the `*_POLICY` suffix and accept three values: `user-choice` (default — user toggles freely), `force-on` (locked enabled), `force-off` (locked disabled). When forced, the panel control is disabled with a "Locked by your administrator" tooltip and any client-side write is ignored. -| Env var | Locks the Settings panel control for | -| ---------------------------------------------- | -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `NBI_EXPLAIN_ERROR_POLICY` | "Explain cell errors" | -| `NBI_OUTPUT_FOLLOWUP_POLICY` | "Ask about cell outputs" | -| `NBI_OUTPUT_TOOLBAR_POLICY` | "Show output toolbar" | -| `NBI_CLAUDE_MODE_POLICY` | "Enable Claude mode" | -| `NBI_CODEX_MODE_POLICY` | "Enable Codex mode" (experimental ACP agent mode; defaults to `force-off`, set `user-choice` to allow it) | -| `NBI_CODEX_FULL_ACCESS_POLICY` | "Full access" for Codex: run tools without asking (defaults to `force-off`, so Codex asks before anything beyond trusted read-only commands; `user-choice` lets users opt into unattended runs) | -| `NBI_CLAUDE_CONTINUE_CONVERSATION_POLICY` | "Remember conversation history" | -| `NBI_CLAUDE_CODE_TOOLS_POLICY` | "Claude Code tools" | -| `NBI_CLAUDE_JUPYTER_UI_TOOLS_POLICY` | "Jupyter UI tools" | -| `NBI_CLAUDE_SETTING_SOURCE_USER_POLICY` | Setting source: User | -| `NBI_CLAUDE_SETTING_SOURCE_PROJECT_POLICY` | Setting source: Project | -| `NBI_STORE_GITHUB_ACCESS_TOKEN_POLICY` | "Remember my GitHub Copilot access token" | -| `NBI_SKILLS_MANAGEMENT_POLICY` | The Skills tab (force-off hides it and 403s the API; also disables the managed-skills reconciler) | -| `NBI_CLAUDE_MCP_MANAGEMENT_POLICY` | The Claude-mode MCP Servers tab (force-off hides it and 403s `/claude-mcp/*`; independent of the non-Claude MCP Servers tab) | -| `NBI_CLAUDE_PLUGINS_MANAGEMENT_POLICY` | The Claude-mode Plugins tab (force-off hides it and 403s `/plugins/*`) | -| `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY` | "Bypass Permissions" in the Claude permission-mode selector (defaults to `force-off`, as do `NBI_CODEX_MODE_POLICY` and `NBI_CODEX_FULL_ACCESS_POLICY`; `user-choice` exposes the option, which the user still arms per session) | -| `NBI_TERMINAL_DRAG_DROP_POLICY` | Terminal drag-drop file attach feature | -| `NBI_REFRESH_OPEN_FILES_ON_DISK_CHANGE_POLICY` | "Refresh open files when changed on disk" | +| Env var | Locks the Settings panel control for | +| ---------------------------------------------- | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `NBI_EXPLAIN_ERROR_POLICY` | "Explain cell errors" | +| `NBI_OUTPUT_FOLLOWUP_POLICY` | "Ask about cell outputs" | +| `NBI_OUTPUT_TOOLBAR_POLICY` | "Show output toolbar" | +| `NBI_CLAUDE_MODE_POLICY` | "Enable Claude mode" | +| `NBI_ACP_MODE_POLICY` | "Enable ACP mode" (experimental agent mode over the Agent Client Protocol, with Codex as the first agent type; defaults to `force-off`, set `user-choice` to allow it) | +| `NBI_ACP_FULL_ACCESS_POLICY` | "Full access" for the ACP agent: run tools without asking (defaults to `force-off`, so the agent asks before anything beyond trusted read-only commands; `user-choice` lets users opt into unattended runs) | +| `NBI_CLAUDE_CONTINUE_CONVERSATION_POLICY` | "Remember conversation history" | +| `NBI_CLAUDE_CODE_TOOLS_POLICY` | "Claude Code tools" | +| `NBI_CLAUDE_JUPYTER_UI_TOOLS_POLICY` | "Jupyter UI tools" | +| `NBI_CLAUDE_SETTING_SOURCE_USER_POLICY` | Setting source: User | +| `NBI_CLAUDE_SETTING_SOURCE_PROJECT_POLICY` | Setting source: Project | +| `NBI_STORE_GITHUB_ACCESS_TOKEN_POLICY` | "Remember my GitHub Copilot access token" | +| `NBI_SKILLS_MANAGEMENT_POLICY` | The Skills tab (force-off hides it and 403s the API; also disables the managed-skills reconciler) | +| `NBI_CLAUDE_MCP_MANAGEMENT_POLICY` | The Claude-mode MCP Servers tab (force-off hides it and 403s `/claude-mcp/*`; independent of the non-Claude MCP Servers tab) | +| `NBI_CLAUDE_PLUGINS_MANAGEMENT_POLICY` | The Claude-mode Plugins tab (force-off hides it and 403s `/plugins/*`) | +| `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY` | "Bypass Permissions" in the Claude permission-mode selector (defaults to `force-off`, as do `NBI_ACP_MODE_POLICY` and `NBI_ACP_FULL_ACCESS_POLICY`; `user-choice` exposes the option, which the user still arms per session) | +| `NBI_TERMINAL_DRAG_DROP_POLICY` | Terminal drag-drop file attach feature | +| `NBI_REFRESH_OPEN_FILES_ON_DISK_CHANGE_POLICY` | "Refresh open files when changed on disk" | The first three also have matching traitlets on `NotebookIntelligence` (`explain_error_policy`, `output_followup_policy`, `output_toolbar_policy`); add the others as needed in the same shape: @@ -240,6 +240,9 @@ Per-user preferences (default on for the cell-output features) live in `config.j | `NBI_CLAUDE_INLINE_COMPLETION_MODEL` | Claude → Auto-complete model | | `ANTHROPIC_API_KEY` | Claude → API Key (input is locked + blanked; the SDK reads the env directly) | | `ANTHROPIC_BASE_URL` | Claude → Base URL | +| `NBI_ACP_CHAT_MODEL` | ACP → Chat model | +| `OPENAI_API_KEY` | ACP → API Key (input is locked + blanked; the agent reads the env directly) | +| `OPENAI_BASE_URL` | ACP → Base URL | Provider IDs: `github-copilot`, `openai-compatible`, `litellm-compatible`, `ollama`, `none`. The `*_MODEL_ID` value is whatever the chosen provider exposes (e.g. `gpt-4o`, `llama3:latest`). Claude model IDs are the literal IDs from the Anthropic API (e.g. `claude-opus-4-7`, `claude-sonnet-4-6`); empty string = "Default (recommended)"; `NBI_CLAUDE_INLINE_COMPLETION_MODEL` also accepts `none` (no inline completion in Claude mode) or `inherit` (use the General-tab Auto-complete model). diff --git a/docs/admin-guide.md b/docs/admin-guide.md index b744ea1f..b148ca5c 100644 --- a/docs/admin-guide.md +++ b/docs/admin-guide.md @@ -75,57 +75,57 @@ If users share a home directory across nodes (NFS-backed shared HPC, classroom l The full surface, in one table. -| Name | Type | Default | Source | Purpose | -| ------------------------------------------------ | ---- | --------------------------- | ---------------------------------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | -| `disabled_providers` | List | `[]` | traitlet on `NotebookIntelligence` | Hide providers from the user dropdown. Values: `github-copilot`, `ollama`, `litellm-compatible`, `openai-compatible`. | -| `allow_enabling_providers_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_PROVIDERS` re-enables hidden providers per pod. | -| `NBI_ENABLED_PROVIDERS` | csv | unset | env | Comma-separated provider IDs to re-enable. Effective only when `allow_enabling_providers_with_env=True`. | -| `disabled_tools` | List | `[]` | traitlet | Hide built-in tools from agent mode. Values listed in [Restricting features](#restricting-features-for-managed-deployments). | -| `allow_enabling_tools_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_BUILTIN_TOOLS` re-enables hidden tools per pod. | -| `NBI_ENABLED_BUILTIN_TOOLS` | csv | unset | env | Comma-separated tool IDs to re-enable. Effective only when `allow_enabling_tools_with_env=True`. | -| `disabled_coding_agent_launchers` | List | `[]` | traitlet | Hide JupyterLab launcher tiles for coding-agent CLIs even when the CLI is on `PATH`. Valid IDs: `claude-code`, `opencode`, `pi`, `github-copilot-cli`, `codex`. See [Disabling coding-agent launcher tiles](#disabling-coding-agent-launcher-tiles). | -| `allow_enabling_coding_agent_launchers_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_CODING_AGENT_LAUNCHERS` re-enables hidden tiles per pod. | -| `NBI_ENABLED_CODING_AGENT_LAUNCHERS` | csv | unset | env | Comma-separated launcher IDs to re-enable. Effective only when `allow_enabling_coding_agent_launchers_with_env=True`. | -| `enable_chat_feedback` | Bool | `False` | traitlet | Enables thumbs-up/down UI in chat and emits in-process `telemetry` events. | -| `enable_chat_feedback_always_visible` | Bool | `False` | traitlet | Renders thumbs-up/down buttons at full opacity on every assistant reply instead of revealing them only on hover. Requires `enable_chat_feedback=True`. | -| `additional_skipped_workspace_directories` | List | `[]` | traitlet | Extra directory names to skip in the chat-sidebar @-mention workspace file picker. Merged with the built-in skips (`__pycache__`, `node_modules`). Match is by directory name only, case-sensitive. | -| `NBI_ADDITIONAL_SKIPPED_WORKSPACE_DIRECTORIES` | csv | unset | env (appends to traitlet) | Comma-separated extra directory names. Resolved at server startup and concatenated with the traitlet value, so a spawn profile can add to (rather than replace) the org-wide list. | -| `allow_github_skill_import` | Bool | `True` | traitlet | When `False`, hides the **Import from GitHub** button in the Skills panel and rejects `/skills/import` POSTs with 403. Does not affect the managed-skills reconciler. | -| `NBI_ALLOW_GITHUB_SKILL_IMPORT` | bool | unset | env (overrides traitlet) | Per-pod override for `allow_github_skill_import`. Accepts `true`/`false`/`1`/`0`/`yes`/`no`/`on`/`off` (case-insensitive). Useful for varying the policy across spawn profiles. | -| `skills_manifest` | str | `""` | traitlet | URL or filesystem path to a managed-skills manifest, or a comma-separated list of either. Manifests are unioned with first-wins URL dedupe; name collisions surface as per-entry errors. See [`docs/skills.md`](skills.md#managed-skills-via-an-org-manifest). | -| `NBI_SKILLS_MANIFEST` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `skills_manifest_interval` | int | `86400` | traitlet | Seconds between reconciles. | -| `NBI_SKILLS_MANIFEST_INTERVAL` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `managed_skills_token` | str | `""` | traitlet | Bearer token for managed-skills GitHub fetches. | -| `NBI_MANAGED_SKILLS_TOKEN` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `allow_github_plugin_import` | Bool | `True` | traitlet | When `False`, hides the "From GitHub" affordance in the Plugins panel and rejects `claude plugin marketplace add` requests whose source resolves as a GitHub URL or `owner/repo` shorthand. Local-path and arbitrary-URL sources remain available. | -| `NBI_ALLOW_GITHUB_PLUGIN_IMPORT` | bool | unset | env (overrides traitlet) | Per-pod override for `allow_github_plugin_import`. Accepts `true`/`false`/`1`/`0`/`yes`/`no`/`on`/`off` (case-insensitive). | -| `skill_max_archive_mb` | Int | `100` | traitlet | Per-archive on-wire size cap (megabytes) for skill bundles fetched from GitHub. Applies to both user imports and managed-skills tarballs. `0` disables the cap. | -| `NBI_SKILL_MAX_ARCHIVE_MB` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `upload_max_mb` | Int | `50` | traitlet | Per-file size cap (megabytes) for the shared upload endpoint used by chat-sidebar attachments and terminal drag-drop. Requests over the cap return HTTP 413. `0` disables the cap. | -| `NBI_UPLOAD_MAX_MB` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `upload_retention_hours` | Int | `24` | traitlet | How long staged uploads survive in the temp directory before the next upload sweeps them. `0` keeps only the atexit purge (uploads survive the session). | -| `NBI_UPLOAD_RETENTION_HOURS` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `mcp_stdio_command_allowlist` | List | `[]` | traitlet | Regex allowlist for the stdio MCP server `command` field. Empty list (default) means no enforcement; non-empty list rejects any stdio MCP server whose command does not match. Matched with `re.search`; anchor (`^...$`) for literal equality. Applies at both Claude `mcp add` and `mcp.json` load. See [Restricting MCP stdio commands](#restricting-mcp-stdio-commands). | -| `NBI_MCP_STDIO_COMMAND_ALLOWLIST` | csv | unset | env (appends to traitlet) | Comma-separated regex patterns added to the traitlet at startup. Per-pod additions on an org baseline. | -| `tour_config_path` | str | `""` | traitlet | Filesystem path to a YAML/JSON file with admin overrides for the first-run sidebar tour copy. See [`docs/admin-tour-config.md`](admin-tour-config.md). | -| `NBI_TOUR_CONFIG_PATH` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | -| `NBI_GH_ACCESS_TOKEN_PASSWORD` | str | `nbi-access-token-password` | env | Password used to encrypt the stored Copilot token in `user-data.json`. **Change in multi-tenant deployments.** | -| `NBI_REFUSE_DEFAULT_TOKEN_PASSWORD_ON_SHARED_FS` | bool | unset | env | When set, refuse to write `user-data.json` if the default `NBI_GH_ACCESS_TOKEN_PASSWORD` is still in use AND `~/.jupyter/nbi/` is readable by group or other. Opt-in to preserve backwards compatibility on single-user deployments where the directory mode is incidental. | -| `NBI_ALLOW_DEFAULT_TOKEN_PASSWORD` | bool | unset | env | Per-pod opt-out that disengages the refuse-on-shared-fs guard above. Admins who knowingly accept the risk (e.g., during a transition before rolling out a per-user password) set this so writes continue. | -| `NBI_RULES_AUTO_RELOAD` | bool | `true` | env | When `false`, ruleset edits require a JupyterLab restart to take effect. | -| `NBI_CLAUDE_CLI_PATH` | str | unset | env | Absolute path to the Claude Code CLI binary. When unset, NBI looks up `claude` on `PATH`. | -| `NBI_OPENCODE_CLI_PATH` | str | unset | env | Absolute path to the opencode CLI. When unset, NBI looks up `opencode` on `PATH`. Gates the opencode launcher tile. | -| `NBI_PI_CLI_PATH` | str | unset | env | Absolute path to the Pi CLI. When unset, NBI looks up `pi` on `PATH`. Gates the Pi launcher tile. | -| `NBI_GITHUB_COPILOT_CLI_PATH` | str | unset | env | Absolute path to the GitHub Copilot CLI. When unset, NBI looks up `copilot` on `PATH`. Gates the GitHub Copilot launcher tile. | -| `NBI_CODEX_CLI_PATH` | str | unset | env | Absolute path to the OpenAI Codex CLI. When unset, NBI looks up `codex` on `PATH`. Gates the Codex launcher tile. | -| `NBI_GHE_SUBDOMAIN` | str | `""` | env | GitHub Enterprise subdomain for GitHub Copilot users on a GHE tenant. Empty selects github.com. | -| `NBI_GITHUB_ENTERPRISE_HOSTS` | csv | `""` | env | Comma-separated hostnames the plugin marketplace detector treats as GitHub. Cookie-domain shape: bare token (`github.acme.com`) matches exactly; leading-dot token (`.acme.com`) matches any subdomain of `acme.com`. Independent of `NBI_GHE_SUBDOMAIN`, which only configures the Copilot OAuth tenant. Required so `allow_github_plugin_import = False` actually gates GHE marketplace adds and so the `GITHUB_TOKEN` / `gh auth token` chain injects on GHE sources. | -| `NBI_LOG_LEVEL` | str | `INFO` | env | Python logging level for the `notebook_intelligence` logger. | -| `LITELLM_LOCAL_MODEL_COST_MAP` | bool | `true` (NBI default) | env | litellm setting that NBI defaults to `true` when it loads litellm, so litellm reads the model-cost map bundled with the installed package instead of fetching it over HTTP (litellm's own default), which stalls on proxied networks. Set to `false` before starting JupyterLab to restore litellm's remote fetch. | -| `NBI_DISABLE_OUTPUT_SCRUB` | bool | unset | env | When set (`1` / `true` / `yes` / `on`), disables the shell-tool output scrubber so raw stdout/stderr (including any env-var values that leak) is sent through to chat. Default off; the scrubber redacts values for sensitive-named env vars (`TOKEN`, `SECRET`, `API_KEY`, ...) plus tokens with well-known credential prefixes (`ghp_`, `sk-ant-`, `AKIA`, ...). Opt out only when debugging credential helpers where the redaction interferes. | -| `GITHUB_TOKEN`, `GH_TOKEN` | str | unset | env | Used (in that order) by user-initiated skill imports and GitHub-sourced plugin marketplace adds for GitHub auth. Falls back to `gh` CLI auth. | -| `NBI_*_POLICY` | str | `user-choice` | env | Lock individual Settings panel toggles. See [README → Admin policies](../README.md#admin-policies) for the full list of `*_POLICY` env vars and matching traitlets, including `NBI_SKILLS_MANAGEMENT_POLICY`, `NBI_CLAUDE_MCP_MANAGEMENT_POLICY`, `NBI_CLAUDE_PLUGINS_MANAGEMENT_POLICY`, `NBI_TERMINAL_DRAG_DROP_POLICY`, and `NBI_REFRESH_OPEN_FILES_ON_DISK_CHANGE_POLICY`. Three exceptions to the `user-choice` default all default to `force-off`: `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY` (see [Allowing Bypass Permissions](#allowing-bypass-permissions-in-the-claude-permission-mode-selector)), `NBI_CODEX_MODE_POLICY` (the experimental Codex agent mode), and `NBI_CODEX_FULL_ACCESS_POLICY` (Codex running tools without asking). | +| Name | Type | Default | Source | Purpose | +| ------------------------------------------------ | ---- | --------------------------- | ---------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | +| `disabled_providers` | List | `[]` | traitlet on `NotebookIntelligence` | Hide providers from the user dropdown. Values: `github-copilot`, `ollama`, `litellm-compatible`, `openai-compatible`. | +| `allow_enabling_providers_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_PROVIDERS` re-enables hidden providers per pod. | +| `NBI_ENABLED_PROVIDERS` | csv | unset | env | Comma-separated provider IDs to re-enable. Effective only when `allow_enabling_providers_with_env=True`. | +| `disabled_tools` | List | `[]` | traitlet | Hide built-in tools from agent mode. Values listed in [Restricting features](#restricting-features-for-managed-deployments). | +| `allow_enabling_tools_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_BUILTIN_TOOLS` re-enables hidden tools per pod. | +| `NBI_ENABLED_BUILTIN_TOOLS` | csv | unset | env | Comma-separated tool IDs to re-enable. Effective only when `allow_enabling_tools_with_env=True`. | +| `disabled_coding_agent_launchers` | List | `[]` | traitlet | Hide JupyterLab launcher tiles for coding-agent CLIs even when the CLI is on `PATH`. Valid IDs: `claude-code`, `opencode`, `pi`, `github-copilot-cli`, `codex`. See [Disabling coding-agent launcher tiles](#disabling-coding-agent-launcher-tiles). | +| `allow_enabling_coding_agent_launchers_with_env` | Bool | `False` | traitlet | If true, `NBI_ENABLED_CODING_AGENT_LAUNCHERS` re-enables hidden tiles per pod. | +| `NBI_ENABLED_CODING_AGENT_LAUNCHERS` | csv | unset | env | Comma-separated launcher IDs to re-enable. Effective only when `allow_enabling_coding_agent_launchers_with_env=True`. | +| `enable_chat_feedback` | Bool | `False` | traitlet | Enables thumbs-up/down UI in chat and emits in-process `telemetry` events. | +| `enable_chat_feedback_always_visible` | Bool | `False` | traitlet | Renders thumbs-up/down buttons at full opacity on every assistant reply instead of revealing them only on hover. Requires `enable_chat_feedback=True`. | +| `additional_skipped_workspace_directories` | List | `[]` | traitlet | Extra directory names to skip in the chat-sidebar @-mention workspace file picker. Merged with the built-in skips (`__pycache__`, `node_modules`). Match is by directory name only, case-sensitive. | +| `NBI_ADDITIONAL_SKIPPED_WORKSPACE_DIRECTORIES` | csv | unset | env (appends to traitlet) | Comma-separated extra directory names. Resolved at server startup and concatenated with the traitlet value, so a spawn profile can add to (rather than replace) the org-wide list. | +| `allow_github_skill_import` | Bool | `True` | traitlet | When `False`, hides the **Import from GitHub** button in the Skills panel and rejects `/skills/import` POSTs with 403. Does not affect the managed-skills reconciler. | +| `NBI_ALLOW_GITHUB_SKILL_IMPORT` | bool | unset | env (overrides traitlet) | Per-pod override for `allow_github_skill_import`. Accepts `true`/`false`/`1`/`0`/`yes`/`no`/`on`/`off` (case-insensitive). Useful for varying the policy across spawn profiles. | +| `skills_manifest` | str | `""` | traitlet | URL or filesystem path to a managed-skills manifest, or a comma-separated list of either. Manifests are unioned with first-wins URL dedupe; name collisions surface as per-entry errors. See [`docs/skills.md`](skills.md#managed-skills-via-an-org-manifest). | +| `NBI_SKILLS_MANIFEST` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `skills_manifest_interval` | int | `86400` | traitlet | Seconds between reconciles. | +| `NBI_SKILLS_MANIFEST_INTERVAL` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `managed_skills_token` | str | `""` | traitlet | Bearer token for managed-skills GitHub fetches. | +| `NBI_MANAGED_SKILLS_TOKEN` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `allow_github_plugin_import` | Bool | `True` | traitlet | When `False`, hides the "From GitHub" affordance in the Plugins panel and rejects `claude plugin marketplace add` requests whose source resolves as a GitHub URL or `owner/repo` shorthand. Local-path and arbitrary-URL sources remain available. | +| `NBI_ALLOW_GITHUB_PLUGIN_IMPORT` | bool | unset | env (overrides traitlet) | Per-pod override for `allow_github_plugin_import`. Accepts `true`/`false`/`1`/`0`/`yes`/`no`/`on`/`off` (case-insensitive). | +| `skill_max_archive_mb` | Int | `100` | traitlet | Per-archive on-wire size cap (megabytes) for skill bundles fetched from GitHub. Applies to both user imports and managed-skills tarballs. `0` disables the cap. | +| `NBI_SKILL_MAX_ARCHIVE_MB` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `upload_max_mb` | Int | `50` | traitlet | Per-file size cap (megabytes) for the shared upload endpoint used by chat-sidebar attachments and terminal drag-drop. Requests over the cap return HTTP 413. `0` disables the cap. | +| `NBI_UPLOAD_MAX_MB` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `upload_retention_hours` | Int | `24` | traitlet | How long staged uploads survive in the temp directory before the next upload sweeps them. `0` keeps only the atexit purge (uploads survive the session). | +| `NBI_UPLOAD_RETENTION_HOURS` | int | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `mcp_stdio_command_allowlist` | List | `[]` | traitlet | Regex allowlist for the stdio MCP server `command` field. Empty list (default) means no enforcement; non-empty list rejects any stdio MCP server whose command does not match. Matched with `re.search`; anchor (`^...$`) for literal equality. Applies at both Claude `mcp add` and `mcp.json` load. See [Restricting MCP stdio commands](#restricting-mcp-stdio-commands). | +| `NBI_MCP_STDIO_COMMAND_ALLOWLIST` | csv | unset | env (appends to traitlet) | Comma-separated regex patterns added to the traitlet at startup. Per-pod additions on an org baseline. | +| `tour_config_path` | str | `""` | traitlet | Filesystem path to a YAML/JSON file with admin overrides for the first-run sidebar tour copy. See [`docs/admin-tour-config.md`](admin-tour-config.md). | +| `NBI_TOUR_CONFIG_PATH` | str | unset | env (overrides traitlet) | Same as above; env takes precedence. | +| `NBI_GH_ACCESS_TOKEN_PASSWORD` | str | `nbi-access-token-password` | env | Password used to encrypt the stored Copilot token in `user-data.json`. **Change in multi-tenant deployments.** | +| `NBI_REFUSE_DEFAULT_TOKEN_PASSWORD_ON_SHARED_FS` | bool | unset | env | When set, refuse to write `user-data.json` if the default `NBI_GH_ACCESS_TOKEN_PASSWORD` is still in use AND `~/.jupyter/nbi/` is readable by group or other. Opt-in to preserve backwards compatibility on single-user deployments where the directory mode is incidental. | +| `NBI_ALLOW_DEFAULT_TOKEN_PASSWORD` | bool | unset | env | Per-pod opt-out that disengages the refuse-on-shared-fs guard above. Admins who knowingly accept the risk (e.g., during a transition before rolling out a per-user password) set this so writes continue. | +| `NBI_RULES_AUTO_RELOAD` | bool | `true` | env | When `false`, ruleset edits require a JupyterLab restart to take effect. | +| `NBI_CLAUDE_CLI_PATH` | str | unset | env | Absolute path to the Claude Code CLI binary. When unset, NBI looks up `claude` on `PATH`. | +| `NBI_OPENCODE_CLI_PATH` | str | unset | env | Absolute path to the opencode CLI. When unset, NBI looks up `opencode` on `PATH`. Gates the opencode launcher tile. | +| `NBI_PI_CLI_PATH` | str | unset | env | Absolute path to the Pi CLI. When unset, NBI looks up `pi` on `PATH`. Gates the Pi launcher tile. | +| `NBI_GITHUB_COPILOT_CLI_PATH` | str | unset | env | Absolute path to the GitHub Copilot CLI. When unset, NBI looks up `copilot` on `PATH`. Gates the GitHub Copilot launcher tile. | +| `NBI_CODEX_CLI_PATH` | str | unset | env | Absolute path to the OpenAI Codex CLI. When unset, NBI looks up `codex` on `PATH`. Gates the Codex launcher tile. | +| `NBI_GHE_SUBDOMAIN` | str | `""` | env | GitHub Enterprise subdomain for GitHub Copilot users on a GHE tenant. Empty selects github.com. | +| `NBI_GITHUB_ENTERPRISE_HOSTS` | csv | `""` | env | Comma-separated hostnames the plugin marketplace detector treats as GitHub. Cookie-domain shape: bare token (`github.acme.com`) matches exactly; leading-dot token (`.acme.com`) matches any subdomain of `acme.com`. Independent of `NBI_GHE_SUBDOMAIN`, which only configures the Copilot OAuth tenant. Required so `allow_github_plugin_import = False` actually gates GHE marketplace adds and so the `GITHUB_TOKEN` / `gh auth token` chain injects on GHE sources. | +| `NBI_LOG_LEVEL` | str | `INFO` | env | Python logging level for the `notebook_intelligence` logger. | +| `LITELLM_LOCAL_MODEL_COST_MAP` | bool | `true` (NBI default) | env | litellm setting that NBI defaults to `true` when it loads litellm, so litellm reads the model-cost map bundled with the installed package instead of fetching it over HTTP (litellm's own default), which stalls on proxied networks. Set to `false` before starting JupyterLab to restore litellm's remote fetch. | +| `NBI_DISABLE_OUTPUT_SCRUB` | bool | unset | env | When set (`1` / `true` / `yes` / `on`), disables the shell-tool output scrubber so raw stdout/stderr (including any env-var values that leak) is sent through to chat. Default off; the scrubber redacts values for sensitive-named env vars (`TOKEN`, `SECRET`, `API_KEY`, ...) plus tokens with well-known credential prefixes (`ghp_`, `sk-ant-`, `AKIA`, ...). Opt out only when debugging credential helpers where the redaction interferes. | +| `GITHUB_TOKEN`, `GH_TOKEN` | str | unset | env | Used (in that order) by user-initiated skill imports and GitHub-sourced plugin marketplace adds for GitHub auth. Falls back to `gh` CLI auth. | +| `NBI_*_POLICY` | str | `user-choice` | env | Lock individual Settings panel toggles. See [README → Admin policies](../README.md#admin-policies) for the full list of `*_POLICY` env vars and matching traitlets, including `NBI_SKILLS_MANAGEMENT_POLICY`, `NBI_CLAUDE_MCP_MANAGEMENT_POLICY`, `NBI_CLAUDE_PLUGINS_MANAGEMENT_POLICY`, `NBI_TERMINAL_DRAG_DROP_POLICY`, and `NBI_REFRESH_OPEN_FILES_ON_DISK_CHANGE_POLICY`. Three exceptions to the `user-choice` default all default to `force-off`: `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY` (see [Allowing Bypass Permissions](#allowing-bypass-permissions-in-the-claude-permission-mode-selector)), `NBI_ACP_MODE_POLICY` (the experimental ACP agent mode), and `NBI_ACP_FULL_ACCESS_POLICY` (the ACP agent running tools without asking). | Configure traitlets in `jupyter_server_config.py`: @@ -569,24 +569,24 @@ c.NotebookIntelligence.claude_bypass_permissions_policy = "user-choice" Or via env: `NBI_CLAUDE_BYPASS_PERMISSIONS_POLICY=user-choice`. -The Claude permission-mode selector (Default / Accept Edits / Plan) always offers its three normal modes; all three keep NBI's tool-call confirmation flow. "Bypass Permissions" removes that confirmation entirely and runs every tool call with the Jupyter user's full access, so it is **disabled by default**: this is one of three policies whose default is `force-off` rather than `user-choice` (the others gate the experimental Codex agent mode and Codex full access, #378). Setting `user-choice` exposes the option, which the user must still arm explicitly per session through a confirm step; the armed state never persists across sessions or reconnects. `force-on` grants the same availability as `user-choice` and never auto-arms bypass. +The Claude permission-mode selector (Default / Accept Edits / Plan) always offers its three normal modes; all three keep NBI's tool-call confirmation flow. "Bypass Permissions" removes that confirmation entirely and runs every tool call with the Jupyter user's full access, so it is **disabled by default**: this is one of three policies whose default is `force-off` rather than `user-choice` (the others gate the experimental ACP agent mode and ACP full access, #378). Setting `user-choice` exposes the option, which the user must still arm explicitly per session through a confirm step; the armed state never persists across sessions or reconnects. `force-on` grants the same availability as `user-choice` and never auto-arms bypass. Independent of this policy, NBI refuses bypass whenever Claude Code's enterprise [managed settings](https://docs.anthropic.com/en/docs/claude-code/settings) set `permissions.disableBypassPermissionsMode`, and honors a managed `permissions.defaultMode` as the selector's starting mode (except bypass, which never starts armed). The clamp is enforced server-side at the request boundary, not just in the UI. -### Gating the experimental Codex agent (#378) +### Gating the experimental ACP agent (#378) -Codex mode runs an external agent (the codex-acp adapter) over the Agent Client Protocol. The admin controls that apply: +ACP mode runs an external coding agent over the Agent Client Protocol; Codex (via the codex-acp adapter) is the first selectable agent type. ACP mode and Claude mode are mutually exclusive: enabling one disables the other on save, and a hand-edited config that enables both resolves to Claude. The admin controls that apply: -- **`NBI_CODEX_MODE_POLICY`** (default `force-off`): whether Codex mode can be enabled at all. While `force-off`, the Codex settings tab is hidden and `enabled` is clamped to false on every config write, so it is the nuclear off switch. -- **`NBI_CODEX_FULL_ACCESS_POLICY`** (default `force-off`): whether Codex may run tools without asking. While `force-off`, NBI pins the agent to `approval_policy = untrusted`, so it asks before anything beyond trusted read-only commands and each request surfaces through NBI's per-tool confirmation. `user-choice` exposes the "Full access" toggle in the Codex tab; `force-on` requires unattended runs. Like bypass, the value is clamped server-side, so a hand-crafted config write can't relax it. -- **Credentials and model** follow the same env-override locks as Claude: `OPENAI_API_KEY`, `OPENAI_BASE_URL`, and `NBI_CODEX_CHAT_MODEL` pin and hide the corresponding fields. +- **`NBI_ACP_MODE_POLICY`** (default `force-off`): whether ACP mode can be enabled at all. While `force-off`, the ACP settings tab is hidden and `enabled` is clamped to false on every config write, so it is the nuclear off switch. +- **`NBI_ACP_FULL_ACCESS_POLICY`** (default `force-off`): whether the agent may run tools without asking. While `force-off`, NBI pins Codex to `approval_policy = untrusted`, so it asks before anything beyond trusted read-only commands and each request surfaces through NBI's per-tool confirmation. `user-choice` exposes the "Full access" toggle in the ACP tab; `force-on` requires unattended runs. Like bypass, the value is clamped server-side, so a hand-crafted config write can't relax it. +- **Credentials and model** follow the same env-override locks as Claude: `OPENAI_API_KEY`, `OPENAI_BASE_URL`, and `NBI_ACP_CHAT_MODEL` pin and hide the corresponding fields. -How the pin is applied: NBI passes `approval_policy` to codex via the codex-acp binary's `-c key=value` command-line override, which takes precedence over the codex config file (`$CODEX_HOME/config.toml`). When an API key is configured, NBI also runs codex with an isolated `CODEX_HOME` under the NBI user directory, so the codex configuration base is NBI-controlled and neither the workspace's nor the user's `~/.codex` config is read (this isolates configuration only; the agent still operates on workspace files, subject to the approval pin). With ChatGPT (OAuth) auth, codex uses the user's own `~/.codex`; the `-c` pin still overrides its top-level `approval_policy`, but a user who controls that home directory is also the account the agent runs as. On shared or locked-down deployments, prefer API-key auth (so `CODEX_HOME` is isolated) and keep `NBI_CODEX_MODE_POLICY=force-off` unless Codex is explicitly needed. +How the pin is applied: NBI passes `approval_policy` to codex via the codex-acp binary's `-c key=value` command-line override, which takes precedence over the codex config file (`$CODEX_HOME/config.toml`). When an API key is configured, NBI also runs codex with an isolated `CODEX_HOME` under the NBI user directory, so the codex configuration base is NBI-controlled and neither the workspace's nor the user's `~/.codex` config is read (this isolates configuration only; the agent still operates on workspace files, subject to the approval pin). With ChatGPT (OAuth) auth, codex uses the user's own `~/.codex`; the `-c` pin still overrides its top-level `approval_policy`, but a user who controls that home directory is also the account the agent runs as. On shared or locked-down deployments, prefer API-key auth (so `CODEX_HOME` is isolated) and keep `NBI_ACP_MODE_POLICY=force-off` unless the agent is explicitly needed. Known limitations: -- NBI gates Codex through the approval posture above and through its per-tool confirmation, but it does not enforce a tool allowlist inside the agent the way Claude's tool policies do. The agent owns its own tool loop and NBI only mediates the approvals the agent chooses to request. -- The pin relies on codex honoring the command-line `-c approval_policy` override above any configuration it loads. NBI does not parse or strip codex's own config; if a future codex version applies a higher-precedence per-project setting, that assumption would need revisiting. Keep `NBI_CODEX_MODE_POLICY=force-off` where these constraints are not acceptable. +- NBI gates the agent through the approval posture above and through its per-tool confirmation, but it does not enforce a tool allowlist inside the agent the way Claude's tool policies do. The agent owns its own tool loop and NBI only mediates the approvals the agent chooses to request. +- The pin relies on codex honoring the command-line `-c approval_policy` override above any configuration it loads. NBI does not parse or strip codex's own config; if a future codex version applies a higher-precedence per-project setting, that assumption would need revisiting. Keep `NBI_ACP_MODE_POLICY=force-off` where these constraints are not acceptable. ### Disabling terminal drag-drop file attach @@ -706,6 +706,8 @@ All routes live under `/notebook-intelligence/`. All require Jupyter authenticat | `/notebook-intelligence/upload-file` | POST | Upload a file to attach as chat context (size and retention governed by `upload_max_mb` / `upload_retention_hours`). | | `/notebook-intelligence/claude-sessions` | GET | List Claude Code sessions for the working directory. | | `/notebook-intelligence/claude-sessions/resume` | POST | Resume a Claude session. | +| `/notebook-intelligence/acp-sessions` | GET | List the ACP agent's stored sessions for the working directory (via the agent's `session/list`). | +| `/notebook-intelligence/acp-sessions/resume` | POST | Resume an ACP session (via the agent's `session/load`). | | `/notebook-intelligence/claude-mcp` | GET/POST | List or add Claude-mode MCP servers. Gated by `claude_mcp_management_policy`. | | `/notebook-intelligence/claude-mcp//` | GET/DELETE | Get or remove a Claude-mode MCP server by scope (user/project/local) and name. | | `/notebook-intelligence/plugins` | GET/POST | List or install Claude plugins. Gated by `claude_plugins_management_policy`. | diff --git a/notebook_intelligence/acp_agent.py b/notebook_intelligence/acp_agent.py index 1b833797..2007e57b 100644 --- a/notebook_intelligence/acp_agent.py +++ b/notebook_intelligence/acp_agent.py @@ -1,10 +1,13 @@ # Copyright (c) Mehmet Bektas -"""Codex agent mode over the Agent Client Protocol (issue #378, Phase 1). +"""ACP agent mode over the Agent Client Protocol (issue #378, Phase 1). A second, off-by-default agent mode that drives the chat panel like Claude mode for the core loop (streaming, tool-call cards with diffs, per-tool approval), -backed by ``codex-acp`` over ACP instead of the Claude SDK. +backed by an ACP agent adapter instead of the Claude SDK. Agent types are +described by ``ACP_AGENTS``; Codex (via ``codex-acp``) is the first entry and +further agents (e.g. Pi, OpenCode) slot in as registry additions once their +adapters are validated. Design notes (validated by the Phase 0 spike under ``spikes/acp-codex/``): @@ -14,13 +17,12 @@ is answered cross-thread through ``wait_for_chat_user_input`` (the same poll a signal pattern Claude uses). - ``agent-client-protocol`` is imported here, and this module is only imported - when Codex mode is enabled, so the dependency loads lazily (matching #370). + when ACP mode is enabled, so the dependency loads lazily (matching #370). - Phase 1 is deliberately allowed to duplicate a little of Claude's card/diff wiring; Phase 2 extracts the shared layer. """ import asyncio -import base64 import concurrent.futures import difflib import logging @@ -28,6 +30,7 @@ import sys import threading import time +from datetime import datetime from typing import Any, Optional import acp @@ -39,49 +42,27 @@ ChatResponse, Host, MarkdownData, + MarkdownPartData, ProgressData, ToolCallData, ConfirmationData, ) +from notebook_intelligence.acp_registry import ( + AcpAgentSpec, + codex_approval_args, + resolve_acp_agent, + resolve_acp_agent_command, +) from notebook_intelligence.base_chat_participant import BaseChatParticipant from notebook_intelligence.util import ThreadSafeWebSocketConnector, get_jupyter_root_dir log = logging.getLogger(__name__) -CODEX_AGENT_CHAT_PARTICIPANT_ID = "codex" -# Pinned in Phase 1 (the version the spike validated); revisit per release. -CODEX_ACP_PACKAGE = "@zed-industries/codex-acp@0.16.0" +ACP_AGENT_CHAT_PARTICIPANT_ID = "acp-agent" _START_TIMEOUT = 60 # seconds to bring up codex-acp + a session _RESPONSE_TIMEOUT = 30 * 60 # a turn can be long; cap so the UI doesn't hang forever _POLL = 0.2 -# The OpenAI mark, matching style/icons/openai.svg. Rendered as an in the -# chat (the response avatar), so the fill is pinned to OpenAI blue rather than -# currentColor, mirroring how the Claude participant pins Anthropic orange. -_CODEX_ICON_SVG = ( - '' - '' -) -CODEX_AGENT_ICON_URL = ( - "data:image/svg+xml;base64," - + base64.b64encode(_CODEX_ICON_SVG.encode("utf-8")).decode("utf-8") -) - _MAX_DIFF_LINES = 60 @@ -125,41 +106,6 @@ def _nbi_status(acp_status: Optional[str]) -> str: return "in_progress" # pending / in_progress / None -def resolve_codex_acp_command() -> list[str]: - """The command that launches codex-acp. - - ``NBI_CODEX_ACP_COMMAND`` overrides (shell-split); otherwise run the pinned - package via ``npx``. Kept separate from the Claude CLI resolver because - codex-acp is an npm package, not a binary on PATH. - """ - override = os.environ.get("NBI_CODEX_ACP_COMMAND", "").strip() - if override: - import shlex - return shlex.split(override) - return ["npx", "-y", CODEX_ACP_PACKAGE] - - -def codex_approval_args(full_access: bool) -> list[str]: - """Codex config overrides that pin its approval posture. - - Default (``full_access`` off) forces ``approval_policy = untrusted`` so - Codex asks before anything beyond trusted read-only commands, surfacing the - request through NBI's per-tool confirmation. ``full_access`` (gated by the - force-off ``codex_full_access`` admin policy) lets it run unattended. The - flag is honored by the codex-acp binary's ``-c key=value`` override, so it - works for both API-key and ChatGPT-auth sessions. - - The override takes precedence over the codex config file. In the API-key - path NBI also isolates ``CODEX_HOME`` (see ``_child_env``), so the config - base is NBI-controlled and neither the workspace nor the user's ~/.codex is - read. With ChatGPT auth, codex uses the user's own ~/.codex; the ``-c`` - pin still overrides its top-level approval_policy. See the admin guide for - the residual caveat on shared deployments. - """ - policy = "never" if full_access else "untrusted" - return ["-c", f'approval_policy="{policy}"'] - - class _NbiAcpClient(acp.Client): """The editor side of ACP. Maps agent events onto NBI's chat surfaces.""" @@ -181,11 +127,14 @@ async def session_update(self, session_id, update, **kw): elif su == "agent_message_chunk": text = _block_text(getattr(update, "content", None)) if text: - resp.stream(MarkdownData(content=text)) + # ACP streams token-sized deltas. MarkdownPartData because the + # frontend concatenates consecutive parts into one block; + # MarkdownData would render every delta as its own paragraph. + resp.stream(MarkdownPartData(content=text)) elif su == "agent_thought_chunk": text = _block_text(getattr(update, "content", None)) if text: - resp.stream(MarkdownData(reasoning_content=text)) + resp.stream(MarkdownPartData(reasoning_content=text)) elif su == "available_commands_update": self._owner.available_commands = [ getattr(c, "name", "") for c in (update.available_commands or []) @@ -224,11 +173,12 @@ async def request_permission(self, options, session_id, tool_call, **kw): ) callback_id = f"acp-perm-{tool_call.tool_call_id}" title = getattr(tool_call, "title", None) or "Run this tool?" + agent_label = self._owner.agent_spec.label resp.stream(ConfirmationData( - title="Codex tool call", + title=f"{agent_label} tool call", message=( f"Approve: {title}?\n\n" - "Codex decides which tools to ask about, so some actions may run " + f"{agent_label} decides which tools to ask about, so some actions may run " "without a prompt." ), confirmArgs={"id": resp.message_id, "data": { @@ -292,6 +242,35 @@ def _block_text(block) -> str: return getattr(block, "text", "") or "" +def _epoch_from_iso(value) -> float: + """ISO-8601 timestamp (as ACP's ``updatedAt``) to epoch seconds, else 0.""" + if not value: + return 0 + if isinstance(value, datetime): + return value.timestamp() + try: + return datetime.fromisoformat(str(value).replace("Z", "+00:00")).timestamp() + except ValueError: + return 0 + + +def _strip_context_preamble(title: str) -> str: + """Drop NBI's leading context lines from an agent-stored session title. + + The agent titles a session with its first prompt, which NBI prefixes with + context lines (current-directory pointer, attachments). The preview should + show the user's actual first question, like the Claude picker does. + """ + from notebook_intelligence.claude_sessions import NBI_CONTEXT_PREFIX + lines = [line for line in title.splitlines() if line.strip()] + while lines and ( + lines[0].startswith(NBI_CONTEXT_PREFIX) + or lines[0].startswith("The user attached ") + ): + lines.pop(0) + return " ".join(lines) if lines else title + + def _diffs_from_content(content) -> list[dict]: out: list[dict] = [] remaining = _MAX_DIFF_LINES @@ -311,8 +290,8 @@ def _diffs_from_content(content) -> list[dict]: class AcpAgentClient: - """Persistent ACP client: one codex-acp subprocess + session on a worker - thread, prompted once per chat request.""" + """Persistent ACP client: one agent-adapter subprocess + session on a + worker thread, prompted once per chat request.""" def __init__(self, host: Host): self._host = host @@ -332,15 +311,29 @@ def __init__(self, host: Host): self._shutdown = None # asyncio.Event, created on the loop self._shutting_down = False self._client: Optional[_NbiAcpClient] = None + self._agent_capabilities = None self._lock = threading.Lock() # Serializes turns: the ACP session runs one prompt at a time, and # current_response is shared, so a second concurrent turn must not # interleave with the first. self._turn_lock = threading.Lock() + def _mcp_servers(self) -> list: + """The NBI MCP server config passed to every session create/load.""" + return [ + schema.McpServerStdio( + name="nbi", command=sys.executable, + args=["-m", "notebook_intelligence.acp_mcp_server"], env=[], + ) + ] + + @property + def acp_settings(self) -> dict: + return self._host.nbi_config.acp_settings + @property - def codex_settings(self) -> dict: - return self._host.nbi_config.codex_settings + def agent_spec(self) -> AcpAgentSpec: + return resolve_acp_agent(self.acp_settings.get("agent")) def _ensure_started(self) -> bool: with self._lock: @@ -359,11 +352,11 @@ def _ensure_started(self) -> bool: self._start_error = None self._shutting_down = False self._thread = threading.Thread( - target=self._thread_main, name="nbi-codex-acp", daemon=True + target=self._thread_main, name="nbi-acp-agent", daemon=True ) self._thread.start() if not self._started.wait(timeout=_START_TIMEOUT): - self._start_error = self._start_error or "Codex agent did not start in time" + self._start_error = self._start_error or "ACP agent did not start in time" return False return self._start_error is None @@ -371,7 +364,7 @@ def _thread_main(self): try: asyncio.run(self._serve()) except Exception as e: - self._start_error = f"Codex agent failed to start: {e}" + self._start_error = f"ACP agent failed to start: {e}" log.error(self._start_error, exc_info=True) self._started.set() @@ -379,11 +372,14 @@ async def _serve(self): self._loop = asyncio.get_running_loop() self._shutdown = asyncio.Event() workdir = get_jupyter_root_dir() - env = self._child_env() - cmd = resolve_codex_acp_command() + codex_approval_args( - bool(self.codex_settings.get("full_access", False)) - ) - log.info("Starting codex-acp: %s", " ".join(cmd)) + spec = self.agent_spec + env = self._child_env(spec) + cmd = list(resolve_acp_agent_command(spec)) + if spec.id == "codex": + cmd += codex_approval_args( + bool(self.acp_settings.get("full_access", False)) + ) + log.info("Starting ACP agent (%s): %s", spec.id, " ".join(cmd)) try: self._proc = await asyncio.create_subprocess_exec( *cmd, stdin=asyncio.subprocess.PIPE, stdout=asyncio.subprocess.PIPE, @@ -401,16 +397,15 @@ async def _serve(self): ), client_info=schema.Implementation(name="notebook-intelligence", version="1.0.0"), ) + self._agent_capabilities = getattr(init, "agent_capabilities", None) await self._authenticate(init) - mcp = schema.McpServerStdio( - name="nbi", command=sys.executable, - args=["-m", "notebook_intelligence.acp_mcp_server"], env=[], + sess = await self._conn.new_session( + cwd=workdir, mcp_servers=self._mcp_servers() ) - sess = await self._conn.new_session(cwd=workdir, mcp_servers=[mcp]) self._session_id = sess.session_id - log.info("codex-acp session ready: %s", self._session_id) + log.info("ACP agent session ready: %s", self._session_id) except Exception as e: - self._start_error = f"Codex agent session failed: {e}" + self._start_error = f"ACP agent session failed: {e}" log.error(self._start_error, exc_info=True) self._started.set() await self._teardown() @@ -421,32 +416,39 @@ async def _serve(self): finally: await self._teardown() - def _child_env(self) -> dict: + def _api_key(self, spec: AcpAgentSpec) -> str: + return (self.acp_settings.get("api_key") or "").strip() \ + or os.environ.get(spec.api_key_env, "").strip() + + def _child_env(self, spec: AcpAgentSpec) -> dict: env = {k: v for k, v in os.environ.items() if k != "CLAUDECODE" and not k.startswith("CLAUDE_CODE_")} - api_key = (self.codex_settings.get("api_key") or "").strip() \ - or os.environ.get("OPENAI_API_KEY", "").strip() + api_key = self._api_key(spec) if api_key: - env["OPENAI_API_KEY"] = api_key - # Use a clean config dir so a stale ChatGPT OAuth token doesn't - # shadow API-key auth (the spike hit refresh_token_reused). - env["CODEX_HOME"] = os.path.join( - self._host.nbi_config.nbi_user_dir, "codex-home" - ) - os.makedirs(env["CODEX_HOME"], exist_ok=True) + env[spec.api_key_env] = api_key + if spec.id == "codex": + # Use a clean config dir so a stale ChatGPT OAuth token doesn't + # shadow API-key auth (the spike hit refresh_token_reused). + env["CODEX_HOME"] = os.path.join( + self._host.nbi_config.nbi_user_dir, "codex-home" + ) + os.makedirs(env["CODEX_HOME"], exist_ok=True) return env async def _authenticate(self, init): methods = [m.id for m in (init.auth_methods or [])] if not methods: return - api_key = (self.codex_settings.get("api_key") or "").strip() \ - or os.environ.get("OPENAI_API_KEY", "").strip() - method = "openai-api-key" if (api_key and "openai-api-key" in methods) else methods[0] + spec = self.agent_spec + method = ( + spec.auth_method + if (self._api_key(spec) and spec.auth_method in methods) + else methods[0] + ) try: await self._conn.authenticate(method_id=method) except Exception as e: - log.warning("codex-acp authenticate(%s) failed: %s", method, e) + log.warning("ACP agent authenticate(%s) failed: %s", method, e) async def _drain_stderr(self): while self._proc and self._proc.stderr: @@ -455,7 +457,7 @@ async def _drain_stderr(self): break s = line.decode(errors="replace").rstrip() if s: - log.debug("codex-acp: %s", s[:200]) + log.debug("acp-agent: %s", s[:200]) async def _teardown(self): if self._proc and self._proc.returncode is None: @@ -476,6 +478,7 @@ async def _teardown(self): self._conn = None self._session_id = None self._client = None + self._agent_capabilities = None self._proc = None self._loop = None @@ -490,7 +493,150 @@ async def _cancel(self): if self._conn and self._session_id: await self._conn.cancel(session_id=self._session_id) except Exception as e: - log.debug("codex-acp cancel failed: %s", e) + log.debug("ACP cancel failed: %s", e) + + def _run_on_loop(self, coro, timeout: float): + """Run ``coro`` on the ACP loop from a foreign thread and wait.""" + loop = self._loop + if loop is None: + coro.close() + raise RuntimeError("agent is not running") + fut = asyncio.run_coroutine_threadsafe(coro, loop) + return fut.result(timeout=timeout) + + def new_session(self) -> Optional[str]: + """Start a fresh session on the running agent (the header's new-chat). + + Returns an error string on failure, else None. When the agent is not + running there is nothing to clear — the next turn starts fresh — so + that case is a silent no-op rather than a spurious subprocess launch. + """ + with self._lock: + running = ( + self._thread is not None + and self._thread.is_alive() + and not self._shutting_down + ) + if not running or self._loop is None: + return None + # Serialize against turns: swapping the session id mid-prompt would + # interleave two conversations. The frontend cancels any in-flight + # request before asking for a new session, so this acquire is brief. + with self._turn_lock: + try: + self._run_on_loop(self._new_session_coro(), _START_TIMEOUT) + return None + except Exception as e: + log.warning("ACP new session failed, restarting client: %s", e) + # The session state is unknown at this point; force a restart + # so the next turn gets a clean one. + self.shutdown() + return f"Failed to start a new session: {e}" + + async def _new_session_coro(self): + sess = await self._conn.new_session( + cwd=get_jupyter_root_dir(), mcp_servers=self._mcp_servers() + ) + self._session_id = sess.session_id + if self._client is not None: + self._client._tool_state.clear() + log.info("ACP agent session ready: %s", self._session_id) + + def list_sessions(self) -> tuple[list[dict], Optional[str]]: + """List the agent's stored sessions for the current workspace. + + Returns ``(sessions, error)``; sessions are newest-first dicts shaped + for the session picker. ``session/list`` is an optional ACP extension + (codex-acp implements it); an agent without it reports a friendly + error instead of raising. + """ + if not self._ensure_started(): + return [], self._start_error or "Agent is not available" + try: + result = self._run_on_loop(self._conn.list_sessions(), 30) + except acp.RequestError as e: + # Only JSON-RPC method-not-found means the capability is missing; + # anything else is a real failure and must not masquerade as one. + if getattr(e, "code", None) == -32601: + return [], "This agent does not support listing sessions" + log.warning("ACP session/list failed: %s", e) + return [], f"Failed to list sessions: {e}" + except Exception as e: + log.warning("ACP session/list failed: %s", e) + return [], f"Failed to list sessions: {e}" + cwd = os.path.realpath(get_jupyter_root_dir()) + sessions = [] + for s in getattr(result, "sessions", None) or []: + s_cwd = getattr(s, "cwd", "") or "" + if os.path.realpath(s_cwd) != cwd: + continue + sessions.append({ + "session_id": getattr(s, "session_id", ""), + "preview": _strip_context_preamble(getattr(s, "title", "") or ""), + "modified_at": _epoch_from_iso(getattr(s, "updated_at", None)), + }) + sessions.sort(key=lambda s: s["modified_at"], reverse=True) + return sessions, None + + def load_session(self, session_id: str) -> Optional[str]: + """Resume a stored session. Returns an error string on failure.""" + if not self._ensure_started(): + return self._start_error or "Agent is not available" + caps = self._agent_capabilities + if not getattr(caps, "load_session", False): + return "This agent does not support resuming sessions" + with self._turn_lock: + try: + self._run_on_loop(self._load_session_coro(session_id), _START_TIMEOUT) + return None + except Exception as e: + log.warning("ACP session/load failed: %s", e) + # A timed-out load may still complete later on the loop: it + # would swap _session_id mid-turn and its replayed + # session_update notifications would stream a resumed + # conversation into whatever turn is then active. The session + # state is unknown either way, so force a restart (mirrors + # new_session's failure path). + self.shutdown() + return f"Failed to resume session: {e}" + + async def _load_session_coro(self, session_id: str): + # The agent replays the resumed conversation through session_update + # notifications during load. current_response is None outside a turn, + # so the replay is deliberately not re-rendered — the sidebar shows a + # "resumed" notice instead, exactly like Claude-mode resume. + await self._conn.load_session( + cwd=get_jupyter_root_dir(), session_id=session_id, + mcp_servers=self._mcp_servers(), + ) + self._session_id = session_id + if self._client is not None: + self._client._tool_state.clear() + log.info("ACP agent session resumed: %s", session_id) + + @staticmethod + def assemble_query(request: ChatRequest) -> str: + """Join this turn's user-role lines into the prompt sent to the agent. + + The websocket handler appends the turn's context lines (attachment + @-mentions, current-file pointer, output context) to the chat history + before the prompt, exactly like Claude mode. Sending only + ``request.prompt`` would silently drop whatever the user just + attached. A trailing slash command drops the context lines instead -- + they are meaningless to commands like ``/compact`` and could break + their parsing (mirrors the Claude-mode join). + """ + query_lines = [] + for msg in request.chat_history: + if msg.get("role") == "user": + content = msg.get("content", "") + if isinstance(content, str) and content: + query_lines.append(content) + if not query_lines: + return request.prompt + if query_lines[-1].startswith("/"): + query_lines = query_lines[-1:] + return "\n".join(line.strip() for line in query_lines) def query(self, request: ChatRequest, response: ChatResponse) -> Optional[str]: """Run one turn. Returns an error string on failure, else None. @@ -499,13 +645,13 @@ def query(self, request: ChatRequest, response: ChatResponse) -> Optional[str]: turn streams on the ACP loop thread. """ if not self._turn_lock.acquire(blocking=False): - return "Codex agent is busy with another request" + return f"{self.agent_spec.label} is busy with another request" try: if not self._ensure_started(): - return self._start_error or "Codex agent is not available" + return self._start_error or f"{self.agent_spec.label} agent is not available" loop = self._loop if loop is None: - return "Codex agent is not available" + return f"{self.agent_spec.label} agent is not available" # Fresh turn: drop the prior turn's accumulated tool-call cards so # the per-id merge cache does not grow without bound. if self._client is not None: @@ -513,10 +659,10 @@ def query(self, request: ChatRequest, response: ChatResponse) -> Optional[str]: self.current_response = response try: fut = asyncio.run_coroutine_threadsafe( - self._run_prompt(request.prompt), loop + self._run_prompt(self.assemble_query(request)), loop ) except RuntimeError as e: - return f"Codex agent is not available: {e}" + return f"{self.agent_spec.label} agent is not available: {e}" start = time.time() try: while True: @@ -529,11 +675,11 @@ def query(self, request: ChatRequest, response: ChatResponse) -> Optional[str]: except concurrent.futures.TimeoutError: pass except Exception as e: - log.error("codex-acp turn failed: %s", e, exc_info=True) - return f"Codex agent error: {e}" + log.error("ACP agent turn failed: %s", e, exc_info=True) + return f"{self.agent_spec.label} agent error: {e}" if time.time() - start > _RESPONSE_TIMEOUT: self._schedule(self._cancel(), loop) - return "Codex agent response timeout" + return f"{self.agent_spec.label} agent response timeout" finally: self.current_response = None finally: @@ -556,27 +702,38 @@ def shutdown(self): pass -class CodexAgentChatParticipant(BaseChatParticipant): +class AcpAgentChatParticipant(BaseChatParticipant): + """The chat participant for ACP mode. + + The participant id is stable; the display name, description, and icon + follow the agent type selected in ``acp_settings`` so the chat header and + message avatars always show which agent is answering. + """ + def __init__(self, host: Host): super().__init__() self._host = host self._client = AcpAgentClient(host) + @property + def _spec(self) -> AcpAgentSpec: + return self._client.agent_spec + @property def id(self) -> str: - return CODEX_AGENT_CHAT_PARTICIPANT_ID + return ACP_AGENT_CHAT_PARTICIPANT_ID @property def name(self) -> str: - return "Codex" + return self._spec.label @property def description(self) -> str: - return "OpenAI Codex (via the Agent Client Protocol)" + return self._spec.description @property def icon_path(self) -> str: - return CODEX_AGENT_ICON_URL + return self._spec.icon_url @property def commands(self) -> list[ChatCommand]: @@ -593,11 +750,24 @@ def websocket_connector(self, connector: ThreadSafeWebSocketConnector): def restart_client(self) -> None: """Stop the running ACP client so the next request restarts it. - Codex reads its credentials and model from codex_settings at launch; a - settings change only takes effect on a fresh subprocess. + The agent reads its credentials and model from acp_settings at launch; + a settings change only takes effect on a fresh subprocess. """ self._client.shutdown() + def clear_chat_history(self) -> Optional[str]: + """Start a fresh agent session (the header's new-chat button). + + Returns an error string on failure, else None. + """ + return self._client.new_session() + + def list_sessions(self) -> tuple[list[dict], Optional[str]]: + return self._client.list_sessions() + + def resume_session(self, session_id: str) -> Optional[str]: + return self._client.load_session(session_id) + def chat_prompt(self, model_provider: str, model_name: str) -> str: return "" @@ -609,9 +779,9 @@ async def handle_chat_request( response.stream(ProgressData("Thinking…")) result = self._client.query(request, response) if isinstance(result, str) and result: - response.stream(MarkdownData(content=f"**Codex agent error:** {result}")) + response.stream(MarkdownData(content=f"**{self._spec.label} agent error:** {result}")) except Exception as e: - log.error("Error handling Codex chat request: %s", e, exc_info=True) + log.error("Error handling ACP chat request: %s", e, exc_info=True) try: response.stream(MarkdownData(content=f"**Error:** {e}")) except Exception: diff --git a/notebook_intelligence/acp_registry.py b/notebook_intelligence/acp_registry.py new file mode 100644 index 00000000..155feaf0 --- /dev/null +++ b/notebook_intelligence/acp_registry.py @@ -0,0 +1,116 @@ +# Copyright (c) Mehmet Bektas + +"""Registry of ACP agent types selectable in ACP mode (issue #378). + +Kept separate from ``acp_agent`` so surfaces that only need agent metadata +(the capabilities response feeding the settings dropdown) can import it +without pulling in the ``acp`` package -- that dependency stays off the +startup path and loads only when ACP mode is actually used. + +Adding an agent type is a registry entry: its adapter package, display +metadata, and how it authenticates. Agent-specific launch quirks that do not +generalize (Codex's approval-policy pin, its CODEX_HOME isolation) stay keyed +off the spec id in ``acp_agent`` rather than growing fields prematurely. +""" + +import base64 +import os +from dataclasses import dataclass +from typing import Optional + +# Pinned in Phase 1 (the version the spike validated); revisit per release. +CODEX_ACP_PACKAGE = "@zed-industries/codex-acp@0.16.0" + +# The OpenAI mark, matching style/icons/openai.svg. Rendered as an in the +# chat (the response avatar), so the fill is pinned to OpenAI blue rather than +# currentColor, mirroring how the Claude participant pins Anthropic orange. +_CODEX_ICON_SVG = ( + '' + '' +) +CODEX_AGENT_ICON_URL = ( + "data:image/svg+xml;base64," + + base64.b64encode(_CODEX_ICON_SVG.encode("utf-8")).decode("utf-8") +) + + +@dataclass(frozen=True) +class AcpAgentSpec: + """One selectable agent type for ACP mode.""" + id: str + label: str + description: str + package: str # npx package spec that launches the ACP adapter + icon_url: str + api_key_env: str # env var the agent reads its API key from + auth_method: str # preferred ACP auth method id when a key is present + + +ACP_AGENTS: dict[str, AcpAgentSpec] = { + "codex": AcpAgentSpec( + id="codex", + label="Codex", + description="OpenAI Codex (via the Agent Client Protocol)", + package=CODEX_ACP_PACKAGE, + icon_url=CODEX_AGENT_ICON_URL, + api_key_env="OPENAI_API_KEY", + auth_method="openai-api-key", + ), +} +DEFAULT_ACP_AGENT = "codex" + + +def resolve_acp_agent(agent_id: Optional[str]) -> AcpAgentSpec: + """The spec for ``agent_id``, falling back to the default for unknown or + missing ids so a stale config value cannot break the mode.""" + return ACP_AGENTS.get(agent_id or "", None) or ACP_AGENTS[DEFAULT_ACP_AGENT] + + +def resolve_acp_agent_command(spec: AcpAgentSpec) -> list[str]: + """The command that launches the agent's ACP adapter. + + ``NBI_ACP_AGENT_COMMAND`` overrides (shell-split); otherwise run the + spec's pinned package via ``npx``. Kept separate from the Claude CLI + resolver because the adapters are npm packages, not binaries on PATH. + """ + override = os.environ.get("NBI_ACP_AGENT_COMMAND", "").strip() + if override: + import shlex + return shlex.split(override) + return ["npx", "-y", spec.package] + + +def codex_approval_args(full_access: bool) -> list[str]: + """Codex config overrides that pin its approval posture. + + Default (``full_access`` off) forces ``approval_policy = untrusted`` so + Codex asks before anything beyond trusted read-only commands, surfacing the + request through NBI's per-tool confirmation. ``full_access`` (gated by the + force-off ``acp_full_access`` admin policy) lets it run unattended. The + flag is honored by the codex-acp binary's ``-c key=value`` override, so it + works for both API-key and ChatGPT-auth sessions. + + The override takes precedence over the codex config file. In the API-key + path NBI also isolates ``CODEX_HOME`` (see ``AcpAgentClient._child_env``), + so the config base is NBI-controlled and neither the workspace nor the + user's ~/.codex is read. With ChatGPT auth, codex uses the user's own + ~/.codex; the ``-c`` pin still overrides its top-level approval_policy. + See the admin guide for the residual caveat on shared deployments. + """ + policy = "never" if full_access else "untrusted" + return ["-c", f'approval_policy="{policy}"'] diff --git a/notebook_intelligence/ai_service_manager.py b/notebook_intelligence/ai_service_manager.py index 395dce4d..ac4f6c96 100644 --- a/notebook_intelligence/ai_service_manager.py +++ b/notebook_intelligence/ai_service_manager.py @@ -132,15 +132,15 @@ def websocket_connector(self, _websocket_connector: ThreadSafeWebSocketConnector self._websocket_connector = _websocket_connector self._mcp_manager.websocket_connector = _websocket_connector self._claude_code_chat_participant.websocket_connector = _websocket_connector - if self._codex_chat_participant is not None: - self._codex_chat_participant.websocket_connector = _websocket_connector + if self._acp_chat_participant is not None: + self._acp_chat_participant.websocket_connector = _websocket_connector def initialize(self): self.chat_participants = {} self._claude_code_chat_participant = ClaudeCodeChatParticipant(self) - # Created lazily the first time Codex mode is active so the ACP SDK + # Created lazily the first time ACP mode is active so the ACP SDK # import stays off the startup path (#378). - self._codex_chat_participant = None + self._acp_chat_participant = None self.register_llm_provider(GitHubCopilotLLMProvider()) self.register_llm_provider(self._openai_compatible_llm_provider) self.register_llm_provider(self._litellm_compatible_llm_provider) @@ -226,32 +226,32 @@ def update_models_from_config(self): else: self.unregister_chat_participant(self._claude_code_chat_participant) - # Codex agent mode (#378). is_codex_mode already reflects the resolved + # ACP agent mode (#378). is_acp_mode already reflects the resolved # active agent, so it is exclusive with is_claude_code_mode. - if self.is_codex_mode: - self._default_chat_participant = self._ensure_codex_participant() - elif self._codex_chat_participant is not None: - self.unregister_chat_participant(self._codex_chat_participant) + if self.is_acp_mode: + self._default_chat_participant = self._ensure_acp_participant() + elif self._acp_chat_participant is not None: + self.unregister_chat_participant(self._acp_chat_participant) self.chat_participants[DEFAULT_CHAT_PARTICIPANT_ID] = self._default_chat_participant - def _ensure_codex_participant(self): - """Create (lazily, to defer the ACP import) and register the Codex - participant, returning it (#378).""" - if self._codex_chat_participant is None: - from notebook_intelligence.acp_agent import CodexAgentChatParticipant - self._codex_chat_participant = CodexAgentChatParticipant(self) - self._codex_chat_participant.websocket_connector = self._websocket_connector - if self._codex_chat_participant.id not in self.chat_participants: - self.register_chat_participant(self._codex_chat_participant) - return self._codex_chat_participant - - def restart_codex_client(self): - """Drop the running ACP subprocess (if any) so the next Codex request - relaunches it with the current codex_settings (#378). No-op when the + def _ensure_acp_participant(self): + """Create (lazily, to defer the ACP import) and register the ACP + agent participant, returning it (#378).""" + if self._acp_chat_participant is None: + from notebook_intelligence.acp_agent import AcpAgentChatParticipant + self._acp_chat_participant = AcpAgentChatParticipant(self) + self._acp_chat_participant.websocket_connector = self._websocket_connector + if self._acp_chat_participant.id not in self.chat_participants: + self.register_chat_participant(self._acp_chat_participant) + return self._acp_chat_participant + + def restart_acp_client(self): + """Drop the running ACP subprocess (if any) so the next request + relaunches it with the current acp_settings (#378). No-op when the participant has never been created.""" - if self._codex_chat_participant is not None: - self._codex_chat_participant.restart_client() + if self._acp_chat_participant is not None: + self._acp_chat_participant.restart_client() def update_mcp_servers(self): self._mcp_manager.update_mcp_servers(self.nbi_config.mcp) @@ -347,53 +347,41 @@ def inline_completion_model(self) -> InlineCompletionModel: def embedding_model(self) -> EmbeddingModel: return self._embedding_model - # Agent modes that take over chat, in the priority order used to pick a - # default when more than one is enabled. Extends as more ACP agents land - # (e.g. gemini, opencode). Claude is first so the historical "Claude wins - # when both are on" behavior is preserved when the user has no preference. - AGENT_MODE_PRIORITY = ("claude", "codex") + # Agent modes that take over chat. Claude mode and ACP mode are mutually + # exclusive -- enabling one disables the other at the settings boundary + # (ConfigHandler) -- so at most one entry is normally enabled. The + # priority order is a safety net for a hand-edited config that enables + # both: Claude wins, preserving the historical behavior. + AGENT_MODE_PRIORITY = ("claude", "acp") def _agent_mode_enabled(self, mode: str) -> bool: if mode == "claude": return bool(self.nbi_config.claude_settings.get("enabled", False)) - if mode == "codex": - return bool(self.nbi_config.codex_settings.get("enabled", False)) + if mode == "acp": + return bool(self.nbi_config.acp_settings.get("enabled", False)) return False @property def enabled_agent_modes(self) -> list[str]: return [m for m in self.AGENT_MODE_PRIORITY if self._agent_mode_enabled(m)] - @staticmethod - def resolve_active_agent(enabled: list[str], preferred: Optional[str]) -> Optional[str]: - """Pick the active agent: the preference when it is enabled, else the - highest-priority enabled mode, else None.""" - if not enabled: - return None - if preferred in enabled: - return preferred - return enabled[0] - @property def active_agent_mode(self) -> Optional[str]: """The single agent mode currently handling chat. - When several agent modes are enabled the user's stored preference - (``active_chat_agent``) wins; otherwise the highest-priority enabled - mode is used. Returns None when no agent mode is enabled (the native - provider path handles chat). + Returns None when no agent mode is enabled (the native provider path + handles chat). """ - return self.resolve_active_agent( - self.enabled_agent_modes, self.nbi_config.get("active_chat_agent") - ) + enabled = self.enabled_agent_modes + return enabled[0] if enabled else None @property def is_claude_code_mode(self) -> bool: return self.active_agent_mode == "claude" @property - def is_codex_mode(self) -> bool: - return self.active_agent_mode == "codex" + def is_acp_mode(self) -> bool: + return self.active_agent_mode == "acp" @property def claude_models(self) -> list[dict]: @@ -524,8 +512,8 @@ def get_chat_participant(self, prompt: str) -> ChatParticipant: async def handle_chat_request(self, request: ChatRequest, response: ChatResponse, options: dict = {}) -> None: is_claude_code_mode = self.is_claude_code_mode - is_codex_mode = self.is_codex_mode - if not is_claude_code_mode and not is_codex_mode and self.chat_model is None: + is_acp_mode = self.is_acp_mode + if not is_claude_code_mode and not is_acp_mode and self.chat_model is None: response.stream(MarkdownData("Chat model is not set!")) response.stream(ButtonData("Configure", "notebook-intelligence:open-configuration-dialog")) response.finish() @@ -533,8 +521,8 @@ async def handle_chat_request(self, request: ChatRequest, response: ChatResponse request.host = self if is_claude_code_mode: prompt_parts = PromptParts(input=request.prompt, participant=CLAUDE_CODE_CHAT_PARTICIPANT_ID) - elif is_codex_mode: - prompt_parts = PromptParts(input=request.prompt, participant=self._ensure_codex_participant().id) + elif is_acp_mode: + prompt_parts = PromptParts(input=request.prompt, participant=self._ensure_acp_participant().id) else: prompt_parts = AIServiceManager.parse_prompt(request.prompt) diff --git a/notebook_intelligence/config.py b/notebook_intelligence/config.py index f3e6c0d4..33008660 100644 --- a/notebook_intelligence/config.py +++ b/notebook_intelligence/config.py @@ -10,12 +10,12 @@ from notebook_intelligence.util import get_claude_config_dir from notebook_intelligence.feature_flags import ( + ACP_SETTINGS_OVERRIDES, CHAT_MODEL_OVERRIDES, CLAUDE_SETTINGS_OVERRIDES, - CODEX_SETTINGS_OVERRIDES, INLINE_COMPLETION_MODEL_OVERRIDES, + apply_acp_policies, apply_claude_policies, - apply_codex_policies, apply_string_overrides, ) @@ -175,12 +175,12 @@ def claude_settings(self): ) @property - def codex_settings(self): - resolved = apply_codex_policies( - self.get('codex_settings', {}), self._feature_policies + def acp_settings(self): + resolved = apply_acp_policies( + self.get('acp_settings', {}), self._feature_policies ) return apply_string_overrides( - resolved, self._string_overrides, CODEX_SETTINGS_OVERRIDES + resolved, self._string_overrides, ACP_SETTINGS_OVERRIDES ) @property diff --git a/notebook_intelligence/extension.py b/notebook_intelligence/extension.py index a402b216..be613161 100644 --- a/notebook_intelligence/extension.py +++ b/notebook_intelligence/extension.py @@ -32,7 +32,7 @@ CHAT_MODEL_OVERRIDES, CLAUDE_CODE_TOOLS_ID, CLAUDE_SETTINGS_OVERRIDES, - CODEX_SETTINGS_OVERRIDES, + ACP_SETTINGS_OVERRIDES, INLINE_COMPLETION_MODEL_OVERRIDES, JUPYTER_UI_TOOLS_ID, POLICY_FORCE_OFF, @@ -40,12 +40,13 @@ POLICY_USER_CHOICE, VALID_POLICIES, apply_claude_policies, - apply_codex_policies, + apply_acp_policies, apply_string_overrides, is_force_off, is_locked, resolve_feature_flag, ) +from notebook_intelligence.acp_registry import ACP_AGENTS from notebook_intelligence._claude_cli import validate_scope from notebook_intelligence.mcp_config_validation import ( MCPConfigValidationError, @@ -293,11 +294,11 @@ def _resolve_positive_int_with_env(env_var_name: str, traitlet_value: int) -> in ("output_followup", "NBI_OUTPUT_FOLLOWUP_POLICY", "output_followup_policy"), ("output_toolbar", "NBI_OUTPUT_TOOLBAR_POLICY", "output_toolbar_policy"), ("claude_mode", "NBI_CLAUDE_MODE_POLICY", "claude_mode_policy"), - ("codex_mode", "NBI_CODEX_MODE_POLICY", "codex_mode_policy"), + ("acp_mode", "NBI_ACP_MODE_POLICY", "acp_mode_policy"), ( - "codex_full_access", - "NBI_CODEX_FULL_ACCESS_POLICY", - "codex_full_access_policy", + "acp_full_access", + "NBI_ACP_FULL_ACCESS_POLICY", + "acp_full_access_policy", ), ( "claude_continue_conversation", @@ -364,17 +365,18 @@ def _resolve_positive_int_with_env(env_var_name: str, traitlet_value: int) -> in # Fallback used when a policies dict hasn't been populated (handler classes # before _setup_handlers, direct construction in tests). Everything defaults -# to user-choice except bypass, the experimental Codex mode, and Codex full +# to user-choice except bypass, the experimental ACP mode, and ACP full # access, which must fail closed. FEATURE_POLICY_DEFAULTS = {name: POLICY_USER_CHOICE for name in FEATURE_POLICY_NAMES} FEATURE_POLICY_DEFAULTS["claude_bypass_permissions"] = POLICY_FORCE_OFF -FEATURE_POLICY_DEFAULTS["codex_mode"] = POLICY_FORCE_OFF -FEATURE_POLICY_DEFAULTS["codex_full_access"] = POLICY_FORCE_OFF +FEATURE_POLICY_DEFAULTS["acp_mode"] = POLICY_FORCE_OFF +FEATURE_POLICY_DEFAULTS["acp_full_access"] = POLICY_FORCE_OFF # ``(setting_lock_name, env_var)`` pairs for the value-presence-locks. The # claude_api_key entry maps to ANTHROPIC_API_KEY (the SDK's native convention) -# rather than an NBI-prefixed env var. Same for claude_base_url. The codex_* -# entries follow the same idea against OpenAI's native env vars. +# rather than an NBI-prefixed env var. Same for claude_base_url. The acp_* +# entries follow the same idea against OpenAI's native env vars (the Codex +# agent's convention; the default ACP agent). STRING_OVERRIDE_SPEC = ( ("chat_model_provider", "NBI_CHAT_MODEL_PROVIDER"), ("chat_model_id", "NBI_CHAT_MODEL_ID"), @@ -384,9 +386,9 @@ def _resolve_positive_int_with_env(env_var_name: str, traitlet_value: int) -> in ("claude_inline_completion_model", "NBI_CLAUDE_INLINE_COMPLETION_MODEL"), ("claude_api_key", "ANTHROPIC_API_KEY"), ("claude_base_url", "ANTHROPIC_BASE_URL"), - ("codex_chat_model", "NBI_CODEX_CHAT_MODEL"), - ("codex_api_key", "OPENAI_API_KEY"), - ("codex_base_url", "OPENAI_BASE_URL"), + ("acp_chat_model", "NBI_ACP_CHAT_MODEL"), + ("acp_api_key", "OPENAI_API_KEY"), + ("acp_base_url", "OPENAI_BASE_URL"), ) SETTING_LOCK_NAMES = tuple(name for name, _ in STRING_OVERRIDE_SPEC) @@ -399,7 +401,7 @@ def _build_feature_policies_response(policies: dict, nbi_config) -> dict: feature only needs an entry here plus a matching env-var resolution. """ claude_settings = nbi_config.claude_settings or {} - codex_settings = nbi_config.codex_settings or {} + acp_settings = nbi_config.acp_settings or {} tools = claude_settings.get("tools") or [] sources = claude_settings.get("setting_sources") or [] @@ -408,11 +410,12 @@ def _build_feature_policies_response(policies: dict, nbi_config) -> dict: "output_followup": nbi_config.enable_output_followup, "output_toolbar": nbi_config.enable_output_toolbar, "claude_mode": bool(claude_settings.get("enabled", False)), - # Experimental Codex (ACP) agent mode; defaults to force-off (#378). - "codex_mode": bool(codex_settings.get("enabled", False)), - # Codex autonomous "full access" posture; defaults to force-off so the - # agent asks before risky actions unless an admin opts in (#378). - "codex_full_access": bool(codex_settings.get("full_access", False)), + # Experimental ACP agent mode; defaults to force-off (#378). + "acp_mode": bool(acp_settings.get("enabled", False)), + # ACP agent autonomous "full access" posture; defaults to force-off + # so the agent asks before risky actions unless an admin opts in + # (#378). + "acp_full_access": bool(acp_settings.get("full_access", False)), "claude_continue_conversation": bool( claude_settings.get("continue_conversation", False) ), @@ -431,8 +434,8 @@ def _build_feature_policies_response(policies: dict, nbi_config) -> dict: # Gates only whether the Bypass Permissions option is offered in # the permission-mode selector; the user still arms it per # session. force-on grants the same availability as user-choice - # and never auto-arms bypass. Defaults to force-off, the only - # policy with a non-user-choice default. + # and never auto-arms bypass. Defaults to force-off, like the ACP + # mode and full-access policies. "claude_bypass_permissions": True, "terminal_drag_drop": True, "refresh_open_files_on_disk_change": nbi_config.refresh_open_files_on_disk_change, @@ -487,7 +490,7 @@ def _scrub_credentials_for_wire( """Strip the api_key from the capabilities response when locked by env. The SDKs read their key env var directly (ANTHROPIC_API_KEY for Claude, - OPENAI_API_KEY for Codex); surfacing the value through the frontend would + OPENAI_API_KEY for the Codex ACP agent); surfacing the value through the frontend would leak the credential. """ if not string_overrides.get(api_key_lock): @@ -618,13 +621,16 @@ def is_provider_enabled(provider_id: str) -> bool: "claude_settings": _scrub_credentials_for_wire( nbi_config.claude_settings, self.string_overrides ), - "codex_settings": _scrub_credentials_for_wire( - nbi_config.codex_settings, self.string_overrides, "codex_api_key" + "acp_settings": _scrub_credentials_for_wire( + nbi_config.acp_settings, self.string_overrides, "acp_api_key" ), - # Which agent modes are enabled and which one is currently active. - # The frontend shows an agent picker when more than one is enabled - # and gates the active agent's UI on active_agent_mode (#378). - "enabled_agent_modes": ai_service_manager.enabled_agent_modes, + # The agent types selectable in ACP mode (settings dropdown). + "acp_agents": [ + {"id": spec.id, "label": spec.label} + for spec in ACP_AGENTS.values() + ], + # The single agent mode handling chat ("claude" / "acp" / None); + # Claude mode and ACP mode are mutually exclusive (#378). "active_agent_mode": ai_service_manager.active_agent_mode, "spinner_verbs": _read_claude_spinner_verbs(), "claude_models": ai_service_manager.claude_models, @@ -700,8 +706,7 @@ def post(self): "inline_completion_debouncer_delay", "mcp_server_settings", "claude_settings", - "codex_settings", - "active_chat_agent", + "acp_settings", "enable_explain_error", "enable_output_followup", "enable_output_toolbar", @@ -733,8 +738,15 @@ def post(self): has_model_change = False has_claude_settings_change = False - has_codex_settings_change = False - has_active_agent_change = False + has_acp_settings_change = False + # Captured before the loop so the exclusivity check below can tell + # which mode this POST newly enabled. + prior_claude_enabled = bool( + (ai_service_manager.nbi_config.get("claude_settings") or {}).get("enabled", False) + ) + prior_acp_enabled = bool( + (ai_service_manager.nbi_config.get("acp_settings") or {}).get("enabled", False) + ) for key in data: if key in locked_keys: continue @@ -771,33 +783,23 @@ def post(self): if self.string_overrides.get("claude_api_key"): value = dict(value) value["api_key"] = "" - elif key == "codex_settings": - value = apply_codex_policies(value, self.feature_policies) + elif key == "acp_settings": + value = apply_acp_policies(value, self.feature_policies) value = apply_string_overrides( - value, self.string_overrides, CODEX_SETTINGS_OVERRIDES + value, self.string_overrides, ACP_SETTINGS_OVERRIDES ) # OPENAI_API_KEY is a credential; same handling as Claude above. - if self.string_overrides.get("codex_api_key"): + if self.string_overrides.get("acp_api_key"): value = dict(value) value["api_key"] = "" # Only act when something actually changed. The settings tab # re-POSTs on mount, and an unconditional restart would bounce # the live ACP subprocess every time the tab is opened. Compare - # against the raw stored value (not the codex_settings property, + # against the raw stored value (not the acp_settings property, # which re-injects env overrides such as OPENAI_API_KEY and would # never match the scrubbed value we persist). - has_codex_settings_change = ( - value != (ai_service_manager.nbi_config.get("codex_settings") or {}) - ) - elif key == "active_chat_agent": - # The preferred agent when several agent modes are enabled. - # Reject unknown ids ("" clears the preference); an unknown - # value would just be ignored by the resolver, but persisting - # junk helps nobody. - if value not in ai_service_manager.AGENT_MODE_PRIORITY and value != "": - continue - has_active_agent_change = ( - value != (ai_service_manager.nbi_config.get("active_chat_agent") or "") + has_acp_settings_change = ( + value != (ai_service_manager.nbi_config.get("acp_settings") or {}) ) ai_service_manager.nbi_config.set(key, value) if key == "store_github_access_token": @@ -819,12 +821,46 @@ def post(self): # needed to disconnect default_chat_participant.update_client_debounced() + # Claude mode and ACP mode are mutually exclusive: turning one on + # turns the other off, most-recent selection wins. Enforced here at + # the settings boundary so every client sees the same resolution + # (the frontend toggles merely reflect it after a config refresh). + claude_enabled = bool( + (ai_service_manager.nbi_config.get("claude_settings") or {}).get("enabled", False) + ) + acp_enabled = bool( + (ai_service_manager.nbi_config.get("acp_settings") or {}).get("enabled", False) + ) + if claude_enabled and acp_enabled: + # ACP wins only when this POST newly enabled it and did not also + # newly enable Claude; every tie (both newly on, or a hand-edited + # config with both on) goes to Claude, matching AGENT_MODE_PRIORITY. + acp_newly_enabled = acp_enabled and not prior_acp_enabled + claude_newly_enabled = claude_enabled and not prior_claude_enabled + if acp_newly_enabled and not claude_newly_enabled: + claude_settings = dict(ai_service_manager.nbi_config.get("claude_settings") or {}) + claude_settings["enabled"] = False + ai_service_manager.nbi_config.set("claude_settings", claude_settings) + has_claude_settings_change = True + # Disconnect the live Claude client now: after + # update_models_from_config below, the default participant is + # the ACP one, so the isinstance-gated disconnect at the end + # of this handler would silently skip and leave the Claude + # SDK subprocess running alongside the ACP agent. + default_chat_participant = ai_service_manager.default_chat_participant + if isinstance(default_chat_participant, ClaudeCodeChatParticipant): + default_chat_participant.update_client_debounced() + else: + acp_settings = dict(ai_service_manager.nbi_config.get("acp_settings") or {}) + acp_settings["enabled"] = False + ai_service_manager.nbi_config.set("acp_settings", acp_settings) + has_acp_settings_change = True + ai_service_manager.nbi_config.save() if ( has_model_change or has_claude_settings_change - or has_codex_settings_change - or has_active_agent_change + or has_acp_settings_change ): ai_service_manager.update_models_from_config() if has_claude_settings_change: @@ -832,10 +868,10 @@ def post(self): if isinstance(default_chat_participant, ClaudeCodeChatParticipant): # needed to reconnect / update default_chat_participant.update_client_debounced() - if has_codex_settings_change: - # Codex reads its key/model from codex_settings at launch, so the + if has_acp_settings_change: + # The agent reads its key/model from acp_settings at launch, so the # running ACP subprocess must restart to pick up the new values. - ai_service_manager.restart_codex_client() + ai_service_manager.restart_acp_client() self.finish(json.dumps({})) @@ -1961,6 +1997,97 @@ def post(self): self.finish(json.dumps({"success": True, "session_id": session_id})) + +def _acp_participant_or_none(): + """The active ACP participant, or None when ACP mode is not active.""" + if not ai_service_manager.is_acp_mode: + return None + participant = ai_service_manager.default_chat_participant + if not callable(getattr(participant, "list_sessions", None)): + return None + return participant + + +class AcpSessionsListHandler(APIHandler): + """Lists the ACP agent's stored sessions for the current workspace. + + Backed by the agent's own ``session/list`` (codex-acp implements it), so + the picker shows the same sessions the agent CLI would resume. Unlike the + Claude handler there is no scope switch: the agent call is already + filtered to the Jupyter cwd on the client side. + """ + + @tornado.web.authenticated + async def get(self): + participant = _acp_participant_or_none() + if participant is None: + self.set_status(404) + self.finish(json.dumps({"error": "ACP mode is not enabled"})) + return + try: + # list_sessions may cold-start the agent subprocess (up to a + # minute through npx); keep the IO loop free while it does. + loop = asyncio.get_event_loop() + sessions, error = await loop.run_in_executor( + None, participant.list_sessions + ) + if error: + self.finish(json.dumps({ + "sessions": [], "current_cwd": "", "error": error, + })) + return + cwd = get_jupyter_root_dir() + self.finish(json.dumps({ + "sessions": sessions, + "current_cwd": os.path.realpath(cwd) if cwd else "", + })) + except Exception as e: + log.exception("Failed to list ACP sessions") + self.set_status(500) + self.finish(json.dumps({"error": str(e)})) + + +class AcpSessionsResumeHandler(APIHandler): + """Resumes an ACP session via the agent's ``session/load``.""" + + @tornado.web.authenticated + async def post(self): + participant = _acp_participant_or_none() + if participant is None: + self.set_status(404) + self.finish(json.dumps({"error": "ACP mode is not enabled"})) + return + + try: + body = json.loads(self.request.body or b"{}") + except json.JSONDecodeError: + self.set_status(400) + self.finish(json.dumps({"error": "Request body must be JSON"})) + return + + session_id = body.get("session_id") + if not isinstance(session_id, str) or not session_id: + self.set_status(400) + self.finish(json.dumps({"error": "session_id is required"})) + return + + try: + loop = asyncio.get_event_loop() + error = await loop.run_in_executor( + None, participant.resume_session, session_id + ) + except Exception as e: + log.exception("Failed to resume ACP session %s", session_id) + self.set_status(500) + self.finish(json.dumps({"error": str(e)})) + return + if error: + self.set_status(500) + self.finish(json.dumps({"error": error})) + return + + self.finish(json.dumps({"success": True, "session_id": session_id})) + class ChatHistory: """ History of chat messages, key is chat id, value is list of messages @@ -2388,11 +2515,15 @@ def on_message(self, message): ) is_claude_code_mode = ai_service_manager.is_claude_code_mode + # ACP agents hold the conversation in their own session, exactly + # like the Claude SDK client, so they share Claude's context + # framing and per-turn history slicing below. + is_agent_session_mode = is_claude_code_mode or ai_service_manager.is_acp_mode chat_history = self.chat_history.get_history(chatId) chat_history_initial_size = len(chat_history) current_directory = data.get('currentDirectory') - if (is_claude_code_mode or chat_mode.id == 'agent') and current_directory is not None: + if (is_agent_session_mode or chat_mode.id == 'agent') and current_directory is not None: current_directory_file_msg = f"{NBI_CONTEXT_PREFIX} '{current_directory}'" if filename != '': current_directory_file_msg += f" and current file is: '{filename}'" @@ -2473,8 +2604,9 @@ def on_message(self, message): context_filename = path.basename(file_path) if is_image: - if is_claude_code_mode: - # Claude Code CLI takes text only; pass file path so agent can read the image + if is_agent_session_mode: + # Agent CLIs take text prompts only; pass the file + # path so the agent can read the image itself. chat_history.append({ "role": "user", "content": f"The user pasted an image. It is saved at this path: '{file_path}'. Please read and analyze it." @@ -2496,12 +2628,12 @@ def on_message(self, message): log.warning(f"Failed to read pasted image '{file_path}': {e}") continue - if is_claude_code_mode: + if is_agent_session_mode: # Hand the agent an @-mention rather than the file's - # contents: Claude's Read tool handles partial reads, - # notebook cell structure, and binary formats natively, - # and avoids the 80% context-window truncation the - # content-injection path would otherwise apply. + # contents: an agent's own read tool handles partial + # reads, notebook cell structure, and binary formats + # natively, and avoids the 80% context-window truncation + # the content-injection path would otherwise apply. if is_upload: mention_path = file_path else: @@ -2626,7 +2758,7 @@ def on_message(self, message): ) # last prompt is added later - request_chat_history = chat_history[chat_history_initial_size:-1] if is_claude_code_mode else chat_history[:-1] + request_chat_history = chat_history[chat_history_initial_size:-1] if is_agent_session_mode else chat_history[:-1] coro = ai_service_manager.handle_chat_request(ChatRequest(chat_mode=chat_mode, tool_selection=tool_selection, prompt=prompt, chat_history=request_chat_history, cancel_token=cancel_token, rule_context=rule_context, permission_mode=permission_mode), response_emitter) thread = threading.Thread(target=self._run_request_thread, args=(coro, messageId)) thread.start() @@ -2687,11 +2819,24 @@ def on_message(self, message): return handlers.response_emitter.on_user_input(msg['data']) elif messageType == RequestDataType.ClearChatHistory: - is_claude_code_mode = ai_service_manager.is_claude_code_mode - if is_claude_code_mode: + if ai_service_manager.is_claude_code_mode: default_chat_participant = ai_service_manager.default_chat_participant if isinstance(default_chat_participant, ClaudeCodeChatParticipant): default_chat_participant.clear_chat_history() + elif ai_service_manager.is_acp_mode: + default_chat_participant = ai_service_manager.default_chat_participant + clear = getattr(default_chat_participant, "clear_chat_history", None) + if callable(clear): + # The ACP session swap round-trips to the agent subprocess + # (up to the start timeout); run it off the websocket + # thread so a slow agent cannot stall the IO loop. A + # failed swap already forces a client restart; surface it + # in the log since there is no UI channel here. + def _clear_acp_session(): + error = clear() + if error: + log.warning("ACP new session failed: %s", error) + threading.Thread(target=_clear_acp_session, daemon=True).start() self.chat_history.clear() elif messageType == RequestDataType.RunUICommandResponse: handlers = self._messageCallbackHandlers.get(messageId) @@ -2950,26 +3095,27 @@ class NotebookIntelligence(ExtensionApp): config=True, ) - codex_mode_policy = TraitletEnum( + acp_mode_policy = TraitletEnum( list(VALID_POLICIES), default_value=POLICY_FORCE_OFF, help=""" - Org-wide policy for whether the experimental Codex (ACP) agent mode is - available (#378). Defaults to force-off; set to user-choice to let users - enable it. Overridden by the NBI_CODEX_MODE_POLICY env var. + Org-wide policy for whether the experimental ACP agent mode (Codex and + future Agent Client Protocol agents) is available (#378). Defaults to + force-off; set to user-choice to let users enable it. Overridden by + the NBI_ACP_MODE_POLICY env var. """, config=True, ) - codex_full_access_policy = TraitletEnum( + acp_full_access_policy = TraitletEnum( list(VALID_POLICIES), default_value=POLICY_FORCE_OFF, help=""" - Org-wide policy for Codex "full access" (#378): running tools - autonomously without asking. Defaults to force-off, so Codex is pinned - to ask before anything beyond trusted read-only commands. Set to - user-choice to let users opt into unattended runs, or force-on to - require it. Overridden by the NBI_CODEX_FULL_ACCESS_POLICY env var. + Org-wide policy for ACP agent "full access" (#378): running tools + autonomously without asking. Defaults to force-off, so the agent is + pinned to ask before anything beyond trusted read-only commands. Set + to user-choice to let users opt into unattended runs, or force-on to + require it. Overridden by the NBI_ACP_FULL_ACCESS_POLICY env var. """, config=True, ) @@ -3355,6 +3501,8 @@ def _setup_handlers(self, web_app, feature_policies: dict, string_overrides: dic route_pattern_upload_file = url_path_join(base_url, "notebook-intelligence", "upload-file") route_pattern_claude_sessions = url_path_join(base_url, "notebook-intelligence", "claude-sessions") route_pattern_claude_sessions_resume = url_path_join(base_url, "notebook-intelligence", "claude-sessions", "resume") + route_pattern_acp_sessions = url_path_join(base_url, "notebook-intelligence", "acp-sessions") + route_pattern_acp_sessions_resume = url_path_join(base_url, "notebook-intelligence", "acp-sessions", "resume") route_pattern_claude_mcp = url_path_join(base_url, "notebook-intelligence", "claude-mcp") route_pattern_claude_mcp_detail = url_path_join( base_url, "notebook-intelligence", "claude-mcp", r"(user|project|local)", r"([^/]+)" @@ -3500,6 +3648,8 @@ def _setup_handlers(self, web_app, feature_policies: dict, string_overrides: dic (route_pattern_upload_file, FileUploadHandler), (route_pattern_claude_sessions_resume, ClaudeSessionsResumeHandler), (route_pattern_claude_sessions, ClaudeSessionsListHandler), + (route_pattern_acp_sessions_resume, AcpSessionsResumeHandler), + (route_pattern_acp_sessions, AcpSessionsListHandler), # Claude-MCP routes: detail before list so {scope}/{name} doesn't # shadow specialized URLs added later (parallels the skills order). (route_pattern_claude_mcp_detail, ClaudeMCPDetailHandler), diff --git a/notebook_intelligence/feature_flags.py b/notebook_intelligence/feature_flags.py index 531d2fbc..0924a3b1 100644 --- a/notebook_intelligence/feature_flags.py +++ b/notebook_intelligence/feature_flags.py @@ -26,10 +26,10 @@ ("claude_api_key", "api_key"), ("claude_base_url", "base_url"), ) -CODEX_SETTINGS_OVERRIDES = ( - ("codex_chat_model", "chat_model"), - ("codex_api_key", "api_key"), - ("codex_base_url", "base_url"), +ACP_SETTINGS_OVERRIDES = ( + ("acp_chat_model", "chat_model"), + ("acp_api_key", "api_key"), + ("acp_base_url", "base_url"), ) @@ -92,21 +92,21 @@ def apply_member_policy(members: list, item: str, policy: str) -> list: return list(members) -def apply_codex_policies(codex_settings: dict, policies: dict) -> dict: - """Apply admin policies to a ``codex_settings`` dict (issue #378). +def apply_acp_policies(acp_settings: dict, policies: dict) -> dict: + """Apply admin policies to an ``acp_settings`` dict (issue #378). - Two gates: ``codex_mode`` clamps ``enabled``, and ``codex_full_access`` + Two gates: ``acp_mode`` clamps ``enabled``, and ``acp_full_access`` clamps ``full_access`` (the autonomous, run-without-asking posture, which defaults to force-off like Claude's bypass-permissions). Used on both the read path and the write-filter path, like ``apply_claude_policies``. """ - result = dict(codex_settings or {}) - mode_policy = policies.get("codex_mode", POLICY_USER_CHOICE) + result = dict(acp_settings or {}) + mode_policy = policies.get("acp_mode", POLICY_USER_CHOICE) if mode_policy == POLICY_FORCE_ON: result["enabled"] = True elif mode_policy == POLICY_FORCE_OFF: result["enabled"] = False - full_access_policy = policies.get("codex_full_access", POLICY_USER_CHOICE) + full_access_policy = policies.get("acp_full_access", POLICY_USER_CHOICE) if full_access_policy == POLICY_FORCE_ON: result["full_access"] = True elif full_access_policy == POLICY_FORCE_OFF: diff --git a/pyproject.toml b/pyproject.toml index 52023548..c7cebcbd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,10 +49,11 @@ dependencies = [ # BerriAI/litellm#25231 for the upstream fix. "mcp>=1.27.0", "claude-agent-sdk", - # Agent Client Protocol client SDK for Codex (and future ACP) agent mode - # (#378). Pinned because the protocol is pre-1.0 and the Python SDK still - # ships occasional breaking changes; imported lazily (only when Codex mode - # is used) per the #370 lazy-provider-import policy. + # Agent Client Protocol client SDK for ACP agent mode (Codex first, more + # agents as their adapters are validated, #378). Pinned because the + # protocol is pre-1.0 and the Python SDK still ships occasional breaking + # changes; imported lazily (only when ACP mode is used) per the #370 + # lazy-provider-import policy. "agent-client-protocol==0.10.1", "anthropic>=0.22.1", # Used to tear down an agent subprocess and its descendants (shells, MCP diff --git a/src/api.ts b/src/api.ts index 5be24b8a..b46d5cb9 100644 --- a/src/api.ts +++ b/src/api.ts @@ -267,8 +267,8 @@ export type FeaturePolicyName = | 'output_followup' | 'output_toolbar' | 'claude_mode' - | 'codex_mode' - | 'codex_full_access' + | 'acp_mode' + | 'acp_full_access' | 'claude_continue_conversation' | 'claude_code_tools' | 'claude_jupyter_ui_tools' @@ -299,9 +299,9 @@ export type SettingLockName = | 'claude_inline_completion_model' | 'claude_api_key' | 'claude_base_url' - | 'codex_chat_model' - | 'codex_api_key' - | 'codex_base_url'; + | 'acp_chat_model' + | 'acp_api_key' + | 'acp_base_url'; export type ISettingLocks = Record; @@ -394,24 +394,24 @@ export class NBIConfig { return this.capabilities.claude_settings; } - get codexSettings(): any { - return this.capabilities.codex_settings ?? {}; + get acpSettings(): any { + return this.capabilities.acp_settings ?? {}; } - // The agent modes that are enabled, in priority order; the frontend shows an - // agent picker when more than one is on. - get enabledAgentModes(): string[] { - return this.capabilities.enabled_agent_modes ?? []; + // The agent types selectable in ACP mode (settings dropdown). + get acpAgents(): { id: string; label: string }[] { + return this.capabilities.acp_agents ?? []; } - // The single agent mode currently handling chat (the user's pick when - // several are enabled), or null when the native provider path is in use. + // The single agent mode currently handling chat ('claude' | 'acp'), or + // null when the native provider path is in use. Claude mode and ACP mode + // are mutually exclusive. get activeAgentMode(): string | null { return this.capabilities.active_agent_mode ?? null; } - get isInCodexMode(): boolean { - return this.activeAgentMode === 'codex'; + get isInAcpMode(): boolean { + return this.activeAgentMode === 'acp'; } get spinnerVerbs(): { mode: string; verbs: string[] } | null { @@ -526,8 +526,8 @@ export class NBIConfig { 'output_followup', 'output_toolbar', 'claude_mode', - 'codex_mode', - 'codex_full_access', + 'acp_mode', + 'acp_full_access', 'claude_continue_conversation', 'claude_code_tools', 'claude_jupyter_ui_tools', @@ -587,9 +587,9 @@ export class NBIConfig { 'claude_inline_completion_model', 'claude_api_key', 'claude_base_url', - 'codex_chat_model', - 'codex_api_key', - 'codex_base_url' + 'acp_chat_model', + 'acp_api_key', + 'acp_base_url' ]; const result = {} as ISettingLocks; for (const name of names) { @@ -707,7 +707,7 @@ export class NBIAPI { static getChatEnabled() { return ( this.config.isInClaudeCodeMode || - this.config.isInCodexMode || + this.config.isInAcpMode || (this.config.chatModel.provider === GITHUB_COPILOT_PROVIDER_ID ? !this.getGHLoginRequired() : this.config.llmProviders.find( @@ -1453,6 +1453,31 @@ export class NBIAPI { }); } + static async listAcpSessions(): Promise { + interface IWireResponse { + sessions?: IClaudeSessionInfo[]; + current_cwd?: string; + error?: string; + } + const data = await requestAPI('acp-sessions', { + method: 'GET' + }); + if (data.error) { + throw new Error(data.error); + } + return { + sessions: data.sessions ?? [], + currentCwd: data.current_cwd ?? '' + }; + } + + static async resumeAcpSession(sessionId: string): Promise { + await requestAPI('acp-sessions/resume', { + method: 'POST', + body: JSON.stringify({ session_id: sessionId }) + }); + } + static async emitTelemetryEvent(event: ITelemetryEvent): Promise { const assistantMode = this.config.isInClaudeCodeMode ? AssistantMode.Claude diff --git a/src/chat-sidebar.tsx b/src/chat-sidebar.tsx index c53b9cbd..a94922e9 100644 --- a/src/chat-sidebar.tsx +++ b/src/chat-sidebar.tsx @@ -79,7 +79,6 @@ import { SafeAnchor } from './components/safe-anchor'; import { mcpServerSettingsToEnabledState } from './components/mcp-util'; import claudeSvgStr from '../style/icons/claude.svg'; import openaiSvgStr from '../style/icons/openai.svg'; -import { AgentSelect } from './components/agent-select'; import { AskUserQuestion } from './components/ask-user-question'; import { ClaudeSessionPicker } from './components/claude-session-picker'; import { @@ -1242,9 +1241,6 @@ function getActiveChatModel(): { provider: string; model: string } { } function SidebarComponent(props: any) { - const [activeAgent, setActiveAgent] = useState( - NBIAPI.config.activeAgentMode ?? '' - ); const [chatMessages, setChatMessages] = useState([]); const [prompt, setPrompt] = useState(''); const [draftPrompt, setDraftPrompt] = useState(''); @@ -1368,6 +1364,7 @@ function SidebarComponent(props: any) { const [workspaceFilesLoaded, setWorkspaceFilesLoaded] = useState(false); const [workspaceFilesLoading, setWorkspaceFilesLoading] = useState(false); const [showClaudeSessionPicker, setShowClaudeSessionPicker] = useState(false); + const [showAcpSessionPicker, setShowAcpSessionPicker] = useState(false); const [workspaceFilesError, setWorkspaceFilesError] = useState(''); const [workspaceScanLimitReached, setWorkspaceScanLimitReached] = useState(false); @@ -2308,7 +2305,6 @@ function SidebarComponent(props: any) { mcpServerSettingsRef.current ); setMCPServerEnabledState(newMcpServerEnabledState); - setActiveAgent(NBIAPI.config.activeAgentMode ?? ''); setRenderCount(renderCount => renderCount + 1); }; NBIAPI.configChanged.connect(handler); @@ -3308,11 +3304,10 @@ function SidebarComponent(props: any) { setChatId(UUID.uuid4()); }; - const handleClaudeSessionResumed = (session: IClaudeSessionInfo) => { - setShowClaudeSessionPicker(false); - // Reset local chat view so the user starts from a clean slate in the - // UI; the Claude Code backend retains the resumed transcript and will - // answer subsequent prompts with full prior context. + // Shared reset for both resume flows: the backend retains the resumed + // transcript (Claude reconnects with --resume; ACP replays via + // session/load), so the UI just resets to a clean slate with a notice. + const resetChatForResumedSession = (notice: string) => { setChatMessages([ { id: UUID.uuid4(), @@ -3322,9 +3317,7 @@ function SidebarComponent(props: any) { { id: UUID.uuid4(), type: ResponseStreamDataType.Markdown, - content: `Resumed Claude session \`${session.session_id.slice(0, 8)}\`${ - session.preview ? ` \u2014 _${session.preview}_` : '' - }.`, + content: notice, created: new Date() } ] @@ -3337,9 +3330,27 @@ function SidebarComponent(props: any) { resetPrefixSuggestions(); setPromptHistory([]); setPromptHistoryIndex(0); + }; + + const handleClaudeSessionResumed = (session: IClaudeSessionInfo) => { + setShowClaudeSessionPicker(false); + resetChatForResumedSession( + `Resumed Claude session \`${session.session_id.slice(0, 8)}\`${ + session.preview ? ` \u2014 _${session.preview}_` : '' + }.` + ); setPermissionMode(NBIAPI.config.claudePermissionDefaultMode); }; + const handleAcpSessionResumed = (session: IClaudeSessionInfo) => { + setShowAcpSessionPicker(false); + resetChatForResumedSession( + `Resumed session \`${session.session_id.slice(0, 8)}\`${ + session.preview ? `: _${session.preview}_` : '' + }.` + ); + }; + const onPromptKeyDown = async (event: KeyboardEvent) => { if (event.key === 'Enter' && !event.shiftKey) { event.stopPropagation(); @@ -4012,14 +4023,18 @@ function SidebarComponent(props: any) { {tourVisible && setTourVisible(false)} />}
Notebook Intelligence
- {NBIAPI.config.isInClaudeCodeMode && ( + {(NBIAPI.config.isInClaudeCodeMode || NBIAPI.config.isInAcpMode) && ( <> @@ -4047,19 +4066,6 @@ function SidebarComponent(props: any) {
- {NBIAPI.config.enabledAgentModes.length > 1 && ( -
- Agent - { - setActiveAgent(mode); - NBIAPI.setConfig({ active_chat_agent: mode }); - }} - /> -
- )}
{skillsReloadedVisible && (
@@ -4090,7 +4096,7 @@ function SidebarComponent(props: any) {
)} {!NBIAPI.config.isInClaudeCodeMode && - !NBIAPI.config.isInCodexMode && + !NBIAPI.config.isInAcpMode && ghLoginRequired && (
@@ -4377,7 +4383,7 @@ function SidebarComponent(props: any) {
{!NBIAPI.config.isInClaudeCodeMode && - !NBIAPI.config.isInCodexMode && ( + !NBIAPI.config.isInAcpMode && (
cannot render the per-agent SVG marks. Mirrors the keyboard / - * focus behavior of the permission-mode selector. - */ -export function AgentSelect(props: IAgentSelectProps): JSX.Element { - const [open, setOpen] = useState(false); - const buttonRef = useRef(null); - const menuRef = useRef(null); - const containerRef = useRef(null); - - useEffect(() => { - if (!open) { - return; - } - const handleClickOutside = (event: MouseEvent) => { - if (!containerRef.current?.contains(event.target as Node)) { - setOpen(false); - } - }; - document.addEventListener('mousedown', handleClickOutside); - return () => document.removeEventListener('mousedown', handleClickOutside); - }, [open]); - - // Land focus on the active item when the menu opens. - useEffect(() => { - if (open) { - menuRef.current - ?.querySelector('[aria-checked="true"]') - ?.focus(); - } - }, [open]); - - const closeMenu = (restoreFocus = true) => { - setOpen(false); - if (restoreFocus) { - buttonRef.current?.focus(); - } - }; - - const choose = (mode: string) => { - closeMenu(); - if (mode !== props.value) { - props.onChange(mode); - } - }; - - return ( -
- - {open && ( -
{ - if (event.key === 'Escape') { - event.stopPropagation(); - closeMenu(); - return; - } - if (event.key === 'ArrowDown' || event.key === 'ArrowUp') { - event.preventDefault(); - const items = Array.from( - menuRef.current?.querySelectorAll( - '[role="menuitemradio"]' - ) ?? [] - ); - const current = items.indexOf( - document.activeElement as HTMLButtonElement - ); - const delta = event.key === 'ArrowDown' ? 1 : -1; - const next = (current + delta + items.length) % items.length; - items[next]?.focus(); - } - }} - > - {props.agents.map(mode => { - const selected = props.value === mode; - return ( - - ); - })} -
- )} -
- ); -} diff --git a/src/components/claude-session-picker.tsx b/src/components/claude-session-picker.tsx index 7d59a731..ce386bce 100644 --- a/src/components/claude-session-picker.tsx +++ b/src/components/claude-session-picker.tsx @@ -16,6 +16,14 @@ export interface IClaudeSessionPickerProps { onResume: (session: IClaudeSessionInfo) => void; onClose: () => void; fetchSessions?: () => Promise; + // Resumes the picked session server-side before onResume fires. Overrides + // the default Claude resume call so the same picker serves ACP mode. + resumeSession?: (sessionId: string) => Promise; + title?: string; + emptyMessage?: string; + // The copy button emits a `claude --resume` shell command; hide it for + // agents that have no equivalent CLI invocation. + showCopyCommand?: boolean; } function formatTimestamp(epochSeconds: number): string { @@ -109,15 +117,20 @@ export function ClaudeSessionPicker( return; } setResuming(true); - // When a custom fetchSessions is provided the caller owns the resume - // lifecycle (e.g. the launcher tile opens a terminal directly), so skip - // the NBI sidebar API call which requires Claude Code mode to be active. - if (props.fetchSessions) { + // When a custom fetchSessions is provided without a resumeSession, the + // caller owns the resume lifecycle (e.g. the launcher tile opens a + // terminal directly), so skip the NBI sidebar API call which requires + // Claude Code mode to be active. + if (props.fetchSessions && !props.resumeSession) { props.onResume(session); return; } try { - await NBIAPI.resumeClaudeSession(session.session_id); + if (props.resumeSession) { + await props.resumeSession(session.session_id); + } else { + await NBIAPI.resumeClaudeSession(session.session_id); + } props.onResume(session); } catch (reason) { setError(String((reason as Error)?.message ?? reason ?? 'Unknown error')); @@ -142,7 +155,9 @@ export function ClaudeSessionPicker(
-
Resume Claude session
+
+ {props.title ?? 'Resume Claude session'} +
) : sessions.length === 0 ? (
- No previous Claude sessions found for this working directory. + {props.emptyMessage ?? + 'No previous Claude sessions found for this working directory.'}
) : (
    @@ -191,17 +207,21 @@ export function ClaudeSessionPicker( > {session.session_id.slice(0, 8)} - + {props.showCopyCommand !== false && ( + + )}
); diff --git a/src/components/settings-panel.tsx b/src/components/settings-panel.tsx index 7e00615e..7f1fcfed 100644 --- a/src/components/settings-panel.tsx +++ b/src/components/settings-panel.tsx @@ -171,20 +171,20 @@ const TABS: TabSpec[] = [ ) }, { - id: 'codex', - label: 'Codex', + id: 'acp', + label: 'ACP', icon: () => ( ), - // Hidden unless an admin has unlocked the experimental Codex mode - // (codex_mode defaults to force-off, which reads as locked + disabled). + // Hidden unless an admin has unlocked the experimental ACP mode + // (acp_mode defaults to force-off, which reads as locked + disabled). visible: ctx => - ctx.featurePolicies.codex_mode.enabled || - !ctx.featurePolicies.codex_mode.locked, - render: () => + ctx.featurePolicies.acp_mode.enabled || + !ctx.featurePolicies.acp_mode.locked, + render: () => }, { id: 'mcp-servers', @@ -1647,27 +1647,30 @@ function SettingsPanelComponentClaude(props: any) { ); } -function SettingsPanelComponentCodex(props: any) { +function SettingsPanelComponentAcp(props: any) { const nbiConfig = NBIAPI.config; - const [codexEnabled, setCodexEnabled] = useState( - nbiConfig.codexSettings?.enabled === true + const acpAgents = nbiConfig.acpAgents; + const [acpEnabled, setAcpEnabled] = useState( + nbiConfig.acpSettings?.enabled === true ); - const [chatModel, setChatModel] = useState( - nbiConfig.codexSettings.chat_model ?? '' + const [agent, setAgent] = useState( + nbiConfig.acpSettings.agent ?? acpAgents[0]?.id ?? 'codex' ); - const [apiKey, setApiKey] = useState(nbiConfig.codexSettings.api_key ?? ''); - const [baseUrl, setBaseUrl] = useState( - nbiConfig.codexSettings.base_url ?? '' + const [chatModel, setChatModel] = useState( + nbiConfig.acpSettings.chat_model ?? '' ); + const [apiKey, setApiKey] = useState(nbiConfig.acpSettings.api_key ?? ''); + const [baseUrl, setBaseUrl] = useState(nbiConfig.acpSettings.base_url ?? ''); const [fullAccess, setFullAccess] = useState( - nbiConfig.codexSettings.full_access === true + nbiConfig.acpSettings.full_access === true ); const { featurePolicies, settingLocks } = useNbiPolicies(); const syncSettingsToServerState = () => { NBIAPI.setConfig({ - codex_settings: { - enabled: codexEnabled, + acp_settings: { + enabled: acpEnabled, + agent: agent, chat_model: chatModel, api_key: apiKey, base_url: baseUrl, @@ -1678,22 +1681,28 @@ function SettingsPanelComponentCodex(props: any) { useEffect(() => { syncSettingsToServerState(); - }, [codexEnabled, chatModel, apiKey, baseUrl, fullAccess]); + }, [acpEnabled, agent, chatModel, apiKey, baseUrl, fullAccess]); return ( -
+
-
Enable Codex mode
+
Enable ACP mode
- Experimental. Routes chat through{' '} - - OpenAI Codex - {' '} - over the Agent Client Protocol. Requires an OpenAI API key and{' '} - npx available on the server. + Experimental. Routes chat through an external coding agent over + the{' '} + + Agent Client Protocol + + . Requires npx available on the server. ACP mode + and Claude mode are mutually exclusive: enabling one turns the + other off.
@@ -1701,25 +1710,22 @@ function SettingsPanelComponentCodex(props: any) {
{ - setCodexEnabled(!codexEnabled); + setAcpEnabled(!acpEnabled); }} >
- - By default Codex asks before anything beyond trusted read-only - commands. Full access lets it run tools (edits, shell) without - asking. Content the agent reads can steer what it runs. + + By default the agent asks before anything beyond trusted + read-only commands. Full access lets it run tools (edits, shell) + without asking. Content the agent reads can steer what it runs.
@@ -1727,15 +1733,13 @@ function SettingsPanelComponentCodex(props: any) {
{ setFullAccess(!fullAccess); }} @@ -1747,28 +1751,47 @@ function SettingsPanelComponentCodex(props: any) {
-
Model
+
Agent
+
+
+ Agent type +
+ +
Chat model (optional)
setChatModel(event.target.value)} />
@@ -1778,7 +1801,7 @@ function SettingsPanelComponentCodex(props: any) {
-
Codex account
+
Agent account
@@ -1787,17 +1810,17 @@ function SettingsPanelComponentCodex(props: any) { API Key (optional)
setApiKey(event.target.value)} />
@@ -1806,17 +1829,17 @@ function SettingsPanelComponentCodex(props: any) { Base URL (optional)
setBaseUrl(event.target.value)} />
diff --git a/src/index.ts b/src/index.ts index c4e1e09c..9cbaadae 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1589,7 +1589,7 @@ const plugin: JupyterFrontEndPlugin = { const isChatEnabled = (): boolean => { return ( NBIAPI.config.isInClaudeCodeMode || - NBIAPI.config.isInCodexMode || + NBIAPI.config.isInAcpMode || (NBIAPI.config.chatModel.provider === GITHUB_COPILOT_PROVIDER_ID ? !githubLoginRequired() : NBIAPI.config.chatModel.provider !== 'none') diff --git a/src/tour/tour-steps.ts b/src/tour/tour-steps.ts index d6a8dc5c..fdc783c2 100644 --- a/src/tour/tour-steps.ts +++ b/src/tour/tour-steps.ts @@ -219,8 +219,10 @@ export const ALL_TOUR_STEPS: readonly ITourStep[] = Object.freeze([ description: defaultText('chat-mode', 'description'), anchorId: TOUR_ANCHOR.chatMode, placement: 'top', - // Mode picker is hidden in Claude mode (Claude owns its own loop). - requires: () => !NBIAPI.config.isInClaudeCodeMode + // Mode picker is hidden in Claude and ACP modes (the agent owns its + // own loop), so skip the step there rather than target a missing anchor. + requires: () => + !NBIAPI.config.isInClaudeCodeMode && !NBIAPI.config.isInAcpMode }, { id: 'launcher-tiles', diff --git a/style/base.css b/style/base.css index 2d9fc088..8f6c560a 100644 --- a/style/base.css +++ b/style/base.css @@ -200,91 +200,6 @@ flex-grow: 1; } -.sidebar-agent-select { - display: flex; - align-items: center; - gap: 6px; - padding: 4px 10px 6px; -} - -.sidebar-agent-label { - font-size: 12px; - color: var(--jp-ui-font-color2); -} - -.agent-select-container { - position: relative; - display: flex; - flex-grow: 1; -} - -.agent-select-button { - display: flex; - align-items: center; - gap: 6px; - width: 100%; - box-sizing: border-box; - padding: 2px 6px; - cursor: pointer; - text-align: left; -} - -.agent-select-button-label { - flex-grow: 1; -} - -.agent-select-icon { - display: inline-flex; - flex-shrink: 0; -} - -.agent-select-icon svg { - width: 14px; - height: 14px; -} - -.agent-select-menu { - position: absolute; - top: 100%; - left: 0; - margin-top: 4px; - z-index: 100; - min-width: 160px; - padding: 4px; - background-color: var(--jp-layout-color1); - border: 1px solid var(--jp-border-color1); - border-radius: 4px; - box-shadow: 0 2px 8px rgb(0 0 0 / 30%); -} - -.agent-select-menu-item { - display: flex; - align-items: center; - gap: 6px; - width: 100%; - box-sizing: border-box; - padding: 4px 8px; - border: none; - background: none; - text-align: left; - cursor: pointer; - color: var(--jp-ui-font-color1); - font-size: var(--jp-ui-font-size1); - border-radius: 2px; -} - -.agent-select-menu-item:hover, -.agent-select-menu-item:focus-visible { - background-color: var(--jp-layout-color2); - outline: none; -} - -.agent-select-menu-check { - display: inline-flex; - width: 16px; - flex-shrink: 0; -} - .sidebar-messages { flex-grow: 1; overflow-y: auto; @@ -1546,7 +1461,7 @@ pre:has(.code-block-header) { color: #d97757; } -.codex-icon svg { +.acp-agent-icon svg { width: 16px; height: 16px; color: #0a84ff; @@ -3039,8 +2954,8 @@ svg.access-token-warning { height: 18px; } -/* The Codex footer badge matches the Claude badge size. */ -.chat-mode-widgets-container .codex-icon svg { +/* The ACP-agent footer badge matches the Claude badge size. */ +.chat-mode-widgets-container .acp-agent-icon svg { margin-top: 4px; width: 18px; height: 18px; diff --git a/tests/test_acp_agent.py b/tests/test_acp_agent.py index cf06647c..870af46a 100644 --- a/tests/test_acp_agent.py +++ b/tests/test_acp_agent.py @@ -1,6 +1,6 @@ # Copyright (c) Mehmet Bektas -"""Unit tests for the Codex/ACP backend mapping (issue #378, Phase 1). +"""Unit tests for the ACP backend mapping (issue #378, Phase 1). These exercise the editor-side translation (ACP events -> NBI cards/approval) without launching codex-acp; the live end-to-end path is covered by the @@ -43,7 +43,10 @@ def finish(self) -> None: def _client_with_response(resp): - owner = SimpleNamespace(current_response=resp) + owner = SimpleNamespace( + current_response=resp, + agent_spec=SimpleNamespace(label="Codex"), + ) return _NbiAcpClient(owner) @@ -105,15 +108,28 @@ def test_partial_update_merges_cached_kind(self): last = [d for d in resp.streamed if d.data_type == ResponseStreamDataType.ToolCall][-1] assert last.kind == "execute" and last.status == "completed" - def test_agent_message_chunk_streams_markdown(self): + def test_agent_message_chunk_streams_markdown_part(self): + # MarkdownPart, not Markdown: ACP delivers token-sized deltas and the + # frontend only concatenates consecutive *parts* into one block. With + # Markdown every delta rendered as its own paragraph (the one-word- + # per-line bug from the PR #380 review). resp = FakeResponse() client = _client_with_response(resp) asyncio.run(client.session_update("s", SimpleNamespace( session_update="agent_message_chunk", content=SimpleNamespace(text="hello world")))) - md = [d for d in resp.streamed if d.data_type == ResponseStreamDataType.Markdown] + md = [d for d in resp.streamed if d.data_type == ResponseStreamDataType.MarkdownPart] assert md and md[0].content == "hello world" + def test_agent_thought_chunk_streams_reasoning_part(self): + resp = FakeResponse() + client = _client_with_response(resp) + asyncio.run(client.session_update("s", SimpleNamespace( + session_update="agent_thought_chunk", + content=SimpleNamespace(text="mulling")))) + md = [d for d in resp.streamed if d.data_type == ResponseStreamDataType.MarkdownPart] + assert md and md[0].reasoning_content == "mulling" + class TestPermission: def _opts(self): @@ -165,34 +181,34 @@ def test_no_response_fails_closed(self): class TestPolicyClamp: def test_force_off_clamps_enabled(self): - from notebook_intelligence.feature_flags import apply_codex_policies - assert apply_codex_policies({"enabled": True}, {"codex_mode": "force-off"}) == {"enabled": False} + from notebook_intelligence.feature_flags import apply_acp_policies + assert apply_acp_policies({"enabled": True}, {"acp_mode": "force-off"}) == {"enabled": False} def test_user_choice_keeps_user_value(self): - from notebook_intelligence.feature_flags import apply_codex_policies - assert apply_codex_policies({"enabled": True}, {"codex_mode": "user-choice"}) == {"enabled": True} + from notebook_intelligence.feature_flags import apply_acp_policies + assert apply_acp_policies({"enabled": True}, {"acp_mode": "user-choice"}) == {"enabled": True} def test_full_access_force_off_clamps(self): - from notebook_intelligence.feature_flags import apply_codex_policies - out = apply_codex_policies( - {"full_access": True}, {"codex_full_access": "force-off"} + from notebook_intelligence.feature_flags import apply_acp_policies + out = apply_acp_policies( + {"full_access": True}, {"acp_full_access": "force-off"} ) assert out["full_access"] is False def test_full_access_user_choice_keeps_value(self): - from notebook_intelligence.feature_flags import apply_codex_policies + from notebook_intelligence.feature_flags import apply_acp_policies assert ( - apply_codex_policies( - {"full_access": True}, {"codex_full_access": "user-choice"} + apply_acp_policies( + {"full_access": True}, {"acp_full_access": "user-choice"} )["full_access"] is True ) def test_full_access_force_on(self): - from notebook_intelligence.feature_flags import apply_codex_policies + from notebook_intelligence.feature_flags import apply_acp_policies assert ( - apply_codex_policies( - {"full_access": False}, {"codex_full_access": "force-on"} + apply_acp_policies( + {"full_access": False}, {"acp_full_access": "force-on"} )["full_access"] is True ) @@ -211,6 +227,69 @@ def test_full_access_runs_unattended(self): assert codex_approval_args(True) == ["-c", 'approval_policy="never"'] +class TestAssembleQuery: + """The turn's context lines (attachments, current-file pointer, output + context) ride along with the prompt — sending only ``request.prompt`` + silently dropped whatever the user had just attached (the file-as-context + bug from the PR #380 review).""" + + def _assemble(self, chat_history, prompt="the prompt"): + from notebook_intelligence.acp_agent import AcpAgentClient + return AcpAgentClient.assemble_query( + SimpleNamespace(prompt=prompt, chat_history=chat_history) + ) + + def test_context_lines_precede_the_prompt(self): + query = self._assemble([ + {"role": "user", "content": "The user attached @data.csv."}, + {"role": "user", "content": "what is in this file?"}, + ]) + assert query == "The user attached @data.csv.\nwhat is in this file?" + + def test_empty_history_falls_back_to_prompt(self): + assert self._assemble([]) == "the prompt" + + def test_non_user_and_non_string_content_skipped(self): + query = self._assemble([ + {"role": "assistant", "content": "earlier answer"}, + {"role": "user", "content": [{"type": "text", "text": "structured"}]}, + {"role": "user", "content": "the prompt"}, + ]) + assert query == "the prompt" + + def test_trailing_slash_command_drops_context(self): + # Context lines are meaningless to a slash command and could break + # its parsing; mirrors the Claude-mode join. + query = self._assemble([ + {"role": "user", "content": "The user attached @data.csv."}, + {"role": "user", "content": "/compact"}, + ]) + assert query == "/compact" + + +class TestStripContextPreamble: + """Session previews should show the user's first question, not the NBI + context lines the agent stored as part of its session title.""" + + def test_strips_leading_context_lines(self): + from notebook_intelligence.acp_agent import _strip_context_preamble + title = ( + "Additional context: Current directory open in Jupyter is: '/w'\n" + "The user attached @facts.md. Read it if relevant.\n" + "What is the launch codename?" + ) + assert _strip_context_preamble(title) == "What is the launch codename?" + + def test_plain_title_unchanged(self): + from notebook_intelligence.acp_agent import _strip_context_preamble + assert _strip_context_preamble("say hi") == "say hi" + + def test_all_context_falls_back_to_original(self): + from notebook_intelligence.acp_agent import _strip_context_preamble + title = "Additional context: Current directory open in Jupyter is: '/w'" + assert _strip_context_preamble(title) == title + + class TestSingleFlight: """The ACP session runs one prompt at a time; a second concurrent turn must be rejected rather than interleave with the first.""" @@ -219,7 +298,7 @@ def _client(self): from notebook_intelligence.acp_agent import AcpAgentClient host = SimpleNamespace( websocket_connector=None, - nbi_config=SimpleNamespace(codex_settings={"enabled": True}), + nbi_config=SimpleNamespace(acp_settings={"enabled": True}), ) return AcpAgentClient(host) @@ -228,7 +307,7 @@ def test_second_concurrent_turn_is_rejected(self): # Simulate a turn already in flight by holding the turn lock. assert client._turn_lock.acquire(blocking=False) try: - req = SimpleNamespace(prompt="hi", cancel_token=None) + req = SimpleNamespace(prompt="hi", cancel_token=None, chat_history=[]) result = client.query(req, FakeResponse()) assert result is not None and "busy" in result.lower() finally: @@ -240,7 +319,7 @@ def test_unavailable_agent_releases_the_lock(self): # the lock (the outer finally). client._ensure_started = lambda: False client._start_error = "boom" - result = client.query(SimpleNamespace(prompt="x", cancel_token=None), FakeResponse()) + result = client.query(SimpleNamespace(prompt="x", cancel_token=None, chat_history=[]), FakeResponse()) assert result == "boom" assert client._turn_lock.acquire(blocking=False) client._turn_lock.release() @@ -271,7 +350,8 @@ def fake_schedule(coro, loop): mod.asyncio.run_coroutine_threadsafe = fake_schedule try: result = client.query( - SimpleNamespace(prompt="hi", cancel_token=None), FakeResponse() + SimpleNamespace(prompt="hi", cancel_token=None, chat_history=[]), + FakeResponse(), ) finally: mod.asyncio.run_coroutine_threadsafe = orig diff --git a/tests/test_ai_service_manager_integration.py b/tests/test_ai_service_manager_integration.py index 0c5a374f..b0da5a32 100644 --- a/tests/test_ai_service_manager_integration.py +++ b/tests/test_ai_service_manager_integration.py @@ -219,30 +219,45 @@ def test_no_fetch_when_claude_mode_disabled(self): assert mock_fetch.call_count == 0 -class TestResolveActiveAgent: - """The active-agent resolution that backs the in-chat agent picker (#378). - - Pure static logic, so it needs no AIServiceManager instance. +class TestActiveAgentMode: + """Active-agent resolution after the modes became mutually exclusive + (#378 review feedback): no preference, at most one enabled mode, Claude + wins as the safety net when a hand-edited config enables both. """ - def test_none_enabled_returns_none(self): - assert AIServiceManager.resolve_active_agent([], "claude") is None - assert AIServiceManager.resolve_active_agent([], None) is None + def _manager_with(self, claude_enabled: bool, acp_enabled: bool) -> AIServiceManager: + # active_agent_mode only reads the two settings dicts, so a bare + # instance with a stub config is enough — no participant/SDK setup. + manager = AIServiceManager.__new__(AIServiceManager) + mock_config = Mock() + mock_config.claude_settings = {"enabled": claude_enabled} + mock_config.acp_settings = {"enabled": acp_enabled} + manager._nbi_config = mock_config + return manager - def test_single_enabled_ignores_preference(self): - assert AIServiceManager.resolve_active_agent(["codex"], None) == "codex" - # A stale preference for a disabled mode is ignored. - assert AIServiceManager.resolve_active_agent(["codex"], "claude") == "codex" + def test_none_enabled_returns_none(self): + manager = self._manager_with(claude_enabled=False, acp_enabled=False) + assert manager.active_agent_mode is None + assert not manager.is_claude_code_mode + assert not manager.is_acp_mode - def test_preference_wins_when_enabled(self): - assert AIServiceManager.resolve_active_agent(["claude", "codex"], "codex") == "codex" - assert AIServiceManager.resolve_active_agent(["claude", "codex"], "claude") == "claude" + def test_claude_only(self): + manager = self._manager_with(claude_enabled=True, acp_enabled=False) + assert manager.active_agent_mode == "claude" + assert manager.is_claude_code_mode + assert not manager.is_acp_mode - def test_no_preference_falls_back_to_priority_order(self): - # Both enabled, no preference -> first (Claude), preserving the - # historical "Claude wins" default. - assert AIServiceManager.resolve_active_agent(["claude", "codex"], None) == "claude" - assert AIServiceManager.resolve_active_agent(["claude", "codex"], "") == "claude" + def test_acp_only(self): + manager = self._manager_with(claude_enabled=False, acp_enabled=True) + assert manager.active_agent_mode == "acp" + assert manager.is_acp_mode + assert not manager.is_claude_code_mode - def test_unknown_preference_falls_back(self): - assert AIServiceManager.resolve_active_agent(["claude", "codex"], "gemini") == "claude" + def test_both_enabled_claude_wins(self): + # ConfigHandler enforces exclusivity on every save, so both-enabled + # can only come from a hand-edited config file; the priority order + # keeps the historical "Claude wins" behavior for that case. + manager = self._manager_with(claude_enabled=True, acp_enabled=True) + assert manager.active_agent_mode == "claude" + assert manager.is_claude_code_mode + assert not manager.is_acp_mode diff --git a/tests/test_cell_output_features_response.py b/tests/test_cell_output_features_response.py index d27898ad..cadc3117 100644 --- a/tests/test_cell_output_features_response.py +++ b/tests/test_cell_output_features_response.py @@ -29,7 +29,7 @@ def _config( store_token=False, refresh_open_files=True, claude_settings=None, - codex_settings=None, + acp_settings=None, ): return SimpleNamespace( enable_explain_error=explain, @@ -38,7 +38,7 @@ def _config( store_github_access_token=store_token, refresh_open_files_on_disk_change=refresh_open_files, claude_settings=claude_settings if claude_settings is not None else {}, - codex_settings=codex_settings if codex_settings is not None else {}, + acp_settings=acp_settings if acp_settings is not None else {}, ) @@ -226,14 +226,14 @@ def test_presence_of_value_locks_the_field(self): # Unrelated keys remain unlocked. assert response["claude_chat_model"] == {"locked": False} - def test_codex_locks_track_their_env_overrides(self): + def test_acp_locks_track_their_env_overrides(self): response = _build_setting_locks_response( - {"codex_api_key": "sk-openai", "codex_base_url": ""} + {"acp_api_key": "sk-openai", "acp_base_url": ""} ) - assert response["codex_api_key"] == {"locked": True} + assert response["acp_api_key"] == {"locked": True} # Empty string is the user-choice signal, not a lock. - assert response["codex_base_url"] == {"locked": False} - assert response["codex_chat_model"] == {"locked": False} + assert response["acp_base_url"] == {"locked": False} + assert response["acp_chat_model"] == {"locked": False} def test_response_includes_every_known_lock_name(self): response = _build_setting_locks_response({}) @@ -246,9 +246,9 @@ def test_response_includes_every_known_lock_name(self): "claude_inline_completion_model", "claude_api_key", "claude_base_url", - "codex_chat_model", - "codex_api_key", - "codex_base_url", + "acp_chat_model", + "acp_api_key", + "acp_base_url", } @@ -267,16 +267,16 @@ def test_claude_key_scrubbed_when_env_locks_it(self): # The source dict is not mutated in place. assert settings["api_key"] == "sk-secret" - def test_codex_key_scrubbed_with_its_own_lock_name(self): + def test_acp_key_scrubbed_with_its_own_lock_name(self): settings = {"api_key": "sk-openai"} # The Claude lock must not scrub the Codex key. assert ( _scrub_credentials_for_wire( - settings, {"claude_api_key": "sk-env"}, "codex_api_key" + settings, {"claude_api_key": "sk-env"}, "acp_api_key" ) is settings ) scrubbed = _scrub_credentials_for_wire( - settings, {"codex_api_key": "sk-env"}, "codex_api_key" + settings, {"acp_api_key": "sk-env"}, "acp_api_key" ) assert scrubbed["api_key"] == "" diff --git a/tests/test_image_context.py b/tests/test_image_context.py index 3c821893..5ac7db92 100644 --- a/tests/test_image_context.py +++ b/tests/test_image_context.py @@ -108,6 +108,7 @@ def test_image_produces_multimodal_message(self, _thread, mock_nbi, mock_ai, tmp mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False image_bytes = b"\x89PNG\r\n\x1a\n" + b"\x00" * 16 img_file = tmp_path / "screenshot.png" @@ -131,6 +132,7 @@ def test_base64_payload_round_trips(self, _thread, mock_nbi, mock_ai, tmp_path): mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False image_bytes = b"\x89PNG\r\n\x1a\n" + b"\xde\xad\xbe\xef" * 8 img_file = tmp_path / "screenshot.png" @@ -148,6 +150,7 @@ def test_mime_type_reflected_in_data_url(self, _thread, mock_nbi, mock_ai, tmp_p mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False img_file = tmp_path / "photo.jpg" img_file.write_bytes(b"\xff\xd8\xff" + b"\x00" * 16) @@ -162,6 +165,7 @@ def test_missing_file_logs_warning_and_skips_entry(self, _thread, mock_nbi, mock mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False missing = tmp_path / "does_not_exist.png" @@ -177,6 +181,7 @@ def test_non_image_upload_uses_text_hint(self, _thread, mock_nbi, mock_ai, tmp_p mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False handler = _make_handler() binary_upload = { @@ -199,6 +204,7 @@ def test_mixed_image_and_text_context(self, _thread, mock_nbi, mock_ai, tmp_path mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False img_file = tmp_path / "shot.png" img_file.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 8) @@ -219,6 +225,7 @@ def test_image_context_not_charged_against_token_budget(self, _thread, mock_nbi, mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False img_file = tmp_path / "shot.png" img_file.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 8) @@ -235,6 +242,7 @@ def test_claude_code_mode_sends_file_path_not_base64(self, _thread, mock_nbi, mo mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = True + mock_ai.is_acp_mode = False img_file = tmp_path / "screenshot.png" img_file.write_bytes(b"\x89PNG\r\n\x1a\n" + b"\x00" * 16) @@ -251,6 +259,7 @@ def test_claude_code_mode_missing_file_still_adds_path_message(self, _thread, mo mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = True + mock_ai.is_acp_mode = False missing = tmp_path / "does_not_exist.png" @@ -271,6 +280,7 @@ def test_workspace_image_drag_reaches_vision_provider( mock_nbi.root_dir = str(tmp_path) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False image_bytes = b"\x89PNG\r\n\x1a\n" + b"\xfe\xed\xfa\xce" * 8 img_file = tmp_path / "diagram.png" @@ -304,6 +314,7 @@ def test_path_traversal_outside_workspace_is_rejected( mock_nbi.root_dir = str(workspace) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False ctx = { "isUpload": False, @@ -339,6 +350,7 @@ def test_absolute_path_outside_workspace_is_rejected( mock_nbi.root_dir = str(workspace) mock_ai.chat_model = None mock_ai.is_claude_code_mode = False + mock_ai.is_acp_mode = False ctx = { "isUpload": False, diff --git a/tests/test_websocket_handler_integration.py b/tests/test_websocket_handler_integration.py index f3ba3c03..971b99cf 100644 --- a/tests/test_websocket_handler_integration.py +++ b/tests/test_websocket_handler_integration.py @@ -226,6 +226,7 @@ def test_on_message_additional_context_includes_file_contents(self, mock_thread, mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = False + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 @@ -288,6 +289,7 @@ def test_on_message_claude_mode_emits_at_mention_not_contents( mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = True + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 @@ -355,6 +357,7 @@ def test_on_message_claude_mode_image_branch_unchanged( mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = True + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 @@ -421,6 +424,7 @@ def test_on_message_claude_mode_rejects_out_of_workspace_path( mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = True + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 @@ -476,6 +480,7 @@ def test_on_message_claude_mode_upload_non_image_uses_absolute_path( mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = True + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 @@ -533,6 +538,7 @@ def test_on_message_rejects_forged_upload_path_outside_upload_dir( mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = True + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 @@ -593,6 +599,7 @@ def test_on_message_claude_mode_rejects_control_char_filename( mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = True + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 @@ -666,6 +673,7 @@ def test_on_message_claude_mode_preserves_notebook_cell_pointer( mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = True + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 @@ -732,6 +740,7 @@ def test_on_message_claude_mode_preserves_selection_line_range( mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = True + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 @@ -796,6 +805,7 @@ def test_on_message_claude_mode_no_selection_no_range_pointer( mock_nb_intel.root_dir = "/workspace" mock_ai_manager.handle_chat_request = Mock() mock_ai_manager.is_claude_code_mode = True + mock_ai_manager.is_acp_mode = False mock_ai_manager.chat_model = Mock() mock_ai_manager.chat_model.context_window = 4096 From e1ce87413fbfbc332c26b6d3ba64de340ce4b280 Mon Sep 17 00:00:00 2001 From: PJ Doland Date: Fri, 10 Jul 2026 20:11:43 -0400 Subject: [PATCH 3/4] fix(acp): strip the joined-form context preamble from session previews Live verification caught the session picker still showing "Additional context: Current directory open in Jupyter is: ..." as previews: codex stores session titles with newlines collapsed to spaces (and truncated), so the line-based strip never matched. Peel the directory pointer off the joined form structurally by its quoted segments, keeping the line-based path for agents that preserve newlines. --- notebook_intelligence/acp_agent.py | 16 +++++++++++++++- tests/test_acp_agent.py | 18 ++++++++++++++++++ 2 files changed, 33 insertions(+), 1 deletion(-) diff --git a/notebook_intelligence/acp_agent.py b/notebook_intelligence/acp_agent.py index 2007e57b..a57b9a6a 100644 --- a/notebook_intelligence/acp_agent.py +++ b/notebook_intelligence/acp_agent.py @@ -27,6 +27,7 @@ import difflib import logging import os +import re import sys import threading import time @@ -260,6 +261,10 @@ def _strip_context_preamble(title: str) -> str: The agent titles a session with its first prompt, which NBI prefixes with context lines (current-directory pointer, attachments). The preview should show the user's actual first question, like the Claude picker does. + + Handles both shapes: newline-separated lines, and the joined form codex + stores (newlines collapsed to spaces, title truncated), where the + directory pointer is matched structurally by its quoted segments. """ from notebook_intelligence.claude_sessions import NBI_CONTEXT_PREFIX lines = [line for line in title.splitlines() if line.strip()] @@ -268,7 +273,16 @@ def _strip_context_preamble(title: str) -> str: or lines[0].startswith("The user attached ") ): lines.pop(0) - return " ".join(lines) if lines else title + stripped = " ".join(lines) + if stripped and stripped != title.strip(): + return stripped + # Joined form: peel the directory pointer off the front by shape. + joined_preamble = re.compile( + re.escape(NBI_CONTEXT_PREFIX) + + r" '[^']*'( and current file is: '[^']*')?\s*" + ) + remainder = joined_preamble.sub("", title, count=1) + return remainder.strip() or title def _diffs_from_content(content) -> list[dict]: diff --git a/tests/test_acp_agent.py b/tests/test_acp_agent.py index 870af46a..5514c50e 100644 --- a/tests/test_acp_agent.py +++ b/tests/test_acp_agent.py @@ -289,6 +289,24 @@ def test_all_context_falls_back_to_original(self): title = "Additional context: Current directory open in Jupyter is: '/w'" assert _strip_context_preamble(title) == title + def test_joined_form_strips_directory_pointer(self): + # codex stores titles with newlines collapsed to spaces (and + # truncated), so the pointer must be peeled off structurally. + from notebook_intelligence.acp_agent import _strip_context_preamble + title = ( + "Additional context: Current directory open in Jupyter is: '' " + "Reply with two short sentences." + ) + assert _strip_context_preamble(title) == "Reply with two short sentences." + + def test_joined_form_with_current_file(self): + from notebook_intelligence.acp_agent import _strip_context_preamble + title = ( + "Additional context: Current directory open in Jupyter is: '/w' " + "and current file is: 'nb.ipynb' What does this cell do?" + ) + assert _strip_context_preamble(title) == "What does this cell do?" + class TestSingleFlight: """The ACP session runs one prompt at a time; a second concurrent turn From 98727499e9335f8ae6f53cf9e00ea19b603a2263 Mon Sep 17 00:00:00 2001 From: PJ Doland Date: Mon, 20 Jul 2026 22:16:25 -0400 Subject: [PATCH 4/4] fix(acp): track Claude mode's slash-command join semantics Main's #388 changed the Claude-mode join so that only control-only commands (/clear, /compact, ...) drop the turn's context lines, while every other command is hoisted to the front with the context preserved as its arguments; the old drop-on-any-slash behavior silently discarded what the user had just attached. The ACP join still mirrored the old behavior. Adopt the same rule using the shared CONTROL_SLASH_COMMANDS set (claude_sessions is SDK-free, so the lazy-import posture of this module is unchanged), keeping the two agent modes' prompt assembly in parity. --- notebook_intelligence/acp_agent.py | 18 ++++++++++++++---- tests/test_acp_agent.py | 22 +++++++++++++++++++--- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/notebook_intelligence/acp_agent.py b/notebook_intelligence/acp_agent.py index a57b9a6a..3b5b8532 100644 --- a/notebook_intelligence/acp_agent.py +++ b/notebook_intelligence/acp_agent.py @@ -55,6 +55,7 @@ resolve_acp_agent_command, ) from notebook_intelligence.base_chat_participant import BaseChatParticipant +from notebook_intelligence.claude_sessions import CONTROL_SLASH_COMMANDS from notebook_intelligence.util import ThreadSafeWebSocketConnector, get_jupyter_root_dir log = logging.getLogger(__name__) @@ -636,9 +637,13 @@ def assemble_query(request: ChatRequest) -> str: @-mentions, current-file pointer, output context) to the chat history before the prompt, exactly like Claude mode. Sending only ``request.prompt`` would silently drop whatever the user just - attached. A trailing slash command drops the context lines instead -- - they are meaningless to commands like ``/compact`` and could break - their parsing (mirrors the Claude-mode join). + attached. Slash commands mirror Claude mode's join (#388): + + - control-only commands (``/clear``, ``/compact``, ...) drop the + context; it is meaningless to them and could break their parsing; + - every other command is moved to the front so the agent still + recognizes it, with the context lines preserved after it (a custom + command receives them as part of its arguments). """ query_lines = [] for msg in request.chat_history: @@ -649,7 +654,12 @@ def assemble_query(request: ChatRequest) -> str: if not query_lines: return request.prompt if query_lines[-1].startswith("/"): - query_lines = query_lines[-1:] + command_line = query_lines[-1] + command_token = command_line.split(None, 1)[0].lower() + if command_token in CONTROL_SLASH_COMMANDS or len(query_lines) == 1: + query_lines = [command_line] + else: + query_lines = [command_line, *query_lines[:-1]] return "\n".join(line.strip() for line in query_lines) def query(self, request: ChatRequest, response: ChatResponse) -> Optional[str]: diff --git a/tests/test_acp_agent.py b/tests/test_acp_agent.py index 5514c50e..c0c337b2 100644 --- a/tests/test_acp_agent.py +++ b/tests/test_acp_agent.py @@ -257,15 +257,31 @@ def test_non_user_and_non_string_content_skipped(self): ]) assert query == "the prompt" - def test_trailing_slash_command_drops_context(self): - # Context lines are meaningless to a slash command and could break - # its parsing; mirrors the Claude-mode join. + def test_control_slash_command_drops_context(self): + # Context lines are meaningless to a control command and could break + # its parsing; mirrors the Claude-mode join after #388. query = self._assemble([ {"role": "user", "content": "The user attached @data.csv."}, {"role": "user", "content": "/compact"}, ]) assert query == "/compact" + def test_custom_slash_command_keeps_context_after_the_command(self): + # A non-control command is hoisted to the front (the agent only + # recognizes a command at the start of the prompt) with the turn's + # context preserved as its arguments; mirrors Claude mode's #388 join. + query = self._assemble([ + {"role": "user", "content": "The user attached @data.csv."}, + {"role": "user", "content": "/analyze"}, + ]) + assert query == "/analyze\nThe user attached @data.csv." + + def test_bare_custom_command_with_no_context_stays_clean(self): + query = self._assemble( + [{"role": "user", "content": "/analyze"}], prompt="/analyze" + ) + assert query == "/analyze" + class TestStripContextPreamble: """Session previews should show the user's first question, not the NBI