diff --git a/README.md b/README.md index 6e0d2a48..e6c74306 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 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. @@ -202,24 +202,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_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: @@ -244,6 +246,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 85ea4f51..db274b64 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_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,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 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 ACP agent (#378) + +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_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_ACP_MODE_POLICY=force-off` unless the agent is explicitly needed. + +Known limitations: + +- 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 ```python @@ -721,6 +736,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 new file mode 100644 index 00000000..3b5b8532 --- /dev/null +++ b/notebook_intelligence/acp_agent.py @@ -0,0 +1,817 @@ +# Copyright (c) Mehmet Bektas + +"""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 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/``): + +- 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 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 concurrent.futures +import difflib +import logging +import os +import re +import sys +import threading +import time +from datetime import datetime +from typing import Any, Optional + +import acp +from acp import schema + +from notebook_intelligence.api import ( + ChatCommand, + ChatRequest, + 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.claude_sessions import CONTROL_SLASH_COMMANDS +from notebook_intelligence.util import ThreadSafeWebSocketConnector, get_jupyter_root_dir + +log = logging.getLogger(__name__) + +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 + +_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 + + +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: + # 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(MarkdownPartData(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?" + agent_label = self._owner.agent_spec.label + resp.stream(ConfirmationData( + title=f"{agent_label} tool call", + message=( + f"Approve: {title}?\n\n" + f"{agent_label} 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 _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. + + 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()] + while lines and ( + lines[0].startswith(NBI_CONTEXT_PREFIX) + or lines[0].startswith("The user attached ") + ): + lines.pop(0) + 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]: + 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 agent-adapter 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._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 agent_spec(self) -> AcpAgentSpec: + return resolve_acp_agent(self.acp_settings.get("agent")) + + 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-acp-agent", daemon=True + ) + self._thread.start() + if not self._started.wait(timeout=_START_TIMEOUT): + self._start_error = self._start_error or "ACP 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"ACP 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() + 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, + 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"), + ) + self._agent_capabilities = getattr(init, "agent_capabilities", None) + await self._authenticate(init) + sess = await self._conn.new_session( + cwd=workdir, mcp_servers=self._mcp_servers() + ) + self._session_id = sess.session_id + log.info("ACP agent session ready: %s", self._session_id) + except Exception as e: + self._start_error = f"ACP 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 _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._api_key(spec) + if api_key: + 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 + 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("ACP agent 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("acp-agent: %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._agent_capabilities = 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("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. 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: + 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("/"): + 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]: + """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 f"{self.agent_spec.label} is busy with another request" + try: + if not self._ensure_started(): + return self._start_error or f"{self.agent_spec.label} agent is not available" + loop = self._loop + if loop is None: + 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: + self._client._tool_state.clear() + self.current_response = response + try: + fut = asyncio.run_coroutine_threadsafe( + self._run_prompt(self.assemble_query(request)), loop + ) + except RuntimeError as e: + return f"{self.agent_spec.label} 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("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 f"{self.agent_spec.label} 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 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 ACP_AGENT_CHAT_PARTICIPANT_ID + + @property + def name(self) -> str: + return self._spec.label + + @property + def description(self) -> str: + return self._spec.description + + @property + def icon_path(self) -> str: + return self._spec.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. + + 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 "" + + 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"**{self._spec.label} agent error:** {result}")) + except Exception as e: + log.error("Error handling ACP 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/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 f303a8a6..ac4f6c96 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._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 ACP mode is active so the ACP SDK + # import stays off the startup path (#378). + 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) @@ -221,8 +226,33 @@ def update_models_from_config(self): else: self.unregister_chat_participant(self._claude_code_chat_participant) + # 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_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_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._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) @@ -317,9 +347,41 @@ def inline_completion_model(self) -> InlineCompletionModel: def embedding_model(self) -> EmbeddingModel: return self._embedding_model + # 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 == "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)] + + @property + def active_agent_mode(self) -> Optional[str]: + """The single agent mode currently handling chat. + + Returns None when no agent mode is enabled (the native provider path + handles chat). + """ + enabled = self.enabled_agent_modes + return enabled[0] if enabled else None + @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_acp_mode(self) -> bool: + return self.active_agent_mode == "acp" @property def claude_models(self) -> list[dict]: @@ -450,13 +512,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_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() 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_acp_mode: + prompt_parts = PromptParts(input=request.prompt, participant=self._ensure_acp_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..33008660 100644 --- a/notebook_intelligence/config.py +++ b/notebook_intelligence/config.py @@ -10,9 +10,11 @@ from notebook_intelligence.util import get_claude_config_dir from notebook_intelligence.feature_flags import ( + ACP_SETTINGS_OVERRIDES, CHAT_MODEL_OVERRIDES, CLAUDE_SETTINGS_OVERRIDES, INLINE_COMPLETION_MODEL_OVERRIDES, + apply_acp_policies, apply_claude_policies, apply_string_overrides, ) @@ -172,6 +174,15 @@ def claude_settings(self): resolved, self._string_overrides, CLAUDE_SETTINGS_OVERRIDES ) + @property + def acp_settings(self): + resolved = apply_acp_policies( + self.get('acp_settings', {}), self._feature_policies + ) + return apply_string_overrides( + resolved, self._string_overrides, ACP_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 e1b6213e..1a3757d4 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, + ACP_SETTINGS_OVERRIDES, INLINE_COMPLETION_MODEL_OVERRIDES, JUPYTER_UI_TOOLS_ID, POLICY_FORCE_OFF, @@ -39,11 +40,13 @@ POLICY_USER_CHOICE, VALID_POLICIES, apply_claude_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, @@ -313,6 +316,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"), + ("acp_mode", "NBI_ACP_MODE_POLICY", "acp_mode_policy"), + ( + "acp_full_access", + "NBI_ACP_FULL_ACCESS_POLICY", + "acp_full_access_policy", + ), ( "claude_continue_conversation", "NBI_CLAUDE_CONTINUE_CONVERSATION_POLICY", @@ -378,13 +387,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, which must fail closed. +# 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["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. +# 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"), @@ -394,6 +408,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"), + ("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) @@ -406,6 +423,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 {} + acp_settings = nbi_config.acp_settings or {} tools = claude_settings.get("tools") or [] sources = claude_settings.get("setting_sources") or [] @@ -414,6 +432,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 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) ), @@ -432,8 +456,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, @@ -482,15 +506,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 the Codex ACP agent); 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 @@ -616,6 +643,17 @@ def is_provider_enabled(provider_id: str) -> bool: "claude_settings": _scrub_credentials_for_wire( nbi_config.claude_settings, self.string_overrides ), + "acp_settings": _scrub_credentials_for_wire( + nbi_config.acp_settings, self.string_overrides, "acp_api_key" + ), + # 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, # Drive launcher-tile visibility (issues #183, #260). Each flag @@ -690,6 +728,7 @@ def post(self): "inline_completion_debouncer_delay", "mcp_server_settings", "claude_settings", + "acp_settings", "enable_explain_error", "enable_output_followup", "enable_output_toolbar", @@ -721,6 +760,15 @@ def post(self): has_model_change = False has_claude_settings_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 @@ -757,6 +805,24 @@ def post(self): if self.string_overrides.get("claude_api_key"): value = dict(value) value["api_key"] = "" + elif key == "acp_settings": + value = apply_acp_policies(value, self.feature_policies) + value = apply_string_overrides( + value, self.string_overrides, ACP_SETTINGS_OVERRIDES + ) + # OPENAI_API_KEY is a credential; same handling as Claude above. + 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 acp_settings property, + # which re-injects env overrides such as OPENAI_API_KEY and would + # never match the scrubbed value we persist). + 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": if value: @@ -777,14 +843,57 @@ 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: + if ( + has_model_change + or has_claude_settings_change + or has_acp_settings_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_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_acp_client() self.finish(json.dumps({})) @@ -1910,6 +2019,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 @@ -2339,11 +2539,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}'" @@ -2436,8 +2640,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." @@ -2459,12 +2664,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: @@ -2590,7 +2795,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, language=language, kernel_name=kernel_name, 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() @@ -2653,11 +2858,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) @@ -2916,6 +3134,31 @@ class NotebookIntelligence(ExtensionApp): config=True, ) + acp_mode_policy = TraitletEnum( + list(VALID_POLICIES), + default_value=POLICY_FORCE_OFF, + help=""" + 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, + ) + + acp_full_access_policy = TraitletEnum( + list(VALID_POLICIES), + default_value=POLICY_FORCE_OFF, + help=""" + 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, + ) + claude_continue_conversation_policy = TraitletEnum( list(VALID_POLICIES), default_value=POLICY_USER_CHOICE, @@ -3297,6 +3540,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"([^/]+)" @@ -3442,6 +3687,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 05e1ca27..0924a3b1 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"), ) +ACP_SETTINGS_OVERRIDES = ( + ("acp_chat_model", "chat_model"), + ("acp_api_key", "api_key"), + ("acp_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_acp_policies(acp_settings: dict, policies: dict) -> dict: + """Apply admin policies to an ``acp_settings`` dict (issue #378). + + 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(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("acp_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..c7cebcbd 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -49,6 +49,12 @@ dependencies = [ # BerriAI/litellm#25231 for the upstream fix. "mcp>=1.27.0", "claude-agent-sdk", + # 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 # servers, dev servers) on cancel; see util.terminate_process_tree. Already diff --git a/src/api.ts b/src/api.ts index c85e692f..d8ae0681 100644 --- a/src/api.ts +++ b/src/api.ts @@ -267,6 +267,8 @@ export type FeaturePolicyName = | 'output_followup' | 'output_toolbar' | 'claude_mode' + | 'acp_mode' + | 'acp_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' + | 'acp_chat_model' + | 'acp_api_key' + | 'acp_base_url'; export type ISettingLocks = Record; @@ -389,6 +394,26 @@ export class NBIConfig { return this.capabilities.claude_settings; } + get acpSettings(): any { + return this.capabilities.acp_settings ?? {}; + } + + // 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 ('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 isInAcpMode(): boolean { + return this.activeAgentMode === 'acp'; + } + 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', + 'acp_mode', + 'acp_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', + 'acp_chat_model', + 'acp_api_key', + 'acp_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.isInAcpMode || (this.config.chatModel.provider === GITHUB_COPILOT_PROVIDER_ID ? !this.getGHLoginRequired() : this.config.llmProviders.find( @@ -1428,6 +1459,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 524a7639..6c3a06d7 100644 --- a/src/chat-sidebar.tsx +++ b/src/chat-sidebar.tsx @@ -78,6 +78,7 @@ 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 { AskUserQuestion } from './components/ask-user-question'; import { ClaudeSessionPicker } from './components/claude-session-picker'; import { @@ -1374,6 +1375,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); @@ -3315,11 +3317,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(), @@ -3329,9 +3330,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() } ] @@ -3344,9 +3343,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(); @@ -4024,14 +4041,18 @@ function SidebarComponent(props: any) { {tourVisible && setTourVisible(false)} />}
Notebook Intelligence
- {NBIAPI.config.isInClaudeCodeMode && ( + {(NBIAPI.config.isInClaudeCodeMode || NBIAPI.config.isInAcpMode) && ( <> @@ -4088,31 +4113,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.isInAcpMode && + ghLoginRequired && ( +
+
+ You are not logged in to GitHub Copilot. Please login now to + activate chat. +
+
+ - + +
-
- )} + )} {chatEnabled && (chatMessages.length === 0 ? ( @@ -4373,45 +4400,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.isInAcpMode && ( +
- )} - {chatMode !== 'ask' && !NBIAPI.config.isInClaudeCodeMode && ( - - )} + + {selectedToolCount > 0 && <>{selectedToolCount}} + + )} {NBIAPI.config.isInClaudeCodeMode && ( )} + {NBIAPI.config.isInAcpMode && ( + + )}
+ {props.showCopyCommand !== false && ( + + )}
); diff --git a/src/components/settings-panel.tsx b/src/components/settings-panel.tsx index 4bececaa..088fbf09 100644 --- a/src/components/settings-panel.tsx +++ b/src/components/settings-panel.tsx @@ -7,6 +7,7 @@ import * as path from 'path'; import copySvgstr from '../../style/icons/copy.svg'; import claudeSvgStr from '../../style/icons/claude.svg'; +import openaiSvgStr from '../../style/icons/openai.svg'; import { ClaudeModelType, ClaudeToolType, @@ -169,6 +170,22 @@ const TABS: TabSpec[] = [ /> ) }, + { + id: 'acp', + label: 'ACP', + icon: () => ( + + ), + // 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.acp_mode.enabled || + !ctx.featurePolicies.acp_mode.locked, + render: () => + }, { id: 'mcp-servers', label: 'MCP Servers', @@ -1171,7 +1188,7 @@ function SettingsPanelComponentClaude(props: any) { const claudeSettingsRef = useRef(nbiConfig.claudeSettings); const [_renderCount, setRenderCount] = useState(1); const [claudeEnabled, setClaudeEnabled] = useState( - nbiConfig.isInClaudeCodeMode + nbiConfig.claudeSettings?.enabled === true ); const [chatModel, setChatModel] = useState( nbiConfig.claudeSettings.chat_model ?? ClaudeModelType.Default @@ -1655,3 +1672,208 @@ function SettingsPanelComponentClaude(props: any) { ); } + +function SettingsPanelComponentAcp(props: any) { + const nbiConfig = NBIAPI.config; + const acpAgents = nbiConfig.acpAgents; + const [acpEnabled, setAcpEnabled] = useState( + nbiConfig.acpSettings?.enabled === true + ); + const [agent, setAgent] = useState( + nbiConfig.acpSettings.agent ?? acpAgents[0]?.id ?? 'codex' + ); + 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.acpSettings.full_access === true + ); + const { featurePolicies, settingLocks } = useNbiPolicies(); + + const syncSettingsToServerState = () => { + NBIAPI.setConfig({ + acp_settings: { + enabled: acpEnabled, + agent: agent, + chat_model: chatModel, + api_key: apiKey, + base_url: baseUrl, + full_access: fullAccess + } + }); + }; + + useEffect(() => { + syncSettingsToServerState(); + }, [acpEnabled, agent, chatModel, apiKey, baseUrl, fullAccess]); + + return ( +
+
+
+
Enable ACP mode
+
+
+ + 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. + +
+
+
+
+ { + setAcpEnabled(!acpEnabled); + }} + > +
+
+
+
+ + 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. + +
+
+
+
+ { + setFullAccess(!fullAccess); + }} + > +
+
+
+
+
+ +
+
Agent
+
+
+
+
+
+ Agent type +
+ +
+
+
+ Chat model (optional) +
+ setChatModel(event.target.value)} + /> +
+
+
+
+
+ +
+
Agent 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 55ee7ea0..e104fc93 100644 --- a/src/index.ts +++ b/src/index.ts @@ -1665,6 +1665,7 @@ const plugin: JupyterFrontEndPlugin = { const isChatEnabled = (): boolean => { return ( NBIAPI.config.isInClaudeCodeMode || + 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 9a61ea06..8f6c560a 100644 --- a/style/base.css +++ b/style/base.css @@ -1461,6 +1461,12 @@ pre:has(.code-block-header) { color: #d97757; } +.acp-agent-icon svg { + width: 16px; + height: 16px; + color: #0a84ff; +} + .send-button { display: flex; align-items: center; @@ -1659,6 +1665,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 +2954,13 @@ svg.access-token-warning { height: 18px; } +/* 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; +} + .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..c0c337b2 --- /dev/null +++ b/tests/test_acp_agent.py @@ -0,0 +1,398 @@ +# Copyright (c) Mehmet Bektas + +"""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 +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, + agent_spec=SimpleNamespace(label="Codex"), + ) + 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_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.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): + 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_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_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_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_acp_policies + assert ( + 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_acp_policies + assert ( + apply_acp_policies( + {"full_access": False}, {"acp_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 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_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 + 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 + + 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 + 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(acp_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, chat_history=[]) + 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, chat_history=[]), 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, chat_history=[]), + 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..b0da5a32 100644 --- a/tests/test_ai_service_manager_integration.py +++ b/tests/test_ai_service_manager_integration.py @@ -217,3 +217,47 @@ def test_no_fetch_when_claude_mode_disabled(self): ): manager.update_models_from_config() assert mock_fetch.call_count == 0 + + +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 _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_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_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_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_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 725fea88..cadc3117 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, + acp_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 {}, + acp_settings=acp_settings if acp_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_acp_locks_track_their_env_overrides(self): + response = _build_setting_locks_response( + {"acp_api_key": "sk-openai", "acp_base_url": ""} + ) + assert response["acp_api_key"] == {"locked": True} + # Empty string is the user-choice signal, not a lock. + 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({}) 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", + "acp_chat_model", + "acp_api_key", + "acp_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_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"}, "acp_api_key" + ) + is settings + ) + scrubbed = _scrub_credentials_for_wire( + 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 dc5aab40..a5b6cf86 100644 --- a/tests/test_websocket_handler_integration.py +++ b/tests/test_websocket_handler_integration.py @@ -236,6 +236,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 @@ -298,6 +299,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 @@ -365,6 +367,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 @@ -431,6 +434,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 @@ -486,6 +490,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 @@ -543,6 +548,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 @@ -603,6 +609,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 @@ -676,6 +683,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 @@ -742,6 +750,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 @@ -806,6 +815,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